fix: channel message notification read status fixes#2482
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughRefactors frontend notification handling and navigation, strengthens websocket notification parsing/validation, adds a unified Zod notification schema, renames and reworks message-notification components, and enables conditional OpenAPI schema generation for notification-related Rust types. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
synoet
approved these changes
Apr 9, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/app-sidebar/channels-unread-widget.tsx`:
- Around line 132-149: The open actions are inconsistent: openInCurrentSplit and
the fullscreen/open-root path use layout.openWithSplit(content()) and thus open
the channel root instead of the specific unread notification; route these
through the same notification target as navigateToLatestNotification so clicks
always open the unread item. Update openInCurrentSplit and the
fullscreen/open-root handler to call navigateToLatestNotification(false) (or
call openNotification(latestNotification(), manager, false) via the
globalSplitManager) instead of layout.openWithSplit(content()), and ensure you
check latestNotification() exists before invoking openNotification so behavior
matches the new-split path.
In `@js/app/packages/channel/Channel/Channel.tsx`:
- Around line 164-170: onCleanup and useBeforeLeave both call markAsViewed()
causing duplicate non-idempotent requests; add a local guard boolean (e.g.,
hasMarkedViewed or didMarkViewed) scoped to the component and check it before
calling markAsViewed in both handlers, setting it to true immediately after the
first call so subsequent invocations (from the other lifecycle hook) are no-ops;
update the onCleanup and useBeforeLeave handlers to reference this guard and
only call markAsViewed when the guard is false.
In `@js/app/packages/notifications/components/MarkMessageNotifications.tsx`:
- Around line 35-40: The call to the async method notificationSource.markAsRead
is currently fire-and-forget in the subscribe callback
(notificationSource.subscribe -> callback using isMessageNotification and
unsubscribe) and at the other call site; either await the promise where the read
state must be confirmed (e.g., make the callback async and await
notificationSource.markAsRead) or, if you intend to keep it fire-and-forget,
append a .catch(...) to notificationSource.markAsRead to log errors (use your
app logger or console.error) so failures aren’t silently swallowed; update both
places that call markAsRead.
- Around line 43-45: The guard "if (existing && !existing.viewed_at) {
notificationSource.markAsRead(existing); }" is redundant because earlier code
returns when existing is falsy; remove the unnecessary truthy check and simplify
to check only viewed state, e.g., call notificationSource.markAsRead(existing)
when !existing.viewed_at, keeping the same variable name "existing" and method
"notificationSource.markAsRead" to locate the change.
In `@js/app/packages/notifications/notification-source.ts`:
- Around line 129-137: The mapWebsocketNotification function currently casts
raw.notification_metadata with "as NotifEvent", bypassing Zod validation; remove
that assertion and either (a) parse/validate notification_metadata with the
existing Zod schema inside mapWebsocketNotification (use schema.safeParse and
set notification_metadata to the parsed value on success or to a validated-safe
fallback on failure) or (b) leave notification_metadata typed as unknown/any on
the returned UnifiedNotification and let the existing safeParse call elsewhere
handle validation—reference mapWebsocketNotification, UnifiedNotification,
ConnGatewayInnerNotifValue, notification_metadata, NotifEvent, and the
schema.safeParse usage when making the change.
In `@js/app/packages/notifications/types.ts`:
- Around line 7-12: The code is accessing Zod internals (_def.left/_def.right)
to build unifiedNotificationSchema from
listTypedNotificationsResponse.shape.items.element; instead, stop using
_baseSchema/_entitySchema/_allOfSchema and rebuild unifiedNotificationSchema via
public Zod APIs: either import the specific exported schema parts from the
generator or operate on listTypedNotificationsResponse.shape.items.element with
public methods (e.g., .omit(), .pick(), .merge() or .extend()) to remove
owner_id and compose the entity, or define a local Zod object for the unified
notification shape and use that in place of the private-internal-based
construction.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0faa6e8d-7d35-4e3c-b52b-b508754a9cb4
⛔ Files ignored due to path filters (15)
js/app/packages/service-clients/service-notification/generated/client.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/connGatewayInnerNotifValue.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/connGatewayInnerNotifValueAllOf.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/connGatewayInnerNotifValueAllOfCreatedAt.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/connGatewayInnerNotifValueAllOfDeletedAt.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/connGatewayInnerNotifValueAllOfSenderId.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/connGatewayInnerNotifValueAllOfUpdatedAt.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/connGatewayInnerNotifValueAllOfViewedAt.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/connGatewayNotificationPayload.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/index.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/macroUserIdStr.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notificationTypeName.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/taggedContentValue.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/zod.tsis excluded by!**/generated/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (16)
js/app/packages/app/component/app-sidebar/channels-unread-widget.tsxjs/app/packages/block-channel/component/NewChannelBlockAdapter.tsxjs/app/packages/channel/Channel/Channel.tsxjs/app/packages/channel/Thread/ChannelThread.tsxjs/app/packages/channel/Thread/ThreadReplyList.tsxjs/app/packages/notifications/components/MarkMessageNotifications.tsxjs/app/packages/notifications/notification-source.tsjs/app/packages/notifications/types.tsjs/app/packages/service-clients/orval.config.tsjs/app/packages/service-clients/service-notification/openapi.jsonrust/cloud-storage/macro_user_id/Cargo.tomlrust/cloud-storage/macro_user_id/src/user_id.rsrust/cloud-storage/notification/Cargo.tomlrust/cloud-storage/notification/src/domain/models.rsrust/cloud-storage/notification/src/domain/models/queue_message.rsrust/cloud-storage/notification_service/src/api/swagger.rs
💤 Files with no reviewable changes (1)
- js/app/packages/block-channel/component/NewChannelBlockAdapter.tsx
js/app/packages/notifications/components/MarkMessageNotifications.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/app/component/app-sidebar/channels-unread-widget.tsx (1)
155-169:⚠️ Potential issue | 🟠 MajorKeep the anchor
hrefaligned with the unread target.Line 156 still points at the channel root. Primary click is intercepted, but browser-native paths like middle-click, cmd/ctrl-click, “open link in new tab”, and copied links will still open the root channel instead of the unread message/thread. Build the
hreffrom the same notification params so every entry point lands on the same target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/app-sidebar/channels-unread-widget.tsx` around lines 155 - 169, The anchor's href currently points to the channel root (using props.group.entityId) so non-primary clicks open the wrong location; change the href to the exact unread-target URL built from the same notification parameters used by navigateToLatestNotification (i.e., derive the target path the same way navigateToLatestNotification computes it) so middle-click/CMD-click/open-in-new-tab/copy-link all land on the unread message/thread; update the href generation in the component to use that same logic (or extract that logic into a helper like getLatestNotificationUrl and reuse it) while keeping the existing onClick handler (navigateToLatestNotification) and classList/isVisible behavior.
♻️ Duplicate comments (1)
js/app/packages/notifications/components/MarkMessageNotifications.tsx (1)
35-44:⚠️ Potential issue | 🟠 MajorWait for
markAsReadto settle before dropping the subscription.Line 37 and Line 44 both fire-and-forget the async read, and Line 38 unsubscribes immediately. If
markAsReadrejects once, this component loses its only retry path and the notification can remain unread until remount.🔧 Suggested change
if (!existing) { unsubscribe = notificationSource.subscribe((n) => { - if (!isMessageNotification(n)) return; - notificationSource.markAsRead(n); - unsubscribe(); + if (!isMessageNotification(n) || n.viewed_at) return; + void notificationSource + .markAsRead(n) + .then(() => unsubscribe()) + .catch((error) => { + console.error(error); + }); }); return; } if (!existing.viewed_at) { - notificationSource.markAsRead(existing); + void notificationSource.markAsRead(existing).catch((error) => { + console.error(error); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/notifications/components/MarkMessageNotifications.tsx` around lines 35 - 44, The component currently calls notificationSource.markAsRead(...) without awaiting it and immediately calls unsubscribe(), which can drop the only retry path on failure; update both places where markAsRead is invoked (the subscriber callback created via notificationSource.subscribe and the branch handling !existing.viewed_at) to await the Promise returned by notificationSource.markAsRead (use async/await), only call unsubscribe() after the await completes successfully, and catch errors (e.g., try/catch) so that on rejection you do not unsubscribe and you can log or surface the error instead of losing the subscription.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/app-sidebar/channels-unread-widget.tsx`:
- Around line 140-149: Remove the unreachable fullscreen navigation path: delete
the _openFullscreen function and any commented-out menu entry that referenced
it, and remove any unused imports or variables only used by that function (e.g.,
latestNotification, getChannelNotificationParams, globalSplitManager,
createPopoverSplit, props.group.entityId) so nothing dead remains; if the menu
entry is intended for a future PR keep a single TODO comment instead of the
broken code.
- Around line 119-129: latestNotification() assumes props.group.notifications[0]
is newest which relies on an undocumented ordering guarantee from
notificationSource.notifications()/user_notifications; to fix, ensure ordering
is explicit by either (a) sorting the array by created_at descending where
groups are assembled (e.g. inside groupByChannel()) before returning the group,
or (b) add a runtime assertion in latestNotification() that verifies
notifications are newest-first (compare first and last timestamps) and fall back
to picking the max created_at if not; reference functions/values:
latestNotification(), props.group.notifications, groupByChannel(),
optimisticInsertNotification(), and the user_notifications API so the chosen
change enforces or documents created_at-desc ordering to avoid opening stale
notifications.
---
Outside diff comments:
In `@js/app/packages/app/component/app-sidebar/channels-unread-widget.tsx`:
- Around line 155-169: The anchor's href currently points to the channel root
(using props.group.entityId) so non-primary clicks open the wrong location;
change the href to the exact unread-target URL built from the same notification
parameters used by navigateToLatestNotification (i.e., derive the target path
the same way navigateToLatestNotification computes it) so
middle-click/CMD-click/open-in-new-tab/copy-link all land on the unread
message/thread; update the href generation in the component to use that same
logic (or extract that logic into a helper like getLatestNotificationUrl and
reuse it) while keeping the existing onClick handler
(navigateToLatestNotification) and classList/isVisible behavior.
---
Duplicate comments:
In `@js/app/packages/notifications/components/MarkMessageNotifications.tsx`:
- Around line 35-44: The component currently calls
notificationSource.markAsRead(...) without awaiting it and immediately calls
unsubscribe(), which can drop the only retry path on failure; update both places
where markAsRead is invoked (the subscriber callback created via
notificationSource.subscribe and the branch handling !existing.viewed_at) to
await the Promise returned by notificationSource.markAsRead (use async/await),
only call unsubscribe() after the await completes successfully, and catch errors
(e.g., try/catch) so that on rejection you do not unsubscribe and you can log or
surface the error instead of losing the subscription.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 50ee802c-806a-4ebf-a80b-30e766ddd238
📒 Files selected for processing (3)
js/app/packages/app/component/app-sidebar/channels-unread-widget.tsxjs/app/packages/notifications/components/MarkMessageNotifications.tsxjs/app/packages/notifications/notification-navigation.ts
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.
a few issues here:
other changes: