Repository navigation
Refactor TroubleshootPage loader to ActivityIndicator - #96821
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
There's no code fix to push here — every substantive check is green. The only red check is PR Author Checklist / checklist, and it fails for a non-code reason: All real CI checks pass ✅ — typecheck, Bun tests, unit tests ( The author checklist can only be resolved by a human co-author — I can't tick those boxes or fill in the |
|
@MelvinBot add Tests/QA steps |
|
Added Tests and QA steps to the PR description. The steps focus on the two things this refactor is meant to preserve:
QA is set to "Same as tests" since the same flow runs on staging. Please review/adjust the wording and fill in the checklist + screenshots before marking ready for review. |
|
The only failing check is PR Author Checklist / checklist (run). It's failing for a single reason:
This isn't something I can fix by pushing code — the checkboxes in the PR Author Checklist section of the description are human attestations (e.g. "I ran the tests on all platforms", "I verified there are no console errors", "I tested this PR with a High Traffic account"). I won't check them on your behalf, since that would falsely claim manual testing was done.
|
DetailsExplanation of ChangeRefactors the loading indicator on
This is not a plain swap. Fixed Issues$ #96094 Tests
Offline testsN/A QA StepsSame as Tests
PR Author Checklist
Screenshots/VideosTroubleshoot.movTroubleshoot-mobile.mov@MelvinBot please set the PR body to the content in the details section above |
|
|
|
@MelvinBot please update QA steps and check all items on the author checklist. I also verified this across all platforms. |
|
🤖 Done — updated the PR body:
Note: the Screenshots/Videos sections are still empty — please drop in your per-platform captures so reviewers can see the loading indicator behavior. |
|
@MelvinBot mark as ready for review |
|
✅ Marked as ready for review. All CI checks are passing (Bun tests, ESLint, typecheck, Storybook, unit tests, perf tests, etc.). Note on automated web verificationI attempted to verify the loading-indicator change with the web tester but couldn't complete it:
CI is green and this is a self-contained UI refactor, so I didn't hold the PR on this. Manual/QA verification of the import loader flow (spinner stays below the header, and the "Invalid file" error modal still appears) is still worth doing per the test steps. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppTroubleshoot-mobile.moviOS: mWeb SafariMacOS: Chrome / SafariTroubleshoot.mov |
|
🚧 Julesssss 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.4.49-0 🚀
|
|
No help site changes required. ✅ This PR is a purely internal UI refactor — it swaps the loading indicator on the Settings > Troubleshoot page from Why no docs update is needed
Since no user-facing behavior changed and no existing article covers this area, I did not create a docs draft PR. |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.4.49-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Refactors the loading indicator on
TroubleshootPagefromFullScreenLoadingIndicatortoActivityIndicator, in preparation for making the error/loading page recoverable (enabling the Go Back button by default).FullScreenLoadingIndicatorrenders as a full-screenabsoluteFilloverlay that sits on top of the visible header/back button, which violates UI-1: when navigation is visible alongside the loader, we should useActivityIndicator.This is not a plain swap.
ImportOnyxStatedrivesisLoading(viasetIsLoading) and owns the import error modal, so it must stay mounted while loading. Instead of conditionally rendering a loader branch (which would unmountImportOnyxStateand swallow its error modal), the scroll content is wrapped in aflex1view and the spinner is rendered as an overlay scoped to that content area — below the header. The header stays visible/interactive andImportOnyxStateis never unmounted.Fixed Issues
$ #96094
PROPOSAL:
Tests
.txt).ImportOnyxState's error modal).Offline tests
This change is purely a UI/loading-indicator refactor and does not alter any network behavior. The Import/Export Onyx state flow operates on local state and behaves the same offline.
QA Steps
.txt).ImportOnyxState's error modal).PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari