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.

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

fix: feed screen after page refresh #538

Merged
merged 7 commits into from
Jul 10, 2024
Merged

fix: feed screen after page refresh #538

merged 7 commits into from
Jul 10, 2024

Conversation

davidgamez
Copy link
Member

@davidgamez davidgamez commented Jul 8, 2024

Summary:

The feed screen is not rendering after hitting refresh on the page.

Fixes: #537

How to reproduce the issue(error loading feeds):

  • Open the app
  • Click Browse
  • Click the any feed and wait for the feed screen to be fully render
  • Refresh page
  • Current behavior => error raised with text : There was an error loading the feed.
  • Expected behavior => Feed screen is rendered correctly

Root cause

The issue was present due to redux store rehydrating the store information with user anonymous before Firebase SDK had a valid anonymous user set. The fix delays the loading of the screens until a valid user information is set on the Firebase SDK.

User session is not shared across tabs.

Fixes: #516 , #443

Root cause

We were using session storage instead of local storage in the redux-persist reducer. In addition, we needed to add a mechanism to send an event to the open tabs when the user authentication changes.

Expected behavior:

The feed screen is rendered after the page is refreshed.

Testing tips:

  • Follow the How to reproduce the issue(error loading feeds). Test with different authentication state combinations.
  • User session is not shared across tabs.
    • Login to the app
    • Click open in a new tap in the feeds page(or any other page)
    • Logout from the first tab
    • Expected: the second tab must also logout
    • (Try different combinations of login and logout and expect both tabs to keep the same authentication state)

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with ./scripts/api-tests.sh to make sure you didn't break anything
  • Add or update any needed documentation to the repo
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@davidgamez davidgamez changed the title load components when app is ready fix: feed screen after page refresh Jul 8, 2024
Copy link

github-actions bot commented Jul 8, 2024

Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-538-z25zp98c.web.app

@@ -93,6 +93,7 @@ export const userProfileSlice = createSlice({
state.status = 'unauthenticated';
state.isSignedInWithProvider = false;
state.isAppRefreshing = false;
state.user = undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not related to the fix. However, I noticed the user information stayed in the store after logout. This might lead to unexpected behavior after completing the logout and before the anonymous login succeeds.

yield app.auth().signOut();
yield put(logoutSuccess());
yield put(anonymousLogin()); // Use anonymous login to keep the user signed in
Copy link
Member Author

Choose a reason for hiding this comment

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

Anonymous login is not needed anymore here. Now anonymous login is being centralized in the root component.

@@ -80,10 +79,9 @@ function* logoutSaga({
navigateTo: NavigateFunction;
}>): Generator {
try {
navigateTo(redirectScreen);
Copy link
Member Author

Choose a reason for hiding this comment

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

We are navigating out of the current screen before logging out to avoid race conditions.

@davidgamez
Copy link
Member Author

davidgamez commented Jul 9, 2024

Login and logout flow with multiple tabs open.

Screen.Recording.2024-07-09.at.2.44.27.PM.mov

@davidgamez davidgamez marked this pull request as ready for review July 9, 2024 19:21
};

const loginUser = (): void => {
// Refresh the page to ensure the user is authenticated
Copy link
Contributor

Choose a reason for hiding this comment

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

I would update Refresh the page to ensure the user is authenticated on this session and other sessions (tabs)

Copy link
Member Author

Choose a reason for hiding this comment

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

The refresh will only affect the current window/session

Comment on lines 44 to 45
createDispatchChannel(LOGOUT_CHANNEL, logoutUser);
createDispatchChannel(LOGIN_CHANNEL, loginUser);
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] how come we created a custom subscribe / dispatch system instead of using Redux reducer dispatch directly?

Copy link
Member Author

@davidgamez davidgamez Jul 10, 2024

Choose a reason for hiding this comment

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

The redux dispatch doesn't dispatch events across tabs; it's scoped to the current session. We are using redux-persist, but the loading of the state from the local storage is only applicable on redux state rehydration(when the app loads the first time or refreshes). I was looking into other redux libraries that sync states across tabs. However, those libraries make the tabs entirely in sync, making it tricky to have different "states" reflected on each tab. Examples open different search results or feeds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added documentation and renamed variable and function names to improve readability.

Copy link
Contributor

@Alessandro100 Alessandro100 left a comment

Choose a reason for hiding this comment

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

@davidgamez davidgamez merged commit 4c04750 into main Jul 10, 2024
4 checks passed
@davidgamez davidgamez deleted the fix/access-token branch July 10, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants