Skip to content

Review 4707

Jump to bottom
Cindy Zhang edited this page Aug 27, 2026 · 2 revisions

Review 4707

Round 1 — 2026-08-27

PR

#4707 fix(cli): filter skeleton component lists to resolvable components (#4677) by MeGaurav4 (bucket: contributor)

HEAD REVIEWED

bc8173163d1ba3a0461f9347c33c30f6681773b6

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: delta re-review after request-changes, CLI behavior contract, prior finding to recheck.

PROBLEM

WHY 1: template --skeleton advertised component names that astryx component <Name> could not resolve. WHY 2: the documented builder workflow asks callers to query each proposed skeleton component before importing it. WHY 3: a template component list that is not aligned with the resolver makes that workflow either fail on phantoms or skip real tags. USER-FACING PROBLEM: builders using skeleton output cannot trust the component list to tell them which Astryx component contracts to query. PROBLEM SEVERITY: harmful friction — #4677 reports the failing contract.

VERDICT: clear

SOLUTION

The PR now builds a resolver-backed component-name set and filters template show/skeleton component lists through it. The previous false positives are gone, and the prior request-changes examples (ResizeHandle, SyntaxTheme, NavHeadingMenu) are restored. One source-fallback resolver path still falls outside the new index, so skeleton output can still contain a resolvable tag that the component list omits.

SOLUTION (2 decisions · ~35 runtime lines of 268 changed lines)

  1. Filter template component lists through a resolver-backed set.
  2. Build the resolver set from doc files and parent-doc component metadata.

BURDEN: medium — async doc loading and cached discovery on every show/skeleton path; no effects, browser work, dependency, or visual surface. BURDEN MATCH: proportionate — the bug is a CLI contract bug and exact resolver filtering is the right layer.

VERDICT: BLOCKS — source-fallback-resolvable names can still be omitted.

ARCHITECTURE

OWNER: CLI component discovery owns “resolvable component name”. TIER 1: astryx component <Name> resolver path. TIER 2: template show/skeleton component metadata. SEAMS: core docs, parent-doc subcomponents, source-fallback components, external/integration docs. BEHAVIOR UNIT: pure utility — listResolvableComponentNames() is directly testable without a browser.

seam driven result
parent-doc doc entries works: ResizeHandle, SyntaxTheme, NavHeadingMenu are included and zero advertised names are unresolved.
source-fallback resolver names fails: MobileNavToggleBasic --skeleton prints <AppShellMobileContext /> but components omits AppShellMobileContext; astryx component AppShellMobileContext exits 0.

VERDICT: BLOCKS — one documented resolver seam is not represented in the index.

IMPACT

The phantom side is fixed. The remaining impact is narrower: builders reading MobileNavToggleBasic/MobileNavToggleShowcase skeleton output see <AppShellMobileContext /> in the skeleton body, but the component list tells them only MobileNav, MobileNavToggle, SideNavItem, and SideNavSection, so they are not prompted to query the context provider before importing it.

VERDICT: BLOCKS — the same contract mismatch remains for a non-ubiquitous skeleton tag.

API

No public API signature changes. Existing JSON response fields change behavior: template.show.data.components, template.skeleton.data.components, and TemplateListEntry.componentsUsed are now documented as exact resolver-backed Astryx component names.

change public? class doc'd? verdict
~ components filtered by listResolvableComponentNames() CLI JSON bug-fix contract yes finding: still incomplete for source-fallback resolver names
~ componentsUsed doc says each name resolves through astryx component <Name> CLI JSON typedef contract clarification yes ok for listed names

OSSIFICATION: the new guarantee is a class guarantee: listed names are exact resolver matches. It does not need human judgement; it needs the index to cover the resolver paths the skeleton can still print.

VERDICT: BLOCKS — behavior does not fully meet the new contract.

THEMING

No theme targets, variables, tokens, StyleX, or rendered styling changed in the PR diff.

VERDICT: clear

BREAKING

BEHAVIOR: yes — component lists now drop unresolvable JSX tags and no longer suffix-rewrite local names. This is the intended bug fix. API: no signature change; JSON content becomes stricter. VISUAL: no — CLI metadata only. THEME: no.

VERDICT: note — intended CLI-output behavior change.

PERFORMANCE & RESOURCES

EFFECTS: zero.

Effect + deps external system why render/handler cannot do it measured render cost lifetime + cleanup focused test
none n/a n/a n/a n/a n/a

RENDER: no UI render path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency change.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: exact PR diff touches CLI discovery/API/test/type files only; no rendered component, style, Storybook, docsite, or template visual source changed.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No user-facing strings, ARIA, keyboard, focus, locale, direction, or rendered text changed. The only contract is CLI component-list metadata.

VERDICT: clear

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION BLOCKS
ARCHITECTURE BLOCKS
IMPACT BLOCKS
API BLOCKS
THEMING clear
BREAKING note
PERFORMANCE clear
VISUAL clear
A11Y & I18N clear

GOAL: partly met — the previous ResizeHandle/SyntaxTheme/NavHeadingMenu misses are fixed and zero advertised names fail to resolve, but a skeleton tag still appears without being advertised. DISPOSITION: one blocker remains; no human decision needed. ADVICE: bounded outcome criteria — every non-ubiquitous tag that remains in skeleton output and resolves through astryx component <Name> should either appear in the components list or be intentionally suppressed from skeleton output too. AUTHOR CAN PROCEED: yes — include source-fallback resolver names in listResolvableComponentNames() or suppress those tags from skeleton output. WORST OUTCOME: “A builder copies MobileNavToggleBasic, sees <AppShellMobileContext />, and the component list never tells them to query that contract before importing it.” → request changes

JUDGEMENT NEEDED: none

request changes

  1. listResolvableComponentNames() is still narrower than the component resolver. → Builders using MobileNavToggleBasic --skeleton see <AppShellMobileContext /> in the skeleton but not in the component list; astryx component AppShellMobileContext exits 0. · component-discovery.mjs:771

REVIEW

Thanks for chasing the doc-file gap; the earlier ResizeHandle/SyntaxTheme/NavHeadingMenu misses are fixed.

I think one resolver edge is still leaking through. MobileNavToggleBasic --skeleton still prints <AppShellMobileContext />, but its component list is only MobileNav, MobileNavToggle, SideNavItem, SideNavSection. astryx component AppShellMobileContext exits 0, so builders still see a skeleton tag the list does not tell them to query — the same #4677 contract, now on the source-fallback path.

Could we either include source-fallback resolvable names in listResolvableComponentNames() or suppress those tags from the skeleton?

If you'd rather talk it through with someone, we're in Discord.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

  • packages/cli/foundation/discovery/component-discovery.mjs:771 — This adds doc names, but source-fallback resolver names like AppShellMobileContext can still appear in skeleton output without being listed.

EVIDENCE I DID NOT SPEND

  • NavHeadingMenuShowcase --skeleton also prints NavHeadingMenuItem while listing only NavHeadingMenu, SideNav, and SideNavHeading; astryx component NavHeadingMenuItem exits 0.
  • Icon, Text, HStack, and similar tags were ignored as intentionally ubiquitous/excluded rather than used as findings.
  • template.test.mjs passes locally but does not assert that every non-ubiquitous resolvable skeleton tag is advertised.

TIME

TIME total 19m setup 5m rules, fresh wiki clones, safety surface, exact-head worktree, dependency seed/install reading 6m prior reviews, issue, PR diff, changed files, affected radius measuring 3m focused Vitest plus resolver/skeleton replay; no browser required writing 4m presentation, critic pass, wiki record waste 1m retried unsupported gh pr/JSON flags and one shell quoting miss

WHAT I COULD NOT VERIFY

  • Vercel remains red, but the failing status is unrelated to this CLI-only contract finding.

What changed before posting

No public PR action taken; this is the proposed request-changes review body.

Posted receipt

  • Request-changes review posted at exact head bc8173163d1ba3a0461f9347c33c30f6681773b6.
  • One inline was posted on component-discovery.mjs:771.
  • No merge or auto-merge action.

Clone this wiki locally