You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/04/02 00:50:42 UTC
Re: [PR] UI: Fix silent errors and layout shift in import error components [airflow]
Copilot commented on code in PR #64267:
URL: https://github.com/apache/airflow/pull/64267#discussion_r3025339154
##########
airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/PluginImportErrors.tsx:
##########
@@ -38,42 +38,50 @@ export const PluginImportErrors = ({ iconOnly = false }: { readonly iconOnly?: b
const importErrors = data?.import_errors ?? [];
if (isLoading) {
- return <Skeleton height="9" width="225px" />;
+ // Skeleton dimensions match the rendered component sizes to prevent layout shift.
+ // iconOnly: 28px × 60px matches StateBadge (height={7} = 28px in Chakra spacing scale).
+ // full: 42px × 175px matches the rendered StatsCard dimensions.
+ return iconOnly ? (
+ <Skeleton height="28px" width="60px" />
+ ) : (
+ <Skeleton height="42px" width="175px" />
+ );
Review Comment:
The loading Skeleton sizes are duplicated and hard-coded (28×60 and 42×175). This risks getting out of sync with `StateBadge` / `StatsCard` styling changes and reintroducing layout shift. Consider reusing `StatsCard`’s loading skeleton for the full variant and centralizing the `iconOnly` placeholder dimensions (or rendering a fixed-size `StateBadge`-like wrapper) to avoid repeating magic numbers.
##########
airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##########
@@ -37,38 +37,46 @@ export const DAGImportErrors = ({ iconOnly = false }: { readonly iconOnly?: bool
const importErrorsCount = data?.total_entries ?? 0;
if (isLoading) {
- return <Skeleton height="9" width="225px" />;
+ // Skeleton dimensions match the rendered component sizes to prevent layout shift.
+ // iconOnly: 28px × 60px matches StateBadge (height={7} = 28px in Chakra spacing scale).
+ // full: 42px × 175px matches the rendered StatsCard dimensions.
+ return iconOnly ? (
+ <Skeleton height="28px" width="60px" />
+ ) : (
+ <Skeleton height="42px" width="175px" />
+ );
Review Comment:
The loading Skeleton sizes are hard-coded here (28×60 and 42×175). This can easily drift from the real rendered sizes if `StateBadge` / `StatsCard` styles change (reintroducing layout shift). Consider deriving these from the same source (e.g. reuse `StatsCard`’s built-in loading skeleton for the full variant, and for `iconOnly` either render a `StateBadge` container with a fixed/min width while loading, or share constants with the badge styles) to avoid duplicated magic numbers.
##########
airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx:
##########
@@ -37,38 +37,46 @@ export const DAGImportErrors = ({ iconOnly = false }: { readonly iconOnly?: bool
const importErrorsCount = data?.total_entries ?? 0;
if (isLoading) {
- return <Skeleton height="9" width="225px" />;
+ // Skeleton dimensions match the rendered component sizes to prevent layout shift.
+ // iconOnly: 28px × 60px matches StateBadge (height={7} = 28px in Chakra spacing scale).
+ // full: 42px × 175px matches the rendered StatsCard dimensions.
+ return iconOnly ? (
+ <Skeleton height="28px" width="60px" />
+ ) : (
+ <Skeleton height="42px" width="175px" />
+ );
}
- if (importErrorsCount === 0) {
+ if (importErrorsCount === 0 && !error) {
return undefined;
}
Review Comment:
This changes behavior so the component renders (via `ErrorAlert`) when the import-errors query fails (previously it returned early when `total_entries` defaulted to 0). Please add a regression test that simulates an API error (e.g. 500) and asserts the error alert is shown and the badge/card is not rendered when `total_entries` is 0.
##########
airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/PluginImportErrors.tsx:
##########
@@ -38,42 +38,50 @@ export const PluginImportErrors = ({ iconOnly = false }: { readonly iconOnly?: b
const importErrors = data?.import_errors ?? [];
if (isLoading) {
- return <Skeleton height="9" width="225px" />;
+ // Skeleton dimensions match the rendered component sizes to prevent layout shift.
+ // iconOnly: 28px × 60px matches StateBadge (height={7} = 28px in Chakra spacing scale).
+ // full: 42px × 175px matches the rendered StatsCard dimensions.
+ return iconOnly ? (
+ <Skeleton height="28px" width="60px" />
+ ) : (
+ <Skeleton height="42px" width="175px" />
+ );
}
if (Boolean(error) && (error as ExpandedApiError).status === 403) {
return undefined;
}
- if (importErrorsCount === 0) {
+ if (importErrorsCount === 0 && !error) {
return undefined;
}
Review Comment:
This updates the early-return guard to allow the `ErrorAlert` to render when the query fails (since `total_entries` defaults to 0 on failure). Please add a regression test that mocks an API error and verifies the error alert shows even when `importErrorsCount` is 0, and that the badge/card only renders when `importErrorsCount > 0`.
##########
airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/PluginImportErrors.tsx:
##########
@@ -38,42 +38,50 @@ export const PluginImportErrors = ({ iconOnly = false }: { readonly iconOnly?: b
const importErrors = data?.import_errors ?? [];
if (isLoading) {
- return <Skeleton height="9" width="225px" />;
+ // Skeleton dimensions match the rendered component sizes to prevent layout shift.
+ // iconOnly: 28px × 60px matches StateBadge (height={7} = 28px in Chakra spacing scale).
+ // full: 42px × 175px matches the rendered StatsCard dimensions.
+ return iconOnly ? (
+ <Skeleton height="28px" width="60px" />
+ ) : (
+ <Skeleton height="42px" width="175px" />
+ );
}
if (Boolean(error) && (error as ExpandedApiError).status === 403) {
Review Comment:
The 403 handling does an unsafe cast `(error as ExpandedApiError).status`. Elsewhere in the UI (e.g. `pages/Dashboard/PoolSummary/PoolSummary.tsx:50-52`) code checks `error?.status === 403` without a cast. Consider typing the query hook’s `error` (via generics) or using optional chaining (`(error as ExpandedApiError | undefined)?.status`) to avoid a potential runtime crash if `error` is not an `ApiError` shape.
```suggestion
if ((error as ExpandedApiError | undefined)?.status === 403) {
```
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org