Skip to content

Preserve chat scroll position while streaming new messages - #51

Merged
juliusmarminge merged 1 commit into
mainfrom
codething/25696837
Feb 15, 2026
Merged

juliusmarminge merged 1 commit into
mainfrom
codething/25696837

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Feb 15, 2026 •

Copy link
Copy Markdown
Member

Summary

  • add isScrollContainerNearBottom utility to detect whether the messages container is close enough to the bottom to auto-scroll
  • track user scroll intent in ChatView with shouldAutoScrollRef and an onScroll handler
  • only auto-scroll on message/worklog updates when the user is near the bottom; keep forced scroll on thread switch
  • replace direct scrollTop/scrollIntoView calls with a shared scrollMessagesToBottom helper
  • add focused unit tests for threshold behavior, invalid inputs, and default fallback logic in chat-scroll.test.ts

Testing

  • apps/web/src/chat-scroll.test.ts: verifies near-bottom detection at bottom, within threshold, above threshold, negative threshold clamping, and NaN fallback
  • Manual check (expected): while a run is streaming, scrolling up in chat should no longer jump back to bottom until user returns near bottom
  • Not run: project lint and full test suite

Open with Devin

Summary by CodeRabbit

  • New Features

    • Improved automatic scrolling behavior for chat messages. Chat now intelligently scrolls to the latest message while respecting user scroll position when reading earlier messages.
    • Added smooth scrolling transitions for a better visual experience.
  • Tests

    • Added test coverage for scroll behavior validation.

- only auto-scroll when the user is already near the bottom
- centralize near-bottom detection in `chat-scroll.ts` with a 64px threshold
- add unit tests covering threshold and edge-case behavior

Co-authored-by: codex <codex@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Feb 15, 2026 •

Copy link
Copy Markdown

Walkthrough

This PR adds a scroll detection utility to determine when a chat scroll container is near the bottom, includes comprehensive tests for the utility, and integrates this logic into ChatView to implement conditional auto-scrolling that respects user scroll position.

Changes

Cohort / File(s) Summary
Scroll Detection Utility
apps/web/src/chat-scroll.ts, apps/web/src/chat-scroll.test.ts
Introduces AUTO_SCROLL_BOTTOM_THRESHOLD_PX constant (64px) and isScrollContainerNearBottom() function to detect scroll proximity. Tests validate threshold handling, negative value clamping, NaN fallback behavior, and distance calculations.
Chat Auto-Scroll Integration
apps/web/src/components/ChatView.tsx
Implements conditional auto-scrolling using scroll detection. Replaces unconditional scroll-on-update with smart scrolling via shouldAutoScrollRef state that tracks user scroll position and triggers smooth scrolling only when appropriate (near bottom or on thread change).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ChatView
    participant ScrollUtil as Scroll Utility
    participant DOM

    User->>DOM: Scrolls manually
    DOM->>ChatView: onScroll event fires
    ChatView->>ScrollUtil: isScrollContainerNearBottom()
    ScrollUtil->>ScrollUtil: Calculate distance from bottom
    ScrollUtil-->>ChatView: Returns boolean
    ChatView->>ChatView: Update shouldAutoScrollRef

    Note over ChatView: New message arrives
    ChatView->>ScrollUtil: isScrollContainerNearBottom()
    ScrollUtil-->>ChatView: Returns true (near bottom)
    ChatView->>DOM: smoothScrollToBottom()
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Preserve chat scroll position while streaming new messages' directly reflects the main change: implementing conditional auto-scroll behavior that respects user scroll intent rather than unconditionally jumping to the bottom when new messages arrive.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codething/25696837

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
apps/web/src/chat-scroll.test.ts (1)

52-64: Consider adding a test for non-finite position metrics.

The implementation returns true when position metrics are non-finite (Lines 18-20 in chat-scroll.ts), but this behavior isn't tested. Adding a test would document this defensive behavior.

📝 Suggested test case
+  it("returns true when position metrics are non-finite", () => {
+    expect(
+      isScrollContainerNearBottom({
+        scrollTop: Number.NaN,
+        clientHeight: 400,
+        scrollHeight: 1_000,
+      }),
+    ).toBe(true);
+  });

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented Feb 15, 2026

Copy link
Copy Markdown

Greptile Summary

This PR implements smart auto-scroll behavior for the chat interface that respects user scroll position. Previously, the chat would aggressively scroll to bottom on every message update, disrupting users trying to review earlier messages during streaming responses.

Key changes:

  • Added isScrollContainerNearBottom utility to detect if user is within 64px of bottom
  • Tracks user scroll intent in shouldAutoScrollRef updated via onScroll handler
  • Only auto-scrolls on message/worklog updates when user is near bottom
  • Always scrolls to bottom on thread switch (correct UX)
  • Consolidated scroll logic into scrollMessagesToBottom helper
  • Comprehensive unit tests cover threshold behavior and edge cases

Confidence Score: 5/5

  • This PR is safe to merge with no issues identified
  • Clean implementation with proper edge case handling, comprehensive tests, and clear UX improvement. No bugs or security concerns identified. Code follows React best practices using refs for non-render state and proper hook dependencies.
  • No files require special attention

Important Files Changed

Filename Overview
apps/web/src/chat-scroll.test.ts Added comprehensive unit tests for scroll position detection utility with edge cases
apps/web/src/chat-scroll.ts New utility to detect when scroll container is near bottom with threshold and fallback handling
apps/web/src/components/ChatView.tsx Implemented smart auto-scroll with user intent tracking via shouldAutoScrollRef and scroll handler

Flowchart

flowchart TD
    A[User Action] --> B{Thread Switch?}
    B -->|Yes| C[scrollMessagesToBottom - instant]
    B -->|No| D{New Message or WorkLog?}
    D -->|Yes| E{shouldAutoScrollRef.current?}
    E -->|true| F[scrollMessagesToBottom - smooth]
    E -->|false| G[No scroll - preserve position]
    D -->|No| G
    
    H[onMessagesScroll Event] --> I[isScrollContainerNearBottom]
    I --> J{Within 64px of bottom?}
    J -->|Yes| K[shouldAutoScrollRef = true]
    J -->|No| L[shouldAutoScrollRef = false]
    
    C --> M[Set shouldAutoScrollRef = true]
    F --> M
Loading

Last reviewed commit: 0476e50

@macroscopeapp

macroscopeapp Bot commented Feb 15, 2026

Copy link
Copy Markdown
Contributor

Preserve chat scroll position in ChatView by auto-scrolling only when near bottom using a 64px threshold

Add AUTO_SCROLL_BOTTOM_THRESHOLD_PX (64) and isScrollContainerNearBottom in apps/web/src/chat-scroll.ts; update ChatView to compute shouldAutoScrollRef on scroll and call scrollMessagesToBottom only when within threshold; add tests in apps/web/src/chat-scroll.test.ts.

📍Where to Start

Start with isScrollContainerNearBottom in apps/web/src/chat-scroll.ts, then review how ChatView uses it in apps/web/src/components/ChatView.tsx.


Macroscope summarized 0476e50.

@juliusmarminge
juliusmarminge merged commit 7fc53da into main Feb 15, 2026
4 checks passed
piero-dev25 added a commit to piero-dev25/devgame that referenced this pull request Aug 4, 2026
Two tasks in one commit because both edit ChatView.tsx and cannot be separated
by pathspec.

DIFF AS A DOCK PANEL (pingdotgg#56). Part B of our own frozen spec, deferred since Part A
shipped. The app had TWO tab systems on screen -- the dockview grid, and a
second tab set inside the single chat panel. Diff moves first because it already
resolved its own thread identity from the URL rather than taking it from
ChatView's scope; the other three take a dozen callbacks each and will need
rewriting.

The structural problem worth recording: ChatView is a DESCENDANT of the dock
(ChatDock -> ChatPanel -> ChatView), so the components that need to open a panel
cannot receive a ref to the thing containing them. Solved with a module-scope
handle registered on mount and cleared on unmount, matching the two singletons
already in that file. It warns loudly rather than no-opping silently when null.

THE MIGRATION IS PROVEN COMPLETE, NOT ASSERTED. A review found Cmd/Ctrl+D still
driving the old store -- and because the render case had been correctly removed
while the tab strip still knew how to draw a Diff tab, the shortcut produced a
VISIBLE TAB THAT RENDERED NOTHING. Third instance of "looks wired, does nothing"
in this repo.

The fix was not to patch two call sites. `"diff"` is gone from RightPanelKind
and RightPanelSurface, so the compiler enumerated every straggler: exactly six,
across two files, all genuinely dead. This is the template for Files, Terminal
and Browser -- move the surface, delete its kind, and completeness becomes a
build error rather than something a human eyeballed. The one place that must
still recognise the retired kind is the persistence migration, which casts past
the union with a comment explaining that handling data older than the current
type is its entire job.

Cmd+D also lost its toggle in the first pass -- open-and-focus with no close.
The lane flagged it rather than letting me find it. Not accepted: the keybinding
is named `diff.toggle`, the handler is named onToggleDiff, and "use the tab x"
defeats the point of a keyboard shortcut. Now a genuine toggle, with the CLOSE
half specifically mutation-proven, because a toggle that only ever opens is the
exact bug being fixed. The layout-persistence risk was checked rather than
assumed: `persist()` stamps knownPanelIds from the current catalog on every
save, so a close survives reload as "closed on purpose" instead of being
re-grafted.

Storage v7->8 strips persisted "diff" surfaces, and recomputes `isOpen` from
surviving surfaces -- without that, a user whose ONLY surface was diff resumed
into an empty right panel they never asked for. Invisible to the first test
because its fixture always had a second surface.

THREE.JS PLAY (pingdotgg#51). `autoOpenPreview` has been declared in contracts, collected
by the settings form, and persisted to disk since it was written -- and nothing
ever read it. A user could tick the box, save, and nothing happened. It now
opens the preview when a script with a previewUrl starts.

It waits for the port scanner to report a listener attributed BY PID to the
exact terminal the script ran in, then opens the CONFIGURED url rather than the
scanner's reported one -- those differ when a port gets bumped, and the user
configured what they configured. Guarded on desktop before anything starts, so
in a browser it genuinely does nothing rather than failing gracefully.

Its mutation proof is the right shape: deleting the open call while still
returning `opened: true` turns the test red with zero calls to the mock -- the
exact "looks wired, does nothing" shape this task exists to fix, reproduced
deliberately to prove the test catches it.

Full typecheck, npm test and fmt:check green. Live browser E2E is an OPEN GATE,
tracked for pingdotgg#55 -- the dock is half migrated, so a live pass today would measure
a transitional state.
aorwall added a commit to aorwall/t3code that referenced this pull request Aug 6, 2026
…gdotgg#58)

* feat(settings): build the Secrets administration page in T3

Replace the Secrets NotInT3Yet placeholder with a real panel: a Global
and a Personal section, each listing its scope's secrets with an enable
switch, edit, and delete, plus a create/edit dialog. Reads go through the
shared moatless query helpers keyed under `secrets/{scope}`, so a write
invalidates both scopes together.

The row-derivation logic (kind labels, active-first ordering) lives in a
pure `secretRows.ts` with co-located tests; the panel only reads atoms and
renders T3's own settings primitives.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(settings): build the Users administration page in T3

Replace the Users NotInT3Yet placeholder with a list and a detail page.
The list shows everyone with an account — monogram, name, admin and bot
tags — and leads to one user's page. The detail page edits name, email,
and the global role with dirty tracking and an explicit save, and shows
the read-only account identity beneath it.

There is no single-user read on the wire, so the detail page reads the
same list the index does and finds its login in it; the two share the
`users` key so a role change refetches both at once. Row derivation
(display name, monogram, humans-before-bots ordering) is a pure
`userRows.ts` with co-located tests.

The detail route uses the trailing-underscore file form so it does not
nest under the list route, and is recorded in the merge inventory.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(settings): build the Moatless Loops administration pages in T3

Add the Loops list and Loop detail settings surfaces, replacing the
NotInT3Yet placeholders. The list shows each loop's state, git provenance
and source summary, and links to a detail page that edits the loop's
general fields, configuration and schedule with the workspace SaveBar
pattern, exposes lifecycle actions (pause/resume/approve-and-activate),
git override/restore, and delete. Source derivation and provenance live
in a pure `loopRows.ts` module with co-located tests.

Detail route uses the underscore form to avoid nested-route rendering.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(settings): build the Moatless Integrations administration page in T3

Replace the NotInT3Yet placeholder on /settings/integrations with a panel
of three sections: Connections (adapter connections a loop can subscribe
to — listed, created, and opened to a detail page that removes them),
Apps (the configured adapter apps and which of each app's secrets are
set, read-only), and GitHub (registered GitHub apps, their installations
and the default installation, read-only). Naming, ordering and secret
fingerprint derivation live in a pure integrationRows.ts module with
co-located tests.

Connection detail route uses the underscore form to avoid nested-route
rendering.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(settings): administer skills plugins and their activation in T3

Adds the Skills administration surface: a plugin catalog and, per plugin,
the skills it sources with a control for the deployment-wide default and the
viewer's own override, side by side.

The delivered badge is read straight from /plugins/effective rather than
recomputed from the two controls — off-for-everyone-plus-on-for-you is the
server's call to resolve, so a skill can read "Not set / Not set" and still be
Delivered because its plugin is on. Precedence lives in skillRows.ts behind a
test that pins the source-off-with-skill-on and unset-versus-off cases.

Detail route uses the underscore form so it does not nest under the list panel.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(spec): mark moatless-admin spec in review and correct stale claims

All five administration surfaces are now built and under review as PRs
pingdotgg#51–pingdotgg#55, so the spec no longer reads "in progress". Records the merge order
the shared queries.ts forces, and the two follow-ups that land once the five
merge (delete NotInT3Yet, move this spec to .plans/completed/).

Also fixes two claims that drifted from what shipped: the acceptance criterion
still said the not-built placeholder links into the SPA, which the placeholder
decision earlier in the same doc deliberately reversed; and two seam-table
module paths pointed at apps/web/src/moatless/ rather than where the pure
modules actually landed under components/settings/moatless/.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(spec): unbreak the surfaces table split by the route-convention note

The trailing-underscore route-convention paragraph sat between two table
rows, so Markdown ended the table at it and rendered every row below —
loops through users, ten of the twelve surfaces — as literal pipes. Move the
paragraph below the last row so the table stays one block.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore(settings): retire the NotInT3Yet placeholder and archive the spec

Every administration surface now has a real panel, so no route renders the
placeholder. Its own comment said to delete the file when the last usage went;
this is that moment. The merge-inventory row stops describing a build where
some entries lead to a placeholder.

Neither of these could be done while the surfaces were five separate branches —
each still needed the placeholder for the other four. Combining them into one
change is what makes both possible, so they land here.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(fork): list the merge inventory as entries, not tables

The inventory conflicted on every branch that touched it, and almost none of
that was disagreement. The formatter re-pads a Markdown table across all of its
rows when one cell changes width, so adding a single route path to the
administration-pages row rewrote all 60 lines of the block; two branches editing
two unrelated entries then conflicted on every line between them.

Measured on this file, adding one route path to the administration pages:
60 changed lines as a table, 1 as a list.

So the tables are gone. Every section is a list of entries: a title line, an
optional nested `Paths:` list with one path per line, and the rule. The
formatter leaves list items and prose alone — verified by running `vp fmt` over
the result and getting a byte-identical file back. The header paragraph says so,
so this does not get turned back into a table.

Content is carried over unchanged. Verified by extracting every backticked path
from both versions: 117 before, 117 after, none lost and none invented. Three
deliberate edits beside the reformat:

- "the three fork plans above" now says four; that entry has listed four plans
  since the administration spec was archived.
- "row" means a table row, and there are no tables now, so policy entries and
  the skill's vocabulary say "entry" — including the `decide, then add an entry`
  policy value the skill defines.
- `.agents/skills/fork-upstream-merge/**` gets a path-policy entry. It is
  fork-authored and was in no entry at all; upstream owns the other four skills
  under `.agents/skills/` and adds more, so a blanket `ours` there would be
  wrong. Checked against a shallow `upstream/main` fetch, which also confirmed
  the two counts this file quotes are still current: 32 of 36 `.plans/` files
  and 34 of 39 under `apps/web/src/browser/`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
maria-rcks pushed a commit to maria-rcks/t3libre that referenced this pull request Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant