feat: implement Math Marauders wave runner experience #3
Conversation
Summary of ChangesHello @cs-util, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers the complete 'Math Marauders' game, transforming a basic template into an interactive browser-based wave runner. Players guide a flock of soldiers through dynamically generated math gates, engage in skirmishes, and manage a strategic retreat under pressure. The game is built with a lightweight, no-build architecture, utilizing native ES modules, rendering visuals on an HTML5 canvas, and managing the user interface with standard HTML. The changes cover the entire game lifecycle, from procedural content generation and combat mechanics to scoring, data persistence, and event tracking, all supported by a well-defined modular structure and a new testing framework. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an impressive pull request that implements the entire Math Marauders game experience from the ground up. The code is well-structured into modular systems, follows modern JavaScript practices, and includes a solid suite of unit tests. The new README is excellent and provides a clear guide to the architecture and setup. My feedback focuses on improving robustness, maintainability, and fixing a few logic inconsistencies. Key areas for improvement include: ensuring UI components handle missing DOM elements gracefully, resolving a logic bug in wave completion, making game-tuning constants more explicit, and addressing some minor code duplication and naming conventions. Overall, this is a fantastic contribution.
| if (this.phase === PHASES.forward) { | ||
| this.startShowdown(); | ||
| } else { | ||
| this.finishWave(true); |
There was a problem hiding this comment.
There's a logic inconsistency in determining wave victory. When the retreat phase ends, finishWave(true) is called, signaling a victory even if the player has 0 soldiers. However, handleSummaryPrimary recalculates victory as this.playerCount > 0. This leads to a confusing user experience where the summary modal shows a "Next" button (implying victory), but clicking it doesn't advance to the next wave if the player had 0 soldiers.
To fix this, the victory condition should be consistent. The call to finishWave should be based on whether the player has any soldiers left.
| this.finishWave(true); | |
| this.finishWave(this.playerCount > 0); |
| function detectQualityPreset() { | ||
| const isMobile = /Mobi|Android/i.test(navigator.userAgent); | ||
| return isMobile ? "mobile" : "desktop"; | ||
| } |
There was a problem hiding this comment.
The detectQualityPreset function is called and its result is logged via telemetry, but the quality preset itself is never used to configure the game. The QUALITY_PRESETS constant is also defined but remains unused. This appears to be an incomplete feature. The quality preset should be passed to the relevant systems (like FlockSystem) to adjust game visuals and performance as intended.
| constructor() { | ||
| this.waveLabel = document.querySelector("#wave-label"); | ||
| this.phaseLabel = document.querySelector("#phase-label"); | ||
| this.playerCount = document.querySelector("#player-count"); | ||
| this.enemyCount = document.querySelector("#enemy-count"); | ||
| this.gateTitle = document.querySelector("#gate-title"); | ||
| this.optionLeft = document.querySelector("#option-left"); | ||
| this.optionRight = document.querySelector("#option-right"); | ||
| this.slider = /** @type {HTMLInputElement} */ (document.querySelector("#lane-slider")); | ||
| this.advanceButton = document.querySelector("#advance-button"); | ||
| this.playButton = document.querySelector("#play-button"); | ||
| this.logContainer = document.querySelector("#log"); | ||
| this.modal = document.querySelector("#summary-modal"); | ||
| this.modalTitle = document.querySelector("#summary-title"); | ||
| this.modalStars = document.querySelector("#summary-stars"); | ||
| this.modalBody = document.querySelector("#summary-body"); | ||
| this.modalPrimary = document.querySelector("#summary-primary"); | ||
| this.modalSecondary = document.querySelector("#summary-secondary"); | ||
| } |
There was a problem hiding this comment.
The constructor directly assigns the results of document.querySelector to class properties without checking if the elements were actually found. If any of the element IDs change or are removed from index.html, this will lead to TypeError exceptions later when trying to access properties on null. For robustness, you should verify that each element exists after querying it and throw an error if it's not found. This makes debugging much easier.
| this.playerCount = 0; | ||
| this.enemyCount = 0; | ||
| this.optimalFinal = 0; | ||
| this.actualFinal = 0; |
There was a problem hiding this comment.
The class property actualFinal is assigned a value in the constructor, startWave, and startShowdown, but it is never read. This unused property should be removed to simplify the class and avoid confusion.
| handleSummaryPrimary() { | ||
| const victory = this.playerCount > 0; | ||
| this.ui.hideSummary(); | ||
| if (victory) { | ||
| this.waveNumber += 1; | ||
| } | ||
| this.resetForNextAttempt(); | ||
| } | ||
|
|
||
| handleSummarySecondary() { | ||
| this.ui.hideSummary(); | ||
| this.resetForNextAttempt(); | ||
| } |
There was a problem hiding this comment.
The methods handleSummaryPrimary and handleSummarySecondary both contain duplicated calls to this.ui.hideSummary() and this.resetForNextAttempt(). This duplication can be removed by extracting it into a private helper method, improving maintainability.
handleSummaryPrimary() {
const victory = this.playerCount > 0;
if (victory) {
this.waveNumber += 1;
}
this._closeSummaryAndReset();
}
handleSummarySecondary() {
this._closeSummaryAndReset();
}
_closeSummaryAndReset() {
this.ui.hideSummary();
this.resetForNextAttempt();
}| if (distance < obstacle.r + 20) { | ||
| const factor = (obstacle.r + 20 - distance) / (obstacle.r + 20); | ||
| agent.vx += (dx / distance) * factor * 0.8; | ||
| agent.vy += (dy / distance) * factor * 0.8; |
There was a problem hiding this comment.
The avoidObstacles method uses magic numbers for its calculations (e.g., 20 for avoidance buffer and 0.8 for avoidance strength). These should be defined as named constants at the top of the file or in src/core/constants.js to improve readability and make the flocking behavior easier to configure.
const avoidanceRadius = obstacle.r + 20; // TODO: Make 20 a constant
if (distance < avoidanceRadius) {
const factor = (avoidanceRadius - distance) / avoidanceRadius;
const avoidanceStrength = 0.8; // TODO: Make 0.8 a constant
agent.vx += (dx / distance) * factor * avoidanceStrength;
agent.vy += (dy / distance) * factor * avoidanceStrength;| * Advances to the next gate. | ||
| * @returns {{gate: ReturnType<GateSystem['peekGate']>, index: number}} | ||
| */ | ||
| nextGate() { |
There was a problem hiding this comment.
The method name nextGate is misleading, as it doesn't advance the gate index (currentIndex). It only returns the current gate, similar to peekGate. The index is only advanced消费 in resolveGate. Renaming this method to something like getCurrentGate or peekCurrentGate would make its behavior clearer and improve the maintainability of the GateSystem and its consumer, GameController.
| nextGate() { | |
| peekCurrentGate() { |
| } | ||
| } | ||
|
|
||
| export { GateOperation, generateGateSequence }; |
There was a problem hiding this comment.
The GateOperation class is exported from this module but is only used internally. To better encapsulate the module's logic and expose a cleaner public API, you should remove GateOperation from the export statement.
| export { GateOperation, generateGateSequence }; | |
| export { generateGateSequence }; |
| const lines = Array.from(this.logContainer.children); | ||
| if (lines.length > 12) { | ||
| lines.slice(12).forEach((node) => node.remove()); | ||
| } |
There was a problem hiding this comment.
The logic for trimming the log container is slightly inefficient. It creates a new array from this.logContainer.children and then another temporary array with slice() on every single log message. A more performant approach would be to remove excess child nodes directly in a loop without creating intermediate arrays.
while (this.logContainer.childElementCount > 12) {
this.logContainer.lastElementChild?.remove();
}| this.modalStars.textContent = stars | ||
| .split("") | ||
| .map((char, index) => (index < data.stars ? char : "☆")) | ||
| .join(""); |
There was a problem hiding this comment.
The star rating is visually represented, which is great. To improve accessibility for screen reader users, you should also set an aria-label on the modalStars element that describes the rating in text, for example, "3 out of 5 stars".
this.modalStars.textContent = stars
.split("")
.map((char, index) => (index < data.stars ? char : "☆"))
.join("");
this.modalStars.setAttribute("aria-label", `${data.stars} out of 5 stars`);![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| applyChasePressure(playerCount, deltaTime) { | ||
| const speed = this.getEnemySpeed(); | ||
| const damage = (speed / ENEMY_BASE_SPEED) * (deltaTime / 1200); | ||
| const casualties = Math.floor(damage * Math.max(1, playerCount * 0.05)); | ||
| const survivors = Math.max(0, playerCount - casualties); |
There was a problem hiding this comment.
The retreat “pressure” calculation floors casualties each frame, so with typical army sizes the product damage * max(1, playerCount * 0.05) is always < 1 and rounds down to 0. For example, with 64 survivors at 60 fps the expression evaluates to ~0.05, so applyChasePressure returns the original count and the pursuing enemy can never kill any soldiers regardless of how long the chase lasts. As a result, retreat phases cannot fail once a player survives the last forward skirmish. Consider accumulating fractional damage or tracking it across frames instead of flooring each tick.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68e37728ec74832c89d0b7000dc8e5eb