fix(copilot): fix copilot chat loading #2769
Merged
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Greptile OverviewGreptile SummaryWhat ChangedThis PR fixes copilot chat loading issues by reorganizing the message persistence strategy. The key architectural change is that the server no longer saves complete messages - it only updates the Core Changes
Architecture Benefits
Issues FoundCritical IssuesNone - the changes follow a sound architectural pattern. Logic Issues
Style Suggestions
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client as Copilot UI
participant Store as Copilot Store
participant ChatAPI as /api/copilot/chat
participant UpdateAPI as /api/copilot/chat/update-messages
participant Agent as Sim Agent
participant DB as Database
Client->>Store: sendMessage()
Store->>ChatAPI: POST with message
ChatAPI->>Agent: Forward to Sim Agent
Agent-->>ChatAPI: Stream SSE events
loop Streaming
ChatAPI-->>Store: Forward SSE events
Store->>Store: Build contentBlocks
Store->>Client: Update UI with content
end
ChatAPI->>DB: Update conversationId only
Note over ChatAPI,DB: Server no longer saves messages<br/>to avoid overwriting client data
Store->>Store: Serialize messages with contentBlocks
Store->>UpdateAPI: POST full messages array
UpdateAPI->>DB: Save messages with contentBlocks
Note over UpdateAPI,DB: Client has full rendering data<br/>including tool states
UpdateAPI-->>Store: Success response
Store->>Client: Update complete
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return JSON.parse(JSON.stringify(obj)) | ||
| } catch { | ||
| const json = JSON.stringify(obj) | ||
| if (!json || json === 'undefined') { |
Contributor
There was a problem hiding this comment.
The check json === 'undefined' will never match the string "undefined" from JSON.stringify(undefined). When you call JSON.stringify(undefined), it returns undefined (the value, not the string), not the string "undefined".
This condition should check for the undefined value itself:
Suggested change
| if (!json || json === 'undefined') { | |
| if (!json || json === undefined) { |
Or more simply:
Suggested change
| if (!json || json === 'undefined') { | |
| if (!json) { |
This ensures the deepClone function properly handles cases where JSON.stringify returns undefined.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/stores/panel/copilot/store.ts
Line: 596:596
Comment:
The check `json === 'undefined'` will never match the string "undefined" from `JSON.stringify(undefined)`. When you call `JSON.stringify(undefined)`, it returns `undefined` (the value, not the string), not the string `"undefined"`.
This condition should check for the undefined value itself:
```suggestion
if (!json || json === undefined) {
```
Or more simply:
```suggestion
if (!json) {
```
This ensures the deepClone function properly handles cases where JSON.stringify returns undefined.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+812
to
+827
| if (responseId) { | ||
| await db | ||
| .update(copilotChats) | ||
| .set({ | ||
| updatedAt: new Date(), | ||
| conversationId: responseId, | ||
| }) | ||
| .where(eq(copilotChats.id, actualChatId!)) | ||
|
|
||
| logger.info(`[${tracker.requestId}] Updated chat ${actualChatId} with new messages`, { | ||
| messageCount: updatedMessages.length, | ||
| savedUserMessage: true, | ||
| savedAssistantMessage: assistantContent.trim().length > 0, | ||
| updatedConversationId: responseId || null, | ||
| }) | ||
| logger.info( | ||
| `[${tracker.requestId}] Updated conversationId for chat ${actualChatId}`, | ||
| { | ||
| updatedConversationId: responseId, | ||
| } | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
The server now only updates conversationId when responseId is truthy, but there's no update to updatedAt when responseId is falsy. This means if a chat completes without a valid responseId, the database won't reflect that the chat was recently active.
Consider always updating updatedAt to track chat activity:
Suggested change
| if (responseId) { | |
| await db | |
| .update(copilotChats) | |
| .set({ | |
| updatedAt: new Date(), | |
| conversationId: responseId, | |
| }) | |
| .where(eq(copilotChats.id, actualChatId!)) | |
| logger.info(`[${tracker.requestId}] Updated chat ${actualChatId} with new messages`, { | |
| messageCount: updatedMessages.length, | |
| savedUserMessage: true, | |
| savedAssistantMessage: assistantContent.trim().length > 0, | |
| updatedConversationId: responseId || null, | |
| }) | |
| logger.info( | |
| `[${tracker.requestId}] Updated conversationId for chat ${actualChatId}`, | |
| { | |
| updatedConversationId: responseId, | |
| } | |
| ) | |
| } | |
| if (responseId) { | |
| await db | |
| .update(copilotChats) | |
| .set({ | |
| updatedAt: new Date(), | |
| conversationId: responseId, | |
| }) | |
| .where(eq(copilotChats.id, actualChatId!)) | |
| logger.info( | |
| `[${tracker.requestId}] Updated conversationId for chat ${actualChatId}`, | |
| { | |
| updatedConversationId: responseId, | |
| } | |
| ) | |
| } else { | |
| // Still update the timestamp even without conversationId | |
| await db | |
| .update(copilotChats) | |
| .set({ updatedAt: new Date() }) | |
| .where(eq(copilotChats.id, actualChatId!)) | |
| } |
This ensures chat activity is properly tracked even in edge cases.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/api/copilot/chat/route.ts
Line: 812:827
Comment:
The server now only updates `conversationId` when `responseId` is truthy, but there's no update to `updatedAt` when `responseId` is falsy. This means if a chat completes without a valid `responseId`, the database won't reflect that the chat was recently active.
Consider always updating `updatedAt` to track chat activity:
```suggestion
if (responseId) {
await db
.update(copilotChats)
.set({
updatedAt: new Date(),
conversationId: responseId,
})
.where(eq(copilotChats.id, actualChatId!))
logger.info(
`[${tracker.requestId}] Updated conversationId for chat ${actualChatId}`,
{
updatedConversationId: responseId,
}
)
} else {
// Still update the timestamp even without conversationId
await db
.update(copilotChats)
.set({ updatedAt: new Date() })
.where(eq(copilotChats.id, actualChatId!))
}
```
This ensures chat activity is properly tracked even in edge cases.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+3214
to
+3219
| if (!saveResponse.ok) { | ||
| const errorText = await saveResponse.text().catch(() => '') | ||
| logger.error('[Stream Done] Failed to save messages to DB', { | ||
| status: saveResponse.status, | ||
| error: errorText, | ||
| }) |
Contributor
There was a problem hiding this comment.
The error handling only logs the error but doesn't provide user feedback. If saving messages fails silently, users won't know their chat history might not be persisted.
Consider setting an error state that the UI can display:
Suggested change
| if (!saveResponse.ok) { | |
| const errorText = await saveResponse.text().catch(() => '') | |
| logger.error('[Stream Done] Failed to save messages to DB', { | |
| status: saveResponse.status, | |
| error: errorText, | |
| }) | |
| if (!saveResponse.ok) { | |
| const errorText = await saveResponse.text().catch(() => '') | |
| logger.error('[Stream Done] Failed to save messages to DB', { | |
| status: saveResponse.status, | |
| error: errorText, | |
| }) | |
| // Set error state for user visibility | |
| set({ saveError: 'Failed to save chat messages' }) |
This provides user awareness when persistence fails.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/stores/panel/copilot/store.ts
Line: 3214:3219
Comment:
The error handling only logs the error but doesn't provide user feedback. If saving messages fails silently, users won't know their chat history might not be persisted.
Consider setting an error state that the UI can display:
```suggestion
if (!saveResponse.ok) {
const errorText = await saveResponse.text().catch(() => '')
logger.error('[Stream Done] Failed to save messages to DB', {
status: saveResponse.status,
error: errorText,
})
// Set error state for user visibility
set({ saveError: 'Failed to save chat messages' })
```
This provides user awareness when persistence fails.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Fix copilot chat loading
Type of Change
Testing
Manual
Checklist