Copilot commented on code in PR #62882:
URL: https://github.com/apache/airflow/pull/62882#discussion_r2884991773
##########
airflow-core/src/airflow/ui/src/pages/Variables/ManageVariable/VariableForm.tsx:
##########
@@ -16,14 +16,95 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Box, Button, Field, HStack, Input, Spacer, Text, Textarea } from
"@chakra-ui/react";
-import { Controller, useForm } from "react-hook-form";
+import { Box, Button, Field, HStack, IconButton, Input, Spacer, Text, Textarea
} from "@chakra-ui/react";
+import { useEffect, useRef, useState } from "react";
+import { type ControllerFieldState, type ControllerRenderProps, Controller,
useForm } from "react-hook-form";
import { useTranslation } from "react-i18next";
-import { FiSave } from "react-icons/fi";
+import { FiCode, FiSave } from "react-icons/fi";
import { ErrorAlert } from "src/components/ErrorAlert";
import { TeamSelector } from "src/components/TeamSelector.tsx";
import { useConfig } from "src/queries/useConfig.tsx";
+import { isJsonString, minifyJson, prettifyJson } from "src/utils";
+
+type ValueFieldProps = {
+ readonly field: ControllerRenderProps<VariableBody, "value">;
+ readonly fieldState: ControllerFieldState;
+};
+
+const ValueField = ({ field, fieldState }: ValueFieldProps) => {
+ const { t: translate } = useTranslation(["admin"]);
+ const [displayValue, setDisplayValue] = useState(field.value);
+ const fieldValueRef = useRef(field.value);
+
+ useEffect(() => {
+ if (fieldValueRef.current !== field.value) {
+ fieldValueRef.current = field.value;
+ setDisplayValue(field.value);
+ }
+ }, [field.value]);
+
+ const showJsonWarning =
+ displayValue.startsWith("{") || displayValue.startsWith("[") ?
!isJsonString(displayValue) : false;
+
+ const handleChange = (event: React.ChangeEvent<HTMLTextAreaElement>) => {
+ const newValue = event.target.value;
+
+ setDisplayValue(newValue);
+ field.onChange(newValue);
+ };
+
+ const handleBlur = () => {
+ field.onBlur();
+ setDisplayValue(prettifyJson(displayValue));
Review Comment:
`handleBlur` prettifies `displayValue` but doesn't update the
react-hook-form field value. This can desync what the user sees from what the
form submits and from `formState.isDirty` (e.g., blur can change the visible
text while the form still considers it unchanged). Consider also calling
`field.onChange` with the prettified value (only when it differs) so the form
state stays consistent with the textarea contents.
```suggestion
const formatted = prettifyJson(displayValue);
if (formatted !== displayValue) {
setDisplayValue(formatted);
field.onChange(formatted);
}
```
##########
airflow-core/src/airflow/ui/src/pages/Variables/ManageVariable/VariableForm.tsx:
##########
@@ -16,14 +16,95 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Box, Button, Field, HStack, Input, Spacer, Text, Textarea } from
"@chakra-ui/react";
-import { Controller, useForm } from "react-hook-form";
+import { Box, Button, Field, HStack, IconButton, Input, Spacer, Text, Textarea
} from "@chakra-ui/react";
+import { useEffect, useRef, useState } from "react";
+import { type ControllerFieldState, type ControllerRenderProps, Controller,
useForm } from "react-hook-form";
import { useTranslation } from "react-i18next";
-import { FiSave } from "react-icons/fi";
+import { FiCode, FiSave } from "react-icons/fi";
import { ErrorAlert } from "src/components/ErrorAlert";
import { TeamSelector } from "src/components/TeamSelector.tsx";
import { useConfig } from "src/queries/useConfig.tsx";
+import { isJsonString, minifyJson, prettifyJson } from "src/utils";
+
+type ValueFieldProps = {
+ readonly field: ControllerRenderProps<VariableBody, "value">;
+ readonly fieldState: ControllerFieldState;
+};
+
+const ValueField = ({ field, fieldState }: ValueFieldProps) => {
+ const { t: translate } = useTranslation(["admin"]);
+ const [displayValue, setDisplayValue] = useState(field.value);
+ const fieldValueRef = useRef(field.value);
+
+ useEffect(() => {
+ if (fieldValueRef.current !== field.value) {
+ fieldValueRef.current = field.value;
+ setDisplayValue(field.value);
+ }
+ }, [field.value]);
+
+ const showJsonWarning =
+ displayValue.startsWith("{") || displayValue.startsWith("[") ?
!isJsonString(displayValue) : false;
+
Review Comment:
`isJsonString(displayValue)` triggers `JSON.parse` and is currently called
multiple times per render (for `showJsonWarning` and again for whether to show
the format button). For large values this can become noticeably expensive while
typing. Consider computing the parsed/validity result once (e.g., via
`useMemo`) and reusing it in both places (and/or parsing only on blur / on
explicit format click).
##########
airflow-core/src/airflow/ui/src/utils/json.ts:
##########
@@ -0,0 +1,72 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Checks if a string is valid JSON
+ * @param string - The string to validate
+ * @returns true if the string is valid JSON, false otherwise
+ */
+export const isJsonString = (string: string): boolean => {
+ try {
+ JSON.parse(string);
Review Comment:
In `isJsonString`, the parameter is named `string`, which is easy to confuse
with the built-in `string` type and is inconsistent with the `jsonString`
naming used in the other helpers. Renaming the parameter to something like
`jsonString`/`value` would improve readability and consistency.
```suggestion
* @param jsonString - The string to validate
* @returns true if the string is valid JSON, false otherwise
*/
export const isJsonString = (jsonString: string): boolean => {
try {
JSON.parse(jsonString);
```
##########
airflow-core/src/airflow/ui/src/utils/json.ts:
##########
@@ -0,0 +1,72 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Checks if a string is valid JSON
+ * @param string - The string to validate
+ * @returns true if the string is valid JSON, false otherwise
+ */
+export const isJsonString = (string: string): boolean => {
+ try {
+ JSON.parse(string);
+ } catch {
+ return false;
+ }
+
+ return true;
+};
+
+/**
+ * Prettifies a JSON string with indentation
+ * @param jsonString - The JSON string to prettify
+ * @param indent - Number of spaces for indentation (default: 2)
+ * @returns Prettified JSON string, or the original string if parsing fails
+ */
+export const prettifyJson = (jsonString: string, indent: number = 2): string
=> {
+ if (!isJsonString(jsonString)) {
+ return jsonString;
+ }
+
+ try {
+ const parsed: unknown = JSON.parse(jsonString);
+
+ return JSON.stringify(parsed, undefined, indent);
Review Comment:
`prettifyJson` calls `isJsonString(jsonString)` (which parses) and then
immediately parses again inside the `try`. This double parsing adds unnecessary
overhead, especially if these utilities are used on large values. Consider
parsing once and reusing the result (or refactor to a shared `tryParseJson`
helper).
##########
airflow-core/src/airflow/ui/src/utils/json.ts:
##########
@@ -0,0 +1,72 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Checks if a string is valid JSON
+ * @param string - The string to validate
+ * @returns true if the string is valid JSON, false otherwise
+ */
+export const isJsonString = (string: string): boolean => {
+ try {
+ JSON.parse(string);
+ } catch {
+ return false;
+ }
+
+ return true;
+};
+
+/**
+ * Prettifies a JSON string with indentation
+ * @param jsonString - The JSON string to prettify
+ * @param indent - Number of spaces for indentation (default: 2)
+ * @returns Prettified JSON string, or the original string if parsing fails
+ */
+export const prettifyJson = (jsonString: string, indent: number = 2): string
=> {
+ if (!isJsonString(jsonString)) {
+ return jsonString;
+ }
+
+ try {
+ const parsed: unknown = JSON.parse(jsonString);
+
+ return JSON.stringify(parsed, undefined, indent);
+ } catch {
+ return jsonString;
+ }
+};
+
+/**
+ * Minifies a JSON string by removing whitespace
+ * @param jsonString - The JSON string to minify
+ * @returns Minified JSON string, or the original string if parsing fails
+ */
+export const minifyJson = (jsonString: string): string => {
+ if (!isJsonString(jsonString)) {
+ return jsonString;
+ }
+
+ try {
+ const parsed: unknown = JSON.parse(jsonString);
+
+ return JSON.stringify(parsed);
Review Comment:
`minifyJson` also parses twice (via the `isJsonString` pre-check and then
again in the `try`). Consider removing the pre-check and doing a single
parse+stringify, returning the original string on failure, to avoid redundant
work.
--
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]
