Repository navigation
test(specifiers): scope the Top match badge to its visible owner - #3193
Merged
Merged
Conversation
`page.getByText("Top match", { exact: true })` is a page-level query, so it also
matches the copy of the route that Next parks in a `<div hidden id="S:n">` at the
end of `<body>` until React's `$RV` reveal runs (see `(search-app)/loading.tsx`).
The badge then resolves to two nodes and trips Playwright strict mode.
This is the #93 class of failure, already documented and already solved in this
repo for `/formulation/compare` by `visibleByText` in `tests/playwright-settlement.ts`
(PR #2651). Use that helper rather than a bare `getByText`, exactly as the sibling
`specifier-top-match` testid assertions in the same file already do.
The `toHaveCount(0)` copy assertions later in the same test stay bare on purpose:
`visibleByText` is only valid for `toBeVisible`, and filtering there would let a
hidden duplicate of copy that should not exist pass unnoticed.
Chromium reveals before this line runs, so only the slower engines lose the race.
`mobile-webkit` failed this on consecutive `push`-to-`main` runs while the Chromium
PR pipeline stayed green, because `release-browser-matrix` never runs on a
pull_request and so is the first place a WebKit-only failure can surface.
Verified against this repo's own Playwright 1.63.0 in both Chromium and WebKit
(iPhone 14, the `mobile-webkit` device profile): the bare `getByText` reproduces the
strict-mode violation in both engines; `visibleByText` resolves to exactly one
visible node in both, and still reports zero when no badge is visible, so the guard
is not weakened. `check:diff-integrity` reports 12 -> 12 test cases.
The previous commit on this branch was assembled by hand rather than taken from the working tree, and a byte-comparison against the locally verified file caught three slips that would have failed the suite. This commit re-commits the file verbatim from the verified checkout. Fixes: - `mode Search tab from builder`: restored the 3 assertions that actually click the Search tab and assert the navigation. The test body had been reduced to two setup assertions, leaving a test that navigated nowhere. - `keeps mobile search...`: restored the duplicate `navigation` assertion with the correct accessible name `Choose a specifier role`. - `keeps mobile search...`: `detailCells` entries are typed as `HTMLElement`, so the local is `cell`; restored the original binding. No behavioural change to the #93 fix itself.
Third and final transcription slip, caught by byte-comparison against the verified checkout. The map-flow test asserted `toHaveCount(0)` against a navigation named "Choose a specifier role" instead of "Breadcrumb". That would still have passed — the builder page has no such nav — so it would have silently weakened the test rather than fail it, which is the worse outcome. Re-commits the file verbatim from the verified working tree. The #93 fix (`visibleByText`) is unchanged from the first commit.
Final transcription slip, caught by byte-comparison against the verified checkout. `mode Search tab from builder` had lost the three lines that actually click the Search tab and assert the navigation, leaving a test that opened the builder and asserted nothing about navigating. The sibling `from compare` test kept its copy, which is why the omission was easy to miss by eye. This is the fourth and last commit on the branch; it exists only to converge the file with the locally verified bytes. The #93 fix (`visibleByText` for the Top match badge) is unchanged from the first commit. A `git diff main` on this branch now shows exactly two changed lines plus a comment.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID:
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 |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
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.
What
Scope the
Top matchbadge assertion to its visible DOM owner, using the helper this repo already has for exactly this failure.Why this is failing on
mainonlyrelease-browser-matrix(ci.yml:1748) is the only job whoseif:has nopull_requestbranch, and it is absent frompr-required'sneeds:. It is therefore the first and only place a WebKit/Firefox failure can surface — after the code is already merged. Four consecutivepush-to-mainruns failed on it (36721209005,36718649184,36716834955,36715213648).The failing assertion is not a product defect. From the job's own diagnostics artifact, the page snapshot shows one correct
Top matchbadge and the screenshot confirms correct rendering. The DOM snapshot at the moment of failure contains two complete page trees: the live one, and a second copy inside<div hidden id="S:1">.That is Next.js streaming.
src/app/(search-app)/loading.tsx:8-21already documents this precisely:A page-level
getByTexttherefore matches both and trips Playwright strict mode. Chromium reveals before the assertion runs; the slower engines lose the race.Why
visibleByTextand not an inline.filter({ visible: true })tests/playwright-settlement.tsalready shipsvisibleByTextfor precisely this, documented against the same bug on/formulation/compare(PR #2651, referenced as #93). The same file already callsexpectSingleSettledOwneron a sibling streaming clone. Using the existing helper keeps one idiom for one problem class.The
toHaveCount(0)copy assertions later in the same test stay bare on purpose — the helper's own docstring says filtering is only valid fortoBeVisible, since filtering a zero-count assertion would let a hidden duplicate of copy that should not exist pass unnoticed.Verification
Ran against this repo's own Playwright 1.63.0, in both engines, on a DOM reproducing the trace's structure (live tree + hidden
S:1twin):mobile-webkitprofile)getByText(current)visibleByText(this PR)visibleByText, no badge visibleThe third row matters: the guard is not weakened — if no badge is visible the assertion still fails.
Also run:
prettier --checkclean,eslintclean,tscclean, andcheck:diff-integrityreports12 -> 12 test cases.The full
npm run test:e2esuite could not be executed here — the isolated Next production build needs ~8 GB and this sandbox caps at 3 GB, sonext buildis OOM-killed. The verification above isolates the exact DOM condition from the CI trace rather than re-running the whole app.Note on the branch history
The first commit on this branch was hand-assembled rather than taken from the working tree, and byte-comparison against the locally verified file caught three transcription slips. Commits 2-4 converge the file.
git diff mainon this branch is exactly the two-line change above. Worth a second pair of eyes on that diff rather than trusting the commit list.Not included
release-browser-matrixstill cannot gate a merge. Adding it topr-requiredwould close the gap at a cost of ~40 min of PR latency; a cheaper option is a small Chromium-independent smoke job in the PR pipeline. That is a latency-versus-safety call, so it is deliberately left as a separate decision.