refactor!: align Thread with Channel - instance prop, provider, children - #3289
Merged
Merged
Conversation
…package root `useSendMessageFn` and `useUpdateMessageFn` imported `useChannel` and friends from `'../../..'` -- the package's own public barrel -- which closes a cycle: root barrel -> components -> these hooks -> root barrel. Any module graph that reaches the hooks before the barrel has finished evaluating gets `undefined` for those bindings, which surfaces as `TypeError: useChannel is not a function` at render time rather than as an import error. It is reproducible from a test that mounts a composer without pulling the root barrel first, which is what turned this up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e subtree `Thread` keyed `ThreadInner` on the thread, so switching thread destroyed everything below it -- header, composer, dropzone, audio provider -- to reset the one thing that actually holds per-thread state. That is the pattern `Channel` shed in #3286, and the same reasoning applies one level down. The reset moves into the message list. `getMessageSourceKey({ channel, thread })` keys on the thread when there is one and the channel otherwise, deliberately mirroring `useMessagePaginator()` -- `thread?.messagePaginator ?? channel.messagePaginator` -- so the list resets exactly when the paginator it reads from changes. Keying on the channel alone could not express this: two threads of one channel share a channel, so a thread switch reset nothing. `channelInstanceKey.ts` becomes `MessageList/messageSourceKey.ts`. The WeakMap and counter are unchanged; it accepts either instance now, and it is a message-list concern rather than a channel one. An audit of what else relied on the remount found nothing that breaks: `Thread` and `ThreadHeader` hold no local state, only store subscriptions that re-subscribe on the thread; `ThreadHead` is already self-keyed on the parent message; the composer swaps via `useMessageComposerController`, whose draft-saving cleanup keys on the composer rather than on mount; `WithAudioPlayback` takes `playbackScope`. Autofocus survives too, though incidentally -- the focus effect depends on `attachments`, and each composer carries its own array. That is thin enough to pin with a test, since making empty attachments a shared constant would break focus with nothing else to catch it. With the key gone, `ThreadInner` had no reason to exist: its `repliesEnabled` gate folds into the null return beside `!threadInstance || !parentMessage`. BREAKING CHANGE: `Thread` no longer rebuilds its subtree when the thread changes. A custom component rendered inside `Thread` that relied on being remounted per thread -- to reset local state, or to re-run a mount effect -- must depend on the thread instead. The SDK's own message list does this by keying on the thread; `WithAudioPlayback` by taking `playbackScope`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Opening the threads view lit up every row at once. The highlight was derived by diffing the manager's threads array by reference, and the first page landing is a transition from `[]` to N threads -- so all of them counted as new. Scrolling did the same thing to each page it loaded. Neither is an arrival. `ThreadManagerState.unseenThreadIds` already carries the right signal: the manager appends to it only when a thread the list does not hold receives a message, which is exactly the case the flash is for. Reading it at the moment the list changes is not enough, though -- `reload()` clears `unseenThreadIds` in the same update that puts those threads into `threads`, so by the time they are on screen the manager no longer calls them unseen. The ids are remembered as they are reported and consumed when the threads they name turn up. Also fixes two things the rewrite made visible: the effect had no dependency array and re-subscribed on every render, and the highlight map was replaced rather than merged, so a second arrival cut short the flash of the first. Extracted to its own module so it can be tested directly; ThreadList.test.tsx mocks `useStateStore` and Virtuoso wholesale, which leaves no room to exercise this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d-instance-axis # Conflicts: # src/components/Thread/Thread.tsx
The two message lists took `disableDateSeparator` while `Thread` took
`enableDateSeparator` for the same setting, so the value had to be negated on
the way down and read as a double negative inside -- `useEnrichedMessages`
computed `const enableDateSeparator = !disableDateSeparator` before calling
`processMessages`, which already spoke in positive terms.
Both lists now take `withDateSeparator`. The defaults are unchanged in
behavior, only in spelling.
BREAKING CHANGE: `disableDateSeparator` is renamed to `withDateSeparator` on
`MessageList` (default `true`, was `disableDateSeparator={false}`) and
`VirtualizedMessageList` (default `false`, was `disableDateSeparator={true}`).
The rename also reaches `processMessages`, `useEnrichedMessages`,
`FloatingDateSeparator`, `useFloatingDateSeparator` and
`useFloatingDateSeparatorMessageList`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e Channel
`Thread` conflated three jobs: it owned the thread's lifecycle, it composed a
fixed header/list/composer, and it read its subject from a `ThreadProvider`
someone else had to render. Customizing any part of the composition meant
another pass-through prop -- there were five, and each new knob needed a
sixth.
It now matches `Channel` on both halves of the pattern: the instance comes in
as a required prop, the provider goes out to the subtree, and what renders
inside is the caller's to write.
<Thread thread={thread}>
<ThreadHeader />
<MessageList withDateSeparator={false} />
<MessageComposer focus />
</Thread>
The parent message stays with the message list, which resolves it from thread
context: it has to sit inside the scroll container, so a caller could not
place it as a sibling. Everything the caller can express directly -- date
separators, composer autofocus, virtualized or not -- now has to be said.
BREAKING CHANGE: `Thread` requires a `thread` prop and renders `children`.
`additionalMessageComposerProps`, `additionalMessageListProps`,
`additionalParentMessageProps`, `additionalVirtualizedMessageListProps`,
`autoFocus` and `virtualized` are removed -- pass those props to the children
you render. `Thread` provides the thread itself, so it no longer needs to be
wrapped in `ThreadProvider`. The `str-chat__thread--virtualized` class is
gone; `Thread` cannot know which list you chose.
BREAKING CHANGE: `Thread.enableDateSeparator` is removed with nothing in its
place. A `MessageList` in a thread now follows its own default and shows date
separators unless you pass `withDateSeparator={false}`.
BREAKING CHANGE: the `head` prop is removed from `MessageList` and
`VirtualizedMessageList`. Both render the thread's parent message from
context; override it with a `ThreadHead` component on `ComponentContext`.
BREAKING CHANGE: `ThreadHeaderProps` is down to `overrideTitle`. `thread`
comes from the thread in context, and `closeThread` goes through workspace
navigation -- customize it centrally via `ChatView`'s
`deriveWorkspaceNavigation`, or call the new `useCloseThread()` hook from a
header of your own. `ComponentContext.ThreadHeader` is removed; compose the
header you want.
BREAKING CHANGE: `ThreadSlot` wraps its children in `<Thread>` instead of
letting them replace the whole panel, mirroring `ChannelSlot`. Markup that
must sit outside the thread container moves outside `ThreadSlot`; anything
needing thread context must sit inside.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The example apps were the first consumers of the new `<Thread thread={...}>`,
and moving them surfaced two things worth recording.
The drag-and-drop wrapper has to sit inside `<Thread>` now, because its
`useChannel()` resolves the thread's own channel from thread context. That
reparenting put a `display: block` element between `.str-chat__thread` and
its header/list/composer, so the thread's flex column stopped reaching them
and the content overflowed its box. `Channel.scss` already had the rule for
exactly this case; `Thread.scss` now mirrors it.
The tutorial apps had been rendering `<Thread />` with no `ThreadProvider`
anywhere, so `useThreadContext()` was undefined and the panel rendered
nothing -- every tutorial's CSS reserves `.str-chat__thread { width: 45% }`
for a panel that never appeared. Steps 2 and 5 had no `ChatView` either,
which is why their threads could not open: "reply in thread" goes through
workspace navigation, and without a layout host that is a no-op. All five
now host a thread slot and render the panel through `ThreadSlot`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lections Three of `Thread`'s four store subscriptions selected more than the component uses, so unrelated activity re-rendered the panel and re-ran its effects: - the `ThreadManager` selector took `threads`, whose identity changes on every mutation of the list -- a reload, a new thread, a reorder -- when all the component asks is whether this thread is in it; - the paginator selector took `items`, which changes on every incoming reply, when both effects only test it for `undefined`; - the thread selector took `replyCount`, likewise only ever read as `> 0`. Each now selects the boolean the code actually branches on, so the panel re-renders when those answers change rather than when their inputs do. The effect conditions read as what they mean (`if (hasLoadedReplies || isLoading)`) instead of reaching back through optional chains. Also drops defensive code left over from when `thread` was resolved from context and could be undefined: `useStateStore` returns undefined only for an undefined store, and `Thread.state`, `Thread.messagePaginator` and `Channel.configState` are all non-optional. The `??` fallback on the manager subscription was the misleading one -- it implied the subscription could miss a first value, which `useSyncExternalStore` does not. The two reload effects stay separate on purpose, and say so: the stale catch-up depends on `isStateStale` alone, so it fires once per staleness episode. Merged into the load effect it would also re-run whenever that one's inputs change -- registering the thread flips `isThreadManaged`, which requests a second reload while the first is still in flight. 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: +3.89 kB (+0.47%) Total Size: 827 kB 📦 View Changed
ℹ️ View Unchanged
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-v15 #3289 +/- ##
==============================================
Coverage ? 84.98%
==============================================
Files ? 524
Lines ? 15287
Branches ? 4861
==============================================
Hits ? 12991
Misses ? 2296
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch was successfully deployed
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
Threadwas the last major component still shaped by the pre-v15 architecture. It resolved its subject from aThreadProvidersomeone else had to render, composed a fixed header/list/composer, and exposed customization only through pass-through props — five of them, with each new knob needing a sixth.This aligns it with
Channel: the instance comes in as a required prop, the provider goes out to the subtree, and the composition is the caller's.🛠 Implementation details
Threadtakes the instance and renders childrenIt keeps only what
Channelkeeps: loading the thread, registering it with theThreadManager, scoping audio playback, and the container element.ThreadSlotnow mirrorsChannelSlot— it resolves the slot's thread, hands it to<Thread>, and wraps its children rather than being replaced by them.The parent message moved to the message list. It has to sit inside the scroll container, so a caller cannot place it as a sibling — both lists resolve it from thread context via
useThreadHead, and theheadprop is gone. This is the one thing left implicit; everything a caller can express directly (date separators, autofocus, virtualized or not) now has to be said.ThreadHeaderstands alone.closeThreadandthreadare removed: the parent message comes from context, and closing goes through workspace navigation, which an app customizes once viaChatView'sderiveWorkspaceNavigation. NewuseCloseThread()gives a custom header the same behavior.disableDateSeparator→withDateSeparatoron both lists, removing the negation thatThreadhad to apply on the way down and the double negative insideuseEnrichedMessages.Earlier on the branch:
ThreadInner's key is gone (the thread-switch reset lives in the message list, which is what actually holds per-thread state),ThreadListflashes only threads that actually arrived rather than every item on load,LegacyThreadContextis removed, andMessageComposerimports from defining modules instead of the package root, which was leaving bindings undefined through an ESM cycle.Finally,
Thread's store subscriptions were narrowed to the booleans it branches on, so the panel no longer re-renders on unrelated thread-list activity or on every incoming reply.🎨 UI Changes
No intended visual change, with two exceptions:
Threadused to suppress the in-list separators for whichever list it rendered. With noThreadprop to do that, aMessageListin a thread follows its own default and shows them — passwithDateSeparator={false}for the v14 look. The parent message still carries a separator of its own either way.str-chat__thread--virtualizedis no longer set on the container;Threadcannot know which list you chose.One layout fix went in with the reparenting: a drag-and-drop wrapper now sits inside
<Thread>(itsuseChannel()resolves the thread's own channel from thread context), and adisplay: blockelement between.str-chat__threadand its children broke the flex column.Thread.scssmirrors the ruleChannel.scssalready had for the same case.💥 Breaking changes
Threadrequires athreadprop and renderschildren.additionalMessageComposerProps,additionalMessageListProps,additionalParentMessageProps,additionalVirtualizedMessageListProps,autoFocus,virtualizedandenableDateSeparatorare removed. It provides the thread itself — no surroundingThreadProvider.disableDateSeparator→withDateSeparatoronMessageList(defaulttrue) andVirtualizedMessageList(defaultfalse); also onprocessMessages,useEnrichedMessages,FloatingDateSeparatorand both floating-separator hooks.headremoved from both message lists — override viaComponentContext.ThreadHead.ThreadHeaderPropsis down tooverrideTitle;ComponentContext.ThreadHeaderremoved.ThreadSlotwraps its children instead of letting them replace the panel.LegacyThreadContextremoved.