Repository navigation
Conversation
About the
|
joelorzet
left a comment
There was a problem hiding this comment.
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:
adminOnlyis declared onMobileNavItemand filtered on invisibleMobileNavItems, but no item sets it. Either use it or drop it.useActiveMember()returnsisLoadingand the sheet ignores it.isOwneris 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
lgbreakpoint.useIsMobileis 768px, which ismd. 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.
|
Addressed all the points. Test no longer reimplements the function. Parity with the sidebar. The two lists can't share one source without restructuring the desktop sidebar's
Description fixed. The PR body now says 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
left a comment
There was a problem hiding this comment.
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.
|
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
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
left a comment
There was a problem hiding this comment.
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-40withnav-items-data.ts:33-adminOnlywas 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, theflatMapcopies onlyid/label/href/requireAuth/ownerOnly, andvisibleMobileNavItems(:60) filters onownerOnlyalone, which leavesNavAccess.isAdmin(:51) unread. The desktop sidebar does enforce it (navigation-sidebar.tsx:896). Add an admin-only destination toNAV_ITEMS_DATAand 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 mentionsadminOnly. Carry the field intoMobileNavItem, propagate it in theflatMap, filter on it, and assert it the wayownerOnlyis 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 samemd:table-cellat:389,:392and:395, and Source has no expanded-row representation at all. The wrapper keepsoverflow-x-autobutmin-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-ICONSisRecord<string, ...>with a?? Globefallback 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- theas stringcast; typeSETTINGS_NAV_ITEM_DATAasNavItemData & { 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 ahasMountedskeleton.components/analytics/runs-table.tsx:369-405- below md thecolSpan={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-mutedandtext-muted-foregroundsurfaces 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
isLoadingpattern to the desktop sidebar, and that went unanswered for two rounds - my fault. Not in this PR; the sidebar gates onhasMountedand not onisLoading, 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.
|
Taken, with one addition that the measurements forced (
Measured at five widths, before and after, same long-name fixture: The 768px pan is unchanged and is not this change: that is the Two things this settles that I had wrong.
If you would rather keep desktop byte-identical and only truncate below 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 Still holding on |
|
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
left a comment
There was a problem hiding this comment.
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. Andwidth: 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 tomax-md:.
Mechanical - actionable as-is
-
runs-table.tsx:547-title={runName}is the only path to the full name, andtitleproduces no tooltip on touch. The external-link anchor at:552-561isopacity-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, aretentionCutoffprop threaded throughRunsTableintoExpandableRunRowandExpandedStepRows, andnetworkExpired/gasExpiredconditional titles with em-dash fallbacks on the Network and Gas cells. Those are the same<td>s this branch rewrites tohidden md:table-cell, so the next rebase is a hand-merge inside the conflict zone and those retention titles have to survive it. Sitting onholdwidens that gap, which is an argument for resolving themobile-stagingquestion 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
titletooltip 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-stagingstill 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).
…tion, scope mobile icon set
…NavItem.id as NavItemId
… 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.
6a4e133 to
f953e8e
Compare
|
Round 2 addressed and pushed as The retention work survived the merge. Blocking: the scoping. All three tokens are scoped now: 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. 1440px, ordinary names, staging then head: 1024px with the long name: 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 Mechanical: the non-hover disclosure. Below md the full name is now rendered in the expanded region, with 375px collapsed, the table filling the scroller with no pan: 375px expanded, showing the disclosure row: 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' 16 nav tests pass and |
What this changes
Previously raised
Does it match the descriptionMatches. The one unscoped remainder of the mobile fix is Mechanical - actionable as-is
With the team
VerdictComment only - every item from the last round lands, and what is left is mine: |
`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.
|
Mechanical addressed, pushed as
Confirmed and fixed: the disclosure row's span is now Measurements. The A/B is one variable on one page - the build with So the fix removes the mid-word breaks and leaves the layout heights alone. Reproduced from the fixed build as well: Captures, each from a build whose source carries that class (not a DOM edit):
Checks: the two unit tests that import this module ( The two open items in your |





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
Verification