Preserve active tabs during navigation#58881
Conversation
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. |
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
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
There was a problem hiding this comment.
I'm not found of this component. Doing comparison and hard redirect between renders we see a flickering.
Instead during navigation, before landing on the page, for instance before going to the 'TI' page with a default tab active and then flickering to a redirect for the persisted local storage. Can't we compute directly the correct final route before redirection (taking into account the persisted value in the storage), and then directly land on the appropriate page, removing that intermediate redirect.
This will prevent the flickering effect that is not great for UX, and also will prevent unnecessary request to be fired from the intermediate page that we actually do not need. (since we are directly being redirected to the storage persisted tab)
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.