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 3 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.
2 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
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate your change worked.
Suggested steps:
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas they need to test for regressions.
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