Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request updates client and contract code across the board. On the client side, dependency versions in the package manifest are bumped and naming is refactored from “mancalaAlpha” to “mancalaSalt” while introducing new UI components and routes (notably a new Boards page). On the contracts side, version and dependency upgrades are applied along with comprehensive configuration and namespace changes. In addition, many source files have been updated to change module, struct, enum, and function visibility from private to public, and auto-generated binding/schema files are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Router
participant A as App Component
participant B as Boards Page
participant BB as BoardBlock Component
U->>R: Navigate to /boards
R->>A: Render App with "/boards" route
A->>B: Display Boards component
B->>BB: Render each BoardBlock for board items
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
contracts/src/tests/utils.cairo (1)
7-39: 🛠️ Refactor suggestionAdd input validation and error handling.
The function handles complex state mutations but lacks input validation and error handling.
Consider adding:
- Validation for
player.len_pitsto ensure it's within valid range- Checks for arithmetic overflow in seed count calculations
- Error handling for invalid pit or seed access
pub fn move_player_seeds_to_store(world: WorldStorage, player: @Player) { + // Validate input + assert(*player.len_pits > 0 && *player.len_pits <= 6, 'Invalid number of pits'); + let mut store: Store = StoreTrait::new(world); let mut player_store = store.get_pit(*player.game_id, *player.address, 7); let store_start_count = player_store.seed_count; let mut total_moved_seeds = 0; let mut idx = 1; loop { if idx > *player.len_pits { break; } let mut pit = store.get_pit(*player.game_id, *player.address, idx); let pit_seed_count = pit.seed_count; + // Check for arithmetic overflow + assert( + store_start_count + total_moved_seeds + pit_seed_count <= u32::max(), + 'Seed count overflow' + ); let mut seeds_idx = 1; loop { if seeds_idx > pit_seed_count { break; } let mut seed = store.get_seed(*player.game_id, *player.address, idx, seeds_idx); seed.pit_number = 7; seed.seed_number = store_start_count + total_moved_seeds + seeds_idx; store.set_seed(seed); seeds_idx += 1; }; total_moved_seeds += pit_seed_count; pit.seed_count = 0; store.set_pit(pit); idx += 1; }; // Update store pit count once at the end player_store.seed_count += total_moved_seeds; store.set_pit(player_store); }
🧹 Nitpick comments (28)
contracts/src/events/move.cairo (1)
8-39: LGTM! Good visibility changes for trait implementations.Making the trait implementations public allows other modules to properly construct events. The constructor functions are well-structured.
Consider adding trailing commas to all constructor parameters for consistency with EndTurn::new():
- fn new(game_id: u128, pit_number: u8, seed_number: u8) -> PlayerMove { + fn new(game_id: u128, pit_number: u8, seed_number: u8,) -> PlayerMove { - fn new(game_id: u128, player: ContractAddress) -> PlayerExtraTurn { + fn new(game_id: u128, player: ContractAddress,) -> PlayerExtraTurn { - fn new(game_id: u128, player: ContractAddress, pit_number: u8, seed_count: u8) -> Capture { + fn new(game_id: u128, player: ContractAddress, pit_number: u8, seed_count: u8,) -> Capture {contracts/src/elements/tasks/mastering.cairo (1)
11-11: Consider improving the description text.The current text "Get {} games win streak" could be more polished. Consider using "Achieve a {} game winning streak" for better readability.
- format!("Get {} games win streak", count) + format!("Achieve a {} game winning streak", count)contracts/src/elements/tasks/collecting.cairo (1)
11-11: Fix grammatical error in description text.The current text has a grammatical error: "seed" should be "seeds" when referring to multiple items.
- format!("Collect {} seed in game lifetime history", count) + format!("Collect {} seeds in game lifetime history", count)client/src/components/boards/board-block.tsx (1)
15-25: Extract hardcoded colors to theme constants.The component uses hardcoded color values which could be moved to theme constants for better maintainability.
Consider creating a theme file with color constants:
// theme.ts export const colors = { boardBackground: '#0F111680', textPrimary: '#FFFFFF', textSecondary: '#C7CAD4', buttonDefault: '#171922', buttonAccent: '#F58229' } as const;Then update the component to use these constants:
-<div className="w-[630px] h-[400px] bg-[#0F111680] rounded-xl"> +<div className="w-[630px] h-[400px] rounded-xl" style={{ backgroundColor: colors.boardBackground }}> <div className="w-full h-full p-4 space-y-1"> {/* ... */} - <h3 className="text-white text-2xl font-semibold">{name}</h3> - <p className="text-[#C7CAD4] text-lg">{description}</p> + <h3 className="text-2xl font-semibold" style={{ color: colors.textPrimary }}>{name}</h3> + <p className="text-lg" style={{ color: colors.textSecondary }}>{description}</p> { - owned ? <Button className="p-5 rounded-3xl bg-[#171922]">Owned</Button> : - <Button className="p-5 rounded-3xl bg-[#F58229]">Buy Board</Button> + owned ? <Button className="p-5 rounded-3xl" style={{ backgroundColor: colors.buttonDefault }}>Owned</Button> : + <Button className="p-5 rounded-3xl" style={{ backgroundColor: colors.buttonAccent }}>Buy Board</Button> } </div> </div>contracts/src/lib.cairo (1)
1-63:Details
❓ Verification inconclusive
Verify the architectural decision to make all modules public.
Making all modules public increases accessibility but could impact encapsulation. Please confirm that this aligns with the project's architectural goals and that all modules are intended to be part of the public API.
Consider:
- Are all these modules meant to be part of the public API?
- Would some modules benefit from remaining internal to maintain better encapsulation?
- Is there documentation that needs to be updated to reflect these public APIs?
Run this script to check for potential unintended usage of these newly public modules:
🏁 Script executed:
#!/bin/bash # Description: Check for usage of these modules outside their intended scope # Search for imports of these modules rg -l "use mancala::(constants|store|models|events|components|elements|systems|types|utils|tests)" --type cairoLength of output: 143
Below is an updated verification script that avoids using the unrecognized file type by instead searching through all files matching the "*.cairo" pattern. Please execute this script and manually verify the usage of the public modules to ensure they are indeed part of the intended public API.
Next Steps:
- Run the above script and confirm that there are no unintended external usages of these modules.
- Verify that the architectural decision to make all modules public aligns with the project's design and documentation.
🏁 Script executed:
[run_scripts] #!/bin/bash # Updated: Check for imports of public modules across all Cairo files. rg -l "use mancala::(constants|store|models|events|components|elements|systems|types|utils|tests)" -g "*.cairo"Length of output: 1410
Architectural Review: Public API Modules
The search results confirm that many modules (e.g., components, elements, events, tests, etc.) are actively imported within various Cairo files across the codebase. This internal usage indicates that, at least within the repository, these modules are intended to be public. However, before finalizing this decision, please verify the following:
- Confirm that exposing all these modules publicly aligns with the intended external API boundaries.
- Review if any modules would benefit from internal-only visibility to improve encapsulation.
- Update the project documentation to clearly differentiate the intended public APIs from internal implementation details if necessary.
Based on the current evidence, the decision to make all modules public appears consistent with internal usage, but these points should be manually verified.
contracts/src/components/initializable.cairo (1)
29-43: Add events and enhance error handling.The initialize function could benefit from emitting events and more descriptive error messages.
Consider adding:
- Event emission for successful initialization
- More descriptive error messages
- Additional validation checks
fn initialize(self: @ComponentState<TState>, world: WorldStorage) { // [Effect] Initialize component let mut store = StoreTrait::new(world); let current_game_counter = store.get_game_counter(1); - assert(current_game_counter.count == 0, 'Counter already initialized'); + assert( + current_game_counter.count == 0, + 'Game counter has already been initialized' + ); // [Effect] Create GameCounter let mut game_counter = GameCounterTrait::new(); // [Effect] GameCounter increment game_counter.increment(); store.set_game_counter(game_counter); + + // Emit initialization event + self.emit(Event::Initialized(game_counter.count)); }contracts/src/systems/profile.cairo (1)
12-14: Clean up commented imports.These commented imports should either be removed if no longer needed or uncommented if they will be used in the future.
- //use starknet::ContractAddress; - //use mancala::models::profile::Profile;client/src/components/message-area.tsx (1)
35-35: Remove or enhance console.log statement.Consider removing this console.log or enhancing it with more meaningful debug information for development purposes.
- console.log("clicked: "); + // Remove for production or enhance with meaningful debug infocontracts/src/components/profile.cairo (1)
3-3: Clean up commented debug import.Remove the commented debug import if it's no longer needed.
- //use core::debug::PrintTrait;client/src/App.tsx (1)
56-56: Consider removing unnecessary type assertions.The double type assertion (
as never as Connector) seems redundant and could be simplified.- const connectors = [new ControllerConnector(options as never) as never as Connector]; + const connectors = [new ControllerConnector(options) as Connector];contracts/src/tests/setup.cairo (1)
1-2: Consider keeping debug trait for development.While removing debug prints in production code is good practice, consider keeping them in test files to aid in debugging test failures.
- //use core::debug::PrintTrait; + #[cfg(test)] + use core::debug::PrintTrait;client/src/pages/Boards.tsx (2)
6-21: Consider moving hardcoded board data to a separate configuration file.The boards array contains hardcoded data that would be better maintained in a separate configuration file. This would improve maintainability and make it easier to update board information.
+// boards.config.ts +export const MANCALA_BOARDS = [ + { + id: 1, + image_path: './assets/default_board_sample.png', + name: 'Default Board', + description: 'Story about it that you can make mention of to have like a description', + owned: true + }, + { + id: 2, + image_path: './assets/starknet_board_sample.png', + name: 'Starknet Board', + description: 'Story about it that you can make mention of to have like a description', + owned: false + } +];
58-61: Consider adding pagination for board grid.The grid uses a fixed height with overflow scroll. As more boards are added, consider implementing proper pagination for better user experience.
contracts/src/store.cairo (2)
23-25: Verify the impact of making Store struct public.Making the Store struct public increases its visibility. Ensure this aligns with the security model and doesn't expose sensitive functionality.
30-30: Consider documenting the public implementation.Since StoreImpl is now public, consider adding documentation to explain its intended usage and any security considerations.
client/src/components/tutorial/tutorial-game-navigation.tsx (1)
21-33: Consider adding proper TypeScript types for props.The component uses
anytype for multiple props. Consider defining proper interfaces/types for better type safety.+interface TutorialGameNavigationProps { + game_players: GamePlayers; + player_names: PlayerNames; + game_node: GameNode; + account: Account; + gameId: string; + timeRemaining: number; + setTimeRemaining: (time: number) => void; + message: string; + setMessage: (message: string) => void; + action: { action: string; message: string }; + setAction: (action: { action: string; message: string }) => void; + moveMessage: string; +}client/src/pages/games/Gameplay.tsx (2)
37-42: Simplify the boolean expression.The ternary operator returning boolean literals can be simplified.
-const involved = - game_players?.mancalaSaltPlayerModels.edges.filter( - (item: any) => item?.node.address === account.address, - ).length > 0 - ? true - : false; +const involved = game_players?.mancalaSaltPlayerModels.edges.filter( + (item: any) => item?.node.address === account.address, +).length > 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 38-42: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
52-53: Consider adjusting polling interval.The polling interval of 100ms might be too aggressive. Consider increasing it to reduce server load unless this frequency is specifically required.
client/src/lib/utils.ts (1)
247-349: Consider refactoring for better maintainability.The
calculateMancalaMovefunction is complex with multiple responsibilities. Consider breaking it down into smaller, focused functions:
- Seed movement logic
- Capture logic
- Pit transition logic
Example refactor:
+function moveSeed(currentPit: number, currentPlayer: string): { newPit: number, newPlayer: string } { + if (currentPlayer === player && currentPit > 7) { + return { newPit: 1, newPlayer: opponent }; + } else if (currentPlayer === opponent && currentPit > 6) { + return { newPit: 1, newPlayer: player }; + } + return { newPit: currentPit, newPlayer: currentPlayer }; +} +function handleCapture( + seeds: any[], + lastSeedPosition: { pit: number, player: string }, + player: string, + opponent: string +): any[] { + if (lastSeedPosition.player === player && lastSeedPosition.pit !== 7) { + // Capture logic here + } + return seeds; +} export function calculateMancalaMove(seeds: any[], selectedPit: number | 0, player: string, opponent: string) { // Get seeds to move const seedsToMove = seeds.filter(/*...*/).sort(/*...*/); if (seedsToMove.length === 0) return seeds; // Keep unchanged seeds const unchangedSeeds = seeds.filter(/*...*/); // Move seeds let currentPit = selectedPit; let currentPlayer = player; let lastSeedPosition = { pit: 0, player }; const movedSeeds = seedsToMove.map((seed, index) => { currentPit++; - // Handle pit transitions - if (currentPlayer === player && currentPit > 7) { - currentPit = 1; - currentPlayer = opponent; - } else if (currentPlayer === opponent && currentPit > 6) { - currentPit = 1; - currentPlayer = player; - } + // Handle pit transitions + const { newPit, newPlayer } = moveSeed(currentPit, currentPlayer); + currentPit = newPit; + currentPlayer = newPlayer; // Update last seed position if (index === seedsToMove.length - 1) { lastSeedPosition = { pit: currentPit, player: currentPlayer }; } // Return moved seed return { /*...*/ }; }); - // Handle capture - if (lastSeedPosition.player === player && lastSeedPosition.pit !== 7) { - // Capture logic - } + // Handle capture + return handleCapture([...unchangedSeeds, ...movedSeeds], lastSeedPosition, player, opponent); - return [...unchangedSeeds, ...movedSeeds]; }contracts/src/utils/board.cairo (1)
9-9: Verify the impact of making these functions public.The change in visibility from private to public for these functions expands their accessibility. Ensure this aligns with the module's design and doesn't expose sensitive operations.
Consider:
- Document the intended usage of these public functions
- Add input validation if these functions can now be called from untrusted contexts
- Consider using an interface trait to expose only necessary functions
Also applies to: 28-28, 45-45, 135-135, 178-178, 199-199, 214-214, 246-246, 280-280, 335-335
contracts/src/components/playable.cairo (1)
2-2: Verify the architectural impact of making the module public.The change in visibility from private to public for the module and error constants expands their accessibility. This aligns with component-based architecture but requires careful consideration.
Consider:
- Document the public interface and its intended usage
- Ensure error messages are appropriate for public exposure
- Add integration tests to verify the component's public interface
Also applies to: 21-25
client/src/components/header.tsx (3)
92-103: Consider memoizing the profile lookup.The profile lookup is performed in a useEffect hook that runs on every change to
accountorplayerData. Consider memoizing the profile lookup to avoid unnecessary recalculations.+const profile = useMemo(() => { + return playerData?.mancalaSaltProfileModels?.edges.find( + (player: any) => player.node.address === account?.address, + ); +}, [account?.address, playerData?.mancalaSaltProfileModels?.edges]); -const profile: any = playerData?.mancalaSaltProfileModels?.edges.find( - (player: any) => player.node.address === account?.address, -);
246-248: Optimize user existence check.The current implementation iterates through all profiles to check if a user exists. Consider using a more efficient lookup method, such as maintaining a Set of addresses.
+const userAddresses = new Set( + profiles.mancalaSaltProfileModels.edges.map( + (profile: any) => profile.node.address + ) +); +const userExists = userAddresses.has(account?.address); -const userExists = profiles.mancalaSaltProfileModels.edges.some( - (profile: any) => profile.node.address === account?.address, -);
298-306: Simplify condition checks.The condition checks can be simplified for better readability.
-if ( - account?.address && - playerData?.mancalaSaltProfileModels?.edges && - !profile -) { +const hasAccount = Boolean(account?.address); +const hasProfiles = Boolean(playerData?.mancalaSaltProfileModels?.edges); +if (hasAccount && hasProfiles && !profile) {client/src/pages/Lobby.tsx (4)
93-94: Optimize game filtering.Consider memoizing the filtered games to avoid unnecessary recalculations on every render.
+const filteredGames = useMemo(() => { + return data?.mancalaSaltMancalaBoardModels?.edges + ?.filter(/* ... */) + .sort(/* ... */) + .map(/* ... */); +}, [data?.mancalaSaltMancalaBoardModels?.edges, account.account?.address, playerData]); -const filteredGames = data?.mancalaSaltMancalaBoardModels?.edges - ?.filter(/* ... */) - .sort(/* ... */) - .map(/* ... */);
119-128: Optimize multiple profile lookups.Consider creating a map of addresses to profiles to avoid multiple iterations through the profiles array.
+const profileMap = useMemo(() => { + return new Map( + playerData?.mancalaSaltProfileModels?.edges?.map( + (profile: any) => [profile.node.address, profile] + ) + ); +}, [playerData?.mancalaSaltProfileModels?.edges]); +const player1Profile = profileMap.get(game.node.player_one); +const player2Profile = profileMap.get(game.node.player_two); +const winner = profileMap.get(game.node.winner); -const player1Profile = playerData?.mancalaSaltProfileModels?.edges?.find( - (profile: any) => profile.node.address === game.node.player_one, -); -const player2Profile = playerData?.mancalaSaltProfileModels?.edges?.find( - (profile: any) => profile.node.address === game.node.player_two, -); -const winner = playerData?.mancalaSaltProfileModels?.edges?.find( - (profile: any) => profile.node.address === game.node.winner, -);
144-145: Optimize transaction filtering.Consider using filter and map instead of reduce for better readability and maintainability.
+const filteredTransactions = data?.mancalaSaltMancalaBoardModels?.edges + ?.filter((game: any) => + game?.node?.player_one === account.account?.address || + game?.node?.player_two === account.account?.address || + (game?.node?.entity.executedAt && game?.node?.player_two === "0x0") + ) + ?.map((game: any) => ({ + ...game.node, + executedAt: game?.node?.entity.executedAt, + })) || []; -const filteredTransactions = data?.mancalaSaltMancalaBoardModels?.edges?.reduce( - (acc: any[], game: any) => { - if ( - game?.node?.player_one === account.account?.address || - game?.node?.player_two === account.account?.address || - (game?.node?.entity.executedAt && game?.node?.player_two === "0x0") - ) { - acc.push({ - ...game.node, - executedAt: game?.node?.entity.executedAt, - }); - } - return acc; - }, - [], -) || [];
218-229: Simplify address extraction.The address extraction can be simplified using Set to automatically deduplicate addresses.
+const allAddresses = new Set( + data?.mancalaSaltMancalaBoardModels?.edges?.flatMap((game: any) => [ + game.node.player_one, + game.node.player_two, + game.node.winner + ].filter(Boolean)) +); -const allAddresses = data?.mancalaSaltMancalaBoardModels?.edges - .flatMap((game: any) => [ - game.node.player_one, - game.node.player_two, - game.node.winner, - ]) - .filter(Boolean);
🛑 Comments failed to post (3)
client/src/components/boards/board-block.tsx (1)
17-17: 🛠️ Refactor suggestion
Improve image handling and accessibility.
The dynamic background image URL construction could be unsafe and lacks error handling. Additionally, the image needs proper accessibility attributes.
Consider these improvements:
-<div className={`w-full h-64 bg-contain bg-center bg-no-repeat bg-[url('${image_path}')]`} /> +<div + className="w-full h-64 bg-contain bg-center bg-no-repeat" + style={{ backgroundImage: `url(${image_path})` }} + role="img" + aria-label={`${name} board image`} +> + {/* Add a fallback for image loading errors */} + <div className="hidden"> + <img src={image_path} alt="" onError={(e) => { + e.currentTarget.parentElement?.parentElement?.classList.add('bg-gray-200'); + }} /> + </div> +</div>📝 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.<div className="w-full h-64 bg-contain bg-center bg-no-repeat" style={{ backgroundImage: `url(${image_path})` }} role="img" aria-label={`${name} board image`} > {/* Add a fallback for image loading errors */} <div className="hidden"> <img src={image_path} alt="" onError={(e) => { e.currentTarget.parentElement?.parentElement?.classList.add('bg-gray-200'); }} /> </div> </div>client/src/components/header.tsx (1)
160-162: 🛠️ Refactor suggestion
Remove duplicate profile lookup.
This profile lookup is duplicated from the previous useEffect hook. Consider reusing the memoized profile to maintain consistency and improve maintainability.
contracts/torii_dev.toml (1)
5-5: 💡 Verification agent
🧩 Analysis chain
Verify the new world address deployment.
Please ensure that the new world address
0x8fb1bf2a0525b8c79ecfac766fa62933e26725effd8741a007f50896ea97d1is correctly deployed and accessible.
🏁 Script executed:
#!/bin/bash # Description: Verify world address deployment and accessibility curl -X POST \ -H "Content-Type: application/json" \ -d '{"jsonrpc":"2.0","method":"starknet_getClassAt","params":["0x8fb1bf2a0525b8c79ecfac766fa62933e26725effd8741a007f50896ea97d1"],"id":1}' \ https://api.cartridge.gg/x/starknet/sepoliaLength of output: 663
World address verification update needed
The verification script currently returns an "Invalid block id" error because the JSON-RPC method requires a block identifier parameter (e.g.,
"latest") alongside the world address. Please update the verification command accordingly to confirm that the new world address is deployed and accessible. For example, modify the JSON payload to:{ "jsonrpc": "2.0", "method": "starknet_getClassAt", "params": [ "0x8fb1bf2a0525b8c79ecfac766fa62933e26725effd8741a007f50896ea97d1", "latest" ], "id": 1 }
- Location:
contracts/torii_dev.toml(verify/update associated deployment/verification scripts if needed)
2 participants
Closes: #274
Summary by CodeRabbit
New Features
Chores
Refactor