Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,11 @@ jobs:
./scripts/gen-scryfall-sets.sh
./scripts/gen-scryfall-printings.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

- name: Save Scryfall bulk data
# data/scryfall is complete once the four generators have run. Banking
# it here keeps a later failure from re-hitting the Scryfall API on the
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,11 @@ jobs:
./scripts/gen-scryfall-sets.sh
./scripts/gen-scryfall-printings.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
Comment on lines +339 to +348

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 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.


- name: Save Scryfall bulk data
# data/scryfall is complete once the four generators have run. Banking
# it here keeps a later failure from re-hitting the Scryfall API on the
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ client/public/card-names.json
client/public/coverage-data.json
client/public/coverage-summary.json
client/public/scryfall-data.json
client/public/scryfall-images.*.json
client/public/scryfall-printings.json
client/public/scryfall-sets.json
client/public/scryfall-token-images.json
Expand Down Expand Up @@ -71,6 +72,8 @@ data/mtgjson/SetList.json
data/mtgjson/.refresh-week
data/mtgjson/decks/
data/mtgjson/sets/
data/mtgjson/AllSetFiles.tar
data/mtgjson/allsets/
!data/abilities/
!data/precons/
!data/learned-weights.json
Expand Down
9 changes: 7 additions & 2 deletions client/src/components/deck-builder/PrintingPickerModal.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { useCallback, useEffect, useMemo, useState } from "react";
import { useTranslation } from "react-i18next";

import { getCardPrintings } from "../../services/scryfall.ts";
import { getCardPrintings, resolvePrintingImageUrl } from "../../services/scryfall.ts";
import type { PrintingEntry } from "../../services/scryfall.ts";
import { usePreferencesStore } from "../../stores/preferencesStore.ts";
import { ModalPanelShell } from "../ui/ModalPanelShell";
Expand Down Expand Up @@ -132,7 +132,12 @@ export function PrintingPickerModal({
<div className="grid gap-3 grid-cols-[repeat(auto-fill,minmax(140px,1fr))]">
{visiblePrintings.map((printing) => {
const isSelected = currentOverride?.scryfallId === printing.id;
const imgUrl = printing.faces[0]?.normal;
// Go through the shared resolver rather than reading the face URL
// directly: it applies the active locale's art, so the picker
// previews each printing in the same language the board renders.
// It also maps Scryfall's "image coming soon" placeholder to null,
// which this tile already renders as a proper "no image" cell.
const imgUrl = resolvePrintingImageUrl(printing, 0, "normal");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const isBorderless = printing.border_color === "borderless";
const isExtended = printing.frame_effects.includes("extendedart");

Expand Down
9 changes: 9 additions & 0 deletions client/src/hooks/__tests__/useCardImage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ describe("useCardImage", () => {
findPrintingById: vi.fn(),
getCardPrintings: vi.fn().mockResolvedValue([]),
isCardImageRotatedSync: vi.fn().mockReturnValue(false),
// Report the art locale as already resolved so the hook's background
// loader short-circuits — this test is about token fallback, not
// localization.
isLocaleArtReady: vi.fn().mockReturnValue(true),
loadLocaleArt: vi.fn().mockResolvedValue(new Map()),
resolveFaceIndexSync: vi.fn().mockReturnValue(null),
resolveOracleIdSync: vi.fn().mockReturnValue(null),
resolvePrintingImageUrl: vi.fn(),
Expand Down Expand Up @@ -166,6 +171,10 @@ describe("useCardImage", () => {
getCardPrintings: vi.fn().mockResolvedValue([]),
isCardImageFlipLayoutSync: vi.fn().mockReturnValue(false),
isCardImageRotatedSync: vi.fn().mockReturnValue(false),
// See the note on the token-fallback mock above: the art locale is
// reported ready so the background loader never runs here.
isLocaleArtReady: vi.fn().mockReturnValue(true),
loadLocaleArt: vi.fn().mockResolvedValue(new Map()),
pickOldestPrinting: vi.fn(),
resolveFaceIndexSync: vi.fn().mockReturnValue(null),
resolveOracleIdSync: vi.fn().mockReturnValue(null),
Expand Down
56 changes: 56 additions & 0 deletions client/src/hooks/useCardImage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
getCardPrintings,
isCardImageFlipLayoutSync,
isCardImageRotatedSync,
isLocaleArtReady,
loadLocaleArt,
pickOldestPrinting,
resolveFaceIndexSync,
resolveOracleIdSync,
Expand Down Expand Up @@ -106,6 +108,37 @@ registerStrategyCacheClearFn(() => {
strategyNoWinnerCache.clear();
});

/**
* Locales whose card-art map is currently being fetched. Same anti-spin-loop
* discipline as `strategyInflight`: without it, every render before the map
* lands would start another fetch.
*
* A failed fetch cannot loop either — `loadLocaleArt` swallows errors and
* resolves an empty map, which still installs, so `isLocaleArtReady` flips true
* and every card simply keeps its English art.
*/
const localeArtInflight = new Set<string>();

/**
* Fetch the active locale's card-art map, then invalidate every mounted tile.
*
* The dispatch deliberately carries no `detail`: unlike a printings fetch (which
* concerns one oracleId), a language change re-resolves the URL of every card on
* screen, and the listener treats a detail-less event as a global invalidation.
*/
function loadLocaleArtInBackground(lang: string): void {
if (isLocaleArtReady(lang) || localeArtInflight.has(lang)) return;
localeArtInflight.add(lang);
loadLocaleArt(lang)
.then(() => {
localeArtInflight.delete(lang);
artCacheEvents.dispatchEvent(new Event("update"));
})
.catch(() => {
localeArtInflight.delete(lang);
});
}

function applyChainEntry(
entry: ArtChainEntry,
printings: PrintingEntry[],
Expand Down Expand Up @@ -241,6 +274,12 @@ function imageRequestKey(
tokenImageRefKey: string,
oracleId: string,
faceName: string,
// `imageRequestCache` stores the FINAL resolved URL, which differs per
// language once localized art is applied — so the art locale belongs in the
// key. The printing-selection caches (`strategyCacheMap`, `printingsCacheMap`)
// stay language-neutral on purpose: which printing wins is a function of the
// user's art preferences, not of their language.
artLocaleKey: string,
): string {
return [
oracleId || cardName,
Expand All @@ -253,6 +292,7 @@ function imageRequestKey(
filterSubtypes,
String(filterHasAbilities),
tokenImageRefKey,
artLocaleKey,
].join("|");
}

Expand Down Expand Up @@ -370,6 +410,21 @@ export function useCardImage(

const artOverrides = usePreferencesStore((s) => s.artOverrides);
const artChain = usePreferencesStore((s) => s.artChain);
// Card art follows the UI language: the printing the user chose is kept, and
// only its image is swapped for the same printing in their language. Cards
// with no localized sibling keep their English art.
const language = usePreferencesStore((s) => s.language);
loadLocaleArtInBackground(language);
/**
* Cache-key component for the resolved image URL. It encodes the *art
* vocabulary* a URL was produced with, not merely the language: before the
* locale map arrives every card legitimately resolves to English art, and
* caching that under a bare `"de"` key would pin it there forever — the
* background load dispatches an invalidation, but `requestKey` would be
* unchanged, so the resolution effect would never re-run. Distinguishing
* pending from ready makes the map's arrival a genuine key change.
*/
const artLocaleKey = isLocaleArtReady(language) ? language : `${language}:pending`;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

const [src, setSrc] = useState<string | null>(null);
const [isRotated, setIsRotated] = useState(false);
Expand Down Expand Up @@ -456,6 +511,7 @@ export function useCardImage(
tokenImageRefKey,
oracleId,
faceName,
artLocaleKey,
);

useEffect(() => {
Expand Down
135 changes: 135 additions & 0 deletions client/src/services/__tests__/scryfall.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import path from "node:path";
import { fileURLToPath, pathToFileURL } from "node:url";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";

import type { PrintingEntry } from "../scryfall.ts";

const REPO_ROOT = path.resolve(
path.dirname(fileURLToPath(import.meta.url)),
"../../../..",
Expand Down Expand Up @@ -1332,3 +1334,136 @@ esac
},
);
});

describe("localized card art", () => {
// Real five-segment `cards.scryfall.io` shape. As with the size-derivation
// suite above, the `makeLocalDataMap` fixture is deliberately NOT reused: its
// one-segment `https://img.example/<Name>.jpg` URLs are not localizable, so
// every assertion here would pass vacuously against them.
const EN_ID = "0dbac7ce-a6fa-466e-b6ba-173cf2dec98e";
const DE_ID = "345a1cf0-e4de-42a9-9c72-ed16826b9067";
const UNMAPPED_ID = "11111111-2222-3333-4444-555555555555";

const cardUrl = (id: string, size = "normal", face = "front", query = "") =>
`https://cards.scryfall.io/${size}/${face}/${id[0]}/${id[1]}/${id}.jpg${query}`;

const printing = (id: string, query = ""): PrintingEntry => ({
id,
set: "mid",
set_name: "Innistrad: Midnight Hunt",
collector_number: "7",
released_at: "2021-09-24",
border_color: "black",
frame_effects: [],
full_art: false,
faces: [
{ normal: cardUrl(id, "normal", "front", query), art_crop: cardUrl(id, "art_crop") },
{ normal: cardUrl(id, "normal", "back", query), art_crop: cardUrl(id, "art_crop", "back") },
],
});

function stubLocaleArt(map: Record<string, string>) {
global.fetch = vi.fn().mockResolvedValue(
new Response(JSON.stringify(map), {
status: 200,
headers: { "Content-Type": "application/json" },
}),
);
}

beforeEach(() => {
vi.restoreAllMocks();
});

it("swaps the image to the localized printing, keeping the chosen printing", async () => {
const mod = await loadScryfallModule();
stubLocaleArt({ [EN_ID]: DE_ID });
await mod.loadLocaleArt("de");

const resolved = mod.resolvePrintingImageUrl(printing(EN_ID), 0, "normal");

expect(resolved).toBe(cardUrl(DE_ID));
// Non-vacuity: a no-op `localizeImageUrl` would return the English URL.
expect(resolved).not.toBe(cardUrl(EN_ID));
});

it("keeps English art when the printing has no localized sibling", async () => {
const mod = await loadScryfallModule();
stubLocaleArt({ [EN_ID]: DE_ID });
await mod.loadLocaleArt("de");

// Reach guard FIRST: prove the map actually loaded and the lookup runs.
// Without this, a failed fetch (empty map) would make the real assertion
// below pass for entirely the wrong reason.
expect(mod.resolvePrintingImageUrl(printing(EN_ID), 0, "normal")).toBe(cardUrl(DE_ID));

expect(mod.resolvePrintingImageUrl(printing(UNMAPPED_ID), 0, "normal")).toBe(
cardUrl(UNMAPPED_ID),
);
});

it("localizes the back face and the art crop", async () => {
const mod = await loadScryfallModule();
stubLocaleArt({ [EN_ID]: DE_ID });
await mod.loadLocaleArt("de");

// A localized Scryfall id addresses the whole printing; front/back is a path
// segment, so one mapping covers both faces of a DFC.
expect(mod.resolvePrintingImageUrl(printing(EN_ID), 1, "normal")).toBe(
cardUrl(DE_ID, "normal", "back"),
);
expect(mod.resolvePrintingImageUrl(printing(EN_ID), 0, "art_crop")).toBe(
cardUrl(DE_ID, "art_crop"),
);
});

it("is a no-op for English", async () => {
const mod = await loadScryfallModule();
stubLocaleArt({ [EN_ID]: DE_ID });
await mod.loadLocaleArt("de");
expect(mod.resolvePrintingImageUrl(printing(EN_ID), 0, "normal")).toBe(cardUrl(DE_ID));

// Assert the *gate*, not just the reset. `useCardImage` skips the load
// entirely when this reports ready, so an unconditionally-ready English
// would strand the German map installed and keep serving German art —
// `loadLocaleArt("en")` below would never run in production.
expect(mod.isLocaleArtReady("en")).toBe(false);

// Switching back to English must drop the map, not keep serving German art.
await mod.loadLocaleArt("en");
expect(mod.isLocaleArtReady("en")).toBe(true);
expect(mod.resolvePrintingImageUrl(printing(EN_ID), 0, "normal")).toBe(cardUrl(EN_ID));
});

it("never rewrites the card back or the placeholder", async () => {
const mod = await loadScryfallModule();
// Map the placeholder's and card back's own ids too, so the guard is doing
// the work rather than a lookup simply missing.
stubLocaleArt({ [EN_ID]: DE_ID, soon: DE_ID, "0aeebaf5-8c7d-4636-9e82-8c27447861f7": DE_ID });
await mod.loadLocaleArt("de");
expect(mod.resolvePrintingImageUrl(printing(EN_ID), 0, "normal")).toBe(cardUrl(DE_ID));

// `deriveImageUrl` is the exported probe for the same `splitSizedImageUrl`
// guard `localizeImageUrl` relies on; a URL it rejects is one localization
// also leaves alone. The placeholder must stay byte-identical or
// `isPlaceholderImageUrl`'s `===` stops gating the printing fallback.
for (const input of [mod.CARD_BACK_URL, "https://errors.scryfall.com/soon.jpg"]) {
expect(mod.imageUrlSize(input)).toBeNull();
expect(mod.deriveImageUrl(input, "small")).toBe(input);
}
});

it("reports readiness and tolerates a missing locale file", async () => {
const mod = await loadScryfallModule();
expect(mod.isLocaleArtReady("en")).toBe(true);
expect(mod.isLocaleArtReady("de")).toBe(false);

global.fetch = vi.fn().mockResolvedValue(new Response("", { status: 404 }));
await mod.loadLocaleArt("de");

// A 404 still counts as resolved — otherwise `useCardImage` would refetch on
// every render. Every card simply keeps its English art.
expect(mod.isLocaleArtReady("de")).toBe(true);
expect(mod.resolvePrintingImageUrl(printing(EN_ID), 0, "normal")).toBe(cardUrl(EN_ID));
});
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Loading
Loading