Skip to content

fix(android): report deleted sleep recompute scope accurately - #2776

Open
kavemang wants to merge 2 commits into
ryanbr:mainfrom
kavemang:agent/android-sleep-recompute-reporting-515
Open

kavemang wants to merge 2 commits into
ryanbr:mainfrom
kavemang:agent/android-sleep-recompute-reporting-515

Conversation

@kavemang

@kavemang kavemang commented Oct 12, 2026 •

Copy link
Copy Markdown

What this PR does

Android offers recompute for deletion markers of any age, but the action requests the normal 21-day scan. Its success toast previously said detection reran for the selected night even when that night was outside the scan window or scoring failed. The card now names the 21-day limit and the toast confirms only “Deletion marker cleared.” The existing import/no-raw warning remains.

The marker-clear/scan orchestration is now the production helper used by the ViewModel and its deleted-window tests. Cancellation during marker removal propagates instead of being swallowed as a failed clear; failed removal still prevents scanning. Scoring remains best-effort after a successful clear.

This matches the disclosures already proposed for Apple in #2660; it does not duplicate that PR. The two Android resource keys match the corrected English slugs and SHA1 prefixes, with every existing locale retained using the matching Apple translations. No detector, scoring, source policy, schema, BLE sequence or design tokens change. The recent scan scope is unchanged.

Type of change

  • Bug fix

How it was tested

  • Local JVM harness compiled the actual production helper, sleep draft and edit guard, and ran 19 tests: five production deleted-window cases (ordering, failed clear, failed scan, cancellation at both stages), eight existing draft cases, two private spring/fall DST probes, and four existing edit-control mirrors. A private mutation restoring swallowed marker cancellation failed the new production regression. No mutation or private harness is committed.
  • Core Tools: 153 passing tests. Source-comment hygiene and whitespace checks pass. Diff-scoped i18n gate passes. Upstream Tools CI also passed capture, repository acceptance, legacy R-R preservation and core suites on the same head; all four expected checks passed.
  • Local cold Android build reached full Kotlin compilation but was cancelled under sustained memory throttling within the worker limits; it is not compile/test evidence. Upstream Android CI passed the full debug APK build and full unit suite on b711aa66 (run 38286487559).
  • Apple app compilation and real-strap behavior were not tested locally; Apple code and BLE behavior are unchanged.

Checklist

  • Full Android compilation and unit tests passed at the updated PR head
  • UI references use the corrected resource keys; no design-token changes
  • No protocol/schema/storage changes or generated/private artifacts
  • Follows repository contribution guidance

Related issues

Refs #515

Review follow-up

Renamed the two keys to l10n_sleep_screen_recompute_clears_the_deletion_marker_and_a8922cbb and l10n_sleep_screen_deletion_marker_cleared_fc781c48 across all nine Android catalogues and both SleepScreen references. Generated the slugs and SHA1 prefixes from current English, verified every translated value is unchanged, parsed all catalogues, and confirmed the old identifiers are absent. The diff-scoped i18n gate and whitespace check passed locally. All four expected CI checks passed at a821db67fe3061d80cd85b509ddf5da2b667dadb: Android APK/full unit suite, i18n, source hygiene and Tools Python. The final check API reports the same four-check roster with zero non-success results, and the PR head was re-read to confirm it matches. Earlier Android success above belongs to b711aa66.

@kavemang
kavemang marked this pull request as ready for review October 12, 2026 17:55
@ryanbr

ryanbr commented Oct 15, 2026

Copy link
Copy Markdown
Owner

The copy correction is right and the test change is an improvement. One thing needs sorting before this goes in.

Verified

  • "scans the most recent 21 days" is accurate. rescoreAfterEdit calls IntelligenceEngine.analyzeRecent without overriding maxDays, whose default is 21. The old wording implied a single night, which was the misleading part.
  • Replacing the hand-written mirror of recomputeDeletedSleep in the test with a call to the production reopenDeletedSleepWindow is the right direction, and the three cancellation tests are live rather than vacuous: rescoreAfterEdit ends with .onFailure { if (it is CancellationException) throw it }, so cancellation really does propagate out through the helper.

Please change: both string keys now hash the wrong text

The keys embed the sha1 prefix of the English value, and the English value changed while the keys did not:

sha1("Recompute a night to clear its deletion marker and scan the available raw data again.")[:8] = fd9e15c3
sha1("Recompute clears the deletion marker and scans the most recent 21 days.")[:8]               = a8922cbb

sha1("Sleep detection reran using the data available for this night.")[:8] = 01757aad
sha1("Deletion marker cleared.")[:8]                                       = fc781c48

So l10n_sleep_screen_sleep_detection_reran_using_the_data_01757aad now holds "Deletion marker cleared.", and the slug describes content the key no longer has. Beyond being confusing to read, the next time keys are generated from values/strings.xml both will come out under new names and all nine translations will orphan.

Renaming both keys to match the new text, and updating the SleepScreen.kt references, covers it.

Not blocking

scanRecent failures are now swallowed without a log line. That matches the new copy, which no longer claims the scan ran, so the behaviour change is coherent. It does mean a recent scan that fails leaves no trace, though rescoreAfterEdit already swallows its own non-cancellation failures internally, so the new runCatching around it is belt-and-braces rather than the thing doing the swallowing.

For the same reason markerClearStillSucceedsWhenRecentScanFails pins a shape the production wiring cannot produce. Harmless, just worth knowing it is testing the injected lambda rather than a reachable state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants