fix: flush _repCount before WORKOUT_COMPLETE handles set completion (#703)

[9thLevelSoftware]

Summary

Fixes off-by-one rep count when ROM reps precede the working set (Issue #703).

Root Cause

When the Nth rep notification triggers WORKOUT_COMPLETE, handleSetCompletion reads coordinator._repCount.value which was still stale (N-1) because the StateFlow write at handleRepNotification line 1334 occurs AFTER repCounter.process() returns.

The race:

1. repCounter.process() updates internal workingReps to N
2. process() fires WORKOUT_COMPLETE via onRepEvent callback
3. Callback launches coroutine -> handleSetCompletion(eventLease)
4. handleSetCompletion reads coordinator._repCount.value -> still N-1
5. captureExitSnapshot persists workingReps = N-1 instead of N

Fix

Flush coordinator._repCount.value = repCounter.getRepCount() inside the WORKOUT_COMPLETE handler before calling handleSetCompletion, ensuring the snapshot sees the correct rep count.

Changes

• ActiveSessionEngine.kt: Added _repCount.value flush in WORKOUT_COMPLETE handler
• WorkoutExitPersistenceTest.kt: Added targeted test for the full repNotification -> handleRepNotification -> WORKOUT_COMPLETE -> handleSetCompletion -> captureExitSnapshot -> persistence path

RCA Reference

• RCA owner: gpt-5.6-terra-xhigh
• RCA model: gpt-5.6-terra
• Architecture review: approved (gpt-5.6-terra)

Fixes #703

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chatgpt-codex-connector]

You have reached your Codex usage limits for security reviews. Please try again later.

[kilo-code-bot]

Code Review Roast 🔥

Verdict: 0 Issues Found | Recommendation: Merge

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	0
🤏 nitpick	0

Incremental Update (commit 5893a27)

The follow-up commit resolves all three findings from the previous review:

1. ✅ ActiveSessionEngine.kt comment — Stale line 1334 breadcrumb removed. The comment now describes the race by behavior (the StateFlow write in handleRepNotification happens after this callback returns) instead of a line number that rots on the next refactor.
2. ✅ WorkoutExitPersistenceTest.kt KDoc — Same stale line-number reference removed; explains the race by behavior.
3. ✅ nowMs() helper — Private nowMs() deleted; both call sites now use harness.nowMs (verified at DWSMTestHarness.kt:70, scheduler-aware: TEST_WALL_CLOCK_EPOCH_MS + testScope.testScheduler.currentTime). One clock, one source of truth, no silent divergence.

No new issues introduced in this incremental commit. The race-condition explanation in ActiveSessionEngine.kt:533-538 is 6 lines for a 1-line behavioral fix, but it documents a genuinely non-obvious StateFlow timing hazard — shrink would risk losing the why, so I'm leaving it.

Ponytail: Lean already. Ship.

---

🏆 Best part: Deleting the private nowMs() helper instead of just changing its body. The author didn't paper over the duplication — they collapsed two clocks into one. That's the correct fix, not a quieter version of the same problem.

💀 Worst part: It took an extra commit to fix the three nits from the previous review. Three small issues, all copy-paste-driven, all caught by the first review, all left in place for a round-trip. The first commit should have shipped clean.

📊 Overall: PR is mergeable. The behavioral fix is correct, the regression test exercises the right path, and the housekeeping is now actually housekept. Two commits to land what should have been one, but the landed diff is solid.

Files Reviewed (2 files)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt - 0 issues
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt - 0 issues

Previous Review Summaries (2 snapshots, latest commit f77aaee)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit f77aaee)

Verdict: 3 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	3
🤏 nitpick	0

Incremental Update (commit f77aaee)

The follow-up commit swaps harness.repNotification(...) for harness.modernRepPacket(...) and adds repsSetTotal = 7 to both rep-emit call sites. Two things worth flagging:

1. This commit is a quiet save. The previous commit (d381652e) introduced a test that referenced harness.repNotification(...) — a helper that does not exist on DWSMTestHarness (verified: only KableBleRepositoryTest's private repNotification(index: Int) matches, and it's a single-arg version with the wrong shape, not a harness member). Without this follow-up, the test wouldn't compile, so this commit silently turns "broken CI" into "compiles." Nice catch, but it also means CI did not catch the original typo — worth a look at why.
2. No new issues introduced. repsSetTotal is required by the modernRepPacket signature (not defaulted), so the new arg is mandatory, not redundant. processModern doesn't actually read repsSetTotal, so it's a no-op semantically — but it's an API requirement, not test bloat.

The three findings from the prior review are unresolved — none of the lines they cite were touched in this incremental commit.

Issue Details (click to expand)

File	Line	Roast
shared/.../ActiveSessionEngine.kt	535	Comment cites "line 1334" — the actual _repCount.value write is at line 1340. Line 1334 is a process() parameter.
shared/.../WorkoutExitPersistenceTest.kt	926	Same wrong "line 1334" reference in the test KDoc — copy-paste from the PR description, never verified.
shared/.../WorkoutExitPersistenceTest.kt	1009	private fun nowMs() duplicates harness.nowMs and silently diverges (returns frozen constant vs. scheduler-aware value).

---

🏆 Best part: The fix itself is dead-on. coordinator._repCount.value = repCounter.getRepCount() before handleSetCompletion is the right place — not in handleRepNotification, not in captureExitSnapshot, but at the exact boundary where the WORKOUT_COMPLETE callback synchronously hands off to the completion handler. Whoever wrote this understood the race instead of papering over it.

💀 Worst part: Three places — two in code, one in the test — confidently cite "line 1334" as where _repCount.value gets written. Line 1334 is isLegacyFormat = notification.isLegacyFormat,. If your breadcrumb is supposed to help future maintainers verify your claim and it points at the wrong line, the breadcrumb is now a trap.

📊 Overall: The follow-up commit is a clean compile-fix; no new findings. The original PR still needs the three documented nits resolved before merge.

Files Reviewed (2 files)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt - 1 issue (unchanged)
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt - 2 issues (unchanged)

Fix these issues in Kilo Cloud

Previous review (commit d381652)

Verdict: 3 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	3
🤏 nitpick	0

Issue Details (click to expand)

File	Line	Roast
shared/.../ActiveSessionEngine.kt	535	Comment cites "line 1334" — the actual _repCount.value write is at line 1340. Line 1334 is a process() parameter.
shared/.../WorkoutExitPersistenceTest.kt	926	Same wrong "line 1334" reference in the test KDoc — copy-paste from the PR description, never verified.
shared/.../WorkoutExitPersistenceTest.kt	1007	private fun nowMs() duplicates harness.nowMs and silently diverges (returns frozen constant vs. scheduler-aware value).

---

🏆 Best part: The fix itself is dead-on. coordinator._repCount.value = repCounter.getRepCount() before handleSetCompletion is the right place — not in handleRepNotification, not in captureExitSnapshot, but at the exact boundary where the WORKOUT_COMPLETE callback synchronously hands off to the completion handler. Whoever wrote this understood the race instead of papering over it.

💀 Worst part: Three places — two in code, one in the test — confidently cite "line 1334" as where _repCount.value gets written. Line 1334 is isLegacyFormat = notification.isLegacyFormat,. If your breadcrumb is supposed to help future maintainers verify your claim and it points at the wrong line, the breadcrumb is now a trap.

📊 Overall: Solid bug-fix PR — the actual logic change is minimal, correct, and well-commented. The new regression test exercises the exact reproduction scenario. The only smell is a private nowMs() helper that reinvents harness.nowMs with a subtly wrong implementation, and two stale line-number references that will rot on the next refactor of handleRepNotification. Fix the three nits and ship.

Files Reviewed (2 files)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt - 1 issue
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt - 2 issues

Fix these issues in Kilo Cloud

---

_{Reviewed by minimax-m3 · Input: 30.1K · Output: 3.3K · Cached: 291.6K}

_{Review guidance: REVIEW.md from base branch main}