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: 🐛 Fix infinit refresh bugs #10943

Merged
merged 2 commits into from
Nov 15, 2024
Merged

fix: 🐛 Fix infinit refresh bugs #10943

merged 2 commits into from
Nov 15, 2024

Conversation

chef-ryan
Copy link
Contributor

@chef-ryan chef-ryan commented Nov 15, 2024


PR-Codex overview

This PR modifies the useUserVoteGauges hook in the apps/web/src/views/GaugesVoting/hooks/useUserVoteGauges.ts file, focusing on the handling of the publicClient and the structure of the returned data.

Detailed summary

  • Reverted the import of publicClient to its original form.
  • Changed the initialData property in the query options to an empty array.
  • Updated the return statement to directly return data instead of using a fallback to an empty array.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

vercel bot commented Nov 15, 2024

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

Name Status Preview Comments Updated (UTC)
web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2024 6:32am
6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
aptos-web ⬜️ Ignored (Inspect) Visit Preview Nov 15, 2024 6:32am
blog ⬜️ Ignored (Inspect) Visit Preview Nov 15, 2024 6:32am
bridge ⬜️ Ignored (Inspect) Visit Preview Nov 15, 2024 6:32am
games ⬜️ Ignored (Inspect) Visit Preview Nov 15, 2024 6:32am
gamification ⬜️ Ignored (Inspect) Visit Preview Nov 15, 2024 6:32am
uikit ⬜️ Ignored (Inspect) Visit Preview Nov 15, 2024 6:32am

Copy link

changeset-bot bot commented Nov 15, 2024

⚠️ No Changeset found

Latest commit: 8be59a3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -62,7 +62,7 @@ const useSelectRowsWithQuery = (gauges: Gauge[] | undefined) => {
const urlHashes = queryHashes.filter((hash) => !newHashes.includes(hash))
setSelectRowsHash(urlHashes.concat(newHashes))
}
}, [isLoading, prevVotedGauges, queryHashes])
}, [isLoading, prevVotedGauges.map((x) => x.hash).join('-'), queryHashes])
Copy link
Contributor

Choose a reason for hiding this comment

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

will map every render, how about to use a memo value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

chefjackson
chefjackson previously approved these changes Nov 15, 2024
@@ -62,7 +62,7 @@ const useSelectRowsWithQuery = (gauges: Gauge[] | undefined) => {
const urlHashes = queryHashes.filter((hash) => !newHashes.includes(hash))
setSelectRowsHash(urlHashes.concat(newHashes))
}
}, [isLoading, prevVotedGauges, queryHashes])
}, [isLoading, prevVotedGauges.map((x) => x.hash).join('-'), queryHashes])
Copy link
Collaborator

Choose a reason for hiding this comment

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

would suggest to fix the ref issue within the useUserVoteGauges hook

Copy link
Contributor

Choose a reason for hiding this comment

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

@chefjackson
Do you mean something like this?

const prevGaugesRef = useRef(prevVotedGauges);

const hasChanges = prevGaugesRef.current.some(
  (prevGauge, index) => prevGauge.hash !== prevVotedGauges[index]?.hash
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Chef-Yogi
Chef-Yogi previously approved these changes Nov 15, 2024
@chef-ryan chef-ryan dismissed stale reviews from Chef-Yogi and chefjackson via 8be59a3 November 15, 2024 06:23
@@ -35,7 +35,7 @@ export const useUserVoteSlopes = () => {
userInfo?.cakePoolProxy,
publicClient,
],

initialData: [],
Copy link
Collaborator

@memoyil memoyil Nov 15, 2024

Choose a reason for hiding this comment

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

if initial data used isLoading will be false always even there is no real fetch, so i would suggest to memoize the data when returning

Copy link
Contributor

Choose a reason for hiding this comment

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

or use placeholderData

Copy link
Collaborator

Choose a reason for hiding this comment

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

placeholderData behaves the same if i am not mistaken

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, my mistake

Copy link
Contributor Author

@chef-ryan chef-ryan Nov 15, 2024

Choose a reason for hiding this comment

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

Hi, I checked, momoyil correct.

Since set either initialData or placeholderData will suggest isloading to be false.
Let's assume this is actually what should be the standard behavior.
Introduct a memod state is too complex for this easy requirement.

So I perfer to do no changes here.
Let's migrate to async atom in future.

@chef-ryan chef-ryan enabled auto-merge (squash) November 15, 2024 08:15
@chef-ryan chef-ryan merged commit b7e013e into develop Nov 15, 2024
19 checks passed
@chef-ryan chef-ryan deleted the fix/gaugelist-selection branch November 15, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants