diff --git a/AGENTS.md b/AGENTS.md index e703dec2..2b3d5428 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -220,6 +220,10 @@ The SDLC skills own the phases. What is specific to this repo: - **jsdom lays nothing out, and the dangerous half of that is the assertions it _passes_.** A width assertion against a 0-wide box fails loudly and is easy to diagnose. Its neighbour — `strip.scrollWidth <= strip.clientWidth + 1`, "nothing overflows" — evaluates `0 <= 1` and goes green having measured nothing at all. So the rule for any geometry claim in this repo, in a story or a `__tests__` spec, is that a passing one in jsdom is worth nothing: the runner's `BROWSER_ONLY` map is where such a story is named, with its reason, and the map is self-verifying (a listed story is still executed and must still throw). - **The dispatch guard is process-wide, so a hanging service call contaminates every test after it.** `src/services/guardedDispatch.ts` keeps its pending set at module scope on purpose — the guarantee is about a command reaching Home Assistant at most once, not about one component. A test that leaves a call unsettled (the workshop's `serviceCall: 'pending'` fixture, a mock that never resolves) leaves that command in flight, and the next identical command is **admitted as a success**. That is how `ActionCard/Activating` rendered the success check while asserting the in-flight spinner — a failure that looks exactly like a card defect and is not. `resetDispatchGuard()` in a `beforeEach` is the fix; reach for it before believing a cross-test result. + **Making a class of assertion executable for the first time breaks `main` on merge order alone, and the PR that does it is not the one at fault.** Every branch in flight is a source of assertions that have never run, so the enabling PR is qualified against the assertions that existed when it was tested, and each in-flight PR's assertions are qualified against a runner that ignored them. Neither can see the other; whichever lands second turns the pair red. It happened here within hours: [0037](docs/changes/0037-card-state-and-capability-correctness.md) PR 3 ([#322](https://github.com/fx/liebe/pull/322)) added a `CoverCard/UnknownEntity` story asserting `.liebe-card`, and 0040 PR 6 ([#323](https://github.com/fx/liebe/pull/323)) made play functions execute — each green alone, red together, `main` broken by neither one's content. + + Two things follow. **Re-run the full suite after merging `main`, and read the failure before assuming it is yours** — a detached worktree at `origin/main` with your branch absent settles ownership in one command, and here it showed the failure was main's. And **expect the first run of a newly-executable gate to fail on somebody else's assertion**: the assertion is usually right and the code usually wrong, which is the whole reason it was written and never checked, so the fix is to establish which — not to weaken the assertion so the gate goes quiet. Weakening it would be a defect in the change, and it also discards the finding the gate just bought. + 7. **Playwright's own two prerequisites** A workspace that has never run the suite is missing both the browser and the libraries it links against, and only the first says so plainly: diff --git a/docs/changes/0043-card-tile-control-semantics.md b/docs/changes/0043-card-tile-control-semantics.md index d59cb36f..43167728 100644 --- a/docs/changes/0043-card-tile-control-semantics.md +++ b/docs/changes/0043-card-tile-control-semantics.md @@ -86,7 +86,11 @@ Three decisions this change does take: - [ ] **PR 3 — Keyboard routes to the hold and double-tap actions**: implement what PR 1 settled for the two secondary gestures, so a `glance` card's detail dialog and a card's `doubleTapAction` are reachable without a pointer; coverage per route, including a card whose `holdAction` is `none` - [ ] **PR 4 — The detail dialog carries the failure, and `Dismiss`**: the shell passes the current failure message and the recovery actions to `EntityDetailDialog` while `isError` holds, and pressing the tile in the error state reaches it at every tier; `Dismiss` clears the error and dispatches nothing; the `title` tooltip is no longer what carries the message; coverage including an icon-only tile, whose accessible name already carries the message and whose press had nowhere to go - [ ] **PR 5 — `Retry` re-dispatches the command that failed**: for the service-call state only, per PR 1's scoping — the failed dispatch is retained so the recovery action can repeat it, through the confirmation gate and through the at-most-once guard rather than around either, implementing PR 1's boundary between a failure that releases the guard and an ambiguous one that does not; coverage for the gated case, the guard-refused case, the guard-admitted case and the ambiguous-outcome case -- [ ] **PR 6 — Reconcile the stand-in error tile with the shell's**: `ErrorDisplay variant="card"` implements the contract in a second place and its callsites then withhold the actions — no production callsite passes `onDismiss`, and the entity-not-found path passes no `onRetry` — so the tile renders a button onto a message the user cannot act on. Fold it onto the shell's, or record why a tile that stands in for a card is not a card tile; either way the rendered tile ends the PR actually offering the recovery actions, rather than merely being a component capable of rendering them +- [ ] **PR 6 — Reconcile the stand-in error tile with the shell's**: `ErrorDisplay variant="card"` implements the contract in a second place and its callsites then withhold the actions — no production callsite passes `onDismiss`, and the entity-not-found path passes no `onRetry` — so the tile renders a button onto a message the user cannot act on. Fold it onto the shell's, or record why a tile that stands in for a card is not a card tile; either way the rendered tile ends the PR actually offering the recovery actions, rather than merely being a component capable of rendering them. + + **This PR is closing a stated MUST, not choosing between two defensible treatments.** [design-system — size-adaptive layouts](../specs/design-system/index.md#size-adaptive-layouts) already requires it: "The states a card renders **instead of** itself — loading, error, unavailable — are tiles like any other and MUST take the tier of the card they stand in for". The two tiles that go through `ErrorDisplay variant="card"` — not-found and disconnected — stamp **no `data-tier` at all**, so they are not merely unthemed, they render at the wrong size in breach of that MUST. They also stamp no `liebe-card`, which is the theming handle; both attributes are missing together and for the same reason. + + **Keep those two claims apart, because conflating them is what produced an error here.** _Bypasses `GridCard`_ is true of **all three** lifecycle tiles — which is why none of them carries the shell's edit affordances, the finding this task exists for. _Lacks `liebe-card` and `data-tier`_ is true of **two**. The skeleton is the one already compliant: `SkeletonCard` renders `
` directly and deliberately, "so a theme reshapes the loading tile with the loaded one". **That gives this task its target shape rather than a design decision to make** — one of the three already does the right thing, so the direction is "make the other two consistent with it", and the remedy may be as small as stamping the same two attributes, without making a stand-in tile a `GridCard` and without handing a card whose entity is merely pending the delete and configure affordances. It is measurable now, and was measured the hard way: change [0037](./0037-card-state-and-capability-correctness.md) PR 3 shipped `CoverCard`'s `UnknownEntity` story with `expect(canvasElement.querySelector('.liebe-card')).toBeInTheDocument()`, which was **right about the contract and red against the code**. Nothing ran it — story `play` functions were not executed until [0040](./0040-test-harness-reliability.md) PR 6 made them a gate — so the two landed hours apart, each green alone, and main was broken by the pair. That assertion has been repointed at what the tile says (`Entity Not Found`, and that it names the entity) to unbreak main, and **this PR owns putting a shell assertion back** — on whichever of the three states it decides the contract covers, or a recorded reason there is not one. The repointed story is deliberately weaker on this exact point than its predecessor, and that is the debt this line exists to keep visible. **Sequencing.** PR 1 lands first and everything after it is an application of what it settled; taken in any other order, the first fix to touch the tile decides the element for every card and every theme without the decision being visible. **PR 2 MUST NOT be folded into anything.** It changes the element every card renders into and it sits under the press pipeline that tap, hold and double-tap all route through, so it is the one PR whose regression surface is the whole product — and its own evidence is a suite it did not modify, which only reads as evidence when nothing else in the diff could have moved. PR 3 follows PR 2 because it needs a focusable tile to route keys to. PRs 4 and 5 both need PR 2 as well, and PR 4 precedes PR 5 because `Retry` needs a surface to be offered on; they are not folded together because `Dismiss` is a call to an existing function and `Retry` is a new mechanism, and a PR containing both would be reviewed as the easier one. PR 6 is last: it is the same contract applied to a surface that already satisfies the old reading of it, and its answer depends on what PR 2 made the shell. diff --git a/src/components/CoverCard/CoverCard.stories.tsx b/src/components/CoverCard/CoverCard.stories.tsx index 6dd6be4d..f3ceb18a 100644 --- a/src/components/CoverCard/CoverCard.stories.tsx +++ b/src/components/CoverCard/CoverCard.stories.tsx @@ -215,8 +215,38 @@ export const Disconnected: Story = { export const UnknownEntity: Story = { parameters: { liebe: { entities: [] } }, play: async ({ canvasElement }) => { - await expect(canvasElement.querySelector('.liebe-card')).toBeInTheDocument() - await expect(within(canvasElement).queryByText('Living Room Blinds')).not.toBeInTheDocument() + const canvas = within(canvasElement) + + /* + * Asserted on what the tile SAYS, not on the shell it renders inside. + * + * This used to require `.liebe-card`, and that assertion was correct about + * the contract and wrong about the code — so it described a gap rather than + * a regression, and went unnoticed until change 0040 PR 6 made play + * functions execute. + * + * The gap is narrower than "this tile is not a card". `liebe-card` is what + * carries a tile's geometry and theming, and `SkeletonCard` stamps it + * directly — `
` — precisely so + * "a theme reshapes the loading tile with the loaded one". The two tiles + * that go through `ErrorDisplay variant="card"`, not-found and + * disconnected, stamp neither it nor `data-tier`, so those two lose the + * theming handle and the tier geometry. Restoring it is a change to a + * shared component's rendered surface rather than an unbreak, and + * docs/changes/0043-card-tile-control-semantics.md PR 6 owns it — including + * restoring an assertion here. + * + * What replaces it is stronger for this story's own purpose, and not + * because the old assertion was undiscriminating — it was not. + * `querySelector('.liebe-card')` returns a node for a skeleton and `null` + * for the two error tiles, so it separates the skeleton from them; what it + * cannot do is separate THIS tile from the disconnected one. `Entity Not + * Found` does, and the message names the entity, which the class never did + * and which is the half the docstring above is actually about. + */ + await expect(canvas.getByText('Entity Not Found')).toBeInTheDocument() + await expect(canvas.getByText(entityId, { exact: false })).toBeInTheDocument() + await expect(canvas.queryByText('Living Room Blinds')).not.toBeInTheDocument() }, }