Skip to content

fix(order-progress): preserve solver ranking when deduplicating competition entries - #7901

Open
tenderdeve wants to merge 7 commits into
cowprotocol:developfrom
tenderdeve:fix/7879-solver-dedup-ranking
Open

fix(order-progress): preserve solver ranking when deduplicating competition entries#7901
tenderdeve wants to merge 7 commits into
cowprotocol:developfrom
tenderdeve:fix/7879-solver-dedup-ranking

Conversation

@tenderdeve

@tenderdeve tenderdeve commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Closes #7879

The solver-competition list was deduplicated through an object keyed by the raw entry.solver, then reversed. Updating an existing key keeps its original insertion slot, so a repeated winner could end up below another solver after the reverse, and legacy aliases (naive / naive-solve) were not collapsed.

Now the list is built with reduceRight — traversing highest to lowest rank, merging via mergeSolverData, and keeping the first occurrence per merged solverId. The highest-ranked entry per solver is retained and the winner stays at index 0. Dedup extracted into an exported buildSolverCompetition helper.

Tests cover repeated raw solver names, aliases that normalize to the same solverId, invalid-entry exclusion, and empty input.

Summary by CodeRabbit

  • Bug Fixes
    • Improved solver competition results displayed in the order progress bar.
    • Ensured results appear in the correct ranking order, with the highest-ranked solver first.
    • Removed duplicate solver entries, including entries using legacy aliases.
    • Filtered out invalid or incomplete competition results.
    • Preserved distinct valid solvers and handled missing competition data without displaying misleading results.

…tition entries

Deduplicate solver-competition entries by merged solverId while traversing
from highest to lowest rank, so the highest-ranked occurrence per solver is
retained and the winner stays at index 0. The previous object-keyed dedup kept
each key's original insertion slot, so reversing could sink the winner below
another solver, and duplicate legacy aliases (e.g. naive / naive-solve) were
not collapsed.
@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 320ad7e1-1d5a-4f92-8340-ff468858a59c

📥 Commits

Reviewing files that changed from the base of the PR and between 01d01b5 and 684aa6e.

📒 Files selected for processing (2)
  • apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.test.ts
  • apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.test.ts

Walkthrough

The order progress bar now builds solver competition results with rank-preserving traversal and canonical solver deduplication. New tests cover repeated solvers, legacy aliases, invalid entries, and missing competition data.

Changes

Solver competition ranking

Layer / File(s) Summary
Ranked competition builder
apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts
buildSolverCompetition merges valid entries while traversing ranked results, deduplicates by merged solverId, and supplies the ordered result to the progress bar.
Competition behavior tests
apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.test.ts
Tests verify winner ordering, alias deduplication, invalid-entry filtering, and empty results for undefined competition data.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: kernelwhisperer, jmg-duarte

Poem

I’m a rabbit who ranks through the queue,
Keeping the top solver in view.
Aliases unite, stale entries depart,
The winner stays first from the start.
Tests twitch their noses: “That’s right!”

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes preserving solver ranking during competition-entry deduplication.
Description check ✅ Passed The description clearly explains the problem, implementation, linked issue, and test coverage, although it omits the repository template headings and checklist.
Linked Issues check ✅ Passed The changes satisfy [#7879] by preserving ranking, deduplicating merged solver identities, excluding invalid entries, and covering the required cases with tests.
Out of Scope Changes check ✅ Passed The implementation and tests remain focused on solver-competition deduplication and ranking requirements from [#7879].
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@vercel

vercel Bot commented Jul 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
explorer-dev Ready Ready Preview Aug 7, 2026 9:59am
swap-dev Ready Ready Preview Aug 7, 2026 9:59am

Request Review

@elena-zh

Copy link
Copy Markdown
Contributor

@tenderdeve , the build failed with these errors
image
Could you pls fix it?

mergeSolverData always assigns a string solverId at runtime, but the
SolverCompetition type keeps it optional, so passing it to a Set<string>
failed typecheck (TS2345). Narrow it explicitly and keep entries without a
solverId instead of deduplicating them.
@tenderdeve

Copy link
Copy Markdown
Contributor Author

@elena-zh fixed in 01d01b5e9 — the two TS2345 errors came from calling seenSolverIds.has/.add with merged.solverId (typed string | undefined). Added an explicit solverId === undefined guard that keeps such entries as-is before the Set operations, so the type narrows to string. tsc --noEmit on cowswap-frontend now passes with 0 errors. Could you re-authorize the Vercel deploy so CI can re-run?

@elena-zh

elena-zh commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Will be reviewed after #7947 is merged

@elena-zh

elena-zh commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@tenderdeve , could you pls resolve conflicts here?

…dedup-ranking

# Conflicts:
#	apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts
@tenderdeve

Copy link
Copy Markdown
Contributor Author

@elena-zh conflicts resolved in 684aa6eb4 (merged latest develop).

The only conflict was in useOrderProgressBarProps.ts, where develop had meanwhile migrated mergeSolverData to be address-based (resolving the CMS solver purely via solversInfoByAddress, dropping the legacy name-based -solve lookup). I kept this PR's dedup fix but adapted it to that newer contract:

  • buildSolverCompetition now takes just (apiSolverCompetition, solversInfoByAddress) and calls develop's 2-arg mergeSolverData; the extra solversInfo map (and its now-removed useSolversInfo source) is gone.
  • The dedup still collapses by the merged solverId via reduceRight (winner kept at index 0) — so a solver's live + retired on-chain deployments (distinct addresses → same CMS solverId) collapse into one entry, which is the case that still matters after the address migration.
  • Updated the unit tests to exercise the address-based path (two addresses resolving to one solverId collapse; distinct solvers don't) instead of the retired name-alias case.

Changed files lint clean. I couldn't run the jest suite locally — the test's import graph pulls in bridgeProvider which crashes under jsdom here (unrelated to this change), so it's left for CI. The solverId-undefined TS2345 guard from the earlier build fix is unchanged. Ready for the re-review after #7947.

@elena-zh

elena-zh commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@tenderdeve , could you please clarify what to test here?

@tenderdeve

Copy link
Copy Markdown
Contributor Author

@elena-zh it's a display-only fix — it changes how the solver competition list in the order progress bar is deduplicated so the winning solver always ends up at index 0.

What broke: the old code keyed dedup on the raw entry.solver and did Object.values().reverse(). When the winning solver appeared more than once in the competition (a repeated winner, or a retired + live address that resolve to the same solver), overwriting an existing key kept its first insertion slot, so after the reverse the winner could render below another solver. The fix walks the entries highest→lowest rank with reduceRight, dedupes on the merged solverId, and keeps the first (highest-ranked) occurrence → winner at index 0.

Automated (primary): the logic lives in the extracted buildSolverCompetition and is covered by unit tests — repeated winner stays first, alias addresses collapse into one row, distinct solvers aren't merged, entries without solver/executedAmounts are dropped, empty input → [].

nx test cowswap-frontend --testPathPattern=useOrderProgressBarProps

Manual smoke: open any filled order's progress bar and confirm the solver list shows the winning solver at the top with no duplicate solver rows. For ordinary orders (no repeated or aliased solver in the competition) there's no visible change — this only corrects that edge case. Forcing the bug in the UI needs a competition payload where the winner's solver appears twice (or two addresses mapping to one solverId), which I can't reliably reproduce against the live backend, so the deterministic unit tests are the main coverage.

@elena-zh elena-zh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks

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.

Preserve solver ranking when deduplicating order-progress competition entries

2 participants