test(e2e): wait for the workspace, not the sidebar row, after a project switch (#290) - #343
Merged
Merged
Conversation
…ct switch (#290) `terminal-scrollback-nav.e2e.ts` failed roughly one run in six inside `npm run gate`, and never on the assertion it exists for. It died one step earlier, at `expect(termN).toBeVisible()` with "element(s) not found" — the terminal panel simply not there after switching back to its project. That is not a restore race in the product. It is the spec reading a panel id out of a DOM that is still showing the project it just left. Opening a project is optimistic on one side and awaited on the other. `projects-store.tsx` calls `setOpenedId(id)` synchronously, before any await, so `.project-item[data-active]` flips on the click. `workspace-store.tsx` then awaits `client.load(projectId)` and deliberately does NOT clear `layout` while that is in flight, because blanking the pane on every switch was a visible flash. So for one RPC the sidebar names the new project while the DOM still holds the old project's TabGroup, and `firstPanelId()` returns a panel belonging to the outgoing project. `terminal-<that id>` can never exist, so the next locator waits out its whole budget and fails — looking for all the world like a slow app or a panel that did not come back. Auto-waiting cannot save this. `firstPanelId()` uses `.evaluate()`, which waits for a `.panel-box` to exist, and one already does. Playwright resolves "not yet rendered"; it cannot resolve "rendered, but stale". MEASURED RATHER THAN REASONED ABOUT. A probe switched back and forth 150 times on an idle machine and read the pane id and the panel id in a single pass: stale workspace-pane reads 3 / 150 wrong firstPanelId, old wait 2 / 150 wrong firstPanelId, new wait 0 / 150 The gap between 3 and 2 is the whole reason this presented as rare and load-dependent: on the third trip the window was open, but `firstPanelId`'s own round trip was enough for the layout to land. Every extra hop between the click and the read shrinks the failure rate without closing anything. The fix uses a signal that already existed one attribute away. `app.tsx` stamps `data-project={layout.projectId}` on `workspace-pane`, from the very state whose arrival mounts the new panels — same commit — so when it reads the project we asked for, the panels on screen are that project's by construction. `switchProject()` and `settleOnActiveProject()` in the harness wait on it; `createProject` now does too, since its own comment already described this trap while waiting on the weaker signal. Also fixed at two more sites found by the same reading: - terminal-command-memory.e2e.ts clicked a project row and read a panel id on the very next line, with no wait at all. - terminal-reload-mode.e2e.ts had noticed the hazard and grown its own copy of the `data-active` wait. It defers to the helper now, so the rule lives in one place. Stressed after the change: the three touched files 11/11, and the #290 test 12/12 at --repeat-each=12 with retries off.
…ally `docs/testing.md`'s "Writing a test that does not flake" already carries two siblings — #321, where the wait lands upstream of the write, and #335, where two writes on independent channels never had an ordering. #290 is the third and needed saying separately, because in this one the signal is on the right channel and does mean what it says. It is simply set BEFORE the work happens, on purpose, so the UI does not feel slow. Records what generalises. The right signal already existed one attribute away, and a test that waits on the optimistic half of an optimistic update is waiting on the request rather than the result. Playwright's auto-waiting does not rescue it, because the wrong answer is already in the DOM. And measuring the WINDOW — 150 switches, both ids read in one pass, 3 open windows and 2 actually-wrong reads — is a better artifact than a flake rate, since it names the mechanism instead of sampling its consequences, and costs twenty seconds instead of six full spec runs. Also corrects a load-bearing comment in `use-terminal.ts` that #290 flagged as untrue. The same-grid branch of `conformGrid` claimed its `term.refresh(0, rows - 1)` gives a never-rendered terminal its viewport scroll area "so the wheel works from the first frame". Read against the installed xterm 6.0.0, it cannot: `Terminal.refresh` is `this._renderService?.refreshRows(a, b)` and nothing more, while the viewport syncs its scroll area from `queueSync()`, subscribed to exactly three things — `_bufferService.onResize`, `buffers.onBufferActivate` (a normal/alternate switch), and the scroll events. A repaint is none of them, and `Terminal.resize()` early-returns when neither dimension changed, so a same-size resize is no way in either. That matches #290 from the other end: a window resize recovers a dead viewport every time, more output sometimes does, and Ctrl+F5 never does — and Ctrl+F5 issues this exact call. The call is KEPT and only the claim corrected. It still costs one render of an already-correct grid, it is what puts a first frame on a panel whose container was not measurable at `open()`, and removing it would be a production change that #290 has no reproduction to justify. The third sync source, `onBufferActivate`, is new to the record here — the earlier reading in the issue named only two.
10 tasks
2 tasks
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
1 participant
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.
Linked issue
Related to #290 — deliberately not
Closes.#290's reported defect ("terminal scrolling dies after a project switch until the window is resized")
is not fixed here and the issue stays open. What this PR fixes is the flake that #290's own last
comment recorded against the spec written for it, plus one load-bearing comment that issue flagged as
untrue. Both belong to #290 and neither closes it. See What this does not do below, which is the
part worth reading.
for [Bug] Terminal scrolling dies after a project switch until the window is resized #290 to be worked next.
bug, plusarea:terminalandarea:projects; title reads[Bug] Terminal scrolling dies after a project switch until the window is resized.Intent (what & why, in human terms)
packages/ui/tests/e2e/terminal-scrollback-nav.e2e.ts— "scrollback still scrolls after a projectswitch and back (#290)" — failed roughly one run in six inside
npm run gate, and the strictflake gate correctly turned that into a red run. A test that reddens the gate at random is a test
people learn to re-run past, and this repo deleted its register of known-failing specs precisely so
that habit could not form.
It also mattered which test: it is the spec written to pin #290, so a reader hitting its red had to
decide whether they had reproduced the defect or hit noise — with no way to tell.
Outcomes (what changed, in human terms)
No user-visible behaviour changed, and no production behaviour changed. One production comment
is corrected; the code under it is byte-for-byte the same.
The flake was the spec, not the product
The failure was never on the scroll assertion the test exists for. It was one step earlier:
The terminal panel simply not there after switching back — which reads like a restore race in the
product, and is not one.
Opening a project is optimistic on one side and awaited on the other:
Not clearing
layoutis the right call — blanking the pane on every switch was a visible flash — butit leaves a window, one RPC long, in which the sidebar names the new project while the DOM still
holds the old project's
<TabGroup/>. The spec waited on the sidebar, then read a panel id, and gota panel belonging to the project it had just left.
terminal-<that id>can never exist, so the nextlocator waits out its whole budget and fails.
Auto-waiting does not rescue this:
firstPanelId()uses.evaluate(), which waits for a.panel-boxto exist, and one already does. Playwright resolves "not yet rendered"; it cannotresolve "rendered, but stale".
Measured, not inferred
A probe switched between two projects 150 times on an idle machine and read the pane id and the
panel id in a single pass:
workspace-panestill on B while the row already said A — the window itselffirstPanelIdreturned B's panel — old waitfirstPanelIdreturned B's panel — new waitThe gap between 3 and 2 is the whole reason this presented as rare and load-dependent: on one trip the
window was open, but
firstPanelId's own round trip was enough for the layout to land. Every extrahop between the click and the read shrinks the failure rate without closing anything.
This is a better artifact than "1 failure in 6 runs" — it names the mechanism instead of sampling its
consequences, and it cost twenty seconds instead of six full spec runs.
The fix was one attribute away
app.tsxalready stampsdata-project={layout.projectId}onworkspace-pane, from the very statewhose arrival mounts the new panels — same commit. When it reads the project we asked for, the panels
on screen are that project's by construction.
switchProject()/settleOnActiveProject()added to the harness and wait on it.createProjectwaits on it too — its own comment already described this exact trap ("a stale idread a few milliseconds too early") while waiting on the weaker signal.
terminal-command-memory.e2e.tsclicked a row and reada panel id on the very next line with no wait at all;
terminal-reload-mode.e2e.tshad noticedthe hazard and grown its own copy of the
data-activewait, and now defers to the helper so therule lives in one place.
One comment corrected, no code changed with it
#290 flagged
conformGrid's same-grid branch as claiming something its call cannot do. Read againstthe installed xterm 6.0.0, that is right:
Terminal.refresh(a, b)isthis._renderService?.refreshRows(a, b)— rows, nothing else.queueSync(), subscribed to exactly three things:_bufferService.onResize,buffers.onBufferActivate(a normal↔alternate switch), and the scrollevents. A repaint is none of them.
Terminal.resize()early-returns when neither dimension changed, so a same-size resize is no way ineither.
Which matches #290 from the other end: a resize recovers a dead viewport every time, more output
sometimes does, and
Ctrl+F5never does — andCtrl+F5issues this exact call.The call is kept and only the claim corrected. It still costs one render of an already-correct
grid and puts a first frame on a panel whose container was not measurable at
open(); removing it isa production change #290 has no reproduction to justify.
onBufferActivateas a third sync source isnew to the record — the earlier reading in the issue named only two.
What this does not do
Issue #290 remains open and unfixed by this PR. The reported defect — every scroll route dying at
once after a project switch, recovered only by a resize — is still unreproduced, and nothing in this
diff addresses it.
Two things were established that narrow it, and both are posted to the issue:
programOwnsKeyboardreading both remain live. The negative resultrecorded in the issue tested the first, not the second.
MAX_SCROLLBACKis 64 KiB (terminal-service.ts:48), and replay is what re-parses a program'skeyboard negotiation into a rebuilt view. A busy session passes 64 KiB easily, so the negotiation
ages out of the replayed tail — which is exactly the condition the stale-flag hypothesis needs, and
it makes that hypothesis materially more plausible than it currently reads.
The diagnostic that settles it still needs the fault in front of a user:
window.__throngTerminalDiagnostics(), readingprogramOwnsKeyboardwhile a plain shell prompt is onscreen.
One unrelated flake found on the way, filed as #342
The first full gate on this branch went red at E2E — not on anything here.
terminal-start-failure-controls.e2e.ts:449failed and passed on retry, timing out the 30slayout-readability poll that #246 itself added. It is load-dependent, and it was excluded as a
consequence of this branch rather than assumed innocent:
harness.tswith this branch's change,--repeat-each=9harness.tsreverted to master,--repeat-each=9Identical, wall-clock included. Immediately after a full gate, on a hot machine, the same spec
reproduces at about 1 in 3; once the machine settles it does not reproduce in 18 consecutive runs.
Filed as #342 with the measurements. Not fixed here — it is a different spec, a different
mechanism, and it wants a budget derivation rather than a bigger magic number.
Testing (observed passing — paste/attach evidence)
npm run test:unit— greennpm run test:integration— greennpm run test:contract— greennpm run test:e2e— green@admin/ elevation-dependent behaviour verified under an elevated run, if applicable — n/a,nothing here depends on elevation.
Test output
Full
npm run gate— all eight stages, no flaky retries (205 parallel + 353 serial E2E tests):The three touched specs, and the #290 test stressed on its own because a flake fix that is not
stressed is not a fix:
Documentation
README.mdreflects the current finite state of the app — n/a, no user-facing behaviour,setup, architecture or capability changed.
docs/guides updated —docs/testing.md's "Writing a test that does not flake" gainsthis race class as the third sibling of the [Bug] navigation-remember flakes on its own anti-vacuity check under a full serial run #321 and [Bug] editor-update-listener's auto-save-timer control flakes on CI #335 entries. Those two are about picking a
signal upstream of the write, or on the wrong channel; this one is about a signal on the right
channel that is written optimistically, before the work happens, on purpose.
CONTRIBUTING.md— n/a, no process, toolchain, testing bar or setup change.Constitution compliance
configuration (X) are respected — test-layer plus one comment; no production seam added. The DRY
point is the substantive one: the switch-and-settle rule existed in three partial copies across the
suite and now exists once, in the harness.
Toolchain & review
Claude Opus 4.8 or a more capable model. Model used:
claude-opus-5AI Code -> Spec Kit artifacts
Spec directory: none — an ad-hoc defect fix in the test suite, so the branch carries no
Ssegment and no
specs/NNN-*/directory is created. The checklist below is n/a for that reason ratherthan skipped.
spec.mdis fully specified — n/a/speckit-clarifywas run — n/a/speckit-planproducedplan.md,research.md,data-model.md,contracts/— n/a/speckit-tasksproduced a dependency-orderedtasks.md— n/a/speckit-analyzewas run — n/a/speckit-superpowers-bridge— n/a/speckit-convergewas run — n/a150-trip probe above plus the full gate.
Licensing