bbovenzi commented on code in PR #74885:
URL: https://github.com/apache/airflow/pull/74885#discussion_r4484909530
##########
airflow-core/src/airflow/ui/src/components/FlexibleForm/FieldDropdown.tsx:
##########
@@ -58,6 +79,18 @@ export const FieldDropdown = ({ name, namespace = "default",
onUpdate }: Flexibl
? (options.find((opt) => opt.value === String(param.value)) ?? null)
: null;
+ // A parent change can leave the value outside the new options, so clear it
+ const isStale = dependsOn !== undefined && param.value !== null &&
currentValue === null;
+
+ useEffect(() => {
Review Comment:
We should avoid unnecessary `useEffect`s. Let's do this as part of the param
write instead of a separate loop.
```
getOptions(param, params, defaults):
if param has no parent: return param.enum
parentValue = params[parent].value ?? defaults[parent].value // conf
may leave out the parent
return param.options_map[str(parentValue)] ?? []
clearStale(params, defaults):
repeat until nothing changes: // handles chains A → B → C
for each param with a parent and a non-null value:
if value not in getOptions(param, params, defaults):
param.value = null
return params
store.setParamsDict(newParams):
params = clearStale(newParams, initialParams) // one write, every
child cleared together
save params and conf
FieldDropdown:
options = getOptions(param, params, initialParams)
// no isStale, no useEffect
```
##########
airflow-core/src/airflow/ui/src/components/FlexibleForm/FieldDropdown.tsx:
##########
@@ -58,6 +79,18 @@ export const FieldDropdown = ({ name, namespace = "default",
onUpdate }: Flexibl
? (options.find((opt) => opt.value === String(param.value)) ?? null)
: null;
+ // A parent change can leave the value outside the new options, so clear it
+ const isStale = dependsOn !== undefined && param.value !== null &&
currentValue === null;
+
+ useEffect(() => {
Review Comment:
This useEffect will call on every change because paramsDict is always
different
##########
airflow-core/src/airflow/ui/src/components/FlexibleForm/FieldDropdown.tsx:
##########
@@ -40,13 +42,32 @@ const labelLookup = (
const enumTypes = ["string", "number", "integer"];
+// Python str() of a string, integer, boolean or null parent value, because
the serialized Dag
+// stores the keys of options_map with str()
+const getOptionKey = (value: unknown): string => {
+ if (value === null || value === undefined) {
+ return "None";
+ }
+ if (typeof value === "boolean") {
+ return value ? "True" : "False";
+ }
+
+ return typeof value === "number" || typeof value === "string" ?
String(value) : JSON.stringify(value);
+};
+
export const FieldDropdown = ({ name, namespace = "default", onUpdate }:
FlexibleFormElementProps) => {
const { t: translate } = useTranslation("components");
const { disabled, paramsDict, setParamsDict } = useParamStore(namespace);
const param = paramsDict[name] ?? paramPlaceholder;
+ const { depends_on: dependsOn, options_map: optionsMap } = param.schema;
+ const enumValues =
+ dependsOn === undefined
+ ? param.schema.enum
+ : (optionsMap?.[getOptionKey(paramsDict[dependsOn]?.value)] ?? []);
Review Comment:
This can force an empty dropdown on a user. We should have a fallback empty
message or an error message like: "Select {{parent_input}} first"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
