Skip to content

fix(rest-client): sort date-desc, date-asc and pullCount-asc over the full catalog - #66

Open
Akil-Dikshan wants to merge 1 commit into
wso2:mainfrom
Akil-Dikshan:fix/sort-full-catalog
Open

Akil-Dikshan wants to merge 1 commit into
wso2:mainfrom
Akil-Dikshan:fix/sort-full-catalog

Conversation

@Akil-Dikshan

Copy link
Copy Markdown
Contributor

Description

Draft: please mark ready for review only after fix/cache-fetched-catalog merges and this branch is rebased onto the then-current upstream/main. A small conflict is expected in the needsFullFetch region — that PR restructures the body of the full-fetch block, this PR widens its condition.

date-desc, date-asc and pullCount-asc only re-sorted the small fast-path buffer (page size + hidden-package count, ~110-150 items), not the full catalog (~836 packages). Results were locally correct but globally wrong — e.g. "Oldest First" showed the oldest item within the fetched buffer, not the true oldest package overall.

This is a routing fix, not a sort-logic fix: sortMergedPackages already had correct comparators for all three sorts. They simply never reached the full-fetch path that pullCount-desc and the name sorts already use.

Cost, stated plainly: on its own, this makes those three sorts pay the full count + batch fetch on every load, same as pullCount-desc already does today. That cost is not reduced by this PR — fix/cache-fetched-catalog caches the fetched catalog across page and sort changes within a session, which is why this PR is a draft until that one merges. No speedup number is claimed here, because none has been measured.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Related Issue(s)

Relates to wso2/product-integrator#<ISSUE_NUMBER>

Changes Made

  • Widened needsFullFetch to include date-desc, date-asc and pullCount-asc, alongside the existing query/name-asc/name-desc/pullCount-desc/keyword-filter conditions.
  • Added an internal skipFullFetch?: boolean flag on SearchParams, scoped only to these three sorts (it cannot suppress a full fetch needed for a query, a keyword filter, pullCount-desc, or the name sorts).
  • fetchFiltersProgressively and fetchAllPackagesForFilters now pass skipFullFetch: true, keeping them on the fast path. These two functions call searchPackages with date-desc on every page load to build filter facets, and don't need a globally correct order — without the flag, they'd trigger ~10 full fetches per load instead of small page-sized requests.
  • No changes to the sort comparators themselves, or to pullCount-desc/name-sort routing.

Known limitation, not fixed here: none of the sort comparators has a tie-break key, so items with equal date or equal pull count keep whatever order the merged catalog happened to have — which can vary between requests until the cache PR lands and stabilizes catalog order within a session.

Follow-up opportunity, not done here: with all six SortOption values now routed through needsFullFetch, the fast-path branch (and getTotalHiddenCount's usage pattern) is only reachable via skipFullFetch. Removing truly dead code paths would be a separate, surgical follow-up.

Testing Performed

Test Environment

  • Browser(s): N/A — verified via Jest with a mocked fetch, not a real browser
  • OS: Ubuntu
  • Node Version: 18.19.1

Test Cases

  • Unit tests pass (npm test) — 6 suites, 106 tests (baseline: 6 suites, 102 tests)
  • Integration tests pass (if applicable)
  • Manual testing completed
  • Tested on mobile devices
  • Tested on desktop browsers

New tests use a 150-item mocked catalog (larger than the old fast-path buffer) with the true oldest/lowest-pull items deliberately placed outside that buffer's range:

  • date-asc page 1 returns the true oldest package, not the oldest within a buffer.
  • date-desc page 1 returns the true newest package.
  • pullCount-asc page 1 returns a true zero-pull package from across the whole catalog.
  • Pagination continuity: paging through the full sorted array produces no duplicates and no gaps (every item appears exactly once across pages), and preserves correct global order across page boundaries.

Confirmed all four new tests fail on the unpatched needsFullFetch condition (reverted only that change, kept the tests): wrong leading item for date-asc/date-desc/pullCount-asc, and wrong cross-page order for the continuity test. Restored the fix afterward.

The two existing fetchFiltersProgressively tests (request-count and onUpdate assertions) pass unchanged, confirming skipFullFetch keeps that path's request pattern intact.

Screenshots

Before

Not applicable — this is a data-ordering/request-pattern change, not a visual one.

After

Not applicable.

Code Quality Checklist

  • Code follows project style guidelines (npm run lint passes)
  • Code is properly formatted (npm run format:check passes)
  • TypeScript compilation succeeds (npx tsc --noEmit)
  • No console.log statements (except console.warn/console.error)
  • Comments added for complex logic
  • Self-review completed

Documentation

  • README updated (if needed)
  • CHANGELOG updated (if needed)
  • Documentation added/updated (if needed)
  • JSDoc comments added for new functions — inline comment added on skipFullFetch instead

Performance Impact

  • No performance impact
  • Performance improved
  • Performance degraded (explain below)

Details: date-desc, date-asc and pullCount-asc now pay the full count + batch fetch cost on every load, until fix/cache-fetched-catalog merges and this branch is rebased on top of it. This is a known, stated tradeoff for correctness — see Description.

Breaking Changes

  • No breaking changes

Additional Notes

  • This PR depends on fix/cache-fetched-catalog and should stay in draft until that merges and this branch is rebased onto the resulting upstream/main.
  • Not run against the real Central API — timing/response-size claims are unverified.

Reviewer Checklist

  • Code reviewed
  • Tests reviewed
  • Documentation reviewed
  • No security concerns
  • Approved

By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8994d50a-a6b0-4e92-b9b5-62b0ebf76e7e
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

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