kaxil commented on code in PR #73693:
URL: https://github.com/apache/airflow/pull/73693#discussion_r4109660388
##########
airflow-core/src/airflow/ui/src/pages/Dag/Header.tsx:
##########
@@ -113,11 +114,27 @@ export const Header = ({
},
...nextRunStat,
{
- label: translate("dagDetails.activeRuns"),
+ label:
+ (dag?.queued_runs_count ?? 0) > 0 ? (
+ <HStack gap={1}>
+ {translate("dagDetails.activeRuns")}
+ <Tooltip
content={translate("dagDetails.activeRunsExceedsMaxTooltip")}>
Review Comment:
This tooltip isn't `portalled`, so the system `Tooltip` renders its content
inline (`<Portal disabled={!portalled}>` in `system-components/Tooltip.tsx`).
Inline, it sits inside HeaderCard's stat label `Box`, which sets
`textTransform="uppercase"`, so the text comes out in caps and covers the
value. It shows in your screenshot: "RUNS BEYOND THE LIMIT WILL NOT START..."
drawn over "1 of 1 (2 queued)". Passing `portalled` fixes both, the same way
`IconButton` and `RunTypeLegend` do.
```suggestion
<Tooltip
content={translate("dagDetails.activeRunsExceedsMaxTooltip")} portalled>
```
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dags.py:
##########
@@ -1503,6 +1505,11 @@ def test_dag_details_includes_active_runs_count(self,
session, test_client):
assert isinstance(body["active_runs_count"], int)
assert body["active_runs_count"] == 1 # only running counts, queued
does not
+ # Verify queued_runs_count field is present and correct
+ assert "queued_runs_count" in body
+ assert isinstance(body["queued_runs_count"], int)
+ assert body["queued_runs_count"] == 1 # only queued counts,
running/success do not
Review Comment:
With one RUNNING and one QUEUED run seeded, this still passes if the new
query filters on `DagRunState.RUNNING` by mistake, since both counts come out
as 1. Seeding a second QUEUED run and asserting `queued_runs_count == 2` next
to `active_runs_count == 1` would tell them apart. The `in` / `isinstance`
asserts don't add anything over the equality checks, and the test name still
only mentions `active_runs_count`.
##########
airflow-core/src/airflow/ui/src/components/HeaderCard.tsx:
##########
@@ -77,8 +77,8 @@ export const HeaderCard = ({ actions, icon, state, stats,
subTitle, title, type
</Flex>
<HStack alignItems="flex-start" flexWrap="wrap" gap={6} my={3}>
- {stats.map((stat) => (
- <Box data-testid="stat" key={stat.key ?? stat.label}>
+ {stats.map((stat, index) => (
+ <Box data-testid="stat" key={stat.key ?? index}>
Review Comment:
`react/no-array-index-key` is an ERROR in this repo, and `key={stat.key ??
index}` only gets past it because the rule never inspects a `??` expression
(its `checkPropValue` handles a bare identifier, template literals, binary
expressions and calls). It also moves every HeaderCard caller that doesn't pass
`key` from a label key to an index key, to serve the one stat whose label is
now a node. Could the active runs stat pass `key: "activeRuns"` and this line
stay `key={stat.key ?? stat.label}`? Typing `stats` so that a non-string
`label` requires a `key` would keep the next caller from hitting the same thing.
##########
airflow-core/src/airflow/ui/src/pages/Dag/Header.test.tsx:
##########
@@ -93,6 +94,32 @@ describe("Header", () => {
expect(screen.getByText("2 of 2")).toBeInTheDocument();
});
+ it("does not show an info icon or queued count when nothing is queued", ()
=> {
+ render(
+ <Wrapper>
+ <Header dag={{ ...mockDag, active_runs_count: 1, max_active_runs: 2 }}
/>
+ </Wrapper>,
+ );
+
+
expect(screen.queryByTestId("active-runs-exceeds-max-info")).not.toBeInTheDocument();
+ expect(screen.getByText("1 of 2")).toBeInTheDocument();
+ });
+
+ it("shows an info icon and the queued count when runs are queued behind the
maximum", () => {
+ render(
+ <Wrapper>
+ <Header dag={{ ...mockDag, active_runs_count: 1, max_active_runs: 1,
queued_runs_count: 2 }} />
+ </Wrapper>,
+ );
+
+
expect(screen.getByTestId("active-runs-exceeds-max-info")).toBeInTheDocument();
+ expect(
+ screen.getByText(
+ i18n.t("common:dagDetails.activeRunsWithQueued", { activeRuns: 1,
maxActiveRuns: 1, queuedRuns: 2 }),
Review Comment:
The test setup doesn't load translation bundles, so both `i18n.t(...)` here
and the component render the raw key, and this assertion passes whatever the
counts are: changing the component's values to `activeRuns: 42, queuedRuns:
999` still passes. Loading the en bundle the way `RenderedJsonField.test.tsx`
does (`i18n.addResourceBundle("en", "common", commonLocale, true, true)`) and
asserting `"1 of 1 (2 queued)"` would pin the output. Separately, the "nothing
queued" case above uses 1 of 2, so nothing checks the icon stays hidden at
exactly capacity with nothing waiting, which is the reason the description
gives for not using the flag. `active_runs_count: 2, max_active_runs: 2` with
nothing queued would cover it.
--
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]
