feat: change to new torii queries #2652
Conversation
edisontim
requested review from
aymericdelab and
bob0005
and removed request for
aymericdelab
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hi @edisontim! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page. You are receiving this comment because I am set to review all PRs. That is configurable here. |
|
Caution Review failedThe pull request is closed. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
client/apps/game/src/dojo/sync.tsOops! Something went wrong! :( ESLint: 9.18.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThe pull request introduces a comprehensive refactoring of data retrieval mechanisms across multiple files in the game client. The primary focus is shifting from subscription-based methods to direct data fetching techniques. Key changes include renaming functions, modifying query parameters, and updating how entities are retrieved from the Torii network. The modifications span files like Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
client/apps/game/src/dojo/queries.ts (1)
Line range hint
107-149: Optimize asynchronous calls in 'getMarketFromTorii'In the
getMarketFromToriifunction, bothgetEntitiescalls are awaited individually, and thenPromise.allis called on their results, which are no longer promises. This can lead to sequential execution rather than concurrent.To run these calls concurrently, initiate both without awaiting them immediately and then await them together:
- const resourcePromise = await getEntities(...); ... - const marketPromise = await getEntities(...); ... - await Promise.all([resourcePromise, marketPromise]); + const resourcePromise = getEntities(...); ... + const marketPromise = getEntities(...); ... + await Promise.all([resourcePromise, marketPromise]);This adjustment allows both asynchronous operations to run in parallel, improving performance.
🧹 Nitpick comments (6)
client/apps/game/src/dojo/queries.ts (1)
104-104: Avoid using 'as any' for improved type safetyCasting
componentstoanyreduces type safety and may introduce runtime errors. Consider refining the types or updating interfaces to eliminate the need foras any:await getEntities(client, query, components, [], entityModels, 40_000);This enhances maintainability and catches potential issues at compile time.
client/apps/game/src/dojo/debounced-queries.ts (2)
Line range hint
15-16: Consider making RequestQueue parameters configurable.The
batchSizeandbatchDelayMsare hardcoded. Consider making these configurable through the constructor to allow for different queue configurations based on use cases.class RequestQueue { private queue: Array<() => Promise<void>> = []; private processing = false; - private batchSize = 3; // Number of concurrent requests - private batchDelayMs = 100; // Delay between batches + constructor( + private readonly batchSize: number = 3, + private readonly batchDelayMs: number = 100 + ) {}
56-65: Standardize debounce delays across functions.The debounce delays vary between functions (250ms vs 500ms) without clear justification. Consider standardizing these delays or documenting why different delays are necessary.
Consider extracting the delays to named constants:
+const ENTITY_DEBOUNCE_DELAY = 250; +const MARKET_DEBOUNCE_DELAY = 500; export const debouncedTwoKeyEntitiesFromTorii = debounce( // ... function implementation - 250, + ENTITY_DEBOUNCE_DELAY, { leading: true }, );Also applies to: 69-79, 82-91, 95-109, 113-122
client/apps/game/src/ui/components/resources/inventory-resources.tsx (1)
8-9: Enhance error handling and cache management.The error handling could be improved, and the cache management could be extracted into a reusable utility.
Consider implementing a more robust error handling and cache management:
+const SYNC_RETRY_ATTEMPTS = 3; +const SYNC_RETRY_DELAY = 1000; + +const getCacheKey = (entityId: string) => `${CACHE_KEY}-${entityId}`; + +const isValidCache = (cachedTime: string | null) => { + if (!cachedTime) return false; + return Date.now() - parseInt(cachedTime) < CACHE_DURATION; +}; + +const syncResources = async (entityId: string, dojo: any) => { + let attempts = 0; + while (attempts < SYNC_RETRY_ATTEMPTS) { try { - console.log("AddToSubscriptionStart - 4"); await debouncedGetEntitiesFromTorii( dojo.network.toriiClient, dojo.network.contractComponents as any, [entityId.toString()] ); + localStorage.setItem(getCacheKey(entityId), Date.now().toString()); + return true; } catch (error) { - console.error("Fetch failed", error); + console.error(`Sync attempt ${attempts + 1} failed:`, error); + attempts++; + if (attempts < SYNC_RETRY_ATTEMPTS) { + await new Promise(resolve => setTimeout(resolve, SYNC_RETRY_DELAY)); + } } + } + return false; +};Also applies to: 62-64
client/apps/game/src/dojo/sync.ts (2)
81-116: Enhance performance monitoring.The performance monitoring could be more structured and reusable.
Consider implementing a performance monitoring utility:
+const measurePerformance = async (name: string, fn: () => Promise<any>) => { + const start = performance.now(); + try { + return await fn(); + } finally { + const end = performance.now(); + console.log(`[composite] ${name}`, end - start); + } +}; -let start = performance.now(); -Promise.all([ +await measurePerformance('big config query', () => Promise.all([ // ... existing getEntities calls -]); -let end = performance.now(); -console.log("[composite] big config query", end - start); +]));
263-297: Consider organizing model names into categories.The large arrays of model names could be better organized for maintainability.
Consider organizing the models into logical categories:
+const MODEL_CATEGORIES = { + CONFIG: { + WORLD: ['s1_eternum-WorldConfig', 's1_eternum-MapConfig'], + SEASON: ['s1_eternum-SeasonAddressesConfig', 's1_eternum-SeasonBridgeConfig'], + HYPERSTRUCTURE: ['s1_eternum-HyperstructureResourceConfig', 's1_eternum-HyperstructureConfig'], + // ... other categories + }, + SINGLE_KEY: { + REALM: ['s1_eternum-Realm', 's1_eternum-RealmMaxLevelConfig'], + BATTLE: ['s1_eternum-Battle', 's1_eternum-BattleConfig'], + // ... other categories + } +}; -const configModels = [ ... ]; -const singleKeyModels = [ ... ]; +const configModels = Object.values(MODEL_CATEGORIES.CONFIG).flat(); +const singleKeyModels = Object.values(MODEL_CATEGORIES.SINGLE_KEY).flat();Also applies to: 299-319
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/apps/game/src/dojo/debounced-queries.ts(2 hunks)client/apps/game/src/dojo/queries.ts(4 hunks)client/apps/game/src/dojo/sync.ts(2 hunks)client/apps/game/src/ui/components/resources/inventory-resources.tsx(2 hunks)client/apps/game/src/ui/components/trading/resource-arrivals.tsx(2 hunks)client/apps/game/src/ui/layouts/world.tsx(4 hunks)client/apps/game/src/ui/modules/loading-screen.tsx(0 hunks)client/public/resource_addresses/local/resource_addresses.json(0 hunks)
💤 Files with no reviewable changes (2)
- client/public/resource_addresses/local/resource_addresses.json
- client/apps/game/src/ui/modules/loading-screen.tsx
🧰 Additional context used
🪛 GitHub Actions: lint
client/apps/game/src/ui/components/trading/resource-arrivals.tsx
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
🪛 GitHub Actions: knip
client/apps/game/src/ui/components/resources/inventory-resources.tsx
[error] 1-4: Multiple unresolved imports: '@/dojo/debouncedQueries', '@/hooks/context/DojoContext', '@/ui/elements/ResourceCost'
client/apps/game/src/ui/layouts/world.tsx
[error] 1-1: Unresolved import: '@/dojo/debouncedQueries'
client/apps/game/src/dojo/queries.ts
[warning] 9-152: Unused exports: 'getTwoKeyModelbyRealmEntityIdFromTorii', 'getOneKeyModelbyRealmEntityIdFromTorii', 'getMarketFromTorii', 'addDonkeysAndArmiesSubscription'
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run pr agent on every pull request, respond to user comments
🔇 Additional comments (5)
client/apps/game/src/dojo/queries.ts (5)
Line range hint
9-33: Remove unused exported function 'getTwoKeyModelbyRealmEntityIdFromTorii'According to the pipeline failure logs, the function
getTwoKeyModelbyRealmEntityIdFromToriiis exported but not used anywhere in the codebase. To keep the codebase clean and maintainable, consider removing this function if it's unnecessary or ensure it is being used appropriately.
Line range hint
37-61: Remove unused exported function 'getOneKeyModelbyRealmEntityIdFromTorii'The function
getOneKeyModelbyRealmEntityIdFromToriiis also exported but not used. Please verify its necessity and remove it if it's redundant.
Line range hint
107-149: Remove unused exported function 'getMarketFromTorii'Per the pipeline failure logs,
getMarketFromToriiis exported but not used. Verify if this function is necessary. If not, consider removing it to clean up the codebase.
Line range hint
153-176: Remove unused exported function 'addDonkeysAndArmiesSubscription'The function
addDonkeysAndArmiesSubscriptionis exported but not utilized. Please check if it's needed and remove it if it's obsolete to reduce unnecessary code.
95-98: Verify usage of 'undefined' in 'keys' arrayIn the
getEntitiesFromToriifunction,undefinedvalues are included in thekeysarray:keys: [String(position?.x || 0), String(position?.y || 0), undefined, undefined],Using
undefinedin thekeysarray might lead to unexpected behavior. Please confirm if this is intentional and adjust if necessary.✅ Verification successful
Usage of
undefinedin keys array is intentional and follows established patternsThe usage of
undefinedvalues in the keys array is consistent with the established patterns throughout the codebase, particularly in sync and query operations. This pattern appears in multiple locations with both "FixedLen" and "VariableLen" pattern matching, indicating it's a deliberate design choice for the Torii client implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the complete implementation of getEntitiesFromTorii ast-grep --pattern 'function getEntitiesFromTorii($$$) { $$$ }' # Search for similar patterns with undefined in keys array rg 'keys:\s*\[.*undefined.*\]' -A 2 -B 2 # Look for PatternMatching type definition ast-grep --pattern 'type PatternMatching = $$$' # Search for any comments or documentation about keys pattern rg -B 3 'keys.*undefined.*undefined'Length of output: 5532
| debouncedGetDonkeysAndArmiesFromTorii, | ||
| debouncedGetEntitiesFromTorii, | ||
| debouncedGetMarketFromTorii, | ||
| debouncedGetOneKeyEntitiesByRealmEntityIdFromTorii, | ||
| } from "@/dojo/debouncedQueries"; |
There was a problem hiding this comment.
Fix unresolved import of '@/dojo/debouncedQueries'
The pipeline reports an unresolved import error:
import {
debouncedGetDonkeysAndArmiesFromTorii,
debouncedGetEntitiesFromTorii,
debouncedGetMarketFromTorii,
debouncedGetOneKeyEntitiesByRealmEntityIdFromTorii,
} from "@/dojo/debouncedQueries";Please ensure that the module '@/dojo/debouncedQueries' exists and that the import path is correct. Verify the file name and its export statements to resolve this issue.
| @@ -46,7 +46,7 @@ export const AllResourceArrivals = memo( | |||
| addSubscribedIds(unsubscribedIds); | |||
|
|
|||
| // Move API call outside of state updates | |||
| addToSubscription(dojo.network.toriiClient, dojo.network.contractComponents as any, unsubscribedIds).catch( | |||
| getEntitiesFromTorii(dojo.network.toriiClient, dojo.network.contractComponents as any, unsubscribedIds, ["s1_eternum-DetachedResource"]).catch( | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix code formatting to match Prettier standards
The pipeline indicates that the code formatting does not comply with Prettier standards. Please run Prettier with the --write flag to automatically fix formatting issues.
🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
| import { debouncedGetEntitiesFromTorii } from "@/dojo/debouncedQueries"; | ||
| import { useDojo } from "@/hooks/context/DojoContext"; | ||
| import { useUIStore } from "@/hooks/store/use-ui-store"; | ||
| import { ResourceCost } from "@/ui/elements/resource-cost"; | ||
| import { ResourceCost } from "@/ui/elements/ResourceCost"; |
There was a problem hiding this comment.
Fix unresolved imports.
The pipeline is failing due to unresolved imports. Ensure the import paths are correct:
- '@/dojo/debouncedQueries'
- '@/hooks/context/DojoContext'
- '@/ui/elements/ResourceCost'
🧰 Tools
🪛 GitHub Actions: knip
[error] 1-4: Multiple unresolved imports: '@/dojo/debouncedQueries', '@/hooks/context/DojoContext', '@/ui/elements/ResourceCost'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/apps/game/src/ui/layouts/world.tsx (2)
154-160: Consider extracting entity types to constants.The entity type strings are repeated across multiple queries. Consider extracting them to a shared constant to improve maintainability.
+const STRUCTURE_ENTITY_TYPES = [ + "s1_eternum-BuildingQuantityv2", + "s1_eternum-Hyperstructure", + "s1_eternum-Resource", + "s1_eternum-Building", + "s1_eternum-Quest", +] as const;
148-148: Consider structured performance logging.The performance monitoring logs are scattered throughout the code. Consider creating a utility function for consistent performance logging:
+const logQueryPerformance = (queryName: string, startTime: number) => { + const duration = performance.now() - startTime; + console.log(`[composite] ${queryName} query: ${duration.toFixed(2)}ms`); +}; -let start = performance.now(); -// ... query code ... -let end = performance.now(); -console.log("[composite] buildings query", end - start); +const start = performance.now(); +// ... query code ... +logQueryPerformance("buildings", start);Also applies to: 165-166, 182-182, 197-198, 200-200, 207-208
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/apps/game/src/ui/components/resources/inventory-resources.tsx(4 hunks)client/apps/game/src/ui/layouts/world.tsx(4 hunks)
🔇 Additional comments (5)
client/apps/game/src/ui/components/resources/inventory-resources.tsx (3)
32-33: LGTM: Component reference updated correctly.The change from
dojo.setup.componentstodojo.network.contractComponentsaligns with the new architecture.
1-1: Verify import resolution.The import statement has been updated to use
debouncedGetEntitiesFromTorii. Please ensure that the pipeline errors regarding unresolved imports have been addressed.✅ Verification successful
Import resolution verified successfully
The
debouncedGetEntitiesFromToriifunction is correctly exported fromclient/apps/game/src/dojo/debounced-queries.tsand the import path is properly resolved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the import path exists and is exported correctly rg -l "export.*debouncedGetEntitiesFromTorii" "src/dojo/"Length of output: 107
Script:
#!/bin/bash # Find the dojo directory and debounced-queries file fd -t d "dojo$" fd "debounced-queries\.(ts|tsx|js|jsx)$" # After finding the files, search for the export rg "export.*debouncedGetEntitiesFromTorii"Length of output: 335
62-67: Consider impact on real-time updates.The switch from subscription to direct query might affect real-time data synchronization. Consider implementing a polling mechanism or WebSocket connection if real-time updates are required.
client/apps/game/src/ui/layouts/world.tsx (2)
Line range hint
254-260: LGTM: Robust error handling.The error handling ensures loading states are properly reset even when errors occur, preventing UI from being stuck in a loading state.
2-5: Verify import resolution.The new query function imports align with the architectural changes. Please ensure that the pipeline errors regarding unresolved imports have been resolved.
✅ Verification successful
Import resolution verified successfully
All debounced query functions are properly exported from
client/apps/game/src/dojo/debounced-queries.tsand are being used successfully across multiple components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all new imports exist and are exported correctly rg -l "export.*\b(debouncedGetDonkeysAndArmiesFromTorii|debouncedGetEntitiesFromTorii|debouncedGetMarketFromTorii|debouncedGetOneKeyEntitiesByRealmEntityIdFromTorii)\b" "src/dojo/"Length of output: 230
Script:
#!/bin/bash # Search for the debounced function definitions across the repository rg -A 2 "export.*\b(debouncedGetDonkeysAndArmiesFromTorii|debouncedGetEntitiesFromTorii|debouncedGetMarketFromTorii|debouncedGetOneKeyEntitiesByRealmEntityIdFromTorii)\b" # Find any files containing these function names to understand the context rg "\b(debouncedGetDonkeysAndArmiesFromTorii|debouncedGetEntitiesFromTorii|debouncedGetMarketFromTorii|debouncedGetOneKeyEntitiesByRealmEntityIdFromTorii)\b"Length of output: 2999
| "s1_eternum-LeaderboardRegisterContribution", | ||
| "s1_eternum-LeaderboardRegisterShare", | ||
| "s1_eternum-CapacityConfigCategory", | ||
| "s1_eternum-ResourceCost", |
| "s1_eternum-LeaderboardRegisterShare", | ||
| "s1_eternum-Epoch", | ||
| "s1_eternum-Progress", | ||
| "s1_eternum-LeaderboardRegisterContribution", |
There was a problem hiding this comment.
Leaderboard models already fetched in previous getEntities
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/apps/game/src/dojo/sync.ts (2)
104-109:⚠️ Potential issueRemove duplicate ResourceCost model.
The "s1_eternum-ResourceCost" model appears twice in the array.
Apply this diff to remove the duplicate:
"s1_eternum-ResourceCost", "s1_eternum-LeaderboardRegisterContribution", "s1_eternum-LeaderboardRegisterShare", - "s1_eternum-CapacityConfigCategory", - "s1_eternum-ResourceCost", + "s1_eternum-CapacityConfigCategory"
149-153:⚠️ Potential issueRemove redundant leaderboard model fetches.
These leaderboard models are already fetched in the previous getEntities call.
Remove the redundant models:
- "s1_eternum-LeaderboardRegisterShare", "s1_eternum-Epoch", "s1_eternum-Progress", - "s1_eternum-LeaderboardRegisterContribution",
🧹 Nitpick comments (1)
client/apps/game/src/dojo/sync.ts (1)
81-82: Consider using structured logging for better performance analysis.The performance logging is helpful but could be improved by using a consistent structured format that would make it easier to analyze query patterns and performance trends.
Consider using an object structure for logging:
-console.log("[composite] big config query", end - start); +console.log({ + type: "query_performance", + name: "big_config_query", + duration_ms: end - start, + timestamp: new Date().toISOString() +});Also applies to: 115-116, 122-122, 160-161, 164-164, 182-183, 191-191, 222-223
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/apps/game/src/dojo/sync.ts(2 hunks)
🔇 Additional comments (2)
client/apps/game/src/dojo/sync.ts (2)
264-320: Well-organized model constants improve maintainability.Good job organizing the models into separate constants. This improves code maintainability and follows the DRY principle.
97-98: Verify the performance impact of using "VariableLen" pattern matching.Changing from "FixedLen" to "VariableLen" pattern matching could affect query performance as it allows for more flexible but potentially slower key matching.
Run this script to analyze the query patterns and their performance impact:
✅ Verification successful
Pattern matching change aligns with codebase patterns
The change to "VariableLen" follows established patterns in the codebase, particularly in queries.ts. Performance monitoring is already in place, and the mixed usage of both patterns suggests intentional design choices based on specific query requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the usage of VariableLen vs FixedLen pattern matching # and their performance implications # Search for all pattern matching usages echo "Pattern Matching Usage Analysis:" rg -A 2 'pattern_matching:' --type ts # Search for any performance-related comments or issues echo -e "\nPerformance Related Context:" rg -i 'performance|slow|optimization' --type tsLength of output: 7740
Summary by CodeRabbit
Refactor
Bug Fixes
Chores