-
-
Notifications
You must be signed in to change notification settings - Fork 282
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?
Changes from 8 commits
ef8093f
fee4770
06046c4
219b825
53183d8
886237e
f3787b8
d27a1a0
968e01c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,15 @@ const MAX_PUBLIC_LIST_LIMIT = 200 | |
| const MAX_LIST_BULK_LIMIT = 200 | ||
| const MAX_LIST_TAKE = 1000 | ||
|
|
||
| const SORT_INDEXES = { | ||
| newest: 'by_active_created', | ||
| updated: 'by_active_updated', | ||
| name: 'by_active_name', | ||
| downloads: 'by_active_stats_downloads', | ||
| stars: 'by_active_stats_stars', | ||
| installs: 'by_active_stats_installs_all_time', | ||
| } as const | ||
|
|
||
| function isSkillVersionId( | ||
| value: Id<'skillVersions'> | null | undefined, | ||
| ): value is Id<'skillVersions'> { | ||
|
|
@@ -162,7 +171,8 @@ async function hardDeleteSkill( | |
| if (related._id === skill._id) continue | ||
| if (related.canonicalSkillId === skill._id || related.forkOf?.skillId === skill._id) { | ||
| await ctx.db.patch(related._id, { | ||
| canonicalSkillId: related.canonicalSkillId === skill._id ? undefined : related.canonicalSkillId, | ||
| canonicalSkillId: | ||
| related.canonicalSkillId === skill._id ? undefined : related.canonicalSkillId, | ||
| forkOf: related.forkOf?.skillId === skill._id ? undefined : related.forkOf, | ||
| updatedAt: now, | ||
| }) | ||
|
|
@@ -720,14 +730,28 @@ export const listPublicPage = query({ | |
| 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) | ||
|
Comment on lines
1042
to
1067
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] For 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: Prompt To Fix With AIThis 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. |
||
|
|
||
| // Build the public skill entries (fetch latestVersion + ownerHandle) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,25 @@ | ||
| { | ||
| "extends": "../tsconfig.json", | ||
| /* This TypeScript project config describes the environment that | ||
| * Convex functions run in and is used to typecheck them. | ||
| * You can modify it, but some settings are required to use Convex. | ||
| */ | ||
| "compilerOptions": { | ||
| /* These settings are not required by Convex and can be modified. */ | ||
| "allowJs": true, | ||
| "strict": true, | ||
| "moduleResolution": "Bundler", | ||
| "skipLibCheck": true | ||
| } | ||
| "jsx": "react-jsx", | ||
| "skipLibCheck": true, | ||
| "allowSyntheticDefaultImports": true, | ||
|
|
||
| /* These compiler options are required by Convex */ | ||
| "target": "ESNext", | ||
| "lib": ["ES2022", "dom"], | ||
| "forceConsistentCasingInFileNames": true, | ||
| "module": "ESNext", | ||
| "isolatedModules": true, | ||
| "noEmit": true | ||
| }, | ||
| "include": ["./**/*"], | ||
| "exclude": ["./_generated"] | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.