Skip to content

feat(web): grouped look-alike UI across the account tables - #8354

Open
tongyaosafe wants to merge 22 commits into
devfrom
feat/address-poisoning-grouped-ui
Open

feat(web): grouped look-alike UI across the account tables#8354
tongyaosafe wants to merge 22 commits into
devfrom
feat/address-poisoning-grouped-ui

Conversation

@tongyaosafe

@tongyaosafe tongyaosafe commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

What it solves

Resolves: WA-2916

Stacked on #8363 (WA-2912).

Look-alike (address poisoning) warnings previously rendered as isolated per-row badges: a flagged row told you it resembled something, but not what, and visually similar accounts could sit far apart in a long list. This PR groups them.

How this PR fixes it

One clustering source (useSimilarityClusters), three presentation primitives on SafeAccountsTable:

  • Similarity band — cluster members are pulled contiguous (placed at their lead's sorted slot, vetted/pinned anchor first) and rendered as rounded cards inside an "Address poisoning warning" band with an explanatory Info tooltip.
  • Cross-list ⚠️ — when a cluster spans two rendered lists (onboarding's My accounts / Owned safe accounts), a single band can't box it, so each member gets an inline ⚠️ whose tooltip lists its peers (with copyable full addresses).
  • Confirm gate — selecting a flagged row still opens the confirmation dialog. In the Manage trusted Safes modal, pinned cluster members are treated as vetted anchors: they stay visible in the band but don't flag or prompt.

Wired into all four account-table surfaces: onboarding Select Safe accounts, Manage trusted Safes modal, workspace Safe accounts, and workspace Add accounts.

Supporting changes:

  • Cross-list presentation policy (bandGroupsForList, buildSimilarWarnings, SimilarWarning) lives in features/address-poisoning — one owner for the rule, decoupled from safe-item shapes.
  • Drag-and-drop in Manual mode preserves the user's stored order: a drop reports only draggable rows and the table weaves them back (weaveReorderedKeys, unit-tested) — the clusters' pin-to-top stays display-only and can't permanently rewrite the manual arrangement.
  • Shared useCopyToClipboard hook (copy state machine deduped from CopyAddressButton); orderGroupsBySimilarity extracted to its own module with tests.

How to test it

  1. Pin two Safes whose addresses share the first/last 4 hex chars → they render as one band, anchor first, on every account-table surface; hover the band's Info icon for the explanation.
  2. Add a third look-alike that is not pinned → in onboarding the pinned pair bands under My accounts, the owned one bands alone under Owned safe accounts, and all cross-list members carry the ⚠️ with peer tooltips.
  3. Select a flagged row → confirmation dialog; in the Manage trusted modal, pinned members select without prompting.
  4. Manual sort + drag: with a band present, drag an unrelated row, then dissolve the cluster (unpin a member) → previously-banded rows return to their original stored slots.
  5. Repeat 1 in dark mode (band surface/border adapt) and in Storybook (SafeAccountsTable similarity stories).

Affected flows

  • Any surface rendering SafeAccountsTable with similarity props: onboarding step 2, Manage trusted Safes modal, workspace Safe accounts, workspace Add accounts.
  • Manual-mode drag-and-drop persistence in those tables.

Blast radius

  • SafeAccountsTable family (index, ReorderableBody, SafeAccountTableRow, new SimilarityBand, tableStyles, orderGroupsBySimilarity) — consumed by the four surfaces above; the new similarity props are optional, so prop-less usages render unchanged.
  • features/address-poisoning gains pure presentation utils; detection engine, thresholds, and the ADDRESS_POISONING_PROTECTION flag gating are untouched.
  • SafeInfoDisplay gains an optional nameAdornment slot (existing badge consumers unaffected).
  • useTrustedSafesModal: flagged now excludes pinned anchors (modal-only policy); onboarding keeps flagging the full set.
  • New shared useCopyToClipboard + weaveReorderedKeys (unit-tested); CopyAddressButton behavior unchanged.
  • No RTK Query/API, persistence-schema, routing, or mobile impact.

Risks / not checked

  • Drag-and-drop × band interplay has unit coverage for the ordering math only — the dnd interaction itself was verified manually, not by automated tests.
  • Not tested on mobile (web-only surfaces).
  • Large accounts (hundreds of Safes) with many simultaneous clusters exercised only with synthetic Storybook data.
  • The modal's pinned-anchor exemption is a deliberate per-surface policy; if product wants pinned members to also confirm-gate there, that's a one-line follow-up.

Visual summary

flowchart LR
    A["useSimilarityClusters(all addresses)"] --> G["groupIdByAddress"]
    A --> F["flagged"]
    G --> O["orderGroupsBySimilarity\ncluster contiguous, anchor first"]
    O --> B["SimilarityBand\nheader + carded rows"]
    G --> X["bandGroupsForList / buildSimilarWarnings\n(features/address-poisoning)"]
    X --> W["inline ⚠️ + peers tooltip\n(cross-list clusters)"]
    F --> C["SimilarityConfirmDialog on select\n(modal: pinned anchors exempt)"]
Loading

Before:
Screenshot 2026-07-27 at 18 48 18
Screenshot 2026-07-27 at 18 48 38

After:
Screenshot 2026-07-27 at 18 50 35
Screenshot 2026-07-27 at 18 51 02

Checklist

  • I've tested the branch on mobile 📱 (N/A — web-only)
  • I've documented how it affects the analytics (if at all) 📊 (no analytics changes)
  • 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.

tongyaosafe and others added 2 commits July 23, 2026 14:06
…orybook)

Add a groupBySimilarity capability to the shared SafeAccountsTable: contiguous
rows sharing a cluster id render inside a light-yellow "Address poisoning
warning" band, each member as its own rounded yellow-bordered card, with a small
inline ⚠️ after the name for flagged look-alikes (replaces the old "High
similarity" pill). New props: similarityGroups (Map<addr, clusterId>);
SafeInfoDisplay gains nameAdornment. Styling lives in the Table sx (keyed off
data attributes) so it composes with the cell-level hover/separator machinery.
Storybook only — no real surface wired yet (Phase 2).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@tongyaosafe

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @tongyaosafe's task in 3m 5s —— View job


Claude PR Review

  • Gather context (read changed files)
  • Analyze code quality, bugs, performance, security, tests
  • Post review feedback

Focused on the new similarity-band / grouping code. Overall this is a clean, well-commented visual layer — most feedback is minor. A couple of items worth a second look:


1) ReorderableBody — dragging silently rewrites the persisted manual order (behavior change)

apps/web/src/features/myAccounts/components/SafeAccountsTable/ReorderableBody.tsx:86-93

const reordered = reorderByKey(draggableGroups, source.index, destination.index, (group) => group.parent.address)
onReorder([...pinnedGroups.map((group) => group.parent.address), ...reordered])

Any drag now commits pinned-first + rest as the new manual order. If a user had previously placed a normal Safe above one that later becomes part of a similarity cluster, the first drag reshuffles it, and if the cluster later dissolves (data change) the original order is gone. Consider either (a) not persisting the pin-to-top separation into the underlying order (only apply the pin visually, keep the stored order untouched), or (b) documenting this as intentional and confirming with product.

Also: pinned rows sit inside the same <Droppable> <TableBody> as the <Draggable> items but are not draggable themselves. Draggable's indices start at 0 for draggableGroups, so the mapping is correct, but this is subtle — worth an integration test to make sure drop-position hit-testing across the pin/drag boundary doesn't misbehave.

2) orderGroupsBySimilarity — dead fallback + double filter

apps/web/src/features/myAccounts/components/SafeAccountsTable/index.tsx:194-195

const members = membersByCluster.get(cluster) ?? [group]
result.push(...members.filter(isAnchor), ...members.filter((member) => !isAnchor(member)))
  • ?? [group] is unreachable — every group whose cluster we reach here was pushed into membersByCluster in the earlier loop. Drop it or add a comment explaining the defensive intent.
  • members.filter(...).filter(...) walks the same array twice per cluster. A single loop that pushes into anchors / flagged arrays is one pass and reads clearer.

3) SimilarityBandHeader — orphan Info icon

apps/web/src/features/myAccounts/components/SafeAccountsTable/SimilarityBand.tsx:18-20

The Info icon is aria-hidden, has no tooltip and no click handler. It implies "click for help" without delivering it. Either wire up the tooltip explaining what "Address poisoning" means, or drop the icon until the tooltip is implemented.

4) Missing unit tests for the two new pure functions

Per AGENTS.md: every code change should include tests. The story-only coverage is nice but doesn't lock behavior. Please add unit tests for:

  • orderGroupsBySimilarity — lead selection (first-seen vs anchor upgrade), anchor-first within band, non-clustered groups unchanged, empty/undefined map is no-op.
  • useOnboardingSafesownedSimilarityGroups — cross-list matches (owned + trusted) stay out of the band; only clusters with ≥2 owned members are banded.

5) Nits

  • ownedSimilarityGroups iterates ownedAddresses twice — one pass with a temp count map and a filter would be one loop over the source array (useOnboardingSafes.ts:48-58).
  • similarityGroups?.get(line.address.toLowerCase()) runs the .toLowerCase() twice per row in the band-header decision (index.tsx:463-465). If lists get large, precompute cluster ids alongside lines in the existing useMemo.
  • Band CSS in index.tsx:365-397 relies on emotion writing sx keys in insertion order to beat the hover pill via source order. It works with the current MUI/emotion setup but is fragile — consider slightly higher specificity for the hover rules instead, so the "put hover last" comment isn't load-bearing.
  • SimilarityWarningIcon uses aria-label; make sure the parent <svg> gets role="img" (lucide adds it via role="img" by default; just worth verifying in prod build) so screen readers announce it.

Nothing security-critical; the band is a rendering layer over existing data. Main asks are (1) the persisted-order side-effect and (2) unit tests for the two new pure helpers.
• Branch: feat/address-poisoning-grouped-ui

@github-actions

github-actions Bot commented Jul 24, 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. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 1.5 MB (🟡 +2.54 KB)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

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

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

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
/new-safe/load 9.48 KB (🟢 -40 B) 1.51 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 Jul 24, 2026

Copy link
Copy Markdown
Contributor

Coverage report for apps/web

St.
Category Percentage Covered / Total
🟢 Statements
83.63% (+0.01% 🔼)
32059/38333
🟡 Branches
67.33% (-0.04% 🔻)
10212/15168
🟡 Functions
70.93% (-0.04% 🔻)
4844/6829
🟢 Lines
84.93% (-0% 🔻)
28711/33806
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / useCopyToClipboard.ts
100% 100% 100% 100%
🟢
... / crossListClusters.ts
100% 100% 100% 100%
🔴
... / SimilarityBand.tsx
58.33% 16.67% 12.5% 57.14%
🟢
... / orderGroupsBySimilarity.ts
100% 93.75% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / CopyAddressButton.tsx
100% (+5.56% 🔼)
50% (-50% 🔻)
100% (+20% 🔼)
100%
🟢
... / index.tsx
84.21% (-2.52% 🔻)
87.76% (-0.79% 🔻)
75% (-5.65% 🔻)
84.21% (-0.97% 🔻)
🟢
... / SafeAccountTableRow.tsx
89.23%
80.21% (-1.31% 🔻)
76.92% 91.8%
🟡
... / ReorderableBody.tsx
66.67% (-7.14% 🔻)
42.11% (-23.28% 🔻)
52.17% (-12.11% 🔻)
67.92% (-8.55% 🔻)

Test suite run success

6641 tests passing in 738 suites.

Report generated by 🧪jest coverage report action from 630dde1

…act table sx

- Fix light-mode band regression: --band-surface/--band-border referenced
  themselves; restore the yellow-50/yellow-400 light values (dark keeps the
  warning-token override).
- Extract the ~90-line body-row sx into tableStyles.ts (bodyRowSx) with ROW/CELL
  selector-prefix constants to kill the repeated selector chains.
- Dedup the band-opening header into bandHeaderAt(), shared by the non-reorder
  body and the reorderable pinned block.
- Suppress the grey row divider inside a band so no line paints between cards.
- Export orderGroupsBySimilarity + add unit tests (interleaved / anchor lead /
  no-anchor / multi-cluster).
- Refresh stale "High similarity" / "Approach A" comments.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tongyaosafe tongyaosafe self-assigned this Jul 24, 2026
tongyaosafe and others added 4 commits July 27, 2026 12:50
…ng step 2 [WA-2912]

Detection already ran on the combined trusted+owned pool, but the result was
filtered to owned rows only, so a look-alike pair where the impostor is pinned
(both rows in My accounts) showed no warning at all. Surface every flagged row:
badge on trusted and owned tables alike, one security banner above the sections,
and the selection confirm-gate now covers trusted rows too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t [WA-2912]

Review follow-up: trusted+owned is a partition of allSafes, so the combined
address pool can map allSafes directly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ounts table

The alert and the table were adjacent fragment children with no spacing; wrap
them in the same flex-col gap-4 used by the onboarding list.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Stacks the grouped similarity-band UI on top of WA-2912's flag policy and
extends grouping to both onboarding lists:
- Adopt WA-2912 flagging: flag against the full trusted+owned pool, surface ⚠️
  on trusted rows too (a pinned impostor must warn), single top banner.
- Generalize onboarding banding from owned-only to per-list via groupsWithinList
  (counted by distinct address): trusted and owned each band their own same-list
  clusters; a cluster split across the two sections stays per-row ⚠️.
- Tests: route each list its own similarity groups; trusted/owned same-list
  cluster + cross-list no-band coverage.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tongyaosafe
tongyaosafe changed the base branch from dev to tongyao/wa-2912-add-address-poisoning-protection-to-onboarding-select-safe July 27, 2026 12:20
tongyaosafe and others added 3 commits July 27, 2026 16:33
Single model for all SafeAccountsTable surfaces:
- Band + tinted card + title conveys same-list look-alikes; NO per-row ⚠️ there.
- The inline ⚠️ (with a peers tooltip listing full addresses + copy) appears ONLY
  for cross-list clusters — onboarding's trusted↔owned split, the one case a band
  can't box. All single-list surfaces (manage-trusted, add-accounts, safe-accounts)
  now show bands only, no icon.

Table API: drop the icon's `flaggedAddresses` prop; add `similarWarnings`
(address → cross-list peers, drives ⚠️+tooltip) and `anchorAddresses` (vetted/
pinned member leads its band, ordering-only). similarityGroups still drives the band.

Onboarding hook: each list bands all its own cluster members (dropped the ≥2 gate,
so a lone cross-list member is a single boxed card); buildSimilarWarnings emits the
cross-list ⚠️ payload. `flagged` still gates the select-confirm dialog.

Peers tooltip uses a bare copy button (theme-adaptive hover) rather than the shared
CopyAddressButton, which hard-codes a light-surface tint and nests its own tooltip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-up: the variable maps allSafes directly and no longer combines
two lists.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oning-protection-to-onboarding-select-safe' into feat/address-poisoning-grouped-ui

# Conflicts:
#	apps/web/src/features/spaces/components/SelectSafesOnboarding/hooks/useOnboardingSafes.ts
@tongyaosafe

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown

Claude finished @tongyaosafe's task in 2m 49s —— View job


Claude PR Review

  • Gather PR diff and changed files
  • Analyze code (bugs, performance, tests, security)
  • Post review feedback

Focused review of the new similarity-band / pinning code. Nothing security-critical — the band is a rendering layer over existing data — but a few things worth polishing before merge.


1) ReorderableBody — a drag silently rewrites the persisted manual order

apps/web/src/features/myAccounts/components/SafeAccountsTable/ReorderableBody.tsx:83-93

const pinnedGroups = groups.filter(isClustered)
const draggableGroups = groups.filter((group) => !isClustered(group))
...
const reordered = reorderByKey(draggableGroups, source.index, destination.index, (group) => group.parent.address)
onReorder([...pinnedGroups.map((group) => group.parent.address), ...reordered])

Any drop commits pinned-first + rest as the new manual order. If a user had a normal Safe placed above one that later joined a similarity cluster, the first drag reshuffles it and the stored order is lost once the cluster dissolves (e.g. after a rename or a poisoning-flag change). Either apply the pin-to-top purely visually while keeping the stored order untouched, or confirm with product that this is desired.

Also: pinned rows sit inside the same <Droppable> <TableBody> as the <Draggable> items but aren't draggable themselves. Draggable indices start at 0 for draggableGroups, so the mapping is correct — but this is subtle enough to justify a smoke test around a pinned row + drop across the boundary.

Coverage on this file dropped meaningfully in this PR (-13.15% stmts / -26.75% branches per the coverage bot) — the new pinning/band-header branches aren't exercised.

2) orderGroupsBySimilarity — dead fallback + double filter

apps/web/src/features/myAccounts/components/SafeAccountsTable/index.tsx:194-203

const members = membersByCluster.get(cluster) ?? [group]
result.push(...members.filter(isAnchor), ...members.filter((member) => !isAnchor(member)))
  • The ?? [group] fallback is unreachable — every group we're iterating whose cluster we reached here was pushed into membersByCluster in the earlier loop. Drop it (or add a comment stating the defensive intent).
  • members.filter(...).filter(...) walks the same array twice per cluster. A single loop partitioning into anchors / flagged is one pass and reads clearer.

3) Orphaned JSDoc block at index.tsx:162

/** Full-width header row that opens an address-poisoning similarity band (tint lives in the Table sx). */
/**
 * Pulls each similarity cluster's members together …
 */
export const orderGroupsBySimilarity = ...

The first single-line comment describes something else (looks like a leftover from the SimilarityBandHeader extraction into SimilarityBand.tsx) and now mis-labels orderGroupsBySimilarity. Remove it.

4) SimilarityBandHeader — orphan Info icon

apps/web/src/features/myAccounts/components/SafeAccountsTable/SimilarityBand.tsx:99-104

The Info icon has aria-hidden, no tooltip, and no click handler. It reads as "click for more info" but does nothing. Either wire the intended tooltip explaining what "Address poisoning" means, or drop the icon for now.

5) Small nits

  • similarityGroups?.get(line.address.toLowerCase()) runs .toLowerCase() twice per row in the band-header decision (SafeAccountsTable/index.tsx:404-408). Precompute cluster ids in the existing useMemo that builds lines so the reduce/pass runs once. Cheap for small lists, matters if the table grows.
  • bodyRowSx band rules in tableStyles.ts:60-72 rely on same-specificity source order to beat the grey hover pill (see the "after the hover rule" comment). It works with the current emotion/MUI setup but is fragile — a slightly higher specificity for the band selectors would remove the "rule order is load-bearing" foot-gun.
  • useOnboardingSafes.ts:47-58 iterates ownedAddresses/trustedSet/ownedSet several times; one pass into a Map<clusterId, {trusted:[], owned:[]}> while first building the sets is cheaper and reads the same.
  • SimilarityWarningIcon in SimilarityBand.tsx:51-84: the tooltip listing peer addresses via a Copy button is nice, but the copy-then-Check state only resets on unmount via the effect and on a 2s timer. Re-mount safe, but if the same tooltip is closed and reopened within 2s the icon stays Check. Not a bug — worth confirming it's the desired UX.

Nothing blocking; main asks are (1) the persisted-order side effect in the reorderable body and (2) trimming the dead/misleading bits in orderGroupsBySimilarity + the orphaned Info icon.
• Branch: feat/address-poisoning-grouped-ui

tongyaosafe and others added 3 commits July 27, 2026 17:51
…-list cluster policy, split orderGroupsBySimilarity

- useCopyToClipboard: shared copied-flag state machine, used by CopyAddressButton
  and the band tooltip's PeerAddress (was a line-for-line duplicate)
- bandGroupsForList/buildSimilarWarnings + SimilarWarning move from the spaces
  onboarding hook / myAccounts barrel into features/address-poisoning, decoupled
  from safe-item shapes — the cross-list presentation rule now has one owner
- orderGroupsBySimilarity gets its own file (tests no longer load the component
  graph), plus micro cleanups: unreachable ?? fallback dropped, single-pass
  anchor partition, orphaned doc comment removed
- drop debug console.logs from useOnboardingSafes

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…xplain the band via Info tooltip

A drop used to persist the pinned-first display order, so one drag during a
similarity warning permanently hoisted cluster members in the user's manual
order. ReorderableBody now reports only the draggable rows; the table weaves
them back into the stored order (weaveReorderedKeys, unit-tested). The band
header's Info icon now opens a tooltip instead of being decorative.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keep the constraints (rule-order dependence, weave contract, WA-2912 intent),
drop the narration.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tongyaosafe

Copy link
Copy Markdown
Collaborator Author

Claude finished @tongyaosafe's task in 2m 49s —— View job

Claude PR Review

  • Gather PR diff and changed files
  • Analyze code (bugs, performance, tests, security)
  • Post review feedback

Focused review of the new similarity-band / pinning code. Nothing security-critical — the band is a rendering layer over existing data — but a few things worth polishing before merge.

1) ReorderableBody — a drag silently rewrites the persisted manual order

apps/web/src/features/myAccounts/components/SafeAccountsTable/ReorderableBody.tsx:83-93

const pinnedGroups = groups.filter(isClustered)
const draggableGroups = groups.filter((group) => !isClustered(group))
...
const reordered = reorderByKey(draggableGroups, source.index, destination.index, (group) => group.parent.address)
onReorder([...pinnedGroups.map((group) => group.parent.address), ...reordered])

Any drop commits pinned-first + rest as the new manual order. If a user had a normal Safe placed above one that later joined a similarity cluster, the first drag reshuffles it and the stored order is lost once the cluster dissolves (e.g. after a rename or a poisoning-flag change). Either apply the pin-to-top purely visually while keeping the stored order untouched, or confirm with product that this is desired.

Also: pinned rows sit inside the same <Droppable> <TableBody> as the <Draggable> items but aren't draggable themselves. Draggable indices start at 0 for draggableGroups, so the mapping is correct — but this is subtle enough to justify a smoke test around a pinned row + drop across the boundary.

Coverage on this file dropped meaningfully in this PR (-13.15% stmts / -26.75% branches per the coverage bot) — the new pinning/band-header branches aren't exercised.

2) orderGroupsBySimilarity — dead fallback + double filter

apps/web/src/features/myAccounts/components/SafeAccountsTable/index.tsx:194-203

const members = membersByCluster.get(cluster) ?? [group]
result.push(...members.filter(isAnchor), ...members.filter((member) => !isAnchor(member)))
  • The ?? [group] fallback is unreachable — every group we're iterating whose cluster we reached here was pushed into membersByCluster in the earlier loop. Drop it (or add a comment stating the defensive intent).
  • members.filter(...).filter(...) walks the same array twice per cluster. A single loop partitioning into anchors / flagged is one pass and reads clearer.

3) Orphaned JSDoc block at index.tsx:162

/** Full-width header row that opens an address-poisoning similarity band (tint lives in the Table sx). */
/**
 * Pulls each similarity cluster's members together …
 */
export const orderGroupsBySimilarity = ...

The first single-line comment describes something else (looks like a leftover from the SimilarityBandHeader extraction into SimilarityBand.tsx) and now mis-labels orderGroupsBySimilarity. Remove it.

4) SimilarityBandHeader — orphan Info icon

apps/web/src/features/myAccounts/components/SafeAccountsTable/SimilarityBand.tsx:99-104

The Info icon has aria-hidden, no tooltip, and no click handler. It reads as "click for more info" but does nothing. Either wire the intended tooltip explaining what "Address poisoning" means, or drop the icon for now.

5) Small nits

  • similarityGroups?.get(line.address.toLowerCase()) runs .toLowerCase() twice per row in the band-header decision (SafeAccountsTable/index.tsx:404-408). Precompute cluster ids in the existing useMemo that builds lines so the reduce/pass runs once. Cheap for small lists, matters if the table grows.
  • bodyRowSx band rules in tableStyles.ts:60-72 rely on same-specificity source order to beat the grey hover pill (see the "after the hover rule" comment). It works with the current emotion/MUI setup but is fragile — a slightly higher specificity for the band selectors would remove the "rule order is load-bearing" foot-gun.
  • useOnboardingSafes.ts:47-58 iterates ownedAddresses/trustedSet/ownedSet several times; one pass into a Map<clusterId, {trusted:[], owned:[]}> while first building the sets is cheaper and reads the same.
  • SimilarityWarningIcon in SimilarityBand.tsx:51-84: the tooltip listing peer addresses via a Copy button is nice, but the copy-then-Check state only resets on unmount via the effect and on a 2s timer. Re-mount safe, but if the same tooltip is closed and reopened within 2s the icon stays Check. Not a bug — worth confirming it's the desired UX.

Nothing blocking; main asks are (1) the persisted-order side effect in the reorderable body and (2) trimming the dead/misleading bits in orderGroupsBySimilarity + the orphaned Info icon. • Branch: feat/address-poisoning-grouped-ui

fixed

@tongyaosafe tongyaosafe changed the title Feat/address poisoning grouped UI feat(web): grouped look-alike UI across the account tables Jul 27, 2026
@tongyaosafe
tongyaosafe marked this pull request as ready for review July 27, 2026 17:21
Base automatically changed from tongyao/wa-2912-add-address-poisoning-protection-to-onboarding-select-safe to dev July 28, 2026 08:34
tongyaosafe and others added 3 commits July 28, 2026 10:53
…-grouped-ui

# Conflicts:
#	apps/web/src/features/spaces/components/SelectSafesOnboarding/components/OnboardingSafesList.tsx
#	apps/web/src/features/spaces/components/SelectSafesOnboarding/components/__tests__/OnboardingSafesList.test.tsx
#	apps/web/src/features/spaces/components/SelectSafesOnboarding/hooks/__tests__/useOnboardingSafes.test.ts
#	apps/web/src/features/spaces/components/SelectSafesOnboarding/hooks/useOnboardingSafes.ts
#	apps/web/src/features/spaces/components/SelectSafesOnboarding/index.tsx
Its only consumer was the High similarity badge, replaced by the inline
nameAdornment warning icon; the remaining badge= call sites in the app belong
to other components' own props.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only bandHeaderAt in the same file renders it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@tmjssz tmjssz 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.

I am not sure whether it is feasible to change all the address types from string to 0x{string}. Could be a too big refactor. But I think it would be nice to make it more type-safe.

export type SimilarWarning = { trusted: string[]; owned: string[] }

/** Address → cluster id for one list's look-alikes, deduped so a multi-chain safe isn't listed twice. */
export const bandGroupsForList = (items: Addressed[], groupIdByAddress: Map<string, string>): Map<string, string> => {

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.

Maybe we can define a new type and reuse it everywhere possible. Also, it would be nice if we can make the address type more strict. Something like

type GroupIdByAddress = Map<`0x{string}`, string>

WDYT?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Need consider doing it by a following refactoring ticket. Otherwise too big change here.

Comment thread apps/web/src/features/myAccounts/components/SafeAccountsTable/ReorderableBody.tsx Outdated
Comment thread apps/web/src/features/myAccounts/components/SafeAccountsTable/ReorderableBody.tsx Outdated
Comment thread apps/web/src/features/myAccounts/components/SafeAccountsTable/ReorderableBody.tsx Outdated
* Full address + inline copy for the warning tooltip. A bare button, not CopyAddressButton — its
* hover tint and nested Tooltip are both wrong inside this dark hover popup.
*/
const PeerAddress = ({ address }: { address: string }) => {

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.

Also, here we could make it more type-safe.

Suggested change
const PeerAddress = ({ address }: { address: string }) => {
const PeerAddress = ({ address }: { address: `0x{string}` }) => {

Comment on lines +13 to +14
'--band-surface': 'var(--color-yellow-50)',
'--band-border': 'var(--color-yellow-400)',

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.

Lets use our theme color variables only -> apps/web/src/styles/vars.css
We have warning color versions there for both light and dark mode

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I didn't find same / similar color variables by apps/web/src/styles/vars.css.

But I need to keep the same for table (MUI) and the AlertBanner (shadcn). Didn't find a better solution. 🥲

Screenshot 2026-07-29 at 18 09 12 Screenshot 2026-07-29 at 18 09 26

Comment thread apps/web/src/features/address-poisoning/utils/crossListClusters.ts
Comment thread apps/web/src/hooks/useCopyToClipboard.ts
Comment thread apps/web/src/features/myAccounts/components/SafeAccountsTable/tableStyles.ts Outdated
Comment thread apps/web/src/features/myAccounts/components/SafeAccountsTable/tableStyles.ts Outdated
tongyaosafe and others added 6 commits July 29, 2026 12:19
- useCopyToClipboard: await navigator.clipboard.writeText inside try/catch;
  a denied write logs (Errors._110) and leaves `copied` false instead of
  silently pretending success. Add unit tests (success reset + reject path).
- Add ErrorCodes._110 "Failed to copy to clipboard".
- reorder tests: fold the weaveReorderedKeys cases into src/utils/reorder.test.ts
  and drop the duplicate __tests__/reorder.test.ts (reviewer flag).
- crossListClusters: unit tests for bandGroupsForList + buildSimilarWarnings.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-up (#8354): replace the two complementary `.filter(isClustered)`
passes with a single `partition(groups, isClustered)`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-up (#8354): pull the two inline map bodies into GroupChildRows,
PinnedGroupRows and DraggableGroupRows, with a SharedRowProps bundle for the
repeated row wiring. Behaviour-neutral — the body is now two one-line maps.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-up (#8354): port bodyRowSx (hover pill, separators, similarity
band) from an inline sx object to tableStyles.module.css for repo consistency.
Mui classes wrapped in :global(); dark kept as :global(.dark) .body; rule order
preserved (band after hover). Applied via className on the <Table>; dynamic
minWidth stays in sx. Behaviour-neutral.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-grouped-ui

# Conflicts:
#	apps/web/src/features/myAccounts/components/SafeAccountsTable/index.tsx
Match the 16px row radius dev landed for the account rows (was 8px); colours
unchanged (yellow-50 surface / yellow-400 border, warning tokens in dark).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

tx-builder Preview

✅ Deploy successful!

Preview URL:
https://feat-address-poisoning-grouped-ui--tx-builder.review.5afe.dev/

@tongyaosafe
tongyaosafe requested a review from tmjssz July 29, 2026 16:44
@tmjssz

tmjssz commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

I checked again and the code looks nice, thanks for fixing everything. But I checked the UI itself now and found some issues there. Also from the figma it looks like the whole block of poisened addresses should be draggable, but that's not possible in your solution rn.
Untitled

@tmjssz

tmjssz commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

One more thing. In case there are multiple address poisoning groups, they are not really well separated. I think a divider line between the groups is missing.
Also, I think we should add some padding between the poisoning group background and the table header.
image

This is how it looks in the figma:
image
There is a divider line at the top of thepoisoning group and also some padding to the yellow background, which makes separates it's better from the other entries.

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.

2 participants