Conversation
Frontend ======== - Field renderer: support button labels Backend ======= - Lights control: setPresetsFromFolder: add labels - Solid effect: add minRGB to skip low light palette values
WalkthroughThis PR refactors the preset and channel control UI system by introducing a new MultiButtonsWidget component and updating FileEditWidget/FileManager to use modal-based dialogs. It also adds palette filtering logic with minRGB threshold, updates control type definitions, implements safe-mode gating for LiveScript nodes, and adjusts logging verbosity levels. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
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)
📝 Coding Plan
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: 1
🧹 Nitpick comments (2)
interface/src/lib/components/moonbase/FieldRenderer.svelte (1)
373-373: Remove debug comment before merge.The commented-out debug line should be removed to keep the codebase clean.
- <!-- <span>{JSON.stringify(value.labels)}</span> -->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/components/moonbase/FieldRenderer.svelte` at line 373, Remove the leftover debug comment in the FieldRenderer.svelte component: delete the commented-out line "<!-- <span>{JSON.stringify(value.labels)}</span> -->" so the file no longer contains the debug markup; ensure no other debug artifacts remain in the FieldRenderer.svelte render block.src/MoonLight/Modules/ModuleLightsControl.h (1)
427-429: Consider removing or uncommenting debug code.These commented-out debug lines should either be removed for cleanliness or uncommented if still needed for development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Modules/ModuleLightsControl.h` around lines 427 - 429, The commented debug block containing "String xxx;", "serializeJson( _state.data[\"preset\"], xxx);", and "EXT_LOGD(ML_TAG, "%s", xxx.c_str());" should be either removed or re-enabled; decide whether you need runtime JSON logging of _state.data["preset"] in ModuleLightsControl and then either delete these three lines to clean up the file or uncomment them and ensure serializeJson and EXT_LOGD are available in this compilation unit (include required headers and handle any name conflicts) so the debug output works as 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 `@src/MoonLight/Modules/ModuleLightsControl.h`:
- Around line 398-421: The label shadowing bug: remove the inner declaration
"char label[20];" inside the nodeName parsing block so the code writes into the
outer label buffer declared earlier; update the loop to write into the existing
label (ensuring it stays null-terminated and trimmed) and then let
_state.data["preset"]["labels"].add((const char*)label) use that populated
buffer; reference the outer variable "label", the parsed "nodeName", the
JsonDocument "doc"/deserializeJson call, and the final call
"_state.data[\"preset\"][\"labels\"].add".
---
Nitpick comments:
In `@interface/src/lib/components/moonbase/FieldRenderer.svelte`:
- Line 373: Remove the leftover debug comment in the FieldRenderer.svelte
component: delete the commented-out line "<!--
<span>{JSON.stringify(value.labels)}</span> -->" so the file no longer contains
the debug markup; ensure no other debug artifacts remain in the
FieldRenderer.svelte render block.
In `@src/MoonLight/Modules/ModuleLightsControl.h`:
- Around line 427-429: The commented debug block containing "String xxx;",
"serializeJson( _state.data[\"preset\"], xxx);", and "EXT_LOGD(ML_TAG, "%s",
xxx.c_str());" should be either removed or re-enabled; decide whether you need
runtime JSON logging of _state.data["preset"] in ModuleLightsControl and then
either delete these three lines to clean up the file or uncomment them and
ensure serializeJson and EXT_LOGD are available in this compilation unit
(include required headers and handle any name conflicts) so the debug output
works as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f8416b1-051b-4092-9ad7-53b90a8a2b70
📒 Files selected for processing (5)
interface/src/lib/components/moonbase/FieldRenderer.sveltelib/framework/WWWData.hsrc/MoonBase/NodeManager.cppsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Effects/E_MoonLight.h
frontend ======= - Field renderer: selectFile livescripts: FileEdit as widget - File edit: Collapsible -> modals, add Close - File Manager: add openFileEditor, remove showEditor backend ======= - Drivers and effects: livescript nodes only if ! safeMode - Lights control: labels bugfix
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Modules/ModuleEffects.h (1)
323-327:⚠️ Potential issue | 🟡 MinorHide or disable LiveScript options in safe mode.
Lines 323-327 now block
LiveScriptNodecreation in safe mode, butaddNodes()still adds every.scfile to the selector at Lines 195-203. That leaves safe-mode users with selectable entries thataddNode()can never instantiate. Please gate the enumeration the same way, or mark those entries unavailable instead of letting the selection fail silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Modules/ModuleEffects.h` around lines 323 - 327, The selector is still populated with .sc files even when safeModeMB prevents creating LiveScriptNode instances; update addNodes() to check the safeModeMB flag (same guard used in addNode()/LiveScriptNode creation) and skip adding .sc entries when safe mode is active, or alternatively mark those selector entries as disabled/unavailable instead of adding live choices that cannot be instantiated; reference the addNodes() enumeration logic and the LiveScriptNode creation guarded by safeModeMB to ensure consistent behavior.
🧹 Nitpick comments (4)
interface/src/routes/moonbase/filemanager/FileManager.svelte (1)
103-109: Good refactor to centralize modal opening logic.The
openFileEditorhelper provides a clean abstraction for opening the file editor modal with the correct props. This makes the code more maintainable.The
as anycast on line 104 is flagged by ESLint. Same workaround as in FieldRenderer for svelte-modals typing with Svelte 5 components.💡 Consider adding ESLint suppression comment
function openFileEditor(isNew: boolean, isFile: boolean, filePath: string) { + // eslint-disable-next-line `@typescript-eslint/no-explicit-any` modals.open(FileEditWidget as any, { newItem: isNew, isFile: isFile, path: filePath }); }🤖 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` around lines 103 - 109, The ESLint error is caused by the as any cast on FileEditWidget in openFileEditor; update the call to remove the cast and add the same eslint-disable-next-line comment used in FieldRenderer for svelte-modals/Svelte 5 typing (apply the specific eslint-disable-next-line `@typescript-eslint/ban-ts-comment` or the exact suppression used in FieldRenderer) directly above the modals.open(FileEditWidget, ...) line so the type suppression is localized to openFileEditor and FileEditWidget while keeping the centralized modal-opening logic.interface/src/lib/components/moonbase/FieldRenderer.svelte (2)
387-395: Remove commented-out code.Line 390 contains a commented-out version of the old class definition. This should be removed to keep the code clean.
🧹 Remove dead code
class="btn {value.labels ? 'flex-col !gap-0 overflow-hidden' : 'btn-square'} w-{property.size} h-{property.size} rounded-lg text-xl {value.selected == - // class="btn btn-square w-{property.size} h-{property.size} rounded-lg text-xl {value.selected == x + y * property.width + 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/components/moonbase/FieldRenderer.svelte` around lines 387 - 395, Remove the leftover commented-out old class definition inside FieldRenderer.svelte: locate the JSX/HTML element that builds the dynamic class attribute (the line starting with class="btn {value.labels ? 'flex-col ...' and the commented line beginning with // class="btn btn-square ...), and delete that commented-out line entirely so only the active dynamic class expression remains (refer to the class attribute in the template around the expression that computes btn-error/btn-success/btn-primary using value.selected and value.list).
217-225: Consider typing the modal component to avoidany.The
as anycast works but silences TypeScript. This is flagged by ESLint. Ifsvelte-modalsexports aComponenttype or similar, you could use that. Otherwise, this is an acceptable workaround for Svelte 5 component typing with modal libraries.💡 Potential improvement
- modals.open(FileEditWidget as any, { path: value }); + modals.open(FileEditWidget as typeof FileEditWidget, { path: value });Or suppress the lint warning if this is intentional:
+ // eslint-disable-next-line `@typescript-eslint/no-explicit-any` modals.open(FileEditWidget as any, { path: value });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/components/moonbase/FieldRenderer.svelte` around lines 217 - 225, Replace the unsafe "as any" cast on the modal component when calling modals.open in FieldRenderer.svelte: instead of modals.open(FileEditWidget as any, { path: value }), import and use a proper component type (e.g., the Component/ComponentType exported by svelte-modals or SvelteComponentTyped/ComponentType from 'svelte') and annotate FileEditWidget with that type; if svelte-modals exports a specific ModalComponent type, use that with modals.open(FileEditWidget as ModalComponent, { path: value }); if no suitable type exists, add a targeted eslint-disable-next-line comment immediately above this call to silence the rule while keeping a TODO to replace with a proper type later.interface/src/lib/components/moonbase/FileEditWidget.svelte (1)
229-260: Consider warning users about unsaved changes when closing.The "Close" button (lines 250-259) closes the modal without saving, even if
changedis true. Users might accidentally lose their edits. Consider either:
- Disabling "Close" when there are unsaved changes (like Cancel/Save)
- Adding a confirmation prompt before closing with unsaved changes
💡 Option 1: Disable Close when there are unsaved changes
<button class="btn btn-neutral text-neutral-content inline-flex items-center" onclick={() => { modals.close(1); }} type="button" + disabled={changed} > <Cancel class="mr-2 h-5 w-5" /> <span>Close</span> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/components/moonbase/FileEditWidget.svelte` around lines 229 - 260, The Close button currently calls modals.close(1) unconditionally and can discard edits when changed is true; update its onclick handler to check the changed boolean and, if true, either confirm with the user (e.g., if (!changed || confirm("You have unsaved changes. Close without saving?")) { modals.close(1) }) or disable the button like the Cancel/Save buttons by adding disabled={!changed}; modify the Close button's onclick (and optionally add disabled={!changed}) so it mirrors the Save/Cancel behavior and prevents accidental loss of edits.
🤖 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/test/moonbase_utilities.test.ts`:
- Around line 66-68: The test suite contains a duplicated test case using the
same spec string "returns unique emojis only" that calls extractEmojis('🔥🔥🔥
triple fire') and asserts ['🔥']; remove the redundant duplicate it(...) block
so only one instance of the "returns unique emojis only" test remains in
moonbase_utilities.test.ts (locate the duplicate it(...) invocation and delete
that entire test function).
---
Outside diff comments:
In `@src/MoonLight/Modules/ModuleEffects.h`:
- Around line 323-327: The selector is still populated with .sc files even when
safeModeMB prevents creating LiveScriptNode instances; update addNodes() to
check the safeModeMB flag (same guard used in addNode()/LiveScriptNode creation)
and skip adding .sc entries when safe mode is active, or alternatively mark
those selector entries as disabled/unavailable instead of adding live choices
that cannot be instantiated; reference the addNodes() enumeration logic and the
LiveScriptNode creation guarded by safeModeMB to ensure consistent behavior.
---
Nitpick comments:
In `@interface/src/lib/components/moonbase/FieldRenderer.svelte`:
- Around line 387-395: Remove the leftover commented-out old class definition
inside FieldRenderer.svelte: locate the JSX/HTML element that builds the dynamic
class attribute (the line starting with class="btn {value.labels ? 'flex-col
...' and the commented line beginning with // class="btn btn-square ...), and
delete that commented-out line entirely so only the active dynamic class
expression remains (refer to the class attribute in the template around the
expression that computes btn-error/btn-success/btn-primary using value.selected
and value.list).
- Around line 217-225: Replace the unsafe "as any" cast on the modal component
when calling modals.open in FieldRenderer.svelte: instead of
modals.open(FileEditWidget as any, { path: value }), import and use a proper
component type (e.g., the Component/ComponentType exported by svelte-modals or
SvelteComponentTyped/ComponentType from 'svelte') and annotate FileEditWidget
with that type; if svelte-modals exports a specific ModalComponent type, use
that with modals.open(FileEditWidget as ModalComponent, { path: value }); if no
suitable type exists, add a targeted eslint-disable-next-line comment
immediately above this call to silence the rule while keeping a TODO to replace
with a proper type later.
In `@interface/src/lib/components/moonbase/FileEditWidget.svelte`:
- Around line 229-260: The Close button currently calls modals.close(1)
unconditionally and can discard edits when changed is true; update its onclick
handler to check the changed boolean and, if true, either confirm with the user
(e.g., if (!changed || confirm("You have unsaved changes. Close without
saving?")) { modals.close(1) }) or disable the button like the Cancel/Save
buttons by adding disabled={!changed}; modify the Close button's onclick (and
optionally add disabled={!changed}) so it mirrors the Save/Cancel behavior and
prevents accidental loss of edits.
In `@interface/src/routes/moonbase/filemanager/FileManager.svelte`:
- Around line 103-109: The ESLint error is caused by the as any cast on
FileEditWidget in openFileEditor; update the call to remove the cast and add the
same eslint-disable-next-line comment used in FieldRenderer for
svelte-modals/Svelte 5 typing (apply the specific eslint-disable-next-line
`@typescript-eslint/ban-ts-comment` or the exact suppression used in
FieldRenderer) directly above the modals.open(FileEditWidget, ...) line so the
type suppression is localized to openFileEditor and FileEditWidget while keeping
the centralized modal-opening logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60671e4f-0c21-4491-a637-378827efeb3d
📒 Files selected for processing (9)
docs/components.mdinterface/src/lib/components/moonbase/FieldRenderer.svelteinterface/src/lib/components/moonbase/FileEditWidget.svelteinterface/src/routes/moonbase/filemanager/FileManager.svelteinterface/src/test/moonbase_utilities.test.tslib/framework/WWWData.hsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Modules/ModuleLightsControl.h
💤 Files with no reviewable changes (1)
- docs/components.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Modules/ModuleLightsControl.h
frontend ======= - Multi Buttons Widget: new - Field Renderer: remove 'pad' and use Multi Buttons Widget instead backend ======= - Module Channels: channel type channel - Lights control: preset type preset
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
interface/src/lib/components/moonbase/MultiButtonsWidget.svelte (1)
98-109: Click handler has 250ms delay and confusing toggle logic.
UX delay: The 250ms timeout to differentiate single vs double-click creates noticeable input lag for single clicks. Consider if double-click is essential, or use a different interaction pattern (e.g., long-press or modifier key).
Toggle logic clarity: Lines 102-106 are hard to follow:
value.select = btnIdx; // Set to clicked index if (value.selected == value.select) value.select = 255; // Reset if same value.selected = value.select; // Copy to selectedThis achieves toggle-deselect but could be clearer:
♻️ Clearer toggle logic
onclick={(event: MouseEvent) => { preventClick = false; clickTimeout = setTimeout(() => { if (!preventClick) { - value.select = btnIdx; - console.log('click', y, x, value.select); - if (value.selected == value.select) value.select = 255; - value.selected = value.select; + // Toggle: deselect if clicking the already-selected button + const isDeselect = value.selected === btnIdx; + value.select = isDeselect ? 255 : btnIdx; + value.selected = value.select; value.action = 'click'; onChange(event); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/components/moonbase/MultiButtonsWidget.svelte` around lines 98 - 109, The onclick handler currently uses a 250ms timeout and convoluted toggle steps (variables: preventClick, clickTimeout, value.select, value.selected, btnIdx, onChange), causing lag and confusing logic; replace the timeout-based approach (or move double-click logic to a dedicated ondblclick if double-click is needed) and simplify toggle to compute a single newSelect = (value.selected === btnIdx ? 255 : btnIdx) then set value.select = newSelect; value.selected = newSelect; value.action = 'click'; and call onChange(event); also remove or refactor preventClick/clickTimeout usage if no longer required.
🤖 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/components/moonbase/EditRowWidget.svelte`:
- Line 100: The button attribute contains an invalid JavaScript-style comment
inside the HTML (the commented "disabled={changed}" with a //), which breaks
Svelte/Prettier parsing; remove the inline // comment and either (a) move the
note outside the element as a normal line comment or an HTML comment, or (b)
restore the intended attribute by using disabled={changed} (or the correct
boolean expression) if you want the button to be disabled when the `changed`
state is true—look for the `disabled={changed}` occurrence in
EditRowWidget.svelte and update accordingly.
In `@interface/src/lib/components/moonbase/MultiButtonsWidget.svelte`:
- Around line 119-125: The onmouseenter handler calls
value.list.includes(btnIdx) which can throw if value.list is undefined; update
the handler in MultiButtonsWidget.svelte (the block using
property.hoverToServer, value.select/value.action, onChange, and
handleMouseEnter) to guard against a missing list by checking value.list before
calling includes (e.g. use optional chaining or a default empty array) so
handleMouseEnter receives a boolean safely; ensure the check preserves the
existing behavior when list is present.
- Line 148: Remove the debug console.log expression from the Svelte template
that renders undefined: delete the `{console.log('node.name', node.name)}`
inside the `{`#each`}` block in MultiButtonsWidget.svelte (the one referencing
node.name) so it no longer injects undefined into the DOM; also remove or guard
the other debug logs found at the top-level (the console.log calls near lines
where node list is built and at the other noted locations) or wrap them behind a
debug flag/ENV check to avoid leaking debug output in production.
- Around line 44-68: The hover handler has a race and type issue: in
handleMouseEnter you should create and store an AbortController per hover (used
by the fetch) and call controller.abort() in handleMouseLeave to cancel
in-flight requests so stale responses cannot overwrite newer ones; ensure
hoverTimeout is cleared and popupCell is only set when the fetch was successful.
Also make fileContent's runtime shape consistent with the template by either
setting fileContent to an object like { error: 'Failed to load file.' } on fetch
failure (or null and keep the popup hidden) so checks like fileContent.nodes
won't throw or show empty content; update handleMouseEnter/handleMouseLeave to
manage the controller and only assign popupCell/fileContent when the fetch
completes for the current hover.
- Around line 29-36: The drag-drop swap in handleDrop mutates property.rows but
the template renders from count/width-derived arrays (Array(count)/Array(rows)),
so swaps never show; change the rendering to source buttons from property.rows
(ensure property.rows is initialized/populated to match count/width), and in
handleDrop validate indices before swapping (check dragSource.row/col and
targetRow/col are within property.rows bounds) and make the assignment reactive
(e.g. replace property.rows with a new array/row copy after swap so Svelte
updates the UI). Reference: handleDrop, property.rows, count, width, rows, and
the rendering loops that use Array(count)/Array(rows).
- Around line 85-92: The Tailwind width/height classes are built dynamically
(w-{property.size}, h-{property.size}) in MultiButtonsWidget.svelte so they
won’t be picked up by the Tailwind extractor; fix by replacing those dynamic
class tokens with a deterministic approach: either (A) compute inline styles
from property.size (e.g. width/height style binding based on property.size), or
(B) create a size-to-class map (e.g. map property.size values to explicit "w-4",
"w-6", etc. class names and use that mapped class in the class string), or (C)
add the known size tokens to the Tailwind safelist; update the class
construction that references property.size and ensure any tests/styles still
match after the change.
---
Nitpick comments:
In `@interface/src/lib/components/moonbase/MultiButtonsWidget.svelte`:
- Around line 98-109: The onclick handler currently uses a 250ms timeout and
convoluted toggle steps (variables: preventClick, clickTimeout, value.select,
value.selected, btnIdx, onChange), causing lag and confusing logic; replace the
timeout-based approach (or move double-click logic to a dedicated ondblclick if
double-click is needed) and simplify toggle to compute a single newSelect =
(value.selected === btnIdx ? 255 : btnIdx) then set value.select = newSelect;
value.selected = newSelect; value.action = 'click'; and call onChange(event);
also remove or refactor preventClick/clickTimeout usage if no longer required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3df56cda-6fc3-403f-a34e-0a38b09db730
📒 Files selected for processing (6)
interface/src/lib/components/moonbase/EditRowWidget.svelteinterface/src/lib/components/moonbase/FieldRenderer.svelteinterface/src/lib/components/moonbase/MultiButtonsWidget.sveltelib/framework/WWWData.hsrc/MoonLight/Modules/ModuleChannels.hsrc/MoonLight/Modules/ModuleLightsControl.h
| modals.close(1); | ||
| }} | ||
| type="button" | ||
| // disabled={changed} // changed is unknown... todo |
There was a problem hiding this comment.
Invalid syntax: JS comment inside HTML element attribute.
Line 100 uses a // comment inside the HTML button element, which is invalid Svelte/HTML syntax. This will cause parsing issues and explains the Prettier failure.
To preserve the TODO note, move the comment outside the element or use an HTML comment:
🐛 Proposed fix
<button
class="btn btn-neutral text-neutral-content inline-flex items-center"
onclick={() => {
modals.close(1);
}}
type="button"
- // disabled={changed} // changed is unknown... todo
>
+ <!-- TODO: disabled={changed} - changed variable is currently undefined -->Alternatively, remove the line entirely and track the TODO separately if disabling the button when changes are pending is desired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/lib/components/moonbase/EditRowWidget.svelte` at line 100, The
button attribute contains an invalid JavaScript-style comment inside the HTML
(the commented "disabled={changed}" with a //), which breaks Svelte/Prettier
parsing; remove the inline // comment and either (a) move the note outside the
element as a normal line comment or an HTML comment, or (b) restore the intended
attribute by using disabled={changed} (or the correct boolean expression) if you
want the button to be disabled when the `changed` state is true—look for the
`disabled={changed}` occurrence in EditRowWidget.svelte and update accordingly.
Comment on lines
+29
to
+36
| function handleDrop(event: DragEvent, targetRow: number, targetCol: number) { | ||
| if (dragSource && property.rows) { | ||
| const temp = property.rows[dragSource.row][dragSource.col]; | ||
| property.rows[dragSource.row][dragSource.col] = property.rows[targetRow][targetCol]; | ||
| property.rows[targetRow][targetCol] = temp; | ||
| dragSource = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how property.rows is used/expected in the codebase
rg -n "property\.rows" --type=svelte -C 3
rg -n '"rows"' --glob '*.ts' --glob '*.svelte' -C 2Repository: MoonModules/MoonLight
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Find and read the MultiButtonsWidget.svelte file
find . -name "MultiButtonsWidget.svelte" -type fRepository: MoonModules/MoonLight
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Search for property.rows usage across the codebase
rg -n "property\.rows" -C 3Repository: MoonModules/MoonLight
Length of output: 1081
🏁 Script executed:
#!/bin/bash
# Search for where rows property is mentioned
rg -n "rows" -C 2 | head -100Repository: MoonModules/MoonLight
Length of output: 7359
🏁 Script executed:
#!/bin/bash
# Read the full MultiButtonsWidget.svelte file
cat -n ./interface/src/lib/components/moonbase/MultiButtonsWidget.svelteRepository: MoonModules/MoonLight
Length of output: 6976
🏁 Script executed:
#!/bin/bash
# Find where MultiButtonsWidget is imported/used
rg -n "MultiButtonsWidget" -C 3Repository: MoonModules/MoonLight
Length of output: 2215
🏁 Script executed:
#!/bin/bash
# Read FieldRenderer.svelte to see how MultiButtonsWidget is called
sed -n '310,335p' ./interface/src/lib/components/moonbase/FieldRenderer.svelteRepository: MoonModules/MoonLight
Length of output: 759
🏁 Script executed:
#!/bin/bash
# Search for where property.rows might be set or used in parent context
rg -n "property\.rows\|\.rows\s*=" -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 47
Drag-drop swap mutates property.rows but rendering uses calculated grid, causing swaps to have no visual effect.
The handleDrop function swaps values in property.rows (lines 31–33), but the actual button rendering (lines 159–177) derives the grid entirely from count and width reactive values. The component calculates rows = Math.ceil(count / width) (line 80) and iterates Array(count) and Array(rows) to render buttons—property.rows is never read during rendering.
Additionally, the swap logic performs no bounds checking before accessing property.rows[dragSource.row][dragSource.col]. It only verifies that dragSource and property.rows exist, but does not validate that the row/col indices are within array bounds.
Since property.rows is never populated anywhere in the codebase, the drag-drop functionality is currently a no-op.
Decide whether:
- Buttons should visually reorder on drag—if so, rendering must read from
property.rowsinstead of recalculating the grid. property.rowsis meant only for external state synchronization—if so, clarify the intended data flow and add bounds validation.- This drag-drop feature is incomplete and should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/lib/components/moonbase/MultiButtonsWidget.svelte` around lines
29 - 36, The drag-drop swap in handleDrop mutates property.rows but the template
renders from count/width-derived arrays (Array(count)/Array(rows)), so swaps
never show; change the rendering to source buttons from property.rows (ensure
property.rows is initialized/populated to match count/width), and in handleDrop
validate indices before swapping (check dragSource.row/col and targetRow/col are
within property.rows bounds) and make the assignment reactive (e.g. replace
property.rows with a new array/row copy after swap so Svelte updates the UI).
Reference: handleDrop, property.rows, count, width, rows, and the rendering
loops that use Array(count)/Array(rows).
Comment on lines
+44
to
+68
| function handleMouseEnter(cell: number, event: MouseEvent, savedPreset: boolean) { | ||
| hoverTimeout = setTimeout(async () => { | ||
| if (!property.hoverToServer && savedPreset) { | ||
| const response = await fetch( | ||
| `/rest/file/.config/presets/preset${cell.toString().padStart(2, '0')}.json` | ||
| ); | ||
| if (response.ok) { | ||
| fileContent = await response.json(); | ||
| console.log(fileContent); | ||
| } else { | ||
| fileContent = 'Failed to load file.'; | ||
| } | ||
| popupCell = cell; | ||
| } | ||
| // Offset the popup a bit to the right and down | ||
| popupX = event.clientX + 16; | ||
| popupY = event.clientY + 16; | ||
| }, 1000); | ||
| } | ||
|
|
||
| function handleMouseLeave() { | ||
| clearTimeout(hoverTimeout); | ||
| popupCell = null; | ||
| fileContent = null; | ||
| } |
There was a problem hiding this comment.
Potential race condition and type inconsistency in hover fetch.
-
Race condition: If the user hovers over multiple cells quickly, earlier fetch responses may arrive after later ones, causing stale data to display. Consider using an
AbortControllerto cancel in-flight requests when leaving. -
Type inconsistency:
fileContentis typed as{ nodes?: ... } | string | null(line 39). When fetch fails (line 54), it's set to a string'Failed to load file.'. However, the template at line 146 checksfileContent.nodes, which would beundefinedon a string, causing the popup to show empty content rather than the error message.
💡 Suggested improvements
+ let abortController: AbortController | null = null;
+
function handleMouseEnter(cell: number, event: MouseEvent, savedPreset: boolean) {
hoverTimeout = setTimeout(async () => {
if (!property.hoverToServer && savedPreset) {
+ abortController?.abort();
+ abortController = new AbortController();
const response = await fetch(
- `/rest/file/.config/presets/preset${cell.toString().padStart(2, '0')}.json`
+ `/rest/file/.config/presets/preset${cell.toString().padStart(2, '0')}.json`,
+ { signal: abortController.signal }
- );
+ ).catch(() => null);
+ if (!response) return;For the error display, either show the error string in the popup or keep the popup hidden on failure.
📝 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
| function handleMouseEnter(cell: number, event: MouseEvent, savedPreset: boolean) { | |
| hoverTimeout = setTimeout(async () => { | |
| if (!property.hoverToServer && savedPreset) { | |
| const response = await fetch( | |
| `/rest/file/.config/presets/preset${cell.toString().padStart(2, '0')}.json` | |
| ); | |
| if (response.ok) { | |
| fileContent = await response.json(); | |
| console.log(fileContent); | |
| } else { | |
| fileContent = 'Failed to load file.'; | |
| } | |
| popupCell = cell; | |
| } | |
| // Offset the popup a bit to the right and down | |
| popupX = event.clientX + 16; | |
| popupY = event.clientY + 16; | |
| }, 1000); | |
| } | |
| function handleMouseLeave() { | |
| clearTimeout(hoverTimeout); | |
| popupCell = null; | |
| fileContent = null; | |
| } | |
| let abortController: AbortController | null = null; | |
| function handleMouseEnter(cell: number, event: MouseEvent, savedPreset: boolean) { | |
| hoverTimeout = setTimeout(async () => { | |
| if (!property.hoverToServer && savedPreset) { | |
| abortController?.abort(); | |
| abortController = new AbortController(); | |
| const response = await fetch( | |
| `/rest/file/.config/presets/preset${cell.toString().padStart(2, '0')}.json`, | |
| { signal: abortController.signal } | |
| ).catch(() => null); | |
| if (!response) return; | |
| if (response.ok) { | |
| fileContent = await response.json(); | |
| console.log(fileContent); | |
| } else { | |
| fileContent = 'Failed to load file.'; | |
| } | |
| popupCell = cell; | |
| } | |
| // Offset the popup a bit to the right and down | |
| popupX = event.clientX + 16; | |
| popupY = event.clientY + 16; | |
| }, 1000); | |
| } | |
| function handleMouseLeave() { | |
| clearTimeout(hoverTimeout); | |
| popupCell = null; | |
| fileContent = null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/lib/components/moonbase/MultiButtonsWidget.svelte` around lines
44 - 68, The hover handler has a race and type issue: in handleMouseEnter you
should create and store an AbortController per hover (used by the fetch) and
call controller.abort() in handleMouseLeave to cancel in-flight requests so
stale responses cannot overwrite newer ones; ensure hoverTimeout is cleared and
popupCell is only set when the fetch was successful. Also make fileContent's
runtime shape consistent with the template by either setting fileContent to an
object like { error: 'Failed to load file.' } on fetch failure (or null and keep
the popup hidden) so checks like fileContent.nodes won't throw or show empty
content; update handleMouseEnter/handleMouseLeave to manage the controller and
only assign popupCell/fileContent when the fetch completes for the current
hover.
Comment on lines
+85
to
+92
| class="btn {value.labels | ||
| ? 'flex-col !gap-0 overflow-hidden' | ||
| : 'btn-square'} w-{property.size} h-{property.size} rounded-lg text-xl {value.selected == | ||
| btnIdx | ||
| ? `btn-error` | ||
| : Array.isArray(value.list) && value.list.includes(btnIdx) | ||
| ? `btn-success` | ||
| : 'btn-primary'}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if size values are used elsewhere (which would include them in the bundle)
rg -n "w-[0-9]+" --type=svelte | head -30
# Check for safelist configuration
fd -e css -e js -e ts "tailwind" --exec grep -l "safelist" {} \;Repository: MoonModules/MoonLight
Length of output: 95
🏁 Script executed:
# Find the MultiButtonsWidget.svelte file and explore its context
find . -name "MultiButtonsWidget.svelte" -type fRepository: MoonModules/MoonLight
Length of output: 130
🏁 Script executed:
# Look for Tailwind configuration files
find . -name "tailwind.config.*" -o -name "tailwind.config.js" -o -name "tailwind.config.ts" | head -20Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Search for property.size usage without file type restriction
rg "property\.size" -A 2 -B 2Repository: MoonModules/MoonLight
Length of output: 569
🏁 Script executed:
# Read the MultiButtonsWidget.svelte file to understand property.size definition
cat interface/src/lib/components/moonbase/MultiButtonsWidget.svelteRepository: MoonModules/MoonLight
Length of output: 5709
🏁 Script executed:
# Check for Tailwind configuration in vite/svelte config or package.json
find . -name "svelte.config.*" -o -name "vite.config.*" -o -name "package.json" | grep -E "(svelte|vite|package)" | head -10Repository: MoonModules/MoonLight
Length of output: 145
🏁 Script executed:
# Read svelte.config.js
cat interface/svelte.config.jsRepository: MoonModules/MoonLight
Length of output: 630
🏁 Script executed:
# Read vite.config.ts
cat interface/vite.config.tsRepository: MoonModules/MoonLight
Length of output: 1075
🏁 Script executed:
# Check package.json for Tailwind and build setup
cat interface/package.json | head -50Repository: MoonModules/MoonLight
Length of output: 1542
🏁 Script executed:
# Search for where property.size is assigned or documented
rg "property\.size\s*=" -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Search for size-related property definitions or documentation
rg "size.*[0-9]+" interface/src --type=ts --type=js --type=svelte | head -20Repository: MoonModules/MoonLight
Length of output: 95
🏁 Script executed:
# Look for tailwind config in root or CSS files
find . -name "app.css" -o -name "global.css" -o -name "tailwind.config*" | xargs grep -l "safelist\|content" 2>/dev/nullRepository: MoonModules/MoonLight
Length of output: 49
🏁 Script executed:
# Search for MultiButtonsWidget usage to understand property.size values
rg "MultiButtonsWidget" -A 5 -B 5Repository: MoonModules/MoonLight
Length of output: 3146
🏁 Script executed:
# Find CSS files and check for Tailwind setup
find interface/src -name "*.css" -type fRepository: MoonModules/MoonLight
Length of output: 86
🏁 Script executed:
# Search for where property.size might be documented or constrained
rg "size\s*:\s*" interface/src --type=ts -A 1 -B 1 | head -40Repository: MoonModules/MoonLight
Length of output: 1271
🏁 Script executed:
# Read app.css to see Tailwind setup
cat interface/src/app.cssRepository: MoonModules/MoonLight
Length of output: 657
🏁 Script executed:
# Search for property objects being initialized or populated with size values
rg "size\s*:" interface/src -A 2 -B 2 | head -50Repository: MoonModules/MoonLight
Length of output: 3263
🏁 Script executed:
# Search for literal w- and h- classes with numbers to see if size values are generated elsewhere
rg "class=\"[^\"]*w-[0-9]+" interface/src
rg "class=\"[^\"]*h-[0-9]+" interface/srcRepository: MoonModules/MoonLight
Length of output: 38278
Dynamic Tailwind classes may not work without safelist.
The classes w-{property.size} and h-{property.size} are constructed dynamically at runtime using template literals. Tailwind CSS (including v4) requires class names to be statically analyzable at build time. With @tailwindcss/vite v4.1.11, the build process scans templates for literal class names. Since property.size values are dynamic and resolved at runtime, these classes won't be extracted during the build unless the values appear literally elsewhere in the codebase or are explicitly safelisted.
Options to fix:
- Use inline styles:
style="width: {property.size * 0.25}rem; height: {property.size * 0.25}rem;" - Add a safelist entry in Tailwind config for expected sizes
- Use a class map with all possible sizes
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/lib/components/moonbase/MultiButtonsWidget.svelte` around lines
85 - 92, The Tailwind width/height classes are built dynamically
(w-{property.size}, h-{property.size}) in MultiButtonsWidget.svelte so they
won’t be picked up by the Tailwind extractor; fix by replacing those dynamic
class tokens with a deterministic approach: either (A) compute inline styles
from property.size (e.g. width/height style binding based on property.size), or
(B) create a size-to-class map (e.g. map property.size values to explicit "w-4",
"w-6", etc. class names and use that mapped class in the class string), or (C)
add the known size tokens to the Tailwind safelist; update the class
construction that references property.size and ensure any tests/styles still
match after the change.
Comment on lines
+119
to
+125
| onmouseenter={(event: MouseEvent) => { | ||
| if (property.hoverToServer) { | ||
| value.select = btnIdx; | ||
| value.action = 'mouseenter'; | ||
| onChange(event); | ||
| } else handleMouseEnter(btnIdx, event, value.list.includes(btnIdx)); | ||
| }} |
There was a problem hiding this comment.
Potential TypeError if value.list is undefined.
Line 124 calls value.list.includes(btnIdx) without checking if value.list is defined. If value.list is undefined or null, this will throw a TypeError.
🐛 Proposed fix
onmouseenter={(event: MouseEvent) => {
if (property.hoverToServer) {
value.select = btnIdx;
value.action = 'mouseenter';
onChange(event);
- } else handleMouseEnter(btnIdx, event, value.list.includes(btnIdx));
+ } else handleMouseEnter(btnIdx, event, Array.isArray(value.list) && value.list.includes(btnIdx));
}}📝 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
| onmouseenter={(event: MouseEvent) => { | |
| if (property.hoverToServer) { | |
| value.select = btnIdx; | |
| value.action = 'mouseenter'; | |
| onChange(event); | |
| } else handleMouseEnter(btnIdx, event, value.list.includes(btnIdx)); | |
| }} | |
| onmouseenter={(event: MouseEvent) => { | |
| if (property.hoverToServer) { | |
| value.select = btnIdx; | |
| value.action = 'mouseenter'; | |
| onChange(event); | |
| } else handleMouseEnter(btnIdx, event, Array.isArray(value.list) && value.list.includes(btnIdx)); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/lib/components/moonbase/MultiButtonsWidget.svelte` around lines
119 - 125, The onmouseenter handler calls value.list.includes(btnIdx) which can
throw if value.list is undefined; update the handler in
MultiButtonsWidget.svelte (the block using property.hoverToServer,
value.select/value.action, onChange, and handleMouseEnter) to guard against a
missing list by checking value.list before calling includes (e.g. use optional
chaining or a default empty array) so handleMouseEnter receives a boolean
safely; ensure the check preserves the existing behavior when list is present.
| > | ||
| {#if fileContent && fileContent.nodes} | ||
| {#each fileContent.nodes as node, ni (ni)} | ||
| {console.log('node.name', node.name)} |
There was a problem hiding this comment.
Remove debug console.log from template - renders undefined to DOM.
The expression {console.log('node.name', node.name)} inside the {#each} block will render undefined into the DOM since console.log() returns undefined. This is a debug artifact that should be removed.
🐛 Proposed fix
{`#if` fileContent && fileContent.nodes}
{`#each` fileContent.nodes as node, ni (ni)}
- {console.log('node.name', node.name)}
<p>{node.name} {node.on ? 'on' : 'off'}</p>
{/each}
{/if}Also consider removing the other console.log statements at lines 52, 103, and 115 before merging, or gate them behind a debug flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/lib/components/moonbase/MultiButtonsWidget.svelte` at line 148,
Remove the debug console.log expression from the Svelte template that renders
undefined: delete the `{console.log('node.name', node.name)}` inside the
`{`#each`}` block in MultiButtonsWidget.svelte (the one referencing node.name) so
it no longer injects undefined into the DOM; also remove or guard the other
debug logs found at the top-level (the console.log calls near lines where node
list is built and at the other noted locations) or wrap them behind a debug
flag/ENV check to avoid leaking debug output in production.
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
New Features
Improvements