Security Fixes - 19 Feb 2025 #884
Conversation
Co-authored-by: Arshpreet Singh <73437174+GoldGroove06@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: snyk-bot <snyk-bot@snyk.io>
Co-authored-by: Arshpreet Singh <73437174+GoldGroove06@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: snyk-bot <snyk-bot@snyk.io>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
WalkthroughThis pull request updates dependency versions in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 19
🔭 Outside diff range comments (4)
src/components/ui/Dropdown/stories/Dropdown.stories.tsx (1)
18-24:⚠️ Potential issueImplement proper event handling and input validation.
The empty
onSelecthandler and unsanitized content could pose security risks in production:
- Empty event handlers might lead to unexpected behavior
- Long text content should be sanitized to prevent XSS attacks
- onSelect={() => {}} label={'Bello'} list={[ + onSelect={(value) => { + // Validate and sanitize the selected value + const sanitizedValue = DOMPurify.sanitize(value); + // Handle the selection + console.log('Selected:', sanitizedValue); + }} + label={'Bello'} + list={[docs/app/showcase/music-app/helpers/MusicPlayer.tsx (2)
12-12:⚠️ Potential issueAdd security measures for external image loading.
The external image URL poses several security concerns:
- No integrity check for the external resource
- Potential for the image to be unavailable
- Possible CSP violations
Consider these security improvements:
- Host the image locally or use a CDN with SRI
- Add error handling for image loading
- Implement proper alt text for accessibility
- <img src="https://upload.wikimedia.org/wikipedia/en/2/2a/Linkin_Park_Hybrid_Theory_Album_Cover.jpg" className='rounded-md' alt="Avatar" width={48} /> + <img + src="/assets/images/album-cover.jpg" + className='rounded-md' + alt="Linkin Park - Hybrid Theory Album Cover" + width={48} + onError={(e) => { + e.currentTarget.src = '/assets/images/fallback-cover.jpg'; + console.error('Failed to load album cover'); + }} + />
23-23: 🛠️ Refactor suggestionRemove inline styles to comply with CSP.
Inline styles might violate Content Security Policy (CSP). Move styles to CSS classes.
- <div className='text-gray-1000 hover:text-purple-900 cursor-pointer' style={{ height: 18, width: 18 }}> + <div className='text-gray-1000 hover:text-purple-900 cursor-pointer h-[18px] w-[18px]'> - <div className='text-gray-1000 mx-2 border-2 border-gray-1000 rounded-full p-2 flex items-center justify-center hover:bg-purple-1000 hover:text-purple-50 hover:border-purple-1000 cursor-pointer' style={{ height: "48px", width: "48px" }}> + <div className='text-gray-1000 mx-2 border-2 border-gray-1000 rounded-full p-2 flex items-center justify-center hover:bg-purple-1000 hover:text-purple-50 hover:border-purple-1000 cursor-pointer h-[48px] w-[48px]'>Also applies to: 33-33
src/components/ui/Table/tests/Table.test.tsx (1)
61-63:⚠️ Potential issueAdd type checking before toString conversion.
Direct toString() calls on potentially undefined values could cause runtime errors. Add type checking for safer conversion.
- expect(cell.textContent).toEqual( - Object.values(employeeData[rowIndex])[cellIndex].toString() - ); + const value = Object.values(employeeData[rowIndex])[cellIndex]; + expect(cell.textContent).toEqual( + value != null ? String(value) : '' + );
🧹 Nitpick comments (27)
docs/components/navigation/Navigation.js (3)
6-8: Remove unused 'useEffect' import.
This import is not used in the component. Removing it will keep the codebase cleaner.- import { useContext, useEffect } from 'react'; + import { useContext } from 'react';
253-255: Consider using Tailwind’s “box-border” utility.
“border-box” might not be recognized by default Tailwind CSS. If you intended the box-border utility, consider the following adjustment:- <div className={`${isDocsNavOpen ? mobileClasses : ""} lg:relative lg:w-full lg:block border-box overflow-y-auto ...`}> + <div className={`${isDocsNavOpen ? mobileClasses : ""} lg:relative lg:w-full lg:block box-border overflow-y-auto ...`}>
261-262: Ensure keyboard accessibility for closing the nav.
Currently, the navigation is closed on click. If accessibility is a priority, consider adding a keydown handler so keyboard users can also close the navigation pane.docs/components/Main/Main.js (1)
30-32: Remove unnecessary Fragment.
Based on the static analysis, the Fragment here only wraps a single block of children. You can directly return {children} to simplify.<NavBar cookies={cookies} ... /> - <> {children} - </> + {children}🧰 Tools
🪛 Biome (1.9.4)
[error] 30-32: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
docs/components/Main/NavBar/index.js (2)
138-142: Extract cookie configuration to constants.The cookie configuration values should be extracted to constants for better maintainability and reusability.
+const COOKIE_CONFIG = { + maxAge: 30 * 24 * 60 * 60, // 30 days + path: "/", +}; - setCookie(cookies, "darkMode", toggledState, { - maxAge: 30 * 24 * 60 * 60, - path: "/", - }); + setCookie(cookies, "darkMode", toggledState, COOKIE_CONFIG);
134-146: Optimize event handlers with useCallback.Consider memoizing the event handlers to prevent unnecessary re-renders.
- const toggleDarkMode = () => { + const toggleDarkMode = useCallback(() => { const toggledState = !darkMode; setDarkMode(toggledState); setCookie(cookies, "darkMode", toggledState, { maxAge: 30 * 24 * 60 * 60, path: "/", }); - }; + }, [darkMode, setDarkMode, cookies, setCookie]); - const handleDocsNavOpen = () => { + const handleDocsNavOpen = useCallback(() => { setIsDocsNavOpen(!isDocsNavOpen); - }; + }, [isDocsNavOpen, setIsDocsNavOpen]);src/core/primitives/Radio/index.tsx (3)
2-2: Avoid using genericanytypes for event handlers.
Usinganymakes it harder to rely on static analysis tools to catch type errors. Consider using more specific types, such as(data: { value: string; checked: boolean }) => void, to ensure stronger type safety.type RadioPrimitiveProps = { - onClick: (data: any) => void; - onChange: (data: any) => void; + onClick: (data: { value: string; checked: boolean }) => void; + onChange: (data: { value: string; checked: boolean }) => void; checked: boolean; name: string; value: string; };
12-15: Improve data-attributes usage for clarity.
Currently,'data-checked': checked.toString()returns"true"or"false". You might consider usingchecked ? 'checked' : 'unchecked'to make debugging DOM attributes clearer.const dataAttributes = { - 'data-checked': checked.toString() + 'data-checked': checked ? 'checked' : 'unchecked' };
27-35: Review necessity of separate onClick and onChange handlers.
Because radios typically rely ononChange, having both might lead to duplicate triggers or confusion. Evaluate whetheronClickis truly needed.docs/components/layout/ScrollContainers/FullHeightScroll.tsx (2)
1-6: Add TypeScript type definitions for props.For better type safety and developer experience, define an interface for the component props.
+interface FullHeightScrollProps { + children: React.ReactNode; + scrollable?: boolean; +} + -const FullHeightScroll = ({ children, scrollable = true }) => { +const FullHeightScroll = ({ children, scrollable = true }: FullHeightScrollProps) => {
3-5: Enhance accessibility for scrollable container.Add ARIA attributes and keyboard navigation support for better accessibility.
- return <div className={`flex flex-1 ${scrollable?'overflow-y-auto overflow-x-hidden':''}`}> + return <div + className={`flex flex-1 ${scrollable?'overflow-y-auto overflow-x-hidden':''}`} + role="region" + aria-label="Scrollable content" + tabIndex={scrollable ? 0 : undefined} + >docs/app/testing/full_page_scroll/page.tsx (3)
7-12: Optimize performance for large lists.Consider using virtualization for better performance when rendering large lists.
Consider using
react-windoworreact-virtualizedto optimize the rendering of large lists:import { FixedSizeList } from 'react-window'; const Row = ({ index, style }) => ( <div style={style} className="w-full h-10 bg-gray-200"> {index} </div> ); const FullPageScroll = () => { return ( <FullHeightScroll scrollable={true}> <div className="w-full h-full bg-gray-200"> <FixedSizeList height={400} width="100%" itemCount={100} itemSize={40} > {Row} </FixedSizeList> </div> </FullHeightScroll> ); };
5-13: Enhance accessibility and semantic structure.Add proper semantic structure and ARIA attributes for better accessibility.
- <div className="w-full h-full bg-gray-200"> + <section + className="w-full h-full bg-gray-200" + aria-label="Scrollable test content" + > + <ul role="list"> { Array.from({ length: 100 }).map((_, index) => ( - <div key={index} className="w-full h-10 bg-gray-200"> + <li key={index} className="w-full h-10 bg-gray-200"> {index} - </div> + </li> )) } + </ul> - </div> + </section>
3-15: Add test description and extract constants.Document the test purpose and extract hardcoded values for better maintainability.
+const ITEM_COUNT = 100; +const ITEM_HEIGHT = 10; + +/** + * Test component for full page scrolling behavior. + * Renders a list of ${ITEM_COUNT} items to verify scrolling functionality. + */ const FullPageScroll = () => { return <FullHeightScroll scrollable={true}> <div className="w-full h-full bg-gray-200"> { - Array.from({ length: 100 }).map((_, index) => ( - <div key={index} className="w-full h-10 bg-gray-200"> + Array.from({ length: ITEM_COUNT }).map((_, index) => ( + <div key={index} className={`w-full h-${ITEM_HEIGHT} bg-gray-200`}>src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx (1)
7-8: Remove unused context values.The component retrieves
defaultCheckedandonChangefrom context but doesn't use them. Consider removing these unused values to improve code clarity.Apply this diff to remove the unused context values:
- const { defaultChecked, onChange, rootClass } = useContext(RadioGroupContext); + const { rootClass } = useContext(RadioGroupContext);src/components/ui/RadioCards/fragments/RadioCardsItem.tsx (1)
8-9: Remove unused context values and consider extracting common logic.
- The component retrieves
defaultCheckedandonChangefrom context but doesn't use them.- This component appears to be a duplicate of
RadioGroupItemwith only the context changed.First, remove the unused context values:
- const { defaultChecked, onChange, rootClass } = useContext(RadioCardsContext); + const { rootClass } = useContext(RadioCardsContext);Then, consider extracting the common logic into a shared higher-order component or hook to reduce code duplication between
RadioCardsItemandRadioGroupItem.src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (1)
6-9: Remove redundantPropsWithChildrentype.The type definition includes
childrenproperty andPropsWithChildren, which is redundant.Apply this diff to remove the redundancy:
export type RadioGroupProps = { children?: React.ReactNode; -} & DetailedHTMLProps<InputHTMLAttributes<HTMLInputElement>, HTMLInputElement> & PropsWithChildren +} & DetailedHTMLProps<InputHTMLAttributes<HTMLInputElement>, HTMLInputElement>docs/app/showcase/layout.js (2)
2-5: Fix import style consistency.The file uses inconsistent import quotes (double vs single). Consider using a consistent style.
Apply this diff to use consistent single quotes:
-import Heading from "@radui/ui/Heading" -import Separator from "@radui/ui/Separator" +import Heading from '@radui/ui/Heading' +import Separator from '@radui/ui/Separator'
8-20: Add prop types validation and consider migrating to TypeScript.The component lacks prop types validation for the
childrenprop. Additionally, consider migrating to TypeScript for better type safety.First, add prop types validation:
+import PropTypes from 'prop-types' + const ShowCase = ({children})=>{ return <FullHeightScroll> <div className='p-4 text-gray-1000'> <Heading as="h6" className='my-4'> Music App </Heading> <Separator/> <div className='overflow-hidden shadow-xl border border-gray-200 bg-gray-50 px-4 py-4 rounded'> {children} </div> </div> </FullHeightScroll> } + +ShowCase.propTypes = { + children: PropTypes.node.isRequired +}Then, consider migrating to TypeScript by renaming the file to
layout.tsxand adding type annotations:import { FC, ReactNode } from 'react' interface ShowCaseProps { children: ReactNode } const ShowCase: FC<ShowCaseProps> = ({children}) => { // ... rest of the component }src/core/primitives/Radio/RadioPrimitive.stories.tsx (2)
12-18: Improve story implementation for better documentation.Several improvements can be made to the story:
- Empty click handlers don't demonstrate the component's behavior.
- Hardcoded values make the story less flexible.
- Label associations are broken (same issue as in
RadioGroupPrimitiveItem.tsx).Apply this diff to improve the story:
- <RadioPrimitive name='radio' value='radio' onClick={() => {}} /> - <label htmlFor='radio'>Radio 1</label> + <RadioPrimitive + id='radio1' + name='radio' + value='radio1' + onClick={(e) => console.log('Radio 1 clicked:', e.target.value)} + /> + <label htmlFor='radio1'>Radio 1</label> </span> <span> - <RadioPrimitive name='radio' value='radio2' onClick={() => {}} /> - <label htmlFor='radio2'>Radio 2</label> + <RadioPrimitive + id='radio2' + name='radio' + value='radio2' + onClick={(e) => console.log('Radio 2 clicked:', e.target.value)} + /> + <label htmlFor='radio2'>Radio 2</label>
28-30: Improve type safety in onClick handler.The
onClickhandler usesanytype which reduces type safety.Apply this diff to improve type safety:
- onClick: (data: any) => { - console.log('data', data); - } + onClick: (event: React.MouseEvent<HTMLInputElement>) => { + console.log('Radio clicked:', event.currentTarget.value); + }src/components/ui/RadioCards/fragments/RadioCardsRoot.tsx (1)
19-25: Remove duplicate customRootClass prop.The
customRootClassprop is passed twice:
- To
customClassSwitcherto generaterootClass- Directly to
RadioGroupPrimitive.RootThis appears redundant as the class is already included in the computed
rootClass.- <RadioGroupPrimitive.Root className={clsx(rootClass, className)} customRootClass={customRootClass}>{children}</RadioGroupPrimitive.Root> + <RadioGroupPrimitive.Root className={clsx(rootClass, className)}>{children}</RadioGroupPrimitive.Root>src/components/ui/Disclosure/Disclosure.tsx (1)
16-24: Use stable unique identifiers as keys.Using array indices as keys can lead to issues with component state and animations when items are reordered, added, or removed. Consider using a stable unique identifier from the item data.
- {items.map((item, index) => ( - <DisclosureItem key={index} value={index}> + {items.map((item, index) => ( + <DisclosureItem key={`${item.title}-${index}`} value={index}>src/components/ui/Card/stories/Card.stories.tsx (1)
9-13: Verify Avatar component's API.The comment indicates that Avatar doesn't have a size prop, but this seems like a limitation. Consider:
- Adding the size prop to the Avatar component
- Using CSS classes for sizing
- Documenting the sizing approach in the component's documentation
src/components/ui/Disclosure/stories/Disclosure.stories.tsx (2)
21-24: Consider using less sensitive sample content in stories.The current sample content includes IT troubleshooting information that could potentially reveal system information. Consider using generic, non-sensitive content for demonstration purposes.
- title: 'Why can\'t I access certain websites?', - content: 'Clear your browser\'s cache and cookies.' + title: 'What is Section A?', + content: 'This is the content for Section A.'- title: 'Why do I keep getting disconnected from the network?', - content: 'Ensure that your network drivers are up-to-date.' + title: 'What is Section B?', + content: 'This is the content for Section B.'Also applies to: 27-29
48-57: Add input sanitization for dynamic content.While the current implementation uses static content, consider adding input sanitization for future implementations that might use dynamic content to prevent XSS attacks.
Consider using a sanitization library like DOMPurify when the content is dynamic:
import DOMPurify from 'dompurify'; // Usage in Disclosure.Content <Disclosure.Content> {typeof content === 'string' ? DOMPurify.sanitize(content) : content} </Disclosure.Content>styles/themes/components/radiocards.scss (1)
21-29: Consider adding hover state styles.The component handles checked and focus states well, but adding hover state styles would improve user interaction feedback.
Add hover state styles:
&:focus-within { border: 1px solid var(--rad-ui-color-accent-800); background-color: var(--rad-ui-color-accent-100); } + + &:hover:not([data-checked="true"]) { + border: 1px solid var(--rad-ui-color-accent-600); + background-color: var(--rad-ui-color-accent-50); + }
🛑 Comments failed to post (19)
docs/components/Main/NavBar/index.js (3)
7-122: 🛠️ Refactor suggestion
Add ARIA labels to icon components for better accessibility.
The icon components should include ARIA labels to improve accessibility for screen readers.
Here's an example implementation for one of the icons:
-const DiscordLogo = () => { +const DiscordLogo = ({ ariaLabel = "Discord" }) => { return ( <svg + aria-label={ariaLabel} + role="img" width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg" >Apply similar changes to all icon components.
Additionally, consider creating a shared base icon component to reduce code duplication:
const BaseIcon = ({ children, ariaLabel, ...props }) => ( <svg aria-label={ariaLabel} role="img" width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg" {...props} > {children} </svg> );
127-132:
⚠️ Potential issueEnhance security for external link handling.
The
openLinkcallback usingwindow.openshould include additional security attributes for external links.Apply this improvement:
const openLink = useCallback( (url) => () => { - window.open(url, "_blank"); + window.open(url, "_blank", "noopener,noreferrer"); }, [] );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const openLink = useCallback( (url) => () => { window.open(url, "_blank", "noopener,noreferrer"); }, [] );
149-217: 🛠️ Refactor suggestion
Enhance navigation accessibility with semantic HTML and ARIA labels.
The navigation structure should use semantic HTML elements and include proper ARIA labels.
<div className={`px-3 py-2 flex items-center justify-between border-b border-gray-500 sticky z-20 top-0 backdrop-blur-md backdrop-saturate-150`} + role="navigation" + aria-label="Main" > <div className="mr-3 flex items-center space-x-8"> <a className="text-gray-1000 flex items-center space-x-2 text-md" href="/" + aria-label="Home" > <RadUILogo /> </a> - <div className="hidden lg:block"> + <nav className="hidden lg:block" aria-label="Primary"> <ul className="text-sm flex items-center space-x-4"> <li> <a className="text-gray-950 hover:text-gray-1000" href="/docs/first-steps/introduction" > Docs </a> </li> {/* ... */} </ul> - </div> + </nav> </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div className={`px-3 py-2 flex items-center justify-between border-b border-gray-500 sticky z-20 top-0 backdrop-blur-md backdrop-saturate-150`} role="navigation" aria-label="Main" > <div className="mr-3 flex items-center space-x-8"> <a className="text-gray-1000 flex items-center space-x-2 text-md" href="/" aria-label="Home" > <RadUILogo /> </a> <nav className="hidden lg:block" aria-label="Primary"> <ul className="text-sm flex items-center space-x-4"> <li> <a className="text-gray-950 hover:text-gray-1000" href="/docs/first-steps/introduction" > Docs </a> </li> <li> <a className="text-gray-950 hover:text-gray-1000" href="/showcase/music-app" > Showcase </a> </li> </ul> </nav> </div> <div className="flex items-center"> <div className="hidden lg:block"> <Button color="gray" variant="soft" onClick={openLink("https://discord.gg/nMaQfeEPNp")} > <span className="text-gray-1000"> <DiscordLogo /> </span> </Button> <Button color="gray" variant="soft" onClick={openLink("https://github.com/rad-ui/ui")} > <span className="text-gray-1000"> <GithubLogo /> </span> </Button> <Button color="gray" variant="soft" onClick={toggleDarkMode}> <span className="text-gray-1000"> {darkMode ? <MoonIcon /> : <SunIcon />} </span> </Button> </div> <div className="lg:hidden"> <Button color="gray" variant={isDocsNavOpen ? "soft" : "outline"} onClick={handleDocsNavOpen} > <MobileMenuIcon /> </Button> </div> </div> </div>src/components/ui/Disclosure/fragments/DisclosureTrigger.tsx (1)
26-48:
⚠️ Potential issueAddress potential XSS vulnerabilities in className and children rendering.
While the button implementation is generally secure and accessible, there are two potential security concerns:
- The
rootClassfrom context is directly interpolated into the className without sanitization- The
childrenprop is rendered without sanitizationConsider these improvements:
- className={clsx(`${rootClass}-trigger`, className)} + className={clsx( + rootClass ? `${rootClass.replace(/[^a-zA-Z0-9-_]/g, '')}-trigger` : '', + className + )}Also, consider implementing a sanitization utility for the children prop if it can contain user-provided content.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<button type='button' className={clsx( rootClass ? `${rootClass.replace(/[^a-zA-Z0-9-_]/g, '')}-trigger` : '', className )} onClick={handleDisclosure} onBlur={handleBlurEvent} onFocus={onFocusHandler} onKeyDown={(e) => { if (e.key === 'ArrowDown') { e.preventDefault(); focusNextItem(); } if (e.key === 'ArrowUp') { e.preventDefault(); focusPrevItem(); } }} aria-expanded={activeItem === itemValue} aria-haspopup='true' > {children} </button>docs/components/layout/Theme.js (1)
2-4:
⚠️ Potential issueAddress boolean vs. string comparison for darkMode.
Although the default prop is a boolean, the check is done against the string'true'. This could cause unexpected behavior. Consider using a stricter approach, such as directly leveraging a boolean check.-<div className={`${className} ${darkMode==='true'?'rad-ui-dark-theme':''}`} > +<div className={`${className} ${darkMode ? 'rad-ui-dark-theme' : ''}`} >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const Theme = ({ children, className='', darkMode=false }) => { return ( <div className={`${className} ${darkMode ? 'rad-ui-dark-theme' : ''}`} >docs/app/testing/disable_full_page_scroll/page.tsx (1)
3-15: 🛠️ Refactor suggestion
Reduce code duplication by reusing test component.
This component is nearly identical to
full_page_scroll/page.tsx. Consider creating a shared test component.Create a new shared component:
// components/test/ScrollTest.tsx interface ScrollTestProps { scrollable: boolean; } export const ScrollTest = ({ scrollable }: ScrollTestProps) => { return ( <FullHeightScroll scrollable={scrollable}> <div className="w-full h-full bg-gray-200"> {Array.from({ length: 100 }).map((_, index) => ( <div key={index} className="w-full h-10 bg-gray-200"> {index} </div> ))} </div> </FullHeightScroll> ); };Then reuse it in both test files:
// full_page_scroll/page.tsx import { ScrollTest } from '@/components/test/ScrollTest'; const FullPageScroll = () => <ScrollTest scrollable={true} />; export default FullPageScroll;// disable_full_page_scroll/page.tsx import { ScrollTest } from '@/components/test/ScrollTest'; const FullPageScroll = () => <ScrollTest scrollable={false} />; export default FullPageScroll;src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (1)
11-14: 🛠️ Refactor suggestion
Improve type safety of the object creation pattern.
The empty object assignment pattern with
as constdoesn't provide proper type checking for the properties.Apply this diff to improve type safety:
-const RadioGroupPrimitive = {} as const; - -RadioGroupPrimitive.Root = RadioGroupPrimitiveRoot; -RadioGroupPrimitive.Item = RadioGroupPrimitiveItem; +const RadioGroupPrimitive = { + Root: RadioGroupPrimitiveRoot, + Item: RadioGroupPrimitiveItem +} as const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const RadioGroupPrimitive = { Root: RadioGroupPrimitiveRoot, Item: RadioGroupPrimitiveItem } as const;src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (2)
6-8:
⚠️ Potential issueFix type name mismatch.
The type is defined as
RadioGroupItemPropsbut used asRadioGroupPrimitiveItemPropsin the component props.Apply this diff to fix the type name:
-type RadioGroupItemProps = PropsWithChildren<{ +type RadioGroupPrimitiveItemProps = PropsWithChildren<{ value: string; }>;Also applies to: 10-10
23-24:
⚠️ Potential issueFix accessibility: Add matching id for label association.
The
htmlForattribute on the label won't work as expected since there's no matchingidon the radio input. This breaks accessibility as screen readers won't associate the label with the input.Apply this diff to fix the label association:
- <RadioPrimitive value={value} name='radio' checked={checkedItem === value} onChange={handleOnChange} /> - <label htmlFor={value}>{children}</label> + <RadioPrimitive id={value} value={value} name='radio' checked={checkedItem === value} onChange={handleOnChange} /> + <label htmlFor={value}>{children}</label>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<RadioPrimitive id={value} value={value} name='radio' checked={checkedItem === value} onChange={handleOnChange} /> <label htmlFor={value}>{children}</label>src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (1)
30-30:
⚠️ Potential issueSecurity: Restrict props spreading to avoid XSS.
Spreading props directly to the div element could allow injection of dangerous attributes like
dangerouslySetInnerHTML.Apply this diff to restrict props spreading:
- <div {...props}>{children}</div> + <div className={props.className} style={props.style}>{children}</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div className={props.className} style={props.style}>{children}</div>src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (1)
15-15:
⚠️ Potential issueFix type mismatch in onChange handler.
The
onChangeprop type expects an input event handler, but the context is using a string parameter.Apply this diff to fix the type mismatch:
- onChange?: (e: React.ChangeEvent<HTMLInputElement>) => void; + onChange?: (value: string) => void; - return <RadioGroupContext.Provider value={{ defaultChecked, rootClass, onChange }}> + return <RadioGroupContext.Provider value={{ defaultChecked, rootClass, onChange: (value) => onChange?.(value) }}>Also applies to: 22-22
src/components/ui/RadioCards/RadioCards.stories.tsx (1)
11-15:
⚠️ Potential issueFix incorrect state variable name and default value.
The state variable
languageand its default value'css'don't match the CPU configuration context. This appears to be copied from the RadioGroup component.Apply this diff to fix the state management:
-const [language, setLanguage] = useState('css'); +const [cpuConfig, setCpuConfig] = useState('8-core CPU');Committable suggestion skipped: line range outside the PR's diff.
src/components/ui/Table/tests/Table.test.tsx (1)
8-11: 🛠️ Refactor suggestion
Replace PII in test data with anonymous information.
The test data contains personally identifiable information (PII). Use anonymous or generated data for testing.
- { id: 1, fullName: 'John Smith', age: 23, isIntern: 'No' }, - { id: 2, fullName: 'Anna Donie', age: 35, isIntern: 'Yes' }, - { id: 3, fullName: 'Hannah Brown', age: 20, isIntern: 'Yes' }, - { id: 4, fullName: 'Johnathan White Jr', age: 36, isIntern: 'No' } + { id: 1, fullName: 'Employee A', age: 23, isIntern: 'No' }, + { id: 2, fullName: 'Employee B', age: 35, isIntern: 'Yes' }, + { id: 3, fullName: 'Employee C', age: 20, isIntern: 'Yes' }, + { id: 4, fullName: 'Employee D', age: 36, isIntern: 'No' }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{ id: 1, fullName: 'Employee A', age: 23, isIntern: 'No' }, { id: 2, fullName: 'Employee B', age: 35, isIntern: 'Yes' }, { id: 3, fullName: 'Employee C', age: 20, isIntern: 'Yes' }, { id: 4, fullName: 'Employee D', age: 36, isIntern: 'No' }src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.js (2)
46-47:
⚠️ Potential issueRemove console logging of form data.
Logging form data to the console poses a security risk as it may expose sensitive information in the browser's console.
Apply this diff to remove the console logs:
- console.log('change', data); - console.log('submit', language);Also applies to: 52-53
50-54:
⚠️ Potential issueAdd CSRF protection to form submission.
The form submission handler lacks CSRF protection, which could make it vulnerable to cross-site request forgery attacks.
Would you like me to provide an implementation of CSRF protection for the form submission?
src/components/ui/Disclosure/fragments/DisclosureItem.tsx (1)
35-35:
⚠️ Potential issueUse safe attribute setting method.
Setting attributes directly using
setAttributecould be vulnerable to XSS if the value is not properly sanitized. Consider using safer DOM methods or React's built-in attribute handling.Apply this diff to use React's built-in attribute handling:
- elem.setAttribute('data-rad-ui-focus-element', ''); + elem.dataset.radUiFocusElement = '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.elem.dataset.radUiFocusElement = '';src/components/ui/Callout/stories/Callout.stories.tsx (1)
6-8:
⚠️ Potential issueSanitize SVG content for security.
Inline SVG content should be sanitized to prevent potential XSS attacks through malicious SVG markup.
Consider using a SVG sanitization library like DOMPurify:
+import DOMPurify from 'dompurify'; + const InfoIcon = () => { - return (<svg width="18" height="18" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M7.49991 0.876892C3.84222 0.876892 0.877075 3.84204 0.877075 7.49972C0.877075 11.1574 3.84222 14.1226 7.49991 14.1226C11.1576 14.1226 14.1227 11.1574 14.1227 7.49972C14.1227 3.84204 11.1576 0.876892 7.49991 0.876892ZM1.82707 7.49972C1.82707 4.36671 4.36689 1.82689 7.49991 1.82689C10.6329 1.82689 13.1727 4.36671 13.1727 7.49972C13.1727 10.6327 10.6329 13.1726 7.49991 13.1726C4.36689 13.1726 1.82707 10.6327 1.82707 7.49972ZM8.24992 4.49999C8.24992 4.9142 7.91413 5.24999 7.49992 5.24999C7.08571 5.24999 6.74992 4.9142 6.74992 4.49999C6.74992 4.08577 7.08571 3.74999 7.49992 3.74999C7.91413 3.74999 8.24992 4.08577 8.24992 4.49999ZM6.00003 5.99999H6.50003H7.50003C7.77618 5.99999 8.00003 6.22384 8.00003 6.49999V9.99999H8.50003H9.00003V11H8.50003H7.50003H6.50003H6.00003V9.99999H6.50003H7.00003V6.99999H6.50003H6.00003V5.99999Z" fill="currentColor" fillRule="evenodd" clipRule="evenodd"></path></svg>); + const svgContent = `<svg width="18" height="18" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M7.49991 0.876892C3.84222 0.876892 0.877075 3.84204 0.877075 7.49972C0.877075 11.1574 3.84222 14.1226 7.49991 14.1226C11.1576 14.1226 14.1227 11.1574 14.1227 7.49972C14.1227 3.84204 11.1576 0.876892 7.49991 0.876892ZM1.82707 7.49972C1.82707 4.36671 4.36689 1.82689 7.49991 1.82689C10.6329 1.82689 13.1727 4.36671 13.1727 7.49972C13.1727 10.6327 10.6329 13.1726 7.49991 13.1726C4.36689 13.1726 1.82707 10.6327 1.82707 7.49972ZM8.24992 4.49999C8.24992 4.9142 7.91413 5.24999 7.49992 5.24999C7.08571 5.24999 6.74992 4.9142 6.74992 4.49999C6.74992 4.08577 7.08571 3.74999 7.49992 3.74999C7.91413 3.74999 8.24992 4.08577 8.24992 4.49999ZM6.00003 5.99999H6.50003H7.50003C7.77618 5.99999 8.00003 6.22384 8.00003 6.49999V9.99999H8.50003H9.00003V11H8.50003H7.50003H6.50003H6.00003V9.99999H6.50003H7.00003V6.99999H6.50003H6.00003V5.99999Z" fill="currentColor" fillRule="evenodd" clipRule="evenodd"></path></svg>`; + return <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(svgContent) }} />;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import DOMPurify from 'dompurify'; const InfoIcon = () => { const svgContent = `<svg width="18" height="18" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M7.49991 0.876892C3.84222 0.876892 0.877075 3.84204 0.877075 7.49972C0.877075 11.1574 3.84222 14.1226 7.49991 14.1226C11.1576 14.1226 14.1227 11.1574 14.1227 7.49972C14.1227 3.84204 11.1576 0.876892 7.49991 0.876892ZM1.82707 7.49972C1.82707 4.36671 4.36689 1.82689 7.49991 1.82689C10.6329 1.82689 13.1727 4.36671 13.1727 7.49972C13.1727 10.6327 10.6329 13.1726 7.49991 13.1726C4.36689 13.1726 1.82707 10.6327 1.82707 7.49972ZM8.24992 4.49999C8.24992 4.9142 7.91413 5.24999 7.49992 5.24999C7.08571 5.24999 6.74992 4.9142 6.74992 4.49999C6.74992 4.08577 7.08571 3.74999 7.49992 3.74999C7.91413 3.74999 8.24992 4.08577 8.24992 4.49999ZM6.00003 5.99999H6.50003H7.50003C7.77618 5.99999 8.00003 6.22384 8.00003 6.49999V9.99999H8.50003H9.00003V11H8.50003H7.50003H6.50003H6.00003V9.99999H6.50003H7.00003V6.99999H6.50003H6.00003V5.99999Z" fill="currentColor" fillRule="evenodd" clipRule="evenodd"></path></svg>`; return <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(svgContent) }} />; };.eslintrc.cjs (2)
98-99:
⚠️ Potential issueAdd missing newline at end of file.
Add a newline at the end of the file to comply with ESLint's
eol-lastrule.Apply this diff:
} -}; \ No newline at end of file +}; +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.} };🧰 Tools
🪛 ESLint
[error] 99-99: Newline required at end of file but not found.
(eol-last)
24-66:
⚠️ Potential issueStrengthen security-related ESLint rules.
Several critical security-related rules are set to 'warn' or are disabled. Consider upgrading these to 'error':
- 'eqeqeq'
- 'no-undef'
- 'array-callback-return'
Apply this diff to strengthen the rules:
- 'no-undef': 'off', + 'no-undef': 'error', - eqeqeq: 'warn', + eqeqeq: 'error', - 'array-callback-return': 'warn', + 'array-callback-return': 'error',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.rules: { 'react/react-in-jsx-scope': 'off', indent: ['warn', 4], 'key-spacing': ['warn', { beforeColon: false, afterColon: true }], 'no-trailing-spaces': 'warn', 'no-mixed-spaces-and-tabs': 'warn', 'no-multi-spaces': 'warn', 'no-unused-vars': 'warn', 'no-undef': 'error', 'import/no-absolute-path': 'off', eqeqeq: 'error', 'array-callback-return': 'error', 'react/no-unescaped-entities': 1, 'no-unexpected-multiline': 'warn', 'no-var': 'warn', 'prefer-const': 'warn', 'space-before-blocks': 'warn', 'space-before-function-paren': ['warn', 'never'], 'react/jsx-first-prop-new-line': [1, 'multiline'], 'space-in-parens': 'warn', 'spaced-comment': 'warn', 'arrow-spacing': 'warn', 'comma-style': 'warn', 'func-call-spacing': 'warn', 'comma-spacing': ['warn', { before: false, after: true }], quotes: ['warn', 'single'], 'react/prop-types': 'off', 'prefer-rest-params': 'off', 'no-func-assign': 'off', 'no-invalid-this': 'off', 'react/no-unknown-property': 'off', camelcase: 'off', 'react/jsx-key': 'off', 'require-jsdoc': 'off', 'guard-for-in': 'off', 'no-empty-pattern': 'off', // ignore long strings 'max-len': 'off', semi: [ 'warn', 'always' ],
Summary by CodeRabbit