feat(baseball-scoreboard): adopt the core scroll orchestration (1.22.0) - #245
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 21 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 (2)
📝 WalkthroughWalkthroughThe baseball scoreboard now delegates scroll orchestration to shared core classes when available. It retains a bundled legacy implementation for older cores, preserves baseball-specific rendering, adds fallback verification, and updates version metadata to 1.22.0. ChangesBaseball scroll migration
Sequence Diagram(s)sequenceDiagram
participant BaseballPlugin
participant ScrollDisplay
participant SportsScroll
participant LegacyScroll
BaseballPlugin->>ScrollDisplay: import scrolling classes
ScrollDisplay->>SportsScroll: load shared implementations
alt Shared core available
SportsScroll-->>ScrollDisplay: return core classes
BaseballPlugin->>ScrollDisplay: render baseball content
else Shared core unavailable
ScrollDisplay->>LegacyScroll: load fallback classes
LegacyScroll-->>ScrollDisplay: return legacy classes
BaseballPlugin->>LegacyScroll: render baseball content
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high |
🟢 Metrics 91 complexity
Metric Results Complexity 91
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.
First B5 adoption. The orchestration half of scroll_display.py -- scroll-helper configuration, frame pumping, completion, settings resolution, native global_config['target_fps'] -- now comes from the core's src.common.sports_scroll (LEDMatrix 3.2.0). Only the baseball-specific content half stays: game cards and league separator icons. Guarded, not bare. The documented recipe subclasses core with an unguarded import, which is a B6 recipe rather than a B5 one: a plugin doing that and flooring at 3.2.0 still reaches users whose core misreports its version. The v3.1.0 release ships __version__ = "1.0.0", which the install gate treats as untrustworthy and lets through -- and that core has no src.common.sports_scroll, so the plugin would die at load with one journal line. Verified against a v3.1.0 worktree. So scroll_display.py selects between two complete implementations at import: the core-backed subclass when the module is there, and scroll_display_legacy (the previous implementation, frozen) when it is not. The except clause matches only the core module's own absence, so a failure raised *inside* a present-but-broken core surfaces instead of silently downgrading. The floor therefore does NOT rise to 3.2.0. The plugin does not require 3.2.0, it prefers it; nobody is cut off from updates. The floor rises at B6, when the fallback is deleted. The versions[0] spelling is migrated to ledmatrix_min_version, satisfying the new manifest gate. Content methods were lifted verbatim from the legacy source rather than retyped -- transcription error is exactly what the byte-identical gate exists to catch, and there was no reason to risk introducing any. Verified: - all 24 harness renders (8 sizes x 3 screens) byte-for-byte identical to 1.21.1: `diff -r` of the before/after output directories is empty - both import paths exercised: core on a 3.2.0 tree, legacy fallback on a v3.1.0 worktree (LegacyScrollDisplay, all content methods present) - attribute audit: every attribute the ported methods touch is provided by the core base, except _game_renderer, which the subclass initialises before super().__init__ because the base calls _load_separator_icons() from there - plugin suite: same 4 pre-existing failures as origin/main, no new ones - module-collision check clean across 42 plugins - on devpi: loads via the CORE path (no fallback message), included in Vegas scroll, building 194x64 items, no tracebacks Live-game behaviour still to be observed -- MLB first pitches are ~19:00 EDT and there were no live games at deploy time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Adopting core code is only safe because scroll_display.py falls back to scroll_display_legacy.py when src.common.sports_scroll is absent. Nothing verified that: the safety harness renders against the *current* core, so it exercises exactly one of the three configurations this plugin can meet. The floor does not close the gap. A user who installed from the v3.1.0 release reports __version__ = "1.0.0", which the install gate treats as untrustworthy and lets through, and that core has no src.common.sports_scroll. The fallback is the only thing between them and a scoreboard that fails to load. core module bundled copy asserted ------------ ------------- --------------------------------------------- present present core implementation, manager wired to it absent present legacy implementation, and still able to draw absent absent B6 state: fails naming the missing module An import blocker simulates the older core rather than a pinned worktree: reversible, leaves the checkout untouched, and reproduces exactly what Python does when the module genuinely is not there. The second row also asserts the fallback still exposes prepare_scroll_content, display_scroll_frame, is_scroll_complete, get_dynamic_duration and _load_separator_icons -- a fallback that imports but cannot draw is no fallback. Verified the test actually bites, by sabotaging the guard to match the unguarded recipe: it fails with the fallback removed (exit 1), passes on the healthy tree (exit 0), and skips with no core on PYTHONPATH (exit 2). That sabotage run caught a flaw in the test itself. It had been deciding "skip" from an exception raised *during* a test, so the escaping ModuleNotFoundError -- the exact regression this file exists to catch -- was swallowed as "no core on PYTHONPATH". The core check is now a pre-flight before any test runs, and any ModuleNotFoundError after that is a failure. Same misclassification this repo just fixed across the other suites. This is the acceptance test B6 needs, available now: it makes the remaining eight adoptions self-verifying instead of hand-checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
ChuckBuilds
force-pushed
the
feat/baseball-adopts-core-scroll
branch
from
August 4, 2026 18:29
ef29415 to
cd122f2
Compare
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/baseball-scoreboard/scroll_display.py (1)
298-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an explicit
Optionalforrankings_cache.Line 303 declares
rankings_cache: Dict[str, int] = None. PEP 484 does not allow implicitOptional. Ruff reports RUF013 here. The override onScrollDisplay.prepare_scroll_content(line 173) already usesOptional[Dict[str, int]], so this also aligns the two signatures.♻️ Proposed fix
def prepare_content( self, games: List[Dict], game_type: str, leagues: List[str], - rankings_cache: Dict[str, int] = None + rankings_cache: Optional[Dict[str, int]] = None ) -> bool:🤖 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 `@plugins/baseball-scoreboard/scroll_display.py` around lines 298 - 304, Update the rankings_cache annotation in prepare_content to explicitly use Optional[Dict[str, int]] while preserving its default None value, matching the existing annotation on prepare_scroll_content.Source: Linters/SAST tools
plugins/baseball-scoreboard/scroll_display_legacy.py (1)
365-412: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
renderercan beNoneand every game then fails through the exception handler.
_get_game_rendererreturnsNonewhenGameRendereris unavailable. Line 398 then raisesAttributeErrorfor each game, and the loop logs a full traceback per game beforecontent_itemsstays empty. The fallback is safe, but the failure mode is noisy. Return early instead.♻️ Proposed early return
renderer = self._get_game_renderer(game_card_width) + if renderer is None: + self.logger.error("GameRenderer not available - cannot prepare scroll content") + return False + # Pass rankings cache to renderer if available - if renderer and rankings_cache: + if rankings_cache: renderer.set_rankings_cache(rankings_cache)🤖 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 `@plugins/baseball-scoreboard/scroll_display_legacy.py` around lines 365 - 412, Handle a missing renderer immediately after _get_game_renderer returns in the surrounding display-rendering method: log one appropriate message if needed and return before entering the games loop. Keep the existing rendering and exception handling unchanged for a valid renderer, avoiding per-game AttributeError tracebacks when GameRenderer is unavailable.
🤖 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 `@plugins/baseball-scoreboard/scroll_display_legacy.py`:
- Line 582: Update the _scroll_displays annotation in
LegacyScrollDisplayManager.__init__ to use the defined LegacyScrollDisplay class
instead of the undefined ScrollDisplay name, preventing runtime NameError during
initialization.
---
Nitpick comments:
In `@plugins/baseball-scoreboard/scroll_display_legacy.py`:
- Around line 365-412: Handle a missing renderer immediately after
_get_game_renderer returns in the surrounding display-rendering method: log one
appropriate message if needed and return before entering the games loop. Keep
the existing rendering and exception handling unchanged for a valid renderer,
avoiding per-game AttributeError tracebacks when GameRenderer is unavailable.
In `@plugins/baseball-scoreboard/scroll_display.py`:
- Around line 298-304: Update the rankings_cache annotation in prepare_content
to explicitly use Optional[Dict[str, int]] while preserving its default None
value, matching the existing annotation on prepare_scroll_content.
🪄 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: ec191a7f-f35d-47aa-8e11-baec975dbda4
📒 Files selected for processing (6)
plugins.jsonplugins/baseball-scoreboard/CHANGELOG.mdplugins/baseball-scoreboard/manifest.jsonplugins/baseball-scoreboard/scroll_display.pyplugins/baseball-scoreboard/scroll_display_legacy.pyplugins/baseball-scoreboard/test_core_fallback.py
…otation Brings this plugin's fallback test up to the version that found the bug in the other six: it resolves the globals each method reads and fails on any the module cannot supply, across both the display and manager classes and on both import paths. This plugin's header was hand-written and was already clean, so nothing changes here -- but it had only the hasattr checks, which is what missed the problem elsewhere. Also corrects the legacy manager's `Dict[str, ScrollDisplay]` annotation to the post-rename name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
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.
First B5 adoption. Baseball chosen over hockey because the MLB season is active, so this can be exercised against live games rather than only the harness.
The orchestration half of
scroll_display.py— scroll-helper configuration, frame pumping, completion, settings resolution, nativeglobal_config['target_fps']— now comes from the core'ssrc.common.sports_scroll(LEDMatrix 3.2.0). Only the baseball-specific content half stays here.Guarded, not bare — and why that matters
The documented recipe subclasses core with an unguarded import. Following it here would have shipped a broken plugin, so I didn't.
A plugin that adopts unguarded and floors at 3.2.0 still reaches users whose core misreports its version. The v3.1.0 release ships
__version__ = "1.0.0", which the install gate treats as untrustworthy and lets through — and that core has nosrc.common.sports_scroll:3.2.03.1.01.0.0ModuleNotFoundError→ scoreboard silently goneVerified against a
v3.1.0worktree: the module is absent there.So
scroll_display.pyselects between two complete implementations at import — the core-backed subclass when available,scroll_display_legacy.py(the previous implementation, frozen) when not. Theexceptmatches only the core module's own absence, so a failure raised inside a present-but-broken core surfaces rather than silently downgrading.The floor does not rise
Because the fallback exists, the plugin does not require 3.2.0 — it prefers it.
ledmatrix_min_versionstays at2.0.0and nobody is cut off from updates. It rises at B6, when the fallback is deleted.This corrects an assumption we'd been carrying: guarded adoption does not gate users behind the new core. Only the sunset does.
The
versions[0]spelling is migrated toledmatrix_min_version, satisfying the new manifest gate in #244.Verification
diff -rof the before/after output directories is empty. This is the acceptance gate.v3.1.0worktree, whereScrollDisplayresolves toLegacyScrollDisplaywith all content methods intact._game_renderer, which the subclass initialises beforesuper().__init__, because the base calls_load_separator_icons()from there.origin/main, no new ones. Module-collision check clean across 42 plugins.194x64items, no tracebacks.Outstanding
Live-game behaviour has not been observed yet — MLB first pitches are ~19:00 EDT and there were no live games at deploy time. That's the specific thing baseball was chosen to test, so I'd hold this until it has been watched through a live game.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit