fix(tab-bar): prevent detaching settings tabs

[Pleasurecruise]

Branch strategy

• Active development targets main.
• v1 maintenance targets v1; forward-port fixes to main separately when needed.

What this PR does

Before this PR:

Settings tabs could be detached through the toolbar button, context menu, or drag gesture, allowing multiple Settings interfaces with independent window-local state.

After this PR:

Settings tabs remain focused tabs in the main window and cannot be detached. Regular tabs retain their existing detach behavior.

Fixes # N/A

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

Settings tabs belong to the main window's focused-tab flow. The tab bar disables Settings detach capabilities for the toolbar, context menu, and drag gesture; TabsProvider rejects direct renderer requests; and the main-process tab.detach handler rejects direct IPC requests as the final boundary.

Settings navigation keeps the existing local-first route delivery with IPC fallback, so this change only affects detach behavior.

The following tradeoffs were made:

The detach policy is specific to Settings routes; general multi-window and tab detach behavior remains unchanged.

The following alternatives were considered:

Hiding only the toolbar button was rejected because the context menu, drag gesture, and direct detach requests would still bypass the policy.

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

Breaking changes

None.

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

N/A

Special notes for your reviewer

Please verify that Settings has no detach affordance, direct Settings detach requests are ignored, regular tabs can still be detached, and Settings navigation retains its local-first 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

Settings tabs can no longer be opened in a separate window, preventing duplicate Settings interfaces with inconsistent window-local state.

[claude]

Code review complete — no findings.

Reviewed the three-layer detach guard (capability table, drag handler, provider guard) and all touched APIs: the getTabCapabilities signature change (now requires url) is fully propagated to every caller; the useTabDrag refactor moving canDetach from a hook option to a per-pointerdown ref value is sound (no stale closure, all 3 handlePointerDown call sites pass caps.detach, effect deps updated correctly); the removed handleDetachTab settings-restore logic is genuinely dead and its previousWorkspaceTabIdRef is still used by handleCloseTab. Direct tab.detach IPC callers (useConversationNavigation, KnowledgeStep) are an out-of-scope "open elsewhere" flow with non-settings URLs and are unaffected. New tests assert real outcomes rather than pinning current behavior.

[kangfenmao]

This comment was translated automatically.

@claude Take a look at the purpose of this PR

---

Original Content

@claude 看看这个 pr 作用

[claude]

Claude finished @kangfenmao's task in 46s —— View job

---

PR 作用分析

@kangfenmao 这个 PR 的目标是禁止设置（Settings）标签页被分离/拖出为独立窗口，从而避免出现多个拥有各自窗口本地状态的 Settings 界面。

背景

设置页面的导航在主窗口中是单例设计，而分离出去的子窗口则是多实例的。如果允许 Settings 标签页被分离，就会出现多个不一致的 Settings 界面，造成状态错乱。

改动机制：三层防御

PR 通过三个层面同时封堵 Settings 标签页的分离路径：

1. 能力层（capability）— AppShellTabBar.tsx

• getTabCapabilities 现在需要读取 tab.url，对 Settings 路径返回 detach: false：

  const detach = ctx.canDetach && !isSettingsPath(tab.url)

  这从源头让上下文菜单、拖拽判断都拿不到 detach 能力。

2. 工具栏按钮 — AppShellTabBar.tsx

• 新增 canDetachActiveTab 计算，只当当前激活标签是普通（非 Settings）标签时才渲染“在新窗口打开”按钮：

  const canDetachActiveTab = !!detachTab && normalTabs.some((tab) => tab.id === activeTabId && !isSettingsPath(tab.url))

3. Provider 守卫（纵深防御）— TabsProvider.tsx

• detachTab 内部增加守卫，即便有人直接调用，也会拒绝 Settings 路径：

  if (!tab || isSettingsPath(tab.url)) return

附带的重构

• useTabDrag.ts：把 canDetach 从 hook 选项改为按每次 pointerdown 传入的参数，存入 dragRef.current，避免闭包过期（stale closure）。handlePointerDown 签名变为 (e, tab, tabType, canDetach)，三个调用点都已更新。依赖数组也已相应调整。
• AppShell.tsx：删除了原来的 handleDetachTab（其中包含 Settings 激活态恢复逻辑）。由于 Settings 现在根本无法被分离，这段逻辑变成了死代码，所以被移除；detachTab={detachTab} 直接透传。其中 previousWorkspaceTabIdRef 仍被 handleCloseTab 使用，所以保留。

测试

AppShellTabBar.test.tsx / TabsProvider.test.tsx / AppShell.test.tsx 均更新，新增了对“Settings 标签页无法分离”这一真实行为的断言（而非简单 pin 现有行为）。

总结

普通标签页的分离行为完全不变；只有 Settings 标签页在工具栏按钮、右键菜单、拖拽手势三条路径以及直接 IPC 调用上都被禁用。这是一个聚焦、低风险的改动。

---

[kangfenmao]

This review was translated automatically.

This is a product behavior change. The settings interface is allowed to be a separate independent window, which makes it convenient to quickly modify settings and verify.

---

Original Content

这个是一个产品行为变更，设置界面是允许单独独立窗口的，这样方便快速修改设置和验证

[Pleasurecruise]

@kangfenmao Adjusted according to your feedback. Settings is still allowed to be dragged out as an independent window, but at most one Settings page is kept within the app: after dragging out, when opening Settings from the sidebar, menu, or protocol entry, it will navigate to and focus the existing independent Settings window, and will not create a second Settings page in the main window; only after the independent window is closed or re-attached will it resume opening in the main window. The multi-window behavior of regular tabs is not affected. Corresponding regression tests have been added; the relevant targeted tests passed 141/141, and pnpm lint and pnpm format passed. Fix commit: f37bcce.

[404-Page-Found]

Code Review: fix(tab-bar): prevent detaching settings tabs

Overall: The approach is sound — centralizing settings-window singleton logic in SubWindowService with defense-in-depth at multiple layers is the right architecture. The tests are well-written and cover the key scenarios. I have a few findings ranging from a correctness concern to minor observations.

---

Findings

1. closed handler changes behavior for ALL sub-windows, not just Settings

The closed handler was changed from:

this.tabIdToWindowId.delete(tabId)
this.windowState.delete(tabId)

to iterating all tabIdToWindowId entries and deleting every one that maps to the destroyed windowId.

This is necessary for the Settings singleton (multiple tabIds → one windowId), but it changes cleanup behavior for every sub-window, not just Settings. In practice this is likely harmless (each non-Settings sub-window normally has exactly one tabId mapping), but it widens the blast radius of the change. Consider scoping the iteration to only run when the window is a Settings path, or accept the generalization explicitly.

2. windowState is not cleaned up for non-active tabIds on close

The closed handler cleans up tabIdToWindowId entries but does not clean up the corresponding windowState entries for all removed tabIds — it only cleans the specific tabId it captured. For the Settings singleton, this means stale state can accumulate in windowState for old tabIds that mapped to the closed window. This is a pre-existing issue (the old code had the same gap for a single tabId), but the PR's multi-tabId-per-window pattern makes it more visible.

3. PR description says "cannot be detached" but the detach affordance is still shown

The PR description states "Settings tabs remain focused tabs in the main window and cannot be detached." However, getTabCapabilities in AppShellTabBar.tsx does not suppress the detach affordance for settings paths — canDetach is purely !!detachTab. The PR implements defense-in-depth at the SubWindowService level (reuse the existing window rather than creating a second one), but the user still sees the detach button/menu/drag gesture. They just get silently rerouted. If the goal is to prevent the action (not just absorb it), the capability layer should be updated too. If the current "absorb silently" behavior is intentional, the PR description should say "cannot be detached into a second window" to be precise.

4. Stale state copy in the singleton reuse path

In createWindow (line ~165 of the new code), when a second settings createWindow is intercepted:

const existingState = existingTabId && this.windowState.get(existingTabId)
this.tabIdToWindowId.set(tabId, this.settingsWindowId)
if (existingState) this.windowState.set(tabId, existingState)

The old tabId's windowState (which may contain stale URL/title from the first settings page) is copied to the new tabId. Since openSettingsWindow immediately sends IPC to navigate the existing window, this copied state is likely never read for the new tabId — but it exists as a latent inconsistency. If it's truly dead, consider omitting the copy to avoid confusion.

---

Positive observations

• Test coverage is good. The new tests cover singleton reuse, window-close-then-reopen, the mainWindowNavigation delegation path, and the SubWindowAppShell IPC listener. The existing tests are properly updated.
• The renderer-side openSettingsTab simplification is clean. Removing the DOM-event-first path in favor of direct IPC makes the routing unambiguous — all settings navigation always goes through the main process singleton check.
• The openSettingsWindow method is well-guarded. It checks for null/destroyed windows and clears settingsWindowId before returning false, enabling clean fallback.
• The SubWindowAppShell IPC listener is correctly scoped — it only navigates the active tab, matching the single-tab-per-sub-window pattern for settings.

[cherry-ai-bot]

Cherry Review · 阻塞

1 blocker · 1 warning
逐条见行内评论。

以下 1 条落在本次 diff 的行内定位之外，改为摘要送达（位置见每条开头）：

• src/renderer/components/layout/AppShellTabBar.tsx:425 必须修复：The PR does not enforce the stated Settings non-detach invariant: the UI still exposes detach, provider and IPC paths accept it, and the first direct Settings detach opens a sub-window. inv_73cae064ff83c6cb#c0

[claude]

Code review of this PR's Settings-tab detach prevention:

Detach prevention — verified correct across all entry paths. The toolbar/focused-tab button (canDetachActiveTab), context menu (getTabCapabilities), drag gesture (per-pointerdown canDetach in useTabDrag), and IPC boundary (tab.detach early-return) all gate on isSettingsPath(tab.url). The useTabDrag refactor (moving canDetach from a hook option into dragRef.current.canDetach) is sound — both the drag-mode transitions and the detach-execution block read it, and a Settings tab cannot enter detach mode. Regular-tab detach and reorder behavior is unchanged. The AppShell.tsx removal of the settings restore-after-detach branch is correct (dead code once detach is blocked). Tests assert real invariants (Settings must not trigger detach), not behavior pinning.

One concern (posted inline on mainWindowNavigation.ts): the openSettingsTab rewrite is orthogonal to the detach-prevention goal and regresses the in-window fast path for ~15 main-window callers — adding an IPC round trip and a redundant showMainWindow() where navigation was previously synchronous and side-effect-free. Not a correctness or blocking issue, but worth either dropping the hunk or preserving the openRoute local-first path.

No blocking findings.

[claude]

Reviewed PR #18428 (prevent detaching Settings tabs) read-only across all three guard layers — the renderer capability/menus/toolbar/drag paths in AppShellTabBar.tsx and useTabDrag.ts, the TabsProvider.detachTab guard, and the main-process tab.detach defense-in-depth in tab.ts.

No high-confidence issues found. The detach capability is consistently disabled for Settings URLs at every entry point (toolbar button, three context-menu sites, pointer-drag detach), and the now-unreachable setActiveTab(previousWorkspaceTabIdRef) branch removed from AppShell.handleDetachTab is genuinely dead code (handleDetachTab is only reachable via the gated detachTab prop). The two other direct tab.detach IPC callers (useConversationNavigation, KnowledgeStep) detach /app/... routes, so the main-process isSettingsPath early-return does not affect them. The getTabCapabilities signature change (optional url?) is backward-compatible, and the focused Settings tab lands in normalTabs, so canDetachActiveTab correctly hides the toolbar action.