feat: component registry — spec + full implementation - #11
Conversation
|
Claude finished @toolpathguy's task in 3m 40s —— View job Review complete: 02-component-registry spec — approve-with-nits
Verdict: approve-with-nits. No blocking issues — the spec traces faithfully to Findings posted as inline comments + a summary review comment:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 2. THE Registry_Module SHALL export a `Registry` type defined as `Record<string, RegistryEntry>` from `/types/registry.ts` | ||
| 3. THE Registry_Module SHALL require the `type` field of each RegistryEntry to be a PascalCase string (matching the pattern `/^[A-Z][a-zA-Z0-9]*$/`) that is identical to its key in the Registry record | ||
| 4. THE Registry_Module SHALL import `PropSchema` from `/types/shared.ts` for the `props` field definition | ||
| 5. IF `acceptsChildren` is set to `false` on a RegistryEntry, THEN THE Registry_Module SHALL disallow the presence of the `allowedChildren` field on that entry |
There was a problem hiding this comment.
Req 1.5 lives under "Registry Type Definitions" and says the module "SHALL disallow the presence of the allowedChildren field" when acceptsChildren is false — that reads as a compile-time guarantee. But design.md's RegistryEntry interface (lines 82–93) declares allowedChildren?: string[] as a plain optional, so { acceptsChildren: false, allowedChildren: [...] } type-checks fine. The design instead maps 1.5 to Property 1 / Zod runtime (design.md:388), which duplicates Req 2.5.
Please resolve the ambiguity: either
- make
RegistryEntrya discriminated union onacceptsChildren({ acceptsChildren: true; allowedChildren?: string[] } | { acceptsChildren: false }) so 1.5 is enforced at the type level, or - reword 1.5 to state it's a runtime validation rule (making it explicitly the same guarantee as 2.5).
As written, 1.5 is not met by the designed interface.
| 3. WHEN the Entries_Module is imported from a plain Node.js or tsx script running outside Vite, THE Entries_Module SHALL load and evaluate without throwing errors within 2 seconds | ||
| 4. THE Registry_Module at `/registry/index.ts` SHALL import entries from the Entries_Module and Vue components from `/app/components/blocks/`, and SHALL export a `registry` object re-exporting all entries keyed by type string | ||
| 5. THE Registry_Module SHALL export a `componentMap` of type `Record<string, Component>` mapping each entry `type` to its Vue component, where every key in `componentMap` has a corresponding entry in the registry and every entry in the registry has a corresponding key in `componentMap` | ||
| 6. THE Registry_Module SHALL export helper functions: `getEntry(type: string)` returning `RegistryEntry | undefined`, `defaultPropsFor(type: string)` returning `Record<string, unknown>`, and `entriesByCategory()` returning `Record<string, RegistryEntry[]>` |
There was a problem hiding this comment.
Internal contradiction: 13.6 states defaultPropsFor(type: string) returns Record<string, unknown>, but 13.8 (and Property 6) require it to return undefined for an unregistered type. design.md:215 correctly reconciles this as Record<string, unknown> | undefined. Update 13.6's stated return type to include | undefined so the acceptance criteria match the design.
| ### Test Framework & Libraries | ||
|
|
||
| - **Test runner**: Vitest (already configured via `vitest.config.ts`) | ||
| - **Property-based testing**: fast-check (already in devDependencies) |
There was a problem hiding this comment.
Nit: the testing strategy assumes fast-check is "already in devDependencies", but it isn't listed in the AI-MAP tech-stack table (Testing | Vitest, @vue/test-utils, @nuxt/test-utils, happy-dom). The property tests (task 8.x) can't run without it. Either add fast-check to Task 01's scope / the tech-stack table, or note it as a dependency this task must add. (The 8.x tests are marked optional *, so this is non-blocking.)
Review:
|
…me validation, note fast-check dep
|
Claude finished @toolpathguy's task in 2m 46s —— View job Review:
|
| for (const [key, entry] of Object.entries(registry)) { | ||
| const result = registryEntryZod.safeParse(entry) | ||
| if (!result.success) { | ||
| const issues = result.error.issues.map(i => i.message).join('; ') |
There was a problem hiding this comment.
Should fix (spec-level, affects traceability): this sketch joins only i.message and discards i.path, but several acceptance criteria require the error to identify which field failed:
- Req 2.3 — "error that includes the entry's
typeand the invalid prop key name". The prop key lives only ini.path(['props', '<key>', 'type']); with adiscriminatedUnionontype, themessagealone won't name the prop. - Req 2.2 — "indicates the label is missing or empty".
z.string().min(1)yields a generic"String must contain at least 1 character(s)"withpath: ['label']; the message alone doesn't say "label". - Req 2.9 — "the invalid category value" (Zod's enum message usually includes the received value, so this one is likely OK).
Recommend folding the path into the message, e.g.:
| const issues = result.error.issues.map(i => i.message).join('; ') | |
| const issues = result.error.issues | |
| .map(i => `${i.path.join('.') || '<root>'}: ${i.message}`) | |
| .join('; ') |
Related: Property 1 (design.md:386) only asserts the message "includes the entry's type or registry key" — the wrapping Registry validation failed for "${key}" already guarantees that regardless of i.path. So the Property-1 test would pass even while Req 2.3's "invalid prop key name" guarantee is unmet. Consider strengthening Property 1 (or a 9.1 unit test) to assert the failing field/prop key appears, so the criterion is actually guarded.
Re-review: `02-component-registry` spec — approve-with-nitsSpec-only PR (requirements + design + tasks). It traces faithfully to `MD Instructions/02-component-registry.md`: the 13-block Nuxt-UI-primary set, the `compileAs`/`U*` convention, dev-only Zod validation, and the purity split all line up. Genuinely nice touches: the requirement↔property traceability table, Property 7's `JSON.parse(JSON.stringify)` serializability guarantee for the pure-data layer, and the discriminated-union PropSchema reuse from `/types/shared.ts`. Prior review feedback — resolved ✅
Should fix (spec clarity → implementation traceability)
Nits (non-blocking)
CI: `ci` + `review` jobs still pending at review time — this is a spec/code-level review only. |
|
Claude finished @toolpathguy's task in 2m 23s —— View job Re-review:
|
| ```vue | ||
| <!-- BlockRichText.vue --> | ||
| <template> | ||
| <div :class="['prose max-w-none', alignClass]" v-html="body" /> |
There was a problem hiding this comment.
Nit (implementation-time, security): BlockRichText renders the body prop with v-html. Req 12.6 does mandate "render the body prop value as HTML content," so v-html is the intended mechanism — but the compiled output is a published Nuxt site, and unsanitized v-html is a stored-XSS sink if body ever originates from anything but the page author's own trusted input. Worth deciding explicitly at implementation: either document that RichText content is trusted-author-only, or sanitize (e.g. via a sanitizer) before binding. Non-blocking for a spec PR.
- Add RegistryEntry/Registry types in /types/registry.ts - Add Zod validation module with cross-field constraints - Add pure-data entries module (13 blocks, zero .vue imports) - Add 13 block components (10 Nuxt UI wrappers + 3 custom gap-fillers) - Add Vue-aware registry module with componentMap and helpers - Add useRegistry() composable - Add 8 property-based tests and 7 unit tests (all passing) - Fix prototype pollution in getEntry/defaultPropsFor with Object.hasOwn()
toolpathguy
changed the title
|
Claude encountered an error after 2m 50s —— View job Reviewing implementation:
|
1 participant
Relates to #1
Summary
Implements the complete Component Registry — the single source of truth for all placeable building blocks in the Nuxt Visual Builder. Includes spec documents, all implementation code, and full test coverage.
What's included
Types & Validation
types/registry.ts—RegistryEntry,Registry, andCategorytype definitionsregistry/validation.ts— Zod schema with cross-field constraints (dev-only)Pure-data entries (no .vue imports)
registry/entries.ts— All 13 block definitions as serializable objects13 Block Components (
app/components/blocks/)Vue-aware registry
registry/index.ts— componentMap, getEntry, defaultPropsFor, entriesByCategory + dev validation guardapp/composables/useRegistry.ts— composable exposing registry to Vue componentsTests (19 total, all passing)
tests/registry.property.test.ts— 8 property-based tests (fast-check, 100 runs each)tests/registry.test.ts— 7 example-based unit testsArchitecture
entries.ts(pure data) vsindex.ts(Vue-aware with componentMap)What was tested
npx vitest run)npx nuxt typecheck)