Skip to content

Fix the two Playwright e2e tests that fail on main - #67

Merged
kudzuweb merged 2 commits into
mainfrom
27-fix-the-two-playwright-e2e-tests-that-fail-on-main
Sep 6, 2026
Merged

kudzuweb merged 2 commits into
mainfrom
27-fix-the-two-playwright-e2e-tests-that-fail-on-main

Conversation

@kudzuweb

@kudzuweb kudzuweb commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Both tests had been failing since upstream refactors moved the elements they query, so neither was reaching the geometry it asserts. Neither is deleted and no assertion is loosened.

Closes #27

The storyboard test queried .homepage-workflow-scene-copy, a class commit 5ec16a2 dropped when it moved homepage styles to Tailwind, so it threw before any measurement; the hook is restored as a data-testid and the test points at it. The stale-write test queried document-status-stack, which commit 2a9c2e2 emptied when it moved the save status into a new top-left corner. Retargeting it at that corner made it fail on real geometry: at 390px the conflict banner spans y 12-240 directly over the save status. That is a genuine layout defect, so the fix is in the layout, with one shared top-offset constant between the two fixed status affordances, not in the assertion.

Verification: full pnpm test:e2e green three consecutive runs at 48 passed; pnpm check passes.

One variance: criticmarkup-review.spec.ts:401 failed once under parallel load, then passed alone and on three full-suite reruns. That spec references nothing this branch touches; it was not bisected against a clean main.

The storyboard mobile overlap test queries .homepage-workflow-scene-copy,
a class 5ec16a2 dropped when it moved the homepage styles to Tailwind
utilities. The test threw before reaching its geometry assertions. Add a
data-testid to the scene copy wrapper, matching the repo selector
convention, and point the test at it.
2a9c2e2 moved the save status indicator out of document-status-stack
into a fixed top-left corner, but did not carry over the offset that
drops the stack below a top notice. On a 390px viewport the conflict
banner spans x 8-382 and y 12-240, covering the corner at x 12-40,
y 12-40.

Share one top-offset class between both fixed status affordances so they
cannot drift again. Retarget the overlap test at the save status corner,
which is where the save status now lives; document-status-stack holds
only the review handoff button and watcher notice, neither of which
renders without a watcher, so it was empty and reported hidden.
@kudzuweb kudzuweb added the agent Work an agent may create, claim or execute label Sep 6, 2026
@kudzuweb kudzuweb linked an issue Sep 6, 2026 that may be closed by this pull request
@kudzuweb

kudzuweb commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

[orchestrator e2fcf357] Build record, as reported by build-27.

What the issue planned

Make the two Playwright tests that fail on main pass, without loosening any assertion.
Delete a test only if it asserts behavior the app no longer has by decision, and justify that
with a recorded decision. Full pnpm test:e2e green on isolated ports.

What was built

Both failures were stale test hooks left behind by upstream refactors, not tests that
encode a superseded decision. Neither test was deleted and no assertion was weakened. One of
the two also uncovered a real layout defect, which is fixed in the app.

Test 1 — homepage-storyboard.spec.ts "docks the storyboard visual without mobile
overlap": the app was wrong, in that it had dropped the hook the test queries.

The test looked up the scene copy wrapper as .homepage-workflow-scene-copy. That class
appears nowhere in packages/app/src (verified: grep -rn "homepage-workflow-scene-copy" packages/app/src packages/app/e2e matched only the spec). Commit 5ec16a2 "fix: keep review
rail cards visible (#75)", Nathan Baschez, 2026-05-12, moved the homepage styles from named
classes to Tailwind utilities and rewrote <div className="homepage-workflow-scene-copy"> as
an unnamed utility div (verified by reading git show 5ec16a2 -- packages/app/src/App.tsx).
It kept the sibling hooks the tests used, such as homepage-workflow-scene and
homepage-workflow-terminal, and missed this one. The querySelector then returned null and
the test threw Expected sticky visual, scene copy, and document preview before reaching a
single geometry assertion.

The fix restores the hook in the repo's own convention: data-testid="homepage-workflow-scene-copy"
on the scene copy wrapper in packages/app/src/App.tsx:604, and the spec queries that testid.
The app's mobile layout was never broken; every geometry assertion in the test passes
unchanged once it can reach them.

Why pnpm test:selectors did not catch the class selector: scripts/check-test-selectors.mjs
matches querySelector("...") on a single line, and Biome had wrapped this call across three
lines, so the regex never saw it. Left as is; widening that script is outside this issue.

Test 2 — stale-write.spec.ts "keeps conflict banner and save status stack from
overlapping": the test pointed at the wrong element, and the app has a real overlap bug.

Two separate problems, both traced to the same upstream commit.

The stale selector: when the test was written in 8249a6c "feat: show document save state and
flush cmd+s saves (#70)", 2026-05-07, document-status-stack contained the
DocumentSaveStatusIndicator, gated only on documentPage (verified in that commit's diff).
Commit 2a9c2e2 "Improve document status and handoff affordances (Lex-Inc#124)", Nathan Baschez,
2026-06-14, moved the indicator into a new fixed corner, document-save-status-corner, at top
left. What remains in document-status-stack is the review handoff button and the review
watcher notice. Both need a review watcher, and the test aborts the events route and opens the
file with no agent watching, so the stack renders empty, has zero area, and Playwright reports
it hidden. That is the toBeVisible failure seen on main.

The layout defect: the same commit gave the new corner a bare fixed top-3 left-3 and did not
carry over the hasTopNotice offset that the status stack already had. The conflict banner is
fixed top-3 left-1/2 -translate-x-1/2 w-[min(calc(100vw-1rem),52rem)], so on a narrow
viewport it spans nearly the full width and sits directly on top of the save status. Measured
in the browser at both viewports the test uses:

Viewport Conflict banner box Save status corner box Overlaps
1280x720 x 224-1056, y 12-140 x 12-40, y 12-40 no
390x844 x 8-382, y 12-240 x 12-40, y 12-40 yes

Per the repo's Prove It pattern I retargeted the assertion first and watched it fail on the
geometry at 390px, then fixed the layout and watched it pass.

The fix hoists the offset both fixed status affordances need into one
fixedStatusTopClass in packages/app/src/DocumentWorkspace.tsx:845, and applies it to the
save status corner as well as the status stack. The class string is the stack's existing
hasTopNotice ? "top-[19rem] sm:top-[7rem]" : "top-3", unchanged, so the stack renders
byte-identically and only the corner moves. Sharing one constant is what stops the two
drifting apart again, which is exactly how this bug arrived.

The test is retargeted at document-save-status-corner and renamed to "keeps conflict banner
and save status corner from overlapping". The intersection maths, both viewports and every
other assertion are untouched. This is not a loosening: the test now measures the element that
actually carries the save status, where before it measured an empty container that could never
overlap anything.

Why the build differs from the issue

It does not differ in outcome. Two points worth flagging.

First, the issue's Files list named packages/app/playwright.config.ts, which needed no
change. Neither failure was a config, viewport or timing problem.

Second, the issue anticipated that a fix might be confined to the specs. Both fixes needed a
small app change: one restores a test hook the app had lost, the other fixes a genuine layout
defect the issue explicitly asked to be fixed in the layout rather than the assertion. The
App.tsx change is a single data-testid attribute, chosen to keep the conflict surface with
the parallel work on #41 as small as possible.

Tests added

None. This issue is about two existing tests. Both were repaired in place and neither was
deleted, so no decision record in docs/adr/ or docs/plans/ was required.

docs/spec/ui-state-screenshot-guide.md gains one row, "Save status under a top notice",
because the layout fix changes a visual state the guide is meant to cover. AGENTS.md requires
that update in the same change.

Left undone

Nothing in scope. Three things observed and deliberately not acted on, all outside this issue:

  1. scripts/check-test-selectors.mjs misses selectors whose querySelector call is wrapped
    across lines, which is why the stale class selector survived. Widening it would touch every
    spec in the repo.
  2. document-status-stack has no e2e coverage of its own populated state, because populating
    it needs a review watcher. Its emptiness here is correct app behavior, not a gap this issue
    should close.
  3. The shared sm:top-[7rem] offset does not fully clear the banner in the roughly 640px to
    848px width band, where the banner is still near-full-width and about 140px tall. Both
    status affordances have always had this, the test's viewports do not cover it, and picking a
    new offset is a design call this issue did not authorize. Flagged rather than changed.

@kudzuweb

kudzuweb commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

[orchestrator e2fcf357 overnight] Final build record for issue #27, replacing the earlier build-record comment on this PR. One measurement in that comment was wrong and is corrected here; the code is unchanged. Reviewer audit-pr-67 returned APPROVE with no blocking findings.

What the issue planned

Make the two Playwright tests that fail on main pass, without loosening any assertion.
Delete a test only if it asserts behavior the app no longer has by decision, and justify that
with a recorded decision. Full pnpm test:e2e green on isolated ports.

What was built

Both failures were stale test hooks left behind by upstream refactors, not tests that
encode a superseded decision. Neither test was deleted and no assertion was weakened. One of
the two also uncovered a real layout defect, which is fixed in the app.

Test 1 — homepage-storyboard.spec.ts "docks the storyboard visual without mobile
overlap": the app was wrong, in that it had dropped the hook the test queries.

The test looked up the scene copy wrapper as .homepage-workflow-scene-copy. That class
appears nowhere in packages/app/src (verified: grep -rn "homepage-workflow-scene-copy" packages/app/src packages/app/e2e matched only the spec). Commit 5ec16a2 "fix: keep review
rail cards visible (#75)", Nathan Baschez, 2026-05-12, moved the homepage styles from named
classes to Tailwind utilities and rewrote <div className="homepage-workflow-scene-copy"> as
an unnamed utility div (verified by reading git show 5ec16a2 -- packages/app/src/App.tsx).
It kept the sibling hooks the tests used, such as homepage-workflow-scene and
homepage-workflow-terminal, and missed this one. The querySelector then returned null and
the test threw Expected sticky visual, scene copy, and document preview before reaching a
single geometry assertion.

The fix restores the hook in the repo's own convention: data-testid="homepage-workflow-scene-copy"
on the scene copy wrapper in packages/app/src/App.tsx:604, and the spec queries that testid.
The app's mobile layout was never broken; every geometry assertion in the test passes
unchanged once it can reach them.

Why pnpm test:selectors did not catch the class selector: scripts/check-test-selectors.mjs
matches querySelector("...") on a single line, and Biome had wrapped this call across three
lines, so the regex never saw it. Left as is; widening that script is outside this issue.

Test 2 — stale-write.spec.ts "keeps conflict banner and save status stack from
overlapping": the test pointed at the wrong element, and the app has a real overlap bug.

Two separate problems, both traced to the same upstream commit.

The stale selector: when the test was written in 8249a6c "feat: show document save state and
flush cmd+s saves (#70)", 2026-05-07, document-status-stack contained the
DocumentSaveStatusIndicator, gated only on documentPage (verified in that commit's diff).
Commit 2a9c2e2 "Improve document status and handoff affordances (Lex-Inc#124)", Nathan Baschez,
2026-06-14, moved the indicator into a new fixed corner, document-save-status-corner, at top
left. What remains in document-status-stack is the review handoff button and the review
watcher notice. Both need a review watcher, and the test aborts the events route and opens the
file with no agent watching, so the stack renders empty, has zero area, and Playwright reports
it hidden. That is the toBeVisible failure seen on main.

The layout defect: the same commit gave the new corner a bare fixed top-3 left-3 and did not
carry over the hasTopNotice offset that the status stack already had. The conflict banner is
fixed top-3 left-1/2 -translate-x-1/2 w-[min(calc(100vw-1rem),52rem)], so on a narrow
viewport it spans nearly the full width and sits directly on top of the save status. Measured
in the browser at both viewports the test uses:

Viewport Conflict banner box Save status corner box Overlaps
1280x720 x 224-1056, y 12-140 x 12-40, y 12-40 no
390x844 x 8-382, y 12-240 x 12-40, y 12-40 yes

Per the repo's Prove It pattern I retargeted the assertion first and watched it fail on the
geometry at 390px, then fixed the layout and watched it pass.

The fix hoists the offset both fixed status affordances need into one
fixedStatusTopClass in packages/app/src/DocumentWorkspace.tsx:845, and applies it to the
save status corner as well as the status stack. The class string is the stack's existing
hasTopNotice ? "top-[19rem] sm:top-[7rem]" : "top-3", unchanged, so the stack renders
byte-identically and only the corner moves. Sharing one constant is what stops the two
drifting apart again, which is exactly how this bug arrived.

The test is retargeted at document-save-status-corner and renamed to "keeps conflict banner
and save status corner from overlapping". The intersection maths, both viewports and every
other assertion are untouched. This is not a loosening: the test now measures the element that
actually carries the save status, where before it measured an empty container that could never
overlap anything.

Why the build differs from the issue

It does not differ in outcome. Two points worth flagging.

First, the issue's Files list named packages/app/playwright.config.ts, which needed no
change. Neither failure was a config, viewport or timing problem.

Second, the issue anticipated that a fix might be confined to the specs. Both fixes needed a
small app change: one restores a test hook the app had lost, the other fixes a genuine layout
defect the issue explicitly asked to be fixed in the layout rather than the assertion. The
App.tsx change is a single data-testid attribute, chosen to keep the conflict surface with
the parallel work on #41 as small as possible.

Tests added

None. This issue is about two existing tests. Both were repaired in place and neither was
deleted, so no decision record in docs/adr/ or docs/plans/ was required.

docs/spec/ui-state-screenshot-guide.md gains one row, "Save status under a top notice",
because the layout fix changes a visual state the guide is meant to cover. AGENTS.md requires
that update in the same change.

Left undone

Nothing in scope. Three things observed and deliberately not acted on, all outside this issue:

  1. scripts/check-test-selectors.mjs misses selectors whose querySelector call is wrapped
    across lines, which is why the stale class selector survived. Widening it would touch every
    spec in the repo.
  2. document-status-stack has no e2e coverage of its own populated state, because populating
    it needs a review watcher. Its emptiness here is correct app behavior, not a gap this issue
    should close.
  3. The shared sm:top-[7rem] offset does not fully clear the banner between roughly 640px and
    900px. The reviewer re-measured this band, each box read twice 800ms apart with both
    readings identical, and the earlier estimate in this record was wrong in both numbers: the
    band runs to about 900px rather than 848px, and the banner is far taller than 140px through
    its lower half, reaching y 12-500 at 640px. At the corner's centre document.elementFromPoint
    returns the save status icon painting on top of file-conflict-notice. This is not a
    regression. Before this change the corner overlapped at every width from 360px to 900px, and
    the right-hand document-status-stack has carried the same insufficient offset all along;
    the change strictly improves it by removing the overlap below 640px, which is where the test
    measures. Picking a new offset is a design call this issue did not authorize, so it is
    flagged rather than changed, and it is now tracked as issue Clear the status affordances of the conflict banner between 640px and 900px #72.

Review outcome

audit-pr-67 verdict: APPROVE, no blocking findings, three non-blocking. It ran the full
pnpm test:e2e twice on isolated ports 4730/4731, 48 passed and 0 failed both times, and
reproduced the retargeted assertion's own intersection formula independently: true against the
unfixed layout at 390px, false after the fix. It confirmed by reading two existing unit tests
this PR does not touch that document-save-status renders inside document-save-status-corner
and not inside document-status-stack, so the retarget points at the element that genuinely
carries the save status rather than at whatever makes the test pass.

The three findings are resolved as follows. The wrong band measurement is corrected above and
the underlying layout gap is filed as issue #72, covering both status affordances. The fixed
save status sitting inside the document reading area below 640px is folded into that same
ticket as a placement question; nothing is covered there, since elementFromPoint under the
corner returns the scroll container. The criticmarkup-review.spec.ts:401 flake the builder saw
is issue #61 and not issue #62: the test at that line clicks review-handoff-button after
asserting only toBeVisible and never toBeEnabled, and isReviewHandoffDisabled returns true
whenever documentDiskChangeState is not clean, which is exactly the gap issue #61's fourth
acceptance criterion exists to close. The builder kept no failure text from that run, so this is
identification by mechanism rather than from the message; neither of the reviewer's two full
suite runs reproduced it.

Merged under Mauria's overnight merge grant of 2026-09-06, which authorizes the orchestrator to
play her role on the tickets: check that what was built makes sense in the broader context, then
either ask for changes or merge.

@kudzuweb
kudzuweb merged commit 80a81bd into main Sep 6, 2026
1 check failed
@kudzuweb
kudzuweb deleted the 27-fix-the-two-playwright-e2e-tests-that-fail-on-main branch September 6, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Work an agent may create, claim or execute

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix the two Playwright e2e tests that fail on main

1 participant