Skip to content

Conversation

guan404ming
Copy link
Contributor

@guan404ming guan404ming commented May 14, 2025

Why

Currently the default dag list sorting isn’t by dag_display_name which is not really straightforward

How

Update the default sorting to alphabetically.


^ 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.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label May 14, 2025
kaxil
kaxil previously approved these changes May 14, 2025
@kaxil kaxil added this to the Airflow 3.0.2 milestone May 14, 2025
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, the default order is last_run_start_date. Which I think is expected because you want to see dags that recently run first.

Why do you think dag_id makes more sense by default ?

@Lee-W
Copy link
Member

Lee-W commented May 14, 2025

Not really, the default order is last_run_start_date. Which I think is expected because you want to see dags that recently run first.

Why do you think dag_id makes more sense by default ?

I don't have a strong opinion, but using last_run_start_date as the default order keeps reordering the dags and makes it a little hard for me to find the dag I want to find. But I guess I just accept it at some point

@pierrejeambrun
Copy link
Member

pierrejeambrun commented May 14, 2025

We can go for dag_display_name by default if this is preferable. But that's not where this is done in the code.

  const orderBy = sort ? `${sort.desc ? "-" : ""}${sort.id}` : "-last_run_start_date";

@kaxil kaxil dismissed their stale review May 14, 2025 13:18

Dissmissing my review since there might be a better place for this fix as Pierre mentioned

@Lee-W
Copy link
Member

Lee-W commented May 14, 2025

We can go for dag_display_name by default if this is preferable. But that's not where this is done in the code.

  const orderBy = sort ? `${sort.desc ? "-" : ""}${sort.id}` : "-last_run_start_date";

Yep, dag_display_name is definitely way better than dag_id.

@guan404ming guan404ming force-pushed the update-dag-list-sorting branch from fb846e7 to 1a22f67 Compare May 14, 2025 13:55
@guan404ming
Copy link
Contributor Author

Thanks for pointing out. I've updated to use dag_display_name to sort. Also fix the mistake in connection default sort.

@aritra24
Copy link
Collaborator

Is it perhaps better to make it configurable somewhere what you want to sort by? Different people might have different preferences of what to sort by.

@ashb
Copy link
Member

ashb commented May 14, 2025

It should be topological, falling back to the order it is added to the dag.

There is an existing bug for this

@ashb ashb closed this May 14, 2025
@ashb
Copy link
Member

ashb commented May 14, 2025

Sorry, totally misread this and thought it was about tasks

@ashb ashb reopened this May 14, 2025
@guan404ming
Copy link
Contributor Author

Make sense to make a config. I would try to implement it. Thanks!

@guan404ming guan404ming marked this pull request as draft May 15, 2025 03:09
@jedcunningham
Copy link
Member

Different people may want different sort order in the same instance too though. Maybe the sort choice should be sticky when you navigate away and back, instead of a global config.

@aritra24
Copy link
Collaborator

Yeah, that makes sense. We do that for a bunch of stuff already with graph view, etc. We can do the same thing here as well.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we can add follow up PR to persist the sorting preference in the localstorage.

@pierrejeambrun pierrejeambrun added the backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch label May 15, 2025
@guan404ming guan404ming marked this pull request as ready for review May 15, 2025 13:35
@guan404ming
Copy link
Contributor Author

Sure, I would open following PR for that. Thanks.

@pierrejeambrun pierrejeambrun merged commit ae9ae04 into apache:main May 15, 2025
117 checks passed
github-actions bot pushed a commit that referenced this pull request May 15, 2025
(cherry picked from commit ae9ae04)

Co-authored-by: Guan Ming(Wesley) Chiu <[email protected]>
Co-authored-by: pierrejeambrun <[email protected]>
Copy link

Backport successfully created: v3-0-test

Status Branch Result
v3-0-test PR Link

pierrejeambrun added a commit that referenced this pull request May 15, 2025
…0652)

(cherry picked from commit ae9ae04)

Co-authored-by: Guan Ming(Wesley) Chiu <[email protected]>
Co-authored-by: pierrejeambrun <[email protected]>
kaxil pushed a commit that referenced this pull request Jun 3, 2025
…0652)

(cherry picked from commit ae9ae04)

Co-authored-by: Guan Ming(Wesley) Chiu <[email protected]>
Co-authored-by: pierrejeambrun <[email protected]>
sanederchik pushed a commit to sanederchik/airflow that referenced this pull request Jun 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants