fix(mobile): land the first thread open above the composer on Android - #5585
fix(mobile): land the first thread open above the composer on Android#5585PollyGlot wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus 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 |
ApprovabilityVerdict: Needs human review An unresolved review comment identifies a potential regression on pre-glass iOS devices where the old inset seeding was removed but the new Android-only fix doesn't cover that case. This platform-specific concern warrants human verification. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9ed20ff. Configure here.
| // on top of the animated padding. | ||
| {...(Platform.OS === "android" && !usesNativeAutomaticInsets | ||
| ? { contentInset: { bottom: bottomContentInset } } | ||
| : {})} |
There was a problem hiding this comment.
Pre-glass iOS loses inset floor
Medium Severity
contentInsetEndStaticAdjustment no longer seeds bottomContentInset when automatic insets are off, and the new declarative contentInset seed is Android-only. Pre-glass iOS (usesAutomaticContentInsets false) is left on the remount reportContentInset path alone, which this PR describes as racing the one-shot initial scroll-to-end—so the first open of an uncached thread can still rest one composer-height low there.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9ed20ff. Configure here.
There was a problem hiding this comment.
Pre-glass iOS does not need the floor: this blind spot is Android-specific. In react-native-keyboard-controller's ScrollViewWithBottomPadding, the animated bottom padding is written as the native iOS contentInset prop (animatedProps.contentInset = effective), so UIKit accounts for it during attach and in the scrollable range, and LegendList reads it back as nativeContentInset from scroll events (its base end inset when no contentInset prop is set). Only Android uses the synthetic contentInsetBottom path — the library's own onContentInsetChange doc says it exists because on Android the synthetic inset is not reflected in onScroll events. That is also why the floor must not be passed on iOS: effective adds the contentInset prop on top of the dynamic padding there, which would double the native inset. Pre-glass iOS behavior is byte-for-byte the same as main (remount re-report only), and the original bug never reproduced on iOS.
On the non-automatic-inset path (Android, pre-glass iOS) the composer overlay only exists as the keyboard integration's animated bottom padding, which LegendList's scroll math cannot see. The imperative reportContentInset issued on the empty->filled remount raced the list's initial scroll-to-end, so the very first (uncached) open of a long thread could rest one composer-height low, hiding the last lines behind the text field. Declare the estimated overlay height through contentInsetEndStaticAdjustment instead, so the initial scroll math includes it from construction, and drop the racy imperative report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Restore the measured reportContentInset re-report on the empty->filled remount (it carries pending approval / user-input card heights the estimate misses) and drop the static-adjustment change that would have double-counted once the keyboard integration reports its own inset. The initial-scroll floor now rides LegendList's declarative contentInset prop instead, Android-only: it is consumed by JS scroll math alone there (Android's ScrollView has no native contentInset) and the first reported override replaces it rather than adding to it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9ed20ff to
743c05c
Compare


Summary
On Android (and pre-glass iOS), the very first open of a thread whose messages are not yet in the client cache can rest with its last lines hidden behind the composer text field. Once the thread has been opened before (messages cached), the same open lands correctly, which is why this mostly bites right after a cold app launch.
Root cause: on the non-automatic-inset path the composer overlay only exists as the keyboard integration's animated bottom padding, which LegendList's scroll math cannot see. The screen compensated with an imperative
reportContentInseton the empty→filled remount, but that call races the list's one-shot initial scroll-to-end; when it loses, the initial position is computed with a zero end inset and rests exactly one composer-height low.Fix: declare the estimated composer overlay height through the existing
contentInsetEndStaticAdjustmentprop (the same channel iOS already uses for UIKit's safe-area overcount), so the scroll math knows about the overlay from construction. The racy imperative report becomes redundant and is removed. Net: 9 insertions, 15 deletions, one file.Before / After
First open of an uncached long thread, right after a cold launch (left: last lines and timestamp rest behind the composer; right: they land above it):
Test plan
tsc --noEmit,vp lint,vp fmtclean.Related: #5582 fixes the separate issue that Android renders no backdrop gradient, so text scrolling behind the composer stayed fully legible. The two are independent.
🤖 Generated with Claude Code
Note
Low Risk
Scoped to Android non-automatic-inset scroll positioning in ThreadFeed; iOS path is explicitly excluded from the new prop.
Overview
Fixes Android thread opens where the feed’s first scroll-to-end could run before the composer overlay height reached LegendList’s scroll math (inset updates arrive asynchronously after an empty→filled list remount), leaving the last lines under the composer.
On Android when native automatic insets are off,
ThreadFeednow passes a declarativecontentInset.bottomseeded frombottomContentInsetso LegendList can use that floor in JS-only scroll math at mount; the laterreportContentInsetfrom layout still replaces that value rather than stacking. iOS is unchanged—the spread is omitted so UIKit would not double-apply insets.Comments around the remount
useLayoutEffectinset re-report are updated to describe this split (declarative floor on Android vs. re-report after remount).Reviewed by Cursor Bugbot for commit 743c05c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix first thread opening above the composer on Android in
ThreadFeedOn Android, the thread list was not correctly accounting for the composer overlay on initial render, causing the first thread to open obscured. ThreadFeed.tsx now conditionally spreads a
contentInsetwith a bottom value equal tobottomContentInsetwhen running on Android and native automatic insets are disabled, seeding the correct inset before the explicit override is reported.Macroscope summarized 743c05c.