Scroll forward immediately on approve/pay/submit instead of after button animation - #98515
Scroll forward immediately on approve/pay/submit instead of after button animation#98515MelvinBot wants to merge 3 commits into
Conversation
…or button animation Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
| * Once the current action-badge target is resolved (e.g. the user approves/pays/submits an older report preview), the badge target | ||
| * advances to the next report preview that requires action. This hook follows it by scrolling down to the new target immediately, so | ||
| * the scroll starts as soon as the action is taken (on click / optimistically) rather than waiting for the resolve animation to | ||
| * finish. |
There was a problem hiding this comment.
Simplified the hook's JSDoc down to two lines in 6e5b187 — same meaning, less verbose.
| // Keep the latest scroll callback in a ref so a scroll scheduled on the next frame still targets the current badge index. | ||
| // The effect below only re-runs when the target id changes, so without this the scheduled callback would close over a stale | ||
| // target index if the list shifts (new message, pagination, resolved preview collapsing) before the frame runs. |
There was a problem hiding this comment.
Trimmed the ref comment from three lines to two in 6e5b187, keeping the "why" (stale index if the list shifts before the frame runs).
| // Scroll to the next target immediately (on the next frame) so the forward-scroll starts as soon as the user acts, rather | ||
| // than waiting for the submit/approve/pay resolve animation to finish. The resolved preview keeps playing its success | ||
| // animation in place while the list scrolls to the next actionable preview. |
There was a problem hiding this comment.
Condensed the scroll comment to two lines in 6e5b187.
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
The one real check failure here is unrelated to this PR — I'm not pushing a fix for it. Failing test: Why it's unrelated: this PR only touches four inbox-report auto-scroll files ( Why it's flaky: the test seeds The remaining red checks ( I'd recommend re-running the failed |
|
🤖 Addressed the review feedback — simplified all three comments in
Each kept its meaning and its "why", just tightened. Formatted and lint-clean. Replied in each thread as well. |
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.
|
|
Update: the flaky test failure has cleared on its own. The only remaining red checks are |
|
🤖 Already handled — all three comments in |
|
Good news — the failure cleared on its own. The latest CI run has all real checks green, including No fix was needed. The only remaining red checks are the human-gated |
|
@MelvinBot pull main |
…mmediatelyOnApprove Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
🤖 Merged
The branch is now up to date with |
|
The failing Failing test: Why it's unrelated:
Root cause (for whoever owns this test)The test's Navigation mock stubs only The other two red checks — |
|
Refining my earlier analysis now that I understand why Failing test: Why it fails only here (verified):
Root cause of the latent flake (the actual bug in the sign-in test)
So when a prior suite in the same shard/worker leaves Fix (belongs in a separate PR to the sign-in test, not this one): add The other two red checks — I can open that separate PR to harden |
|
@MelvinBot no scroll animation on native |
|
Native already scrolls without animation — the badge-follow scroll relies on the platform-split scroll manager, which jumps instantly on native and animates only on web. So no code change is needed for that behavior, but I can make it explicit if you'd prefer. Why native doesn't animate today
If you'd still like it stated explicitly in code (e.g. an explicit |
DetailsExplanation of ChangeWhen a user approves/pays/submits an older report preview from the workspace chat, the auto-scroll that follows the action badge to the next actionable preview was waiting for the resolve button animation (submit/approve/pay) to finish before scrolling. That delay made the forward-scroll feel laggy. This change makes the forward-scroll fire immediately on click (on the next frame, driven by the optimistic badge-target advance) instead of waiting for the animation. The resolved preview keeps playing its success animation in place while the list scrolls to the next actionable preview. Since the animation wait was the only reason Files changed:
Fixed Issues$ #96427 Tests
Offline testsSame as Tests QA StepsSame as Tests
PR Author Checklist
Screenshots/Videos@MelvinBot please set the PR body to the content in the details section above |
|
🤖 Updated the PR body to the content you provided. |
|
@MelvinBot mark as ready for review |
|
@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] |
|
🤖 Marked this PR as ready for review. Note: the |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #96427 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
The failing check is unrelated to this PR — it's a pre-existing breakage on Failing check: Root cause: A merge collision between two PRs that each passed independently on
Both the The fix belongs on The other failing check, |
|
The failing checks are not related to this PR — there's nothing to fix on this branch.
Since none of these stem from this PR's changes, I haven't pushed anything. |
|
I re-checked all four failing checks — none are caused by this PR, so there's no fix to push here. (The branch head is unchanged since my last comment, so these are the same runs.)
Since none of these stem from this PR's changes, I haven't pushed anything. |
|
I re-analyzed every failing check (five now, including the new Three are infrastructure flakes —
Per policy I don't push unrelated/pre-existing test fixes into an unrelated PR — doing so would mix a signin-navigation change into a scroll PR. The actual fix is a one-liner on |
Explanation of Change
When a user approves/pays/submits an older report preview from the workspace chat, the auto-scroll that follows the action badge to the next actionable preview was waiting for the resolve button animation (submit/approve/pay) to finish before scrolling. That delay made the forward-scroll feel laggy.
This change makes the forward-scroll fire immediately on click (on the next frame, driven by the optimistic badge-target advance) instead of waiting for the animation. The resolved preview keeps playing its success animation in place while the list scrolls to the next actionable preview.
Since the animation wait was the only reason
getActionBadgeScrollDelayand theactionBadge/prevActionBadgeplumbing existed, they are removed anduseFollowActionBadgeTargetnow always schedules the scroll on the nextrequestAnimationFrame. The direction/should-follow logic (shouldFollowActionBadgeTarget) is unchanged, so the scroll still only follows the badge downward to newer actionable previews.Files changed:
src/pages/inbox/report/useFollowActionBadgeTarget.ts— always scroll on the next frame; dropped the animation-delay branch and the now-unusedactionBadgeparam.src/pages/inbox/report/getActionBadgeScrollDelay.ts— deleted (only existed to compute the animation wait).tests/unit/getActionBadgeScrollDelayTest.ts— deleted alongside the helper.src/pages/inbox/report/ReportActionsList.tsx— stopped passing the removedactionBadgeparam.Fixed Issues
$ #96427
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos