-
Notifications
You must be signed in to change notification settings - Fork 5.4k
fix(web): chat pane preserves scroll position when todo card grows #2299
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
Open
neogenix
wants to merge
9
commits into
nexu-io:main
Choose a base branch
from
eefynet:fix/web-chat-autoscroll
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
7168a5c
fix(web): chat pane preserves scroll position when todo card grows
neogenix 54125c6
fixup! fix(web): chat pane preserves scroll position when todo card g…
neogenix af9539a
fix(web): correctness extend MutationObserver to pane ancestor for Pi…
neogenix 96eb2cc
test(e2e): chat pane auto-scroll on todo card growth
neogenix 96384d8
docs(agents): note pinned-todo observer coverage in chat UI conventions
neogenix ba5b217
fix(web): validate OD_WORKSPACE_ROOT, harden autoscroll test precondi…
08a100a
fix(web): validate OD_WORKSPACE_ROOT existence, make autoscroll preco…
76eab2a
fix(web): throw on invalid OD_WORKSPACE_ROOT instead of warn-and-fall…
neogenix 7a1b1c8
fix(web): require pnpm-workspace.yaml at OD_WORKSPACE_ROOT, drop dead…
neogenix File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
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
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
217 changes: 217 additions & 0 deletions
217
apps/web/tests/components/chat-todo-autoscroll.test.tsx
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.