fix(web): Resolve font display errors and config API CSRF issues #152
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
📝 WalkthroughWalkthroughRemoves CSRF setup from the Flask app, standardizes API v3 error handling to use a unified Changes
Sequence Diagram(s)(silently skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_interface/templates/v3/partials/fonts.html (1)
410-415:baseUrlis undefined in multiple fetch calls (runtime ReferenceError)
baseUrlis declared as a localconstinsideloadFontData, but it’s used in other functions (addFontOverride,deleteFontOverride,uploadSelectedFonts) that don’t see that binding. Calling any of these will throwReferenceError: baseUrl is not defined, breaking overrides and uploads.Define
baseUrlonce at the top of the IIFE and reuse it everywhere, removing the innerconst:Proposed fix
(function() { + // Base URL for API calls (works under HTMX reloads) + const baseUrl = window.location.origin; @@ - // Use absolute URLs to ensure they work when loaded via HTMX - const baseUrl = window.location.origin; const [catalogRes, tokensRes, overridesRes] = await Promise.all([ fetch(`${baseUrl}/api/v3/fonts/catalog`), fetch(`${baseUrl}/api/v3/fonts/tokens`), fetch(`${baseUrl}/api/v3/fonts/overrides`) ]);No other changes are needed to the
fetch(${baseUrl}/...` calls below.Also applies to: 566-572, 598-600, 789-792
🧹 Nitpick comments (3)
web_interface/blueprints/api_v3.py (1)
630-640: Standardized error response looks good; tweak f-string per RuffThe switch to
error_responsewithErrorCode.CONFIG_SAVE_FAILEDand traceback details is consistent and solid. The only nit isf"Error saving configuration: {str(e)}"inside the f-string, which triggers Ruff RUF010; you can drop thestr()and rely on implicit conversion (or use!s):- error_msg = f"Error saving config: {str(e)}\n{traceback.format_exc()}" + error_msg = f"Error saving config: {e}\n{traceback.format_exc()}" @@ - 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}", + details=traceback.format_exc(), + status_code=500 + )web_interface/app.py (2)
29-36: CSRF disabled: validate threat model and deployment assumptionsExplicitly disabling CSRF (
csrf = None) for a “local-only” app is acceptable only if the UI truly never leaves a trusted network (no port-forwarding, reverse proxy, or public exposure). If there’s any chance this gets exposed beyond a private LAN, you’ll want a follow-up issue to reintroduce CSRF and wire tokens into HTMX forms as your comment suggests.
525-531: Remove dead CSRF exemption block for clarityWith
csrf = None, theif csrf:block exempting SSE streams is now dead code, and the inline note about the api_v3 blueprint being exempted is stale. Consider deleting this block and moving the comment near the CSRF section to avoid future confusion.Suggested minimal cleanup
-# 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: +if limiter: limiter.limit("20 per minute")(stream_stats) limiter.limit("20 per minute")(stream_display) limiter.limit("20 per minute")(stream_logs)
📜 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 (1)
web_interface/blueprints/api_v3.py (1)
675-701: ConfigError vs UNKNOWN_ERROR flow is correct and consistentThe new
error_responsebranches forConfigError(includingconfig_pathin both message andcontext) vs generic exceptions withErrorCode.UNKNOWN_ERRORare well-structured and align with the helper’s usage elsewhere. No functional issues from this change.
- 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'
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web_interface/templates/v3/partials/fonts.html (2)
570-570:baseUrlis undefined, causing ReferenceError.
baseUrlis defined locally inloadFontData()(line 410) but is referenced here inaddFontOverride(). This will throw a ReferenceError at runtime.Proposed fix
Define
baseUrlat the top of the IIFE so all functions can access it:// Create references that can be reassigned var fontCatalog = window.fontCatalog; var fontTokens = window.fontTokens; var fontOverrides = window.fontOverrides; var selectedFontFiles = window.selectedFontFiles; var fontsTabInitialized = window.fontsTabInitialized; + var baseUrl = window.location.origin;Then remove the local declaration in
loadFontData()at line 410:try { // Use absolute URLs to ensure they work when loaded via HTMX - const baseUrl = window.location.origin; const [catalogRes, tokensRes, overridesRes] = await Promise.all([Also applies to: 602-602, 793-793
728-728: Local reassignment doesn't updatewindow.selectedFontFiles.Line 220 creates a local variable pointing to
window.selectedFontFiles, but reassigningselectedFontFileshere only updates the local reference, leavingwindow.selectedFontFilespointing to the old array. This creates state inconsistency.Proposed fix
Update both the window property and the local reference:
- selectedFontFiles = validFiles; + window.selectedFontFiles = validFiles; + selectedFontFiles = window.selectedFontFiles; showUploadForm();Apply the same fix at line 758:
- selectedFontFiles = []; + window.selectedFontFiles = []; + selectedFontFiles = window.selectedFontFiles; document.getElementById('upload-form').style.display = 'none';Also applies to: 758-758
♻️ Duplicate comments (1)
web_interface/templates/v3/partials/fonts.html (1)
223-228: ThefontsTabInitializedguard still prevents re-initialization on HTMX reloads.This issue was previously flagged: when HTMX swaps in new DOM, the script sees
_fontsScriptLoadedis true (line 196) and calls the existingwindow.initializeFontsTab(). However,fontsTabInitializedremains true from the previous initialization, so the function returns early at line 226. The new DOM elements (#upload-dropzone, buttons, etc.) never receive event listeners, breaking the fonts UI after reload.Remove the
fontsTabInitializedcheck entirely—the_fontsScriptLoadedguard already prevents redeclaration of functions.Proposed fix
function initializeFontsTab() { - // Check if already initialized - if (fontsTabInitialized) { - console.log('Fonts tab already initialized, skipping...'); - return; - } - const detectedEl = document.getElementById('detected-fonts'); const availableEl = document.getElementById('available-fonts');Also remove the assignments at lines 243-244:
- // Mark as initialized to prevent duplicate initialization - window.fontsTabInitialized = true; - fontsTabInitialized = true;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/templates/v3/partials/fonts.html
🔇 Additional comments (5)
web_interface/templates/v3/partials/fonts.html (5)
193-207: LGTM: IIFE prevents redeclaration on HTMX reloads.The script load guard using
window._fontsScriptLoadedcorrectly prevents redeclaration of functions and variables when HTMX swaps content. The 50ms delay before callinginitializeFontsTab()allows the new DOM to settle.
209-221: LGTM: Window-scoped state enables persistence across HTMX reloads.Moving font state to
windowproperties ensures data persists when HTMX replaces content, while local references maintain code readability.
438-446: LGTM: Synchronizing window properties and local references.The pattern of updating
window.fontCatalog,window.fontTokens, andwindow.fontOverridesfirst, then syncing local references, ensures font data is accessible both globally (for re-initialization) and locally (for current function scope).
530-536: LGTM: Font path logic correctly avoids double-prefixing.The updated logic properly handles all cases:
- Absolute paths (starting with
/) are used as-is- Paths already prefixed with
assets/are used as-is- Bare filenames are prefixed with
assets/fonts/This resolves the previous issue where paths like
assets/fonts/press_start.ttffromget_fonts_catalogwere being prefixed again.
837-837: LGTM: IIFE properly closed.The IIFE is correctly closed, encapsulating all font management logic to prevent global scope pollution and redeclaration issues on HTMX reloads.
… 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)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web_interface/templates/v3/partials/fonts.html (4)
282-324: Event listeners accumulate on each HTMX reload, causing duplicate handler execution.Every time
initializeFontsTab()runs (including after HTMX swaps), new event listeners are attached to the same DOM elements without removing old ones. This causes each user action to trigger multiple handlers—2× after the first reload, 3× after the second, and so on.Either:
- Remove old listeners before adding new ones using
AbortControlleror cloned elements- Check if listeners are already attached before adding
- Use event delegation on a stable parent container
🔎 Proposed fix using AbortController
+// Store abort controller on window to cancel previous listeners +if (window._fontsAbortController) { + window._fontsAbortController.abort(); +} +window._fontsAbortController = new AbortController(); +const signal = window._fontsAbortController.signal; + if (uploadDropzone && fontFileInput) { - uploadDropzone.addEventListener('click', () => { + uploadDropzone.addEventListener('click', () => { fontFileInput.click(); - }); + }, { signal }); } if (fontFileInput) { - fontFileInput.addEventListener('change', handleFileSelection); + fontFileInput.addEventListener('change', handleFileSelection, { signal }); } if (uploadFontsBtn) { - uploadFontsBtn.addEventListener('click', uploadSelectedFonts); + uploadFontsBtn.addEventListener('click', uploadSelectedFonts, { signal }); } if (cancelUploadBtn) { - cancelUploadBtn.addEventListener('click', cancelFontUpload); + cancelUploadBtn.addEventListener('click', cancelFontUpload, { signal }); } if (addOverrideBtn) { - addOverrideBtn.addEventListener('click', addFontOverride); + addOverrideBtn.addEventListener('click', addFontOverride, { signal }); } if (updatePreviewBtn) { - updatePreviewBtn.addEventListener('click', updateFontPreview); + updatePreviewBtn.addEventListener('click', updateFontPreview, { signal }); } // Drag and drop for upload area if (uploadDropzone) { uploadDropzone.addEventListener('dragover', (e) => { e.preventDefault(); uploadDropzone.classList.add('drag-over'); - }); + }, { signal }); uploadDropzone.addEventListener('dragleave', () => { uploadDropzone.classList.remove('drag-over'); - }); + }, { signal }); uploadDropzone.addEventListener('drop', (e) => { e.preventDefault(); uploadDropzone.classList.remove('drag-over'); handleFileSelection({ target: { files: e.dataTransfer.files } }); - }); + }, { signal }); }
560-560:baseUrlis undefined—breaks font override and upload functionality.
baseUrlis defined insideloadFontData()(line 400) but referenced in:
- Line 560 (
addFontOverride)- Line 592 (
deleteFontOverride)- Line 783 (
uploadSelectedFonts)These calls will fail with
ReferenceError: baseUrl is not defined.🔎 Proposed fix: define baseUrl in outer scope
})(); // End of script load guard - prevents redeclaration on HTMX reload + +// Define baseUrl at module scope for all functions to use +const baseUrl = window.location.origin; async function initializeFontManagement() { try { @@ -397,7 +400,6 @@ try { // Use absolute URLs to ensure they work when loaded via HTMX - const baseUrl = window.location.origin; const [catalogRes, tokensRes, overridesRes] = await Promise.all([ fetch(`${baseUrl}/api/v3/fonts/catalog`), fetch(`${baseUrl}/api/v3/fonts/tokens`),Also applies to: 592-592, 783-783
638-638:deleteFontOverridenot exposed to global scope—onclick handler will fail.Line 638 uses
onclick="deleteFontOverride('${elementKey}')"but the function (defined at line 586) is not exposed on thewindowobject. Inline onclick handlers require global scope access.🔎 Proposed fix: expose function to window
Add after the function definition around line 608:
} } + +// Expose to global scope for onclick handler +window.deleteFontOverride = deleteFontOverride; function displayCurrentOverrides() {
718-718:selectedFontFileslocal reassignment doesn't sync towindow.selectedFontFiles.The dual-state pattern establishes both
window.selectedFontFiles(line 213) and a local reference (line 219). However, lines 718 and 748 reassign only the local variable:selectedFontFiles = validFiles; // line 718 selectedFontFiles = []; // line 748This leaves
window.selectedFontFilesstale. On the next HTMX reload, line 219 pulls the old value, causing inconsistent state.🔎 Proposed fix: sync both window and local references
- selectedFontFiles = validFiles; + window.selectedFontFiles = validFiles; + selectedFontFiles = window.selectedFontFiles; showUploadForm();Similarly for line 748:
function cancelFontUpload() { - selectedFontFiles = []; + window.selectedFontFiles = []; + selectedFontFiles = window.selectedFontFiles; document.getElementById('upload-form').style.display = 'none'; document.getElementById('font-file-input').value = ''; }Also applies to: 748-748
🧹 Nitpick comments (1)
web_interface/templates/v3/partials/fonts.html (1)
543-543: InconsistentshowNotificationusage—sometimes direct, sometimes viawindow.The function is defined as
window.showNotification(line 238) and calls like line 543 rely on automatic global scope lookup. However, checks like line 455 usetypeof showNotification === 'function'which tests local scope first. For clarity and consistency:Either consistently use
window.showNotificationthroughout, or create a local reference at the top ofinitializeFontsTab():const showNotification = window.showNotification;Also applies to: 455-460, 570-570, 578-578, 582-582, 598-598, 602-602, 606-606, 710-710, 715-715, 774-774, 791-791, 793-793, 809-809
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/templates/v3/partials/fonts.html
🔇 Additional comments (2)
web_interface/templates/v3/partials/fonts.html (2)
193-204: LGTM: IIFE guard correctly prevents redeclaration on HTMX reload.The pattern of checking
window._fontsScriptLoadedand callingwindow.initializeFontsTab()after a brief delay addresses the HTMX content swap scenario effectively. This resolves the previous concern about thefontsTabInitializedguard blocking reinitialization.
520-530: Path prefix logic correctly prevents double-prefixing.The backend
get_fonts_catalogreturns objects with apathproperty containing the full relative path (e.g.,'assets/fonts/press_start.ttf'). The frontend correctly detects this already-prefixed format withfontPath.startsWith('assets/')and uses the path as-is, eliminating the double-prefix issue from the prior review.
Description
Fixes multiple critical errors in the web interface:
path.startsWithfails (path is object, not string)Changes Made
Font Display Fixes
updateAvailableFontsDisplay()to handle font catalog entries as objects instead of stringsConfig API Error Handling
save_main_configto useerror_response()helper instead of simplejsonify()save_raw_main_configerror handling to useerror_response()consistentlyCSRF Protection
Files Changed
web_interface/templates/v3/partials/fonts.html- Font display and script reload fixesweb_interface/blueprints/api_v3.py- Improved error handlingweb_interface/app.py- Removed CSRF protectionTesting
Type of Change
Checklist
Summary by CodeRabbit
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.