pierrejeambrun commented on code in PR #68570:
URL: https://github.com/apache/airflow/pull/68570#discussion_r3459932039
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/utils.ts:
##########
@@ -152,6 +147,17 @@ export const transformGanttData = ({
endMs = dayjs(endDate).valueOf();
}
+ // Include scheduled/queued/start/end times in tooltip data
whenever the timestamps exist.
+ // start_when/end_when are carried on every segment of a try so
the tooltip reports the
+ // task's actual start and end on the scheduled and queued bars
too, not just the
+ // execution bar's own bounds.
+ const tryWhenForTooltip = {
+ ...(scheduledMs === undefined ? {} : { scheduled_when:
scheduledDttm }),
+ ...(queuedMs === undefined ? {} : { queued_when: queuedDttm }),
+ ...(startDate === null ? {} : { start_when: startDate }),
+ ...(startDate === null && !hasTaskRunning ? {} : { end_when:
dayjs(endMs).toISOString() }),
Review Comment:
These two lines aren't symmetric with each other:
- Gate: `start_when` skips only when `startDate === null`; `end_when` adds
`&& !hasTaskRunning`. The extra branch only fires for queued/scheduled tasks
(`isStatePending` is true) with no `start_date`, where the tooltip would then
show `Start Date` = the bar's own start (segment fallback) + `End Date` =
`Date.now()`. Two different scales in the same tooltip.
- Value: `start_when` keeps the raw API string
(`"2024-03-14T10:00:00+00:00"`); `end_when` is re-serialized via
`dayjs(endMs).toISOString()` to `"...Z"` with `.000` millis. The test even pins
this skew. Renders fine because dayjs reparses, but it's needless and it's the
reason the `tryWhenForTooltip` block had to move below `endMs`.
Suggest collapsing both into a single symmetric form — derive an
`effectiveEndDate` and gate on it like `start_when` does on `startDate`:
```suggestion
const effectiveEndDate = hasTaskRunning ? new
Date().toISOString() : endDate;
// Include scheduled/queued/start/end times in tooltip data
whenever the timestamps exist.
// start_when/end_when are carried on every segment of a try so
the tooltip reports the
// task's actual start and end on the scheduled and queued bars
too, not just the
// execution bar's own bounds.
const tryWhenForTooltip = {
...(scheduledMs === undefined ? {} : { scheduled_when:
scheduledDttm }),
...(queuedMs === undefined ? {} : { queued_when: queuedDttm }),
...(startDate === null ? {} : { start_when: startDate }),
...(effectiveEndDate === null ? {} : { end_when:
effectiveEndDate }),
};
```
Then the block doesn't need to live below `endMs` anymore (no ordering
dependency) and finished tasks keep the API's raw `end_date` string. The test
for the finished task can drop the `new Date(...).toISOString()` wrapping and
assert against the raw `"2024-03-14T10:05:00+00:00"` like it does for
`start_when`.
--
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]
