Repository navigation
Fix #1312: restore map thumbnail on expanded server cards - #1326
Merged
Merged
Conversation
The v2.0.0 redesign at #1123 D1 rebuilt page_servers.tpl from the handoff card grid and dropped the legacy `<img id="mapimg_{$server.sid}">` slot. The API handler still returns the `mapimg` URL (api_servers_host_players → GetMapImage), so the field had no consumer — every expanded card showed the map name as text only. Restore the surface by adding a hidden `<img data-testid="server-map-img">` inside the players panel and wiring the inline initializer to patch `src` from `r.data.mapimg`. The slot only paints once `onload` fires; on `error` it stays hidden so missing files (a fork without `nomap.jpg`, an unknown map without a screenshot) degrade to "no thumbnail" instead of a broken-image icon. Regression guards: - web/tests/integration/ServerMapImageRenderTest.php pins the static contract: handler still emits `mapimg`, template ships the `<img>` slot inside the players panel, inline JS wires `d.mapimg` + `onload`/`onerror`, GetMapImage falls back to `nomap` and the bundled `nomap.jpg` placeholder ships. - web/tests/e2e/specs/flows/server-map-thumbnail.spec.ts pins the runtime observable across the three terminal paths: bundled image loads + becomes visible, missing image keeps the slot hidden, server-offline keeps the slot hidden. Documents the new render path in AGENTS.md's "Where to find what" table so the surface stays discoverable.
6 tasks done
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.
Summary
Fixes #1312.
The v2.0.0 redesign at #1123 D1 rebuilt
page_servers.tplfrom the handoff card grid and dropped the legacy<img id="mapimg_{\$server.sid}">slot. The API handler still returns themapimgURL (api_servers_host_players→GetMapImage()), so the field had no consumer — every expanded server card showed the map name as text only.This PR restores the surface by:
<img data-testid="server-map-img">inside the players panel (so it rides the same[hidden]toggle that surfaces the player list — the issue's "expanded server card" framing).srcfromr.data.mapimg. The slot only paints onceonloadfires; onerrorit stays hidden so missing files (a fork withoutnomap.jpg, an unknown map without a screenshot) degrade to "no thumbnail" instead of a broken-image icon.nomap.jpgalready ships inweb/images/maps/so the standard fallback path renders the bundled placeholder.AGENTS.md's "Where to find what" table.No backend / API contract changes — the handler already emits
mapimg. PHPGetMapImage()(web/includes/system-functions.php) is unchanged.Regression guards
web/tests/integration/ServerMapImageRenderTest.php— static contract: handler still emitsmapimg, template ships the<img>slot inside the players panel, inline JS wiresd.mapimg+onload/onerror,GetMapImage()falls back tonomapand the bundlednomap.jpgplaceholder ships.web/tests/e2e/specs/flows/server-map-thumbnail.spec.ts— runtime observable across the three terminal paths: bundled image loads + becomes visible, missing image keeps the slot hidden, server-offline keeps the slot hidden.Test plan
Quality gates run inside the parallel Docker stack scoped to
sbpp-task-1312:./sbpp.sh phpstan— pass (228/228, no errors)./sbpp.sh test— pass (408 tests, 1777 assertions)./sbpp.sh test --filter ServerMapImageRenderTest— pass (5 tests, 12 assertions, all green)./sbpp.sh ts-check— pass (no.jsfiles touched; the inline<script>in the.tplis already covered by the existing JSDoc)./sbpp.sh e2e specs/flows/server-map-thumbnail.spec.ts --workers=1— pass (6 tests across bothchromium+mobile-chromiumprojects)./sbpp.sh e2e specs/smoke/servers.spec.ts specs/flows/empty-states.spec.ts --workers=1— pass (no regressions on the related surfaces)./sbpp.sh e2e --grep "servers" specs/a11y/routes.spec.ts --workers=1— pass (0 critical axe violations on?p=serverslight + dark)API contract regen not required — no handler signatures changed.