fix: Wrap dialog content in an overlay to allow scrolling within other components e.g. combobox#681
Conversation
…r components e.g. combobox
🦋 Changeset detectedLatest commit: f1162e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR refactors dialog components across multiple packages and examples to wrap content within a DialogOverlay element instead of rendering them as siblings, and adds a --fail-on-differences CLI flag to the diff command with workflow integration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR contains homogeneous refactoring patterns across multiple dialog components reducing individual file review friction, but the spread across five dialog files, addition of new CLI logic with conditional error handling, workflow changes, and need to verify DOM structural changes across the codebase warrant careful attention to ensure consistency and functional correctness. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/witty-ends-hide.md(1 hunks)examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsx(2 hunks)packages/ui-components/src/components/ui/dialog/dialog.stories.tsx(2 hunks)packages/ui-components/src/components/ui/dialog/dialog.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx}: TypeScript with strict type checking
Always include return types on top-level functions including React components (React.ReactElement)
Include absolute paths in import statements via tsconfig paths (@src/is the alias forsrc/)
If a particular interface or type is not exported, change the file so it is exportedIf a particular interface or type is not exported, update the TypeScript file so it is exported
Files:
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsxpackages/ui-components/src/components/ui/dialog/dialog.stories.tsxpackages/ui-components/src/components/ui/dialog/dialog.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
Node 16 module resolution - include file extensions in imports (
.js)
Files:
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsxpackages/ui-components/src/components/ui/dialog/dialog.stories.tsxpackages/ui-components/src/components/ui/dialog/dialog.tsx
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx,js}: Sort imports by group: external libs first, then local imports
Use camelCase for variables/functions, PascalCase for types/classes
Order functions such that functions are placed below the variables/functions they use
We use the prefer using nullish coalescing operator (??) ESLint rule instead of a logical or (||), as it is a safer operator
Use console.info/warn/error instead of console.log
Files:
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsxpackages/ui-components/src/components/ui/dialog/dialog.stories.tsxpackages/ui-components/src/components/ui/dialog/dialog.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
Use kebab-case for file names
Files:
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsxpackages/ui-components/src/components/ui/dialog/dialog.stories.tsxpackages/ui-components/src/components/ui/dialog/dialog.tsx
examples/blog-with-auth/packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (examples/blog-with-auth/CLAUDE.md)
examples/blog-with-auth/packages/**/src/**/*.{ts,tsx}: Always use .js extensions in import statements (even when importing from TypeScript files)
Useimport typefor type-only imports
Always specify explicit return types for all functions
Add JSDoc to all exported functions, interfaces, and classes (document parameters, return values, and fields)
Files:
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsx
{packages/project-builder-web/**,packages/ui-components/**}/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/ui-rules.mdc)
{packages/project-builder-web/**,packages/ui-components/**}/*.tsx: Use ShadCN-based components from@baseplate-dev/ui-componentsand always prefer these components over creating custom ones
Use Tailwind CSS utilities exclusively for styling; avoid writing custom CSS classes and use Tailwind's utility classes for all styling needs
In plugins, prefix all Tailwind classes with the plugin name (e.g.,auth-,storage-)
Use icons fromreact-icons/md(Material Design icons) and import them likeimport { MdAdd, MdDelete } from 'react-icons/md'; avoid using other icon libraries
Files:
packages/ui-components/src/components/ui/dialog/dialog.stories.tsxpackages/ui-components/src/components/ui/dialog/dialog.tsx
.changeset/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
.changeset/**/*.md: When adding a new feature or changing an existing feature, add a new Changeset in the .changeset/ directory
Changeset files must follow the specified frontmatter format with a package entry set to patch and a description body
Files:
.changeset/witty-ends-hide.md
🧬 Code graph analysis (2)
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsx (1)
packages/ui-components/src/components/ui/dialog/dialog.tsx (1)
DialogOverlay(160-160)
packages/ui-components/src/components/ui/dialog/dialog.tsx (1)
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsx (1)
DialogOverlay(159-159)
🪛 LanguageTool
.changeset/witty-ends-hide.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...g within other components e.g. combobox.
(QB_NEW_EN)
⏰ 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). (3)
- GitHub Check: test
- GitHub Check: Test E2E
- GitHub Check: Lint
🔇 Additional comments (5)
packages/ui-components/src/components/ui/dialog/dialog.stories.tsx (1)
6-6: LGTM!The import follows coding guidelines with the
.jsextension and is properly organized.examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsx (2)
15-16: LGTM!The comment accurately documents the overlay wrapping change and includes a helpful reference to the related GitHub issue.
70-91: LGTM!The overlay now correctly wraps the dialog content, enabling scrolling behavior for nested components. The structure change preserves all styling and the close button placement.
packages/ui-components/src/components/ui/dialog/dialog.tsx (1)
71-91: LGTM!The overlay now correctly wraps the dialog content, enabling scrolling behavior for nested components. The structure preserves all styling, positioning, and the close button placement.
.changeset/witty-ends-hide.md (1)
1-5: LGTM!The changeset follows the correct format with proper package name, patch level, and a clear description. The static analysis grammar hint is a false positive.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/verify-examples-sync.yml (1)
106-119: The if/else block is now unreachable dead code.With
--fail-on-differencesadded at line 106, the command will throw an error and exit with a non-zero status when differences are found. In GitHub Actions, this causes the step to fail immediately, so lines 108-119 will never execute.You should either:
- Remove the redundant if/else block (lines 108-119), or
- Remove
--fail-on-differencesfrom line 106 and rely on the if/else logic for error handling and detailed messagingOption 2 preserves the helpful error messages and guidance currently in the else block (lines 111-118), which provide more context than the generic error thrown by the CLI.
If you choose to keep the detailed error messages, apply this diff:
- pnpm start diff ${{ matrix.example }} --compact --fail-on-differences + pnpm start diff ${{ matrix.example }} --compactOtherwise, to clean up the dead code, apply this diff:
pnpm start diff ${{ matrix.example }} --compact --fail-on-differences - - if [ $? -eq 0 ]; then - echo "✅ Example '${{ matrix.example }}' is in sync with generators" - else - echo "❌ Example '${{ matrix.example }}' has differences between generated and committed code" - echo "" - echo "This means the example project is not in sync with the latest generator code." - echo "Please run 'pnpm start diff ${{ matrix.example }}' locally to see the differences" - echo "and either:" - echo " 1. Update the generators to match the current code, or" - echo " 2. Regenerate the example code with 'baseplate start sync ${{ matrix.example }}'" - exit 1 - fi
🧹 Nitpick comments (1)
packages/project-builder-cli/src/commands/diff.ts (1)
41-54: Drop unreachable workflow block or embed its guidance in diff.ts
The if/else at.github/workflows/verify-examples-sync.ymllines 108–119 never runs when--fail-on-differencesexits non-zero. Either remove that block or update the Error thrown inpackages/project-builder-cli/src/commands/diff.tsto include the same troubleshooting steps (runpnpm start diff …, update or regenerate the example).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
examples/blog-with-auth/packages/admin/baseplate/generated/src/components/ui/dialog.tsxis excluded by!**/generated/**,!**/generated/**examples/todo-with-auth0/packages/admin/baseplate/generated/src/components/ui/dialog.tsxis excluded by!**/generated/**,!**/generated/**examples/todo-with-auth0/packages/web/baseplate/generated/src/components/ui/dialog.tsxis excluded by!**/generated/**,!**/generated/**tests/simple/packages/web/baseplate/generated/src/components/ui/dialog.tsxis excluded by!**/generated/**,!tests/**,!**/generated/**tests/simple/packages/web/src/components/ui/dialog.tsxis excluded by!tests/**
📒 Files selected for processing (7)
.changeset/free-lights-marry.md(1 hunks).github/workflows/verify-examples-sync.yml(3 hunks)examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsx(2 hunks)examples/todo-with-auth0/packages/admin/src/components/ui/dialog.tsx(2 hunks)examples/todo-with-auth0/packages/web/src/components/ui/dialog.tsx(2 hunks)packages/project-builder-cli/src/commands/diff.ts(1 hunks)packages/react-generators/src/generators/core/react-components/templates/components/ui/dialog.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/free-lights-marry.md
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx}: TypeScript with strict type checking
Always include return types on top-level functions including React components (React.ReactElement)
Include absolute paths in import statements via tsconfig paths (@src/is the alias forsrc/)
If a particular interface or type is not exported, change the file so it is exportedIf a particular interface or type is not exported, update the TypeScript file so it is exported
Files:
packages/project-builder-cli/src/commands/diff.tsexamples/blog-with-auth/packages/admin/src/components/ui/dialog.tsxexamples/todo-with-auth0/packages/web/src/components/ui/dialog.tsxpackages/react-generators/src/generators/core/react-components/templates/components/ui/dialog.tsxexamples/todo-with-auth0/packages/admin/src/components/ui/dialog.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
Node 16 module resolution - include file extensions in imports (
.js)
Files:
packages/project-builder-cli/src/commands/diff.tsexamples/blog-with-auth/packages/admin/src/components/ui/dialog.tsxexamples/todo-with-auth0/packages/web/src/components/ui/dialog.tsxpackages/react-generators/src/generators/core/react-components/templates/components/ui/dialog.tsxexamples/todo-with-auth0/packages/admin/src/components/ui/dialog.tsx
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx,js}: Sort imports by group: external libs first, then local imports
Use camelCase for variables/functions, PascalCase for types/classes
Order functions such that functions are placed below the variables/functions they use
We use the prefer using nullish coalescing operator (??) ESLint rule instead of a logical or (||), as it is a safer operator
Use console.info/warn/error instead of console.log
Files:
packages/project-builder-cli/src/commands/diff.tsexamples/blog-with-auth/packages/admin/src/components/ui/dialog.tsxexamples/todo-with-auth0/packages/web/src/components/ui/dialog.tsxpackages/react-generators/src/generators/core/react-components/templates/components/ui/dialog.tsxexamples/todo-with-auth0/packages/admin/src/components/ui/dialog.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/code-style.mdc)
Use kebab-case for file names
Files:
packages/project-builder-cli/src/commands/diff.tsexamples/blog-with-auth/packages/admin/src/components/ui/dialog.tsxexamples/todo-with-auth0/packages/web/src/components/ui/dialog.tsxpackages/react-generators/src/generators/core/react-components/templates/components/ui/dialog.tsxexamples/todo-with-auth0/packages/admin/src/components/ui/dialog.tsx
examples/blog-with-auth/packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (examples/blog-with-auth/CLAUDE.md)
examples/blog-with-auth/packages/**/src/**/*.{ts,tsx}: Always use .js extensions in import statements (even when importing from TypeScript files)
Useimport typefor type-only imports
Always specify explicit return types for all functions
Add JSDoc to all exported functions, interfaces, and classes (document parameters, return values, and fields)
Files:
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsx
examples/todo-with-auth0/packages/{admin,backend}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (examples/todo-with-auth0/CLAUDE.md)
examples/todo-with-auth0/packages/{admin,backend}/src/**/*.{ts,tsx}: Always use .js extensions in import statements, even when importing from TypeScript files
Useimport typefor type-only imports in TypeScript
Always specify explicit return types for all functions
Files:
examples/todo-with-auth0/packages/admin/src/components/ui/dialog.tsx
examples/todo-with-auth0/{packages/admin/src/{app,components,pages,hooks,services}/**/*.{ts,tsx},packages/backend/src/{modules,plugins,prisma,services,utils}/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (examples/todo-with-auth0/CLAUDE.md)
Add JSDoc to all exported functions, interfaces, and classes including descriptions, params, returns, and fields (exclude generated code)
Files:
examples/todo-with-auth0/packages/admin/src/components/ui/dialog.tsx
🧬 Code graph analysis (1)
packages/project-builder-cli/src/commands/diff.ts (4)
packages/project-builder-cli/src/utils/list-projects.ts (1)
resolveProject(123-145)packages/project-builder-cli/src/utils/create-service-action-context.ts (1)
createServiceActionContext(13-26)packages/project-builder-server/src/actions/utils/cli.ts (1)
invokeServiceActionAsCli(13-26)packages/project-builder-server/src/actions/diff/diff-project.action.ts (1)
diffProjectAction(50-130)
⏰ 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). (2)
- GitHub Check: Test E2E
- GitHub Check: Lint
🔇 Additional comments (7)
examples/todo-with-auth0/packages/admin/src/components/ui/dialog.tsx (2)
16-16: Documentation updated appropriately.The comment clearly explains the structural change and references the Radix UI issue for context.
71-91: Verify scroll behavior with nested combobox.The restructuring wraps
DialogPrimitive.ContentinsideDialogOverlay, following the Radix UI maintainer's recommendation to fix scroll issues with nested interactive components. The implementation looks correct—both components usez-50, but since content is now a child of the overlay, stacking context is preserved.Ensure that:
- Clicking outside the content (but within the overlay) still closes the dialog
- Scrolling works correctly when a combobox or similar component is rendered inside the dialog
- The close button remains functional
examples/todo-with-auth0/packages/web/src/components/ui/dialog.tsx (1)
16-16: LGTM—consistent with admin package.The changes are identical to the admin package implementation, maintaining consistency across the todo-with-auth0 example packages.
Also applies to: 71-91
packages/react-generators/src/generators/core/react-components/templates/components/ui/dialog.tsx (1)
17-17: Template updated consistently.The template file reflects the same structural changes as the concrete implementations. The
$cnplaceholder and@ts-nocheckdirective are appropriate for a generator template.Also applies to: 72-92
examples/blog-with-auth/packages/admin/src/components/ui/dialog.tsx (1)
16-16: LGTM—consistent implementation.The changes match the pattern applied across all dialog implementations in this PR.
Also applies to: 71-91
packages/project-builder-cli/src/commands/diff.ts (1)
24-27: LGTM! Clean addition of the CLI flag.The option is well-named and the description clearly communicates its purpose.
.github/workflows/verify-examples-sync.yml (1)
91-93: Examples aren’t included in pnpm-workspace.yaml; explicit install is required.
The root workspace covers onlypackages/*andplugins/*, sopnpm install --frozen-lockfilein eachexamples/…directory is necessary.
kingston
deleted the
kingston/eng-886-scroll-doesnt-work-in-combobox-in-a-dialog
branch
1 participant
Summary by CodeRabbit
Release Notes
New Features
--fail-on-differencesflag to the diff tool, allowing verification to fail when differences are detected.Bug Fixes
Chores