-
Notifications
You must be signed in to change notification settings - Fork 19
[Improvement, GoodCollective] The total amount on the GC home page is not right #267 #298
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
[Improvement, GoodCollective] The total amount on the GC home page is not right #267 #298
Conversation
- Add donorCollectives query to fetch flowRate, timestamp, and contribution - Calculate real-time streaming amounts for active donor collectives - Aggregate all donor collective totals instead of using stale totalDonations - Handle both streaming and non-streaming donor collectives Fixes issue where total donations didn't reflect current streaming amounts from donor collectives, only showing last recorded totals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @EmekaManuel - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/app/src/hooks/useTotalStats.ts:22` </location>
<code_context>
- const donationsFormatted = formatGoodDollarAmount(totalDonations?.toString() ?? '0', 2);
+ if (!stats) return undefined;
+
+ const currentTime = Math.floor(Date.now() / 1000);
+
+ let totalFromDonorCollectives = BigInt(0);
</code_context>
<issue_to_address>
Using Date.now() directly may cause hydration mismatches in SSR environments.
Consider moving the Date.now() call into a useEffect or similar client-only logic to prevent SSR hydration issues.
</issue_to_address>
### Comment 2
<location> `packages/app/src/hooks/useTotalStats.ts:28` </location>
<code_context>
+
+ stats.donorCollectives?.forEach((donorCollective) => {
+ const flowRate = BigInt(donorCollective.flowRate || '0');
+ const lastUpdateTime = donorCollective.timestamp || 0;
+ const baseContribution = BigInt(donorCollective.contribution || '0');
+
</code_context>
<issue_to_address>
Defaulting timestamp to 0 may result in large, unrealistic time intervals.
Defaulting to 0 can inflate timeElapsed and donation calculations. Please handle missing timestamps more carefully.
</issue_to_address>
### Comment 3
<location> `packages/app/src/hooks/useTotalStats.ts:32` </location>
<code_context>
+ const baseContribution = BigInt(donorCollective.contribution || '0');
+
+ if (flowRate > 0) {
+ const timeElapsed = BigInt(currentTime - lastUpdateTime);
+ const additionalStreamingAmount = flowRate * timeElapsed;
+ const totalForThisCollective = baseContribution + additionalStreamingAmount;
</code_context>
<issue_to_address>
No check for negative timeElapsed could lead to incorrect calculations.
If currentTime is earlier than lastUpdateTime, timeElapsed becomes negative, potentially causing incorrect results. Please add a check to prevent negative timeElapsed values.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (flowRate > 0) {
const timeElapsed = BigInt(currentTime - lastUpdateTime);
const additionalStreamingAmount = flowRate * timeElapsed;
const totalForThisCollective = baseContribution + additionalStreamingAmount;
totalFromDonorCollectives += totalForThisCollective;
} else {
=======
if (flowRate > 0) {
let timeElapsed = BigInt(currentTime - lastUpdateTime);
if (timeElapsed < 0) {
timeElapsed = BigInt(0);
}
const additionalStreamingAmount = flowRate * timeElapsed;
const totalForThisCollective = baseContribution + additionalStreamingAmount;
totalFromDonorCollectives += totalForThisCollective;
} else {
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| stats.donorCollectives?.forEach((donorCollective) => { | ||
| const flowRate = BigInt(donorCollective.flowRate || '0'); | ||
| const lastUpdateTime = donorCollective.timestamp || 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Defaulting timestamp to 0 may result in large, unrealistic time intervals.
Defaulting to 0 can inflate timeElapsed and donation calculations. Please handle missing timestamps more carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add error handling in this flow, and show a message in UI if it fails to show/calculate the current flowing total amount.
Better show error message, then a wrong amount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! To address this, you should:
- Add error handling in the
useTotalStatshook (or wherever the streaming calculation is performed). If an error occurs (e.g., missing or invalid timestamp, failed fetch, or calculation error), set an error state. - In the
HomePagecomponent, check for this error state. If present, display a clear error message in the UI (e.g., using aTextcomponent or a banner) instead of the total amount.
This way, users will see an explicit error message if the calculation fails, rather than a potentially misleading or incorrect total. If you need a code example for this pattern, let me know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EmekaManuel Sorry, this addition of errors is a bit overdoing it.
You can handle it with just a single try/catch block in the forEach loop
| if (flowRate > 0) { | ||
| const timeElapsed = BigInt(currentTime - lastUpdateTime); | ||
| const additionalStreamingAmount = flowRate * timeElapsed; | ||
| const totalForThisCollective = baseContribution + additionalStreamingAmount; | ||
|
|
||
| totalFromDonorCollectives += totalForThisCollective; | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): No check for negative timeElapsed could lead to incorrect calculations.
If currentTime is earlier than lastUpdateTime, timeElapsed becomes negative, potentially causing incorrect results. Please add a check to prevent negative timeElapsed values.
| if (flowRate > 0) { | |
| const timeElapsed = BigInt(currentTime - lastUpdateTime); | |
| const additionalStreamingAmount = flowRate * timeElapsed; | |
| const totalForThisCollective = baseContribution + additionalStreamingAmount; | |
| totalFromDonorCollectives += totalForThisCollective; | |
| } else { | |
| if (flowRate > 0) { | |
| let timeElapsed = BigInt(currentTime - lastUpdateTime); | |
| if (timeElapsed < 0) { | |
| timeElapsed = BigInt(0); | |
| } | |
| const additionalStreamingAmount = flowRate * timeElapsed; | |
| const totalForThisCollective = baseContribution + additionalStreamingAmount; | |
| totalFromDonorCollectives += totalForThisCollective; | |
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Redundant Timestamp Calculation ▹ view | ✅ Fix detected | |
| Missing timestamp validation in streaming calculation ▹ view | ✅ Fix detected | |
| Extract Complex Calculation Logic ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| packages/app/src/subgraph/useSubgraphTotalStats.ts | ✅ |
| packages/app/src/subgraph/useSubgraphData.ts | ✅ |
| packages/app/src/hooks/useTotalStats.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| if (flowRate > 0) { | ||
| const timeElapsed = BigInt(currentTime - lastUpdateTime); | ||
| const additionalStreamingAmount = flowRate * timeElapsed; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| stats.donorCollectives?.forEach((donorCollective) => { | ||
| const flowRate = BigInt(donorCollective.flowRate || '0'); | ||
| const lastUpdateTime = donorCollective.timestamp || 0; | ||
| const baseContribution = BigInt(donorCollective.contribution || '0'); | ||
|
|
||
| if (flowRate > 0) { | ||
| const timeElapsed = BigInt(currentTime - lastUpdateTime); | ||
| const additionalStreamingAmount = flowRate * timeElapsed; | ||
| const totalForThisCollective = baseContribution + additionalStreamingAmount; | ||
|
|
||
| totalFromDonorCollectives += totalForThisCollective; | ||
| } else { | ||
| totalFromDonorCollectives += baseContribution; | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EmekaManuel I think mostly looks good.
But one thing is missing, which is that the stream is not reflecting on the home-page as stream.
It does update the amount after a page refresh.
Look at the other pages where show flowing balances.
-
on a collectives page: https://dev-goodcollective.vercel.app/collective/0x627dbf00ce1a54067f5a34d6596a217a029c1532
-
In code you can look for example to the flow of 'this wallet has donated' here:
<GoodDollarAmount -
also check out the useFlowingBalance hook: https://github.com/GoodDollar/GoodCollective/blob/master/packages/app/src/hooks/useFlowingBalance.tsx
That should give you an idea how you can handle it for the homepage.
Any questions, let us know!
Screen.Recording.2025-07-21.at.23.36.00.movimplemented the streaming / flowing balance functionality - @L03TJ3 |
|
|
||
| stats.donorCollectives?.forEach((donorCollective) => { | ||
| const flowRate = BigInt(donorCollective.flowRate || '0'); | ||
| const lastUpdateTime = donorCollective.timestamp || 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add error handling in this flow, and show a message in UI if it fails to show/calculate the current flowing total amount.
Better show error message, then a wrong amount
packages/app/src/pages/HomePage.tsx
Outdated
| totalMembers: { | ||
| copy: 'GoodCollective Members Paid', | ||
| }, | ||
| const useDonorCollectivesData = (collectiveAddresses: string[], tokenPrice: number) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unneccary and duplicate of the calculation logic done in useTotalStats.
keep the calculation(s) in the useTotalStats, and get the needed amounts there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented @L03TJ3
…mplementation incase of errors
|
@EmekaManuel The request is:
Here is an example of how this can be handled in useTotalStats: https://gist.github.com/L03TJ3/01c702d5245328656d233cc3802f7a39 |
thanks for this, i have made the necessary changes as requested |
@L03TJ3 @sirpy
Description
The total donations calculation was using stale
totalDonationsvalues from the subgraph, which didn't include current streaming amounts from donor collectives. This caused the displayed total to be lower than the actual current total.This PR implements real-time streaming calculation by fetching donor collectives data and calculating additional streamed amounts since the last update for active streams, then aggregating all donor collective totals for accurate current donations.
About
#267
Description by Korbit AI
What change is being made?
Refactor the calculation and display logic for total amounts on the GoodCollective home page to accurately reflect donations by incorporating donor collective balances and improving error handling.
Why are these changes being made?
The current total amount displayed on the homepage was incorrect, necessitating a restructure to account for flowing balances of donor collectives, integrate token price conversion, and handle error states gracefully. This improves the reliability and user experience of the application, ensuring users see accurate, meaningful data.