Codebase audit: fix shipping bugs, remove verified-dead code, repair doc drift, add regression guards - #438
Merged
Merged
Conversation
…invalidates api_v3.py's font upload/delete handlers import delete_cached from web_interface.cache, but the function was never defined. The surrounding except ImportError silently swallowed the failure, so the fonts_catalog cache entry survived uploads/deletes and newly uploaded fonts did not appear until the TTL expired or the service restarted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
The partial dispatcher still routed 'weather' and 'stocks' to loaders rendering v3/partials/weather.html and stocks.html — templates that no longer exist since weather and stocks became store plugins. Requesting either partial raised TemplateNotFound, which the catch-all turned into a 500. No template or JS references these partials (the only 'weather' hit in the front end is a plugin-store category filter option), so the branches and both loader functions are removed; unknown partials now fall through to the existing 404 handler. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
requirements.txt's optional-install comment recommended psutil>=5.9,<6.0 while web_interface/requirements.txt hard-requires >=6.0,<7.0 — anyone following the comment ends up with an unsatisfiable pair. The comment now recommends the same range the web interface requires (all psutil APIs used — Process, boot_time, cpu_percent, disk_usage, virtual_memory — are stable in 6.x). Flask-Limiter gains the same <4.0 cap in both files and freetype-py the same >=2.5.1 floor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
display.hardware gains pixel_mapper_config, row_address_type, multiplexing and panel_type (read at display_manager.py with these exact fallbacks — users on non-standard panels previously had no way to discover them from the template). vegas_scroll gains frame_based_scrolling and scroll_delay, the only two of its 27 keys the template omitted (read in src/vegas_mode/config.py). plugin_system gains development_mode, which the web UI reads and writes but the template never declared. Every added value is byte-identical to the code-side .get() fallback, so ConfigManager._migrate_config() merging these keys into existing user configs cannot change behavior on any installed device. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
clear_cache.py and download_nba_logos.py pointed sys.path at a 'src' directory relative to the script's own folder (scripts/utils/src and scripts/src — neither exists), so both crashed on import; they now insert the project root and import via the src package like the other scripts. debug_web_manual.py resolved 'project root' to scripts/debug/ instead of two levels up. fix_nhl_cache.sh is removed: it used Python docstring syntax in a bash script and invoked clear_nhl_cache.py, which does not exist anywhere in the repo — it cannot ever have worked in its current location. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
…radiction CLAUDE.md and .cursorrules disagreed about plugin-directory fallback behavior; the code (SchemaManager.get_schema_path) probes plugins/ BEFORE plugin-repos/, and the main discovery path has no fallback at all — both files now describe the real behavior, preferring symbol names over line numbers so the references rot slower. REST_API_REFERENCE.md pointed at app.py:144/:607 for mounts that live at :199/:799 and counted 92 routes where there are 94. PLUGIN_ARCHITECTURE_SPEC.md's historical banner gains a note that its example imports (src/plugin_system/base_classes/*_plugin.py) never shipped — the real base classes are src.base_classes.sports.SportsCore and src.base_classes.hockey.Hockey. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
…iption Repairs every broken relative link in active docs (targets renamed or archived long ago: PLUGIN_DEVELOPMENT.md -> PLUGIN_DEVELOPMENT_GUIDE.md, API_REFERENCE.md -> REST_API_REFERENCE.md, PLUGIN_STORE_USER_GUIDE.md -> PLUGIN_STORE_GUIDE.md, plugin_docs/ dir, TROUBLESHOOTING_QUICK_START.md, and MIGRATION_GUIDE's README link that silently resolved to the docs index instead of the project README). Replaces commands invoking scripts that do not exist (scripts/update_stats.py, validate_registry.py, check_updates.py, fix_permissions.sh) with the real tooling, and rewrites HOW_TO_RUN_TESTS.md's CI section, which described a security-audit workflow that was never committed and a pytest workflow 'queued to land' that landed long ago as test.yml. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
docs/README.md's own policy says every page must be linked from the index, yet five weren't — including the entire skin system (SKIN_SYSTEM.md, CREATING_SKINS.md), ADAPTIVE_LAYOUT.md, plugin-safety-harness.md and SPORTS_UNIFICATION.md. Each is now listed in the section it belongs to, and PLUGIN_ARCHITECTURE_SPEC.md is marked historical in the index (the doc itself already carries the banner). web_interface/README.md's static/v3 tree showed only app.css/app.js; it now reflects the actual contents. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
…ore plugins - src/common/cli.py: imports a 'ledmatrix_common' package that exists nowhere (not in this repo, any requirements file, or the plugin monorepo), so it cannot ever have run; its README section claimed scripts/dev/* used it, which was also untrue. - src/web_interface/logging_config.py: zero callers — the web app uses web_interface/logging_config.py (a different module), and nothing imports the src copy. - handle_errors decorator in src/web_interface/error_handler.py: zero call sites (the module's response helpers stay — they are used). - ConfigManager.get_clock_config(): reads a 'clock' config key that no longer exists anywhere; only caller was its own unit test. Deliberately kept despite zero in-repo callers: DisplayError, src/common/config_helper.py and display_helper.py — all documented as plugin-facing API (docs/PLUGIN_ERROR_HANDLING.md, src/common/README.md), and third-party plugins outside the official monorepo cannot be enumerated. Verified against a fresh clone of ledmatrix-plugins (43 plugins): zero references to any removed symbol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
The four test_nba_*.py files imported nba_managers, leaderboard_manager and odds_manager — top-level modules deleted when sports displays became plugins — inside try/except blocks that swallowed the ImportError, so they passed while exercising nothing. test_nba_data_structure.py and debug_nba_api.py (a diagnostic script living in test/) made live ESPN API calls rather than testing repo code. None were enrolled in CI. scripts/debug/direct_fix_imports.py and check_imports.py were one-shot artifacts that edited/inspected a hardcoded ~/LEDMatrix/web_interface/ app.py to fix an import problem solved long ago; nothing references them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
…bs that do not exist The script's only function is to merge JSON artifacts (bandit/semgrep/pip-audit/safety/gitleaks results) produced by a security-audit workflow that was never committed — .github/workflows/ has no such jobs, so there is nothing for it to aggregate and no way to run it usefully. Its siblings stay: prove_security.py and audit_plugins.py both run standalone (verified), and .codacy.yml stays because the Codacy service (README badge) reads it server-side without a workflow file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
time-picker.js, file-upload-single.js and plugin-file-manager.js register widgets that installed store plugins reference in their config schemas (countdown uses x-widget: time-picker and file-upload-single; of-the-day uses plugin-file-manager), but base.html never included the scripts. plugin_config.html renders such fields as an empty container that polls LEDMatrixWidgets.get(...) on a 50ms loop forever, so those plugin config fields appeared permanently blank. The audit initially flagged these files as dead code; the monorepo cross-check proved the opposite — they were unreachable, not unused. example-color-picker.js (the documented custom-widget example) gains an explicit warning that including it in base.html would shadow the built-in color-picker widget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
No code reads a top-level youtube secrets key: the youtube-stats plugin receives its API key namespaced under its own plugin id (declared via x-secret in its config schema), like every other store plugin. The key survives only in state_reconciliation.py's non-plugin-key exclusion set, which stays — existing installs still carry the key in their generated config_secrets.json, and the exclusion prevents it from being misclassified as a plugin config. New installs simply stop being asked for a YouTube API key they have nowhere to use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
… move mypy to test deps Removed from requirements.txt: python-socketio, python-engineio, websockets, websocket-client — zero imports anywhere in this repo, and the one store plugin that needs Socket.IO (ledmatrix-music) declares it in its own requirements.txt, which the plugin store installs. Removed the same quartet plus timezonefinder, geopy, google-auth-oauthlib, google-auth-httplib2, google-api-python-client, unidecode, icalevents, python-dateutil, flask-wtf and the werkzeug pin from web_interface/requirements.txt — all leftovers from the deleted built-in weather/calendar/music displays (flask-wtf was doubly dead: app.py explicitly disables CSRF and sets csrf=None). scripts/ install_dependencies_apt.py, which mirrors these lists for the first-time installer, drops the same packages. Added: urllib3 (imported directly in four core modules), jinja2 and markupsafe (imported directly in pages_v3.py) — previously reachable only as transitives. mypy moves from runtime requirements to requirements-test.txt. Verified in a fresh venv: all four requirements files co-install, pip check is clean, the full CI-enrolled suite (907 tests) and a Flask boot smoke pass with the trimmed dependency set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
src/cache_manager.py and src/cache/disk_cache.py each defined an identical DateTimeEncoder (datetime -> ISO-8601). The disk_cache copy is the only one actually used for serialization; cache_manager now re-exports it instead of defining a twin, so the two can never silently diverge. Import compatibility is preserved — from src.cache_manager import DateTimeEncoder still works and is the same class object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
The audit surfaced several near-duplicate implementations that turned out to be either deliberate forks or behaviorally different — merging any of them would risk changing behavior on installed devices, so each now carries an explicit comment stating the relationship: - VisualDisplayManager: headless fork of DisplayManager; header now lists the ~15 mirrored methods and warns that DisplayManager changes must be mirrored. - normalize_abbreviation: LogoDownloader's version (called directly by nine scoreboard plugins) replaces filesystem-unsafe characters; LogoHelper's strips spaces. Logo filenames on existing installs depend on both behaviors staying put. - The two PluginTestBase classes: the shipped one is plugin-author API, the repo's own richer harness lives in test/plugins/ — now cross-referenced. Also verified (no change needed): ConfigManager's backup/rollback methods genuinely delegate to AtomicConfigManager, and SportsCore already delegates _read_bdf_native_size to FontManager. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
There was no single place documenting what lives in config.json — display.* keys were scattered across README sections, vegas_scroll lived in ADVANCED_FEATURES.md, and dim_schedule, display.double_sided, sync.follower_position, plugin_system.development_mode and the four newly-templated hardware keys were documented nowhere. CONFIG_REFERENCE.md now lists every template key plus the code-read-only keys, each with type, default, and the code location that reads it, and explains the secrets file's plugin-id namespacing. Linked from the docs index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
The Core Features section still presented clock/weather/sports/stocks/ music displays as built into the project, when all of them are store plugins installed from the ledmatrix-plugins monorepo — only starlark-apps and web-ui-info ship in this repo. The intro now says so (the showcase itself is unchanged; those are real displays available in the store). The display_durations reference drops its built-in-calendar example in favor of plugin-id keys, and the Configuration section links the new CONFIG_REFERENCE.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
… document assets/ PLUGIN_CUSTOM_ICONS_FEATURE.md was a 'What Was Implemented' status report duplicating the actual guide (PLUGIN_CUSTOM_ICONS.md) — moved to docs/archive/ per the docs index's own policy. The overlapping plugin-config docs keep their content but PLUGIN_CONFIG_ARCHITECTURE.md now states up front which doc is canonical for which purpose. assets/README.md is new and load-bearing: assets/stocks, weather, news_logos and broadcast_logos have zero references in this repo's code, which makes them look deletable — but store plugins (ledmatrix-stocks, ledmatrix-weather, news, odds-ticker) resolve those exact paths at runtime against the install directory. The README records that evidence so a future cleanup doesn't break installed plugins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
Three lightweight static checks, all enrolled in CI's unit-test allowlist along with the new web-cache test: - test_template_targets.py: every literal render_template() target must exist (would have caught the weather/stocks partial 500s at commit time). - test_widget_scripts.py: every widget JS file must be script-included in base.html or explicitly allowlisted with a reason (would have caught the unloaded time-picker/file-upload-single/plugin-file-manager widgets), and allowlisted files must NOT be included (prevents the example widget from shadowing the real color-picker). - test_doc_links.py: relative markdown links in active docs must resolve (docs/archive/ exempt). Each guard was verified to fail against the pre-PR tree and pass now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change aligns plugin and configuration documentation, removes obsolete dependencies and utilities, updates web-interface behavior, adds widget and cache safeguards, and expands CI coverage for documentation, templates, widgets, and caching. ChangesArchitecture alignment
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -5 |
| Duplication | -1 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Commit 1ec22db removed the werkzeug>=3.1.6,<4.0.0 pin along with the genuinely-unused packages, but this one was a version floor on Flask's transitive dependency, not a phantom: Flask 3.1.3 itself only requires werkzeug>=3.1.0, so dropping the pin let fresh installs resolve 3.1.0-3.1.5. Restored with a comment explaining why it exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
…-picker registration pixel_mapper_config was the only display.hardware key absent from both the display_fields detection allowlist and the hardware write loop in the settings save path. No form posts it today, but if one ever did the key would fall through to the generic handler and land at the TOP level of config.json — where state_reconciliation would mistake it for a missing plugin id and loop auto-repair attempts (the failure class the 'github'/'youtube' exclusion comment documents). It now round-trips into display.hardware like its siblings. time-picker.js gains the same LEDMatrixWidgets-undefined guard its two sibling widgets already have; correct today only via defer ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
first_time_install.sh's fallback secrets heredoc (used only when the template is missing) still wrote the legacy youtube block — now matches the template (github only). install_dependencies_apt.py drops the IMPORT_NAME_MAP entries for packages no longer in its install lists and a stale google-api reference in a docstring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
Owner
Author
Re-verification: no production functionality removedAt the maintainer's request, two independent adversarial reviews re-examined all 20 commits, each explicitly tasked with proving a change breaks something. Every deletion, dependency removal, and behavior change came back SAFE. Highlights of the evidence:
The reviews surfaced four small hardening items, now pushed as three follow-up commits:
All 916 CI-enrolled tests pass after the follow-ups. Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install_dependencies_apt.py (1)
98-112: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlign the FreeType package name with the declared dependency.
requirements.txtrequiresfreetype-py, butscripts/install_dependencies_apt.py#L142installs and checks forfreetype. If the APT package is unavailable,pip install freetypeinstalls a different package instead offreetype-py, which satisfies the manifest and imports asfreetype.Proposed mapping
- 'freetype': 'python3-freetype', + 'freetype-py': 'python3-freetype', -IMPORT_NAME_MAP = {} +IMPORT_NAME_MAP = { + 'freetype-py': 'freetype', +} - 'freetype', + 'freetype-py',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install_dependencies_apt.py` around lines 98 - 112, Align the FreeType dependency handling in check_package_installed and the related installation flow with requirements.txt: map the declared package name freetype-py to its import name freetype, and ensure the fallback pip installation uses freetype-py rather than freetype. Apply this in scripts/install_dependencies_apt.py; requirements.txt requires no direct change.Sources: Coding guidelines, MCP tools
🧹 Nitpick comments (2)
requirements.txt (1)
56-58: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueKeep the root
requirements.txtcomment in sync with the dependency source.
requirements.txtonly repeatspsutil>=6.0.0,<7.0.0in a comment, whileweb_interface/requirements.txtdeclares the live dependency. Update the note to reference the active requirement manifest instead of implyingrequirements.txtenforces it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@requirements.txt` around lines 56 - 58, Update the psutil comment in the root requirements.txt to clarify that web_interface/requirements.txt is the active dependency manifest defining the >=6.0.0,<7.0.0 range, rather than implying the root file enforces it; keep the stated version range synchronized.test/test_doc_links.py (1)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd return type hints to the new test helpers and test functions.
test/test_doc_links.py#L15-L22: annotate_md_files()andtest_relative_markdown_links_resolve().test/test_template_targets.py#L15-L20: annotate_python_sources()andtest_all_literal_render_template_targets_exist().test/web_interface/test_cache.py#L7-L38: annotateclean_cache()and each test function.Use
Iterator[Path]for file generators,Iterator[None]for the yielding fixture, andNonefor test functions.As per coding guidelines, “Use type hints for function parameters and return values.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_doc_links.py` around lines 15 - 22, Annotate the new test helpers and functions with return types: in test/test_doc_links.py lines 15-22, use Iterator[Path] for _md_files() and None for test_relative_markdown_links_resolve(); in test/test_template_targets.py lines 15-20, use Iterator[Path] for _python_sources() and None for test_all_literal_render_template_targets_exist(); in test/web_interface/test_cache.py lines 7-38, use Iterator[None] for clean_cache() and None for each test function.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.cursorrules:
- Around line 6-20: Update the “Plugin Loading Process” section and the “File
Organization” tree to match PluginManager.discover_plugins(): describe discovery
through plugin_system.plugins_directory, using plugin-repos/ as the default, and
label plugins/ only as the development symlink location where applicable. Remove
contradictory wording that implies the main discovery path always scans
plugins/.
In `@config/config.template.json`:
- Line 113: Align the fallback for limit_refresh_rate_hz in the display manager
with the documented configuration default of 100. Update the runtime default
used by the relevant configuration lookup in src/display_manager.py, or
consistently revise the template and README if 90 is intentional; ensure the
documented and runtime defaults no longer conflict.
In `@docs/ADVANCED_FEATURES.md`:
- Around line 987-991: Update the permission-script documentation block to
ensure all four referenced scripts—fix_assets_permissions.sh,
fix_cache_permissions.sh, fix_plugin_permissions.sh, and
fix_web_permissions.sh—can be invoked by the documented ./ commands; either mark
the script files executable or change each command to invoke bash explicitly,
keeping the commands consistent.
In `@docs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.md`:
- Around line 194-200: Update both visual-example fenced blocks surrounding the
“Before” and “After” custom-icon examples to use the text language label,
preserving their existing contents.
- Around line 304-309: Update the “Backward Compatibility” section’s wording
from “Fully backward compatible” to “Fully backward-compatible,” preserving the
surrounding content.
- Around line 32-40: Update the manifest examples in
PLUGIN_CUSTOM_ICONS_FEATURE.md, including the quick-reference block, so every
block labeled JSON is valid, copyable JSON: remove inline comments and ellipsis
placeholders, and replace standalone icon-property fragments with complete
manifest objects. Preserve the documented icon values and example intent.
- Around line 81-91: Align the Clock Simple icon documentation by updating
either the manifest example near “clock-simple plugin” or the expected output in
the testing instructions to use the same icon value, preserving the intended
documented behavior consistently across both sections.
In `@docs/CONFIG_REFERENCE.md`:
- Line 58: Update the hardware_mapping entry in CONFIG_REFERENCE.md to
distinguish the configuration template value from the runtime fallback used by
display_manager.py, documenting that the missing-key default is
"adafruit-hat-pwm" while preserving the existing table format.
In `@docs/PLUGIN_REGISTRY_SETUP_GUIDE.md`:
- Around line 326-341: Update the documented check_plugin.py command in the
plugin setup guide to pass the plugin identifier using its supported -p or
--plugin option instead of a bare positional argument; leave the other
documented commands unchanged.
In `@README.md`:
- Around line 53-60: Update the plugin inventory paragraph around “everything
else lives” to qualify the statement as applying to official or most plugins,
and explicitly note that the core calendar is an exception to the plugin model.
Keep the existing bundled-plugin and repository references intact.
In `@test/test_template_targets.py`:
- Around line 23-25: Update the source scan in the test’s template-target
validation to run RENDER_RE.finditer(text) over the complete text instead of
processing each line separately. For each match, calculate lineno from the match
offset and preserve the existing target validation behavior, including support
for render_template calls split across lines.
- Around line 30-32: Remove the unnecessary f-string prefix from the string
literal in the render_template() assertion message, while preserving the
existing concatenation with the joined missing-template names.
In `@test/test_widget_scripts.py`:
- Around line 23-26: Update test_every_widget_script_is_included_in_base_html to
assert WIDGETS_DIR.is_dir() before calling glob, so missing or moved widget
assets fail explicitly instead of yielding an empty iteration.
- Around line 29-43: Update both widget inclusion checks in
test_widget_scripts.py to inspect only actual HTML <script> elements and their
src values, rather than searching the full BASE_HTML text for path substrings.
Reuse the extracted script-source collection in the missing and wrongly_included
calculations while preserving the existing assertions and allowlist behavior.
In `@web_interface/blueprints/api_v3.py`:
- Line 810: Validate pixel_mapper_config as a string in the API request handling
before the value is assigned to display.hardware; reject object, list, and other
non-string values while preserving valid string mapper specifications. Use the
existing field validation flow near the row_address_type and pixel_mapper_config
request keys.
---
Outside diff comments:
In `@scripts/install_dependencies_apt.py`:
- Around line 98-112: Align the FreeType dependency handling in
check_package_installed and the related installation flow with requirements.txt:
map the declared package name freetype-py to its import name freetype, and
ensure the fallback pip installation uses freetype-py rather than freetype.
Apply this in scripts/install_dependencies_apt.py; requirements.txt requires no
direct change.
---
Nitpick comments:
In `@requirements.txt`:
- Around line 56-58: Update the psutil comment in the root requirements.txt to
clarify that web_interface/requirements.txt is the active dependency manifest
defining the >=6.0.0,<7.0.0 range, rather than implying the root file enforces
it; keep the stated version range synchronized.
In `@test/test_doc_links.py`:
- Around line 15-22: Annotate the new test helpers and functions with return
types: in test/test_doc_links.py lines 15-22, use Iterator[Path] for _md_files()
and None for test_relative_markdown_links_resolve(); in
test/test_template_targets.py lines 15-20, use Iterator[Path] for
_python_sources() and None for test_all_literal_render_template_targets_exist();
in test/web_interface/test_cache.py lines 7-38, use Iterator[None] for
clean_cache() and None for each test function.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b7ea2f6-3b1b-4e96-b9a9-3017877e948e
📒 Files selected for processing (62)
.cursorrules.github/workflows/test.ymlCLAUDE.mdREADME.mdassets/README.mdconfig/config.template.jsonconfig/config_secrets.template.jsondocs/ADVANCED_FEATURES.mddocs/CONFIG_REFERENCE.mddocs/DEVELOPER_QUICK_REFERENCE.mddocs/HOW_TO_RUN_TESTS.mddocs/MIGRATION_GUIDE.mddocs/PLUGIN_ARCHITECTURE_SPEC.mddocs/PLUGIN_CONFIG_ARCHITECTURE.mddocs/PLUGIN_CUSTOM_ICONS.mddocs/PLUGIN_DEPENDENCY_TROUBLESHOOTING.mddocs/PLUGIN_DEVELOPMENT_GUIDE.mddocs/PLUGIN_REGISTRY_SETUP_GUIDE.mddocs/PLUGIN_STORE_GUIDE.mddocs/README.mddocs/REST_API_REFERENCE.mddocs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.mdfirst_time_install.shrequirements-test.txtrequirements.txtscripts/debug/check_imports.pyscripts/debug/debug_web_manual.pyscripts/debug/direct_fix_imports.pyscripts/download_nba_logos.pyscripts/fix_perms/README.mdscripts/fix_perms/fix_nhl_cache.shscripts/generate_report.pyscripts/install_dependencies_apt.pyscripts/utils/clear_cache.pysrc/cache_manager.pysrc/common/README.mdsrc/common/cli.pysrc/common/logo_helper.pysrc/config_manager.pysrc/logo_downloader.pysrc/plugin_system/testing/plugin_test_base.pysrc/plugin_system/testing/visual_display_manager.pysrc/web_interface/error_handler.pysrc/web_interface/logging_config.pytest/debug_nba_api.pytest/test_config_manager.pytest/test_doc_links.pytest/test_nba_core_functionality.pytest/test_nba_data_structure.pytest/test_nba_integration.pytest/test_nba_leaderboard_fix.pytest/test_template_targets.pytest/test_widget_scripts.pytest/web_interface/test_cache.pyweb_interface/README.mdweb_interface/blueprints/api_v3.pyweb_interface/blueprints/pages_v3.pyweb_interface/cache.pyweb_interface/requirements.txtweb_interface/static/v3/js/widgets/example-color-picker.jsweb_interface/static/v3/js/widgets/time-picker.jsweb_interface/templates/v3/base.html
💤 Files with no reviewable changes (17)
- scripts/fix_perms/README.md
- src/common/README.md
- src/config_manager.py
- first_time_install.sh
- test/test_nba_data_structure.py
- src/web_interface/logging_config.py
- test/test_nba_core_functionality.py
- test/test_nba_integration.py
- test/test_config_manager.py
- scripts/debug/direct_fix_imports.py
- scripts/debug/check_imports.py
- web_interface/blueprints/pages_v3.py
- test/test_nba_leaderboard_fix.py
- scripts/generate_report.py
- src/common/cli.py
- scripts/fix_perms/fix_nhl_cache.sh
- test/debug_nba_api.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install_dependencies_apt.py (1)
98-112: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlign the FreeType package name with the declared dependency.
requirements.txtrequiresfreetype-py, butscripts/install_dependencies_apt.py#L142installs and checks forfreetype. If the APT package is unavailable,pip install freetypeinstalls a different package instead offreetype-py, which satisfies the manifest and imports asfreetype.Proposed mapping
- 'freetype': 'python3-freetype', + 'freetype-py': 'python3-freetype', -IMPORT_NAME_MAP = {} +IMPORT_NAME_MAP = { + 'freetype-py': 'freetype', +} - 'freetype', + 'freetype-py',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install_dependencies_apt.py` around lines 98 - 112, Align the FreeType dependency handling in check_package_installed and the related installation flow with requirements.txt: map the declared package name freetype-py to its import name freetype, and ensure the fallback pip installation uses freetype-py rather than freetype. Apply this in scripts/install_dependencies_apt.py; requirements.txt requires no direct change.Sources: Coding guidelines, MCP tools
🧹 Nitpick comments (2)
requirements.txt (1)
56-58: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueKeep the root
requirements.txtcomment in sync with the dependency source.
requirements.txtonly repeatspsutil>=6.0.0,<7.0.0in a comment, whileweb_interface/requirements.txtdeclares the live dependency. Update the note to reference the active requirement manifest instead of implyingrequirements.txtenforces it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@requirements.txt` around lines 56 - 58, Update the psutil comment in the root requirements.txt to clarify that web_interface/requirements.txt is the active dependency manifest defining the >=6.0.0,<7.0.0 range, rather than implying the root file enforces it; keep the stated version range synchronized.test/test_doc_links.py (1)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd return type hints to the new test helpers and test functions.
test/test_doc_links.py#L15-L22: annotate_md_files()andtest_relative_markdown_links_resolve().test/test_template_targets.py#L15-L20: annotate_python_sources()andtest_all_literal_render_template_targets_exist().test/web_interface/test_cache.py#L7-L38: annotateclean_cache()and each test function.Use
Iterator[Path]for file generators,Iterator[None]for the yielding fixture, andNonefor test functions.As per coding guidelines, “Use type hints for function parameters and return values.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_doc_links.py` around lines 15 - 22, Annotate the new test helpers and functions with return types: in test/test_doc_links.py lines 15-22, use Iterator[Path] for _md_files() and None for test_relative_markdown_links_resolve(); in test/test_template_targets.py lines 15-20, use Iterator[Path] for _python_sources() and None for test_all_literal_render_template_targets_exist(); in test/web_interface/test_cache.py lines 7-38, use Iterator[None] for clean_cache() and None for each test function.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.cursorrules:
- Around line 6-20: Update the “Plugin Loading Process” section and the “File
Organization” tree to match PluginManager.discover_plugins(): describe discovery
through plugin_system.plugins_directory, using plugin-repos/ as the default, and
label plugins/ only as the development symlink location where applicable. Remove
contradictory wording that implies the main discovery path always scans
plugins/.
In `@config/config.template.json`:
- Line 113: Align the fallback for limit_refresh_rate_hz in the display manager
with the documented configuration default of 100. Update the runtime default
used by the relevant configuration lookup in src/display_manager.py, or
consistently revise the template and README if 90 is intentional; ensure the
documented and runtime defaults no longer conflict.
In `@docs/ADVANCED_FEATURES.md`:
- Around line 987-991: Update the permission-script documentation block to
ensure all four referenced scripts—fix_assets_permissions.sh,
fix_cache_permissions.sh, fix_plugin_permissions.sh, and
fix_web_permissions.sh—can be invoked by the documented ./ commands; either mark
the script files executable or change each command to invoke bash explicitly,
keeping the commands consistent.
In `@docs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.md`:
- Around line 194-200: Update both visual-example fenced blocks surrounding the
“Before” and “After” custom-icon examples to use the text language label,
preserving their existing contents.
- Around line 304-309: Update the “Backward Compatibility” section’s wording
from “Fully backward compatible” to “Fully backward-compatible,” preserving the
surrounding content.
- Around line 32-40: Update the manifest examples in
PLUGIN_CUSTOM_ICONS_FEATURE.md, including the quick-reference block, so every
block labeled JSON is valid, copyable JSON: remove inline comments and ellipsis
placeholders, and replace standalone icon-property fragments with complete
manifest objects. Preserve the documented icon values and example intent.
- Around line 81-91: Align the Clock Simple icon documentation by updating
either the manifest example near “clock-simple plugin” or the expected output in
the testing instructions to use the same icon value, preserving the intended
documented behavior consistently across both sections.
In `@docs/CONFIG_REFERENCE.md`:
- Line 58: Update the hardware_mapping entry in CONFIG_REFERENCE.md to
distinguish the configuration template value from the runtime fallback used by
display_manager.py, documenting that the missing-key default is
"adafruit-hat-pwm" while preserving the existing table format.
In `@docs/PLUGIN_REGISTRY_SETUP_GUIDE.md`:
- Around line 326-341: Update the documented check_plugin.py command in the
plugin setup guide to pass the plugin identifier using its supported -p or
--plugin option instead of a bare positional argument; leave the other
documented commands unchanged.
In `@README.md`:
- Around line 53-60: Update the plugin inventory paragraph around “everything
else lives” to qualify the statement as applying to official or most plugins,
and explicitly note that the core calendar is an exception to the plugin model.
Keep the existing bundled-plugin and repository references intact.
In `@test/test_template_targets.py`:
- Around line 23-25: Update the source scan in the test’s template-target
validation to run RENDER_RE.finditer(text) over the complete text instead of
processing each line separately. For each match, calculate lineno from the match
offset and preserve the existing target validation behavior, including support
for render_template calls split across lines.
- Around line 30-32: Remove the unnecessary f-string prefix from the string
literal in the render_template() assertion message, while preserving the
existing concatenation with the joined missing-template names.
In `@test/test_widget_scripts.py`:
- Around line 23-26: Update test_every_widget_script_is_included_in_base_html to
assert WIDGETS_DIR.is_dir() before calling glob, so missing or moved widget
assets fail explicitly instead of yielding an empty iteration.
- Around line 29-43: Update both widget inclusion checks in
test_widget_scripts.py to inspect only actual HTML <script> elements and their
src values, rather than searching the full BASE_HTML text for path substrings.
Reuse the extracted script-source collection in the missing and wrongly_included
calculations while preserving the existing assertions and allowlist behavior.
In `@web_interface/blueprints/api_v3.py`:
- Line 810: Validate pixel_mapper_config as a string in the API request handling
before the value is assigned to display.hardware; reject object, list, and other
non-string values while preserving valid string mapper specifications. Use the
existing field validation flow near the row_address_type and pixel_mapper_config
request keys.
---
Outside diff comments:
In `@scripts/install_dependencies_apt.py`:
- Around line 98-112: Align the FreeType dependency handling in
check_package_installed and the related installation flow with requirements.txt:
map the declared package name freetype-py to its import name freetype, and
ensure the fallback pip installation uses freetype-py rather than freetype.
Apply this in scripts/install_dependencies_apt.py; requirements.txt requires no
direct change.
---
Nitpick comments:
In `@requirements.txt`:
- Around line 56-58: Update the psutil comment in the root requirements.txt to
clarify that web_interface/requirements.txt is the active dependency manifest
defining the >=6.0.0,<7.0.0 range, rather than implying the root file enforces
it; keep the stated version range synchronized.
In `@test/test_doc_links.py`:
- Around line 15-22: Annotate the new test helpers and functions with return
types: in test/test_doc_links.py lines 15-22, use Iterator[Path] for _md_files()
and None for test_relative_markdown_links_resolve(); in
test/test_template_targets.py lines 15-20, use Iterator[Path] for
_python_sources() and None for test_all_literal_render_template_targets_exist();
in test/web_interface/test_cache.py lines 7-38, use Iterator[None] for
clean_cache() and None for each test function.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b7ea2f6-3b1b-4e96-b9a9-3017877e948e
📒 Files selected for processing (62)
.cursorrules.github/workflows/test.ymlCLAUDE.mdREADME.mdassets/README.mdconfig/config.template.jsonconfig/config_secrets.template.jsondocs/ADVANCED_FEATURES.mddocs/CONFIG_REFERENCE.mddocs/DEVELOPER_QUICK_REFERENCE.mddocs/HOW_TO_RUN_TESTS.mddocs/MIGRATION_GUIDE.mddocs/PLUGIN_ARCHITECTURE_SPEC.mddocs/PLUGIN_CONFIG_ARCHITECTURE.mddocs/PLUGIN_CUSTOM_ICONS.mddocs/PLUGIN_DEPENDENCY_TROUBLESHOOTING.mddocs/PLUGIN_DEVELOPMENT_GUIDE.mddocs/PLUGIN_REGISTRY_SETUP_GUIDE.mddocs/PLUGIN_STORE_GUIDE.mddocs/README.mddocs/REST_API_REFERENCE.mddocs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.mdfirst_time_install.shrequirements-test.txtrequirements.txtscripts/debug/check_imports.pyscripts/debug/debug_web_manual.pyscripts/debug/direct_fix_imports.pyscripts/download_nba_logos.pyscripts/fix_perms/README.mdscripts/fix_perms/fix_nhl_cache.shscripts/generate_report.pyscripts/install_dependencies_apt.pyscripts/utils/clear_cache.pysrc/cache_manager.pysrc/common/README.mdsrc/common/cli.pysrc/common/logo_helper.pysrc/config_manager.pysrc/logo_downloader.pysrc/plugin_system/testing/plugin_test_base.pysrc/plugin_system/testing/visual_display_manager.pysrc/web_interface/error_handler.pysrc/web_interface/logging_config.pytest/debug_nba_api.pytest/test_config_manager.pytest/test_doc_links.pytest/test_nba_core_functionality.pytest/test_nba_data_structure.pytest/test_nba_integration.pytest/test_nba_leaderboard_fix.pytest/test_template_targets.pytest/test_widget_scripts.pytest/web_interface/test_cache.pyweb_interface/README.mdweb_interface/blueprints/api_v3.pyweb_interface/blueprints/pages_v3.pyweb_interface/cache.pyweb_interface/requirements.txtweb_interface/static/v3/js/widgets/example-color-picker.jsweb_interface/static/v3/js/widgets/time-picker.jsweb_interface/templates/v3/base.html
💤 Files with no reviewable changes (17)
- scripts/fix_perms/README.md
- src/common/README.md
- src/config_manager.py
- first_time_install.sh
- test/test_nba_data_structure.py
- src/web_interface/logging_config.py
- test/test_nba_core_functionality.py
- test/test_nba_integration.py
- test/test_config_manager.py
- scripts/debug/direct_fix_imports.py
- scripts/debug/check_imports.py
- web_interface/blueprints/pages_v3.py
- test/test_nba_leaderboard_fix.py
- scripts/generate_report.py
- src/common/cli.py
- scripts/fix_perms/fix_nhl_cache.sh
- test/debug_nba_api.py
🛑 Comments failed to post (4)
docs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.md (4)
32-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep manifest examples valid JSON.
The blocks are marked as JSON but contain
//comments and...placeholders. The quick-reference block also contains standalone icon properties instead of a complete object. Remove the comments and placeholders, or label the examples as non-JSON fragments. Prefer copyable JSON because these examples describemanifest.json.Also applies to: 348-374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.md` around lines 32 - 40, Update the manifest examples in PLUGIN_CUSTOM_ICONS_FEATURE.md, including the quick-reference block, so every block labeled JSON is valid, copyable JSON: remove inline comments and ellipsis placeholders, and replace standalone icon-property fragments with complete manifest objects. Preserve the documented icon values and example intent.
81-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the
Clock Simpleicon examples.The manifest example at Lines 88-91 uses Font Awesome
fas fa-clock. The testing instructions at Line 318 expect the emoji🕐. Update one example so the documented manifest value and expected output match.Also applies to: 311-322
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.md` around lines 81 - 91, Align the Clock Simple icon documentation by updating either the manifest example near “clock-simple plugin” or the expected output in the testing instructions to use the same icon value, preserving the intended documented behavior consistently across both sections.
194-200: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language labels to the visual-example fences.
The two visual examples use unlabeled fenced blocks. Add
textto both fences to satisfy Markdown lint rule MD040.🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 194-194: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 199-199: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.md` around lines 194 - 200, Update both visual-example fenced blocks surrounding the “Before” and “After” custom-icon examples to use the text language label, preserving their existing contents.Source: Linters/SAST tools
304-309: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a hyphenated compound adjective.
Change
Fully backward compatibleat Line 306 toFully backward-compatible.🧰 Tools
🪛 LanguageTool
[uncategorized] ~306-~306: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...y ## Backward Compatibility ✅ Fully backward compatible - Plugins withouticonfield stil...(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/archive/PLUGIN_CUSTOM_ICONS_FEATURE.md` around lines 304 - 309, Update the “Backward Compatibility” section’s wording from “Fully backward compatible” to “Fully backward-compatible,” preserving the surrounding content.Source: Linters/SAST tools
Verified each finding against the code; fixes for the valid ones: - install_dependencies_apt.py: the installer listed 'freetype', but the declared dependency is freetype-py — an apt miss would pip-install the wrong PyPI package. Now installs freetype-py with an import-name mapping (pre-existing bug, surfaced by the review). - api_v3.py: pixel_mapper_config is validated as a string before being saved to display.hardware (JSON callers could previously store an object/list the matrix library can't use). - .cursorrules: the Plugin Loading Process and File Organization sections still said discovery scans plugins/ — now consistent with the corrected overview (configured directory, default plugin-repos/). - README.md: removed the stale '(except the core calendar)' claim — no core calendar exists in src/ — and qualified the plugin inventory (official plugins in the monorepo; third-party from their own repos). - CONFIG_REFERENCE.md: hardware_mapping now shows the code fallback (adafruit-hat-pwm) alongside the template value. - PLUGIN_REGISTRY_SETUP_GUIDE.md: check_plugin.py takes --plugin, not a positional id. - scripts/fix_perms/fix_*.sh: exec bits set so the documented 'sudo ./...' invocations work. - Guard tests hardened: template guard now catches multi-line render_template() calls; widget guard parses actual <script> src values and fails if the widgets dir goes missing; type hints and docstrings added per repo coding guidelines. Skipped with reasons (noted on the PR): limit_refresh_rate_hz 100-vs-90 is documented as intentional in CONFIG_REFERENCE.md; the psutil comment already names the enforcing manifest; docs/archive/ findings are out of scope per the docs policy (archive may rot). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
Owner
Author
|
CodeRabbit's 15 findings triaged and resolved in Fixed: the freetype/freetype-py installer mismatch (real pre-existing bug — an apt miss would have pip-installed the wrong PyPI package); Fixed with a correction: the README suggestion asked to "state the core calendar exception" — but no core calendar exists in Skipped:
All 916 CI-enrolled tests pass after the changes, boot smoke is green, and the new validation was exercised live (non-string Generated by Claude Code |
…-cleanup-s28o2e # Conflicts: # .github/workflows/test.yml
…E.md The maintainer no longer uses Cursor. .cursorrules, .cursorignore and the .cursor/ tree (rules, plugin templates, a parallel 751-line plugins guide) are removed; measurement showed near-zero literal overlap risk — the canonical content already lives in docs/. Unique guidance worth keeping moved before deletion: - CLAUDE.md gains the dev workflow (dev_plugin_setup.sh, dev_server.py, run.py -e, check_plugin.py), the plugin-secrets namespacing contract, and the no-draw_image()/paste-onto-PIL pitfall. - PLUGIN_DEVELOPMENT_GUIDE.md absorbs the plugin version-management rules (pre-push hook install, SKIP_TAG, version resolution order) that its own text previously linked out to .cursorrules for. - The one completed plan doc (.cursor/plans/) is archived to docs/archive/ per the docs policy rather than deleted. One of the deleted rule files (sports-managers.mdc) targeted src/*_managers.py globs that have matched nothing since the plugin migration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
…phaned JS, misfiled test deps - first_time_install.sh: removed the pip fallback branch that installed from requirements_web_v2.txt — a file that has not existed since the v2 web interface was removed (the branch always printed its own 'not found; skipping' warning). - scripts/remove_plugin_backups.sh deleted: its PROJECT_ROOT resolved to the repo's PARENT directory, and its verify_submodules() checks for plugin submodules from an era before plugins moved to the store — it could never have worked from its current location. - plugins_manager.js: removed three functions with zero call sites anywhere (addKeyValuePair, formatCommit, togglePasswordVisibility) — verified against all templates, all JS, and the dynamic window[name] dispatch sites, which resolve widget-registry keys only. Also replaced base.html's misleading 'Legacy ... during migration' label: the file is deliberately loaded last and provides the LIVE implementations of seven window.* plugin actions that shadow same-named definitions in app.js/app-shell.js. - pytest/pytest-cov/pytest-mock moved from runtime requirements.txt to requirements-test.txt (CI already installs both files; the installer's line-by-line loop simply installs three fewer packages on devices; no store plugin declares pytest). HOW_TO_RUN_TESTS.md updated. - scripts/add_defaults_to_schemas.py and analyze_plugin_schemas.py scanned the empty legacy plugins/ dir — now scan plugin-repos/. Verified: fresh venv installs all four requirements files with pip check clean and pytest available; bash -n on the installer; node --check on the JS; widget/cache guard tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
…r guides A second-pass content audit checked the guides' substantive claims against the code (the first pass only fixed mechanical drift). Fixes: - GETTING_STARTED: described booting a prebuilt SD image and seeing default clock/weather plugins — neither exists. Now documents the real install (Pi OS Lite + one-shot installer / first_time_install.sh) and that displays come from the Plugin Store. Duration and ordering instructions moved to the Rotation tab where the controls actually live. - WEB_INTERFACE_GUIDE: three whole tabs were undocumented (Rotation, Backup & Restore, Tools) and the Display tab's Vegas Scroll section was unmentioned. Fonts overrides are per display element (not per plugin); Logs has an Auto-scroll checkbox (not a Pause button); the aspirational keyboard-shortcut list and no-JS claim removed. - TROUBLESHOOTING: the hand-written service-file template (wrong user, wrong ExecStart, dropped the autostart gate) replaced with the real systemd/ units + install scripts; recovery steps no longer copy placeholder units verbatim; WiFi curl endpoint corrected to /api/v3/; cache-clearing advice now targets the real cache locations. - ADVANCED_FEATURES: removed a false claim that CacheManager has no delete(); fixed two example snippets that raise TypeError (BackgroundDataService and get_config_file_mode signatures); fixed cache paths, a 5-minute TTL that is actually 1 hour, and the vegas table now links the complete 26-key reference. - EMULATOR_SETUP_GUIDE: documented run.py flags that don't exist (--plugin/--test-plugins) removed in favor of dev_server.py and check_plugin.py; shipped emulator config values corrected (browser adapter default on :8888, not pygame). - PLUGIN_QUICK_REFERENCE: drag-and-drop reordering is shipped, not 'not yet supported'; discovery-fallback and registry-repo claims corrected. PLUGIN_API_REFERENCE: get_vegas_segment_width returns panels, not pixels. CONTRIBUTING: the repo uses flake8/mypy/bandit pre-commit hooks, not black/ruff, and tests need requirements-test.txt. - SKIN_SYSTEM/DEVELOPER_QUICK_REFERENCE: stale module paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
…eliberate re-export All three of these were introduced by this PR, which is what makes them worth fixing here rather than deferring. `urllib3` and `jinja2` were added to the requirements so that direct imports stop relying on transitives — right call, but both floors were set to the version that introduced the API rather than a version that is safe to install. `urllib3>=1.26.0` sits below roughly ten CVEs including a decompression-bomb safeguard bypass, and `jinja2>=3.1.0` below five including two sandbox breakouts. Raised to 2.7.0 and 3.1.6, which is what a working device already runs, so no install is disturbed. The comments now say the floor is a security floor, since the next person to read "imported directly" would otherwise reasonably lower it again. This is the same reasoning the PR already applied to werkzeug; these two just missed it. The `DateTimeEncoder` import in cache_manager is unused on purpose — the canonical class moved to src.cache.disk_cache and this re-export keeps the documented import path working. flake8 cannot see intent, so it gets an explicit `# noqa: F401` rather than being removed and quietly breaking anything importing it from here. Verified the re-export still resolves to the same object and still serialises datetimes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 43: Add installation of the pre-commit CLI to the contribution setup
workflow before the existing pre-commit install command, either via python -m
pip install pre-commit or by including pre-commit in the documented development
dependencies.
In `@docs/ADVANCED_FEATURES.md`:
- Around line 664-670: Specify a language on the fenced example block under
display_on_demand_processed_id by changing the opening fence to text or json,
while preserving the example content and surrounding documentation.
In `@docs/EMULATOR_SETUP_GUIDE.md`:
- Around line 100-106: Remove the obsolete suppress_adapter_load_errors entry
from the Debug Mode JSON example so it matches the documented options table. Use
the existing option-table entries as the configuration contract and do not
restore the key unless runtime support is confirmed.
In `@docs/GETTING_STARTED.md`:
- Around line 302-309: Fix the blockquote formatting in the documentation around
the cached-data note: remove the blockquote marker from the blank line or make
the paragraphs one continuous blockquote so markdownlint-cli2 no longer reports
MD028.
In `@docs/PLUGIN_QUICK_REFERENCE.md`:
- Around line 125-130: Update the plugin configuration example near the
referenced REST API guidance to remove the obsolete per-plugin display_duration
field and use the display.display_durations setting instead, matching the
current rotation configuration guidance.
In `@first_time_install.sh`:
- Line 1192: The web dependency flow must not print success or create
.web_deps_installed unless installation actually succeeded. Update the logic
around the Step 5 success message and marker creation to track the install
result, and fail or skip marker creation when the requirements file is missing,
pip fails, or the smart installer is unavailable.
In `@scripts/install_dependencies_apt.py`:
- Line 47: Update check_package_installed() and the freetype-py handling at the
referenced package-installation paths to inspect the installed freetype-py
version, not just whether freetype imports successfully. Parse the installed
version and treat the APT package as satisfied only when it is at least 2.5.1;
otherwise continue through the pip installation path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 173b7a67-3baa-41b7-aa1e-d09882b4dfd4
📒 Files selected for processing (58)
.cursor/README.md.cursor/plugin_templates/QUICK_START.md.cursor/plugin_templates/README.md.template.cursor/plugin_templates/config_schema.json.template.cursor/plugin_templates/manager.py.template.cursor/plugin_templates/manifest.json.template.cursor/plugin_templates/requirements.txt.template.cursor/plugin_templates/test_manager.py.template.cursor/plugins_guide.md.cursor/rules/coding-standards.mdc.cursor/rules/configuration-management.mdc.cursor/rules/error-handling-logging.mdc.cursor/rules/git-workflow.mdc.cursor/rules/github-branches-rule.mdc.cursor/rules/project-structure.mdc.cursor/rules/raspberry-pi-development.mdc.cursor/rules/sports-managers.mdc.cursor/rules/testing-standards.mdc.cursorignore.cursorrules.github/workflows/test.ymlCLAUDE.mdCONTRIBUTING.mdREADME.mddocs/ADVANCED_FEATURES.mddocs/CONFIG_REFERENCE.mddocs/DEVELOPER_QUICK_REFERENCE.mddocs/EMULATOR_SETUP_GUIDE.mddocs/GETTING_STARTED.mddocs/HOW_TO_RUN_TESTS.mddocs/PLUGIN_API_REFERENCE.mddocs/PLUGIN_DEVELOPMENT_GUIDE.mddocs/PLUGIN_QUICK_REFERENCE.mddocs/PLUGIN_REGISTRY_SETUP_GUIDE.mddocs/SKIN_SYSTEM.mddocs/TROUBLESHOOTING.mddocs/WEB_INTERFACE_GUIDE.mddocs/archive/CURSOR_PLUGIN_SCHEMA_AUDIT_PLAN.mdfirst_time_install.shrequirements-test.txtrequirements.txtscripts/add_defaults_to_schemas.pyscripts/analyze_plugin_schemas.pyscripts/fix_perms/fix_assets_permissions.shscripts/fix_perms/fix_cache_permissions.shscripts/fix_perms/fix_plugin_permissions.shscripts/fix_perms/fix_web_permissions.shscripts/install_dependencies_apt.pyscripts/remove_plugin_backups.shsrc/cache_manager.pytest/test_doc_links.pytest/test_template_targets.pytest/test_widget_scripts.pytest/web_interface/test_cache.pyweb_interface/blueprints/api_v3.pyweb_interface/requirements.txtweb_interface/static/v3/plugins_manager.jsweb_interface/templates/v3/base.html
💤 Files with no reviewable changes (22)
- .cursor/rules/raspberry-pi-development.mdc
- .cursor/plugin_templates/requirements.txt.template
- .cursor/rules/error-handling-logging.mdc
- .cursorignore
- .cursor/plugin_templates/README.md.template
- .cursor/rules/configuration-management.mdc
- .cursor/rules/git-workflow.mdc
- .cursor/rules/github-branches-rule.mdc
- .cursor/plugin_templates/config_schema.json.template
- .cursor/rules/testing-standards.mdc
- .cursor/plugin_templates/manifest.json.template
- .cursorrules
- .cursor/plugins_guide.md
- .cursor/rules/coding-standards.mdc
- .cursor/plugin_templates/test_manager.py.template
- scripts/remove_plugin_backups.sh
- .cursor/plugin_templates/manager.py.template
- .cursor/README.md
- web_interface/static/v3/plugins_manager.js
- .cursor/plugin_templates/QUICK_START.md
- .cursor/rules/sports-managers.mdc
- .cursor/rules/project-structure.mdc
🚧 Files skipped from review as they are similar to previous changes (12)
- test/test_template_targets.py
- docs/PLUGIN_REGISTRY_SETUP_GUIDE.md
- docs/HOW_TO_RUN_TESTS.md
- docs/CONFIG_REFERENCE.md
- README.md
- test/web_interface/test_cache.py
- docs/DEVELOPER_QUICK_REFERENCE.md
- src/cache_manager.py
- test/test_doc_links.py
- web_interface/templates/v3/base.html
- web_interface/requirements.txt
- requirements.txt
- install_dependencies_apt.py: an import-only check let Debian Bookworm's python3-freetype 2.3.0 satisfy the freetype-py>=2.5.1 pin. check_package_installed() now verifies the installed freetype-py version, and an apt install that lands below the minimum falls through to pip instead of counting as success. - first_time_install.sh: the .web_deps_installed marker was created even when the smart installer failed, so re-runs skipped installation with dependencies missing. The marker is now created only on success. - CONTRIBUTING.md: document installing the pre-commit CLI before 'pre-commit install' (the requirements files don't provide it). - Doc lint: fence language on the on-demand cache example (MD040), blockquote continuation in GETTING_STARTED (MD028), and the suppress_adapter_load_errors key removed from the emulator debug example to match the options table. Skipped one finding with reason (noted on the PR): the per-plugin display_duration field in PLUGIN_QUICK_REFERENCE's example is not obsolete — BasePlugin.get_display_duration() reads it and PLUGIN_CONFIG_CORE_PROPERTIES.md documents it as a core property; display.display_durations is a per-mode override, not a replacement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
Owner
Author
|
CodeRabbit's second-round findings resolved in Fixed:
Skipped: the Guard tests and doc-link checks pass on the new head. Generated by Claude Code |
23 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full codebase audit and cleanup: fixes seven shipping bugs (silent font-cache invalidation failure, partial routes that 500, plugin config widgets that never load, contradictory dependency pins, config keys missing from the template, broken utility scripts), removes verified-dead code, corrects extensive documentation drift (wrong file:line citations, 14 broken links, phantom scripts, a pre-plugin-era README), and adds three CI regression guards so these bug classes can't silently return. Every deletion was cross-checked against a fresh clone of the
ledmatrix-pluginsmonorepo (43 plugins); anything documented as plugin-facing API or referenced by any plugin was kept.Type of change
Related issues
None — originated from a full-repo audit request.
Test plan
EMULATOR=true python3 run.py) — via Flask boot smoke: app builds, every parameterless GET route returns < 500, removed partials 404 cleanlyscripts/dev_server.py)pytest) — full suite before and after: the failure set only shrank (2 pre-existingtest_state_reconciliation.pyfailures exist on the base commit and are untouched; zero new failures). All 907 CI-enrolled tests green, coverage gate (≥30%) still met at 53.27%Additional verification:
pip checkclean, CI-enrolled suite + boot smoke pass with the trimmed dependency setflake8(pre-commit's exact args): identical finding set before and after (70 pre-existing, zero new)python -m compileall src web_interface scriptsclean.get()fallbacks, so_migrate_config()merging them into existing user configs cannot change behaviorDocumentation
README.mdif user-facing behavior changeddocs/if developer behavior changedNew:
docs/CONFIG_REFERENCE.md(every config key with type/default/reader) andassets/README.md(records which asset dirs are runtime dependencies of store plugins).Plugin compatibility
Every deletion candidate was grepped against a fresh clone of
ChuckBuilds/ledmatrix-plugins(43 plugins). Kept despite zero in-repo callers because plugins or docs depend on them:src/base_classes/{baseball,basketball,football,hockey}.py,src/common/helper modules (incl.config_helper/display_helper),DisplayError,LogoDownloader.normalize_abbreviation(9 plugin callers),assets/{stocks,weather,news_logos,broadcast_logos}(runtime-resolved by store plugins), and the websocket packages now live only in plugin-declared requirements (ledmatrix-music already declares its own). One fix actively helps plugins: countdown and of-the-day declaretime-picker/file-upload-single/plugin-file-managerwidgets whose JS was never loaded — their config fields rendered permanently blank; those scripts are now included.Checklist
CONTRIBUTING.mdCONTRIBUTING.mdandCODE_OF_CONDUCT.mdconfig_schema.json) — added keys (panel_type,row_address_type,multiplexing,pixel_mapper_config,development_mode) are already handled by the existing display/general forms inapi_v3.py's allowlistsNotes for reviewer
Commit-by-commit guide (each commit stands alone with its evidence in the message):
delete_cachedimplemented; dead weather/stocks partial routes removed; psutil/Flask-Limiter/freetype-py pin contradictions resolved; missing template keys added (defaults byte-identical to code fallbacks); broken script sys.paths repairedplugins/first), 14 broken links, phantom scripts, stale CI description, docs index completed (the skin system was invisible from it)cli.py(imports a package that exists nowhere), deadsrc/web_interface/logging_config.py, unusedhandle_errors,get_clock_config, manager-era NBA tests (they swallowed ImportErrors of deleted modules and passed while testing nothing), artifact-boundgenerate_report.py, legacy youtube secrets block; plus the widget-loading fixapp.pydisables CSRF); declared directly-imported transitives (urllib3, jinja2, markupsafe);install_dependencies_apt.pymirrors the cleanupDateTimeEncoder(import-compatible); behaviorally-different near-duplicates got divergence comments instead of merging (the twonormalize_abbreviations differ — merging would change logo filename resolution on installed devices)CONFIG_REFERENCE.md, README brought into the plugin era, status-report doc archived,assets/README.mdDeliberately NOT done (audited, judged too risky or wrong for this PR):
assets/dirs (132MB "orphans" are runtime dependencies of the stocks/weather/news/odds plugins — now documented inassets/README.md)VisualDisplayManagerintoDisplayManager(deliberate headless fork; fork-warning header added listing the ~15 mirrored methods)display_controller.pyshim — installed devices have sudoers NOPASSWD entries pointing at its absolute path (configure_web_sudo.sh)logging_config.pyfiles (different processes), the twoPluginTestBaseclasses (both are API; cross-referenced), or the overlapping plugin-config docs (banner added instead).codacy.ymlandprove_security.pykept — Codacy runs server-side off the README badge; prove_security runs standalone (verified)Found but not fixed (flagged for a future decision):
LogoHelper.normalize_abbreviationandLogoDownloader.normalize_abbreviationnormalize differently (space-stripping vs filesystem-char replacement). Logo filenames on existing installs depend on both, so unifying them needs a migration, not a code cleanup. Also the 2 pre-existingtest_state_reconciliation.pyfailures (fail on the base commit; look environment-dependent — registry fetch).Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation