Skip to content

Return 0 from sortFunc when values are equal - #7169

Open
Ujjwal-Gowda wants to merge 1 commit into
pipe-cd:masterfrom
Ujjwal-Gowda:fix/web-sort-comparator-equal-values
Open

Return 0 from sortFunc when values are equal#7169
Ujjwal-Gowda wants to merge 1 commit into
pipe-cd:masterfrom
Ujjwal-Gowda:fix/web-sort-comparator-equal-values

Conversation

@Ujjwal-Gowda

Copy link
Copy Markdown

What this PR does:

sortFunc never returned 0. For equal values a > b is false, so it fell through to -1 in ASC and 1 in DESC, reporting both a < b and b < a for identical input.

Array.prototype.sort is only stable when the comparator returns 0 for equal elements, so equal entries were reordered instead of keeping their original order. With 12 entries where 11 share a name, the equal ones came out fully reversed. The result also varied with list length, because V8 switches sorting strategy for larger arrays.

This adds the a === b short-circuit, and adds web/src/utils/common.test.ts (the file did not exist before) covering ordering, equality, and the stability regression.

Why we need it:

This function sorts the piped and application lists in the four application forms and in the encrypt secret drawer, where duplicate names are normal. sortDateFunc delegates to sortFunc and had the same problem for equal timestamps.

The visible effect is that the dropdown order for same-named entries is unstable: it changes depending on how many items are in the list, so a user picking "the second api piped" is not picking a consistent thing.

Which issue(s) this PR fixes:

None — found while reading the application form code. Happy to open an issue first if that is preferred.

Does this PR introduce a user-facing change?:

  • How are users affected by this change: Piped and application dropdowns keep same-named entries in their original order instead of reversing them.
  • Is this breaking change: No.
  • How to migrate (if breaking change): N/A

sortFunc never returned 0. For equal values `a > b` is false, so it fell
through to -1 in ASC and 1 in DESC, reporting both a < b and b < a for
identical input.

Array.prototype.sort is only stable when the comparator returns 0 for
equal elements, so equal entries were reordered rather than keeping their
original order. With 12 entries where 11 share a name, the equal ones came
out fully reversed. The result also varied with list length because V8
changes sorting strategy for larger arrays.

This sorts the piped and application lists in the four application forms
and in the encrypt secret drawer, where duplicate names are normal.
sortDateFunc delegates to sortFunc and had the same problem for equal
timestamps.

Add web/src/utils/common.test.ts, which did not exist before, covering
ordering, equality, and the stability regression.

Signed-off-by: Ujjwal Gowda <crystalninja0123@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi @Ujjwal-Gowda, welcome to PipeCD and thanks for opening your first pull request!

We’re really happy to have you here

Before your PR gets merged, please check a few important things below.


Helpful resources


DCO Sign-off

All commits must include a Signed-off-by line to comply with the Developer Certificate of Origin (DCO).

In case you forget to sign-off your commit(s), follow these steps:

For the last commit:

git commit --amend --signoff
git push --force-with-lease

For multiple commits:

git rebase --signoff origin/master
git push --force-with-lease

Run checks locally

Before pushing updates, please run:

make check

This runs the same checks as CI and helps catch issues early.


💬 Need help?

If anything is unclear, feel free to ask in this PR or join us on the CNCF Slack in the #pipecd channel.
You can get your Slack invite from: https://communityinviter.com/apps/cloud-native/cncf

Thanks for contributing to PipeCD! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant