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".
| snapshot.justLiftDefaults?.let { justLiftDefaults -> | ||
| settingsManager.mutateWorkout(snapshot.lease.profileId) { workoutPreferences -> | ||
| workoutPreferences.copy(justLiftDefaults = justLiftDefaults) | ||
| } |
There was a problem hiding this comment.
When snapshot I/O is slower than teardown/summary advancement—particularly when the summary is skipped—handleSetCompletion launches persistSnapshot without awaiting it, while proceedFromSummaryFor can reset to Idle and return to the Just Lift screen. That screen's LaunchedEffect(readyProfileId) reads the persisted defaults only once, so this late write (after session, metric, and biomechanics writes) allows it to reload the old TUT value; the eventual preference update does not rerun the effect because the profile ID is unchanged. Make the captured defaults visible before the setup reload or synchronize that reload with this write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in 52a8078 — pushed a follow-up that closes the race you flagged. Confirmed your read of the runtime path: handleSetCompletion schedules persistSnapshot as a separate coroutine, then the completion job flips WorkoutState to Idle and the navigation observer pops back to JustLiftScreen before the async write completes. LaunchedEffect(readyProfileId) reads getJustLiftDefaults() once on recomposition and never re-runs (profile id unchanged), so the screen captures the stale TUT the user keeps seeing.
Fix: extracted the captured Just Lift defaults write from persistSnapshot into a new internal suspending helper persistCapturedJustLiftDefaultsSnapshot(snapshot) and called it from the isJustLift branch of the completion job, BEFORE resetForNewWorkout() and coordinator._workoutState.value = WorkoutState.Idle. That makes the write visible to the return-to-setup reload on the same execution frame as the state flip. The async persistSnapshot still calls the same helper for retained-snapshot retry / process recovability (idempotent re-write of the same value), so the architecture-review-approved snapshot pattern is preserved — only the timing of the user-facing write is moved.
Regression test: Issue714 Just Lift defaults persist before WorkoutState becomes Idle in skipSummary path in WorkoutExitPersistenceTest. It installs a beforeWorkoutMutation observer and asserts after advanceUntilIdle():
- at least one
MUTATE_BEFORE_IDLEevent (the new synchronous completion-job write), - at least one
MUTATE_AFTER_IDLEevent (the redundant asyncpersistSnapshotwrite), - final persisted value = Old School (not TUT),
WorkoutStateended asIdle.
Without the synchronous write, only MUTATE_AFTER_IDLE events would fire and the test would fail on the first assertion, so this is a real regression check on the ordering rather than a tautology.
Re-checked the architecture review constraints: no rememberSaveable primary fix, no TUT/TUT Beast picker UX change, no portal/Supabase/schema migration, no machine-resistance or iOS autostart work. The fix is in the exact persistence path the architecture review approved (settingsManager.mutateWorkout(snapshot.lease.profileId) on the captured snapshot); only the dispatch timing changed. 🤖 Generated with Hermes Agent
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * Returns null when [params] is not a Just Lift workout. See issue #714. | ||
| */ | ||
| internal fun toJustLiftDefaultsDocumentOrNull(params: com.devil.phoenixproject.domain.model.WorkoutParameters): JustLiftDefaultsDocument? { | ||
| if (!params.isJustLift) return null |
There was a problem hiding this comment.
🔥 The Roast: You imported WorkoutParameters at line 78 like a responsible engineer, then immediately wrote com.devil.phoenixproject.domain.model.WorkoutParameters on line 6699 like you didn't notice. The import is sitting right there, patient, waiting to be used. It's giving "I copied this from a different file and never looked back."
🩹 The Fix:
| if (!params.isJustLift) return null | |
| internal fun toJustLiftDefaultsDocumentOrNull(params: WorkoutParameters): JustLiftDefaultsDocument? { |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
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".
| // return-to-setup reload. Uses the same profile-scoped mutateWorkout | ||
| // and the snapshot's lease profile id, so it cannot read mutable | ||
| // coordinator state after teardown. | ||
| persistCapturedJustLiftDefaultsSnapshot(snapshot) |
There was a problem hiding this comment.
When two Just Lift sets complete before the first persistSnapshot reaches this call, the second completion can synchronously save its newer defaults and then this older coroutine can acquire the preferences mutex afterward and overwrite them with its captured values. Session, metric, and biomechanics writes before this line make that reordering possible, so consecutive sets using different modes can leave the profile reverted to the earlier set; skip this redundant write after successful synchronous persistence or add ordering/version protection.
Useful? React with 👍 / 👎.
| terminalSnapshot?.let { snapshot -> | ||
| if (snapshot.justLiftDefaults != null) { | ||
| persistCapturedJustLiftDefaultsSnapshot(snapshot) | ||
| } |
There was a problem hiding this comment.
If mutateWorkout throws—for example because the profile context is switching/stale or the preferences repository fails—the exception escapes this completion job, so execution never reaches resetForNewWorkout() or the transition to WorkoutState.Idle. This leaves the user stuck after machine teardown rather than merely reporting a persistence failure; catch the write failure and let completion proceed, while relying on the retained snapshot retry path already used by persistSnapshot.
Useful? React with 👍 / 👎.
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".
| terminalSnapshot?.let { snapshot -> | ||
| if (snapshot.justLiftDefaults != null) { | ||
| persistCapturedJustLiftDefaultsSnapshot(snapshot) |
There was a problem hiding this comment.
When mutateWorkout suspends longer than the summary countdown or a quick user dismissal, this write occurs after SetSummary has already been published; proceedFromSummaryFor can independently reset the workout to Idle, returning the user to Just Lift setup while the old defaults are still visible. The follow-up's new ordering is fresh evidence beyond the prior comment: it places the write before this completion job's own Idle assignment, but not before the externally actionable SetSummary transition. Persist before publishing the summary or prevent summary advancement until this write completes.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| // Issue #714: writes the Just Lift defaults captured in the immutable exit |
There was a problem hiding this comment.
🔥 The Roast: You wrote a 9-line essay on a 6-line helper. The function name persistCapturedJustLiftDefaultsSnapshot is already half a documentation page. The rest is just you retelling the PR description to the file with stage directions.
🩹 The Fix: Collapse the comment to the one load-bearing sentence. Something like:
| // Issue #714: writes the Just Lift defaults captured in the immutable exit | |
| // Issue #714: extracted so the completion job can persist Just Lift defaults | |
| // synchronously before the WorkoutState.Idle flip that navigates back to setup. | |
| internal suspend fun persistCapturedJustLiftDefaultsSnapshot(snapshot: WorkoutExitSnapshot) { |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| Logger.d("Just Lift: IMMEDIATE reset for next set (while showing summary)") | ||
|
|
||
| // Issue #714 (Codex P1): JustLiftScreen's return-to-setup reload reads | ||
| // persisted defaults on `LaunchedEffect(readyProfileId)` — once per |
There was a problem hiding this comment.
🔥 The Roast: Eleven lines of commentary on top of five lines of code. The function name plus the // Issue #714: stamp on the helper itself already say everything — this block is the same story told twice, with the dramatic pause included.
🩹 The Fix:
| // persisted defaults on `LaunchedEffect(readyProfileId)` — once per | |
| // Issue #714: write synchronously so the JustLiftScreen reload | |
| // observes fresh defaults before the Idle navigation fires. |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // Old School (or whatever the user picked). The async `persistSnapshot` | ||
| // path still runs the same write idempotently for retained-snapshot | ||
| // retry and process-recovability. | ||
| terminalSnapshot?.let { snapshot -> |
There was a problem hiding this comment.
🔥 The Roast: terminalSnapshot?.let { snapshot -> if (snapshot.justLiftDefaults != null) { ... } } — three layers of null-guarding to invoke a helper that already null-checks the same field. Kotlin's ?. and takeIf were built for this exact moment, and you're queueing behind them in person.
🩹 The Fix:
| terminalSnapshot?.let { snapshot -> | |
| terminalSnapshot?.takeIf { it.justLiftDefaults != null }?.let(::persistCapturedJustLiftDefaultsSnapshot) |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // 1. Seed TUT defaults — the user's original problem state. | ||
| val readyBefore = harness.fakeUserProfileRepo.activeProfileContext.value | ||
| as com.devil.phoenixproject.data.repository.ActiveProfileContext.Ready | ||
| val seededTut = com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument( |
There was a problem hiding this comment.
🔥 The Roast: You imported seventeen types from com.devil.phoenixproject.domain.model at the top of the file like a responsible engineer, then in the new test you typed com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument and com.devil.phoenixproject.domain.model.RepCountTiming like a copy-paste survivor from 2003. The compiler isn't impressed — neither is your future self, who'll be doing this same find-and-replace in every new test.
🩹 The Fix: Add two imports and drop the FQNs:
| val seededTut = com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument( | |
| import com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument | |
| import com.devil.phoenixproject.domain.model.RepCountTiming |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…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>
| // JustLiftScreen's reload sees fresh defaults before any navigation | ||
| // fires; try/catch so a transient prefs failure doesn't strand the | ||
| // user mid-teardown. | ||
| // reads persisted defaults on `LaunchedEffect(readyProfileId)` — once per |
There was a problem hiding this comment.
🔥 The Roast: You applied the suggestion from the previous review by adding the 4-line summary on top of the existing 16-line block instead of replacing the block. The result is 19 lines of comment saying roughly the same thing twice — same over-explained-comment pattern, but now wearing the same costume in two consecutive scenes. The 4-line summary at L11264-11267 is the entire story; this 15-line block is now a redundant director's commentary playing on mute.
🩹 The Fix: Delete the entire old verbose block (lines 11268-11282). The 4-line summary above already carries the sync-before-flip rationale and the try/catch resilience note; the helper's KDoc at ActiveSessionEngine.kt:9036 carries the architectural story. After deletion, the if (isJustLift && terminalSnapshot?.justLiftDefaults != null) { block sits directly under the new summary.
� Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
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