Improvements and fixes - 2 Aug 2025 batch #1286
Conversation
WalkthroughThe changes update the pre-commit hook script to run tests and set up the shell environment before linting staged files, add a new lint-staged configuration and dependencies, fix a documentation comment to show the "Text" API table, correct a warning message in the Collapsible component, and refactor the VisuallyHidden component to use default inline styles instead of a CSS class, removing the related stylesheet and import. Additionally, the VisuallyHidden component's stories and tests were expanded significantly to demonstrate usage and improve coverage. A new React hook Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.husky/pre-commit(1 hunks)docs/app/docs/components/callout/page.mdx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: the code object in docs/app/docsv2/components/aspect-ratio/docs/codeusage.js requires two closing br...
Learnt from: GoldGroove06
PR: rad-ui/ui#889
File: docs/app/docsv2/components/aspect-ratio/docs/codeUsage.js:16-17
Timestamp: 2025-02-22T18:53:12.639Z
Learning: The code object in docs/app/docsv2/components/aspect-ratio/docs/codeUsage.js requires two closing braces - one for the inner `javascript` object and another for the outer `code` object.
Applied to files:
docs/app/docs/components/callout/page.mdx
📚 Learning: in npm, lifecycle scripts with `pre` and `post` prefixes automatically run before and after their co...
Learnt from: GoldGroove06
PR: rad-ui/ui#1214
File: package.json:64-64
Timestamp: 2025-07-14T18:13:36.606Z
Learning: In npm, lifecycle scripts with `pre` and `post` prefixes automatically run before and after their corresponding main scripts. For example, a `prebuild` script will automatically run before the `build` script when executing `npm run build`, so there's no need to explicitly call `npm run prebuild` in the build script.
Applied to files:
.husky/pre-commit
🔇 Additional comments (2)
docs/app/docs/components/callout/page.mdx (1)
50-50: LGTM! Text API documentation table is now visible.The uncommenting of the Text API documentation table correctly exposes this component's API documentation to users.
.husky/pre-commit (1)
6-6: Excellent addition of testing to pre-commit workflow.Adding
npm testto the pre-commit hook ensures code quality by running tests before each commit, which aligns well with the PR objectives of improvements and fixes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package.json (2)
70-72: Script overlap ‒ risk of divergent lint behaviour betweenlintandlint:stagedThe new
lint:stagedscript is great for pre-commit, but the existinglintscript still lints all files with hard-coded flags. Over time these two command lines can drift (different flags, plugins, caching, etc.).Consider refactoring to keep one single source of truth, e.g.:
- "lint": "eslint --ext .js,.jsx,.ts,.tsx . --fix && stylelint \"**/*.{css,scss,sass}\" --fix", + "lint": "npm run lint:staged -- --no-stash",or move the common logic into an
npm run lint:basethat both scripts call.
This reduces duplication and ensures both CI and pre-commit use identical rules.
76-83: Speed up staged linting with ESLint cache
eslint --fixre-analyses unchanged files each commit. Adding--cachecuts the run time dramatically while keeping correctness.- "*.{js,jsx,ts,tsx}": [ - "eslint --fix" - ], + "*.{js,jsx,ts,tsx}": [ + "eslint --cache --fix" + ],(No equivalent cache flag exists for Stylelint yet.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.husky/pre-commit(1 hunks)package.json(3 hunks)src/components/ui/Collapsible/Collapsible.tsx(1 hunks)src/components/ui/VisuallyHidden/VisuallyHidden.tsx(1 hunks)styles/themes/components/visuallyhidden.scss(0 hunks)styles/themes/default.scss(0 hunks)
💤 Files with no reviewable changes (2)
- styles/themes/components/visuallyhidden.scss
- styles/themes/default.scss
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/Collapsible/Collapsible.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .husky/pre-commit
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
📚 Learning: in npm, lifecycle scripts with `pre` and `post` prefixes automatically run before and after their co...
Learnt from: GoldGroove06
PR: rad-ui/ui#1214
File: package.json:64-64
Timestamp: 2025-07-14T18:13:36.606Z
Learning: In npm, lifecycle scripts with `pre` and `post` prefixes automatically run before and after their corresponding main scripts. For example, a `prebuild` script will automatically run before the `build` script when executing `npm run build`, so there's no need to explicitly call `npm run prebuild` in the build script.
Applied to files:
package.json
📚 Learning: in the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and...
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
Applied to files:
package.json
📚 Learning: ensure to verify existing aria attributes in components before suggesting additions during code revi...
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Applied to files:
src/components/ui/VisuallyHidden/VisuallyHidden.tsx
📚 Learning: in the `toggleprimitive` component (`src/core/primitives/toggle/index.tsx`), when the component beco...
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Applied to files:
src/components/ui/VisuallyHidden/VisuallyHidden.tsx
⏰ 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)
- GitHub Check: build
🔇 Additional comments (4)
package.json (1)
134-134: Dependency addition LGTMAdding
lint-staged@16.1.2is a standard, non-breaking dev-only dependency and aligns with the new script/config.src/components/ui/VisuallyHidden/VisuallyHidden.tsx (3)
2-2: LGTM!The addition of
CSSPropertiesimport is correct and necessary for typing the inline styles.
14-15: Enhanced props typing improves flexibility.The addition of the optional
styleprop and extension withReact.HTMLAttributes<HTMLDivElement>provides better flexibility and type safety while maintaining backward compatibility.
33-50: Well-implemented style merging approach.The component correctly merges default visually hidden styles with user-provided styles, allowing for customization while maintaining the core accessibility functionality. The refactored JSX structure improves readability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/hooks/useIsDocumentHidden/index.ts (1)
11-11: Consider SSR safety for document access.Direct access to
document.hiddenduring initialization could cause issues in server-side rendering environments wheredocumentis undefined.- const [isDocumentHidden, setIsDocumentHidden] = React.useState(document.hidden); + const [isDocumentHidden, setIsDocumentHidden] = React.useState( + typeof document !== 'undefined' ? document.hidden : false + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/hooks/useIsDocumentHidden/index.ts(1 hunks)src/core/hooks/useIsDocumentHidden/useIsDocumentHidden.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
Learnt from: GoldGroove06
PR: rad-ui/ui#1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (`src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx`), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array `[]` is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local `isChecked` changes back to the group's `checkedValues`. This pattern prevents infinite loops while maintaining proper synchronization.
📚 Learning: in the checkboxgroupprimitivetrigger component (`src/core/primitives/checkboxgroup/fragments/checkbo...
Learnt from: GoldGroove06
PR: rad-ui/ui#1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (`src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx`), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array `[]` is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local `isChecked` changes back to the group's `checkedValues`. This pattern prevents infinite loops while maintaining proper synchronization.
Applied to files:
src/core/hooks/useIsDocumentHidden/index.tssrc/core/hooks/useIsDocumentHidden/useIsDocumentHidden.test.tsx
🧬 Code Graph Analysis (1)
src/core/hooks/useIsDocumentHidden/useIsDocumentHidden.test.tsx (1)
src/core/hooks/useIsDocumentHidden/index.ts (1)
useIsDocumentHidden(10-22)
⏰ 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)
- GitHub Check: build
🔇 Additional comments (6)
src/core/hooks/useIsDocumentHidden/index.ts (1)
3-8: Good documentation with proper attribution.The JSDoc comment provides clear documentation and properly attributes the original source.
src/core/hooks/useIsDocumentHidden/useIsDocumentHidden.test.tsx (5)
23-36: Excellent test setup with callback capture mechanism.The beforeEach setup with callback capture is well-designed and allows for proper simulation of visibility change events.
43-59: Good coverage of initial state scenarios.The tests properly verify that the hook initializes with the correct state based on the current
document.hiddenvalue in both visible and hidden states.
70-102: Thorough testing of state updates on visibility changes.The tests properly use
act()to wrap state updates and verify that the hook responds correctly to visibility changes in both directions.
149-172: Great test for multiple hook instances.This test ensures that multiple instances of the hook work independently while all responding to the same global visibility changes, which is important for the hook's reusability.
174-197: Excellent edge case testing with rapid changes.Testing rapid visibility changes within a single
act()block ensures the hook handles high-frequency state updates correctly.
src/core/hooks/useIsDocumentHidden/useIsDocumentHidden.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/ui/Progress/stories/Progress.stories.tsx (1)
49-61: Consider improving type assertion for null value.The new buttons effectively demonstrate state transitions. However, the
null as anytype assertion on Line 52 bypasses TypeScript checking. Consider updating the component types to properly supportnullvalues or use a more specific type assertion.Consider replacing:
-setValue(null as any); +setValue(null);If this causes TypeScript errors, the component's type definitions should be updated to properly support nullable values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Progress/fragments/ProgressIndicator.tsx(1 hunks)src/components/ui/Progress/stories/Progress.stories.tsx(3 hunks)src/components/ui/Progress/tests/Progress.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Progress/fragments/ProgressIndicator.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
Learnt from: GoldGroove06
PR: rad-ui/ui#1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (`src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx`), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array `[]` is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local `isChecked` changes back to the group's `checkedValues`. This pattern prevents infinite loops while maintaining proper synchronization.
📚 Learning: ensure to verify existing aria attributes in components before suggesting additions during code revi...
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Applied to files:
src/components/ui/Progress/tests/Progress.test.tsxsrc/components/ui/Progress/stories/Progress.stories.tsx
📚 Learning: the accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props l...
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Applied to files:
src/components/ui/Progress/stories/Progress.stories.tsx
🧬 Code Graph Analysis (1)
src/components/ui/Progress/stories/Progress.stories.tsx (1)
src/core/primitives/Primitive/Primitive.stories.tsx (1)
Button(20-25)
⏰ 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)
- GitHub Check: build
🔇 Additional comments (7)
src/components/ui/Progress/tests/Progress.test.tsx (4)
6-6: LGTM: Type update supports indeterminate state.The type change from
numbertonumber | nullcorrectly reflects the component's new support for indeterminate progress state.
17-45: Good architectural pattern: Multiple progressbar elements.The switch to
getAllByRole('progressbar')correctly handles the component's structure with separate root and indicator elements. The comment on Line 24 about root vs indicator clamping behavior is particularly helpful for understanding the component's design.
47-275: Excellent comprehensive test coverage.The new test suites provide thorough validation of:
- Data attributes for both root and indicator elements
- All progress states (loading, complete, indeterminate)
- Custom label functionality with
getValueLabel- Dynamic state transitions
- Boundary conditions and edge cases
The test organization with nested describe blocks makes the suite easy to navigate and maintain.
133-133: No changes needed: null values deliberately default to 0 for getValueLabel
TheProgressRootimplementation callsgetValueLabelwith(value ?? 0), so passing0whenvalueisnullis intentional and aligns with the component’s API (its signature only acceptsnumber). The test’s expectation is correct—no code changes required.src/components/ui/Progress/stories/Progress.stories.tsx (3)
10-35: Excellent comprehensive documentation.The detailed documentation clearly explains the Progress component's features including data attributes, state management, and accessibility support. This will be very helpful for developers using the component.
80-133: Excellent interactive demonstration of data attributes.This story provides valuable hands-on experience with the component's data attributes. The real-time updates and explanatory text make it easy to understand how the attributes work.
135-333: Comprehensive story coverage of advanced features.The three additional stories provide excellent coverage:
- StateTransitions: Great animated demonstration with proper cleanup
- CustomLabels: Shows flexibility of
getValueLabelwith multiple practical examples- CustomRanges: Demonstrates non-standard value ranges effectively
The implementation quality is high with proper state management and educational value.
Summary by CodeRabbit
Chores
Documentation
Bug Fixes
New Features
Chores
Tests