-
Notifications
You must be signed in to change notification settings - Fork 3.1k
perf(web-shell): derive the session workflow projection once and share it across surfaces (#10865) #11237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
perf(web-shell): derive the session workflow projection once and share it across surfaces (#10865) #11237
Changes from all commits
a0e85e8
1688c31
6c44841
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,179 @@ | ||||||||||
| // @vitest-environment jsdom | ||||||||||
|
|
||||||||||
| import { act } from 'react'; | ||||||||||
| import { createRoot, type Root } from 'react-dom/client'; | ||||||||||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||||||||||
| import type { TodoItem } from '../../adapters/types'; | ||||||||||
| import { I18nProvider } from '../../i18n'; | ||||||||||
| import { TranscriptRenderModeProvider } from '../../transcriptRenderMode'; | ||||||||||
|
|
||||||||||
| // Acceptance #10865, graph-side guarantees, pinned by counting rather than | ||||||||||
| // by inspection: | ||||||||||
| // - hovering a node re-renders without re-running the topological layering | ||||||||||
| // or the topology serialization; | ||||||||||
| // - `measure` runs at most once per animation frame even when a resize | ||||||||||
| // storm lands several schedule calls inside one frame. | ||||||||||
| // In its own file so the module mock cannot reach the behavioural suite | ||||||||||
| // next to it. | ||||||||||
| const counts = vi.hoisted(() => ({ layers: 0 })); | ||||||||||
|
|
||||||||||
| vi.mock('./PlanExecutionView', async (importOriginal) => { | ||||||||||
| const actual = await importOriginal<typeof import('./PlanExecutionView')>(); | ||||||||||
| return { | ||||||||||
| ...actual, | ||||||||||
| layerPlanTodos: (...args: Parameters<typeof actual.layerPlanTodos>) => { | ||||||||||
| counts.layers += 1; | ||||||||||
| return actual.layerPlanTodos(...args); | ||||||||||
| }, | ||||||||||
| }; | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| const { PlanExecutionView } = await import('./PlanExecutionView'); | ||||||||||
|
|
||||||||||
| const todos: TodoItem[] = [ | ||||||||||
| { id: 'research', content: 'Research', status: 'completed' }, | ||||||||||
| { | ||||||||||
| id: 'build', | ||||||||||
| content: 'Build', | ||||||||||
| status: 'in_progress', | ||||||||||
| blockedBy: ['research'], | ||||||||||
| }, | ||||||||||
| { | ||||||||||
| id: 'verify', | ||||||||||
| content: 'Verify', | ||||||||||
| status: 'pending', | ||||||||||
| blockedBy: ['build'], | ||||||||||
| }, | ||||||||||
| ]; | ||||||||||
|
|
||||||||||
| // Each test's tree is unmounted after the test: the graph binds a window | ||||||||||
| // resize listener, and a leaked listener would double-count the next test's | ||||||||||
| // resize storm. | ||||||||||
| const roots: Root[] = []; | ||||||||||
|
|
||||||||||
| function mount(): HTMLElement { | ||||||||||
| const container = document.createElement('div'); | ||||||||||
| document.body.appendChild(container); | ||||||||||
| const root = createRoot(container); | ||||||||||
| roots.push(root); | ||||||||||
| act(() => { | ||||||||||
| root.render( | ||||||||||
| <I18nProvider language="en"> | ||||||||||
| <TranscriptRenderModeProvider> | ||||||||||
| <PlanExecutionView todos={todos} tools={[]} tasks={[]} /> | ||||||||||
|
Comment on lines
+62
to
+63
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The new derivation test renders TranscriptRenderModeProvider with no value prop. transcriptRenderMode.ts:9-10 exports it as the bare TranscriptRenderModeContext.Provider, so omitting value yields undefined and overrides the createContext('interactive') default at :5-6 — React logs a required-prop error on every run, and the component under test receives a render mode no production host produces. documentMode at PlanExecutionView.tsx:201 (const documentMode = useTranscriptRenderMode() === 'document') is computed from undefined rather than from a declared mode, and it gates real behaviour at :898 (disabled={documentMode}) on the plan-node button. So the two guarantees this file exists to pin — hover does not re-run the layering, one measure per animation frame across a resize storm — are asserted only for an accidental undefined mode, never for the modes production uses; and every CI run of the web-shell suite carries a React provider-misuse warning attributed to this new file, which readers must triage as a non-defect each time. Every other usage in the package passes a value (PlanExecutionView.test.tsx:95, UserMessage.test.tsx:77, MessageList.dom.test.tsx:430, ParallelAgentsGroup.test.tsx:105, production WebShellTranscript.tsx:249); the sibling new suite session-workflow-surfaces.test.tsx omits the provider entirely and so warns nothing. Witness:
Suggested change
Pass the mode the assertions target — — matching the context default, or drop the wrapper and let the default apply as the sibling suite does. Constraint the fix must not violate: The value must keep documentMode false: PlanExecutionView.tsx:201 has exactly one read site, :898 disabled={documentMode}, on the plan-node button, so value="document" (the mode the sibling PlanExecutionView.test.tsx:95 uses) is not a drop-in choice here — it would render disabled on the very node the hover assertion targets via container.querySelector('[data-plan-node-id="build"]')?.closest('article'). Use 'interactive' or 'readonly', per transcriptRenderMode.ts:3 export type TranscriptRenderMode = 'interactive' | 'readonly' | 'document'. 中文说明新增的 derivation 测试渲染
(上面的 Witness 是程序输出,按原样保留。) 修复方向:传入断言真正针对的模式 约束:传入的值必须让 验收:N/A——没有任何断言读取这个 context,所以去掉新加的 — qwen3.8-max via Qwen Code /review (v0.23.0) |
||||||||||
| </TranscriptRenderModeProvider> | ||||||||||
| </I18nProvider>, | ||||||||||
| ); | ||||||||||
| }); | ||||||||||
| return container; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| afterEach(() => { | ||||||||||
| for (const root of roots.splice(0)) { | ||||||||||
| act(() => root.unmount()); | ||||||||||
| } | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| describe('PlanExecutionView derivation discipline', () => { | ||||||||||
| it('does not re-run the layering or the topology serialization on hover', () => { | ||||||||||
| const container = mount(); | ||||||||||
| const stringifySpy = vi.spyOn(JSON, 'stringify'); | ||||||||||
| const layersAfterMount = counts.layers; | ||||||||||
| const stringifyAfterMount = stringifySpy.mock.calls.length; | ||||||||||
|
|
||||||||||
| const buildNode = container | ||||||||||
| .querySelector('[data-plan-node-id="build"]') | ||||||||||
| ?.closest('article'); | ||||||||||
| expect(buildNode).toBeTruthy(); | ||||||||||
|
|
||||||||||
| const edges = container.querySelector('[data-plan-edge]')?.closest('svg'); | ||||||||||
| expect(edges?.getAttribute('data-focused')).toBeNull(); | ||||||||||
|
|
||||||||||
| // jsdom has no PointerEvent; React synthesizes onPointerEnter from a | ||||||||||
| // bubbling pointerover, and onPointerLeave from pointerout. | ||||||||||
| act(() => { | ||||||||||
| buildNode?.dispatchEvent( | ||||||||||
| new MouseEvent('pointerover', { bubbles: true }), | ||||||||||
| ); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| // The hover did re-render (focus state flipped)... | ||||||||||
| const focused = container.querySelector('[data-plan-edge]')?.closest('svg'); | ||||||||||
| expect(focused?.getAttribute('data-focused')).toBe('true'); | ||||||||||
|
|
||||||||||
| // ...but the derivation did not re-run: no extra topological layering, | ||||||||||
| // no extra topology serialization. | ||||||||||
| expect(counts.layers).toBe(layersAfterMount); | ||||||||||
| expect(stringifySpy.mock.calls.length).toBe(stringifyAfterMount); | ||||||||||
|
|
||||||||||
| act(() => { | ||||||||||
| buildNode?.dispatchEvent(new MouseEvent('pointerout', { bubbles: true })); | ||||||||||
| }); | ||||||||||
| const unfocused = container | ||||||||||
| .querySelector('[data-plan-edge]') | ||||||||||
| ?.closest('svg'); | ||||||||||
| expect(unfocused?.getAttribute('data-focused')).toBeNull(); | ||||||||||
| expect(counts.layers).toBe(layersAfterMount); | ||||||||||
| expect(stringifySpy.mock.calls.length).toBe(stringifyAfterMount); | ||||||||||
|
|
||||||||||
| stringifySpy.mockRestore(); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it('coalesces a same-frame resize storm into one measure per animation frame', () => { | ||||||||||
| const frames: FrameRequestCallback[] = []; | ||||||||||
| const animationSpy = vi | ||||||||||
| .spyOn(window, 'requestAnimationFrame') | ||||||||||
| .mockImplementation((callback) => { | ||||||||||
| frames.push(callback); | ||||||||||
| return frames.length; | ||||||||||
| }); | ||||||||||
| const rectSpy = vi | ||||||||||
| .spyOn(HTMLElement.prototype, 'getBoundingClientRect') | ||||||||||
| .mockReturnValue({ | ||||||||||
| x: 0, | ||||||||||
| y: 0, | ||||||||||
| top: 0, | ||||||||||
| left: 0, | ||||||||||
| width: 100, | ||||||||||
| height: 80, | ||||||||||
| right: 100, | ||||||||||
| bottom: 80, | ||||||||||
| toJSON: () => ({}), | ||||||||||
| } as DOMRect); | ||||||||||
|
|
||||||||||
| const container = mount(); | ||||||||||
| const nodes = container.querySelectorAll('[data-plan-node-id]').length; | ||||||||||
| expect(nodes).toBe(todos.length); | ||||||||||
|
|
||||||||||
| const framesAfterMount = frames.length; | ||||||||||
|
|
||||||||||
| // One viewport change lands as several schedule calls in the same | ||||||||||
| // frame (the window resize plus, in a real browser, the resize | ||||||||||
| // observer's per-node batch). All of them must share a single | ||||||||||
|
Comment on lines
+150
to
+152
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The coalescing test drives only the window resize listener; the ResizeObserver source its own comment names is never exercised, because client/test/setup.ts:38-43 installs a ResizeObserverStub whose observe() is a silent no-op that never invokes the callback. packages/web-shell/vitest.config.ts sets setupFiles: ['./test/setup.ts'] with root: 'client', so the stub applies to this file. PlanExecutionView.tsx:512-517 does construct the observer with scheduleMeasure and calls observe() on the graph element plus every node, but the stub never fires, so the only path reaching scheduleMeasure in this test is window.dispatchEvent(new Event('resize')) three times. The observer half of the storm — which the component's own comment at PlanExecutionView.tsx:493-498 names as the dominant real-world source ("Every node is observed and a window resize lands in the same frame as the observer's own batch, so one viewport change ran measure many times over") — has zero coverage. Rewire the observer to the un-coalesced function (new ResizeObserver(measure) at :515) and in a browser a single viewport change on a 20-step plan runs 20+ full measure passes, each reading 21 rects and concatenating a 20-edge signature string, while both assertions stay green. Witness: Give the test a controllable observer, following the pattern already used three times in this package (PaneHeaderActions.test.tsx:20-27 and :113, data-table.test.tsx:142-163, App.test.tsx:13270-13291): collect the constructor callbacks via vi.stubGlobal('ResizeObserver', …) before mount(), then fire the captured callback once per observed target inside the SAME act() as the window resizes, and assert the rect count taken BEFORE the frame runs is still nodes + 1. Restore with vi.unstubAllGlobals() alongside the existing animationSpy.mockRestore(). Constraint the fix must not violate: The observer callbacks must be fired inside the same act() as the window resizes and strictly before frames.at(-1)!(0) runs the frame — PlanExecutionView.tsx:502-506 is if (pending) return; pending = true; frame = requestAnimationFrame(() => { pending = false; measure(); });, so pending is cleared only when the callback executes; firing the observer after the frame has run would legitimately schedule a second frame and the delta assertion would read 2 on correct code. Acceptance: The observable that flips is the rect count taken before the frame runs, NOT the frame delta. Measured with the observer callbacks driven 4x in the same act(): INTACT {"observerCallbacksFired":4,"framesDelta":1,"rectsAfterStorm":4} green vs MUTANT {"observerCallbacksFired":4,"framesDelta":1,"rectsAfterStorm":20} -> 'AssertionError: expected 20 to be 4'. A direct measure never schedules a frame so framesDelta stays 1, and the shipped test's rectSpy.mockClear() sits between the storm and the frame run, erasing the observer-driven reads — so an assertion on frames.length - framesAfterMount would not catch this. 中文说明这个合并调度的测试只驱动了
(上面的 Witness 是程序输出,按原样保留。) 修复方向:给测试一个可控的 observer,沿用本包已有的三处写法( 约束:observer 回调必须在与 window resize 同一个 验收:会翻转的可观测量是帧运行之前的 rect 计数,不是帧的 delta。实测把 observer 回调在同一 — qwen3.8-max via Qwen Code /review (v0.23.0) |
||||||||||
| // animation frame, and the frame must run `measure` exactly once. | ||||||||||
| act(() => { | ||||||||||
| window.dispatchEvent(new Event('resize')); | ||||||||||
| window.dispatchEvent(new Event('resize')); | ||||||||||
| window.dispatchEvent(new Event('resize')); | ||||||||||
| }); | ||||||||||
| expect(frames.length - framesAfterMount).toBe(1); | ||||||||||
|
|
||||||||||
| rectSpy.mockClear(); | ||||||||||
| act(() => { | ||||||||||
| frames.at(-1)!(0); | ||||||||||
| }); | ||||||||||
| // One measure pass reads the graph container's rect plus one rect per | ||||||||||
| // node — not one batch per schedule call. | ||||||||||
| expect(rectSpy.mock.calls.length).toBe(nodes + 1); | ||||||||||
|
|
||||||||||
| // The next storm schedules one new frame again. | ||||||||||
| act(() => { | ||||||||||
| window.dispatchEvent(new Event('resize')); | ||||||||||
| window.dispatchEvent(new Event('resize')); | ||||||||||
| }); | ||||||||||
| expect(frames.length - framesAfterMount).toBe(2); | ||||||||||
|
|
||||||||||
| animationSpy.mockRestore(); | ||||||||||
| rectSpy.mockRestore(); | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The counts.layers counter this new test relies on can never increment, so both expect(counts.layers).toBe(layersAfterMount) assertions reduce to 0 === 0 and the "hover does not re-run the topological layering" half of the file's stated guarantee is unpinned. vi.mock('./PlanExecutionView', … importOriginal) replaces the module's exported binding, but the component under test is ...actual, whose closure calls layerPlanTodos(todos) through the module-local declaration inside its own useMemo — a module mock does not rewrite intra-module call sites.
Hoist const layers = hasDependencies ? layerPlanTodos(todos) : [todos.slice()] out of the useMemo at PlanExecutionView.tsx:249 into the component body, or split layering into a separate memo keyed on focus/hover state, and every pointerover/pointerout/focus/blur re-runs the O(V+E) topological sort — the exact regression issue #10865's acceptance criterion forbids and this file's header claims to pin "by counting rather than by inspection" — while the suite stays green. The co-located JSON.stringify spy IS a genuine global-property spy and does pin the serialization half, so only the layering counter is dead; the contrast is the sibling suites, which mock a different module from the one their consumers import, and whose counters therefore do increment.
Witness:
Give the layering a seam a mock can reach: move layerPlanTodos into its own module (e.g. planTodoLayering.ts), import it in PlanExecutionView.tsx, re-export it from there alongside the existing taskExecutionIndex shim, and point this test's vi.mock at the new module. Then add a mount-time sanity assertion — expect(layersAfterMount).toBeGreaterThan(0) — so a non-intercepting mock fails loudly instead of passing silently. Alternative: delete the dead counter and its two assertions and correct the header comment to claim only the serialization pin.
Constraint the fix must not violate: PlanExecutionView.test.tsx:10-18 imports layerPlanTodos plus getActiveAgents, getAttentionAgentTool, getPlanNodeState, nestedAgentToolsForTool and nestedTasksForTool from './PlanExecutionView', and the comment this diff adds at PlanExecutionView.tsx:30-32 states the re-export block exists "to keep this module's public surface stable for its existing importers (tests included)" — any extraction must preserve that re-export.
Acceptance: PlanExecutionView.derivation.test.tsx › 'does not re-run the layering or the topology serialization on hover'. After the fix, expect(layersAfterMount).toBeGreaterThan(0) must hold at mount, and moving the layerPlanTodos(todos) call out of the useMemo into the component body must turn expect(counts.layers).toBe(layersAfterMount) red after the pointerover dispatch. Both mutations leave the test green today — that is the gap.
中文说明
这个新测试所依赖的
counts.layers计数器永远不会自增,因此两处expect(counts.layers).toBe(layersAfterMount)断言实际都退化成0 === 0,文件头声称要钉住的「hover 不会重跑拓扑分层」这半边保证并没有被钉住。vi.mock('./PlanExecutionView', … importOriginal)替换的是模块导出的绑定,但被测组件是...actual,它的闭包通过模块内部的局部声明调用layerPlanTodos(todos)(在组件自己的useMemo里)。模块 mock 不会改写模块内部的调用点,所以包装函数根本不在渲染路径上。把
layerPlanTodos(todos)从useMemo里提到组件函数体,或把分层拆进另一个以 hover/focus 状态为依赖的 memo,那么每次pointerover/pointerout/focus/blur都会重跑 O(V+E) 的拓扑排序——正是 issue #10865 验收标准禁止、也是本文件头声称「用计数而非人工检查钉住」的那个回归——而测试套件依然全绿。需要说明的边界:同一个测试里的
JSON.stringifyspy 是真正的全局属性 spy,它确实钉住了拓扑序列化那一半,所以失效的只有分层计数器。对照的是同批新增的另外两个套件:它们 mock 的是与消费方不同的模块,所以计数器是真正的跨模块命名空间查找,确实会自增。(上面的 Witness 是程序输出,按原样保留。)
修复方向:给分层一个 mock 能拦到的接缝——把
layerPlanTodos移到独立模块,在PlanExecutionView.tsx里 import 并 re-export,然后把本测试的vi.mock指向新模块;同时补一条挂载期自检断言expect(layersAfterMount).toBeGreaterThan(0),让「mock 没拦住」这种情况直接失败而不是静默通过。另一种选择是删掉这个死计数器与两处断言,并把文件头注释改为只声称钉住了序列化。约束:
PlanExecutionView.test.tsx:10-18从'./PlanExecutionView'导入layerPlanTodos等六个名字,而本 diff 在PlanExecutionView.tsx:30-32新增的注释说明 re-export 块正是「为了保持本模块对既有导入方(含测试)的公开面稳定」——任何抽取都必须保留这个 re-export。验收:修好之后,
expect(layersAfterMount).toBeGreaterThan(0)在挂载后必须成立;把layerPlanTodos(todos)调用移出useMemo到组件体,必须让 hover 之后的expect(counts.layers).toBe(layersAfterMount)变红。今天这两种变更都保持绿色,这就是缺口。— qwen3.8-max via Qwen Code /review (v0.23.0)