Repository navigation
today: let the Recovery Vitals rows open their trends, like the cards do (Android + Apple) - #1902
Conversation
From a screenshot: HRV 31 ms, Resting HR 62 bpm and Breaths 13.3 rpm sitting in the RECOVERY VITALS card, none of them tappable, while every neighbouring card in the same scroll - Session, Journal, Data sources - carries a chevron and opens something. The three vitals already HAVE trends. The HRV / Resting HR / Respiratory dashboard cards open `vital_detail/hrv|rhr|resp`, the route is registered and VitalDetailScreen renders it. The summary card shows the same three metrics and simply had no way through, so it was the one place on Today where a metric was a dead end. Nothing removed a handler - these rows never had one; the card was built to mirror the iOS section, which does not have one either. The rows take their keys from `dashboardCardMetricKey` rather than repeating the strings. A literal here could send the row and the card to different trends for the same vital with nothing to catch it, which is the same "one value, two places" shape that caused today's read-cap bug; sourcing it makes drift impossible rather than merely tested. The tests pin both halves anyway - that the keys match the cards', and that the three cards always resolve, so the requireNotNull cannot fire in the field. The chevron appears only when a row goes somewhere. `dashboardCardDestination` already states that rule for cards - every one resolves to a destination, so the chevron is always honest - and this was its inverse: three rows that could have gone somewhere and did not say so. Android only. The iOS `recoveryVitalsSection` has the identical dead end, but that view has no metric-detail navigation in reach, so wiring it is a different change and not one I can verify.
The first draft resolved each row's key through a helper that asserted the key was present. That turns a missing destination into a crash on tap - the worst possible trade for a navigation convenience, and on the one surface a user reaches for when a number looks wrong. The row now takes a nullable key and loses the tap AND the chevron together when it is absent, which is also the honest rendering: a row that cannot go anywhere should not claim it can. The degradation is not hypothetical either - cards with their own screen (Sleep, Stress, Coach) return null by design, and a future vital row pointed at one would have hit the assert. Drift is still impossible: the key is passed straight from `dashboardCardMetricKey`, so the row and its dashboard-card twin cannot open different trends for the same vital. The tests now pin the null path too, so the degradation is covered rather than assumed.
|
Two things a second read turned up that I deliberately have NOT changed, because 1. The rows will ripple; almost nothing else on this screen does. 17 of the 23 I left it that way on purpose, and the reasoning cuts both ways. But it IS the minority convention, and if the ripple looks wrong against the 2. The touch target is about 26dp tall, against Android's 48dp guideline. The row's height is driven by the 26dp liquid vessel; there is no explicit Raising it changes the card's height (three rows, so roughly 50dp taller unless Worth checking both on the next testing build. |
The iOS card had the identical dead end: three vitals that are tappable as dashboard cards and inert as rows, from the same defaultSelection Android ships. Leaving it meant the same report arriving from an iPhone. BOTH live Today views, because both carry it - LiquidTodayView and TodayView are chosen by liquidTodayEnabled and neither is legacy. They use different row builders (vitalRow with spacing, metricRow with dividers), so each gets the change in its own idiom rather than one being bent to the other's shape. Each row builder takes an OPTIONAL route and renders exactly what shipped before when it is nil - the three other metricRow callers are untouched, and a row that cannot navigate gains neither a link nor a chevron. LiquidPressStyle is not decoration: a bare NavigationLink applies the default link chrome and would tint the whole row, which is why cardLink already carries it. Routes are the iOS ones, taken from liquidCard's own cases - hrv, rhr and resp_rate. Respiratory is deliberately NOT Android's "resp": these are each platform's own route keys, not a shared contract, and mirroring Android's would push to a destination that does not exist here. Correcting the record: the earlier claim that this view had no metric-detail navigation in reach was wrong. cardLink sits in the same file and is a plain NavigationLink; I had grepped for the wrong name and concluded the mechanism was absent rather than differently spelled. What actually made it non-trivial was the two views, which is a smaller obstacle than the one I reported. Verified only as far as this machine allows: both files parse, doc-comment lint and the i18n gate are clean, and every name relied on already resolves in the files that now use it - TabRoute.metric, LiquidPressStyle, and a navigationDestination(for: TabRoute.self) registered centrally, with TodayView already using NavigationLink(value:) at another call site. Nothing here has been compiled or seen.
ryanbr
changed the title
From a screenshot: HRV 31 ms, Resting HR 62 bpm and Breaths 13.3 rpm in the
RECOVERY VITALS card, none of them tappable, while every neighbouring card in the
same scroll - Session, Journal, Data sources - carries a chevron and opens
something.
What was actually wrong
The three vitals already have trends. The HRV / Resting HR / Respiratory
dashboard cards open
vital_detail/hrv|rhr|resp, the route is registered inAppRoot.kt, andVitalDetailScreenrenders it. The RECOVERY VITALS summarycard shows the same three metrics and had no way through - the one place on Today
where a metric was a dead end.
Worth being precise, because the report described it as a regression: nothing
removed a handler. These rows never had one. The card mirrors the iOS
recoveryVitalsSection, which does not have one either. What makes theexpectation reasonable anyway is that the same three metrics ARE tappable as
cards, so the app teaches you they open, then gives you a copy that doesn't.
The keys are sourced, not repeated
The rows take their keys from
dashboardCardMetricKeyvia a smallvitalRowKeyhelper rather than writing
"hrv"/"rhr"/"resp"again. A literal here couldsend the row and the card to different trends for the same vital and nothing
would catch it - the same "one value, two places" shape behind today's read-cap
bug. Sourcing it makes drift impossible rather than merely tested; the tests pin
it regardless, including that the three cards always resolve so the
requireNotNullcannot fire in the field.The chevron
Added only when a row goes somewhere.
dashboardCardDestinationalready statesthe rule for cards - every one resolves to a destination, so the chevron is
always honest (#706/#684) - and this was its inverse: three rows that could have
gone somewhere and did not say so.
Verification
Not verified on a device. The tap target, the chevron's placement in the row,
and the trend screens opening with real data are all things only a build shows.
Both platforms
Android and Apple, because both carried the identical dead end from the identical
defaultSelection. On Apple that meant BOTH live Today views -LiquidTodayViewand
TodayVieware chosen byliquidTodayEnabledand neither is legacy. They usedifferent row builders (
vitalRowwith spacing,metricRowwith dividers), soeach got the change in its own idiom.
Each builder takes an OPTIONAL route and renders exactly what shipped when it is
nil, so a row that cannot navigate gains neither a link nor a chevron.
LiquidPressStyleis not decoration: a bareNavigationLinkapplies default linkchrome and would tint the whole row, which is why
cardLinkalready carries it.Correcting two things I said earlier in this PR.
I claimed the Apple view had "no metric-detail navigation in reach". Wrong -
cardLinkis in the same file and is a plainNavigationLink(value:). I hadgrepped for the wrong name and concluded the mechanism was missing rather than
differently spelled. The real obstacle was the two Today views, which is smaller
than what I reported.
I also justified the optional route by saying it left "the three other
metricRowcallers untouched". There are none.metricRowhas exactly threecall sites and all three are these vitals rows; my count had included the
definition line. The optional route still earns its place - it keeps a
non-navigating row honest and mirrors the Android twin, where the key genuinely
can be null - but not for the reason I gave.
Routes are each platform's own:
hrv/rhr/resp_rateon Apple,hrv/rhr/respon Android. Respiratory differing is correct, not drift - theseare internal route keys, not a shared contract, and mirroring Android's would
push to a destination Apple does not have.
What is verified, and what is not
Android: 5316 tests, 0 failures, 3 new; doc-comment lint clean; no new strings.
Apple: nothing has been compiled or seen. Both files parse, and every name
relied on already resolves in the file using it -
TabRoute.metricexists,LiquidPressStyleis already used inTodayView, andnavigationDestination(for: TabRoute.self)is registered centrally withTodayViewalready usingNavigationLink(value:)elsewhere. That last checkmattered most: a link with no matching destination silently does nothing, which
would have reproduced the exact bug this fixes.
Still unseen on any device, on either platform: the tap targets, the chevrons'
placement, and whether the press style reads correctly on a row inside a frosted
card. The Android touch target is about 26dp against a 48dp guideline - see the
comment below.