feat: fall back to ENS for voter table and public profile names#335
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Gaston Review
Verdict: Comment
Score: ████████░░ 8/10
Pull Request Summary
This PR adds ENS name resolution as a fallback in two places: the VoterTable (flow councils membership) and the public profile page. When a user doesn't have a platform display name, it now tries to resolve their ENS name before falling back to a truncated address. The VoterTable implementation is smart about only resolving ENS for the current page of voters (up to 50) rather than the entire roster.
Review Summary
🟡 Warning — Truthy check changed to nullish coalescing for profile name
In VoterTable, the old code used name ? name : truncateAddress(account) which treats empty strings as falsy (falling through to truncateAddress). The new code uses name ?? ensByAddress?.[account]?.name ?? truncateAddress(account) which only checks for null/undefined. If the profile API ever returns an empty string for a name, the old code would show a truncated address, but the new code would render an empty cell. Depending on the API contract this may not be an issue, but it is a behavior change.
🔵 Suggestion — Inconsistent fallback operators between VoterTable and PublicProfile
VoterTable uses ?? (nullish coalescing) for the ENS fallback chain, while PublicProfile uses || (logical OR). Both should use the same operator for the same semantic operation. Since most of the codebase uses ?? for ENS lookups, the PublicProfile should probably use ?? as well — or if empty-string protection is desired, both should use ||.
Clean, focused change. The pagination-scoped ENS resolution in VoterTable is a good design choice. The stable-key memoization pattern avoids unnecessary re-fetches. I have one behavioral concern around the operator change from truthy to nullish coalescing, and a minor inconsistency between the two files.
📌 2 inline comments
🟡 Warning: 1 · ⚪ Nitpick: 1
🔍 Reviewed by Gaston
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: ████████░░ 8/10
Pull Request Summary
This PR adds ENS name resolution as a fallback for display names in two places: the VoterTable (flow councils membership roster) and the public profile page. When a user lacks a platform display name, it resolves their ENS name before falling through to a truncated address. The VoterTable implementation scopes ENS resolution to the current page of voters only, avoiding thousands of unnecessary mainnet reverse lookups.
Both concerns from the prior review are addressed: the fallback chain now uses || consistently in both files, preserving the original empty-string-falls-through behavior. Key casing is correct — VoterTable lowercases account before lookup, and PublicProfile uses address.toLowerCase() for the ENS map key while the hook normalizes internally. Clean, well-scoped change that's ready to ship.
🔍 Reviewed by Gaston
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: ████████░░ 8/10
Pull Request Summary
This PR centralizes the display-name fallback chain (profile name → ENS name → truncated address) into a shared resolveDisplayName utility used by VoterTable, PublicProfile, and MessageItem. The useEnsResolution hook gets a significant upgrade: module-level caching to avoid redundant RPC calls when paging back and forth, an avatars option to skip avatar resolution where unneeded, and a proper cancellation pattern for stale async results. VoterTable scopes ENS resolution to the visible page only and debounces filter inputs to avoid fanning out lookups per keystroke.
Both concerns from the first review (inconsistent ?? vs || operators, empty-string fallthrough behavior) are resolved — || is used consistently throughout. The caching strategy in useEnsResolution is well-designed: successful lookups are cached, failures are left uncached for retry, and the cancellation flag correctly prevents stale state writes without discarding cache-worthy data. The debounce + page-scoped ENS resolution in VoterTable is a smart performance call. Clean, production-ready work.
📌 2 inline comments
⚪ Nitpick: 2
🔍 Reviewed by Gaston
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: ████████░░ 8/10
Pull Request Summary
This PR adds ENS name resolution as a fallback for display names in VoterTable, PublicProfile, and MessageItem, centralizing the fallback chain (profile name → ENS name → truncated address) into a shared resolveDisplayName utility. The useEnsResolution hook gets module-level caching with a 10k-entry cap and oldest-first eviction, an avatars option to skip avatar lookups, and a cancellation pattern for stale async results. VoterTable scopes ENS resolution to the visible page and debounces filter inputs.
The latest push addresses the one remaining nitpick from the prior review — the module-level caches are now capped at 10,000 entries with FIFO eviction. The implementation is correct and minimal. This PR has been through three review rounds, all prior concerns are resolved, and the code is production-ready.
🔍 Reviewed by Gaston
Anywhere the app renders a wallet's platform name, the display now falls back to its ENS name before the truncated address, matching what the comms feeds and the Permissions table already do. Order everywhere: platform name, then ENS name, then truncated address.
Changes
/profile/[address]): the heading falls back to ENS between the platform display name and the truncated address.Already correct (unchanged)
Permissions table, comms feed messages,
useProfileDisplayName, andAccountDropdown.Out of scope
The flow-splitters and flow-guilds graphs and activity feeds resolve ENS but never use the platform name (the opposite gap), so they were left as-is.