Skip to content

feat: add Landing AI integration for cloud auto-labeling#129

Open
vusallyv wants to merge 13 commits intodeveloper0hye:masterfrom
vusallyv:feat/landing-ai
Open

feat: add Landing AI integration for cloud auto-labeling#129
vusallyv wants to merge 13 commits intodeveloper0hye:masterfrom
vusallyv:feat/landing-ai

Conversation

@vusallyv
Copy link
Copy Markdown
Contributor

@vusallyv vusallyv commented Feb 25, 2026

Summary

Extends the cloud auto-label feature with a second provider: Landing AI. Users can select the model from a combo box in the AI Settings tab.

Depends on PR #128 (feat: add cloud auto-label via yololabel.com), which is now merged into master. This branch is rebased on top of current master.

Key changes (delta over #128)

  • Model combo in the AI Settings tab: YoloLabel AI (default) and Landing AI
  • syncAiSettingsTab() and saveAiSettings() switch between providers' keys based on selection; Landing AI key stored as "landingApiKey" in QSettings
  • checkLandingConsent() — one-time privacy notice before images are uploaded to api.va.landing.ai (separate from the yololabel.com consent)
  • submitLandingAIJob(), doLandingAIJob(), landingAIAutoLabelAll(), landingAIProcessNextInQueue() — full single-image and all-images flows
  • Cancel button shown for both single and batch Landing AI operations; cancelAutoLabel() sets m_landingCancelled and clears the queue
  • Retry logic: up to 3 retries on network failure (matching CloudAutoLabeler::MAX_RETRIES)
  • CloudAutoLabeler::mimeForImage() reused for PNG/BMP/TIFF/WebP/JPEG Content-Type
  • CloudAutoLabeler::backupLabelFile() called before overwriting any label file
  • save_label_data() + ui->label_image->saveState() called before labeling so Ctrl+Z undo works
  • QImageReader.size() used to get image dimensions without decoding pixels; falls back to QImage only if size query fails
  • Full JSON validation: data[0] checked as array, bounding_box values checked as numeric, coordinates validated in [0,1] range
  • Progress counter shows done/total where done = completed images (in-flight not counted as done)

Test plan

  • Select "Landing AI" from the model combo — privacy notice appears on first use only
  • Enter a Landing AI API key, click Save; switch back to YoloLabel AI — key field shows the yololabel.com key
  • Click "Auto Label AI" → verify detections and label file written correctly; Ctrl+Z undoes
  • Click "Auto Label All AI" → verify progress counter and labels for all images; Cancel stops the run
  • Submit with empty key → verify prompt to enter key in AI Settings tab
  • Simulate network error → verify retry message and eventual error dialog after 3 tries

🤖 Generated with Claude Code

@vusallyv vusallyv force-pushed the feat/landing-ai branch 3 times, most recently from e466750 to ba12606 Compare February 25, 2026 12:55
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Thanks for adding Landing AI support! However, this PR has a critical structural issue that needs to be resolved before it can be merged.

Root Cause: Branch not rebased on merged #128

This branch was forked from feat/cloud-auto-label before PR #128 went through its 4 review rounds. The first commit (514ac52) contains the pre-review version of #128's code, re-introducing bugs that were already fixed during #128's review process. Since #128 is now merged into master, this branch needs to be rebased on current master so only the Landing AI delta remains.

Once rebased, the three BLOCKER regressions below will disappear automatically.


BLOCKER — #128 review regressions (will be fixed by rebase)

# Issue Detail
1 Session restore deleted saveSession() / restoreLastSession() are removed entirely. App restart loses previous session. (PR #127 feature, fixed in #128 round 3)
2 Progress display 0-based set_label_progress changed from fileIndex+1 / size to fileIndex / size-1 — shows "0 / 4" instead of "1 / 5". (Fixed in #128 round 3)
3 Homebrew ONNX include path removed YoloLabel.pro drops include/onnxruntime subdirectory support — breaks macOS Homebrew builds. (Fixed in #128 round 3)

CRITICAL — Landing AI code issues

4. #include <QDir> removed from mainwindow.cpp
The PR removes #include <QDir> but QDir::currentPath() is still used in on_loadModel_clicked(). This relies on transitive includes and is fragile.

5. No structural validation of Landing AI response

  • dataVal.toArray().first().toArray() — no check that first() is actually an array
  • No range check on normalized coordinates (cx, cy, nw, nh could be negative or > 1)
  • No validation that bounding_box values are numeric

HIGH

6. No Cancel button for Landing AI batch
landingAIAutoLabelAll() never calls m_btnCancelAutoLabel->setVisible(true). Users cannot cancel a Landing AI batch operation. Compare with cloudAutoLabelAll() which shows it.

7. No upload consent for Landing AI
The yololabel.com path has a one-time privacy notice (checkUploadConsent()). Landing AI uploads images to api.va.landing.ai without any consent prompt. Users should be informed before their images are sent to a third-party service.

8. No backup before overwrite
CloudAutoLabeler calls backupLabelFile() before writing labels. doLandingAIJob() directly opens the file with WriteOnly and overwrites without creating a .bak backup.

9. No retry logic
doLandingAIJob() shows a QMessageBox and clears the queue on the first network failure. CloudAutoLabeler retries up to 3 times (MAX_RETRIES).

10. MIME type only covers PNG/JPEG
doLandingAIJob() only checks .png and defaults to image/jpeg. CloudAutoLabeler::mimeForImage() also handles BMP, TIFF, and WebP. Consider reusing that helper.

11. No saveState() call
Landing AI single-image labeling doesn't call saveState(), so Ctrl+Z (undo) won't work after a cloud label. cloudAutoLabelAll() calls save_label_data() — Landing AI should match.


MEDIUM

12. Dead member m_landingPendingImagePath — Set in doLandingAIJob() but never read anywhere.

13. Shared prompt semantics — Both providers share m_cloudPrompt, but yololabel.com sends it as a single semicolon-separated string while Landing AI splits it into separate prompts multipart fields. Switching providers with the same prompt could produce unexpected results.

14. Progress off-by-one — In landingAIProcessNextInQueue(), done = m_imgList.size() - m_landingQueue.size() counts the current (in-flight) image as "done" before it finishes.


Architecture suggestion

The Landing AI logic is implemented directly in MainWindow (~200 lines of networking code), which is the same pattern that was refactored out of MainWindow into CloudAutoLabeler during #128's first review round. Consider either:

  • Extending CloudAutoLabeler with a provider abstraction (enum or strategy) so both yololabel.com and Landing AI share the same cancel/retry/backup/consent infrastructure, or
  • Creating a separate LandingAILabeler class following the same QObject + signals pattern as CloudAutoLabeler

Either approach would eliminate the duplicate button-management code in MainWindow and automatically give Landing AI the same robustness (retry, cancel, backup, consent) that CloudAutoLabeler already has.


Recommended next steps

  1. Rebase on current master — this resolves all 3 BLOCKERs
  2. Fix the CRITICAL and HIGH issues listed above
  3. Consider the architecture suggestion to reduce duplication

Happy to re-review once rebased!

vusallyv and others added 2 commits February 26, 2026 18:40
Extends the AI Settings tab with a model selector (YoloLabel AI /
Landing AI) so users can choose between the yololabel.com API and
the Landing AI text-to-object-detection endpoint.

Key changes:
- QComboBox model selector added to AI Settings tab
- syncAiSettingsTab() / saveAiSettings() are model-aware
- submitLandingAIJob() / doLandingAIJob(): POST to Landing AI endpoint
  with image + prompt; parse bbox response and write YOLO .txt files
- QImageReader used to get image dimensions for coordinate normalization
  (falls back to QImage if size() returns invalid)
- Full JSON + bounds validation on Landing AI responses
- landingAIAutoLabelAll() / landingAIProcessNextInQueue() for batch flow
- m_landingNet: dedicated QNetworkAccessManager for Landing AI requests
- Saves/restores landingApiKey under "YoloLabel"/"CloudAI" QSettings key

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Rebased onto current master (resolves the 3 BLOCKERs: session
restore, progress display, and Homebrew ONNX path — all fixed in
the upstream merge of PR developer0hye#128).

CRITICAL:
- QDir include already present after rebase (no change needed)
- Validate that data[0] is an array before indexing (firstVal.isArray())
- Check bounding_box values are numeric (isDouble())
- Skip boxes with coords outside [0,1] or non-positive size

HIGH:
- Add checkLandingConsent() — one-time privacy notice for Landing AI
  (separate from checkUploadConsent() used by yololabel.com)
- Show m_btnCancelAutoLabel in both submitLandingAIJob() and
  landingAIAutoLabelAll(); cancelAutoLabel() now also sets
  m_landingCancelled=true and clears m_landingQueue
- Call CloudAutoLabeler::backupLabelFile() before overwriting label file
- Add retry logic: up to MAX_LANDING_RETRIES=3 on network failure
- Use CloudAutoLabeler::mimeForImage() for PNG/BMP/TIFF/WebP/JPEG
- Call save_label_data() and ui->label_image->saveState() in
  submitLandingAIJob() so Ctrl+Z undo works after single-image labeling

MEDIUM:
- Remove dead member m_landingPendingImagePath (set but never read)
- Fix progress off-by-one: show done-1/total while image is in-flight

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Both static helpers are called from mainwindow.cpp but were declared
private, causing C2248 access errors on MSVC and equivalent errors on
GCC/Clang. Move them to the public section.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great work addressing all the issues from the previous review! The rebase is clean, JSON validation is solid, and the consent/retry/backup/undo flows are all in place. Two remaining issues need fixing before merge.


HIGH — Must fix before merge

1. Image slider not disabled during Landing AI batch

cloudAutoLabelAll() disables the slider via CloudAutoLabeler::busyChanged:

connect(m_cloudLabeler, &CloudAutoLabeler::busyChanged, this, [this](bool busy) {
    ui->horizontalSlider_images->setEnabled(!busy);
});

Landing AI bypasses CloudAutoLabeler, so the slider (and A/D key navigation) remains active during batch processing. This is the same race condition fixed in PR #128 round 2 ("Batch + A/D navigation race"), now reintroduced for the Landing AI path.

Fix: Disable the slider in submitLandingAIJob() and landingAIAutoLabelAll(), re-enable in resetCloudButtons() (or wherever the Landing AI flow completes).

2. File write failure silently ignored

In doLandingAIJob():

if (lf.open(QIODevice::WriteOnly | QIODevice::Text)) {
    // ... write labels ...
    lf.close();
}
// if open() fails → no error, silent continue

This was flagged as CRITICAL in PR #128 round 4 ("File write failure silent success") and fixed in CloudAutoLabeler. The Landing AI path doesn't have the same fix.

Fix: Show an error (e.g., QMessageBox::warning or statusBar()->showMessage) when the file can't be opened for writing.


MEDIUM — Strongly recommended

3. Label matching is case-sensitive and exact

int classId = m_objList.indexOf(label);
if (classId < 0) continue;  // silently skipped

If Landing AI returns "Person" but the class file has "person", all detections are silently dropped. The user sees "0 detections" with no explanation.

Suggestion: Use case-insensitive matching, or at minimum log skipped labels to the status bar (e.g., "Landing AI: 3 detections skipped (unknown labels: Person, Car)").

4. No upload size warning for Landing AI batch

cloudAutoLabelAll() warns when estimated upload exceeds 500 MB. landingAIAutoLabelAll() doesn't have this check. Since Landing AI processes one image at a time (vs. batches of 20), the bandwidth impact is actually higher for large datasets.


LOW / Nit

5. Magic number 1 for Landing AI combo index — Used in 4+ places (currentIndex() == 1). An enum or named constant would improve readability.

6. QNetworkAccessManager transitive includemainwindow.h declares QNetworkAccessManager *m_landingNet but relies on the include from cloud_labeler.h. A forward declaration would be safer.

7. setBody() vs setBodyDevice() — PR #128 used setBodyDevice() to halve peak memory. Landing AI uses f.readAll() + setBody(). Minor since it's one image at a time, but worth aligning for consistency.


What looks good

  • Clean rebase — no conflicts with current master
  • QImageReader.size() for dimensions without decoding pixels
  • Full JSON validation (firstVal.isArray(), isDouble(), coordinate range checks)
  • Separate consent dialog for Landing AI privacy notice
  • 3-retry logic matching CloudAutoLabeler::MAX_RETRIES
  • backupLabelFile() before overwriting
  • save_label_data() + saveState() for Ctrl+Z undo support
  • Progress counter correctly accounts for in-flight image (done = total - remaining - 1)

Recommended next steps

  1. Fix HIGH #1 and #2 — required for merge
  2. Address MEDIUM #3 (label matching) — strongly recommended
  3. The rest can be follow-up PRs

… round 2)

Fix the two HIGH issues flagged in the second review round:

1. Disable image slider during Landing AI operations to prevent the A/D
   navigation race condition that was already fixed for the yololabel.com
   path via CloudAutoLabeler::busyChanged. Slider is disabled in
   submitLandingAIJob() and landingAIAutoLabelAll(), and re-enabled in
   resetCloudButtons().

2. Handle file write failure in doLandingAIJob(): when lf.open() fails,
   show a status bar message instead of silently continuing.

Also address MEDIUM developer0hye#3: when Landing AI returns labels not found in the
class list, collect the unknown label names and report them to the status
bar (e.g. "detections skipped (unknown labels: Person, Car)") so users
understand why detections were dropped.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict Review of PR #129 — Landing AI Integration

CI passes (all 3 platforms + DCO). The rebase is clean and the round 2 fixes look good. However, comparing this PR with the robustness standards established in PR #128 reveals several remaining issues.


CRITICAL — Must fix before merge

1. A/D keyboard navigation race condition during Landing AI operations

next_img() and prev_img() only guard against CloudAutoLabeler:

void MainWindow::next_img(bool bSavePrev)
{
    if (m_cloudLabeler && m_cloudLabeler->isBusy()) return;  // ← Landing AI not checked
    if(bSavePrev && ui->label_image->isOpened()) save_label_data();
    goto_img(m_imgIndex + 1);
}

During a Landing AI batch, m_cloudLabeler->isBusy() is false. The slider is correctly disabled, but A/D keyboard shortcuts bypass the slider and call next_img()/prev_img() directly. This means:

  • User presses A/D during batch → save_label_data() fires for the current on-screen image
  • The Landing AI callback is simultaneously writing labels for a different queued image
  • If the user navigates to an image that's currently being processed, both paths can write to the same .txt file

This is the exact same race condition fixed in PR #128 round 2 ("Batch + A/D navigation race"). The slider disable covers part of it, but the keyboard shortcut path needs a Landing AI busy guard.

Fix: Add a check for Landing AI busy state in next_img()/prev_img():

void MainWindow::next_img(bool bSavePrev)
{
    if (m_cloudLabeler && m_cloudLabeler->isBusy()) return;
    if (!m_landingQueue.isEmpty() || m_landingCancelled == false /* during in-flight job */) return;
    ...
}

Or better: introduce a single m_landingBusy bool that is set/cleared alongside button state.

2. No stale callback protection (generation counter missing)

CloudAutoLabeler uses m_generation to invalidate in-flight callbacks when the user cancels:

// CloudAutoLabeler pattern:
int gen = ++m_generation;
connect(reply, &QNetworkReply::finished, this, [this, gen, ...]() {
    if (gen != m_generation) return;  // stale → discard
    ...
});

The Landing AI path has no equivalent. Scenario:

  1. User clicks "Auto Label AI" → doLandingAIJob() sends request, connects lambda
  2. User clicks Cancel → m_landingCancelled = true
  3. User immediately clicks "Auto Label AI" again → m_landingCancelled = false, new request sent
  4. Old reply's lambda firesm_landingCancelled is now false → lambda proceeds and writes label file for the old image with stale data

This was explicitly fixed in PR #128 round 2 ("CRITICAL: Stale callbacks after cancel") with a generation counter. The Landing AI path needs the same protection.


HIGH — Should fix before merge

3. saveState() not called in landingAIAutoLabelAll()

Single-image path correctly calls both:

void MainWindow::submitLandingAIJob() {
    save_label_data();
    ui->label_image->saveState();  // ✓ Ctrl+Z will work
    ...
}

Batch path only calls one:

void MainWindow::landingAIAutoLabelAll() {
    save_label_data();
    // ← missing ui->label_image->saveState()
    ...
}

Without saveState(), Ctrl+Z cannot undo the batch labeling. This was flagged as HIGH in PR #128 round 3 ("HIGH 1: No undo after cloud label").

4. No early validation when class file is not loaded

If m_objList is empty (user hasn't loaded a class file), Landing AI will process all images, receive detections with label names like "person", "car", etc., and then silently drop ALL of them because m_objList.indexOf(label) returns -1 for every label.

The user sees "Landing AI: 0 detections" or "Landing AI: detections skipped (unknown labels: ...)" after waiting for the network round-trip. This is confusing.

For YoloLabel AI, CloudAutoLabeler::setClasses() is called before submission and the server uses the class list directly. For Landing AI, the class list is critical for mapping label names → class IDs.

Fix: Add an early check in submitLandingAIJob() and landingAIAutoLabelAll():

if (m_objList.isEmpty()) {
    QMessageBox::information(this, "Landing AI",
        "Please load a class file first — Landing AI returns label names\n"
        "that must be mapped to your class list.");
    return;
}

5. Missing forward declaration for QNetworkAccessManager in mainwindow.h

mainwindow.h declares QNetworkAccessManager *m_landingNet but relies on the transitive include from cloud_labeler.h. If cloud_labeler.h ever changes its includes, mainwindow.h would fail to compile. Add either #include <QNetworkAccessManager> or a forward declaration class QNetworkAccessManager;.


MEDIUM — Strongly recommended

6. Inconsistent behavior for empty vs. filtered-out detections

  • Server returns empty data array → old label file preserved (early return at line 1527, no file write)
  • Server returns detections that are ALL filtered out (bad coords, unknown labels) → backup created, file truncated to 0 bytes (file opened at line 1564, nothing written)

This inconsistency means a model returning "no detections" preserves labels, but a model returning detections that all fail validation destroys them. Both should behave the same way — either both preserve or both overwrite.

7. Case-sensitive label matching still not addressed

Previous review (round 2, MEDIUM #3) recommended case-insensitive matching. Current code:

int classId = m_objList.indexOf(label);  // exact, case-sensitive

If Landing AI returns "Person" but the class file has "person", the detection is silently dropped. The skippedLabels reporting (added in round 2 fix) mitigates the UX, but a case-insensitive fallback would be more robust:

int classId = m_objList.indexOf(label);
if (classId < 0) {
    // Case-insensitive fallback
    for (int i = 0; i < m_objList.size(); ++i) {
        if (m_objList[i].compare(label, Qt::CaseInsensitive) == 0) {
            classId = i; break;
        }
    }
}

8. Model selection not persisted across restarts

The combo box always starts at index 0 ("YoloLabel AI"). If a user primarily uses Landing AI, they have to reselect it every launch. Save/restore via QSettings like the API keys.

9. Magic number 1 for Landing AI combo index

Used in 4+ places:

if (m_modelCombo->currentIndex() == 1) submitLandingAIJob();
if (m_modelCombo->currentIndex() == 1) landingAIAutoLabelAll();
bool isLanding = (m_modelCombo->currentIndex() == 1);  // x2

An enum would improve readability:

enum CloudProvider { ProviderYoloLabel = 0, ProviderLandingAI = 1 };

LOW / Nit

10. Missing explicit includes in mainwindow.cppQHttpMultiPart, QHttpPart, QJsonDocument, QJsonObject, QJsonArray are used but not included (they come transitively from cloud_labeler.h). Adding them makes the dependency explicit and protects against future refactors.

11. setBody() vs setBodyDevice() — PR #128 used setBodyDevice() to halve peak memory per image. Landing AI uses f.readAll() + setBody(). Minor since it's one image at a time, but worth aligning for consistency.

12. Architecture note (reiteration from round 1) — ~300 lines of networking code directly in MainWindow duplicates the cancel/retry/backup/consent infrastructure already in CloudAutoLabeler. This is the same pattern refactored out during PR #128's first review round. A provider abstraction in CloudAutoLabeler or a separate LandingAILabeler class would eliminate the duplication and automatically inherit the robustness of the existing infrastructure (generation counter, busy state, etc.). Not blocking, but every new bug above (stale callbacks, navigation race, saveState) exists precisely because this code reimplements what CloudAutoLabeler already does correctly.


What looks good

  • Clean rebase on current master — no regressions from PR #128
  • QImageReader.size() for dimensions without decoding pixels — efficient
  • Full JSON validation: firstVal.isArray(), isDouble() checks, coordinate range validation
  • Separate consent dialog for Landing AI (distinct from yololabel.com notice)
  • 3-retry logic matching CloudAutoLabeler::MAX_RETRIES
  • backupLabelFile() before overwriting label files
  • save_label_data() + saveState() for single-image undo support
  • Progress counter correctly accounts for in-flight image (done = total - remaining - 1)
  • Unknown label reporting to status bar (skippedLabels)
  • File write failure detection with status bar message
  • Slider disabled during Landing AI operations

Summary

Severity Count Blocking?
CRITICAL 2 Yes
HIGH 3 Yes (3, 4); Recommended (5)
MEDIUM 4 No
LOW 3 No

The two CRITICAL issues (#1 navigation race, #2 stale callbacks) are bugs that were explicitly fixed in PR #128 and are re-introduced here because the Landing AI path reimplements networking outside of CloudAutoLabeler. Fix those plus the HIGH items, and this is ready to merge.

CRITICAL:
- Add m_landingBusy flag; check it in next_img()/prev_img() to block
  A/D keyboard navigation during Landing AI batch operations (same
  race condition fixed in PR developer0hye#128 round 2 for CloudAutoLabeler)
- Add m_landingGeneration counter; increment on cancel and new job
  start; capture gen in doLandingAIJob() lambda and discard stale
  callbacks when gen != m_landingGeneration

HIGH:
- Add ui->label_image->saveState() in landingAIAutoLabelAll() so
  Ctrl+Z undo works after batch labeling
- Add early m_objList.isEmpty() guard in submitLandingAIJob() and
  landingAIAutoLabelAll() to prevent silent drop of all detections
  when no class file is loaded
- Add #include <QNetworkAccessManager> to mainwindow.h to remove
  reliance on transitive include from cloud_labeler.h

MEDIUM:
- Collect valid detections into outputLines before opening the label
  file; only write (and backup) if non-empty — makes "all detections
  filtered" behave consistently with "no detections" (both preserve
  the existing file)
- Add case-insensitive fallback in label matching so Landing AI
  returning "Person" correctly maps to class "person"
- Persist model combo selection to QSettings (key "modelCombo") in
  saveAiSettings(); restore in initSideTabWidget() after combo init
- Replace magic number 1 with CloudProvider enum (ProviderYoloLabel,
  ProviderLandingAI) in all four usage sites

LOW:
- Add explicit includes: QHttpMultiPart, QHttpPart, QJsonArray,
  QJsonDocument, QJsonObject, QNetworkReply, QNetworkRequest
- Switch doLandingAIJob() image upload from readAll()+setBody() to
  setBodyDevice() with QFile parented to multiPart (consistent with
  CloudAutoLabeler, avoids redundant in-memory copy)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
…loper0hye#4)

Mirrors the ≥500MB upload warning already present in cloudAutoLabelAll():
compute total byte size across all images before the confirmation dialog;
if it exceeds 500 MB, append the estimated size and a bandwidth notice
to the confirmation message.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict Review — PR #129: Landing AI Integration (Round 4)

The latest two commits (301520a, 80ef2f4) addressed most of the round 3 issues well — generation counter, m_landingBusy guard, saveState() in batch, m_objList.isEmpty() guard, explicit includes, setBodyDevice(), case-insensitive matching, CloudProvider enum, model combo persistence, and upload size warning. CI passes on all 3 platforms.

However, after cross-referencing with the robustness standards established in PR #128 (which went through 6 rounds of review), there are still issues that need attention. The root cause is architectural: ~350 lines of networking code reimplemented directly in MainWindow, duplicating infrastructure that CloudAutoLabeler already provides. Every bug below exists because the Landing AI path re-implements what CloudAutoLabeler handles correctly.


CRITICAL 1: cancelAutoLabel() fails to reset buttons when only Landing AI is running

mainwindow.cppcancelAutoLabel(), cloud_labeler.cpp:81-86

cancelAutoLabel() does not call resetCloudButtons() directly. It relies on m_cloudLabeler->cancel() emitting busyChanged(false), which triggers the connected lambda:

// Line 155-159: busyChanged handler
connect(m_cloudLabeler, &CloudAutoLabeler::busyChanged, this, [this](bool busy) {
    ui->horizontalSlider_images->setEnabled(!busy);
    if (!busy) resetCloudButtons();
});

But CloudAutoLabeler::setBusy() has a guard:

// cloud_labeler.cpp:81-86
void CloudAutoLabeler::setBusy(bool busy) {
    if (m_busy == busy) return;   // ← Already false? No signal emitted!
    m_busy = busy;
    emit busyChanged(busy);
}

Scenario:

  1. User selects Landing AI, clicks "Auto Label All" → m_landingBusy = true, buttons disabled, slider disabled. CloudAutoLabeler is not busy (m_busy == false).
  2. User clicks Cancel → cancelAutoLabel() calls m_cloudLabeler->cancel()setBusy(false) → guard fires (false == false) → no signal emittedresetCloudButtons() never called.
  3. The in-flight Landing AI callback fires → gen != m_landingGeneration → returns immediately without calling resetCloudButtons().
  4. Result: Buttons stuck in disabled/cancel state permanently. The user must quit and restart the app.

Required fix: Call resetCloudButtons() directly in cancelAutoLabel():

void MainWindow::cancelAutoLabel()
{
    ++m_landingGeneration;
    m_cloudLabeler->cancel();
    m_landingCancelled = true;
    m_landingQueue.clear();
    resetCloudButtons();  // ← Ensure buttons always reset
}

Note: the busyChanged(false) handler also calls resetCloudButtons(), so when CloudAutoLabeler IS busy, it will be called twice. resetCloudButtons() is idempotent (sets fixed states), so double-calling is safe.


CRITICAL 2: doLandingAIJob() stale-callback path calls resetCloudButtons() on cancel but NOT on generation mismatch — inconsistent cleanup

mainwindow.cppdoLandingAIJob() lambda

connect(reply, &QNetworkReply::finished, this, [this, reply, imagePath, retryCount, gen]() {
    reply->deleteLater();
    if (gen != m_landingGeneration) return;        // ← Path A: just return, no cleanup
    if (m_landingCancelled) {
        resetCloudButtons();                        // ← Path B: cleanup
        return;
    }
    ...
});

And at the top of doLandingAIJob():

void MainWindow::doLandingAIJob(const QString &imagePath, int retryCount, int gen)
{
    if (gen != m_landingGeneration || m_landingCancelled) {
        resetCloudButtons();                        // ← Path C: cleanup (synchronous call)
        return;
    }
    ...
}

Path A (gen mismatch in async lambda) just returns without any cleanup. This is correct IF the caller of cancel/new-job has already reset the buttons. But as shown in CRITICAL 1, cancelAutoLabel() does NOT reset buttons directly. So:

  • Cancel → ++m_landingGeneration → stale lambda fires → Path A → no cleanup → buttons stuck

Path C (synchronous check) is only reached when landingAIProcessNextInQueue() calls doLandingAIJob() and the generation was bumped in between. This correctly cleans up. But Path A (the more common async case) does not.

This is the same issue as CRITICAL 1 — fixing CRITICAL 1 by adding resetCloudButtons() to cancelAutoLabel() will also fix this path. But it's worth noting the inconsistency for maintainability.


HIGH 1: Division by zero when image dimensions cannot be determined

mainwindow.cppdoLandingAIJob() lambda

QImageReader reader(imagePath);
QSize imgSize = reader.size();
if (!imgSize.isValid() || imgSize.isEmpty())
    imgSize = QImage(imagePath).size();
double imgW = imgSize.width();
double imgH = imgSize.height();
// ...
double cx = ((x1 + x2) / 2.0) / imgW;  // imgW could be 0

If both QImageReader and QImage fail (corrupted file, unsupported format, permission denied), imgSize remains invalid/empty. imgW and imgH would be 0, causing division by zero in the coordinate normalization. While IEEE 754 would produce infinity (filtered by the range check), this is technically undefined behavior in C++.

Required fix: Guard against zero dimensions:

if (imgW <= 0 || imgH <= 0) {
    statusBar()->showMessage("Landing AI: could not read image dimensions: " + imagePath, 5000);
    if (m_landingQueue.isEmpty()) resetCloudButtons();
    else                          landingAIProcessNextInQueue();
    return;
}

HIGH 2: Stale callback from single-image can interfere with subsequent batch

The generation counter is incremented in landingAIAutoLabelAll() and submitLandingAIJob(), but consider this edge case:

  1. User clicks "Auto Label AI" (single image) → gen = ++m_landingGeneration (gen=1)
  2. Network is slow, reply hasn't arrived yet
  3. User clicks "Auto Label All AI" → ++m_landingGeneration (gen=2), batch starts processing
  4. Old single-image reply arrives → gen(1) != m_landingGeneration(2) → returns immediately ✓

This case IS correctly handled by the generation counter. Good.

But: submitLandingAIJob() sets m_landingBusy = true and m_landingCancelled = false. If the old reply from step 2 is still in flight, and the user starts a new single-image job (not a batch), the old callback sees gen != m_landingGeneration and returns. Fine.

Actually, on closer inspection, there's a subtler issue: submitLandingAIJob() does not clear m_landingQueue. If a batch was cancelled (queue cleared by cancelAutoLabel) and then a single image is submitted, this is fine. But if the user rapidly clicks "Auto Label AI" multiple times, each call creates a new callback. The generation counter handles this correctly. OK, this one is fine.

Downgrading to informational — generation counter correctly protects this case.


HIGH 3: MAX_LANDING_RETRIES scoped as function-local static — inconsistent with CloudAutoLabeler

void MainWindow::doLandingAIJob(const QString &imagePath, int retryCount, int gen)
{
    ...
    static constexpr int MAX_LANDING_RETRIES = 3;
    ...
}

MAX_LANDING_RETRIES is defined inside the function body. It works correctly but:

  • Not accessible from other functions (e.g., submitLandingAIJob() if retry logic is ever needed at the submission level)
  • Inconsistent with CloudAutoLabeler::MAX_RETRIES which is a class-level constant
  • A static constexpr inside a function is fine semantically, but it looks like it was meant to be a class-level constant matching the CloudAutoLabeler pattern

Suggested fix: Move to a private constant in MainWindow or match CloudAutoLabeler::MAX_RETRIES directly.


MEDIUM 1: Architecture concern — networking code in MainWindow (reiteration, non-blocking)

The initial review of PR #128 identified "636 lines added directly to mainwindow.cpp" as the first issue and extracted all cloud logic into CloudAutoLabeler. PR #129 adds ~350 lines of networking code back into MainWindow. The result:

  • Cancel/retry/backup/consent/generation counter logic is duplicated rather than shared
  • 5 of the 6 commits in this PR are bug fixes for issues that CloudAutoLabeler already handles correctly
  • Future providers would each add another ~300 lines to MainWindow

A provider abstraction in CloudAutoLabeler (or a separate LandingAILabeler class following the same QObject+signals pattern) would:

  • Eliminate CRITICAL 1 (cancel would go through the same setBusy path)
  • Eliminate the class of bugs where Landing AI path misses safety mechanisms
  • Make future providers trivial to add

Not blocking merge, but strongly recommended as a follow-up refactor before adding any more providers.


MEDIUM 2: Batch "done" count can confuse users

landingAIProcessNextInQueue():

int done  = m_imgList.size() - m_landingQueue.size() - 1;

For a 5-image batch:

  • Processing image 1: button shows "☁ Auto Label All (0/5)…"
  • Processing image 2: button shows "☁ Auto Label All (1/5)…"

CloudAutoLabeler shows "1/N" for the first image being processed (progress signal emits done=1). The Landing AI path shows "0/N". This is a minor UX inconsistency between the two providers.


MEDIUM 3: m_landingNet is never deleted explicitly

m_landingNet = new QNetworkAccessManager(this);

This is actually fine — it's parented to this (MainWindow), so Qt's parent-child ownership handles cleanup. Just noting it's correct.

Non-issue on inspection.


Summary

Severity # Issue Blocking?
CRITICAL 1 cancelAutoLabel() doesn't reset buttons when only Landing AI is running Yes
CRITICAL 2 Inconsistent cleanup in stale callback paths (same root cause as #1) Yes
HIGH 1 Division by zero when image dimensions cannot be determined Yes
HIGH 3 MAX_LANDING_RETRIES function-local scope No (style)
MEDIUM 1 Architecture: ~350 lines of networking reimplemented in MainWindow No (follow-up)
MEDIUM 2 Batch progress counter inconsistency No

What looks good

  • Clean rebase on current master — no regressions from PR #128
  • Generation counter (m_landingGeneration) correctly handles stale callbacks in the async lambda
  • m_landingBusy flag properly blocks A/D navigation during Landing AI operations
  • QImageReader.size() for efficient dimension queries without full decode
  • Full JSON validation: firstVal.isArray(), isDouble() checks, coordinate range validation
  • Case-insensitive label matching with fallback
  • Separate consent dialog for Landing AI (distinct from yololabel.com)
  • 3-retry logic with parameter-based retry counting (matches CloudAutoLabeler pattern)
  • backupLabelFile() before overwriting label files
  • save_label_data() + saveState() for both single-image and batch undo support
  • setBodyDevice() for memory-efficient image upload
  • CloudProvider enum replacing magic numbers
  • Model selection persisted to QSettings
  • Upload size warning for large batches
  • m_objList.isEmpty() early guard preventing silent detection drops
  • Unknown label reporting to status bar (skippedLabels)
  • File write failure detection with status bar message
  • Slider disabled during Landing AI operations
  • Only writes file when detections are non-empty (consistent with "no detections" path)

Recommended action

Fix CRITICAL 1 (one-line fix in cancelAutoLabel()) and HIGH 1 (image dimension guard). Both are small, targeted changes. CRITICAL 2 resolves automatically when CRITICAL 1 is fixed. After those, this is ready to merge.

Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict Review — PR #129: Landing AI Integration (Round 4)

The latest two commits (301520a, 80ef2f4) addressed most round 3 issues well — generation counter, m_landingBusy guard, saveState() in batch, m_objList.isEmpty() guard, explicit includes, setBodyDevice(), case-insensitive matching, CloudProvider enum, model combo persistence, and upload size warning. CI passes on all 3 platforms.

However, after cross-referencing with the robustness standards established in PR #128 (which went through 6 rounds of review), there are still issues that need attention.


CRITICAL: cancelAutoLabel() fails to reset buttons when only Landing AI is running

mainwindow.cpp cancelAutoLabel(), cloud_labeler.cpp:81-86

cancelAutoLabel() does not call resetCloudButtons() directly. It relies on m_cloudLabeler->cancel() emitting busyChanged(false), which triggers the connected lambda (line 155-159):

connect(m_cloudLabeler, &CloudAutoLabeler::busyChanged, this, [this](bool busy) {
    ui->horizontalSlider_images->setEnabled(!busy);
    if (!busy) resetCloudButtons();
});

But CloudAutoLabeler::setBusy() has a guard:

// cloud_labeler.cpp:81-86
void CloudAutoLabeler::setBusy(bool busy) {
    if (m_busy == busy) return;   // Already false? No signal emitted!
    m_busy = busy;
    emit busyChanged(busy);
}

Scenario:

  1. User selects Landing AI, clicks "Auto Label All" -> m_landingBusy = true, buttons disabled, slider disabled. CloudAutoLabeler is NOT busy (m_busy == false).
  2. User clicks Cancel -> cancelAutoLabel() calls m_cloudLabeler->cancel() -> setBusy(false) -> guard fires (false == false) -> no signal emitted -> resetCloudButtons() never called.
  3. The in-flight Landing AI callback fires -> gen != m_landingGeneration -> returns immediately without calling resetCloudButtons().
  4. Result: Buttons stuck in disabled/cancel state permanently. The user must quit and restart the app.

Additionally, in the doLandingAIJob() lambda, the stale-callback path (gen mismatch) just returns without cleanup:

if (gen != m_landingGeneration) return;        // No resetCloudButtons()!

While the cancel path does clean up:

if (m_landingCancelled) {
    resetCloudButtons();                        // Cleanup
    return;
}

But since cancelAutoLabel() increments m_landingGeneration, the lambda always hits the gen-mismatch path (Path A, no cleanup) rather than the m_landingCancelled path (Path B, with cleanup).

Required fix: Call resetCloudButtons() directly in cancelAutoLabel():

void MainWindow::cancelAutoLabel()
{
    ++m_landingGeneration;
    m_cloudLabeler->cancel();
    m_landingCancelled = true;
    m_landingQueue.clear();
    resetCloudButtons();  // Ensure buttons always reset
}

resetCloudButtons() is idempotent, so double-calling (when CloudAutoLabeler IS busy and busyChanged fires too) is safe.


HIGH 1: Division by zero when image dimensions cannot be determined

mainwindow.cpp doLandingAIJob() lambda

QImageReader reader(imagePath);
QSize imgSize = reader.size();
if (!imgSize.isValid() || imgSize.isEmpty())
    imgSize = QImage(imagePath).size();
double imgW = imgSize.width();
double imgH = imgSize.height();
// ...
double cx = ((x1 + x2) / 2.0) / imgW;  // imgW could be 0!

If both QImageReader and QImage fail (corrupted file, unsupported format, permission denied), imgSize remains invalid. imgW and imgH would be 0, causing division by zero in the coordinate normalization.

Required fix: Guard against zero dimensions:

if (imgW <= 0 || imgH <= 0) {
    statusBar()->showMessage(
        "Landing AI: could not read image dimensions: " + imagePath, 5000);
    if (m_landingQueue.isEmpty()) resetCloudButtons();
    else                          landingAIProcessNextInQueue();
    return;
}

HIGH 2: MAX_LANDING_RETRIES function-local scope

void MainWindow::doLandingAIJob(const QString &imagePath, int retryCount, int gen)
{
    ...
    static constexpr int MAX_LANDING_RETRIES = 3;
    ...
}

MAX_LANDING_RETRIES is defined inside the function body as a static constexpr. It works correctly but is inconsistent with CloudAutoLabeler::MAX_RETRIES which is a class-level constant. Consider moving to a class-level constant.

Not blocking, but noted for consistency.


MEDIUM 1: Architecture concern (reiteration, non-blocking)

The initial review of PR #128 identified "636 lines added directly to mainwindow.cpp" as the first issue and extracted all cloud logic into CloudAutoLabeler. PR #129 adds ~350 lines of networking code back into MainWindow. The CRITICAL bug above exists precisely because the Landing AI path bypasses CloudAutoLabeler's cancel/busy infrastructure.

A provider abstraction in CloudAutoLabeler (or a separate LandingAILabeler class following the same QObject+signals pattern) would eliminate CRITICAL 1 entirely — cancel would go through the same setBusy path. Strongly recommended as a follow-up refactor before adding more providers.


MEDIUM 2: Batch progress counter inconsistency

landingAIProcessNextInQueue():

int done = m_imgList.size() - m_landingQueue.size() - 1;

For a 5-image batch, processing image 1 shows "Auto Label All (0/5)..." while CloudAutoLabeler shows "1/N" for the first image. Minor UX inconsistency between providers.


Summary

Severity Issue Blocking?
CRITICAL cancelAutoLabel() doesn't reset buttons when only Landing AI is running Yes
HIGH Division by zero when image dimensions cannot be determined Yes
HIGH MAX_LANDING_RETRIES function-local scope No (style)
MEDIUM Architecture: ~350 lines of networking reimplemented in MainWindow No (follow-up)
MEDIUM Batch progress counter inconsistency No

What looks good

  • Clean rebase on current master — no regressions from PR #128
  • Generation counter (m_landingGeneration) correctly handles stale callbacks in the async lambda
  • m_landingBusy flag properly blocks A/D navigation during Landing AI operations
  • QImageReader.size() for efficient dimension queries without full decode
  • Full JSON validation: firstVal.isArray(), isDouble() checks, coordinate range validation
  • Case-insensitive label matching with fallback
  • Separate consent dialog for Landing AI (distinct from yololabel.com)
  • 3-retry logic with parameter-based retry counting (matches CloudAutoLabeler pattern)
  • backupLabelFile() before overwriting label files
  • save_label_data() + saveState() for both single-image and batch undo support
  • setBodyDevice() for memory-efficient image upload
  • CloudProvider enum replacing magic numbers
  • Model selection persisted to QSettings
  • Upload size warning for large batches
  • m_objList.isEmpty() early guard preventing silent detection drops
  • Unknown label reporting to status bar (skippedLabels)
  • File write failure detection with status bar message
  • Slider disabled during Landing AI operations
  • Only writes file when detections are non-empty (consistent with "no detections" path)

Recommended action

Fix the CRITICAL (one-line fix in cancelAutoLabel()) and HIGH 1 (image dimension guard). Both are small, targeted changes. After those, this is ready to merge.

- cancelAutoLabel(): call resetCloudButtons() directly so buttons are
  always re-enabled even when CloudAutoLabeler is already idle (m_busy
  == false prevents busyChanged signal from firing)
- doLandingAIJob(): guard against zero image dimensions to avoid
  division by zero when QImageReader and QImage both fail
- Move MAX_LANDING_RETRIES from function-local static to class-level
  constexpr, consistent with CloudAutoLabeler::MAX_RETRIES

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

@vusallyv vusallyv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed all blocking issues in ea558dc:

CRITICAL 1 fixedcancelAutoLabel() now calls resetCloudButtons() directly, so buttons always re-enable regardless of whether CloudAutoLabeler is busy. The comment explains why: busyChanged is only emitted when m_busy actually changes, so it's a no-op when CloudAutoLabeler is already idle.

CRITICAL 2 — Resolved automatically by the CRITICAL 1 fix. The gen-mismatch path in the stale lambda no longer needs to call resetCloudButtons() itself, since cancelAutoLabel() handles it synchronously before any callbacks can fire.

HIGH 1 fixed — Added a imgW <= 0 || imgH <= 0 guard in doLandingAIJob() after the QImageReader/QImage fallback. On failure, shows a status bar message and either resets buttons (last image) or advances the queue.

HIGH 3 (style) fixedMAX_LANDING_RETRIES moved from function-local static constexpr to a class-level static constexpr in mainwindow.h, consistent with CloudAutoLabeler::MAX_RETRIES.

@vusallyv vusallyv requested a review from developer0hye March 2, 2026 08:40
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict review findings

  1. CRITICAL: Landing batch uses mutable UI state (m_imgList) as its job source, so mid-run dataset mutations can mislabel wrong files or hit invalid indices.
  • Queue is built from indices of current list: mainwindow.cpp:1712, mainwindow.cpp:1714
  • Each step dereferences current m_imgList[idx]: mainwindow.cpp:1745
  • m_imgList can still change during Landing run:
    • open new dataset has no busy guard: mainwindow.cpp:284
    • delete image has no busy guard: mainwindow.cpp:472
    • delete shortcuts remain active: mainwindow.cpp:48, mainwindow.cpp:49
    • only prev/next are guarded by m_landingBusy: mainwindow.cpp:417, mainwindow.cpp:425
  1. MEDIUM: Unknown-label diagnostics are immediately overwritten, so users lose the reason detections were dropped.
  • unknown-label message is shown: mainwindow.cpp:1652
  • then immediately overwritten by generic detection count: mainwindow.cpp:1660

I could not run build/tests locally in this environment because qmake is not installed; this review is static analysis of branch feat/landing-ai.

…ages

- on_pushButton_open_files_clicked() and remove_img() now early-return
  when CloudAutoLabeler or Landing AI batch is running, preventing
  m_imgList mutations that would invalidate queued indices (CRITICAL developer0hye#1)
- Merged skipped-labels status message with detection-count message
  on final/single image so the unknown-label info is never overwritten
  (MEDIUM developer0hye#2 from round 5 review)

Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
@vusallyv
Copy link
Copy Markdown
Contributor Author

vusallyv commented Mar 2, 2026

Addressed both round 5 issues in 6ec7644:

CRITICAL fixedon_pushButton_open_files_clicked() and remove_img() now early-return when m_landingBusy (or CloudAutoLabeler) is busy. This covers the open-dataset button, the Delete key shortcut, and Ctrl+D — all three mutation paths that could invalidate queued indices during a Landing AI batch.

MEDIUM fixed — Removed the separate skippedLabels showMessage() call that preceded the queue-empty check. When the queue is empty (last/only image), detection count and any skipped labels are now combined into a single status bar message (e.g. "Landing AI: 3 detection(s) — skipped unknown labels: Person, Car"). During mid-batch images the skipped-labels message is still shown independently since landingAIProcessNextInQueue() doesn't touch the status bar.

@vusallyv vusallyv requested a review from developer0hye March 2, 2026 12:31
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict review findings

  1. CRITICAL: Cancel does not stop in-flight Landing AI network activity.
  • cancelAutoLabel() only flips flags/clears queue/reset UI: mainwindow.cpp:1307
  • Active request is created in doLandingAIJob() and not retained for abort: mainwindow.cpp:1517
  • Stale callback path only ignores completion (gen mismatch) instead of stopping upload: mainwindow.cpp:1524

Impact: after clicking Cancel, the upload/request may still continue until completion on the wire; only callback effects are suppressed.

  1. HIGH: Landing batch reads mutable settings (m_landingApiKey, m_cloudPrompt) per-image, so one batch can run with mixed settings.
  • Settings can change at runtime via Save: mainwindow.cpp:1275
  • Each request recomputes prompt/auth from current members: mainwindow.cpp:1494, mainwindow.cpp:1514

Impact: if user saves settings mid-batch, later images can run with different key/prompt than earlier images, producing inconsistent behavior and hard-to-debug failures.

  1. MEDIUM: No automated test coverage for the new Landing AI flow in MainWindow.
  • Existing tests focus on CloudAutoLabeler behavior (e.g. tests/test_cloud_labeler.cpp:2)
  • New paths (submitLandingAIJob, landingAIAutoLabelAll, doLandingAIJob, cancelAutoLabel) have no regression tests for cancel, queue progression, and response edge cases.

I could not run build/tests locally because qmake is not installed in this environment; this review is static analysis of branch feat/landing-ai.

1. CRITICAL: abort in-flight QNetworkReply on cancel
   - Store active reply in m_activeLandingReply
   - cancelAutoLabel() calls reply->abort() before incrementing generation,
     stopping the upload on the wire rather than only suppressing the callback

2. HIGH: snapshot settings at batch start
   - Add m_batchLandingApiKey / m_batchLandingPrompt, captured in
     submitLandingAIJob() and landingAIAutoLabelAll() before the first request
   - doLandingAIJob() uses the snapshot; mid-batch Save no longer changes
     auth or prompt for images already in the queue

3. MEDIUM: add unit tests for Landing AI JSON response parsing
   - Move parseLandingAIDetections() from MainWindow to CloudAutoLabeler
     so it can be tested without instantiating the full widget stack
   - Add tests/test_landing_ai_parser.{cpp,pro} with 15 cases covering:
     exact label match, case-insensitive fallback, unknown/empty labels,
     missing/short/non-numeric bounding_box, out-of-range and zero-size
     boxes, multiple detections with mixed validity, coordinate math

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
@vusallyv vusallyv requested a review from developer0hye March 2, 2026 13:38
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict Review — PR #129: Landing AI Integration (Round 8)

CI passes on all 3 platforms + DCO. The PR has improved significantly through 8 rounds — the latest commits address cancel-abort, settings snapshot, open/delete guards, and unit tests. Cross-referencing with CloudAutoLabeler's robustness patterns reveals the following remaining issues.


CRITICAL: effectivePrompt reads mutable m_objList at request time, not at batch start

mainwindow.cppdoLandingAIJob()

QString effectivePrompt = m_batchLandingPrompt.isEmpty()
    ? m_objList.join(" ; ")       // ← reads mutable m_objList at request time
    : m_batchLandingPrompt;

The API key and prompt were correctly snapshotted into m_batchLandingApiKey / m_batchLandingPrompt (round 5 fix). But when the user-set prompt is empty (the most common case — Landing AI prompt defaults to class names), m_objList is read per-request, not snapshotted. If the user opens a different class file mid-batch (not blocked — open_obj_file has no m_landingBusy guard), later images would use different prompts than earlier ones.

Fix: Snapshot the effective prompt at batch start:

// In submitLandingAIJob() / landingAIAutoLabelAll():
m_batchLandingPrompt = m_cloudPrompt.isEmpty()
    ? m_objList.join(" ; ")
    : m_cloudPrompt;
// In doLandingAIJob(): always use m_batchLandingPrompt (remove the fallback)

HIGH 1: No mutual exclusion between YoloLabel AI and Landing AI operations

There is no guard preventing both providers from running concurrently. Scenario:

  1. User starts a YoloLabel AI batch → CloudAutoLabeler busy, buttons disabled
  2. CloudAutoLabeler finishes → busyChanged(false)resetCloudButtons() re-enables all buttons and sets m_landingBusy = false
  3. User switches combo to Landing AI, clicks "Auto Label AI"
  4. Meanwhile, if timing is close, both providers could run concurrently sharing the same buttons and cancel infrastructure

Fix: The button-clicked lambdas should check both providers before proceeding:

connect(m_btnCloudAutoLabel, &QPushButton::clicked, this, [this](){
    if (m_landingBusy || (m_cloudLabeler && m_cloudLabeler->isBusy())) return;
    if (m_modelCombo->currentIndex() == ProviderLandingAI) submitLandingAIJob();
    else submitCloudJob();
});

Same for m_btnCloudAutoLabelAll.

HIGH 2: cancelAutoLabel() ordering — landing state set after CloudAutoLabeler cancel

void MainWindow::cancelAutoLabel()
{
    ++m_landingGeneration;
    if (m_activeLandingReply) {
        m_activeLandingReply->abort();
        m_activeLandingReply = nullptr;
    }
    m_cloudLabeler->cancel();       // ← triggers busyChanged(false) → resetCloudButtons() → m_landingBusy=false
    m_landingCancelled = true;      // ← set AFTER resetCloudButtons() already ran
    m_landingQueue.clear();         // ← cleared AFTER resetCloudButtons() already ran
    resetCloudButtons();
}

m_cloudLabeler->cancel() triggers setBusy(false)busyChanged(false)resetCloudButtons(), which sets m_landingBusy = false. Between that moment and the explicit assignments below, the landing state is inconsistent (busy=false but cancelled/queue not yet cleared). Safe in practice (single-threaded Qt event loop), but fragile and confusing.

Fix: Set landing state before m_cloudLabeler->cancel():

void MainWindow::cancelAutoLabel()
{
    ++m_landingGeneration;
    if (m_activeLandingReply) {
        m_activeLandingReply->abort();
        m_activeLandingReply = nullptr;
    }
    m_landingCancelled = true;
    m_landingQueue.clear();
    m_cloudLabeler->cancel();
    resetCloudButtons();
}

MEDIUM 1: Batch skipped-labels message is immediately overwritten

if (!skippedLabels.isEmpty()) {
    statusBar()->showMessage(
        QString("Landing AI: detections skipped (unknown labels: %1)")
            .arg(skippedLabels.join(", ")), 5000);
}
landingAIProcessNextInQueue();  // immediately fires next request

The skipped-labels message is set with 5s timeout, but the next request completes and overwrites it immediately. During a batch, the user never sees which labels were skipped for intermediate images.

Suggestion: Accumulate skipped labels across the batch and report them in the final summary message.

MEDIUM 2: Architecture — ~350 lines of networking in MainWindow (reiteration, non-blocking)

Every round has found bugs that CloudAutoLabeler already handles correctly (cancel, stale callbacks, busy guards, retry, backup, settings snapshot). The pattern is clear: reimplemented infrastructure drifts from the reference implementation. A provider abstraction would eliminate this entire class of bugs. Not blocking, but strongly recommended before adding a third provider.

MEDIUM 3: LANDING_AI_ENDPOINT should be constexpr

static const char *LANDING_AI_ENDPOINT = ...;

File-scope static const char*static constexpr const char* would be more idiomatic C++17 and consistent with CloudAutoLabeler's constants.


LOW / Nits

  1. Double blank line between doLandingAIJob() and landingAIAutoLabelAll() — style inconsistency
  2. m_modelCombo not initialized to nullptr in the member list — initialized in initSideTabWidget() but a default = nullptr would be safer
  3. 30s transfer timeout (req.setTransferTimeout(30000)) may be too short for large images on slow connections — CloudAutoLabeler doesn't set a transfer timeout

Summary

Severity # Issue Blocking?
CRITICAL 1 effectivePrompt reads mutable m_objList per-request Yes
HIGH 1 No mutual exclusion between providers Yes
HIGH 2 cancelAutoLabel() ordering No (fragile, not unsafe)
MEDIUM 1 Batch skipped-labels message overwritten No (UX)
MEDIUM 2 Architecture: networking reimplemented in MainWindow No (follow-up)
MEDIUM 3 const char* vs constexpr No (style)
LOW 3 Style nits No

What looks good

  • Clean rebase — no regressions from PR #128
  • Generation counter correctly handles stale callbacks
  • m_landingBusy guards A/D navigation and open/delete during batch
  • m_activeLandingReply->abort() stops in-flight uploads on cancel
  • Settings snapshot (m_batchLandingApiKey / m_batchLandingPrompt) prevents mid-batch key/prompt drift
  • QImageReader.size() for efficient dimension queries without full decode
  • Full JSON validation: firstVal.isArray(), isDouble() checks, coordinate range validation, division-by-zero guard
  • Case-insensitive label matching with fallback
  • Separate consent dialog for Landing AI (distinct from yololabel.com)
  • 3-retry logic, backupLabelFile(), save_label_data() + saveState() for undo
  • setBodyDevice() for memory-efficient upload
  • CloudProvider enum replacing magic numbers, model combo persisted to QSettings
  • parseLandingAIDetections() extracted as pure static function with 15 unit tests
  • Upload size warning for large batches, m_objList.isEmpty() early guard
  • Unknown label reporting to status bar, file write failure detection

Recommended action

  1. Fix CRITICAL — snapshot effective prompt at batch start (small change in 2 functions)
  2. Fix HIGH 1 — add mutual exclusion guards in button lambdas (2 lines each)
  3. Fix HIGH 2 — reorder cancelAutoLabel() (just move 2 lines up)
  4. Everything else can be follow-up PRs

CRITICAL: snapshot effective prompt at batch start in submitLandingAIJob()
and landingAIAutoLabelAll() using m_objList.join() when m_cloudPrompt is
empty, preventing mid-batch class file changes from affecting later images.
doLandingAIJob() now always uses m_batchLandingPrompt directly.

HIGH 1: add mutual exclusion guards in Auto Label / Auto Label All button
lambdas — both check m_landingBusy and m_cloudLabeler->isBusy() before
proceeding, preventing concurrent YoloLabel AI + Landing AI operations.

HIGH 2: reorder cancelAutoLabel() to set m_landingCancelled=true and clear
m_landingQueue before calling m_cloudLabeler->cancel(), eliminating the
window where landing state is inconsistent with busy state.

Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
@vusallyv vusallyv requested a review from developer0hye March 4, 2026 10:31
@developer0hye
Copy link
Copy Markdown
Owner

@vusallyv Why did you choose Landing AI?

@vusallyv
Copy link
Copy Markdown
Contributor Author

vusallyv commented Mar 4, 2026

Landing AI uses the same prompt-to-detections approach as YoloLabel AI, so the integration fits naturally without changing the UX. That said, in my testing YoloLabel AI gives more accurate detections and is less expensive — Landing AI is there as an alternative for users who already have a Landing AI API key.

Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict review findings (blocking)

1) HIGH — Landing AI write-failure is reported as success

  • In doLandingAIJob(), file open failure is detected and a status error is shown:
    • mainwindow.cpp:1613 (if (lf.open(...)))
    • mainwindow.cpp:1619 (could not write label file)
  • But execution falls through and still shows a success-style message based on detection count:
    • mainwindow.cpp:1626 ("Landing AI: %1 detection(s)")

Impact: users can be told labeling succeeded even when no label file was written.

2) HIGH — Landing label matching breaks with CRLF/trailing-space class names

  • Class labels are loaded without trimming:
    • mainwindow.cpp:535 (m_objList << qstrLabel)
  • Landing parser compares raw label strings:
    • cloud_labeler.cpp:722 (objList.indexOf(label))
    • cloud_labeler.cpp:726 (case-insensitive compare, still untrimmed)

I reproduced this quickly with objList={"cat\r"} and detection label "cat": parser returned no output lines and treated the label as skipped.

Impact: detections are silently dropped for common Windows-formatted class files.

3) MEDIUM — New parser test is not wired into CI

  • PR adds parser tests:
    • tests/test_landing_ai_parser.cpp
  • But CI currently runs only:
    • test_cloud_labeler, test_label_img, test_yolo_detector
    • .github/workflows/ci-release.yml:130-140 and :150-163

Impact: parser regressions can merge undetected.


Verification performed locally

  • qmake YoloLabel.pro && make (build passed)
  • tests/test_landing_ai_parser (16 passed)
  • tests/test_cloud_labeler (37 passed)

- fix: write-failure in doLandingAIJob() no longer falls through to
  success message; early return after status bar error, with proper
  queue continuation or button reset
- fix: class names trimmed on load (CRLF/trailing-space in Windows
  class files no longer silently drops Landing AI detections)
- fix: parseLandingAIDetections trims both objList entries and the
  incoming label before case-insensitive comparison
- test: add crlfObjList_matchesTrimmed and trailingSpaceLabel_matchesTrimmed
  cases to test_landing_ai_parser
- ci: wire test_landing_ai_parser into CI for Unix and Windows

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
@vusallyv vusallyv requested a review from developer0hye March 4, 2026 10:47
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. HIGH: parseLandingAIDetections() still accepts partially off-image boxes. It only validates normalized center/size, not the raw corners, so a box like [-10, 10, 90, 90] on a 100x100 image passes and gets written as a YOLO annotation even though part of the box lies outside the image. That will poison label files instead of rejecting or clamping invalid detections. See cloud_labeler.cpp:734, cloud_labeler.cpp:741, cloud_labeler.cpp:746.

  2. HIGH: malformed top-level Landing responses are still treated as "no detections". The !dataVal.isArray() case is routed through the same branch as an empty detection list, so a 200 OK payload like {"error":"quota exceeded"} or {"data":{}} will silently preserve labels and, in batch mode, continue processing as if nothing went wrong. That hides protocol or service failures behind a false "0 detections" outcome. See mainwindow.cpp:1564, mainwindow.cpp:1565, mainwindow.cpp:1571.

  3. MEDIUM: batch completion reporting is inaccurate after recoverable per-image failures. If dimension lookup fails or the label file cannot be opened, the code skips to the next image, but the terminal branch still reports Landing AI: all %1 images done. Earlier status-bar errors are transient and get overwritten, so users can be told the batch completed successfully even when some images were never labeled. See mainwindow.cpp:1594, mainwindow.cpp:1619, mainwindow.cpp:1713.

Verification performed locally: built and ran tests/test_landing_ai_parser (18 passed) and tests/test_cloud_labeler (37 passed) in /tmp/pr129-review/tests.

…sponse handling, batch failure count

1. HIGH: clamp partially off-image corners to image bounds in
   parseLandingAIDetections() instead of discarding the entire detection;
   boxes entirely outside the image still produce zero size and are skipped.
   Add partiallyOffImageBox_clamped test (19 pass, 0 fail).

2. HIGH: split !dataVal.isArray() (malformed/error response) from
   dataVal.toArray().isEmpty() (no detections) in the Landing AI reply
   handler. A non-array "data" field now shows a warning and stops
   processing instead of silently falling through as zero detections.

3. MEDIUM: track m_landingFailCount across a batch run; reset to 0 at
   batch start, increment on dimension-read or file-write failure, and
   emit "X/N images labeled (Y failed)" in the completion message so
   per-image errors are visible to the user.

Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
@vusallyv vusallyv requested a review from developer0hye March 9, 2026 12:38
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found 3 issues in the Landing AI integration.

  1. Blocker: the request builder always explodes the effective prompt into multiple prompts form fields while hardcoding model=agentic (mainwindow.cpp:1463, mainwindow.cpp:1504, mainwindow.cpp:1514). The current Landing AI OpenAPI says the agentic model allows only one prompt for /v1/tools/text-to-object-detection, so the default “use class file labels” path will 422 on any normal multi-class dataset instead of labeling it.

  2. High: the reply handler rejects any non-array data value as malformed (mainwindow.cpp:1564), but the current ODResponse schema allows data to be null. A contract-compliant empty-result response would therefore be surfaced as “Unexpected response from server” and abort the run instead of behaving like “no detections.”

  3. Low: canceling a Landing AI run still calls CloudAutoLabeler::cancel() unconditionally (mainwindow.cpp:1309), and that method always emits Cloud auto-label cancelled. even when no cloud job is active (cloud_labeler.cpp:69). The shared cancel button added in this PR will therefore show the wrong provider/status message on every Landing cancel.

I built the PR in a temporary worktree at /tmp/yolo-pr129, built YoloLabel.pro, and ran tests/test_landing_ai_parser successfully (19 passed). I did not run live Landing AI requests because there is no API key in the workspace; the first two findings come from the current official provider schema plus the PR code path.

1. Send all class names as a single `prompts` field instead of splitting
   into multiple fields. The agentic model only accepts one prompt per
   request; multi-class datasets would previously 422.

2. Accept `data: null` in the Landing AI response as empty detections
   instead of treating it as a malformed response. The ODResponse schema
   allows null.

3. Only call CloudAutoLabeler::cancel() when the cloud labeler is
   actually busy, preventing a spurious "Cloud auto-label cancelled."
   message when cancelling a Landing AI run.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
@vusallyv vusallyv requested a review from developer0hye March 25, 2026 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants