Raschel sync #2555
Raschel sync #2555
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces modifications across several client-side files, focusing on entity fetching, state management, and system logging. The changes primarily impact the Changes
Possibly related PRs
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 (
|
|
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: 3
🧹 Nitpick comments (3)
client/src/ui/layouts/World.tsx (1)
127-144: Consider error retry mechanism for fetch operations.The fetch operation could benefit from a retry mechanism for transient failures. Currently, it only logs the error and sets loading states to false.
Consider implementing exponential backoff:
const fetch = async () => { + const maxRetries = 3; + const backoff = (attempt: number) => Math.min(1000 * Math.pow(2, attempt), 5000); + let attempt = 0; + try { await Promise.all([ addToSubscription( dojo.network.toriiClient, dojo.network.contractComponents as any, [structureEntityId.toString()], [{ x: position?.x || 0, y: position?.y || 0 }], ), ]); } catch (error) { console.error("Fetch failed", error); + if (attempt < maxRetries) { + attempt++; + setTimeout(() => fetch(), backoff(attempt)); + return; + } } finally { setWorldLoading(false); setMarketLoading(false); } };client/src/three/scenes/Worldmap.ts (2)
737-790: Consider optimizing position entity fetching and enhancing error handling.While the parallel fetching of position entities is a good addition, consider the following improvements:
- The large number of components being fetched (6 components) might impact performance. Consider:
- Fetching only essential components initially
- Implementing lazy loading for less critical components
- Add specific error handling for position entities to distinguish between tile and position fetch failures
const promisePositions = getEntities( this.dojo.network.toriiClient, { Composite: { operator: "And", clauses: [ // ... existing clauses ... ], }, }, this.dojo.network.contractComponents as any, [], - [ - "s0_eternum-Army", - "s0_eternum-Position", - "s0_eternum-Health", - "s0_eternum-EntityOwner", - "s0_eternum-Protectee", - "s0_eternum-Stamina", - ], + // Initial fetch of essential components + ["s0_eternum-Position", "s0_eternum-Army"], 1000, false, );
791-792: Remove commented-out code.The commented Promise.all code is no longer needed since we're now handling both tiles and positions. Remove it to maintain code clarity.
Promise.all([promiseTiles, promisePositions]).then(([tiles, positions]) => { - // Promise.all([promiseTiles]).then(([tiles]) => { });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/src/dojo/setup.ts(1 hunks)client/src/three/scenes/Worldmap.ts(1 hunks)client/src/three/systems/SystemManager.ts(1 hunks)client/src/ui/layouts/World.tsx(4 hunks)
client/src/dojo/setup.ts
Outdated
| // improve | ||
| // "s0_eternum-Army", | ||
| "s0_eternum-Structure", | ||
| "s0_eternum-Battle", | ||
| "s0_eternum-EntityOwner", | ||
| // improve | ||
| // "s0_eternum-EntityOwner", |
There was a problem hiding this comment.
💡 Codebase verification
Both Army and EntityOwner components are actively used in the codebase
The components marked for improvement are actually critical parts of the system:
Armycomponent is used in battle systems and army-related operations (create, delete, buy troops, merge troops)EntityOwnercomponent is used in conjunction with Army components for ownership tracking
Commenting them out in the entity fetch configuration (setup.ts) while they're still actively used elsewhere could lead to synchronization issues.
- Remove the "improve" comments and uncomment these components in
client/src/dojo/setup.ts - If improvements are needed, create separate tickets to track the improvements while keeping the components functional
🔗 Analysis chain
Clarify the improvement plan for commented-out entities.
The commented-out entities "s0_eternum-Army" and "s0_eternum-EntityOwner" are marked with "improve" comments, but there's no clear indication of the improvement plan or why they were commented out.
Let's verify if these entities are still being used:
Would you like me to help create a GitHub issue to track the improvement plan for these entities?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Army and EntityOwner usage
rg -A 3 "s0_eternum-Army|s0_eternum-EntityOwner" --type ts --type tsx
Length of output: 99
Script:
#!/bin/bash
# Search for Army and EntityOwner usage with correct file type
rg -A 3 "s0_eternum-Army|s0_eternum-EntityOwner" --type typescript
# Also search for any references to these components in a case-insensitive manner
rg -i "army|entityowner" --type typescript
# Check if these components exist in any system definitions
fd "system" -e ts -x cat {} | rg -A 3 "Army|EntityOwner"
Length of output: 1761
Script:
#!/bin/bash
# Let's check the component definitions
fd "component" -e ts -x cat {} | rg -A 3 "Army|EntityOwner"
# Let's also check the setup file content to understand the context better
cat client/src/dojo/setup.ts
Length of output: 7823
client/src/ui/layouts/World.tsx
Outdated
| useEffect(() => { | ||
| const structuresEntityIds = structures.map((structure) => structure.entity_id.toString()); | ||
|
|
||
| const fetch = async () => { | ||
| try { | ||
| await Promise.all([ | ||
| addToSubscriptionOneKeyModelbyRealmEntityId( | ||
| dojo.network.toriiClient, | ||
| dojo.network.contractComponents as any, | ||
| [...filteredStructures.map((structure) => structure.entity_id.toString())], | ||
| structuresEntityIds, | ||
| ), | ||
| addToSubscription( | ||
| addToSubscriptionTwoKeyModelbyRealmEntityId( | ||
| dojo.network.toriiClient, | ||
| dojo.network.contractComponents as any, | ||
| [structureEntityId.toString()], | ||
| [{ x: position?.x || 0, y: position?.y || 0 }], | ||
| structuresEntityIds, | ||
| ), | ||
| addToSubscription(dojo.network.toriiClient, dojo.network.contractComponents as any, structuresEntityIds, [ | ||
| ...structures.map((structure) => ({ x: structure.position.x, y: structure.position.y })), | ||
| ]), | ||
| ]); | ||
| } catch (error) { | ||
| console.error("Fetch failed", error); | ||
| } finally { | ||
| setWorldLoading(false); | ||
| setMarketLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetch(); | ||
| }, [structures]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize batch fetching for structures.
The current implementation makes multiple API calls in parallel for each structure. Consider batching these calls to reduce network overhead.
Consider consolidating the API calls:
const fetch = async () => {
try {
+ const batchSize = 50;
+ const batches = [];
+ for (let i = 0; i < structuresEntityIds.length; i += batchSize) {
+ const batch = structuresEntityIds.slice(i, i + batchSize);
await Promise.all([
addToSubscriptionOneKeyModelbyRealmEntityId(
dojo.network.toriiClient,
dojo.network.contractComponents as any,
- structuresEntityIds,
+ batch,
),
addToSubscriptionTwoKeyModelbyRealmEntityId(
dojo.network.toriiClient,
dojo.network.contractComponents as any,
- structuresEntityIds,
+ batch,
),
]);
+ }
} catch (error) {
console.error("Fetch failed", error);
}
};Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| console.log({ armyUpdates: army }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or conditionally enable debug logging.
The console.log statement for army updates could impact performance and clutter the console in production.
Consider using a debug flag:
+const DEBUG_ARMY_UPDATES = process.env.NODE_ENV === 'development';
const army = getComponentValue(this.setup.components.Army, update.entity);
-console.log({ armyUpdates: army });
+if (DEBUG_ARMY_UPDATES) {
+ console.log({ armyUpdates: army });
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/dojo/queries.ts (2)
158-159: Improve logging consistency and information.The logging messages are inconsistent and could be more informative:
- Start message uses "ArrivalsEnd: starting" while end message uses just "ArrivalsEnd"
- Missing important context like the number of entities processed
Consider this improvement:
- console.log("ArrivalsEnd: starting resource arrivals"); + console.log("Arrivals: Starting resource arrivals for", entityIds.length, "entities"); // ... query execution ... - console.log("ArrivalsEnd", end - start); + console.log("Arrivals: Completed in", (end - start).toFixed(2), "ms");Also applies to: 203-204
153-205: Consider code reuse opportunities.The function shares similar patterns with existing functions like
addToSubscriptionandaddMarketSubscription. Consider extracting common functionality:
- Performance monitoring
- Entity fetching with error handling
- Logging
Consider creating utility functions:
// Utility for performance monitoring and logging async function withPerformanceLogging<T>( operation: string, fn: () => Promise<T> ): Promise<T> { const start = performance.now(); console.log(`${operation}: Starting...`); try { const result = await fn(); const end = performance.now(); console.log(`${operation}: Completed in ${(end - start).toFixed(2)} ms`); return result; } catch (error) { console.error(`${operation}: Failed:`, error); throw error; } } // Example usage: export const addArrivalsSubscription = async <S extends Schema>( entityIds: number[], client: ToriiClient, components: Component<S, Metadata, undefined>[], ) => { return withPerformanceLogging('Arrivals', () => getEntities(/* ... */)); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/dojo/queries.ts(1 hunks)
🔇 Additional comments (2)
client/src/dojo/queries.ts (2)
153-157: LGTM! Function signature is well-defined.
The function signature follows the established pattern and clearly indicates its purpose through proper naming and typing.
160-202: 🛠️ Refactor suggestion
Consider query limitations and error handling.
A few concerns about the query implementation:
- The hard limit of 1000 results might truncate data if there are more arrivals.
- The nested AND operators could impact query performance.
- Missing error handling for the async operation.
Let's verify the potential impact of the result limit:
Consider these improvements:
export const addArrivalsSubscription = async <S extends Schema>(
entityIds: number[],
client: ToriiClient,
components: Component<S, Metadata, undefined>[],
) => {
const start = performance.now();
console.log("ArrivalsEnd: starting resource arrivals");
- await getEntities(
- client,
- {
- // ... query configuration ...
- },
- components,
- [],
- [/* models */],
- 1000,
- false,
- );
+ try {
+ // Paginate results if needed
+ const batchSize = 1000;
+ let offset = 0;
+ let allResults = [];
+
+ while (true) {
+ const results = await getEntities(
+ client,
+ {
+ // ... same query configuration ...
+ },
+ components,
+ [],
+ [/* same models */],
+ batchSize,
+ false,
+ );
+
+ if (!results || results.length === 0) break;
+ allResults = allResults.concat(results);
+ if (results.length < batchSize) break;
+ offset += batchSize;
+ }
+ } catch (error) {
+ console.error("Failed to fetch arrivals:", error);
+ throw error;
+ }
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
client/src/ui/layouts/World.tsx (1)
Line range hint
141-151: Consider adding error handling for loading states.The error case doesn't reset the loading state, which could leave the UI in a perpetual loading state if an error occurs.
} catch (error) { console.error("Fetch failed", error); + setLoading(LoadingStateKey.SelectedStructure, false); }
♻️ Duplicate comments (1)
client/src/ui/layouts/World.tsx (1)
162-200: 🛠️ Refactor suggestionOptimize batch fetching for structures.
The current implementation makes multiple API calls in parallel for each structure. Consider batching these calls to reduce network overhead.
🧹 Nitpick comments (9)
client/src/ui/components/trading/ResourceArrivals.tsx (2)
14-14: Consider making DISPLAYED_ARRIVALS configurableThe constant DISPLAYED_ARRIVALS could be made configurable through props to allow different display counts in different contexts.
-const DISPLAYED_ARRIVALS = 3; +interface ResourceArrivalsProps { + arrivals: ArrivalInfo[]; + className?: string; + initialDisplayCount?: number; +} export const AllResourceArrivals = memo( - ({ arrivals, className = "" }: { arrivals: ArrivalInfo[]; className?: string }) => { + ({ arrivals, className = "", initialDisplayCount = 3 }: ResourceArrivalsProps) => { const dojo = useDojo(); - const [displayCount, setDisplayCount] = useState(DISPLAYED_ARRIVALS); + const [displayCount, setDisplayCount] = useState(initialDisplayCount);Also applies to: 31-32
56-65: Consider memoizing filtered arrivalsThe filtering operation could be memoized to prevent unnecessary recalculations on re-renders.
- const filteredArrivals = showOnlyArrived - ? arrivals.filter((arrival) => arrival.arrivesAt < nextBlockTimestamp) - : arrivals; + const filteredArrivals = useMemo(() => + showOnlyArrived + ? arrivals.filter((arrival) => arrival.arrivesAt < nextBlockTimestamp) + : arrivals, + [arrivals, showOnlyArrived, nextBlockTimestamp] + );client/src/dojo/debouncedQueries.ts (1)
133-140: Consider adding request type to error loggingThe market subscription error handling could be more specific by including the operation type in logs.
async <S extends Schema>( client: ToriiClient, components: Component<S, Metadata, undefined>[], onComplete?: () => void, ) => { - await marketQueue.add(() => addMarketSubscription(client, components), onComplete); + await marketQueue.add( + async () => { + try { + await addMarketSubscription(client, components); + } catch (error) { + console.error('Market subscription failed:', error); + throw error; + } + }, + onComplete + ); },client/src/dojo/queries.ts (1)
194-220: Consider pagination for large realmsThe query has a hard limit of 1000 entities which might be insufficient for large realms. Consider:
- Implementing pagination
- Monitoring if the limit is ever reached in production
client/src/dojo/setup.ts (1)
Line range hint
33-34: Fix misleading comment about debounce timeThe comment states "Increased debounce time to 1 second" but the actual value is 200ms.
- }, 200); // Increased debounce time to 1 second for larger batches + }, 200); // Debounce time of 200ms for batch updatesclient/src/ui/modules/navigation/QuestMenu.tsx (1)
107-111: Consider consolidating loading state checks.The component has multiple repeated checks for
questsLoadedacross different buttons. This could be simplified to reduce code duplication.+ const buttonDisabled = questsLoaded; + const loadingText = questsLoaded ? "Loading..." : undefined; - disabled={questsLoaded} + disabled={buttonDisabled} - {questsLoaded ? "Loading..." : "Claim"} + {loadingText || "Claim"}Also applies to: 119-126, 137-144, 167-169, 175-177, 183-185, 193-195
client/src/ui/layouts/World.tsx (1)
204-217: Add retry mechanism for critical subscriptions.Market and bank subscriptions are critical for functionality. Consider adding a retry mechanism for failed subscriptions.
+ const MAX_RETRIES = 3; + const retry = async (fn: () => Promise<void>, retries = MAX_RETRIES) => { + try { + await fn(); + } catch (error) { + if (retries > 0) { + console.warn(`Retrying subscription... ${MAX_RETRIES - retries + 1}/${MAX_RETRIES}`); + await retry(fn, retries - 1); + } else { + throw error; + } + } + };client/src/three/scenes/Worldmap.ts (2)
679-679: Remove debug console.log statement.Debug logging should not be committed to production code.
- console.log({ chunkKey });
794-795: Consider adding retry logic for failed entity fetches.The current error handling simply removes the chunk from the cache, but a more robust solution would include retry logic with exponential backoff.
Would you like me to provide an implementation of a retry mechanism with exponential backoff?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
client/src/App.tsx(2 hunks)client/src/dojo/debouncedQueries.ts(6 hunks)client/src/dojo/queries.ts(2 hunks)client/src/dojo/setup.ts(5 hunks)client/src/hooks/store/useUIStore.tsx(3 hunks)client/src/hooks/store/useWorldLoading.tsx(1 hunks)client/src/main.tsx(2 hunks)client/src/three/scenes/Worldmap.ts(4 hunks)client/src/three/systems/SystemManager.ts(1 hunks)client/src/ui/components/WorldLoading.tsx(1 hunks)client/src/ui/components/trading/MarketModal.tsx(0 hunks)client/src/ui/components/trading/ResourceArrivals.tsx(4 hunks)client/src/ui/components/worldmap/armies/SelectedArmy.tsx(1 hunks)client/src/ui/layouts/World.tsx(5 hunks)client/src/ui/modules/navigation/QuestMenu.tsx(5 hunks)client/src/ui/modules/world-structures/WorldStructuresMenu.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- client/src/ui/components/trading/MarketModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/three/systems/SystemManager.ts
🔇 Additional comments (25)
client/src/hooks/store/useWorldLoading.tsx (4)
1-18: Excellent introduction of an enum for loading states.
Using a dedicated enum to represent different loading keys improves code readability and helps avoid hard-coded strings scattered across the codebase. This approach makes it easier to standardize and reference loading states.
20-22: Great approach to define a strongly-typed object for loading states.
Mapping each enum key to a boolean in the type ensures that all loading states are trackable, avoiding missing or extra properties.
24-26: Clear separation of concerns.
Having a dedicated interface (WorldStore) that includes both the loading states and the setter function provides a cohesive design. The type signatures are clear and straightforward to use.
29-52: Effective store slice creation.
The pattern of using “createWorldStoreSlice” to initialize the loading states and provide a consistent setter function is a clean approach. It adheres to the DRY principle by consolidating logic for state updates into a single place.
client/src/App.tsx (1)
4-4: Injecting the WorldLoading component is a good idea.
Placing in the main App ensures the overlay is always available whenever loading flags are set. This seamlessly integrates with the rest of the UI without cluttering individual screens.
Also applies to: 13-13
client/src/ui/components/WorldLoading.tsx (4)
1-3: Well-structured imports.
Importing the store and the enum from the hooks is straightforward, keeping the component’s dependencies minimal.
4-8: Readability and clarity.
Using “anyLoading” as a single boolean to indicate the presence of ongoing loads simplifies the conditional rendering logic, making it clear when the overlay should be displayed.
9-24: Good approach to itemized loading updates.
By mapping loading states to descriptive labels, you provide clarity to the user, showing precisely which items are in progress. This granular approach helps with debugging and user experience.
26-46: Dynamic styling and transitions are well-handled.
The overlay’s style ensures it appears and disappears smoothly. The transition logic is easy to read, and toggling via “anyLoading” is straightforward.
client/src/main.tsx (3)
16-16: Logical place for the UI store import.
Importing useUIStore here allows for late initialization or usage of global state before starting the main setup flow. This keeps the main file self-contained.
66-67: Capturing the current state for setup is consistent with the new store design.
Utilizing “useUIStore.getState()” ensures that your setup function has up-to-date references to the global loading states.
69-69: Clear organization of setup arguments.
Spreading “dojoConfig” and passing it with “state” nicely aligns with the updated signature in “setup”. This maintains backward compatibility while enabling new capabilities.
client/src/ui/components/worldmap/armies/SelectedArmy.tsx (2)
66-74: LGTM: Clean className template literal structure
The className template literal is well-structured with proper spacing and clear conditional logic.
75-93: Verify the tooltip visibility behavior during loading states
While removing the loading state dependency simplifies the code, we should ensure the tooltip doesn't flash or appear prematurely during data loading.
✅ Verification successful
Based on the search results, I can now generate a final response since I have enough context about the tooltip visibility handling:
Tooltip visibility is properly controlled by multiple conditions
The tooltip visibility is already well-managed through multiple conditions:
- Only shows when there's a selected hex (
selectedHex) - Requires an owned army to be present (
ownArmy) - Must be in map view (
isMapView) - Has proper CSS transitions for smooth appearance/disappearance
These conditions effectively prevent premature or flashy appearances without needing an explicit loading state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might handle the loading state for this component
ast-grep --pattern 'const $loading = useUIStore((state) => state.$$$)'
Length of output: 72
Script:
#!/bin/bash
# Let's search for loading state patterns and tooltip visibility handling in the component
rg -A 5 "showTooltip" client/src/ui/components/worldmap/armies/SelectedArmy.tsx
# Also search for any state management related to this component
rg -A 5 "useUIStore" client/src/ui/components/worldmap/armies/SelectedArmy.tsx
# Check how the component is initialized and what props/states it uses
ast-grep --pattern 'const SelectedArmy = ($_) => {
$$$
}'
Length of output: 1498
client/src/dojo/debouncedQueries.ts (1)
105-114: Ensure consistent debounce timing across resource operations
The new debounceAddResourceArrivals function uses the same timing as subscription operations (250ms). Consider if resource arrivals need different timing based on their usage patterns.
✅ Verification successful
Let me search for other debounce timings in the codebase to make a proper comparison.
Let me check the specific timing values for these debounced functions.
Debounce timing for resource arrivals is consistent with related operations
The debounce timing of 250ms for debounceAddResourceArrivals is consistent with other subscription-related operations in the codebase:
- Position sync operations use 100ms (more time-sensitive)
- General subscription operations use 250ms (including resource arrivals)
- Market subscription operations use 500ms (less time-sensitive)
The current timing appears appropriate as resource arrivals are part of the subscription system and share the same queue (subscriptionQueue).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for resource arrival related operations to verify timing consistency
rg -A 2 'debounce.*Resource' --type ts
Length of output: 676
Script:
#!/bin/bash
# Search for all debounce operations and their timings
rg -B 2 -A 2 'debounce\(' --type ts
Length of output: 2166
Script:
#!/bin/bash
# Search for debounce timing values
rg -B 5 -A 1 '\d+\s*,' --type ts client/src/dojo/debouncedQueries.ts
Length of output: 1462
client/src/dojo/queries.ts (1)
139-143: LGTM! Improved query efficiency
The change from Keys to Member query with resource_amount > 0 filter is more efficient as it filters at the query level rather than client-side.
client/src/hooks/store/useUIStore.tsx (1)
13-13: LGTM! Clean integration of WorldStore
The WorldStore integration follows the established pattern of store composition and maintains consistency with existing store management.
Also applies to: 74-74, 143-143
client/src/dojo/setup.ts (2)
120-146: LGTM! Robust error handling with loading states
The implementation properly manages loading states with try/finally blocks, ensuring loading indicators are always cleared even if errors occur.
177-179: LGTM! Consistent loading state management
The loading state management is consistently implemented across different operations using finally blocks.
Also applies to: 213-215
client/src/ui/modules/navigation/QuestMenu.tsx (2)
21-21: LGTM: Loading state management is properly initialized.
The loading state is correctly retrieved from the UI store using the specific key for player structures.
82-83: Verify mouse event handling with loading states.
The mouse event handlers now check questsLoaded instead of the previous loading state. Ensure that tooltip behavior remains consistent during loading and loaded states.
Also applies to: 123-124
✅ Verification successful
Tooltip behavior is correctly implemented with loading states
The code shows consistent tooltip handling with loading states:
- The tooltip is only shown when
!questsLoaded(line:onMouseLeave={() => !questsLoaded && setTooltip(null)}) - The claim button is disabled during loading (line:
disabled={currentQuest?.status !== QuestStatus.Completed || questsLoaded}) - All interactive elements consistently check the
questsLoadedstate and are properly disabled during loading - The UI provides clear loading feedback with "Loading..." text across all elements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tooltip implementations to verify consistency
rg -A 3 'setTooltip.*null.*loading'
Length of output: 37
Script:
#!/bin/bash
# Search for tooltip-related code in QuestMenu.tsx and surrounding context
rg -A 5 "tooltip" client/src/ui/modules/navigation/QuestMenu.tsx
# Search for mouse event handlers in QuestMenu.tsx
rg -A 5 "onMouse" client/src/ui/modules/navigation/QuestMenu.tsx
# Look for loading state handling patterns
rg -A 5 "questsLoaded" client/src/ui/modules/navigation/QuestMenu.tsx
Length of output: 3908
client/src/ui/layouts/World.tsx (1)
106-106: LGTM: Loading state management is properly initialized.
The loading state setter is correctly retrieved from the UI store.
client/src/ui/modules/world-structures/WorldStructuresMenu.tsx (2)
37-38: LGTM: Loading state management is properly initialized.
The loading states are correctly retrieved and managed through the UI store.
349-349: LGTM: Callback-based completion handling.
The function now properly uses a callback for completion notification instead of directly managing loading state.
Also applies to: 385-385
client/src/three/scenes/Worldmap.ts (1)
740-793: LGTM: Well-structured entity query.
The position entities query is well-structured with comprehensive component selection. The query includes essential components like Army, Position, Health, EntityOwner, Protectee, and Stamina.
| async add(request: () => Promise<void>, onComplete?: () => void) { | ||
| this.queue.push(async () => { | ||
| await request(); | ||
| onComplete?.(); // Call onComplete after the request is processed | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for onComplete callback
The onComplete callback should be protected from errors to prevent queue processing interruption.
async add(request: () => Promise<void>, onComplete?: () => void) {
this.queue.push(async () => {
- await request();
- onComplete?.();
+ try {
+ await request();
+ onComplete?.();
+ } catch (error) {
+ console.error('Request failed:', error);
+ throw error; // Re-throw to be caught by processQueue
+ }
});📝 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.
| async add(request: () => Promise<void>, onComplete?: () => void) { | |
| this.queue.push(async () => { | |
| await request(); | |
| onComplete?.(); // Call onComplete after the request is processed | |
| }); | |
| async add(request: () => Promise<void>, onComplete?: () => void) { | |
| this.queue.push(async () => { | |
| try { | |
| await request(); | |
| onComplete?.(); | |
| } catch (error) { | |
| console.error('Request failed:', error); | |
| throw error; // Re-throw to be caught by processQueue | |
| } | |
| }); |
| // todo: waiting on ghlim to check issue with this query | ||
| // { | ||
| // Composite: { | ||
| // operator: "And", | ||
| // clauses: [ | ||
| // { | ||
| // Composite: { | ||
| // operator: "Or", | ||
| // clauses: entityIds.map((id) => ({ | ||
| // Member: { | ||
| // model: "s0_eternum-EntityOwner", | ||
| // member: "entity_owner_id", | ||
| // operator: "Eq", | ||
| // value: { Primitive: { U32: id } }, | ||
| // }, | ||
| // })), | ||
| // }, | ||
| // }, | ||
| // { | ||
| // Member: { | ||
| // model: "s0_eternum-OwnedResourcesTracker", | ||
| // member: "resource_types", | ||
| // operator: "Neq", | ||
| // value: { Primitive: { U256: "0" } }, | ||
| // }, | ||
| // }, | ||
| // ], | ||
| // }, | ||
| // }, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered evidence, I can now provide a final response:
The commented-out query with OwnedResourcesTracker filtering is necessary
The codebase analysis reveals that OwnedResourcesTracker is a crucial component used to track resource ownership across the system:
- It's used in multiple places to filter entities that have resources (
NotValue(OwnedResourcesTracker, { resource_types: 0n })), indicating it's an important filtering condition - The component maintains a bitmap of owned resource types and is actively used in resource transfer operations (as seen in contracts/src/models/resources.cairo)
- The simpler query without the OwnedResourcesTracker check could include entities that don't actually have any resources, leading to incorrect resource tracking
Action items:
- Restore the commented-out query to ensure proper resource tracking
- Remove the TODO comment and implement the complete query with OwnedResourcesTracker filtering
- If there's a specific issue with the query, it should be documented in a proper issue tracker rather than a TODO comment
🔗 Analysis chain
Verify if the simplified query is sufficient
The commented-out code shows a more complex query that includes additional filtering for OwnedResourcesTracker. While waiting for the issue to be resolved:
- Document why the simpler query was chosen as temporary solution
- Create a tracking issue for implementing the complete query once the blocker is resolved
Let's verify if the simpler query might miss important cases:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are cases where OwnedResourcesTracker is crucial
rg -A 3 "OwnedResourcesTracker" --type typescript
Length of output: 84
Script:
#!/bin/bash
# Let's try without file type restriction since the ripgrep version might not support typescript
rg -A 3 "OwnedResourcesTracker"
Length of output: 16197
Script:
#!/bin/bash
# Search for any references to resources or tracking in the queries file
rg -A 5 "resource" client/src/dojo/queries.ts
Length of output: 549
Script:
#!/bin/bash
# Look for the actual query implementation
ast-grep --pattern 'addArrivalsSubscription($$$)'
Length of output: 197
Script:
#!/bin/bash
# Let's look at the actual implementation of addArrivalsSubscription
rg -B 5 -A 10 "export const addArrivalsSubscription" client/src/dojo/queries.ts
Length of output: 529
Script:
#!/bin/bash
# Check how OwnedResourcesTracker is used in the codebase
ast-grep --pattern 'NotValue(OwnedResourcesTracker, { resource_types: $_})'
Length of output: 303
| @@ -142,7 +141,7 @@ export const WorldStructuresMenu = ({ className }: { className?: string }) => { | |||
| [selectedTab, hyperstructures, fragmentMines, showOnlyMine, account.address, myHyperstructures], | |||
| ); | |||
|
|
|||
| if (isStructuresLoading) { | |||
| if (hyperstructuresLoaded) { | |||
There was a problem hiding this comment.
Verify loading state condition.
The loading condition seems inverted. The loading UI is shown when hyperstructuresLoaded is true, which is counterintuitive.
- if (hyperstructuresLoaded) {
+ if (!hyperstructuresLoaded) {📝 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.
| if (hyperstructuresLoaded) { | |
| if (!hyperstructuresLoaded) { |
| setLoading(LoadingStateKey.Hyperstructure, false), | ||
| await fetchHyperstructureData(toriiClient, contractComponents as any, hyperstructuresLoaded, () => | ||
| setLoading(LoadingStateKey.Hyperstructure, true), | ||
| ); |
There was a problem hiding this comment.
Fix incorrect loading state logic.
The loading states are being set in the wrong order. The state should be set to true before fetching and false after completion.
- setLoading(LoadingStateKey.Hyperstructure, false),
- await fetchHyperstructureData(toriiClient, contractComponents as any, hyperstructuresLoaded, () =>
- setLoading(LoadingStateKey.Hyperstructure, true),
+ setLoading(LoadingStateKey.Hyperstructure, true);
+ await fetchHyperstructureData(toriiClient, contractComponents as any, hyperstructuresLoaded, () =>
+ setLoading(LoadingStateKey.Hyperstructure, false),📝 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.
| setLoading(LoadingStateKey.Hyperstructure, false), | |
| await fetchHyperstructureData(toriiClient, contractComponents as any, hyperstructuresLoaded, () => | |
| setLoading(LoadingStateKey.Hyperstructure, true), | |
| ); | |
| setLoading(LoadingStateKey.Hyperstructure, true); | |
| await fetchHyperstructureData(toriiClient, contractComponents as any, hyperstructuresLoaded, () => | |
| setLoading(LoadingStateKey.Hyperstructure, false), | |
| ); |
| @@ -687,6 +689,7 @@ export default class WorldmapScene extends HexagonScene { | |||
| console.log(startCol, startRow, range); | |||
|
|
|||
| try { | |||
| this.state.setLoading(LoadingStateKey.Map, true); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for loading state.
The loading state is correctly managed but should handle errors gracefully to ensure the loading state is always cleared.
try {
this.state.setLoading(LoadingStateKey.Map, true);
const promiseTiles = getEntities(...);
const promisePositions = getEntities(...);
- Promise.all([promiseTiles, promisePositions]).then(() => {
- this.state.setLoading(LoadingStateKey.Map, false);
- });
+ await Promise.all([promiseTiles, promisePositions]);
} catch (error) {
this.fetchedChunks.delete(chunkKey);
console.error("Error fetching tile entities:", error);
+ throw error;
+ } finally {
+ this.state.setLoading(LoadingStateKey.Map, false);
}Also applies to: 794-795
Summary by CodeRabbit
New Features
Bug Fixes
Chores