bbovenzi commented on code in PR #45790:
URL: https://github.com/apache/airflow/pull/45790#discussion_r1927856564
##########
airflow/ui/src/components/TriggerDag/TriggerDAGForm.tsx:
##########
@@ -139,7 +129,7 @@ const TriggerDAGForm = ({ dagId, onClose, open }:
TriggerDAGFormProps) => {
size="lg"
variant="enclosed"
>
- <FlexibleForm params={paramsDict} />
+ <FlexibleForm initialParamsDict={initialParamsDict} />
Review Comment:
Since we have this now. I think we should remove the ConfigurationJson under
Advanced
##########
airflow/ui/src/components/FlexibleForm/FieldMultiSelect.tsx:
##########
@@ -29,7 +30,9 @@ const labelLookup = (key: string, valuesDisplay:
Record<string, string> | undefi
return key;
};
-export const FieldMultiSelect = ({ name, param }: FlexibleFormElementProps) =>
{
+export const FieldMultiSelect = ({ name }: FlexibleFormElementProps) => {
+ const { paramsDict, setParamsDict } = useParamStore();
+ const param = paramsDict[name] ?? paramPlaceholder;
Review Comment:
Yeah right now removing all the options will return an empty array which the
backend thinks is a valid value even if the field is required.
##########
airflow/ui/src/components/FlexibleForm/FieldMultiSelect.tsx:
##########
@@ -39,6 +42,13 @@ export const FieldMultiSelect = ({ name, param }:
FlexibleFormElementProps) => {
: [],
);
+ useEffect(() => {
+ if (paramsDict[name]) {
+ paramsDict[name].value = selectedOptions.map((option) => option.value);
+ }
+ setParamsDict(paramsDict);
Review Comment:
What are we trying to do here? Again, We should avoid useEffect or include a
comment on why its needed.
##########
airflow/ui/src/components/FlexibleForm/FlexibleForm.tsx:
##########
@@ -16,15 +16,39 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Stack, StackSeparator } from "@chakra-ui/react";
+import { Box, Stack, StackSeparator } from "@chakra-ui/react";
+import { useEffect } from "react";
-import { flexibleFormDefaultSection, type FlexibleFormProps } from ".";
-import { Accordion, Alert } from "../ui";
+import type { DagParamsSpec } from "src/queries/useDagParams";
+
+import { flexibleFormDefaultSection } from ".";
+import { useParamStore } from "../TriggerDag/useParamStore";
+import { Accordion } from "../ui";
import { Row } from "./Row";
-export const FlexibleForm = ({ params }: FlexibleFormProps) => {
+export type FlexibleFormProps = {
+ initialParamsDict: { paramsDict: DagParamsSpec };
+};
+
+export const FlexibleForm = ({ initialParamsDict }: FlexibleFormProps) => {
+ const { paramsDict: params, setParamsDict } = useParamStore();
const processedSections = new Map();
+ useEffect(() => {
+ if (Object.keys(initialParamsDict.paramsDict).length > 0 &&
Object.keys(params).length === 0) {
+ const paramsCopy = structuredClone(initialParamsDict.paramsDict);
+
+ setParamsDict(paramsCopy);
+ }
+ }, [initialParamsDict, params, setParamsDict]);
+
+ useEffect(
+ () => () => {
+ setParamsDict({});
+ },
+ [setParamsDict],
+ );
Review Comment:
What are you trying to do here? We should either avoid `useEffect` or
provide a comment on why its needed
##########
airflow/ui/src/components/FlexibleForm/FieldAdvancedArray.tsx:
##########
@@ -16,37 +16,66 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { Text } from "@chakra-ui/react";
import { json } from "@codemirror/lang-json";
import { githubLight, githubDark } from "@uiw/codemirror-themes-all";
import CodeMirror from "@uiw/react-codemirror";
+import { useState } from "react";
import { useColorMode } from "src/context/colorMode";
import type { FlexibleFormElementProps } from ".";
+import { paramPlaceholder, useParamStore } from "../TriggerDag/useParamStore";
-export const FieldAdvancedArray = ({ name, param }: FlexibleFormElementProps)
=> {
+export const FieldAdvancedArray = ({ name }: FlexibleFormElementProps) => {
const { colorMode } = useColorMode();
+ const { paramsDict, setParamsDict } = useParamStore();
+ const param = paramsDict[name] ?? paramPlaceholder;
+ const [error, setError] = useState<unknown>(undefined);
+
+ const handleChange = (value: string) => {
+ setError(undefined);
+ try {
+ const parsedValue = JSON.parse(value) as JSON;
+
+ if (!Array.isArray(parsedValue)) {
+ throw new TypeError("Value must be an array");
+ }
+
+ if (paramsDict[name]) {
+ paramsDict[name].value = parsedValue;
+ }
+
+ setParamsDict(paramsDict);
+ } catch (_error) {
+ setError(_error);
+ }
+ };
return (
- <CodeMirror
- basicSetup={{
- autocompletion: true,
- bracketMatching: true,
- foldGutter: true,
- lineNumbers: true,
- }}
- extensions={[json()]}
- height="200px"
- id={`element_${name}`}
- style={{
- border: "1px solid var(--chakra-colors-border)",
- borderRadius: "8px",
- outline: "none",
- padding: "2px",
- width: "100%",
- }}
- theme={colorMode === "dark" ? githubDark : githubLight}
- value={JSON.stringify(param.value ?? [], undefined, 2)}
- />
+ <>
+ <CodeMirror
+ basicSetup={{
+ autocompletion: true,
+ bracketMatching: true,
+ foldGutter: true,
+ lineNumbers: true,
+ }}
+ extensions={[json()]}
+ height="200px"
+ id={`element_${name}`}
+ onChange={handleChange}
+ style={{
+ border: "1px solid var(--chakra-colors-border)",
+ borderRadius: "8px",
+ outline: "none",
+ padding: "2px",
+ width: "100%",
+ }}
+ theme={colorMode === "dark" ? githubDark : githubLight}
+ value={JSON.stringify(param.value ?? [], undefined, 2)}
+ />
+ {Boolean(error) ? <Text color="red">{String(error)}</Text> : undefined}
Review Comment:
```suggestion
{Boolean(error) ? <Text color="text.error"
size="xs">{String(error)}</Text> : undefined}
```
Semantic colors and let's the text match the helper labels we're already
using
--
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]
