Updates inbox: make it scroll, and make the release row actually open (#984) - #989
Conversation
…#984) Two symptoms in the report, one shared cause plus a second bug underneath. The sheet could not scroll. ModalBottomSheet hands its content a plain ColumnScope and does not scroll it, and the inbox root was a bare Column — so everything past the bottom of the screen was clipped and unreachable, the Clear all / Mark all read footer included. That is also why a tapped row looked deleted. Tapping marks the row read, which moves it out of "New" and down into "Earlier" — rendered below, i.e. into the part that could not be reached. The row was never removed; it relocated somewhere the user could not get to. Underneath that, the What's New row was posted with no deepLink at all while its own message reads "tap to read what's new". handleTap bails on a null link, so the tap could never have opened anything: every release since the inbox shipped has posted an entry that cannot be read. The destination already existed — WhatsNewSheet, the same one Settings > About opens — it was simply never wired to the row. Fixes all three: verticalScroll on the root, a deepLink on the seeded row, and a "whatsNew" case in AppRoot's onDeepLink that presents the changelog. It is handled beside the route table rather than in it because What's New is a sheet, not a nav destination. The tap rule moves into UpdateStore.deepLinkTarget so it can be unit-tested — the store's own persistence is SharedPreferences + org.json and is not reachable from a plain JVM test, the same constraint NapStoreTest documents. It also falls back by kind, which matters: What's New rows already sitting in people's inboxes carry no deepLink and would otherwise stay inert until a later release replaced them. That includes the reporter's. iOS is deliberately not matched. Its inbox is a ScrollView, so the scroll bug does not exist there. It has the same missing deepLink, but per #980 the inbox is only mounted in classic TodayView and not in Liquid, which is the default — so there is no reachable surface to fix until that lands. Tests: 6 JVM tests over the tap rule, including the already-posted-row case.
|
Re-reviewed. No new defects. Four things checked that could each have made this subtly wrong: Only one mount. The fallback cannot mislead. I was concerned that an OLD What's New row (say 9.2.1) still sitting in an inbox would now open the changelog and show the wrong version. It does not: A claim I made in the PR body, now verified rather than asserted. I wrote that every release since the inbox shipped has posted an unopenable entry. The production change is small. 143 insertions / 7 deletions, of which 74 are the test file — so the actual behaviour change is about 25 lines across three files. One pre-existing wrinkle, recorded rather than fixedSetting Fixing it properly means making the close path suspend so it can await the hide animation, which changes shared behaviour for every deep link and is more than a bug fix should quietly take on. Flagging it so it is a decision rather than a surprise: if the transition looks abrupt on device, that is why, and it is a separate small change. Everything else stands from the first pass — 6 JVM tests green in CI, all 7 assertions also executed locally against the shipped function body, module compiles at main's exact baseline with Still not device-tested. The scroll gesture against the sheet's drag-to-dismiss, and the sheet-over-sheet presentation, are the two parts I would want a real device to confirm. |
85 commits from ryanbr/noop (a26bd3a..d94d568, 6–9 Aug): Oura decode fixes (0x5D bucket order and pairing, RSA gating), a batch of BLE work (5/MG bond watchdog, empty-offload backoff, MTU settle, battery poll throttle, DIS-based 4.0/5.0 correction), sleep-card reordering, the stress motion gate, the ryanbr#103 SpO₂ @82 candidate, language settings, and the ryanbr#1068 visual-system pass. 24 files conflicted. Resolved so that upstream wins on behaviour and this fork wins on its own visual identity: - Liquid Today, the Sleep hero, ScreenScaffold, StrandCard/Typography/Components keep the fork's design. Upstream's ryanbr#1068 rewrite (NoopPanelSurface everywhere, SF-Rounded roles, native tab bar) is deliberately not adopted — it targets a screen layout this fork replaced. - Features upstream re-mounted in ryanbr#980 are taken: HealthAlertBanner and AutoWorkoutCard now render on Liquid Today, which is the default screen here, so a raised health alert finally has a home surface. `dataSourcesSection` is NOT re-mounted — the reorderable card block already renders it. - Also taken: the ryanbr#1112 Sleep card reordering (all six of this fork's cards are covered by upstream's section enum), ryanbr#989 hydration, ryanbr#804 Fix A's provided hypnogram, the accent-colour storage, SyncChipState, and ryanbr#103's SpO₂ candidate — the last one rerouted through `windowedSpark` so the tile still honours the Detailed-tiles window picker. - The string catalog was merged structurally, per key, rather than textually: 4352 keys, 35 new from upstream, every focus locale complete. Two silent regressions from upstream's rewrite were caught and restored: ScreenScaffold's `overSky` title contrast (fork-only, deleted by ryanbr#1068) and the `icon:` argument that ktile gained. Both heroes now share `LiquidScoreGauge` instead of each inlining a vessel. Verified: Strand (macOS) and NOOPiOS build; StrandTests 1864 pass; WhoopProtocol 558, WhoopStore 367, StrandAnalytics 1302 pass; i18n audit clean. One upstream test (AppLanguageTests, new in ryanbr#1181) asserted through NSArgumentDomain, which this fork's test scheme pins to `en` — rewritten to read the suite's own domain, matching what the test already does for its second assertion. NOT verified: Android does not compile here (no SDK on this machine), and none of the BLE changes have been near a strap.
…ryanbr#984) (ryanbr#989) Two symptoms in the report, one shared cause plus a second bug underneath. The sheet could not scroll. ModalBottomSheet hands its content a plain ColumnScope and does not scroll it, and the inbox root was a bare Column — so everything past the bottom of the screen was clipped and unreachable, the Clear all / Mark all read footer included. That is also why a tapped row looked deleted. Tapping marks the row read, which moves it out of "New" and down into "Earlier" — rendered below, i.e. into the part that could not be reached. The row was never removed; it relocated somewhere the user could not get to. Underneath that, the What's New row was posted with no deepLink at all while its own message reads "tap to read what's new". handleTap bails on a null link, so the tap could never have opened anything: every release since the inbox shipped has posted an entry that cannot be read. The destination already existed — WhatsNewSheet, the same one Settings > About opens — it was simply never wired to the row. Fixes all three: verticalScroll on the root, a deepLink on the seeded row, and a "whatsNew" case in AppRoot's onDeepLink that presents the changelog. It is handled beside the route table rather than in it because What's New is a sheet, not a nav destination. The tap rule moves into UpdateStore.deepLinkTarget so it can be unit-tested — the store's own persistence is SharedPreferences + org.json and is not reachable from a plain JVM test, the same constraint NapStoreTest documents. It also falls back by kind, which matters: What's New rows already sitting in people's inboxes carry no deepLink and would otherwise stay inert until a later release replaced them. That includes the reporter's. iOS is deliberately not matched. Its inbox is a ScrollView, so the scroll bug does not exist there. It has the same missing deepLink, but per ryanbr#980 the inbox is only mounted in classic TodayView and not in Liquid, which is the default — so there is no reachable surface to fix until that lands. Tests: 6 JVM tests over the tap rule, including the already-posted-row case.
Fixes #984 (bartmuskala, Android 9.2.3). Two reported symptoms, one shared cause — plus a third bug sitting underneath them.
What was happening
The sheet could not scroll.
ModalBottomSheethands its content a plainColumnScopeand does not scroll it, and the inbox root was a bareColumn. Everything past the bottom of the screen was clipped and unreachable — the Clear all / Mark all read footer included.That is also why a tapped row looked deleted. Tapping marks the row read, which moves it out of
Newand down intoEarlier— rendered below, i.e. into the part that could not be reached. The row was never removed. It relocated somewhere the user could not get to. The reporter's "on click it disappears" is precisely that.And underneath: the release row was never linked to anything.
seedWhatsNewIfNeededposts it with nodeepLink, while its own message reads "NOOP 9.2.3 is here — tap to read what's new."handleTapdoesval key = item.deepLink ?: return, so it returned immediately. Every release since the inbox shipped has posted an entry that cannot be opened. The destination already existed —WhatsNewSheet, the same one Settings › About opens — it was simply never wired to the row.The fix
verticalScrollon the inbox root. Safe: nothing in the screen is itself lazily scrolling, so there is noLazyColumn-inside-verticalScrollinfinite-height crash, andModalBottomSheetalready handles nested scroll for drag-to-dismiss.deepLinkon the seeded row."whatsNew"case inAppRoot'sonDeepLinkthat presents the changelog. Handled beside the route table rather than in it, because What's New is a full-screen sheet, not a nav destination — which is why it fell through toelseand did nothing before.The Dialog is a sibling of the inbox sheet, not nested inside it, so it survives
onClose()dismissing the inbox in the same frame.The tap rule is now testable
UpdateStore.deepLinkTarget(item)— pulled out of the Compose row deliberately. The store's own persistence is SharedPreferences +org.jsonand is not reachable from a plain JVM test (the same constraintNapStoreTestdocuments), so this is the half of the fix that can be pinned, and it is the half that broke.It also falls back by kind, which matters for real users rather than for new installs: a What's New row already sitting in someone's inbox carries no
deepLinkand would otherwise stay inert until a later release replaced it. That includes the reporter's 9.2.3 row. There is a test for exactly that case.iOS is deliberately not matched
UpdatesInboxViewis aScrollView.deepLinkdoes exist there, identically. But per Liquid Today: remount the illness banner, the auto-workout prompt, and the hydration value #980,UpdatesInboxViewhas exactly one mount repo-wide — classicTodayView— and Liquid is the default Today. So there is no reachable surface to fix until that PR lands. Worth doing as a follow-up on top of it.Two independent reports converging: #984 says the Android row does nothing when tapped, #980 says the iOS row cannot be reached at all. Same dead feature, broken two different ways.
Verification
UpdateDeepLinkTest), including the already-posted-row case and that an explicit link is never overridden by the fallback. Android CI runs these.AppRoot.ktconfirmed analysed by injecting a deliberate error and watching it get reported.doc_comment_lintandi18n_audit --ciboth exit 0. No new user-facing strings — the changelog sheet brings its own.