game winner deactivates doomclockExecution.

[JB940]

Before opening a PR: discuss new features on Discord first, and file bugs or small improvements as issues. You must be assigned to an approved issue — unsolicited PRs will be auto-closed.

Add approved & assigned issue number here:

Resolves #4803

Description:

Instead of keeping the UI active, doomsdayclock is disabled. 1 line of core change plus an improvement change to let it waste execution doing nothing.

Reasoning: The sandbox afterwards is fun and should be preserved, vast majority of players don't play doomsday for the "vibe" but to avoid excessive stalemates, the sandbox after should still be fun.

Please complete the following:

• I have added relevant tests to the test directory

Please put your Discord username so you can be contacted if a bug or regression is found:

JB940

[coderabbitai]

Walkthrough

The Doomsday Clock now stops when a winner exists. It clears all contenders’ clock state, deactivates the execution, and includes test coverage for this behavior.

Changes

Doomsday Clock shutdown

Layer / File(s)	Summary
Winner detection and clock cleanup src/core/execution/DoomsdayClockExecution.ts, tests/DoomsdayClockExecution.test.ts	The execution checks for a winner, clears Doomsday Clock flags, and becomes inactive. FakeGame exposes winner state, and tests verify cleanup after a win.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: zixer1

Poem

A winner appears,
The clock clears every flag,
Its ticking work now rests.
The game moves on in peace,
No doomsday shadows linger.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The implementation stops Doomsday Clock execution after a winner, but [#4803] explicitly expects the UI to remain active.	Keep the Doomsday Clock UI active after a win while preventing its effects from continuing, and add a test for that behavior.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states that a game winner deactivates Doomsday Clock execution, matching the core change.
Description check	✅ Passed	The description directly explains disabling Doomsday Clock after a winner and references issue #4803 and tests.
Out of Scope Changes check	✅ Passed	All code and test changes concern Doomsday Clock behavior after a winner, so no unrelated changes are present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/core/execution/DoomsdayClockExecution.ts (1)

120-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Clear rotState when the execution stops.

If inactive executions remain attached to the game during the post-game sandbox, this branch leaves DoomsdayClockExecution.rotState populated. Its front maps can retain many tile references after no future tick can use them. Clear the map before deactivation, or verify that the execution object is discarded immediately.

Proposed cleanup

    if (mg.getWinner() !== null || sides.length < 2) {
      for (const p of contenders) p.clearDoomsdayClock();
+     this.rotState.clear();
      this.active = false;
      return;
    }

This follows the PR objective to preserve the post-game sandbox while stopping Doomsday Clock work.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/execution/DoomsdayClockExecution.ts` around lines 120 - 124, Update
the early-stop branch in DoomsdayClockExecution so it clears rotState, including
its front maps and retained tile references, before setting active to false.
Preserve the existing contender cleanup and ensure stopped executions do not
retain rotation state if they remain attached to the game.

tests/DoomsdayClockExecution.test.ts (1)

374-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert that the execution becomes inactive.

The test title says it halts execution, but the assertions only check player flags. A regression that removes this.active = false still passes. Add this assertion:

Proposed assertion

    expect(a.inDoomsdayClock()).toBe(false);
    expect(b.inDoomsdayClock()).toBe(false);
+   expect(exec.isActive()).toBe(false);

The PR objective includes deactivating the execution after a winner is set.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/DoomsdayClockExecution.test.ts` around lines 374 - 384, Update the test
case around the execution created by makeExec to assert that the execution is
inactive after game.winnerPlayer is set and the subsequent runAt call processes
the winner. Keep the existing assertions verifying both players’ doomsday clock
flags are cleared.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/DoomsdayClockExecution.test.ts`:
- Around line 197-208: Move the winner-state scenario from the partial
FakeGame/twoPlayerGame path into setup() from tests/util/Setup.ts, using
appropriate map data from tests/testdata/maps/. Exercise the real game
simulation and remove the FakeGame winner additions and related setup so the
core test no longer relies on the partial fake.

---

Nitpick comments:
In `@src/core/execution/DoomsdayClockExecution.ts`:
- Around line 120-124: Update the early-stop branch in DoomsdayClockExecution so
it clears rotState, including its front maps and retained tile references,
before setting active to false. Preserve the existing contender cleanup and
ensure stopped executions do not retain rotation state if they remain attached
to the game.

In `@tests/DoomsdayClockExecution.test.ts`:
- Around line 374-384: Update the test case around the execution created by
makeExec to assert that the execution is inactive after game.winnerPlayer is set
and the subsequent runAt call processes the winner. Keep the existing assertions
verifying both players’ doomsday clock flags are cleared.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 58689265-c694-4fed-8588-2a72035ac3ef

📥 Commits

Reviewing files that changed from the base of the PR and between 5abc519 and 19d0d9b.

📒 Files selected for processing (2)

• src/core/execution/DoomsdayClockExecution.ts
• tests/DoomsdayClockExecution.test.ts

[JB940]

@coderabbitai Your second nitpick will be implemented. Re the first nitpick: TileRef is a primitive scalar number, not an object or class instance. It is does not prevent GC clean-up and when DoomsdayclockExecution is deactivated, the GameImpl filters it out in the next tick, and it is all available for clean-up. It is a tick late and I agree your nitpick is valid, but it is extremely minor.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@JB940 Thanks for the clarification. TileRef does not retain an object, and GameImpl removes the inactive execution on the next tick. The one-tick delay has no meaningful garbage-collection impact. I withdraw the first nitpick.

The planned change for the second nitpick is sufficient.

🐇

_{You are interacting with an AI system.}