fix(web): chat pane preserves scroll position when todo card grows#2299
Conversation
|
Fixed in 7a1b1c8 — addresses the three remaining @nettee findings:
|
7a1b1c8 to
ddad708
Compare
nettee
left a comment
There was a problem hiding this comment.
@neogenix I reviewed the current head's ChatPane observer wiring, the OD_WORKSPACE_ROOT guard, and the new jsdom/Playwright regression coverage. The mount/unmount observer path and both pinned/non-pinned scroll invariants now line up with the implementation, and I don't see any remaining actionable issues in the changed ranges. Thanks for tightening both the behavior fix and the regression coverage here.
The PinnedTodoSlot renders outside the chat-log scroll container. When the todo card grows (new tasks added via TodoWrite), the scroll container's clientHeight shrinks in the flex layout, drifting the user away from the bottom. The existing ResizeObserver only observed children of the chat-log div, so pinned-todo growth was invisible to followLatestIfPinned. Fix: pass a containerRef to PinnedTodoSlot and observe that element in the same ResizeObserver. syncPinnedTodo() is called on effect setup and from the MutationObserver callback so observation stays current as the slot appears and disappears across TodoWrite snapshots. Red spec: apps/web/tests/components/chat-todo-autoscroll.test.tsx
…rows Clarify test comment: the second test confirms followLatestIfPinned snaps scroll to bottom when fired. The structural guarantee (pinned-todo element is observed) is separately asserted in test 1, which is the check that goes red on main without the fix.
…nnedTodoSlot mount detection The MutationObserver was only watching the .chat-log element. PinnedTodoSlot (.chat-pinned-todo) is a sibling of .chat-log-wrap inside .pane, outside the observed subtree. syncPinnedTodo inside the MutationObserver callback was therefore dead code for mount/unmount transitions of the slot. Add a second observation on paneEl (el.parentElement?.parentElement) with childList-only so the MutationObserver fires when PinnedTodoSlot mounts or unmounts and syncPinnedTodo can register/deregister the element with the ResizeObserver.
Add Playwright spec that goes red on origin/main and green on this fix branch. Scenario A asserts that a chat-log pinned to the bottom snaps back after the PinnedTodoCard grows (the ResizeObserver-on-pinned-todo path). Scenario B asserts that a deliberate scroll-up is not overridden. Also allow OD_WORKSPACE_ROOT env override in next.config.ts so Turbopack resolves node_modules correctly when the web app is booted from a worktree whose node_modules symlinks resolve outside the default workspace root.
PinnedTodoSlot sits outside the .chat-log scroll container, so the ResizeObserver and MutationObserver coverage that keeps auto-scroll working when the todo card grows is non-obvious to future implementers. Document the invariant in the Chat UI conventions section.
…ndition unconditional
… test branch Three follow-ups to nettee's review feedback: 1. apps/web/next.config.ts gains a pnpm-workspace.yaml existence check after the relative-path validation. Without it, an override like '<repo>/apps' or '<repo>/apps/web' passes the relative(resolved, WEB_ROOT) check but the resolved path is missing the sibling packages/* directory that apps/web imports from (for example @open-design/contracts). Next would later fail deep inside file tracing / Turbopack with a much harder-to-diagnose error. Now we throw at config load with a clear message. 2. e2e/ui/chat-todo-autoscroll.test.ts drops the redundant 'if (scrollUpOccurred)' branch. The hard precondition above it already guarantees distanceAfterScroll > 80, so the if was dead code that read as a false-green path. The body now runs unconditionally. 3. Same test tightens the post-grow assertion. The previous toBeGreaterThan(60) would pass even if a regression dragged the log most of the way back to the bottom (e.g. before=150, after=61). Replaced with Math.abs(distanceAfterGrow - distanceAfterScroll) less than SCROLL_PRESERVATION_TOLERANCE_PX (20) — a delta check that actually verifies the comment's claim of 'within ~20px of where the user left it'.
…cenario B assertion - Use realpathSync on both resolved and WEB_ROOT before the ancestor check so that symlinked paths (macOS /tmp vs /private/tmp, worktree checkouts) compare correctly instead of false-throwing on a physically valid override. - Add isAbsolute(rel) guard for the Windows cross-drive case where path.relative() returns an absolute path instead of a ..-prefixed string. - Scenario B: replace distance-to-bottom delta assertion with scrollTop preservation check. Growing the pinned todo naturally increases distance-to-bottom by ~extraPx (clientHeight shrinks while scrollTop is held fixed), so the old Math.abs(after - before) < 20 check would fail on correct behavior. asserting scrollTop directly catches the real regression: followLatestIfPinned incorrectly snapping a non-pinned user back to the bottom. - Add hard precondition that clientHeight actually changed so the test fails fast if the layout stops exercising the non-pinned path.
ef67f4e to
698e9c5
Compare
nettee
left a comment
There was a problem hiding this comment.
@neogenix I reviewed the current head's ChatPane observer wiring, the OD_WORKSPACE_ROOT guard, and the new jsdom/Playwright regression coverage. Together with the current green CI matrix, the changed ranges look consistent and I don't see any remaining actionable issues here. Thanks for tightening both the autoscroll fix and its regression coverage.
|
Hey @neogenix! 🎉 This is live on The regression coverage earns its weight here. The jsdom unit spec verifying the new ref path in Thanks for working through all the iteration rounds with @nettee and landing this in good shape. ❤️ |
















































Why
While running a complex session with the agent making frequent TodoWrite
updates, I noticed the chat pane stopped following the latest output
whenever a new task was added to the pinned TodoCard. The chat log
silently drifted off-bottom even though the user had not scrolled.
Tracing it:
PinnedTodoSlotrenders OUTSIDE the.chat-logscrollcontainer — it sits as a sibling of
.chat-log-wrapinside.pane, NOTas a child. The existing
ResizeObserverinChatPane.tsxonly observedchildren of
.chat-log, so when the todo card grew, the chat log'sclientHeightshrank in the flex layout and the user drifted away fromthe bottom without
followLatestIfPinnedever firing.The existing scroll-pin logic (
pinnedToBottomRef, the 80 px cutoff,the "preserve position if user scrolled up" branch) was already correct.
The ResizeObserver just wasn't seeing the source of the resize.
What users will see
When TodoWrite emits a new snapshot that adds items to the pinned task
card, the chat log auto-scrolls to keep the latest output visible — but
only when the user is already pinned near the bottom (within 80 px). If
the user has scrolled up to read earlier output, their position is
preserved exactly as before. This is the same smart-follow behavior
Discord and terminal apps use; no new setting needed.
Surface area
No new CLI subcommand needed: this is a pure UI behavior change, not a
new capability. The chat pane already exists; only its auto-scroll
trigger surface was incomplete.
Screenshots
The fix is a behavior change visible only in motion. Reproduction: start
any long agent session that calls TodoWrite multiple times, watch the
chat log, observe that on
mainthe scroll drifts up as the todo cardgrows; on this branch the chat log stays pinned to bottom unless the
user has deliberately scrolled up.
Bug fix verification
apps/web/tests/components/chat-todo-autoscroll.test.tsxobservedElementscontains the.chat-pinned-tododiv.main: structural failure (the new ref path doesn't exist).e2e/ui/chat-todo-autoscroll.test.tsRed on
mainwithdistance = 995 px, green here withdistance = 0.subsequent TodoCard growth.
Validation
pnpm guard(clean)pnpm typecheck(clean)pnpm --filter @open-design/web test— 9/9 chat-suite tests pass.32 unrelated pre-existing failures in
useCritiqueTheaterEnabled,AssistantMessage,DesignsTab.select-mode,SettingsDialog.execution,WorkspaceTabsBarare present onmainand unchanged here.pnpm exec playwright test -c playwright.config.ts chat-todo-autoscroll— 2 scenarios pass (browser-verified in Chromium).
Adjacent issues (out of scope)
ChatPane.tsx(lines ~423 and~466) ignore
prefers-reduced-motion. Not introduced here; deferredas a separate a11y pass.
apps/web/next.config.tsgains a smallOD_WORKSPACE_ROOTenv-varfallback so Turbopack can resolve
apps/web/node_moduleswhen thePlaywright harness runs from a worktree whose
node_modulesis asymlink pointing outside the worktree root. Default behavior is
unchanged (the override only fires when the env var is explicitly
set). CI runs from the main checkout where the default already
resolves correctly. Included here only because the
e2e/ui/Playwright spec requires it during local-worktree dev.
AGENTS.md"Chat UI conventions" gains one sentence documenting thePinnedTodoSlot observer coverage so future implementers don't trip
over the same out-of-subtree gotcha.