Conversation
- lint.yml: add cpp lint, cppcheck suppress updates - cpp_lint.py: new backend - add cppcheck-suppress - general: small changes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds CI linting and IWYU jobs, a custom C++ lint script, native and frontend unit tests, and Vitest config; extracts and reorganizes C++ utilities into testable headers; updates many include paths; tightens TypeScript typings across frontend components; and introduces native test harness and supporting stubs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 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 (1)
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
164-175:⚠️ Potential issue | 🟠 MajorUnsigned slider bounds prevent negative swirl reversal from UI.
Line 174's
addControl(swirlVal, "swirl", "slider")defaults to0–255bounds, butswirlValis declaredint8_tto support negative values for the reversal check at line 215. The UI slider cannot be adjusted below 0, making theswirlVal < 0branch unreachable from the UI.🔧 Suggested fix
- addControl(swirlVal, "swirl", "slider"); + addControl(swirlVal, "swirl", "slider", -127, 127);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Modifiers/M_MoonLight.h` around lines 164 - 175, The slider control added in setup uses addControl(swirlVal, "swirl", "slider") but swirlVal is an int8_t allowing negative values (checked later via swirlVal < 0), yet the slider defaults to unsigned 0–255 so the UI cannot produce negatives; change the control to specify signed bounds or a control type that supports negatives (e.g., provide min/max parameters or use a signed slider API) so the UI can set swirlVal < 0, updating the addControl call in setup (the one that registers swirlVal/"swirl") to include an explicit negative minimum (and appropriate max) consistent with the int8_t range.
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Effects/E_WLED.h (1)
159-159: Prefer removing dead writes in effect loops instead of suppressing them.These assignments are currently unused/no-op, so deleting them is cleaner and avoids extra per-frame work.
♻️ Example cleanup direction
- unsigned pixIntensity = MIN(2.0f * sharedData.bands[freqBand], 255); // cppcheck-suppress unreadVariable -- WLED ported code + // removed unused pixIntensity - aux1 = aux0; // cppcheck-suppress unreadVariable -- WLED ported code + // removed no-op state shuffle (locals are pass-by-value here) - CRGB color = CRGB(0, 0, 0); + CRGB color;Also applies to: 1699-1699, 1949-1954, 2824-2824, 2857-2857, 3528-3533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_WLED.h` at line 159, The assignment to pixIntensity (and other similar dead writes flagged at the noted ranges) is unused and should be removed rather than suppressed; find the dead-write statements (e.g., the line assigning 'unsigned pixIntensity = MIN(2.0f * sharedData.bands[freqBand], 255);' and the other occurrences around the referenced ranges) inside the WLED effect loops and delete those unused variable assignments and any accompanying cppcheck-suppress comments, ensuring no remaining references rely on those variables (if they do, refactor to use the value where intended).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/cpp_lint.py`:
- Around line 165-169: The serial-print checker currently strips comments only,
causing false positives when string literals contain "Serial.print"; update the
logic that builds line_no_comment before calling RE_SERIAL_PRINT.search by also
removing double-quoted and single-quoted string literals using the same regex
patterns used in count_braces() (i.e. strip "..." and '...' sequences), then run
RE_SERIAL_PRINT.search on that cleaned string and keep the existing RE_OK_LINT
check and violation append (refer to the symbols line_no_comment,
RE_SERIAL_PRINT, RE_OK_LINT and the count_braces() string regexes to implement
the change).
- Around line 108-120: The class inheritance detection (RE_CLASS_OPEN handling)
fails for multiline declarations; modify the logic so when RE_CLASS_OPEN matches
but the opening '{' is not yet seen on that line you buffer subsequent lines
into a single declaration string until you encounter the '{' (or reach
end-of-file), then run the Node-inheritance check (use re.search on the buffered
declaration for r':\s*(?:public\s+)?(?:\w+,\s*)*Node\b') to set is_node; ensure
you do not shadow loop variables or mutate the main line index (use a local
iterator or while loop to read ahead), and record depth_at_open using the
current brace_depth before counting the buffered line's braces to avoid
off-by-one errors when pushing into class_stack.
---
Outside diff comments:
In `@src/MoonLight/Nodes/Modifiers/M_MoonLight.h`:
- Around line 164-175: The slider control added in setup uses
addControl(swirlVal, "swirl", "slider") but swirlVal is an int8_t allowing
negative values (checked later via swirlVal < 0), yet the slider defaults to
unsigned 0–255 so the UI cannot produce negatives; change the control to specify
signed bounds or a control type that supports negatives (e.g., provide min/max
parameters or use a signed slider API) so the UI can set swirlVal < 0, updating
the addControl call in setup (the one that registers swirlVal/"swirl") to
include an explicit negative minimum (and appropriate max) consistent with the
int8_t range.
---
Nitpick comments:
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Line 159: The assignment to pixIntensity (and other similar dead writes
flagged at the noted ranges) is unused and should be removed rather than
suppressed; find the dead-write statements (e.g., the line assigning 'unsigned
pixIntensity = MIN(2.0f * sharedData.bands[freqBand], 255);' and the other
occurrences around the referenced ranges) inside the WLED effect loops and
delete those unused variable assignments and any accompanying cppcheck-suppress
comments, ensuring no remaining references rely on those variables (if they do,
refactor to use the value where intended).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/lint.ymlci/cpp_lint.pysrc/MoonBase/Char.hsrc/MoonBase/Modules/FileManager.cppsrc/MoonBase/Modules/ModuleDevices.hsrc/MoonBase/Modules/ModuleIO.hsrc/MoonBase/Utilities.cppsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Nodes/Drivers/D_IMU.hsrc/MoonLight/Nodes/Drivers/D__Sandbox.hsrc/MoonLight/Nodes/Effects/E_FastLED.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Layouts/L_MoonLight.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.hsrc/main.cpp
💤 Files with no reviewable changes (2)
- src/MoonLight/Nodes/Layouts/L_MoonLight.h
- src/MoonLight/Nodes/Effects/E_FastLED.h
Nodes: addControl and updateControl: add support for int8_t PinWheel: enable negative swirl
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ci/cpp_lint.py (1)
108-130:⚠️ Potential issue | 🟠 MajorBuffer class declarations until
{before enteringclass_stack.At Line 116, a class frame is pushed even when the class body starts later; at Line 129 it is popped immediately when
brace_depthis unchanged. This still misses valid multiline declarations and prevents Node-only rules from running for those classes.🔧 Suggested direction
- m = RE_CLASS_OPEN.match(line) + m = RE_CLASS_OPEN.match(line) if m: class_name = m.group(1) - is_node = bool(re.search(r':\s*(?:public\s+)?(?:\w+,\s*)*Node\b', line)) - class_stack.append({ - 'name': class_name, - 'depth_at_open': brace_depth, - 'is_node': is_node, - }) + # Parse declaration header across following lines until '{' or ';' + # Then push class frame only if '{' is part of that declaration. + # Evaluate Node inheritance against the buffered header.#!/bin/bash python3 - <<'PY' import re node_re = re.compile(r':\s*(?:public\s+)?(?:\w+,\s*)*Node\b') cases = [ ("class A : public Node {", True, "single-line"), ("class A", False, "multiline first line (current implementation checks only this line)"), (": public Node {", True, "multiline inheritance line"), ("class A : public Base, public Node {", False, "multiple inheritance with access specifier"), ] for text, expected_current, name in cases: got = bool(node_re.search(text)) print(f"{name:45} got={got} expected_current={expected_current}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/cpp_lint.py` around lines 108 - 130, The code currently pushes a class frame to class_stack as soon as RE_CLASS_OPEN matches, which captures classes whose '{' appears on a later line and then gets immediately popped because brace_depth hasn't increased; instead buffer the class declaration until the opening brace is seen: when RE_CLASS_OPEN matches, store a pending_class (with 'name' and an initial 'is_node' computed from that line) but do not append to class_stack; always update brace_depth via count_braces(line); after updating brace_depth, if a pending_class exists and this line contains the opening brace (or delta>0 indicating a '{' was counted), push the pending_class onto class_stack with 'depth_at_open' set to the brace_depth value before counting the new brace (i.e., previous_brace_depth) and the final 'is_node' determined by scanning the buffered declaration lines (include following lines until the '{') using the same Node regex; clear pending_class after pushing; ensure the pop loop that compares brace_depth to class_stack[-1]['depth_at_open'] remains unchanged so Node-only rules run correctly for multi-line declarations (reference symbols: RE_CLASS_OPEN, class_stack, pending_class, is_node, count_braces, brace_depth).
🧹 Nitpick comments (1)
src/MoonBase/Nodes.h (1)
149-158: Replace magic control-size codes with named constants.Line 150, Line 156, and Line 158 use encoded literals (
108,33,34). Centralizing these in one constant set avoids mismatch withNode::updateControl.♻️ Proposed refactor
--- a/src/MoonBase/Nodes.h +++ b/src/MoonBase/Nodes.h @@ class Node { public: + static constexpr uint8_t CONTROL_SIZE_U8 = 8; + static constexpr uint8_t CONTROL_SIZE_I8 = 108; + static constexpr uint8_t CONTROL_SIZE_U16 = 16; + static constexpr uint8_t CONTROL_SIZE_U32 = 32; + static constexpr uint8_t CONTROL_SIZE_INT = 33; + static constexpr uint8_t CONTROL_SIZE_FLOAT = 34; @@ - if (std::is_same<ControlType, uint8_t>::value) { - control["size"] = 8; + if (std::is_same<ControlType, uint8_t>::value) { + control["size"] = CONTROL_SIZE_U8; } else if (std::is_same<ControlType, int8_t>::value) { - control["size"] = 108; // code for int8_t + control["size"] = CONTROL_SIZE_I8; // code for int8_t } else if (std::is_same<ControlType, uint16_t>::value) { - control["size"] = 16; + control["size"] = CONTROL_SIZE_U16; } else if (std::is_same<ControlType, uint32_t>::value) { - control["size"] = 32; + control["size"] = CONTROL_SIZE_U32; } else if (std::is_same<ControlType, int>::value) { - control["size"] = 33; // code for int (which is 32 bits) + control["size"] = CONTROL_SIZE_INT; // code for int (32-bit) } else if (std::is_same<ControlType, float>::value) { - control["size"] = 34; // code for float float (which is 32 bits) + control["size"] = CONTROL_SIZE_FLOAT; // code for float (32-bit) } else {--- a/src/MoonBase/Nodes.cpp +++ b/src/MoonBase/Nodes.cpp @@ - if (control["size"] == 8) { + if (control["size"] == Node::CONTROL_SIZE_U8) { @@ - } else if (control["size"] == 108) { + } else if (control["size"] == Node::CONTROL_SIZE_I8) { @@ - } else if (control["size"] == 16) { + } else if (control["size"] == Node::CONTROL_SIZE_U16) { @@ - } else if (control["size"] == 32) { + } else if (control["size"] == Node::CONTROL_SIZE_U32) { @@ - } else if (control["size"] == 33) { + } else if (control["size"] == Node::CONTROL_SIZE_INT) { @@ - } else if (control["size"] == 34) { + } else if (control["size"] == Node::CONTROL_SIZE_FLOAT) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Nodes.h` around lines 149 - 158, The code uses magic numeric "size" codes (e.g., 108, 33, 34) when setting control["size"] for certain ControlType specializations; replace these literals with named constants (e.g., CONTROL_SIZE_INT8, CONTROL_SIZE_INT, CONTROL_SIZE_FLOAT) declared centrally (preferably as enum or constexpr in the same header/namespace) and use those constants in the branches for std::is_same<ControlType,...>; ensure the new constant names are consistent with Node::updateControl expectations and update any references to the old numeric codes to use the new constants so the mapping is centralized and less error-prone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/cpp_lint.py`:
- Around line 165-167: The current sanitization removes line comments before
stripping string literals so occurrences of '//' inside strings get treated as
comments; in the code that builds line_no_comment (using re.sub calls), move the
string-literal stripping calls (the two re.subs that replace double-quoted and
single-quoted strings) to run before the re.sub that removes '//' comments
(i.e., perform the '"' and "'" regex replacements first, then remove //
comments) so Serial.print and similar tokens inside strings are not
inadvertently truncated; update the sequence around the line_no_comment variable
and ensure the three re.sub calls retain their existing patterns and order for
double vs single quotes.
---
Duplicate comments:
In `@ci/cpp_lint.py`:
- Around line 108-130: The code currently pushes a class frame to class_stack as
soon as RE_CLASS_OPEN matches, which captures classes whose '{' appears on a
later line and then gets immediately popped because brace_depth hasn't
increased; instead buffer the class declaration until the opening brace is seen:
when RE_CLASS_OPEN matches, store a pending_class (with 'name' and an initial
'is_node' computed from that line) but do not append to class_stack; always
update brace_depth via count_braces(line); after updating brace_depth, if a
pending_class exists and this line contains the opening brace (or delta>0
indicating a '{' was counted), push the pending_class onto class_stack with
'depth_at_open' set to the brace_depth value before counting the new brace
(i.e., previous_brace_depth) and the final 'is_node' determined by scanning the
buffered declaration lines (include following lines until the '{') using the
same Node regex; clear pending_class after pushing; ensure the pop loop that
compares brace_depth to class_stack[-1]['depth_at_open'] remains unchanged so
Node-only rules run correctly for multi-line declarations (reference symbols:
RE_CLASS_OPEN, class_stack, pending_class, is_node, count_braces, brace_depth).
---
Nitpick comments:
In `@src/MoonBase/Nodes.h`:
- Around line 149-158: The code uses magic numeric "size" codes (e.g., 108, 33,
34) when setting control["size"] for certain ControlType specializations;
replace these literals with named constants (e.g., CONTROL_SIZE_INT8,
CONTROL_SIZE_INT, CONTROL_SIZE_FLOAT) declared centrally (preferably as enum or
constexpr in the same header/namespace) and use those constants in the branches
for std::is_same<ControlType,...>; ensure the new constant names are consistent
with Node::updateControl expectations and update any references to the old
numeric codes to use the new constants so the mapping is centralized and less
error-prone.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ci/cpp_lint.pysrc/MoonBase/Nodes.cppsrc/MoonBase/Nodes.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Nodes/Effects/E_WLED.h
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci/cpp_lint.py (1)
108-120:⚠️ Potential issue | 🟠 MajorHandle multiline class headers before computing
is_node.At Line 113, Node inheritance is checked only on the current line, and the frame pushed at Line 116 can be popped before a later
{is seen (Line 129). This misses valid multiline C++ declarations and silently skips rule enforcement for those classes.🔧 Suggested fix
def check_file(path: Path) -> list: @@ - class_stack: list = [] + class_stack: list = [] + pending_class = None # {'name': str, 'depth_at_open': int, 'header': str} @@ - m = RE_CLASS_OPEN.match(line) - if m: - class_name = m.group(1) - is_node = bool(re.search(r':\s*(?:public\s+)?(?:\w+,\s*)*Node\b', line)) - class_stack.append({ - 'name': class_name, - 'depth_at_open': brace_depth, - 'is_node': is_node, - }) + header_line = re.sub(r'//.*', '', line) + m = RE_CLASS_OPEN.match(header_line) + if pending_class: + pending_class['header'] += ' ' + header_line.strip() + if '{' in header_line: + class_stack.append({ + 'name': pending_class['name'], + 'depth_at_open': pending_class['depth_at_open'], + 'is_node': bool(re.search(r':\s*(?:public\s+)?(?:\w+,\s*)*Node\b', pending_class['header'])), + }) + pending_class = None + elif m: + pending_class = { + 'name': m.group(1), + 'depth_at_open': brace_depth, + 'header': header_line.strip(), + } + if '{' in header_line: + class_stack.append({ + 'name': pending_class['name'], + 'depth_at_open': pending_class['depth_at_open'], + 'is_node': bool(re.search(r':\s*(?:public\s+)?(?:\w+,\s*)*Node\b', pending_class['header'])), + }) + pending_class = None#!/bin/bash python3 - << 'PY' import re RE_CLASS_OPEN = re.compile(r'^\s*(?:class|struct)\s+(\w+)') RE_NODE = re.compile(r':\s*(?:public\s+)?(?:\w+,\s*)*Node\b') def count_braces(line): s = re.sub(r'"(?:[^"\\]|\\.)*"', '""', line) s = re.sub(r"'(?:[^'\\]|\\.)*'", "''", s) s = re.sub(r'//.*', '', s) return s.count('{') - s.count('}') lines = [ "class Foo", " : public Node", "{", " String badMember;", "};", ] class_stack = [] brace_depth = 0 for lineno, line in enumerate(lines, start=1): m = RE_CLASS_OPEN.match(line) if m: class_stack.append({ "name": m.group(1), "depth_at_open": brace_depth, "is_node": bool(RE_NODE.search(line)), }) brace_depth += count_braces(line) while class_stack and brace_depth <= class_stack[-1]["depth_at_open"]: class_stack.pop() print(f"{lineno}: depth={brace_depth}, stack={class_stack}") PYAlso applies to: 129-130
🧹 Nitpick comments (2)
interface/src/routes/moonbase/filemanager/FileManager.svelte (1)
37-37: Type assertion on empty object may mask missing fields.Using
{} as FilesStatebypasses TypeScript's check for required properties. While this works becausegetState()populates the object before use, consider initializing with a proper default object (similar toeditableFileon lines 39-50) for consistency and to catch accidental early access.♻️ Optional: Initialize with default values
-let filesState = $state({} as FilesState); +let filesState: FilesState = $state({ + name: '', + path: '', + isFile: false, + size: 0, + time: 0, + contents: '', + files: [], + fs_total: 0, + fs_used: 0, + showHidden: false +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/moonbase/filemanager/FileManager.svelte` at line 37, The filesState is initialized using a type assertion "{} as FilesState", which hides missing required fields; replace that with a proper default object that matches the FilesState shape (similar to how editableFile is initialized) and pass that default into $state instead of "{} as FilesState". Locate the filesState declaration and FilesState type, create a defaultFilesState (with empty arrays/nullable fields/default primitives matching the type), use getState() as before to populate it later, and ensure any consumers of filesState still work with the concrete defaults.ci/cpp_lint.py (1)
74-83: Block comment handling is incomplete, though no instances exist in the current codebase.The
count_braces()function at lines 74–83 strips//comments but ignores/* ... */blocks. While this could theoretically cause scope-tracking errors if braces appear inside block comments, a search of the codebase found zero instances of braces within block comments. This is a robustness gap rather than an active bug, but making the stripping logic more complete would be prudent for future-proofing.The suggested refactor to track multiline block comments is reasonable if you want to make the code more defensive, though it is not urgent given the current state of the code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/cpp_lint.py` around lines 74 - 83, The count_braces function only strips // and string/char literals and ignores /* ... */ block comments, so braces inside block comments can be miscounted; update count_braces (or its caller) to handle C-style block comments by tracking a multiline in_block_comment state: either (A) pre-process the entire file to remove all /*...*/ spans before calling count_braces, or (B) change count_braces signature to accept an in_block_comment boolean and return (net_brace_delta, updated_in_block_comment), then inside the function detect and strip /*...*/ content that starts and/or ends on the current line (handling cases where the comment spans lines) before counting '{' and '}', and update callers to propagate the in_block_comment state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ci/cpp_lint.py`:
- Around line 74-83: The count_braces function only strips // and string/char
literals and ignores /* ... */ block comments, so braces inside block comments
can be miscounted; update count_braces (or its caller) to handle C-style block
comments by tracking a multiline in_block_comment state: either (A) pre-process
the entire file to remove all /*...*/ spans before calling count_braces, or (B)
change count_braces signature to accept an in_block_comment boolean and return
(net_brace_delta, updated_in_block_comment), then inside the function detect and
strip /*...*/ content that starts and/or ends on the current line (handling
cases where the comment spans lines) before counting '{' and '}', and update
callers to propagate the in_block_comment state.
In `@interface/src/routes/moonbase/filemanager/FileManager.svelte`:
- Line 37: The filesState is initialized using a type assertion "{} as
FilesState", which hides missing required fields; replace that with a proper
default object that matches the FilesState shape (similar to how editableFile is
initialized) and pass that default into $state instead of "{} as FilesState".
Locate the filesState declaration and FilesState type, create a
defaultFilesState (with empty arrays/nullable fields/default primitives matching
the type), use getState() as before to populate it later, and ensure any
consumers of filesState still work with the concrete defaults.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ci/cpp_lint.pyinterface/src/lib/components/moonbase/EditRowWidget.svelteinterface/src/lib/components/moonbase/FieldRenderer.svelteinterface/src/lib/components/moonbase/FileEditWidget.svelteinterface/src/lib/components/moonbase/RowRenderer.svelteinterface/src/lib/types/moonbase_models.tsinterface/src/routes/moonbase/filemanager/FileManager.svelteinterface/src/routes/moonbase/module/Module.svelte
- Add .cache in .gitignore - Add unit tests in CLAUDE.md - Add unit tests in .claude/settings.json - pio.ini extend env:native - add test_utilities.cpp - cpp_lint.py, extend Track class/struct openings
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/framework/WWWData.h (1)
5-5: Reduce generated binary churn by using deterministic gzip metadata.These updates look like regenerated gzip headers across embedded assets. Consider generating with fixed
mtime(and stable compression settings) so unrelated PRs don’t produce large binary-only diffs.Also applies to: 36-36, 750-750, 765-765, 772-772, 779-779, 13557-13557, 14631-14631, 16110-16110, 17173-17173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/WWWData.h` at line 5, The embedded gzip blobs in WWWData.h (the generated byte arrays at the top of the file) are non-deterministic due to gzip header metadata; regenerate these assets with deterministic gzip options (set a fixed mtime like 0, strip original filename/OS fields via --no-name or equivalent, and use consistent compression level) so the byte arrays (embedded gzip blobs) become stable across builds; update your asset generation script or tool that produces WWWData.h (the step that emits the 0x1F,0x8B,... arrays) to pass --mtime=0 and --no-name (or use a reproducible gzip library with mtime control) and re-run it for all affected embedded blobs so the diffs stop showing spurious binary churn..claude/settings.json (1)
8-8: Avoid hardcoded PlatformIO binary path in the hook.Line [8] uses
~/.platformio/penv/bin/pio, which is environment-specific and may fail for contributors/CI with different setups. Preferpio test -e native(PATH-based) for portability.Portable command adjustment
- "command": "echo '=== MoonLight lint ===' && cd \"$CLAUDE_PROJECT_DIR/interface\" && npm run lint && echo '--- backend ---' && cd \"$CLAUDE_PROJECT_DIR\" && python3 ci/cpp_lint.py && echo '--- unit tests ---' && ~/.platformio/penv/bin/pio test -e native" + "command": "echo '=== MoonLight lint ===' && cd \"$CLAUDE_PROJECT_DIR/interface\" && npm run lint && echo '--- backend ---' && cd \"$CLAUDE_PROJECT_DIR\" && python3 ci/cpp_lint.py && echo '--- unit tests ---' && pio test -e native"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/settings.json at line 8, The hook command in the JSON "command" value currently invokes the PlatformIO binary via a hardcoded path (~/.platformio/penv/bin/pio test -e native); update that "command" entry to call pio via PATH instead (replace "~/.platformio/penv/bin/pio test -e native" with "pio test -e native") so the hook is portable across environments, and ensure callers rely on a PATH-installed pio or document that pio must be in PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/cpp_lint.py`:
- Around line 120-133: The code currently creates pending_class for any "class"
header line, causing forward declarations like "class Foo;" to be treated as
pending and later misalign class_stack; update the logic around pending_class
creation (the block that sets pending_class and inspects header_line) to first
detect and skip forward declarations (e.g., header_line matching a
forward-declaration pattern or ending with ';') so you only create pending_class
and push onto class_stack when the header is a real definition (has '{' or
otherwise not a mere declaration); reference pending_class, class_stack, and
header_line when applying the guard.
In `@test/test_native/test_utilities.cpp`:
- Line 71: The lcm function can divide by zero when gcd(a, b) == 0 (e.g., lcm(0,
0)); update the uint16_t lcm(uint16_t a, uint16_t b) function to guard against
zero inputs by checking a == 0 || b == 0 and returning 0 in that case before
performing a / gcd(a, b) * b; keep using the existing gcd(a, b) helper for
non-zero cases to compute the result.
---
Nitpick comments:
In @.claude/settings.json:
- Line 8: The hook command in the JSON "command" value currently invokes the
PlatformIO binary via a hardcoded path (~/.platformio/penv/bin/pio test -e
native); update that "command" entry to call pio via PATH instead (replace
"~/.platformio/penv/bin/pio test -e native" with "pio test -e native") so the
hook is portable across environments, and ensure callers rely on a
PATH-installed pio or document that pio must be in PATH.
In `@lib/framework/WWWData.h`:
- Line 5: The embedded gzip blobs in WWWData.h (the generated byte arrays at the
top of the file) are non-deterministic due to gzip header metadata; regenerate
these assets with deterministic gzip options (set a fixed mtime like 0, strip
original filename/OS fields via --no-name or equivalent, and use consistent
compression level) so the byte arrays (embedded gzip blobs) become stable across
builds; update your asset generation script or tool that produces WWWData.h (the
step that emits the 0x1F,0x8B,... arrays) to pass --mtime=0 and --no-name (or
use a reproducible gzip library with mtime control) and re-run it for all
affected embedded blobs so the diffs stop showing spurious binary churn.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.claude/settings.json.gitignoreCLAUDE.mdci/cpp_lint.pyinterface/src/routes/moonbase/filemanager/FileManager.sveltelib/framework/WWWData.hplatformio.initest/test_native/test_utilities.cpp
🔥 Files not summarized due to errors (1)
- lib/framework/WWWData.h: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (1)
- .gitignore
- pio.ini: move from unitytest to doctest - Char: remove PureFunctions and move to PureFunctions.h - Coord3D: remove from Utilities to new class - Utiltities: remove Coord3D and PureFunctions
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/test_native/test_utilities.cpp (1)
31-35: Addlcmzero-input regression checks.Please add edge-case assertions so the zero-handling contract is permanently covered.
✅ Proposed test additions
TEST_CASE("lcm") { CHECK_EQ(lcm(12, 18), 36); CHECK_EQ(lcm(7, 13), 91); CHECK_EQ(lcm(12, 8), 24); + CHECK_EQ(lcm(0, 0), 0); + CHECK_EQ(lcm(0, 5), 0); + CHECK_EQ(lcm(5, 0), 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_native/test_utilities.cpp` around lines 31 - 35, Add assertions to cover zero-handling for the lcm function: verify that lcm(0, 0) returns 0 and that lcm(0, n) and lcm(n, 0) return 0 for several nonzero n (e.g., 1, 12, 18) in the existing TEST_CASE("lcm") so the zero-input regression is permanently covered; update the test to call lcm(0,0), lcm(0,12), lcm(12,0) (and optionally lcm(0,1), lcm(1,0)) with CHECK_EQ expecting 0.
🤖 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/MoonBase/Coord3D.h`:
- Around line 55-57: The division and modulo operators in Coord3D
(operator/(const Coord3D), operator/(const int), and operator%(const Coord3D))
can perform division/modulo by zero causing undefined behavior; update these
operators to check each divisor component (rhs.x, rhs.y, rhs.z for Coord3D and
rhs for int) before performing the operation and handle zero cases
deterministically (e.g., throw a std::invalid_argument or std::domain_error with
a clear message, or assert/return a safe value depending on project
error-handling conventions) so no division or modulo by zero occurs; ensure the
error message identifies the failing operator and the offending component to aid
debugging.
- Around line 60-77: Change the assignment-style operators in class Coord3D to
take their RHS by const reference and return a reference to *this so chaining
and copies are corrected: update operator=(const Coord3D rhs) to operator=(const
Coord3D& rhs) and return Coord3D&; update operator+=(const Coord3D rhs) to
operator+=(const Coord3D& rhs) and return Coord3D&; update operator/=(const
Coord3D rhs) to operator/=(const Coord3D& rhs) and return Coord3D&; keep the
existing member-wise operations but ensure the functions return *this as a
reference.
In `@src/MoonBase/PureFunctions.h`:
- Around line 30-39: extractPath's API is unsafe: it omits the destination
buffer size and uses nonstandard strlcpy; change extractPath(const char*
filepath, char* path) to accept a size parameter (e.g., extractPath(const char*
filepath, char* path, size_t pathSize)), compute the desired path length from
lastSlash (size_t pathLen = lastSlash - filepath + 1), then copy into path using
only standard functions with bounds checking (copy min(pathLen, pathSize-1)
bytes and always null-terminate), and handle the no-slash case by writing an
empty string when pathSize>0; update callers to pass the buffer size and
consider returning a status or the number of bytes written if helpful.
In `@test/test_native/test_utilities.cpp`:
- Around line 8-10: The file-level header comment in
test/test_native/test_utilities.cpp is stale: it claims helper functions are
copied into the file and references ESP32 header avoidance, but the tests now
include PureFunctions.h and Coord3D.h directly; update the top comment to state
that the tests consume PureFunctions.h and Coord3D.h (and mention the run
command if still relevant), and remove the outdated note about copied helpers
and ESP32 dependencies so the comment accurately reflects current usage (look
for the existing header comment at the top of test_utilities.cpp and edit it
accordingly).
---
Nitpick comments:
In `@test/test_native/test_utilities.cpp`:
- Around line 31-35: Add assertions to cover zero-handling for the lcm function:
verify that lcm(0, 0) returns 0 and that lcm(0, n) and lcm(n, 0) return 0 for
several nonzero n (e.g., 1, 12, 18) in the existing TEST_CASE("lcm") so the
zero-input regression is permanently covered; update the test to call lcm(0,0),
lcm(0,12), lcm(12,0) (and optionally lcm(0,1), lcm(1,0)) with CHECK_EQ expecting
0.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
platformio.inisrc/MoonBase/Char.hsrc/MoonBase/Coord3D.hsrc/MoonBase/PureFunctions.hsrc/MoonBase/Utilities.cppsrc/MoonBase/Utilities.hsrc/MoonLight/Nodes/Effects/E_MoonLight.htest/test_native/test_utilities.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- platformio.ini
- src/MoonLight/Nodes/Effects/E_MoonLight.h
src/MoonBase/Coord3D.h
Outdated
Comment on lines
+55
to
+57
| Coord3D operator/(const Coord3D rhs) const { return Coord3D(x / rhs.x, y / rhs.y, z / rhs.z); } | ||
| Coord3D operator/(const int rhs) const { return Coord3D(x / rhs, y / rhs, z / rhs); } | ||
| Coord3D operator%(const Coord3D rhs) const { return Coord3D(x % rhs.x, y % rhs.y, z % rhs.z); } |
There was a problem hiding this comment.
Prevent UB in divide/modulo operators.
These operators can divide/mod by zero and invoke undefined behavior.
🐛 Proposed fix
- Coord3D operator/(const Coord3D rhs) const { return Coord3D(x / rhs.x, y / rhs.y, z / rhs.z); }
- Coord3D operator/(const int rhs) const { return Coord3D(x / rhs, y / rhs, z / rhs); }
- Coord3D operator%(const Coord3D rhs) const { return Coord3D(x % rhs.x, y % rhs.y, z % rhs.z); }
+ Coord3D operator/(const Coord3D& rhs) const {
+ Coord3D out(*this);
+ out /= rhs; // keeps current zero-handling contract from operator/=
+ return out;
+ }
+ Coord3D operator/(int rhs) const {
+ if (rhs == 0) return *this;
+ return Coord3D(x / rhs, y / rhs, z / rhs);
+ }
+ Coord3D operator%(const Coord3D& rhs) const {
+ return Coord3D(
+ rhs.x ? x % rhs.x : x,
+ rhs.y ? y % rhs.y : y,
+ rhs.z ? z % rhs.z : z
+ );
+ }📝 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
| Coord3D operator/(const Coord3D rhs) const { return Coord3D(x / rhs.x, y / rhs.y, z / rhs.z); } | |
| Coord3D operator/(const int rhs) const { return Coord3D(x / rhs, y / rhs, z / rhs); } | |
| Coord3D operator%(const Coord3D rhs) const { return Coord3D(x % rhs.x, y % rhs.y, z % rhs.z); } | |
| Coord3D operator/(const Coord3D& rhs) const { | |
| Coord3D out(*this); | |
| out /= rhs; // keeps current zero-handling contract from operator/= | |
| return out; | |
| } | |
| Coord3D operator/(int rhs) const { | |
| if (rhs == 0) return *this; | |
| return Coord3D(x / rhs, y / rhs, z / rhs); | |
| } | |
| Coord3D operator%(const Coord3D& rhs) const { | |
| return Coord3D( | |
| rhs.x ? x % rhs.x : x, | |
| rhs.y ? y % rhs.y : y, | |
| rhs.z ? z % rhs.z : z | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/Coord3D.h` around lines 55 - 57, The division and modulo
operators in Coord3D (operator/(const Coord3D), operator/(const int), and
operator%(const Coord3D)) can perform division/modulo by zero causing undefined
behavior; update these operators to check each divisor component (rhs.x, rhs.y,
rhs.z for Coord3D and rhs for int) before performing the operation and handle
zero cases deterministically (e.g., throw a std::invalid_argument or
std::domain_error with a clear message, or assert/return a safe value depending
on project error-handling conventions) so no division or modulo by zero occurs;
ensure the error message identifies the failing operator and the offending
component to aid debugging.
src/MoonBase/Coord3D.h
Outdated
Comment on lines
+60
to
+77
| Coord3D operator=(const Coord3D rhs) { | ||
| x = rhs.x; | ||
| y = rhs.y; | ||
| z = rhs.z; | ||
| return *this; | ||
| } | ||
| Coord3D operator+=(const Coord3D rhs) { | ||
| x += rhs.x; | ||
| y += rhs.y; | ||
| z += rhs.z; | ||
| return *this; | ||
| } | ||
| Coord3D operator/=(const Coord3D rhs) { | ||
| if (rhs.x) x /= rhs.x; | ||
| if (rhs.y) y /= rhs.y; | ||
| if (rhs.z) z /= rhs.z; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify assignment-style operator signatures in Coord3D.
echo "=== Checking for operators returning by value ==="
rg -nP 'Coord3D\s+operator(?:=|\+=|/=)\s*\(' src/MoonBase/Coord3D.h || echo "No matches found"
echo ""
echo "=== Checking for operators returning by reference ==="
rg -nP 'Coord3D&\s+operator(?:=|\+=|/=)\s*\(' src/MoonBase/Coord3D.h || echo "No matches found"
echo ""
echo "=== Full context around line 60-77 ==="
sed -n '60,77p' src/MoonBase/Coord3D.hRepository: MoonModules/MoonLight
Length of output: 725
Return references from assignment-style operators.
All three assignment operators (operator=, operator+=, operator/=) at lines 60, 66, and 72 return by value and take parameters by value, causing unnecessary copies and breaking expected C++ chaining semantics.
♻️ Proposed fix
- Coord3D operator=(const Coord3D rhs) {
+ Coord3D& operator=(const Coord3D& rhs) {
x = rhs.x;
y = rhs.y;
z = rhs.z;
return *this;
}
- Coord3D operator+=(const Coord3D rhs) {
+ Coord3D& operator+=(const Coord3D& rhs) {
x += rhs.x;
y += rhs.y;
z += rhs.z;
return *this;
}
- Coord3D operator/=(const Coord3D rhs) {
+ Coord3D& operator/=(const Coord3D& rhs) {
if (rhs.x) x /= rhs.x;
if (rhs.y) y /= rhs.y;
if (rhs.z) z /= rhs.z;
return *this;
}📝 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
| Coord3D operator=(const Coord3D rhs) { | |
| x = rhs.x; | |
| y = rhs.y; | |
| z = rhs.z; | |
| return *this; | |
| } | |
| Coord3D operator+=(const Coord3D rhs) { | |
| x += rhs.x; | |
| y += rhs.y; | |
| z += rhs.z; | |
| return *this; | |
| } | |
| Coord3D operator/=(const Coord3D rhs) { | |
| if (rhs.x) x /= rhs.x; | |
| if (rhs.y) y /= rhs.y; | |
| if (rhs.z) z /= rhs.z; | |
| return *this; | |
| } | |
| Coord3D& operator=(const Coord3D& rhs) { | |
| x = rhs.x; | |
| y = rhs.y; | |
| z = rhs.z; | |
| return *this; | |
| } | |
| Coord3D& operator+=(const Coord3D& rhs) { | |
| x += rhs.x; | |
| y += rhs.y; | |
| z += rhs.z; | |
| return *this; | |
| } | |
| Coord3D& operator/=(const Coord3D& rhs) { | |
| if (rhs.x) x /= rhs.x; | |
| if (rhs.y) y /= rhs.y; | |
| if (rhs.z) z /= rhs.z; | |
| return *this; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/Coord3D.h` around lines 60 - 77, Change the assignment-style
operators in class Coord3D to take their RHS by const reference and return a
reference to *this so chaining and copies are corrected: update operator=(const
Coord3D rhs) to operator=(const Coord3D& rhs) and return Coord3D&; update
operator+=(const Coord3D rhs) to operator+=(const Coord3D& rhs) and return
Coord3D&; update operator/=(const Coord3D rhs) to operator/=(const Coord3D& rhs)
and return Coord3D&; keep the existing member-wise operations but ensure the
functions return *this as a reference.
Lint === Add iwyu to lint.yml Add frondend unit tests using Vitest Add moonbase_utilities.test.ts frontend ======= - Initialize with safe defaults - vite.config.ts: add rollupOptions backend ======= Move utility files to MoonBase/utilities Extract alloc functions from Utilities.h and move to MemAlloc.h Coord3D: add by reference (&)
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Collaborator
Author
|
@coderabbitai, please review da3bd4d |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4486 🏁 Script executed: Length of output: 9966 🏁 Script executed: Length of output: 4363 🏁 Script executed: Length of output: 1811
✅ Highlights1. IWYU Tool Implementation (
2. Memory Allocation Utilities (
3. Vitest Integration
4. File Reorganization
5. Enhanced Documentation
🔍 Minor Observations
🎯 Overall AssessmentThis is a well-executed refactoring that improves:
The changes maintain backward compatibility while setting up better foundations for future development. The "informational only" approach for IWYU is particularly wise—it provides value without blocking development flow. Great work! 🚀 🐰✨ 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
interface/src/lib/stores/socket.ts (1)
51-67:⚠️ Potential issue | 🟠 MajorBinary decode failure bypasses new monitor fallback path
At Line 58, the
returninsidecatchexits before Lines 63-64 run. So binary frames that fail decode are emitted as errors and never routed to'monitor', which conflicts with the new fallback intent at Line 61.Suggested fix
try { payload = binary ? msgpack.decode(new Uint8Array(payload)) : JSON.parse(payload); } catch (error) { - console.error('[WebSocket] Decode error:', error); // 🌙 - listeners.get('error')?.forEach((listener) => { - listener(error); - }); + if (binary) { + listeners.get('monitor')?.forEach((listener) => { + listener(new Uint8Array(message.data)); + }); + return; + } + console.error('[WebSocket] Decode error:', error); // 🌙 + listeners.get('error')?.forEach((listener) => { + listener(error); + }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/stores/socket.ts` around lines 51 - 67, The try/catch in the message handler currently returns on decode errors, preventing the binary-fallback monitor path from running; instead, on decode failure you should preserve the binary flag and avoid early return so that when payload is falsy or non-object the existing check (binary -> listeners.get('monitor')...) runs; modify the error handling around msgpack.decode/JSON.parse in the handler (the payload variable and binary flag in this function) to call listeners.get('error') for the decode error but do not return—allow execution to reach the subsequent non-object check and route raw Uint8Array data to the 'monitor' listeners when binary is true.src/MoonBase/utilities/Utilities.cpp (1)
114-116:⚠️ Potential issue | 🔴 CriticalFix missing
moonmanpng.hinclude to unblock CI.Line 115 is currently breaking PlatformIO CI (
moonmanpng.h: No such file or directory). Please guard or correct the include path soesp32-s3-atoms3rbuilds reliably.🔧 Proposed resilient include fix
`#if` USE_M5UNIFIED - `#include` "moonmanpng.h" + `#if` __has_include("moonmanpng.h") + `#include` "moonmanpng.h" + `#elif` __has_include("../moonmanpng.h") + `#include` "../moonmanpng.h" + `#else` + // Fallback to keep builds working when the generated asset header is absent. + unsigned char moonmanpng[] = {0}; + unsigned int moonmanpng_len = 0; + `#endif` `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/utilities/Utilities.cpp` around lines 114 - 116, The include for "moonmanpng.h" in Utilities.cpp causes CI failures when the file is absent; fix by guarding it with a compile-time presence check: replace the plain `#if` USE_M5UNIFIED / `#include` "moonmanpng.h" block with a conditional that uses defined(USE_M5UNIFIED) && __has_include("moonmanpng.h") (or an alternate known path) so the compiler only includes moonmanpng.h when USE_M5UNIFIED is enabled and the header is actually available; reference the existing symbols USE_M5UNIFIED and the header name moonmanpng.h when making the change.
🧹 Nitpick comments (4)
ci/iwyu.py (1)
44-54: Optional: Use iterable unpacking for cleaner command construction.The subprocess security warning (S603) is a false positive here since paths come from controlled local
rglob()and flags are hardcoded constants.✨ Optional style improvement using unpacking
def run_iwyu(path: Path) -> tuple[bool, str]: """Run IWYU on one file. Returns (compiled_ok, iwyu_suggestions).""" - cmd = ["include-what-you-use"] + INCLUDE_FLAGS + [str(path)] + cmd = ["include-what-you-use", *INCLUDE_FLAGS, str(path)] try: result = subprocess.run(cmd, capture_output=True, text=True, timeout=30)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/iwyu.py` around lines 44 - 54, The command list construction in run_iwyu currently uses concatenation; replace it with iterable unpacking for clarity by building cmd as ["include-what-you-use", *INCLUDE_FLAGS, str(path)] (keep use of subprocess.run, capture_output, text, timeout and the existing return behavior), referencing run_iwyu, INCLUDE_FLAGS and path to locate the code; no other logic changes needed.misc/upstream-prs/001-lint-fixes.md (1)
125-130: LGTM! Clear and consistent Vitest documentation.The Vitest documentation follows the established pattern and correctly scopes testing to MoonLight-specific files, which aligns with the project's approach of minimizing changes to upstream code. The run commands are clearly documented alongside Prettier and ESLint.
Optional suggestion: The specific test count on Line 129 ("9 tests") may become stale as tests are added or removed. Consider either:
- Removing the count and keeping just the scope: "Current test coverage:
moonbase_utilities.test.tscoversinitCapandgetTimeAgo."- Or accepting that this will need periodic updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@misc/upstream-prs/001-lint-fixes.md` around lines 125 - 130, The documentation line in misc/upstream-prs/001-lint-fixes.md currently hard-codes "9 tests" which will become stale; update the sentence in the block referencing moonbase_utilities.test.ts to remove the numeric test count and instead state that moonbase_utilities.test.ts covers the functions initCap and getTimeAgo (or alternately make the count dynamic), so edit the line containing "Current test coverage: `moonbase_utilities.test.ts` — 9 tests for `initCap` and `getTimeAgo`" to a non-count form like "Current test coverage: `moonbase_utilities.test.ts` covers `initCap` and `getTimeAgo`."interface/src/routes/wifi/sta/EditNetwork.svelte (1)
37-46: Deduplicate the defaultKnownNetworkItemobject to avoid drift.The same default literal is declared twice (prop fallback and
$stateinit), which is easy to desync later.♻️ Proposed refactor
+const EMPTY_NETWORK_EDITABLE: KnownNetworkItem = { + ssid: '', + password: '', + static_ip_config: false, + local_ip: undefined, + subnet_mask: undefined, + gateway_ip: undefined, + dns_ip_1: undefined, + dns_ip_2: undefined +}; let { isOpen, title, - networkEditable: _networkEditable = { - ssid: '', - password: '', - static_ip_config: false, - local_ip: undefined, - subnet_mask: undefined, - gateway_ip: undefined, - dns_ip_1: undefined, - dns_ip_2: undefined - } as KnownNetworkItem, + networkEditable: _networkEditable = EMPTY_NETWORK_EDITABLE, onSaveNetwork }: Props = $props(); -let networkEditable: KnownNetworkItem = $state({ - ssid: '', - password: '', - static_ip_config: false, - local_ip: undefined, - subnet_mask: undefined, - gateway_ip: undefined, - dns_ip_1: undefined, - dns_ip_2: undefined -}); +let networkEditable: KnownNetworkItem = $state({ ...EMPTY_NETWORK_EDITABLE });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/wifi/sta/EditNetwork.svelte` around lines 37 - 46, Duplicate default KnownNetworkItem literal is declared twice (used as the prop fallback and in the networkEditable $state init); extract that default into a single constant (e.g., DEFAULT_KNOWN_NETWORK or defaultKnownNetwork) and use that constant both where the prop fallback is defined and when initializing networkEditable so the default shape is maintained in one place; update references to KnownNetworkItem, networkEditable, and any prop that used the inline literal to consume the new constant.src/MoonBase/utilities/Coord3D.h (1)
66-83: Return references from mutating operators.
operator=,operator+=, andoperator/=currently returnCoord3Dby value. ReturningCoord3D&avoids unnecessary copies and matches standard C++ operator semantics.♻️ Proposed signature fix
- Coord3D operator=(const Coord3D& rhs) { + Coord3D& operator=(const Coord3D& rhs) { x = rhs.x; y = rhs.y; z = rhs.z; return *this; } - Coord3D operator+=(const Coord3D& rhs) { + Coord3D& operator+=(const Coord3D& rhs) { x += rhs.x; y += rhs.y; z += rhs.z; return *this; } - Coord3D operator/=(const Coord3D& rhs) { + Coord3D& operator/=(const Coord3D& rhs) { if (rhs.x) x /= rhs.x; if (rhs.y) y /= rhs.y; if (rhs.z) z /= rhs.z; return *this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/utilities/Coord3D.h` around lines 66 - 83, The mutating operators operator=, operator+=, and operator/= in Coord3D should return Coord3D& instead of Coord3D to avoid unnecessary copies and follow C++ conventions; update the signatures of Coord3D::operator=(const Coord3D&), Coord3D::operator+=(const Coord3D&), and Coord3D::operator/=(const Coord3D&) to return Coord3D&, ensure each returns *this by reference, and (optionally) add a self-assignment guard in operator= if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/lib/stores/socket.ts`:
- Around line 61-67: The change modifies upstream frontend file socket.ts
(inside the WebSocket message handler) which violates the frontend allowlist;
revert the edit in the handler that inspects payload/binary and sending monitor
data (the block referencing payload, binary, listeners.get('monitor'),
message.data and Uint8Array) and instead implement the desired behavior in an
allowed location (e.g., export a helper from
src/lib/stores/moonbase_utilities.ts or a component under
src/lib/components/moonbase/) that accepts the raw message and performs the
binary check and dispatch to listeners, then call that helper from the approved
frontend file; ensure the original socket.ts handler is restored to its prior
state and the new logic references the same symbols (listeners, monitor,
payload, binary, message.data) but lives only in an allowed path.
- Line 64: Convert all expression-bodied forEach callbacks in this file (the
calls on the Map named listeners such as listeners.get('monitor')?.forEach(...)
and the similar calls at the other locations) to use block-statement callbacks
so they don't implicitly return a value; for each occurrence (lines with
listeners.get('...')?.forEach((listener) => ...)) replace the arrow expression
form with a block body: (listener) => { listener(...); } ensuring the argument
construction (e.g., new Uint8Array(message.data) or whatever the original
expression was) is executed inside the block and no value is returned. Apply
this change consistently to the four instances referenced (the callbacks that
invoke listener) and keep the surrounding logic and variable names (listeners,
listener, message) intact.
In `@interface/src/routes/ethernet/Ethernet.svelte`:
- Around line 50-62: You modified upstream frontend route Ethernet.svelte
(symbols: ethernetEditable, strEthernetEditable, getEthernetSettings); move
these edits out of interface/src/routes/ethernet/Ethernet.svelte into an
approved MoonLight path or submit them upstream: either refactor the state
initialization and string-state logic into an allowed file (e.g., a new module
under src/lib/components/moonbase/ or src/lib/stores/moonbase_utilities.ts) and
import it into the route, or revert this file and open a PR against the upstream
repo with the intended changes; ensure references to ethernetEditable,
strEthernetEditable and getEthernetSettings are updated to use the new module or
the upstream change.
- Around line 61-64: The dirty flag is true by default because
strEthernetEditable is initialized to '' and compared to a JSON snapshot; change
initialization so strEthernetEditable is set to the serialized initial snapshot
instead of '', or assign/update strEthernetEditable after getEthernetSettings()
finishes loading (e.g., set strEthernetEditable =
JSON.stringify(ethernetEditable, emptyStringToUndefinedReplacer) once settings
are loaded) so the comparison in isSettingsDirty (which uses ethernetEditable
and strEthernetEditable) only becomes true after a real user change.
In `@interface/src/routes/user/EditUser.svelte`:
- Around line 31-41: You modified upstream component EditUser.svelte (variables
user, errorUsername, usernameEditable and the $effect.pre initializer) under
interface/src/routes/user/, which is not allowed; instead create a
MoonLight-specific variant or wrapper in an allowed path (e.g.,
src/routes/moonbase/ or src/lib/components/moonbase/) and move the customization
there: implement the same state initialization and the $effect.pre hook inside a
new Moonbase EditUser wrapper component (or expose the behavior from
src/lib/stores/moonbase_utilities.ts and import it), then have the upstream
EditUser import/use the Moonbase wrapper only when running MoonLight (or
register the Moonbase route) so no direct edits remain in
interface/src/routes/user/EditUser.svelte.
In `@interface/src/routes/wifi/sta/EditNetwork.svelte`:
- Around line 36-55: The change modifies an upstream frontend file that is
off-limits; move the edits out of
interface/src/routes/wifi/sta/EditNetwork.svelte by reverting this file and
applying equivalent updates in an approved MoonLight-specific location (e.g.,
under src/routes/moonbase/ or src/lib/components/moonbase/) or create a
documented exception; specifically, restore the original networkEditable and
staticIPConfig initialization and $effect.pre usage in EditNetwork.svelte, then
implement the intended behavior (initializing KnownNetworkItem, the $state
wrapper, and staticIPConfig assignment) in a MoonLight-managed component or
utility and wire it into the app, or add a formal exception note in the repo
policy before merging.
- Around line 51-55: The inline comment claiming "$effect.pre" is a Svelte 5.36+
feature is incorrect; update the comment above the $effect.pre block (where
networkEditable is set from _networkEditable and staticIPConfig from
_networkEditable.static_ip_config) to either remove the version claim or reword
it to say "$effect.pre is available in Svelte 5 (used here to load from prop
before first render)", leaving the code ($effect.pre(() => { networkEditable = {
..._networkEditable }; staticIPConfig = _networkEditable.static_ip_config; }))
unchanged.
In `@platformio.ini`:
- Around line 315-331: The native environment section [env:native] is missing a
test_framework declaration and includes platform-specific libdeps paths; add
test_framework = doctest under [env:native] and remove the hardcoded esp32-s3
include paths (-I.pio/libdeps/esp32-s3/FastLED/src and
-I.pio/libdeps/esp32-s3/FastLED/tests) from build_flags so the native config is
platform-independent; ensure framework, extra_scripts, lib_deps remain empty as
intended and that any test includes rely on the declared test_framework not on
esp32-specific libdeps.
In `@src/MoonBase/utilities/MemAlloc.h`:
- Around line 104-118: freeMBObject currently frees placement-new memory without
running destructors; change freeMBObject(T*& obj) to check obj for nullptr,
explicitly call the destructor (obj->~T()), then call freeMB(obj, "object") and
set obj to nullptr so non-trivial types are torn down correctly; this keeps
allocMBObject and freeMB usage but ensures placement-new objects are properly
destroyed before raw memory is released.
- Line 31: reallocMB() and reallocMB2() currently perform reallocations without
updating the global counter totalAllocatedMB, causing accounting drift; change
both functions to compute the old allocation size in MB (e.g., oldSizeMB) before
calling realloc, call realloc, and only on successful reallocation compute
newSizeMB and apply totalAllocatedMB += (newSizeMB - oldSizeMB) so the counter
increases or decreases by the size delta (mirror the behaviour of
allocMB/freeMB); ensure the adjustment happens after verifying the returned
pointer is non-null and use the same MB rounding logic as allocMB/freeMB.
- Around line 14-16: The header is missing direct includes for APIs it uses; add
`#include` <utility> to provide std::forward used in the template forwarding
functions and add `#include` <esp_heap_caps.h> to provide heap_caps_calloc_prefer,
heap_caps_realloc_prefer, heap_caps_get_allocated_size, heap_caps_free and the
MALLOC_CAP_SPIRAM / MALLOC_CAP_DEFAULT macros so MemAlloc.h does not rely on
include order; update the top of MemAlloc.h to include these two headers so
functions like the template that uses std::forward and the memory functions
compile reliably.
- Around line 37-39: reallocMB and reallocMB2 perform "n * sizeof(T)" without
guarding against integer overflow; before calling heap_caps_realloc_prefer or
similar, compute the byte count safely (e.g., size_t bytes = n * sizeof(T)) and
validate it does not wrap by checking n <= SIZE_MAX / sizeof(T) (handle n==0
appropriately), then use that validated byte count for the allocation; follow
the pattern used in allocMB (separate count/elem size) or explicitly pass the
checked byte size to avoid truncated allocations and potential buffer overruns.
In `@test/test_native/test_utilities.cpp`:
- Line 14: The native test environment is missing the doctest framework setting
required for tests that include doctest.h and use doctest macros; open
platformio.ini and in the [env:native] section (after platform = native) add the
directive test_framework = doctest so the native environment uses the doctest
framework for tests referenced by test/test_native/test_utilities.cpp and any
code calling doctest::Approx() or other doctest symbols.
---
Outside diff comments:
In `@interface/src/lib/stores/socket.ts`:
- Around line 51-67: The try/catch in the message handler currently returns on
decode errors, preventing the binary-fallback monitor path from running;
instead, on decode failure you should preserve the binary flag and avoid early
return so that when payload is falsy or non-object the existing check (binary ->
listeners.get('monitor')...) runs; modify the error handling around
msgpack.decode/JSON.parse in the handler (the payload variable and binary flag
in this function) to call listeners.get('error') for the decode error but do not
return—allow execution to reach the subsequent non-object check and route raw
Uint8Array data to the 'monitor' listeners when binary is true.
In `@src/MoonBase/utilities/Utilities.cpp`:
- Around line 114-116: The include for "moonmanpng.h" in Utilities.cpp causes CI
failures when the file is absent; fix by guarding it with a compile-time
presence check: replace the plain `#if` USE_M5UNIFIED / `#include` "moonmanpng.h"
block with a conditional that uses defined(USE_M5UNIFIED) &&
__has_include("moonmanpng.h") (or an alternate known path) so the compiler only
includes moonmanpng.h when USE_M5UNIFIED is enabled and the header is actually
available; reference the existing symbols USE_M5UNIFIED and the header name
moonmanpng.h when making the change.
---
Nitpick comments:
In `@ci/iwyu.py`:
- Around line 44-54: The command list construction in run_iwyu currently uses
concatenation; replace it with iterable unpacking for clarity by building cmd as
["include-what-you-use", *INCLUDE_FLAGS, str(path)] (keep use of subprocess.run,
capture_output, text, timeout and the existing return behavior), referencing
run_iwyu, INCLUDE_FLAGS and path to locate the code; no other logic changes
needed.
In `@interface/src/routes/wifi/sta/EditNetwork.svelte`:
- Around line 37-46: Duplicate default KnownNetworkItem literal is declared
twice (used as the prop fallback and in the networkEditable $state init);
extract that default into a single constant (e.g., DEFAULT_KNOWN_NETWORK or
defaultKnownNetwork) and use that constant both where the prop fallback is
defined and when initializing networkEditable so the default shape is maintained
in one place; update references to KnownNetworkItem, networkEditable, and any
prop that used the inline literal to consume the new constant.
In `@misc/upstream-prs/001-lint-fixes.md`:
- Around line 125-130: The documentation line in
misc/upstream-prs/001-lint-fixes.md currently hard-codes "9 tests" which will
become stale; update the sentence in the block referencing
moonbase_utilities.test.ts to remove the numeric test count and instead state
that moonbase_utilities.test.ts covers the functions initCap and getTimeAgo (or
alternately make the count dynamic), so edit the line containing "Current test
coverage: `moonbase_utilities.test.ts` — 9 tests for `initCap` and `getTimeAgo`"
to a non-count form like "Current test coverage: `moonbase_utilities.test.ts`
covers `initCap` and `getTimeAgo`."
In `@src/MoonBase/utilities/Coord3D.h`:
- Around line 66-83: The mutating operators operator=, operator+=, and
operator/= in Coord3D should return Coord3D& instead of Coord3D to avoid
unnecessary copies and follow C++ conventions; update the signatures of
Coord3D::operator=(const Coord3D&), Coord3D::operator+=(const Coord3D&), and
Coord3D::operator/=(const Coord3D&) to return Coord3D&, ensure each returns
*this by reference, and (optionally) add a self-assignment guard in operator= if
desired.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
interface/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.claude/settings.json.github/workflows/lint.ymlCLAUDE.mdci/iwyu.pyinterface/package.jsoninterface/src/lib/components/moonbase/EditRowWidget.svelteinterface/src/lib/components/moonbase/FileEditWidget.svelteinterface/src/lib/components/moonbase/RowRenderer.svelteinterface/src/lib/stores/moonbase_utilities.test.tsinterface/src/lib/stores/socket.tsinterface/src/routes/ethernet/Ethernet.svelteinterface/src/routes/user/EditUser.svelteinterface/src/routes/wifi/sta/EditNetwork.svelteinterface/vite.config.tslib/framework/WWWData.hmisc/upstream-prs/001-lint-fixes.mdplatformio.inisrc/MoonBase/Module.hsrc/MoonBase/Modules/FileManager.cppsrc/MoonBase/Modules/ModuleDevices.hsrc/MoonBase/utilities/Char.hsrc/MoonBase/utilities/Coord3D.hsrc/MoonBase/utilities/MemAlloc.hsrc/MoonBase/utilities/PureFunctions.hsrc/MoonBase/utilities/Utilities.cppsrc/MoonBase/utilities/Utilities.hsrc/MoonBase/utilities/pal.hsrc/MoonBase/utilities/pal_espidf.cppsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/PhysicalLayer.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Drivers/parlio.cppsrc/MoonLight/Nodes/Effects/E_MoonLight.htest/test_native/test_utilities.cpp
💤 Files with no reviewable changes (1)
- interface/vite.config.ts
✅ Files skipped from review due to trivial changes (3)
- src/MoonBase/Module.h
- src/MoonLight/Modules/ModuleLightsControl.h
- src/MoonLight/Layers/PhysicalLayer.h
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/lint.yml
- src/MoonBase/Modules/FileManager.cpp
- interface/src/lib/components/moonbase/FileEditWidget.svelte
- src/MoonLight/Nodes/Effects/E_MoonLight.h
interface/src/lib/stores/socket.ts
Outdated
| if (!payload || typeof payload !== 'object') { | ||
| console.error('[WebSocket] Invalid payload:', payload); | ||
| if (binary) { | ||
| listeners.get('monitor')?.forEach((listener) => listener(new Uint8Array(message.data))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read content around line 64
if [ -f "interface/src/lib/stores/socket.ts" ]; then
echo "=== File found ==="
wc -l interface/src/lib/stores/socket.ts
echo -e "\n=== Content around line 64 ==="
sed -n '55,75p' interface/src/lib/stores/socket.ts | cat -n
else
echo "File not found"
fiRepository: MoonModules/MoonLight
Length of output: 875
🏁 Script executed:
# Search for forEach patterns in the file to understand the context
echo "=== All forEach patterns in socket.ts ==="
rg -n "\.forEach\(" interface/src/lib/stores/socket.ts -A 1Repository: MoonModules/MoonLight
Length of output: 917
🏁 Script executed:
# Check if socket.ts is actually modified in this PR or if it's upstream
echo "=== Git status of socket.ts ==="
git status interface/src/lib/stores/socket.ts 2>/dev/null || echo "Not in git or file status unavailable"
echo -e "\n=== Check file history ==="
git log --oneline -5 interface/src/lib/stores/socket.ts 2>/dev/null | head -5Repository: MoonModules/MoonLight
Length of output: 262
🏁 Script executed:
# Check for Biome configuration to understand the lint rule scope
echo "=== Looking for Biome configuration ==="
find . -name "biome.json*" -o -name ".biomeignore" 2>/dev/null | head -10
# Check current Biome configuration
if [ -f "biome.json" ]; then
echo -e "\n=== biome.json content ==="
cat biome.json
fiRepository: MoonModules/MoonLight
Length of output: 1920
🏁 Script executed:
# Check if socket.ts is marked with upstream markers
echo "=== Checking for upstream markers in socket.ts ==="
rg -n "🌙|💫" interface/src/lib/stores/socket.ts
# Also check the specific lines with expression-bodied forEach
echo -e "\n=== All forEach expression-body instances ==="
rg -n "\.forEach\(\(\w+\)\s*=>\s*\w+\(" interface/src/lib/stores/socket.tsRepository: MoonModules/MoonLight
Length of output: 732
🏁 Script executed:
# Final check: verify if socket.ts was in original repo or is a new MoonBase addition
echo "=== Check git history depth ==="
git log --oneline interface/src/lib/stores/socket.ts | wc -l
echo -e "\n=== First commit mentioning socket.ts ==="
git log --oneline --reverse interface/src/lib/stores/socket.ts | head -1Repository: MoonModules/MoonLight
Length of output: 213
Fix all forEach callbacks with expression bodies to use block statements
Line 64 uses an expression-bodied callback that returns a value, which triggers the Biome lint error (useIterableCallbackReturn). However, this same pattern appears on lines 27, 38, and 49 and should be fixed consistently across the file.
Additionally, per coding guidelines, interface/src/lib/stores/socket.ts is not in the approved list of exceptions for upstream file modifications (allowed: src/routes/moonbase/, src/lib/components/moonbase/, src/lib/stores/moonbase_utilities.ts, src/lib/types/moonbase_models.ts, vite-plugin-littlefs.ts). Consider whether this file should be relocated or consolidated with MoonBase utilities.
Suggested fix for all instances
Lines 27, 38, 49, 64 should all be converted from:
-listeners.get('key')?.forEach((listener) => listener(value));
+listeners.get('key')?.forEach((listener) => {
+ listener(value);
+});📝 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
| listeners.get('monitor')?.forEach((listener) => listener(new Uint8Array(message.data))); | |
| listeners.get('monitor')?.forEach((listener) => { | |
| listener(new Uint8Array(message.data)); | |
| }); |
🧰 Tools
🪛 Biome (2.4.4)
[error] 64-64: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/lib/stores/socket.ts` at line 64, Convert all expression-bodied
forEach callbacks in this file (the calls on the Map named listeners such as
listeners.get('monitor')?.forEach(...) and the similar calls at the other
locations) to use block-statement callbacks so they don't implicitly return a
value; for each occurrence (lines with listeners.get('...')?.forEach((listener)
=> ...)) replace the arrow expression form with a block body: (listener) => {
listener(...); } ensuring the argument construction (e.g., new
Uint8Array(message.data) or whatever the original expression was) is executed
inside the block and no value is returned. Apply this change consistently to the
four instances referenced (the callbacks that invoke listener) and keep the
surrounding logic and variable names (listeners, listener, message) intact.
src/MoonBase/utilities/MemAlloc.h
Outdated
Comment on lines
+14
to
+16
| #include "ArduinoJson.h" | ||
| #include <cstddef> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading MemAlloc.h includes and initial lines ==="
head -20 src/MoonBase/utilities/MemAlloc.h
echo -e "\n=== Checking for std::forward usage ==="
rg -n 'std::forward' src/MoonBase/utilities/MemAlloc.h || echo "Not found"
echo -e "\n=== Checking for heap_caps_* and MALLOC_CAP_* usage ==="
rg -n 'heap_caps_|MALLOC_CAP_' src/MoonBase/utilities/MemAlloc.h || echo "Not found"
echo -e "\n=== Full file includes section ==="
head -30 src/MoonBase/utilities/MemAlloc.hRepository: MoonModules/MoonLight
Length of output: 3522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Build metadata search ==="
fd -HI 'platformio\.ini|CMakeLists\.txt|idf_component\.yml|library\.json|library\.properties' . | while read -r f; do
if rg -q 'ArduinoJson|esp_heap_caps|heap_caps' "$f" 2>/dev/null; then
echo "--- Found in: $f ---"
rg -n -C2 'ArduinoJson|esp_heap_caps|heap_caps' "$f"
fi
done
echo -e "\n=== Check for any existing includes of esp_heap_caps or utility in codebase ==="
rg -l 'include.*esp_heap_caps|include.*utility' src/ | head -10Repository: MoonModules/MoonLight
Length of output: 572
Add explicit headers for APIs used in this header.
Line 108 uses std::forward, and lines 38, 40, 49, 59, 73, 75 use heap_caps_calloc_prefer, heap_caps_realloc_prefer, heap_caps_get_allocated_size, and heap_caps_free with MALLOC_CAP_SPIRAM and MALLOC_CAP_DEFAULT macros, but <utility> and <esp_heap_caps.h> are not explicitly included. This makes the header brittle to include order and violates C++ header best practices.
🔧 Suggested include fix
`#include` "ArduinoJson.h"
`#include` <cstddef>
+#include <esp_heap_caps.h>
+#include <utility>📝 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
| #include "ArduinoJson.h" | |
| #include <cstddef> | |
| `#include` "ArduinoJson.h" | |
| `#include` <cstddef> | |
| `#include` <esp_heap_caps.h> | |
| `#include` <utility> |
🧰 Tools
🪛 Clang (14.0.6)
[error] 14-14: 'ArduinoJson.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/utilities/MemAlloc.h` around lines 14 - 16, The header is
missing direct includes for APIs it uses; add `#include` <utility> to provide
std::forward used in the template forwarding functions and add `#include`
<esp_heap_caps.h> to provide heap_caps_calloc_prefer, heap_caps_realloc_prefer,
heap_caps_get_allocated_size, heap_caps_free and the MALLOC_CAP_SPIRAM /
MALLOC_CAP_DEFAULT macros so MemAlloc.h does not rely on include order; update
the top of MemAlloc.h to include these two headers so functions like the
template that uses std::forward and the memory functions compile reliably.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
interface/src/lib/stores/socket.ts (1)
54-75:⚠️ Potential issue | 🟠 MajorUpstream frontend allowlist is still being violated in this file
These edits are in
interface/src/lib/stores/socket.ts, which is outside the permitted MoonLight frontend exception paths. Please move this behavior to an allowed MoonLight path and keep upstream file modifications minimal.As per coding guidelines "
**/interface/src/**/*.{ts,tsx,svelte,js}: Minimize changes to upstream files in the frontend — avoid modifying files in/interface/src/except forsrc/routes/moonbase/,src/lib/components/moonbase/,src/lib/stores/moonbase_utilities.ts,src/lib/types/moonbase_models.ts, andvite-plugin-littlefs.ts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/stores/socket.ts` around lines 54 - 75, Revert the upstream edits in the socket handler so you no longer add special-case monitor/binary routing and extra console.error calls directly in the socket file; instead create a helper in the allowed MoonLight area (e.g., a new function like handleRawMonitorData or routeMonitorMessage) that encapsulates the logic checking binary, converting to new Uint8Array(message.data), and emitting to listeners.get('monitor') or listeners.get('error'), and then call that helper from the socket code where the payload decode/error and the check (!payload || typeof payload !== 'object') happen; keep the socket file minimal (emit listeners.get('error') on decode failure using the existing error object and only delegate binary/raw monitor routing to the new MoonLight helper so the upstream file changes are minimal.src/MoonBase/utilities/MemAlloc.h (3)
14-17:⚠️ Potential issue | 🟠 MajorAdd direct includes for APIs used in this header
Line 39, Line 50, and Line 60 use
heap_caps_*/MALLOC_CAP_*, and Line 109 usesstd::forward, but the header does not directly include their defining headers. This keepsMemAlloc.hinclude-order fragile.Suggested fix
`#include` <cstddef> +#include <esp_heap_caps.h> +#include <utility> `#include` "ArduinoJson.h"#!/bin/bash set -euo pipefail echo "== API usage in MemAlloc.h ==" rg -n 'std::forward|heap_caps_|MALLOC_CAP_' src/MoonBase/utilities/MemAlloc.h echo echo "== Direct includes currently present ==" rg -n '^#include\s*<(utility|esp_heap_caps\.h)>' src/MoonBase/utilities/MemAlloc.h || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/utilities/MemAlloc.h` around lines 14 - 17, MemAlloc.h uses heap_caps_* functions and MALLOC_CAP_* macros and std::forward but doesn't include their defining headers; update the header to directly include the ESP heap caps API and the C++ utility header by adding `#include` <esp_heap_caps.h> (or the appropriate ESP-IDF header that defines heap_caps_* and MALLOC_CAP_*) and `#include` <utility> so symbols like heap_caps_malloc, MALLOC_CAP_* and std::forward are available without relying on include order; ensure these includes are placed near the top of MemAlloc.h alongside the existing standard includes to avoid fragile include-order dependencies.
50-60:⚠️ Potential issue | 🔴 CriticalGuard
n * sizeof(T)overflow beforeheap_caps_realloc_preferLine 50 and Line 60 multiply
n * sizeof(T)without a bounds check. If it wraps, allocation can be smaller than expected and callers may overrun the buffer.Suggested fix
template <typename T> T* reallocMB(T* p, size_t n, const char* name = nullptr) { + if (n > SIZE_MAX / sizeof(T)) { + EXT_LOGE(MB_TAG, "realloc overflow for %s", name ? name : "x"); + return nullptr; + } + const size_t bytes = n * sizeof(T); - T* res = (T*)heap_caps_realloc_prefer(p, n * sizeof(T), 2, MALLOC_CAP_SPIRAM, MALLOC_CAP_DEFAULT); // calloc is malloc + memset(0); + T* res = (T*)heap_caps_realloc_prefer(p, bytes, 2, MALLOC_CAP_SPIRAM, MALLOC_CAP_DEFAULT); ... } template <typename T> void reallocMB2(T*& p, size_t& pSize, size_t n, const char* name = nullptr) { + if (n > SIZE_MAX / sizeof(T)) { + EXT_LOGE(MB_TAG, "realloc overflow for %s, keeping old %zu", name ? name : "x", pSize); + return; + } + const size_t bytes = n * sizeof(T); - T* res = (T*)heap_caps_realloc_prefer(p, n * sizeof(T), 2, MALLOC_CAP_SPIRAM, MALLOC_CAP_DEFAULT); // calloc is malloc + memset(0); + T* res = (T*)heap_caps_realloc_prefer(p, bytes, 2, MALLOC_CAP_SPIRAM, MALLOC_CAP_DEFAULT); ... }#!/bin/bash set -euo pipefail echo "== Realloc byte computations ==" rg -n 'reallocMB|reallocMB2|n \* sizeof\(T\)' src/MoonBase/utilities/MemAlloc.h🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/utilities/MemAlloc.h` around lines 50 - 60, The multiplication n * sizeof(T) in reallocMB and reallocMB2 can overflow; before calling heap_caps_realloc_prefer compute a safe byte_count and guard it with if (sizeof(T) != 0 && n > SIZE_MAX / sizeof(T)) (or equivalently compare n to SIZE_MAX/sizeof(T)), log/handle the error (e.g., EXT_LOGE and return nullptr) and only call heap_caps_realloc_prefer with the verified byte_count; update any pSize bookkeeping in reallocMB2 after a successful realloc. Ensure these checks are added to both reallocMB and reallocMB2 and reference the exact expression "n * sizeof(T)" in your change.
49-65:⚠️ Potential issue | 🟠 MajorFix
totalAllocatedMBdrift on successful reallocationLine 41 and Line 74 maintain
totalAllocatedMBfor alloc/free, but Line 49–56 and Line 59–68 reallocate without applying size deltas. The counter will drift as buffers grow/shrink.Suggested fix
template <typename T> T* reallocMB(T* p, size_t n, const char* name = nullptr) { + const int oldSize = p ? static_cast<int>(heap_caps_get_allocated_size(p)) : 0; T* res = (T*)heap_caps_realloc_prefer(p, n * sizeof(T), 2, MALLOC_CAP_SPIRAM, MALLOC_CAP_DEFAULT); // calloc is malloc + memset(0); if (res) { + const int newSize = static_cast<int>(heap_caps_get_allocated_size(res)); + totalAllocatedMB += (newSize - oldSize); // EXT_LOGD(...) } else EXT_LOGE(MB_TAG, "heap_caps_malloc for %s of %d x %d not succeeded", name ? name : "x", n, sizeof(T)); return res; } template <typename T> void reallocMB2(T*& p, size_t& pSize, size_t n, const char* name = nullptr) { + const int oldSize = p ? static_cast<int>(heap_caps_get_allocated_size(p)) : 0; T* res = (T*)heap_caps_realloc_prefer(p, n * sizeof(T), 2, MALLOC_CAP_SPIRAM, MALLOC_CAP_DEFAULT); // calloc is malloc + memset(0); if (res) { + const int newSize = static_cast<int>(heap_caps_get_allocated_size(res)); + totalAllocatedMB += (newSize - oldSize); // EXT_LOGD(...) p = res; pSize = n;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/utilities/MemAlloc.h` around lines 49 - 65, The realloc functions (reallocMB and reallocMB2) currently call heap_caps_realloc_prefer without adjusting the global totalAllocatedMB, causing drift when buffers change size; fix by computing the old allocated byte size (for reallocMB call heap_caps_get_allocated_size(p) if p!=nullptr, for reallocMB2 use pSize*sizeof(T) or heap_caps_get_allocated_size(p) to be safe), perform the realloc, and on success update totalAllocatedMB by adding (newSizeBytes - oldSizeBytes) (newSizeBytes = n * sizeof(T)); ensure p and pSize are updated in reallocMB2 as now, and do not change totalAllocatedMB when realloc fails.platformio.ini (1)
330-331:⚠️ Potential issue | 🟠 MajorRemove board-specific FastLED include paths from
[env:native].Line 330 and Line 331 hardcode
esp32-s3libdeps in the native environment, which breaks portability on clean/native-only runners.🔧 Proposed adjustment
build_flags = -std=c++17 -Isrc/MoonBase -Isrc/MoonBase/utilities - -I.pio/libdeps/esp32-s3/FastLED/src - -I.pio/libdeps/esp32-s3/FastLED/tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 330 - 331, The platformio.ini's [env:native] contains hardcoded board-specific FastLED include paths (-I.pio/libdeps/esp32-s3/FastLED/src and -I.pio/libdeps/esp32-s3/FastLED/tests) which break portability; remove these two include entries from the native environment (or move them under the esp32-s3 env or replace with an environment-agnostic lib_deps reference) so [env:native] no longer references esp32-s3 libdeps.
🧹 Nitpick comments (2)
interface/src/lib/stores/socket.ts (1)
69-75: Deduplicate monitor-frame emission logic
new Uint8Array(message.data)+listeners.get('monitor')is now repeated in multiple branches. Extracting a tiny helper will reduce drift and keep behavior consistent.♻️ Proposed refactor
ws.onmessage = (message) => { resetUnresponsiveCheck(); let payload = message.data; + const emitMonitor = () => { + listeners.get('monitor')?.forEach((listener) => { + listener(new Uint8Array(message.data)); + }); + }; const binary = payload instanceof ArrayBuffer; listeners.get(binary ? 'binary' : 'message')?.forEach((listener) => listener(payload)); try { payload = binary ? msgpack.decode(new Uint8Array(payload)) : JSON.parse(payload); } catch (error) { if (binary) { - listeners.get('monitor')?.forEach((listener) => { - listener(new Uint8Array(message.data)); - }); + emitMonitor(); return; } console.error('[WebSocket] Decode error:', error); // 🌙 listeners.get('error')?.forEach((listener) => { listener(error); }); return; } // 🌙 Non-object binary = raw monitor data (not msgpack); route directly and skip destructuring if (!payload || typeof payload !== 'object') { if (binary) { - listeners.get('monitor')?.forEach((listener) => { - listener(new Uint8Array(message.data)); - }); + emitMonitor(); } else { console.error('[WebSocket] Invalid payload:', payload); } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/stores/socket.ts` around lines 69 - 75, The monitor-frame emission (listeners.get('monitor') and new Uint8Array(message.data)) is duplicated; create a small helper function (e.g., emitMonitorFrame or emitMonitorPayload) that accepts the raw data or payload, constructs the Uint8Array from message.data (or uses the provided buffer) and calls listeners.get('monitor')?.forEach(listener => listener(...)). Replace both branches so they call this helper instead of repeating new Uint8Array(message.data) and the listeners.get('monitor') logic, ensuring identical behavior in all paths.src/MoonBase/utilities/Coord3D.h (1)
14-20: ReplaceMIN/MAXmacros withstd::min/std::maxin this header.These macros leak into the global namespace and create collision risks—CoreDump.cpp already defines its own
MINmacro with different semantics, demonstrating the maintenance burden. The macro implementations also risk double-evaluation bugs.Proposed refactor
+#include <algorithm> @@ -#ifndef MIN - `#define` MIN(a, b) (((a) < (b)) ? (a) : (b)) -#endif - -#ifndef MAX - `#define` MAX(a, b) (((a) > (b)) ? (a) : (b)) -#endif @@ - Coord3D maximum(const Coord3D& rhs) const { return Coord3D(MAX(x, rhs.x), MAX(y, rhs.y), MAX(z, rhs.z)); } + Coord3D maximum(const Coord3D& rhs) const { return Coord3D(std::max(x, rhs.x), std::max(y, rhs.y), std::max(z, rhs.z)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/utilities/Coord3D.h` around lines 14 - 20, The header defines global MIN and MAX macros that should be removed; delete the MIN/MAX macro definitions in Coord3D.h, add `#include` <algorithm>, and update any code in this header (and callers if needed) to use std::min and std::max (fully qualified) instead of MIN(...) / MAX(...) to avoid macro collisions and double-evaluation; search for occurrences of MIN and MAX in functions or inline code in Coord3D.h and replace them with std::min/std::max (or std::clamp where appropriate) and ensure <algorithm> is included so the symbols resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/iwyu.py`:
- Around line 53-54: The except subprocess.TimeoutExpired handler currently
returns (False, "(timeout)") but the reporting code later (the block that maps
failures to "ESP32-toolchain skip" for all non-success results) blindly treats
every non-zero result as an ESP32 skip; change the exception handler to return a
distinct, specific failure status (e.g., False with a code or message like
"timeout" or raise a custom TimeoutError) and update the reporting logic that
inspects the iwyu result to only classify a skip as ESP32-toolchain when a real
ESP32-toolchain indicator is present (rather than any non-success), otherwise
surface timeouts and other errors with their actual messages; locate and modify
the TimeoutExpired except branch and the reporting branch that currently
collapses all failures into an ESP32-toolchain skip to implement this
distinction.
In `@src/MoonBase/utilities/Coord3D.h`:
- Around line 87-90: The distanceSquared function in Coord3D currently
multiplies int values and returns unsigned which can overflow; change
Coord3D::distanceSquared to return uint64_t, compute deltas and their squared
products using widened signed type (e.g., int64_t) and accumulate into uint64_t
to avoid UB, and include <cstdint> if needed; then update all call sites that
assumed an unsigned result (e.g., uses in E_MoonLight where the result was
stored in unsigned variables) to use uint64_t (or a matching wider type) and
adjust any comparisons or casts accordingly.
---
Duplicate comments:
In `@interface/src/lib/stores/socket.ts`:
- Around line 54-75: Revert the upstream edits in the socket handler so you no
longer add special-case monitor/binary routing and extra console.error calls
directly in the socket file; instead create a helper in the allowed MoonLight
area (e.g., a new function like handleRawMonitorData or routeMonitorMessage)
that encapsulates the logic checking binary, converting to new
Uint8Array(message.data), and emitting to listeners.get('monitor') or
listeners.get('error'), and then call that helper from the socket code where the
payload decode/error and the check (!payload || typeof payload !== 'object')
happen; keep the socket file minimal (emit listeners.get('error') on decode
failure using the existing error object and only delegate binary/raw monitor
routing to the new MoonLight helper so the upstream file changes are minimal.
In `@platformio.ini`:
- Around line 330-331: The platformio.ini's [env:native] contains hardcoded
board-specific FastLED include paths (-I.pio/libdeps/esp32-s3/FastLED/src and
-I.pio/libdeps/esp32-s3/FastLED/tests) which break portability; remove these two
include entries from the native environment (or move them under the esp32-s3 env
or replace with an environment-agnostic lib_deps reference) so [env:native] no
longer references esp32-s3 libdeps.
In `@src/MoonBase/utilities/MemAlloc.h`:
- Around line 14-17: MemAlloc.h uses heap_caps_* functions and MALLOC_CAP_*
macros and std::forward but doesn't include their defining headers; update the
header to directly include the ESP heap caps API and the C++ utility header by
adding `#include` <esp_heap_caps.h> (or the appropriate ESP-IDF header that
defines heap_caps_* and MALLOC_CAP_*) and `#include` <utility> so symbols like
heap_caps_malloc, MALLOC_CAP_* and std::forward are available without relying on
include order; ensure these includes are placed near the top of MemAlloc.h
alongside the existing standard includes to avoid fragile include-order
dependencies.
- Around line 50-60: The multiplication n * sizeof(T) in reallocMB and
reallocMB2 can overflow; before calling heap_caps_realloc_prefer compute a safe
byte_count and guard it with if (sizeof(T) != 0 && n > SIZE_MAX / sizeof(T)) (or
equivalently compare n to SIZE_MAX/sizeof(T)), log/handle the error (e.g.,
EXT_LOGE and return nullptr) and only call heap_caps_realloc_prefer with the
verified byte_count; update any pSize bookkeeping in reallocMB2 after a
successful realloc. Ensure these checks are added to both reallocMB and
reallocMB2 and reference the exact expression "n * sizeof(T)" in your change.
- Around line 49-65: The realloc functions (reallocMB and reallocMB2) currently
call heap_caps_realloc_prefer without adjusting the global totalAllocatedMB,
causing drift when buffers change size; fix by computing the old allocated byte
size (for reallocMB call heap_caps_get_allocated_size(p) if p!=nullptr, for
reallocMB2 use pSize*sizeof(T) or heap_caps_get_allocated_size(p) to be safe),
perform the realloc, and on success update totalAllocatedMB by adding
(newSizeBytes - oldSizeBytes) (newSizeBytes = n * sizeof(T)); ensure p and pSize
are updated in reallocMB2 as now, and do not change totalAllocatedMB when
realloc fails.
---
Nitpick comments:
In `@interface/src/lib/stores/socket.ts`:
- Around line 69-75: The monitor-frame emission (listeners.get('monitor') and
new Uint8Array(message.data)) is duplicated; create a small helper function
(e.g., emitMonitorFrame or emitMonitorPayload) that accepts the raw data or
payload, constructs the Uint8Array from message.data (or uses the provided
buffer) and calls listeners.get('monitor')?.forEach(listener => listener(...)).
Replace both branches so they call this helper instead of repeating new
Uint8Array(message.data) and the listeners.get('monitor') logic, ensuring
identical behavior in all paths.
In `@src/MoonBase/utilities/Coord3D.h`:
- Around line 14-20: The header defines global MIN and MAX macros that should be
removed; delete the MIN/MAX macro definitions in Coord3D.h, add `#include`
<algorithm>, and update any code in this header (and callers if needed) to use
std::min and std::max (fully qualified) instead of MIN(...) / MAX(...) to avoid
macro collisions and double-evaluation; search for occurrences of MIN and MAX in
functions or inline code in Coord3D.h and replace them with std::min/std::max
(or std::clamp where appropriate) and ensure <algorithm> is included so the
symbols resolve.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/lint.ymlci/iwyu.pyinterface/src/lib/stores/socket.tsinterface/src/routes/wifi/sta/EditNetwork.svelteinterface/vite-plugin-littlefs.tslib/framework/WWWData.hmisc/upstream-prs/001-lint-fixes.mdplatformio.inisrc/MoonBase/utilities/Coord3D.hsrc/MoonBase/utilities/MemAlloc.h
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/routes/wifi/sta/EditNetwork.svelte
pio.ini: fix env:native frontend ======= - statusbar: show ethernet and battery conditional
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 `@interface/src/routes/statusbar.svelte`:
- Around line 134-136: This upstream statusbar.svelte change injects
MoonLight-specific UI (the {`#if` page.data.features.ethernet &&
$telemetry.ethernet.connected} conditional and the PlugConnected element) and
must be removed from the upstream route; extract that conditional UI into a new
MoonLight-only component (e.g., StatusbarMoonlight.svelte) placed in the
MoonLight frontend area, implement the same logic there referencing
$telemetry.ethernet.connected and PlugConnected, and update MoonLight-specific
routes to import and render that component instead; finally revert the changes
in statusbar.svelte to the original upstream state so it contains no
MoonLight-only conditionals.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
interface/src/routes/statusbar.sveltelib/framework/WWWData.hplatformio.inisrc/native_stub.cpp
✅ Files skipped from review due to trivial changes (1)
- src/native_stub.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- platformio.ini
Collaborator
Author
|
@coderabbitai, I am about to merge this PR, can you do an in depth review of all the changes made in this PR, give a merge advice and provide a list with must have and possible follow up actions before merging this pr and after merging this pr Please review especially the automated unit tests added: doctest, vitest and IWYU. Has this been setup correctly? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7124 🏁 Script executed: Length of output: 4120 🏁 Script executed: Length of output: 1730
🔍 In-Depth Test Framework Review✅ 1. Vitest (Frontend Unit Tests) — PROPERLY CONFIGURED
|
| Component | Status | Grade | Notes |
|---|---|---|---|
| Vitest Setup | ✅ Ready | A | Clean configuration, runs in CI |
| Doctest Setup | C | Configured but missing CI integration | |
| IWYU Setup | ✅ Excellent | A+ | Professional, informational-only approach |
| cpp_lint Setup | ✅ Ready | A | Custom rules, proper CI integration |
| CI Workflow | B- | Missing native test execution | |
| Documentation | ✅ Good | B+ | CLAUDE.md updated with test guidance |
| Test Coverage | 🟡 Basic | C+ | Only 2 utility functions tested |
🎯 Merge Recommendation
Verdict: MERGE AFTER adding the doctest CI job.
Reasoning:
- ✅ All test frameworks are technically sound
- ✅ IWYU is brilliantly implemented (informational-only is the right choice)
- ✅ Custom cpp_lint rules enforce coding standards
- ✅ Frontend tests work and run in CI
⚠️ Backend tests exist but don't run in CI (critical gap)- ✅ Overall architecture is production-ready
Time Estimate: 5 minutes to add the backend-tests job to lint.yml
🔧 Quick Fix Script
Want me to generate the complete updated lint.yml with the backend-tests job included? Just confirm and I can provide the full file ready to commit.
Post-merge: The badges can be added in a follow-up PR as they're cosmetic improvements, not functional requirements.
Does this assessment help? Should I generate the updated workflow file for you?
🧠 Learnings used
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T11:14:24.005Z
Learning: Applies to **/*.{cpp,h,ts,tsx,js} : Mark upstream sveltekit changes with `// 🌙` (moonbase additions) or `// 💫` (moonlight additions) in source code and consider whether changes could be submitted as a PR to upstream
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T11:14:24.005Z
Learning: Applies to **/interface/src/routes/moonbase/**/*.{ts,tsx,svelte,js} : ESLint only lints MoonLight-specific files in `src/routes/moonbase/`, `src/lib/components/moonbase/`, and moonbase stores/types — not upstream paths
Collaborator
Author
|
@coderabbitai, regarding
Yes |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2794
name: Lint
on:
push:
branches: [main]
pull_request:
branches: [main]
jobs:
frontend-lint:
name: Frontend (ESLint + Prettier)
runs-on: ubuntu-latest
defaults:
run:
working-directory: interface
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: '22'
cache: 'npm'
cache-dependency-path: interface/package-lock.json
- name: Install dependencies
run: npm ci
- name: Generate SvelteKit types
run: npx svelte-kit sync
- name: Lint
run: npm run lint
- name: Unit tests
run: npm run test
iwyu:
name: IWYU (Include What You Use)
runs-on: ubuntu-latest
continue-on-error: true # informational only — never blocks CI
steps:
- uses: actions/checkout@v4
- name: Install IWYU
run: sudo apt-get install -y iwyu
- uses: actions/setup-python@v5
with:
python-version: '3.11'
- name: Install PlatformIO
run: pip install platformio
- name: Install native dependencies
run: pio pkg install -e native
- name: Run IWYU analysis
run: python3 ci/iwyu.py
backend-lint:
name: Backend (cppcheck + cpp_lint)
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Run custom lint rules
run: python3 ci/cpp_lint.py
- name: Install cppcheck
run: sudo apt-get install -y cppcheck
- name: Run cppcheck
run: |
cppcheck \
--enable=warning,performance,style \
--suppress=missingInclude \
--suppress=missingIncludeSystem \
--inline-suppr \
--language=c++ \
--std=c++20 \
--error-exitcode=1 \
-DPLATFORMIO=1 \
-DFT_MOONLIGHT=1 \
-DFT_MOONBASE=1 \
"-DFT_ENABLED(x)=x" \
--suppress=noExplicitConstructor \
--suppress=functionStatic \
--suppress=constParameterReference \
--suppress=constParameterPointer \
--suppress=constParameterCallback \
--suppress=constVariablePointer \
--suppress=constVariableReference \
--suppress=constVariable \
--suppress=cstyleCast \
--suppress=unusedStructMember \
--suppress=useStlAlgorithm \
--suppress=variableScope \
--suppress=shadowVariable \
--suppress=shadowArgument \
--suppress=stlFindInsert \
--suppress=noConstructor \
--template="{file}:{line}: {severity}: {message} [{id}]" \
$(find src \( -name "*.cpp" -o -name "*.h" \) ! -path "*moonmanpng*" -print)
backend-tests:
name: Backend Unit Tests (doctest)
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: '3.11'
- name: Install PlatformIO
run: pip install platformio
- name: Install native dependencies
run: pio pkg install -e native
- name: Run native tests
run: pio test -e native📝 What ChangedAdded new job:
This will now execute your 30+ test cases in Ready to commit! 🚀 Just replace the entire 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, is cppcheck now defined on 2 places (also in Pio.ini)? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2412
1️⃣ cppcheck in Two Places — Different PurposesYES, cppcheck appears in both locations, but they serve different purposes: A. PlatformIO (
|
| Component | Can Remove? | Impact |
|---|---|---|
| cppcheck in platformio.ini | Optional | Loses local pio check convenience |
| cppcheck in CI workflow | ❌ No | Breaks automated linting |
| env:native | ❌ ABSOLUTELY NOT | Destroys all backend unit tests |
| native_stub.cpp | ❌ No | Required by PlatformIO build system |
If you remove env:native, the backend-tests CI job I just recommended will fail, and you'll have zero automated C++ testing.
Keep everything as-is — the setup is correct! 🚀
Does this clarify the architecture? The tests you've built are properly structured for both local and CI execution.
🧠 Learnings used
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T11:14:24.005Z
Learning: Applies to **/*.{cpp,h,ts,tsx,js} : Mark upstream sveltekit changes with `// 🌙` (moonbase additions) or `// 💫` (moonlight additions) in source code and consider whether changes could be submitted as a PR to upstream
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T11:14:24.005Z
Learning: Applies to **/interface/src/routes/moonbase/**/*.{ts,tsx,svelte,js} : ESLint only lints MoonLight-specific files in `src/routes/moonbase/`, `src/lib/components/moonbase/`, and moonbase stores/types — not upstream paths
lint.yml: add doctest index/readme: add badges
Collaborator
Author
|
@coderabbitai, resume, review 3aaedf7 and verify and summarize the readiness to merge (see above, all boxes checked now?) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5096 🏁 Script executed: Length of output: 3791
✅ Commit 3aaedf7 ReviewYour latest commit successfully addresses the critical missing piece: Changes in this commit:
📋 Final Merge Readiness Checklist🟢 MUST-HAVE Items (Critical)
🟡 RECOMMENDED Items
🔵 OPTIONAL Items (Post-Merge)
🎯 FINAL VERDICT: ✅ READY TO MERGEAll critical requirements are met! Your PR now has: ✅ Complete CI test automation:
✅ Proper status visibility:
✅ Correct architecture:
🚀 You're Clear to Merge!The only minor cosmetic issue I noticed is that some badge text appears outside the image tags in the README (e.g., "Lint Status" and "Build Status" will render as plain text), but this is purely visual and doesn't affect functionality. You can fix this in a follow-up if desired: <!-- Current (works but shows extra text): -->
<a href="..."><img src="..." alt="Lint Status">Lint Status</a>
<!-- Cleaner alternative: -->
<a href="..."><img src="..." alt="Lint Status"></a>But don't let this minor detail block the merge — your testing infrastructure is solid! 🎉 Go ahead and merge with confidence! 🚀 🧠 Learnings used |
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 by CodeRabbit