feat(platform): simplify chat auto-scroll and fix streaming markdown flicker#830
Merged
Conversation
Remove auto-follow scrolling during streaming responses. Instead, scroll to the last user message on submit and show a "scroll to bottom" button when the user is not at the bottom. Simplifies useAutoScroll hook by removing the enabled/polling logic and using ResizeObserver for content changes.
…ght response area Switch from a fixed spacer + scrollIntoView approach to a response-area pattern where min-height is computed so that scroll-to-bottom naturally positions the last user message at the viewport top. Uses a three-phase strategy: useLayoutEffect for pre-paint approximation, rAF for accurate correction, and ResizeObserver for ongoing updates. Consolidates the initial-load scroll into a single effect keyed by threadId.
…during streaming Streaming LLM responses with partially-revealed markdown (unclosed backticks, bold markers, code fences) causes react-markdown to oscillate between parse trees on consecutive frames. remendMarkdown appends minimal closing tokens in a single O(n) forward pass so the parser always produces a stable result.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR refactors the chat scrolling system and markdown rendering pipeline. The Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/features/chat/components/chat-interface.tsx`:
- Around line 228-248: The MutationObserver in handleLoadMore is currently
attached to containerRef.current (the whole container) which can be triggered by
non-message nodes; change it to observe the message list node (e.g.,
contentRef.current or the specific messages DOM node) so the scroll delta uses
prevScrollHeight of that messages node and callback adjusts container.scrollTop
by the messages node.scrollHeight delta after loadMore(count) completes; update
any other similar observer usages in this file (the other load-more handler
around the later block) to follow the same pattern, referencing the same
symbols: handleLoadMore, containerRef, contentRef, loadMore, and the
MutationObserver instance.
- Around line 192-209: The visibility update for showScrollButton (inside the
useEffect) only observes contentRef but isAtBottom() depends on
container.scrollHeight which can change when the sticky footer (outside
contentRef) resizes; update the effect in chat-interface.tsx to also observe the
footer (or accept/use a footerRef) with the ResizeObserver and call the same
check callback so footer height changes trigger setShowScrollButton, ensuring
you add observer.observe(footerElement) and disconnect it in the cleanup;
reference useEffect, containerRef, contentRef, isAtBottom, showScrollButton, and
ResizeObserver when making the change.
- Around line 211-226: The effect blindly re-scrolls whenever a new threadId
appears, which yanks users back to bottom when a pending send is materialized;
update the useEffect in chat-interface.tsx (the effect that uses
scrolledForThreadRef, threadId, messages.length and containerRef) to skip the
auto-scroll if the messages array contains any pending message (e.g.,
message.status === 'pending' or similar flag used in chat-messages.tsx), so the
separate scroll-on-send in chat-messages.tsx can handle the initial send; keep
the existing scrolledForThreadRef guard but add an early return when a pending
message exists to avoid re-scrolling on pending→real threadId transitions.
In `@services/platform/app/features/chat/components/chat-messages.tsx`:
- Around line 170-196: Store the "was at bottom" state separately (e.g.,
isAtBottomRef) by adding a scroll handler on the container that updates
isAtBottomRef based on container.scrollTop + container.clientHeight >=
container.scrollHeight - 2, then in the ResizeObserver/update logic use that
stored isAtBottomRef value (instead of recalculating from the already-resized
geometry) to decide whether to scrollTo bottom; keep references to containerRef,
responseAreaRef, computeResponseMinHeight, lastUserMessageRef and ensure the
scroll listener is added/removed in the same useEffect cleanup.
In `@services/platform/app/features/chat/utils/__tests__/remend-markdown.test.ts`:
- Around line 156-159: The test named "returns empty string for undefined-like
input" is misleading because it only asserts remedMarkdown('') — update the test
to accurately reflect its intent by renaming it to "returns empty string for
empty input" (or remove the redundant test entirely if it's duplicate), keeping
the assertion expect(remendMarkdown('')).toBe(''); ensure the test references
remedMarkdown so reviewers can locate it.
- Around line 247-273: The timing assertion in the test "processes typical
streaming chunks in under 0.1ms" is flaky on CI; update the test around
remendMarkdown to avoid false failures by either skipping it on CI (use a
conditional skip when process.env.CI is set for the spec that contains the
performance test) or relax the threshold (e.g., increase
expect(perCall).toBeLessThan(...) to a more generous value like 0.5ms); ensure
you only change the test harness (the describe/it block for the performance
test) and keep the remendMarkdown invocation loops intact so the performance
check remains meaningful.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9d924a3-2bca-43ef-b982-723c9e71fbfc
📒 Files selected for processing (8)
services/platform/app/features/chat/components/chat-interface.tsxservices/platform/app/features/chat/components/chat-messages.tsxservices/platform/app/features/chat/components/incremental-markdown.tsxservices/platform/app/features/chat/utils/__tests__/remend-markdown.test.tsservices/platform/app/features/chat/utils/remend-markdown.tsservices/platform/app/globals.cssservices/platform/app/hooks/__tests__/use-auto-scroll.test.tsservices/platform/app/hooks/use-auto-scroll.ts
Comment on lines
+192
to
+209
| // Scroll button visibility — updated on both scroll events and content resize | ||
| useEffect(() => { | ||
| const container = containerRef.current; | ||
| const content = contentRef.current; | ||
| if (!container || !content) return; | ||
|
|
||
| const targetScrollTop = | ||
| container.scrollTop + (aiAreaRect.top - containerRect.top) - 80; | ||
| const check = () => setShowScrollButton(!isAtBottom()); | ||
|
|
||
| scrollTo(Math.max(0, targetScrollTop)); | ||
| } | ||
| }, [containerRef, scrollTo]); | ||
| const observer = new ResizeObserver(check); | ||
| observer.observe(content); | ||
| container.addEventListener('scroll', check, { passive: true }); | ||
| check(); | ||
|
|
||
| useEffect(() => { | ||
| if (isLoading && shouldScrollToAIRef.current) { | ||
| requestAnimationFrame(() => { | ||
| scrollToAIResponse(); | ||
| shouldScrollToAIRef.current = false; | ||
| }); | ||
| } | ||
| }, [isLoading, scrollToAIResponse]); | ||
| return () => { | ||
| observer.disconnect(); | ||
| container.removeEventListener('scroll', check); | ||
| }; | ||
| }, [containerRef, contentRef, isAtBottom]); |
There was a problem hiding this comment.
Observe footer resizes when computing showScrollButton.
Line 200 only observes content, but isAtBottom() depends on the full container.scrollHeight. The sticky footer lives outside contentRef here, so multiline input growth or attachment rows can change the bottom distance without firing check(). showScrollButton then stays stale until the user scrolls or message content changes. services/platform/app/components/layout/panel-footer.tsx (Lines 8-16) confirms the footer is a separate sticky sibling, so it needs its own observation path or a shared footer ref.
Also applies to: 329-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/chat-interface.tsx` around
lines 192 - 209, The visibility update for showScrollButton (inside the
useEffect) only observes contentRef but isAtBottom() depends on
container.scrollHeight which can change when the sticky footer (outside
contentRef) resizes; update the effect in chat-interface.tsx to also observe the
footer (or accept/use a footerRef) with the ResizeObserver and call the same
check callback so footer height changes trigger setShowScrollButton, ensuring
you add observer.observe(footerElement) and disconnect it in the cleanup;
reference useEffect, containerRef, contentRef, isAtBottom, showScrollButton, and
ResizeObserver when making the change.
Comment on lines
+211
to
+226
| // Scroll to bottom on initial load (all threads). | ||
| // For new threads with a pending message, ChatMessages also scrolls | ||
| // to bottom on send — the response area's min-height ensures the | ||
| // user message ends up at the viewport top. | ||
| const scrolledForThreadRef = useRef<string | null>(null); | ||
| useEffect(() => { | ||
| const container = containerRef.current; | ||
| if (!container) return; | ||
| if (!threadId || messages.length === 0) return; | ||
| if (scrolledForThreadRef.current === threadId) return; | ||
|
|
||
| const checkScroll = () => { | ||
| setShowScrollButton(!isAtBottom()); | ||
| }; | ||
| scrolledForThreadRef.current = threadId; | ||
|
|
||
| container.addEventListener('scroll', checkScroll, { passive: true }); | ||
| return () => container.removeEventListener('scroll', checkScroll); | ||
| }, [containerRef, isAtBottom]); | ||
| containerRef.current?.scrollTo({ | ||
| top: containerRef.current.scrollHeight, | ||
| behavior: 'instant', | ||
| }); | ||
| }, [threadId, messages.length, containerRef]); |
There was a problem hiding this comment.
Don’t re-scroll when a pending send simply acquires a real threadId.
services/platform/app/features/chat/components/chat-messages.tsx already scrolls on send when the pending user message appears. This effect scrolls again the first time every threadId shows up, so a user who has scrolled away during the pending/streaming window gets yanked back to bottom once the server materializes the thread.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/chat-interface.tsx` around
lines 211 - 226, The effect blindly re-scrolls whenever a new threadId appears,
which yanks users back to bottom when a pending send is materialized; update the
useEffect in chat-interface.tsx (the effect that uses scrolledForThreadRef,
threadId, messages.length and containerRef) to skip the auto-scroll if the
messages array contains any pending message (e.g., message.status === 'pending'
or similar flag used in chat-messages.tsx), so the separate scroll-on-send in
chat-messages.tsx can handle the initial send; keep the existing
scrolledForThreadRef guard but add an early return when a pending message exists
to avoid re-scrolling on pending→real threadId transitions.
Comment on lines
+228
to
+248
| // Load-more scroll preservation: keep viewport stable when older messages prepend | ||
| const handleLoadMore = useCallback( | ||
| (count: number) => { | ||
| const container = containerRef.current; | ||
| if (!container) { | ||
| loadMore(count); | ||
| return; | ||
| } | ||
|
|
||
| const prevScrollHeight = container.scrollHeight; | ||
| const observer = new MutationObserver(() => { | ||
| observer.disconnect(); | ||
| container.scrollTop += container.scrollHeight - prevScrollHeight; | ||
| }); | ||
| } | ||
| }, [threadId, messages.length, containerRef]); | ||
| observer.observe(container, { childList: true, subtree: true }); | ||
| loadMore(count); | ||
|
|
||
| useEffect(() => { | ||
| hasScrolledOnLoadRef.current = false; | ||
| }, [threadId]); | ||
| // Safety timeout to disconnect if no mutation fires | ||
| setTimeout(() => observer.disconnect(), 2000); | ||
| }, | ||
| [containerRef, loadMore], |
There was a problem hiding this comment.
Limit load-more scroll preservation to message prepends.
The observer is attached to the whole container, which also contains the sticky footer and floating scroll button. Any child insertion there can fire first, disconnect the observer, and apply the scroll compensation before older messages are actually prepended. Observe contentRef.current or the message list node instead so the delta is based only on the load-more DOM change.
Also applies to: 294-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/chat-interface.tsx` around
lines 228 - 248, The MutationObserver in handleLoadMore is currently attached to
containerRef.current (the whole container) which can be triggered by non-message
nodes; change it to observe the message list node (e.g., contentRef.current or
the specific messages DOM node) so the scroll delta uses prevScrollHeight of
that messages node and callback adjusts container.scrollTop by the messages
node.scrollHeight delta after loadMore(count) completes; update any other
similar observer usages in this file (the other load-more handler around the
later block) to follow the same pattern, referencing the same symbols:
handleLoadMore, containerRef, contentRef, loadMore, and the MutationObserver
instance.
Comment on lines
+170
to
+196
| useEffect(() => { | ||
| const container = containerRef.current; | ||
| const responseArea = responseAreaRef.current; | ||
| if (!container || !responseArea) return; | ||
|
|
||
| const update = () => { | ||
| const atBottom = | ||
| container.scrollHeight - container.scrollTop - container.clientHeight <= | ||
| 2; | ||
|
|
||
| responseArea.style.minHeight = `${computeResponseMinHeight(container, responseArea, lastUserMessageRef.current)}px`; | ||
|
|
||
| if (atBottom) { | ||
| container.scrollTo({ | ||
| top: container.scrollHeight, | ||
| behavior: 'instant', | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| const ro = new ResizeObserver(update); | ||
| ro.observe(container); | ||
| const footer = container.querySelector('[class*="sticky"]'); | ||
| if (footer instanceof HTMLElement) ro.observe(footer); | ||
|
|
||
| return () => ro.disconnect(); | ||
| }, [containerRef, lastUserMessageRef]); |
There was a problem hiding this comment.
Capture the bottom state before the resize lands.
Line 176 computes atBottom after the ResizeObserver has already seen the new footer/container geometry. If the user was exactly at bottom and the composer grows, scrollHeight increases first, atBottom flips to false, and the view stops re-pinning. The newest content then drifts off-screen even though the user never scrolled away.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/components/chat-messages.tsx` around
lines 170 - 196, Store the "was at bottom" state separately (e.g.,
isAtBottomRef) by adding a scroll handler on the container that updates
isAtBottomRef based on container.scrollTop + container.clientHeight >=
container.scrollHeight - 2, then in the ResizeObserver/update logic use that
stored isAtBottomRef value (instead of recalculating from the already-resized
geometry) to decide whether to scrollTo bottom; keep references to containerRef,
responseAreaRef, computeResponseMinHeight, lastUserMessageRef and ensure the
scroll listener is added/removed in the same useEffect cleanup.
Comment on lines
+156
to
+159
| it('returns empty string for undefined-like input', () => { | ||
| // The function signature accepts string, but guard anyway | ||
| expect(remendMarkdown('')).toBe(''); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Misleading test name.
The test is named "returns empty string for undefined-like input" but only tests the empty string ''. Since remendMarkdown accepts string, this is effectively a duplicate of the previous test.
Consider removing this test or renaming it to clarify its purpose.
Suggested fix
- it('returns empty string for undefined-like input', () => {
- // The function signature accepts string, but guard anyway
- expect(remendMarkdown('')).toBe('');
- });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('returns empty string for undefined-like input', () => { | |
| // The function signature accepts string, but guard anyway | |
| expect(remendMarkdown('')).toBe(''); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/utils/__tests__/remend-markdown.test.ts`
around lines 156 - 159, The test named "returns empty string for undefined-like
input" is misleading because it only asserts remedMarkdown('') — update the test
to accurately reflect its intent by renaming it to "returns empty string for
empty input" (or remove the redundant test entirely if it's duplicate), keeping
the assertion expect(remendMarkdown('')).toBe(''); ensure the test references
remedMarkdown so reviewers can locate it.
Comment on lines
+247
to
+273
| describe('performance', () => { | ||
| it('processes typical streaming chunks in under 0.1ms', () => { | ||
| const chunks = [ | ||
| 'Hello **world**, this is a test with `code` and *italic* text.', | ||
| '```python\ndef foo():\n return bar\n', | ||
| 'Some text with ~~strike~~ and **bold', | ||
| '**bold text that is not yet clos', | ||
| 'Normal paragraph with no special syntax at all, just words flowing.', | ||
| ]; | ||
|
|
||
| // Warm up | ||
| for (const chunk of chunks) { | ||
| remendMarkdown(chunk); | ||
| } | ||
|
|
||
| const iterations = 10_000; | ||
| const start = performance.now(); | ||
| for (let i = 0; i < iterations; i++) { | ||
| for (const chunk of chunks) { | ||
| remendMarkdown(chunk); | ||
| } | ||
| } | ||
| const elapsed = performance.now() - start; | ||
| const perCall = elapsed / (iterations * chunks.length); | ||
|
|
||
| expect(perCall).toBeLessThan(0.1); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Performance test may be flaky in CI.
Wall-clock timing tests can fail intermittently on slow or loaded CI runners. The 0.1ms threshold is reasonable for local development but may not hold in all environments.
Consider either:
- Skipping this test in CI via
it.skipIf(process.env.CI) - Increasing the threshold with a generous margin (e.g., 0.5ms)
- Testing relative performance (e.g., O(n) behavior) instead of absolute timing
Option: Skip in CI
- it('processes typical streaming chunks in under 0.1ms', () => {
+ it.skipIf(process.env.CI)('processes typical streaming chunks in under 0.1ms', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe('performance', () => { | |
| it('processes typical streaming chunks in under 0.1ms', () => { | |
| const chunks = [ | |
| 'Hello **world**, this is a test with `code` and *italic* text.', | |
| '```python\ndef foo():\n return bar\n', | |
| 'Some text with ~~strike~~ and **bold', | |
| '**bold text that is not yet clos', | |
| 'Normal paragraph with no special syntax at all, just words flowing.', | |
| ]; | |
| // Warm up | |
| for (const chunk of chunks) { | |
| remendMarkdown(chunk); | |
| } | |
| const iterations = 10_000; | |
| const start = performance.now(); | |
| for (let i = 0; i < iterations; i++) { | |
| for (const chunk of chunks) { | |
| remendMarkdown(chunk); | |
| } | |
| } | |
| const elapsed = performance.now() - start; | |
| const perCall = elapsed / (iterations * chunks.length); | |
| expect(perCall).toBeLessThan(0.1); | |
| }); | |
| describe('performance', () => { | |
| (process.env.CI ? it.skip : it)('processes typical streaming chunks in under 0.1ms', () => { | |
| const chunks = [ | |
| 'Hello **world**, this is a test with `code` and *italic* text.', | |
| ' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/chat/utils/__tests__/remend-markdown.test.ts`
around lines 247 - 273, The timing assertion in the test "processes typical
streaming chunks in under 0.1ms" is flaky on CI; update the test around
remendMarkdown to avoid false failures by either skipping it on CI (use a
conditional skip when process.env.CI is set for the spec that contains the
performance test) or relax the threshold (e.g., increase
expect(perCall).toBeLessThan(...) to a more generous value like 0.5ms); ensure
you only change the test harness (the describe/it block for the performance
test) and keep the remendMarkdown invocation loops intact so the performance
check remains meaningful.
…-free streaming Extend the streaming markdown remend function to handle incomplete link and image syntax. Incomplete links (e.g. `[text](url`) now show display text only instead of raw markdown syntax, and incomplete images are removed entirely to avoid broken placeholders. Adds 20 new test cases covering all link/image streaming states.
This was referenced Mar 23, 2026
yannickmonney
pushed a commit
that referenced
this pull request
Apr 8, 2026
5 tasks
This was referenced Apr 23, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
min-heightso the last user message stays pinned at the top without a spacer element.remendMarkdown— a single-pass O(n) function that auto-closes incomplete markdown syntax (unclosed backticks, bold, italic, strikethrough, code fences) during streaming. This prevents react-markdown from oscillating between parse trees on consecutive frames.overflow: clipon body/html to prevent scroll bleed.Test plan
remendMarkdownunit tests pass (bun run --filter @tale/platform test)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style