From b4c66974913ac456c1192a55d8e7ce97347700ce Mon Sep 17 00:00:00 2001 From: Scott Cazan Date: Thu, 22 Jan 2026 23:05:14 +0100 Subject: [PATCH 1/7] Add in cursor pagination --- .../src/services/profile/listProfileUsers.ts | 84 ++++++- .../routers/profile/users/listUsers.test.ts | 234 +++++++++++++++--- .../src/routers/profile/users/listUsers.ts | 14 +- 3 files changed, 290 insertions(+), 42 deletions(-) diff --git a/packages/common/src/services/profile/listProfileUsers.ts b/packages/common/src/services/profile/listProfileUsers.ts index 71b5a1a74..f72ef3fa3 100644 --- a/packages/common/src/services/profile/listProfileUsers.ts +++ b/packages/common/src/services/profile/listProfileUsers.ts @@ -3,7 +3,12 @@ import { profileUsers, profiles, users } from '@op/db/schema'; import type { User } from '@op/supabase/lib'; import { assertAccess, permission } from 'access-zones'; -import { SortDir } from '../../utils/db'; +import { + decodeCursor, + encodeCursor, + getCursorCondition, + type SortDir, +} from '../../utils/db'; import { UnauthorizedError } from '../../utils/error'; import { getProfileAccessUser } from '../access'; import { assertProfile } from '../assert'; @@ -14,8 +19,14 @@ import type { export type ProfileUserOrderBy = 'name' | 'email' | 'role'; +type PaginatedProfileUsersResult = { + items: ProfileUserWithRelations[]; + next: string | null; + hasMore: boolean; +}; + /** - * List all members of a profile + * List all members of a profile with cursor-based pagination */ export const listProfileUsers = async ({ profileId, @@ -23,13 +34,17 @@ export const listProfileUsers = async ({ orderBy = 'name', dir = 'asc', query, + cursor, + limit = 50, }: { profileId: string; user: User; orderBy?: ProfileUserOrderBy; dir?: SortDir; query?: string; -}): Promise => { + cursor?: string | null; + limit?: number; +}): Promise => { const [profileAccessUser] = await Promise.all([ getProfileAccessUser({ user, profileId }), assertProfile(profileId), @@ -66,11 +81,31 @@ export const listProfileUsers = async ({ })() : undefined; - const whereClause = searchFilter - ? and(eq(profileUsers.profileId, profileId), searchFilter) - : eq(profileUsers.profileId, profileId); - - // Fetch all profile users with their roles and user profiles + // Determine the column to use for cursor-based pagination + // For role sorting, we use email as the cursor column since role values can be duplicated + const orderByColumn = + orderBy === 'email' ? profileUsers.email : profileUsers.email; + + // Build cursor condition for pagination + // Always use id as tiebreaker for stable pagination + const cursorCondition = cursor + ? getCursorCondition({ + column: orderByColumn, + tieBreakerColumn: profileUsers.id, + cursor: decodeCursor<{ value: string; id?: string }>(cursor), + direction: dir, + }) + : undefined; + + // Combine all conditions + const baseCondition = eq(profileUsers.profileId, profileId); + const conditions = [baseCondition, searchFilter, cursorCondition].filter( + Boolean, + ); + const whereClause = conditions.length > 1 ? and(...conditions) : baseCondition; + + // Fetch profile users with their roles and user profiles + // Request one extra to check if there are more results const profileUserResults = await db._query.profileUsers.findMany({ where: whereClause, with: { @@ -103,19 +138,28 @@ export const listProfileUsers = async ({ ORDER BY ar.name LIMIT 1 )`; - return [orderFn(roleNameSubquery)]; + // Add email as secondary sort for consistent cursor pagination + return [orderFn(roleNameSubquery), orderFn(table.email)]; } if (orderBy === 'email') { return [orderFn(table.email)]; } - // Default to name - return [orderFn(table.name)]; + // Default to name, with email as secondary for consistent ordering + return [orderFn(table.name), orderFn(table.email)]; }, + limit: limit + 1, }); - return profileUserResults.map((result) => { + // Check if there are more results + const hasMore = profileUserResults.length > limit; + const resultItems = hasMore + ? profileUserResults.slice(0, limit) + : profileUserResults; + + // Transform results + const items = resultItems.map((result) => { const { serviceUser, roles, ...baseProfileUser } = result as ProfileUserQueryResult; const userProfile = serviceUser?.profile; @@ -128,4 +172,20 @@ export const listProfileUsers = async ({ roles: roles.map((roleJunction) => roleJunction.accessRole), }; }); + + // Build next cursor from last item + const lastItem = resultItems[resultItems.length - 1]; + const nextCursor = + hasMore && lastItem + ? encodeCursor<{ value: string; id?: string }>({ + value: lastItem.email, + id: lastItem.id, + }) + : null; + + return { + items, + next: nextCursor, + hasMore, + }; }; diff --git a/services/api/src/routers/profile/users/listUsers.test.ts b/services/api/src/routers/profile/users/listUsers.test.ts index e4a91ca1a..7a6c65748 100644 --- a/services/api/src/routers/profile/users/listUsers.test.ts +++ b/services/api/src/routers/profile/users/listUsers.test.ts @@ -39,10 +39,10 @@ describe.concurrent('profile.users.listUsers', () => { profileId: profile.id, }); - expect(result).toHaveLength(3); - expect(result.map((u) => u.email)).toContain(adminUser.email); - expect(result.map((u) => u.email)).toContain(memberUsers[0]?.email); - expect(result.map((u) => u.email)).toContain(memberUsers[1]?.email); + expect(result.items).toHaveLength(3); + expect(result.items.map((u) => u.email)).toContain(adminUser.email); + expect(result.items.map((u) => u.email)).toContain(memberUsers[0]?.email); + expect(result.items.map((u) => u.email)).toContain(memberUsers[1]?.email); }); it('should return users with their roles', async ({ @@ -61,7 +61,7 @@ describe.concurrent('profile.users.listUsers', () => { profileId: profile.id, }); - const admin = result.find((u) => u.email === adminUser.email); + const admin = result.items.find((u) => u.email === adminUser.email); expect(admin).toBeDefined(); expect(admin?.roles).toHaveLength(1); expect(admin?.roles[0]?.name).toBe(ROLES.ADMIN.name); @@ -133,14 +133,14 @@ describe.concurrent('profile.users.listUsers', () => { dir: 'desc', }); - expect(resultAsc).toHaveLength(3); - expect(resultDesc).toHaveLength(3); + expect(resultAsc.items).toHaveLength(3); + expect(resultDesc.items).toHaveLength(3); // Test data creates names like "Test Admin User" and "Test Member User" // "Test Admin User" < "Test Member User" alphabetically (A < M after "Test ") // So admin should be first in ASC order and last in DESC order - expect(resultAsc[0]?.email).toBe(adminUser.email); - expect(resultDesc[resultDesc.length - 1]?.email).toBe(adminUser.email); + expect(resultAsc.items[0]?.email).toBe(adminUser.email); + expect(resultDesc.items[resultDesc.items.length - 1]?.email).toBe(adminUser.email); }); it('should reverse order when switching between asc and desc for email', async ({ @@ -167,12 +167,12 @@ describe.concurrent('profile.users.listUsers', () => { dir: 'desc', }); - expect(resultAsc).toHaveLength(3); - expect(resultDesc).toHaveLength(3); + expect(resultAsc.items).toHaveLength(3); + expect(resultDesc.items).toHaveLength(3); // Emails are unique, so ascending and descending should be exact reverses - const ascEmails = resultAsc.map((u) => u.email); - const descEmails = resultDesc.map((u) => u.email); + const ascEmails = resultAsc.items.map((u) => u.email); + const descEmails = resultDesc.items.map((u) => u.email); expect(ascEmails).toEqual([...descEmails].reverse()); }); @@ -200,13 +200,13 @@ describe.concurrent('profile.users.listUsers', () => { dir: 'desc', }); - expect(resultAsc).toHaveLength(3); - expect(resultDesc).toHaveLength(3); + expect(resultAsc.items).toHaveLength(3); + expect(resultDesc.items).toHaveLength(3); // "Admin" comes before "Member" alphabetically // So admin should be first in ASC order and last in DESC order - expect(resultAsc[0]?.email).toBe(adminUser.email); - expect(resultDesc[resultDesc.length - 1]?.email).toBe(adminUser.email); + expect(resultAsc.items[0]?.email).toBe(adminUser.email); + expect(resultDesc.items[resultDesc.items.length - 1]?.email).toBe(adminUser.email); }); }); @@ -227,7 +227,7 @@ describe.concurrent('profile.users.listUsers', () => { profileId: profile.id, }); - expect(result).toHaveLength(3); + expect(result.items).toHaveLength(3); }); it('should filter users by name match', async ({ @@ -249,8 +249,8 @@ describe.concurrent('profile.users.listUsers', () => { query: 'Admin', }); - expect(result).toHaveLength(1); - expect(result[0]?.email).toBe(adminUser.email); + expect(result.items).toHaveLength(1); + expect(result.items[0]?.email).toBe(adminUser.email); }); it('should filter users by email match', async ({ @@ -272,9 +272,9 @@ describe.concurrent('profile.users.listUsers', () => { query: '-member-', }); - expect(result).toHaveLength(2); - expect(result.map((u) => u.email)).toContain(memberUsers[0]?.email); - expect(result.map((u) => u.email)).toContain(memberUsers[1]?.email); + expect(result.items).toHaveLength(2); + expect(result.items.map((u) => u.email)).toContain(memberUsers[0]?.email); + expect(result.items.map((u) => u.email)).toContain(memberUsers[1]?.email); }); it('should be case-insensitive', async ({ task, onTestFinished }) => { @@ -292,8 +292,8 @@ describe.concurrent('profile.users.listUsers', () => { query: 'admin', }); - expect(result).toHaveLength(1); - expect(result[0]?.email).toBe(adminUser.email); + expect(result.items).toHaveLength(1); + expect(result.items[0]?.email).toBe(adminUser.email); }); it('should return empty array when no matches found', async ({ @@ -313,7 +313,7 @@ describe.concurrent('profile.users.listUsers', () => { query: 'nonexistent-user-xyz', }); - expect(result).toHaveLength(0); + expect(result.items).toHaveLength(0); }); it('should reject queries shorter than 2 characters', async ({ @@ -357,10 +357,188 @@ describe.concurrent('profile.users.listUsers', () => { dir: 'desc', }); - expect(result).toHaveLength(2); + expect(result.items).toHaveLength(2); // Verify results are sorted descending by email - const emails = result.map((u) => u.email); + const emails = result.items.map((u) => u.email); expect(emails).toEqual([...emails].sort().reverse()); }); }); + + describe('pagination', () => { + it('should return paginated response with items, next cursor, and hasMore', async ({ + task, + onTestFinished, + }) => { + const testData = new TestProfileUserDataManager(task.id, onTestFinished); + const { profile, adminUser } = await testData.createProfile({ + users: { admin: 1, member: 2 }, + }); + + const { session } = await createIsolatedSession(adminUser.email); + const caller = createCaller(await createTestContextWithSession(session)); + + const result = await caller.listUsers({ + profileId: profile.id, + limit: 2, + }); + + expect(result.items).toHaveLength(2); + expect(result.hasMore).toBe(true); + expect(result.next).toBeTruthy(); + }); + + it('should return hasMore=false when all results fit in limit', async ({ + task, + onTestFinished, + }) => { + const testData = new TestProfileUserDataManager(task.id, onTestFinished); + const { profile, adminUser } = await testData.createProfile({ + users: { admin: 1, member: 2 }, + }); + + const { session } = await createIsolatedSession(adminUser.email); + const caller = createCaller(await createTestContextWithSession(session)); + + const result = await caller.listUsers({ + profileId: profile.id, + limit: 10, + }); + + expect(result.items).toHaveLength(3); + expect(result.hasMore).toBe(false); + expect(result.next).toBeNull(); + }); + + it('should paginate through all results using cursor', async ({ + task, + onTestFinished, + }) => { + const testData = new TestProfileUserDataManager(task.id, onTestFinished); + const { profile, adminUser, memberUsers } = await testData.createProfile({ + users: { admin: 1, member: 4 }, + }); + + const { session } = await createIsolatedSession(adminUser.email); + const caller = createCaller(await createTestContextWithSession(session)); + + // First page + const page1 = await caller.listUsers({ + profileId: profile.id, + limit: 2, + orderBy: 'email', + dir: 'asc', + }); + + expect(page1.items).toHaveLength(2); + expect(page1.hasMore).toBe(true); + expect(page1.next).toBeTruthy(); + + // Second page using cursor + const page2 = await caller.listUsers({ + profileId: profile.id, + limit: 2, + cursor: page1.next!, + orderBy: 'email', + dir: 'asc', + }); + + expect(page2.items).toHaveLength(2); + expect(page2.hasMore).toBe(true); + + // Third page - last page + const page3 = await caller.listUsers({ + profileId: profile.id, + limit: 2, + cursor: page2.next!, + orderBy: 'email', + dir: 'asc', + }); + + expect(page3.items).toHaveLength(1); + expect(page3.hasMore).toBe(false); + expect(page3.next).toBeNull(); + + // Verify all users were returned across pages (no duplicates) + const allEmails = [ + ...page1.items.map((u) => u.email), + ...page2.items.map((u) => u.email), + ...page3.items.map((u) => u.email), + ]; + expect(allEmails).toHaveLength(5); + expect(allEmails).toContain(adminUser.email); + memberUsers.forEach((m) => { + expect(allEmails).toContain(m.email); + }); + }); + + it('should work with search and pagination combined', async ({ + task, + onTestFinished, + }) => { + const testData = new TestProfileUserDataManager(task.id, onTestFinished); + const { profile, adminUser, memberUsers } = await testData.createProfile({ + users: { admin: 1, member: 4 }, + }); + + const { session } = await createIsolatedSession(adminUser.email); + const caller = createCaller(await createTestContextWithSession(session)); + + // Search for "member" with pagination + const page1 = await caller.listUsers({ + profileId: profile.id, + query: 'member', + limit: 2, + orderBy: 'email', + dir: 'asc', + }); + + expect(page1.items).toHaveLength(2); + expect(page1.hasMore).toBe(true); + + // Second page + const page2 = await caller.listUsers({ + profileId: profile.id, + query: 'member', + limit: 2, + cursor: page1.next!, + orderBy: 'email', + dir: 'asc', + }); + + expect(page2.items).toHaveLength(2); + expect(page2.hasMore).toBe(false); + + // All 4 members returned, admin filtered out + const allEmails = [ + ...page1.items.map((u) => u.email), + ...page2.items.map((u) => u.email), + ]; + expect(allEmails).toHaveLength(4); + expect(allEmails).not.toContain(adminUser.email); + memberUsers.forEach((m) => { + expect(allEmails).toContain(m.email); + }); + }); + + it('should default to limit of 50 when not specified', async ({ + task, + onTestFinished, + }) => { + const testData = new TestProfileUserDataManager(task.id, onTestFinished); + const { profile, adminUser } = await testData.createProfile({ + users: { admin: 1, member: 2 }, + }); + + const { session } = await createIsolatedSession(adminUser.email); + const caller = createCaller(await createTestContextWithSession(session)); + + const result = await caller.listUsers({ + profileId: profile.id, + }); + + // With only 3 users, all should be returned + expect(result.items).toHaveLength(3); + expect(result.hasMore).toBe(false); + }); + }); }); diff --git a/services/api/src/routers/profile/users/listUsers.ts b/services/api/src/routers/profile/users/listUsers.ts index 3bc431412..e7f48b696 100644 --- a/services/api/src/routers/profile/users/listUsers.ts +++ b/services/api/src/routers/profile/users/listUsers.ts @@ -14,13 +14,21 @@ export const listUsersRouter = router({ .object({ profileId: z.uuid(), query: z.string().min(2).optional(), + cursor: z.string().nullish(), + limit: z.number().min(1).max(100).optional(), }) .merge(profileUserSortable), ) - .output(z.array(profileUserEncoder)) + .output( + z.object({ + items: z.array(profileUserEncoder), + next: z.string().nullable(), + hasMore: z.boolean(), + }), + ) .query(async ({ ctx, input }) => { const { user } = ctx; - const { profileId, orderBy, dir, query } = input; + const { profileId, orderBy, dir, query, cursor, limit } = input; return listProfileUsers({ profileId, @@ -28,6 +36,8 @@ export const listUsersRouter = router({ orderBy, dir, query, + cursor, + limit, }); }), }); From e90aa1e46f4e2422ce8ba11c495cc15eddea75d1 Mon Sep 17 00:00:00 2001 From: Scott Cazan Date: Thu, 22 Jan 2026 23:11:43 +0100 Subject: [PATCH 2/7] Fix format --- packages/common/src/services/profile/listProfileUsers.ts | 5 +++-- services/api/src/routers/profile/users/listUsers.test.ts | 8 ++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/common/src/services/profile/listProfileUsers.ts b/packages/common/src/services/profile/listProfileUsers.ts index f72ef3fa3..015197b9d 100644 --- a/packages/common/src/services/profile/listProfileUsers.ts +++ b/packages/common/src/services/profile/listProfileUsers.ts @@ -4,10 +4,10 @@ import type { User } from '@op/supabase/lib'; import { assertAccess, permission } from 'access-zones'; import { + type SortDir, decodeCursor, encodeCursor, getCursorCondition, - type SortDir, } from '../../utils/db'; import { UnauthorizedError } from '../../utils/error'; import { getProfileAccessUser } from '../access'; @@ -102,7 +102,8 @@ export const listProfileUsers = async ({ const conditions = [baseCondition, searchFilter, cursorCondition].filter( Boolean, ); - const whereClause = conditions.length > 1 ? and(...conditions) : baseCondition; + const whereClause = + conditions.length > 1 ? and(...conditions) : baseCondition; // Fetch profile users with their roles and user profiles // Request one extra to check if there are more results diff --git a/services/api/src/routers/profile/users/listUsers.test.ts b/services/api/src/routers/profile/users/listUsers.test.ts index 7a6c65748..c75e4696a 100644 --- a/services/api/src/routers/profile/users/listUsers.test.ts +++ b/services/api/src/routers/profile/users/listUsers.test.ts @@ -140,7 +140,9 @@ describe.concurrent('profile.users.listUsers', () => { // "Test Admin User" < "Test Member User" alphabetically (A < M after "Test ") // So admin should be first in ASC order and last in DESC order expect(resultAsc.items[0]?.email).toBe(adminUser.email); - expect(resultDesc.items[resultDesc.items.length - 1]?.email).toBe(adminUser.email); + expect(resultDesc.items[resultDesc.items.length - 1]?.email).toBe( + adminUser.email, + ); }); it('should reverse order when switching between asc and desc for email', async ({ @@ -206,7 +208,9 @@ describe.concurrent('profile.users.listUsers', () => { // "Admin" comes before "Member" alphabetically // So admin should be first in ASC order and last in DESC order expect(resultAsc.items[0]?.email).toBe(adminUser.email); - expect(resultDesc.items[resultDesc.items.length - 1]?.email).toBe(adminUser.email); + expect(resultDesc.items[resultDesc.items.length - 1]?.email).toBe( + adminUser.email, + ); }); }); From 464c3dc5b463abece573dd6911c9c461b9cb69cf Mon Sep 17 00:00:00 2001 From: Scott Cazan Date: Thu, 22 Jan 2026 23:18:05 +0100 Subject: [PATCH 3/7] Change default limit --- packages/common/src/services/profile/listProfileUsers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/common/src/services/profile/listProfileUsers.ts b/packages/common/src/services/profile/listProfileUsers.ts index 015197b9d..63891efe1 100644 --- a/packages/common/src/services/profile/listProfileUsers.ts +++ b/packages/common/src/services/profile/listProfileUsers.ts @@ -35,7 +35,7 @@ export const listProfileUsers = async ({ dir = 'asc', query, cursor, - limit = 50, + limit = 25, }: { profileId: string; user: User; From a0e6b08c92fe5eb6735f200149b658a2b46faa19 Mon Sep 17 00:00:00 2001 From: Scott Cazan Date: Thu, 22 Jan 2026 23:28:04 +0100 Subject: [PATCH 4/7] Update tests --- .../routers/profile/users/listUsers.test.ts | 236 +++++++++++++----- 1 file changed, 167 insertions(+), 69 deletions(-) diff --git a/services/api/src/routers/profile/users/listUsers.test.ts b/services/api/src/routers/profile/users/listUsers.test.ts index c75e4696a..51d521785 100644 --- a/services/api/src/routers/profile/users/listUsers.test.ts +++ b/services/api/src/routers/profile/users/listUsers.test.ts @@ -413,7 +413,7 @@ describe.concurrent('profile.users.listUsers', () => { expect(result.next).toBeNull(); }); - it('should paginate through all results using cursor', async ({ + it('should paginate through all results using cursor with no duplicates', async ({ task, onTestFinished, }) => { @@ -425,54 +425,49 @@ describe.concurrent('profile.users.listUsers', () => { const { session } = await createIsolatedSession(adminUser.email); const caller = createCaller(await createTestContextWithSession(session)); - // First page - const page1 = await caller.listUsers({ - profileId: profile.id, - limit: 2, - orderBy: 'email', - dir: 'asc', - }); - - expect(page1.items).toHaveLength(2); - expect(page1.hasMore).toBe(true); - expect(page1.next).toBeTruthy(); - - // Second page using cursor - const page2 = await caller.listUsers({ - profileId: profile.id, - limit: 2, - cursor: page1.next!, - orderBy: 'email', - dir: 'asc', - }); - - expect(page2.items).toHaveLength(2); - expect(page2.hasMore).toBe(true); + // Collect all emails across pages + const allEmails: string[] = []; + let cursor: string | null | undefined; + let pageCount = 0; - // Third page - last page - const page3 = await caller.listUsers({ - profileId: profile.id, - limit: 2, - cursor: page2.next!, - orderBy: 'email', - dir: 'asc', - }); + do { + const page = await caller.listUsers({ + profileId: profile.id, + limit: 2, + cursor: cursor ?? undefined, + orderBy: 'email', + dir: 'asc', + }); + + allEmails.push(...page.items.map((u) => u.email)); + cursor = page.next; + pageCount++; + + // Safety check to prevent infinite loops + if (pageCount > 10) { + throw new Error('Too many pages - possible infinite loop'); + } + } while (cursor); + + // Verify we got all 5 items + expect(allEmails).toHaveLength(5); - expect(page3.items).toHaveLength(1); - expect(page3.hasMore).toBe(false); - expect(page3.next).toBeNull(); + // Verify no duplicates + const uniqueEmails = new Set(allEmails); + expect(uniqueEmails.size).toBe(5); - // Verify all users were returned across pages (no duplicates) - const allEmails = [ - ...page1.items.map((u) => u.email), - ...page2.items.map((u) => u.email), - ...page3.items.map((u) => u.email), - ]; - expect(allEmails).toHaveLength(5); + // Verify all expected users are present expect(allEmails).toContain(adminUser.email); memberUsers.forEach((m) => { expect(allEmails).toContain(m.email); }); + + // Verify correct ascending order across all pages + const sortedEmails = [...allEmails].sort(); + expect(allEmails).toEqual(sortedEmails); + + // Verify we needed 3 pages (2 + 2 + 1) + expect(pageCount).toBe(3); }); it('should work with search and pagination combined', async ({ @@ -488,43 +483,33 @@ describe.concurrent('profile.users.listUsers', () => { const caller = createCaller(await createTestContextWithSession(session)); // Search for "member" with pagination - const page1 = await caller.listUsers({ - profileId: profile.id, - query: 'member', - limit: 2, - orderBy: 'email', - dir: 'asc', - }); - - expect(page1.items).toHaveLength(2); - expect(page1.hasMore).toBe(true); + const allEmails: string[] = []; + let cursor: string | null | undefined; - // Second page - const page2 = await caller.listUsers({ - profileId: profile.id, - query: 'member', - limit: 2, - cursor: page1.next!, - orderBy: 'email', - dir: 'asc', - }); + do { + const page = await caller.listUsers({ + profileId: profile.id, + query: 'member', + limit: 2, + cursor: cursor ?? undefined, + orderBy: 'email', + dir: 'asc', + }); - expect(page2.items).toHaveLength(2); - expect(page2.hasMore).toBe(false); + allEmails.push(...page.items.map((u) => u.email)); + cursor = page.next; + } while (cursor); // All 4 members returned, admin filtered out - const allEmails = [ - ...page1.items.map((u) => u.email), - ...page2.items.map((u) => u.email), - ]; expect(allEmails).toHaveLength(4); + expect(new Set(allEmails).size).toBe(4); // No duplicates expect(allEmails).not.toContain(adminUser.email); memberUsers.forEach((m) => { expect(allEmails).toContain(m.email); }); }); - it('should default to limit of 50 when not specified', async ({ + it('should return all results when no limit specified', async ({ task, onTestFinished, }) => { @@ -540,9 +525,122 @@ describe.concurrent('profile.users.listUsers', () => { profileId: profile.id, }); - // With only 3 users, all should be returned + // With only 3 users (less than default limit), all should be returned expect(result.items).toHaveLength(3); expect(result.hasMore).toBe(false); }); + + it('should throw error for invalid cursor', async ({ + task, + onTestFinished, + }) => { + const testData = new TestProfileUserDataManager(task.id, onTestFinished); + const { profile, adminUser } = await testData.createProfile({ + users: { admin: 1 }, + }); + + const { session } = await createIsolatedSession(adminUser.email); + const caller = createCaller(await createTestContextWithSession(session)); + + await expect( + caller.listUsers({ + profileId: profile.id, + limit: 10, + cursor: 'invalid-cursor', + }), + ).rejects.toThrow(); + }); + + it('should paginate correctly when ordering by name', async ({ + task, + onTestFinished, + }) => { + const testData = new TestProfileUserDataManager(task.id, onTestFinished); + const { profile, adminUser } = await testData.createProfile({ + users: { admin: 1, member: 4 }, + }); + + const { session } = await createIsolatedSession(adminUser.email); + const caller = createCaller(await createTestContextWithSession(session)); + + // Collect all names across pages, ordered by name ascending + const allNames: (string | null)[] = []; + let cursor: string | null | undefined; + let pageCount = 0; + + do { + const page = await caller.listUsers({ + profileId: profile.id, + limit: 2, + cursor: cursor ?? undefined, + orderBy: 'name', + dir: 'asc', + }); + + allNames.push(...page.items.map((u) => u.name)); + cursor = page.next; + pageCount++; + + if (pageCount > 10) { + throw new Error('Too many pages - possible infinite loop'); + } + } while (cursor); + + // Verify we got all 5 items + expect(allNames).toHaveLength(5); + + // Verify no duplicates (by checking unique count matches total) + const uniqueNames = new Set(allNames); + expect(uniqueNames.size).toBe(5); + + // Verify correct alphabetical order + const sortedNames = [...allNames].sort((a, b) => + (a ?? '').localeCompare(b ?? ''), + ); + expect(allNames).toEqual(sortedNames); + }); + + it('should paginate correctly when ordering by role', async ({ + task, + onTestFinished, + }) => { + const testData = new TestProfileUserDataManager(task.id, onTestFinished); + const { profile, adminUser } = await testData.createProfile({ + users: { admin: 1, member: 4 }, + }); + + const { session } = await createIsolatedSession(adminUser.email); + const caller = createCaller(await createTestContextWithSession(session)); + + // Collect all users across pages, ordered by role ascending + const allEmails: string[] = []; + let cursor: string | null | undefined; + let pageCount = 0; + + do { + const page = await caller.listUsers({ + profileId: profile.id, + limit: 2, + cursor: cursor ?? undefined, + orderBy: 'role', + dir: 'asc', + }); + + allEmails.push(...page.items.map((u) => u.email)); + cursor = page.next; + pageCount++; + + if (pageCount > 10) { + throw new Error('Too many pages - possible infinite loop'); + } + } while (cursor); + + // Verify we got all 5 items with no duplicates + expect(allEmails).toHaveLength(5); + expect(new Set(allEmails).size).toBe(5); + + // Admin should be first (A < M alphabetically) + expect(allEmails[0]).toBe(adminUser.email); + }); }); }); From d3eb581118c29dc80f638921326524d98bdfa4ee Mon Sep 17 00:00:00 2001 From: Scott Cazan Date: Thu, 22 Jan 2026 23:33:26 +0100 Subject: [PATCH 5/7] Use generic type for pagination --- .../common/src/services/profile/listProfileUsers.ts | 13 +++---------- packages/common/src/utils/db.ts | 7 +++++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/common/src/services/profile/listProfileUsers.ts b/packages/common/src/services/profile/listProfileUsers.ts index 63891efe1..e4b73a1c0 100644 --- a/packages/common/src/services/profile/listProfileUsers.ts +++ b/packages/common/src/services/profile/listProfileUsers.ts @@ -4,6 +4,7 @@ import type { User } from '@op/supabase/lib'; import { assertAccess, permission } from 'access-zones'; import { + type PaginatedResult, type SortDir, decodeCursor, encodeCursor, @@ -19,12 +20,6 @@ import type { export type ProfileUserOrderBy = 'name' | 'email' | 'role'; -type PaginatedProfileUsersResult = { - items: ProfileUserWithRelations[]; - next: string | null; - hasMore: boolean; -}; - /** * List all members of a profile with cursor-based pagination */ @@ -44,7 +39,7 @@ export const listProfileUsers = async ({ query?: string; cursor?: string | null; limit?: number; -}): Promise => { +}): Promise> => { const [profileAccessUser] = await Promise.all([ getProfileAccessUser({ user, profileId }), assertProfile(profileId), @@ -155,9 +150,7 @@ export const listProfileUsers = async ({ // Check if there are more results const hasMore = profileUserResults.length > limit; - const resultItems = hasMore - ? profileUserResults.slice(0, limit) - : profileUserResults; + const resultItems = profileUserResults.slice(0, limit); // Transform results const items = resultItems.map((result) => { diff --git a/packages/common/src/utils/db.ts b/packages/common/src/utils/db.ts index 8392dad43..49be65aa6 100644 --- a/packages/common/src/utils/db.ts +++ b/packages/common/src/utils/db.ts @@ -6,6 +6,13 @@ import { CommonError } from './error'; /** Standard sort direction type for database queries */ export type SortDir = 'asc' | 'desc'; +/** Generic paginated result type for cursor-based pagination */ +export type PaginatedResult = { + items: T[]; + next: string | null; + hasMore: boolean; +}; + // Cursor utilities type GenericCursor = { date: Date; From 0760924f267b984d6116c7ad458182af3b6ed7b2 Mon Sep 17 00:00:00 2001 From: Scott Cazan Date: Thu, 22 Jan 2026 23:39:26 +0100 Subject: [PATCH 6/7] Use generic type and fix unnecessary hasMore check --- .../src/services/decision/getResults.ts | 27 ++++++++++--------- .../services/decision/listDecisionProfiles.ts | 9 ++----- .../organization/listOrganizations.ts | 2 +- .../common/src/services/posts/listPosts.ts | 2 +- .../posts/listRelatedOrganizationPosts.ts | 2 +- .../src/services/profile/listProfiles.ts | 2 +- .../requests/listProfileJoinRequests.ts | 13 ++++----- 7 files changed, 28 insertions(+), 29 deletions(-) diff --git a/packages/common/src/services/decision/getResults.ts b/packages/common/src/services/decision/getResults.ts index 27dbb1190..1672577d9 100644 --- a/packages/common/src/services/decision/getResults.ts +++ b/packages/common/src/services/decision/getResults.ts @@ -11,13 +11,26 @@ import { User } from '@op/supabase/lib'; import { assertAccess, permission } from 'access-zones'; import { count as countFn } from 'drizzle-orm'; -import { NotFoundError, decodeCursor, encodeCursor } from '../../utils'; +import { + NotFoundError, + type PaginatedResult, + decodeCursor, + encodeCursor, +} from '../../utils'; import { getOrgAccessUser } from '../access'; import { listProposals } from './listProposals'; // Uses selectionRank with id as tiebreaker for stable ordering type SelectionCursor = { selectionRank: number | null; id: string }; +type ResultProposalItem = Awaited< + ReturnType +>['proposals'][number] & { + selectionRank: number | null; + voteCount: number; + allocated: string | null; +}; + export const getLatestResultWithProposals = async ({ processInstanceId, user, @@ -28,17 +41,7 @@ export const getLatestResultWithProposals = async ({ user: User; limit?: number; cursor?: string | null; -}): Promise<{ - items: Array< - Awaited>['proposals'][number] & { - selectionRank: number | null; - voteCount: number; - allocated: string | null; - } - >; - next: string | null; - hasMore: boolean; -} | null> => { +}): Promise | null> => { const instanceWithOrg = await db .select({ instanceId: processInstances.id, diff --git a/packages/common/src/services/decision/listDecisionProfiles.ts b/packages/common/src/services/decision/listDecisionProfiles.ts index c5cf69fc0..f7a9d8fee 100644 --- a/packages/common/src/services/decision/listDecisionProfiles.ts +++ b/packages/common/src/services/decision/listDecisionProfiles.ts @@ -9,6 +9,7 @@ import { import { User } from '@op/supabase/lib'; import { + type PaginatedResult, constructTextSearch, decodeCursor, encodeCursor, @@ -58,12 +59,6 @@ type DecisionProfileItem = Omit< }; }; -type ListDecisionProfilesResult = { - items: DecisionProfileItem[]; - next: string | null; - hasMore: boolean; -}; - export const listDecisionProfiles = async ({ user, search, @@ -82,7 +77,7 @@ export const listDecisionProfiles = async ({ dir?: 'asc' | 'desc'; cursor?: string | null; ownerProfileId?: string | null; -}): Promise => { +}): Promise> => { // Get the column to order by const orderByColumn = orderBy === 'name' diff --git a/packages/common/src/services/organization/listOrganizations.ts b/packages/common/src/services/organization/listOrganizations.ts index 31e57e8d2..775d14597 100644 --- a/packages/common/src/services/organization/listOrganizations.ts +++ b/packages/common/src/services/organization/listOrganizations.ts @@ -79,7 +79,7 @@ export const listOrganizations = async ({ }); const hasMore = result.length > limit; - const items = hasMore ? result.slice(0, limit) : result; + const items = result.slice(0, limit); const lastItem = items[items.length - 1]; const orderByValue = diff --git a/packages/common/src/services/posts/listPosts.ts b/packages/common/src/services/posts/listPosts.ts index 121e1c0ab..57cc85832 100644 --- a/packages/common/src/services/posts/listPosts.ts +++ b/packages/common/src/services/posts/listPosts.ts @@ -101,7 +101,7 @@ export const listPosts = async ({ const filteredResult = result.filter((item) => item.post !== null); const hasMore = filteredResult.length > limit; - const items = hasMore ? filteredResult.slice(0, limit) : filteredResult; + const items = filteredResult.slice(0, limit); const lastItem = items[items.length - 1]; const nextCursor = hasMore && lastItem && lastItem.createdAt diff --git a/packages/common/src/services/posts/listRelatedOrganizationPosts.ts b/packages/common/src/services/posts/listRelatedOrganizationPosts.ts index 1cf7a43ee..f4ac6be31 100644 --- a/packages/common/src/services/posts/listRelatedOrganizationPosts.ts +++ b/packages/common/src/services/posts/listRelatedOrganizationPosts.ts @@ -88,7 +88,7 @@ export const listAllRelatedOrganizationPosts = async ( ]); const hasMore = result.length > limit; - const items = hasMore ? result.slice(0, limit) : result; + const items = result.slice(0, limit); const lastItem = items[items.length - 1]; const nextCursor = hasMore && lastItem && lastItem.createdAt diff --git a/packages/common/src/services/profile/listProfiles.ts b/packages/common/src/services/profile/listProfiles.ts index 2919e576d..fb52d3833 100644 --- a/packages/common/src/services/profile/listProfiles.ts +++ b/packages/common/src/services/profile/listProfiles.ts @@ -106,7 +106,7 @@ export const listProfiles = async ({ } const hasMore = result.length > limit; - const items = hasMore ? result.slice(0, limit) : result; + const items = result.slice(0, limit); const lastItem = items[items.length - 1]; const cursorValue = match(orderBy, { diff --git a/packages/common/src/services/profile/requests/listProfileJoinRequests.ts b/packages/common/src/services/profile/requests/listProfileJoinRequests.ts index a372093e9..89fb62b9a 100644 --- a/packages/common/src/services/profile/requests/listProfileJoinRequests.ts +++ b/packages/common/src/services/profile/requests/listProfileJoinRequests.ts @@ -3,7 +3,12 @@ import { JoinProfileRequestStatus, joinProfileRequests } from '@op/db/schema'; import { User } from '@op/supabase/lib'; import { and, eq } from 'drizzle-orm'; -import { decodeCursor, encodeCursor, getCursorCondition } from '../../../utils'; +import { + type PaginatedResult, + decodeCursor, + encodeCursor, + getCursorCondition, +} from '../../../utils'; import { assertTargetProfileAdminAccess } from './assertTargetProfileAdminAccess'; import { JoinProfileRequestWithProfiles } from './types'; @@ -30,11 +35,7 @@ export const listProfileJoinRequests = async ({ cursor?: string | null; limit?: number; dir?: 'asc' | 'desc'; -}): Promise<{ - items: JoinProfileRequestWithProfiles[]; - next: string | null; - hasMore: boolean; -}> => { +}): Promise> => { // Build cursor condition for pagination const cursorCondition = cursor ? getCursorCondition({ From fe37251b6cca08a4b14659f4aa80c64d6333a532 Mon Sep 17 00:00:00 2001 From: Scott Cazan Date: Fri, 23 Jan 2026 19:53:24 +0100 Subject: [PATCH 7/7] Update cursor sorting issues --- .../src/services/profile/listProfileUsers.ts | 127 +++++++++++++----- 1 file changed, 94 insertions(+), 33 deletions(-) diff --git a/packages/common/src/services/profile/listProfileUsers.ts b/packages/common/src/services/profile/listProfileUsers.ts index e4b73a1c0..d200dc0c2 100644 --- a/packages/common/src/services/profile/listProfileUsers.ts +++ b/packages/common/src/services/profile/listProfileUsers.ts @@ -1,4 +1,4 @@ -import { and, db, eq, or, sql } from '@op/db/client'; +import { and, db, eq, gt, lt, or, sql } from '@op/db/client'; import { profileUsers, profiles, users } from '@op/db/schema'; import type { User } from '@op/supabase/lib'; import { assertAccess, permission } from 'access-zones'; @@ -8,7 +8,6 @@ import { type SortDir, decodeCursor, encodeCursor, - getCursorCondition, } from '../../utils/db'; import { UnauthorizedError } from '../../utils/error'; import { getProfileAccessUser } from '../access'; @@ -20,6 +19,20 @@ import type { export type ProfileUserOrderBy = 'name' | 'email' | 'role'; +/** + * Builds a subquery to get the first role name (alphabetically) for a profile user. + * Used for both ORDER BY and cursor conditions to ensure consistency. + * Returns empty string if user has no roles (via COALESCE) to match JS cursor encoding. + */ +const buildRoleNameSubquery = (profileUserIdColumn: unknown) => sql`COALESCE(( + SELECT ar.name + FROM "profileUser_to_access_roles" pur + INNER JOIN "access_roles" ar ON ar.id = pur.access_role_id + WHERE pur.profile_user_id = ${profileUserIdColumn} + ORDER BY ar.name + LIMIT 1 +), '')`; + /** * List all members of a profile with cursor-based pagination */ @@ -76,22 +89,47 @@ export const listProfileUsers = async ({ })() : undefined; - // Determine the column to use for cursor-based pagination - // For role sorting, we use email as the cursor column since role values can be duplicated - const orderByColumn = - orderBy === 'email' ? profileUsers.email : profileUsers.email; - // Build cursor condition for pagination - // Always use id as tiebreaker for stable pagination - const cursorCondition = cursor - ? getCursorCondition({ - column: orderByColumn, - tieBreakerColumn: profileUsers.id, - cursor: decodeCursor<{ value: string; id?: string }>(cursor), - direction: dir, - }) + // The cursor must match the ORDER BY columns for correct pagination + type ProfileUserCursor = { value: string; tiebreaker?: string }; + const decodedCursor = cursor + ? decodeCursor(cursor) : undefined; + const compareFn = dir === 'asc' ? gt : lt; + + const buildCursorCondition = () => { + if (!decodedCursor) { + return undefined; + } + + if (orderBy === 'email') { + // Email is unique, no tiebreaker needed + return compareFn(profileUsers.email, decodedCursor.value); + } + + if (orderBy === 'name') { + // ORDER BY name, email - compound condition + return or( + compareFn(profileUsers.name, decodedCursor.value), + and( + eq(profileUsers.name, decodedCursor.value), + compareFn(profileUsers.email, decodedCursor.tiebreaker ?? ''), + ), + ); + } + + // orderBy === 'role' - uses shared subquery helper + const roleSubquery = buildRoleNameSubquery(profileUsers.id); + const compareOp = dir === 'asc' ? sql`>` : sql`<`; + return sql`( + ${roleSubquery} ${compareOp} ${decodedCursor.value} + OR (${roleSubquery} = ${decodedCursor.value} AND ${profileUsers.email} ${compareOp} ${decodedCursor.tiebreaker ?? ''}) + )`; + }; + + const cursorCondition = buildCursorCondition(); + // Combine all conditions const baseCondition = eq(profileUsers.profileId, profileId); const conditions = [baseCondition, searchFilter, cursorCondition].filter( @@ -124,16 +162,8 @@ export const listProfileUsers = async ({ const orderFn = dir === 'desc' ? desc : asc; if (orderBy === 'role') { - // Use a subquery to get the first role name for sorting - // Note: Using raw SQL strings because Drizzle's sql template uses outer query aliases - const roleNameSubquery = sql`( - SELECT ar.name - FROM "profileUser_to_access_roles" pur - INNER JOIN "access_roles" ar ON ar.id = pur.access_role_id - WHERE pur.profile_user_id = ${table.id} - ORDER BY ar.name - LIMIT 1 - )`; + // Use shared subquery helper for consistency with cursor condition + const roleNameSubquery = buildRoleNameSubquery(table.id); // Add email as secondary sort for consistent cursor pagination return [orderFn(roleNameSubquery), orderFn(table.email)]; } @@ -168,14 +198,45 @@ export const listProfileUsers = async ({ }); // Build next cursor from last item - const lastItem = resultItems[resultItems.length - 1]; - const nextCursor = - hasMore && lastItem - ? encodeCursor<{ value: string; id?: string }>({ - value: lastItem.email, - id: lastItem.id, - }) - : null; + // Cursor value must match the primary ORDER BY column + const lastResult = profileUserResults[resultItems.length - 1]; + const buildNextCursor = (): string | null => { + if (!hasMore || !lastResult) { + return null; + } + + if (orderBy === 'email') { + return encodeCursor({ value: lastResult.email }); + } + + if (orderBy === 'name') { + return encodeCursor({ + value: lastResult.name ?? '', + tiebreaker: lastResult.email, + }); + } + + // orderBy === 'role' - get first role name alphabetically (matching the ORDER BY subquery) + // Use simple string comparison to match PostgreSQL's default collation + const sortedRoles = [...lastResult.roles].sort((a, b) => { + const nameA = a.accessRole?.name ?? ''; + const nameB = b.accessRole?.name ?? ''; + if (nameA < nameB) { + return -1; + } + if (nameA > nameB) { + return 1; + } + return 0; + }); + const firstRoleName = sortedRoles[0]?.accessRole?.name ?? ''; + return encodeCursor({ + value: firstRoleName, + tiebreaker: lastResult.email, + }); + }; + + const nextCursor = buildNextCursor(); return { items,