Add skin system: user-installable visual overlays for sports scoreboards - #419
Merged
Conversation
Skins restyle a scoreboard's live/recent/upcoming rendering while the host plugin keeps doing data fetching, scheduling, caching, live priority, and vegas mode — the anti-fork alternative for users who only want a different layout. - src/skin_system/: ScoreboardSkin API, SkinContext (canvas + adaptive layout + logo/font helpers), discovery/loading runtime with API major version gating and per-skin module namespacing - src/base_classes/sports.py: _render_game() seam at the three _draw_scorebug_layout call sites; skin-first with built-in fallback, 3-strikes session disable, slow-render warning; per-mode skin config - scripts/validate_skin.py: headless multi-mode/multi-size validator with bundled per-sport fixtures (no hardware or network needed) - skins/example-classic-baseball/: working reference skin - Web UI: served-schema Visual Skin dropdown (validation never enum-restricted, so uninstalled skins can't invalidate configs) and GET /api/v3/skins - Store: registry entries with type "skin" install to skins/ - docs/SKIN_SYSTEM.md (architecture), docs/CREATING_SKINS.md (author guide incl. Claude Code prompt), view-model contract locked by tests Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01LrCusPasy1qeUN5anK3aA1
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 169 |
| Duplication | 4 |
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.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds an installable scoreboard skin system with a public rendering API, runtime discovery and loading, sports display integration with fallback behavior, configuration and store support, a baseball example skin, validation tooling, fixtures, tests, and documentation. ChangesScoreboard skin system
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DisplayLoop
participant SportsCore
participant SkinRuntime
participant Skin
participant BuiltInRenderer
DisplayLoop->>SportsCore: display current game
SportsCore->>SkinRuntime: resolve and load configured skin
SkinRuntime-->>SportsCore: return skin context and instance
SportsCore->>Skin: render live, recent, or upcoming mode
Skin-->>SportsCore: handled or declined
SportsCore->>BuiltInRenderer: render built-in layout on failure or decline
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 13
🧹 Nitpick comments (3)
web_interface/blueprints/api_v3.py (1)
5314-5318: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the existing discovery cache for this read endpoint.
force_refresh=Truereparses every skin manifest on each unfiltered request despite directory-mtime cache invalidation already detecting installs and removals. Calldiscover_skins()normally to reduce Raspberry Pi disk I/O and CPU usage.As per coding guidelines, “Optimize code for Raspberry Pi's limited RAM and CPU capabilities.”
🤖 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 `@web_interface/blueprints/api_v3.py` around lines 5314 - 5318, Update the unfiltered branch in the endpoint around plugin_id to call skin_runtime.discover_skins() without force_refresh=True, preserving the existing skin_runtime.skins_for_plugin(plugin_id) behavior for filtered requests.Source: Coding guidelines
test/test_skin_system.py (1)
326-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the extractor’s returned mapping, not its source text.
String matching can pass when a key appears only in comments or unreachable code. Invoke representative concrete sport extractors and assert
GUARANTEED_KEYSagainst their returned dictionaries.As per coding guidelines, tests should validate individual component behavior in isolation.
🤖 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_skin_system.py` around lines 326 - 337, Replace the source-text inspection in test_extractor_produces_guaranteed_keys with direct calls to representative concrete sport extractor implementations, then assert every GUARANTEED_KEYS entry exists in each returned mapping. Keep the test focused on the extractor contract and validate each concrete component’s output independently rather than inspecting SportsCore._extract_game_details_common source.Source: Coding guidelines
scripts/validate_skin.py (1)
40-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the
FixtureHostcontract.Annotate
__init__,_load_fonts,_load_and_resize_logo, and_draw_text_with_outline; this shim must remain aligned with the host interface consumed bybuild_context().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 `@scripts/validate_skin.py` around lines 40 - 84, Add type annotations to FixtureHost.__init__, _load_fonts, _load_and_resize_logo, and _draw_text_with_outline, covering every parameter and return value. Use types consistent with the host interface consumed by build_context(), including appropriate image, font, mapping, optional, and drawing-context types.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 `@docs/CREATING_SKINS.md`:
- Around line 37-55: Make the skin.json manifest example valid JSON by removing
all inline // comments from the fenced json block. Keep the manifest fields and
values unchanged so the example can be copied and parsed directly.
- Around line 78-81: Update the logo-loading guidance around ctx.load_logo and
render_live so render hooks never trigger network I/O or blocking downloads.
Require logos to be preloaded before rendering, or provide and document a
strictly cache-only helper for render-time use; remove the “auto-downloaded”
recommendation and preserve the existing cache_key guidance.
In `@docs/SKIN_SYSTEM.md`:
- Around line 24-31: Add the text language identifier to the fenced architecture
diagram in docs/SKIN_SYSTEM.md lines 24-31, the package-layout fence in
docs/SKIN_SYSTEM.md lines 100-107, and the directory-layout fence in
skins/README.md lines 6-11. Do not alter the contents of these diagrams.
In `@scripts/validate_skin.py`:
- Around line 97-102: Update parse_size to reject zero or negative width and
height, raising a clear argparse.ArgumentTypeError before the skin is loaded. In
the --options parsing/validation flow near build_context, require the decoded
JSON value to be an object/dictionary and return a clear argparse error for
arrays, strings, or other non-object values before dict(...) is called.
- Around line 191-195: Update the output reporting in the render flow around
ctx.canvas.save and the corresponding Vegas-render reporting at lines 204-206 so
paths outside PROJECT_ROOT are supported. Replace unconditional
Path.relative_to(PROJECT_ROOT) usage with a fallback that prints the original
output path when it is not repository-relative, while preserving
repository-relative formatting for paths inside PROJECT_ROOT and ensuring valid
renders are not reported as failures.
In `@skins/example-classic-baseball/skin.py`:
- Around line 21-27: Validate the accent_color option during ClassicBaseballSkin
instantiation rather than in _accent, requiring a three-channel RGB sequence
with valid channel values and rejecting malformed strings or lengths. On invalid
input, log a clear error and fall back to the default (255, 200, 0), while
preserving _accent’s use of the validated option at render time.
In `@src/base_classes/sports.py`:
- Around line 302-304: Update the card-render exception handler in the relevant
render method to increment the skin’s _skin_failures count before returning
None. Reuse the existing three-strike disablement logic and ensure these
exceptions follow the same session-level skin disabling behavior as other skin
failures.
In `@src/plugin_system/schema_manager.py`:
- Around line 389-406: Update the skin schema construction in the surrounding
schema-enhancement method to detect object-valued current_value and preserve
per-mode configurations via a oneOf schema. Support both the existing global
string selector and an object containing live, recent, and upcoming selectors,
while retaining the current global enum/default behavior for non-object values.
In `@src/plugin_system/store_manager.py`:
- Around line 2279-2284: Centralize safe skin-ID validation and path-containment
checks in the skin install/uninstall flow. In src/plugin_system/store_manager.py
lines 2279-2284, validate the registry ID before constructing the target and
require its resolved parent to equal the resolved skins directory; in lines
2346-2353, validate the manifest ID and reject registry/manifest mismatches; in
lines 2364-2369, apply the same validation before uninstalling. Fail fast on
invalid IDs or containment violations.
- Around line 2282-2285: Update the skin installation flow around target and
_safe_remove_directory to stage the replacement in a temporary sibling directory
before touching the existing installation. Perform download, manifest JSON
parsing, and API compatibility validation against the staged directory, then
atomically swap it into target only after validation succeeds. Preserve the
current installation on any failure and roll back the swap if replacement fails,
allowing non-critical installation errors to degrade gracefully.
In `@src/skin_system/fixtures/football_upcoming.json`:
- Around line 7-9: Update the upcoming fixture score data to use pregame values.
In src/skin_system/fixtures/football_upcoming.json at lines 7-9, retain the
upcoming state and reset both scores at lines 14-20 to "0"; make the same score
reset in src/skin_system/fixtures/hockey_upcoming.json at lines 14-20 while
retaining its upcoming state at lines 7-9.
In `@src/skin_system/skin_runtime.py`:
- Around line 94-117: Update the discovery cache logic in the skin discovery
function to include each existing skin.json manifest’s modification time in the
cache fingerprint, not only the skins directory mtime. Recompute and compare
this fingerprint before returning _discovery_cache entries so in-place manifest
updates trigger rediscovery while preserving cached results when neither
directories nor manifests changed.
- Around line 174-220: Update the sibling-loading flow around the alias check
and entry-point execution so cached namespaced modules are always rebound to
their sibling bare names before importing the entry point; do not skip alias
creation merely because _skin_<skin_id>_<sibling> already exists. Track each
bare-name binding that was replaced or added, then restore the original
sys.modules entries in the cleanup after entry import, while preserving cached
module reuse and existing failure cleanup.
---
Nitpick comments:
In `@scripts/validate_skin.py`:
- Around line 40-84: Add type annotations to FixtureHost.__init__, _load_fonts,
_load_and_resize_logo, and _draw_text_with_outline, covering every parameter and
return value. Use types consistent with the host interface consumed by
build_context(), including appropriate image, font, mapping, optional, and
drawing-context types.
In `@test/test_skin_system.py`:
- Around line 326-337: Replace the source-text inspection in
test_extractor_produces_guaranteed_keys with direct calls to representative
concrete sport extractor implementations, then assert every GUARANTEED_KEYS
entry exists in each returned mapping. Keep the test focused on the extractor
contract and validate each concrete component’s output independently rather than
inspecting SportsCore._extract_game_details_common source.
In `@web_interface/blueprints/api_v3.py`:
- Around line 5314-5318: Update the unfiltered branch in the endpoint around
plugin_id to call skin_runtime.discover_skins() without force_refresh=True,
preserving the existing skin_runtime.skins_for_plugin(plugin_id) behavior for
filtered requests.
🪄 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: 10d00a56-03a2-4b72-ad49-0fcbdffa18f4
⛔ Files ignored due to path filters (3)
skins/example-classic-baseball/preview.pngis excluded by!**/*.pngsrc/skin_system/fixtures/placeholder_away.pngis excluded by!**/*.pngsrc/skin_system/fixtures/placeholder_home.pngis excluded by!**/*.png
📒 Files selected for processing (30)
.gitignoreCLAUDE.mdREADME.mddocs/CREATING_SKINS.mddocs/PLUGIN_DEVELOPMENT_GUIDE.mddocs/SKIN_SYSTEM.mdscripts/validate_skin.pyskins/README.mdskins/example-classic-baseball/skin.jsonskins/example-classic-baseball/skin.pysrc/base_classes/sports.pysrc/plugin_system/schema_manager.pysrc/plugin_system/store_manager.pysrc/skin_system/__init__.pysrc/skin_system/fixtures/baseball_live.jsonsrc/skin_system/fixtures/baseball_recent.jsonsrc/skin_system/fixtures/baseball_upcoming.jsonsrc/skin_system/fixtures/basketball_live.jsonsrc/skin_system/fixtures/basketball_recent.jsonsrc/skin_system/fixtures/basketball_upcoming.jsonsrc/skin_system/fixtures/football_live.jsonsrc/skin_system/fixtures/football_recent.jsonsrc/skin_system/fixtures/football_upcoming.jsonsrc/skin_system/fixtures/hockey_live.jsonsrc/skin_system/fixtures/hockey_recent.jsonsrc/skin_system/fixtures/hockey_upcoming.jsonsrc/skin_system/skin_base.pysrc/skin_system/skin_runtime.pytest/test_skin_system.pyweb_interface/blueprints/api_v3.py
- skin_runtime: cache the entry module so the 2nd/3rd load of the same skin (live/recent/upcoming hosts) doesn't re-execute it with unbound sibling aliases; rebind cached sibling modules to their bare names around entry import and restore prior bindings after; include per- manifest mtimes in the discovery cache fingerprint so in-place skin updates are picked up - sports.py: count render_skin_card exceptions toward the 3-strike session disable - store_manager: validate skin ids (pattern + resolved-path containment in skins/), reject registry/manifest id mismatches, and stage+validate downloads in a temp sibling before replacing an existing skin - schema_manager: leave the schema untouched when the configured skin value is a per-mode mapping (a string dropdown could overwrite it) - validate_skin.py: reject non-positive sizes and non-object --options at parse time; support --output-dir outside the repo; type annotations - example skin: validate accent_color once at load with logged fallback - fixtures: pregame 0-0 scores in football/hockey upcoming fixtures - api /skins: rely on the self-invalidating discovery cache instead of force_refresh - docs: valid JSON manifest example, load_logo caching semantics spelled out, language ids on fenced blocks - tests: view-model contract test now exercises the real extractor; regression test for repeated same-skin loads with sibling modules Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01LrCusPasy1qeUN5anK3aA1
Owner
Author
|
Pushed 813cd61 addressing the review. All findings fixed as suggested except two, resolved differently on purpose:
Also of note: the sibling-module finding was a real bug (the same skin loads three times, once per mode host) — fixed by caching the entry module and rebinding cached sibling aliases, with a regression test. Generated by Claude Code |
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.
Pull Request
Summary
Adds a first-class skin system so users can restyle a sports scoreboard's visuals (live/recent/upcoming) without forking the plugin — the host plugin keeps doing data fetching, scheduling, caching, live-priority, and vegas mode, while a skin (~100 lines of drawing code in
skins/<skin-id>/) replaces only the rendering. This is the anti-fork answer to community forks like the MLB scoreboard variant that lost duration/scheduling/vegas/caching support.How it works: all sports scoreboards funnel rendering through three
_draw_scorebug_layoutcall sites insrc/base_classes/sports.py; those now go through_render_game(), which tries the configured skin first (ScoreboardSkin.render_<mode>(ctx, game)drawing onto a provided canvas) and falls back to the built-in layout onFalse, exception (3 strikes disables the skin for the session), or no skin. The existing extracted game dict is frozen as a versioned view-model contract, locked by tests. Includes a headless validator (scripts/validate_skin.py, fixtures, no hardware/network needed), a reference skin (skins/example-classic-baseball/), a web UI "Visual Skin" dropdown +GET /api/v3/skins, store support for registry entries with"type": "skin", and full docs (docs/SKIN_SYSTEM.md,docs/CREATING_SKINS.mdwith a ready-made Claude Code authoring prompt).Type of change
Related issues
None — implements the "visual overlay / skin" idea for reusing plugin data + framework with custom layouts.
Test plan
EMULATOR=true python3 run.py)pytest)scripts/dev_server.py)Details:
test/test_skin_system.py: 52 tests covering discovery, manifest validation, API major-version gating, per-skin module namespacing/isolation, fallback semantics (decline / no skin / 3-strikes disable), game-dict copy protection, per-mode skin config resolution, view-model contract keys, plugin↔skin matching, schema enum injection, and the example skin rendering all modes at 128x32 / 64x32 / 128x64.EMULATOR=true pytest test/ --ignore=test/plugins): 1034 passed; the 4 remaining failures (test_save_double_sided_*, 2 state-reconciliation tests) were verified to fail on a clean checkout too — pre-existing, unrelated.python scripts/validate_skin.py --skin example-classic-baseball --size 128x32 --size 64x32 --size 128x64: 9/9 renders pass; PNGs eyeballed.skinconfigured,_render_game()is a straight passthrough to_draw_scorebug_layout— existing behavior verified unchanged by the suite.Documentation
README.mdif user-facing behavior changeddocs/if developer behavior changed (newdocs/SKIN_SYSTEM.md,docs/CREATING_SKINS.md; pointers indocs/PLUGIN_DEVELOPMENT_GUIDE.md,CLAUDE.md,skins/README.md)Plugin compatibility
The hook lands entirely in this repo's
SportsCorebase classes; monorepo sport plugins need zero changes (their_draw_scorebug_layoutoverrides become the fallback path).skin/skin_optionsare injected as always-allowed core properties in schema validation, so no plugin'sconfig_schema.jsonneeds touching. Important invariant introduced: the guaranteed view-model keys emitted by_extract_game_details_common(and sport extras) are now a published contract for skins —TestViewModelContractfails CI on renames.Checklist
CONTRIBUTING.mdCONTRIBUTING.mdandCODE_OF_CONDUCT.mdconfig_schema.json)The web UI "Visual Skin" dropdown is injected into the served schema only (
SchemaManager.inject_skin_selector); validation is deliberately never enum-restricted so a config referencing an uninstalled skin stays valid and rendering falls back to built-in.Notes for reviewer
skins/, not plugin dirs: plugin reinstall/update deletes the whole plugin directory (store_manager), so skins must live outside it to survive; it also lets one skin target multiple plugins (mlb + milb)."skin"accepts a plain id (all modes) or{"live": "a", "recent": "built-in"}— one skin per mode per plugin, different skins across modes/plugins are fine.CREATING_SKINS.md. Skins are arbitrary Python at plugin trust level — stated plainly in the docs.skin_runtimeis deliberately sports-agnostic so a futureBasePluginopt-in (weather/music skins) is a small follow-up. No animation API in v1 (one frame per render call).skin_runtime.skins_for_plugin(used only for the UI dropdown; explicittargets.pluginsis available for exact matching).🤖 Generated with Claude Code
https://claude.ai/code/session_01LrCusPasy1qeUN5anK3aA1
Generated by Claude Code
Summary by CodeRabbit