test(e2e): add Link Wizard candidate verification coverage - #353
Open
chiptus wants to merge 18 commits into
Open
test(e2e): add Link Wizard candidate verification coverage#353chiptus wants to merge 18 commits into
chiptus wants to merge 18 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical routing, authentication, mocking, interception, and selection issues currently prevent reliable test execution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Playwright E2E coverage for the Link Wizard candidate verification flow using mocked edge-function responses.
Changes:
- Tests candidate sorting, descriptions, pagination, link-out isolation, and set information.
- Covers candidate selection and the Save & Next workflow.
Review findings:
- Critical — 3 votes each: Incorrect non-admin route; sign-in user lacks
admin_roles; mocked batch request aborts the seeded Kiara request;OPTIONSpreflight is parsed as JSON before handling. - Critical — 3 votes: PATCH mock omits the
?id=...query and may mutate shared seed data. - Critical — 2 votes: Artist selection is nondeterministic, so the Spotify flow may target Nils Frahm instead of Kiara.
- Moderate — 3 votes each: Card locator matches nothing; popup URL may be read before navigation; save assertion does not verify mutation success; set-info and co-performer assertions are incomplete; sorting is not verified by DOM order.
- Moderate — 2 votes each: Candidate counts rely on non-retrying snapshots after fixed delays.
File summaries
| File | Description |
|---|---|
tests/e2e/link-wizard-candidate-verification.spec.ts |
New Link Wizard end-to-end coverage and regression guard. |
Review details
Suppressed comments (3)
tests/e2e/link-wizard-candidate-verification.spec.ts:265
- The second edge-function route has the same
OPTIONSpreflight problem: it readspostData.artistNamesfor a request with no body. Add the same preflight handling beforepostDataJSON()so this regression test can reach the mocked POST response.
await route.fulfill({
status: 200,
tests/e2e/link-wizard-candidate-verification.spec.ts:330
- The second popup assertion has the same race: the page event does not guarantee that navigation has completed before
url()is read. Wait for the URL with a retrying assertion before closing the page.
tests/e2e/link-wizard-candidate-verification.spec.ts:299 - This test also relies on the first missing-link artist being Kiara, but the query order is unspecified and Nils Frahm is another seeded incomplete artist. In that case the test has no Spotify input and the mocked result is for the wrong provider. Select Kiara explicitly after loading the wizard.
await expect(linkWizardTitle).toBeVisible({ timeout: 15000 });
- Files reviewed: 1/1 changed files
- Comments generated: 13
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Playwright test results
Details
|
Playwright test results
Details
Flaky testsMobile Safari › not-found-routes.spec.ts › Invalid festival/edition slugs › shows edition-specific not found UI for an invalid edition slug |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Multiple unresolved moderate findings remain in the E2E coverage and unrelated tooling changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
tests/e2e/link-wizard-candidate-verification.spec.ts:169
- This describe is tagged
@smoke, but each of its two tests performs its own OTP login. The repository's smoke-test guidance says to amortize sign-in with a sharedbeforeAll/serial context (tests/README.md:100-105); this doubles the slow setup and makes the spec more likely to hit the CI timeout. Share one authenticated context or remove the smoke tag.
tests/e2e/link-wizard-candidate-verification.spec.ts:267 - Waiting for the PATCH response only verifies transport-level success; it does not verify the mutation's awaited invalidations or the per-call
onNextcallback. A failure in either could leave the wizard on Kiara while this test still passes. After clicking, assert that the wizard advances to a different artist (and/or a success state) in addition to checking the PATCH.
tests/e2e/link-wizard-candidate-verification.spec.ts:235 - Checking only
spotifyUrlInputleaves a gap in the required “no staged-fields preview update” behavior: this candidate also has an image and description, so a link-out regression that stages either field while leaving the URL empty would pass. Assert that the staged description and image state remain unchanged after the link click.
tests/e2e/link-wizard-candidate-verification.spec.ts:306 - The regression-guard test also checks only the Spotify URL after link-out. Because the first candidate has both
imageUrlanddescription, a bug that stages either non-URL field would still pass this guard; assert the complete staged preview state, not just the URL input.
tests/e2e/link-wizard-candidate-verification.spec.ts:267 - A successful PATCH response does not prove that the selected candidate was submitted: a regression sending
spotify_url: nullor another value would still return 2xx while the staged input assertion remains green. Assert the PATCH request body containshttps://spotify.com/artist/test-rising(or re-read the artist after saving).
- Files reviewed: 68/68 changed files
- Comments generated: 6
- Review effort level: Lite
Comment on lines
+16
to
+18
| test.afterEach(async () => { | ||
| await fetch( | ||
| `${TEST_CONFIG.SUPABASE_URL}/rest/v1/artists?id=eq.${KIARA_ARTIST_ID}`, |
Adds comprehensive Playwright e2e spec covering the full candidate verification flow: mocks the search-artist-links edge function with a controlled dataset of 5 candidates (varying followers and descriptions), tests set-info panel displays correct details, verifies candidates render in descending-follower order with only 3 shown initially, confirms "Show more" reveals all candidates, validates candidate descriptions render, tests provider link-out opens new tab without selecting candidate (with regression guard), and verifies full selection and save flow. Closes #349 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015MfZ5GedRwGhQ3ae4HCrtE
Addresses Copilot review findings: wrong non-admin route path, missing admin role on the signed-in user, unhandled CORS preflight on the mocked edge function, a candidate card locator that matched nothing, nondeterministic artist selection, and several flaky snapshot-then-assert patterns. Also avoids mutating the shared seeded artist row across test runs.
The set description assertion matched both the read-only set card and the staged description textarea (same seeded text for artist and set). Scope it to the set-info panel. Also wait for the candidates panel label and give candidate rendering a longer timeout before asserting card visibility.
…onse The mocked POST response for search-artist-links lacked Access-Control-Allow-Origin. The browser accepted the mocked OPTIONS preflight but then silently blocked reading the actual POST response body as a cross-origin violation, so no candidates ever rendered.
…card A wildcard Access-Control-Allow-Headers does not cover the Authorization header per the CORS spec, so the browser was silently blocking supabase-js's actual request even though the mocked preflight itself returned 200 — candidates never rendered. List the headers explicitly, matching the real edge function's own buildCorsHeaders.
The mock's CORS headers were already fixed, but candidates still don't render in CI and the timeout gives no clue why. Fail fast with the panel's actual error alert text instead of a generic locator-not-found timeout.
The diagnostic error-alert check didn't trigger, ruling out an isError state, but candidates still don't render. Log what the mock actually receives so the next CI run shows whether/how the request differs from what's expected.
Debug logging showed the mock was intercepting correctly and returning the expected candidates, but requests kept repeating in a growing backoff pattern — a sign of TanStack Query retrying a throwing queryFn. searchResponseSchema's error field is optional but rejects null; the mock's `error: null` on every result failed zod's .parse() on every attempt, so the query retried until exhausted and candidates never rendered. Omitting the field (matching what "no error" actually looks like) fixes it. Also drops the debug logging now that root cause is confirmed, keeping the panel-error diagnostic as a useful safety net.
Candidates now render correctly (previous fix worked), but .bg-card also matched the wizard's own outer Card, since it wraps the candidate cards as descendants and Playwright's hasText/has filters match descendant content. Use the more specific .bg-card.p-3 — only leaf CandidateCard instances pass that extra class.
The regression-guard test now passes fully. The happy-path test's real (unmocked) PATCH to /rest/v1/artists occasionally exceeds 10s under CI load; give it 20s before asserting the success toast.
…eout The success-toast timeout was bumped to 20s but still fails at the same spot — suggesting the mutation may be erroring rather than being slow. Capture whichever toast actually appears and include its text in the failure so the next CI run shows the real cause (success vs error vs none).
The last CI failure was not a real toast/mutation problem — the diagnostic race reported "got none" because Playwright killed the test at its default 30s timeout while still inside the up-to-20s wait. Prior steps (sign-in, navigation, several UI interactions) already used a good chunk of that budget. Raise the test's own timeout to 60s so the save step actually gets to complete and the diagnostic can do its job.
…irty form.formState.isDirty was only read inside onSubmit's closure, which react-hook-form's Proxy-based formState never keeps live outside of render — so the save handler always took the early-return branch and never called the update mutation. Reading isDirty during render fixes it. Also replace the e2e test's toast-visibility race (too transient under CI load) with a waitForResponse assertion on the actual PATCH request. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
chiptus
force-pushed
the
feat/349/e2e-candidate-verification
branch
from
August 22, 2026 16:34
99e8d44 to
f244328
Compare
chiptus
commented
Aug 22, 2026
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues were identified in the supplied final comments.
Review details
Suppressed comments (7)
tests/e2e/link-wizard-candidate-verification.spec.ts:20
- With
fullyParallel: trueinplaywright.config.ts:37, these tests can run concurrently across the browser projects. Both use and mutate the same Kiara row, so one test can save Spotify before the other loads its missing-links query (or cleanup can reset it mid-run), making the row disappear or the assertions nondeterministic. Use isolated per-test/per-project fixture rows or avoid mutating the shared seed instead of relying on this hook.
test.afterEach(async () => {
await fetch(
`${TEST_CONFIG.SUPABASE_URL}/rest/v1/artists?id=eq.${KIARA_ARTIST_ID}`,
{
method: "PATCH",
tests/e2e/link-wizard-candidate-verification.spec.ts:198
- The set-info assertions cover the title, stage, and description but never verify the rendered time or the co-performer section required by #349. A regression removing either part of
ArtistSetCardwould still pass; assert the formatted time and, with this seed, at least theNo other co-performersstate (or use a fixture with another performer for the positive case).
await expect(setInfoPanel).toBeVisible();
await expect(setInfoPanel.getByText("Club Stage")).toBeVisible();
await expect(setInfoPanel.getByText(KIARA_SET_DESCRIPTION)).toBeVisible();
tests/e2e/link-wizard-candidate-verification.spec.ts:8
- The PR verification instructions use
/festivals/test/editions/2025/links, but this spec uses/admin/festivals/test/editions/2025/links; the Link Wizard route is only defined under the admin prefix. Update the documented verification path so the manual check reaches this flow.
const LINK_WIZARD_PATH = "/admin/festivals/test/editions/2025/links";
tests/e2e/link-wizard-candidate-verification.spec.ts:226
- This click navigates to the live
https://spotify.comsite. In a local/mocked e2e run, blocked or slow outbound access can fail the click or consume the navigation timeout even though the popup behavior is correct. Fulfill this domain in the test context (or otherwise inspect the popup URL without a live provider request) before clicking.
const [newPage] = await Promise.all([
tests/e2e/link-wizard-candidate-verification.spec.ts:29
- Selecting the Rising candidate with
Select allstages its image because the seeded artist has no image, and the successful PATCH persists that value. This cleanup only clearsspotify_url, so the test leavesimage_urlset to the mock image and contaminates the shared seed for later tests or runs; reset every field this flow changes.
body: JSON.stringify({ spotify_url: null }),
},
);
tests/e2e/link-wizard-candidate-verification.spec.ts:129
- The mock response returns
MOCK_CANDIDATESin the same descending-follower order in which it is declared, so these assertions pass even if the UI stops sorting and renders API order. Make the mocked response intentionally unsorted (while keeping the expected DOM order sorted) so this is a real sorting regression guard.
candidates:
provider === "spotify" ? (candidatesByArtist[name] ?? []) : [],
})),
tests/e2e/link-wizard-candidate-verification.spec.ts:240
- After expanding, the test checks only the count and that Micro is present. It never verifies that the fourth and fifth cards are Niche and Micro, so a sort regression affecting the tail of the list still passes; assert the order of all five after clicking Show more.
// 4. "Show more" reveals all 5 candidates, including one without a description.
await page.getByRole("button", { name: "Show more" }).click();
await expect(candidateCards).toHaveCount(5);
await expect(page.getByText("Test Artist Micro")).toBeVisible();
- Files reviewed: 2/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
WebKit can't intercept requests that pass through an active Service Worker — page.route() silently misses them, unlike Chromium's CDP-based interception. The app's PWA service worker (active even in dev per devOptions.enabled) was letting mocked API calls (e.g. search-artist-links) fall through to the real, unavailable local edge function, causing link-wizard-candidate-verification.spec.ts to fail only on webkit in CI. Blocking service workers for the webkit project keeps route() mocks reliable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Mobile Safari still permits service workers, so the mocked edge function may be bypassed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
tests/e2e/link-wizard-candidate-verification.spec.ts:20
- These tests share and mutate the same seeded Kiara row, but the suite runs with
fullyParallel: trueand local workers are not limited to one. The second test can fetch after the first PATCH and omit Kiara before thisafterEachreset runs, so the regression guard is order-dependent. Serialize this suite and/or isolate the artist state per project.
test.afterEach(async () => {
await fetch(
`${TEST_CONFIG.SUPABASE_URL}/rest/v1/artists?id=eq.${KIARA_ARTIST_ID}`,
{
method: "PATCH",
tests/e2e/link-wizard-candidate-verification.spec.ts:129
- Despite the flow requiring an unsorted fixture,
MOCK_CANDIDATESis declared in the same descending-follower order asserted below and this response preserves that order. A regression that removessortCandidatesByFollowerswould still pass. Send an intentionally unsorted copy (or reorder the fixture) before fulfilling.
candidates:
provider === "spotify" ? (candidatesByArtist[name] ?? []) : [],
})),
tests/e2e/link-wizard-candidate-verification.spec.ts:198
- The set-info checks cover only the card's presence, stage, and description. They do not assert the rendered time, and Kiara's seeded set has no other
set_artistsrow, so the co-performer rendering path is never exercised even though the requested flow includes both. Add those assertions with a fixture containing a co-performer.
await expect(setInfoPanel).toBeVisible();
await expect(setInfoPanel.getByText("Club Stage")).toBeVisible();
await expect(setInfoPanel.getByText(KIARA_SET_DESCRIPTION)).toBeVisible();
tests/e2e/link-wizard-candidate-verification.spec.ts:267
- Waiting for and checking this PATCH only proves the server accepted the request; it does not verify that the mutation's
onSuccessran and advanced the wizard. A client-side failure after a 2xx response could pass this test. Assert that theLink Wizard - Kiara Scuroheading is gone (or that the next step is shown) after the response.
page.getByRole("button", { name: /save & next/i }).click(),
]);
expect(patchResponse.ok()).toBe(true);
tests/e2e/link-wizard-candidate-verification.spec.ts:8
- The manual verification step in the PR description points to
/festivals/test/editions/2025/links, but the route exercised here is the admin-only/admin/festivals/test/editions/2025/links; the public path is not the Link Wizard route. Update the verification instructions so they exercise this spec.
const LINK_WIZARD_PATH = "/admin/festivals/test/editions/2025/links";
tests/e2e/link-wizard-candidate-verification.spec.ts:27
- The happy-path selection also stages the Rising candidate's image because Kiara starts with no image, but cleanup only clears
spotify_url. The test therefore leavesimage_urlpersisted in the shared database and can contaminate later tests; reset the image field here as well.
body: JSON.stringify({ spotify_url: null }),
- Files reviewed: 3/4 changed files
- Comments generated: 1
- Review effort level: Lite
Mobile Safari is also WebKit-engine-based, so it hits the same page.route() vs. active-service-worker gap fixed for the webkit project in 95dbedf — link-wizard-candidate-verification.spec.ts was still failing on Mobile Safari in CI for the identical reason. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Reorder MOCK_CANDIDATES so it's not pre-sorted by followers, making the descending-follower-order assertions a real regression guard. - Reset image_url (in addition to spotify_url) in afterEach, since "Select all" on the Rising candidate also stages/persists an image. - Add data-testid to ArtistSetInfoPanel's and CandidateCard's Card instead of matching on Tailwind utility classes, which break silently on styling changes. - Assert the set-info panel's rendered time range and the seed-driven "No other co-performers" state, per #349's acceptance criteria. - Use the real `Page` type import instead of inline `import("@playwright/test").Page` expressions. - Move helper functions below test.describe, matching this repo's function-declarations-after-return convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The E2E spec has two unresolved moderate issues: missing set-title coverage and unchecked teardown reset failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
playwright.config.ts:89
- The app enables the PWA service worker in both development and preview, but this workaround is applied only to the WebKit projects. Playwright route interception can be bypassed for requests handled by an active service worker in the unblocked Chromium/Firefox projects, so this spec may call the real edge function instead of receiving the controlled dataset. Apply
serviceWorkers: "block"in the shareduseconfiguration (or to every project running this spec), rather than only Safari.
// WebKit can't intercept requests that pass through an active Service
// Worker (page.route() silently misses them, unlike Chromium's
// CDP-based interception) — the app's PWA service worker was letting
// mocked API calls (e.g. search-artist-links) fall through to the real
// network. Blocking service workers for this project keeps route()
// mocks reliable.
use: { ...devices["Desktop Safari"], serviceWorkers: "block" },
tests/e2e/link-wizard-candidate-verification.spec.ts:10
- This spec mutates the fixed seeded Kiara row, but
playwright.config.tsruns this file in fully-parallel browser projects (and the two tests can also overlap).afterEachcleanup is not synchronization: one project can load Kiara after another has saved Spotify, causing the candidates panel to be omitted, or reset the row while another test is using it. Use an isolated record per project/test (or mock the artist write/read) rather than sharing this row.
const KIARA_ARTIST_ID = "a3333333-3333-3333-3333-333333333333";
tests/e2e/link-wizard-candidate-verification.spec.ts:234
signInAsAdminuses the sametest@example.comfor every test instance. This spec runs across all browser projects while local tests are fully parallel, so concurrent OTP requests share one Mailpit inbox;fetchOtpCodecan return another context's newer code and cause intermittent sign-in failures. Use isolated pre-onboarded admin identities/contexts or serialize all consumers of this seeded account.
await submitOtpSignIn(page, TEST_CONFIG.SEEDED_ONBOARDED_USER_EMAIL);
tests/e2e/link-wizard-candidate-verification.spec.ts:159
- After expanding the list, the assertions only check the count and that Micro is present; they never verify the relative order of the two newly revealed candidates. A regression that sorts the initial three correctly but leaves the remaining response order (
MicrobeforeNiche) would pass this spec despite violating the required descending-follower ordering. Assert the complete five-card order.
await page.getByRole("button", { name: "Show more" }).click();
await expect(candidateCards).toHaveCount(5);
await expect(page.getByText("Test Artist Micro")).toBeVisible();
tests/e2e/link-wizard-candidate-verification.spec.ts:186
- This only waits for a successful PATCH and never checks the post-save wizard state, despite the flow's
onSuccess: onNextbehavior. A regression that sends the request but fails to advance (or leaves an error state afterward) would still pass; assert that the Kiara step is gone or the next step is visible after the mutation settles.
expect(patchResponse.ok()).toBe(true);
- Files reviewed: 5/6 changed files
- Comments generated: 2
- Review effort level: Lite
| // the formatted time range and the (seed-driven) co-performers state. | ||
| const setInfoPanel = page.getByTestId("artist-set-info-panel"); | ||
| await expect(setInfoPanel).toBeVisible(); | ||
| await expect(setInfoPanel.getByText("Club Stage")).toBeVisible(); |
Comment on lines
+20
to
+32
| await fetch( | ||
| `${TEST_CONFIG.SUPABASE_URL}/rest/v1/artists?id=eq.${KIARA_ARTIST_ID}`, | ||
| { | ||
| method: "PATCH", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| apikey: TEST_CONFIG.SUPABASE_SERVICE_ROLE_KEY, | ||
| Authorization: `Bearer ${TEST_CONFIG.SUPABASE_SERVICE_ROLE_KEY}`, | ||
| Prefer: "return=minimal", | ||
| }, | ||
| body: JSON.stringify({ spotify_url: null, image_url: null }), | ||
| }, | ||
| ); |
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.
Adds comprehensive e2e spec for the Link Wizard candidate verification flow with mocked edge function responses, testing candidate sorting, description display, link-out isolation, show-more pagination, and full selection+save workflow.
Verification
/admin/festivals/test/editions/2025/linkswith local test Supabase and run the specCloses #349
Generated by Claude Code