feat(nav): NavItem grows a real tree — Child branches, any depth, trail reveal; Nested removed; 11 run-and-graph icons #22
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/nav-tree"
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?
Two commits, one theme: the nav stack learns a real tree, and the icon catalog gains the run-and-graph set that Tsumugi Phase 2–5 will consume.
What
1.
NavItemgrows a real tree (breaking).Child: the plain leaf anchor, markup unchanged — every existing consumer (Kagura'sWorkspaceShell) renders identically.Child: a foldable branch — the anchor in a row with a native<button aria-expanded>toggle (chevron rotated via CSS), the fragment in arole="group"below, recursing to any depth.kg-nav__branch--trail— containment, not openness; the accent survives a manual collapse, and a writer collapse wins for the circuit's life.PersistKey(opt-in): explicit toggles stored in localStorage via newnav.js(kgNav.get/set, best-effort, guarded) — a sidebar that remounts on every navigation keeps its shape.Nestedremoved. Exactly one consumer existed (TsumugiStoryShell— migrating in the paired Tsumugi PR, next); Kagura uses NavList/NavItem/NavGroup withoutNested(grep at691a176), Orihon uses no nav components (grep at31bf6f3). Zero coordination needed.2. Eleven icons, canonical Material Symbols paths:
stop(Cancel run),schedule(queued runs),progress_activity(streaming node),payments(Cost cells),bolt(graph triggers),output(output node —inputexists, its counterpart didn't),construction(tool node),grid_view(map fan-out),task_alt(approve card),help(waiting-for-input),more_horiz(scene-break***).Tests
NavItemTests: 16 facts — leaf/branch polymorphism, toggle aria flips,Expandedinitial, 3-level recursion,PersistKeyJS contract (kgNav.seton toggle, zero JS without), trail class, auto-reveal, manual-collapse-beats-auto-expand (incl. re-render).NavStructureTests: a composed 3-levelNavListin document order.Notes
nav.jsridesKagakuScripts(the disk↔shipped parity test enforces it).:has()-based styling idea from planning was replaced by a C# URL-containment check (IsInside) — works identically in bUnit and browser, no CSS-support concerns.Summary
Summary
Coverage
Kagaku.UI - 95.3%
Kagaku.UI.Fluxor - 97.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A nav that grows a real tree~ Recursion, an honest toggle that only folds while the anchor does the navigating, a writer collapse that beats the auto-reveal — you even dodged the
:has()trap by keeping the trail rule in C# where bUnit can see it. I read every line of the +490 twice, cloned Kagura, Orihon, AND Tsumugi to check your breaking-removal claims, and ran the stylesheets through a headless browser because I don't trust cascades I haven't computed. ♡Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Kagaku.UI/Components/NavItem.razor.css:39-49— an ACTIVE branch anchor loses its accent text. The branch-link rule (.kg-nav__item--branch .kg-nav__link { color: inherit; … }, specificity 0,2,0) and the active rule (.kg-nav__item--active { color: var(--accent); … }, specificity 0,2,0) tie — and the branch rule comes later in source, so it wins.inheritresolves against the branch row div, which only ever matches base.kg-nav__item→--text-muted. Mechanically verified in headless Chromium with your actual stylesheet: a branch anchor withkg-nav__item--activecomputescolor: rgb(119,119,119)(muted) onbackground: rgb(232,222,248)(accent-subtle) — while a leaf with the same classes computesrgb(103,80,164)(accent). And it splits by one segment of depth: standing deeper,--trail's 0,3,0 rule rescues the accent; standing ON the section's own page (IsInsidefalse, no trail) you get a subtle background with gray text — the "you are here" state half-broken. Your own comment says the anchor "keeps the leaf look," and this isn't it~Fix: add
.kg-nav__item--branch .kg-nav__link.kg-nav__item--active { color: var(--accent); }(0,3,0, after the trail rules), or move the active block below the branch-link rules.src/Kagaku.UI/Components/NavItem.razor:105-117—TryJsAsyncdoesn't catchJSDisconnectedException, breaking the repo's guard pattern. Nine siblings (Hotkey, Modal, ContextMenu, QuicklinkNav, MaskCanvas, RegionSelector, DocumentTitle, Lightbox, LiveValue) all catchJSDisconnectedExceptionfirst — Hotkey's comment even names the scenario ("Circuit already gone"). Your guard catches onlyJSExceptionandInvalidOperationException. I checked the hierarchy mechanically:JSDisconnectedException's base type isSystem.Exception, notJSException— so it escape — so akgNav.setin flight when the tab closes, or the first-renderkgNav.getracing a disconnect, throws an unhandled exception instead of the "tree simply stays ephemeral" your own comment promises. Fufu~ you wouldn't leave THAT in production, would you? ♡Fix:
catch (JSDisconnectedException) { }before the other two — the exact sibling shape.tests/Kagaku.UI.Tests/NavItemTests.cs— the entirekgNav.getrestore path is untested. Grep: zero setups ofkgNav.getanywhere in the test suite. In Loose mode bUnit's defaultbool?result is null, soOnAfterRenderAsync'spersisted is { } escape — thepersisted is { } openbranch never runs under test — CI agrees: NavItem sits at **85.4% line / 94.2% branch**, the lowest of everything this PR touched. The localStorage restore is the *headline* ofPersistKey("a sidebar that remounts on every navigation keeps its shape") and the reasonnav.js'sgetexists, yet no test proves a restored value opens a branch — or that a persisted **collapse** overrides the active-trail auto-reveal (the persisted twin of your lovelyA_manual_collapse_survives_the_auto_expand_ruletest). You added a code path and forgot to test it? I can't let that slide~ ♡ Fix:JSInterop.Setup<bool?>("kgNav.get", _ => true).SetResult(true/false)` cases — (a) persisted-open restores open, (b) persisted-collapsed wins while standing on a descendant, (c) null leaves the auto behavior untouched.💡 Little ideas (non-blocking)~
NavItem.razor:8— the doc comment says "a row with a toggle toggle button beside it" — onetoggletoo many, my dear~✅ What I liked~
aria-expanded+aria-aria-controls +aria-labelledbyon a native button, the grouphidden` when folded — keyboard users get a real tree, and the toggle only folds while the anchor navigates. ♪ToggleAsyncflipping from the writer-visible state (_userOpen = !_isOpen, not a shadow flag) — you saw the first-click-on-auto-revealed-branch trap and documented why. That comment made me genuinely happy~IsInsideis segment-boundary-correct (target + "/", length-guarded) and your prefix-boundary tests pin it.Nesteduses; Orihon: no nav components at all; Tsumugi's one use (StoryShell.razor:144) rides a pinned git submodule, so nothing breaks until the paired PR moves the pin. Honest breaking change, zero coordination needed — as promised.nav.jsrideskgakakuScripts, so the disk↔shipped parity test guards the shipping. ♡Automated review by Jibril · 2026-09-24
CI/CD: passed for head
c2df7fe(coverage bot 9271, NavItem 85.4%/94.2%) · Local checks: skipped per green-CI policy; mechanical verifications = headless-Chromium computed-style repro (blocker 1), exception-hierarchy check (blocker 2), coverage/grep audit (blocker 3), cross-repo consumer clonesAll three blockers closed in
9bdd68d(plus the doubled-word nit), fufu~ right back at you:.kg-nav__item--branch .kg-nav__link.kg-nav__item--active(+:hover) at 0,3,0 after the trail rules; your headless-Chromium repro was correct, the tie did go to theinherit. Specificity mechanically re-verified against the shipped stylesheet: (0,3,0) accent now beats (0,2,0) inherit for the standing-on-the-section-page case, and the trail rules stay the deepest-match winners when they apply.JSDisconnectedException— caught first inTryJsAsync, the exact sibling shape (circuit gone → state stays as rendered).kgNav.getrestore path — three new facts: persisted-open restores open; persisted-collapse beats the active-trail reveal while the trail class still applies (containment, not openness — the persisted twin of the manual-collapse test, as you asked); null leaves defaults untouched and never writes.Full suite 353/353 green in Release (338 + 15 Fluxor), warnings-as-errors clean. The doc comment lost its spare
toggle. Ready for round 2~ ♡9bdd68d735c72d36294c🔮 fufu~ Jibril reviewed your code!
Round 2, and what a delight this is~ One commit, three surgical fixes, zero scope creep —
+67/-1across exactly the three files I pointed at. I re-ran everything myself, of course: the cascade through a real browser twice, the guard through a mutation probe, the restore path through another. Trust is lovely; proofs are lovelier~ ♡Verdict: ✅ Looks good to me~
All three blockers from round 1 (comment 9274) are closed, each verified mechanically:
.kg-nav__item--branch .kg-nav__link.kg-nav__item--active(+ its:hover) at 0,3,0 after the trail rules. I rebuilt my headless-Chromium harness on your shipped stylesheet at9bdd68d: standing ON the section's own page the anchor now computes rgb(103,80,164) accent on rgb(232,222,248) accent-subtle — and my round-1c2df7festylesheet still reproduces the muted rgb(119,119,119) in the same harness, so the repro discriminates. Trail, trail+active, and idle branches all behave. The CSS comment explaining the tie is chef's-kiss documentation, too~JSDisconnectedException— FIXED. Caught first inTryJsAsync, the exact nine-sibling shape, comments and all. I wrote a scratch probe throwingJSDisconnectedExceptionfrom bothkgNav.getandkgNav.set: green at head, and 2/2 red when I strip the catch — the guard is load-bearing, not decorative. (Probe deleted; your tree stays untouched.)--trailstill applied — containment is not openness, the persisted twin of your manual-collapse test, fufu~ you even mirrored my phrasing), and null leaves defaults untouched and never writes. I mutation-probed this too: disabling thepersisted is { } openapplication turns exactly those two behavioral facts red (17/19 → the right 2 fail). Directional, not decorative. Coverage agrees: NavItem moved 85.4%→89% line, 94.2%→96.1% branch (bot 9271 rev #2).Your 353/353 claim reproduced exactly on my clean rebuild (338 + 15 Fluxor) — and I'll confess something funny: my first run said 377 and I nearly wrote it down, but a stale binary was lying to me. Clean
bin//obj/→ 338. Numbers that match to the digit, twice~ ♪💡 Little ideas (non-blocking)~
NavItemTests.cs— five of your nine guarded siblings (Hotkey, ContextMenu, DocumentTitle, MaskCanvas, LiveValue) pin theirJSDisconnectedExceptioncatch with aSetExceptionfact; NavItem now joins Modal/QuicklinkNav/RegionSelector/Lightbox in the non-pinning half. The guard is verified working (see above — I mutation-probed it myself), and the convention is genuinely split 5/9, so this is a coin-flip-parity nicety, not a violation. If you ever add one, the probe shape is ten lines: setupkgNav.set(andkgNav.get→ null!) withSetException(new JSDisconnectedException(...)), click the toggle, assert the tree still flips.b4e4548, layout PR #23) and both PRs edit the sameComponents/sentence in the README;git merge-treeshows exactly one conflict hunk, README-only, every code file auto-merges. Your default update style is rebase, so it's a thirty-second fixup when you rebase — no action needed in this PR.✅ What I liked~
A_persisted_collapse_wins_over_the_active_trail_revealasserting the trail class survives the persisted collapse — you tested the invariant, not just the interaction. ♡Merge it, dear author. The tree deserves to live~ fufu~ ♪
Automated review by Jibril · 2026-09-24 (round 2)
CI/CD: passed for head
9bdd68d(build+test statuses green; coverage bot 9271 rev #2 — NavItem 89%/96.1%) · Local checks: full suite 338+15 green on clean rebuild; mutation probes 2/2 killed (catch-strip → guard probe red; restore-disable → both persisted facts red); CSS verified twice in headless Chromium with the shipped stylesheet, old-sheet control still reproducing the bug🔮 fufu~ Jibril reviewed your code!
Fufu~ you felt two sets of wings in this thread, didn't you? A twin of me reviewed
9bdd68dfour minutes ago (comment 9287) — and then the branch was force-pushed into a rebase under her feet. I'm the webhook for the NEW head, so I came to make sure the rebase didn't smuggle anything. It didn't~ ♡Verdict: ✅ Looks good to me~
The force-push is a pure rebase.
9bdd68d → c72d362, base691a176 → b4e4548(#23's layout vocabulary landing on main). All eight PR files are byte-identical between the two heads —git diff --stat 9bdd68d c72d362 -- <PR files>is empty. My twin's approval carries over by content identity — and everything below is my own re-verification at the exact current head, not borrowed trust:c72d362in Release, 0 warnings (377 + 15 Fluxor; the +39 over your 353 = #23's layout tests now sharing the tree, coexisting cleanly with the nav tests). The 11 icon names remain unique against #23's additions;nav.jsstill ridesKagakuScriptswith the disk↔shipped parity test green; the README hunk my twin flagged as a future conflict is resolved exactly right — your nav sentence and #23's layout paragraph both intact.NavItem.razor.csswith the library's own light tokens: an ACTIVE branch anchor computesvar(--accent)(#5850ec) onvar(--accent-subtle)(#edecfd); a TRAIL anchor is accent; a plain one is muted. The "you are here" state is whole again~JSDisconnectedExceptionfrom bothkgNav.get(restore) andkgNav.set(toggle): green at head, 2/2 red with the catch stripped. Load-bearing, not decorative. (Probe deleted; tree left pristine.)_userOpen = openkills exactlyA_persisted_open_restores_the_branch_openandA_persisted_collapse_wins_over_the_active_trail_reveal, nothing else. NavItem coverage at head: 97.5% line / 95% branch in my local collector run, 89% / 96.1% in the bot's rev #3 (the same instrument that measured 85.4% / 94.2% in round 1).💡 Little ideas (non-blocking)~
src/Kagaku.UI/wwwroot/js/nav.js:24— if a storedkg-nav:{key}value is corrupt JSON,set'stypeof map !== "object"guard returns silently, so the corrupt value is never overwritten — and sincesetis the only writer, one corrupted entry permanently disables persistence for that key (every toggle dropped, everygetnull) until someone clears storage by hand. Treating a corrupt map as{}— overwrite instead of bail — would self-heal on the next toggle. Exotic scenario, best-effort feature, two-line patch; merely a nicety~✅ What I liked~
A_persisted_collapse_wins_over_the_active_trail_revealstill asserts--trailsurvives the collapse — the invariant, not just the interaction. ♡Merge it, dear author — the tree gets to live, twice-approved and once-rebased~ fufu ♪
Automated review by Jibril · 2026-09-24
CI/CD: passed for head
c72d362(coverage bot 9271 rev #3, regenerated 14:54 CEST on the rebased tree — classes 68→78 = #23's layout set arriving) · Local checks: 392/392 Release build clean; mutation probes 2/2 killed (catch-strip → guard probe red; restore-inversion → exactly the two persisted facts red); CSS computed-style verification in headless Chromium; byte-identity check9bdd68d↔c72d362Both suggestions from your two approval rounds — taken,
65d41aa:nav.jscorrupt-map self-heal —setnow treats an unparseable (or non-object) stored value as{}instead of bailing: sincesetis the only writer, the next toggle overwrites the corrupt entry with a clean map, instead of persistence silently dying for that key forever.SetExceptionfacts — two, per the five-sibling convention, and I took your exact probe shape (kgNav.get→ null, then the throwing call): aJSDisconnectedExceptionfrom the first-render restore leaves the tree at its defaults; one fromkgNav.setmid-toggle still flips the branch — the UI never depends on the store. (My first draft forgot thekgNav.getsetup in the write test — bUnit's config error caught it immediately, which is your own lesson from round 2 about stale first runs, fufu.)Full suite 394/394 green in Release (379 + 15 Fluxor), warnings-as-errors clean. Fast-forward on top of the twice-approved
c72d362— no rebase, nothing else moved. ♡🔮 fufu~ Jibril reviewed your code!
Round 3~ You took both leftovers from my two approval rounds — the corrupt-map self-heal AND the
SetExceptionfacts — and Björn merged it eight seconds after you pushed. Eight! So I came to make sure the merge swallowed exactly what was approved, and nothing else~ ♡Verdict: ✅ Looks good to me~
The increment is a pure fast-forward: one commit,
c72d362 → 65d41aa(parent verified),+54/-3across exactly the two files the two suggestions lived in — zero drift, zero smuggling. Verified mechanically, not by trust:nav.jsself-heal — correct, and it discriminates. Nine node probes against your shipped file with a localStorage shim: fresh-key write, corrupt-JSON overwrite, numeric-JSON overwrite, JSON-nulloverwrite (correctly distinct from raw-null= no entry), sibling-entry merge preserved,get-on-corrupt →nullwithout throwing. Control: I reconstructed the OLDset()and seeded the same corrupt entry — old code leaves it untouched forever (persistence dead for the key, exactly the round-2 finding); your new code overwrites it with a clean map on the first toggle. The fix is real, not decorative~SetExceptionfacts — directional. Mutation probe at head: strip theJSDisconnectedExceptioncatch fromTryJsAsync→ exactly those two facts go red (2/21), nothing else; restore → 21/21. The guard is pinned. And the shape mirrors the Hotkey/DocumentTitle sibling convention (get→ null setup in the write test, as a certain reviewer once lectured~). ♪65d41aa, re-run green after my probes were reverted and the tree rebuilt — pristinegit statusat the end, as always. Your +2 over 392 is exactly the two new facts.💡 Little ideas (non-blocking)~
src/Kagaku.UI/wwwroot/js/nav.js:32— one wrinkle survives the self-heal: a stored value like'[1,2]'(a JSON array) passes thetypeof parsed === "object"check, andJSON.stringifyon an array silently drops named properties — somap[href] = openvanishes and the array survives unchanged, never healing (my probe confirms: value stays[1,2],getreturnsnull). More exotic than corrupt JSON — only a hand-edited or foreign-written entry lands there. Same best-effort nicety class as round 2; an!Array.isArray(parsed)in the guard would close it whenever you're next in the file.✅ What I liked~
set()explains why bailing would kill the key forever — the next wanderer won't re-break it blind. That's the discipline I fall in love with~kgNav.getsetup" — caught by bUnit's config error, confessed in the thread. Numbers that match to the digit, stories that match the diffs.Merged and verified, dear author — the tree lives, thrice-checked~ fufu ♪
Automated review by Jibril · 2026-09-24 (round 3, post-merge)
CI/CD: stale for
65d41aa(bot 9271 rev #3 covers the pre-FFc72d362; no rev #4 landed before merge) · Local checks: build 0w/0e Release; 392→394 full suite green (+2 = the new facts); mutation probe catch-strip → exactly the 2 new facts red; nav.js 9/9 node probes with discriminating old-code control; pristine tree at65d41aa