Fix missing donkeys in bridge step 2 #2598
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces performance optimizations in two key files: Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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 (
|
|
You are out of MentatBot reviews. Your usage will refresh December 30 at 08:00 AM. |
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
landing/src/hooks/helpers/useDonkeyArrivals.tsx (3)
23-33: Remove or justify commented-out query.If the single-query approach is deprecated, please remove these lines to reduce clutter. Otherwise, add a comment explaining its future usage.
34-42: Monitor performance overhead for multiple queries.Using
useQueriesfor each realm ID is granular but could cause latency for large arrays. Consider batching or caching if performance issues arise.
85-90: Refine the queryenabledcondition.
enabled: !!donkeyEntityIdsmight evaluate true for an empty array. If you only want to query when there’s at least one ID, useenabled: donkeyEntityIds.length > 0.landing/src/components/modules/bridge-out-step-2.tsx (1)
127-134: Verify stale closure indebounce.
updateResourcesFromSelectedDonkeysis captured at mount. If that function changes later, it might not be reflected by the debounced function unless you add it to dependencies. Evaluate whether that is a concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
landing/src/components/modules/bridge-out-step-2.tsx(3 hunks)landing/src/hooks/helpers/useDonkeyArrivals.tsx(2 hunks)
🔇 Additional comments (14)
landing/src/hooks/helpers/useDonkeyArrivals.tsx (10)
3-3: Confirm usage of both useQuery and useQueries.
It’s good to import both if needed, but verify we are optimizing for readability and performance.
12-13: Avoid hard-coding realm ID exclusion.
Filtering out ID 54 might be intentional, but if behavior changes or more IDs need exclusion, consider parametrizing instead of hard-coding.
44-53: Flattening edges looks correct.
The flatten operation is straightforward and likely correct for ensuring a standardized data structure. No issues found.
55-57: Concise state derivation.
Checking loading/error states individually and aggregating them is a solid approach. No improvements needed here.
58-64: Bank position memoization.
Memoizing the bank position is appropriate for performance. Looks fine.
65-79: Validate resource condition for “donkeys at bank.”
Ensuring resource?.resource_types !== "0x0" is good for filtering out empty donkey entities. Just confirm that "0x0" is indeed the correct sentinel value.
80-83: Fluid donkey ID extraction.
Mapping donkey IDs from arrived donkeys is a clean approach.
91-112: Modular donkey info retrieval.
Storing arrival timestamps and resource balances in one function is fine. If this grows more complex, consider a separate helper or sub-hook for clarity.
114-117: Memoizing donkey info.
Using useMemo for donkey info array is logical for performance reasons.
118-126: Return object is well-structured.
Exposing donkey arrivals, donkey infos, bank position, and donkey resources is intuitive and consistent.
landing/src/components/modules/bridge-out-step-2.tsx (4)
9-9: Imports for React hooks are correct.
The useState, useCallback, and useEffect imports are standard and necessary here.
19-38: Custom debounce implementation.
This is a straightforward approach to debouncing calls. The built-in cancel method for cleanup is a good defensive measure.
139-139: Checking donkey arrival time vs. unconditional add.
Logic changed to always add donkeys to newSelected. If that’s the intended behavior, confirm no side effects.
142-147: Cleanup in effect is handled well.
Calling debouncedUpdateResources.cancel() ensures no pending tasks execute on unmount or dependency changes.
| const { | ||
| data: entityPositions, | ||
| isLoading: isLoadingPositions, | ||
| error: positionsError, | ||
| } = useQuery({ | ||
| queryKey: ["entityPositionBank"], | ||
| queryFn: () => execute(GET_ENTITY_DISTANCE, { entityIds: [Number(ADMIN_BANK_ENTITY_ID)] }), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refetch intervals for consistency.
This query for entityPositions doesn’t specify a refetchInterval, whereas donkey queries do. If real-time updates are required, set a consistent interval value.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
landing/src/components/modules/bridge-out-step-2.tsx (1)
Line range hint
110-118: Fix incomplete useEffect implementationThe effect creates a new Set but doesn't use it, and includes an unnecessary dependency.
The effect should update the selected donkeys and trigger the resource update:
useEffect(() => { const newSelected = new Set<bigint>(); donkeyInfos?.forEach((donkey) => { if (Number(donkey?.donkeyArrivalTime) * 1000 <= Date.now()) { newSelected.add(BigInt(donkey?.donkeyEntityId || 0)); } }); + setSelectedDonkeys(newSelected); + updateResourcesFromSelectedDonkeys(newSelected); -}, [donkeyInfos, selectedDonkeys]); +}, [donkeyInfos, updateResourcesFromSelectedDonkeys]);
🧹 Nitpick comments (1)
landing/src/components/modules/bridge-out-step-2.tsx (1)
Line range hint
52-67: Enhance error handling and user feedbackThe current error handling only logs to console without providing user feedback.
Consider implementing proper error handling:
try { setIsLoading(true); const tx = await bridgeFinishWithdrawFromRealm(donkeyResources, ADMIN_BANK_ENTITY_ID); if (tx) { setSelectedDonkeys(new Set()); setSelectedResourceIds([]); setSelectedResourceAmounts({}); + // Add success notification + toast.success('Resources successfully withdrawn'); } } catch (e) { - console.log(e); + console.error('Bridge withdrawal failed:', e); + // Add error notification + toast.error( + e instanceof Error + ? e.message + : 'Failed to withdraw resources. Please try again.' + ); } finally { setIsLoading(false); }Don't forget to import the toast notification library of your choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
landing/src/components/modules/bridge-out-step-2.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
landing/src/components/modules/bridge-out-step-2.tsx
[error] 100-100: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (1)
landing/src/components/modules/bridge-out-step-2.tsx (1)
9-9: LGTM: Import added for useMemo
The import statement is correctly added to support the new memoization implementation.
| const updateResourcesFromSelectedDonkeys = useMemo( | ||
| () => (selectedDonkeyIds: Set<bigint>) => { | ||
| const allResources = Array.from(selectedDonkeyIds).flatMap( | ||
| (id) => | ||
| donkeyInfos?.find((d) => d?.donkeyEntityId && BigInt(d.donkeyEntityId) === id)?.donkeyResourceBalances || [], | ||
| ); | ||
|
|
||
| setSelectedResourceIds(allResources.map((r) => r.resourceId as never)); | ||
| setSelectedResourceAmounts( | ||
| allResources.reduce( | ||
| (acc, r) => ({ | ||
| ...acc, | ||
| [r.resourceId]: (acc[r.resourceId] || 0) + r.amount / RESOURCE_PRECISION, | ||
| }), | ||
| {}, | ||
| ), | ||
| ); | ||
| }, | ||
| [donkeyInfos, selectedDonkeys], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize memoization implementation
There are two issues with the current implementation:
- The dependency array includes
selectedDonkeysbut it's not used inside the memoized function. - The memoization benefit is partially negated by creating new objects in every call.
Apply these optimizations:
const updateResourcesFromSelectedDonkeys = useMemo(
() => (selectedDonkeyIds: Set<bigint>) => {
const allResources = Array.from(selectedDonkeyIds).flatMap(
(id) =>
donkeyInfos?.find((d) => d?.donkeyEntityId && BigInt(d.donkeyEntityId) === id)?.donkeyResourceBalances || [],
);
setSelectedResourceIds(allResources.map((r) => r.resourceId as never));
setSelectedResourceAmounts(
allResources.reduce(
(acc, r) => {
- return {
- ...acc,
- [r.resourceId]: (acc[r.resourceId] || 0) + r.amount / RESOURCE_PRECISION,
- }
+ acc[r.resourceId] = (acc[r.resourceId] || 0) + r.amount / RESOURCE_PRECISION;
+ return acc;
},
{} as { [key: string]: number }
),
);
},
- [donkeyInfos, selectedDonkeys],
+ [donkeyInfos],
);📝 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.
| const updateResourcesFromSelectedDonkeys = useMemo( | |
| () => (selectedDonkeyIds: Set<bigint>) => { | |
| const allResources = Array.from(selectedDonkeyIds).flatMap( | |
| (id) => | |
| donkeyInfos?.find((d) => d?.donkeyEntityId && BigInt(d.donkeyEntityId) === id)?.donkeyResourceBalances || [], | |
| ); | |
| setSelectedResourceIds(allResources.map((r) => r.resourceId as never)); | |
| setSelectedResourceAmounts( | |
| allResources.reduce( | |
| (acc, r) => ({ | |
| ...acc, | |
| [r.resourceId]: (acc[r.resourceId] || 0) + r.amount / RESOURCE_PRECISION, | |
| }), | |
| {}, | |
| ), | |
| ); | |
| }, | |
| [donkeyInfos, selectedDonkeys], | |
| ); | |
| const updateResourcesFromSelectedDonkeys = useMemo( | |
| () => (selectedDonkeyIds: Set<bigint>) => { | |
| const allResources = Array.from(selectedDonkeyIds).flatMap( | |
| (id) => | |
| donkeyInfos?.find((d) => d?.donkeyEntityId && BigInt(d.donkeyEntityId) === id)?.donkeyResourceBalances || [], | |
| ); | |
| setSelectedResourceIds(allResources.map((r) => r.resourceId as never)); | |
| setSelectedResourceAmounts( | |
| allResources.reduce( | |
| (acc, r) => { | |
| acc[r.resourceId] = (acc[r.resourceId] || 0) + r.amount / RESOURCE_PRECISION; | |
| return acc; | |
| }, | |
| {} as { [key: string]: number } | |
| ), | |
| ); | |
| }, | |
| [donkeyInfos], | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 100-100: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
* Fix missing arrival when sending lords & donkeys (#2594) * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter
* Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]>
* Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) * im (#2604) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]>
* Merge into main (#2603) * Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]> * merge into main (#2605) * Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) * im (#2604) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]> * Fix build (#2610) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]>
* Merge into main (#2603) * Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]> * merge into main (#2605) * Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) * im (#2604) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]> * Fix build (#2610) * filter out donkeys w/ empty balance --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]>
* Fix/donkeys empty balance (#2611) * Merge into main (#2603) * Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]> * merge into main (#2605) * Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) * im (#2604) --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]> * Fix build (#2610) * filter out donkeys w/ empty balance --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]> * add stats resources bridged on empire (#2618) * add resourcers bridged * change logo * remove looping gql calls (#2620) * remove looping gql calls * batch queries --------- Co-authored-by: Credence <[email protected]> Co-authored-by: Loaf <[email protected]> Co-authored-by: raschel <[email protected]>
* Fix/donkeys empty balance (#2611) * Merge into main (#2603) * Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) --------- * merge into main (#2605) * Fix missing arrival when sending lords & donkeys (#2594) * contracts: fix share points check * contracts: fix share points check * Add resource filter for contributions (#2596) * Add resource filter for contributions * Change % for specific resource contribution * Fix missing donkeys in bridge step 2 (#2598) * Fix missing donkeys in bridge step 2 * fix rerenders * Remove filter * Fix registration delay (#2602) * Fix registration delay * styling * Open popup on load * Add burned donkeys back (#2601) * im (#2604) --------- * Fix build (#2610) * filter out donkeys w/ empty balance --------- * add stats resources bridged on empire (#2618) * add resourcers bridged * change logo * remove looping gql calls (#2620) * remove looping gql calls * batch queries --------- Co-authored-by: Loaf <[email protected]> Co-authored-by: Bob <[email protected]> Co-authored-by: Credence <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Refactor