Repository navigation
fix: stamp a device as seen when it connects, not when it was added (#1527) - #1530
Conversation
…1527) A WHOOP that syncs every day reported "Last seen 45 d ago" on its Devices card. The same session's strap log said "Last sync: just now" and carried 37 day rows. `lastSeenAt` was written in four places and none of them was a connection: the schema migration that creates the row, the add-device wizard, DataSourcesScreen, and the promote-to-active query. The BLE path made no registry write at all, so a strap could connect, bond, offload and score for weeks without the column moving. It was reporting time-since-ADDED. Stamped now from `connectedPeripheralChanged` on both platforms, which is where the connected strap is already resolved to a registry row and already refuses to overwrite a row pinned to a different strap: - connect, identity adopted or already matching -> stamp - the #52 stale-pin re-adopt (Swift) -> stamp, it is the same confirmed strap - a DIFFERENT strap under this row -> no stamp; that strap is not this row - disconnect -> stamp, because the strap was in hand right up to that instant The disconnect edge matters on its own: stamping only on connect would leave an overnight session reading "Last seen 10 h ago" the moment the link dropped. A null republish with no prior link is not a sighting and writes nothing. Once per connect and once per disconnect, not per packet -- a write per second would buy no more truth. Archived rows are excluded in the SQL on both sides: "Removed - data kept" is a deliberate resting state and a stray connect must not resurrect one into looking live. Scoped to the WHOOP path, which is what was reported; the other source kinds have their own connect paths and are unchanged. Verified: 6 new tests in SourceCoordinatorAdoptionTest covering both edges, the different-strap guard, the bare-null case and the archived guard; full Android suite 4195 tests, 0 failures (--no-build-cache --rerun-tasks, 4189 before). Swift half validated by app-build -- see the PR.
…r is active
Found re-reviewing the branch. The disconnect edge stamped `activeDeviceId`, and
the doc claimed the `isWhoop` guard stopped a strap switch from stamping the
wrong row. It does not: it only checks the active row is A WHOOP, so on a
multi-WHOOP install strap A could drop while B was active and B would be
recorded as seen despite never having been connected.
That is the same mis-mapping the peripheralId guard three lines below exists to
prevent, so the disconnect now resolves the row by the identity the strap
adopted -- the address it was connected on -- and stamps that. An address no row
has adopted stamps nothing rather than guessing.
Simpler as well as more correct: the identity lookup subsumes the isWhoop check,
because only a WHOOP row can have adopted a WHOOP peripheral id.
Two tests, both verified to FAIL against the previous implementation and pass
against this one:
- a strap switch mid-session stamps the strap that was connected, not the
newly-active one
- a disconnect from a never-adopted address stamps nothing
Verified: full Android suite 4197 tests, 0 failures (--no-build-cache
--rerun-tasks); doc lint and i18n audit clean. Swift half via app-build.
|
Re-reviewed my own branch and found a real defect in it, fixed in The bugThe disconnect edge stamped That is precisely the mis-mapping the The fixThe disconnect now resolves the row by the identity the strap adopted — the address it was connected on — via the existing Simpler as well as more correct: the identity lookup subsumes the Verification of the tests themselvesTwo new tests, and I checked they are worth having by reverting the helper to the old implementation and confirming both fail: then restoring and confirming 18/0. A regression test that passes against the bug it names is worse than none.
Also worth recording: an intermediate |
…insensitively (Android) Second re-review pass. The connect path treats a stored address differing only in case as the SAME strap -- `existing.equals(address, ignoreCase = true)`, which reads as deliberate, someone saw MAC-case variance -- but the disconnect resolved it through `deviceForPeripheralId`, whose `WHERE peripheralId = ?` is case-SENSITIVE in SQLite. So a case-different address would adopt and stamp on connect, then silently stamp nothing on disconnect: the same quiet staleness this branch exists to remove, reintroduced through a stricter lookup on one edge than the other. Matched to the adjacent guard instead of going through the shared query, which other callers use and which the Swift store mirrors exactly. Deliberately NOT changed on Swift: its connect path compares exactly (`case .some(uuid)`), so its exact lookup already agrees with its own adopt rule. Each side matches ITS OWN rule; making them match each other would break one of them. Also checked this pass and found clean: the Swift publisher is `.removeDuplicates()` before the sink, so a republish of the same uuid cannot write repeatedly. One test, verified to FAIL against the exact-match lookup and pass against this one. Full Android suite 4198 tests, 0 failures (--no-build-cache --rerun-tasks). Swift and Packages are untouched by this commit, so app-build 32530048388 still covers the Apple half.
|
Second re-review pass — one more real defect, Android-only. Matching identity two different waysThe connect path treats an address differing only in case as the same strap: existing.equals(address, ignoreCase = true)That reads as deliberate — someone saw MAC-case variance. But my disconnect resolved the row through Now matched to the adjacent guard rather than the shared query, which other callers use and which the Swift store mirrors exactly. Deliberately not changed on Swift. Its connect path compares exactly ( Checked and clean
VerificationOne new test, again verified to fail against the previous lookup: then passing at 19/0 after the fix. Full Android suite 4,198 tests, 0 failures, doc lint and i18n clean. No new app-build for this one: the commit touches only |
Fixes #1527. Reported from a real device: a WHOOP 4.0 syncing every day showed "Last seen 45 d ago" on its Devices card, while the same session's strap log said
Last sync: just nowand carriedHistory: 37 day rows.Cause
lastSeenAtwas written in exactly four places, none of them a connection — the schema migration that creates the row, the add-device wizard,DataSourcesScreen, and the promote-to-active query.WhoopBleClientmakes no registry write at all, so a strap could connect, bond, offload and score for weeks without the column moving. It was reporting time since the row was added.The fix
Stamped from
connectedPeripheralChangedon both platforms — the function that already resolves the connected strap to a registry row, and already refuses to overwrite a row pinned to a different strap. Sitting there means the fix inherits those guards instead of re-deriving identity:The disconnect edge earns its place: stamping only on connect would leave an overnight session reading "Last seen 10 h ago" the moment the link dropped, which is a subtler version of the same lie.
Once per connect and once per disconnect, not per packet — a write per second would buy no more truth. Archived rows are excluded in the SQL on both sides, since "Removed · data kept" is a deliberate resting state and a stray connect must not resurrect one into looking live.
Scoped to the WHOOP path, which is what was reported. The other source kinds have their own connect paths and are untouched.
Verification
SourceCoordinatorAdoptionTest— both edges, the different-strap guard, the bare-null case, and the archived guard. Confirmed they actually ran (the class goes 10 → 16) rather than the filter matching nothing.--no-build-cache --rerun-tasks; 4,189 on main, +6).Test Strand— so the app-target Swift compiles andStrandTestspasses. Default CI covers neither.Worth flagging
Writing this I inserted all three new declarations between an existing doc block and its declaration. Kotlin's
/** */blocks stack visibly and the doc lint caught two of them — but the Swift one merged silently, because consecutive///lines join into one block:connectedPeripheralChangedlost its entire doc comment into my helper's, and the lint did not flag it. All three are moved above their neighbours now, and I checked each doc is reattached to the declaration it describes. The Swift blind spot is worth remembering; it fails quietly.Not fixed here, from the same report: the green Active badge is the registry status, not a live connection, so "Active" next to a stale last-seen reads as a contradiction. Fixing the timestamp removes most of the sting. The charging disconnect that prompted all this is #1528, awaiting a log.