Skip to content

Raise test tools and the offline banner above required 2FA. - #102955

Open
neerajbachani wants to merge 10 commits into
Expensify:mainfrom
neerajbachani:fix/102574-test-tools-above-required-2fa
Open

neerajbachani wants to merge 10 commits into
Expensify:mainfrom
neerajbachani:fix/102574-test-tools-above-required-2fa

Conversation

@neerajbachani

@neerajbachani neerajbachani commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

The required-2FA overlay is drawn after RootStack.Navigator at z-index 20, so it covers the test-tools route and the offline banner that ScreenWrapper paints inside those screens. Cmd/Ctrl+D and the four-finger tap still navigate to test tools, but that screen stays underneath the overlay.

This keeps the overlay above normal app UI, including onboarding and the welcome modal. Two things move above it, and only while the overlay is showing:

  1. Offline banner. The overlay renders the existing OfflineIndicator in a bottom-aligned layer at z-index 21. The home-indicator inset sits on the container around that row, not on the row itself. The row is a fixed height, so padding on the same view clips the label on native and leaves only the cloud icon. The banner is absent when the device is online, so normal screens keep their own banner and there is no second copy.
  2. Test tools. A portal host at z-index 22 sits after the overlay. The nested test-tools stack stays in the root navigator, so the URL, backTo, the Server page, and goBack keep working. Only the focused screen is portaled, with its navigation, route, and card-animation contexts put back. Portaling the whole tree dropped those contexts, and Overlay then threw "Couldn't find values for card animation", which the error boundary showed as "Uh-oh, something went wrong." Web and Android use that portal. Android's transparent modal stays inside the stack, under the overlay, so leaving the screen in place keeps the tools covered. iOS already presents this navigator in its own window above the overlay, so the screen stays in that modal. Portaling it out left an empty opaque window that covered the tools once the modal transition finished. The nested native screen background is transparent so the parent dim still shows. While test tools are focused, the overlay's Enable 2FA Enter shortcut is hidden. The overlay sits outside a screen, so that shortcut otherwise stays active and Enter starts 2FA setup.

The overlay stays mounted underneath test tools. On web, its focus trap turns off while the test-tools route is focused so the portaled tools can take input. Enable, Sign out, and the onboarding path snapshot are unchanged. When required 2FA is off, or the user is inside 2FA setup, nothing is portaled.

Fixed Issues

$ #102574
PROPOSAL:

Tests

  1. Log in as a user who must complete domain-required 2FA and wait for the required-2FA screen.
  2. On desktop, press Cmd+D or Ctrl+D. Verify test tools open above that screen, including Force offline, and the page does not show "Uh-oh, something went wrong."
  3. On a phone, including Android native, use the four-finger tap on the required-2FA screen. Verify test tools open above that screen and can be used.
  4. On iOS native, open test tools from the dev menu (Open Test Preferences). Verify the tools stay visible and are not covered by a white screen after they open. Open Server, then go back, and verify the push does not show a see-through sheet.
  5. On Android native, open test tools from the dev menu. Verify they sit above the required-2FA screen, the area around the sheet is dimmed, and tapping that dim closes the tools.
  6. In test tools, open Server, then go back. Verify the Server page opens and returns to the test-tools list, still above the required-2FA screen.
  7. While test tools are open, press Enter. Verify 2FA setup does not start.
  8. Close test tools with Escape, Cmd/Ctrl+D, and by clicking outside the modal. Verify the required-2FA screen is still there, and Enable and Sign out still work.
  9. From a normal screen, with required 2FA not showing, press Cmd/Ctrl+D. Verify test tools open and close as before, including Server and back.
  10. Verify that no errors appear in the JS console
  • Verify that no errors appear in the JS console

Offline tests

  1. With the required-2FA screen open and test tools closed, disconnect the device from the network. Verify the full "You appear to be offline" label is visible along the bottom, above the required-2FA screen, including on iOS where the home indicator would otherwise clip the text and leave only the cloud icon.
  2. Reconnect. Verify that banner disappears and the required-2FA screen stays up.
  3. Open test tools above the required-2FA screen and turn on Force offline. Verify the same banner appears above the required-2FA screen. Turn Force offline off and verify the banner goes away.
  4. On a normal screen, disconnect the network. Verify that screen still shows its own offline banner and a second banner does not appear.

QA Steps

Same as tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified any copy / text that was added to the app is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Screen.Recording.2026-10-13.at.12.00.00.PM.mov
Android: mWeb Chrome
WhatsApp.Video.2026-10-13.at.12.07.34.mp4
iOS: Native
Screen.Recording.2026-10-12.at.11.32.21.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-10-12.at.10.40.10.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-10-12.at.10.10.32.AM.mov

Portal only the focused test-tools screen so Cmd/Ctrl+D stays usable on that overlay without losing the stack context that was crashing the modal.
@neerajbachani
neerajbachani marked this pull request as ready for review October 9, 2026 05:12
@neerajbachani
neerajbachani requested review from a team as code owners October 9, 2026 05:12
@melvin-bot
melvin-bot Bot requested review from garrettmknight and thelullabyy and removed request for a team October 9, 2026 05:12
@melvin-bot

melvin-bot Bot commented Oct 9, 2026

Copy link
Copy Markdown

@thelullabyy 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]

@melvin-bot
melvin-bot Bot removed the request for review from a team October 9, 2026 05:12
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-09T05:28:20.169519Z e82b687 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@neerajbachani

Copy link
Copy Markdown
Contributor Author

@codex review

Comment thread tests/ui/RequireTwoFactorOverlayStackingTest.tsx
Comment thread src/pages/RequireTwoFactorAuthenticationOverlay.tsx Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e82b687355

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

Comment thread tests/unit/hooks/useRequire2FAOverlayVisibility.test.ts
Document the overlay banner and test setup, and dim the portaled test-tools frame on native so the backdrop is not left under the overlay.
@thelullabyy

thelullabyy commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified any copy / text that was added to the app is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-10-18.at.14.36.29.mov

Portaling the tools out of the native modal left an empty white screen over them, and safe-area padding on the fixed-height offline row clipped the text.
CSpell rejects "Portaling" in the native test-tools comments.
@neerajbachani

Copy link
Copy Markdown
Contributor Author

@thelullabyy PR is ready for review

@thelullabyy

Copy link
Copy Markdown
Contributor

@MelvinBot Deep review

@MelvinBot

Copy link
Copy Markdown
Contributor

Deep review

The web fix looks sound: the portal brings back the contexts the test-tools pages need, and the host stacks above the overlay. Two test steps likely still fail, though, so I'd request changes before approving.

Should fix

  1. Android native: test tools probably still render under the overlay. The web portal is skipped on every platform except web (TestToolsModalNavigator.tsx:99, :155). That relies on the native transparentModal drawing above the overlay. iOS presents it in a separate view controller (OverFullScreen), so iOS is fine. On Android, react-native-screens 4.28 keeps transparentModal as a fragment inside the ScreenStack view. The overlay is a later sibling with zIndex 20, so it should still paint over the tools and take their touches. The native UI test only sets Platform.OS = 'ios'. Please confirm on an Android emulator. If it fails, Android needs a fix, for example the portal path, or hiding the overlay while test tools are focused.

  2. The four-finger tap can't open test tools while the overlay is showing (test steps 3–4). The gesture's only handler is a PanResponder on ScreenWrapperContainer, inside the root-stack screens (ScreenWrapperContainer.tsx:164-169). The overlay is a full-screen sibling outside that tree (RequireTwoFactorAuthenticationOverlay.tsx:125-128). So touches on the overlay never reach that capture handler, on native or mWeb. Cmd/Ctrl+D and the native dev menu still work. To fix it, attach the same pan handlers to the overlay root.

Worth addressing

  1. Enter can trigger Enable 2FA from inside test tools (web). The overlay's Enable button keeps <Button.KeyboardShortcut /> (:158). It sits outside any screen, so withNavigationFallback reports it as focused, and the shortcut stays active (ButtonKeyboardShortcut/index.tsx:51). If the focused element in the tools has no ARIA role, pressing Enter navigates to 2FA setup. Gate the shortcut on !isTestToolsRouteFocused, the same way the PR already gates the focus trap.

  2. Flipping isRequire2FAOverlayVisible remounts the nested navigator. :155-159 switches between returning stack and <TestToolsModalFrame>{stack}</TestToolsModalFrame>. The parent element type changes, so Stack.Navigator remounts and nested state resets (Server goes back to Root). Each screen also swaps between <Screen> and <Portal> (lines 99–121). This happens if the overlay's visibility changes while the tools are open, for example Onyx loading late on a cold load of the test-tools URL. Keep the tree shape stable, for example by always rendering the frame and making it a pass-through when portaled.

  3. The dim animation changed on web. Overlay now reads the nested stack's per-screen CardAnimationContext (:113), not the root modal card's. Based on the code:

    • On open, the dim appears at full opacity with no fade.
    • Going from Root to Server, the dim drops to 0 and fades back in.
    • On close, the tools disappear instantly.

    This is cosmetic only, but a reviewer will notice it.

Nits

  • Platform branching uses Platform.OS === 'web' inline (:99, :155). The App standards prefer .web.tsx / .native.tsx files for this.
  • backgroundColor: 'transparent' (:140) should use theme.transparent.
  • REQUIRE_2FA_FOCUSED_SCREEN and getRequire2FAFocusedScreen are exported only for tests.
  • The UI test mocks useThemeStyles, PortalHost, and the navigator, and never renders AuthScreens. Nothing checks the actual z-index stacking. The unit test uses only a flat navigation state, not the real nested root → navigator → screen shape.
What I checked and found fine
  • Contexts: neither test-tools page uses ScreenWrapper. Their navigation hooks (useIsFocused, useRoute, useFocusEffect, useCardAnimation) only need the three contexts the portal re-provides. App-level contexts come from providers above the PortalHost.
  • Portal: PortalProvider is mounted globally, and the host name is unique. zIndex order is 20 (overlay), 21 (banner), 22 (host). The MFA navigator still sits above the host.
  • useNavigation() in the hook falls back to the container ref outside a screen, the same as the old overlay code.
  • Offline banner: OfflineIndicator hides itself when online. The safe-area args put the inset on the container, as the description says. There is no double banner, because TestToolsScreenWrapper renders no offline indicator.
  • Transparent contentStyle: the native card stays opaque, because TestToolsScreenWrapper applies defaultModalContainer, which sets componentBG.
  • Tests: every case has Given/When/Then comments.

I reviewed the code only. I didn't run the tests or the React Compiler check, and I didn't test on a device. Items 1 and 2 need an Android, iOS, or mWeb run to confirm.


view run

@neerajbachani

Copy link
Copy Markdown
Contributor Author

Two of these are fixed in 4f36638.

Android test tools now use the same portal as web (Platform.OS !== 'ios'). Android's transparent modal stays inside the stack, so the overlay was still covering an in-place screen. iOS stays in the modal. Moving that screen into the portal host leaves an empty window that covers the tools after the transition.

The Enable 2FA Enter shortcut is not rendered while the test-tools route is focused. The overlay sits outside a screen, so that shortcut otherwise stays active.

Four-finger tap on Android mWeb already opens test tools while required 2FA is showing, so I did not add a second responder.

I left the navigator remount and the web dim animation. The remount only happens if required-2FA visibility flips while tools are already open. Opening tools from the overlay does not flip that flag. The dim follows the nested card animation after the portal, and it does not block the tools.

Comment thread src/pages/RequireTwoFactorAuthenticationOverlay.tsx
Comment thread src/pages/RequireTwoFactorAuthenticationOverlay.tsx Outdated
@neerajbachani

Copy link
Copy Markdown
Contributor Author

@thelullabyy The PR is ready for another review. Thanks!!

@thelullabyy

Copy link
Copy Markdown
Contributor

@neerajbachani Could you please recheck this point, I don't see the fix added yet? #102955 (comment)

@thelullabyy thelullabyy left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I left some inline comments. The main ones are the Android dim, the web card animation, and the navigator remount.

A few other things:

  • tests/ui/RequireTwoFactorOverlayStackingTest.tsx mocks the navigator, the portal and nearly every hook, so it mostly checks testIDs rather than real stacking and wouldn't catch the issues above. Fine as a smoke test, but please don't rely on it alone.
  • The PR description still has // TODO: These must be filled out… under QA steps

Comment thread src/libs/Navigation/AppNavigator/Navigators/TestToolsModalNavigator.tsx Outdated
Comment thread src/libs/Navigation/AppNavigator/Navigators/TestToolsModalNavigator.tsx Outdated
Comment thread src/libs/Navigation/AppNavigator/Navigators/TestToolsModalNavigator.tsx Outdated
Comment thread src/libs/Navigation/AppNavigator/Navigators/TestToolsModalNavigator.tsx Outdated
Comment thread src/pages/RequireTwoFactorAuthenticationOverlay.tsx Outdated
Comment thread src/hooks/useRequire2FAOverlayVisibility.ts
@codecov

codecov Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
src/hooks/useRequire2FAOverlayVisibility.ts 100.00% <100.00%> (ø)
src/hooks/useTestToolsTapResponder.ts 100.00% <100.00%> (ø)
src/libs/Navigation/AppNavigator/AuthScreens.tsx 84.46% <100.00%> (+0.62%) ⬆️
...on/AppNavigator/Navigators/TestToolsLiftContext.ts 100.00% <100.00%> (ø)
...ppNavigator/Navigators/TestToolsModalNavigator.tsx 100.00% <100.00%> (+93.33%) ⬆️
...avigators/TestToolsPortalBackdrop/index.native.tsx 100.00% <100.00%> (ø)
...rc/pages/RequireTwoFactorAuthenticationOverlay.tsx 88.23% <100.00%> (+39.75%) ⬆️
src/styles/index.ts 66.22% <ø> (ø)
...gator/Navigators/TestToolsPortalBackdrop/index.tsx 0.00% <0.00%> (ø)
...rs/shouldLiftTestToolsAboveRequire2FA/index.ios.ts 0.00% <0.00%> (ø)
... and 2 more
... and 15 files with indirect coverage changes

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants