Stop "Duplicate as manual" deleting the session it copied (both platforms) - #1500
Merged
Conversation
ryanbr
force-pushed
the
fix/duplicate-as-manual-deletes-original
branch
from
August 21, 2026 01:17
52ee434 to
e60f9ba
Compare
ryanbr
added a commit
that referenced
this pull request
Aug 21, 2026
Editing a workout looked like it saved and changed nothing, across restarts. `workout`'s primary key is (deviceId, startTs, sport), and the edit sheet builds every manual row on the "my-whoop" seed. On a strap whose ACTIVE id is something else - re-paired, or identified by serial - the original row sits under that active id, so the edit did not overwrite it, it inserted a second row beside it. saveManualWorkout then only deleted the original when startTs or sport had moved, which on a plain "change the duration" edit they had not. It compared two thirds of the primary key, skipped the delete, and the upsert wrote to a different key anyway. Both rows survived, and workoutsUnion reads [activeDeviceId, "my-whoop"] keeping the FIRST row per (startTs, sport) - so the stale one shadowed the edit for good. That is why a restart did not help: both rows were on disk. supersedesStoredRow now compares the whole key, so a row under an active strap id counts as moved even when startTs and sport are untouched. It sits beside dedupWorkoutsByKey so the predicate is unit-testable rather than buried in a suspend function. Landing on "my-whoop" is also the durable choice: it is in the read union unconditionally, while a strap's active id is only there while that strap is active. Widening that delete exposed a second defect. "Duplicate as manual" pre-fills the sheet from a strap / Apple / lifting row and passes the pre-fill on as `replacing`; the copy claimed source "manual" while still carrying the original's deviceId, so the delete would have named the strap's own namespace and retired the session it was copied from - on a menu item whose whole promise is that it does not touch the original. asManualCopy re-seeds deviceId alongside source, so every delete SAVEMANUALWORKOUT can issue is confined to "my-whoop" - the store's delete is keyed by deviceId, so re-seeding is enough to protect the row. That is a guarantee about the store and nothing else. `replacing` also reaches the Health Connect write-back, which deletes by startTs alone, and no namespace re-seeding can reach that. Stopping the copy travelling as `replacing` at all is the fix that covers both consumers, and it goes out with the Apple twin in #1500. Android only. The two platforms disagree about what "the strap source" is: Android's importedDeviceId is "my-whoop", where the engine reads and where manual rows have always been written, while Apple's IntelligenceEngine is injected the ACTIVE strap id. Moving Apple's writes to canonical while its reads stayed put would have made the #107 cross-source dedup guard and rescoreManualWorkouts silently miss every manual row. Android needs none of it - its rows carry their own deviceId and nothing reconstructs it. Apple's own defects are pre-existing and filed as #1501. 6 tests, 0 failures: the regression case, the in-place edit that must NOT delete, the sport re-key that always worked, the union dedupe that made the bug silent, the detected routing, and that a duplicate of a read-only row cannot name the original's key.
ryanbr
changed the title
ryanbr
added a commit
that referenced
this pull request
Aug 21, 2026
) Editing a workout looked like it saved and changed nothing, across restarts. `workout`'s primary key is (deviceId, startTs, sport), and the edit sheet builds every manual row on the "my-whoop" seed. On a strap whose ACTIVE id is something else - re-paired, or identified by serial - the original row sits under that active id, so the edit did not overwrite it, it inserted a second row beside it. saveManualWorkout then only deleted the original when startTs or sport had moved, which on a plain "change the duration" edit they had not. It compared two thirds of the primary key, skipped the delete, and the upsert wrote to a different key anyway. Both rows survived, and workoutsUnion reads [activeDeviceId, "my-whoop"] keeping the FIRST row per (startTs, sport) - so the stale one shadowed the edit for good. That is why a restart did not help: both rows were on disk. supersedesStoredRow now compares the whole key, so a row under an active strap id counts as moved even when startTs and sport are untouched. It sits beside dedupWorkoutsByKey so the predicate is unit-testable rather than buried in a suspend function. Landing on "my-whoop" is also the durable choice: it is in the read union unconditionally, while a strap's active id is only there while that strap is active. Widening that delete exposed a second defect. "Duplicate as manual" pre-fills the sheet from a strap / Apple / lifting row and passes the pre-fill on as `replacing`; the copy claimed source "manual" while still carrying the original's deviceId, so the delete would have named the strap's own namespace and retired the session it was copied from - on a menu item whose whole promise is that it does not touch the original. asManualCopy re-seeds deviceId alongside source, so every delete SAVEMANUALWORKOUT can issue is confined to "my-whoop" - the store's delete is keyed by deviceId, so re-seeding is enough to protect the row. That is a guarantee about the store and nothing else. `replacing` also reaches the Health Connect write-back, which deletes by startTs alone, and no namespace re-seeding can reach that. Stopping the copy travelling as `replacing` at all is the fix that covers both consumers, and it goes out with the Apple twin in #1500. Android only. The two platforms disagree about what "the strap source" is: Android's importedDeviceId is "my-whoop", where the engine reads and where manual rows have always been written, while Apple's IntelligenceEngine is injected the ACTIVE strap id. Moving Apple's writes to canonical while its reads stayed put would have made the #107 cross-source dedup guard and rescoreManualWorkouts silently miss every manual row. Android needs none of it - its rows carry their own deviceId and nothing reconstructs it. Apple's own defects are pre-existing and filed as #1501. 6 tests, 0 failures: the regression case, the in-place edit that must NOT delete, the sport re-key that always worked, the union dedupe that made the bug silent, the detected routing, and that a duplicate of a read-only row cannot name the original's key.
"Duplicate as manual" is offered on read-only rows - strap, Apple, lifting,
activity file - and the menu's own comment says it is "a copy-to-manual edit path
that doesn't touch it". It can touch it.
asManualCopy builds a row claiming source "manual" while carrying the ORIGINAL's
startTs and sport, and the sheet hands that pre-fill straight back as
`replacing:` (onSave(row, editing)). If the user then changes the sport or the
start - the ordinary reason to duplicate a session rather than keep it as-is -
saveManualWorkout sees a natural key that moved and deletes the OLD key under the
active strap id. A strap-recorded session lives under exactly that id, so the
original is retired.
It is not reliable enough to have been noticed: the delete names
displaySport(original.sport), so it misses whenever the label differs from the
stored sport ("TraditionalStrengthTraining" renders with spaces) and lands
whenever it does not ("Run", "Ride", "Walk"). Camel-cased WHOOP sports survive;
plainly named ones do not.
Repository.saveManualWorkout already documents the invariant - "an IMPORTED row
is never passed here as `replacing` (duplicating one is a pure add), so its
history is never touched". Nothing enforced it. The sheet target now carries an
isCopy flag and a duplicate passes `replacing: nil`, so the form still pre-fills
and the save is an add.
Swift's WorkoutRow carries no deviceId, so the repository chooses the write id
and the caller is the only place this distinction can live. Kotlin's row does
carry one, so the Android twin re-seeds the copy's namespace instead and gets the
guarantee by construction (#1488).
Compile-checked via app-build; StrandTests has no coverage of the sheet, which is
a View.
…k either Android already had the guard this PR adds to Apple, and it never worked. ManualWorkoutDialog passes `replacing` only for a MANUAL or DETECTED row, with a comment saying a duplicate of an imported row is "a pure ADD - never pass it, or a changed key would delete the imported original". But the copy is built with source "manual" so the form treats it as editable, so it classifies as MANUAL and passes straight through, carrying the ORIGINAL's startTs. The test can never reject what it is looking at. Android's DATABASE survived that because deleteWorkoutByKey is keyed by deviceId, and #1489 re-seeds the copy's namespace so the key can only name "my-whoop". The Health Connect write-back is not keyed that way. AppViewModel deletes the old records on ANY edit, and deleteExercise removes "noop-workout-<startTs>" with no deviceId in the client-record id at all - so duplicating a strap session removed the ORIGINAL's Health Connect records, and a duplicate saved at a new start left them deleted with nothing to restore them. So the deviceId re-seed is not the guarantee I claimed it was in #1489. It is sound for the store and says nothing about anything downstream of `replacing`. Apple's fix - stop the copy travelling as `replacing` at all - is the one that covers both consumers, and Android needs the same. DialogTarget carries isCopy, because a row's source cannot express the difference by design. Extracted as WorkoutEditing.replacingRowFor so the decision is unit-testable rather than sitting inside a Composable. 5 tests: a duplicate replaces nothing despite reading as manual, a real manual edit and a detected bout still do, an imported row is still refused without the flag (a second lock, not a swap), and a fresh add replaces nothing either way. Health Connect write-back is opt-in, so this only ever bit users who enabled it.
ryanbr
force-pushed
the
fix/duplicate-as-manual-deletes-original
branch
from
August 21, 2026 03:36
e60f9ba to
6c78971
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
"Duplicate as manual" can delete the session it was copied from.
Cause
The action is offered only on read-only rows — strap, Apple, lifting, activity file — and the menu's own
comment describes it as "a copy-to-manual edit path that doesn't touch it".
asManualCopybuilds a row claimingsource: "manual"while carrying the original'sstartTsandsport, and the sheet hands that pre-fill straight back asreplacing:(onSave(row, editing)). If theuser then changes the sport or the start — the ordinary reason to duplicate a session rather than keep it
as-is —
saveManualWorkoutsees a natural key that moved and deletes the old key under the active strapid. A strap-recorded session lives under exactly that id.
The DB row isn't the only casualty. On Apple the same
replacingdrives the #10 GPS route re-key, whichreads the route under
old.startTs/old.sport— the original's key — stores it under the duplicate'snew key, and then removes it from the original. So duplicating a GPS session and changing its sport moved
the route off the session it was copied from: the original's map and distance simply vanish. Three
consumers, one cause.
It's intermittent, which is likely why it went unnoticed: the delete names
displaySport(original.sport),so it misses whenever the rendered label differs from the stored sport (
TraditionalStrengthTrainingrenders with spaces) and lands whenever it doesn't (
Run,Ride,Walk). Camel-cased WHOOP sportssurvive; plainly named ones don't.
Fix
Repository.saveManualWorkoutalready documents the invariant — "an IMPORTED row is never passed here asreplacing(duplicating one is a pure add), so its history is never touched". Nothing enforced it. Thesheet target now carries an
isCopyflag and a duplicate passesreplacing: nil: the form stillpre-fills, and the save is an add.
Both platforms, because Android had the same guard and it never worked either:
ManualWorkoutDialogpassesreplacingonly for a MANUAL or DETECTED row, but the copy is built with source"manual"so the formtreats it as editable — so it classifies as MANUAL and passes straight through. Android's store survived
that (its delete is keyed by
deviceId, and #1489 re-seeds the copy's namespace), but the Health Connectwrite-back deletes
noop-workout-<startTs>with nodeviceIdin the key at all — so duplicating astrap session removed the original's records, and a duplicate saved at a new start left them deleted with
nothing to restore them.
DialogTarget.isCopycarries what a row's source cannot, andWorkoutEditing.replacingRowFormakes the decision unit-testable.Health Connect write-back is opt-in, so this only ever affected users who enabled it.
Scope
Only the duplicate path. This changes no storage location and no analytics. Two further Apple defects in
this area — an edit stored under an active strap id stops being read once that id changes, and
deleteWorkoutcan't find a manual row under any other id — are pre-existing and filed as #1501; bothneed a design decision about which id manual rows should live under, not a bug fix.
Verification
Android: 5 unit tests, 0 failures — a duplicate replaces nothing despite reading as manual, a real manual
edit and a detected bout still do, an imported row is still refused without the flag (a second lock, not a
swap), and a fresh add replaces nothing either way. Plus
compileFullDebugKotlin.Apple: compiled via
app-build, both legs green (StrandmacOS,NOOPiOSiOS). App-target Swift has nodefault CI, so that workflow is the only thing that builds it.
StrandTestshas no coverage of the sheet,which is a
View.On hardware: duplicate a strap-recorded session, change its sport to something plainly named, save, and
confirm the original is still there — with its GPS route and distance intact, and (on Android with Health
Connect write-back enabled) still present in Health Connect.