feat: add note-scoped SSE event channel for page list (#860 Phase 4)#867
Merged
Conversation
Introduces `GET /api/notes/:noteId/events`, an SSE feed that pushes `page.added`, `page.updated`, `page.deleted`, and `note.permission_changed` events to subscribed clients so the `useInfiniteNotePages` cache patches in place instead of refetching every window after each mutation. Hocuspocus stays decoupled — it continues to own page-body sync over WebSocket while this feed covers note-list metadata only. - In-memory pub/sub (`noteEventBroadcaster`) partitioned by noteId, mirroring the `apiErrorBroadcaster` pattern with a 256-subscriber cap; designed to be swapped for Redis Pub/Sub without changing call sites. - Producers: `POST/DELETE /api/notes/:noteId/pages`, `POST /api/pages`, `DELETE /api/pages/:id`, `PUT /api/pages/:id/content` (only when title or content_preview changes), `PUT /api/notes/:noteId`, and member CRUD endpoints. - Frontend hook `useNotePageEvents` opens an `EventSource`, applies events to `noteKeys.pagesWindowByNoteId` via `setQueriesData`, and re-`invalidateQueries` on reconnect to cover events missed during the gap. Wired into `PageGrid` so the connection is scoped to the note view lifetime.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an in-memory per-note broadcaster, a subscribe-before-ready SSE endpoint at GET /api/notes/:noteId/events, server emission of note/page events from mutation handlers, and a frontend EventSource hook that patches React Query page windows; includes unit and integration tests across the stack. ChangesNote Page SSE Streaming & Real-Time Cache Patching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d470f13a5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+83
to
+85
| const { role, note } = await getNoteRole(noteId, userId, userEmail, db); | ||
| if (!note) throw new HTTPException(404, { message: "Note not found" }); | ||
| if (!role) throw new HTTPException(403, { message: "Forbidden" }); |
There was a problem hiding this comment.
This route authorizes only once before opening the stream, then keeps pushing page.* events without any further permission check. If a user is removed from a private/restricted note while their SSE connection is already open, they can continue receiving note metadata events until they disconnect, which leaks updates after revocation. Please gate each emitted event (or proactively close streams) when getNoteRole no longer grants access.
Useful? React with 👍 / 👎.
Comment on lines
+287
to
+289
| if (isFirstReady) { | ||
| isFirstReady = false; | ||
| return; |
There was a problem hiding this comment.
The client deliberately skips invalidation on the first ready, but the server emits ready before subscribeNoteEvents is registered. Any mutation published in that window is dropped and never reconciled, so the page window can stay stale until a later reconnect/manual refresh. Triggering one invalidate on the first ready (or subscribing before sending ready) is needed to close this startup race.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements real-time updates for note-scoped page mutations using Server-Sent Events (SSE). It introduces an in-memory broadcaster service, a new API endpoint for event streaming, and a frontend hook to patch the React Query cache dynamically. Feedback was provided to optimize the maybeEmitPageUpdated function by using the .returning() clause in the preceding update call, which would eliminate an unnecessary database roundtrip.
Comment on lines
+89
to
+102
| async function maybeEmitPageUpdated( | ||
| db: Database, | ||
| pageId: string, | ||
| body: { title?: string; content_preview?: string }, | ||
| ): Promise<void> { | ||
| if (body.title === undefined && body.content_preview === undefined) return; | ||
| const [row] = await db.select().from(pages).where(eq(pages.id, pageId)).limit(1); | ||
| if (!row || row.isDeleted) return; | ||
| publishNoteEvent({ | ||
| type: "page.updated", | ||
| note_id: row.noteId, | ||
| page: pageRowToWindowItem(row), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The maybeEmitPageUpdated function performs a separate db.select to fetch the page row. This adds an unnecessary database roundtrip since the updated row is often already available or can be retrieved using .returning() from the preceding update call in applyPagesMetadataUpdate. Consider refactoring applyPagesMetadataUpdate to return the updated row and passing it directly to this function.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@server/api/src/routes/notes/events.ts`:
- Around line 98-130: The event subscriber is registered after sending the
initial "ready" event which can lose events published in the gap; move the
subscribe call so subscribeNoteEvents(noteId, ...) is invoked and unsubscribe is
assigned before sending the ready event via stream.writeSSE, ensuring
writeChain/serializeNoteEvent/writeSSE logic remains attached to the stream
before the client commits; preserve the existing error handling and capacity
race comment and still emit the "ready" event (with retry) only after
subscription is live so no events are missed.
In `@server/api/src/routes/pages.ts`:
- Around line 89-101: The emit currently triggers just because
title/content_preview were present in the request; change the flow so emits only
happen when metadata actually changed by having applyPagesMetadataUpdate surface
a metadataChanged boolean (or include it in its return value) and pass that into
maybeEmitPageUpdated; update maybeEmitPageUpdated signature to accept
metadataChanged and only call publishNoteEvent when metadataChanged is true, and
update all call sites to propagate the new return/argument so behavior is gated
on real metadata changes.
In `@server/api/src/services/noteEventBroadcaster.ts`:
- Around line 117-127: The code increments totalSubscribers unconditionally
after bucket.add(listener), but Set.add is idempotent; change to detect if the
listener was actually new before incrementing: check bucket.has(listener) (or
capture size) before calling bucket.add(listener), only increment
totalSubscribers when it was not already present, and keep the existing logic in
the returned unsubscribe closure (which already decrements only when
current.delete(listener) succeeds) so add/removal are symmetric; reference the
bucket, listenersByNote, totalSubscribers, listener, and noteId symbols in the
listener registration/teardown closure.
In `@src/hooks/useNotePageEvents.test.ts`:
- Around line 178-187: The test currently reuses the same InfiniteData instance
`data` when calling `client.setQueryData` for two different keys returned by
`noteKeys.pagesWindow`, which can hide mutation bugs; create two distinct
InfiniteData objects (e.g., `dataPreviewThumb` and `dataPreview`) rather than
reusing `data`, each with its own `pages` array (copying `seedItems` into new
arrays) and pass those separate objects to `client.setQueryData` for the keys
using `NOTE_ID`, `USER_ID`, `USER_EMAIL`, the different view arrays
(["preview","thumbnail"] vs ["preview"]) and page sizes (50 vs 25) so mutations
to one cached entry won't affect the other.
In `@src/hooks/useNotePageEvents.ts`:
- Around line 129-145: The current page.updated handler uses replacePageInItems
to swap the item in-place; instead treat it as remove+prepend so the updated
page moves to the head of the cached windows to preserve server ordering. Update
the logic inside updateAllWindowsForNote: scan all pages to remove any existing
instance of event.page (use replacePageInItems or a new helper to delete
occurrences and flag changes), then insert event.page at the front of the first
window's pages[0].items (ensuring you don’t duplicate if it was already there),
adjust anyChanged accordingly, and trim or shift items across pages only as
needed to keep page sizes consistent; keep using queryClient and do not re-sort
by updated_at (honour Phase 3 invariant). Ensure functions referenced:
updateAllWindowsForNote and replacePageInItems are updated/used to implement
remove-then-prepend semantics for "page.updated".
- Around line 280-297: Detect the "note.permission_changed" event inside the
event callback passed to attachNoteEventListeners (the same place
applyNoteEventToCache is called); when you see that event, immediately close the
current EventSource by calling the existing detach() and then recreate the
stream by calling attachNoteEventListeners again (reassigning detach to the new
return) so the new EventSource will re-run authorization; keep the same ready
callback (which invalidates noteKeys.pagesWindowByNoteId) and the same event
handling (applyNoteEventToCache) when reattaching to avoid duplicating logic or
leaking listeners.
🪄 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: 55a1b701-d744-460c-a701-a72f5bdcac1a
📒 Files selected for processing (17)
server/api/src/__tests__/routes/notes/events.test.tsserver/api/src/__tests__/routes/notes/pages.test.tsserver/api/src/__tests__/services/noteEventBroadcaster.test.tsserver/api/src/routes/notes/crud.tsserver/api/src/routes/notes/eventHelpers.tsserver/api/src/routes/notes/events.tsserver/api/src/routes/notes/index.tsserver/api/src/routes/notes/members.tsserver/api/src/routes/notes/pages.tsserver/api/src/routes/pages.tsserver/api/src/services/noteEventBroadcaster.tssrc/components/page/PageGrid.test.tsxsrc/components/page/PageGrid.tsxsrc/hooks/useNotePageEvents.test.tssrc/hooks/useNotePageEvents.tssrc/lib/api/index.tssrc/lib/api/noteEvents.ts
- Server: subscribe to the note broadcaster before emitting `ready` so events published during the handshake aren't lost (Codex P2 / coderabbitai). After delivering `note.permission_changed`, eagerly unsubscribe and close the SSE stream so a revoked caller stops receiving subsequent events (Codex P1 / coderabbitai critical). - Broadcaster: drive subscriber accounting off the Set.add size delta so a duplicate subscribe of the same listener does not inflate `totalSubscribers` and falsely exhaust the cap (coderabbitai major). - pages.ts: refactor `applyPagesMetadataUpdate` to return the updated row from `.returning()` plus a `metadataChanged` flag, eliminating the follow-up SELECT and gating `page.updated` emits on actual value diffs so round-tripped values don't broadcast (gemini-code-assist medium / coderabbitai major). - Client hook: invalidate `pagesWindowByNoteId` on every `ready` (including the first one) so the T0→subscribe race after the initial list fetch is reconciled. Change `page.updated` cache handling to remove-then-prepend so the freshly bumped row matches the server's `updated_at DESC, id DESC` order (coderabbitai major). On `note.permission_changed`, close and reopen the `EventSource` immediately instead of waiting for the 30s retry, giving instant revocation (coderabbitai critical). - Tests: add regression coverage for the subscribe-before-ready invariant, the permission-change stream close, double-subscribe idempotency, remove+prepend on `page.updated`, distinct `InfiniteData` per cached window key, and the no-op metadata save skipping the pages UPDATE.
The `applyPagesMetadataUpdate` refactor in fe8ef33 introduced references to the `Page` type without importing it, which only surfaced as a `tsc --noEmit` failure (API Type Check job). Pull it in from `../schema/index.js`.
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 `@server/api/src/routes/notes/events.ts`:
- Around line 126-189: The subscriber slot can leak if an exception occurs
before stream.onAbort() or the trailing unsubscribe?.() run; wrap the
subscription lifecycle in a try/finally so unsubscribe is always called: ensure
the subscribeNoteEvents(noteId, ...) and the subsequent initial
writeSSE("ready") + heartbeat loop are inside a try block and perform
unsubscribe?.() in the finally (and set unsubscribe = null after calling it),
while keeping existing stream.close() calls where permission revocation requires
immediate close; this guarantees unsubscribe is invoked even if writeSSE or the
ready write throws.
In `@src/hooks/useNotePageEvents.ts`:
- Around line 247-255: The spread order in the SSE event reconstruction inside
wrap(name, (msg) => { ... }) is inverted: currently { type: name, ...parsed }
lets a parsed.type override the SSE event name; change it to { ...parsed, type:
name } so the wire event name (name) is authoritative when building the
NoteEvent passed to onEvent. Locate the block where tryParseJson(msg.data) is
parsed and the event is constructed (the NoteEvent creation) and swap the spread
order accordingly.
🪄 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: d269b598-2a49-400b-95c1-b8f65f752848
📒 Files selected for processing (8)
server/api/src/__tests__/routes/notes/events.test.tsserver/api/src/__tests__/routes/pages.test.tsserver/api/src/__tests__/services/noteEventBroadcaster.test.tsserver/api/src/routes/notes/events.tsserver/api/src/routes/pages.tsserver/api/src/services/noteEventBroadcaster.tssrc/hooks/useNotePageEvents.test.tssrc/hooks/useNotePageEvents.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/api/src/tests/services/noteEventBroadcaster.test.ts
- server/api/src/services/noteEventBroadcaster.ts
Two minor follow-ups from coderabbitai on PR #867: - `events.ts`: wrap the `ready` write + keep-alive loop in try/finally so the broadcaster subscription is always released, even if the initial `writeSSE` throws (e.g. client disconnects during handshake). Repeated failed handshakes would otherwise leak slots and eventually exhaust the 256-subscriber cap. - `useNotePageEvents.ts`: swap the spread order to `{ ...parsed, type: name }` so the SSE wire event name is authoritative and a malformed payload cannot override the discriminator the `addEventListener` wiring already validated.
This was referenced May 14, 2026
Merged
5 tasks
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.
Introduces
GET /api/notes/:noteId/events, an SSE feed that pushespage.added,page.updated,page.deleted, andnote.permission_changedevents to subscribed clients so theuseInfiniteNotePagescache patches in place instead of refetching everywindow after each mutation. Hocuspocus stays decoupled — it continues to
own page-body sync over WebSocket while this feed covers note-list
metadata only.
noteEventBroadcaster) partitioned by noteId,mirroring the
apiErrorBroadcasterpattern with a 256-subscriber cap;designed to be swapped for Redis Pub/Sub without changing call sites.
POST/DELETE /api/notes/:noteId/pages,POST /api/pages,DELETE /api/pages/:id,PUT /api/pages/:id/content(only when title or content_previewchanges),
PUT /api/notes/:noteId, and member CRUD endpoints.useNotePageEventsopens anEventSource, appliesevents to
noteKeys.pagesWindowByNoteIdviasetQueriesData, andre-
invalidateQuerieson reconnect to cover events missed during thegap. Wired into
PageGridso the connection is scoped to the noteview lifetime.
Summary by CodeRabbit
New Features
Tests