You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Ei-Sandi (via GitHub)" <gi...@apache.org> on 2026/07/27 15:42:42 UTC
[PR] Keep Grid scroll position when opening a task. [airflow]
Ei-Sandi opened a new pull request, #70536:
URL: https://github.com/apache/airflow/pull/70536
## Summary
In the Dag details view, the Grid on the left keeps its own vertical scroll position. Clicking a task that is scrolled below the fold currently snaps the Grid back to the top the **first** time you do it — losing your place. This PR keeps the scroll position across that navigation, while still resetting to the top when you leave the Dag entirely and come back.
## Problem
On a Dag with enough tasks to overflow the Grid, scroll down and click a task that is off the top of the viewport. The list jumps back to `scrollTop = 0` (the first task) even though the layout looks unchanged. Clicking a *second* task (task → task) does **not** reset the scroll — it stays put. So the reset happens exactly once, on the first click, and it gets more painful the longer the list is (e.g. when task groups are expanded).
## Root cause
The Grid lives inside `DetailsLayout`, and **every** top-level page renders its own `DetailsLayout`:
- `dags/:dagId` → `<Dag>`
- `dags/:dagId/tasks/:taskId` → `<Task>`
- `dags/:dagId/runs/:runId` → `<Run>`
- `dags/:dagId/runs/:runId/tasks/:taskId` → `<TaskInstance>`
- `<GroupTaskInstance>`, `<MappedTaskInstance>`
These are **sibling routes** in `router.tsx`, not nested under a shared layout.
- **First click** (`<Dag>` → `<Task>`): the matched route element changes, so React unmounts `<Dag>` (destroying its `DetailsLayout`, its `Grid`, and the scroll container DOM node) and mounts a fresh `<Task>`. The new scroll container starts at `scrollTop = 0`, and `useVirtualizer` reads `0` on mount and renders the top rows.
- **Subsequent clicks** (`<Task>` → `<Task>`): the matched element type is unchanged; only the `:taskId` param updates. React keeps the instance and its DOM, so the scroll position survives.
That asymmetry — cross-element on the first hop, same-element afterwards — is why it resets exactly once. It is not task-group specific; groups just make the list longer so more scroll is lost.
## Solution (this PR)
Remember the Grid's `scrollTop` across the remount and restore it, scoped to a single Dag visit.
- **Persist + restore** — `useGridScrollRestoration` saves `scrollTop` (in a module-scoped `Map` keyed by `dagId`, which survives the remount) on scroll, and restores it once, after the rows exist, in a `useLayoutEffect` (before
paint, no flash). Covers both the plain-Grid and the shared Grid + Gantt scroll containers.
- **Reset on leave** — `useResetGridScrollOnLeave` (called once from the always-mounted `BaseLayout`) clears the store whenever the route is not a Dag detail path (`/dags/<id>…`). So scroll is kept only *within* a Dag; leaving to
the Dags list / home / assets and returning starts at the top.
| Navigation | Scroll |
|---|---|
| Dag → Task → Run → Task Instance (same `dagId`) | preserved |
| Dag → Dags list / home / assets → back to Dag | reset to top |
| First visit to a Dag | starts at top |
Files: `useGridScrollRestoration.ts` (+ test), `Grid.tsx` (wire-up), and one line in `BaseLayout.tsx` (reset-on-leave). Routing and the panel layout are untouched.
## Alternative considered (not taken)
**Make `DetailsLayout` a single shared parent route** so the Grid mounts once and persists across Dag ↔ Run ↔ Task ↔ TaskInstance (the "correct" React Router structure), with per-page content flowing through the existing `<Outlet/>`.
Rejected as disproportionate: it is a large refactor with real regression risk.
## Testing
New unit tests cover:
- **Persist/restore:** save-on-scroll, restore-once-rows-exist, deferred restore when rows arrive later, restore fires only once (does not clobber later user scrolling), no-op when there is no saved offset / a zero offset / a missing scroll element, and listener cleanup on unmount.
- **Reset-on-leave:** `isDagDetailsPath` for detail vs non-detail paths, `clearGridScrollOffsets` drops every entry, and the hook clears on a non-detail path but keeps offsets while inside a Dag (via `MemoryRouter`).
## Screenrecords
#### Original
https://github.com/user-attachments/assets/f4eec01c-efe5-4e1a-9f2d-286633942a5e
#### Current
https://github.com/user-attachments/assets/f44ec326-7e56-4fb7-89be-6f555381dd5a
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [X] Yes (please specify the tool below)
Co-authored-by: [Claude Opus 5] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r3972464774
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
Shouldn't we know where in the grid's structure the task in question sits and which task groups are open or closed? Then all the rows are a fixed height so we can calculate the height offset. "center" is fine after that
The problem with storing a px top offset is that if the dag structure changes between versions or the expanded task groups change then it breaks again.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r4026062086
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
hey @bbovenzi! I believe I've fixed it here -> https://github.com/apache/airflow/pull/73123. Can you please take a look when you will have some time. thank you!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r3972413532
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
Yeah go for 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r3972464774
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
Shouldn't we know where in the grid's structure the task in question sits and which task groups are open or closed? Then all the rows are a fixed height so we can calculate the height offset. The problem with storing a px top offset is that if the dag structure changes between versions or the expanded task groups change then it breaks again.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #70536:
URL: https://github.com/apache/airflow/pull/70536#issuecomment-5781776172
Closing in favor of https://github.com/apache/airflow/pull/73123
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r4026827359
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
hey @bbovenzi ! Can you please take a look into the new approach -> https://github.com/apache/airflow/pull/73123. We don't store a raw pixel top-offset as the source of truth. On the remount that opening a task triggers, we restore the last scrollTop only to avoid the Grid jumping; and because that remount never changes the structure, the offset still lines up. If the structure does change (new version, or groups expanded/collapsed), the restored offset no longer matches, so we fall back to locating the selected task by id in the flattened rows and, since rows are fixed-height, use the index to center it — no reliance on the stored pixels. So the failure mode you flagged is handled by the index path.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "Ei-Sandi (via GitHub)" <gi...@apache.org>.
Ei-Sandi commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r3704867770
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
@bbovenzi Hi, I looked into the option, `rowVirtualizer.scrollToIndex()` won't work as we have to choose start, center, end or auto and we cannot get back the exact position for the task as when we clicked.
Is there any alternative would you like to suggest ?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r3675914159
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
We can just call `const { dagId } = useParams();`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r3949896585
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
@bbovenzi do you think we can try to move with the current approach?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi closed pull request #70536: Keep Grid scroll position when opening a task.
URL: https://github.com/apache/airflow/pull/70536
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r4026062086
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
hey @bbovenzi! I believe I've fixed it here -> https://github.com/apache/airflow/pull/73123. Can you please take a look when you will have some time. thank you!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r3976088713
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
Ill try to catch this changes and a bit refactor. thank you!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep Grid scroll position when opening a task. [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #70536:
URL: https://github.com/apache/airflow/pull/70536#discussion_r3972413532
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridScrollRestoration.ts:
##########
@@ -0,0 +1,103 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useLayoutEffect, useRef } from "react";
+import { useLocation } from "react-router-dom";
+
+/**
+ * Grid vertical scroll offset per ``dagId``, kept at module scope so it survives the
+ * remount that happens when navigating between the sibling ``<Dag>`` / ``<Run>`` /
+ * ``<Task>`` / ``<TaskInstance>`` routes — each mounts its own ``Grid``, which would
+ * otherwise start at ``scrollTop = 0``.
+ */
+const gridScrollOffsetByDagId = new Map<string, number>();
+
+// Matches a specific Dag (`/dags/<id>...`) but not the `/dags` list.
+const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u;
+
+export const extractDagIdFromPath = (pathname: string): string | undefined =>
+ DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId;
Review Comment:
Yeah go for 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org