[63253] (1/2) Open transaction thread on top #70250
Conversation
… is within the viewport
…transaction-thread-on-top
|
@vit @c3024 PR is ready for review! We'll need confirmation from design team regarding this: 
Previously there was a top blank space because the content was vertically aligned to the bottom. I switched this to be aligned to the top, but now the blank space lies at the bottom. IMO it's easier to read the transaction details when using the header arrows to move between transactions. |
|
@Expensify/design Can you please confirm the above? Noting here that with this alignment, if the transaction thread has several messages, the view will show the transaction details rather than the latest message details. Screen.Recording.2025-09-12.at.7.40.03.AM.mov |
|
I believe we've recently introduced this so it's by design. @Expensify/design for triple check |
|
Indeed that is desired behavior. And I love it! |
|
On Android, clicking on a transaction while the onscreen keyboard is open adds extra space at the bottom, as if the keyboard is still present after navigating to the transaction. This works correctly on main. On main: threadAndroidKeyboardOnMain.movOn this branch: threadAndroidKeyboardOnThisBranch.mov |
| * random enough to avoid collisions | ||
| */ | ||
| function keyExtractor(item: OnyxTypes.ReportAction): string { | ||
| if (item.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) { |
There was a problem hiding this comment.
Why a separate key extractor for created action?
There was a problem hiding this comment.
@c3024 this is to prevent UI flickering after fetching the real created action
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
PR SummaryModified the report action list to ensure transaction threads and money request reports scroll to the top by default instead of the bottom. Implemented proper keyboard handling to prevent content from being obscured when the keyboard is open, and fixed various scrolling behaviors across different platforms. Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (11)
- 6d142c1: Merge branch 'main' into pac-guerreiro/fix/63253-after-revert-1-open-transaction-thread-on-top
- f23a6cd: fix(63253): top blank space on transaction threads in RHP
- 4342e9c: fix(63253): keyboard overlapping last visible message
- ca4f5d6: fix(63253): keyboard overlapping messages on native platforms
- 8d67cb7: Merge branch 'main' into pac-guerreiro/fix/63253-after-revert-1-open-transaction-thread-on-top
- 858d0c1: chore(63253): remove unused import
- a25f522: fix(63253): does not scroll down to the sent message when keyboard is opened
- 207ffed: fix(63253): transaction thread does not scroll correctly when menu/keyboard is opened
- 10523fb: fix(63253): blank space at the bottom in the transaction thread in self DM
- db872f6: fix(63253): new message button appears on top when the unread message is within the viewport
- 571b8af: Revert "Merge pull request #69784 from Expensify/revert-68932-pac-guerreiro/fix/63253-1-open-transaction-thread-on-top"
This reverts commit 12d2426, reversing
changes made to c001b19.
Files Processed (8)
- src/components/ActionSheetAwareScrollView/index.ios.tsx (3 hunks)
- src/components/ActionSheetAwareScrollView/index.tsx (2 hunks)
- src/components/ActionSheetAwareScrollView/types.ts (1 hunk)
- src/components/ActionSheetAwareScrollView/useActionSheetKeyboardSpace.tsx (3 hunks)
- src/hooks/useReportScrollManager/index.native.ts (1 hunk)
- src/hooks/useReportScrollManager/index.ts (1 hunk)
- src/hooks/useReportScrollManager/types.ts (1 hunk)
- src/pages/home/report/ReportActionsList.tsx (20 hunks)
Actionable Comments (3)
-
src/components/ActionSheetAwareScrollView/useActionSheetKeyboardSpace.tsx [244-255]
performance: "Inefficient conditional style calculation in animatedStyle"
-
src/pages/home/report/ReportActionsList.tsx [138-142]
possible bug: "Potential key collision in keyExtractor function"
-
src/pages/home/report/ReportActionsList.tsx [866-873]
performance: "Inefficient function creation in getRenderScrollComponentWithAnimationProps"
Skipped Comments (3)
-
src/hooks/useReportScrollManager/index.native.ts [57-63]
possible issue: "Missing default value for animated parameter"
-
src/pages/home/report/ReportActionsList.tsx [350-359]
performance: "Unnecessary recalculation of topOffset on every render"
-
src/pages/home/report/ReportActionsList.tsx [771-794]
maintainability: "Complex keyboard handling logic in onLayoutInner function"
| const animatedStyle = useAnimatedStyle( | ||
| () => | ||
| isInvertedScrollView | ||
| ? { | ||
| paddingTop: translateY.get(), | ||
| } | ||
| : { | ||
| // On non-inverted scroll views we use bottom to ensure that content is displayed above the context menu / keyboard | ||
| bottom: translateY.get(), | ||
| }, | ||
| [isInvertedScrollView], | ||
| ); |
There was a problem hiding this comment.
The animatedStyle function is recreating a new object on every render due to the conditional logic inside the style calculation. This violates [PERF-4] which requires memoizing objects passed as props. Consider using two separate memoized style objects and conditionally selecting between them:
const invertedStyle = useAnimatedStyle(() => ({ paddingTop: translateY.get() }), []);
const normalStyle = useAnimatedStyle(() => ({ bottom: translateY.get() }), []);
const animatedStyle = isInvertedScrollView ? invertedStyle : normalStyle;This approach ensures that the style objects are properly memoized and only recreated when their dependencies change.
| if (item.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) { | ||
| return CONST.REPORT.ACTIONS.TYPE.CREATED; | ||
| } | ||
|
|
||
| return item.reportActionID; |
There was a problem hiding this comment.
The keyExtractor function now returns a constant string CONST.REPORT.ACTIONS.TYPE.CREATED for all created actions, which could lead to key collisions if multiple created actions exist in the list. React requires unique keys for optimal rendering performance and to prevent unexpected behavior. Consider appending a unique identifier (like a timestamp or index) to ensure uniqueness:
if (item.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) {
return `${CONST.REPORT.ACTIONS.TYPE.CREATED}-${item.created || Date.now()}`;
}| const getRenderScrollComponentWithAnimationProps = useCallback( | ||
| (render: NonNullable<RenderScrollComponentType>) => (props: ScrollViewProps) => | ||
| render({ | ||
| ...props, | ||
| isInvertedScrollView: !isTransactionThreadReport, | ||
| }), | ||
| [isTransactionThreadReport], | ||
| ); |
There was a problem hiding this comment.
The getRenderScrollComponentWithAnimationProps function creates a new function on every render, which violates [PERF-4] requiring memoization of functions passed as props. This could cause unnecessary re-renders of child components. The function itself is properly memoized with useCallback, but it returns a new function that isn't memoized. Consider restructuring this to avoid creating a new function on each render:
const renderScrollComponentWithProps = useCallback(
(props: ScrollViewProps) => {
if (!renderScrollComponent) return undefined;
return renderScrollComponent({
...props,
isInvertedScrollView: !isTransactionThreadReport,
});
},
[isTransactionThreadReport, renderScrollComponent]
);Then use renderScrollComponentWithProps directly in your component.
This comment has been minimized.
This comment has been minimized.
|
Generally this is feeling great! I found one case that I can reproduce fairly often, when a single expense is a one-expense report, when I open it from Reports > Expenses, sometimes you quickly see the empty report empty state and then the report loads but it's not docked to the top. Video: CleanShot.2025-09-12.at.17.55.36.mp4 |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
@pac-guerreiro Can we please fix the conflicts here? |
|
Nice, I think it resolved the issues we were having before! |
…transaction-thread-on-top # Conflicts: # src/components/ActionSheetAwareScrollView/index.ios.tsx # src/components/ActionSheetAwareScrollView/index.tsx # src/components/ActionSheetAwareScrollView/types.ts # src/components/ActionSheetAwareScrollView/useActionSheetKeyboardSpacing.ts # src/pages/home/report/ReportActionsList.tsx
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 1fe9b8d: Merge branch 'main' into pac-guerreiro/fix/63253-after-revert-1-open-transaction-thread-on-top
Conflicts:
src/components/ActionSheetAwareScrollView/index.ios.tsx
src/components/ActionSheetAwareScrollView/index.tsx
src/components/ActionSheetAwareScrollView/types.ts
src/components/ActionSheetAwareScrollView/useActionSheetKeyboardSpacing.ts
src/pages/home/report/ReportActionsList.tsx
Files Processed (2)
- src/components/ActionSheetAwareScrollView/index.ios.tsx (2 hunks)
- src/components/ActionSheetAwareScrollView/types.ts (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
src/components/ActionSheetAwareScrollView/index.ios.tsx [27-36]
performance: "Animated style is recreated on every render"
-
src/components/ActionSheetAwareScrollView/index.ios.tsx [9-9]
best practice: "Missing default value for optional prop"
-
src/components/ActionSheetAwareScrollView/types.ts [4-7]
performance: "Prop value should be memoized by parent components"
|
Looking good! I saw one instance of the empty state flash before loading, but I can't reproduce it now. My issue with some expenses not loading at the top is gone in the latest build—woohooooo. |
…oney request reports
…transaction-thread-on-top
|
@c3024 I'm not able to reproduce the issue you mentioned anymore, could you confirm? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppthreadAndroid.movAndroid: mWeb ChromethreadAndroidmWeb.moviOS: HybridApp
threadiOS3.moviOS: mWeb SafarithreadiOSmWeb1.movthreadiOSmWeb2.movMacOS: Chrome / SafariUploading threadChrome.mov… threadChrome1.movthreadChrome3.movMacOS: Desktop |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
thanks! Great effort, lets hope we can pull this one through! Seems like all the previously found issues been resolved.
cc @adamgrzybowski as this might influence some things in your PR
| (offset: number, animated = true) => { | ||
| if (!flatListRef?.current) { | ||
| return; | ||
| } | ||
|
|
||
| flatListRef.current.scrollToOffset({animated: true, offset}); | ||
| flatListRef.current.scrollToOffset({animated, offset}); |
There was a problem hiding this comment.
NAB: I guess it would make sense to leave a short comment to explain the difference between the native and web
| const {initialHeight} = useInitialWindowDimensions(); | ||
| const flatListVerticalOffset = useRef(0); | ||
| const prevFlatListVerticalOffset = useRef(0); | ||
| // initialHeight - windowHeight gives us the height of the keyboard when it's open on mobile Chrome. |
There was a problem hiding this comment.
| // initialHeight - windowHeight gives us the height of the keyboard when it's open on mobile Chrome. | |
| // initialHeight - windowHeight gives us the height of the keyboard when it's open on mobile Chrome. |
|
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
all the tests been passing afaik |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.16-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.2.16-14 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.17-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.2.17-6 🚀
|
8 participants
Explanation of Change
This is the second attempt after the first PR was reverted.
We needed to split the changes from the original PR into smaller, more manageable PRs.
In this one, we aim to ensure that when opening the transaction thread, the view scrolls to the top by default rather than to the bottom
With fix:
478002387-ecb76fe7-c6cb-4c5a-adc3-4dd2744b991a.mp4
On staging:
478002409-1f1e9984-329d-4ca5-a2e6-21e465d0048d.mp4
Fixed Issues
$#63253
PROPOSAL:
Tests
Regression tests:
$ #69701
$ #69761
$ #69758
This needs to be tested on both
Android - ChromeandiOS - Safariplatforms.Offline tests
Same as tests.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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_Native.mp4
Android: mWeb Chrome
Android_Chrome.mp4
iOS: Native
iOS_Native.mp4
iOS: mWeb Safari
iOS_Safari.mp4
MacOS: Chrome / Safari
MacOS_Chrome.mp4
MacOS: Desktop
MacOS_Desktop.mp4