Repository navigation
Fix #1311: cache + debounce A2S queries on the public servers page - #1329
Merged
Merged
Conversation
…#1311) The public servers page (`?p=servers`) and its per-tile Re-query button fanned one anonymous-callable A2S `GetInfo + GetPlayers` UDP probe out to each configured `:prefix_servers` row on every panel hit and every button click. A hand-mash of the refresh button — or a `for` loop hitting `?p=servers` — translated 1:1 to A2S queries leaving the panel host. The dispatcher's only friction was CSRF, which anonymous browsers get for free off the public chrome. Server-side: every public server-query handler (`servers.host_players` / `host_property` / `host_players_list` / `players`) now goes through `Sbpp\Servers\SourceQueryCache::fetch($ip, $port)` — an on-disk cache keyed by `(ip, port)` with a ~30s window. Mirrors the `system.check_version` cache shape (atomic tempfile + `rename()`). Both success and failure paths cache so an unreachable host costs ONE probe per window, not one per request. The cache is user-agnostic; the handler stamps per-caller fields (`is_owner`, `can_ban`, `trunchostname`) on top after the fetch. Client-side: the per-tile Re-query button (`[data-testid="server-refresh"]`) now starts `disabled` and only re-enables after the bootstrap probe settles, mirroring the pre-existing toggle button gate. The JS-side `tile.__sbppLoading` flag short-circuits clicks that race the `disabled` attribute settling in the DOM. Tests: `web/tests/integration/SourceQueryCacheTest.php` covers the cache shape (hit / miss / negative cache / TTL expiry / atomic write / invalidation) via the test-only probe override; new handler-shape assertions in `web/tests/api/ServersTest.php` verify rapid repeat calls coalesce and unreachable servers are negative-cached. Playwright spec `web/tests/e2e/specs/flows/server-refresh-debounce.spec.ts` asserts five rapid clicks coalesce into a single XHR. Docs: `_register.php` documents the intentional `public=true` reach on the `servers.host_*` family + why the cache is what makes it safe; `AGENTS.md` and `ARCHITECTURE.md` add the Servers/ subsystem row. Fixes #1311
rumblefrog
added a commit
that referenced
this pull request
May 10, 2026
Two test-only fixes surfaced when the PR rebased on top of #1320 + #1329 landing in main: - `BanListIpColumnTest::renderBanList` (from #1320) constructs a `BanListView` with positional + named arguments. This PR adds a 28th constructor parameter (`is_advanced_search_open`) which the test must now pass — default it to `false` so the IP-column suite stays exercising the column-render contract it was written for, not the disclosure state. - `ServerMapImageRenderTest::testHandlerStillEmitsMapimgField` (from #1326) asserted a literal `"'mapimg' => GetMapImage("` substring in `web/api/handlers/servers.php`. #1329's cache + debounce rewrite shifted the surrounding column alignment to 5 spaces, breaking the brittle match without changing the contract the test guards (the field is still emitted). Switch to a regex that tolerates any whitespace between the key and `=>` so a future alignment shift doesn't silently re-trigger the same false positive.
rumblefrog
added a commit
that referenced
this pull request
May 10, 2026
Two test-only fixes surfaced when the PR rebased on top of #1320 + - `BanListIpColumnTest::renderBanList` (from #1320) constructs a `BanListView` with positional + named arguments. This PR adds a 28th constructor parameter (`is_advanced_search_open`) which the test must now pass — default it to `false` so the IP-column suite stays exercising the column-render contract it was written for, not the disclosure state. - `ServerMapImageRenderTest::testHandlerStillEmitsMapimgField` (from #1326) asserted a literal `"'mapimg' => GetMapImage("` substring in `web/api/handlers/servers.php`. #1329's cache + debounce rewrite shifted the surrounding column alignment to 5 spaces, breaking the brittle match without changing the contract the test guards (the field is still emitted). Switch to a regex that tolerates any whitespace between the key and `=>` so a future alignment shift doesn't silently re-trigger the same false positive.
Rushaway
pushed a commit
to srcdslab/sourcebans-pp
that referenced
this pull request
May 15, 2026
…ban-meta + Re-apply + collapsible adv search) (sbpp#1330) * fix(banlist+commslist): restore unban-meta line + Re-apply + collapsible advanced search (sbpp#1315) The v2.0 redesign collapsed the sourcebans.js-driven legacy public lists into Smarty-only `page_bans.tpl` / `page_comms.tpl` and dropped three power-user surfaces along the way. This PR restores them. 1. Inline unban-meta line on admin-lifted rows. The reason cell on the desktop table emits "Unbanned by <admin>: <reason>" (`[data-testid="ban-unban-meta"]`) when `state == 'unbanned'`; mobile cards mirror with the `-mobile` suffix. Same shape on the commslist for `state == 'unmuted'` — higher priority there because no drawer fallback exists. Both are gated on `!$hideadminname` so anonymous viewers under a hidden-admins config don't get the admin name leaked. Read-side render only — no overlap with sbpp#1301 / sbpp#1323. 2. Re-apply (Reban) icon affordance for expired / unbanned rows on the banlist desktop, gated on `ADMIN_OWNER | ADMIN_ADD_BAN`. Deep-links the smart-default `?p=admin&c=bans§ion=add-ban&rebanid=<bid>` URL the existing `BansPrepareReban` JSON action already pre-populates. Mobile parity deferred — drawer is canonical. 3. Advanced-search disclosure on both lists. Default-collapsed `<details class="filters-details">` wrapping the legacy multi- criterion search form (loaded via `{load_template file="admin.bans.search"}`). Auto-opens on a post-submit `?advType=&advSearch=` paint via a new `BanListView::$is_advanced_search_open` / `CommsListView::$is_advanced_search_open` boolean. Reuses the `.filters-details` chrome sbpp#1303 introduced for admin-admins (CSS rules ship in this PR; will dedupe harmlessly when sbpp#1318 merges). Tests: - PHPUnit: `PublicBanListRegressionTest` (7 cases — disclosure default-closed, auto-open, unban-meta render, Re-apply gating + URL shape, banlist + commslist parity). RunInSeparateProcess per method to dodge the page-handler `setPostKey()` redeclare trap, mirroring `Php82DeprecationsTest`'s pattern. - Playwright: `flows/public-banlist-regressions.spec.ts` (4 cases across chromium + mobile-chromium — disclosure default-closed, click-to-open, post-submit auto-open via URL navigation; closed-state axe coverage). Re-apply / unban-meta DB-state assertions live in the PHPUnit test where seeding is cleaner. AGENTS.md "Where to find what" gains three rows for the new patterns. * fix(tests): update integration tests after rebase on main Two test-only fixes surfaced when the PR rebased on top of sbpp#1320 + - `BanListIpColumnTest::renderBanList` (from sbpp#1320) constructs a `BanListView` with positional + named arguments. This PR adds a 28th constructor parameter (`is_advanced_search_open`) which the test must now pass — default it to `false` so the IP-column suite stays exercising the column-render contract it was written for, not the disclosure state. - `ServerMapImageRenderTest::testHandlerStillEmitsMapimgField` (from sbpp#1326) asserted a literal `"'mapimg' => GetMapImage("` substring in `web/api/handlers/servers.php`. sbpp#1329's cache + debounce rewrite shifted the surrounding column alignment to 5 spaces, breaking the brittle match without changing the contract the test guards (the field is still emitted). Switch to a regex that tolerates any whitespace between the key and `=>` so a future alignment shift doesn't silently re-trigger the same false positive. * fix(tests): login as admin in BanListIpColumnTest hideplayerips=false case sbpp#1315's `<details class="filters-details">` disclosure on the public ban list `{load_template file="admin.bans.search"}`s the legacy advanced-search partial. The partial's backing `admin.bans.search.php` re-derives `hideplayerips` / `hideadminname` from `Config::getBool('banlist.hideplayerips') && !$userbank->is_admin()` and `Renderer::render`s the result into Smarty, overwriting whatever the parent BanListView had assigned. In production this is a no-op because the parent (`page.banlist.php`) computes the SAME formula — both halves converge. But `BanListIpColumnTest` hand-crafts `BanListView::$hideplayerips` to exercise the column-gating contract in isolation, so the partial's overwrite collapses the test's fixture-controlled value down to whatever the test environment's Config + $userbank happen to evaluate to. Fix by logging in as admin in the two test methods that pass `hideplayerips: false`. With `is_admin()=true`, the partial's formula reduces to `(…) && !true = false`, matching the View's hand-crafted `false`. The suppressed-state test (`hideplayerips: true`) stays anonymous so the formula reduces to `Config && true` — `true` under `data.sql`'s `banlist.hideplayerips=1` default — also matching the View's input. The test pre-dates sbpp#1315; it was written against the v2.0 template that didn't yet `{load_template}` the partial, so the parent's assigns were never overwritten. The new disclosure brings the partial back into the render tree and the test has to maintain the production invariant ("BanListView's hideplayerips matches AdminBansSearchView's recomputed value") explicitly.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #1311.
Summary
Sbpp\Servers\SourceQueryCache::fetch($ip, $port)wraps every public server-query handler (servers.host_players/host_property/host_players_list/players) so back-to-back calls coalesce into ONE A2SGetInfo + GetPlayersUDP probe per(ip, port)per ~30s window. On-disk underSB_CACHE/srvquery/<sha1>.json, atomic tempfile +rename()writes mirroringsystem.check_version's release cache. Both success and failure paths cache so an unreachable host costs ONE probe per window, not one per request. The cache is user-agnostic; per-caller fields (is_owner,can_ban,trunchostname) are stamped on top by the handler after the fetch returns.[data-testid="server-refresh"]inpage_servers.tpl) now startsdisabledand only re-enables after the bootstrap probe settles, mirroring the pre-existing toggle button gate. The JS-sidetile.__sbppLoadingflag short-circuits clicks that race thedisabledattribute settling in the DOM._register.phpdocuments the intentionalpublic=truereach on theservers.host_*family plus why the cache is what makes it safe;AGENTS.mdadds theSbpp\Servers\SourceQueryCacherow in both the namespace table and the "Where to find what" index;ARCHITECTURE.mdadds theServers/directory row + a "Server query cache" section under the Web panel chapter.Test plan
./sbpp.sh phpstan./sbpp.sh test./sbpp.sh test --filter='SourceQueryCache|HostPlayers'host_playerscoalescing + negative cache)./sbpp.sh ts-check./sbpp.sh composer api-contractapi_servers_host_players)./sbpp.sh e2e specs/flows/server-refresh-debounce.spec.tsservers.host_playersPOST and the refresh button is server-rendereddisableduntil the bootstrap probe lands./sbpp.sh e2e specs/smoke/servers.spec.tsThe new E2E spec opts out of the
mobile-chromiumproject (file-level comment explains: contract is browser-shape-agnostic, and the second project's worker would race truncate-and-reseed against the sharedsourcebans_e2eschema on the local-defaultworkers: undefined; CI'sworkers: 1would be safe but the file-local skip keeps the test deterministic everywhere).