-
Notifications
You must be signed in to change notification settings - Fork 3.5k
No animation for Search report when opened as not the first screen in RHP v2 #79109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
No animation for Search report when opened as not the first screen in RHP v2 #79109
Conversation
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.
|
576c636 to
7b55f70
Compare
230c102 to
240b6c9
Compare
|
@ZhenjaHorbach 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] |
src/components/WideRHPContextProvider/useShowSuperWideRHPVersion/index.ts
Show resolved
Hide resolved
| const shouldRenderTertiaryOverlay = useShouldRenderOverlay(isRHPFocused && isWideRHPBelow && isSuperWideRHPBelow, thirdOverlayProgress); | ||
|
|
||
| /** | ||
| * Effect that shows/hides the expanded RHP progress based on the number of wide RHP routes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ PERF-6 (docs)
The useEffect on line 176 is updating expandedRHPProgress based on the lengths of superWideRHPRouteKeys and wideRHPRouteKeys arrays. However, since expandedRHPProgress is an Animated.Value (not React state), and the guard condition checks ref values, this pattern is acceptable for animation synchronization.
Actually, upon closer inspection, the guard added in lines 179-181 prevents updates during closing transitions by checking isWideRHPClosingRef.current || isSuperWideRHPClosingRef.current. This introduces timing dependencies where the effect behavior depends on ref state set by other hooks.
Consider deriving the target progress value directly when needed, or consolidating the closing state management into a single location to avoid scattered state dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to save info if the rhp is closing to the variable. The best place to do that is in the hook showing wide rhps.
I introduced this change as there is a moment when the wide rhp is removed from the state (useBeforeRemove is called then), but it's still closing. If we update expandedRHPProgress during this update, it will cause animation issues. That's why I use early return here and it should break [PERF-6] rule
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 240b6c9e95
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-01-09.10.41.38.movAndroid: mWeb Chrome2026-01-09.10.44.34.moviOS: HybridApp2026-01-09.10.27.39.moviOS: mWeb Safari2026-01-09.10.28.51.movMacOS: Chrome / Safari2026-01-09.10.21.46.mov |
|
Okay |
We'll try to figure it out, but for now we should go ahead with the Super Wide RHP :D We'll handle it as a followup |
mountiny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the AI reviewer comments, the changes look fine now and looking forwards completing the SuperWide logic and hopefully looking for some ways to simplify the logic then
240b6c9 to
6133d81
Compare
mountiny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to move this ahead, thanks for the simplification
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
cc: @mountiny @ZhenjaHorbach
Explanation of Change
This PR fixes the animation issues with
SearchReportpage. Previously this structure was more complex, this page was wrapped by the modal stack that was animated but the page doesn't have animations itself. Now this page is displayed directly inRightModalNavigator. We want to animate it when showing and closing but not when switching using arrow buttons. To do that we need to performSET_PARAMSaction instead ofREPLACEto not trigger animation.Now it's consistent with expense reports as on these pages
SET_PARAMSaction is triggered when pressing the arrow buttons.In this PR, the deploy blocker which appeared after merging v1 has been fixed. #78437
To avoid this animation glitch SearchReport won't be animated when opening from a narrow RHP on a wide layout.
Fixed Issues
$ #78340
$ #78437
PROPOSAL:
Tests
Offline tests
QA Steps
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))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
Screen.Recording.2026-01-08.at.18.42.15.mov
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-01-08.at.18.13.25.mov
iOS: mWeb Safari
Screen.Recording.2026-01-08.at.18.11.55.mov
MacOS: Chrome / Safari
Screen.Recording.2026-01-08.at.17.52.09.mov