Skip to content

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

Merged
suisuss merged 11 commits into
KeeperHub:mobile-stagingfrom
subheeksh5599:feat/2295-mobile-monitoring
Sep 15, 2026
Merged

suisuss merged 11 commits into
KeeperHub:mobile-stagingfrom
subheeksh5599:feat/2295-mobile-monitoring

Conversation

@subheeksh5599

@subheeksh5599 subheeksh5599 commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

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

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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 joelorzet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

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 joelorzet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

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 suisuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@suisuss suisuss added decision-needed Blocked on a maintainer decision, not on the contributor requested-evidence Screenshots or video requested from the contributor labels Sep 4, 2026
@subheeksh5599

Copy link
Copy Markdown
Contributor Author

Taken, with one addition that the measurements forced (6a4e13326).

truncate on the name plus min-w-0 on its flex parent, as you asked, plus w-full max-w-0 on the name cell, and the full name in a title attribute. The cell constraint is the part I did not expect to need, so here is why it is there.

truncate on its own reintroduces the sideways pan. nowrap raises the cell's min-content width to the entire name, and because the table is auto-layout at w-full, its preferred width grows with it: 838px of table inside a 261px scroll container at 375, and 1123px inside 910 at 1024. Both pan. w-full max-w-0 on the cell keeps the column sized by the table instead, so the span is what overflows and ellipsizes.

Measured at five widths, before and after, same long-name fixture:

width   table/scroller        pans            cell          name lines   row
375     261 / 261             no              104 -> 108    12 -> 1      265 -> 49
700     586 / 586             no              429 -> 433     2 -> 1       65 -> 49
768     700 / 654             yes -> yes      258 -> 262     3 -> 1       85 -> 49
1024    910 / 910             no              468 -> 472     2 -> 1       65 -> 49
1440    910 / 910             no              468 -> 472     2 -> 1       65 -> 49

The 768px pan is unchanged and is not this change: that is the md:min-w-[700px] floor in a 654px container, exactly as staging has it.

Two things this settles that I had wrong.

  1. It is not desktop-identical. I said truncate only binds when the cell is narrower than the text and that the floor leaves Name room at md and above. At 1024 the cell is 468px and the name is 653px of text, so it binds there too: a 105-character name now ellipsizes instead of wrapping to a second line, and the row is 49px instead of 65px. Uniform rows with the full text in the title is defensible, but it is a visible desktop change and it is yours to accept or reject.
  2. The max-w-0 is load-bearing, not cosmetic. Without it the 375px pan comes back, which is the regression the previous round was about.

If you would rather keep desktop byte-identical and only truncate below md:, that is max-w-0 scoped to a max-md: variant, one token, and I will measure it the same way.

On the method, since it bit me once: the device-metrics override does not survive a navigation, so an override set before loading silently leaves the page at the window size. The numbers above re-apply it after the load and read window.innerWidth back to confirm. My first pass reported a 653px name cell at "375px" for that reason.

Still holding on mobile-staging, as you said. Nothing here changes until it exists.

@suisuss suisuss added the requested-evidence Screenshots or video requested from the contributor label Sep 11, 2026
@suisuss

suisuss commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

We are going to need some screenshots of progress @subheeksh5599. We will probably merge this into a different branch on our remote and get you to work from there for the rest of the mobile responsiveness we are looking for. We want to make this a cohesive, comprehensive update. So the plan is to work from that branch together with whatever QA concerns the team raises.

@suisuss suisuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instruction I gave you was not satisfiable, and you were right to say so rather than quietly ship something that looked like it. I said truncate plus min-w-0 on the flex parent, and that it would be desktop-identical. Those two cannot both hold: truncate sets white-space: nowrap, which raises the cell's min-content width, and nothing scopes that to a breakpoint. Your measurements are the proof - 1123px of table inside a 910px scroller at 1024px with the fix exactly as I specified it. I should have read the cascade before writing the instruction.

Two corrections to what I told you, both from reading the code at head:

min-w-0 at :541 is on the element I named and is doing nothing. That div is a block-level flex container inside a <td>, not itself a flex item, so its min-width: auto already resolves to 0. What makes the truncation work is truncate zeroing the span's automatic minimum size, and max-w-0 with w-full on the <td> at :540 stopping the column growing to fit. You found the third token yourself; it is the load-bearing one.

And the fallback you proposed is one token short of working. Scoping only max-w-0 to max-md: leaves truncate unscoped, so white-space: nowrap still applies at md+ and you get the 1123px pan back at desktop. Keeping desktop byte-identical needs all three scoped - max-md:w-full max-md:max-w-0 on the td and max-md:truncate on the span.

Blocking

  • components/analytics/runs-table.tsx:540,544 - all three tokens are unprefixed, so both changes reach desktop. Long names ellipsize where they wrapped, which you measured at 1024px as a 65px row becoming 49px. And width: 100% on one cell of an auto-layout table makes that column absorb all slack while the other seven fall back to max-content, where staging spread it across all eight. -> That second one is unmeasured by either of us: all five of your captures use the long-name fixture, where Name was already the elastic column, so the 468 to 472 delta does not characterise the common case. At 1440px with 15-to-30-character names I would expect Name to gain a large block of whitespace while Status, Source, Duration, Network, Gas and Time pull tight left. -> Scope all three to max-md:.

Mechanical - actionable as-is

  • runs-table.tsx:547 - title={runName} is the only path to the full name, and title produces no tooltip on touch. The external-link anchor at :552-561 is opacity-0 group-hover:opacity-100, also hover-gated, and expanding the row renders step names rather than the workflow name. So on the 375px target this change exists for, a long name goes from fully readable at 12 wrapped lines to unreadable past about fourteen characters. My phrase "the row's existing hover affordance or a title attribute" assumed a hover that is not there. If the columns stay hidden below md, the name needs a non-hover disclosure.

  • The branch is 217 commits behind staging, and this file has since taken the KEEP-1042 retention work - stepLogsExpired(), STEP_LOGS_EXPIRED_MESSAGE, a retentionCutoff prop threaded through RunsTable into ExpandableRunRow and ExpandedStepRows, and networkExpired/gasExpired conditional titles with em-dash fallbacks on the Network and Gas cells. Those are the same <td>s this branch rewrites to hidden md:table-cell, so the next rebase is a hand-merge inside the conflict zone and those retention titles have to survive it. Sitting on hold widens that gap, which is an argument for resolving the mobile-staging question sooner rather than later.

Screenshots

Three states nothing covers yet, and the first is the one that settles the blocker:

  • 1440px collapsed with ordinary short names, staging and head side by side. That is the unmeasured column-redistribution case.
  • 1024px collapsed with the 105-character name - the admitted desktop change, ellipsis against the two-line wrap.
  • A pointer hovering the truncated name, to show the title tooltip is a real escape hatch and whether it collides with the row-hover link icon appearing at the same moment.

With the team

  • Whether Source, Duration, Network and Gas should be hidden below md at all. Unchanged, and your long-name measurement was the input I wanted: even with them hidden, Name is the dominant variable at 375px. I am settling it with the rest of the mobile question.

  • mobile-staging still does not exist, so there is still nothing for you to retarget to. Mine, and this stays on hold until it is there.

Verdict

Changes requested on scoping the three tokens to max-md: - the fix works at 375px, and as written it also changes every desktop width, including one case neither of us has looked at.

Two gaps from #2295 (accepted, read-only monitoring scope):

1. Navigation: on a phone the entire sidebar (the only nav to /analytics,
   /activity, /earnings, /settings) returned null, so the monitoring
   surfaces were unreachable. Add a MobileNavSheet (hamburger + left Sheet)
   mounted in the persistent toolbar, visible only below the lg breakpoint,
   mirroring the sidebar's destinations with the same auth/owner gating
   (useSession + useActiveMember + openAuthPrompt). Reuses the existing
   useIsMobile + shadcn Sheet pattern per the issue.

2. Analytics runs table: the 700px-min, 7-column table forced sideways
   panning on a phone. Hide the secondary columns (Source, Duration,
   Network, Gas) below md so the essential status/time/name fit without
   panning; the full detail stays in the expandable per-run rows. Desktop
   is unchanged (hidden md:table-cell).
Split the MobileNavSheet's pure logic (visible items by access level,
active-route detection, tap action) into mobile-nav-items.ts so it is
testable without pulling the React/Sheet/Sentry tree into jsdom — the
repo's established pattern (settings-nav-search, use-persisted-nav-state
test pure helpers, not rendered components).

12 tests cover: member/admin/owner visibility (owner-only Held Payments
hidden for non-owners), no flyout/overlay-only entries leaking into the
mobile set, active-route matching incl. subroutes and the root edge, and
auth-prompt-vs-route decisions for signed-in / signed-out / anonymous
users on requireAuth destinations.
…bar parity tests

Review feedback (#2302):
- mobile-nav-items.ts now imports lib/is-anonymous's isAnonymousUser and
  decideMobileNavAction no longer takes an injected predicate, so the
  auth-prompt tests exercise the shipped code, not a local copy.
- adminOnly was declared and filtered but never set by any item; dropped.
- visibleMobileNavItems keeps the isOwner-only filter (ownerOnly).
- MobileNavSheet holds the owner-only destination until the active-member
  query resolves, so Held Payments no longer pops in after load.
- Tests add desktop-sidebar parity invariants (routable coverage,
  requireAuth gating, owner-only alignment). 15 tests total.
Second review pass (#2302). The parity tests hand-copied the sidebar's list,
so they compared mobile against a second copy rather than NAV_ITEMS - a
destination added to the sidebar would stay green on mobile with no failure.

- nav-items-data.ts is now the single source: NavItemData (id as a NavItemId
  literal union, label, href-or-null, mobileHref for the workflows
  flyout-to-page case, requireAuth, adminOnly/ownerOnly, actionItem), plus
  NAV_ITEMS_DATA, SETTINGS_NAV_ITEM_DATA and ACTION_ITEM_IDS. No icons, no
  React - tests can import it without the lucide runtime.
- navigation-sidebar.tsx imports the data and resolves icons locally via a
  NAV_ICONS map keyed by NavItemId, so a new destination without an icon is
  a compile error, not a render crash. Behaviour is unchanged: same items,
  same order, same action/flyout handling.
- mobile-nav-items.ts derives MOBILE_NAV_ITEMS from the same source (routable
  surface = href or mobileHref, plus settings), deleting its hand-copied list.
- The parity tests now assert against NAV_ITEMS_DATA itself: routable
  destinations must appear on mobile, and requireAuth/ownerOnly gating must
  match the source. A new destination either appears on mobile or fails.

Verified: 15/15 unit tests, typecheck clean (only pre-existing generated
module errors), biome clean, token-audit clean for these files.
… runs-table comment

Review feedback (#2302, suisuss):
- adminOnly 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 fail open on mobile (shown to every signed-in user) while the
  desktop sidebar hid it. Carried through and filtered; parity test added
  (admin-only items hidden from members, shown to admins, aligned with source).
- mobile ICONS is now Record<Exclude<NavItemId, 'address-book'>, LucideIcon>
  instead of Record<string, ...>, so a destination added to NAV_ITEMS_DATA
  without an icon is a compile error on mobile too, not a silent Globe.
- SETTINGS_NAV_ITEM_DATA typed as NavItemData & { href: string }; the as
  string cast in the mobile derivation is gone.
- runs-table comment corrected: it claimed the hidden secondary columns
  remain reachable in the expanded per-run rows; they do not (same md
  hiding). The comment now states the real behaviour - hidden below md per
  the mobile issue, not re-exposed on a phone, desktop unaffected.
- Branch rebased onto current upstream staging (was 20 commits behind).
… row

The name cell had no truncation, so a long workflow name wrapped: at 375px a
105-character name took 12 lines and made the row 265px tall against 49px for its
siblings. Take the treatment from the review: truncate on the name and min-w-0 on
its flex parent, with the full name in a title attribute.

Adding truncate alone is not enough and is not desktop-identical, which the
measurements show. nowrap raises the cell's min-content width to the whole name,
so the table's preferred width grows with it: 838px of table inside a 261px
scroll container at 375, and 1123px inside 910 at 1024, both of which pan
sideways where the branch currently does not. Constraining the cell itself
(w-full max-w-0) keeps the column sized by the table, so the span is the thing
that overflows and ellipsizes:

  width   table/scroller   pans    cell   name lines   row
  375     261 / 261        no      104 -> 108   12 -> 1    265 -> 49
  700     586 / 586        no      429 -> 433    2 -> 1     65 -> 49
  768     700 / 654        yes     258 -> 262    3 -> 1     85 -> 49
  1024    910 / 910        no      468 -> 472    2 -> 1     65 -> 49
  1440    910 / 910        no      468 -> 472    2 -> 1     65 -> 49

Measured with the long-name fixture at each width after re-applying the device
metrics override post-navigation (set before a navigation, it does not survive
the reload and the measurement silently reports the 1440px layout). The 768px pan
is unchanged and pre-existing: it is the md:min-w-[700px] floor in a 654px
container. The reachable cost is desktop: a 105-character name now ellipsizes
rather than wrapping to a second line, so rows are uniform and the full text sits
in the title.
… full name

Three corrections from the review.

All three tokens are now scoped to max-md:. max-md:w-full and max-md:max-w-0 on
the td and max-md:truncate on the span, so desktop is left exactly as staging has
it. The review was right that the unscoped form reached every width: width:100%
on one cell of an auto-layout table lets that column absorb the slack the other
seven shared, and that is the case neither of us had measured.

min-w-0 on the flex wrapper is gone. It is a block-level flex container inside a
td rather than a flex item, so its min-width:auto already resolved to 0 and the
declaration was doing nothing. What carries the truncation is max-w-0 with w-full
on the td, plus truncate on the span.

Below md the name column ellipsizes and title is the only other copy of the
string, which a touch device never shows. Expanding the row is a click, so the
full name is now rendered in the expanded region below md, with break-all so a
long unbroken name cannot overflow it.

Also rebased onto staging, which is what the review warned about: the retention
work landed in these same cells. stepLogsExpired, STEP_LOGS_EXPIRED_MESSAGE, the
retentionCutoff prop and the networkExpired/gasExpired titles with their em dash
fallbacks all survive, on the same td elements that now carry the mobile classes.
@subheeksh5599

Copy link
Copy Markdown
Contributor Author

Round 2 addressed and pushed as f953e8e73. This is a forced update, so the SHAs you cited are gone; I rebased onto staging rather than layering on the old base, because the retention work is in exactly the cells this PR rewrites and you warned the gap would widen. The mobile-staging question is still open and still yours, so this is a rebase, not a retarget.

The retention work survived the merge. stepLogsExpired, STEP_LOGS_EXPIRED_MESSAGE, the retentionCutoff prop and the networkExpired / gasExpired titles with their em dash fallbacks are all present, on the same <td> elements that now carry the mobile classes. The only conflict was those two cells and both sides are kept.

Blocking: the scoping. All three tokens are scoped now: max-md:w-full max-md:max-w-0 on the td and max-md:truncate on the span. min-w-0 is gone, on your reading: it is a block-level flex container inside a td rather than a flex item, so its min-width: auto already resolved to 0. You were right that the unscoped form reached every width, and right that the case neither of us had measured is the ordinary-name one.

That case is now measured, and the result is stronger than "unchanged". At 1440px with ordinary short names, and at 1024px with the 105 character name, the two builds produce byte-identical captures: the same sha256 for each pair.

6dd05eb50a3b1130295d51c3b025fb485666f6053743a46d32ea584689bb2f7a  head-1440-ordinary
6dd05eb50a3b1130295d51c3b025fb485666f6053743a46d32ea584689bb2f7a  staging-1440-ordinary
5a47bfce1f9ada6a9e127f5d77a1fe4b96794c681197dc94672a77be3cac17b7  head-1024-longname
5a47bfce1f9ada6a9e127f5d77a1fe4b96794c681197dc94672a77be3cac17b7  staging-1024-longname

1440px, ordinary names, staging then head:

staging 1440 ordinary

head 1440 ordinary

1024px with the long name:

head 1024 longname

Numbers behind them: table 910, name cell 468, name span 440, two rendered lines, row height 65, in both builds. The column redistribution you predicted does not happen.

The hover capture you asked for is moot at desktop. With desktop wrapping as staging does, nothing is truncated there for a tooltip to reveal. The disclosure problem moved to below md, where the column does ellipsize and title is the only other copy of the string.

Mechanical: the non-hover disclosure. Below md the full name is now rendered in the expanded region, with break-all so an unbroken name cannot overflow it. Expanding the row is a click, so a touch device can reach it.

375px collapsed, the table filling the scroller with no pan:

375 collapsed

375px expanded, showing the disclosure row:

375 expanded

Something you should see rather than take from me. In that last capture the table is 328px inside a 261px scroller, so the expanded state still pans 67px at 375. I attributed it before assuming: with my disclosure row removed from the DOM the table is still 328, and the widest cells are the expanded step rows' colSpan=4 name cells. So it is not the name fix, and it is not the disclosure. Staging pans far worse in the same state, 700px in 261, because its min-width floor applies below md. Same story at 768, where both builds pan 700 in 654: that is staging's floor, untouched by this PR. I have not fixed the expanded step rows, since that is not what the review asked for and the adjacent question about which columns belong below md is already with the team. Say the word and it is a separate change.

16 nav tests pass and pnpm type-check is clean.

@suisuss

suisuss commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

What this changes

components/analytics/runs-table.tsx only. The other six files are byte-identical across the rebase.

  • :579,583 - the three name-cell tokens are breakpoint-scoped now: max-md:w-full max-md:max-w-0 on the td, max-md:truncate on the span. At 768px and up neither element carries anything the diff adds.
  • :580 - min-w-0 is gone from the flex wrapper. The span still shrinks below its min-content, because truncate sets overflow: hidden and that zeroes a flex item's automatic minimum size.
  • :646-653 - a new md:hidden row inside the expanded region renders the full runName at colSpan={8}, matching the existing colSpan={8} at :480.
  • The base moved from 59b6090 to caa60d1. The KEEP-1042 retention work survived into the cells this branch rewrites: stepLogsExpired at :424, STEP_LOGS_EXPIRED_MESSAGE at :431, retentionCutoff threaded at :656 and :780, networkExpired/gasExpired at :528-529, and the em-dash fallbacks with their titles at :619-634 - on the same tds that carry hidden md:table-cell. Below md those two cells are display: none, so the retention signal on a phone comes from :480, which does render there.

Previously raised

  • All three tokens unprefixed, so the mobile fix reached desktop - addressed at :579,583; the 1440px ordinary-name pair and the 1024px long-name pair are byte-identical between staging and head, which I confirmed by downloading all four.
  • title is the only path to the full name and produces no tooltip on touch - addressed at :646-653.
  • The branch was 217 behind and the retention work sits in these cells - addressed; every retention symbol is present and on the right td, listed above.
  • 1440px collapsed, ordinary names, staging against head - supplied.
  • 1024px collapsed with the 105-character name - supplied; head and staging hash the same, so the desktop change is gone rather than photographed.
  • A pointer over the truncated name - retired by the fix. With max-md:truncate nothing truncates at md+, so there is no tooltip to capture. You were right to say so rather than stage one.

Does it match the description

Matches. The one unscoped remainder of the mobile fix is title={runName} at :586, which staging does not have: at md+ it puts a native tooltip on text that is fully visible. Below md with a pointer it is still the working escape hatch, so it earns the line.

Mechanical - actionable as-is

  • runs-table.tsx:651 - break-all breaks ordinary words mid-word, not just unbreakable ones -> in the 375px expanded capture "transactions" renders as to t / tions multisig, and "very" as v / ery long name -> wrap-anywhere, which this file already uses at :350, :485 and :492 for the same guard and only breaks when a word cannot fit.

With the team

  • Whether Source, Duration, Network and Gas should be hidden below md at all. I'm weighing the four-column table against a stacked card or a secondary line under the run name - the tradeoff is a second layout to maintain against gas and network getting cramped. I'm discussing it with the core team now and will come back with a verdict shortly.
  • Where the 67px expanded-state pan gets fixed. Your 375px expanded capture shows the new disclosure row clipped at the scroller edge, so the row reaches the name but not all of it; the cause is not the row - break-all gives it a one-character min-content contribution, so it cannot be the widest cell - it is the colSpan={4} step-name cells at :369. I'm weighing taking it here against holding it for the branch below - the tradeoff is reopening a clean diff against shipping a disclosure that is itself partly unreadable at 375. I'm discussing it with the core team now and will come back with a verdict shortly.

Verdict

Comment only - every item from the last round lands, and what is left is mine: mobile-staging still does not exist on the remote, so there is nothing here for you to act on.

`break-all` breaks ordinary words mid-word, so at 375px the expanded row
rendered `operations` and `very` split across lines - measured on the real
build: 2 of the row's 17 words broken. `wrap-anywhere` is the guard this file
already uses for the same purpose (`:350`, `:485`, `:492`); it breaks a word
only when it cannot fit, so the same row now breaks none, with the line count
and row height unchanged (3 lines, 77px).

Measured by flipping only that class in the live DOM on one build, so the
comparison is one variable on one page.
@subheeksh5599

Copy link
Copy Markdown
Contributor Author

Mechanical addressed, pushed as 45b06dd58.

runs-table.tsx:651 - break-all breaks ordinary words mid-word, not just unbreakable ones -> in the 375px expanded capture "transactions" renders as to t / tions multisig, and "very" as v / ery long name -> wrap-anywhere, which this file already uses at :350, :485 and :492 for the same guard and only breaks when a word cannot fit.

Confirmed and fixed: the disclosure row's span is now wrap-anywhere, matching the three existing uses.

Measurements. The A/B is one variable on one page - the build with break-all loaded at 375px with the row expanded, then only that class flipped in the live DOM, re-measured in place. Per-word breakage is measured by asking the layout how many line boxes each word occupies (Range.getClientRects() over each word's character offsets), so "broken" means a word spanning more than one line top:

                     break-all              wrap-anywhere
broken words         2 of 17 (operations,  0 of 17
                     very)                 (the 105-char name still breaks, as it must)
word-break           break-all             normal
overflow-wrap        normal                anywhere
span width           274px                 255px
lines                3                     3
row height           77px                  77px

So the fix removes the mid-word breaks and leaves the layout heights alone. Reproduced from the fixed build as well: overflowWrap: anywhere, wordBreak: normal, 0 broken words, 3 lines, 77px.

Captures, each from a build whose source carries that class (not a DOM edit):

Checks: the two unit tests that import this module (analytics-run-gas-display, analytics-runs-response) still pass, 12 cases; Biome clean on the file. Nothing asserts these classes, so the measurement above is the verification rather than a test - the repo has no React testing library for the component branch.

The two open items in your With the team section (whether Source/Duration/Network/Gas stay hidden below md, and where the 67px expanded pan gets fixed) are untouched here, as is the mobile-staging retarget - this is the mechanical item only.

This branch was previously deployed

1 inactive deployment
staging — 45b06dd5 Deployed Sep 15, 2026 by suisuss via e2e-tests / should-run #5416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approve Triage: reviewed and good - not a GitHub approval hold waiting on something else to go live

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Monitoring surfaces are not usable on a phone: analytics and run history are desktop-only

3 participants