Add notifications from the kilocode backend #1561
Conversation
🦋 Changeset detectedLatest commit: a5b1323 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces a notifications system integrated with the Kilocode backend. It adds support for fetching, displaying, dismissing, and tracking notifications both in the extension state and in the webview UI. The update includes schema changes, backend handlers, UI components, and state management to support notifications, along with Storybook test coverage and a changeset entry. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ChatView/KilocodeNotifications
participant Webview as Webview JS
participant ExtHost as Extension Host (webviewMessageHandler)
participant Provider as ClineProvider
participant Kilocode as Kilocode Backend
UI->>Webview: postMessage({ type: "fetchKilocodeNotifications" })
Webview->>ExtHost: "fetchKilocodeNotifications"
ExtHost->>Provider: fetchKilocodeNotificationsHandler
Provider->>Kilocode: GET /notifications (with token)
Kilocode-->>Provider: Notifications[]
Provider->>ExtHost: Notifications[]
ExtHost->>Webview: postMessage({ type: "kilocodeNotificationsResponse", notifications })
Webview->>UI: Event: kilocodeNotificationsResponse
UI->>User: Display notifications
User->>UI: Dismiss notification
UI->>Webview: postMessage({ type: "dismissNotificationId", notificationId })
Webview->>ExtHost: "dismissNotificationId"
ExtHost->>Provider: Update dismissedNotificationIds
ExtHost->>Webview: postMessage({ type: "state", dismissedNotificationIds })
Webview->>UI: Update context state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
markijbema
changed the title
.changeset/early-lemons-guess.md
Outdated
| "kilo-code": patch | ||
| "@roo-code/vscode-webview": patch | ||
| "@roo-code/storybook": patch | ||
| "@roo-code/types": patch |
There was a problem hiding this comment.
Atm we should not update versions for the roo code dependencies because this breaks the release process currently and clashes a bit with future merge.
| setEnhancementApiConfigId: (value: string) => void | ||
| commitMessageApiConfigId?: string // kilocode_change | ||
| setCommitMessageApiConfigId: (value: string) => void // kilocode_change | ||
| markNotificationAsDismissed: (notificationId: string) => void // kilocode_change |
There was a problem hiding this comment.
suggestion: something like setNotificationDismissed or anything with set since this is a bit the convention here. No blocker if you disagree.
There was a problem hiding this comment.
the reason we choose for another name is that set also implies you can pass in a value, whereas this will always add it to the dismissed set
Co-authored-by: Christiaan Arnoldus <christiaan.arnoldus@outlook.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/core/webview/webviewMessageHandler.ts (1)
2567-2581: Code organization follows established patterns.The implementation correctly handles both new message types within the kilocode_change markers as discussed in past reviews. The dismissNotificationId handler appropriately uses getGlobalState as noted in previous comments, and the overall approach aligns with the file's existing structure.
🧹 Nitpick comments (1)
webview-ui/src/components/kilocode/KilocodeNotifications.tsx (1)
18-24: Consider optimizing notification filtering.The filtered notifications are recalculated on every render. Consider memoizing this computation for better performance.
Apply this diff to add memoization:
+import React, { useEffect, useState, useMemo } from "react" export const KilocodeNotifications: React.FC = () => { const { dismissedNotificationIds } = useExtensionState() const [notifications, setNotifications] = useState<Notification[]>([]) - const filteredNotifications = notifications.filter( - (notification) => !dismissedNotificationIds.includes(notification.id), - ) + const filteredNotifications = useMemo( + () => notifications.filter( + (notification) => !dismissedNotificationIds.includes(notification.id), + ), + [notifications, dismissedNotificationIds] + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.changeset/early-lemons-guess.md(1 hunks)apps/storybook/stories/ChatView.stories.tsx(2 hunks)packages/types/src/global-settings.ts(2 hunks)src/core/kilocode/webview/webviewMessageHandlerUtils.ts(2 hunks)src/core/webview/ClineProvider.ts(3 hunks)src/core/webview/webviewMessageHandler.ts(2 hunks)src/shared/ExtensionMessage.ts(3 hunks)src/shared/WebviewMessage.ts(3 hunks)webview-ui/src/components/chat/ChatView.tsx(3 hunks)webview-ui/src/components/kilocode/KilocodeNotifications.tsx(1 hunks)webview-ui/src/context/ExtensionStateContext.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*kilocode*)/**/*.{ts,tsx,js,jsx,html,css,scss}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/!(*kilocode*)/**/*.{ts,tsx,js,jsx,html,css,scss}: For single line changes, add the comment '// kilocode_change' at the end of the line.
For multiple consecutive lines changed, wrap them with '// kilocode_change start' and '// kilocode_change end' comments.
Files:
apps/storybook/stories/ChatView.stories.tsxsrc/shared/ExtensionMessage.tswebview-ui/src/components/kilocode/KilocodeNotifications.tsxwebview-ui/src/context/ExtensionStateContext.tsxpackages/types/src/global-settings.tswebview-ui/src/components/chat/ChatView.tsxsrc/core/webview/webviewMessageHandler.tssrc/shared/WebviewMessage.tssrc/core/kilocode/webview/webviewMessageHandlerUtils.tssrc/core/webview/ClineProvider.ts
**/!(*kilocode*)/**/*.{html,jsx,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
In HTML/JSX/TSX files, use '{/* kilocode_change start /}' and '{/ kilocode_change end */}' to wrap multi-line changes.
Files:
apps/storybook/stories/ChatView.stories.tsxwebview-ui/src/components/kilocode/KilocodeNotifications.tsxwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/components/chat/ChatView.tsx
**/*kilocode*/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
If the filename or directory name contains 'kilocode', no marking with comments is required.
Files:
webview-ui/src/components/kilocode/KilocodeNotifications.tsxsrc/core/kilocode/webview/webviewMessageHandlerUtils.ts
🧬 Code Graph Analysis (4)
webview-ui/src/components/kilocode/KilocodeNotifications.tsx (2)
webview-ui/src/context/ExtensionStateContext.tsx (1)
useExtensionState(564-572)webview-ui/src/utils/vscode.ts (1)
vscode(80-80)
webview-ui/src/context/ExtensionStateContext.tsx (1)
webview-ui/src/utils/vscode.ts (1)
setState(69-76)
webview-ui/src/components/chat/ChatView.tsx (2)
webview-ui/src/components/kilocode/KilocodeNotifications.tsx (1)
KilocodeNotifications(18-121)webview-ui/src/components/kilocode/chat/IdeaSuggestionsBox.tsx (1)
IdeaSuggestionsBox(4-37)
src/core/kilocode/webview/webviewMessageHandlerUtils.ts (1)
src/core/webview/ClineProvider.ts (1)
ClineProvider(95-2224)
⏰ 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). (4)
- GitHub Check: storybook-playwright-snapshot
- GitHub Check: test-extension (windows-latest)
- GitHub Check: test-webview (windows-latest)
- GitHub Check: compile
🔇 Additional comments (27)
.changeset/early-lemons-guess.md (1)
1-5: No release process conflict detected for kilo-code update.
- All reviewed changesets in
.changeset/reference onlykilo-code, with noroo-codeversion bumps.- Although many
package.jsonfiles listroo-codedependencies, there are no changesets updating them.- Therefore, this
kilo-codepatch should not clash with past release process constraints concerningroo-code.src/core/webview/webviewMessageHandler.ts (3)
57-57: LGTM!The import statement correctly follows the established pattern and includes the required kilocode_change comment.
2567-2570: LGTM!The handler correctly delegates to the imported handler function and follows the established pattern in the file. The kilocode_change comments are properly applied.
2571-2581: LGTM!The handler properly validates the notification ID, safely updates the global state by appending to the existing dismissed notification IDs array, and keeps the webview in sync. The implementation follows established patterns and coding guidelines.
src/shared/WebviewMessage.ts (3)
234-234: LGTM!The new message type "dismissNotificationId" is appropriately named and follows the established pattern. The kilocode_change comment is correctly applied.
244-244: LGTM!The new message type "fetchKilocodeNotifications" is appropriately named and follows the established pattern.
273-273: LGTM!The optional notificationId property is correctly typed and appropriately placed. It supports the notification dismissal functionality and follows the established naming convention.
src/core/kilocode/webview/webviewMessageHandlerUtils.ts (1)
7-7: LGTM!The axios import is correctly added and needed for the HTTP request functionality.
apps/storybook/stories/ChatView.stories.tsx (2)
107-223: Well-implemented Storybook story with proper mocking.The story implementation is excellent:
- Proper event simulation: Uses
MessageEventto simulate the webview message system- Realistic mock data: Provides comprehensive notification examples
- Cleanup handling: Properly clears the timeout to prevent memory leaks
- Appropriate configuration: Sets API provider to "kilocode" and enables telemetry to trigger notification display
The story serves as a good demonstration of the notification system integration.
1-1: Incorrect coding guideline compliance.According to the coding guidelines, files that don't contain 'kilocode' in their path should mark changes with comments. However, since this appears to be a new file, each line should be marked individually rather than using a single comment for the entire file.
-// kilocode_change - new file +// Since this is a new file, each line should be marked individually according to the coding guidelinesLikely an incorrect or invalid review comment.
webview-ui/src/context/ExtensionStateContext.tsx (5)
37-37: LGTM!The property declaration is correctly typed and follows the established pattern in the interface.
130-130: Function naming is appropriate.Based on the past review discussion, the current naming
markNotificationAsDismissedis well-chosen as it clearly indicates the action being performed without implying a generic setter behavior.
220-220: LGTM!The property is correctly initialized as an empty array, which is the appropriate default state.
497-504: Implementation is correct and follows immutability patterns.The method correctly:
- Uses functional state update pattern
- Prepends the new notification ID to maintain order
- Handles the case where
dismissedNotificationIdsmight be undefined with fallback to empty array- Returns a new state object maintaining immutability
553-553: LGTM!The property is correctly included in the context value with proper fallback to empty array.
src/core/webview/ClineProvider.ts (3)
1507-1507: LGTM! Consistent state management for dismissed notifications.The addition of
dismissedNotificationIdsfollows the established pattern in the codebase for state management, with appropriate default values.
1636-1636: LGTM! Proper state propagation to webview.The
dismissedNotificationIdsis correctly included in the state object sent to the webview with a sensible default value.
1786-1786: LGTM! Complete state management implementation.The
dismissedNotificationIdsproperty is properly included in the main state getter with consistent defaulting behavior.src/shared/ExtensionMessage.ts (3)
123-123: LGTM! Well-structured message type addition.The new
kilocodeNotificationsResponsemessage type follows the established naming convention and integrates cleanly with existing message types.
194-204: LGTM! Well-defined notification structure.The notification interface is properly typed with all necessary fields:
- Required:
id,title,messagefor core notification data- Optional:
actionobject withactionTextandactionURLfor interactive notificationsThis provides a flexible structure that supports both informational and actionable notifications.
280-280: LGTM! Consistent ExtensionState type extension.The addition of
dismissedNotificationIdsto the ExtensionState type maintains consistency with the existing type structure and naming conventions.webview-ui/src/components/kilocode/KilocodeNotifications.tsx (1)
74-99: LGTM! Well-structured notification UI.The notification display logic is well implemented with:
- Proper conditional rendering
- Clean CSS classes using VSCode theme variables
- Accessible buttons with titles
- Good user experience with dismiss functionality
webview-ui/src/components/chat/ChatView.tsx (2)
60-60: LGTM!The import statement is correctly added with proper kilocode_change comment as required by the coding guidelines.
1788-1827: Layout restructuring looks good with proper conditional rendering.The changes successfully:
- Update container classes to support notifications with better flex layout
- Correctly invert the telemetry conditional logic (show notifications when telemetry is NOT "unset")
- Maintain existing functionality while adding notification support
- Follow coding guidelines with proper kilocode_change comments
The conditional rendering logic change is logical: when telemetry is unset, show the telemetry banner; otherwise, show notifications.
packages/types/src/global-settings.ts (3)
17-17: LGTM!The import statement is correctly added with proper kilocode_change comment.
139-141: Schema additions support the notification system well.The new properties are properly defined:
dismissedNotificationIdsas optional array to track dismissed notificationscommitMessageApiConfigIdas optional string for API configurationghostServiceSettingsas required schema for ghost service settingsNote that
ghostServiceSettingsis required while others are optional - verify this is intentional.
252-254: Default settings properly initialize the new notification properties.The defaults are well-chosen:
dismissedNotificationIds: []- empty array for fresh startsystemNotificationsEnabled: true- reasonable default to enable notificationsghostServiceSettings: {}- empty object matching the required schemaThese align perfectly with the schema definitions above.
| export const fetchKilocodeNotificationsHandler = async (provider: ClineProvider) => { | ||
| try { | ||
| const { apiConfiguration } = await provider.getState() | ||
| const kilocodeToken = apiConfiguration?.kilocodeToken | ||
|
|
||
| if (!kilocodeToken || apiConfiguration?.apiProvider !== "kilocode") { | ||
| provider.postMessageToWebview({ | ||
| type: "kilocodeNotificationsResponse", | ||
| notifications: [], | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| const response = await axios.get("https://kilocode.ai/api/users/notifications", { | ||
| headers: { | ||
| Authorization: `Bearer ${kilocodeToken}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| timeout: 5000, | ||
| }) | ||
|
|
||
| provider.postMessageToWebview({ | ||
| type: "kilocodeNotificationsResponse", | ||
| notifications: response.data?.notifications || [], | ||
| }) | ||
| } catch (error: any) { | ||
| provider.log(`Error fetching Kilocode notifications: ${error.message}`) | ||
| provider.postMessageToWebview({ | ||
| type: "kilocodeNotificationsResponse", | ||
| notifications: [], | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider security and maintainability improvements.
The function implementation is functionally correct but has several areas for improvement:
- Hardcoded API endpoint: The URL
https://kilocode.ai/api/users/notificationsshould be configurable - Token exposure in logs: If the request fails, the token might be exposed in error messages
- Generic error handling: All errors result in empty notifications array, losing specific error information
+// Consider moving to a config file
+const KILOCODE_API_BASE_URL = process.env.KILOCODE_API_URL || "https://kilocode.ai/api"
export const fetchKilocodeNotificationsHandler = async (provider: ClineProvider) => {
try {
const { apiConfiguration } = await provider.getState()
const kilocodeToken = apiConfiguration?.kilocodeToken
if (!kilocodeToken || apiConfiguration?.apiProvider !== "kilocode") {
provider.postMessageToWebview({
type: "kilocodeNotificationsResponse",
notifications: [],
})
return
}
- const response = await axios.get("https://kilocode.ai/api/users/notifications", {
+ const response = await axios.get(`${KILOCODE_API_BASE_URL}/users/notifications`, {
headers: {
Authorization: `Bearer ${kilocodeToken}`,
"Content-Type": "application/json",
},
timeout: 5000,
})
provider.postMessageToWebview({
type: "kilocodeNotificationsResponse",
notifications: response.data?.notifications || [],
})
} catch (error: any) {
- provider.log(`Error fetching Kilocode notifications: ${error.message}`)
+ // Log error without potentially exposing sensitive information
+ provider.log(`Error fetching Kilocode notifications: ${error.response?.status || 'Network error'}`)
provider.postMessageToWebview({
type: "kilocodeNotificationsResponse",
notifications: [],
})
}
}📝 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.
| export const fetchKilocodeNotificationsHandler = async (provider: ClineProvider) => { | |
| try { | |
| const { apiConfiguration } = await provider.getState() | |
| const kilocodeToken = apiConfiguration?.kilocodeToken | |
| if (!kilocodeToken || apiConfiguration?.apiProvider !== "kilocode") { | |
| provider.postMessageToWebview({ | |
| type: "kilocodeNotificationsResponse", | |
| notifications: [], | |
| }) | |
| return | |
| } | |
| const response = await axios.get("https://kilocode.ai/api/users/notifications", { | |
| headers: { | |
| Authorization: `Bearer ${kilocodeToken}`, | |
| "Content-Type": "application/json", | |
| }, | |
| timeout: 5000, | |
| }) | |
| provider.postMessageToWebview({ | |
| type: "kilocodeNotificationsResponse", | |
| notifications: response.data?.notifications || [], | |
| }) | |
| } catch (error: any) { | |
| provider.log(`Error fetching Kilocode notifications: ${error.message}`) | |
| provider.postMessageToWebview({ | |
| type: "kilocodeNotificationsResponse", | |
| notifications: [], | |
| }) | |
| } | |
| } | |
| // Consider moving to a config file | |
| const KILOCODE_API_BASE_URL = process.env.KILOCODE_API_URL || "https://kilocode.ai/api" | |
| export const fetchKilocodeNotificationsHandler = async (provider: ClineProvider) => { | |
| try { | |
| const { apiConfiguration } = await provider.getState() | |
| const kilocodeToken = apiConfiguration?.kilocodeToken | |
| if (!kilocodeToken || apiConfiguration?.apiProvider !== "kilocode") { | |
| provider.postMessageToWebview({ | |
| type: "kilocodeNotificationsResponse", | |
| notifications: [], | |
| }) | |
| return | |
| } | |
| const response = await axios.get(`${KILOCODE_API_BASE_URL}/users/notifications`, { | |
| headers: { | |
| Authorization: `Bearer ${kilocodeToken}`, | |
| "Content-Type": "application/json", | |
| }, | |
| timeout: 5000, | |
| }) | |
| provider.postMessageToWebview({ | |
| type: "kilocodeNotificationsResponse", | |
| notifications: response.data?.notifications || [], | |
| }) | |
| } catch (error: any) { | |
| // Log error without potentially exposing sensitive information | |
| provider.log(`Error fetching Kilocode notifications: ${error.response?.status || 'Network error'}`) | |
| provider.postMessageToWebview({ | |
| type: "kilocodeNotificationsResponse", | |
| notifications: [], | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/core/kilocode/webview/webviewMessageHandlerUtils.ts lines 71 to 103,
improve security and maintainability by making the API endpoint configurable via
environment variables or configuration files instead of hardcoding it. Avoid
exposing the kilocodeToken in any logs or error messages by ensuring error
logging does not include sensitive data. Enhance error handling by
distinguishing different error types or statuses and providing more informative
messages or fallback behaviors rather than always returning an empty
notifications array.
| useEffect(() => { | ||
| const handleMessage = (event: MessageEvent) => { | ||
| const message = event.data | ||
| if (message.type === "kilocodeNotificationsResponse") { | ||
| setNotifications(message.notifications || []) | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener("message", handleMessage) | ||
|
|
||
| setCurrentIndex(Math.max(0, Math.min(currentIndex, filteredNotifications.length - 1))) | ||
|
|
||
| return () => { | ||
| window.removeEventListener("message", handleMessage) | ||
| } | ||
| }, [currentIndex, filteredNotifications.length, dismissedNotificationIds]) |
There was a problem hiding this comment.
Fix useEffect dependency and index bounds logic.
The useEffect has a problematic dependency on dismissedNotificationIds and the index adjustment logic could cause issues.
Apply this diff to fix the dependency and bounds checking:
useEffect(() => {
const handleMessage = (event: MessageEvent) => {
const message = event.data
if (message.type === "kilocodeNotificationsResponse") {
setNotifications(message.notifications || [])
}
}
window.addEventListener("message", handleMessage)
- setCurrentIndex(Math.max(0, Math.min(currentIndex, filteredNotifications.length - 1)))
return () => {
window.removeEventListener("message", handleMessage)
}
-}, [currentIndex, filteredNotifications.length, dismissedNotificationIds])
+}, [])And add a separate useEffect for index bounds checking:
+useEffect(() => {
+ if (filteredNotifications.length > 0) {
+ setCurrentIndex(prev => Math.max(0, Math.min(prev, filteredNotifications.length - 1)))
+ } else {
+ setCurrentIndex(0)
+ }
+}, [filteredNotifications.length])📝 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.
| useEffect(() => { | |
| const handleMessage = (event: MessageEvent) => { | |
| const message = event.data | |
| if (message.type === "kilocodeNotificationsResponse") { | |
| setNotifications(message.notifications || []) | |
| } | |
| } | |
| window.addEventListener("message", handleMessage) | |
| setCurrentIndex(Math.max(0, Math.min(currentIndex, filteredNotifications.length - 1))) | |
| return () => { | |
| window.removeEventListener("message", handleMessage) | |
| } | |
| }, [currentIndex, filteredNotifications.length, dismissedNotificationIds]) | |
| useEffect(() => { | |
| const handleMessage = (event: MessageEvent) => { | |
| const message = event.data | |
| if (message.type === "kilocodeNotificationsResponse") { | |
| setNotifications(message.notifications || []) | |
| } | |
| } | |
| window.addEventListener("message", handleMessage) | |
| return () => { | |
| window.removeEventListener("message", handleMessage) | |
| } | |
| }, []) | |
| useEffect(() => { | |
| if (filteredNotifications.length > 0) { | |
| setCurrentIndex(prev => | |
| Math.max(0, Math.min(prev, filteredNotifications.length - 1)) | |
| ) | |
| } else { | |
| setCurrentIndex(0) | |
| } | |
| }, [filteredNotifications.length]) |
🤖 Prompt for AI Agents
In webview-ui/src/components/kilocode/KilocodeNotifications.tsx around lines 26
to 41, the useEffect incorrectly includes dismissedNotificationIds in its
dependency array and combines message handling with index bounds adjustment,
which can cause unnecessary re-renders and logic errors. To fix this, remove
dismissedNotificationIds from the dependencies of the message event listener
useEffect and isolate the index bounds checking into a separate useEffect that
only depends on currentIndex and filteredNotifications.length. This separation
ensures proper event handling and correct index adjustment without unintended
side effects.
We noticed people missing that they still had free credits waiting, so added a way to tell them!
Summary by CodeRabbit
New Features
Enhancements
Storybook