UI: Fix silent errors and layout shift in import error components#64267
UI: Fix silent errors and layout shift in import error components#64267vamsi2246 wants to merge 2 commits intoapache:mainfrom
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
|
Thanks. Can you provide some screenshots or videos of before and after? |
airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/PluginImportErrors.tsx
Outdated
Show resolved
Hide resolved
becc9a0 to
c16879d
Compare
|
Thanks for the review! I'll add before/after screenshots shortly. |
|
Still asking for videos of the before and after conditions. |
pierrejeambrun
left a comment
There was a problem hiding this comment.
LGTM, would love some screenshot to document the PR as mentioned by Brent.
There was a problem hiding this comment.
Pull request overview
This PR improves the Dashboard “import errors” UI by ensuring API failures are visible (instead of being silently hidden) and by reducing layout shift during loading, plus a small icon-size polish.
Changes:
- Prevents silent-swallowing of API errors by only early-returning when the error count is 0 and there is no query error.
- Updates loading skeleton dimensions (including
iconOnly) to better match final rendered sizes and reduce flicker/layout shift. - Standardizes
iconOnlywarning icon size to16for better visibility.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/PluginImportErrors.tsx | Fixes error visibility when the query fails; adjusts loading skeleton sizing; updates icon size; keeps 403 hidden. |
| airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/DAGImportErrors.tsx | Fixes error visibility when the query fails; adjusts loading skeleton sizing; updates icon size. |
| 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" /> | ||
| ); |
There was a problem hiding this 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.
| 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" /> | ||
| ); |
There was a problem hiding this 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.
| if (importErrorsCount === 0 && !error) { | ||
| return undefined; | ||
| } |
There was a problem hiding this 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.
| if (importErrorsCount === 0 && !error) { | ||
| return undefined; | ||
| } |
There was a problem hiding this 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.
| ); | ||
| } | ||
|
|
||
| if (Boolean(error) && (error as ExpandedApiError).status === 403) { |
There was a problem hiding this 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.
| if (Boolean(error) && (error as ExpandedApiError).status === 403) { | |
| if ((error as ExpandedApiError | undefined)?.status === 403) { |
This PR improves the robustness and UX of the
DAGImportErrorsandPluginImportErrorscomponents by addressing three issues:undefinedearly when the error count was 0 (the default on failure).iconOnlymode to match the final badge size, eliminating a noticeable flicker on the Dags List and Dashboard pages.8(too small) to a standard16for better visibility.Was generative AI tooling used to co-author this PR?
Generated-by: Antigravity following the guidelines