Skip to content

Commit dd17d6c

Browse files
rasmivivtsai
rasmi
authored and
vivtsai
committed
Fix variable editor schema handling for multi-value configs.
1 parent 12a8b41 commit dd17d6c

File tree

4 files changed

+210
-76
lines changed

4 files changed

+210
-76
lines changed

‎frontend/src/components/experiment_builder/variable_editor.ts‎

Lines changed: 52 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {ExperimentEditor} from '../../services/experiment.editor';
2020

2121
import {
2222
type BalancedAssignmentVariableConfig,
23-
type RandomPermutationVariableConfig,
23+
type MultiValueVariableConfigType,
2424
type StaticVariableConfig,
2525
type VariableConfig,
2626
BalanceAcross,
@@ -35,18 +35,20 @@ import {
3535
sanitizeVariableName,
3636
createShuffleConfig,
3737
mapScopeToSeedStrategy,
38+
isMultiValueConfig,
3839
addPropertyToSchema,
3940
createSchemaForType,
4041
getDefaultValue,
4142
removePropertyFromSchema,
42-
renamePropertyInObject,
4343
safeParseJson,
4444
serializeForInput,
4545
setValueAtPath,
4646
updateArrayItem,
4747
updateObjectProperty,
4848
updatePropertyInSchema,
49-
updateSchemaAtPath,
49+
updateSchemaForConfig,
50+
setValueForConfig,
51+
renamePropertyForConfig,
5052
extractVariablesFromVariableConfigs,
5153
findUnusedVariables,
5254
} from '@deliberation-lab/utils';
@@ -687,16 +689,26 @@ export class VariableEditor extends MobxLitElement {
687689
path: string,
688690
config: VariableConfig,
689691
configIndex: number,
692+
/** Set to true after unwrapping the root array for multi-value configs */
693+
rootArrayUnwrapped = false,
690694
): TemplateResult {
691-
const type = schema.type as string;
692-
const isRandomPermutationRoot =
693-
config.type === VariableConfigType.RANDOM_PERMUTATION && path === '';
694-
695-
// For RandomPermutation at root, skip showing the array type and go straight to items
696-
if (isRandomPermutationRoot && type === 'array') {
697-
return this.renderArraySchema(schema, path, config, configIndex);
695+
const isMultiValueAtRoot = isMultiValueConfig(config.type) && path === '';
696+
697+
// For multi-value configs at root, extract item schema and show it directly.
698+
// Users work with individual items, not the array wrapper.
699+
// Only do this once (check rootArrayUnwrapped to prevent over-extraction for nested arrays).
700+
if (
701+
isMultiValueAtRoot &&
702+
!rootArrayUnwrapped &&
703+
'items' in schema &&
704+
schema.items
705+
) {
706+
const itemSchema = schema.items as TSchema;
707+
return this.renderSchemaType(itemSchema, path, config, configIndex, true);
698708
}
699709

710+
const type = schema.type as string;
711+
700712
return html`
701713
<div class="schema-type-editor ${path ? 'nested' : ''}">
702714
<div class="type-selector">
@@ -707,8 +719,8 @@ export class VariableEditor extends MobxLitElement {
707719
const newLocalSchema = createSchemaForType(
708720
(e.target as HTMLSelectElement).value,
709721
);
710-
const newFullSchema = updateSchemaAtPath(
711-
toJS(config.definition.schema) as unknown as TSchema,
722+
const newFullSchema = updateSchemaForConfig(
723+
config,
712724
path,
713725
newLocalSchema,
714726
);
@@ -801,8 +813,8 @@ export class VariableEditor extends MobxLitElement {
801813
(e.target as HTMLSelectElement).value,
802814
);
803815
const newArraySchema = Type.Array(newItemSchema);
804-
const newFullSchema = updateSchemaAtPath(
805-
toJS(config.definition.schema) as unknown as TSchema,
816+
const newFullSchema = updateSchemaForConfig(
817+
config,
806818
path,
807819
newArraySchema,
808820
);
@@ -811,7 +823,7 @@ export class VariableEditor extends MobxLitElement {
811823
configIndex,
812824
newFullSchema,
813825
path,
814-
getDefaultValue(newArraySchema),
826+
getDefaultValue(newItemSchema),
815827
);
816828
}}
817829
>
@@ -825,6 +837,12 @@ export class VariableEditor extends MobxLitElement {
825837
${itemType === 'object'
826838
? this.renderObjectSchema(items, path, config, configIndex)
827839
: nothing}
840+
${
841+
/* NOTE: Deeply nested arrays (3+ levels) may not update correctly
842+
because path doesn't change when entering array items. If needed,
843+
add [] notation to paths (e.g., 'prop.[].[].field') and handle
844+
in updateSchemaAtPath. */ ''
845+
}
828846
${itemType === 'array'
829847
? this.renderArraySchema(items, path, config, configIndex)
830848
: nothing}
@@ -849,12 +867,13 @@ export class VariableEditor extends MobxLitElement {
849867
};
850868
}
851869

852-
// Handle multi-value configs (RandomPermutation)
870+
// Handle multi-value configs (RandomPermutation, BalancedAssignment)
871+
// Each value is a single item, so we use the helper which operates on item schema
853872
if ('values' in config && Array.isArray(config.values)) {
854873
const updatedValues = config.values.map((jsonValue: string) => {
855874
const parsed = safeParseJson(jsonValue, {});
856875
const updatedValue = path
857-
? setValueAtPath(parsed, newFullSchema, path, defaultValue)
876+
? setValueForConfig(parsed, config, path, defaultValue)
858877
: defaultValue;
859878
return JSON.stringify(updatedValue);
860879
});
@@ -933,21 +952,17 @@ export class VariableEditor extends MobxLitElement {
933952
const newObjSchema = Type.Object(newProps);
934953

935954
// Update schema at the right path
936-
const updatedFullSchema = updateSchemaAtPath(
937-
toJS(config.definition.schema) as unknown as TSchema,
938-
path,
939-
newObjSchema,
940-
);
955+
const updatedFullSchema = updateSchemaForConfig(config, path, newObjSchema);
941956

942957
// Rename property in values
943958
const oldPropPath = path ? `${path}.${oldName}` : oldName;
944959

945960
// Update both schema and values in a single call
946961
if (config.type === VariableConfigType.STATIC) {
947962
const parsed = safeParseJson(config.value, {});
948-
const updated = renamePropertyInObject(
963+
const updated = renamePropertyForConfig(
949964
parsed,
950-
toJS(config.definition.schema) as unknown as TSchema,
965+
config,
951966
oldPropPath,
952967
newName,
953968
);
@@ -959,12 +974,13 @@ export class VariableEditor extends MobxLitElement {
959974
},
960975
value: JSON.stringify(updated),
961976
});
962-
} else if (config.type === VariableConfigType.RANDOM_PERMUTATION) {
977+
} else if (isMultiValueConfig(config)) {
978+
// Type guard narrows config to MultiValueVariableConfig
963979
const updatedValues = config.values.map((jsonValue: string) => {
964980
const parsed = safeParseJson(jsonValue, {});
965-
const updated = renamePropertyInObject(
981+
const updated = renamePropertyForConfig(
966982
parsed,
967-
toJS(config.definition.schema) as unknown as TSchema,
983+
config,
968984
oldPropPath,
969985
newName,
970986
);
@@ -1025,8 +1041,8 @@ export class VariableEditor extends MobxLitElement {
10251041
name,
10261042
newPropSchema,
10271043
);
1028-
const newFullSchema = updateSchemaAtPath(
1029-
toJS(config.definition.schema) as unknown as TSchema,
1044+
const newFullSchema = updateSchemaForConfig(
1045+
config,
10301046
path,
10311047
newObjSchema,
10321048
);
@@ -1062,8 +1078,8 @@ export class VariableEditor extends MobxLitElement {
10621078
@click=${() => {
10631079
if (!confirm(`Remove property "${name}"?`)) return;
10641080
const newObjSchema = removePropertyFromSchema(props, name);
1065-
const newFullSchema = updateSchemaAtPath(
1066-
toJS(config.definition.schema) as unknown as TSchema,
1081+
const newFullSchema = updateSchemaForConfig(
1082+
config,
10671083
path,
10681084
newObjSchema,
10691085
);
@@ -1103,11 +1119,7 @@ export class VariableEditor extends MobxLitElement {
11031119
() => {
11041120
if (!confirm(`Remove property "${name}"?`)) return;
11051121
const newObjSchema = removePropertyFromSchema(props, name);
1106-
const newFullSchema = updateSchemaAtPath(
1107-
toJS(config.definition.schema) as unknown as TSchema,
1108-
path,
1109-
newObjSchema,
1110-
);
1122+
const newFullSchema = updateSchemaForConfig(config, path, newObjSchema);
11111123
this.updateDefinition(config, configIndex, {
11121124
schema: newFullSchema as unknown as typeof config.definition.schema,
11131125
});
@@ -1133,11 +1145,7 @@ export class VariableEditor extends MobxLitElement {
11331145
alert('Property already exists');
11341146
return;
11351147
}
1136-
const newFullSchema = updateSchemaAtPath(
1137-
toJS(config.definition.schema) as unknown as TSchema,
1138-
path,
1139-
newObjSchema,
1140-
);
1148+
const newFullSchema = updateSchemaForConfig(config, path, newObjSchema);
11411149
// New properties default to string type with empty string value
11421150
const propPath = path ? `${path}.${name}` : name;
11431151
this.updateSchemaAndResetValues(
@@ -1172,17 +1180,17 @@ export class VariableEditor extends MobxLitElement {
11721180
`;
11731181
}
11741182

1175-
// ===== Value Editor (for RandomPermutation and BalancedAssignment values) =====
1183+
// ===== Value Editor (for multi-value configs) =====
11761184

11771185
private renderValueEditor(
1178-
config: RandomPermutationVariableConfig | BalancedAssignmentVariableConfig,
1186+
config: MultiValueVariableConfigType,
11791187
configIndex: number,
11801188
jsonValue: string,
11811189
valueIndex: number,
11821190
) {
11831191
// Strip MobX observability to prevent stack overflow (see renderSchemaEditor for details)
11841192
const arraySchema = toJS(config.definition.schema) as unknown as TSchema;
1185-
// Both RandomPermutation and BalancedAssignment store schema as Array(ItemType),
1193+
// Multi-value configs store schema as Array(ItemType),
11861194
// so extract the item schema for validation
11871195
const itemSchema =
11881196
'items' in arraySchema ? (arraySchema.items as TSchema) : arraySchema;

‎utils/src/variables.schema.utils.ts‎

Lines changed: 104 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import {Type, type TSchema, type TObject, type TArray} from '@sinclair/typebox';
2-
import {parseVariableValue} from './variables.utils';
2+
import {VariableConfig} from './variables';
3+
import {
4+
parseVariableValue,
5+
isMultiValueConfig,
6+
getItemSchemaIfArray,
7+
} from './variables.utils';
38

49
type TProperties = Record<string, TSchema>;
510

@@ -89,27 +94,40 @@ export function updateSchemaAtPath(
8994
if (objSchema.properties && first in objSchema.properties) {
9095
const propSchema = objSchema.properties[first];
9196

92-
// Check if this property is an array schema and we need to navigate deeper
93-
if (
94-
'type' in propSchema &&
95-
propSchema.type === 'array' &&
96-
rest.length > 0
97-
) {
97+
// Check if this property is an array schema
98+
if ('type' in propSchema && propSchema.type === 'array') {
9899
const arrayPropSchema = propSchema as TArray;
99-
// Navigate deeper into the items schema
100100
const itemsSchema = arrayPropSchema.items || Type.String();
101-
const updatedItems = updateSchemaAtPath(
102-
itemsSchema,
103-
rest.join('.'),
104-
newValue,
105-
);
106-
return Type.Object({
107-
...objSchema.properties,
108-
[first]: Type.Array(updatedItems),
109-
}) as TSchema;
101+
102+
if (rest.length > 0) {
103+
// Navigate deeper into the items schema
104+
const updatedItems = updateSchemaAtPath(
105+
itemsSchema,
106+
rest.join('.'),
107+
newValue,
108+
);
109+
return Type.Object({
110+
...objSchema.properties,
111+
[first]: Type.Array(updatedItems),
112+
}) as TSchema;
113+
}
114+
115+
// At the array property itself (rest.length === 0)
116+
// If newValue is not an array, assume we're updating the items schema
117+
// If newValue IS an array, we're replacing the entire array schema
118+
const isReplacingWithArray =
119+
'type' in newValue && newValue.type === 'array';
120+
if (!isReplacingWithArray) {
121+
// Update items schema, preserve array wrapper
122+
return Type.Object({
123+
...objSchema.properties,
124+
[first]: Type.Array(newValue),
125+
}) as TSchema;
126+
}
127+
// Fall through to replace the property entirely with the new array
110128
}
111129

112-
// Regular property update (non-array)
130+
// Regular property update
113131
const updatedProp =
114132
rest.length > 0
115133
? updateSchemaAtPath(propSchema, rest.join('.'), newValue)
@@ -186,7 +204,7 @@ export function setValueAtPath(
186204
return newValue;
187205
}
188206

189-
// If obj is not an object, we can't navigate further
207+
// If obj is not an object/array, we can't navigate further
190208
if (!obj || typeof obj !== 'object') return obj;
191209

192210
const o = obj as Record<string, unknown>;
@@ -308,3 +326,70 @@ export function renamePropertyInObject(
308326

309327
return renameAtLevel(obj, schema, parentPath);
310328
}
329+
330+
// ===== Multi-Value Config Helpers =====
331+
// These functions handle the implicit array wrapper for multi-value configs
332+
// (RandomPermutation, BalancedAssignment). The schema is Array(ItemType) but
333+
// users work with individual items directly in the editor.
334+
335+
/**
336+
* Updates the schema at a path, handling multi-value config implicit array wrapper.
337+
* For multi-value configs, unwraps the root array, updates the item schema, then rewraps.
338+
*/
339+
export function updateSchemaForConfig(
340+
config: VariableConfig,
341+
path: string,
342+
newSchema: TSchema,
343+
): TSchema {
344+
const fullSchema = config.definition.schema as unknown as TSchema;
345+
346+
if (isMultiValueConfig(config) && 'items' in fullSchema) {
347+
// Multi-value configs: unwrap array → update item schema → rewrap
348+
const itemSchema = getItemSchemaIfArray(fullSchema);
349+
const updatedItemSchema = updateSchemaAtPath(itemSchema, path, newSchema);
350+
return Type.Array(updatedItemSchema) as TSchema;
351+
}
352+
return updateSchemaAtPath(fullSchema, path, newSchema);
353+
}
354+
355+
/**
356+
* Sets a value at a path, handling multi-value config implicit array wrapper.
357+
* For multi-value configs, the value is a single item (not an array), so we
358+
* operate on the item schema directly.
359+
*/
360+
export function setValueForConfig(
361+
value: unknown,
362+
config: VariableConfig,
363+
path: string,
364+
newValue: unknown,
365+
): unknown {
366+
const fullSchema = config.definition.schema as unknown as TSchema;
367+
368+
if (isMultiValueConfig(config) && 'items' in fullSchema) {
369+
// Multi-value configs: operate on item schema directly
370+
const itemSchema = getItemSchemaIfArray(fullSchema);
371+
return setValueAtPath(value, itemSchema, path, newValue);
372+
}
373+
return setValueAtPath(value, fullSchema, path, newValue);
374+
}
375+
376+
/**
377+
* Renames a property in a value, handling multi-value config implicit array wrapper.
378+
* For multi-value configs, the value is a single item (not an array), so we
379+
* operate on the item schema directly.
380+
*/
381+
export function renamePropertyForConfig(
382+
value: unknown,
383+
config: VariableConfig,
384+
path: string,
385+
newName: string,
386+
): unknown {
387+
const fullSchema = config.definition.schema as unknown as TSchema;
388+
389+
if (isMultiValueConfig(config) && 'items' in fullSchema) {
390+
// Multi-value configs: operate on item schema directly
391+
const itemSchema = getItemSchemaIfArray(fullSchema);
392+
return renamePropertyInObject(value, itemSchema, path, newName);
393+
}
394+
return renamePropertyInObject(value, fullSchema, path, newName);
395+
}

0 commit comments

Comments
 (0)