fix(web): chat pane preserves scroll position when todo card grows#2299
fix(web): chat pane preserves scroll position when todo card grows#2299neogenix wants to merge 9 commits into
Conversation
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.
lefarcen
added
size/L
|
|
||
| const WEB_ROOT = dirname(fileURLToPath(import.meta.url)); | ||
| const WORKSPACE_ROOT = dirname(dirname(WEB_ROOT)); | ||
| const WORKSPACE_ROOT = process.env.OD_WORKSPACE_ROOT ?? dirname(dirname(WEB_ROOT)); |
There was a problem hiding this comment.
OD_WORKSPACE_ROOT is used verbatim here, unlike the other path overrides in this file. That means an empty string or a relative path will flow straight into outputFileTracingRoot and turbopack.root, which makes the new worktree override fail in opaque ways instead of either resolving predictably or failing fast. Please normalize/validate this env var before using it here (for example: ignore blank values, and either resolve relative paths against WEB_ROOT or throw unless the override is absolute).
| // If the viewport is shorter than 150 px of content the scroll moved us away; | ||
| // if the chat-log has no scrollable room (few messages, large viewport) the | ||
| // scroll will be a no-op and the test is a guaranteed pass. Both are fine. | ||
| const scrollUpOccurred = distanceAfterScroll > 80; |
There was a problem hiding this comment.
This branch lets scenario B pass without asserting anything if the synthetic scroll-up does not move the log more than 80px from the bottom. That creates a false-green path where a later layout or seed change stops exercising the regression entirely, but the test still passes. Please make the precondition explicit instead: assert scrollable overflow and distanceAfterScroll > 80 before growPinnedTodo, so the spec fails fast when it no longer reaches the non-pinned path it is supposed to verify.
|
|
||
| function resolveWorkspaceRoot(): string { | ||
| const override = process.env.OD_WORKSPACE_ROOT; | ||
| if (override && override.trim()) { |
There was a problem hiding this comment.
OD_WORKSPACE_ROOT is 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 contain apps/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 in resolveWorkspaceRoot(): the override is trimmed and resolved, but never checked for existence or containment before being assigned to outputFileTracingRoot and turbopack.root. Please validate the resolved path here (for example, stat/realpath it and throw when it does not exist or when relative(resolvedRoot, WEB_ROOT) escapes the root) so this new override follows the repo's fail-fast rule.
| // scroll will be a no-op and the test is a guaranteed pass. Both are fine. | ||
| const scrollUpOccurred = distanceAfterScroll > 80; | ||
|
|
||
| if (scrollUpOccurred) { |
There was a problem hiding this comment.
Scenario B can still go false-green because the actual regression assertion only runs inside if (scrollUpOccurred). If a later layout, viewport, or seed change leaves distanceAfterScroll <= 80, this test now exits cleanly without ever exercising the "user scrolled up, do not yank them back" path it claims to cover. The new precondition check helps only after the branch is entered, so it does not close the original gap. Please make the precondition mandatory instead: assert scrollable overflow first, then fail if the manual scroll does not move the log beyond the 80px threshold (or keep scrolling until it does) before calling growPinnedTodo().
…ndition unconditional
| if (!existsSync(resolved)) { | ||
| console.warn( | ||
| `[next.config] OD_WORKSPACE_ROOT="${override}" resolved to "${resolved}" which does not exist; ` + | ||
| `falling back to computed default "${computed}".`, | ||
| ); | ||
| return computed; | ||
| } | ||
| const rel = relative(resolved, WEB_ROOT); | ||
| if (rel.startsWith('..')) { | ||
| console.warn( | ||
| `[next.config] OD_WORKSPACE_ROOT="${override}" resolved to "${resolved}" but WEB_ROOT "${WEB_ROOT}" ` + | ||
| `escapes it (relative path "${rel}"); falling back to computed default "${computed}".`, | ||
| ); | ||
| return computed; | ||
| } |
There was a problem hiding this comment.
resolveWorkspaceRoot() now treats an invalid OD_WORKSPACE_ROOT as a warning-and-fallback path instead of failing at config load. That matters because this PR introduced the override specifically to make worktree Playwright runs deterministic; a typo or stale path can silently switch outputFileTracingRoot/turbopack.root back to the computed default and send the run back to the original opaque resolution behavior. The evidence is in these branches: both the missing-directory case and the WEB_ROOT-outside-root case call console.warn(...) and return computed. Please replace those fallbacks with a thrown configuration error (or equivalent hard failure) so an explicitly provided but invalid OD_WORKSPACE_ROOT fails fast with a clear message.
| const rel = relative(resolved, WEB_ROOT); | ||
| 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.`, | ||
| ); | ||
| } | ||
| return resolved; |
There was a problem hiding this comment.
This validation still accepts values that are ancestors of apps/web but are not actually valid workspace roots for the web app. For example, OD_WORKSPACE_ROOT=<repo>/apps or <repo>/apps/web passes this relative(resolved, WEB_ROOT) check, but WORKSPACE_ROOT is then wired into outputFileTracingRoot and turbopack.root below while the app imports workspace packages from packages/* (for example @open-design/contracts in apps/web/src/components/ChatPane.tsx). That means the new override can still bless a broken path and send the worktree/dev run back to incomplete tracing or opaque module-resolution failures. Please validate the override as the actual repository workspace root instead of any ancestor of apps/web—for example require pnpm-workspace.yaml and/or the expected top-level workspace directories at resolved.
| const distanceAfterGrow = await chatLogBottomDistance(page); | ||
| // The scroll position should be roughly preserved (within 20 px tolerance | ||
| // for rounding or minor layout shifts the browser may apply). | ||
| expect( | ||
| distanceAfterGrow, | ||
| `expected scroll position preserved after todo card grew (before=${distanceAfterScroll} after=${distanceAfterGrow})`, | ||
| ).toBeGreaterThan(60); |
There was a problem hiding this comment.
This assertion is weaker than the contract the comment describes. The test says scenario B should preserve the user's scroll position within about 20px, but toBeGreaterThan(60) still passes if a regression drags the log most of the way back to the bottom—for example distanceAfterScroll = 150 and distanceAfterGrow = 61. That can false-green the new regression coverage even though the user's position changed materially. Please compare distanceAfterGrow against distanceAfterScroll directly (for example Math.abs(distanceAfterGrow - distanceAfterScroll) <= 20) instead of only checking that it stays somewhere above 60.
… 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'.
|
Fixed in 7a1b1c8 — addresses the three remaining @nettee findings:
|
| `Fix the path or unset the variable to use the computed default.`, | ||
| ); | ||
| } | ||
| const rel = relative(resolved, WEB_ROOT); |
There was a problem hiding this comment.
OD_WORKSPACE_ROOT was added here for worktree / symlinked checkouts, but this ancestor check is still purely lexical. On macOS /tmp vs /private/tmp (and the same kind of aliasing through symlinked worktree paths), both strings can name the same physical repo while path.relative() still returns a .. path, so this branch throws even though the override is a valid physical ancestor. The evidence is in the changed lines here: resolved is validated with relative(resolved, WEB_ROOT) and then used for the pnpm-workspace.yaml lookup without canonicalizing either side first. That means the new escape hatch can still reject exactly the non-standard checkout layouts it was introduced to support. Please canonicalize both resolved and WEB_ROOT with realpathSync before the ancestor and workspace-root checks, then validate the canonical paths instead.
3 participants
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.