Fix flash of disabled feature page before redirect to More features - #98549
Fix flash of disabled feature page before redirect to More features#98549MelvinBot wants to merge 4 commits into
Conversation
Co-authored-by: truph01 <truph01@users.noreply.github.com>
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Fixed the failing Oxfmt check. The DetailsThe Oxfmt job runs - const shouldRedirectToMoreFeatures =
- isFocused && !isEmptyObject(policy) && !isFeatureEnabled && !(pendingField && !isOffline) && !shouldShowNotFoundPage;
+ const shouldRedirectToMoreFeatures = isFocused && !isEmptyObject(policy) && !isFeatureEnabled && !(pendingField && !isOffline) && !shouldShowNotFoundPage;No logic change — formatting only. The other red checks are not actionable code failures: |
|
All actionable checks are now green — the Oxfmt check that was failing has been fixed (formatting-only change, already pushed). The two checks still showing red are process gates, not code failures, and can't be fixed with a code push:
The remaining |
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Fixed the failing Details
The test policies were built from The fix adds The remaining red checks ( |
|
There's nothing left for me to fix here — the Oxfmt check I fixed in the previous run is now green. The only two remaining red checks are process/review gates that require human action, not code changes:
Once those are addressed by a person, all checks should pass. |
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.
|
|
@codex review |
|
No actionable check failures remain. The Oxfmt check is now passing after the earlier formatting fix. The two remaining red checks are process/review gates, not code failures, so there's nothing for me to push:
The performance tests ( |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@MelvinBot Mark this PR as ready for review |
|
🤖 Marked this PR as ready for review. Before flipping it, I verified the core fix on web: with a workspace that had Expensify Card enabled and Workflows disabled, navigating to the disabled Workflows settings page now redirects to More features without rendering (flashing) the disabled page — a Testing notes
Screenshots from the run are attached in a separate comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // The feature linked to this page is disabled, so the redirect effect above will navigate to the More Features page. | ||
| // Render a loader instead of the page's children so the disabled page is never shown for a frame (avoids a visible flash). | ||
| if (shouldRedirectToMoreFeatures) { | ||
| return <FullscreenLoadingIndicator />; |
There was a problem hiding this comment.
❌ UI-1 (docs)
This newly added FullscreenLoadingIndicator is returned from a conditional branch with no navigation component (no HeaderWithBackButton or close button) in the same return. If the redirect ever hangs, the user has no escape route because there is no visible navigation. Per UI-1, a full-screen loader rendered without accompanying navigation should set shouldUseGoBackButton so an emergency "Go Back" button appears after a timeout.
if (shouldRedirectToMoreFeatures) {
return <FullscreenLoadingIndicator shouldUseGoBackButton />;
}Reviewed at: 1bb089b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Addressed. The UI-1 suggestion is valid — this loader branch has no navigation, so if the microtask redirect ever hung the user would be stuck. Added shouldUseGoBackButton so the emergency "Go Back" button appears after a timeout:
if (shouldRedirectToMoreFeatures) {
return <FullscreenLoadingIndicator shouldUseGoBackButton />;
}In the normal fast-redirect path nothing changes (the button only shows after the timeout). ESLint passes on the file. Pushed in 2afe348.
I scoped the change to the new branch only; the pre-existing shouldShowFullScreenLoadingIndicator loader above it wasn't touched by this PR, so I left it as-is.
|
The only failing checks are PR Author Checklist and PR Reviewer Checklist — every substantive check (Bun tests, Both are failing by design, not because of a code problem:
There's no code fix to push here. These clear once a human completes the checklists:
So nothing is broken in the branch — the reds are just the unchecked author/reviewer checklists. |
|
Still the same as my earlier reply — nothing new is broken. The only two red checks are PR Author Checklist and PR Reviewer Checklist. Every substantive check (Bun tests, all 8 These two fail by design, not because of a code problem:
|
|
Still the same situation as my previous analysis: the only failing checks are PR Author Checklist and PR Reviewer Checklist. Every substantive check (Bun tests, all Both fail by design, not because of a code problem — the Author Checklist errors with To clear the reds, a human needs to:
|
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
@youssef-lr 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] |
|
Re-checked — the currently-failing checks are all ones that fail by design, not code problems:
The rest of the suite that has finished is green, and several checks (perf tests, ESLint, build) are still re-running from the latest push. There's nothing for me to fix in the branch. These three clear on their own once (1) the human co-author fills out the Tests/QA Steps sections and checks the author checklist, and (2) a reviewer approves and completes the reviewer checklist. |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 roryabraham 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! 🧪🧪
|
|
requested retest on the AdHoc build: https://expensify.slack.com/archives/C09V78U42D8/p1786669656109099 |
5 participants
Explanation of Change
The Workflows page (and any other feature-gated workspace page) briefly flashes before redirecting to More features when its feature is disabled. Repro: on a workspace with Expensify Card enabled but Workflows disabled, open Expensify Card → Issue card → Choose a limit type → tap Enable approvals. The link points at the Workflows page, but since Workflows is disabled the page is shown for a frame before navigating to More features.
Root cause:
AccessOrNotFoundWrappermounts the target page's children first and only redirects afterwards, from inside auseEffectthat further defers navigation to the microtask queue. So a disabled feature page renders for at least one frame beforegoBackfires — the visible flash. This is a property of the wrapper, so it isn't specific to the Expensify Card flow.Change: the redirect condition is extracted into a single
shouldRedirectToMoreFeaturesderived value, reused as the existinguseEffectguard (behavior unchanged, including thependingField/offline guard that protects against transient feature-state changes during workspace creation). During render, when that value is true, the wrapper now returns aFullscreenLoadingIndicatorinstead of the page's children, so the disabled page is never painted and there is no flash. This fixes the flash centrally for every feature-gated page routed through the wrapper.Fixed Issues
$ #98504
PROPOSAL: #98504 (comment)
Tests
Retest the linked issue
Offline tests
n/a
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))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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari