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: remove hard reloads #4183

Closed
wants to merge 5 commits into from
Closed

fix: remove hard reloads #4183

wants to merge 5 commits into from

Conversation

neo773
Copy link
Contributor

@neo773 neo773 commented Dec 11, 2023

Removes hard reloads in the codebase

hey.mp4

Related issues

Fixes #4182

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking small changes to existing functionality)

/claim #4182

@neo773 neo773 requested a review from bigint as a code owner December 11, 2023 19:02
@algora-pbc algora-pbc bot mentioned this pull request Dec 11, 2023
Open
@height Height
Copy link

height bot commented Dec 11, 2023

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@vercel Vercel
Copy link

vercel bot commented Dec 11, 2023 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
web ✅ Ready (Inspect) Visit Preview Dec 12, 2023 1:46pm

@neo773 neo773 changed the title refactor: remove hard reloads fix: remove hard reloads Dec 11, 2023
bigint
bigint requested changes Dec 11, 2023
View reviewed changes
apps/web/src/components/Common/Layout.tsx Outdated Show resolved Hide resolved
apps/web/src/components/Common/Providers/LensSubscriptionsProvider.tsx Outdated Show resolved Hide resolved
apps/web/src/components/Shared/Login/WalletSelector.tsx Outdated Show resolved Hide resolved
apps/web/src/components/Shared/Navbar/MenuItems.tsx Outdated Show resolved Hide resolved
apps/web/src/components/Shared/Navbar/MenuItems.tsx Outdated Show resolved Hide resolved
@neo773
Copy link
Contributor Author

neo773 commented Dec 11, 2023

@bigint
Pushed a new commit

@bigint
Copy link
Member

bigint commented Dec 12, 2023

Screen.Recording.2023-12-12.at.12.28.58.PM.mov

@neo773 It seems like some data are not updated, I guess something to do with apollo garbage collection or evict all cache on logout

@bigint
Copy link
Member

bigint commented Dec 12, 2023

Try with profile on testnet!

bigint
bigint requested changes Dec 12, 2023
View reviewed changes
apps/web/src/components/Common/Layout.tsx
Comment on lines 52 to 54
resetPreferences();
resetFeatureFlags();
resetPro();
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can move all these inside signout method itself so we can just call signout everywhere!

apps/web/src/components/Shared/Navbar/MenuItems.tsx
Comment on lines +21 to +22
console.log(currentProfile, 'currentProfile');
console.log(sessionProfileId, 'sessionProfileId');
Copy link
Member

Choose a reason for hiding this comment

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

Remove it

packages/lib/getStampFyiURL.ts
@@ -7,6 +7,10 @@ import urlcat from 'urlcat';
* @returns The cdn.stamp.fyi URL.
*/
const getStampFyiURL = (address: string): string => {
// race condition fix when loging out
if (!address) {
return '';
Copy link
Member

Choose a reason for hiding this comment

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

you can return stamp url with null address aka export const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';

@neo773
Copy link
Contributor Author

neo773 commented Dec 12, 2023

Screen.Recording.2023-12-12.at.12.28.58.PM.mov
@neo773 It seems like some data are not updated, I guess something to do with apollo garbage collection or evict all cache on logout

Expected behaviour is navigating to home page every site does this,
It's impossible to account for the entire state of the App without it getting messy and would be out of scope of the original issue.

@bigint
Copy link
Member

bigint commented Dec 12, 2023

We mentioned it on the issue 🙇🏼

Make sure we clear all local cache while logging out!

@neo773
Copy link
Contributor Author

neo773 commented Dec 12, 2023

@bigint
Is soft reloading current URL acceptable?
Would reset the state with no edge cases.

@bigint
Copy link
Member

bigint commented Dec 12, 2023

WDYM soft reloading?

@bigint
Copy link
Member

bigint commented Dec 12, 2023

Will it reload the entire app?

@neo773
Copy link
Contributor Author

neo773 commented Dec 12, 2023

Router refresh, so navigates to same URL

@bigint
Copy link
Member

bigint commented Dec 12, 2023 via email

@bigint
Copy link
Member

bigint commented Dec 12, 2023 

const { resetStore } = useApolloClient()

@bigint
Copy link
Member

bigint commented Dec 15, 2023

is this good to review?

@neo773
Copy link
Contributor Author

neo773 commented Dec 15, 2023

@bigint
nah left a message on discord

@bigint
Copy link
Member

bigint commented Dec 17, 2023

Closing this, we can rethink this later!

@bigint bigint closed this Dec 17, 2023
@bigint bigint deleted the reloadd branch December 18, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bigint bigint bigint requested changes

Assignees
No one assigned
Labels
🙋 Bounty claim
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove hard location.reload or hard navigate everywhere
2 participants
@neo773 @bigint