Skip to content

🎨 Improved theme validation modals#28748

Merged
ErisDS merged 1 commit into
mainfrom
theme-validation-modals
Jun 19, 2026
Merged

🎨 Improved theme validation modals#28748
ErisDS merged 1 commit into
mainfrom
theme-validation-modals

Conversation

@ErisDS

@ErisDS ErisDS commented Jun 19, 2026

Copy link
Copy Markdown
Member

Why

Theme validation outcomes weren't clear: blocking and non-blocking issues were mixed together, and titles like "Upload successful with errors" muddled whether an upload had actually worked. This makes the outcome obvious — what blocked the theme vs. what's optional to fix later.

This is the production version of the approved spike #28254 (visual design is unchanged from that PR's screenshots), with the spike scaffolding removed.

What

  • Failed upload/save/activation modals show the blocking issue(s) first, with a collapsible "non-blocking issues" disclosure beneath.
  • Successful upload/install modals lead with the success outcome and keep optional compatibility issues secondary.
  • A shared theme-validation-details view drives both, so the modals stay consistent.
  • ConfirmationModalContent gains stickyFooter + ReactNode titles so long validation content can't hide the modal actions.

Behaviour is unchanged

  • problem.fatal === true is the only blocking discriminator. By default in production only fatal errors block; errors/warnings/recommendations are grouped as non-blocking (collapsed in production, expanded in development).
  • GScan levels and the API data shape are unchanged. Severity badges map straight from problem.level (error/warning/recommendation) with no remapping — a problem's reported level is what's shown.
  • ThemeProblem.level is widened to include recommendation to match GScan's actual three levels (it previously only typed error/warning).

Difference vs. the spike

  • The Storybook spike and the live in-Admin scenario toolbar are dropped — the validation modal is an Admin feature, not a Shade component.
  • Severity is no longer bumped down a level; it reflects the real GScan level.

Testing

  • Lint clean on changed files — admin-x-settings, admin-x-framework, admin-x-design-system (Node 22.18.0)
  • Typecheck clean for the theme validation files
  • pnpm dev boots and serves Admin
  • No automated test added — the change is presentational (modal layout/copy); the blocking logic is unchanged.

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The PR refactors theme validation UI across upload, install, and activation flows. A new theme-validation-details.tsx module introduces typed helpers (getIssuesFromFatalErrors, getIssuesFromInstalledTheme) and shared UI components (ValidationProblemCard, ThemeValidationDetailsDisclosure, ErrorTextCard, OutcomeBanner). InvalidThemeModal and ThemeInstalledModal are rewritten to use these components, removing their inline rendering logic. Design-system Modal and ConfirmationModal have their title prop widened to React.ReactNode and gain a stickyFooter prop. ThemeProblem's Level union gains 'recommendation'. Modal titles and prompt copy are updated across all call sites, and acceptance tests are adjusted to match.

Possibly related PRs

  • TryGhost/Ghost#27656: Both PRs modify the handleActivate fatalErrors branch in advanced-theme-settings.tsx, with one updating the activation-failure modal title/prompt and the other adjusting the fatalErrors type cast.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: improving theme validation modals with clearer outcome presentation and better UI organization.
Description check ✅ Passed The description clearly explains the PR's motivation, implementation details, and scope, relating directly to the changeset focused on theme validation modal improvements.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch theme-validation-modals

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud

nx-cloud Bot commented Jun 19, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 7e58bd8

Command Status Duration Result
nx run @tryghost/admin-x-settings:test:acceptance ✅ Succeeded 10m 23s View ↗
nx build @tryghost/announcement-bar ✅ Succeeded <1s View ↗
nx build @tryghost/sodo-search ✅ Succeeded <1s View ↗
nx build @tryghost/portal ✅ Succeeded <1s View ↗
nx build @tryghost/admin-toolbar ✅ Succeeded <1s View ↗
nx build @tryghost/comments-ui ✅ Succeeded <1s View ↗
nx build @tryghost/activitypub ✅ Succeeded 1s View ↗
nx build @tryghost/signup-form ✅ Succeeded <1s View ↗
Additional runs (8) ✅ Succeeded ... View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-06-19 14:08:24 UTC

Redesigned the invalid/installed/code-editor theme validation modals
around a single shared details view: blocking errors are shown prominently
with a collapsible "non-blocking issues" disclosure (expanded in
development, collapsed in production).

The blocking rule is unchanged — only problems flagged `fatal` block saving
or activation; everything else is grouped as non-blocking. Severity badges
map straight from gscan's `level` (error/warning/recommendation) with no
remapping, so a problem's reported level is what's shown.
@ErisDS ErisDS force-pushed the theme-validation-modals branch from d8cf993 to 7e58bd8 Compare June 19, 2026 13:55

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
apps/admin-x-settings/src/components/settings/site/theme/theme-installed-modal.tsx (1)

11-17: ⚡ Quick win

Drop or wire the prompt prop to avoid a dead public API.

ThemeInstalledModal still declares prompt (Line 13), but it is not destructured or rendered. That leaves callers passing copy that has no effect and makes this component contract misleading.

Suggested cleanup
 const ThemeInstalledModal: React.FC<{
     title: string
-    prompt: ReactNode
     installedTheme: InstalledTheme;
     validationDetailsDefaultOpen?: boolean;
     onActivate?: () => void;
-}> = ({title, installedTheme, validationDetailsDefaultOpen, onActivate}) => {
+}> = ({title, installedTheme, validationDetailsDefaultOpen, onActivate}) => {

Also applies to: 53-72

🤖 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
`@apps/admin-x-settings/src/components/settings/site/theme/theme-installed-modal.tsx`
around lines 11 - 17, The ThemeInstalledModal component declares the prompt prop
in its type definition on line 13, but it is not destructured from the
component's props parameter on line 17 and is never rendered anywhere in the
component. Either remove the prompt prop from the component's type definition if
it is not needed, or add it to the destructured props in the function signature
and render it at the appropriate location in the component (likely in the 53-72
range) to make this a functional part of the component's public API.
apps/admin-x-settings/src/components/settings/site/theme/theme-validation-details.tsx (1)

114-117: ⚡ Quick win

Expose expanded state on the disclosure buttons.

Both controls toggle hidden content visually, but screen readers do not get the open/closed state. Add aria-expanded to the toggles.

♿ Proposed accessibility tweak
             <button
+                aria-expanded={expanded}
                 className='flex w-full items-start justify-between gap-3 text-left'
                 type='button'
                 onClick={() => setExpanded(!expanded)}
@@
             <button
+                aria-expanded={open}
                 className='flex w-full items-center justify-between gap-3 rounded-lg border border-border bg-background p-4 text-left transition-colors hover:bg-muted/40'
                 type='button'
                 onClick={() => setOpen(!open)}

Also applies to: 164-167

🤖 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
`@apps/admin-x-settings/src/components/settings/site/theme/theme-validation-details.tsx`
around lines 114 - 117, The disclosure buttons that toggle expanded content are
missing the aria-expanded attribute needed for screen reader accessibility. Add
aria-expanded attribute to both button elements (the ones with onClick={() =>
setExpanded(!expanded)} and the similar toggle at the second location mentioned)
and bind the attribute value to the expanded state variable so it properly
communicates the open/closed state to assistive technologies.
🤖 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
`@apps/admin-x-settings/src/components/settings/site/theme/theme-validation-details.tsx`:
- Line 90: The problem.details field on line 90 and problem.rule field on line
124 are rendered directly via dangerouslySetInnerHTML without sanitization.
Since these HTML strings come from the Ghost theme validation API, they could
contain malicious scripts that execute in the Admin panel context. Sanitize both
problem.details and problem.rule using DOMPurify before passing them to
dangerouslySetInnerHTML to prevent potential XSS attacks, or alternatively strip
the HTML entirely and render as plain text if HTML formatting is not necessary.

---

Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/site/theme/theme-installed-modal.tsx`:
- Around line 11-17: The ThemeInstalledModal component declares the prompt prop
in its type definition on line 13, but it is not destructured from the
component's props parameter on line 17 and is never rendered anywhere in the
component. Either remove the prompt prop from the component's type definition if
it is not needed, or add it to the destructured props in the function signature
and render it at the appropriate location in the component (likely in the 53-72
range) to make this a functional part of the component's public API.

In
`@apps/admin-x-settings/src/components/settings/site/theme/theme-validation-details.tsx`:
- Around line 114-117: The disclosure buttons that toggle expanded content are
missing the aria-expanded attribute needed for screen reader accessibility. Add
aria-expanded attribute to both button elements (the ones with onClick={() =>
setExpanded(!expanded)} and the similar toggle at the second location mentioned)
and bind the attribute value to the expanded state variable so it properly
communicates the open/closed state to assistive technologies.
🪄 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: CHILL

Plan: Pro

Run ID: 0a507859-7166-41d1-9e4f-43b57e6ab6d0

📥 Commits

Reviewing files that changed from the base of the PR and between d8cf993 and 7e58bd8.

📒 Files selected for processing (11)
  • apps/admin-x-design-system/src/global/modal/confirmation-modal.tsx
  • apps/admin-x-design-system/src/global/modal/modal.tsx
  • apps/admin-x-framework/src/api/themes.ts
  • apps/admin-x-settings/src/components/settings/site/theme-modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/invalid-theme-modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/theme-installed-modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/theme-validation-details.tsx
  • apps/admin-x-settings/test/acceptance/site/design.test.ts
  • apps/admin-x-settings/test/acceptance/site/theme.test.ts
✅ Files skipped from review due to trivial changes (3)
  • apps/admin-x-settings/test/acceptance/site/theme.test.ts
  • apps/admin-x-framework/src/api/themes.ts
  • apps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
  • apps/admin-x-design-system/src/global/modal/confirmation-modal.tsx
  • apps/admin-x-design-system/src/global/modal/modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/invalid-theme-modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme-modal.tsx

@ErisDS ErisDS merged commit b89f71a into main Jun 19, 2026
48 checks passed
@ErisDS ErisDS deleted the theme-validation-modals branch June 19, 2026 19:13
@ErisDS ErisDS mentioned this pull request Jun 19, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant