refactor(ui): polish code quality across components#175
Conversation
Remove dead code (unused MetaMask comments, sign-in-with-apple), clean up console.log/error statements, replace TypeScript `any` with proper types, improve accessibility by using semantic <button> elements, and add skeleton loading state for NFT gallery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces several improvements across the codebase, focusing on code quality, accessibility, and user experience. It removes unused code, cleans up console logs, and replaces Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
PR Review: refactor(ui): polish code quality across componentsOverall this is a solid cleanup PR — -332/+53 lines with meaningful improvements across TypeScript safety, accessibility, and UX. Here's my detailed feedback: ✅ Strong improvements
|
| Category | Status |
|---|---|
| TypeScript improvements | ✅ All correct |
| Accessibility | ✅ Good, with one concern (nested buttons) |
| Dead code removal | ✅ Clean |
| Console cleanup | ✅ Good |
| NFT skeleton UX | ✅ Better |
w-144 Tailwind class |
|
emoji-picker no-op |
|
| Mock upload function |
The nested button issue in confirm-dialog is the most important to resolve before merge. The rest are minor.
🤖 Reviewed with Claude Code
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| <button | ||
| ref={refs.setReference} | ||
| type="button" | ||
| onClick={() => setIsOpen(true)} | ||
| className="cursor-pointer" | ||
| > | ||
| {children} | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
🔴 Changing ConfirmDialog wrapper from <div> to <button> creates invalid nested buttons
The ConfirmDialog component's wrapper was changed from <div> to <button>, but all existing usages pass a <button> element as children. This creates invalid HTML with nested <button> elements, which is prohibited by the HTML spec and causes undefined browser behavior (e.g., click events may not propagate correctly, React hydration warnings).
Both callers pass button children
In src/app/dash/[userid]/comments/[commentid]/comment-header.tsx:82-122, a <button> is rendered inside ConfirmDialog's children.
In src/app/dash/[userid]/clippings/[clippingid]/@comments/comment.tsx:85-113, a <button> is also rendered inside ConfirmDialog's children.
Both result in <button><button>...</button></button> in the final DOM.
Prompt for agents
In src/components/confirm-dialog/confirm-dialog.tsx lines 123-130, the wrapper element was changed from div to button, but the children passed by callers (comment-header.tsx:82 and comment.tsx:85) are already button elements. This creates invalid nested buttons. Either: (1) Revert the wrapper back to a div element with role='button' tabIndex={0} and onKeyDown for accessibility, OR (2) Keep the button wrapper but update all callers to pass non-interactive children (e.g. replace their <button> wrappers with <span> or <div>).
Was this helpful? React with 👍 or 👎 to provide feedback.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of polishing the codebase by removing dead code, cleaning up console logs, improving type safety by replacing any, and enhancing accessibility. The changes are clean and follow the PR's objectives.
My main feedback revolves around a recurring pattern of removing console.error statements without replacing them with a proper logging mechanism. While this cleans up the browser console, it also swallows important error information that is crucial for debugging production issues. I've left several comments suggesting to log these errors to a dedicated service (e.g., Sentry).
Additionally, I've pointed out a potential UI issue where a div was converted to a button for accessibility but without the necessary styles to make it appear unstyled, which could affect the layout.
Overall, this is a solid refactoring effort. Addressing the points about error logging will make the application more robust and maintainable.
| }) | ||
| } catch (e) { | ||
| console.error(e) | ||
| } catch { |
There was a problem hiding this comment.
While removing console.error aligns with the PR's goal of cleaning up console output, completely swallowing the error here removes valuable debugging information. If fetching the profile fails, we won't know why. It's better to log the error to a dedicated logging service (like Sentry, if used in the project) instead of just catching and ignoring it.
| @@ -101,7 +101,6 @@ export default function AppleStandaloneLoginButton({ | |||
|
|
|||
| onSuccess?.() | |||
| } catch (error) { | |||
There was a problem hiding this comment.
Removing console.error is part of the PR's goal, but swallowing the error object completely can make debugging difficult. While an error message is shown to the user via toast, the full error context is lost. Consider logging the error object to a monitoring service for better error tracking.
| toast.success('Background updated successfully!') | ||
| handleClose() | ||
| } catch (error) { | ||
| } catch { |
There was a problem hiding this comment.
The error object is being swallowed here. While a generic failure toast is shown, logging the actual error to a service would be very helpful for debugging why the background update failed.
| <button | ||
| ref={refs.setReference} | ||
| type="button" | ||
| onClick={() => setIsOpen(true)} | ||
| className="cursor-pointer" | ||
| > | ||
| {children} | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
Great job on improving accessibility by replacing the div with a button. However, the <button> element has default browser styling that might alter the appearance of its children. To ensure the UI remains consistent, it's best to reset the button's styles to make it a transparent wrapper.
| <button | |
| ref={refs.setReference} | |
| type="button" | |
| onClick={() => setIsOpen(true)} | |
| className="cursor-pointer" | |
| > | |
| {children} | |
| </div> | |
| </button> | |
| <button | |
| ref={refs.setReference} | |
| type="button" | |
| onClick={() => setIsOpen(true)} | |
| className="cursor-pointer border-none bg-transparent p-0 text-left" | |
| > | |
| {children} | |
| </button> |
| return JSON.parse(props.data.metadata) | ||
| } catch (e) { | ||
| console.error(e) | ||
| } catch { |
There was a problem hiding this comment.
Silently catching the JSON.parse error and returning null is acceptable for the UI, but it hides potential issues with the metadata format. For easier debugging of malformed metadata, consider logging the error to a monitoring service.
| onError: (err: Error) => { | ||
| toast.error(err.toString()) |
There was a problem hiding this comment.
It's good that you've typed the error as Error instead of any. However, by removing console.log, you're losing the full error details which can be crucial for debugging. While err.toString() is shown in a toast, logging the full err object to a proper logging service would be beneficial.
1 participant
Summary
sign-in-with-apple/component, removed ~195 lines of commented-out MetaMask code, cleaned commented code in emoji-picker, dashboard-container, nft-gallary-itemconsole.log/console.errorstatements across 10 files where toasts or silent handling is sufficientany→ proper types: Fixedanyin 7 files (markdown-components, input, otp-box, preview4, apple-standalone, apple, my-file hooks)<div>with semantic<button>in confirm-dialog, nft-gallary-item, avatar_selectedFilein background-upload-modal-332 lines / +53 lines across 20 files.
Test plan
pnpm lintpasses (0 errors)pnpm buildcompiles successfullypnpm testall 7 tests pass🤖 Generated with Claude Code