[WIP] feat: Open report with linked/unread report actions in the middle of the chat#90375
Conversation
|
This is working relatively well already, though there are some bugs that i still need to fix. Before, it was already possible to open the report with the bottom edge of the linked item in the vertical middle of the chat. The tricky part is to instead render the top edge of the linked item (where we show the "unread message" line) in the middle, since we need to render the item and re-adjust the scroll position. Since we cannot estimate the message heights, this involves some tricky logic. Screen.Recording.2026-05-12.at.21.58.28.mov |
|
There are some bugs that i need to fix, like the report reloading when the message is marked unread. Also, i have to test whether the measurement of the linked item and re-adjusting the scroll position adds a significant delay to the initial render of the chat. |
…n-the-middle-of-chat
…n-the-middle-of-chat
|
@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] |
…n-the-middle-of-chat
…n-the-middle-of-chat
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdf02dceb4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return () => { | ||
| isCancelled = true; | ||
| }; | ||
| }, [hasInitialScrollTarget, initialScrollKey, linkedReportActionID, listID, report.reportID, resetMeasuredScroll, shouldMeasureScrollTarget, skipMeasuredScroll]); |
There was a problem hiding this comment.
Memoize measured-scroll callbacks before using as deps
useLayoutEffect depends on resetMeasuredScroll/skipMeasuredScroll, but those callbacks are recreated on every render by useMeasuredLinkedRowScroll. That makes this effect run every render, and each run queues setSessionSnapshot(nextSession) with a fresh object, which can cause a render loop and repeatedly reset initial viewport state (isInitialViewportLoading/measured scroll) instead of letting the initial positioning settle.
Useful? React with 👍 / 👎.
| if (mountedIndices.size < range.requiredMountedItems) { | ||
| return false; |
There was a problem hiding this comment.
Add fallback when initial viewport mount count is under-estimated
This gate requires mountedIndices.size >= requiredMountedItems, where requiredMountedItems is inferred from an average row height. For chats with very tall actions near the anchor, FlashList may mount fewer cells than that estimate, so isInitialViewportCovered never returns true and the full-screen initial skeleton can remain indefinitely. Unlike measured scroll, there is no timeout fallback for this loading gate.
Useful? React with 👍 / 👎.
|
Please fix conflict and failing tests |
|
Is this still WIP? If ready for review, remove it from title. |
Code ReviewThis is a well-structured approach to centering linked/unread report actions. The hook decomposition ( High Priority1. Unstable function references cause effects to re-fire every render
Since they only close over const skipMeasuredScroll = useCallback(() => {
const runtime = runtimeRef.current;
runtime.shouldSkipScroll = true;
resetMeasuredScrollRuntime(runtime);
}, []);Same for 2.
Either wrap in Medium Priority3. This function ( 4. Test coverage is thin Only 3 test cases cover the happy path and unread-marker-only change. Missing:
Low Priority / Observations5. Linear scan duplication — 6. Follow-up rAF edge case — If Items 1-2 are the main blockers — the unstable function references mean the initialization and pagination effects fire far more often than intended. The rest are improvements worth considering. |
|
Closing as this feature would introduce a major initial list render delay. This is something that |
@quinthar
Explanation of Change
Updates the initial chat rendering logic so that the initial/unread report action is displayed in the middle of the chat rather then at the bottom.
The centered rendering is acting by the following approach:
Fixed Issues
$ #88502
PROPOSAL:
Tests
Big screen (list height > report action height)
Small screen (list height < report action height)
Offline tests
None needed.
QA Steps
Same as in Tests.
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
iOS: Native
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-05-19.at.16.53.46.mov
MacOS: Chrome / Safari
Screen.Recording.2026-05-19.at.16.51.52.mov