fix: render stable notification host for Channel and Thread - #3291
Merged
Merged
Conversation
…ce key Raise a notification, switch channel, and the toast restarted its countdown and replayed its entry animation -- so it read as a notification about the channel just opened, and could follow the user indefinitely. `MessageList` keys its inner component on the message source, which rebuilds the subtree on a channel or thread switch. That is deliberate: it is what stops the previous source's scroll position carrying over. The notification area was rendered inside that subtree and got reset with it. `NotificationList` tracks the ids it has already started a countdown for in a ref, so a fresh instance asked `NotificationManager` for a new timeout and lost the original deadline; the entry animation restarted because a remounted element is a newly inserted one. A notification reports something the user just did. It does not belong to the message source, so it does not belong inside a boundary that resets when the source changes. `MessageListMainPanel` and `NotificationList` now render in the outer component, beside the keyed element rather than within it, so a switch replaces only the list. The notification keeps its component instance, its timer bookkeeping and its DOM node, and nothing has to move to preserve them. For `VirtualizedMessageList` this also puts the panel above `if (!processedMessages) return null`, which used to take the notification area down with the list. The rendered DOM is unchanged: the panel holds the same children in the same order, with the notification list last. Covered by a new `MessageList notification area` block in `MessageList.test.tsx`: the element is the same across a switch, it expires on its original schedule however many channels are opened, and it is still displayed for a notification raised afterwards. `channelSwitchReset` and `threadSwitchReset` used the panel element as their witness that the list had been rebuilt. The panel is now outside the key by design, so they assert on `.str-chat__message-list` instead -- inside the key, and the element that actually carries the scroll state the key exists to reset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An attachment rejected by the size limit while typing in a thread surfaced in the channel, not the thread. Notifications are routed to a panel by a `target:` tag or `origin.context.panel`, both of which the React SDK attaches when it publishes through `useNotificationApi`. `stream-chat` publishes these itself, so they carried no target at all, and `isNotificationForPanel` resolves an untargeted notification to `fallbackPanel ?? 'channel'`. The missing information only exists at the emit site: `AttachmentManager` belongs to a `MessageComposer`, and that composer is what says whether the upload was started in a thread or a channel. `stream-chat` now carries it in `origin.context` (as several composer-scoped emitters already did), so the panel is derived from its `contextType`. A panel remains the SDK's vocabulary -- the client names the composer, not the surface. Behaviour change worth noting: a composer-raised notification is no longer untargeted, so a `NotificationList` configured with a `fallbackPanel` no longer claims it. That is the point -- it belongs to one panel now -- but it is visible to anyone relying on the fallback to collect these. Inert until the `stream-chat` change ships: without the composer in the context the notification stays untargeted and falls back to the channel panel exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Size Change: -774 B (-0.09%) Total Size: 826 kB 📦 View Changed
ℹ️ View Unchanged
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-v15 #3291 +/- ##
==============================================
Coverage ? 85.12%
==============================================
Files ? 524
Lines ? 15288
Branches ? 4861
==============================================
Hits ? 13014
Misses ? 2274
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`NotificationList` kept a ref of the notification ids it had already started a countdown for, because `startTimeout` always restarts: call it twice and the notification silently gets another full lifetime. That record was load-bearing and lived in the wrong place -- a component that remounts starts with an empty one, calls again, and the notification outlives its duration while still looking like it is counting down. It is what made a channel switch able to keep a toast on screen indefinitely. `stream-chat` now offers `ensureTimeout`, which starts a countdown only when none is running, so the guarantee belongs to the thing that owns the deadline. `useNotificationApi` exposes it as `ensureNotificationTimeout` beside the existing `startNotificationTimeout`, which is unchanged and still restarts for callers that mean to. With that, the ref goes, along with the cleanup in `dismiss`, the effect pruning stale entries on every store change, and the delete in the exit handler. The list asks once per intersection and lets the manager decide. The remount that made this matter is already fixed -- the notification area no longer sits inside the message list's key -- so this removes the fallback rather than the bug. Requires a `stream-chat` release carrying `ensureTimeout`; the peer and dev ranges need bumping to it before this can build against the published package. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
🎯 Goal
Fixes #3279: upload an oversized file in channel A, switch to channel B, and the same toast re-mounts in B. Two independent faults cause it.
🛠 Implementation details
1. The notification area sat inside the message list's reset boundary
Both lists key their inner component on the message source, which is correct — it resets the scroll state the list owns.
NotificationListwas rendered inside that subtree, so a switch rebuilt it too:startNotificationTimeoutagain andNotificationManagergrants a fresh full duration;A notification reports something the user just did, so it doesn't belong inside a boundary keyed on the message source.
MessageListis already two components; the panel and the notification area moved to the outer one, above the key:React rebuilds only what's inside the keyed element, so the notification keeps its component instance (timer bookkeeping intact) and its DOM node (no animation restart). Nothing has to move to preserve them. For
VirtualizedMessageListthis also lifts the panel aboveif (!processedMessages) return null, which used to take the notification area with it.2. An upload failure raised in a thread surfaced in the channel
target:tags are attached by the React SDK when publishing viauseNotificationApi.stream-chatpublishes upload notifications itself, so they carried no target, andisNotificationForPanelresolves untargeted ones tofallbackPanel ?? 'channel'.Only the emit site knows:
AttachmentManagerbelongs to aMessageComposer, which knows whether the upload started in a thread.stream-chatnow carries that composer inorigin.context(GetStream/stream-chat-js#1871, merged), andgetNotificationTargetPanelderives the panel from itscontextType. Panels stay the SDK's vocabulary — the client names the composer, not the surface.fallbackPanelis how aNotificationListclaims notifications that carry no target. Uploadnotifications used to have none, so a list could be configured to collect them that way. They are
now targeted by the composer that raised them, which means
fallbackPanelis no longer consultedfor them — they go to the panel the upload started in. Intended, but it silently overrides any
existing
fallbackPanelconfiguration aimed at these.Part 2 is inert until GetStream/stream-chat-js#1871 ships; until then these stay untargeted and fall back to
channelas before. Either order merges safely.Depends on GetStream/stream-chat-js#1872 too.