Repository navigation
[CP Staging] Fix: bring chart spinner back in the same render on reload - #105163
luacmartins merged 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@luacmartins @daledah this fixes the deploy blocker #105108 caused by #103210. It is a 1 line change in |
|
Thanks @sharabai for the PR, i'll review it in a couple of hours |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-10-28.at.00.46.21.movMacOS: Chrome / SafariScreen.Recording.2026-10-28.at.00.48.59.mov |
|
@luacmartins all yours |
…ghts-scroll-jump [CP Staging] Fix: bring chart spinner back in the same render on reload (cherry picked from commit 0b8e418) (cherry-picked to staging by jasperhuangg)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🔁 Filed a Staging retest request for deploy blockers #105108 after this PR was cherry-picked to staging. |
|
🚀 Cherry-picked to staging by https://github.com/jasperhuangg in version: 9.5.23-3 🚀
|
|
No help site update needed. This PR fixes a loading glitch where the Insights card briefly emptied and the Home page jumped when switching charts, which is an implementation detail that no article describes. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.23-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/jasperhuangg in version: 9.5.24-1 🚀
|
|
No help site update needed. This PR only changes how the Insights chart card shows its loading spinner when you switch charts, which is loading behavior that no article describes. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.24-8 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
A regression from #103210. When a chart that is already shown starts loading again without remounting, as when the Insights card switches from Top spenders to Top categories (both bar charts), the bar body renders nothing at once, but
ChartRevealonly learns about it from the chart's layout effect a commit later. For that commit the card is empty, and on Android Home's scroll view clamps to the shorter page.ChartFramenow passes itsisLoadingtoChartReveal, which counts it as loading, so the spinner comes back at full height in the same render.The jump only shows on an insight's first load after a sign-in or a cache clear, since an insight that is already cached renders without going back to loading.
Fixed Issues
$ #105108
PROPOSAL: N/A
Tests
Preconditions:
insightsPagebeta.1. Home stays on the Insights card when it switches insight (Android: Native, exercises the fix)
2. Chart loading on web (MacOS: Chrome, regression)
Offline tests
N/A: the change only affects how the chart card draws a load it already knows about, and offline adds no path to it.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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
Android: Native
Screen.Recording.2026-10-27.at.18.07.54.mov
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari