fix(question-feed): direct-access Yes/No chips, narrowing after no-match, art-crop gap documented - #687
Merged
Conversation
…tch, art-crop gap documented Three owner-reported question-feed interaction defects: 1. Attribute chips (attributeChipRender.tsx, shared by the question feed's ring and the display page's rail) required a tap sequence (untouched -> positive -> negative -> untouched) to reach "No", and an implied-negative sibling rendered dimmed while casting no vote of its own. Each chip is now a labelled Yes/No button pair - any of the three states is reachable in exactly one tap, and the button reflecting the voter's own explicit answer no longer depends on the fluctuating community-lean fill. 2. A not-official-printing no-match reason (altered-frame/upscaled/ no-collector-line/non-english - the artwork is genuine) now returns to the same item's Level 2 candidate grid with the attribute-chip filter already expanded, instead of skipping straight to the next item - reusing the funnel's existing chip-narrowing filter rather than a new selector. A not-official-art reason (custom-art/ai-art/external-ip) still advances straight through - there's nothing left to narrow towards on that axis. 3. Confirmed and documented (docs/features/printing-tags.md's Known gaps): illustration-grouped candidate tiles still render each printing's own scan rather than a shared art crop, because no per-candidate or per-illustration art-crop URL exists on the questionFeed payload today (only an unrelated item-level scryfallIllustrationUrl consumed by the artist question type). Making this fixable needs a backend payload change, out of scope here and not attempted.
WilfordGrimley
added a commit
that referenced
this pull request
Aug 4, 2026
…didate Adds PrintingCandidate.artCropUrl, sourced from the existing CanonicalPrintingMetadata.art_crop_url sidecar field (no new fetch or harvest - the data is already on disk), following the illustrationId precedent in shape and style: - CanonicalCard.serialise_as_printing_candidate populates it, null- tolerant for both the no-sidecar and empty-art_crop_url cases. - PrintingCandidate in both schema_types.py and schema_types.ts gains the optional field (backend from_dict/to_dict, frontend interface + typeMap entry). - docs/features/printing-tags.md's Known gaps section notes the field now exists but nothing consumes it yet - rendering it is future frontend work. Query efficiency: every path that reaches serialise_as_printing_candidate (get_ranked_printing_candidates's two branches, and the ai_vote lookup in question_feed._confirm_suggestion_item) already select_related's printing_metadata for illustrationId's sake, so no new prefetch wiring was needed. A new test proves the candidate-grid serialisation issues zero additional queries regardless of candidate count. This closes the backend half of the gap PR #687 (question-feed-frontend- fixes branch, not yet merged) identified: illustration-clustered candidate tiles have no shared art crop to render instead of each candidate's own full-card scan. Consuming this field in the frontend grid remains out of scope here.
17 tasks
WilfordGrimley
added a commit
that referenced
this pull request
Aug 4, 2026
…s; reconcile docs Item 1: merged origin/master (PR #685 was CONFLICTING) - the reconciled Known-gaps bullet takes the branch side (artCropUrl now exists, nothing consumed it) since it is the informed successor to master's #687 bullet. Item 2: illustration-clustered candidate tiles now render PrintingCandidate.artCropUrl in place of mediumThumbnailUrl, falling back to the printing scan when a candidate's metadata sidecar has none. Ungrouped/identify_printing tiles are unaffected. Item 4: the Known-gaps bullet this closes is removed; the shipped behaviour is documented on the existing 'Illustration grouping' architecture bullet instead.
WilfordGrimley
added a commit
that referenced
this pull request
Aug 5, 2026
…on (#685) * fix(image_evidence): wire the artist-crop fallback into live extraction artist_ocr_name stays blank on 87,371 evidence rows that carry non-blank artist_ocr_raw_text - the "Illus." anchor misses OCR-noisy reads (e.g. "Soot Itus." never matches _ILLUS_RE), and the 2026-07-29 collector-line recovery (recover_artist_from_card_text) only reads collector_line_raw_text/ legal_line_raw_text, so it is structurally blind to old-border proxies whose only credit is a centred "Illus. <name>" line with no bottom print row at all. cardpicker.modern_artist_credit.recognize_artist_credit already exists for exactly this shape of input (re-reading artist_ocr_raw_text) but was only ever reachable through its own standalone backfill_modern_artist_names command, never during real extraction. This wires it into compute_card_evidence as a third fallback, tried only after both the anchor and the collector/legal-line recovery find nothing storable - strictly additive (new modern_artist_lexicon param, default None, no extractor_versions bump), and threaded through both live Stage C entry points (stage_e_dispatch.py, run_image_evidence_cohort.py). Verified against real production evidence rows (ids 221241/221268/221274): recognize_artist_credit recovers "Sebastian Giacobino"/"Aaron Miller"/ "Andrey Kuzinskiy" from their stored artist_ocr_raw_text at ratio 1.0 against the real ~2.5k-name lexicon. A fourth reproduction row (id 221289, "David Rapoza") turned out to be a correct abstention, not a bug: the real lexicon holds both "Dave Rapoza" and "David Rapoza" as distinct canonical artists, so the reading is genuinely ambiguous under the 2026-07-29 ruling (fuzzy matching yes, fuzzy storage no) - confirmed by direct measurement, not assumed. Full-population measurement of the 87,371-row split: 19,478 (22.3%) genuinely recoverable via the artist-crop fallback, 329 (0.4%) genuinely ambiguous (correctly abstained), 67,564 (77.3%) carry no artist credit findable by any means - consistent with this catalog's known proxy population rendered without standard bottom-row metadata. * fix(tests): thread modern_artist_lexicon through remaining compute_card_evidence stubs test_stage_e_shakedown.py and test_stream_full_catalog.py's _install_ok_stage_c_stub helpers mirrored compute_card_evidence's pre-modern_artist_lexicon signature, missed by the sibling repair in test_stage_e_dispatch.py and test_run_image_evidence_cohort.py (bf17be1) because neither module was run locally. Same fix, same shape: accept and ignore the new keyword-only param. * feat(question-feed): expose per-candidate art-crop URL on PrintingCandidate Adds PrintingCandidate.artCropUrl, sourced from the existing CanonicalPrintingMetadata.art_crop_url sidecar field (no new fetch or harvest - the data is already on disk), following the illustrationId precedent in shape and style: - CanonicalCard.serialise_as_printing_candidate populates it, null- tolerant for both the no-sidecar and empty-art_crop_url cases. - PrintingCandidate in both schema_types.py and schema_types.ts gains the optional field (backend from_dict/to_dict, frontend interface + typeMap entry). - docs/features/printing-tags.md's Known gaps section notes the field now exists but nothing consumes it yet - rendering it is future frontend work. Query efficiency: every path that reaches serialise_as_printing_candidate (get_ranked_printing_candidates's two branches, and the ai_vote lookup in question_feed._confirm_suggestion_item) already select_related's printing_metadata for illustrationId's sake, so no new prefetch wiring was needed. A new test proves the candidate-grid serialisation issues zero additional queries regardless of candidate count. This closes the backend half of the gap PR #687 (question-feed-frontend- fixes branch, not yet merged) identified: illustration-clustered candidate tiles have no shared art crop to render instead of each candidate's own full-card scan. Consuming this field in the frontend grid remains out of scope here. * feat(question-feed): render artCropUrl on illustration-clustered tiles; reconcile docs Item 1: merged origin/master (PR #685 was CONFLICTING) - the reconciled Known-gaps bullet takes the branch side (artCropUrl now exists, nothing consumed it) since it is the informed successor to master's #687 bullet. Item 2: illustration-clustered candidate tiles now render PrintingCandidate.artCropUrl in place of mediumThumbnailUrl, falling back to the printing scan when a candidate's metadata sidecar has none. Ungrouped/identify_printing tiles are unaffected. Item 4: the Known-gaps bullet this closes is removed; the shipped behaviour is documented on the existing 'Illustration grouping' architecture bullet instead. * feat(image-evidence): bump artist_ocr extractor to v3 (PR #685) modern_artist_credit.recognize_artist_credit lands in this PR as a third artist fallback inside compute_card_evidence, but without a version bump the fix reaches no existing row: Stage C's resume filter (MANIFEST_EXTRACTOR_CURRENT_VERSIONS, issue #509) compares stored extractor_versions against the manifest and skips any row whose value already matches, and all 230,378 production rows currently carry artist_ocr: artist-ocr-v2. Bumps ARTIST_OCR_EXTRACTOR_VERSION and its MANIFEST_EXTRACTOR_CURRENT_VERSIONS counterpart to artist-ocr-v3, with a dedicated version-history comment on the constant (the shared v1->v2 OCR-engine-swap comment above it now covers only collector_line_ocr/collector_line_tsv, which are not bumped here). Updates the three test fixtures asserting the literal "artist-ocr-v2" string to match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three owner-reported question-feed interaction defects (frontend-only;
question_feed.py/reason_tags.pyand every other Python file are out ofscope here, handled by a separate concurrent worker):
Several clicks before a vote counts. Confirmed root cause by reading
nextChipStateand the level3/candidate render blocks: attribute chips(
attributeChipRender.tsx, shared verbatim by the question feed's ringand the display page's rail — a header comment enforces this) required a
tap sequence (
untouched -> positive -> negative -> untouched) toreach "No", and an implied-negative exclusion-group sibling rendered
dimmed at opacity 0.45 while having cast no vote of its own — a voter
reading that dimming as "I already said no" was looking at UI that
disagreed with the data. Each chip is now a labelled Yes/No button pair:
any of the three meaningful answers (yes / no / no opinion) is reachable
in exactly one tap from any prior state, and the button reflecting the
voter's own explicit answer is now visually independent of the
fluctuating community-plus-machine lean fill (which is what let a
just-cast vote appear to "not have registered" once the server's real
weighted polarity replaced the optimistic one). Retraction (tapping the
already-active button back to untouched, casting
RETRACT_POLARITY) isunchanged — only how many taps it takes to reach any of the three states
changed.
Bringing the narrowing selector in after a no-match. On the
NoMatchReasonStrip's not-official-printing axis (altered-frame /upscaled / no-collector-line / non-english — the artwork is genuine,
only which printing is still in question), choosing a reason now returns
to the same item's Level 2 candidate grid with the attribute-chip filter
panel already expanded, instead of skipping straight to the next item.
This reuses the funnel's own existing chip-narrowing filter
(
filterCandidatesByChipStates+ the sharedrenderAttributeChip)rather than forking in the unified editor's
SelectVersionResults.tsxselector pieces, and candidate selection still goes through the
unchanged, already-supported
selectCandidatepath — no new printingassertion is submitted. On the not-official-art axis (custom-art /
ai-art / external-ip), the artwork itself isn't from any official card,
so there's nothing to narrow towards — that axis still advances straight
through, unchanged.
Printings shown where an art crop would do. Confirmed by reading the
candidate-tile render and
schema_types.ts: illustration-clusteredtiles vote at illustration granularity (
selectIllustrationGroup->/2/submitIllustrationVote/) but still render each candidate's ownmediumThumbnailUrl(a per-printing scan), because no per-candidate orper-illustration art-crop URL exists on the questionFeed payload —
QuestionFeedItem.scryfallIllustrationUrlis a single, item-level fieldconsumed only by the unrelated
artistquestion type's subject-cardre-frame. Documented as a known gap in
docs/features/printing-tags.md, with what a backend change would needto add to make this fixable. No backend change was made or attempted —
out of scope for this PR.
Test plan
attributeChips.test.ts— pass, individually and combinedAttributeChipPanel.test.tsx— pass, individually and combined(rewritten: the old "cycles through 3 states" test is now "reaches
each state in exactly one tap, no cycle")
QuestionFeed.test.tsx— pass, individually and combinedNoMatchReasonStrip.spec.ts(Playwright e2e, real browser + mockednetwork) — pass; two new cases added (not-official-printing returns
to the grid, not-official-art still advances) and the existing
"OTHER group" case updated to match the new not-official-printing
routing
QuestionFeed.spec.ts+QuestionFeedResponsive.spec.ts(Playwrighte2e) — pass, including the 44px tap-target-floor check against the
redesigned chip
npx tsc --noEmit— cleannpx eslinton every touched file — 0 errors (pre-existingno-img-elementwarnings only, unrelated to this change)npm test— same 22 pre-existing failures as before thisbranch (
comparison.test.ts'sSet.prototype.symmetricDifference,unavailable on this sandbox's Node 20 runtime; untouched by this PR),
no new failures
Task-end checks (CLAUDE.md)
docs/features/ printing-tags.md's "Known gaps" section updated in place with theart-crop finding; no new dated section appended.
wiki documents beyond what
printing-tags.mdalready covers — no wikiedit needed.
against
master, not merged — merges are the owner's alone.