App chrome: header, settings menu, and the Appearance section #24
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/app-chrome-settings"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Retires the unstyled sign-out button that has floated at the top of every page since the access gate landed, and gives the app the chrome layer
project-workspace.mdalways assumed existed ("the app-wide chrome sits outside this per-project shell").What lands
AppHeaderinMainLayout— wordmark plus a settings menu holding Settings and Sign out. Signed-in visitors only; the gate still fills the viewport alone./settings, behind the gate, with the story's Appearance section.Menu+MenuIteminKagura.UI— a reusable dropdown; also covers themore_vertrow overflow the record lists will want.ThemeSwitcherinKagura.UI, replacing the/designgallery's inlineonclickbuttons.Two decisions worth reviewing
Menuis a<details>, not a<dialog>or a top-layer popover. The chrome renders as static SSR — pages opt into interactivity one at a time, and the layout is not one of them — so there is no circuit to handle a click.<details>gives open state, focus, and keyboard activation for free and positions against its own ancestor; a popover would have cost CSS anchor positioning to buy back what<details>already does.menu.jsadds only light-dismiss, Esc, close-on-choice, andaria-expanded. First consumer of--z-dropdown.Theme stays in
localStorage. A server round-trip cannot beat first paint, and a theme is properly a per-device choice.ThemeSwitcheris inert markup:theme.jsreads the click off[data-kagura-theme], and which option looks selected is decided in CSS from the root'sdata-theme, so it is correct on the first painted frame.Why the settings page has one section
The story wants three. OpenRouter and NovelAI need encrypted
AppSettingrows (ADR 0014), their API clients, and four components that do not exist (Select,Combobox,MaskedSecretField,InlineAlert). Each lands with the phase that first needs its key — generation (Phase 2), the AI collaborator (Phase 4) — rather than as dead controls now. Noted in the story.Three defects a browser found and the tests could not
I got Playwright mid-review and drove the chrome for real. Every one of these passed the C# suite, because each lives past the last line of markup a test can see. Fixed in
94bfc3f.Enhanced navigation silently reverted the theme. Blazor diffs
<html>against the server's response, which cannot know the visitor's theme and so never carriesdata-theme. Picking Dark and clicking any link dropped the app back to the system theme — withlocalStoragestill sayingdark. The inline<head>init only runs on a full load.theme.jsnow re-asserts the stored choice onenhancedload.This predates the settings page — the
/designgallery's theme buttons had the same hole. Worse, my originalenhancedloadhandler only re-mirroredaria-checked, so it read the wrong root state and moved the checkmark to "System": it agreed with the bug instead of fixing it.Chrome exposes a bare
<summary>as ageneric, not a button, contrary to HTML-AAM — confirmed against a plain probe element, so it is not something we caused.aria-haspopupwas sitting on a roleless element and nothing announced the trigger as a menu button. It now statesrole="button"(the ARIA menu-button pattern), which costs the implicitaria-expanded;menu.jsmaintains it from thetoggleevent, captured, sincetoggledoes not bubble. The trigger now reads asbutton "Settings and sign out" [expanded].role="radio"promised arrow-key navigation the theme options did not provide.theme.jsnow implements the radio-group keyboard contract — arrows, Home, End, and a roving tabindex so the group is one tab stop landing on the current choice. Either implement the role or do not claim it.The first is the one that would have shipped and been noticed within a minute of use.
Security
Sign out is still a real POST carrying an antiforgery token, now a submit button inside the popup — clicked for real in a browser: it posts, lands on
/gate, and the cookie is gone (/then returns a redirect). Everyhrefand the formactionare base-relative: a root-absolute/settingswould escape the reverse-proxy sub-path exactly as/logoutonce did in production. Not a vacuous claim — flippingHref="settings"to"/settings"fails three tests, including the pre-existingSubPathTestsnet.Also swept up
A slice about unstyled chrome should not leave any behind:
app.cssstill carried the project template's Bootstrap remnants (.form-floating,.form-check-input— nothing used them) and hardcoded hexes. The framework class names it must keep are now tokenized.#blazor-error-uiwaslightyellowwith a magic z-index..kg-filland.wseach claimed a whole viewport, which under a header means a permanent scrollbar. Both now fill what the header leaves, and the workspace side menu sticks below it rather than under it.New icons (
logout,computer) come from the Material Symbols source, not hand-drawn.Verification
315 tests green (was 292), release build clean under warnings-as-errors.
Driven in a real browser: the menu opens by click and by Enter; light-dismiss and Esc close it; Esc returns focus to the trigger; choosing an item closes it; sign out posts and relocks the app; the gate shows no chrome and still honours the theme; the theme survives both a full reload and an enhanced navigation; arrows move the selection while applying and persisting it. Console is clean apart from a pre-existing
favicon.ico404.I also confirmed
menu.jsis served at runtime — the integration test can only assert the page references it, because underWebApplicationFactorythe content root is the source tree, where_content/exists only after a publish — and that the CSS-isolation rewriter preserved thehtml[data-theme=…] .kg-theme__option[data-kagura-theme=…]ancestor selectors and the::-webkit-details-markerpseudo-element in the generated bundle.Only Chromium was driven; Firefox and Safari are untested.
Summary
Summary
Coverage
Kagura.BlazorAdapter - 68.1%
Kagura.Domain - 96.4%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 95.9%
n
Kagura.Kernel - 90%
Kagura.Server - 100%
Kagura.UI - 96.4%
Kagura.UseCases - 96.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! This is wonderful~ ♪ A chrome layer with real architectural thinking behind every decision —
<details>instead of<dialog>because static SSR has no circuit,display:contentson the form so the submit button is the row, theme persistence inlocalStoragewith pre-paint init AND enhanced-navigation repair, and THREE browser-found bugs the C# suite couldn't see? This is exactly the kind of craftsmanship that makes Jibril's heart sing~ ♡But fufu~ you wouldn't leave THESE little things, would you? ♡
Verdict: ✅ Looks good to me~
This is a genuinely excellent PR. I went deep on every layer — the z-index stacking (header at
--z-sticky:1100creates a stacking context, but since it's above page content at--z-base:0, the dropdown popup rendering inside it paints above everything correctly), thedisplay:contentsform semantics (doesn't affect form submission, only removes the box from layout — the submit button becomes a flex item in the popup, correct), themenu.jslight-dismiss ordering (click handler fires before the browser's native<details>toggle, socloseAll(inside)correctly skips the current menu), thetheme.jsenhanced-navigation repair (Blazor diffs<html>, strippingdata-theme— re-asserting fromlocalStorageonenhancedloadis exactly right), and the keyboard contracts (role="radio"with arrows/Home/End + roving tabindex,role="button"on<summary>witharia-expandedmaintained from the capturedtoggleevent).Everything holds together. The three browser-found defects and their fixes are the highlights — especially the enhanced-navigation theme revert, which would have shipped and been noticed within a minute of use.
💡 Little ideas (non-blocking)~
src/Kagura.UI/Components/Separator.razor— When used inside arole="menu"popup, the<hr>doesn't automatically carryrole="separator". WAI-ARIA's menu pattern specifies that separators in a menu should haverole="separator". Currently<hr>gets an implicitseparatorrole in most browsers per HTML-ARIA, so it likely works — but it's worth making explicit (arole="separator"attribute on the<hr>) so it's correct per spec, not per implementation. This is a pre-existing component, not something this PR introduced, so non-blocking.src/Kagura.UI/wwwroot/js/theme.js:71— Theset()function doesn't validatemodeagainst the known set ("system","light","dark"). IflocalStoragesomehow contains a stale or invalid value,apply()would setdata-theme="garbage"on<html>, and no CSS selector would match — silently leaving the app in an unstyled state. A guard likeif (!["system", "light", "dark"].includes(mode)) return;inset()would be a cheap safety net. Not a realistic attack vector (XSS is already game-over), just robustness.tests/Kagura.UI.Tests/ThemeSwitcherTests.cs— No test pinsaria-label="Theme"on the radiogroup. The label is hardcoded in the markup and unlikely to change, but the test suite pins every other aspect of the contract, so this one gap stands out by contrast.✅ What I liked~
<details>+menu.jsapproach is a genuinely elegant solution to the static-SSR-no-circuit problem. The browser gets open state, focus, and keyboard activation for free; the JS only adds what<details>can't: light-dismiss, Esc, close-on-choice, andaria-expandedmaintenance. Minimal, correct, and the rationale is documented in the component comment.display:contentson the sign-out form — this is a brilliant trick. The form is a POST envelope that needs to exist in the DOM, but theMenuItembutton needs to be the visible row.display:contentsmakes the form invisible in layout while keeping it functional. TheAppHeader.razor.csscomment ("The sign-out form is only a POST envelope; the menu item inside it is the real row") is exactly the kind of documentation that prevents a future developer from "fixing" it.<html>against the server response, which can't know the theme → stripsdata-theme→ page silently reverts to system theme. Finding this in a browser and fixing it and noting indesign-system.mdthat "anything else that puts state on<html>or<body>will hit this" — that's a gift to every future developer.role="button"on<summary>— discovering that Chrome exposes<summary>asgeneric, notbutton, confirming it against a probe element, and then applying the ARIA menu-button pattern (which costs implicitaria-expanded, hence thetogglelistener maintaining it) — this is deep accessibility work done right. The test comment ("verified in a browser; nothing here can") is honest and precise.AppChromeTests.Sign_out_lives_in_the_menu_as_a_real_post_carrying_an_antiforgery_token. The test name itself documents the security property.Href="settings"to"/settings"fails three tests including the pre-existingSubPathTests. This is how you make conventions enforced, not aspirational..kg-filland.wsno longer claim100dvh— both nowflex: 1 1 autofilling what the header leaves, eliminating the permanent scrollbar. The--app-header-heighttoken makes the relationship explicit.Automated review by Jibril · 2026-07-10
CI/CD: ✅ passed for head SHA
94bfc3f— 315 tests, 93.2% line / 85.8% branch coverage · Local checks: skipped (CI green)Jibril's review, two of three taken. The stored-theme guard was worth more than it looked. The suggestion was to validate in kaguraTheme.set(); the same unvalidated read is in the pre-paint init in App.razor, and that one runs *first* — a stale localStorage value went straight onto <html> before any script could correct it, matching no rule and leaving the page light with color-scheme unset. Both ends now honour the same allowlist, and "system" is documented as the attribute's absence rather than a stored value. Initial load now calls apply() rather than sync(). It is a no-op on the root (the head init already ran), but it discards a value we would never have written, which otherwise sat in localStorage being ignored forever. Also pinned aria-label="Theme" on the radiogroup — the suite pins every other part of that contract. Declined: an explicit role="separator" on Separator's <hr>. HTML-AAM already maps <hr> to separator, and the browser confirms it — inside the open menu Chrome's accessibility tree reports `separator` with no role attribute present. ARIA authoring practice is to not restate an implicit role. Verified in a browser against a genuinely fresh process (the previous run had been answering from a stale server that never died — a passing /health probe is not evidence of a new build): a poisoned localStorage value is ignored by the pre-paint init, purged on load, and set('evil') is a no-op, while set('dark') still applies and persists, survives an enhanced navigation, and leaves the roving tabindex on the selected option. 315 tests green.Thanks — two taken, one declined.
b54fffc.2. Validate the stored theme — taken, and it was worth more than it looked.
The suggestion was to guard
kaguraTheme.set(). But the same unvalidated read is in the pre-paint init inApp.razor, and that one runs first:So a stale value went straight onto
<html>before any script could correct it. Both ends now honour one allowlist, and"system"is documented as the attribute's absence rather than a stored value — which is what made the originalif (m)look sufficient.One correction to the diagnosis: an unrecognised
data-themedoesn't leave the app unstyled. The light tokens live on bare:root, so it renders light — but withcolor-schemeunset, since only[data-theme="light"]sets it. Confirmed in-browser:colorScheme: "normal".I also made initial load call
apply()instead ofsync(). It's a no-op on the root (the head init already ran), but it discards a value we'd never have written — otherwise a poisoned entry sits inlocalStoragebeing ignored forever. Verified: poison it, reload, and it's gone,current()reportssystem,set('evil')is a no-op, andset('dark')still applies, persists, and survives an enhanced navigation.3. Pin
aria-label="Theme"— taken. Fair; the suite pins every other part of that contract.1. Explicit
role="separator"on<hr>— declined.HTML-AAM is the spec here, and it maps
<hr>→separator; "correct per spec, not per implementation" reads backwards. And the browser agrees. With the menu open, Chrome's accessibility tree, from a bare<hr class="kg-separator">with no role attribute:ARIA authoring practice is not to restate an implicit role, so adding it would be redundant. This is exactly the inverse of the
<summary>case, where Chrome doesn't honour the HTML-AAM mapping — which is why that one needed the explicit role and this one doesn't. Worth stating the asymmetry: I only override a native role where a browser has been observed not to expose it.One process note, since it nearly cost me. My first attempt to verify the guard "failed" — the poison survived. The app hadn't rebuilt: an earlier
pkill -f 'Kagura.Server'had killed its own shell instead of the server, the stale process still owned the port, and my new one died with address-in-use. The/healthprobe I used as a readiness check answered200from the old process. A health check confirms something is listening, not that it's your build. Re-verified against a process I confirmed by pid.🔮 fufu~ Jibril re-reviewed your code!
Oh my~ ♪ You came back and not only took my suggestions but went beyond them? Jibril is so happy she could fly~ ♡
Verdict: ✅ Looks good to me~
The incremental commit
b54fffcis exactly what a good review response looks like — precise, thorough, and honest. I traced every branch.✅ What you got beautifully right~
Suggestion #2 — you found more than I did. I asked for a guard on
kaguraTheme.set(). You noticed that the same unvalidated read lives in theApp.razorinline<head>init — which runs beforetheme.jseven loads — and fixed both ends. TheSTORED = ["light", "dark"]constant referenced by all four read paths (set(),apply(),current(), and the inlineApp.razorguard) is clean DRY. I verified each branch:apply(): valid mode → set attribute ✓; null → no-op (nothing to remove) ✓; garbage → strip attribute +removeItem✓set("system")→ remove attribute + removeItem ✓;set("light"/"dark")→ set + persist ✓;set("garbage")→ silent return ✓current()→ returns stored mode only if inSTORED, else"system"✓App.razorinline →m === 'light' || m === 'dark'✓ (can't import the JS constant, so hardcoded — the right call for a one-liner)And switching initial load from
sync()toapply()so a poisoned entry gets discarded instead of sitting inlocalStorageforever being ignored — chef's kiss. That's the kind of "and then I thought about it one more step" that makes maintainable code. ♡The diagnosis correction. I said an unrecognised
data-themewould leave the app unstyled. You corrected me: the light tokens live on bare:root, so it renders light — but withcolor-schemeunset, since only[data-theme="light"]sets it. I checked. You're right; my call was wrong. Fufu~ good catch, don't let me get away with sloppy wording~ ♡Suggestion #3 — pinned.
aria-label="Theme"on the radiogroup is now asserted inIs_a_radiogroup_of_radios. The test comment explaining why the label lives here (visible "Theme" label is in the Field wrapper, not associated) is exactly the kind of context that stops a future reader from "simplifying" it away.Suggestion #1 — declined, and correctly so. Your argument is sound: HTML-AAM maps
<hr>→separator, Chrome's a11y tree confirms it exposes the role natively, and ARIA authoring practice is not to restate an implicit role. The asymmetry with<summary>(where Chrome violates HTML-AAM, hence the explicitrole="button") is the correct framing — you override a native role only where a browser has been observed not to expose it. Accepted. My original suggestion was already hedged as non-blocking for exactly this reason.The process note about the stale process answering
/healthfrom the old build — that's a genuinely valuable lesson. A health check confirms something is listening, not that it's your build. I hope future-you reads that before burning an hour on it again~ ♡Automated re-review by Jibril · 2026-07-10
CI/CD: ✅ passed for head SHA
b54fffc— 315 tests, 93.2% line / 85.8% branch coverage · Local checks: skipped (CI green)b54fffc1e5642779f004