zenmemes

New post

Step 4: Implementation Summary — Feature Flags Page Redesign

Stage 1 - Backend data model extensions

  • Changes:

    • FeatureFlagDefinition: added optional ?string $group constructor param and a groupKey(): string method (falls back to the key's prefix before the first _ when group is unset).

    • FeatureFlagRegistry: added a groupLabel(string $groupKey): string lookup (FORUM → "Forum", DEDALUS → "Dedalus agent", LLM → "LLM exchanges"; unknown keys pass through unchanged).

    • FeatureFlagRegistry: wired requiresEnabledFlag: self::DEDALUS_AGENT_REPLIES_ENABLED onto DEDALUS_AGENT_REPLIES_AUTOMATIC_ENABLED.

  • Verification:

    • php -l on both modified source files and the modified test file — no syntax errors.

    • php tests/run.php FeatureFlagEvaluatorTest — 9 run, 9 passed (7 pre-existing unchanged + 2 new).

    • php tests/run.php FeatureFlagsBehaviorTest — 2 run, 2 passed (unaffected, sanity check).

    • Added testAutomaticAgentRepliesDependsOnAgentReplies (asserts effectiveValue=false, source='dependency' when the parent is off via private config) and testGroupKeyFallsBackToPrefixAndRegistryLabelsKnownGroups.

  • Notes:

    • Deviation from the Step 3 plan: did not wire requiresEnabledFlag for LLM_CONVERSATION_UI_ENABLED → LLM_CONVERSATION_RECORDING_ENABLED. Checked the actual call sites (Application::viewerCanInspectLlmExchanges() and Application::llmExchangeStore(), both in src/ForumRewrite/Application.php) — neither checks the recording flag; both gate only on the UI flag and read the same on-disk exchange DB the recorder writes to. There is no code-enforced dependency here, unlike the Dedalus case (agentRepliesAutomaticEnabled() explicitly short-circuits on its parent). Wiring it as planned would have made the UI show a false "inactive — requires recording" state that doesn't match actual behavior. Flagged to the user; only the confirmed Dedalus dependency was implemented.

    • group is intentionally kept separate from the existing category field (per Step 2/3), which still drives FeatureFlagEvaluator's private-config gating and is untouched here.

Stage 2 - State/view-model helpers

  • Changes:

    • FeatureFlagState: added isLocked(): bool, lockReason(): ?string (branches on env / private-config / plain non-mutable default), and isBlockedByDependency(): bool.

    • FeatureFlagState: added ?bool $dependencyParentEnabled constructor param.

    • FeatureFlagEvaluator::evaluate(): now always resolves and attaches dependencyParentEnabled for any flag with a requiresEnabledFlag, not just when the child's own value is currently on — so the template can show "requires X" / dim the row even when the child is already off for its own reasons.

    • ToolsPageController::featureFlags(): passes a FeatureFlagRegistry instance to the template as registry (needed for parent/group label lookups in Stage 3); no markup changed.

  • Verification:

    • php -l on all four modified files — no syntax errors.

    • php tests/run.php FeatureFlagEvaluatorTest — 12 run, 12 passed (added testDependencyParentEnabledIsExposedEvenWhenChildIsAlreadyOff, testLockReasonBranchesBySource, testLockReasonIsNullOnSiteErrorSoTheBannerOwnsThatMessage).

    • php tests/run.php FeatureFlagsBehaviorTest LocalAppSmokeTest WebServerRoutingTest — 97 run, 92 passed; the 5 failures are pre-existing/long-standing (activity commit-manifest tests, unrelated to feature flags, tracked since 2026-09-25 per the runner's own history and todo.txt's "clean up the failing tests" item). Both feature-flags-specific smoke tests (testFeatureFlagsPageShowsSiteBackedValuesAndLayoutUsesThem, testFeatureFlagsPageReportsInvalidSiteRecordWithoutBreakingSite) pass, confirming the controller change didn't regress the current template.

  • Notes:

    • Found and fixed a real bug while writing tests: the first isLocked() draft excluded the lock badge whenever siteError !== null anywhere on the state, but FeatureFlagEvaluator attaches the evaluator-wide site error to every flag's state regardless of which branch produced its value (environment/private-config/default all carry it through). That meant one broken site record would have silently hidden the lock badge on every environment- and private-config-locked flag, not just the ones actually affected by the broken record. Fixed by keying the exclusion off source !== 'invalid-site-value' (the specific branch that broken-record fallback produces) instead of the raw siteError field. Caught by testLockReasonBranchesBySource failing before the fix.

Stage 3 - Template markup rewrite

  • Changes:

    • templates/pages/feature_flags.php rewritten from a <table> to a grouped-list layout: summary line (feature-flags-summary), one feature-flag-group section per group with a heading ("N of M on"), and one feature-flag-row div per flag containing name/badges, description, key, dependency line, and a switch/state control.

    • Preserved data-feature-flag-row, data-flag-key, data-feature-flag-form hooks unchanged; data-role="feature-flag-effective" and data-role="feature-flag-source" are still present (moved from <code>/<td> to <span> elements — the exact element tag changed, tracked for Stage 7 selector updates).

    • Overridden badge shows whenever !$flag->isDefault(); locked badge + tooltip uses $flag->isLocked()/lockReason() from Stage 2; dependency line uses $flag->isBlockedByDependency() and looks up the parent's label via the new registry template var and FeatureFlagDefinition::requiresEnabledFlag.

    • Reset-to-default is a second data-feature-flag-form form per mutable overridden flag, posting the flag's own default value — reuses the same save contract as the toggle, no new endpoint.

    • Removed the <select>/Save-button pair; the switch is now a single submit button posting the flipped value via a hidden input, so it still works with JS off.

    • Toolbar/search markup intentionally omitted (deferred feature). Page-level error banner for invalid-site-value intentionally deferred to Stage 6 per the plan (the per-row data-role="feature-flag-source" hook already reflects that source value in the interim).

  • Verification:

    • php -l templates/pages/feature_flags.php — no syntax errors.

    • php tests/run.php LocalAppSmokeTest — 93 run, 85 passed. 3 new failures, all exact-string assertions expecting <code data-role="feature-flag-source">...</code> where the markup now emits <span data-role="feature-flag-source">...</span> — expected per the Step 3 plan's flagged risk, tracked for Stage 7. The 5 pre-existing long-standing activity-manifest failures are unchanged and unrelated.

    • Manually rendered /tools/feature-flags/ via Application::handle() for three cases and inspected raw HTML: (1) an overridden+dependency-blocked flag (FORUM_EMOJI_AUTHORED_TEXT with FORUM_UNICODE_AUTHORED_TEXT off) shows the is-blocked row class, the "&#9888; inactive — requires Unicode authored text" line, and a working reset-to-default form on the overridden FORUM_APP_VERSION_NOTIFICATION row; (2) a locked private flag (DEDALUS_AGENT_REPLIES_ENABLED) shows the locked badge with "Not configurable from the site." tooltip and a disabled switch with correct aria-checked; (3) group headers show correct "N of M on" counts.

  • Notes:

    • No PHP fatals/exceptions in any rendered case — the 3 broken assertions are purely about the hook's element tag, not page correctness.

    • CSS for all new classes (.feature-flag-*, .badge, .switch, .sr-only) doesn't exist yet — the page currently renders unstyled; Stages 4–5 add it.

Stage 4 - CSS: grouped list, row, switch, mobile

  • Changes:

    • Resolved the plan's flagged risk about the asset build pipeline: there is no build step. AssetFingerprint::fingerprintedPath() (src/ForumRewrite/Host/AssetFingerprint.php) hashes the live file content on every request, so editing a CSS file directly makes it live immediately. feature_flags.php already loads a page-specific stylesheet, public/assets/tool-details.css (wired via TemplateRenderer::PAGE_STYLESHEET_PATHS), alongside the shared site.css — that's the file this stage (and Stage 5) edits. The large hashed site.f6d673d3f6f8.css file some earlier prebuilt static HTML references is a stale leftover snapshot, not the live source.

    • Added .feature-flags-summary, .feature-flag-group/-heading, .feature-flag-list/-row/-info/-name/-description/-meta/-key/-dependency/-control/-state, .switch (with [aria-checked]/:disabled/:focus-visible states), .link-button, and .sr-only to public/assets/tool-details.css. Every color/font value is a var(--token) already defined in site.css and redefined identically by all 13 public/assets/theme-*.css files — no per-theme CSS was written or is needed.

    • Mobile (@media (max-width: 520px)): switch pinned top-right via position: absolute on .feature-flag-control with padding-right reserved on the row, text flows full width below — matches the spec's explicit mobile requirement.

  • Verification:

    • Set up a local PHP built-in server (php -S with a small router calling FrontController::handle(), scratchpad-only, not committed) against a temp repository fixture with a mix of overridden/dependency-blocked/locked/enabled flags.

    • Used Selenium + Chromium (chromedriver against /usr/bin/chromium-browser; /opt/google/chrome failed chromedriver's binary check in this sandbox) to screenshot and visually inspect the real rendered page — not just reasoning about the CSS text.

    • Verified at 1150px in light, dark, and word97 (a structurally different, boxy retro theme) — layout, grouping, switches, and dependency text all render correctly with zero per-theme overrides, confirming the token-only approach works. Also checked at 375px.

    • No mid-word wrapping of any key or name at either width, in any theme checked.

  • Notes:

    • Found and fixed a real bug via screenshot: the switch's off-state thumb used --ink-soft (a blue accent color), which visually read as "on" even for disabled flags — a genuine legibility problem invisible from reading the CSS alone. Changed to --line (the muted/neutral border token) so only the actual on-state uses color (green).

    • Found and fixed a real spec deviation via screenshot: my first mobile layout put the switch below the text (column stacking) instead of "top-right of the row, text flows full width below" as specified. Fixed with position: absolute placement; re-verified by screenshot.

    • Investigated an apparent 375px horizontal-overflow/clipping artifact seen when using chromium/google-chrome --headless=new --screenshot=... directly from the CLI; it reproduced identically on the untouched /tools/ page, but turned out to be a limitation of that specific screenshot invocation, not a real site bug — Selenium's set_window_size against the same URL shows correct wrapping with no overflow. The nav item "Feature Flags" wrapping alone onto its own row at narrow widths is real and matches the Step 3 plan's Stage 5 "nav wrap fix" item, still pending.

    • Badge text ("overridden", "locked") is currently unstyled plain text next to the flag name, which can wrap awkwardly (e.g. "Automatic agent replies locked" wraps to two lines at 375px) — expected, Stage 5 adds the pill-shaped .badge styling.

Stage 5 - CSS: badges, error banner, nav wrap fix

  • Changes:

    • Added .badge (base pill shape) plus .badge-overridden (amber, --status-warning) and .badge-locked (neutral, --line/--ink-soft) to public/assets/tool-details.css, all token-based.

    • Error banner needs no new CSS: .feedback/.feedback-error already exist in site.css (padding, border, --feedback-error-bg background) and are reused as-is when Stage 6 adds the actual banner markup to the template.

    • Nav wrap fix: added .board-controls-nav > *:last-child { margin-left: 0; } to tool-details.css, overriding site.css's margin-left: auto on the last sub-nav item. Scoped deliberately to tool-details.css (only loaded by feature_flags.php, codebase_state.php, llm_exchanges.php) rather than editing the shared rule in site.css directly, because board.php also uses .board-controls-nav and relies on that same rule to right-align its sort options from its view options — a real, intentional use elsewhere that a global edit would have broken.

  • Verification:

    • Re-screenshotted via the same Selenium+Chromium setup at 1150px and 375px, light and dark themes.

    • Badges render legibly (text + color, not color alone) in both themes; "OVERRIDDEN" and "LOCKED" pills are clearly distinct from the on/off switch state.

    • Nav fix confirmed: "Feature Flags" now wraps onto its own row left-aligned with the other sub-nav items, instead of being pushed to the right edge and looking detached (previous behavior, caused by site.css's :last-child right-alignment, verified in both light and dark screenshots).

    • Did not re-check board.php's own layout since tool-details.css isn't loaded there — the override can't reach it by construction (selector scoped by stylesheet, not by additional class), so there's nothing to regress.

  • Notes: none beyond what's captured above.

Stage 6 - Save-flow JS rewrite + error banner wiring

  • Changes:

    • public/assets/feature_flags.js rewritten: submit handler now updates aria-checked on the switch, the .feature-flag-state text/is-on class, the toggle form's hidden value input (flipped to the new opposite, so a second click without reload still posts the correct value), the data-role="feature-flag-source" text, and shows/hides the "overridden" badge (creating it if it doesn't exist yet, since a default-valued flag can become overridden without a reload) and the reset-to-default form (hiding it if the new value now matches default — showing one that didn't exist yet is not handled, see Notes). Status feedback (Saving…/Saved./error) now falls back to the row's shared status element so the reset form's submit also gets feedback, not just the toggle form's.

    • templates/pages/feature_flags.php: added data-feature-flag-toggle on the toggle form (distinguishes it from the reset form for the JS) and data-flag-default="true|false" on the row (lets the JS know when the new value is the default without re-deriving it). Added the page-level error banner (<div class="feedback feedback-error">) rendered from $flags[0]->siteError when the site's canonical flags record fails to parse — reuses the existing site-wide .feedback/.feedback-error component from site.css, no new CSS.

    • public/assets/tool-details.css: added .badge[hidden] { display: none; } — caught before it became a bug (see Notes).

  • Verification:

    • php -l/node --check on the modified template and JS — no syntax errors. php tests/run.php FeatureFlagEvaluatorTest FeatureFlagsBehaviorTest LocalAppSmokeTest WebServerRoutingTest — 109 run, 101 passed; failures are the same 3 element-tag mismatches from Stage 3 (tracked for Stage 7) plus the same 5 pre-existing/unrelated activity-manifest failures. No new failures.

    • Live end-to-end click-through via Selenium against the running dev server: clicking a switch without a root-approved identity correctly hit the real /api/set_feature_flag 403 guard and surfaced the server's exact error text in the status line — confirms the fetch/error path is wired correctly end-to-end.

    • Isolated DOM-update test: served a minimal static fixture using the actual, unmodified public/assets/feature_flags.js (not a copy/paraphrase), with window.fetch mocked in the browser to return a canned successful response — this exercises the real production code's updateRow logic without needing a full signed/approved identity fixture (disproportionate effort for testing a pure client-side DOM update). Confirmed both directions: toggling a default-off flag to on creates the overridden badge, sets aria-checked=true, flips the hidden value input to false, and updates state/source text; clicking an existing reset-to-default form hides both the badge and the reset form itself.

    • Simulated a broken site record (FORUM_APP_VERSION_NOTIFICATION: nope) against the live dev server and confirmed the error banner renders with the exact parse-failure message, reusing the standard .feedback.feedback-error look; confirmed via screenshot. Restored the fixture afterward.

  • Notes:

    • Found and fixed a real bug before it shipped: .badge already declares display: inline-block as an author-origin rule, which overrides the browser's default [hidden] { display: none } UA-origin rule regardless of specificity (author origin always wins over user-agent origin for the same property) — so badge.hidden = true alone would silently not hide the badge. Added an explicit .badge[hidden] { display: none; } override and confirmed via element.is_displayed() in the Selenium test that it now actually hides.

    • Known simplification: hiding an existing reset-to-default form when a flag returns to default is handled (it already exists in the DOM); creating a reset form when a default-valued flag becomes overridden via the switch is not — only the badge is created dynamically. This is acceptable because these are boolean flags: when overridden, clicking the switch again always reaches default (there are only two possible values), so the switch itself is always a fully functional substitute for "reset to default" even without the explicit link appearing until the next full page load.

Stage 7 - Tests + final manual pass

  • Changes:

    • Fixed the 3 selector breaks flagged since Stage 3 in tests/LocalAppSmokeTest.php (testApplicationRendersCoreRoutes, testFeatureFlagsPageShowsSiteBackedValuesAndLayoutUsesThem, testFeatureFlagsPageReportsInvalidSiteRecordWithoutBreakingSite): updated data-role="feature-flag-source">…</code> assertions to </span>, replaced the removed "current value is/differs from default" meta-text assertions with assertions on the new markup (role="switch", badge-overridden, feedback feedback-error).

    • Found and fixed a 4th break the Step 3 plan's canonical-files list didn't predict: tests/WriteApiSmokeTest.php::testFeatureFlagFormSubmitRequiresRootApprovedIdentityAndRedirectsAfterCommit — a pre-existing, more thorough no-JS round-trip test (GET form → unauthorized POST → root-approved POST → redirect → re-render) that also asserted the old </code> tag. Only found by running the entire suite instead of the subset of files the plan predicted; fixed the same way.

    • Added LocalAppSmokeTest::testFeatureFlagsPageShowsLockedBadgeWithReasonForNonMutableFlags (asserts badge-locked and the exact lock-reason tooltip text) and testFeatureFlagsPageDimsAndWarnsOnDependencyBlockedFlag (asserts feature-flag-row is-blocked and the "inactive — requires …" text) — new coverage for the badge/dependency rendering the plan called for.

    • Did not add a separate new no-JS-POST-path test: testFeatureFlagFormSubmitRequiresRootApprovedIdentityAndRedirectsAfterCommit (existing, now fixed) already covers exactly that path end-to-end.

  • Verification:

    • Full suite: php tests/run.php — 564 run, 558 passed. The 6 remaining failures are pre-existing and unrelated (activity-manifest tests failing since 2026-09-25, a Node-helper compose-signing test failing since 2026-09-25 — both tracked separately, predating this feature). Confirmed one additional failure (WriteApiSmokeTest::testIncrementalApprovalMatchesFreshRebuildForTransitiveApprovalAndScoreRefresh, an approval/read-model parity test with no relation to feature flags) was flaky, not a regression — it failed once and passed twice on immediate reruns with no code changes in between.

    • Manual 50-flag scannability check: rendered the real, unmodified templates/pages/feature_flags.php directly against 52 synthetic FeatureFlagState objects across 5 groups (including one group key with no display-name mapping, to confirm the fallback-to-raw-key behavior) — not committed to the actual registry. Screenshotted via Selenium: grouping, summary counts, badges (including a dynamically-unmapped group label), switches, and the reset link all render correctly at scale; the page is a long scroll without the deferred search/filter, as expected and already called out in Step 1/2.

    • Stopped the scratch PHP dev servers used for manual testing throughout Stages 4–7.

  • Notes:

    • The Step 3 plan's predicted canonical files (FeatureFlagsBehaviorTest.php, FeatureFlagEvaluatorTest.php, WebServerRoutingTest.php) didn't fully match reality: the actual breakage was in LocalAppSmokeTest.php and WriteApiSmokeTest.php. FeatureFlagEvaluatorTest.php additions happened in Stages 1–2 instead. Worth remembering for future FDP plans: predict test-file impact by grepping for the markup/selector being changed, not by class-name association.