fix(desktop): keep the slash picker stable across same-content skill refreshes

[Benjamin-eecs]

Summary

Typing /r and waiting made the slash picker alternate between two popup geometries with a replayed open transition. Every session updated/turn-status-change/rebound event and every MCP change reloads the invocable Skill projection, and the reload cleared the list before repopulating it: the open menu collapsed to its commands-only shape, then sprang back one IPC round trip later, each time replaying the trigger-menu transition because the projection's identity had changed twice.

Two changes in useComposerMentions:

• The fail-closed clear now applies only to a key change (session, project, model, or mode switch), where showing the previous surface's Skills would be wrong. A same-key refresh keeps the current list on screen until the fresh one arrives. A Skill withdrawn inside that window still fails safely, because selection resolves through the Runtime resolver that no longer knows it.
• A refresh that changed nothing keeps the previous array identity, so the composer's trigger memo and its menu-replay effect stay quiet and the menu is not re-searched at all.

Fixes #2667

Verification

• New e2e drives the reported shape with a pre-armed MutationObserver: with the menu open on /, three same-content projection refreshes (thinking-level updates publishing the session's updated event) must remove neither the listbox nor its groups. Without the fix the watch counts 15 removals; with it, zero, and the Skills group stays visible throughout.
• The full slash-command-menu e2e suite passes three consecutive runs.

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Astro-Han]

Reviewed against the slash-picker flicker.

Same-key skill reloads no longer clear the list, and a same-content refresh keeps the array identity so the menu-replay effect stays quiet. The four-field InvocableSkillEntry comparison matches the type today. A stale picker entry still goes through resolveSkillInvocations, so a withdrawn skill is not executed.

Non-blocking: the effect still lists skills and the new-session model as fail-closed keys, even though neither enters the invocable projection. Pinning or refreshing Skills can therefore replay the same collapse. Astryx light-dismiss hides it from a normal click. [...next] is also an extra copy; the IPC payload is already a new array.

Approve.

AI-assisted review: Grok 4.6, opencode-go/deepseek-v4-flash:max, and ark-coding-plan/glm-5.3 each reviewed the PR independently. I checked the identity path from useComposerMentions through composer.tsx and the e2e that counts menu-node removals. Unverified by me: I did not rerun the desktop e2e locally.

[Astro-Han]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Failed refresh keeps stale skills 🐞 Bug ≡ Correctness

Description

A same-key listInvocable rejection now leaves the previous skill projection visible indefinitely,
rather than limiting staleness to one IPC round trip or leaving / with no suggestions as
documented. The stale entries remain until another successful refresh or key change, although
selecting one may subsequently fail in Runtime.

Code

apps/desktop/src/renderer/use-composer-mentions.ts[R63-65]

+    const refresh = (options?: { failClosed?: boolean }) => {
      const version = ++requestVersion;
-      setMentionSkills([]);
+      if (options?.failClosed) setMentionSkills([]);

Relevance

●●● Strong

Finding contradicts the documented fail-soft behavior; accepted precedents prioritize clearing stale
UI across asynchronous failure paths.

PR-#3048
PR-#3075
PR-#3147

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The newly conditional clear runs only for failClosed refreshes, while session and MCP
notifications invoke refresh() without that option. The rejection handler performs no state update
even though the IPC can reject when the Runtime catalog is unstable, so the retained projection has
no expiry after failure.

apps/desktop/src/renderer/use-composer-mentions.ts[63-86]
apps/desktop/src/renderer/use-composer-mentions.ts[89-102]
apps/desktop/src/main/runtime-host-client.ts[399-410]
apps/desktop/src/main/runtime-host-skills-ipc-main.ts[81-99]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Same-key refreshes preserve the current skill list while loading, but a rejected authoritative refresh never clears that list. Clear skills when the latest refresh fails so the temporary stale window cannot become indefinite.

## Issue Context
The rejection handler must apply only when the effect is still active and the failed request remains the latest `requestVersion`; otherwise an older failure could erase a newer successful projection.

## Fix Focus Areas
- apps/desktop/src/renderer/use-composer-mentions.ts[63-86]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Test misses delayed refresh commits 🐞 Bug ☼ Reliability

Description

The zero-removal poll succeeds immediately because zero is already the initial value, without
waiting for any projection refresh or resulting React commit to finish. Consequently, the regression
test can pass before the old clear-and-repopulate behavior removes the menu groups.

Code

apps/desktop/e2e/slash-command-menu.spec.ts[R201-204]

+  await expect
+    .poll(
+      () =>
+        page.evaluate(

Relevance

●●● Strong

Recent accepted E2E precedents require explicit synchronization for asynchronous DOM and observer
behavior.

PR-#3101
PR-#3057
PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The observer starts at zero, and Playwright polling stops as soon as the matcher succeeds, so the
timeout does not create an observation window. setThinkingLevel awaits the configuration update
and emits the session event before returning, but the renderer's follow-up projection request and
DOM render are separate asynchronous work for which the test has no completion signal.

apps/desktop/e2e/slash-command-menu.spec.ts[187-211]
apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts[185-190]
apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts[212-221]
apps/desktop/src/renderer/use-composer-mentions.ts[90-102]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Add a deterministic completion barrier proving that all three skill projection reloads have completed before asserting the observer's removal count. The current `expect.poll(...).toBe(0)` returns on its first zero observation and does not provide the delay claimed by the comment.

## Issue Context
Reuse the existing `window.maka.skills.listInvocable` seam in the test to count or otherwise observe completed reloads. Waiting only for `setThinkingLevel` is insufficient because the main handler emits the session event before returning, while renderer event handling, the subsequent skills IPC request, and React's DOM commit can still occur later.

## Fix Focus Areas
- apps/desktop/e2e/slash-command-menu.spec.ts[176-211]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: ⚖️ Balanced: This is a localized but behavior-changing state/refresh fix in the composer, with fail-closed semantics and UI identity invariants that warrant a careful single-pass review; it is not broad or defect-dense enough for extended.

_{ⓘ  1 issues published inline · 2 in summary}

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

1. Failed refresh keeps stale skills 🐞 Bug ≡ Correctness

A same-key listInvocable rejection now leaves the previous skill projection visible indefinitely,
rather than limiting staleness to one IPC round trip or leaving / with no suggestions as
documented. The stale entries remain until another successful refresh or key change, although
selecting one may subsequently fail in Runtime.

Agent Prompt

## Issue description
Same-key refreshes preserve the current skill list while loading, but a rejected authoritative refresh never clears that list. Clear skills when the latest refresh fails so the temporary stale window cannot become indefinite.

## Issue Context
The rejection handler must apply only when the effect is still active and the failed request remains the latest `requestVersion`; otherwise an older failure could erase a newer successful projection.

## Fix Focus Areas
- apps/desktop/src/renderer/use-composer-mentions.ts[63-86]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[Astro-Han]

This PR changes the visible slash-picker behavior during Skill refreshes. Could you please add a screenshot showing the stable open picker after a same-content refresh? A short recording would also be helpful for the no-collapse behavior, but the screenshot is the required UI evidence. Thanks!

Posted by Codex on behalf of Astro-Han.

[Astro-Han]

Hi — this PR conflicts with current main and cannot be merged as-is.

I tested a rebase onto current main locally (in a throwaway worktree — your branch was not touched). It stops on these files:

• apps/desktop/src/renderer/use-composer-mentions.ts

These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current main and resolve them yourself, then push. Once the branch is conflict-free and CI is green on the new head, I will pick it up for review.

git fetch upstream && git rebase upstream/main
# resolve, then
git push --force-with-lease

Thanks for the contribution — happy to help if any conflict is unclear.

---

AI-assisted maintenance note, not a review. It does not count as the required human review under CONTRIBUTING.md §Review.

[Astro-Han]

This branch is now 388 commits behind main, and it cannot be rebased mechanically: use-composer-mentions.ts was rewritten wholesale by #3122 and #3469. The patch here targets the old structure (the old setMentionSkills plus the listInvocable(sessionId, context) signature), while the hook is now a single catalog state with a settled verdict. The conflict has no mechanical resolution.

That said, I checked and the problem this PR set out to fix is still present on main:

git show origin/main:apps/desktop/src/renderer/use-composer-mentions.ts

The current refresh() (lines 108-114) preserves settled across a same-context refresh but still sets skills to [], and the exported mentionSkills is exactly liveCatalog.skills (line 205). So #3469 stabilised the Skills row in the + menu, while the / menu list still empties and refills on every session / MCP event — the menu height jump you reported should still reproduce.

Suggestion: write a small fresh patch against current main that preserves the previous skills on a same-context refresh and keeps array identity when the content is unchanged. Your e2e case is still valuable and worth carrying over. I am not merging this one.

简体中文

这个分支已经落后 main 388 个提交，而且没法直接 rebase：use-composer-mentions.ts 在 #3122、#3469 之后被整体重写了，这个 PR 的补丁是针对旧结构写的（旧的 setMentionSkills + listInvocable(sessionId, context) 签名），现在的 hook 已经改成单一的 catalog 状态加 settled 判定，冲突没法机械解决。

不过我核对了一下，这个 PR 要修的问题在 main 上还在：现在的 refresh()（第 108-114 行）在同 context 刷新时保留了 settled，但仍然把 skills 置为 []；而对外暴露的 mentionSkills 就是 liveCatalog.skills（第 205 行）。也就是说 #3469 稳住的是 ＋ 菜单的 Skills 行，/ 菜单的列表在每次 session / MCP 事件刷新时依然会先空掉再填回来，你报的菜单高度跳动应该还会复现。

建议在当前 main 上重写一个小补丁：同 context 刷新时保留上一份 skills，并在内容相同时保持数组引用不变。原来的 e2e 用例还是有价值的，可以一起带过来。我这边先不合并这条。

[Astro-Han]

I rebased this onto current main and pushed to your branch — the head is now ee926225b7e4138cd4f39e4b3783936c5acefc73. The branch was 391 commits behind and use-composer-mentions.ts had been rewritten underneath it, so the original patch no longer applied. Please take a look and tell me if I got the intent wrong.

The bug is still live on main. refresh() still sets skills: [] on every call, and mentionSkills still reads straight from it, so an open / menu still loses its Skills group on every session or MCP event.

What changed since you opened this. main now models the catalog as one value — { contextKey, loading, settled, skills } — and already applies your argument to one of its facets: a same-context refresh keeps its settled verdict, and a render whose contextKey no longer matches falls back to a frozen empty list without waiting for the effect. So the context-switch case already fails closed at render time.

How I re-expressed the fix. Rather than a failClosed flag on refresh(), the clear now keys off the same signal main already uses:

• Same contextKey → keep the Skills on screen ({ ...previous, loading: true }). The backend surface has not changed, so there is nothing to fail closed against, and a Skill withdrawn inside the one-round-trip window still fails safely because selection resolves through the Runtime resolver that no longer knows it.
• Changed contextKey → clear, exactly as before.

Your invocableSkillListsEqual identity-preservation is kept as you wrote it, applied on settle so an unchanged refresh keeps the previous array and the trigger memo and menu-replay effect stay quiet.

No regression to the ＋ menu. composer-plus-menu-stability.spec.ts pins the Plan-toggle flicker, and a Plan toggle moves newSessionCollaborationMode, which is part of contextKey — so that path still clears and still reads settled. The two fixes cover complementary cases rather than colliding.

Your regression spec applied unchanged; I verified its fixture and selectors (invocableSkillsWindow, the 命令和技能 listbox, the Skills group, sessions.setThinkingLevel) all still exist on current main.

Checks: Biome clean on both files. Renderer typecheck produced an identical error set to a clean main checkout in the same environment (235 files either way), so nothing is attributable to this change — the local noise is stale workspace links. The e2e spec needs a full desktop build, so CI is the real verdict there; the repository's Actions queue is backed up right now, and I will confirm once it runs.

Since the code moved, the existing approval no longer describes this head. I will re-review at ee926225 and re-approve there.

简体中文

我把这个 PR rebase 到了当前 main 并推到了你的分支，新 head 是 ee926225b7e4138cd4f39e4b3783936c5acefc73。分支落后 391 个提交，且 use-composer-mentions.ts 在这期间被重写，原补丁已经打不上了。麻烦你看一下，如果我理解错了意图请告诉我。

这个 bug 在 main 上仍然存在：refresh() 每次都会 skills: []，而 mentionSkills 直接读它，所以打开的 / 菜单在每次 session 或 MCP 事件时仍会丢失 Skills 分组。

这期间的变化：main 现在把 catalog 建模成一个整体值 { contextKey, loading, settled, skills }，并且已经把你的论证用在了其中一个面上——同 context 的刷新会保留 settled，而 contextKey 不匹配的渲染会直接回落到冻结的空列表，不必等 effect。所以「切换 context」这一档已经在渲染层失败关闭了。

我怎么重新表达这个修复：不再给 refresh() 加 failClosed 参数，而是复用 main 已有的同一个信号——

• contextKey 相同 → 保留屏幕上的 Skills（{ ...previous, loading: true }）。后端面没变，没有需要失败关闭的对象；即使某个 Skill 在这一次 IPC 往返内被撤下，选中它仍然是安全的，因为解析走的是已经不认识它的 Runtime resolver。
• contextKey 变化 → 照旧清空。

你写的 invocableSkillListsEqual 原样保留，放在 settle 时使用，使内容未变的刷新沿用同一个数组引用，从而不惊动 trigger memo 和菜单重放 effect。

没有回退 ＋ 菜单的修复：composer-plus-menu-stability.spec.ts 钉的是 Plan 切换闪烁，而 Plan 切换会改变 newSessionCollaborationMode，它属于 contextKey，所以那条路径仍然清空、仍然读 settled。两个修复覆盖互补的情形，不冲突。

你的回归用例原样适用；我核对过它用到的 fixture 和选择器（invocableSkillsWindow、命令和技能 listbox、Skills 分组、sessions.setThinkingLevel）在当前 main 上都还在。

检查结果：两个文件 Biome 通过。渲染层 typecheck 与同环境下干净 main 检出的报错文件集合完全一致（都是 235 个），因此没有可归因于本次改动的问题——本地噪音来自过期的 workspace 链接。e2e 用例需要完整的桌面构建，真正的判据是 CI；仓库的 Actions 队列目前积压，跑完后我会再确认。

由于代码已经变动，原有的 approve 不再描述这个 head。我会在 ee926225 上重新复审并重新 approve。

[Astro-Han]

Approving at ee926225b. CI is terminal green on this exact head (run 32714136676), including the Desktop e2e job that carries your regression spec.

Since I rebased this branch and re-expressed the fix against main's rewritten use-composer-mentions.ts, I re-checked it rather than relying on the earlier approval, which was bound to 9ef3e4f86 and no longer describes this code.

The open review comment about a failed refresh is addressed. The concern was that a same-key listInvocable rejection would leave the previous projection visible indefinitely. That was accurate against the original patch, but it does not survive the rebase: main's rejection handler already fails soft by clearing, and I kept it unchanged, so a rejected refresh still lands on { loading: false, settled: 'empty', skills: [] } under the same cancelled || version !== requestVersion guard the comment asked for. The stale window stays bounded by one IPC round trip.

What else I verified:

• Merged against current main — which moved to d77e854f7 after #3573 landed — not just against the PR head. The merge is clean, Biome is clean on both files, and the renderer typecheck produces an error set identical to a clean main checkout in the same environment (235 files either way), so nothing is attributable to this change.
• The ＋ menu's separate fail-closed path is intact. A Plan toggle moves newSessionCollaborationMode, which is part of contextKey, so that path still clears and still reads settled. composer-plus-menu-stability.spec.ts and your new spec cover complementary cases rather than competing ones.
• The bug was still live on main before this: refresh() cleared skills on every call and mentionSkills read straight from it.

Thank you for the original diagnosis and for the regression spec — the spec applied unchanged and is what makes this safe to land. Merging now.

简体中文

在 ee926225b 上给出 approve。这个 exact head 的 CI 是终态绿（运行记录 32714136676），其中包含承载你那条回归用例的 Desktop e2e 任务。

由于这个分支是我 rebase 的，并且我把修复按 main 重写后的 use-composer-mentions.ts 重新表达了一遍，所以我重新核对了结果，而没有沿用之前那条 approve：它绑在 9ef3e4f86 上，已经不描述当前代码了。

关于「刷新失败」的那条未解决评论已经不成立了。 它担心的是：同 key 的 listInvocable 被拒绝后，之前的投影会无限期地留在界面上。这一点对原始补丁是准确的，但 rebase 之后不再成立：main 原有的拒绝分支本来就是失败即清空，我原样保留了它，因此被拒绝的刷新仍然落到 { loading: false, settled: 'empty', skills: [] }，并且带着评论所要求的那个 cancelled || version !== requestVersion 守卫。过期窗口仍然被限制在一次 IPC 往返之内。

我另外核对的内容：

• 是对当前 main 做的合并验证——#3573 合入后 main 已经变成 d77e854f7——而不只是看 PR head。合并干净，两个文件 Biome 通过，渲染层 typecheck 的报错集合与同环境下干净 main 检出完全一致（都是 235 个文件），因此没有可归因于本次改动的问题。
• ＋ 菜单那条独立的失败关闭路径没有被破坏。 Plan 切换会改变 newSessionCollaborationMode，它属于 contextKey，因此那条路径仍然清空、仍然读 settled。composer-plus-menu-stability.spec.ts 和你新增的用例覆盖的是互补情形，而不是互相竞争。
• 这个 bug 在 main 上此前确实仍然存在：refresh() 每次调用都会清空 skills，而 mentionSkills 直接读它。

感谢你最初的定位和那条回归用例——用例原样适用，也正是它让这次落地是安全的。现在合并。