Fix TigerTag dropdown: fetch from TigerTag API with hardcoded fallback#10
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDark/light styling for Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Writer Page (Client)
participant DOM as Selects / Inputs
participant API as api.tigertag.io (External)
Page->>API: GET /materials
Page->>API: GET /brands
API-->>Page: materials JSON
API-->>Page: brands JSON
Page->>Page: validateResponse()
Page->>DOM: populateSelect(material_id, materials)
Page->>DOM: populateSelect(brand_id, brands)
DOM-->>Page: user selects material
Page->>Page: autoFillFromMaterial(selected)
Page->>DOM: set nozzle/bed temp inputs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/TigerTagWriterHTML.h (1)
310-327: Consider adding validation to populateSelect for defense-in-depth.Lines 316–317 use API data directly without validation. While current API data is confirmed clean (0 invalid rows across materials and brands endpoints), the code should validate before populating options. This prevents potential issues if API data quality changes and ensures downstream
uint16_tencoding insrc/WebServerManager.cpp(lines 870, 884) receives only valid IDs in the 0–65535 range.Suggested hardening patch
function populateSelect(selectId, items, valueFn, labelFn) { var el = document.getElementById(selectId); + if (!el || !Array.isArray(items)) return; var currentVal = el.value; var frag = document.createDocumentFragment(); items.forEach(function(item) { + var rawValue = valueFn(item); + var value = Number(rawValue); + var label = String(labelFn(item) || '').trim(); + if (!Number.isInteger(value) || value < 0 || value > 65535 || !label) return; var opt = document.createElement('option'); - opt.value = valueFn(item); - opt.textContent = labelFn(item); + opt.value = String(value); + opt.textContent = label; frag.appendChild(opt); }); + if (!frag.firstChild) return; // keep hardcoded fallback el.innerHTML = ''; el.appendChild(frag); // Restore previous selection if it exists if (currentVal) { el.value = currentVal; if (!el.value) el.selectedIndex = 0; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TigerTagWriterHTML.h` around lines 310 - 327, populateSelect currently trusts API data and inserts option values directly, risking invalid or out-of-range IDs reaching uint16_t encoding in WebServerManager (e.g., places handling material/brand IDs). Add validation in populateSelect: before creating each option call valueFn(item) and ensure it's a finite integer within 0–65535 (and non-negative), and skip or coerce invalid entries; also validate labelFn(item) returns a safe string (fallback to empty or "Unknown"); keep existing selection restore logic. Reference: function populateSelect and the valueFn/labelFn callbacks so the checks are applied where items are iterated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/TigerTagWriterHTML.h`:
- Around line 310-327: populateSelect currently trusts API data and inserts
option values directly, risking invalid or out-of-range IDs reaching uint16_t
encoding in WebServerManager (e.g., places handling material/brand IDs). Add
validation in populateSelect: before creating each option call valueFn(item) and
ensure it's a finite integer within 0–65535 (and non-negative), and skip or
coerce invalid entries; also validate labelFn(item) returns a safe string
(fallback to empty or "Unknown"); keep existing selection restore logic.
Reference: function populateSelect and the valueFn/labelFn callbacks so the
checks are applied where items are iterated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32f5d22a-1ce0-4457-ba63-158994552dce
📒 Files selected for processing (2)
src/SharedCSS.hsrc/TigerTagWriterHTML.h
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/TigerTagWriterHTML.h`:
- Line 108: The anchor opening the external issues URL uses target="_blank"
without protection against tabnabbing; update the HTML string in
TigerTagWriterHTML.h that contains the link with
href="https://github.com/TigerTag-Project/TigerTag-RFID-Guide/issues" and link
text "Request a new brand →" to include rel="noopener noreferrer" alongside
target="_blank" so the anchor is hardened against tabnabbing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d56ab955-8db2-435f-839e-bd8a045c98d0
📒 Files selected for processing (2)
src/SharedCSS.hsrc/TigerTagWriterHTML.h
| <option value="8384">Taulman3D</option> | ||
| </select> | ||
| <div style="font-size:11px;color:var(--muted);margin-top:4px"> | ||
| Brand not listed? Use Generic. <a href="https://github.com/TigerTag-Project/TigerTag-RFID-Guide/issues" target="_blank" style="color:var(--blue)">Request a new brand →</a> |
There was a problem hiding this comment.
Harden external _blank link against tabnabbing.
Line 108 opens a third-party URL in a new tab without rel="noopener noreferrer".
🔐 Proposed fix
- Brand not listed? Use Generic. <a href="https://github.com/TigerTag-Project/TigerTag-RFID-Guide/issues" target="_blank" style="color:var(--blue)">Request a new brand →</a>
+ Brand not listed? Use Generic. <a href="https://github.com/TigerTag-Project/TigerTag-RFID-Guide/issues" target="_blank" rel="noopener noreferrer" style="color:var(--blue)">Request a new brand →</a>📝 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.
| Brand not listed? Use Generic. <a href="https://github.com/TigerTag-Project/TigerTag-RFID-Guide/issues" target="_blank" style="color:var(--blue)">Request a new brand →</a> | |
| Brand not listed? Use Generic. <a href="https://github.com/TigerTag-Project/TigerTag-RFID-Guide/issues" target="_blank" rel="noopener noreferrer" style="color:var(--blue)">Request a new brand →</a> |
🧰 Tools
🪛 Clang (14.0.6)
[error] 108-108: expected identifier or '('
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TigerTagWriterHTML.h` at line 108, The anchor opening the external issues
URL uses target="_blank" without protection against tabnabbing; update the HTML
string in TigerTagWriterHTML.h that contains the link with
href="https://github.com/TigerTag-Project/TigerTag-RFID-Guide/issues" and link
text "Request a new brand →" to include rel="noopener noreferrer" alongside
target="_blank" so the anchor is hardened against tabnabbing.
|
Fixed — added rel="noopener noreferrer" to the external link. |
|
All CodeRabbit findings addressed. API fetch working with validation, auto-fill temps, fallback to hardcoded, and tabnabbing fix applied. Tested on ESP32-S3. Merging to dev. |
1 participant
Summary
TigerTag writer dropdowns now fetch materials and brands from the TigerTag API on page load. Hardcoded options remain as fallback if the API is unreachable.
Changes
api.tigertag.io/api:tigertag/SpoolmanDB/materialsapi.tigertag.io/api:tigertag/brand/get/allHow to Test
spoolsense.local/writer/tigertagChecklist
Closes #1
Summary by CodeRabbit
New Features
Style