✨ Added a built-in theme code editor#27656
Merged
Merged
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:
Hidden review stack artifactWalkthroughAdds a browser-based theme editor and utilities: new default-exported ThemeCodeEditorModal and a theme-editor-utils module (types, ZIP extraction/packing, diffing, path normalization, rename mapping, archive limits, and errors). UI and routing updated to support 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (3)
apps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.tsx (1)
129-141: ⚡ Quick winExtract the editor-entry routing logic into a shared helper/hook.
This
handleEditCodeflow duplicates the same limit-check + route-building logic inapps/admin-x-settings/src/components/settings/site/change-theme.tsx. Centralizing it will prevent entry-point drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.tsx` around lines 129 - 141, handleEditCode duplicates the same limit-check and route-building flow found in change-theme.tsx; extract this into a shared helper or hook (e.g., openThemeEditor or useThemeEditorEntry) that encapsulates calling checkThemeLimitError(isDefaultOrLegacyTheme(...) ? '.' : theme.name), showing LimitModal with the same onOk handler, and calling updateRoute with the encoded path; replace the local handleEditCode in advanced-theme-settings.tsx and the equivalent code in change-theme.tsx to call the new helper, exporting it from a common module so both components reuse the exact same logic and avoid drift.apps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsx (2)
568-590: ⚡ Quick winConsider adding dependencies or using a ref for the keyboard handler.
This
useEffecthas no dependency array, causing the event listener to be added and removed on every render. While this ensureshandleSavealways uses current values, it's inefficient.A cleaner approach would be to use a ref for the handler or wrap
handleSaveinuseCallback:♻️ Suggested refactor using ref pattern
+const handleSaveRef = useRef(handleSave); +useEffect(() => { + handleSaveRef.current = handleSave; +}); + useEffect(() => { const handleKeydown = (event: KeyboardEvent) => { if ((event.metaKey || event.ctrlKey) && event.key.toLowerCase() === 's') { event.preventDefault(); - void handleSave(); + void handleSaveRef.current(); return; } if (event.key !== 'Escape') { return; } event.preventDefault(); event.stopPropagation(); event.stopImmediatePropagation(); }; window.addEventListener('keydown', handleKeydown, true); return () => { window.removeEventListener('keydown', handleKeydown, true); }; -}); +}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsx` around lines 568 - 590, The effect attaching handleKeydown is missing a dependency array causing add/remove on every render; change it to stable behavior by either (a) wrapping handleSave in useCallback and providing a proper dependency array to useEffect, or (b) keep handleSave as-is but move the handler into a ref (e.g., handlerRef) and read handlerRef.current inside handleKeydown so useEffect can pass an empty dependency array; update the effect that defines handleKeydown and the cleanup removeEventListener accordingly to reference the stable handler.
943-958: 💤 Low valueConsider defensive check for malformed success response.
If
response.json()fails butresponse.okis true (unlikely edge case of server returning invalid JSON on success), accessingdata.themes[0]on line 958 would throw.🛡️ Optional defensive fix
const data = await response.json().catch(() => null) as ThemesInstallResponseType & {errors?: FatalErrors}; if (!response.ok) { if (response.status === 422 && data?.errors) { NiceModal.show(InvalidThemeModal, { title: 'Invalid Theme', prompt: <>Fix the validation errors below and try saving again.</>, fatalErrors: data.errors as FatalErrors }); return; } throw new Error((data as {errors?: Array<{message?: string}>} | null)?.errors?.[0]?.message || 'Theme upload failed.'); } +if (!data?.themes?.[0]) { + throw new Error('Invalid response from server.'); +} + const uploadedTheme = data.themes[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsx` around lines 943 - 958, The success path assumes valid JSON and a populated themes array, but data can be null or malformed (response.json() may fail), so after the response.ok check and before using data.themes[0] validate that data is non-null and that Array.isArray(data.themes) and data.themes.length>0; if the check fails, throw a descriptive Error (or show an error modal similar to the existing error handling) so uploadedTheme is never read from an undefined value; apply this validation around the variable named data (typed as ThemesInstallResponseType) and before assigning uploadedTheme.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/admin-x-settings/src/components/settings/site/theme/theme-editor-utils.ts`:
- Around line 232-237: Reject archive entries with non-normalized or
path-traversal segments: in the extraction loop where zipPath, entry and
displayPath are used (rootPrefix, zipPath.slice(...)), normalize displayPath
using a POSIX-normalize equivalent and then skip/throw for any path that is
absolute (starts with '/'), contains '..' segments or resolves to empty or a
different traversal target (e.g., begins with '../' or contains '/..' or '\\'),
and also reject entries containing '.' segments that change structure (like
'a/./b') rather than trusting zipPath as-is; apply the same validation in the
other extraction loop referenced (the block around lines 260-268) so only
well-normalized, non-traversing paths are extracted or re-packed.
- Around line 153-160: getEntryUncompressedSize currently reads the private
JSZip field _data to get uncompressedSize; replace this brittle approach by
streaming/decompressing the entry via the public JSZip API (e.g.,
ZipObject.nodeStream() or ZipObject.async()) and count bytes from the
decompressed output to determine size, throwing
ThemeArchiveExtractionError('invalid_archive', invalidArchiveMessage) if the
per-entry or running total exceeds configured limits; update any callers that
assumed getEntryUncompressedSize returns a numeric size to await the
streaming/counting operation, remove reliance on ThemeArchiveEntry._data, and
keep the same error type/message when aborting on size violations.
---
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.tsx`:
- Around line 129-141: handleEditCode duplicates the same limit-check and
route-building flow found in change-theme.tsx; extract this into a shared helper
or hook (e.g., openThemeEditor or useThemeEditorEntry) that encapsulates calling
checkThemeLimitError(isDefaultOrLegacyTheme(...) ? '.' : theme.name), showing
LimitModal with the same onOk handler, and calling updateRoute with the encoded
path; replace the local handleEditCode in advanced-theme-settings.tsx and the
equivalent code in change-theme.tsx to call the new helper, exporting it from a
common module so both components reuse the exact same logic and avoid drift.
In
`@apps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsx`:
- Around line 568-590: The effect attaching handleKeydown is missing a
dependency array causing add/remove on every render; change it to stable
behavior by either (a) wrapping handleSave in useCallback and providing a proper
dependency array to useEffect, or (b) keep handleSave as-is but move the handler
into a ref (e.g., handlerRef) and read handlerRef.current inside handleKeydown
so useEffect can pass an empty dependency array; update the effect that defines
handleKeydown and the cleanup removeEventListener accordingly to reference the
stable handler.
- Around line 943-958: The success path assumes valid JSON and a populated
themes array, but data can be null or malformed (response.json() may fail), so
after the response.ok check and before using data.themes[0] validate that data
is non-null and that Array.isArray(data.themes) and data.themes.length>0; if the
check fails, throw a descriptive Error (or show an error modal similar to the
existing error handling) so uploadedTheme is never read from an undefined value;
apply this validation around the variable named data (typed as
ThemesInstallResponseType) and before assigning uploadedTheme.
🪄 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: 758a245a-bca3-4220-9731-cc55a7734da3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/admin-x-framework/src/test/acceptance.tsapps/admin-x-settings/package.jsonapps/admin-x-settings/src/components/providers/settings-router.tsxapps/admin-x-settings/src/components/settings/site/change-theme.tsxapps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsxapps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.tsxapps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsxapps/admin-x-settings/src/components/settings/site/theme/theme-editor-utils.tsapps/admin-x-settings/test/acceptance/site/theme.test.tsapps/admin-x-settings/test/unit/utils/theme-editor-utils.test.ts
JohnONolan
force-pushed
the
codex/theme-editor-core
branch
from
403b513 to
3f8f5fc
Compare
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)
apps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsx (1)
61-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel in-flight limit checks when the route changes.
Both async limit effects can still call
setState,modal.remove(), andupdateRoute()after the user has already navigated elsewhere. In particular, thecurrentPath !== 'theme/install'check afterawaitis reading the stale value captured when the effect started, so it does not prevent a late response from redirecting a different screen. Mirror theisCancelledpattern you already use in the editor-access effect.Also applies to: 156-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsx` around lines 61 - 89, The async effect in useEffect that defines checkIfThemeChangeAllowed can continue after navigation and mutate state or call modal.remove; fix it by implementing the same isCancelled pattern used in the editor-access effect: introduce a local let isCancelled = false at the top of the effect, pass that into checkIfThemeChangeAllowed and after every await (especially after calling await checkThemeLimitError()) check if isCancelled and return early if true so you don't call setThemeChangeError, setIsCheckingLimit, showThemeLimitModal, modal.remove or updateRoute when stale; also set isCancelled = true in the effect cleanup and apply the identical cancellation check to the other async limit-check effect (the one referenced around the 156-252 region) so all late promises respect currentPath/isThemeLimitCheckReady changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsx`:
- Around line 18-31: The parser currently accepts multi-segment or encoded-slash
names; update the logic around encodedThemeName (derived from
path.slice('theme/edit/'.length)) and the decodeURIComponent call to reject any
names containing a slash either before or after decoding: if encodedThemeName is
empty OR encodedThemeName.includes('/') OR
decodeURIComponent(encodedThemeName).includes('/') then return {themeName: null,
isInvalid: true}; otherwise return the decoded name as before. Apply this change
in the function that computes encodedThemeName/decodes it so malformed routes
are flagged invalid before rendering ThemeCodeEditorModal.
---
Outside diff comments:
In
`@apps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsx`:
- Around line 61-89: The async effect in useEffect that defines
checkIfThemeChangeAllowed can continue after navigation and mutate state or call
modal.remove; fix it by implementing the same isCancelled pattern used in the
editor-access effect: introduce a local let isCancelled = false at the top of
the effect, pass that into checkIfThemeChangeAllowed and after every await
(especially after calling await checkThemeLimitError()) check if isCancelled and
return early if true so you don't call setThemeChangeError, setIsCheckingLimit,
showThemeLimitModal, modal.remove or updateRoute when stale; also set
isCancelled = true in the effect cleanup and apply the identical cancellation
check to the other async limit-check effect (the one referenced around the
156-252 region) so all late promises respect currentPath/isThemeLimitCheckReady
changes.
🪄 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: 4a4da06f-a2c0-4a57-8b33-bf2ac83968cd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/admin-x-framework/src/test/acceptance.tsapps/admin-x-settings/package.jsonapps/admin-x-settings/src/components/providers/settings-router.tsxapps/admin-x-settings/src/components/settings/site/change-theme.tsxapps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsxapps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.tsxapps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsxapps/admin-x-settings/src/components/settings/site/theme/theme-editor-utils.tsapps/admin-x-settings/test/acceptance/site/theme.test.tsapps/admin-x-settings/test/unit/utils/theme-editor-utils.test.ts
✅ Files skipped from review due to trivial changes (3)
- apps/admin-x-settings/src/components/providers/settings-router.tsx
- apps/admin-x-settings/package.json
- apps/admin-x-settings/src/components/settings/site/change-theme.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/admin-x-framework/src/test/acceptance.ts
- apps/admin-x-settings/test/unit/utils/theme-editor-utils.test.ts
- apps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.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-editor-utils.ts
JohnONolan
force-pushed
the
codex/theme-editor-core
branch
from
3f8f5fc to
0f262ad
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin-x-settings/test/acceptance/site/theme.test.ts (1)
54-54: ⚡ Quick winEscape
themeNamebefore constructingRegExpin locator filter.Using raw
themeNameinnew RegExp(themeName, 'i')can mis-match if special regex characters appear in future fixtures/theme names.Suggested change
- const themeListItem = modal.getByTestId('theme-list-item').filter({hasText: new RegExp(themeName, 'i')}); + const escapedThemeName = themeName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const themeListItem = modal.getByTestId('theme-list-item').filter({hasText: new RegExp(escapedThemeName, 'i')});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/test/acceptance/site/theme.test.ts` at line 54, The locator construction uses new RegExp(themeName, 'i') which will break if themeName contains regex metacharacters; update the test to escape regex-special characters before building the RegExp used in modal.getByTestId('theme-list-item').filter({ hasText: new RegExp(...) }) — e.g. add and use an escapeRegExp helper and pass escapeRegExp(themeName) to RegExp (case-insensitive) when creating the filter for themeListItem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin-x-settings/test/acceptance/site/theme.test.ts`:
- Line 54: The locator construction uses new RegExp(themeName, 'i') which will
break if themeName contains regex metacharacters; update the test to escape
regex-special characters before building the RegExp used in
modal.getByTestId('theme-list-item').filter({ hasText: new RegExp(...) }) — e.g.
add and use an escapeRegExp helper and pass escapeRegExp(themeName) to RegExp
(case-insensitive) when creating the filter for themeListItem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38475e38-44aa-47cb-ba36-382c5cfc0bfd
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsxapps/admin-x-settings/test/acceptance/site/theme.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27656 +/- ##
==========================================
- Coverage 73.84% 73.81% -0.03%
==========================================
Files 1520 1521 +1
Lines 128308 128416 +108
Branches 15384 15386 +2
==========================================
+ Hits 94749 94794 +45
- Misses 32602 32687 +85
+ Partials 957 935 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
Theme editor PR — change list Review of the editor implementation plus the existing theme upload/download endpoints. Verified against the actual code on the branch. Grouped by confidence that the change is worth making. Worth doing (high confidence)
Real but minor (medium-high confidence)
Worth considering (real concern, fix path requires a judgement call)
Defensive / lower priority
|
ErisDS
force-pushed
the
codex/theme-editor-core
branch
from
0cb6c60 to
21726b0
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin-x-settings/test/acceptance/site/theme.test.ts (1)
50-59: 💤 Low valueConsider escaping the theme name for regex safety.
The static analysis tool flags
new RegExp(themeName, 'i')as a potential ReDoS risk. While this is test code with controlled inputs, escaping special regex characters would be a minor defensive improvement.♻️ Optional fix
async function openInstalledThemeEditor(page: Page, themeName: string) { const modal = await openChangeThemeModal(page); await modal.getByRole('tab', {name: 'Installed'}).click(); - const themeListItem = modal.getByTestId('theme-list-item').filter({hasText: new RegExp(themeName, 'i')}); + const escapedName = themeName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const themeListItem = modal.getByTestId('theme-list-item').filter({hasText: new RegExp(escapedName, 'i')}); await themeListItem.getByRole('button', {name: 'Menu'}).click(); await page.getByTestId('popover-content').getByRole('button', {name: 'Edit code'}).click(); return page.getByTestId('theme-code-editor-modal'); }🤖 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/test/acceptance/site/theme.test.ts` around lines 50 - 59, The filter uses new RegExp(themeName, 'i') which can misinterpret special regex chars; update openInstalledThemeEditor to escape themeName before constructing the RegExp (e.g., add a small helper escapeRegExp(name) that replaces /[-\/\\^$*+?.()|[\]{}]/g with '\\$&' and then use new RegExp(escapeRegExp(themeName), 'i') when creating the regex for themeListItem.filter); reference the existing function openInstalledThemeEditor and the RegExp(themeName, 'i') usage when applying this change.
🤖 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.
Nitpick comments:
In `@apps/admin-x-settings/test/acceptance/site/theme.test.ts`:
- Around line 50-59: The filter uses new RegExp(themeName, 'i') which can
misinterpret special regex chars; update openInstalledThemeEditor to escape
themeName before constructing the RegExp (e.g., add a small helper
escapeRegExp(name) that replaces /[-\/\\^$*+?.()|[\]{}]/g with '\\$&' and then
use new RegExp(escapeRegExp(themeName), 'i') when creating the regex for
themeListItem.filter); reference the existing function openInstalledThemeEditor
and the RegExp(themeName, 'i') usage when applying this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77650f8a-fa33-4401-884b-922f90e8495b
📒 Files selected for processing (10)
apps/admin-x-framework/src/test/acceptance.tsapps/admin-x-settings/package.jsonapps/admin-x-settings/src/components/providers/settings-router.tsxapps/admin-x-settings/src/components/settings/site/change-theme.tsxapps/admin-x-settings/src/components/settings/site/design-and-theme-modal.tsxapps/admin-x-settings/src/components/settings/site/theme/advanced-theme-settings.tsxapps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsxapps/admin-x-settings/src/components/settings/site/theme/theme-editor-utils.tsapps/admin-x-settings/test/acceptance/site/theme.test.tsapps/admin-x-settings/test/unit/utils/theme-editor-utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/admin-x-settings/src/components/providers/settings-router.tsx
- apps/admin-x-settings/test/unit/utils/theme-editor-utils.test.ts
- apps/admin-x-settings/src/components/settings/site/theme/theme-editor-utils.ts
- apps/admin-x-settings/src/components/settings/site/change-theme.tsx
- apps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsx
- apps/admin-x-framework/src/test/acceptance.ts
ErisDS
force-pushed
the
codex/theme-editor-core
branch
4 times, most recently
from
c38f857 to
00c989d
Compare
The routed editor now has to enforce theme entitlement checks itself so direct URLs cannot bypass plan limits, and rename/save-as flows need to preserve file invariants and the addressable editor route to avoid silent corruption or stale refresh targets.
Updated the theme settings acceptance suite to match the current editor copy and flows, and added coverage for active-theme entry, return routing, direct-route gating, and non-editable file messaging so editor regressions are caught in one place.
This adds a Ghost-native browser editor for installed themes so theme changes can happen directly inside settings instead of requiring a separate local workflow. The implementation keeps the existing theme upload/download contract, adds native routing and entry points, preserves binary assets during ZIP round-trips, and includes route/archive hardening so invalid URLs or oversized archives fail safely rather than breaking the settings UI.
This follows up on review feedback around hostile archive handling in the browser theme editor. The extractor now rejects non-normalized archive paths and enforces extracted-size limits using public JSZip read APIs instead of private metadata fields, which keeps the validation stable across dependency updates while preserving the editor hardening added earlier.
This follows up on review feedback about malformed editor routes. The parser now rejects encoded slashes before opening the editor, and the acceptance suite covers the direct-route fallback so malformed theme edit URLs return to a safe settings state instead of leaking through to the modal flow.
The closeEditor flow triggers a discard confirmation when changes are pending, but neither cancel nor confirm was exercised by tests. A review flagged this as a UX path easy to regress silently. The new test makes an edit then runs both branches: cancel keeps the editor open with the change intact; confirm tears it down. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Every other dep in this package.json uses an exact version; jszip was the lone caret-pinned entry, introduced alongside the theme editor. Aligning it with the convention so future pnpm installs will not silently bump to a new minor without a deliberate change. The lockfile diff also includes incidental cleanup pnpm applied on this install (a stale @tryghost/members-csv@2.0.5 entry that nothing references, and a couple of optional-dep flag tweaks). These are unrelated to the version pin. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Cmd+S / Escape handler was inside a useEffect with no dep array, so the global keydown listener was added and removed on every render of the theme editor — including every keystroke in the CodeMirror editor, because typing updates currentFiles state. The listener cannot have handleSave directly in its dep array either: handleSave closes over currentFiles, changes, and other state, so it is itself rebuilt on every keystroke. The literal "wrap in useCallback" fix would not reduce churn meaningfully — useCallback only returns a stable reference when all its deps are unchanged, and currentFiles changes per keystroke. Switched to the latest-ref pattern: the listener registers once on mount, and reads the freshest handleSave through a ref that is updated in a follow-up effect after each render. Same Cmd+S behaviour, no per-keystroke add/remove on the window. The mutable ref bypasses React's data flow but is the established workaround for "global event handler outside React scope needs to call into the component". Long term, React 19.2 (Oct 2025) shipped useEffectEvent as a stable API that wraps this exact pattern more ergonomically. This codebase is on React 18.3.1, so the ref pattern is the right fix today; the listener can migrate to useEffectEvent when admin-x-settings upgrades. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The editor was wrapping [oneDark, getLanguageExtension(path)] in Promise.all and spreading the result into the extensions array. oneDark is a static Extension, not a thenable; Promise.all coerced it via Promise.resolve() so it worked, but the shape of the code suggested both values were async. The .catch branch already had to re-list oneDark by itself, which gave away that only the language extension was actually async. Only getLanguageExtension is async (a dynamic import for the CodeMirror language module). Awaiting just that, and listing oneDark alongside the other static extensions in both the success and error branches, keeps the Promise types honest and removes the misleading parallel-load shape. No behaviour change; same extensions loaded in the same order. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The save-as input previously blocked only forward/back slashes and the
two default theme names. Everything else was sent to the server, where
BaseStorage.getSanitizedFileName silently replaces any character
outside [\w@.] with a dash — so a user typing "My Theme!" had their
theme stored as "my-theme-" with no visible explanation.
Replaced the deny-list with a positive allowlist matching
/^[a-z0-9][\w-]{0,63}$/ on the lower-cased input. This rejects null
bytes, leading dots, Unicode characters the server would replace, and
names over 64 chars — all with a single clear error instead of a
silent server-side rename. The default-theme guard stays separate so
"Built-in themes cannot be overwritten" still surfaces specifically.
Dots are deliberately excluded entirely (not just at the start of the
name). Real Ghost themes are kebab-case and have no legitimate use
for dots, while allowing them invites extension confusion ("mytheme.zip"
typed as a name) and Windows strips trailing dots silently when archives
are unzipped on deploy.
Added an acceptance test that exercises both rejection paths:
disallowed characters trigger the new format error, and built-in
theme names trigger the existing specific guard.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
theme-code-editor-modal.tsx had grown to 1,358 lines with two NiceModal definitions, the recursive file-tree renderer, the top toolbar, the full review-overlay modal, and a handful of helper types/constants all inline. The util layer was already separated; the UI side now matches. Extracted five sibling files plus a shared style constants module: - theme-editor-confirm-modal.tsx: ThemeEditorConfirmModal NiceModal - theme-editor-input-modal.tsx: ThemeEditorInputModal NiceModal - theme-file-tree.tsx: ThemeFileTree component (also owns the TreeNode type, SelectedNode type, buildTree, sortTreeNodes, and the fileActionButtonClass style — all only used here) - theme-editor-toolbar.tsx: ThemeEditorToolbar component (header bar with theme name, modified-count badge, Close, Save) - theme-review-modal.tsx: ThemeReviewModal component for the "All changes" overlay, plus the ReviewItem type and the buildReviewItems / formatReviewSummary helpers. selectedReviewPath state and its auto-select effect moved inside since they are private to the review flow — the parent now only needs reviewItems, an onClose callback, onOpenInEditor, and onRevert. - theme-editor-styles.ts: button-class strings shared between the toolbar and the review pane Main file dropped from 1,358 to ~947 lines. Behaviour is unchanged; the 27 existing theme acceptance tests, 169 admin-x-settings unit tests, and tsc all pass without modification — these tests already exercise every extracted surface (typing in the file tree, both NiceModals via the save/discard/rename flows, Cmd+S and click Save on the toolbar, opening the review overlay and reverting from it), so the existing suite is the safety net for this refactor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
getReturnRouteFromHash was passing the ?from= query parameter directly into updateRoute when the editor closed. updateRoute can only navigate within the admin SPA (it writes to window.location.hash), so this is not an open redirect — but a crafted URL could still bounce a logged-in admin onto an unrelated admin route after closing the editor, which is a phishing-shape concern even if not exploitable on its own. Constrained the return route to a small allowlist of prefixes that cover every legitimate caller (the editor is only ever opened from a theme/* or design/* route today). Empty from= is kept as a pass- through because it is the legitimate signal that the editor was opened from the settings root (/#/settings), and we still want to return there. Behaviour: - ?from missing → null → falls back to the safe default 'design/change-theme' - ?from= (empty) → '' → returns to /#/settings (unchanged from today) - ?from=theme[/...] or ?from=design[/...] → returned as-is - ?from=anything-else → null → falls back to the safe default Added two acceptance tests pairing the malicious and legitimate cases. Test-first: the malicious-from= test was failing before the implementation landed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
new URL(hash, sameOrigin) throws on truly malformed input — e.g. a
hash that starts with "://" or another unparseable protocol-like
prefix. The Ghost admin SPA never produces such hashes today, so this
is purely defensive: a manual hash edit in devtools, a future routing
change, or a partially-decoded value during a race could trip it.
Without a guard, the throw escapes into closeEditor and would crash
the editor unmount.
Wrapped the new URL call in try/catch, returning null on failure so
the caller's existing fallback ('design/change-theme') still applies.
No test added. Reaching the throw branch via the admin UI is not
possible today (the SPA controls hash format), and the alternative —
extracting parseReturnRoute as a pure exported helper just to mock
window.location — is a heavier refactor than the recommendation
called for. If a real failure case turns up, that refactor + a unit
test is the right follow-up.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The review recommendation called for extracting recurring hex colours
into local constants. After the earlier component split, the hex
literals are scattered across smaller files but a handful of full
tailwind class strings still repeat within individual files. Those are
the practical extraction targets.
theme-review-modal.tsx (4 / 2 / 2 duplicates):
- previewBlockClass for the diff/preview <pre> element
- previewSectionLabelClass for the "Before"/"After" labels
- previewEmptyStateClass for the "Select a changed file" / binary-file
placeholders
theme-code-editor-modal.tsx (3 duplicates):
- editorPaneEmptyStateClass for the three right-pane placeholder states
("no file selected", "folder selected", "this file cannot be edited")
The hex values themselves stay inlined where they are used in tailwind
arbitrary-value classes. Replacing them with JS constants would break
tailwind's build-time class extraction; the right substitute is design
tokens in the shade design system, which is broader follow-up work
flagged in the original recommendation ("Candidate for a design-token
follow-up") and out of scope here.
No behaviour change; all 29 theme acceptance tests pass without
modification.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the new server-side upload caps (PR TryGhost#27903) reject a save, the response carries a structured Ghost error with a code (COMPRESSED_TOO_LARGE / ENTRY_TOO_LARGE / TOTAL_TOO_LARGE) and errorDetails (entryName, observedBytes, limitBytes). The editor was throwing the parsed message into the generic useHandleError() pipeline, which collapsed it to "Something went wrong, please try again." The user had no way to know which file was too large or what the cap was. Intercept the three size-limit codes before the throw, format the errorDetails into a user-friendly toast, and skip the generic handler: - COMPRESSED_TOO_LARGE → "The theme archive is too large to upload (max X MB)." - ENTRY_TOO_LARGE → "The file 'foo.hbs' is too large (max X MB per file)." - TOTAL_TOO_LARGE → "The theme contents exceed the maximum allowed size of X MB." All other non-422 failures still fall through to the existing throw + handleError path. The 422 InvalidThemeModal branch (gscan validation errors) is unchanged. Test-first: a new acceptance test mocks /themes/upload/ to return a 415 with an ENTRY_TOO_LARGE shape and asserts the toast mentions the offending file and the limit (not the generic message). The test was failing before this change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ErisDS
force-pushed
the
codex/theme-editor-core
branch
from
00c989d to
f68c4eb
Compare
|
🚀 |
ErisDS
added a commit
that referenced
this pull request
May 15, 2026
Escapes theme names before constructing the installed-theme Playwright locator regexp. Follows up on CodeRabbit nitpick from #27656 so future fixture names with regexp metacharacters match literally.
Contributor
|
@JohnONolan how does this feature interact with file-system themes? I.e. themes stored in the |
Member
Author
|
Yep |
Contributor
|
@JohnONolan it also, unfortunately, seems to destroy the Is this behaviour intended? It would be super cool if the theme could continue to be a git repo, so that changes made inside the Ghost editor could be pushed to the remote repo! |
Member
Author
|
Sounds like a bug - if you can open an issue with repro steps that would be great! |
This was referenced May 29, 2026
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
4 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.
What
This adds a Ghost-native theme code editor inside settings.
Based on community work originally done here by @muratcorlu:
https://github.com/synapsmedia/ghost-theme-editor