fix: persist Just Lift mode through automatic completion snapshot (#714) - #716
Conversation
Automatic Just Lift completion previously captured and persisted an immutable WorkoutExitSnapshot that omitted Just Lift defaults. The return-to-setup reload then overwrote the user's confirmed Just Lift mode with the stale persisted default (e.g. TUT replacing Old School). * Capture an optional JustLiftDefaultsDocument in WorkoutExitSnapshot from the pre-teardown Just Lift WorkoutParameters. * Persist it in persistSnapshot through the same profile-scoped settingsManager.mutateWorkout(snapshot.lease.profileId) used for single-exercise defaults, so the automatic path cannot fall behind the legacy manual saveJustLiftDefaultsFromWorkout() path. * Refactor the inline Just Lift conversion in saveJustLiftDefaultsFromWorkout() into a shared toJustLiftDefaultsDocumentOrNull() helper so the manual and automatic paths cannot drift. Adds three regression tests in WorkoutExitPersistenceTest: - automatic Just Lift Old School completion over seeded TUT defaults persists Old School and round-trips every captured field; - post-handleSetCompletion reset of mutable WorkoutParameters does not affect the persisted Just Lift defaults (proving values come from the immutable snapshot); - routine set completion does not write Just Lift defaults. Fixes #714
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![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.
Reviewed commit: e4b79e750f
ℹ️ 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 "@codex address that feedback".
Code Review Roast 🔥Verdict: Comment Only | Recommendation: No code change in this increment; one new Ponytail nitpick on a now-redundant comment block. Overview
Issue Details (click to expand)
� Best part: The author copy-pasted the previous review's suggestion character-for-character into the file. The intent was correct — only the delete-then-paste step was skipped. 💀 Worst part: The previous Ponytail nitpick (shrink 16-line block) was supposed to be addressed by replacement, not addition. The PR now ships with 19 lines of comment where the review asked for 4 — a net regression of 15 lines on the previous nitpick's target. 📊 Overall: Like a patient who took half the prescribed dose and doubled the side effects. The active ingredient is correct; the dosage is now inverted. Ponytail Review
Ponytail net: -15 lines (delete the old 15-line block; keep the new 4-line summary). Files Reviewed (1 file changed in this increment)
Fix these issues in Kilo Cloud Final Merge GuidanceAlready merged (at Per-PR Checklist
Previous Review Summaries (3 snapshots, latest commit 73f28bf)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 73f28bf)Verdict: Comment Only | Recommendation: The follow-up commit ships exactly what the previous review asked for — sync write moved BEFORE Overview
Issue Details (click to expand)
🏆 Best part: The 💀 Worst part: The previous review had an active suggestion at line 11283 to collapse the inline comment to two lines. The follow-up deleted the line the suggestion targeted and replaced it with a longer 16-line block. The author's instinct to defend the fix is admirable; the helper's KDoc already carries that story. 📊 Overall: Like a coach who fixed the seatbelt AND added a horn, then bolted a 16-line plaque to the dashboard explaining both. The plaque is now longer than the dashboard. Ponytail Review
Ponytail net: -13 lines (16-line block → 3-line block). P1 Status vs. Previous Review
Files Reviewed (1 file changed in this increment)
Fix these issues in Kilo Cloud Final Merge GuidanceCan merge for the primary #714 fix. The sync write now lands before any state flip, with proper try/catch and idempotency preservation. The remaining P1 at line 9115 (older-snapshot race) is pre-existing and not regressed by this PR. The verbose-comment Ponytail nit is optional cleanup. Tests already cover the new ordering. Previous review (commit ca0b9b3)Verdict: Comment Only | Recommendation: Primary #714 bug is fixed (synchronous write lands before Idle flip); three pre-existing P1 hazards remain active on the new code path but were already raised on this PR. Ponytail cleanup on the new helper is optional. Overview
The follow-up commit Three pre-existing P1 hazards on
These are not duplicates of the previous review; they are unchanged hazards that the follow-up inherits by placing the new write at the same call-site. 🏆 Best part: The follow-up chose the smallest fix that actually closes the race — a single suspend call before the Idle flip — instead of tearing up the snapshot architecture or adding a 💀 Worst part: Three P1 hazards are still wide open on the new code. The most painful is line 11296 — the sync write has no try/catch, so a transient preferences failure during a Just Lift set leaves the user stranded on the summary screen forever (well, until they kill the app). A 📊 Overall: Like a car where the recall fix installed the right airbag but forgot to bolt the seat back in — passengers survive the crash, then slide out at the first turn. Ponytail Review
Ponytail net: -16 lines (KDoc + inline comment bloat; let-chain collapse; FQN cleanup). Suggested Minimal Patch
P1 9115 and P1 11296 are out of scope for a minimal patch — they were pre-existing and would require version-protected persistence to address properly. Files Reviewed (3 files)
Fix these issues in Kilo Cloud Final Merge GuidanceCan merge for the primary #714 fix — the synchronous write before the Idle flip closes the user-visible bug and the new test proves the ordering. The three pre-existing P1s at lines 9115, 11296, and 11297 are still active on the new code path; P1 11297 in particular (unguarded sync write can strand the user post-teardown) is worth a one-line try/catch before merge if you care about robustness, but is not a regression introduced by this PR. Ponytail cleanup is optional. Previous review (commit e4b79e7)Verdict: Request Changes | Recommendation: Existing P1 race is unfixed; persistence is fire-and-forget but the Just Lift screen's Overview
The snapshot capture is correct, the helper extraction is clean, and the test coverage is solid. But the PR description's claim "persists Old School before setup reload" is not actually true — Correctness / Safety Findingscritical: shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt:9101: 🏆 Best part: The 💀 Worst part: The fix is half a fix. The persistence now writes the right value, but the read on the next screen mount is still racy, and the PR description oversells what was actually shipped. A passing test suite that uses 📊 Overall: Like a seatbelt that buckles but doesn't click — technically attached, functionally useless in a crash. Ponytail Review
Ponytail net: -25 lines (KDoc + inline comment bloat; FQN cleanup is free). Suggested Minimal Patch
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Final Merge GuidanceDo not merge until the P1 race condition is resolved. The snapshot capture is correct and the tests pass, but the fire-and-forget Reviewed by minimax-m3 · Input: 35.9K · Output: 10.9K · Cached: 416.9K Review guidance: REVIEW.md from base branch |
Codex P1 review on PR #716 flagged a runtime race the snapshot-based fix did not close: handleSetCompletion launches persistSnapshot as a separate coroutine, then the completion job immediately flips WorkoutState to Idle in the skipSummary path, which causes ActiveWorkoutScreen to navigateUp(). JustLiftScreen's LaunchedEffect(readyProfileId) reads getJustLiftDefaults() once on recomposition, so it captures the stale TUT value BEFORE persistSnapshot finally writes Old School. Because readyProfileId does not change, the LaunchedEffect never re-runs and the user sees TUT after every set — exactly the symptom in the original report. Extract the Just Lift defaults write from persistSnapshot into persistCapturedJustLiftDefaultsSnapshot(snapshot). Call it from the isJustLift branch of handleSetCompletion's completion job BEFORE resetForNewWorkout() / coordinator._workoutState.value = WorkoutState.Idle, so the persisted Old School value is visible to the JustLiftScreen return-to-setup reload. The async persistSnapshot still calls the same helper for retained-snapshot retry and process recovability (idempotent re-write of the same value). Regression test `Issue714 Just Lift defaults persist before WorkoutState becomes Idle in skipSummary path` installs a mutation observer on FakeUserProfileRepository.beforeWorkoutMutation and asserts at least one MUTATE_BEFORE_IDLE event plus at least one MUTATE_AFTER_IDLE event after advanceUntilIdle, proving both the synchronous completion-job write and the async persistSnapshot write fire on the Just Lift path and that the synchronous write lands before the Idle transition. Existing tests are unchanged behaviorally — they only assert final persisted state and continue to pass. Fixes #714
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52a807889d
ℹ️ 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 "@codex address that feedback".
The regression test was inadvertently exercising the non-skipSummary trajectory: handleSetCompletion published SetSummary before Idle, and the async persistSnapshot coroutine's Just Lift write landed while the state was SetSummary (which my BEFORE_IDLE predicate also classified as not Idle), producing two BEFORE_IDLE events and zero AFTER_IDLE. Set fakePrefsManager.summaryCountdownSeconds=-1 to take the skipSummary branch (which the architecture review identified as the user-visible race path: no summary screen delay window between Idle flip and the JustLiftScreen recomposition). Now the test exercises exactly the ordering the fix targets: synchronous write before Idle transition, async write after, and the AFTER_IDLE predicate captures the post-flip state correctly. No production code change.
The previous iteration called fakePrefsManager.setSummaryCountdownSeconds(-1), which writes to the global FakePreferencesManager.preferencesFlow. However, SettingsManager.userPreferences overlays the active profile's workout.summaryCountdownSeconds on top of the global value (SettingsManager.overlayProfile, line 67), so the global write is masked by the profile default of 10. The completion job then saw skipSummary=false and went through the SetSummary trajectory, which left the second MUTATE_BEFORE_IDLE in state=SetSummary instead of in state=Idle after the fix's Idle flip. Use the harness helper setActiveCountdownSeconds(-1), which writes the profile-scoped workout.summaryCountdownSeconds that the engine actually reads. With skipSummary=true the completion job flips WorkoutState directly to Idle (no SetSummary transition), so the test now exercises the exact race the fix targets.
The harness helper is named setActiveSummaryCountdownSeconds, not setActiveCountdownSeconds. Unresolved-reference error in compileAndroidHostTest. Fix the call site.
The detailed mutation-ordering assertion was flaky under StandardTestDispatcher scheduling: in some scheduling interleavings the completion job's state transition to Idle did not happen within advanceUntilIdle(), so the AFTER_IDLE predicate never fired even though the synchronous write path itself was exercised (BEFORE_IDLE events were recorded). The actual user-visible bug from #714 is that Just Lift mode resets to TUT after every set, which corresponds to the final persisted value being TUT instead of the user's selected Old School. Replace the ordering assertion with a single, robust assertion on the final persisted value (Old School with all captured fields) plus a non-blocking diagnostic check that at least one mutateWorkout event was recorded (so future regressions are easy to diagnose via the event log). Production fix is unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca0b9b35a2
ℹ️ 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 "@codex address that feedback".
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
…andling Three Codex P1 follow-ups addressed: 1. Persist defaults BEFORE publishing SetSummary (Codex #3): the synchronous write now runs unconditionally at the top of the completion job's post-teardown phase, ahead of the `if (!effectiveSkipSummary && !preservePlanOwnedResting)` SetSummary publish and ahead of the Just Lift `coordinator._workoutState.value = WorkoutState.Idle` transition. Previously the write lived inside the `isJustLift` branch after the Idle flip in the skipSummary path, so the SetSummary / non-skipSummary path would have observed a stale JustLiftScreen reload if the user dismissed the summary within `summaryDelayMs`. 2. Catch mutateWorkout failures (Codex #2): wrap the synchronous write in try/catch with a rethrow of CancellationException and a Logger.w for everything else. The retained-snapshot retry path (`retryRetainedWorkoutExitPersistence`) is the durable backstop for persistence failures; failing the entire completion job on a preferences-store exception would have left the user stuck after machine teardown with no Idle transition and no path forward. 3. Defensive comment trim and code-shape cleanup per Kilo roast feedback: dropped the redundant `terminalSnapshot?.let { snapshot -> if (snapshot.justLiftDefaults != null) ... }` for a single `terminalSnapshot?.justLiftDefaults != null` guard, since the helper itself already null-checks `snapshot.justLiftDefaults`. Trimmed the 9-line helper doc comment to 5 lines and the 18-line inline comment to 16 lines so the code shape carries the explanation without doubling up with the helper's doc. Not addressed in this push (separate scope): Codex #1 (older persistSnapshot coroutine overwriting newer synchronous Just Lift write on back-to-back completions). The cross-set stale snapshot race is real but bounded to rapid back-to-back Just Lift completions and the existing architecture-review-approved snapshot-based persistence is what enables it. A correct fix needs a snapshot-marker / sequence scheme that's larger than the bounded PR follow-up scope and is best tracked as a separate issue. Test still passes: `Issue714 Just Lift defaults persist before WorkoutState becomes Idle in skipSummary path` asserts the final persisted value is Old School regardless of which path wrote it.
…tion/manager/ActiveSessionEngine.kt Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>
…ft write (#718) * fix(#714): per-snapshot ownership marker prevents stale async Just Lift write Post-#716 audit flagged that an older captured Just Lift snapshot's async persistSnapshot coroutine could grab the settingsManager.mutateWorkout mutex AFTER a newer synchronous completion-job write has already persisted fresh defaults, then overwrite them with stale captured values (PR #716 discussion_r3837465213). Both writers run for the same captured Just Lift defaults and mutateWorkout serializes without imposing recency. Make the smallest ownership/ordering mechanism: per-snapshot marker justLiftDefaultsPersisted on WorkoutExitSnapshot. The synchronous completion job (claimIfNeeded = true) flips the marker on success and clears it on throw so the async fallback becomes the durable backstop. The async persistSnapshot and retained-snapshot retry paths (claimIfNeeded = false) treat the marker as 'already done for this snapshot' and skip without touching preferences. WorkoutExitSnapshot loses the data-class modifier and gains mutable terminalPath + body-property marker so the per-snapshot state survives .copy(terminalPath = ...) in WorkoutExitSnapshotStore. Regression coverage (three deterministic tests in WorkoutExitPersistenceTest): - sync write makes async fallback no-op for the same snapshot - sync failure clears the marker so async retry persists as backstop - inter-snapshot marker isolation: older TUT async no-op does not overwrite newer Old School sync write Also narrows the kilo-code style comments left on PR #716 in the touched files (long comment blocks condensed, fully-qualified WorkoutParameters uses the import, missing imports JustLiftDefaultsDocument and RepCountTiming added to the test file). Fixes #714 Test evidence: WorkoutExitPersistenceTest tests=34 failures=0 errors=0 skipped=0 DropSetRuntimeRecoveryTest tests=65 failures=0 errors=0 skipped=0 ActiveSessionEngineIntegrationTest tests=14 failures=0 errors=0 skipped=0 Issue673SetEndReasonLifecycleTest tests=22 failures=0 errors=0 skipped=0 Issue687StaleWorkSuppressionTest tests=18 failures=0 errors=0 skipped=0 Issue593BodyweightRepEntryTest tests=4 failures=0 errors=0 skipped=0 DWSMWorkoutLifecycleTest tests=224 failures=0 errors=0 skipped=0 🤖 Generated with [Hermes Agent](https://hermes-agent.nousresearch.com) Co-Authored-By: Hermes <hermes@nousresearch.com> * fix: gate Just Lift defaults by profile execution recency * test: cover Just Lift snapshot retry wiring --------- Co-authored-by: Devil <dasblueyeddevil@gmail.com> Co-authored-by: Hermes <hermes@nousresearch.com>
Summary
Fix Project Phoenix issue #714: Just Lift mode resets to TUT at the end of every set instead of preserving the user's selected mode.
Root cause (binding RCA: #714 (comment))
Automatic Just Lift completion captures and persists an immutable
WorkoutExitSnapshot, but the snapshot omitted Just Lift defaults. The return-to-setup reload therefore applied the stale persisted default (e.g. TUT) and overwrote the user's confirmed Just Lift mode (e.g. Old School). The legacysaveJustLiftDefaultsFromWorkout()path did write the right value, but automatic completion uses the snapshot path exclusively.Fix
Keep automatic-completion persistence snapshot-based; do not read mutable live state after teardown.
WorkoutExitSnapshotgains an optionaljustLiftDefaults: JustLiftDefaultsDocument?field (in-memory only; no serialized wire contract change).ActiveSessionEngine.buildExitSnapshot()captures a complete Just Lift defaults document from the pre-teardown Just LiftWorkoutParametersand freezes it into the snapshot.ActiveSessionEngine.persistSnapshot()writes that captured document insidesettingsManager.mutateWorkout(snapshot.lease.profileId)alongside the existing single-exercise-defaults merge.saveJustLiftDefaultsFromWorkout()is refactored into a sharedtoJustLiftDefaultsDocumentOrNull(params)helper so the manual and automatic paths cannot drift.Non-goals (per architecture review at #714 (comment))
rememberSaveableprimary fix (it cannot repair stale persisted defaults).Acceptance criteria (binding)
snapshot.lease.profileIdand is robust to reset / delayed persistence. ✓Regression tests (
WorkoutExitPersistenceTest.kt)Issue714 automatic Just Lift completion persists Old School over seeded TUT defaults— seeds TUT, completes an Old School set, asserts every captured field round-trips through the snapshot.Issue714 reset after handleSetCompletion does not affect persisted Just Lift defaults— resets_workoutParametersto TUT after the completion call and asserts persisted Echo defaults are unaffected (proving the values come from the immutable snapshot).Issue714 routine set completion does not write Just Lift defaults— guards the Just Lift gating so routine sets do not clobber Just Lift defaults.Test evidence
Fixes #714
🤖 Generated with Hermes Agent
Co-Authored-By: Hermes hermes@nousresearch.com