Open linked message at top - 3#95045
Conversation
|
|
…-top-3 # Conflicts: # src/pages/inbox/report/ReportActionsList.tsx
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1431e3a498
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
melvin-bot
Bot
requested review from
aimane-chnaif and
heyjennahay
and removed request for
a team
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 897bfe9415
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| + const initialScrollIndexParams = this.propsRef.initialScrollIndexParams; | ||
| + const viewPosition = initialScrollIndexParams === null || initialScrollIndexParams === void 0 ? void 0 : initialScrollIndexParams.viewPosition; |
There was a problem hiding this comment.
This initial-render adjustment reads only viewPosition, but applyInitialScrollIndex later adds initialScrollIndexParams.viewOffset before doing the corrective scrollToOffset. ReportActionsList now passes {viewPosition: 1, viewOffset: CONST.REPORT.ACTIONS.LINKED_MESSAGE_OFFSET}, so opening a linked/unread message first paints at one offset and then snaps 40px on the correction, reintroducing the visible jump this patch is trying to avoid. Include the same viewOffset in initialItemOffset before assigning engagedIndicesTracker.scrollOffset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'm not sure we need this change here.
applyInitialScrollAdjustment (this code) and applyInitialScrollIndex do two different jobs:
applyInitialScrollAdjustmentsetsengagedIndicesTracker.scrollOffset, which only feedscomputeVisibleIndices— i.e. it decides which cells get rendered, not the on-screen scroll position. That window is expanded bydrawDistance, so a 40pxviewOffsetdifference stays well within the buffer and renders the same cells either way.applyInitialScrollIndexis what actually positions the content viascrollToOffset. It runs inonCommitLayoutEffectand it does includeviewOffset. So the first painted frame is already at the correct offset; there's no "paint then snap 40px"
This asymmetry is also the pre-existing upstream design: before this patch, applyInitialScrollAdjustment had no initialScrollIndexParams handling at all, while applyInitialScrollIndex has always applied viewOffset. The anchor only needs to be "close enough" to render the right cells; the controller needs the exact offset because it sets the visible position.
| // the target item's position. Reading before recompute would capture a | ||
| // stale offset, causing the wrong items to be rendered. | ||
| - this.layoutManager.recomputeLayouts(0, initialScrollIndex); | ||
| + this.layoutManager.recomputeLayouts(0, this.getDataLength() - 1); |
There was a problem hiding this comment.
Changing this to this.getDataLength() - 1 makes every FlashList with initialScrollIndex recompute layouts for the entire dataset during mount, not only report links. I checked existing callers such as src/components/LHNOptionsList/LHNOptionsList.tsx, where web restores the saved LHN scroll with initialScrollIndex; on accounts with large report lists this turns reopening the LHN into an O(n) layout pass before first paint even when the saved index is near the top. Limit the widened range to the linked-message/viewPosition path or to the indices needed to calculate the target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This change intentionally matches the upstream fix Shopify/flash-list#2318, you can check for more details there. Narrowing the range would also bring back the bug this fixes: stopping at initialScrollIndex leaves the layout table non-monotonic, so the binary search resolves to the wrong item.
…-top-3 # Conflicts: # src/components/FlashList/InvertedFlashList/index.tsx # src/components/FlashList/useFlashListScrollKey.ts
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.movweb-unread.mov |
|
@codex review |
|
🤖 Code review — this is a clean, well-structured refactor. Moving mount positioning into Worth verifying
Regression coverageThe main risk here is platform-specific FlashList positioning behavior — exactly what bit #94804. The 4 prior DBs (#94741, #94745, #94751, #94753) should be explicitly re-verified on native iOS/Android + mWeb, not just desktop web, since the deleted two-frame RAF handoff was platform-sensitive. The PR includes videos across platforms, so this is largely covered — flagging it as the area to weight QA toward. Nothing here is blocking from a code-correctness standpoint; #1 and #4 are the two I'd want an explicit answer on before merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c25429fa50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| : undefined; | ||
| const shouldFocusToTopOnMount = shouldBeAlignedToTop && !initialScrollKey; | ||
| const [shouldAutoscrollToBottom, setShouldAutoscrollToBottom] = useState(shouldFocusToTopOnMount); | ||
| const [shouldDisablePillTracking, setShouldDisablePillTracking] = useState(!!initialScrollKey); |
There was a problem hiding this comment.
When the same report screen instance is reused and reportActionID/the unread anchor changes, the FlashList is remounted via its key, but this state initializer does not run again after the prior onLoad set the value back to false. In that in-app deep-link/unread-navigation path the new initial initialScrollIndex positioning runs with pill tracking enabled, so intermediate scroll/content events can show the latest-messages pill again even though this change is meant to suppress that flicker during initial linked-message positioning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I've tested different scenarios, and haven't found a case when the anchor changes without a report change (remount). Even if I deep-link to another message in the same report, the whole page is remounted.
@aimane-chnaif do you have any flow like this in mind?
|
Confirmed skeleton issues are not reproducible |
|
iOS flicker: ios-flicker.movAndroid not working well for report preview: (around 20-22s) android.mov |
|
Thank you! I'll prioritize it all on Monday! |
|
@aimane-chnaif, can you please try now and let me know if the flicker on iOS is fixed for you? |
|
@VickyStash what is test email to invite? |
|
Please, add viktoryia.kliushun+a@callstack.com |
|
@aimane-chnaif It works as expected for me in the current PR state: Monosnap.screencast.2026-07-06.14-53-38.mp4 |
|
Regarding removing the custom
- Similar updates were already done through the app for performance gains:
But it can potentially cause white spaces during quick scrolling (and it's mentioned in similar PRs too) Maybe we can try to ship it as a small separate PR. cc @mountiny for opinion |
|
Yeah I like keeping that separate |
…-top-3 # Conflicts: # src/hooks/useReportActionsScroll.ts # src/pages/inbox/report/ReportActionsList.tsx # src/pages/inbox/report/useReportUnreadMessageScrollTracking.ts
Works fine in latest codebase |
|
@aimane-chnaif What about iOS flicker? |
|
But bad flicker when scrolling down Screen.Recording.2026-07-07.at.8.49.25.AM.mov |
Flicker not happening but instead bad scrolling and content blank issue as you reported bug.MP4 |
|
@aimane-chnaif I've applied a little different fix, please, let me know what you think 🙏 |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
4 participants
Explanation of Change
Previous PR was reverted #94804 with next DB:
#94741
#94745
#94751
#94753
This PR reworks how a report opens at a linked/unread message. It replaces the old data-slicing approach (
useFlashListScrollKey, which sliced data so the target landed at the visual bottom of the inverted list) with a native FlashList positioning approach usinginitialScrollIndex+initialScrollIndexParams.viewPosition.Key changes:
viewPositionsupport toinitialScrollIndexParams(mirrors scrollToIndex), anchors the first painted frame at the target position (avoids the jump), adds a bottom-crop hint for inverted lists, widensrecomputeLayoutsrange, and makes the deferred re-scroll read the latest offset (fixes stale-offset snap-back).useReportActionsScrollnow owns mount positioning and returnsinitialScrollIndex,initialScrollIndexParams, andmaintainVisibleContentPosition. For a linked message it scrolls the target to the top (viewPosition: 1) with aLINKED_MESSAGE_OFFSET(40px) so the preceding message is partly visible.Cleanup enabled by the new approach:
useFlashListScrollKeyand simplifiedInvertedFlashListactionIndexMapworkaround inReportActionsList— since the data is no longer sliced, FlashList's providedindexis correct fordisplayAsGroup.Fixed Issues
$ #92152
PROPOSAL: N/A
Tests
Latest messagespill appears. Copy link of any visible message. Open the copied link. Make sure theLatest messagespill displays and does not flicker.Previously reported DB should also be verified:
#94741
#94745
#94751
#94753
Offline tests
Same, as in Tests section
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in Tests section
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
web1.mp4
web2.mp4
Previous Deploy Blockers
DB_part1.mp4
DB_part2.mp4