Skip to content

Max updates: Game series - Snake#63

Open
BaseMax wants to merge 24 commits intomainfrom
max-updates-games-snake
Open

Max updates: Game series - Snake#63
BaseMax wants to merge 24 commits intomainfrom
max-updates-games-snake

Conversation

@BaseMax
Copy link
Copy Markdown
Member

@BaseMax BaseMax commented Apr 5, 2026

No description provided.

@BaseMax BaseMax requested a review from jbampton as a code owner April 5, 2026 20:22
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
metropolis-retro Ready Ready Preview, Comment Apr 9, 2026 6:12am
metropolis-retro-github-io Ready Ready Preview, Comment Apr 9, 2026 6:12am

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive Snake game implementation using React 19, TypeScript, and HTML5 Canvas. The changes include a new standalone game package with features like difficulty levels, power-ups, and sound effects, along with its integration into the main application. Review feedback highlights the need to correct non-existent dependency versions in package.json, improve state reactivity for active power-ups within the game controller hook, and refine touch event handling to prevent accidental game restarts during UI interactions.

@BaseMax
Copy link
Copy Markdown
Member Author

BaseMax commented Apr 5, 2026

/geminai review

@BaseMax
Copy link
Copy Markdown
Member Author

BaseMax commented Apr 5, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive Snake game implementation, including a custom game engine, React components, and integration into the main application. The game features classic mechanics, difficulty levels, power-ups, and sound effects. Review feedback identifies a critical bug in the collision detection logic where wrapped positions are not re-validated, potentially leading to unintended overlaps. Additionally, several dependency versions in the project's configuration are invalid or non-existent, which must be corrected to ensure the project can be installed and built successfully.

@github-project-automation github-project-automation bot moved this to Backlog in METROPOLIS Apr 6, 2026
@jbampton jbampton moved this from Backlog to In progress in METROPOLIS Apr 6, 2026
@jbampton jbampton added this to the Retro milestone Apr 6, 2026
@socket-security
Copy link
Copy Markdown

socket-security bot commented Apr 6, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updatednpm/​vite@​8.0.8 ⏵ 8.0.394 -678 -2282 -1899 -1100
Updatednpm/​react@​19.2.5 ⏵ 19.2.410010084 -1697 -3100
Updatednpm/​react-dom@​19.2.5 ⏵ 19.2.41001009298100

View full report

@jbampton
Copy link
Copy Markdown
Member

jbampton commented Apr 6, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive, standalone Snake game built with React 19 and HTML5 Canvas, featuring difficulty settings, power-ups, and sound effects. The code review highlights several areas for improvement, including correcting unstable build tool versions in the package configuration and addressing potential audio playback issues caused by browser autoplay policies. Further feedback suggests enhancing code maintainability by removing magic numbers and improving component encapsulation by avoiding direct global DOM manipulations.

@socket-security
Copy link
Copy Markdown

socket-security bot commented Apr 6, 2026

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Warn High
Obfuscated code: npm vite is 91.0% likely obfuscated

Confidence: 0.91

Location: Package overview

From: games/pacman/package-lock.jsonnpm/vite@8.0.3

ℹ Read more on: This package | This alert | What is obfuscated code?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/vite@8.0.3. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@deepsource-io
Copy link
Copy Markdown

deepsource-io bot commented Apr 6, 2026

DeepSource Code Review

We reviewed changes in 13b905c...2cbb0dd on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade  

Focus Area: Reliability
Security  

Reliability  

Complexity  

Hygiene  

Feedback

Edge-case handling around control and state

  • Several issues cluster around “control surfaces”: fullscreen handling, keyboard repeat, touch restart, and async resume all behave correctly in the normal case but break on edge conditions (WebKit fullscreen, key repeat, tap-anywhere, stale async).
  • Might be worth treating input/state transitions as first-class: define explicit allowed transitions and guard them consistently.

Null-safety exception vs overall strictness

  • The codebase is generally strict and type-safe, but there’s a single non-null assertion punching through that contract.
  • With everything else leaning on strict TS, that one escape hatch is more likely to hide a real edge case than in a looser codebase.

Code Review Summary

Analyzer Status Updated (UTC) Details
JavaScript Apr 9, 2026 6:11a.m. Review ↗
Secrets Apr 9, 2026 6:11a.m. Review ↗
Python Apr 9, 2026 6:11a.m. Review ↗

@jbampton jbampton moved this from In progress to In review in METROPOLIS Apr 6, 2026
@BaseMax
Copy link
Copy Markdown
Member Author

BaseMax commented Apr 6, 2026

I am working on AI review...

Comment on lines +30 to +56
export default function SnakePage() {
return (
<div>
<Header />
<main className="pt-24 md:pt-32">
<section className="border-b border-border py-8 md:py-12">
<div className="mx-auto max-w-[1280px] px-6 md:px-12">
<Link
href="/games"
className="mb-6 inline-flex items-center gap-2 text-sm text-muted-foreground transition-colors hover:text-foreground"
>
<ArrowLeft className="h-4 w-4" />
Back to games
</Link>
</div>
</section>

<section className="py-8 md:py-12">
<div className="mx-auto max-w-[1280px] px-6 md:px-12">
<SnakeEmbed />
</div>
</section>
</main>
<Footer />
</div>
)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-level declarations pollute global scope causing collisions


Declaring variables or functions directly in the global scope risks name collisions and unexpected behaviors if other scripts declare identifiers with the same names. This is especially problematic in environments where scripts share the same global context.

Use module scoping or encapsulate declarations within functions or blocks to avoid global pollution and ensure variables are local to the module or intended scope.

@@ -0,0 +1,23 @@
import js from '@eslint/js'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid import statement with non-standard syntax


The statement import js from '@eslint/js' does not conform to standard import syntax and will cause a syntax error, stopping the script from running. This syntax error must be fixed before committing to version control.

Replace the import statement with a valid syntax, such as import * as js from '@eslint/js' or import js from '@eslint/js' if the module supports default exports.

Comment on lines +3 to +9
export default function App() {
return (
<div className="w-full h-full bg-slate-950">
<SnakeGame />
</div>
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicit global variables risk name collisions


Declaring variables or functions at the global level exposes them to the entire runtime environment, risking collisions with other global symbols and causing unpredictable behavior. This is problematic especially in browsers where multiple scripts coexist.
Avoid declaring variables or functions globally; instead use module-scoped declarations, closures, or explicitly attach globals to controlled namespaces like window in browsers to prevent collisions and ensure isolated scopes.

Comment on lines +7 to +26
export function DPad({ onDirection }: DPadProps) {
const fire = (dir: Direction) => (e: React.TouchEvent | React.MouseEvent) => {
e.preventDefault();
onDirection(dir);
};

return (
<div className="grid grid-cols-3 grid-rows-3 gap-1 pointer-events-auto" style={{ width: 180, height: 180 }}>
<span />
<button className={BTN} onTouchStart={fire(Direction.Up)} onMouseDown={fire(Direction.Up)} aria-label="Up">▲</button>
<span />
<button className={BTN} onTouchStart={fire(Direction.Left)} onMouseDown={fire(Direction.Left)} aria-label="Left">◀</button>
<span />
<button className={BTN} onTouchStart={fire(Direction.Right)} onMouseDown={fire(Direction.Right)} aria-label="Right">▶</button>
<span />
<button className={BTN} onTouchStart={fire(Direction.Down)} onMouseDown={fire(Direction.Down)} aria-label="Down">▼</button>
<span />
</div>
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-level variable declaration pollutes global scope in ES module


The fire function declared at line 8 in the ES module is scoped to the module level but still hoisted and visible throughout the module. This can unintentionally pollute the module's scope and cause naming conflicts if multiple modules declare similar names.

Refactor to declare fire inside the event handler or use it in a way that confines its scope more narrowly within the function body or component lifecycle to minimize scope pollution.

Comment on lines +3 to +15
export function GameBoard({ canvasRef, width, height }: GameBoardProps) {
return (
<div className="relative flex items-center justify-center w-full max-w-full overflow-hidden">
<canvas
ref={canvasRef}
width={width}
height={height}
className="rounded-lg border border-slate-700/50 max-w-full h-auto"
style={{ imageRendering: "pixelated" }}
/>
</div>
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-level declarations in `GameBoard` risk global scope pollution


Declaring the GameBoard component at the top-level in a script without proper modularization may leak identifiers into the global scope, causing name collisions or runtime errors if other scripts define the same names. This is mostly an issue in non-module browser scripts where top-level declarations become global.

Use ES module syntax or wrap code in an IIFE to scope declarations locally. Explicitly export components in ES modules to prevent unwanted global exposure.

Comment on lines +11 to +13
export function isInBounds(pos: Position, width: number, height: number): boolean {
return pos.x >= 0 && pos.x < width && pos.y >= 0 && pos.y < height;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-level variables in global scope risk naming collisions


The function isInBounds is declared at the global scope, which can risk name collisions especially in browser environments where multiple scripts share the global namespace. This can cause unintended side effects or runtime conflicts.

Wrap the declaration in a module or an IIFE, or explicitly scope it to avoid polluting the global namespace and prevent collisions with other scripts.

Comment on lines +15 to +20
export function getRandomPosition(width: number, height: number): Position {
return {
x: Math.floor(Math.random() * width),
y: Math.floor(Math.random() * height),
};
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-level declarations risk global namespace collisions


Declaring variables or functions at the top level in non-module script contexts can add them to the global scope, risking name collisions and unpredictable behavior if other scripts define the same names. The function getRandomPosition defined at the top-level may therefore accidentally conflict in non-module environments.
Avoid polluting the global scope by using module syntax, wrapping code in an IIFE, or explicitly attaching variables to a controlled namespace such as window in browsers to isolate them.

Comment on lines +22 to +32
export function getOccupiedPositions(state: GameState): Position[] {
const occupied: Position[] = [...state.snake];
occupied.push(state.food.position);
if (state.powerUp) {
occupied.push(state.powerUp.position);
}
for (const obstacle of state.obstacles) {
occupied.push(obstacle.position);
}
return occupied;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-level variable declarations risk global scope pollution


The variable occupied declared inside the getOccupiedPositions function is safe from polluting the global scope because it is function-scoped, not top-level in the module. However, the warning suggests caution with top-level declarations as they can collide with globals in certain environments.

Ensure all variables intended to be local are declared inside functions or modules to avoid polluting the global scope and prevent runtime collisions.

Comment on lines +34 to +57
export function getSafeRandomPosition(
width: number,
height: number,
occupied: Position[],
maxAttempts = 1000,
): Position | null {
for (let i = 0; i < maxAttempts; i++) {
const pos = getRandomPosition(width, height);
if (!isPositionInList(pos, occupied)) {
return pos;
}
}

for (let y = 0; y < height; y++) {
for (let x = 0; x < width; x++) {
const pos = { x, y };
if (!isPositionInList(pos, occupied)) {
return pos;
}
}
}

return null;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-level declarations risk global scope pollution


Top-level declarations like functions or variables can unintentionally add to the global scope, especially in browser environments, causing conflicts with other scripts and unpredictable behavior. This includes the getSafeRandomPosition function if the file isn't treated as a module.

Use ES modules syntax or wrap declarations in closures to confine scope, preventing global scope pollution and potential conflicts.

return occupied;
}

export function getSafeRandomPosition(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High decision points in `getSafeRandomPosition` reduce maintainability


The function getSafeRandomPosition has cyclomatic complexity exceeding recommended thresholds, indicating many paths and decision points. This complexity makes the function harder to understand, maintain, and increases the chance of bugs.

Break getSafeRandomPosition into smaller helper functions and simplify complex logic to reduce its cyclomatic complexity, improving code clarity and testability.


export default function SnakePage() {
return (
<div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excessive JSX nesting hampers code readability


Deep nesting of JSX elements makes components difficult to read and understand, increasing the likelihood of mistakes during development and maintenance. This impacts the code's clarity and maintainability.

Refactor deeply nested JSX by creating smaller reusable components or passing props to consolidate nested elements into simpler structures.

Comment on lines +7 to +9
export function isPositionInList(pos: Position, list: Position[]): boolean {
return list.some((p) => isPositionEqual(p, pos));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global variable usage risks name collisions


Declaring variables or functions in the global scope risks collisions with other scripts' globals, causing unpredictable runtime behavior. This is especially problematic in browser scripts where global scope is shared.
Use module-scoped variables or immediately-invoked function expressions (IIFE) to contain variables locally within the module or function. This prevents accidental global pollution and keeps the namespace clean.

Comment on lines +19 to +26
const [now, setNow] = useState(0);

useEffect(() => {
if (activePowerUps.length === 0) return;

const id = window.setInterval(() => {
setNow(performance.now());
}, 250);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`useState(0)` causes incorrect `remaining` power-up countdown


GameHUD renders PowerUpIndicator with a stale now value on first frame. Users can see power-up timers jump from a large value to the real duration, which is a reliability/UI correctness bug.
Initialize now with performance.now() or set it immediately before starting setInterval.

Comment on lines +44 to +45
const inFullscreen = Boolean(document.fullscreenElement);
setIsFullscreen(inFullscreen);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`document.fullscreenElement` only check breaks WebKit fullscreen state


toggleFullscreen supports withWebkit.webkitRequestFullscreen, but onFullscreenChange derives state only from document.fullscreenElement. In Safari-style implementations, the component may never mark fullscreen active, so labels, cursor behavior, and exit flow become inconsistent.

Use WebKit-compatible checks/events. Add document.webkitFullscreenElement fallback and subscribe to webkitfullscreenchange alongside fullscreenchange.

@jbampton
Copy link
Copy Markdown
Member

Please resolve the conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

2 participants