Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
a021fd0 to
cdd4b51
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner deciding whether the seven Swift rule records in packages/native/specs/swift/ should land as authority: current: their sources on main say Status: draft, and no approval covers this head yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner; otherwise the records can land as authority: draft. Either way, these record fixes are needed:
theme-generator.spec.mdG3 still requiresswift-ios-1/core/Theme/Generated/paths, which contradicts AST-093 FR1 and the record's own Owning code (packages/native/apple/Sources/Astryx/).- The Current-state notes in
styles,theme-environment,component-apiandtheme-generatorsay the Swift views, theme and generator do not exist yet. accessibility.spec.mdcalls itself Draft in its Contract-at-a-glance table, andpackages/native/apple/README.mdstill links the deletedlibrary/theme-generator.md.
The test-node (1/4) failure is the watch-mode case in build-theme.icons-specifier.test.mjs, outside this change, and it passes when run on its own at this head.
[Automated review]
| ## Owning code | ||
|
|
||
| - `packages/native/scripts/` — the theme generator and its drift check. | ||
| - `packages/native/apple/Sources/Astryx/` — the generated Swift themes. |
There was a problem hiding this comment.
This names packages/native/apple/Sources/Astryx/ for the generated themes, but G3 (lines 65 and 73) still requires swift-ios-1/core/Theme/Generated/Tokens.swift and swift-ios-1/core/Theme/Generated/<SwiftName>Theme.swift. AST-093 FR1 puts Swift in packages/native/apple/, and the generator's SWIFT_THEME_DIR is apple/Sources/Astryx/Theme/Generated/, so G3's two paths need updating before this record is current.
| | System behavior | Name, role, state, and value come from the core contract; disabled controls stay reachable; covered messages announce once after load; every setting in X6 is honored. | | ||
| | End-user impact | A VoiceOver user on iPhone hears the same name, role, state, and messages as a screen-reader user on the web; people using Reduce Motion, Bold Text, and the other settings get them honored. | | ||
| | Builder impact | A Swift component author maps the core contract to SwiftUI accessibility APIs by these rules instead of deciding them per component. | | ||
| | Compatibility/readiness | Draft. No Swift component yet has accessibility tests or an audit. | |
There was a problem hiding this comment.
The frontmatter says authority: current, but this projection says "Draft." The template uses this cell for the record's authority and implementation state, so it should say current, keeping the note that no Swift component has accessibility tests or an audit yet.
cdd4b51 to
6a7da52
Compare
6a7da52 to
e5f77a9
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner deciding whether the seven Swift rule records in packages/native/specs/swift/ land as authority: current: their sources on main say Status: draft, and no approval covers this head. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner; otherwise the records can land as authority: draft, and since the branch now conflicts with main in packages/native/apple/Sources/Astryx/Icon/Icon.spec.md, the decision belongs on the rebased head. Either way, these fixes are needed:
validatePlatformComponentRecordaccepts a Swift record saved in another component's folder (for exampleIcon/Banner.spec.md); require the folder to match the record's name, as Core and Lab records already must.- The Current-state notes in
styles.spec.md,theme-environment.spec.mdandslots.spec.mdsay the Swift views, theme and icon slot do not exist. accessibility.spec.mdstill calls itself Draft in its Contract-at-a-glance table, and the Swift README's "the record wins" sentence ranks these records below every other record.
[Automated review]
| * keeps the shared contract; until it has one, the core component's public | ||
| * source and documentation (`packages/core/src/<PublicName>/`) stand in. | ||
| */ | ||
| function validatePlatformComponentRecord(root, record, location, recordsById) { |
There was a problem hiding this comment.
This derives the id and platform_of from the file name only, and the platform branch at line 1019 skips the root check that Core and Lab records get at line 1057. On this head packages/native/apple/Sources/Astryx/Icon/Banner.spec.md (id component:swift/Banner, platform_of: component:Banner) passes check:knowledge, and specs:for then routes the Icon folder's files to Banner's records. The knowledge-contracts Owning code counts the platform component directories as component roots, where "the public name normally matches the root", and AST-093 FR1 gives each native component a folder named for it. Requiring location.publicName === location.componentRoot here, with a test beside the other DEC-13 cases, closes it.
| Rules shared by every Astryx component written in Swift, for iOS and macOS, as | ||
| architecture records (`architecture:native-swift-<slug>`), one concern per | ||
| file. A component spec follows these and records only where its component | ||
| differs. Where one of them and an approved record disagree, the record wins. |
There was a problem hiding this comment.
Now that these files are architecture records, "Where one of them and an approved record disagree, the record wins" gives every other approved record precedence over them once they are current, which knowledge-contracts INV9 rules out ("A newer, narrower, or more local current record does not silently override another current record"). Dropping the sentence, or naming the records that win (AST-082 and AST-093, as AST-093 FR1 does), keeps the README consistent.
e5f77a9 to
011c380
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner deciding whether the seven Swift rule records in packages/native/specs/swift/ land as authority: current: their sources on main say Status: draft, and no approval covers this head. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner; otherwise the records can land as authority: draft, and since a new commit makes an approval stale, the decision belongs on the head that carries the fixes below. These fixes are needed:
validatePlatformComponentRecordaccepts a Swift record saved in another component's folder (for exampleIcon/Banner.spec.md); require the folder to match the record's name, as Core and Lab records already must.- Before the records are current: the Current-state notes in
styles.spec.md,theme-environment.spec.mdandslots.spec.mdsay the Swift views, theme and icon slot do not exist,accessibility.spec.mdcalls itself Draft in its Contract-at-a-glance table, and the Swift README's "the record wins" sentence ranks these records below every other record.
[Automated review]
011c380 to
4d4837b
Compare
e95b58d to
31ffdb3
Compare
There was a problem hiding this comment.
Approval waits on two engineering-owner decisions: whether the seven Swift rule records in packages/native/specs/swift/ land as authority: current (their sources on main say Status: draft, and no approval covers this head or #8500's), and whether AST-093 FR1 places a member component's Swift record in the folder where core keeps it, as this checker requires (#8593 covers members that have a core record, not ones like ListItem that core documents only through a .doc.mjs). An engineering owner records each decision by commenting /approve-spec <full head SHA> on the pull request that lands that spec text, and the author may do so as an engineering owner; otherwise the records land as authority: draft and the checker accepts each component's own folder. Two changes are needed whichever way those decisions go, and a third if family folders are approved:
- Land #8500 first and rebase this branch without its commit: at 2,143 authored lines, this head is over the 2,000-line limit.
scripts/migrate-knowledge-v5.mjscrashes with ENOENT when a tracked*.spec.mdhas been moved or deleted but not staged; skip paths that no longer exist, ascheck:knowledgedoes.pnpm specs:forsends every file inSources/Astryx/Avatar/to AvatarStatusDot's records as well; route a member's record only to its own same-stem files, as core does.
[Automated review]
| if (record.platform) { | ||
| const platformRoot = `${record.platform.componentTree}/${record.platform.componentRoot}/`; | ||
| if (filePath.startsWith(platformRoot)) { | ||
| matches.push({ | ||
| record, | ||
| via: 'placement', | ||
| detail: `platform component record for ${platformRoot}`, | ||
| }); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This matches a platform record to every file in its folder, so with the Swift AvatarStatusDot record in Avatar/, pnpm specs:for --include-draft packages/native/apple/Sources/Astryx/Avatar/Avatar.swift also lists component:swift/AvatarStatusDot, core's component:AvatarStatusDot through platform_of, and the records only AvatarStatusDot inherits. Core routes Avatar.tsx to component:Avatar alone, because a member record matches only its same-stem files (lines 305-320). Applying the same rule here, a record whose name matches its folder covering the folder and a member covering only its same-stem files, keeps Swift lookups in line with core.
| for (const filePath of new Set(specRecordFiles(ROOT))) { | ||
| const absolute = path.join(ROOT, filePath); | ||
| const migrated = migrateComponentRecord(fs.readFileSync(absolute, 'utf8')); |
There was a problem hiding this comment.
git ls-files --cached --others still lists a tracked *.spec.md that was moved or deleted without staging, so this read throws ENOENT. For example, moving the Swift AvatarStatusDot record into AvatarStatusDot/ with a plain mv, or deleting packages/core/src/Kbd/Kbd.spec.md before git rm, makes both --check and a plain run exit 1 with a stack trace. check:knowledge filters these paths with fs.existsSync (scripts/check-knowledge.mjs:727-730); the same filter in specRecordFiles, with a test beside the untracked-file case, keeps the two in step.
| if (!coreFolders.has(location.componentRoot)) { | ||
| const places = [...coreFolders] | ||
| .sort() | ||
| .map(folder => `${location.componentTree}/${folder}/`) | ||
| .join(' or '); | ||
| problems.push( | ||
| `${record.filePath}: a platform component record lives in the folder where core keeps its component: ${location.publicName}.spec.md belongs in ${places}.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
For the 80 core names without their own packages/core/src/<Name>/ folder, this refuses Sources/Astryx/<Name>/<Name>.spec.md, the path AST-093 FR1 gives every native component. Whether family folders are allowed is an engineering-owner decision on FR1. #8593's proposed sentence covers the 48 members that have a core record in another folder (AvatarStatusDot), but not the 32 that core documents only through a .doc.mjs, such as ListItem in List/, which this check also places by their doc's folder.
31ffdb3 to
27f006c
Compare
There was a problem hiding this comment.
Approval waits on two engineering-owner decisions: whether the seven Swift rule records in packages/native/specs/swift/ land as authority: current (their sources on main say Status: draft, and no approval covers this head or #8500's), and whether AST-093 FR1 is amended so a member component's Swift record sits in the folder where core keeps it, as this checker requires (#8593 proposes that amendment and is not yet approved). An engineering owner records each decision by commenting /approve-spec <full head SHA> on the pull request that lands that spec text, and the author may do so as an engineering owner; otherwise the records land as authority: draft and the checker accepts each component's own folder. One change is needed whichever way those decisions go, and one more if family folders are approved:
- Land #8500 first and rebase this branch without its commit: at 2,208 authored lines, this head is over the 2,000-line limit.
packages/native/specs/README.mdandpackages/native/specs/swift/README.mdstill give every componentSources/Astryx/<Name>/<Name>.spec.md, so they need the member case if family folders are approved.
[Automated review]
| - **A native component's spec** sits in the component's own folder beside its | ||
| implementation, the way core keeps `TabList.spec.md` next to `TabList.tsx`: | ||
| `packages/native/apple/Sources/Astryx/<Name>/<Name>.spec.md`. It is a |
There was a problem hiding this comment.
This gives every native component Sources/Astryx/<Name>/<Name>.spec.md, but scripts/check-knowledge.mjs:954-965 refuses that path for a member core keeps in another folder: with the Swift AvatarStatusDot record moved to AvatarStatusDot/, the check says it "belongs in packages/native/apple/Sources/Astryx/Avatar/", and ListItem/ListItem.spec.md is refused in favour of List/. If the AST-093 FR1 amendment in #8593 is approved, this bullet and packages/native/specs/swift/README.md:20-21 need the member case; if it is not, the checker should accept the own folder instead.
27f006c to
93c9740
Compare
93c9740 to
fb53f32
Compare
There was a problem hiding this comment.
Approval waits on two engineering-owner decisions. The first is whether the seven Swift rule records in packages/native/specs/swift/ land as authority: current: their sources on main say Status: draft, and no approval covers this head or #8500's. The second is whether AST-093 FR1 is amended so a member component's Swift record sits in the folder where core keeps it, as this checker requires (#8593 proposes that amendment and is not yet approved). An engineering owner, the author included, records each decision by commenting /approve-spec <full head SHA> on the pull request that lands that spec text; otherwise the records land as authority: draft and the checker accepts each component's own folder.
- Whichever way those go, land #8500 first and rebase this branch without its commit: at 2,208 authored lines, this head is over the 2,000-line limit.
[Automated review]
Maintainer impact
architecture:knowledge-contractsINV27 and DEC-13 say every specification is a knowledge record, butpnpm check:knowledgeonly reads records indocs/, themes, and the component packages'src/. The seven Swift specs underpackages/native/specs/swift/library/and the package's ownTEMPLATE.mdskip structure checks, the spec-owner gate, andpnpm specs:forrouting, and nothing stops the next package from inventing its own spec format.Intended invariant
architecture:knowledge-contractsINV27 and DEC-13, and AST-093 FR1:*.spec.md, or Markdown under aspecs/directory, apart fromREADME.md,plan.md, and the paths discovery ignores) is a valid record at its kind's location, orcheck:knowledgefails;packages/native/apple/Sources/Astryx/<Name>/<Name>.spec.md, idcomponent:swift/<Name>, and names its core component inplatform_of;packages/<package>/specs/**/<slug>.spec.md, with the id that path gives.Change
.github/scripts/knowledge-paths.cjs,scripts/check-knowledge.mjs):platform_ofmust namecomponent:<Name>. That resolves to a Core or Lab record. Until one exists,packages/core/src/<Name>/or a<Name>.doc.mjsunderpackages/core/src/stands in. The record sits where core keeps that component (AST-093 FR1, with spec(AST-093): a native component lives in the folder where core keeps it (FR1) #8593): its own folder when core uses one, or the folder core keeps it in, as core keeps AvatarStatusDot inAvatar/and ListItem inList/.platform_of: null.architectureand the id comes from the path.docs/schemas/knowledge/v5.json) addsplatform_ofto component records and lets component ids carry a language (component:swift/Button). The component template moves to v8.node scripts/migrate-knowledge-v5.mjsmoves every component record to schema 5 withplatform_of: nullafterid, and changes nothing else. It is deterministic and idempotent, and--checklists records that still need it. Its output is the separate final commit. Thecheck:knowledgemessage for an unmigrated component record names the command.pnpm specs:forsends a Swift file to its platform record and, throughplatform_of, to the core record.change-scopetreats platform records as spec records and package records like architecture records, so the owner gate covers both.Rebasing
Component records keep landing on
main. To rebase: drop the finalchore(knowledge): move component records to schema v5commit, rebase the other two, runnode scripts/migrate-knowledge-v5.mjs, and commit its output as the final commit again.Evidence
main,packages/native/specs/swift/library/*.mdhave no frontmatter, andcheck:knowledgepasses. On this branch, the same files at their old paths each fail with an INV27 message (covered incheck-knowledge.test.mjs).node scripts/check-knowledge.mjsand--base origin/mainprint "Knowledge templates and records are aligned."Scope
Writing rubric results
The converted native records keep the approved text; only section names, frontmatter, and cross-references changed. The rubric was run over the added sections and the AST-093 bullet:
Since the last review
scripts/migrate-knowledge-v5.mjslists records ascheck:knowledgediscovers them, tracked and untracked but not ignored, so a new record the check flags is migrated before it's staged. There's a test for it.validatePlatformComponentRecordrequires a platform record to sit where core keeps its component: the core record's folder, the core<Name>/directory, or the folder of<Name>.doc.mjs(AST-093 FR1 with spec(AST-093): a native component lives in the folder where core keeps it (FR1) #8593). Tests: Avatar/AvatarStatusDot and List/ListItem pass; ListItem/ListItem, Card/ListItem and Icon/Banner fail, each naming the right folder.pnpm specs:forroutes a platform member record (Swift AvatarStatusDot inAvatar/) only to its same-stem files, as core routes a member record. The folder's own component record still covers the whole folder.Avatar/AvatarStatusDot.spec.md), following spec(AST-093): a native component lives in the folder where core keeps it (FR1) #8593.migrate-knowledge-v5.mjsskips a tracked record that was moved or deleted but not staged, ascheck:knowledgedoes.Testing
pnpm vitest run --project node scripts/check-knowledge.test.mjs scripts/specs-for-file.test.mjs .github/scripts/knowledge-paths.test.mjs .github/scripts/change-scope.test.mjs .github/scripts/spec-owner-decision.test.mjs .github/scripts/spec-owner-reconcile.test.mjs .github/scripts/spec-owner-workflow.test.mjs: 8 files, 424 tests pass, includingscripts/migrate-knowledge-v5.test.mjs. New cases cover INV27 candidates, package-record ids, platform records (path id,platform_ofwith and without a core record, the anatomy map, a spec outside the Swift component tree),specs:forrouting throughplatform_of, and change-scope.node scripts/check-knowledge.mjsandnode scripts/check-knowledge.mjs --base origin/main: aligned.node scripts/migrate-knowledge-v5.mjs --check: every component record is on schema v5.