new onboarding flow #2672
new onboarding flow #2672
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Hi @aymericdelab! 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. |
|
Warning Rate limit exceeded@aymericdelab has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces comprehensive changes across multiple files in the game client and core packages. The modifications primarily focus on enhancing navigation, account management, and system call authentication. Key changes include adding new hooks for navigation, simplifying spectator mode logic, updating tile and system call management, and improving error handling. The changes aim to streamline the codebase, enhance type safety, and provide more robust mechanisms for account handling and navigation. Changes
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 (
|
|
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: 15
🧹 Nitpick comments (15)
client/apps/game/src/hooks/helpers/use-structure-entity-id.ts (2)
19-23: Add input validation for hexPosition.The position calculation assumes hexPosition values are valid. Consider adding validation to handle potential undefined or invalid values.
const position = new PositionInterface({ + x: hexPosition?.col ?? 0, + y: hexPosition?.row ?? 0, - x: hexPosition.col, - y: hexPosition.row, }).getContract();
29-31: Consider adding state transition logging.For debugging purposes, it might be helpful to log structure ID changes, especially when falling back to UNDEFINED_STRUCTURE_ENTITY_ID.
const newStructureId = structure?.entity_id; + if (process.env.NODE_ENV === 'development') { + console.debug('Structure ID transition:', { + from: useUIStore.getState().structureEntityId, + to: newStructureId || UNDEFINED_STRUCTURE_ENTITY_ID + }); + } setStructureEntityId(newStructureId || UNDEFINED_STRUCTURE_ENTITY_ID);packages/core/src/dojo/create-system-calls.ts (2)
4-7: Add optional callbacks for more flexible authentication states
The newSystemCallAuthHandleris a good approach to centralizing account and error handling. Consider extending this type to handle additional authentication states (e.g., user-supplied signers or fallback login) and specify typed error codes to make the error-handling flow more predictable.
18-35: Consider distinct error classification during authentication
ThewithAuthwrapper brilliantly enforces that a signer be present. However, consider adding specialized error classes (e.g.,NoAccountError,AuthError) to differentiate missing signers from other runtime issues. This can help consumers fine-tune their error handling.packages/core/src/managers/tile-manager.ts (1)
426-426: Redundantreturn await
Althoughreturn awaitcan improve stack traces, it's generally unnecessary. Consider removingawaitif you don’t need the extra context.- return await this.systemCalls.create_hyperstructure({ + return this.systemCalls.create_hyperstructure({ signer, creator_entity_id: entityId, coords, });client/apps/game/src/ui/components/not-logged-in-message.tsx (1)
1-15: Simple and effective user feedback
The component clearly displays a message when the user isn’t connected. Consider adding accessible text or an aria-label for screen readers.client/apps/game/src/ui/components/home-button.tsx (1)
1-21: NewHomeButtoncomponent
Implementation is straightforward. Including a tooltip or accessible label could further clarify this button’s function for users.packages/react/src/hooks/helpers/use-query.ts (1)
Line range hint
8-15: Consider using a more descriptive event name for URL changes.The custom event name "urlChanged" is quite generic. Consider using a more specific name like "navigationStateChanged" or "routeChanged" to better indicate its purpose in the navigation system.
- window.dispatchEvent(new Event("urlChanged")); + window.dispatchEvent(new Event("navigationStateChanged"));client/apps/game/src/ui/components/modal-container.tsx (1)
37-39: Improve modal accessibility and keyboard navigation.While the styling changes look good, consider these accessibility improvements:
- Add ARIA attributes for better screen reader support
- Add focus trap for keyboard navigation
- Consider using a more semantic variant for the close button
- <div className={`z-50 bg-brown/90 text-gold ${containerClasses} fixed`} tabIndex={0}> + <div + role="dialog" + aria-modal="true" + className={`z-50 bg-brown/90 text-gold ${containerClasses} fixed`} + tabIndex={-1} + > <div className="flex justify-end p-3"> - <Button className="!p-4" size="xs" variant="danger" onClick={() => toggleModal(null)}> + <Button + className="!p-4" + size="xs" + variant="secondary" + onClick={() => toggleModal(null)} + aria-label="Close modal" + > <X className="w-4 h-4" /> </Button>client/apps/landing/src/hooks/context/dojo-context.tsx (1)
Line range hint
71-94: Enhance type safety for account management.The account handling in DojoContextProvider could benefit from stronger type safety.
Consider adding type guards:
const displayAddr = controllerAccount ? displayAddress(controllerAccount.address) : displayAddress(masterAddress); + +const validateAccount = (account: AccountInterface | null): account is AccountInterface => { + return account !== null && typeof account.address === 'string'; +}; return ( <DojoContext.Provider value={{ ...value, masterAccount, account: { - account: controllerAccount, + account: validateAccount(controllerAccount) ? controllerAccount : null, accountDisplay: displayAddr, }, }} >client/apps/game/src/main.tsx (1)
73-86: Enhance error handling and user feedback.Good addition of callbacks to handle no-account scenarios and system call errors. However, consider expanding the error handling to provide user feedback for system call errors instead of just logging them.
onError: (error) => { console.error("System call error:", error); - // Handle other types of errors if needed + state.setModal( + <ErrorModal + message="An error occurred during the system call. Please try again." + error={error} + />, + true + ); },client/apps/game/src/ui/modules/navigation/left-navigation-module.tsx (1)
75-77: Remove debug console.log statement.The console.log statement should be removed before merging to production.
const disableButtons = !structureIsMine && account.address !== "0x0"; - console.log({ account, structureIsMine, disableButtons });client/apps/game/src/ui/components/military/army-chip.tsx (1)
142-147: Consider simplifying the Position instantiation.The explicit number conversion can be simplified since the Position constructor should handle type coercion internally.
-position={ - new Position({ - x: Number(updatedArmy!.position.x), - y: Number(updatedArmy!.position.y), - }) -} +position={new Position(updatedArmy!.position)}client/apps/game/src/ui/layouts/world.tsx (1)
282-291: Consider using React's SyntheticEvent type for event handlers.The event handlers should explicitly type their event parameters for better type safety.
-onClick={(e) => { +onClick={(e: React.MouseEvent) => { e.stopPropagation(); }} -onDoubleClick={(e) => { +onDoubleClick={(e: React.MouseEvent) => { e.stopPropagation(); }} -onMouseMove={(e) => { +onMouseMove={(e: React.MouseEvent) => { e.stopPropagation(); }}client/apps/game/src/ui/modules/navigation/top-left-navigation.tsx (1)
242-249: Improve the account validation logic.The account check could be more robust by validating the address format.
-!account || account.address === "0x0" +!account?.address || account.address === "0x0" || !/^0x[a-fA-F0-9]{40}$/.test(account.address)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/public/assets/icons/home.svgis excluded by!**/*.svg
📒 Files selected for processing (45)
client/apps/game/.gitignore(1 hunks)client/apps/game/src/hooks/context/dojo-context.tsx(7 hunks)client/apps/game/src/hooks/helpers/use-navigate-to-realm-view-by-account.ts(1 hunks)client/apps/game/src/hooks/helpers/use-navigate.ts(1 hunks)client/apps/game/src/hooks/helpers/use-structure-entity-id.ts(1 hunks)client/apps/game/src/hooks/store/use-ui-store.ts(3 hunks)client/apps/game/src/main.tsx(2 hunks)client/apps/game/src/three/helpers/location-manager.ts(0 hunks)client/apps/game/src/three/scenes/hexception.tsx(2 hunks)client/apps/game/src/three/scenes/worldmap.tsx(4 hunks)client/apps/game/src/ui/components/bank/confirmation-popup.tsx(2 hunks)client/apps/game/src/ui/components/battles/battle-list-item.tsx(2 hunks)client/apps/game/src/ui/components/home-button.tsx(1 hunks)client/apps/game/src/ui/components/military/army-chip.tsx(3 hunks)client/apps/game/src/ui/components/military/army-list.tsx(1 hunks)client/apps/game/src/ui/components/military/army-management-card.tsx(5 hunks)client/apps/game/src/ui/components/modal-container.tsx(1 hunks)client/apps/game/src/ui/components/not-logged-in-message.tsx(1 hunks)client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx(4 hunks)client/apps/game/src/ui/components/trading/market-order-panel.tsx(7 hunks)client/apps/game/src/ui/layouts/no-account-modal.tsx(1 hunks)client/apps/game/src/ui/layouts/world.tsx(3 hunks)client/apps/game/src/ui/modules/controller/controller.tsx(1 hunks)client/apps/game/src/ui/modules/entity-details/building-entity-details.tsx(2 hunks)client/apps/game/src/ui/modules/entity-details/realm/buildings.tsx(1 hunks)client/apps/game/src/ui/modules/navigation/left-navigation-module.tsx(7 hunks)client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx(2 hunks)client/apps/game/src/ui/modules/navigation/top-left-navigation.tsx(7 hunks)client/apps/game/src/ui/modules/onboarding/steps.tsx(1 hunks)client/apps/game/src/ui/modules/social/player-id.tsx(1 hunks)client/apps/game/src/ui/modules/stream/event-stream.tsx(2 hunks)client/apps/game/src/ui/modules/world-structures/world-structures-menu.tsx(2 hunks)client/apps/game/src/utils/realms.ts(1 hunks)client/apps/landing/src/hooks/context/dojo-context.tsx(1 hunks)packages/core/src/__tests__/config.test.ts(0 hunks)packages/core/src/dojo/create-system-calls.ts(2 hunks)packages/core/src/dojo/setup.ts(1 hunks)packages/core/src/managers/config-manager.ts(0 hunks)packages/core/src/managers/tile-manager.ts(6 hunks)packages/core/src/managers/utils/army-movement-utils.ts(0 hunks)packages/core/src/provider/index.ts(0 hunks)packages/core/src/types/provider.ts(1 hunks)packages/react/src/hooks/helpers/use-hyperstructures.ts(0 hunks)packages/react/src/hooks/helpers/use-query.ts(1 hunks)packages/react/src/hooks/helpers/use-screen-orientation.ts(0 hunks)
💤 Files with no reviewable changes (7)
- packages/react/src/hooks/helpers/use-screen-orientation.ts
- packages/core/src/tests/config.test.ts
- packages/core/src/managers/config-manager.ts
- packages/core/src/managers/utils/army-movement-utils.ts
- packages/react/src/hooks/helpers/use-hyperstructures.ts
- client/apps/game/src/three/helpers/location-manager.ts
- packages/core/src/provider/index.ts
✅ Files skipped from review due to trivial changes (1)
- client/apps/game/src/ui/modules/controller/controller.tsx
🔇 Additional comments (54)
client/apps/game/src/hooks/helpers/use-structure-entity-id.ts (3)
4-4: LGTM! Import statements are now more focused.The removal of unused imports and streamlining to only necessary dependencies improves code clarity.
15-16: LGTM! Simplified state management.The removal of conditional checks (isMapView, spectator mode) makes the code more maintainable and easier to reason about.
11-13: Verify account-related functionality.The removal of account from useDojo suggests ownership checks are no longer needed. Please ensure this aligns with the new onboarding flow requirements.
✅ Verification successful
Account removal from use-structure-entity-id.ts is correct
The removal of account from useDojo in this hook is appropriate as the hook's purpose is entity ID handling, while account-related functionality is properly maintained in the action handlers where it's needed (structure placement, production management, battle actions).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining account-dependent checks in structure-related files rg -l "account.*structure" --type tsLength of output: 485
Script:
#!/bin/bash # Get context around account and structure usage rg "account.*structure" --type ts -C 3Length of output: 6804
packages/core/src/dojo/create-system-calls.ts (2)
9-10: Clearer naming conveys usage intent
UsingSystemCallsas a simple alias forReturnType<typeof createSystemCalls>is concise. This naming is coherent and reads well, so no changes are strictly necessary.
11-17: OptionalauthHandlerimplementation is well-structured
AllowingauthHandlerto be optional ensures backward compatibility. This strikes a good balance between providing authentication features and not forcing existing consumers to adopt them.packages/core/src/dojo/setup.ts (3)
3-3: Import statement improvements
ImportingSystemCallAuthHandlertogether withcreateSystemCallsis logical here and keeps related types together.
8-12: OptionalauthHandlerfosters a flexible setup
The updatedsetupfunction signature preserves backward compatibility while enabling advanced authentication scenarios. This is a clean way to allow incremental adoption.
15-18: Seamless integration ofauthHandler
Passing bothproviderandauthHandlerintocreateSystemCallsis coherent with the new design. No changes required on this snippet.packages/core/src/managers/tile-manager.ts (5)
19-20: Dependency update looks good
ImportingSystemCallsaligns well with the new system calls approach.
29-29: Constructor injection
AcceptingsystemCallsin the constructor is consistent with the refactor fromprovidertosystemCalls.
354-354: Optimistic building parameter
ForwardingstructureEntityIdto_optimisticBuildingproperly aligns with the signature.
362-364: Successful system call wrap
Callingthis.systemCalls.create_buildingunder a try/catch block ensures override cleanup on failure.
341-341: Verify usage of newstructureEntityIdparameter
Ensure all callers ofplaceBuildinghave been updated to passstructureEntityIdcorrectly.✅ Verification successful
✓
structureEntityIdparameter usage verified
The only caller ofplaceBuildinginhexception.tsxcorrectly passes thestructureEntityIdparameter fromthis.state.structureEntityId.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for placeBuilding usage to confirm correct usage of `structureEntityId` rg -A 4 "placeBuilding"Length of output: 797
Script:
#!/bin/bash # Using ast-grep to find all method calls to placeBuilding ast-grep --pattern '$_.placeBuilding($_)'Length of output: 43
Script:
#!/bin/bash # Search for method calls to placeBuilding with multiple arguments ast-grep --pattern '$_.placeBuilding($x, $y, $z, $w)' # Search for direct function calls to placeBuilding ast-grep --pattern 'placeBuilding($$$)' # Backup search with ripgrep using word boundaries rg '\bplaceBuilding\b' -A 5Length of output: 1055
client/apps/game/src/hooks/helpers/use-navigate.ts (1)
5-19: Add error handling for invalid positions.The hook should validate the Position object before navigation to prevent potential runtime errors.
Consider adding validation:
return (position: Position) => { + if (!position || typeof position.toHexLocationUrl !== 'function') { + console.error('Invalid position object provided to useNavigateToHexView'); + return; + } const url = position.toHexLocationUrl();client/apps/game/src/ui/components/bank/confirmation-popup.tsx (1)
26-27: Validate account state before allowing confirmations.The component retrieves account state but doesn't validate it before allowing confirmations.
Add validation:
const { account } = useAccount(); const setModal = useUIStore((state) => state.setModal); + +const isValidAccount = account && account.address; +if (!isValidAccount) { + disabled = true; + warning = "Please connect your wallet to continue"; +}client/apps/landing/src/hooks/context/dojo-context.tsx (1)
Line range hint
43-52: Breaking change: network property removal needs migration plan.The removal of the network property from useDojo hook's return type is a breaking change that could affect existing consumers.
Let's verify the impact:
client/apps/game/src/main.tsx (2)
21-21: LGTM! Import added for NoAccountModal.The import aligns with the new onboarding flow requirements.
89-89: Verify the impact of synchronous ETERNUM_CONFIG call.The change from async to sync call could affect initialization timing. Ensure this doesn't block the UI thread.
client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx (2)
5-5: LGTM! Import added for HomeButton.The import aligns with the navigation requirements.
114-114: Verify HomeButton placement in navigation.The HomeButton is added at the end of the navigation menu. Consider if this placement provides the best user experience for new users during onboarding.
✅ Verification successful
Home button placement is appropriate and follows good UX patterns
The HomeButton's placement at the end of the navigation menu is well-considered as it:
- Creates a clear visual boundary for the navigation menu
- Follows common patterns for home/exit actions
- Maintains consistent styling with other navigation elements
- Is easily recognizable with its home icon
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if HomeButton is consistently placed in other navigation components rg -l "HomeButton"Length of output: 137
Script:
#!/bin/bash # View the full content of both files echo "=== secondary-menu-items.tsx ===" cat client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx echo -e "\n=== home-button.tsx ===" cat client/apps/game/src/ui/components/home-button.tsxLength of output: 4980
client/apps/game/src/ui/components/military/army-list.tsx (1)
23-26: Verify the impact of TileManager initialization change.The switch from
network.providertosetup.systemCallsaligns with the system calls refactor. Ensure this change is consistent across all TileManager instances.✅ Verification successful
TileManager initialization is consistent across the codebase
All instances of TileManager have been properly updated to use the new system calls pattern, either through
dojo.setup.systemCallsorthis.dojo.systemCallsdepending on the context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other TileManager instantiations ast-grep --pattern 'new TileManager($_, $_, $_)'Length of output: 2193
client/apps/game/src/ui/components/battles/battle-list-item.tsx (2)
6-6: LGTM! Import added for Position type.Good addition of proper typing for position data.
102-104: Verify Position type usage consistency.The change from direct position object to Position class improves type safety. However, ensure this pattern is followed consistently across the codebase.
✅ Verification successful
Position class usage is consistent across the codebase ✅
The change from direct position object to Position class in
battle-list-item.tsxfollows the established pattern. All instances ofViewOnMapIconand related components consistently use the Position class for position handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other ViewOnMapIcon usages ast-grep --pattern 'ViewOnMapIcon $$$' # Search for direct position object usage rg "position={.*}"Length of output: 167
Script:
#!/bin/bash # Search for ViewOnMapIcon usages and imports ast-grep --pattern 'import { ViewOnMapIcon } from $$$' echo "=== ViewOnMapIcon component usages ===" ast-grep --pattern '<ViewOnMapIcon $$$' # Search for Position class imports and usage echo -e "\n=== Position imports ===" ast-grep --pattern 'import { Position } from $$$' echo -e "\n=== Position instantiations ===" ast-grep --pattern 'new Position' # Search for position prop patterns echo -e "\n=== position prop patterns ===" rg "position=\{" --type ts --type tsxLength of output: 4997
client/apps/game/src/hooks/store/use-ui-store.ts (3)
67-67: LGTM! Good addition of the setModal method.The new method provides a more controlled way to manage modal state by handling both content and visibility in a single call.
81-83: LGTM! Clear block-style implementation.The block-style implementation improves readability and maintainability.
136-136: LGTM! Consistent implementation of setModal.The implementation correctly updates both modalContent and showModal states.
client/apps/game/src/ui/modules/social/player-id.tsx (1)
176-177: Verify position validation requirements.The direct usage of
structure.positionwithout normalization might need validation to ensure correct coordinates are passed to the navigation components.Run this script to check for position validation patterns in the codebase:
client/apps/game/src/ui/modules/stream/event-stream.tsx (1)
171-172: LGTM! Proper Position object instantiation.The explicit instantiation of Position objects ensures type safety and proper position handling.
client/apps/game/src/ui/modules/entity-details/building-entity-details.tsx (2)
95-104: LGTM! Clean refactor of TileManager initialization.The changes improve type safety by using
structureEntityIdand follow React best practices with proper dependency injection throughsystemCalls.
113-122: LGTM! Consistent implementation of TileManager changes.The changes maintain consistency with the previous refactor, improving the overall code quality.
client/apps/game/src/ui/modules/onboarding/steps.tsx (2)
6-6: LGTM! Clean import organization.The new imports support the simplified navigation approach in the onboarding flow.
Also applies to: 13-15
21-23: LGTM! Clean destructuring of Dojo setup.The destructuring improves code readability.
client/apps/game/src/ui/components/military/army-management-card.tsx (1)
158-158: LGTM! Improved type safety and navigation handling.The changes enhance type safety with
PositionInterfaceand simplify navigation using the new hook.Also applies to: 280-280, 285-285, 295-295
client/apps/game/src/ui/modules/navigation/left-navigation-module.tsx (1)
124-124: LGTM! Consistent button state management.The changes improve UX by consistently handling button states based on account status and structure ownership.
Also applies to: 139-139, 154-154, 169-169, 185-185, 224-224
client/apps/game/src/ui/components/military/army-chip.tsx (1)
149-149: LGTM! Clean and consistent Position usage.The direct instantiation of Position from the army position is clean and matches the pattern used elsewhere.
client/apps/game/src/ui/modules/world-structures/world-structures-menu.tsx (1)
Line range hint
320-320: LGTM! Consistent Position instantiation pattern.The Position objects are created consistently for both navigation components, following the same pattern used throughout the codebase.
Also applies to: 324-324
client/apps/game/src/ui/layouts/world.tsx (2)
279-281: LGTM! Good addition of authentication check.The NotLoggedInMessage component is appropriately placed at the top level, ensuring users are informed of their authentication status.
329-338: LGTM! Well-structured conditional rendering.The desktop-only components are properly wrapped in a conditional block with clear separation of concerns.
client/apps/game/src/ui/modules/navigation/top-left-navigation.tsx (2)
154-176: LGTM! Well-structured navigation logic.The navigation functions are well-organized with:
- Clear separation of concerns using hooks
- Proper null checks and early returns
- Consistent Position object usage
320-320: LGTM! Consistent Position instantiation pattern.The Position objects are created consistently for both navigation components.
Also applies to: 324-324
client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx (1)
1-1: LGTM! Navigation logic has been simplified.The changes streamline the navigation flow by:
- Using the new
useNavigateToHexViewhook- Removing the intermediate
goToHexViewfunction- Directly calling
navigateToHexView(position)in the button'sonClickhandlerAlso applies to: 45-45, 71-71
packages/core/src/types/provider.ts (1)
5-7: LGTM! Interface visibility has been updated.The
SystemSignerinterface is now exported, making it accessible to other modules that need to implement system call authentication.client/apps/game/src/ui/components/trading/market-order-panel.tsx (4)
384-397: LGTM! Error handling has been added for order cancellation.The
onCancelfunction properly handles errors and manages loading states.
429-429: LGTM! Confirmation flow has been added for order actions.The button click now triggers a confirmation modal instead of directly executing the action, preventing accidental cancellations.
Also applies to: 437-442
454-495: LGTM! Confirmation popup content has been improved.The confirmation popup now:
- Shows different titles based on the action type
- Displays relevant order details
- Includes input validation and donkey requirements
509-509: LGTM! Order creation flow has been enhanced.The changes improve the order creation flow by:
- Adding a confirmation step
- Adding error handling
- Managing loading states
- Displaying order details in the confirmation popup
Also applies to: 548-563, 701-729
client/apps/game/src/three/scenes/hexception.tsx (2)
140-140: LGTM! TileManager initialization has been updated.The
TileManagernow usessystemCallsinstead ofprovider, aligning with the broader refactor of system call authentication.
308-309: LGTM! Account validation has been added.The changes add a safety check to ensure an account exists before allowing building placement actions.
Also applies to: 316-317
client/apps/game/src/three/scenes/worldmap.tsx (3)
88-88: LGTM! Consistent with the broader refactor.The change to use
this.dojo.systemCallsaligns with similar modifications across the codebase, standardizing how theTileManagerinteracts with system calls.
301-302: LGTM! Improved readability with account state handling.Storing the account state in a variable before use improves code readability and maintains consistency with similar changes across the codebase.
336-338: LGTM! Consistent account state handling.The account state handling follows the same pattern as other methods, improving code readability and maintainability.
Also applies to: 354-354
client/apps/game/.gitignore (1)
29-29: LGTM! Appropriate addition to .gitignore.Adding
*.ts.timestamp-*.mjsto ignore generated TypeScript timestamp files is a good practice.client/apps/game/src/ui/modules/entity-details/realm/buildings.tsx (2)
53-56: LGTM! Consistent TileManager initialization.The change to use
dojo.setup.systemCallsaligns with similar modifications across the codebase, standardizing how theTileManagerinteracts with system calls.
59-61: LGTM! Improved accuracy with entity ID usage.The change to use
structureEntityIddirectly improves the accuracy of building operations and aligns with similar changes across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
client/apps/game/src/ui/layouts/no-account-modal.tsx (1)
15-39: 🛠️ Refactor suggestionEnhance modal accessibility and keyboard navigation.
Previous accessibility concerns remain valid. The modal needs ARIA attributes and keyboard interaction improvements.
return ( <ModalContainer> - <div className="flex items-start justify-center pt-32"> + <div + className="flex items-start justify-center pt-32" + role="dialog" + aria-modal="true" + aria-labelledby="modal-title" + aria-describedby="modal-description" + > <div className="flex container max-w-lg mx-auto bg-brown/90 bg-hex-bg rounded-xl border border-gold/40"> <div className="w-full p-8 prose prose-pink"> - <h3 className="text-center mb-6">Account Required</h3> + <h3 id="modal-title" className="text-center mb-6">Account Required</h3> - <p className="text-center mb-8"> + <p id="modal-description" className="text-center mb-8"> You need to have a Cartridge Controller account to play this game. Please login or create an account to continue. </p> <div className="flex justify-center"> <Button onClick={handleHomeClick} + onKeyDown={(e) => { + if (e.key === 'Escape') handleHomeClick(); + }} className="!bg-[#FCB843] !text-black border-none hover:!bg-[#FCB843]/80" variant="default" + aria-label="Log in with Cartridge Controller" + tabIndex={0} >client/apps/game/src/utils/realms.ts (1)
27-37: 🛠️ Refactor suggestionAdd validation and improve error handling for player realm retrieval.
The function needs proper validation and error handling to be more robust.
export const getPlayerFirstRealm = (components: ClientComponents, address: ContractAddress) => { + if (!components?.Structure || !components?.Realm || !components?.Owner) { + throw new Error("Invalid components: Structure, Realm, and Owner components are required"); + } + + if (!address) { + throw new Error("Invalid address"); + } + const realms = runQuery([ Has(components.Structure), Has(components.Realm), HasValue(components.Owner, { address: address }), ]); - const realm = realms.values().next().value; + if (realms.size === 0) { + throw new Error(`No realms found for address: ${address}`); + } + const realm = realms.values().next().value; return realm; };client/apps/game/src/hooks/context/dojo-context.tsx (1)
135-137: 🛠️ Refactor suggestionEnhance error handling for account state transitions.
The account initialization and transition logic needs more robust error handling.
+ const [connectionError, setConnectionError] = useState<string | null>(null); + + const handleAccountTransition = (account: AccountInterface | null) => { + try { + if (!account && isConnected) { + setConnectionError("Connected but account not available"); + } + // ... existing logic + } catch (error) { + setConnectionError(`Account transition failed: ${error}`); + } + };Also applies to: 145-151
🧹 Nitpick comments (3)
client/apps/game/src/ui/layouts/no-account-modal.tsx (1)
10-13: Consider adding error handling for state updates.While the function is well-structured, it would benefit from try-catch error handling to gracefully handle potential state update failures.
const handleHomeClick = () => { + try { setModal(null, false); setShowBlankOverlay(true); + } catch (error) { + console.error('Failed to update modal state:', error); + // Consider showing a user-friendly error message + } };packages/core/src/dojo/create-system-calls.ts (2)
18-35: Consider enhancing error handling inwithAuth.While the authentication wrapper is well-implemented, the error handling could be more specific:
- The generic error type in the catch block could mask specific error types
- The "No account connected" error message could be more descriptive
Consider this enhancement:
const withAuth = <T extends (...args: any[]) => Promise<any>>(fn: T): T => { return (async (...args: Parameters<T>) => { try { const props = args[0] as SystemProps.SystemSigner; // Check for signer specifically if (!props?.signer || props?.signer.address === "0x0") { authHandler?.onNoAccount?.(); - throw new Error("No account connected"); + throw new Error("Authentication required: No account connected or invalid signer address"); } return await fn(...args); } catch (error) { - authHandler?.onError?.(error as Error); + // Preserve the original error type if it's already an Error + const normalizedError = error instanceof Error ? error : new Error(String(error)); + authHandler?.onError?.(normalizedError); - throw error; + throw normalizedError; } }) as T; };
303-373: Consider grouping related system calls.The system calls object could benefit from better organization. Consider grouping related calls using TypeScript namespaces or nested objects for better maintainability.
Example refactor:
const systemCalls = { // Resource management resources: { send: withAuth(send_resources), sendMultiple: withAuth(send_resources_multiple), pickup: withAuth(pickup_resources), transfer: withAuth(transfer_resources), buy: withAuth(buy_resources), sell: withAuth(sell_resources), mint: withAuth(mint_resources), }, // Battle system battle: { start: withAuth(battle_start), forceStart: withAuth(battle_force_start), resolve: withAuth(battle_resolve), // ... other battle methods }, // ... other groupings // Utility functions utils: { isLive, uuid, }, } as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
client/apps/game/src/hooks/context/dojo-context.tsx(7 hunks)client/apps/game/src/hooks/helpers/use-navigate-to-realm-view-by-account.ts(1 hunks)client/apps/game/src/hooks/helpers/use-navigate.ts(1 hunks)client/apps/game/src/ui/layouts/no-account-modal.tsx(1 hunks)client/apps/game/src/utils/realms.ts(1 hunks)packages/core/src/dojo/create-system-calls.ts(2 hunks)packages/core/src/managers/tile-manager.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/apps/game/src/hooks/helpers/use-navigate-to-realm-view-by-account.ts
- client/apps/game/src/hooks/helpers/use-navigate.ts
🔇 Additional comments (10)
client/apps/game/src/ui/layouts/no-account-modal.tsx (2)
1-4: LGTM! Imports are well-organized.The imports follow a logical pattern and all imported components/hooks are utilized in the code.
6-9: LGTM! Component structure follows React best practices.Good separation of concerns with individual hook calls and clear component naming.
packages/core/src/managers/tile-manager.ts (3)
29-29: LGTM! Good dependency injection improvement.Replacing the provider with systemCalls in the constructor improves the separation of concerns and makes the dependencies more explicit.
382-400: LGTM! Proper error handling implementation.The error handling in
destroyBuildingis well implemented:
- Uses optimistic updates
- Properly reverts the optimistic changes on error
- Re-throws the error for upstream handling
402-418: LGTM! Consistent error handling pattern.The error handling in
pauseProductionandresumeProductionfollows the same robust pattern:
- Optimistic updates
- Try-catch block
- Cleanup on error
- Error propagation
Also applies to: 421-437
client/apps/game/src/utils/realms.ts (1)
4-25: LGTM! Well-structured implementation with proper validation and optimization.The function includes:
- Input validation
- Error handling
- Performance optimization for large sets
client/apps/game/src/hooks/context/dojo-context.tsx (2)
23-26: LGTM! Good use of const assertion for type safety.The NULL_ACCOUNT constant provides a centralized, type-safe way to represent an uninitialized account.
Line range hint
205-229: LGTM! Clear and comprehensive connection state handling.The UI rendering logic properly handles all connection states and provides appropriate feedback to users.
packages/core/src/dojo/create-system-calls.ts (2)
4-7: LGTM! Well-structured type definition for auth handling.The
SystemCallAuthHandlertype provides a clean interface for handling authentication scenarios with optional callbacks for no account and error cases.
316-316: LGTM! Correctly exposed non-authenticated endpoints.The
isLiveanduuidfunctions are appropriately exposed without authentication wrappers as they are read-only operations.Also applies to: 331-331
| const onSpectatorModeClick = () => { | ||
| setSpectatorMode(true); | ||
| handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); | ||
| window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); | ||
| showBlankOverlay(false); | ||
| const randomRealmEntity = getRandomRealmEntity(value.components); | ||
| const position = randomRealmEntity && getComponentValue(value.components.Position, randomRealmEntity); | ||
| position && navigateToHexView(new Position(position)); |
There was a problem hiding this comment.
Implement the navigateToHexView function.
The spectator mode click handler relies on an unimplemented navigation function, which will throw an error when used.
Consider implementing the navigation function or using an existing navigation utility from your codebase. The function should handle the Position type and navigate to the appropriate hex view.
Also applies to: 252-254
| send_resources: withAuth(send_resources), | ||
| send_resources_multiple: withAuth(send_resources_multiple), | ||
| pickup_resources: withAuth(pickup_resources), | ||
| remove_liquidity: withAuth(remove_liquidity), | ||
| add_liquidity: withAuth(add_liquidity), | ||
| sell_resources: withAuth(sell_resources), | ||
| buy_resources: withAuth(buy_resources), | ||
| change_bank_owner_fee: withAuth(change_bank_owner_fee), | ||
| open_account: withAuth(open_account), | ||
| create_bank: withAuth(create_bank), | ||
| explore: withAuth(explore), | ||
| set_address_name: withAuth(set_address_name), | ||
| set_entity_name: withAuth(set_entity_name), | ||
| isLive: isLive, | ||
| create_order: withAuth(create_order), | ||
| accept_order: withAuth(accept_order), | ||
| cancel_order: withAuth(cancel_order), | ||
| accept_partial_order: withAuth(accept_partial_order), | ||
| upgrade_realm: withAuth(upgrade_realm), | ||
| create_multiple_realms: withAuth(create_multiple_realms), | ||
| transfer_resources: withAuth(transfer_resources), | ||
| travel_hex: withAuth(travel_hex), | ||
| destroy_building: withAuth(destroy_building), | ||
| pause_production: withAuth(pause_production), | ||
| resume_production: withAuth(resume_production), | ||
| create_building: withAuth(create_building), | ||
| create_army: withAuth(create_army), | ||
| delete_army: withAuth(delete_army), | ||
| uuid: uuid, | ||
|
|
||
| create_hyperstructure: withAuth(create_hyperstructure), | ||
| contribute_to_construction: withAuth(contribute_to_construction), | ||
| set_access: withAuth(set_access), | ||
| set_co_owners: withAuth(set_co_owners), | ||
| get_points: withAuth(get_points), | ||
| end_game: withAuth(end_game), | ||
| register_to_leaderboard: withAuth(register_to_leaderboard), | ||
| claim_leaderboard_rewards: withAuth(claim_leaderboard_rewards), | ||
|
|
||
| claim_quest: withAuth(claim_quest), | ||
| mint_resources: withAuth(mint_resources), | ||
|
|
||
| army_buy_troops: withAuth(army_buy_troops), | ||
| army_merge_troops: withAuth(army_merge_troops), | ||
|
|
||
| create_guild: withAuth(create_guild), | ||
| join_guild: withAuth(join_guild), | ||
| whitelist_player: withAuth(whitelist_player), | ||
| transfer_guild_ownership: withAuth(transfer_guild_ownership), | ||
| remove_guild_member: withAuth(remove_guild_member), | ||
| disband_guild: withAuth(disband_guild), | ||
| remove_player_from_whitelist: withAuth(remove_player_from_whitelist), | ||
|
|
||
| battle_start: withAuth(battle_start), | ||
| battle_force_start: withAuth(battle_force_start), | ||
| battle_resolve: withAuth(battle_resolve), | ||
| battle_leave: withAuth(battle_leave), | ||
| battle_join: withAuth(battle_join), | ||
| battle_claim: withAuth(battle_claim), | ||
| battle_pillage: withAuth(battle_pillage), | ||
| battle_leave_and_claim: withAuth(battle_leave_and_claim), | ||
| battle_leave_and_pillage: withAuth(battle_leave_and_pillage), | ||
|
|
||
| mint_test_realm: withAuth(mint_test_realm), | ||
| mint_season_passes: withAuth(mint_season_passes), | ||
| attach_lords: withAuth(attach_lords), | ||
| detach_lords: withAuth(detach_lords), | ||
| mint_test_lords: withAuth(mint_test_lords), | ||
| bridge_resources_into_realm: withAuth(bridge_resources_into_realm), | ||
| bridge_start_withdraw_from_realm: withAuth(bridge_start_withdraw_from_realm), | ||
| bridge_finish_withdraw_from_realm: withAuth(bridge_finish_withdraw_from_realm), |
There was a problem hiding this comment.
💡 Codebase verification
Fix naming inconsistency in battle methods
The system calls object exposes battle_leave_and_claim but the provider implements battle_claim_and_leave. These method names should be aligned to maintain consistency and prevent confusion.
battle_leave_and_claimin system calls should be renamed tobattle_claim_and_leaveto match the provider implementation
🔗 Analysis chain
Verify the completeness of system calls.
The system calls object appears comprehensive, but let's verify that all provider methods are properly exposed and wrapped.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all provider methods are properly exposed in systemCalls
# Find all methods in the provider interface
echo "Provider methods:"
ast-grep --pattern 'interface EternumProvider {
$$$
}' packages/core/src/provider/index.ts
# Compare with system calls
echo -e "\nSystem calls:"
rg "provider\." packages/core/src/dojo/create-system-calls.ts
Length of output: 3115
Script:
#!/bin/bash
# Get the provider interface definition and battle-related methods
echo "Provider interface definition:"
cat packages/core/src/provider/index.ts
echo -e "\nBattle-related method declarations:"
rg "battle_.*?:" packages/core/src/provider/index.ts
Length of output: 65753
| function navigateToHexView(arg0: Position) { | ||
| throw new Error("Function not implemented."); | ||
| } |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Refactoring