Notifications n plus one#2348
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes and JWT-awares wave-notification "mark as read" logic with identity caching and request queuing; adds a reusable leave/cleanup hook; merges connected-account and active-profile unread counts; replaces inline API/invalidation with marker hooks across drops, realtime, and MyStreamWaveChat; and expands tests across many notification scenarios. ChangesWave read marker, identity & integration
Sequence DiagramsequenceDiagram
actor User
participant MyStream as MyStreamWaveChat
participant Cleanup as useWaveChatLeaveCleanup
participant Marker as useMarkWaveNotificationsRead
participant Identity as useWaveReadIdentityState
participant Auth as getAuthJwt / jwtDecode
participant API as Server (POST /notifications/wave/{id}/read)
participant RQ as React Query
User->>MyStream: leave wave
MyStream->>Cleanup: register cleanup(enabled, waveId, callbacks)
Cleanup->>Cleanup: on cleanup -> setUnreadDividerSerialNo(null)
alt document.visibilityState == "visible"
Cleanup->>Marker: markWaveNotificationsRead(waveId)
Marker->>Identity: resolve identity keys & headers
Identity->>Auth: decode/verify JWT
Identity-->>Marker: identity keys + auth headers
Marker->>API: POST /notifications/wave/{waveId}/read (Authorization: Bearer <jwt>)
API-->>Marker: response
Marker->>RQ: invalidateNotifications()
RQ-->>Marker: queries invalidated
else document hidden
Cleanup-->>MyStream: skip network calls
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 0d9ebcac46
ℹ️ 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".
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
contexts/wave/hooks/useWaveRealtimeUpdater.ts (1)
305-321: 💤 Low valueConsider combining the two IIFEs for cleaner code.
Both async operations are independent and currently run in parallel, which is correct. However, they could be combined into a single IIFE for slightly cleaner code structure.
♻️ Optional: Combine into single IIFE
if (activeWaveId === waveId && document.visibilityState === "visible") { - void (async () => { - try { - await removeWaveDeliveredNotifications(waveId); - } catch (error) { - console.error( - "Failed to remove wave delivered notifications:", - error - ); - } - })(); - void (async () => { - try { - await markWaveNotificationsRead(waveId); - } catch (error) { - console.error("Failed to mark wave as read:", error); - } - })(); + void (async () => { + try { + await removeWaveDeliveredNotifications(waveId); + } catch (error) { + console.error("Failed to remove wave delivered notifications:", error); + } + try { + await markWaveNotificationsRead(waveId); + } catch (error) { + console.error("Failed to mark wave as read:", error); + } + })(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contexts/wave/hooks/useWaveRealtimeUpdater.ts` around lines 305 - 321, Two adjacent IIFEs call removeWaveDeliveredNotifications(waveId) and markWaveNotificationsRead(waveId) separately but can be combined for cleaner code while preserving parallel execution; replace both with a single IIFE that runs both calls in parallel (e.g., await Promise.all([removeWaveDeliveredNotifications(waveId), markWaveNotificationsRead(waveId)])) inside one try/catch and log errors with a clear message on failure so behavior and error handling remain the same.components/waves/drops/wave-drops-all/index.tsx (2)
222-251: 💤 Low valueThe type assertion is verbose but functional.
The expanded type cast handles the structural mismatch between
ApiWaveMinand the expected shape forisWaveDirectMessage. This works but is fragile if the API types change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/drops/wave-drops-all/index.tsx` around lines 222 - 251, The verbose inline type assertion around drop.wave is fragile; update the typing so handleQuoteClick can pass drop.wave directly to isWaveDirectMessage without that cast. Either change isWaveDirectMessage’s parameter type to accept the actual ApiWaveMin/ApiWave shape used by ApiDrop (or a union/partial that includes chat?.scope?.group?.is_direct_message) or import the correct ApiWave type and cast drop.wave to that single type (instead of the long ad-hoc object). Then remove the big assertion in handleQuoteClick and call isWaveDirectMessage(drop.wave.id, drop.wave) before building href with getWaveRoute.
208-220: ⚡ Quick winConsider logging pagination errors instead of silently swallowing them.
The
.catch(() => undefined)pattern completely silences all errors fromfetchNextPage. While this prevents unhandled rejections, it also hides legitimate errors (network failures, API errors) that could be useful for debugging.The
voidoperator usage (flagged by SonarCloud) is appropriate here for fire-and-forget async calls, but the error swallowing pattern deserves consideration.♻️ Proposed fix to log errors
void fetchNextPage( { waveId, type: DropSize.FULL, }, dropId - ).catch(() => undefined); + ).catch((error) => { + console.error("Failed to fetch next page:", error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/drops/wave-drops-all/index.tsx` around lines 208 - 220, The handleTopIntersection callback currently swallows all errors from the fire-and-forget fetchNextPage call; update it to catch and log errors instead of using .catch(() => undefined). Keep the void fetchNextPage(...) pattern for fire-and-forget but replace the empty catch with a handler that logs the caught error (using the project's logger if available, e.g., console.error or the app's logger) and include context (waveId, dropId, and DropSize.FULL) so failures in fetchNextPage can be diagnosed; references: handleTopIntersection, fetchNextPage, canFetchMoreDrops, waveId, dropId, DropSize.FULL.__tests__/components/brain/my-stream/MyStreamWaveChat.test.tsx (1)
22-30: 💤 Low valueConsider extracting the visibility state helper to a shared test utility.
This same helper pattern appears in multiple test files (
useWaveRealtimeUpdater.test.ts,MyStreamWaveChat.test.tsx). Extracting it to a shared test utility would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/brain/my-stream/MyStreamWaveChat.test.tsx` around lines 22 - 30, The duplicated visibility helper (documentVisibilityState and setDocumentVisibilityState) should be extracted into a shared test utility: create a new test helper module that exports the DocumentVisibilityState-typed documentVisibilityState variable and the setDocumentVisibilityState function, and then replace the inline definitions in tests like MyStreamWaveChat.test.tsx and useWaveRealtimeUpdater.test.ts with imports from that helper; ensure the helper uses Object.defineProperty(document, "visibilityState", ...) and export types/signatures so existing tests can call setDocumentVisibilityState(...) without other changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/auth/SeizeConnectContext.tsx`:
- Around line 1158-1195: The merge currently unconditionally replaces the active
account's unread count with activeUnreadNotifications?.unread_count ?? 0 which
wipes a valid JWT-backed value on first load or failed handle query; modify the
assignment in connectedAccountUnreadNotifications so it only overwrites the
JWT-backed count when the handle-scoped query has a real value (e.g.
activeUnreadNotifications is defined and unread_count is not null/undefined).
Use the existing jwtConnectedAccountUnreadNotifications as the fallback (keep
jwtPollingStoredConnectedAccounts/jwtConnectedAccountUnreadNotifications and
activeStoredAccount references) and assign
unreadNotificationsByAddress[activeAccountAddress] =
activeUnreadNotifications?.unread_count if that value exists, otherwise leave
the previous jwtConnectedAccountUnreadNotifications value intact.
In `@hooks/useMarkWaveNotificationsRead.helpers.ts`:
- Around line 62-68: The module-scoped maps inFlightWaveReadRequests,
pendingWaveReadRequests, clearedWaveReadIdentityKeysByAddress, and
latestVerifiedWaveReadIdentityByAddress can leak when auth/identity becomes
impossible or consumers unmount; add an explicit eviction and teardown path:
implement a function (e.g., clearPendingWaveReadsForAddress(address) and
clearAllWaveReadState()) that iterates pendingWaveReadRequests to reject each
pending promise with a clear error, delete entries from pendingWaveReadRequests
and inFlightWaveReadRequests, and clear the sets/maps
clearedWaveReadIdentityKeysByAddress and
latestVerifiedWaveReadIdentityByAddress; call the per-address clear on
identity-change/disconnect transitions and call clearAllWaveReadState in any
exported teardown/useEffect cleanup (or where useWaveChatLeaveCleanup awaits) so
queued promises cannot remain unresolved indefinitely.
In `@hooks/useMarkWaveNotificationsRead.identity.ts`:
- Around line 99-112: The decoded JWT in the walletAuth path (using jwtDecode
and WaveReadJwtPayload in useMarkWaveNotificationsRead.identity.ts) currently
ignores token expiry; update the identity extraction to check decodedJwt.exp
(expecting UNIX epoch seconds) against the current time and return undefined if
exp is missing or exp <= now so expired tokens are rejected before building
verifiedIdentity and attaching auth headers; ensure you lowercase decodedJwt.sub
as before and only proceed to build the addressKey/proxyCreatorId when the token
is not expired.
---
Nitpick comments:
In `@__tests__/components/brain/my-stream/MyStreamWaveChat.test.tsx`:
- Around line 22-30: The duplicated visibility helper (documentVisibilityState
and setDocumentVisibilityState) should be extracted into a shared test utility:
create a new test helper module that exports the DocumentVisibilityState-typed
documentVisibilityState variable and the setDocumentVisibilityState function,
and then replace the inline definitions in tests like MyStreamWaveChat.test.tsx
and useWaveRealtimeUpdater.test.ts with imports from that helper; ensure the
helper uses Object.defineProperty(document, "visibilityState", ...) and export
types/signatures so existing tests can call setDocumentVisibilityState(...)
without other changes.
In `@components/waves/drops/wave-drops-all/index.tsx`:
- Around line 222-251: The verbose inline type assertion around drop.wave is
fragile; update the typing so handleQuoteClick can pass drop.wave directly to
isWaveDirectMessage without that cast. Either change isWaveDirectMessage’s
parameter type to accept the actual ApiWaveMin/ApiWave shape used by ApiDrop (or
a union/partial that includes chat?.scope?.group?.is_direct_message) or import
the correct ApiWave type and cast drop.wave to that single type (instead of the
long ad-hoc object). Then remove the big assertion in handleQuoteClick and call
isWaveDirectMessage(drop.wave.id, drop.wave) before building href with
getWaveRoute.
- Around line 208-220: The handleTopIntersection callback currently swallows all
errors from the fire-and-forget fetchNextPage call; update it to catch and log
errors instead of using .catch(() => undefined). Keep the void
fetchNextPage(...) pattern for fire-and-forget but replace the empty catch with
a handler that logs the caught error (using the project's logger if available,
e.g., console.error or the app's logger) and include context (waveId, dropId,
and DropSize.FULL) so failures in fetchNextPage can be diagnosed; references:
handleTopIntersection, fetchNextPage, canFetchMoreDrops, waveId, dropId,
DropSize.FULL.
In `@contexts/wave/hooks/useWaveRealtimeUpdater.ts`:
- Around line 305-321: Two adjacent IIFEs call
removeWaveDeliveredNotifications(waveId) and markWaveNotificationsRead(waveId)
separately but can be combined for cleaner code while preserving parallel
execution; replace both with a single IIFE that runs both calls in parallel
(e.g., await Promise.all([removeWaveDeliveredNotifications(waveId),
markWaveNotificationsRead(waveId)])) inside one try/catch and log errors with a
clear message on failure so behavior and error handling remain the same.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3f7b00e-ff07-4d61-b172-2edc3254c9cb
📒 Files selected for processing (22)
__tests__/components/auth/SeizeConnectContext.addAccount.test.tsx__tests__/components/auth/SeizeConnectContext.switch-sync.test.tsx__tests__/components/auth/SeizeConnectContext.test.tsx__tests__/components/brain/my-stream/MyStreamWaveChat.test.tsx__tests__/components/brain/my-stream/useWaveChatLeaveCleanup.test.tsx__tests__/components/react-query-wrapper/ReactQueryWrapper.test.tsx__tests__/components/waves/drops/WaveDropsAll.test.tsx__tests__/components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.test.tsx__tests__/hooks/useConnectedAccountsUnreadNotifications.test.ts__tests__/hooks/useMarkWaveNotificationsRead.test.tsx__tests__/useWaveRealtimeUpdater.test.tscomponents/auth/SeizeConnectContext.tsxcomponents/brain/my-stream/MyStreamWaveChat.tsxcomponents/brain/my-stream/useWaveChatLeaveCleanup.tscomponents/react-query-wrapper/ReactQueryWrapper.tsxcomponents/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.tscomponents/waves/drops/wave-drops-all/index.tsxcontexts/wave/hooks/useWaveRealtimeUpdater.tshooks/useConnectedAccountsUnreadNotifications.tshooks/useMarkWaveNotificationsRead.helpers.tshooks/useMarkWaveNotificationsRead.identity.tshooks/useMarkWaveNotificationsRead.ts
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/waves/drops/wave-drops-all/index.tsx (1)
208-226: 💤 Low valueConsider removing the
voidoperator per SonarCloud guidance.The
voidoperator is flagged by static analysis. Since you're already swallowing errors with.catch(() => undefined), you can simply not assign the result to avoid the lint warning.♻️ Proposed fix
const handleTopIntersection = useCallback(() => { if (!canFetchMoreDrops) { return; } - void fetchNextPage( + fetchNextPage( { waveId, type: DropSize.FULL, }, dropId ).catch(() => undefined); }, [canFetchMoreDrops, fetchNextPage, waveId, dropId]);🤖 Prompt for 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. In `@components/waves/drops/wave-drops-all/index.tsx` around lines 208 - 226, The handleTopIntersection callback currently calls fetchNextPage with a leading void operator, which SonarCloud flags; remove the void operator and invoke fetchNextPage(...) directly (still appending .catch(() => undefined)) inside handleTopIntersection so the promise is handled without using void; update the closure to reference canFetchMoreDrops, fetchNextPage, waveId, and dropId as before and keep the early return when !canFetchMoreDrops.components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.ts (1)
211-218: 💤 Low valueConsider removing the
voidoperator per SonarCloud guidance.Same pattern as in
index.tsx- thevoidoperator is flagged by static analysis.♻️ Proposed fix
const syncReadStateWhenVisible = () => { if (document.visibilityState === "visible") { - void syncReadState({ + syncReadState({ waveId, identityKey, proxyRoleIdentityKey, }); } };🤖 Prompt for 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. In `@components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.ts` around lines 211 - 218, The use of the `void` operator before the promise-returning call in syncReadStateWhenVisible should be removed and the returned promise must be handled; update the syncReadStateWhenVisible implementation that calls syncReadState({ waveId, identityKey, proxyRoleIdentityKey }) to either make syncReadStateWhenVisible async and await the syncReadState call, or call syncReadState(...).catch(err => {/* log or handle error */}) so the promise is not ignored; refer to the syncReadStateWhenVisible function and the syncReadState call to locate and fix the code.
🤖 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.
Nitpick comments:
In `@components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.ts`:
- Around line 211-218: The use of the `void` operator before the
promise-returning call in syncReadStateWhenVisible should be removed and the
returned promise must be handled; update the syncReadStateWhenVisible
implementation that calls syncReadState({ waveId, identityKey,
proxyRoleIdentityKey }) to either make syncReadStateWhenVisible async and await
the syncReadState call, or call syncReadState(...).catch(err => {/* log or
handle error */}) so the promise is not ignored; refer to the
syncReadStateWhenVisible function and the syncReadState call to locate and fix
the code.
In `@components/waves/drops/wave-drops-all/index.tsx`:
- Around line 208-226: The handleTopIntersection callback currently calls
fetchNextPage with a leading void operator, which SonarCloud flags; remove the
void operator and invoke fetchNextPage(...) directly (still appending .catch(()
=> undefined)) inside handleTopIntersection so the promise is handled without
using void; update the closure to reference canFetchMoreDrops, fetchNextPage,
waveId, and dropId as before and keep the early return when !canFetchMoreDrops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 170b5fd0-6d9e-40be-a2fe-31b6ee17a15f
📒 Files selected for processing (10)
__tests__/components/brain/my-stream/MyStreamWaveChat.test.tsx__tests__/components/brain/my-stream/useWaveChatLeaveCleanup.test.tsx__tests__/components/waves/drops/WaveDropsAll.test.tsx__tests__/components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.test.tsx__tests__/hooks/useMarkWaveNotificationsRead.test.tsx__tests__/useWaveRealtimeUpdater.test.tscomponents/brain/my-stream/MyStreamWaveChat.tsxcomponents/brain/my-stream/useWaveChatLeaveCleanup.tscomponents/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.tscomponents/waves/drops/wave-drops-all/index.tsx
✅ Files skipped from review due to trivial changes (2)
- components/brain/my-stream/useWaveChatLeaveCleanup.ts
- tests/components/brain/my-stream/MyStreamWaveChat.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/components/brain/my-stream/useWaveChatLeaveCleanup.test.tsx
- components/brain/my-stream/MyStreamWaveChat.tsx
- tests/components/waves/drops/WaveDropsAll.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
__tests__/useWaveRealtimeUpdater.test.ts (2)
312-386: ⚡ Quick winAdd a positive test for replaying a queued read once JWT becomes available.
These new cases only prove the delayed read is dropped. If the retry path stops flushing queued reads when auth appears, this suite still passes. Please add one case where the wave stays active, the tab stays visible,
getAuthJwt()flips fromnullto"test-jwt", and the read POST is sent exactly once.Possible test shape
+ it("flushes a delayed active-wave read once JWT becomes available", async () => { + getAuthJwtMock.mockReturnValue(null); + + const store = { + wave1: { drops: [], latestFetchedSerialNo: 10 }, + }; + const props = baseProps(store); + props.activeWaveId = "wave1"; + const { result, rerender } = renderHook(() => + useWaveRealtimeUpdater(props) + ); + + await act(async () => + result.current.processIncomingDrop( + { id: "d-delayed-success", wave: { id: "wave1" }, author: {} } as any, + ProcessIncomingDropType.DROP_INSERT + ) + ); + await flushPromises(); + + expect(commonApiPostWithoutBodyAndResponse).not.toHaveBeenCalled(); + + getAuthJwtMock.mockReturnValue("test-jwt"); + rerender(); + await flushPromises(); + + expect(commonApiPostWithoutBodyAndResponse).toHaveBeenCalledTimes(1); + expect(commonApiPostWithoutBodyAndResponse).toHaveBeenCalledWith({ + endpoint: "notifications/wave/wave1/read", + headers: { Authorization: "Bearer test-jwt" }, + }); + });🤖 Prompt for 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. In `@__tests__/useWaveRealtimeUpdater.test.ts` around lines 312 - 386, Add a positive test to confirm a queued read is replayed when auth appears: create a test that uses useWaveRealtimeUpdater and calls processIncomingDrop with a DROP_INSERT when getAuthJwtMock initially returns null, keep props.activeWaveId unchanged (e.g., "wave1") and document visibility "visible" (use setDocumentVisibilityState if needed), then flip getAuthJwtMock to "test-jwt" and rerender; assert removeWaveDeliveredNotifications was called for "wave1" and that commonApiPostWithoutBodyAndResponse is called exactly once for the queued read (and not zero times) to verify the retry path in useWaveRealtimeUpdater replays the queued read. Ensure you reference processIncomingDrop, getAuthJwtMock, commonApiPostWithoutBodyAndResponse, removeWaveDeliveredNotifications, rerender and setDocumentVisibilityState in the test.
59-65: ⚡ Quick winDispatch
visibilitychangewhen you flipdocument.visibilityState.The hook subscribes to
visibilitychangeevents (line 429 ofuseWaveRealtimeUpdater.ts) and refreshes eligibility when the tab becomes visible. Redefining the getter alone only changes what a reread returns. Without dispatching the event, these tests cannot exercise the actual listener path—only the path triggered byrerender(). Update the helper to emit the event:const setDocumentVisibilityState = (state: DocumentVisibilityState) => { documentVisibilityState = state; Object.defineProperty(document, "visibilityState", { configurable: true, get: () => documentVisibilityState, }); document.dispatchEvent(new Event("visibilitychange")); };🤖 Prompt for 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. In `@__tests__/useWaveRealtimeUpdater.test.ts` around lines 59 - 65, The test helper setDocumentVisibilityState currently only redefines document.visibilityState but doesn't dispatch the "visibilitychange" event, so the hook's listener in useWaveRealtimeUpdater.ts (which listens for visibilitychange) isn't exercised; update setDocumentVisibilityState to dispatch document.dispatchEvent(new Event("visibilitychange")) after redefining the getter so the hook's visibilitychange handler runs during tests and triggers the real refresh/eligibility code path.
🤖 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 `@__tests__/hooks/useMarkWaveNotificationsRead.test.tsx`:
- Around line 128-146: The test fixture currently aliases
activeProfileProxyCreatorId to activeProfileProxyId via proxyCreatorId which
makes proxy id and creator id identical and hides regressions; change the setup
so proxyCreatorId is not defaulted to activeProfileProxyId (e.g., derive
proxyCreatorId directly from activeProfileProxyCreatorId or leave it
null/unique) and ensure the useAuthMock.mockReturnValue uses
activeProfileProxyCreatorId (or proxyCreatorId when explicitly provided) for
created_by.id instead of the aliased value—update any places that build
JwtPayload (sub/role) and the useSeizeConnectContextMock/ useAuthMock references
to reflect the distinct IDs.
- Around line 152-154: The beforeEach currently calls apiPostMock.mockReset()
which removes the previous async stub; after mockReset() re-stub apiPostMock to
return a resolved Promise by calling apiPostMock.mockResolvedValue(undefined) so
tests that rely on an async promise-returning API still get a promise
(preserving async/await sequencing); update the beforeEach where
jwtPayloadsByToken.clear() is called to add the mockResolvedValue restore for
apiPostMock.
In `@hooks/useMarkWaveNotificationsRead.cache.ts`:
- Around line 295-307: The cleanup currently in
useClearWaveReadStateOnLastUnmount uses useLayoutEffect and immediately calls
clearAllWaveReadState when mountedWaveNotificationsReadMarkerHookCount drops to
0, which triggers during React Strict Mode remounts; change this to defer the
actual clear by scheduling a setTimeout when the counter hits 0 and store the
timer id in a module-level variable, cancel that timeout if a new hook mounts
before it fires (clearTimeout) so the clearAllWaveReadState call is aborted on
remount, and ensure the timeout is cleared on real unmount after firing to avoid
leaks; update the logic inside the useLayoutEffect mount/unmount handlers around
mountedWaveNotificationsReadMarkerHookCount and clearAllWaveReadState to use
this deferred timer approach.
In `@hooks/useMarkWaveNotificationsRead.requests.ts`:
- Around line 497-545: The flush path groups and sends queuedRequests without
verifying each queuedRequest.addressEpoch against its current
latestAddressEpochRef.current, allowing stale reads from a prior session to be
replayed; before grouping/send (inside the loop over queuedRequests that
references pendingWaveReadRequests and getRequestKey), drop any queuedRequest
whose addressEpoch !== queuedRequest.latestAddressEpochRef.current (remove it
from pendingWaveReadRequests and do not add to queuedRequestsByRequestKey) so
only current-session requests are replayed; ensure the same stale-check is
applied for all places that call createWaveReadSendIntent/getRetryContext and
that you reject or resolve downstream promises appropriately when discarding
requests.
---
Nitpick comments:
In `@__tests__/useWaveRealtimeUpdater.test.ts`:
- Around line 312-386: Add a positive test to confirm a queued read is replayed
when auth appears: create a test that uses useWaveRealtimeUpdater and calls
processIncomingDrop with a DROP_INSERT when getAuthJwtMock initially returns
null, keep props.activeWaveId unchanged (e.g., "wave1") and document visibility
"visible" (use setDocumentVisibilityState if needed), then flip getAuthJwtMock
to "test-jwt" and rerender; assert removeWaveDeliveredNotifications was called
for "wave1" and that commonApiPostWithoutBodyAndResponse is called exactly once
for the queued read (and not zero times) to verify the retry path in
useWaveRealtimeUpdater replays the queued read. Ensure you reference
processIncomingDrop, getAuthJwtMock, commonApiPostWithoutBodyAndResponse,
removeWaveDeliveredNotifications, rerender and setDocumentVisibilityState in the
test.
- Around line 59-65: The test helper setDocumentVisibilityState currently only
redefines document.visibilityState but doesn't dispatch the "visibilitychange"
event, so the hook's listener in useWaveRealtimeUpdater.ts (which listens for
visibilitychange) isn't exercised; update setDocumentVisibilityState to dispatch
document.dispatchEvent(new Event("visibilitychange")) after redefining the
getter so the hook's visibilitychange handler runs during tests and triggers the
real refresh/eligibility code path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1d4622b-ca28-4f9c-b1ea-2909583bd7d2
📒 Files selected for processing (11)
__tests__/components/brain/my-stream/MyStreamWaveChat.test.tsx__tests__/components/waves/drops/WaveDropsAll.test.tsx__tests__/components/waves/drops/wave-drops-all/hooks/useWaveDropsNotificationRead.test.tsx__tests__/hooks/useMarkWaveNotificationsRead.test.tsx__tests__/useWaveRealtimeUpdater.test.tscomponents/auth/SeizeConnectContext.tsxhooks/useMarkWaveNotificationsRead.cache.tshooks/useMarkWaveNotificationsRead.helpers.tshooks/useMarkWaveNotificationsRead.identity.tshooks/useMarkWaveNotificationsRead.requests.tshooks/useMarkWaveNotificationsRead.types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/useMarkWaveNotificationsRead.helpers.ts
|
ragnep
approved these changes
May 7, 2026
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 by CodeRabbit
New Features
Bug Fixes