Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Warning banner & post-login redirect both fail when starting Take Now while already logged into Web App as wrong respondent (M2-8029) #539

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

fix: Warning banner & post-login redirect both fail when starting Take Now while already logged into Web App as wrong respondent (M2-8029) #539

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b6c170
Fix to the useSessionBanners hook to ensure that logic such as banne…
LashaunnaS Oct 21, 2024
f2d04c8
Store the original url so that when the app logs out the previously a…
LashaunnaS Oct 22, 2024
8aa8fe0
Capture possibility of of user coming from the loginroute once authen…
LashaunnaS Oct 22, 2024
63045fd
Remove use of useEffect hook. Which seems to be causing removeAllbann…
LashaunnaS Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/entities/user/model/hooks/useOnLogin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export const useOnLogin = (params: Params) => {
secureTokensStorage.setTokens(tokens);

if (params.backRedirectPath !== undefined) {
navigate(params.backRedirectPath);
navigate(params.backRedirectPath, { replace: true });
} else {
Mixpanel.track('Login Successful');
Mixpanel.login(user.id);
Expand Down
17 changes: 15 additions & 2 deletions src/features/Logout/lib/useLogout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { useCallback } from 'react';

import { useLocation } from 'react-router-dom';

import { appletModel } from '~/entities/applet';
import { useLogoutMutation, userModel } from '~/entities/user';
import { AutoCompletionModel } from '~/features/AutoCompletion';
Expand All @@ -14,6 +16,7 @@ type UseLogoutReturn = {

export const useLogout = (): UseLogoutReturn => {
const navigator = useCustomNavigation();
const location = useLocation();

const { clearUser } = userModel.hooks.useUserState();
const { clearStore } = appletModel.hooks.useClearStore();
Expand All @@ -37,8 +40,18 @@ export const useLogout = (): UseLogoutReturn => {
Mixpanel.track('logout');
Mixpanel.logout();
FeatureFlags.logout();
return navigator.navigate(ROUTES.login.path);
}, [clearUser, clearStore, clearAutoCompletionState, navigator, logoutMutation]);

const backRedirectPath = `${location.pathname}${location.search}`;
return navigator.navigate(ROUTES.login.path, { state: { backRedirectPath } });
}, [
clearUser,
clearStore,
clearAutoCompletionState,
location.pathname,
location.search,
navigator,
logoutMutation,
]);

return {
logout,
Expand Down
5 changes: 3 additions & 2 deletions src/pages/AuthorizedRoutes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Navigate, Route, Routes } from 'react-router-dom';
import AppletDetailsPage from './AppletDetailsPage';
import AppletListPage from './AppletListPage';
import AutoCompletion from './AutoCompletion';
import LoginPage from './Login';
import ProfilePage from './Profile';
import PublicAutoCompletion from './PublicAutoCompletion';
import SettingsPage from './Settings';
Expand Down Expand Up @@ -48,10 +49,10 @@ function AuthorizedRoutes({ refreshToken }: Props) {
<Route path={ROUTES.privateJoin.path} element={<PrivateJoinPage />} />
<Route path={ROUTES.publicJoin.path} element={<PublicAppletDetailsPage />} />
<Route path={ROUTES.transferOwnership.path} element={<TransferOwnershipPage />} />

<Route path="*" element={<Navigate to={ROUTES.appletList.path} />} />
</Route>
</Route>
<Route path={ROUTES.login.path} element={<LoginPage />} />
<Route path="*" element={<Navigate to={ROUTES.appletList.path} />} />
</Routes>
</LogoutTracker>
);
Expand Down
4 changes: 2 additions & 2 deletions src/shared/utils/hooks/useSessionBanners/useSessionBanners.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useRef } from 'react';
import { useMemo, useRef } from 'react';

import { useBanners } from '~/entities/banner/model';
import { userModel } from '~/entities/user';
Expand All @@ -9,7 +9,7 @@ export const useSessionBanners = () => {

const prevIsAuthorized = useRef(isAuthorized);

useEffect(() => {
useMemo(() => {
if (prevIsAuthorized.current !== isAuthorized && !isAuthorized) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't say I actually understand what this change is doing, even after reading your PR description. I get that with a useMemo, the callback is executed inline immediately the first time it's encountered by the interpreter, whereas with useEffect, the callback is called async after the component has completed the initial render. Could you walk me through exactly what was going wrong here when useEffect was in place?

I've also never used useMemo for a situation where the return value was discarded, but maybe I just haven't stumbled upon this use-case before now.

The React docs on useMemo say:

You should only rely on useMemo as a performance optimization. If your code doesn’t work without it, find the underlying problem and fix it first. Then you may add useMemo to improve performance.

I guess I'm also wondering if this code aligns with that rule or not…

Copy link
Author

Choose a reason for hiding this comment

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

@farmerpaul that's a good callout. With further debugging, I think there may be a re-rendering issue stemming from the redux logic w/ user, token, and isAuthorized state. I can't quite pin down what's triggering the re-renders. 🤔

Copy link
Author

@LashaunnaS LashaunnaS Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@farmerpaul Removing the useEffect does ensure the banner isn't automatically cleared because of that particular component re-rendering. I do still have some concerns about the amount of re-rendering taking place, which seems to be a performance issue already established on prod.

I think the rerendering we're seeing is coming from how far along the app renders down the tree as the already authenticated user before it determines that the new user isn't authenticated.

removeAllBanners();
}
Expand Down
Loading