refactor: Add global definition validation system with fixes, issue checkers, and bottom-up schema transformation#779
Merged
kingston merged 7 commits intoFeb 25, 2026
Conversation
…ers, and bottom-up schema transformation
🦋 Changeset detectedLatest commit: 8550de2 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 |
📝 WalkthroughWalkthroughAdds a global definition-validation system: bottom-up schema-guided data transforms, registries for per-schema fixes and field-level issue-checkers, utilities to apply fixes and collect/partition issues, and integration into the web app save pipeline to auto-fix and block saves on definition-level errors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Web as Web App
participant Schema as Schema Utilities
participant Registry as Fix / Issue Registries
participant Plugin as PluginSpecStore
participant Pipeline as Save Pipeline
User->>Web: Trigger save
activate Web
Web->>Plugin: build/reuse PluginSpecStore
Web->>Schema: derive defSchema (withFix / withIssueChecker)
Schema->>Registry: read attached fixes & checkers
Web->>Pipeline: applyDefinitionFixes(defSchema, rawData)
activate Pipeline
Pipeline->>Registry: get fixes for nodes
Pipeline->>Pipeline: bottom-up transformDataWithSchema + apply fixes
Pipeline-->>Web: fixedData
deactivate Pipeline
Web->>Pipeline: collectDefinitionIssues(defSchema, fixedData, Plugin)
activate Pipeline
Pipeline->>Registry: get field-level checkers
Pipeline->>Plugin: get definition-level checkers
Pipeline->>Pipeline: run checkers -> issues[]
Pipeline-->>Web: issues
deactivate Pipeline
alt issues contains severity 'error'
Web->>Web: partitionIssuesBySeverity(issues) -> throw DefinitionIssueError
Web-->>User: show error
else
Web->>Pipeline: proceed with fixRefDeletions & persist
Web-->>User: success
end
deactivate Web
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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-web/src/routes/data/models/-hooks/use-model-form.ts (1)
93-96:⚠️ Potential issue | 🟠 MajorUse
updatedModel.idfor duplicate exclusion to avoid false self-collisions.At Line 95, using
data.idcan misfire whenidis omitted from form data, causing the current model to be flagged as a duplicate of itself.Suggested fix
const existingModel = definition.models.find( (m) => - m.id !== data.id && + m.id !== updatedModel.id && m.name.toLowerCase() === updatedModel.name.toLowerCase(), );🤖 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/-hooks/use-model-form.ts` around lines 93 - 96, The duplicate-check uses data.id which can be missing and causes self-collisions; in the existingModel lookup (definition.models.find) replace the exclusion check that compares m.id !== data.id to use updatedModel.id (or fallback to updatedModel.id if data may be undefined) so the current model is correctly excluded when comparing m.name.toLowerCase() === updatedModel.name.toLowerCase().
🧹 Nitpick comments (8)
packages/project-builder-web/src/utils/error.ts (1)
31-35: Consider makingDefinitionIssueError.issuesimmutable.Right now the error stores the original array reference, so downstream mutation can change the thrown error payload.
Suggested hardening
export class DefinitionIssueError extends UserVisibleError { - public issues: DefinitionIssue[]; + public readonly issues: readonly DefinitionIssue[]; constructor(issues: DefinitionIssue[]) { super(issues.map((i) => i.message).join('; ')); - this.issues = issues; + this.issues = [...issues]; this.name = 'DefinitionIssueError'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/utils/error.ts` around lines 31 - 35, The DefinitionIssueError currently stores the original array reference which allows external mutation; update the DefinitionIssueError class by making the issues property immutable (e.g., declare it as readonly) and assign a defensive copy of the input in the constructor (use issues.slice() or [...issues]) and optionally call Object.freeze(this.issues) to prevent further mutation; reference the DefinitionIssueError class, its issues property, and its constructor when applying this change.packages/project-builder-lib/src/schema/creator/extend-parser-context-with-defaults.ts (1)
29-36: Add a fail-fast guard for invalid default values.Line 30 accepts
unknown, and Line 33 applies it directly. A mismatched default won’t surface until parse-time elsewhere, which makes failures harder to trace.Suggested guard
export function withDefault( defaultValue: unknown, ): <T extends z.ZodType>(schema: T) => WithDefaultResult<T> { return <T extends z.ZodType>(schema: T): WithDefaultResult<T> => { + if (!schema.safeParse(defaultValue).success) { + throw new Error('withDefault: defaultValue does not match schema'); + } const result = (schema as z.ZodType).prefault(defaultValue).optional(); definitionDefaultRegistry.set(result, { defaultValue }); return result as WithDefaultResult<T>; }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/creator/extend-parser-context-with-defaults.ts` around lines 29 - 36, The withDefault function currently accepts any defaultValue and applies it without checking against the provided schema; add a fail-fast guard that validates defaultValue against the schema (use schema.safeParse or equivalent on the Zod type) before calling prefault and optional, and if validation fails throw a clear Error describing the mismatch; update the implementation around withDefault, the generic T extends z.ZodType handling, and the spots that call (schema as z.ZodType).prefault/default registration (definitionDefaultRegistry.set) so the registry only stores defaults that successfully validate.packages/project-builder-lib/src/schema/creator/definition-issue-checker-spec.ts (1)
38-40: Avoid exposing the live mutable checker map.Returning
values.checkersdirectly leaks internal mutable state across module boundaries.Suggested boundary hardening
{ use: (values) => ({ - getAllCheckers: (): Map<string, DefinitionIssueChecker> => - values.checkers, + getAllCheckers: (): ReadonlyMap<string, DefinitionIssueChecker> => + new Map(values.checkers), }), }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/creator/definition-issue-checker-spec.ts` around lines 38 - 40, getAllCheckers currently returns the live mutable Map stored in values.checkers which leaks internal state; change getAllCheckers to return a defensive copy (e.g., a new Map constructed from values.checkers) or an immutable/read-only view so callers cannot mutate the original Map, and update any callers expecting mutability to work with the copied Map; locate the getAllCheckers function and replace the direct return of values.checkers with a safe copy/immutable alternative.packages/project-builder-lib/src/parser/transform-data-with-schema.ts (2)
283-286: Record case doesn't handleundefinedremoval like the object case does.The
objectcase (line 110–113) removes keys whose transformed value isundefined. Therecordcase usesObject.fromEntries(entries)directly, which will preserve entries withundefinedvalues. If a transform returnsundefinedfor a record entry intending to remove it, the key will remain with valueundefinedinstead of being deleted.This may be intentional (records have no schema-defined shape to determine which keys belong), but the inconsistency could surprise callers. Consider documenting this difference or aligning the behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/parser/transform-data-with-schema.ts` around lines 283 - 286, The record branch currently sets childResult = Object.fromEntries(entries) and thus preserves keys with undefined values whereas the object branch filters out entries whose transformed value is undefined; update the record handling in transform-data-with-schema (the block that builds entries/changing childResult and uses Object.fromEntries) to drop any entries with value === undefined before calling Object.fromEntries, or if the intended semantics differ, add a clear comment/docstring in the record branch explaining why undefined should be preserved to make the behavior explicit to callers.
288-303: Intersection transforms the left side, then feeds that result into the right-side traversal.For overlapping fields between
leftandright, a field could be transformed twice — once during the left-side traversal and again during the right-side traversal. This is documented via the inline comment ("Transform left first, then transform the result via right"), but callers should be aware that registered transforms on overlapping shapes fire twice. If intersections are rare in definition schemas, this is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/parser/transform-data-with-schema.ts` around lines 288 - 303, Intersection traversal currently runs transformNode on the left then passes the full leftResult into transformNode for the right, causing registered transforms on overlapping fields to run twice; modify transformNode (and its callers) to accept and propagate an "alreadyTransformedKeys" (or similar) parameter so that when handling a ZodIntersection in transform-data-with-schema.ts you collect keys mutated by the left traversal (from transformNode calls) and pass that set into the right-side transformNode call (typed._zod.def.right) to skip reapplying transforms for those keys, updating transformNode signature and internal logic to consult that set when deciding to run registered transforms.packages/project-builder-lib/src/references/extend-parser-context-with-refs.ts (1)
108-139: Near-duplicate entity registration logic between standalonewithEntand parser-contextwithEnt.The standalone curried
withEnt(lines 119–138) and the inlinewithEntinsideextendParserContextWithRefs(lines 228–248) both defaultidPathto['id']and calldefinitionRefRegistry.addwith the same shape. The duplication exists because the standalone version trades type-safety ongetNameResolver(usesunknown) for the curried.apply()ergonomics.Consider extracting the shared registration logic into a small internal helper to keep them in sync, though this is a minor concern given the differing type constraints.
Also applies to: 228-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/references/extend-parser-context-with-refs.ts` around lines 108 - 139, Extract the duplicated entity-registration steps into a small internal helper and call it from both the standalone withEnt function and the inline withEnt inside extendParserContextWithRefs: the helper should take the schema, entity input, and resolved idPath (defaulting idPath to ['id'] when undefined) and invoke definitionRefRegistry.add with the shared object shape (kind: 'entity', type, idPath, getNameResolver, parentSlot, provides), allowing each caller to preserve its current typing differences for getNameResolver while centralizing the shared logic.packages/project-builder-lib/src/schema/creator/definition-issue-checkers.unit.test.ts (1)
35-42: Remove the unsafe double-cast in the null/undefined case.
as unknown as { name: string }[]weakens the test’s type signal and can hide future regressions.♻️ Proposed cleanup
const items = [ { name: undefined }, { name: null }, { name: '' }, { name: undefined }, - ] as unknown as { name: string }[]; + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/creator/definition-issue-checkers.unit.test.ts` around lines 35 - 42, The test uses an unsafe double-cast on items; replace "as unknown as { name: string }[]" with a correct type that permits undefined/null/empty values (e.g. Array<{ name?: string | null | undefined }> or { name: string | null | undefined }[]) so the items array is strongly typed for the null/undefined cases; update the items declaration in definition-issue-checkers.unit.test.ts and keep the rest of the test calling checker(items, { path: [] }) unchanged.packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx (1)
90-93: Build the cached container with a plugin store derived from the finalized definition.
pluginStoreis created from raw input but used after multiple fix/mutation stages. Recomputing from the final definition avoids stale parser/plugin context in the new container.♻️ Proposed adjustment
- const pluginStore = createPluginSpecStore( + const pluginStore = createPluginSpecStore( parserContext.pluginStore, rawProjectDefinition, ); @@ const fixedProjectDefinition = result.value; + const finalPluginStore = createPluginSpecStore( + parserContext.pluginStore, + fixedProjectDefinition, + ); fixedProjectDefinition.cliVersion = cliVersion; const definitionContainer = new ProjectDefinitionContainer( result.refPayload, parserContext, - pluginStore, + finalPluginStore, );Also applies to: 132-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx` around lines 90 - 93, The plugin store is being built from rawProjectDefinition (createPluginSpecStore(parserContext.pluginStore, rawProjectDefinition)) before the project definition is finalized, which can lead to stale plugin/parser context; move or re-run createPluginSpecStore after the fix/mutation stages and pass the finalized project definition (e.g., finalProjectDefinition or fixedProjectDefinition) instead of rawProjectDefinition, and update the other occurrence mentioned (the block around lines 132-136) to do the same so the cached container is constructed from the finalized definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/project-builder-lib/src/parser/clean-default-values.ts`:
- Around line 21-25: The callback passed to transformDataWithSchema can return
undefined at the root, violating the T return type; modify the default-stripping
condition in the callback (used with transformDataWithSchema,
definitionDefaultRegistry, isEqual) to only return undefined for non-root paths
(e.g., check ctx.path.length > 0 or similar) so the root node is never replaced
with undefined—otherwise return the original value.
In `@packages/project-builder-lib/src/parser/collect-definition-issues.ts`:
- Around line 78-79: Update the docstring that currently states "Issues do not
block saving but block syncing until resolved" to accurately reflect the defined
semantics: note that only issues with severity "error" are save-blocking while
other severities are not; reference the error severity in the description and
clarify that non-error severities block syncing but not saving. Update the
comment near the collectDefinitionIssues implementation (or the severity
enum/constant definitions used by collectDefinitionIssues) to explicitly state
"error = save-blocking; other severities = do not block save (may block sync)".
Ensure the wording matches the code's semantics and uses the exact token "error"
and "save-blocking" so it stays consistent with the implementation.
In `@packages/project-builder-lib/src/parser/transform-data-with-schema.ts`:
- Around line 144-153: The optional/nullable/nonoptional/readonly branches
incorrectly treat both null and undefined as equivalent; update the case
handlers in transformNode (the 'optional', 'nullable', 'nonoptional', 'readonly'
switch cases) so they only early-exit for the appropriate value: 'optional' and
'nonoptional' and 'readonly' should break only when data === undefined, while
'nullable' should break only when data === null; ensure you use the inner type
extraction (e.g., (schema as ZodOptional)._zod.def.innerType or the
corresponding ZodNullable innerType) and otherwise call transformNode as before,
and apply the identical fixes in schema-walker.ts to keep behavior consistent.
In
`@packages/project-builder-lib/src/schema/creator/definition-issue-checkers.ts`:
- Around line 25-31: The checker currently assumes items is an array and
directly calls items.entries(), which throws if items is undefined or not
iterable; add a defensive guard at the start of the returned function (the
function signature (items, ctx) => { ... }) to ensure items is an array (e.g.,
if items is falsy or not Array.isArray(items) return an empty issues array)
before creating seen or iterating with for (const [i, item] of items.entries());
this preserves existing behavior of returning DefinitionIssue[] but prevents
runtime exceptions when traversal supplies malformed input.
In
`@packages/project-builder-lib/src/schema/creator/definition-issue-registry.ts`:
- Around line 60-64: The example that builds a DefinitionIssue inside
withIssueChecker is missing the required severity field, causing type errors;
update the sample to include severity (e.g., severity: 'error' or an appropriate
enum value) when pushing to the issues array so the created object conforms to
the DefinitionIssue type; ensure the example in definition-issue-registry.ts
that references DefinitionIssue, withIssueChecker, and ctx.path shows something
like issues.push({ message: '...', path: ctx.path, severity: /* appropriate
severity */ }) to satisfy the typechecker.
In `@packages/project-builder-lib/src/schema/models/models.ts`:
- Around line 261-293: The checker function checkServiceMethods currently casts
unknown input to ModelServiceConfig and directly dereferences
service.create/service.update which can throw on malformed input; update the
function to first validate that value is a non-null object (e.g., typeof value
=== 'object' && value !== null) and that service.create and service.update exist
before accessing .enabled, using safe guards/optional chaining
(service?.create?.enabled) and short-circuit returns or add an issue for invalid
shape; apply the same hardening pattern to the other checker referenced (the
similar checker around lines 399-425) so both functions validate the input shape
before reading fields or transformerNames and gracefully report shape errors
instead of throwing.
In
`@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx`:
- Around line 106-123: The code validates newProjectDefinition before calling
fixRefDeletions but does not re-validate the mutated result, which can leave new
errors unchecked; after calling fixRefDeletions (result), re-run
collectDefinitionIssues(resultingSchema? actually pass defSchema, result,
pluginStore) and partitionIssuesBySeverity on that result and, if errors.length
> 0, throw a DefinitionIssueError(errors) to block persistence; update the block
around fixRefDeletions/createProjectDefinitionSchema so the persisted definition
is the validated/fixed result and any new issues are caught via
collectDefinitionIssues -> partitionIssuesBySeverity -> throw
DefinitionIssueError.
---
Outside diff comments:
In
`@packages/project-builder-web/src/routes/data/models/-hooks/use-model-form.ts`:
- Around line 93-96: The duplicate-check uses data.id which can be missing and
causes self-collisions; in the existingModel lookup (definition.models.find)
replace the exclusion check that compares m.id !== data.id to use
updatedModel.id (or fallback to updatedModel.id if data may be undefined) so the
current model is correctly excluded when comparing m.name.toLowerCase() ===
updatedModel.name.toLowerCase().
---
Nitpick comments:
In `@packages/project-builder-lib/src/parser/transform-data-with-schema.ts`:
- Around line 283-286: The record branch currently sets childResult =
Object.fromEntries(entries) and thus preserves keys with undefined values
whereas the object branch filters out entries whose transformed value is
undefined; update the record handling in transform-data-with-schema (the block
that builds entries/changing childResult and uses Object.fromEntries) to drop
any entries with value === undefined before calling Object.fromEntries, or if
the intended semantics differ, add a clear comment/docstring in the record
branch explaining why undefined should be preserved to make the behavior
explicit to callers.
- Around line 288-303: Intersection traversal currently runs transformNode on
the left then passes the full leftResult into transformNode for the right,
causing registered transforms on overlapping fields to run twice; modify
transformNode (and its callers) to accept and propagate an
"alreadyTransformedKeys" (or similar) parameter so that when handling a
ZodIntersection in transform-data-with-schema.ts you collect keys mutated by the
left traversal (from transformNode calls) and pass that set into the right-side
transformNode call (typed._zod.def.right) to skip reapplying transforms for
those keys, updating transformNode signature and internal logic to consult that
set when deciding to run registered transforms.
In
`@packages/project-builder-lib/src/references/extend-parser-context-with-refs.ts`:
- Around line 108-139: Extract the duplicated entity-registration steps into a
small internal helper and call it from both the standalone withEnt function and
the inline withEnt inside extendParserContextWithRefs: the helper should take
the schema, entity input, and resolved idPath (defaulting idPath to ['id'] when
undefined) and invoke definitionRefRegistry.add with the shared object shape
(kind: 'entity', type, idPath, getNameResolver, parentSlot, provides), allowing
each caller to preserve its current typing differences for getNameResolver while
centralizing the shared logic.
In
`@packages/project-builder-lib/src/schema/creator/definition-issue-checker-spec.ts`:
- Around line 38-40: getAllCheckers currently returns the live mutable Map
stored in values.checkers which leaks internal state; change getAllCheckers to
return a defensive copy (e.g., a new Map constructed from values.checkers) or an
immutable/read-only view so callers cannot mutate the original Map, and update
any callers expecting mutability to work with the copied Map; locate the
getAllCheckers function and replace the direct return of values.checkers with a
safe copy/immutable alternative.
In
`@packages/project-builder-lib/src/schema/creator/definition-issue-checkers.unit.test.ts`:
- Around line 35-42: The test uses an unsafe double-cast on items; replace "as
unknown as { name: string }[]" with a correct type that permits
undefined/null/empty values (e.g. Array<{ name?: string | null | undefined }> or
{ name: string | null | undefined }[]) so the items array is strongly typed for
the null/undefined cases; update the items declaration in
definition-issue-checkers.unit.test.ts and keep the rest of the test calling
checker(items, { path: [] }) unchanged.
In
`@packages/project-builder-lib/src/schema/creator/extend-parser-context-with-defaults.ts`:
- Around line 29-36: The withDefault function currently accepts any defaultValue
and applies it without checking against the provided schema; add a fail-fast
guard that validates defaultValue against the schema (use schema.safeParse or
equivalent on the Zod type) before calling prefault and optional, and if
validation fails throw a clear Error describing the mismatch; update the
implementation around withDefault, the generic T extends z.ZodType handling, and
the spots that call (schema as z.ZodType).prefault/default registration
(definitionDefaultRegistry.set) so the registry only stores defaults that
successfully validate.
In
`@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx`:
- Around line 90-93: The plugin store is being built from rawProjectDefinition
(createPluginSpecStore(parserContext.pluginStore, rawProjectDefinition)) before
the project definition is finalized, which can lead to stale plugin/parser
context; move or re-run createPluginSpecStore after the fix/mutation stages and
pass the finalized project definition (e.g., finalProjectDefinition or
fixedProjectDefinition) instead of rawProjectDefinition, and update the other
occurrence mentioned (the block around lines 132-136) to do the same so the
cached container is constructed from the finalized definition.
In `@packages/project-builder-web/src/utils/error.ts`:
- Around line 31-35: The DefinitionIssueError currently stores the original
array reference which allows external mutation; update the DefinitionIssueError
class by making the issues property immutable (e.g., declare it as readonly) and
assign a defensive copy of the input in the constructor (use issues.slice() or
[...issues]) and optionally call Object.freeze(this.issues) to prevent further
mutation; reference the DefinitionIssueError class, its issues property, and its
constructor when applying this change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
.changeset/global-definition-validators.mdpackages/project-builder-lib/src/parser/apply-definition-fixes.tspackages/project-builder-lib/src/parser/apply-definition-fixes.unit.test.tspackages/project-builder-lib/src/parser/clean-default-values.tspackages/project-builder-lib/src/parser/collect-definition-issues.tspackages/project-builder-lib/src/parser/collect-definition-issues.unit.test.tspackages/project-builder-lib/src/parser/index.tspackages/project-builder-lib/src/parser/schema-walker.tspackages/project-builder-lib/src/parser/schema-walker.unit.test.tspackages/project-builder-lib/src/parser/transform-data-with-schema.tspackages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.tspackages/project-builder-lib/src/references/extend-parser-context-with-refs.tspackages/project-builder-lib/src/references/extract-definition-refs.tspackages/project-builder-lib/src/references/index.tspackages/project-builder-lib/src/schema/creator/definition-default-registry.tspackages/project-builder-lib/src/schema/creator/definition-fix-registry.tspackages/project-builder-lib/src/schema/creator/definition-issue-checker-spec.tspackages/project-builder-lib/src/schema/creator/definition-issue-checkers.tspackages/project-builder-lib/src/schema/creator/definition-issue-checkers.unit.test.tspackages/project-builder-lib/src/schema/creator/definition-issue-registry.tspackages/project-builder-lib/src/schema/creator/definition-issue-types.tspackages/project-builder-lib/src/schema/creator/extend-parser-context-with-defaults.tspackages/project-builder-lib/src/schema/creator/index.tspackages/project-builder-lib/src/schema/models/models.tspackages/project-builder-lib/src/schema/project-definition.tspackages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsxpackages/project-builder-web/src/routes/data/models/-hooks/use-model-form.tspackages/project-builder-web/src/utils/error.ts
Comment on lines
+21
to
25
| return transformDataWithSchema(schema, data, (value, ctx) => { | ||
| const meta = definitionDefaultRegistry.get(ctx.schema); | ||
| if (meta && isEqual(value, meta.defaultValue)) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Prevent stripping the root node to undefined.
As written, a root-level default match can return undefined from a function typed as T. Restrict default-stripping to non-root paths.
🐛 Proposed fix
return transformDataWithSchema(schema, data, (value, ctx) => {
const meta = definitionDefaultRegistry.get(ctx.schema);
- if (meta && isEqual(value, meta.defaultValue)) {
+ if (ctx.path.length > 0 && meta && isEqual(value, meta.defaultValue)) {
return undefined;
}
return value;
});📝 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 transformDataWithSchema(schema, data, (value, ctx) => { | |
| const meta = definitionDefaultRegistry.get(ctx.schema); | |
| if (meta && isEqual(value, meta.defaultValue)) { | |
| return undefined; | |
| } | |
| return transformDataWithSchema(schema, data, (value, ctx) => { | |
| const meta = definitionDefaultRegistry.get(ctx.schema); | |
| if (ctx.path.length > 0 && meta && isEqual(value, meta.defaultValue)) { | |
| return undefined; | |
| } | |
| return value; | |
| }); |
🤖 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
21 - 25, The callback passed to transformDataWithSchema can return undefined at
the root, violating the T return type; modify the default-stripping condition in
the callback (used with transformDataWithSchema, definitionDefaultRegistry,
isEqual) to only return undefined for non-root paths (e.g., check
ctx.path.length > 0 or similar) so the root node is never replaced with
undefined—otherwise return the original value.
Comment on lines
+78
to
+79
| * Issues do not block saving but block syncing until resolved. | ||
| */ |
There was a problem hiding this comment.
Fix the save-blocking semantics in this docstring.
This says issues do not block saving, but error severity is defined as save-blocking.
📝 Suggested doc fix
- * Issues do not block saving but block syncing until resolved.
+ * Error issues block saving; warning issues allow saving but block syncing.📝 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
| * Issues do not block saving but block syncing until resolved. | |
| */ | |
| * Error issues block saving; warning issues allow saving but block syncing. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/parser/collect-definition-issues.ts` around
lines 78 - 79, Update the docstring that currently states "Issues do not block
saving but block syncing until resolved" to accurately reflect the defined
semantics: note that only issues with severity "error" are save-blocking while
other severities are not; reference the error severity in the description and
clarify that non-error severities block syncing but not saving. Update the
comment near the collectDefinitionIssues implementation (or the severity
enum/constant definitions used by collectDefinitionIssues) to explicitly state
"error = save-blocking; other severities = do not block save (may block sync)".
Ensure the wording matches the code's semantics and uses the exact token "error"
and "save-blocking" so it stays consistent with the implementation.
Comment on lines
+144
to
+153
| case 'optional': { | ||
| if (data === undefined || data === null) break; | ||
| childResult = transformNode( | ||
| (schema as ZodOptional)._zod.def.innerType as z.ZodType, | ||
| data, | ||
| path, | ||
| transform, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the transform-data-with-schema.ts file around the optional case
fd 'transform-data-with-schema.ts' packages/project-builder-lib/src/parser/Repository: halfdomelabs/baseplate
Length of output: 135
🏁 Script executed:
# Read the file and examine the optional/nonoptional/readonly cases
head -n 200 packages/project-builder-lib/src/parser/transform-data-with-schema.ts | tail -n 80Repository: halfdomelabs/baseplate
Length of output: 2017
🏁 Script executed:
# Check schema-walker.ts for comparison
fd 'schema-walker.ts' packages/project-builder-lib/src/parser/Repository: halfdomelabs/baseplate
Length of output: 122
🏁 Script executed:
# If schema-walker exists, check its optional handling
rg -n 'case.*optional' packages/project-builder-lib/src/parser/ -A 6 -B 1Repository: halfdomelabs/baseplate
Length of output: 3126
🏁 Script executed:
# Search for tests covering optional/nullable combinations
rg -l 'optional.*nullable|nullable.*optional' packages/project-builder-lib/src --type tsRepository: halfdomelabs/baseplate
Length of output: 132
🏁 Script executed:
# Look for any tests of the transform function with null data
rg -n 'null' packages/project-builder-lib/src/parser/__tests__ -A 2 -B 2 --type ts | head -100Repository: halfdomelabs/baseplate
Length of output: 155
🏁 Script executed:
# Check if there are comments explaining the null handling logic
rg -n 'optional|nullable' packages/project-builder-lib/src/parser/transform-data-with-schema.ts -B 3 | grep -E '\/\/|\/\*'Repository: halfdomelabs/baseplate
Length of output: 48
🏁 Script executed:
# Find test files for transform-data-with-schema
fd 'transform.*test' packages/project-builder-lib/src/parser/Repository: halfdomelabs/baseplate
Length of output: 145
🏁 Script executed:
# Check the test file for schema-walker to understand expected behavior
fd '\.test\.ts$|\.spec\.ts$' packages/project-builder-lib/src/parser/ | head -5Repository: halfdomelabs/baseplate
Length of output: 441
🏁 Script executed:
# Let's understand Zod's semantics for optional/nullable
rg -n 'z\.optional|z\.nullable' packages/project-builder-lib/src/parser/ -A 2 -B 2 | head -60Repository: halfdomelabs/baseplate
Length of output: 48
🏁 Script executed:
# Check if there's any documentation or type definitions explaining the behavior
rg -n 'ZodOptional|ZodNullable' packages/project-builder-lib/src/parser/transform-data-with-schema.ts -B 2Repository: halfdomelabs/baseplate
Length of output: 494
🏁 Script executed:
# Read the transform test file to understand expected behavior
wc -l packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.tsRepository: halfdomelabs/baseplate
Length of output: 149
🏁 Script executed:
# Check the test file for optional/nullable cases
rg -n 'optional|nullable' packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.ts -B 2 -A 5 | head -80Repository: halfdomelabs/baseplate
Length of output: 1268
🏁 Script executed:
# Look specifically for tests with null data
rg -n 'null' packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.ts -B 2 -A 3 | head -60Repository: halfdomelabs/baseplate
Length of output: 48
🏁 Script executed:
# Read the full test file to look for nullable/null tests
sed -n '150,250p' packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.tsRepository: halfdomelabs/baseplate
Length of output: 2937
🏁 Script executed:
# Search for any test combining optional and nullable
rg -n 'nullable' packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.ts -B 3 -A 8Repository: halfdomelabs/baseplate
Length of output: 48
🏁 Script executed:
# Check the entire test section around optional handling
sed -n '154,200p' packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.tsRepository: halfdomelabs/baseplate
Length of output: 1441
🏁 Script executed:
# Check if there are any tests with nullable data in optional contexts
grep -n 'nullable\|null' packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.ts | head -40Repository: halfdomelabs/baseplate
Length of output: 48
🏁 Script executed:
# Look for any tests that combine nullable and optional
sed -n '1,288p' packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.ts | grep -A 10 -B 2 'nullable'Repository: halfdomelabs/baseplate
Length of output: 48
🏁 Script executed:
# Check the full implementation context around nullable handling
sed -n '144,165p' packages/project-builder-lib/src/parser/transform-data-with-schema.tsRepository: halfdomelabs/baseplate
Length of output: 649
🏁 Script executed:
# Let's verify the semantic definition by checking how the types are used in context
sed -n '1,50p' packages/project-builder-lib/src/parser/transform-data-with-schema.tsRepository: halfdomelabs/baseplate
Length of output: 1420
🏁 Script executed:
# Check if there's documentation about the intended behavior
rg -n 'transformDataWithSchema|SchemaTransformContext' packages/project-builder-lib/src/parser/transform-data-with-schema.ts -B 5 | head -50Repository: halfdomelabs/baseplate
Length of output: 743
🏁 Script executed:
# Let's check the full transformNode function to understand step 2 (transform callback)
sed -n '320,360p' packages/project-builder-lib/src/parser/transform-data-with-schema.tsRepository: halfdomelabs/baseplate
Length of output: 872
🏁 Script executed:
# Check if there are any comments or context around why both conditions are checked
sed -n '50,90p' packages/project-builder-lib/src/parser/transform-data-with-schema.tsRepository: halfdomelabs/baseplate
Length of output: 1401
optional, nullable, nonoptional, and readonly cases incorrectly break on both null and undefined.
Semantically, z.optional(T) means T | undefined, so null should not trigger early exit—it should descend to the inner type. Similarly, z.nullable(T) means T | null, so only null should be checked, not undefined. The current code breaks on both conditions for all four wrapper types.
This pattern is also present in schema-walker.ts (line 173 for optional, line 192 for nonoptional), suggesting it may be intentional. However, for correct semantic handling:
optionalandreadonlyshould only break ondata === undefinednullableshould only break ondata === nullnonoptionalshould only break ondata === undefined(since it removes the optional wrapper)
In practice, the impact is minimal for definition schemas (the transform callback at the wrapper level still fires; only inner transforms are skipped for invalid data), but the semantic behavior should match Zod's type semantics.
Proposed fixes
case 'optional': {
- if (data === undefined || data === null) break;
+ if (data === undefined) break;
childResult = transformNode( case 'nullable': {
- if (data === undefined || data === null) break;
+ if (data === null) break;
childResult = transformNode( case 'nonoptional': {
- if (data === undefined || data === null) break;
+ if (data === undefined) break;
childResult = transformNode( case 'readonly': {
- if (data === undefined || data === null) break;
+ if (data === undefined) break;
childResult = transformNode(Apply the same fixes to schema-walker.ts for consistency.
📝 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
| case 'optional': { | |
| if (data === undefined || data === null) break; | |
| childResult = transformNode( | |
| (schema as ZodOptional)._zod.def.innerType as z.ZodType, | |
| data, | |
| path, | |
| transform, | |
| ); | |
| break; | |
| } | |
| case 'optional': { | |
| if (data === undefined) break; | |
| childResult = transformNode( | |
| (schema as ZodOptional)._zod.def.innerType as z.ZodType, | |
| data, | |
| path, | |
| transform, | |
| ); | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/parser/transform-data-with-schema.ts` around
lines 144 - 153, The optional/nullable/nonoptional/readonly branches incorrectly
treat both null and undefined as equivalent; update the case handlers in
transformNode (the 'optional', 'nullable', 'nonoptional', 'readonly' switch
cases) so they only early-exit for the appropriate value: 'optional' and
'nonoptional' and 'readonly' should break only when data === undefined, while
'nullable' should break only when data === null; ensure you use the inner type
extraction (e.g., (schema as ZodOptional)._zod.def.innerType or the
corresponding ZodNullable innerType) and otherwise call transformNode as before,
and apply the identical fixes in schema-walker.ts to keep behavior consistent.
Comment on lines
+25
to
+31
| return (items, ctx) => { | ||
| const issues: DefinitionIssue[] = []; | ||
| const seen = new Map<unknown, ReferencePath>(); | ||
|
|
||
| for (const [i, item] of items.entries()) { | ||
| const value = item[field]; | ||
| if (value === undefined || value === null || value === '') continue; |
There was a problem hiding this comment.
Add a defensive array guard before iterating items.
This checker currently assumes array input and can throw on .entries() if invoked with undefined or malformed data during traversal, which would break issue collection.
🛡️ Proposed fix
return (items, ctx) => {
+ if (!Array.isArray(items)) return [];
+
const issues: DefinitionIssue[] = [];
const seen = new Map<unknown, ReferencePath>();📝 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 (items, ctx) => { | |
| const issues: DefinitionIssue[] = []; | |
| const seen = new Map<unknown, ReferencePath>(); | |
| for (const [i, item] of items.entries()) { | |
| const value = item[field]; | |
| if (value === undefined || value === null || value === '') continue; | |
| return (items, ctx) => { | |
| if (!Array.isArray(items)) return []; | |
| const issues: DefinitionIssue[] = []; | |
| const seen = new Map<unknown, ReferencePath>(); | |
| for (const [i, item] of items.entries()) { | |
| const value = item[field]; | |
| if (value === undefined || value === null || value === '') continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/schema/creator/definition-issue-checkers.ts`
around lines 25 - 31, The checker currently assumes items is an array and
directly calls items.entries(), which throws if items is undefined or not
iterable; add a defensive guard at the start of the returned function (the
function signature (items, ctx) => { ... }) to ensure items is an array (e.g.,
if items is falsy or not Array.isArray(items) return an empty issues array)
before creating seen or iterating with for (const [i, item] of items.entries());
this preserves existing behavior of returning DefinitionIssue[] but prevents
runtime exceptions when traversal supplies malformed input.
Comment on lines
+261
to
+293
| function checkServiceMethods( | ||
| value: unknown, | ||
| ctx: FieldIssueCheckerContext, | ||
| ): DefinitionIssue[] { | ||
| const service = value as ModelServiceConfig; | ||
| const issues: DefinitionIssue[] = []; | ||
|
|
||
| if ( | ||
| service.create.enabled && | ||
| !service.create.fields?.length && | ||
| !service.create.transformerNames?.length | ||
| ) { | ||
| issues.push({ | ||
| message: 'Create method must have at least one field or transformer.', | ||
| path: [...ctx.path, 'create'], | ||
| severity: 'error', | ||
| }); | ||
| } | ||
|
|
||
| if ( | ||
| service.update.enabled && | ||
| !service.update.fields?.length && | ||
| !service.update.transformerNames?.length | ||
| ) { | ||
| issues.push({ | ||
| message: 'Update method must have at least one field or transformer.', | ||
| path: [...ctx.path, 'update'], | ||
| severity: 'error', | ||
| }); | ||
| } | ||
|
|
||
| return issues; | ||
| } |
There was a problem hiding this comment.
Guard issue checkers before dereferencing unknown input.
Both checkers assume a valid shape immediately. If malformed data reaches this stage, these property accesses can throw and short-circuit issue reporting.
🛡️ Proposed hardening
function checkServiceMethods(
value: unknown,
ctx: FieldIssueCheckerContext,
): DefinitionIssue[] {
- const service = value as ModelServiceConfig;
+ const service = value as Partial<ModelServiceConfig> | undefined;
const issues: DefinitionIssue[] = [];
+ if (!service?.create || !service?.update) {
+ return issues;
+ }
if (
service.create.enabled &&
!service.create.fields?.length &&
!service.create.transformerNames?.length
) {
@@
function checkModelConstraints(
value: unknown,
ctx: FieldIssueCheckerContext,
): DefinitionIssue[] {
- const model = value as {
- model: { fields: unknown[]; primaryKeyFieldRefs: string[] };
- };
+ const model = value as {
+ model?: { fields?: unknown[]; primaryKeyFieldRefs?: string[] };
+ };
const issues: DefinitionIssue[] = [];
+ const fields = model.model?.fields;
+ const primaryKeyFieldRefs = model.model?.primaryKeyFieldRefs;
+ if (!Array.isArray(fields) || !Array.isArray(primaryKeyFieldRefs)) {
+ return issues;
+ }
- if (model.model.fields.length === 0) {
+ if (fields.length === 0) {
issues.push({
message: 'Model must have at least one field.',
path: [...ctx.path, 'model', 'fields'],
severity: 'error',
});
}
- if (model.model.primaryKeyFieldRefs.length === 0) {
+ if (primaryKeyFieldRefs.length === 0) {
issues.push({
message: 'Model must have at least one primary key field.',
path: [...ctx.path, 'model', 'primaryKeyFieldRefs'],
severity: 'error',
});
}Also applies to: 399-425
🤖 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 261 -
293, The checker function checkServiceMethods currently casts unknown input to
ModelServiceConfig and directly dereferences service.create/service.update which
can throw on malformed input; update the function to first validate that value
is a non-null object (e.g., typeof value === 'object' && value !== null) and
that service.create and service.update exist before accessing .enabled, using
safe guards/optional chaining (service?.create?.enabled) and short-circuit
returns or add an issue for invalid shape; apply the same hardening pattern to
the other checker referenced (the similar checker around lines 399-425) so both
functions validate the input shape before reading fields or transformerNames and
gracefully report shape errors instead of throwing.
Comment on lines
+106
to
123
| // Collect and check definition issues | ||
| const issues = collectDefinitionIssues( | ||
| defSchema, | ||
| newProjectDefinition, | ||
| pluginStore, | ||
| ); | ||
| const { errors } = partitionIssuesBySeverity(issues); | ||
|
|
||
| // Block save on errors | ||
| if (errors.length > 0) { | ||
| throw new DefinitionIssueError(errors); | ||
| } | ||
|
|
||
| const result = fixRefDeletions( | ||
| createProjectDefinitionSchema, | ||
| newProjectDefinition, | ||
| { plugins: pluginStore }, | ||
| schemaCreatorOptions, | ||
| ); |
There was a problem hiding this comment.
Re-validate after fixRefDeletions before persisting.
The current pipeline validates once, then mutates again via reference-deletion fixes. That can introduce new violations that are never checked before save.
✅ Proposed follow-up validation step
- const result = fixRefDeletions(
+ const result = fixRefDeletions(
createProjectDefinitionSchema,
newProjectDefinition,
schemaCreatorOptions,
);
if (result.type === 'failure') {
throw new RefDeleteError(result.issues);
}
const fixedProjectDefinition = result.value;
+ const postRefDeletionIssues = collectDefinitionIssues(
+ defSchema,
+ fixedProjectDefinition,
+ pluginStore,
+ );
+ const { errors: postRefDeletionErrors } =
+ partitionIssuesBySeverity(postRefDeletionIssues);
+ if (postRefDeletionErrors.length > 0) {
+ throw new DefinitionIssueError(postRefDeletionErrors);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx`
around lines 106 - 123, The code validates newProjectDefinition before calling
fixRefDeletions but does not re-validate the mutated result, which can leave new
errors unchecked; after calling fixRefDeletions (result), re-run
collectDefinitionIssues(resultingSchema? actually pass defSchema, result,
pluginStore) and partitionIssuesBySeverity on that result and, if errors.length
> 0, throw a DefinitionIssueError(errors) to block persistence; update the block
around fixRefDeletions/createProjectDefinitionSchema so the persisted definition
is the validated/fixed result and any new issues are caught via
collectDefinitionIssues -> partitionIssuesBySeverity -> throw
DefinitionIssueError.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/project-builder-web/src/routes/data/models/-hooks/use-model-form.ts (1)
95-99:⚠️ Potential issue | 🟠 MajorUse
updatedModel.idfor duplicate self-exclusionLine 97 uses
data.idin the duplicate-name filter. Ifidis omitted from form payload, this can flag the current model as a duplicate and block valid saves.Proposed fix
const existingModel = definition.models.find( (m) => - m.id !== data.id && + m.id !== updatedModel.id && m.name.toLowerCase() === updatedModel.name.toLowerCase(), );🤖 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/-hooks/use-model-form.ts` around lines 95 - 99, The duplicate-name check in existingModel uses data.id which may be missing from the form payload; change the self-exclusion to use updatedModel.id instead. Locate the definition.models.find(...) call (variable existingModel) and replace the comparator that currently checks m.id !== data.id with m.id !== updatedModel.id so the current model is correctly excluded when validating duplicate names.packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx (1)
77-113:⚠️ Potential issue | 🟡 MinorCache key should include plugin version and config to detect plugin updates.
The key is built by
.toSorted().join(',')over plugin IDs (lines 101–103), which represents only the set of plugin names/packages. If a plugin is updated (same ID/name, different version or config), the cache returns the stalePluginSpecStorebuilt with the old plugin configuration. Include version and config in the cache key to properly invalidate when plugin definitions change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx` around lines 77 - 113, The cache key currently computed as pluginIds = (rawProjectDefinition.plugins?.map((p) => p.id) ?? []).toSorted().join(',') only includes plugin IDs and can return stale PluginSpecStore; update the key generation inside saveDefinition to incorporate each plugin's version and relevant config (e.g., include p.version and a stable serialization of p.config) when building the key used by pluginSpecStoreCache.current; ensure the same augmented key is used for both the cache check (pluginSpecStoreCache.current?.key) and when setting pluginSpecStoreCache.current after creating pluginStore via createPluginSpecStore so changes in version or config invalidate the cache.
♻️ Duplicate comments (1)
packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx (1)
139-146: Re-validate afterfixRefDeletionsbefore persisting.The current pipeline validates
newProjectDefinitiononce beforefixRefDeletions, butfixRefDeletionscan mutate the definition (deletions, SET_UNDEFINED operations), potentially introducing new violations that are never checked before the save completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx` around lines 139 - 146, After calling fixRefDeletions(createProjectDefinitionSchema, newProjectDefinition, schemaCreatorOptions) you must re-validate the mutated newProjectDefinition against createProjectDefinitionSchema before persisting; if the second validation fails, throw/propagate a validation error (similar to how RefDeleteError is thrown for deletion failures) with the validation issues so mutated violations are not saved. Locate the call site around fixRefDeletions and add a second safeParse/validate step on the returned/modified newProjectDefinition, and ensure you handle both failure and success paths (throwing an appropriate error on failure and continuing to persist only on success).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/project-builder-lib/src/parser/transform-data-with-schema.ts`:
- Around line 166-186: The record branch currently builds entries including keys
whose transformed value is undefined, causing Object.fromEntries(entries) to
preserve keys instead of omitting them; update the loop in the 'record' case
(around transformNode(children.valueSchema, value, [...path, key], transform),
entries, changed) to skip pushing entries when transformed === undefined (or
filter entries before calling Object.fromEntries), and mark changed true when a
key is removed or a value is changed so childResult =
Object.fromEntries(filteredEntries) only includes keys with defined values.
In `@packages/project-builder-lib/src/parser/walk-data-with-schema.ts`:
- Around line 70-146: The cleanup invocation currently only runs on the happy
path in walkNode (file walk-data-with-schema.ts); wrap the traversal/children
switch and all recursive walkNode/getSchemaChildren calls in a try...finally so
that the for-loop that runs cleanups (cleanups[i]()) executes in the finally
block, preserving original behavior (still rethrowing any exceptions) and
ensuring visitors/slot state (e.g., ref-schema-visitor) are restored even when
an error occurs.
---
Outside diff comments:
In
`@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx`:
- Around line 77-113: The cache key currently computed as pluginIds =
(rawProjectDefinition.plugins?.map((p) => p.id) ?? []).toSorted().join(',') only
includes plugin IDs and can return stale PluginSpecStore; update the key
generation inside saveDefinition to incorporate each plugin's version and
relevant config (e.g., include p.version and a stable serialization of p.config)
when building the key used by pluginSpecStoreCache.current; ensure the same
augmented key is used for both the cache check
(pluginSpecStoreCache.current?.key) and when setting
pluginSpecStoreCache.current after creating pluginStore via
createPluginSpecStore so changes in version or config invalidate the cache.
In
`@packages/project-builder-web/src/routes/data/models/-hooks/use-model-form.ts`:
- Around line 95-99: The duplicate-name check in existingModel uses data.id
which may be missing from the form payload; change the self-exclusion to use
updatedModel.id instead. Locate the definition.models.find(...) call (variable
existingModel) and replace the comparator that currently checks m.id !== data.id
with m.id !== updatedModel.id so the current model is correctly excluded when
validating duplicate names.
---
Duplicate comments:
In
`@packages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsx`:
- Around line 139-146: After calling
fixRefDeletions(createProjectDefinitionSchema, newProjectDefinition,
schemaCreatorOptions) you must re-validate the mutated newProjectDefinition
against createProjectDefinitionSchema before persisting; if the second
validation fails, throw/propagate a validation error (similar to how
RefDeleteError is thrown for deletion failures) with the validation issues so
mutated violations are not saved. Locate the call site around fixRefDeletions
and add a second safeParse/validate step on the returned/modified
newProjectDefinition, and ensure you handle both failure and success paths
(throwing an appropriate error on failure and continuing to persist only on
success).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
packages/project-builder-lib/src/parser/collect-definition-issues.tspackages/project-builder-lib/src/parser/index.tspackages/project-builder-lib/src/parser/schema-structure.tspackages/project-builder-lib/src/parser/schema-structure.unit.test.tspackages/project-builder-lib/src/parser/schema-walker.tspackages/project-builder-lib/src/parser/transform-data-with-schema.tspackages/project-builder-lib/src/parser/walk-data-with-schema.tspackages/project-builder-lib/src/parser/walk-data-with-schema.unit.test.tspackages/project-builder-lib/src/references/extend-parser-context-with-refs.tspackages/project-builder-lib/src/references/extract-definition-refs.tspackages/project-builder-lib/src/references/ref-schema-visitor.tspackages/project-builder-lib/src/schema/creator/definition-fix-registry.tspackages/project-builder-lib/src/schema/creator/definition-issue-registry.tspackages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsxpackages/project-builder-web/src/routes/data/models/-hooks/use-model-form.ts
💤 Files with no reviewable changes (1)
- packages/project-builder-lib/src/parser/schema-walker.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/project-builder-lib/src/references/extract-definition-refs.ts
- packages/project-builder-lib/src/parser/collect-definition-issues.ts
- packages/project-builder-lib/src/schema/creator/definition-fix-registry.ts
Comment on lines
+70
to
+146
| // Step 2: Structural descent based on schema children. | ||
| const children = getSchemaChildren(schema, data, path); | ||
| switch (children.kind) { | ||
| case 'leaf': { | ||
| break; | ||
| } | ||
| case 'leaf-union': { | ||
| break; | ||
| } | ||
| case 'wrapper': { | ||
| if (children.skipIfNullish && (data === undefined || data === null)) | ||
| break; | ||
| walkNode(children.innerSchema, data, path, visitors); | ||
| break; | ||
| } | ||
| case 'object': { | ||
| if (data === null || data === undefined || typeof data !== 'object') | ||
| break; | ||
| for (const [key, fieldSchema] of children.entries) { | ||
| walkNode( | ||
| fieldSchema, | ||
| (data as Record<string, unknown>)[key], | ||
| [...path, key], | ||
| visitors, | ||
| ); | ||
| } | ||
| break; | ||
| } | ||
| case 'array': { | ||
| if (!Array.isArray(data)) break; | ||
| for (const [i, datum] of data.entries()) { | ||
| walkNode(children.elementSchema, datum, [...path, i], visitors); | ||
| } | ||
| break; | ||
| } | ||
| case 'tuple': { | ||
| if (!Array.isArray(data)) break; | ||
| for (const [i, itemSchema] of children.items.entries()) { | ||
| if (i < data.length) { | ||
| walkNode(itemSchema, data[i], [...path, i], visitors); | ||
| } | ||
| } | ||
| if (children.rest) { | ||
| for (let i = children.items.length; i < data.length; i++) { | ||
| walkNode(children.rest, data[i], [...path, i], visitors); | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| case 'record': { | ||
| if (data === null || data === undefined || typeof data !== 'object') | ||
| break; | ||
| for (const [key, value] of Object.entries( | ||
| data as Record<string, unknown>, | ||
| )) { | ||
| walkNode(children.valueSchema, value, [...path, key], visitors); | ||
| } | ||
| break; | ||
| } | ||
| case 'discriminated-union': { | ||
| if (children.match) { | ||
| walkNode(children.match, data, path, visitors); | ||
| } | ||
| break; | ||
| } | ||
| case 'intersection': { | ||
| // Walk both sides — data satisfies both schemas simultaneously | ||
| walkNode(children.left, data, path, visitors); | ||
| walkNode(children.right, data, path, visitors); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Step 3: Run cleanup functions in reverse order (innermost first). | ||
| for (let i = cleanups.length - 1; i >= 0; i--) { | ||
| cleanups[i](); | ||
| } |
There was a problem hiding this comment.
Ensure cleanup callbacks run even when traversal throws.
Cleanup callbacks currently run only on the happy path. If getSchemaChildren(...) or any recursive walkNode(...) throws, cleanup is skipped, which can leak visitor state (e.g., slot scope restoration in packages/project-builder-lib/src/references/ref-schema-visitor.ts).
💡 Proposed fix (run cleanup in finally)
function walkNode(
schema: z.ZodType,
data: unknown,
path: ReferencePath,
visitors: readonly SchemaNodeVisitor[],
): void {
const ctx: SchemaWalkContext = { path };
// Step 1: Call all visitors, collect any cleanup functions returned.
const cleanups: (() => void)[] = [];
for (const visitor of visitors) {
const cleanup = visitor.visit(schema, data, ctx);
if (cleanup) cleanups.push(cleanup);
}
- // Step 2: Structural descent based on schema children.
- const children = getSchemaChildren(schema, data, path);
- switch (children.kind) {
- // ...
- }
-
- // Step 3: Run cleanup functions in reverse order (innermost first).
- for (let i = cleanups.length - 1; i >= 0; i--) {
- cleanups[i]();
- }
+ try {
+ // Step 2: Structural descent based on schema children.
+ const children = getSchemaChildren(schema, data, path);
+ switch (children.kind) {
+ // ...
+ }
+ } finally {
+ // Step 3: Run cleanup functions in reverse order (innermost first).
+ for (let i = cleanups.length - 1; i >= 0; i--) {
+ cleanups[i]();
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/parser/walk-data-with-schema.ts` around
lines 70 - 146, The cleanup invocation currently only runs on the happy path in
walkNode (file walk-data-with-schema.ts); wrap the traversal/children switch and
all recursive walkNode/getSchemaChildren calls in a try...finally so that the
for-loop that runs cleanups (cleanups[i]()) executes in the finally block,
preserving original behavior (still rethrowing any exceptions) and ensuring
visitors/slot state (e.g., ref-schema-visitor) are restored even when an error
occurs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/project-builder-lib/src/parser/transform-data-with-schema.ts`:
- Around line 147-163: The tuple-transform code builds resultItems and sets
childResult = resultItems when any item changed, but if children.rest is absent
this drops any original trailing elements beyond children.items.length; change
the assignment so that when children.rest is falsy you preserve the original
tail by appending the original data.slice(children.items.length) to resultItems
before assigning to childResult (use transformNode, children.items,
children.rest, resultItems, childResult, changed to locate the logic).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/project-builder-lib/src/parser/transform-data-with-schema.tspackages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/project-builder-lib/src/parser/transform-data-with-schema.unit.test.ts
Comment on lines
+147
to
+163
| // Transform rest elements if the tuple has a rest schema | ||
| if (children.rest) { | ||
| for (let i = children.items.length; i < data.length; i++) { | ||
| const transformed = transformNode( | ||
| children.rest, | ||
| data[i], | ||
| [...path, i], | ||
| transform, | ||
| ); | ||
| if (transformed !== data[i]) changed = true; | ||
| resultItems.push(transformed); | ||
| } | ||
| } | ||
|
|
||
| if (changed) { | ||
| childResult = resultItems; | ||
| } |
There was a problem hiding this comment.
Prevent tail truncation when transforming tuples without rest.
At Line 161, rebuilding childResult from resultItems drops elements beyond children.items.length when children.rest is absent and any earlier tuple item changed. This can silently lose trailing user data in partially invalid inputs.
💡 Proposed fix
if (changed) {
+ // Preserve trailing items not covered by tuple schemas when there is no rest schema.
+ if (!children.rest && data.length > children.items.length) {
+ resultItems.push(...data.slice(children.items.length));
+ }
childResult = resultItems;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/parser/transform-data-with-schema.ts` around
lines 147 - 163, The tuple-transform code builds resultItems and sets
childResult = resultItems when any item changed, but if children.rest is absent
this drops any original trailing elements beyond children.items.length; change
the assignment so that when children.rest is falsy you preserve the original
tail by appending the original data.slice(children.items.length) to resultItems
before assigning to childResult (use transformNode, children.items,
children.rest, resultItems, childResult, changed to locate the logic).
kingston
deleted the
kingston/eng-1007-introduce-global-validators-for-settings
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
Behavior Changes
Models