Preserve active tabs during navigation #58881
Conversation
RoyLee1224
requested review from
bbovenzi,
guan404ming,
pierrejeambrun,
ryanahamilton and
shubhamraj-git
as code owners
pierrejeambrun
added
the
backport-to-v3-1-test
There was a problem hiding this comment.
The problem with this approach is that it will not recall the tabs if we start switching from a TI to a DagRun back and forth. Only from TI to TI and Runs to Runs. Which is still a regression from 2.x.
In 2.x to achieve that, we would persist the active tabs in the local storage. To not have a multitude of storage entries. We could imagine 1 entry per 'type' of entities (that has specific tabs layout).
For instance, TI, Mapped TI, Task Group, Task, Run and DAG. And persist the active tab in the storage. Then everytime we redirect somewhere, read the appropriate value from the storage (depending on our current entity type), and default redirect to that if defined.
This will also have the advantage of persisting the tab accross different dags. (all groups will default to something, all Runs will default to something, etc.)
|
@pierrejeambrun Thanks for the review, the localStorage approach makes much more sense. I'll update to support that. 🚀 |
|
@RoyLee1224 are you still working on this PR? |
|
Hi, yes! I'll be wrapping up the remaining this week. |
RoyLee1224
force-pushed
the
feat/tab-memory
branch
from
d1d7ee8 to
620536e
Compare
|
@pierrejeambrun I have updated the PR to use the Entity-based LocalStorage approach you recommended. This fixes the persistence issues across different Dags and simplifies the logic significantly. Ready for your review! |
guan404ming
left a comment
guan404ming
left a comment
There was a problem hiding this comment.
Looks nice, just left some nit!
| return ""; | ||
| }; | ||
|
|
||
| export const getTaskAdditionalPath = (pathname: string): string => { |
There was a problem hiding this comment.
getTaskInstanceAdditionalPath validates against taskInstanceRoutes and plugin prefixes. Would it be worth adding a similar check and also with more tests?
| refetchInterval: isStatePending(taskInstance?.state) ? refetchInterval : false, | ||
| }); | ||
|
|
||
| const storageKey = |
There was a problem hiding this comment.
Maybe we should consider defaulting based on URL shape (e.g., checking for /mapped/ in the path) rather than waiting for API data, which may cause a redirect during the async call
6 participants
Related
closes: #57625
What
Refactors tab preservation logic to use Entity-based LocalStorage.
useTabMemoryhook to acceptstorageKeyparameter with options object patterngetTaskAdditionalPathfor Task page navigationgetTaskInstanceAdditionalPathto only handle TaskInstance pages (with/runs/)Screenshots
preserve_tabs.mp4
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.