-
Notifications
You must be signed in to change notification settings - Fork 16k
Preserve active tabs during navigation #58881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
| return (params: { | ||
| isGroup?: boolean; | ||
| isMapped?: boolean; | ||
| mapIndex?: string; | ||
| runId: string; | ||
| taskId: string; | ||
| }): string => { | ||
| const { isGroup = false, isMapped = false, mapIndex, runId, taskId } = params; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern seems odd. (maybe I missed something).
We allow passing isMapped, runId, taskId to the returned function. But those are actually overridden by the query params? Why do we allow to path them in the first place if those are not used?
Same for bellow hooks.
Precedence here is unclear, if we need both.
| const DAG_RUN_ROUTES = ["required_actions", "asset_events", "events", "code", "details"] as const; | ||
| const TASK_ROUTES = ["task_instances", "required_actions", "events"] as const; | ||
| const TASK_INSTANCE_ROUTES = [ | ||
| "required_actions", | ||
| "rendered_templates", | ||
| "xcom", | ||
| "asset_events", | ||
| "events", | ||
| "code", | ||
| "details", | ||
| ] as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to what we had before, can this come from the router definition ?
Having an hardcoded list here and in the router will make things harder to maintain. They can easily go out of sync.
Related
closes: #57625
What
TaskLink,GridButton,useNavigation).task_instancesfrom the allowed sub-routes list in test. It maps to "" instead of/task_instances.Screenshots
Preserve active tabs for Runs, Tasks, and TIs
run_and_task.mp4
Before:
/tasks/task/mapped/rendered_templates❌before.mp4
After:
/tasks/task/mapped✅after.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.