feat(moderator): give apps/moderator a test suite, and restore the report-actioning coverage #3573 lost

`apps/moderator` was the only workspace member with no vitest config, so nothing selected it and it
had zero tests. That is why #4179 had to DELETE the preview specs covering report actioning rather
than move them: #3573 removed `report.getAll`/`report.setStatus` from the main app, and there was no
suite here for the assertions to land in.

Adds the project (`app:moderator`, picked up by the existing `apps/*/vitest.config.*` glob and by
scripts/ci/assert-workspace-suites-ran.mjs) and 76 tests over the report queue.

WHAT THE TESTS PIN. Every case is a defect the service's own comments record as having shipped: a
report that no longer exists reporting success, `changed:false` collapsing so re-actioning
double-rewards the reporters, a sweep counting its loop instead of its results, filters silently
omitted. The Kysely fake answers from the RECORDED CHAIN rather than a fixture, which is what makes
it able to see query shape at all — without `.returning()`, real Kysely resolves an UPDATE to a
truthy `UpdateResult`, so a fake that resolves to a fixture cannot tell that from correct code.

DB-BACKED TIER. Most of `reports.service.ts` is raw `sql` assembled from `REPORT_ENTITIES`, where
not one identifier is typechecked and the unit tier mocks the database away. `src/test/explain-
harness.ts` compiles through Kysely's DummyDriver and sends only the SQL text as EXPLAIN WITHOUT
ANALYZE — parsed and planned, never executed, safe for writes. Verified it catches what nothing else
can: a wrong `fk`, a wrong report table, a typo in the comment deep-link CASE and a wrong resolver
join each fail with the Postgres error. Skips with a warning where no database is configured.
`DATABASE_REPLICA_URL` is deliberately withheld from the test env so a suite that forgets to mock
`$lib/server/db` throws on import instead of connecting.

FIXES A LIVE DEFECT the tests found. `updateReportNotes` discarded its result, so saving notes
against a report deleted mid-edit answered `{success:true}` and dropped the text — a direct
violation of this app's own "treat 0 affected rows as a failure" rule. It now reports the outcome,
and the page reloads the queue on it so the row leaves the screen instead of accepting more clicks.
`setStatus`'s existing "no longer exists" path joins the same outcome.

The page moves onto `FormState`, which the other 23 pages already use. Its doc describes exactly the
failure a page-level `form` prop produces here — one panel showing another's refusal — and holding
the result per form removes that routing question rather than answering it. Refusals now render
beside the form that caused them; a `gone` refusal closes the sheet, so that one goes to a toast,
which is the only surface that outlives it. Refusal payloads move from `message` to `error`, the key
`FormState` reads and the one 82 of the app's 97 refusals already use.

`vitest` is declared rather than resolved through the root, matching apps/auth; the lockfile entry
comes with it so `pnpm install --frozen-lockfile` still passes.

Closes #4182

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
briant
2026-08-20 13:06:55 -06:00
parent 6c9904b389
commit baf9a54b99
15 changed files with 937 additions and 25 deletions
+6
View File
@@ -59,6 +59,12 @@ provenance is the only reason it differs from the standard at all.
- **Two databases.** `$lib/server/db.ts` is the main app's Postgres; `getModeratorDb()` is moderation
data that never lived there (notes, strikes, help requests), typed by hand in
`moderator-db-types.ts` because those tables are not in the Prisma schema.
- **The test suite has a DB-backed tier, and `vitest.config.ts` feeds it the root `.env`.** See the
Tests section of the standard for the shape. Two things specific to this app: `DATABASE_REPLICA_URL`
is deliberately **not** surfaced, so a suite that forgets to mock `$lib/server/db` throws on import
instead of connecting to whatever that URL is (do not "fix" such a failure by adding the variable);
and the report queries are the reason the tier exists — most of `reports.service.ts` is raw `sql`
assembled from `REPORT_ENTITIES`, where not one identifier is typechecked.
- **When porting, classify every source query before writing code**, and add the fourth
export-vs-build review the standard describes. Three code reviews pass cleanly over a faithful
implementation of the wrong thing — that is how four capabilities were missed on one page after
+2
View File
@@ -10,6 +10,7 @@
"prepare": "svelte-kit sync",
"check": "svelte-kit sync && svelte-check --tsconfig ./tsconfig.json",
"typecheck": "svelte-check --tsconfig ./tsconfig.json",
"test": "vitest run",
"lint": "eslint . --ext .js,.ts,.svelte --cache --cache-strategy metadata"
},
"dependencies": {
@@ -50,6 +51,7 @@
"svelte-eslint-parser": "^0.43.0",
"tailwindcss": "^4.1.0",
"typescript": "^5.9.2",
"vitest": "^4.0.18",
"vite": "^7.0.4"
}
}
@@ -0,0 +1,222 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { ReportStatus } from '$lib/reports';
/**
* Replaces the end-to-end coverage #4179 had to drop when #3573 deleted `report.setStatus` from the
* main app. Every case below pins a defect that shipped once — see the service's own comments.
*/
const updateSpy = vi.fn();
const recordModActivity = vi.fn();
const rewardReportReporters = vi.fn();
let existingReportId: number | undefined;
/** `undefined` means the guarded UPDATE matched nothing, i.e. the status was already set. */
let updateResult: { userId: number; alsoReportedBy: number[] | null } | undefined;
let affectedRows: number;
type Call = [string, unknown[]];
/**
* A stand-in for the two Kysely chains this module builds.
*
* 🔴 It must answer from the RECORDED CHAIN, never from a variable alone, or it cannot see the query
* shape at all. The case that forces this: without `.returning()`, real Kysely resolves an UPDATE to
* `UpdateResult { numUpdatedRows }` — TRUTHY — so `changed: !!updated` would be permanently true, the
* 409 path unreachable, and every re-action would reward the reporters again. A fake that resolves to
* a fixture cannot distinguish that from correct code. Hence:
* - the UPDATE yields the returned row only when `returning` was called, and otherwise the truthy
* `UpdateResult` real Kysely would give;
* - both chains match on the recorded `where`, so a query scoped to the wrong row — or to no row at
* all — resolves to nothing rather than passing.
*/
function chain(record: (calls: Call[]) => void, resolve: (calls: Call[]) => unknown) {
const calls: Call[] = [];
const builder: Record<string, unknown> = {};
for (const method of ['select', 'set', 'where', 'returning']) {
builder[method] = (...args: unknown[]) => {
calls.push([method, args]);
return builder;
};
}
builder.executeTakeFirst = async () => {
record(calls);
return resolve(calls);
};
return builder;
}
const wheres = (calls: Call[]) => calls.filter(([m]) => m === 'where').map(([, a]) => a);
vi.mock('../db', () => ({
dbRead: {},
dbWrite: {
selectFrom: (table: string) =>
chain(
() => undefined,
(calls) => {
expect(table).toBe('Report');
const matchesRow = wheres(calls).some(
([column, op, value]) => column === 'id' && op === '=' && value === existingReportId
);
return existingReportId !== undefined && matchesRow
? { id: existingReportId }
: undefined;
}
),
updateTable: (table: string) =>
chain(
(calls) => updateSpy(calls),
(calls) => {
expect(table).toBe('Report');
// Scoped, like the SELECT. Without this an UPDATE that lost `.where('id','=',id)` — which
// rewrites the column on EVERY report in the table — resolved exactly like a correct one.
expect(
wheres(calls).map(([column]) => column),
'an UPDATE here must be scoped to a single report'
).toContain('id');
const returning = calls.some(([m]) => m === 'returning');
if (!returning) return { numUpdatedRows: BigInt(affectedRows) };
return updateResult;
}
),
},
}));
vi.mock('../mod-activity', () => ({ recordModActivity }));
vi.mock('../rewards', () => ({ rewardReportReporters }));
const { setReportStatus, updateReportNotes } = await import('../reports.service');
beforeEach(() => {
vi.clearAllMocks();
existingReportId = 1;
updateResult = { userId: 10, alsoReportedBy: null };
affectedRows = 1;
});
/** The `where` arguments the UPDATE was built with, as `[column, op, value]` triples. */
const updateWheres = () => wheres((updateSpy.mock.calls[0]?.[0] as Call[]) ?? []);
const updateSet = () =>
(((updateSpy.mock.calls[0]?.[0] as Call[]) ?? []).find(([m]) => m === 'set')?.[1][0] ??
{}) as Record<string, unknown>;
describe('setReportStatus', () => {
it('refuses a report that no longer exists instead of reporting success', async () => {
existingReportId = undefined;
const result = await setReportStatus({ id: 404, status: ReportStatus.Actioned, userId: 7 });
expect(result).toEqual({ ok: false, error: expect.stringContaining('no longer exists') });
// Nothing may follow the existence check — a forged id must not leave a ModActivity trail.
expect(updateSpy).not.toHaveBeenCalled();
expect(recordModActivity).not.toHaveBeenCalled();
expect(rewardReportReporters).not.toHaveBeenCalled();
});
it('looks the report up by the id it was asked about', async () => {
existingReportId = 55;
expect(await setReportStatus({ id: 55, status: ReportStatus.Actioned, userId: 7 })).toEqual({
ok: true,
changed: true,
});
expect(await setReportStatus({ id: 56, status: ReportStatus.Actioned, userId: 7 })).toEqual({
ok: false,
error: expect.stringContaining('no longer exists'),
});
});
it('reports changed:false when another moderator already set that status', async () => {
updateResult = undefined; // the `status != status` guard matched nothing
const result = await setReportStatus({ id: 1, status: ReportStatus.Actioned, userId: 7 });
expect(result).toEqual({ ok: true, changed: false });
});
it('does not re-reward a report that was already Actioned', async () => {
updateResult = undefined;
await setReportStatus({ id: 1, status: ReportStatus.Actioned, userId: 7 });
expect(rewardReportReporters).not.toHaveBeenCalled();
});
it('still records the review when the status was already set', async () => {
// Deliberate: the moderator did work the report. Only a NONEXISTENT one is guarded against.
updateResult = undefined;
await setReportStatus({ id: 1, status: ReportStatus.Actioned, userId: 7 });
expect(recordModActivity).toHaveBeenCalledTimes(1);
});
it('keeps the status guard on the UPDATE, which is what makes changed:false reachable', async () => {
await setReportStatus({ id: 1, status: ReportStatus.Actioned, userId: 7 });
expect(updateWheres()).toContainEqual(['status', '!=', ReportStatus.Actioned]);
expect(updateWheres()).toContainEqual(['id', '=', 1]);
});
it('rewards the filer AND every also-reporter when the report is actioned', async () => {
updateResult = { userId: 10, alsoReportedBy: [11, 12] };
const result = await setReportStatus({
id: 1,
status: ReportStatus.Actioned,
userId: 7,
ip: '203.0.113.9',
});
expect(result).toEqual({ ok: true, changed: true });
expect(rewardReportReporters).toHaveBeenCalledWith({
reportId: 1,
reporterIds: [10, 11, 12],
ip: '203.0.113.9',
});
});
it('stamps previouslyReviewedCount when actioning', async () => {
await setReportStatus({ id: 1, status: ReportStatus.Actioned, userId: 7 });
expect(updateSet()).toHaveProperty('previouslyReviewedCount');
});
it('rewards nobody for a non-Actioned status, and does not stamp previouslyReviewedCount', async () => {
await setReportStatus({ id: 1, status: ReportStatus.Unactioned, userId: 7 });
expect(rewardReportReporters).not.toHaveBeenCalled();
expect(updateSet()).not.toHaveProperty('previouslyReviewedCount');
expect(updateSet()).toMatchObject({ status: ReportStatus.Unactioned, statusSetBy: 7 });
});
it('records who actioned it, on every path that touched the row', async () => {
existingReportId = 42;
await setReportStatus({ id: 42, status: ReportStatus.Actioned, userId: 7 });
expect(recordModActivity).toHaveBeenCalledWith({
userId: 7,
entityType: 'report',
entityId: 42,
activity: 'review',
});
});
});
describe('updateReportNotes', () => {
it('reports the notes stored when the report was there', async () => {
expect(await updateReportNotes({ id: 1, internalNotes: 'spam ring' })).toEqual({ ok: true });
});
it('reports `gone` when the row was deleted while the notes were being typed', async () => {
affectedRows = 0;
expect(await updateReportNotes({ id: 1, internalNotes: 'spam ring' })).toEqual({
ok: false,
gone: true,
});
});
});
@@ -0,0 +1,99 @@
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest';
import { reportEntities, ReportReason, ReportStatus } from '$lib/reports';
import { explainHarness } from '../../../test/explain-harness';
/**
* Every statement PLANNED against the live schema, none executed — see `src/test/explain-harness.ts`.
*
* Does NOT cover results: EXPLAIN cannot show that the `to` bound excludes, that `ilike` anchors as a
* prefix, or that the offset paginates. Those need rows, and so a disposable database.
*/
const h = explainHarness();
// Both point at the same DummyDriver: nothing this file compiles can reach a connection.
vi.mock('../db', () => ({ dbRead: h.db, dbWrite: h.db }));
vi.mock('../mod-activity', () => ({ recordModActivity: vi.fn() }));
vi.mock('../rewards', () => ({ rewardReportReporters: vi.fn() }));
const service = await import('../reports.service');
beforeEach(() => h.reset());
afterAll(() => h.destroy());
/** Plan everything the call compiled, and fail with the plan text if Postgres rejects a statement. */
async function plans() {
const out = await h.explainAll();
expect(out.length).toBeGreaterThan(0);
return out;
}
describe.skipIf(!h.hasDb)('report queries plan against the real schema', () => {
// Driven off `reportEntities` so a newly added type is covered without editing this file.
it.each(reportEntities)('getReports(%s) — queue page and its count', async (type) => {
await service.getReports({ type, statuses: 'all', reasons: 'all' });
expect(h.queries.length).toBe(2);
await plans();
});
it.each(reportEntities)('getReportHistory(%s)', async (type) => {
await service.getReportHistory(type);
await plans();
});
it('getReports with every filter applied at once', async () => {
// Filters append to one builder, so a clash only reachable in combination needs them together.
await service.getReports({
type: 'image',
page: 3,
limit: 20,
statuses: [ReportStatus.Pending, ReportStatus.Processing],
reasons: [ReportReason.NSFW, ReportReason.TOSViolation],
reportedBy: 'alice',
from: new Date('2026-01-01T00:00:00Z'),
to: new Date('2026-02-01T00:00:00Z'),
});
await plans();
});
// The only fragment that joins Thread to itself, and only these two types emit it.
it.each(['comment', 'commentV2'] as const)(
'getReports(%s) resolves the deep-link CASE over Thread',
async (type) => {
await service.getReports({ type, statuses: 'all', reasons: 'all' });
const [, page] = h.queries;
expect(page.sql).toMatch(/highlight/);
await plans();
}
);
it('getReportCounts — the materialized CTE and all fifteen union branches', async () => {
await service.getReportCounts();
const [counts] = h.queries;
// Per-branch joins back to Report seq-scanned it once per entity type; hence the CTE.
expect(counts.sql).toMatch(/with .*open_reports as materialized/i);
expect(counts.sql.match(/union all/gi)).toHaveLength(reportEntities.length - 1);
await plans();
});
it('getMostReported — the LIMIT-in-a-CTE shape with its seventeen subplans', async () => {
// Subplans resolve OUTSIDE the CTE: Postgres cannot project through a Sort, so flattening this
// evaluates them for every pending report of the week.
await service.getMostReported(20, 1_767_225_600_000);
await plans();
});
});
// Opt-in, so a checkout without a database must not read as covered. Runs either way.
describe('the EXPLAIN tier reports whether it ran', () => {
it('is wired to a database, or is visibly skipped', () => {
if (!h.hasDb) {
console.warn(
'[reports.queries.explain] no TEST_DATABASE_URL/DATABASE_URL — the report SQL was NOT planned.'
);
}
expect(typeof h.hasDb).toBe('boolean');
});
});
@@ -281,14 +281,26 @@ export async function setReportStatus({
return { ok: true as const, changed: !!updated };
}
/**
* Reports whether the row was there, rather than throwing. Zero rows here means the report was
* deleted while the moderator was typing — a race, not an error on their part — and the caller has
* to be able to tell the difference, because the right answer is to take the row off their screen
* rather than to leave them staring at notes that were silently dropped.
*/
export async function updateReportNotes({
id,
internalNotes,
}: {
id: number;
internalNotes: string | null;
}) {
await dbWrite.updateTable('Report').set({ internalNotes }).where('id', '=', id).execute();
}): Promise<{ ok: true } | { ok: false; gone: true }> {
const result = await dbWrite
.updateTable('Report')
.set({ internalNotes })
.where('id', '=', id)
.executeTakeFirst();
return result.numUpdatedRows > 0n ? { ok: true } : { ok: false, gone: true };
}
export type MostReportedRow = {
@@ -68,7 +68,7 @@ export const actions: Actions = {
const data = await request.formData();
const id = Number(data.get('id'));
const status = String(data.get('status'));
if (!id || !isStatus(status)) return fail(400, { message: 'Invalid input' });
if (!id || !isStatus(status)) return fail(400, { error: 'Invalid input' });
// `setReportStatus` RETURNS its outcome; discarding it reported success for a report another
// moderator had already actioned or deleted. Same defect the batch sweep had — and this is the
@@ -79,9 +79,11 @@ export const actions: Actions = {
userId: locals.user.id,
ip: getClientAddress(),
});
if (!result.ok) return fail(400, { message: result.error });
// `gone` rather than a bare message: the page reloads the queue on it, so the row the moderator
// is looking at leaves the screen instead of sitting there accepting further clicks.
if (!result.ok) return fail(410, { error: result.error, gone: true });
if (!result.changed)
return fail(409, { message: 'Someone else already set that status. Reload.' });
return fail(409, { error: 'Someone else already set that status. Reload.' });
return { success: true };
},
/**
@@ -94,7 +96,7 @@ export const actions: Actions = {
actionResolvedPosts: async ({ locals, getClientAddress }) => {
const ids = await getResolvedPostReportIds();
if (!ids.length)
return fail(400, { message: 'No post reports are already resolved by content.' });
return fail(400, { error: 'No post reports are already resolved by content.' });
// `setReportStatus` RETURNS its outcome rather than throwing: `ok:false` when the report is gone,
// and `ok:true, changed:false` when someone else already put it in this status. Counting the loop
@@ -122,25 +124,29 @@ export const actions: Actions = {
*/
removePlacement: async ({ request, locals }) => {
// Acting on reported content, not merely reading the queue — gated on its own path.
if (!canAccess(locals.user, '/reports')) return fail(403, { message: 'Not permitted.' });
if (!canAccess(locals.user, '/reports')) return fail(403, { error: 'Not permitted.' });
const data = await request.formData();
const placementId = Number(data.get('placementId'));
if (!Number.isInteger(placementId) || placementId <= 0)
return fail(400, { message: 'Invalid placement.' });
return fail(400, { error: 'Invalid placement.' });
const result = await removePlacement({ placementId, moderatorId: locals.user.id });
if (!result.ok) return fail(400, { message: result.error });
if (!result.ok) return fail(400, { error: result.error });
return { success: true, placementRemoved: placementId };
},
saveNotes: async ({ request }) => {
const data = await request.formData();
const id = Number(data.get('id'));
if (!id) return fail(400, { message: 'Invalid input' });
if (!id) return fail(400, { error: 'Invalid input' });
const internalNotes = String(data.get('internalNotes') ?? '').trim() || null;
await updateReportNotes({ id, internalNotes });
// Discarding the outcome reports success for notes that were never stored, because the report
// was deleted while they were being typed.
const result = await updateReportNotes({ id, internalNotes });
if (!result.ok)
return fail(410, { error: 'That report was deleted while you were editing.', gone: true });
return { success: true };
},
};
@@ -1,6 +1,6 @@
<script lang="ts">
import { enhance } from '$app/forms';
import { goto } from '$app/navigation';
import { goto, invalidateAll } from '$app/navigation';
import { page } from '$app/state';
import { IconExternalLink } from '@tabler/icons-svelte';
import {
@@ -28,6 +28,7 @@
SheetHeader,
SheetTitle,
} from '@civitai/ui/components/ui/sheet/index.js';
import { toast } from '@civitai/ui/components/ui/sonner/index.js';
import { Textarea } from '@civitai/ui/components/ui/textarea/index.js';
import { Input } from '@civitai/ui/components/ui/input/index.js';
import { MultiCombobox } from '@civitai/ui/components/ui/multi-combobox/index.js';
@@ -41,10 +42,29 @@
reportedPlacementId,
reportDetail,
} from '$lib/reports';
import type { ActionResult } from '@sveltejs/kit';
import { FormState } from '$lib/form-state.svelte';
import type { ActionData, PageData } from './$types';
let { data, form }: { data: PageData; form: ActionData } = $props();
/**
* A `gone` refusal means the report was deleted underneath the moderator, so the queue reloads and
* the row leaves — which closes the sheet through `selected`, taking the panel's own error with it.
* A toast is the only surface that outlives that, and without one the row simply vanishes.
* Every other refusal leaves the sheet open to fix and retry, which is `FormState`'s default.
*/
const reloadIfGone = (result: ActionResult) => {
if (result.type !== 'failure' || !result.data?.gone) return;
toast.error(String(result.data.error ?? 'That report no longer exists.'));
invalidateAll();
};
const statusForm = new FormState({ onSuccess: null, reload: true, onSettled: reloadIfGone });
const notesForm = new FormState({ onSuccess: null, reload: true, onSettled: reloadIfGone });
const placementForm = new FormState({ onSuccess: null, reload: true });
const sweepForm = new FormState({ onSuccess: null, reload: true });
let selectedId = $state<number | null>(null);
// Armed per placement id, so closing the sheet and opening another report cannot leave a live
// destructive button pointed at the previous one.
@@ -97,6 +117,10 @@
function openDetails(id: number) {
selectedId = id;
// A refusal belongs to the report it was raised on; opening another must not inherit it.
statusForm.error = null;
notesForm.error = null;
placementForm.error = null;
}
</script>
@@ -108,22 +132,24 @@
<!-- Retool's `ActionAllPostReports`. Only meaningful here: the query keys on every image in the post
already being blocked, which is a post-shaped question. -->
{#if data.type === 'post'}
<form method="POST" action="?/actionResolvedPosts" use:enhance class="mb-4">
<form method="POST" action="?/actionResolvedPosts" use:enhance={sweepForm.enhance} class="mb-4">
<Button type="submit" variant="outline" size="sm">Action reports already resolved by content</Button>
<span class="ml-2 text-xs text-muted-foreground">
Pending reports whose post is entirely blocked already.
</span>
</form>
<!-- A bulk action that reports nothing is indistinguishable from one that did nothing. -->
{#if form && 'message' in form && form.message}
<p class="mb-4 text-sm text-amber-300" role="status">{form.message}</p>
{:else if form && 'actioned' in form && form.actioned != null}
{#if form && 'actioned' in form && form.actioned != null}
<p class="mb-4 text-sm text-green-300" role="status">
Actioned {form.actioned} of {form.found}{form.skipped ? `, ${form.skipped} already handled` : ''}{form.more ? ' — more remain, run it again.' : ''}
</p>
{/if}
{/if}
{#if sweepForm.error}
<p class="mb-4 text-sm text-red-300" role="alert">{sweepForm.error}</p>
{/if}
<div class="mb-4 flex flex-wrap items-end gap-x-6 gap-y-3">
<div class="flex flex-col gap-1">
<span class="text-xs font-medium text-muted-foreground">Status</span>
@@ -314,9 +340,11 @@
sticker off the image for everyone.
</p>
{#if confirmingPlacement === placementId}
<form method="POST" action="?/removePlacement" use:enhance class="flex flex-wrap gap-2">
<form method="POST" action="?/removePlacement" use:enhance={placementForm.enhance} class="flex flex-wrap gap-2">
<input type="hidden" name="placementId" value={placementId} />
<Button type="submit" size="sm" variant="destructive">Yes, remove it</Button>
<Button type="submit" size="sm" variant="destructive" disabled={placementForm.submitting}>
Yes, remove it
</Button>
<Button
type="button"
size="sm"
@@ -326,7 +354,11 @@
Cancel
</Button>
</form>
{:else}
{/if}
{#if placementForm.error}
<p class="mt-2 text-sm text-red-300" role="alert">{placementForm.error}</p>
{/if}
{#if confirmingPlacement !== placementId}
<Button size="sm" variant="destructive" onclick={() => (confirmingPlacement = placementId)}>
Remove placement
</Button>
@@ -334,7 +366,7 @@
</div>
{/if}
<form method="POST" action="?/setStatus" use:enhance class="flex flex-col gap-2">
<form method="POST" action="?/setStatus" use:enhance={statusForm.enhance} class="flex flex-col gap-2">
<input type="hidden" name="id" value={selected.id} />
<span class="text-sm font-medium">Status</span>
<div class="flex flex-wrap gap-2">
@@ -351,17 +383,33 @@
</Button>
{/each}
</div>
{#if statusForm.error}
<p class="text-sm text-red-300" role="alert">{statusForm.error}</p>
{/if}
</form>
<form method="POST" action="?/saveNotes" use:enhance class="flex flex-col gap-2">
<form method="POST" action="?/saveNotes" use:enhance={notesForm.enhance} class="flex flex-col gap-2">
<input type="hidden" name="id" value={selected.id} />
<span class="text-sm font-medium">Internal notes</span>
<Textarea name="internalNotes" rows={3} value={selected.internalNotes ?? ''} />
<Button type="submit" size="sm" class="self-end">Save notes</Button>
<Button type="submit" size="sm" class="self-end" disabled={notesForm.submitting}>
Save notes
</Button>
{#if notesForm.error}
<p class="text-sm text-red-300" role="alert">{notesForm.error}</p>
{/if}
</form>
</div>
{:else}
<div class="p-6 text-sm text-muted-foreground">Report updated.</div>
<!-- Reached while the sheet plays its exit animation with `selected` already null, which happens
two ways: an action emptied the row (bits-ui does not fire `onOpenChange` for a
parent-driven close, so `selectedId` still holds it) or the moderator clicked the X (which
does, so it is null). Only the first is an update, and only a `success` outcome is one at
all — `gone` means the report was deleted and the edit dropped. Both conditions, or this
says "Report updated." at someone who read a report and closed it. -->
{#if selectedId !== null && form && 'success' in form && form.success}
<div class="p-6 text-sm text-muted-foreground">Report updated.</div>
{/if}
{/if}
</SheetContent>
</Sheet>
@@ -0,0 +1,225 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { ReportStatus } from '$lib/reports';
/**
* What only the action layer decides: that a service outcome is TRANSLATED rather than discarded.
* Discarding it is the defect the actions' own comments record success for a report someone else
* had already actioned, and "actioned N of N" for a run that changed nothing.
*/
const setReportStatus = vi.fn();
const getReports = vi.fn(async () => ({ items: [], totalItems: 0, page: 1, limit: 20 }));
const updateReportNotes = vi.fn();
const getResolvedPostReportIds = vi.fn(async () => [] as number[]);
const removePlacement = vi.fn();
const canAccess = vi.fn(() => true);
vi.mock('$lib/server/reports.service', () => ({
getReports,
setReportStatus,
updateReportNotes,
}));
vi.mock('$lib/server/moderation-board.service', () => ({ getResolvedPostReportIds }));
vi.mock('$lib/server/user-actions.service', () => ({ removePlacement }));
// Stubbed for a different reason than the others: no database, but `canAccess` reads a grants store
// only the request hook fills, so the real one answers false for everything here.
vi.mock('$lib/server/access', () => ({ canAccess }));
const { actions } = await import('../+page.server');
const MOD = { id: 7 };
/**
* A REAL `FormData`, not a Map: `Map.get` gives `undefined` where `FormData.get` gives `null`, and
* `Number()` maps those to NaN and 0 opposite sides of `removePlacement`'s `Number.isInteger`.
*/
const event = (form: Record<string, string> = {}) => {
const data = new FormData();
for (const [key, value] of Object.entries(form)) data.append(key, value);
return {
request: { formData: async () => data },
locals: { user: MOD },
getClientAddress: () => '203.0.113.9',
} as never;
};
/** A `fail()` result, unwrapped to the shape an action test cares about. */
const failure = (result: unknown) => {
const r = result as { status: number; data: { error: string } };
return { status: r.status, error: r.data.error };
};
beforeEach(() => {
vi.clearAllMocks();
canAccess.mockReturnValue(true);
setReportStatus.mockResolvedValue({ ok: true, changed: true });
updateReportNotes.mockResolvedValue({ ok: true });
});
describe('setStatus action', () => {
it('actions a report and reports success', async () => {
const result = await actions.setStatus(event({ id: '42', status: ReportStatus.Actioned }));
expect(setReportStatus).toHaveBeenCalledWith({
id: 42,
status: ReportStatus.Actioned,
userId: 7,
ip: '203.0.113.9',
});
expect(result).toEqual({ success: true });
});
it('surfaces a 410 gone when the report vanished, rather than reporting success', async () => {
setReportStatus.mockResolvedValue({
ok: false,
error: 'That report no longer exists. Reload.',
});
const result = await actions.setStatus(event({ id: '42', status: ReportStatus.Actioned }));
// `gone` is what makes the page reload; a bare message would render and change nothing.
expect(failure(result)).toEqual({
status: 410,
error: 'That report no longer exists. Reload.',
});
expect((result as { data: { gone?: boolean } }).data.gone).toBe(true);
});
it('surfaces a 409 when another moderator already set that status', async () => {
setReportStatus.mockResolvedValue({ ok: true, changed: false });
const result = await actions.setStatus(event({ id: '42', status: ReportStatus.Actioned }));
expect(failure(result).status).toBe(409);
});
it('rejects a status that is not a real ReportStatus without reaching the service', async () => {
const result = await actions.setStatus(event({ id: '42', status: 'Deleted' }));
expect(failure(result).status).toBe(400);
expect(setReportStatus).not.toHaveBeenCalled();
});
it('rejects a missing id without reaching the service', async () => {
const result = await actions.setStatus(event({ status: ReportStatus.Actioned }));
expect(failure(result).status).toBe(400);
expect(setReportStatus).not.toHaveBeenCalled();
});
});
describe('actionResolvedPosts action', () => {
it('counts what actually changed, not how many ids it looped over', async () => {
getResolvedPostReportIds.mockResolvedValue([1, 2, 3]);
setReportStatus
.mockResolvedValueOnce({ ok: true, changed: true })
.mockResolvedValueOnce({ ok: true, changed: false })
.mockResolvedValueOnce({ ok: false, error: 'gone' });
const result = await actions.actionResolvedPosts(event());
expect(result).toMatchObject({ success: true, actioned: 1, skipped: 2, found: 3 });
});
it('sweeps as Actioned — the content was removed, so the reports were right', async () => {
getResolvedPostReportIds.mockResolvedValue([1]);
await actions.actionResolvedPosts(event());
expect(setReportStatus).toHaveBeenCalledWith(
expect.objectContaining({ status: ReportStatus.Actioned })
);
});
it('flags a full batch as probably having more behind it', async () => {
getResolvedPostReportIds.mockResolvedValue(Array.from({ length: 500 }, (_, i) => i + 1));
const result = await actions.actionResolvedPosts(event());
expect(result).toMatchObject({ more: true, found: 500 });
});
it('does not claim a sweep when there was nothing to sweep', async () => {
getResolvedPostReportIds.mockResolvedValue([]);
const result = await actions.actionResolvedPosts(event());
expect(failure(result).status).toBe(400);
expect(setReportStatus).not.toHaveBeenCalled();
});
});
describe('removePlacement action', () => {
it('is gated on /reports even though the page load is not', async () => {
canAccess.mockReturnValue(false);
const result = await actions.removePlacement(event({ placementId: '5' }));
expect(canAccess).toHaveBeenCalledWith(MOD, '/reports');
expect(failure(result).status).toBe(403);
expect(removePlacement).not.toHaveBeenCalled();
});
it('delegates to the service so escrow settles in one place', async () => {
removePlacement.mockResolvedValue({ ok: true });
const result = await actions.removePlacement(event({ placementId: '5' }));
expect(removePlacement).toHaveBeenCalledWith({ placementId: 5, moderatorId: 7 });
expect(result).toMatchObject({ success: true, placementRemoved: 5 });
});
it('rejects a non-positive placement id without reaching the service', async () => {
const result = await actions.removePlacement(event({ placementId: '0' }));
expect(failure(result).status).toBe(400);
expect(removePlacement).not.toHaveBeenCalled();
});
it('rejects a missing placement id — absent reads as 0, not as NaN', async () => {
const result = await actions.removePlacement(event());
expect(failure(result).status).toBe(400);
expect(removePlacement).not.toHaveBeenCalled();
});
it('surfaces an escrow failure rather than reporting the placement removed', async () => {
// A refusal here is money that did not move.
removePlacement.mockResolvedValue({ ok: false, error: 'Escrow already settled.' });
const result = await actions.removePlacement(event({ placementId: '5' }));
expect(failure(result)).toEqual({ status: 400, error: 'Escrow already settled.' });
});
});
describe('saveNotes action', () => {
it('persists the notes it was given', async () => {
const result = await actions.saveNotes(event({ id: '42', internalNotes: ' spam ring ' }));
expect(updateReportNotes).toHaveBeenCalledWith({ id: 42, internalNotes: 'spam ring' });
expect(result).toEqual({ success: true });
});
it('stores empty notes as null rather than an empty string', async () => {
await actions.saveNotes(event({ id: '42', internalNotes: ' ' }));
expect(updateReportNotes).toHaveBeenCalledWith({ id: 42, internalNotes: null });
});
it('rejects a missing id without reaching the service', async () => {
const result = await actions.saveNotes(event({ internalNotes: 'x' }));
expect(failure(result).status).toBe(400);
expect(updateReportNotes).not.toHaveBeenCalled();
});
it('reports notes that were never stored, rather than a green save over nothing', async () => {
updateReportNotes.mockResolvedValue({ ok: false, gone: true });
const result = await actions.saveNotes(event({ id: '42', internalNotes: 'spam ring' }));
expect(failure(result).status).toBe(410);
expect((result as { data: { gone?: boolean } }).data.gone).toBe(true);
});
});
@@ -0,0 +1,134 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { isHttpError, isRedirect } from '@sveltejs/kit';
import {
DEFAULT_REPORT_REASONS,
DEFAULT_REPORT_STATUSES,
ReportReason,
ReportStatus,
reportReasons,
} from '$lib/reports';
/**
* Filter resolution above `getReports` where every recorded defect in this area has been: a filter
* silently omitted, or a default set that drifted from what the queue badge counts.
*/
const getReports = vi.fn(async (_params: unknown) => ({
items: [],
totalItems: 0,
page: 1,
limit: 20,
}));
vi.mock('$lib/server/reports.service', () => ({
getReports,
setReportStatus: vi.fn(),
updateReportNotes: vi.fn(),
}));
vi.mock('$lib/server/moderation-board.service', () => ({ getResolvedPostReportIds: vi.fn() }));
vi.mock('$lib/server/user-actions.service', () => ({ removePlacement: vi.fn() }));
vi.mock('$lib/server/access', () => ({ canAccess: vi.fn(() => true) }));
const { load } = await import('../+page.server');
/** `load` for one slug and query string. `redirect()`/`error()` throw, so callers that expect one catch. */
const run = (slug: string, query = '?status=Pending') =>
load({ params: { slug }, url: new URL(`https://mod.example/reports/${slug}${query}`) } as never);
const caught = async (slug: string, query?: string) => {
try {
await run(slug, query);
} catch (thrown) {
return thrown;
}
throw new Error('expected load to throw');
};
/** The params `load` passed down to the service. */
const asked = () => getReports.mock.calls[0][0] as Record<string, unknown>;
beforeEach(() => vi.clearAllMocks());
describe('report queue load', () => {
it('404s an unknown report type rather than querying for it', async () => {
const thrown = await caught('not-a-report-type');
expect(isHttpError(thrown)).toBe(true);
expect((thrown as { status: number }).status).toBe(404);
expect(getReports).not.toHaveBeenCalled();
});
it('canonicalizes a bare landing so the active default filters are in the URL', async () => {
const thrown = await caught('image', '');
expect(isRedirect(thrown)).toBe(true);
const location = (thrown as { location: string }).location;
const statuses = new URL(location, 'https://mod.example').searchParams.getAll('status');
expect(statuses).toEqual(DEFAULT_REPORT_STATUSES);
expect(getReports).not.toHaveBeenCalled();
});
it('reads a present-but-empty status as an explicit "all", not as absent', async () => {
await run('image', '?status=');
expect(asked().statuses).toBe('all');
});
it('reads a present-but-empty reason as an explicit "all", and stops hiding automated', async () => {
const data = await run('image', '?status=Pending&reason=');
expect(asked().reasons).toBe('all');
expect(data).toMatchObject({ hidingAutomated: false });
});
it('hides automated reports by default — they outnumber human ones by orders of magnitude', async () => {
const data = await run('image', '?status=Pending');
expect(asked().reasons).toEqual(DEFAULT_REPORT_REASONS);
expect(data).toMatchObject({ hidingAutomated: true });
});
it('does not echo the default reasons into the filter control', async () => {
const data = await run('image', '?status=Pending');
expect(data).toMatchObject({ reasons: [] });
});
it('drops values that are not real statuses or reasons instead of passing them to SQL', async () => {
await run('image', '?status=Pending&status=Nope&reason=NSFW&reason=Nope');
expect(asked().statuses).toEqual([ReportStatus.Pending]);
expect(asked().reasons).toEqual([ReportReason.NSFW]);
});
it('passes page and reportedBy through, and floors a bad page at 1', async () => {
await run('image', '?status=Pending&page=-4&reportedBy=%20alice%20');
expect(asked()).toMatchObject({ page: 1, reportedBy: 'alice', type: 'image' });
});
});
describe('the default reason set', () => {
// Literals, NOT `reportReasons.filter(r => r !== Automated)` — that reproduces the definition in
// `lib/reports.ts` character for character and so cannot fail. Narrowing this list once left
// pending NSFW, CSAM and StickerPlacement reports behind a zero badge.
it('carries every reason a human files, and excludes only Automated', () => {
expect([...DEFAULT_REPORT_REASONS].sort()).toEqual([
'AdminAttention',
'CSAM',
'Claim',
'NSFW',
'Ownership',
'Spam',
'StickerPlacement',
'TOSViolation',
]);
});
// Derived on purpose: here the relationship to the enum IS the property.
it('leaves no newly added reason silently unqueued', () => {
expect([...DEFAULT_REPORT_REASONS].sort()).toEqual(
reportReasons.filter((r) => r !== ReportReason.Automated).sort()
);
});
});
+4
View File
@@ -0,0 +1,4 @@
// Test stub for `$env/dynamic/private` — backs the SvelteKit virtual module with process.env, the same
// shape apps/auth uses. Modules under test set values via process.env (`$lib/server/db` requires
// DATABASE_URL at import time, so anything reaching it needs one set before the import).
export const env = process.env as Record<string, string | undefined>;
@@ -0,0 +1,70 @@
import {
DummyDriver,
Kysely,
PostgresAdapter,
PostgresIntrospector,
PostgresQueryCompiler,
type CompiledQuery,
CompiledQuery as CompiledQueryClass,
} from 'kysely';
import { createKyselyClients } from '@civitai/db/kysely';
import type { DB } from '@civitai/db-schema/kysely';
/**
* DB-backed tier, modelled on `packages/civitai-db-queries/src/test/harness.ts`. Catches what neither
* TypeScript nor the mocked unit tier can: a wrong table or column inside the raw `sql` that
* `reports.service.ts` assembles from `REPORT_ENTITIES`.
*
* 🔴 NOTHING IS EXECUTED. The query is compiled by a DummyDriver client and only the SQL text is
* sent, as `EXPLAIN` WITHOUT `ANALYZE` parsed and planned, never run, safe for writes too. A suite
* here may PLAN against `DATABASE_URL` and must never write fixtures to it.
*/
export function testDbUrl(): string | undefined {
return process.env.TEST_DATABASE_URL ?? process.env.DATABASE_URL;
}
export function explainHarness() {
const queries: CompiledQuery[] = [];
// No plugins, matching `$lib/server/db.ts` — any here would compile SQL this app never issues.
const db = new Kysely<DB>({
dialect: {
createAdapter: () => new PostgresAdapter(),
createDriver: () => new DummyDriver(),
createIntrospector: (kysely) => new PostgresIntrospector(kysely),
createQueryCompiler: () => new PostgresQueryCompiler(),
},
log: (event) => {
if (event.level === 'query') queries.push(event.query);
},
});
const url = testDbUrl();
const realDb = url
? createKyselyClients<DB>({ connectionString: url, singleClient: true, sslNoVerify: true }).db
: null;
async function explain(cq: CompiledQuery): Promise<string> {
if (!realDb)
throw new Error('explainHarness: no DB URL (set TEST_DATABASE_URL or DATABASE_URL)');
return realDb.connection().execute(async (conn) => {
const result = await conn.executeQuery<Record<string, string>>(
CompiledQueryClass.raw(`explain ${cq.sql}`, [...cq.parameters])
);
return result.rows.map((row) => row['QUERY PLAN']).join('\n');
});
}
return {
db,
queries,
hasDb: !!realDb,
reset: () => queries.splice(0, queries.length),
/** ALL of them, not the last: one service call issues several (`getReports` counts, then pages). */
explainAll: async () => {
if (!queries.length) throw new Error('explainHarness: nothing was compiled to plan');
return Promise.all(queries.map(explain));
},
destroy: () => realDb?.destroy() ?? Promise.resolve(),
};
}
+57
View File
@@ -0,0 +1,57 @@
import { existsSync, readFileSync } from 'node:fs';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
import { defineConfig } from 'vitest/config';
// Node-env unit tests over plain modules — the same shape as apps/auth and apps/creator-studio, and
// NOT the SvelteKit pipeline. `name` is required and must keep the `app:` prefix; see the `apps/*`
// note in the root vitest.config.mts for why dropping it moves this suite into the packages job.
const dir = path.dirname(fileURLToPath(import.meta.url));
const from = (p: string) => path.resolve(dir, p);
/**
* Feeds the EXPLAIN tier a connection string, as `packages/civitai-db-queries/vitest.config.ts` does.
* Absent (CI) and those suites skip.
*
* 🔴 DATABASE_REPLICA_URL is withheld ON PURPOSE. `$lib/server/db` demands both variables at module
* scope, so a suite that forgets to mock it throws on import instead of connecting to whatever a
* developer's `.env` points at. Adding it here to "fix" such a failure removes that protection.
*/
function dbEnvFromRootDotenv(): Record<string, string> {
const out: Record<string, string> = {};
const envPath = from('../../.env');
if (!existsSync(envPath)) return out;
for (const line of readFileSync(envPath, 'utf-8').split('\n')) {
const trimmed = line.trim();
if (!trimmed || trimmed.startsWith('#')) continue;
const eq = trimmed.indexOf('=');
if (eq === -1) continue;
const key = trimmed.slice(0, eq).trim();
if (key !== 'DATABASE_URL' && key !== 'TEST_DATABASE_URL') continue;
let value = trimmed.slice(eq + 1).trim();
if (
(value.startsWith('"') && value.endsWith('"')) ||
(value.startsWith("'") && value.endsWith("'"))
) {
value = value.slice(1, -1);
}
out[key] = value;
}
return out;
}
export default defineConfig({
// This config does not load the SvelteKit plugin, so `$lib` and `$env` need aliasing by hand.
resolve: {
alias: {
$lib: from('./src/lib'),
'$env/dynamic/private': from('./src/test/env.mock.ts'),
},
},
test: {
name: 'app:moderator',
environment: 'node',
include: ['src/**/*.{test,spec}.ts'],
env: dbEnvFromRootDotenv(),
},
});
+23
View File
@@ -201,6 +201,29 @@ That asymmetry is the reason to run `build` **once** before handing work over, e
part of the edit→verify loop. Once — not as a diagnostic loop. It took two of these to reach production
unnoticed because the loop that would have caught them is the one we tell you not to run.
### Tests
Each app owns a `vitest.config.ts` declaring `name: 'app:<slug>'`, and the root config globs those
**config files** — so an app without one is silently not selected. Run one app with
`pnpm --filter @civitai/<app> test`, or every app with `pnpm run test:apps:run` (CI's `App unit tests`
job). The `app:` prefix is load-bearing: every app is also published as `@civitai/*`, so dropping the
`name` moves the suite into the packages job instead.
These are **node-env tests over plain modules** — no SvelteKit pipeline, so `$lib` and the `$env`
virtual modules are aliased in each app's config, and a module reaching an unaliased `$app/*` cannot be
imported at all. Route logic is reachable: import `load`/`actions` from a `+page.server.ts` and call
them with the slice of the event they read. Component behaviour is **not** — no SvelteKit app has a
browser-test project, so anything that depends on `use:enhance`, bindings or lifecycle is verified by
review and by opening the page, not by a test.
🔴 **A suite must not open a connection to whatever `DATABASE_URL` points at.** Mock the app's db
module. Where a suite genuinely needs the real schema, plan the statement rather than run it — compile
through Kysely's `DummyDriver` and send `EXPLAIN` *without* `ANALYZE`, which validates columns, joins
and types without executing, safely for writes as well as reads. Gate it on `describe.skipIf(!hasDb)` so
a checkout with no database still runs the rest. Worked example:
[`apps/moderator/src/test/explain-harness.ts`](../apps/moderator/src/test/explain-harness.ts); the
original is `packages/civitai-db-queries`. Never write fixtures to a URL you did not create.
## Reviews: run these before calling a segment done
Three agents, on the diff for the segment:
+3
View File
@@ -1236,6 +1236,9 @@ importers:
vite:
specifier: 6.4.3
version: 6.4.3(@types/node@20.19.9)(jiti@2.7.0)(lightningcss@1.32.0)(sass@1.90.0)(sugarss@5.0.1(postcss@8.5.23))(terser@5.48.0)(tsx@4.20.3)(yaml@2.8.1)
vitest:
specifier: ^4.0.18
version: 4.0.18(@opentelemetry/api@1.9.0)(@types/node@20.19.9)(@vitest/browser-playwright@4.0.18)(happy-dom@20.9.0(bufferutil@4.0.9)(utf-8-validate@5.0.10))(jiti@2.7.0)(jsdom@27.4.0(@noble/hashes@1.8.0)(bufferutil@4.0.9)(utf-8-validate@5.0.10))(lightningcss@1.32.0)(msw@2.12.10(@types/node@20.19.9)(typescript@5.9.2))(sass@1.90.0)(sugarss@5.0.1(postcss@8.5.23))(terser@5.48.0)(tsx@4.20.3)(yaml@2.8.1)
apps/notifications:
dependencies:
+3 -2
View File
@@ -270,8 +270,9 @@ export default defineConfig({
'packages/*/vitest.config.mts',
// The `apps/*` suites, on exactly the same footing and for exactly the same reason:
// nothing in CI invoked them either. Same CONFIG-FILE glob, same rationale — a bare
// `apps/*` glob would adopt `moderator`, which has no vitest config, and hand it a
// default `include` it was never written against.
// `apps/*` glob would adopt any app that has no vitest config and hand it a default
// `include` it was never written against. Every app carries one today, but the glob
// stays keyed on the config file so the next app added is opted in deliberately.
//
// Unlike the packages, these set `test.name` themselves (`app:auth`,
// `app:notifications`, ...) rather than inheriting their `package.json` name. That is