fix: use card_type field instead of track_name string in variant renders (#149) #153

Claude · 2026-04-08T03:32:50Z

Claude commented

2026-04-08 03:32:50 +00:00

Closes #149

What changed

Replaced fragile track_name string matching in _trigger_variant_renders() with the structured card_type field already present in refractor API data.

Before:

track = tier_up.get("track_name", "Batter")
card_type = "pitching" if track.lower() == "pitcher" else "batting"

After:

ct = tier_up.get("card_type", "batter")
card_type = "pitching" if ct in ("sp", "rp") else "batting"

The old code would silently render batting cards for pitchers if the API ever returned "Starting Pitcher", "SP", or any string other than exactly "Pitcher". The card_type field (sp/rp/batter) is the authoritative structured value from the refractor API.

Files changed

command_logic/logic_gameplay.py — 2-line fix in _trigger_variant_renders()
tests/test_post_game_render.py — updated fixtures to use card_type field; strengthened assertions to verify correct card type in URL

Tests

All 4 tests pass. Assertions now verify that battingcard/pitchingcard appears in the render URL, which would have caught the original bug.

Closes #149 ## What changed Replaced fragile `track_name` string matching in `_trigger_variant_renders()` with the structured `card_type` field already present in refractor API data. **Before:** ```python track = tier_up.get("track_name", "Batter") card_type = "pitching" if track.lower() == "pitcher" else "batting" ``` **After:** ```python ct = tier_up.get("card_type", "batter") card_type = "pitching" if ct in ("sp", "rp") else "batting" ``` The old code would silently render batting cards for pitchers if the API ever returned "Starting Pitcher", "SP", or any string other than exactly "Pitcher". The `card_type` field (`sp`/`rp`/`batter`) is the authoritative structured value from the refractor API. ## Files changed - `command_logic/logic_gameplay.py` — 2-line fix in `_trigger_variant_renders()` - `tests/test_post_game_render.py` — updated fixtures to use `card_type` field; strengthened assertions to verify correct card type in URL ## Tests All 4 tests pass. Assertions now verify that `battingcard`/`pitchingcard` appears in the render URL, which would have caught the original bug.

Claude added 1 commit 2026-04-08 03:32:50 +00:00

fix: use card_type field instead of track_name string in variant renders (#149 )

Ruff Lint / lint (pull_request) Successful in 14s

Details

a56004fbd4

Closes #149

Replaces fragile track_name string matching ('Pitcher' check) with the
structured card_type field ('sp', 'rp', 'batter') already present in
refractor API data. Prevents silent wrong-type renders when track_name
varies ('Starting Pitcher', 'SP', etc.). Updates test fixtures and adds
assertions that verify the correct card type URL segment.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude referenced this pull request

2026-04-08 03:33:01 +00:00

fix: variant render uses fragile track_name string mapping #149

Claude added the

ai-reviewing

label 2026-04-08 03:45:42 +00:00

Claude reviewed 2026-04-08 03:48:50 +00:00

Claude left a comment

AI Code Review

Files Reviewed

command_logic/logic_gameplay.py (modified — 2-line fix in _trigger_variant_renders)
tests/test_post_game_render.py (modified — fixture fields and assertions)

Findings

Correctness

Concern — unverified API field path (potential silent regression):

The fix assumes card_type is a top-level field in the refractor/evaluate-game tier_up objects (tier_up.get("card_type", "batter")). Two pieces of evidence suggest this may not be the case:

tests/test_complete_game_hook.py:190–199 — the comment there explicitly says "Full API response shape — the evaluate-game endpoint returns all these keys" and the fixture contains: player_id, team_id, player_name, old_tier, new_tier, current_value, track_name. No card_type.
cogs/players.py:513 — the analogous card_type lookup from the refractor/cards endpoint accesses it nested inside a track dict:
```
track_type = match.get("track", {}).get("card_type", "batter")
```
If evaluate-game has the same nesting, tier_up.get("card_type", "batter") always returns "batter" regardless of pitcher status.

Risk: If card_type is not a top-level field in evaluate-game responses, this change is a silent regression — the original code at least correctly handled track_name == "Pitcher" (the common case). The new code would silently render all pitchers as batting cards, with no error or warning.

The fix is logically correct if the API does return card_type at the top level. But that contract isn't documented or validated in this codebase.

Recommendation: Verify against the actual refractor/evaluate-game response shape. If the field exists at the top level, update test_complete_game_hook.py:190–199 to include card_type in the documented shape. If it's nested (like refractor/cards), use: tier_up.get("track", {}).get("card_type", "batter").

Security

No issues found.

Style & Conventions

The in ("sp", "rp") pattern mirrors cogs/players.py:514 exactly. Consistent with existing conventions.
ct as a variable name is acceptable but card_type_raw or ct in context is fine — not a blocker.

Suggestions

If the API field is confirmed, add a "card_type" key to the test_complete_game_hook.py:191 fixture and update the "Full API response shape" comment. That fixture is now stale documentation of the tier_up shape.

Verdict: COMMENT

The logic is correct in principle — card_type as a structured field is strictly better than string-matching track_name. The in ("sp", "rp") pattern is consistent with the rest of the codebase. However, the API contract for the card_type field's location in evaluate-game tier_up objects is not verified within this codebase. Confirm the field is top-level (not nested in track) before merging. If it is, this is a clean APPROVE.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `command_logic/logic_gameplay.py` (modified — 2-line fix in `_trigger_variant_renders`) - `tests/test_post_game_render.py` (modified — fixture fields and assertions) ### Findings #### Correctness **Concern — unverified API field path (potential silent regression):** The fix assumes `card_type` is a **top-level** field in the `refractor/evaluate-game` tier_up objects (`tier_up.get("card_type", "batter")`). Two pieces of evidence suggest this may not be the case: 1. **`tests/test_complete_game_hook.py:190–199`** — the comment there explicitly says *"Full API response shape — the evaluate-game endpoint returns all these keys"* and the fixture contains: `player_id`, `team_id`, `player_name`, `old_tier`, `new_tier`, `current_value`, `track_name`. No `card_type`. 2. **`cogs/players.py:513`** — the analogous `card_type` lookup from the `refractor/cards` endpoint accesses it **nested** inside a `track` dict: ```python track_type = match.get("track", {}).get("card_type", "batter") ``` If `evaluate-game` has the same nesting, `tier_up.get("card_type", "batter")` always returns `"batter"` regardless of pitcher status. **Risk:** If `card_type` is not a top-level field in `evaluate-game` responses, this change is a silent regression — the original code at least correctly handled `track_name == "Pitcher"` (the common case). The new code would silently render all pitchers as batting cards, with no error or warning. The fix is logically correct if the API does return `card_type` at the top level. But that contract isn't documented or validated in this codebase. **Recommendation:** Verify against the actual `refractor/evaluate-game` response shape. If the field exists at the top level, update `test_complete_game_hook.py:190–199` to include `card_type` in the documented shape. If it's nested (like `refractor/cards`), use: `tier_up.get("track", {}).get("card_type", "batter")`. #### Security No issues found. #### Style & Conventions - The `in ("sp", "rp")` pattern mirrors `cogs/players.py:514` exactly. Consistent with existing conventions. - `ct` as a variable name is acceptable but `card_type_raw` or `ct` in context is fine — not a blocker. #### Suggestions - If the API field is confirmed, add a `"card_type"` key to the `test_complete_game_hook.py:191` fixture and update the "Full API response shape" comment. That fixture is now stale documentation of the tier_up shape. ### Verdict: COMMENT The logic is correct in principle — `card_type` as a structured field is strictly better than string-matching `track_name`. The `in ("sp", "rp")` pattern is consistent with the rest of the codebase. However, the API contract for the `card_type` field's location in `evaluate-game` tier_up objects is not verified within this codebase. Confirm the field is top-level (not nested in `track`) before merging. If it is, this is a clean APPROVE. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-04-08 03:49:11 +00:00

Claude removed the

ai-reviewed

label 2026-04-11 19:23:49 +00:00

cal force-pushed issue/149-fix-variant-render-uses-fragile-track-name-string from a56004fbd4 to 9eb9669151

2026-04-12 14:38:47 +00:00

Compare

cal merged commit 71f47eb412 into main