Feature/widget registry system #190
Conversation
- Create core widget registry system (registry.js, base-widget.js) - Extract existing widgets to separate modules: - file-upload.js: Image upload with drag-and-drop, preview, delete, scheduling - checkbox-group.js: Multi-select checkboxes for array fields - custom-feeds.js: Table-based RSS feed editor with logo uploads - Implement plugin widget loading system (plugin-loader.js) - Add comprehensive documentation (widget-guide.md, README.md) - Include example custom widget (example-color-picker.js) - Maintain backwards compatibility with existing plugins - All widget handlers available globally for existing functionality This enables: - Reusable UI components for plugin configuration forms - Third-party plugins to create custom widgets without modifying LEDMatrix - Modular widget architecture for future enhancements Existing plugins (odds-ticker, static-image, news) continue to work without changes.
📝 WalkthroughWalkthroughAdds a client-side LEDMatrix widget system: a global widget registry and loader, BaseWidget utilities, multiple widgets (file-upload, custom-feeds, checkbox-group, color-picker example), documentation, template script imports, a VSCode workspace file, and a script to update plugin repositories. Changes
Sequence Diagram(s)sequenceDiagram
participant Form as Config Form
participant Registry as LEDMatrixWidgets
participant PluginLoader as Plugin Loader
participant Widget as Widget Script
participant DOM as Browser DOM
Form->>Registry: has(widgetName)
Registry-->>Form: true/false
alt widget not registered
Form->>PluginLoader: loadPluginWidget(pluginId, widgetName)
PluginLoader->>DOM: fetch/append widget script (multiple path fallbacks)
DOM-->>Widget: execute script
Widget->>Registry: register(widgetName, definition)
Registry-->>Form: registered
end
Form->>Registry: render(widgetName, container, config, value, options)
Registry->>Widget: render(container, config, value, options)
Widget->>DOM: create/update DOM
DOM-->>Form: rendered UI
User->>DOM: interacts
DOM->>Widget: event handlers
Widget->>Registry: getValue(widgetName, fieldId)
Registry-->>Widget: value returned
Widget-->>Form: updated value
sequenceDiagram
participant User as User
participant FileWidget as File Upload Widget
participant Server as Upload API
participant DataInput as Hidden Data Input
participant DOM as Browser DOM
User->>FileWidget: drop/select files
FileWidget->>FileWidget: validate (count, size, type)
alt validation fails
FileWidget->>User: show error/notification
else
FileWidget->>FileWidget: show progress UI
loop each file
FileWidget->>Server: POST FormData (file + metadata)
Server-->>FileWidget: response {success, path, id}
FileWidget->>DataInput: append serialized image entry
FileWidget->>DOM: build/update image list item + schedule UI
end
FileWidget->>User: hide progress, update UI
end
User->>FileWidget: edit schedule or delete image
FileWidget->>DataInput: update serialized state
FileWidget->>DOM: refresh list/preserve open schedule panels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✏️ Tip: You can disable this entire section by setting 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: 11
🤖 Fix all issues with AI agents
In `@web_interface/static/v3/js/widgets/base-widget.js`:
- Around line 114-121: escapeHtml currently returns String(text) for non-strings
without escaping, which allows unescaped content to reach places like showError
where it's inserted via innerHTML; change escapeHtml to always coerce the input
to a string (e.g., String(text)) and then escape it by assigning that string to
a DOM node's textContent and returning node.innerHTML so every input type is
properly escaped; update the escapeHtml implementation (referenced by escapeHtml
and callers like showError) to perform the coercion first and always use
textContent -> innerHTML for the returned escaped string.
In `@web_interface/static/v3/js/widgets/checkbox-group.js`:
- Around line 60-75: In setValue, values.includes(optionValue) can fail when
types differ (e.g., optionValue is "1" but values contains 1); normalize types
before comparison by converting the incoming values to strings or numbers (or
convert optionValue) so comparisons are consistent—e.g., create a
normalizedValues collection (e.g., values.map(String) or values.map(Number)) and
then use normalizedValues.includes(String(optionValue)) (update references in
setValue and leave updateCheckboxGroupData unchanged).
In `@web_interface/static/v3/js/widgets/custom-feeds.js`:
- Around line 77-95: The current setValue function clears the DOM (tbody for
fieldId) but does not recreate rows, which can erase user data; update setValue
(in CustomFeedsWidget) to be defensive: either throw a clear NotImplementedError
(e.g., throw new Error('CustomFeedsWidget.setValue not implemented')) at the top
when Array.isArray(feeds) is true to avoid mutating the DOM, or implement the
full behavior by using options.fullKey and options.pluginId to build each row
for each feed, creating row elements and appending them to the tbody (use
fieldId to locate `${fieldId}_tbody`), and only clear tbody.innerHTML
immediately before appending the new rows; ensure you reference setValue,
fieldId, feeds, tbody, options.fullKey and options.pluginId in the change.
In `@web_interface/static/v3/js/widgets/example-color-picker.js`:
- Around line 49-76: The inline template in container.innerHTML uses
escapeHtml(currentValue) directly inside the style attribute (background-color:
${escapeHtml(currentValue)}) which allows CSS injection; update the code in
example-color-picker.js where container.innerHTML is built to first validate
currentValue against a strict hex color regex (e.g., /^#[0-9A-Fa-f]{6}$/) and
only insert it into the style and input values if it passes, otherwise
substitute a safe default like "#000000"; keep using escapeHtml for HTML
contexts but ensure the value used in the style attribute is the
validated/sanitized hex and apply the same check when setting the value for the
text input (IDs: ${fieldId}_color, ${fieldId}_hex, and preview element
${fieldId}_preview).
In `@web_interface/static/v3/js/widgets/file-upload.js`:
- Around line 323-387: The list re-render in updateImageList clears any open
schedule editor because callers like updateImageSchedule* fully replace
innerHTML; change updateImageList to support a "preserve open" path (or a
data-only patch) so schedule editors are not closed: detect/open state by
reading the open schedule id from the DOM (the schedule_<id> element or a new
data attribute on img_<id>), then when updating use targeted DOM updates for
changed fields (update hidden input fieldId_images_data and only replace
elements inside img_<id> that changed: thumbnail, text nodes, size/date,
schedule summary) instead of resetting imageList.innerHTML, or restore the saved
open schedule content into schedule_<id> after rendering; update callers
(updateImageSchedule* and similar) to pass the preserve flag or rely on the
DOM-detected open id so editing remains open.
- Around line 335-376: The current use of imageList.innerHTML building strings
(including img.path, img.filename, fieldId, pluginId) and inline onclick
handlers (window.openImageSchedule, window.deleteUploadedImage) creates a stored
XSS risk; replace the HTML string construction in the imageList.innerHTML block
with programmatic DOM creation: create container elements (matching the same
classes/structure and id pattern "img_<...>"), set textContent/attributes (use
the existing escapeHtml or prefer textContent) for filenames and summaries, set
img.src via DOM property (not via innerHTML), store identifiers in data-*
attributes on the element, and attach event listeners with addEventListener that
read those data attributes and call
window.openImageSchedule/window.deleteUploadedImage; ensure any user-controlled
values used in ids are sanitized to safe token (as currently done with replace)
and never injected into inline JS strings.
- Around line 233-239: When filtering out the deleted file in file-upload.js,
normalize types so string fileId matches numeric file.id; modify the filter
inside the code that builds newFiles (which currently references
currentFiles.filter and compares (file.id || file.category_name) !== fileId) to
coerce both sides to the same type (e.g., String(file.id) or
String(file.category_name) compared to String(fileId)) or use a loose equality
on normalized values, then call window.updateImageList(fieldId, newFiles) as
before; update the comparison near currentFiles, fileId, file.id,
file.category_name, getCurrentImages and updateImageList to ensure proper
removal of the deleted item.
- Around line 489-495: openImageSchedule uses the raw imageId when querying the
DOM ("schedule_${imageId || imageIdx}") which can fail if IDs contain special
chars; align it with updateImageList's sanitization by computing the same
sanitizedId (e.g. const sanitized = (imageId ||
imageIdx).toString().replace(/[^a-z0-9-_]/gi, '') or call the existing sanitizer
if one exists) and use document.getElementById(`schedule_${sanitized}`) so the
lookup matches updateImageList's DOM IDs.
- Around line 96-139: The max-files check in window.handleFiles runs before
filtering and doesn't honor uploadConfig.allowed_types; move the maxFiles
enforcement so it occurs after you build validFiles, and derive allowedTypes
from uploadConfig.allowed_types (falling back to the existing default list) when
validating each file. Specifically: in handleFiles, use
uploadConfig.allowed_types ||
['image/png','image/jpeg','image/jpg','image/bmp','image/gif'] to validate
file.type (and still handle 'json' case), collect validFiles first, then check
if currentFiles.length + validFiles.length > maxFiles and notify or trim the
validFiles array accordingly before proceeding with uploads. Ensure you
reference maxFiles, allowedTypes (or uploadConfig.allowed_types), currentFiles
and validFiles in the updated logic.
In `@web_interface/static/v3/js/widgets/README.md`:
- Around line 293-300: Extend the Security section to advise sanitizing
identifiers used as element IDs and in CSS selectors (e.g., fieldId) in addition
to escaping HTML; instruct authors to validate and normalize IDs (allow only a
safe subset like [A-Za-z0-9_-], trim/replace invalid chars, or prefix IDs) and
to use DOM-safe APIs (textContent, escapeHtml) and CSS.escape or a sanitizeId
helper before using values in querySelector, getElementById, or setting id
attributes to prevent selector injection/XSS; reference existing functions/terms
like escapeHtml, textContent, and the identifier fieldId so reviewers can update
code to call a sanitizeId routine where IDs are generated or used.
- Around line 120-203: The examples use fieldId directly in the template and
querySelector causing XSS/selector-injection; add a sanitization helper (e.g.,
sanitizeId or escapeCssIdentifier) and use it in render, getValue, and setValue
when building the id attribute and when querying the DOM (replace occurrences of
`${fieldId}_input` and `#${fieldId}_input`), ensuring the helper strips/encodes
unsafe characters or produces a safe DOM id and a safe CSS selector; keep
escapeHtml for values but never interpolate raw fieldId into HTML or selectors
without running it through the new sanitizer.
♻️ Duplicate comments (2)
web_interface/static/v3/js/widgets/README.md (1)
321-451: Apply fieldId sanitization to all widget examples.The color picker and slider examples have the same XSS and selector injection vulnerabilities as the basic widget example:
Color Picker Widget:
- Lines 333, 337: Unsanitized
fieldIdin element IDs- Lines 344-345, 361-366: Unsafe querySelector usage
Slider Widget:
- Lines 402, 410: Unsanitized
fieldIdin element IDs- Lines 416-417, 426, 431-437: Unsafe querySelector usage
Apply the same sanitization approach from the previous comment to these examples.
docs/widget-guide.md (1)
117-200: Same fieldId sanitization issue as in the web_interface README.This example has the same XSS and selector injection vulnerabilities:
- Line 155:
id="${fieldId}_input"- unsanitized fieldId in HTML- Lines 172, 180:
querySelector('#${fieldId}_input')- unsafe selector usageWhile this example includes an inline
escapeHtmlfunction (lines 146-150), it only sanitizes the value, not thefieldId.Apply the same sanitization fix as recommended for the other README file.
🧹 Nitpick comments (6)
web_interface/static/v3/js/widgets/README.md (1)
476-485: Clarify the relationship between server-side and client-side rendering.The migration section states "server-side rendering remains the primary method" but the entire guide focuses on client-side widget creation with
LEDMatrixWidgets.register(). This could confuse developers about when and how to use each approach.Consider adding a section that explicitly explains:
- When widgets are server-side rendered (existing core widgets)
- When client-side rendering is used (custom plugin widgets)
- The current capabilities vs. future enhancements
- How the two approaches coexist
docs/widget-guide.md (1)
1-347: Consider consolidating or clearly differentiating the two widget guides.There are now two widget documentation files with significant overlap:
web_interface/static/v3/js/widgets/README.md(491 lines)docs/widget-guide.md(347 lines)While they have some differences (this file includes "Used by" references and implementation status, while the README has more detailed examples), the duplication could lead to:
- Maintenance burden (updating both files)
- Inconsistencies over time
- Confusion about which to reference
Consider one of these approaches:
- Single source of truth: Keep only one comprehensive guide and remove the other
- Clear differentiation: Make one a "Developer Reference" (comprehensive API docs) and the other a "Quick Start Guide" (getting started tutorial)
- Cross-reference: Have one file be the main guide and the other reference it with "See [other file] for complete documentation"
web_interface/static/v3/js/widgets/base-widget.js (1)
51-56: ClarifysanitizeValue’s security guarantees.This only strips
<script>tags; it won’t neutralize event handlers or unsafe URLs if values are later injected into HTML. Consider documenting the limitation to avoid a false sense of XSS protection.💡 Suggested clarification
- // Basic XSS prevention + // NOTE: This only strips <script> tags. If values are injected into HTML, + // callers must escape or sanitize appropriately. return value.replace(/<script\b[^<]*(?:(?!<\/script>)<[^<]*)*<\/script>/gi, '');web_interface/static/v3/js/widgets/plugin-loader.js (1)
85-92: Misleading comment: no core widget loading logic follows.The comment at line 87 states "Continue to check if it's a core widget" but the code simply returns
false. Consider removing or updating the comment to reflect actual behavior.📝 Suggested fix
} catch (error) { console.warn(`[PluginWidgetLoader] Could not load widget ${widgetName} from plugin ${pluginId}:`, error); - // Continue to check if it's a core widget } } - // Widget not found + // Widget not found in registry and could not be loaded return false;web_interface/static/v3/js/widgets/example-color-picker.js (1)
167-181: Inconsistent event dispatch target.This widget dispatches
widget-changeondocument(line 174), whilecheckbox-group.jsdispatches on thehiddenInputelement. Consider using a consistent pattern across widgets for easier event handling.Additionally, line 177's selector
input[name*="${fieldId}"]uses a substring match which could match unintended inputs iffieldIdis a common substring.web_interface/static/v3/js/widgets/file-upload.js (1)
157-183: Handle non-OK and non-JSON upload responses.
response.json()will throw on HTML/empty error bodies and hides HTTP status. A small guard improves resilience and user-facing error quality.♻️ Suggested hardening
- const data = await response.json(); + if (!response.ok) { + throw new Error(`Upload failed (${response.status})`); + } + let data; + try { + data = await response.json(); + } catch { + throw new Error('Upload failed (invalid JSON response)'); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/widget-guide.mdplugins/basketball-scoreboardweb_interface/static/v3/js/widgets/README.mdweb_interface/static/v3/js/widgets/base-widget.jsweb_interface/static/v3/js/widgets/checkbox-group.jsweb_interface/static/v3/js/widgets/custom-feeds.jsweb_interface/static/v3/js/widgets/example-color-picker.jsweb_interface/static/v3/js/widgets/file-upload.jsweb_interface/static/v3/js/widgets/plugin-loader.jsweb_interface/static/v3/js/widgets/registry.jsweb_interface/templates/v3/base.html
🧰 Additional context used
🧬 Code graph analysis (6)
web_interface/static/v3/js/widgets/base-widget.js (3)
web_interface/static/v3/js/widgets/custom-feeds.js (1)
container(310-310)web_interface/static/v3/js/widgets/example-color-picker.js (3)
div(44-44)fieldId(39-39)event(169-173)web_interface/static/v3/js/widgets/checkbox-group.js (1)
event(108-112)
web_interface/static/v3/js/widgets/plugin-loader.js (1)
web_interface/static/v3/js/widgets/file-upload.js (2)
pluginId(98-98)pluginId(333-333)
web_interface/static/v3/js/widgets/checkbox-group.js (3)
web_interface/static/v3/js/widgets/custom-feeds.js (1)
container(310-310)web_interface/static/v3/js/widgets/example-color-picker.js (3)
fieldId(39-39)hiddenInput(177-177)event(169-173)web_interface/static/v3/js/widgets/file-upload.js (2)
hiddenInput(307-307)hiddenInput(324-324)
web_interface/static/v3/js/widgets/custom-feeds.js (1)
web_interface/static/v3/js/widgets/file-upload.js (12)
pluginId(98-98)pluginId(333-333)notifyFn(107-107)notifyFn(116-116)notifyFn(124-124)notifyFn(132-132)notifyFn(173-173)notifyFn(176-176)notifyFn(181-181)notifyFn(241-241)notifyFn(244-244)notifyFn(249-249)
web_interface/static/v3/js/widgets/file-upload.js (1)
web_interface/static/v3/js/widgets/checkbox-group.js (2)
hiddenInput(43-43)hiddenInput(89-89)
web_interface/static/v3/js/widgets/example-color-picker.js (2)
web_interface/static/v3/js/widgets/file-upload.js (3)
escapeHtml(341-345)escapeHtml(510-514)div(511-511)web_interface/static/v3/js/widgets/checkbox-group.js (1)
event(108-112)
🔇 Additional comments (25)
plugins/basketball-scoreboard (1)
1-1: No action needed. The basketball-scoreboard submodule has not been updated in this PR—it remains at commit d626e538c0fbfcb92cbc3f1c351ffe6d2c6c00bb in both HEAD and the current state. No changes to the submodule were found.Likely an incorrect or invalid review comment.
web_interface/static/v3/js/widgets/README.md (1)
1-92: LGTM! Clear and comprehensive documentation.The overview and core widgets documentation is well-structured with clear schema examples and feature descriptions. This provides excellent guidance for developers.
docs/widget-guide.md (2)
1-116: LGTM! Clear documentation with helpful plugin references.The core widgets documentation includes "Used by:" fields that provide valuable context about which plugins already use each widget. This helps developers understand real-world usage patterns.
324-342: Excellent implementation status section!This "Current Implementation Status" section clearly explains:
- What's been completed (Phase 1)
- Current behavior (server-side rendering)
- Future enhancements (client-side rendering)
- Backwards compatibility guarantees
This clarity is valuable and helps set appropriate expectations for developers.
web_interface/templates/v3/base.html (1)
4820-4826: LGTM: widget scripts are loaded in dependency order.Registry and base classes load before widget implementations, and the
deferusage keeps the page non-blocking.web_interface/static/v3/js/widgets/plugin-loader.js (3)
1-17: LGTM!Good defensive initialization pattern - checking for the registry existence before attempting to extend it.
25-65: LGTM!The multiple path fallback pattern with proper error aggregation is well implemented. The warning when a script loads but doesn't register the widget is helpful for debugging plugin development issues.
101-126: LGTM!Good error handling - failures for individual widgets are logged but don't block loading of other widgets from the manifest.
web_interface/static/v3/js/widgets/checkbox-group.js (3)
1-17: LGTM!Consistent initialization pattern matching other widget files.
42-53: LGTM!Proper error handling for JSON parsing with a safe fallback to an empty array.
87-114: LGTM!Clean implementation that aggregates checked values and dispatches a properly structured
widget-changeevent.web_interface/static/v3/js/widgets/example-color-picker.js (2)
1-22: LGTM!Good documentation explaining how to use this as an example widget for plugin developers.
129-156: LGTM!Good validation in
setValueto reject invalid hex values, and appropriate null handling ingetValue.web_interface/static/v3/js/widgets/registry.js (5)
1-17: LGTM!Clean module structure with Map-based storage for widgets and handlers.
30-55: LGTM!Solid validation for required properties. Note that re-registering a widget with the same name silently overwrites the previous definition, which is reasonable for hot-reload scenarios.
101-131: LGTM!Robust container resolution with proper validation and error handling.
139-175: LGTM!Proper delegation to widget-specific getValue/setValue with appropriate fallback behavior and error handling.
177-217: LGTM!Useful cleanup methods and debug introspection for development.
web_interface/static/v3/js/widgets/custom-feeds.js (5)
1-17: LGTM!Consistent initialization pattern.
42-70: LGTM!Clean extraction of feed data from DOM with proper null handling for logo fields.
109-218: LGTM!Good use of DOM API for element creation, avoiding innerHTML with user data. Proper max items validation and required field attributes.
224-260: LGTM!Proper row removal with reindexing logic. The regex pattern
/\.\d+\./correctly handles the expected name format.
291-377: LGTM!Good DOM-based construction avoiding innerHTML with server response data. Proper error handling with user notification fallback.
web_interface/static/v3/js/widgets/file-upload.js (2)
13-61: Registry guard + widget registration look solid.Nice defensive check and clean registry integration.
306-327: No action required—hidden input ID suffix is already aligned.Templates consistently use
${fieldId}_images_datafor file upload widgets (matching file-upload.js), while other widgets use${fieldId}_datafor different field types. There is no suffix mismatch; uploads will persist correctly.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- base-widget.js: Fix escapeHtml to always escape (coerce to string first) - base-widget.js: Add sanitizeId helper for safe DOM ID usage - base-widget.js: Use DOM APIs in showError instead of innerHTML - checkbox-group.js: Normalize types in setValue for consistent comparison - custom-feeds.js: Implement setValue with full row creation logic - example-color-picker.js: Validate hex colors before using in style attributes - file-upload.js: Replace innerHTML with DOM creation to prevent XSS - file-upload.js: Preserve open schedule editors when updating image list - file-upload.js: Normalize types when filtering deleted files - file-upload.js: Sanitize imageId in openImageSchedule and all schedule handlers - file-upload.js: Fix max-files check order and use allowed_types from config - README.md: Add security guidance for ID sanitization in examples
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@scripts/update_plugin_repos.py`:
- Around line 19-40: Update load_workspace_plugins to open WORKSPACE_FILE with
explicit encoding (e.g., open(WORKSPACE_FILE, 'r', encoding='utf-8')) and wrap
the file read and json.load call in a try/except that catches FileNotFoundError,
PermissionError, and json.JSONDecodeError; for each catch, emit a clear
user-friendly message (via logging or print) that includes WORKSPACE_FILE and
the exception details, and return an empty plugins list or re-raise a wrapped
exception as appropriate. Ensure references to WORKSPACE_FILE, GITHUB_DIR, and
the plugins list remain intact so the subsequent loop over
workspace.get('folders', []) and creation of plugin_path using GITHUB_DIR /
plugin_name continues unchanged.
- Around line 49-75: The git fetch result isn't checked and the broad except
hides specific subprocess issues; change the first subprocess.run to store in a
distinct variable (e.g., fetch_result), verify fetch_result.returncode == 0
before continuing (on non-zero print fetch_result.stderr and return False),
avoid reusing the name "result" for both fetch and pull (use pull_result for the
git pull call and check pull_result.returncode and output), and replace the bare
"except Exception as e" with a specific catch like "except
(subprocess.SubprocessError, OSError) as e" so only relevant errors from
subprocess/os are handled; reference the existing git fetch subprocess.run, git
pull subprocess.run, branch_result/current_branch, and the try/except block in
scripts/update_plugin_repos.py.
In `@web_interface/static/v3/js/widgets/base-widget.js`:
- Around line 173-180: The notify method can throw when called without a type
because it calls type.toUpperCase(); update notify (and its fallback logging) to
guard against undefined/null type by normalizing type (e.g., default to 'info'
or cast to string) before calling toUpperCase(); locate the notify method and
its call to getNotificationFunction() in base-widget.js and ensure the fallback
console.log uses the normalizedType (or a safe conditional) instead of calling
toUpperCase() directly on the possibly undefined type.
In `@web_interface/static/v3/js/widgets/file-upload.js`:
- Around line 630-660: The template injects unsanitized fieldId into inline
onchange handlers (e.g., in scheduleContainer.innerHTML for elements with IDs
schedule_enabled_${sanitizedId} and schedule_mode_${sanitizedId}), allowing JS
injection; remove inline handlers and instead set safe data-* attributes (e.g.,
data-field-id, data-image-idx) on those elements when building innerHTML, then
after assigning scheduleContainer.innerHTML query for those IDs
(schedule_enabled_${sanitizedId}, schedule_mode_${sanitizedId},
schedule_options_${sanitizedId}) and attach event listeners with
addEventListener that call the existing functions
window.toggleImageScheduleEnabled and window.updateImageScheduleMode using the
values from the data attributes (fieldId, sanitizedId, imageIdx); ensure
sanitizedId is used in DOM IDs and do not interpolate raw fieldId into HTML
strings.
- Around line 520-535: The UI always shows a hardcoded "(PNG, JPG, GIF, BMP)"
even though hideUploadProgress reads allowedTypes from uploadConfig; update
hideUploadProgress to derive a user-friendly extension list from allowedTypes
(e.g., map MIME types like "image/png" -> "PNG" and handle common file
extensions) and inject that generated string into the dropZone.innerHTML instead
of the hardcoded text; keep the existing fallback to the default allowedTypes
array and ensure dropZone and fieldId are used as before.
In `@web_interface/static/v3/js/widgets/README.md`:
- Around line 349-411: The Color Picker example registered under
register('color-picker') uses fieldId directly in render, getValue, setValue and
handlers.onChange; replace those usages with a sanitized identifier (e.g., call
the documented sanitizeId(fieldId) once in render and store as sanitizedFieldId)
and use sanitizedFieldId when building element IDs and querySelector strings,
and also use the same sanitizedFieldId in getValue, setValue, and
handlers.onChange to match the documentation's sanitization pattern.
🧹 Nitpick comments (5)
scripts/update_plugin_repos.py (1)
8-12: Unused importos.The
osmodule is imported but never used in this script. Consider removing it.import json -import os import subprocess import sys from pathlib import Pathweb_interface/static/v3/js/widgets/base-widget.js (1)
51-58: Regex-based XSS prevention is easily bypassed.The script-tag regex is fragile and can be circumvented with variations like
<script >,<SCRIPT>, event handlers (onerror), or other injection vectors. Consider using a proper sanitization library (e.g., DOMPurify) or DOM-based sanitization if actual HTML sanitization is needed, or document that this is only a minimal safeguard.💡 Alternative approach using DOM-based sanitization
sanitizeValue(value) { - // Base implementation - widgets should override for specific needs - if (typeof value === 'string') { - // Basic XSS prevention - return value.replace(/<script\b[^<]*(?:(?!<\/script>)<[^<]*)*<\/script>/gi, ''); - } - return value; + // Base implementation - widgets should override for specific needs + if (typeof value === 'string') { + // Strip all HTML tags for basic XSS prevention + const div = document.createElement('div'); + div.textContent = value; + return div.textContent; + } + return value; }web_interface/static/v3/js/widgets/checkbox-group.js (1)
111-118: Minor inconsistency in event dispatch target.This widget dispatches
widget-changeon thehiddenInputelement, whileBaseWidget.triggerChangedispatches ondocument. Both work due to event bubbling, but consider aligning the pattern for consistency across widgets.web_interface/static/v3/js/widgets/file-upload.js (1)
488-492: Preserved schedule content restoration uses innerHTML.Line 490 restores
preservedScheduleContentviainnerHTML. While this preserves UX by keeping the schedule editor open, it could re-inject XSS if the schedule content was previously compromised. Consider rebuilding the schedule UI from data instead of preserving raw HTML.web_interface/static/v3/js/widgets/custom-feeds.js (1)
370-375: Regex for reindexing may match unintended positions.The pattern
/\.\d+\./on line 374 replaces any numeric segment in the name, which could incorrectly modify names likefeeds.v2.0.name(replacing2instead of0). Consider a more precise approach.🔧 Proposed fix: Match index at end of second-to-last segment
r.querySelectorAll('input, button').forEach(input => { const name = input.getAttribute('name'); if (name) { - // Replace pattern like "feeds.custom_feeds.0.name" with "feeds.custom_feeds.1.name" - input.setAttribute('name', name.replace(/\.\d+\./, `.${index}.`)); + // Replace index before final segment: "feeds.custom_feeds.0.name" -> "feeds.custom_feeds.1.name" + // Match only the last numeric segment followed by a dot and property name + input.setAttribute('name', name.replace(/\.(\d+)\.([^.]+)$/, `.${index}.$2`)); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
LEDMatrix.code-workspacescripts/update_plugin_repos.pyweb_interface/static/v3/js/widgets/README.mdweb_interface/static/v3/js/widgets/base-widget.jsweb_interface/static/v3/js/widgets/checkbox-group.jsweb_interface/static/v3/js/widgets/custom-feeds.jsweb_interface/static/v3/js/widgets/example-color-picker.jsweb_interface/static/v3/js/widgets/file-upload.js
✅ Files skipped from review due to trivial changes (1)
- LEDMatrix.code-workspace
🚧 Files skipped from review as they are similar to previous changes (1)
- web_interface/static/v3/js/widgets/example-color-picker.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
**/*.py: Prefer clear, readable code over clever optimizations (Simplicity First principle)
Make intentions clear through naming and structure (Explicit over Implicit principle)
Validate inputs and handle errors early (Fail Fast principle)
Use docstrings for classes and complex functions
Use PascalCase for class names (e.g.,NHLRecentManager)
Use snake_case for function and variable names (e.g.,fetch_game_data)
Use UPPER_SNAKE_CASE for constants (e.g.,ESPN_NHL_SCOREBOARD_URL)
Use leading underscore for private methods (e.g.,_fetch_data)
Use structured logging with context (e.g.,[NHL Recent]) for logging messages
Catch specific exceptions, not bareexcept:statements
Provide user-friendly error messages that explain what went wrong and potential solutions
Implement graceful degradation to continue operation when non-critical features fail
Use type hints for function parameters and return values
Validate required configuration fields on initialization
Provide sensible default values in code rather than in configuration files
Handle different deployment contexts with environment awareness in code
**/*.py: Code must run on Raspberry Pi, not Windows development machine
Optimize code for Raspberry Pi's limited RAM and CPU capabilities
Clean up resources regularly to manage memory effectively
Implement comprehensive logging for remote debugging on Raspberry Pi
Provide clear error messages for troubleshooting
Files:
scripts/update_plugin_repos.py
🧠 Learnings (1)
📚 Learning: 2025-12-27T19:16:24.801Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Install pre-push git hook from LEDMatrix repo to handle automatic version bumping: copy from `scripts/git-hooks/pre-push-plugin-version` to plugin repo `.git/hooks/pre-push`
Applied to files:
scripts/update_plugin_repos.py
🧬 Code graph analysis (1)
web_interface/static/v3/js/widgets/base-widget.js (2)
web_interface/static/v3/js/widgets/file-upload.js (10)
notifyFn(114-114)notifyFn(122-122)notifyFn(129-129)notifyFn(140-140)notifyFn(177-177)notifyFn(180-180)notifyFn(185-185)notifyFn(249-249)notifyFn(252-252)notifyFn(257-257)web_interface/static/v3/js/widgets/checkbox-group.js (1)
event(112-116)
🪛 Ruff (0.14.11)
scripts/update_plugin_repos.py
51-51: Starting a process with a partial executable path
(S607)
55-55: Starting a process with a partial executable path
(S607)
60-60: subprocess call: check for execution of untrusted input
(S603)
60-60: Starting a process with a partial executable path
(S607)
73-73: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
scripts/update_plugin_repos.py (1)
78-105: LGTM!The main function has clear flow: discover plugins, update each, and report results with appropriate exit codes. The summary output is helpful for users.
web_interface/static/v3/js/widgets/base-widget.js (1)
122-128: LGTM - escapeHtml now properly handles all input types.The function correctly coerces input to string before escaping, addressing the previous review concern.
web_interface/static/v3/js/widgets/checkbox-group.js (1)
60-79: LGTM - Type coercion issue addressed.The
setValuefunction now normalizes values to strings (line 67) and uses string comparison (line 74), resolving the previous type mismatch concern.web_interface/static/v3/js/widgets/file-upload.js (2)
96-143: LGTM - Max-file validation now runs after filtering.The validation logic correctly builds
validFilesfirst (lines 111-136), then checks the max files limit (lines 139-143), addressing the previous review concern.
237-247: LGTM - ID type normalization implemented.The comparison now uses
String()coercion on both sides (lines 240-243), resolving the previous type mismatch concern.web_interface/static/v3/js/widgets/custom-feeds.js (2)
78-93: LGTM - Defensive approach implemented for setValue.The function now throws an explicit error when required options are missing (lines 85-87), preventing silent data loss. The full implementation properly rebuilds rows from data.
436-494: LGTM - Safe DOM construction for logo cell.The logo upload handler uses programmatic DOM creation (
document.createElement,textContent,appendChild) instead of innerHTML, preventing XSS vulnerabilities.web_interface/static/v3/js/widgets/README.md (1)
314-328: LGTM - Security section now includes comprehensive identifier sanitization guidance.The section addresses the previous review concern by documenting the need to sanitize identifiers used in selectors and IDs, with specific examples and allowed character sets.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- scripts/update_plugin_repos.py: Add explicit UTF-8 encoding and proper error handling for file operations - scripts/update_plugin_repos.py: Fix git fetch/pull error handling with returncode checks and specific exception types - base-widget.js: Guard notify method against undefined/null type parameter - file-upload.js: Remove inline handlers from schedule template, use addEventListener with data attributes - file-upload.js: Update hideUploadProgress to show dynamic file types from config instead of hardcoded list - README.md: Update Color Picker example to use sanitized fieldId throughout
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web_interface/static/v3/js/widgets/README.md`:
- Around line 426-492: The Slider example uses fieldId directly in DOM IDs and
selectors (in render, getValue, setValue, and handlers.onChange) which breaks
the documented sanitization pattern; fix it by computing a sanitized identifier
(e.g., const safeId = sanitizeFieldId(fieldId) or similar helper used in the
Color Picker example) at the top of render/getValue/setValue and use safeId for
all generated IDs (#${safeId}_slider, #${safeId}_value) and query selectors and
event payloads so that every reference to the DOM uses the sanitized identifier
consistently.
🧹 Nitpick comments (4)
scripts/update_plugin_repos.py (3)
8-12: Remove unusedosimport.The
osmodule is imported but never used in the script.Proposed fix
import json -import os import subprocess import sys from pathlib import Path
19-50: Well-structured error handling.The function properly handles
FileNotFoundError,PermissionError, andjson.JSONDecodeErrorwith clear user-friendly messages, and uses explicit UTF-8 encoding. The plugin filtering logic is correct.Consider adding type hints per coding guidelines:
def load_workspace_plugins() -> list[dict]:
53-89: Solid git operation handling with proper error checks.The function correctly:
- Validates the
.gitdirectory exists before operations- Checks
fetch_result.returncodebefore proceeding to pull- Catches specific exceptions
(subprocess.SubprocessError, OSError)per guidelines- Provides clear status messages for all outcomes
The Ruff S607 warnings about partial executable paths (
git) are acceptable for developer utility scripts where git is expected to be in PATH.Consider adding type hints:
def update_repo(repo_path: Path) -> bool:web_interface/static/v3/js/widgets/file-upload.js (1)
557-563: Consider handling edge case for invalid byte values.The
formatFileSizefunction handles zero correctly, but negative orNaNbyte values would causeMath.logto returnNaN, resulting in an undefined size unit. This is unlikely in practice but could occur with corrupted data.🔧 Optional defensive check
window.formatFileSize = function(bytes) { - if (bytes === 0) return '0 B'; + if (!bytes || bytes <= 0) return '0 B'; const k = 1024; const sizes = ['B', 'KB', 'MB']; const i = Math.floor(Math.log(bytes) / Math.log(k)); - return Math.round(bytes / Math.pow(k, i) * 100) / 100 + ' ' + sizes[i]; + return Math.round(bytes / Math.pow(k, i) * 100) / 100 + ' ' + (sizes[i] || 'B'); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/update_plugin_repos.pyweb_interface/static/v3/js/widgets/README.mdweb_interface/static/v3/js/widgets/base-widget.jsweb_interface/static/v3/js/widgets/file-upload.js
🚧 Files skipped from review as they are similar to previous changes (1)
- web_interface/static/v3/js/widgets/base-widget.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
**/*.py: Prefer clear, readable code over clever optimizations (Simplicity First principle)
Make intentions clear through naming and structure (Explicit over Implicit principle)
Validate inputs and handle errors early (Fail Fast principle)
Use docstrings for classes and complex functions
Use PascalCase for class names (e.g.,NHLRecentManager)
Use snake_case for function and variable names (e.g.,fetch_game_data)
Use UPPER_SNAKE_CASE for constants (e.g.,ESPN_NHL_SCOREBOARD_URL)
Use leading underscore for private methods (e.g.,_fetch_data)
Use structured logging with context (e.g.,[NHL Recent]) for logging messages
Catch specific exceptions, not bareexcept:statements
Provide user-friendly error messages that explain what went wrong and potential solutions
Implement graceful degradation to continue operation when non-critical features fail
Use type hints for function parameters and return values
Validate required configuration fields on initialization
Provide sensible default values in code rather than in configuration files
Handle different deployment contexts with environment awareness in code
**/*.py: Code must run on Raspberry Pi, not Windows development machine
Optimize code for Raspberry Pi's limited RAM and CPU capabilities
Clean up resources regularly to manage memory effectively
Implement comprehensive logging for remote debugging on Raspberry Pi
Provide clear error messages for troubleshooting
Files:
scripts/update_plugin_repos.py
🧠 Learnings (1)
📚 Learning: 2025-12-27T19:16:24.801Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Install pre-push git hook from LEDMatrix repo to handle automatic version bumping: copy from `scripts/git-hooks/pre-push-plugin-version` to plugin repo `.git/hooks/pre-push`
Applied to files:
scripts/update_plugin_repos.py
🪛 Ruff (0.14.11)
scripts/update_plugin_repos.py
61-61: Starting a process with a partial executable path
(S607)
69-69: Starting a process with a partial executable path
(S607)
74-74: subprocess call: check for execution of untrusted input
(S603)
74-74: Starting a process with a partial executable path
(S607)
🔇 Additional comments (13)
scripts/update_plugin_repos.py (1)
92-119: LGTM!The main function correctly orchestrates the workflow with proper exit codes (0 for success, 1 for failures or no plugins found) and provides clear progress feedback to the user.
web_interface/static/v3/js/widgets/README.md (4)
1-113: Documentation structure looks good.The overview, available core widgets documentation, and usage instructions are well-organized and provide clear guidance for plugin developers.
139-220: Custom widget example demonstrates proper sanitization.The example correctly shows sanitization of
fieldIdinrender,getValue, andsetValuemethods, and includes a reusablesanitizeIdhelper. This addresses the security concerns from previous reviews.
312-328: Security best practices section is comprehensive.The security guidance now properly covers HTML escaping, input validation, value sanitization, and importantly, identifier sanitization with detailed examples for CSS selectors and HTML attributes.
349-424: Color Picker example correctly applies sanitization.The example now properly sanitizes
fieldIdin all methods (render,getValue,setValue) before using it in DOM IDs and selectors, aligning with the documented security best practices.web_interface/static/v3/js/widgets/file-upload.js (8)
10-61: Widget registration structure is well-organized.The IIFE pattern with registry check, placeholder render for server-side rendered widgets, and delegation to window-scoped functions maintains backwards compatibility while integrating with the new registry system.
96-197: File validation and upload logic is correct.The implementation properly validates file types against
allowedTypesfrom config, checks max files after filtering valid files, and handles upload errors appropriately. The shadowedcurrentFilesvariable on line 171 is harmless since the re-fetched value is intentionally fresher.
237-247: ID type normalization implemented correctly.The filter now properly normalizes both
fileIdandfile.id/file.category_nameto strings before comparison, ensuring deletion works regardless of whether IDs arrive as numbers or strings.
267-307: Config retrieval logic handles common cases.The schema traversal logic works for the documented widget patterns. The underscore-to-dot conversion (line 274) assumes field IDs use underscores to represent dot-separated paths, which aligns with the server-side rendering conventions.
332-498: DOM creation approach properly mitigates XSS risks.The implementation uses programmatic DOM element creation for user-controlled data (filenames, paths, IDs), sanitizes IDs for element attributes, and attaches event listeners via
addEventListenerinstead of inline handlers. The schedule preservation correctly restores self-generated HTML.
520-550: File type display now reflects configuration.The implementation dynamically generates the extension list from
allowedTypeswith a sensible mapping and deduplication, addressing the previous concern about hardcoded file type text.
615-754: Schedule editor properly implements security mitigations.The implementation sanitizes
imageIdfor DOM lookups (line 621), usesescapeHtmlfor data attributes containingfieldId, and attaches event listeners programmatically instead of using inline handlers. This addresses the previous concerns about XSS and selector injection.
837-963: Schedule handlers consistently apply ID sanitization.All schedule update handlers properly sanitize
imageIdbefore DOM queries, maintaining consistency withupdateImageList's ID generation. The schedule preservation logic inupdateImageList(lines 345-365, 488-492) ensures the UI doesn't collapse during edits.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add sanitizeId helper to Slider example render, getValue, and setValue methods - Use sanitizedFieldId for all DOM IDs and query selectors - Maintain consistency with Color Picker example pattern
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.