Skip to content

feat: add chip suggestions#406

Open
kamikazechaser wants to merge 1 commit intopreviewfrom
sohail/ens-chip-suggestions
Open

feat: add chip suggestions#406
kamikazechaser wants to merge 1 commit intopreviewfrom
sohail/ens-chip-suggestions

Conversation

@kamikazechaser
Copy link
Copy Markdown
Member

@kamikazechaser kamikazechaser commented Apr 14, 2026

  • Only triggers for alphanumeric. Full complete addresses and phone numbers don't trigger it. Reuses existing utils
  • closes ENS without sarafu.eth #405
image

* Only triggers for alphanumeric, full complete addresses and phone numbers don't trigger it. Reuses existing utils
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Apr 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
sarafu-network Ready Ready Preview, Comment Apr 14, 2026 7:40am

Request Review

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 14, 2026

Claude finished @kamikazechaser's task in 1m 1s —— View job


Code Review

  • Gather context and read changed files
  • Analyze code quality, security, performance, and testing
  • Post review feedback

Overall: Clean, focused addition. The feature works logically within the existing component pattern. A few minor observations below.


Code Quality

What's done well:

  • type="button" on the chips prevents accidental form submission — correct.
  • useMemo([inputValue]) is the right dependency — suggestions recompute only when the input changes.
  • Chips disappear after selection because the full ENS name ends with .eth, triggering the early-return [] — clean self-managing behavior.
  • Reuses existing isAddress / isPhoneNumber utilities consistently with the rest of the component.

Minor observations:

  1. Strings with dots that don't end in .eth — typing something like alice.bob (e.g., a mistyped ENS name) would produce suggestions alice.bob.sarafu.eth and alice.bob.eth. This is valid ENS syntax, but from a UX perspective it could look odd. Worth considering whether to suppress suggestions when inputValue.includes(".") (and it doesn't end in .eth).

    // Potential guard (optional)
    inputValue.includes(".") ||
  2. suffix property is only used as the key prop — the button renders s.full, not s.suffix. The property name is a bit misleading. Renaming to key or label would clarify intent, though it's not incorrect.

  3. Overflow on long inputs — for very long usernames, the chip text could overflow the flex container. Adding max-w-[200px] truncate (or similar) to the button className would prevent layout issues:

    className="text-xs px-2 py-0.5 rounded-md bg-muted text-muted-foreground
      hover:bg-accent hover:text-accent-foreground transition-colors cursor-pointer
      max-w-[200px] truncate"   // ← consider adding

Security

No concerns. Input values flow through the existing handleChange path, React handles JSX escaping, and no raw HTML is being rendered.

Performance

No issues. useMemo is appropriately scoped — the computation is O(1) and runs only when inputValue changes.

Testing

No tests were added for the new chip suggestion logic. Since this is a useMemo with clear branching conditions (empty input, address, phone number, .eth suffix), a unit test for the filtering logic would be straightforward and would guard against regressions. Not a blocker for this PR given the small scope.

Summary

The implementation is correct and minimal. The three points above (dot-containing strings, property naming, overflow handling) are suggestions rather than blockers. Approve once the author has considered them.

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.

1 participant