Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/web-shell/client/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ type ChatEditorTestProps = {
type AddWorkspaceDialogTestProps = {
onClose: () => void;
onAdd: (cwd: string, persist: boolean, displayName?: string) => Promise<void>;
onSuggest?: (prefix: string) => Promise<unknown>;
onPick?: () => Promise<string | undefined>;
displayNameEnabled?: boolean;
persistenceSupported?: boolean;
Expand Down Expand Up @@ -7985,7 +7986,7 @@ describe('App session callbacks', () => {
},
],
} as typeof mockWorkspace.capabilities;
const { container } = renderApp();
const { container, rerender } = renderApp();
await flush();

act(() => {
Expand All @@ -7998,6 +7999,16 @@ describe('App session callbacks', () => {
displayNameEnabled: true,
persistenceSupported: true,
});
// The dialog's fetch effect re-runs on every onSuggest identity
// change, so App must pass the memoized workspace action itself,
// not a per-render closure; pin the reference across a re-render.
expect(testState.latestAddWorkspaceDialogProps?.onSuggest).toBe(
mockWorkspaceActions.suggestWorkspacePaths,
);
rerender();
expect(testState.latestAddWorkspaceDialogProps?.onSuggest).toBe(
mockWorkspaceActions.suggestWorkspacePaths,
);
mockWorkspaceActions.pickWorkspaceDirectory.mockResolvedValue({
kind: 'workspace-directory-picker',
selected: true,
Expand Down
14 changes: 5 additions & 9 deletions packages/web-shell/client/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ import {
copyFromLastAssistantMessage,
COPY_MESSAGES,
} from './utils/copyCommand';
import { isEditableTarget } from './utils/dom';
import { getShadowAwareActiveElement, isEditableTarget } from './utils/dom';
import {
invokeSlashCommandHandler,
SLASH_COMMAND_PATTERN,
Expand Down Expand Up @@ -3861,8 +3861,8 @@ export function App({
}
// document.activeElement retargets to the shadow host in shadow-DOM
// portal mode; read the focused node from the surface's own root.
const surfaceRoot = surface.getRootNode() as Document | ShadowRoot;
if (!surface.contains(surfaceRoot.activeElement)) surface.focus();
const surfaceActive = getShadowAwareActiveElement(surface);
if (!surface.contains(surfaceActive)) surface.focus();
// Keydowns inside the sandboxed HTML preview iframe never reach the
// surface's Tab-wrap handler or the window Escape handler, and a Tab
// past the preview's last focusable lands focus natively outside the
Expand Down Expand Up @@ -3922,9 +3922,7 @@ export function App({
return;
}
const [first, last] = getFullscreenSurfaceTabEdges(event.currentTarget);
const focused = (
event.currentTarget.getRootNode() as Document | ShadowRoot
).activeElement;
const focused = getShadowAwareActiveElement(event.currentTarget);
if (!first || !last) {
if (focused === event.currentTarget) event.preventDefault();
return;
Expand Down Expand Up @@ -9660,9 +9658,7 @@ export function App({
<AddWorkspaceDialog
onClose={() => setShowAddWorkspaceDialog(false)}
onAdd={handleAddWorkspace}
onSuggest={(prefix) =>
workspaceActions.suggestWorkspacePaths(prefix)
}
onSuggest={workspaceActions.suggestWorkspacePaths}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R7-4: The switch to the stable onSuggest reference is load-bearing but unpinned by any App-level test. The dialog's fetch effect lists onSuggest in its deps [path, onSuggest, closeList], bumps suggestSeqRef on every run, and clears the 150 ms debounce in its cleanup — so a fresh per-render identity here restarts the debounce and invalidates in-flight lookups on every App re-render. Stability holds today only because DaemonWorkspaceProvider memoizes the actions object on [baseUrl, token]; App.test.tsx fully mocks the dialog, captures its props, and asserts the onPick/onClose/onAdd wiring, but never onSuggest. — Failure scenario: a future change re-introduces a per-render closure (the exact shape this diff removes) or widens the provider memo's deps → during streaming, when App re-renders constantly, every re-render while the dialog is open re-runs the fetch effect, so workspace suggestions churn and effectively never appear; the suite cannot detect the regression in either direction.

Suggested fix: in the existing App-level add-workspace test, assert latestAddWorkspaceDialogProps.onSuggest is mockWorkspaceActions.suggestWorkspacePaths and remains referentially identical across a re-render.

中文说明

改用稳定的 onSuggest 引用是承重的,但没有任何 App 层测试将其钉住。对话框的 fetch effect 把 onSuggest 列在依赖 [path, onSuggest, closeList] 中,每次运行都递增 suggestSeqRef,并在清理函数中清除 150 ms 去抖定时器——因此此处若重新出现每次渲染都变化的引用,App 每次重渲染都会重启去抖并使在飞行中的查询失效。当前的稳定性仅因 DaemonWorkspaceProvider[baseUrl, token] 为依赖记忆化 actions 对象而成立;App.test.tsx 完整 mock 了该对话框、捕获其 props,并断言了 onPick/onClose/onAdd 的接线,却从未断言 onSuggest。— 失败场景:未来某次改动重新引入每次渲染都变化的闭包(正是本 diff 移除的形态)或放宽 provider 记忆化的依赖 → 流式输出期间 App 持续重渲染,对话框打开时的每次重渲染都会重跑 fetch effect,工作区建议将不断抖动、实际上永远无法出现;且现有测试套件无法从任何方向检测到该回归。

建议修复:在现有的 App 层"添加工作区"测试中,断言 latestAddWorkspaceDialogProps.onSuggestmockWorkspaceActions.suggestWorkspacePaths,且在一次重渲染后引用保持同一。

— qwen3.8-max via Qwen Code /review (v0.21.9)

onPick={async () => {
const result = await workspaceActions.pickWorkspaceDirectory();
return result.selected ? result.path : undefined;
Expand Down
Loading
Loading