From fa3300134a61fa7cb986a120698ea264980cd41c Mon Sep 17 00:00:00 2001 From: guerler Date: Fri, 6 Feb 2026 16:39:59 +0300 Subject: [PATCH] Add comments and limitations for follow-ups --- client/src/components/Form/FormDisplay.vue | 15 +++- .../Form/composables/useFormState.js | 47 ++++++++-- .../Form/composables/useFormState.test.js | 85 +++++++++++++------ 3 files changed, 113 insertions(+), 34 deletions(-) diff --git a/client/src/components/Form/FormDisplay.vue b/client/src/components/Form/FormDisplay.vue index 31768b3b28f..124c92d8ed3 100644 --- a/client/src/components/Form/FormDisplay.vue +++ b/client/src/components/Form/FormDisplay.vue @@ -131,14 +131,17 @@ const { allowEmptyValueOnRequiredInput: toRef(props, "allowEmptyValueOnRequiredInput"), }); +// getCurrentInstance is fragile but required in Vue 2.7 Composition API +// to access $el for scrollToElement. Acceptable given the constraint. const instance = getCurrentInstance(); function onChange(refreshOnChange) { rebuildIndex(); const params = buildFormData(); if (JSON.stringify(params) != JSON.stringify(formData.value)) { + formData.value = params; resetErrors(); - emit("onChange", formData.value, refreshOnChange); + emit("onChange", params, refreshOnChange); } } @@ -175,13 +178,19 @@ function updateActiveNode(activeNodeId) { } // --- Initialization (replaces created() hook) --- +// Bootstrap: runs during setup before watchers are active. +// Emits initial formData so the parent has submittable data immediately. cloneInputs(props.inputs); -buildFormData(); +formData.value = buildFormData(); emit("onChange", formData.value); applyWarnings(props.warnings); applyErrors(props.errors); // --- Watchers --- +// All watchers use flush: "sync" to match Options API synchronous watcher timing. +// This ensures attribute sync, error application, and formData emission happen +// in the same tick as the prop change. Performance note: sync flush bypasses +// Vue's batching; acceptable here since these watchers do lightweight work. watch( () => props.activeNodeId, () => { @@ -193,7 +202,7 @@ watch( () => props.id, () => { cloneInputs(props.inputs); - buildFormData(); + formData.value = buildFormData(); emit("onChange", formData.value); applyWarnings(props.warnings); applyErrors(props.errors); diff --git a/client/src/components/Form/composables/useFormState.js b/client/src/components/Form/composables/useFormState.js index 62b1e8159a1..7540bb0cb5a 100644 --- a/client/src/components/Form/composables/useFormState.js +++ b/client/src/components/Form/composables/useFormState.js @@ -2,6 +2,9 @@ import { computed, ref, set } from "vue"; import { matchInputs, validateInputs, visitAllInputs, visitInputs } from "../utilities"; +// Fields owned by the client (clone), never copied from server response into attributes. +const CLIENT_OWNED_FIELDS = new Set(["value", "error", "warning"]); + /** * Composable for form state management. * Handles cloning, indexing, attribute sync, errors, warnings, and validation. @@ -20,8 +23,11 @@ export function useFormState(options = {}) { function cloneInputs(inputs) { formInputs.value = JSON.parse(JSON.stringify(inputs)); + // set() required here: error and warning are genuinely new properties + // on freshly cloned plain objects that Vue 2.7 hasn't observed yet. visitAllInputs(formInputs.value, (input) => { set(input, "error", null); + set(input, "warning", null); }); rebuildIndex(); } @@ -34,24 +40,41 @@ export function useFormState(options = {}) { formIndex.value = index; } + /** + * Pure computation — builds a flat {paramName: value} dict from the active + * params in formIndex. Does NOT mutate formData; the caller is responsible + * for assigning formData.value when appropriate. + */ function buildFormData() { const params = {}; Object.entries(formIndex.value).forEach(([key, input]) => { params[key] = input.value; }); - formData.value = params; return params; } + /** + * Patches input.attributes on clone nodes from the server response. + * Only copies server-owned fields; value, error, and warning are excluded + * to maintain the separation between server state (attributes) and client + * state (the clone's own properties). + */ function syncServerAttributes(newInputs) { const newAttributes = {}; visitAllInputs(newInputs, (input, name) => { newAttributes[name] = input; }); visitAllInputs(formInputs.value, (input, name) => { - const newValue = newAttributes[name]; - if (newValue != undefined) { - set(input, "attributes", newValue); + const serverNode = newAttributes[name]; + if (serverNode != undefined) { + const attrs = {}; + for (const key in serverNode) { + if (!CLIENT_OWNED_FIELDS.has(key)) { + attrs[key] = serverNode[key]; + } + } + // set() required: attributes is a genuinely new property on clone nodes. + set(input, "attributes", attrs); } }); } @@ -78,23 +101,33 @@ export function useFormState(options = {}) { function setError(inputId, message) { const input = formIndex.value[inputId]; if (input) { - set(input, "error", message); + // Plain assignment: error property exists from cloneInputs initialization. + input.error = message; } } function setWarning(inputId, message) { const input = formIndex.value[inputId]; if (input) { - set(input, "warning", message); + // Plain assignment: warning property exists from cloneInputs initialization. + input.warning = message; } } function resetErrors() { Object.values(formIndex.value).forEach((input) => { - set(input, "error", null); + // Plain assignment: error property exists from cloneInputs initialization. + input.error = null; }); } + /** + * Known debt: replaceParams and client-side default selection (FormSelect, + * FormData auto-selecting first option) are indistinguishable from user edits. + * Both flow through v-model → onChange → formData. The system has no concept + * of "inferred default" vs "user intent". This is existing behavior inherited + * from the Options API implementation, not introduced by this refactor. + */ function replaceParams(params) { let refreshOnChange = false; Object.entries(params).forEach(([key, value]) => { diff --git a/client/src/components/Form/composables/useFormState.test.js b/client/src/components/Form/composables/useFormState.test.js index a821abf65e5..efe1e425fe6 100644 --- a/client/src/components/Form/composables/useFormState.test.js +++ b/client/src/components/Form/composables/useFormState.test.js @@ -52,7 +52,7 @@ describe("useFormState", () => { }).not.toThrow(); }); - it("I2: syncServerAttributes only sets input.attributes", () => { + it("I2: syncServerAttributes only copies server-owned fields", () => { const { cloneInputs, formInputs, syncServerAttributes } = useFormState(); cloneInputs(makeConditionalInputs()); @@ -60,21 +60,29 @@ describe("useFormState", () => { const textInput = formInputs.value[0]; const valueBefore = textInput.value; const errorBefore = textInput.error; + const warningBefore = textInput.warning; - // Create server response with different value + // Create server response with different value, options, and error const serverInputs = makeConditionalInputs(); serverInputs[0].value = "server_value"; serverInputs[0].options = [["opt1", "opt1"]]; + serverInputs[0].error = "server_error"; + serverInputs[0].warning = "server_warning"; syncServerAttributes(serverInputs); - // Only attributes changed + // Server-owned fields are in attributes expect(textInput.attributes).toBeDefined(); - expect(textInput.attributes.value).toEqual("server_value"); - // Original value preserved + expect(textInput.attributes.options).toEqual([["opt1", "opt1"]]); + expect(textInput.attributes.name).toEqual("text_field"); + // Client-owned fields excluded from attributes + expect(textInput.attributes.value).toBeUndefined(); + expect(textInput.attributes.error).toBeUndefined(); + expect(textInput.attributes.warning).toBeUndefined(); + // Clone's own properties preserved expect(textInput.value).toEqual(valueBefore); - // Error unchanged expect(textInput.error).toEqual(errorBefore); + expect(textInput.warning).toEqual(warningBefore); }); it("I4: formIndex contains active params only", () => { @@ -90,9 +98,8 @@ describe("useFormState", () => { }); it("I5: errors apply only to active params", () => { - const { cloneInputs, buildFormData, applyErrors, formIndex } = useFormState(); + const { cloneInputs, applyErrors, formIndex } = useFormState(); cloneInputs(makeConditionalInputs()); - buildFormData(); applyErrors({ text_field: "error on text", @@ -106,19 +113,25 @@ describe("useFormState", () => { expect(formIndex.value["cond|in_b"]).toBeUndefined(); }); - it("I3: formData is updated exclusively by buildFormData", () => { + it("I3: buildFormData is a pure computation over formIndex", () => { const { cloneInputs, formData, buildFormData } = useFormState(); cloneInputs(makeConditionalInputs()); - // Before buildFormData, formData is empty + // formData starts empty; buildFormData does not mutate it expect(formData.value).toEqual({}); - buildFormData(); - expect(formData.value).toEqual({ + const result = buildFormData(); + // buildFormData returns the params but does not assign formData.value + expect(formData.value).toEqual({}); + expect(result).toEqual({ text_field: "hello", "cond|sel": "a", "cond|in_a": "va", }); + + // Caller assigns formData.value explicitly + formData.value = result; + expect(formData.value).toEqual(result); }); it("syncServerAttributes patches all conditional cases", () => { @@ -143,7 +156,7 @@ describe("useFormState", () => { it("conditional switch updates formIndex and formData", () => { const { cloneInputs, formInputs, rebuildIndex, buildFormData, formIndex, formData } = useFormState(); cloneInputs(makeConditionalInputs()); - buildFormData(); + formData.value = buildFormData(); expect(formIndex.value["cond|in_a"]).toBeDefined(); expect(formIndex.value["cond|in_b"]).toBeUndefined(); @@ -152,7 +165,7 @@ describe("useFormState", () => { // Switch conditional formInputs.value[1].test_param.value = "b"; rebuildIndex(); - buildFormData(); + formData.value = buildFormData(); expect(formIndex.value["cond|in_a"]).toBeUndefined(); expect(formIndex.value["cond|in_b"]).toBeDefined(); @@ -172,10 +185,10 @@ describe("useFormState", () => { }); it("validation computed reacts to formData changes", () => { - const { cloneInputs, buildFormData, formInputs, rebuildIndex, validation } = useFormState(); + const { cloneInputs, buildFormData, formInputs, rebuildIndex, formData, validation } = useFormState(); const inputs = [{ name: "required_field", type: "text", value: "has_value" }]; cloneInputs(inputs); - buildFormData(); + formData.value = buildFormData(); // Valid — has value expect(validation.value).toBeNull(); @@ -183,7 +196,7 @@ describe("useFormState", () => { // Set value to null (required field) formInputs.value[0].value = null; rebuildIndex(); - buildFormData(); + formData.value = buildFormData(); expect(validation.value).toEqual(["required_field", "Please provide a value for this option."]); }); @@ -191,12 +204,12 @@ describe("useFormState", () => { it("allowEmptyValueOnRequiredInput option is respected", () => { // allowEmptyValueOnRequiredInput=true means empty string IS treated as invalid const allow = ref(true); - const { cloneInputs, buildFormData, formInputs, rebuildIndex, validation } = useFormState({ + const { cloneInputs, buildFormData, formInputs, rebuildIndex, formData, validation } = useFormState({ allowEmptyValueOnRequiredInput: allow, }); const inputs = [{ name: "field", type: "text", value: "" }]; cloneInputs(inputs); - buildFormData(); + formData.value = buildFormData(); // With allow=true, empty string on required field triggers error expect(validation.value).toEqual(["field", "Please provide a value for this option."]); @@ -205,25 +218,49 @@ describe("useFormState", () => { allow.value = false; formInputs.value[0].value = ""; rebuildIndex(); - buildFormData(); + formData.value = buildFormData(); expect(validation.value).toBeNull(); // Null always fails regardless of the flag formInputs.value[0].value = null; rebuildIndex(); - buildFormData(); + formData.value = buildFormData(); expect(validation.value).toEqual(["field", "Please provide a value for this option."]); }); - it("R6: cloneInputs sets error=null on all cases including inactive", () => { + it("R6: cloneInputs sets error and warning to null on all cases including inactive", () => { const { cloneInputs, formInputs } = useFormState(); const inputs = makeConditionalInputs(); - // Pre-set errors on inputs + // Pre-set errors and warnings on inputs inputs[1].cases[1].inputs[0].error = "stale error"; + inputs[1].cases[1].inputs[0].warning = "stale warning"; cloneInputs(inputs); - // Error should be cleared even on inactive case + // Both should be cleared even on inactive case const caseBInput = formInputs.value[1].cases[1].inputs[0]; expect(caseBInput.error).toBeNull(); + expect(caseBInput.warning).toBeNull(); + }); + + /** + * Known debt: replaceParams and client-side default selection (FormSelect, + * FormData auto-selecting first option when value===null) flow through the + * same v-model → onChange path as user edits. The system does not distinguish + * "inferred default" from "user intent". This is existing behavior inherited + * from the Options API implementation. + */ + it("known debt: replaceParams is indistinguishable from user edits", () => { + const { cloneInputs, formIndex, replaceParams, buildFormData, formData } = useFormState(); + cloneInputs(makeConditionalInputs()); + formData.value = buildFormData(); + + const before = { ...formData.value }; + replaceParams({ text_field: "programmatic_value" }); + formData.value = buildFormData(); + + // The replaced value appears in formData identically to a user edit. + // There is no metadata distinguishing the source of the change. + expect(formData.value["text_field"]).toEqual("programmatic_value"); + expect(before["text_field"]).toEqual("hello"); }); });