fix(js): dedupe notifications by id in cache.unshift#10785
Open
avasis-ai wants to merge 1 commit intonovuhq:nextfrom
Open
fix(js): dedupe notifications by id in cache.unshift#10785avasis-ai wants to merge 1 commit intonovuhq:nextfrom
avasis-ai wants to merge 1 commit intonovuhq:nextfrom
Conversation
…cates in useNotifications (novuhq#10762)
👷 Deploy request for dashboard-v2-novu-staging pending review.Visit the deploys page to approve it
|
Contributor
📝 WalkthroughWalkthroughFixed a deduplication bug in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/js/src/cache/notifications-cache.ts`:
- Around line 304-308: The current update only removes duplicates from the
single cached entry (`cachedData.notifications`) which still allows `getAll()`
to return duplicates across other cached pages; change the update to deduplicate
across the entire aggregated filter scope used by `getAll()`: locate all cache
entries that share the same filter/key (the same scope `getAll()` aggregates),
remove any notifications with `notification.id` from each of those cached
entries, and then insert `notificationInstance` at the front of the appropriate
page (e.g., the current `cachedData` entry) before calling `update(args,
{...})`; ensure you operate on every cached page for that filter so `getAll()`
cannot return duplicates.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb0ded45-9829-4cf7-b7b2-7ab99a34e7ab
📒 Files selected for processing (2)
packages/js/src/cache/notifications-cache.test.tspackages/js/src/cache/notifications-cache.ts
Comment on lines
+304
to
+308
| const dedupedNotifications = cachedData.notifications.filter((n) => n.id !== notification.id); | ||
|
|
||
| this.update(args, { | ||
| ...cachedData, | ||
| notifications: [notificationInstance, ...cachedData.notifications], | ||
| notifications: [notificationInstance, ...dedupedNotifications], |
Contributor
There was a problem hiding this comment.
Deduplicate across the aggregated filter scope, not only this cache key.
getAll() aggregates all pages with the same filter, but this only removes duplicates from the exact limit/offset cache entry. If the same notification already exists in another cached page for the same filter, getAll() can still return duplicates after unshift.
🐛 Proposed fix
const dedupedNotifications = cachedData.notifications.filter((n) => n.id !== notification.id);
+
+ const filterKey = getFilterKey(getFilter(cacheKey));
+ this.#cache.keys().forEach((key) => {
+ if (key === cacheKey || getFilterKey(getFilter(key)) !== filterKey) {
+ return;
+ }
+
+ const value = this.#cache.get(key);
+ if (!value) {
+ return;
+ }
+
+ const notifications = value.notifications.filter((n) => n.id !== notification.id);
+ if (notifications.length !== value.notifications.length) {
+ this.#cache.set(key, { ...value, notifications });
+ }
+ });
this.update(args, {
...cachedData,
notifications: [notificationInstance, ...dedupedNotifications],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/js/src/cache/notifications-cache.ts` around lines 304 - 308, The
current update only removes duplicates from the single cached entry
(`cachedData.notifications`) which still allows `getAll()` to return duplicates
across other cached pages; change the update to deduplicate across the entire
aggregated filter scope used by `getAll()`: locate all cache entries that share
the same filter/key (the same scope `getAll()` aggregates), remove any
notifications with `notification.id` from each of those cached entries, and then
insert `notificationInstance` at the front of the appropriate page (e.g., the
current `cachedData` entry) before calling `update(args, {...})`; ensure you
operate on every cached page for that filter so `getAll()` cannot return
duplicates.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Fixes #10762
The
notifications.cache.unshift()method did not deduplicate bynotification.id, causing duplicate notifications inuseNotifications()/cache.getAll()when multiple Novu client instances receive the same socket event (e.g., multiple browser tabs).Changes
packages/js/src/cache/notifications-cache.ts: Before prepending a notification inunshift(), filter out any existing entry with the sameid. This makesunshift()idempotent per notification id within a given query scope.packages/js/src/cache/notifications-cache.test.ts: Added two tests:How to test
<NovuProvider>anduseNotifications({ archived: false, limit: 10 })in 2+ browser tabsNote
Low Risk
Low risk: small, localized change to in-memory cache update logic plus added tests; main impact is altering list ordering/contents when the same notification id is unshifted repeatedly.
Overview
Fixes duplicate entries when prepending notifications into the JS
NotificationsCache.unshift()now filters out any existing cached notification with the sameidbefore prepending the new instance, making repeated socket events for the same notification idempotent.Adds tests covering (1) replacing an existing notification with the same
idand (2) prepending a genuinely new notification without creating duplicates.Reviewed by Cursor Bugbot for commit 4a7b784. Configure here.
What changed
The notification cache's unshift method now deduplicates notifications by ID. When prepending a notification to the cache, the method filters out any existing entry with the same ID before adding it. This prevents duplicate notifications from appearing when multiple client instances (e.g., multiple browser tabs) receive the same socket event.
Affected areas
js: Updated
NotificationsCache.unshiftto filter out existing notifications with the same ID before prepending a new notification instance, ensuring idempotent behavior across query scopes. Added two test cases that verify deduplication (unshifting a notification with an existing ID updates that entry without increasing the count) and prepend behavior (unshifting a new notification correctly adds it to the list).Testing
Added two Jest test cases for
NotificationsCache.unshiftthat verify: