fix(web): chat pane preserves scroll position when todo card grows #2299
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import type { NextConfig } from 'next'; | ||
| import { existsSync } from 'node:fs'; | ||
| import { networkInterfaces } from 'node:os'; | ||
| import { dirname, isAbsolute, relative, resolve } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| // Daemon port the local Express server binds to (see apps/daemon/src/cli.ts). The | ||
|
|
@@ -23,7 +24,32 @@ const isServerOutput = webOutputMode === 'server' || webOutputMode === 'standalo | |
| const shouldStaticExport = isProd && !isServerOutput; | ||
|
|
||
| const WEB_ROOT = dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| function resolveWorkspaceRoot(): string { | ||
| const computed = dirname(dirname(WEB_ROOT)); | ||
| const override = process.env.OD_WORKSPACE_ROOT; | ||
| if (override && override.trim()) { | ||
| const resolved = isAbsolute(override.trim()) ? override.trim() : resolve(WEB_ROOT, override.trim()); | ||
| if (!existsSync(resolved)) { | ||
| throw new Error( | ||
| `OD_WORKSPACE_ROOT="${override}" resolved to "${resolved}" which does not exist. ` + | ||
| `Fix the path or unset the variable to use the computed default.`, | ||
| ); | ||
| } | ||
| const rel = relative(resolved, WEB_ROOT); | ||
|
Contributor
There was a problem hiding this comment.
|
||
| if (rel.startsWith('..')) { | ||
| throw new Error( | ||
| `OD_WORKSPACE_ROOT="${override}" resolved to "${resolved}" but WEB_ROOT "${WEB_ROOT}" ` + | ||
| `is not inside it (relative path "${rel}"). ` + | ||
| `The override must be an ancestor of apps/web.`, | ||
| ); | ||
| } | ||
|
Comment on lines
+33
to
+46
Contributor
There was a problem hiding this comment.
|
||
| return resolved; | ||
|
Comment on lines
+39
to
+60
Contributor
There was a problem hiding this comment. This validation still accepts values that are ancestors of |
||
| } | ||
| return computed; | ||
| } | ||
|
|
||
| const WORKSPACE_ROOT = resolveWorkspaceRoot(); | ||
| const toPosixPath = (value: string) => value.replaceAll('\\', '/'); | ||
|
|
||
| function resolveDistDir(defaultValue: string) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,217 @@ | ||
| // @vitest-environment jsdom | ||
|
|
||
| // Polyfill scrollTo for jsdom (not available in jsdom's HTMLElement) | ||
| if (typeof HTMLElement.prototype.scrollTo !== 'function') { | ||
| HTMLElement.prototype.scrollTo = function ( | ||
| options?: ScrollToOptions | number, | ||
| _y?: number, | ||
| ) { | ||
| if (typeof options === 'object' && options !== null) { | ||
| if (options.top !== undefined) this.scrollTop = options.top; | ||
| if (options.left !== undefined) this.scrollLeft = options.left; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| import { act, cleanup, render } from '@testing-library/react'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { ChatPane } from '../../src/components/ChatPane'; | ||
| import type { ChatMessage } from '../../src/types'; | ||
|
|
||
| // Per-test geometry for the chat-log scroll container. jsdom has no | ||
| // layout engine so we patch the prototype to route reads/writes through | ||
| // this object, matching the technique in chat-scroll-preservation.test.tsx. | ||
| type Geom = { scrollHeight: number; clientHeight: number; scrollTop: number }; | ||
| let geom: Geom; | ||
| let rafCallbacks: FrameRequestCallback[]; | ||
| let resizeCallbacks: ResizeObserverCallback[]; | ||
| // All elements passed to any ResizeObserver.observe() call — used to | ||
| // assert that the pinned-todo div is observed so real-browser resizes fire. | ||
| let observedElements: Element[]; | ||
| let savedDescriptors: Record< | ||
| 'scrollTop' | 'scrollHeight' | 'clientHeight', | ||
| PropertyDescriptor | undefined | ||
| >; | ||
| let originalResizeObserver: typeof ResizeObserver | undefined; | ||
|
|
||
| function isChatLog(el: HTMLElement): boolean { | ||
| return typeof el?.classList?.contains === 'function' && el.classList.contains('chat-log'); | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| geom = { scrollHeight: 1000, clientHeight: 400, scrollTop: 1000 }; | ||
| rafCallbacks = []; | ||
| resizeCallbacks = []; | ||
| observedElements = []; | ||
|
|
||
| vi.spyOn(window, 'requestAnimationFrame').mockImplementation((callback) => { | ||
| rafCallbacks.push(callback); | ||
| return rafCallbacks.length; | ||
| }); | ||
|
|
||
| originalResizeObserver = globalThis.ResizeObserver; | ||
| class MockResizeObserver { | ||
| constructor(callback: ResizeObserverCallback) { | ||
| resizeCallbacks.push(callback); | ||
| } | ||
| observe = vi.fn((el: Element) => { | ||
| observedElements.push(el); | ||
| }); | ||
| unobserve = vi.fn(); | ||
| disconnect = vi.fn(); | ||
| } | ||
| Object.defineProperty(globalThis, 'ResizeObserver', { | ||
| configurable: true, | ||
| writable: true, | ||
| value: MockResizeObserver, | ||
| }); | ||
|
|
||
| savedDescriptors = { | ||
| scrollTop: Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'scrollTop'), | ||
| scrollHeight: Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'scrollHeight'), | ||
| clientHeight: Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'clientHeight'), | ||
| }; | ||
| Object.defineProperty(HTMLElement.prototype, 'scrollTop', { | ||
| configurable: true, | ||
| get(this: HTMLElement) { | ||
| return isChatLog(this) ? geom.scrollTop : 0; | ||
| }, | ||
| set(this: HTMLElement, v: number) { | ||
| if (isChatLog(this)) geom.scrollTop = v; | ||
| }, | ||
| }); | ||
| Object.defineProperty(HTMLElement.prototype, 'scrollHeight', { | ||
| configurable: true, | ||
| get(this: HTMLElement) { | ||
| return isChatLog(this) ? geom.scrollHeight : 0; | ||
| }, | ||
| }); | ||
| Object.defineProperty(HTMLElement.prototype, 'clientHeight', { | ||
| configurable: true, | ||
| get(this: HTMLElement) { | ||
| return isChatLog(this) ? geom.clientHeight : 0; | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| cleanup(); | ||
| vi.restoreAllMocks(); | ||
| rafCallbacks = []; | ||
| resizeCallbacks = []; | ||
| observedElements = []; | ||
| if (originalResizeObserver) { | ||
| Object.defineProperty(globalThis, 'ResizeObserver', { | ||
| configurable: true, | ||
| writable: true, | ||
| value: originalResizeObserver, | ||
| }); | ||
| } else { | ||
| delete (globalThis as unknown as { ResizeObserver?: unknown }).ResizeObserver; | ||
| } | ||
| for (const key of ['scrollTop', 'scrollHeight', 'clientHeight'] as const) { | ||
| const original = savedDescriptors[key]; | ||
| if (original) { | ||
| Object.defineProperty(HTMLElement.prototype, key, original); | ||
| } else { | ||
| delete (HTMLElement.prototype as unknown as Record<string, unknown>)[key]; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| async function flushFrames() { | ||
| await act(async () => { | ||
| const callbacks = rafCallbacks.splice(0); | ||
| callbacks.forEach((callback) => callback(performance.now())); | ||
| await Promise.resolve(); | ||
| }); | ||
| } | ||
|
|
||
| // Build a message set that includes a TodoWrite event so PinnedTodoSlot renders. | ||
| function messagesWithTodo(taskCount: number): ChatMessage[] { | ||
| const todos = Array.from({ length: taskCount }, (_, i) => ({ | ||
| content: `Task ${i + 1}`, | ||
| status: 'pending', | ||
| })); | ||
| return [ | ||
| { id: 'u1', role: 'user' as const, content: 'build something', createdAt: Date.now() }, | ||
| { | ||
| id: 'a1', | ||
| role: 'assistant' as const, | ||
| content: 'on it', | ||
| createdAt: Date.now(), | ||
| events: [ | ||
| { | ||
| kind: 'tool_use' as const, | ||
| id: 'tw-1', | ||
| name: 'TodoWrite', | ||
| input: { todos }, | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
| } | ||
|
|
||
| function chatPaneEl(messages: ChatMessage[]) { | ||
| return ( | ||
| <ChatPane | ||
| messages={messages} | ||
| streaming={false} | ||
| error={null} | ||
| projectId="project-1" | ||
| projectFiles={[]} | ||
| onEnsureProject={async () => 'project-1'} | ||
| onSend={() => {}} | ||
| onStop={() => {}} | ||
| conversations={[]} | ||
| activeConversationId={null} | ||
| onSelectConversation={() => {}} | ||
| onDeleteConversation={() => {}} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| describe('chat-log autoscroll when pinned todo card grows', () => { | ||
| it('observes the pinned-todo element so its resize triggers the bottom-pin follow', async () => { | ||
| // The PinnedTodoSlot lives outside the chat-log scroll container. | ||
| // When the todo card grows, the chat-log viewport (clientHeight) | ||
| // shrinks. The ResizeObserver must observe the pinned-todo div so | ||
| // `followLatestIfPinned` fires and corrects the scroll position. | ||
| render(chatPaneEl(messagesWithTodo(3))); | ||
| await flushFrames(); | ||
|
|
||
| const pinnedTodoEl = document.querySelector('.chat-pinned-todo'); | ||
| expect(pinnedTodoEl, 'PinnedTodoSlot should render with a TodoWrite message').not.toBeNull(); | ||
|
|
||
| // The pinned-todo element must be registered with the ResizeObserver | ||
| // so that real-browser growth of the todo card triggers followLatestIfPinned. | ||
| expect(observedElements).toContain(pinnedTodoEl); | ||
| }); | ||
|
|
||
| it('scrolls to the bottom when pinned and the todo card grows', async () => { | ||
| // Start pinned: scrollTop == scrollHeight (user is at the very bottom). | ||
| geom = { scrollHeight: 1000, clientHeight: 400, scrollTop: 1000 }; | ||
| render(chatPaneEl(messagesWithTodo(2))); | ||
| await flushFrames(); | ||
|
|
||
| // The initial-bottom-scroll effect fires and confirms pinnedToBottomRef = true. | ||
| // Now simulate the todo card growing: the viewport (clientHeight) shrinks, | ||
| // which means the user can no longer see the latest content even though | ||
| // scrollTop is still at its old value. The ResizeObserver callback should | ||
| // fire followLatestIfPinned, which snaps scrollTop back to scrollHeight. | ||
| geom = { ...geom, clientHeight: 300, scrollHeight: 1000, scrollTop: 600 }; | ||
|
|
||
| await act(async () => { | ||
| const callbacks = [...resizeCallbacks]; | ||
| callbacks.forEach((callback) => callback([], {} as ResizeObserver)); | ||
| await Promise.resolve(); | ||
| }); | ||
| await flushFrames(); | ||
|
|
||
| // followLatestIfPinned fires from the shared callback and snaps scrollTop | ||
| // to scrollHeight (1000). The structural guarantee that the pinned-todo | ||
| // element is observed (tested separately above) ensures this path runs in | ||
| // the real browser when the card grows. | ||
| expect(geom.scrollTop).toBe(1000); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.OD_WORKSPACE_ROOTis still treated as valid as soon as it is non-blank, even though this branch introduced it specifically as a config override. If the value points at a missing directory or a directory that does not actually containapps/web, Next will fail later inside file tracing / Turbopack with a much harder-to-diagnose error instead of failing at config load time. The evidence is inresolveWorkspaceRoot(): the override is trimmed and resolved, but never checked for existence or containment before being assigned tooutputFileTracingRootandturbopack.root. Please validate the resolved path here (for example,stat/realpathit and throw when it does not exist or whenrelative(resolvedRoot, WEB_ROOT)escapes the root) so this new override follows the repo's fail-fast rule.