From 38c2ef7f9a8f9e65fdecfabf6ed9391ae965f050 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 6 Aug 2026 01:41:41 +0000 Subject: [PATCH 1/2] questionFeed: render one representative tile per illustration group The vote path (selectIllustrationGroup) already submits a single illustrationId for the whole cluster, so the render side rendering every member of the cluster showed the same artwork up to 9 times for a question with one answer. Render only a representative tile per group instead: prefer a member with an art crop (the same signal a tile's own image already prefers), falling back to the group's first member when none has one, so the pick is stable across renders of the same data. The submitted illustrationId is unchanged - every group member shares it. --- .../questionFeed/QuestionFeed.test.tsx | 93 ++++++++++++++++++- .../features/questionFeed/QuestionFeed.tsx | 35 ++++--- 2 files changed, 110 insertions(+), 18 deletions(-) diff --git a/frontend/src/features/questionFeed/QuestionFeed.test.tsx b/frontend/src/features/questionFeed/QuestionFeed.test.tsx index 9cd937cc3..167b7cc47 100644 --- a/frontend/src/features/questionFeed/QuestionFeed.test.tsx +++ b/frontend/src/features/questionFeed/QuestionFeed.test.tsx @@ -892,7 +892,7 @@ describe("QuestionFeed", () => { expect(illustrationVoteCalled).toBe(false); }); - it("clustered tiles render the candidate's art crop, falling back to the printing scan when absent; ungrouped tiles are unaffected", async () => { + it("renders exactly one representative tile per illustration group, preferring a member with an art crop; ungrouped tiles are unaffected", async () => { const withArtCrop = { ...groupedItem, candidates: [ @@ -930,10 +930,10 @@ describe("QuestionFeed", () => { "src", "https://example.com/art-crop-1.png" ); - expect(within(group).getByAltText("xyz 42")).toHaveAttribute( - "src", - groupedItem.candidates[1].mediumThumbnailUrl - ); + expect(within(group).queryByAltText("xyz 42")).not.toBeInTheDocument(); + expect( + within(group).getByText("Same illustration - 2 printings") + ).toBeInTheDocument(); const ungroupedGrid = await screen.findByTestId( "question-feed-candidate-grid-ungrouped" @@ -943,5 +943,88 @@ describe("QuestionFeed", () => { groupedItem.candidates[2].mediumThumbnailUrl ); }); + + it("falls back to the group's first member when no member has an art crop", async () => { + server.use(groupedQuestionFeedOnce()); + renderFeed(); + await revealCard(); + + const group = await screen.findByTestId( + "question-feed-illustration-group" + ); + expect(within(group).getByAltText("abc 1")).toHaveAttribute( + "src", + groupedItem.candidates[0].mediumThumbnailUrl + ); + expect(within(group).queryByAltText("xyz 42")).not.toBeInTheDocument(); + }); + + it("picks whichever group member has an art crop regardless of position, and still submits the group's shared illustrationId", async () => { + const artCropOnSecondMember = { + ...groupedItem, + candidates: [ + { ...groupedItem.candidates[0], artCropUrl: null }, + { + ...groupedItem.candidates[1], + artCropUrl: "https://example.com/art-crop-2.png", + }, + groupedItem.candidates[2], + ], + }; + server.use( + http.get(buildRoute("2/questionFeed/"), () => + HttpResponse.json( + { + item: artCropOnSecondMember, + remainingEstimate: { + total: 1, + confirmable: 0, + contested: 0, + fresh: 1, + }, + }, + { status: 200 } + ) + ) + ); + let illustrationVoteBody: Record | undefined; + server.use( + http.post( + buildRoute("2/submitIllustrationVote/"), + async ({ request }) => { + illustrationVoteBody = (await request.json()) as Record< + string, + unknown + >; + return HttpResponse.json( + { + illustrationId: sharedIllustrationId, + isUnknown: false, + printingVoteCast: false, + artistVoteCast: true, + }, + { status: 200 } + ); + } + ) + ); + renderFeed(); + await revealCard(); + + const group = await screen.findByTestId( + "question-feed-illustration-group" + ); + const tile = within(group).getByAltText("xyz 42"); + expect(tile).toHaveAttribute("src", "https://example.com/art-crop-2.png"); + expect(within(group).queryByAltText("abc 1")).not.toBeInTheDocument(); + + fireEvent.click(tile); + + await waitFor(() => expect(illustrationVoteBody).toBeDefined()); + expect(illustrationVoteBody).toMatchObject({ + identifier: groupedItem.card.identifier, + illustrationId: sharedIllustrationId, + }); + }); }); }); diff --git a/frontend/src/features/questionFeed/QuestionFeed.tsx b/frontend/src/features/questionFeed/QuestionFeed.tsx index 8e3a37340..7417dc40b 100644 --- a/frontend/src/features/questionFeed/QuestionFeed.tsx +++ b/frontend/src/features/questionFeed/QuestionFeed.tsx @@ -1619,6 +1619,14 @@ export function QuestionFeed() { const illustrationArtist = group .map((candidate) => candidate.artist) .find((artist) => artist.trim() !== ""); + // selectIllustrationGroup already submits one illustrationId for the whole + // cluster (see that function's own comment), so one tile fully represents what + // is being voted on - showing every member here just repeats the same artwork + // up to N times. Prefer a member with an art crop, the same signal a tile's own + // image already prefers (renderCandidateTile's imageUrl default below), and fall + // back to the first member so the same data always picks the same tile. + const representative = + group.find((candidate) => candidate.artCropUrl) ?? group[0]; return ( )} - {group.map((candidate) => - renderCandidateTile( - candidate, - () => - // every member of `group` shares this non-null illustrationId - see the - // grouping logic above, which only clusters candidates that have one. - selectIllustrationGroup( - candidate.illustrationId as string, - candidate - ), - false, - candidate.artCropUrl || candidate.mediumThumbnailUrl - ) + {renderCandidateTile( + representative, + () => + // every member of `group` shares this non-null illustrationId - see + // the grouping logic above, which only clusters candidates that have + // one - so submitting the representative's illustrationId is + // identical to submitting any other member's. + selectIllustrationGroup( + representative.illustrationId as string, + representative + ), + false, + representative.artCropUrl || + representative.mediumThumbnailUrl )} From 56c2d17dead2f6ed02d093bead43b2ebfb2d4ad4 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 6 Aug 2026 02:10:53 +0000 Subject: [PATCH 2/2] QuestionFeed.spec.ts: update illustration-group specs for the one-representative-tile render Three Level 2 illustration-grouping specs still asserted the retired one-tile-per-group-member render: they expected candidateB (which shares an illustration with candidateA but has no art crop) to get its own DOM tile, both inside the group container and in the top-level candidate list. The group now renders exactly one representative tile per cluster (group.find((c) => c.artCropUrl) ?? group[0]), so a non-representative member renders no tile of its own anywhere - it's represented through the group's shared illustration vote, not silently dropped. Renamed and rewrote the three affected titles to assert the new contract directly (representative renders, non-representative member renders nowhere, ungrouped candidates are unaffected) and added the matching coverage-ack entries for the renames. --- .github/coverage-acks.txt | 10 +++++++++ frontend/tests/QuestionFeed.spec.ts | 33 +++++++++++++++++++---------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/.github/coverage-acks.txt b/.github/coverage-acks.txt index d2046fce1..83c7a4921 100644 --- a/.github/coverage-acks.txt +++ b/.github/coverage-acks.txt @@ -127,3 +127,13 @@ coverage-ack: frontend/tests/DisplayPage.spec.ts::DisplayPage (Proposal H, Step # Same surface, same assertions, only the URL (and the describe/test title naming it) changed. coverage-ack: frontend/tests/ContrastAudit.spec.ts::Contrast audit - /contributions (owner defect 1+2: accordion header + body) > Contribution Guidelines accordion - collapsed and expanded — route moved /contributions -> /stats in PR #558; same surface, same assertions coverage-ack: frontend/tests/ContrastAudit.spec.ts::Link colour audit (owner-approved open item 2 - $link-color -> $theme-info) > a real production link (contributions guidelines' ISO-639-1 reference, inside the accordion body's panel-bg) clears strict-AAA-normal — route moved /contributions -> /stats in PR #558; same surface, same assertions + +# PR #730 - QuestionFeed.tsx's illustration-group render now emits ONE representative tile per +# group (`group.find((c) => c.artCropUrl) ?? group[0]`) instead of one tile per member, so a +# non-representative member (candidateB in this fixture set) no longer gets its own DOM tile +# anywhere - it's represented by the group's shared vote, not silently dropped. The three old +# titles asserted the retired one-tile-per-member behavior; all three are renamed (not deleted) +# to describe the new representative-tile contract, same surface, same fixture set. +coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > every candidate in a mixed illustration set still renders in the grid - none silently dropped — PR #730: renamed to "every candidate in a mixed illustration set is accounted for - a group collapses to its one representative tile, ungrouped candidates still render individually" now that a group renders one representative tile, not one tile per member. +coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > candidates sharing an illustration render inside one illustration-group container; unique/null-illustration candidates don't — PR #730: renamed to "only the illustration group's representative candidate renders inside its one illustration-group container; unique/null-illustration candidates don't", same reason as the sibling ack above. +coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > clustered tiles render each candidate's art crop, falling back to the printing scan when absent; ungrouped tiles keep the printing scan regardless — PR #730: renamed to "the illustration group's one tile renders its representative candidate's art crop; ungrouped tiles keep the printing scan regardless", same reason as the sibling acks above. diff --git a/frontend/tests/QuestionFeed.spec.ts b/frontend/tests/QuestionFeed.spec.ts index 709941dd5..2012da6cf 100644 --- a/frontend/tests/QuestionFeed.spec.ts +++ b/frontend/tests/QuestionFeed.spec.ts @@ -232,7 +232,7 @@ test.describe("question feed - Level 3 (conditional open-attribute confirm)", () // frequently-absent shape (CanonicalPrintingMetadata.illustration_id, see // local_illustration.py:137's isnull filter). test.describe("question feed - Level 2 illustration grouping", () => { - test("every candidate in a mixed illustration set still renders in the grid - none silently dropped", async ({ + test("every candidate in a mixed illustration set is accounted for - a group collapses to its one representative tile, ungrouped candidates still render individually", async ({ page, network, }) => { @@ -242,12 +242,26 @@ test.describe("question feed - Level 2 illustration grouping", () => { ); await loadPageWithDefaultBackend(page, "whatsthat"); + // candidateA carries the art crop, so it's the group's chosen representative + // (QuestionFeed.tsx's `group.find((c) => c.artCropUrl) ?? group[0]`) - the group renders + // ONE tile for the whole cluster, not one per member. + await expect( + page.locator( + `[data-card-identifier="${illustrationGroupCandidateA.identifier}"]` + ) + ).toHaveCount(1); + // candidateB shares the illustration but loses the representative pick to A - it renders + // no tile of its own anywhere; its vote is carried by A's tile, not silently dropped. + await expect( + page.locator( + `[data-card-identifier="${illustrationGroupCandidateB.identifier}"]` + ) + ).toHaveCount(0); + // The regression guard this task calls out explicitly: an exact count, not just "at least // one" - a candidate silently vanishing (e.g. because it has no illustrationId) is exactly // the correctness regression a weaker assertion would miss. for (const candidate of [ - illustrationGroupCandidateA, - illustrationGroupCandidateB, illustrationGroupCandidateC, illustrationGroupCandidateD, ]) { @@ -257,7 +271,7 @@ test.describe("question feed - Level 2 illustration grouping", () => { } }); - test("candidates sharing an illustration render inside one illustration-group container; unique/null-illustration candidates don't", async ({ + test("only the illustration group's representative candidate renders inside its one illustration-group container; unique/null-illustration candidates don't", async ({ page, network, }) => { @@ -278,11 +292,13 @@ test.describe("question feed - Level 2 illustration grouping", () => { `[data-card-identifier="${illustrationGroupCandidateA.identifier}"]` ) ).toHaveCount(1); + // candidateB shares the illustration but has no art crop, so A wins the representative + // pick and the group renders only A's tile - not B's. await expect( group.locator( `[data-card-identifier="${illustrationGroupCandidateB.identifier}"]` ) - ).toHaveCount(1); + ).toHaveCount(0); // candidateC (distinct illustrationId, no sibling) and candidateD (null illustrationId) // both render OUTSIDE the clustered group - neither forms (or joins) a cluster of one. @@ -298,7 +314,7 @@ test.describe("question feed - Level 2 illustration grouping", () => { ).toHaveCount(0); }); - test("clustered tiles render each candidate's art crop, falling back to the printing scan when absent; ungrouped tiles keep the printing scan regardless", async ({ + test("the illustration group's one tile renders its representative candidate's art crop; ungrouped tiles keep the printing scan regardless", async ({ page, network, }) => { @@ -314,11 +330,6 @@ test.describe("question feed - Level 2 illustration grouping", () => { `[data-card-identifier="${illustrationGroupCandidateA.identifier}"] img` ) ).toHaveAttribute("src", illustrationGroupCandidateA.artCropUrl as string); - await expect( - group.locator( - `[data-card-identifier="${illustrationGroupCandidateB.identifier}"] img` - ) - ).toHaveAttribute("src", illustrationGroupCandidateB.mediumThumbnailUrl); const ungroupedGrid = page.getByTestId( "question-feed-candidate-grid-ungrouped"