-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Troubleshoot menu to PublicScreens #56926
Add Troubleshoot menu to PublicScreens #56926
Conversation
0c8832c
to
581179a
Compare
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-app-2025-02-17_17.43.05.mp4Android: mWeb ChromeN/A iOS: Nativeios-app-2025-02-17_17.49.38.mp4iOS: mWeb SafariN/A MacOS: Chrome / Safaridesktop-chrome-2025-02-17_17.10.11.mp4MacOS: Desktopdesktop-app-2025-02-17_17.20.51.mp4 |
@OlimpiaZurek Looking good so far! Just a quick question, how were you able to simulate the four-finger tap on iOS? |
This comment was marked as resolved.
This comment was marked as resolved.
I wasn’t able to simulate the four-finger tap, it works after pressing Cmd+D, just like in browsers. |
I couldn’t reproduce it on my end, but it’s possible that some options should be disabled before logging in. I’ll take a look and see which ones don’t make sense to have active here. |
aa55641
to
a79f87b
Compare
src/components/TestToolMenu.tsx
Outdated
/> | ||
</TestToolRow> | ||
)} | ||
|
||
{/* When toggled the app will be forced offline. */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed by accident, reverted
src/components/TestToolMenu.tsx
Outdated
{/* Option to switch between staging and default api endpoints. | ||
This enables QA, internal testers and external devs to take advantage of sandbox environments for 3rd party services like Plaid and Onfido. | ||
This toggle is not rendered for internal devs as they make environment changes directly to the .env file. */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
src/components/TestToolMenu.tsx
Outdated
disabled={!!isUsingImportedState || !!network?.shouldSimulatePoorConnection || network?.shouldFailAllRequests} | ||
/> | ||
</TestToolRow> | ||
|
||
{/* When toggled the app will randomly change internet connection every 2-5 seconds */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
src/components/TestToolMenu.tsx
Outdated
disabled={!!isUsingImportedState || !!network?.shouldFailAllRequests || network?.shouldForceOffline} | ||
/> | ||
</TestToolRow> | ||
|
||
{/* When toggled all network requests will fail. */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
src/components/TestToolsModal.tsx
Outdated
@@ -25,7 +25,6 @@ function TestToolsModal() { | |||
const StyleUtils = useStyleUtils(); | |||
const styles = useThemeStyles(); | |||
const {translate} = useLocalize(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you revert this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -74,6 +74,9 @@ type BackToAndForwardToParms = { | |||
forwardTo?: Routes; | |||
}; | |||
|
|||
type ConsoleNavigatorParamList = { | |||
[SCREENS.CONSOLE_DEBUG]: undefined; | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}; | |
}; | |
const route = useRoute<PlatformStackRouteProp<SettingsNavigatorParamList, typeof SCREENS.SETTINGS.CONSOLE>>(); | ||
const route = useRoute<PlatformStackRouteProp<SettingsNavigatorParamList, typeof SCREENS.CONSOLE_DEBUG>>(); | ||
const [session] = useOnyx(ONYXKEYS.SESSION); | ||
const isAuthenticated = useMemo(() => !!(session?.authToken ?? null), [session]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should create a useIsAuthenticated
hook for this? I'm seeing this exact piece of code in several files now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
8ebb782
to
a528005
Compare
a528005
to
4dfeace
Compare
@jjcoffee I've made few changes. Can you take a look and review again? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, LGTM
src/components/TestToolMenu.tsx
Outdated
This enables QA, internal testers and external devs to take advantage of sandbox environments for 3rd party services like Plaid and Onfido. | ||
This toggle is not rendered for internal devs as they make environment changes directly to the .env file. */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you revert the spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I'll leave it up to the assigned engineer on whether this needs to be sorted.
We did not find an internal engineer to review this PR, trying to assign a random engineer to #56925 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving again to get the auto-assigned engineer!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
The order has changed due to simplified logic, as some options are now hidden behind the isAuthenticated flag. I don’t think this will be an issue once these changes are deployed. |
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.1.5-0 🚀
|
This PR is failing for iOS and Android because of issue #57350 |
@IuliiaHerets I can repro the same issue before this PR's changes. |
@jjcoffee Yep, issue is repro on prod too. But PR asks to check if a menu is correctly displayed. In this case, PR is fail |
Revert "Merge pull request #56926 from callstack-internal/feat/add-tr…
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.1.5-5 🚀
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.1.6-0 🚀
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.1.6-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.1.6-1 🚀
|
Explanation of Change
This PR adds the Troubleshoot menu to the sign in page to be accessible in browsers (CMD+D) and Standalone App (four-finger tap).
Some options are unnecessary on the Sign In page, so they've been disabled.
Additionally, to ensure the Debug Console modal works properly, it also had to be accessible from the
PublicScreen
. I extracted it from the Settings screen to keep it public while hiding other options with theisAuthenticated
flag.Fixed Issues
$ #56925
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen_Recording_20250217-141443_New.Expensify.Dev.mp4
Android: mWeb Chrome
N/A
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.-.2025-02-17.at.14.18.00.mp4
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
Screen.Recording.2025-02-17.at.14.17.36.mov
MacOS: Desktop