-
Notifications
You must be signed in to change notification settings - Fork 495
feat: add localStorage nickname mapping for wallet addresses #743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: add localStorage nickname mapping for wallet addresses #743
Conversation
|
@kadhirash is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to cc08dc3 in 1 minute and 47 seconds. Click for details.
- Reviewed
431lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. app/components/common/Address.tsx:70
- Draft comment:
Avoid mutating the prop 'truncate' (line 70). Instead, assign its value to a local variable (e.g.,const shouldTruncate = truncateUnknown && address === display ? true : truncate) to prevent side‐effects. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. app/components/common/Address.tsx:57
- Draft comment:
Instead of typecasting the event handler to EventListener (line 63), explicitly type the parameter as CustomEvent to improve clarity and type safety. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment is suggesting to "explicitly type the parameter as CustomEvent" but the parameter is already explicitly typed as CustomEvent on line 57. The typecast to EventListener is necessary because addEventListener's type signature expects EventListener, not CustomEvent. The suggestion doesn't make sense - you can't just remove the typecast without causing a TypeScript error. The handler is already properly typed. This appears to be a misunderstanding of why the typecast is needed. The comment doesn't provide a clear alternative that would actually work. Maybe there's a better way to handle this that I'm not aware of, such as using a more specific event type or a different pattern for custom events in TypeScript. Perhaps the comment is suggesting a refactor I don't fully understand. Even if there's a better pattern, the comment doesn't provide clear, actionable guidance on what to do instead. It says to "explicitly type the parameter" but the parameter is already explicitly typed. Without a concrete code suggestion that would actually work, this comment is not actionable and would likely confuse the PR author. The comment should be deleted because it's not actionable - the parameter is already explicitly typed as CustomEvent, and the typecast is necessary for TypeScript compatibility with addEventListener. The comment doesn't provide a clear alternative solution.
3. app/components/common/NicknameEditor.tsx:88
- Draft comment:
The 'Remove' button condition calls getNickname(address) inline (line 88). Consider using the local state (i.e. the 'nickname' state variable) to decide whether to render the button, so you avoid re-reading from localStorage on every render. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a new file, so all code in it is "changed". The comment is about a code quality/performance optimization - avoiding unnecessary localStorage reads on every render. The suggestion is actionable and specific. However, I need to consider: 1) Is this clearly correct? The local state would work since it's initialized from localStorage and kept in sync. 2) Is this important enough? This is a minor performance optimization that probably doesn't matter much for a simple modal. 3) Does it violate the "obvious or unimportant" rule? This seems like a minor optimization that may not be worth the comment. The component will work fine either way, and the performance impact is negligible for a modal that's not frequently re-rendered. While the suggestion is technically correct, this is a very minor performance optimization for a simple modal component. The localStorage read is fast, and the component likely doesn't re-render frequently. This might fall under "obvious or unimportant" comments that should be avoided. The code works correctly as-is. The comment is technically correct and actionable, but it's a micro-optimization that doesn't significantly impact the functionality or performance of this modal. It's the kind of refactor that could be done but isn't necessary. Given the rule to avoid obvious or unimportant comments, this should probably be deleted. This comment suggests a minor performance optimization that is technically correct but not important enough to warrant a review comment. The current code works fine, and the performance impact is negligible. This falls under "obvious or unimportant" comments that should be avoided.
4. app/utils/nicknames.ts:10
- Draft comment:
The utility functions correctly handle JSON parsing and errors. If usage grows or performance becomes a concern, consider caching or batching localStorage accesses. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
Workflow ID: wflow_rqbPrCrLttMTziJY
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Hello, I wasn't sure what the process is but it seems that y'all have write access. Was wondering if you could take a look at this PR as well! Thank you |
app/components/common/Address.tsx
Outdated
| return () => { | ||
| window.removeEventListener('nicknameUpdated', handleNicknameUpdate as EventListener); | ||
| }; | ||
| }, [address]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please extract this hook into a separate file? app/features/nicknames/model/use-nickname.ts is a good place imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, extracted the hook to app/features/nicknames/model/use-nickname.ts.
app/components/common/Address.tsx
Outdated
| > | ||
| <path d="M11 4H4a2 2 0 0 0-2 2v14a2 2 0 0 0 2 2h14a2 2 0 0 0 2-2v-7" /> | ||
| <path d="M18.5 2.5a2.121 2.121 0 0 1 3 3L12 15l-4 1 1-4 9.5-9.5z" /> | ||
| </svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd suggest to move svg to separate file and to load it as a component or with <Image ...>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, moved the SVG to app/features/nicknames/ui/EditIcon.tsx as a reusable component with configurable width, height, and className props.
|
|
||
| return ( | ||
| <div | ||
| className="position-fixed top-0 start-0 w-100 h-100 d-flex align-items-center justify-content-center" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend changing the background for the modal to better align with Explorer's theme. White modal looks a bit overwhelming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @@ -0,0 +1,51 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We started to split the logic inside the Explorer according to the FSD. I'd suggest moving it to the features/ directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restructured to the following FSD pattern:
app/features/nicknames/lib/for utility functionsapp/features/nicknames/model/for hooksapp/features/nicknames/ui/for components & storiesapp/features/nicknames/index.tsfor feature exports
| @@ -0,0 +1,51 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the 'use client' directive should be added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 'use client' directive to lib/nicknames.ts and model/use-nickname.ts.
| onClose: () => void; | ||
| }; | ||
|
|
||
| export function NicknameEditor({ address, onClose }: Props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you choose to move files to a separate feature directory, that is also a candidate to move there.
Also it would be great to have a Storybook's story for that component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created app/features/nicknames/ui/stories/NicknameEditor.stories.tsx with three stories:
- Primary (new nickname)
- WithExistingNickname (edit existing)
- LongAddress (edge case)
| <button className="btn btn-sm btn-secondary" onClick={onClose}> | ||
| Cancel | ||
| </button> | ||
| <button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest minor UX improvement to cycle tab-navigation between form elements by focusing the inputRef in case Tab is pressed when Save button is focused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented handleSaveButtonKeyDown that cycles focus from the Save button back to the input field when Tab is pressed, creating a better keyboard navigation UX
|
Hey @rogaldh, are you able to take a look again? Thank you |
|
Hey @jnwng are you able to take a look to review as well? |
Skaybeili
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kFZUpSGr9uSfpv1vDVQS8FdQST1PH11JnGHhWzb8Lbd
|
Hey @rogaldh just wanted to follow up on this PR, it's been open for a while and merge conflicts seem to occur as others PRs get merged. @jacobcreech Are you available able to review this? |
|
Hi. I didn't have time to check yet, sorry. I'll try to check it this week. |
|
No worries @rogaldh, thank you! |
|
@rogaldh any update? |

Description
Adds localStorage-based nickname mapping for wallet addresses. Users can click a pencil icon next to any address to set a memorable nickname that persists across sessions. Nicknames are displayed as "Nickname" (address).
Type of change
Screenshots
Testing
pnpm test)Related Issues
Closes #591
Checklist
Additional Notes
Important
Add localStorage-based nickname mapping for wallet addresses with a new
NicknameEditorcomponent and utility functions for management.Address.tsx.NicknameEditorcomponent for editing nicknames.getNickname,setNickname, andremoveNicknameinnicknames.tsfor localStorage management.nicknames.test.tswith 11 test cases for CRUD operations on nicknames.nicknameUpdatedevents.This description was created by
for cc08dc3. You can customize this summary. It will automatically update as commits are pushed.