fix: preserve Just Lift auto-start session - #759
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Code Review Roast 🔥Verdict: Comment only | Recommendation: Optional cleanup, then ship Overview
Correctness / Safety FindingsThe current commit fixes a real second-order false-positive in One nitpick (already posted inline at L41): Ponytail Review
val effectBlocks = Regex("""LaunchedEffect\(workoutState\)\s*\{((?:[^{}]|\{[^{}]*\})*)\}""").findAll(source)Optional — the current code is readable enough. The split/takeWhile approach is fine for the current codebase's style. Ponytail net: -3 lines (optional, if you collapse to a regex). Suggested Minimal PatchOptional: replace lines 39-47 with the one-level-nesting regex above. Otherwise no patch required. Final Merge GuidanceCan merge as-is. The fix is correct against the current source, the test guards the realistic bug shape, and the remaining nitpick is a defensive hardening that's not required to land this PR. 🏆 Best part: You correctly recognized that the previous test was structurally wrong (it banned 💀 Worst part: The replacement heuristic has the same false-negative flavor as the bug it replaced — just a different shape. Two iterations of string-matching tests in a row is a smell. If Kotlin had a real parser in stdlib I'd say use it; until then, brace-aware regex (or accept the limitation) is the honest fix. 📊 Overall: Like a chain of small bandage fixes that each cover the last one's blind spot. The PR is good enough to ship; the test brittleness is a project-wide smell worth a future refactor, not a blocker for #756. Files Reviewed (1 file in this increment)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit de20495)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit de20495)Verdict: Approve | Recommendation: Merge Overview
Correctness / Safety FindingsNo correctness or safety findings. The current commit is a surgical bug fix to the The previous assertion used a two-conjunct This addresses the outstanding Codex P1 inline comment (id 3941883631, now outdated, line: null) that flagged exactly this false-positive string-match bug. Ponytail Review
Ponytail net: -8 lines (optional, unchanged from previous review). Suggested Minimal PatchNo patch required. The current commit fixes a real CI-breaking test bug, simplifies the assertion, and is itself a Ponytail shrink. Final Merge GuidanceCan merge. The previous P1 blockers are closed, the current commit fixes a self-inflicted test bug that would have broken CI on first run, and tests #1 and #2 pin the actual structural contract. 🏆 Best part: Fixing your own test bug instead of papering over it. The old assertion was an 💀 Worst part: Test #3 is still alive. 📊 Overall: The merge-gate fix is solid, the current commit is a textbook shrink-with-bug-fix, and the only remaining smell is a tautological third test that has overstayed its welcome by one review cycle. Ship the patch, optionally euthanize test #3, sleep well. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 977f39a)Verdict: Approve | Recommendation: Merge Overview
Correctness / Safety FindingsNo correctness or safety findings. The P1 blocker from the merge-gate review is properly addressed: the new commit wraps The earlier inline comment ( Ponytail Review
Ponytail net: -8 lines (optional). Suggested Minimal PatchNo patch required. The race fix is correct, the new Final Merge GuidanceCan merge. The P1 merge-gate blocker is closed, the prepare-once invariant is now robust across process death and recreation, and tests #1 and #2 pin the structural contract. 🏆 Best part: Choosing 💀 Worst part: Test #3 now asserts "the file contains the letters I-N-I-T-I-A-L-I-Z-I-N-G." Congratulations, you've written a test that passes for any file with a state machine in it. I've seen less ambitious tests hold a door open. 📊 Overall: The P1 fix is exactly the right shape — minimal, idiomatic, and the only test that deserved to die got softer without actually dying. Ship it. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit ac0e3c7)Verdict: Approve | Recommendation: Merge as-is Overview
Correctness / Safety FindingsNo correctness or safety findings. The fix swaps a keyed-on-every-state-change Ponytail Review
Ponytail net: -9 lines (optional). Suggested Minimal PatchNo patch required. The race fix is correct and minimal. The single Ponytail item is optional cleanup — feel free to skip if you want the comment-content guard to stay as belt-and-suspenders. Final Merge GuidanceCan merge as-is. The race that issue #756 describes is closed, the new test pins the entry-once contract, and the code is actually smaller than what it replaced. Sometimes the diff lies to you in the other direction. 🏆 Best part: Replacing 6 lines of reaction-effect logic with 3 lines of entry-effect logic. Deletion is the feature. 💀 Worst part: A regression test that asserts on a comment string. We're one Drive By Editor away from "the comment said Initializing, now it's Initialising" taking down CI. 📊 Overall: Tight fix, accurate intent, comment-coupled test is the only wart. Ship the patch, ship the second test as-is, sleep well. Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 37.1K · Output: 10.7K · Cached: 255.3K Review guidance: REVIEW.md from base branch |
![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: ac0e3c76a9
ℹ️ 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".
| LaunchedEffect(Unit) { | ||
| viewModel.prepareForJustLift() |
There was a problem hiding this comment.
When this composable is recreated while auto-start is already Initializing or Countdown—for example during an iOS composition recreation or Android configuration change—the unconditional entry effect still calls prepareForJustLift(). That method resets every non-Idle state to Idle in ActiveSessionEngine.kt:6627-6639, so it cancels precisely the in-flight execution this change intends to preserve; it can also reset an Active session, which the previous condition explicitly exempted. The entry action must skip states owned by an existing execution, or the manager method must make that distinction.
Useful? React with 👍 / 👎.
Merge gate review — BLOCKED / changes requiredCI is green (10/10 checks pass) and the PR's state is The new unconditional Required before approval/merge:
No approval or merge was issued. |
…creation LaunchedEffect(Unit) re-fires after process death / config change, calling prepareForJustLift() → resetForNewWorkout() which invalidates the in-flight auto-start execution (Initializing/Countdown states). Add a rememberSaveable flag so the reset only fires on genuine first entry, not screen recreation. rememberSaveable survives process death, so after recreation the flag is restored as true and the reset is skipped. P1 fix for merge gate review on PR #759. Fixes #756
P1 Fix: rememberSaveable guard against screen recreationAddressed the merge gate P1 blocker: Problem: Fix: Added a
Files changed:
Commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 977f39a1cc
ℹ️ 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".
| !source.contains("LaunchedEffect(workoutState) {\n if (workoutState !is WorkoutState.Idle") && | ||
| !source.contains("viewModel.prepareForJustLift()\n }"), |
There was a problem hiding this comment.
Whenever the common tests run, this assertion fails against the code added in the same commit: the new guarded block still contains the exact substring viewModel.prepareForJustLift()\n }, so the negated second conjunct is false even though the call is no longer inside LaunchedEffect(workoutState). Check the effect key directly, or otherwise scope the search to the obsolete block, so this regression test can pass.
Useful? React with 👍 / 👎.
…tching The second contains() check was matching the new if-block's closing brace, not the old LaunchedEffect(workoutState) pattern. Use a simpler check for the presence of LaunchedEffect(workoutState) in the source.
The file has a legitimate LaunchedEffect(workoutState) for navigation (Active → ActiveWorkout). The test was incorrectly rejecting any LaunchedEffect(workoutState) in the file. Now checks only that prepareForJustLift is not called inside any workoutState-keyed effect.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // (Other LaunchedEffect(workoutState) usages for navigation are fine.) | ||
| val workoutStateEffectSections = source.split("LaunchedEffect(workoutState)") | ||
| for (section in workoutStateEffectSections.drop(1)) { | ||
| val blockContent = section.takeWhile { it != '}' } |
There was a problem hiding this comment.
🔥 The Roast: You've replaced one false-negative string-match with a more sophisticated one. takeWhile { it != '}' } stops at the first } it sees — which is usually the closing brace of a nested block (like the if inside the navigation LaunchedEffect). If someone reintroduces the bug as LaunchedEffect(workoutState) { if (foo) { bar() } prepareForJustLift() }, this test waves it through because it only inspects up to the } after bar(). Same flavor of false-negative as the original Codex P1 this PR is fixing — the new heuristic just narrows the bug shape.
🩹 The Fix: Use a regex that matches one level of nested braces, e.g.:
val effectBlocks = Regex("""LaunchedEffect\(workoutState\)\s*\{((?:[^{}]|\{[^{}]*\})*)\}""").findAll(source)
for (match in effectBlocks) {
assertTrue(
!match.groupValues[1].contains("prepareForJustLift"),
"prepareForJustLift must not be called by workout-state changes; " +
"Initializing/Countdown belong to the active Just Lift execution.",
)
}Or accept this as a known limitation — the current coding style doesn't write the bug this way, so the test guards the realistic failure mode.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a26edcde1
ℹ️ 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".
| LaunchedEffect(Unit) { | ||
| if (!hasPreparedSession) { | ||
| hasPreparedSession = true | ||
| viewModel.prepareForJustLift() |
There was a problem hiding this comment.
On a fresh, already-connected entry, the preceding LaunchedEffect(connectionState) arms handle detection before this call, while ActiveSessionEngine.prepareForJustLift() only queues its parameter update in scope.launch. If the user grabs the handles in that interval, the handle collector observes the previous useAutoStart value and ignores the Grabbed transition; the preparation method's later enableHandleDetection() is debounce-skipped, so the detector remains grabbed and auto-start does not retry until the user releases and grabs again. Complete preparation before enabling detection, or atomically arm it as part of preparation.
Useful? React with 👍 / 👎.
Fixes #756
Summary
Verification