Refetch transactions and metadatas at an interval after the last refetch #245
Conversation
|
Warning Rate limit exceeded@hansl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis change introduces an interval-based refresh mechanism to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
==========================================
+ Coverage 55.08% 55.10% +0.02%
==========================================
Files 200 201 +1
Lines 16873 16905 +32
==========================================
+ Hits 9294 9316 +22
- Misses 7579 7589 +10 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/bank/components/historyBox.tsx (1)
9-9: Use standard comment syntax.The comment on line 9 uses triple slashes
///, which are typically reserved for TypeScript directives. Consider using double slashes//for regular comments to improve readability and prevent any unintended compiler interpretations.
hansl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/bank/components/historyBox.tsx(2 hunks)components/groups/components/groupControls.tsx(1 hunks)
🔇 Additional comments (4)
components/bank/components/historyBox.tsx (4)
1-1: LGTM!The imports are necessary for implementing the interval-based refresh mechanism.
43-43: Ensurerefetchconsistently returns aPromise<void>.To maintain the integrity of the Promise chain in
refetchInner, it's important thatrefetchalways returns aPromise<void>. Usinganytype is not a good practice as it bypasses TypeScript's type checking.Apply this diff to update the
refetchtype signature:-refetch: () => Promise<any>; +refetch: () => Promise<void>;
49-49: LGTM!The destructuring of
refetchMetadatasis necessary for implementing the metadata refresh functionality.
53-76: RefactoruseEffectfor reliable interval management and correct dependencies.The current
useEffectimplementation uses a recursivesetTimeoutwith manual Promise chaining and adoneflag, which can be error-prone and complex. Additionally, the dependency array[refetchMetadatas]may lead to unintended re-executions of the effect, especially ifmetadataschanges frequently.Recommendations:
- Simplify interval management by using
setInterval: This reduces complexity and ensures that the refetch functions are called at consistent intervals.- Adjust the dependency array: Include
refetchandrefetchMetadatasto ensure that the effect responds correctly if these functions change. If these functions are stable (do not change between renders), you can provide an empty dependency array[].- Ensure
refetchandrefetchMetadatasare stable: If they are recreated on every render, consider wrapping them withuseCallbackto prevent unnecessary effect re-runs.Apply this refactored code:
useEffect(() => { - let latestRefetch = Promise.resolve(); - let done = false; - let timer = setTimeout(refetchInner, HISTORY_BOX_REFRESH_INTERVAL); - function refetchInner() { - if (done) return; - latestRefetch = latestRefetch - .then(() => refetch()) - .then(() => refetchMetadatas()) - .then(() => { - timer = setTimeout(refetchInner, HISTORY_BOX_REFRESH_INTERVAL); - }); - } - return () => { - done = true; - clearTimeout(timer); - }; - }, [refetchMetadatas]); + useEffect(() => { + const intervalId = setInterval(() => { + refetch(); + refetchMetadatas(); + }, HISTORY_BOX_REFRESH_INTERVAL); + + return () => { + clearInterval(intervalId); + }; + }, [refetch, refetchMetadatas]);This refactor:
- Uses
setIntervalfor consistent periodic execution.- Ensures proper cleanup with
clearIntervalto prevent memory leaks.- Updates the dependency array to include only
refetchandrefetchMetadatasfor correct effect re-initialization.
| @@ -42,7 +42,7 @@ type GroupControlsProps = { | |||
| denomLoading: boolean; | |||
| isDenomError: boolean; | |||
| refetchBalances: () => void; | |||
| refetchHistory: () => void; | |||
| refetchHistory: () => Promise<any>; | |||
There was a problem hiding this comment.
Use Promise<void> instead of Promise<any>.
Using any type bypasses TypeScript's type checking. Since refetchHistory is used to trigger a refresh without returning any meaningful value, it should return Promise<void>.
Apply this diff to update the type signature:
-refetchHistory: () => Promise<any>;
+refetchHistory: () => Promise<void>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| refetchHistory: () => Promise<any>; | |
| refetchHistory: () => Promise<void>; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/bank/components/historyBox.tsx (2)
43-43: Consider using a more specific return type forrefetch.Using
Promise<any>reduces type safety. Consider using a more specific return type that matches the actual return value of the refetch operation.- refetch: () => Promise<any>; + refetch: () => Promise<void>;
61-68: Add tests for the refetch functionality.The refetch logic is currently not covered by tests. We can help implement tests using Jest's timer mocks to avoid actual delays.
Would you like me to generate test cases that demonstrate how to test the refetch functionality without introducing actual delays? The tests would use
jest.useFakeTimers()andjest.advanceTimersByTime()to simulate the interval without waiting.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-68: components/bank/components/historyBox.tsx#L61-L68
Added lines #L61 - L68 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/bank/components/historyBox.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
components/bank/components/historyBox.tsx (1)
Learnt from: hansl
PR: liftedinit/manifest-app#245
File: components/bank/components/historyBox.tsx:0-0
Timestamp: 2025-02-06T01:36:10.277Z
Learning: In the HistoryBox component, requests need to be debounced to prevent concurrent executions, ensuring that a new request starts only after the previous one completes.
🪛 GitHub Check: codecov/patch
components/bank/components/historyBox.tsx
[warning] 61-68: components/bank/components/historyBox.tsx#L61-L68
Added lines #L61 - L68 were not covered by tests
🔇 Additional comments (2)
components/bank/components/historyBox.tsx (2)
9-11: Adjust the refresh interval to match the PR objective.The PR objective mentions a 3-second wait time, but the code uses 1 second (1000 milliseconds).
-const HISTORY_BOX_REFRESH_INTERVAL = 1000; +const HISTORY_BOX_REFRESH_INTERVAL = 3000;
49-49: LGTM!The hook usage is correct and follows React hooks best practices.
Fixes #239
This is lacking tests as I'm not sure how to test refetch without having to wait 3 seconds between mocks.
I tested it (and false tested too) manually and it works wonderfully.
Summary by CodeRabbit
New Features
Refactor