feat(Modal): adds zIndex control and renderDismissButton#460
Merged
joacotornello merged 22 commits intomasterfrom May 4, 2026
Merged
feat(Modal): adds zIndex control and renderDismissButton#460joacotornello merged 22 commits intomasterfrom
joacotornello merged 22 commits intomasterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two new z-index levels (1000, 1100), propagates them through tokens, theme contract, globals, and CSS; introduces variant-based z-index styles for modal; extends Modal API with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
Getting startedPlease make sure you read our documentation on how to write code for components, stories and styles.
|
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/settings.json:
- Line 3: The allow list in .claude/settings.json is overly broad—specifically
the entries "Bash(npx eslint *)" and "Bash(git stash *)" permit wildcard
execution and state-mutating git commands; remove or tighten these entries by
eliminating "Bash(git stash *)" and replacing wildcard Bash allowances with
explicit, least-privilege commands/arguments (e.g., specific npx eslint
subcommands needed) so only the minimal exact commands are permitted; update the
"allow" array to list those exact command signatures instead of wildcard
patterns.
In `@packages/core/styles/src/packages/composite/modal/nimbus-modal.css.ts`:
- Around line 102-109: overlayScoped is a static style hardcoding zIndex[600]
and therefore doesn't follow the zIndex variant model used by overlay
(styleVariants with base/top); change overlayScoped to be a styleVariants (like
overlay) that maps the same zIndex variants (base -> zIndex[600], top ->
zIndex[1000]) and then update the Modal usage for scoped mode to pick the
correct variant (e.g., use modal.classnames.overlayScoped[zIndex] instead of
modal.classnames.overlayScoped) so scoped modals respect the zIndex prop.
- Around line 97-100: The CHANGELOG is missing a breaking-change entry
documenting that the Modal z-index values have migrated to design tokens; add a
breaking-changes bullet to the CHANGELOG.md noting that modal z-index references
(seen in overlay styleVariants using varsThemeBase.zIndex with values
600/1000/1100 via overlay and overlayPortaled) were migrated to design tokens in
v9.0.0, include the affected symbols (overlay, overlayPortaled,
varsThemeBase.zIndex) and a short migration note for consumers to update any
overrides that referenced the numeric z-index keys directly.
In `@packages/react/src/composite/Modal/src/modal.spec.tsx`:
- Around line 79-91: Extend the tests to also assert the overlay element's class
when zIndex changes: after calling makeSut (using makeSut and zIndex: "top")
query for the overlay node (e.g., via getByTestId if FloatingOverlay renders a
test id, or by querying document.body for the portal/overlay element) and add an
expectation that its className contains modal.classnames.overlay.base for the
default case and modal.classnames.overlay.top when zIndex is "top"; locate
references to makeSut, modal.classnames.container, modal.classnames.overlay,
zIndex and FloatingOverlay in the spec to implement the additional assertions.
In `@packages/react/src/composite/Modal/src/modal.stories.tsx`:
- Around line 255-292: The story modalOverSidebar mistakenly shares the same
open state for both Sidebar and Modal via useArgs and updateArgs, causing them
to open together; change the Sidebar to have independent state (e.g., set
Sidebar open always true or manage it with its own useState) while keeping the
modal controlled by useArgs/updateArgs (handleClose and open) so the modal can
be shown above an already-open sidebar; update references to Sidebar and Modal
in modalOverSidebar accordingly and ensure only Modal uses the args-provided
open/onDismiss.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcf0dbd5-7ad2-44f5-a5e4-ee119b3aa8e9
⛔ Files ignored due to path filters (2)
.yarn/versions/65f6cf1a.ymlis excluded by!**/.yarn/**packages/react/src/composite/Modal/src/modal.docs.jsonis excluded by!**/*.docs.json
📒 Files selected for processing (10)
.claude/settings.jsonpackages/core/styles/src/packages/composite/modal/nimbus-modal.css.tspackages/core/styles/src/properties/css.tspackages/core/styles/src/themes/contract.css.tspackages/core/styles/src/themes/globals.css.tspackages/core/tokens/src/zIndex/sys.jsonpackages/react/src/composite/Modal/src/Modal.tsxpackages/react/src/composite/Modal/src/modal.spec.tsxpackages/react/src/composite/Modal/src/modal.stories.tsxpackages/react/src/composite/Modal/src/modal.types.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react/src/composite/Modal/src/modal.spec.tsx (1)
79-97:⚠️ Potential issue | 🟠 MajorAdd overlay z-index assertions (portal and scoped root) to complete feature coverage.
Current tests validate only
containerZIndex. The feature also applies z-index variants to overlays inModal.tsx(Line 130 and Line 147), so overlay regressions would pass unnoticed.✅ Suggested test additions
describe("WHEN zIndex selects a stacking layer", () => { @@ it("AND applies the top containerZIndex class when zIndex='top'", () => { makeSut({ children: <div>content</div>, zIndex: "top" }); const modalElement = screen.getByTestId("modal-element"); expect(modalElement.className).toContain(modal.classnames.container); expect(modalElement.className).toContain( modal.classnames.containerZIndex.top ); }); + + it("AND applies the top overlayZIndex class in portal mode", () => { + makeSut({ children: <div>content</div>, zIndex: "top" }); + const overlay = document.querySelector(`.${modal.classnames.overlay}`); + expect(overlay?.className).toContain(modal.classnames.overlayZIndex.top); + }); + + it("AND applies the top overlayScopedZIndex class in scoped mode", () => { + const root = document.createElement("div"); + document.body.appendChild(root); + makeSut({ children: <div>content</div>, zIndex: "top", root }); + const scopedOverlay = root.querySelector( + `.${modal.classnames.overlayScoped}` + ); + expect(scopedOverlay?.className).toContain( + modal.classnames.overlayScopedZIndex.top + ); + }); });As per coding guidelines,
**/*.spec.tsx: Write Jest unit tests for all components testing all component states, accessibility features, and edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/composite/Modal/src/modal.spec.tsx` around lines 79 - 97, The tests currently only assert container z-index classes; add assertions that the overlay elements (both the portal overlay and the scoped-root overlay rendered by makeSut) also include the overlay base/top classes: check that each overlay element contains modal.classnames.overlay and the appropriate modal.classnames.overlayZIndex.base (for default) or modal.classnames.overlayZIndex.top (when calling makeSut with zIndex: "top"); reuse makeSut to render the modal and query the overlay nodes (by the same test render output or DOM queries used in other tests) and assert their className contains those modal.classnames values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react/src/composite/Modal/src/modal.spec.tsx`:
- Around line 79-97: The tests currently only assert container z-index classes;
add assertions that the overlay elements (both the portal overlay and the
scoped-root overlay rendered by makeSut) also include the overlay base/top
classes: check that each overlay element contains modal.classnames.overlay and
the appropriate modal.classnames.overlayZIndex.base (for default) or
modal.classnames.overlayZIndex.top (when calling makeSut with zIndex: "top");
reuse makeSut to render the modal and query the overlay nodes (by the same test
render output or DOM queries used in other tests) and assert their className
contains those modal.classnames values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0151196e-211b-42de-9a12-4c826f461dd4
⛔ Files ignored due to path filters (1)
.gitignoreis excluded by!**/.gitignore
📒 Files selected for processing (4)
.coderabbit.yamlpackages/core/styles/src/packages/composite/modal/nimbus-modal.css.tspackages/react/src/composite/Modal/src/Modal.tsxpackages/react/src/composite/Modal/src/modal.spec.tsx
💤 Files with no reviewable changes (1)
- .coderabbit.yaml
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #460 +/- ##
=======================================
Coverage 96.07% 96.08%
=======================================
Files 426 426
Lines 4283 4288 +5
Branches 922 908 -14
=======================================
+ Hits 4115 4120 +5
- Misses 159 166 +7
+ Partials 9 2 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish-rc.yml:
- Around line 50-55: The workflow is using actions/setup-node@v4 with an invalid
input auth-oidc and a Node.js version too old for OIDC Trusted Publishing;
remove the auth-oidc: true line from the setup step (the action ignores that
input) and bump node-version from '20' to a supported OIDC runtime (e.g., '22'
or '22.14.0'+) in the same setup block; ensure the existing explicit
NODE_AUTH_TOKEN: "" environment entry remains unchanged (required for OIDC) and
leave the yarn OIDC-related config (clearing npmAuthIdent and disabling
npmAlwaysAuth) as-is.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04574014-47a2-4114-973b-50919e96af55
📒 Files selected for processing (1)
.github/workflows/publish-rc.yml
harrytiendanube
previously approved these changes
Apr 24, 2026
This reverts commit 91ea77b.
harrytiendanube
previously approved these changes
Apr 29, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts (1)
124-130:⚠️ Potential issue | 🔴 CriticalDon't log OTP credentials; exclude secrets from console output.
Publishing to npm with OTP (line 126) logs the full command string including the 2FA token, exposing the secret in CI logs and local terminal history. OTP credentials should never appear in console output.
Additionally, building the publish command as an interpolated string is fragile; pass arguments explicitly to avoid shell interpretation risks and improve clarity.
🔧 Proposed fix
-import { execSync } from "child_process"; +import { execFileSync } from "child_process"; @@ - const accessFlag = `--access ${access}`; - const tagFlag = `--tag ${tag}`; - const otpFlag = otp ? ` --otp=${otp}` : ""; - - const command = `npm publish --workspace=${packageName} ${accessFlag} ${tagFlag}${otpFlag}`; - - console.log(`📤 Publishing with command: ${command}`); - - execSync(command, { - stdio: "inherit", - }); + const args = [ + "publish", + `--workspace=${packageName}`, + "--access", + access, + "--tag", + tag, + ...(otp ? [`--otp=${otp}`] : []), + ]; + + console.log(`📤 Publishing with npm publish --workspace=${packageName}`); + + execFileSync("npm", args, { + stdio: "inherit", + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts` around lines 124 - 130, The code currently builds a single interpolated command string (const command) that includes otpFlag and logs it, exposing the OTP; replace this by constructing an argument array (e.g., ['publish', `--workspace=${packageName}`, ...] including otpFlag only as a separate element) and invoke child_process.execFileSync or spawnSync with that args array (shell: false) instead of execSync to avoid shell interpolation; remove or sanitize the console.log so it does not print otpFlag (log only non-sensitive parts like packageName and flags without OTP) and ensure exec/execFile/ spawn is called with stdio: 'inherit'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish-rc.yml:
- Around line 50-56: Reorder the Node setup steps so the custom dependency setup
action runs before the OIDC-enabled setup-node step: move the
./.github/actions/setup-node-deps invocation to run prior to
actions/setup-node@v4 (which finalizes registry and auth), ensuring the internal
setup-node@v3 call inside setup-node-deps handles Node/version/caching first and
the actions/setup-node@v4 call runs last to finalize registry-url and auth-oidc
configuration before publish.
In `@packages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts`:
- Line 126: The current console.log prints the full `command` string which can
leak `--otp=...`; before logging in npm-utils.ts, redact OTPs by replacing
patterns like `--otp=VALUE` and `--otp VALUE` with a masked placeholder (e.g.,
`--otp=*****`) and log that redacted string instead (use a small helper to run a
case-insensitive regex against the `command` variable and output
`redactedCommand` rather than the raw `command`).
---
Outside diff comments:
In `@packages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts`:
- Around line 124-130: The code currently builds a single interpolated command
string (const command) that includes otpFlag and logs it, exposing the OTP;
replace this by constructing an argument array (e.g., ['publish',
`--workspace=${packageName}`, ...] including otpFlag only as a separate element)
and invoke child_process.execFileSync or spawnSync with that args array (shell:
false) instead of execSync to avoid shell interpolation; remove or sanitize the
console.log so it does not print otpFlag (log only non-sensitive parts like
packageName and flags without OTP) and ensure exec/execFile/ spawn is called
with stdio: 'inherit'.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15c15585-c056-45e7-a68c-14cf32e479ef
📒 Files selected for processing (2)
.github/workflows/publish-rc.ymlpackages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts (1)
124-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact the OTP before logging this command.
Line 126 still prints the full publish command, so
--otp=...will end up in CI logs. This is the same leak risk previously called out and it remains unresolved.🔒 Proposed fix
- console.log(`📤 Publishing with command: ${command}`); + const safeCommand = otp + ? command.replace(/--otp=\S+/, "--otp=***") + : command; + console.log(`📤 Publishing with command: ${safeCommand}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts` around lines 124 - 126, The published command is logged with the raw otpFlag which leaks OTPs; update the logging to sanitize or remove the OTP before printing by constructing a sanitizedCommand (e.g., replace otpFlag value with a constant like '--otp=REDACTED' or omit otpFlag) and then console.log the sanitizedCommand instead of command; update the code around the variables command, otpFlag, packageName, accessFlag and tagFlag so the actual npm publish invocation still uses the original command but only the sanitizedCommand is logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts`:
- Around line 124-126: The published command is logged with the raw otpFlag
which leaks OTPs; update the logging to sanitize or remove the OTP before
printing by constructing a sanitizedCommand (e.g., replace otpFlag value with a
constant like '--otp=REDACTED' or omit otpFlag) and then console.log the
sanitizedCommand instead of command; update the code around the variables
command, otpFlag, packageName, accessFlag and tagFlag so the actual npm publish
invocation still uses the original command but only the sanitizedCommand is
logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3014c08-1c45-4b15-b0df-bb426eb782b2
📒 Files selected for processing (2)
.github/workflows/publish-rc.ymlpackages/core/scripts/src/packagePublisher/core/npm-utils/npm-utils.ts
Contributor
|
🚀✨ Your Storybook preview is ready! Happy reviewing! 🎉 |
|
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
3 participants
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.
Type
Screenshots
Changes proposed ✔️
Summary by CodeRabbit
New Features
Documentation