fix(chat): pin all chat panels to bottom across async layout shifts#598
fix(chat): pin all chat panels to bottom across async layout shifts#598b-client-vm wants to merge 3 commits into
Conversation
Closes spacedriveapp#594. The previous useEffect-based scroll missed several height-changing events: tool result expansion (status flip without array length change), the ThinkingIndicator toggle (derived state, not a dep), and async markdown reflow (highlighter, fonts, image loads finish after the scroll has run). behavior: "smooth" also raced the layout shifts and landed short. Replaced with a ResizeObserver-based hook (useStickToBottom): - Observes the messages content directly, so any height change re-pins uniformly — no need to enumerate dep-array entries per layout source. - Tracks "near bottom" (within 64px) on scroll. New content only re-pins when the user is already at the bottom, so scrolling up to read history is preserved. - Uses behavior: "auto" so layout shifts during the scroll can't make it land short — the next observed shift snaps us forward again. Removes the old messagesEndRef sentinel; the ResizeObserver tracks the content element's height directly.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR replaces a useEffect/scrollIntoView approach with a new useStickToBottom hook that uses a ResizeObserver and a pinned-state heuristic, and integrates it into CortexChatPanel and PortalTimeline via dedicated scrollRef and contentRef, removing the previous bottom sentinel element. ChangesAuto-Scroll Stick-to-Bottom Behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
interface/src/hooks/useStickToBottom.ts (1)
37-37: 💤 Low valueOptional: Make scrollTop assignment more explicit.
Setting
scroll.scrollTop = scroll.scrollHeightworks because browsers clamp to the maximum valid value (scrollHeight - clientHeight), but the intent would be clearer as:- scroll.scrollTop = scroll.scrollHeight; + scroll.scrollTop = scroll.scrollHeight - scroll.clientHeight;This is purely a readability suggestion—the current code is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interface/src/hooks/useStickToBottom.ts` at line 37, The current assignment in useStickToBottom (setting scroll.scrollTop) should be made explicit: compute the target as scroll.scrollHeight minus scroll.clientHeight and assign that value to scroll.scrollTop, clamping with Math.max to avoid negative values (i.e., use Math.max(0, scroll.scrollHeight - scroll.clientHeight)); update the assignment at the line that currently sets scroll.scrollTop to scroll.scrollHeight to use this explicit, clamped value for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@interface/src/hooks/useStickToBottom.ts`:
- Line 37: The current assignment in useStickToBottom (setting scroll.scrollTop)
should be made explicit: compute the target as scroll.scrollHeight minus
scroll.clientHeight and assign that value to scroll.scrollTop, clamping with
Math.max to avoid negative values (i.e., use Math.max(0, scroll.scrollHeight -
scroll.clientHeight)); update the assignment at the line that currently sets
scroll.scrollTop to scroll.scrollHeight to use this explicit, clamped value for
clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20842779-63b7-41e4-8bea-92ffdd8bec86
📒 Files selected for processing (2)
interface/src/components/CortexChatPanel.tsxinterface/src/hooks/useStickToBottom.ts
User reported: "sometimes the response text scrolls down, but still doesn't expose the full response + copy button as it streams in." That matches a category of bugs the single-pass scroll missed: - Scrollbar appearing AFTER the initial scroll narrows the content's available width (non-overlay scrollbars), forcing one more line of wrap → content grows by ~1 row → we're now ~1 row above the new bottom. - Web-font swap: fallback → loaded font widens text → reflow → 1 row taller. - A hover-action button or toolbar mounted on the next frame (e.g. a message copy button) extends content past where we just scrolled. ResizeObserver does fire for these, but only once. The settleToBottom helper now snaps on the current frame plus the next two RAFs, which is enough to absorb the typical 1-2 frame delay these layout shifts have. isPinned is checked before each follow-up snap so a user scroll-up in between cancels the chase. Cheap to over-call — scrollTop = scrollHeight is a no-op once we're at the bottom.
The portal chat (main agent conversation view) had the same scroll-pinning bug as the Cortex chat panel from spacedriveapp#594, with its own copy of the logic: - Effect on [visibleItems.length, isTyping] missed tool-result expansion and the per-message copy-button mount that fires when MessageBubble's onCopy renders. - Effect on [sendCount] used behavior: "smooth" inside requestAnimationFrame, same animation-vs-layout-shift race the issue called out. Replaced both with the same useStickToBottom hook used in CortexChatPanel, plus a short useEffect that force-snaps to bottom on sendCount change (preserves the "I just sent a message, take me to my message" behavior even when the user had scrolled up). Channel timeline (ChannelDetail.tsx) uses flex-col-reverse for a CSS-only scroll pin and doesn't need this hook.
Closes #594.
Why
Two chat panels in the dashboard had the same scroll-pinning bug, with their own copies of the broken logic:
CortexChatPanel.tsx) — the case called out in Chat sometimes doesn't scroll all the way to bottom on new message #594. Effect on[messages.length, isStreaming, toolActivity.length]missed tool-result expansion (status flip without length change), theThinkingIndicatortoggle, async Markdown reflow, and `behavior: "smooth"` raced layout shifts.PortalTimeline.tsx) — same pattern. Effect on `[visibleItems.length, isTyping]` missed tool-result expansion and per-message copy-button mount (`MessageBubble onCopy`); send-time effect used `behavior: "smooth"` inside `requestAnimationFrame`.The channel timeline (
ChannelDetail.tsx) usesflex-col-reversefor a CSS-only pin and didn't need the hook.How
New hook `useStickToBottom(scrollRef, contentRef)`:
Wired into both Cortex and Portal panels: refs on the existing scroll container + inner content div, drop the old `useEffect`s + the now-unused `messagesEndRef` / `previousLengthRef`. Portal keeps a small useEffect on `sendCount` that force-snaps to the bottom — preserves "I just sent, take me to my message" even when the user had scrolled up.
Validation