Fix chat scroll and charts message persistance #22
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes chat scroll behavior and ensures charts persist with their associated messages. The changes address two main issues: correcting scroll functionality in chat components and implementing a message-to-charts mapping system so charts remain accessible after new messages are added.
Key changes:
- CSS updates to fix chat scroll behavior by adjusting container heights and overflow properties
- Implementation of a
messageChartsMap to associate chart data with specific message IDs - Refactored chart display logic to show charts for any bot message with associated chart data, not just the latest message
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| client/src/styles/styles.css | Fixed scroll behavior by adjusting container heights, overflow properties, and corrected CSS class names from charts-preview to chat-preview |
| client/src/pages/KnowledgeAssistant.tsx | Updated chat container styling to use inline styles for dynamic height control |
| client/src/pages/FinanceAnalysis.tsx | Implemented message-to-charts mapping system to persist chart associations and display charts for all relevant messages |
| client/src/hooks/useChatBot.tsx | Added messageId to StreamingResponse interface and associated it with bot messages for chart tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setMessages(prev => prev.filter(msg => msg.id !== typingMessageId)); | ||
|
|
||
| // Then add complete response after a tiny delay | ||
| const botMessageId = Date.now() + 2; |
There was a problem hiding this comment.
Using Date.now() + 2 for message ID generation can lead to collisions if multiple messages are created in rapid succession. Consider using a more robust ID generation strategy like incrementing a counter or a UUID library.
| setMessageCharts(prev => { | ||
| const newMap = new Map(prev); | ||
| newMap.set(chatBot.latestResponse!.messageId!, validCharts); |
There was a problem hiding this comment.
Using non-null assertions (!) on potentially undefined values is unsafe. While the outer condition checks for messageId, the non-null assertion could mask potential runtime errors. Consider using optional chaining or a guard clause instead.
| setMessageCharts(prev => { | |
| const newMap = new Map(prev); | |
| newMap.set(chatBot.latestResponse!.messageId!, validCharts); | |
| const messageId = chatBot.latestResponse.messageId; | |
| setMessageCharts(prev => { | |
| const newMap = new Map(prev); | |
| if (messageId) { | |
| newMap.set(messageId, validCharts); | |
| } |
| const charts = messageCharts.get(props.item.id); | ||
| if (charts) { | ||
| setSelectedCharts(charts); | ||
| setIsChartsExpanded(true); | ||
| } |
There was a problem hiding this comment.
The chart retrieval logic is duplicated - it's already retrieved as chartsForThisMessage on line 208. Consider reusing that variable instead of calling messageCharts.get(props.item.id) again.
inikolova
force-pushed
the
fix-chat-scroll
branch
from
cb4a073 to
bfd2368
Compare
No description provided.