From 8b18ab6a8533abb6d753fe1b5d3d6e251b9d3c90 Mon Sep 17 00:00:00 2001 From: tommy230 Date: Mon, 28 Sep 2026 19:39:05 +0000 Subject: [PATCH] fix(publisher): repair invalid props one by one instead of resetting the node When Value.Parse rejected a node's props, validateNodeProps replaced every declared prop with the module default, so a link with one unsupported target published as "Click here" pointing at "#" while its href and text sat valid in the store. The catch branch now parses each declared prop on its own and falls back to the default only for the props that still fail. Fast path, successful slow path, optional props and injected keys are unchanged. Co-Authored-By: Claude Opus 5.5 --- .../module-engine/validateNodeProps.test.ts | 117 +++++++++++++++++- src/core/module-engine/validateNodeProps.ts | 64 +++++++++- 2 files changed, 172 insertions(+), 9 deletions(-) diff --git a/src/__tests__/module-engine/validateNodeProps.test.ts b/src/__tests__/module-engine/validateNodeProps.test.ts index 2f98bef5e..31c1481db 100644 --- a/src/__tests__/module-engine/validateNodeProps.test.ts +++ b/src/__tests__/module-engine/validateNodeProps.test.ts @@ -12,6 +12,8 @@ * rawProps for them); Ref/recursive/cyclic schemas degrade safely to * the slow path. * (f) Every base module's propsSchema is fast-path eligible. + * (g) Per-prop repair: when one prop cannot be coerced, only that prop + * takes the module default; every other authored prop survives. */ import { describe, it, expect } from 'bun:test' @@ -86,8 +88,7 @@ describe('validateNodeProps — (a) coerce to schema defaults', () => { expect(result.visible).toBe(false) }) - it('falls back to module defaults when coercion fails catastrophically', () => { - // Provide a deeply invalid value that Value.Parse cannot recover. + it('falls back to the module default for a prop coercion cannot fix', () => { // We force a failure by using a schema whose type can't be coerced. const strictSchema = Type.Object({ id: Type.String({ pattern: '^[a-z]+$', default: 'fallback' }), @@ -98,11 +99,121 @@ describe('validateNodeProps — (a) coerce to schema defaults', () => { }) // "123" fails the /^[a-z]+$/ pattern — coercion cannot fix it. const result = validateNodeProps(strictDef, { id: '123' }) - // Should fall back to defaults expect(result.id).toBe('fallback') }) }) +// --------------------------------------------------------------------------- +// (g) Per-prop repair — one bad prop never resets the whole node +// --------------------------------------------------------------------------- + +describe('validateNodeProps — (g) per-prop repair', () => { + const RepairSchema = Type.Object({ + href: Type.String({ default: '#' }), + text: Type.String({ default: 'Click here' }), + target: Type.Union([Type.Literal('_self'), Type.Literal('_blank')], { default: '_self' }), + htmlAttributes: Type.Record(Type.String(), Type.String(), { default: {} }), + note: Type.Optional(Type.String()), + }) + const def = stubDef({ + propsSchema: RepairSchema, + defaults: Value.Create(RepairSchema) as Record, + }) + + it('a bad enum value takes its default while sibling props keep their authored values', () => { + const result = validateNodeProps(def, { + href: '/contact/', + text: 'Contact us', + target: '', + htmlAttributes: { id: 'cta' }, + }) + expect(result.target).toBe('_self') + expect(result.href).toBe('/contact/') + expect(result.text).toBe('Contact us') + expect(result.htmlAttributes).toEqual({ id: 'cta' }) + }) + + it('a bad nested object resets only that prop', () => { + const result = validateNodeProps(def, { + href: '/contact/', + text: 'Contact us', + target: '_blank', + htmlAttributes: { id: { nested: true } }, + }) + expect(result.htmlAttributes).toEqual({}) + expect(result.href).toBe('/contact/') + expect(result.text).toBe('Contact us') + expect(result.target).toBe('_blank') + }) + + it('still coerces and default-fills the props that can be repaired', () => { + const CountSchema = Type.Object({ + count: Type.Number({ default: 1 }), + mode: Type.Union([Type.Literal('a'), Type.Literal('b')], { default: 'a' }), + label: Type.String({ default: 'x' }), + }) + const countDef = stubDef({ + propsSchema: CountSchema, + defaults: Value.Create(CountSchema) as Record, + }) + const result = validateNodeProps(countDef, { count: '7', mode: 'zzz' }) + expect(result.count).toBe(7) + expect(result.mode).toBe('a') + expect(result.label).toBe('x') + }) + + it('an absent optional prop stays absent during repair', () => { + const result = validateNodeProps(def, { href: '/x', text: 'X', target: 'nope' }) + expect('note' in result).toBe(false) + expect(result.target).toBe('_self') + }) + + it('an absent optional prop with a default is filled during repair', () => { + const OptSchema = Type.Object({ + mode: Type.Union([Type.Literal('a'), Type.Literal('b')], { default: 'a' }), + opt: Type.Optional(Type.String({ default: 'OPT' })), + }) + const optDef = stubDef({ propsSchema: OptSchema, defaults: { mode: 'a' } }) + expect(validateNodeProps(optDef, { mode: 'zzz' }).opt).toBe('OPT') + }) + + it('repair does not throw when a plugin module ships non-object defaults', () => { + const badDef = stubDef({ + propsSchema: RepairSchema, + defaults: undefined as unknown as Record, + }) + expect(() => validateNodeProps(badDef, { href: '/x', text: 'X', target: 'nope' })).not.toThrow() + }) + + it('injected unknown keys survive repair', () => { + const media = { img: { url: 'https://example.com/a.jpg' } } + const result = validateNodeProps(def, { target: 'nope', _resolvedMediaByKey: media }) + expect(result._resolvedMediaByKey).toBe(media) + }) + + it('base.link: an unsupported target keeps href and text at publish time', () => { + const link = registry.getOrThrow('base.link') + const result = validateNodeProps(link, { + href: '/contact/', + text: 'Contact us', + target: '', + htmlAttributes: {}, + }) + expect(result.target).toBe('_self') + expect(result.href).toBe('/contact/') + expect(result.text).toBe('Contact us') + }) + + it('a non-object props schema still falls back to the module defaults wholesale', () => { + const arrDef = stubDef({ + propsSchema: Type.Array(Type.Number()), + defaults: { items: [1] }, + }) + const result = validateNodeProps(arrDef, { items: ['x'] }) + expect(result.items).toEqual([1]) + }) +}) + // --------------------------------------------------------------------------- // (b) Unknown injected fields survive validation // --------------------------------------------------------------------------- diff --git a/src/core/module-engine/validateNodeProps.ts b/src/core/module-engine/validateNodeProps.ts index 820fe6d4b..3e7a51044 100644 --- a/src/core/module-engine/validateNodeProps.ts +++ b/src/core/module-engine/validateNodeProps.ts @@ -24,6 +24,10 @@ * exactly as before, for non-conforming values and for schemas where * Check-pass does not imply Parse-identity. * + * 3. REPAIR — when the slow path throws, each declared prop is parsed on + * its own and only the props that still fail take the module default + * (see `repairProps`). One unsupported value never resets the node. + * * Design constraints: * - SOFT boundary — exceptions from coercion are caught; never bubbles. * - Unknown/injected keys survive — the fast path returns them untouched on @@ -181,8 +185,11 @@ function fastPathEligible(schema: TSchema): boolean { * - Schema present, coercion succeeds → `{ ...rawProps, ...cleanedProps }`. * Known props are coerced/defaulted by Value.Parse; unknown keys from * rawProps survive untouched. - * - Schema present, coercion fails → `{ ...rawProps, ...def.defaults }`. - * Falls back to module defaults for known keys, unknown keys still survive. + * - Schema present, coercion fails → `{ ...rawProps, ...repaired }` where + * each declared prop is parsed individually and only the props that + * cannot be coerced fall back to the module default for that key, when + * one is declared. + * Unknown keys still survive. */ export function validateNodeProps( def: AnyModuleDefinition, @@ -203,9 +210,54 @@ export function validateNodeProps( const cleaned = parseValue(def.propsSchema, rawProps) as Record return { ...rawProps, ...cleaned } } catch (_err) { - // Value.Parse threw — the input is unrecoverable for this schema even - // after applying defaults and type coercions. Fall back to the module's - // declared defaults, while still preserving any injected unknown keys. - return { ...rawProps, ...def.defaults } + // Value.Parse threw — at least one prop cannot be coerced into shape. + // Repair per prop so one bad value costs only that value, not the + // node's whole authored content; injected unknown keys still survive. + return { ...rawProps, ...repairProps(def, rawProps) } + } +} + +/** + * Tier 3 — per-prop repair for an object schema whose whole-value Parse + * failed. Every declared prop is parsed on its own (Default + Convert + + * Check for that leaf); a prop that still fails takes the module's declared + * default for that key. Props that pass stay exactly as authored, so a link + * with an unsupported `target` keeps its `href` and `text`, and a stale + * enum on one field does not wipe the rest of the node. + * + * A non-object schema has no per-prop granularity and keeps the old + * behaviour: the module defaults replace the value wholesale. So does a + * schema without TypeBox's `Kind` symbol (for example one rebuilt from + * JSON), because Value.Parse cannot read it. + */ +function repairProps( + def: AnyModuleDefinition, + rawProps: Record, +): Record { + const schema = def.propsSchema as TSchema + const properties: unknown = schema.properties + if (schema[Kind] !== 'Object' || !isSchemaObject(properties)) { + return { ...def.defaults } + } + + // Browser-loaded plugin packs pass `defaults` through unvalidated; this + // boundary never throws, so treat a non-object as "no defaults". + const defaults: Record = + typeof def.defaults === 'object' && def.defaults !== null ? def.defaults : {} + const repaired: Record = {} + for (const [key, propSchema] of Object.entries(properties)) { + if (!isSchemaObject(propSchema)) continue + const value = rawProps[key] + // An absent optional prop without a default is valid as-is; Parse on the + // bare leaf would reject `undefined` because the Optional modifier only + // means something inside the parent object. One with a default falls + // through so it is filled, as the successful slow path does. + if (value === undefined && OptionalKind in propSchema && !('default' in propSchema)) continue + try { + repaired[key] = parseValue(propSchema, value) + } catch (_err) { + if (key in defaults) repaired[key] = defaults[key] + } } + return repaired }