fix(chat): land on the message you jumped to - #29559
Open
chrisnojima wants to merge 5 commits into
Open
Conversation
Two library fixes the app cannot get at from outside, generated from fix/jump-to-item-scroll-targeting off v3.3.7. scrollTargetSettle: a scrollToIndex/scrollToItem with a viewPosition is aimed at positions computed from estimatedItemSize, so it lands wherever the estimates above the target were wrong and never comes back. The list now holds the index + viewPosition target across the measurement settle and re-aims as rows report their real heights, which is what makes a search hit or a permalink land on the message instead of near it. headerLayout trigger for maintainScrollAtEnd: a header that lays out taller than the size it was rendered with pushes every item down by the difference, so a list that had already reached its end is short of it by that much with nothing to bring it back. Measured opening a one-to-one on desktop: the at-end initial scroll dispatched to 449 against 1228px of content, setHeaderSize reported 100 -> 152 in the same tick, and the thread sat 52px above its newest message permanently, on roughly half of opens. With the trigger, 30 of 30 opens landed on the newest message. Both are load-bearing: removing either makes one of the desktop flows added here fail.
The desktop thread centred a search hit, a reply-quote jump or a permalink with a
closed loop of its own: measure the target row's offset from the viewport centre, ask
for a corrected scroll offset, repeat for up to 3s, and abort on a wheel event. It was
compensating for the library aiming at positions built from estimatedItemSize, and it
compensated badly — it fought maintainVisibleContentPosition for the same scroll
offset, it re-centred a reader who had scrolled away while the loop was still running,
and near either end of a thread it spun against a clamp.
With scrollTargetSettle in the legend-list patch the list holds the target across the
measurement settle itself, so the app's job shrinks to issuing one scroll per target
and getting out of the way. useScrollToCentered does exactly that: wait for the target
to be present in messageOrdinals (a centred load clears the thread first, so it is
briefly absent), scroll once, and reset per dataset rather than per conversation.
The scroll is unconditional rather than deferring to the list's own initialScrollIndex
bootstrap. A permalink mounts the thread with no centred target, so the list is built
with initialScrollAtEnd and the bootstrap it re-arms when the centred dataset lands
does not move it — the thread settles at its end and the target is never shown. That
path is indistinguishable here from a warm in-thread jump, so this has to be the one
authority that always fires.
maintainVisibleContentPosition also becomes the documented boolean. A partial config
object opts out of whatever it does not name, and `data` defaults to false, so
{data: true} was quietly narrowing it the way maintainScrollAtEnd's {on: {...}} list
once did.
Native is untouched: it is still the FlatList thread master ships.
Three flows, all of which were manual checks: chat-search-hit opens a named conversation (the inbox is ordered by recency, so "the first row" is a different thread from one run to the next), searches it, and steps through hits with Enter — the wrap-around at the end is the jump that used to land off screen. Then it scrolls away and watches: a thread the reader has moved must not pull itself back once the rows scrolled past finish measuring. chat-thread-bottom opens conversations and measures how far each settled from its newest message. Image responses are delayed so the row growth lands after the initial scroll rather than before it, which is what makes this able to fail rather than passing on whatever the disk cache had warm. MAX_DISTANCE_FROM_END is 8px and retries are off on purpose: the growth is timing dependent, and a retry that passes hides the regression the flow exists for. chat-link-jump covers the cold path the other two do not — a thread mounting with a centred target already pending, the way a permalink arrives. It reaches it through the app's own affordances (copy a link to a message in B, paste and send it in A, click it), reuses a link an earlier run left behind rather than writing new data into the smoke user's real history each time, and checks the target both landed on screen and near the middle. The testIDs they need are added alongside: the desktop header search button, the centre-highlighted row, the per-message "..." menu, the big-team channel inbox row, and the header above the oldest loaded message.
The reuse path never engaged: LINK_PATTERN was anchored without the m flag but matched against the [data-ordinal] wrapper's innerText, which also carries the separator and an author header, and the scroll-back wheeled the inbox because the pointer was never moved into the message list. Every run therefore sent a fresh link into the smoke account's real chat history - twice per full suite run, since two projects match flows/**. Also log the number of leftover link messages so a run shows the count holding, and give retries: 0 its real reason. fix(chat): say what actually keeps the centred scroll authoritative The comment credited the weaker guarantee. Every imperative scroll calls supersedeInitialScroll synchronously inside runScrollWithPromise, so the app's call cancels the list bootstrap by construction; note the datasetKey dep too.
Fabric view-flattening drops a Box2 that carries nothing but a testID, which takes the testID with it and makes the row unfindable to the native e2e drivers. Desktop ignores the prop, so carrying it costs nothing there.
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.
Evaluation branch, split out of
nojima/HOTPOT-legends-310so the jump work can be reviewed without the native-LegendList migration.The bug
Jumping to a message — a thread-search hit, a reply quote, the pinned banner, a permalink — did not reliably land on it. The app issued its own imperative scroll while the list was separately resolving a target of its own, and the two fought.
The change
shared/patches/@legendapp+list+3.3.7.patch— two library fixes: hold an index+viewPosition target across the measurement settle, and follow the end when the header measures after an at-end initial scroll. Both are proven necessary; removing either makes the flows below fail. Source:fix/jump-to-item-scroll-targetingon the vendored fork.useScrollToCenteredreplaces the old closed-loop centring — one scroll per centred target, per dataset.maintainVisibleContentPositionnow passed as the documented boolean rather than a partial config. Identical today (sizedefaults true), but a partial config opts out of whatever it does not name — which is howmaintainScrollAtEnd's{on: {...}}list silently lost a trigger once already.Most of the list configuration this relies on (
alignItemsAtEnd,initialScrollIndex,maintainScrollAtEnd,dataKey) already landed on master; the app-side delta here is small.Behaviour change riding along
useScrollToCentereddrops anelsebranch the old code had: when the centred ordinal cleared and the thread contained the latest message, the list used to scroll to the end.jumpToRecentis unaffected (it scrolls itself), but closing thread search no longer snaps to the newest message — it leaves the list where the reader was. Deliberate, and called out here because it is a UX change inside a bugfix.Native
App code for native is untouched — the chat thread still renders
FlatList. The runtime is not: the patch changes the native LegendList bundle as well, and the inbox already drives LegendList on native throughcommon-adapters/list.tsx'sLegendListNative—chat/inbox/list-helpers.tsxandchat/inbox/index.tsxboth callscrollToIndexwith aviewPosition. So native scroll targeting does change here, and it is unmeasured. If inbox jump-to-unread regresses on device, this is the PR to look at.Tests
Three desktop end-to-end flows, passing together 3/3 consecutive runs:
chat-search-hit— every hit lands, and the thread then stays where the reader put itchat-thread-bottom— opening a conversation lands on its newest message, with row growth deliberately delayed so late measurement can break itchat-link-jump— new; clicking a message link opens the target conversation landed on that message, which is the only flow covering a thread that mounts with a target already pending. It reuses a link an earlier run left behind rather than sending a new one each time, so running the suite does not accumulate messages in the smoke account's real history.chat-link-jumpasserts the target lands within a third of the viewport of centre, not merely that it is on screen.chat-search-hitis the looser one: it checks visibility only.yarn lint:allclean: 0 bailouts, 0 whole-props deps, tsc clean both projects.What a reviewer should look at
The two hooks in
list-area/index.tsxand their comments — the ordering and the predicate there are load-bearing and were arrived at by measurement, including one intermediate approach that an e2e test falsified and which was removed.🤖 Generated with Claude Code