fix(1441): kick the player from their server when the ban is added

AGENTS.md

@@ -2512,6 +2512,60 @@ contacting every contributor individually.
  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".
+- `typeof window.<LegacyHelper> === 'function'` guards on a v1.x
+ helper inside a v2.0 page-tail handler (e.g.
+ `typeof window.ShowKickBox === 'function'` for the kick-on-ban
+ surface; `typeof window.TabToReload === 'function'` for the
+ post-action page-reload trigger) → both were defined in
+ `web/scripts/sourcebans.js`, deleted at #1123 D1 (v2.0.0). The
+ `typeof` test silently resolves to `false`, the body of the
+ `if` never runs, AND no console error fires — so the surrounding
+ success branch falls through to a generic toast while the
+ actual side-effect (kick a player, reload the tab) silently
+ doesn't happen. This is the worst class of UI regression: the
+ chrome reads as success while the load-bearing action never
+ fired. The kick-on-ban path was the canonical case (#1441 —
+ operators reported "banning doesn't kick" on the v2.0 panel; the
+ DB row landed and SourceMod's on-connect check did reject
+ subsequent connects, but the player's existing session stayed
+ alive until they voluntarily disconnected, often 20-40min later
+ on a low-turnover server). The fix shape is the existing
+ comms.add → blockit.php iframe pattern (canonical:
+ `web/themes/default/page_admin_comms_add.tpl`'s
+ `Actions.CommsAdd .then()` handler; mirrored at
+ `web/themes/default/page_admin_bans_add.tpl`'s
+ `Actions.BansAdd .then()` handler post-#1441): create a hidden
+ `<iframe id="srvkicker" style="display: none">` pointing at
+ `pages/admin.kickit.php?check=…&type=…` and let the iframe's
+ server-side template enumerate enabled servers and fire the
+ rcon kick command via `Actions.KickitLoadServers` +
+ `Actions.KickitKickPlayer`. Mirror the comms.add structure
+ (success branch keeps button busy through a 2s page reload, no
+ `setBusy(btn, false)` in that branch). **The URL parameter
+ `check=` is symmetric across both surfaces; the JS envelope
+ field names are NOT — `r.data.block.steam` on comms.add,
+ `r.data.kickit.check` on bans.add — because `api_bans_add` and
+ `api_comms_add` chose different envelope shapes. Read the
+ server-side response shape FIRST when porting between the two
+ branches; the field-name asymmetry is structural API drift, not
+ a contract violation.** Regression guard:
+ `web/tests/e2e/specs/flows/ban-kickit-iframe.spec.ts` (pins the
+ iframe-spawn contract end-to-end — stubs both
+ `Actions.BansAdd` AND `pages/admin.kickit.php` at the network
+ layer so the spec stays robust under multi-worker DB pressure
+ against the shared `sourcebans_e2e` DB, with `toBe(1)` on the
+ iframe-request count so a future double-mount regression
+ surfaces immediately). The deeper rcon round-trip is exercised
+ by `web/tests/e2e/specs/flows/kickit-iframe.spec.ts` (#1433 —
+ the topbar-context-menu path through the same
+ `pages/admin.kickit.php` document) plus
+ `web/tests/api/KickitTest.php` (PHPUnit — handler permission +
+ malformed-input envelopes). Search anchor for any future sweep:
+ `rg "typeof window\.\w+ === 'function'" web/themes/` —
+ every such guard is a probable dead-helper trap unless the
+ helper is in fact still defined somewhere (e.g.
+ `window.SBPP.showToast` from `theme.js`, which IS the live
+ contract).
 - MooTools `$('id').value` / `$('id').checked` / `$('id').setStyle(…)`
  idioms in inline page-tail scripts → MooTools is gone (deleted
  with sourcebans.js at #1123 D1). Use vanilla DOM:
@@ -4173,6 +4227,7 @@ contacting every contributor individually.
 | 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. |
 | Re-apply (Reban) affordance on the public banlist for expired / unbanned rows | `web/themes/default/page_bans.tpl`. The desktop row-actions cell emits `<a class="btn btn--secondary btn--sm" data-testid="row-action-reapply" href="index.php?p=admin&c=bans&section=add-ban&rebanid={$ban.bid}&key={$admin_postkey}">…<i data-lucide="rotate-ccw">…Re-apply</a>` when `$can_add_ban && ($ban.state == 'expired' \|\| $ban.state == 'unbanned')`. The smart-default block on `admin.bans.php`'s `add-ban` section detects `?rebanid=…` and pre-populates the form via `BansPrepareReban`. Mobile parity (this PR) — the mobile card was restructured from a single wrapping `<a>` to the `page_comms.tpl` shape (`<div data-testid="ban-card">` wrapping `<a class="ban-card__summary" data-testid="drawer-trigger">` + a sibling `<div class="row-actions ban-card__actions">`), so the same Re-apply button surfaces under `data-testid="row-action-reapply-mobile"`. The drawer trigger stays the inner anchor's `data-drawer-href` + `data-testid="drawer-trigger"` so the existing responsive spec selector keeps working. |
+| Fan rcon kick / block commands out to every enabled server after a successful `Actions.BansAdd` / `Actions.CommsAdd` (the hidden `<iframe id="srvkicker">` pattern) | `web/themes/default/page_admin_bans_add.tpl` (#1441 — `Actions.BansAdd .then(r => ...)` handler reads `r.data.kickit` and spawns the iframe pointing at `pages/admin.kickit.php?check=<check>&type=<type>`) + `web/themes/default/page_admin_comms_add.tpl` (the older sibling — `Actions.CommsAdd .then(r => ...)` reads `r.data.block` and spawns the iframe pointing at `pages/admin.blockit.php?check=<steam>&type=<type>&length=<length>`). The URL parameter `check=` is symmetric across both surfaces; the JS envelope field names are NOT — bans.add returns `r.data.kickit.check` while comms.add returns `r.data.block.steam`, because `api_bans_add` and `api_comms_add` chose different envelope shapes when the kickit/blockit envelopes were originally added. Always read the server-side envelope shape (`web/api/handlers/bans.php::api_bans_add` / `web/api/handlers/comms.php::api_comms_add`) FIRST when porting between the two branches. The iframe template (`web/themes/default/page_kickit.tpl` / `page_blockit.tpl`) auto-fires `Actions.KickitLoadServers` (or `Actions.BlockitLoadServers`) on `window.addEventListener('load', ...)` which enumerates enabled servers and fans out per-server `Actions.KickitKickPlayer` / `Actions.BlockitBlockPlayer` rcon calls. The iframe is `display: none` — the operator never sees it, but the panel can spawn it from any surface that needs server-side rcon side-effects (the right-click context menu's "Kick player" item navigates to the same `pages/admin.kickit.php` URL as a top-level document instead — see "Add or extend the server-player right-click context menu" row). Don't reach for `setBusy(submitBtn, false)` after spawning the iframe — the success branch keeps the button busy through the 2s `setTimeout` that reloads the parent page (matching `page_admin_comms_add.tpl`'s shape), preventing operators from queuing a second submit while rcon fans out. Regression guards: `web/tests/e2e/specs/flows/ban-kickit-iframe.spec.ts` (#1441 — stubs both `Actions.BansAdd` and `pages/admin.kickit.php` at the network layer; asserts the iframe is spawned exactly once with `toBe(1)` on the request count so a future double-mount regression — e.g. a `.then()` chain that resolves twice, a `dispatchEvent` re-firing the submit handler, a `setBusy` gap — surfaces immediately) + `web/tests/e2e/specs/flows/kickit-iframe.spec.ts` (#1433 — exercises the deeper rcon round-trip via the context-menu path) + `web/tests/api/KickitTest.php` (PHPUnit — handler permission gate + malformed-input envelopes). Pre-#1441 the bans-add side carried a `typeof window.ShowKickBox === 'function'` guard pointing at a v1.x helper that was deleted at #1123 D1; the `if` body silently never ran and operators reported "banning doesn't kick" (the canonical "chrome reads as success while the load-bearing action never fired" UI regression — see the matching anti-pattern entry under Anti-patterns). |
 | Wrap a public-list legacy advanced-search form (banlist / commslist) in a default-collapsed `<details class="filters-details">` disclosure | `web/themes/default/page_bans.tpl` + `web/themes/default/page_comms.tpl` (#1315). The same `.filters-details` rules in `web/themes/default/css/theme.css` cover the chrome (summary chevron, `[open]` state, hover bg, focus ring, count badge); the public-list usage adds a `.filters-details__body > form.card` rule that suppresses the inner card framing because the disclosure body wraps a `{load_template file="admin.bans.search"}` (or `…comms.search`) — i.e. a sibling page-render that emits its own `<form class="card">`. The View DTO (`Sbpp\View\BanListView` / `Sbpp\View\CommsListView`) carries a `bool $is_advanced_search_open` set by the page handler from `isset($_GET['advSearch']) && (string) $_GET['advSearch'] !== ''` so the legacy `?advSearch=…&advType=…` URL shim auto-opens the disclosure. Bare `?p=banlist` / `?p=commslist` and simple-bar filters (`?searchText=` / `?server=` / `?time=`) leave it closed so the unfiltered list reaches above the fold. Selectors anchor on `[data-testid="banlist-advsearch-disclosure"]` / `[data-testid="commslist-advsearch-disclosure"]` (the `<details>`) + the `…-toggle` (the `<summary>`) + the `…-active` count badge. The same `.filters-details` chrome was introduced for admin-admins by #1303 — both surfaces share the CSS so a future tweak is single-source. |
 | Surface admin-authored per-ban / per-comm comments inline on the public banlist + commslist (`<details data-testid="ban-comments-inline">` / `comm-comments-inline`) | `web/themes/default/page_bans.tpl` + `web/themes/default/page_comms.tpl` (#BANLIST-COMMENTS, this PR). Both desktop tables render a native `<details class="ban-comments-inline">` next to the player-name cell whose `<summary>` doubles as the count chip (icon + tabular-nums count + "comment(s)" label) and whose body lists each comment with `<strong>{$com.comname}</strong>` + timestamp + `<div data-testid="ban-comment-text">{$com.commenttxt nofilter}</div>`. The `nofilter` is load-bearing because `$com.commenttxt` is server-built HTML produced by `encodePreservingBr` (per-segment `htmlspecialchars`, only `<br/>` survives) plus the URL-wrap regex that wraps already-escaped `https?://...` strings in `<a>` tags — same trust contract as the existing comment-edit-mode "Other comments" foreach at the top of `page_bans.tpl`. The mobile cards emit a non-interactive `<div data-testid="ban-comments-count-mobile">` count indicator instead — the card wraps every cell in a single `<a data-drawer-href>`, so a nested `<details>` would be invalid HTML (interactive content inside interactive content); mobile users tap through to the drawer (`renderOverviewPane` paints the same comments under `[data-testid="drawer-comments"]`). Both surfaces gate on `$view_comments` (mirrors the page handler's `Config::getBool('config.enablepubliccomments') \|\| $userbank->is_admin()` check at `page.banlist.php` line 766 / `page.commslist.php` line 734). The drawer surface (`api_bans_detail.comments_visible` + `api_bans_detail.comments`) is unchanged — both surfaces share the same data path and same gate so there's no leak risk. The commslist regression was worse than the banlist's because there's no drawer fallback on `<tr data-testid="comm-row">`; the inline disclosure is the ONLY on-page way for admins to see comm-block comment text. Regression guards: `web/tests/integration/BanlistCommentsVisibilityTest.php` (admin sees disclosure regardless of flag, anonymous gated correctly, drawer mirrors disclosure via `bans.detail`) + `web/tests/e2e/specs/flows/banlist-comments-visibility.spec.ts` (desktop disclosure renders + opens, drawer paints same data, mobile count indicator is a `<div>` not a `<details>`). The fix restores the v1.x `mooaccordion` discoverability that was lost when the v2.0 rewrite of `page_bans.tpl` collapsed the inline panel down to a silent `<span>[N]</span>` count badge with no affordance. |
 | Filter the public banlist by ban state (`?p=banlist&state=permanent\|active\|expired\|unbanned`) — server-side rowset narrowing for a chip strip | `web/pages/page.banlist.php` (#1352). The `?state=` value is allowlisted (`['permanent','active','expired','unbanned']`, anything else falls through to "All"), composed into the existing `$publicFilterAnd` / `$publicFilterWheren` SQL fragments via the per-state predicates documented inline. **Symmetric defensive shape across the four arms**: `permanent` / `active` BOTH require `RemovedOn IS NULL` (otherwise a pre-2.0 admin-lifted row with `ends > now` would match BOTH `?state=active` AND `?state=unbanned`); `expired` carries THREE arms (`RemoveType = 'E'` post-migration shape + `RemoveType IS NULL AND length > 0 AND ends < now AND RemovedOn IS NULL` for pre-475 installs the prune writer never touched + `RemoveType IS NULL AND RemovedOn IS NOT NULL AND length > 0 AND (RemovedBy IS NULL OR RemovedBy = 0)` for the fork-divergence shape `810.php` pass 2 backfills); `unbanned` carries TWO arms (`RemoveType IN ('D', 'U')` post-migration shape + `RemovedOn IS NOT NULL AND RemoveType IS NULL AND RemovedBy IS NOT NULL AND RemovedBy > 0` for pre-2.0 admin-lifts that `810.php` pass 1 backfills). The `RemovedBy > 0` vs `RemovedBy IS NULL OR = 0` split between unbanned / expired is the **load-bearing distinction** — natural expiry sets `RemovedBy = 0` (PruneBans) or NULL (pre-475); admin lifts set `RemovedBy = <aid>`. The View DTO (`Sbpp\View\BanListView`) carries `string $active_state` + `string $chip_base_link`; the chip strip in `web/themes/default/page_bans.tpl` renders as real `<a href="index.php?p=banlist{$chip_base_link}&state=…">` anchors with `aria-current="true"` (NOT `aria-pressed` — only valid on role=button; axe rejects it on `<a>` as `aria-allowed-attr`) and `data-active="true"` server-rendered on the matching chip — never `<button onclick=…>` (the legacy `web/scripts/banlist.js applyStateFilter` row-hide layer was a client-side filter on top of server-side pagination, so on installs with thousands of bans the unbanned chip silently rendered an empty page 1). The "Hide inactive" toggle is suppressed in the template when `$active_state !== ''` so the two predicates (`hideinactive` is `RemoveType IS NULL`; `?state=expired` / `unbanned` ask for the OPPOSITE) don't visually fight. Pagination URLs preserve `&state=` via `$stateFilterLink`; the per-row state classifier in the same file mirrors the SQL filter (a row pulled in by `?state=unbanned` renders the Unbanned pill, never the legacy mis-classified "Active" pill). The JSON API parity surface is `api_bans_detail` + `api_bans_player_history` in `web/api/handlers/bans.php` — same `isPre2AdminLift` defensive branch so the drawer's detail view + the history pane don't visibly contradict the SQL filter. Regression guards: `web/tests/integration/BanListStateFilterTest.php` (per-state row inclusion / exclusion + chip render contract + suppressed Hide-inactive toggle + symmetric `expired` arm 3 contract), `web/tests/integration/UpdaterBackfillRemoveTypeTest.php` (the migration's idempotency + two-pass cross-contamination contract), `web/tests/api/BansTest.php::testDetailReportsUnbannedForPre2AdminLiftWithRemoveTypeNull` (API parity), `web/tests/e2e/specs/responsive/banlist.spec.ts` + `web/tests/e2e/specs/responsive/filters.spec.ts` (mobile chip click navigates + asserts `aria-current` rather than `history.replaceState` + `aria-pressed`'ing). |