AIP-38 | List Import Errors #44021
AIP-38 | List Import Errors #44021
Conversation
shubhamraj-git
requested review from
bbovenzi,
pierrejeambrun,
ryanahamilton and
jscheffl
as code owners
| @@ -18,7 +18,8 @@ | |||
| */ | |||
| import { Box, Heading } from "@chakra-ui/react"; | |||
|
|
|||
| import { Health } from "./Health"; | |||
| import { Health } from "./Health/Health"; | |||
There was a problem hiding this 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.
|
One point to add is that current import errors are shown all at once in the legacy home page which makes using |
| {importErrorsCount} | ||
| </Badge> | ||
| <Box alignItems="center" display="flex" gap={1}> | ||
| <Text fontWeight="bold">DAG Import Errors</Text> |
There was a problem hiding this comment.
Dag is used instead of DAG in most parts with renaming done in #43325 . Probably could also be done in this PR for user visible content. Thanks.
@tirkarthi I have added a search option to do that. |
| open: boolean; | ||
| }; | ||
|
|
||
| const PAGE_LIMIT = 5; |
There was a problem hiding this comment.
Doing some manual testing. I think we can increase this to 15
| <Heading size="xl">Dag Import Errors</Heading> | ||
| <Input | ||
| mt={4} | ||
| onChange={(letters) => setSearchQuery(letters.target.value)} |
There was a problem hiding this comment.
| onChange={(letters) => setSearchQuery(letters.target.value)} | |
| onChange={(event) => setSearchQuery(event.target.value)} |
We're accessing a ChangeEvent. Let's not rename it.
|
|
||
| <Pagination.Root | ||
| count={filteredErrors.length} | ||
| onPageChange={(each) => setPage(each.page)} |
There was a problem hiding this comment.
| onPageChange={(each) => setPage(each.page)} | |
| onPageChange={(event) => setPage(event.page)} |
| <Text color="fg.error" fontSize="sm" whiteSpace="pre-wrap"> | ||
| <code>{importError.stack_trace}</code> | ||
| </Text> |
There was a problem hiding this comment.
| <Text color="fg.error" fontSize="sm" whiteSpace="pre-wrap"> | |
| <code>{importError.stack_trace}</code> | |
| </Text> | |
| <Code color="fg.error" fontSize="sm" whiteSpace="pre-wrap"> | |
| {importError.stack_trace} | |
| </Code> |
We can just use chakra's Code component
|
|
||
| const { | ||
| data, | ||
| error: DagImportErrorFailure, |
There was a problem hiding this comment.
| error: DagImportErrorFailure, | |
| error, |
First, we don't want to capitalize regular variable names. Second, I think just error is fine, apiError if you want to be more specific
| const importErrorsCount = data?.total_entries ?? 0; | ||
| const importErrors = data?.import_errors ?? []; | ||
|
|
||
| if (isFetching || isLoading) { |
There was a problem hiding this comment.
| if (isFetching || isLoading) { | |
| if (iisLoading) { |
Let's do just isLoading, so we can fetch in the background without bothering the user.
| useEffect(() => { | ||
| const query = searchQuery.toLowerCase(); | ||
| const filtered = importErrors.filter((error) => | ||
| error.filename.toLowerCase().includes(query), | ||
| ); | ||
|
|
||
| setFilteredErrors(filtered); | ||
| setPage(1); | ||
| }, [searchQuery, importErrors]); |
There was a problem hiding this comment.
Eventually we probably want to add a search param to the request and handle all search+pagination through the API.
Let's leave a TODO comment for that at least.
closes: #43711
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.