fix: register-uid deduplicates filaments and spools#136
Conversation
Replace unconditional POST with find-or-create using existing helpers: - enrichFindOrCreateFilament searches by vendor+material+color first - enrichFindSpoolByUid checks for existing spool by nfc_id before creating - Existing spools are PATCHed via enrichUpdateSpool instead of duplicated Likely root cause of community-reported spool duplication (#130).
|
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 27 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 (1)
📝 WalkthroughWalkthroughReplaced inline filament creation and unconditional spool creation in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebServerManager
participant Helper as Enrich Helpers
participant Spoolman as Spoolman API
Client->>WebServerManager: handleApiRegisterUid(uid, filamentData, spoolData)
rect rgba(100, 150, 200, 0.5)
Note over WebServerManager,Helper: Filament resolution
WebServerManager->>Helper: enrichFindOrCreateFilament(filamentData)
Helper->>Spoolman: GET /api/v1/filament?query
alt found
Spoolman-->>Helper: filament {id}
else not found
Helper->>Spoolman: POST /api/v1/filament
Spoolman-->>Helper: filament {id}
end
Helper-->>WebServerManager: filamentId
end
rect rgba(150, 100, 200, 0.5)
Note over WebServerManager,Helper: Spool resolution
WebServerManager->>Helper: enrichFindSpoolByUid(nfc_id)
Helper->>Spoolman: GET /api/v1/spool?query
alt found
Spoolman-->>Helper: spool {id, weight}
Helper->>Helper: enrichUpdateSpool(spoolId, mergedData)
Helper->>Spoolman: PATCH /api/v1/spool/{id}
Spoolman-->>Helper: updated {id}
else not found
Helper->>Spoolman: POST /api/v1/spool
Spoolman-->>Helper: spool {id}
end
Helper-->>WebServerManager: spoolId
end
WebServerManager->>Client: 200 {spoolId, filamentId}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/WebServerManager.cpp`:
- Around line 394-420: The inline spool creation in the register-uid path sets
remaining_weight directly which is inconsistent with
enrichCreateSpool()/enrichUpdateSpool() that derive used_weight and set an
initial_weight (avoiding direct remaining_weight); update the register-uid flow
to call enrichCreateSpool() (or extend enrichCreateSpool() to accept an optional
initialWeight and pass the user-provided initialWeight) so the created JSON uses
initial_weight and used_weight semantics (matching spoolman expectations)
instead of setting remaining_weight directly; ensure you remove the direct
remaining_weight write in the inline block and preserve quotedUid in the
extra.nfc_id field when delegating to enrichCreateSpool().
- Around line 416-418: The current code builds a JSON error body by
concatenating response into errMsg and calling _server.send which can produce
invalid JSON if response contains quotes/special chars; replace that custom JSON
construction with the existing sendError() helper (or call _server.send with a
properly escaped/JSON-encoded response) so the HTTP error is returned safely and
consistently—locate the block using errMsg, xSemaphoreGive, and _server.send and
change it to use sendError() (or a JSON-encoding routine) while keeping the
existing log of the full Spoolman response for debugging.
🪄 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: 93c5e47a-331b-43f9-be44-baf724a9b7e9
📒 Files selected for processing (1)
src/WebServerManager.cpp
| } else { | ||
| // No existing spool — create new | ||
| StaticJsonDocument<512> spoolBody; | ||
| spoolBody["filament_id"] = filamentId; | ||
| if (initialWeight > 0) spoolBody["initial_weight"] = initialWeight; | ||
| if (remainingWeight > 0) spoolBody["remaining_weight"] = remainingWeight; | ||
|
|
||
| JsonObject extra = spoolBody.createNestedObject("extra"); | ||
| extra["nfc_id"] = quotedUid; | ||
|
|
||
| String spoolJson; | ||
| serializeJson(spoolBody, spoolJson); | ||
| Serial.printf("register-uid: creating spool: %s\n", spoolJson.c_str()); | ||
|
|
||
| snprintf(url, sizeof(url), "%s/api/v1/spool", baseUrl); | ||
| http.begin(client, url); | ||
| http.addHeader("Content-Type", "application/json"); | ||
| code = http.POST(spoolJson); | ||
| if (code != 200 && code != 201) { | ||
| response = http.getString(); | ||
| http.end(); | ||
| Serial.printf("register-uid: spool creation failed HTTP %d: %s\n", code, response.c_str()); | ||
| String errMsg = "Failed to create spool (HTTP " + String(code) + "): " + response; | ||
| xSemaphoreGive(g_httpMutex); | ||
| _server.send(500, "application/json", "{\"error\":\"" + errMsg + "\"}"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Inconsistent weight handling between inline creation and helper function.
The inline spool creation path sets remaining_weight directly (line 399), while enrichCreateSpool() (lines 1954-1959) and enrichUpdateSpool() (lines 1931-1937) convert remaining weight to used_weight with an explicit initial_weight. Spoolman computes remaining_weight = initial_weight - used_weight, so setting remaining_weight directly may behave differently or be less reliable.
Consider reusing enrichCreateSpool() here for consistency:
Proposed refactor to use enrichCreateSpool()
} else {
// No existing spool — create new
- StaticJsonDocument<512> spoolBody;
- spoolBody["filament_id"] = filamentId;
- if (initialWeight > 0) spoolBody["initial_weight"] = initialWeight;
- if (remainingWeight > 0) spoolBody["remaining_weight"] = remainingWeight;
-
- JsonObject extra = spoolBody.createNestedObject("extra");
- extra["nfc_id"] = quotedUid;
-
- String spoolJson;
- serializeJson(spoolBody, spoolJson);
- Serial.printf("register-uid: creating spool: %s\n", spoolJson.c_str());
-
- snprintf(url, sizeof(url), "%s/api/v1/spool", baseUrl);
- http.begin(client, url);
- http.addHeader("Content-Type", "application/json");
- code = http.POST(spoolJson);
- if (code != 200 && code != 201) {
- response = http.getString();
- http.end();
- Serial.printf("register-uid: spool creation failed HTTP %d: %s\n", code, response.c_str());
- String errMsg = "Failed to create spool (HTTP " + String(code) + "): " + response;
- xSemaphoreGive(g_httpMutex);
- _server.send(500, "application/json", "{\"error\":\"" + errMsg + "\"}");
- return;
- }
- response = http.getString();
- http.end();
-
- StaticJsonDocument<1024> spoolDoc;
- if (!deserializeJson(spoolDoc, response)) {
- spoolId = spoolDoc["id"] | -1;
- }
+ Serial.printf("register-uid: creating spool for uid=%s\n", uid);
+ spoolId = enrichCreateSpool(client, http, baseUrl, filamentId, remainingWeight, quotedUid);
+ if (spoolId < 0) {
+ xSemaphoreGive(g_httpMutex);
+ sendError(500, "Failed to create spool");
+ return;
+ }
}Note: enrichCreateSpool() uses a hardcoded 1000g initial weight when remainingG > 0. If preserving the user-provided initialWeight is important, you may need to extend enrichCreateSpool() to accept an optional initial weight parameter.
🧰 Tools
🪛 Clang (14.0.6)
[note] 394-394: +1, nesting level increased to 1
(clang)
[note] 412-412: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 412-412: +1
(clang)
[warning] 396-396: variable 'spoolBody' is not initialized
(cppcoreguidelines-init-variables)
[warning] 401-401: variable 'extra' is not initialized
(cppcoreguidelines-init-variables)
[warning] 404-404: variable 'spoolJson' is not initialized
(cppcoreguidelines-init-variables)
[warning] 416-416: variable 'errMsg' 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 394 - 420, The inline spool creation
in the register-uid path sets remaining_weight directly which is inconsistent
with enrichCreateSpool()/enrichUpdateSpool() that derive used_weight and set an
initial_weight (avoiding direct remaining_weight); update the register-uid flow
to call enrichCreateSpool() (or extend enrichCreateSpool() to accept an optional
initialWeight and pass the user-provided initialWeight) so the created JSON uses
initial_weight and used_weight semantics (matching spoolman expectations)
instead of setting remaining_weight directly; ensure you remove the direct
remaining_weight write in the inline block and preserve quotedUid in the
extra.nfc_id field when delegating to enrichCreateSpool().
| String errMsg = "Failed to create spool (HTTP " + String(code) + "): " + response; | ||
| xSemaphoreGive(g_httpMutex); | ||
| _server.send(500, "application/json", "{\"error\":\"" + errMsg + "\"}"); |
There was a problem hiding this comment.
Unescaped error message can produce malformed JSON.
The response from Spoolman may contain quotes or special characters. Concatenating it directly into the JSON string bypasses the escaping logic in sendError(), potentially producing invalid JSON responses.
Proposed fix using sendError()
if (code != 200 && code != 201) {
response = http.getString();
http.end();
Serial.printf("register-uid: spool creation failed HTTP %d: %s\n", code, response.c_str());
- String errMsg = "Failed to create spool (HTTP " + String(code) + "): " + response;
xSemaphoreGive(g_httpMutex);
- _server.send(500, "application/json", "{\"error\":\"" + errMsg + "\"}");
+ char errMsg[128];
+ snprintf(errMsg, sizeof(errMsg), "Failed to create spool (HTTP %d)", code);
+ sendError(500, errMsg);
return;
}This truncates the Spoolman response detail but ensures valid JSON. Alternatively, use sendError() with just the HTTP code and log the full response for debugging (which is already done on line 415).
📝 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.
| String errMsg = "Failed to create spool (HTTP " + String(code) + "): " + response; | |
| xSemaphoreGive(g_httpMutex); | |
| _server.send(500, "application/json", "{\"error\":\"" + errMsg + "\"}"); | |
| xSemaphoreGive(g_httpMutex); | |
| char errMsg[128]; | |
| snprintf(errMsg, sizeof(errMsg), "Failed to create spool (HTTP %d)", code); | |
| sendError(500, errMsg); |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 416-416: variable 'errMsg' 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 416 - 418, The current code builds a
JSON error body by concatenating response into errMsg and calling _server.send
which can produce invalid JSON if response contains quotes/special chars;
replace that custom JSON construction with the existing sendError() helper (or
call _server.send with a properly escaped/JSON-encoded response) so the HTTP
error is returned safely and consistently—locate the block using errMsg,
xSemaphoreGive, and _server.send and change it to use sendError() (or a
JSON-encoding routine) while keeping the existing log of the full Spoolman
response for debugging.
Same pattern as readISO14443Pages — check tagSessionActive_ before calling setupRF() + activateTypeA(). The scan loop's detectTag() leaves the tag active, but writeISO14443Pages was unconditionally resetting the RF field, killing the session and failing to reactivate. This fixes write failures when a tag is already on the reader and a write is queued from the web UI.
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/HardwareNFCConnection.cpp`:
- Around line 377-384: The write flow in writeISO14443Pages() leaves
tagSessionActive_ set even when the code halts the tag, causing subsequent calls
to skip reactivation; update writeISO14443Pages() (and any helper that issues
HALT) to clear tagSessionActive_ (set it false) immediately after performing
iso14443a_->halt()/halting logic so the fast-path reactivation code (which
checks tagSessionActive_) behaves correctly on the next call; ensure any
early-return paths after halting also clear tagSessionActive_ before returning.
🪄 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: 3bfd69dd-f11a-408b-94c9-f6142603a449
📒 Files selected for processing (1)
src/HardwareNFCConnection.cpp
| if (!tagSessionActive_) { | ||
| // Tag was halted — must reactivate before writing | ||
| iso14443a_->setupRF(); | ||
| uidLen = iso14443a_->activateTypeA(response, 1); | ||
| if (uidLen < 4) { | ||
| Serial.println("HardwareNFC: writeISO14443Pages - tag reactivation failed"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Keep tagSessionActive_ in sync with HALT in write flow.
This new fast-path depends on tagSessionActive_, but writeISO14443Pages() halts at Line 414/Line 421 without clearing the flag. That can make the next call skip activation incorrectly and fall into avoidable retries/failures.
🔧 Proposed fix
if (!tagSessionActive_) {
// Tag was halted — must reactivate before writing
iso14443a_->setupRF();
uidLen = iso14443a_->activateTypeA(response, 1);
if (uidLen < 4) {
Serial.println("HardwareNFC: writeISO14443Pages - tag reactivation failed");
return false;
}
+ tagSessionActive_ = true;
}
@@
uidLen = iso14443a_->activateTypeA(response, 1);
if (uidLen < 4) {
Serial.printf("HardwareNFC: writeISO14443Pages - re-activation failed on retry %d\n", retry + 1);
continue;
}
+ tagSessionActive_ = true;
pageOk = iso14443a_->mifareBlockWrite4(page, data + (i * 4));
}
@@
Serial.printf("HardwareNFC: writeISO14443Pages - FAILED at page %d after retries (%lums, %d retries total)\n",
page, elapsed, retryCount);
iso14443a_->mifareHalt();
+ tagSessionActive_ = false;
return false; // total write failure; clean exit for caller to retry entire sequence
}
@@
unsigned long elapsed = millis() - writeStart;
iso14443a_->mifareHalt();
+ tagSessionActive_ = false;
Serial.printf("HardwareNFC: writeISO14443Pages - wrote %d pages starting at page %d (%lums, %d retries)\n",
pageCount, startPage, elapsed, retryCount);Also applies to: 414-422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HardwareNFCConnection.cpp` around lines 377 - 384, The write flow in
writeISO14443Pages() leaves tagSessionActive_ set even when the code halts the
tag, causing subsequent calls to skip reactivation; update writeISO14443Pages()
(and any helper that issues HALT) to clear tagSessionActive_ (set it false)
immediately after performing iso14443a_->halt()/halting logic so the fast-path
reactivation code (which checks tagSessionActive_) behaves correctly on the next
call; ensure any early-return paths after halting also clear tagSessionActive_
before returning.
1 participant
Summary
enrichFindOrCreateFilament()— searches by vendor + material + color before creatingenrichFindSpoolByUid()check — if spool with samenfc_idexists, PATCH it viaenrichUpdateSpool()instead of creating a duplicateRoot Cause
handleApiRegisterUid()was the only Spoolman write path that didn't check for existing entries. Every web UI tag write created new filament + spool entries unconditionally, causing duplicates when:Test plan
Closes #134, closes #135
Related: #130
Summary by CodeRabbit