Changed advanced settings dialogs to controlled components - #30014
Merged
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 5m 55s | View ↗ |
nx run-many -t test:unit -p @tryghost/admin |
✅ Succeeded | 2m 35s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 22s | View ↗ |
nx run-many -t lint -p @tryghost/admin,ghost-mo... |
✅ Succeeded | 1m 18s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 19s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-18 14:20:19 UTC
no ref The code injection, webhook, labs YAML editor, feature-toggle confirmation and universal import dialogs were the last NiceModal.create dialogs under settings/advanced. They now take an onClose prop and are rendered by the component that opens them, using SettingsModal's controlled onClose path that the routed settings dialogs already use. This removes five of the remaining NiceModal call sites so the provider and dependency can be torn down once the theme and membership dialogs follow. Dialogs opened from inside a settings group render through a small settings dialog portal whose host sits beside the settings layout. The settings groups and the fixed content wrapper each open a stacking context, so a SettingsModal rendered in place paints below the exit button and neighbouring groups; the portal keeps it where NiceModal's placeholder used to render. The webhook dialog opens from inside a routed dialog and does not need it. The universal importer and the automations feature-toggle confirmation had no acceptance coverage, so tests were added for both, and the code injection test now checks that nothing paints over the open dialog.
9larsons
force-pushed
the
slars/settings-advanced-dialogs-controlled
branch
from
August 17, 2026 14:28
4ff0cee to
1643384
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 6 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
🧰 Additional context used📓 Path-based instructions (6)**/*.{ts,tsx}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
apps/admin/**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,mts,cts}⚙️ CodeRabbit configuration file
Files:
apps/{admin,activitypub,admin-x-framework,shade}/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
**/*{.,-}{test,spec}.{js,jsx,ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (4)📚 Learning: 2026-04-30T10:51:48.759ZApplied to files:
📚 Learning: 2026-07-21T19:57:01.324ZApplied to files:
📚 Learning: 2026-06-04T15:15:20.265ZApplied to files:
📚 Learning: 2026-08-08T20:30:54.860ZApplied to files:
🔇 Additional comments (18)
WalkthroughThe settings application now provides a shared Merge Risk: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
no ref Preserved controlled modal ownership while retaining the new CSV import support and its tests.
9larsons
enabled auto-merge (squash)
August 18, 2026 14:13
9larsons
added a commit
that referenced
this pull request
Aug 18, 2026
no ref Uses the settings `DialogPortal` from #30014. The custom field, welcome email and welcome email customize dialogs were the last `NiceModal.create` dialogs in the membership and email settings areas. They now take an `onClose` prop and are rendered by the component that opens them: the two `SettingsModal` dialogs through the settings dialog portal so they paint above the settings chrome, and the customize dialog directly, since its `EmailDesignModal` is already a portalled Shade `Dialog`. With these converted (plus #30014 and #30015), no settings dialog is created or shown through NiceModal any more, which clears the way to remove the provider, the dependency and the compatibility bridges.
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.
no ref
The code injection, webhook, labs YAML editor, feature-toggle confirmation and universal import dialogs were the last
NiceModal.createdialogs undersettings/advanced. They now take anonCloseprop and are rendered by the component that opens them, usingSettingsModal's controlledonClosepath that the routed settings dialogs already use. This removes five of the remaining NiceModal call sites in settings so the provider and dependency can be torn down once the theme and membership dialogs follow.Dialogs opened from inside a settings group render through a small settings
DialogPortalwhose host sits beside the settings layout (next to<Outlet />). The settings groups (relative z-10) and the fixed content wrapper each open a stacking context, so aSettingsModalrendered in place paints below the exit-settings button and neighbouring groups; the portal keeps it where NiceModal's placeholder used to render. The webhook dialog opens from inside a routed dialog and does not need it.Verification
pnpm test:acceptance src/settings/advanced— 39/39 (three new tests: universal import upload + cancel, automations feature-toggle confirmation; the code injection test now also samples the viewport to assert nothing paints over the open dialog — it fails without the portal)tsc --noEmitand eslint clean onapps/admin