✨ Added a built-in theme code editor#27656
Open
JohnONolan wants to merge 5 commits intoTryGhost:mainfrom
Open
Conversation
Contributor
WalkthroughAdds 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)
Review rate limit: 3/5 reviews remaining, refill in 23 minutes and 17 seconds. 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
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.
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
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.
JohnONolan
force-pushed
the
codex/theme-editor-core
branch
from
3f8f5fc to
0f262ad
Compare
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.
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.17% 73.16% -0.01%
==========================================
Files 1561 1561
Lines 127051 127051
Branches 15383 15380 -3
==========================================
- Hits 92970 92961 -9
- Misses 33105 33133 +28
+ Partials 976 957 -19
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:
|
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
2 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