Copilot commented on code in PR #64387:
URL: https://github.com/apache/airflow/pull/64387#discussion_r3025327760
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/Gantt.tsx:
##########
@@ -224,21 +234,70 @@ export const Gantt = ({ dagRunState, limit, runType,
triggeringUser }: Props) =>
};
return (
- <Box
- height={`${fixedHeight}px`}
- minW="250px"
- ml={-2}
- mt={`${GRID_BODY_OFFSET_PX}px`}
- onMouseLeave={handleChartMouseLeave}
- w="100%"
- >
- <Bar
- data={chartData}
- options={chartOptions}
- style={{
- paddingTop: flatNodes.length === 1 ? 15 : 1.5,
- }}
- />
- </Box>
+ <>
+ {/* Date inputs that trigger a new API fetch when changed */}
+ <Box mb="4" display="flex" gap="4" alignItems="flex-end">
+ <Box display="flex" flexDirection="column">
+ <Text color="fg.muted" fontSize="xs" mb={1}>
+ {translate("startDate")}
+ </Text>
+ <Input
+ fontSize="sm"
+ fontWeight="medium"
+ maxW="200px"
Review Comment:
The `<Text>` elements are visually acting as labels but aren’t
programmatically associated with their corresponding `<Input>` elements. Use a
proper label association (e.g., Chakra `FormControl` + `FormLabel`, or set `id`
on the input and `htmlFor` on a label) and/or add `aria-label` so screen
readers can correctly announce the fields.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/Gantt.tsx:
##########
@@ -224,21 +234,70 @@ export const Gantt = ({ dagRunState, limit, runType,
triggeringUser }: Props) =>
};
return (
- <Box
- height={`${fixedHeight}px`}
- minW="250px"
- ml={-2}
- mt={`${GRID_BODY_OFFSET_PX}px`}
- onMouseLeave={handleChartMouseLeave}
- w="100%"
- >
- <Bar
- data={chartData}
- options={chartOptions}
- style={{
- paddingTop: flatNodes.length === 1 ? 15 : 1.5,
- }}
- />
- </Box>
+ <>
+ {/* Date inputs that trigger a new API fetch when changed */}
+ <Box mb="4" display="flex" gap="4" alignItems="flex-end">
+ <Box display="flex" flexDirection="column">
+ <Text color="fg.muted" fontSize="xs" mb={1}>
+ {translate("startDate")}
+ </Text>
+ <Input
+ fontSize="sm"
+ fontWeight="medium"
+ maxW="200px"
+ onChange={(e) => {
+ const value = e.target.value;
+ if (filterEndDate && value > filterEndDate) {
+ alert("Start date cannot be after end date");
+ return;
+ }
+ setFilterStartDate(value);
+ }}
+ placeholder="YYYY-MM-DD"
+ size="sm"
+ type="date"
+ value={filterStartDate}
+ />
+ </Box>
+ <Box display="flex" flexDirection="column">
+ <Text color="fg.muted" fontSize="xs" mb={1}>
+ {translate("endDate")}
+ </Text>
+ <Input
+ fontSize="sm"
+ fontWeight="medium"
+ maxW="200px"
+ onChange={(e) => {
+ const value = e.target.value;
+ if (filterStartDate && value < filterStartDate) {
+ alert("End date cannot be before start date");
+ return;
+ }
+ setFilterEndDate(value);
+ }}
Review Comment:
This validation blocks clearing the end date: when `value` is an empty
string, `value < filterStartDate` is true, so users can’t remove the end-date
filter after setting a start date. Fix by only validating when `value` is
non-empty (e.g., check `value && filterStartDate && value < filterStartDate`)
so the field can be cleared.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/gantt.py:
##########
@@ -59,8 +62,17 @@ def get_gantt_data(
dag_id: str,
run_id: str,
session: SessionDep,
+ start_date: Annotated[datetime | None, Query()] = None,
+ end_date: Annotated[datetime | None, Query()] = None,
) -> GanttResponse:
+
"""Get all task instance tries for Gantt chart."""
+
Review Comment:
There are blank lines containing trailing whitespace around the function
docstring. This can fail linters/formatters (e.g., pre-commit hooks). Remove
the trailing spaces so the blank lines are truly empty.
```suggestion
"""Get all task instance tries for Gantt chart."""
```
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/Gantt.tsx:
##########
@@ -224,21 +234,70 @@ export const Gantt = ({ dagRunState, limit, runType,
triggeringUser }: Props) =>
};
return (
- <Box
- height={`${fixedHeight}px`}
- minW="250px"
- ml={-2}
- mt={`${GRID_BODY_OFFSET_PX}px`}
- onMouseLeave={handleChartMouseLeave}
- w="100%"
- >
- <Bar
- data={chartData}
- options={chartOptions}
- style={{
- paddingTop: flatNodes.length === 1 ? 15 : 1.5,
- }}
- />
- </Box>
+ <>
+ {/* Date inputs that trigger a new API fetch when changed */}
+ <Box mb="4" display="flex" gap="4" alignItems="flex-end">
+ <Box display="flex" flexDirection="column">
+ <Text color="fg.muted" fontSize="xs" mb={1}>
+ {translate("startDate")}
+ </Text>
+ <Input
+ fontSize="sm"
+ fontWeight="medium"
+ maxW="200px"
+ onChange={(e) => {
+ const value = e.target.value;
+ if (filterEndDate && value > filterEndDate) {
+ alert("Start date cannot be after end date");
+ return;
+ }
Review Comment:
Using `alert()` for validation is disruptive, not styled consistently with
Chakra UI, and the strings aren’t localized. Prefer showing validation feedback
inline (e.g., FormErrorMessage) or using a Chakra toast, and source the message
from i18n (`translate(...)`) to match the rest of the UI.
##########
newsfragments/64387.significant.rst:
##########
@@ -0,0 +1 @@
+Add start date and end date filter to the Gantt view with server-side
filtering.
Review Comment:
Grammar: this should be plural (“filters”) since both start and end date are
added.
```suggestion
Add start date and end date filters to the Gantt view with server-side
filtering.
```
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_gantt.py:
##########
@@ -326,3 +326,27 @@ def test_should_response_404(self, test_client, dag_id,
run_id):
with assert_queries_count(3):
response = test_client.get(f"/gantt/{dag_id}/{run_id}")
assert response.status_code == 404
+
+ def test_gantt_with_start_date(self, test_client):
+ response = test_client.get(
+ f"/gantt/{DAG_ID}/run_1",
+ params={"start_date": "2024-11-30T10:06:00"},
+ )
+ assert response.status_code == 200
+
+ data = response.json()
+
+ # running task (NULL end_date) should STILL be included
+ task_ids = [ti["task_id"] for ti in data["task_instances"]]
+ assert "task3" in task_ids
Review Comment:
This test only asserts that one running task is included; it would still
pass even if the backend ignored `start_date` entirely. Strengthen it by also
asserting that at least one known task instance that should fall *outside* the
selected range is excluded (or that the returned set/count changes compared to
the unfiltered response).
--
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]
