fix(odds-ticker): restore team logos for NCAA basketball and baseball - #256
Merged
Conversation
`_get_team_logo()` discarded the logo directory the data fetcher had already resolved for the league (`_ = logo_dir`) and looked the league up in a hardcoded map instead. That map had drifted: the fetcher emits `ncaam_basketball` and `ncaa_baseball`, and the map had neither, so both resolved to no directory and returned None for every team. The failure was invisible from both sides. On screen the layout reserves `logo_size` width unconditionally but only draws a logo `if home_logo:`, so a missing logo is a blank gap rather than a closed-up row -- the game renders with its info and empty space where the logos belong. In the logs the miss was recorded at debug, so nothing showed up at the default level to explain the gap. Fixed at the root rather than by adding the two absent keys: the renderer now tries the directory it was handed first and falls back to the map, which means a league added to the fetcher works here without a second table being updated in lockstep. The map is completed as well, for callers that pass no directory. Missing logos now warn once per team, naming every path searched. Verified against the assets on a live 512x64 device, resolving a real team for each league the fetcher can emit, with and without logo_dir: before, ncaam_basketball and ncaa_baseball failed both ways (4 failures); after, all seven leagues resolve. MLB was already working -- rendering a real Rays game end-to-end draws both team logos correctly -- so this is not the cause of every missing-logo report, but it is a real one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe odds ticker now uses league-specific logo fallbacks, supports NCAA basketball and baseball directories, and limits missing-logo warnings to one per team. Plugin metadata and release notes now identify version 1.1.10. ChangesOdds ticker logo resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant OddsRenderer
participant LogoFilesystem
participant Logger
OddsRenderer->>LogoFilesystem: Try caller-provided logo directory
OddsRenderer->>LogoFilesystem: Try mapped league directories
LogoFilesystem-->>OddsRenderer: Return logo or load failure
OddsRenderer->>Logger: Warn once for missing (league, team)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
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.
19 tasks
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.
Reported as "the odds ticker shows game info but no team logos". Investigating it turned up a definite cause — though not for the game that prompted the report; see the honesty note at the end.
The bug
_get_team_logo()discarded the logo directory the data fetcher had already resolved for the league (_ = logo_dir) and looked the league up in a hardcoded map instead. That map had drifted out of sync with the fetcher:logo_leaguethe fetcher emitsnfl,mlb,nba,nhl,milb,ncaa_fbncaam_basketballncaa_baseballBoth resolved to no directory and returned
Nonefor every team, in every game.Why nobody could see it
The failure was invisible from both directions, which is why it survived:
logo_sizewidth unconditionally, but only drawsif home_logo:. A missing logo is therefore a blank gap, not a closed-up row — the game renders normally with empty space where the logos belong, which reads as a layout quirk rather than a failure.debug, so at the default level there was nothing to explain the gap.The fix
Fixed at the root rather than by adding the two absent keys. The renderer now tries the directory it was handed first and falls back to the map, so a league added to the fetcher works here without a second table having to be updated in lockstep — the exact drift that caused this. The map is completed too, for callers that pass no directory.
Missing logos now warn once per team, naming every path searched.
Verification
Against the real assets on a live 512x64 device, resolving a genuine team for each league the fetcher can emit — both with
logo_dirand with it omitted so the fallback map carries it alone:Core harness passes at 128x32 and 512x64.
Honesty note on the original report
The report named a Rays game, and MLB was already working —
TB.pngresolves, and rendering the real Rays game (event 401816411) end-to-end through this renderer draws both team logos correctly. So this PR is not the explanation for that particular sighting. It is a real instance of the same failure, found while chasing it.If a missing logo recurs on MLB, the new warning will now name the exact directories searched, which is what was missing to diagnose it the first time.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
Bug Fixes
Release