✨ Address message bloat causing crash in extension #949
Conversation
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
WalkthroughThis PR introduces a selective mutation mechanism for chat messages in the extension state management. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes This change involves mixed complexity across multiple files. While the pattern is consistent (adding a new mutation method, updating call sites, and handling partial updates), it requires understanding the state mutation flow, the relationship between full and partial updates, and how the webview context processes different event types. The changes span interface definitions, implementation logic, refactored method calls, and event handling paths. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webview-ui/src/context/ExtensionStateContext.tsx (1)
70-78: Improve type safety for chat messages.The union type uses
any[]forchatMessages, which reduces type safety. Consider using the specificChatMessage[]type from the shared package.Additionally, the
timestampfield in theCHAT_MESSAGES_UPDATEpayload is sent but not used in this handler. Consider either using it (e.g., for debugging or staleness checks) or removing it from the payload.Apply this diff to improve type safety:
- const handleMessage = (event: MessageEvent<ExtensionData | { type: string; chatMessages: any[]; timestamp: string }>) => { + const handleMessage = (event: MessageEvent<ExtensionData | { type: 'CHAT_MESSAGES_UPDATE'; chatMessages: ChatMessage[]; timestamp: string }>) => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)vscode/core/src/extension.ts(2 hunks)vscode/core/src/extensionState.ts(1 hunks)vscode/core/src/utilities/ModifiedFiles/processMessage.ts(3 hunks)webview-ui/src/components/ViolationIncidentsList.tsx(2 hunks)webview-ui/src/context/ExtensionStateContext.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
vscode/core/src/extensionState.ts (1)
shared/src/types/types.ts (1)
ExtensionData(114-136)
vscode/core/src/extension.ts (1)
shared/src/types/types.ts (1)
ExtensionData(114-136)
webview-ui/src/context/ExtensionStateContext.tsx (1)
shared/src/types/types.ts (1)
ExtensionData(114-136)
🪛 GitHub Check: Lint
webview-ui/src/context/ExtensionStateContext.tsx
[warning] 86-86:
Replace ⏎··········?·data.enhancedIncidents⏎········· with ·?·data.enhancedIncidents
[warning] 75-75:
Replace ·?·event.data.chatMessages with ⏎············?·event.data.chatMessages⏎···········
[warning] 73-73:
Replace prevState with (prevState)
[warning] 72-72:
Replace 'type'·in·event.data·&&·event.data.type·===·'CHAT_MESSAGES_UPDATE' with "type"·in·event.data·&&·event.data.type·===·"CHAT_MESSAGES_UPDATE"
[warning] 70-70:
Replace event:·MessageEvent<ExtensionData·|·{·type:·string;·chatMessages:·any[];·timestamp:·string·}> with ⏎······event:·MessageEvent<ExtensionData·|·{·type:·string;·chatMessages:·any[];·timestamp:·string·}>,⏎····
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (macos)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
🔇 Additional comments (4)
.gitignore (1)
20-21: LGTM!These ignore patterns appropriately exclude VSCode test artifacts and Eclipse workspace files from version control.
vscode/core/src/extensionState.ts (1)
28-28: LGTM!The new
mutateChatMessagesmethod signature correctly mirrors the existingmutateDatapattern and provides the foundation for selective chat message updates.webview-ui/src/components/ViolationIncidentsList.tsx (1)
114-114: LGTM!Removing debug console logs improves performance and code cleanliness for production.
Also applies to: 205-205
vscode/core/src/utilities/ModifiedFiles/processMessage.ts (1)
164-164: Correct usage of the new API, but effectiveness depends on fixing the duplicate propagation issue.The migration from
state.mutateDatatostate.mutateChatMessagesis appropriate since all three call sites only mutate thechatMessagesarray. However, the full benefit of this change depends on resolving the duplicate state propagation issue identified invscode/core/src/extension.ts(lines 119-136).Also applies to: 295-295, 308-308
| // Selective state mutator for chat messages only | ||
| const mutateChatMessages = ( | ||
| recipe: (draft: ExtensionData) => void, | ||
| ): Immutable<ExtensionData> => { | ||
| const data = produce(getData(), recipe); | ||
| setData(data); | ||
|
|
||
| // Send only chat messages to webview instead of full state | ||
| this.state.webviewProviders.forEach((provider) => { | ||
| provider.sendMessageToWebview({ | ||
| type: "CHAT_MESSAGES_UPDATE", | ||
| chatMessages: data.chatMessages, | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
| }); | ||
|
|
||
| return data; | ||
| }; |
There was a problem hiding this comment.
Critical: Duplicate state propagation defeats message bloat reduction.
The mutateChatMessages function calls setData(data) on line 124, which fires the _onDidChange event. The existing listeners on lines 676-680 are subscribed to this event and send the FULL ExtensionData to all webviews:
this.onDidChangeData((data) => {
provider.sendMessageToWebview(data); // Sends full state including all chatMessages
})Then lines 127-133 send the selective CHAT_MESSAGES_UPDATE. This means webviews receive:
- Full
ExtensionData(including the completechatMessagesarray) - Selective
CHAT_MESSAGES_UPDATE(just thechatMessages)
This sends MORE data than before, not less, defeating the PR's goal of addressing message bloat.
Solution: Consider one of these approaches:
- Introduce a separate event emitter for chat-only changes that doesn't trigger full state propagation
- Modify
mutateChatMessagesto update internal state without firingonDidChange - Add a flag to distinguish between full state changes and chat-only changes, and filter in the listeners
🤖 Prompt for AI Agents
In vscode/core/src/extension.ts around lines 119-136, calling setData(data)
inside mutateChatMessages triggers the global onDidChangeData listeners (sending
the full ExtensionData) and then you also send a CHAT_MESSAGES_UPDATE, causing
duplicate/full-state propagation and negating the message-bloat fix; fix by
preventing the global onDidChangeData event from firing for chat-only updates:
add a dedicated chat-only EventEmitter (e.g., onChatMessagesChange) and have
mutateChatMessages update the in-memory data without invoking the global emitter
(either by using a setDataSilent internal helper or by passing an options flag
to setData to suppress the global event), then emit only the new chat-only event
to notify webviews; update existing global onDidChangeData listeners to remain
unchanged and replace chat-specific webview sends to subscribe to the new chat
emitter so webviews receive only the selective CHAT_MESSAGES_UPDATE.
2 participants
Related to #888
Summary by CodeRabbit
Release Notes
Refactor
Chores