Repository navigation
fix(toast): lift Sbpp\View\Toast::emit + convert 35 ShowBox sites (#1403) - #1411
Merged
Merged
Conversation
…1403) `ShowBox()` lived in `web/scripts/sourcebans.js`, deleted at #1123 D1 (v2.0.0). Every `echo "<script>ShowBox(...)</script>"` server-side emit since then has thrown `ReferenceError: ShowBox is not defined` and (worse) suppressed the page body because the toast `echo` ran upstream of `PageDie()`'s footer — users saw a literally blank page on lostpassword resets, protest submits, banlist/commslist GET-fallback unbans, and admin comm-block edits, even though the data-changing path itself completed server-side. Lift the canonical `emitSubmitToast()` helper from `page.submit.php` into `Sbpp\View\Toast::emit(string $kind, string $title, string $body, ?string $redirect = null): void` (new file at `web/includes/View/Toast.php`, PSR-4 autoloaded, no `class_alias` shim — every call site uses the FQN). The helper stashes the payload in a `<script type="application/json" class="sbpp-pending-toast" data-testid="pending-toast">{...}</script>` block, JSON-encoded with `JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT` so it can never escape its `<script>` wrapper (the `<script type="application/json">` slot is the chrome's escape contract). `theme.js`'s new `flushPendingToasts()` reads every `script.sbpp-pending-toast` on `DOMContentLoaded`, calls `window.SBPP.showToast(...)` for each payload, and honours the optional `redirect` field with a 1500ms post-paint settle so the user sees the toast before the navigation lands. Multiple pending toasts are supported (FIFO-rendered, redirect taken from the last payload that carries one). The element is removed from the DOM after parse so a future `flushPendingToasts()` call (defensive) is a no-op. Mechanically converted 35 sites across: - `page.lostpassword.php` (5) — incl. the marquee "your password has been reset and emailed" success - `page.protest.php` (3) - `page.banlist.php` (9) — GET-fallback unban / delete toasts - `page.commslist.php` (12) — GET-fallback ungag / unmute / unblock - `admin.edit.comms.php` (5) — URL-key error, missing-id, access denied, "Block updated", "Block not found" - `page.submit.php` (1) — the lift's source; converted to use the new FQN like every other site `page.home.php` `$info['popup']` (1 site) intentionally NOT touched — sister #1404's scope (dead PHP-side data field; the dashboard template never consumed it anyway). `admin.edit.comms.php`'s "block not found" branch (line 167) ALSO gained a `PageDie()` after the toast emit. Pre-#1403 the legacy `ShowBox` call carried its own `window.location` navigation that beat the page render to the user's eyeballs; post-lift the toast honours the redirect via the helper's 4th arg + theme.js's 1500ms settle, so the user MUST NOT see a borked form skeleton in the gap. Dereferencing the `false` `$res` from the failed SELECT in `Renderer::render` + the inline `<script>` further down emits PHP warnings under `display_errors=On`, and those warnings land literally inside JavaScript string literals (the `selectLengthTypeReason('<?= ?>', '<?= ?>')` short-echo block) — every warning is plain text inside quoted JS strings, which is a JS `Invalid or unexpected token` parse error. `PageDie()` cuts the render before any of that runs. Regression guards: - `web/tests/integration/ToastEmitRegressionTest.php` - Static grep across `web/pages/*.php` for `<script>ShowBox(` / `<script>showBox(` (excluding inline tail-script handlers that legitimately reference the legacy helper for back-compat, which is sister #1402's scope). - Wire-format contract: payload shape, JSON encoding flags actually escape `<` / `&` / `'` / `"`, redirect field omitted when null. - Call-site presence: every audited file still emits at least one `Toast::emit(...)` so a future "let's drop the toast" accident fails the build. - E2E coverage under `web/tests/e2e/specs/flows/`: - `lostpassword-toast.spec.ts` — invalid + mismatch validation paths render the toast + footer chrome. - `protest-toast.spec.ts` — invalid SteamID format + valid-but- not-banned SteamID. - `banlist-getfallback-toast.spec.ts` — missing `ureason` + nonexistent bid on `?p=banlist&a=unban` / `&a=delete`. Seeds via `seedBanViaApi` in `beforeEach` (tolerates `already_banned` on the shared e2e DB for replay safety). - `commslist-getfallback-toast.spec.ts` — missing `ureason` + nonexistent cid on `?p=commslist&a=ungag` / `&a=unmute` / `&a=remove`. Seeds via `seedCommViaApi`. - `admin-edit-comms-toast.spec.ts` — URL-key mismatch, missing `?id=`, nonexistent block id. Seeds a comm in `beforeEach` so `extractPostkey` can pull a real session-postkey from a `data-fallback-href` on the commslist page. All five specs share the same assertion contract: 1. The `<script type="application/json" class="sbpp-pending-toast">` block exists in the rendered HTML (the wire-layer contract). 2. The toast `[data-testid="toast"]` renders post-DOMContentLoaded with the expected title + body. 3. The chrome footer is present (regression guard against the pre-fix "blank page" symptom — `PageDie()` runs and the toast doesn't suppress it). 4. No `Invalid or unexpected token` console errors (regression guard for the `admin.edit.comms.php` `display_errors=On` shape above). 5. No raw `<script>ShowBox(...)` blob survives in the response body (per-spec static guard at the wire layer). Overlap with #1402: `admin.edit.comms.php`'s `$errorScript` block (originally line 181, MooTools `window.addEvent('domready', …)` wrapping `$('id').setStyle(...)` form-field error highlights) is sister #1402's scope. This PR leaves the wrapper alone and only converts the inner `ShowBox` calls into `Toast::emit`. When #1402 rewires the form-field error highlights, the `Toast::emit` calls inside `$errorScript` stay as-is — the helper has no MooTools dependency. Quality gates (all from the worktree at parallel project `sbpp-task-1403`): phpstan ✓, test (682 ✓), ts-check ✓, composer api-contract (no diff) ✓, e2e --workers=1 (299 ✓ / 279 skipped / 0 failed; the focused #1403 subset is 53 ✓ / 15 skipped).
…th coverage, AGENTS.md docs (#1403) Addresses the adversarial review of the lift commit (8484b83) with five real fixes, the mandatory AGENTS.md doc updates, and one clarification: 1. **AGENTS.md doc updates** (HIGH — the "Keep the docs in sync" mandate). Added: a "Server-side toast emission (`Sbpp\View\Toast::emit`)" Conventions block documenting wire format, encoder flags, FIRST-wins-redirect semantics, escapeHtml body rule, 1500ms settle delay; an Anti-patterns entry on raw `<script>ShowBox(...)</script>` blobs (and the `<script type="text/javascript">` variant) with sister anti-pattern entries on dropping `JSON_INVALID_UTF8_SUBSTITUTE` and adding `data-testid` back to the wire-format block; a "Where to find what" row pointing at `Sbpp\View\Toast::emit`; a per-class namespacing table row. 2. **`JSON_INVALID_UTF8_SUBSTITUTE`** (REAL DEFECT). The reviewer verified that interpolating a malformed-UTF-8 `$row['name']` from the GET-fallback unban / unmute / delete paths in `page.banlist.php` / `page.commslist.php` threw `JsonException` on `Toast::emit` — the unban / delete SQL had already committed so the audit log showed success while the operator got a 500. Added the substitute flag to the encoder; paired `testToastEmitSubstitutesMalformedUtf8InsteadOfThrowing` test asserts the offending bytes substitute to U+FFFD and the payload parses cleanly downstream. Player names from the #1108 / #765 Latin-1-on-utf8 truncation shape are the primary reproducer. 3. **Happy-path lostpassword E2E test** (REAL GAP). The marquee user-reported regression from the issue body was specifically the password-reset success branch — the spec only covered the two error branches. Added `Happy path: password reset → "Password Reset" toast + chrome footer + email lands in mailpit` which seeds SMTP config + a validation token via a new `seed-lostpassword-e2e.php` shim, drives the success branch, asserts the toast paints AND polls mailpit's HTTP API for the password-reset email at the seeded admin's address. New `seedLostpasswordE2e` helper in `fixtures/db.ts` mirrors the existing `seedAnnouncementsE2e` / `seedCommsRawE2e` shape; the spec gates on `process.env.E2E_MAILPIT_BASE_URL ?? 'http://mailpit:8025'` so it works both in-container and from the host (via the worktree-local published port `:10191`). 4. **Drop `data-testid="pending-toast"` from the wire format** (REAL DEFECT). The wire-format testid would collide across multi-emit responses (the helper allows several emits per request); Playwright's `getByTestId(...)` strict mode rejects multi-match. The `sbpp-pending-toast` class is the consumer selector; E2E specs anchor on the rendered `[data-testid="toast"]` element (chrome-rendered by `showToast`) or `[role="status"]`. Updated `ToastEmitRegressionTest::testToastEmitWireFormatStaysStable` to assert the wire-format block carries NO `data-testid` attribute. None of the existing E2E specs anchored on the wire-format testid (they probe `class="sbpp-pending-toast"` on the response body) so no spec changes were needed beyond docblock scrubbing. 5. **Tightened `ToastEmitRegressionTest` docblock + test name** (LOW nit #6 — judgment call documented). Renamed `testNoRawShowBoxBlobsRemainInPageHandlers` → `testNoRawShowBoxBlobsRemainInPhpPageHandlers` and added a "Scope: PHP page handlers only" section to the file docblock explicitly stating template-side `<script>ShowBox(...)</script>` regressions are sister #1402's surface (the AGENTS.md "Legacy 1.4.11 JS handler names" anti-pattern). Keeps this gate's purpose obvious without expanding to every conceivable `ShowBox` vector. Worker self-report correction (HIGH #2): the previous PR description claimed "last redirect wins"; the implementation in `theme.js`'s `flushPendingToasts` is actually FIRST-wins (`if (redirectTo === null …) redirectTo = data.redirect;`). The implementation is correct; the self-report was wrong. The PR body documents the correct FIRST-wins semantic and a paired `testToastEmitRedirectFirstWinsContract` test pins the wire-side contract (the encoder must emit redirects in call order so the chrome can pick its own winner — pre-collapsing would silently change which URL the chrome navigates to). LOW nit #9 (`?p=commsist` → `?p=commslist` typo fix) was already landed in the first commit per the reviewer's recommendation; documented in the PR description for git-blame. Follow-up issues filed: #1409 — Optional ?int $duration_ms parameter on Toast::emit (preserves the v1.x ShowBox(..., true) "don't auto-close" semantic the mechanical lift dropped on 5 NOT-* branches) #1410 — admin.edit.comms.php runs permission check before the not-found guard (pre-existing bug, out of #1403 scope, surfaced during this review) Quality gates: PHPStan + PHPUnit (684 tests, 2729 assertions) + ts-check + api-contract + Playwright E2E (301 passed, 279 skipped, 0 failures) at --workers=1.
…landed The `web/scripts/sourcebans.js` anti-pattern entry was the only file in conflict: - #1403 (this branch) added the `<script>ShowBox(...)</script>` raw- blob anti-pattern entry + two follow-on bullets (the `JSON_INVALID_UTF8_SUBSTITUTE` requirement and the testid-on-the- wire-format ban) right after the entry's tail line. - #1404 (main) extended the SAME tail line with the per-row `LoadServerHost` / `LoadServerHostPlayersList` history paragraph. Both extensions belong; the resolution interleaves them — #1404's extended-tail paragraph stays at the end of the existing entry, then the three new bullets from #1403 (ShowBox / UTF-8 / testid) land as sibling bullets in the same anti-patterns block. Also de-duplicated the now-obsolete "page.home.php's $info['popup'] field is the one intentional survivor at the PHP layer (sister #1404 owns the cleanup)" sentence by trimming the parenthetical (#1404 actually shipped that cleanup, so the survivor framing is no longer accurate — replaced with "went at sister #1404 (dead data; pure cleanup, no toast-emit conversion needed)."). The auto-merged page.banlist.php / page.commslist.php only have the Toast::emit insertions ride next to the unrelated dead-field deletions; no manual resolution needed there.
…nded The fixtures helper file had two clean side-by-side additions: - #1403 added SEED_LOSTPASSWORD_INSIDE_CONTAINER + seedLostpasswordE2e() - #1408 added SET_SETTING_INSIDE_CONTAINER + setSettingE2e() The resolution keeps both helpers — they're independent shells around different per-spec PHP fixtures (#1403's seeds an admin token + SMTP config for the lostpassword happy path; #1408's writes a sb_settings row for the groupban dispatcher spec). No semantic overlap, no ordering concerns. The auto-merges in admin.edit.comms.php / page.banlist.php / page.commslist.php / AGENTS.md are clean — #1403's Toast::emit conversions sit upstream of #1408's page-tail script rewrites in each file, and AGENTS.md added entries to two distinct anti-pattern blocks that didn't share anchor lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1403.
Six PHP page handlers emitted
<script>ShowBox(...)</script>blobs from server-side branches;ShowBoxwas deleted at #1123 D1 so each threwReferenceError. Worse, several ran upstream ofPageDie()(which renders the footer + exits), suppressing the template body too — the user saw a literally blank page with no indication of success or failure. The marquee case:?p=lostpasswordsuccess path silently emailed the new password while showing nothing to the user, leading to "I clicked Reset Password three times because nothing happened" double-submits that burned three validation tokens.Fix shape:
Sbpp\View\Toast::emit(string $kind, string $title, string $body, ?string $redirect = null): voidatweb/includes/View/Toast.php. Stashes the payload in<script type="application/json" class="sbpp-pending-toast">…</script>whichtheme.js'sflushPendingToastspicks up onDOMContentLoaded(sidesteps thePageDie()ordering problem because the toast container exists by the time the chrome JS reads the blob).<script>ShowBox(...)</script>sites converted across 6 files (lostpassword 5, protest 3, banlist 9, commslist 12, admin.edit.comms 5, page.submit 1 = the lift source).JSON_THROW_ON_ERROR | JSON_INVALID_UTF8_SUBSTITUTE | JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOTso malformed UTF-8 in player names (historical Latin-1-on-utf8 truncation shape, see Fix character-encoding regression in ban and server-info flows #1108/Query_AddBlockInsert failed: Incorrect string value #765) substitutes to U+FFFD instead of throwing a 500 — important because the unban / delete SQL has ALREADY committed by the time the toast emits.try/catchso a malformed entry can't take down the queue. (Thetheme.jsconsumer readsif (redirectTo === null && typeof data.redirect === 'string' && data.redirect !== '') redirectTo = data.redirect;— first non-empty redirect wins; subsequent emits' redirects are dropped silently. In practice a single request never emits more than one redirect, but FIRST-wins is the safer default.)data-testidon the wire-format<script>block — a multi-emit response would emit several blocks with the same testid and Playwright'sgetByTestId(...)strict mode would reject the match. E2E specs anchor on the painted[data-testid=\"toast\"]element (chrome-rendered byshowToast) or[role=\"status\"]; wire-layer specs probe the response body directly forclass=\"sbpp-pending-toast\".selectLengthTypeReason(…)short-echo block inadmin.edit.comms.php's not-found branch crashed underdisplay_errors=Onwhen$reswas false — added pairedPageDie()so the toast can paint cleanly (flagged in the PR for review attention).data-testidback); "Where to find what" row; per-class Namespacing table entry. Per the "Keep the docs in sync" mandate.Drive-by fixes flagged transparently:
page.commslist.php:204—?p=commsist(404) →?p=commslist. Pre-existing typo in legacyShowBoxcall.Test plan
ToastEmitRegressionTest: 7 tests covering wire format, JSON escape contract, malformed-UTF-8 substitute, FIRST-wins-redirect contract, static grep guard for PHP page handlers, regression guard for "every audited page still calls Toast::emit")--workers=1(301 passed, 279 skipped, 0 failures):lostpassword-toast.spec.ts(invalid token / mismatch / happy-path: reset → toast + chrome footer + email lands in mailpit)protest-toast.spec.tsbanlist-getfallback-toast.spec.ts(GET fallback unban / delete)commslist-getfallback-toast.spec.ts(GET fallback ungag / unmute / remove)admin-edit-comms-toast.spec.tsb048195cfor the per-defect mapping.Commits
Two commits, intentionally not squashed (review audit history):
8484b834— the mechanical lift (helper + 35 site conversions + initial tests)b048195c— review-fix commit (UTF-8 substitute flag + happy-path mailpit test + drop wire-formatdata-testid+ tighten static-guard scope + AGENTS.md doc updates)Follow-up issues filed
?int \$duration_msonToast::emitfor persistent error toasts (preserves the v1.xShowBox(..., 'red', ..., true)"don't auto-close" semantic that this PR's mechanical lift dropped on 5 NOT-* branches; non-blocking, 4s default is fine for the converted call sites)admin.edit.comms.phpruns the permission check before theif (!\$res)not-found guard, so non-Owner admins hitting a non-existent bid see "no access" instead of "block not found" (pre-existing bug, surfaced during this review, out of Audit follow-up: silent <script>ShowBox(...)</script> blobs lose user feedback on lostpassword / protest / banlist / commslist / admin.edit.comms #1403 scope)Related
sourcebans.js