Repository navigation
docs(spec): ChatMessageList records its shipped contract as current - #9270
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This record promotion waits on an engineering owner's decision: any engineering owner, the author included if they are one, approves the proposed current text by commenting /approve-spec <full head SHA> on this pull request. Whatever that decision, a few fixes are needed before the record can merge as current:
- Restate FR5 as where a short list rests (block end for
bottom, block start fortop) rather than requiring the spacer element, which the record's own Current-state impact says conflicts with component-style-authoring INV7. - Fix the verification map. The FR8
data-testidtest still passes with the...restspread removed, andthemingTargets.test.tsdoes not fail when density reflection is lost. The gap override, the density spacings and the observer root have no failing test but are missing from "Read from source". - Align the description with the record: the record lists one implementation gap, and PR2 names an Effect as the registration mechanism.
[Automated review]
| | FR2 | The chosen density is reflected as `data-density` on the root and flows to child messages through `ChatListContext`; it selects three spacing groups. | Current source, tests, and docs | | ||
| | FR3 | The optional `gap` replaces only the top-level row gap; density still selects padding and context. | Current source, tests, and docs | | ||
| | FR4 | `emptyState` renders only when `children` is nullish, `false`, or an empty top-level array. Otherwise the passed children render unchanged. | Current source and tests | | ||
| | FR5 | `align="bottom"` includes a flex spacer before messages; `align="top"` omits it. Overflowing content is not given a different rendering path. | Current source and tests | |
There was a problem hiding this comment.
FR5 makes the spacer element binding, but the Current-state impact above lists that same element as a component-style-authoring INV7 gap. Consider stating the placement instead (with bottom a short list rests at the block end, with top at the block start, and overflow renders the same), so the gap can close without a spec change.
| - An Effect registers the inner content element with ChatLayout, then clears | ||
| that registration on cleanup. This draft does not prescribe an internal hook | ||
| or listener implementation. | ||
| - **PR2 — Layout registration.** An Effect registers the inner content element |
There was a problem hiding this comment.
PR2 now names an Effect as the registration mechanism; the draft explicitly left that open. FR7 already states the observable registration, so this line could drop the mechanism and keep an equivalent callback-ref registration conformant.
| | FR4 | `ChatMessageList.test.tsx` empty-state tests | empty; numeric empty state | A missing or hidden empty state fails. | `audit:ChatMessageList/§4` | | ||
| | FR5 | `ChatMessageList.test.tsx` spacer tests | default, bottom, top | A spacer that appears or vanishes wrongly fails. | `audit:ChatMessageList/§4` | | ||
| | FR6 | `ChatMessageList.test.tsx` "loads older messages through the top sentinel and exposes pending status" | pending, settled | A lost action or stuck spinner fails. | `audit:ChatMessageList/§6` | | ||
| | FR8 | `ChatMessageList.test.tsx` `data-testid` test | data prop | A dropped prop fails. | `audit:ChatMessageList/§3` | |
There was a problem hiding this comment.
This test cannot fail on the passthrough: data-testid is destructured and re-applied explicitly in ChatMessageList.tsx, so deleting {...rest} keeps every test green. The Chromium spec's aria-label assertion exercises the spread; credit it, or list DOM, data and ARIA passthrough under "Read from source".
There was a problem hiding this comment.
This record promotion waits on an engineering owner's decision: any engineering owner, the author included if they are one, approves the proposed current text by commenting /approve-spec <full head SHA> on this pull request. FR5, PR2 and the description now read well, and a few fixes remain before the record can merge as current:
- The Representative states row "Bottom includes a spacer, top omits it" is now a required invariant; state the resting position there and in the Owns bullet, as FR5 does.
- The FR5 verification row credits spacer-count tests that still pass when a short bottom-aligned list rests at the top; list FR5's placement under "Read from source" instead.
- FR1's
aria-live="polite"is neither credited nor listed; credit the Chromium spec's assertion or add it to the list.
The red "Touched components" check comes from this branch predating that script on main; a rebase clears it.
[Automated review]
| ### Representative states | ||
|
|
||
| | State | Observed result | Allowed variation | | ||
| | State | Required invariant | Allowed variation | |
There was a problem hiding this comment.
With this column now Required invariant, the short top/bottom row below ("Bottom includes a spacer, top omits it") requires the spacer that FR5 and Current-state impact call an INV7 gap. Consider "Bottom rests at the block end, top at the block start", and the same wording for the Owns bullet.
| | FR8 | `themingTargets.test.ts`, focused root passthrough inspection | ref, DOM/ARIA, styling | Consumer inputs fail to reach or compose on the root | `audit:ChatMessageList/§3` | | ||
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | -------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------- | ------------------------------------------------------------ | -------------------------- | | ||
| | FR1, AR1 | `ChatMessageList.test.tsx` role and `aria-busy` tests | rest, streaming | A dropped log role or busy state fails. | `audit:ChatMessageList/§1` | |
There was a problem hiding this comment.
aria-live="polite" is part of FR1, but no jsdom test fails when it is removed and the "Read from source" list names only tabIndex. The Chromium spec asserts it (expect(observed.live).toBe('polite')); credit that here or list aria-live below.
| | FR2 | `ChatMessage.test.tsx` "inherits list density and lets an explicit prop override it"; `ChatMessageBubble.test.tsx` list-density case | child message in a list | Density that stops reaching child messages fails. | `audit:ChatMessageList/§2` | | ||
| | FR7 | `ChatLayout.test.tsx` "first-fill scroll positioning" cases; `useChatNewMessages.test.tsx` "works end-to-end with ChatLayout conditional ChatMessageList" | list inside ChatLayout | A list that stops registering its content element fails. | `audit:ChatMessageList/§7` | | ||
| | FR4 | `ChatMessageList.test.tsx` empty-state tests | empty; numeric empty state | A missing or hidden empty state fails. | `audit:ChatMessageList/§4` | | ||
| | FR5 | `ChatMessageList.test.tsx` spacer tests | default, bottom, top | A spacer that appears or vanishes wrongly fails. | `audit:ChatMessageList/§4` | |
There was a problem hiding this comment.
These tests count [aria-hidden] elements, so they still pass when the spacer stops growing and a short bottom-aligned list rests at the top, and they would fail under the INV7 fix while FR5 holds. Consider listing FR5's resting position under "Read from source" and naming the spacer tests as covering the gap.
| - FR7: removing the registration on cleanup. | ||
| - FR8: `ref`, DOM, data, and ARIA props, `className`, `style`, and `xstyle` on | ||
| the root, and owned log semantics winning over forwarded props. | ||
| - PR1–PR2: the observer's root, and observer and registration cleanup. |
There was a problem hiding this comment.
Observer cleanup is tested: the FR6 sentinel test asserts disconnect on unmount and fails when observer.disconnect() is removed. Only the observer's root and the registration cleanup belong here.
…d corrects FR1/FR5 evidence
950371e to
b5a62c4
Compare
There was a problem hiding this comment.
This record promotion waits on an engineering owner's decision: any engineering owner, the author included if they are one, approves the proposed current text by commenting /approve-spec <full head SHA> on this pull request. The empty-state row and the FR4 verification wording now match what ships, and every credited test fails when its claim is broken, so nothing else is required before that approval. Optionally, one accessibility line linking the status-announcements spec (AST-086) would record that the list announces its loading through the shared channel.
[Automated review]
| interactive button or directional keyboard behavior of its own. | ||
| - Caller-supplied content remains responsible for names and semantics inside | ||
| messages. This record does not claim one browser/AT pairing proves another. | ||
| - **AR2 — No keyboard model.** The loading spinner is a separate child of the |
There was a problem hiding this comment.
Optional: the loader is also announced through the shared channel, which spec:AST-086 owns (FR4, FR8; useBusyAnnouncement in ChatMessageList.tsx). One clause here linking AST-086 would keep a reader of this contract from concluding that loading is unannounced.
|
/approve-spec d334dda |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
This record promotion waits on an engineering owner's decision: any engineering owner, the author included if they are one, approves the proposed current text by commenting /approve-spec <full head SHA> on this pull request. The earlier review points are all addressed, and the verification map now matches what the tests prove. One fix remains before the record can merge as current, and making it before approval avoids a second approval:
- FR5's "Overflowing content renders the same either way" is not what ships: with
align="bottom"the collapsed spacer keeps one row gap (16px at balanced density) above the first message, so either go back to "takes the same rendering path" or state the gap and add it to the Current-state impact.
[Automated review]
| | FR2 | The chosen density is reflected as `data-density` on the root and flows to child messages through `ChatListContext`; it selects three spacing groups. | Current source, tests, and docs | | ||
| | FR3 | The optional `gap` replaces only the top-level row gap; density still selects padding and context. | Current source, tests, and docs | | ||
| | FR4 | `emptyState` renders only when `children` is nullish, `false`, or an empty top-level array. Otherwise the passed children render unchanged. | Current source and tests | | ||
| | FR5 | With `align="bottom"` a short list rests at the block end; with `align="top"` it rests at the block start. Overflowing content renders the same either way. Implementation gap: the bottom placement uses an added spacer element (Current-state impact). | Current source and tests | |
There was a problem hiding this comment.
"Overflowing content renders the same either way" is not what ships: the spacer is a flex item in a box with a row gap (ChatMessageList.tsx:145-148, :325-327), so when it collapses to 0px the gap beside it stays, and an overflowing bottom list starts one row gap (16px at balanced density) lower than top. Consider "Overflowing content takes the same rendering path", or state the gap here and in Current-state impact.
There was a problem hiding this comment.
This record promotion waits on an engineering owner's decision: any engineering owner, the author included if they are one, approves the proposed current text by commenting /approve-spec <full head SHA> on this pull request. The FR5 overflow wording and its Current-state impact now match what ships; one more correction is needed before the record can merge as current, and making it before approval avoids a second approval:
- The Representative states row "Optional centered empty state" is now a required invariant, but with the default
align="bottom"the spacer takes the upper half of the free space, so the empty state sits about three-quarters down the list; state where it rests for each alignment and add it to the Current-state impact beside the overflow row gap. - Minor: the FR4 verification row says a hidden empty state fails, but both empty-state tests stay green when the wrapper is hidden; "a missing empty state, or a dropped numeric one" matches what they prove.
[Automated review]
| | State | Required invariant | Allowed variation | | ||
| | ----------------------------- | ----------------------------------------------------- | ------------------------------------------ | | ||
| | Two messages and a system row | Polite log with messages in source order | Caller controls row content | | ||
| | Empty top-level children | Optional centered empty state | An omitted empty state leaves an empty log | |
There was a problem hiding this comment.
This row is now a required invariant, but under the default align="bottom" the spacer and the empty wrapper are both flex: 1, so the empty state is centered only in the lower half: in the Empty story's 320px list its text sits at 236px, not 160px. Consider "Centered in the space below the spacer for bottom, in the whole list for top", and add it to the Current-state impact beside the overflow row gap.
| | FR2–FR3 | `ChatMessageList.test.tsx` density and gap tests | densities; gap override | Lost density reflection or a gap that changes density fails. | `audit:ChatMessageList/§2` | | ||
| | FR2 | `ChatMessage.test.tsx` "inherits list density and lets an explicit prop override it"; `ChatMessageBubble.test.tsx` list-density case | child message in a list | Density that stops reaching child messages fails. | `audit:ChatMessageList/§2` | | ||
| | FR7 | `ChatLayout.test.tsx` "first-fill scroll positioning" cases; `useChatNewMessages.test.tsx` "works end-to-end with ChatLayout conditional ChatMessageList" | list inside ChatLayout | A list that stops registering its content element fails. | `audit:ChatMessageList/§7` | | ||
| | FR4 | `ChatMessageList.test.tsx` empty-state tests | empty; numeric empty state | A missing or hidden empty state fails. | `audit:ChatMessageList/§4` | |
There was a problem hiding this comment.
Rendering the empty wrapper with hidden keeps both empty-state tests green, since getByText and toHaveTextContent ignore visibility. Consider "A missing empty state, or a dropped numeric one, fails."
Exact decision needed
Promote
component:ChatMessageListfrom draft to current. No behavior, prop, default, target, or export changes.Existing authority searched
packages/core/src/Chat/ChatMessageList.spec.mdand the component's source, tests, and consumer docs on main.spec:AST-029owns the audit evidence inventory the verification map follows; the record's other cited owners are unchanged.Decision
component:ChatMessageList,authority: current, approved by cixzhang. Draft and audit wording is removed, basis cells read from current source and tests, and the verification map credits only tests that fail when a claim breaks, with a "Read from source, with no focused test yet" list for the rest. One implementation gap: bottom alignment uses an added spacer element (architecture:component-style-authoringINV7), recorded under Current-state impact. OQ1–OQ2 stay open.Rejected alternatives
None.
Claim scope and non-goals
Claims are scoped to what main's source and tests show.
Evidence and implementation relationship
The component's colocated tests, as listed in the verification map.
Writing rubric results
Scope
Validation
pnpm check:knowledge: alignedgit diff --check: clean