feat: populate tag writer forms from Spoolman spool picker (#32)#65
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a Spoolman picker UI to three tag-writer pages and new client-side JS to fetch spools from /api/spoolman/spools, filter results, and populate existing form inputs (material, manufacturer, color, weight, diameter, and temperature ranges) when a spool is selected. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as "Writer Page (JS)"
participant API as "Spoolman API (/api/spoolman/spools)"
participant Form as "Form Inputs (DOM)"
User->>Page: open writer page / interact with picker
Page->>API: GET /api/spoolman/spools
API-->>Page: JSON spools list
Page->>Page: render picker UI + handle search
User->>Page: type query / click spool row
Page->>Page: filterSpoolmanPicker -> selectSpoolmanSpool(spoolId)
Page->>Form: fillFromSpoolman(spool) (set values, set data-autoFilled, dispatch input events)
Form-->>Page: input events trigger existing sync handlers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
🤖 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 449-460: The current import code calls fill('material',
fil.material || fil.name || '') and fill('manufacturer', vendor) and only
dispatches synthetic input events, which leaves hidden ID fields set to fallback
values; instead, when the imported label does not resolve to a local ID you must
clear or invalidate the hidden ID field so it won't silently persist a default.
Update the logic around fill(...) and the input dispatch for fieldMap.material
and fieldMap.manufacturer to detect unresolved labels (no exact-match mapping)
and set the corresponding hidden ID element (the element referenced by
document.getElementById(fieldMap.material) /
document.getElementById(fieldMap.manufacturer)) to an empty/invalid value (or
add a data-invalid flag), then dispatch the 'input' event so downstream handlers
know the ID is cleared; keep the existing successful-path behavior unchanged.
- Around line 467-479: The single-temp fields 'nozzle_single' and 'bed_single'
are being populated from the lower bound (extruder.min / bed.min) which causes
ranges like {min:190,max:220} to set the main print temps to 190; change those
to use the primary/high value by using extruder.max || extruder for
'nozzle_single' and bed.max || bed for 'bed_single' (keep the existing fill(...)
calls and logic for 'nozzle_min', 'nozzle_max', 'bed_min', 'bed_max' unchanged).
- Around line 417-427: The result items are rendered as non-focusable divs so
keyboard users can't focus or activate them; update the template in
results.innerHTML to render each item as a real interactive element (use a
<button type="button"> per item) instead of a <div>, keep the inline style
(padding/border) and swatch/escaped label text, keep the onclick handler calling
selectSpoolmanSpool(spoolId), and ensure the button remains keyboard-accessible
(no role/tabindex hacks needed when using a button); locate the HTML
construction around results.innerHTML and the spoolId/selectSpoolmanSpool usage
to make this change.
🪄 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: f27ebfdc-573c-4563-b40a-d88ace855971
📒 Files selected for processing (4)
src/OpenPrintTagWriterHTML.hsrc/OpenTag3DWriterHTML.hsrc/SharedJS.hsrc/TigerTagWriterHTML.h
Comment on lines
+417
to
+427
| results.innerHTML = filtered.slice(0, 20).map(function(spool) { | ||
| var fil = spool.filament || {}; | ||
| var vendor = fil.vendor ? fil.vendor.name : ''; | ||
| var esc = function(s) { var d = document.createElement('div'); d.textContent = s; return d.innerHTML; }; | ||
| var color = (fil.color_hex || '').replace(/[^0-9a-fA-F]/g, ''); | ||
| var swatch = color ? '<span style="display:inline-block;width:14px;height:14px;border-radius:50%;background:#' + color + ';vertical-align:middle;margin-right:6px"></span>' : ''; | ||
| var remaining = spool.remaining_weight ? Math.round(spool.remaining_weight) + 'g' : '?'; | ||
| var spoolId = spool.id; | ||
| return '<div style="padding:8px 10px;border-bottom:1px solid var(--border);cursor:pointer" onclick="selectSpoolmanSpool(' + spoolId + ')">' | ||
| + swatch + '#' + spoolId + ' ' + (vendor ? esc(vendor) + ' ' : '') + esc(fil.material || fil.name || '?') + ' \u2014 ' + remaining | ||
| + '</div>'; |
There was a problem hiding this comment.
Render picker results as real buttons.
These entries are non-focusable <div>s with only onclick, so keyboard users can search but cannot focus a result or activate it with Enter/Space.
♿ Minimal fix
- return '<div style="padding:8px 10px;border-bottom:1px solid var(--border);cursor:pointer" onclick="selectSpoolmanSpool(' + spoolId + ')">'
+ return '<button type="button" style="display:block;width:100%;padding:8px 10px;border:0;border-bottom:1px solid var(--border);background:transparent;color:inherit;text-align:left;font:inherit;cursor:pointer" onclick="selectSpoolmanSpool(' + spoolId + ')">'
+ swatch + '#' + spoolId + ' ' + (vendor ? esc(vendor) + ' ' : '') + esc(fil.material || fil.name || '?') + ' \u2014 ' + remaining
- + '</div>';
+ + '</button>';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SharedJS.h` around lines 417 - 427, The result items are rendered as
non-focusable divs so keyboard users can't focus or activate them; update the
template in results.innerHTML to render each item as a real interactive element
(use a <button type="button"> per item) instead of a <div>, keep the inline
style (padding/border) and swatch/escaped label text, keep the onclick handler
calling selectSpoolmanSpool(spoolId), and ensure the button remains
keyboard-accessible (no role/tabindex hacks needed when using a button); locate
the HTML construction around results.innerHTML and the
spoolId/selectSpoolmanSpool usage to make this change.
Comment on lines
+449
to
+460
| fill('material', fil.material || fil.name || ''); | ||
| fill('manufacturer', vendor); | ||
|
|
||
| // Dispatch input events to trigger hidden ID sync handlers | ||
| if (fieldMap.material) { | ||
| var matEl = document.getElementById(fieldMap.material); | ||
| if (matEl) matEl.dispatchEvent(new Event('input', {bubbles: true})); | ||
| } | ||
| if (fieldMap.manufacturer) { | ||
| var mfgEl = document.getElementById(fieldMap.manufacturer); | ||
| if (mfgEl) mfgEl.dispatchEvent(new Event('input', {bubbles: true})); | ||
| } |
There was a problem hiding this comment.
Fail closed when an imported label cannot be resolved to a local ID.
This path only updates the visible inputs and dispatches synthetic input events. On OpenPrintTag/TigerTag, the downstream handlers reset hidden IDs to default values before an exact-match lookup, and /api/spoolman/spools forwards Spoolman's raw labels unchanged. Any vendor/material miss will therefore write PLA/Generic IDs silently. Clear or invalidate the hidden ID instead of keeping the fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SharedJS.h` around lines 449 - 460, The current import code calls
fill('material', fil.material || fil.name || '') and fill('manufacturer',
vendor) and only dispatches synthetic input events, which leaves hidden ID
fields set to fallback values; instead, when the imported label does not resolve
to a local ID you must clear or invalidate the hidden ID field so it won't
silently persist a default. Update the logic around fill(...) and the input
dispatch for fieldMap.material and fieldMap.manufacturer to detect unresolved
labels (no exact-match mapping) and set the corresponding hidden ID element (the
element referenced by document.getElementById(fieldMap.material) /
document.getElementById(fieldMap.manufacturer)) to an empty/invalid value (or
add a data-invalid flag), then dispatch the 'input' event so downstream handlers
know the ID is cleared; keep the existing successful-path behavior unchanged.
Comment on lines
+467
to
+479
| // Temps | ||
| var extruder = fil.settings_extruder_temp; | ||
| var bed = fil.settings_bed_temp; | ||
| if (extruder) { | ||
| fill('nozzle_min', extruder.min || extruder); | ||
| fill('nozzle_max', extruder.max || extruder); | ||
| fill('nozzle_single', extruder.min || extruder); | ||
| } | ||
| if (bed) { | ||
| fill('bed_min', bed.min || bed); | ||
| fill('bed_max', bed.max || bed); | ||
| fill('bed_single', bed.min || bed); | ||
| } |
There was a problem hiding this comment.
Populate the single-temp fields from the primary value, not the lower bound.
When Spoolman returns {min,max}, nozzle_single and bed_single are filled from .min. OpenTag3D maps those to required print_temp_c / bed_temp_c, so a 190–220 spool currently imports 190 as the main print temperature.
🌡️ Safer range handling
+ function tempMin(value) {
+ return (value && typeof value === 'object' && value.min !== undefined && value.min !== null)
+ ? value.min
+ : value;
+ }
+
+ function tempMax(value) {
+ if (value && typeof value === 'object') {
+ if (value.max !== undefined && value.max !== null) return value.max;
+ if (value.min !== undefined && value.min !== null) return value.min;
+ }
+ return value;
+ }
+
// Temps
var extruder = fil.settings_extruder_temp;
var bed = fil.settings_bed_temp;
if (extruder) {
- fill('nozzle_min', extruder.min || extruder);
- fill('nozzle_max', extruder.max || extruder);
- fill('nozzle_single', extruder.min || extruder);
+ fill('nozzle_min', tempMin(extruder));
+ fill('nozzle_max', tempMax(extruder));
+ fill('nozzle_single', tempMax(extruder));
}
if (bed) {
- fill('bed_min', bed.min || bed);
- fill('bed_max', bed.max || bed);
- fill('bed_single', bed.min || bed);
+ fill('bed_min', tempMin(bed));
+ fill('bed_max', tempMax(bed));
+ fill('bed_single', tempMax(bed));
}📝 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
| // Temps | |
| var extruder = fil.settings_extruder_temp; | |
| var bed = fil.settings_bed_temp; | |
| if (extruder) { | |
| fill('nozzle_min', extruder.min || extruder); | |
| fill('nozzle_max', extruder.max || extruder); | |
| fill('nozzle_single', extruder.min || extruder); | |
| } | |
| if (bed) { | |
| fill('bed_min', bed.min || bed); | |
| fill('bed_max', bed.max || bed); | |
| fill('bed_single', bed.min || bed); | |
| } | |
| function tempMin(value) { | |
| return (value && typeof value === 'object' && value.min !== undefined && value.min !== null) | |
| ? value.min | |
| : value; | |
| } | |
| function tempMax(value) { | |
| if (value && typeof value === 'object') { | |
| if (value.max !== undefined && value.max !== null) return value.max; | |
| if (value.min !== undefined && value.min !== null) return value.min; | |
| } | |
| return value; | |
| } | |
| // Temps | |
| var extruder = fil.settings_extruder_temp; | |
| var bed = fil.settings_bed_temp; | |
| if (extruder) { | |
| fill('nozzle_min', tempMin(extruder)); | |
| fill('nozzle_max', tempMax(extruder)); | |
| fill('nozzle_single', tempMax(extruder)); | |
| } | |
| if (bed) { | |
| fill('bed_min', tempMin(bed)); | |
| fill('bed_max', tempMax(bed)); | |
| fill('bed_single', tempMax(bed)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SharedJS.h` around lines 467 - 479, The single-temp fields
'nozzle_single' and 'bed_single' are being populated from the lower bound
(extruder.min / bed.min) which causes ranges like {min:190,max:220} to set the
main print temps to 190; change those to use the primary/high value by using
extruder.max || extruder for 'nozzle_single' and bed.max || bed for 'bed_single'
(keep the existing fill(...) calls and logic for 'nozzle_min', 'nozzle_max',
'bed_min', 'bed_max' unchanged).
This was referenced Apr 4, 2026
4 tasks
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
/api/spoolman/spoolsproxy endpointCloses #32
Codex Review Fixes
Test Plan
Summary by CodeRabbit