Fix/font display and config api errors#158
Conversation
- Fix font catalog display error where path.startsWith fails (path is object, not string) - Update save_main_config to use error_response() helper - Improve save_raw_main_config error handling consistency - Add proper error codes and traceback details to API responses
- Use window object to store global font variables - Check if script has already loaded before declaring variables - Update both window properties and local references on assignment - Fixes 'Identifier fontCatalog has already been declared' error
- Wrap entire script in IIFE that only runs once - Check if script already loaded before declaring variables/functions - Expose initializeFontsTab to window for re-initialization - Prevents 'Identifier has already been declared' errors on HTMX reload
- Exempt save_raw_main_config, save_raw_secrets_config, and save_main_config from CSRF - These endpoints are called via fetch from JavaScript and don't include CSRF tokens - Fixes 500 error when saving config via raw JSON editor
- Exempt execute_system_action from CSRF - Fixes 500 error when using system action buttons (restart display, restart Pi, etc.) - These endpoints are called via HTMX and don't include CSRF tokens
- Add before_request handler to exempt all api_v3.* endpoints - All API endpoints are programmatic (HTMX/fetch) and don't include CSRF tokens - Prevents future CSRF errors on any API endpoint - Cleaner than exempting individual endpoints
- CSRF is designed for internet-facing apps to prevent cross-site attacks - For local-only Raspberry Pi app, threat model is different - All endpoints were exempted anyway, so it wasn't protecting anything - Forms use HTMX without CSRF tokens - If exposing to internet later, can re-enable with proper token implementation
- Only prefix with 'assets/fonts/' if path is a bare filename - If path starts with '/' (absolute) or 'assets/' (already prefixed), use as-is - Fixes double-prefixing when get_fonts_catalog returns relative paths like 'assets/fonts/press_start.ttf'
… on HTMX reload - Remove fontsTabInitialized check that prevented re-initialization on HTMX content swap - The window._fontsScriptLoaded guard is sufficient to prevent function redeclaration - Allow initializeFontsTab() to run on each HTMX swap to attach listeners to new DOM elements - Fixes fonts UI breaking after HTMX reload (buttons, upload dropzone, etc. not working)
… config - Add _is_field_required() helper to check if fields are required in schema - Update _parse_form_value_with_schema() to preserve empty strings for optional string fields - Fixes 400 error when saving MQTT plugin config with empty username/password - Resolves validation error: 'Expected type string, got NoneType'
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes comprise three distinct areas: CSRF protection removal with local security rationale documentation in the app initialization, standardized API error handling with new schema-aware form parsing helpers for optional field detection, and enhanced fonts template with HTMX reload guards that expose font state and initialization functions to the global scope for re-initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant HTMX as HTMX Framework
participant Server
participant Fonts as Fonts Script
User->>HTMX: Trigger fonts tab reload
HTMX->>Server: Request fonts partial HTML
Server->>HTMX: Return fonts.html content
HTMX->>User: Swap DOM content
rect rgb(200, 220, 255)
Note over Fonts: Load Guard Check
Fonts->>Fonts: Check if window.fontCatalog exists?
Fonts->>Fonts: If exists, skip re-declaration
end
rect rgb(220, 240, 200)
Note over Fonts: Initialization / Re-initialization
Fonts->>Fonts: Access/reset window.fontCatalog, <br/>window.fontTokens, etc.
Fonts->>Server: Fetch font data via API
Server->>Fonts: Return font list
Fonts->>Fonts: Populate UI & sync globals
Fonts->>Fonts: Register HTMX listener
end
rect rgb(240, 220, 200)
Note over Fonts: Ready for Next Reload
HTMX->>Fonts: htmx:afterSwap event
Fonts->>Fonts: Call initializeFontsTab()
Fonts->>Fonts: Re-initialize with existing globals
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (4)
web_interface/blueprints/api_v3.py (1)
635-640: Use explicit conversion flag in error message f-stringStructured error response here looks good and consistent with the rest of the API, but Ruff’s RUF010 warning is right:
str(e)inside an f-string is redundant. Prefer an explicit conversion flag for clarity:Proposed tweak
- return error_response( - ErrorCode.CONFIG_SAVE_FAILED, - f"Error saving configuration: {str(e)}", - details=traceback.format_exc(), - status_code=500 - ) + return error_response( + ErrorCode.CONFIG_SAVE_FAILED, + f"Error saving configuration: {e!s}", + details=traceback.format_exc(), + status_code=500 + )web_interface/app.py (1)
29-36: CSRF disabled: document deployment assumptions and clean up stale exemption commentSetting
csrf = Nonewith an inline threat-model explanation is clear for a local-only Pi install, and all later CSRF uses are guarded withif csrf:, so this won’t break anything at runtime.Two follow‑ups to consider:
- Make the “local‑only” assumption more explicit/configurable, e.g. via an env flag like
LEDMATRIX_ENABLE_CSRF, so users exposing the UI over the internet can re‑enable protection without code edits.- The comment at Line 530 (“api_v3 blueprint is exempted above after registration”) is now stale: there’s no
csrf.exempt(api_v3)in this file anymore. Either remove or update it to avoid confusion.web_interface/templates/v3/partials/fonts.html (2)
193-331: HTMX reload guard and global font state pattern look solid; double‑checkbaseUrlavailabilityThe outer IIFE with
window._fontsScriptLoadedpluswindow.initializeFontsTabexport is a good pattern for HTMX: it prevents duplicate handler registration while still allowing re‑initialization on each content swap. Usingwindow.fontCatalog/fontTokens/fontOverrides/selectedFontFilesas the single source of truth, with local aliases inside the IIFE, also makes re‑entry safe.One thing to verify: helpers like
addFontOverride,deleteFontOverride, anduploadSelectedFontsstill usebaseUrlin theirfetch()calls, but in this filebaseUrlis only declared as a localconstinsideloadFontData. If there isn’t a separate globalbaseUrldefined elsewhere (e.g., in the base layout), those calls will throw at runtime.If you want to keep this self‑contained, consider either:
- Promoting
baseUrlto the IIFE scope (e.g.,var baseUrl = window.location.origin;once near the top), or- Dropping
baseUrlentirely and using absolute paths (fetch('/api/v3/fonts/overrides', …)) everywhere, as these already work correctly under HTMX.
520-526: Improved font path handling avoids double prefixes and supports object catalog entriesThe updated
updateAvailableFontsDisplaylogic correctly:
- Accepts either a bare string path or an object with a
pathfield (as returned by/api/v3/fonts/catalog), and- Only prefixes with
"assets/fonts/"when the path is a bare filename (no leading/and no existingassets/prefix), leaving absolute or already‑prefixed paths unchanged.This should fix the
assets/fonts/assets/fonts/...issue without breaking existing callers. The only minor edge case is whenfontPathis empty, in which case the display showsassets/fonts/for that entry — acceptable for now, but easy to tighten later if needed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web_interface/app.pyweb_interface/blueprints/api_v3.pyweb_interface/templates/v3/partials/fonts.html
🧰 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:
web_interface/blueprints/api_v3.pyweb_interface/app.py
🪛 Ruff (0.14.10)
web_interface/blueprints/api_v3.py
637-637: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (3)
web_interface/blueprints/api_v3.py (2)
686-700: Nice move to centralized, code-based error responses for raw main configSwitching both
ConfigErrorand generic exception paths toerror_responsewithCONFIG_SAVE_FAILEDandUNKNOWN_ERRORcodes, plus tracebackdetailsand optionalcontext, gives callers a much more debuggable and uniform API surface. No functional issues spotted here.
2917-2980: Schema-aware required check and optional-string handling look correct (with clear scope)The new
_is_field_required(key_path, schema)and the updated empty-value branch in_parse_form_value_with_schemacorrectly:
- Treat fields listed in a parent object’s
requiredarray as required, both at the top level and one nesting level down.- Preserve
""for optionaltype: "string"fields while still mapping empty values for arrays/objects to[]/{}and everything else toNone.This should fix the “can’t clear optional text field” behavior without regressing validation for genuinely required fields (those still become
Noneand get caught by schema validation).Note this helper intentionally doesn’t handle more complex schema constructs (e.g., arrays of objects with their own
required,oneOf/allOftrees, ortype: ["string", "null"]unions); that’s fine if current plugin schemas stick to simpleproperties+requiredpatterns, but worth keeping in mind if schemas grow more complex later.If you start to lean on array-of-object
requiredsemantics in schemas, you may want a follow-up that teaches_get_schema_property/_is_field_requiredhow to walk throughitemstoo.web_interface/templates/v3/partials/fonts.html (1)
428-436: Synchronizing window‑level font state with local aliases is correct and avoids stale referencesWriting
catalogData/tokensData/overridesDataintowindow.fontCatalog/fontTokens/fontOverrides, then immediately rebinding the localfontCatalog/fontTokens/fontOverridesaliases ensures all subsequent code (including logs and UI updates) sees the latest state and that re‑initializations after HTMX swaps don’t end up reading stale objects.This is a clean way to share state across partial reloads without re‑declaring functions.
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.