fix(ui): stop the restored draft caret from taking focus

[Astro-Han]

Summary

The composer took keyboard focus nobody directed at it. caretToContentEnd restores the draft caret with a selection, and a selection inside a contenteditable is never only a caret: the browser focuses the element to carry it, and sequential focus navigation then resumes from the selection instead of from the top of the document. That reached the user on two paths — on a cold start, tens of milliseconds in with no focus() call to explain it, so Tab from the document start began inside the composer instead of at the 跳到主要内容 skip link; and on a session swap, where activating a sidebar row moved focus off the row the user had just activated.

The caret is therefore owed rather than placed whenever the editor is not focused, and lands on the editor's next real focus — the first moment the offset is the only thing being decided. A pointer press places the caret itself and is the more specific intent, so it drops the claim instead of being overruled by it.

A held caret is suspended across the Skill-chip redraw rather than left armed. redrawSkillTokens drives insertToken through the document selection, and its first range focuses the editor — which would otherwise fire the focus lander onto the range the redraw is holding, collapsing it to the end so the chip landed at the end and its source text stayed in the draft. The redraw collects the caret itself on success, so the claim is settled there, and is handed back untouched on a pass that redrew nothing.

This is the fix #4577 named: it wrapped the skip-link walk in a re-parking expect(...).toPass({ timeout: 30_000 }) to survive the theft, and said the removal belonged to this change. The retry and the comment explaining it go here, and the walk is a one-shot park plus budgeted walk again.

Refs #4577

Verification

Cold start, measured on the accessibility-coverage fixture by parking focus on body right after a renderer reload, sampling document.activeElement for 1.5s, then counting Tab presses to the skip link:

	focus drift	presses to skip link
before	body → DIV[消息输入框]	9
after	body	3

Session swap, measured on the same fixture (send a message, leave a plain-text unsent draft, 新任务, click the session row back):

	focusin after the row click	ends focused
before	BUTTON:<session row> → DIV:消息输入框	composer
after	BUTTON:<session row>	session row

The second row is now an e2e test (activating a session row with an unsent draft keeps focus on the row), because the focus side effect of a selection only exists in a real browser: the unit harness models it and cannot observe it. It fails on base 898b86d6.

• npm run format — no fixes applied
• npm run lint — clean
• npm exec -w @maka/desktop -- npm run typecheck — clean
• npm exec -w @maka/ui -- npm run test:dist — 332 passed, including 4 new tests. On base 898b86d6, two of them fail (the cold-start restore places a selection; a pointer press cannot drop a caret that was never held) and one passes as a continuity guard; the fourth's contract is the e2e above, which base fails.
• apps/desktop → npx playwright test --config e2e/playwright.config.ts e2e/accessibility-coverage.spec.ts e2e/composer-skill-invocation.spec.ts — 8 passed with the retry removed
• npm run check:asf-headers — clean

Review focus

The first revision of this change deferred the caret only when document.activeElement was null or body, on the premise that a selection takes focus only when nobody holds it. That premise is false — measured on Electron 43.4.1 / Chromium 150, a selection placed here takes focus from a focused button exactly as it takes it from body — and the review that caught it also caught that the unit harness modelled addRange without that side effect, which made the focus assertion vacuous. The harness now reproduces it, and the boundary is activeElement !== editable.

One issue raised in review is deferred, not fixed here: caretToContentEnd returns without recording a claim when editableNode() finds no editor, which a host that mounts the composer disabled can reach (it renders contenteditable="false"). It behaves identically on main, it is a caret-offset defect rather than a focus one, and closing it wants a disabled-composer fixture this change does not have.

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code — investigated the focus theft, wrote the fix, the tests and this description. Commits carry Generated-by: Claude Code.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[jackwener]

I reviewed exact head 974b4cff0527d90d301ea3ba01f37ea146188412. One P1, one P3. Not approving.

The cold-start half of this works, and the debt it was written to clear is genuinely cleared. Parking focus on body and walking to the skip link is a one-shot again, the toPass retry and its explanatory comment are gone, and the ten-press budget is untouched — the accessibility suite passes 30/30 under --repeat-each=5 without the retry. The diagnosis is also right: a collapsed selection inside a contenteditable is not just a caret, and moving the sequential focus navigation start point is what put the skip link behind the user.

P1 — "another element is focused" is not the safe case it is treated as

packages/ui/src/composer.tsx:601-605 defers the caret only when document.activeElement is null or body. With any other element focused it places the selection immediately, on the premise that a selection is only a caret when something else holds focus.

That premise does not hold in the shipping runtime. On Electron 43.4.1 (Chromium 150.0.7871.224), in a visible window with document.hasFocus() === true, selectNodeContents + collapse(false) + addRange moves focus into the editor from a focused button, not only from body:

button-focused              → active: "out"
after-place-from-button     → active: "ed"      ← taken
body-focused                → active: "BODY"
after-place-from-body       → active: "ed"      ← taken

The product path reproduces it. Create a session, leave a plain-text unsent draft (no Skills, so no token redraw), go to 新任务, then click the original session row: focusin fires BUTTON:<session row> → DIV:消息输入框, and the composer ends up focused. A keyboard user activating a session row still has focus taken out from under them — the same defect as the cold-start one, on a different path.

To be clear about attribution: the focus-stealing side effect of addRange is pre-existing. What this change introduces is the boundary that decides when to avoid it, and that boundary is drawn in the wrong place.

Why the new tests pass anyway. In packages/ui/src/__tests__/composer-draft-caret-focus.test.tsx, the stand-in selection implements addRange as selected.push(range) and never touches the mocked activeElement — only the harness's own focus() helper writes it. So this assertion cannot fail under that model no matter what a real browser does:

assert.equal(dom.focused(), dom.outside(), 'the swap took focus off the row that caused it');

It is asserting a property of the harness. That also explains a discrepancy worth correcting in the description: run against base 898b86d6, the four new tests are 2 fail / 2 pass, not 3 fail — and the outside-focus case is one of the two that already pass. A test green before the fix is not evidence for the fix.

Fixing it needs more than widening the condition. Changing the guard to active !== editable would defer the caret on the session-swap path too, and redrawSkillTokens writes its Skill chips through that same selection — an earlier revision that deferred whenever the editor was blurred lost a redrawn token's source text, which composer-skill-invocation caught. The caret placement and the chip rewrite need separating before the boundary can move. The acceptance evidence should be a contract in a real browser context: after restoring a plain-text draft by activating a session row, the session row still holds focus.

P3 — an owed caret is dropped when no editable exists yet (pre-existing)

caretToContentEnd returns at composer.tsx:599-600 when editableNode() finds nothing, without recording the claim, while the calling effect has already cleared caretToEndRef. A host that mounts the composer disabled renders contenteditable="false", and the lookup asks for "true" — so a draft set in that window loses its caret restore, and the next real focus lands at offset 0 with typing prepending to the draft.

This behaves identically on main, which has the same early return and the same caller ordering, so it is not a regression here. It is raised because this change introduces exactly the mechanism that would close it: setting the pending claim in that branch as well is a two-line addition.

---

label and test are green on this head. The block is the P1.

简体中文

我审的是 974b4cff0527d90d301ea3ba01f37ea146188412：一条 P1、一条 P3。不批。

冷启动那一半是有效的，而且它要还的那笔账确实还清了：把焦点停在 body 再走到跳转链接，重新变回一次性动作，toPass 重试和解释它的注释都删掉了，十次按键的预算没有放宽；去掉重试后可及性套件在 --repeat-each=5 下 30/30 通过。诊断也是对的：在 contenteditable 里放一个折叠选区不只是放光标，它还会挪动顺序焦点导航的起点，这正是跳转链接跑到用户身后的原因。

P1：把「有别的元素持有焦点」当成安全分支，是不成立的。 composer.tsx:601-605 只在 activeElement 是 null 或 body 时才把光标欠着；只要焦点在别的元素上，就立刻放选区，前提是「有别的元素持有焦点时，选区只是光标」。

这个前提在实际运行时不成立。在 Electron 43.4.1（Chromium 150.0.7871.224）、窗口可见且 document.hasFocus() 为 true 的情况下，selectNodeContents + collapse(false) + addRange 会从一个已聚焦的按钮上把焦点抢进编辑器，不只是从 body：

button-focused              → active: "out"
after-place-from-button     → active: "ed"      ← 被抢
body-focused                → active: "BODY"
after-place-from-body       → active: "ed"      ← 被抢

产品路径可复现：建一个会话，留一份纯文本未发送草稿（不带 Skill，排除芯片重绘），进「新任务」，再点回原来的会话行——focusin 顺序是 BUTTON:<会话行> → DIV:消息输入框，最后焦点落在输入框上。键盘用户激活会话行之后，焦点仍然会被夺走，和冷启动那条是同一个缺陷，只是换了一条路径。

归属要说清楚：addRange 抢焦点这个副作用是既有的。这次改动引入的是「什么时候要避开它」这条边界，而边界画错了位置。

为什么新测试还是全绿。 composer-draft-caret-focus.test.tsx 里那个替身 selection，addRange 的实现只是 selected.push(range)，从不碰被模拟的 activeElement——只有 harness 自己的 focus() 会写它。所以下面这条断言在该模型下不可能失败，无论真实浏览器是什么行为：

assert.equal(dom.focused(), dom.outside(), 'the swap took focus off the row that caused it');

它断言的是 harness 的属性。这也解释了描述里一处需要更正的数字：把这 4 条新测试接到 base 898b86d6 上，实际是 2 条失败 / 2 条通过，不是正文说的 3 条失败——而 outside-focus 正是已经通过的那两条之一。一条在修复前就绿的测试，不能作为修复的证据。

修法不只是把条件放宽。 若改成 active !== editable，切会话那条路径的光标也会被推迟，而 redrawSkillTokens 正是靠同一个选区写 Skill 芯片——此前有个「只要编辑器失焦就推迟」的版本，就丢了重绘芯片的源文本，被 composer-skill-invocation 抓到。得先把「放光标」和「重绘芯片」拆开，边界才能移动。验收证据应当是真实浏览器环境下的一条契约：通过激活会话行恢复纯文本草稿之后，会话行仍然持有焦点。

P3：编辑器还不存在时，欠下的光标被丢掉（既有问题）。 composer.tsx:599-600 在 editableNode() 找不到时直接返回，没有记下这笔欠账，而调用它的 effect 已经把 caretToEndRef 清掉了。宿主以 disabled 挂载时渲染的是 contenteditable="false"，而查询要的是 "true"。于是这段窗口里设置的草稿会丢掉光标恢复，下次真正聚焦时光标落在位置 0，打字会插到草稿前面。

这在 main 上行为完全一样（同样的提前返回、同样的调用顺序），所以不是这次的回归。之所以提出来，是因为这次改动恰好引入了能关掉它的机制：在那条分支上也置一下欠账，两行的事。

这个 head 上 label 和 test 都是绿的，拦下来的是那条 P1。

---

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]

Both points landed. The P1 is real and my premise was wrong — I reproduced it independently before changing anything, and the fix moved the boundary rather than widening it. The P3 is real and deferred, with a reason. Details in the inline replies; head is now e996a2689.

One correction to your P1 write-up, in your favour: the reason widening the guard broke composer-skill-invocation is not that the caret placement and the chip rewrite need separating. redrawSkillTokens sets its own range and calls insertToken, and that first addRange focuses the editor — which fired the new focus lander onto the range the redraw was holding, collapsing it to the end. The chip then landed at the end and its source text stayed. So the conflict is re-entrancy during the redraw, not shared ownership of the selection, and it is fixed where it happens.

[Astro-Han]

P1 — accepted, verified independently, fixed here.

I did not take the measurement on faith; I ran my own probe in the shipping runtime before touching the guard, and it reproduces exactly what you reported:

hasFocus=true
button-focused           → BUTTON#probe-out
after-place-from-button  → EDITABLE      ← taken
body-focused             → BODY
after-place-from-body    → EDITABLE      ← taken

And the product path, on the window fixture (send a message, leave a plain-text unsent draft, 新任务, click the session row back):

focusin: BUTTON:<session row> → DIV:消息输入框
active = DIV:消息输入框

Run against base 898b86d6, that probe gives the identical result, which settles the attribution the way you put it: the focus side effect is pre-existing, and what the first revision got wrong was the boundary.

The premise is gone: the guard is document.activeElement !== editable, so the caret is owed whenever the editor is not focused, whoever holds focus. Same probe after the change: focusin: BUTTON:<session row> and nothing else, and the row still holds focus.

[Astro-Han]

This is what made widening the guard safe, and it is narrower than "separate the caret placement from the chip rewrite".

redrawSkillTokens writes a range per token and calls insertToken. That first addRange focuses the editor — the very side effect this PR is about — which fired the focus lander, and the lander ran caretToContentEnd on top of the range the redraw was holding, collapsing it to the end. insertToken then deleted nothing and inserted the chip at the end, which is exactly the Project Only run it /skill:workspace-only Workspace Only you would have seen had the earlier revision reached CI here.

So the claim is suspended across the redraw rather than left armed. On success the redraw's own restoreCaret settles it; on a pass that redrew nothing it is handed back untouched, so a stale Skill catalog still gets its retry. composer-skill-invocation passes.

[Astro-Han]

P3 — accepted as real, deferred, not fixed here.

You are right on both the mechanism and the attribution: the early return drops the claim, the caller has already cleared caretToEndRef, and a composer mounted disabled renders contenteditable="false" so the lookup misses. main behaves identically.

Deferring rather than taking the two lines, for two reasons. It is a different defect — a caret at offset 0, not focus taken from the user — so it does not gate the contract this change is here to establish. And there is no disabled-composer fixture in this suite, so the two lines would ship without a test that fails without them; that is the same standard you held the outside-focus case to, and I would rather meet it in a change that can.

Recorded under Review focus in the description so it is not lost.

[Astro-Han]

The vacuous assertion — accepted. The stand-in addRange only pushed the range, so document.activeElement moved for no reason other than the harness's own focus() helper, and the focus assertion could not fail. It now reproduces the side effect the probe measured: a selection whose container resolves to a [contenteditable="true"] focuses it.

The test that rested on it is rewritten as the contract rather than its inverse — a session swap leaves focus on the row that caused it.

The base counts — corrected. Your 2 fail / 2 pass was right for the head you reviewed, and the description said 3, which was true only of an earlier revision of the tests. Against base 898b86d6 now: a draft restored while nothing holds focus places no selection and a pointer press into the composer drops the owed caret fail; the owed caret lands at the end of the draft on the next focus passes as a continuity guard; the session-swap one does not complete there — the file stops after the third test, which I chased far enough to place in teardown rather than in the assertion, and no further. I am not counting it as base evidence. Its evidence is the e2e.

[Astro-Han]

The acceptance evidence you asked for, as a contract in a real browser: after restoring a plain-text draft by activating a session row, the row still holds focus.

It is here and not in a unit test on purpose — the focus a selection takes exists only in a browser, so the harness can model it but never observe it. Plain text, no Skill token, so the token redraw's own selection write stays out of it.

It fails on base 898b86d6 and passes on this head. Full run of accessibility-coverage plus composer-skill-invocation: 8 passed.

[jackwener]

Approving at exact head e996a26892cc4febd50b97ea19e14f56b907df7c. No P0 or P1. The blocking finding from my previous review is closed on the plain-text path. Two comments below, neither blocking.

The guard is now simply document.activeElement !== editable, which is the right shape: a selection inside a contenteditable takes focus from whatever held it, so "something else is focused" was never the safe case. Activating a session row with a plain-text draft now leaves focus on the row.

The test model is fixed too, and that mattered more than the guard. addRange in the harness now moves the modelled focus into the editor, so the assertion that the swap leaves focus outside can finally fail — restoring the old implementation no longer passes it. The previous version could not express focus movement at all, which is why four green tests sat on top of a live defect. There is also now a real-browser contract covering the whole path, which is where the side effect actually lives.

P2 — a draft containing a Skill token still takes focus from the session row

redrawSkillTokens writes its chips through the document selection and calls selection.addRange(range) itself, so it never passes through the new guard. The suspend added around it holds caretPendingRef aside so the focus lander cannot collapse the redraw's first range to the end — that fixes a redrawn chip leaving its source text in the draft. It does not stop the addRange from taking focus.

So with focus on a session row and a draft containing a resolvable /skill:…, the effect runs once the catalog is ready and focusin still goes row → composer. The chips land correctly; the focus is lost exactly as before. The new end-to-end case is deliberately plain text, so it does not reach this path.

This is pre-existing — redrawSkillTokens places that selection on main too — and it is raised because this change is what establishes "restoring a draft is not a reason to move focus" as the rule. Half of it now holds. The other half cannot be fixed by widening the guard: writing a chip requires a selection, and a selection takes focus, so there is no third path that does both. Separating the chip rewrite from the caret placement is the change that would close it, and it is its own piece of work rather than an omission from this one.

P3 — a throwing redraw swallows the owed caret

The suspend clears caretPendingRef before calling redrawSkillTokens() and restores it from the return value, with no try/finally. If insertToken throws, heldCaret never comes back; redrawPendingRef stays true so the chips retry on the next pass, but the caret claim is gone and the next real focus lands at offset 0, prepending to the draft. Restoring it in a finally is enough.

The !editable early return still drops a claim without recording it — pre-existing, unchanged, and already noted last round.

test is terminal green on this head, Desktop e2e included.

简体中文

在 e996a26892cc4febd50b97ea19e14f56b907df7c 上批准。没有 P0/P1。上一条评审里那条拦截性发现,在纯文本路径上已经收口。 下面两条都不阻塞。

guard 现在简化成 document.activeElement !== editable,这个形状是对的:contenteditable 里的选区会从任何持有者手上夺走焦点,所以「有别的元素持有焦点」从来就不是安全分支。现在用纯文本草稿激活会话行,焦点留在行上。

测试模型也修好了,而这件事比 guard 本身更要紧。 harness 里的 addRange 现在会把被模拟的焦点移进编辑器,于是「切换后焦点仍在外面」这条断言终于能失败了——把旧实现接回去,它不再通过。之前那个版本根本无法表达焦点移动,这正是四条全绿的测试压在一个真实缺陷上的原因。另外现在有了一条真实浏览器契约覆盖整条路径,而那正是这个副作用真实发生的地方。

P2:草稿里含 Skill 芯片时,切会话仍会从会话行夺走焦点。

redrawSkillTokens 靠文档选区写芯片,自己调 selection.addRange(range),根本不经过新的 guard。围绕它加的 suspend 只是把 caretPendingRef 暂时拔掉,防止 focus lander 把重绘的第一个 range 塌到末尾——那修的是「重绘后的芯片把源文本留在草稿里」这个洞,不是「不夺焦点」那个洞。

于是:焦点在会话行上、草稿含可解析的 /skill:…,catalog 就绪后 effect 一跑,focusin 仍然是 行 → 输入框。芯片写对了,焦点照样丢。新增的端到端用例特意用纯文本,走不到这条路径。

这是既有行为——main 上 redrawSkillTokens 同样会放这个选区。之所以提出来,是因为正是这次改动把「恢复草稿不构成移动焦点的理由」立成了规则,而现在这条规则只成立一半。另一半没法靠放宽 guard 解决:写芯片必须有选区,而选区必然夺焦点,代码里不存在同时满足两者的第三条路。真正能关掉它的是把芯片重写和光标放置拆开,那是一件独立的工作,不算这次的遗漏。

P3:重绘抛异常会吞掉欠下的光标。 suspend 在调用 redrawSkillTokens() 之前把 caretPendingRef 清掉,再按返回值恢复,中间没有 try/finally。如果 insertToken 抛出,heldCaret 就再也回不来了;redrawPendingRef 仍是 true,芯片下一拍会重试,但光标欠账已经没了,下次真正聚焦会落在 offset 0,打字插到草稿前面。放进 finally 恢复即可。

!editable 的提前返回仍然会丢掉欠账而不记录——既有问题,本次未动,上一轮已提过。

这个 head 上 test 已终态通过,含 Desktop e2e。

---

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.

[jackwener]

Your correction is right, and I owe it a note rather than a quiet edit.

Widening the guard did not need the chip rewrite and the caret placement separated. You moved the boundary to activeElement !== editable directly, and the thing that broke composer-skill-invocation was re-entrancy — the redraw's own first addRange focuses the editor, the new focus lander then fires onto the range the redraw is holding and collapses it to the end. Suspending the claim across the redraw fixes that where it happens. My previous review said the boundary could not move until those two concerns were separated; that was wrong, and your fix demonstrates it.

One thing to keep separate, because my approval was posted before I had read this and repeats the phrasing: the P2 there is not the re-entrancy. Re-entrancy is fixed. What remains is that redrawSkillTokens calls selection.addRange(range) itself and therefore never passes the guard, so a draft containing a resolvable /skill:… still takes focus off the session row — chips correct, focus lost. That is pre-existing and non-blocking, and nothing in your correction touches it. I mention it only so "re-entrancy is fixed" is not read as closing it.

简体中文

你的更正是对的,我用新评论说明,不回头改旧的。

放宽 guard 并不需要先把芯片重写和光标放置拆开。你直接把边界移到了 activeElement !== editable,而打破 composer-skill-invocation 的其实是重入——重绘自己的第一个 addRange 会聚焦编辑器,新加的 focus lander 随即触发到重绘正持有的那个 range 上,把它塌到末尾。在重绘期间挂起欠账,就在发生处修好了它。我上一条评审说「边界要移动,必须先拆开这两件事」——那是错的,你的修复证明了这一点。

有一点要分开,因为我那条批准是在读到你这条之前发的,里面沿用了同样的措辞:那里的 P2 不是重入问题。重入已经修好了。剩下的是 redrawSkillTokens 自己调 selection.addRange(range),因此根本不经过 guard——所以草稿里含可解析的 /skill:… 时,焦点仍会从会话行被夺走:芯片是对的,焦点丢了。这条是既有问题、不阻塞,你的更正也没有触及它。我提一句,只是为了避免「重入已修」被读成「这条也解决了」。

---

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.