Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi merged PR #44021: URL: https://github.com/apache/airflow/pull/44021 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
shubhamraj-git commented on PR #44021: URL: https://github.com/apache/airflow/pull/44021#issuecomment-2480456235 @bbovenzi Also, since this is an error showing page, I saw the old UI have names too in red, So what do you think, which of the two options look good? Red or normal black? https://github.com/user-attachments/assets/84383519-7933-4e8e-aa3a-b2e459bb53a7";> -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
shubhamraj-git commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1844923084
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrorsModal.tsx:
##
@@ -0,0 +1,125 @@
+/*!
+ * 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.
+ */
+import { Heading, Text, HStack, Input } from "@chakra-ui/react";
+import { useEffect, useState } from "react";
+import { PiFilePy } from "react-icons/pi";
+
+import type { ImportErrorResponse } from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+import { Accordion, Dialog } from "src/components/ui";
+import { Pagination } from "src/components/ui/Pagination";
+
+type ImportDAGErrorModalProps = {
+ importErrors: Array;
+ onClose: () => void;
+ open: boolean;
+};
+
+const PAGE_LIMIT = 5;
+
+export const DAGImportErrorsModal: React.FC = ({
+ importErrors,
+ onClose,
+ open,
+}) => {
+ const [page, setPage] = useState(1);
+ const [searchQuery, setSearchQuery] = useState("");
+ const [filteredErrors, setFilteredErrors] = useState(importErrors);
+
+ const startRange = (page - 1) * PAGE_LIMIT;
+ const endRange = startRange + PAGE_LIMIT;
+ const visibleItems = filteredErrors.slice(startRange, endRange);
+
+ const onOpenChange = () => {
+setSearchQuery("");
+setPage(1);
+onClose();
+ };
+
+ useEffect(() => {
+const query = searchQuery.toLowerCase();
+const filtered = importErrors.filter((error) =>
+ error.filename.toLowerCase().includes(query),
+);
+
+setFilteredErrors(filtered);
+setPage(1);
+ }, [searchQuery, importErrors]);
+
+ return (
+
+
+
+ Dag Import Errors
+ setSearchQuery(letters.target.value)}
+placeholder="Search by file"
+value={searchQuery}
+ />
+
+
+
+
+
+
+{visibleItems.map((importError) => (
+
+
+
+ {importError.filename}
+
+
+
+Timestamp:
+
+
+{importError.stack_trace}
+
Review Comment:
https://github.com/user-attachments/assets/0064184d-54bb-4eb8-9ac0-149b0a02cab3";>
In that case, we need to handle the bg color of code block or change variant
of Accordion.root to be subtle, which removes the border though.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
shubhamraj-git commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1844920931
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrorsModal.tsx:
##
@@ -0,0 +1,125 @@
+/*!
+ * 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.
+ */
+import { Heading, Text, HStack, Input } from "@chakra-ui/react";
+import { useEffect, useState } from "react";
+import { PiFilePy } from "react-icons/pi";
+
+import type { ImportErrorResponse } from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+import { Accordion, Dialog } from "src/components/ui";
+import { Pagination } from "src/components/ui/Pagination";
+
+type ImportDAGErrorModalProps = {
+ importErrors: Array;
+ onClose: () => void;
+ open: boolean;
+};
+
+const PAGE_LIMIT = 5;
+
+export const DAGImportErrorsModal: React.FC = ({
+ importErrors,
+ onClose,
+ open,
+}) => {
+ const [page, setPage] = useState(1);
+ const [searchQuery, setSearchQuery] = useState("");
+ const [filteredErrors, setFilteredErrors] = useState(importErrors);
+
+ const startRange = (page - 1) * PAGE_LIMIT;
+ const endRange = startRange + PAGE_LIMIT;
+ const visibleItems = filteredErrors.slice(startRange, endRange);
+
+ const onOpenChange = () => {
+setSearchQuery("");
+setPage(1);
+onClose();
+ };
+
+ useEffect(() => {
+const query = searchQuery.toLowerCase();
+const filtered = importErrors.filter((error) =>
+ error.filename.toLowerCase().includes(query),
+);
+
+setFilteredErrors(filtered);
+setPage(1);
+ }, [searchQuery, importErrors]);
Review Comment:
Ohh okay, I though to just have Client-Side Pagination and Search, since the
data won't be that big for this.
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrorsModal.tsx:
##
@@ -0,0 +1,125 @@
+/*!
+ * 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.
+ */
+import { Heading, Text, HStack, Input } from "@chakra-ui/react";
+import { useEffect, useState } from "react";
+import { PiFilePy } from "react-icons/pi";
+
+import type { ImportErrorResponse } from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+import { Accordion, Dialog } from "src/components/ui";
+import { Pagination } from "src/components/ui/Pagination";
+
+type ImportDAGErrorModalProps = {
+ importErrors: Array;
+ onClose: () => void;
+ open: boolean;
+};
+
+const PAGE_LIMIT = 5;
+
+export const DAGImportErrorsModal: React.FC = ({
+ importErrors,
+ onClose,
+ open,
+}) => {
+ const [page, setPage] = useState(1);
+ const [searchQuery, setSearchQuery] = useState("");
+ const [filteredErrors, setFilteredErrors] = useState(importErrors);
+
+ const startRange = (page - 1) * PAGE_LIMIT;
+ const endRange = startRange + PAGE_LIMIT;
+ const visibleItems = filteredErrors.slice(startRange, endRange);
+
+ const onOpenChange = () => {
+setSearchQuery("");
+setPage(1);
+onClose();
+ };
+
+ useEffect(() => {
+const query = searchQuery.toLowerCase();
+const filtered = importErrors.filter((error) =>
+ error.filename.toLowerCase().includes(query),
+);
+
+setFilteredErrors(filtered);
+setPage(1);
+ }, [searchQuery, importErrors]);
Review Comment:
Ohh okay, I thought to just have Client-Side Pagination and Search, since
the data won't be that big for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub a
Re: [PR] AIP-38 | List Import Errors [airflow]
shubhamraj-git commented on PR #44021: URL: https://github.com/apache/airflow/pull/44021#issuecomment-2479889069 > One point to add is that current import errors are shown all at once in the legacy home page which makes using `Ctrl+F` easier to search especially on large Airflow instances with lot of users and hundreds of import errors which might not be possible anymore with pagination. @tirkarthi I have added a search option to do that. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1844475924
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrorsModal.tsx:
##
@@ -0,0 +1,125 @@
+/*!
+ * 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.
+ */
+import { Heading, Text, HStack, Input } from "@chakra-ui/react";
+import { useEffect, useState } from "react";
+import { PiFilePy } from "react-icons/pi";
+
+import type { ImportErrorResponse } from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+import { Accordion, Dialog } from "src/components/ui";
+import { Pagination } from "src/components/ui/Pagination";
+
+type ImportDAGErrorModalProps = {
+ importErrors: Array;
+ onClose: () => void;
+ open: boolean;
+};
+
+const PAGE_LIMIT = 5;
+
+export const DAGImportErrorsModal: React.FC = ({
+ importErrors,
+ onClose,
+ open,
+}) => {
+ const [page, setPage] = useState(1);
+ const [searchQuery, setSearchQuery] = useState("");
+ const [filteredErrors, setFilteredErrors] = useState(importErrors);
+
+ const startRange = (page - 1) * PAGE_LIMIT;
+ const endRange = startRange + PAGE_LIMIT;
+ const visibleItems = filteredErrors.slice(startRange, endRange);
+
+ const onOpenChange = () => {
+setSearchQuery("");
+setPage(1);
+onClose();
+ };
+
+ useEffect(() => {
+const query = searchQuery.toLowerCase();
+const filtered = importErrors.filter((error) =>
+ error.filename.toLowerCase().includes(query),
+);
+
+setFilteredErrors(filtered);
+setPage(1);
+ }, [searchQuery, importErrors]);
+
+ return (
+
+
+
+ Dag Import Errors
+ setSearchQuery(letters.target.value)}
Review Comment:
```suggestion
onChange={(event) => setSearchQuery(event.target.value)}
```
We're accessing a ChangeEvent. Let's not rename it.
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrorsModal.tsx:
##
@@ -0,0 +1,125 @@
+/*!
+ * 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.
+ */
+import { Heading, Text, HStack, Input } from "@chakra-ui/react";
+import { useEffect, useState } from "react";
+import { PiFilePy } from "react-icons/pi";
+
+import type { ImportErrorResponse } from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+import { Accordion, Dialog } from "src/components/ui";
+import { Pagination } from "src/components/ui/Pagination";
+
+type ImportDAGErrorModalProps = {
+ importErrors: Array;
+ onClose: () => void;
+ open: boolean;
+};
+
+const PAGE_LIMIT = 5;
+
+export const DAGImportErrorsModal: React.FC = ({
+ importErrors,
+ onClose,
+ open,
+}) => {
+ const [page, setPage] = useState(1);
+ const [searchQuery, setSearchQuery] = useState("");
+ const [filteredErrors, setFilteredErrors] = useState(importErrors);
+
+ const startRange = (page - 1) * PAGE_LIMIT;
+ const endRange = startRange + PAGE_LIMIT;
+ const visibleItems = filteredErrors.slice(startRange, endRange);
+
+ const onOpenChange = () => {
+setSearchQuery("");
+setPage(1);
+onClose();
+ };
+
+ useEffect(() => {
+const query = searchQuery.toLowerCase();
+const filtered = importErrors.filter((error) =>
+ error.filename.toLowerCase().includes(query),
+);
+
+setFilteredErrors(filtered);
+setPage(1);
+ }, [searchQuery, importErrors]);
+
+ return (
+
+
+
+ Dag Import Errors
+ setSearchQuery(let
Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1844356582
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { useImportErrorServiceGetImportErrors } from "openapi/queries";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+
+ const { data, error } = useImportErrorServiceGetImportErrors();
Review Comment:
Let's make sure to add a loading indicator with a `Skeleton` thats roughly
the same size as the button
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1844478058
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrorsModal.tsx:
##
@@ -0,0 +1,125 @@
+/*!
+ * 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.
+ */
+import { Heading, Text, HStack, Input } from "@chakra-ui/react";
+import { useEffect, useState } from "react";
+import { PiFilePy } from "react-icons/pi";
+
+import type { ImportErrorResponse } from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+import { Accordion, Dialog } from "src/components/ui";
+import { Pagination } from "src/components/ui/Pagination";
+
+type ImportDAGErrorModalProps = {
+ importErrors: Array;
+ onClose: () => void;
+ open: boolean;
+};
+
+const PAGE_LIMIT = 5;
+
+export const DAGImportErrorsModal: React.FC = ({
+ importErrors,
+ onClose,
+ open,
+}) => {
+ const [page, setPage] = useState(1);
+ const [searchQuery, setSearchQuery] = useState("");
+ const [filteredErrors, setFilteredErrors] = useState(importErrors);
+
+ const startRange = (page - 1) * PAGE_LIMIT;
+ const endRange = startRange + PAGE_LIMIT;
+ const visibleItems = filteredErrors.slice(startRange, endRange);
+
+ const onOpenChange = () => {
+setSearchQuery("");
+setPage(1);
+onClose();
+ };
+
+ useEffect(() => {
+const query = searchQuery.toLowerCase();
+const filtered = importErrors.filter((error) =>
+ error.filename.toLowerCase().includes(query),
+);
+
+setFilteredErrors(filtered);
+setPage(1);
+ }, [searchQuery, importErrors]);
+
+ return (
+
+
+
+ Dag Import Errors
+ setSearchQuery(letters.target.value)}
+placeholder="Search by file"
+value={searchQuery}
+ />
+
+
+
+
+
+
+{visibleItems.map((importError) => (
+
+
+
+ {importError.filename}
+
+
+
+Timestamp:
+
+
+{importError.stack_trace}
+
Review Comment:
```suggestion
{importError.stack_trace}
```
We can just use chakra's `Code` component
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1844431509
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { useImportErrorServiceGetImportErrors } from "openapi/queries";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+
+ const { data, error } = useImportErrorServiceGetImportErrors();
+ const importErrorsCount = data?.total_entries ?? 0;
+ const importErrors = data?.import_errors ?? [];
+
+ return (
+importErrorsCount > 0 && (
+
+
+
Review Comment:
Actually we can just do `colorPalette="red"` and let the badge component
handle it
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { useImportErrorServiceGetImportErrors } from "openapi/queries";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+
+ const { data, error } = useImportErrorServiceGetImportErrors();
+ const importErrorsCount = data?.total_entries ?? 0;
+ const importErrors = data?.import_errors ?? [];
+
+ return (
+importErrorsCount > 0 && (
+
+
+
Review Comment:
Actually we can just do `colorPalette="red"` and let the badge component
handle the rest
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r189160
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { useImportErrorServiceGetImportErrors } from "openapi/queries";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+
+ const { data, error } = useImportErrorServiceGetImportErrors();
+ const importErrorsCount = data?.total_entries ?? 0;
+ const importErrors = data?.import_errors ?? [];
+
+ return (
+importErrorsCount > 0 && (
+
+
+
Review Comment:
Ok nevermind then.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
shubhamraj-git commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r182488
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { useImportErrorServiceGetImportErrors } from "openapi/queries";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+
+ const { data, error } = useImportErrorServiceGetImportErrors();
+ const importErrorsCount = data?.total_entries ?? 0;
+ const importErrors = data?.import_errors ?? [];
+
+ return (
+importErrorsCount > 0 && (
+
+
+
Review Comment:
Yaa, we can do, But in that case the bg will light red and number will
displayed in red.
It would be different from screenshot attached to issue.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1844357907
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { useImportErrorServiceGetImportErrors } from "openapi/queries";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+
+ const { data, error } = useImportErrorServiceGetImportErrors();
+ const importErrorsCount = data?.total_entries ?? 0;
+ const importErrors = data?.import_errors ?? [];
+
+ return (
+importErrorsCount > 0 && (
+
+
+
Review Comment:
```suggestion
```
Let's make sure to use the correct semantic tokens.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1844357907
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { useImportErrorServiceGetImportErrors } from "openapi/queries";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+
+ const { data, error } = useImportErrorServiceGetImportErrors();
+ const importErrorsCount = data?.total_entries ?? 0;
+ const importErrors = data?.import_errors ?? [];
+
+ return (
+importErrorsCount > 0 && (
+
+
+
Review Comment:
```suggestion
```
Let's make sure to use the correct semantic tokens.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
tirkarthi commented on PR #44021: URL: https://github.com/apache/airflow/pull/44021#issuecomment-2476858937 One point to add is that current import errors are shown all at once in the legacy home page which makes using `Ctrl+F` easier to search especially on large Airflow instances with lot of users and hundreds of import errors which might not be possible anymore with pagination. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
tirkarthi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1842575801
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { useState } from "react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+ const [importErrorsCount, setImportErrorsCount] = useState(-1);
+
+ const handleImportErrorsCount = (count: number) => {
+setImportErrorsCount(count);
+ };
+
+ return (
+
+ {importErrorsCount > 0 && (
+
+
+{importErrorsCount}
+
+
+DAG Import Errors
Review Comment:
Dag is used instead of DAG in most parts with renaming done in
https://github.com/apache/airflow/pull/43325 . Probably could also be done in
this PR for user visible content. Thanks.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-38 | List Import Errors [airflow]
bbovenzi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1842512783
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * 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.
+ */
+import { Box, Badge, Text, Button, useDisclosure } from "@chakra-ui/react";
+import { useState } from "react";
+import { FiChevronRight } from "react-icons/fi";
+
+import { DAGImportErrorsModal } from "./DAGImportErrorsModal";
+
+export const DAGImportErrors = () => {
+ const { onClose, onOpen, open } = useDisclosure();
+ const [importErrorsCount, setImportErrorsCount] = useState(-1);
+
+ const handleImportErrorsCount = (count: number) => {
+setImportErrorsCount(count);
+ };
Review Comment:
```suggestion
```
We should just use the `total_entries` variable for our count. There's no
need to make it a component state.
##
airflow/ui/src/pages/Dashboard/Dashboard.tsx:
##
@@ -18,7 +18,8 @@
*/
import { Box, Heading } from "@chakra-ui/react";
-import { Health } from "./Health";
+import { Health } from "./Health/Health";
+import { Stats } from "./Stats/Stats";
Review Comment:
Let's add a `/Stats/index.ts` file so the import statements are a little
cleaner
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrorsModal.tsx:
##
@@ -0,0 +1,131 @@
+/*!
+ * 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.
+ */
+import { VStack, Heading, Text, Box, Button, HStack } from "@chakra-ui/react";
+import { useEffect, useState } from "react";
+import { PiFilePy } from "react-icons/pi";
+
+import type { ImportErrorResponse } from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+import { Accordion, Dialog } from "src/components/ui";
+import { useImportErrors } from "src/queries/useDagsImportErrors";
+
+type ImportDAGErrorModalProps = {
+ onClose: () => void;
+ onErrorCountChange: (count: number) => void;
+ open: boolean;
+};
+
+const PAGE_LIMIT = 5;
+
+export const DAGImportErrorsModal: React.FC = ({
+ onClose,
+ onErrorCountChange,
+ open,
+}) => {
+ const [currentPage, setCurrentPage] = useState(0);
+
+ const { data } = useImportErrors({
+limit: PAGE_LIMIT,
+offset: currentPage * PAGE_LIMIT,
+ });
+
+ const importErrors: Array = data.import_errors;
+ const importErrorsCount: number = data.total_entries || 0;
+ const totalPages = Math.ceil(importErrorsCount / PAGE_LIMIT);
+
+ useEffect(() => {
+onErrorCountChange(importErrorsCount);
+ }, [importErrorsCount, onErrorCountChange]);
+
+ useEffect(() => {
+if (!open) {
+ setCurrentPage(0);
+}
+ }, [open]);
+
+ const handleNextPage = () => {
+if (currentPage < totalPages - 1) {
+ setCurrentPage((prev) => prev + 1);
+}
+ };
+
+ const handlePreviousPage = () => {
+if (currentPage > 0) {
+ setCurrentPage((prev) => prev - 1);
+}
+ };
+
+ return (
+
Review Comment:
Let's make a `onOpenChange` function that resets the page count and calls
`onClose` then we don't need the `useEffect` above
##
airflow/ui/src/queries/useDagsImportErrors.tsx:
##
Review Comment:
We don't need this file. All of the query options are already set by
default. Let's just call `useImportErrorServiceGetImportErrors` directly in our
component
##
airflow/ui/src/pages/Dashboard/Stats/DAGImportErrorsModal.tsx:
##
@@ -0,0 +1,131 @@
+/*!
+ *
Re: [PR] AIP-38 | List Import Errors [airflow]
tirkarthi commented on code in PR #44021:
URL: https://github.com/apache/airflow/pull/44021#discussion_r1842528620
##
airflow/ui/src/pages/Dashboard/Dashboard.tsx:
##
@@ -18,7 +18,8 @@
*/
import { Box, Heading } from "@chakra-ui/react";
-import { Health } from "./Health";
+import { Health } from "./Health/Health";
Review Comment:
I guess you can create `Health/index.ts` to have `export { Health } from
'./Health'` to avoid `./Health/Health` and just `./Health` . Similar change for
Stats.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
