fix realms limit #2583
fix realms limit #2583
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces significant refactoring across multiple components and hooks related to realm management and bridge interactions. The changes primarily focus on simplifying the data retrieval and processing logic for player realms, removing the Changes
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 23 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: 0
🔭 Outside diff range comments (1)
landing/src/components/modules/bridge-in.tsx (1)
Line range hint
247-255: Add null check for playerRealms arrayWhile the implementation works, it could be more robust by handling the case when playerRealms is undefined.
Consider this safer implementation:
-{playerRealms?.length +{playerRealms?.length > 0 ? playerRealms.map((realm) => { return ( <SelectItem key={realm.realmId} value={realm.entityId.toString()}> #{realm.realmId} - {realm.name} </SelectItem> ); }) - : "No Realms settled in Eternum"} + : <SelectItem value="" disabled>No Realms settled in Eternum</SelectItem>}
🧹 Nitpick comments (10)
landing/src/hooks/helpers/useEntities.tsx (2)
8-11: Interface 'S0EternumRealm' is clear and minimal
The structure is straightforward and helps ensure type safety when dealing with realm data.
17-45: Comprehensive data retrieval with useEntities
- The query setup and usage of the "playerRealms" array appear well-organized.
- The "useMemo" block filters and maps the data properly, leveraging the type guard to ensure only valid realm models are included.
- The final result of “playerRealms” is cleanly structured, returning the realm ID, entity ID, and name.
- Checking for "data" early is an excellent defensive check to avoid runtime errors.
Consider adding unit tests for multiple scenarios (e.g., no data, partial data, etc.) to confirm that "playerRealms" always renders as expected.
landing/src/hooks/gql/gql.ts (2)
16-19: Query definitions updated for getEternumOwnerRealmIds
Adding "limit: 1000" streamlines the retrieval logic. Just confirm that no other queries need the same treatment.
37-81: Consistent formatting of GraphQL function signatures
- The move toward consistent and more readable doc strings is beneficial.
- The large “documents” object is convenient for development but consider the potential dev vs. production trade-offs.
landing/src/components/modules/bridge-out-step-1.tsx (4)
44-45: Use of local state isFeesOpen
This local state is properly encapsulated. Using a named state update function or grouping related states can improve readability if more dialog-related logic is added in the future.
48-48: Use of refetchInterval
Re-fetching capacity and speed config every 10 seconds might cause extra load if large user numbers exist. Consider whether a manual refresh or a more extended interval is enough.
232-232: Mapping over playerRealms inside Select
This is the correct usage for a dynamic list. Potentially handle a scenario where "playerRealms" is a large array (loading states, etc.).
277-277: Donkeys needed vs donkeyBalance
The presentation is clear. If donkey supply is insufficient, consider showing a warning or disabling the button as appropriate.landing/src/dojo/modelManager/ConfigManager.ts (1)
234-241: Track TODO: Review direct EternumGlobalConfig usageThe implementation works but is marked as temporary due to lack of access to recs. This should be tracked and revisited.
Would you like me to create a GitHub issue to track the TODO for implementing a proper solution using recs?
Additionally, consider adding error logging:
getResourceWeight(resourceId: number): number { // todo: using EternumGlobalConfig because no access to recs return this.getValueOrDefault(() => { const weightGram = EternumGlobalConfig.resources.resourceWeightsGrams[ resourceId as keyof typeof EternumGlobalConfig.resources.resourceWeightsGrams ]; + if (weightGram === undefined) { + console.warn(`No weight configuration found for resource ${resourceId}`); + } return weightGram; }, 0); }landing/src/components/modules/bridge-out-step-2.tsx (1)
24-24: Remove debug console.logDebug logging should be removed before merging to production.
- console.log({ playerRealms });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
landing/src/components/modules/bridge-in.tsx(5 hunks)landing/src/components/modules/bridge-out-step-1.tsx(5 hunks)landing/src/components/modules/bridge-out-step-2.tsx(4 hunks)landing/src/dojo/modelManager/ConfigManager.ts(1 hunks)landing/src/hooks/gql/gql.ts(2 hunks)landing/src/hooks/helpers/useEntities.tsx(1 hunks)landing/src/hooks/query/entities.tsx(1 hunks)
🔇 Additional comments (15)
landing/src/hooks/helpers/useEntities.tsx (3)
3-3: Use of useMemo is well-placed for performance
Using "useMemo" around data transformations is good practice to avoid unnecessary re-computations.
6-6: Import dependencies consistently
Good job bringing in "useRealm" here. Make sure you verify that there's no duplication of logic or any overshadowed naming in adjacent files.
13-15: Type guard isS0EternumRealm
Distinct type guard is beneficial for narrowing the type from generic data structures. Good approach.
landing/src/hooks/query/entities.tsx (1)
5-5: Limit added to the query
Limiting the query results to 1000 helps prevent excessive data fetching and potential performance issues. Ensure your application design can safely handle truncation (e.g., if a user has more than 1000 realms, consider pagination or other approaches).
landing/src/components/modules/bridge-out-step-1.tsx (4)
52-58: Memoized donkeyConfig
Good approach to avoid unnecessary re-renders. Watch out for potential changes in the data shape. If the query shape or the node structure changes, the destructuring might fail silently.
81-81: Resource balances fetched with getBalance
The logic is straightforward. Confirm the underlying implementation for error handling if the query fails to fetch the current balance.
90-90: playerRealms retrieval from useEntities
Neat integration with the new API. Combining realm selection logic with resource selection flow is well structured.
94-94: computing travelTime with computeTravelTime
The argument order here is slightly different from the initial call. Make sure the function’s signature is consistent with your usage.
landing/src/components/modules/bridge-in.tsx (2)
31-31: LGTM: Resource balance handling is implemented correctly
The resource balance hook usage is properly implemented with loading state handling and balance validation.
Also applies to: 119-119
38-44: LGTM: Donkey configuration is properly memoized
The configuration is correctly memoized with appropriate default values and dependency tracking.
landing/src/components/modules/bridge-out-step-2.tsx (5)
9-9: LGTM: Explicit hook import
Good practice to explicitly import the useEffect hook.
22-22: LGTM: Simplified realm data retrieval
The change to use useEntities directly instead of useDojo aligns with the PR's objective of simplifying realm management. The mapping of realm entity IDs is now more straightforward.
Also applies to: 26-26
93-94: LGTM: Improved null safety
Good addition of optional chaining operator for safer access to donkeyInfos.
247-247: LGTM: Formatting only
No functional changes.
22-26: Verify bridge withdrawal functionality
The changes to realm data retrieval could impact the bridge withdrawal process. Let's verify that all bridge-related functionality remains intact.
✅ Verification successful
Let me gather more information about the bridge-related components and their interactions.
Let me check one final aspect to verify the useEntities implementation and its interaction with the bridge components.
Bridge withdrawal functionality is correctly implemented
The changes to realm data retrieval are consistent with the bridge withdrawal process. The useEntities hook properly fetches and formats player realms data, which is then correctly used by the bridge components for:
- Realm selection in bridge-in/out steps
- Donkey arrival tracking for withdrawals
- Travel time calculations
The integration between useEntities, useBridge, and useDonkeyArrivals maintains the expected bridge withdrawal workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify bridge withdrawal implementation consistency
# Check for any other files using the old useDojo pattern for bridge operations
rg -l "useDojo.*bridge"
# Check for any potential breaking changes in useEntities implementation
ast-grep --pattern 'export function useEntities() {
$$$
}'
# Look for other bridge components that might need similar updates
fd -e tsx -e ts bridge
Length of output: 383
Script:
#!/bin/bash
# Check the implementation of bridge-related hooks and components
rg -A 5 "useEntities" landing/src/components/modules/bridge-*.tsx
# Check the implementation of useBridge hook
cat landing/src/hooks/useBridge.tsx
# Look for any dependencies on playerRealms in bridge components
rg -A 3 "playerRealms" landing/src/components/modules/bridge-*.tsx
Length of output: 9335
Script:
#!/bin/bash
# Check the implementation of useEntities hook
cat landing/src/hooks/helpers/useEntities.tsx
# Check for any donkey-related functionality that might be affected
rg -A 5 "useDonkeyArrivals" landing/src/components/modules/bridge-*.tsx
Length of output: 3005
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
landing/src/components/modules/bridge-out-step-1.tsx (1)
277-277: Add null check for donkey balanceThe current implementation might break if
donkeyBalanceis undefined.Add a null check:
-{donkeysNeeded} / {divideByPrecision(donkeyBalance)} <ResourceIcon resource={"Donkey"} size="md" /> +{donkeysNeeded} / {divideByPrecision(donkeyBalance ?? 0)} <ResourceIcon resource={"Donkey"} size="md" />landing/src/components/modules/bridge-in.tsx (1)
247-255: Improve error handling in structure selectionThe current implementation shows "No Structure settled in Eternum" as plain text, which isn't consistent with the UI pattern.
Consider using a proper SelectItem for the empty state:
{playerStructures?.length ? playerStructures.map((structure) => { return ( <SelectItem key={structure.realmId} value={structure.entityId.toString()}> #{structure.realmId} - {structure.name} </SelectItem> ); }) - : "No Structure settled in Eternum"} + : <SelectItem disabled value="none">No Structure settled in Eternum</SelectItem>}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
landing/src/components/modules/bridge-in.tsx(5 hunks)landing/src/components/modules/bridge-out-step-1.tsx(5 hunks)landing/src/components/modules/bridge-out-step-2.tsx(4 hunks)landing/src/hooks/gql/graphql.ts(1 hunks)landing/src/hooks/helpers/use-sync-entity.tsx(0 hunks)landing/src/hooks/helpers/useEntities.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- landing/src/hooks/helpers/use-sync-entity.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- landing/src/components/modules/bridge-out-step-2.tsx
🔇 Additional comments (3)
landing/src/hooks/helpers/useEntities.tsx (1)
31-45: LGTM! Well-structured realm data processing
The implementation efficiently processes realm data with proper null checks and type guards.
landing/src/components/modules/bridge-in.tsx (1)
Line range hint 3-3: Verify impact of useRealm removal
Let's verify that all realm-related functionality is properly migrated to useEntities.
Also applies to: 80-80, 90-90
landing/src/hooks/gql/graphql.ts (1)
7355-7355: LGTM! Consider verifying if 1000 is the optimal limit.
The addition of a limit parameter is a good practice to prevent unbounded queries. The value of 1000 seems reasonable but should be verified against:
- The maximum number of realms a user typically owns
- Any performance implications on the GraphQL server
Let's verify the typical realm ownership patterns:
✅ Verification successful
Let me try a different approach to verify the realm ownership patterns.
Based on the search results, I found important context that helps verify the limit:
- From
landing/src/components/modules/realm-mint-dialog.tsx, there's a maximum of 4 Realms that can be minted per user. - The query is used to fetch owned realms, and with the 4-realm limit per user, the current limit of 1000 is more than sufficient.
The limit of 1000 is appropriate and well above the maximum possible ownership.
Since users can only mint up to 4 Realms, setting the query limit to 1000 provides a large safety margin while still preventing unbounded queries. The limit will never be a bottleneck for legitimate users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the distribution of realm ownership per address to validate the 1000 limit
# Get realm counts per owner
rg -A 5 "s0_eternum_Owner.*realm_id" | sort | uniq -c | sort -nr | head -n 5
Length of output: 6115
Script:
#!/bin/bash
# Search for realm-related configurations or constants that might indicate expected ownership limits
rg -i "realm.*limit|max.*realm" --type ts
# Look for any realm-related configuration files
fd -e json -e yaml -e toml -e ts realm config
Length of output: 13996
| export function isS0EternumStructure(model: any): model is S0EternumRealm { | ||
| return model?.__typename === "s0_eternum_Structure"; | ||
| } |
There was a problem hiding this comment.
Fix incorrect type usage in isS0EternumStructure guard
The type guard isS0EternumStructure incorrectly uses S0EternumRealm as its return type. This should be a separate interface for structures.
Create a separate interface for structures:
+export interface S0EternumStructure {
+ __typename: "s0_eternum_Structure";
+}
-export function isS0EternumStructure(model: any): model is S0EternumRealm {
+export function isS0EternumStructure(model: any): model is S0EternumStructure {
return model?.__typename === "s0_eternum_Structure";
}Committable suggestion skipped: line range outside the PR's diff.
| const playerStructures = useMemo(() => { | ||
| if (!data) return []; | ||
|
|
||
| return data.s0EternumOwnerModels?.edges | ||
| ?.map((structure) => { | ||
| const structureModel = structure?.node?.entity?.models?.find(isS0EternumStructure); | ||
| if (!structureModel) return null; | ||
| const realmModel = structure?.node?.entity?.models?.find(isS0EternumRealm); | ||
| const entityId = structure?.node?.entity_id; | ||
| return { | ||
| realmId: realmModel?.realm_id || entityId, | ||
| entityId, | ||
| name: realmModel ? getRealmNameById(realmModel?.realm_id ?? 0) : "Structure", | ||
| }; | ||
| }) | ||
| .filter(Boolean) as { realmId: number | undefined; entityId: number; name: string }[]; | ||
| }, [data, getRealmNameById]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving structure identification logic
The current implementation has two potential issues:
- Falling back to
entityIdwhenrealmIdis missing could lead to confusion - Using "Structure" as a fallback name loses valuable information about the structure type
Consider this improved implementation:
return {
- realmId: realmModel?.realm_id || entityId,
+ realmId: realmModel?.realm_id,
entityId,
- name: realmModel ? getRealmNameById(realmModel?.realm_id ?? 0) : "Structure",
+ name: realmModel ? getRealmNameById(realmModel?.realm_id ?? 0) : `Structure #${entityId}`,
+ type: structureModel?.__typename ?? "Unknown"
};Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
getEternumOwnerRealmIdsquery to restrict results to 1000 entries.Bug Fixes
Refactor
Documentation
useEntitieshook.