refactor: extract shared write flow UX from writer pages (#96)#120
Merged
Conversation
Moves duplicated write flow logic (waitForTag, writeFlow, read button, enrichment save) into SharedJS.h. Each writer page now calls sharedWriteFlow() with a config object. Saves ~19KB flash.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughExtracts per-page tag read/write/enrichment flows into a shared, configurable client-side library ( Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (UI)
participant Server as Server (API)
participant Spoolman as Spoolman (External)
Browser->>Server: GET /api/status (poll) // waitForTag
Server-->>Browser: {present, uid, tag_kind, status...}
alt format check required
Browser->>Server: POST /api/format-check {uid}
Server-->>Browser: format result
end
Browser->>Server: POST /api/write... {payload} // sharedWriteFlow -> write endpoint
Server-->>Browser: write accepted (uid)
loop verify loop (sharedWriteFlow)
Browser->>Server: GET /api/status
Server-->>Browser: status (verify condition)
end
Browser->>Server: POST /api/spoolman/save-enrichment {fields} // saveEnrichmentToSpoolman
Server->>Spoolman: GET /spools?limit=200
Spoolman-->>Server: spool list
alt existing spool found
Server->>Spoolman: PATCH spool (initial_weight/used_weight)
else create new spool
Server->>Spoolman: POST spool (initial_weight/used_weight)
end
Spoolman-->>Server: save result
Server-->>Browser: final status/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
The DevKitC's GPIO 48 WS2812B uses GRB ordering, not RGB like the S3-Zero. Red was displaying as green.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 284-289: The verify function currently only checks tag_kind ===
'OpenSpoolTag' which allows false positives; update the verify implementation
inside the sharedWriteFlow call to fetch the expected values from buildPayload
(or the payload passed to the write step) and explicitly compare the
OpenSpoolTag's fields (e.g., brand, material, color_hex and temperature fields
such as temp_hotend/temp_bed or whatever names are used in the payload) against
those expected values, returning true only if all those fields match the
payload; keep the tag_kind check but add equality checks for each required field
to ensure the written tag actually reflects the new OpenSpool data.
In `@src/OpenTag3DWriterHTML.h`:
- Around line 511-516: The current verify predicate passed into sharedWriteFlow
only checks status.tag_kind === 'OpenTag3D', which allows pre-existing tags to
falsely pass; update the verify function (the verify: ... passed alongside
endpoint '/api/write-opentag3d' and buildPayload) to perform a deep/content
check of the returned status against the actual write payload (compare required
properties from buildPayload such as the tag's unique id/name, geometry/mesh
fields, and any metadata/attributes that must match), e.g., derive the
expectedPayload from buildPayload and assert equality or presence of those
fields in status so the poll only succeeds when the read-back matches the
intended OpenTag3D payload.
- Around line 577-603: The fillForm function is populating decoded tag fields
(print_temp_c, bed_temp_c, density, min/max temps, etc.) then dispatching an
input on '#base_material', which triggers ot3dAutoFill to overwrite those values
because '#material_modifiers' wasn’t synced first; update fillForm so it either
(a) dispatches an input on the material modifiers first (element id
'material_modifiers') and waits for any change handlers to run before
dispatching on 'base_material', or (b) temporarily mark the fields as
"filled-from-tag" (e.g., set a data-auto-filled flag on the inputs) and clear
that flag after material modifier sync so ot3dAutoFill will not override those
values; implement the chosen approach inside fillForm and ensure ot3dAutoFill
checks that flag (or that material modifiers are synced) before applying
defaults.
In `@src/SharedJS.h`:
- Around line 527-531: The shared waitForTag() function currently hard-codes
"NTAG" in the user-facing instruction (via setResult('resultBox', 'Place and
hold an NTAG tag on the scanner.', '')) which is incorrect for other flows like
ISO15693; update waitForTag to accept a tagType (or optional message) parameter
and use that parameter to compose a generic/injection-safe instruction (e.g.,
"Place and hold a [tagType] tag on the scanner." or a neutral "Place and hold a
tag on the scanner.") and update all callers (e.g., any pages invoking
waitForTag) to pass the appropriate tag type so setResult and setBanner use the
correct text; keep function name waitForTag and the UI setters (setResult,
setBanner, setStepState) unchanged aside from the message change.
In `@src/TigerTagWriterHTML.h`:
- Around line 559-568: The verify predicate passed to sharedWriteFlow only
checks identity fields (material_id and brand_id) on status.tigertag versus
payload, so edits to mutable fields (e.g., weight, color, diameter,
temperature(s)) will be missed; update the verify function in the
sharedWriteFlow call to also compare the mutable fields on status.tigertag
(e.g., tt.weight, tt.color, tt.diameter, tt.temperature or temperature_* as
used) against the corresponding properties on payload and return false if any
mismatch is found, keeping the existing tag_kind and tigertag existence checks.
- Around line 625-643: The fillForm callback currently calls syncMaterialId()
which can overwrite the decoded tt.material_id and never restores
tt.diameter_mm/diameter_id; instead preserve and apply the parsed IDs directly:
set document.getElementById('material_id').value = tt.material_id and set the
diameter element (e.g., 'diameter_id' or the appropriate diameter input) from
tt.diameter_mm/tt.diameter_id if present, and remove or defer the
syncMaterialId() call (or add a guarded flag to syncMaterialId to skip when a
parsed material_id is present) so the async material list does not replace the
decoded values in fillForm.
🪄 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: 5f02c61f-69ea-424f-95e2-cb7780ed4c35
📒 Files selected for processing (5)
src/OpenPrintTagWriterHTML.hsrc/OpenSpoolWriterHTML.hsrc/OpenTag3DWriterHTML.hsrc/SharedJS.hsrc/TigerTagWriterHTML.h
Comment on lines
+284
to
+289
| sharedWriteFlow({ | ||
| stepIds: ['step-wait', 'step-detect', 'step-write', 'step-verify'], | ||
| endpoint: '/api/write-openspool', | ||
| formatName: 'OpenSpool', | ||
| buildPayload: buildPayload, | ||
| verify: function(status) { return status.tag_kind === 'OpenSpoolTag'; }, |
There was a problem hiding this comment.
OpenSpool verification needs to compare written fields.
Right now any present OpenSpoolTag satisfies verify(). If the tag already contains OpenSpool data, the loop can report success without proving that brand, material, color_hex, or temperatures changed.
💡 Suggested fix
- verify: function(status) { return status.tag_kind === 'OpenSpoolTag'; },
+ verify: function(status, payload) {
+ if (status.tag_kind !== 'OpenSpoolTag' || !status.openspool) return false;
+ var os = status.openspool;
+ return String(os.brand || '') === String(payload.brand)
+ && String(os.material || '') === String(payload.type)
+ && String((os.color_hex || '').replace(/^#/, '').toUpperCase()) === String(payload.color_hex).toUpperCase()
+ && Number(os.min_temp || 0) === Number(payload.min_temp)
+ && Number(os.max_temp || 0) === Number(payload.max_temp);
+ },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenSpoolWriterHTML.h` around lines 284 - 289, The verify function
currently only checks tag_kind === 'OpenSpoolTag' which allows false positives;
update the verify implementation inside the sharedWriteFlow call to fetch the
expected values from buildPayload (or the payload passed to the write step) and
explicitly compare the OpenSpoolTag's fields (e.g., brand, material, color_hex
and temperature fields such as temp_hotend/temp_bed or whatever names are used
in the payload) against those expected values, returning true only if all those
fields match the payload; keep the tag_kind check but add equality checks for
each required field to ensure the written tag actually reflects the new
OpenSpool data.
Comment on lines
+577
to
+603
| fillForm: function(status) { | ||
| var ot = status.opentag3d || {}; | ||
| 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); | ||
| } | ||
| setBanner('statusBanner', 'Tag written \u2713 Spoolman enrichment saved \u2713'); | ||
| } catch(e) { | ||
| setBanner('statusBanner', 'Tag written \u2713 Spoolman save failed \u2014 check connection'); | ||
| if (ot.color_name) setVal('color_name', ot.color_name); | ||
| var matEl = document.getElementById('base_material'); | ||
| if (matEl) matEl.dispatchEvent(new Event('input')); | ||
| }, |
There was a problem hiding this comment.
Don’t let ot3dAutoFill() overwrite the values you just read.
This callback fills print_temp_c, bed_temp_c, density, min/max temps, etc., then dispatches input on #base_material. Because those fields still count as auto-filled, ot3dAutoFill() can immediately replace the decoded tag values with material DB defaults — and it can do so with the wrong modifier profile because #material_modifiers is never synced first.
💡 Suggested fix
fillForm: function(status) {
var ot = status.opentag3d || {};
setVal('base_material', ot.base_material || '');
+ setVal('material_modifiers', ot.modifiers || '');
setVal('manufacturer', ot.manufacturer || '');
@@
if (ot.color_name) setVal('color_name', ot.color_name);
+ ['print_temp_c','bed_temp_c','min_print_temp_c','max_print_temp_c','min_bed_temp_c','max_bed_temp_c','density','max_dry_temp_c','dry_time_hours']
+ .forEach(function(id) {
+ var el = document.getElementById(id);
+ if (el && el.value !== '') el.dataset.autoFilled = 'false';
+ });
var matEl = document.getElementById('base_material');
if (matEl) matEl.dispatchEvent(new Event('input'));
},📝 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
| fillForm: function(status) { | |
| var ot = status.opentag3d || {}; | |
| 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); | |
| } | |
| setBanner('statusBanner', 'Tag written \u2713 Spoolman enrichment saved \u2713'); | |
| } catch(e) { | |
| setBanner('statusBanner', 'Tag written \u2713 Spoolman save failed \u2014 check connection'); | |
| if (ot.color_name) setVal('color_name', ot.color_name); | |
| var matEl = document.getElementById('base_material'); | |
| if (matEl) matEl.dispatchEvent(new Event('input')); | |
| }, | |
| fillForm: function(status) { | |
| var ot = status.opentag3d || {}; | |
| setVal('base_material', ot.base_material || ''); | |
| setVal('material_modifiers', ot.modifiers || ''); | |
| 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); | |
| ['print_temp_c','bed_temp_c','min_print_temp_c','max_print_temp_c','min_bed_temp_c','max_bed_temp_c','density','max_dry_temp_c','dry_time_hours'] | |
| .forEach(function(id) { | |
| var el = document.getElementById(id); | |
| if (el && el.value !== '') el.dataset.autoFilled = 'false'; | |
| }); | |
| var matEl = document.getElementById('base_material'); | |
| if (matEl) matEl.dispatchEvent(new Event('input')); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenTag3DWriterHTML.h` around lines 577 - 603, The fillForm function is
populating decoded tag fields (print_temp_c, bed_temp_c, density, min/max temps,
etc.) then dispatching an input on '#base_material', which triggers ot3dAutoFill
to overwrite those values because '#material_modifiers' wasn’t synced first;
update fillForm so it either (a) dispatches an input on the material modifiers
first (element id 'material_modifiers') and waits for any change handlers to run
before dispatching on 'base_material', or (b) temporarily mark the fields as
"filled-from-tag" (e.g., set a data-auto-filled flag on the inputs) and clear
that flag after material modifier sync so ot3dAutoFill will not override those
values; implement the chosen approach inside fillForm and ensure ot3dAutoFill
checks that flag (or that material modifiers are synced) before applying
defaults.
Comment on lines
+527
to
+531
| async function waitForTag(timeoutMs) { | ||
| var deadline = Date.now() + timeoutMs; | ||
| setStepState('step-wait', 'active'); | ||
| setBanner('statusBanner', 'Waiting for tag\u2026'); | ||
| setResult('resultBox', 'Place and hold an NTAG tag on the scanner.', ''); |
There was a problem hiding this comment.
Don’t hard-code NTAG instructions in the shared wait state.
waitForTag() now drives every writer page, but it still hard-codes “NTAG”. src/OpenPrintTagWriterHTML.h is an ISO15693 flow, so this shared copy now tells users to present the wrong tag type before the write even starts.
💡 Suggested fix
-async function waitForTag(timeoutMs) {
+async function waitForTag(timeoutMs, waitPrompt) {
var deadline = Date.now() + timeoutMs;
setStepState('step-wait', 'active');
setBanner('statusBanner', 'Waiting for tag…');
- setResult('resultBox', 'Place and hold an NTAG tag on the scanner.', '');
+ setResult('resultBox', waitPrompt || 'Place and hold a tag on the scanner.', '');
@@
- var presentStatus = await waitForTag(8000);
+ var presentStatus = await waitForTag(8000, config.waitPrompt);Also applies to: 560-560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SharedJS.h` around lines 527 - 531, The shared waitForTag() function
currently hard-codes "NTAG" in the user-facing instruction (via
setResult('resultBox', 'Place and hold an NTAG tag on the scanner.', '')) which
is incorrect for other flows like ISO15693; update waitForTag to accept a
tagType (or optional message) parameter and use that parameter to compose a
generic/injection-safe instruction (e.g., "Place and hold a [tagType] tag on the
scanner." or a neutral "Place and hold a tag on the scanner.") and update all
callers (e.g., any pages invoking waitForTag) to pass the appropriate tag type
so setResult and setBanner use the correct text; keep function name waitForTag
and the UI setters (setResult, setBanner, setStepState) unchanged aside from the
message change.
Comment on lines
+559
to
+568
| sharedWriteFlow({ | ||
| stepIds: ['step-wait', 'step-detect', 'step-write', 'step-verify'], | ||
| endpoint: '/api/write-tigertag', | ||
| formatName: 'TigerTag', | ||
| buildPayload: buildPayload, | ||
| verify: function(status, payload) { | ||
| if (status.tag_kind !== 'TigerTag' || !status.tigertag) return false; | ||
| var tt = status.tigertag; | ||
| return tt.material_id === payload.material_id && tt.brand_id === payload.brand_id; | ||
| }, |
There was a problem hiding this comment.
Verify a mutable TigerTag field too.
material_id + brand_id is only spool identity. If the user edits weight, color, diameter, or temperatures on the same TigerTag, this predicate still returns true and sharedWriteFlow() reports a verified write even when those bytes did not change.
💡 Suggested fix
verify: function(status, payload) {
if (status.tag_kind !== 'TigerTag' || !status.tigertag) return false;
var tt = status.tigertag;
- return tt.material_id === payload.material_id && tt.brand_id === payload.brand_id;
+ return tt.material_id === payload.material_id
+ && tt.brand_id === payload.brand_id
+ && Number(tt.weight_g) === Number(payload.weight_g)
+ && Number(tt.nozzle_temp_min || 0) === Number(payload.nozzle_min)
+ && Number(tt.nozzle_temp_max || 0) === Number(payload.nozzle_max);
},📝 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
| sharedWriteFlow({ | |
| stepIds: ['step-wait', 'step-detect', 'step-write', 'step-verify'], | |
| endpoint: '/api/write-tigertag', | |
| formatName: 'TigerTag', | |
| buildPayload: buildPayload, | |
| verify: function(status, payload) { | |
| if (status.tag_kind !== 'TigerTag' || !status.tigertag) return false; | |
| var tt = status.tigertag; | |
| return tt.material_id === payload.material_id && tt.brand_id === payload.brand_id; | |
| }, | |
| sharedWriteFlow({ | |
| stepIds: ['step-wait', 'step-detect', 'step-write', 'step-verify'], | |
| endpoint: '/api/write-tigertag', | |
| formatName: 'TigerTag', | |
| buildPayload: buildPayload, | |
| verify: function(status, payload) { | |
| if (status.tag_kind !== 'TigerTag' || !status.tigertag) return false; | |
| var tt = status.tigertag; | |
| return tt.material_id === payload.material_id | |
| && tt.brand_id === payload.brand_id | |
| && Number(tt.weight_g) === Number(payload.weight_g) | |
| && Number(tt.nozzle_temp_min || 0) === Number(payload.nozzle_min) | |
| && Number(tt.nozzle_temp_max || 0) === Number(payload.nozzle_max); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TigerTagWriterHTML.h` around lines 559 - 568, The verify predicate passed
to sharedWriteFlow only checks identity fields (material_id and brand_id) on
status.tigertag versus payload, so edits to mutable fields (e.g., weight, color,
diameter, temperature(s)) will be missed; update the verify function in the
sharedWriteFlow call to also compare the mutable fields on status.tigertag
(e.g., tt.weight, tt.color, tt.diameter, tt.temperature or temperature_* as
used) against the corresponding properties on payload and return false if any
mismatch is found, keeping the existing tag_kind and tigertag existence checks.
Comment on lines
+625
to
+643
| fillForm: function(status) { | ||
| 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); | ||
| } | ||
| setBanner('statusBanner', 'Tag written \u2713 Spoolman enrichment saved \u2713'); | ||
| } catch(e) { | ||
| setBanner('statusBanner', 'Tag written \u2713 Spoolman save failed \u2014 check connection'); | ||
| 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; | ||
| syncMaterialId(); |
There was a problem hiding this comment.
Keep the parsed TigerTag IDs and diameter instead of re-deriving them.
This callback already has exact tt.material_id and tt.diameter_mm from the decoded tag. Calling syncMaterialId() afterward can overwrite the parsed ID with the default PLA mapping when the async material list has not finished loading, and diameter_id is never restored at all.
💡 Suggested fix
fillForm: function(status) {
var tt = status.tigertag || {};
setVal('material_search', tt.material_name || '');
setVal('brand_name', tt.brand_name || '');
@@
if (tt.dry_temp) setVal('dry_temp', tt.dry_temp);
if (tt.dry_time_hours) setVal('dry_time', tt.dry_time_hours);
+ if (tt.diameter_mm) setVal('diameter_id', tt.diameter_mm >= 2.0 ? '221' : '56');
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;
- syncMaterialId();
+ if (tt.material_id === undefined) syncMaterialId();
},📝 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
| fillForm: function(status) { | |
| 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); | |
| } | |
| setBanner('statusBanner', 'Tag written \u2713 Spoolman enrichment saved \u2713'); | |
| } catch(e) { | |
| setBanner('statusBanner', 'Tag written \u2713 Spoolman save failed \u2014 check connection'); | |
| 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; | |
| syncMaterialId(); | |
| fillForm: function(status) { | |
| 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.diameter_mm) setVal('diameter_id', tt.diameter_mm >= 2.0 ? '221' : '56'); | |
| 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; | |
| if (tt.material_id === undefined) syncMaterialId(); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TigerTagWriterHTML.h` around lines 625 - 643, The fillForm callback
currently calls syncMaterialId() which can overwrite the decoded tt.material_id
and never restores tt.diameter_mm/diameter_id; instead preserve and apply the
parsed IDs directly: set document.getElementById('material_id').value =
tt.material_id and set the diameter element (e.g., 'diameter_id' or the
appropriate diameter input) from tt.diameter_mm/tt.diameter_id if present, and
remove or defer the syncMaterialId() call (or add a guarded flag to
syncMaterialId to skip when a parsed material_id is present) so the async
material list does not replace the decoded values in fillForm.
- Spoolman uses used_weight not remaining_weight; send initial_weight and calculated used_weight instead - Spool lookup by nfc_id was broken (extra_field filter unreliable); now fetches all spools and matches nfc_id client-side, skipping archived spools - Add 500ms delay before enrichment save to avoid connection reset from concurrent ESP32 requests
- OpenSpool verify now checks brand, material, and color - OpenTag3D verify now checks base_material, manufacturer, and weight - TigerTag verify now also checks weight_g - waitForTag() no longer says "NTAG" (OpenPrintTag uses ISO15693)
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/SharedJS.h`:
- Around line 690-694: enrichmentHasData currently treats "0" as no data because
it checks parseFloat(el.value) > 0; change it to detect whether a value was
provided instead of requiring strictly positive numbers: in enrichmentHasData
(and the similar check around the other occurrence noted) check that the element
exists and el.value is not an empty string (e.g., el.value.trim() !== '') and
consider the field as present even if the parsed number is 0 or other falsy
numeric values; also ensure you still guard against non-numeric input by
validating parseFloat/el.value is a valid number (isFinite) where the calling
code expects numeric content.
- Around line 533-540: The polling loops call api('/api/status') and allow
rejections to escape; update both loops (the one using
setStepState('step-wait','done') and the similar loop around api('/api/status')
at the other location) to catch transient errors and treat them as a poll miss:
wrap the await api('/api/status') in try/catch, on error log/ignore it (matching
setupReadButton() transient handling), await sleep(500) and continue rather than
throwing, so a short-lived 503/reset won't fail the whole flow; apply the same
change to the second occurrence.
- Around line 724-726: The client currently sends filament_id = -2 when the user
declines a matched filament, but
WebServerManager::handleApiSpoolmanSaveEnrichment() only treats -1 as
"search/create" and treats other negatives as failures; update the server to
recognize -2 as "skip lookup/create" (i.e., if filament_id == -2, do not attempt
findOrCreate and leave the filament unset/unchanged) and ensure the code path
does not return "filament create failed"; alternatively, if you prefer a
client-only fix, keep the decline sentinel local by not sending filament_id at
all (or send a distinct null/absent value) from the client code that sets
filamentId (the block using fr and fconfirmed) so the server sees no filament_id
and skips lookup/create—apply the same change for the other occurrence
referenced near the fr handling.
- Around line 596-607: In sharedWriteFlow's verification loop, don't treat any
matching tag as success—ensure the verified tag UID matches the originally
detected tag: change the condition that currently uses config.verify(status,
payload) to also require the same UID (e.g., compare status.uid or
status.present.uid to the earlier detected presentStatus.uid or payload.uid).
Update the while-loop condition in sharedWriteFlow (the block using
api('/api/status'), config.verify, setStepState, and afterSuccess) so success is
only declared when status.present && config.verify(...) && status.uid ===
<originalDetectedUid>.
In `@src/WebServerManager.cpp`:
- Around line 1952-1970: The current lookup sets spoolId==-1 on any GET/JSON
failure or when only the first page (?limit=200) is checked, causing create
logic to run prematurely; change the lookup so it paginates (e.g., add
offset/page param in the URL and loop calls to http.GET()/deserializeJson) and
only attempt to create a new spool when every page was successfully fetched and
searched without finding a matching quotedUid (use the same variables: baseUrl,
url, http.GET(), response, deserializeJson into sDoc, JsonArray arr, and compare
arr[i]["extra"]["nfc_id"] to quotedUid); also treat any HTTP or JSON error as a
failed lookup and do NOT proceed to create—exit the lookup loop and leave
spoolId==-1 until a full successful exhaustion confirms absence.
🪄 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: e9e75c63-9285-4253-8edd-05e50f5e64b3
📒 Files selected for processing (3)
src/LEDManager.cppsrc/SharedJS.hsrc/WebServerManager.cpp
Comment on lines
+533
to
+540
| while (Date.now() < deadline) { | ||
| var status = await api('/api/status'); | ||
| if (status.present) { | ||
| setStepState('step-wait', 'done'); | ||
| return status; | ||
| } | ||
| await sleep(500); | ||
| } |
There was a problem hiding this comment.
Retry transient /api/status failures instead of failing the whole flow.
Both polling loops let api('/api/status') rejections escape, so one short-lived 503/reset turns a recoverable poll miss into a hard write failure. setupReadButton() already treats the same failure mode as transient.
💡 Suggested change
while (Date.now() < deadline) {
- var status = await api('/api/status');
- if (status.present) {
- setStepState('step-wait', 'done');
- return status;
- }
+ try {
+ var status = await api('/api/status');
+ if (status.present) {
+ setStepState('step-wait', 'done');
+ return status;
+ }
+ } catch (e) {
+ // retry until timeout
+ }
await sleep(500);
}
@@
while (Date.now() < verifyDeadline) {
await sleep(500);
- var status = await api('/api/status');
+ var status;
+ try {
+ status = await api('/api/status');
+ } catch (e) {
+ continue;
+ }
if (status.present && config.verify(status, payload)) {Also applies to: 596-600
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SharedJS.h` around lines 533 - 540, The polling loops call
api('/api/status') and allow rejections to escape; update both loops (the one
using setStepState('step-wait','done') and the similar loop around
api('/api/status') at the other location) to catch transient errors and treat
them as a poll miss: wrap the await api('/api/status') in try/catch, on error
log/ignore it (matching setupReadButton() transient handling), await sleep(500)
and continue rather than throwing, so a short-lived 503/reset won't fail the
whole flow; apply the same change to the second occurrence.
Comment on lines
+596
to
+607
| var verifyDeadline = Date.now() + 15000; | ||
| while (Date.now() < verifyDeadline) { | ||
| await sleep(500); | ||
| var status = await api('/api/status'); | ||
| if (status.present && config.verify(status, payload)) { | ||
| setBanner('statusBanner', 'Tag verified \u2014 hold for a moment\u2026'); | ||
| await sleep(2000); | ||
| setStepState('step-verify', 'done'); | ||
| setBanner('statusBanner', 'Write complete \u2014 safe to remove tag.'); | ||
| setResult('resultBox', config.formatName + ' tag written and verified successfully.', 'success'); | ||
| await sleep(500); | ||
| if (config.afterSuccess) await config.afterSuccess(presentStatus.uid); |
There was a problem hiding this comment.
Require the same UID during verification.
sharedWriteFlow() already knows which tag was detected, but success is declared on any config.verify(...) match. The current OpenSpool and OpenTag3D call sites only verify tag_kind, so swapping to another same-format tag can still show “verified”.
💡 Suggested change
- if (status.present && config.verify(status, payload)) {
+ if (status.present &&
+ status.uid === presentStatus.uid &&
+ config.verify(status, payload)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SharedJS.h` around lines 596 - 607, In sharedWriteFlow's verification
loop, don't treat any matching tag as success—ensure the verified tag UID
matches the originally detected tag: change the condition that currently uses
config.verify(status, payload) to also require the same UID (e.g., compare
status.uid or status.present.uid to the earlier detected presentStatus.uid or
payload.uid). Update the while-loop condition in sharedWriteFlow (the block
using api('/api/status'), config.verify, setStepState, and afterSuccess) so
success is only declared when status.present && config.verify(...) && status.uid
=== <originalDetectedUid>.
Comment on lines
+690
to
+694
| function enrichmentHasData(fieldIds) { | ||
| return fieldIds.some(function(id) { | ||
| var el = document.getElementById(id); | ||
| return el && el.value && parseFloat(el.value) > 0; | ||
| }); |
There was a problem hiding this comment.
Preserve remaining_g = 0 end-to-end.
enrichmentHasData() rejects 0 as “no data”, and the request body later collapses falsy values back to 0. That makes an empty spool impossible to represent unambiguously through this path. Track whether the field was provided instead of using strict-positive/falsy coercion.
Also applies to: 740-740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SharedJS.h` around lines 690 - 694, enrichmentHasData currently treats
"0" as no data because it checks parseFloat(el.value) > 0; change it to detect
whether a value was provided instead of requiring strictly positive numbers: in
enrichmentHasData (and the similar check around the other occurrence noted)
check that the element exists and el.value is not an empty string (e.g.,
el.value.trim() !== '') and consider the field as present even if the parsed
number is 0 or other falsy numeric values; also ensure you still guard against
non-numeric input by validating parseFloat/el.value is a valid number (isFinite)
where the calling code expects numeric content.
Comment on lines
+724
to
+726
| if (fr.found) { | ||
| var fconfirmed = confirm('Found existing filament "' + fr.name + '" in Spoolman. Use it?'); | ||
| filamentId = fconfirmed ? fr.id : -2; |
There was a problem hiding this comment.
Declining a matched filament currently guarantees a failed save.
This sends filament_id = -2 on “No”, but WebServerManager::handleApiSpoolmanSaveEnrichment() only searches/creates when filament_id == -1; any other negative value falls through to filament create failed. Keep the decline sentinel client-local, or teach the server to treat -2 as “skip lookup and create”.
Also applies to: 746-746
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SharedJS.h` around lines 724 - 726, The client currently sends
filament_id = -2 when the user declines a matched filament, but
WebServerManager::handleApiSpoolmanSaveEnrichment() only treats -1 as
"search/create" and treats other negatives as failures; update the server to
recognize -2 as "skip lookup/create" (i.e., if filament_id == -2, do not attempt
findOrCreate and leave the filament unset/unchanged) and ensure the code path
does not return "filament create failed"; alternatively, if you prefer a
client-only fix, keep the decline sentinel local by not sending filament_id at
all (or send a distinct null/absent value) from the client code that sets
filamentId (the block using fr and fconfirmed) so the server sees no filament_id
and skips lookup/create—apply the same change for the other occurrence
referenced near the fr handling.
Comment on lines
+1952
to
+1970
| snprintf(url, sizeof(url), "%s/api/v1/spool?limit=200", baseUrl); | ||
| http.begin(client, url); | ||
| http.setTimeout(5000); | ||
| code = http.GET(); | ||
| if (code == 200) { | ||
| response = http.getString(); | ||
| StaticJsonDocument<2048> sDoc; | ||
| DynamicJsonDocument sDoc(16384); | ||
| if (!deserializeJson(sDoc, response)) { | ||
| JsonArray arr = sDoc.as<JsonArray>(); | ||
| if (arr.size() > 0) spoolId = arr[0]["id"] | -1; | ||
| for (size_t i = 0; i < arr.size(); i++) { | ||
| const char* nfcId = arr[i]["extra"]["nfc_id"] | ""; | ||
| if (strcmp(nfcId, quotedUid) == 0) { | ||
| bool archived = arr[i]["archived"] | false; | ||
| if (archived) continue; | ||
| spoolId = arr[i]["id"] | -1; | ||
| existingInitialWeight = arr[i]["initial_weight"] | 0.0f; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't create a new spool unless the UID lookup actually exhausted the result set.
This only inspects ?limit=200, and any GET/JSON failure also leaves spoolId == -1. The create branch then runs anyway, so a larger Spoolman instance or transient lookup failure can duplicate an already-linked NFC ID.
Also applies to: 1999-2025
🧰 Tools
🪛 Clang (14.0.6)
[note] 1956-1956: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 1959-1959: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[warning] 1958-1958: variable 'sDoc' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1960-1960: variable 'arr' 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 1952 - 1970, The current lookup sets
spoolId==-1 on any GET/JSON failure or when only the first page (?limit=200) is
checked, causing create logic to run prematurely; change the lookup so it
paginates (e.g., add offset/page param in the URL and loop calls to
http.GET()/deserializeJson) and only attempt to create a new spool when every
page was successfully fetched and searched without finding a matching quotedUid
(use the same variables: baseUrl, url, http.GET(), response, deserializeJson
into sDoc, JsonArray arr, and compare arr[i]["extra"]["nfc_id"] to quotedUid);
also treat any HTTP or JSON error as a failed lookup and do NOT proceed to
create—exit the lookup loop and leave spoolId==-1 until a full successful
exhaustion confirms absence.
This was referenced Apr 7, 2026
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
sharedWriteFlow(config)— configurable write/verify/enrich pipelinesetupReadButton(config)— shared Read/Cancel toggle with tag pollingsaveEnrichmentToSpoolman(uid, config)— shared Spoolman enrichment savewaitForTag(),enrichmentHasData(),setVal()— shared helpersChanges
Test plan
Summary by CodeRabbit