webui: hide disabled feature pages from the admin nav (closes #437)#453
Merged
Conversation
The sidebar listed every page unconditionally, so Notices, Polls, Reaction Roles, Announcements and Voice Channels showed up even when their feature flag was off — an operator would click through to an inert page for a feature they hadn't enabled. admin-layout.ts: - NavItem gains an optional `featureKey` — the `<feature>.enabled` config key that gates the page. Dashboard, Settings, Permissions, Setup Wizard, Database and Bootstrap have none and are always shown. - NAV_ITEMS annotates the five gated entries (announcements, polls, reactionroles, notices, voicechannels). - renderNav filters out items whose gating feature resolves to false. An unknown featureKey (absent from the status map) is treated as enabled — a wiring gap fails open rather than blanking the nav. - AdminPageOptions gains an optional navFeatureStatus map; when omitted the nav shows everything (keeps existing direct callers and tests working). - New resolveNavFeatureStatus helper builds the status map from an injected async boolean reader, so admin-layout stays free of a direct ConfigService dependency and remains unit-testable. admin-views.ts: - CommonProps carries navFeatureStatus; all 14 renderAdminPage calls thread it through. read-only-routes.ts: - commonFromReq is now async and resolves navFeatureStatus via ConfigService.getBoolean. All 10 call sites updated to await it. write-routes.ts: - The four write-router page renders (import preview, wizard start/step/confirm) resolve navFeatureStatus too, via a small navStatusForPage helper, so their sidebar matches the read-only pages. Scope note: feature pages stay reachable by direct URL — they already render a sensible read-only/disabled view. #437 only suppresses the nav advertisement, which is the lighter of the two options the issue left open. Tests: 5 new cases in admin-layout.test.ts covering nav filtering (omitted map shows all, disabled hidden / enabled kept / ungated always shown, missing-key fails open) and resolveNavFeatureStatus querying exactly the gated keys.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the WebUI admin layout so the sidebar navigation only advertises pages for features that are currently enabled, while still allowing direct URL access to disabled feature pages (per the scope decision in #437).
Changes:
- Added optional feature gating metadata to admin nav items and filtered the rendered nav based on a per-request feature-status map.
- Threaded
navFeatureStatusthrough admin view renderers so every page can render a consistent sidebar. - Resolved
navFeatureStatusin both read-only and write routes viaConfigService.getBoolean, and added unit tests around nav filtering/status resolution.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/web/admin-layout.ts |
Adds featureKey to nav items, introduces resolveNavFeatureStatus, and filters sidebar items based on navFeatureStatus. |
src/web/admin-views.ts |
Extends common page props with optional navFeatureStatus and passes it through to renderAdminPage. |
src/web/read-only-routes.ts |
Makes commonFromReq async to compute navFeatureStatus and updates all call sites to await it. |
src/web/write-routes.ts |
Adds a helper to compute navFeatureStatus for write-rendered pages (wizard/import) to keep sidebar consistent. |
__tests__/web/admin-layout.test.ts |
Adds coverage for nav filtering behavior and resolveNavFeatureStatus key resolution. |
| * are always shown. The page URL itself stays reachable by direct | ||
| * navigation; only the nav advertisement is suppressed. | ||
| */ | ||
| featureKey?: string; |
Comment on lines
+112
to
+115
| * omitted, every nav item is shown (keeps direct callers and tests that | ||
| * don't care about nav filtering working unchanged). | ||
| */ | ||
| navFeatureStatus?: Record<string, boolean>; |
Comment on lines
+95
to
+101
| const status: Record<string, boolean> = {}; | ||
| for (const item of NAV_ITEMS) { | ||
| if (item.featureKey) { | ||
| status[item.featureKey] = await isEnabled(item.featureKey); | ||
| } | ||
| } | ||
| return status; |
Comment on lines
+116
to
125
| const config = ConfigService.getInstance(); | ||
| const navFeatureStatus = await resolveNavFeatureStatus((key) => | ||
| config.getBoolean(key, false), | ||
| ); | ||
| return { | ||
| guildId: session.guildId, | ||
| csrfToken: getCsrfToken(req), | ||
| remainingMs: getDisplayedRemainingMs(session), | ||
| navFeatureStatus, | ||
| }; |
Owner
Author
There was a problem hiding this comment.
Skipping this one deliberately. The duplicate getBoolean is a ConfigService in-memory cache hit on a request that already does Discord API round-trips and Mongo queries — the cost is noise. Against that, reusing navFeatureStatus for page-level enabled flags would couple ~5 page handlers to a map that exists specifically for sidebar filtering: a future change to which nav items are feature-gated (or to the NavFeatureStatus shape) would silently alter page-render logic in unrelated handlers. The two reads are intentionally independent concerns that happen to share a key.
The "drift if defaults change" point doesn't apply here either — both call sites pass the same explicit false default, matching defaultConfig. Addressed the other three comments in 85fc420.
Generated by Claude Code
Three type-safety / efficiency improvements from review: 1. featureKey is now typed `keyof ConfigSchema` instead of plain `string`, so a typo in NAV_ITEMS fails at compile time rather than silently never matching at runtime. 2. New exported `NavFeatureStatus` type — `Partial<Record<keyof ConfigSchema, boolean>>`. The map intentionally allows missing keys (renderNav fails open on an absent key); the previous `Record<string, boolean>` typed every index access as a non-optional boolean, which didn't model that. Threaded through AdminPageOptions, CommonProps, commonFromReq, and navStatusForPage. 3. resolveNavFeatureStatus now resolves feature keys in parallel via Promise.all and de-duplicates them first, so two nav items sharing a key (none today, but defensively) trigger a single read. Not changed: the reviewer also suggested route handlers reuse common.navFeatureStatus for their page-level `enabled` flags instead of re-reading the *.enabled key. Skipped — see PR reply. The duplicate read is a ConfigService cache hit, and reusing the nav map would couple five page handlers to a structure built for sidebar filtering.
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.
Closes #437.
Problem
The admin sidebar (
NAV_ITEMS/renderNavinsrc/web/admin-layout.ts) listed every page unconditionally. So Notices, Polls, Reaction Roles, Announcements and Voice Channels appeared even with their feature flag off — an operator would click through to an inert page for a feature they hadn't enabled.Changes
src/web/admin-layout.tsNavItemgains an optionalfeatureKey— the<feature>.enabledconfig key gating the page. Dashboard, Settings, Permissions, Setup Wizard, Database and Bootstrap have none and are always shown.NAV_ITEMSannotates the five gated entries.renderNavfilters out items whose gating feature resolves tofalse. An unknownfeatureKey(absent from the status map) is treated as enabled — a wiring gap fails open rather than blanking the nav.AdminPageOptionsgains an optionalnavFeatureStatusmap; when omitted the nav shows everything (keeps existing direct callers + tests working).resolveNavFeatureStatushelper builds the status map from an injected async boolean reader, soadmin-layoutstays free of a directConfigServicedependency and remains unit-testable.src/web/admin-views.tsCommonPropscarriesnavFeatureStatus; all 14renderAdminPagecalls thread it through.src/web/read-only-routes.tscommonFromReqis now async and resolvesnavFeatureStatusviaConfigService.getBoolean. All 10 call sites updated toawait.src/web/write-routes.tsnavFeatureStatustoo, via a smallnavStatusForPagehelper, so their sidebar matches the read-only pages.Scope decision
The issue left the direct-navigation behaviour open ("the existing page or a 'feature disabled' notice"). Feature pages stay reachable by direct URL — they already render a sensible read-only/disabled view, so no per-route guard or redirect is added. #437 only suppresses the nav advertisement, which is the lighter, lower-risk of the two options.
Verification
npm test— 744 passed, 1 skipped, 0 failed (5 new tests).npx tsc --noEmit— clean.npm run lint— 0 errors.npm run format:check— clean.notices.enabledin Settings → Notices disappears from the sidebar on the next page load; re-enable → it returns./admin/noticestyped directly still renders.Test plan
resolveNavFeatureStatusqueries exactly the gated keys.Generated by Claude Code