-
Notifications
You must be signed in to change notification settings - Fork 13k
feat: Add dispatchCustomNotification to desktop API
#37491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughIntroduces Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant API as DesktopAPI
participant Notif as Notification System
App->>API: dispatchCustomNotification({<br/>type: 'voice' | 'text',<br/>id?: string,<br/>payload: {...}<br/>})
activate API
API->>Notif: Process notification
Notif-->>App: Display notification
deactivate API
Note over App,Notif: Notification displayed
App->>API: closeCustomNotification(id)
activate API
API->>Notif: Dismiss notification
Notif-->>App: Notification closed
deactivate API
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37491 +/- ##
===========================================
- Coverage 68.98% 68.97% -0.01%
===========================================
Files 3358 3358
Lines 114228 114228
Branches 20537 20537
===========================================
- Hits 78796 78787 -9
- Misses 33343 33352 +9
Partials 2089 2089
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/desktop-api/src/index.ts (1)
30-31: Consider consistency with existing notification API.The new methods use different naming and type conventions compared to the existing notification API:
createNotification/destroyNotificationvs.dispatchCustomNotification/closeCustomNotificationcreateNotificationreturnsPromise<unknown>vs.dispatchCustomNotificationreturnsvoiddestroyNotificationtakesid: unknownvs.closeCustomNotificationtakesid: stringIf these differences are intentional (e.g., custom notifications work synchronously), consider adding JSDoc comments to clarify the distinction. Otherwise, aligning the naming and signatures would improve API consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/desktop-api/src/index.ts(1 hunks)
⏰ 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 Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
Proposed changes (including videos or screenshots)
Introduces a new method signature to the desktop API in order to allow for dispatching custom notifications.
Issue(s)
VGA-59
Steps to test or reproduce
Further comments
Summary by CodeRabbit