Conversation
WalkthroughThis PR refactors the Battleship game from a monolithic architecture into a modular ES6 structure with separate concerns (Game, Board, Ship, AI, UI). Includes comprehensive CSS redesign with design tokens, modern layout patterns, and interactive visual effects. Adds repository guidelines and updates the build configuration. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant battleship.js
participant BattleshipGame
participant Board
participant Ship
participant UIController
User->>battleship.js: selectShip(size)
battleship.js->>BattleshipGame: selectShip(size)
BattleshipGame->>UIController: updateShipCount(counterId, remaining)
User->>battleship.js: previewPlayerPlacement(index)
battleship.js->>BattleshipGame: previewPlayerPlacement(index)
BattleshipGame->>Board: previewPlacement(length, startIndex, orientation)
Board-->>BattleshipGame: validity check
BattleshipGame->>UIController: showPlacementPreview(cells, isValid)
User->>battleship.js: clickTarget(index)
battleship.js->>BattleshipGame: placePlayerShip(index)
BattleshipGame->>Board: placeShip(length, startIndex, orientation)
Board->>Ship: recordHit/tracking
BattleshipGame->>UIController: renderShip(board, cells)
UIController->>UIController: Update DOM with ship visual
User->>battleship.js: clickStartGame()
battleship.js->>BattleshipGame: startGame()
BattleshipGame->>BattleshipGame: deployCpuFleet()
BattleshipGame->>UIController: prepareForBattle()
sequenceDiagram
actor User
participant battleship.js
participant BattleshipGame
participant Board
participant AIController
participant UIController
User->>battleship.js: clickCpuBoard(index)
battleship.js->>BattleshipGame: playerAttack(index)
BattleshipGame->>Board: receiveAttack(index)
Board-->>BattleshipGame: result (hit/miss/sunk)
BattleshipGame->>UIController: renderHit/renderMiss(board, index)
alt Not all CPU ships sunk
BattleshipGame->>BattleshipGame: cpuTurn()
BattleshipGame->>AIController: nextAttack()
AIController-->>BattleshipGame: attack index
BattleshipGame->>Board: receiveAttack(index)
Board-->>BattleshipGame: result
BattleshipGame->>AIController: handleAttackResult(index, result)
BattleshipGame->>UIController: renderHit/renderMiss(board, index)
else All CPU ships sunk
BattleshipGame->>UIController: showDefeat()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff encompasses substantial architectural refactoring with six new interdependent modules, comprehensive CSS redesign with multiple systems (variables, layout, components, responsiveness), and complete rewrite of the main entry point. While individual module changes are relatively straightforward (each focuses on a specific concern), the heterogeneity—spanning game logic, rendering, styling, and configuration—combined with the critical nature of the game loop requires careful verification of state management, attack/placement logic, and UI synchronization across all components. Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
AGENTS.md (1)
11-12: Call out event delegation in JS guidelines.Suggest adding a short note recommending event delegation for board hover/preview handlers to avoid per-cell listeners. This aligns with the modular ES setup and improves perf.
battleship.js (2)
40-55: Delegate board hover events to the table to cut 200+ listeners.Use a single mouseover + one mouseleave on the table instead of per-cell enter/leave. Behavior unchanged; less overhead.
-const playerSquares = document.querySelectorAll(".player-board .square"); -playerSquares.forEach((square) => { - square.addEventListener("mouseenter", () => { - game.previewPlayerPlacement(Number(square.id)); - }); - square.addEventListener("mouseleave", () => { - game.clearPlacementPreview(); - }); -}); - -const playerBoardTable = document.querySelector(".player-board table"); -if (playerBoardTable) { - playerBoardTable.addEventListener("mouseleave", () => { - game.clearPlacementPreview(); - }); -} +const playerBoardTable = document.querySelector(".player-board table"); +if (playerBoardTable) { + playerBoardTable.addEventListener("mouseover", (e) => { + const cell = e.target.closest(".square"); + if (cell && playerBoardTable.contains(cell)) { + game.previewPlayerPlacement(Number(cell.id)); + } + }); + playerBoardTable.addEventListener("mouseleave", () => { + game.clearPlacementPreview(); + }); +}
34-36: Optional: Namespace the bridge to avoid global key collisions.Phase-2 improvement: expose window.battleship = bridge and update inline handlers to call battleship.clickTarget(...). Keeps globals tidy. Not blocking.
index.html (1)
31-36: Move away from inline onclick handlers (follow-up).Inline handlers work via the bridge, but migrating to addEventListener with delegation will improve security and maintainability. Applies to selection controls and both boards. Non-blocking for this PR.
Also applies to: 41-46, 49-57, 61-196, 202-335
src/constants.js (1)
15-18: Freeze ORIENTATION to prevent accidental mutation.Minor hardening; avoids bugs from accidental writes.
-export const ORIENTATION = { - HORIZONTAL: "horizontal", - VERTICAL: "vertical", -}; +export const ORIENTATION = Object.freeze({ + HORIZONTAL: "horizontal", + VERTICAL: "vertical", +});src/ship.js (1)
4-9: Validate constructor inputs to enforce invariants.Guard against mismatched length/cells or non-iterables to prevent subtle bugs.
export class Ship { constructor(length, cells) { + if (!Number.isInteger(length) || length <= 0) { + throw new Error("Ship.length must be a positive integer"); + } + if (!cells || typeof cells[Symbol.iterator] !== "function") { + throw new Error("Ship.cells must be an iterable of indices"); + } + const unique = Array.from(new Set(cells)); + if (unique.length !== length) { + throw new Error("Ship.cells size must equal Ship.length"); + } this.id = `ship-${shipIdCounter++}`; this.length = length; - this.cells = new Set(cells); + this.cells = new Set(unique); this.hits = new Set(); }src/game.js (1)
198-198: Simplify redundant victory condition check.The condition
playerHits >= this.totalShipCells || this.cpuBoard.allShipsSunk()is redundant because if all ship cells are hit, then by definition all ships are sunk. You can simplify this to justthis.cpuBoard.allShipsSunk().Apply this diff:
- if (this.playerHits >= this.totalShipCells || this.cpuBoard.allShipsSunk()) { + if (this.cpuBoard.allShipsSunk()) { this.gameStarted = false; this.ui.showVictory(); this.ui.setMessage("Congratulations! You sank the fleet."); return; }The same applies to line 237 in
cpuTurn().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
AGENTS.md(1 hunks)battleship.css(1 hunks)battleship.js(1 hunks)index.html(3 hunks)src/ai-controller.js(1 hunks)src/board.js(1 hunks)src/constants.js(1 hunks)src/game.js(1 hunks)src/ship.js(1 hunks)src/ui-controller.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/board.js (2)
src/constants.js (4)
BOARD_SIZE(1-1)BOARD_SIZE(1-1)ORIENTATION(15-18)ORIENTATION(15-18)src/ship.js (1)
Ship(3-24)
src/game.js (3)
src/constants.js (6)
SHIP_DEFINITIONS(3-8)SHIP_DEFINITIONS(3-8)ORIENTATION(15-18)ORIENTATION(15-18)TOTAL_SHIP_CELLS(10-13)TOTAL_SHIP_CELLS(10-13)src/board.js (1)
Board(4-142)src/ai-controller.js (1)
AIController(1-59)
battleship.js (3)
src/ui-controller.js (1)
UIController(1-117)src/game.js (1)
BattleshipGame(14-271)src/constants.js (2)
ORIENTATION(15-18)ORIENTATION(15-18)
🔇 Additional comments (30)
index.html (1)
350-351: LGTM: Module entry point.type="module" at end of body is correct; imports resolve and DOM is ready.
battleship.js (1)
1-7: ****The review comment identifies a mismatch that does not exist. The script output confirms the constructor calls
this.initializeState()on line 17 and the method is defined asinitializeState()on line 20—both correctly use camelCase. No fix is needed; the code is correct.Likely an incorrect or invalid review comment.
src/ai-controller.js (4)
1-10: LGTM!The constructor and reset logic are clean and correctly initialize the AI state.
12-20: LGTM!The attack prioritization logic correctly processes the focus queue first before falling back to random selection.
22-33: LGTM!The result handling correctly implements a "hunt and target" AI strategy by enqueueing neighbors on hits and resetting focus when a ship is sunk.
35-58: LGTM!The neighbor enumeration correctly computes cardinal directions with proper bounds checking and deduplication.
src/board.js (6)
4-15: LGTM!The constructor and reset logic properly initialize the board state with appropriate data structures.
17-29: LGTM!The coordinate conversion logic is mathematically correct and the bounds checking properly validates grid boundaries.
31-48: LGTM!The bounds checking and cell generation logic correctly handles both orientations and prevents out-of-bounds placements.
50-76: LGTM!The placement logic correctly validates bounds and overlap, and properly maintains both the ship registry and occupied cells tracking.
78-121: LGTM!The preview and attack mechanics correctly handle state queries and mutations with appropriate return values for UI feedback.
123-141: LGTM!The random selection and victory condition checks are implemented correctly with proper edge case handling.
src/ui-controller.js (6)
1-10: LGTM!The constructor properly initializes DOM references with a testable doc parameter. The lack of null checks after queries is acceptable if the HTML structure is guaranteed.
12-27: LGTM!The message methods safely use
textContentand include appropriate null checks.
29-38: LGTM!The battle preparation logic correctly manages DOM elements and CSS classes.
54-58: LGTM!Clearing content with
innerHTML = ""is acceptable for this use case.
60-88: LGTM!The placement preview logic correctly manages CSS classes and tracks state for efficient cleanup.
112-116: LGTM!The element lookup logic correctly handles different ID conventions for player and CPU boards.
src/game.js (7)
1-12: LGTM!The imports and computed constants are well-structured and correct.
14-42: LGTM!The initialization logic properly sets up all game state and synchronizes with the UI.
44-73: LGTM!The ship selection logic correctly validates inventory and provides clear user feedback.
75-155: LGTM!The placement preview and execution logic correctly handle validation, state updates, and UI synchronization.
157-177: LGTM!The game start logic correctly validates ship placement and transitions to battle mode.
208-242: LGTM!The CPU turn logic correctly handles AI attacks with a safety fallback for edge cases. The while loop is safe because
randomUnattackedIndex()returns null when no cells are available.Note: Line 237 has the same redundancy as mentioned in the
playerAttackreview.
244-270: LGTM!The CPU fleet deployment correctly attempts random placements with retry logic. The while loop is acceptable because the ship definitions are designed to fit within the board size.
battleship.css (5)
1-52: LGTM!The CSS custom properties provide a solid design token system, and the base styles are well-structured.
54-227: LGTM!The layout and component styles use modern CSS Grid and Flexbox patterns with consistent interactive states.
228-342: LGTM!The game board styling creates a cohesive visual experience with clear interactive feedback through hover and preview states.
468-645: LGTM!The announcement UI, animations, and responsive media queries create a polished user experience across different screen sizes.
443-443: SVG assets are present in the repository.Both
explosion.svgandsplash-1.svgexist in theimages/directory. The CSS references are valid and will function correctly.Likely an incorrect or invalid review comment.
| showVictory() { | ||
| if (this.announcementEl) { | ||
| this.announcementEl.innerHTML = | ||
| "<span class='announcement'><br/>YOU WIN!</span><span onclick='location.reload()' class='retry'>RETRY</span>"; | ||
| } | ||
| } | ||
|
|
||
| showDefeat() { | ||
| if (this.announcementEl) { | ||
| this.announcementEl.innerHTML = | ||
| "<span class='announcement'><br/>YOU LOSE!</span><span onclick='location.reload()' class='retry'>RETRY</span>"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace innerHTML and inline event handlers with safer alternatives.
Using innerHTML with inline onclick handlers is a security anti-pattern that violates Content Security Policy (CSP) and creates maintenance issues.
Apply this diff to use safer DOM manipulation:
showVictory() {
if (this.announcementEl) {
- this.announcementEl.innerHTML =
- "<span class='announcement'><br/>YOU WIN!</span><span onclick='location.reload()' class='retry'>RETRY</span>";
+ this.announcementEl.innerHTML = "";
+ const announcement = this.doc.createElement('span');
+ announcement.className = 'announcement';
+ announcement.innerHTML = '<br/>YOU WIN!';
+ const retry = this.doc.createElement('span');
+ retry.className = 'retry';
+ retry.textContent = 'RETRY';
+ retry.addEventListener('click', () => location.reload());
+ this.announcementEl.append(announcement, retry);
}
}
showDefeat() {
if (this.announcementEl) {
- this.announcementEl.innerHTML =
- "<span class='announcement'><br/>YOU LOSE!</span><span onclick='location.reload()' class='retry'>RETRY</span>";
+ this.announcementEl.innerHTML = "";
+ const announcement = this.doc.createElement('span');
+ announcement.className = 'announcement';
+ announcement.innerHTML = '<br/>YOU LOSE!';
+ const retry = this.doc.createElement('span');
+ retry.className = 'retry';
+ retry.textContent = 'RETRY';
+ retry.addEventListener('click', () => location.reload());
+ this.announcementEl.append(announcement, retry);
}
}🤖 Prompt for AI Agents
In src/ui-controller.js around lines 40 to 52, avoid using innerHTML and inline
onclick handlers; instead clear announcementEl (e.g., announcementEl.textContent
= '') and build DOM nodes with document.createElement: create a span for the
announcement text (use textContent, set className='announcement' and insert a br
element if needed) and a button/span for retry (className='retry'), append them
to announcementEl, and attach a click event listener with addEventListener that
calls location.reload(); apply the same pattern for both showVictory and
showDefeat.
| renderSquare(board, index, className) { | ||
| const el = this.getSquareElement(board, index); | ||
| if (!el) { | ||
| return; | ||
| } | ||
| el.innerHTML = `<div class='${className}'></div>`; | ||
| } |
There was a problem hiding this comment.
Replace innerHTML with safer DOM manipulation.
Using innerHTML with template literals creates a potential XSS vector if the className parameter is ever sourced from user input (though currently it's hardcoded).
Apply this diff:
renderSquare(board, index, className) {
const el = this.getSquareElement(board, index);
if (!el) {
return;
}
- el.innerHTML = `<div class='${className}'></div>`;
+ el.innerHTML = "";
+ const div = this.doc.createElement('div');
+ div.className = className;
+ el.appendChild(div);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/ui-controller.js around lines 104 to 110, replace the innerHTML usage in
renderSquare with safe DOM manipulation: instead of building a string, create a
new div element, assign its class via elClass.className = className (or
elClass.classList.add(...) if multiple classes), clear existing children of el
(e.g., removeChild loop or el.textContent = '') and append the created element
with el.appendChild(elClass); keep the existing null check and ensure className
is not inserted into HTML directly to avoid XSS.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
1 participant
code rabbit test 2
Summary by CodeRabbit
Release Notes
New Features
Style
Documentation