fix(cover): assert the not-found tile's message, not the shell it lacks - #327
Conversation
The UnknownEntity story required `.liebe-card` on a card that renders no card: `liebe-card` is the shell GridCard emits, and all three lifecycle tiles - skeleton, not-found, disconnected - render outside it through a Radix Card. The assertion was right about the contract and red against the code, and nothing executed it until story play functions became a gate, so it and that gate landed hours apart and main was broken by the pair rather than by either one's content. Repointed at what the tile says rather than weakened: `Entity Not Found`, which is what separates this tile from the disconnected one and from a skeleton - `.liebe-card` could tell none of the three apart - plus that the message names the entity, which is what the story's own docstring promises. The negative assertion on the friendly name is unchanged. Folding the tiles onto the shell is a contract decision, not an unbreak, and 0043 PR 6 already owns it. Its task line now records that all three tiles are outside the shell, that this story had to give up the shell assertion to unbreak main, and that PR 6 owns putting one back or recording why there is not one.
When a PR makes a class of assertion executable for the first time, every other PR in flight is a source of assertions that have never run. The enabling PR is qualified against the assertions that existed when it was tested; each in-flight PR's assertions are qualified against a runner that ignored them. Neither can see the other, and whichever lands second turns the pair red. Adds the two consequences: re-run the suite after merging main and establish whose the failure is before assuming it is yours - a detached worktree at origin/main answers it in one command - and expect the first run of a newly-executable gate to fail on somebody else's assertion, which is usually right and usually finding a real gap. Weakening it to quiet the gate is a defect in the change and throws away what the gate just bought.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
Pull request overview
Unbreaks main by updating the CoverCard/UnknownEntity Storybook play assertions to validate the not-found tile’s user-facing output (title + entity id) rather than asserting the presence of the .liebe-card shell, and records the resulting contract/decision debt in docs.
Changes:
- Repoint
CoverCard/UnknownEntitystory assertions to check forEntity Not Foundand the missingentityId. - Update change doc 0043 task notes to record the lifecycle-tile vs shell mismatch and track restoring (or explicitly rejecting) a shell-level assertion.
- Add an AGENTS.md note about newly-executable gates breaking
mainbased on merge order (and how to triage ownership).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/CoverCard/CoverCard.stories.tsx | Updates UnknownEntity story play assertions to validate the not-found tile’s content instead of .liebe-card. |
| docs/changes/0043-card-tile-control-semantics.md | Records lifecycle tile rendering/shell relationship as explicit debt owned by PR 6 of change 0043. |
| AGENTS.md | Documents the “new gate makes old assertions executable” merge-order failure mode and expected triage steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
==========================================
+ Coverage 88.14% 88.34% +0.20%
==========================================
Files 233 233
Lines 8461 8461
Branches 2847 2847
==========================================
+ Hits 7458 7475 +17
+ Misses 801 788 -13
+ Partials 202 198 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Both the story comment and 0043 PR 6's task line claimed all three
lifecycle tiles render outside `liebe-card`. That is false for the
skeleton: SkeletonCard renders `<div className="liebe-card"
data-tier={tier}>` directly, and deliberately, "so a theme reshapes the
loading tile with the loaded one". Only the two that go through
ErrorDisplay variant="card" - not-found and disconnected - stamp neither
the class nor the tier.
The correction narrows the task rather than widening it, and points at a
smaller remedy: stamping the two attributes may be enough, without making
a stand-in tile a GridCard and without handing a card whose entity is
merely pending the delete and configure affordances. Caught by codex
review; the claim would have sent PR 6 after behaviour that is already
correct and already reasoned about in the skeleton's own doc comment.
Two corrections Copilot raised on the first draft. The story comment still carried "`.liebe-card` could not tell them apart", which reasoned from the claim the previous commit had already fixed. It is false: querySelector returns a node for a skeleton and null for the two error tiles, so the class separates the skeleton from them - what it cannot separate is not-found from disconnected. The repoint rests on the right ground now: the new assertions name the state and pin the entity id, which the class never did. And the missing attribute that matters is `data-tier`, not the theming handle. design-system - size-adaptive layouts already requires that the states a card renders instead of itself "MUST take the tier of the card they stand in for", and the two ErrorDisplay tiles stamp no tier at all, so they render at the wrong size in breach of a stated MUST. 0043 PR 6's task line now leads with that, keeps "bypasses GridCard" (all three) apart from "lacks the class and the tier" (two), and records that the skeleton is the one already compliant - which gives that PR a target shape rather than a design decision.
Summary
mainis red and this unbreaks it.CoverCard'sUnknownEntitystory assertedexpect(canvasElement.querySelector('.liebe-card')).toBeInTheDocument()on a card that renders no card:liebe-cardis the shellGridCardemits, and all three lifecycle tiles — skeleton, not-found and disconnected — render outside it, through a RadixCardinrenderCardLifecycle. The assertion was right about the contract and red against the code.The gap is narrower than "this tile is not a card", and I had it wrong at first.
liebe-cardcarries a tile's geometry and theming, andSkeletonCardstamps it directly —<div className="liebe-card" data-tier={tier}>— deliberately, "so a theme reshapes the loading tile with the loaded one". Only the two tiles that go throughErrorDisplay variant="card", not-found and disconnected, stamp neither it nordata-tier. My first draft said all three were outside the shell;codex reviewcaught it, and the correction narrows PR 6 rather than widening it — the remedy may be as small as stamping the same two attributes, without making a stand-in tile aGridCardand without giving a card whose entity is merely pending the delete and configure affordances.The story is repointed, not weakened. It now asserts what its own docstring says the state is for — "the card reports it missing and names it" — with
Entity Not Foundand the message naming the entity. That is strictly stronger for this story's purpose than what it replaces: all three lifecycle tiles are "something rendered instead of the card", and.liebe-cardcould not tell them apart, whileEntity Not Foundseparates this one from the disconnected tile and from a skeleton. The negative assertion on the friendly name is unchanged.The contract decision is deferred, deliberately. Restoring the selector is a change to a shared component's rendered surface, not an unbreak, and
docs/changes/0043-card-tile-control-semantics.mdPR 6 already owns it: "fold it onto the shell's, or record why a tile that stands in for a card is not a card tile". Its task line now records which two tiles are affected, that the skeleton is the counter-example showing how small the fix may be, that this story had to give up its shell assertion to unbreak main, and that PR 6 owns putting one back or recording why there is not one. That debt is deliberately visible rather than quietly settled here.0043stays**Status:** draft;docs/index.ymlanddocs/index.mdare untouched.Why main broke, which is the part worth keeping
Two PRs, each green alone, red together, and neither one's content at fault:
UnknownEntitystory with that assertion. Nothing executed it — storyplayfunctions were not run by any gate.playfunctions execute, qualified against the stories that existed when it was tested.Neither could see the other; merge order alone decided who discovered it. The irony is also the diagnosis: #323 exists to enforce
REVIEW.md's "an assertion no gate executes is documentation, not verification", and the first thing it caught was an assertion from the PR that landed beside it. The gate is working.Generalised into
AGENTS.mdas a separatedocs:commit: when a PR makes a class of assertion executable for the first time, every other PR in flight is a source of assertions that have never run, so the first run of a newly-executable gate should be expected to fail on somebody else's assertion — which is usually right, and usually finding a real gap. The two consequences recorded there are to re-run the suite after mergingmainand establish whose the failure is before assuming it is yours (a detached worktree atorigin/mainsettles it in one command), and never to weaken such an assertion to quiet the gate.Review
codex review --base mainraised two findings and both were right about the facts: my "all three tiles" claim was false for the skeleton, and the task line as first written could have sent PR 6 after behaviour that is already correct. Both are fixed in88371c4. Its third suggestion — keep the.liebe-cardassertion and fix the error surface in this PR instead — is declined for this PR only, and recorded rather than dismissed: it is the right end state, it is PR 6's subject, and an unbreak that also changes what a shared component renders is not one that should land unreviewed and fast. Local CodeRabbit (cr --agent --base main, not rate-limited): 0 findings.Verification
origin/mainfails this spec with the branch absent — checked in a detached worktree atorigin/mainrather than inferred:Tests 1 failed | 12 passed | 649 skipped (662). After this change,npm testis255 passed (255)/6600 passed (6600).Probes — both caught, both executing the expected 13 tests, worktree clean before the first mutation and after every restore:
Entity Not Found→Error)Both mutate toward what the new assertions forbid rather than toward what they happen to check. A first run scored both INVALID on the test count and was discarded rather than reported:
bcis not installed here, so the counter returned empty — the probes were fine and the arithmetic was not, which is exactly the case the count check exists to separate from a real result.Gates
Each run unpiped, exit status read:
npm testexit 0 (6600 passed) ·npm run lintexit 0 (0 errors, the one pre-existingMigrationCutoffswarning; Prettier clean) ·npm run typecheckexit 0 ·npm run test:coverageexit 0 ·npm run build-storybookexit 0.Patch coverage checked on both
DA:andBRDA:: the only changed source file isCoverCard.stories.tsx, which is excluded from the LCOV report, so there are no patch lines to cover — itsplayfunction is nonetheless executed bysrc/__tests__/stories.test.tsx, which is what the probes above confirm.