feat(client): localize card art to the UI language - #7070
Conversation
Changing the UI language now swaps displayed card art to the same printing in that language. The printing the user chose is preserved -- only its image is exchanged for its localized sibling -- and cards with no sibling in that language keep their English art. The localized printing id comes from MTGJSON `foreignData[].identifiers .scryfallId`, which we already download for the text sidecars, so this needs no new upstream dependency. Scryfall image URLs are constructible from the id alone, and the English id is already embedded in the URLs we store, so the sidecar is a bare id->id map rather than a second copy of the image data. English users download nothing extra. The generator walks AllSetFiles one set at a time rather than parsing AllPrintings whole: a whole-file jq parse peaks at ~3.5 GB RSS against ~95 MB per-set, a 36x reduction. Localization is applied in `localFaceImageUrl`, the single funnel every stored-URL path reaches, and composes with `splitSizedImageUrl` so the card back and Scryfall's "image coming soon" placeholder come back byte-identical -- `isPlaceholderImageUrl` compares with `===`, so rewriting them would silently disable the printing-fallback chain. `imageRequestCache` keys on the art vocabulary a URL was produced with (`de` vs `de:pending`) rather than the bare language, so the locale map's arrival is a genuine key change; the printing-selection caches stay language-neutral because which printing wins is a function of art preferences, not language. Known limitation: deck-builder search results hold URLs from search time, so switching language mid-search leaves them English until the next search.
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change adds locale-specific Scryfall artwork maps for five languages. Build and deployment scripts generate and publish the maps. Client image resolution loads locale data and applies localized artwork with English and placeholder fallbacks. ChangesLocalized card-art pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UserInterface
participant useCardImage
participant loadLocaleArt
participant LocaleMap
participant ScryfallResolver
UserInterface->>useCardImage: Request card image
useCardImage->>loadLocaleArt: Load active language
loadLocaleArt->>LocaleMap: Fetch locale map
useCardImage->>ScryfallResolver: Resolve image URL
ScryfallResolver->>LocaleMap: Find localized printing ID
ScryfallResolver-->>useCardImage: Return localized or fallback URL
useCardImage-->>UserInterface: Render image
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 317-320: Move the Generate locale card-art maps step before the
MTGJSON cache save so AllSetFiles.tar and allsets/ are included, and roll the
mtgjson-full-* cache key to invalidate existing incomplete entries.
Alternatively, add a separate versioned cache covering those inputs while
preserving the existing generation flow.
In `@client/src/components/deck-builder/PrintingPickerModal.tsx`:
- Around line 135-140: Subscribe PrintingPickerModal to the locale-art readiness
signal used by useCardImage before calling resolvePrintingImageUrl, so map
completion triggers tile rerender and preserves the resolver’s locale-aware
behavior. Update or add colocated frontend coverage that changes language while
the locale map is pending and verifies tiles refresh after readiness completes.
In `@client/src/hooks/useCardImage.ts`:
- Around line 413-427: Move loadLocaleArtInBackground(language) out of render
and into a [language]-dependent effect that runs after the language subscription
is registered. Keep the existing artLocaleKey calculation and ensure loading
uses the committed preference language, preventing discarded renders from
updating desiredArtLang.
In `@client/src/services/__tests__/scryfall.test.ts`:
- Around line 1338-1469: Add a request-deduplication test in the localized card
art suite that keeps the mocked fetch pending, calls loadLocaleArt("de") twice
before resolving it, and verifies both calls share one fetch request. Resolve
the response, await both promises, and assert the resulting localization map is
installed and used by resolvePrintingImageUrl.
In `@client/vite.config.ts`:
- Around line 323-338: The locale-map RegExpRoute in the Workbox runtimeCaching
configuration must match both cross-origin R2 URLs and same-origin paths. Update
the urlPattern for the card-art locale-map rule to add the anchored
DATA_BASE_URL branch used by the engine WASM rule while retaining the existing
/scryfall-images.<lng>.json$ fallback.
In `@scripts/deploy-cf.sh`:
- Around line 49-53: Serialize updates to DEPLOY_CACHE in the surrounding
cache-update block: do not let the background upload workers concurrently write
or move DEPLOY_CACHE.tmp. Collect successful tags from the workers and merge
them after all uploads complete, or use one shared lock around each cache update
while preserving existing deployment behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b77c22e5-e182-4be6-9863-e673e368bfc8
⛔ Files ignored due to path filters (1)
client/src/vite-env.d.tsis excluded by!**/*.d.ts
📒 Files selected for processing (14)
.github/workflows/deploy.yml.github/workflows/release.yml.gitignoreclient/src/components/deck-builder/PrintingPickerModal.tsxclient/src/hooks/__tests__/useCardImage.test.tsxclient/src/hooks/useCardImage.tsclient/src/services/__tests__/scryfall.test.tsclient/src/services/scryfall.tsclient/vite.config.tsclient/vitest.config.tsdata-files.jsonscripts/deploy-cf.shscripts/gen-scryfall-locale-images.shscripts/setup.sh
| # Separate from the Scryfall step: this reads MTGJSON set files, not the | ||
| # Scryfall bulk exports, so it is not covered by the data/scryfall cache. | ||
| - name: Generate locale card-art maps | ||
| run: ./scripts/gen-scryfall-locale-images.sh |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Cache the AllSetFiles input after locale-map generation.
The mtgjson-full-* cache is saved before this step. It cannot contain AllSetFiles.tar or allsets/. Because GitHub Actions caches are immutable, later cache hits cannot add them. Each release then downloads and extracts the full MTGJSON archive again.
Generate the maps before saving the MTGJSON cache and roll the cache key, or add a separate versioned cache for the AllSetFiles inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 317 - 320, Move the Generate
locale card-art maps step before the MTGJSON cache save so AllSetFiles.tar and
allsets/ are included, and roll the mtgjson-full-* cache key to invalidate
existing incomplete entries. Alternatively, add a separate versioned cache
covering those inputs while preserving the existing generation flow.
Six findings from the PR review, all confirmed against the code first: - Workbox never routed the locale maps in production. Workbox's RegExpRoute refuses a cross-origin match that does not begin at index 0 of the href, and these are served from R2 — so the bare suffix pattern silently skipped the very requests the rule exists for. Now two anchored branches, mirroring the engine-WASM rule. - CI re-downloaded the ~169 MB AllSetFiles archive every run: the MTGJSON cache is saved before the generator runs and GitHub caches are immutable. Adds a restore/save pair for the five generated maps, mirroring the draft-pools idiom — caching the small output rather than adding ~1 GB of extracted set files to a shared entry. The generator already no-ops when the maps exist, so a hit skips the download. - deploy-cf.sh ran every upload in its own background subshell, each doing a read-modify-write through one shared "$DEPLOY_CACHE.tmp": two workers interleaving lose entries. Workers now record a private tag file and a single writer merges after the wait loop. Pre-existing, but five more entries made it materially more likely. - The locale-art load ran during render, where it wrote module-global state that decides whose fetch may install. A discarded concurrent render could let an uncommitted language win that race; it now runs in a [language] effect placed after the invalidation subscription. - PrintingPickerModal resolved tile URLs during render with no subscription, so opening it while the map was in flight left English art with no re-render on arrival. Adds `useLocaleArt`, which owns the load and the invalidation tick. useCardImage deliberately does not use it — it has an oracleId-filtered subscription, and an unfiltered second one per tile would resurrect the re-render storm that filter prevents. - Adds the missing request-deduplication test, and a picker test that drives a mounted modal through pending -> resolved.
deploy.yml runs the locale-map generator in `card-data`, but `mtgjson-key` is declared in `preview-inputs`. A `steps.` reference cannot cross a job boundary and does not fail when it dangles — it expands to the empty string, so the cache key collapsed to a constant and the maps would never have invalidated when MTGJSON published new data. release.yml was already correct: there the generator and the key step share the `build-wasm` job.
|
All six review findings verified against the code and fixed in
Two notes beyond the findings:
Verification: full frontend suite 2641 passed / 294 files, type-check and lint clean. The picker test was mutation-checked — removing |
1 participant
Changing the UI language now swaps displayed card art to the same
printing in that language. The printing the user chose is preserved --
only its image is exchanged for its localized sibling -- and cards with
no sibling in that language keep their English art.
The localized printing id comes from MTGJSON
foreignData[].identifiers .scryfallId, which we already download for the text sidecars, so thisneeds no new upstream dependency. Scryfall image URLs are constructible
from the id alone, and the English id is already embedded in the URLs we
store, so the sidecar is a bare id->id map rather than a second copy of
the image data. English users download nothing extra.
The generator walks AllSetFiles one set at a time rather than parsing
AllPrintings whole: a whole-file jq parse peaks at ~3.5 GB RSS against
~95 MB per-set, a 36x reduction.
Localization is applied in
localFaceImageUrl, the single funnel everystored-URL path reaches, and composes with
splitSizedImageUrlso thecard back and Scryfall's "image coming soon" placeholder come back
byte-identical --
isPlaceholderImageUrlcompares with===, sorewriting them would silently disable the printing-fallback chain.
imageRequestCachekeys on the art vocabulary a URL was produced with(
devsde:pending) rather than the bare language, so the localemap's arrival is a genuine key change; the printing-selection caches
stay language-neutral because which printing wins is a function of art
preferences, not language.
Known limitation: deck-builder search results hold URLs from search
time, so switching language mid-search leaves them English until the
next search.
Summary by CodeRabbit
New Features
Bug Fixes