fix: default ClickableText button type to "button" #1963
Conversation
Prevents unintended form submissions by explicitly setting type="button" when used as a native button element. The type prop can still be overridden when needed.
|
|
Finished running flow.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
Prevents unintended form submissions by setting a default type="button" for the ClickableText component when used as a native button element, while still allowing the type prop to be overridden when needed.
- Added type prop to the component interface and destructuring
- Restructured the component to handle type prop correctly for both asChild and native button usage
- Set default type="button" for native button elements while preserving override capability
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| <Comp | ||
| {...sharedProps} | ||
| {...props} | ||
| {...(type !== undefined && { type })} |
There was a problem hiding this comment.
The conditional spread operator with undefined check is unnecessarily complex. Since Slot components pass through all props, you can simply spread type directly: {...props} already includes the type prop, making the conditional spread redundant.
| {...(type !== undefined && { type })} |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| <Comp | ||
| {...sharedProps} | ||
| {...props} | ||
| {...(type !== undefined && { type })} |
There was a problem hiding this comment.
Bug: Inconsistent Prop Behavior Causes Form Submission Issues
When asChild is true, the type prop is only passed if explicitly defined, unlike the non-asChild case which defaults to type="button". This inconsistency means button elements used with asChild will default to type="submit", potentially causing unintended form submissions.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughImplements explicit handling for the button type prop in ClickableText, introducing a branch-based render for asChild vs non-asChild. Builds sharedProps (className, ref), conditionally spreads type, and defaults type to "button" when not provided in the non-asChild path. Ensures ClickableText is exported. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant ClickableText
participant Child as Child Element
participant Button as Button Element
Caller->>ClickableText: props { asChild?, type?, className, ref, ... }
alt asChild = true
Note over ClickableText: Build sharedProps { className, ref }<br/>Include type only if provided
ClickableText->>Child: Render with {...sharedProps, ...props, type?}
else asChild = false
Note over ClickableText: Build sharedProps { className, ref }<br/>type = props.type ?? "button"
ClickableText->>Button: Render with {...sharedProps, ...props, type}
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
Files:
**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
Files:
**/*📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
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. Comment |
📋 Manual QA ChecklistBased on the changes in this PR, here are the key areas to test manually:
✨ Prompt for AI AgentsUse the following prompts with Cursor or Claude Code to automate E2E testing: 📝 E2E Test Generation Prompt |
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
1 participant
User description
Prevents unintended form submissions by explicitly setting type="button" when used as a native button element. The type prop can still be overridden when needed.
Summary
fix: default ClickableText button type to "button"
Related Issue
None.
Changes
Prevents unintended form submissions by explicitly setting type="button" when used as a native button element. The type prop can still be overridden when needed.
Testing
Other Information
PR Type
Bug fix
Description
Default button type to "button" to prevent form submissions
Maintain type prop override capability for flexibility
Refactor component logic for better prop handling
Diagram Walkthrough
File Walkthrough
clickable-text.tsx
Default button type to prevent form submissionsapps/studio.giselles.ai/components/ui/clickable-text.tsx
typeprop from component props for explicit handlingtype="button"for native buttonelements
asChildis trueSummary by CodeRabbit