Repository navigation
fix(timer): the rest timer and the timed hold can no longer run at once, and a displaced hold keeps what it held - #251
Conversation
The store has always said the rest and the hold mean opposite things and must never run together, but only startWork enforced it. Ticking a timed set's own checkbox by hand starts a rest while the app's hold keeps running: the rest bar with its Skip and its +/-15 s sat behind the hold bar, and then the hold reached zero under a rest that was still counting down -- beeping its own end, and calling onDone with the full target for a set nobody was holding any more. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
pageHiddenAt marks "the app went away", and only a tick clears it -- so switching apps with no timer running left it set for good. The next timer read it on its very first tick and finished as a catch-up: a one-second rest, started on screen and over on screen, with no beep, no vibration and no flash. Only timers short enough to complete on their first tick could hit it, which is why it went unnoticed. Each timer now takes the mark from where the page is at the moment it starts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
stopWork() sat above the "rest timer set to Off" guard, so with the timer Off a tick anywhere took down a hold running on another exercise and started nothing in its place: a 40 s plank in progress gone, its elapsed seconds never logged, and its own onDone never fired. The guard's whole point is that nothing happens. Below the guard the invariant still holds -- the two timers can only run together if a rest actually starts -- and rest Off keeps its promise: tapping each hold, and nothing else touching it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Stopping the hold whenever a rest starts closed the two-timers hole but threw the hold away without a trace: hold a 45 s plank, tick a set on another row or another exercise, and the rest replaced it with nothing written anywhere. The row kept its planned seconds as if the plank had never happened. The hold is handed back on the way out now (abandonWork beside finishWorkEarly and stopWork) and its own row keeps what it held. It is explicitly not a finish: the row stays unticked and starts no rest of its own, because the rest that displaced it is the one counting down. A rest that never starts hands back nothing, because the hold is still going. Under two seconds nothing is handed back at all -- that is a play button tapped by accident, and rounding it up the way an early Done does would write a one-second plank over a real plan. Ticking the held row's own Check comes through the same path from the other side: the tick starts the rest, the rest displaces the hold, and the hand-back lands on the row the tick just ticked -- so that row now logs what was actually held instead of its target. `sec` on a timed row is both the plan and the log, so writing what a part-held set managed would become the next hold's target: 3 seconds of a 30 second plank, and every hold after it 3 seconds long. The plan is kept aside in `planSec` until the row is held to the end, ticked, or given a duration you typed yourself -- each of those IS the new plan -- and it never reaches S.workouts: a finished session keeps only what was logged, so an unfinished row goes back to recording its plan there, exactly as it did before. startWork hands a hold back too, for the same reason a rest does. Every play button is disabled while a hold runs, so nothing in the UI reaches that path today; it is there so the invariant does not depend on which buttons happen to be disabled. The case that asserted the hold simply going now asserts the hand-back, and checks the gone hold still cannot fire late mid-rest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8d83f95 to
e01d482
Compare
|
Ordering note, since these two look independent and are not: #165 builds on this one. Both PRs give A trial merge of the two conflicts in four hunks across Cheapest order is this PR first: 4 commits over 6 files, against 13 commits over 36 files for #165. I will rebase #165 on top once this lands and make that call there. If you would rather take them the other way round I will restructure, it is just considerably more rework. Both are rebased onto v1.3.8 and green as they stand. |
|
Thanks @kurktchiev, the rest timer and a timed hold can't run at once any more, and a displaced hold keeps what it held. For #165: the hold callback is now Released in v1.3.9: https://github.com/DuarteSantos8/openGym/releases/tag/v1.3.9 |
…no hold outlives its workout Second review pass on the self-running timed exercise. The hold now has an owner index like the rest (work.forIdx), moved along by shiftRestOwner when an exercise is added above it, and handed to onDone in the options object DuarteSantos8#251 gave it — onDone(elapsed, { chimed, abandoned, forIdx }), from the hold's own end, an early finish and a rest displacing it alike — so the end-of-hold write, and a displaced hold's hand-back, land on the moved row and are refused for any other exercise. The newest render's handlers live at module level rather than in a per-instance ref: the workout view unmounts on every tab switch, and a hand-over that fires after coming back must run on the instance that is on screen. Finishing a workout, or starting a new one or a backfill, stops a running hold as well as the rest. Tests pin each: the owner handed at the end, on an early finish and on a displacement, a hold that ends after an exercise was added above it, a rest length changed in Settings mid-hold reaching the chained rest.
Four commits on the two timers in
useUI.js. The store's comment above the work timer has always saidthe rest and the hold "mean opposite things, they must never run together", but only
startWorkenforced it. Closing that hole properly turned out to need three more things, which is why this is
four commits rather than one.
Reproduced and tested against the suite. Sections 1, 3 and 4 each have cases that fail on
mainandpass here; section 2's case passes on
mainand is there to stop section 1 from regressing it, whichit did until this commit.
1. Only one side of the invariant was enforced
What breaks.
startWorkcallsstopRest(), butstartRestnever calledstopWork(). Ticking atimed set's own Check while its hold is running goes straight through
Workout.toggle→startRest,so both timers run. The rest bar with its Skip and its ±15 s sits behind the hold bar, and then the
hold reaches zero under a rest that is still counting down, beeping its own end, vibrating, flashing
the screen mid-rest, and calling
onDone(wk.total), which writes the full target over the row andticks it. The Check has no disabled state in any layout, so it was always reachable.
Reproduce. Start a timed exercise's hold, then tap that row's own checkbox. Two bars, and at the
hold's zero the row logs the prescription rather than what you held.
What changed.
startRestcallsstopWork(), the other way round fromstartWork.2. …but a rest that never starts must leave the hold alone
What breaks (introduced by 1, fixed here).
stopWork()sat above the "rest timer set to Off"guard, so with the timer set to Off a tick anywhere took down a hold running on another exercise and
started nothing in its place. A 40 s plank in progress gone, its seconds never logged, its
onDonenever fired. The guard's whole point is that nothing happens.
What changed.
stopWork()moves below the guard. Below it the invariant still holds, since thetwo timers can only coexist if a rest actually starts, and rest Off keeps its promise that you tap
each hold and nothing else touches it.
3. A hold a rest displaces keeps the time it held
What breaks (the remaining cost of 1). Stopping the hold closes the hole but throws the hold away
without a trace. Hold a 45 s plank, tick a set on another row or another exercise, one tap away in the
List layout, and the rest replaces it with nothing written anywhere. The row keeps its planned seconds
as if the plank had never happened.
What changed.
abandonWork()sits besidefinishWorkEarlyandstopWork. The hold is handedback on the way out and its own row keeps what it held. The flag in
onDone(elapsed, abandoned)saysthis was not a finish, so the row stays unticked and starts no rest of its own, because the rest that
displaced it is the one counting down. Under two seconds nothing is handed back at all. That is a play
button tapped by accident, and rounding it up to one second the way an early Done does would write a
one-second plank over a real plan.
startWorkhands a hold back too, for the same reason a rest does.Every play button is disabled while a hold runs, so nothing in the UI reaches that path today; it is
there so the invariant does not depend on which buttons happen to be disabled.
That exposed a plan/log problem.
secon a timed row is both the plan and the log, so writing what apart-held set managed would become the next hold's target, and 3 seconds of a 30 second plank would
make every hold after it 3 seconds long. The plan is kept aside in
planSecuntil the row is held tothe end, ticked, or given a duration you typed yourself. Each of those is the new plan. It never
reaches
S.workouts.buildCompletedWorkoutstrips the key and an unfinished row goes back torecording its plan there, exactly as it did before, with rows that do not carry it passed through by
reference so an ordinary session is the shape it always was.
The same path answers section 1's symptom. Ticking the held row's own Check starts the rest, the
rest displaces the hold, and the hand-back lands on the row the tick just ticked, so that row logs
what was actually held instead of its target, with no second mechanism.
4. A hide from before the timer is not a countdown you missed
What breaks.
pageHiddenAtmarks "the app went away", and only a tick clears it, so switchingapps with no timer running left it set for good. The next timer read it on its very first tick and
finished as a catch-up. A one-second rest, started on screen and over on screen, with no beep, no
vibration and no flash. Only timers short enough to complete on their first tick could hit it, which
is why it went unnoticed.
Reproduce. With the screen flash on: switch away from the app and back with nothing running, then
start a one-second rest (or a one-second hold). No flash.
What changed. Each timer takes the mark from where the page is at the moment it starts.
Tests
useUI.test.js, "a rest and a hold never run together": a rest starting ends the hold; a left-overhold cannot reach zero under a running rest, asserted on the callback's arguments rather than its
count, since a hold left running reaches its own zero and calls back too, with the full target for a
set that stopped being held long before; the displaced hold hands back what it held marked as no
finish; nothing under two seconds; a hold displaced by another hold; and a rest that never starts
hands back nothing. Plus, in "rest timer set to Off", that Off leaves a running hold alone, and in
the flash describe, the two stale-hide cases for a rest and for a hold.
Workout.test.jsx, "a hold a rest displaced": keeps its seconds, stays unticked, starts no rest;shows what it held without becoming the next hold's target; a plan you edited yourself survives;
typing a duration is the new plan; ticking the row drops the plan; a hold held to the end still logs
and ticks; and ticking the held row by hand logs what was held, not the target.
finish-workout.test.js:planSecnever reaches a stored workout, an unfinished row goes back torecording its plan, and rows with nothing to strip come through by reference.
One harness change.
startWorkinWorkout.test.jsx'suiSnapshotwas a freshvi.fn()per read,so a test could not reach the callback the view handed over. It is now one stable mock on the harness,
exactly like
startRestbeside it.Checklist
npx vitest runpasses infrontend/: 1486 passing across 105 files (mainis 1468 across105; +18 tests, no new files)
npm testinapi/unchanged: 181 passing, same asmainnpx vite buildsucceeds (no new warnings)node scripts/check-locales.mjs: 14 locales, 1291 keys each, in syncCHANGELOG.mdis left alone🤖 Generated with Claude Code