fix(desktop): land prompt-rail jumps at the top and fence the e2e sends - #4577
Conversation
cb8c1e4 to
83fe919
Compare
Astro-Han
changed the title
780425e to
4d07e78
Compare
github-actions
Bot
added
effort/M
19af944 to
1c7d8d7
Compare
Three Desktop e2e specs failed on CI with `element(s) not found` right after a send, and were green on rerun. Each pressed Enter after an intermediate signal — a mounted composer, a cleared draft, an updated model label — none of which says the shell will accept a submission. `awaitSendReady` waits for 发送 to be enabled instead — connections projected, no confirmed blocker, a draft to send, no send already in flight — and every send in these files goes through it. What the button cannot say is whether the readiness probe has answered: an unresolved snapshot is not a hard block, so Send stays enabled while the probe is in flight and `send()` awaits it again on its own. A slow probe is absorbed by the assertions below; a probe that comes back blocked still drops the submission with no feedback, and no test-side fence can close that. The assertions that observe the resulting Turn carry an explicit 20s, above the config-wide 10s default and below the 60s test timeout. In `accessibility-coverage.spec.ts` the fence goes after the Tab walk, not before it: a tooltip-carrying Astryx Button is disabled through `aria-disabled` and stays focusable, so `tabTo` reaches it either way and a fence placed earlier would only reopen the same window. Separately, `promptRailMotionWindow` gated readiness on `[data-turn-id]` alone. The transcript and the fixture attributes arrive on two unordered async paths — `runDeferredStartupRefreshes` fires `refreshSessions()` and `applyE2eFixture()` side by side, and only the second writes `data-maka-scroll-motion` once its `e2eFixture.getState()` IPC resolves — so a turn could paint before the document said anything about scroll motion, and the one-shot `page.evaluate` reading it saw `undefined`. One selector now requires both. Generated-by: Claude Code
The e2e config already sets `trace: 'retain-on-failure'` plus video and screenshot, but nothing collected the directory, so the evidence died with the runner: `gh api .../artifacts` is empty for every failing e2e run, which is why the mechanism behind a `transcript-scroll` failure could not be separated from the logs. Retention matches the diagnostic artifacts in `cli-package-validation.yml`. Generated-by: Claude Code
Clicking a rail tick for a prompt outside the resident range asks the shell to load it, and the shell answers a load request by publishing a scroll target. That target is the search reveal: `block: 'center'` with the app's scroll motion. So two writers aim at the same turn with different answers — the rail's instant `block: 'start'` jump, and a smooth centring animation a frame or two behind it. Traced on `chat-prompt-rail`, whose head is 110 turns outside the resident range: the rail lands turn 1 at the top of the scrollport, the centring scroll then walks the transcript back down over ~950ms and clamps at `scrollTop` 0, leaving the prompt 68px below the top. The rail's hold re-aims and wins that race most of the time, which is what made this look like flake rather than a bug; when the range arrives while the animation is already running, the hold sees a moving scroller every frame, never re-aims, and the prompt never reaches the top at all. The reveal is not redundant — it is also what records the reading position, so dropping it for rail navigations loses the anchor a session switch restores from. Only its alignment conflicts, and the transcript is where both facts meet: it aimed the turn and it consumes the reveal, so it reconciles them itself and the shell keeps publishing one kind of target. A reveal for the turn the rail is holding is instant and top-aligned; every other one, a search result included, is centred and animated as before. Navigating anywhere else drops the claim, so a click the shell ignored cannot re-aim a later search. Generated-by: Claude Code
`enterMainFromSkipLink` parks focus on `body` and asserts the skip link is within ten Tab presses of the document start. Parking is not a one-shot the renderer respects: the composer restores its draft caret with `getSelection().addRange(...)`, and a range set inside a `contenteditable` focuses it — so once per cold start, tens of milliseconds after the park and with no `focus()` call anywhere to fence on, focus lands in the composer. A walk that starts there has to run out the rest of the tab ring and wrap around, which is nine of the ten presses on a quiet surface and over budget as soon as the footer carries one more control. The restore fires once, so the park and the walk retry together rather than the budget growing to absorb the wrap-around. The budget is the assertion — it says the skip link is a few presses from the document start — and it stays where it was. Generated-by: Claude Code
Every other fixture window passes one; this one did not, so its renderer took the host's. Any test that reaches a control by its label — the send fence in `transcript-scroll.spec.ts:259` is the one that caught it — therefore passed on a Chinese desktop and failed on the English CI runner with `element(s) not found`. Generated-by: Claude Code
1c7d8d7 to
f0de24e
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I reviewed this at f0de24ee9b44234ea4b59c21a7f3f266915edd51. No P0 or P1. One P2, inlined.
The send flakes are a missing fence, not a product race: Enter was pressed on a mounted composer / cleared draft / updated model label, none of which means the shell will accept a submission. awaitSendReady waiting on 发送 enabled, plus 20s on the Turn assertions, is the right test cut. The product gate that would have disabled Send while the readiness probe was unresolved was correctly reverted — sendCurrent discards on sendBlocked, so that gate would have dropped cold-start Enter silently. The remaining silent-drop is #4573 item 3; a test cannot close it.
The one product change is real. A rail click for an unloaded prompt already aims block: 'start' and holds; openSessionInChat also publishes the search reveal (center + app scroll motion). Those two writers fighting is why the prompt landed 68px below the top. Reconciling in ChatView — the place that both aims and consumes the reveal — keeps one target kind in the shell. Same-turn rail claims are instant and top-aligned; search stays centred and animated. Claim is set in the same click as setSearchTarget, so the next render sees both. Navigating elsewhere drops the claim.
Accessibility: fencing Send after the Tab walk is right (aria-disabled keeps the button focusable). Naming zh on the prompt-rail worker is right. The skip-link retry is a test fence around a real product defect (caret restore via addRange steals focus on cold start). I am not filing that as a finding on this diff; it is called out as a separate fix, and this retry does not widen the ten-press budget.
P2: the title still says the product blocks sends until readiness is known. That gate is not in the diff. Rename to the rail-scroll fix and the e2e fences.
Bugfix plus tests. Hosted test was still running when I posted. I am not merging it.
简体中文
我审的是 f0de24ee9b44234ea4b59c21a7f3f266915edd51。没有 P0/P1。一条 P2，已行内。
发送 flake 是测试没围栏，不是产品竞态。awaitSendReady 等「发送」可点是对的。产品侧在 probe 未决时禁用发送被正确撤回了，否则冷启动 Enter 会被悄悄丢掉。
产品改动只有一处：未加载的提问轨点击和 search reveal 两个 writer 抢同一条 turn，收成 rail 认领时 instant + start。可及性：Tab 完再围栏、fixture 写明 zh，都对。skip link 重试盖的是既有 caret 抢焦点，不在本 diff 里开单。
P2：标题还在写「就绪前挡住发送」，门已经不在 diff 里。改标题。
hosted test 当时还在跑。我不合入。
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I reviewed exact head f0de24ee9b44234ea4b59c21a7f3f266915edd51. I found no P0 or P1. I am adding three P2 findings and one P3 finding inline.
The product rail fix closes the original two-writer race, but its hidden ownership marker survives the command it belongs to and changes a later search for the same Turn. Two test changes also leave the flake/accessibility contracts weaker than their comments claim: the new 20-second observation still sits below the product's supported 30-second first-send barrier, and retrying the whole skip-link assertion makes the known focus theft pass once it stops. The remaining P3 is another prompt-rail fixture that still inherits the host locale.
Local verification passed the full UI suite (327/327), the complete Desktop build and typecheck, renderer architecture, Biome, and all 31 affected Desktop E2E tests. A real Electron probe additionally reproduced the rail-then-search alignment issue. The current-main synthetic merge is clean and preserves the exact patch. Hosted Desktop E2E is green; the overall exact-head test job is still running its later steps. I am not merging this PR.
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I reviewed exact head f0de24ee9b44234ea4b59c21a7f3f266915edd51. No P0 or P1. One P2, below.
Everything else I found independently is already filed on this head and I agree with all of it: the title no longer describing the diff, the rail alignment claim outliving the command that set it, the 20-second assertions sitting under the product's 30-second first-send bound, the skip-link retry, and the second prompt-rail fixture that still takes the host locale. I am not restating any of it.
Two things I can add.
The exact-head run is now terminal green. Both earlier reviews were posted while test was still going. Run 33674595696 has since completed successfully on f0de24ee, so the head that was reviewed is the head that passed.
P2 — both deferrals this PR leans on are untracked, and merging erases the last record of them.
The description defers two known defects, and in each case points at work that does not exist:
- The caret restore that takes focus on cold start.
caretToContentEnd(packages/ui/src/composer.tsx:582) sets a range inside thecontenteditable, which focuses it, and the effect atpackages/ui/src/composer.tsx:731-739calls it with nofocus()of its own — exactly the mechanism the description gives. The description says this "is being fixed separately" and that the separate change "removes this retry as part of its own diff". I searched open issues and pull requests and there is no such work. #4579 changes which window takes the foreground, which is a different problem from which element takes focus. sendCurrentdiscarding a submission with no feedback. #4573 records it as "worth its own issue". That issue was never opened.
Merging removes the record of both at once, because the description says Closes #4573: the issue is the only written trace of item 2, and the skip-link retry is what took away CI's ability to surface item 1.
This is what decides how the skip-link retry should be read. As a fence held in front of a fix that is actually coming, it is reasonable and the ten-press budget survives intact. As a permanent one, it means a keyboard user losing the skip link at cold start is now a state the suite reports as healthy, with nothing anywhere that says it is still owed. The gap is small and cheap to close: open the two follow-ups and link them here, or downgrade Closes #4573 to a plain reference so the issue stays open on its unfinished item.
I read this at source level and did not run the suite on my side, so the passing evidence here is the hosted run above and the local runs reported by the other reviewers, not mine.
Approving. The diff is correct, the flakes it fences are real, and nothing above blocks the merge.
简体中文
我审的是 f0de24ee9b44234ea4b59c21a7f3f266915edd51，没有 P0/P1，一条 P2。
我自己独立看出来的其它问题，前面两份审查都已经写过了，我同意，不再重复：标题和改动对不上、轨道点击的对齐认领在命令结束后仍然残留、20 秒的断言低于产品自己的 30 秒首次发送上限、跳转链接的重试、以及第二个 prompt-rail fixture 仍然没有指定语言。
我能补充两点。
这个 head 的检查现在已经跑完并且是绿的。 前面两份审查发布时 test 还在运行，现在 33674595696 已经在 f0de24ee 上成功结束，被审查的 head 就是通过检查的 head。
P2：这个 PR 依赖的两处「后续再修」都没有任何地方在跟踪，一旦合入，最后的记录也会消失。
描述里推迟了两个已知缺陷，而两处指向的后续工作都不存在：
- 冷启动时光标恢复会抢走焦点。
caretToContentEnd（packages/ui/src/composer.tsx:582）在可编辑区域内设置了一个 range，这本身就会让该元素获得焦点，而packages/ui/src/composer.tsx:731-739的 effect 调用它时并没有自己的focus()调用——正是描述中说的那个机制。描述称这个问题「另行修复」，并且那个改动「会在自己的 diff 里去掉这个重试」。我搜过所有开着的 issue 和 PR，没有这样的工作。#4579 改的是哪个窗口获得前台，和哪个元素获得焦点不是一回事。 sendCurrent会静默丢弃一次提交。#4573 里写明这个「值得单开一个 issue」，但那个 issue 从未被创建。
合入会同时抹掉这两者的记录，因为描述里写了 Closes #4573：这个 issue 是第 2 点唯一的书面记录，而跳转链接的重试正是让持续集成不再能暴露第 1 点的原因。
跳转链接那个重试该怎么看，取决于这一点。如果它挡在一个真的要来的修复前面，那它是合理的，十次按键的预算也完整保留了。但如果它是永久的，就意味着「键盘用户在冷启动时够不到跳转链接」这个状态，从此会被测试套件报告为健康，而且没有任何地方记着这笔账还欠着。这个缺口很小，补起来也便宜：把两个后续 issue 开出来并在这里链上，或者把 Closes #4573 降成普通引用，让这个 issue 带着它没做完的那一条继续开着。
我是对着源码审的，没有在我这边跑测试套件，所以这里的通过证据是上面那次托管运行和其他审查者报告的本地运行，不是我的。
同意合入。改动是对的，它围住的 flake 是真的，上面这些都不构成阻塞。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Astro-Han
changed the title
…wns it A rail click on an unloaded prompt aims the turn at the top of the scrollport and leaves a claim so the shell's reveal for that turn agrees with it instead of centring it a frame later. The claim named a turn and nothing consumed it, so it outlived the navigation it belonged to: the shell issues a fresh nonce for every navigation but the same turnId, so a later global search for that same turn was still read as the rail's, and lost its own contract — smooth and centred — to an instant top jump. Bind the claim to the first target that arrives for its turn, and spend it on anything else. A later search for the same turn is a different command with its own nonce and gets the search contract back. The rule is now a pure function so the rail-then-search sequence has a unit test rather than another Electron round trip. Two smaller corrections in the e2e fixtures: `promptRailMotionWindow` was the remaining window inheriting the host locale, which is the failure that made this branch's CI red once already; and `awaitSendReady`'s comment claimed its 20s assertions absorbed a slow readiness probe, which they do not — the probe's own barrier gives up at 30s, and the 60s test budget, not that barrier, is what bounds the window a send can take. Generated-by: Claude Code
a800d00 to
84e3a85
Compare
2 participants
Summary
Desktop e2e tests failed on main and on PR reruns with
element(s) not foundright after a send, and were green on rerun. Every send goes through an asynchronous admission chain, and the specs pressed Enter after an intermediate signal — a mounted composer, a cleared draft, an updated model label — none of which says the shell will accept a submission. Moving Desktop e2e to 4 workers on one runner (#4523) cut each worker's CPU share and removed the margin that had been hiding it.Fencing those sends left three more failures in the same specs, none of them a send. All three are chased to their cause here rather than deferred, so the specs this PR touches run clean end to end: a second writer moving the transcript scroller in the product, a cold-start focus race in the test, and a fixture window that never named its locale.
Test changes
fixtures.ts— one exportedawaitSendReady(page)waits for 发送 to be enabled: connections projected, no confirmed blocker, a draft to send, no send already in flight. One send instreaming-remount.spec.tsalready did this by hand; the helper is that wait named once instead of copied. What the button cannot say is whether the readiness probe has answered — an unresolved snapshot is not a hard block, so Send stays enabled while the probe is in flight andsend()awaits it again on its own. The 20s assertions cover every admission measured here, but not the whole 30s first-send observation barrier; widening them past it buys nothing, because a 60s test budget is the real cap and a send admitted at 25s leaves the multi-send specs unable to finish either way. A probe that comes back blocked still drops the submission silently, which is the third item in flaky(e2e): unfenced Enter races a 30s send admission against a 10s expect (streaming-remount:161, transcript-scroll:316) #4573 and is a product gap no test-side fence can close.streaming-remount.spec.ts—await expect(composer).toHaveText('')after 新任务 proves only that the draft key swapped. Every send in the file goes throughawaitSendReady, and the assertions that observe the resulting Turn carry an explicit{ timeout: 20_000 }— above the config-wide 10s default, below the 60s test timeout.transcript-scroll.spec.ts—await expect(modelSwitcher).toContainText('glm-4.5')waits on the switcher's local label, not on the Session committing model and connection identity.sendPromptnow fences, and the.maka-user-messageassertion carries 20s.accessibility-coverage.spec.ts— three sends fenced. In the two that reach Send by keyboard the fence goes after the Tab walk: a tooltip-carrying Astryx Button is disabled througharia-disabledand stays focusable, sotabToreaches it either way and an earlier fence would only reopen the window it was meant to close.fixtures.ts—promptRailMotionWindowgated readiness on[data-turn-id]alone. The transcript and the fixture attributes arrive on two unordered async paths:runDeferredStartupRefreshesfiresrefreshSessions()andapplyE2eFixture()side by side, and only the second writesdata-maka-scroll-motion, after itse2eFixture.getState()IPC resolves. One selector —html[data-maka-scroll-motion="smooth"] [data-turn-id]— now requires both.No e2e test is added: the diff under
apps/desktop/e2eadds zerotest(blocks and only changes what existing tests wait on. Roughly 40 bare-Enter sends remain in other specs; they can adopt the helper when they fail.Product change: two writers aiming the same turn
prompt-rail.spec.ts:213failed on the clicked prompt reaches the top — 68px against a 24px bound, held for the full 10s poll. A settled wrong position, not motion still in flight, and it reproduces outside the test.Clicking a rail tick for a prompt outside the resident range asks the shell to load it, and the shell answers a load request by publishing a scroll target. That target is the search reveal:
block: 'center'with the app's scroll motion. So two writers aim at the same turn with different answers — the rail's instantblock: 'start'jump, and a smooth centring animation a frame or two behind it. Onchat-prompt-rail, whose head is 110 turns outside the resident range, the rail lands turn 1 at the top, the centring scroll then walks the transcript back down over ~950ms and clamps atscrollTop0, leaving the prompt 68px below the top. The rail's hold re-aims and wins that race most of the time, which is what made this look like flake; when the range arrives while the animation is already running, the hold sees a moving scroller every frame, never re-aims, and the prompt never reaches the top at all.The reveal is not redundant — it also records the reading position, so dropping it for rail navigations loses the anchor a session switch restores from. Only its alignment conflicts, and the transcript is where both facts meet: it aimed the turn and it consumes the reveal, so
ChatViewreconciles them and the shell keeps publishing one kind of target. A reveal for the turn the rail is holding is instant and top-aligned; every other one, a search result included, is centred and animated as before. The claim belongs to the one navigation the click asked for, not to the turn: it binds to the first target that arrives for that turn and is spent on anything else, so a later search for the same turn — a different command with its own nonce — gets the search contract back.resolveRailAlignedTargetis that rule as a pure function, covered bypackages/ui/src/__tests__/rail-alignment-claim.test.ts.Fixture locale
transcript-scroll.spec.ts:259failed on CI at the fence itself withelement(s) not found, on an English page.promptRailWorkerpassed nolocale, so its renderer took the host's: Chinese on a developer's desktop, English on the CI runner, and every label-addressed control a coin flip. It now nameszh, and so doespromptRailMotionWindow, the other window that had been inheriting the host locale — it only reaches data attributes today, but the next label-addressed step there would bring the same failure back.Skip link
accessibility-coverage.spec.ts:222failed onenterMainFromSkipLink, which parks focus onbodyand asserts the skip link is within ten Tab presses of the document start. Parking is not a one-shot the renderer respects: the composer restores its draft caret withgetSelection().addRange(...), and a range set inside acontenteditablefocuses it — so once per cold start, tens of milliseconds after the park and with nofocus()call to fence on, focus lands in the composer. A walk that starts there runs out the tab ring and wraps around, over budget. The restore fires once, so the park and the walk retry together rather than the budget growing to absorb the wrap-around; the budget is the assertion and it stays where it was.The product side of that — a caret restore that takes focus, and moves the sequential focus navigation starting point even if focus is handed back — is a real a11y defect and is being fixed separately; that change removes this retry as part of its own diff.
What this PR deliberately does not do
An earlier revision also made the shell block sends while the readiness probe was unresolved, so that an enabled Send meant admission was settled. CI disproved it:
pendingreaches the composer throughsendBlocked, andsendCurrentdiscards a submission onsendBlockedrather than waiting, so an Enter pressed during the probe window —send-message.spec.ts:35does exactly that at cold start, and so does a user who types immediately after launch — was silently dropped where it previously waited for the probe and went through. Splitting the two facts (disable the button, still hand the keystroke tosend()) needs one more prop across the shell boundary, whichapp-shell.tsxcannot afford under the renderer architecture ratchet. So the gate is out, and the honest order is: give a dropped submission feedback first (#4573, item 3), then tighten the button.Closes #4573
Verification
playwright test e2e/send-message.spec.ts e2e/streaming-remount.spec.ts e2e/transcript-scroll.spec.ts e2e/accessibility-coverage.spec.ts e2e/prompt-rail.spec.ts→ 31 passed.prompt-rail.spec.tsrepeated 20 times gave 16 passed / 4 failed, every failure the same reaches the top assertion at 68px; with the fixture motion gate reverted and everything else identical, 18 / 2. The"smooth"/undefinedfailure the issue names appeared in neither run of 20 — the motion gate is a real race, but the anchoring bug is what dominated that test.npm exec -w @maka/ui -- npm run test:dist→ 328 passed, including the rail-then-search alignment sequence.npm run format,npm run lint,npm exec -w @maka/desktop -- npm run typecheck, andnode apps/desktop/scripts/check-renderer-architecture.mjs --base origin/main— all clean.The CI artifact step is not exercised locally; it only runs on a failing job. It is what produced the evidence for the locale diagnosis above.
AI use
Select exactly one:
Tool(s) and scope: Claude Code drafted the spec and fixture changes, the scroll-anchor fix, the CI step and this description; the root-cause analysis, the scope decisions and the verification runs were reviewed by the author.
Checklist
Does this PR entail a change in behavior?