diff --git a/drizzle/0016_doubts_composite_indexes.sql b/drizzle/0016_doubts_composite_indexes.sql new file mode 100644 index 00000000..0966ff99 --- /dev/null +++ b/drizzle/0016_doubts_composite_indexes.sql @@ -0,0 +1,9 @@ +-- Composite indexes for the doubts feed (issue #319). +-- Support the common filter + recency-ordering access pattern: +-- WHERE classroomId = ? [AND type = ? | AND isSolved = ?] ORDER BY createdAt DESC +-- createdAt is the trailing column so the index covers both the filter and the +-- ORDER BY (no separate sort step). Idempotent so it is safe to re-run via the +-- manual migrate runner. +CREATE INDEX IF NOT EXISTS "doubts_classroomId_createdAt_idx" ON "doubts" USING btree ("classroomId", "createdAt");--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "doubts_classroomId_type_createdAt_idx" ON "doubts" USING btree ("classroomId", "type", "createdAt");--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "doubts_classroomId_isSolved_createdAt_idx" ON "doubts" USING btree ("classroomId", "isSolved", "createdAt"); diff --git a/drizzle/meta/_journal.json b/drizzle/meta/_journal.json index 76912be3..b0bf65d3 100644 --- a/drizzle/meta/_journal.json +++ b/drizzle/meta/_journal.json @@ -18,6 +18,7 @@ { "idx": 13, "version": "7", "when": 1781300000000, "tag": "0013_identity_system_update", "breakpoints": true }, { "idx": 14, "version": "7", "when": 1781340000000, "tag": "0014_practice_attempts", "breakpoints": true }, { "idx": 15, "version": "7", "when": 1781354492608, "tag": "0015_add_onboarding_fields", "breakpoints": true }, - { "idx": 16, "version": "7", "when": 1781400000000, "tag": "0016_video_jobs", "breakpoints": true } + { "idx": 16, "version": "7", "when": 1781400000000, "tag": "0016_doubts_composite_indexes", "breakpoints": true } + { "idx": 17, "version": "7", "when": 1781400000000, "tag": "0017_video_jobs", "breakpoints": true } ] -} \ No newline at end of file +} diff --git a/src/__tests__/api/teacher-insights.test.ts b/src/__tests__/api/teacher-insights.test.ts index af2705c9..a203e55f 100644 --- a/src/__tests__/api/teacher-insights.test.ts +++ b/src/__tests__/api/teacher-insights.test.ts @@ -64,7 +64,7 @@ describe('Teacher Insights API Endpoint', () => { const json = await res.json(); expect(res.status).toBe(400); - expect(json.error).toBe('classroomId is required'); + expect(json.error).toBe('Invalid classroom ID'); }); it('returns 403 when the user is not the teacher of the classroom', async () => { @@ -79,7 +79,7 @@ describe('Teacher Insights API Endpoint', () => { const json = await res.json(); expect(res.status).toBe(403); - expect(json.error).toBe('Forbidden: not the teacher of this classroom'); + expect(json.error).toBe('Access denied to this classroom'); }); it('returns classroom-scoped insights for the teacher', async () => { diff --git a/src/__tests__/inngest/digest-functions.test.ts b/src/__tests__/inngest/digest-functions.test.ts index e145f792..4be76da3 100644 --- a/src/__tests__/inngest/digest-functions.test.ts +++ b/src/__tests__/inngest/digest-functions.test.ts @@ -61,6 +61,15 @@ function makeStep() { }; } +// Inngest's createFunction returns a function object whose handler is stored on +// `.fn`; invoke that directly to unit-test the handler with a mock step. +function runHandler( + fn: unknown, + step: ReturnType, +): Promise { + return (fn as { fn: (ctx: { step: unknown }) => Promise }).fn({ step }); +} + // ── Tests ───────────────────────────────────────────────────────────────────── describe("sendDailyDigest — per-user step isolation", () => { @@ -115,8 +124,7 @@ describe("sendDailyDigest — per-user step isolation", () => { const { sendDailyDigest } = await import("@/inngest/functions"); const step = makeStep(); - // @ts-expect-error — internal test invocation bypasses Inngest runtime types - await expect(sendDailyDigest({ step })).rejects.toThrow("SMTP timeout"); + await expect(runHandler(sendDailyDigest, step)).rejects.toThrow("SMTP timeout"); // Alice's row MUST have been deleted (email succeeded). expect(dbMock.delete).toHaveBeenCalledTimes(1); @@ -146,19 +154,17 @@ describe("sendDailyDigest — per-user step isolation", () => { const deleteChain = { where: jest.fn().mockResolvedValue(undefined) }; (dbMock.delete as jest.Mock).mockReturnValue(deleteChain); - mockSendDigestEmail.mockResolvedValue(undefined); + mockSendDigestEmail.mockResolvedValue({ success: true }); const { sendDailyDigest } = await import("@/inngest/functions"); // First run — completes successfully. const step = makeStep(); - // @ts-expect-error - await sendDailyDigest({ step }); + await runHandler(sendDailyDigest, step); expect(mockSendDigestEmail).toHaveBeenCalledTimes(1); // Simulate Inngest retry: same step shim (memoised results) → per-user step is a no-op. - // @ts-expect-error - await sendDailyDigest({ step }); + await runHandler(sendDailyDigest, step); // sendDigestEmail must NOT be called again. expect(mockSendDigestEmail).toHaveBeenCalledTimes(1); }); diff --git a/src/__tests__/lib/anonymity.test.ts b/src/__tests__/lib/anonymity.test.ts index 5b544fc1..62682cf2 100644 --- a/src/__tests__/lib/anonymity.test.ts +++ b/src/__tests__/lib/anonymity.test.ts @@ -54,10 +54,9 @@ describe("anonymity: fail closed in production", () => { afterEach(() => { if (origEnv === undefined) delete mutableEnv.NODE_ENV; else mutableEnv.NODE_ENV = origEnv; - if (origSalt === undefined) delete process.env.ANON_HANDLE_SALT; - else process.env.ANON_HANDLE_SALT = origSalt; + if (origSalt === undefined) delete mutableEnv.ANON_HANDLE_SALT; + else mutableEnv.ANON_HANDLE_SALT = origSalt; }); - it("throws when ANON_HANDLE_SALT is missing in production", () => { delete process.env.ANON_HANDLE_SALT; diff --git a/src/__tests__/lib/pagination.test.ts b/src/__tests__/lib/pagination.test.ts new file mode 100644 index 00000000..d14271ea --- /dev/null +++ b/src/__tests__/lib/pagination.test.ts @@ -0,0 +1,46 @@ +import { encodeCursor, decodeCursor } from "@/lib/pagination"; + +describe("pagination cursor", () => { + it("round-trips a (createdAt, id) key", () => { + const createdAt = new Date("2026-06-01T12:34:56.000Z"); + const cursor = encodeCursor(createdAt, 42); + const decoded = decodeCursor(cursor); + expect(decoded).not.toBeNull(); + expect(decoded!.id).toBe(42); + expect(decoded!.createdAt.toISOString()).toBe(createdAt.toISOString()); + }); + + it("accepts a Date, ISO string, or epoch ms as input", () => { + const iso = "2026-01-15T08:00:00.000Z"; + const fromDate = decodeCursor(encodeCursor(new Date(iso), 1)); + const fromString = decodeCursor(encodeCursor(iso, 1)); + const fromEpoch = decodeCursor(encodeCursor(new Date(iso).getTime(), 1)); + expect(fromDate!.createdAt.toISOString()).toBe(iso); + expect(fromString!.createdAt.toISOString()).toBe(iso); + expect(fromEpoch!.createdAt.toISOString()).toBe(iso); + }); + + it("produces an opaque base64 string (no raw delimiter leakage)", () => { + const cursor = encodeCursor(new Date("2026-06-01T00:00:00.000Z"), 7); + expect(cursor).toMatch(/^[A-Za-z0-9+/]+=*$/); + expect(cursor).not.toContain("|"); + }); + + it("returns null for null/empty/garbage input instead of throwing", () => { + expect(decodeCursor(null)).toBeNull(); + expect(decodeCursor(undefined)).toBeNull(); + expect(decodeCursor("")).toBeNull(); + expect(decodeCursor("not-base64-!!!")).toBeNull(); + expect(decodeCursor(Buffer.from("garbage-no-separator").toString("base64"))).toBeNull(); + }); + + it("rejects a cursor with a non-numeric id", () => { + const bad = Buffer.from("2026-06-01T00:00:00.000Z|abc").toString("base64"); + expect(decodeCursor(bad)).toBeNull(); + }); + + it("rejects a cursor with an invalid date", () => { + const bad = Buffer.from("not-a-date|5").toString("base64"); + expect(decodeCursor(bad)).toBeNull(); + }); +}); diff --git a/src/app/api/doubts/[id]/accept/route.test.ts b/src/app/api/doubts/[id]/accept/route.test.ts index 035d54b4..b056f0f8 100644 --- a/src/app/api/doubts/[id]/accept/route.test.ts +++ b/src/app/api/doubts/[id]/accept/route.test.ts @@ -21,7 +21,10 @@ let mockReply: { let mockUpdatedDoubt: { id: number } | null = { id: 1 }; +// jest.mock factories are hoisted, and the hoist guard only permits references to +// out-of-scope variables whose names match /^mock/i: hence these names. const mockInngestSend = jest.fn(); +let mockSelectCallCount = 0; // ── Mocks ───────────────────────────────────────────────────────────────────── jest.mock("@clerk/nextjs/server", () => ({ @@ -97,7 +100,7 @@ async function callPost(replyId = 42) { const { POST } = await import("./route"); return POST(makeRequest(replyId), { params: Promise.resolve({ id: "1" }), - } as any); + } as unknown as { params: Promise<{ id: string }> }); } // ── Tests ───────────────────────────────────────────────────────────────────── @@ -164,7 +167,7 @@ describe("POST /api/doubts/[id]/accept — idempotency (issue #687)", () => { it("returns 500 with a generic message and does not leak error details", async () => { // Make the DB throw to exercise the catch block const { db } = await import("@/configs/db"); - jest.mocked(db.select).mockImplementationOnce(() => { + (db.select as jest.Mock).mockImplementationOnce(() => { throw new Error("relation \"doubts\" does not exist"); }); @@ -177,4 +180,4 @@ describe("POST /api/doubts/[id]/accept — idempotency (issue #687)", () => { expect(JSON.stringify(body)).not.toContain("relation"); expect(JSON.stringify(body)).not.toContain("does not exist"); }); -}); \ No newline at end of file +}); diff --git a/src/app/api/doubts/route.ts b/src/app/api/doubts/route.ts index ef9d65ab..2fac8e07 100644 --- a/src/app/api/doubts/route.ts +++ b/src/app/api/doubts/route.ts @@ -18,6 +18,7 @@ import { isNull, or, not, + lt, sql, SQL, ilike, @@ -38,6 +39,7 @@ import { buildRankOrder } from "@/lib/search"; import { canTeach } from "@/lib/auth/membership-guard"; import { currentUser } from "@clerk/nextjs/server"; import { parsePositiveInt } from "@/lib/utils"; +import { decodeCursor, encodeCursor } from "@/lib/pagination"; import { toPublicDoubt } from "@/lib/anonymity"; export async function GET(req: Request) { @@ -144,6 +146,19 @@ export async function GET(req: Request) { : 0; const page = Math.floor(offset / limit) + 1; + // Cursor-based pagination (issue #319): opt-in via the `cursor` query param + // (present even when empty, for the first page). Cursor mode uses pure + // (createdAt, id) keyset ordering end-to-end, so `nextCursor` is only ever + // issued from an already-cursor-ordered response: never from a pinned-first + // offset page, which could otherwise skip newer unpinned doubts. Only honored + // for the default recency sort with no search. The keyset predicate is applied + // once a cursor token is actually present. + const cursorRequested = searchParams.has("cursor"); + const decodedCursor = decodeCursor(searchParams.get("cursor")); + const cursorCreatedAt = decodedCursor?.createdAt ?? null; + const cursorId = decodedCursor?.id ?? null; + const useCursor = cursorRequested && sort === "newest" && !search; + if (tag && tag !== "All") { const normalizedTag = tag.trim().replace(/\s+/g, " ").toLowerCase(); conditions.push( @@ -167,6 +182,20 @@ export async function GET(req: Request) { Number, ); + // EXISTS subqueries (issue #319): fold per-user like/bookmark status into the + // main query instead of two extra full-scan round-trips. Anonymous viewers + // resolve to false. + const hasLikedSql = ( + email + ? sql`EXISTS (SELECT 1 FROM ${likesTable} WHERE ${likesTable.doubtId} = ${doubtsTable.id} AND ${likesTable.userEmail} = ${email})` + : sql`false` + ).mapWith(Boolean); + const hasBookmarkedSql = ( + email + ? sql`EXISTS (SELECT 1 FROM ${bookmarksTable} WHERE ${bookmarksTable.doubtId} = ${doubtsTable.id} AND ${bookmarksTable.userEmail} = ${email})` + : sql`false` + ).mapWith(Boolean); + const [totalCountRow] = await db .select({ count: count() }) .from(doubtsTable) @@ -177,53 +206,64 @@ export async function GET(req: Request) { .select({ ...getTableColumns(doubtsTable), replyCount: replyCountSql, + hasLiked: hasLikedSql, + hasBookmarked: hasBookmarkedSql, }) .from(doubtsTable); - const orderByFields: SQL[] = [desc(doubtsTable.isPinned)]; - - if (search) { - const rankOrder = buildRankOrder(search); - if (rankOrder) orderByFields.push(rankOrder); - } - - if (sort === "popular") { - orderByFields.push(desc(doubtsTable.likes)); - } else if (sort === "most-replied") { - orderByFields.push(desc(replyCountSql)); - } - orderByFields.push(desc(doubtsTable.createdAt)); - - let doubts = await query - .where(and(...conditions)) - .orderBy(...orderByFields) - .limit(limit) - .offset(offset); + // Cursor mode orders purely by (createdAt, id) for a stable keyset; offset mode + // keeps pinned-first plus rank/popular/most-replied ordering. + const orderByFields: SQL[] = useCursor + ? [desc(doubtsTable.createdAt), desc(doubtsTable.id)] + : [desc(doubtsTable.isPinned)]; - if (email && doubts.length > 0) { - const userLikes = await db - .select({ doubtId: likesTable.doubtId }) - .from(likesTable) - .where(eq(likesTable.userEmail, email)); + if (!useCursor) { + if (search) { + const rankOrder = buildRankOrder(search); + if (rankOrder) orderByFields.push(rankOrder); + } - const likedIds = new Set(userLikes.map((l) => l.doubtId)); - doubts = doubts.map((doubt) => ({ - ...doubt, - hasLiked: likedIds.has(doubt.id), - })); + if (sort === "popular") { + orderByFields.push(desc(doubtsTable.likes)); + } else if (sort === "most-replied") { + orderByFields.push(desc(replyCountSql)); + } + orderByFields.push(desc(doubtsTable.createdAt)); } - if (doubts.length > 0 && email) { - const userBookmarks = await db - .select({ doubtId: bookmarksTable.doubtId }) - .from(bookmarksTable) - .where(eq(bookmarksTable.userEmail, email)); - - const bookmarkedIds = new Set(userBookmarks.map((b) => b.doubtId)); - doubts = doubts.map((doubt) => ({ - ...doubt, - hasBookmarked: bookmarkedIds.has(doubt.id), - })); + // Apply the keyset predicate only once a cursor token is present (the first + // cursor-mode page has none and just returns the newest rows in keyset order). + const cursorKeyset = + cursorCreatedAt !== null && cursorId !== null + ? or( + lt(doubtsTable.createdAt, cursorCreatedAt), + and( + eq(doubtsTable.createdAt, cursorCreatedAt), + lt(doubtsTable.id, cursorId), + ), + ) + : undefined; + + // Cursor mode over-fetches one sentinel row so `hasMore` is exact (avoids a + // false positive + dead cursor when the remaining rows are exactly `limit`). + let doubts = useCursor + ? await query + .where(cursorKeyset ? and(...conditions, cursorKeyset) : and(...conditions)) + .orderBy(...orderByFields) + .limit(limit + 1) + : await query + .where(and(...conditions)) + .orderBy(...orderByFields) + .limit(limit) + .offset(offset); + + // hasLiked / hasBookmarked are now resolved inline via EXISTS subqueries above. + + const hasMore = useCursor + ? doubts.length > limit + : offset + doubts.length < totalCount; + if (useCursor && hasMore) { + doubts = doubts.slice(0, limit); // drop the sentinel before enrichment } if (doubts.length > 0) { @@ -256,7 +296,15 @@ export async function GET(req: Request) { })); } - const hasMore = offset + doubts.length < totalCount; + // Only emit a cursor from an already-cursor-ordered response (see useCursor), + // so clients never resume keyset paging from a pinned-first offset page. + const nextCursor = + useCursor && hasMore + ? encodeCursor( + doubts[doubts.length - 1].createdAt, + doubts[doubts.length - 1].id, + ) + : null; // Strip author identifiers (userEmail), the internal embedding vector and // soft-delete marker before returning. Only the anonymized handle and a @@ -269,6 +317,7 @@ export async function GET(req: Request) { totalCount, page, limit, + nextCursor, }); } catch (error) { const { status, body } = buildErrorResponse(error); diff --git a/src/configs/schema.ts b/src/configs/schema.ts index 9686cf69..9d6b0614 100644 --- a/src/configs/schema.ts +++ b/src/configs/schema.ts @@ -212,6 +212,22 @@ export const doubtsTable = pgTable("doubts", { table.userEmail, table.classroomId, ), + // Composite indexes supporting the feed's filter + recency ordering + // (WHERE classroomId = ? [AND type/isSolved] ORDER BY createdAt DESC). + classroomCreatedAtIndex: index("doubts_classroomId_createdAt_idx").on( + table.classroomId, + table.createdAt, + ), + classroomTypeIndex: index("doubts_classroomId_type_createdAt_idx").on( + table.classroomId, + table.type, + table.createdAt, + ), + classroomSolvedIndex: index("doubts_classroomId_isSolved_createdAt_idx").on( + table.classroomId, + table.isSolved, + table.createdAt, + ), userEmailFk: foreignKey({ columns: [table.userEmail], foreignColumns: [usersTable.email], diff --git a/src/lib/pagination.ts b/src/lib/pagination.ts new file mode 100644 index 00000000..0d8fc728 --- /dev/null +++ b/src/lib/pagination.ts @@ -0,0 +1,46 @@ +/** + * Keyset (cursor) pagination helpers for the doubts feed (issue #319). + * + * A cursor encodes the ordering key of the last row seen - the doubt's + * `createdAt` timestamp plus its `id` as a tiebreaker - so the next page can be + * fetched with a `WHERE (createdAt, id) < (cursorCreatedAt, cursorId)` keyset + * predicate instead of a growing `OFFSET`. This keeps deep pagination O(log n) + * on an index rather than O(n). + * + * The cursor is an opaque base64 string from the client's perspective. + */ + +export interface DecodedCursor { + createdAt: Date; + id: number; +} + +/** Encode a (createdAt, id) ordering key into an opaque cursor string. */ +export function encodeCursor(createdAt: Date | string | number, id: number): string { + const iso = new Date(createdAt).toISOString(); + return Buffer.from(`${iso}|${id}`).toString("base64"); +} + +/** + * Decode a cursor string back into its ordering key. Returns `null` for any + * malformed input so callers can safely fall back to offset pagination rather + * than throwing on attacker- or bug-supplied cursors. + */ +export function decodeCursor(cursor: string | null | undefined): DecodedCursor | null { + if (!cursor) return null; + try { + const decoded = Buffer.from(cursor, "base64").toString("utf8"); + const sep = decoded.lastIndexOf("|"); + if (sep <= 0) return null; + const iso = decoded.slice(0, sep); + const idRaw = decoded.slice(sep + 1); + const createdAt = new Date(iso); + const id = Number(idRaw); + if (Number.isNaN(createdAt.getTime()) || !Number.isInteger(id) || idRaw.trim() === "") { + return null; + } + return { createdAt, id }; + } catch { + return null; + } +}