mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-04-10 13:02:59 +00:00
fix: post-audit follow-up code fixes (cache, fonts, icons, dev script) (#307)
* fix: post-audit follow-up code fixes (cache, fonts, icons, dev script, 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> * fix: address CodeRabbit review comments on #307 - 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> --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -62,7 +62,7 @@ display_manager.defer_update(lambda: self.update_cache(), priority=0)
|
||||
# Basic caching
|
||||
cached = cache_manager.get("key", max_age=3600)
|
||||
cache_manager.set("key", data)
|
||||
cache_manager.clear_cache("key") # there is no delete() method
|
||||
cache_manager.delete("key") # alias for clear_cache(key)
|
||||
|
||||
# Advanced caching
|
||||
data = cache_manager.get_cached_data_with_strategy("key", data_type="weather")
|
||||
|
||||
@@ -138,29 +138,28 @@ font = self.font_manager.resolve_font(
|
||||
|
||||
## For Plugin Developers
|
||||
|
||||
> ⚠️ **Status**: the plugin-font registration described below is
|
||||
> implemented in `src/font_manager.py:150` (`register_plugin_fonts()`)
|
||||
> but is **not currently wired into the plugin loader**. Adding a
|
||||
> `"fonts"` block to your plugin's `manifest.json` will silently have
|
||||
> no effect — the FontManager method exists but nothing calls it.
|
||||
> **Note**: plugins that ship their own fonts via a `"fonts"` block
|
||||
> in `manifest.json` are registered automatically during plugin load
|
||||
> (`src/plugin_system/plugin_manager.py` calls
|
||||
> `FontManager.register_plugin_fonts()`). The `plugin://…` source
|
||||
> URIs documented below are resolved relative to the plugin's
|
||||
> install directory.
|
||||
>
|
||||
> Until that's connected, plugin authors who need a custom font
|
||||
> should load it directly with PIL (or `freetype-py` for BDF) in
|
||||
> their plugin's `manager.py` — `FontManager.resolve_font(family=…,
|
||||
> size_px=…)` takes a **family name**, not a file path, so it can't
|
||||
> be used to pull a font from your plugin directory. The
|
||||
> `plugin://…` source URIs described below are only honored by
|
||||
> `register_plugin_fonts()` itself, which isn't wired up.
|
||||
>
|
||||
> The `/api/v3/fonts/overrides` endpoints and the **Fonts** tab in
|
||||
> the web UI are currently **placeholder implementations** — they
|
||||
> return empty arrays and contain "would integrate with the actual
|
||||
> font system" comments. 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.
|
||||
> The **Fonts** tab in the web UI that lists detected
|
||||
> manager-registered fonts is still a **placeholder
|
||||
> implementation** — fonts that managers register through
|
||||
> `register_manager_font()` do not yet appear there. The
|
||||
> programmatic per-element override workflow described in
|
||||
> [Manual Font Overrides](#manual-font-overrides) below
|
||||
> (`set_override()` / `remove_override()` / the
|
||||
> `config/font_overrides.json` store) **does** work today and is
|
||||
> the supported way to override a font for an element until the
|
||||
> Fonts tab is wired up. If you can't wait and need a workaround
|
||||
> right now, you can also just load the font directly with PIL
|
||||
> (or `freetype-py` for BDF) inside your plugin's `manager.py`
|
||||
> and skip the override system entirely.
|
||||
|
||||
### Plugin Font Registration (planned)
|
||||
### Plugin Font Registration
|
||||
|
||||
In your plugin's `manifest.json`:
|
||||
|
||||
|
||||
@@ -336,15 +336,15 @@ pytest --cov=src --cov-report=html
|
||||
|
||||
## Continuous Integration
|
||||
|
||||
There is currently no CI test workflow in this repo — `pytest` runs
|
||||
locally but is not gated on PRs. The only GitHub Actions workflow is
|
||||
[`.github/workflows/security-audit.yml`](../.github/workflows/security-audit.yml),
|
||||
which runs bandit and semgrep on every push.
|
||||
|
||||
If you'd like to add a test workflow, the recommended setup is a
|
||||
`.github/workflows/tests.yml` that runs `pytest` against the
|
||||
supported Python versions (3.10, 3.11, 3.12, 3.13 per
|
||||
`requirements.txt`). Open an issue or PR if you want to contribute it.
|
||||
The repo runs
|
||||
[`.github/workflows/security-audit.yml`](../.github/workflows/security-audit.yml)
|
||||
(bandit + semgrep) on every push. A pytest CI workflow at
|
||||
`.github/workflows/tests.yml` is queued to land alongside this
|
||||
PR ([ChuckBuilds/LEDMatrix#307](https://github.com/ChuckBuilds/LEDMatrix/pull/307));
|
||||
the workflow file itself was held back from that PR because the
|
||||
push token lacked the GitHub `workflow` scope, so it needs to be
|
||||
committed separately by a maintainer. Once it's in, this section
|
||||
will be updated to describe what the job runs.
|
||||
|
||||
## Best Practices
|
||||
|
||||
|
||||
@@ -1,16 +1,5 @@
|
||||
# Plugin Custom Icons Guide
|
||||
|
||||
> ⚠️ **Status:** the `icon` field in `manifest.json` is currently
|
||||
> **not honored by the v3 web interface**. Plugin tab icons are
|
||||
> hardcoded to `fas fa-puzzle-piece` in
|
||||
> `web_interface/templates/v3/base.html:515` and `:774`. The icon
|
||||
> field was originally read by a `getPluginIcon()` helper in the v2
|
||||
> templates, but that helper wasn't ported to v3. Setting `icon` in a
|
||||
> manifest is harmless (it's just ignored) so plugin authors can leave
|
||||
> it in place for when this regression is fixed.
|
||||
>
|
||||
> Tracking issue: see the LEDMatrix repo for the open ticket.
|
||||
|
||||
## Overview
|
||||
|
||||
Plugins can specify custom icons that appear next to their name in the web interface tabs. This makes your plugin instantly recognizable and adds visual polish to the UI.
|
||||
|
||||
@@ -1,13 +1,12 @@
|
||||
# Plugin Custom Icons Feature
|
||||
|
||||
> ⚠️ **Status:** this doc describes the v2 web interface
|
||||
> implementation of plugin custom icons. The feature **regressed when
|
||||
> the v3 web interface was built** — the `getPluginIcon()` helper
|
||||
> referenced below lived in `templates/index_v2.html` (which is now
|
||||
> archived) and was not ported to the v3 templates. Plugin tab icons
|
||||
> in v3 are hardcoded to `fas fa-puzzle-piece`
|
||||
> (`web_interface/templates/v3/base.html:515` and `:774`). The
|
||||
> `icon` field in `manifest.json` is currently silently ignored.
|
||||
> **Note:** this doc was originally written against the v2 web
|
||||
> interface. The v3 web interface now honors the same `icon` field
|
||||
> in `manifest.json` — the API passes it through at
|
||||
> `web_interface/blueprints/api_v3.py` and the three plugin-tab
|
||||
> render sites in `web_interface/templates/v3/base.html` read it
|
||||
> with a `fas fa-puzzle-piece` fallback. The guidance below still
|
||||
> applies; only the referenced template/helper names differ.
|
||||
|
||||
## What Was Implemented
|
||||
|
||||
|
||||
Reference in New Issue
Block a user