fix: post-audit follow-up code fixes (cache, fonts, icons, dev script)#307
Merged
ChuckBuilds merged 2 commits intomainfrom Apr 8, 2026
Merged
fix: post-audit follow-up code fixes (cache, fonts, icons, dev script)#307ChuckBuilds merged 2 commits intomainfrom
ChuckBuilds merged 2 commits intomainfrom
Conversation
…, CI) The docs refresh effort (#306, ledmatrix-plugins#92) surfaced seven code bugs that were intentionally left out of the docs PRs because they required code changes rather than doc fixes. This PR addresses the six that belong in LEDMatrix (the seventh — a lacrosse-scoreboard mode rename — lives in the plugins repo). Bug 1: cache_manager.delete() AttributeError src/common/api_helper.py:287 and src/plugin_system/resource_monitor.py:343 both call cache_manager.delete(key), which doesn't exist — only clear_cache(key=None). Added a delete() alias method on CacheManager that forwards to clear_cache(key). Reverts the "There is no delete() method" wording in DEVELOPER_QUICK_REFERENCE, .cursorrules so the docs match the new shim. Bug 2: dev_plugin_setup.sh PROJECT_ROOT resolution scripts/dev/dev_plugin_setup.sh:9 set PROJECT_ROOT to SCRIPT_DIR instead of walking up two levels to the repo root, so PLUGINS_DIR resolved to scripts/dev/plugins/ and created symlinks under the script's own directory. Fixed the path and removed the stray scripts/dev/plugins/of-the-day symlink left by earlier runs. Bug 3: plugin custom icons regressed from v2 to v3 web_interface/blueprints/api_v3.py built the /plugins/installed response without including the manifest's "icon" field, and web_interface/templates/v3/base.html hardcoded fas fa-puzzle-piece in all three plugin-tab render sites. Pass the icon through the API and read it from the templates with a puzzle-piece fallback. Reverts the "currently broken" banners in docs/PLUGIN_CUSTOM_ICONS.md and docs/PLUGIN_CUSTOM_ICONS_FEATURE.md. Bug 4: register_plugin_fonts was never wired up src/font_manager.py:150 defines register_plugin_fonts(plugin_id, font_manifest) but nothing called it, so plugin manifests with a "fonts" block were silently no-ops. Wired the call into PluginManager.load_plugin() right after plugin_loader.load_plugin returns. Reverts the "not currently wired" warning in docs/FONT_MANAGER.md's "For Plugin Developers" section. Bug 5: dead web_interface_v2 import pattern (LEDMatrix half) src/base_odds_manager.py had a try/except importing web_interface_v2.increment_api_counter, falling back to a no-op stub. The module doesn't exist anywhere in the v3 codebase and no API metrics dashboard reads it. Deleted the import block and the single call site; the plugins-repo half of this cleanup lands in ledmatrix-plugins#<next>. Bug 7: no CI test workflow .github/workflows/ only contained security-audit.yml; pytest ran locally but was not gated on PRs. Added .github/workflows/tests.yml running pytest against Python 3.10, 3.11, 3.12 in EMULATOR=true mode, skipping tests marked hardware or slow. Updated docs/HOW_TO_RUN_TESTS.md to reflect that the workflow now exists. Verification done locally: - CacheManager.delete(key) round-trips with set/get - base_odds_manager imports without the v2 module present - dev_plugin_setup.sh PROJECT_ROOT resolves to repo root - api_v3 and plugin_manager compile clean - tests.yml YAML parses - Script syntax check on dev_plugin_setup.sh Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Plugin as Plugin module
participant PM as PluginManager
participant FM as FontManager
Plugin->>PM: import plugin module / read manifest
PM->>PM: extract manifest['fonts'] if present
alt font_manager available & has register_plugin_fonts
PM->>FM: register_plugin_fonts(plugin_id, font_manifest)
FM-->>PM: success / raises Exception
alt Exception
PM->>PM: logger.warning("font registration failed")
end
end
PM->>PM: continue plugin validation & lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
docs/HOW_TO_RUN_TESTS.md (1)
339-343: Consider adding a direct PR/issue link for the planned pytest workflow.
“See the PR adding it” is hard to follow later without a concrete reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/HOW_TO_RUN_TESTS.md` around lines 339 - 343, The docs text currently says "see the PR adding it" without a concrete reference; replace that vague reference with a direct link and identifier to the PR or issue that adds the `.github/workflows/tests.yml` pytest workflow (for example: PR number and URL or issue number and URL). Update the sentence around the `.github/workflows/tests.yml` mention to embed the direct PR/issue link so future readers can find the exact change (refer to the `.github/workflows/tests.yml` filename and the PR/issue ID in the updated sentence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/FONT_MANAGER.md`:
- Around line 149-152: Update the paragraph that currently reads "the web UI are
still **placeholder implementations** — manually registered manager fonts do not
yet flow into that tab. If you need an override today, load the font directly in
your plugin and skip the override system." to explicitly separate what is
placeholder vs what works: state that the web UI tab for manager-registered
fonts is a placeholder (manually registered fonts do not appear there yet) while
the "Manual Font Overrides" workflow later in the document (refer to the "Manual
Font Overrides" section) remains functional for per-plugin overrides — keep the
advice to load fonts directly in plugins as a workaround and add a brief
parenthetical noting that the UI override controls are not yet wired to
manager-registered fonts.
In `@src/cache_manager.py`:
- Around line 336-339: The delete method currently calls clear_cache(key) which
treats falsy values the same as no key and can wipe the entire cache when passed
an empty string; update delete(key) to validate the input (raise ValueError if
key is None or an empty string) and call clear_cache only for valid keys, and
change clear_cache to branch explicitly on key is None (not on truthiness) so
None means "clear all" while empty string is rejected; update the docstring for
delete and clear_cache accordingly and reference the delete and clear_cache
methods when making the changes.
In `@src/plugin_system/plugin_manager.py`:
- Around line 367-376: The code is incorrectly trying to get the font manager
from self.display_manager (using getattr) which is None; change the logic to use
the injected self.font_manager directly: when font_manifest is present, check
self.font_manager (not getattr(self.display_manager, 'font_manager', None)), and
if it implements register_plugin_fonts call
self.font_manager.register_plugin_fonts(plugin_id, font_manifest) inside the
try/except and keep the same warning via self.logger if registration raises an
exception.
In `@web_interface/templates/v3/base.html`:
- Around line 1964-1967: The current code uses this.escapeHtml(plugin.icon) and
then injects it inside tabButton.innerHTML into an attribute, which is unsafe
for attribute-context (e.g., a quote in plugin.icon can break out). Replace the
innerHTML composition in the full tab renderer: create the <i> element with
document.createElement('i'), set its.className from a safe value (sanitize
plugin.icon or whitelist classes) using element.className = iconClass, set the
text node for the label via textContent using this.escapeHtml(plugin.name ||
plugin.id) (or directly textContent = plugin.name || plugin.id), and append both
nodes to tabButton instead of using innerHTML; ensure you reference iconClass,
plugin.icon, plugin.name, plugin.id, escapeHtml, and tabButton in the change.
---
Nitpick comments:
In `@docs/HOW_TO_RUN_TESTS.md`:
- Around line 339-343: The docs text currently says "see the PR adding it"
without a concrete reference; replace that vague reference with a direct link
and identifier to the PR or issue that adds the `.github/workflows/tests.yml`
pytest workflow (for example: PR number and URL or issue number and URL). Update
the sentence around the `.github/workflows/tests.yml` mention to embed the
direct PR/issue link so future readers can find the exact change (refer to the
`.github/workflows/tests.yml` filename and the PR/issue ID in the updated
sentence).
🪄 Autofix (Beta)
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
Run ID: 09ee95ee-eb26-4754-a0eb-b15821b478b0
📒 Files selected for processing (13)
.cursorrulesdocs/DEVELOPER_QUICK_REFERENCE.mddocs/FONT_MANAGER.mddocs/HOW_TO_RUN_TESTS.mddocs/PLUGIN_CUSTOM_ICONS.mddocs/PLUGIN_CUSTOM_ICONS_FEATURE.mdscripts/dev/dev_plugin_setup.shscripts/dev/plugins/of-the-daysrc/base_odds_manager.pysrc/cache_manager.pysrc/plugin_system/plugin_manager.pyweb_interface/blueprints/api_v3.pyweb_interface/templates/v3/base.html
💤 Files with no reviewable changes (2)
- scripts/dev/plugins/of-the-day
- docs/PLUGIN_CUSTOM_ICONS.md
ChuckBuilds
pushed a commit
to ChuckBuilds/ledmatrix-plugins
that referenced
this pull request
Apr 8, 2026
The docs refresh audit on #92 flagged two code bugs affecting the plugins repo. Neither could be fixed in a docs-only PR, so they were deferred to this follow-up. The companion LEDMatrix PR (ChuckBuilds/LEDMatrix#307) handles the six bugs that belong in the app repo. Bug 6: lacrosse-scoreboard display-mode collision (BREAKING) lacrosse-scoreboard shipped six display modes (ncaa_mens_recent / upcoming / live and ncaa_womens_recent / upcoming / live) that collide with the same six modes exposed by hockey-scoreboard. LEDMatrix's display controller stores modes in a flat dict keyed by mode name (src/display_controller.py), so whichever plugin loaded second silently overrode the first — with no warning in the logs. Renamed all six modes with a plugin-specific lax_ prefix: ncaa_mens_recent -> lax_ncaa_mens_recent ncaa_mens_upcoming -> lax_ncaa_mens_upcoming ncaa_mens_live -> lax_ncaa_mens_live ncaa_womens_recent -> lax_ncaa_womens_recent ncaa_womens_upcoming -> lax_ncaa_womens_upcoming ncaa_womens_live -> lax_ncaa_womens_live Manifest, dispatch logic in manager.py (_get_current_manager, _record_dynamic_progress, get_cycle_duration, and the granular mode-parser that walks the league registry) all updated to use the new names. The split('_', 2)[2] parser has been replaced with rsplit('_', 1)[1] where the new four-segment mode strings (lax_ncaa_mens_recent) need the last segment. README "Known conflict" warning replaced with a breaking-change note pointing at the new CHANGELOG.md, which spells out the rename table and migration steps for anyone with the old mode names hardcoded in their config.json (display_durations, rotation_order, etc.). Version bumped 1.0.3 -> 1.1.0 (minor bump per semver — breaking config change). No backward-compat aliases. Bug 5: dead web_interface_v2 import pattern (plugins half) Every plugin that used to talk to the v2 API-counter helper carried a try/except block importing web_interface_v2 and falling back to a no-op stub. The web_interface_v2 module doesn't exist anywhere in the v3 codebase — the counter was silently a no-op for every plugin, and no API-metrics dashboard reads the counter. Deleted the import block, all direct calls, one self.increment_api_counter hasattr-guarded call site in odds-ticker/data_fetcher.py, and the orphan "# Increment API counter" comment lines left behind. Touched plugins (14 files): - baseball-scoreboard/base_odds_manager.py - basketball-scoreboard/base_odds_manager.py - football-scoreboard/base_odds_manager.py - hockey-scoreboard/base_odds_manager.py - lacrosse-scoreboard/base_odds_manager.py - soccer-scoreboard/base_odds_manager.py - ufc-scoreboard/base_odds_manager.py - ledmatrix-leaderboard/data_fetcher.py - ledmatrix-music/manager.py - ledmatrix-stocks/data_fetcher.py - ledmatrix-weather/manager.py - odds-ticker/manager.py - odds-ticker/data_fetcher.py - youtube-stats/manager.py Verification done locally: - ast.parse on all plugin .py files (0 syntax errors) - py_compile on every touched file - No web_interface_v2 or increment_api_counter references remain anywhere under plugins/ (except the unrelated src.api_counter mock in lacrosse-scoreboard/test_lacrosse_plugin.py) - Lacrosse lax_ prefix applied consistently across manifest, default mode fallback, dispatch logic, and mode-string parser Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merged
4 tasks
ChuckBuilds
added a commit
to ChuckBuilds/ledmatrix-plugins
that referenced
this pull request
Apr 8, 2026
…ts) (#93) The docs refresh audit on #92 flagged two code bugs affecting the plugins repo. Neither could be fixed in a docs-only PR, so they were deferred to this follow-up. The companion LEDMatrix PR (ChuckBuilds/LEDMatrix#307) handles the six bugs that belong in the app repo. Bug 6: lacrosse-scoreboard display-mode collision (BREAKING) lacrosse-scoreboard shipped six display modes (ncaa_mens_recent / upcoming / live and ncaa_womens_recent / upcoming / live) that collide with the same six modes exposed by hockey-scoreboard. LEDMatrix's display controller stores modes in a flat dict keyed by mode name (src/display_controller.py), so whichever plugin loaded second silently overrode the first — with no warning in the logs. Renamed all six modes with a plugin-specific lax_ prefix: ncaa_mens_recent -> lax_ncaa_mens_recent ncaa_mens_upcoming -> lax_ncaa_mens_upcoming ncaa_mens_live -> lax_ncaa_mens_live ncaa_womens_recent -> lax_ncaa_womens_recent ncaa_womens_upcoming -> lax_ncaa_womens_upcoming ncaa_womens_live -> lax_ncaa_womens_live Manifest, dispatch logic in manager.py (_get_current_manager, _record_dynamic_progress, get_cycle_duration, and the granular mode-parser that walks the league registry) all updated to use the new names. The split('_', 2)[2] parser has been replaced with rsplit('_', 1)[1] where the new four-segment mode strings (lax_ncaa_mens_recent) need the last segment. README "Known conflict" warning replaced with a breaking-change note pointing at the new CHANGELOG.md, which spells out the rename table and migration steps for anyone with the old mode names hardcoded in their config.json (display_durations, rotation_order, etc.). Version bumped 1.0.3 -> 1.1.0 (minor bump per semver — breaking config change). No backward-compat aliases. Bug 5: dead web_interface_v2 import pattern (plugins half) Every plugin that used to talk to the v2 API-counter helper carried a try/except block importing web_interface_v2 and falling back to a no-op stub. The web_interface_v2 module doesn't exist anywhere in the v3 codebase — the counter was silently a no-op for every plugin, and no API-metrics dashboard reads the counter. Deleted the import block, all direct calls, one self.increment_api_counter hasattr-guarded call site in odds-ticker/data_fetcher.py, and the orphan "# Increment API counter" comment lines left behind. Touched plugins (14 files): - baseball-scoreboard/base_odds_manager.py - basketball-scoreboard/base_odds_manager.py - football-scoreboard/base_odds_manager.py - hockey-scoreboard/base_odds_manager.py - lacrosse-scoreboard/base_odds_manager.py - soccer-scoreboard/base_odds_manager.py - ufc-scoreboard/base_odds_manager.py - ledmatrix-leaderboard/data_fetcher.py - ledmatrix-music/manager.py - ledmatrix-stocks/data_fetcher.py - ledmatrix-weather/manager.py - odds-ticker/manager.py - odds-ticker/data_fetcher.py - youtube-stats/manager.py Verification done locally: - ast.parse on all plugin .py files (0 syntax errors) - py_compile on every touched file - No web_interface_v2 or increment_api_counter references remain anywhere under plugins/ (except the unrelated src.api_counter mock in lacrosse-scoreboard/test_lacrosse_plugin.py) - Lacrosse lax_ prefix applied consistently across manifest, default mode fallback, dispatch logic, and mode-string parser Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- src/cache_manager.py: clear_cache(key) treated empty string as
"wipe all" because of `if key:`. Switched to `key is None`
branching, made delete(key) and clear_cache(key) reject empty
strings and None outright with ValueError, and updated both
docstrings to make the contract explicit. Verified locally
with a round-trip test that clear_cache() (no arg) still
wipes everything but clear_cache("") and delete("") raise.
- src/plugin_system/plugin_manager.py: was reaching for the
font manager via getattr(self.display_manager, 'font_manager',
None). PluginManager already takes a dedicated font_manager
parameter (line 54) and stores it as self.font_manager
(line 69), so the old path was both wrong and could miss the
font manager entirely when the host injects them separately.
Switched to self.font_manager directly with the same try/except
warning behavior.
- web_interface/templates/v3/base.html: in the full plugin-tab
renderer, the icon was injected with
`<i class="${escapeHtml(plugin.icon)}">` — but escapeHtml only
escapes <, >, and &, not double quotes, so a manifest with a
quote in its icon string could break out of the class
attribute. Replaced the innerHTML template with createElement
for the <i> tag, set className from plugin.icon directly
(no string interpolation), and used a text node for the
label. Same fix shape would also harden the two stub-renderer
sites at line 515 / 774, but those already escape `"` to
" and CodeRabbit only flagged this site, so leaving them
for now.
- docs/FONT_MANAGER.md: clarified that the Manual Font Overrides
*workflow* (set_override / remove_override / font_overrides.json)
is the supported override path today, and only the Fonts tab
in the web UI is the placeholder. Previous wording conflated
the two and made it sound like overrides themselves were
broken.
- docs/HOW_TO_RUN_TESTS.md: replaced the vague "see the PR
adding it" with a concrete link to #307 and a note that the
workflow file itself is held back pending the workflow scope.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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
Fixes six code bugs surfaced by the systematic doc-vs-code crosschecks
done during the docs refresh effort in #306. Those findings were
intentionally deferred out of #306 (a docs-only PR) and tracked for
this follow-up.
The seventh bug from that audit (lacrosse-scoreboard display-mode
collision with hockey-scoreboard) is a plugin-repo change and lives
in a companion PR against
ledmatrix-plugins.Bugs fixed
cache_manager.delete()AttributeError — two callers(
src/common/api_helper.py:287,src/plugin_system/resource_monitor.py:343) already calledcache_manager.delete(key), butCacheManageronly definedclear_cache(key=None). Added adelete()alias that forwardsto
clear_cache(key). Reverts the "nodelete()method"wording in
docs/DEVELOPER_QUICK_REFERENCE.mdand.cursorrules.scripts/dev/dev_plugin_setup.shPROJECT_ROOT— line 9set
PROJECT_ROOT="$SCRIPT_DIR", soPLUGINS_DIRresolved toscripts/dev/plugins/instead of the repo'splugins/. Walkedup two levels and removed the stray
scripts/dev/plugins/of-the-daysymlink left over from earlierruns.
Plugin custom icons regressed from v2 to v3 —
web_interface/blueprints/api_v3.pybuilt the/plugins/installedresponse without the manifest'siconfield, and
web_interface/templates/v3/base.htmlhardcodedfas fa-puzzle-piecein all three plugin-tab render sites.Pass
iconthrough the API and read it from the templateswith a puzzle-piece fallback. Reverts the "currently broken"
banners in
docs/PLUGIN_CUSTOM_ICONS.mdanddocs/PLUGIN_CUSTOM_ICONS_FEATURE.md.register_plugin_fonts()never wired up —src/font_manager.py:150definedregister_plugin_fonts(plugin_id, font_manifest)but no callerexisted, so a
"fonts"block inmanifest.jsonwas a silentno-op. Wired the call into
PluginManager.load_plugin()rightafter
plugin_loader.load_plugin()returns. Reverts the "notcurrently wired" warning in
docs/FONT_MANAGER.md.Dead
web_interface_v2import pattern (LEDMatrix half) —src/base_odds_manager.pyhad atry/exceptimportingweb_interface_v2.increment_api_counter, falling back to ano-op stub. The
web_interface_v2module doesn't existanywhere in the v3 codebase and no API-metrics dashboard
reads the counter. Deleted the import block and the single
call site. The plugins-repo half of this cleanup is in the
companion
ledmatrix-pluginsPR.Not included in this PR
CI pytest workflow (bug 7 from the plan) — the new
.github/workflows/tests.ymlis ready locally but my OAuthtoken lacks the
workflowscope, so the push was rejected.The file is waiting on disk (
/var/home/chuck/Github/LEDMatrix/.github/workflows/tests.yml)— I can hand you the contents to drop in, or you can push it
yourself from a shell with the
workflowscope. I left the"Continuous Integration" section in
docs/HOW_TO_RUN_TESTS.mdwording as "planned" until the workflow actually lands.
Lacrosse-scoreboard display-mode rename (bug 6) — ships in
the companion
ledmatrix-pluginsPR.Verification done locally
Test plan
web_interface_v2` warnings in `journalctl -u ledmatrix`.
manifest (e.g. hockey-scoreboard with `fas fa-hockey-puck`),
restart the web service, confirm the plugin tab in the second
nav row shows the custom icon.
restart the display service, confirm the font manager logs
`"Successfully registered font ... for plugin ..."`.
confirm it operates against the repo's `plugins/` directory,
not `scripts/dev/plugins/`.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
delete()alias for targeted cache removal.Improvements
Documentation