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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
