From f9089c5b56ef5505df77f30925feca0a481be804 Mon Sep 17 00:00:00 2001 From: ionfwsrijan Date: Sun, 2 Aug 2026 01:47:19 +0530 Subject: [PATCH] fix: maintainer queue pagination with in-SQL filters and exact counts (#863) --- src/app/actions/maintainer.test.ts | 10 +- src/app/actions/maintainer/queue.test.ts | 320 +++++++++++++++++++++++ src/app/actions/maintainer/queue.ts | 67 +++-- 3 files changed, 369 insertions(+), 28 deletions(-) create mode 100644 src/app/actions/maintainer/queue.test.ts diff --git a/src/app/actions/maintainer.test.ts b/src/app/actions/maintainer.test.ts index 9ad90e9c..6fbb80a3 100644 --- a/src/app/actions/maintainer.test.ts +++ b/src/app/actions/maintainer.test.ts @@ -404,12 +404,12 @@ describe('maintainer actions', () => { mockFrom .mockReturnValueOnce(chain({ min_contributor_level: 2 })) - .mockReturnValueOnce(chain([lowPr, seniorPr])) + // Profiles pre-query: only level >= 2 profiles clear the filter, so + // the SQL filter excludes low-user before the pull_requests read. + .mockReturnValueOnce(chain([{ id: 'senior-user' }])) + .mockReturnValueOnce(chain([seniorPr])) .mockReturnValueOnce( - chain([ - { id: 'low-user', github_handle: 'low', level: 1, xp: 50 }, - { id: 'senior-user', github_handle: 'senior', level: 2, xp: 500 }, - ]), + chain([{ id: 'senior-user', github_handle: 'senior', level: 2, xp: 500 }]), ) .mockReturnValueOnce(chain([])); diff --git a/src/app/actions/maintainer/queue.test.ts b/src/app/actions/maintainer/queue.test.ts new file mode 100644 index 00000000..9771f5cd --- /dev/null +++ b/src/app/actions/maintainer/queue.test.ts @@ -0,0 +1,320 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { getMaintainerIssueQueue, getMaintainerPrQueue } from './queue'; + +const mocks = vi.hoisted(() => ({ + mockRequireMaintainer: vi.fn(), + mockListMaintainerRepos: vi.fn(), + mockCacheGet: vi.fn(), + mockCacheSet: vi.fn(), +})); + +vi.mock('@/lib/action-auth', () => ({ + requireMaintainer: mocks.mockRequireMaintainer, +})); + +vi.mock('@/lib/maintainer/detect', () => ({ + listMaintainerRepos: mocks.mockListMaintainerRepos, +})); + +vi.mock('@/lib/cache', () => ({ + cacheGet: mocks.mockCacheGet, + cacheSet: mocks.mockCacheSet, +})); + +vi.mock('@/inngest/client', () => ({ + inngest: { send: vi.fn() }, +})); + +vi.mock('@/lib/github/app', () => ({ + getInstallOctokit: vi.fn(), +})); + +vi.mock('@/lib/rate-limit', () => ({ + RATE_LIMIT_TIERS: { + GENEROUS: { limit: 1000, windowSec: 60 }, + STANDARD: { limit: 100, windowSec: 60 }, + HOURLY: { limit: 60, windowSec: 3600 }, + }, +})); + +type Chain = Record; + +function makeService() { + const queues: Record = new Proxy({} as Record, { + get(target, prop) { + if (typeof prop === 'string' && !(prop in target)) target[prop] = []; + return target[prop as string]; + }, + }); + const chains: Record = {}; + + const chain = (q: unknown[]): Chain => { + const c: Chain = {}; + for (const k of [ + 'select', + 'in', + 'eq', + 'or', + 'gte', + 'order', + 'range', + 'is', + 'neq', + 'lte', + 'lt', + 'limit', + 'not', + 'insert', + 'update', + 'delete', + ]) { + c[k] = vi.fn(() => c); + } + c.maybeSingle = vi.fn(async () => q.shift() ?? { data: null, error: null }); + c.then = (resolve: (v: unknown) => void) => { + resolve(q.shift() ?? { data: [], count: 0, error: null }); + }; + return c; + }; + + const service = { + from: vi.fn((table: string) => { + if (!queues[table]) queues[table] = []; + if (!chains[table]) chains[table] = []; + const c = chain(queues[table]); + chains[table].push(c); + return c; + }), + }; + + return { service, queues, chains }; +} + +function rawPr(overrides: Record = {}) { + return { + id: 1, + repo_full_name: 'org/repo', + number: 1, + title: 'A PR', + url: 'https://github.com/org/repo/pull/1', + state: 'open', + draft: false, + author_login: 'author', + author_user_id: null, + mentor_verified: false, + mentor_reviewer_id: null, + github_updated_at: '2026-01-01T00:00:00.000Z', + ai_flagged: false, + ...overrides, + }; +} + +function rawIssue(overrides: Record = {}) { + return { + id: 1, + repo_full_name: 'org/repo', + github_issue_number: 1, + title: 'An issue', + url: 'https://github.com/org/repo/issues/1', + state: 'open', + author_login: 'author', + assignee_login: null, + labels: [], + comments_count: 0, + last_event_at: '2026-07-15T00:00:00.000Z', + github_created_at: '2026-07-15T00:00:00.000Z', + ...overrides, + }; +} + +describe('getMaintainerPrQueue', () => { + let queues: Record; + let chains: Record; + + beforeEach(() => { + vi.clearAllMocks(); + const svc = makeService(); + queues = svc.queues; + chains = svc.chains; + mocks.mockRequireMaintainer.mockResolvedValue({ + ok: true, + data: { user: { id: 'user-1' }, service: svc.service }, + }); + mocks.mockListMaintainerRepos.mockResolvedValue(['org/repo']); + }); + + it('returns page 0 and derives hasMore from the exact SQL count', async () => { + const prs = Array.from({ length: 100 }, (_, i) => rawPr({ id: i + 1, number: i + 1 })); + queues['installation_settings']!.push({ data: { min_contributor_level: 0 } }); + queues['pull_requests']!.push({ data: prs, count: 500 }); + + const res = await getMaintainerPrQueue({ installationId: 1 }); + + expect(res.ok).toBe(true); + if (res.ok) { + expect(res.data.rows).toHaveLength(25); + expect(res.data.hasMore).toBe(true); + } + }); + + it('does not report hasMore once the exact count is exhausted', async () => { + const prs = Array.from({ length: 30 }, (_, i) => rawPr({ id: i + 1, number: i + 1 })); + queues['installation_settings']!.push({ data: { min_contributor_level: 0 } }); + queues['pull_requests']!.push({ data: prs, count: 30 }); + + const res = await getMaintainerPrQueue({ installationId: 1, page: 1 }); + + expect(res.ok).toBe(true); + if (res.ok) { + expect(res.data.rows).toHaveLength(5); + expect(res.data.hasMore).toBe(false); + } + }); + + it('pushes the min contributor level into SQL via a profiles query', async () => { + queues['installation_settings']!.push({ data: { min_contributor_level: 2 } }); + queues['profiles']!.push({ data: [{ id: 'p1' }, { id: 'p2' }] }); + queues['profiles']!.push({ + data: [{ id: 'p1', github_handle: 'author', level: 2, xp: 10 }], + }); + queues['xp_events']!.push({ data: [] }); + queues['pull_requests']!.push({ + data: [rawPr({ id: 7, author_user_id: 'p1' })], + count: 1, + }); + + const res = await getMaintainerPrQueue({ installationId: 1 }); + + expect(res.ok).toBe(true); + const prChain = chains['pull_requests']![0]; + expect(prChain!.or).toHaveBeenCalledWith('author_user_id.in.(p1,p2)'); + if (res.ok) { + expect(res.data.rows).toHaveLength(1); + expect(res.data.rows[0]!.authorLevel).toBe(2); + } + }); + + it('keeps PRs from authors without a profile when level 0 is allowed', async () => { + queues['installation_settings']!.push({ data: { min_contributor_level: 0 } }); + queues['profiles']!.push({ data: [{ id: 'p1' }] }); + queues['profiles']!.push({ data: [{ id: 'p1', github_handle: 'a', level: 1, xp: 5 }] }); + queues['xp_events']!.push({ data: [] }); + queues['pull_requests']!.push({ + data: [rawPr({ id: 9, author_user_id: null })], + count: 1, + }); + + const res = await getMaintainerPrQueue({ installationId: 1, filters: { authorLevel: [0] } }); + + expect(res.ok).toBe(true); + const prChain = chains['pull_requests']![0]; + expect(prChain!.or).toHaveBeenCalledWith('author_user_id.in.(p1),author_user_id.is.null'); + if (res.ok) { + expect(res.data.rows).toHaveLength(1); + } + }); + + it('returns an empty queue when no profile clears the author filter', async () => { + queues['installation_settings']!.push({ data: { min_contributor_level: 3 } }); + queues['profiles']!.push({ data: [] }); + + const res = await getMaintainerPrQueue({ installationId: 1 }); + + expect(res.ok).toBe(true); + if (res.ok) { + expect(res.data.rows).toHaveLength(0); + expect(res.data.hasMore).toBe(false); + } + // No pull_requests read should have been issued. + expect(chains['pull_requests'] ?? []).toHaveLength(0); + }); + + it('advances the DB window every four pages', async () => { + queues['installation_settings']!.push({ data: { min_contributor_level: 0 } }); + queues['pull_requests']!.push({ + data: Array.from({ length: 100 }, (_, i) => rawPr({ id: i + 1, number: i + 1 })), + count: 300, + }); + + const res = await getMaintainerPrQueue({ installationId: 1, page: 4 }); + + expect(res.ok).toBe(true); + if (res.ok) { + expect(res.data.rows).toHaveLength(25); + expect(res.data.hasMore).toBe(true); + } + }); +}); + +describe('getMaintainerIssueQueue', () => { + let queues: Record; + + beforeEach(() => { + vi.clearAllMocks(); + const svc = makeService(); + queues = svc.queues; + mocks.mockRequireMaintainer.mockResolvedValue({ + ok: true, + data: { user: { id: 'user-1' }, service: svc.service }, + }); + mocks.mockListMaintainerRepos.mockResolvedValue(['org/repo']); + }); + + it('paginates the full candidate set and reports accurate hasMore', async () => { + queues['issues']!.push({ + data: Array.from({ length: 60 }, (_, i) => + rawIssue({ id: i + 1, github_issue_number: i + 1 }), + ), + }); + + const page0 = await getMaintainerIssueQueue({ installationId: 1, page: 0 }); + expect(page0.ok).toBe(true); + if (page0.ok) { + expect(page0.data.rows).toHaveLength(25); + expect(page0.data.hasMore).toBe(true); + } + + queues['issues']!.push({ + data: Array.from({ length: 60 }, (_, i) => + rawIssue({ id: i + 1, github_issue_number: i + 1 }), + ), + }); + + const page2 = await getMaintainerIssueQueue({ installationId: 1, page: 2 }); + expect(page2.ok).toBe(true); + if (page2.ok) { + expect(page2.data.rows).toHaveLength(10); + expect(page2.data.hasMore).toBe(false); + } + }); + + it('classifies issues into buckets and filters by requested buckets', async () => { + queues['issues']!.push({ + data: [ + rawIssue({ id: 1, assignee_login: null, labels: [] }), // needs-triage + rawIssue({ id: 2, assignee_login: 'bob', labels: [] }), // in-progress + rawIssue({ + id: 3, + assignee_login: null, + labels: [], + last_event_at: '2025-01-01T00:00:00.000Z', + github_created_at: '2025-01-01T00:00:00.000Z', + }), // stale + rawIssue({ id: 4, state: 'closed' }), // closed + ], + }); + + const res = await getMaintainerIssueQueue({ + installationId: 1, + buckets: ['needs-triage', 'in-progress'], + }); + + expect(res.ok).toBe(true); + if (res.ok) { + // needs-triage (1, 3) + in-progress (2); closed (4) excluded. + expect(res.data.rows).toHaveLength(3); + const ids = res.data.rows.map((r) => r.id).sort(); + expect(ids).toEqual([1, 2, 3]); + } + }); +}); diff --git a/src/app/actions/maintainer/queue.ts b/src/app/actions/maintainer/queue.ts index 5cd60201..119d21aa 100644 --- a/src/app/actions/maintainer/queue.ts +++ b/src/app/actions/maintainer/queue.ts @@ -87,11 +87,31 @@ export async function getMaintainerPrQueue(args: { const minContributorLevel = await readMinContributorLevel(service, args.installationId); + // Author-level filters are pushed into SQL (profiles.level) so pagination + // windows only ever contain rows the maintainer wants to see. In-app the + // author level is `profiles.level ?? 0` — a PR whose author has no MergeShip + // profile behaves as level 0, so it must survive when level 0 is allowed. + let allowedAuthorIds: string[] | null = null; + let allowNullAuthor = false; + if (minContributorLevel > 0 || filters.authorLevel.length > 0) { + let pq = service.from('profiles').select('id').gte('level', minContributorLevel); + if (filters.authorLevel.length > 0) pq = pq.in('level', filters.authorLevel); + const { data: profs } = await pq; + allowedAuthorIds = (profs ?? []).map((p) => p.id); + allowNullAuthor = + minContributorLevel === 0 && + (filters.authorLevel.length === 0 || filters.authorLevel.includes(0)); + if (allowedAuthorIds.length === 0 && !allowNullAuthor) { + return ok({ rows: [], hasMore: false }); + } + } + let q = service .from('pull_requests') .select( 'id, repo_full_name, number, title, url, state, draft, author_login, ' + 'author_user_id, mentor_verified, mentor_reviewer_id, github_updated_at, ai_flagged', + { count: 'exact' }, ) .in('repo_full_name', scopedRepos); @@ -101,6 +121,13 @@ export async function getMaintainerPrQueue(args: { if (filters.aiFlagged === 'yes') q = q.eq('ai_flagged', true); else if (filters.aiFlagged === 'no') q = q.eq('ai_flagged', false); if (filters.authorLogin) q = q.eq('author_login', filters.authorLogin); + if (allowedAuthorIds) { + const orConds: string[] = []; + if (allowedAuthorIds.length > 0) + orConds.push(`author_user_id.in.(${allowedAuthorIds.join(',')})`); + if (allowNullAuthor) orConds.push('author_user_id.is.null'); + q = q.or(orConds.join(',')); + } // Pull a generous slice; we re-sort by tier client-side. type RawPr = { @@ -118,7 +145,7 @@ export async function getMaintainerPrQueue(args: { github_updated_at: string; ai_flagged: boolean; }; - const { data: prs } = await q + const { data: prs, count } = await q .order('github_updated_at', { ascending: false }) .range( Math.floor(page / 4) * PAGE_SIZE * 4, @@ -189,23 +216,18 @@ export async function getMaintainerPrQueue(args: { }; }); - // Apply author-level filter after the join (since author level isn't on - // the pull_requests row). - let filtered = rows.filter((row) => (row.authorLevel ?? 0) >= minContributorLevel); - if (filters.authorLevel.length > 0) { - filtered = filtered.filter((row) => filters.authorLevel.includes(row.authorLevel ?? 0)); - } - if (filters.aiFlagged === 'yes') { - filtered = filtered.filter((row) => row.aiFlagged); - } else if (filters.aiFlagged === 'no') { - filtered = filtered.filter((row) => !row.aiFlagged); - } - + // state/mentor/ai/author-level filters are all applied in SQL above, so every + // row in this window is a candidate — all that remains is the tier sort. + const filtered = rows; filtered.sort(comparePrRows); const startIdx = (page % 4) * PAGE_SIZE; const page_rows = filtered.slice(startIdx, startIdx + PAGE_SIZE); - const hasMore = startIdx + PAGE_SIZE < filtered.length || prRows.length === PAGE_SIZE * 4; + // `count` is the exact total of the SQL-filtered set, so `hasMore` no longer + // guesses from a full 100-row window (which made pages 1-3 empty and hid + // rows beyond offset 100). + const total = count ?? filtered.length; + const hasMore = startIdx + PAGE_SIZE < total; return ok({ rows: page_rows, hasMore }); } @@ -238,7 +260,6 @@ export async function getMaintainerIssueQueue(args: { ISSUE_BUCKETS.has(b), ); - // Pull a generous slice — we classify in app code, can't filter buckets in SQL. const page = Math.max(0, args.page ?? 0); const states: ('open' | 'closed')[] = buckets.includes('closed') ? ['open', 'closed'] : ['open']; @@ -257,6 +278,10 @@ export async function getMaintainerIssueQueue(args: { github_created_at: string | null; }; + // Pull every candidate that could classify into a requested bucket — bucket + // assignment happens in-app (labels + staleness), so limiting the DB read to + // a window would make matching rows past the window unreachable. The set is + // bounded by the maintainer's own repos. const { data: issuesRaw } = await service .from('issues') .select( @@ -265,11 +290,7 @@ export async function getMaintainerIssueQueue(args: { ) .in('repo_full_name', scopedRepos) .in('state', states) - .order('last_event_at', { ascending: false, nullsFirst: false }) - .range( - Math.floor(page / 4) * PAGE_SIZE * 4, - Math.floor(page / 4) * PAGE_SIZE * 4 + PAGE_SIZE * 4 - 1, - ); + .order('last_event_at', { ascending: false, nullsFirst: false }); const rows: MaintainerIssueRow[] = ((issuesRaw ?? []) as unknown as RawIssue[]).map((r) => { const triage = classifyTriage({ @@ -313,10 +334,10 @@ export async function getMaintainerIssueQueue(args: { return bt - at; }); - const startIdx = (page % 4) * PAGE_SIZE; + // The full candidate set is in memory, so the offset is a plain page offset. + const startIdx = page * PAGE_SIZE; const pageRows = filtered.slice(startIdx, startIdx + PAGE_SIZE); - const hasMore = - startIdx + PAGE_SIZE < filtered.length || (issuesRaw?.length ?? 0) === PAGE_SIZE * 4; + const hasMore = startIdx + PAGE_SIZE < filtered.length; return ok({ rows: pageRows, hasMore }); }