refactor: reduce cyclomatic complexity in NFCManager + WebServerManager#121
Conversation
…Write() - buildNdefTlv() — shared NDEF TLV wrapper (was duplicated in OpenTag3D + OpenSpool) - validateWriteUid() — shared mutex + UID validation (was repeated 6 times) - forceRescan() — shared rescan trigger - executeTigerTagWrite(), executeOpenTag3DWrite(), executeOpenSpoolWrite(), executeAtomicWrite() - executeWrite() is now a 30-line dispatcher Complexity: 126 → ~15 for executeWrite(), extracted functions are 15-40 each.
Moves the 220-line ISO14443A tag classification block (TigerTag → OpenTag3D → OpenSpool → GenericUID detection, NDEF TLV parsing, state update, and message dispatch) out of scanLoop() into its own method. scanLoop() now calls readAndProcessISO14443Tag() in one line. scanLoop complexity: ~90 → ~45 branch points.
- serializeTigerTagStatus(), serializeOpenTag3DStatus(), serializeOpenSpoolStatus(), serializeGenericUidStatus(), serializeOpenPrintTagStatus() — each ~15-40 lines - serializeEnrichment() — shared, was duplicated twice - handleApiStatus() is now a 30-line dispatcher with switch Complexity: 74 → ~10 for handleApiStatus()
…ichment() - enrichFindOrCreateVendor() — search by name, create if not found - enrichFindOrCreateFilament() — client-side match by material+color, create if not found - enrichFindSpoolByUid() — fetch all spools, match nfc_id client-side - enrichUpdateSpool() / enrichCreateSpool() — PATCH or POST with used_weight - handleApiSpoolmanSaveEnrichment() is now a 50-line orchestrator Complexity: 57 → ~12 for the handler
…fPayload The 130-line, 5-level-deep NDEF scan in readAndProcessISO14443Tag is now two reusable helpers: - findNdefMediaRecord() — scans TLV records, returns MIME type + payload position - readNdefPayload() — reads payload bytes with extended page read fallback The inline OpenTag3D and OpenSpool detection blocks now each call these helpers instead of duplicating the TLV walk and payload extraction. Max nesting: 5 → 2.
|
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 11 minutes and 9 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)
📝 WalkthroughWalkthroughRefactors NFCManager by extracting ISO14443 NDEF read/processing and splitting write execution into per-format helpers; refactors WebServerManager by extracting /api/status serializers and replacing Spoolman enrichment inline logic with helper HTTP operations; replaces stream-based spool lookup with ArduinoJson filter parsing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebServerManager
participant Helper as EnrichHelpers
participant HTTPClient
participant SpoolmanAPI
Client->>WebServerManager: POST /api/spoolman/save-enrichment (uid + payload)
WebServerManager->>Helper: enrichFindOrCreateVendor(...)
Helper->>HTTPClient: GET/POST /vendors
HTTPClient->>SpoolmanAPI: HTTP request
SpoolmanAPI-->>HTTPClient: vendor response
HTTPClient-->>Helper: vendor id
WebServerManager->>Helper: enrichFindOrCreateFilament(...)
Helper->>HTTPClient: GET/POST /filaments
HTTPClient->>SpoolmanAPI: HTTP request
SpoolmanAPI-->>HTTPClient: filament response
HTTPClient-->>Helper: filament id
WebServerManager->>Helper: enrichFindSpoolByUid(...)
Helper->>HTTPClient: GET /spools?nfc_id="uid"
HTTPClient->>SpoolmanAPI: HTTP request
SpoolmanAPI-->>HTTPClient: spools list
HTTPClient-->>Helper: spool match / initial weight
alt existing spool found
WebServerManager->>Helper: enrichUpdateSpool(...existingId...)
Helper->>HTTPClient: PATCH /spools/{id}
HTTPClient->>SpoolmanAPI: PATCH request
SpoolmanAPI-->>HTTPClient: 200/err
HTTPClient-->>WebServerManager: result
else no spool
WebServerManager->>Helper: enrichCreateSpool(...)
Helper->>HTTPClient: POST /spools
HTTPClient->>SpoolmanAPI: POST request
SpoolmanAPI-->>HTTPClient: 201/err
HTTPClient-->>WebServerManager: result
end
WebServerManager-->>Client: 200/400 based on result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 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 @.gitignore:
- Line 13: You added docs/REFACTORING_GUIDE.md to .gitignore but the file
doesn't exist yet; decide whether this guide should be committed as part of the
repo (if it documents refactoring patterns like helper extraction or
architecture decisions) or truly be a personal/WIP note — then either remove
docs/REFACTORING_GUIDE.md from .gitignore and add the file, or keep it ignored
and add a short inline comment in .gitignore explaining it's for personal/WIP
notes only so intent is clear to future maintainers.
In `@src/NFCManager.cpp`:
- Around line 358-391: In the TigerTag handling block (where currentSpool.kind
is set to TagKind::TigerTag and lastTigerTag_ / lastTigerTagValid_ are updated),
also clear the OpenSpool validity by setting lastOpenSpoolValid_ = false so
stale OpenSpool data can't persist; locate the TigerTag branch around
currentSpool and lastTigerTag_ and add the missing lastOpenSpoolValid_ = false
alongside the existing lastOpenTag3DValid_ = false.
In `@src/WebServerManager.cpp`:
- Around line 1908-1934: The DynamicJsonDocument buffer (16384) in
enrichFindSpoolByUid is too small for up to 200 spools and can cause
deserializeJson to fail; after calling http.getString() allocate the JSON buffer
based on the response size (e.g., DynamicJsonDocument sDoc(response.length() +
1024) or response.length()*1.2) and then call deserializeJson(sDoc, response)
checking its return value for success, or alternatively reduce the "?limit=200"
to a smaller value—adjust the code around the response, DynamicJsonDocument
sDoc, and the deserializeJson call in enrichFindSpoolByUid accordingly.
🪄 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: 8860db08-03de-4eea-be2c-23d02c02d48b
📒 Files selected for processing (5)
.gitignoresrc/NFCManager.cppsrc/NFCManager.hsrc/WebServerManager.cppsrc/WebServerManager.h
| docs/opentag3d_architecture.md | ||
| docs/tigertag-architecture.md | ||
| docs/deep-thoughts.md | ||
| docs/REFACTORING_GUIDE.md |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if REFACTORING_GUIDE.md exists and preview its content to assess value.
if [ -f "docs/REFACTORING_GUIDE.md" ]; then
echo "=== File exists. First 30 lines: ==="
head -n 30 "docs/REFACTORING_GUIDE.md"
echo ""
echo "=== Word count: ==="
wc -w "docs/REFACTORING_GUIDE.md"
else
echo "File does not exist yet."
fiRepository: SpoolSense/spoolsense_scanner
Length of output: 97
Clarify the intent of ignoring a non-existent documentation file.
The docs/REFACTORING_GUIDE.md file doesn't currently exist, but you're adding it to .gitignore, suggesting it will be created as temporary or personal content. If this guide will document the complexity reduction patterns established in this PR—such as the helper extraction strategies or architectural decisions—consider committing it instead to help future maintainers understand the refactoring principles. If it's intended for temporary work-in-progress notes, ignoring it is appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 13, You added docs/REFACTORING_GUIDE.md to .gitignore but
the file doesn't exist yet; decide whether this guide should be committed as
part of the repo (if it documents refactoring patterns like helper extraction or
architecture decisions) or truly be a personal/WIP note — then either remove
docs/REFACTORING_GUIDE.md from .gitignore and add the file, or keep it ignored
and add a short inline comment in .gitignore explaining it's for personal/WIP
notes only so intent is clear to future maintainers.
| if (isTigerTag) { | ||
| currentSpool.kind = TagKind::TigerTag; | ||
| currentSpool.tag_data_valid = false; | ||
| lastTigerTag_ = tigerData; | ||
| lastTigerTagValid_ = true; | ||
| lastOpenTag3DValid_ = false; | ||
| Serial.printf("NFCManager: TigerTag detected — %s %s %s\n", | ||
| tigerData.brand_name, tigerData.material_name, tigerData.aspect1_name); | ||
| } else if (isOpenTag3D) { | ||
| currentSpool.kind = TagKind::OpenTag3D; | ||
| currentSpool.tag_data_valid = false; | ||
| lastOpenTag3D_ = ot3dData; | ||
| lastOpenTag3DValid_ = true; | ||
| lastTigerTagValid_ = false; | ||
| lastOpenSpoolValid_ = false; | ||
| Serial.printf("NFCManager: OpenTag3D detected — %s %s %.2fmm %ug\n", | ||
| ot3dData.manufacturer, ot3dData.base_material, | ||
| opentag3d_diameter_mm(&ot3dData), ot3dData.target_weight_g); | ||
| } else if (isOpenSpool) { | ||
| currentSpool.kind = TagKind::OpenSpoolTag; | ||
| currentSpool.tag_data_valid = false; | ||
| lastOpenSpool_ = openSpoolData; | ||
| lastOpenSpoolValid_ = true; | ||
| lastTigerTagValid_ = false; | ||
| lastOpenTag3DValid_ = false; | ||
| Serial.printf("NFCManager: OpenSpool detected — %s %s #%s\n", | ||
| openSpoolData.brand, openSpoolData.material, openSpoolData.color_hex); | ||
| } else { | ||
| currentSpool.kind = TagKind::GenericUidTag; | ||
| currentSpool.tag_data_valid = false; | ||
| lastTigerTagValid_ = false; | ||
| lastOpenTag3DValid_ = false; | ||
| lastOpenSpoolValid_ = false; | ||
| } |
There was a problem hiding this comment.
Missing lastOpenSpoolValid_ = false in TigerTag branch.
When a TigerTag is detected (lines 358-365), lastOpenTag3DValid_ is set to false but lastOpenSpoolValid_ is not cleared. The OpenTag3D branch (lines 366-375) correctly clears both other validity flags. This inconsistency could cause stale OpenSpool data to persist when switching from an OpenSpool tag to a TigerTag.
Proposed fix
if (isTigerTag) {
currentSpool.kind = TagKind::TigerTag;
currentSpool.tag_data_valid = false;
lastTigerTag_ = tigerData;
lastTigerTagValid_ = true;
lastOpenTag3DValid_ = false;
+ lastOpenSpoolValid_ = false;
Serial.printf("NFCManager: TigerTag detected — %s %s %s\n",
tigerData.brand_name, tigerData.material_name, tigerData.aspect1_name);📝 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 (isTigerTag) { | |
| currentSpool.kind = TagKind::TigerTag; | |
| currentSpool.tag_data_valid = false; | |
| lastTigerTag_ = tigerData; | |
| lastTigerTagValid_ = true; | |
| lastOpenTag3DValid_ = false; | |
| Serial.printf("NFCManager: TigerTag detected — %s %s %s\n", | |
| tigerData.brand_name, tigerData.material_name, tigerData.aspect1_name); | |
| } else if (isOpenTag3D) { | |
| currentSpool.kind = TagKind::OpenTag3D; | |
| currentSpool.tag_data_valid = false; | |
| lastOpenTag3D_ = ot3dData; | |
| lastOpenTag3DValid_ = true; | |
| lastTigerTagValid_ = false; | |
| lastOpenSpoolValid_ = false; | |
| Serial.printf("NFCManager: OpenTag3D detected — %s %s %.2fmm %ug\n", | |
| ot3dData.manufacturer, ot3dData.base_material, | |
| opentag3d_diameter_mm(&ot3dData), ot3dData.target_weight_g); | |
| } else if (isOpenSpool) { | |
| currentSpool.kind = TagKind::OpenSpoolTag; | |
| currentSpool.tag_data_valid = false; | |
| lastOpenSpool_ = openSpoolData; | |
| lastOpenSpoolValid_ = true; | |
| lastTigerTagValid_ = false; | |
| lastOpenTag3DValid_ = false; | |
| Serial.printf("NFCManager: OpenSpool detected — %s %s #%s\n", | |
| openSpoolData.brand, openSpoolData.material, openSpoolData.color_hex); | |
| } else { | |
| currentSpool.kind = TagKind::GenericUidTag; | |
| currentSpool.tag_data_valid = false; | |
| lastTigerTagValid_ = false; | |
| lastOpenTag3DValid_ = false; | |
| lastOpenSpoolValid_ = false; | |
| } | |
| if (isTigerTag) { | |
| currentSpool.kind = TagKind::TigerTag; | |
| currentSpool.tag_data_valid = false; | |
| lastTigerTag_ = tigerData; | |
| lastTigerTagValid_ = true; | |
| lastOpenTag3DValid_ = false; | |
| lastOpenSpoolValid_ = false; | |
| Serial.printf("NFCManager: TigerTag detected — %s %s %s\n", | |
| tigerData.brand_name, tigerData.material_name, tigerData.aspect1_name); | |
| } else if (isOpenTag3D) { | |
| currentSpool.kind = TagKind::OpenTag3D; | |
| currentSpool.tag_data_valid = false; | |
| lastOpenTag3D_ = ot3dData; | |
| lastOpenTag3DValid_ = true; | |
| lastTigerTagValid_ = false; | |
| lastOpenSpoolValid_ = false; | |
| Serial.printf("NFCManager: OpenTag3D detected — %s %s %.2fmm %ug\n", | |
| ot3dData.manufacturer, ot3dData.base_material, | |
| opentag3d_diameter_mm(&ot3dData), ot3dData.target_weight_g); | |
| } else if (isOpenSpool) { | |
| currentSpool.kind = TagKind::OpenSpoolTag; | |
| currentSpool.tag_data_valid = false; | |
| lastOpenSpool_ = openSpoolData; | |
| lastOpenSpoolValid_ = true; | |
| lastTigerTagValid_ = false; | |
| lastOpenTag3DValid_ = false; | |
| Serial.printf("NFCManager: OpenSpool detected — %s %s #%s\n", | |
| openSpoolData.brand, openSpoolData.material, openSpoolData.color_hex); | |
| } else { | |
| currentSpool.kind = TagKind::GenericUidTag; | |
| currentSpool.tag_data_valid = false; | |
| lastTigerTagValid_ = false; | |
| lastOpenTag3DValid_ = false; | |
| lastOpenSpoolValid_ = false; | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[note] 358-358: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 366-366: +1, nesting level increased to 2
(clang)
[note] 376-376: +1, nesting level increased to 2
(clang)
[note] 385-385: +1, nesting level increased to 2
(clang)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NFCManager.cpp` around lines 358 - 391, In the TigerTag handling block
(where currentSpool.kind is set to TagKind::TigerTag and lastTigerTag_ /
lastTigerTagValid_ are updated), also clear the OpenSpool validity by setting
lastOpenSpoolValid_ = false so stale OpenSpool data can't persist; locate the
TigerTag branch around currentSpool and lastTigerTag_ and add the missing
lastOpenSpoolValid_ = false alongside the existing lastOpenTag3DValid_ = false.
| int WebServerManager::enrichFindSpoolByUid(WiFiClient& client, HTTPClient& http, | ||
| const char* baseUrl, const char* quotedUid, float& outInitialWeight) { | ||
| char url[256]; | ||
| snprintf(url, sizeof(url), "%s/api/v1/spool?limit=200", baseUrl); | ||
| http.begin(client, url); | ||
| http.setTimeout(5000); | ||
| int code = http.GET(); | ||
| int spoolId = -1; | ||
| outInitialWeight = 0.0f; | ||
| if (code == 200) { | ||
| String response = http.getString(); | ||
| DynamicJsonDocument sDoc(16384); | ||
| if (!deserializeJson(sDoc, response)) { | ||
| JsonArray arr = sDoc.as<JsonArray>(); | ||
| for (size_t i = 0; i < arr.size(); i++) { | ||
| const char* nfcId = arr[i]["extra"]["nfc_id"] | ""; | ||
| if (strcmp(nfcId, quotedUid) != 0) continue; | ||
| if (arr[i]["archived"] | false) continue; | ||
| spoolId = arr[i]["id"] | -1; | ||
| outInitialWeight = arr[i]["initial_weight"] | 0.0f; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| http.end(); | ||
| return spoolId; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider larger buffer for enrichFindSpoolByUid when handling many spools.
The function fetches up to 200 spools (?limit=200) but uses a 16KB DynamicJsonDocument. With typical spool JSON (~100-200 bytes each), 200 spools could exceed 16KB, causing deserialization to fail silently. Consider either reducing the limit or increasing the buffer.
Proposed adjustment
- DynamicJsonDocument sDoc(16384);
+ DynamicJsonDocument sDoc(32768); // 32KB for up to 200 spoolsAlternatively, reduce the limit:
- snprintf(url, sizeof(url), "%s/api/v1/spool?limit=200", baseUrl);
+ snprintf(url, sizeof(url), "%s/api/v1/spool?limit=100", baseUrl);📝 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.
| int WebServerManager::enrichFindSpoolByUid(WiFiClient& client, HTTPClient& http, | |
| const char* baseUrl, const char* quotedUid, float& outInitialWeight) { | |
| char url[256]; | |
| snprintf(url, sizeof(url), "%s/api/v1/spool?limit=200", baseUrl); | |
| http.begin(client, url); | |
| http.setTimeout(5000); | |
| int code = http.GET(); | |
| int spoolId = -1; | |
| outInitialWeight = 0.0f; | |
| if (code == 200) { | |
| String response = http.getString(); | |
| DynamicJsonDocument sDoc(16384); | |
| if (!deserializeJson(sDoc, response)) { | |
| JsonArray arr = sDoc.as<JsonArray>(); | |
| for (size_t i = 0; i < arr.size(); i++) { | |
| const char* nfcId = arr[i]["extra"]["nfc_id"] | ""; | |
| if (strcmp(nfcId, quotedUid) != 0) continue; | |
| if (arr[i]["archived"] | false) continue; | |
| spoolId = arr[i]["id"] | -1; | |
| outInitialWeight = arr[i]["initial_weight"] | 0.0f; | |
| break; | |
| } | |
| } | |
| } | |
| http.end(); | |
| return spoolId; | |
| } | |
| int WebServerManager::enrichFindSpoolByUid(WiFiClient& client, HTTPClient& http, | |
| const char* baseUrl, const char* quotedUid, float& outInitialWeight) { | |
| char url[256]; | |
| snprintf(url, sizeof(url), "%s/api/v1/spool?limit=200", baseUrl); | |
| http.begin(client, url); | |
| http.setTimeout(5000); | |
| int code = http.GET(); | |
| int spoolId = -1; | |
| outInitialWeight = 0.0f; | |
| if (code == 200) { | |
| String response = http.getString(); | |
| DynamicJsonDocument sDoc(32768); // 32KB for up to 200 spools | |
| if (!deserializeJson(sDoc, response)) { | |
| JsonArray arr = sDoc.as<JsonArray>(); | |
| for (size_t i = 0; i < arr.size(); i++) { | |
| const char* nfcId = arr[i]["extra"]["nfc_id"] | ""; | |
| if (strcmp(nfcId, quotedUid) != 0) continue; | |
| if (arr[i]["archived"] | false) continue; | |
| spoolId = arr[i]["id"] | -1; | |
| outInitialWeight = arr[i]["initial_weight"] | 0.0f; | |
| break; | |
| } | |
| } | |
| } | |
| http.end(); | |
| return spoolId; | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 1908-1908: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 1908-1908: method 'enrichFindSpoolByUid' can be made static
(readability-convert-member-functions-to-static)
[warning] 1908-1908: 2 adjacent parameters of 'enrichFindSpoolByUid' of similar type ('int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 1908-1908: the first parameter in the range is 'client'
(clang)
[note] 1908-1908: the last parameter in the range is 'http'
(clang)
[warning] 1909-1909: 2 adjacent parameters of 'enrichFindSpoolByUid' of similar type ('const char *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 1909-1909: the first parameter in the range is 'baseUrl'
(clang)
[note] 1909-1909: the last parameter in the range is 'quotedUid'
(clang)
[warning] 1910-1910: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 1914-1914: variable 'code' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1916-1916: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 1918-1918: variable 'response' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1919-1919: variable 'sDoc' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1921-1921: 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 1908 - 1934, The DynamicJsonDocument
buffer (16384) in enrichFindSpoolByUid is too small for up to 200 spools and can
cause deserializeJson to fail; after calling http.getString() allocate the JSON
buffer based on the response size (e.g., DynamicJsonDocument
sDoc(response.length() + 1024) or response.length()*1.2) and then call
deserializeJson(sDoc, response) checking its return value for success, or
alternatively reduce the "?limit=200" to a smaller value—adjust the code around
the response, DynamicJsonDocument sDoc, and the deserializeJson call in
enrichFindSpoolByUid accordingly.
…filter streamFindSpoolByNfcId() was a 190-line, 64-branch-point character-level state machine. Replaced with ArduinoJson's DeserializationOption::Filter which parses only id, archived, and extra.nfc_id from the stream — same low memory usage (~4KB vs ~600B), dramatically simpler code. 190 lines → 50 lines. 64 branch points → 8.
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/SpoolmanManager.cpp`:
- Around line 255-285: The current use of an unbounded JsonDocument
(JsonDocument doc) when calling deserializeJson(...,
DeserializationOption::Filter(filter)) can exhaust heap for large spool lists;
replace it with a bounded StaticJsonDocument sized to a safe upper bound
(estimate per-spool filtered size ~50 bytes × max expected spools) and pass that
to deserializeJson, or detect DeserializationError::NoMemory from err and handle
it (log and fall back to a streaming/iterative parse or return an error). Update
references to JsonDocument doc to the new StaticJsonDocument instance and keep
the existing deserialization and iteration logic (deserializeJson,
DeserializationError err, streamHttp.getStreamPtr(), doc.as<JsonArray>(), spool
loop) so overflow is caught and controlled.
🪄 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: e8b3cd50-3479-4890-944f-47199bbc3cfa
📒 Files selected for processing (1)
src/SpoolmanManager.cpp
| // Filter: only extract id, archived, and extra.nfc_id from each spool | ||
| JsonDocument filter; | ||
| filter[0]["id"] = true; | ||
| filter[0]["archived"] = true; | ||
| filter[0]["extra"]["nfc_id"] = true; | ||
|
|
||
| int bestMatchId = -1; | ||
| int currentId = -1; | ||
| bool nfcIdMatched = false; // true if nfc_id matched in current spool object | ||
| int depth = 0; | ||
| bool inExtra = false; | ||
| int extraDepth = 0; | ||
|
|
||
| char buf[512]; | ||
| char keyBuf[16] = {}; | ||
| int keyPos = 0; | ||
| char valBuf[80] = {}; | ||
| int valPos = 0; | ||
| bool inString = false; | ||
| bool escaped = false; | ||
| bool inKey = false; | ||
| bool inValue = false; | ||
| bool inNumValue = false; | ||
|
|
||
| unsigned long lastData = millis(); | ||
| bool timedOut = false; | ||
|
|
||
| while (stream->available() || stream->connected()) { | ||
| if (millis() - lastData > 10000) { | ||
| Serial.println("SpoolmanManager: streamFind timeout"); | ||
| timedOut = true; | ||
| break; | ||
| } | ||
|
|
||
| int avail = stream->available(); | ||
| if (avail <= 0) { delay(1); continue; } | ||
| lastData = millis(); | ||
|
|
||
| int toRead = avail > (int)sizeof(buf) ? (int)sizeof(buf) : avail; | ||
| int bytesRead = stream->readBytes(buf, toRead); | ||
|
|
||
| for (int i = 0; i < bytesRead; i++) { | ||
| char c = buf[i]; | ||
|
|
||
| if (escaped) { | ||
| escaped = false; | ||
| if (inString && inKey && keyPos < (int)sizeof(keyBuf) - 1) | ||
| keyBuf[keyPos++] = c; | ||
| if (inString && inValue && valPos < (int)sizeof(valBuf) - 1) | ||
| valBuf[valPos++] = c; | ||
| continue; | ||
| } | ||
|
|
||
| if (c == '\\' && inString) { | ||
| escaped = true; | ||
| if (inValue && valPos < (int)sizeof(valBuf) - 1) | ||
| valBuf[valPos++] = c; | ||
| continue; | ||
| } | ||
| JsonDocument doc; | ||
| DeserializationError err = deserializeJson(doc, *streamHttp.getStreamPtr(), | ||
| DeserializationOption::Filter(filter)); | ||
| streamHttp.end(); | ||
|
|
||
| if (c == '"') { | ||
| if (!inString) { | ||
| inString = true; | ||
| if (!inValue) { | ||
| inKey = true; | ||
| keyPos = 0; | ||
| memset(keyBuf, 0, sizeof(keyBuf)); | ||
| } else { | ||
| valPos = 0; | ||
| memset(valBuf, 0, sizeof(valBuf)); | ||
| } | ||
| } else { | ||
| inString = false; | ||
| if (inKey) { | ||
| inKey = false; | ||
| keyBuf[keyPos] = '\0'; | ||
| } | ||
| if (inValue) { | ||
| valBuf[valPos] = '\0'; | ||
| if (inExtra && strcmp(keyBuf, "nfc_id") == 0) { | ||
| // Strip escaped inner quotes: \"UUID\" → UUID | ||
| char* nfcVal = valBuf; | ||
| int nfcLen = strlen(nfcVal); | ||
| if (nfcLen >= 2 && nfcVal[0] == '\\' && nfcVal[1] == '"') { | ||
| nfcVal += 2; nfcLen -= 2; | ||
| } | ||
| if (nfcLen >= 2 && nfcVal[nfcLen - 2] == '\\' && nfcVal[nfcLen - 1] == '"') { | ||
| nfcVal[nfcLen - 2] = '\0'; | ||
| } | ||
| nfcLen = strlen(nfcVal); | ||
| if (nfcLen >= 2 && nfcVal[0] == '"' && nfcVal[nfcLen - 1] == '"') { | ||
| nfcVal[nfcLen - 1] = '\0'; | ||
| nfcVal++; | ||
| } | ||
| if (strcasecmp(nfcVal, uuid) == 0) { | ||
| nfcIdMatched = true; | ||
| } | ||
| } | ||
| inValue = false; | ||
| } | ||
| } | ||
| continue; | ||
| } | ||
| if (err) { | ||
| Serial.printf("SpoolmanManager: streamFind parse error: %s\n", err.c_str()); | ||
| return -2; | ||
| } | ||
|
|
||
| if (inString) { | ||
| if (inKey && keyPos < (int)sizeof(keyBuf) - 1) | ||
| keyBuf[keyPos++] = c; | ||
| if (inValue && valPos < (int)sizeof(valBuf) - 1) | ||
| valBuf[valPos++] = c; | ||
| continue; | ||
| } | ||
| // Build quoted UID for comparison ("04A651AD8F6180") | ||
| char quotedUuid[130]; | ||
| snprintf(quotedUuid, sizeof(quotedUuid), "\"%s\"", uuid); | ||
|
|
||
| // Structural characters outside strings | ||
| if (c == '{') { | ||
| depth++; | ||
| inValue = false; // opening brace ends the value context | ||
| inNumValue = false; | ||
| if (depth == 1) { | ||
| currentId = -1; | ||
| nfcIdMatched = false; | ||
| inExtra = false; | ||
| } else if (depth >= 2 && strcmp(keyBuf, "extra") == 0) { | ||
| inExtra = true; | ||
| extraDepth = depth; | ||
| } | ||
| } else if (c == '}') { | ||
| if (inExtra && depth == extraDepth) { | ||
| inExtra = false; | ||
| } | ||
| // Clear numeric state on every closing brace to prevent | ||
| // nested id values (filament.id, vendor.id) from leaking | ||
| inValue = false; | ||
| inNumValue = false; | ||
| depth--; | ||
| if (depth == 0) { | ||
| // Deferred match — both id and nfc_id are now known | ||
| if (nfcIdMatched && currentId > bestMatchId) { | ||
| bestMatchId = currentId; | ||
| } | ||
| } | ||
| } else if (c == ':') { | ||
| inValue = true; | ||
| valPos = 0; | ||
| memset(valBuf, 0, sizeof(valBuf)); | ||
| inNumValue = false; | ||
| } else if (c == '[') { | ||
| inValue = false; | ||
| inNumValue = false; | ||
| } else if (c == ',' || c == ']') { | ||
| if (inNumValue && depth == 1 && strcmp(keyBuf, "id") == 0) { | ||
| valBuf[valPos] = '\0'; | ||
| currentId = atoi(valBuf); | ||
| } | ||
| inValue = false; | ||
| inNumValue = false; | ||
| } else if (inValue && c >= '0' && c <= '9') { | ||
| inNumValue = true; | ||
| if (valPos < (int)sizeof(valBuf) - 1) | ||
| valBuf[valPos++] = c; | ||
| } else if (inValue && (c == '-' || c == '.')) { | ||
| if (valPos < (int)sizeof(valBuf) - 1) | ||
| valBuf[valPos++] = c; | ||
| } | ||
| int bestMatchId = -1; | ||
| for (JsonObject spool : doc.as<JsonArray>()) { | ||
| if (spool["archived"] | false) continue; | ||
| const char* nfcId = spool["extra"]["nfc_id"] | ""; | ||
| // nfc_id is stored double-quoted in Spoolman: "\"UUID\"" | ||
| // Compare both with and without outer quotes | ||
| if (strcasecmp(nfcId, uuid) == 0 || strcasecmp(nfcId, quotedUuid) == 0) { | ||
| int id = spool["id"] | -1; | ||
| if (id > bestMatchId) bestMatchId = id; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
ArduinoJson deserializeJson Filter option array syntax
💡 Result:
In ArduinoJson, the Filter for an array is expressed by creating an array in the filter document with only one element at index 0. That element is used as the filter for every item in the input array; any other filter elements ([1], [2], …) are ignored. [1], [2]
Array of objects (keep selected fields from each object)
StaticJsonDocument<256> filter;
filter["items"][0]["id"] = true;
filter["items"][0]["name"] = true;
deserializeJson(doc, input, DeserializationOption::Filter(filter));This keeps id and name for each object in the input items array. [1], [2]
Array of primitives (keep the values)
StaticJsonDocument<64> filter;
filter["values"][0] = true;
deserializeJson(doc, input, DeserializationOption::Filter(filter));This keeps the values array contents (primitives) while still allowing other parts of the JSON to be filtered out. [1]
Nested arrays
You apply the same rule at each array level:
StaticJsonDocument<256> filter;
filter["outer"][0]["inner"][0]["x"] = true;Meaning: for every element of outer, look at inner (array) and for every element of inner, keep field x. [1], [2]
Notes
- You must pass the filter as
DeserializationOption::Filter(filter). [1] - Filtering needs a bit of extra
JsonDocumentcapacity (keys are stored temporarily). [1]
Sources: ArduinoJson deserializeJson() docs and filtering notes [1], ArduinoJson filtering announcement (array rule) [2].
🏁 Script executed:
# Find and read SpoolmanManager.cpp around lines 255-285
fd SpoolmanManager.cppRepository: SpoolSense/spoolsense_scanner
Length of output: 96
🏁 Script executed:
# Check for ArduinoJson version in project configuration
find . -type f \( -name "CMakeLists.txt" -o -name "platformio.ini" -o -name "library.json" -o -name "package.json" \) | head -20Repository: SpoolSense/spoolsense_scanner
Length of output: 161
🏁 Script executed:
# Read SpoolmanManager.cpp around the specified lines (250-290)
sed -n '250,290p' src/SpoolmanManager.cppRepository: SpoolSense/spoolsense_scanner
Length of output: 1580
🏁 Script executed:
# Check platformio.ini for ArduinoJson version and dependencies
cat platformio.iniRepository: SpoolSense/spoolsense_scanner
Length of output: 1598
Clean refactoring using ArduinoJson Filter—well done.
The Filter approach correctly extracts only needed fields (id, archived, extra.nfc_id) from the array response. The UUID matching logic properly handles both raw and double-quoted storage formats, and strcasecmp is appropriate for case-insensitive hex UID comparison.
Consider bounded document sizing for memory-constrained environments: JsonDocument doc; uses dynamic sizing. For Spoolman instances with large spool catalogs, the filtered document may still consume significant heap (each spool contributes ~40–60 bytes for extracted fields). If memory becomes constrained, consider implementing chunked streaming or a bounded document capacity with overflow detection.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 256-256: variable 'filter' is not initialized
(cppcoreguidelines-init-variables)
[warning] 261-261: variable 'doc' is not initialized
(cppcoreguidelines-init-variables)
[warning] 262-262: variable 'err' is not initialized
(cppcoreguidelines-init-variables)
[warning] 272-272: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SpoolmanManager.cpp` around lines 255 - 285, The current use of an
unbounded JsonDocument (JsonDocument doc) when calling deserializeJson(...,
DeserializationOption::Filter(filter)) can exhaust heap for large spool lists;
replace it with a bounded StaticJsonDocument sized to a safe upper bound
(estimate per-spool filtered size ~50 bytes × max expected spools) and pass that
to deserializeJson, or detect DeserializationError::NoMemory from err and handle
it (log and fall back to a streaming/iterative parse or return an error). Update
references to JsonDocument doc to the new StaticJsonDocument instance and keep
the existing deserialization and iteration logic (deserializeJson,
DeserializationError err, streamHttp.getStreamPtr(), doc.as<JsonArray>(), spool
loop) so overflow is caught and controlled.
…noJson Single spool response is ~700 bytes — no need for a streaming parser. Replaced 190-line hand-rolled json_reader with direct deserializeJson(). 190 lines → 40 lines. ~40 branch points → ~8.
scanLoop() was ~215 lines with RF management, tag detection, classification dispatch, and tag removal all inline. Now a 30-line main loop that calls: - prepareRF() — watchdog checks, RF field management - isSkippableDuplicate() — duplicate + write suppression check - handleNewTag() — classify and dispatch to format handler - handleTagAbsent() — clear state, send TAG_REMOVED scanLoop complexity: ~45 → ~10 branch points.
…ing helper - optMaterialToString() — switch extracted as static lookup, PA variants collapsed - sendPrinterWarning() — shared message builder (was duplicated for type + temp) - checkFilamentMismatch() flattened with early returns, 93 → 30 lines Complexity: ~33 → ~10 branch points.
1 participant
Summary
executeWrite()(126 → ~15 branch points)readAndProcessISO14443Tag()fromscanLoop()(90 → ~45)handleApiStatus()(74 → ~10)handleApiSpoolmanSaveEnrichment()(57 → ~12)findNdefMediaRecord()+readNdefPayload()(5 levels → 2)buildNdefTlv(),validateWriteUid(),forceRescan(),serializeEnrichment()Test plan
/api/statusreturns correct dataSummary by CodeRabbit
Improvements
Bug Fixes
Refactor
Chores