Skip to content

feat(notification): sync last_read across devices via homeserver SSE - #1955

Open
Taewa wants to merge 17 commits into
devfrom
bug/1595-notification-read-cross-device-sync
Open

feat(notification): sync last_read across devices via homeserver SSE#1955
Taewa wants to merge 17 commits into
devfrom
bug/1595-notification-read-cross-device-sync

Conversation

@Taewa

@Taewa Taewa commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1595 — read notifications on one device now clear on other active sessions. Sibling to the mute-list sync (#1787).

Root cause

  • Read state lives on the homeserver (last_read), but a device only read it once, at startup.
  • Opening the notifications page writes a new last_read; other devices never re-read it, so their
    unread badge stayed stale.
  • The cross-device re-read also returned a cached value: single-file reads went through the browser
    HTTP cache, so a value written elsewhere stayed invisible until the local cache was invalidated. (Mute
    didn't hit this — it reads a directory listing, which isn't cached.)

Fix

  • Live sync coordinator — subscribes to the homeserver event stream for last_read; on change,
    debounces then refreshes the local read state and recomputes the unread badge.
  • Race guard — never moves last_read backward (ignores stale/echo values).
  • Cache bypass — the cross-device refresh reads the live value instead of the cached copy. Normal
    startup read keeps the cached path.
  • Bounded reconnect/refresh — exponential backoff with a max-attempt cap so a persistently
    unreachable homeserver can't loop forever.

Testing

  • Unit: application, controller, coordinator (events, debounce, race guard, route/visibility gating,
    cursor persistence, retry cap), homeserver cache-bypass, bootstrap regression.
  • Manual: marked read on Chrome → Firefox badge clears, and vice versa; verified the fresh (non-cached)
    last_read GET in the network tab.

Manual test

1955-notification-sync.mp4

Tested on URL: https://pubky-app-preview-pr-1955-fzxmjul7ya-oa.a.run.app/
pubky-app-preview-pr-1955 • 549d379

Read state marked on one device now propagates to other active sessions.
A NotificationLastReadSyncCoordinator subscribes to the homeserver
last_read event stream and, on PUT, refreshes the local lastRead and
recomputes the unread badge (race-guarded so the value never goes
backwards). Mirrors the mute-list SSE pattern (#1787).

- Add HomeserverService.request({ noCache }) — bypasses the HTTP cache
via a no-store SDK fetch so cross-device reads see the live value
(owned-session GET was served stale from cache)
- Add NotificationApplication.fetchLastReadFromHomeserver /
subscribeLastReadEventStream; bootstrap GET refactored to reuse them
- Add NotificationController.refreshLastReadFromHomeserver /
subscribeLastReadEventStream
- Cap reconnect/refresh retries (max 10, exponential backoff) to avoid
unbounded loops and Sentry storms on persistent failure
- Register coordinator in CoordinatorsManager; clear its SSE cursor on
logout
- Tests: application, controller, coordinator, homeserver, bootstrap

Closes #1595
@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds cross-device last_read sync for notifications by subscribing to the homeserver SSE stream and debouncing a cache-bypassed refresh when another session marks notifications as read.

  • NotificationLastReadSyncCoordinator — a long-lived coordinator that opens an SSE stream for /pub/pubky.app/last_read, applies a 500 ms debounce on PUT events, and calls NotificationController.refreshLastReadFromHomeserver with a race guard (never moves last_read backward) and exponential-backoff retry capped at 10 attempts.
  • Cache bypassHomeserverService.requestFreshJson issues cache: 'no-store' via the SDK client for cross-device reads; the normal startup path keeps the cached route.
  • Cleanup — sessionStorage cursors are cleared on logout; the previously flagged TLastReadEvent cross-layer type dependency is resolved by moving the type to application/notification/notification.types.ts.

Confidence Score: 5/5

Safe to merge — the coordinator, controller, application, and service changes are consistent and layer boundaries are respected throughout.

All three layers (coordinator → controller → application → service) are correctly wired with no cross-layer shortcuts. The previously flagged TLastReadEvent type has been moved to the application layer, resolving that concern. The race guard, generation-based loop cancellation, debounce, and bounded backoff are all verified by comprehensive unit tests. No logic bugs, missing guards, or architectural violations were found.

No files require special attention.

Important Files Changed

Filename Overview
src/core/coordinators/notification-last-read-sync/notification-last-read-sync.ts New coordinator; subscribes to the homeserver SSE for last_read changes with exponential backoff, debounced refresh, race guard, and generation-based loop cancellation. Layer boundaries respected: only calls NotificationController.
src/core/application/notification/notification.types.ts Adds TLastReadEvent to the application types layer — correctly resolves the previously flagged cross-layer type import.
src/core/controllers/notification/notification.ts Adds refreshLastReadFromHomeserver (with race guard) and subscribeLastReadEventStream delegation. Both correctly go through NotificationApplication; Zustand store access is appropriate at the controller layer.
src/core/services/homeserver/homeserver.ts Adds requestFreshJson private method for cache-bypassed GETs (cache: no-store via SDK client); noCache flag in request() routes to it. Pattern mirrors the existing request paths correctly.
src/components/organisms/CoordinatorsManager/CoordinatorsManager.tsx Wires NotificationLastReadSyncCoordinator into the app shell alongside existing coordinators; start/stop/setRoute delegations are symmetric and correct.
src/core/controllers/auth/auth.ts Clears the last_read SSE cursor from sessionStorage on logout, consistent with the mute-list pattern already in place.

Sequence Diagram

sequenceDiagram
    participant TabA as Device A (Chrome)
    participant TabB as Device B (Firefox)
    participant HS as Homeserver
    participant CM as CoordinatorsManager
    participant NLR as NotificationLastReadSyncCoordinator
    participant NC as NotificationController
    participant NA as NotificationApplication
    participant Store as NotificationStore

    CM->>NLR: start() / setRoute()
    NLR->>NC: subscribeLastReadEventStream(pubky, cursor)
    NC->>NA: subscribeLastReadEventStream(pubky, cursor)
    NA->>HS: SSE /pub/pubky.app/last_read (SDK stream)
    HS-->>NLR: stream open

    TabA->>NC: getOrFetchNotifications() [opens notifications page]
    NC->>HS: PUT last_read (new timestamp)
    HS-->>NLR: PUT event (cursor)
    Note over NLR: debounce 500ms
    NLR->>NC: refreshLastReadFromHomeserver(pubky)
    NC->>NA: "fetchLastReadFromHomeserver(url, noCache=true)"
    NA->>HS: GET last_read (cache: no-store)
    HS-->>NA: "{timestamp: T}"
    Note over NC: race guard: remote > local?
    NC->>Store: setLastRead(T)
    NC->>Store: setUnread(count)
    TabB-->>TabB: badge clears
Loading

Reviews (3): Last reviewed commit: "Merge branch 'dev' into bug/1595-notific..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

🚀 Preview Deployment

URL: https://pubky-app-pr-1955-fzxmjul7ya-oa.a.run.app

pubky-app-pr-1955d9e6489

@Taewa

Taewa commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator Author

@greptile-apps
Fixed your feedback "Upward layer dependency: Application imports from Controller".

Review again plz.

@Taewa Taewa changed the title [WIP] feat(notification): sync last_read across devices via homeserver SSE feat(notification): sync last_read across devices via homeserver SSE Jun 6, 2026
@Taewa
Taewa requested a review from infin1t3 June 8, 2026 07:38
@Taewa Taewa self-assigned this Jun 8, 2026
@Taewa Taewa added 📖 documentation Improvements or additions to documentation 📈 enhancement New feature or request 🪾 homeserver Sync homeserver events ⚙️ core labels Jun 8, 2026
@Taewa Taewa added this to the v1.6.0 milestone Jun 8, 2026
@infin1t3

infin1t3 commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

@greptileai review

}

private setupListeners(): void {
this.authStoreUnsubscribe = useAuthStore.subscribe((state, prevState) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

    this.authStoreUnsubscribe = useAuthStore.subscribe((state, prevState) => {
      const isAuthenticated = state.selectIsAuthenticated();
      const wasAuthenticated = prevState.selectIsAuthenticated();
      const profileChanged = state.hasProfile !== prevState.hasProfile;
      const userChanged = state.currentUserPubky !== prevState.currentUserPubky;
      if (isAuthenticated !== wasAuthenticated || profileChanged || userChanged) {
        this.evaluateStreaming();
      }
    });

Exists verbatim in mute-list-sync.ts - you could consider extracting to an utility or a helper.

More importantly - This should compare the snapshot fields directly instead of calling selectIsAuthenticated() on state / prevState.

selectIsAuthenticated() is defined as a function that uses Zustand’s get() internally. That means the function does not read from the object it is attached to. It always asks the store for the latest current state.

So inside this subscription:

useAuthStore.subscribe((state, prevState) => {
  const isAuthenticated = state.selectIsAuthenticated();
  const wasAuthenticated = prevState.selectIsAuthenticated();
});

both calls can read the same latest store value. If session just changed from null to restored, we expect isAuthenticated = true and wasAuthenticated = false, but both calls may return true.

Suggested fix:

const isAuthenticated = state.session !== null;
const wasAuthenticated = prevState.session !== null;

Same duplicated pattern exists in MuteListSyncCoordinator, so we should update both coordinators for consistency.

* Example: device A marks read at t=1000 → PUT → SSE echo back to A → refresh()
* fetches remote=1000, local=1000 → no-op.
*/
static async refreshLastReadFromHomeserver(pubky: Pubky): Promise<void> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This one is from Cursor - refreshLastReadFromHomeserver introduces a refresh* controller method.

Why it matters: Controller prefixes are standardized (fetch*, get*, getOrFetch*, commit*, subscribe*) so call sites communicate cache/network/write behavior.

Fix: Rename to a documented prefix, e.g. fetchLastReadFromHomeserverAndReconcile or similar, then update coordinator/tests/docs.

  • fetch* — network only, no cache
  • get* — local only
  • getMany* — bulk local reads, returns Map<Pubky, T>
  • getOrFetch* — local first, network fallback
  • getMany*OrFetch — bulk local first, fetch missing
  • commitCreate* / commitUpdate* / commitDelete* — optimistic local write + network sync
  • subscribe* — long-lived live stream subscription

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚙️ core 📖 documentation Improvements or additions to documentation 📈 enhancement New feature or request 🪾 homeserver Sync homeserver events

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Read notifications on devices still shows unread on another

2 participants