chore: integrate advisor/resume-service-improvements (plans 001, 003, 005)

packages/api/src/dto/application.test.ts

@@ -83,3 +83,24 @@ describe("applicationDto zero-argument inputs", () => {
 		expect(applicationDto.tags.input.parse(undefined)).toEqual({});
 	});
 });
+
+// Bulk operations cap `ids` at 200 to bound memory/DB work from a single call.
+describe("applicationDto bulk id caps", () => {
+	const idsOfLength = (n: number) => Array.from({ length: n }, (_, i) => String(i));
+
+	it("rejects a bulkDelete ids array over the cap", () => {
+		expect(applicationDto.bulkDelete.input.safeParse({ ids: idsOfLength(201) }).success).toBe(false);
+	});
+
+	it("accepts a bulkDelete ids array at the cap", () => {
+		expect(applicationDto.bulkDelete.input.safeParse({ ids: idsOfLength(200) }).success).toBe(true);
+	});
+
+	it("rejects a bulkUpdate ids array over the cap", () => {
+		expect(applicationDto.bulkUpdate.input.safeParse({ ids: idsOfLength(201) }).success).toBe(false);
+	});
+
+	it("accepts a bulkUpdate ids array at the cap", () => {
+		expect(applicationDto.bulkUpdate.input.safeParse({ ids: idsOfLength(200) }).success).toBe(true);
+	});
+});

packages/api/src/dto/application.ts

@@ -162,7 +162,7 @@ export const applicationDto = {
 	// Table bulk actions: move stage, archive/unarchive, add tags across a selection.
 	bulkUpdate: {
 		input: z.object({
-			ids: z.array(z.string()).min(1),
+			ids: z.array(z.string()).min(1).max(200, "Too many items in a single bulk operation"),
 			status: applicationStatusSchema.optional(),
 			archived: z.boolean().optional(),
 			addTags: z.array(z.string()).optional(),
@@ -171,7 +171,7 @@ export const applicationDto = {
 	},
 
 	bulkDelete: {
-		input: z.object({ ids: z.array(z.string()).min(1) }),
+		input: z.object({ ids: z.array(z.string()).min(1).max(200, "Too many items in a single bulk operation") }),
 		output: z.object({ deleted: z.number() }),
 	},
 

packages/api/src/features/resume/service.test.ts

@@ -0,0 +1,341 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+// Characterization tests for the resume service. The goal is to pin down CURRENT behavior
+// (CRUD / lock / password / statistics branching) so later changes are deliberate. The DB
+// layer and side-effecting helpers are mocked; the branching in service.ts is what's under test.
+
+const dbMock = vi.hoisted(() => ({
+	select: vi.fn(),
+	insert: vi.fn(),
+	update: vi.fn(),
+	delete: vi.fn(),
+	transaction: vi.fn(),
+}));
+const hashMock = vi.hoisted(() => vi.fn());
+const compareMock = vi.hoisted(() => vi.fn());
+const publishResumeUpdatedMock = vi.hoisted(() => vi.fn());
+const grantResumeAccessMock = vi.hoisted(() => vi.fn());
+const hasResumeAccessMock = vi.hoisted(() => vi.fn());
+const storageDeleteMock = vi.hoisted(() => vi.fn());
+
+vi.mock("@reactive-resume/db/client", () => ({ db: dbMock }));
+vi.mock("@reactive-resume/db/schema", () => ({
+	resume: {
+		id: "id",
+		userId: "user_id",
+		slug: "slug",
+		name: "name",
+		tags: "tags",
+		data: "data",
+		isPublic: "is_public",
+		isLocked: "is_locked",
+		password: "password",
+		updatedAt: "updated_at",
+		createdAt: "created_at",
+	},
+	resumeStatistics: {
+		resumeId: "resume_id",
+		views: "views",
+		downloads: "downloads",
+		lastViewedAt: "last_viewed_at",
+		lastDownloadedAt: "last_downloaded_at",
+	},
+	resumeStatisticsDaily: {
+		resumeId: "resume_id",
+		date: "date",
+		views: "views",
+		downloads: "downloads",
+	},
+	resumeVersion: {
+		id: "id",
+		resumeId: "resume_id",
+		userId: "user_id",
+		data: "data",
+		label: "label",
+		createdAt: "created_at",
+	},
+	resumeAnalysis: { resumeId: "resume_id", analysis: "analysis" },
+	user: { id: "id", username: "username" },
+}));
+vi.mock("drizzle-orm", () => ({
+	and: (...a: unknown[]) => a,
+	arrayContains: (...a: unknown[]) => a,
+	asc: (x: unknown) => x,
+	desc: (x: unknown) => x,
+	eq: (...a: unknown[]) => a,
+	gte: (...a: unknown[]) => a,
+	isNotNull: (...a: unknown[]) => a,
+	notInArray: (...a: unknown[]) => a,
+	sql: Object.assign((strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values }), {
+		join: (values: unknown[]) => values,
+	}),
+}));
+vi.mock("bcrypt", () => ({ hash: hashMock, compare: compareMock }));
+vi.mock("./events", () => ({ publishResumeUpdated: publishResumeUpdatedMock }));
+vi.mock("./access", () => ({
+	grantResumeAccess: grantResumeAccessMock,
+	hasResumeAccess: hasResumeAccessMock,
+}));
+vi.mock("../storage/service", () => ({
+	getStorageService: () => ({ delete: storageDeleteMock }),
+}));
+
+const { resumeService } = await import("./service");
+
+// A `db.update(...).set(...).where(...).returning(...)` chain that resolves to `rows`.
+const createUpdateChain = (rows: unknown[]) => {
+	const returning = vi.fn(() => Promise.resolve(rows));
+	const where = vi.fn(() => ({ returning }));
+	const set = vi.fn(() => ({ where }));
+	return { chain: { set }, set, where, returning };
+};
+
+// A `db.select(...).from(...).where(...)` chain that resolves to `rows`.
+const createSelectChain = (rows: unknown[]) => ({
+	from: () => ({ where: () => Promise.resolve(rows) }),
+});
+
+beforeEach(() => {
+	dbMock.select.mockReset();
+	dbMock.insert.mockReset();
+	dbMock.update.mockReset();
+	dbMock.delete.mockReset();
+	dbMock.transaction.mockReset();
+	hashMock.mockReset();
+	compareMock.mockReset();
+	publishResumeUpdatedMock.mockReset();
+	grantResumeAccessMock.mockReset();
+	hasResumeAccessMock.mockReset();
+	storageDeleteMock.mockReset();
+	hashMock.mockResolvedValue("hashed-password");
+	publishResumeUpdatedMock.mockResolvedValue(undefined);
+	storageDeleteMock.mockResolvedValue(true);
+});
+
+it("imports", () => {
+	expect(resumeService).toBeDefined();
+});
+
+describe("update", () => {
+	it("throws RESUME_LOCKED when the pre-read reports the resume is locked", async () => {
+		dbMock.select.mockReturnValueOnce(createSelectChain([{ isLocked: true }]));
+
+		await expect(resumeService.update({ id: "r1", userId: "u1", name: "New" })).rejects.toMatchObject({
+			code: "RESUME_LOCKED",
+		});
+	});
+
+	it("returns the updated row on success", async () => {
+		dbMock.select.mockReturnValueOnce(createSelectChain([{ isLocked: false }]));
+		const row = {
+			id: "r1",
+			name: "New",
+			slug: "slug",
+			tags: [],
+			data: {},
+			isPublic: false,
+			isLocked: false,
+			updatedAt: new Date("2026-01-01T00:00:00Z"),
+			hasPassword: false,
+		};
+		dbMock.update.mockReturnValueOnce(createUpdateChain([row]).chain);
+
+		const result = await resumeService.update({ id: "r1", userId: "u1", name: "New" });
+
+		expect(result).toEqual(row);
+		expect(publishResumeUpdatedMock).toHaveBeenCalledTimes(1);
+	});
+
+	it("throws NOT_FOUND when the UPDATE ... RETURNING matches no row", async () => {
+		dbMock.select.mockReturnValueOnce(createSelectChain([{ isLocked: false }]));
+		dbMock.update.mockReturnValueOnce(createUpdateChain([]).chain);
+
+		await expect(resumeService.update({ id: "r1", userId: "u1", name: "New" })).rejects.toMatchObject({
+			code: "NOT_FOUND",
+		});
+	});
+
+	it("maps a resume_slug_user_id_unique violation to RESUME_SLUG_ALREADY_EXISTS", async () => {
+		dbMock.select.mockReturnValueOnce(createSelectChain([{ isLocked: false }]));
+		dbMock.update.mockReturnValueOnce({
+			set: () => ({
+				where: () => ({
+					returning: () => {
+						const error = new Error("duplicate key") as Error & { cause: { constraint: string } };
+						error.cause = { constraint: "resume_slug_user_id_unique" };
+						return Promise.reject(error);
+					},
+				}),
+			}),
+		});
+
+		await expect(resumeService.update({ id: "r1", userId: "u1", slug: "taken" })).rejects.toMatchObject({
+			code: "RESUME_SLUG_ALREADY_EXISTS",
+		});
+	});
+});
+
+describe("setLocked", () => {
+	it("resolves and notifies on success (mutation: lock)", async () => {
+		dbMock.update.mockReturnValueOnce(
+			createUpdateChain([{ id: "r1", updatedAt: new Date("2026-01-01T00:00:00Z") }]).chain,
+		);
+
+		await expect(resumeService.setLocked({ id: "r1", userId: "u1", isLocked: true })).resolves.toBeUndefined();
+
+		expect(publishResumeUpdatedMock).toHaveBeenCalledTimes(1);
+		expect(publishResumeUpdatedMock).toHaveBeenCalledWith(expect.objectContaining({ mutation: "lock" }));
+	});
+
+	// Plan 003: no matching row now rejects with NOT_FOUND (previously a silent resolve).
+	it("throws NOT_FOUND when no row matches, without notifying", async () => {
+		dbMock.update.mockReturnValueOnce(createUpdateChain([]).chain);
+
+		await expect(resumeService.setLocked({ id: "r1", userId: "u1", isLocked: true })).rejects.toMatchObject({
+			code: "NOT_FOUND",
+		});
+		expect(publishResumeUpdatedMock).not.toHaveBeenCalled();
+	});
+});
+
+describe("setPassword", () => {
+	it("hashes the password then resolves and notifies on success (mutation: password)", async () => {
+		dbMock.update.mockReturnValueOnce(
+			createUpdateChain([{ id: "r1", updatedAt: new Date("2026-01-01T00:00:00Z") }]).chain,
+		);
+
+		await expect(resumeService.setPassword({ id: "r1", userId: "u1", password: "secret" })).resolves.toBeUndefined();
+
+		expect(hashMock).toHaveBeenCalledWith("secret", 10);
+		expect(publishResumeUpdatedMock).toHaveBeenCalledTimes(1);
+		expect(publishResumeUpdatedMock).toHaveBeenCalledWith(expect.objectContaining({ mutation: "password" }));
+	});
+
+	// Plan 003: no matching row now rejects with NOT_FOUND (previously a silent resolve).
+	it("throws NOT_FOUND when no row matches, without notifying", async () => {
+		dbMock.update.mockReturnValueOnce(createUpdateChain([]).chain);
+
+		await expect(resumeService.setPassword({ id: "r1", userId: "u1", password: "secret" })).rejects.toMatchObject({
+			code: "NOT_FOUND",
+		});
+		expect(publishResumeUpdatedMock).not.toHaveBeenCalled();
+	});
+});
+
+describe("removePassword", () => {
+	it("resolves and notifies on success (mutation: password)", async () => {
+		dbMock.update.mockReturnValueOnce(
+			createUpdateChain([{ id: "r1", updatedAt: new Date("2026-01-01T00:00:00Z") }]).chain,
+		);
+
+		await expect(resumeService.removePassword({ id: "r1", userId: "u1" })).resolves.toBeUndefined();
+
+		expect(publishResumeUpdatedMock).toHaveBeenCalledTimes(1);
+		expect(publishResumeUpdatedMock).toHaveBeenCalledWith(expect.objectContaining({ mutation: "password" }));
+	});
+
+	// Plan 003: no matching row now rejects with NOT_FOUND (previously a silent resolve).
+	it("throws NOT_FOUND when no row matches, without notifying", async () => {
+		dbMock.update.mockReturnValueOnce(createUpdateChain([]).chain);
+
+		await expect(resumeService.removePassword({ id: "r1", userId: "u1" })).rejects.toMatchObject({
+			code: "NOT_FOUND",
+		});
+		expect(publishResumeUpdatedMock).not.toHaveBeenCalled();
+	});
+});
+
+describe("verifyPassword", () => {
+	it("throws INVALID_PASSWORD when no matching row is found", async () => {
+		dbMock.select.mockReturnValueOnce({
+			from: () => ({ innerJoin: () => ({ where: () => Promise.resolve([]) }) }),
+		});
+
+		await expect(resumeService.verifyPassword({ slug: "s", username: "u", password: "p" })).rejects.toMatchObject({
+			code: "INVALID_PASSWORD",
+		});
+	});
+
+	it("throws INVALID_PASSWORD when bcrypt.compare returns false", async () => {
+		dbMock.select.mockReturnValueOnce({
+			from: () => ({ innerJoin: () => ({ where: () => Promise.resolve([{ id: "r1", password: "hash" }]) }) }),
+		});
+		compareMock.mockResolvedValueOnce(false);
+
+		await expect(resumeService.verifyPassword({ slug: "s", username: "u", password: "p" })).rejects.toMatchObject({
+			code: "INVALID_PASSWORD",
+		});
+	});
+
+	it("returns true and grants access when bcrypt.compare returns true", async () => {
+		dbMock.select.mockReturnValueOnce({
+			from: () => ({ innerJoin: () => ({ where: () => Promise.resolve([{ id: "r1", password: "hash" }]) }) }),
+		});
+		compareMock.mockResolvedValueOnce(true);
+		const responseHeaders = new Headers();
+
+		const result = await resumeService.verifyPassword({
+			slug: "s",
+			username: "u",
+			password: "p",
+			responseHeaders,
+		});
+
+		expect(result).toBe(true);
+		expect(grantResumeAccessMock).toHaveBeenCalledWith(responseHeaders, "r1", "hash");
+	});
+});
+
+describe("delete", () => {
+	const runTransaction = (tx: unknown) => {
+		dbMock.transaction.mockImplementationOnce(async (cb: (tx: unknown) => Promise<unknown>) => cb(tx));
+	};
+
+	it("throws NOT_FOUND when the row is missing", async () => {
+		runTransaction({
+			select: () => createSelectChain([]),
+		});
+
+		await expect(resumeService.delete({ id: "r1", userId: "u1" })).rejects.toMatchObject({ code: "NOT_FOUND" });
+	});
+
+	it("throws RESUME_LOCKED when the row is locked", async () => {
+		runTransaction({
+			select: () => createSelectChain([{ isLocked: true }]),
+		});
+
+		await expect(resumeService.delete({ id: "r1", userId: "u1" })).rejects.toMatchObject({
+			code: "RESUME_LOCKED",
+		});
+	});
+
+	it("deletes storage for screenshot and pdf keys on success", async () => {
+		const deleteWhere = vi.fn(() => Promise.resolve());
+		runTransaction({
+			select: () => createSelectChain([{ isLocked: false }]),
+			delete: () => ({ where: deleteWhere }),
+		});
+
+		await resumeService.delete({ id: "r1", userId: "u1" });
+
+		expect(deleteWhere).toHaveBeenCalledTimes(1);
+		expect(storageDeleteMock).toHaveBeenCalledWith("uploads/u1/screenshots/r1");
+		expect(storageDeleteMock).toHaveBeenCalledWith("uploads/u1/pdfs/r1");
+		expect(publishResumeUpdatedMock).toHaveBeenCalledWith(expect.objectContaining({ mutation: "delete" }));
+	});
+});
+
+describe("statistics.increment", () => {
+	it("writes both resumeStatistics and resumeStatisticsDaily inside one transaction", async () => {
+		const values = vi.fn(() => ({ onConflictDoUpdate: vi.fn(() => Promise.resolve()) }));
+		const txInsert = vi.fn(() => ({ values }));
+		dbMock.transaction.mockImplementationOnce(async (cb: (tx: unknown) => Promise<unknown>) =>
+			cb({ insert: txInsert }),
+		);
+
+		await resumeService.statistics.increment({ id: "r1", views: true });
+
+		expect(dbMock.transaction).toHaveBeenCalledTimes(1);
+		expect(txInsert).toHaveBeenCalledTimes(2);
+	});
+});

packages/api/src/features/resume/service.ts

@@ -17,6 +17,7 @@ import { getStorageService } from "../storage/service";
 import { grantResumeAccess, hasResumeAccess } from "./access";
 import { assertCanView, isOwner, redactResumeForViewer, shouldCountForStatistics } from "./access-policy";
 import { publishResumeUpdated } from "./events";
+import { clientKeyFromHeaders, shouldCountView } from "./view-dedup";
 
 type DbOrTx = typeof db | Parameters<Parameters<typeof db.transaction>[0]>[0];
 
@@ -503,7 +504,10 @@ export const resumeService = {
 		}
 
 		if (shouldCountForStatistics(resume, viewer)) {
-			await resumeService.statistics.increment({ id: resume.id, views: true });
+			const key = `${resume.id}:${clientKeyFromHeaders(input.requestHeaders)}`;
+			if (shouldCountView(key, Date.now())) {
+				await resumeService.statistics.increment({ id: resume.id, views: true });
+			}
 		}
 
 		return toSharedResumeResponse(redactResumeForViewer(resume, isOwner(resume, viewer)), resume.hasPassword);
@@ -667,7 +671,7 @@ export const resumeService = {
 			.where(and(eq(schema.resume.id, input.id), eq(schema.resume.userId, input.userId)))
 			.returning({ id: schema.resume.id, updatedAt: schema.resume.updatedAt });
 
-		if (!resume) return;
+		if (!resume) throw new ORPCError("NOT_FOUND");
 
 		await notifyResumeUpdated({
 			type: "resume.updated",
@@ -687,7 +691,7 @@ export const resumeService = {
 			.where(and(eq(schema.resume.id, input.id), eq(schema.resume.userId, input.userId)))
 			.returning({ id: schema.resume.id, updatedAt: schema.resume.updatedAt });
 
-		if (!resume) return;
+		if (!resume) throw new ORPCError("NOT_FOUND");
 
 		await notifyResumeUpdated({
 			type: "resume.updated",
@@ -730,7 +734,7 @@ export const resumeService = {
 			.where(and(eq(schema.resume.id, input.id), eq(schema.resume.userId, input.userId)))
 			.returning({ id: schema.resume.id, updatedAt: schema.resume.updatedAt });
 
-		if (!resume) return;
+		if (!resume) throw new ORPCError("NOT_FOUND");
 
 		await notifyResumeUpdated({
 			type: "resume.updated",

packages/api/src/features/resume/view-dedup.test.ts

@@ -0,0 +1,53 @@
+import { describe, expect, it } from "vitest";
+import { clientKeyFromHeaders, shouldCountView } from "./view-dedup";
+
+// `seen` is module-level state shared across tests, so each test uses a unique key.
+const WINDOW_MS = 60 * 60 * 1000;
+
+describe("shouldCountView", () => {
+	it("counts the first view, skips repeats within the window, counts again after it", () => {
+		const key = "resume-1:viewer-a";
+		const t0 = 1_000_000;
+
+		expect(shouldCountView(key, t0)).toBe(true);
+		expect(shouldCountView(key, t0 + 1)).toBe(false);
+		expect(shouldCountView(key, t0 + WINDOW_MS - 1)).toBe(false);
+		// Once now is past the window, the same key counts again.
+		expect(shouldCountView(key, t0 + WINDOW_MS + 1)).toBe(true);
+	});
+
+	it("treats different keys independently", () => {
+		const t0 = 2_000_000;
+
+		expect(shouldCountView("resume-2:viewer-a", t0)).toBe(true);
+		expect(shouldCountView("resume-2:viewer-b", t0)).toBe(true);
+		expect(shouldCountView("resume-2:viewer-a", t0 + 1)).toBe(false);
+	});
+});
+
+describe("clientKeyFromHeaders", () => {
+	it("derives distinct keys from distinct trusted-IP headers", () => {
+		const a = clientKeyFromHeaders(new Headers({ "X-Forwarded-For": "1.1.1.1" }));
+		const b = clientKeyFromHeaders(new Headers({ "X-Forwarded-For": "2.2.2.2" }));
+
+		expect(a).toBe("ip:1.1.1.1");
+		expect(a).not.toBe(b);
+	});
+
+	it("uses the first IP from a comma-delimited proxy chain", () => {
+		const key = clientKeyFromHeaders(new Headers({ "X-Forwarded-For": "3.3.3.3, 10.0.0.1" }));
+		expect(key).toBe("ip:3.3.3.3");
+	});
+
+	it("falls back to a stable user-agent fingerprint when no trusted IP header is present", () => {
+		const headers = new Headers({ "user-agent": "Mozilla/5.0", "accept-language": "en-US,en" });
+
+		const first = clientKeyFromHeaders(headers);
+		const second = clientKeyFromHeaders(headers);
+
+		expect(first).toBe(second);
+		expect(first.startsWith("fp:")).toBe(true);
+		// A different UA yields a different fallback key.
+		expect(clientKeyFromHeaders(new Headers({ "user-agent": "curl/8" }))).not.toBe(first);
+	});
+});

packages/api/src/features/resume/view-dedup.ts

@@ -0,0 +1,45 @@
+import { TRUSTED_IP_HEADERS } from "@reactive-resume/utils/rate-limit";
+
+// ponytail: in-memory per-process dedup window. Single-instance is the default deploy; for
+// multi-instance, swap the Map for a Redis SETNX+EXPIRE keyed the same way (REDIS_URL already
+// exists in env). Upgrade only if you scale out — each instance dedups independently otherwise.
+const WINDOW_MS = 60 * 60 * 1000; // 1 hour
+const MAX_ENTRIES = 50_000; // bound the Map; prune expired entries before growing past this.
+
+const seen = new Map<string, number>(); // key -> expiry timestamp
+
+/**
+ * Returns `true` at most once per `key` per window. `now` is a parameter (not `Date.now()`)
+ * so callers stay testable with a driven clock.
+ */
+export function shouldCountView(key: string, now: number): boolean {
+	const expiry = seen.get(key);
+	if (expiry !== undefined && expiry > now) return false;
+
+	if (seen.size >= MAX_ENTRIES) {
+		for (const [k, exp] of seen) {
+			if (exp <= now) seen.delete(k);
+		}
+	}
+
+	seen.set(key, now + WINDOW_MS);
+	return true;
+}
+
+// Mirrors the rate-limit middleware's client-key derivation so dedup and rate limiting agree on
+// "who is this viewer": trusted proxy IP first, then a user-agent + language fingerprint fallback.
+export function clientKeyFromHeaders(headers: Headers): string {
+	for (const headerName of TRUSTED_IP_HEADERS) {
+		const raw = headers.get(headerName)?.trim();
+		if (!raw) continue;
+
+		// Some proxies provide a comma-delimited chain; the first item is the original client.
+		const ip = raw.split(",")[0]?.trim();
+		if (ip) return `ip:${ip}`;
+	}
+
+	const userAgent = headers.get("user-agent")?.trim() ?? "unknown";
+	const language = headers.get("accept-language")?.split(",")[0]?.trim() ?? "none";
+
+	return `fp:${userAgent.slice(0, 64)}:${language.slice(0, 16)}`;
+}