Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): stop the flow node inspector writing a spec-refused description, and converge the duplicated node/edge shapes - #6330
Merged
Conversation
…description` `FlowNodeSchema` is `.strict()` and refuses `description` by name, so the node inspector's Description field produced an unsavable draft on every keystroke (`unrecognized_keys` in client validation, 422 on save) and nothing ever read the value back. Remove the field, and strip a stored `description` on write so flows that already carry one heal on the author's first edit — the same migrate-on-write boundary the retired `ui` geometry gets. Converge the hand-written shape copies that hid the drift: the inspector's node and edge types now alias `FlowNodeLike` / `FlowDesignerEdge`, and `flow-decision-edges`' fourth edge copy aliases the canvas edge instead of restating it with a `condition?: unknown` that outlived objectui#3202's narrowing, taking its workaround cast with it. Pinned in both worlds: a compile-time assertion that the declared members of the node read type (index signature stripped) are a subset of the spec's own node keys, and runtime assertions that what the inspector emits parses through the real `FlowNodeSchema`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This was referenced Aug 25, 2026
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-support-ai
marked this pull request as ready for review
August 25, 2026 13:27
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes#6287
The card is right that
FlowNodeInspector's localFlowNodeadmits a key the strict contract refuses. What the measurements changed is what that costs and where the fix has to land: this is not type hygiene, it is a live UI control that made flows unsavable, and the type-only version of the fix would have been a ghost.What was measured first
Both premise probes reproduced against the installed
@objectstack/spec@17.2.0(lockfile read, not the^17.0.0specifier: single resolution17.2.0(ai@7.0.65(zod@4.4.3)), on-disk17.2.0), each with a positive control so a refusal cannot be an artefact of a broken probe:FlowNodeSchema's authorable surface is exactly eleven keys —id type label config connectorConfig position timeoutMs inputSchema outputSchema waitEventConfig boundaryConfig— with no note key of any spelling. So the premise holds, and the premise fork does not open: there is no reader for a node description to grow into, nothing inmetadata-adminreadsnode.descriptionback, and adding one to the spec would be a new authoring surface with zero measured consumers.The part the card did not have: the type was describing a live producer
FlowNodeInspector.tsx:373rendered a Description text field whoseonCommitwrotepatchNode({ description: v || undefined }). By this package's own established reading of.strict(), that is not untidiness:descriptionis refused by that same mechanism, measured above. So every keystroke in that box made the flow unsavable, and nothing read the value back. The field is removed, and its two now-dead locale entries with it.Stored flows heal on the author's first edit. Removing the field alone would have made a pre-existing bad state unrecoverable — an author who had already saved a
descriptionwould have had no control left to clear it. All three node write paths (patchNode,setField,commitAdvanced) now strip it, the same migrate-on-write boundarywithCanonicalGeometrygives the retireduigeometry. Deliberately a named key rather than "everything the spec does not list": the index signature is load-bearing (the canvas round-trips node properties this layer does not understand), so a blanket strip would be exactly the data loss that type exists to prevent.Why the obvious fix would have been a ghost — measured, twice
const node = loc?.node ?? nullisFlowNodeLike(exported fromflow-nested-selection.ts), a third copy of the shape carrying the samedescription?: string. Narrowing onlyFlowNodeInspector's copy would have changed no read in the file.[k: string]: unknownon the shape, an object literal carryingdescriptiontype-checks before and after. A@ts-expect-errornegative test does not go red before the fix — it goes red after it, as an unused directive. That trap is documented in the new test's header so the next reader does not re-lay it.So the pin is placed where the two worlds actually differ, and was proved red before the fix:
FlowNodekeys. Red before:tscexit 2, one error,FlowNodeInspector.specKeys.test.tsx(158,33): error TS2344: Type 'false' does not satisfy the constraint 'true'— the_NoRefusedKeyline. Green after. This closes the whole class, not the one key.FlowNodeSchema. Red before: 3 failed / 2 passed, the heal test failing onexpect('description' in node).toBe(false). Green after.Both carry anti-degeneracy guards:
IsAnyprobes plus an assertion that the index-signature strip left a non-empty key set (a strip that removed everything would pass every subset test while measuring nothing), and on the runtime side a positive control that the schema under test really is the strict one that refusesdescription, plusgetByLabelTextfinding the controls that should be present before asserting the Description control absent.The dispatch ruled
labelmust become required. Measured, under a trap with the mutation proved on disk (label?: string1→0,label: string0→1, blob9b19f411→dd5504a3) and the restore proved (blob back to9b19f411,git diff HEAD --statempty):tscexit 0, zero errors. Not one call site breaks.Every node reaches this type through
as FlowNodeLike[]casts out ofRecord<string, unknown>, and a cast bypasses a required member. So the change would catch nothing — while asserting over stored metadata a guarantee the reader cannot enforce. It is also mildly harmful:node.label ?? ''andnode.label || node.idare guards the optional type forces, and making it required would letnode.label.trim()compile against a value that is genuinely absent.That is the layer difference this repo already adjudicated, in the canonical canvas type's own words:
The two halves are not the same defect and do not point the same way.
descriptionwas an over-wide write: the panel produced a key the contract refuses. An optionallabelis a read looseness over data that genuinely occurs; it admits nothing the contract rejects, it only declines to promise something about stored data. The new pin is a key-set assertion, so it closes the refused-key class without freezing this optionality either way — if the maintainer wantslabelrequired, that stays available as a separate decision, on evidence about producers rather than about the contract.The duplicate copies
FlowPreview.tsxalready made this move for its own pair and its comment — "two copies of one shape is how the wrong one survives being fixed" — is about this one. Aliasing was reachable: both imports areimport type, erased at compile time, andflow-canvas-layoutis dependency-free by design, so no cycle and no new dependency.FlowNodeInspector's node type →FlowNodeLike(the type its value already had).FlowNodeInspector's edge type →FlowDesignerEdge, whoseconditionis the spec'sExpressionInput.flow-decision-edgesboundary — a fourth copy,DecisionEdge, still spellingcondition?: unknownmonths after #3202 narrowed the designer's edge, and incompatible in both directions (unknown→ExpressionInput, and no index signature the other way). The two ways forward were casts in this file, or converging that copy. Casts at the consumer are the tolerant-fallback anti-pattern — the defect is in the producer — soDecisionEdgeis now= FlowDesignerEdge, which also deletes thecondTextworkaround that existed only to cast back to the narrow type on every read. That helper is the module's own written admission that the looseness was never wanted: the one value it ever writes toconditionis a bare CEL string, whichExpressionInputhas always admitted. Mechanical and self-contained —tscwent 5 errors → 0, andflow-decision-edges.test.tspasses untouched.⛔
FlowEdgeInspector's declaration is not modified — it is the reference, and it agrees withFlowDesignerEdgeon the load-bearing member. Three copies of the edge shape are now one, and this panel inheritsflow-designer-edge.types.test.ts's existing pin instead of needing its own.Verification
All gates below ran on the final commit
081e2239d; exit codes captured before any pipe, and each result quoted from the gate's own verdict line.tsc -p packages/app-shell/tsconfig.test.jsonFINAL_TSC_EXIT=0, 0 errorstsc --noEmit(package source)FINAL_SRC_TSC_EXIT=0, 0 errorsTest Files 12 passed (12)eslint .(wholeapp-shell, unnarrowed)ESLINT_EXIT=0— 0 errors, 2679 pre-existing warningscheck:control-bytes✅ OK (scanned 5195 tracked text file(s))check:spec-symbols✅ spec symbol derivation: 1305 files scannedcheck:vi-mock-specifiers✅ OK (432 carry a mock)check:i18n-keys✅ Every in-scope call-site key resolves against the en packcheck:i18n-drift✅ No en value changed in this rangecheck-changeset-presence.mjs✅ 4 source file(s) of 1 released package(s) changed … declares 1 changeset(s)check-changeset-no-major.mjs✅ No changeset declares a major bumpThe new test file was confirmed genuinely in the checked program with
--listFiles, not assumed. The dependency closure was built first (pnpm --filter '@object-ui/app-shell^...' build) — the worktree had nodistanywhere, andtsconfig.test.jsonsetspaths: {}, so an unbuilt run would have buried the pin's single error under module-resolution noise and read as a convincing false red.Out of scope
ActionParam(ActionDefaultInspector.tsx:266/ActionPreview.tsx:47) was verified onorigin/mainand filed as #6329 — the two copies disagree onlabel's type,options,helpText,defaultValue, and the index signature. Not folded in: different files, different pair, and the card excluded it.scripts/check-spec-symbol-derivation.mjs's non-exported-declaration hole is #5899's and was not touched.Generated by Claude Code
Generated by Claude Code