Cleanup Scanning Page#70
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the scanning workflow UI to select an image type and derive the corresponding record type and modality automatically, aligning with issues #64/#65 around scan type selection and AI-assisted defaults.
Changes:
- Replace “Record Type” + “Modality” inputs with a single “Image Type” selector.
- Update AI autofill to set the image type from the
image_typesvalueset. - Derive
record_typeandmodalityclient-side during submit based on the selected image type (plus bump VERSION to a-devsuffix).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| bfd9000_web/archive/templates/archive/scan.html | Simplifies metadata inputs, loads image_types valueset, adjusts AI fill + submit payload logic. |
| bfd9000_web/VERSION | Switches version string to 0.1.3-dev. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ation on valuesets
…ose configuration
zgypa
left a comment
There was a problem hiding this comment.
[claude-sonnet-4.6 / @worker]
Overall the PR is clean and the direction is correct. A few issues worth addressing before merge:
1. Network call in initialize blocks on external FHIR endpoint (medium)
import_valuesets --all now runs unconditionally inside initialize when --skip-migrate is not set. If the FHIR endpoint at http://terminology.open-ortho.org/... is unreachable (offline dev, CI without network), the entire init fails with an unhandled URLError. There is no --skip-valueset-import flag and no fallback. Consider wrapping in try/except with a warning, or adding a skip flag.
2. _fetch_valueset decodes as ASCII, silently drops non-ASCII (low)
response.read().decode("ascii", errors="ignore") silently strips non-ASCII bytes (e.g. accented display names). FHIR JSON is UTF-8; use "utf-8" or read the Content-Type charset.
3. AI confidence display crashes if type_probability is missing (medium)
scan.html line 1034 reads result.type_probability.toFixed(2). After this PR, queryAI returns { ...result, class_code, class_prediction, class_probability } — if type_probability is absent from the AI response this throws TypeError in the catch block, showing an error to the user even though classification succeeded. Should use result.class_probability ?? result.type_probability (or ?.toFixed).
4. AI code lookup cascade has wrong ordering (low)
Line 1013 of scan.html:
result.code || result.record_type || result.prediction || result.class_code || result.class_prediction
result.prediction is the raw text label (e.g. "Lateral"), not a valueset code, so it silently fails the find check — but only when result.code and result.record_type are both falsy. class_code (newly added this PR, the intended code) is placed after result.prediction, meaning a non-null prediction string shadows it. Code fields should come before text-label fields.
5. Behavioral change: patient orientation now set by rt_coding.code == 'L' not image_type.code == 'L' (worth confirming)
serializers.py:578 changed from checking image_type.code to rt_coding.code. This should be equivalent per the new 1-to-1 mapping, but it is worth explicitly confirming that no record type with code L can represent anything other than a lateral cephalogram, since ['A', 'F'] will now be auto-applied unconditionally for any record type L.
| "import_valuesets", | ||
| "--all", | ||
| verbosity=verbosity, | ||
| ) |
There was a problem hiding this comment.
[claude-sonnet-4.6 / @worker] The initialize command now runs import_valuesets --all unconditionally (whenever --skip-migrate is not set). This makes a network request to http://terminology.open-ortho.org/.... If that endpoint is unreachable (offline dev environment, CI runner without outbound network, air-gapped deploy), the init command will fail with an unhandled URLError before the superuser or data import steps run. There is no --skip-valueset-import flag and no fallback. Consider wrapping in a try/except that emits a warning and continues, or adding a dedicated skip flag.
There was a problem hiding this comment.
Wrap and emit warning. no need for extra flag.
There was a problem hiding this comment.
[github-copilot/claude-sonnet-4.6] Fixed: wrapped the import_valuesets --all call in a try/except block. On failure a Django warning is emitted and initialization continues. No extra CLI flag added.
| const canSubmit = hasSubject && hasEncounter && hasFile && hasRecordType && hasModality; | ||
| const canSubmit = hasSubject && hasEncounter && hasFile && hasRecordType; | ||
| document.getElementById('submit-button').disabled = !canSubmit; | ||
|
|
There was a problem hiding this comment.
[claude-sonnet-4.6 / @worker] result.type_probability may be undefined here. After this PR queryAI spreads the original result but stores the probability as class_probability. If the AI service does not return a type_probability field, calling .toFixed(2) throws TypeError, which is caught by the outer catch and shown as an error to the user even though classification succeeded. Fix: (result.class_probability ?? result.type_probability)?.toFixed(2) ?? 'N/A'.
There was a problem hiding this comment.
[github-copilot/claude-sonnet-4.6] Applied: probability display now uses (result.class_probability ?? result.type_probability)?.toFixed(2) ?? 'N/A' to safely handle the case where type_probability is absent.
… JS probability access, and AI class_code cascade. - initialize.py: Wrap import_valuesets in try/except and warn on failure as directed in PR review (no CLI flag). - importers/valuesets.py: Decode FHIR JSON response as UTF-8 per spec, not ascii. - scan.html: Use null-safe access for probability, simplify AI classification field to class_code||code, show user warning if AI fails classification.
Fixes #64 and #65
Instead of selecting the record type and modality, you select the image type and it figures it out. Currently hardcoded it in the submit function on the client side code.
There are some inconsistencies there—I'll make an issue about it.
aiImageTypewas redundant,aiPatientOrientationwas not actually given by the AI. Although it is a field on theRecordmodel it was pointless to set it.