fix(composer): restore focus after dialog close by ousugo · Pull Request #16733 · CherryHQ/cherry-studio

ousugo · 2026-07-04T00:26:10Z

🚨 Branch strategy — read before opening this PR

The v2 refactor has merged into main, so main is the default branch for active development (v1 and v2 code currently coexist there — expect large, breaking changes).

Active development (features, refactors, optimizations, fixes for the current codebase) → target main (the default base).

v1 maintenance (hotfixes and subsequent v1 releases) → branch from and target v1, not main.

A v1 fix does not auto-carry to main: if the same bug exists on main, open a separate forward-port PR targeting main. Before touching subsystems being replaced, read docs/references/data/ and watch for @deprecated markers — they flag code being deleted.

What this PR does

Before this PR:

Closing composer-adjacent dialogs and native pickers could leave focus on the dialog trigger, the page body, or another intermediate control instead of returning to the composer input. The save path for assistant/agent edit dialogs could also schedule duplicate focus restoration callbacks.

After this PR:

Composer tool actions and assistant/agent resource dialogs restore focus consistently after closing. Save callbacks refresh data without scheduling a second focus restoration, and tests cover cancel/save paths plus tool-menu and visible-button web search flows.

Fixes # N/A

Why we need it and why it was done in this way

The following tradeoffs were made:

Focus restoration is explicitly wired through the composer input adapter and deferred with requestAnimationFrame so it runs after dialogs and overlays close. For the visible Web Search button, the fallback focus target is the clicked trigger because that path does not have a composer input adapter.

The following alternatives were considered:

Relying on Radix/native default focus behavior was rejected because these flows are opened from several indirect entry points and the restored target was inconsistent. Using duplicated requestAnimationFrame stubs in each test was also rejected in favor of a shared test helper.

Links to places where the discussion took place: N/A

Breaking changes

If this PR introduces breaking changes, please describe the changes and the impact on users.

None.

Special notes for your reviewer

The file picker cancel path intentionally restores focus to the composer as part of the same "close the transient UI and continue typing" behavior.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Branch: This PR targets the correct branch — main for active development, v1 for v1 maintenance fixes
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

Fixes composer focus restoration after closing prompt, resource, web search, and file picker dialogs.

kangfenmao

LGTM — verified no double focus restoration on edit-save (onOpenChange fires before onSaved; handleEditSaved intentionally does not re-restore) and single restore on create-submit/cancel paths. Visible WebSearch button safely captures event.currentTarget before the async close. Types and renderBelowControls wiring check out; CI green. Two optional test-convention nits (prefer the @test-mocks/requestAnimationFrame alias over the deep relative path; move the mid-file import to the top in the three tool test files) — non-blocking.

eeee0717

Findings:

Important: src/renderer/components/composer/tools/components/WebSearchButton.tsx:97 restores focus from afterClose even when the user confirms the missing-provider dialog and onOk navigates to /settings/websearch. AppModal awaits onOk, closes the modal, then runs afterClose from the delayed removal path, so the composer/editor can already be stale or unmounted by the time inputAdapter.focus() runs. Please restrict the restore to dismiss/cancel/overlay close, or guard that the target is still mounted/current before focusing it.

Open questions:

None.

Summary:
The broader focus-restoration wiring looks sound, and I did not repeat the existing non-blocking import-convention/test-style notes. This is a comment review rather than a request for changes because the issue is limited to the missing-provider navigation branch.

Verification:

Reviewed PR #16733 at head 0932be501d9613840c548afee21ef4e53ea9d74f in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260706T0830Z/pr-16733-0932be5 with three reviewer slices plus coordinator pass.
Checked GitHub CI via gh; static/core checks passed, and final long-running jobs were refreshed before submission.
Ran git diff --check origin/main...HEAD.
Did not run local pnpm lint/test/format, per repository review rules.

修复后请 request eeee0717 再进行后续 review。

AtomsH4

LGTM

范围：为 composer 的对话框、选择器、快捷短语、附件 picker、web search confirm 等交互补上输入框 focus restore，并增加 requestAnimationFrame 测试 mock 和相关 UI 回归测试。

影响：用户从这些浮层/菜单退出后可以回到 composer 继续输入，减少键盘流中断；restore 只在相关关闭/确认路径触发，未看到重复聚焦或误抢焦点问题。未发现 blocker/warning。

ousugo · 2026-07-06T08:59:39Z

This comment was translated automatically.

@eeee0717 I've addressed your review and pushed to the PR branch: ce191521b6.

Fix: When web search provider is missing, the confirmed redirect to /settings/websearch path now skips the modal afterClose composer focus restore; cancel, mask close, etc. still follow the current composer close path and continue to restore focus.

Additional verification:

pnpm test:renderer src/renderer/components/composer/tools/components/__tests__/WebSearchButton.test.tsx
biome.CMD check src/renderer/components/composer/tools/components/WebSearchButton.tsx src/renderer/components/composer/tools/components/__tests__/WebSearchButton.test.tsx
git diff --check

I've added regression tests to cover the scenario where focus is not restored after confirmed redirect. Please help review again.

Original Content

@eeee0717 已按你的 review 处理并推到 PR 分支：ce191521b6。

修复点：缺少 web search provider 时，确认跳转 /settings/websearch 的路径现在会跳过 modal afterClose 的 composer focus restore；取消、遮罩关闭等仍停留在当前 composer 的关闭路径继续恢复焦点。

补充验证：

pnpm test:renderer src/renderer/components/composer/tools/components/__tests__/WebSearchButton.test.tsx
biome.CMD check src/renderer/components/composer/tools/components/WebSearchButton.tsx src/renderer/components/composer/tools/components/__tests__/WebSearchButton.test.tsx
git diff --check

已补回归测试覆盖确认跳转后不恢复焦点的场景，麻烦再帮忙 review。

AtomsH4

改动范围：本次新增了 composer Web Search 按钮/plus menu 弹窗关闭后的焦点恢复逻辑，并补充了缺少搜索 provider 时跳转设置页不应恢复触发按钮焦点的测试。

影响：交互方向是合理的，可以避免从确认弹窗进入设置页时焦点被拉回旧按钮；但最新提交在测试文件里重复导入 installSyncRafMock，导致 basic-checks 的 oxlint 失败。该 CI blocker 修掉后我再复核。

eeee0717

Findings:

None.

Open questions:

None.

Summary:
The previously reported missing-provider navigation focus issue is fixed at head c039728. The follow-up duplicate RAF mock import was removed, and I did not find additional blocking issues in the composer/tool/selector focus restoration changes.

Verification:

Reviewed PR #16733 at head c039728 in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260706T0906Z/pr-16733-c039728 with three reviewer slices plus coordinator pass.
Checked GitHub CI via gh: changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, and render-test passed; notify/translate/claude skipped.
Ran git diff --check origin/main...HEAD.
Did not run local pnpm lint/test/format, per repository review rules.

AtomsH4

LGTM

范围：为 composer 的对话框、选择器、快捷短语、附件 picker、web search confirm 等交互补上输入框 focus restore；最新提交还修复了 WebSearchButton 测试里重复导入 installSyncRafMock 的 lint blocker。

影响：用户从这些浮层/菜单退出后可以回到 composer 继续输入；确认跳转到 Web Search 设置页时不会再把焦点拉回旧按钮。当前 head c039728 的 basic-checks、general-test、render-test 均已通过，未发现新的 blocker/warning。

- Settings window removal (#16489): drop SettingsApp's host mount; the settings page now renders inside the main window, already host-covered. - Composer focus-restore (#16733): reimplement over the popup track — add ConfirmPopupProps.focusOnClose (Radix onCloseAutoFocus + preventDefault) and route WebSearchButton through it, replacing the afterClose + requestAnimationFrame focus race. - Re-apply the window.toast/window.modal -> toast/popup codemod onto upstream's moved/renamed files (Appearance/Dependencies/System settings, MCP, paintings, knowledge) and SystemSettings' new call sites.

ousugo requested a review from a team July 4, 2026 00:26

ousugo closed this Jul 4, 2026

ousugo deleted the fix/composer-focus-restore branch July 4, 2026 12:35

ousugo restored the fix/composer-focus-restore branch July 4, 2026 12:55

ousugo reopened this Jul 4, 2026

kangfenmao approved these changes Jul 6, 2026

View reviewed changes

fix(composer): restore focus after dialog close

0932be5

ousugo force-pushed the fix/composer-focus-restore branch from cbac630 to 0932be5 Compare July 6, 2026 08:27

ousugo requested review from AtomsH4, eeee0717 and zhangjiadi225 July 6, 2026 08:30

eeee0717 reviewed Jul 6, 2026

View reviewed changes

AtomsH4 approved these changes Jul 6, 2026

View reviewed changes

fix(composer): skip focus restore before web search settings

ce19152

ousugo requested a review from eeee0717 July 6, 2026 08:59

AtomsH4 requested changes Jul 6, 2026

View reviewed changes

Comment thread src/renderer/components/composer/tools/components/__tests__/WebSearchButton.test.tsx

fix(test): remove duplicate raf mock import

c039728

eeee0717 approved these changes Jul 6, 2026

View reviewed changes

AtomsH4 approved these changes Jul 6, 2026

View reviewed changes

ousugo added the ready-to-merge This PR has sufficient reviewer approvals but is awaiting code owner approval. label Jul 6, 2026

Merge branch 'main' into fix/composer-focus-restore

961bc3d

0xfullex merged commit 9b3e4ab into CherryHQ:main Jul 6, 2026
14 of 15 checks passed

ousugo deleted the fix/composer-focus-restore branch July 6, 2026 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(composer): restore focus after dialog close#16733

fix(composer): restore focus after dialog close#16733
0xfullex merged 4 commits into
CherryHQ:mainfrom
ousugo:fix/composer-focus-restore

ousugo commented Jul 4, 2026

Uh oh!

kangfenmao left a comment

Uh oh!

eeee0717 left a comment

Uh oh!

AtomsH4 left a comment

Uh oh!

ousugo commented Jul 6, 2026 •

edited by github-actions Bot

Loading

Uh oh!

AtomsH4 left a comment

Uh oh!

Uh oh!

eeee0717 left a comment

Uh oh!

AtomsH4 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

ousugo commented Jul 4, 2026

🚨 Branch strategy — read before opening this PR

What this PR does

Why we need it and why it was done in this way

Breaking changes

Special notes for your reviewer

Checklist

Release note

Uh oh!

kangfenmao left a comment

Choose a reason for hiding this comment

Uh oh!

eeee0717 left a comment

Choose a reason for hiding this comment

Uh oh!

AtomsH4 left a comment

Choose a reason for hiding this comment

Uh oh!

ousugo commented Jul 6, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AtomsH4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eeee0717 left a comment

Choose a reason for hiding this comment

Uh oh!

AtomsH4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ousugo commented Jul 6, 2026 •

edited by github-actions Bot

Loading