🐛 Fixed theme settings reset when editing Source or Casper - #29839
Merged
Merged
Conversation
…copy fixes https://linear.app/ghost/issue/ONC-1936/theme-editor-saving-an-edited-default-theme-as-a-copy-resets-all - custom theme settings are stored against the theme name, so the copy the theme editor creates when saving an edited default theme started from defaults and activating it unexpectedly reverted the site's design - the editor now asks the API to seed the copy's settings from the theme it was edited from (`?copy_settings_from`), and the existing activation sync reconciles the copied values against the new theme's settings definition - copying no-ops when the destination theme already has stored settings so saving over an existing theme never clobbers its customisations
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 58s | View ↗ |
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 6m 31s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 5s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 47s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 40s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 23s | View ↗ |
nx run-many -t test:unit -p @tryghost/admin,ghost |
✅ Succeeded | 31s | View ↗ |
nx run-many -t lint -p @tryghost/admin,ghost,gh... |
✅ Succeeded | 22s | View ↗ |
Additional runs (2) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-10 09:42:11 UTC
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTheme save-as uploads now include the original theme name. The upload endpoint passes this value to theme installation. Theme installation validates the source and copies persisted custom settings to the renamed theme. Existing destination settings remain unchanged. Tests cover successful copying, validation failures, and rollback behavior. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ghost/core/core/server/services/themes/storage.js`:
- Around line 104-109: Make theme replacement recoverable across both affected
sites: in ghost/core/core/server/services/themes/storage.js lines 104-109,
update the settings-copy failure path around copySettingsBetweenThemes to remove
the newly saved theme, restore backupName when present, and rethrow; in
ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.js
lines 179-197, make the copied setting writes atomic or clean up all destination
rows created by a failed copy so partial settings cannot remain.
In `@ghost/core/test/e2e-api/admin/themes.test.js`:
- Around line 430-483: Update the “Can copy custom theme settings when uploading
a theme under a new name” test to declare tmpDir and perform its setup,
assertions, and cleanup inside try/finally. In finally, restore the source
theme, remove source-edited when it exists, and delete tmpDir recursively,
ensuring cleanup runs after any failure.
🪄 Autofix
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: aa91cced-cbba-4c1c-90c0-27942f770914
📒 Files selected for processing (7)
apps/admin/src/settings/app/components/settings/site/theme/theme-code-editor-modal.tsxapps/admin/src/settings/site/theme.acceptance.test.tsxghost/core/core/server/api/endpoints/themes.jsghost/core/core/server/services/themes/storage.jsghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.jsghost/core/test/e2e-api/admin/themes.test.jsghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29839 +/- ##
==========================================
- Coverage 75.58% 75.41% -0.17%
==========================================
Files 1613 1606 -7
Lines 142620 140427 -2193
Branches 17644 17398 -246
==========================================
- Hits 107805 105909 -1896
+ Misses 33764 33442 -322
- Partials 1051 1076 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ref https://linear.app/ghost/issue/ONC-1936/theme-editor-saving-an-edited-default-theme-as-a-copy-resets-all - a failure part-way through copying settings rows could leave a partial copy behind that a retry would then treat as complete - the copy now removes any rows it created before rethrowing, and runs before the theme's files are touched so a failed copy leaves the installed themes exactly as they were - the settings-copy e2e test now cleans up its temp dir and restores the active theme in a finally block so a failure can't leak state into other tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.js`:
- Around line 200-206: Update the settings-copy operation in the relevant
service method to execute all destination inserts through a single database
transaction, using the repository’s existing transaction API and passing its
transaction context to each insert. Roll back the transaction when any insert
fails and rethrow the original error; avoid relying on the catch-block cleanup
loop so retries cannot encounter partial destination records.
🪄 Autofix
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: b96d24a8-cc62-425a-bc87-c251e63e6b76
📒 Files selected for processing (4)
ghost/core/core/server/services/themes/storage.jsghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.jsghost/core/test/e2e-api/admin/themes.test.jsghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js
- ghost/core/test/e2e-api/admin/themes.test.js
- ghost/core/core/server/services/themes/storage.js
ref https://linear.app/ghost/issue/ONC-1936/theme-editor-saving-an-edited-default-theme-as-a-copy-resets-all - the previous delete-on-failure cleanup could itself fail part-way, leaving partial rows that make later copy attempts skip the copy - running all inserts in one transaction means a failed copy can never leave anything behind, regardless of which write fails
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js (1)
68-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that copied rows use the transaction context.
The stub rolls back all writes even when
add()does not receive{transacting}. The test can pass if the production code stops forwarding the transaction context, but production writes would then not participate in the transaction.
ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js#L68-L77: expose a transaction sentinel that represents the active transaction.ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js#L544-L557: assert that eachadd()call receives the active transaction sentinel inoptions.transacting.🤖 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 `@ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js` around lines 68 - 77, Update ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js:68-77 so transaction() exposes a distinct active transaction sentinel through its callback context, then update ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js:544-557 to assert every add() call receives that sentinel as options.transacting. Use the existing transaction test setup and preserve rollback behavior.
🤖 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
`@ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.js`:
- Around line 191-201: The custom theme settings copy transaction does not
atomically protect the destination from concurrent inserts. Update the flow
around the repository transaction and destination-existence check to enforce
uniqueness for each (theme, key) pair using a database constraint or appropriate
locking, and add a test that runs concurrent copies and verifies no duplicate
destination rows are created.
---
Nitpick comments:
In `@ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js`:
- Around line 68-77: Update
ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js:68-77 so
transaction() exposes a distinct active transaction sentinel through its
callback context, then update
ghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js:544-557
to assert every add() call receives that sentinel as options.transacting. Use
the existing transaction test setup and preserve rollback behavior.
🪄 Autofix
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: 5d804c07-96be-4ff8-a85f-81b6199046fe
📒 Files selected for processing (3)
ghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-bread-service.jsghost/core/core/shared/custom-theme-settings-cache/custom-theme-settings-service.jsghost/core/test/unit/shared/custom-theme-settings-cache/service.test.js
ref https://linear.app/ghost/issue/ONC-1936/theme-editor-saving-an-edited-default-theme-as-a-copy-resets-all - the destination-empty check ran outside the copy transaction, so two concurrent copies could both see an empty destination and insert duplicate (theme, key) rows - the check now runs inside the transaction with a forUpdate lock, which serialises concurrent copies on MySQL; the second one then sees the copied rows and no-ops - a schema-level unique constraint was considered but needs a migration with dedupe handling and matches the existing check-then-insert shape of the activation sync, so it's left out of scope here
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.
fixes https://linear.app/ghost/issue/ONC-1936/
Editing a default theme (Source/Casper) in the built-in theme editor forces a "save as new theme" flow because default themes can't be overwritten. Custom theme settings are stored against the theme name, so the newly-named copy starts with the theme's default settings — the moment it was activated, all customised design settings (fonts, colours, header styles, etc.) silently reverted to defaults and unexpectedly broke the site's appearance.
Fixed it so the editor asks the API to carry the settings over:
POST /themes/uploadnow accepts an optionalcopy_settings_fromquery option. After the theme is stored, the custom theme settings service duplicates the source theme's stored settings under the new theme name, so activating the copy keeps the site's design.copy_settings_fromwhenever a save results in a new theme name — the forced save-as for default themes, and any rename of a custom theme.