feat: Spoolman enrichment on reader + writer pages (#97)#99
Merged
Conversation
…n filament response
…endor/filament/save-enrichment endpoints
… + Spoolman enrichment
…te, with vendor/filament confirmation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughPerforms read-only Spoolman UID lookups for smart NFC tag formats, caches returned enrichment separately from generic UID display, exposes enrichment via /api/status, adds server endpoints to find/save Spoolman vendor/filament/spool, and adds client-side Read + enrichment UI flows in multiple writer pages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Client UI
participant Web as WebServer
participant App as ApplicationManager
participant SpoolMgr as SpoolmanManager
participant Spoolman as Spoolman API
participant NFC as NFC Reader
User->>Client: Click "Read" (writer)
Client->>Web: Poll GET /api/status (loop ~30s)
rect rgba(100,200,150,0.5)
NFC->>App: SPOOL_DETECTED (smart tag)
App->>SpoolMgr: SpoolmanSyncRequest(lookup_only=true)
SpoolMgr->>Spoolman: GET /spool/UID/{uid}
Spoolman-->>SpoolMgr: Spool details (remaining, density, diameter, temps)
SpoolMgr->>App: SPOOLMAN_SYNCED (is_uid_lookup=true)
App->>App: Cache smartTagEnrichment_
end
Web->>App: getSmartTagEnrichment()
App-->>Web: enrichment payload
Web-->>Client: /api/status includes spoolman enrichment
Client->>Client: prefill form from status
User->>Client: Submit enrichment/save
Client->>Web: POST /api/spoolman/save-enrichment
rect rgba(150,200,150,0.5)
Web->>Spoolman: POST/PATCH spool, find/create vendor/filament
Spoolman-->>Web: Success
end
Web-->>Client: {success:true, spool_id}
Client->>User: Show banner
sequenceDiagram
participant NFC as Reader
participant App as ApplicationManager
participant UI as Reader UI
participant Web as WebServer
participant SpoolMgr as SpoolmanManager
participant Spoolman as Spoolman API
NFC->>App: SPOOL_DETECTED (smart tag)
App->>SpoolMgr: SpoolmanSyncRequest(lookup_only=true)
SpoolMgr->>Spoolman: GET spool by UID
Spoolman-->>SpoolMgr: details (remaining, density, diameter)
SpoolMgr->>App: SPOOLMAN_SYNCED (is_uid_lookup=true)
App->>App: store smartTagEnrichment_ (no display update)
Web->>App: /api/status
App-->>Web: include smartTagEnrichment
Web-->>UI: status with spoolman enrichment
UI->>UI: render enrichment rows and badge
NFC->>App: TAG_REMOVED
App->>App: enrichment remains until next scan/reset
UI->>UI: clear display when appropriate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ApplicationManager.cpp`:
- Around line 724-757: The smartTagEnrichment_ update is incorrectly gated by if
(display_) and lacks the presence/UID guard, so move the smartTagEnrichment_
writes out of the display_ conditional (so display-disabled units still populate
/api/status.spoolman) and before/independent of display_->showSpool;
additionally reuse the same CurrentSpoolState presence/UID check used earlier
(call NFCManager::getInstance().getCurrentSpoolState(state) and verify
state.present && state.spool_id == msg.payload.spoolmanSynced.spool_id) before
updating either smartTagEnrichment_ or calling display_->showSpool to prevent
stale/delayed lookup A from overwriting data for tag B.
In `@src/OpenTag3DWriterHTML.h`:
- Line 576: The current call to saveEnrichment(presentStatus.uid) can persist
enrichment for the first-detected UID rather than the UID that passed
verification; update the logic so that you save using the verified UID (e.g.,
use the UID from the successful verification result such as status.uid or the
variable returned when tag_kind === 'OpenTag3D' verification succeeds) or add a
check that the presentStatus.uid matches the verified status.uid before calling
saveEnrichment; locate the verify loop and replace the unconditional
saveEnrichment(presentStatus.uid) with a save tied to the verified UID (or guard
the save with a UID equality check) so enrichment is written only for the
verified Spoolman spool.
- Around line 708-714: The current handler calls fillEnrichmentFromStatus and
breaks immediately on the first OpenTag3D status, which can occur before
status.spoolman is populated; change the logic in the OpenTag3D status handling
(the block calling fillEnrichmentFromStatus and showMatchBadge) to continue
polling instead of breaking until either status.spoolman is present (and
status.spoolman.spool_id > 0) or the read timeout expires; when status.spoolman
appears call fillEnrichmentFromStatus and showMatchBadge('Spool #' +
status.spoolman.spool_id + ' matched'), and only after timeout
showMatchBadge('no Spoolman match') so enrichment fields are not cleared
prematurely.
- Around line 683-707: The code only restores a subset of opentag3d fields and
then triggers matEl.dispatchEvent(new Event('input')) which can auto-fill and
overwrite scanned values; update the restore routine to also set missing fields
exposed by the status API (e.g., modifiers, serial_number, measured_weight_g,
empty_spool_g) via setVal or direct element assignment, and stop silently
triggering the material auto-fill: either remove the matEl.dispatchEvent call or
suppress the input handler while populating (e.g., set a temporary flag the
input handler checks) so restoring ot (and functions like setVal, the matEl
element, and the diameter_um handling) round-trips without losing or altering
density/temperature/weight fields.
In `@src/ReaderHTML.h`:
- Around line 194-205: The Spoolman ID can be rendered twice because the
top-level check s.spoolman_id > 0 always adds a linked row and a later check
s.spoolman.spool_id > 0 may add another row; update the logic so only one
Spoolman ID row is emitted: when s.spoolman && s.spoolman.spool_id > 0, prefer
rendering the spoolman.spool_id (with spoolmanBadge()) and skip the initial
s.spoolman_id link; otherwise fall back to rendering the s.spoolman_id link
(using spoolmanUrl). Modify the conditional around the initial row(...) call
(and/or wrap it with a guard that s.spoolman?.spool_id is falsy) so you don’t
produce duplicate Spoolman ID rows.
In `@src/TigerTagWriterHTML.h`:
- Around line 750-756: The code currently calls fillEnrichmentFromStatus and
immediately shows "no Spoolman match" (and breaks) when status.spoolman is
falsy, but status.tigertag can be set before the asynchronous UID->spoolman
lookup finishes; instead, replace this immediate conclusion with a polling/retry
loop: after fillEnrichmentFromStatus, repeatedly check status.spoolman (and
update UI via showMatchBadge and enrichment fields when it becomes non-null)
until either status.spoolman is present or the read deadline/timeout expires,
and only then show the "no Spoolman match" badge; update the logic around the
break so the read loop continues while waiting and aborts on deadline,
referencing fillEnrichmentFromStatus, status.tigertag, status.spoolman, and
showMatchBadge to find where to change behavior.
- Around line 731-749: The code reads status.tigertag fields (e.g., diameter_mm
and aspect names) but never maps them back into the corresponding ID fields
(diameter_id, aspect1_id, aspect2_id) and then unconditionally calls
syncMaterialId(), which can overwrite an existing material_id when local lists
are stale; update the read path to set the ID inputs when the payload contains
these IDs (e.g., if tt.diameter_id !== undefined → set element 'diameter_id',
and likewise for aspect1_id and aspect2_id) and additionally, when only
human-readable fields exist (diameter_mm, aspect1_name, aspect2_name) perform
the appropriate lookup to convert those into their IDs before writing inputs;
finally, avoid calling syncMaterialId() when material_id is already present (or
call it only as a fallback) so we don’t clobber an exact material_id set from
the payload.
- Around line 820-829: The JSON payload is hardcoding diameter_mm to 0; instead
convert the selected diameter identifier (diameter_id) to its millimeter value
and assign that to diameter_mm before POSTing in the save/enrichment routine
(the block that builds the body JSON.stringify). Locate where diameter_id (or
the select/dropdown value) is read in the same scope and replace the constant 0
with the computed millimeter value (e.g., map/parse diameter_id to 1.75 or 2.85
mm) so the Spoolman filament metadata is created/updated with the correct
diameter.
In `@src/WebServerManager.cpp`:
- Around line 1769-1786: The code currently skips sending remaining_weight
unless remainingG > 0, preventing explicit zero from persisting; in the
WebServerManager handlers that parse doc (where remainingG is read and later
used in create/patch payloads) change the logic to detect presence with
doc.containsKey("remaining_g") and include remaining_weight in the outgoing
payload whenever that key exists (sending 0.0 when the key is present and value
is 0), instead of only when remainingG > 0; update both the create and patch
code paths (the same parsing block around remainingG and the sections referenced
at lines ~1769 and ~1934) to use this presence check when building the request
to SpoolmanManager.
- Around line 1811-1848: The handler currently treats
confirmedVendorId/confirmedFilamentId == -1 the same whether the UI "didn't
check yet" or the user explicitly "declined reuse", causing it to run the dedupe
lookup and attach existing records; fix by introducing and honoring explicit
reuse flags or a distinct sentinel for "declined". Concretely: change the
vendor/filament decision logic around vendorId/confirmedVendorId and
filamentId/confirmedFilamentId so the dedupe GET and fallback reuse blocks only
run when a new boolean (e.g., vendorReuseAllowed / filamentReuseAllowed) is true
(or when the id == -1 meaning "not checked"); treat a new sentinel (e.g., -2) or
vendorReuseAllowed==false as "user declined" and skip the GET/attach logic and
the color-ignoring fallback; ensure the filament fallback (the block referencing
color) additionally checks filamentReuseAllowed and compares color before
attaching an existing filament. Update all branches that set vendorId/filamentId
(including creation POST paths) to respect these flags/sentinels so a declined
choice never reuses an existing record.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4bd18ab1-f425-44c2-aa81-64e8039eb90d
📒 Files selected for processing (12)
.gitignoresrc/ApplicationManager.cppsrc/ApplicationManager.hsrc/OpenPrintTagWriterHTML.hsrc/OpenSpoolWriterHTML.hsrc/OpenTag3DWriterHTML.hsrc/ReaderHTML.hsrc/SpoolmanManager.cppsrc/SpoolmanManager.hsrc/TigerTagWriterHTML.hsrc/WebServerManager.cppsrc/WebServerManager.h
Comment on lines
724
to
+757
| if (display_) { | ||
| if (msg.payload.spoolmanSynced.success && msg.payload.spoolmanSynced.is_uid_lookup) { | ||
| // UID lookup — show spool graphic with Spoolman data (tag had no data) | ||
| DisplaySpoolData spool{}; | ||
| strncpy(spool.brand, msg.payload.spoolmanSynced.manufacturer, sizeof(spool.brand) - 1); | ||
| strncpy(spool.material, materialName, sizeof(spool.material) - 1); | ||
| const char* colorSrc = msg.payload.spoolmanSynced.color_hex; | ||
| if (colorSrc[0] == '#') colorSrc++; | ||
| strncpy(spool.colorHex, colorSrc, sizeof(spool.colorHex) - 1); | ||
| spool.remainingWeight = kgRemaining * 1000.0f; | ||
| spool.totalWeight = msg.payload.spoolmanSynced.initial_weight_g; | ||
| spool.tagType = 5; | ||
| display_->showSpool(spool); | ||
| // Determine if the current tag is a generic UID tag or a smart tag | ||
| #ifndef NATIVE_TEST | ||
| CurrentSpoolState state; | ||
| bool gotState = NFCManager::getInstance().getCurrentSpoolState(state); | ||
| bool isGeneric = !gotState || state.kind == TagKind::GenericUidTag; | ||
| #else | ||
| bool isGeneric = true; | ||
| #endif | ||
| if (isGeneric) { | ||
| // Original path: show spool graphic with Spoolman data (tag had no data) | ||
| DisplaySpoolData spool{}; | ||
| strncpy(spool.brand, msg.payload.spoolmanSynced.manufacturer, sizeof(spool.brand) - 1); | ||
| strncpy(spool.material, materialName, sizeof(spool.material) - 1); | ||
| const char* colorSrc = msg.payload.spoolmanSynced.color_hex; | ||
| if (colorSrc[0] == '#') colorSrc++; | ||
| strncpy(spool.colorHex, colorSrc, sizeof(spool.colorHex) - 1); | ||
| spool.remainingWeight = kgRemaining * 1000.0f; | ||
| spool.totalWeight = msg.payload.spoolmanSynced.initial_weight_g; | ||
| spool.tagType = 5; | ||
| display_->showSpool(spool); | ||
| } else { | ||
| // Smart tag enrichment — store result, don't change display | ||
| smartTagEnrichment_.valid = true; | ||
| smartTagEnrichment_.spoolman_id = msg.payload.spoolmanSynced.spoolman_id; | ||
| smartTagEnrichment_.remaining_g = kgRemaining * 1000.0f; | ||
| smartTagEnrichment_.bed_temp = msg.payload.spoolmanSynced.bed_temp; | ||
| smartTagEnrichment_.extruder_temp = msg.payload.spoolmanSynced.extruder_temp; | ||
| smartTagEnrichment_.density = msg.payload.spoolmanSynced.density; | ||
| smartTagEnrichment_.diameter_mm = msg.payload.spoolmanSynced.diameter_mm; | ||
| Serial.printf("ApplicationManager: Smart tag enrichment stored — spool %d, %.0fg remaining\n", | ||
| smartTagEnrichment_.spoolman_id, smartTagEnrichment_.remaining_g); | ||
| } |
There was a problem hiding this comment.
Ignore stale lookup results, and do not gate the cache on display_.
The new smartTagEnrichment_ write sits inside if (display_), so display-disabled units never populate /api/status.spoolman. This block also only inspects state.kind; it does not verify state.present and state.spool_id == msg.payload.spoolmanSynced.spool_id, so a delayed lookup for tag A can overwrite the cache or LCD while tag B is on the reader. Reuse the same presence/UID guard from Lines 701-709 before updating either the cache or the display.
Also applies to: 761-770
🧰 Tools
🪛 Clang (14.0.6)
[note] 724-724: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 725-725: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 725-725: +1
(clang)
[note] 730-730: +1
(clang)
[note] 734-734: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 740-740: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 746-746: +1, nesting level increased to 3
(clang)
[warning] 724-724: implicit conversion 'DisplayI *' -> bool
(readability-implicit-bool-conversion)
[warning] 740-740: statement should be inside braces
(readability-braces-around-statements)
[warning] 742-742: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` around lines 724 - 757, The smartTagEnrichment_
update is incorrectly gated by if (display_) and lacks the presence/UID guard,
so move the smartTagEnrichment_ writes out of the display_ conditional (so
display-disabled units still populate /api/status.spoolman) and
before/independent of display_->showSpool; additionally reuse the same
CurrentSpoolState presence/UID check used earlier (call
NFCManager::getInstance().getCurrentSpoolState(state) and verify state.present
&& state.spool_id == msg.payload.spoolmanSynced.spool_id) before updating either
smartTagEnrichment_ or calling display_->showSpool to prevent stale/delayed
lookup A from overwriting data for tag B.
| setStepState('step-verify', 'done'); | ||
| setBanner('statusBanner', 'Write complete \u2014 safe to remove tag.'); | ||
| setResult('resultBox', 'OpenTag3D data written and verified successfully.', 'success'); | ||
| await saveEnrichment(presentStatus.uid); |
There was a problem hiding this comment.
Tie enrichment saves to the UID that actually verified.
saveEnrichment(presentStatus.uid) still uses the first detected UID. The verify loop only checks tag_kind === 'OpenTag3D', so swapping to another OpenTag3D tag before verification completes can persist enrichment against the wrong Spoolman spool. Save the verified status.uid, or require the UID to match before declaring success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenTag3DWriterHTML.h` at line 576, The current call to
saveEnrichment(presentStatus.uid) can persist enrichment for the first-detected
UID rather than the UID that passed verification; update the logic so that you
save using the verified UID (e.g., use the UID from the successful verification
result such as status.uid or the variable returned when tag_kind === 'OpenTag3D'
verification succeeds) or add a check that the presentStatus.uid matches the
verified status.uid before calling saveEnrichment; locate the verify loop and
replace the unconditional saveEnrichment(presentStatus.uid) with a save tied to
the verified UID (or guard the save with a UID equality check) so enrichment is
written only for the verified Spoolman spool.
Comment on lines
+683
to
+707
| setVal('base_material', ot.base_material || ''); | ||
| setVal('manufacturer', ot.manufacturer || ''); | ||
| if (ot.color_hex) { | ||
| var c = ot.color_hex.startsWith('#') ? ot.color_hex : '#' + ot.color_hex; | ||
| setVal('colorHex', c); | ||
| setVal('colorPicker', c); | ||
| } | ||
| if (ot.target_weight_g) setVal('target_weight_g', ot.target_weight_g); | ||
| if (ot.density) setVal('density', ot.density); | ||
| if (ot.print_temp) setVal('print_temp_c', ot.print_temp); | ||
| if (ot.bed_temp) setVal('bed_temp_c', ot.bed_temp); | ||
| if (ot.min_print_temp) setVal('min_print_temp_c', ot.min_print_temp); | ||
| if (ot.max_print_temp) setVal('max_print_temp_c', ot.max_print_temp); | ||
| if (ot.min_bed_temp) setVal('min_bed_temp_c', ot.min_bed_temp); | ||
| if (ot.max_bed_temp) setVal('max_bed_temp_c', ot.max_bed_temp); | ||
| if (ot.dry_temp) setVal('max_dry_temp_c', ot.dry_temp); | ||
| if (ot.dry_time_hours) setVal('dry_time_hours', ot.dry_time_hours); | ||
| if (ot.diameter_mm) { | ||
| var dEl = document.getElementById('diameter_um'); | ||
| if (dEl) dEl.value = Math.round(ot.diameter_mm * 1000); | ||
| } | ||
| if (ot.color_name) setVal('color_name', ot.color_name); | ||
| // Trigger material auto-fill | ||
| var matEl = document.getElementById('base_material'); | ||
| if (matEl) matEl.dispatchEvent(new Event('input')); |
There was a problem hiding this comment.
Read → Write is not round-trippable yet.
This path only restores a subset of status.opentag3d and skips fields the status API already exposes, such as modifiers, serial_number, measured_weight_g, and empty_spool_g. Then Line 707 dispatches input, which can auto-fill over the scanned density/temp values. Rewriting an existing tag can silently strip or alter data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenTag3DWriterHTML.h` around lines 683 - 707, The code only restores a
subset of opentag3d fields and then triggers matEl.dispatchEvent(new
Event('input')) which can auto-fill and overwrite scanned values; update the
restore routine to also set missing fields exposed by the status API (e.g.,
modifiers, serial_number, measured_weight_g, empty_spool_g) via setVal or direct
element assignment, and stop silently triggering the material auto-fill: either
remove the matEl.dispatchEvent call or suppress the input handler while
populating (e.g., set a temporary flag the input handler checks) so restoring ot
(and functions like setVal, the matEl element, and the diameter_um handling)
round-trips without losing or altering density/temperature/weight fields.
Comment on lines
+708
to
+714
| fillEnrichmentFromStatus(status); | ||
| if (status.spoolman && status.spoolman.spool_id > 0) { | ||
| showMatchBadge('Spool #' + status.spoolman.spool_id + ' matched'); | ||
| } else { | ||
| showMatchBadge('no Spoolman match'); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Keep polling until the async Spoolman lookup finishes.
The tag payload and status.spoolman arrive on different cycles. Breaking on the first OpenTag3D status and immediately showing no Spoolman match will frequently miss a valid UID lookup and leave the enrichment fields blank. Keep polling until status.spoolman is present or the read timeout expires.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenTag3DWriterHTML.h` around lines 708 - 714, The current handler calls
fillEnrichmentFromStatus and breaks immediately on the first OpenTag3D status,
which can occur before status.spoolman is populated; change the logic in the
OpenTag3D status handling (the block calling fillEnrichmentFromStatus and
showMatchBadge) to continue polling instead of breaking until either
status.spoolman is present (and status.spoolman.spool_id > 0) or the read
timeout expires; when status.spoolman appears call fillEnrichmentFromStatus and
showMatchBadge('Spool #' + status.spoolman.spool_id + ' matched'), and only
after timeout showMatchBadge('no Spoolman match') so enrichment fields are not
cleared prematurely.
Comment on lines
+731
to
+749
| var tt = status.tigertag || {}; | ||
| setVal('material_search', tt.material_name || ''); | ||
| setVal('brand_name', tt.brand_name || ''); | ||
| if (tt.color_hex) { | ||
| var c = tt.color_hex.startsWith('#') ? tt.color_hex : '#' + tt.color_hex; | ||
| setVal('colorHex', c); | ||
| setVal('colorPicker', c); | ||
| } | ||
| if (tt.weight_g) setVal('weight_g', tt.weight_g); | ||
| if (tt.nozzle_temp_min) setVal('nozzle_min', tt.nozzle_temp_min); | ||
| if (tt.nozzle_temp_max) setVal('nozzle_max', tt.nozzle_temp_max); | ||
| if (tt.bed_temp_min) setVal('bed_min', tt.bed_temp_min); | ||
| if (tt.bed_temp_max) setVal('bed_max', tt.bed_temp_max); | ||
| if (tt.dry_temp) setVal('dry_temp', tt.dry_temp); | ||
| if (tt.dry_time_hours) setVal('dry_time', tt.dry_time_hours); | ||
| if (tt.material_id !== undefined) document.getElementById('material_id').value = tt.material_id; | ||
| if (tt.brand_id !== undefined) document.getElementById('brand_id').value = tt.brand_id; | ||
| // Sync material ID from name if not set directly | ||
| syncMaterialId(); |
There was a problem hiding this comment.
Read → Write can change the TigerTag payload.
The status payload already includes diameter_mm and aspect names, but this path never maps them back into diameter_id, aspect1_id, or aspect2_id. Then Line 749 calls syncMaterialId(), which can overwrite the exact material_id from Line 746 when the local list is stale. Read/modify/write will silently change existing tags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TigerTagWriterHTML.h` around lines 731 - 749, The code reads
status.tigertag fields (e.g., diameter_mm and aspect names) but never maps them
back into the corresponding ID fields (diameter_id, aspect1_id, aspect2_id) and
then unconditionally calls syncMaterialId(), which can overwrite an existing
material_id when local lists are stale; update the read path to set the ID
inputs when the payload contains these IDs (e.g., if tt.diameter_id !==
undefined → set element 'diameter_id', and likewise for aspect1_id and
aspect2_id) and additionally, when only human-readable fields exist
(diameter_mm, aspect1_name, aspect2_name) perform the appropriate lookup to
convert those into their IDs before writing inputs; finally, avoid calling
syncMaterialId() when material_id is already present (or call it only as a
fallback) so we don’t clobber an exact material_id set from the payload.
Comment on lines
+750
to
+756
| fillEnrichmentFromStatus(status); | ||
| if (status.spoolman && status.spoolman.spool_id > 0) { | ||
| showMatchBadge('Spool #' + status.spoolman.spool_id + ' matched'); | ||
| } else { | ||
| showMatchBadge('no Spoolman match'); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Do not conclude “no Spoolman match” on the first TigerTag read.
status.tigertag becomes available before the asynchronous UID lookup finishes. Breaking here and showing no Spoolman match means many valid matches will never populate the enrichment fields. Keep polling until status.spoolman is present or the read deadline expires.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TigerTagWriterHTML.h` around lines 750 - 756, The code currently calls
fillEnrichmentFromStatus and immediately shows "no Spoolman match" (and breaks)
when status.spoolman is falsy, but status.tigertag can be set before the
asynchronous UID->spoolman lookup finishes; instead, replace this immediate
conclusion with a polling/retry loop: after fillEnrichmentFromStatus, repeatedly
check status.spoolman (and update UI via showMatchBadge and enrichment fields
when it becomes non-null) until either status.spoolman is present or the read
deadline/timeout expires, and only then show the "no Spoolman match" badge;
update the logic around the break so the read loop continues while waiting and
aborts on deadline, referencing fillEnrichmentFromStatus, status.tigertag,
status.spoolman, and showMatchBadge to find where to change behavior.
Comment on lines
+820
to
+829
| body: JSON.stringify({ | ||
| uid: uid, | ||
| manufacturer: manufacturer, | ||
| material: material, | ||
| color_hex: colorHex.replace('#', ''), | ||
| remaining_g: remainingG, | ||
| bed_temp: bedTemp, | ||
| nozzle_temp: nozzleTemp, | ||
| diameter_mm: 0, | ||
| density: density, |
There was a problem hiding this comment.
Preserve the selected filament diameter when saving enrichment.
diameter_mm is hard-coded to 0, so a 2.85 mm TigerTag cannot create or update the correct Spoolman filament metadata. Convert diameter_id to millimeters before posting.
Suggested fix
- diameter_mm: 0,
+ diameter_mm: intVal('diameter_id', 56) === 221 ? 2.85 : 1.75,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| body: JSON.stringify({ | |
| uid: uid, | |
| manufacturer: manufacturer, | |
| material: material, | |
| color_hex: colorHex.replace('#', ''), | |
| remaining_g: remainingG, | |
| bed_temp: bedTemp, | |
| nozzle_temp: nozzleTemp, | |
| diameter_mm: 0, | |
| density: density, | |
| body: JSON.stringify({ | |
| uid: uid, | |
| manufacturer: manufacturer, | |
| material: material, | |
| color_hex: colorHex.replace('#', ''), | |
| remaining_g: remainingG, | |
| bed_temp: bedTemp, | |
| nozzle_temp: nozzleTemp, | |
| diameter_mm: intVal('diameter_id', 56) === 221 ? 2.85 : 1.75, | |
| density: density, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TigerTagWriterHTML.h` around lines 820 - 829, The JSON payload is
hardcoding diameter_mm to 0; instead convert the selected diameter identifier
(diameter_id) to its millimeter value and assign that to diameter_mm before
POSTing in the save/enrichment routine (the block that builds the body
JSON.stringify). Locate where diameter_id (or the select/dropdown value) is read
in the same scope and replace the constant 0 with the computed millimeter value
(e.g., map/parse diameter_id to 1.75 or 2.85 mm) so the Spoolman filament
metadata is created/updated with the correct diameter.
Comment on lines
+1769
to
+1786
| StaticJsonDocument<512> doc; | ||
| if (deserializeJson(doc, _server.arg("plain"))) { | ||
| _server.send(400, "application/json", "{\"error\":\"bad JSON\"}"); | ||
| return; | ||
| } | ||
|
|
||
| if (!SpoolmanManager::getInstance().isConfigured()) { | ||
| _server.send(503, "application/json", "{\"error\":\"Spoolman not configured\"}"); | ||
| return; | ||
| } | ||
|
|
||
| const char* uid = doc["uid"] | ""; | ||
| const char* manufacturer = doc["manufacturer"] | ""; | ||
| const char* material = doc["material"] | ""; | ||
| const char* colorHex = doc["color_hex"] | ""; | ||
| float diameter = doc["diameter_mm"] | 1.75f; | ||
| float density = doc["density"] | 0.0f; | ||
| float remainingG = doc["remaining_g"] | 0.0f; |
There was a problem hiding this comment.
Allow remaining_g = 0 to persist.
The writer UI accepts zero, but both the patch and create paths only send remaining_weight when remainingG > 0. That makes it impossible to save an empty spool; Spoolman will keep the previous weight instead. Track field presence with doc.containsKey("remaining_g") and send zero when it is explicitly provided.
Suggested fix
StaticJsonDocument<512> doc;
if (deserializeJson(doc, _server.arg("plain"))) {
_server.send(400, "application/json", "{\"error\":\"bad JSON\"}");
return;
}
+ const bool hasRemainingG = doc.containsKey("remaining_g");
@@
- float remainingG = doc["remaining_g"] | 0.0f;
+ float remainingG = doc["remaining_g"] | 0.0f;
@@
- if (remainingG > 0) patch["remaining_weight"] = remainingG;
+ if (hasRemainingG) patch["remaining_weight"] = remainingG;
@@
- if (remainingG > 0) sBody["remaining_weight"] = remainingG;
+ if (hasRemainingG) sBody["remaining_weight"] = remainingG;Also applies to: 1934-1956
🧰 Tools
🪛 Clang (14.0.6)
[note] 1770-1770: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1775-1775: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[warning] 1769-1769: variable 'doc' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1780-1780: variable 'uid' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1781-1781: variable 'manufacturer' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1782-1782: variable 'material' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1783-1783: variable 'colorHex' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1784-1784: variable 'diameter' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1785-1785: variable 'density' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1786-1786: variable 'remainingG' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.cpp` around lines 1769 - 1786, The code currently skips
sending remaining_weight unless remainingG > 0, preventing explicit zero from
persisting; in the WebServerManager handlers that parse doc (where remainingG is
read and later used in create/patch payloads) change the logic to detect
presence with doc.containsKey("remaining_g") and include remaining_weight in the
outgoing payload whenever that key exists (sending 0.0 when the key is present
and value is 0), instead of only when remainingG > 0; update both the create and
patch code paths (the same parsing block around remainingG and the sections
referenced at lines ~1769 and ~1934) to use this presence check when building
the request to SpoolmanManager.
Comment on lines
+1811
to
+1848
| int vendorId = confirmedVendorId; | ||
| if (vendorId < 0 && manufacturer[0] != '\0') { | ||
| String encodedMfg = urlEncode(manufacturer); | ||
| snprintf(url, sizeof(url), "%s/api/v1/vendor?name=%s", baseUrl, encodedMfg.c_str()); | ||
| http.begin(client, url); | ||
| http.setTimeout(5000); | ||
| code = http.GET(); | ||
| if (code == 200) { | ||
| response = http.getString(); | ||
| DynamicJsonDocument vDoc(2048); | ||
| if (!deserializeJson(vDoc, response)) { | ||
| for (JsonObject v : vDoc.as<JsonArray>()) { | ||
| if (strcasecmp(v["name"] | "", manufacturer) == 0) { | ||
| vendorId = v["id"] | -1; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| http.end(); | ||
|
|
||
| if (vendorId < 0) { | ||
| StaticJsonDocument<128> vBody; | ||
| vBody["name"] = manufacturer; | ||
| String vJson; | ||
| serializeJson(vBody, vJson); | ||
| snprintf(url, sizeof(url), "%s/api/v1/vendor", baseUrl); | ||
| http.begin(client, url); | ||
| http.setTimeout(5000); | ||
| http.addHeader("Content-Type", "application/json"); | ||
| code = http.POST(vJson); | ||
| if (code == 200 || code == 201) { | ||
| response = http.getString(); | ||
| StaticJsonDocument<256> vResp; | ||
| if (!deserializeJson(vResp, response)) vendorId = vResp["id"] | -1; | ||
| } | ||
| http.end(); | ||
| } |
There was a problem hiding this comment.
The backend currently ignores a user’s “don’t reuse this vendor/filament” choice.
When the UI leaves vendor_id or filament_id at -1 after the user declines confirmation, this handler reruns the dedupe search and reuses the first existing record anyway. The filament fallback at Lines 1855-1869 also ignores color, so a decline on a color mismatch can still attach the spool to the wrong filament. Use separate “reuse allowed” flags, or distinct sentinel values for “declined” vs “not checked”.
Also applies to: 1852-1874
🧰 Tools
🪛 Clang (14.0.6)
[note] 1812-1812: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1812-1812: +1
(clang)
[note] 1818-1818: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1821-1821: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1832-1832: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1842-1842: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1842-1842: +1
(clang)
[warning] 1813-1813: variable 'encodedMfg' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1820-1820: variable 'vDoc' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1833-1833: variable 'vBody' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1835-1835: variable 'vJson' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1844-1844: variable 'vResp' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.cpp` around lines 1811 - 1848, The handler currently
treats confirmedVendorId/confirmedFilamentId == -1 the same whether the UI
"didn't check yet" or the user explicitly "declined reuse", causing it to run
the dedupe lookup and attach existing records; fix by introducing and honoring
explicit reuse flags or a distinct sentinel for "declined". Concretely: change
the vendor/filament decision logic around vendorId/confirmedVendorId and
filamentId/confirmedFilamentId so the dedupe GET and fallback reuse blocks only
run when a new boolean (e.g., vendorReuseAllowed / filamentReuseAllowed) is true
(or when the id == -1 meaning "not checked"); treat a new sentinel (e.g., -2) or
vendorReuseAllowed==false as "user declined" and skip the GET/attach logic and
the color-ignoring fallback; ensure the filament fallback (the block referencing
color) additionally checks filamentReuseAllowed and compares color before
attaching an existing filament. Update all branches that set vendorId/filamentId
(including creation POST paths) to respect these flags/sentinels so a declined
choice never reuses an existing record.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenSpoolWriterHTML.h`:
- Around line 250-253: In cancelRead(), remove the redundant direct assignment
to the module/state variable by deleting "readWaiting = false" and rely solely
on the setReadWaiting(false) call; update the cancelRead function (symbol:
cancelRead) so it only calls setReadWaiting(false) to clear the waiting flag.
- Around line 266-272: The function fillEnrichmentFromStatus calls
sp.remaining_g.toFixed(1) without guarding against null; change the check in
fillEnrichmentFromStatus so it ensures sp.remaining_g is not null or undefined
(e.g., use a null-safe check like sp.remaining_g != null) before calling
.toFixed(1), preventing a TypeError when remaining_g is null and keeping the
rest of the field checks (sp.bed_temp, sp.diameter_mm, sp.density) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d52a5fac-813e-4734-ad14-46f6035fefe4
📒 Files selected for processing (1)
src/OpenSpoolWriterHTML.h
Comment on lines
+250
to
+253
| function cancelRead() { | ||
| readWaiting = false; | ||
| setReadWaiting(false); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor redundancy in cancelRead.
The direct assignment readWaiting = false on line 251 is redundant since setReadWaiting(false) on line 252 already sets readWaiting = active (which is false).
🔧 Suggested simplification
function cancelRead() {
- readWaiting = false;
setReadWaiting(false);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function cancelRead() { | |
| readWaiting = false; | |
| setReadWaiting(false); | |
| } | |
| function cancelRead() { | |
| setReadWaiting(false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenSpoolWriterHTML.h` around lines 250 - 253, In cancelRead(), remove
the redundant direct assignment to the module/state variable by deleting
"readWaiting = false" and rely solely on the setReadWaiting(false) call; update
the cancelRead function (symbol: cancelRead) so it only calls
setReadWaiting(false) to clear the waiting flag.
Comment on lines
+266
to
+272
| function fillEnrichmentFromStatus(status) { | ||
| var sp = status.spoolman || {}; | ||
| if (sp.remaining_g !== undefined) setVal('enrich-remaining', sp.remaining_g.toFixed(1)); | ||
| if (sp.bed_temp && sp.bed_temp > 0) setVal('enrich-bed-temp', sp.bed_temp); | ||
| if (sp.diameter_mm && sp.diameter_mm > 0) setVal('enrich-diameter', sp.diameter_mm); | ||
| if (sp.density && sp.density > 0) setVal('enrich-density', sp.density); | ||
| } |
There was a problem hiding this comment.
Add null check for remaining_g to prevent potential TypeError.
Line 268 checks !== undefined but not !== null. If remaining_g is null, calling .toFixed(1) will throw a TypeError.
🛡️ Proposed defensive fix
function fillEnrichmentFromStatus(status) {
var sp = status.spoolman || {};
- if (sp.remaining_g !== undefined) setVal('enrich-remaining', sp.remaining_g.toFixed(1));
+ if (sp.remaining_g != null) setVal('enrich-remaining', sp.remaining_g.toFixed(1));
if (sp.bed_temp && sp.bed_temp > 0) setVal('enrich-bed-temp', sp.bed_temp);
if (sp.diameter_mm && sp.diameter_mm > 0) setVal('enrich-diameter', sp.diameter_mm);
if (sp.density && sp.density > 0) setVal('enrich-density', sp.density);
}Using != null checks both null and undefined in one expression.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function fillEnrichmentFromStatus(status) { | |
| var sp = status.spoolman || {}; | |
| if (sp.remaining_g !== undefined) setVal('enrich-remaining', sp.remaining_g.toFixed(1)); | |
| if (sp.bed_temp && sp.bed_temp > 0) setVal('enrich-bed-temp', sp.bed_temp); | |
| if (sp.diameter_mm && sp.diameter_mm > 0) setVal('enrich-diameter', sp.diameter_mm); | |
| if (sp.density && sp.density > 0) setVal('enrich-density', sp.density); | |
| } | |
| function fillEnrichmentFromStatus(status) { | |
| var sp = status.spoolman || {}; | |
| if (sp.remaining_g != null) setVal('enrich-remaining', sp.remaining_g.toFixed(1)); | |
| if (sp.bed_temp && sp.bed_temp > 0) setVal('enrich-bed-temp', sp.bed_temp); | |
| if (sp.diameter_mm && sp.diameter_mm > 0) setVal('enrich-diameter', sp.diameter_mm); | |
| if (sp.density && sp.density > 0) setVal('enrich-density', sp.density); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenSpoolWriterHTML.h` around lines 266 - 272, The function
fillEnrichmentFromStatus calls sp.remaining_g.toFixed(1) without guarding
against null; change the check in fillEnrichmentFromStatus so it ensures
sp.remaining_g is not null or undefined (e.g., use a null-safe check like
sp.remaining_g != null) before calling .toFixed(1), preventing a TypeError when
remaining_g is null and keeping the rest of the field checks (sp.bed_temp,
sp.diameter_mm, sp.density) unchanged.
…val, writer section placement, spool picker UX
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/TigerTagWriterHTML.h (1)
617-627:⚠️ Potential issue | 🟠 MajorVerify the same physical tag before saving enrichment.
Line 619 only compares
material_idandbrand_id, then Line 626 saves againstpresentStatus.uid. Another TigerTag with the same material and brand can satisfy this and cause enrichment to be written to the wrong UID.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TigerTagWriterHTML.h` around lines 617 - 627, The current verification only checks tt.material_id and tt.brand_id so a different physical tag with the same material/brand could pass; update the check in the block that guards setBanner/setResult/saveEnrichment to also verify the tag's unique identifier (e.g., compare tt.uid or status.uid against presentStatus.uid) before proceeding, and ensure saveEnrichment is called with that verified UID (not just presentStatus.uid blindly) so enrichment is saved to the exact verified tag.src/OpenSpoolWriterHTML.h (1)
465-476:⚠️ Potential issue | 🟠 MajorTie success and enrichment saves to the verified UID.
The verify loop accepts any
OpenSpoolTag, thensaveEnrichment(presentStatus.uid)writes against the first detected UID. If the user swaps tags before verification completes, the wrong tag can be marked successful and the wrong Spoolman spool updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenSpoolWriterHTML.h` around lines 465 - 476, The verification loop currently accepts any OpenSpoolTag but later calls saveEnrichment(presentStatus.uid), which can use a stale/other tag UID; fix by capturing the verified tag's UID from the verified response (the local status variable returned by api('/api/status')) when the condition status.present && status.tag_kind === 'OpenSpoolTag' is met, store it in a local constant (e.g. verifiedUid = status.uid or status.present.uid depending on response shape), and use that verifiedUid for all subsequent actions (setBanner messages, setStepState, setResult, saveEnrichment(verifiedUid), and showing backBtn) instead of referencing presentStatus.
♻️ Duplicate comments (9)
src/OpenTag3DWriterHTML.h (3)
569-576:⚠️ Potential issue | 🟠 MajorTie enrichment saves to the UID that actually verified.
The loop still accepts any
OpenTag3Dstatus, thensaveEnrichment(presentStatus.uid)persists against the first detected UID. Swapping tags before verification finishes can update the wrong spool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenTag3DWriterHTML.h` around lines 569 - 576, The code uses presentStatus.uid when calling saveEnrichment but verification accepts any status and can race if a different tag appears; capture the UID from the specific status that passed verification (the local status object used in the if-check) before awaiting sleep and use that captured UID in saveEnrichment so the enrichment is saved against the exact tag that was verified (i.e., store e.g. verifiedUid = status.uid immediately after the if condition and call saveEnrichment(verifiedUid) later).
707-713:⚠️ Potential issue | 🟠 MajorDon't conclude “no Spoolman match” on the first OpenTag3D read.
status.opentag3dis populated before the async UID lookup finishes. Breaking here frequently misses valid matches and leavesenrich-remainingblank.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenTag3DWriterHTML.h` around lines 707 - 713, The code currently calls fillEnrichmentFromStatus(status) and immediately shows "no Spoolman match" if status.spoolman is missing, but status.opentag3d's async UID lookup may still be in progress; update the logic in the block containing fillEnrichmentFromStatus, showMatchBadge and the break so you do not display or conclude "no Spoolman match" until the async lookup completes: remove or guard the else branch that calls showMatchBadge('no Spoolman match') and instead only call showMatchBadge('Spool #... matched') when status.spoolman is present, and add a check for the lookup-in-progress state on status.opentag3d (or a pending flag from the UID lookup) to defer the negative badge until the lookup finishes; keep using fillEnrichmentFromStatus, status.spoolman and status.opentag3d as the identifying symbols to locate the change.
681-706:⚠️ Potential issue | 🟠 MajorRead → Write still is not round-trippable.
This restore path still skips fields the API already exposes, such as
modifiers,serial_number,measured_weight_g, andempty_spool_g. The syntheticinputevent at Line 706 can also auto-fill over the scanned density and temperature values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenTag3DWriterHTML.h` around lines 681 - 706, The restore code in the status.opentag3d block is skipping API-exposed fields (modifiers, serial_number, measured_weight_g, empty_spool_g) and then fires a synthetic input on the base_material element which can overwrite restored density/temperature values; update the restore to call setVal for modifiers, serial_number, measured_weight_g, and empty_spool_g (use the same setVal helper used for base_material/colorHex/etc.), populate the diameter_um element as you already do, and only dispatch the auto-fill input on base_material after all these fields are set — or better, avoid dispatching a plain Event('input') and instead call the specific autofill function or dispatch a namespaced/custom event so it does not clobber restored density/print_temp/bed_temp values (reference status.opentag3d, setVal, element IDs: base_material, modifiers, serial_number, measured_weight_g, empty_spool_g, colorHex, colorPicker, density, print_temp_c, bed_temp_c, diameter_um).src/TigerTagWriterHTML.h (3)
730-748:⚠️ Potential issue | 🟠 MajorRead → Write can still change the TigerTag payload.
This path still never restores
diameter_id,aspect1_id, oraspect2_id, and the unconditionalsyncMaterialId()can overwrite an exactmaterial_idfrom the tag when the local material list is stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TigerTagWriterHTML.h` around lines 730 - 748, The read path currently mutates the TigerTag payload by not restoring diameter_id, aspect1_id, and aspect2_id and by always calling syncMaterialId(), which can overwrite a valid material_id; update the block that reads status.tigertag (tt) so that if tt.diameter_id, tt.aspect1_id, or tt.aspect2_id are present you set the corresponding DOM elements (document.getElementById('diameter_id').value, 'aspect1_id', 'aspect2_id') just like you do for material_id/brand_id, and change the unconditional call to syncMaterialId() to only run when tt.material_id is undefined (i.e., call syncMaterialId() only if no explicit material_id was restored) to avoid overwriting an exact material_id from the tag.
749-755:⚠️ Potential issue | 🟠 MajorDon't conclude “no Spoolman match” on the first TigerTag read.
status.tigertagbecomes available before the async UID lookup finishes. Breaking here will frequently miss a valid match and leave the enrichment fields empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TigerTagWriterHTML.h` around lines 749 - 755, The code currently shows "no Spoolman match" as soon as status.tigertag is present which happens before the async UID lookup completes; update the logic in the block containing fillEnrichmentFromStatus, status.spoolman and showMatchBadge so you do not conclude "no Spoolman match" prematurely: only call showMatchBadge('no Spoolman match') when the async lookup has finished and status.spoolman is explicitly null (e.g. check a sentinel like status.spoolman === null or a lookup-complete flag), otherwise omit the else branch (or show a "lookup pending" state) and let fillEnrichmentFromStatus / the async callback decide and call showMatchBadge on success/failure. Ensure you reference and update the handling around fillEnrichmentFromStatus, status.tigertag, status.spoolman and showMatchBadge accordingly.
819-828:⚠️ Potential issue | 🟠 MajorSend the selected filament diameter to Spoolman.
diameter_mmis still hard-coded to0, so 2.85 mm tags can create or update the wrong filament metadata in Spoolman.Suggested fix
- diameter_mm: 0, + diameter_mm: intVal('diameter_id', 56) === 221 ? 2.85 : 1.75,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TigerTagWriterHTML.h` around lines 819 - 828, The POST payload currently hard-codes diameter_mm: 0 when building the body JSON; replace that literal with the actual selected filament diameter variable used elsewhere in this scope (e.g., diameter, diameterMm, selectedDiameter) so Spoolman receives the real diameter in millimeters; ensure you convert/parse the UI value to a numeric mm value (strip units/strings if necessary) before inserting it into diameter_mm in the object being JSON.stringify'd (the object containing uid, manufacturer, material, color_hex, remaining_g, bed_temp, nozzle_temp, density).src/ApplicationManager.cpp (1)
724-770:⚠️ Potential issue | 🟠 MajorDon't gate the smart-tag cache on
display_, and ignore stale UID lookups.
smartTagEnrichment_is still only written insideif (display_), so headless units never populate/api/status.spoolman. This branch also doesn't verifystate.present && strcmp(state.spool_id, msg.payload.spoolmanSynced.spool_id) == 0before caching, so a delayed lookup for tag A can overwrite the enrichment while tag B is on the reader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ApplicationManager.cpp` around lines 724 - 770, smartTagEnrichment_ is only updated inside the display_ check and lacks validation against the currently-present tag, so headless devices don't populate enrichment and delayed UID lookups can overwrite data for another tag; move the smart-tag caching out of the if (display_) block so smartTagEnrichment_ is updated regardless of display_, and before writing validate the current NFC state via NFCManager::getInstance().getCurrentSpoolState(state) ensuring gotState && state.present && strcmp(state.spool_id, msg.payload.spoolmanSynced.spool_id) == 0 (and that state.kind != TagKind::GenericUidTag when appropriate) before assigning smartTagEnrichment_.spoolman_id, remaining_g, bed_temp, extruder_temp, density, diameter_mm and smartTagEnrichment_.valid.src/WebServerManager.cpp (2)
1803-1804:⚠️ Potential issue | 🟠 MajorThe backend still ignores a user’s “don't reuse this vendor/filament” choice.
vendor_idandfilament_idof-1are treated as “search again”, so after the UI declines reuse this handler still re-runs the dedupe lookup and can attach the existing vendor or filament anyway. The filament fallback here also ignorescolor_hex, so a decline on a color mismatch can still bind the wrong filament.Also applies to: 1825-1919
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 1803 - 1804, The handler currently treats confirmedVendorId/confirmedFilamentId == -1 as "search again" and still re-runs dedupe/attach logic (and the filament fallback ignores color_hex), so change the flow so that when doc["vendor_id"] or doc["filament_id"] yields -1 the code explicitly skips any dedupe/lookup and does not attach an existing vendor/filament; specifically, update the logic that reads confirmedVendorId and confirmedFilamentId to treat -1 as "do not reuse" (do not call the dedupe/search functions or assign existing IDs), and in the filament fallback path ensure you check and enforce color_hex match before attaching any existing filament (if color_hex mismatches, do not bind the existing filament). Use the variables confirmedVendorId and confirmedFilamentId and the filament fallback/dedupe attachment code paths to implement these checks.
1783-1800:⚠️ Potential issue | 🟠 MajorAllow
remaining_g = 0to persist.The handler still only sends
remaining_weightwhenremainingG > 0in both the PATCH and create paths. That makes it impossible to save an empty spool; Spoolman keeps the previous remaining weight instead.Suggested fix
StaticJsonDocument<512> doc; if (deserializeJson(doc, _server.arg("plain"))) { _server.send(400, "application/json", "{\"error\":\"bad JSON\"}"); return; } + const bool hasRemainingG = doc.containsKey("remaining_g"); @@ - if (remainingG > 0) patch["remaining_weight"] = remainingG; + if (hasRemainingG) patch["remaining_weight"] = remainingG; @@ - if (remainingG > 0) sBody["remaining_weight"] = remainingG; + if (hasRemainingG) sBody["remaining_weight"] = remainingG;Also applies to: 1950-1970
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 1783 - 1800, The handler currently treats remaining_g==0 as "not provided" because remainingG is defaulted to 0 and code only sends remaining_weight when remainingG>0; change the logic to check whether the JSON actually contained the key instead of its numeric value: use doc.containsKey("remaining_g") (or equivalent) and, when true, include remaining_weight (even if zero) in both the create and PATCH code paths that reference remainingG; apply the same change to the duplicate block around the 1950-1970 region so empty spools can be persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenSpoolWriterHTML.h`:
- Around line 279-295: The code exits the status-handling loop as soon as an
OpenSpool status arrives, causing "no Spoolman match" to be shown if the async
spoolman enrichment hasn't completed; instead, defer concluding "no match" until
the enrichment or the overall read timeout finishes. Modify the OpenSpoolTag
branch (the block that reads status.openspool, calls fillEnrichmentFromStatus
and showMatchBadge) to NOT break immediately when status.spoolman is missing;
rather poll/retry/check for status.spoolman completion (or use the existing
read-timeout mechanism) and only call showMatchBadge('no Spoolman match') after
the timeout elapses, while still updating brand/type/color and calling
fillEnrichmentFromStatus when data is available. Ensure you remove the premature
break so the loop continues until spoolman is resolved or timeout is reached.
In `@src/ReaderHTML.h`:
- Around line 377-393: pollForEnrichment() spawns its own interval that can
outlive pollTimer, causing stale enrichment results to be applied to a new scan;
change it to either (A) store its interval id in the shared pollTimer and
clear/replace that in startPolling()/stopPolling(), or (B) capture a unique
identifier of the tag (e.g., the scanned tag's id from lastTagStatus) when
pollForEnrichment() is started and, inside the api('/api/status') callback,
verify the id still matches lastTagStatus before merging s.spoolman and calling
render(lastTagStatus); also ensure all created timers are cleared on
stopPolling() so no stale interval can update the UI.
In `@src/WebServerManager.cpp`:
- Around line 1206-1218: The emitted spoolman enrichment is ambiguous because
SmartTagEnrichment lacks the tag UID, so change the logic to only persist
enrichment when it is tied to the active tag: add a UID field to
SmartTagEnrichment (e.g., enrichment.uid), populate it when creating/updating
the enrichment in ApplicationManager, and in WebServerManager.cpp verify that
enrichment.valid && enrichment.uid == currentTagUid (or that a non-empty active
tag is present) before creating the "spoolman" JsonObject; alternatively, if
adding a UID is not possible, only emit enrichment when doc contains an explicit
tag identifier (i.e., require the current scan/tag to be present) to prevent
leaking stale values.
---
Outside diff comments:
In `@src/OpenSpoolWriterHTML.h`:
- Around line 465-476: The verification loop currently accepts any OpenSpoolTag
but later calls saveEnrichment(presentStatus.uid), which can use a stale/other
tag UID; fix by capturing the verified tag's UID from the verified response (the
local status variable returned by api('/api/status')) when the condition
status.present && status.tag_kind === 'OpenSpoolTag' is met, store it in a local
constant (e.g. verifiedUid = status.uid or status.present.uid depending on
response shape), and use that verifiedUid for all subsequent actions (setBanner
messages, setStepState, setResult, saveEnrichment(verifiedUid), and showing
backBtn) instead of referencing presentStatus.
In `@src/TigerTagWriterHTML.h`:
- Around line 617-627: The current verification only checks tt.material_id and
tt.brand_id so a different physical tag with the same material/brand could pass;
update the check in the block that guards setBanner/setResult/saveEnrichment to
also verify the tag's unique identifier (e.g., compare tt.uid or status.uid
against presentStatus.uid) before proceeding, and ensure saveEnrichment is
called with that verified UID (not just presentStatus.uid blindly) so enrichment
is saved to the exact verified tag.
---
Duplicate comments:
In `@src/ApplicationManager.cpp`:
- Around line 724-770: smartTagEnrichment_ is only updated inside the display_
check and lacks validation against the currently-present tag, so headless
devices don't populate enrichment and delayed UID lookups can overwrite data for
another tag; move the smart-tag caching out of the if (display_) block so
smartTagEnrichment_ is updated regardless of display_, and before writing
validate the current NFC state via
NFCManager::getInstance().getCurrentSpoolState(state) ensuring gotState &&
state.present && strcmp(state.spool_id, msg.payload.spoolmanSynced.spool_id) ==
0 (and that state.kind != TagKind::GenericUidTag when appropriate) before
assigning smartTagEnrichment_.spoolman_id, remaining_g, bed_temp, extruder_temp,
density, diameter_mm and smartTagEnrichment_.valid.
In `@src/OpenTag3DWriterHTML.h`:
- Around line 569-576: The code uses presentStatus.uid when calling
saveEnrichment but verification accepts any status and can race if a different
tag appears; capture the UID from the specific status that passed verification
(the local status object used in the if-check) before awaiting sleep and use
that captured UID in saveEnrichment so the enrichment is saved against the exact
tag that was verified (i.e., store e.g. verifiedUid = status.uid immediately
after the if condition and call saveEnrichment(verifiedUid) later).
- Around line 707-713: The code currently calls fillEnrichmentFromStatus(status)
and immediately shows "no Spoolman match" if status.spoolman is missing, but
status.opentag3d's async UID lookup may still be in progress; update the logic
in the block containing fillEnrichmentFromStatus, showMatchBadge and the break
so you do not display or conclude "no Spoolman match" until the async lookup
completes: remove or guard the else branch that calls showMatchBadge('no
Spoolman match') and instead only call showMatchBadge('Spool #... matched') when
status.spoolman is present, and add a check for the lookup-in-progress state on
status.opentag3d (or a pending flag from the UID lookup) to defer the negative
badge until the lookup finishes; keep using fillEnrichmentFromStatus,
status.spoolman and status.opentag3d as the identifying symbols to locate the
change.
- Around line 681-706: The restore code in the status.opentag3d block is
skipping API-exposed fields (modifiers, serial_number, measured_weight_g,
empty_spool_g) and then fires a synthetic input on the base_material element
which can overwrite restored density/temperature values; update the restore to
call setVal for modifiers, serial_number, measured_weight_g, and empty_spool_g
(use the same setVal helper used for base_material/colorHex/etc.), populate the
diameter_um element as you already do, and only dispatch the auto-fill input on
base_material after all these fields are set — or better, avoid dispatching a
plain Event('input') and instead call the specific autofill function or dispatch
a namespaced/custom event so it does not clobber restored
density/print_temp/bed_temp values (reference status.opentag3d, setVal, element
IDs: base_material, modifiers, serial_number, measured_weight_g, empty_spool_g,
colorHex, colorPicker, density, print_temp_c, bed_temp_c, diameter_um).
In `@src/TigerTagWriterHTML.h`:
- Around line 730-748: The read path currently mutates the TigerTag payload by
not restoring diameter_id, aspect1_id, and aspect2_id and by always calling
syncMaterialId(), which can overwrite a valid material_id; update the block that
reads status.tigertag (tt) so that if tt.diameter_id, tt.aspect1_id, or
tt.aspect2_id are present you set the corresponding DOM elements
(document.getElementById('diameter_id').value, 'aspect1_id', 'aspect2_id') just
like you do for material_id/brand_id, and change the unconditional call to
syncMaterialId() to only run when tt.material_id is undefined (i.e., call
syncMaterialId() only if no explicit material_id was restored) to avoid
overwriting an exact material_id from the tag.
- Around line 749-755: The code currently shows "no Spoolman match" as soon as
status.tigertag is present which happens before the async UID lookup completes;
update the logic in the block containing fillEnrichmentFromStatus,
status.spoolman and showMatchBadge so you do not conclude "no Spoolman match"
prematurely: only call showMatchBadge('no Spoolman match') when the async lookup
has finished and status.spoolman is explicitly null (e.g. check a sentinel like
status.spoolman === null or a lookup-complete flag), otherwise omit the else
branch (or show a "lookup pending" state) and let fillEnrichmentFromStatus / the
async callback decide and call showMatchBadge on success/failure. Ensure you
reference and update the handling around fillEnrichmentFromStatus,
status.tigertag, status.spoolman and showMatchBadge accordingly.
- Around line 819-828: The POST payload currently hard-codes diameter_mm: 0 when
building the body JSON; replace that literal with the actual selected filament
diameter variable used elsewhere in this scope (e.g., diameter, diameterMm,
selectedDiameter) so Spoolman receives the real diameter in millimeters; ensure
you convert/parse the UI value to a numeric mm value (strip units/strings if
necessary) before inserting it into diameter_mm in the object being
JSON.stringify'd (the object containing uid, manufacturer, material, color_hex,
remaining_g, bed_temp, nozzle_temp, density).
In `@src/WebServerManager.cpp`:
- Around line 1803-1804: The handler currently treats
confirmedVendorId/confirmedFilamentId == -1 as "search again" and still re-runs
dedupe/attach logic (and the filament fallback ignores color_hex), so change the
flow so that when doc["vendor_id"] or doc["filament_id"] yields -1 the code
explicitly skips any dedupe/lookup and does not attach an existing
vendor/filament; specifically, update the logic that reads confirmedVendorId and
confirmedFilamentId to treat -1 as "do not reuse" (do not call the dedupe/search
functions or assign existing IDs), and in the filament fallback path ensure you
check and enforce color_hex match before attaching any existing filament (if
color_hex mismatches, do not bind the existing filament). Use the variables
confirmedVendorId and confirmedFilamentId and the filament fallback/dedupe
attachment code paths to implement these checks.
- Around line 1783-1800: The handler currently treats remaining_g==0 as "not
provided" because remainingG is defaulted to 0 and code only sends
remaining_weight when remainingG>0; change the logic to check whether the JSON
actually contained the key instead of its numeric value: use
doc.containsKey("remaining_g") (or equivalent) and, when true, include
remaining_weight (even if zero) in both the create and PATCH code paths that
reference remainingG; apply the same change to the duplicate block around the
1950-1970 region so empty spools can be persisted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c638bfcb-ff05-47cd-ac0f-fe6451a9993c
📒 Files selected for processing (7)
src/ApplicationManager.cppsrc/OpenSpoolWriterHTML.hsrc/OpenTag3DWriterHTML.hsrc/ReaderHTML.hsrc/SharedJS.hsrc/TigerTagWriterHTML.hsrc/WebServerManager.cpp
Comment on lines
+279
to
+295
| if (status.present && status.tag_kind === 'OpenSpoolTag') { | ||
| var os = status.openspool || {}; | ||
| setVal('brand', os.brand || ''); | ||
| setVal('type', os.material || ''); | ||
| if (os.color_hex) { | ||
| setVal('colorHex', os.color_hex); | ||
| setVal('colorPicker', os.color_hex.startsWith('#') ? os.color_hex : '#' + os.color_hex); | ||
| } | ||
| if (os.min_temp) setVal('min_temp', os.min_temp); | ||
| if (os.max_temp) setVal('max_temp', os.max_temp); | ||
| fillEnrichmentFromStatus(status); | ||
| if (status.spoolman && status.spoolman.spool_id > 0) { | ||
| showMatchBadge('Spool #' + status.spoolman.spool_id + ' matched'); | ||
| } else { | ||
| showMatchBadge('no Spoolman match'); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Keep polling until the Spoolman UID lookup finishes.
status.openspool can arrive before the async status.spoolman enrichment. Breaking on the first OpenSpool read and immediately showing “no Spoolman match” will miss valid matches and leave the enrichment fields empty. Only conclude “no match” once the read timeout expires.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenSpoolWriterHTML.h` around lines 279 - 295, The code exits the
status-handling loop as soon as an OpenSpool status arrives, causing "no
Spoolman match" to be shown if the async spoolman enrichment hasn't completed;
instead, defer concluding "no match" until the enrichment or the overall read
timeout finishes. Modify the OpenSpoolTag branch (the block that reads
status.openspool, calls fillEnrichmentFromStatus and showMatchBadge) to NOT
break immediately when status.spoolman is missing; rather poll/retry/check for
status.spoolman completion (or use the existing read-timeout mechanism) and only
call showMatchBadge('no Spoolman match') after the timeout elapses, while still
updating brand/type/color and calling fillEnrichmentFromStatus when data is
available. Ensure you remove the premature break so the loop continues until
spoolman is resolved or timeout is reached.
Comment on lines
+1206
to
+1218
| // Include enrichment even when tag is not present (persists until next scan) | ||
| { | ||
| SmartTagEnrichment enrichment = ApplicationManager::getInstance().getSmartTagEnrichment(); | ||
| if (enrichment.valid && !doc.containsKey("spoolman")) { | ||
| JsonObject sp = doc.createNestedObject("spoolman"); | ||
| sp["spool_id"] = enrichment.spoolman_id; | ||
| sp["remaining_g"] = enrichment.remaining_g; | ||
| if (enrichment.bed_temp > 0) sp["bed_temp"] = enrichment.bed_temp; | ||
| if (enrichment.extruder_temp > 0) sp["extruder_temp"] = enrichment.extruder_temp; | ||
| if (enrichment.density > 0) sp["density"] = enrichment.density; | ||
| if (enrichment.diameter_mm > 0) sp["diameter_mm"] = enrichment.diameter_mm; | ||
| } | ||
| } |
There was a problem hiding this comment.
/api/status.spoolman is ambiguous once you persist it past removal.
This block emits cached enrichment even when no tag is present or when the current tag is blank or generic, but SmartTagEnrichment does not carry the UID it belongs to. Clients cannot tell whether spoolman belongs to the active scan or a previous tag, so stale remaining and temperature values can leak into new reader/writer sessions.
🧰 Tools
🪛 Clang (14.0.6)
[note] 1209-1209: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[warning] 1210-1210: variable 'sp' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1210-1210: variable name 'sp' is too short, expected at least 3 characters
(readability-identifier-length)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.cpp` around lines 1206 - 1218, The emitted spoolman
enrichment is ambiguous because SmartTagEnrichment lacks the tag UID, so change
the logic to only persist enrichment when it is tied to the active tag: add a
UID field to SmartTagEnrichment (e.g., enrichment.uid), populate it when
creating/updating the enrichment in ApplicationManager, and in
WebServerManager.cpp verify that enrichment.valid && enrichment.uid ==
currentTagUid (or that a non-empty active tag is present) before creating the
"spoolman" JsonObject; alternatively, if adding a UID is not possible, only emit
enrichment when doc contains an explicit tag identifier (i.e., require the
current scan/tag to be present) to prevent leaking stale values.
… respected, stale enrichment poll cleanup, duplicate Spoolman ID suppressed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /api/spoolman/find-vendor,GET /api/spoolman/find-filament,POST /api/spoolman/save-enrichmentSmartTagEnrichmentstruct,SpoolDetailsdensity/diameter,/api/statusspoolman sub-object for smart tagsTest plan
Summary by CodeRabbit