Skip to content

fix: build the CSS-visible layout for the Assets table between 600–767px (WA-3437) - #8584

Open
Guilherme Fekete Endres (gfendres) wants to merge 2 commits into
devfrom
claude/bold-babbage-qo1hbz
Open

fix: build the CSS-visible layout for the Assets table between 600–767px (WA-3437)#8584
Guilherme Fekete Endres (gfendres) wants to merge 2 commits into
devfrom
claude/bold-babbage-qo1hbz

Conversation

@gfendres

Copy link
Copy Markdown
Collaborator

🤖 This pull request was produced entirely by an automated Claude run. A human wrote the ticket below. The research, the plan, the code, the tests, and this description all came from the automated run — no human was in the loop before this PR was opened. Please review it exactly like you would a new colleague's first pull request: check the claims, don't assume anything was double-checked by a person first.

Ticket

WA-3437 — Assets list renders blank between 600–767px viewport width

What it solves

Resolves: WA-3437

Between 600px and 767px viewport width, the Assets page token list rendered completely blank. The "Total assets value" total still showed, so it read as "still loading" rather than an obvious bug — people would wait or resize rather than report it. This is reachable on an ordinary laptop at 150–175% browser zoom, a half-split window, or common tablet widths (iPad mini portrait, Z Fold unfolded) — no unusual device required.

How this PR fixes it

AssetsTable decided which layout to build (useIsBelowSm(), flips at 600px) independently from the CSS that decided which layout to show (@media (max-width: 767.98px), flips at 768px). In the 168px gap between them, the desktop table was built and then hidden by CSS, and the mobile list was never built — nothing was left to show.

The fix makes both sides agree at the same threshold: swap useIsBelowSm() for useIsMobile() (shadcn's stock 768px hook), which is already the hook this codebase pairs with this exact 767.98px CSS convention elsewhere (PaginatedDataTable). One import, one call site.

flowchart TB
    subgraph Before["Before — two thresholds disagree"]
        A1["viewport width"] --> B1{"useIsBelowSm()<br/>flips at 600px"}
        B1 -->|"< 600px"| C1["build mobile list"]
        B1 -->|"≥ 600px"| D1["build desktop table"]
        D1 --> E1{"CSS @media<br/>flips at 768px"}
        E1 -->|"600–767px"| F1["❌ desktop table hidden by CSS<br/>nothing else was built — blank"]
        E1 -->|"≥ 768px"| G1["desktop table shown"]
        C1 --> H1["mobile list shown (CSS never hides it below 768px)"]
    end

    subgraph After["After — one threshold, both sides"]
        A2["viewport width"] --> B2{"useIsMobile()<br/>flips at 768px"}
        B2 -->|"< 768px"| C2["build mobile list"] --> D2["CSS shows it (< 768px) ✅"]
        B2 -->|"≥ 768px"| E2["build desktop table"] --> F2["CSS shows it (≥ 768px) ✅"]
    end
Loading

How to test it

  1. Check out this branch, yarn install, yarn workspace @safe-global/web dev.
  2. Open a Safe that holds tokens (an empty Safe shows the "Add funds" CTA, which sits outside the affected area — it isn't useful for this repro).
  3. Go to Assets.
  4. Resize the browser window to any width between 600px and 767px (or zoom to ~150–175% on a 1280px-wide window).
  5. Before this fix: the area below "Total assets value" is completely blank. After this fix: the stacked mobile token list renders.
  6. Resize across 768px and 600px in both directions to confirm the desktop table and mobile list still show correctly outside the gap (no regression at the boundaries).
  7. Automated: yarn workspace @safe-global/web test AssetsTable — includes a new regression test (breakpoint gap regression (AUD-48)) that fails against the pre-fix code and passes against this fix.

Affected flows

  • Viewing the Assets/Balances token list at 600–767px viewport width (the primary bug: blank list → visible list).
  • Viewing the Assets/Balances token list below 600px and at/above 768px (must render unchanged — verified by existing tests, both hooks agree in those ranges).
  • Loading skeleton for the Assets list at 600–767px (was also invisible in this gap since it's gated by the same flag; now shows the mobile skeleton correctly, as a side effect of the same fix).

Blast radius

  • Touched: apps/web/src/components/balances/AssetsTable/index.tsx — one hook swap (useIsBelowSmuseIsMobile), no other logic changed.
  • Not touched, not affected: useIsBelowSm/useMediaQuery.ts itself — still used as-is by SafeListItem, ModalDialog, SrcEthHashInfo, BreadcrumbItem, BlockedAddress.
  • Consumers of AssetsTable (pages/balances/index.tsx, features/assets/components/AssetsList/index.tsx, components/dashboard/Assets/index.tsx) all inherit the fix automatically since they render the same component — no per-consumer changes needed.
  • Mobile app (apps/mobile): untouched. This is a web-only CSS/JS breakpoint mismatch; no shared-package (packages/**) code is involved.

Risks / not checked

  • Did not verify on the native mobile app — not applicable, this bug and fix are web-only.
  • Did not capture a live-browser screenshot beyond Storybook (see below) — no funded, deployed Safe with real wallet access was available in this run to screenshot the actual /balances route; the Storybook story uses the same production AssetsTable component with realistic MSW-mocked balance data, which reproduces the bug and fix faithfully.
  • Did not fix the related AUD-1 finding from the same security review (the Argos visual-regression gate's pull_request trigger is commented out in .github/workflows/web-argos-storybook.yml) — that's a separate CI/workflow change, out of scope for this ticket, and called out here rather than silently left or folded into this PR's scope.
  • Did not re-derive whether useIsMobile's effect-based mount behavior (assumes desktop for one frame before its effect runs, vs. useIsBelowSm's hydration-safe useSyncExternalStore) causes a visible flash in production — this trade-off already exists today in PaginatedDataTable, which uses the same hook for the same kind of table/list swap, so it isn't a new risk introduced by this PR.

Visual summary

Captured via Storybook (Components/Balances/AssetsTableDefault, real MSW-mocked balance data), at a 700px × 900px viewport — squarely inside the 600–767px gap the ticket describes.

Before After
Before: blank list at 700px After: token list renders at 700px
Completely blank below the header — no error, no skeleton, no empty state. The mobile stacked list renders correctly.

What to review closely

  1. The hook swap itself (apps/web/src/components/balances/AssetsTable/index.tsx, the useIsMobile() line) — this is the entire behavioral change. Confirm you agree useIsMobile() (768px) is the right authoritative threshold rather than moving the CSS down to 599.95px (the ticket's own analysis rejects that alternative, since the desktop table isn't designed for a 600px column layout).
  2. The new regression test (AssetsTable/__tests__/AssetsTable.test.tsx, breakpoint gap regression (AUD-48) describe block) — it mocks useIsMobile() directly (matching the existing test convention from PaginatedDataTable) rather than simulating a real viewport width; worth double-checking this actually would have failed pre-fix (it does — verified by hand before writing this PR).
  3. AUD-1 (Argos visual-regression gate disabled) — flagged above as out of scope; worth a follow-up ticket since this exact class of bug is what that gate would have caught automatically.

How I got here

  • Read the ticket in full, including the linked PR 8040 Security Review (Notion) attachment and the dev repro link.
  • Read the three cited files directly (AssetsTable/index.tsx, useMediaQuery.ts, styles.module.css) against current dev HEAD — byte-identical to what the ticket describes, confirming the root cause first-hand rather than trusting the ticket's claim alone.
  • Searched for an existing precedent for pairing a JS hook with this exact CSS threshold, and found PaginatedDataTable already does exactly this with useIsMobile() — that became the fix, rather than inventing a new hook or moving the CSS.
  • Checked jest.setup.js to confirm window.matchMedia defaults to "desktop" in tests, so switching hooks wouldn't silently change any existing test's outcome.
  • Did not have Datadog/Mixpanel signal to add — this is a pure layout/CSS bug with no server-side or event trail; noting "checked, nothing relevant" rather than skipping the check silently would apply, but for a rendering-only bug like this those tools have nothing to add. No dashboard, monitor, or Mixpanel report was created or modified.

Trade-offs

  • Chose "make the JS threshold match the CSS threshold" over "make the CSS threshold match the JS threshold" — the ticket's own analysis already covers why: moving the CSS down to 599.95px would render the desktop table's fixed-percentage columns in a 600px-wide space it wasn't designed for.
  • Chose the existing useIsMobile() hook (effect-based, one-frame-late on mount) over introducing a new useSyncExternalStore-based 767.98px query in useMediaQuery.ts (hydration-safe, no flash). The latter would be marginally more correct in isolation, but it would be a new, one-off pattern; useIsMobile() is already the established, tested convention for this exact desktop/mobile table split elsewhere in the codebase, so consistency won over a marginal improvement.

Checklist

  • I've tested the branch on mobile 📱 — not applicable, web-only fix.
  • I've documented how it affects the analytics (if at all) 📊 — not applicable, no analytics impact.
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻
  • I've listed affected flows and blast radius, and named what I did not verify 🎯

CLA signature

With the submission of this Pull Request, I confirm that I have read and agree to the terms of the Contributor License Agreement.


Generated by Claude Code

useIsBelowSm() (600px) picked the JS branch while styles.module.css's
media query (767.98px) picked which one CSS shows, so nothing was built
for CSS to reveal between 600 and 767px — the token list rendered blank.

Switch to useIsMobile() (768px), the hook already paired with this same
CSS threshold elsewhere in the codebase (PaginatedDataTable), so the
built branch always matches the visible one.

WA-3437
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

📦 Next.js Bundle Analysis for @safe-global/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

One Page Changed Size

The following page changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/balances 25.52 KB (-1 B) 1.55 MB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Coverage report for apps/web

St.
Category Percentage Covered / Total
🟢 Statements
85.1% (-0.06% 🔻)
35315/41496
🟡 Branches
70.12% (-0.18% 🔻)
11549/16470
🟡 Functions
73.5% (-0.04% 🔻)
5390/7333
🟢 Lines
86.35% (-0.05% 🔻)
31602/36597
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / UpsertProposer.tsx
82.05% 60.87% 60% 83.04%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / utils.ts
77.68% (-3.57% 🔻)
62.32% (-2.9% 🔻)
72.73% (-4.55% 🔻)
79.17% (-3.13% 🔻)
🟢
... / constants.ts
58.33% (-8.33% 🔻)
100% 100% 100%
🟢
... / index.ts
81.25% (-6.25% 🔻)
100% 100% 100%
🟢
... / useAllAddressBooks.ts
89.47% (-1.05% 🔻)
70.37%
80% (-5% 🔻)
93.59%
🟢
... / index.ts
58.33% (-5.95% 🔻)
100% 100% 100%
🟡
... / EditProposerDialog.tsx
56.52% (-8.18% 🔻)
0% 0%
61.9% (-6.85% 🔻)
🟢
... / useSubmitDelegation.ts
96.97% (-0.09% 🔻)
85.71% (+5.71% 🔼)
100%
96.88% (-0.09% 🔻)
🟡
... / useTransactionType.tsx
66.07% (-33.93% 🔻)
41.51% (-54.32% 🔻)
100%
64.81% (-35.19% 🔻)
🟢
... / index.tsx
88.46% (-0.43% 🔻)
33.33% 66.67%
87.5% (-0.5% 🔻)
🟢
... / index.tsx
86.96% (-0.28% 🔻)
56.25% (-8.04% 🔻)
33.33%
92.86% (-0.17% 🔻)
🟡
... / AddManually.tsx
62.22% (-6.67% 🔻)
33.33%
11.11% (-33.33% 🔻)
64.29% (-4.76% 🔻)

Test suite run success

7747 tests passing in 884 suites.

Report generated by 🧪jest coverage report action from 9288b2f

Review feedback: the five-line explainer restated what the test names and
assertions already say. Keep one line for the CSS/JS threshold pairing,
drop the rest, and fix the stale AUD-48 ticket reference to WA-3437.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TpCpfmjRvCNigwGpUQxwK7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants