opensource/galaxy
3
0
Fork 0
mirror of https://github.com/galaxyproject/galaxy.git synced 2026-10-31 02:28:09 +08:00
Code Issues Packages Projects Releases Wiki Activity

Add comments and limitations for follow-ups

Browse Source
This commit is contained in:
guerler
2026-02-12 11:49:56 +03:00
parent de64755680
commit fa3300134a
3 changed files with 113 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/src/components/Form/FormDisplay.vue
+12 -3
View File
@@ -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);
client/src/components/Form/composables/useFormState.js
+40 -7
View File
@@ -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]) => {
client/src/components/Form/composables/useFormState.test.js
+61 -24
View File
@@ -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");
});
});