feat(chat): implement message navigation and fix scroll-to-previous functionality#460
Open
menny wants to merge 1 commit intositeboon:mainfrom
Open
feat(chat): implement message navigation and fix scroll-to-previous functionality#460menny wants to merge 1 commit intositeboon:mainfrom
menny wants to merge 1 commit intositeboon:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughEnhances the chat interface with reverse-chronological message navigation by introducing a scroll-to-previous-user-message feature and improved pagination logic. Updates the useChatSessionState hook with scroll restoration, message loading refinements, and new scroll-control callbacks threaded through ChatInterface components to ChatInputControls. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ChatInputControls
participant useChatSessionState
participant MessageLoading
participant ScrollSystem
User->>ChatInputControls: Clicks "scroll to previous user message"
ChatInputControls->>useChatSessionState: onScrollToPreviousUserMessage()
alt Message visible in current window
useChatSessionState->>ScrollSystem: Scroll to message index
else Message not visible
useChatSessionState->>MessageLoading: loadOlderMessages()
MessageLoading->>useChatSessionState: Update visibleMessageCount
useChatSessionState->>useChatSessionState: Set pendingScrollToMessageRef
useChatSessionState->>ScrollSystem: Adjust scroll position & restore
ScrollSystem->>ChatInputControls: Render updated messages
end
Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/chat/hooks/useChatSessionState.ts (1)
305-309: Remove debug logging from scroll paths before merge.
console.log('[ScrollDebug]', ...)in frequent navigation/layout paths adds noise and avoidable overhead.🧹 Cleanup diff
- console.log('[ScrollDebug] scrollToPreviousUserMessage', { - viewportTop, - firstVisibleGlobalIndex, - totalChatMessages: chatMessages.length - }); ... - console.log('[ScrollDebug] No earlier user message found in loaded history'); if (hasMoreMessages) { - console.log('[ScrollDebug] Triggering server load'); loadOlderMessages(container); } return; } - - console.log('[ScrollDebug] Target user message found at global index', targetGlobalIndex); ... if (targetEl) { - console.log('[ScrollDebug] Scrolling to rendered message element'); container.scrollTop = Math.max(0, targetEl.offsetTop - 20); } else { - console.log('[ScrollDebug] Message not rendered, increasing visible count'); pendingScrollToMessageRef.current = targetGlobalIndex; ... - console.log('[ScrollDebug] Scrolling to pending message element via data-index', { targetGlobalIndex, offsetTop: targetEl.offsetTop }); container.scrollTop = Math.max(0, targetEl.offsetTop - 20);Also applies to: 321-324, 329-329, 334-334, 338-338, 397-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatSessionState.ts` around lines 305 - 309, Remove the debug console.log calls for scroll tracing from the useChatSessionState hook: delete the console.log('[ScrollDebug] ...') in scrollToPreviousUserMessage and all similar '[ScrollDebug]' logs in the same file (e.g., in scrollToNextUserMessage, scrollToMessageAtIndex, ensureMessageVisible/other scroll handlers) so navigation/layout code no longer emits debug output or adds overhead; keep any necessary error handling or real logging mechanisms intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/chat/hooks/useChatSessionState.ts`:
- Around line 747-751: After completing the full-history load, synchronize
pagination state to avoid later duplicate fetches: set messagesOffsetRef.current
to allLoadedMessages.length, set the hasMoreMessages flag/ref (hasMoreMessages
or hasMoreMessagesRef) to false, and update totalMessages (or totalMessagesRef /
setTotalMessages) to allLoadedMessages.length; do this right after calling
setSessionMessages and before/unconditionally with
setAllMessagesLoaded/setLoadAllJustFinished so pagination and refs reflect the
“all loaded” state.
- Around line 720-741: The loop in useChatSessionState.ts currently prepends
each chunk with allLoadedMessages = [...chunk, ...allLoadedMessages], causing
quadratic reallocations; change the accumulation to an append-friendly strategy
(e.g., push each chunk into allLoadedMessages via
allLoadedMessages.push(...chunk) or collect chunks in an array-of-chunks and do
a single concat at the end) and if required restore the original message order
after the loop (reverse or single concat) so behavior of selectedProject/session
loading remains the same; update references to allLoadedMessages, currentOffset
and the while loop accordingly.
In `@src/components/chat/view/subcomponents/ChatInputControls.tsx`:
- Around line 127-131: In ChatInputControls.tsx update the new navigation
<button> (the one with onClick={onScrollToPreviousUserMessage}) to use the i18n
translation key instead of hardcoded tooltip text, add an aria-label that uses
the same localized string, and set type="button" for safety; locate the button
in the ChatInputControls component and replace the hardcoded title with t('...')
(or the project’s translation helper) and add aria-label={t('...')} plus
type="button".
In `@src/components/chat/view/subcomponents/ChatMessagesPane.tsx`:
- Around line 246-247: The render loop in ChatMessagesPane.tsx uses
chatMessages.indexOf(message) inside visibleMessages.map (assigning
globalIndex), causing O(n²) behavior; instead, compute a lookup map once when
chatMessages changes (e.g., build a Map from message unique id or object
reference to its global index in chatMessages) and then read globalIndex =
indexMap.get(message.id || message) inside the visibleMessages.map. Update both
occurrences that use chatMessages.indexOf(message) (the one creating globalIndex
and the similar usage around the later occurrence) to use the precomputed
indexMap to avoid repeated linear scans.
---
Nitpick comments:
In `@src/components/chat/hooks/useChatSessionState.ts`:
- Around line 305-309: Remove the debug console.log calls for scroll tracing
from the useChatSessionState hook: delete the console.log('[ScrollDebug] ...')
in scrollToPreviousUserMessage and all similar '[ScrollDebug]' logs in the same
file (e.g., in scrollToNextUserMessage, scrollToMessageAtIndex,
ensureMessageVisible/other scroll handlers) so navigation/layout code no longer
emits debug output or adds overhead; keep any necessary error handling or real
logging mechanisms intact.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/chat/hooks/useChatSessionState.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/ChatComposer.tsxsrc/components/chat/view/subcomponents/ChatInputControls.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/components/chat/view/subcomponents/MessageComponent.tsx
Comment on lines
+720
to
+741
| let allLoadedMessages: any[] = []; | ||
| let currentOffset = 0; | ||
| let hasMore = true; | ||
|
|
||
| if (currentSessionId !== requestSessionId) return; | ||
|
|
||
| if (response.ok) { | ||
| const data = await response.json(); | ||
| const allMessages = data.messages || data; | ||
| while (hasMore) { | ||
| const response = await (api.sessionMessages as any)( | ||
| selectedProject.name, | ||
| selectedSession.id, | ||
| 100, // Load in larger chunks | ||
| currentOffset, | ||
| sessionProvider, | ||
| ); | ||
|
|
||
| if (container) { | ||
| pendingScrollRestoreRef.current = { | ||
| height: previousScrollHeight, | ||
| top: previousScrollTop, | ||
| }; | ||
| if (!response.ok) { | ||
| throw new Error('Failed to load messages'); | ||
| } | ||
|
|
||
| setSessionMessages(Array.isArray(allMessages) ? allMessages : []); | ||
| setHasMoreMessages(false); | ||
| setTotalMessages(Array.isArray(allMessages) ? allMessages.length : 0); | ||
| messagesOffsetRef.current = Array.isArray(allMessages) ? allMessages.length : 0; | ||
|
|
||
| setVisibleMessageCount(Infinity); | ||
| setAllMessagesLoaded(true); | ||
| const data = await response.json(); | ||
| const chunk = data.messages || []; | ||
| allLoadedMessages = [...chunk, ...allLoadedMessages]; | ||
| currentOffset += chunk.length; | ||
| hasMore = Boolean(data.hasMore); |
Contributor
There was a problem hiding this comment.
Load-all accumulation is quadratic and will degrade on big histories.
allLoadedMessages = [...chunk, ...allLoadedMessages] inside the loop repeatedly reallocates growing arrays.
⚡ Proposed fix
- let allLoadedMessages: any[] = [];
+ const messageChunks: any[][] = [];
let currentOffset = 0;
let hasMore = true;
...
const data = await response.json();
const chunk = data.messages || [];
- allLoadedMessages = [...chunk, ...allLoadedMessages];
+ messageChunks.push(chunk);
currentOffset += chunk.length;
hasMore = Boolean(data.hasMore);
...
- setSessionMessages(allLoadedMessages);
+ const allLoadedMessages = messageChunks.reverse().flat();
+ setSessionMessages(allLoadedMessages);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatSessionState.ts` around lines 720 - 741, The
loop in useChatSessionState.ts currently prepends each chunk with
allLoadedMessages = [...chunk, ...allLoadedMessages], causing quadratic
reallocations; change the accumulation to an append-friendly strategy (e.g.,
push each chunk into allLoadedMessages via allLoadedMessages.push(...chunk) or
collect chunks in an array-of-chunks and do a single concat at the end) and if
required restore the original message order after the loop (reverse or single
concat) so behavior of selectedProject/session loading remains the same; update
references to allLoadedMessages, currentOffset and the while loop accordingly.
Comment on lines
+747
to
+751
| setSessionMessages(allLoadedMessages); | ||
| setAllMessagesLoaded(true); | ||
| allMessagesLoadedRef.current = true; | ||
| setVisibleMessageCount(allLoadedMessages.length); | ||
| setLoadAllJustFinished(true); |
Contributor
There was a problem hiding this comment.
Synchronize pagination state after loading full history.
After full fetch, pagination refs/state remain stale (messagesOffsetRef, hasMoreMessages, totalMessages). If all-loaded mode is later reset, top-pagination can issue redundant/duplicate fetches.
🧭 Proposed fix
const allLoadedMessages = messageChunks.reverse().flat();
setSessionMessages(allLoadedMessages);
+ messagesOffsetRef.current = allLoadedMessages.length;
+ setHasMoreMessages(false);
+ setTotalMessages(allLoadedMessages.length);
setAllMessagesLoaded(true);
allMessagesLoadedRef.current = true;
setVisibleMessageCount(allLoadedMessages.length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatSessionState.ts` around lines 747 - 751,
After completing the full-history load, synchronize pagination state to avoid
later duplicate fetches: set messagesOffsetRef.current to
allLoadedMessages.length, set the hasMoreMessages flag/ref (hasMoreMessages or
hasMoreMessagesRef) to false, and update totalMessages (or totalMessagesRef /
setTotalMessages) to allLoadedMessages.length; do this right after calling
setSessionMessages and before/unconditionally with
setAllMessagesLoaded/setLoadAllJustFinished so pagination and refs reflect the
“all loaded” state.
Comment on lines
+127
to
+131
| <button | ||
| onClick={onScrollToPreviousUserMessage} | ||
| className="w-7 h-7 sm:w-8 sm:h-8 bg-primary hover:bg-primary/90 text-primary-foreground rounded-lg shadow-sm flex items-center justify-center transition-all duration-200 hover:scale-105" | ||
| title="Scroll to previous user message" | ||
| > |
Contributor
There was a problem hiding this comment.
Localize and label the new navigation button.
The new tooltip text is hardcoded and not localized. Add i18n key usage and an aria-label (and type="button" for safety).
🌐 Suggested fix
- <button
+ <button
+ type="button"
onClick={onScrollToPreviousUserMessage}
className="w-7 h-7 sm:w-8 sm:h-8 bg-primary hover:bg-primary/90 text-primary-foreground rounded-lg shadow-sm flex items-center justify-center transition-all duration-200 hover:scale-105"
- title="Scroll to previous user message"
+ title={t('input.scrollToPreviousUserMessage', { defaultValue: 'Scroll to previous user message' })}
+ aria-label={t('input.scrollToPreviousUserMessage', { defaultValue: 'Scroll to previous user message' })}
>📝 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
| <button | |
| onClick={onScrollToPreviousUserMessage} | |
| className="w-7 h-7 sm:w-8 sm:h-8 bg-primary hover:bg-primary/90 text-primary-foreground rounded-lg shadow-sm flex items-center justify-center transition-all duration-200 hover:scale-105" | |
| title="Scroll to previous user message" | |
| > | |
| <button | |
| type="button" | |
| onClick={onScrollToPreviousUserMessage} | |
| className="w-7 h-7 sm:w-8 sm:h-8 bg-primary hover:bg-primary/90 text-primary-foreground rounded-lg shadow-sm flex items-center justify-center transition-all duration-200 hover:scale-105" | |
| title={t('input.scrollToPreviousUserMessage', { defaultValue: 'Scroll to previous user message' })} | |
| aria-label={t('input.scrollToPreviousUserMessage', { defaultValue: 'Scroll to previous user message' })} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/view/subcomponents/ChatInputControls.tsx` around lines
127 - 131, In ChatInputControls.tsx update the new navigation <button> (the one
with onClick={onScrollToPreviousUserMessage}) to use the i18n translation key
instead of hardcoded tooltip text, add an aria-label that uses the same
localized string, and set type="button" for safety; locate the button in the
ChatInputControls component and replace the hardcoded title with t('...') (or
the project’s translation helper) and add aria-label={t('...')} plus
type="button".
Comment on lines
+246
to
+247
| // index here is relative to visibleMessages. We need the global index in chatMessages. | ||
| const globalIndex = chatMessages.indexOf(message); |
Contributor
There was a problem hiding this comment.
Avoid O(n²) global-index lookup in render loop.
chatMessages.indexOf(message) inside visibleMessages.map scales poorly and can slow scrolling/rendering on large sessions. Precompute an index map once per chatMessages change.
⚡ Proposed fix
-import { useCallback, useRef } from 'react';
+import { useCallback, useMemo, useRef } from 'react';
...
+ const globalIndexMap = useMemo(() => {
+ const map = new WeakMap<ChatMessage, number>();
+ chatMessages.forEach((msg, idx) => map.set(msg, idx));
+ return map;
+ }, [chatMessages]);
...
- const globalIndex = chatMessages.indexOf(message);
+ const globalIndex = globalIndexMap.get(message);
...
- index={globalIndex !== -1 ? globalIndex : index}
+ index={globalIndex ?? index}Also applies to: 253-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/view/subcomponents/ChatMessagesPane.tsx` around lines 246
- 247, The render loop in ChatMessagesPane.tsx uses
chatMessages.indexOf(message) inside visibleMessages.map (assigning
globalIndex), causing O(n²) behavior; instead, compute a lookup map once when
chatMessages changes (e.g., build a Map from message unique id or object
reference to its global index in chatMessages) and then read globalIndex =
indexMap.get(message.id || message) inside the visibleMessages.map. Update both
occurrences that use chatMessages.indexOf(message) (the one creating globalIndex
and the similar usage around the later occurrence) to use the precomputed
indexMap to avoid repeated linear scans.
Collaborator
|
Hey @menny, thanks for the PR! Can you clarify what issues it solves in more detail and why the changes were necessary? |
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
2 participants
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
This PR isolates and implements the message navigation features in the chat interface, specifically focusing on "Scroll to Previous User Message" and "Scroll to Bottom" functionality. It also resolves several build and runtime issues encountered after rebasing onto the latest
main.Key Changes
Feature Isolation
ChatInterface.tsx.Message Navigation Fixes
data-message-indextoMessageComponent.tsxroot elements to enable accurate Targeting by the scroll logic.ChatMessagesPane.tsxto calculate and pass global message indices (fromchatMessages) instead of relative view indices, ensuring "Scroll to previous" correctly identifies targets across the entire history.Summary by CodeRabbit
New Features
Improvements