feat: include refractor card image in tier-up notification embed (#144) #159

Claude · 2026-04-08T14:39:02Z

Claude commented

2026-04-08 14:39:02 +00:00

Closes #144

Summary

The tier-up notification embed previously sent text-only. This PR adds the newly rendered refractor card image to the embed so players see card art at the peak moment of the progression loop.

Changes

helpers/refractor_notifs.py

build_tier_up_embed() accepts optional image_url: str | None = None; calls embed.set_image(url=image_url) when provided
notify_tier_completion() accepts optional image_url and passes it through to the embed builder

command_logic/logic_gameplay.py

_trigger_variant_renders() now captures the render endpoint response and returns dict[player_id, image_url] instead of None
_run_post_game_refractor_hook() ordering changed: renders run first (to obtain image URLs from the S3 upload response), then notifications fire with image_url= kwarg — card art appears in the embed immediately

tests/test_post_game_refractor_hook.py

Updated mocks to return {} from _trigger_variant_renders (was returning AsyncMock)
test_channel_passed_to_notify updated: assertion now includes image_url=None
test_notifications_before_render_trigger → test_render_before_notification: ordering flipped to ["render", "notify"], fake_notify signature updated, docstring updated

Notes

If the render endpoint returns None or a response without image_url (e.g. Playwright timeout), the notification sends without an image — graceful degradation. Team name/logo was not added as the tier_up dict from evaluate-game does not currently include team data.

All 17 tests pass.

Closes #144 ## Summary The tier-up notification embed previously sent text-only. This PR adds the newly rendered refractor card image to the embed so players see card art at the peak moment of the progression loop. ### Changes **`helpers/refractor_notifs.py`** - `build_tier_up_embed()` accepts optional `image_url: str | None = None`; calls `embed.set_image(url=image_url)` when provided - `notify_tier_completion()` accepts optional `image_url` and passes it through to the embed builder **`command_logic/logic_gameplay.py`** - `_trigger_variant_renders()` now captures the render endpoint response and returns `dict[player_id, image_url]` instead of `None` - `_run_post_game_refractor_hook()` ordering changed: renders run first (to obtain image URLs from the S3 upload response), then notifications fire with `image_url=` kwarg — card art appears in the embed immediately **`tests/test_post_game_refractor_hook.py`** - Updated mocks to return `{}` from `_trigger_variant_renders` (was returning `AsyncMock`) - `test_channel_passed_to_notify` updated: assertion now includes `image_url=None` - `test_notifications_before_render_trigger` → `test_render_before_notification`: ordering flipped to `["render", "notify"]`, `fake_notify` signature updated, docstring updated ## Notes If the render endpoint returns `None` or a response without `image_url` (e.g. Playwright timeout), the notification sends without an image — graceful degradation. Team name/logo was not added as the `tier_up` dict from `evaluate-game` does not currently include team data. All 17 tests pass.

Claude added 1 commit 2026-04-08 14:39:03 +00:00

feat: include refractor card image in tier-up notification embed (#144 )

Ruff Lint / lint (pull_request) Successful in 21s

Details

25b63b407f

Closes #144

- build_tier_up_embed() accepts optional image_url and calls set_image() when provided
- notify_tier_completion() accepts optional image_url and passes it through
- _trigger_variant_renders() now captures the render response image_url per player and returns a player_id->image_url dict
- _run_post_game_refractor_hook() triggers renders first (to obtain image URLs), then sends notifications with card art included
- Updated test_post_game_refractor_hook.py to reflect new render-before-notify ordering and image_url kwarg in notify calls

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

Claude referenced this pull request

2026-04-08 14:39:12 +00:00

feat: tier-up notification should include refractor card image #144

Claude added the

ai-reviewing

label 2026-04-08 14:45:07 +00:00

Claude reviewed 2026-04-08 14:48:12 +00:00

Claude left a comment

AI Code Review

Files Reviewed

helpers/refractor_notifs.py (modified)
command_logic/logic_gameplay.py (modified)
tests/test_post_game_refractor_hook.py (modified)

Findings

Correctness

No issues found.

build_tier_up_embed adds image_url: str | None = None before set_footer — correct position in the embed build. if image_url: guard correctly skips set_image when URL is absent or falsy.
_trigger_variant_renders now returns image_urls: dict. The if result and isinstance(result, dict): guard safely handles None/non-dict responses (e.g. Playwright timeout). Players with failed renders are omitted from the map, so image_url_map.get(player_id) returns None → graceful degradation path works correctly.
_run_post_game_refractor_hook ordering: _trigger_variant_renders → then notify loop with image_url=img. The tier_up.get("player_id") safe lookup means a missing player_id key returns None → image_url_map.get(None) → None → no image, no crash.
All tests that call _trigger_variant_renders through _run_post_game_refractor_hook now have return_value={} on the mock. Tests where the guard short-circuits (tier_ups=[] or absent key) correctly omit the return_value since the mock is never called.
fake_render in test_render_before_notification correctly returns {} — necessary because the production code now does image_url_map.get(...) on the return value.
fake_notify signature updated to (ch, tu, image_url=None) — required since production now calls with image_url=img kwarg; old two-arg signature would have raised TypeError.
test_channel_passed_to_notify assertion updated to mock_notify.assert_called_once_with(channel, tier_up, image_url=None) — correct given _trigger_variant_renders returns {} so image_url_map.get(1) → None.

Security

No issues found. image_url originates from the internal database API response (trusted source), not user input — no injection risk.

Style & Conventions

No issues found. Docstrings updated, naming follows existing patterns, return type annotation on _trigger_variant_renders changed to dict (accurate).

Suggestions

Coverage gap: No test verifies the happy path where _trigger_variant_renders returns a real URL that gets forwarded to notify_tier_completion. A test like mock_render.return_value = {101: "https://example.com/card.png"} then asserting mock_notify.assert_called_once_with(channel, tier_up, image_url="https://example.com/card.png") would close this. Not a blocker — the pass-through logic is simple — but would serve as a regression guard.

Verdict: APPROVED

Clean implementation. The ordering flip (renders before notifications) is well-motivated and correctly sequenced. Graceful degradation is handled at every failure point. Tests are updated consistently — no stale assertions. The coverage gap above is a suggestion only.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `helpers/refractor_notifs.py` (modified) - `command_logic/logic_gameplay.py` (modified) - `tests/test_post_game_refractor_hook.py` (modified) ### Findings #### Correctness No issues found. - `build_tier_up_embed` adds `image_url: str | None = None` before `set_footer` — correct position in the embed build. `if image_url:` guard correctly skips `set_image` when URL is absent or falsy. - `_trigger_variant_renders` now returns `image_urls: dict`. The `if result and isinstance(result, dict):` guard safely handles `None`/non-dict responses (e.g. Playwright timeout). Players with failed renders are omitted from the map, so `image_url_map.get(player_id)` returns `None` → graceful degradation path works correctly. - `_run_post_game_refractor_hook` ordering: `_trigger_variant_renders` → then notify loop with `image_url=img`. The `tier_up.get("player_id")` safe lookup means a missing `player_id` key returns `None` → `image_url_map.get(None)` → `None` → no image, no crash. - All tests that call `_trigger_variant_renders` through `_run_post_game_refractor_hook` now have `return_value={}` on the mock. Tests where the guard short-circuits (`tier_ups=[]` or absent key) correctly omit the `return_value` since the mock is never called. - `fake_render` in `test_render_before_notification` correctly returns `{}` — necessary because the production code now does `image_url_map.get(...)` on the return value. - `fake_notify` signature updated to `(ch, tu, image_url=None)` — required since production now calls with `image_url=img` kwarg; old two-arg signature would have raised `TypeError`. - `test_channel_passed_to_notify` assertion updated to `mock_notify.assert_called_once_with(channel, tier_up, image_url=None)` — correct given `_trigger_variant_renders` returns `{}` so `image_url_map.get(1)` → `None`. #### Security No issues found. `image_url` originates from the internal database API response (trusted source), not user input — no injection risk. #### Style & Conventions No issues found. Docstrings updated, naming follows existing patterns, return type annotation on `_trigger_variant_renders` changed to `dict` (accurate). #### Suggestions - **Coverage gap**: No test verifies the happy path where `_trigger_variant_renders` returns a real URL that gets forwarded to `notify_tier_completion`. A test like `mock_render.return_value = {101: "https://example.com/card.png"}` then asserting `mock_notify.assert_called_once_with(channel, tier_up, image_url="https://example.com/card.png")` would close this. Not a blocker — the pass-through logic is simple — but would serve as a regression guard. ### Verdict: APPROVED Clean implementation. The ordering flip (renders before notifications) is well-motivated and correctly sequenced. Graceful degradation is handled at every failure point. Tests are updated consistently — no stale assertions. The coverage gap above is a suggestion only. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-04-08 14:48:30 +00:00

cal approved these changes 2026-04-08 15:25:45 +00:00

Dismissed

cal left a comment

Approved via pd-pr

cal approved these changes 2026-04-08 15:25:49 +00:00

cal left a comment

Approved via pd-pr