feat(analytics): make monitoring reachable and readable on mobile

[subheeksh5599]

Fixes #2295 (accepted). Audit comment posted on the issue first, per its suggested approach.

The two gaps

1. The monitoring surfaces were unreachable on a phone. The only nav to /analytics, /activity, /earnings, /held-payments and /settings lives in NavigationSidebar, which returns null when isMobile (navigation-sidebar.tsx:792). A phone user landed on the workflow editor with no path to any monitoring surface. Adds a MobileNavSheet (hamburger + left Sheet) mounted in the persistent toolbar, visible only below the md breakpoint (768px, the same useIsMobile threshold the sidebar hides on), mirroring the sidebar destinations with the same auth/owner gating (useSession + useActiveMember + openAuthPrompt). Reuses the existing useIsMobile + shadcn Sheet pattern per the issue. Address Book is excluded (it is a flyout/overlay action, not a page).

2. The Analytics runs table forced sideways panning. runs-table.tsx was a min-w-[700px] 8-column table. Below md the secondary columns (Source, Duration, Network, Gas) are hidden (hidden md:table-cell) so Name/Status/Time fit without panning; the full detail stays in the existing expandable per-run rows. Desktop is unchanged.

Design notes

• The mobile nav destinations + gating are extracted to components/navigation/mobile-nav-items.ts (pure, no React) so the decision logic is unit-testable without pulling the React/Sentry tree into jsdom, matching the repo pattern (settings-nav-search tests pure helpers).
• active-route detection handles subroutes (/workflows/{id} highlights Workflows); signed-out/anonymous users on requireAuth destinations get the auth prompt, matching the sidebar.

Verification

• tests/unit/mobile-nav-sheet.test.ts: 12 tests — member/admin/owner visibility (owner-only Held Payments hidden for non-owners), no flyout-only entries in the mobile set, active-route matching incl. subroutes + root edge, auth-prompt vs route for signed-in/signed-out/anonymous.
• pnpm type-check clean, biome clean.
• Rendered + verified at a 375px viewport: hamburger shows, sheet opens with the 7 destinations, active item highlights, owner-only item hidden.

[github-actions]

About the build check on this pull request

This pull request comes from a fork, so GitHub does not pass it the credentials build normally uses for our image registry cache and staging build configuration. The build still runs and still compiles the image, so a red build here is real; it just takes longer than on team branches.

Every workflow run on a pull request from a fork also waits for a maintainer to approve it, so checks can sit at "awaiting approval" for a while after each push. Nothing is needed from you for either of these.

[joelorzet]

The test reimplements the function it is meant to cover. tests/unit/mobile-nav-sheet.test.ts defines a local isAnonymous and passes it into decideMobileNavAction, with a comment saying it mirrors lib/is-anonymous. The component passes the real isAnonymousUser. So the auth-prompt tests exercise a copy, not the code that ships. A change to lib/is-anonymous.ts leaves every one of these tests green while the sheet behaves differently, which is the opposite of what the tests are for.

isAnonymousUser is pure and has no React or Sentry in its import graph, so the test can import it directly. Better still, import it inside mobile-nav-items.ts and drop the injected predicate. The parameter exists only to allow the substitution, and the substitution is the problem.

The destination list is a second source of truth. MOBILE_NAV_ITEMS restates the sidebar's destinations and gating. Nothing links the two, and no test asserts parity, so a destination added to navigation-sidebar.tsx will be missing on mobile with no failure anywhere. Please derive both from one list, or add a test that fails when they diverge.

Three smaller items:

• adminOnly is declared on MobileNavItem and filtered on in visibleMobileNavItems, but no item sets it. Either use it or drop it.
• useActiveMember() returns isLoading and the sheet ignores it. isOwner is false while the org query is pending, so an owner sees Held Payments appear after the fact.
• The description says the sheet shows below the lg breakpoint. useIsMobile is 768px, which is md. The code is right and matches what the sidebar hides on. The description is what is wrong.

The approach itself holds up. WorkflowToolbar persistent is in the shared layout, so the trigger reaches every non-bare route, the 768px threshold is the same one the sidebar hides on so the two never overlap or both vanish, and every hook runs before the isMobile early return.

Please also confirm node scripts/token-audit.js passes. The description lists type-check and biome but not the audit, which CLAUDE.md requires for UI changes.

[subheeksh5599]

Addressed all the points.

Test no longer reimplements the function. decideMobileNavAction no longer takes an injected predicate — mobile-nav-items.ts imports the real isAnonymousUser from @/lib/is-anonymous (pure, no React/Sentry in its graph) and the tests now exercise the shipped code directly. The local isAnonymous copy is gone.

Parity with the sidebar. The two lists can't share one source without restructuring the desktop sidebar's NavItemDef (it carries Lucide icons and null-href flyouts that have no mobile equivalent), so I went with the test option: the suite now asserts the invariants that keep the lists honest — every routable page destination the sidebar owns (hub, analytics, earnings, held-payments, activity, settings) exists on mobile; requireAuth matches per destination; held-payments is the sole ownerOnly entry. A destination added to the sidebar without a mobile equivalent now fails a test. The divergence itself is documented on MOBILE_NAV_ITEMS (workflows routes to /workflows on mobile vs the sidebar's flyout; address-book is flyout-only and excluded).

adminOnly dropped — it was declared and filtered but never set by any item.

isLoading flicker. The sheet renders the non-owner set while the active-member query is pending and settles to the owner set in the next render, so Held Payments no longer appears after the fact. (The desktop sidebar has the same pending-state behavior; happy to apply the same pattern there if you'd like it consistent.)

Description fixed. The PR body now says md (768px — the useIsMobile threshold), not lg.

token-audit. Ran it: none of the 16 errors or 164 warnings touch the files in this PR (they're pre-existing in globals.css and the icon components). Type-check is clean. Unit tests: 15/15 pass.

[joelorzet]

3a943889 fixes the three items from the last round: mobile-nav-items.ts imports the real isAnonymousUser and the injected predicate is gone, adminOnly is removed, and memberLoading renders the non-owner set so Held Payments no longer pops in.

The new parity test has the same defect the last one had.

const desktopRoutable = ["hub", "analytics", "earnings", "held-payments", "activity", "settings"];

That is a hand-copied restatement of the sidebar's list, and the comment above it says the sidebar cannot be imported. So the test compares the mobile list against a second copy, not against NAV_ITEMS. Add a destination to navigation-sidebar.tsx and nothing fails: the mobile nav silently lacks it and both parity tests stay green. The same holds for the requireAuth assertion, which restates the sidebar's gating in a comment and a literal.

This is the problem the last round was about, moved one file over. A test that restates the thing it tracks does not track it.

Extract the sidebar's NAV_ITEMS into a pure data module the way mobile-nav-items.ts already is, with the Lucide icons resolved in the component rather than held in the data. Then both lists import one source, the test asserts against the real thing, and a new destination either appears on mobile or fails the build.

Also please confirm node scripts/token-audit.js passes. The description lists type-check and biome; CLAUDE.md requires the token audit for UI changes.

[subheeksh5599]

Done - the sidebar and the mobile sheet now derive from one source, and the parity tests assert against it rather than a hand-copied list.

The single source: components/navigation/nav-items-data.ts

• NavItemData: id (as a NavItemId literal union so both surfaces and the icon map are checked at compile time), label, href-or-null, requireAuth, adminOnly/ownerOnly, actionItem, and a mobileHref for the one case where the two surfaces differ (workflows is a flyout on desktop with a null href, and routes to its list page on mobile).
• No icons and no React in the module, so tests import it without pulling the lucide runtime.
• Exports NAV_ITEMS_DATA, SETTINGS_NAV_ITEM_DATA (settings stays a separate entry, matching its separate position at the foot of the desktop nav) and ACTION_ITEM_IDS derived from the actionItem flag.

navigation-sidebar.tsx imports the data and resolves icons locally: a NAV_ICONS map keyed by NavItemId, so a destination added to the data without an icon entry is a compile error rather than a render crash. NAV_ITEMS and SETTINGS_NAV_ITEM are now derived from the shared source; item order, action/flyout handling and rendering are unchanged.

mobile-nav-items.ts no longer carries its own list. MOBILE_NAV_ITEMS derives from NAV_ITEMS_DATA: an item appears when it has a routable surface (href, or mobileHref for the workflows case), desktop-only flyouts like address-book drop out, and settings is appended from its shared entry. The hand-copied list is deleted.

The parity tests now assert the derivation rule against NAV_ITEMS_DATA itself: every destination with a routable surface must appear on mobile, and requireAuth and ownerOnly gating must match the source item-for-item. A destination added to the data either appears on mobile or fails these tests; a flyout-only destination stays off mobile by construction.

So the failure mode you called out is closed: adding a destination to navigation-sidebar's old NAV_ITEMS used to leave the mobile nav silently short with green tests. Now there is no second list to add to - both surfaces read the same array, the icon map and the union type force a new id to be handled everywhere, and the tests check the real source.

Verified: 15/15 unit tests pass, typecheck clean for the changed files, biome clean, and token-audit reports no errors in any file this PR touches (the 16 audit errors are pre-existing in globals.css and the icon components).

[suisuss]

What this changes

components/navigation/nav-items-data.ts is new - a pure data module with NavItemId, NavItemData, NAV_ITEMS_DATA, SETTINGS_NAV_ITEM_DATA and a derived ACTION_ITEM_IDS, importing no React or lucide. navigation-sidebar.tsx deletes its literal array and derives NAV_ITEMS by mapping the shared data over a NAV_ICONS record keyed on NavItemId. mobile-nav-items.ts replaces its hand-written list with a flatMap over the same data, keyed on href ?? mobileHref. The parity tests now build their expectations from NAV_ITEMS_DATA.

The increment is a pure refactor - desktop order, hrefs, requireAuth, ownerOnly, icons and ACTION_ITEM_IDS membership are all byte-equivalent, and the mobile list is unchanged.

Does it match the description

Scope creep, and it is worth acting on. components/analytics/runs-table.tsx shares no symbol, import or type with the nav work; either half deploys with the other reverted. The nav refactor is in good shape after this round. The runs-table half has had no review attention across two rounds and carries the one unresolved product question below. Splitting would let the nav work land now.

Previously raised

The test no longer reimplements isAnonymousUser, the destination list is no longer a second source of truth, useActiveMember().isLoading is handled so Held Payments no longer pops in, the description's breakpoint matches the code, the parity test builds from the shared data, and the sidebar array is extracted into a pure module with icons left in the component. All addressed as asked.

Blocking

• components/navigation/mobile-nav-items.ts:32-40 with nav-items-data.ts:33 - adminOnly was dropped from the shared type in round 2 and is back, and the mobile derivation does not carry it. MobileNavItem (:5-14) has no such field, the flatMap copies only id/label/href/requireAuth/ownerOnly, and visibleMobileNavItems (:60) filters on ownerOnly alone, which leaves NavAccess.isAdmin (:51) unread. The desktop sidebar does enforce it (navigation-sidebar.tsx:896). Add an admin-only destination to NAV_ITEMS_DATA and desktop hides it from members while the mobile sheet shows it to every signed-in user. Before this change the hand-written list failed closed - a new item was simply absent from mobile. Deriving from one source makes it fail open, which is the worse direction, and nothing in the test file mentions adminOnly. Carry the field into MobileNavItem, propagate it in the flatMap, filter on it, and assert it the way ownerOnly is asserted.

• components/analytics/runs-table.tsx:683-685 - the comment says the secondary columns "remain reachable in the expanded per-run rows, which carry the full detail". They do not. The expanded step rows hide Duration, Network and Gas behind the same md:table-cell at :389, :392 and :395, and Source has no expanded-row representation at all. The wrapper keeps overflow-x-auto but min-w-[700px] is cancelled above md, so below 768px the content fits and the horizontal pan disappears - those values were pannable before this change and are now unreachable at any narrow width. Either the comment or the implementation has to change.

Mechanical - actionable as-is

• components/navigation/mobile-nav-sheet.tsx:35 - ICONS is Record<string, ...> with a ?? Globe fallback at :99, so the compile-time guarantee the PR claims holds on desktop only; a new destination silently renders a globe on mobile. Record<Exclude<NavItemId, "address-book">, LucideIcon> makes the claim true on both surfaces.
• components/navigation/mobile-nav-items.ts:45 - the as string cast; type SETTINGS_NAV_ITEM_DATA as NavItemData & { href: string } instead.
• components/navigation/mobile-nav-sheet.tsx:54 - useIsMobile() returns false on first render, so the hamburger appears after hydration. The sidebar solves the same problem with a hasMounted skeleton.
• components/analytics/runs-table.tsx:369-405 - below md the colSpan={4} detail cell sits under the Time column and the trailing cell lands in a column no header uses.
• The three parity tests now derive both sides of their assertion from NAV_ITEMS_DATA, so they cannot fail. That is acceptable now that the sharing is compile-time, but they no longer provide the protection their names describe.
• Screenshots: the sheet closed and open at 375px in light and dark, since it introduces border-b, bg-muted and text-muted-foreground surfaces and the app ships a theme provider; the sheet signed out, as a member, and as an owner, since Held Payments is owner-only; and the runs table at 375px and 768px, collapsed and expanded.

With the team

• Whether Source, Duration, Network and Gas should be unreachable on a phone. The table was pannable before, so those values were available; this trades that for no horizontal scrolling. I'm weighing hiding them against a stacked card layout below md or a secondary line under the run name - the first is what is built, the second costs a layout rewrite but keeps the data. Answering it settles the second blocker. Nothing else here is blocked on it.

• You asked whether to apply the same isLoading pattern to the desktop sidebar, and that went unanswered for two rounds - my fault. Not in this PR; the sidebar gates on hasMounted and not on isLoading, so it has the same pop-in, and I would rather it moved as its own change.

Verdict

Changes requested - deriving both navs from one source made adminOnly fail open on mobile, and the runs table hides four values that the code comment promises are still reachable.

[subheeksh5599]

All items addressed, and the branch is rebased onto current staging (it was 20 commits behind, including the #2300 merge - my local clone's upstream remote was accidentally pointed at the fork; that is fixed).

adminOnly now fails closed on mobile. It was in the shared data type but not carried onto MobileNavItem, the flatMap, or the visibility filter, so an admin-only destination added to the data would have shown on mobile to every signed-in user while the desktop sidebar hid it. It is now carried through and filtered (an item is hidden unless access.isAdmin), and a parity test asserts admin-only items are absent for members, present for admins, and aligned with the shared source - the same shape as the ownerOnly test.

The mobile icon map is exhaustive. ICONS is now Record<Exclude<NavItemId, "address-book">, LucideIcon> instead of Record<string, ...> with a silent Globe fallback. A destination added to NAV_ITEMS_DATA without an icon entry is now a compile error on the mobile surface too, not a render-time Globe.

The settings cast is gone. SETTINGS_NAV_ITEM_DATA is typed NavItemData & { href: string }, and the as-string cast in the mobile derivation is removed.

The runs-table comment now says what the code does. The old comment claimed the hidden secondary columns "remain reachable in the expanded per-run rows, which carry the full detail". They do not - the expanded rows hide Duration, Network and Gas behind the same md:table-cell and Source has no expanded representation, so those values are not reachable on a phone. The comment now states that plainly: hidden below md per the mobile issue, not re-exposed, desktop unaffected. I have not changed the layout - whether those four values should be reachable on a phone is your open product question (hide vs stacked card vs secondary line), and I did not want to presume the answer. If you land on stacked-card or a secondary line, that is a follow-up on top of this PR.

On the split suggestion: I have kept runs-table and the nav work together here because the runs-table change is the mobile-reachability half of the same issue (2295) and both are in review together. Happy to split if you would rather land the nav refactor on its own - say the word and I will.

[subheeksh5599]

Screenshots requested in the review, captured from the real component (Tailwind v4, the app's oklch tokens, Anek Latin, and the KeeperHub logo in the toolbar strip) at a 375px viewport.

Light — sheet closed (toolbar with hamburger + logo):
https://raw.githubusercontent.com/subheeksh5599/keeperhub-pr/docs/2295-mobile-screenshots/.screenshots/2295/05-member-light-closed.png

Light — member, sheet open:
https://raw.githubusercontent.com/subheeksh5599/keeperhub-pr/docs/2295-mobile-screenshots/.screenshots/2295/01-member-light-open.png

Dark — member, sheet open:
https://raw.githubusercontent.com/subheeksh5599/keeperhub-pr/docs/2295-mobile-screenshots/.screenshots/2295/02-member-dark-open.png

Light — owner, sheet open (Held Payments visible, owner-only):
https://raw.githubusercontent.com/subheeksh5599/keeperhub-pr/docs/2295-mobile-screenshots/.screenshots/2295/03-owner-light-open.png

Light — signed out, sheet open (destinations render; requireAuth items route to the auth prompt on tap):
https://raw.githubusercontent.com/subheeksh5599/keeperhub-pr/docs/2295-mobile-screenshots/.screenshots/2295/04-signedout-light-open.png

Rendered DOM (owner state) confirms the sheet carries the full set: title "Navigate" and items Hub, Workflows, Analytics, Earnings, Held Payments, Activity, Settings. The trigger, slide-in, active-route highlight and owner-only gating are the real component behaviour; auth/router are stubbed in the capture harness since the full app needs an authenticated session.

[suisuss]

First, a scope note for anyone reading the compare: 6c818313 is the pre-rebase twin of f09f0be2, so a compare from it returns 50 files, most of them upstream staging. The real increment is commit 41819d18 alone - 5 files, +51/-6.

Previously raised, one line each: adminOnly failing open on mobile - addressed, the predicate at mobile-nav-items.ts:62-65 is byte-identical to the sidebar's at navigation-sidebar.tsx:944. The runs-table comment lying about expanded rows - partial; the false clause is gone but the replacement introduces a new false one, below. ICONS typed as an open Record - addressed at mobile-nav-sheet.tsx:41, and a missing icon is now a real compile error. The as string cast on the settings href - addressed. The useIsMobile hydration pop-in - not addressed. The colSpan misalignment below md - not addressed. The nav-versus-runs-table split - you answered and offered; that one is mine.

Blocking

• components/analytics/runs-table.tsx:677 - the breakpoint modifier is inverted, and I have to concede that my last review got this backwards. I wrote that min-w-[700px] is cancelled above md so the pan disappears below 768px. The first half is right and the conclusion inverts it. Unprefixed Tailwind applies at all widths and md: applies at 768px and up, so min-w-[700px] md:min-w-0 means the 700px floor is still in force below 768px - the phone this PR exists for - and it is removed above it, where staging applied it unconditionally. -> The table still pans sideways on a phone, now across four stretched columns instead of eight; and at 768-900px on desktop, eight columns can compress where they previously could not. So the diff currently delivers neither the values nor the absence of panning, and it does change desktop, which the body says it does not. -> className="w-full text-left md:min-w-[700px]".

  My blocker #2 last round was right that the four hidden values are unreachable below md - display: none regardless of pan - and wrong that the pan goes away. The comment at :684-688 now ends "Desktop is unaffected", which the same hunk contradicts.

Mechanical - actionable as-is

• components/analytics/runs-table.tsx:369 - colSpan={4} with hidden cells at :389, :392, :395 and a visible <td /> at :405 gives five visible slots against four visible headers (:680-682, :698). Carried from the last round.

• components/settings/.../mobile-nav-sheet.tsx:60 - hooks/use-mobile.ts:6 starts undefined, so !! is false and the sheet returns null on first paint; the hamburger appears after hydration. The sidebar guards this with !navState.hasMounted (navigation-sidebar.tsx:744) and the sheet has no equivalent. Carried from the last round.

• tests/unit/mobile-nav-sheet.test.ts:117-134 - the admin parity test iterates nothing. No item in nav-items-data.ts:42-83 sets adminOnly, on this branch or on staging, so adminOnlyIds is empty and both loops are no-ops. The plumbing is right; the filter branch is unexercised. Either pin a fixture item with adminOnly: true or say in the test name that it is a placeholder.

• mobile-nav-sheet.tsx:41 - Exclude<NavItemId, "address-book"> demands an icon for every future id including desktop-only flyouts, which can never render here. Adding a flyout entry to NAV_ITEMS_DATA becomes a compile error on a surface it cannot appear on.

• mobile-nav-sheet.tsx:55-58 - useSession and useActiveMember run before the if (!isMobile) return null at :60, so they subscribe on desktop where nothing renders. They share the sidebar's cache (navigation-sidebar.tsx:569) so there is likely no extra request, but worth a look.

With the team

• What happens to Source, Duration, Network and Gas below md - hidden, stacked into a card, or folded into a secondary line. The inverted modifier sharpens this: today's code loses the values and keeps the pan, so neither half of the tradeoff has actually been tried. I'm weighing a stacked card against a secondary line under the run name; the cost of the card is a second layout to maintain, the cost of the line is that gas and network get cramped. I'll come back with a verdict.

• Whether the nav change and the runs-table change should be one PR. You offered to split and I have not answered. They pass the split test - each ships and is correct with the other reverted - and the decision above only touches the table. I'll settle it with the above.

Verdict

Changes requested - the min-w modifier is on the wrong side, so the mobile pan the PR exists to remove is still there.

The adminOnly fix is right and I checked the predicate against the sidebar rather than reading the diff alone. Colours are all tokens, so both themes are safe. Accessibility is fine: aria-label on the trigger and the nav, aria-current="page", and 44px targets via h-11. The refactor preserved every flag - staging's pre-PR list has the same ids, the same requireAuth, and the same single ownerOnly on held-payments; adminOnly was already unused there, so the sidebar's "e.g. org Activity" comment at :942 is pre-existing stale rather than something you introduced.

Screenshots

Five are attached and I checked them against the ask. Covered: sheet open at 375px in light and dark, and member versus owner. Not covered, and the label stays on for these:

• The runs table at any width. There are no table screenshots at all, and that is the half carrying both the open decision and the blocker above. A 375px shot of the table would have caught the inverted min-w immediately.
• Sheet closed in dark.
• 04-signedout-light-open.png is byte-identical to 01-member-light-open.png - same sha256, relabelled. Member and signed-out do render the same item set, so this is not a fabrication, but it evidences nothing beyond 01, and the actual signed-out behaviour (tap, then the auth prompt at mobile-nav-sheet.tsx:76-78) is not shown.

One note on fidelity, not a request: these are a capture harness rather than the app - blank page body, placeholder org, a debug /analytics string in the corner - which you disclosed. The sheet's own rendering is credible from them; the sheet over real content is not shown, and neither is the loading skeleton row at runs-table.tsx:427-439 or the colSpan={8} empty state at :454, both of which this diff touches.

[subheeksh5599]

Addressed this round (d8c0a925c), rebased onto current staging so the compare is the real increment:

Blocker - inverted min-w. min-w-[700px] w-full text-left md:min-w-0 was exactly backwards: the 700px floor applied at every width and only got cancelled at md+, so the pan the PR exists to remove was still in force on the phone. Now w-full text-left md:min-w-[700px]: below 768px the table sizes to its four visible columns (no pan), at md+ the 700px floor returns as staging had it.

Mechanical:

• Trailing <td /> in StepLogRow and the skeleton row is now hidden md:table-cell, so below md the expanded rows render exactly the colSpan=4 cells over four visible columns (no stray fifth slot).
• MobileNavSheet now gates on a mounted state (set in an effect) alongside isMobile, so the first paint does not decide - no trigger pop-in after hydration, matching the sidebar's hasMounted guard. Hooks stay above the conditional return.
• ICONS is keyed by a MobileReachableNavItemId union (hub/workflows/analytics/earnings/held-payments/activity/settings) instead of Exclude<NavItemId, "address-book">, so a future desktop-only flyout added to NAV_ITEMS_DATA does not force an icon on a surface it cannot render on. Comment documents the derivation rule to keep in sync.
• The admin parity test now pins a fixture item with adminOnly: true and asserts the filter hides it from members and shows it to admins - previously adminOnlyIds was empty so every loop was a no-op. The carried-flag assertion against NAV_ITEMS_DATA is kept.

16/16 mobile-nav tests pass, biome and type-check clean.

On the two open questions (runs-table below-md layout, nav+runs-table split): both are yours to settle - happy to split the PR or take direction on the stacked-card vs secondary-line question.

[suisuss]

The blocker is fixed: runs-table.tsx:677 is now w-full text-left md:min-w-[700px], so below 768px there is no floor and the pan is gone, and at 768px and up the floor returns exactly as staging had it. The colSpan mismatch is fixed too - :405 and :439 are now hidden md:table-cell, giving four slots against four visible headers below md and eight against eight above. The admin parity test at tests/unit/mobile-nav-sheet.test.ts:139-145 now pins a fixture adminOnly item and exercises the real branch in both directions.

Two carried over: the hooks at mobile-nav-sheet.tsx:56-60 still run above the early return - not fixable in place, since hooks cannot be conditional, so it needs a wrapper component if it is worth doing at all. And the mounted guard at :57,63-70 now matches the sidebar, but it is functionally redundant: useIsMobile already starts undefined and coerces to false (hooks/use-mobile.ts:6), and both effects land after the same first commit, so the trigger still appears on the second render and the pop-in is unchanged. My "skeleton" wording last round was wrong - navigation-sidebar.tsx:744 returns null under that guard, same as yours.

Blocking

• nav-items-data.ts:45-53 with mobile-nav-sheet.tsx:42,115 - MobileReachableNavItemId is a second hand-written union decoupled from the data, and its own comment says "keep in sync" with nothing enforcing it. -> Add a routable destination to NAV_ITEMS_DATA: NavItemId forces a compile error, desktop NAV_ICONS at navigation-sidebar.tsx:540 forces an icon, but MobileReachableNavItemId is untouched so ICONS at :42 is already satisfied, MOBILE_NAV_ITEMS includes the item anyway via the href ?? mobileHref derivation, and :115 falls through to Globe. Desktop fails closed, mobile fails open - the same asymmetry as the adminOnly bug from the earlier round. -> Revert to Exclude<NavItemId, "address-book">.

  This one is on me. I raised the Exclude type as a minor nit, you acted on it, and the replacement reintroduced the silent Globe the type existed to prevent. A spurious compile error on a future desktop-only flyout is a much cheaper failure than a wrong icon shipping silently. Typing MobileNavItem.id as NavItemId rather than string would also drop the cast at :115 and make the lookup checked.

Mechanical - actionable as-is

• tests/unit/mobile-nav-sheet.test.ts:132-136 - dataAdminOnlyIds is recomputed inside the for (const item of MOBILE_NAV_ITEMS) loop; it was hoisted before.

With the team

• What happens to Source, Duration, Network and Gas below md - hidden, a stacked card, or a secondary line under the run name. Now that the pan is actually gone, this is the only thing gating the runs-table half. I'll come back with a verdict.

• Whether the nav and runs-table halves ship as one PR. You have offered twice and I have not answered; I'll settle it with the above.

Verdict

Changes requested - the icon type change fails open on a future routable destination.

Screenshots

requested-evidence stays on. No new media this round - the five PNGs from 09-04 are byte-for-byte unchanged, and 04-signedout-light-open.png is still byte-identical to 01-member-light-open.png.

The gap is sharper now than last round: the two fixes above, :677 and :405/:439, are exactly what a 375px capture of the runs table would confirm, and both are currently verified only by reading CSS. Whether Name truncates sanely in a four-column layout with no width floor is unverified. Attach the runs table at 375px and at 768px, collapsed and expanded. The nav half's evidence is adequate; the analytics half has none.

[subheeksh5599]

Round 4 addressed (338c505fc, rebased onto current staging):

Blocking - the icon type failed open. Reverted to Exclude<NavItemId, "address-book"> and deleted the hand-written MobileReachableNavItemId union entirely. You were right that it was a second union decoupled from the data with nothing enforcing sync, and that desktop failing closed while mobile fell through to Globe was the same asymmetry as the adminOnly bug. The compile error on a future desktop-only flyout is the cheaper failure by far.

Checked lookup. MobileNavItem.id is now typed NavItemId rather than string, so the derivation itself is checked at the source. The lookup keeps the as keyof typeof ICONS cast - with the Exclude keying, indexing by the full NavItemId union cannot type-check (TS cannot narrow that visible items exclude address-book), so the cast is what keeps the exhaustive guarantee while the ?? Globe remains as the runtime safety.

Mechanical. dataAdminOnlyIds hoisted out of the loop in the admin parity test.

16/16 mobile-nav tests pass; biome and type-check clean.