Repository navigation
Fix #1306: drop misleading right-click hint on the public Servers page - #1319
Merged
Merged
Conversation
The Servers page header (`?p=servers`) used to render an admin-only
hint that promised a right-click context menu on each expanded
server card's player rows ("Right-click a player on an expanded card
to kick, ban, or message them"). The supporting JS lived in
`web/scripts/sourcebans.js`'s `LoadServerHost(...)` helper, which
deleted the per-row `AddContextMenu(...)` call along with the rest
of the file at #1123 D1. The v2.0.0 `page_servers.tpl` rewrite never
re-registered the menu — so the help text described a feature that
no longer ships, and right-clicking a player row falls through to
the browser's native context menu.
Restoring the menu would need an extra RCON `status` round-trip per
server card (the SourceQuery `GetPlayers()` UDP response does not
carry SteamIDs) — doubling per-card latency and only working for
admins mapped to servers with rcon — which is out of scope for this
public surface. The kick/ban/mute UX is reachable from
`?p=admin&c=kickit`, `?p=admin&c=blockit`, and the per-row
affordances on the public ban / comm lists, so admins are not
without a path.
Changes:
- Drop the `<p data-testid="servers-rcon-hint">` block from
`page_servers.tpl`; replace it with an in-template comment
pointing future contributors at the missing wiring so the hint
isn't accidentally re-added without it.
- Drop `IN_SERVERS_PAGE` and `access_bans` from `ServersView` (the
hint conditional was their only consumer in `page_servers.tpl`)
and update `page.servers.php` and `page.home.php` accordingly.
`page.home.php` now computes `access_bans` directly via
`HasAccess(...)` and passes the literal `false` for
`IN_SERVERS_PAGE` (the dashboard always was the false case).
- Burn the dead context-menu helpers: delete `contextMenoo.js`,
remove `sb.contextMenu` and the global `AddContextMenu` shim from
`sb.js`, and drop the matching `SbContextMenuItem` /
`contextMenoo` / `AddContextMenu` ambient declarations from
`globals.d.ts`. None had call sites anywhere in the panel.
- Add a paired anti-pattern entry to `AGENTS.md` so a future
contributor can't reintroduce just the hint copy without the
underlying menu (and the SteamID extension to
`api_servers_host_players` it would need).
- Update `ARCHITECTURE.md` to drop the deleted `contextMenoo.js`
row from the `web/scripts/` listing and the per-file role table.
- Add `ServerListHintRegressionTest` (`web/tests/integration/`)
pinning two invariants: the rendered HTML for `?p=servers` (admin
+ at least one seeded server) does NOT contain the hint testid or
copy, and `Sbpp\View\ServersView`'s constructor no longer accepts
the `IN_SERVERS_PAGE` / `access_bans` named params. Mirrors the
AdminAdminsSearchTest harness shape (real Smarty + `ob_start`
capture against the production page handler).
Quality gates (./sbpp.sh):
- phpstan: pass
- test (full PHPUnit, 405 tests): pass
- ts-check: pass
- composer api-contract: clean diff
- e2e (CI=1, workers=1, full suite, 192 tests): pass
#1306 The "for the transitively included page_servers.tpl keeps type-checking" rationale these comments cited no longer applies — #1306 dropped IN_SERVERS_PAGE / access_bans from page_servers.tpl along with the right-click hint they gated. Update the HomeDashboardView docblock to describe what each preserved prop actually does today (access_bans gates the dashboard's "Open the servers page…" hint; IN_SERVERS_PAGE survives as parity scaffolding only; opened_server sits in the PHPStan baseline), the page_dashboard.tpl parity-block comment to drop the same stale rationale, and the ServersView \$server_list[evOnClick] docblock to refer to IN_HOME (the actual gate in page.servers.php) instead of IN_SERVERS_PAGE. Pure documentation tightening — no behaviour change. PHPStan, PHPUnit, ts-check, and the Playwright suite all stay green.
rumblefrog
pushed a commit
that referenced
this pull request
May 10, 2026
Resolve ARCHITECTURE.md conflict by keeping the PR's banlist.js row update (drops the docblock claim that banlist.js owns the SteamID copy button wiring; documents that theme.js's document-level [data-copy] delegate is the single source). The contextMenoo.js row was deleted in main by #1306/#1319; the PR side hadn't applied that deletion because it was branched before the merge.
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
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 #1306.
Summary
The Servers page header (
?p=servers) used to render an admin-onlyhint that promised a right-click context menu on each expanded
server card's player rows ("Right-click a player on an expanded card
to kick, ban, or message them"). The supporting JS lived in
web/scripts/sourcebans.js'sLoadServerHost(...)helper, whichdeleted the per-row
AddContextMenu(...)call along with the restof the file at #1123 D1. The v2.0.0
page_servers.tplrewrite neverre-registered the menu — so the help text described a feature that
no longer ships, and right-clicking a player row falls through to
the browser's native context menu.
Restoring the menu would need an extra RCON
statusround-trip perserver card (the SourceQuery
GetPlayers()UDP response does notcarry SteamIDs) — doubling per-card latency and only working for
admins mapped to servers with rcon — which is out of scope for this
public surface. The kick/ban/mute UX is reachable from
?p=admin&c=kickit,?p=admin&c=blockit, and the per-rowaffordances on the public ban / comm lists, so admins are not
without a path. This PR takes the issue's "drop the help text"
option and burns the dead supporting code in the same commit so a
future contributor can't accidentally re-add the hint without the
underlying menu.
Changes
<p data-testid=\"servers-rcon-hint\">block frompage_servers.tpl; replace it with an in-template commentpointing future contributors at the missing wiring so the hint
isn't accidentally re-added without it.
IN_SERVERS_PAGEandaccess_bansfromServersView(thehint conditional was their only consumer in
page_servers.tpl)and update
page.servers.phpandpage.home.phpaccordingly.page.home.phpnow computesaccess_bansdirectly viaHasAccess(...)and passes the literalfalseforIN_SERVERS_PAGE(the dashboard always was the false case).contextMenoo.js,remove
sb.contextMenuand the globalAddContextMenushim fromsb.js, and drop the matchingSbContextMenuItem/contextMenoo/AddContextMenuambient declarations fromglobals.d.ts. None had call sites anywhere in the panel.AGENTS.mdso a futurecontributor can't reintroduce just the hint copy without the
underlying menu (and the SteamID extension to
api_servers_host_playersit would need).ARCHITECTURE.mdto drop the deletedcontextMenoo.jsrow from the
web/scripts/listing and the per-file role table.ServerListHintRegressionTest(web/tests/integration/)pinning two invariants: the rendered HTML for
?p=servers(admincopy, and
Sbpp\View\ServersView's constructor no longer acceptsthe
IN_SERVERS_PAGE/access_bansnamed params. Mirrors theAdminAdminsSearchTestharness shape (real Smarty +ob_startcapture against the production page handler).
Test plan
All five quality gates run locally (mirroring CI) — all pass:
./sbpp.sh phpstan— clean (no new errors / no baseline drift)./sbpp.sh test— 405 tests pass (ServerListHintRegressionTestgreen; the two new methods cover the rendered-HTML assertion and
the
ServersViewconstructor signature)./sbpp.sh ts-check— clean (touchedweb/scripts/sb.js+web/scripts/globals.d.ts)./sbpp.sh composer api-contract— diff stays clean (no APIhandler changes)
CI=1 ./sbpp.sh e2e— full Playwright suite (192 specs,workers: 1to match CI) passes; the hint never had its ownspec but the existing servers-page chrome specs still cover the
surface and didn't regress
Manual verification:
?p=serversasadmin/adminwith seeded servers — themisleading hint no longer appears in the header strip.
?p=homeasadmin/admin— dashboard renders unchanged(the dashboard never showed the hint anyway, since
IN_SERVERS_PAGEwas alwaysfalsethere; verified theexplicit
falseliteral preserves that behaviour).