c/fix(notifications): check the notification is not already present w… #98
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the notifications system by preventing duplicate notifications from being added. The change adds a check to verify that a notification with the same ID is not already present before adding it to the store.
- Added duplicate notification prevention logic
- Imported the
getfunction from Svelte store to access notification values - Modified the
addNotificationfunction to check for existing notifications by ID
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
bromiesTM
force-pushed
the
kh/fix/multiple-survey-notifications
branch
from
e392cb2 to
17ad526
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
bromiesTM
force-pushed
the
kh/fix/multiple-survey-notifications
branch
from
17ad526 to
0213dc9
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| export const addNotification = (newNotificationStore: SubscribableNotification): void => { | ||
| const newNotification: Notification = get(newNotificationStore); | ||
| const currentNotifications = get(notifications); | ||
| const alreadyPresent: boolean = currentNotifications.some(n => get(n).id === newNotification.id); |
There was a problem hiding this comment.
This implementation calls get() on every notification store in the array for each duplicate check, which can be inefficient with many notifications. Consider caching the notification values or using a more efficient lookup mechanism like a Set of IDs.
| const alreadyPresent: boolean = currentNotifications.some(n => get(n).id === newNotification.id); | |
| const existingIds = new Set(currentNotifications.map(n => get(n).id)); | |
| const alreadyPresent: boolean = existingIds.has(newNotification.id); |
…n trying to add Signed-off-by: Kai Henseler <[email protected]>
Refs: PRODAI-380
Refs: PRODAI-380
It now takes the ID. Refs: PRODAI-380
Change from store.update() to removeNotification() to avoid duplicated logic. Refs: PRODAI-380
… .ts extension Refs: PRODAI-380
To fix typechecker error:
/app/src/lib/IONOS/components/notifications/NotificationBanner.svelte:23:13
Error: This comparison appears to be unintentional because the types 'NotificationType.SUCCESS | NotificationType.WARNING' and 'NotificationType.INFO' have no overlap. (ts)
<EmojiSad className="text-red-500"/>
{:else if $notification.type === NotificationType.INFO}
<Touch />
Reason FEEDBACK had the same value as INFO, thus always the first branch
was hit, never the last.
Strictly the types "PWA" or "feedback" should not belong to a
notification data type. This should be refactored in a separate commit.
Refs: PRODAI-380
…pe declaration We're iterating over SubscribableNotification. Refs: PRODAI-380
bromiesTM
force-pushed
the
kh/fix/multiple-survey-notifications
branch
from
0213dc9 to
05eaf2a
Compare
No description provided.