refactor: Switch reference system to use WeakMap metadata instead of markers#776
Merged
kingston merged 25 commits intoFeb 19, 2026
Conversation
…erence-system-to-use-zod-metadata
Deploying baseplate-storybook with
|
| Latest commit: |
d2410de
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5a84d60a.baseplate-storybook.pages.dev |
| Branch Preview URL: | https://kingston-eng-1017-switch-pro.baseplate-storybook.pages.dev |
🦋 Changeset detectedLatest commit: 24787c0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR refactors the project-builder reference system from a marker-based approach using transforms to a metadata-registry and schema-walker approach. It introduces new infrastructure for collecting and managing reference annotations, updates schema creators to use discriminated unions, and modifies the serialization/parsing flow to work with defaults cleanup and reference extraction via parallel schema traversal. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Parser
participant Schema as Zod Schema<br/>(with registry metadata)
participant Walker as SchemaWalker
participant Visitors as SchemaNodeVisitor(s)
participant Registry as DefinitionRefRegistry<br/>& DefaultRegistry
User->>Walker: walkSchemaWithData(schema, data, visitors)
Walker->>Schema: traverse schema structure
Walker->>Registry: getAll(schema) for metadata
alt metadata found
Walker->>Visitors: visit(schema, data, ctx)
Visitors->>Registry: record entity/ref/expression/slot
Visitors-->>Walker: cleanup callback
end
Walker->>Walker: post-order cleanup (if returned)
Walker-->>User: traversal complete
Note over User,Registry: Later: Serialization Pipeline
User->>Schema: parse(input) for validation
User->>Walker: extract refs via walkSchemaWithData
User->>Registry: cleanDefaultValues using registered defaults
User-->>User: final serialized output with names resolved
sequenceDiagram
participant User as SchemaCreator
participant Parser as SchemaParserContext
participant RefBuilder as withRef/withEnt<br/>Builders
participant Registry as DefinitionRefRegistry
participant DefFactory as createSchema(ctx)
User->>Parser: createDefinitionSchemaParserContext(pluginStore)
Parser->>RefBuilder: extendParserContextWithRefs()
RefBuilder-->>Parser: return {withRef, withEnt, refContext, withExpression}
Note over User,DefFactory: Schema Construction Phase
User->>DefFactory: createEntitySchema(ctx)
DefFactory->>RefBuilder: ctx.withEnt(schema, entity)
RefBuilder->>Registry: add(schema, EntitySchemaMeta)
RefBuilder-->>DefFactory: augmented schema
DefFactory-->>User: schema with metadata attached
Note over User,Registry: Later: Reference Collection Phase
User->>Parser: parseSchemaWithTransformedReferences()
Parser->>User: validate input via schema.parse()
Parser->>Parser: extractDefinitionRefs(schema, data)
Parser-->>User: resolved references with names
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/project-builder-lib/src/references/deserialize-schema.ts (1)
19-21:⚠️ Potential issue | 🟡 MinorStale JSDoc: still references "transform-based approach".
The comment says "new transform-based approach" but this PR removes the transform-based system in favor of the WeakMap/registry approach.
📝 Suggested fix
/** - * Deserialize a schema with references using the new transform-based approach. - * This function converts human-readable names back to entity IDs. + * Deserialize a schema with references. + * This function converts human-readable names back to entity IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/references/deserialize-schema.ts` around lines 19 - 21, Update the stale JSDoc on deserializeSchema to remove references to the "new transform-based approach" and instead describe the current WeakMap/registry-based mechanism used to convert human-readable names back to entity IDs; specifically edit the comment above the deserializeSchema function in deserialize-schema.ts to mention the WeakMap/registry approach (or registry lookup) and its purpose for resolving references.
🧹 Nitpick comments (18)
packages/project-builder-web/src/routes/data/models/edit.$key/-components/fields/model-add-field-button.tsx (1)
91-103:asassertion silences structural checks — consider eliminating the need for the castThe assertion is needed because TypeScript cannot reduce
{ id: string } & Omit<DiscriminatedUnion, 'id'>back to the source discriminated union (ModelScalarFieldConfigInput). While the runtime behavior is correct (all hardcodedtypevalues are valid discriminants), theascast means any future required property added to a variant inModelScalarFieldConfigInputwill not produce a compile error here.A structurally safe alternative is to include
iddirectly in theAutoAddField.fieldstype and generate the ID before building the array — or to usesatisfiesin each literal object insideavailableAutoFieldsso TypeScript validates each field against the discriminated union at the definition site rather than at the call site.♻️ Suggested alternative: validate each field literal at definition site
interface AutoAddField { name: string; - fields: (Omit<ModelScalarFieldConfigInput, 'id'> & { - isPrimaryKey?: boolean; - })[]; + fields: (ModelScalarFieldConfigInput & { isPrimaryKey?: boolean })[]; }Then in
applyAutoField, drop the cast entirely:const applyAutoField = (autoField: AutoAddField): void => { for (const { isPrimaryKey, ...field } of autoField.fields) { - const fieldId = modelScalarFieldEntityType.generateNewId(); if (!fieldNames.includes(field.name)) { appendField({ - id: fieldId, - ...field, - } as ModelScalarFieldConfigInput); + ...field, + }); } if (isPrimaryKey) { - setValue('model.primaryKeyFieldRefs', [fieldId], { shouldDirty: true }); + setValue('model.primaryKeyFieldRefs', [field.id], { shouldDirty: true }); } } };And update each literal in
availableAutoFieldsto include a generatedidfield (or generate IDs upfront inuseMemo), so TypeScript validates the shape at the definition site viasatisfies ModelScalarFieldConfigInput & { isPrimaryKey?: boolean }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/fields/model-add-field-button.tsx around lines 91 - 103, The current cast in applyAutoField hides structural type errors by asserting the built object as ModelScalarFieldConfigInput; instead ensure fields already contain an id (or are validated at definition) so no cast is needed. Update the AutoAddField.fields type (or the place you build availableAutoFields) to produce objects typed as ModelScalarFieldConfigInput & { isPrimaryKey?: boolean } — e.g. generate IDs up front in a useMemo or add id to the AutoAddField fields and use the satisfies operator when declaring availableAutoFields — then remove the "as ModelScalarFieldConfigInput" in applyAutoField (which uses modelScalarFieldEntityType.generateNewId, appendField and setValue) so TypeScript performs structural checks at the definition site.packages/project-builder-lib/src/schema/plugins/definition.ts (2)
48-52: Unsafe double cast on discriminated union variants.The
as unknown as [typeof basePluginDefinitionSchema]cast erases the actual variant types (each hasz.literal(...)forid). This is a common workaround for Zod's strict tuple typing ondiscriminatedUnion, but it silently hides type mismatches. If you adopt the.extend()suggestion above, the inferred element types would be closer totypeof basePluginDefinitionSchema, reducing (but not fully eliminating) the gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/plugins/definition.ts` around lines 48 - 52, The discriminated union call is using an unsafe double-cast (as unknown as [typeof basePluginDefinitionSchema]) which erases variant types; fix by ensuring the `variants` value passed into z.discriminatedUnion is strongly typed as the exact array/tuple of basePluginDefinitionSchema variants (or a ReadonlyArray/Array<typeof basePluginDefinitionSchema>) instead of using the double-cast — adjust the producer of `variants` (or adopt the earlier .extend() change so each element is inferred as typeof basePluginDefinitionSchema) and pass that correctly into z.discriminatedUnion in the ctx.withEnt call to preserve type safety for id-discriminated variants.
36-46: Reduce field duplication by extending the base schema instead of manually recreating all fields.Lines 38-45 duplicate every field from
basePluginDefinitionSchema. Using.extend()creates a new schema instance (safe for metadata registration) while eliminating the duplication and maintenance risk. This pattern is consistent with existing code in the codebase (e.g.,baseAdminCrudInputSchema.extend(),baseFieldSchema.extend()).♻️ Proposed refactor
const variants = pluginKeys.map((key) => { const configSchema = schemaCreators.get(key)?.(ctx) ?? z.unknown(); - return z.object({ - id: z.literal(pluginEntityType.idFromKey(key)), - packageName: z.string(), - name: z.string(), - version: z.string(), - config: configSchema, - configSchemaVersion: z.number().optional(), - }); + return basePluginDefinitionSchema.extend({ + id: z.literal(pluginEntityType.idFromKey(key)), + config: configSchema, + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/plugins/definition.ts` around lines 36 - 46, The variant schema currently re-declares all fields; replace the z.object({...}) in the pluginKeys.map by extending basePluginDefinitionSchema to avoid duplication and keep metadata-safe instances: call basePluginDefinitionSchema.extend({ id: z.literal(pluginEntityType.idFromKey(key)), config: configSchema }) (and keep any other per-key overrides if needed, e.g., configSchemaVersion stays from base), using schemaCreators.get(key)?.(ctx) for configSchema; this yields a new schema per key while preserving base schema fields and registration safety.packages/project-builder-web/src/routes/data/models/edit.$key/-components/fields/model-field-default-value-input.tsx (2)
280-282: Returnnullinstead of<div />whenenumRefis absent
<div />emits an empty DOM node that can affect layout (flex/grid spacing). All other early-exit paths in this file returnnull; this branch should follow the same convention.♻️ Proposed fix
- return <div />; + return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/fields/model-field-default-value-input.tsx around lines 280 - 282, The early-exit branch in model-field-default-value-input.tsx that checks if (!optionsValue?.enumRef) returns an empty <div />, which creates an unwanted DOM node; change that return to null so it matches the other early-exit paths in this component (replace the return <div /> inside the if (!optionsValue?.enumRef) branch with return null).
280-288: Use non-throwingEnumUtils.byIdto gracefully handle deleted enum referencesWhile a guard on line 280 checks for missing
enumRef, if an enum is deleted after a field is configured,byIdOrThrowon line 284 will throw an error. Though an error boundary catches this at the route level, the component should degrade gracefully instead of relying on error recovery.The non-throwing
byIdvariant is already exported and can be used here:Suggested fix
- const fieldEnum = EnumUtils.byIdOrThrow(definition, optionsValue.enumRef); + const fieldEnum = EnumUtils.byId(definition, optionsValue.enumRef); + if (!fieldEnum) { + return <div />; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/fields/model-field-default-value-input.tsx around lines 280 - 288, The component currently uses EnumUtils.byIdOrThrow to resolve optionsValue.enumRef which will throw if the referenced enum was deleted; change this to the non-throwing EnumUtils.byId and handle a missing result by returning an empty placeholder (like the existing early return) so the component degrades gracefully. Specifically, replace EnumUtils.byIdOrThrow(definition, optionsValue.enumRef) with const fieldEnum = EnumUtils.byId(definition, optionsValue.enumRef) and if fieldEnum is falsy return the same <div /> before mapping fieldEnum.values to build enumValues.packages/project-builder-lib/src/references/expression-types.ts (1)
112-115: Stale JSDoc: "cached on the marker" references the removed marker-based system.The comment on
parse()reads "The result is cached on the marker for subsequent operations" — "marker" is the old concept this PR removes. This should be updated to describe the new WeakMap/metadata caching approach.💡 Suggested wording
- * The result is cached on the marker for subsequent operations. + * The result may be cached by the caller for subsequent operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/references/expression-types.ts` around lines 112 - 115, Update the stale JSDoc on the parse() function to remove the reference to the removed "marker" system and instead describe the new caching approach: state that parse() parses the raw expression value, returns the parse result, and caches that result in the metadata cache (a WeakMap keyed by the expression/object) for subsequent operations so repeated parses reuse the stored result; ensure the wording references parse() and the WeakMap/metadata caching concept rather than "marker".packages/project-builder-lib/src/plugins/store/store.ts (2)
25-27:getPluginKeys()exposes the internal array by referenceCallers can mutate the returned array and alter the store's internal state. Consider returning a shallow copy or using
as readonly string[]in the return type to signal immutability.♻️ Proposed fix
- getPluginKeys(): string[] { - return this.pluginKeys_; + getPluginKeys(): readonly string[] { + return this.pluginKeys_; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/plugins/store/store.ts` around lines 25 - 27, getPluginKeys() currently returns the internal mutable array pluginKeys_ by reference, allowing callers to mutate store state; change getPluginKeys to return a shallow copy (e.g., return this.pluginKeys_.slice() or [...this.pluginKeys_]) or change its signature to return a readonly array (e.g., as readonly string[]) so callers cannot modify pluginKeys_ directly; update the getPluginKeys method and its return type accordingly (referencing getPluginKeys and pluginKeys_).
12-12: Non-standard field naming: trailing underscore is unusual in TypeScript
pluginKeys_uses a trailing-underscore convention common in Python but not idiomatic TypeScript. Since the field is alreadyprivate, there's no naming conflict—private pluginKeys: string[]paired with a constructor parameter of the same name resolves cleanly viathis.pluginKeys = pluginKeys.♻️ Suggested rename
- private pluginKeys_: string[]; + private pluginKeys: string[]; constructor( instances = new Map<string, PluginSpecInitializerResult>(), pluginKeys: string[] = [], ) { this.instances = instances; - this.pluginKeys_ = pluginKeys; + this.pluginKeys = pluginKeys; } getPluginKeys(): string[] { - return this.pluginKeys_; + return this.pluginKeys; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/plugins/store/store.ts` at line 12, Rename the private field pluginKeys_ to the idiomatic TypeScript name pluginKeys and update all references accordingly: change the declaration from pluginKeys_ to pluginKeys, adjust the constructor parameter/assignment to use this.pluginKeys = pluginKeys (or accept a constructor parameter named pluginKeys), and update any methods or usages that reference pluginKeys_ (e.g., getters, setters, or internal logic) to use pluginKeys so the private field naming is consistent and idiomatic.packages/project-builder-lib/src/plugins/spec/config-spec.ts (1)
76-76:getAllSchemaCreators()exposes the internalMapby referenceCallers could mutate the returned
Map(adding/deleting entries), affecting all other consumers that share the samevalues.schemasreference. A defensive copy prevents this.♻️ Suggested fix
- getAllSchemaCreators: () => values.schemas, + getAllSchemaCreators: () => new Map(values.schemas),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/plugins/spec/config-spec.ts` at line 76, getAllSchemaCreators currently returns the internal Map reference (values.schemas) which allows external callers to mutate shared state; change getAllSchemaCreators to return a defensive shallow copy of the Map (e.g. new Map(values.schemas) or Array.from(values.schemas.entries()) wrapped back into a Map) so callers receive an independent Map and cannot modify the original values.schemas.packages/project-builder-lib/src/schema/models/models.ts (1)
90-99: Minor DRY opportunity:decimalandjsonoptions duplicatedefaultOptionsSchema.Lines 93 and 98 define
z.object({ default: z.string().optional() }).default({})inline, which is identical todefaultOptionsSchemaon line 36-38. Reusing the variable improves consistency.Suggested fix
z.object({ ...commonFields, type: z.literal('decimal'), - options: z.object({ default: z.string().optional() }).default({}), + options: defaultOptionsSchema, }), z.object({ ...commonFields, type: z.literal('json'), - options: z.object({ default: z.string().optional() }).default({}), + options: defaultOptionsSchema, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/models/models.ts` around lines 90 - 99, The decimal and json field schemas inline their options with z.object({ default: z.string().optional() }).default({}) which duplicates the existing defaultOptionsSchema; update the two schema entries for type: 'decimal' and type: 'json' to reuse defaultOptionsSchema instead of the inline object (retain ...commonFields and the type: z.literal(...) wrappers) so both blocks reference defaultOptionsSchema for consistency.packages/project-builder-lib/src/schema/definition.test-helper.ts (1)
32-59: Double-spread ofmodelcan silently discard nested defaults.The final
...modelat line 57 will overwrite the carefully constructedmodelandservicekeys built at lines 37-56 if the caller provides those nested properties. For example,createTestModel({ model: { fields: [myField] } })will lose the defaultprimaryKeyFieldRefsbecause the...modelspread replaces the wholemodelobject.This is a common deep-merge pitfall in test helpers. If this is intentional (caller takes full ownership of nested objects when overriding), it's fine — but it means the
...model.modelat line 48 and...model.serviceat line 55 become dead code in those cases.One possible fix: spread `model` first, then construct nested objects after
export function createTestModel(model: Partial<ModelConfig> = {}): ModelConfig { + const { model: modelOverride, service: serviceOverride, ...rest } = model; return { id: modelEntityType.generateNewId(), name: capitalize(faker.word.noun()), featureRef: 'mockFeature', model: { fields: [ { id: modelScalarFieldEntityType.generateNewId(), name: 'id', type: 'uuid', options: { genUuid: true, default: '123' }, isOptional: false, }, ], primaryKeyFieldRefs: ['id'], - ...model.model, + ...modelOverride, }, service: { create: { enabled: false }, update: { enabled: false }, delete: { enabled: false }, transformers: [], - ...model.service, + ...serviceOverride, }, - ...model, + ...rest, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/definition.test-helper.ts` around lines 32 - 59, The createTestModel helper currently spreads ...model last which fully replaces the constructed nested model and service defaults; instead spread the incoming model first and then build the nested objects so their defaults are merged (e.g., start from the root by applying ...model, then construct the model object using the default fields/primaryKeyFieldRefs and ...model.model, and similarly construct service using its defaults + ...model.service) ensuring id/name generation still occurs (or allow model.id/model.name to override if present) — update the createTestModel function to merge nested objects in this order so nested defaults like primaryKeyFieldRefs and transformer defaults are not lost.packages/project-builder-lib/src/references/extract-definition-refs.ts (1)
227-234: Duplicate-ID error message could include path and type for easier debugging.When a duplicate ID is found, including the entity type and path of both the original and duplicate entries would significantly speed up diagnosis.
Suggested improvement
- const idSet = new Set<string>(); - for (const entity of entities) { - if (idSet.has(entity.id)) { - throw new Error(`Duplicate ID found: ${entity.id}`); + const idMap = new Map<string, DefinitionEntityAnnotation>(); + for (const entity of entities) { + const existing = idMap.get(entity.id); + if (existing) { + throw new Error( + `Duplicate ID found: ${entity.id} (type: ${entity.type.name}) at path ${entity.path.join('.')} — conflicts with ${existing.type.name} at ${existing.path.join('.')}`, + ); } - idSet.add(entity.id); + idMap.set(entity.id, entity); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/references/extract-definition-refs.ts` around lines 227 - 234, The duplicate-ID check currently uses idSet and only throws Error(`Duplicate ID found: ${entity.id}`); update this to track the first seen entity for each id (replace idSet with a Map<string, Entity> or keep both) so that when you detect a duplicate for entity.id you can include the original entry's type and path and the duplicate entry's type and path in the thrown Error; reference the variables/entities named idSet (or new map), entities, and entity.id to locate and change the logic so the error message contains id, original.type, original.path, duplicate.type, and duplicate.path for easier debugging.packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-columns/admin-crud-column.ts (1)
1-1: Use named import for Zod to match project convention.This file uses
import z from 'zod'(default import), but the dominant pattern across the codebase and withinproject-builder-libisimport { z } from 'zod'(named import). Most files in the package use the named import style (40+ files vs. 6 using default), so aligning to this convention would improve consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-columns/admin-crud-column.ts` at line 1, Replace the default Zod import with the project's named-import convention: change the current default import of the Zod symbol (currently referenced as "z") to a named import { z } from 'zod' in admin-crud-column.ts, and verify all uses of the "z" identifier in that file remain unchanged and compile with the updated import style.packages/project-builder-lib/src/references/ref-schema-visitor.ts (1)
70-91: Implicit ordering dependency within the meta processing loop.When a single schema node carries multiple
DefinitionRefMetaentries (e.g., bothref-contextandentity), aref-contextmeta at position i extendsactiveSlotPathsfor all subsequent metas at positions i+1, i+2, etc. This is correct given thatdefinitionRefRegistry.add()preserves insertion order and the schema builder registersref-contextbefore inner metas — but worth noting as an invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/references/ref-schema-visitor.ts` around lines 70 - 91, The loop over metaList has an implicit ordering dependency: a 'ref-context' meta must extend activeSlotPaths before other metas on the same node are visited; to make this explicit and order-safe, first scan metaList for metas with meta.kind === 'ref-context' and update activeSlotPaths accordingly (using the same logic that creates extended = new Map(activeSlotPaths) and sets slot.id => ctx.path), then in a second pass construct RefWalkContext (path: ctx.path, activeSlotPaths) and invoke collectors (matching collector.kind === meta.kind) to call collector.visit(meta, data, refCtx) for every meta; this ensures ref-context is applied regardless of insertion order while preserving the existing symbols metaList, activeSlotPaths, collectors, RefWalkContext, and visit.packages/project-builder-lib/src/parser/clean-default-values.ts (2)
29-31: Path key collision with.join('.')separator.Using
.join('.')as a map key could produce collisions if any object key contains a literal'.'(e.g.,['a.b', 'c']and['a', 'b.c']both produce'a.b.c'). This is unlikely given typical Zod definition schemas but could be guarded by using a separator that can't appear in keys (e.g.,\0) or by keeping the path as an array and using a more robust keying strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/parser/clean-default-values.ts` around lines 29 - 31, The map key uses ctx.path.join('.') which can collide when path segments contain '.'; change the keying to a collision-safe representation (for example use JSON.stringify(ctx.path) or join with a non-printable separator like '\0') and update all places that read from defaults accordingly; locate the defaults.set(ctx.path.join('.'), meta) call and replace the key expression with the chosen safe key (e.g., JSON.stringify(ctx.path)), and ensure corresponding lookups (defaults.get(...)) use the same transformation.
54-105: Solid recursive cleaning with proper object/array separation.The
Object.getPrototypeOf(value) === Object.prototypecheck at line 65 correctly separates plain objects from arrays. The post-recursion default comparison (lines 99–102) enables cascading naturally.One edge case to be aware of: if a default is registered on an individual array element schema (e.g.,
z.array(withDefault(z.string(), 'x'))), cleaned elements would becomeundefinedwithin the array rather than being removed. The comment at line 82 notes elements are "never removed" to preserve indices, butundefinedvalues in an array can be surprising. This seems unlikely in practice given howwithDefaultis used on object fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/parser/clean-default-values.ts` around lines 54 - 105, The array-handling branch in cleanNode currently pushes childResult even when it's undefined (causing holes when an array element equals a registered default); change the Array.isArray branch so that if cleanNode returns undefined for an element you instead keep the original item (push item) and do not treat that as a removal — only mark changed when the pushed value differs from the original. Update the loop over value.entries() in cleanNode to push item when childResult === undefined, otherwise push childResult, and adjust the changed flag logic accordingly so arrays never gain undefined elements from default-stripping.packages/project-builder-lib/src/parser/schema-walker.ts (1)
230-251: Union type casting: cast toZodDiscriminatedUnionworks but is type-unsafe.Line 231 casts unconditionally to
ZodDiscriminatedUnion, but the schema may be a plainZodUnion. Both types share the same base_zod.defstructure—plain unions lack thediscriminatorfield (undefined) while discriminated unions have it. The code handles both correctly via theif (discriminator)check, but the cast misleads TypeScript.For clarity, cast to a union of both types (
ZodUnion | ZodDiscriminatedUnion) or move the cast into the discriminated path after checking for the discriminator field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/parser/schema-walker.ts` around lines 230 - 251, The code unconditionally casts schema to ZodDiscriminatedUnion (variable typed) which is type-unsafe; change the cast to either ZodUnion | ZodDiscriminatedUnion or defer casting until after checking for the discriminator so TypeScript reflects both possibilities—update the 'union' case (the block that uses typed._zod.def, discriminator, options) to first access schema._def in a type-safe way, then only cast to ZodDiscriminatedUnion when discriminator is present before calling findDiscriminatedUnionMatch and walkNode; keep existing logic for isLeafUnion and the error path intact.packages/project-builder-lib/src/references/definition-ref-registry.ts (1)
164-166:getAll()returns a direct reference to the internal array, bypassingadd's duplicate guard.
map.get(schema)exposes the exact array instance thatadd()pushes into. Any caller can mutate it directly (e.g.,.push()), circumventing the duplicate-detection logic on lines 149–152.Return a defensive copy instead:
Fix: Return a shallow copy
getAll(schema: z.ZodType): DefinitionRefMeta[] { - return map.get(schema) ?? []; + return map.get(schema)?.slice() ?? []; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/references/definition-ref-registry.ts` around lines 164 - 166, getAll currently returns the internal array from map.get(schema) allowing callers to mutate it and bypass add's duplicate-guard; change getAll to return a defensive shallow copy of the array returned by map.get(schema) (e.g., copy the array or use slice/spread) so callers cannot push into the registry's internal array and must use add() to modify entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/blog-with-auth/baseplate/project-definition.json`:
- Around line 753-756: Remove the redundant "updatedAt": false entry from the
"renewedAt" field's options so it matches the project's convention for
non-auto-updated dateTime fields; leave "options": { "defaultToNow": true } for
the "renewedAt" field and do not add an explicit updatedAt key when it's false
(refer to the "renewedAt" field definition to locate the change).
In
`@packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-columns/admin-crud-column.ts`:
- Around line 16-21: Compute the array of member schemas first (e.g. const
schemas = [...columns.values()].map(c => c.createSchema(ctx, slots))), check
that schemas.length > 0 and if not throw a clear Error like "No built-in admin
CRUD column schemas available for z.discriminatedUnion" before calling
z.discriminatedUnion('type', schemas as [typeof baseAdminCrudColumnSchema]);
apply the same defensive guard pattern to the analogous places using
z.discriminatedUnion in admin-crud-action.ts and the crud-form input schema to
avoid invoking z.discriminatedUnion with an empty array (references: columns,
createSchema, baseAdminCrudColumnSchema, z.discriminatedUnion).
In `@packages/project-builder-lib/src/schema/models/models.ts`:
- Around line 100-109: The 'date' branch in the schema (the z.object with type:
z.literal('date') and options) erroneously uses .prefault({}) instead of
.default({}), causing inconsistent handling versus other branches and
potentially breaking logic in definitionDefaultRegistry / cleanDefaultValues;
change the options call in that date schema to use .default({}) to match the
other type branches (the surrounding identifiers to locate are the z.object for
type 'date' and commonFields).
In
`@packages/project-builder-web/src/routes/admin-sections`.$appKey/-components/action-dialog.tsx:
- Around line 98-105: The form uses inconsistent defaults for position: the
SelectFieldController and reset()/initial values use 'inline' while onSubmit
falls back to 'dropdown'; pick one canonical default (recommended: keep 'inline'
to match the UI) and make it consistent by changing the fallback in onSubmit
(inside the onSubmit handler that calls handleSubmit) to the canonical value, or
if you choose 'dropdown' instead, update reset() and initial values to
'dropdown' as well; ensure the change touches the onSubmit block, the
reset()/values initialization, and any code referencing
adminCrudActionEntityType.generateNewId() so saved items get the same canonical
position.
In
`@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/fields/model-field-default-value-input.tsx:
- Around line 304-312: The Reset button currently sets
optionsValue.defaultEnumValueRef to an empty string, which conflicts with the
onChange normalization that converts falsy values to undefined; update the
Button onClick to call onOptionsChange with defaultEnumValueRef: undefined
(rather than ''), so the value is omitted consistently and matches the onChange
behavior and expected Zod schema; ensure you modify the onClick in the component
that references optionsValue.defaultEnumValueRef and uses onOptionsChange.
- Around line 197-200: The conditional currently uses nullish coalescing with
the extracted vars (optionsValue, defaultToNow, updatedAt) so an explicit false
for defaultToNow hides updatedAt; change the check in the component from using
"??" to a logical OR so the condition becomes true if either defaultToNow is
truthy or updatedAt is truthy (i.e., replace the `if (defaultToNow ??
updatedAt)` check with a logical OR-based check using defaultToNow and
updatedAt).
In `@plugins/plugin-auth/src/auth/core/schema/roles/schema.ts`:
- Around line 48-57: The superRefine validator that iterates AUTH_DEFAULT_ROLES
must also verify the matched role's builtIn flag; update the
.superRefine((roles, ctx) => { ... }) logic so for each defaultRole you find a
role r where r.name === defaultRole.name AND r.builtIn === true, and if not
present call ctx.addIssue with the same message; reference AUTH_DEFAULT_ROLES,
the roles parameter, and the builtIn property on the matched role when
implementing this check so default roles cannot be overridden by user-defined
entries.
---
Outside diff comments:
In `@packages/project-builder-lib/src/references/deserialize-schema.ts`:
- Around line 19-21: Update the stale JSDoc on deserializeSchema to remove
references to the "new transform-based approach" and instead describe the
current WeakMap/registry-based mechanism used to convert human-readable names
back to entity IDs; specifically edit the comment above the deserializeSchema
function in deserialize-schema.ts to mention the WeakMap/registry approach (or
registry lookup) and its purpose for resolving references.
---
Duplicate comments:
In
`@packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-actions/admin-crud-action.ts`:
- Around line 11-19: createAdminCrudActionSchema builds a discriminated union
from ctx.plugins.use(adminCrudActionSpec) but can pass an empty array to
z.discriminatedUnion if there are no registered actions; guard this by
collecting schemas from [...actions.values()].map(a => a.createSchema(ctx)), and
if that array is empty fallback to a single-entry array containing
baseAdminCrudActionSchema (or otherwise return the base schema) before calling
z.discriminatedUnion; update createAdminCrudActionSchema to perform that check
so z.discriminatedUnion never receives an empty array.
In
`@packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-form/admin-crud-input.ts`:
- Around line 13-24: createAdminCrudInputSchema can pass an empty array into
z.discriminatedUnion when ctx.plugins.use(adminCrudInputSpec).inputs has no
entries; ensure you guard against that by building the union array from inputs
but falling back to include baseAdminCrudInputSchema when inputs is empty. In
practice, inside createAdminCrudInputSchema reference the inputs map from
adminCrudInputSpec, compute the mapped array via inputs.values().map(input =>
input.createSchema(ctx, slots)), and if that results in an empty array, push or
replace with baseAdminCrudInputSchema before calling
z.discriminatedUnion('type', ...), so the discriminated union always has at
least baseAdminCrudInputSchema as a member.
In
`@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/graphql/graph-ql-object-type-section.tsx:
- Line 164: The conditional should rely on the discriminated union so keep the
expression checking field.type === 'enum' and access field.options.enumRef
without optional chaining; ensure the type definitions for the field union
include a literal 'type' = 'enum' branch that defines options (with enumRef) so
TypeScript narrows correctly — update the Field type/interface if necessary to
add the 'enum' variant or refine the union so that
graph-ql-object-type-section.tsx (the local variable/prop named field) can
safely access field.options.enumRef after the field.type === 'enum' guard.
---
Nitpick comments:
In `@packages/project-builder-lib/src/parser/clean-default-values.ts`:
- Around line 29-31: The map key uses ctx.path.join('.') which can collide when
path segments contain '.'; change the keying to a collision-safe representation
(for example use JSON.stringify(ctx.path) or join with a non-printable separator
like '\0') and update all places that read from defaults accordingly; locate the
defaults.set(ctx.path.join('.'), meta) call and replace the key expression with
the chosen safe key (e.g., JSON.stringify(ctx.path)), and ensure corresponding
lookups (defaults.get(...)) use the same transformation.
- Around line 54-105: The array-handling branch in cleanNode currently pushes
childResult even when it's undefined (causing holes when an array element equals
a registered default); change the Array.isArray branch so that if cleanNode
returns undefined for an element you instead keep the original item (push item)
and do not treat that as a removal — only mark changed when the pushed value
differs from the original. Update the loop over value.entries() in cleanNode to
push item when childResult === undefined, otherwise push childResult, and adjust
the changed flag logic accordingly so arrays never gain undefined elements from
default-stripping.
In `@packages/project-builder-lib/src/parser/schema-walker.ts`:
- Around line 230-251: The code unconditionally casts schema to
ZodDiscriminatedUnion (variable typed) which is type-unsafe; change the cast to
either ZodUnion | ZodDiscriminatedUnion or defer casting until after checking
for the discriminator so TypeScript reflects both possibilities—update the
'union' case (the block that uses typed._zod.def, discriminator, options) to
first access schema._def in a type-safe way, then only cast to
ZodDiscriminatedUnion when discriminator is present before calling
findDiscriminatedUnionMatch and walkNode; keep existing logic for isLeafUnion
and the error path intact.
In `@packages/project-builder-lib/src/plugins/spec/config-spec.ts`:
- Line 76: getAllSchemaCreators currently returns the internal Map reference
(values.schemas) which allows external callers to mutate shared state; change
getAllSchemaCreators to return a defensive shallow copy of the Map (e.g. new
Map(values.schemas) or Array.from(values.schemas.entries()) wrapped back into a
Map) so callers receive an independent Map and cannot modify the original
values.schemas.
In `@packages/project-builder-lib/src/plugins/store/store.ts`:
- Around line 25-27: getPluginKeys() currently returns the internal mutable
array pluginKeys_ by reference, allowing callers to mutate store state; change
getPluginKeys to return a shallow copy (e.g., return this.pluginKeys_.slice() or
[...this.pluginKeys_]) or change its signature to return a readonly array (e.g.,
as readonly string[]) so callers cannot modify pluginKeys_ directly; update the
getPluginKeys method and its return type accordingly (referencing getPluginKeys
and pluginKeys_).
- Line 12: Rename the private field pluginKeys_ to the idiomatic TypeScript name
pluginKeys and update all references accordingly: change the declaration from
pluginKeys_ to pluginKeys, adjust the constructor parameter/assignment to use
this.pluginKeys = pluginKeys (or accept a constructor parameter named
pluginKeys), and update any methods or usages that reference pluginKeys_ (e.g.,
getters, setters, or internal logic) to use pluginKeys so the private field
naming is consistent and idiomatic.
In `@packages/project-builder-lib/src/references/definition-ref-registry.ts`:
- Around line 164-166: getAll currently returns the internal array from
map.get(schema) allowing callers to mutate it and bypass add's duplicate-guard;
change getAll to return a defensive shallow copy of the array returned by
map.get(schema) (e.g., copy the array or use slice/spread) so callers cannot
push into the registry's internal array and must use add() to modify entries.
In `@packages/project-builder-lib/src/references/expression-types.ts`:
- Around line 112-115: Update the stale JSDoc on the parse() function to remove
the reference to the removed "marker" system and instead describe the new
caching approach: state that parse() parses the raw expression value, returns
the parse result, and caches that result in the metadata cache (a WeakMap keyed
by the expression/object) for subsequent operations so repeated parses reuse the
stored result; ensure the wording references parse() and the WeakMap/metadata
caching concept rather than "marker".
In `@packages/project-builder-lib/src/references/extract-definition-refs.ts`:
- Around line 227-234: The duplicate-ID check currently uses idSet and only
throws Error(`Duplicate ID found: ${entity.id}`); update this to track the first
seen entity for each id (replace idSet with a Map<string, Entity> or keep both)
so that when you detect a duplicate for entity.id you can include the original
entry's type and path and the duplicate entry's type and path in the thrown
Error; reference the variables/entities named idSet (or new map), entities, and
entity.id to locate and change the logic so the error message contains id,
original.type, original.path, duplicate.type, and duplicate.path for easier
debugging.
In `@packages/project-builder-lib/src/references/ref-schema-visitor.ts`:
- Around line 70-91: The loop over metaList has an implicit ordering dependency:
a 'ref-context' meta must extend activeSlotPaths before other metas on the same
node are visited; to make this explicit and order-safe, first scan metaList for
metas with meta.kind === 'ref-context' and update activeSlotPaths accordingly
(using the same logic that creates extended = new Map(activeSlotPaths) and sets
slot.id => ctx.path), then in a second pass construct RefWalkContext (path:
ctx.path, activeSlotPaths) and invoke collectors (matching collector.kind ===
meta.kind) to call collector.visit(meta, data, refCtx) for every meta; this
ensures ref-context is applied regardless of insertion order while preserving
the existing symbols metaList, activeSlotPaths, collectors, RefWalkContext, and
visit.
In
`@packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-columns/admin-crud-column.ts`:
- Line 1: Replace the default Zod import with the project's named-import
convention: change the current default import of the Zod symbol (currently
referenced as "z") to a named import { z } from 'zod' in admin-crud-column.ts,
and verify all uses of the "z" identifier in that file remain unchanged and
compile with the updated import style.
In `@packages/project-builder-lib/src/schema/definition.test-helper.ts`:
- Around line 32-59: The createTestModel helper currently spreads ...model last
which fully replaces the constructed nested model and service defaults; instead
spread the incoming model first and then build the nested objects so their
defaults are merged (e.g., start from the root by applying ...model, then
construct the model object using the default fields/primaryKeyFieldRefs and
...model.model, and similarly construct service using its defaults +
...model.service) ensuring id/name generation still occurs (or allow
model.id/model.name to override if present) — update the createTestModel
function to merge nested objects in this order so nested defaults like
primaryKeyFieldRefs and transformer defaults are not lost.
In `@packages/project-builder-lib/src/schema/models/models.ts`:
- Around line 90-99: The decimal and json field schemas inline their options
with z.object({ default: z.string().optional() }).default({}) which duplicates
the existing defaultOptionsSchema; update the two schema entries for type:
'decimal' and type: 'json' to reuse defaultOptionsSchema instead of the inline
object (retain ...commonFields and the type: z.literal(...) wrappers) so both
blocks reference defaultOptionsSchema for consistency.
In `@packages/project-builder-lib/src/schema/plugins/definition.ts`:
- Around line 48-52: The discriminated union call is using an unsafe double-cast
(as unknown as [typeof basePluginDefinitionSchema]) which erases variant types;
fix by ensuring the `variants` value passed into z.discriminatedUnion is
strongly typed as the exact array/tuple of basePluginDefinitionSchema variants
(or a ReadonlyArray/Array<typeof basePluginDefinitionSchema>) instead of using
the double-cast — adjust the producer of `variants` (or adopt the earlier
.extend() change so each element is inferred as typeof
basePluginDefinitionSchema) and pass that correctly into z.discriminatedUnion in
the ctx.withEnt call to preserve type safety for id-discriminated variants.
- Around line 36-46: The variant schema currently re-declares all fields;
replace the z.object({...}) in the pluginKeys.map by extending
basePluginDefinitionSchema to avoid duplication and keep metadata-safe
instances: call basePluginDefinitionSchema.extend({ id:
z.literal(pluginEntityType.idFromKey(key)), config: configSchema }) (and keep
any other per-key overrides if needed, e.g., configSchemaVersion stays from
base), using schemaCreators.get(key)?.(ctx) for configSchema; this yields a new
schema per key while preserving base schema fields and registration safety.
In
`@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/fields/model-add-field-button.tsx:
- Around line 91-103: The current cast in applyAutoField hides structural type
errors by asserting the built object as ModelScalarFieldConfigInput; instead
ensure fields already contain an id (or are validated at definition) so no cast
is needed. Update the AutoAddField.fields type (or the place you build
availableAutoFields) to produce objects typed as ModelScalarFieldConfigInput & {
isPrimaryKey?: boolean } — e.g. generate IDs up front in a useMemo or add id to
the AutoAddField fields and use the satisfies operator when declaring
availableAutoFields — then remove the "as ModelScalarFieldConfigInput" in
applyAutoField (which uses modelScalarFieldEntityType.generateNewId, appendField
and setValue) so TypeScript performs structural checks at the definition site.
In
`@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/fields/model-field-default-value-input.tsx:
- Around line 280-282: The early-exit branch in
model-field-default-value-input.tsx that checks if (!optionsValue?.enumRef)
returns an empty <div />, which creates an unwanted DOM node; change that return
to null so it matches the other early-exit paths in this component (replace the
return <div /> inside the if (!optionsValue?.enumRef) branch with return null).
- Around line 280-288: The component currently uses EnumUtils.byIdOrThrow to
resolve optionsValue.enumRef which will throw if the referenced enum was
deleted; change this to the non-throwing EnumUtils.byId and handle a missing
result by returning an empty placeholder (like the existing early return) so the
component degrades gracefully. Specifically, replace
EnumUtils.byIdOrThrow(definition, optionsValue.enumRef) with const fieldEnum =
EnumUtils.byId(definition, optionsValue.enumRef) and if fieldEnum is falsy
return the same <div /> before mapping fieldEnum.values to build enumValues.
Comment on lines
753
to
756
| "isOptional": false, | ||
| "name": "renewedAt", | ||
| "options": { | ||
| "default": "", | ||
| "defaultToNow": true, | ||
| "updatedAt": false | ||
| }, | ||
| "options": { "defaultToNow": true, "updatedAt": false }, | ||
| "type": "dateTime" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all instances of "updatedAt": false in JSON files
echo "=== Searching for 'updatedAt': false instances ==="
rg -n '"updatedAt":\s*false' --glob '*.json'
echo ""
echo "=== Counting dateTime fields in project-definition files ==="
# Find dateTime field definitions
rg -n '"type":\s*"dateTime"' --glob '*project-definition*.json' | wc -l
echo ""
echo "=== Examining dateTime fields with their options ==="
# Show dateTime fields with their context
rg -B2 '"type":\s*"dateTime"' --glob '*project-definition*.json'Repository: halfdomelabs/baseplate
Length of output: 11673
Remove redundant "updatedAt": false to align with consistent pattern across the project.
The renewedAt field explicitly includes "updatedAt": false, which is inconsistent with every other non-auto-updated dateTime field in the project. All other such fields (e.g., createdAt, expiresAt, expire) omit the updatedAt key entirely when it's not an auto-update field. Similarly, auto-updated fields explicitly set "updatedAt": true. The renewedAt field breaks this pattern by serializing the false value.
Proposed fix
- "options": { "defaultToNow": true, "updatedAt": false },
+ "options": { "defaultToNow": true },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "isOptional": false, | |
| "name": "renewedAt", | |
| "options": { | |
| "default": "", | |
| "defaultToNow": true, | |
| "updatedAt": false | |
| }, | |
| "options": { "defaultToNow": true, "updatedAt": false }, | |
| "type": "dateTime" | |
| "isOptional": false, | |
| "name": "renewedAt", | |
| "options": { "defaultToNow": true }, | |
| "type": "dateTime" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/blog-with-auth/baseplate/project-definition.json` around lines 753 -
756, Remove the redundant "updatedAt": false entry from the "renewedAt" field's
options so it matches the project's convention for non-auto-updated dateTime
fields; leave "options": { "defaultToNow": true } for the "renewedAt" field and
do not add an explicit updatedAt key when it's false (refer to the "renewedAt"
field definition to locate the change).
Comment on lines
+16
to
+21
| return z.discriminatedUnion( | ||
| 'type', | ||
| [...columns.values()].map((column) => | ||
| column.createSchema(ctx, slots), | ||
| ) as [typeof baseAdminCrudColumnSchema], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for BUILT_IN_ADMIN_CRUD_COLUMNS
rg -n "BUILT_IN_ADMIN_CRUD_COLUMNS" --type=ts -C3Repository: halfdomelabs/baseplate
Length of output: 2698
🏁 Script executed:
#!/bin/bash
# Check the actual admin-crud-column.ts file
find . -name "admin-crud-column.ts" -type f | head -5Repository: halfdomelabs/baseplate
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Look for the adminCrudColumnSpec definition and how columns are populated
rg -n "adminCrudColumnSpec" --type=ts -C5 | head -100Repository: halfdomelabs/baseplate
Length of output: 5287
🏁 Script executed:
#!/bin/bash
# Check admin-crud-action.ts for the pattern
cat -n packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-actions/admin-crud-action.ts | head -30Repository: halfdomelabs/baseplate
Length of output: 1029
🏁 Script executed:
#!/bin/bash
# Check admin-crud-input.ts for the pattern
cat -n packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-inputs/admin-crud-input.ts | head -30Repository: halfdomelabs/baseplate
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Search for BUILT_IN_ADMIN_CRUD_ACTIONS and BUILT_IN_ADMIN_CRUD_INPUTS
rg "BUILT_IN_ADMIN_CRUD_(ACTIONS|INPUTS)" --type=ts -C2Repository: halfdomelabs/baseplate
Length of output: 3696
🏁 Script executed:
#!/bin/bash
# Check built-in-actions.ts to count entries
rg "export const BUILT_IN_ADMIN_CRUD_ACTIONS" -A 10 packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-actions/built-in-actions.tsRepository: halfdomelabs/baseplate
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Check built-in-input.ts to count entries
rg "export const BUILT_IN_ADMIN_CRUD_INPUTS" -A 10 packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-form/built-in-input.tsRepository: halfdomelabs/baseplate
Length of output: 310
Add defensive check for non-empty built-in types in discriminatedUnion pattern.
z.discriminatedUnion requires at least one schema member. While adminCrudColumnSpec, adminCrudActionSpec, and adminCrudInputSpec are always initialized with built-in entries, adding an explicit length check before calling discriminatedUnion would provide clearer error messages and prevent confusing Zod errors if the invariant is broken in the future. This same pattern appears in admin-crud-action.ts and the crud-form input schema.
Proposed guard
const { columns } = ctx.plugins.use(adminCrudColumnSpec);
+ const columnSchemas = [...columns.values()].map((column) =>
+ column.createSchema(ctx, slots),
+ );
+ if (columnSchemas.length === 0) {
+ throw new Error('adminCrudColumnSpec must provide at least one column type');
+ }
return z.discriminatedUnion(
'type',
- [...columns.values()].map((column) =>
- column.createSchema(ctx, slots),
- ) as [typeof baseAdminCrudColumnSchema],
+ columnSchemas as [typeof baseAdminCrudColumnSchema],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return z.discriminatedUnion( | |
| 'type', | |
| [...columns.values()].map((column) => | |
| column.createSchema(ctx, slots), | |
| ) as [typeof baseAdminCrudColumnSchema], | |
| ); | |
| const columnSchemas = [...columns.values()].map((column) => | |
| column.createSchema(ctx, slots), | |
| ); | |
| if (columnSchemas.length === 0) { | |
| throw new Error('adminCrudColumnSpec must provide at least one column type'); | |
| } | |
| return z.discriminatedUnion( | |
| 'type', | |
| columnSchemas as [typeof baseAdminCrudColumnSchema], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-columns/admin-crud-column.ts`
around lines 16 - 21, Compute the array of member schemas first (e.g. const
schemas = [...columns.values()].map(c => c.createSchema(ctx, slots))), check
that schemas.length > 0 and if not throw a clear Error like "No built-in admin
CRUD column schemas available for z.discriminatedUnion" before calling
z.discriminatedUnion('type', schemas as [typeof baseAdminCrudColumnSchema]);
apply the same defensive guard pattern to the analogous places using
z.discriminatedUnion in admin-crud-action.ts and the crud-form input schema to
avoid invoking z.discriminatedUnion with an empty array (references: columns,
createSchema, baseAdminCrudColumnSchema, z.discriminatedUnion).
Comment on lines
+100
to
+109
| z.object({ | ||
| ...commonFields, | ||
| type: z.literal('date'), | ||
| options: z | ||
| .object({ | ||
| default: z.string().optional(), | ||
| defaultToNow: z.boolean().optional(), | ||
| }) | ||
| .prefault({}), | ||
| }), |
There was a problem hiding this comment.
date options use .prefault({}) while all other branches use .default({}) — likely unintended.
Every other type branch applies .default({}) on its options object, but date uses .prefault({}). In Zod 4, .default() short-circuits when input is undefined, while .prefault() injects the value before parsing. For a plain object schema the observable behavior is similar, but this inconsistency may cause the definitionDefaultRegistry or cleanDefaultValues logic to treat date options differently during serialization.
Suggested fix
options: z
.object({
default: z.string().optional(),
defaultToNow: z.boolean().optional(),
})
- .prefault({}),
+ .default({}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/schema/models/models.ts` around lines 100 -
109, The 'date' branch in the schema (the z.object with type: z.literal('date')
and options) erroneously uses .prefault({}) instead of .default({}), causing
inconsistent handling versus other branches and potentially breaking logic in
definitionDefaultRegistry / cleanDefaultValues; change the options call in that
date schema to use .default({}) to match the other type branches (the
surrounding identifiers to locate are the z.object for type 'date' and
commonFields).
Comment on lines
98
to
105
| const onSubmit = handleSubmit((data) => { | ||
| onSave(data); | ||
| onSave({ | ||
| ...data, | ||
| id: data.id ?? adminCrudActionEntityType.generateNewId(), | ||
| position: data.position ?? 'dropdown', | ||
| }); | ||
| onOpenChange(false); | ||
| }); |
There was a problem hiding this comment.
position fallback 'dropdown' is inconsistent with the form's visual default of 'inline'.
When editing a pre-existing action whose stored position is undefined (e.g., data created before this field existed), the SelectFieldController renders with an empty/undefined value, and onSubmit saves it as 'dropdown'. However, the reset() and values initialization both use 'inline'. The two defaults should agree; pick one canonical default and apply it in both places.
💡 Proposed fix if `'inline'` is canonical
onSave({
...data,
id: data.id ?? adminCrudActionEntityType.generateNewId(),
- position: data.position ?? 'dropdown',
+ position: data.position ?? 'inline',
});or, if 'dropdown' is canonical, update the initial/reset values:
values: action ?? {
type: 'edit',
- position: 'inline' as const,
+ position: 'dropdown' as const,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const onSubmit = handleSubmit((data) => { | |
| onSave(data); | |
| onSave({ | |
| ...data, | |
| id: data.id ?? adminCrudActionEntityType.generateNewId(), | |
| position: data.position ?? 'dropdown', | |
| }); | |
| onOpenChange(false); | |
| }); | |
| const onSubmit = handleSubmit((data) => { | |
| onSave({ | |
| ...data, | |
| id: data.id ?? adminCrudActionEntityType.generateNewId(), | |
| position: data.position ?? 'inline', | |
| }); | |
| onOpenChange(false); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/project-builder-web/src/routes/admin-sections`.$appKey/-components/action-dialog.tsx
around lines 98 - 105, The form uses inconsistent defaults for position: the
SelectFieldController and reset()/initial values use 'inline' while onSubmit
falls back to 'dropdown'; pick one canonical default (recommended: keep 'inline'
to match the UI) and make it consistent by changing the fallback in onSubmit
(inside the onSubmit handler that calls handleSubmit) to the canonical value, or
if you choose 'dropdown' instead, update reset() and initial values to
'dropdown' as well; ensure the change touches the onSubmit block, the
reset()/values initialization, and any code referencing
adminCrudActionEntityType.generateNewId() so saved items get the same canonical
position.
Comment on lines
+197
to
+200
| const optionsValue = rawOptions as DateTimeOptions | undefined; | ||
| const { defaultToNow, updatedAt } = optionsValue ?? {}; | ||
|
|
||
| if (defaultToNow ?? updatedAt) { |
There was a problem hiding this comment.
?? should be || — false is not nullish, so updatedAt is never checked when defaultToNow is explicitly false
defaultToNow ?? updatedAt evaluates to defaultToNow whenever it is false (not null/undefined), silently ignoring updatedAt. If persisted data arrives with { defaultToNow: false, updatedAt: true } the component will show the empty input form instead of the "Last Updated" label.
🐛 Proposed fix
- if (defaultToNow ?? updatedAt) {
+ if (defaultToNow || updatedAt) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/fields/model-field-default-value-input.tsx
around lines 197 - 200, The conditional currently uses nullish coalescing with
the extracted vars (optionsValue, defaultToNow, updatedAt) so an explicit false
for defaultToNow hides updatedAt; change the check in the component from using
"??" to a logical OR so the condition becomes true if either defaultToNow is
truthy or updatedAt is truthy (i.e., replace the `if (defaultToNow ??
updatedAt)` check with a logical OR-based check using defaultToNow and
updatedAt).
Comment on lines
+304
to
312
| {optionsValue.defaultEnumValueRef && ( | ||
| <Button | ||
| title="Reset" | ||
| onClick={() => { | ||
| onOptionsChange({ | ||
| ...optionsValue, | ||
| defaultEnumValueRef: value ? value : undefined, | ||
| defaultEnumValueRef: '', | ||
| }); | ||
| }} |
There was a problem hiding this comment.
Reset sets defaultEnumValueRef: '' but onChange normalises falsy values to undefined — inconsistent
The onChange handler (line 298) converts any falsy value to undefined. The reset button (line 310) instead writes an empty string ''. Depending on the Zod schema, '' may fail validation or be serialised as a present-but-empty reference, while undefined causes the key to be omitted entirely. Use undefined consistently.
🐛 Proposed fix
onOptionsChange({
...optionsValue,
- defaultEnumValueRef: '',
+ defaultEnumValueRef: undefined,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {optionsValue.defaultEnumValueRef && ( | |
| <Button | |
| title="Reset" | |
| onClick={() => { | |
| onOptionsChange({ | |
| ...optionsValue, | |
| defaultEnumValueRef: value ? value : undefined, | |
| defaultEnumValueRef: '', | |
| }); | |
| }} | |
| {optionsValue.defaultEnumValueRef && ( | |
| <Button | |
| title="Reset" | |
| onClick={() => { | |
| onOptionsChange({ | |
| ...optionsValue, | |
| defaultEnumValueRef: undefined, | |
| }); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/project-builder-web/src/routes/data/models/edit`.$key/-components/fields/model-field-default-value-input.tsx
around lines 304 - 312, The Reset button currently sets
optionsValue.defaultEnumValueRef to an empty string, which conflicts with the
onChange normalization that converts falsy values to undefined; update the
Button onClick to call onOptionsChange with defaultEnumValueRef: undefined
(rather than ''), so the value is omitted consistently and matches the onChange
behavior and expected Zod schema; ensure you modify the onClick in the component
that references optionsValue.defaultEnumValueRef and uses onOptionsChange.
Comment on lines
+48
to
+57
| .superRefine((roles, ctx) => { | ||
| for (const defaultRole of AUTH_DEFAULT_ROLES) { | ||
| if (!roles.some((r) => r.name === defaultRole.name)) { | ||
| ctx.addIssue({ | ||
| code: 'custom', | ||
| message: `Missing built-in role: ${defaultRole.name}`, | ||
| }); | ||
| } | ||
| } | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full schema.ts file to understand the context
head -100 plugins/plugin-auth/src/auth/core/schema/roles/schema.tsRepository: halfdomelabs/baseplate
Length of output: 1644
🏁 Script executed:
# Search for all uses of builtIn field in the codebase
rg -n '\bbuiltIn\b' --type ts -C 2 plugins/plugin-auth/Repository: halfdomelabs/baseplate
Length of output: 5193
🏁 Script executed:
# Find the AUTH_DEFAULT_ROLES definition to confirm what we're validating against
rg -n 'AUTH_DEFAULT_ROLES' --type ts -A 10 plugins/plugin-auth/Repository: halfdomelabs/baseplate
Length of output: 3327
Validation does not enforce builtIn: true on roles matching default role names.
The check at line 50 verifies that a role with the matching name exists, but it does not assert that the matched role has builtIn: true. This is functionally significant: the codebase relies on the builtIn flag to distinguish between system-managed roles (which cannot be edited or deleted) and user-defined roles. Multiple UI components filter using !role.builtIn to hide default roles from edit/delete operations (see role-editor-form.tsx lines 95–96).
Without the additional check, a role defined as { name: 'public', builtIn: false } would pass validation but be treated as a user-defined role in the UI, allowing deletion of what should be a system-reserved role.
Add the builtIn check:
Proposed fix
- if (!roles.some((r) => r.name === defaultRole.name)) {
+ if (!roles.some((r) => r.name === defaultRole.name && r.builtIn)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/plugin-auth/src/auth/core/schema/roles/schema.ts` around lines 48 -
57, The superRefine validator that iterates AUTH_DEFAULT_ROLES must also verify
the matched role's builtIn flag; update the .superRefine((roles, ctx) => { ...
}) logic so for each defaultRole you find a role r where r.name ===
defaultRole.name AND r.builtIn === true, and if not present call ctx.addIssue
with the same message; reference AUTH_DEFAULT_ROLES, the roles parameter, and
the builtIn property on the matched role when implementing this check so default
roles cannot be overridden by user-defined entries.
kingston
deleted the
kingston/eng-1017-switch-project-builder-reference-system-to-use-zod-metadata
branch
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores