pierrejeambrun commented on code in PR #67595:
URL: https://github.com/apache/airflow/pull/67595#discussion_r3372549130
##########
airflow-core/src/airflow/ui/src/pages/Dag/Calendar/calendarUtils.test.ts:
##########
@@ -153,6 +154,39 @@ describe("createCalendarScale", () => {
});
});
+ it("returns the failed color for a failed-only cell in total mode", () => {
Review Comment:
Could you also add a test for the hasPending && hasActual branch when
multiple actual states coexist? Today both getColor callbacks now prefer failed
over running over success in that case, but nothing asserts it — e.g. {
planned: 1, failed: 1, success: 1 } should pick red for the actual triangle and
stone for the planned triangle.
##########
airflow-core/src/airflow/ui/src/pages/Dag/Calendar/calendarUtils.ts:
##########
@@ -258,7 +275,29 @@ export const createCalendarScale = (
return PLANNED_COLOR;
}
- return actualCount === 0 ? EMPTY_COLOR : singleColor;
+ if (hasActual) {
+ if (failedCount > 0 && runningCount > 0) {
+ return { actual: RUNNING_COLOR, planned: failedColor };
+ }
+ if (failedCount > 0 && successCount > 0) {
+ return { actual: successColor, planned: failedColor };
+ }
+ if (runningCount > 0 && successCount > 0) {
+ return { actual: successColor, planned: RUNNING_COLOR };
+ }
+
+ if (failedCount > 0) {
+ return failedColor;
+ }
+ if (runningCount > 0) {
+ return RUNNING_COLOR;
+ }
+ if (successCount > 0) {
+ return successColor;
+ }
+ }
+
+ return EMPTY_COLOR;
Review Comment:
In the new branches at L283–297 (and the mirror at L378–397 in the gradient
path), { actual, planned } is reused for state-vs-state mixing — e.g.
failed+success returns { actual: green, planned: red }.
Since the keys originally meant "actual runs vs pending runs," reusing them
for "lower-priority state vs higher-priority state" is a bit of a trap for the
next reader. Could you either rename to something less ambiguous.
##########
airflow-core/src/airflow/ui/src/pages/Dag/Calendar/calendarUtils.test.ts:
##########
@@ -153,6 +154,39 @@ describe("createCalendarScale", () => {
});
});
Review Comment:
All four new tests use the default run() date so they collapse into a single
cell and exercise the single-value branch only. Could you add a couple of tests
that pass runs at different dates to force the gradient branch? Something like:
##########
airflow-core/src/airflow/ui/src/pages/Dag/Calendar/calendarUtils.ts:
##########
@@ -321,21 +375,40 @@ export const createCalendarScale = (
return PLANNED_COLOR;
}
- const targetCount = actualCount;
+ if (hasActual) {
+ if (failedCount > 0 && runningCount > 0) {
+ return {
+ actual: RUNNING_COLOR,
+ planned: getIntensityColor(failedCount, FAILURE_COLOR_INTENSITIES),
+ };
+ }
- if (targetCount === 0) {
- return colorScheme[0] ?? EMPTY_COLOR;
- }
+ if (failedCount > 0 && successCount > 0) {
+ return {
+ actual: getIntensityColor(successCount, TOTAL_COLOR_INTENSITIES),
+ planned: getIntensityColor(failedCount, FAILURE_COLOR_INTENSITIES),
+ };
+ }
- for (let index = uniqueThresholds.length - 1; index >= 1; index -= 1) {
- const threshold = uniqueThresholds[index];
+ if (runningCount > 0 && successCount > 0) {
+ return {
+ actual: getIntensityColor(successCount, TOTAL_COLOR_INTENSITIES),
+ planned: RUNNING_COLOR,
+ };
+ }
- if (threshold !== undefined && targetCount >= threshold) {
- return colorScheme[Math.min(index, colorScheme.length - 1)] ??
EMPTY_COLOR;
+ if (failedCount > 0) {
+ return getIntensityColor(failedCount, FAILURE_COLOR_INTENSITIES);
+ }
+ if (runningCount > 0) {
Review Comment:
The state-priority ladder (failed+running / failed+success / running+success
/ then singles) shows up twice — once in the single-value getColor and once in
the gradient getColor. Could you extract a small helper that takes the three
resolved colors and returns the right output? Not blocking, but it'd reduce the
chance of the two paths drifting later.
##########
airflow-core/src/airflow/ui/src/pages/Dag/Calendar/calendarUtils.ts:
##########
@@ -291,24 +330,39 @@ export const createCalendarScale = (
actual: string | { _dark: string; _light: string };
planned: string | { _dark: string; _light: string };
} => {
- const actualCount = getActualRunCount(counts, viewMode);
+ const failedCount = counts.failed;
+ const runningCount = viewMode === "total" ? counts.running : 0;
+ const successCount = viewMode === "total" ? counts.success : 0;
+
const hasPending = getPendingRunCount(counts) > 0;
- const hasActual = actualCount > 0;
+ const hasActual = failedCount > 0 || runningCount > 0 || successCount > 0;
- if (hasPending && hasActual) {
- let actualColor = colorScheme[0] ?? EMPTY_COLOR;
+ type ColorValue = string | { _dark: string; _light: string };
+ const getIntensityColor = (count: number, scheme: Array<ColorValue>) => {
Review Comment:
The legend below the calendar is still built from the count-intensity scheme
(green-only in total mode), so once cells start rendering red/cyan/mixes they
won't be reflected in the legend anymore. Could be a follow-up or fixed here,
as you prefer
--
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]
