-
-
Notifications
You must be signed in to change notification settings - Fork 275
fix(skills): keep global sorting across pagination #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@knox-glorang is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
| export const listPublicPageV2 = query({ | ||
| args: { | ||
| paginationOpts: paginationOptsValidator, | ||
| sort: v.optional( | ||
| v.union( | ||
| v.literal('newest'), | ||
| v.literal('updated'), | ||
| v.literal('downloads'), | ||
| v.literal('installs'), | ||
| v.literal('stars'), | ||
| v.literal('name'), | ||
| ), | ||
| ), | ||
| dir: v.optional(v.union(v.literal('asc'), v.literal('desc'))), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| // Use the new index to filter out soft-deleted skills at query time. | ||
| const sort = args.sort ?? 'newest' | ||
| const dir = args.dir ?? (sort === 'name' ? 'asc' : 'desc') | ||
|
|
||
| // Use the index to filter out soft-deleted skills at query time. | ||
| // softDeletedAt === undefined means active (non-deleted) skills only. | ||
| const result = await paginator(ctx.db, schema) | ||
| .query('skills') | ||
| .withIndex('by_active_updated', (q) => q.eq('softDeletedAt', undefined)) | ||
| .order('desc') | ||
| .withIndex(SORT_INDEXES[sort], (q) => q.eq('softDeletedAt', undefined)) | ||
| .order(dir) | ||
| .paginate(args.paginationOpts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] listPublicPageV2 can return a different order than the client expects when sorting by stats, because the index tie-breaker fields don’t match the client-side comparator.
For downloads/stars/installs, the index is statsX, updatedAt but the UI’s results.sort only compares the primary stat (and doesn’t fall back to updatedAt / slug). This means within equal-stat groups you’ll get a stable-but-different ordering depending on whether results came from server pagination vs the client search path.
This matters when switching between search and non-search views or when many items share the same stat value (often 0). Consider aligning the server index/order with the UI’s full comparator (e.g., add deterministic secondary keys) or updating the client comparator to match the server ordering.
Also appears in: src/routes/skills/index.tsx:169-189
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.ts
Line: 730:755
Comment:
[P1] `listPublicPageV2` can return a different order than the client expects when sorting by stats, because the index tie-breaker fields don’t match the client-side comparator.
For `downloads`/`stars`/`installs`, the index is `statsX, updatedAt` but the UI’s `results.sort` only compares the primary stat (and doesn’t fall back to `updatedAt` / slug). This means within equal-stat groups you’ll get a stable-but-different ordering depending on whether results came from server pagination vs the client search path.
This matters when switching between search and non-search views or when many items share the same stat value (often 0). Consider aligning the server index/order with the UI’s full comparator (e.g., add deterministic secondary keys) or updating the client comparator to match the server ordering.
Also appears in: `src/routes/skills/index.tsx:169-189`
How can I resolve this? If you propose a fix, please make it concise.
What changed
listPublicPageV2defaultdirwith UI semantics (namedefaults toasc)Why
Sorting by stars/installs/downloads should apply to the full dataset, not only within the currently loaded page(s).
Test plan
UI verification (screenshot)
/skills?sort=stars&dir=descand confirm the top rows show the highest star counts.Related: #92
Greptile Overview
Greptile Summary
This PR updates the public skills listing to preserve global sorting across pagination by relying on server-side ordering (instead of re-sorting paginated results on the client). It also extends the Convex
listPublicPageV2query to acceptsort/dirparameters and adds newskillsindexes to support those sort modes efficiently, plus updates the UI and unit tests to pass the new defaults.Key touchpoints:
convex/schema.ts: adds severalby_active_*indexes to filter out soft-deleted skills while sorting.convex/skills.ts: introducesSORT_INDEXESand uses it inlistPublicPageV2with configurablesort/dir.src/routes/skills/index.tsx: passessort/dirto the paginated query and avoids client-side sorting when not searching.Confidence Score: 4/5
(2/5) Greptile learns from your feedback when you react with thumbs up/down!
Context used:
dashboard- AGENTS.md (source)