fix(client): render a name/text tile for tokens with no official art (#6156) - #6214
Conversation
…hase-rs#6156) Tokens with no official paper printing — e.g. the Banana tokens Kibo, Uktabi Prince creates — resolve to a null token-image src. `CardImage` folded that `!src` case into the loading branch, so the tile animated a featureless dark `bg-gray-700` pulse forever and read as a black/blank square with no way to tell what the permanent was. Split "still resolving art" (`isLoading`) from "resolution finished with no image" (`!src`), and route the latter — together with the existing `imageError` (broken `<img>`) case — through a shared `CardArtFallback` tile that renders the card/token name (and Oracle text when the engine knows it). This covers every artless card or token, not a single hard-coded name, and face-down cards and normal art paths are untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a CardArtFallback component in CardImage.tsx to handle cases where a card or token has no renderable art (either because the art resolution returned no source, or the image failed to load). It also adds a comprehensive test suite in CardImage.test.tsx to verify these fallback behaviors, including loading states, text tile rendering, and Oracle text inclusion. No review comments were provided, and the changes are clean and well-tested, so there is no additional feedback.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Deferred by maintainer intake policy — not ignored. This current head ( A maintainer must explicitly take this PR or add a local frontend-review exception before it can receive substantive review. The defer label is a routing marker only, not a verdict on the change. |
|
Understood on the State of this one:
Happy to split it smaller, fold it into a larger frontend batch, or drop it if the artless-token fallback isn't a direction you want. If a frontend-review exception isn't something you want to hand out, that's completely fine — I'd just appreciate knowing so I can aim future work at engine-side issues instead. |
… fallback The initial fix split "still resolving art" from "resolution finished with no art" in CardImage only. ArtCropCard carried the identical collapsed condition and is the *default* battlefield renderer (preferencesStore defaults battlefieldCardDisplay to "art_crop"), so issue phase-rs#6156 still reproduced out of the box: a Banana token resolved src: null with isLoading: false and returned a featureless animate-pulse tile forever. - Extract CardArtFallback into its own module as the single authority for every artless render, with an artCrop/fullCard variant mirroring the vocabulary SummoningSicknessOverlay already uses. artCrop tiles show the name alone; Oracle text is unreadable at that scale. - Apply the isLoading / !src split in ArtCropCard and route !src to the tile. - CardImagePreview had the same collapse, which left its own already-written named placeholder unreachable for artless tokens; !src now falls through to it. - Reset imageError when src changes, mirroring CardPreview's existing effect — a latched error otherwise pinned the text tile across a face-up flip or a DFC transform for the surviving component instance. Tests: ArtCropCard gains artless-token and still-pulsing cases (the former fails against the pre-fix condition); CardImage gains a face-down case pinning the one path where src === null must not produce a name tile, a src-change retry case, and a positive reach-guard on the loading assertion that previously asserted only negatives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Deferred by maintainer intake policy — not ignored. This refresh applies to the current head ( |
The previous commit early-returned a bare fallback tile before ArtCropCard's frame, so an artless permanent lost its P/T box, counters, damage, loyalty badge and DFC button. That path is not rare: useCardImage leaves src null with isLoading false on ANY rejected fetch, so a CDN outage would strip game state from every permanent on the board — trading the reported information loss for a worse one. Swap only the art instead. The frame (whose header already renders the name) stays, and CardArtFallback fills the art slot, so an artless permanent loses its picture but never its game state. src is non-null for face-down cards (CARD_BACK_URL), so those still render the card back. Also narrow CardArtFallback's doc comment: it claimed to be the single authority for every artless render while CardPreview kept its own placeholder, which is the drift the extraction was supposed to prevent. The preview placeholder is deliberate (it centres one name at a much larger scale), so the comment now says so and the placeholder gains the matching role/aria-label it was missing. The regression test now asserts the P/T box and counters survive alongside the tile; it fails against the early-return version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… tile
Third review pass found the fallback still had two gaps at the same seam the
previous commit fixed elsewhere:
- ArtCropCard's <img> had no onError, so a URL that resolves but 404s (a
future-dated set not yet on the CDN, a stale token image ref) still painted
the browser's broken-image glyph in the DEFAULT battlefield renderer, while
CardImage recovered from the identical case. CardArtFallback's doc claimed
ArtCropCard routed "broken art" here; now it actually does.
- CardImage still early-returned the fallback before its wrapper <div>, so an
artless card lost the amber unimplemented-mechanics badge. Swapped in place of
the <img> instead, matching what ArtCropCard now does.
Tests: art-404 recovery in art_crop (asserting the frame survives too), badge
retention on CardImage's tile, and the counter assertion the previous test's own
comment claimed but never made.
Known and deliberately not fixed here: useCardImage reports {src: null,
isLoading: false} for a hook parked on "", so un-parking one (a morph turned
face up, a Ctrl-peeked DFC) can paint a single frame of the fallback before the
pulse. The cause is a hook-contract gap, not a rendering bug — the fix belongs
in useCardImage's return shape and would touch every consumer, so it should not
ride along in a display-layer PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… shape Fourth review pass found phase-rs#6156 still reproduced verbatim on phones. Both arms of MobilePreviewOverlay gated the art on `{src && …}`, so tapping an artless token opened a full-screen overlay containing nothing at all — the reported blank square, on the one path none of the previous commits touched. Neither arm had an onError either, so a resolved-but-404 URL painted the broken-image glyph. - Route both mobile arms through CardArtFallback on `!src || artError`, with onError and a src-change reset, matching the board renderers. - Give the desktop placeholder `aspect-[5/7]`. `frameClass` is width-only when upright (the <img> normally supplies the height), so routing the `!src` class into it made every artless hover render a squat strip under a card-sized border. The loading branch already compensates this way. - Test the ArtCropCard artError reset, which was previously unasserted — deleting the effect left all six cases green. ArtCropCard is memo()'d on objectId, so the test pushes a fresh object through the store rather than re-rendering with identical props, which is what a real transform does. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…odal art Regression, self-inflicted in b9394a9: MobilePreviewOverlay dropped isLoading from the destructure and derived the fallback from `!src` alone. useCardImage assigns src in a post-render effect, so src is null on EVERY first paint — every mobile card tap flashed the "no art" tile before its art, on cards with perfect art. This is the exact conflation the board renderers were fixed for, and which ArtCropCard's own test asserts must not happen. Both mobile arms now gate on a settled lookup and render a skeleton (modal) or nothing (the non-blocking compact peek) while resolving. Two further art paths carried the original defect and are now covered: - StackEntry: explicitly token-aware (sourceIsToken / sourceTokenImageRef), so an artless token's triggered or activated ability rendered a permanently pulsing square — worse than a blank one, since it implies art is still coming. Now splits isLoading from settled-and-artless, plus onError. - CardTextboxPreview: returned null, erasing the card-identification band from ChoiceModal, PermanentTypeSlotModal and AlternativeCostModal for exactly the cards whose identity is hardest to infer. Now renders a named strip. Test added for the loading-flash invariant, since it is the failure mode most likely to be reintroduced. Known remaining, deliberately out of scope: ~26 further art sites (zone piles, reveal overlays, deck builder, draft) lack onError, and several render an anonymous blank box rather than a name. CardArtFallback now exists to make each a one-line swap, but sweeping them is a separate change, not a phase-rs#6156 fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The text-box preview never enters its new 404 fallback. Evidence: client/src/components/modal/CardTextboxPreview.tsx:36-75 creates artError and branches on it, but the rendered <img> has no onError handler to call setArtError(true). Why it matters: a failed image request still leaves a blank/broken preview in the choice modals this PR explicitly intends to keep identifiable. Suggested fix: attach onError={() => setArtError(true)} and add a component test that fires that error and asserts the named fallback.
…reachable Addresses matthewevans' review on phase-rs#6214. The previous commit added `artError` state and a fallback branch to CardTextboxPreview but never attached an onError handler, so nothing could ever set it — the branch was unreachable and a failed image request still left a broken preview in the choice modals this PR exists to keep identifiable. Attach onError, and add the component test the review asked for: it fires the error and asserts the named fallback replaces the <img>. Verified discriminating — removing the handler fails that case while the other two still pass. The new test file also pins the two states that had no coverage at all: absent while in flight, named strip once settled with no art. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — you were exactly right, and it was my bug. I added the Fixed in
Verified locally on that head: 2078 tests pass, Two notes on the rest of the PR while you have it open: Known remaining, deliberately out of scope. A review sweep enumerated ~31 card-art render sites in Unrelated CI red. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved after the current head fixed the missing image-error transition in CardTextboxPreview and added a component-level regression test that fires the image error and verifies the named fallback.
Closes #6156
Summary
Banana tokens created by Kibo, Uktabi Prince — and any other true token with no official paper printing — render as a featureless dark square with no way to tell what the permanent is. This implements the issue's first-listed resolution ("a generic artifact-token fallback frame/text tile"): a deliberate name (+ Oracle text) tile, so the permanent stays identifiable.
Root cause
useCardImagereturnssrc: nullfor a token whose art can't be resolved (notoken_image_refinscryfall-token-images.jsonand no Scryfall name hit — true for Banana, which Wizards never printed on paper).CardImagefolded that!srccase into its loading branch:So a token that will never resolve art animated the loading pulse forever — a blank/black square. The component already had a rich name + Oracle-text tile, but only for the separate
imageError(broken<img>) case.Fix
isLoading) from "resolution finished with no image" (!src). Only the former pulses.!srcandimageErrorthrough one sharedCardArtFallbacktile (extracted; previously the error tile was inline). It renders the card/token name and, when the engine knows it, the Oracle text.Built for the class: any artless card or token is covered, not a hard-coded name. Face-down cards (
CARD_BACK_URL) and the normal loaded-art path are untouched.Anchored on (Gate B — pattern anchoring)
client/src/components/card/CardImage.tsx— the pre-existingimageErrorfallback tile (name +RichLabelOracle text) is the exact visualCardArtFallbacknow generalizes; this change widens its trigger from "broken<img>" to "broken<img>OR no resolvable art".client/src/components/card/__tests__/CardPreview.test.tsx— the sibling component test'svi.mock("../../../hooks/useCardImage.ts", …)+ React Testing Libraryrender/screenpattern is the harness reused verbatim in the newCardImage.test.tsx.Scope / policy note
The issue flags that the broader asset-sourcing policy for no-paper tokens (curated local art, generated art, an engine token-image-ref strategy) should be discussed. This PR deliberately stays policy-neutral: it only replaces the blank square with an identifiable text tile using data the client already has, and leaves any art-sourcing decision as separate follow-up.
Test plan
New
client/src/components/card/__tests__/CardImage.test.tsx(RTL + Vitest, mockinguseCardImage):pnpm exec vitest run src/components/card/__tests__/CardImage.test.tsxCases: (1)
isLoading→ pulse, no text tile; (2) resolved withsrc: null→ name tile, and no<img>element is emitted (nothing can render as a black square); (3) Oracle text appears in the tile when known; (4) a resolved<img>that fails to load swaps in the same tile.AI-contributor disclosure
Authored with an LLM (Claude). Pure frontend/display change — no engine, parser, or MTG-rules logic touched, so no CR annotations apply. Combinator-purity Gate A passes trivially (no parser diff). Local
pnpm/Vitest could not be run in the authoring sandbox (network-restricted dependency install); CI runs the frontend suite. Please flag anything you'd like reworked.🤖 Generated with Claude Code