-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix sync #2469
fix sync #2469
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 16 at 08:00 AM. |
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
client/src/ui/layouts/World.tsx (1)
143-147
: Separate market subscription into its own useEffectThe market subscription logic should be separated from the structure-specific useEffect for several reasons:
- Market subscription is independent of structure changes
- Current placement causes unnecessary retries on structure changes
- Loading state isn't properly handled for market subscription failures
Consider refactoring to:
+ // Separate useEffect for market subscription + useEffect(() => { + const fetchMarketData = async () => { + try { + setWorldLoading(true); + await addMarketSubscription(dojo.network.toriiClient, dojo.network.contractComponents as any); + } catch (error) { + console.error("Market fetch failed:", error); + } finally { + setWorldLoading(false); + } + }; + + fetchMarketData(); + }, []); // Run once on component mount useEffect(() => { if (!structureEntityId || subscriptions[structureEntityId.toString()] || structureEntityId === 999999998) { return; } const position = getComponentValue( dojo.setup.components.Position, getEntityIdFromKeys([BigInt(structureEntityId)]), ); setWorldLoading(true); setSubscriptions((prev) => ({ ...prev, [structureEntityId.toString()]: true })); const fetch = async () => { try { await 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); - } finally { - setWorldLoading(false); - } - - try { - await addMarketSubscription(dojo.network.toriiClient, dojo.network.contractComponents as any); - } catch (error) { - console.error("Fetch failed", error); - } + } catch (error) { + console.error("Structure fetch failed:", error); + } finally { + setWorldLoading(false); + } }; fetch(); }, [structureEntityId, subscriptions, setWorldLoading, setSubscriptions]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/dojo/queries.ts
(2 hunks)client/src/ui/layouts/World.tsx
(2 hunks)
🔇 Additional comments (1)
client/src/dojo/queries.ts (1)
69-78
: LGTM: Conditional entity fetching based on position
The conditional execution of getEntities
based on position presence fixes unnecessary fetches. The implementation correctly handles optional position parameter.
Let's verify the impact of this change:
✅ Verification successful
Let me check the implementation of addToSubscription
to understand how it handles the position parameter.
Let me try a different approach to find the implementation.
Let me check the complete implementation of the function to understand how it uses the position parameter.
The conditional entity fetching change is safe and correct
Based on the verification:
- The
addToSubscription
function properly handles the optional position parameter in both its signature and implementation - All calls to
addToSubscription
found in the codebase pass the position parameter as optional, matching the function's signature - The conditional execution of
getEntities
is correctly guarded by the position check, preventing unnecessary fetches when position is not provided
The change aligns with the function's design and its usage across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to addToSubscription to ensure they handle optional position correctly
rg -A 3 "addToSubscription\("
Length of output: 1709
Script:
#!/bin/bash
# Search for the implementation of addToSubscription
ast-grep --pattern 'export const addToSubscription = async $_'
Length of output: 485
Script:
#!/bin/bash
# Search for the implementation of addToSubscription using ripgrep
rg -A 10 "export const addToSubscription"
Length of output: 1485
Script:
#!/bin/bash
# Get the complete implementation of addToSubscription
rg -A 20 "export const addToSubscription" client/src/dojo/queries.ts
Length of output: 614
export const addMarketSubscription = async <S extends Schema>( | ||
client: ToriiClient, | ||
components: Component<S, Metadata, undefined>[], | ||
) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for getEntities failure
The new market subscription function should handle potential failures from getEntities
to prevent silent failures.
Consider wrapping the call with try-catch:
export const addMarketSubscription = async <S extends Schema>(
client: ToriiClient,
components: Component<S, Metadata, undefined>[],
) => {
+ try {
await getEntities(
client,
{
Keys: {
keys: [undefined, undefined],
pattern_matching: "FixedLen",
models: ["s0_eternum-DetachedResource"],
},
},
components,
10_000,
false,
);
+ } catch (error) {
+ console.error("Failed to fetch market entities:", error);
+ throw error; // Re-throw to allow caller to handle the error
+ }
};
📝 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.
export const addMarketSubscription = async <S extends Schema>( | |
client: ToriiClient, | |
components: Component<S, Metadata, undefined>[], | |
) => { | |
export const addMarketSubscription = async <S extends Schema>( | |
client: ToriiClient, | |
components: Component<S, Metadata, undefined>[], | |
) => { | |
try { | |
await getEntities( | |
client, | |
{ | |
Keys: { | |
keys: [undefined, undefined], | |
pattern_matching: "FixedLen", | |
models: ["s0_eternum-DetachedResource"], | |
}, | |
}, | |
components, | |
10_000, | |
false, | |
); | |
} catch (error) { | |
console.error("Failed to fetch market entities:", error); | |
throw error; // Re-throw to allow caller to handle the error | |
} | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
client/src/ui/components/trading/MarketModal.tsx (1)
207-207
: Consider enhancing loading indicator accessibility.While the implementation is clean and non-intrusive, consider adding ARIA attributes for better accessibility.
Apply this diff to improve accessibility:
- <div className="market-modal-selector container border mx-auto grid grid-cols-12 bg-dark border-gold/30 h-full row-span-12 rounded-2xl relative"> + <div className="market-modal-selector container border mx-auto grid grid-cols-12 bg-dark border-gold/30 h-full row-span-12 rounded-2xl relative" role="dialog" aria-label="Market Modal"> {/* ... existing content ... */} {isMarketLoading && ( - <div className="absolute bottom-4 inset-x-0 z-10 flex justify-center pointer-events-none"> + <div className="absolute bottom-4 inset-x-0 z-10 flex justify-center pointer-events-none" role="status" aria-live="polite"> <div className="bg-brown/90 text-sm px-4 py-1 rounded-t-lg border border-gold/30 flex items-center gap-2"> <div className="w-2 h-2 bg-gold/50 rounded-full animate-pulse" /> - <span>Syncing market data...</span> + <span aria-label="Loading">Syncing market data...</span> </div> </div> )}Also applies to: 350-357
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/hooks/store/useWorldLoading.tsx
(1 hunks)client/src/ui/components/trading/MarketModal.tsx
(4 hunks)client/src/ui/layouts/World.tsx
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/ui/layouts/World.tsx
🔇 Additional comments (2)
client/src/hooks/store/useWorldLoading.tsx (1)
6-8
: LGTM! Loading state management implementation follows best practices.
The implementation:
- Maintains consistency with existing patterns
- Properly types the new market loading state
- Follows Zustand's recommended approach for state updates
Also applies to: 13-15
client/src/ui/components/trading/MarketModal.tsx (1)
18-18
: LGTM! Clean integration of market loading state.
The hook usage follows React best practices by:
- Properly importing the store
- Using selector pattern for optimal performance
Also applies to: 68-69
{isMarketLoading && ( | ||
<div className="absolute bottom-4 inset-x-0 z-10 flex justify-center pointer-events-none"> | ||
<div className="bg-brown/90 text-sm px-4 py-1 rounded-t-lg border border-gold/30 flex items-center gap-2"> | ||
<div className="w-2 h-2 bg-gold/50 rounded-full animate-pulse" /> | ||
<span>Syncing market data...</span> | ||
</div> | ||
</div> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding error handling for market data sync failures.
The loading indicator provides good feedback during loading, but there's no handling for potential sync failures.
Consider implementing error handling:
const isMarketLoading = useWorldStore((state) => state.isMarketLoading);
+const [syncError, setSyncError] = useState<string | null>(null);
+
+useEffect(() => {
+ if (syncError) {
+ const timer = setTimeout(() => setSyncError(null), 5000);
+ return () => clearTimeout(timer);
+ }
+}, [syncError]);
// ... existing JSX ...
{isMarketLoading && (
<div className="absolute bottom-4 inset-x-0 z-10 flex justify-center pointer-events-none">
<div className="bg-brown/90 text-sm px-4 py-1 rounded-t-lg border border-gold/30 flex items-center gap-2">
<div className="w-2 h-2 bg-gold/50 rounded-full animate-pulse" />
<span>Syncing market data...</span>
</div>
</div>
)}
+ {syncError && (
+ <div className="absolute bottom-4 inset-x-0 z-10 flex justify-center pointer-events-none">
+ <div className="bg-red-900/90 text-sm px-4 py-1 rounded-t-lg border border-red-500/30 flex items-center gap-2">
+ <span>⚠️ {syncError}</span>
+ </div>
+ </div>
+ )}
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
addMarketSubscription
method for subscribing to market data.World
component to support market subscriptions alongside existing functionalities.MarketModal
to provide feedback during market data synchronization.Bug Fixes