fix(chat): stop recycled message rows from leaking state and stale heights#29401
Merged
Conversation
…ights These are list-agnostic row fixes, split out of the native LegendList work. Desktop already renders the thread with a recycling LegendList, so each of these is a live bug there today; on the native FlatList they are inert guards. - per-row state (sent animation, reaction picker, exploding retained height, ash tower) is now keyed to messageKey. A recycled container reuses the component instance for a different message, so mount-captured state leaked across rows: the picker stayed open on the wrong row, the sent-animation wrapper stuck on, and a measured retain height was applied to the wrong message (and could not self-correct, since retainHeight forces the style height so onLayout only ever reports the forced value back) - recycle-pool suffixes are limited to ones that are stable for the message's lifetime (:failed, :reply). The pool label is recorded when a container is allocated and never updated in place, so :pending (flips on every send confirmation) and :reactions (toggles) left stale labels behind and recycled containers painted at the wrong pooled height - getItemType splits headered rows into their own pool (:hdr). A message that leads its author group is ~40px taller than a grouped follow-on of the same render type, so a shared pool paints recycled views at the wrong height for a frame. It reads the same sticky username cache the rows render with, else a row that keeps its header after a scroll-back load is typed headerless and poisons the headerless pool's height average - useSyncRowLayout: when a row's content settles to a new height after first paint (flip result streams in, reactions appear, an unfurl loads), flush the row measure synchronously so the list's bottom re-pin uses the final height on the same frame instead of a frame late, which otherwise parks the thread above the newest message - SwipeableRow takes an `enabled` prop that conditionally spreads its pan handlers, so a list can shed per-row touch evaluation during a fast fling without unmounting the row. Toggling the Swipeable's subtree instead would remount its children and flash images. No caller passes it yet - desktop LegendList: dataKey replaces the key remount on conversation switch, and maintainScrollAtEnd opts back into footerLayout, which 3.x stopped implying once the trigger set is given explicitly
chrisnojima
force-pushed
the
nojima/HOTPOT-message-fixes
branch
from
a621b5c to
32a520c
Compare
dataKey is not a substitute for the key={listKey} remount that #29399 added.
A dataKey change sets shouldResetFreshDataLayout, which calls
resetInitialRenderState({resetLayout: true}): readyToRender drops to false and
the list then waits on a container layout event that never arrives, so the
thread renders blank forever. A remount escapes that because the reset is
guarded on !isFirst.
Desktop cannot use dataKey here at all, not just on clear: the layoutReady rAF
gate deliberately feeds an empty data array on every listKey change, and
previousDataLength === 0 is the other trigger for the same reset.
Repro: search a thread and jump to a hit (a centered load clears the thread
before refetching, bumping clearVersion). Fixed and confirmed on desktop.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple correctness issues caused by message-row recycling (LegendList) in chat threads, preventing per-row UI state and measured heights from leaking across different messages and improving bottom-anchoring behavior when rows grow after initial paint.
Changes:
- Key row-local state and retained measurements to
messageKeyto avoid recycled-row state/height leakage. - Restrict recycle-pool type suffixes to lifetime-stable attributes and split headered rows into a separate recycling pool.
- Add
useSyncRowLayoutto synchronously flush row measurement when height-affecting content changes post-paint, plus groundwork for disabling swipe handlers during fast scroll.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/common-adapters/swipeable-row.shared.tsx | Adds an enabled prop to control whether pan handlers are attached. |
| shared/common-adapters/swipeable-row.native.tsx | Conditionally spreads pan handlers based on enabled. |
| shared/chat/conversation/messages/wrapper/wrapper.tsx | Resets per-row state on messageKey changes and keys <Sent> per message to avoid stale animation instances. |
| shared/chat/conversation/messages/wrapper/sent.native.tsx | Documents the requirement to key the animated container per message. |
| shared/chat/conversation/messages/wrapper/exploding-height-retainer/index.tsx | Keys retained height and ash-tower animation state per message to prevent recycled-height leakage. |
| shared/chat/conversation/messages/use-sync-row-layout.tsx | Introduces a hook to synchronously flush LegendList row measurement when a per-message signature changes. |
| shared/chat/conversation/messages/text/unfurl/unfurl-list/image/index.tsx | Flushes row measurement when unfurl image dimensions change. |
| shared/chat/conversation/messages/text/coinflip/index.tsx | Flushes row measurement when coinflip status changes alter card height. |
| shared/chat/conversation/messages/row-metadata.tsx | Limits recycle-type suffixes to stable lifetime properties (:failed, :reply). |
| shared/chat/conversation/messages/row-metadata.test.ts | Updates tests to reflect stable-suffix row typing behavior. |
| shared/chat/conversation/list-area/index.tsx | Splits headered vs grouped rows into separate pools (:hdr) and re-enables footerLayout in maintain-scroll triggers. |
Comment on lines
+935
to
+946
| const [rowState, setRowState] = React.useState(() => ({ | ||
| animateSent: isMobile && mdata.youSent, | ||
| key: messageKey, | ||
| showingPicker: false, | ||
| })) | ||
| if (rowState.key !== messageKey) { | ||
| setRowState({animateSent: isMobile && mdata.youSent, key: messageKey, showingPicker: false}) | ||
| } | ||
| const {animateSent, showingPicker} = rowState | ||
| const setShowingPicker = React.useCallback((s: boolean) => { | ||
| setRowState(prev => (prev.showingPicker === s ? prev : {...prev, showingPicker: s})) | ||
| }, []) |
Comment on lines
+36
to
+40
| const [heightState, setHeightState] = React.useState(() => ({height: 17, key: messageKey})) | ||
| if (heightState.key !== messageKey) { | ||
| setHeightState({height: 17, key: messageKey}) | ||
| } | ||
| const height = heightState.height |
Comment on lines
70
to
74
| const setBoxRef = React.useCallback((ref: Kb.MeasureRef | null) => { | ||
| const measuredHeight = ref?.getBoundingClientRect().height | ||
| if (measuredHeight) { | ||
| setHeight(lastHeight => (lastHeight === measuredHeight ? lastHeight : measuredHeight)) | ||
| setHeightState(prev => (prev.height === measuredHeight ? prev : {...prev, height: measuredHeight})) | ||
| } |
Comment on lines
+175
to
+179
| const [heightState, setHeightState] = React.useState(() => ({height: 20, key: messageKey})) | ||
| if (heightState.key !== messageKey) { | ||
| setHeightState({height: 20, key: messageKey}) | ||
| } | ||
| const height = heightState.height |
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
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.
Split out of the native LegendList experiment (#29361, stacked on top of this branch) so the list-agnostic row fixes can land on their own.
Desktop already renders the thread with a recycling
LegendList, so each of these is a live bug on master today. On the nativeFlatListthey are inert guards.What's here
Per-row state keyed to
messageKey. A recycled container reuses the component instance for a different message, so mount-captured state leaked across rows: the reaction picker stayed open on the wrong row, the sent-animation wrapper stuck on, and a measured retain height was applied to the wrong message (and could not self-correct, sinceretainHeightforces the style height soonLayoutonly ever reports the forced value back). Coverswrapper.tsxrow state, the exploding-height retainer on both platforms, the ash tower, and<Sent key={messageKey}>.Recycle-pool suffixes limited to lifetime-stable ones (
:failed,:reply). The pool label is recorded when a container is allocated and never updated in place, so:pending(flips on every send confirmation) and:reactions(toggles) left stale labels behind and recycled containers painted at the wrong pooled height. Tests updated.getItemTypesplits headered rows into their own pool (:hdr). A message that leads its author group is ~40px taller than a grouped follow-on of the same render type, so a shared pool paints recycled views at the wrong height for a frame. It reads the same sticky username cache the rows render with — otherwise a row that keeps its header after a scroll-back load is typed headerless and poisons the headerless pool's height average.useSyncRowLayout. When a row's content settles to a new height after first paint (a flip result streams in, reactions appear, an unfurl loads), flush the row measure synchronously so the list's bottom re-pin uses the final height on the same frame instead of a frame late, which otherwise parks the thread above the newest message. Noops wherever the row isn't inside a LegendList container.SwipeableRowgains anenabledprop that conditionally spreads its pan handlers, letting a list shed per-row touch evaluation during a fast fling without unmounting the row — toggling the Swipeable's subtree instead would remount its children and flash images. No caller passes it yet; the consumer lands in WIP: try legends on native again #29361.Desktop
LegendList:maintainScrollAtEndopts back intofooterLayout, which 3.x stopped implying once the trigger set is given explicitly. Thekey={listKey}remount from fix(chat): desktop thread search jumps to the right message #29399 stays as-is — see the second commit for whydataKeycannot replace it.Verification
yarn tscandyarn lintclean;row-metadata.test.tspasses (4 tests). Desktop thread search / jump-to-hit confirmed working in the app (an earlier revision of this branch regressed it into the blank-thread deadlock; fixed in the second commit).