Skip to content

Match the assign screen title by text, not by heading role - #1415

Merged
davidleomay merged 4 commits into
developfrom
fix/e2e-assign-title-locator
Aug 20, 2026
Merged

Match the assign screen title by text, not by heading role#1415
davidleomay merged 4 commits into
developfrom
fix/e2e-assign-title-locator

Conversation

@davidleomay

@davidleomay davidleomay commented Aug 20, 2026

Copy link
Copy Markdown
Member

Fixes the last failing check on the DFXswiss/backend release
#5090, which is blocked by this test.

The guest assign test added in #1407 asserts the screen title with getByRole('heading', …). That
locator never matches. The title comes from useLayoutOptions and renders in the app bar as a plain
element — the Playwright trace from the failing run shows it as generic [ref=e10]: Assign transaction, with no heading role on the page at all.

The feature is not broken. The same trace shows the guest form rendering correctly: title, the
single buy target preselected, and the submit button.

- generic [ref=e5]:
    - button [ref=e6]                            # back
    - generic [ref=e10]: Assign transaction      # app-bar title — no heading role
    - img [ref=e13]
- generic [ref=e20]:
    - paragraph [ref=e21]: Remittance info
    - button "1C32-386F-20BE Ethereum/ETH 0xc6…" [disabled]
    - button "Assign transaction" [ref=e30]      # submit — same accessible name

The fix

getByText, which is what the other title assertions in this file already use (Transaction status at 467/495,
Transaction refund at 733/796).

first() is required because the submit button carries the same accessible name, so the text locator
matches twice — the app bar precedes the form in the DOM, so first() is the title. Without it the
assertion trades one failure for a strict-mode violation.

The negative assertion on the following line stays role-based deliberately: Your Transactions is a
real in-page heading, as the passing list tests at lines 48, 110 and 179 demonstrate, so
getByRole('heading', …).not.toBeVisible() keeps its meaning there.

Verification

Diagnosed from the e2e-stack-report artifact of the failing run (job 96361851800): the trace's DOM
snapshot is what establishes both that the title has no heading role and that the button shares the
name. I did not run the harness locally — it needs the full compose stack — so the locator change
itself is verified by CI on this PR rather than by a local run.


The route-coverage gate this unmasked

Fixing the locator let the [coverage-gate] project run for the first time — it is skipped while any
earlier spec fails, which is why release #5090 never reported it. It then failed:

Routes claimed but never opened (1) — the claiming suite must navigate there:
  - /tx/:id/refund

#1407 added /tx/:id/refund to App.tsx and claimed it in specs/registry/transactions.ts, but every
refund test navigates the guest variant /tx/:id/:secret/refund. The screen opens on either input —
isRefund = ... && (hasActionSecret || isLoggedIn) in transaction.screen.tsx — so the logged-in half
of that condition was untested and the claimed route never visited.

route-coverage.spec.ts states: "Do not weaken or remove a claim just to keep this green; add real
coverage instead." So this adds the missing navigation rather than dropping the claim: open the route
with a session and no secret, assert the refund form renders.

CI evidence for the first commit

The locator fix is confirmed by the run on this branch - the previously failing test passed, and the
serial-mode skips it was causing disappeared (232 -> 238 passing):

231 [chromium] specs/transactions.spec.ts:570:5 public uid assign route opens the guest assign form
    and assigns to the single buy target (2.8s)  PASSED
238 passed (9.9m)

The guest assign test added in #1407 asserts the screen title with
getByRole('heading'), which never matches: the title comes from
useLayoutOptions and renders in the app bar as a plain element. The
captured Playwright trace of the failing run shows it as
`generic [ref=e10]: Assign transaction`, with no heading role anywhere on
the page. The other title assertions in this file already use getByText
for exactly that reason.

The feature itself works — the same trace shows the guest form rendered,
the single buy target preselected and the submit button present, so this
was only ever a wrong locator.

getByText matches twice here, because the submit button carries the same
accessible name (`button "Assign transaction" [ref=e30]`); the app bar
precedes the form in the DOM, so first() takes the title. The negative
assertion on the next line stays role-based: 'Your Transactions' is a
real in-page heading, as the passing list tests at lines 48, 110 and 179
show, so it keeps its meaning.

This blocks the DFXswiss/backend release, where it is the last failing
check.
Fixing the assign locator let the [coverage-gate] project run for the
first time on this branch — it is skipped while any earlier spec fails,
which is why #5090 never reported it. It then failed:

  Routes claimed but never opened (1) — the claiming suite must navigate
  there:  - /tx/:id/refund

#1407 added /tx/:id/refund to App.tsx and claimed it in
specs/registry/transactions.ts, but every refund test navigates the
guest variant /tx/:id/:secret/refund. The screen opens on either input —
`isRefund = ... && (hasActionSecret || isLoggedIn)` — so the logged-in
half was untested and the claimed route never visited.

route-coverage.spec.ts says not to weaken or remove a claim to keep the
gate green, so this adds the missing navigation as real coverage: open
the route with a session and no secret, and assert the refund form
renders.
Codex conformance review: the comment explained first() via "the smallest
element whose own text nodes match". That is the legacy text= engine's
model and is not what makes ancestors safe here. Exact matching resolves
to the smallest element whose text is exactly the string, and a wrapping
container is never the smallest match — that is the reason, and the
comment now says it.
@davidleomay
davidleomay marked this pull request as ready for review August 20, 2026 13:05
@davidleomay

Copy link
Copy Markdown
Member Author

Two review passes — Codex logic, Codex quality, conformance — three findings in the first, none in the second. All three were overclaims of mine rather than defects in the fix.

The comment explained first() with the wrong model. It said exact getByText matches "the smallest element whose own text nodes match". That is the legacy text= engine's description and is not what makes ancestors safe. Exact matching resolves to the smallest element whose text is the string, and a wrapping container is never the smallest match — that is the actual reason, and the comment now says it.

Two stale references in the description. It cited Transaction refund at 699/762, which were pre-change line numbers; they are 733/796, and 699 is now userDataId. It also framed the work as two commits when the branch had three.

Worth recording because it nearly shipped: the first version of this fix would have failed differently. Plain getByText matches twice here — the app-bar title and the identically named submit button — and would have traded "element not found" for a strict-mode violation. What caught it was reading the trace's accessibility tree rather than reasoning about the markup, which is also what established that the title is a generic with no heading role at all.

The second commit exists because fixing the locator let the [coverage-gate] project run for the first time — it is skipped while any earlier spec fails, which is why release #5090 has never reported it. It then failed on /tx/:id/refund: claimed in specs/registry/transactions.ts by #1407, but every refund test navigates the :secret variant, leaving the isLoggedIn half of isRefund = … && (hasActionSecret || isLoggedIn) untested. route-coverage.spec.ts says not to weaken a claim to stay green, so this adds the missing navigation instead.

Verified by CI, not locally — the harness needs a compose stack I cannot run, and I want that limitation on the record rather than implied away. The run is green with both tests named: ✓ 231 public uid assign route … and ✓ 233 logged-in refund route …, 240 passed, zero failures including the coverage gate.

One judged non-fix: the title assertion does not isolate the title — if the app bar lost it, first() would fall through to the submit button and still pass. The line below it (Your Transactions heading absent) plus the button and URL assertions already establish that the guest assign screen opened, and scoping to the app bar would need a selector the external components package does not expose.

@davidleomay
davidleomay merged commit b4f4865 into develop Aug 20, 2026
10 of 11 checks passed
@davidleomay
davidleomay deleted the fix/e2e-assign-title-locator branch August 20, 2026 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant