fix(masters): post-tournament phase and headshot bugs (v2.4.2)#103
Merged
ChuckBuilds merged 2 commits intoChuckBuilds:mainfrom Apr 16, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis PR updates the Masters Tournament plugin with four distinct changes: enhanced caching support for player headshots when URLs are available, a new version entry in the manifest, corrected fallback tournament end-time calculation for EDT midnight boundaries, and refined post-tournament classification logic using calendar-day differences instead of duration windows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
plugins/masters-tournament/logo_loader.py (1)
167-198: Normalize URL before using it as cache/log identifier.Using raw URLs directly can create high-cardinality cache keys and log query parameters. Prefer a normalized URL (or hash) when
player_idis absent.♻️ Suggested refinement
@@ import logging import os from io import BytesIO from pathlib import Path from typing import Dict, Optional, Tuple +from urllib.parse import urlsplit, urlunsplit @@ - cache_key = f"player_{player_id or url}_{max_size}" + normalized_url = None + if url: + parts = urlsplit(url) + normalized_url = urlunsplit((parts.scheme, parts.netloc, parts.path, "", "")) + id_for_cache = player_id or normalized_url + cache_key = f"player_{id_for_cache}_{max_size}" @@ - logger.warning(f"Failed to download headshot for {player_id or url}: {e}") + logger.warning(f"Failed to download headshot for {id_for_cache}: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/masters-tournament/logo_loader.py` around lines 167 - 198, The cache/logging currently uses the raw URL when player_id is absent which can create high-cardinality keys and leak query params; update the logic around cache_key creation (used in cache_key, player_id, url) to normalize the URL or replace it with a stable hashed identifier (e.g., compute an sha256/hex of url) before building cache_key and before logging in the download error path, and ensure the disk filename logic (players_dir / f"{player_id}.png") remains unchanged when player_id exists; apply this normalization/hash in both the cache lookup and when emitting logger.warning in the download/cached-load exception handlers so no raw URL/query string is stored or logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/masters-tournament/masters_helpers.py`:
- Line 458: Replace the non-ASCII multiplication sign in the inline comment
"means N full calendar days, not N×24 hours from the padded end" with an ASCII
character to satisfy Ruff RUF003; update that comment (the one containing "means
N full calendar days, not N×24 hours from the padded end") to use "x" or "times"
(e.g., "not N x 24 hours" or "not N times 24 hours") so only ASCII characters
remain.
---
Nitpick comments:
In `@plugins/masters-tournament/logo_loader.py`:
- Around line 167-198: The cache/logging currently uses the raw URL when
player_id is absent which can create high-cardinality keys and leak query
params; update the logic around cache_key creation (used in cache_key,
player_id, url) to normalize the URL or replace it with a stable hashed
identifier (e.g., compute an sha256/hex of url) before building cache_key and
before logging in the download error path, and ensure the disk filename logic
(players_dir / f"{player_id}.png") remains unchanged when player_id exists;
apply this normalization/hash in both the cache lookup and when emitting
logger.warning in the download/cached-load exception handlers so no raw
URL/query string is stored or logged.
🪄 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: 0cd5d819-21d9-47b0-86d5-ff3345082196
📒 Files selected for processing (5)
plugins.jsonplugins/masters-tournament/logo_loader.pyplugins/masters-tournament/manifest.jsonplugins/masters-tournament/masters_data.pyplugins/masters-tournament/masters_helpers.py
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/masters-tournament/logo_loader.py (1)
204-205: Narrow exception handling in download path.At line 204,
except Exceptionis too broad and may mask non-I/O bugs. Catch request/image-related exceptions explicitly:- except Exception as e: + except (requests.RequestException, OSError) as e: logger.warning(f"Failed to download headshot for {stable_key}: {e}")This keeps failures scoped to expected exceptions from HTTP requests, image processing, and file I/O operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/masters-tournament/logo_loader.py` around lines 204 - 205, Replace the broad "except Exception as e" that logs "Failed to download headshot for {stable_key}" with narrow exception handling for expected I/O/image/HTTP errors: catch requests.exceptions.RequestException, PIL.UnidentifiedImageError (or PIL.Image.DecompressionBombError if relevant), and OSError (file I/O) and log the same message with the caught exception; ensure you add the necessary imports (requests.exceptions and PIL exception classes) and keep the logger.warning call and stable_key reference intact so only request/image/file errors are masked, not unrelated bugs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/masters-tournament/logo_loader.py`:
- Around line 204-205: Replace the broad "except Exception as e" that logs
"Failed to download headshot for {stable_key}" with narrow exception handling
for expected I/O/image/HTTP errors: catch requests.exceptions.RequestException,
PIL.UnidentifiedImageError (or PIL.Image.DecompressionBombError if relevant),
and OSError (file I/O) and log the same message with the caught exception;
ensure you add the necessary imports (requests.exceptions and PIL exception
classes) and keep the logger.warning call and stable_key reference intact so
only request/image/file errors are masked, not unrelated bugs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 654a506f-2495-4165-91f6-89703f214d15
📒 Files selected for processing (2)
plugins/masters-tournament/logo_loader.pyplugins/masters-tournament/masters_helpers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/masters-tournament/masters_helpers.py
Phase transition: - _computed_fallback_meta used _masters_thursday() (noon UTC / 08:00 EDT) as its start time; adding +3 days +23:59:59 landed on Monday morning UTC rather than Sunday 23:59 EDT, making end_e.date() one day too late and extending post-tournament by ~8 extra hours on the fallback path. Fix: normalise start to midnight EDT (04:00 UTC) before computing end. - get_detailed_phase post-tournament check used exact timedelta from the padded end timestamp rather than calendar days in EDT, making N-day semantics unintuitive. Fix: use (date.date() - end_e.date()).days <= N. Headshots in Vegas scroll: - get_player_headshot returned None immediately when player_id was empty, discarding a valid headshot_url. Post-tournament ESPN sometimes omits the athlete id field. Fix: proceed to URL download when url is provided, using the url itself as the in-memory cache key; only skip disk caching when there is no stable player_id filename. - Download failures were logged at DEBUG (invisible). Raised to WARNING. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace non-ASCII multiplication sign in masters_helpers.py comment to satisfy Ruff RUF003 - Normalize URL-based headshot cache keys using sha256 prefix instead of raw URL to avoid high-cardinality keys and query param leakage in logs; disk filename logic for player_id unchanged Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ChuckBuilds
force-pushed
the
feat/masters-post-tournament-improvements
branch
from
678e54b to
a349969
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/masters-tournament/logo_loader.py (1)
178-180: Persist URL-only headshots to disk usingstable_keyas filename.Right now, URL-only calls (empty
player_id) skip disk cache entirely. Inplugins/masters-tournament/masters_renderer.py(Lines 630-645), this call pattern is expected, so you lose persistent cache benefits and become more network-dependent across restarts.💾 Suggested adjustment
- # Check disk cache (only when we have a stable player_id filename) - player_path = self.players_dir / f"{player_id}.png" if player_id else None - if player_path and player_path.exists(): + # Check disk cache for both player_id and URL-only paths. + # stable_key is player_id when present, otherwise a URL hash. + player_path = self.players_dir / f"{stable_key}.png" + if player_path.exists(): try: img = Image.open(player_path).convert("RGBA") img = self._crop_to_fill(img, max_size) self._cache[cache_key] = img return img except Exception as e: logger.warning(f"Failed to load cached headshot {player_id}: {e}") ... - if player_path: - img.save(player_path, "PNG") + img.save(player_path, "PNG")Also applies to: 198-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/masters-tournament/logo_loader.py` around lines 178 - 180, The code skips disk caching when player_id is empty; change the logic that builds player_path in the logo loader so it uses stable_key as the filename when player_id is missing (i.e., set player_path = self.players_dir / f"{player_id or stable_key}.png" when stable_key is available), then continue to check player_path.exists() and read/write the cache as normal; update any related branches (the similar block around the other occurrence) to mirror this behavior and ensure stable_key is validated before using it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/masters-tournament/logo_loader.py`:
- Around line 178-180: The code skips disk caching when player_id is empty;
change the logic that builds player_path in the logo loader so it uses
stable_key as the filename when player_id is missing (i.e., set player_path =
self.players_dir / f"{player_id or stable_key}.png" when stable_key is
available), then continue to check player_path.exists() and read/write the cache
as normal; update any related branches (the similar block around the other
occurrence) to mirror this behavior and ensure stable_key is validated before
using it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc58f4f5-8089-4adc-9c70-a1bccb73e712
📒 Files selected for processing (4)
plugins/masters-tournament/logo_loader.pyplugins/masters-tournament/manifest.jsonplugins/masters-tournament/masters_data.pyplugins/masters-tournament/masters_helpers.py
✅ Files skipped from review due to trivial changes (1)
- plugins/masters-tournament/manifest.json
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/masters-tournament/masters_helpers.py
- plugins/masters-tournament/masters_data.py
This was referenced Apr 16, 2026
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.
Summary
end_datetimestamp, causing it to linger inpost-tournamentlonger than the configuredpost_tournament_display_days. Switched to calendar-day comparison in EDT so that "2 days after Sunday" reliably means Tuesday regardless of sub-day timestamp offsets._computed_fallback_meta()was building the tournament end window from the raw_masters_thursday()noon UTC timestamp, which shiftedend_e.date()one day forward (to Monday instead of Sunday). Now normalizes the start to midnight EDT before computing the end.get_player_headshot()returnedNoneimmediately whenplayer_idwas empty, discarding a validurl. The fix usesurlas a cache/disk key fallback and proceeds to download if a URL is available.Test plan
post-tournament→ countdown after the configured number of days (calendar days, not exact hours)masters_live_actionmode)post_tournament_display_days = 0skips post-tournament phase entirely (goes straight to countdown)post_tournament_display_days = 2holds results for Monday + Tuesday after Sunday final🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements