Copilot commented on code in PR #64284:
URL: https://github.com/apache/airflow/pull/64284#discussion_r3025330566
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskNames.tsx:
##########
@@ -67,6 +67,26 @@ export const TaskNames = ({ nodes, onRowClick, virtualItems
}: Props) => {
}
};
+ const handleToggleGroupOnLinkClick = (groupNodeId?: string) => {
+ if (groupNodeId === undefined || groupNodeId === "") {
+ return;
+ }
+
+ const id = groupNodeId;
+ const isViewingSameGroup = typeof groupId === "string" && groupId === id;
+ const isOpen = openGroupIds.includes(id);
+
+ if (isViewingSameGroup) {
+ toggleGroupId(id);
+
+ return;
+ }
Review Comment:
When clicking a group name while already viewing that same group, this
handler toggles the group but the RouterLink click will still proceed with
navigation (replace to the same route). That can cause redundant
navigation/revalidation and doesn’t match the intended “toggle inline”
behavior. Consider passing the click event into this handler and calling
preventDefault/stopPropagation when `isViewingSameGroup` is true so the click
only toggles.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskNames.tsx:
##########
@@ -67,6 +67,26 @@ export const TaskNames = ({ nodes, onRowClick, virtualItems
}: Props) => {
}
};
+ const handleToggleGroupOnLinkClick = (groupNodeId?: string) => {
+ if (groupNodeId === undefined || groupNodeId === "") {
+ return;
+ }
+
+ const id = groupNodeId;
+ const isViewingSameGroup = typeof groupId === "string" && groupId === id;
+ const isOpen = openGroupIds.includes(id);
+
+ if (isViewingSameGroup) {
+ toggleGroupId(id);
+
+ return;
+ }
+
+ if (!isOpen) {
+ setOpenGroupIds([...new Set([...openGroupIds, id])]);
Review Comment:
`setOpenGroupIds` from `OpenGroupsProvider` cancels any pending debounced
updates before setting immediately. Calling it here can unintentionally drop a
very recent `toggleGroupId()` change (within the debounce window) made just
before the user clicks a group name. To avoid losing pending toggles, consider
using a non-cancelling “add/open group” helper in the context, or otherwise
merging with the latest open-group state without cancelling queued updates.
```suggestion
toggleGroupId(id);
```
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskNames.tsx:
##########
@@ -109,7 +129,12 @@ export const TaskNames = ({ nodes, onRowClick,
virtualItems }: Props) => {
{node.isGroup ? (
<Link asChild data-testid={node.id} display="block" width="100%">
<RouterLink
- onClick={onRowClick}
+ onClick={() => {
+ handleToggleGroupOnLinkClick(node.id);
+ if (onRowClick) {
+ onRowClick();
+ }
+ }}
Review Comment:
This change introduces new user-facing interaction in Grid View (clicking
the task-group name affects open/closed state before navigation and behaves
differently when already in the group), but there’s no automated coverage for
it. The existing Playwright grid spec
(`airflow-core/src/airflow/ui/tests/e2e/specs/dag-grid-view.spec.ts`) doesn’t
assert task-group expand/drilldown behavior; please add a regression test for
the new click semantics.
--
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]
