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.
1 participant
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