fix(mods): restore Delete affordance with confirm-reason dialog (#1397) (#1400)

The trash-can button on `?p=admin&c=mods` carried an unguarded
`onclick="RemoveMod(this.dataset.modName, this.dataset.modId);"`
that referenced a v1.x sourcebans.js helper deleted at #1123 D1.
Every click threw `ReferenceError: RemoveMod is not defined` and
the delete never fired — from the operator's POV "Delete just
doesn't work."

This is the loud sister of #1352's admins-delete bug. Same
anti-pattern (inline `onclick` calling a long-removed helper), same
structural fix (mirror the canonical `data-action` + `<dialog>` +
inline page-tail dispatcher shape from `page_admin_admins_list.tpl`,
route through `Actions.ModsRemove`, capture an optional reason for
the audit log).

Server: `api_mods_remove` now accepts an optional `ureason`, trims
it, and appends `Reason: <reason>` to the audit-log body when
non-empty — symmetric with `api_admins_remove` / `api_bans_unban`
/ `api_comms_unblock`. Empty / omitted reason flows through
unchanged so the no-JS / no-dispatcher fallback still works.

Chrome: the page-tail script flips the busy contract on the
submit button (`window.SBPP.setBusy` with local fallback for
themes that strip theme.js), reads / decrements the new
`[data-testid="mod-count"]` badge, removes the row in place, and
fires `window.SBPP.showToast` for confirmation. Cancel closes
without firing the API; missing dispatcher falls back to the mods
list landing (no legacy GET handler for `o=remove`).

Test plan
---------
- ./sbpp.sh phpstan                     ✓
- ./sbpp.sh test                        ✓ (651 tests, includes
                                            new ModsDeleteDialogTest
                                            + extended ModsTest)
- ./sbpp.sh ts-check                    ✓
- ./sbpp.sh composer api-contract       ✓ (regenerated +
                                            tightened typedef)
- CI=1 ./sbpp.sh e2e                    ✓ (266 passed, includes
                                            new mod-delete-confirm
                                            spec — 2 tests covering
                                            confirm-with-reason +
                                            optional-empty-reason)

Mirrors #1352's admins-delete framing; the only delta is the optional-reason
contract is the same shape (mods being a lifecycle action, not a
moderation flip — same justification as admins-delete).

Author: rumblefrog

AGENTS.md

@@ -2142,25 +2142,39 @@ contacting every contributor individually.
 - `onclick="if (typeof <Helper> === 'function') <Helper>(...)"`
   legacy-helper presence guards in templates (the v1.x sourcebans.js
   defensiveness pattern that survived the #1123 D1 deletion of the
-  bulk JS file) → wire to the JSON API via `data-action` + a page-tail
-  vanilla-JS dispatcher per the canonical confirm-modal shape under
-  "Add a confirm + reason modal" in "Where to find what". Pre-#1352 the
+  bulk JS file) AND the unguarded sister shape
+  `onclick="<Helper>(...)"` that drops the `typeof` test entirely →
+  wire to the JSON API via `data-action` + a page-tail vanilla-JS
+  dispatcher per the canonical confirm-modal shape under "Add a
+  confirm + reason modal" in "Where to find what". Pre-#1352 the
   trash-can button on `?p=admin&c=admins` carried
   `onclick="if (typeof RemoveAdmin === 'function') RemoveAdmin(...)"`;
   the `typeof X === 'function'` test silently resolved to `false`
   (sourcebans.js was deleted with v2.0.0 — there's no `RemoveAdmin`
   anywhere) so every click was a no-op with no console error / no
-  toast / no API call. The class of bug is invisible by design (the
-  guard exists precisely to swallow the missing-helper case), so
-  there's no runtime gate; every call site needs the structural fix.
-  When migrating: drop the inline `onclick`, mark the trigger with
+  toast / no API call. **Pre-#1397** the trash-can button on
+  `?p=admin&c=mods` carried the LOUD sister-shape
+  `onclick="RemoveMod(this.dataset.modName, this.dataset.modId);"` —
+  no `typeof` guard at all — so every click threw
+  `ReferenceError: RemoveMod is not defined` (visible in the browser
+  console, but no toast / no API call / no row removal; from the
+  operator's POV "Delete just doesn't work"). The guarded shape's
+  bug-class is invisible by design (the guard exists precisely to
+  swallow the missing-helper case); the unguarded shape is loud but
+  equally non-functional — both need the structural fix. There's no
+  runtime gate beyond the in-process render tests
+  (`AdminsDeleteDialogTest`, `ModsDeleteDialogTest`); every call
+  site needs the structural fix. When migrating: drop the inline
+  `onclick`, mark the trigger with
   `data-action="<surface>-<verb>"` + `data-<id>` + `data-name` +
   `data-fallback-href`, ship a `<dialog>` for the confirm + optional
   reason field, and a page-tail script that dispatches to
   `sb.api.call(Actions.PascalName, …)`. The `Actions.PascalName`
   shape (NOT a string literal) catches typos at api-contract
-  regen time. Search anchor for the cleanup sweep:
-  `rg "typeof \w+ === ['\"]function['\"]" web/themes/`.
+  regen time. Search anchors for the cleanup sweep:
+  `rg "typeof \w+ === ['\"]function['\"]" web/themes/` for the
+  guarded shape, and `rg "onclick=\"[A-Z]\w+\(" web/themes/` for
+  the unguarded sister shape.
 - `web/scripts/contextMenoo.js` / `sb.contextMenu` / global
   `AddContextMenu` → removed at #1306. The vanilla shims were
   back-compat scaffolding for the MooTools-era right-click menu the
@@ -2762,7 +2776,7 @@ contacting every contributor individually.
 | Edit a template                        | `web/themes/default/*.tpl`                               |
 | Reuse the moderation-queue card layout (admin submissions / protests, mobile-stacked summary rows) | `web/themes/default/css/theme.css` (`.queue-row`, `.queue-row__body`, `.queue-row__date` — #1207 PUB-2). Apply by adding `class="queue-row …"` to the outer `<details>` and dropping the inline `flex` / `flex-shrink:0` styles from the summary children. |
 | Add visible row actions to a table-rendered admin list (Edit / Unmute / Remove buttons + responsive mobile-card mirror) | `web/themes/default/page_comms.tpl` (#1207 ADM-5) is the canonical reference: `<button class="btn btn--secondary btn--sm">` / `<a class="btn btn--ghost btn--sm">` inside a `.row-actions` cell, plus `.ban-card__actions` row of identical-data-action buttons in the mobile card. Wire destructive / state-changing buttons via `data-action="…"` + `data-bid` + `data-fallback-href`; the inline page-tail JS calls `sb.api.call(Actions.PascalName)` and falls back to the GET URL if the JSON dispatcher is absent. The public banlist (`web/themes/default/page_bans.tpl`) follows the same shape — same chrome (Lucide icon + visible text label inside `.btn--ghost` / `.btn--secondary btn--sm`), same `.ban-card__actions` mobile row, same `data-action` / `data-fallback-href` wiring (`bans-unban` / `bans-delete`). The Remove affordance points at the legacy GET handler (`?p=banlist&a=delete&id=…&key=…` at the top of `page.banlist.php`) because no JSON `bans.delete` action exists yet — the inline JS `confirm()`-prompts then navigates, mirroring commslist's flow without adding a new handler / snapshot / permission-matrix entry. |
-| Add a confirm + reason modal for an irreversible row-level action (unban, lift comm block, delete admin, …) | `web/themes/default/page_bans.tpl` (`#bans-unban-dialog`, `Actions.BansUnban`) and `web/themes/default/page_comms.tpl` (`#comms-unblock-dialog`, `Actions.CommsUnblock`) are the canonical reference (#1301), with `web/themes/default/page_admin_admins_list.tpl` (`#admins-delete-dialog`, `Actions.AdminsRemove`, #1352) as the third reference for the optional-reason variant. Shape: a `<dialog hidden>` with a `<form method="dialog">` carrying a `<textarea aria-required="true">` (or `aria-required="false"` for the optional-reason variant — see admins-delete) (NOT the native `required` — that lets the browser block the form submit before our handler runs, swallowing the inline-error UX), a Cancel button, and a Confirm submit button. The page-tail JS opens the dialog via `showModal()` on `[data-action]` clicks, validates the trimmed reason on submit (load-bearing gate is server-side), forwards `ureason` to the JSON action, and on success flips the row in place via the same `flipRowToUnbanned`/`flipRowToUnmuted` helper the legacy single-click flow used (or removes the row outright + decrements the count badge for the admins-delete variant where there's no "now-unbanned" state to render). The legacy GET fallback (`?p=banlist&a=unban&id=…&key=…&ureason=…` / `?p=commslist&a=ungag…&ureason=…`) is the no-JS / hand-edited-URL path; both halves now reject empty `ureason` server-side so the audit log carries the *why*. The admins-delete variant has no legacy GET handler — `RemoveAdmin()` always went through the JSON dispatcher pre-#1123 D1 — so its `data-fallback-href` lands the operator back at the admins list as a graceful no-op when the JSON dispatcher is missing entirely (third-party theme stripping `api.js`); the audit-log "Reason: …" suffix is only emitted when `ureason` is non-empty (vs always-emitted on the bans / comms variants where reason is required). **Do not** put `onclick="event.stopPropagation()"` on the trigger button — `document.addEventListener('click')` is how the dialog opener picks the click up, and stopPropagation would silently swallow it (the action button isn't inside any `[data-drawer-href]` ancestor anyway, so the defensiveness was a copy-paste from the row-name anchor that doesn't apply here). The submit button MUST flip through `setBusy(submitBtn, true)` BEFORE `sb.api.call(...)` leaves the page and clear via `setBusy(submitBtn, false)` on every non-navigating response branch — see "Loading state on action buttons" in Conventions for the contract, the inline-script local wrapper shape, and the regression guard. |
+| Add a confirm + reason modal for an irreversible row-level action (unban, lift comm block, delete admin, delete mod, …) | `web/themes/default/page_bans.tpl` (`#bans-unban-dialog`, `Actions.BansUnban`) and `web/themes/default/page_comms.tpl` (`#comms-unblock-dialog`, `Actions.CommsUnblock`) are the canonical reference (#1301), with `web/themes/default/page_admin_admins_list.tpl` (`#admins-delete-dialog`, `Actions.AdminsRemove`, #1352) and `web/themes/default/page_admin_mods_list.tpl` (`#mod-delete-dialog`, `Actions.ModsRemove`, #1397) as the third and fourth references for the optional-reason variant. Shape: a `<dialog hidden>` with a `<form method="dialog">` carrying a `<textarea aria-required="true">` (or `aria-required="false"` for the optional-reason variant — see admins-delete / mod-delete) (NOT the native `required` — that lets the browser block the form submit before our handler runs, swallowing the inline-error UX), a Cancel button, and a Confirm submit button. The page-tail JS opens the dialog via `showModal()` on `[data-action]` clicks, validates the trimmed reason on submit (load-bearing gate is server-side), forwards `ureason` to the JSON action, and on success flips the row in place via the same `flipRowToUnbanned`/`flipRowToUnmuted` helper the legacy single-click flow used (or removes the row outright + decrements the count badge for the admins-delete / mod-delete variants where there's no "now-unbanned" state to render). The legacy GET fallback (`?p=banlist&a=unban&id=…&key=…&ureason=…` / `?p=commslist&a=ungag…&ureason=…`) is the no-JS / hand-edited-URL path; both halves now reject empty `ureason` server-side so the audit log carries the *why*. The admins-delete and mod-delete variants have no legacy GET handler — `RemoveAdmin()` / `RemoveMod()` always went through the JSON dispatcher pre-#1123 D1 — so their `data-fallback-href` lands the operator back at the list page as a graceful no-op when the JSON dispatcher is missing entirely (third-party theme stripping `api.js`); the audit-log "Reason: …" suffix is only emitted when `ureason` is non-empty (vs always-emitted on the bans / comms variants where reason is required). **Do not** put `onclick="event.stopPropagation()"` on the trigger button — `document.addEventListener('click')` is how the dialog opener picks the click up, and stopPropagation would silently swallow it (the action button isn't inside any `[data-drawer-href]` ancestor anyway, so the defensiveness was a copy-paste from the row-name anchor that doesn't apply here). The submit button MUST flip through `setBusy(submitBtn, true)` BEFORE `sb.api.call(...)` leaves the page and clear via `setBusy(submitBtn, false)` on every non-navigating response branch — see "Loading state on action buttons" in Conventions for the contract, the inline-script local wrapper shape, and the regression guard. |
 | Add a loading indicator to an action button that fires `sb.api.call(...)` without a page refresh | `window.SBPP.setBusy(btn, busy)` (`web/themes/default/js/theme.js`) writes the `data-loading="true"` + `aria-busy="true"` + `disabled` triple atomically; the CSS spinner lives in `web/themes/default/css/theme.css` under `.btn[data-loading="true"]` + the `sbpp-btn-spin` keyframe. Inline page-tail scripts inside `.tpl` files define a local `setBusy(btn, busy)` wrapper that delegates to `window.SBPP.setBusy` when present and falls back to `btn.disabled = busy` so third-party themes that strip `theme.js` still gate against double-clicks. Canonical reference shapes: the three confirm-dialog flows (`page_comms.tpl` / `page_bans.tpl` / `page_admin_admins_list.tpl`), the form-submit flows (`page_admin_groups_list.tpl` / `page_admin_groups_add.tpl` / `page_admin_bans_add.tpl` / `page_admin_bans_email.tpl` / `page_youraccount.tpl` / `page_lostpassword.tpl` / `page_login.tpl`), the row-action flows (`page_admin_servers_list.tpl` / `page_admin_bans_protests.tpl` / `page_admin_bans_protests_archiv.tpl` / `page_admin_bans_submissions.tpl` / `page_admin_bans_submissions_archiv.tpl`), and the drawer Notes paths (`theme.js`'s `submitNoteForm` / `deleteNote`). Comment edit on the banlist (`web/scripts/banlist.js`) carries the same pattern for the `sb.api.call(BansEditComment)` round-trip. Regression guards: `web/tests/e2e/specs/flows/action-loading-indicator.spec.ts` (stalls `Actions.CommsUnblock` via `page.route`, asserts the busy-attribute triple on the submit button while in flight, releases the route, and confirms the row flips in-place; the second test counts requests to prove the disabled gate blocks a double-click) **plus** `web/tests/e2e/specs/flows/loading-animations.spec.ts` (#1362 — samples `getComputedStyle(::after).transform` at multiple frame boundaries under both `reducedMotion: 'reduce'` AND `'no-preference'`, asserts the matrix values change across samples; catches the v2.0 RC1 regression where the global `prefers-reduced-motion: reduce` reset froze the spinner under reduced motion). |
 | Add a loading indicator to the player drawer or one of its lazy panes (so the chrome doesn't read as blank while the JSON action is in flight) | `renderDrawerLoading()` (header skeleton for the in-flight `bans.detail`) and `renderPaneSkeleton()` (placeholder for History / Comms / Notes activation) in `web/themes/default/js/theme.js`. Both lean on the `.skel` CSS rule in `theme.css` (linear-gradient + `shimmer` keyframe + dark-mode override + the `@media (prefers-reduced-motion: reduce)` per-rule override that keeps the shimmer sliding even under reduced motion, #1362). The header skeleton carries `[data-testid="drawer-loading"]` + `aria-busy="true"` + per-block `[data-skeleton]` (terminal markers under `#drawer-root[data-loading="true"]`); the lazy-pane skeleton carries `[data-pane-empty]` + `aria-busy="true"` and deliberately omits `[data-skeleton]` because the panel parent's `hidden` attribute doesn't compose into `[data-skeleton]:not([hidden])` and a nested marker would stall every page-load waiter that runs after the drawer opens. Class name is `.skel` (singular) — NOT `.skeleton`; the pre-fix `class="skeleton"` typo had no matching rule and the shimmer rows rendered as transparent zero-background divs (the user-visible "drawer is blank" regression). Regression guards: `web/tests/e2e/specs/flows/drawer-loading-indicator.spec.ts` (stalls `bans.detail` then `bans.player_history` via `page.route`, asserts the skeleton header is visible + the `.skel` block paints a `linear-gradient` background via `getComputedStyle(el).backgroundImage`, releases the routes, and confirms the drawer flips to `renderDrawerBody` / the pane fills with content) **plus** `web/tests/e2e/specs/flows/loading-animations.spec.ts` (#1362 — samples `getComputedStyle(.skel).backgroundPositionX` at multiple frame boundaries under both `reducedMotion: 'reduce'` AND `'no-preference'`, asserts the values change across samples; catches the v2.0 RC1 regression where the global reset froze the shimmer alongside the spinner). |
 | Surface unban-reason / removed-by inline on a public-list row (admin-lifted bans / comms — banlist-ureason or commslist-ureason inline) | `web/themes/default/page_bans.tpl` + `web/themes/default/page_comms.tpl` (#1315). Reason cell on the desktop table emits a `<div class="text-xs text-faint mt-1" data-testid="ban-unban-meta">` (or `comm-unban-meta` for comms) with "Unbanned by `<admin>`: `<reason>`" when `$ban.state == 'unbanned'` (or `$comm.state == 'unmuted'`); mobile cards mirror with the `-mobile` testid suffix. Always gated on `!$hideadminname` so anonymous viewers under a hidden-admins config don't get the admin name leaked. The `ureason` / `removedby` row fields come from the page handler's existing data path (`page.banlist.php` lines 635-643, `page.commslist.php` lines 626-635) — read-only render, no write-side overlap with #1301 / #1323's unban-reason flow. The commslist surface is higher-priority than the banlist (no drawer fallback on `<tr data-testid="comm-row">`); banlist users have the drawer as the canonical detail view. |

web/api/handlers/mods.php

@@ -1,15 +1,7 @@
 <?php
-/*************************************************************************
-This file is part of SourceBans++
-
-SourceBans++ (c) 2014-2024 by SourceBans++ Dev Team
-
-The SourceBans++ Web panel is licensed under a
-Creative Commons Attribution-NonCommercial-ShareAlike 3.0 Unported License.
-
-You should have received a copy of the license along with this
-work.  If not, see <http://creativecommons.org/licenses/by-nc-sa/3.0/>.
-*************************************************************************/
+// SourceBans++ (c) 2014-2026 SourceBans++ Dev Team
+// Licensed under Creative Commons Attribution-NonCommercial-ShareAlike 3.0.
+// See LICENSE.md for the full license text and THIRD-PARTY-NOTICES.txt for attributions.
 
 function api_mods_add(array $params): array
 {
@@ -43,9 +35,39 @@ function api_mods_add(array $params): array
     ];
 }
 
+/**
+ * Delete a mod row + its on-disk icon (#1397).
+ *
+ * Modern JSON twin of the v1.x sourcebans.js `RemoveMod()` helper
+ * (deleted at #1123 D1) — `page_admin_mods_list.tpl` wires the
+ * trash-can button through `Actions.ModsRemove` via the
+ * `#mod-delete-dialog` confirm + reason modal. There is no legacy
+ * GET fallback for `o=remove` here (the v1.x JS helper went
+ * straight to xajax then to this handler), so this is the single
+ * delete path; the modal's no-JS / no-dispatcher fallback just
+ * lands the operator back on the mods list.
+ *
+ * Inputs:
+ *   - `mid`     (int, required)    — the mod id to remove.
+ *   - `ureason` (string, optional) — admin-supplied reason. We trim
+ *     it and append `Reason: …` to the audit-log entry when
+ *     non-empty. Empty / omitted is allowed (the modal carries
+ *     `aria-required="false"`); mod deletion is a lifecycle
+ *     action, not a moderation flip, so we don't gate the call on
+ *     it the way `bans.unban` / `comms.unblock` do.
+ *
+ * @param array{ mid?: int|string, ureason?: string } $params
+ * @return array{
+ *     remove: string,
+ *     message: array{ title: string, body: string, kind: string, redir: string }
+ * }
+ */
 function api_mods_remove(array $params): array
 {
     $mid = (int)($params['mid'] ?? 0);
+    // Trim whitespace so a textarea that contains only spaces produces an
+    // empty reason (audit-log suffix omitted) rather than `Reason:    `.
+    $ureason = trim((string)($params['ureason'] ?? ''));
 
     $GLOBALS['PDO']->query("SELECT icon, name FROM `:prefix_mods` WHERE mid = :mid");
     $GLOBALS['PDO']->bind(':mid', $mid);
@@ -63,7 +85,17 @@ function api_mods_remove(array $params): array
         throw new ApiError('delete_failed', 'There was a problem deleting the MOD from the database. Check the logs for more info');
     }
 
-    Log::add(LogType::Message, 'MOD Deleted', "MOD ({$row['name']}) has been deleted.");
+    // #1397: trail the optional admin-supplied reason in the audit-log
+    // entry so admins reading the log later can see *why* the mod was
+    // removed. Mirrors the canonical "Reason: $ureason" suffix shape
+    // from `api_admins_remove` / `api_bans_unban` / `api_comms_unblock`
+    // — the suffix is omitted when the operator left the field blank.
+    $modName = ($row && isset($row['name'])) ? (string) $row['name'] : ('mid ' . $mid);
+    $logBody = "MOD ({$modName}) has been deleted.";
+    if ($ureason !== '') {
+        $logBody .= " Reason: {$ureason}";
+    }
+    Log::add(LogType::Message, 'MOD Deleted', $logBody);
 
     return [
         'remove'  => "mid_$mid",