-
Notifications
You must be signed in to change notification settings - Fork 32
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
Staging #228
Staging #228
Conversation
…pdated position data to include player and opponent states with native and non-native coordinates. Simplified position retrieval logic based on pit number and player type. Improved animation handling for seed positioning.
* opponents position update * isNative confirmations * pit 7 ops * pit 7 plays * pit 7 plays * prettier * update dojo config
* feat: fixed the trophy icon set * feat: arcade progression wip * feat: trophy element changes & controller client config * feat: torii config * feat: wip header
* fixed burner issue * fixed burner issue * fixed burner issue * loading screen upgrade * loading screen upgrade * up update * optimize tables * update ui * changed text color in game board * added error boundary ui * optimized user section * optimized user section * default url * auto connect * Merge into staging-ui-upgrade * add eliza * ui update * ui update * fixed build issue * eliza starter * eliza starter * animate-spin --------- Co-authored-by: kelvin_joshua <[email protected]>
* fix: patch first attempt * fix: path2 achievements
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces significant updates to the Mancala game project, focusing on adding entries to the Changes
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 (
|
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: 35
🔭 Outside diff range comments (2)
contracts/src/tests/test_world.cairo (1)
Line range hint
46-46
: Useset_caller_address
instead ofset_contract_address
for changing the callerYou are using
set_contract_address(ANYONE);
to change the caller in the test. However,set_contract_address
sets the address of the current contract, not the caller's address. To simulate calls from different accounts, you should useset_caller_address(ANYONE);
.Apply this diff to fix the issue:
- set_contract_address(ANYONE); + set_caller_address(ANYONE);contracts/dojo_dev.toml (1)
Line range hint
27-31
: Security: IPFS credentials should not be in version controlThe configuration file contains hardcoded IPFS credentials, which poses a security risk.
Consider:
- Moving credentials to environment variables
- Using a secure secrets management solution
- Rotating the exposed credentials immediately
-ipfs_config.username = "2EBrzr7ZASQZKH32sl2xWauXPSA" -ipfs_config.password = "12290b883db9138a8ae3363b6739d220" +ipfs_config.username = "${IPFS_USERNAME}" +ipfs_config.password = "${IPFS_PASSWORD}"
🧹 Nitpick comments (48)
client/src/AppWrapper.tsx (2)
27-34
: Consider enhancing edge case handlingWhile the current implementation is good, consider handling the case where
currentStep
exists but isn't a key inSETUP_STATUS_MESSAGES
.return currentStep - ? SETUP_STATUS_MESSAGES[currentStep as keyof typeof SETUP_STATUS_MESSAGES] + ? SETUP_STATUS_MESSAGES[currentStep as keyof typeof SETUP_STATUS_MESSAGES] ?? "Setting up game..." : "Loading...";
48-53
: Consider optimizing logo animation performanceThe animated logo looks good, but continuous animation might impact performance on lower-end devices. Consider:
- Using CSS
animation-duration
with a slower speed- Implementing
will-change: transform
for better performance- Adding
@media (prefers-reduced-motion: reduce)
support- <img src={logo} width={126} height={126} alt="logo" className="animate-spin" /> + <img + src={logo} + width={126} + height={126} + alt="logo" + className="animate-spin motion-safe:animate-spin will-change-transform" + style={{ animationDuration: '3s' }} + />client/src/components/lobby/leaderboard.tsx (3)
10-11
: Consider using responsive design patterns.The current implementation uses fixed dimensions which may cause layout issues on different screen sizes. Consider:
- Using relative units (e.g., vh, vw) or responsive classes for container heights
- Implementing responsive width classes instead of fixed pixel values
- Ensuring the scrollable div's height (500px) aligns with its container (437px)
- <div className="bg-[#0F1116] bg-contain bg-center bg-no-repeat border-2 border-[#272A32] rounded-2xl w-[928px] h-[437px] flex flex-col items-center justify-center px-8"> - <div className="w-[928px] rounded-t-xl rounded-b-xl fixed h-[500px] overflow-y-scroll hide-scrollbar pb-4"> + <div className="bg-[#0F1116] bg-contain bg-center bg-no-repeat border-2 border-[#272A32] rounded-2xl w-full max-w-[928px] h-[437px] flex flex-col items-center justify-center px-8"> + <div className="w-full rounded-t-xl rounded-b-xl fixed h-[437px] overflow-y-scroll hide-scrollbar pb-4">
20-26
: Review sticky header positioning.The negative top value in sticky positioning might cause inconsistent behavior across browsers. Consider using a positive offset and adjusting the container padding instead.
- <thead className="sticky -top-3.5 bg-[#0F1116] z-10"> + <thead className="sticky top-0 bg-[#0F1116] z-10">
35-105
: Consider performance optimizations for large datasets.The current implementation might face performance issues with large datasets. Consider:
- Implementing virtualization for the table rows
- Adding pagination or infinite scroll
- Memoizing the player row component
Would you like me to provide an example implementation using react-window or react-virtualized?
client/src/components/profile/game-history.tsx (2)
69-71
: Consider adding error handling for the empty state check.The empty state check could be more robust by handling undefined/null cases.
- } else if (data?.length === 0) { + } else if (!data || data.length === 0) {
105-111
: Improve date handling and memoize color calculations.The color and date calculations could be optimized:
- Date parsing on every render
- Color calculations could be memoized
+ const memoizedColors = useMemo(() => { + return data?.map((_, index) => ({ + challenger: colors[index % colors.length], + challenged: colors[(index + 3) % colors.length] + })); + }, [data?.length]); return ( // ... in the map function: - const challengerColor = colors[index % colors.length]; - const challengedColor = colors[(index + 3) % colors.length]; - const date = new Date(item.date); + const { challenger: challengerColor, challenged: challengedColor } = memoizedColors[index]; + const date = useMemo(() => new Date(item.date), [item.date]);contracts/src/components/initializable.cairo (1)
34-34
: Consider defining error messages in anerrors
module for consistency.For better maintainability and consistency across components, define the error message
'Counter already initialized'
in anerrors
module, similar to other components.Apply this diff to refactor:
+// Errors +mod errors { + const COUNTER_ALREADY_INITIALIZED: felt252 = 'Counter already initialized'; +} ... -assert(current_game_counter.count == 0, 'Counter already initialized'); +assert(current_game_counter.count == 0, errors::COUNTER_ALREADY_INITIALIZED);contracts/src/systems/profile.cairo (1)
38-46
: Add input validation forname
andnew_uri
parameters.Currently, there is no validation on the
name
andnew_uri
parameters in thecreate_player_profile
andupdate_player_profile
functions. Consider adding input validation to ensure these parameters meet expected criteria (e.g., length limits, acceptable character sets) to enhance security and prevent potential issues.contracts/src/components/profile.cairo (2)
27-39
: Add input validation forname
parameter innew_profile
function.To ensure data integrity and prevent potential issues, consider adding validation for the
name
parameter in thenew_profile
function.
41-55
: Add input validation forname
andnew_uri
parameters inupdate_player_profile
function.Adding input validation for the
name
andnew_uri
parameters in theupdate_player_profile
function will enhance security and prevent potential issues.client/src/components/header.tsx (1)
103-137
: Simplify conditional rendering for better readability.The current conditional rendering logic within the JSX is a bit complex and can be refactored for clarity. Consider creating separate components or helper functions for the different button states to make the code more maintainable and easier to read.
contracts/src/systems/mancala.cairo (1)
62-88
: Add error handling for the initialization process.In the
dojo_init
function, there is no error handling for the initialization processes ofinitializable
andachievable
components. Consider adding error handling or logging to capture any issues that may arise during initialization.Apply this diff to add error handling:
fn dojo_init(self: @ContractState) { - self.initializable.initialize(self.world_storage()); + let init_result = self.initializable.initialize(self.world_storage()); + if init_result.is_err() { + // Handle the error accordingly + } // [Event] Emit all Trophy events let world = self.world(@NAMESPACE()); let mut trophy_id: u8 = TROPHY_COUNT; while trophy_id > 0 { let trophy: Trophy = trophy_id.into(); let create_result = self .achievable .create( world, id: trophy.identifier(), hidden: trophy.hidden(), index: trophy.index(), points: trophy.points(), start: trophy.start(), end: trophy.end(), group: trophy.group(), icon: trophy.icon(), title: trophy.title(), description: trophy.description(), tasks: trophy.tasks(), data: trophy.data(), ); + if create_result.is_err() { + // Handle the error accordingly + } trophy_id -= 1; } }client/src/components/gameplay/game-board.tsx (2)
58-96
: Consider refactoring duplicateuseEffect
hooks for better maintainabilityThe
useEffect
hooks for handling captures (lines 58~ to 76~) and extra turns (lines 78~ to 96~) have similar structures. Refactoring this logic into a reusable function or a custom hook can reduce code duplication and improve code maintainability.Here's how you might abstract the common logic:
+const useGameEventNotification = (data, dataKey, titleMap, descriptionMap) => { + const { toast } = useToast(); + useEffect(() => { + const events = data?.[dataKey]?.edges; + if (events && events.length > 0) { + const latestEvent = events[events.length - 1]?.node; + + if (latestEvent) { + const isPlayerEvent = latestEvent.player === account.account?.address; + + toast({ + title: isPlayerEvent ? titleMap.player : titleMap.opponent, + description: isPlayerEvent + ? descriptionMap.player(latestEvent) + : descriptionMap.opponent(latestEvent), + duration: 3000, + }); + } + } + }, [data, account.account?.address, toast]); +}; + useEffect(() => { - const captures = captureData?.mancalaAlphaCaptureModels?.edges; - if (captures && captures.length > 0) { - const latestCapture = captures[captures.length - 1]?.node; - - if (latestCapture) { - const isPlayerCapture = - latestCapture.player === account.account?.address; - - toast({ - title: isPlayerCapture ? "Seeds Captured!" : "Seeds Lost!", - description: isPlayerCapture - ? `You captured ${latestCapture.seed_count} seeds from pit ${latestCapture.pit_number}` - : `Opponent captured ${latestCapture.seed_count} seeds from pit ${latestCapture.pit_number}`, - duration: 3000, - }); - } - } + useGameEventNotification( + captureData, + 'mancalaAlphaCaptureModels', + { player: 'Seeds Captured!', opponent: 'Seeds Lost!' }, + { + player: (event) => `You captured ${event.seed_count} seeds from pit ${event.pit_number}`, + opponent: (event) => `Opponent captured ${event.seed_count} seeds from pit ${event.pit_number}`, + } + ); }, [captureData, account.account?.address, toast]); useEffect(() => { - const extraTurns = extraTurnData?.mancalaAlphaPlayerExtraTurnModels?.edges; - if (extraTurns && extraTurns.length > 0) { - const latestExtraTurn = extraTurns[extraTurns.length - 1]?.node; - - if (latestExtraTurn) { - const isPlayerExtraTurn = - latestExtraTurn.player === account.account?.address; - - toast({ - title: isPlayerExtraTurn ? "Extra Turn!" : "Opponent Extra Turn", - description: isPlayerExtraTurn - ? "You get another turn!" - : "Opponent gets another turn", - duration: 3000, - }); - } - } + useGameEventNotification( + extraTurnData, + 'mancalaAlphaPlayerExtraTurnModels', + { player: 'Extra Turn!', opponent: 'Opponent Extra Turn' }, + { + player: () => 'You get another turn!', + opponent: () => 'Opponent gets another turn', + } + ); }, [extraTurnData, account.account?.address, toast]);
151-161
: Simplify seed count retrieval using.find
When calculating
player_pot_seed_count
andopponent_pot_seed_count
, you can replace the chained.filter()
calls with a single.find()
to improve efficiency and readability.Here's the suggested change:
-player_pot_seed_count = game_players?.mancalaAlphaPitModels.edges .filter( (item: any) => item?.node.player === game_players?.mancalaAlphaPlayerModels.edges[player_position]?.node.address, ) .filter((item: any) => item?.node.pit_number === 7)[0]?.node ?.seed_count || 0; +player_pot_seed_count = + game_players?.mancalaAlphaPitModels.edges + .find( + (item: any) => + item?.node.player === + game_players?.mancalaAlphaPlayerModels.edges[player_position]?.node.address && + item?.node.pit_number === 7, + )?.node + ?.seed_count || 0;Apply the same logic to
opponent_pot_seed_count
.client/src/components/gameplay/game-message.tsx (2)
205-285
: Avoid logging sensitive game data in productionThe
console.log
statements inconstructElizaMessage
(lines 207~ and 241~) can expose sensitive game data in the browser's console. Consider removing or conditioning these logs to prevent potential information leakage.[security_issue]
Remove or conditionally disable debug logs:
- console.log({ game_node }); ... - console.log({ - message - })
281-287
: Optimize address normalization functionThe
normalizeAddress
function can lead to inconsistent results if addresses are not in the expected format. Consider using a standardized library or method for address normalization to ensure consistency across different parts of the application.Alternatively, adjust the function to handle different possible address formats robustly.
contracts/src/types/trophy.cairo (2)
28-40
: Simplifylevel
method implementationThe
level
method contains repeated patterns that can be simplified. As all trophies exceptTrophy::None
have an associated level, consider refactoring to eliminate redundancy.Example:
fn level(self: Trophy) -> u8 { match self { Trophy::None => 0, _ => { // Define a mapping from Trophy variant to level, if possible. }, } }
267-272
: Implement a more informativeThe
Example:
fn print(self: Trophy) { self.title().print(); }contracts/src/tests/test_world.cairo (1)
389-389
: Consider removing commented-out codeThe code at line 389~ is commented out. If this code is no longer needed, consider removing it to keep the codebase clean. If you plan to use it later, add a comment explaining its purpose.
client/src/components/profile/user-section.tsx (1)
113-113
: Simplify null checks using optional chainingYou can simplify the null checks by using optional chaining when accessing
event.target.files[0]
.Apply this diff to simplify the code:
- if (event.target.files && event.target.files[0]) { + if (event.target.files?.[0]) {🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
contracts/src/components/playable.cairo (1)
243-264
: Duplicate code when updating task progressThe code for updating task progress after capturing seeds is duplicated. Consider refactoring this into a helper function to improve maintainability.
Extract the repeated code into a function, for example:
fn update_task_progress(player_address: ContractAddress, captured_seeds: u8, world: WorldStorage) { let arcade_store: ArcadeStore = ArcadeStoreTrait::new(world); let task_id = Task::Collecting.identifier(0); let extractor_task_id = Task::Clearing.identifier(0); let time = get_block_timestamp(); arcade_store.progress(player_address.into(), task_id, captured_seeds.into(), time); arcade_store.progress(player_address.into(), extractor_task_id, captured_seeds.into(), time); }Then call
update_task_progress
where needed.client/src/lib/constants.ts (2)
783-812
: Handle Possible Empty Results in New QueriesThe new
MancalaCaptureQuery
andMancalaExtraTurnQuery
have been added. Ensure that the application gracefully handles scenarios where these queries return empty results or errors to prevent runtime exceptions.Consider adding default values or checks when processing the data returned by these queries.
813-1100
: Optimize thepositions
Function for MaintainabilityThe
positions
function contains extensive hard-coded coordinate arrays, which can be difficult to maintain and prone to errors. Refactor the function to generate coordinates programmatically if possible.Consider the following suggestions:
Mathematical Generation: Use mathematical formulas to calculate positions based on indices.
Data Structures: Store patterns or offsets in a data structure that can be looped over.
Helper Functions: Break down the coordinate generation into smaller helper functions for clarity.
Example refactor:
export const positions = (type: "player" | "opponent" | undefined) => Array.from({ length: 7 }, (_, playerIndex) => { const xBase = type === "player" ? playerIndex * 120 : 600 - playerIndex * 120; const yBase = type === "player" ? 0 : -110; const generateCoords = () => { let coords = []; for (let i = 0; i < NUM_SEEDS; i++) { coords.push({ x: xBase + calculateXOffset(i), y: yBase + calculateYOffset(i), }); } return coords; }; return { player: { native: generateCoords(), non_native: generateCoords(), }, opponent: { native: generateCoords(), non_native: generateCoords(), }, }; });Where
calculateXOffset
andcalculateYOffset
are functions that compute offsets.contracts/src/elements/tasks/mastering.cairo (1)
11-11
: Improve description grammar.The current message "Get {} games win streak" could be clearer. Consider these alternatives:
- "Get a {} game win streak"
- "Win {} games in a row"
- format!("Get {} games win streak", count) + format!("Get a {} game win streak", count)contracts/src/elements/tasks/collecting.cairo (1)
11-11
: Improve description clarity and grammar.The current message has grammar issues and could be more concise:
- "seed" should be plural when count > 1
- "in game lifetime history" is verbose
- format!("Collect {} seed in game lifetime history", count) + format!("Collect {} seeds throughout your games", count)contracts/src/elements/trophies/interface.cairo (1)
4-15
: Well-structured trophy system interface.The TrophyTrait provides a comprehensive interface for trophy management with:
- Clear separation between trophy attributes and tasks
- Type-safe level parameters
- Flexible task integration via BushidoTask
Consider adding documentation comments for each method to improve maintainability.
client/src/components/controller-trophy.tsx (1)
16-20
: Add loading state to prevent multiple clicks.The button should show a loading state while the profile is being opened.
+ const [isLoading, setIsLoading] = useState(false); + + const handleTrophyClick = useCallback(async () => { + setIsLoading(true); if (!connector?.controller) { // error handling + setIsLoading(false); return; } - connector?.controller.openProfile("achievements"); + try { + await connector.controller.openProfile("achievements"); + } finally { + setIsLoading(false); + } }, [connector]); return ( - <Button variant="outline" onClick={() => handleTrophyClick()}> + <Button variant="outline" onClick={handleTrophyClick} disabled={isLoading}> <Trophy /> </Button> );client/src/components/lobby/empty-duels.tsx (1)
3-3
: Props interface should be extracted.For better maintainability and reuse, define a separate props interface.
+interface EmptyDuelsProps { + message: string; + onCreateGame?: () => void; +} -export default function EmptyDuels({ message }: { message: string }) +export default function EmptyDuels({ message, onCreateGame }: EmptyDuelsProps)contracts/src/lib.cairo (1)
21-51
: Well-structured module organization!The new module structure effectively separates concerns and provides a clear hierarchy for the game's components:
- Components layer for core functionalities
- Elements layer for task and trophy management
- Systems layer for game mechanics
- Types layer for shared enums
This organization promotes maintainability and follows good architectural practices.
Consider documenting the responsibility of each module layer in the README to help future contributors understand the codebase structure.
contracts/src/elements/trophies/victor.cairo (1)
3-54
: Well-structured trophy implementation!The implementation follows a clear pattern and provides all necessary properties for the trophy system.
Consider documenting magic numbers.
The implementation uses magic numbers for points (100) and count (100). Consider extracting these to named constants with documentation explaining their significance.
+/// Points awarded for achieving the Victor trophy +const VICTOR_POINTS: u16 = 100; +/// Number of tasks required to earn the Victor trophy +const VICTOR_TASK_COUNT: u32 = 100; + impl Victor of TrophyTrait { #[inline] fn points(level: u8) -> u16 { - 100 + VICTOR_POINTS } #[inline] fn count(level: u8) -> u32 { - 100 + VICTOR_TASK_COUNT } }contracts/src/elements/trophies/dominator.cairo (1)
3-54
: Consistent trophy implementation!The implementation follows the same pattern as other trophies, maintaining consistency across the codebase.
Consider extracting common patterns and documenting magic numbers.
- The implementation uses magic numbers for points (50) and count (500). Consider extracting these to named constants.
- There's potential for code duplication across trophy implementations. Consider extracting common patterns into a base implementation.
+/// Points awarded for achieving the Dominator trophy +const DOMINATOR_POINTS: u16 = 50; +/// Number of tasks required to earn the Dominator trophy +const DOMINATOR_TASK_COUNT: u32 = 500; + impl Dominator of TrophyTrait { #[inline] fn points(level: u8) -> u16 { - 50 + DOMINATOR_POINTS } #[inline] fn count(level: u8) -> u32 { - 500 + DOMINATOR_TASK_COUNT } }Consider creating a macro to reduce boilerplate across trophy implementations:
// Example macro (syntax may need adjustment) macro_rules! implement_trophy { ($name:ident, $id:expr, $points:expr, $count:expr) => { impl $name of TrophyTrait { // Common implementation details... } } }client/src/components/boundaries/error-boundary.tsx (1)
16-29
: Enhanced error UI with better visual hierarchy!The updated error boundary provides a more polished and user-friendly experience.
Consider accessibility improvements.
While the UI looks good, consider enhancing accessibility:
- Add aria-label to the error container
- Ensure sufficient color contrast for text visibility
- Add aria-live region for screen readers
<div role="alert" + aria-label="Error occurred" className="grid w-full h-screen place-items-center bg-[#0F1116] bg-[url('./assets/bg.png')] bg-cover bg-center" > <div className="flex flex-col items-center justify-center space-y-4"> - <div className="flex flex-col items-center justify-center space-y-1.5 w-[400px] h-[275px] rounded-lg bg-[#4920003D] backdrop-blur-md"> + <div + className="flex flex-col items-center justify-center space-y-1.5 w-[400px] h-[275px] rounded-lg bg-[#4920003D] backdrop-blur-md" + aria-live="polite" + > <img src={logo} width={126} height={126} alt="logo" /> <h2 className="text-white font-semibold">Ouch, an error occurred</h2> - <div className="text-white/80 text-center px-4">{error.message}</div> + <div className="text-white/90 text-center px-4">{error.message}</div>client/src/pages/Home.tsx (1)
24-27
: Consider adding loading state to the button.The connection process might take time, and the button should reflect this state to improve user experience.
<button className="bg-[#1A1D25] text-[#F58229] py-2.5 px-7 rounded-full flex flex-row items-center justify-center space-x-1"> <Bubble /> - <p>Go to lobby</p> + <p>{account ? 'Go to lobby' : 'Connecting...'}</p> </button>contracts/src/elements/trophies/strategist.cairo (1)
80-83
: Document the task type choice.The use of
Task::Mastering
instead ofTask::Collecting
should be documented to explain the different requirements.Add a comment explaining why Mastering tasks are used for the Strategist trophy:
fn tasks(level: u8) -> Span<BushidoTask> { + // Using Mastering tasks as they represent strategic gameplay achievements let total: u32 = Self::count(level); Task::Mastering.tasks(level, total, total) }
client/src/components/pits.tsx (1)
56-56
: Consider adding error handling for audio playback.While the sound effect integration is good, consider wrapping the
playEmptyPitSound()
call in a try-catch block to handle potential audio playback failures gracefully.- playEmptyPitSound(); + try { + playEmptyPitSound(); + } catch (error) { + console.warn('Failed to play sound effect:', error); + }client/src/components/seed.tsx (1)
24-34
: Consider memoizing position calculations more efficiently.The current implementation recalculates positions on every render. Consider simplifying the condition chain.
const position = useMemo(() => { - return isNative && type === "player" - ? _positions[pit_number - 1]?.player?.native[seed_number - 1] - : isNative && type === "opponent" - ? _positions[pit_number - 1]?.opponent?.native[seed_number - 1] - : !isNative && type === "player" - ? _positions[pit_number - 1]?.player?.non_native[seed_number - 1] - : !isNative && type === "opponent" - ? _positions[pit_number - 1]?.opponent?.non_native[seed_number - 1] - : { x: 0, y: 0 }; + if (!type || pit_number < 1 || seed_number < 1) { + return { x: 0, y: 0 }; + } + const positionType = isNative ? 'native' : 'non_native'; + return _positions[pit_number - 1]?.[type]?.[positionType]?.[seed_number - 1] || { x: 0, y: 0 }; }, [isNative, type, _positions, pit_number, seed_number]);client/src/pages/Profile.tsx (1)
39-44
: Consider adding error handling for auto-connect.While auto-connect improves UX, it should handle potential connection failures gracefully.
useEffect(() => { if (!account?.account?.address) { - connect({ connector: connectors[0] }); + try { + connect({ connector: connectors[0] }); + } catch (error) { + console.error('Failed to auto-connect:', error); + // Consider showing a user-friendly error message + } } }, [account, connect, connectors]);client/src/components/lobby/duels-skeleton.tsx (1)
43-43
: Consider making the number of skeleton rows configurable.The hardcoded value of 5 rows could be made configurable to better match different view states.
+const SKELETON_ROWS = 5; export function DuelsSkeleton() { return ( // ... - {Array.from({ length: 5 }).map((_, index) => { + {Array.from({ length: SKELETON_ROWS }).map((_, index) => {client/src/pages/games/Gameplay.tsx (1)
55-60
: Consider user consent for automatic wallet connection.While automatic connection enhances UX, it's best practice to request user consent before connecting to their wallet. Consider adding a connection prompt or button for better user control.
- useEffect(() => { - if (!account?.account?.address) { - connect({ connector: connectors[0] }); - } - }, [account, connect, connectors]); + useEffect(() => { + const autoConnect = localStorage.getItem('autoConnect') === 'true'; + if (!account?.account?.address && autoConnect) { + connect({ connector: connectors[0] }); + } + }, [account, connect, connectors]);client/src/components/lobby/live-duels.tsx (1)
12-30
: Consider making the filter condition more explicit.While filtering out "0x0" addresses is correct, consider making the validation more explicit for better maintainability.
- const data = games - ?.filter( - (game: any) => - game.node.player_one !== "0x0" && game.node.player_two !== "0x0", - ) + const isValidAddress = (address: string) => address !== "0x0"; + const data = games + ?.filter( + (game: any) => + isValidAddress(game.node.player_one) && isValidAddress(game.node.player_two), + )contracts/src/utils/board.cairo (1)
Line range hint
214-243
: LGTM! Consider documenting the return value.The modification to return the total number of captured seeds is a good addition that could be useful for tracking game statistics or achievements. The implementation correctly calculates the total before modifying the game state.
Consider adding a comment to document that the function returns the total number of seeds captured, which could help other developers understand the purpose of this return value.
client/src/components/lobby/duels.tsx (2)
102-136
: Consider extracting table components for better maintainability.The table structure is well-organized but quite complex. Consider extracting the table header and row components to improve code maintainability and reusability.
291-330
: Enhance accessibility for sharing buttons.The sharing functionality is well-implemented, but consider adding ARIA labels and keyboard navigation support for better accessibility.
Example improvements:
-<div className="flex flex-row items-center justify-center space-x-1 pr-10 cursor-pointer" +<button + aria-label={copied ? "Game link copied" : "Copy game link"} + className="flex flex-row items-center justify-center space-x-1 pr-10 cursor-pointer"client/src/pages/Lobby.tsx (2)
373-397
: Consider using radio group for player type selection.The player type selection using buttons could be improved by using a radio group component for better accessibility and state management.
Example improvement:
-<div className="flex flex-row items-center justify-center space-x-3.5"> +<RadioGroup + value={playWith} + onValueChange={setPlayWith} + className="flex flex-row items-center justify-center space-x-3.5" +> <button>Human Player</button> <button>Play with Eliza AI</button> -</div> +</RadioGroup>
400-408
: Remove unnecessary fragment.The fragment wrapper is redundant as it contains only one child element.
Apply this diff to fix the issue:
-{playWith === "Human" && ( - <> - <input - className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium" - placeholder="0x..." - onChange={(e) => setPlayer2(e.target.value)} - /> - </> -)} +{playWith === "Human" && ( + <input + className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium" + placeholder="0x..." + onChange={(e) => setPlayer2(e.target.value)} + /> +)}🧰 Tools
🪛 Biome (1.9.4)
[error] 401-407: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
contracts/src/elements/trophies/extractor.cairo (1)
20-22
: Consider extracting magic numbers as constantsThe
points()
andcount()
methods use magic numbers (100 and 10000). Consider extracting these as named constants for better maintainability.+const EXTRACTOR_POINTS: u16 = 100; +const EXTRACTOR_COUNT: u32 = 10000; + impl Extractor of TrophyTrait { #[inline] fn points(level: u8) -> u16 { - 100 + EXTRACTOR_POINTS } #[inline] fn count(level: u8) -> u32 { - 10000 + EXTRACTOR_COUNT }Also applies to: 45-47
contracts/manifest_dev.json (1)
1611-1616
: New achievement system implementation detected.The addition of AchievableComponent and trophy-related events (TrophyCreation, TrophyProgression) suggests a new achievement system. This enhances the game's engagement mechanics.
Consider implementing the following for the achievement system:
- Achievement progress persistence
- Achievement notification system
- Achievement unlock conditions validation
Also applies to: 1961-1968
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (39)
.github/workflows/test.yml
is excluded by none and included by none.gitignore
is excluded by none and included by noneclient/pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
,!client/pnpm-lock.yaml
and included byclient/**
client/src/assets/bg.png
is excluded by!**/*.png
and included byclient/**
client/src/assets/home-box.png
is excluded by!**/*.png
and included byclient/**
client/src/assets/small-logo.png
is excluded by!**/*.png
and included byclient/**
client/src/assets/square-avatar.png
is excluded by!**/*.png
and included byclient/**
client/src/dojo/generated/generated.ts
is excluded by!**/generated/**
and included byclient/**
client/src/dojo/generated/setup.ts
is excluded by!**/generated/**
and included byclient/**
client/src/music/empty-pit.mp3
is excluded by!**/*.mp3
and included byclient/**
client/src/music/seed-drop.mp3
is excluded by!**/*.mp3
and included byclient/**
contracts/Scarb.lock
is excluded by!**/*.lock
and included bycontracts/**
eliza-starter/.env.example
is excluded by none and included by noneeliza-starter/.gitignore
is excluded by none and included by noneeliza-starter/LICENSE
is excluded by none and included by noneeliza-starter/README.md
is excluded by none and included by noneeliza-starter/actions/join_game_action.ts
is excluded by none and included by noneeliza-starter/actions/move_game_action.ts
is excluded by none and included by noneeliza-starter/actions/new_game_action.ts
is excluded by none and included by noneeliza-starter/characters/eliza.character.json
is excluded by none and included by noneeliza-starter/characters/tate.character.json
is excluded by none and included by noneeliza-starter/characters/trump.character.json
is excluded by none and included by noneeliza-starter/dist/actions/join_game_action.d.ts
is excluded by!**/dist/**
and included by noneeliza-starter/dist/actions/move_game_action.d.ts
is excluded by!**/dist/**
and included by noneeliza-starter/dist/actions/new_game_action.d.ts
is excluded by!**/dist/**
and included by noneeliza-starter/dist/environment.d.ts
is excluded by!**/dist/**
and included by noneeliza-starter/dist/src/character.d.ts
is excluded by!**/dist/**
and included by noneeliza-starter/dist/src/index.d.ts
is excluded by!**/dist/**
and included by noneeliza-starter/dist/utils/index.d.ts
is excluded by!**/dist/**
and included by noneeliza-starter/environment.ts
is excluded by none and included by noneeliza-starter/message-templates.json
is excluded by none and included by noneeliza-starter/package.json
is excluded by none and included by noneeliza-starter/pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
and included by noneeliza-starter/src/character.ts
is excluded by none and included by noneeliza-starter/src/index.ts
is excluded by none and included by noneeliza-starter/tsconfig.json
is excluded by none and included by noneeliza-starter/utils/index.ts
is excluded by none and included by nonepackage.json
is excluded by none and included by nonepnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
and included by none
📒 Files selected for processing (74)
client/.gitignore
(1 hunks)client/dev.sh
(1 hunks)client/dojoConfig.ts
(1 hunks)client/package.json
(3 hunks)client/src/App.tsx
(2 hunks)client/src/AppWrapper.tsx
(2 hunks)client/src/components/boundaries/error-boundary.tsx
(2 hunks)client/src/components/controller-trophy.tsx
(1 hunks)client/src/components/gameplay/end-game-button.tsx
(1 hunks)client/src/components/gameplay/game-board.tsx
(7 hunks)client/src/components/gameplay/game-message.tsx
(6 hunks)client/src/components/gameplay/game-navigation.tsx
(1 hunks)client/src/components/gameplay/restart-button.tsx
(1 hunks)client/src/components/gameplay/timeout-button.tsx
(1 hunks)client/src/components/header.tsx
(3 hunks)client/src/components/lobby/duels-skeleton.tsx
(1 hunks)client/src/components/lobby/duels.tsx
(2 hunks)client/src/components/lobby/empty-duels.tsx
(1 hunks)client/src/components/lobby/leaderboard.tsx
(2 hunks)client/src/components/lobby/live-duels.tsx
(5 hunks)client/src/components/message-area.tsx
(2 hunks)client/src/components/pits.tsx
(3 hunks)client/src/components/profile/empty-game-history.tsx
(1 hunks)client/src/components/profile/game-history-skeleton.tsx
(1 hunks)client/src/components/profile/game-history.tsx
(1 hunks)client/src/components/profile/user-section.tsx
(5 hunks)client/src/components/seed.tsx
(2 hunks)client/src/components/ui/dialog.tsx
(1 hunks)client/src/components/ui/svgs/bubble.tsx
(1 hunks)client/src/components/ui/svgs/users-plus.tsx
(1 hunks)client/src/dojo/DojoContext.tsx
(2 hunks)client/src/dojo/createSystemCalls.ts
(1 hunks)client/src/hooks/useAudioControl.ts
(4 hunks)client/src/hooks/useControllerData.ts
(0 hunks)client/src/lib/config.ts
(1 hunks)client/src/lib/constants.ts
(9 hunks)client/src/pages/Home.tsx
(2 hunks)client/src/pages/Leaderboard.tsx
(0 hunks)client/src/pages/Lobby.tsx
(19 hunks)client/src/pages/Profile.tsx
(4 hunks)client/src/pages/games/Gameplay.tsx
(2 hunks)contracts/.tool-versions
(1 hunks)contracts/Scarb.toml
(2 hunks)contracts/dojo_dev.toml
(2 hunks)contracts/dojo_sepolia.toml
(2 hunks)contracts/dojo_slot.toml
(1 hunks)contracts/manifest_dev.json
(18 hunks)contracts/manifest_sepolia.json
(18 hunks)contracts/src/components/initializable.cairo
(1 hunks)contracts/src/components/playable.cairo
(14 hunks)contracts/src/components/profile.cairo
(1 hunks)contracts/src/constants.cairo
(1 hunks)contracts/src/elements/tasks/clearing.cairo
(1 hunks)contracts/src/elements/tasks/collecting.cairo
(1 hunks)contracts/src/elements/tasks/dominating.cairo
(1 hunks)contracts/src/elements/tasks/interface.cairo
(1 hunks)contracts/src/elements/tasks/mastering.cairo
(1 hunks)contracts/src/elements/tasks/reigning.cairo
(1 hunks)contracts/src/elements/trophies/collector.cairo
(1 hunks)contracts/src/elements/trophies/dominator.cairo
(1 hunks)contracts/src/elements/trophies/extractor.cairo
(1 hunks)contracts/src/elements/trophies/interface.cairo
(1 hunks)contracts/src/elements/trophies/strategist.cairo
(1 hunks)contracts/src/elements/trophies/victor.cairo
(1 hunks)contracts/src/lib.cairo
(1 hunks)contracts/src/systems/actions.cairo
(0 hunks)contracts/src/systems/mancala.cairo
(1 hunks)contracts/src/systems/profile.cairo
(1 hunks)contracts/src/tests/setup.cairo
(4 hunks)contracts/src/tests/test_world.cairo
(15 hunks)contracts/src/types/task.cairo
(1 hunks)contracts/src/types/trophy.cairo
(1 hunks)contracts/src/utils/board.cairo
(2 hunks)contracts/torii_dev.toml
(1 hunks)
💤 Files with no reviewable changes (3)
- contracts/src/systems/actions.cairo
- client/src/pages/Leaderboard.tsx
- client/src/hooks/useControllerData.ts
✅ Files skipped from review due to trivial changes (9)
- contracts/.tool-versions
- client/src/components/gameplay/timeout-button.tsx
- contracts/src/constants.cairo
- client/src/components/gameplay/restart-button.tsx
- client/src/components/gameplay/end-game-button.tsx
- client/src/components/ui/svgs/users-plus.tsx
- client/src/lib/config.ts
- client/.gitignore
- client/src/components/ui/svgs/bubble.tsx
🧰 Additional context used
🪛 Shellcheck (0.10.0)
client/dev.sh
[warning] 2-2: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
🪛 Biome (1.9.4)
client/src/pages/Lobby.tsx
[error] 401-407: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
client/src/components/profile/user-section.tsx
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
client/src/pages/games/Gameplay.tsx
[error] 39-43: 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)
🪛 Gitleaks (8.21.2)
contracts/dojo_slot.toml
27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
contracts/dojo_sepolia.toml
27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (58)
client/src/dojo/DojoContext.tsx (3)
27-27
: LGTM: Client extraction from setup resultThe addition of the
client
property aligns with theSetupResult
type and maintains consistency with the context interface.
36-36
: Good refactoring: Simplified burner manager data handlingThe change to store the complete burner manager data improves maintainability by eliminating the need to manually update destructured properties when the burner manager interface changes.
43-46
: LGTM: Robust context value constructionThe changes provide:
- Complete burner manager functionality through spreading
- Proper fallback to masterAccount
- Type-safe inclusion of the client property
client/src/AppWrapper.tsx (2)
3-3
: Well-structured setup status messagesGood addition of user-friendly status messages with proper typing. The messages are clear and informative for users during the loading process.
Also applies to: 7-7, 10-19
22-25
: LGTM: Type-safe state managementGood use of TypeScript's Partial type for setup status state management.
contracts/src/components/initializable.cairo (1)
1-45
: Initialization logic is correctly implemented.The
initialize
function properly sets up the game counter with appropriate checks and updates.contracts/src/types/task.cairo (1)
1-81
: LGTM!The implementation of the
Task
enum and its associated traits is clean and well-structured. The conversion implementations betweenTask
andu8
are correctly defined, and the match statements cover all task variants with appropriate default cases.contracts/src/systems/mancala.cairo (1)
67-87
: Verify the trophy initialization logic to prevent off-by-one errors.In the loop initializing trophies, the loop decrements
trophy_id
until it reaches zero. Ensure that all intended trophies are initialized and that you're not missing the trophy withtrophy_id
zero if it's valid.Run the following script to check the range of initialized trophies:
If trophy ID
0
is a valid trophy, consider adjusting the loop condition to include it.✅ Verification successful
Trophy initialization logic is correct
The current implementation correctly initializes all valid trophies. Trophy ID 0 represents
Trophy::None
, which is a special case used to represent invalid or no trophy, and should not be initialized. The loop correctly processes all valid trophies from 9 down to 1, matching theTROPHY_COUNT
of 9.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that trophies with IDs from 1 to TROPHY_COUNT are initialized. # Test: Check for usage of trophy_id == 0 ast-grep --pattern $'let mut trophy_id: u8 = TROPHY_COUNT;\nwhile trophy_id > 0 {\n $$$\n trophy_id -= 1;\n}'Length of output: 108
Script:
#!/bin/bash # Find Trophy type definition and TROPHY_COUNT ast-grep --pattern 'enum Trophy {' # Find TROPHY_COUNT constant rg "const TROPHY_COUNT" -A 2 # Look for any trophy with ID 0 references rg "Trophy::(\w+) = 0" --type cairoLength of output: 276
Script:
#!/bin/bash # View trophy type definition cat contracts/src/types/trophy.cairo # Search for any zero references in trophy definitions without file type restriction rg "Trophy::\w+ = 0"Length of output: 11443
client/src/components/gameplay/game-board.tsx (1)
206-215
: Verify logic for determiningisPlayerSeed
In the function determining
isPlayerSeed
(lines 206~ to 215~), ensure that the logic correctly handles cases whereaccount.account?.address
may beundefined
or when the user is not logged in. This could lead to unexpected behavior when determining seed ownership.client/src/components/gameplay/game-message.tsx (1)
132-154
: Handle Eliza's states consistently inmoveMessageOnTimer
In the
moveMessageOnTimer
function, the conditions for displaying Eliza's messages are nested within the check forgame_node?.winner === "0x0"
. Ensure that the logic correctly handles scenarios where the game has a winner or the game status changes, to prevent inconsistent UI messages.contracts/src/components/playable.cairo (2)
46-46
: Validate state mutation withref self
The change to use
ref self: ComponentState<TState>
indicates intentional state mutation. Ensure that this aligns with the component's design and that all state mutations are managed correctly.
228-234
: Handle potential integer overflows when updating tasksWhen updating task progress with
arcade_store.progress
, ensure that thecaptured_seeds
value is within the expected range to prevent integer overflows.client/src/lib/constants.ts (3)
619-655
: Confirm Policy Targets and Methods AlignmentIn the
POLICIES
array, thetarget
for each policy has been updated to useMANCALA_ADDRESS
andPROFILE_ADDRESS
. Ensure that each method corresponds to the correct contract address and that the smart contracts expose these methods.
Line range hint
661-771
: Update GraphQL Queries to Reflect New Backend ModelsThe GraphQL queries have been updated to use the
mancalaAlpha
namespace (e.g.,mancalaAlphaMancalaBoardModels
). Verify that the backend service has these models and that the queries will function as expected.
608-615
: Verify the Accuracy of Contract AddressesNew constants
MANCALA_ADDRESS
,PROFILE_ADDRESS
, andELIZA_ADDRESS
have been added. Please ensure that these addresses are correct and correspond to the intended smart contracts on the specified network.Run the following script to verify the addresses:
✅ Verification successful
Contract Addresses Verified Successfully
The addresses are consistently used throughout the codebase and their usage patterns align with their intended purposes. MANCALA_ADDRESS is used for game operations, PROFILE_ADDRESS for profile management, and ELIZA_ADDRESS for AI opponent functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the contract addresses are correctly defined. # Expected addresses (update these with the correct values if known) EXPECTED_MANCALA_ADDRESS="0x470f026bf97f0781492a95ebb9031da1c524601e8c2a285534545d33ca44797" EXPECTED_PROFILE_ADDRESS="0x4a992660cbd6a91cda1e1697e43c52641f3818aeabcf96658a986322603ef85" EXPECTED_ELIZA_ADDRESS="0x076795eb2cdc3e799364f661a409edbe0f204b67625b179a6880733893f7004d" # Actual addresses from the code ACTUAL_MANCALA_ADDRESS="0x470f026bf97f0781492a95ebb9031da1c524601e8c2a285534545d33ca44797" ACTUAL_PROFILE_ADDRESS="0x4a992660cbd6a91cda1e1697e43c52641f3818aeabcf96658a986322603ef85" ACTUAL_ELIZA_ADDRESS="0x076795eb2cdc3e799364f661a409edbe0f204b67625b179a6880733893f7004d" # Verify addresses if [ "$ACTUAL_MANCALA_ADDRESS" != "$EXPECTED_MANCALA_ADDRESS" ]; then echo "MANCALA_ADDRESS does not match the expected address." fi if [ "$ACTUAL_PROFILE_ADDRESS" != "$EXPECTED_PROFILE_ADDRESS" ]; then echo "PROFILE_ADDRESS does not match the expected address." fi if [ "$ACTUAL_ELIZA_ADDRESS" != "$EXPECTED_ELIZA_ADDRESS" ]; then echo "ELIZA_ADDRESS does not match the expected address." fiLength of output: 992
Script:
#!/bin/bash # Search for occurrences of these addresses and related contract references echo "=== Searching for MANCALA_ADDRESS ===" rg -i "0x470f026bf97f0781492a95ebb9031da1c524601e8c2a285534545d33ca44797" --type ts --type js rg -i "mancala.*address" --type ts --type js echo -e "\n=== Searching for PROFILE_ADDRESS ===" rg -i "0x4a992660cbd6a91cda1e1697e43c52641f3818aeabcf96658a986322603ef85" --type ts --type js rg -i "profile.*address" --type ts --type js echo -e "\n=== Searching for ELIZA_ADDRESS ===" rg -i "0x076795eb2CDc3E799364F661A409EDBE0f204b67625B179a6880733893f7004d" --type ts --type js rg -i "eliza.*address" --type ts --type js echo -e "\n=== Looking for contract deployment files ===" fd -e ts -e js -e json "deploy|contract" --type fLength of output: 4188
contracts/src/elements/tasks/interface.cairo (1)
1-4
: IntroduceTaskTrait
Interface CorrectlyThe
TaskTrait
is defined with two methods:identifier
anddescription
. Ensure that the return types and parameters match the intended use and that any implementing structures conform to this trait.contracts/src/elements/tasks/clearing.cairo (1)
10-12
:⚠️ Potential issueEnsure Proper String Handling in
description
MethodThe
description
method usesformat!
to create a string. Verify that theformat!
macro is supported and that it returns aByteArray
as intended. In Cairo, string manipulation is limited, and you may need to construct theByteArray
manually.Consider constructing the
ByteArray
explicitly:fn description(count: u32) -> ByteArray { - format!("Collect {} seed", count) + ByteArray::new("Collect ") + ByteArray::from_u32(count) + ByteArray::new(" seed") }Ensure that the
ByteArray
operations are valid and that the necessary imports or helper functions are included.Likely invalid or redundant comment.
contracts/src/elements/tasks/dominating.cairo (1)
1-13
: LGTM! Clean and well-structured implementation.The implementation correctly follows the TaskTrait interface with appropriate use of #[inline] attributes and clear description formatting.
contracts/src/elements/tasks/reigning.cairo (1)
1-13
: LGTM! Clear and precise implementation.The implementation correctly follows the TaskTrait interface with good description clarity by using "total" in the message.
client/dojoConfig.ts (2)
7-7
: Environment change from test to staging confirmed.The toriiUrl endpoint change from
mancala-t
tomancala-e
aligns with the staging transition.
9-9
: Verify the masterAddress case change.The masterAddress has a case change from 'E' to 'e'. While Starknet addresses are case-insensitive for hex characters, let's verify this doesn't impact any existing integrations.
✅ Verification successful
Case difference in masterAddress is safe to proceed
The case change from 'E' to 'e' in the masterAddress is safe as:
- Starknet addresses are case-insensitive for hex characters
- The address appears in only two isolated contexts (contract config and client config)
- No other variations or dependent code found that could be impacted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any hardcoded references to the old address format rg -i "0x02A54eFf65D55a2E7D2fC7E23eF0F0e6030dB0313b16c8F0eaD0b4a83807f6d6"Length of output: 284
Script:
#!/bin/bash # Search for imports of dojoConfig.ts and dojo_sepolia.toml echo "=== Searching for dojoConfig.ts imports ===" rg -l "dojoConfig" echo -e "\n=== Searching for dojo_sepolia.toml usage ===" rg -l "dojo_sepolia" echo -e "\n=== Searching for any other variations of the address with different casing ===" # Using parts of the address to catch any variations rg -i "2[aA]54[eE][fF][fF].*2[eE]7[dD]2[fF][cC]7[eE]23"Length of output: 754
client/src/pages/Home.tsx (1)
13-17
: 🛠️ Refactor suggestionAdd error handling for connection logic.
The auto-connection logic should handle cases where no connectors are available to prevent potential runtime errors.
Consider adding error handling:
useEffect(() => { if (!account?.address) { + if (connectors.length === 0) { + console.warn("No connectors available"); + return; + } connect({ connector: connectors[0] }); } }, [account, connect, connectors]);contracts/src/elements/trophies/collector.cairo (1)
80-83
: Verify task generation logic.The task generation uses the same value for both parameters of
tasks()
, which might not be intentional.✅ Verification successful
Task generation logic is correct
The consistent pattern of using the same value for both
count
andtotal
parameters is intentional and correct. All trophy implementations follow this pattern, and theTask::tasks
method uses these parameters for different purposes:count
for description generation andtotal
for task creation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check task generation patterns across trophies rg "Task::\w+\.tasks\(level, total, total\)" --type cairoLength of output: 89
Script:
#!/bin/bash # Search for task generation patterns without file type restriction echo "=== Task generation patterns ===" rg "Task::\w+\.tasks\(.*\)" echo -e "\n=== Task enum definition ===" rg "enum Task" echo -e "\n=== All task method implementations ===" rg "fn tasks.*->.*BushidoTask"Length of output: 1425
Script:
#!/bin/bash # Look for Task enum implementation and its tasks method echo "=== Task implementation ===" rg -A 10 "enum Task \{" contracts/src/types/task.cairo echo -e "\n=== Task::tasks method implementation ===" rg -A 5 "fn tasks\(" contracts/src/types/task.cairoLength of output: 691
contracts/src/elements/trophies/strategist.cairo (1)
25-32
: Points progression is well-balanced.The doubling of points per level (20 → 40 → 80) provides a good balance between reward and challenge.
client/src/App.tsx (2)
18-23
: Verify the configuration values for alpha deployment.The namespace and slot values have been updated for alpha stage. Ensure these values match the deployment configuration.
65-72
: 🛠️ Refactor suggestionClean up leaderboard-related code.
The leaderboard route has been removed, but there might be leftover references or components.
Ensure all leaderboard-related code is removed to maintain a clean codebase.
client/src/components/pits.tsx (1)
3-3
: LGTM! Audio feedback integration looks good.The addition of the
useAudioControl
hook and its integration enhances the user experience by providing audio feedback.Also applies to: 47-47
contracts/src/tests/setup.cairo (3)
12-14
: LGTM! System dispatcher transition looks good.The transition from actions to a dedicated Mancala system dispatcher improves the code organization by making it more domain-specific.
74-77
: LGTM! Systems struct initialization is correct.The initialization of the Mancala system dispatcher is properly implemented.
49-53
: Verify trophy system integration.The addition of trophy-related events suggests a new achievement system. Please ensure that:
- Trophy creation and progression events are properly handled
- Achievement triggers are correctly implemented in the Mancala system
client/src/pages/Profile.tsx (1)
18-18
: Verify data model transition to alpha environment.The transition from dev to alpha environment (
mancalaDevMancalaBoardModels
→mancalaAlphaMancalaBoardModels
) needs verification:
- Ensure all queries are updated consistently
- Verify data schema compatibility
Also applies to: 36-36
client/src/components/profile/game-history-skeleton.tsx (1)
8-77
: LGTM! Skeleton UI improvements enhance user experience.The restructured skeleton loading state using Material Tailwind components provides:
- Better visual hierarchy with Card component
- Improved layout organization
- Consistent styling with the actual content
client/src/components/lobby/duels-skeleton.tsx (1)
7-81
: LGTM! Consistent skeleton UI implementation.The skeleton UI implementation maintains consistency with the game history component while providing:
- Clean table layout
- Proper spacing and alignment
- Responsive design considerations
client/src/pages/games/Gameplay.tsx (2)
62-62
: Clean component structure with proper styling.The main component's structure is well-organized with appropriate styling and background implementation.
1-1
: Environment transition looks good, but verify data model compatibility.The transition from dev to alpha environment (
mancalaDevMancalaBoardModels
→mancalaAlphaMancalaBoardModels
) is implemented correctly. The addition of connection management enhances user experience.Run this script to verify the data model compatibility:
Also applies to: 5-5, 34-34, 39-40
client/src/components/ui/dialog.tsx (1)
1-120
: Well-implemented dialog components with proper accessibility.The dialog implementation follows best practices:
- Uses Radix UI for robust accessibility
- Proper TypeScript typing with forwardRef
- Includes screen reader support
- Clean component composition
client/src/dojo/createSystemCalls.ts (1)
62-64
: Verify contract event emission order change.The change from
events[1]
toevents[0]
suggests a modification in contract event emission order. While the optional chaining is a good addition, we should verify this change.client/src/components/lobby/live-duels.tsx (1)
38-40
: Good empty state handling.The implementation of empty state with a customizable message is clean and user-friendly.
client/src/components/lobby/duels.tsx (1)
4-17
: LGTM! Clean implementation of clipboard functionality.The addition of clipboard-related icons and state management is well-structured and follows React best practices.
Also applies to: 94-95
contracts/torii_dev.toml (1)
17-20
: Verify historical events configurationThe configuration tracks trophy-related events. Ensure these event names match the exact event names emitted by the contracts.
Let's verify the event names in the contracts:
✅ Verification successful
Event names in historical configuration are correct
The event names
mancala_alpha-TrophyCreation
andmancala_alpha-TrophyProgression
match exactly with the events defined in the contracts and are consistently used across all configuration files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for trophy-related event definitions rg -A 2 "event\s+(TrophyCreation|TrophyProgression)" --type cairoLength of output: 97
Script:
#!/bin/bash # First find cairo files echo "=== Finding Cairo files ===" fd -e cairo # Then search for trophy events (both with and without prefix) echo -e "\n=== Searching for trophy events ===" rg -i "event.*trophy(creation|progression)" # Also search with exact names from config echo -e "\n=== Searching for exact event names ===" rg "mancala_alpha-TrophyCreation|mancala_alpha-TrophyProgression"Length of output: 3208
contracts/Scarb.toml (1)
4-4
: Version bump and dependency updates look goodThe version bump to 0.3.0 is appropriate given the significant changes. Dependencies are correctly pinned to specific versions.
Also applies to: 13-14
contracts/dojo_slot.toml (2)
29-31
: Security: Same IPFS credential exposure issueSimilar to dojo_dev.toml, this file also contains hardcoded IPFS credentials.
33-34
: Verify raw event processing configurationThe configuration enables raw event processing and tracks trophy progression events. Ensure this aligns with the event processing capabilities of your infrastructure.
Let's verify the event processing setup:
✅ Verification successful
Raw event processing configuration is properly integrated
The configuration aligns with the project's infrastructure as evidenced by consistent raw event processing setup across development and Sepolia environments, proper manifest integration, and test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for raw event processing implementations rg -l "TrophyProgression.*event" --type cairoLength of output: 77
Script:
#!/bin/bash # Search for TrophyProgression without file type restriction rg -l "TrophyProgression" # Search for event processing related configurations rg -l "raw.*=.*true" --type toml # Find all dojo_slot.toml files fd "dojo_slot.toml"Length of output: 398
contracts/dojo_sepolia.toml (3)
19-21
: LGTM! Writers configuration updated for the new namespace.The writers configuration has been properly updated to reflect the new namespace structure, including trophy-related components.
33-34
: LGTM! Additional configuration flags for trophy system.The raw flag and historical tracking for trophy progression have been properly configured.
7-8
: Verify the configuration changes for the staging environment.The changes in world configuration indicate a transition from development to alpha stage:
- Seed updated to "mancala-achievement"
- Namespace changed to "mancala_alpha"
These changes align with the staging objectives.
Run the following script to verify the configuration consistency across files:
Also applies to: 16-16
✅ Verification successful
Configuration changes for staging environment are properly implemented
The configuration changes show correct environment separation:
- Staging environment (dojo_sepolia.toml) uses unique seed "mancala-achievement"
- Development environments maintain original "mancala" seed
- Namespace "mancala_alpha" is consistently set across all environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any inconsistencies in namespace and seed configuration # across different environment files. echo "Checking namespace and seed configuration..." rg -l "mancala_dev|mancala_alpha|mancala-achievement" --type tomlLength of output: 268
Script:
#!/bin/bash # Check actual content of configuration files for seed and namespace values echo "Checking seed configurations..." rg -A 1 -B 1 "^seed\s*=" --type toml echo -e "\nChecking namespace configurations..." rg -A 1 -B 1 "namespace" --type tomlLength of output: 1047
client/src/components/message-area.tsx (1)
60-60
: Verify the useEffect dependencies are exhaustive.The restart_game function has been added to the dependency array, but ensure all other dependencies used within the effect are also included.
Run the following script to check for potential missing dependencies:
client/package.json (3)
29-29
: LGTM! New dependencies added.The following dependencies have been added:
- @headlessui/react for UI components
- @radix-ui/react-dialog for dialog components
- concurrently for running multiple commands
Also applies to: 36-36, 49-49
8-8
: Verify the dev script changes.The development script has been updated to use a shell script. Ensure that
dev.sh
exists and has proper execute permissions.Run the following script to verify the dev script setup:
✅ Verification successful
Dev script exists but doesn't require execute permissions
The
client/dev.sh
file exists and is properly referenced in package.json. Since the script is executed usingbash dev.sh
rather than direct execution, it doesn't require execute permissions to work correctly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dev script existence and permissions echo "Checking dev script..." if [ -f "dev.sh" ]; then ls -l dev.sh else echo "dev.sh not found" fiLength of output: 117
Script:
#!/bin/bash # Description: Verify dev script in client directory echo "Checking dev script in client directory..." if [ -f "client/dev.sh" ]; then ls -l client/dev.sh else echo "dev.sh not found in client directory" fiLength of output: 205
17-25
: Review package version changes for compatibility.Several package versions have been modified:
- @cartridge/connector downgraded to ^0.5.5
- @cartridge/controller upgraded to ^0.5.8
- @dojoengine/* packages upgraded to 1.0.8
Run the following script to check for potential version conflicts:
✅ Verification successful
Package versions are compatible and well-structured
The version changes are safe because:
- All @dojoengine packages (except recs) are consistently set to 1.0.8
- @dojoengine/recs at ^2.0.13 is an intended different major version
- @Cartridge packages use compatible version ranges
- Required peer dependencies (e.g., starknet) are satisfied
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential version conflicts and security advisories # Check npm for version compatibility echo "Checking version compatibility..." npm info "@cartridge/connector@^0.5.5" peerDependencies npm info "@cartridge/controller@^0.5.8" peerDependencies npm info "@dojoengine/[email protected]" peerDependencies # Check for known vulnerabilities echo "Checking for vulnerabilities..." npm auditLength of output: 703
Script:
#!/bin/bash # Find all package.json files and their content echo "=== Package.json files ===" fd package.json --type f --exec cat {} # Search for import statements of these packages echo -e "\n=== Package imports ===" rg -A 1 "from ['\"]@(cartridge|dojoengine)/" --type ts --type tsx --type js --type jsx # Look for version-specific patterns or constraints echo -e "\n=== Version constraints ===" rg "(@cartridge|@dojoengine)" --type jsonLength of output: 6713
contracts/manifest_sepolia.json (3)
1308-1314
: Review state mutability changes in contract functions.Several functions have had their state mutability changed from "view" to "external". This change affects how the functions can modify contract state.
Ensure that:
- All state-modifying functions are properly marked as "external"
- Pure view functions remain as "view"
- Access control is properly implemented for external functions
Also applies to: 1365-1505
1958-1968
: LGTM! New trophy system events added.Trophy-related events have been properly configured:
- TrophyCreation
- TrophyProgression
These align with the achievement system implementation.
4-5
: Verify contract addresses and seed configuration.The world configuration has been updated with new addresses and seed values. Ensure these values are correct for the respective environments.
Run the following script to verify contract addresses:
✅ Verification successful
Contract addresses and seed configurations verified successfully
The contract addresses and seed values are correctly configured and consistent across all environment files (manifest and dojo configuration files):
- Sepolia environment with seed "mancala-achievement"
- Dev environment with seed "mancala"
- Slot environment with seed "mancala"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify contract addresses across environments echo "Checking contract addresses..." rg -A 1 '"address":\s*"0x[a-fA-F0-9]+"' --type jsonLength of output: 1829
Script:
#!/bin/bash # Search for any references to these addresses in other files # and check for any documentation about contract configurations echo "Checking for address references and documentation..." rg -l "0x2570a4240bf789d09bdbc2cb8f870ad7879b0737d453ee1d70f5dfaedd9f3f6|0x7e6333cfb9768d94ffa94daf40d8261b27a245a154e180e8ed052781a2e1779|0x4068272626b01237de6009110492f508fb9de4d9358b6ecf15ffb02a1c7e0a3" echo -e "\nChecking for configuration documentation..." fd -t f "README|CONTRIBUTING|DEPLOY|CONFIG" -e md -e txtLength of output: 762
Script:
#!/bin/bash # Check contracts README and verify configuration consistency echo "Checking contracts README..." cat contracts/README.md echo -e "\nVerifying configuration consistency..." echo "Sepolia configs:" cat contracts/dojo_sepolia.toml echo -e "\nDev configs:" cat contracts/dojo_dev.toml echo -e "\nSlot configs:" cat contracts/dojo_slot.tomlLength of output: 6400
contracts/manifest_dev.json (5)
1255-1256
: Contract deployment changes detected.The contract address and class hash have been updated, indicating a new deployment. This is consistent with the transition to alpha stage.
Line range hint
1358-1506
: Well-structured game system interface.The IMancalaSystem interface provides a comprehensive set of functions for managing the Mancala game, including:
- Game lifecycle (new_game, join_game, create_private_game)
- Game mechanics (move, get_score, is_game_over)
- Player management (timeout, forfeited)
- Game state management (request_restart_game, restart_current_game)
All functions are correctly marked as external for proper access control.
Line range hint
1728-1750
: Profile system implementation looks good.The IPlayerProfile interface provides essential functionality for player profile management:
- Profile creation with name
- Profile updates with URI support
- View-only functions for data safety
The implementation follows best practices for player data management.
1899-1930
: Complete set of game models with proper alpha stage tagging.The models cover all essential aspects of the game:
- Game state (GameCounter, MancalaBoard)
- Player data (Player, Profile)
- Game mechanics (Pit, Seed)
All models are correctly tagged with
mancala_alpha
prefix.
1937-1968
: Comprehensive event system for game actions and achievements.The event system includes:
- Game mechanics events (Capture, EndTurn, PlayerMove)
- Special events (PlayerExtraTurn)
- Achievement events (TrophyCreation, TrophyProgression)
All events are properly tagged with
mancala_alpha
prefix and have unique selectors.
{players[index]?.name | ||
? players[index]?.name | ||
: truncateString(address)} |
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
Improve type safety in name rendering.
The optional chaining usage suggests potential type safety issues. Consider using proper TypeScript types and null coalescing.
- {players[index]?.name
- ? players[index]?.name
- : truncateString(address)}
+ {players[index]?.name ?? truncateString(address)}
📝 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.
{players[index]?.name | |
? players[index]?.name | |
: truncateString(address)} | |
{players[index]?.name ?? truncateString(address)} |
</td> | ||
<td> | ||
<p className="font-normal text-[#BDC2CC] text-center"> | ||
Lvl {wins < 4 ? 1 : Math.floor(wins / 4) + 1} |
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
Extract level calculation logic.
The level calculation logic should be moved to a utility function for better maintainability and reusability.
+// In utils.ts
+export const calculatePlayerLevel = (wins: number): number =>
+ wins < 4 ? 1 : Math.floor(wins / 4) + 1;
+// In component
- Lvl {wins < 4 ? 1 : Math.floor(wins / 4) + 1}
+ Lvl {calculatePlayerLevel(wins)}
Committable suggestion skipped: line range outside the PR's diff.
@@ -2,7 +2,7 @@ import mancala_duels_logo from "../../assets/mancala_duels.png"; | |||
|
|||
export default function EmptyGameHistory({ id }: { id: string }) { | |||
return ( | |||
<div className="w-[874px] h-[874px] bg-[url('./assets/lobby-box-long.png')] bg-contain bg-no-repeat p-8"> | |||
<div className="bg-[#0F1116] bg-contain bg-center bg-no-repeat border-2 border-[#272A32] rounded-2xl w-[full h-[437px] flex flex-col items-center justify-center"> |
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.
Fix CSS class syntax error and improve accessibility.
The className has syntax errors and accessibility can be improved:
w-[full
is missing a closing bracket- Missing ARIA attributes for better screen reader support
Apply this diff to fix the issues:
- <div className="bg-[#0F1116] bg-contain bg-center bg-no-repeat border-2 border-[#272A32] rounded-2xl w-[full h-[437px] flex flex-col items-center justify-center">
+ <div
+ className="bg-[#0F1116] bg-contain bg-center bg-no-repeat border-2 border-[#272A32] rounded-2xl w-full h-[437px] flex flex-col items-center justify-center"
+ role="region"
+ aria-label="Empty game history">
📝 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="bg-[#0F1116] bg-contain bg-center bg-no-repeat border-2 border-[#272A32] rounded-2xl w-[full h-[437px] flex flex-col items-center justify-center"> | |
<div | |
className="bg-[#0F1116] bg-contain bg-center bg-no-repeat border-2 border-[#272A32] rounded-2xl w-full h-[437px] flex flex-col items-center justify-center" | |
role="region" | |
aria-label="Empty game history"> |
> | ||
<UserIcon | ||
color="#F58229" | ||
className="w-5 h-5 text-white" | ||
/> | ||
</div> | ||
</div> | ||
<p className="font-normal text-white"> | ||
{item.challenger.name | ||
? item.challenger.name | ||
: truncateString( | ||
games[index].node.player_one, | ||
)} | ||
</p> | ||
</div> | ||
</td> | ||
<td className="flex flex-row px-4 h-16 space-x-5 w-[200px] text-center border-y-2 border-[#1A1E25] bg-[#111419]"> | ||
{item.challenged !== "0x0" ? ( | ||
<div className="flex flex-row items-center space-x-2.5 w-fit"> | ||
<div | ||
className="w-8 h-8 flex items-center justify-center rounded-full" | ||
style={{ backgroundColor: challengedColor }} | ||
> | ||
<UserIcon color="#F58229" className="w-5 h-5 text-white" /> | ||
</div> | ||
</td> | ||
<td className="flex flex-row items-center p-4 space-x-5 w-[200px] justify-center"> | ||
{games[index].node.player_two !== "0x0" ? ( | ||
<div className="flex flex-row items-center space-x-2.5 w-fit"> | ||
{/* <img src={item.challenged.profilePicture} width={35} height={35} alt={`${item.challenged.name} profile picture`} className="rounded-full" /> */} | ||
<div className="p-1 rounded-full bg-gradient-to-r bg-[#15181E] from-[#2E323A] via-[#4B505C] to-[#1D2026] relative"> | ||
<div | ||
className="bg-[#FFE600] w-8 h-8 flex items-center justify-center rounded-full" | ||
style={{ backgroundColor: challengedColor }} | ||
> | ||
<UserIcon | ||
color="#F58229" | ||
className="w-5 h-5 text-white" | ||
/> | ||
</div> | ||
</div> | ||
<p className="font-normal text-white"> | ||
{item.challenged.name | ||
? item.challenged.name | ||
: truncateString( | ||
games[index].node.player_two, | ||
)} | ||
</p> | ||
</div> | ||
) : ( | ||
<p className="text-white">Matchmaking</p> | ||
)} | ||
</td> | ||
<td className="w-[200px] text-center ml-8"> | ||
<p className="font-normal text-[#FAB580]"> | ||
{item.winner.name | ||
? item.winner.name | ||
: truncateString(games[index].node.winner)} | ||
<Typography variant="paragraph" className="font-medium leading-none text-white"> | ||
{truncateString(item.challenged)} | ||
</Typography> | ||
</div> | ||
) : ( | ||
<div className="flex flex-row items-center justify-center w-full h-16"> | ||
<p className="text-[#646976] font-semibold text-center w-full"> | ||
Matchmaking... | ||
</p> | ||
</td> | ||
<td className="w-[200px] text-end pr-12"> | ||
<p className="font-normal text-[#F97E22]"> | ||
{date.toLocaleDateString()} | ||
</p> | ||
</td> | ||
</tr> | ||
); | ||
})} | ||
</tbody> | ||
</table> | ||
</div> | ||
</div> | ||
</Card> | ||
</div> | ||
)} | ||
</td> | ||
<td className="w-[125px] px-4 text-center h-16 flex flex-col justify-center bg-[#111419] border-y-2 border-[#1A1E25]"> | ||
<p className="font-normal text-[#FAB580]"> | ||
{truncateString(item.winner)} | ||
</p> | ||
</td> | ||
<td className="w-[200px] px-4 text-end h-16 flex flex-col items-start justify-center bg-[#111419] border-y-2 border-r-2 border-[#1A1E25] rounded-r-xl"> | ||
<p className="font-normal text-[#F97E22] ml-2.5"> | ||
{date.toLocaleDateString().replace(/\//g, "-")} | ||
</p> | ||
</td> | ||
</tr> | ||
); | ||
})} | ||
</tbody> | ||
</table> | ||
</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
Optimize performance and improve type safety.
The table body implementation has several areas for improvement:
- Fixed positioning with absolute dimensions could cause layout issues
- Missing TypeScript interfaces for data structure
- Potential performance issues with large datasets
Consider creating an interface for the game data:
interface GameHistoryItem {
challenger: string;
challenged: string;
winner: string;
date: string;
}
And optimize the table body:
- <tbody className="h-[450px] w-[800px] rounded-b-xl fixed overflow-y-scroll overflow-x-clip space-y-2 bg-[#0F1116]">
+ <tbody className="max-h-[450px] w-full rounded-b-xl overflow-y-auto space-y-2 bg-[#0F1116]">
Also, consider implementing virtualization for better performance with large datasets using a library like react-window
or react-virtualized
.
return ( | ||
<div className="w-full"> | ||
<Card className="w-full h-full bg-transparent"> | ||
<div className="w-full text-left bg-transparent table-auto"> | ||
<div className="flex flex-row items-center justify-between w-full bg-[#0F1116] pt-2 rounded-t-xl px-2.5"> | ||
<div className="flex flex-row items-center justify-between w-full"> | ||
{duels_header.map((head) => ( | ||
<div | ||
key={head.id} | ||
className={clsx( | ||
head.id === 2 | ||
? "text-center ml-20" | ||
: head.id === 3 | ||
? "text-center -ml-6" | ||
: head.id === 4 && "-ml-16", | ||
head.id === 5 && "hidden", | ||
"w-[175px] p-4 text-start" | ||
)} | ||
> | ||
<Typography | ||
variant="small" | ||
className="font-semibold leading-none text-[#BDC2CC]" | ||
> | ||
{head.name === "Winner" ? "Result" : head.name} | ||
</Typography> | ||
</div> | ||
</div> | ||
<div className="absolute h-[275px] w-[814px] overflow-y-scroll scrollbar-hidden"> | ||
<table className="w-full text-left table-auto"> | ||
<thead className="border-b border-[#313640] hidden"> | ||
<tr className="w-full bg-[#0F1116] flex flex-row items-center justify-between"> | ||
{duels_header.map((head) => ( | ||
<th key={head.id} className="p-4"> | ||
<Typography | ||
variant="small" | ||
className="font-medium leading-none text-[#BDC2CC]" | ||
))} | ||
</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
Improve table header accessibility and responsiveness.
The table header structure needs accessibility improvements and better responsive design handling:
- Missing proper table semantics
- Hard-coded pixel values could break on different screen sizes
- Missing ARIA attributes
- <div className="w-full">
- <Card className="w-full h-full bg-transparent">
- <div className="w-full text-left bg-transparent table-auto">
+ <div className="w-full" role="region" aria-label="Game history">
+ <Card className="w-full h-full bg-transparent">
+ <div className="w-full text-left bg-transparent">
+ <table className="w-full table-auto">
+ <thead>
Committable suggestion skipped: line range outside the PR's diff.
@@ -0,0 +1,20 @@ | |||
rpc = "https://api.cartridge.gg/x/starknet/sepolia" | |||
|
|||
allowed_origins = ["*"] |
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.
Security: Restrict CORS allowed origins
The configuration allows all origins ("*"
) for CORS, which is overly permissive. Additionally, there's a duplicate CORS setting in the [indexing]
section.
Consider:
- Restricting allowed origins to specific domains
- Removing the duplicate CORS setting from the
[indexing]
section
-allowed_origins = ["*"]
+allowed_origins = ["https://your-domain.com", "https://staging.your-domain.com"]
[indexing]
-allowed_origins = ["*"]
Also applies to: 9-9
ipfs_config.url = "https://ipfs.infura.io:5001" | ||
ipfs_config.username = "2EBrzr7ZASQZKH32sl2xWauXPSA" | ||
ipfs_config.password = "12290b883db9138a8ae3363b6739d220" |
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.
Security Issue: IPFS credentials exposed in configuration file.
The IPFS configuration contains sensitive credentials that should not be committed to the repository.
Recommendations:
- Remove the credentials from the configuration file
- Use environment variables or a secure secrets management system
- Update the
.gitignore
file to prevent accidental commits of sensitive data
Apply this diff to use environment variables instead:
-ipfs_config.username = "2EBrzr7ZASQZKH32sl2xWauXPSA"
-ipfs_config.password = "12290b883db9138a8ae3363b6739d220"
+ipfs_config.username = "${IPFS_USERNAME}"
+ipfs_config.password = "${IPFS_PASSWORD}"
📝 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.
ipfs_config.url = "https://ipfs.infura.io:5001" | |
ipfs_config.username = "2EBrzr7ZASQZKH32sl2xWauXPSA" | |
ipfs_config.password = "12290b883db9138a8ae3363b6739d220" | |
ipfs_config.url = "https://ipfs.infura.io:5001" | |
ipfs_config.username = "${IPFS_USERNAME}" | |
ipfs_config.password = "${IPFS_PASSWORD}" |
@@ -28,6 +28,7 @@ export default function MessageArea({ | |||
const { system } = useDojo(); | |||
const [_, setRestarted] = useState(false); | |||
const { gameId } = useParams(); | |||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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
Avoid disabling ESLint rules without justification.
The ESLint rule for exhaustive dependencies is being disabled without explanation. This rule helps prevent bugs related to stale closures in React hooks.
Instead of disabling the rule:
- Add all required dependencies to the useEffect dependency array
- If certain dependencies need to be omitted, add a comment explaining why
- Consider using useCallback for the restart_game function if it needs to be stable across renders
@@ -3,7 +3,7 @@ | |||
"class_hash": "0x45575a88cc5cef1e444c77ce60b7b4c9e73a01cbbe20926d5a4c72a94011410", | |||
"address": "0x7e6333cfb9768d94ffa94daf40d8261b27a245a154e180e8ed052781a2e1779", | |||
"seed": "mancala", | |||
"name": "Mancala", | |||
"name": "RealmOfRa: Mancala", |
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.
💡 Codebase verification
Update project name in all manifest files
The project name has been updated to "RealmOfRa: Mancala" in manifest_dev.json
, but it's still set as "Mancala" in:
manifest_sepolia.json
manifest_slot.json
Please synchronize the name across all manifest files to maintain consistency between environments.
🔗 Analysis chain
Verify name consistency across the codebase.
The project has been rebranded from "Mancala" to "RealmOfRa: Mancala". This change might require updates in related documentation, UI components, and other references.
Run this script to check for inconsistencies:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent naming across the codebase
# Search for old name references
echo "Searching for old 'Mancala' references that might need updating..."
rg --type md --type txt --type yaml --type json "\"name\":\s*\"Mancala\"" || true
# Search for inconsistent new name references
echo "Checking for inconsistent 'RealmOfRa: Mancala' references..."
rg --type md --type txt --type yaml --type json "\"name\":\s*\"RealmOfRa:\s*Mancala\"" || true
Length of output: 611
{ | ||
"type": "function", | ||
"name": "dojo_init", | ||
"inputs": [], | ||
"outputs": [], | ||
"state_mutability": "view" | ||
}, |
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.
💡 Codebase verification
State mutability mismatch in dojo_init function
The dojo_init
implementation in contracts/src/systems/mancala.cairo
performs state modifications (initializes world storage and emits events), but it's declared as a view
function in the manifest files. This mismatch could lead to runtime issues.
- Change
state_mutability
from"view"
to an appropriate value in:- contracts/manifest_dev.json
- contracts/manifest_sepolia.json
- contracts/manifest_slot.json
🔗 Analysis chain
Verify dojo_init implementation.
The dojo_init
function is marked as view-only, which is appropriate for initialization. However, ensure that any state modifications required during initialization are handled correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for state modifications in dojo_init implementations
# Search for state modifications in dojo_init
echo "Checking dojo_init implementations for state modifications..."
ast-grep --pattern 'fn dojo_init($$$) { $$$ }'
Length of output: 181
Script:
#!/bin/bash
# Search for dojo_init across all files with context
echo "Searching for dojo_init definitions and usage..."
rg "dojo_init" -A 5 -B 5
# Also check for any files containing dojo_init in their name
echo -e "\nChecking for files containing dojo_init..."
fd "dojo.*init"
Length of output: 4377
* more ui updates * matchmaking ui * hover pot * create game colors * Do not show capture and extra turns if game is finished * audio ui * Remove leaderboard button from gameplay * fix score alignment * swapped header position * thumb border removed * Reduced spinner speed * added survey button below * updated ui * fixed leaderboard * removed burner wallet
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: 10
♻️ Duplicate comments (2)
client/src/components/profile/user-section.tsx (1)
150-154
:⚠️ Potential issuePass imageUrl to create_player_profile.
The imageUrl parameter is missing in the create_player_profile call, which will result in the user's profile image not being saved for new users.
Apply this fix:
await system.create_player_profile( account.account, displayName.toString(), + imageUrl, setLoading );
client/src/components/gameplay/game-board.tsx (1)
43-60
:⚠️ Potential issueAdd cleanup for polling operations
The polling operations need cleanup when component unmounts.
useEffect(() => { startCapturePolling(1000); startExtraTurnPolling(1000); + return () => { + startCapturePolling(0); + startExtraTurnPolling(0); + }; }, [startCapturePolling, startExtraTurnPolling]);
🧹 Nitpick comments (28)
client/src/components/ui/svgs/create-loader.tsx (1)
4-4
: LGTM! Consider adding a prop for animation speed control.The transition to a slower animation looks good. For better reusability, consider making the animation speed configurable through props.
-export default function CreateLoaderSVG() { +interface CreateLoaderSVGProps { + speed?: 'slow' | 'normal' | 'fast'; +} + +export default function CreateLoaderSVG({ speed = 'slow' }: CreateLoaderSVGProps) { return ( <svg - className="text-white animate-spin-slow w-fit" + className={`text-white ${ + speed === 'slow' ? 'animate-spin-slow' : + speed === 'fast' ? 'animate-spin-fast' : + 'animate-spin' + } w-fit`}client/src/index.css (1)
137-144
: LGTM! Consider documenting animation usage.The keyframes animation is well-defined with appropriate transform properties.
Add a comment block above the keyframes to document its purpose and usage:
+/** + * Defines a slow rotation animation primarily used for loading indicators. + * Duration: 2.5s + * Timing: Linear + * Usage: Apply via .animate-spin-slow class + */ @keyframes spin-slow {client/src/dojo/DojoContext.tsx (1)
Line range hint
27-31
: Consider memoizing the Account instantiation dependencies.While the
masterAccount
creation is correctly memoized, consider extracting the provider configuration into a separate memoized value to prevent unnecessary Account re-instantiation.+ const providerConfig = useMemo( + () => ({ + provider: dojoProvider.provider, + address: masterAddress, + privateKey: masterPrivateKey, + }), + [dojoProvider.provider, masterAddress, masterPrivateKey] + ); const masterAccount = useMemo( () => - new Account(dojoProvider.provider, masterAddress, masterPrivateKey, "1"), - [masterAddress, masterPrivateKey, dojoProvider.provider], + new Account(providerConfig.provider, providerConfig.address, providerConfig.privateKey, "1"), + [providerConfig], );client/src/pages/Lobby.tsx (4)
173-190
: Remove commented-out code.The audio-related code is commented out and appears to be unused. If this functionality is no longer needed, it should be removed to maintain code cleanliness.
- // const [isPlaying, setPlaying] = useAtom(isPlayingAtom); - - // const audioRef = useRef(new Audio(audio)); - // useEffect(() => { - // if (isPlaying) { - // try { - // audioRef.current.play(); - // audioRef.current.loop = true; - // } catch (error) { - // console.error("Error playing the audio", error); - // } - // } else { - // audioRef.current.pause(); - // } - // return () => { - // audioRef.current.pause(); - // }; - // }, [isPlaying]);
104-128
: Extract sorting logic into a separate function.The game sorting logic is complex and would benefit from being extracted into a separate function for better maintainability and reusability.
+ const sortGames = (a: any, b: any) => { + // First priority: Games without player two (excluding 0x0) + const aHasNoPlayerTwo = !a.node.player_two || a.node.player_two === ""; + const bHasNoPlayerTwo = !b.node.player_two || b.node.player_two === ""; + if (aHasNoPlayerTwo !== bHasNoPlayerTwo) { + return aHasNoPlayerTwo ? -1 : 1; + } + + // Second priority: Games with player_two = 0x0 + const aHasZeroPlayerTwo = a.node.player_two === "0x0"; + const bHasZeroPlayerTwo = b.node.player_two === "0x0"; + if (aHasZeroPlayerTwo !== bHasZeroPlayerTwo) { + return aHasZeroPlayerTwo ? -1 : 1; + } + + // Third priority: Games with both players + return 0; + }; const filteredGames = data?.mancalaAlphaMancalaBoardModels?.edges ?.filter( (game: any) => game?.node?.player_one === account.account?.address || game?.node?.player_two === account.account?.address || game?.node?.player_two === "0x0", ) - .sort((a: any, b: any) => { - // First priority: Games without player two (excluding 0x0) - const aHasNoPlayerTwo = !a.node.player_two || a.node.player_two === ""; - const bHasNoPlayerTwo = !b.node.player_two || b.node.player_two === ""; - if (aHasNoPlayerTwo !== bHasNoPlayerTwo) { - return aHasNoPlayerTwo ? -1 : 1; - } - - // Second priority: Games with player_two = 0x0 - const aHasZeroPlayerTwo = a.node.player_two === "0x0"; - const bHasZeroPlayerTwo = b.node.player_two === "0x0"; - if (aHasZeroPlayerTwo !== bHasZeroPlayerTwo) { - return aHasZeroPlayerTwo ? -1 : 1; - } - - // Third priority: Games with both players - return 0; - }) + .sort(sortGames)
415-423
: Remove unnecessary Fragment.The Fragment wrapper is redundant as it contains only one child element.
- {playWith === "Human" && ( - <> - <input - className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium" - placeholder="0x..." - onChange={(e) => setPlayer2(e.target.value)} - /> - </> - )} + {playWith === "Human" && ( + <input + className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium" + placeholder="0x..." + onChange={(e) => setPlayer2(e.target.value)} + /> + )}🧰 Tools
🪛 Biome (1.9.4)
[error] 416-422: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
516-518
: Add accessibility attributes to the feedback button.The feedback button should have proper accessibility attributes to improve user experience for screen readers.
<Link to="/" className="absolute bottom-10 left-1/2 transform -translate-x-1/2"> - <Button className="bg-[#0F1116] hover:bg-[#0F1116] text-[#C7CAD4] font-medium hover:cursor-pointer rounded-xl">Give feedbacks and get a chance to win Lord of the Mancala</Button> + <Button + className="bg-[#0F1116] hover:bg-[#0F1116] text-[#C7CAD4] font-medium hover:cursor-pointer rounded-xl" + aria-label="Submit feedback for a chance to win Lord of the Mancala" + > + Give feedbacks and get a chance to win Lord of the Mancala + </Button> </Link>client/src/components/header.tsx (2)
93-95
: Improve audio management implementation.The current audio implementation has several issues:
- Audio object initialization should be in useEffect
- Volume changes should be debounced
- Play/pause error handling could be improved
Consider implementing this improved version:
- const [audio] = useState(new Audio(audio_url)); + const [audio, setAudio] = useState<HTMLAudioElement | null>(null); + + useEffect(() => { + const audioInstance = new Audio(audio_url); + audioInstance.loop = true; + setAudio(audioInstance); + + return () => { + audioInstance.pause(); + audioInstance.src = ''; + }; + }, []); useEffect(() => { + if (!audio) return; audio.volume = volume / 100; const playAudio = async () => { try { if (volume > 0 && !isPlaying) { await audio.play(); setIsPlaying(true); } else if (volume === 0 && isPlaying) { audio.pause(); setIsPlaying(false); } } catch (error) { console.error('Audio playback error:', error); setIsPlaying(false); } }; playAudio(); }, [volume, audio, isPlaying]);Also applies to: 98-105
150-152
: Extract name truncation logic to a utility function.The inline name truncation logic reduces readability. Consider extracting it to a reusable function.
+ const truncateName = (name: string, maxLength: number = 16) => + name.length > maxLength ? `${name.slice(0, maxLength)}...` : name; - {playerName - ? (playerName.length > 16 ? playerName.slice(0, 16) + "..." : playerName) - : (username.length > 16 ? username.slice(0, 16) + "..." : username)} + {truncateName(playerName || username)}client/src/components/profile/user-section.tsx (3)
22-25
: Consider moving SaveStatus type to shared types file.The SaveStatus type could be reused across other components that need similar save operation states.
Consider moving it to a shared types file (e.g.,
types/common.ts
).
42-48
: Consider consolidating profile-related state.Multiple related state variables could be consolidated into a single profile state object for better maintainability.
Consider refactoring to:
- const [playerName, setPlayerName] = useState(""); - const [playerImage, setPlayerImage] = useState(""); - const [selectedImage, setSelectedImage] = useState<string | null>(null); - const [imageUrl, setImageUrl] = useState<string>(""); - const [displayName, setDisplayName] = useState(""); - const [initialDisplayName, setInitialDisplayName] = useState(""); - const [initialImageUrl, setInitialImageUrl] = useState(""); + const [profile, setProfile] = useState({ + playerName: "", + playerImage: "", + selectedImage: null as string | null, + imageUrl: "", + displayName: "", + initialDisplayName: "", + initialImageUrl: "" + });
248-250
: Move max level to constants.The maximum level (25) is hardcoded in multiple places. This should be defined as a constant for better maintainability.
Consider moving it to constants:
+ const MAX_LEVEL = 25; <p className="font-medium text-white"> - {level.toString().padStart(2, "0")}/25 + {level.toString().padStart(2, "0")}/{MAX_LEVEL} </p> // ... style={{ - width: `${(level / 25) * 100}%`, + width: `${(level / MAX_LEVEL) * 100}%`, }}Also applies to: 256-256
client/src/pages/Profile.tsx (1)
41-41
: Consider extracting background styles to a CSS classThe inline background styles could be moved to a CSS class for better maintainability.
- <div className="w-full h-screen bg-[#0F1116] bg-[url('./assets/bg.png')] bg-cover bg-center bg-no-repeat space-y-4 fixed"> + <div className="w-full h-screen bg-profile space-y-4 fixed">Add to your CSS:
.bg-profile { background-color: #0F1116; background-image: url('./assets/bg.png'); background-size: cover; background-position: center; background-repeat: no-repeat; }Also applies to: 60-69
client/src/components/gameplay/game-board.tsx (3)
62-100
: Consider debouncing toast notificationsThe current implementation might show too many toasts in quick succession. Consider debouncing or adding a cooldown period.
+ const [lastToastTime, setLastToastTime] = useState(0); + const TOAST_COOLDOWN = 3000; // 3 seconds useEffect(() => { const captures = captureData?.mancalaAlphaCaptureModels?.edges; if (captures && captures.length > 0 && game_node?.status !== "Finished") { const latestCapture = captures[captures.length - 1]?.node; - if (latestCapture) { + const now = Date.now(); + if (latestCapture && now - lastToastTime >= TOAST_COOLDOWN) { const isPlayerCapture = latestCapture.player === account.account?.address; toast({ title: isPlayerCapture ? "Seeds Captured!" : "Seeds Lost!", description: isPlayerCapture ? `You captured ${latestCapture.seed_count} seeds from pit ${latestCapture.pit_number}` : `Opponent captured ${latestCapture.seed_count} seeds from pit ${latestCapture.pit_number}`, duration: 3000, }); + setLastToastTime(now); } } }, [captureData, account.account?.address, toast, game_node?.status]);
180-206
: Optimize seed rendering performanceThe current implementation maps over all 48 seeds and filters null values. Consider pre-filtering the valid seeds.
- {Array.from({ length: 48 }, (_, i) => i + 1).map((seedNumber) => { - const seedDetails = getSeed(seedNumber); - if (!seedDetails) return null; + {seeds.map((seed) => { + const seedNumber = parseInt(seed.seed_id, 16); + const seedDetails = { + ...seed, + isNative: (seed.player === game_node?.player_one && seedNumber >= 1 && seedNumber <= 24) || + (seed.player === game_node?.player_two && seedNumber >= 25 && seedNumber <= 48) + };
141-169
: Extract complex filtering logic into utility functionsThe pit filtering logic is repeated and complex. Consider extracting it into a utility function.
const getPitsByPlayer = (pits: any[], players: any[], position: number) => { return pits .filter(item => item?.node.player === players[position]?.node.address) .filter(item => item?.node.pit_number === 7)[0]?.node?.seed_count || 0; };Then use it like:
- const player_pot_seed_count = - game_players?.mancalaAlphaPitModels.edges - .filter( - (item: any) => - item?.node.player === - game_players?.mancalaAlphaPlayerModels.edges[player_position]?.node - .address, - ) - .filter((item: any) => item?.node.pit_number === 7)[0]?.node - ?.seed_count || 0; + const player_pot_seed_count = getPitsByPlayer( + game_players?.mancalaAlphaPitModels.edges, + game_players?.mancalaAlphaPlayerModels.edges, + player_position + );client/src/AppWrapper.tsx (1)
26-33
: Consider simplifying the loading message logic.While functional, the current implementation could be more straightforward and type-safe.
Consider this alternative implementation:
const currentLoadingMessage = useMemo(() => { - const currentStep = Object.entries(setupStatus).find( - ([, value]) => value, - )?.[0]; - return currentStep - ? SETUP_STATUS_MESSAGES[currentStep as keyof typeof SETUP_STATUS_MESSAGES] - : "Loading..."; + const currentStep = Object.keys(setupStatus).find(key => setupStatus[key as keyof SetupStatus]); + return currentStep ? SETUP_STATUS_MESSAGES[currentStep as keyof typeof SETUP_STATUS_MESSAGES] : "Loading..."; }, [setupStatus]);client/src/components/gameplay/audio-section.tsx (4)
32-37
: Remove redundant 'ended' event listenerYou have set
audio.loop = true
at line 32 and also added an 'ended' event listener to replay the audio. This is redundant since settingaudio.loop = true
will automatically loop the audio.Consider removing the 'ended' event listener:
audio.loop = true; - audio.addEventListener('ended', handleEnded);
48-55
: AdjusttogglePlay
function to reflect actual play stateThe
togglePlay
function currently relies onvolume
to determine whether to play or pause the audio, which may not accurately represent the audio's actual play state. It's better to toggle based on the audio'spaused
property.Here's how you can adjust the function:
const togglePlay = () => { - if (volume > 0) { - audio.play().catch(console.error); - } else { - audio.pause(); - setVolume(0); - } + if (audio.paused) { + audio.play().catch(console.error); + } else { + audio.pause(); + } };
105-105
: Avoid hardcoding volume values when toggling muteAt line 105, when toggling mute, the volume is set to 0 or 35. Using hardcoded values can lead to an inconsistent user experience. Consider storing the previous volume level to restore it upon unmuting.
Modify the mute toggle logic:
+ const [previousVolume, setPreviousVolume] = useState(volume); const toggleMute = () => { if (volume > 0) { + setPreviousVolume(volume); setVolume(0); } else { - setVolume(35); + setVolume(previousVolume || 50); // Default to 50 if previousVolume is undefined } }; ... <img src={volume > 0 ? unmuteFlat : muteFlat} width={50} height={50} alt="toggle play" className="rounded-full cursor-pointer" - onClick={() => setVolume(volume > 0 ? 0 : 35)} + onClick={toggleMute}
123-124
: Specify the correct type inonValueChange
handlerIn the
Slider
component at line 123, theonValueChange
handler usesany
type. To enhance type safety, specify the correct type forvalue
.Update the handler:
- <Slider value={[volume]} onValueChange={([value]: any) => setVolume(value)} className="w-full" ... /> + <Slider value={[volume]} onValueChange={(value: number[]) => setVolume(value[0])} className="w-full" ... />client/src/lib/utils.ts (2)
11-13
: Set default value fornum
parameter intruncateString
Currently,
(num || 6)
is used multiple times in thetruncateString
function. Setting a default value fornum
simplifies the code and improves readability.Update the function signature:
- export function truncateString(str?: string, num?: number) { + export function truncateString(str?: string, num: number = 6) { if (str) { - return str.length > (num || 6) ? str.slice(0, num || 6) + "..." + str.slice(-(num || 6)) : str; + return str.length > num ? str.slice(0, num) + "..." + str.slice(-num) : str; } }
164-170
: Remove unnecessary console logs fromformatPlayerName
The
console.log
statements at lines 165-168 appear to be left over from debugging. They should be removed to clean up the console output and prevent potential exposure of sensitive information.Remove the
console.log
block:export const formatPlayerName = (name: string, address: string, num?: number) => { - console.log({ - name, - address, - }); if (!name || name === "0x0" || name === address) { return truncateString(address, num); }client/src/App.tsx (1)
63-70
: Fix indentation in Routes component.The Routes component has inconsistent indentation which affects readability.
- <Router> - <Routes> - <Route index element={<Home />} /> - <Route path="/lobby" element={<Lobby />} /> - <Route path="/profile" element={<Profile />} /> - <Route path="/games/:gameId" element={<Gameplay />} /> - </Routes> - </Router> + <Router> + <Routes> + <Route index element={<Home />} /> + <Route path="/lobby" element={<Lobby />} /> + <Route path="/profile" element={<Profile />} /> + <Route path="/games/:gameId" element={<Gameplay />} /> + </Routes> + </Router>client/src/pages/games/Gameplay.tsx (1)
39-43
: Simplify boolean expression.The involved check can be simplified by removing the unnecessary boolean literals.
- const involved = - game_players?.mancalaAlphaPlayerModels.edges.filter( - (item: any) => item?.node.address === account.address, - ).length > 0 - ? true - : false; + const involved = game_players?.mancalaAlphaPlayerModels.edges.filter( + (item: any) => item?.node.address === account.address, + ).length > 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 39-43: 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)
client/src/components/lobby/live-duels.tsx (2)
12-16
: Consider adding type safety for game data.The code uses
any
type for the games data structure. Consider defining proper interfaces for better type safety and code maintainability.+interface GameNode { + player_one: string; + player_two: string; + player_one_name: string; + player_two_name: string; + game_id: string; +} +interface Game { + node: GameNode; +} -const data = games?.filter((game: any) => +const data = games?.filter((game: Game) =>
158-162
: Consider using a date formatting library.Direct manipulation of dates can be error-prone. Consider using a date formatting library like
date-fns
ordayjs
for consistent date formatting across the application.+import { format } from 'date-fns'; -<p className="font-normal text-[#BDC2CC]"> - {date?.toLocaleDateString().replace(/\//g, "-")} -</p> +<p className="font-normal text-[#BDC2CC]"> + {date ? format(date, 'yyyy-MM-dd') : ''} +</p>client/src/components/lobby/duels.tsx (1)
231-286
: Simplify complex conditional rendering.The button text logic is overly complex and nested. Consider extracting it to a separate function for better readability and maintainability.
+const getButtonText = (game: any, index: number) => { + if (game.player_one === account.account?.address || + game.player_two === account.account?.address) { + return "Go to game"; + } + + if (joinStatus?.status === "JOINING" && joinStatus.index === index) { + return "Joining..."; + } + + if (joinStatus?.status === "ERROR" && joinStatus.index === index) { + return "Cannot join game"; + } + + if (game.player_one !== "0x0" && game.player_two !== "0x0") { + return "Spectate game"; + } + + return "Join game"; +}; -{(() => { - // ... existing complex logic -})()} +{getButtonText(games[index].node, index)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
client/pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
,!client/pnpm-lock.yaml
and included byclient/**
client/src/assets/sn.png
is excluded by!**/*.png
and included byclient/**
client/src/dojo/generated/setup.ts
is excluded by!**/generated/**
and included byclient/**
📒 Files selected for processing (26)
client/package.json
(2 hunks)client/src/App.tsx
(2 hunks)client/src/AppWrapper.tsx
(2 hunks)client/src/atom/atoms.ts
(0 hunks)client/src/components/gameplay/audio-section.tsx
(4 hunks)client/src/components/gameplay/game-board.tsx
(6 hunks)client/src/components/gameplay/game-navigation.tsx
(1 hunks)client/src/components/header.tsx
(2 hunks)client/src/components/lobby/duels.tsx
(3 hunks)client/src/components/lobby/leaderboard.tsx
(2 hunks)client/src/components/lobby/live-duels.tsx
(5 hunks)client/src/components/pits.tsx
(2 hunks)client/src/components/profile/user-section.tsx
(5 hunks)client/src/components/seed.tsx
(2 hunks)client/src/components/ui/dropdown-menu.tsx
(1 hunks)client/src/components/ui/slider.tsx
(1 hunks)client/src/components/ui/svgs/create-loader.tsx
(1 hunks)client/src/components/ui/svgs/logout.tsx
(1 hunks)client/src/dojo/DojoContext.tsx
(2 hunks)client/src/dojo/useDojo.tsx
(0 hunks)client/src/hooks/useAudioControl.ts
(0 hunks)client/src/index.css
(1 hunks)client/src/lib/utils.ts
(3 hunks)client/src/pages/Lobby.tsx
(20 hunks)client/src/pages/Profile.tsx
(4 hunks)client/src/pages/games/Gameplay.tsx
(3 hunks)
💤 Files with no reviewable changes (3)
- client/src/hooks/useAudioControl.ts
- client/src/dojo/useDojo.tsx
- client/src/atom/atoms.ts
✅ Files skipped from review due to trivial changes (1)
- client/src/components/ui/svgs/logout.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- client/src/components/gameplay/game-navigation.tsx
- client/src/components/lobby/leaderboard.tsx
- client/src/components/pits.tsx
- client/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/pages/Lobby.tsx
[error] 416-422: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
client/src/components/profile/user-section.tsx
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
client/src/pages/games/Gameplay.tsx
[error] 39-43: 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)
🪛 Gitleaks (8.21.2)
client/src/components/header.tsx
90-90: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (15)
client/src/index.css (1)
146-148
: LGTM! Animation properties are well configured.The animation class is properly defined with appropriate duration, timing function, and iteration count.
client/src/dojo/DojoContext.tsx (2)
24-24
: LGTM! Clean integration of the client property.The addition of the
client
property is well-implemented:
- Properly destructured from the input value
- Correctly passed through the context provider
Also applies to: 38-38
Line range hint
1-40
: Verify the impact of context structure changes on consumers.The context structure has been significantly simplified by removing the
account
property and its associated destructured properties, replacing them with a directclient
property. While this change improves maintainability, it's crucial to ensure all context consumers are updated accordingly.Let's verify the context usage across the codebase:
✅ Verification successful
Context changes are safe to proceed
The verification shows that all context access is properly centralized through the
useDojo
hook, and there are no direct dependencies on the removed context properties. Theaccount
references in the codebase are from a different source and unrelated to the context changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components using the removed account properties rg "useContext\(DojoContext\)" -A 5 "client/src" # Search specifically for any remaining references to the old account properties rg "account\." "client/src"Length of output: 6388
client/src/pages/Lobby.tsx (1)
62-69
: Add error handling for game creation.The game creation logic should include error handling to provide feedback to users when creation fails.
Also applies to: 80-85
client/src/components/header.tsx (2)
28-29
: Removeas any
type assertions for better type safety.Using
as any
bypasses TypeScript's type checking mechanisms. Define proper types for better type safety.
51-52
: Move polling initialization into useEffect.Direct polling calls can lead to multiple intervals being created. Wrap these calls in useEffect hooks.
Also applies to: 53-55
client/src/pages/Profile.tsx (2)
8-9
: LGTM: Connection management enhancementAdded useConnect hook and useEffect for automatic wallet connection, improving user experience.
18-18
: Verify data model transition completenessThe transition from dev to alpha models needs to be verified across the codebase to ensure consistency.
Also applies to: 36-36
✅ Verification successful
Model transition is complete, regenerate contract components
The transition to alpha models is complete across the codebase. Only the auto-generated contract components file still contains legacy dev references. These will be updated when you regenerate the contract components.
client/src/dojo/generated/contractComponents.ts
: Regenerate this file to update the remaining dev references🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining dev model references echo "Checking for remaining dev model references..." rg -i "mancaladev" --type ts --type tsx echo "Verifying alpha model references..." rg -i "mancalaalpha" --type ts --type tsxLength of output: 321
Script:
#!/bin/bash # Search for any remaining dev model references without type restrictions echo "Checking for dev model references..." rg -i "mancaladev" rg -i "mancala[_\s-]?dev" echo "Checking alpha model references..." rg -i "mancalaalpha" rg -i "mancala[_\s-]?alpha" # Use ast-grep to find type references that might be missed by simple text search echo "Checking type references..." ast-grep --pattern 'type $_ = { mancala$_: $_; }'Length of output: 14689
client/src/AppWrapper.tsx (3)
12-18
: Well-structured setup status messages!The setup status messages are clear, descriptive, and provide good user feedback for each stage of the initialization process.
21-24
: Well-typed state management!The state management is well-structured with appropriate types and separation of concerns between setup result, loading state, and error handling.
36-42
: 🛠️ Refactor suggestionImprove type safety and error handling in setup process.
The current implementation has two concerns:
- The type assertion to
never
suggests a potential type mismatch that should be addressed properly- The error handling could be more specific and informative to users
Consider this improvement:
- setup(dojoConfig, setSetupStatus) - .then((res) => setSetupResult(res as never)) + setup(dojoConfig, setSetupStatus) + .then((res) => { + if (!res) throw new Error('Setup failed: No result returned'); + setSetupResult(res); + }) .catch((errorConnectingDojo) => { - console.error("Connection error", errorConnectingDojo); - setError(new Error(errorConnectingDojo)); + const errorMessage = errorConnectingDojo instanceof Error + ? errorConnectingDojo.message + : 'Failed to connect to game network'; + console.error("Connection error:", errorMessage); + setError(new Error(errorMessage)); })Let's verify the setup function's return type:
client/src/components/ui/dropdown-menu.tsx (1)
1-198
: Well-structured implementation of dropdown menu components!The implementation follows React best practices:
- Uses Radix UI primitives ensuring accessibility
- Properly implements ref forwarding
- Maintains consistent styling patterns
- Has a clear component hierarchy
client/src/components/seed.tsx (1)
43-56
: Consider cleanup for audio timeouts.The audio cleanup in useEffect could be improved to prevent memory leaks.
client/src/components/lobby/live-duels.tsx (2)
76-77
: Consider improving table responsiveness.The table implementation uses fixed dimensions and positioning which might cause issues on different screen sizes:
- Fixed height of 450px
- Fixed width of 928px
- Fixed positioning
92-92
:⚠️ Potential issueRemove fixed positioning from tbody.
Using
fixed
positioning on thetbody
element can cause layout issues, especially with scrolling and overlapping content.-<tbody className="h-[450px] w-[928px] rounded-b-xl fixed overflow-y-scroll overflow-x-clip space-y-2 bg-[#0F1116]"> +<tbody className="h-[450px] w-full rounded-b-xl overflow-y-scroll overflow-x-clip space-y-2 bg-[#0F1116]">Likely invalid or redundant comment.
const { data: balanceData, error } = useBalance({ | ||
address: account?.address, | ||
token: "0x04718f5a0fc34cc1af16a1cdee98ffb20c31f5cd61d6ab07201858f4287c938d", | ||
}); |
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
Move token address to environment configuration.
The hardcoded token address should be moved to a configuration file or environment variable for better security and maintainability.
- token: "0x04718f5a0fc34cc1af16a1cdee98ffb20c31f5cd61d6ab07201858f4287c938d",
+ token: process.env.REACT_APP_STRK_TOKEN_ADDRESS,
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Gitleaks (8.21.2)
90-90: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
const { data: playerData, startPolling: startPollingPlayerData } = useQuery(MancalaPlayerNames); | ||
startPollingPlayerData(1000); |
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 cleanup for polling and consider making interval configurable.
The polling interval is hardcoded and there's no cleanup when the component unmounts.
Consider:
- Adding cleanup in useEffect
- Making the polling interval configurable
+ const POLL_INTERVAL = 1000; // Consider moving to config
- startPollingPlayerData(1000);
+ useEffect(() => {
+ startPollingPlayerData(POLL_INTERVAL);
+ return () => {
+ // Stop polling when component unmounts
+ stopPollingPlayerData();
+ };
+ }, [startPollingPlayerData, stopPollingPlayerData]);
Committable suggestion skipped: line range outside the PR's diff.
<input | ||
type="file" | ||
className="hidden" | ||
ref={fileInputRef} | ||
onChange={handleImageChange} | ||
accept="image/*" | ||
/> |
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.
Add file validation for image uploads.
The file input accepts any image type without size validation, which could lead to performance issues or security vulnerabilities.
Consider adding:
- File size limits
- Specific image format restrictions
- Basic image validation
<input
type="file"
className="hidden"
ref={fileInputRef}
onChange={handleImageChange}
- accept="image/*"
+ accept=".jpg,.jpeg,.png,.webp"
/>
And in handleImageChange:
if (event.target.files && event.target.files[0]) {
const file = event.target.files?.[0];
+ const MAX_SIZE = 5 * 1024 * 1024; // 5MB
+
+ if (file.size > MAX_SIZE) {
+ setSaveStatus({
+ status: 'error',
+ message: 'File size must be less than 5MB'
+ });
+ return;
+ }
setImageLoading(true);
// ...
}
Committable suggestion skipped: line range outside the PR's diff.
<DojoProvider value={setupResult as never}> | ||
<App /> | ||
<Toaster /> | ||
</DojoProvider> | ||
</ApolloProvider> |
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
Improve error handling and type safety in the provider setup.
The error handling could be more graceful, and the type assertion needs attention.
Consider this improvement:
- if (error) throw error;
+ if (error) {
+ return (
+ <div role="alert" className="grid w-full h-screen place-items-center bg-[#0F1116]">
+ <div className="p-4 rounded-lg bg-red-50 dark:bg-red-900">
+ <h2 className="text-red-800 dark:text-red-100">Failed to initialize application</h2>
+ <p className="text-red-700 dark:text-red-200">{error.message}</p>
+ </div>
+ </div>
+ );
+ }
return (
<ApolloProvider client={apollo_client}>
- <DojoProvider value={setupResult as never}>
+ <DojoProvider value={setupResult}>
<App />
<Toaster />
</DojoProvider>
</ApolloProvider>
);
Committable suggestion skipped: line range outside the PR's diff.
<div className="grid w-full h-screen place-items-center bg-[#0F1116] bg-[url('./assets/bg.png')] bg-cover bg-center"> | ||
<div className="flex flex-col items-center justify-center space-y-4"> | ||
<div className="flex flex-col items-center justify-center space-y-1.5 w-[400px] h-[275px] rounded-lg bg-[#4920003D] backdrop-blur-md"> | ||
<img src={logo} width={126} height={126} alt="logo" className="animate-spin-slow" /> | ||
<p className="text-white font-semibold">{currentLoadingMessage}</p> | ||
</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
Enhance loading UI accessibility and robustness.
While the loading UI looks good, it needs accessibility improvements and more robust image handling.
Consider these improvements:
- <div className="grid w-full h-screen place-items-center bg-[#0F1116] bg-[url('./assets/bg.png')] bg-cover bg-center">
+ <div
+ role="progressbar"
+ aria-busy="true"
+ aria-label="Loading application"
+ className="grid w-full h-screen place-items-center bg-[#0F1116] bg-[url('./assets/bg.png')] bg-cover bg-center"
+ >
<div className="flex flex-col items-center justify-center space-y-4">
- <div className="flex flex-col items-center justify-center space-y-1.5 w-[400px] h-[275px] rounded-lg bg-[#4920003D] backdrop-blur-md">
+ <div className="flex flex-col items-center justify-center space-y-1.5 w-[400px] h-[275px] rounded-lg bg-[rgba(73,32,0,0.24)] backdrop-blur-md">
- <img src={logo} width={126} height={126} alt="logo" className="animate-spin-slow" />
+ <img
+ src={logo}
+ width={126}
+ height={126}
+ alt="Mancala game logo"
+ className="animate-spin-slow"
+ onError={(e) => {
+ e.currentTarget.style.display = 'none';
+ }}
+ />
- <p className="text-white font-semibold">{currentLoadingMessage}</p>
+ <p className="text-white font-semibold" aria-live="polite">{currentLoadingMessage}</p>
</div>
</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="grid w-full h-screen place-items-center bg-[#0F1116] bg-[url('./assets/bg.png')] bg-cover bg-center"> | |
<div className="flex flex-col items-center justify-center space-y-4"> | |
<div className="flex flex-col items-center justify-center space-y-1.5 w-[400px] h-[275px] rounded-lg bg-[#4920003D] backdrop-blur-md"> | |
<img src={logo} width={126} height={126} alt="logo" className="animate-spin-slow" /> | |
<p className="text-white font-semibold">{currentLoadingMessage}</p> | |
</div> | |
<div | |
role="progressbar" | |
aria-busy="true" | |
aria-label="Loading application" | |
className="grid w-full h-screen place-items-center bg-[#0F1116] bg-[url('./assets/bg.png')] bg-cover bg-center" | |
> | |
<div className="flex flex-col items-center justify-center space-y-4"> | |
<div className="flex flex-col items-center justify-center space-y-1.5 w-[400px] h-[275px] rounded-lg bg-[rgba(73,32,0,0.24)] backdrop-blur-md"> | |
<img | |
src={logo} | |
width={126} | |
height={126} | |
alt="Mancala game logo" | |
className="animate-spin-slow" | |
onError={(e) => { | |
e.currentTarget.style.display = 'none'; | |
}} | |
/> | |
<p className="text-white font-semibold" aria-live="polite">{currentLoadingMessage}</p> | |
</div> | |
</div> | |
</div> |
useEffect(() => { | ||
audio.loop = true; | ||
|
||
audio.addEventListener('ended', () => { | ||
audio.play().catch(console.error); | ||
}); | ||
|
||
audio.addEventListener('error', (e) => { | ||
console.error('Audio playback error:', e); | ||
}); | ||
|
||
return () => { | ||
audio.pause(); | ||
audio.currentTime = 0; | ||
}; | ||
}, [audio]); |
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.
Add cleanup functions for event listeners to prevent memory leaks
In the useEffect
hook starting at line 31, event listeners are added to the audio
object, but they are not removed when the component unmounts. This could lead to memory leaks or unexpected behavior.
To fix this, define the event handlers and add cleanup functions:
useEffect(() => {
+ const handleEnded = () => {
+ audio.play().catch(console.error);
+ };
+ const handleError = (e: any) => {
+ console.error('Audio playback error:', e);
+ };
audio.loop = true;
- audio.addEventListener('ended', () => {
- audio.play().catch(console.error);
- });
- audio.addEventListener('error', (e) => {
- console.error('Audio playback error:', e);
- });
+ audio.addEventListener('ended', handleEnded);
+ audio.addEventListener('error', handleError);
return () => {
audio.pause();
audio.currentTime = 0;
+ audio.removeEventListener('ended', handleEnded);
+ audio.removeEventListener('error', handleError);
};
}, [audio]);
📝 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.
useEffect(() => { | |
audio.loop = true; | |
audio.addEventListener('ended', () => { | |
audio.play().catch(console.error); | |
}); | |
audio.addEventListener('error', (e) => { | |
console.error('Audio playback error:', e); | |
}); | |
return () => { | |
audio.pause(); | |
audio.currentTime = 0; | |
}; | |
}, [audio]); | |
useEffect(() => { | |
const handleEnded = () => { | |
audio.play().catch(console.error); | |
}; | |
const handleError = (e: any) => { | |
console.error('Audio playback error:', e); | |
}; | |
audio.loop = true; | |
audio.addEventListener('ended', handleEnded); | |
audio.addEventListener('error', handleError); | |
return () => { | |
audio.pause(); | |
audio.currentTime = 0; | |
audio.removeEventListener('ended', handleEnded); | |
audio.removeEventListener('error', handleError); | |
}; | |
}, [audio]); |
const Slider = React.forwardRef<any, any>(({ className, track, thumb, range, hideThumb, ...props }, ref) => ( | ||
<SliderPrimitive.Root | ||
ref={ref} | ||
className={cn( | ||
"relative flex w-full touch-none select-none items-center", | ||
className | ||
)} | ||
{...props} | ||
> | ||
<SliderPrimitive.Track className={clsx("relative h-2 w-full grow overflow-hidden rounded-full bg-[#C7CAD4]", track)}> | ||
<SliderPrimitive.Range className={clsx("absolute h-full bg-white", range)} /> | ||
</SliderPrimitive.Track> | ||
{!hideThumb && <SliderPrimitive.Thumb className={clsx("block h-5 w-5 rounded-full bg-background transition-colors focus-visible:outline-none border-0 disabled:pointer-events-none disabled:opacity-50", thumb)} />} | ||
</SliderPrimitive.Root> | ||
)) |
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.
Define explicit TypeScript types for the Slider
component
The Slider
component currently uses any
for its props and ref, which negates the benefits of TypeScript's type checking. Defining explicit types will improve code reliability and maintainability.
Modify the component to include proper types:
import * as React from "react";
import * as SliderPrimitive from "@radix-ui/react-slider";
import { cn } from "@/lib/utils";
import clsx from "clsx";
+ interface SliderProps extends SliderPrimitive.SliderProps {
+ className?: string;
+ track?: string;
+ thumb?: string;
+ range?: string;
+ hideThumb?: boolean;
+ }
- const Slider = React.forwardRef<any, any>(({ className, track, thumb, range, hideThumb, ...props }, ref) => (
+ const Slider = React.forwardRef<React.ElementRef<typeof SliderPrimitive.Root>, SliderProps>(
+ ({ className, track, thumb, range, hideThumb, ...props }, ref) => (
<SliderPrimitive.Root
ref={ref}
className={cn(
"relative flex w-full touch-none select-none items-center",
className
)}
{...props}
>
<SliderPrimitive.Track className={clsx("relative h-2 w-full grow overflow-hidden rounded-full bg-[#C7CAD4]", track)}>
<SliderPrimitive.Range className={clsx("absolute h-full bg-white", range)} />
</SliderPrimitive.Track>
{!hideThumb && (
<SliderPrimitive.Thumb
className={clsx(
"block h-5 w-5 rounded-full bg-background transition-colors focus-visible:outline-none border-0 disabled:pointer-events-none disabled:opacity-50",
thumb
)}
/>
)}
</SliderPrimitive.Root>
)
);
📝 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.
const Slider = React.forwardRef<any, any>(({ className, track, thumb, range, hideThumb, ...props }, ref) => ( | |
<SliderPrimitive.Root | |
ref={ref} | |
className={cn( | |
"relative flex w-full touch-none select-none items-center", | |
className | |
)} | |
{...props} | |
> | |
<SliderPrimitive.Track className={clsx("relative h-2 w-full grow overflow-hidden rounded-full bg-[#C7CAD4]", track)}> | |
<SliderPrimitive.Range className={clsx("absolute h-full bg-white", range)} /> | |
</SliderPrimitive.Track> | |
{!hideThumb && <SliderPrimitive.Thumb className={clsx("block h-5 w-5 rounded-full bg-background transition-colors focus-visible:outline-none border-0 disabled:pointer-events-none disabled:opacity-50", thumb)} />} | |
</SliderPrimitive.Root> | |
)) | |
interface SliderProps extends SliderPrimitive.SliderProps { | |
className?: string; | |
track?: string; | |
thumb?: string; | |
range?: string; | |
hideThumb?: boolean; | |
} | |
const Slider = React.forwardRef<React.ElementRef<typeof SliderPrimitive.Root>, SliderProps>( | |
({ className, track, thumb, range, hideThumb, ...props }, ref) => ( | |
<SliderPrimitive.Root | |
ref={ref} | |
className={cn( | |
"relative flex w-full touch-none select-none items-center", | |
className | |
)} | |
{...props} | |
> | |
<SliderPrimitive.Track className={clsx("relative h-2 w-full grow overflow-hidden rounded-full bg-[#C7CAD4]", track)}> | |
<SliderPrimitive.Range className={clsx("absolute h-full bg-white", range)} /> | |
</SliderPrimitive.Track> | |
{!hideThumb && ( | |
<SliderPrimitive.Thumb | |
className={clsx( | |
"block h-5 w-5 rounded-full bg-background transition-colors focus-visible:outline-none border-0 disabled:pointer-events-none disabled:opacity-50", | |
thumb | |
)} | |
/> | |
)} | |
</SliderPrimitive.Root> | |
) | |
); |
client/src/pages/games/Gameplay.tsx
Outdated
useEffect(() => { | ||
if (!account?.account?.address) { | ||
connect({ connector: connectors[0] }); | ||
} | ||
}, [account, connect, connectors]); |
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 to auto-connect.
The auto-connect implementation should include error handling for failed connection attempts.
useEffect(() => {
if (!account?.account?.address) {
- connect({ connector: connectors[0] });
+ connect({ connector: connectors[0] })
+ .catch((error) => {
+ console.error('Failed to connect:', error);
+ // Handle connection error (e.g., show user feedback)
+ });
}
}, [account, connect, connectors]);
📝 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.
useEffect(() => { | |
if (!account?.account?.address) { | |
connect({ connector: connectors[0] }); | |
} | |
}, [account, connect, connectors]); | |
useEffect(() => { | |
if (!account?.account?.address) { | |
connect({ connector: connectors[0] }) | |
.catch((error) => { | |
console.error('Failed to connect:', error); | |
// Handle connection error (e.g., show user feedback) | |
}); | |
} | |
}, [account, connect, connectors]); |
const [copied, setCopied] = useState(false); | ||
|
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
Improve clipboard copy feedback mechanism.
The current implementation has several issues:
- The timeout is hardcoded and might not be enough for users to notice the feedback.
- The state is global for all items, which could lead to incorrect feedback if multiple items are copied quickly.
-const [copied, setCopied] = useState(false);
+const [copiedItems, setCopiedItems] = useState<Record<string, boolean>>({});
-setCopied(true);
+setCopiedItems(prev => ({ ...prev, [index]: true }));
-setTimeout(() => {
- setCopied(false);
-}, 2000);
+const timer = setTimeout(() => {
+ setCopiedItems(prev => ({ ...prev, [index]: false }));
+}, 3000);
+return () => clearTimeout(timer);
Also applies to: 311-318
{games[index].node.player_one != "0x0" && | ||
games[index].node.player_two !== "0x0" ? ( | ||
<Link | ||
target="_blank" | ||
to={`https://x.com/intent/tweet?text=Check%20out%20this%20game%20on%20Dojo%20Duels!%20${window.location.origin}/${games[index].node.game_id}`} | ||
> | ||
<div className="flex flex-row items-center justify-center space-x-1 pr-10"> | ||
className="w-5 h-5 text-[#FAB580]" | ||
color="#FAB580" | ||
fill="#FAB580" | ||
/> | ||
<p className="text-sm">Share</p> | ||
</div> | ||
</td> | ||
<td className="flex flex-row items-center p-4 space-x-5 w-[200px]"> | ||
{games[index].node.player_two !== "0x0" ? ( | ||
<div className="flex flex-row items-center space-x-2.5 w-fit"> | ||
<div | ||
className="bg-[#FFE600] w-8 h-8 flex items-center justify-center rounded-full" | ||
style={{ backgroundColor: challengedColor }} | ||
> | ||
<UserIcon | ||
color="#F58229" | ||
className="w-5 h-5 text-white" | ||
/> | ||
</div> | ||
<p className="font-normal text-white"> | ||
{item.challenged_name | ||
? item.challenged_name | ||
: truncateString( | ||
games[index].node.player_two, | ||
)} | ||
</p> | ||
</div> | ||
</Link> |
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
Improve social sharing URL construction.
The Twitter share URL construction is not properly encoded and might break with special characters. Also, consider adding Open Graph meta tags for better social media sharing.
+const getShareUrl = (gameId: string) => {
+ const baseUrl = window.location.origin;
+ const text = encodeURIComponent(`Check out this game on Dojo Duels!`);
+ const url = encodeURIComponent(`${baseUrl}/${gameId}`);
+ return `https://x.com/intent/tweet?text=${text}&url=${url}`;
+};
-to={`https://x.com/intent/tweet?text=Check%20out%20this%20game%20on%20Dojo%20Duels!%20${window.location.origin}/${games[index].node.game_id}`}
+to={getShareUrl(games[index].node.game_id)}
Consider adding Open Graph meta tags in your HTML head for better social media sharing:
<meta property="og:title" content="Dojo Duels Game" />
<meta property="og:description" content="Check out this exciting game on Dojo Duels!" />
<meta property="og:image" content="[Your game preview image URL]" />
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: 6
🔭 Outside diff range comments (1)
client/src/components/lobby/duels.tsx (1)
Line range hint
71-93
: Improve error handling and use constants for game states.The current implementation has several areas for improvement:
- Error handling only logs to console
- Game status uses string literals
+const GAME_STATES = { + CLOSED: 'closed', + OPEN: 'open', +} as const; +const handleGameError = (error: Error) => { + console.error(error); + // Add user-friendly error notification here +}; const runGameAction = (index: number) => { const game = games[index].node; const isPlayerInGame = game.player_one === account.account?.address || game.player_two === account.account?.address; const isGameFull = game.player_one !== "0x0" && game.player_two !== "0x0"; - const isGameClosed = game.status === "closed"; + const isGameClosed = game.status === GAME_STATES.CLOSED; if (isGameClosed) { - alert("This game is closed."); + handleGameError(new Error("This game is closed")); return; } if (isPlayerInGame || isGameFull) { navigate(`/games/${game.game_id}`); } else { join_game(game.game_id, index) .then((res) => console.info(res)) - .catch((errorJoiningGame) => console.error(errorJoiningGame)); + .catch(handleGameError); } };
🧹 Nitpick comments (23)
client/src/pages/games/Gameplay.tsx (3)
33-33
: Simplify the boolean expression for better readability.The involved check can be simplified by removing the unnecessary ternary operator.
- const involved = - game_players?.mancalaAlphaPlayerModels.edges.filter( - (item: any) => item?.node.address === account.address, - ).length > 0 - ? true - : false; + const involved = game_players?.mancalaAlphaPlayerModels.edges.filter( + (item: any) => item?.node.address === account.address, + ).length > 0;Also applies to: 38-42, 44-46, 50-51
54-54
: Document the default volume value.Consider adding a comment explaining why 35 was chosen as the default volume level. This helps maintain the code's clarity for future developers.
+ // Default volume set to 35 (35% of maximum) for optimal initial playback experience const [volume, setVolume] = useState(35);
Also applies to: 84-85, 88-88
56-56
: Consider using environment variables for asset paths.To improve maintainability and prevent broken paths if assets are reorganized, consider moving the background image path to an environment variable or constants file.
- className="min-h-screen w-full bg-[#0F1116] bg-[url('./assets/bg.png')] bg-cover bg-center bg-no-repeat flex flex-col items-center overflow-y-scroll" + className={`min-h-screen w-full bg-[#0F1116] bg-[url('${ASSET_PATHS.backgroundImage}')] bg-cover bg-center bg-no-repeat flex flex-col items-center overflow-y-scroll`}client/src/components/ui/dropdown-menu.tsx (4)
19-56
: Consider adding ARIA labels for better accessibility.While the implementation is solid, consider adding ARIA labels to improve accessibility for screen readers, especially for the ChevronRight icon.
<DropdownMenuPrimitive.SubTrigger ref={ref} + aria-label={`${typeof children === 'string' ? children : ''} submenu`} className={cn( "flex cursor-default gap-2 select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none focus:bg-accent data-[state=open]:bg-accent [&_svg]:pointer-events-none [&_svg]:size-4 [&_svg]:shrink-0", inset && "pl-8", className )} {...props} > {children} - <ChevronRight className="ml-auto" /> + <ChevronRight className="ml-auto" aria-hidden="true" /> </DropdownMenuPrimitive.SubTrigger>
57-74
: Consider lazy loading for better initial performance.The content is always rendered within a Portal. Consider adding the
forceMount={false}
prop to enable lazy loading, which would only render the content when the dropdown is open.<DropdownMenuPrimitive.Content ref={ref} sideOffset={sideOffset} + forceMount={false} className={cn( "z-50 min-w-[8rem] overflow-hidden rounded-md border p-1 shadow-md data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", className )} {...props} />
75-138
: Enhance keyboard navigation feedback.The menu items have good visual styling but could benefit from more explicit keyboard focus states.
"relative flex cursor-default select-none items-center gap-2 rounded-sm px-2 py-1.5 text-sm outline-none transition-colors [&_svg]:pointer-events-none [&_svg]:shrink-0", + "focus:bg-accent focus:text-accent-foreground data-[disabled]:pointer-events-none data-[disabled]:opacity-50", inset && "pl-8",
139-181
: Enhance shortcut visibility and semantics.The shortcut component could benefit from better semantic markup and visibility states.
const DropdownMenuShortcut = ({ className, ...props -}: React.HTMLAttributes<HTMLSpanElement>) => { +}: React.HTMLAttributes<HTMLElement>) => { return ( - <span + <kbd className={cn( - "ml-auto text-xs tracking-widest opacity-60", + "ml-auto text-xs tracking-widest opacity-60 font-mono", + "hidden sm:inline-block", // Hide on mobile className )} {...props} - /> + /> ) }client/src/App.tsx (2)
16-21
: Consider adding type safety and environment configuration.
- Add type safety by explicitly typing the options object:
-const options = { +const options: ControllerOptions = {
- Consider moving hardcoded values to environment variables for better configuration management:
const options = { theme: "realm-of-ra", policies: POLICIES, - namespace: "mancala_alpha", - slot: "mancala-e", + namespace: process.env.REACT_APP_MANCALA_NAMESPACE ?? "mancala_alpha", + slot: process.env.REACT_APP_MANCALA_SLOT ?? "mancala-e", rpc: SLOT_RPC_URL, };
62-68
: Fix indentation for better readability.The Router and Routes components have inconsistent indentation. Apply this diff to fix:
<StarknetConfig> - <Router> - <Routes> - <Route index element={<Home />} /> - <Route path="/lobby" element={<Lobby />} /> - <Route path="/games/:gameId" element={<Gameplay />} /> - </Routes> - </Router> + <Router> + <Routes> + <Route index element={<Home />} /> + <Route path="/lobby" element={<Lobby />} /> + <Route path="/games/:gameId" element={<Gameplay />} /> + </Routes> + </Router>client/src/pages/Lobby.tsx (3)
166-183
: Remove commented-out code.The commented-out audio-related code should be removed if it's no longer needed. Keeping commented-out code adds unnecessary noise to the codebase.
- // const [isPlaying, setPlaying] = useAtom(isPlayingAtom); - - // const audioRef = useRef(new Audio(audio)); - // useEffect(() => { - // if (isPlaying) { - // try { - // audioRef.current.play(); - // audioRef.current.loop = true; - // } catch (error) { - // console.error("Error playing the audio", error); - // } - // } else { - // audioRef.current.pause(); - // } - // return () => { - // audioRef.current.pause(); - // }; - // }, [isPlaying]);
207-212
: Optimize useEffect dependencies.The useEffect dependency array includes state variables that might not be necessary for the URL update logic.
useEffect(() => { if (gameId) { setGameUrl(`${window.location.origin}/games/${gameId}`); setCreating(false); } -}, [gameId, creating, open, type, playWith]); +}, [gameId]);
467-472
: Add loading state for wallet connection.The wallet connection button should show a loading state while the connection is in progress.
+const [isConnecting, setIsConnecting] = useState(false); const connectWallet = async () => { + setIsConnecting(true); + try { await connect({ connector: connectors[0] }); + } catch (error) { + console.error('Failed to connect wallet:', error); + // Add user feedback here + } finally { + setIsConnecting(false); + } }; <Button className="flex justify-center items-center font-medium mx-auto relative bg-[#F58229] hover:bg-[#F58229] text-lg white whitespace-nowrap rounded-full py-4 px-5 text-[#FCE3AA]" onClick={connectWallet} + disabled={isConnecting} > - Connect Wallet + {isConnecting ? <CreateLoaderSVG /> : 'Connect Wallet'} </Button>client/src/components/lobby/duels.tsx (3)
Line range hint
31-52
: Consider extracting data transformation logic.The data transformation logic should be moved outside the component to improve maintainability and reusability. Also, avoid hardcoding the truncation length.
+const transformGameData = (games: any[], transactions: any[]) => + games.map((data: any, index: number) => ({ + challenger: data.node.player_one, + challenged: data.node.player_two, + challenger_name: data.node.player_one === "0x0" + ? "0x0" + : formatPlayerName(data.node.player_one_name, data.node.player_one), + challenged_name: data.node.player_two === "0x0" + ? "0x0" + : formatPlayerName(data.node.player_two_name, data.node.player_two), + winner: data.node.winner === "0x0" + ? "0x0" + : formatPlayerName(data.node.winner, data.node.winner), + date: transactions[index].executedAt, + status: data.node.status, + })); export default function Duels({ games, transactions, loading, }: { games: any; transactions: any; loading: boolean; }) { - const data = games?.map((data: any, index: number) => { - return { - challenger: data.node.player_one, - challenged: data.node.player_two, - challenger_name: - data.node.player_one === "0x0" - ? "0x0" - : formatPlayerName(data.node.player_one_name, data.node.player_one), - challenged_name: - data.node.player_two === "0x0" - ? "0x0" - : formatPlayerName(data.node.player_two_name, data.node.player_two), - winner: - data.node.winner === "0x0" - ? "0x0" - : formatPlayerName(data.node.winner, data.node.winner, 4), - date: transactions[index].executedAt, - status: data.node.status, - }; - }); + const data = transformGameData(games, transactions);
98-99
: Add a meaningful default message for empty duels.Passing an empty string as the message prop doesn't provide helpful feedback to users.
- return <EmptyDuels message="" />; + return <EmptyDuels message="No active duels found. Create a new game to get started!" />;
133-135
: Improve table structure for better accessibility.The current table structure could be improved for better semantics and accessibility.
- <div className="absolute h-[450px] w-full overflow-x-clip overflow-y-scroll scrollbar rounded-b-xl"> - <table className="w-full text-left table-auto px-2.5"> - <tbody className="h-[450px] w-[928px] rounded-b-xl fixed overflow-y-scroll overflow-x-clip space-y-2 bg-[#0F1116]"> + <div className="relative h-[450px] w-full rounded-b-xl"> + <table className="w-full text-left table-auto"> + <thead> + <tr> + {/* Add table headers here */} + </tr> + </thead> + <tbody className="block h-[450px] w-full overflow-y-auto space-y-2 bg-[#0F1116]">client/src/components/ui/svgs/edit-pencil.tsx (1)
1-19
: Add accessibility attributes to the SVG icon.While the SVG implementation is correct, it's missing accessibility attributes that would improve screen reader support.
Apply this diff to add accessibility attributes:
export default function EditPencil() { return( <svg xmlns="http://www.w3.org/2000/svg" width="15" height="15" fill="none" viewBox="0 0 15 15" + role="img" + aria-label="Edit pencil icon" >client/src/components/ui/svgs/x-icon.tsx (1)
1-23
: Improve accessibility and optimize SVG path.The implementation has two areas for improvement:
- Missing accessibility attributes for screen readers
- The path data could be simplified for better performance
Apply these improvements:
export function XIcon() { return ( <svg xmlns="http://www.w3.org/2000/svg" width="18" height="16" fill="none" viewBox="0 0 18 16" + role="img" + aria-label="Close icon" > <g clipPath="url(#clip0_5290_28558)"> <path fill="#fff" - d="M17.704 15.992h-2.532c-.89 0-1.78-.006-2.669.005a.48.48 0 0 1-.434-.219q-1.952-2.565-3.92-5.115a8 8 0 0 0-.198-.25c-.081.087-.153.162-.222.241a2855 2855 0 0 0-4.461 5.093c-.155.177-.307.258-.544.254-.772-.015-1.543-.009-2.394-.009l6.35-7.244C4.46 5.854 2.242 2.958-.001.032.101.022.175.006.243.006 1.945.006 3.644.01 5.349 0c.198 0 .3.084.404.223q1.749 2.314 3.504 4.624c.063.084.13.165.209.265.732-.837 1.447-1.65 2.161-2.466S13.068 1 13.793.179c.065-.074.165-.167.25-.168.845-.011 1.692-.005 2.611-.005l-5.938 6.773zM3.141 1.535l.62.807 9.054 11.81c.063.081.15.201.229.205.46.02.923.01 1.435.01-.1-.141-.159-.23-.223-.312l-4.196-5.53a7872 7872 0 0 0-5.135-6.77c-.07-.092-.185-.206-.284-.21-.474-.022-.95-.01-1.5-.01" + d="M17.704 15.992H12.503a.48.48 0 0 1-.434-.219L8.149 10.658a8 8 0 0 0-.198-.25L3.49 15.501c-.155.177-.307.258-.544.254H0l6.35-7.244L0 .032h5.349c.198 0 .3.084.404.223l3.713 4.889 2.161-2.466L13.793.179c.065-.074.165-.167.25-.168h3.661l-5.938 6.773L17.704 15.992z" ></path> </g>client/src/components/ui/svgs/catridge-icon.tsx (2)
1-1
: Fix typo in component name.The component name has a typo: "Catridge" should be "Cartridge".
-export default function CatridgeIcon() { +export default function CartridgeIcon() {
1-33
: Optimize image usage and improve accessibility.Consider the following improvements:
The base64 encoded image increases the bundle size. Consider moving it to an external asset.
Missing accessibility attributes for screen readers.
Extract the image to an external file:
import cartridgeImage from '../../../assets/images/cartridge.png'; export default function CartridgeIcon() { return ( <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" fill="none" viewBox="0 0 24 24" role="img" aria-label="Cartridge icon" > <image href={cartridgeImage} width="24" height="24" /> </svg> ); }
- If you need to keep the base64 image inline, at least add accessibility attributes:
<svg xmlns="http://www.w3.org/2000/svg" xmlnsXlink="http://www.w3.org/1999/xlink" width="24" height="24" fill="none" viewBox="0 0 24 24" + role="img" + aria-label="Cartridge icon" >client/src/components/header.tsx (1)
206-214
: Improve error handling in image upload.The error is only logged to the console without providing feedback to the user.
Consider updating the error handling to provide user feedback:
try { const url = await uploadFile(file as File); setImageUrl(url); } catch (error) { console.error("Error uploading image:", error); - // Optionally handle error state here + setSaveStatus({ + status: 'error', + message: 'Failed to upload image. Please try again.' + }); } finally { setImageLoading(false); }client/src/lib/constants.ts (3)
810-817
: Add type safety and documentation for the positions function.The function could benefit from:
- TypeScript type for the return value
- JSDoc documentation explaining the coordinate system
- Constants for magic numbers
+/** Coordinate type for seed positions */ +type Coordinate = { + x: number; + y: number; +}; + +/** Position configuration for a player's seeds */ +type PlayerPosition = { + native: Coordinate[]; + non_native: Coordinate[]; +}; + +/** Complete position configuration for both players */ +type Position = { + player: PlayerPosition; + opponent: PlayerPosition; +}; + +// Constants for layout configuration +const PLAYER_X_STEP = 120; +const OPPONENT_Y_OFFSET = -110; +const OPPONENT_X_BASE = 600; + +/** + * Calculates seed positions for the Mancala board + * @param type - The type of player ('player' or 'opponent') + * @returns Array of position configurations for each pit + */ -export const positions = (type: "player" | "opponent" | undefined) => +export const positions = (type: "player" | "opponent" | undefined): Position[] => Array.from({ length: 7 }, (_, playerIndex) => { // Base x-offset for this player/opponent pair const xOffset = - type === "player" ? playerIndex * 120 : 600 - playerIndex * 120; - const yOffset = type === "player" ? 0 : -110; + type === "player" ? playerIndex * PLAYER_X_STEP : OPPONENT_X_BASE - playerIndex * PLAYER_X_STEP; + const yOffset = type === "player" ? 0 : OPPONENT_Y_OFFSET;
818-867
: Document the coordinate system for baseCoords.Add documentation explaining the coordinate system and what each set of coordinates represents in the game board layout.
// Base coordinates from player 1 + // Coordinates are relative to the top-left corner of each pit + // Format: { x: <distance from left>, y: <distance from top> } + // Groups of coordinates represent different seed arrangements within the pit const baseCoords = [
1064-1073
: Simplify coordinate offset calculations.The offset calculations could be more readable with named constants and clearer variable names.
+ const PLAYER_BOARD_WIDTH = 782.5; + const SEED_OFFSET = 185; + const offsetCoords = baseCoords.map((coord) => ({ x: coord.x + xOffset, y: coord.y + yOffset, })); - const coordsOffest = coords.map((coord) => ({ + const adjustedCoords = coords.map((coord) => ({ - x: type === "player" ? 782.5 + (185 - coord.x) + 185 : coord.x, + x: type === "player" ? PLAYER_BOARD_WIDTH + (SEED_OFFSET - coord.x) + SEED_OFFSET : coord.x, y: coord.y, }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
client/src/App.tsx
(2 hunks)client/src/components/gameplay/game-message.tsx
(4 hunks)client/src/components/header.tsx
(2 hunks)client/src/components/lobby/duels.tsx
(3 hunks)client/src/components/profile/user-section.tsx
(5 hunks)client/src/components/ui/dropdown-menu.tsx
(1 hunks)client/src/components/ui/svgs/catridge-icon.tsx
(1 hunks)client/src/components/ui/svgs/edit-pencil.tsx
(1 hunks)client/src/components/ui/svgs/x-icon.tsx
(1 hunks)client/src/lib/constants.ts
(9 hunks)client/src/pages/Home.tsx
(2 hunks)client/src/pages/Lobby.tsx
(21 hunks)client/src/pages/Profile.tsx
(0 hunks)client/src/pages/games/Gameplay.tsx
(2 hunks)
💤 Files with no reviewable changes (1)
- client/src/pages/Profile.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/pages/Home.tsx
- client/src/components/gameplay/game-message.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/components/header.tsx
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
client/src/components/profile/user-section.tsx
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
client/src/pages/games/Gameplay.tsx
[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)
🪛 Gitleaks (8.21.2)
client/src/components/header.tsx
103-103: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (15)
client/src/components/ui/dropdown-menu.tsx (2)
1-18
: LGTM! Well-structured imports and component definitions.The imports are properly organized, and the basic component definitions follow Radix UI's recommended patterns for component composition.
182-198
: LGTM! Well-organized exports.The exports are comprehensive and logically organized, making it easy for consumers to import specific components as needed.
client/src/pages/Lobby.tsx (2)
Line range hint
57-67
: Add error handling for game creation.The game creation function lacks error handling, which could lead to a poor user experience if the creation fails.
const create_game = async () => { setCreating(true); if (account.account) { + try { await system.create_game(account.account, setGameId); if (gameId) { setCreating(false); } + } catch (error) { + console.error('Failed to create game:', error); + setCreating(false); + // Add user feedback here, e.g., toast notification + } } else { console.log("Account not found"); + setCreating(false); } };
97-121
: LGTM: Well-structured filtering and sorting logic.The filtering and sorting logic is well-implemented with clear priorities:
- Games without player two
- Games with player_two = 0x0
- Games with both players
The implementation is clean and maintainable.
client/src/components/lobby/duels.tsx (2)
94-95
: Improve clipboard copy feedback mechanism.The clipboard implementation still needs improvement as mentioned in the previous review.
Also applies to: 297-304
286-292
: Improve social sharing URL construction.The Twitter share URL construction still needs improvement as mentioned in the previous review.
client/src/components/profile/user-section.tsx (3)
40-41
: Add cleanup for polling and consider making interval configurable.The polling interval is hardcoded and there's no cleanup when the component unmounts.
150-154
: PassimageUrl
tocreate_player_profile
.In the
handleSaveProfile
function, when creating a new profile, theimageUrl
is not passed tocreate_player_profile
. This may result in the user's selected profile image not being saved for new users.
113-128
: Add file validation for image uploads.The file input accepts any image type without size validation, which could lead to performance issues or security vulnerabilities.
🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
client/src/components/header.tsx (2)
106-136
: Well-implemented audio handling!The audio implementation includes proper error handling, volume control, and cleanup in the useEffect hook.
101-104
:⚠️ Potential issueMove token address to environment configuration.
The hardcoded token address should be moved to a configuration file or environment variable for better security and maintainability.
Apply this diff to move the token address to environment configuration:
- token: "0x04718f5a0fc34cc1af16a1cdee98ffb20c31f5cd61d6ab07201858f4287c938d", + token: process.env.REACT_APP_STRK_TOKEN_ADDRESS,Likely invalid or redundant comment.
🧰 Tools
🪛 Gitleaks (8.21.2)
103-103: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
client/src/lib/constants.ts (4)
354-357
: LGTM! The header entry is properly structured.The "Date" header entry is correctly implemented with both name and id properties.
Line range hint
658-768
: LGTM! GraphQL queries are consistently updated.The migration from
mancalaDev
tomancalaAlpha
is applied consistently across all existing queries.
780-808
: LGTM! New queries follow established patterns.The new
MancalaCaptureQuery
andMancalaExtraTurnQuery
are well-structured and follow the same naming conventions and patterns as existing queries.
608-612
: Verify contract addresses on Starknet Sepolia.The contract addresses appear to be valid Starknet addresses. Please ensure these addresses are deployed and verified on Starknet Sepolia network.
✅ Verification successful
Contract addresses verified in Sepolia manifest
The contract addresses are correctly configured and documented in
contracts/manifest_sepolia.json
. Both addresses are properly deployed on Sepolia network with their respective class hashes:
- Mancala: 0x470f026bf97f0781492a95ebb9031da1c524601e8c2a285534545d33ca44797 (class_hash: 0x4841cd2...)
- Profile: 0x4a992660cbd6a91cda1e1697e43c52641f3818aeabcf96658a986322603ef85 (class_hash: 0x745472c...)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify contract deployment and code verification status # Check if contracts are deployed curl -s "$SLOT_RPC_URL" \ -X POST \ -H "Content-Type: application/json" \ -d '{ "jsonrpc": "2.0", "method": "starknet_getCode", "params": ["0x470f026bf97f0781492a95ebb9031da1c524601e8c2a285534545d33ca44797"], "id": 1 }' curl -s "$SLOT_RPC_URL" \ -X POST \ -H "Content-Type: application/json" \ -d '{ "jsonrpc": "2.0", "method": "starknet_getCode", "params": ["0x4a992660cbd6a91cda1e1697e43c52641f3818aeabcf96658a986322603ef85"], "id": 2 }'Length of output: 421
Script:
#!/bin/bash # Search for network configuration and environment files fd -e json -e env -e yaml -e yml -e config # Search for references to these addresses rg -A 2 "0x470f026bf97f0781492a95ebb9031da1c524601e8c2a285534545d33ca44797" rg -A 2 "0x4a992660cbd6a91cda1e1697e43c52641f3818aeabcf96658a986322603ef85" # Look for network-related configuration rg -i "sepolia|testnet|mainnet" --type tsLength of output: 2097
game_players?.mancalaAlphaPlayerModels.edges.filter( | ||
(item: any) => item?.node.address === account.address, | ||
).length > 0 | ||
? true | ||
: false; | ||
const player_position = involved | ||
? game_players?.mancalaDevPlayerModels.edges.findIndex( | ||
? game_players?.mancalaAlphaPlayerModels.edges.findIndex( | ||
(item: any) => item?.node.address === account.address, | ||
) |
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
Improve type safety by replacing 'any' with proper types.
The use of 'any' type reduces type safety. Consider defining proper interfaces for the player models and edges.
interface PlayerNode {
address: string;
// Add other relevant fields
}
interface PlayerEdge {
node: PlayerNode;
}
// Then use these types instead of 'any'
(item: PlayerEdge) => item?.node.address === account.address
🧰 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)
client/src/App.tsx
Outdated
<Router> | ||
<Routes> | ||
<Route index element={<Home />} /> | ||
<Route path="/lobby" element={<Lobby />} /> | ||
<Route path="/games/:gameId" element={<Gameplay />} /> | ||
</Routes> | ||
</Router> |
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.
💡 Codebase verification
Inconsistency found: Leaderboard route removed but components still active
The removal of the leaderboard route will break existing functionality. Found:
- Navigation link to "/leaderboard" in
client/src/components/gameplay/leaderboard-button.tsx
- Active leaderboard component usage in
client/src/pages/Lobby.tsx
- Multiple leaderboard-related components still in use
🔗 Analysis chain
Verify the removal of the Leaderboard route.
The Leaderboard route has been removed. Let's verify if there are any remaining references to this route that need to be cleaned up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the leaderboard route or component
echo "Searching for leaderboard references in components and navigation..."
rg -i "leaderboard" ./client/src -t tsx -t ts
echo "Searching for potential navigation links to /leaderboard..."
rg -i "to=[\"\']*/leaderboard" ./client/src -t tsx -t ts
Length of output: 503
Script:
#!/bin/bash
# Search for any remaining references to leaderboard
echo "Searching for leaderboard references..."
rg -i "leaderboard" ./client/src
echo -e "\nSearching for potential navigation links..."
rg -i 'to=["'\'']?/leaderboard' ./client/src
echo -e "\nSearching for Leaderboard component files..."
fd -i leaderboard ./client/src
Length of output: 2434
className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium" | ||
placeholder="0x..." | ||
onChange={(e) => setPlayer2(e.target.value)} | ||
/> |
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.
Add input validation for wallet address.
The player2 address input lacks validation, which could lead to invalid addresses being submitted.
<input
className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium"
placeholder="0x..."
- onChange={(e) => setPlayer2(e.target.value)}
+ onChange={(e) => {
+ const address = e.target.value;
+ if (address.startsWith('0x') && /^0x[a-fA-F0-9]{1,64}$/.test(address)) {
+ setPlayer2(address);
+ }
+ }}
+ pattern="0x[a-fA-F0-9]{1,64}"
/>
📝 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.
className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium" | |
placeholder="0x..." | |
onChange={(e) => setPlayer2(e.target.value)} | |
/> | |
className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium" | |
placeholder="0x..." | |
onChange={(e) => { | |
const address = e.target.value; | |
if (address.startsWith('0x') && /^0x[a-fA-F0-9]{1,64}$/.test(address)) { | |
setPlayer2(address); | |
} | |
}} | |
pattern="0x[a-fA-F0-9]{1,64}" | |
/> |
fill="none" | ||
viewBox="0 0 24 24" | ||
> | ||
<path fill="url(#pattern0_3630_15563)" d="M-2 0h28.8v24H-2z"></path> |
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
Fix negative x-coordinate in path.
The path has a negative x-coordinate (-2) which could cause rendering issues in some browsers.
- <path fill="url(#pattern0_3630_15563)" d="M-2 0h28.8v24H-2z"></path>
+ <path fill="url(#pattern0_3630_15563)" d="M0 0h26.8v24H0z"></path>
📝 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.
<path fill="url(#pattern0_3630_15563)" d="M-2 0h28.8v24H-2z"></path> | |
<path fill="url(#pattern0_3630_15563)" d="M0 0h26.8v24H0z"></path> |
|
||
const color = getColorOfTheDay(account?.address || "", new Date()); | ||
const { data, startPolling } = useQuery(MancalaHeaderQuery); | ||
startPolling(1000); |
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.
Consolidate polling calls and add cleanup.
Multiple polling calls with the same interval (1000ms) are made throughout the component. This could lead to performance issues.
Consider:
- Consolidating the polling calls into a single useEffect.
- Adding cleanup to prevent memory leaks.
+const POLL_INTERVAL = 1000;
+
+useEffect(() => {
+ startPolling(POLL_INTERVAL);
+ startPollingPlayerData(POLL_INTERVAL);
+ startPollingProfiles(POLL_INTERVAL);
+
+ return () => {
+ // Cleanup polling on unmount
+ };
+}, [startPolling, startPollingPlayerData, startPollingProfiles]);
-startPolling(1000);
-startPollingPlayerData(1000);
-startPollingProfiles(1000);
Also applies to: 137-137, 187-187
* sliding * sliding * wip * update restart * proper messaging * pit color changing
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: 2
🔭 Outside diff range comments (1)
client/src/components/message-area.tsx (1)
Line range hint
33-41
: Improve type safety and error handling in restart_game.The function uses a type assertion to 'never' and lacks error handling:
- const restart_game = async () => { - if (account) { - await system.restart_game( - account as never, - gameId || "", - setRestarted, - opponent_requested_restart, - ); - } - }; + const restart_game = async () => { + try { + if (!account) { + throw new Error('Account not connected'); + } + if (!gameId) { + throw new Error('Game ID not found'); + } + await system.restart_game( + account, + gameId, + setRestarted, + opponent_requested_restart, + ); + } catch (error) { + setMessage(`Failed to restart game: ${error.message}`); + } + };
🧹 Nitpick comments (6)
client/src/components/message-area.tsx (1)
19-27
: Simplify and type-safe the data access pattern.The nested optional chaining and type assertions make the code fragile. Consider creating proper types and helper functions:
interface Player { address: string; restart_requested: boolean; } interface GamePlayer { node: Player; } function getActivePlayers(players: GamePlayer[]): Player[] { return players?.map(item => item.node) ?? []; }Then simplify the access:
- const active_players_addrs = - game_players?.mancalaAlphaPlayerModels?.edges?.map( - (item: any) => item?.node?.address, - ) ?? []; + const players = getActivePlayers(game_players?.mancalaAlphaPlayerModels?.edges ?? []); + const active_players_addrs = players.map(player => player.address);client/src/components/gameplay/game-navigation.tsx (2)
28-31
: Add proper TypeScript interfaces for props.The props are currently typed as 'any', which reduces type safety. Consider adding proper interfaces:
interface Action { action: (() => void) | undefined; message: string; } interface GameNavigationProps { // ... other props ... message: string; setMessage: (message: string) => void; action: Action; setAction: (action: Action) => void; }
58-59
: Simplify the isCurrentUserPlaying check.The current check can be simplified using optional chaining and destructuring.
-const isCurrentUserPlaying = account?.account?.address && (account?.account?.address === game_node?.player_one || account?.account?.address === game_node?.player_two); +const { address } = account?.account ?? {}; +const isCurrentUserPlaying = address && (address === game_node?.player_one || address === game_node?.player_two);client/src/pages/games/Gameplay.tsx (1)
38-42
: Simplify the involved check.The boolean literal in the ternary operation is unnecessary.
-const involved = - game_players?.mancalaAlphaPlayerModels.edges.filter( - (item: any) => item?.node.address === account.address, - ).length > 0 - ? true - : false; +const involved = game_players?.mancalaAlphaPlayerModels.edges.some( + (item: any) => item?.node.address === account.address +);🧰 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)
client/src/components/gameplay/game-message.tsx (1)
1-1
: Remove ESLint disable and fix dependencies.Instead of disabling the ESLint rule, properly manage the effect dependencies.
Consider using useCallback for functions passed as props and including all necessary dependencies in the useEffect dependency array.
client/src/components/gameplay/game-board.tsx (1)
182-208
: Optimize seed rendering logic.The seed rendering logic could be optimized by memoizing the player address check.
const playerAddressCheck = useMemo(() => { const playerAddress = account.account?.address; return (seedPlayer: string) => { if (playerAddress === game_node?.player_one) { return seedPlayer === game_node?.player_one; } if (playerAddress === game_node?.player_two) { return seedPlayer === game_node?.player_two; } return seedPlayer === game_node?.player_one; }; }, [account.account?.address, game_node?.player_one, game_node?.player_two]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/src/assets/main-message-section.png
is excluded by!**/*.png
and included byclient/**
client/src/assets/message-slide.png
is excluded by!**/*.png
and included byclient/**
📒 Files selected for processing (9)
client/src/components/gameplay/end-game-button.tsx
(2 hunks)client/src/components/gameplay/game-board.tsx
(7 hunks)client/src/components/gameplay/game-message.tsx
(8 hunks)client/src/components/gameplay/game-navigation.tsx
(3 hunks)client/src/components/gameplay/restart-button.tsx
(3 hunks)client/src/components/gameplay/timeout-button.tsx
(3 hunks)client/src/components/message-area.tsx
(2 hunks)client/src/components/pits.tsx
(4 hunks)client/src/pages/games/Gameplay.tsx
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- client/src/components/gameplay/timeout-button.tsx
- client/src/components/gameplay/restart-button.tsx
- client/src/components/gameplay/end-game-button.tsx
- client/src/components/pits.tsx
🧰 Additional context used
📓 Learnings (1)
client/src/components/gameplay/game-message.tsx (1)
Learnt from: supreme2580
PR: realm-of-ra/mancala#149
File: client/src/pages/games/Gameplay.tsx:0-0
Timestamp: 2024-11-12T06:00:31.919Z
Learning: The `moveMessageOnTimer` function in `client/src/pages/games/Gameplay.tsx` has been updated to use `React.createElement` instead of `dangerouslySetInnerHTML` to avoid XSS attacks.
🪛 Biome (1.9.4)
client/src/pages/games/Gameplay.tsx
[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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
client/src/components/message-area.tsx (1)
32-32
: Avoid disabling ESLint rules without justification.The ESLint rule for exhaustive dependencies is being disabled without explanation. This rule helps prevent bugs related to stale closures in React hooks.
Instead of disabling the rule:
- Add all required dependencies to the useEffect dependency array
- If certain dependencies need to be omitted, add a comment explaining why
- Consider using useCallback for the restart_game function if it needs to be stable across renders
Also applies to: 42-50
client/src/pages/games/Gameplay.tsx (1)
50-51
: Add null check for opposition_address.The opposition_address could be undefined if the opponent's node is not found.
-const opposition_address = - game_players?.mancalaAlphaPlayerModels.edges[opponent_position]?.node - .address; +const opposition_address = + game_players?.mancalaAlphaPlayerModels.edges[opponent_position]?.node + ?.address;✅ Verification successful
The review comment is correct and the suggested fix is necessary
The suggested addition of optional chaining for the
address
property access aligns with the codebase's patterns. This is particularly important here because:
- The code uses direct array indexing which could return undefined
- Similar patterns throughout the codebase consistently use optional chaining for node property access
- The fix prevents potential runtime errors when the opponent's node is not found
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for type definitions related to MancalaAlphaPlayerModel ast-grep --pattern 'type MancalaAlphaPlayerModel = { $$$ }' # Search for interface definitions ast-grep --pattern 'interface MancalaAlphaPlayerModel { $$$ }' # Look for similar access patterns rg "\.node\.address" -A 2 -B 2 # Search for type definitions in GraphQL if present fd "\.graphql$" --exec cat {}Length of output: 6998
client/src/components/gameplay/game-board.tsx (1)
57-60
:⚠️ Potential issueAdd cleanup for polling intervals.
The polling should be stopped when the component unmounts to prevent memory leaks.
useEffect(() => { startCapturePolling(1000); startExtraTurnPolling(1000); + return () => { + startCapturePolling(0); + startExtraTurnPolling(0); + }; }, [startCapturePolling, startExtraTurnPolling]);Likely invalid or redundant comment.
setMessage: any; | ||
setAction: any; |
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
Define proper TypeScript types instead of using 'any'.
Using any
type reduces type safety. Consider defining proper types for these props:
interface IMessageAreaProps {
address: string | undefined;
game_players: any;
- setMessage: any;
- setAction: any;
+ setMessage: (message: string) => void;
+ setAction: (action: { action: (() => Promise<void>) | undefined, message: string }) => void;
}
📝 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.
setMessage: any; | |
setAction: any; | |
interface IMessageAreaProps { | |
address: string | undefined; | |
game_players: any; | |
setMessage: (message: string) => void; | |
setAction: (action: { action: (() => Promise<void>) | undefined, message: string }) => void; | |
} |
setAction({ action: undefined, messsage: '' }) | ||
setMessage(''); |
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.
Fix typo in setAction call.
There's a typo in the property name 'messsage' (three 's').
- setAction({ action: undefined, messsage: '' })
+ setAction({ action: undefined, message: '' })
📝 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.
setAction({ action: undefined, messsage: '' }) | |
setMessage(''); | |
setAction({ action: undefined, message: '' }) | |
setMessage(''); |
* sliding * sliding * wip * update restart * proper messaging * pit color changing * fixed names in game message * image url fix * image url fix * decoupled seed audio * merged with staging * merged with staging
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: 4
♻️ Duplicate comments (2)
client/src/components/gameplay/audio-section.tsx (1)
31-46
:⚠️ Potential issueAdd cleanup functions for event listeners.
Event listeners are added but not removed, which could lead to memory leaks.
Apply this diff to add cleanup functions:
useEffect(() => { + const handleEnded = () => { + audio.play().catch(console.error); + }; + const handleError = (e: any) => { + console.error('Audio playback error:', e); + }; + audio.loop = true; - audio.addEventListener('ended', () => { - audio.play().catch(console.error); - }); - audio.addEventListener('error', (e) => { - console.error('Audio playback error:', e); - }); + audio.addEventListener('ended', handleEnded); + audio.addEventListener('error', handleError); return () => { audio.pause(); audio.currentTime = 0; + audio.removeEventListener('ended', handleEnded); + audio.removeEventListener('error', handleError); }; }, [audio]);client/src/pages/Lobby.tsx (1)
384-387
:⚠️ Potential issueAdd input validation for wallet address.
The player2 address input lacks validation.
Apply this diff to add validation:
<input className="p-2.5 w-72 rounded-xl border border-[#1D212B] bg-transparent outline-none placeholder:text-[#4F5666] placeholder:font-medium text-[#4F5666] font-medium" placeholder="0x..." - onChange={(e) => setPlayer2(e.target.value)} + onChange={(e) => { + const address = e.target.value; + if (address.startsWith('0x') && /^0x[a-fA-F0-9]{1,64}$/.test(address)) { + setPlayer2(address); + } + }} + pattern="0x[a-fA-F0-9]{1,64}" />
🧹 Nitpick comments (11)
client/src/components/gameplay/game-message.tsx (4)
1-1
: Consider using inline eslint-disable comments instead of file-level disable.Rather than disabling the
react-hooks/exhaustive-deps
rule for the entire file, consider using inline eslint-disable comments only for specific hooks where needed. This helps maintain better dependency tracking for other hooks.-/* eslint-disable react-hooks/exhaustive-deps */
107-116
: Simplify player profile resolution logic.The nested profile resolution logic could be simplified and made more readable.
- const findPlayerProfile = (address: string) => { - return profiles?.mancalaAlphaProfileModels?.edges?.find( - (item: any) => normalizeAddress(item?.node.address) === normalizeAddress(address) - )?.node?.name || ""; - }; - - const currentPlayerAddress = game_node?.current_player; - const found_profile_name = shortString.decodeShortString(findPlayerProfile(normalizeAddress(currentPlayerAddress))) - const decodedName = found_profile_name === "0" ? undefined : found_profile_name; + const findPlayerProfile = (address: string): string | undefined => { + const normalizedAddress = normalizeAddress(address); + const profile = profiles?.mancalaAlphaProfileModels?.edges?.find( + (item: any) => normalizeAddress(item?.node.address) === normalizedAddress + ); + const name = profile?.node?.name; + if (!name) return undefined; + + const decoded = shortString.decodeShortString(name); + return decoded === "0" ? undefined : decoded; + }; + + const decodedName = findPlayerProfile(game_node?.current_player);
171-176
: Add input validation to address normalization.The address normalization function should handle invalid inputs more robustly.
const normalizeAddress = (address: string) => { + if (!address) return ''; + if (typeof address !== 'string') return ''; // Remove '0x' prefix, convert to lowercase, and pad with leading zeros if needed const cleanAddress = address?.toLowerCase()?.replace('0x', ''); // Pad to 64 characters (32 bytes) with leading zeros return cleanAddress?.padStart(64, '0'); };
203-231
: Consider extracting animation configuration.The animation configuration could be extracted to improve maintainability.
+ const slideAnimation = { + initial: { y: -40, opacity: 0 }, + animate: (message || action?.message) && !close + ? { y: 0, opacity: 1 } + : { y: -40, opacity: 0 }, + transition: { + type: "spring", + stiffness: 100, + damping: 15, + duration: 5 + } + }; <motion.div className="w-[390px] h-20 bg-[url('./assets/message-slide.png')] bg-center bg-contain bg-no-repeat absolute -bottom-1.5 flex flex-col" - initial={{ y: -40, opacity: 0 }} - animate={(message || action?.message) && !close ? { y: 0, opacity: 1 } : { y: -40, opacity: 0 }} - transition={{ - type: "spring", - stiffness: 100, - damping: 15, - duration: 5 - }} + {...slideAnimation} >client/src/components/ui/dialog.tsx (1)
44-48
: Remove or reintroduce the commented-out close button.
Commented-out code may cause confusion or bloat. If you plan to keep this close button, consider enabling it for better accessibility; otherwise, remove it to keep the code clean.client/src/components/ui/dropdown-menu.tsx (1)
81-90
: Consider modularizing repeated menu item styles.
Classes like"relative flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none transition-colors"
appear in multiple items. You can encapsulate these in a shared utility or style to reduce duplication and ease maintenance.- className={cn( - "relative flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none transition-colors", - inset && "pl-8", - className - )} + const baseMenuItemClass = "relative flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none transition-colors"; + className={cn( + baseMenuItemClass, + inset && "pl-8", + className + )}client/src/components/header.tsx (1)
199-201
: Use optional chaining for file check.
Instead of separately checkingevent.target.files && event.target.files[0]
, optional chaining clarifies code and avoids potential null references.- if (event.target.files && event.target.files[0]) { - const file = event.target.files?.[0]; - ... - } + const file = event.target.files?.[0]; + if (file) { + ... + }🧰 Tools
🪛 Biome (1.9.4)
[error] 201-201: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
client/src/components/seed.tsx (1)
27-37
: Simplify complex position calculation logic.The nested ternary operations make the code hard to read and maintain. Consider refactoring to improve readability.
- const position = useMemo(() => { - return isNative && type === "player" - ? _positions[pit_number - 1]?.player?.native[seed_number - 1] - : isNative && type === "opponent" - ? _positions[pit_number - 1]?.opponent?.native[seed_number - 1] - : !isNative && type === "player" - ? _positions[pit_number - 1]?.player?.non_native[seed_number - 1] - : !isNative && type === "opponent" - ? _positions[pit_number - 1]?.opponent?.non_native[seed_number - 1] - : { x: 0, y: 0 }; - }, [isNative, type, _positions, pit_number, seed_number]); + const position = useMemo(() => { + if (!type || !_positions[pit_number - 1]) { + return { x: 0, y: 0 }; + } + + const playerType = type === "player" ? "player" : "opponent"; + const nativeType = isNative ? "native" : "non_native"; + + return _positions[pit_number - 1]?.[playerType]?.[nativeType]?.[seed_number - 1] + ?? { x: 0, y: 0 }; + }, [isNative, type, _positions, pit_number, seed_number]);client/src/lib/utils.ts (1)
78-79
: Consider adding type safety to the sorting logic.The sorting function uses 'any' type and directly accesses properties without type checking.
Apply this diff to add type safety:
+interface Player { + wins: number; +} + - return players.sort((a: any, b: any) => (b.wins * 50) - (a.wins * 50)); + return players.sort((a: Player, b: Player) => (b.wins * 50) - (a.wins * 50));client/src/pages/Lobby.tsx (2)
97-121
: Add type safety to game filtering and sorting logic.The filtering and sorting logic uses 'any' type extensively.
Consider creating interfaces for the game and node types:
interface GameNode { player_one: string; player_two: string; } interface Game { node: GameNode; }
480-482
: Consider making the feedback link more accessible.The feedback link could benefit from additional accessibility attributes.
Apply this diff to improve accessibility:
-<Link to="https://docs.google.com/forms/d/e/1FAIpQLSerjYkAywGpu3dh3Q999cVUoEBAEGlDlt089Uc0BHRRf9uLnw/viewform?usp=header" target="_blank" className="absolute bottom-10 left-1/2 transform -translate-x-1/2"> +<Link + to="https://docs.google.com/forms/d/e/1FAIpQLSerjYkAywGpu3dh3Q999cVUoEBAEGlDlt089Uc0BHRRf9uLnw/viewform?usp=header" + target="_blank" + rel="noopener noreferrer" + aria-label="Provide feedback and get a chance to win Lord of the Mancala" + className="absolute bottom-10 left-1/2 transform -translate-x-1/2">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/src/components/gameplay/audio-section.tsx
(5 hunks)client/src/components/gameplay/game-message.tsx
(6 hunks)client/src/components/gameplay/game-navigation.tsx
(3 hunks)client/src/components/header.tsx
(2 hunks)client/src/components/seed.tsx
(2 hunks)client/src/components/ui/dialog.tsx
(1 hunks)client/src/components/ui/dropdown-menu.tsx
(1 hunks)client/src/lib/utils.ts
(3 hunks)client/src/pages/Lobby.tsx
(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/components/gameplay/game-navigation.tsx
🧰 Additional context used
📓 Learnings (1)
client/src/components/gameplay/game-message.tsx (1)
Learnt from: supreme2580
PR: realm-of-ra/mancala#149
File: client/src/pages/games/Gameplay.tsx:0-0
Timestamp: 2024-11-12T06:00:31.919Z
Learning: The `moveMessageOnTimer` function in `client/src/pages/games/Gameplay.tsx` has been updated to use `React.createElement` instead of `dangerouslySetInnerHTML` to avoid XSS attacks.
🪛 Biome (1.9.4)
client/src/components/header.tsx
[error] 201-201: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 Gitleaks (8.21.2)
client/src/components/header.tsx
105-105: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (19)
client/src/components/gameplay/game-message.tsx (2)
82-82
: RemovesetTimeRemaining
from dependencies array.Including
setTimeRemaining
in the dependencies array is unnecessary as setState functions are stable and don't change between renders.- }, [game_players?.player_one?.edges, game_players?.player_two?.edges, game_node, gameStarted, startTime, setTimeRemaining]); + }, [game_players?.player_one?.edges, game_players?.player_two?.edges, game_node, gameStarted, startTime]);
223-223
: Fix typo in setAction call.There's a typo in the property name 'messsage' (three 's').
- setAction({ action: undefined, messsage: '' }) + setAction({ action: undefined, message: '' })client/src/components/ui/dialog.tsx (2)
15-27
: Good use offorwardRef
for accessibility.
ExportingDialogOverlay
through aforwardRef
is a recommended approach with Radix UI. This setup looks clean and matches best practices.
109-120
: Confirm the need for exportingDialogClose
.
DialogClose
is exported at line 113 but remains unused in this file. If it’s not used elsewhere, it might be best to remove it. Otherwise, ensure it’s properly integrated to close the dialog.client/src/components/ui/dropdown-menu.tsx (2)
19-37
: Neat approach withDropdownMenuSubTrigger
.
UtilizingReact.forwardRef
and theinset
prop is a great way to keep these components flexible and accessible. Implementation looks solid.
169-180
: Excellent lightweight shortcut display.
DropdownMenuShortcut
is minimal and clear, providing a good example for other shortcuts or keyboard-based hints. Nicely done.client/src/components/header.tsx (3)
94-100
: Use thecontroller
variable consistently to access profile methods.
You’re definingcontroller
at line 46 but usingconnector?.controller
here, which duplicates the logic. Align this with the approach recommended in prior reviews.-const handleTrophyClick = useCallback(() => { - if (!connector?.controller) { - console.error("Connector not initialized"); - return; - } - connector?.controller.openProfile("achievements"); -}, [connector]); +const handleTrophyClick = useCallback(() => { + if (!controller) { + console.error("Controller not initialized"); + return; + } + controller.openProfile("achievements"); +}, [controller]);
68-71
: Avoid directly invokingstartPolling(1000)
in the component body.
Similar to earlier feedback, multiple direct polling calls risk performance issues and potential memory leaks. Wrap them in auseEffect
so they run once on mount, and consider cleaning up on unmount.Also applies to: 139-139
105-105
: Confirm that the “0x0471...” value is not a secret key.
Static analysis flags this token string as a generic API key. If it’s merely a contract address, feel free to ignore. Otherwise, secure it in environment variables.🧰 Tools
🪛 Gitleaks (8.21.2)
105-105: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
client/src/components/seed.tsx (3)
39-41
: LGTM!The animation delay calculation is clean and straightforward.
58-62
: LGTM! Audio cleanup implementation improved.The audio timeout cleanup has been properly implemented using timerRef, addressing the concerns from the previous review.
73-92
: Verify initial position change impact.The initial position has been changed from
{x: 0, y: 0}
to{x: 100, y: 100}
. While the animation parameters have been improved for smoother movement, ensure this new starting position doesn't cause visual glitches.client/src/components/gameplay/audio-section.tsx (3)
9-20
: LGTM! Props and state management look good.The component correctly accepts volume control props and manages dropdown state internally.
22-29
: LGTM! Audio playback logic is well-implemented.The useEffect hook properly manages audio playback based on volume changes.
114-116
: LGTM! Volume control UI is well-implemented.The Slider component and volume display are properly integrated with the volume state.
client/src/lib/utils.ts (2)
11-14
: LGTM! Flexible string truncation implementation.The function now supports customizable truncation length with a sensible default.
164-166
: LGTM! Improved name formatting with flexible truncation.The function correctly handles the new optional truncation parameter.
client/src/pages/Lobby.tsx (2)
40-44
: LGTM! State management is well-organized.The component properly manages game state with useState hooks.
207-212
: 🛠️ Refactor suggestionAdd error handling for game URL generation.
The useEffect hook should handle potential errors when generating game URLs.
Apply this diff to add error handling:
useEffect(() => { if (gameId) { - setGameUrl(`${window.location.origin}/games/${gameId}`); - setCreating(false); + try { + setGameUrl(`${window.location.origin}/games/${gameId}`); + } catch (error) { + console.error('Error generating game URL:', error); + setGameUrl(undefined); + } finally { + setCreating(false); + } } }, [gameId, creating, open, type, playWith]);Likely invalid or redundant comment.
}: { | ||
game_node: any; | ||
game_players: any; | ||
player_one_name: any; | ||
player_two_name: any; | ||
account: any; | ||
profiles: any; | ||
gameStarted: any; | ||
timeRemaining: any; | ||
setTimeRemaining: any; | ||
setProfiles: any; | ||
message: string; | ||
setMessage: any; | ||
action: { action: any, message: string }; | ||
setAction: any; |
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
Replace 'any' types with specific TypeScript interfaces.
The extensive use of any
type defeats TypeScript's purpose of type safety. Consider creating proper interfaces for your props.
interface GamePlayer {
player_one: {
edges: Array<any>; // Define specific edge type
};
player_two: {
edges: Array<any>; // Define specific edge type
};
}
interface GameAction {
action: (() => void) | undefined;
message: string;
}
interface GameMessageProps {
game_node: {
status: string;
winner: string;
current_player: string;
max_block_between_move: string;
player_one: string;
};
game_players: GamePlayer;
player_one_name: string;
player_two_name: string;
account: {
account?: {
address: string;
};
};
gameStarted: boolean;
timeRemaining: number;
setTimeRemaining: (time: number) => void;
message: string;
setMessage: (message: string) => void;
action: GameAction;
setAction: (action: GameAction) => void;
}
const userExists = profiles.mancalaAlphaProfileModels.edges.some( | ||
(profile: any) => profile.node.address === account?.address, | ||
); |
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.
Null-check profiles
before usage.
Accessing profiles.mancalaAlphaProfileModels.edges
without verifying profiles
may cause runtime errors. Add a check or optional chaining to safely handle potential undefined
values.
- const userExists = profiles.mancalaAlphaProfileModels.edges.some(
+ if (!profiles?.mancalaAlphaProfileModels) {
+ console.error("Profiles not loaded yet");
+ return;
+ }
+ const userExists = profiles.mancalaAlphaProfileModels.edges?.some(
(profile: any) => profile.node.address === account?.address,
);
Committable suggestion skipped: line range outside the PR's diff.
useEffect(() => { | ||
audio.volume = 0.35; | ||
// if (volume <= 0) { | ||
// audio.pause(); | ||
// } | ||
|
||
return () => { | ||
if (timerRef.current) { | ||
clearTimeout(timerRef.current); | ||
} | ||
} | ||
|
||
// New logic for pits 1-6 | ||
const INNER_GRID_GAP = 2; | ||
const RING_GAP = 1; | ||
const BASE_OFFSET = 8; | ||
const X_SHIFT = 10; | ||
const Y_SHIFT = 10; | ||
|
||
const positionInSet = ((seedNumber - 1) % 16) + 1; | ||
|
||
// First 4 seeds in a 2x2 grid | ||
if (seedNumber <= 4) { | ||
const row = Math.floor((seedNumber - 1) / 2); | ||
const col = (positionInSet - 1) % 2; | ||
return { | ||
gridX: (col - 0.5) * (SEED_SIZE + INNER_GRID_GAP) + X_SHIFT, | ||
gridY: (row - 0.5) * (SEED_SIZE + INNER_GRID_GAP) + Y_SHIFT, | ||
}; | ||
} | ||
|
||
// Ring positioning for seeds 5 and up | ||
const ringPosition = seedNumber - 4; // Position in the ring pattern | ||
let x = 0, y = 0; | ||
|
||
if (ringPosition <= 4) { | ||
// Left side: 4 seeds | ||
x = -(SEED_SIZE + BASE_OFFSET); | ||
y = ((ringPosition - 2.5) * (SEED_SIZE + RING_GAP)); | ||
} else if (ringPosition <= 6) { | ||
// Top: 2 seeds | ||
x = ((ringPosition - 5.5) * (SEED_SIZE + RING_GAP)); | ||
y = -(SEED_SIZE + BASE_OFFSET); | ||
} else if (ringPosition <= 10) { | ||
// Right side: 4 seeds | ||
x = (SEED_SIZE + BASE_OFFSET); | ||
y = ((ringPosition - 8.5) * (SEED_SIZE + RING_GAP)); | ||
} else if (ringPosition <= 12) { | ||
// Bottom: 2 seeds | ||
x = ((ringPosition - 11.5) * (SEED_SIZE + RING_GAP)); | ||
y = (SEED_SIZE + BASE_OFFSET); | ||
} | ||
|
||
return { | ||
gridX: x + X_SHIFT, | ||
gridY: y + Y_SHIFT, | ||
audio.pause(); | ||
audio.currentTime = 0; | ||
}; | ||
}; | ||
|
||
// Get base position based on type and pit number | ||
const basePosition = | ||
type === "player" | ||
? playerPositions[pit_number as keyof typeof playerPositions] | ||
: opponentPositions[pit_number as keyof typeof opponentPositions]; | ||
}, [volume, audio]); |
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
Replace hardcoded volume value.
The audio volume is hardcoded to 0.35 despite having a volume prop. Consider using the volume prop instead.
- audio.volume = 0.35;
+ audio.volume = volume;
Also, uncomment and implement the volume control logic:
- // if (volume <= 0) {
- // audio.pause();
- // }
+ if (volume <= 0) {
+ audio.pause();
+ }
📝 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.
useEffect(() => { | |
audio.volume = 0.35; | |
// if (volume <= 0) { | |
// audio.pause(); | |
// } | |
return () => { | |
if (timerRef.current) { | |
clearTimeout(timerRef.current); | |
} | |
} | |
// New logic for pits 1-6 | |
const INNER_GRID_GAP = 2; | |
const RING_GAP = 1; | |
const BASE_OFFSET = 8; | |
const X_SHIFT = 10; | |
const Y_SHIFT = 10; | |
const positionInSet = ((seedNumber - 1) % 16) + 1; | |
// First 4 seeds in a 2x2 grid | |
if (seedNumber <= 4) { | |
const row = Math.floor((seedNumber - 1) / 2); | |
const col = (positionInSet - 1) % 2; | |
return { | |
gridX: (col - 0.5) * (SEED_SIZE + INNER_GRID_GAP) + X_SHIFT, | |
gridY: (row - 0.5) * (SEED_SIZE + INNER_GRID_GAP) + Y_SHIFT, | |
}; | |
} | |
// Ring positioning for seeds 5 and up | |
const ringPosition = seedNumber - 4; // Position in the ring pattern | |
let x = 0, y = 0; | |
if (ringPosition <= 4) { | |
// Left side: 4 seeds | |
x = -(SEED_SIZE + BASE_OFFSET); | |
y = ((ringPosition - 2.5) * (SEED_SIZE + RING_GAP)); | |
} else if (ringPosition <= 6) { | |
// Top: 2 seeds | |
x = ((ringPosition - 5.5) * (SEED_SIZE + RING_GAP)); | |
y = -(SEED_SIZE + BASE_OFFSET); | |
} else if (ringPosition <= 10) { | |
// Right side: 4 seeds | |
x = (SEED_SIZE + BASE_OFFSET); | |
y = ((ringPosition - 8.5) * (SEED_SIZE + RING_GAP)); | |
} else if (ringPosition <= 12) { | |
// Bottom: 2 seeds | |
x = ((ringPosition - 11.5) * (SEED_SIZE + RING_GAP)); | |
y = (SEED_SIZE + BASE_OFFSET); | |
} | |
return { | |
gridX: x + X_SHIFT, | |
gridY: y + Y_SHIFT, | |
audio.pause(); | |
audio.currentTime = 0; | |
}; | |
}; | |
// Get base position based on type and pit number | |
const basePosition = | |
type === "player" | |
? playerPositions[pit_number as keyof typeof playerPositions] | |
: opponentPositions[pit_number as keyof typeof opponentPositions]; | |
}, [volume, audio]); | |
useEffect(() => { | |
audio.volume = volume; | |
if (volume <= 0) { | |
audio.pause(); | |
} | |
return () => { | |
if (timerRef.current) { | |
clearTimeout(timerRef.current); | |
} | |
audio.pause(); | |
audio.currentTime = 0; | |
}; | |
}, [volume, audio]); |
pit_number, | ||
seed_number, | ||
isNative, | ||
volume, |
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.
💡 Codebase verification
Volume prop is unused despite being required
The volume prop is properly typed and received but not implemented:
- The audio volume is hardcoded to 0.35
- Volume control logic is commented out (lines 45-47)
- The prop is in the useEffect dependency array but has no effect
🔗 Analysis chain
Verify volume prop usage.
The volume prop is added but only partially implemented. The commented-out code (lines 45-47) suggests incomplete volume control implementation.
Also applies to: 20-20
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for volume prop usage in parent components
rg -l "volume.*Seed" --type tsx
Length of output: 61
Script:
#!/bin/bash
# Search in TypeScript and TypeScript React files
echo "=== Searching for Seed component usage with volume prop ==="
rg -t ts -t tsx "volume.*Seed|Seed.*volume" -A 2 -B 2
echo -e "\n=== Searching for volume-related code in Seed component ==="
rg -t ts -t tsx "volume" client/src/components/seed.tsx
Length of output: 431
Script:
#!/bin/bash
# Find all potential TypeScript files first
echo "=== Finding TypeScript files ==="
fd "\.(ts|tsx)$"
echo -e "\n=== Searching for Seed component usage ==="
find . -type f -name "*.ts*" -exec grep -l "Seed" {} \;
echo -e "\n=== Checking volume in seed.tsx ==="
if [ -f "client/src/components/seed.tsx" ]; then
cat client/src/components/seed.tsx
fi
Length of output: 6046
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: 4
♻️ Duplicate comments (1)
client/src/components/message-area.tsx (1)
9-10
: 🛠️ Refactor suggestionDefine proper TypeScript types instead of using 'any'.
Using
any
type reduces type safety and bypasses TypeScript's type checking benefits.interface IMessageAreaProps { address: string | undefined; game_players: { mancalaAlphaPlayerModels?: { edges?: Array<{ node?: { address: string; restart_requested?: boolean; }; }>; }; }; - setMessage: any; - setAction: any; + setMessage: (message: string) => void; + setAction: (action: { + action: (() => Promise<void>) | undefined; + message: string; + }) => void; }
🧹 Nitpick comments (2)
client/src/components/message-area.tsx (1)
20-27
: Improve type safety and simplify array operations.While the optional chaining is good, we can improve type safety and readability.
const active_players_addrs = - game_players?.mancalaAlphaPlayerModels?.edges?.map( - (item: any) => item?.node?.address, + game_players?.mancalaAlphaPlayerModels?.edges?.map( + (item: { node?: { address: string } }) => item?.node?.address, ) ?? []; const opponent_position = active_players_addrs.indexOf(address) === 0 ? 1 : 0; const opponent_requested_restart = - game_players?.mancalaAlphaPlayerModels?.edges?.filter( - (item: any) => item?.node?.restart_requested === true, - )[opponent_position]?.node?.restart_requested; + game_players?.mancalaAlphaPlayerModels?.edges?.[opponent_position]?.node?.restart_requested ?? false;client/src/components/header.tsx (1)
125-132
: Improve audio error handling and user feedback.The audio error handling could be improved to provide better user feedback.
audio.addEventListener('ended', () => { audio.play().catch(console.error); }); audio.addEventListener('error', (e) => { console.error('Audio playback error:', e); setIsPlaying(false); + // Provide user feedback + const errorMessage = e.target instanceof HTMLMediaElement + ? `Audio playback error: ${e.target.error?.message || 'Unknown error'}` + : 'Failed to play audio'; + // You can set this to a state variable to display in the UI });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/components/header.tsx
(2 hunks)client/src/components/message-area.tsx
(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
client/src/components/header.tsx
105-105: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (1.9.4)
client/src/components/header.tsx
[error] 201-201: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
client/src/components/message-area.tsx (2)
16-17
: LGTM! Clean props destructuring.
46-52
: LGTM! Well-structured effect hook.The effect hook is well implemented with:
- Complete dependency array
- Clear separation of message and action states
- Proper cleanup of action state when condition is false
client/src/components/header.tsx (2)
103-106
:⚠️ Potential issueMove token address to environment configuration.
The hardcoded token address should be moved to environment variables for better security and maintainability.
- token: "0x04718f5a0fc34cc1af16a1cdee98ffb20c31f5cd61d6ab07201858f4287c938d", + token: process.env.REACT_APP_STRK_TOKEN_ADDRESS,Likely invalid or redundant comment.
🧰 Tools
🪛 Gitleaks (8.21.2)
105-105: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
45-46
: 🛠️ Refactor suggestionRemove type assertions and define proper types.
Using
as any
removes TypeScript's type safety benefits. Define proper types foraccount
andconnector
.-const { account, connector } = useAccount() as any; -const controller = connectors[0] as ControllerConnector; +const { account, connector } = useAccount(); +const controller = connectors[0];Likely invalid or redundant comment.
const restart_game = useCallback(async () => { | ||
if (account) { | ||
await system.restart_game( | ||
account.account, | ||
account as never, | ||
gameId || "", | ||
setRestarted, | ||
opponent_requested_restart, | ||
); | ||
} | ||
}; | ||
}, [account, gameId, system, opponent_requested_restart]); // Add dependencies |
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
Avoid using 'as never' type assertion.
The type assertion as never
is a code smell that bypasses TypeScript's type checking. Consider properly typing the account parameter expected by the system.restart_game function.
const restart_game = useCallback(async () => {
if (account) {
await system.restart_game(
- account as never,
+ account,
gameId || "",
setRestarted,
opponent_requested_restart,
);
}
}, [account, gameId, system, opponent_requested_restart]);
Committable suggestion skipped: line range outside the PR's diff.
useEffect(() => { | ||
const fetchUsername = async () => { | ||
if (!controller) return; | ||
try { | ||
const name = await controller.username(); | ||
setUsername(name || ""); | ||
} catch (error) { | ||
console.error("Error fetching username:", error); | ||
setUsername(""); | ||
} | ||
}; | ||
|
||
const handleDropdownToggleClose = () => { | ||
setIsDropdownClose(!isDropdownClose); | ||
}; | ||
const handleDisconnect = () => { | ||
setIsDropdownClose(!isDropdownClose); | ||
disconnectWallet(); | ||
}; | ||
if (account?.address) { | ||
fetchUsername(); | ||
} else { | ||
setUsername(""); // Reset username when disconnected | ||
} | ||
}, [account?.address, controller]); |
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
Remove duplicate username fetching logic.
The username fetching logic is duplicated in two separate useEffect hooks. Consolidate them into a single hook.
-useEffect(() => {
- const fetchUsername = async () => {
- if (!controller) return;
- try {
- const name = await controller.username();
- setUsername(name || "");
- } catch (error) {
- console.error("Error fetching username:", error);
- setUsername("");
- }
- };
-
- if (account?.address) {
- fetchUsername();
- } else {
- setUsername(""); // Reset username when disconnected
- }
-}, [account?.address, controller]);
-useEffect(() => {
- const fetchUsername = async () => {
- const name = await controller?.username();
- setUsername(name || "");
- };
-
- fetchUsername();
-}, [controller, setUsername]);
+useEffect(() => {
+ const fetchUsername = async () => {
+ if (!controller) return;
+ try {
+ const name = await controller.username();
+ setUsername(name || "");
+ } catch (error) {
+ console.error("Error fetching username:", error);
+ setUsername("");
+ }
+ };
+
+ if (account?.address) {
+ fetchUsername();
+ } else {
+ setUsername(""); // Reset username when disconnected
+ }
+}, [account?.address, controller, setUsername]);
Also applies to: 265-272
const { data, startPolling } = useQuery(MancalaHeaderQuery); | ||
startPolling(1000); |
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
Consolidate polling calls and add cleanup.
Multiple polling calls with the same interval are made throughout the component. This could lead to performance issues and memory leaks.
+const POLL_INTERVAL = 1000;
+
+useEffect(() => {
+ startPolling(POLL_INTERVAL);
+ startPollingPlayerData(POLL_INTERVAL);
+ startPollingProfiles(POLL_INTERVAL);
+
+ return () => {
+ // Add cleanup logic here when available from the API
+ };
+}, [startPolling, startPollingPlayerData, startPollingProfiles]);
-startPolling(1000);
-startPollingPlayerData(1000);
-startPollingProfiles(1000);
Also applies to: 71-73, 187-189
if (event.target.files && event.target.files[0]) { | ||
const file = event.target.files?.[0]; |
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
Improve file handling with optional chaining and add file validation.
The file handling code could be improved with optional chaining and should include file size and type validation.
- const file = event.target.files?.[0];
+ const file = event.target.files?.[0];
+ if (!file) return;
+
+ // Validate file size (e.g., 5MB limit)
+ const MAX_FILE_SIZE = 5 * 1024 * 1024;
+ if (file.size > MAX_FILE_SIZE) {
+ console.error("File size exceeds 5MB limit");
+ return;
+ }
+
+ // Validate file type
+ const ALLOWED_TYPES = ['image/jpeg', 'image/png', 'image/gif'];
+ if (!ALLOWED_TYPES.includes(file.type)) {
+ console.error("Invalid file type");
+ return;
+ }
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 201-201: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Summary by CodeRabbit
Release Notes
New Features
Bubble
component for UI enhancements.Slider
component for volume control.Logout
icon component.CatridgeIcon
component for UI representation.EditPencil
component for editing actions.XIcon
component for close actions.Performance Improvements
UI/UX Enhancements
ErrorFallback
component.Bug Fixes
Breaking Changes
mancala_dev
tomancala_alpha
.