Fold countdown module into voting round as private implementation

[spokvulcan]

Closes #219

Move arm, cancel, and evaluate from convex/model/countdown.ts into convex/model/votingRound.ts as private functions. The countdown had exactly one adapter (the voting round module), making it a hypothetical seam per the codebase-design vocabulary.

Changes:

• 3 private helpers folded into votingRound.ts; countdown.ts deleted (70 lines)
• cancel stays as one private helper used from 5 call sites (not inlined)
• Tests: removed standalone Countdown describe blocks; added armCountdown helper exercising the round's public castVote interface
• Net: +106 / -208 lines. All 160 tests pass, typecheck clean.

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
agilekit	Ready	Preview, Comment, Open in v0	Jun 19, 2026 2:23pm

[claude]

Code Review

Overview

This PR eliminates a single-consumer module (countdown.ts) by folding its three private helpers into votingRound.ts. The justification is solid — a seam with exactly one adapter is speculative abstraction, and collapsing it reduces indirection without any loss of clarity. Net: −102 lines, all 160 tests pass.

---

What's Good

• Correct privacy model. The three functions are now truly module-private (unexported), which is exactly right. Nothing outside votingRound.ts needs to call arm, cancel, or evaluate directly.
• Doc comments are high quality. The WHY is preserved on all three helpers — the token-idempotency invariant on arm, the safety semantics on cancel, and the all-voted predicate on evaluate. These would be easy to drop in a mechanical move.
• Test migration to the public interface is the right call. Replacing direct Countdown.arm calls with the armCountdown helper (which drives castVote) tests the same code paths through observable behavior rather than internal implementation details.

---

Concerns

1. A defensive edge case lost test coverage (moderate)

The removed Countdown.evaluate suite included:

it("does not arm a room with no non-spectator members (empty is not all-in)", ...)

The test comment explained why this exists: [].every() is vacuously true, so a spectator-only room could otherwise auto-reveal a round nobody voted in. castVote now refuses spectator ballots, so this path can't be hit through the current public API — but the defensive guard in Votes.areAllVotesIn still exists and could drift. Consider adding a note in the evaluate function comment that this guard defends against legacy data, or add a direct model-layer test using t.run to call evaluate after seeding a spectator-only room with raw votes (the way the old deleteVote helper bypassed the API).

2. armCountdown helper has an implicit invariant (minor)

async function armCountdown(t, roomId, voterIds) {
  for (const userId of voterIds) {
    await t.run(ctx => VotingRound.castVote(ctx, { roomId, userId, ... }))
  }
}

This only arms the countdown if voterIds is exactly the set of all non-spectator members (so areAllVotesIn returns true). If a future test passes a partial list, the countdown silently won't arm and the test will fail in a confusing way. A one-line comment stating this pre-condition would make misuse easier to spot: // voterIds must be ALL non-spectator members in the room to trigger arming.

3. cancelCountdown is now a one-liner wrapper (observation, not a blocker)

export async function cancelCountdown(ctx, roomId) {
  await cancel(ctx, roomId);
}

This is fine as a named public-API entry point, but it's now semantically identical to calling cancel directly. If cancelCountdown is meant to stay in the public surface as a named facilitator action, keeping it is correct. Just noting that the wrapper's body is now trivial.

---

Summary

The refactoring is well-motivated and cleanly executed. Two actionable items:

1. Either add a raw t.run-based test for the spectator-only room guard in evaluate, or add a comment to the evaluate function acknowledging the coverage gap is intentional (since the public API now prevents that path from being triggered normally).
2. Document the pre-condition on armCountdown with a one-line comment.

Neither is a blocker, but (1) in particular is a defensive behavior that the original author took care to document — it would be a shame to lose the signal entirely.

[claude]

---

Code Review

Overview

This PR folds convex/model/countdown.ts into convex/model/votingRound.ts as three private functions (arm, cancel, evaluate), deleting the file entirely. The motivation is solid: the module had exactly one caller and no polymorphic seam, so the boundary was overhead rather than structure. Net −82 lines of production code.

---

What works well

• Correct encapsulation direction. Making arm/cancel/evaluate private prevents other modules from accidentally coupling to countdown internals. Previously they were exported and reachable.
• Test philosophy improvement. Replacing Countdown.arm direct calls with armCountdown (which drives through castVote) is strictly better: it tests the same invariant while also exercising validation, auth guards, and real state transitions. Tests that drove private functions directly were fragile.
• Preserved safety semantics. The token-based stale-job guard, idempotency of arm, and atomicity of cancel (clearing both fields together) all survive the move unchanged.
• New edge-case test. The dropVoter "stray spectator votes" test replaces the old Countdown.evaluate coverage of the vacuously-true [].every() bug — and does it through the public interface, which is better.

---

Concerns

1. cancelCountdown is a one-liner wrapper — consider whether it needs phase validation

cancel is private; cancelCountdown is exported. This is correct as a named public action if the API layer calls it. Worth checking whether it should assert the room is actually in countingDown phase before proceeding. At the moment the action silently no-ops when called outside the expected phase, which may or may not be intentional.

2. Evaluate-specific behaviours have no direct test coverage anymore

The removed Countdown.evaluate describe block had four distinct cases: (a) arms when all non-spectators voted, (b) does not arm when a non-spectator is still missing a vote, (c) does not arm on a spectator-only room (vacuously-true guard), (d) cancels when a voter retracts.

The spectator-only guard is now covered via dropVoter. The "arms when all-in" path is exercised by every test that calls armCountdown. But the "cancels when voter retracts" path is only covered if there is a retractVote describe block elsewhere in the file. Worth confirming one explicit retractVote → countdown cancelled test is still present.

3. armCountdown helper behaviour may surprise future test authors

This helper only arms the countdown if voterIds contains all non-spectator members in the room (so areAllVotesIn returns true on the last iteration). If a test adds a member and forgets to include them in voterIds, the countdown will not arm and the test fails with a confusing assertion. A short comment — "caller must pass all non-spectator member IDs" — would prevent confusion.

---

Nits

• The module-level doc comment on the old countdown.ts ("move as one unit, never half-torn-down") is gone. The per-function JSDoc comments are preserved but the higher-level rationale for atomicity has been lost. Consider carrying one sentence of that reasoning into the cancel JSDoc.
• convex/_generated/api.d.ts: the model/countdown removal is correct and expected for this project. No action needed.

---

Summary

Clean, well-motivated refactor. Two things worth a look before merge: (a) confirm there is still a retractVote → countdown cancelled test path somewhere in the file, and (b) add a caller note to armCountdown. Everything else is in good shape.

🤖 Generated with Claude Code