fix(web-ui): Fix GitHub token warning persistence and improve UX #154
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
- Made checkGitHubAuthStatus() return Promise for proper async handling - Clear sessionStorage dismissal flag when token is saved - Add delay before status check to ensure backend token reload - Wait for status check completion before hiding settings panel Fixes issue where GitHub token warnings and pop-ups would not disappear after successfully saving a token in the web UI.
…havior - Add token validation to backend API endpoint to check if token is valid/expired - Implement _validate_github_token() method in PluginStoreManager with caching - Update frontend to show warning only when token is missing or invalid - Keep settings panel accessible (collapsible) when token is configured - Collapse settings panel content after successful token save instead of hiding - Display specific error messages for invalid/expired tokens - Clear sessionStorage dismissal flag when token becomes valid Fixes issue where GitHub token warnings and settings panel would not properly hide/show based on token status. Now validates token validity and provides better UX with collapsible settings panel.
- Update all hide/show operations to use both classList and style.display - Fix checkGitHubAuthStatus() to properly hide/show warning and settings - Fix dismissGithubWarning() to use both methods - Fix toggleGithubTokenSettings() with improved state checking - Fix collapse button handler with improved state checking - Fix saveGithubToken() to properly show/collapse settings panel This ensures elements actually hide/show when status changes, matching the pattern used elsewhere in the codebase (like toggleSection). All buttons (dismiss, close, collapse) should now work correctly.
- Convert collapse button handler to named function (toggleGithubTokenContent) - Improve state checking using class, inline style, and computed style - Re-attach event listener after saving token to ensure it works - Add console logging for debugging - Make function globally accessible for better reliability Fixes issue where expand button didn't work after saving token.
- Remove X (close) button from GitHub token configuration panel - Replace toggleGithubTokenSettings() with openGithubTokenSettings() that only opens - Auto-collapse panel when token is valid (user must click expand to edit) - Auto-detect token status on page load (no need to click save) - Simplify saveGithubToken() to rely on checkGitHubAuthStatus() for UI updates - Ensure expand button works correctly with proper event listener attachment The panel now remains visible but collapsed when a token is configured, allowing users to expand it when needed without the ability to completely hide it.
|
Warning Rate limit exceeded@ChuckBuilds has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a script to inject defaults into plugin config schemas, strengthens defaults-aware merging and GitHub token validation (with caching), implements schema-aware plugin config parsing/validation and structured API error responses, updates plugin-manager UI/token flows and fonts partial idempotency, and advances a plugin submodule pointer. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser as Browser UI
participant API as api_v3
participant SchemaMgr as Schema Manager
participant StoreMgr as Store Manager
participant PluginSys as Plugin System
rect `#dceBFF`
Note over User,API: Plugin config save (schema-aware)
User->>Browser: Submit plugin config form
Browser->>API: POST /api/v3/plugin/<id>/config (form data)
API->>API: Parse form values (indexed arrays & coercions)
API->>SchemaMgr: merge_with_defaults(user_cfg, defaults)
SchemaMgr->>SchemaMgr: Deep-merge and replace None with defaults
SchemaMgr-->>API: merged_config
API->>API: Validate merged_config against schema
alt validation fails
API-->>Browser: error_response (ErrorCode + details)
Browser-->>User: Display validation errors
else validation succeeds
API->>PluginSys: Persist config and secrets (separated)
PluginSys-->>API: Save OK
API->>PluginSys: Notify instances (on_config_change)
PluginSys-->>API: Ack
API-->>Browser: success_response
Browser-->>User: Show success
end
end
rect `#fff5dc`
Note over User,StoreMgr: GitHub token status check + caching
User->>Browser: Request token status
Browser->>API: GET /api/v3/github/auth-status
API->>StoreMgr: _validate_github_token(token)
alt cached
StoreMgr-->>API: (is_valid, error_msg) [cache hit]
else not cached
StoreMgr->>StoreMgr: Call GitHub /user
alt success
StoreMgr->>StoreMgr: Cache success
StoreMgr-->>API: (true, None)
else fail
StoreMgr-->>API: (false, error_msg)
end
end
API-->>Browser: token_status (none/valid/invalid) + details
Browser-->>User: Update banner/panel/controls
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
web_interface/templates/v3/partials/fonts.html (3)
400-400: Critical:baseUrlis scoped toloadFontData()but used in other functions.The
baseUrlconstant is defined insideloadFontData()(line 400) but is referenced inaddFontOverride()(line 560),deleteFontOverride()(line 592), anduploadSelectedFonts()(line 783). This will cause aReferenceErrorwhen those functions execute.🔎 Proposed fix: Define baseUrl at module scope
Move the
baseUrldefinition outsideloadFontData()to make it accessible to all functions:+// Define baseUrl at module scope for use across all functions +const baseUrl = window.location.origin; + async function loadFontData() { const detectedContainer = document.getElementById('detected-fonts'); const availableContainer = document.getElementById('available-fonts'); // Ensure containers exist before proceeding if (!detectedContainer || !availableContainer) { console.error('Font containers not found, cannot load font data'); return; } // Show loading states detectedContainer.innerHTML = '<div class="text-blue-400">Loading font data...</div>'; availableContainer.innerHTML = '<div class="text-blue-400">Loading font data...</div>'; try { - // Use absolute URLs to ensure they work when loaded via HTMX - const baseUrl = window.location.origin; const [catalogRes, tokensRes, overridesRes] = await Promise.all([
586-608: Critical:deleteFontOverrideis not exposed to global scope but called from HTML.The
deleteFontOverridefunction is defined inside the IIFE but is called from an inlineonclickhandler on line 638. Since the IIFE wraps all functions,deleteFontOverrideis not accessible from the global scope where the onclick handler executes, resulting in a "function is not defined" error.🔎 Proposed fix: Expose deleteFontOverride to window scope
Add this line after the function definition to make it globally accessible:
async function deleteFontOverride(elementKey) { if (!confirm(`Are you sure you want to remove the font override for "${elementKey}"?`)) { return; } try { const response = await fetch(`${baseUrl}/api/v3/fonts/overrides/${elementKey}`, { method: 'DELETE' }); const data = await response.json(); if (data.status === 'success') { showNotification('Font override removed successfully', 'success'); await loadFontData(); displayCurrentOverrides(); } else { showNotification('Error removing font override: ' + data.message, 'error'); } } catch (error) { console.error('Error deleting font override:', error); showNotification('Error removing font override: ' + error, 'error'); } } + +// Expose to window for onclick handler +window.deleteFontOverride = deleteFontOverride;
718-719: Major: ReassigningselectedFontFilesbreaks window reference.On lines 718 and 748, the local
selectedFontFilesvariable is reassigned, which breaks its connection towindow.selectedFontFiles. Since the guard at line 219 creates a reference to the window property (var selectedFontFiles = window.selectedFontFiles), reassigning the local variable doesn't updatewindow.selectedFontFiles. This causes state loss across HTMX content reloads.🔎 Proposed fix: Update both window and local reference
Update both the window property and local reference when reassigning:
Line 718:
- selectedFontFiles = validFiles; + window.selectedFontFiles = validFiles; + selectedFontFiles = window.selectedFontFiles;Line 748:
- selectedFontFiles = []; + window.selectedFontFiles = []; + selectedFontFiles = window.selectedFontFiles;Alternatively, use a more concise form:
- selectedFontFiles = validFiles; + selectedFontFiles = window.selectedFontFiles = validFiles;Also applies to: 748-749
web_interface/blueprints/api_v3.py (1)
3646-3854: Fix logger usage before assignment and avoid logging full plugin configs (may include secrets).Two issues in this region:
Runtime error risk:
loggerused before assignment
Inside
normalize_config_values, you calllogger.warning(...)(Line 3736) butloggeris never defined in the enclosing scope prior to possible invocation.Immediately after normalization, you do:
if 'rotation_settings' in plugin_config ...: seed_value = ... logger.debug(...)before
logger = logging.getLogger(__name__)is assigned at Line 3870.Both of these will cause an
UnboundLocalErrorwhen those branches are hit.A minimal fix is to avoid relying on a shared
loggervariable and use the logging module directly:Proposed fix for logger usage
--- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ def normalize_config_values(config, schema_props, prefix=''): - # Log a warning for debugging - logger.warning(f"Could not normalize field {field_path}: value={repr(value)}, type={type(value)}, schema_type={prop_type}") + # Log a warning for debugging + import logging + logging.getLogger(__name__).warning( + "Could not normalize field %s: value=%r, type=%s, schema_type=%s", + field_path, + value, + type(value), + prop_type, + ) @@ - # Debug logging for union type fields (temporary) - if 'rotation_settings' in plugin_config and 'random_seed' in plugin_config.get('rotation_settings', {}): - seed_value = plugin_config['rotation_settings']['random_seed'] - logger.debug(f"After normalization, random_seed value: {repr(seed_value)}, type: {type(seed_value)}") + # Debug logging for union type fields (temporary) + if 'rotation_settings' in plugin_config and 'random_seed' in plugin_config.get('rotation_settings', {}): + import logging + seed_value = plugin_config['rotation_settings']['random_seed'] + logging.getLogger(__name__).debug( + "After normalization, random_seed value: %r, type: %s", + seed_value, + type(seed_value), + ) @@ - if schema: - # Log what we're validating for debugging - import logging - logger = logging.getLogger(__name__) - logger.info(f"Validating config for {plugin_id}") - logger.info(f"Config keys being validated: {list(plugin_config.keys())}") - logger.info(f"Full config: {plugin_config}") + if schema: + # Log what we're validating for debugging + import logging + logger = logging.getLogger(__name__) + logger.info("Validating config for %s", plugin_id) + logger.info("Config keys being validated: %s", list(plugin_config.keys())) + # Avoid logging full config values here; may contain secrets + logger.debug("Config summary keys: %s", list(plugin_config.keys()))Potential leakage of secrets in logs
logger.info(f"Full config: {plugin_config}")logs the entire plugin config at info level before secrets are separated out, which can easily include API keys, tokens, etc.- Given secrets are explicitly modeled via
x-secretin schemas, this is a real privacy/security concern.The diff above switches to key‑level logging and moves full config detail to an optional
debuglog (or you could drop it entirely). If you still need deep diagnostics, consider redactingx-secretfields before logging.Also applies to: 3862-3873
web_interface/static/v3/plugins_manager.js (1)
4705-5039: Settings panel becomes undiscoverable after warning dismissal—this is a UX regressionThe token settings flow is well-structured overall, but there's a critical usability issue:
openGithubTokenSettings()has only one UI entrypoint: the "Configure Token" link inside the warning banner.dismissGithubWarning()hides both the warning and the settings panel, removing the only way for users to access token configuration.- After dismissing the warning once, users cannot recover the settings panel unless they reload the page, which clears the dismissal flag.
Fix: Either (1) modify
dismissGithubWarning()to hide only the warning, not the settings panel, or (2) add an alternate "Configure Token" button/link outside the warning banner.Two secondary concerns:
- Token status transitions (line 4948): If status changes from
invalidback tononewithout a full page reload, the warning text will still display "Token Invalid…" instead of reverting to the default message. Consider caching the original HTML or using a data attribute to track state.- Brittle selector (line 4948): The
querySelector('p.text-sm.text-yellow-700')is fragile. Adding anidordata-roleattribute to that<p>element would make it more robust against Tailwind class changes.
🧹 Nitpick comments (11)
src/plugin_system/store_manager.py (1)
80-155: Well-structured token validation with caching.The implementation correctly:
- Uses a privacy-preserving cache key (first 10 chars)
- Caches successful validations and definitive failures (401, 403)
- Avoids caching transient errors (timeout, network issues)
- Returns structured tuple for caller to handle appropriately
Minor refinement per static analysis hints:
🔎 Optional: Use explicit string conversion and specific exception
except requests.exceptions.RequestException as e: - error_msg = f"Network error: {str(e)}" + error_msg = f"Network error: {e!s}" result = (False, error_msg) # Don't cache network errors return result - except Exception as e: - error_msg = f"Unexpected error: {str(e)}" + except (ValueError, TypeError, KeyError) as e: + error_msg = f"Unexpected error: {e!s}" result = (False, error_msg) # Don't cache unexpected errors return resultThe
!sconversion flag is more explicit per PEP 3101. However, keeping a broadExceptioncatch here is acceptable as a safety net for an external API call—just be aware it may mask unexpected issues.scripts/add_defaults_to_schemas.py (2)
123-136: Mutable default argument pattern.Using
modified: List[str] = Nonewith initialization inside the function is correct, but the type hint should reflect the nullable type explicitly.🔎 Fix type hint per PEP 484
-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:This makes the type annotation explicit about the parameter being optional.
196-198: Remove unnecessary f-string prefix.Line 197 has an f-string without any placeholders.
🔎 Fix per Ruff F541
else: - print(f" ✓ No changes needed") + print(" ✓ No changes needed") return Falseweb_interface/templates/v3/base.html (2)
4212-4259: Config save status handling and messaging look correct; only minor defensive/DRY opportunitiesThe switch to explicit HTTP status checks and server-derived messages is solid and correctly guarded with optional chaining and try/catch. As an optional polish:
- If
handleConfigSavemight ever be reused outside HTMX, consider guardingevent.detail/event.detail.xhrand early‑returning when absent to avoid treating that as a generic failure.- The success/error response‑parsing logic here is very similar to other handlers (e.g., plugin update). A small shared helper like
parseXhrMessage(xhr, { defaultSuccess, defaultError })would reduce duplication and keep behavior consistent across future changes.
4265-4311: Toggle response handler mirrors config status logic well; consider adding structured error loggingThe 2xx check plus parsing
xhr.responseJSON/responseTextfor messages is correct and matches the config‑save behavior. One optional improvement:
- In the non‑2xx branch, add a
console.error('Plugin toggle failed', { status, statusText: xhr?.statusText, responseText: xhr?.responseText, pluginId });similar tohandleConfigSave. This would make debugging failed toggles easier without changing user‑visible behavior.web_interface/blueprints/api_v3.py (5)
629-640: Good move to unified error_response; tweak f-string per Ruff (RUF010).The new
error_responseusage on exception is consistent with the rest of the file and adds structured details viadetails=traceback.format_exc().Ruff’s
RUF010warning here (line 637) is aboutf"Error saving configuration: {str(e)}". Inside an f-string, prefer an explicit conversion flag instead ofstr():Proposed small cleanup for RUF010
- 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 + )
681-701: Structured error_response for raw main config looks solid; consider detail exposure.Switching from bare
jsonifytoerror_responsewithErrorCode.CONFIG_SAVE_FAILED/UNKNOWN_ERRORandcontext={'config_path': ...}is a good improvement for diagnosability.One thing to consider is whether returning
traceback.format_exc()to the client (details=...) is acceptable in your deployment context, since it can leak file paths and internal structure. If this is user‑facing, you might want to keep full tracebacks only in logs and return a shorter, user‑friendlydetails.
2767-2812: GitHub token status endpoint behavior looks correct and front‑end friendly.The updated
/plugins/store/github-statusnow:
- Differentiates between
token_status: 'none' | 'valid' | 'invalid'.- Avoids unnecessary validation calls when no token is configured.
- Uses
plugin_store_manager._validate_github_token(token)to classify validity and still returnsauthenticated,rate_limit, and human‑readablemessage.This should align well with the new UI behavior (warning only when missing/invalid). If you later want to distinguish “network error” or “rate-limited but token OK”, you could extend
token_statuswith an'error'/'rate_limited'state, but current behavior is coherent.
2946-2981: _is_field_required + _SKIP_FIELD: simple and effective, but limited to properties‑only schemas.
_is_field_required(key_path, schema)correctly:
- Checks top‑level
schema['required']for simple fields.- Walks into nested objects via
_get_schema_propertyand the parent’srequiredlist.- Returns
Falsewhen schema/properties are missing, which is a safe default.Combined with
_SKIP_FIELD, this gives you a clear mechanism to decide when an empty form value should be skipped vs. set.Just be aware that it currently only understands straight
properties/requiredlayouts; anyallOf/anyOf/$ref‑heavy schemas won’t be respected here. That’s fine if your plugin schemas stick to simple object properties.
2984-3025: Empty value handling now respects schema requirements; confirm semantics for optional numerics.The changes in
_parse_form_value_with_schemaaround empty values:
- Return
[]/{}for array/object types, avoidingnullwhere an empty container is more natural.- For optional string fields (
type: "string"and not required), return""instead ofNone, which should keep validation happy and let you distinguish “unset” from “missing key”.- For numeric fields:
- Use the schema
defaultif present.- If not required and no default: return
_SKIP_FIELDso the caller can leave the existing config value alone.- If required and empty: return
None, letting schema validation fail as it should.This is a sensible model, but there’s a behavioral nuance: for optional numeric fields without defaults, clearing the field in the UI will not remove or null out any existing value in the stored config; the old value remains because
_SKIP_FIELDprevents_set_nested_valuefrom updating that path.If your intent is that “blank optional numeric field” should revert to schema defaults (or effectively remove the key), you may want to treat that case differently. Otherwise, current behavior (“blank = don’t change”) is fine—just worth confirming.
web_interface/static/v3/plugins_manager.js (1)
4029-4083: GitHub token collapse toggle is correct; consider small robustness/cleanup tweaksThe collapse handler and clone‑replace pattern look solid and should resolve the “multiple listener” issues, and the combined classList + style.display checks are a good safeguard against template inconsistencies.
Two small nits you may want to address:
- The comment in Line 4032 still references a “close button”, which no longer exists on the token panel; updating this to describe the actual interaction (e.g., “prevent parent click handlers”) would keep intent clear.
- For Line 4053 and Line 4064, hard‑coding
display = 'block'/'none'works today but makes it harder to change layout (e.g., flex/grid) later. Using''to defer to CSS (while still toggling thehiddenclass) would be more future‑proof.- The clone‑and‑attach logic for
toggle-github-token-collapseis duplicated here and again incheckGitHubAuthStatus(); a tiny helper likeattachGithubTokenCollapseHandler()called from both places would reduce drift risk.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
plugins/mqtt-notificationsscripts/add_defaults_to_schemas.pysrc/plugin_system/schema_manager.pysrc/plugin_system/store_manager.pyweb_interface/app.pyweb_interface/blueprints/api_v3.pyweb_interface/static/v3/plugins_manager.jsweb_interface/templates/v3/base.htmlweb_interface/templates/v3/partials/fonts.htmlweb_interface/templates/v3/partials/plugins.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:
src/plugin_system/schema_manager.pyweb_interface/app.pyscripts/add_defaults_to_schemas.pysrc/plugin_system/store_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.pysrc/plugin_system/store_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.pysrc/plugin_system/store_manager.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/config_schema.json : `config_schema.json` must define JSON schema for plugin configuration validation
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/config_schema.json : Use `config_schema` key in plugin configuration to reference schema file
📚 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 (3)
src/plugin_system/schema_manager.py (3)
web_interface/blueprints/api_v3.py (1)
deep_merge(2850-2863)web_interface/static/v3/plugins_manager.js (1)
target(690-690)src/config_manager.py (2)
_merge_template_defaults(358-367)_deep_merge(258-264)
scripts/add_defaults_to_schemas.py (1)
web_interface/static/v3/plugins_manager.js (1)
max(2240-2240)
web_interface/blueprints/api_v3.py (1)
src/plugin_system/store_manager.py (1)
_validate_github_token(80-155)
🪛 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)
src/plugin_system/store_manager.py
147-147: Use explicit conversion flag
Replace with conversion flag
(RUF010)
151-151: Do not catch blind exception: Exception
(BLE001)
152-152: Use explicit conversion flag
Replace with conversion flag
(RUF010)
web_interface/blueprints/api_v3.py
637-637: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (12)
web_interface/templates/v3/partials/fonts.html (3)
193-220: IIFE guard pattern correctly prevents redeclaration on HTMX reloads.The use of
window._fontsScriptLoadedflag and the IIFE wrapper properly handles HTMX content swaps without redeclaring variables/functions. The pattern of maintaining both window-scoped and local variable references is necessary for this use case.However, be aware that this dual-reference pattern requires careful handling: reassigning local variables (e.g.,
fontCatalog = newValue) will break the connection towindow.fontCatalog, while mutating properties (e.g.,fontCatalog.key = value) will correctly update the window property.
329-330: LGTM!Exposing
initializeFontsTabto window scope enables clean re-initialization on HTMX reloads.
520-528: LGTM! Enhanced path handling for font catalog display.The logic correctly handles
fontInfoas either a string or an object with apathproperty, and intelligently constructs the display path by:
- Preserving absolute paths (starting with
/)- Preserving paths already prefixed with
assets/- Adding
assets/fonts/prefix to bare filenamesThis improves robustness when dealing with different font catalog formats.
plugins/mqtt-notifications (1)
1-1: Clarify the scope and intent of the submodule update.This PR's stated objectives focus on GitHub token warning persistence and UX improvements, yet the mqtt-notifications submodule is being updated. The reason for this submodule update is not documented in the PR description.
Please clarify:
- Why is the mqtt-notifications submodule being updated as part of this PR?
- What specific changes are included in the new submodule commit (5c23a842...)?
- Is this update intentional and directly related to the PR's objectives?
Submodule updates should be explicitly justified and documented to avoid introducing unintended changes. If this update is incidental or unrelated to the main PR objectives, consider separating it into a distinct PR.
src/plugin_system/schema_manager.py (1)
394-446: LGTM! Robust defaults propagation with None replacement.The enhanced
merge_with_defaultscorrectly:
- Starts from a deep copy of defaults to avoid mutation
- Handles None values by replacing them with defaults during merge
- Performs a second pass to catch any remaining None values in nested structures
The two-pass approach is defensive and ensures complete None elimination. The deep copying of dict/list values prevents unintended shared references.
src/plugin_system/store_manager.py (1)
95-100: Cache key collision consideration.Using
token[:10]as the cache key provides privacy but could theoretically cause cache collisions if two different tokens share the same prefix. Since GitHub tokens have high entropy after theghp_prefix, the practical risk is minimal. The 5-minute TTL further limits any impact.Consider whether hashing the full token (e.g.,
hashlib.sha256(token.encode()).hexdigest()[:16]) would provide better uniqueness while maintaining privacy, though the current approach is acceptable for this use case.scripts/add_defaults_to_schemas.py (1)
102-106: Broad keyword matching may skip legitimate fields.The keyword
'key'in the sensitive list could match non-sensitive fields likeprimary_key,sort_key,key_color, etc. Consider using more specific patterns or adding an allowlist for known safe field patterns.This is conservative behavior (erring on the side of not adding defaults), so it's not a bug, but worth noting if fields unexpectedly lack defaults.
web_interface/templates/v3/partials/plugins.html (1)
78-80: LGTM! Correct update to open-only settings pattern.The change from
toggleGithubTokenSettings()toopenGithubTokenSettings()aligns with the PR objective of improving UX by auto-collapsing the panel when token is valid and only opening when user explicitly requests it.web_interface/app.py (2)
29-36: LGTM! Clear rationale for disabled CSRF.The decision to disable CSRF protection is well-documented with clear reasoning:
- Local-only threat model is different from internet-facing apps
- API endpoints use programmatic requests without CSRF tokens
- Clear guidance provided for enabling CSRF if needed
This is appropriate for a local Raspberry Pi application where CSRF's value is minimal.
526-530: Clean conditional guard for disabled CSRF.The
if csrf:guard correctly prevents errors when CSRF is disabled while maintaining forward compatibility if CSRF is re-enabled in the future.web_interface/blueprints/api_v3.py (2)
3106-3135: _set_nested_value correctly honors _SKIP_FIELD and avoids clobbering existing structure.The updated
_set_nested_value:
- Early‑returns when
value is _SKIP_FIELD, so optional fields can be left untouched.- Still creates intermediate dicts for dotted paths.
- Prevents overwriting an existing key with
Noneunless the key doesn’t exist yet.This meshes well with
_parse_form_value_with_schema’s use of_SKIP_FIELDand should prevent accidental overwrites when form inputs are intentionally left blank.
3307-3343: Form parsing: good integration of _SKIP_FIELD with indexed and non‑indexed fields.In the form‑based plugin config path:
- Indexed array fields (
foo.0,foo.1, …) are correctly grouped into a combined value, parsed via_parse_form_value_with_schema, and only written ifparsed_value is not _SKIP_FIELD.- Non‑indexed fields similarly pass through
_parse_form_value_with_schemaand only call_set_nested_valuewhen not_SKIP_FIELD.This prevents optional numeric fields (without defaults) from being forcibly set to
Noneor empty when left blank, while still allowing explicit user input to override existing config.
- Update comment to reflect actual behavior (prevent parent click handlers) - Use empty string for display to defer to CSS instead of hard-coding block/none - Extract duplicate clone-and-attach logic into attachGithubTokenCollapseHandler() helper - Make helper function globally accessible for reuse in checkGitHubAuthStatus() Improves maintainability and makes code more future-proof for layout changes.
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 (1)
web_interface/static/v3/plugins_manager.js (1)
4905-4918: Settings panel becomes inaccessible after dismissing warning with missing/invalid token—add persistent alternate entrypoint.The refactor correctly uses
/github-statusto drive UI state and implements dismissal viasessionStorage. However, there's a critical UX issue: the only way to callopenGithubTokenSettings()is via the "Configure Token" link inside the warning banner (line 78 of plugins.html). When a user dismisses the warning with a missing or invalid token,dismissGithubWarning()hides both the banner and the settings panel, and the dismissal flag prevents the banner from reappearing in that session. This leaves the settings panel inaccessible without a page reload.Add a persistent control (outside the warning banner) that can always call
openGithubTokenSettings()when the token is missing or invalid—for example, a configuration link in the "GitHub API Configuration" header of the settings panel itself, or a dedicated settings icon in the main plugin section header.
🧹 Nitpick comments (2)
web_interface/static/v3/plugins_manager.js (2)
4712-4752: Open‑only behavior and state sync look correct; display usage could be made more consistent (optional).The function correctly:
- Makes the settings panel visible,
- Expands the token content and updates icon/label,
- Hides the warning and clears the dismissal flag,
- Loads the token.
The only nit is that here you use
tokenContent.style.display = 'block'while the collapsible logic elsewhere prefers''plus Tailwind’shiddenclass. Aligning on the same pattern would avoid any future surprises if CSS defaults change, but it’s not required for correctness.
4028-4092: Collapse handler logic is sound; parentNode guard is optional but could improve robustness.The cloning approach effectively prevents duplicate listeners and the
isHiddencomputation correctly reconciles class and style state. The template does not currently use inline onclick handlers on the button, so no double-handling risk exists. That said, adding a simple check beforeparentNode.replaceChild()would make the code more defensive against potential future markup changes:if (toggleTokenCollapseBtn && toggleTokenCollapseBtn.parentNode) { const newBtn = toggleTokenCollapseBtn.cloneNode(true); toggleTokenCollapseBtn.parentNode.replaceChild(newBtn, toggleTokenCollapseBtn); // ... }
…isplay
- Use style.removeProperty('display') instead of style.display = ''
- This properly removes inline styles and defers to CSS classes
- Fixes issue where collapse/expand button stopped working after refactor
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 (1)
web_interface/static/v3/plugins_manager.js (1)
4935-5040: Sanitize invalid-token error text before injecting intoinnerHTMLThe status handling and banner/panel visibility logic look correct and align with the new UX: only show the warning for
none/invalid, hide it forvalid, keep the settings panel visible but collapsed when valid, and persist dismissals viasessionStorage.One concern: in the
invalidbranch you do:const errorMsg = authData.message || authData.error; warningText.innerHTML = `<strong>Token Invalid:</strong> ${errorMsg}. ...`;If
message/errorever contain unexpected markup (now or in future backend changes), this becomes an XSS vector.Given you already have
escapeHtml(), it’s safer to escape before injecting:Proposed hardening
- if (tokenStatus === 'invalid' && authData.error) { + if (tokenStatus === 'invalid' && authData.error) { const warningText = warning.querySelector('p.text-sm.text-yellow-700'); if (warningText) { - // Preserve the structure but update the message - const errorMsg = authData.message || authData.error; - warningText.innerHTML = `<strong>Token Invalid:</strong> ${errorMsg}. Please update your GitHub token to increase API rate limits to 5,000 requests/hour.`; + // Preserve the structure but update the message (escaped to avoid XSS) + const rawErrorMsg = authData.message || authData.error || ''; + const safeErrorMsg = escapeHtml(String(rawErrorMsg)); + warningText.innerHTML = `<strong>Token Invalid:</strong> ${safeErrorMsg}. Please update your GitHub token to increase API rate limits to 5,000 requests/hour.`; } }This keeps the UX the same while avoiding any HTML/JS injection risk from server‑provided strings. The updated
dismissGithubWarninglogic that hides both the banner and settings (and records dismissal) otherwise looks good and consistent with the new status handling.
🧹 Nitpick comments (2)
web_interface/static/v3/plugins_manager.js (2)
4028-4092: GitHub token collapse handler and toggle look robustThe cloning pattern to drop old listeners and the use of
style.removeProperty('display')plus class/computed-style checks should fix the previous stuck-collapse behavior and is defensive against mixed CSS/inline states. Only minor note: cloning will also remove any other listeners or ad‑hoc data attached at runtime to#toggle-github-token-collapse, so if that button ever gets additional behavior, it should be wired through this helper as well.
4715-4934: Token settings open/save flow is coherent; consider minor consistency tweaksThe new
openGithubTokenSettings+saveGithubToken+checkGitHubAuthStatusinteraction gives the desired UX: opening always shows the panel/content and hides the banner, saving clears the dismissal flag and re-syncs state, and status is now Promise-based for potential callers that want to await it.If you want to tighten things further (optional):
- In
openGithubTokenSettings, usingsettings.style.removeProperty('display')instead ofstyle.display = ''would align with the collapse handler and rely purely on CSS for visibility.- Since
checkGitHubAuthStatus()now returns a Promise, callers that care about ordering (e.g., future UI flows) couldawaitit rather than relying on the fixed 300 ms timeout, though for the current use case the timeout is acceptable.
- Use removeProperty('display') consistently in all places
- Fix checkGitHubAuthStatus() to use removeProperty instead of inline style
- Simplify state checking to rely on hidden class with computed style fallback
- Ensures collapse/expand button works correctly by deferring to CSS classes
- Simplify state checking to rely on hidden class only (element has class='block') - Only remove inline display style if it exists (check before removing) - Add console logging to debug handler attachment - Ensure collapse/expand works by relying on CSS classes Fixes issues where: - Collapse button did nothing - Auto-detection of token status wasn't working
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web_interface/static/v3/plugins_manager.js (4)
4028-4090: GitHub token collapse handler looks solid; consider guarding against potential double‑handlersThe
attachGithubTokenCollapseHandler+toggleGithubTokenContentflow correctly centralizes state in thehiddenclass and usesremoveProperty('display'), which should fix the previous “stuck open/closed” behavior.One subtle risk:
cloneNode(true)will preserve any inlineonclickhandlers on#toggle-github-token-collapse. If the template ever still includes anonclick="toggleGithubTokenContent(...)", you’ll end up with both the inline handler and the new listener firing, effectively toggling twice and leaving the UI unchanged.A small hardening tweak would be:
- After cloning, explicitly clear any inline handler on the new button before
addEventListener, e.g.newBtn.onclick = null;, or- Use
cloneNode(false)and copy over only the child content, not event attributes.This avoids regressions if the markup drifts back to using inline handlers later.
4930-5017: Status‑driven warning/settings behavior is mostly spot‑on; verify access after dismissal in invalid/none states
checkGitHubAuthStatus()now:
- Normalizes
tokenStatusto'none' | 'invalid' | 'valid'(with a fallback to the oldauthenticatedflag),- Shows the banner only for
none/invalidand only if not dismissed in this session,- Updates the rate‑limit counter and, for invalid tokens, injects a more specific error message, and
- For
valid, hides the banner, makes the settings panel visible but collapsed, and resets the collapse button state.Two UX edge cases to be aware of:
Dismissed + invalid/none: If the user dismisses the warning while the token is missing/invalid,
sessionStorageprevents the banner from reappearing on reload within the same session, andcheckGitHubAuthStatusdoes not force#github-token-settingsvisible in those states. If there is no other “Configure token” entrypoint outside the banner, the settings panel could be effectively unreachable until the browser session is restarted.Handler attachment order: The conditional call to
window.attachGithubTokenCollapseHandler()is safe given the guard, andsetupCollapsibleSections()will attach it later on initial load, but the coupling between the two is a little non‑obvious.If you don’t already have a separate “Configure GitHub token” link elsewhere on the page, consider either:
- Ensuring the settings panel is also made visible (not necessarily expanded) when
tokenStatusisnone/invalid, or- Adding/confirming another entrypoint that calls
openGithubTokenSettings()even when the banner is dismissed.That keeps token management discoverable in all states.
5025-5039: Dismissing the warning hides the settings panel too; confirm that’s desired for missing/invalid tokens
dismissGithubWarning()now hides both the banner and#github-token-settings(via class and inline style) and records a session‑scoped dismissal flag.This is a good fix for the persistence issue, but in the
tokenStatus === 'none' || 'invalid'case it may also remove the user’s only obvious path to open the token settings, depending on your template.If there isn’t another control elsewhere that calls
openGithubTokenSettings(), you might want to:
- Stop hiding the settings panel here (only the banner), or
- Ensure a second “Configure token” control outside the banner is always present.
That will prevent a user from painting themselves into a corner by dismissing the warning with an invalid/missing token.
4620-4661: DuplicateformatDatedefinitions can be confusing and brittleThere are two top‑level
formatDatefunctions:
- Lines 4620‑4661: relative “Today / Yesterday / X days/weeks ago / formatted date”.
- Lines 5467‑5475: absolute “date + time” formatter.
Because the second declaration overwrites the first, anything earlier in the file that expected the relative formatting is actually getting the later absolute formatter.
Not directly related to this PR, but to reduce surprises it would be cleaner to:
- Consolidate to a single implementation, or
- Give them distinct names (e.g.
formatRelativeDateandformatFullDate) and use each explicitly where needed.This will make future date‑formatting changes much less error‑prone.
Also applies to: 5467-5475
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/static/v3/plugins_manager.js
🔇 Additional comments (2)
web_interface/static/v3/plugins_manager.js (2)
4711-4751:openGithubTokenSettingsbehavior matches the new UX requirementsOpening the token settings now:
- Ensures the panel is visible (
classList+style.display = ''),- Expands the inner content and synchronizes the collapse icon/button text,
- Hides the warning and clears the dismissal flag, and
- Loads the stored token.
This cleanly separates “open panel” from collapse behavior and lines up with the “only opens, doesn’t close” design.
4903-4912: Post‑save status refresh is correct; be aware of backend latency assumptionsClearing
github-auth-warning-dismissedand re‑invokingcheckGitHubAuthStatus()after a short delay is a good way to let the backend reload the token and re‑drive the UI (warning vs collapsed panel) from the true status.If you ever see flakiness on slower devices, you may want to slightly increase the 300 ms delay or consider a simple retry/poll pattern, but the current approach is reasonable for this UI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web_interface/static/v3/plugins_manager.js (4)
4028-4101: GitHub token collapse handler and content toggle look correct; minor robustness nitThe pattern of cloning
#toggle-github-token-collapseto remove old listeners, then attachingwindow.toggleGithubTokenContent, is consistent with the rest of the file and should avoid duplicate handlers. The toggle logic correctly uses thehiddenclass as the source of truth and clears any inlinedisplaythat might conflict.If you want to harden this further, you could add a defensive
if (toggleTokenCollapseBtn.parentNode)check beforereplaceChildto avoid a rare null‑parent edge case during HTMX swaps, but it’s not strictly necessary.
4913-4920: Post‑save status refresh is a good addition; consider surfacing errors via the returned PromiseClearing
github-auth-warning-dismissedand then callingcheckGitHubAuthStatus()after a short delay nicely re-syncs the banner/panel with backend validation after saving a token.If you ever need to react to failures from
/github-status(e.g., to show a toast), you couldreturn checkGitHubAuthStatus()from inside the timeout and optionally chain a.catch, but current behavior is acceptable since errors are already logged insidecheckGitHubAuthStatus.
4940-5030: Status‑driven GitHub token UI logic is consistent; small messaging edge caseThe updated
checkGitHubAuthStatus():
- Derives
tokenStatusfromdata.data.token_statusorauthenticated.- Shows the warning only for
none/invalid, honoring the session dismissal flag.- Updates the message for
invalidtokens usingauthData.message || authData.error.- Hides the warning and ensures the settings panel is visible but collapsed when the token is
valid, and re‑attaches the collapse handler.- Clears the dismissal flag when status becomes
valid.This matches the described behavior: warning only for missing/invalid tokens and a permanently accessible, collapsible settings panel when valid.
One minor edge case: if the backend ever transitions from
invalidback tononeduring the same page session, the custom “Token Invalid” text isn’t reset; the banner would still show the invalid‑token wording. If that matters, you could cache the original banner text (e.g., indataset.defaultMessage) and restore it whentokenStatus === 'none'.
5039-5052: Dismissal behavior remains consistent; style.display sync is a solid hardening
dismissGithubWarning()now hides both the warning and (if visible) the settings panel via class andstyle.display, and persists dismissal insessionStorage. That should address cases where inline styles kept the banner visible despite thehiddenclass.Given this is per‑session, users can still get the banner back on a fresh load; the only nuance is that within a session they must rely on other entrypoints (if any) to reopen token settings after dismissal. If ensuring a way to reach settings without reloading is important, you might later add a separate “Configure Token” affordance outside the dismissible banner.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/static/v3/plugins_manager.js
🔇 Additional comments (1)
web_interface/static/v3/plugins_manager.js (1)
4721-4761: openGithubTokenSettings() behavior matches the new UX contract
openGithubTokenSettingsnow only opens (no toggling), ensures the settings panel is visible via both class and style, expands the token content, syncs the collapse icon/text to “Collapse”, hides the warning, clears the dismissal flag, and then loads the token. That aligns with “open-on-click, auto-collapse when valid” behavior and avoids stale dismissal state.No functional issues spotted in this block.
- Add console logs to track function calls and element detection - Improve state detection to use computed style as fallback - Add wrapper function for click handler to ensure it's called - Better error messages to identify why handler might not attach This will help identify why the collapse button isn't working.
- Add console logs to checkGitHubAuthStatus() to track execution - Re-attach collapse handler after plugin store is rendered - Add error stack traces for better debugging - Ensure handler is attached when content is dynamically loaded This will help identify why: - Auto-detection of token status isn't working - Collapse button isn't functioning
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
web_interface/static/v3/plugins_manager.js (3)
4041-4073: Helper correctly de-duplicates GitHub token collapse listenersCloning and replacing
#toggle-github-token-collapseto strip prior listeners before wiringtoggleGithubTokenContentis a solid pattern for HTMX-replaced DOM and prevents handler accumulation across re-renders.If you later add ARIA attributes (e.g.,
aria-expanded), consider centralizing their updates in this helper too so accessibility state always matches the active handler.
4075-4138: Token content toggle logic looks robust across CSS/inline statesUsing both the
hiddenclass andgetComputedStyle(...).displayto infer collapsed/expanded state, plus cleaningstyle.display, should resolve the previous “stuck open/closed” issues and keep the button text/icon in sync;stopPropagation/preventDefaultalso avoid parent row clicks interfering.If console noise becomes an issue, you might want to gate the
console.logcalls in this handler behind the existingPLUGIN_DEBUGflag.
4978-5085: Token status handling and warning behavior are consistent with backend statesMapping
authData.token_statuswith a fallback to the legacyauthenticatedflag, and explicitly branching on'none' | 'invalid' | 'valid', gives a clear contract for when to show the warning and how to treat valid tokens (panel visible but collapsed, banner hidden, dismissal cleared). The invalid-token message override that reuses the existing banner structure is also a nice touch.Right now the warning is only shown when both
warningandrateLimitelements exist; to make this more resilient to future template tweaks, consider always showing the banner whenwarningexists and only updatingrateLimit.textContentwhen the span is present.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/static/v3/plugins_manager.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/config_schema.json : `config_schema.json` must define JSON schema for plugin configuration validation
🔇 Additional comments (4)
web_interface/static/v3/plugins_manager.js (4)
3829-3841: Re-attaching token handler after store render is well placedHooking
attachGithubTokenCollapseHandler()(and a freshcheckGitHubAuthStatus()) right afterrenderPluginStoreensures the collapse button works even after HTMX/content reloads, while theif (window.attachGithubTokenCollapseHandler)guard keeps this safe during early initialization and repeated searches.
4759-4798: openGithubTokenSettings cleanly opens and synchronizes panel stateThe function now strictly “opens” the settings: it makes the panel visible via both class and style, expands
#github-token-content, updates the chevron/text on the collapse button, hides the warning, and clears the dismissal flag before callingloadGithubToken(), which aligns with the desired UX.
4951-4958: Post-save flow correctly resets dismissal and delegates UI to status checkClearing
github-auth-warning-dismissedand then re-runningcheckGitHubAuthStatus()after a short delay is a good way to let the backend’s validation drive whether the banner/panel should be shown or collapsed, instead of hard-coding UI decisions in the save path.If there are environments where reloading the token on the backend can take noticeably longer than ~300 ms, you might want to confirm this delay is sufficient or consider calling
checkGitHubAuthStatus()immediately and letting the API reflect eventual consistency.
5089-5102: Dismiss flow hides both banner and settings; confirm accessibility of settings afterwardHiding
#github-auth-warningand, if currently shown,#github-token-settings, then persisting dismissal insessionStoragematches a “don’t bother me again this session” UX.Please double-check that, when the token is missing or invalid and the user clicks “Dismiss”, there remains an obvious way in the UI to reopen the GitHub token settings (e.g., a separate “Configure token” entry point) so configuration isn’t effectively inaccessible for the rest of the session.
- Move checkGitHubAuthStatus function definition before IIFE starts - Function was defined after IIFE but called inside it, causing it to be undefined - Now function is available when called during initialization - This should fix auto-detection of token status on page load
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
web_interface/static/v3/plugins_manager.js (6)
452-561: GitHub token status fetch + UI updates look correct; consider small robustness tweaksThe status handling (
none/invalidvsvalid) and the way you coordinate banner + settings visibility with bothclassListandstyle.displayis sound and matches the PR goals (including sessionStorage-based dismissal). Two minor improvements you might consider:
- When
authData.rate_limitis missing,rateLimit.textContentwill showundefined; defaulting to something like'0'or'-'would avoid odd strings.- If
github-auth-warningandgithub-token-settingsare both absent (e.g., plugin tab not present), you could early-return after logging to skip DOM work, though this is purely defensive.
829-834: Initializing GitHub auth status during plugin init is appropriateHooking
window.checkGitHubAuthStatus()intoinitializePlugins()ensures the token state is probed as soon as the plugin page initializes, without blocking the rest of the init flow. If you ever need to gate subsequent logic on the result, you can trivially makeinitializePluginsasyncandawaitthe returned Promise.
3944-3956: Post-store-render reattachment of GitHub handlers keeps UI in syncRe-attaching the GitHub token collapse handler and re-checking auth status after store render is a good way to handle HTMX swaps and filtered searches where the DOM is re-built. The
setTimeoutguard also avoids racing the initial render. If you want to reduce the 100ms heuristic, you could first try callingattachGithubTokenCollapseHandler()immediately and only fall back tosetTimeoutwhen the button isn’t found, but the current approach is acceptable.
4156-4253: Collapse handler cloning solves duplicate listeners; just ensure no inline onclick remainsThe pattern of cloning
#toggle-github-token-collapseto strip old listeners and then wiring a singletoggleGithubTokenContenthandler is a clean fix for the “listener pile-up” problem. The state detection using both thehiddenclass and computeddisplayalso covers prior inconsistent states.One thing to double‑check in the template: if
#toggle-github-token-collapsestill has an inlineonclickattribute, cloning will preserve it and you’ll end up with both the inline handler and this new listener firing. As long as that inline handler was removed (as implied by the PR description), this implementation is solid.
5067-5075: Refreshing status after saving token is the right hookClearing
github-auth-warning-dismissedon save and then re-runningcheckGitHubAuthStatus()after a short delay is a good way to: (a) let the backend pick up the new token, and (b) let the UI auto-hide the warning and collapse the panel when the token validates. If you see any flakiness on slow systems, you could consider a slightly longer delay or a simple retry, but this is likely sufficient.
5099-5106: Warning dismissal now also hides the settings panel; UX trade-off looks intentionalExtending
dismissGithubWarningto hide#github-token-settingsas well (both via class andstyle.display) ensures the combined banner/panel block fully disappears when the user dismisses it, and the sessionStorage flag prevents it from reappearing mid-session. BecauseopenGithubTokenSettings()still forces the panel visible and clears the dismissal state, users retain a clear way to reconfigure later. This behavior seems consistent with the new UX.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/static/v3/plugins_manager.js
🔇 Additional comments (1)
web_interface/static/v3/plugins_manager.js (1)
4874-4915: openGithubTokenSettings correctly synchronizes panel, content, and warning stateThe updated
openGithubTokenSettings:
- Makes the settings panel visible via class and inline style.
- Expands the token content and updates the collapse icon/text to the “expanded” state.
- Hides the warning banner and clears the dismissal flag so subsequent status checks behave consistently.
- Triggers a token load on open.
This matches the desired UX (explicit open action, consistent collapse state) and plays nicely with
checkGitHubAuthStatus’s auto‑collapse behavior for valid tokens.
- Add logging when checkGitHubAuthStatus is defined - Add logging when function is called during initialization - Add logging in attachGithubTokenCollapseHandler - Add logging in store render callback - This will help identify why functions aren't executing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web_interface/static/v3/plugins_manager.js (4)
452-562: GitHub status handling and UI transitions look correct; consider handling non-success responses explicitlyThe status derivation (
token_status/authenticated) and the show/hide logic for warning + settings + token content match the intended UX (none/invalid → banner, valid → hide banner, show but collapse settings, clear dismissal). DOM access is well‑guarded and sessionStorage use is consistent.As a small robustness improvement, you might:
- Handle
data.status !== 'success'by optionally surfacing a non-blocking message (or at least logging it viapluginLog) so silent backend failures don’t just result in “no banner, no status”.- Optionally gate the very chatty
console.logcalls behindPLUGIN_DEBUGonce you’re done diagnosing the initialization issues.
3955-3983: Re-attaching collapse handler after store render works; cloning button every time is slightly heavyRe-running
attachGithubTokenCollapseHandler()andcheckGitHubAuthStatus()after each successful store fetch is a good way to keep the GitHub token UI in sync with HTMX/DOM changes.Because
attachGithubTokenCollapseHandler()clones and replaces the button, calling it on every store search will repeatedly recreate that node (dropping focus and any runtime state on it). It’s not incorrect, but you could avoid extra churn by having either:
- A guard inside
attachGithubTokenCollapseHandler(e.g., set adata-collapse-handler-attachedflag on the button), or- A cheap check here to only call it when the button exists and hasn’t already been wired.
4183-4286: Collapse helper is robust; clone-based listener reset may keep inline onclicksThe new
attachGithubTokenCollapseHandler/toggleGithubTokenContentpair correctly:
- Protect against missing DOM nodes and missing handler.
- Decide expand/collapse state using both the
hiddenclass and computeddisplay.- Keep the icon and button text (“Expand”/“Collapse”) in sync with the content state.
One nuance: cloning the button (
cloneNode(true)) retains any inlineonclickattribute from the template while also attaching a newaddEventListenerhandler, so if the HTML ever has an inline handler you’ll end up with both firing. If you want to guarantee a single code path, you could strip that attribute on the clone before attaching the listener:const newBtn = toggleTokenCollapseBtn.cloneNode(true); newBtn.removeAttribute('onclick'); parent.replaceChild(newBtn, toggleTokenCollapseBtn);Not required for correctness given current templates, but makes the behavior future‑proof.
5098-5107: Refreshing token status after save is a good touch; delay is acceptable but arbitraryClearing
github-auth-warning-dismissedand then kicking offcheckGitHubAuthStatus()after a short delay ensures the banner + settings state reflects the new, validated token without requiring a page reload, which is exactly what you want here.If you later find
_validate_github_tokenuses longer-lived caching, you might either:
- Drop the delay and rely on backend cache invalidation when the token changes, or
- Extend the delay / drive the UI from explicit status in the save response.
As it stands, what you have is reasonable for the bugfix scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/static/v3/plugins_manager.js
🔇 Additional comments (3)
web_interface/static/v3/plugins_manager.js (3)
831-844: Guarded initialization call into checkGitHubAuthStatus is appropriateUsing a type check plus try/catch around
window.checkGitHubAuthStatus()ensures plugin initialization won’t be broken by transient issues in the status fetch/UI logic, while still attempting the status check when available. This is a good balance for a non-critical, UX‑oriented feature.
4906-4947: openGithubTokenSettings now matches the desired “open-only + expand content” behaviorShowing the settings panel via both class and
style.display, expandinggithub-token-content, updating the chevron and button label to “Collapse”, and hiding the warning while clearing the dismissal flag all align with the new UX. CallingloadGithubToken()here keeps the input in sync with secrets.No functional issues spotted.
5131-5138: Warning dismissal now consistently uses both class and inline styleAligning
dismissGithubWarningwith the rest of the code by toggling bothhiddenandstyle.displayon the warning (and on the settings panel when open) makes the hide/show behavior much more predictable across different callers and template defaults. The sessionStorage flag is preserved correctly.
- Move attachGithubTokenCollapseHandler and toggleGithubTokenContent outside IIFE - These functions need to be available when store renders, before IIFE completes - Add logging to initializePlugins to track when it's called - This should fix the 'undefined' error when store tries to attach handlers
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
web_interface/static/v3/plugins_manager.js (5)
452-657: GitHub token collapse + status handling looks solid; consider tightening logging and HTML injectionThe new
attachGithubTokenCollapseHandler,toggleGithubTokenContent, andcheckGitHubAuthStatusflows correctly cover:
- DOM availability checks and safe early returns.
- Robust expand/collapse state based on both classes and computed style.
- Token status differentiation (
none/invalid/valid) with appropriate warning visibility and settings-panel behavior.- Re-attaching the collapse handler after DOM updates.
Two minor improvements:
The numerous
console.logcalls here are always-on and fairly verbose. Consider gating them behind the existingpluginDebugflag (or another debug toggle) so production consoles stay cleaner.
warningText.innerHTML = ... errorMsg ...uses server-provided strings (authData.message/authData.error) directly in HTML. Even if they’re currently “safe”, it would be more defensive to settextContentor escape the message before interpolation to avoid any future HTML injection risk.
4052-4081: Re-attaching the collapse handler and refreshing token status after store render makes senseHooking
attachGithubTokenCollapseHandlerandcheckGitHubAuthStatusafter a successfulsearchPluginStorerender addresses the DOM-replacement issues:
- The clone-and-replace pattern ensures a single, fresh listener on
#toggle-github-token-collapseeven after HTMX swaps.- Calling
checkGitHubAuthStatusagain keeps the banner/settings aligned with the latest backend view.Optional tweaks:
- If you find
/github-statusgetting hit very frequently during heavy store usage, you could debounce or cache the last result in-memory and only re-check after actions that might change token validity (e.g.,saveGithubToken). Not necessary for correctness, just an optimization knob.
4281-4283: Collapse handler attachment assumes no inline handlers on the buttonCalling
attachGithubTokenCollapseHandler()fromsetupCollapsibleSectionsis a good “first attach” point, and the helper itself strips existing listeners via cloning.One thing to double‑check: if the
#toggle-github-token-collapsebutton still has any inlineonclickor framework-driven handlers,cloneNode(true)will keep those attributes and your new listener will run in addition to them, potentially double-toggling. It looks like templates were updated to rely on this JS handler, but it’s worth confirming.
5097-5106: Refreshing token status after save is correct; be aware of backend cachingClearing
github-auth-warning-dismissedand then callingcheckGitHubAuthStatusafter a short timeout is a nice way to:
- Let the backend pick up the new secrets.
- Immediately transition the banner/panel into the appropriate state (
valid→ hide banner & collapse content,invalid→ show message).If
_validate_github_tokenon the server has a non-trivial cache TTL, you might still briefly see a stale status right after saving. If that shows up in practice, consider either busting the backend cache on save or making the status endpoint ignore the cache when called from this path.
5127-5137: Warning dismissal also hides settings; ensure that matches desired UX
dismissGithubWarningnow:
- Hides the warning via both
hiddenandstyle.display = 'none'.- Hides
#github-token-settingsif it’s currently visible (also via both mechanisms).- Persists dismissal in
sessionStorage.That’s robust for the banner itself, but combined with
checkGitHubAuthStatus’s “don’t reshow if dismissed” logic it means:
- For the rest of the session, neither the warning nor the settings are auto-presented.
- If the only entry point to
openGithubTokenSettingsis inside the banner, users who dismissed it can’t re-open the settings until they reload (and thus clear the session flag).If that’s intentional (“I don’t care about configuring a token this session”), this is fine. Otherwise, consider leaving the settings card visible when dismissing, or adding a separate always-visible “GitHub token settings” entry that calls
openGithubTokenSettings().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/static/v3/plugins_manager.js
🔇 Additional comments (2)
web_interface/static/v3/plugins_manager.js (2)
915-971: Initialization guard + early token-status check are appropriateThe added logging and
checkGitHubAuthStatuscall insideinitializePluginsare well-placed:
- Guard on
pluginsInitializedprevents redundant setup across HTMX swaps.- The
typeof window.checkGitHubAuthStatuscheck plustry/catchkeeps failures from breaking overall initialization.- Not awaiting the Promise is fine since it only drives UI side effects.
No changes requested here.
4905-4945: openGithubTokenSettings behavior matches the new UX, but relies on other entry points after dismissalThe revised
openGithubTokenSettings:
- Forces the settings panel visible via both
classListandstyle.display.- Expands
github-token-contentand syncs the collapse icon/button text.- Hides the warning banner and clears the dismissal flag.
- Calls
loadGithubToken()so the field reflects current secrets.This aligns with “open only, don’t toggle” and the “edit from panel, not banner” UX.
Given
dismissGithubWarningnow hides both the warning and the settings container, please confirm that there’s at least one other clickable entry point (outside the banner) wired toopenGithubTokenSettings, otherwise in the same browser session a user who dismissed the warning may have no UI way to change the token without reloading.
- Element has 'block' class in HTML which conflicts with 'hidden' class - When hiding: add 'hidden', remove 'block', set display:none inline - When showing: remove 'hidden', add 'block', remove inline display - This ensures proper visibility toggle for the GitHub API Configuration section
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
web_interface/static/v3/plugins_manager.js (5)
452-488: GitHub collapse handler cloning approach looks sound; consider toning down loggingThe clone-and-replace pattern to clear prior listeners is a good fit for HTMX/dynamic content and should avoid duplicate handlers. The only minor nit is the always-on
console.lognoise in production; if this is no longer under active debugging, you might want to guard these logs behindPLUGIN_DEBUGor remove them.
551-652: Token-status handling is correct; add small resilience around storage and data fieldsThe status branching (
none/invalid/valid) and DOM updates match the backend contract and PR intent, including special messaging for invalid tokens and ensuring settings stay visible-but-collapsed when valid. Two small robustness ideas:
- Wrap the
sessionStoragegetItem/removeItemcalls in atry/catchin case storage is disabled or unavailable in some browser contexts.- When setting
rateLimit.textContent = authData.rate_limit, consider a default (e.g.'?') ifrate_limitisnull/undefinedto avoid rendering"undefined".Both are non-blocking polish items; the core logic looks good.
917-944: Initialization path correctly makes auth-status check optional and safeThe new
initializePluginsdebug logging plus guardedcheckGitHubAuthStatuscall are fine: you probe for the function, call it for side effects, and catch any thrown errors internally. SincecheckGitHubAuthStatusalready has its own.catch, not awaiting the Promise here is acceptable. If the extraconsole.lognoise is no longer needed, you could gate it behindPLUGIN_DEBUG.
4055-4083: Post-store-render reattachment works; watch for unnecessary repeated status callsRe-attaching the collapse handler after
renderPluginStoreis a good fix for HTMX/dynamic content—the clone-based handler ensures a clean single listener. CallingcheckGitHubAuthStatushere keeps the warning/settings in sync even if markup changes.One thing to watch: on every successful store search (including filtered queries), this will re-fetch
/github-status. If that endpoint is inexpensive this is likely fine, but if it becomes a bottleneck you could cache the last tokenStatus in JS and skip re-checks when it hasn’t changed or when the last check was very recent.
4907-4947: GitHub token settings/warning flows are consistent; consider race and UX edge casesThe combined behavior of
openGithubTokenSettings,saveGithubToken, anddismissGithubWarningmatches the PR goals:
- Opening settings makes the panel visible, expands the inner content, hides the warning, and clears any dismissal flag.
- Saving the token clears the dismissal flag and re-runs
checkGitHubAuthStatusafter a short delay so the banner/panel state reflects the new validity.- Dismissing the warning hides both the banner and the combined settings block and records the dismissal for the session.
Two minor edge cases to keep in mind:
- There’s a small race where
checkGitHubAuthStatus(triggered elsewhere) could complete after the user has just opened the panel and re-collapse it based on token status. Not a correctness bug, but if users report the panel “closing itself,” you may want a simple “user-opened” flag that suppresses auto-collapse while editing.dismissGithubWarningcurrently hides the settings block as well; if the settings container isn’t recreated or re-shown elsewhere, users might temporarily lose the visible entry point until the nextcheckGitHubAuthStatusrun that marks a valid token and forces it visible again. From the PR description it sounds like this is acceptable, but it’s worth double-checking against the template/UI wiring.Overall, the flows look coherent and improve the UX compared to the previous toggle-based approach.
Also applies to: 5099-5108, 5128-5143
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/static/v3/plugins_manager.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/config_schema.json : `config_schema.json` must define JSON schema for plugin configuration validation
🔇 Additional comments (2)
web_interface/static/v3/plugins_manager.js (2)
490-549: Collapse/expand logic correctly normalizes both classes and inlinedisplayThe new
toggleGithubTokenContentcorrectly derives visibility from bothhiddenclass and computeddisplay, and then keeps classList andstyle.displayin sync while updating icon and label text. This should fix the prior “stuck open/closed” states for the content panel.
4283-4286: Initial collapse-handler attachment is a good safety netHooking
attachGithubTokenCollapseHandlerfromsetupCollapsibleSectionsensures the collapse button works even before the store render hook runs, and the helper’s clone logic should make multiple calls safe. No functional concerns here.
- Call checkGitHubAuthStatus immediately when script loads (if elements exist) - Call it early in initPluginsPage (before full initialization completes) - Use requestAnimationFrame instead of setTimeout(100ms) for store render callback - Reduce save token delay from 300ms to 100ms - Token detection now happens in parallel with other initialization tasks - This makes token status visible much faster on page load
- Fix Installed Plugins section collapse/expand button - Fix Plugin Store section collapse/expand button - Fix GitHub Install section collapse/expand button - Apply same fixes as GitHub token button: * Clone buttons to remove existing listeners * Handle block/hidden class conflicts properly * Add proper event prevention (stopPropagation/preventDefault) * Add logging for debugging - All collapse/expand buttons should now work correctly
- Ensure all handler setup code is inside the function - Add comment to mark function end clearly
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
web_interface/static/v3/plugins_manager.js (6)
452-489: GitHub token collapse handler/content toggle logic looks solid, minor consistency nitThe clone-node pattern to strip old listeners and the combined use of
classList+style.display+ computed style checks should address the previous “stuck open/closed” issues and event duplication. One small consistency tweak: whenopenGithubTokenSettingsexpands the content it only removeshidden/displaybut doesn’t add theblockclass the waytoggleGithubTokenContentdoes, which could diverge behavior if CSS changes. Consider adding the sameblockclass there for symmetry and future-proofing.Also applies to: 490-549
551-659: GitHub token status orchestration is correct but somewhat over-eager; consider centralizing callsThe new
checkGitHubAuthStatusflow correctly:
- Derives
tokenStatus(none/invalid/valid),- Shows the warning only for
none/invalidand respects the session-dismiss flag,- Hides the warning + collapses the panel for
valid, and- Ensures the collapse handler is (re)attached after DOM changes.
Multiple call sites (early in
initPluginsPage, insideinitializePlugins, after store render, after save, and at script load) are all safe but somewhat redundant. You could slightly simplify by choosing one or two well-defined lifecycle hooks (e.g., after plugin DOM load + after saving the token) as the only triggers, to reduce duplicate fetches and console noise.Also applies to: 779-785, 925-951, 4062-4091, 5225-5235, 5981-5985
4243-4303: Collapsible sections refactor via cloning is soundUsing
cloneNode(true)+ replace to clear old listeners and then wiring fresh click handlers for the Installed, Store, and GitHub-install sections is a good way to avoid accumulation of duplicate listeners across HTMX swaps. The show/hide logic that combines Tailwind classes with inlinedisplayalso matches the new GitHub token patterns.One minor cleanup: several locals like
hasBlockClassare computed but unused; you can drop them to reduce noise.Also applies to: 4311-4364, 4459-4519
5033-5073: openGithubTokenSettings behavior aligns with UX goals, with two small nitsBehavior matches the PR intent:
- Ensures the settings panel is visible,
- Expands the token content when explicitly opened,
- Updates icon/text to “Collapse”, and
- Hides the warning + clears the dismissal flag, which is what you want when the user is actively configuring.
Two minor follow-ups:
- As noted earlier, also add the
blockclass ontokenContentfor consistency withtoggleGithubTokenContent.- Optional: call
attachGithubTokenCollapseHandler()here if the collapse button might exist before store render (to be extra defensive against template changes).
452-456: Consider gating new console logging behind the existing debug flagsThere are many new unconditional
console.log/console.warncalls (especially around the GitHub token helpers and collapsible sections) whereas most of this file already uses_PLUGIN_DEBUG_EARLY/PLUGIN_DEBUG+pluginLogfor debug output. For a quieter production console, you may want to wrap these new logs with those flags or route them throughpluginLog.Also applies to: 491-495, 556-558, 4243-4255, 4311-4315, 4459-4471, 5981-5985
4942-4967: Duplicate globalformatDatedefinitions could be unifiedThere are two different
formatDatefunctions in this file; the later one overwrites the earlier definition. While unchanged in this PR, it’s easy to get surprising behavior as different call sites may have been written with different semantics in mind (relative “Today / X days ago” vs. absolute timestamp).Not urgent for this PR, but consider consolidating into one well-documented helper or renaming one of them to make the intent clear.
Also applies to: 5697-5705
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web_interface/static/v3/plugins_manager.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/config_schema.json : `config_schema.json` must define JSON schema for plugin configuration validation
🔇 Additional comments (1)
web_interface/static/v3/plugins_manager.js (1)
5225-5235: Post-save status refresh + dismissal reset is correctClearing
github-auth-warning-dismissedbefore re-checking status and shortening the delay to 100ms are both sensible; it ensures the banner/panel state is recomputed solely from backend token validity and not stuck on previous dismissal.
…Plugin Store - Remove collapse/expand buttons from Installed Plugins section - Remove collapse/expand buttons from Plugin Store section - Remove related JavaScript handler code - These sections are now always visible for better UX - GitHub token section still has collapse functionality
Description
Fixes issue where GitHub token warnings and settings panel would not properly hide/show after saving a token. Also improves the UX by removing the close button and ensuring auto-detection of token status.
Changes
Backend
_validate_github_token()method in PluginStoreManager with caching/api/v3/plugins/store/github-statusendpoint to return token status: 'none', 'valid', or 'invalid'Frontend
classListandstyle.displaytoggleGithubTokenSettings()withopenGithubTokenSettings()that only opens panelTesting
Type of Change
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.