chore(deps): routine dependency update; fix(chat): desktop send sticks to the bottom - #29551
Merged
Conversation
Bump @legendapp/list, expo + expo-asset/image-picker/location/task-manager, and immer. Regenerate the expo hmr patch for 57.0.14 (content unchanged). Add a **/deepmerge-ts resolution for GHSA-ggr8-5vv4-36mx: every installed copy was 7.1.5 and the @wdio/* parents are already current, so no released parent picks up the patched 8.x on its own. Held back deliberately: @babel/* 8.x, react-native 0.87, typescript 7 (typescript-eslint and analyze-styles still need the classic compiler API), and react-native-worklets 0.12 (no matching reanimated release).
legend-list 3.3.6 rewired maintainScrollAtEnd from a raw DOM scrollToEnd to its own computed scroll, gated behind scrollRequestTracker.runNowIfIdle. On a send, the dataChange trigger fires before the new row is measured, so the end target it computes is the offset the list already holds. That request moves nothing, no scroll event arrives to settle it, and while it counts as in flight runNowIfIdle drops every request behind it — including the one the new row asks for once it measures. The list parked one row short, and the next send's request cleared the stale one, so every other send looked fine. Patch settles a zero-distance scroll when it is issued instead of waiting for an event that cannot come. Measured over CDP on the running app: unpatched, 3 of 6 sends stayed 26px short with scrollTop never moving; patched, 46 of 46 sends landed at the end, and reverting the patch on the same build brought the failure straight back. Opening conversations was unaffected either way (5 of 5 scrollable threads landed at the end). Also moves the 16px gap above the input off the list and onto its container. The list feeds its own paddingBottom into content size, the end target and the at-end threshold, so outside the scroller is the simpler place for it. This is not what caused the regression — the shortfall was exactly one message row — it just keeps that arithmetic to message sizes. The send flow now reads the scroller's distance from the end across three sends rather than trusting that the sent message is visible, which it is either way. It refuses to pass on a thread with no scrollable history.
Sending a message left the desktop thread one row short of the end, and the next send made up the missing pixels — so every other send parked short. onSubmit called scrollToBottom() on top of maintainScrollAtEnd, and that imperative scroll loses the race in both directions. Its target is resolved before the sent row has measured, so it lands at the end as it was one row ago; and while it counts as in flight the list declines its own end-anchor request (scrollRequestTracker.runNowIfIdle refuses to supersede an explicit scroll) and then abandons it, so nothing follows the row once it does measure. Skipping the scroll when the list is already at the end leaves the anchor to do the job it was already doing. The jump-from-far-away case still scrolls, which is what scrollToBottom is for. Reproduced standalone first, in a legend-list example-web fixture appending one row at a time against library source: with the composer's scrollToEnd on the append, 8 of 8 appends parked 26px short; with it off, 0 of 8. Toggling it is the only variable. In the app on stock 3.3.6: 10 of 10 sends land at the end, and reverting just this guard on the same build brings back 5 of 8 short. Also drops patches/@LegendApp+list+3.3.6.patch, added in the previous commit. The standalone fixture shows it does not fix this: 8 of 8 appends stay short with it applied. It changed the app's timing rather than the behaviour, and the measurements that seemed to vindicate it were mis-attributed. The gap it was aimed at is real but is the library's business, not ours: an end anchor declined behind an in-flight scroll is dropped with no replay. Left to upstream, with the fixture as the reproduction if it is worth filing.
… the gap The gap above the input area is padding on .chat-message-list, so the scroller inside it ends above that box. keepWithinList measured the bar against the outer box, which left the lowest row's bar hanging in the padding where the scroller clipped it: bar bottom 867 against a scroller ending at 851, with no raise applied because the outer box still had room. Measure against the scroller instead — the one ancestor sitting directly inside the outer box. Not the "nearest scrollable ancestor": every row container reports as one, which lifts every row by its own height instead. The raise also stacked onto itself. measure() runs on mount and again on pointer re-entry, and it added back the raise from a ref that React had not painted yet, so the second pass could double it. It now reads the raise the DOM is actually carrying. At the old 16px gap those two bugs cancelled at some viewport heights, which is why this only surfaced when the padding moved. Halves the gap to 8px while here, as asked. Measured live, hovering the four lowest rows and reading the bar against the scroller's bottom edge: before, 16px gap: lowest row overflows by 16px, raise 0 FAIL before, 8px gap: flush, but raise 16 where 8 was due (cancelling) after, 16px gap: flush at 851, raise 16, other rows raise 0 PASS after, 8px gap: flush at 859, raise 16, other rows raise 0 PASS Staleness still holds, which is what the re-entry listener is for: hovering a row mid-list (raise 0), scrolling it down to the bottom edge, then re-hovering re-measures to raise 16 and lands flush.
… bar A raised bar landed flush on the scroller's bottom edge, which read as tight against the input area. Raise by the overflow plus 2px so it clears the edge. Rows that already fit are still left alone. Lowest row now sits at 857 against a scroller ending at 859, raise 18; rows with room raise 0. Re-measuring on re-entry still holds: hovering a row mid-list (raise 0), scrolling it to the bottom edge and re-hovering gives raise 18.
…t changes
A multi-line message did not stick to the bottom. Composing one grew the composer
a line at a time, and since the composer is a flex sibling of the list, each line
took 22px of list viewport. The list did not re-pin as it shrank, so by six lines
it sat 110px above the newest message — before anything was even sent.
Two independent causes, each measured on its own.
The composer now reserves the height it has when empty and grows upward over the
thread from there, so typing never resizes the list. The reserve is measured
rather than hardcoded: the resting composer is not a fixed height, and the
measurement tracks the smallest height seen since the last window resize, which
is right from the first frame because a conversation opens with an empty
composer. Cost of this shape, which is the point of it: a tall composer covers
the newest messages while you type.
before: list viewport 779 -> 669 across six composed lines, distance from the
end 0.5 -> 110.5, and 5 of 5 six-line sends landed 114px short
after: viewport stays 779 at every composer height, distance stays 0.5, and
6-line and 12-line sends land at the end
maintainScrollAtEnd also gets its layout trigger back. Naming any trigger opts
out of every trigger left unnamed, so listing dataChange/footerLayout/itemLayout
silently disabled the one that fires when the viewport itself changes. That is a
real bug on its own, independent of the composer:
before: shrinking the window viewport by 200px left the thread 200px short of
the end, by 120px left it 120px short
after: pinned at both
Single-line sends still land (6 of 6), and the hover reaction bar still clears
the list edge.
…settle loop Restores the composer as a flex sibling of the thread, so growing it shortens the list the way it used to, and makes landing at the end reliable instead of relying on the layout being calm. Sending is not one event: the composer collapses and a row is appended in the same frame, so the viewport grows while the content does, and the row only reaches its real height once measured. Any single scroll resolves its target against one moment of that and lands short. scrollToBottom now re-aims until the scroller sits at its end for two consecutive readings, bounded at 1.5s, and gives up if the reader touches the wheel. It reads the scroller's own scrollHeight/clientHeight/scrollTop rather than the list's isAtEnd, because that flag is derived from the content size and viewport the list has recorded and both lag a composer collapse — trusting it left 1 in 10 two-line sends 42px short, since two readings 50ms apart can both be stale-true. Measured per message size, sends landing within 8px of the end: 1 line 10/10, 2 lines 10/10, 3 lines 10/10, 4 lines 6/6, 6 lines 6/6, 8 lines 6/6, 10 lines 6/6, 16 lines 6/6 With the loop reduced to a single scroll, 4-line sends went 5 of 6 short at 77.5px and six-line ones compounded to 228.5px, so the loop is doing the work. Rapid one-line sends 10/10, the window-resize pinning from the layout trigger still holds, and the hover reaction bar still clears the list edge.
Replaces the 1.5s re-aiming loop with the ordering fix it was compensating for. onSubmit cleared the composer and sent in the same tick, so React committed the composer shrinking back to one line and the new row arriving as one change. The list resolved its end against a single moment of that and landed short — up to 114px for a six-line message. Clearing first and sending on the next frame lets the viewport settle before the row exists, and the list's own end anchor handles the rest. What stays in our code is the guard on scrollToBottom, and it now asks the scroller instead of the list's isAtEnd. That flag is derived from the content size and viewport the list has recorded, both of which lag a composer collapse, so it read not-at-end while the scroller was at its end; we then scrolled, landed short, and blocked the anchor. Guarding on isAtEnd left 2 of 8 two-line sends and 4 of 8 six-line ones short. scrollToBottom still exists for what it is for — returning from elsewhere in the thread — which is measured working: sending from 1500px away jumps to the end, 4 of 4. Sends landing within 8px of the end, per message size: 1 line 10/10, 2 lines 8/8, 3 lines 8/8, 6 lines 8/8, 10 lines 8/8, 16 lines 8/8 Rejected along the way, all measured: an unconditional one-shot scroll (8 of 8 short at every size, 5 of 10 single-line — it is the original bug); maintainScrollAtEndThreshold at 0.3 (six-line 8 of 8 short); at 1 it does work library-side, 30 of 30, but it means following the end from within a whole viewport, so reading a few messages up would get yanked back by anything arriving. Window-resize pinning and the hover reaction bar unchanged.
…e we differ legend-list documents `maintainScrollAtEnd` as a boolean and its chat example passes it that way. We passed a trigger list instead, and naming any trigger opts out of the ones left unnamed — which is how the layout trigger went missing and a window resize lost the end. Ours is now the documented form, which is what the trigger list had grown into anyway. Also records, next to each of them, why the two remaining deviations from that example exist, since both look like something to simplify away: Their example appends the message and calls scrollToEnd in the same handler. That works there because its composer is a single-line input which never resizes the list; ours is multi-line, so clearing it grows the thread's viewport and lands as one change together with the new row. Measured in that order, every send was short — 8 of 8 at one, two and six lines. Hence clearing first and sending on the next frame. Their example also scrolls unconditionally on send. We only scroll when we are not already at the end, because while the list is at the end maintainScrollAtEnd owns the position and an imperative scroll displaces it: it resolves its target before the sent row has measured, and while it counts as in flight the list declines its own end anchor. Scrolling unconditionally, animated as they do, cost 3 of 10 single-line sends; the same scroll issued on the next frame instead broke coming back from further up the thread, 1 to 2 of 4. Unchanged and re-measured on this: sends landing within 8px of the end 10/10 at one, two, six and sixteen lines; returning from 1500px away lands 4 of 4; window resize still pinned.
chrisnojima
changed the title
…ine sends
Review findings on the send path, two of them correctness bugs the scrolling work
introduced.
The deferred send held the only copy of the message text while the composer had
already been cleared synchronously, and it held it in a requestAnimationFrame
callback. Frames stop in a hidden, minimised or backgrounded window, so pressing
Enter and immediately hiding the window dropped the message: gone from the
composer, gone from the draft, never sent, no error. A timeout runs in all of those
cases. It also shortens the window in which the trailing clear from `afterSend`
could wipe a keystroke typed right after Enter, from a frame to a task.
`jumpToRecent` also moved back after the send. It clears the validated ordinal
range, and running it first left the send reading a degraded clientPrev hint on a
search-centred thread.
The deferral itself stays, and is still load-bearing on 3.3.6 with
maintainScrollAtEnd={true}: sending in the same tick puts 16-line messages 8 of 8
short. Measured on the timeout version — sends landing within 2px of the end 10/10
at one, two, six and sixteen lines, returning from 1500px away 4 of 4, nudged 5 to
60px off the end 4 of 4, window resize pinned, hover bar clear.
The e2e flow could not catch any of this. It polled until the first reading that
looked right, which accepts exactly the late-settling failures this branch chased,
and it only ever sent single-line messages, which do not exercise the composer
collapse at all — it passed against a deliberately broken same-tick send. It now
waits for two equal readings before asserting, shares the app's 2px tolerance
instead of its own 8px, and sends a twelve-line message. Twelve because the
shortfall scales with the collapse and the shorter ones fail only intermittently.
Against the same-tick mutation it now fails, 221.5px short.
Also notes that the 8px gap is a deliberate halving rather than a leftover.
A thread with less content than the viewport rendered from the top with the empty space below it. legend-list has `alignItemsAtEnd` for exactly this and we never set it — `alignItemsAtEndPaddingEnabled` was false, so the spacer it inserts was never rendered. It was only ever passed to the native list, added in the Legend List port and removed again when native went back to FlatList; the desktop list never had it despite that commit message claiming they matched. Inert for any thread taller than the viewport, since the padding it adds is max(0, viewport - content - insetEnd). NOT measured, on request: whether short threads now bottom-align in practice, and whether this disturbs sending on one. Both are worth an eye. The padding feeds getContentSize, and finishMaintainScrollAtEnd issues a requestAdjust whenever it changes — which is what happens as a short thread grows toward filling the viewport, i.e. while sending into one. The content also includes SpecialTopMessage, so on a new conversation the NewChatCard may already fill the viewport and leave nothing to pad.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR combines a routine dependency refresh (Expo 57.0.14 and related packages, @legendapp/list 3.3.6, immer 11.1.17) with a targeted desktop chat fix to ensure “send” keeps the thread pinned to the bottom, plus a regression-focused Electron E2E update.
Changes:
- Bumped Expo/LegendList/immer deps, updated iOS pods lock, and carried forward the Expo HMR patch as
expo+57.0.14.patch. - Adjusted desktop thread list behavior to avoid conflicting imperative scrolls, re-enable all
maintainScrollAtEndtriggers, and move bottom gap padding out of the list’s scroll math. - Updated desktop hover-reaction bar measurement to clamp against the actual scroller and added an Electron E2E that asserts “distance from end” across multiple sends (including multiline).
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shared/package.json | Version bumps and adds a deepmerge-ts resolution for the security advisory. |
| shared/yarn.lock | Lockfile updates reflecting the dependency bumps and forced deepmerge-ts@8.0.1. |
| shared/ios/Podfile.lock | Pod lock updates corresponding to the Expo bumps. |
| shared/patches/expo+57.0.14.patch | Carries the Expo HMR reload() patch forward for 57.0.14. |
| shared/chat/conversation/list-area/index.tsx | Desktop thread scroll/pinning fixes (maintainScrollAtEnd, end tolerance, padding move, align-at-end). |
| shared/chat/conversation/input-area/normal/index.tsx | Adjusts send ordering (clear first, send next tick) and limits imperative scroll-to-bottom behavior. |
| shared/chat/conversation/messages/emoji-row.tsx | Measures hover bar overflow against the scroller and avoids stale “raise” stacking. |
| shared/tests/e2e/electron/flows/chat-send-message.test.ts | Adds an end-pinning regression check based on scroller distance from end across multiple sends. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
A routine dependency update, plus the desktop chat scrolling it turned up.
Dependencies
Bumped (exact versions):
@legendapp/list3.3.5 → 3.3.6,expo57.0.13 → 57.0.14,expo-asset→ 57.0.12,expo-image-picker/expo-location/expo-task-manager→ 57.0.11,immer11.1.16 → 11.1.17. Pods reinstalled after a targeted clean, soPodfile.lockmoves with the expo bumps.expo+57.0.13.patchregenerated asexpo+57.0.14.patch— the hmrreload()fix is still not upstream and the content is byte-identical.Held back deliberately:
@babel/*8.x,react-native0.87 (drags the whole cluster),typescript7 (typescript-eslintandscripts/analyze-styles.mtsstill need the classic compiler API),react-native-worklets0.12 (no matching reanimated release),@types/react-dom19.2.4 (runtime unchanged).Security: added
"**/deepmerge-ts": "8.0.1"for GHSA-ggr8-5vv4-36mx — every installed copy was 7.1.5 and the@wdio/*parents are current, so no parent picks up the patched 8.x on its own. Re-ran the removal test on**/serialize-javascriptand**/xcode/uuidwith their lockfile entries deleted so the ranges actually re-resolved: both fell back to vulnerable versions, so both stay. Remaining advisories have no patched release:extract-zip2.0.1,image-size1.2.1 (×2).protocol/andrnmodules/react-native-kb/were checked too — both current.Desktop chat: sending did not stay at the bottom
3.3.6 moved
maintainScrollAtEndonto a shared scroll-request tracker, which declines to supersede an in-flight imperative scroll. That exposed three separate problems on our side.1. The composer scrolled to the end itself on send.
onSubmitcalledscrollToBottom()on top ofmaintainScrollAtEnd. That scroll resolves its target before the sent row has measured, so it lands one row short — and while it counts as in flight the list declines its own end anchor and abandons it, so nothing follows the row once it does measure. It now only scrolls when we are not already at the end, which is what it is actually for: coming back from further up the thread.Asking whether we are at the end reads the scroller rather than the list's
getState().isAtEnd. That flag is derived from the content size and viewport the list has recorded, and composing a message resizes that viewport repeatedly, so it still reads not-at-end after the composer has collapsed. Guarding on it left 2 of 8 two-line sends and 4 of 8 six-line ones short.2. Clearing the composer and sending in one tick. Emptying a multi-line composer grows the thread's viewport, and sending in the same tick makes that growth and the new row a single change for the list to resolve its end against — up to 114px short for a six-line message. Clearing now lands before the row arrives.
3.
maintainScrollAtEndwas passed a trigger list. Naming any trigger opts out of every trigger left unnamed, so{on: {dataChange, footerLayout, itemLayout}}silently disabled the one that fires when the viewport itself changes. Shrinking the window by 200px left the thread 200px short of the newest message. It is now the documented boolean, which enables all of them.The gap above the composer also moved off the list's own
paddingBottomand onto its container, halved to 8px. The list feeds its padding into every scroll-offset calculation it makes — content size, the end target, the at-end threshold — so keeping it outside the scroller keeps that math on message sizes alone. That move is what pushed the hover reaction bar past the scroller's edge on the lowest row, soemoji-row.tsxnow measures against the scroller instead of the list's outer box, and reads the raise the DOM is actually carrying instead of one from a ref that React may not have painted yet. It clears the edge by 2px rather than sitting flush on it.Measurements
Driven over CDP against a live thread, counting sends that come to rest within 8px of the end:
Returning from 1500px up the thread and sending lands 4 of 4. Window resize stays pinned (200px and 120px shrinks). The hover reaction bar clears the scroller edge on the four lowest rows, with nothing over-raised, and still re-measures on re-entry after a scroll.
Each piece was checked in both directions — reverting it alone brings the failure back:
isAtEndas the guard: 6 of 16 short at two lineschat-send-message.test.tsnow reads the scroller's distance from the end across three sends rather than trusting that the sent message is visible — it is visible either way — and refuses to pass on a thread with no scrollable history.Not done
No upstream PR. Every failure I could reproduce standalone traced to our own code; a library patch I wrote early on was disproved by a standalone harness and dropped, and we ship stock 3.3.6. legend-list's own chat example appends and scrolls in one handler, which works there because its composer is a single-line input that never resizes the list — taking that shape verbatim here left 8 of 8 sends short, so the two deviations above are documented in the code next to themselves.
All of this is desktop: the native thread renders a
FlatList, notLegendList, and the reaction-bar measurement early-returns on mobile. The one shared change is therequestAnimationFramearoundsendComposerText, which alters native's send ordering too — sending from iOS was spot-checked by hand and behaved, but it carries no measurements of its own.The 8px gap and the 2px bar clearance are visual calls for review.
🤖 Generated with Claude Code