fix: restore lab import review in on-device fallback path

[bisonbet]

Motivation

• The on-device document processing path could fall back to a placeholder blood test without providing reviewable candidates, causing the UI review/accept flow to never appear for on-device imports.
• The intent is to ensure the item-based (on-device) mapping path has the same review behavior and metadata as the full-text AI mapping path so users can review and accept parsed lab values.

Description

• Parse and expose structured healthDataItems early from ProcessedDocumentResult and pass them into createBloodTestResultFromText so the item-based fallback mapping has real context instead of an empty list (file: Services/DocumentProcessor.swift).
• In the AI mapping from items path, create a PendingImportReview and assign it to pendingImportReview on the MainActor when mappingResult.needsReview so the UI review sheet is triggered the same way as the full-text path.
• Persist the review-related metadata keys (needs_review, import_groups_count, pending_review) onto the BloodTestResult metadata and unify creation of the enhanced blood test result so downstream UI/DB logic can detect pending reviews.

Testing

• No automated unit or UI tests were executed for this patch (per repo guidance to avoid xcodebuild unless explicitly requested).
• Changes were validated via targeted source inspection and diffs to ensure the new branches and metadata assignments are present in Services/DocumentProcessor.swift and align with the full-text mapping behavior; CI should run the full build and XCTest to verify runtime integration.

---

Codex Task

[bisonbet]

@claude review this PR

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2fe542bcac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Avoid feeding generic structured items into lab fallback

Passing healthDataItems here changes the failure path for text mapping: if mapDocumentToBloodTest(documentText) throws, createBloodTestResultFromText now falls back to createBloodTestResultFromItems, which can end in createLegacyBloodTestResult that converts every item into a BloodTestItem. Because ProcessedDocumentResult.healthDataItems includes non-lab fields (e.g., vitals/medications), AI outages can now import incorrect lab results instead of the previous safe placeholder behavior. Please filter to lab-only items (or keep this fallback input empty) before calling this path.

Useful? React with 👍 / 👎.

[bisonbet]

Fixed in 67bcf8a. Added a labOnlyItems filter that excludes known non-lab item types ("Personal Information", "Demographics", "Vital Signs", "Imaging", "Radiology") before passing to createBloodTestResultFromText. If AI mapping fails and the code falls through to createBloodTestResultFromItems → createLegacyBloodTestResult, only lab-specific items will be converted to BloodTestItem entries. The full healthDataItems set is still passed to the structured fallback blocks that handle vitals, demographics, and imaging separately.

[claude]

Code Review — PR #33: restore lab import review in on-device fallback path

Overview

This PR fixes a real and important bug: the item-based (on-device) document processing path was silently swallowing the review flow by never setting pendingImportReview, causing the UI sheet to never appear. The two-part fix — (1) passing real labOnlyItems into createBloodTestResultFromText so the fallback chain has data, and (2) mirroring the pendingImportReview assignment into createBloodTestResultFromItems — is the right approach.

---

Issues

Code duplication — medium priority

The pending-review block (create PendingImportReview, await MainActor.run, write three metadata keys) is now copy-pasted verbatim between createBloodTestResultFromText (lines 736-759) and createBloodTestResultFromItems (lines 821-843). Any future change to this logic (new metadata key, different flag name) must be made in two places, and they will inevitably drift. Extract a helper:

private func applyPendingReview(
    _ mappingResult: BloodTestMappingResult,
    document: MedicalDocument,
    to bloodTest: inout BloodTestResult
) async {
    guard mappingResult.needsReview else { return }
    let pendingReview = PendingImportReview(
        documentId: document.id,
        documentName: document.fileName,
        importGroups: mappingResult.importGroups,
        bloodTestResult: bloodTest
    )
    await MainActor.run { self.pendingImportReview = pendingReview }
    var meta = bloodTest.metadata ?? [:]
    meta["needs_review"] = "true"
    meta["import_groups_count"] = String(mappingResult.importGroups.count)
    meta["pending_review"] = "true"
    bloodTest.metadata = meta
}

Redundant metadata keys — low priority

needs_review and pending_review both encode the same boolean. Pick one and remove the other, otherwise downstream code that reads only one key will silently behave differently from code that reads the other.

Hardcoded type-name strings — low priority

let nonLabTypes: Set<String> = ["Personal Information", "Demographics", "Vital Signs", "Imaging", "Radiology"]

These strings must match whatever HealthDataItem.type produces at runtime. If the source of those strings ever changes (e.g. a Docling format update), the filter will silently stop working and non-lab items will again slip into blood test results. Consider deriving these from the same constants used when building HealthDataItem, or at minimum add a comment pointing to where those strings originate.

extractedItems parameter now used — clarify intent

Previously createBloodTestResultFromText was called with extractedItems: [] and a comment reading // We'll use only the text. The parameter is only consumed in the catch block (createBloodTestResultFromItems(from: extractedItems, ...)). The new comment // Lab-only items as safe fallback if AI text mapping fails is accurate — but the original misleading comment inside the function body (// Use the full document text for AI analysis) was not updated. A brief note there that extractedItems is the fallback input would help the next reader.

---

Correctness

• The await MainActor.run placement is correct. Both callers can run off the main actor, and pendingImportReview is @Published and must be mutated on the main thread.
• createBloodTestResultFromText calls createBloodTestResultFromItems only in its catch block, so neither can set pendingImportReview twice for a single document. No overwrite race.
• The early parse of healthDataItems and the repositioned log line are safe refactors with no behaviour change to the non-lab path.

---

Test Coverage

The PR description acknowledges no automated tests were added. Given that this is a UI-observable state change, a unit test verifying that pendingImportReview is non-nil after processing a document whose AI mapping returns needsReview == true would be valuable. Without it, a future refactor could re-introduce the bug silently.

---

Summary

The fix is correct and addresses a genuine user-facing bug. The main ask before merging is extracting the duplicated pending-review block into a shared helper to prevent future drift. The hardcoded type strings and redundant metadata key are lower priority but worth a follow-up.