fix: fix /children directive on root fragment (take 2)#19
Conversation
commit: |
Reject explicit fragment role in ARIA templates and preserve root-level children directives at the match boundary. Co-authored-by: Codex <noreply@openai.com>
Reject authored fragment roles and unwrap synthetic fragment wrappers only at the match root boundary. Co-authored-by: Codex <noreply@openai.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Fixes root-level /children directives on ARIA template fragments by treating fragment as an internal root wrapper and correctly applying the root fragment’s containerMode during top-level child-list merging.
Changes:
- Normalize synthetic root
fragmentwrappers inmatchAriaTree, lifting/restoring root/childrencontainer mode semantics. - Reject YAML-authored
fragmentroles at parse time and assert that fragment wrappers are only handled at the root boundary. - Add/adjust tests for root
/children: equal|deep-equalbehavior and root directive rendering.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/aria.test.ts | Adds coverage for root-level /children directives, fragment-role rejection, and root directive rendering. |
| src/aria/template.ts | Renders root fragment /children directive when container mode is non-default. |
| src/aria/match.ts | Applies root fragment containerMode correctly and enforces the “root-only fragment wrapper” invariant. |
| src/aria/folk/isomorphic/ariaSnapshot.ts | Rejects explicit fragment role in YAML parsing to preserve matching invariants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function isAriaTemplateFragement( | ||
| template: AriaTemplateNode | ||
| ): template is AriaTemplateRoleNode { | ||
| return template.kind === 'role' && template.role === 'fragment' | ||
| } |
There was a problem hiding this comment.
isAriaTemplateFragement is misspelled ("Fragement" vs "Fragment"), which makes the helper harder to discover/search and is easy to propagate into new call sites. Rename it to isAriaTemplateFragment (and update all references) to keep naming consistent with the fragment concept used throughout this module.
toMatchAriaInlineSnapshotcannot match empty aria trees vitest#10158/childrendirective on root fragment #14 (supersedes)This PR fixes root-level
/childrendirectives on ARIA templates. Previously, the matcher outright ignoredcontainerModeon the root template fragment, so root/children: equaland/children: deep-equalbehaved like default contain matching.The fix treats
fragmentas an internal root wrapper.matchAriaTreenow lifts the root wrapper’scontainerModebefore unwrapping synthetic actual/template fragments, then passes that mode into the top-level child-list merge. When the match passes, the root directive is restored intoresolved; when it fails, it is dropped consistently with existing failed-branch directive behavior.To keep this model explicit, YAML-authored
fragmentroles are rejected and recursive matching asserts that fragment wrappers have already been unwrapped at the root boundary.