chore: migrate AuthScreens from withOnyx to useOnyx #52417
Conversation
melvin-bot
bot
requested review from
deetergp
and removed request for
a team
|
@c3024 Ready for review |
| } | ||
|
|
||
| return ( | ||
| <AuthScreensMemoized |
There was a problem hiding this comment.
I see we usually fetch the data with useOnyx directly in the main function. This way of using another parent and passing the Onyx fetched values as props is new to me. Any benefit of doing this vis-a-vis fetching these values directly with useOnyx in the existing main AuthScreens function?
There was a problem hiding this comment.
I have followed this approach, @blazejkustra Has explained pretty neat here.
#49103 (comment)
There was a problem hiding this comment.
Thanks! Missed that comment.
Co-authored-by: Błażej Kustra <[email protected]>
|
Logging out and logging back into an account navigates to the Profile page instead of LHN and Report Central Pane. This is fine on staging. profilePageOpensOnLogin1.mp4profilePageOpensOnLogin.mp4 |
|
@BhuvaneshPatil any thoughts on the above comment? |
…til/App into 49103-onyx-migration
|
I spent some time on that not able to figure out what is root cause |
|
@BhuvaneshPatil Are you saying we need to close this PR and re-open the GH for other proposals? |
|
@BhuvaneshPatil if you could please let us know ^ |
|
@deetergp We can pull current C+ to take a closer look. |
|
@c3024 Do you have any thoughts on this? |
|
Can you please help here? With this suggestion, if we logout and log back into an account it navigates to Profile Settings page instead of a Report page as mentioned here. I see that we found two regressions with the previous PR #49185 I think the previous PR #49185 can be modified to fix these like (1) using a const isAnyOnyxValueStillLoading = isLoadingOnyxValue(sessionStatus, lastOpenedPublicRoomIDStatus, initialLastUpdateIDAppliedToClientStatus);
const areAllOnyxValuesLoadedFirstTime = useRef(true);
useEffect(() => {
if (!areAllOnyxValuesLoadedFirstTime.current || !isAnyOnyxValueStillLoading) {
return;
}
areAllOnyxValuesLoadedFirstTime.current = false;
.....
}, [isAnyOnyxValueStillLoading]); |
|
Can you try the changes specified above and see if you find any issue? |
Sorry, I was OOO last week. Let me take a look now 👀 |
|
The |
|
Hey @c3024, are you sure the Screen.Recording.2024-12-17.at.13.55.50.mp4 |
|
Yes, my mistake! |
@BhuvaneshPatil Let's just do this change and we should be good 🚀 |
|
I recently changed how initialReportID is created, so maybe that helped with the other issue @c3024. Now it uses useState: const [initialReportID] = useState(() => {
const currentURL = getCurrentUrl();
const reportIdFromPath = currentURL && new URL(currentURL).pathname.match(CONST.REGEX.REPORT_ID_FROM_PATH)?.at(1);
if (reportIdFromPath) {
return reportIdFromPath;
}
const initialReport = ReportUtils.findLastAccessedReport(!canUseDefaultRooms, shouldOpenOnAdminRoom(), activeWorkspaceID);
return initialReport?.reportID ?? '';
}); |
|
@BhuvaneshPatil Has any of this helped you resolve this one? |
|
@c3024 Screen.Recording.2024-12-19.at.7.53.58.AM.mov |
|
|
@c3024 If I understand correctly, it sounds like we don't need to hold this PR up any further then? |
|
No. This PR migrates withOnyx usage in AuthScreens to useOnyx. But, this migration is causing an issue where navigation to the profile page occurs upon login. I have not yet identified the root cause. I think someone from the expert agency group can help here. |
FYI, Software Mansion is OOO until January 7. We'll come back to this issue then |
|
Heya @blazejkustra Happy New Year! Is SWM back in action now? |
|
Hi @deetergp! This is something I wanted to check out today, but I had to finish a few other things. I will look into it on Monday. Thanks for your patience! 🙇 |
|
Monday sounds fine @adamgrzybowski, thanks! |
|
Hi, I'm from SWM and I took over this task from @adamgrzybowski, I'm investigating it now |
|
Hey, I'm still investigating it, I already have an idea how to fix it but I want to check it more thoroughly, I'll come back tomorrow with the next update |
|
Hey, I'm back with the next update :) I've already figured out what is probably wrong and how to fix it. There are two issues:
diff --git a/src/libs/actions/Session/index.ts b/src/libs/actions/Session/index.ts
index 11eab64a36d..f2e73a7981c 100644
--- a/src/libs/actions/Session/index.ts
+++ b/src/libs/actions/Session/index.ts
@@ -804,20 +804,11 @@ function clearSignInData() {
}
/**
- * Reset all current params of the Home route
+ * Reset navigation state after logout
*/
-function resetHomeRouteParams() {
+function resetNavigationState() {
Navigation.isNavigationReady().then(() => {
- const routes = navigationRef.current?.getState()?.routes;
- const homeRoute = routes?.find((route) => route.name === SCREENS.HOME);
-
- const emptyParams: Record<string, undefined> = {};
- Object.keys(homeRoute?.params ?? {}).forEach((paramKey) => {
- emptyParams[paramKey] = undefined;
- });
-
- Navigation.setParams(emptyParams, homeRoute?.key ?? '');
- Onyx.set(ONYXKEYS.IS_CHECKING_PUBLIC_ROOM, false);
+ navigationRef.resetRoot(navigationRef.getRootState());
});
}
@@ -836,7 +827,7 @@ function cleanupSession() {
PersistedRequests.clear();
NetworkConnection.clearReconnectionCallbacks();
SessionUtils.resetDidUserLogInDuringSession();
- resetHomeRouteParams();
+ resetNavigationState();
clearCache().then(() => {
Log.info('Cleared all cache data', true, {}, true);
});
~
~
~
~
~
~
~
diff --git a/src/libs/Navigation/AppNavigator/createResponsiveStackNavigator/CustomRouter.ts b/src/libs/Navigation/AppNavigator/createResponsiveStackNavigator/CustomRouter.ts
index 3cbb5acb81e..1928630c855 100644
--- a/src/libs/Navigation/AppNavigator/createResponsiveStackNavigator/CustomRouter.ts
+++ b/src/libs/Navigation/AppNavigator/createResponsiveStackNavigator/CustomRouter.ts
@@ -13,6 +13,7 @@ import {isCentralPaneName, isOnboardingFlowName} from '@libs/NavigationUtils';
import * as Welcome from '@userActions/Welcome';
import CONST from '@src/CONST';
import NAVIGATORS from '@src/NAVIGATORS';
+import ROUTES from '@src/ROUTES';
import SCREENS from '@src/SCREENS';
import syncBrowserHistory from './syncBrowserHistory';
@@ -40,13 +41,8 @@ function insertRootRoute(state: State<RootStackParamList>, routeToInsert: Naviga
}
function compareAndAdaptState(state: StackNavigationState<RootStackParamList>) {
- // If the state of the last path is not defined the getPathFromState won't work correctly.
- if (!state?.routes.at(-1)?.state) {
- return;
- }
-
// We need to be sure that the bottom tab state is defined.
- const topmostBottomTabRoute = getTopmostBottomTabRoute(state);
+ const topmostBottomTabRoute = getTopmostBottomTabRoute(state) ?? {name: SCREENS.HOME};
const isNarrowLayout = getIsNarrowLayout();
// This solutions is heuristics and will work for our cases. We may need to improve it in the future if we will have more cases to handle.
@@ -61,7 +57,9 @@ function compareAndAdaptState(state: StackNavigationState<RootStackParamList>) {
// We will generate a template state and compare the current state with it.
// If there is a difference in the screens that should be visible under the overlay, we will add the screen from templateState to the current state.
const pathFromCurrentState = getPathFromState(state, linkingConfig.config);
- const {adaptedState: templateState} = getAdaptedStateFromPath(pathFromCurrentState, linkingConfig.config);
+ // If the state of the bottom tab has not been fully initialized, the generated path must be adjusted
+ const adjustedPath = pathFromCurrentState === `/${ROUTES.ROOT}` ? `/${ROUTES.REPORT}` : pathFromCurrentState;
+ const {adaptedState: templateState} = getAdaptedStateFromPath(adjustedPath, linkingConfig.config);
if (!templateState) {
return;
~
~
~
|
|
This PR is much more complex than we thought. I think it is better to close this PR and compensate @BhuvaneshPatil for the efforts they spent. SWM/ @WojtekBoman can open a fresh PR handling all required changes for the migration to cc: @MonilBhavsar |
|
@WojtekBoman @MonilBhavsar Do you both concur that this is the direction we should go? |
Explanation of Change
migrate AuthScreens component to have useOnyx instead of withOnyx
Fixed Issues
$ #49103
PROPOSAL: #49103 (comment)
Tests
This is migration task
Verify if all the flows on app same as before
Test if loading spinner is shown
Test removing report id from url and if correct report id is shown
Verify that no errors appear in the JS console
Offline tests
Same as above, migration task. App should behave as it was
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
Screen.Recording.2024-11-13.at.7.26.39.PM.mov
Android: mWeb Chrome
Screen.Recording.2024-11-14.at.9.54.23.PM.mov
iOS: Native
Screen.Recording.2024-11-13.at.6.49.57.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-11-14.at.9.45.30.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-11-13.at.7.28.46.PM.mov
MacOS: Desktop
Screen.Recording.2024-11-13.at.7.34.26.PM.mov