Skip to content

Session Based Chat Notifications - #302

Merged
ND68 merged 2 commits into
mainfrom
andy/291-session-based-chat-notifications
Mar 9, 2026
Merged

ND68 merged 2 commits into
mainfrom
andy/291-session-based-chat-notifications

Conversation

@idke64

@idke64 idke64 commented Mar 8, 2026 •

Copy link
Copy Markdown
Contributor

Description

Resolves ticket number: #291

Explain what your code changes:
Added support for using sessionStorage to persist and mirror chat notifications across refreshes. Also cleaned up logic for dismissing notifications by putting it in NotificationsHandler.tsx.

List the steps you took to test your code:
Triggered chat notifications and navigated across pages.

Checklist

  • The ticket is mentioned above
  • The changes fulfill the success criteria of the ticket
  • The changes were self-reviewed
  • A review was requested

@netlify

netlify Bot commented Mar 8, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for hope-for-haiti ready!

Name Link
🔨 Latest commit 7c7da36
🔍 Latest deploy log https://app.netlify.com/projects/hope-for-haiti/deploys/69ad0d06590bef0008d53141
😎 Deploy Preview https://deploy-preview-302--hope-for-haiti.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@greptile-apps

greptile-apps Bot commented Mar 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds sessionStorage persistence for chat notifications so they survive page refreshes within the same tab, and consolidates dismiss logic into a single dismissNotification function exposed through NotificationContext. It also fixes a pre-existing bug where chat notifications were impossible to dismiss from the notification panel.

Key changes:

  • NotificationHandler now hydrates chat notifications from sessionStorage on mount and mirrors them back on every change, clearing on logout
  • A new dismissNotification callback is added to the context, centralising the delete-from-API + remove-from-state logic that was previously duplicated in NotificationCard
  • FloatingNotification is decoupled from its prop dependency and now consumes notifications directly via useNotifications()
  • Auto-dismiss of chat notifications when the user navigates to the notification's target URL

Area needing improvement:

  • The isChat guard was removed from dismissNotification. For chat notifications that fall back to Date.now() as their ID (when event.message?.id is undefined), Number(id) > 0 evaluates to true and an unnecessary DELETE /api/notifications/<timestamp> request is fired. The original !isChat && Number(id) > 0 check should be preserved.

Confidence Score: 4/5

  • Safe to merge after addressing the missing isChat guard in dismissNotification to prevent unnecessary API calls.
  • The refactoring is clean and the feature works correctly for the common case. One logic bug exists: the missing isChat check can trigger unintended DELETE API calls for chat notifications with numeric fallback IDs. This should be fixed before merging to avoid unnecessary network requests.
  • src/components/NotificationHandler.tsx — specifically the dismissNotification callback (lines 113–125)

Sequence Diagram

sequenceDiagram
    participant Browser as Browser (Tab)
    participant NH as NotificationHandler
    participant SS as sessionStorage
    participant API as /api/notifications
    participant Stream as Stream Chat

    Note over NH: On Mount
    NH->>SS: getItem("chat_notifications")
    SS-->>NH: stored JSON (or null)
    NH->>NH: setNotifications([...hydrated, ...prev])
    NH->>NH: isHydrated.current = true

    Note over NH: After re-render (notifications updated)
    NH->>SS: setItem("chat_notifications", chatNotifs)

    Note over NH: On user event (new chat message)
    Stream-->>NH: notification.message_new
    NH->>NH: setNotifications([newChatNotif, ...prev])
    NH->>SS: setItem("chat_notifications", updated)

    Note over NH: On dismissNotification(id)
    alt !isChat && Number(id) > 0 (DB notification)
        NH->>API: DELETE /api/notifications/:id
        API-->>NH: 200 OK
    end
    NH->>NH: setNotifications(prev.filter(n => n.id !== id))
    NH->>SS: setItem("chat_notifications", filtered)

    Note over NH: On navigation to action URL
    NH->>NH: filter out matching chat notifications
    NH->>SS: setItem("chat_notifications", remaining)

    Note over NH: On logout
    NH->>SS: removeItem("chat_notifications")
Loading

Last reviewed commit: 6b0a95a

Comment on lines +113 to +125
const dismissNotification = useCallback(
async (id: string | number) => {
if (Number(id) > 0) {
try {
await apiClient.delete(`/api/notifications/${id}`);
} catch (error) {
console.error(`Failed to delete notification ${id}: ${error}`);
}
}
setNotifications((prev) => prev.filter((n) => n.id !== id));
},
[apiClient]
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing isChat guard before DELETE call

The dismissNotification callback checks Number(id) > 0 before making the DELETE API call, but this is insufficient for chat notifications.

Chat notification IDs fall back to Date.now() when event.message?.id is undefined (line 269). A timestamp like 1709913600000 evaluates to Number(id) > 0 === true, so the callback fires an unnecessary DELETE /api/notifications/<timestamp> request for a notification that only exists locally (in sessionStorage), not in the database.

Since every notification has an explicit isChat flag, the fix is straightforward — look up the notification from state and preserve the original guard:

const dismissNotification = useCallback(
  async (id: string | number) => {
    const notification = notifications.find((n) => n.id === id);
    if (!notification?.isChat && Number(id) > 0) {
      try {
        await apiClient.delete(`/api/notifications/${id}`);
      } catch (error) {
        console.error(`Failed to delete notification ${id}: ${error}`);
      }
    }
    setNotifications((prev) => prev.filter((n) => n.id !== id));
  },
  [apiClient, notifications]
);

@idke64 idke64 changed the title session based chat notifications Session Based Chat Notifications Mar 8, 2026
@idke64
idke64 force-pushed the andy/291-session-based-chat-notifications branch from a02ec5a to 7c7da36 Compare March 8, 2026 05:45
@ND68

ND68 commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

nice work, thanks for refactoring some of the notification handler functions, the code is really clean

@ND68
ND68 merged commit c2f609b into main Mar 9, 2026
5 checks passed
@ND68
ND68 deleted the andy/291-session-based-chat-notifications branch March 9, 2026 17:02
@ND68 ND68 linked an issue Mar 9, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Session-Based Chat Notifications

2 participants