feat(vue-vuetify): improve mixed and additional property editors - #2610
feat(vue-vuetify): improve mixed and additional property editors#2610kchobantonov wants to merge 2 commits into
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@sdirix please review |
|
coverage: 84.286%. remained the same — kchobantonov:feat/vue-vuetify-mixed-additional-properties into eclipsesource:master |
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Reworks mixed-type and dynamic additionalProperties editing in @jsonforms/vue-vuetify: object/array mixed values now get a splitpanes tree + detail editor, nested mixed controls navigate into that tree instead of nesting renderers, and property renaming is added both in the tree and in the Additional Properties editor.
The extraction of src/util/dynamicProperties.ts is the strongest part: findPropertySchema and getPropertyNameSchema come straight out of AdditionalProperties.vue and are now shared by both rename paths, with unit tests. The non-mutating schema preparation in createMixedRenderInfos and replacing the JSON.stringify descriptor-rebuild check with haveAdditionalPropertyNamesChanged are both real fixes.
Where to focus:
getPathAncestorNodeIdsinMixedRenderer.vuemixes up absolute and relative paths, so "reveal in tree" never expands ancestors when the mixed control's own path is non-empty (the normaladditionalPropertiescase). Reproduced in the running example app on both the Additional Properties and Mixed Object examples.- Making
splitpanesa required peer dependency plus re-implementing its base CSS inVSplitpanes.sassis a packaging decision that deserves an explicit call. MixedRenderer.vueis now ~1600 lines; the pure tree-building layer would be better off insrc/utilwhere it can be tested.
pnpm test, pnpm lint, pnpm type-check and the full pnpm build all pass on the merged branch, and the CSS output includes the splitpanes rules.
| const getPathAncestorNodeIds = (path: string): string[] => { | ||
| const segments = path.split('.').filter(Boolean); | ||
| const result = [toTreeNodeId(input.control.value.path)]; | ||
| let currentPath = input.control.value.path; | ||
|
|
||
| segments.slice(0, -1).forEach((segment) => { | ||
| currentPath = composePropertyPath(currentPath, segment); | ||
| result.push(toTreeNodeId(currentPath)); | ||
| }); | ||
|
|
||
| return result; | ||
| }; |
There was a problem hiding this comment.
This treats path as relative to input.control.value.path, but every caller passes an absolute path (selectCurrentPath, commitRename, deleteNode). For control path dynamic and target dynamic.a.b it produces dynamic, dynamic.dynamic, dynamic.dynamic.a, so the actual ancestor dynamic.a is never opened and the bogus ids get dropped again by the treeNodes watcher.
I reproduced it in the example app: with additionalProperties data {"dynamic":{"a":{"b":{"c":1}}}}, clicking the eye button on b switches the detail pane but leaves a collapsed and nothing highlighted in the tree. Depth 1 appears to work only because the root id is always included, and a root-scoped mixed control works because the prefix is empty. Running getRelativePath(path) first fixes it.
| additionalPropertyRowClasses(element: AdditionalPropertyType): string[] { | ||
| const schemaType = element.schema?.type; | ||
| const classes = ['additional-property-row']; | ||
|
|
||
| if (Array.isArray(schemaType)) { | ||
| classes.push('additional-property-row--mixed'); | ||
| } else if (schemaType === 'object') { | ||
| classes.push('additional-property-row--object'); | ||
| } else if (schemaType === 'array') { | ||
| classes.push('additional-property-row--array'); | ||
| } else { | ||
| classes.push('additional-property-row--primitive'); | ||
| } | ||
|
|
||
| return classes; | ||
| }, |
There was a problem hiding this comment.
The --mixed/--object/--array/--primitive modifiers aren't referenced by the new scoped styles or anywhere else in the package, so this computes classes nobody uses. Either style them or drop the method and put class="additional-property-row" on the row directly.
| <v-text-field | ||
| v-if="renamingNodeId === item.nodeId" | ||
| v-model="renameValue" | ||
| class="mixed-rename-input" | ||
| density="compact" | ||
| hide-details | ||
| autofocus | ||
| :error="Boolean(renameError)" | ||
| :title="renameError ?? undefined" | ||
| v-bind="vuetifyProps('v-text-field')" | ||
| @click.stop | ||
| @keydown.stop.enter="commitRename(item)" | ||
| @keydown.stop.esc="cancelRename" | ||
| @blur="commitRename(item)" | ||
| /> |
There was a problem hiding this comment.
The rename error is only reachable via the native title tooltip here (the field is hide-details), and renameError isn't recomputed while typing, so it stays red after you've fixed the name. The rename menu in AdditionalProperties.vue does this properly with :error-messages plus @update:model-value="updateRenameError(...)"; worth matching. In the browser the "already defined" message is invisible unless you hover the field.
| "dayjs": "^1.10.6", | ||
| "lodash": "^4.17.21", | ||
| "maska": "^2.1.11", | ||
| "splitpanes": "^3.1.5", |
There was a problem hiding this comment.
This makes splitpanes a hard install requirement for every consumer of the package, including those that never render a mixed type, and there's no peerDependenciesMeta marking it optional. Combined with VSplitpanes.sass re-implementing splitpanes' own base layout CSS (the splitpanes/dist/splitpanes.css import is gone), the package takes on 218 lines that have to track upstream. A maintainer should make that call explicitly, and the README quick start probably needs the new peer added.
| parentPath: string, | ||
| propertyName: string, | ||
| ): string => (parentPath ? `${parentPath}.${propertyName}` : propertyName); | ||
|
|
There was a problem hiding this comment.
Always dot-joining is right for core's current path handling, since resolveData and setDataAt split purely on . (setData.ts#L31), whereas core's compose drops the separator when the segment starts with [ (path.ts#L29). The downside is that a [foo] property created here won't resolve in material/vanilla/angular, which all still use composePaths. Fixing compose in core would make bracket support work everywhere instead of just this renderer set.
| function prepareObjectSchema(schema: JsonSchema): JsonSchema { | ||
| const objectSchema = cleanSchema(cloneDeep({ ...schema, type: 'object' })); | ||
| objectSchema.additionalProperties = | ||
| objectSchema.additionalProperties !== false | ||
| ? (objectSchema.additionalProperties ?? true) | ||
| : false; | ||
| return objectSchema; | ||
| } | ||
|
|
||
| function prepareArraySchema( | ||
| schema: JsonSchema, | ||
| rootSchema: JsonSchema, | ||
| ): JsonSchema { | ||
| const arraySchema = cleanSchema(cloneDeep({ ...schema, type: 'array' })); | ||
| arraySchema.items = arraySchema.items ?? {}; | ||
| arraySchema.items = cloneDeep( | ||
| resolveSchema(arraySchema.items as JsonSchema, rootSchema), | ||
| ); | ||
|
|
||
| if ((arraySchema.items as any) === true) { | ||
| arraySchema.items = { | ||
| type: [...JSON_TYPES], | ||
| }; | ||
| } else if ( | ||
| typeof (arraySchema.items as JsonSchema7).type !== 'string' && | ||
| !Array.isArray((arraySchema.items as JsonSchema7).type) | ||
| ) { | ||
| (arraySchema.items as JsonSchema7).type = [...JSON_TYPES]; | ||
| } | ||
|
|
||
| return arraySchema; | ||
| } | ||
|
|
||
| function prepareChildSchema( | ||
| childType: JsonDataType, | ||
| currentSchema: JsonSchema, | ||
| key: string, | ||
| index: number | null, | ||
| rootSchema: JsonSchema, | ||
| itemLabel?: string, | ||
| ): JsonSchema { | ||
| let childSchema: JsonSchema | undefined; | ||
|
|
||
| if (index !== null) { | ||
| childSchema = getArrayItemSchema(currentSchema, index, rootSchema); | ||
| childSchema = childSchema | ||
| ? { ...childSchema, title: itemLabel } | ||
| : { | ||
| type: [...JSON_TYPES], | ||
| title: itemLabel, | ||
| }; | ||
| } else { | ||
| childSchema = findPropertySchema(currentSchema, key, rootSchema); | ||
| childSchema = childSchema | ||
| ? { ...childSchema, title: key } | ||
| : { | ||
| type: [...JSON_TYPES], | ||
| title: key, | ||
| }; | ||
| } | ||
|
|
||
| if ( | ||
| childType !== 'object' && | ||
| childType !== 'array' && | ||
| (!childSchema.type || (childSchema.type as any) === true) | ||
| ) { | ||
| childSchema.type = [...JSON_TYPES]; | ||
| } | ||
|
|
||
| if (childType === 'object') { | ||
| return prepareObjectSchema(childSchema); | ||
| } | ||
|
|
||
| if (childType === 'array') { | ||
| return prepareArraySchema(childSchema, rootSchema); | ||
| } | ||
|
|
||
| return childSchema; | ||
| } | ||
|
|
||
| function createFallbackChildSchema(title: string): JsonSchema { | ||
| return { | ||
| type: [...JSON_TYPES], | ||
| title, | ||
| }; | ||
| } | ||
|
|
||
| function getSchemaDefaultType(schema: JsonSchema): JsonDataType { | ||
| const schemaTypes = getSchemaTypesAsArray(schema); | ||
| const firstType = | ||
| schemaTypes.find((type) => type !== 'null') ?? schemaTypes[0]; | ||
| return (firstType ?? 'object') as JsonDataType; | ||
| } | ||
|
|
||
| function createTreeNodeControl( | ||
| schema: JsonSchema, | ||
| path: string, | ||
| label: string, | ||
| enabled: boolean, | ||
| readonly: boolean, | ||
| ): TreeNodeControl { | ||
| return { | ||
| id: path, | ||
| schema, | ||
| uischema: createControlElement('#'), | ||
| path, | ||
| label, | ||
| required: false, | ||
| enabled, | ||
| readonly, | ||
| }; | ||
| } | ||
|
|
||
| function withoutEmptyChildren(node: MixedTreeNode): MixedTreeNode { | ||
| const children = node.children?.map(withoutEmptyChildren) ?? []; | ||
| if (children.length === 0) { | ||
| const rest = { ...node }; | ||
| delete rest.children; | ||
| return rest; | ||
| } | ||
| return { | ||
| ...node, | ||
| children, | ||
| }; | ||
| } | ||
|
|
||
| function getDisplayTitle(label: string, type: JsonDataType): string { | ||
| if (label) { | ||
| return label; | ||
| } | ||
| return type === 'array' ? '[]' : '{}'; | ||
| } | ||
|
|
||
| function isDynamicProperty(parentSchema: JsonSchema, key: string): boolean { | ||
| return !parentSchema.properties?.[key]; | ||
| } | ||
|
|
||
| function buildTreeFromData( | ||
| data: any, | ||
| schema: JsonSchema, | ||
| rootSchema: JsonSchema, | ||
| path: string, | ||
| label: string, | ||
| enabled: boolean, | ||
| readonly: boolean, | ||
| showPrimitives: boolean, | ||
| itemLabel: (index: number) => string, | ||
| ): MixedTreeNode[] { | ||
| const dataType = getJsonDataType(data); | ||
| if (dataType !== 'object' && dataType !== 'array') { | ||
| return []; | ||
| } | ||
|
|
||
| const nodes: MixedTreeNode[] = []; | ||
|
|
||
| function traverse( | ||
| value: any, | ||
| currentPath: string, | ||
| currentLabel: string, | ||
| currentSchema: JsonSchema, | ||
| children: MixedTreeNode[], | ||
| canRename = false, | ||
| canDelete = false, | ||
| ) { | ||
| const type = getJsonDataType(value); | ||
|
|
||
| if (type === 'object') { | ||
| const objectSchema = prepareObjectSchema(currentSchema); | ||
| const nodeId = toTreeNodeId(currentPath); | ||
| const node: MixedTreeNode = { | ||
| nodeId, | ||
| title: getDisplayTitle(currentLabel, type), | ||
| jsonType: type, | ||
| label: currentLabel, | ||
| canRename, | ||
| canDelete, | ||
| control: createTreeNodeControl( | ||
| objectSchema, | ||
| currentPath, | ||
| currentLabel, | ||
| enabled, | ||
| readonly, | ||
| ), | ||
| children: [], | ||
| }; | ||
| children.push(node); | ||
|
|
||
| Object.keys(value).forEach((key) => { | ||
| const childValue = value[key]; | ||
| const childPath = composePropertyPath(currentPath, key); | ||
| const rawChildType = getJsonDataType(childValue); | ||
| const childCanRename = isDynamicProperty(currentSchema, key); | ||
| const childCanDelete = true; | ||
| const initialChildSchema = | ||
| findPropertySchema(currentSchema, key, rootSchema) ?? | ||
| createFallbackChildSchema(key); | ||
| const childType = | ||
| rawChildType ?? getSchemaDefaultType(initialChildSchema); | ||
| const childSchema = prepareChildSchema( | ||
| childType, | ||
| currentSchema, | ||
| key, | ||
| null, | ||
| rootSchema, | ||
| ); | ||
|
|
||
| if (childType === 'object' || childType === 'array') { | ||
| traverse( | ||
| childValue ?? (childType === 'array' ? [] : {}), | ||
| childPath, | ||
| key, | ||
| childSchema, | ||
| node.children!, | ||
| childCanRename, | ||
| childCanDelete, | ||
| ); | ||
| } else if (showPrimitives) { | ||
| const nodeId = toTreeNodeId(childPath); | ||
| node.children!.push({ | ||
| nodeId, | ||
| title: key, | ||
| jsonType: childType, | ||
| label: key, | ||
| canRename: childCanRename, | ||
| canDelete: childCanDelete, | ||
| control: createTreeNodeControl( | ||
| childSchema, | ||
| childPath, | ||
| key, | ||
| enabled, | ||
| readonly, | ||
| ), | ||
| children: [], | ||
| }); | ||
| } | ||
| }); | ||
| } else if (type === 'array') { | ||
| const arraySchema = prepareArraySchema(currentSchema, rootSchema); | ||
| const nodeId = toTreeNodeId(currentPath); | ||
| const node: MixedTreeNode = { | ||
| nodeId, | ||
| title: getDisplayTitle(currentLabel, type), | ||
| jsonType: type, | ||
| label: currentLabel, | ||
| canRename, | ||
| canDelete, | ||
| control: createTreeNodeControl( | ||
| arraySchema, | ||
| currentPath, | ||
| currentLabel, | ||
| enabled, | ||
| readonly, | ||
| ), | ||
| children: [], | ||
| }; | ||
| children.push(node); | ||
|
|
||
| value.forEach((childValue: any, index: number) => { | ||
| const childType = getJsonDataType(childValue); | ||
| const childPath = composePropertyPath(currentPath, `${index}`); | ||
| const childLabel = itemLabel(index); | ||
| const childSchema = prepareChildSchema( | ||
| childType ?? 'object', | ||
| currentSchema, | ||
| '', | ||
| index, | ||
| rootSchema, | ||
| childLabel, | ||
| ); | ||
| const resolvedChildType = | ||
| childType ?? getSchemaDefaultType(childSchema); | ||
| if (resolvedChildType === 'object' || resolvedChildType === 'array') { | ||
| traverse( | ||
| childValue ?? (resolvedChildType === 'array' ? [] : {}), | ||
| childPath, | ||
| childLabel, | ||
| childSchema, | ||
| node.children!, | ||
| false, | ||
| true, | ||
| ); | ||
| } else if (showPrimitives) { | ||
| const nodeId = toTreeNodeId(childPath); | ||
| node.children!.push({ | ||
| nodeId, | ||
| title: childLabel, | ||
| jsonType: resolvedChildType, | ||
| label: childLabel, | ||
| canRename: false, | ||
| canDelete: true, | ||
| control: createTreeNodeControl( | ||
| childSchema, | ||
| childPath, | ||
| childLabel, | ||
| enabled, | ||
| readonly, | ||
| ), | ||
| children: [], | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| traverse(data, path, label, resolveSchema(schema, rootSchema), nodes); | ||
|
|
||
| return nodes.map(withoutEmptyChildren); | ||
| } | ||
|
|
||
| function flattenTree(nodes: MixedTreeNode[]): MixedTreeNode[] { | ||
| return nodes.flatMap((node) => [node, ...flattenTree(node.children ?? [])]); | ||
| } | ||
|
|
||
| function findNodeByPath( | ||
| nodes: MixedTreeNode[], | ||
| targetPath: string, | ||
| ): MixedTreeNode | undefined { | ||
| for (const node of nodes) { | ||
| if (node.nodeId === targetPath) { | ||
| return node; | ||
| } | ||
| const child = findNodeByPath(node.children ?? [], targetPath); | ||
| if (child) { | ||
| return child; | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
These are all pure, Vue-free functions and they're the part of this PR most likely to regress. Moving them next to the new src/util/dynamicProperties.ts would let them be unit tested the same way the validation helpers now are, and would cut this SFC back to something reviewable. Small thing while you're in there: findNodeByPath compares node.nodeId, so targetPath is a misleading parameter name.
| return { | ||
| id: path, | ||
| schema, | ||
| uischema: createControlElement('#'), | ||
| path, | ||
| label, | ||
| required: false, | ||
| enabled, | ||
| readonly, | ||
| }; |
There was a problem hiding this comment.
id, required and label are set for every node but never read (the detail pane only uses schema/uischema/path/enabled/readonly, and the template reads item.label). Dropping them from TreeNodeControl makes it obvious what the detail pane actually needs.
| :aria-label="mixedTranslations.viewAriaLabel(computedLabel)" | ||
| variant="text" | ||
| color="primary" | ||
| :disabled="!navigationContext" |
There was a problem hiding this comment.
This can never be true: the branch only renders when isNestedComplexType is set, which requires !isRoot, which requires navigationContext to exist.
| const treeNodes = computed(() => | ||
| showTreeView.value | ||
| ? buildTreeFromData( | ||
| input.control.value.data, | ||
| resolvedSchema.value ?? input.control.value.schema, | ||
| input.control.value.rootSchema, | ||
| input.control.value.path, | ||
| vuetifyControl.computedLabel.value, | ||
| input.control.value.enabled, | ||
| input.control.value.readonly, | ||
| showPrimitivesInTree.value, | ||
| mixedTranslations.itemLabel, | ||
| ) | ||
| : [], | ||
| ); |
There was a problem hiding this comment.
treeNodes depends on control.data, so every keystroke in the detail pane rebuilds the whole tree and each node runs cloneDeep + cleanSchema (twice for arrays, via prepareArraySchema). Fine for the example data, but it could get noticeable on larger objects; caching per node path or keying the rebuild on the data shape rather than the value would help.
3 participants
Summary
This PR improves the Vue Vuetify handling of mixed-type and dynamic additional properties.
It adds a tree-based editor for object and array values while keeping primitive values editable through the existing mixed renderer. The implementation is scoped to the Vue Vuetify package.
Changes
propertyNamespatternPropertiessplitpanesto the Vue Vuetify peer and development dependencies.Validation