From eba2d4a71197b714530231aa6858ce16244faeca Mon Sep 17 00:00:00 2001 From: Chuck Date: Tue, 7 Apr 2026 19:11:41 -0400 Subject: [PATCH] docs: address CodeRabbit review comments on #306 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewed all 12 CodeRabbit comments on PR #306, verified each against the current code, and fixed the 11 valid ones. The 12th finding is a real code bug (cache_manager.delete() calls in api_helper.py and resource_monitor.py) that's already in the planned follow-up code-fix PR, so it stays out of this docs PR. Fixed: .cursor/plugins_guide.md, .cursor/README.md, .cursorrules - I claimed "there is no --emulator flag" in 3 places. Verified in run.py:19-20 that the -e/--emulator flag is defined and functional (it sets os.environ["EMULATOR"]="true" before the display imports). Other docs I didn't touch (.cursor/plugin_templates/QUICK_START.md, docs/PLUGIN_DEVELOPMENT_GUIDE.md) already use the flag correctly. Replaced all 3 wrong statements with accurate guidance that both forms work and explains the CLI flag's relationship to the env var. .cursorrules, docs/GETTING_STARTED.md, docs/WEB_INTERFACE_GUIDE.md, docs/PLUGIN_DEVELOPMENT_GUIDE.md - Four places claimed "the plugin loader also falls back to plugins/". Verified that PluginManager.discover_plugins() (src/plugin_system/plugin_manager.py:154) only scans the configured directory — no fallback. The fallback to plugins/ exists only in two narrower places: store_manager.py:1700-1718 (store install/update/uninstall operations) and schema_manager.py:70-80 (schema lookup for the web UI form generator). Rewrote all four mentions with the precise scope. Added a recommendation to set plugin_system.plugins_directory to "plugins" for the smoothest dev workflow with dev_plugin_setup.sh symlinks. docs/FONT_MANAGER.md - The "Status" warning told plugin authors to use display_manager.font_manager.resolve_font(...) as a workaround for loading plugin fonts. Verified in src/font_manager.py that resolve_font() takes a family name, not a file path — so the workaround as written doesn't actually work. Rewrote to tell authors to load the font directly with PIL or freetype-py in their plugin. - The same section said "the user-facing font override system in the Fonts tab still works for any element that's been registered via register_manager_font()". Verified in web_interface/blueprints/api_v3.py:5404-5428 that /api/v3/fonts/overrides is a placeholder implementation that returns empty arrays and contains "would integrate with the actual font system" comments — the Fonts tab does not have functional integration with register_manager_font() or the override system. Removed the false claim and added an explicit note that the tab is a placeholder. docs/ADVANCED_FEATURES.md:523 - The on-demand section said REST/UI calls write a request "into the cache manager (display_on_demand_config key)". Wrong — verified via grep that api_v3.py:1622 and :1687 write to display_on_demand_request, and display_on_demand_config is only written by the controller during activation (display_controller.py:1195, cleared at :1221). Corrected the key name and added controller file:line references so future readers can verify. docs/ADVANCED_FEATURES.md:803 - "Plugins using the background service" paragraph listed all scoreboard plugins but an orphaned "⏳ MLB (baseball)" bullet remained below from the old version of the section. Removed the orphan and added "baseball/MLB" to the inline list for clarity. web_interface/README.md - The POST /api/v3/system/action action list was incomplete. Verified in web_interface/app.py:1383,1386 that enable_autostart and disable_autostart are valid actions. Added both. - The Plugin Store section was missing GET /api/v3/plugins/store/github-status (verified at api_v3.py:3296). Added it. - The SSE line-range reference was app.py:607-615 but line 619 contains the "Exempt SSE streams from CSRF and add rate limiting" block that's semantically part of the same feature. Extended the range to 607-619. docs/GETTING_STARTED.md - Rows/Columns step said "Columns: 64 or 96 (match your hardware)". The web UI's validation accepts any integer in 16-128. Clarified that 64 and 96 are the common bundled-hardware values but the valid range is wider. Not addressed (out of scope for docs PR): - .cursorrules:184 CodeRabbit comment flagged the non-existent cache_manager.delete() calls in src/common/api_helper.py:287 and src/plugin_system/resource_monitor.py:343. These are real CODE bugs, not doc bugs, and they're the first item in the planned post-docs-refresh code-cleanup PR (see /home/chuck/.claude/plans/warm-imagining-river.md). The docs in this PR correctly state that delete() doesn't exist on CacheManager — the fix belongs in the follow-up code PR that either adds a delete() shim or updates the two callers. Co-Authored-By: Claude Opus 4.6 (1M context) --- .cursor/README.md | 10 ++++++---- .cursor/plugins_guide.md | 5 ++++- .cursorrules | 29 ++++++++++++++++++++++++----- docs/ADVANCED_FEATURES.md | 14 +++++++++----- docs/FONT_MANAGER.md | 22 +++++++++++++++------- docs/GETTING_STARTED.md | 10 ++++++++-- docs/PLUGIN_DEVELOPMENT_GUIDE.md | 16 +++++++++++----- docs/WEB_INTERFACE_GUIDE.md | 5 ++++- web_interface/README.md | 7 +++++-- 9 files changed, 86 insertions(+), 32 deletions(-) diff --git a/.cursor/README.md b/.cursor/README.md index 0d62e0e1..0b6d621f 100644 --- a/.cursor/README.md +++ b/.cursor/README.md @@ -53,7 +53,8 @@ cp ../../.cursor/plugin_templates/*.template . ```bash # Emulator mode (development, no hardware required) -EMULATOR=true python3 run.py +python3 run.py --emulator +# (equivalent: EMULATOR=true python3 run.py) # Hardware (production, requires the rpi-rgb-led-matrix submodule built) python3 run.py @@ -65,9 +66,10 @@ sudo systemctl start ledmatrix python3 scripts/dev_server.py # then open http://localhost:5001 ``` -There is no `--emulator` flag — the emulator is selected via the -`EMULATOR=true` environment variable, which `src/display_manager.py:2` -checks at import time. +The `-e`/`--emulator` CLI flag is defined in `run.py:19-20` and +sets `os.environ["EMULATOR"] = "true"` before any display imports, +which `src/display_manager.py:2` then reads to switch between the +hardware and emulator backends. ### Managing Plugins diff --git a/.cursor/plugins_guide.md b/.cursor/plugins_guide.md index 4b989c38..62ba63b6 100644 --- a/.cursor/plugins_guide.md +++ b/.cursor/plugins_guide.md @@ -403,7 +403,10 @@ cd /path/to/LEDMatrix 2. **Test with the dev preview server**: `python3 scripts/dev_server.py` (then open `http://localhost:5001`). Or run the full display in emulator mode with - `EMULATOR=true python3 run.py`. There is no `--emulator` flag. + `python3 run.py --emulator` (or equivalently + `EMULATOR=true python3 run.py`). The `-e`/`--emulator` CLI flag is + defined in `run.py:19-20` and sets the same `EMULATOR` environment + variable internally. 3. **Check logs** for errors or warnings 4. **Update configuration** in `config/config.json` if needed 5. **Iterate** until plugin works correctly diff --git a/.cursorrules b/.cursorrules index ae732017..d0e073ed 100644 --- a/.cursorrules +++ b/.cursorrules @@ -6,9 +6,27 @@ The LEDMatrix project uses a plugin-based architecture. All display functionality (except core calendar) is implemented as plugins that are dynamically loaded from the directory configured by `plugin_system.plugins_directory` in `config.json` — the default is -`plugin-repos/` (per `config/config.template.json:130`), and the loader -also falls back to `plugins/` (used by `scripts/dev/dev_plugin_setup.sh` -for symlinks). +`plugin-repos/` (per `config/config.template.json:130`). + +> **Fallback note (scoped):** `PluginManager.discover_plugins()` +> (`src/plugin_system/plugin_manager.py:154`) only scans the +> configured directory — there is no fallback to `plugins/` in the +> main discovery path. A fallback to `plugins/` does exist in two +> narrower places: +> - `store_manager.py:1700-1718` — store operations (install/update/ +> uninstall) check `plugins/` if the plugin isn't found in the +> configured directory, so plugin-store flows work even when your +> dev symlinks live in `plugins/`. +> - `schema_manager.py:70-80` — `get_schema_path()` probes both +> `plugins/` and `plugin-repos/` for `config_schema.json` so the +> web UI form generation finds the schema regardless of where the +> plugin lives. +> +> The dev workflow in `scripts/dev/dev_plugin_setup.sh` creates +> symlinks under `plugins/`, which is why the store and schema +> fallbacks exist. For day-to-day development, set +> `plugin_system.plugins_directory` to `plugins` so the main +> discovery path picks up your symlinks. ## Plugin Structure @@ -74,8 +92,9 @@ Plugins are configured in `config/config.json`: open `http://localhost:5001`) — renders plugins in the browser without running the full display loop - Or run the full display in emulator mode: - `EMULATOR=true python3 run.py` (or `./scripts/dev/run_emulator.sh`). - There is no `--emulator` flag. + `python3 run.py --emulator` (or equivalently + `EMULATOR=true python3 run.py`, or `./scripts/dev/run_emulator.sh`). + The `-e`/`--emulator` CLI flag is defined in `run.py:19-20`. - Test plugin loading: Check logs for plugin discovery and loading - Validate configuration: Ensure config matches `config_schema.json` diff --git a/docs/ADVANCED_FEATURES.md b/docs/ADVANCED_FEATURES.md index ae3958c3..ec94df6c 100644 --- a/docs/ADVANCED_FEATURES.md +++ b/docs/ADVANCED_FEATURES.md @@ -519,7 +519,12 @@ curl http://localhost:5000/api/v3/display/on-demand/status > There is no public Python on-demand API. The display controller's > on-demand machinery is internal — drive it through the REST endpoints > above (or the web UI buttons), which write a request into the cache -> manager (`display_on_demand_config` key) that the controller polls. +> manager under the `display_on_demand_request` key +> (`web_interface/blueprints/api_v3.py:1622,1687`) that the controller +> polls at `src/display_controller.py:921`. A separate +> `display_on_demand_config` key is used by the controller itself +> during activation to track what's currently running (written at +> `display_controller.py:1195`, cleared at `:1221`). ### Duration Modes @@ -795,12 +800,11 @@ Enable background service per plugin in `config/config.json`: ### Plugins using the background service -The background data service is now used by all of the sports scoreboard -plugins (football, hockey, baseball, basketball, soccer, lacrosse, F1, -UFC), the odds ticker, and the leaderboard plugin. Each plugin's +The background data service is used by all of the sports scoreboard +plugins (football, hockey, baseball/MLB, basketball, soccer, lacrosse, +F1, UFC), the odds ticker, and the leaderboard plugin. Each plugin's `background_service` block (under its own config namespace) follows the same shape as the example above. -- ⏳ MLB (baseball) ### Error Handling & Fallback diff --git a/docs/FONT_MANAGER.md b/docs/FONT_MANAGER.md index 9178df6a..db0b216d 100644 --- a/docs/FONT_MANAGER.md +++ b/docs/FONT_MANAGER.md @@ -144,13 +144,21 @@ font = self.font_manager.resolve_font( > `"fonts"` block to your plugin's `manifest.json` will silently have > no effect — the FontManager method exists but nothing calls it. > -> Until that's connected, plugin authors should ship custom fonts as -> regular files inside the plugin directory (e.g., `assets/myfont.ttf`) -> and reference them by relative path from the plugin's `manager.py` -> via `display_manager.font_manager.resolve_font(...)` or by loading -> with PIL directly. The user-facing font override system in the -> **Fonts** tab still works for any element that's been registered via -> `register_manager_font()`. +> 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. ### Plugin Font Registration (planned) diff --git a/docs/GETTING_STARTED.md b/docs/GETTING_STARTED.md index 9958ee19..de6e15b6 100644 --- a/docs/GETTING_STARTED.md +++ b/docs/GETTING_STARTED.md @@ -72,7 +72,9 @@ You should see: 1. Open the **Display** tab 2. Set your matrix configuration: - **Rows**: 32 or 64 (match your hardware) - - **Columns**: 64 or 96 (match your hardware) + - **Columns**: commonly 64 or 96; the web UI accepts any integer + in the 16–128 range, but 64 and 96 are the values the bundled + panel hardware ships with - **Chain Length**: Number of panels chained horizontally - **Hardware Mapping**: usually `adafruit-hat-pwm` (with the PWM jumper mod) or `adafruit-hat` (without). See the root README for the full list. @@ -284,7 +286,11 @@ sudo journalctl -u ledmatrix-web -f > The plugin install location is configurable via > `plugin_system.plugins_directory` in `config.json`. The default is -> `plugin-repos/`; the loader also searches `plugins/` as a fallback. +> `plugin-repos/`. Plugin discovery (`PluginManager.discover_plugins()`) +> only scans the configured directory — it does not fall back to +> `plugins/`. However, the Plugin Store install/update path and the +> web UI's schema loader do also probe `plugins/` so the dev symlinks +> created by `scripts/dev/dev_plugin_setup.sh` keep working. ### Web Interface diff --git a/docs/PLUGIN_DEVELOPMENT_GUIDE.md b/docs/PLUGIN_DEVELOPMENT_GUIDE.md index e368bd62..acd7b00a 100644 --- a/docs/PLUGIN_DEVELOPMENT_GUIDE.md +++ b/docs/PLUGIN_DEVELOPMENT_GUIDE.md @@ -15,11 +15,17 @@ The solution uses **symbolic links** to connect plugin repositories to the `plug > **Plugin directory note:** the dev workflow described here puts > symlinks in `plugins/`. The plugin loader's *production* default is > `plugin-repos/` (set by `plugin_system.plugins_directory` in -> `config.json`), but it falls back to `plugins/` so the dev symlinks -> are picked up automatically. The Plugin Store installs to -> `plugin-repos/`. If you want both your dev symlinks *and* store -> installs to share the same directory, set `plugins_directory` to -> `plugins` in the General tab of the web UI. +> `config.json`). Importantly, the main discovery path +> (`PluginManager.discover_plugins()`) only scans the configured +> directory — it does **not** fall back to `plugins/`. Two narrower +> paths do: the Plugin Store install/update logic in `store_manager.py`, +> and `schema_manager.get_schema_path()` (which the web UI form +> generator uses to find `config_schema.json`). That's why plugins +> installed via the Plugin Store still work even with symlinks in +> `plugins/`, but your own dev plugin won't appear in the rotation +> until you either move it to `plugin-repos/` or change +> `plugin_system.plugins_directory` to `plugins` in the General tab +> of the web UI. The latter is the smoother dev setup. ## Quick Start diff --git a/docs/WEB_INTERFACE_GUIDE.md b/docs/WEB_INTERFACE_GUIDE.md index b4e6d2a2..09afc494 100644 --- a/docs/WEB_INTERFACE_GUIDE.md +++ b/docs/WEB_INTERFACE_GUIDE.md @@ -399,7 +399,10 @@ The web interface uses modern web technologies: **Plugins:** - Plugin directory: configurable via `plugin_system.plugins_directory` in `config.json` (default - `plugin-repos/`); the loader also searches `plugins/` as a fallback + `plugin-repos/`). Main plugin discovery only scans this directory; + the Plugin Store install flow and the schema loader additionally + probe `plugins/` so dev symlinks created by + `scripts/dev/dev_plugin_setup.sh` keep working. - Plugin config: `/config/config.json` (per-plugin sections) --- diff --git a/web_interface/README.md b/web_interface/README.md index 4f0cb0db..9fade350 100644 --- a/web_interface/README.md +++ b/web_interface/README.md @@ -86,7 +86,8 @@ reference for the most common endpoints: - `GET /api/v3/system/status` - System status - `POST /api/v3/system/action` - Control display (action body: `start_display`, `stop_display`, `restart_display_service`, - `restart_web_service`, `git_pull`, `reboot_system`, `shutdown_system`) + `restart_web_service`, `git_pull`, `reboot_system`, `shutdown_system`, + `enable_autostart`, `disable_autostart`) - `GET /api/v3/display/current` - Current display frame - `GET /api/v3/display/on-demand/status` - On-demand status - `POST /api/v3/display/on-demand/start` - Trigger on-demand display @@ -105,11 +106,13 @@ reference for the most common endpoints: ### Plugin Store - `GET /api/v3/plugins/store/list` - List available registry plugins +- `GET /api/v3/plugins/store/github-status` - GitHub authentication status - `POST /api/v3/plugins/store/refresh` - Refresh registry from GitHub ### Real-time Streams (SSE) SSE stream endpoints are defined directly on the Flask app -(`app.py:607-615`), not on the api_v3 blueprint: +(`app.py:607-619` — includes the CSRF exemption and rate-limit hookup +alongside the three route definitions), not on the api_v3 blueprint: - `GET /api/v3/stream/stats` - System statistics stream - `GET /api/v3/stream/display` - Display preview stream - `GET /api/v3/stream/logs` - Service logs stream