fix: hydration error and button disabled state#3019
fix: hydration error and button disabled state#3019revogabe wants to merge 2 commits intounkeyed:mainfrom
Conversation
revogabe
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@revogabe is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis pull request updates the OAuth components used for user authentication. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as OAuthSignIn
participant U as useIsClient Hook
participant LB as LastUsed Component
participant OB as OAuthButton
C->>S: Request Sign In Page
S->>U: Check if running on the client
U-->>S: Return isClient (true/false)
alt isClient is true
S->>LB: Render LastUsed (GitHub, Google)
else isClient is false
S-xLB: Skip rendering LastUsed
end
S->>OB: Render OAuthButton (apply disabled state if loading)
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
github-actions
bot
added
Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/app/auth/oauth-button.tsx (1)
7-16: Consider using rest props for additional button attributesWhile the current implementation works well, you could make the component even more flexible by using the rest pattern to forward additional props to the button element.
-export const OAuthButton: React.FC<ButtonElementProps> = ({ onClick, disabled, children }) => { +export const OAuthButton: React.FC<ButtonElementProps> = ({ onClick, disabled, children, ...rest }) => { return ( <button type="button" disabled={disabled} className="relative flex items-center justify-center h-10 gap-2 px-4 text-sm font-semibold text-white duration-500 border rounded-lg bg-white/10 enabled:hover:bg-white enabled:hover:text-black border-white/10 disabled:opacity-50 disabled:cursor-not-allowed" onClick={onClick} + {...rest} > {children} </button> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/auth/oauth-button.tsx(1 hunks)apps/dashboard/app/auth/sign-in/oauth-signin.tsx(2 hunks)
🔇 Additional comments (11)
apps/dashboard/app/auth/sign-in/oauth-signin.tsx (6)
8-8: Good addition of useIsClient hookAdding the useIsClient hook is an excellent way to handle rendering differences between server and client, which directly addresses the hydration issues mentioned in the PR objectives.
13-13: Well-implemented client-side detectionUsing the useIsClient hook to create a boolean flag that determines client-side rendering is a clean approach to solving hydration issues.
40-43: Good implementation of disabled stateAdding the disabled property when the GitHub authentication is in progress prevents multiple submissions and improves user experience, which aligns well with the PR objectives.
49-49: Proper fix for hydration errorConditionally rendering the LastUsed component only on the client side effectively prevents hydration mismatches between server and client rendering.
51-54: Consistent implementation for Google OAuthThe disabled state is correctly implemented for the Google OAuth button, maintaining consistency with the GitHub button implementation.
60-60: Proper conditional rendering for LastUsedConsistent with the GitHub implementation, the LastUsed component for Google is also only rendered on the client side, preventing hydration issues.
apps/dashboard/app/auth/oauth-button.tsx (5)
3-3: Good type import for better component typingUsing ComponentProps from React is a good practice for creating type-safe components that properly extend HTML element props.
5-5: Well-defined button props typeCreating a type that extends all standard button properties allows the component to accept any valid button attribute without requiring additional prop definitions.
7-7: Improved component signatureUpdating the component to use ButtonElementProps makes it more flexible and aligned with standard HTML button behavior, allowing it to accept all standard button attributes including the disabled state.
11-11: Properly implemented disabled stateCorrectly forwarding the disabled prop to the native button element enables the functionality required by the PR objectives.
12-12: Well-crafted disabled stylingThe className now properly handles the disabled state with appropriate visual feedback (opacity reduction and not-allowed cursor), and correctly uses the enabled: prefix for hover states to ensure they only apply when the button is active.
|
I am going to reject this request. For two reasons:
This is because we need to make sure it won't collide or be immediately obsolete with anything that we are working on. |
|
i'm so sorry @perkinsjr |
All good 🫡 I just don't want you wasting your engineering efforts and finding out that we are working on it, or similarly we have plans to change everything. |
2 participants
What does this PR do?
Description
This PR fixes a hydration error on the auth page and also adds a disabled state to the OAuthButton.
Changes
Hydration Error
Using a hook to check if it's on the client side and allowing Last Used to render only when on the client to prevent a hydration error.
Disabled State
I added a disabled state to disable the button while making the request to Clerk. This will prevent glitches and improve the UX. I also adjusted the typing to accept props like a button without adding extra props manually.
Fixes #3018
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit