From 872bdfa64e92f505735460315296da0b88bcab4e Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Tue, 15 Sep 2026 13:47:05 -0400 Subject: [PATCH] [www] tweaks to mod panel --- apps/api/src/reports-db.ts | 17 +- apps/api/src/test/integration/api.test.ts | 52 +++ apps/www/src/client/App.tsx | 55 ++- apps/www/src/client/Moderation.tsx | 472 +++++++++++++++------- apps/www/src/staff.ts | 14 +- apps/www/src/test/integration/api.test.ts | 67 ++- 6 files changed, 513 insertions(+), 164 deletions(-) diff --git a/apps/api/src/reports-db.ts b/apps/api/src/reports-db.ts index f0c02bb..e666555 100644 --- a/apps/api/src/reports-db.ts +++ b/apps/api/src/reports-db.ts @@ -431,14 +431,25 @@ export async function getTopReported( } /** - * Every ban in force right now, longest-lasting first — the staff panel's standing-bans - * list (`www`: `GET /api/staff/bans`). + * Every ban in force right now, MOST RECENTLY HANDED DOWN FIRST — the staff panel's + * standing-bans list (`www`: `GET /api/staff/bans`). * * "In force" is `getActiveBan`'s test applied to the whole table rather than to one * player: a row whose `ban_expires` has passed has served its time and is not listed, * though it stays as the record that it happened. A player with several in force appears * once per ban — each is a separate report, and which report justified which ban is the * point of the list. + * + * Ordered by WHEN THE BAN LANDED because the list's job is catching mistakes: the ban most + * likely to be wrong is the one just handed down, and a moderator who has slipped goes + * looking for it at the top. Ordering by severity instead (longest-lasting first, as this + * did) buries a fresh one-day ban under every permanent ban ever issued — exactly the + * wrong way round for the one read this list exists to serve. + * + * `COALESCE(banned_at, created_at)`: a ban set before 0020_report_ban_audit.sql added + * `banned_at` has only its report's date to sort by, which is the same fallback the block + * screen uses. Without the coalesce those rows would sort as NULL — all together at one + * end, in no order at all. */ export async function getBansInForce( db: D1Database, @@ -449,7 +460,7 @@ export async function getBansInForce( .prepare( `SELECT * FROM report WHERE banned = 1 AND (ban_expires IS NULL OR ban_expires > ?1) - ORDER BY ban_expires IS NOT NULL, ban_expires DESC, id DESC + ORDER BY COALESCE(banned_at, created_at) DESC, id DESC LIMIT ?2` ) .bind(now.toISOString(), Math.min(Math.max(take, 1), 500)) diff --git a/apps/api/src/test/integration/api.test.ts b/apps/api/src/test/integration/api.test.ts index 7708135..b2d4f7f 100644 --- a/apps/api/src/test/integration/api.test.ts +++ b/apps/api/src/test/integration/api.test.ts @@ -4834,6 +4834,58 @@ describe('player reports', () => { expect(banned.find((r) => r.playerId === 3350)?.bannedNow).toBe(true) }) + // The standing-bans list exists to catch MISTAKES, so it is ordered by when the ban + // landed: the one most likely to be wrong is the one just handed down. Ordering by + // severity instead would bury a fresh one-day ban under every permanent ban ever + // issued. + test('getBansInForce puts the most recently handed-down ban first', async () => { + const first = await createReport(env.DB, { reporterPlayerId: 3371, reportedPlayerId: 3370 }) + const second = await createReport(env.DB, { reporterPlayerId: 3373, reportedPlayerId: 3372 }) + const third = await createReport(env.DB, { reporterPlayerId: 3375, reportedPlayerId: 3374 }) + + // Handed down oldest-first, and deliberately mixed in severity: the permanent one + // is banned FIRST, so an order by "longest lasting" would float it to the top. + await banFromReport(env.DB, first.id) + await env.DB.prepare('UPDATE report SET banned_at = ?2 WHERE id = ?1') + .bind(first.id, '2024-01-01T00:00:00.000Z') + .run() + await banFromReport(env.DB, second.id, { banExpires: '2999-01-01T00:00:00.000Z' }) + await env.DB.prepare('UPDATE report SET banned_at = ?2 WHERE id = ?1') + .bind(second.id, '2024-06-01T00:00:00.000Z') + .run() + await banFromReport(env.DB, third.id, { banExpires: '2999-01-01T00:00:00.000Z' }) + await env.DB.prepare('UPDATE report SET banned_at = ?2 WHERE id = ?1') + .bind(third.id, '2025-01-01T00:00:00.000Z') + .run() + + const ordered = (await getBansInForce(env.DB)) + .map((b) => b.id) + .filter((id) => [first.id, second.id, third.id].includes(id)) + expect(ordered).toEqual([third.id, second.id, first.id]) + }) + + // A ban set before 0020 added `banned_at` has only its report's date to sort by — + // the same fallback the block screen uses. Without the coalesce every such row would + // sort as NULL: all at one end, in no order at all. + test('getBansInForce falls back to created_at for a ban with no banned_at', async () => { + const old = await createReport(env.DB, { reporterPlayerId: 3381, reportedPlayerId: 3380 }) + const recent = await createReport(env.DB, { reporterPlayerId: 3383, reportedPlayerId: 3382 }) + // The pre-migration shape: banned, with no record of when. + await env.DB.prepare( + 'UPDATE report SET banned = 1, banned_at = NULL, created_at = ?2 WHERE id = ?1' + ) + .bind(old.id, '2023-01-01T00:00:00.000Z') + .run() + await banFromReport(env.DB, recent.id) + + const ordered = (await getBansInForce(env.DB)) + .map((b) => b.id) + .filter((id) => [old.id, recent.id].includes(id)) + // The undated one sorts by its report's date, so it lands behind today's ban + // rather than at an arbitrary end of the list. + expect(ordered).toEqual([recent.id, old.id]) + }) + test('getBansInForce lists in-force bans only, and getReportById reads one row', async () => { const permanent = await createReport(env.DB, { reporterPlayerId: 3361, diff --git a/apps/www/src/client/App.tsx b/apps/www/src/client/App.tsx index 323ad9f..b2ec284 100644 --- a/apps/www/src/client/App.tsx +++ b/apps/www/src/client/App.tsx @@ -441,20 +441,40 @@ const coachMessage = ( /** Minimal history-based router: current pathname + a navigate() that pushes state. */ function useRouter() { - const [path, setPath] = useState(() => window.location.pathname) + // Pathname and query string are tracked SEPARATELY rather than as one string, because + // the pages match on the pathname alone and one of them is reached with a query on it: + // `/claim?code=…` is Discord's redirect, so a `path` that carried the search would stop + // matching `/claim` the moment it mattered. `search` is here for the pages whose own + // state belongs in the URL — the moderation panel's filters and paging — so that the + // back button walks back through them instead of leaving the page. + const [location, setLocation] = useState(() => ({ + path: window.location.pathname, + search: window.location.search, + })) useEffect(() => { - const onPop = () => setPath(window.location.pathname) + const onPop = () => + setLocation({ path: window.location.pathname, search: window.location.search }) window.addEventListener('popstate', onPop) return () => window.removeEventListener('popstate', onPop) }, []) const navigate = useCallback((to: string) => { - if (to !== window.location.pathname) { + // `to` may carry a query string, so the "did anything change" test is against the + // whole of what's on screen — otherwise a filter change (same path, new query) would + // never push, and the back button would have nothing to walk back through. + const [path, search = ''] = to.split('?') + // Read before pushing: afterwards `window.location` IS the destination, and every + // "did the page change" test would answer no. + const samePage = path === window.location.pathname + if (to !== window.location.pathname + window.location.search) { window.history.pushState(null, '', to) - window.scrollTo(0, 0) + // Staying on the same page with a different query is a re-filtered list, where + // jumping to the top would throw away the reader's place. A real page change + // still scrolls up. + if (!samePage) window.scrollTo(0, 0) } - setPath(to) + setLocation({ path: path ?? '/', search: search === '' ? '' : `?${search}` }) }, []) - return { path, navigate } + return { ...location, navigate } } type Navigate = (to: string) => void @@ -709,7 +729,7 @@ export function App() { // undefined until the config lands. Signup is treated as closed until told otherwise, // so a slow (or failed) config fetch can't flash a form the server would refuse. const [config, setConfig] = useState(undefined) - const { path, navigate } = useRouter() + const { path, search, navigate } = useRouter() const roomId = roomIdFromPath(path) useEffect(() => { @@ -768,12 +788,17 @@ export function App() { // Its own page rather than a dashboard tab: this path is Discord's registered // redirect URI, so it has to be one stable URL a cold load can land on. - ) : path === '/moderation' ? ( - // A whole surface rather than another dashboard tab, and its own URL so a - // moderator can keep it open. The path is deliberately NOT in wrangler.jsonc's - // `run_worker_first`, so a cold load falls through to the SPA shell; the page - // then gates itself on the token's role, and every endpoint behind it re-checks. - + ) : path === '/moderation' || path.startsWith('/moderation/') ? ( + // A whole surface rather than another dashboard tab, and its own URLS: the + // panel routes UNDER this prefix (`/moderation/players/123`, and its filters in + // the query string), so the back button walks back through a moderator's + // session instead of dropping them on the homepage. Matched by prefix here for + // that reason; the panel picks the view apart itself. + // + // None of these paths is in wrangler.jsonc's `run_worker_first`, so a cold load + // on any of them falls through to the SPA shell; the page then gates itself on + // the token's role, and every endpoint behind it re-checks. + ) : roomId !== null ? ( ) : ( @@ -839,7 +864,9 @@ function NavBar({ Moderation diff --git a/apps/www/src/client/Moderation.tsx b/apps/www/src/client/Moderation.tsx index 48ed127..61dbfd0 100644 --- a/apps/www/src/client/Moderation.tsx +++ b/apps/www/src/client/Moderation.tsx @@ -35,6 +35,43 @@ const TABS: Array<{ id: Tab; label: string }> = [ { id: 'new', label: 'File a report' }, ] +const TAB_IDS = new Set(TABS.map((t) => t.id)) + +/** + * Which view a `/moderation/…` path names. + * + * Every one of these is a REAL URL rather than a piece of component state, so the back + * button walks back out of a drill-down instead of leaving the panel — which is what it + * did when the tab and the open player were held in `useState`: a moderator two clicks + * deep pressed Back and landed on the homepage, losing the whole session. Along the way it + * makes each view linkable, which is how one moderator hands another a player to look at. + * + * `/moderation` alone is the queue, so the nav link needs no tab in it and a bookmark of + * the bare path still opens on something. + */ +type View = + | { kind: 'tab'; tab: Tab } + | { kind: 'player'; playerId: number } + | { kind: 'ban'; reportId: number } + +function viewForPath(path: string): View { + const rest = path.replace(/^\/moderation\/?/, '').replace(/\/$/, '') + if (rest === '') return { kind: 'tab', tab: 'queue' } + + const player = /^players\/(\d+)$/.exec(rest) + if (player) return { kind: 'player', playerId: Number.parseInt(player[1]!, 10) } + + const ban = /^reports\/(\d+)\/ban$/.exec(rest) + if (ban) return { kind: 'ban', reportId: Number.parseInt(ban[1]!, 10) } + + // An unknown segment falls back to the queue rather than rendering nothing: these paths + // are typed and pasted, and a typo should land somewhere usable. + return { kind: 'tab', tab: TAB_IDS.has(rest) ? (rest as Tab) : 'queue' } +} + +/** Where a tab lives. The queue is the bare path, not `/moderation/queue`. */ +const tabPath = (tab: Tab): string => (tab === 'queue' ? '/moderation' : `/moderation/${tab}`) + /** * A stored report, as `www` serves the row — snake_case, because it IS the row. Not every * column is rendered; the ones that are not (the measured heights, the instance type) are @@ -240,22 +277,31 @@ function useNames(ids: number[]) { export function ModerationPage({ account, + path, + search, navigate, }: { account: { accountId: number; username: string } | null | undefined + /** The full pathname — the panel routes under `/moderation`; see {@link viewForPath}. */ + path: string + /** The query string, where the search tab keeps its filters and its page. */ + search: string navigate: (to: string) => void }) { - const [tab, setTab] = useState('queue') - // The player whose history is open, or null for the tab's own view. Local state rather - // than a route: it's a drill-down inside the panel, and a moderator walks in and out of - // several in a sitting without wanting each one in their back button. - const [openPlayer, setOpenPlayer] = useState(null) - // The report a ban is being composed against. Held here so the dialog can be opened - // from any of the tables. - const [banning, setBanning] = useState(null) - // Bumped after any write, to re-read whichever tables are on screen. + const view = viewForPath(path) + // Bumped after a write that leaves you where you are — lifting a ban from a table. A + // write that NAVIGATES (banning, filing a report) needs no bump: the view it lands on + // mounts fresh and reads for itself. const [revision, setRevision] = useState(0) const changed = useCallback(() => setRevision((n) => n + 1), []) + const openPlayer = useCallback( + (playerId: number) => navigate(`/moderation/players/${playerId}`), + [navigate] + ) + const openBan = useCallback( + (report: ReportRow) => navigate(`/moderation/reports/${report.id}/ban`), + [navigate] + ) if (account === undefined) { return ( @@ -306,64 +352,83 @@ export function ModerationPage({ - {openPlayer !== null ? ( + {view.kind === 'player' ? ( setOpenPlayer(null)} + navigate={navigate} + onBan={openBan} /> + ) : view.kind === 'ban' ? ( + ) : (
- {tab === 'queue' ? ( - - ) : tab === 'search' ? ( - + ) : view.tab === 'search' ? ( + - ) : tab === 'bans' ? ( - + ) : view.tab === 'bans' ? ( + ) : ( - { - changed() - setBanning(report) - }} - /> + // Filing a report navigates straight to the ban dialog for it — filing one + // is almost always the first half of banning somebody, and the new report's + // id is then in the URL, so the ban is a real page rather than a modal that + // a reload would lose. + )}
)} - - {banning !== null && ( - setBanning(null)} - onDone={() => { - setBanning(null) - changed() - }} - /> - )} ) } +/** + * Back out of a drill-down — the in-page counterpart of the browser's own back button, + * and deliberately the SAME action, so the two can't disagree about where "back" is. + * + * Falls forward to the panel's front page when there is no history to pop: a moderator who + * arrived on a pasted link has nothing behind them, and a button that did nothing would + * read as broken. + */ +function BackLink({ navigate, label }: { navigate: (to: string) => void; label: string }) { + return ( + + ) +} + /** * The report queue: who has collected the most reports lately. * @@ -373,14 +438,27 @@ export function ModerationPage({ * is a problem now rather than whoever has ever accumulated the most. */ function ReportQueue({ + search, revision, + navigate, onOpenPlayer, }: { + search: string revision: number + navigate: (to: string) => void onOpenPlayer: (id: number) => void }) { - const [sinceDays, setSinceDays] = useState('30') - const [minReports, setMinReports] = useState('3') + // In the URL for the same reason the search's filters are: a moderator who widens the + // window to 90 days, opens a player and presses Back must come back to the 90 days + // they were reading, not to a list quietly reset to the default. + const params = new URLSearchParams(search) + const sinceDays = params.get('sinceDays') ?? '30' + const minReports = params.get('minReports') ?? '3' + const queueUrl = (next: { sinceDays?: string; minReports?: string }) => { + const updated = new URLSearchParams({ sinceDays, minReports, ...next }) + return `/moderation?${updated.toString()}` + } + const { data, error } = useStaffData( `/api/staff/reports/top-reported?sinceDays=${encodeURIComponent(sinceDays)}&minReports=${encodeURIComponent(minReports)}`, [revision] @@ -398,7 +476,10 @@ function ReportQueue({
@@ -459,81 +548,145 @@ function ReportQueue({ ) } -/** The search form's state — mirrors the query params `searchReportsHandler` reads. */ +/** + * The search form's state. These are the SAME names the staff endpoint reads as query + * params (`searchReportsHandler`) and the same ones the panel's own URL carries, so the + * form, the address bar and the request can't drift apart — one spelling throughout. + * + * `from`/`to` are the `YYYY-MM-DD` a date input gives, converted to instants only when the + * request goes out; keeping the URL in the input's own format is what lets a bookmarked + * search repopulate the boxes exactly as they were typed. + */ interface SearchFilters { - reported: string - reporter: string - category: string + reportedPlayerId: string + reporterPlayerId: string + reportCategory: string banned: string from: string to: string } const EMPTY_FILTERS: SearchFilters = { - reported: '', - reporter: '', - category: '', + reportedPlayerId: '', + reporterPlayerId: '', + reportCategory: '', banned: '', from: '', to: '', } +/** Read the filters back out of the panel's URL. Absent keys read as empty. */ +function filtersFromSearch(search: string): SearchFilters { + const params = new URLSearchParams(search) + const read = (key: keyof SearchFilters) => params.get(key) ?? '' + return { + reportedPlayerId: read('reportedPlayerId'), + reporterPlayerId: read('reporterPlayerId'), + reportCategory: read('reportCategory'), + banned: read('banned'), + from: read('from'), + to: read('to'), + } +} + +/** + * The panel URL for a set of filters and a page. Empty values are left OUT rather than + * written as blanks, so an unfiltered search is the bare `/moderation/search` and the + * address bar says only what was actually asked for. + */ +function searchUrl(filters: SearchFilters, skip: number): string { + const params = new URLSearchParams() + for (const [key, value] of Object.entries(filters)) { + if (value !== '') params.set(key, value) + } + if (skip > 0) params.set('skip', String(skip)) + const query = params.toString() + return query === '' ? '/moderation/search' : `/moderation/search?${query}` +} + /** * Search the report log. * + * The filters and the page live in the URL, not in component state: a moderator searches, + * opens a player, and presses Back — and has to land on the results they were reading, + * not on an empty form. It also makes a search linkable, which is how one moderator hands + * another a slice of the log to look at. + * * The two player boxes take either an id or an `@username`, resolved through `accounts`' * search before the query goes out — a moderator following up a Discord report has a name, * not a number, and making them look it up elsewhere first is the kind of friction that - * ends in nobody using the panel. + * ends in nobody using the panel. The URL holds the resolved ID, since that is what was + * searched and what makes the link reproducible. */ function ReportSearch({ + search, revision, + navigate, onOpenPlayer, onBan, onChanged, }: { + search: string revision: number + navigate: (to: string) => void onOpenPlayer: (id: number) => void onBan: (report: ReportRow) => void onChanged: () => void }) { - const [form, setForm] = useState(EMPTY_FILTERS) - // The query actually in flight, separate from what's typed: a search runs when it is - // submitted, not on every keystroke, and the ids in it are resolved by then. - const [query, setQuery] = useState('?take=' + PAGE_SIZE) - const [skip, setSkip] = useState(0) + const applied = filtersFromSearch(search) + const skip = Number.parseInt(new URLSearchParams(search).get('skip') ?? '0', 10) || 0 + // What is TYPED, which runs ahead of what is applied — a search happens on submit, not + // on every keystroke. Keyed on the URL so that arriving at a different search (a link, + // or the back button) resets the boxes to it instead of stranding the last thing typed. + const [form, setForm] = useState(applied) + const [formKey, setFormKey] = useState(search) + if (formKey !== search) { + setFormKey(search) + setForm(applied) + } const { pending, error: formError, run } = useAction() - const path = query === null ? null : `/api/staff/reports${query}&skip=${skip}` - const { data, error } = useStaffData<{ reports: ReportRow[]; total: number }>(path, [revision]) + const params = new URLSearchParams({ take: String(PAGE_SIZE), skip: String(skip) }) + if (applied.reportedPlayerId !== '') params.set('reportedPlayerId', applied.reportedPlayerId) + if (applied.reporterPlayerId !== '') params.set('reporterPlayerId', applied.reporterPlayerId) + if (applied.reportCategory !== '') params.set('reportCategory', applied.reportCategory) + if (applied.banned !== '') params.set('banned', applied.banned) + // A date input gives a bare `YYYY-MM-DD`, so the window is sent as local midnights, + // which is what the moderator meant by it. `to` is EXCLUSIVE on the server, so the day + // typed there is included only by pushing the bound to the next midnight. + if (applied.from !== '') params.set('from', new Date(applied.from).toISOString()) + if (applied.to !== '') { + const to = new Date(applied.to) + to.setDate(to.getDate() + 1) + params.set('to', to.toISOString()) + } + + const { data, error } = useStaffData<{ reports: ReportRow[]; total: number }>( + `/api/staff/reports?${params.toString()}`, + [revision] + ) const reports = data?.reports ?? [] const names = useNames(reports.flatMap((r) => [r.reported_player_id, r.reporter_player_id])) const submit = (e: React.FormEvent) => { e.preventDefault() void run(async () => { - const params = new URLSearchParams({ take: String(PAGE_SIZE) }) - // Either box may hold a name; resolve before searching so the query is by id, as - // the report rows are. - if (form.reported.trim() !== '') { - params.set('reportedPlayerId', String(await playerIdFrom(form.reported))) + // Either box may hold a name; resolve before navigating so the URL carries the id, + // as the report rows do. + const resolved: SearchFilters = { + ...form, + reportedPlayerId: + form.reportedPlayerId.trim() === '' + ? '' + : String(await playerIdFrom(form.reportedPlayerId)), + reporterPlayerId: + form.reporterPlayerId.trim() === '' + ? '' + : String(await playerIdFrom(form.reporterPlayerId)), } - if (form.reporter.trim() !== '') { - params.set('reporterPlayerId', String(await playerIdFrom(form.reporter))) - } - if (form.category !== '') params.set('reportCategory', form.category) - if (form.banned !== '') params.set('banned', form.banned) - // A date input gives a bare `YYYY-MM-DD`; `to` is exclusive on the server, so a - // day entered there includes that whole day only if it is pushed to the next - // midnight. Both are sent as local midnights, which is what the moderator meant. - if (form.from !== '') params.set('from', new Date(form.from).toISOString()) - if (form.to !== '') { - const to = new Date(form.to) - to.setDate(to.getDate() + 1) - params.set('to', to.toISOString()) - } - setSkip(0) - setQuery(`?${params.toString()}`) + // Back to page one: the new filters have their own result set, and staying on + // page three of it would usually show nothing. + navigate(searchUrl(resolved, 0)) return '' }) } @@ -550,24 +703,24 @@ function ReportSearch({