feat(sort): add SortToggle for Logs, History, and Network (#1371)

[cliffhall]

Summary

• Adds a reusable SortToggle element (Mantine Select with "Sort: Newest First" / "Sort: Oldest First").
• Wires it into the Logs, History, and Network panels and applies a memoized chronological sort to the rendered entries (source state is untouched — sort stays presentational).
• Persists each screen's choice independently to localStorage under inspector.sortDirection.{logs,history,network} via @mantine/hooks useLocalStorage. Invalid stored values fall back to the newest-first default so the toggle never renders in an unselectable state.
• Adds a small in-memory localStorage shim in the unit-test setup because Node 22+ exposes an experimental placeholder that masks happy-dom's implementation when --localstorage-file is not provided.

Closes #1371.

Test plan

• npm run validate (format, lint, build, unit tests + coverage)
• npm run test:storybook (323 stories pass under headless Chromium)
• Manually flip sort in each screen, reload, and confirm restore
• Manually corrupt inspector.sortDirection.logs in DevTools and confirm fallback to "Sort: Newest First"

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 44s —— View job

---

Claude PR Review — feat(sort) SortToggle for Logs, History, Network

• Read PR diff against origin/v2/main
• Review SortToggle element + tests/stories
• Review per-panel sort wiring (Logs, History, Network) and persistence
• Review InspectorView shim + test setup localStorage shim
• Post final review

---

Overall this is a clean, well-tested PR. The state shape (source order untouched, sort is purely presentational) is the right call, the inspector.sortDirection.<panel> namespace is consistent, and the corruption-fallback path is both implemented and covered. A few findings, ordered by impact.

Suggestions

1. Initial-mount flicker — useLocalStorage hydrates in useEffect (minor UX)

@mantine/hooks useLocalStorage defaults to getInitialValueInEffect: true, so on first render logsSort / historySort / networkSort all return SORT_DEFAULT ("newest-first") and then re-render with the persisted value after the effect runs. A user who previously chose "oldest-first" sees one frame of newest-first ordering, then the list re-sorts. It's particularly visible on the Logs and History panels because their rendered order changes.

Since this is a pure SPA (no SSR), reading synchronously is safe:

const [logsSort, setLogsSort] = useLocalStorage<SortDirection>({
  key: "inspector.sortDirection.logs",
  defaultValue: SORT_DEFAULT,
  deserialize: deserializeSortDirection,
  serialize: serializeSortDirection,
  getInitialValueInEffect: false, // hydrate on first render, no flicker
});

Fix this →

2. Test-setup localStorage shim is installed unconditionally (minor)

clients/web/src/test/setup.ts:31 installs MemoryStorage for every test, replacing happy-dom's implementation even when Node isn't exposing the experimental placeholder. That's harmless today, but it means the shim — not happy-dom — is what the suite is actually testing against. Two small upsides if you guard it:

• Tests stay closer to "what the app would do in a browser"
• If happy-dom ever ships a new Storage event behavior, you get it for free

Suggested guard:

if (typeof window.localStorage?.setItem !== "function") {
  Object.defineProperty(window, "localStorage", {
    configurable: true,
    value: new MemoryStorage(),
  });
}

Not blocking — happy to keep the unconditional version as long as the comment makes the intent explicit ("we always replace, not just when missing").

3. Redundant array copy before .sort() (trivial)

In LogStreamPanel.tsx:76, HistoryListPanel.tsx:89, and NetworkStreamPanel.tsx:100:

const matched = entries.filter(...);
const sorted = [...matched].sort(...);

.filter() already returns a fresh array, so the spread is a no-op copy. matched.sort(...) works correctly and avoids the extra allocation. Cosmetic — performance impact is negligible at the entry counts these panels handle.

4. SortToggle width is hard-coded to 190px (minor)

SortToggle.tsx:28 sets w={190}. That fits the current "Sort: Newest First" / "Sort: Oldest First" copy, but it'll truncate or look cramped if these strings are ever localized (German "Sortierung: Neueste zuerst" is ~30 chars, vs. ~18 today). Not actionable in this PR, but worth a TODO or moving the width into the theme so it's easy to revisit.

Nits (no action needed)

• SortToggle.test.tsx:8 — screen.getByDisplayValue("Sort: Newest First") is a great anchor for the textbox role-based queries below. Nice.
• setup.ts:18 — getItem could simplify to this.store.get(key) ?? null instead of has + non-null assertion; same observable behavior.
• InspectorView.tsx:58 — serializeSortDirection is intentional (overrides Mantine's default JSON.stringify so storage holds the raw enum value and the test on line 362 can getItem it directly). Worth a one-line comment so the next reader doesn't try to "simplify" it away.

Things I liked

• Source state stays untouched — sort is computed in the presentation layer (useMemo on entries). Easy to reason about and trivially testable.
• deserializeSortDirection clamps invalid values to the default and the InspectorView test (falls back to newest-first when a corrupted sort value is stored) covers it. That's exactly the right shape for a forward-compatible enum.
• Each panel's SortToggle has a distinct aria-label ("Logs sort direction", etc.) so there's no a11y collision when the screens swap.
• Persistence test covers the round trip (mount → flip → unmount → remount → restore). Good signal.
• Auto-scroll cleanup is complete — no dangling references in src/.

Verdict

LGTM with the suggestions above. The getInitialValueInEffect: false change is the only one I'd push for before merge; the rest are easy follow-ups or cosmetic.
· branch: feat/1371-sort-toggle

[cliffhall]

Thanks for the review! Responses below, applied in 1730c8b.

Suggestions

1. Initial-mount flicker — getInitialValueInEffect: false ✅ Done. Added to all three useLocalStorage calls in InspectorView.tsx and noted the SPA / no-SSR rationale in the comment.

2. Conditional localStorage shim in test setup — Left as unconditional. The existing block comment already documents that the Node 22+ experimental placeholder masks happy-dom's implementation, and the shim is intentionally a uniform substrate across CI / local / future Node versions. I'd rather not have the suite behave one way on Node 22+ and another way on older runtimes.

3. Redundant [...matched].sort() ✅ Done in all three panels. Replaced with a single chained .filter().sort() and a one-line comment noting the in-place sort is safe because .filter() already returns a fresh array.

4. Hard-coded 190px SortToggle width — Deferred per your own note ("not actionable in this PR"). If/when we localize, the right move is to lift the width into the theme or switch to min-w-based intrinsic sizing; either way it's a separate change.

Nits

• ✅ MemoryStorage.getItem collapsed to this.store.get(key) ?? null.
• ✅ Added a comment on serializeSortDirection explaining the override of Mantine's default JSON.stringify.
• SortToggle.test.tsx:8 — thanks!

Validate + unit + storybook all green after the changes.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 4s —— View job

---

Claude PR Re-Review — feat(sort) SortToggle for Logs, History, Network

• Read PR diff against origin/v2/main
• Verify previous-review fixes landed correctly
• Re-review SortToggle element + tests
• Re-review per-panel sort wiring (Logs, History, Network)
• Re-review InspectorView persistence + test setup shim

---

Re-reviewing after 1730c8b2. The fixes from the prior round all landed cleanly and the rationale on the deferrals is fair. The code is in good shape — a few small observations below, none blocking.

Verification of previous-round changes

Item	Status	Notes
getInitialValueInEffect: false on all three useLocalStorage calls	✅	InspectorView.tsx:318,325,332. Comment at lines 307–312 captures the SPA / no-SSR rationale.
Redundant [...matched].sort() collapsed	✅	LogStreamPanel.tsx:73, HistoryListPanel.tsx:86, NetworkStreamPanel.tsx:97 — all three carry the same "fresh array, in-place sort is safe" comment. Consistent.
MemoryStorage.getItem simplified	✅	setup.ts:17–19 now this.store.get(key) ?? null.
Comment on serializeSortDirection	✅	InspectorView.tsx:58–60 explains the override and the test-assertion benefit.
Conditional localStorage shim	🟡 Deferred — sound reasoning (uniform substrate across CI/local/future Node).
190px hard-coded width	🟡 Deferred — flagged in PR comments as a follow-up.

Fresh observations on this revision

1. Three near-identical useLocalStorage blocks in InspectorView (trivial, optional)

InspectorView.tsx:313–333 repeats the same 6-line block three times, differing only in the scope key (logs / history / network). A tiny local helper would dry it up:

function useSortDirection(scope: "logs" | "history" | "network") {
  return useLocalStorage<SortDirection>({
    key: `inspector.sortDirection.${scope}`,
    defaultValue: SORT_DEFAULT,
    deserialize: deserializeSortDirection,
    serialize: serializeSortDirection,
    getInitialValueInEffect: false,
  });
}

…then const [logsSort, setLogsSort] = useSortDirection("logs"), etc. Not blocking — three repetitions sits right at the line where "extract" and "leave inline" are both defensible, and the current form keeps the storage adapter visible at the call site. Mention it only because if a fourth panel adopts sort later, this is the obvious moment.

2. isSortDirection and deserializeSortDirection validate the same union (nit, no action)

SortToggle.tsx:16 and InspectorView.tsx:54 both type-guard the same "oldest-first" | "newest-first" set. They live in different layers (component input vs. storage hydration) and don't need to share, but it's worth noting if the enum ever grows a third value — both call sites need to be updated. A single exported isSortDirection from SortToggle.tsx would centralize it; up to you whether the indirection is worth it.

3. Sort-then-reverse vs. directional comparator (stylistic)

LogStreamPanel.tsx:76–77:

.sort((a, b) => a.receivedAt.getTime() - b.receivedAt.getTime());
if (sortDirection === "newest-first") sorted.reverse();

You could fold direction into the comparator and skip the reverse(), but the current form is arguably clearer — one canonical ascending sort, then a final flip. Leave as-is.

Things I liked (carried over + new)

• Source state is still untouched — sort lives in useMemo on the rendered list. Clean separation.
• Cross-tab sync comes for free via useLocalStorage's storage-event listener — and per-screen keys mean Logs sync to Logs, History to History, etc., without interference.
• Persistence round-trip test (InspectorView.test.tsx:337–386) covers mount → flip → unmount → remount → restore. Good signal.
• Corruption-fallback test (InspectorView.test.tsx:388–409) directly asserts the deserializer clamps "garbage" back to "newest-first". Exactly the right shape.
• Distinct aria-labels per panel (Logs sort direction, History sort direction, Network sort direction) keep the a11y story clean when screens swap.
• Test setup shim docstring (setup.ts:5–8) accurately captures why the shim exists. Future readers won't be confused.

Verdict

LGTM. Previous review items are all addressed correctly; the remaining notes are all optional polish.
· branch: feat/1371-sort-toggle

[cliffhall]

Thanks for the re-review! Applied in 2ef62cb.

Fresh observations

1. DRY the useLocalStorage blocks ✅ Done. With four compact hooks added since the last review (servers/resources/history/network) the duplication had grown to 7 blocks, so I extracted two scoped helpers (useSortDirection(scope) and useListCompact(scope, defaultValue)) in InspectorView.tsx. The call sites now read as e.g. useSortDirection(\"logs\") and useListCompact(\"resources\", false) — the storage shape (namespace, adapters, getInitialValueInEffect) lives in the helper and the rationale comment moved up there too. Net: −62/+49 lines.

2. Duplicate isSortDirection / deserializeSortDirection validation — Deferred. The two-value union and two-site footprint make the indirection feel heavier than the duplication. If we add a third option the noise from updating both becomes obvious enough that we'd want to consolidate then.

3. Sort-then-reverse vs. directional comparator — Left as-is per your own note that the current form is arguably clearer.