fix structure owners & transfers #2544
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces significant modifications across multiple files in the client-side codebase, focusing on query management, state handling, and loading mechanisms. The changes primarily involve renaming and updating query functions to handle multiple entity IDs, enhancing the flexibility of data retrieval. A new loading state for structures has been added to the world store, and the Changes
Sequence DiagramsequenceDiagram
participant Client
participant QueryManager
participant DataStore
Client->>QueryManager: Request multiple entity IDs
QueryManager->>DataStore: Composite query with "Or" operator
DataStore-->>QueryManager: Retrieve matching entities
QueryManager-->>Client: Return entity data
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
|
Failed to generate code suggestions for PR |
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look good overall - removing console.logs, adding proper loading states, and consolidating query functions. The error handling could be more robust and there are a few minor optimizations possible, but the core changes are solid. The new loading state management for structures is a good addition but could benefit from timeout handling to prevent stuck states.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
| ...entityID.map((id) => ({ | ||
| Keys: { | ||
| keys: [id, undefined], | ||
| pattern_matching: "VariableLen" as PatternMatching, |
There was a problem hiding this comment.
Consider adding error handling and retry logic for failed entity fetches
| const arrivals = positions.flatMap((position) => { | ||
| return Array.from(runQuery([HasValue(Position, { x: position.x, y: position.y }), ...queryFragments])); | ||
| }); | ||
| return arrivals; |
There was a problem hiding this comment.
This intermediate variable assignment is unnecessary. You can return the flatMap result directly.
| const arrivals = positions.flatMap((position) => { | |
| return Array.from(runQuery([HasValue(Position, { x: position.x, y: position.y }), ...queryFragments])); | |
| }); | |
| return arrivals; | |
| return positions.flatMap((position) => { | |
| return Array.from(runQuery([HasValue(Position, { x: position.x, y: position.y }), ...queryFragments])); | |
| }); |
| isStructuresLoading: boolean, | ||
| setStructuresLoading: (loading: boolean) => void, | ||
| ) => { | ||
| if (!isStructuresLoading) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider adding a timeout to prevent infinite loading states
![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
🔭 Outside diff range comments (1)
client/src/hooks/helpers/use-resource-arrivals.tsx (1)
Line range hint
98-113: Ensure subscriptions are properly cleaned upIn the
useEffecthook starting at line 98, make sure to return a clean-up function that unsubscribes from thequery.update$subscription. This prevents memory leaks and unintended side effects when the component unmounts.
🧹 Nitpick comments (6)
client/src/dojo/queries.ts (1)
44-47: Extract model names to constants or enumsModel names like
"s0_eternum-BuildingQuantityv2"are hardcoded strings. Extracting these into constants or an enum would prevent typos, facilitate reuse, and make future changes easier.client/src/hooks/store/useWorldLoading.tsx (1)
7-10: Align interface properties for readabilityThe properties in the
WorldStateinterface are not consistently aligned, which affects readability. Align the new properties with the existing ones for better code clarity.Apply this diff to align the code:
interface WorldState { isWorldLoading: boolean; isMarketLoading: boolean; + isStructuresLoading: boolean; setWorldLoading: (loading: boolean) => void; setMarketLoading: (loading: boolean) => void; + setStructuresLoading: (loading: boolean) => void; }client/src/hooks/helpers/use-resource-arrivals.tsx (1)
62-65: Simplify function by returning directlyIn
getArrivalsWithResourceOnPosition, the variablearrivalsis assigned but not modified before being returned. You can simplify the function by returning the result directly, improving readability.Apply this diff:
const getArrivalsWithResourceOnPosition = useCallback((positions: Position[]) => { - const arrivals = positions.flatMap((position) => { + return positions.flatMap((position) => { return Array.from(runQuery([HasValue(Position, { x: position.x, y: position.y }), ...queryFragments])); }); - return arrivals; }, []);client/src/ui/layouts/World.tsx (1)
150-154: Enhance error handling in subscription managementWhile the new subscription approach is good, consider improving error handling to ensure graceful degradation if subscriptions fail.
addToSubscriptionOneKeyModelbyRealmEntityId( dojo.network.toriiClient, dojo.network.contractComponents as any, - [...filteredStructures.map((structure) => structure.entity_id.toString())], + [...filteredStructures.map((structure) => structure.entity_id.toString())], +).catch((error) => { + console.error('Failed to subscribe to one-key models:', error); + // Consider showing a user-friendly error message ),client/src/ui/modules/world-structures/WorldStructuresMenu.tsx (2)
356-356: Remove console.log statementRemove debugging console.log statement before merging to production.
- console.log("Fetching hyperstructure data");
382-387: Extract magic number to a named constantConsider extracting the timeout value (40_000) to a named constant for better maintainability.
+const ENTITIES_QUERY_TIMEOUT = 40_000; + await getEntities( client, // ... other params ... - 40_000, + ENTITIES_QUERY_TIMEOUT, false, ).finally(() => { setStructuresLoading(false); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/src/dojo/queries.ts(3 hunks)client/src/dojo/setup.ts(1 hunks)client/src/hooks/helpers/use-resource-arrivals.tsx(1 hunks)client/src/hooks/store/useWorldLoading.tsx(1 hunks)client/src/ui/layouts/World.tsx(2 hunks)client/src/ui/modules/world-structures/WorldStructuresMenu.tsx(6 hunks)
🔇 Additional comments (7)
client/src/dojo/queries.ts (2)
35-49: Ensure correct usage of undefined in keys
In addToSubscriptionTwoKeyModelbyRealmEntityId, the keys array includes [id, undefined] for each entity ID. Using undefined may lead to unintended behavior in the pattern matching or query results. Verify that this is intentional and aligns with the expected API behavior.
Line range hint 60-78: Refactor duplicate code in subscription functions
Similar to the previous function, addToSubscriptionOneKeyModelbyRealmEntityId also contains duplicated logic. Refactoring this common logic into a shared helper function would improve code quality and maintainability.
client/src/hooks/store/useWorldLoading.tsx (1)
16-19: Verify initial loading state of structures
isStructuresLoading is initialized to true. Confirm that this reflects the actual loading state at initialization. If structures are not loading immediately, consider initializing this value to false to prevent misleading the UI.
client/src/dojo/setup.ts (1)
143-143: Addition of EntityOwner model improves data completeness
Including "s0_eternum-EntityOwner" in the models list enhances the data retrieved during entity synchronization. This allows for ownership information to be readily available in components that rely on it.
client/src/hooks/helpers/use-resource-arrivals.tsx (1)
Line range hint 109-114: Optimize state updates to prevent unnecessary re-renders
The handleArrivalUpdate function updates state for every component update matching the query, which could lead to performance issues due to frequent re-renders. Consider batching state updates or adding conditions to minimize unnecessary updates.
[performance]
client/src/ui/layouts/World.tsx (1)
6-11: LGTM: Import changes align with the new subscription management approach
The new imports suggest a more flexible approach to handling entity subscriptions, supporting the PR's objective of fixing structure owners & transfers.
client/src/ui/modules/world-structures/WorldStructuresMenu.tsx (1)
36-37: LGTM: Improved loading state management
Good addition of loading state management with proper UI feedback for better user experience.
Also applies to: 145-151
| export const addToSubscriptionTwoKeyModelbyRealmEntityId = async <S extends Schema>( | ||
| client: ToriiClient, | ||
| components: Component<S, Metadata, undefined>[], | ||
| entityID: string, | ||
| entityID: string[], | ||
| ) => { | ||
| await getEntities( | ||
| client, | ||
| { | ||
| Keys: { | ||
| keys: [entityID, undefined], | ||
| pattern_matching: "FixedLen" as PatternMatching, | ||
| models: ["s0_eternum-BuildingQuantityv2"], | ||
| Composite: { | ||
| operator: "Or", | ||
| clauses: [ | ||
| ...entityID.map((id) => ({ | ||
| Keys: { | ||
| keys: [id, undefined], | ||
| pattern_matching: "VariableLen" as PatternMatching, | ||
| models: ["s0_eternum-BuildingQuantityv2"], | ||
| }, | ||
| })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code in subscription functions
The functions addToSubscriptionTwoKeyModelbyRealmEntityId and addToSubscriptionOneKeyModelbyRealmEntityId share similar logic in constructing composite queries. Consider creating a helper function to reduce code duplication and improve maintainability.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores