fix: Spoolman filament dedup, temp averaging, diameter default (#92, #100, #102, #103)#104
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughClient-side enrichment writers now send a default diameter of 1.75 mm (instead of 0) and compute temperatures as the rounded midpoint when both min and max exist; server-side filament lookup and creation were changed to fetch by vendor only and perform case-insensitive name/color matching, with diameters clamped to 1.75 mm when non-positive. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Writer JS)
participant Web as WebServerManager
participant Spool as Spoolman API
participant DB as Spoolman (filament store)
Client->>Web: POST /api/spoolman/save-enrichment (diameter_mm, nozzleTemp, bedTemp, ... )
Web->>Spool: GET /api/v1/filament?vendor_id=... (no material filter)
Spool->>Web: 200 OK [filament list JSON]
Web->>Web: parse JSON (DynamicJsonDocument, buffer 8192), case-insensitive match by name/material and color
alt existing filament found
Web->>Spool: PATCH filament (update diameter, name if needed)
Spool->>Web: 200 OK
else not found
Web->>Spool: POST create filament (name=material+aspect, diameter set or 1.75)
Spool->>Web: 201 Created
end
Web->>Client: 200 OK (result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/WebServerManager.cpp (2)
1753-1769:⚠️ Potential issue | 🔴 CriticalVariant-aware matching still diverges from the scanner path.
src/SpoolmanManager.cppnow keys filaments on basematerialplusname, but these endpoints still search and create on the rawmaterialstring only. That means an existingmaterial="PLA", name="PLA Silk"filament will never be found here when TigerTag sends"PLA Silk", and the fallback POST will create a second filament with the wrongmaterialvalue. Please pass/derive a separate filament name or modifier field here and reuse the same name-aware matcher as the sync path.Also applies to: 1903-1905
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 1753 - 1769, The endpoint currently matches filaments only by the raw material string (variable material) which diverges from SpoolmanManager's keying (base material + name); modify the lookup to derive/use the same name-aware keying used in SpoolmanManager: extract a separate filament name/modifier from the incoming request (e.g., a "name" or "modifier" field or by splitting the provided material into baseMaterial and variantName) and use that when comparing entries (replace the strcasecmp on f["material"] with the name-aware matcher used in SpoolmanManager, and compare f["name"] as part of the match). Ensure the same derived baseMaterial/name pair is used when creating the fallback POST (so result["material"] and the created resource use the canonical base material and name fields consistently).
1868-1870:⚠️ Potential issue | 🟠 MajorA declined filament suggestion becomes a hard error.
The writer pages send
filament_id = -2when the user cancels the “use existing filament?” prompt. This branch only handles-1, so declining a suggestion skips both lookup and create and then returns"filament create failed"at Line 1932. Treat-2as “force create” instead of as an unresolved ID.Control-flow fix
- int filamentId = confirmedFilamentId; - if (filamentId == -1 && material[0] != '\0') { + const bool forceCreateFilament = (confirmedFilamentId == -2); + int filamentId = confirmedFilamentId > 0 ? confirmedFilamentId : -1; + if (filamentId == -1 && material[0] != '\0') { // Search for existing filament before creating - if (vendorId > 0) { + if (vendorId > 0 && !forceCreateFilament) {Also applies to: 1932-1935
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 1868 - 1870, The code treats confirmedFilamentId == -2 (user declined suggestion) as an unresolved ID and later returns "filament create failed"; fix this by treating -2 as a force-create. Update the logic around filamentId/confirmedFilamentId so that either (a) normalize confirmedFilamentId to -1 when it equals -2 before using filamentId, or (b) change the branch condition to if ((filamentId == -1 || filamentId == -2) && material[0] != '\0') so the create/lookup path runs for declined suggestions; ensure the later error return branch around the "filament create failed" message (the code using filamentId and createFilament/lookupFilament) no longer triggers for -2.src/SpoolmanManager.cpp (1)
627-633:⚠️ Potential issue | 🔴 CriticalA lookup/parsing failure is still treated as “filament not found.”
When
deserializeJson()fails,findExactFilament()returnsfalse, and this caller also falls through to the create block when the GET itself fails. That turns transient Spoolman/network/heap issues into duplicate filament creation again. Please distinguish “no match” from “lookup failed” here and abort on errors, likefindOrCreateVendor()already does.Also applies to: 682-690, 783-790
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SpoolmanManager.cpp` around lines 627 - 633, The code treats JSON parse/lookup failures in findExactFilament as "not found" (returning false) which lets callers fall through to create; change the error-path behavior so parse/lookup failures abort instead of signaling "no match": when deserializeJson() fails inside findExactFilament (and the analogous blocks at the other indicated spots), log the error and return a distinct error result (e.g., change the function to return an enum or throw an exception) rather than false, then update the caller(s) to check for that error result and abort any create flow (mirror the behavior of findOrCreateVendor which aborts on lookup failure). Ensure all three failing blocks referenced are updated consistently.
🤖 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/SpoolmanManager.cpp`:
- Around line 637-640: The comparison for material uses strcmp which is
case-sensitive (const char* mat = obj["material"] | ""; compare via strcmp(mat,
targetMaterial)), causing mismatches; change that check to a case-insensitive
comparison (use strcasecmp(mat, targetMaterial) != 0) to match the existing
color logic (strcasecmp for color_hex) and WebServerManager behavior, and ensure
the appropriate header for strcasecmp is included if not already present.
In `@src/WebServerManager.cpp`:
- Around line 1873-1882: The vendor-wide filament lookup currently treats any
non-200 response or JSON deserialization failure as a cache-miss and proceeds to
create a new filament (leaving filamentId < 0); change the logic in the handler
around the HTTP GET (using http, code, response, deserializeJson, fSearchDoc,
filamentId, vendorId, baseUrl) so that any non-200 code or deserializeJson()
failure returns an error response to the caller instead of falling through to
the POST/create path — e.g., detect code != 200 and deserializeJson() failures
and send an appropriate HTTP error (and do not call the POST /api/v1/filament
creation logic); apply the same fix to the second lookup block around lines
1900-1908.
---
Outside diff comments:
In `@src/SpoolmanManager.cpp`:
- Around line 627-633: The code treats JSON parse/lookup failures in
findExactFilament as "not found" (returning false) which lets callers fall
through to create; change the error-path behavior so parse/lookup failures abort
instead of signaling "no match": when deserializeJson() fails inside
findExactFilament (and the analogous blocks at the other indicated spots), log
the error and return a distinct error result (e.g., change the function to
return an enum or throw an exception) rather than false, then update the
caller(s) to check for that error result and abort any create flow (mirror the
behavior of findOrCreateVendor which aborts on lookup failure). Ensure all three
failing blocks referenced are updated consistently.
In `@src/WebServerManager.cpp`:
- Around line 1753-1769: The endpoint currently matches filaments only by the
raw material string (variable material) which diverges from SpoolmanManager's
keying (base material + name); modify the lookup to derive/use the same
name-aware keying used in SpoolmanManager: extract a separate filament
name/modifier from the incoming request (e.g., a "name" or "modifier" field or
by splitting the provided material into baseMaterial and variantName) and use
that when comparing entries (replace the strcasecmp on f["material"] with the
name-aware matcher used in SpoolmanManager, and compare f["name"] as part of the
match). Ensure the same derived baseMaterial/name pair is used when creating the
fallback POST (so result["material"] and the created resource use the canonical
base material and name fields consistently).
- Around line 1868-1870: The code treats confirmedFilamentId == -2 (user
declined suggestion) as an unresolved ID and later returns "filament create
failed"; fix this by treating -2 as a force-create. Update the logic around
filamentId/confirmedFilamentId so that either (a) normalize confirmedFilamentId
to -1 when it equals -2 before using filamentId, or (b) change the branch
condition to if ((filamentId == -1 || filamentId == -2) && material[0] != '\0')
so the create/lookup path runs for declined suggestions; ensure the later error
return branch around the "filament create failed" message (the code using
filamentId and createFilament/lookupFilament) no longer triggers for -2.
🪄 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: c4d9ce6c-d184-4e41-ab5e-d04a62783445
📒 Files selected for processing (5)
src/OpenSpoolWriterHTML.hsrc/OpenTag3DWriterHTML.hsrc/SpoolmanManager.cppsrc/TigerTagWriterHTML.hsrc/WebServerManager.cpp
| // Fetch all filaments for the vendor — server-side material filter is | ||
| // unreliable (can return 400 or empty array); match material+color client-side. | ||
| snprintf(url, sizeof(url), "%s/api/v1/filament?vendor_id=%d", baseUrl, vendorId); | ||
| http.begin(client, url); | ||
| http.setTimeout(5000); | ||
| code = http.GET(); | ||
| if (code == 200) { | ||
| response = http.getString(); | ||
| DynamicJsonDocument fSearchDoc(4096); | ||
| DynamicJsonDocument fSearchDoc(8192); | ||
| if (!deserializeJson(fSearchDoc, response)) { |
There was a problem hiding this comment.
Lookup errors still fall through to POST /api/v1/filament.
After switching to vendor-wide filament queries, any non-200 or deserializeJson() failure leaves filamentId < 0 and this code drops straight into creation below. That turns transient HTTP/heap/parsing failures into duplicate filament writes. Return an error instead of treating lookup failure as a definitive miss.
Also applies to: 1900-1908
🧰 Tools
🪛 Clang (14.0.6)
[note] 1879-1879: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1882-1882: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[warning] 1881-1881: variable 'fSearchDoc' 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 1873 - 1882, The vendor-wide filament
lookup currently treats any non-200 response or JSON deserialization failure as
a cache-miss and proceeds to create a new filament (leaving filamentId < 0);
change the logic in the handler around the HTTP GET (using http, code, response,
deserializeJson, fSearchDoc, filamentId, vendorId, baseUrl) so that any non-200
code or deserializeJson() failure returns an error response to the caller
instead of falling through to the POST/create path — e.g., detect code != 200
and deserializeJson() failures and send an appropriate HTTP error (and do not
call the POST /api/v1/filament creation logic); apply the same fix to the second
lookup block around lines 1900-1908.
sjordan0228
force-pushed
the
fix/spoolman-bugs
branch
from
42e839b to
3e2ccca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/SpoolmanManager.cpp (1)
619-647:⚠️ Potential issue | 🟠 MajorCase-sensitive material comparison may cause duplicate filaments.
At line 632,
strcmp(mat, targetMaterial)performs a case-sensitive comparison whilecolorandnamecomparisons usestrcasecmp. Existing Spoolman entries created asplaorPetgwill not match and trigger new filament creation. This diverges fromWebServerManager.cppwhich usesstrcasecmpfor material at line 1756 and 1883.🔧 Fix to use case-insensitive material matching
- if (strcmp(mat, targetMaterial) != 0) continue; + if (strcasecmp(mat, targetMaterial) != 0) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SpoolmanManager.cpp` around lines 619 - 647, In findExactFilament, the material comparison uses strcmp(mat, targetMaterial) which is case-sensitive; change it to a case-insensitive comparison (use strcasecmp(mat, targetMaterial)) so material matching is consistent with the existing color/name comparisons (color uses strcasecmp) and with WebServerManager.cpp; ensure you still skip this entry when the comparison != 0 and leave the rest of the matching logic (color, name, outId extraction) unchanged.src/WebServerManager.cpp (1)
1871-1894:⚠️ Potential issue | 🟠 MajorLookup errors still fall through to filament creation.
When the HTTP GET to
/api/v1/filament?vendor_id=...returns a non-200 status (e.g., 500 server error, timeout) ordeserializeJson()fails due to heap exhaustion,filamentIdremains-1and the code proceeds to create a new filament at line 1898. This can cause duplicate filaments on transient failures.Consider returning an error response for non-200 codes instead of treating all failures as "filament not found."
🛡️ Proposed fix to distinguish lookup errors from "not found"
code = http.GET(); if (code == 200) { response = http.getString(); DynamicJsonDocument fSearchDoc(8192); if (!deserializeJson(fSearchDoc, response)) { const char* colorCmp = colorHex; if (colorCmp[0] == '#') colorCmp++; for (JsonObject f : fSearchDoc.as<JsonArray>()) { if (strcasecmp(f["material"] | "", material) != 0) continue; if (colorHex[0] != '\0') { const char* fc = f["color_hex"] | ""; if (fc[0] == '#') fc++; if (strcasecmp(fc, colorCmp) != 0) continue; } filamentId = f["id"] | -1; break; } } + } else if (code != 404) { + // Transient error — abort rather than create duplicate + http.end(); + xSemaphoreGive(g_httpMutex); + char errMsg[64]; + snprintf(errMsg, sizeof(errMsg), "Filament lookup failed (HTTP %d)", code); + _server.send(502, "application/json", "{\"error\":\"" + String(errMsg) + "\"}"); + return; } http.end();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 1871 - 1894, The lookup currently treats any non-200 response or JSON deserialization failure the same as "not found" and proceeds to create a new filament; update the block that calls http.GET() and deserializeJson() so that: if code != 200, log the HTTP error (including code and response if present) and return/propagate an error response instead of leaving filamentId == -1; if deserializeJson(...) returns an error (including heap exhaustion), log the deserialization error and return/propagate an error response as well; only allow creation to proceed when the GET returned 200 and the JSON parsed successfully but no matching filament was found. Reference the variables and calls in this block (http.GET(), http.getString(), deserializeJson(), DynamicJsonDocument fSearchDoc, and filamentId) when making the change.
🤖 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/WebServerManager.cpp`:
- Line 1800: The if-statement that sets a default for the variable diameter
currently lacks braces: change the statement "if (diameter <= 0) diameter =
1.75f;" so it uses a braced block (e.g., if (diameter <= 0) { diameter = 1.75f;
}) to match the codebase style and silence static-analysis warnings; update the
occurrence in WebServerManager.cpp where diameter is assigned.
---
Duplicate comments:
In `@src/SpoolmanManager.cpp`:
- Around line 619-647: In findExactFilament, the material comparison uses
strcmp(mat, targetMaterial) which is case-sensitive; change it to a
case-insensitive comparison (use strcasecmp(mat, targetMaterial)) so material
matching is consistent with the existing color/name comparisons (color uses
strcasecmp) and with WebServerManager.cpp; ensure you still skip this entry when
the comparison != 0 and leave the rest of the matching logic (color, name, outId
extraction) unchanged.
In `@src/WebServerManager.cpp`:
- Around line 1871-1894: The lookup currently treats any non-200 response or
JSON deserialization failure the same as "not found" and proceeds to create a
new filament; update the block that calls http.GET() and deserializeJson() so
that: if code != 200, log the HTTP error (including code and response if
present) and return/propagate an error response instead of leaving filamentId ==
-1; if deserializeJson(...) returns an error (including heap exhaustion), log
the deserialization error and return/propagate an error response as well; only
allow creation to proceed when the GET returned 200 and the JSON parsed
successfully but no matching filament was found. Reference the variables and
calls in this block (http.GET(), http.getString(), deserializeJson(),
DynamicJsonDocument fSearchDoc, and filamentId) when making the 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: 083d6618-0977-4b5a-9aad-30c7f6bfccdb
📒 Files selected for processing (2)
src/SpoolmanManager.cppsrc/WebServerManager.cpp
| const char* material = doc["material"] | ""; | ||
| const char* colorHex = doc["color_hex"] | ""; | ||
| float diameter = doc["diameter_mm"] | 1.75f; | ||
| if (diameter <= 0) diameter = 1.75f; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add braces for consistency with codebase style.
Static analysis flags the missing braces. While functionally correct, adding braces improves readability and prevents future bugs if the block is extended.
🔧 Suggested fix
- if (diameter <= 0) diameter = 1.75f;
+ if (diameter <= 0) {
+ diameter = 1.75f;
+ }📝 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.
| if (diameter <= 0) diameter = 1.75f; | |
| if (diameter <= 0) { | |
| diameter = 1.75f; | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[note] 1800-1800: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[warning] 1800-1800: statement should be inside braces
(readability-braces-around-statements)
[warning] 1800-1800: 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/WebServerManager.cpp` at line 1800, The if-statement that sets a default
for the variable diameter currently lacks braces: change the statement "if
(diameter <= 0) diameter = 1.75f;" so it uses a braced block (e.g., if (diameter
<= 0) { diameter = 1.75f; }) to match the codebase style and silence
static-analysis warnings; update the occurrence in WebServerManager.cpp where
diameter is assigned.
…duplicates (#92, #103) - findExactFilament(): add targetName param; match material+color+name; use DynamicJsonDocument(8192) to handle full vendor filament list - findOrCreateFilament(): remove &material= from query (fetch all vendor filaments, match client-side); build name from formula (material + aspect); pass filamentName to findExactFilament(); upgrade bare-material name to formula on existing filament patch - handleApiSpoolmanFindFilament(): remove &material= from query; match client-side (was already doing client-side material+color match, now fetches full vendor list) - handleApiSpoolmanSaveEnrichment(): remove &material= from filament search query; add color_hex matching to client-side search; increase fSearchDoc to 8192; filament create name is just material (no modifier in enrichment payload)
sjordan0228
force-pushed
the
fix/spoolman-bugs
branch
from
3e2ccca to
8aac5dd
Compare
1 participant
Summary
?material=filter. Prevents duplicate filament creation on every scan. (Bug: streaming spool lookup doesn't filter archived spools #92)material + modifier/aspect(e.g. "PLA Silk", "PETG CF"). Dedup match now includes name to keep variants separate. (fix: consistent filament naming convention + dedup match on vendor + name + color #103)Test plan
Summary by CodeRabbit
Bug Fixes
Improvements