Repository navigation
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
Open
Akil-Dikshan wants to merge 1 commit into
Akil-Dikshan wants to merge 1 commit into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Draft: please mark ready for review only after
fix/cache-fetched-catalogmerges and this branch is rebased onto the then-currentupstream/main. A small conflict is expected in theneedsFullFetchregion — that PR restructures the body of the full-fetch block, this PR widens its condition.date-desc,date-ascandpullCount-asconly 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:
sortMergedPackagesalready had correct comparators for all three sorts. They simply never reached the full-fetch path thatpullCount-descand 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-descalready does today. That cost is not reduced by this PR —fix/cache-fetched-catalogcaches 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
Related Issue(s)
Relates to wso2/product-integrator#<ISSUE_NUMBER>
Changes Made
needsFullFetchto includedate-desc,date-ascandpullCount-asc, alongside the existingquery/name-asc/name-desc/pullCount-desc/keyword-filter conditions.skipFullFetch?: booleanflag onSearchParams, 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).fetchFiltersProgressivelyandfetchAllPackagesForFiltersnow passskipFullFetch: true, keeping them on the fast path. These two functions callsearchPackageswithdate-descon 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.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
SortOptionvalues now routed throughneedsFullFetch, the fast-path branch (andgetTotalHiddenCount's usage pattern) is only reachable viaskipFullFetch. Removing truly dead code paths would be a separate, surgical follow-up.Testing Performed
Test Environment
fetch, not a real browserTest Cases
npm test) — 6 suites, 106 tests (baseline: 6 suites, 102 tests)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-ascpage 1 returns the true oldest package, not the oldest within a buffer.date-descpage 1 returns the true newest package.pullCount-ascpage 1 returns a true zero-pull package from across the whole catalog.Confirmed all four new tests fail on the unpatched
needsFullFetchcondition (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
fetchFiltersProgressivelytests (request-count andonUpdateassertions) pass unchanged, confirmingskipFullFetchkeeps 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
npm run lintpasses)npm run format:checkpasses)npx tsc --noEmit)Documentation
skipFullFetchinsteadPerformance Impact
Details:
date-desc,date-ascandpullCount-ascnow pay the full count + batch fetch cost on every load, untilfix/cache-fetched-catalogmerges and this branch is rebased on top of it. This is a known, stated tradeoff for correctness — see Description.Breaking Changes
Additional Notes
fix/cache-fetched-catalogand should stay in draft until that merges and this branch is rebased onto the resultingupstream/main.Reviewer Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.