fix(config): Add defaults to schemas and fix config validation issues#153
fix(config): Add defaults to schemas and fix config validation issues#153ChuckBuilds merged 12 commits intomainfrom
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'
- Updated merge_with_defaults to replace None values with defaults - Fixed form processing to skip empty optional fields without defaults - Added script to automatically add defaults to all plugin config schemas - Added defaults to 89 fields across 10 plugin schemas - Prevents validation errors from None values in configs Changes: - schema_manager.py: Enhanced merge_with_defaults to replace None with defaults - api_v3.py: Added _SKIP_FIELD sentinel to skip optional fields without defaults - add_defaults_to_schemas.py: Script to add sensible defaults to schemas - Plugin schemas: Added defaults for number, boolean, and array fields
- Fixed handleConfigSave to check xhr.status instead of event.detail.successful - With hx-swap="none", HTMX doesn't set event.detail.successful - Now properly detects successful saves (status 200-299) and stops spinner - Improved error message extraction from API responses - Also fixed handleToggleResponse for consistency
📝 WalkthroughWalkthroughThis PR updates the mqtt-notifications plugin submodule, introduces a utility script for injecting default values into plugin configuration schemas, refactors the schema manager to perform deep None-aware merging, disables CSRF protection in the web interface, enhances API v3 config endpoints with schema-aware form parsing and improved error handling, and refactors font initialization to support HTMX content reloads with global state management. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Web Client
participant API as API v3 Handler
participant SchemaMgr as Schema Manager
participant ConfigFile as Config File
User->>Client: Submit config form
Client->>API: POST form data (with parsed values)
rect rgb(220, 240, 255)
Note over API: Schema-Aware Parsing
API->>API: _parse_form_value_with_schema()<br/>for each field
API->>API: Determine required,<br/>skip optional unset fields
end
rect rgb(220, 250, 235)
Note over API,SchemaMgr: Deep Merge with Defaults
API->>SchemaMgr: merge_with_defaults()<br/>user config + defaults
SchemaMgr->>SchemaMgr: deep_merge()<br/>recursive merge with<br/>None-aware replacement
SchemaMgr-->>API: merged config
end
rect rgb(240, 240, 250)
Note over API: Validation & Secrets
API->>API: Validate against schema
API->>API: Separate secrets from config
API->>ConfigFile: Save main config
API->>ConfigFile: Save secrets config
end
alt Success (2xx)
API-->>Client: 200 OK + message
Client->>User: Show success notification
else Validation Error
API-->>Client: error_response with<br/>validation details
Client->>User: Show validation errors
else Save Failed
API-->>Client: CONFIG_SAVE_FAILED<br/>error code
Client->>User: Show error details
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web_interface/templates/v3/partials/fonts.html (2)
371-405:baseUrlis function-scoped but used in three other functions; will throwReferenceErrorat runtime
loadFontData()definesconst baseUrl = window.location.origin;(line 400) within its function scope, butaddFontOverride()(line 560),deleteFontOverride()(line 592), anduploadSelectedFonts()(line 783) reference${baseUrl}/api/v3/...without definingbaseUrlin their own scope. With the IIFE wrapper around the entire script, there is no globalbaseUrlfallback, so these functions will hit aReferenceErroron first use.Fix by either:
- Hoisting
baseUrlto IIFE scope (near the top of the function wrapper) and removing the inner declaration inloadFontData, or- Adding
const baseUrl = window.location.origin;locally at the top of each of the three functions.
586-608:deleteFontOverrideis inaccessible from inlineonclickhandlers due to IIFE scopingThe outer IIFE (lines 194–827) encapsulates all function declarations, but only
initializeFontsTabis exposed to the window object (line 330). ThedisplayCurrentOverridesfunction renders buttons withonclick="deleteFontOverride('...')"(line 638), which will fail with aReferenceErrorat runtime sincewindow.deleteFontOverrideis undefined.Fix by exposing the function:
function deleteFontOverride(elementKey) { ... } +window.deleteFontOverride = deleteFontOverride;Or refactor to remove inline
onclickand attach event listeners in JavaScript instead.web_interface/blueprints/api_v3.py (1)
3617-3825:normalize_config_valuesusesloggerbefore it's guaranteed to be definedInside
save_plugin_config, the nestednormalize_config_valuesfunction (lines 3617–3820) callslogger.warning(...)at line 3707, butloggeris only conditionally defined:
- Lines 3289 and 3309: logger is created only if array fields exist in the form data branch (lines 3224+)
- For JSON requests (lines 3216–3223), the entire form data branch is skipped, so logger is never defined
- Line 3841: logger is defined AFTER the
normalize_config_valuescall at line 3823When
normalize_config_valuesis called at line 3823 (which executes for both JSON and form requests) and the normalization path hits thelogger.warningfallback at line 3707, you risk aNameErrorif logger doesn't exist.Initialize logger inside
normalize_config_valuesto make it self-contained:def normalize_config_values(config, schema_props, prefix=''): """Recursively normalize config values based on schema types""" + import logging + logger = logging.getLogger(__name__) + if not isinstance(config, dict) or not isinstance(schema_props, dict): return config
🧹 Nitpick comments (8)
web_interface/app.py (1)
526-531: Stale/misleading comment.The comment on line 530 states "api_v3 blueprint is exempted above after registration", but since
csrf = Noneon line 36, this exemption block never executes. The comment is misleading about what actually happens.Consider either:
- Removing the comment since CSRF is disabled entirely, or
- Updating the comment to clarify that CSRF exemption is unnecessary because CSRF protection is disabled
🔎 Suggested fix
# Exempt SSE streams from CSRF and add rate limiting if csrf: csrf.exempt(stream_stats) csrf.exempt(stream_display) csrf.exempt(stream_logs) - # Note: api_v3 blueprint is exempted above after registration if limiter:src/plugin_system/schema_manager.py (1)
431-445: Consider if the second pass is necessary.The
deep_mergefunction already handlesNonereplacement (lines 419-421). Thereplace_none_with_defaultspass appears to be a safety net for edge cases.This is acceptable as defensive coding, but if performance becomes a concern with large configs, you could verify whether this second pass ever actually replaces any values. If not, it could be removed.
scripts/add_defaults_to_schemas.py (2)
123-136: Fix type hint for mutable default argument.The type hint
modified: List[str] = Noneis technically incorrect per PEP 484. While the None-check pattern at lines 135-136 correctly handles the mutable default argument issue, the type hint should reflect thatNoneis an acceptable value.🔎 Suggested fix
-def add_defaults_recursive(schema: Dict[str, Any], path: str = "", modified: List[str] = None) -> bool: +def add_defaults_recursive(schema: Dict[str, Any], path: str = "", modified: Optional[List[str]] = None) -> bool:
178-198: Minor cleanup: redundant f-string and broad exception.Two minor issues flagged by static analysis:
Line 181: Catching bare
Exceptionis acceptable for a utility script that should continue processing other files, but you could catch(json.JSONDecodeError, OSError)for more precise error handling.Line 197: The f-string has no placeholders - remove the
fprefix.🔎 Suggested fix
try: with open(schema_path, 'r', encoding='utf-8') as f: schema = json.load(f) - except Exception as e: + except (json.JSONDecodeError, OSError) as e: print(f" Error reading schema: {e}") return False modified_fields = [] changes_made = add_defaults_recursive(schema, modified=modified_fields) if changes_made: # Write back with pretty formatting with open(schema_path, 'w', encoding='utf-8') as f: json.dump(schema, f, indent=2, ensure_ascii=False) f.write('\n') # Add trailing newline print(f" ✓ Modified {len(modified_fields)} fields") return True else: - print(f" ✓ No changes needed") + print(" ✓ No changes needed") return Falseweb_interface/templates/v3/base.html (2)
4212-4258: Config save now keying off HTTP status looks correct; consider small defensive hardeningSwitching to
xhr.statusto determine success should resolve the hx-swap="none" spinner issue and is more robust than relying onevent.detail.successful. The JSON/error parsing and validation error surfacing also look good.Two minor, non‑blocking suggestions:
- Add a guard for unexpected call sites so this doesn’t throw if
event.detailorevent.detail.xhris ever missing (e.g., a non‑HTMX event or a future refactor):Optional defensive guard
- window.handleConfigSave = function(event, pluginId) { - const btn = event.target.querySelector('[type=submit]'); + window.handleConfigSave = function(event, pluginId) { + if (!event || !event.detail || !event.detail.xhr) { + console.error('handleConfigSave called without HTMX xhr detail'); + return; + } + const btn = event.target.querySelector('[type=submit]'); @@ - const xhr = event.detail.xhr; + const xhr = event.detail.xhr;
- When JSON parsing fails (non‑JSON error responses), consider including the HTTP status code in
errorMessageso users see at least “Failed to save configuration (500 Internal Server Error)” instead of a generic message.
4265-4312: Toggle handler’s status-based logic is solid; mirror config-save defensivenessUsing
xhr.statushere as well is consistent withhandleConfigSaveand should behave correctly forhx-swap="none"responses while keeping the UI in sync and reverting the checkbox on failure.As with
handleConfigSave, you may want to:
- Guard against
event.detail/event.detail.xhrbeing absent to avoid runtime errors if this ever gets wired to a non‑HTMX event.- Optionally append the HTTP status to
errorMessagewhen JSON parsing fails so the user gets slightly more actionable feedback.These are polish-level improvements; the current change is otherwise sound.
web_interface/blueprints/api_v3.py (2)
635-640: Use oferror_responsehere looks good; Ruff RUF010 aboutstr(e)is optionalThe updated
save_main_configexception handler now consistently returnserror_responsewithErrorCode.CONFIG_SAVE_FAILED, which aligns with the rest of the API error contract and includes traceback details.Ruff’s RUF010 hint on Line 637 is purely stylistic (
f"... {str(e)}"vsf"... {e!s}"). If you’re enforcing that rule repo‑wide, consider changing to:f"Error saving configuration: {e!s}",Otherwise this can be safely ignored.
1917-1953:_is_field_requiredworks for object properties; doesn’t yet handle array item pathsThe helper correctly inspects
requiredat the root and at nested object levels via_get_schema_property, which is sufficient for the form field paths you’re currently passing ("mqtt.username", etc.).Note that it won’t detect required-ness for properties inside arrays-of-objects (e.g. something like
"servers.0.host"or a future dot-notation for array items). If you ever surface per‑item fields in forms, you’ll likely need to extend_get_schema_property/_is_field_requiredto walk throughitemsschemas as well.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
plugins/mqtt-notificationsscripts/add_defaults_to_schemas.pysrc/plugin_system/schema_manager.pyweb_interface/app.pyweb_interface/blueprints/api_v3.pyweb_interface/templates/v3/base.htmlweb_interface/templates/v3/partials/fonts.html
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/app.pyscripts/add_defaults_to_schemas.pysrc/plugin_system/schema_manager.pyweb_interface/blueprints/api_v3.py
**/*manager*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
All sports managers should implement the BaseManager pattern with
__init__(self, config, display_manager, cache_manager),update()for fetching/processing data, anddisplay(self, force_clear=False)for rendering
Files:
src/plugin_system/schema_manager.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
All sports, weather, and stock managers should be organized in src/ directory
src/**/*.py: Use rpi-rgb-led-matrix library for hardware control on Raspberry Pi
Use cache_manager.py for data persistence and caching
Implement non-blocking data fetching using background services
Minimize unnecessary redraws to optimize LED matrix display performance
Implement health checks and status reporting
Provide web interface for remote configuration and monitoring on Raspberry Pi
Files:
src/plugin_system/schema_manager.py
🧠 Learnings (2)
📚 Learning: 2025-12-27T19:16:34.369Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursor/rules/coding-standards.mdc:0-0
Timestamp: 2025-12-27T19:16:34.369Z
Learning: Applies to **/*.py : Provide sensible default values in code rather than in configuration files
Applied to files:
scripts/add_defaults_to_schemas.py
📚 Learning: 2025-12-27T19:16:42.155Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursor/rules/configuration-management.mdc:0-0
Timestamp: 2025-12-27T19:16:42.155Z
Learning: Applies to src/*.py : Provide sensible defaults in code, not just config
Applied to files:
scripts/add_defaults_to_schemas.py
🧬 Code graph analysis (1)
src/plugin_system/schema_manager.py (1)
web_interface/blueprints/api_v3.py (1)
deep_merge(2821-2834)
🪛 Ruff (0.14.10)
scripts/add_defaults_to_schemas.py
123-123: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
181-181: Do not catch blind exception: Exception
(BLE001)
197-197: f-string without any placeholders
Remove extraneous f prefix
(F541)
web_interface/blueprints/api_v3.py
637-637: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (12)
plugins/mqtt-notifications (1)
1-1: Remove this comment - the submodule reference is valid.The mqtt-notifications submodule update (commit
5c23a842d8efc6e1253bc3dfc67c65ce37a193cc) is already at the correct HEAD of the main branch in the configured repository (ChuckBuilds/ledmatrix-mqtt-notifications). No compatibility concerns exist.Likely an incorrect or invalid review comment.
src/plugin_system/schema_manager.py (1)
394-446: Well-structured deep merge with None handling.The implementation correctly:
- Starts with a deep copy of defaults as the base
- Recursively merges user config, preserving user values
- Replaces
Nonevalues with defaults where available- Uses deep copies for mutable types to avoid shared references
scripts/add_defaults_to_schemas.py (3)
15-80: Sensible default value logic.The heuristics for determining default values are well-thought-out:
- Booleans →
False(safe default)- Numbers/integers →
minimumconstraint if available, otherwise computed value- Strings → Only enum values get defaults (avoids problematic empty strings)
- Arrays/objects → Empty containers
This aligns with the coding guideline to "provide sensible default values in code." Based on learnings, this approach is consistent with project standards.
98-106: Good security practice for sensitive fields.Correctly skips adding defaults for:
- Fields marked with
x-secret- Fields containing sensitive keywords (key, password, secret, token, auth, credential)
This prevents the script from accidentally providing defaults for credentials that should be user-supplied.
201-230: Clean utility script implementation.The main function properly:
- Uses
pathlibfor cross-platform path handling- Provides clear console output with progress and summary
- Handles missing plugins directory gracefully
web_interface/blueprints/api_v3.py (4)
686-700: Nice upgrade to structured errors insave_raw_main_configThe new handling for
ConfigErrorvs generic exceptions cleanly routes failures througherror_responsewithCONFIG_SAVE_FAILEDandUNKNOWN_ERRORplusdetailsand optionalconfig_pathcontext. This should make frontend handling and debugging much easier without changing behavior for successful saves.
2954-2996: Empty-field handling now matches “skip optional numeric fields” goalThe changes in
_parse_form_value_with_schemaforvalue is None/ empty string achieve:
- Arrays →
[]- Objects →
{}- Optional strings →
""(instead ofNone)- Numeric fields:
- If default present → use default
- If not required and no default →
_SKIP_FIELDso existing config/defaults remain- If required and empty →
Noneso schema validation can fail loudlyThis matches the PR intent to avoid
Nonein optional number fields while preserving strictness for required fields. Looks good.
3078-3105:_set_nested_valuecorrectly ignores_SKIP_FIELDsentinelsEarly‑returning when
value is _SKIP_FIELDcleanly prevents accidental writes of “empty optional” fields into the config while leaving the rest of the nested-set logic unchanged. This dovetails well with the new_parse_form_value_with_schemabehavior.
3287-3314: Array-index field combining respects_SKIP_FIELDWhen collapsing indexed fields like
foo.0,foo.1into a single array value, you’re now only calling_set_nested_valueifparsed_value is not _SKIP_FIELD. That ensures optional numeric arrays with no defaults can be left untouched rather than forced toNoneor an empty list. The same pattern in the non‑indexed field loop keeps behavior consistent.No functional issues here.
web_interface/templates/v3/partials/fonts.html (3)
193-220: HTMX script-load guard is a solid approachWrapping the fonts script in an IIFE with
window._fontsScriptLoadedand exposingwindow.initializeFontsTabgives you idempotent behavior on HTMX swaps without redeclaring everything. The delayed re‑init on subsequent loads (50 ms timeout) is also reasonable to ensure the DOM is present.No issues here.
428-437: Good: global + local font state kept in sync after API loadUpdating both
window.fontCatalog/fontTokens/fontOverridesand then refreshing the localfontCatalog/fontTokens/fontOverridesreferences ensures:
- Other code that relies on the global window state sees fresh data.
- The rest of this module continues to use the local variables without stale references.
This matches the new global-state pattern introduced at the top of the script.
520-526: Font path handling correctly distinguishes bare filenames vs full pathsThe updated
updateAvailableFontsDisplaynow treatsfontCatalogvalues as either:
- A string path, or
- An object with a
.pathproperty,and only prefixes
"assets/fonts/"when the path is a bare filename (no/and not already starting withassets/or/). This avoids double‑prefixing while still normalizing simple filenames.Looks correct and should work with the new catalog format exposed by the backend.
1 participant
Summary
This PR fixes configuration validation issues and ensures configs never start with None values.
Changes
1. Fixed None Value Handling
merge_with_defaultsinschema_manager.pyto replace None values with defaults2. Fixed Form Processing
_SKIP_FIELDsentinel to skip empty optional fields without defaults3. Added Defaults to Plugin Schemas
4. Fixed Save Button Spinner
handleConfigSaveto check HTTP status code instead ofevent.detail.successfulhx-swap="none", HTMX doesn't setevent.detail.successfulFiles Changed
src/plugin_system/schema_manager.py- Enhanced merge_with_defaultsweb_interface/blueprints/api_v3.py- Fixed form processing with _SKIP_FIELDweb_interface/templates/v3/base.html- Fixed save button handlerscripts/add_defaults_to_schemas.py- New script to add defaults to schemasTesting
Summary by CodeRabbit
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.