feat: add notification queue and priority management#1700
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesNotificationQueue feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/widgets/src/feedback/NotificationQueue.ts`:
- Around line 32-33: The getAll() method in NotificationQueue exposes the
mutable internal queue array directly, allowing callers to modify it and bypass
the add() method's invariants like priority defaults and sorting. Fix this by
returning a shallow copy of the queue array instead of returning this.queue
directly, using either the spread operator or the slice() method to create a new
array instance that callers can modify without affecting the internal state.
In `@packages/widgets/src/index.ts`:
- Around line 132-139: The `SplitPaneOptions` type is being exported twice from
the same source file './layout/SplitPane.js'. Remove `SplitPaneOptions` from the
second export statement (the one that also exports `SplitDirection`) and keep
only `SplitDirection` in that second export. The first export of
`SplitPaneOptions` on line 132 should remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4c416c3-9c74-43e5-80af-3a83d0ef5c2e
📒 Files selected for processing (2)
packages/widgets/src/feedback/NotificationQueue.tspackages/widgets/src/index.ts
| getAll(): Notification[] { | ||
| return this.queue; |
There was a problem hiding this comment.
getAll() exposes mutable internal state.
Line 33 returns the backing array directly, so callers can mutate queue contents/order and bypass add() invariants (priority defaults + sorting).
Proposed fix
- getAll(): Notification[] {
- return this.queue;
- }
+ getAll(): ReadonlyArray<Notification> {
+ return [...this.queue];
+ }📝 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.
| getAll(): Notification[] { | |
| return this.queue; | |
| getAll(): ReadonlyArray<Notification> { | |
| return [...this.queue]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/feedback/NotificationQueue.ts` around lines 32 - 33, The
getAll() method in NotificationQueue exposes the mutable internal queue array
directly, allowing callers to modify it and bypass the add() method's invariants
like priority defaults and sorting. Fix this by returning a shallow copy of the
queue array instead of returning this.queue directly, using either the spread
operator or the slice() method to create a new array instance that callers can
modify without affecting the internal state.
| export type { SplitPaneOptions } from './layout/SplitPane.js'; | ||
| export { NotificationQueue } from "./feedback/NotificationQueue.js"; | ||
|
|
||
| export type { | ||
| Notification, | ||
| NotificationPriority | ||
| } from "./feedback/NotificationQueue.js"; | ||
| export type { SplitPaneOptions, SplitDirection } from './layout/SplitPane.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify duplicate SplitPaneOptions type exports in the index module
rg -n "export type \{[^}]*SplitPaneOptions" packages/widgets/src/index.tsRepository: Karanjot786/TermUI
Length of output: 212
🏁 Script executed:
sed -n '130,141p' packages/widgets/src/index.tsRepository: Karanjot786/TermUI
Length of output: 536
🏁 Script executed:
# Check for any other duplicate exports in the file
rg "^export (type )?\{ [^}]*\}" packages/widgets/src/index.ts | sort | uniq -dRepository: Karanjot786/TermUI
Length of output: 44
Remove duplicate SplitPaneOptions type export.
SplitPaneOptions is exported on line 132 and again on line 139. TypeScript will error on the duplicate identifier. Keep the export on line 132 and remove it from line 139.
Fix
export { SplitPane } from './layout/SplitPane.js';
export type { SplitPaneOptions } from './layout/SplitPane.js';
export { NotificationQueue } from "./feedback/NotificationQueue.js";
export type {
Notification,
NotificationPriority
} from "./feedback/NotificationQueue.js";
-export type { SplitPaneOptions, SplitDirection } from './layout/SplitPane.js';
+export type { SplitDirection } from './layout/SplitPane.js';📝 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 type { SplitPaneOptions } from './layout/SplitPane.js'; | |
| export { NotificationQueue } from "./feedback/NotificationQueue.js"; | |
| export type { | |
| Notification, | |
| NotificationPriority | |
| } from "./feedback/NotificationQueue.js"; | |
| export type { SplitPaneOptions, SplitDirection } from './layout/SplitPane.js'; | |
| export type { SplitPaneOptions } from './layout/SplitPane.js'; | |
| export { NotificationQueue } from "./feedback/NotificationQueue.js"; | |
| export type { | |
| Notification, | |
| NotificationPriority | |
| } from "./feedback/NotificationQueue.js"; | |
| export type { SplitDirection } from './layout/SplitPane.js'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/index.ts` around lines 132 - 139, The `SplitPaneOptions`
type is being exported twice from the same source file './layout/SplitPane.js'.
Remove `SplitPaneOptions` from the second export statement (the one that also
exports `SplitDirection`) and keep only `SplitDirection` in that second export.
The first export of `SplitPaneOptions` on line 132 should remain unchanged.
|
@Karanjot786 resolved |
Description
Added a notification queue system with priority handling and queue controls.
Changes
Closes #1695
Summary by CodeRabbit