Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,12 @@ The SDLC skills own the phases. What is specific to this repo:

**Exclude the separator row from that key.** Its padding is dashes rather than spaces, so stripping whitespace leaves `| ---- |` and `| --------- |` two different strings: main's reflowed separator reads as a row only one side has, and the obvious correction restores it as a second separator in the middle of the table. "Normalise the padding" sounds total and is not — it covers the padding made of spaces.

**A repeated column HEADER is the same trap one step along, and the separator rule above does not cover it.** `| Token | Default | Purpose |` is not made of dashes — it is ordinary words — so every correction aimed at the separator's padding leaves it firing. It fires on `docs/specs/design-system/index.md` in particular because that spec has **two token tables**, so the row appears twice in **both parents**: the merged file then carries a duplicate count of 1 that the merge did not introduce. Two agents hit it independently on that one file within the same hour — PR [#325](https://github.com/fx/liebe/pull/325) and PR [#329](https://github.com/fx/liebe/pull/329) — each with a row extractor that excluded separators and counted headers as rows, and **each read it as a real duplicate to remove before tracing it**. Removing a header that legitimately appears twice breaks the second table, which is item 7's own warning arriving twice in one hour: the false positive is the dangerous direction, because it invites "fixing" something that was right.

Two forms of the fix work, and the first is better: **key rows within their own table**, so an identical row in a different table is a different key — which also handles two tables that legitimately share a _data_ row; or **exclude the header row of every table**, not only the separator, which does not. Neither is needed if the count is taken as "no duplicate the merge introduced" per the bullet above — the subtraction handles the header case, both instances above, and whatever the third turns out to be. The row-type exclusions are belt-and-braces for a check applied without it.

Then verify, on a normalised key, in this order:
- **Duplicate count** — `len(rows) - len(set(rows))` must be `0`.
- **Duplicate count** — the threshold is **not** "zero duplicates in the merged file", it is **no duplicate the merge introduced**. Compare each row's multiplicity in the merge against the **highest** multiplicity either parent already had, and flag only rows that exceed it. Do **not** subtract both parents' duplicate counts: a baseline they share is one baseline, so subtracting it twice makes a correct merge score −1 and lets a genuinely duplicated row (three headers where the parents had two) score 0. A document may legitimately repeat a row, and comparing against the parents' own maximum is what makes that fall out instead of needing an exception per row type. See below for the case that taught it.
- **Sequence** — main's rows and this branch's rows must each keep their relative order in the merge. **Appended rows only**: see below.
- **Set difference in both directions** — merged∖expected and expected∖merged must both be empty, where expected is main's rows ∪ this branch's rows.
- **Whole-file** — the merged file should differ from `origin/main` by this branch's own additions and nothing else, every hunk a `+`. Additions, not added rows: see below.
Expand All @@ -285,6 +289,12 @@ The SDLC skills own the phases. What is specific to this repo:

1. **Patch coverage is a hard gate, and it is the one people miss.** `codecov/patch` requires every new or modified line to be exercised. Run `npm run test:coverage` locally before opening the PR — discovering it in CI costs a round trip, and the fix is usually a test you would rather have written while the code was fresh.

**On a red tree the patch check has no input, and both of its failure modes look like a check that ran.** Vitest with the v8 provider writes no `coverage/lcov.info` when the run fails **and clears the previous one**, so a patch-coverage check taken while anything is failing is reading either a stale file or nothing at all — and "nothing", intersected with the diff, yields no uncovered lines, which is a **pass**. That is the dangerous half: the check reports clean about a tree it never measured. It bites whenever `main` is red, which is exactly when a branch is most likely to be re-verified.

The workaround, needed twice on 2026-07-30 while a main-side story test was failing: rerun with `--exclude '<the failing spec>'` so the reporter gets a clean run to write, and **state the exclusion alongside the number**. The exclusion is invisible in the figure, so a percentage quoted without it is a claim about a suite nobody ran (change [0042](docs/changes/0042-tall-tile-control-geometry.md) PR 3, PR [#325](https://github.com/fx/liebe/pull/325)).

Comment thread
coderabbitai[bot] marked this conversation as resolved.
**That figure is diagnostic, and it is not the gate.** The report comes from a reduced suite, and the excluded spec may be one that covers changed lines — so a clean patch check under an exclusion does not establish the 100% `codecov/patch` requires. It tells you your own lines look covered while the tree is unrunnable; the claim only becomes gate-grade after a **full-suite** rerun once the tree is green, which is also what CI will do.

2. **The PR body MUST name the change document** it works on — `docs/changes/<NNNN>-<name>.md` and which task it completes. That link is how a reviewer finds the requirements the PR is claiming to satisfy; without it they are reviewing the diff against nothing.

3. **Do not pipe the gate.** A shell pipeline exits with the status of its _last_ command, so `npm test 2>&1 | tail -4 && git push` pushes whatever the tests did — `tail` succeeded, and `&&` believes it. The `&&` is right there in the command, which is what makes this worth stating: it reads as a gate, and the failure is invisible unless you already know how pipeline status works. It has happened here, on a run that had genuinely failed.
Expand Down
4 changes: 2 additions & 2 deletions docs/changes/0042-tall-tile-control-geometry.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

The embedded control's fixed 42px thickness (`--liebe-control-height`) and the tile's 14px inset (`--liebe-card-padding`) were never reconciled with the width a single grid column actually gives a tile, and the `tall` tier is by definition one column wide. On a 12-column desktop screen a 1-wide tile is about 63px, leaving a ~35px content region — so a 42px vertical slider bleeds ~3.5px per side into the padding (cosmetic, not clipped), and an `input_number` card at `tall` with `controlStyle: 'stepper'` renders a 156px control row in that same 35px, overflowing ~46.5px past each tile edge and getting cut off by the tile's own `overflow: hidden`. Both are pre-existing and share one cause; the fix is a design-system decision, because [design-system — card anatomy](../specs/design-system/index.md#card-anatomy) pins the slider at a "42px-tall track" and [options/input-helpers](../specs/entity-cards/options/input-helpers.md) calls for a "compact stepper" at `tall` that has never been defined.

**Spec:** [design-system](../specs/design-system/index.md) → [card anatomy](../specs/design-system/index.md#card-anatomy) and [size-adaptive layouts](../specs/design-system/index.md#size-adaptive-layouts); [options/input-helpers](../specs/entity-cards/options/input-helpers.md) → tier layouts · **Status:** draft · **Depends on:** —
**Spec:** [design-system](../specs/design-system/index.md) → [card anatomy](../specs/design-system/index.md#card-anatomy) and [size-adaptive layouts](../specs/design-system/index.md#size-adaptive-layouts); [options/input-helpers](../specs/entity-cards/options/input-helpers.md) → tier layouts · **Status:** complete · **Depends on:** —

## Motivation

Expand Down Expand Up @@ -88,7 +88,7 @@ Two further decisions this change does take:
- [x] **PR 1 — Settle the tall-tier control-geometry contract** (spec only, no source changes): choose among routes A–D per consequence; design-system records the relationship between a fixed control size, the tile inset and the narrowest tile a tier can occupy, with the ≥44px touch floor and the omit-never-clip rule both explicitly satisfied by the choice; the geometry token table reflects any token added or re-pinned, with the theming migration note if a name or meaning changes; input-helpers either defines "compact stepper" or drops the phrase; changelog entries on both specs
- [x] **PR 2 — Stop the `input_number` stepper clipping at `tall`**: implement what PR 1 settled for the stepper, `row` and `full` untouched; unit coverage for the tier's control selection plus one browser-level assertion that the control's box stays inside **the tile's own edge** at `tall` on a 1-wide cell, per the measurement requirement above, with the assumed container width recorded in the fixture and `controlStyle: 'stepper'` set explicitly
- [x] **PR 3 — Make the vertical slider cross-axis flexible**: implement PR 1's decision across the light, cover, fan and `input_number` `tall` layouts — the track takes `--liebe-control-height` where the content region affords it and the region's width where it does not, and is omitted below the 24px floor, reading the shell's content-width signal rather than measuring the DOM. `--liebe-card-padding` is not touched. **Both floors, not just the cross-axis one**: whatever capacity signal the long-axis floor needs is PR 3's to establish alongside the width one, since a card may not measure the DOM for either. Browser-level measurements of the track against the tile's content box at a recorded container width **and a recorded span**, one covering each omission — the sub-floor width (LCARS on a 12-column grid) and the sub-floor band (the minimum 1×2 `tall` span) — plus the token-contract test for the token's re-pinned meaning
- [ ] **PR 4 — Bring the remaining fixed-size parts under cross-axis fit**: `input_text`'s 100/150px field and `input_datetime`'s 120/200px inputs first — both render at `tall`, both carry inline minimum widths larger than the stepper's buttons, and both therefore overflow a 35px content region today rather than latently — then the icon circle (40px), the chip (34px) and the vertical pill group (38px), which have no measured symptom either way. Each measured against the content region the way PRs 2 and 3 were, and each made cross-axis flexible, omitted below the floors, or recorded as conforming with the measurement that shows it
- [x] **PR 4 — Bring the remaining fixed-size parts under cross-axis fit**: `input_text`'s 100/150px field and `input_datetime`'s 120/200px inputs first — both render at `tall`, both carry inline minimum widths larger than the stepper's buttons, and both therefore overflow a 35px content region today rather than latently — then the icon circle (40px), the chip (34px) and the vertical pill group (38px), which have no measured symptom either way. Each measured against the content region the way PRs 2 and 3 were, and each made cross-axis flexible, omitted below the floors, or recorded as conforming with the measurement that shows it

**Sequencing.** PR 1 landed first, and PRs 2, 3 and 4 are applications of the contract it settled — either fix taken first would have settled that contract implicitly. PR 2 SHOULD still precede PR 3 because it is the user-visible half, but the reason it might have made PR 3 empty is gone: PR 1 ruled acceptance out, so PR 3 has source changes to make. **Neither PR may resolve its half by moving `--liebe-card-padding`** — the inset is shared, so a change made for the slider moves the stepper too, in the direction that reintroduces the clip under LCARS. PR 1's decision removes the temptation rather than relying on the warning: the slider is bounded by the region instead of being given more of it. PR 4 MUST come last, since it is the same contract applied to parts nothing has measured yet, and it MUST NOT be folded into PR 2 or PR 3 — a measurement that turns up a fourth defect belongs where it can be reviewed on its own.

Expand Down
2 changes: 1 addition & 1 deletion docs/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,5 @@
| 0039 | [Responsive Drag Layout Integrity](changes/0039-responsive-drag-integrity.md) | [Grid Layout](specs/grid-layout/) | complete | — |
| 0040 | [Test & CI Harness Reliability](changes/0040-test-harness-reliability.md) | [Architecture](specs/architecture/) | complete | — |
| 0041 | [Card Conventions & Reference Accuracy](changes/0041-card-conventions-and-reference.md) | [Entity Cards](specs/entity-cards/) | complete | — |
| 0042 | [Tall-Tile Control Geometry](changes/0042-tall-tile-control-geometry.md) | [Design System](specs/design-system/) | draft | — |
| 0042 | [Tall-Tile Control Geometry](changes/0042-tall-tile-control-geometry.md) | [Design System](specs/design-system/) | complete | — |
| 0043 | [Card-Tile Control Semantics & Error Recovery](changes/0043-card-tile-control-semantics.md) | [Design System](specs/design-system/) | draft | — |
2 changes: 1 addition & 1 deletion docs/index.yml
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ changes:
path: changes/0042-tall-tile-control-geometry.md
description: Reconcile the 42px control height and 14px card padding with the ~35px content region a 1-wide desktop tile gives the tall tier — the input_number stepper is clipped there and the vertical slider bleeds into the padding
spec: design-system
status: draft
status: complete
depends_on: []
- id: '0043'
name: card-tile-control-semantics
Expand Down
Loading
Loading