Finalize: The Readout Always Fits Its Box (#43) - #46
Finalize: The Readout Always Fits Its Box (#43)#46wing-commander-bot[bot] wants to merge 16 commits into
Conversation
… shape Pure extract-method refactor: computeTopStripLayout's band/collapse/picker/ readout-width-cap steps now live in computeReadoutWidthCap, and every occupant's layout entry carries a capped flag (hardcoded false for now), with maxLines added to the readout. No placement arithmetic changes.
Adds the 360 CSS px portrait/landscape pair and 412 landscape pair so all three FR-020 pinned widths exist in both orientations, plus a plain stand-in helper for the readout's wrapped height at a given capped width — the shared fixtures every user story's fit/cap tests build on.
computeTopStripLayout now resolves the readout's height from a real height-at-capped-width measurement, grown up to availableBox.height/3 and capped/elided past that bound, while the reserved-region subtraction that decides usable width is computed from the growth allowance rather than any occupant's achieved height — severing the width->height->width cycle 012 left standing (FR-016a). Every occupant's layout entry now carries a real `capped` flag and the readout carries `maxLines`. App.svelte adds the shell's two-pass measurement: a nowrap natural-size probe and a second hidden probe pinned to computeReadoutWidthCap's result, whose measured height feeds computeTopStripLayout's new fourth argument. The visible .readout gains a structural line-clamp clip and an aria-label fallback to the full text when capped, so a sizing mistake degrades to less text shown rather than text on the cave (FR-002). tests/lib/layout/topStrip.test.ts pins FR-004/FR-009/FR-016a/SC-001/SC-003 with a height-for-width stand-in (no DOM) over the 320/360/412 px pinned viewport set in both orientations, plus the desktop and title-screen no-regression cases. All 90 topStrip tests and the full 674-test suite pass unchanged elsewhere.
Fixes readout.rect.y to center against the readout's natural (single-line) height rather than its achieved height, so growth only ever extends the box downward from a fixed top edge — the achieved height now reaches only readout.rect.height/capped/maxLines, never x/y/width or any other occupant's rect, matching FR-013/FR-022's severed-cycle guarantee exactly. Adds test coverage: mute/picker rects identical across one-line, tallest- permitted, and deliberately-wrong readout heights at every pinned viewport; two identical calls are deep-equal; a wrong achieved height changes only the readout's own height-derived fields; a grown readout never flips a borderline collapse decision; and the full set of 012's non-overlap/containment/reserved-region properties hold with the readout grown to the tallest the allowance permits.
…tory 3) The theme picker's collapsed control gains the same single-line elision (overflow/text-overflow/nowrap) and aria-label-when-capped fallback the readout already has, reading the same generic `capped` flag computeTopStripLayout already produces for every occupant — no new mechanism, no theme id or per-count branch. Adds test coverage: an oversized collapsed picker is contained and flagged capped; capped stays false across every sampled theme count where the shared collapsed size fits; and a wider collapsed sample alone (no code change) demonstrates the same policy covers a future long display name.
…ng check Adds a node-only test that pins the readout's placed height to its unwrapped natural height (today's shipped bug) and asserts it fails the FR-004 fit assertion at 320px and 360px in both orientations, on the existing runner with no browser. Records a new "Top-strip content never renders outside its box (013, #43)" item in docs/manual-verification.md's Standing checks section, alongside 012's overlap item, instructing the maintainer to re-check on the narrowest real device whenever the top-strip markup/CSS or layout module changes. Spec 012's own spec.md is untouched.
npm test (build + full vitest suite) passes: 689 tests across 49 files, including the single-file build check. Diff against main touches only src/lib/layout/topStrip.ts, src/App.svelte, tests/lib/layout/topStrip.test.ts, and docs/manual-verification.md (plus this feature's own spec artifacts) — no src/sim/ file, no cave data, no theme data file, no theme id in topStrip.ts or its App.svelte wiring, and no viewport width hard-coded outside the test file.
charlesguse
left a comment
There was a problem hiding this comment.
The pure rule is right, and I want to say that first, because the part of this
that was hard to get right is correct. computeReadoutWidthCap reads only
geometry and the mute/picker natural sizes; the reserved-subtraction band uses
the growth allowance while centerY keeps 012's natural-size band, which is
exactly the substitution FR-016a asks for without moving the mute button or
picker a pixel; and SC-006's "deliberately wrong achieved band height" test is a
test that actually fails if the cycle is ever reintroduced. 689 tests green,
build clean.
The defect is in the shell wiring, not the rule — and it is the same shape as
the cycle the rule just severed.
1. The second measurement pass does not re-run when the cap width changes
readoutHeightAtCapWidth (src/App.svelte) tracks topStripProbeTick,
theme.displayName, hudText, and readoutCappedProbeEl. It does not read
readoutWidthCap, insetBox, or touchLayout. So the width the capped probe
is pinned to can change without the height at that width ever being re-measured,
and computeTopStripLayout is then handed a height measured at a different
width than the one it is about to assign. Its own comment states the contract it
is not keeping: "the readout's real wrapped height at exactly
computeReadoutWidthCap(...)'s result".
FR-016b enumerates the triggers as "per resize, orientation change, or content
change". The cap has a fourth input those three do not cover: reservedRects,
via touchLayout, via controlsVisible — which flips on lastInputSource and
session.screen. refreshInsetBox() bumps the tick for the first three. Nothing
bumps it for the fourth.
Concrete repro, on a device class already in 007's outstanding manual items.
Touchscreen laptop, playing with the keyboard, so controlsVisible is false and
reservedRects is empty. Pause. hudText is now frozen — the time value stops
advancing and the score cannot change. Tap the screen: lastInputSource becomes
touch, controlsVisible flips true while screen === 'paused', the touch
controls' reserved regions appear, and the cap narrows. topStripLayout
recomputes with the narrower width and the old, too-short height. The readout
now needs more lines than its box has, and overflow: hidden plus
-webkit-line-clamp do their job: the text is silently cut. It stays cut for the
whole pause, until a resize, a theme switch, or resuming.
That is a hidden readout value with nothing to signal it — the failure mode
FR-011 rejects by name and 005 FR-021 / 012 SC-002 forbid.
The acceptance criterion I would like, rather than a specific patch: the height
measurement must re-run whenever the cap width changes, from any of the cap's
inputs, and the re-measure must observe the probe at its updated width.
I am deliberately not prescribing "read readoutWidthCap inside the derived" —
that is the obvious one-liner, but a getBoundingClientRect() inside a
$derived is only correct if the probe's style attribute has already been
flushed when the derivation runs, and making the derived depend on the cap does
not by itself establish that ordering. An $effect-based measurement may be the
honest shape. Please pick the one that makes the ordering explicit rather than
incidental, and keep the pass count at FR-016b's fixed two.
If a node test cannot reach this, then per the spec's own standard the guarantee
lives only as a call-site convention — the thing FR-016a exists to eliminate one
level down. At minimum I would like the trigger set stated as a comment naming
all four inputs, so the next person adding an input to the cap sees the
obligation.
2. Minor, same iteration: maxLines is Infinity at zero text metrics
maxLines = Math.max(1, Math.floor(growthAllowance / sizes.readout.height))
divides by the natural height. The Edge Case "text metrics that are unavailable
or report zero" is handled for the height (readoutHeightAtCapWidth ?? natural)
but not here: a zero natural height gives Infinity, and
-webkit-line-clamp: Infinity is an invalid declaration that the browser drops.
Containment still holds — overflow: hidden and the fixed height are the
belt-and-braces layer and neither depends on maxLines — so this is a note, not
a driver. It rides along for free.
Nothing else. The centerY equivalence, the conservative direction of the taller
reserved-subtraction band, the capped flag generalising to the collapsed
picker, and the FR-023 standing-checks entry all check out.
|
Important PR conversation stage: in-scope-change Maintainer found that Planned action: Append a 'Maintainer Feedback' section to specs/013-readout-overflow-policy/tasks.md with traceable checkbox tasks to fix Run: https://github.com/charlesguse/whatever/actions/runs/33999072260 Cost: $0.3565 · 6/40 turns · claude-sonnet-5 |
|
Important PR conversation stage: in-scope-change Maintainer flagged (as a minor, non-blocking note) that Planned action: Append a small guard-clause task to specs/013-readout-overflow-policy/tasks.md's Maintainer Feedback section for the next implement/converge pass. Run: https://github.com/charlesguse/whatever/actions/runs/33999072260 Cost: $0.3565 · 6/40 turns · claude-sonnet-5 |
…easurement pass (`readoutHeightAtCapWidth`, lines 369-374) does not re-run when the readout's width cap narrows solely because `reservedRects` changes (via `touchLayout`/`controlsVisible`/`lastInputSource`/`session.screen`), so `computeTopStripLayout` can be handed a width from one measurement and a height measured at a different, stale width — reopening exactly the 'hidden value with no indication' failure that FR-011/FR-016b exist to prevent (concrete repro: pausing on a touch-capable device used with a keyboard, then tapping mid-pause narrows the cap while `hudText` is frozen, so the readout silently clips for the rest of the pause).
|
Folded the maintainer's finding into specs/013-readout-overflow-policy: appended a new "Maintainer Feedback" section (T023, T024) to tasks.md describing the fix for Folded. Once every item in this review has folded, the pipeline dispatches implement/converge for the whole review in a single cycle. Cost: $0.2224 · 9/40 turns · claude-sonnet-5 |
…`maxLines = Math.max(1, Math.floor(growthAllowance / sizes.readout.height))` in `src/lib/layout/topStrip.ts` evaluates to `Infinity` when an occupant's natural height is measured as zero, producing an invalid `-webkit-line-clamp: Infinity` declaration the browser drops — containment still holds via the fixed height and `overflow: hidden`, so this is a CSS-hygiene gap rather than a spill risk, but it's the one place the spec's 'zero/unavailable text metrics' edge case isn't guarded the way the height fallback already is.
|
Appended a new "Maintainer Feedback" section (T025) to specs/013-readout-overflow-policy/tasks.md, capturing the maintainer's minor finding that Folded. Once every item in this review has folded, the pipeline dispatches implement/converge for the whole review in a single cycle. Cost: $0.2317 · 10/40 turns · claude-sonnet-5 |
|
Warning This review's fold did not complete cleanly for every item:
Check this run's job list for the failing leg — nothing else in this review was affected. |
|
Implementation cycle 2 dispatched: https://github.com/charlesguse/whatever/actions/runs/33999281067 — note this re-dispatched iteration may hit implement.yml's own iteration cap; if so, that run's own reply on the lifecycle issue is the authoritative outcome. Folded in this review:
|
…es Infinity T023: readoutHeightAtCapWidth is now an $effect-backed state instead of a $derived.by, so it re-runs after the DOM reflects a new readoutWidthCap (insetBox or touchLayout/reservedRects changes), not just on the resize/orientationchange tick. T024: environment is node-only (no jsdom/browser), so shell-wiring DOM ordering is unreachable from vitest; documented the four trigger inputs at the derivation site instead. T025: maxLines falls back to 1 when the readout's natural height is zero or unavailable, instead of dividing by it and producing Infinity.
Branch:
spec/013-readout-overflow-policy→mainIteration: 2 | Stage: review
Tasks: 25/25 checked in tasks.md
src/App.svelte's second height-measurement pass (readoutHeightAtCapWidth, lines 369-374) does not re-run when the readout's width cap narrows solely becausereservedRectschanges (viatouchLayout/controlsVisible/lastInputSource/session.screen), socomputeTopStripLayoutcan be handed a width from one measurement and a height measured at a different, stale width — reopening exactly the 'hidden value with no indication' failure that FR-011/FR-016b exist to prevent (concrete repro: pausing on a touch-capable device used with a keyboard, then tapping mid-pause narrows the cap whilehudTextis frozen, so the readout silently clips for the rest of the pause)..Spec 013: The Readout Always Fits Its Box
This specification fixes a layout defect (issue #43) where the status readout's text could overflow its designated box on phones narrower than 412 CSS px. The defect stems from a width/height mismatch: the readout's width is capped based on space left by other controls, but its height comes from a natural-size probe measured before that cap exists — and on narrow screens, the capped width needs more lines than the probe reported.
The fix restructures the placement algorithm to sever the width→height→width feedback cycle. Instead of deriving the readout's width cap from occupants' heights, the cap is now computed from a structural growth allowance (at most one third of the available box's height). This makes the width cap deterministic and independent of any occupant's height, enabling a fixed two-pass measurement process: pass 1 measures true natural sizes (readout forced to one line); pass 2 measures the readout's height at the now-fixed cap width; one placement call then sizes and positions all three occupants, idempotently and with no feedback loop.
Implementation scope — four areas:
Pure layout algorithm (
src/lib/layout/topStrip.ts): A new exportcomputeReadoutWidthCapextracts the width-cap calculation;computeTopStripLayoutgains a fourth parameter (readoutHeightAtCapWidth) and implements the growth-allowance / cap-severing / per-occupant-height sizing. TheTopStripLayoutreturn shape now carries per-occupantcapped: booleanflags and amaxLinescount for the readout.Shell measurement and wiring (
src/App.svelte): A second hidden measurement probe (the readout with explicit width, nonowrap) replaces the inferred height. The derivedreadoutHeightAtCapWidthre-runs whenever any input affecting the width cap changes (insetBox, reservedRects via touchLayout, content, theme). The visible readout is styled withoverflow: hidden; display: -webkit-box; -webkit-line-clampas a structural backstop.Test suite (
tests/lib/layout/topStrip.test.ts): Four user stories, each adding targeted property coverage. User Story 1 proves every box is sized for its content at the width given; Story 2 proves the mute and picker boxes never move regardless of readout height; Story 3 proves the genericcappedpolicy extends to the collapsed theme picker; Story 4 includes a deliberate-regression test (pinning the bug's shipped behavior fails the suite) and standing checks documentation.Maintainer feedback (
src/App.svelte,src/lib/layout/topStrip.ts): Two fixes applied: (T023) the height measurement derivation now re-runs on all four cap-affecting inputs, fixing the case where reserved rects narrow the cap after the shell's first measurement; (T025)maxLinesguards against zero/unavailable natural height to prevent invalidInfinityCSS declarations.Properties guaranteed: (1) Every placed occupant's box is at least as tall as its content needs at the width it was given, across the pinned 320/360/412 px widths and both orientations. (2) The mute control and theme picker boxes never move regardless of readout height. (3) The placement algorithm is single-pass and idempotent — re-running it on its own output produces identical results. (4) All original spec-012 properties hold (non-overlap, containment, reserved-region clearance). (5) At 412 px and desktop, the behavior is unchanged from before. (6) No file under
src/sim/changes; no theme data or physics data is touched.How to see it
main...spec/013-readout-overflow-policy
docs/manual-verification.md
specs/013-readout-overflow-policy/contracts/topstrip-api.md
specs/013-readout-overflow-policy/data-model.md
specs/013-readout-overflow-policy/plan.md
specs/013-readout-overflow-policy/quickstart.md
specs/013-readout-overflow-policy/research.md
specs/013-readout-overflow-policy/spec-meta.json
specs/013-readout-overflow-policy/tasks.md
src/App.svelte
src/lib/layout/topStrip.ts
tests/lib/layout/topStrip.test.ts
Remaining manual work
Code review and merge to main
Manual verification by maintainer: confirm on 320px and 360px real devices that readout text does not spill outside its background box
Lifecycle issue: #43