Repository navigation
chore: add docs, tests, and ci follow-ups - #1944
Merged
Merged
Conversation
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPublishes Browser Support and Flaky Test Quarantine docs and nav entries, updates contributor checklist and DocsTable rendering, adds SSR hydration helpers and overlay hydration tests, extends Theme appearance tests, and tweaks coverage/docs CI workflows plus Jest CI config. ChangesDocumentation and Testing Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/components/layout/Documentation/helpers/DocsTable.js`:
- Around line 90-92: The conditional in renderCellValue that checks if value ===
"boolean" || value === "false" || value === "true" is effectively dead given
current columns (only non-standard id is "shortcut" which holds React <Kbd>
nodes); either remove this branch or convert it to an explicit defensive check
with a clarifying comment: update the renderCellValue function in DocsTable.js
by removing the entire if block that returns InlineCode for those string
literals, or replace it with a clear comment like "defensive: handle
boolean-like string values for potential future non-standard columns" and keep
the condition but ensure it only runs when value is a string (typeof value ===
"string") before comparing to "boolean"/"true"/"false" to avoid misleading dead
code.
In `@src/components/ui/HoverCard/tests/HoverCard.test.tsx`:
- Around line 7-13: Extract the duplicated SSR/hydration scaffolding into a
shared test helper module: move the global TextEncoder/TextDecoder polyfill, the
require(...) calls for react-dom/server's renderToString and react-dom/client's
hydrateRoot (ensuring the globals are set before those requires), the flush()
helper, and the console warn/error filtering logic into that helper; then update
HoverCard.test.tsx (and the other overlay tests) to import the helper and remove
the duplicated blocks. Also change the act usage to import act from
`@testing-library/react` (replace act from react-dom/test-utils) so tests use the
testing-library act compatible with React 19.
- Around line 163-185: The TS2454 error comes from declaring root without a
guaranteed assignment before it's used; fix by changing the declaration of root
in HoverCard.test.tsx to use a definite-assignment assertion (e.g., declare root
with a trailing !: let root!: ReturnType<typeof hydrateRoot>), so the compiler
knows hydrateRoot assigned it inside the async act block before you call
root.unmount(); alternatively, you can initialize root to null (let root:
ReturnType<typeof hydrateRoot> | null = null) and narrow/check before calling
root.unmount(), but the simplest fix is the definite-assignment assertion on the
root variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e4d7ff1-6b84-401a-8d9c-aad2f9856937
📒 Files selected for processing (14)
README.mddocs/app/docs/contributing/contributor-checklist/content.mdxdocs/app/docs/contributing/flaky-test-quarantine/content.mdxdocs/app/docs/contributing/flaky-test-quarantine/page.tsxdocs/app/docs/contributing/flaky-test-quarantine/seo.tsdocs/app/docs/docsNavigationSections.tsxdocs/app/docs/guides/browser-support/content.mdxdocs/app/docs/guides/browser-support/page.tsxdocs/app/docs/guides/browser-support/seo.tsdocs/components/layout/Documentation/helpers/DocsTable.jssrc/components/ui/HoverCard/tests/HoverCard.test.tsxsrc/components/ui/Popover/tests/Popover.test.tsxsrc/components/ui/Theme/tests/Theme.test.tsxsrc/components/ui/Tooltip/tests/Tooltip.test.tsx
This was referenced Jun 4, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/coverage.yml (1)
48-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBase coverage can fail only because the config file may not exist on base.
At Line 48, running Jest with
--config ./jest.coverage-ci.config.tsaftergit checkout $BASE_SHAcan fail when base doesn’t have this new file yet, which forces the dummy zero baseline and skews comparison output.Suggested fix
- if npx jest --coverage --config ./jest.coverage-ci.config.ts; then + JEST_CONFIG_ARG="" + if [ -f ./jest.coverage-ci.config.ts ]; then + JEST_CONFIG_ARG="--config ./jest.coverage-ci.config.ts" + fi + + if npx jest --coverage $JEST_CONFIG_ARG; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/coverage.yml around lines 48 - 56, After checking out the base commit (git checkout $BASE_SHA) verify the existence of the config file used by Jest (./jest.coverage-ci.config.ts) before running npx jest --coverage --config ./jest.coverage-ci.config.ts; if the file is missing, skip running Jest, set base_coverage_success=false and create the dummy base-coverage.json (so comparisons don’t break); otherwise run the existing jest command and copy coverage/coverage-summary.json to base-coverage.json and set base_coverage_success=true. Ensure references to the jest command, the config path ./jest.coverage-ci.config.ts, base-coverage.json and the BASE_SHA checkout remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docs-build.yml:
- Around line 44-53: Replace the two-step global linking flow ("Link local
library package" using `npm link` and "Use local library package in docs" using
`pnpm link `@radui/ui``) with a single pnpm v10-style local link executed from the
docs working directory: remove the `npm link` step and change the `pnpm link
`@radui/ui`` invocation (in the step named "Use local library package in docs" or
equivalent) to `pnpm link ..` so pnpm consumes the local package from the
repository root when run with working-directory: docs.
---
Outside diff comments:
In @.github/workflows/coverage.yml:
- Around line 48-56: After checking out the base commit (git checkout $BASE_SHA)
verify the existence of the config file used by Jest
(./jest.coverage-ci.config.ts) before running npx jest --coverage --config
./jest.coverage-ci.config.ts; if the file is missing, skip running Jest, set
base_coverage_success=false and create the dummy base-coverage.json (so
comparisons don’t break); otherwise run the existing jest command and copy
coverage/coverage-summary.json to base-coverage.json and set
base_coverage_success=true. Ensure references to the jest command, the config
path ./jest.coverage-ci.config.ts, base-coverage.json and the BASE_SHA checkout
remain intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ed47d44-647e-4b13-ad08-e8a77a812b59
📒 Files selected for processing (3)
.github/workflows/coverage.yml.github/workflows/docs-build.ymljest.coverage-ci.config.ts
✅ Files skipped from review due to trivial changes (1)
- jest.coverage-ci.config.ts
kotAPI
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ui/Popover/tests/Popover.test.tsx (1)
172-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard console spy cleanup with
try/finallyIf an assertion throws before cleanup, mocked
consolemethods leak into later tests. Wrap the test body intry/finallysomockRestore()and DOM cleanup always run.Suggested fix
test('hydrates SSR markup without warnings when open', async() => { const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); const error = jest.spyOn(console, 'error').mockImplementation(() => {}); - - const html = renderToString( - <Popover.Root open> - <Popover.Trigger>Open</Popover.Trigger> - <Popover.Content>Popover body</Popover.Content> - </Popover.Root> - ); - - const container = document.createElement('div'); - container.innerHTML = html; - document.body.appendChild(container); - - let root!: ReturnType<typeof hydrateRoot>; - await act(async() => { - root = hydrateRoot(container, ( - <Popover.Root open> - <Popover.Trigger>Open</Popover.Trigger> - <Popover.Content>Popover body</Popover.Content> - </Popover.Root> - )); - await flush(); - }); - - expectNoUnexpectedHydrationWarnings(warn, error); - - await act(() => root.unmount()); - container.remove(); - warn.mockRestore(); - error.mockRestore(); + let container: HTMLDivElement | null = null; + let root: ReturnType<typeof hydrateRoot> | null = null; + try { + const html = renderToString( + <Popover.Root open> + <Popover.Trigger>Open</Popover.Trigger> + <Popover.Content>Popover body</Popover.Content> + </Popover.Root> + ); + + container = document.createElement('div'); + container.innerHTML = html; + document.body.appendChild(container); + + await act(async() => { + root = hydrateRoot(container!, ( + <Popover.Root open> + <Popover.Trigger>Open</Popover.Trigger> + <Popover.Content>Popover body</Popover.Content> + </Popover.Root> + )); + await flush(); + }); + + expectNoUnexpectedHydrationWarnings(warn, error); + } finally { + if (root) { + await act(() => root!.unmount()); + } + container?.remove(); + warn.mockRestore(); + error.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/Popover/tests/Popover.test.tsx` around lines 172 - 204, The test "hydrates SSR markup without warnings when open" must guard console spies and DOM cleanup in a try/finally: wrap the body between jest.spyOn(...) and mockRestore() in a try block and move warn.mockRestore(), error.mockRestore(), container.remove(), and root.unmount() into the finally so they always run; ensure you only call root.unmount() if root is defined (check root !== undefined) to avoid throwing in the finally. This keeps the existing logic using hydrateRoot, act, flush, and expectNoUnexpectedHydrationWarnings but guarantees cleanup even if an assertion fails.src/components/ui/Tooltip/tests/Tooltip.test.tsx (1)
151-183:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
try/finallyfor spy and container cleanupThis test has the same failure-path cleanup gap: if an assertion throws,
mockRestore()may not run and can contaminate later tests.Suggested fix
test('hydrates SSR markup without warnings', async() => { const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); const error = jest.spyOn(console, 'error').mockImplementation(() => {}); - - const html = renderToString( - <Tooltip.Root> - <Tooltip.Trigger>Hover me</Tooltip.Trigger> - <Tooltip.Content>label</Tooltip.Content> - </Tooltip.Root> - ); - - const container = document.createElement('div'); - container.innerHTML = html; - document.body.appendChild(container); - - let root!: ReturnType<typeof hydrateRoot>; - await act(async() => { - root = hydrateRoot(container, ( - <Tooltip.Root> - <Tooltip.Trigger>Hover me</Tooltip.Trigger> - <Tooltip.Content>label</Tooltip.Content> - </Tooltip.Root> - )); - await flush(); - }); - - expectNoUnexpectedHydrationWarnings(warn, error); - - await act(() => root.unmount()); - container.remove(); - warn.mockRestore(); - error.mockRestore(); + let container: HTMLDivElement | null = null; + let root: ReturnType<typeof hydrateRoot> | null = null; + try { + const html = renderToString( + <Tooltip.Root> + <Tooltip.Trigger>Hover me</Tooltip.Trigger> + <Tooltip.Content>label</Tooltip.Content> + </Tooltip.Root> + ); + + container = document.createElement('div'); + container.innerHTML = html; + document.body.appendChild(container); + + await act(async() => { + root = hydrateRoot(container!, ( + <Tooltip.Root> + <Tooltip.Trigger>Hover me</Tooltip.Trigger> + <Tooltip.Content>label</Tooltip.Content> + </Tooltip.Root> + )); + await flush(); + }); + + expectNoUnexpectedHydrationWarnings(warn, error); + } finally { + if (root) { + await act(() => root!.unmount()); + } + container?.remove(); + warn.mockRestore(); + error.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/Tooltip/tests/Tooltip.test.tsx` around lines 151 - 183, The test 'hydrates SSR markup without warnings' leaves console spies and the DOM container cleanup to the end of the test, which can be skipped if an assertion throws; wrap the core test logic in a try/finally so that warn.mockRestore(), error.mockRestore(), container.remove(), and root.unmount() (if defined) always run; locate the test block using Tooltip.test.tsx's test function and the local symbols warn, error, container, root, hydrateRoot, act, flush, and expectNoUnexpectedHydrationWarnings and move the cleanup calls into the finally clause so they execute regardless of failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/ui/Popover/tests/Popover.test.tsx`:
- Around line 172-204: The test "hydrates SSR markup without warnings when open"
must guard console spies and DOM cleanup in a try/finally: wrap the body between
jest.spyOn(...) and mockRestore() in a try block and move warn.mockRestore(),
error.mockRestore(), container.remove(), and root.unmount() into the finally so
they always run; ensure you only call root.unmount() if root is defined (check
root !== undefined) to avoid throwing in the finally. This keeps the existing
logic using hydrateRoot, act, flush, and expectNoUnexpectedHydrationWarnings but
guarantees cleanup even if an assertion fails.
In `@src/components/ui/Tooltip/tests/Tooltip.test.tsx`:
- Around line 151-183: The test 'hydrates SSR markup without warnings' leaves
console spies and the DOM container cleanup to the end of the test, which can be
skipped if an assertion throws; wrap the core test logic in a try/finally so
that warn.mockRestore(), error.mockRestore(), container.remove(), and
root.unmount() (if defined) always run; locate the test block using
Tooltip.test.tsx's test function and the local symbols warn, error, container,
root, hydrateRoot, act, flush, and expectNoUnexpectedHydrationWarnings and move
the cleanup calls into the finally clause so they execute regardless of
failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80941f8c-ed1e-472d-ab9f-ecbe150fb723
📒 Files selected for processing (7)
.github/workflows/coverage.yml.github/workflows/docs-build.ymldocs/components/layout/Documentation/helpers/DocsTable.jssrc/components/ui/HoverCard/tests/HoverCard.test.tsxsrc/components/ui/Popover/tests/Popover.test.tsxsrc/components/ui/Tooltip/tests/Tooltip.test.tsxsrc/components/ui/tests/ssrHydration.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/docs-build.yml
- src/components/ui/HoverCard/tests/HoverCard.test.tsx
- .github/workflows/coverage.yml
- docs/components/layout/Documentation/helpers/DocsTable.js
Contributor
Coverage
✅ Coverage thresholds met! All tests passing. Run |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Themeappearance regression tests for light, dark, and system modesPopover,HoverCard, andTooltipValidation
npx jest src/components/ui/Theme/tests/Theme.test.tsx src/components/ui/Popover/tests/Popover.test.tsx src/components/ui/HoverCard/tests/HoverCard.test.tsx src/components/ui/Tooltip/tests/Tooltip.test.tsx --runInBand --modulePathIgnorePatterns=.worktreeslint-stagedhooks duringgit commitIssues
Closes #1859
Closes #1865
Closes #1912
Closes #1820
Closes #1823
Summary by CodeRabbit
New Features
Documentation
Tests
Style
Chores