From 93cbb94b90987eb5f410858436a14a2173d0bfb1 Mon Sep 17 00:00:00 2001 From: Chris Raible Date: Fri, 16 Aug 2024 12:09:11 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20a=20bug=20causing=20new?= =?UTF-8?q?=20drafts=20to=20only=20save=20if=20the=20title=20is=20populate?= =?UTF-8?q?d=20(#20769)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/tryghost/issue/ONC-253/drafts-only-save-if-the-title-is-populated - A [commit](https://github.com/TryGhost/Ghost/commit/c8ba9e8027bd8269f8fb0ff3303da6d1e34b9696) in `v5.89.1` introduced a bug that caused new drafts to only save if the post title was populated, causing potential data loss if a user is working on a new draft without setting the title. - This commit reverts the one that introduced this bug to prevent data loss. This reverts commit c8ba9e8027bd8269f8fb0ff3303da6d1e34b9696. --- .../components/gh-koenig-editor-lexical.hbs | 2 - .../components/gh-koenig-editor-lexical.js | 7 -- .../app/components/gh-post-settings-menu.hbs | 1 - .../app/components/koenig-lexical-editor.js | 57 +++++++--------- .../app/components/modal-post-history.hbs | 1 - .../app/components/modal-post-history.js | 7 -- ghost/admin/app/controllers/lexical-editor.js | 66 ++++++++----------- ghost/admin/app/models/post.js | 3 - ghost/admin/app/templates/lexical-editor.hbs | 3 - .../tests/unit/controllers/editor-test.js | 45 +------------ 10 files changed, 51 insertions(+), 141 deletions(-) diff --git a/ghost/admin/app/components/gh-koenig-editor-lexical.hbs b/ghost/admin/app/components/gh-koenig-editor-lexical.hbs index 3ad4aae7a0..af3524b998 100644 --- a/ghost/admin/app/components/gh-koenig-editor-lexical.hbs +++ b/ghost/admin/app/components/gh-koenig-editor-lexical.hbs @@ -95,9 +95,7 @@ @placeholder={{@bodyPlaceholder}} @cardConfig={{@cardOptions}} @onChange={{@onBodyChange}} - @updateSecondaryInstanceModel={{@updateSecondaryInstanceModel}} @registerAPI={{this.registerEditorAPI}} - @registerSecondaryAPI={{this.registerSecondaryEditorAPI}} @cursorDidExitAtTop={{if this.feature.editorExcerpt this.focusExcerpt this.focusTitle}} @updateWordCount={{@updateWordCount}} @updatePostTkCount={{@updatePostTkCount}} diff --git a/ghost/admin/app/components/gh-koenig-editor-lexical.js b/ghost/admin/app/components/gh-koenig-editor-lexical.js index be28f73a5f..2319ae9c8a 100644 --- a/ghost/admin/app/components/gh-koenig-editor-lexical.js +++ b/ghost/admin/app/components/gh-koenig-editor-lexical.js @@ -15,7 +15,6 @@ export default class GhKoenigEditorLexical extends Component { uploadUrl = `${ghostPaths().apiRoot}/images/upload/`; editorAPI = null; - secondaryEditorAPI = null; skipFocusEditor = false; @tracked titleIsHovered = false; @@ -233,12 +232,6 @@ export default class GhKoenigEditorLexical extends Component { this.args.registerAPI(API); } - @action - registerSecondaryEditorAPI(API) { - this.secondaryEditorAPI = API; - this.args.registerSecondaryAPI(API); - } - // focus the editor when the editor canvas is clicked below the editor content, // otherwise the browser will defocus the editor and the cursor will disappear @action diff --git a/ghost/admin/app/components/gh-post-settings-menu.hbs b/ghost/admin/app/components/gh-post-settings-menu.hbs index ee91b2af80..1d01dcc5bc 100644 --- a/ghost/admin/app/components/gh-post-settings-menu.hbs +++ b/ghost/admin/app/components/gh-post-settings-menu.hbs @@ -853,7 +853,6 @@ post=this.post editorAPI=this.editorAPI toggleSettingsMenu=this.toggleSettingsMenu - secondaryEditorAPI=this.secondaryEditorAPI }} @close={{this.closePostHistory}} @modifier="total-overlay post-history" /> diff --git a/ghost/admin/app/components/koenig-lexical-editor.js b/ghost/admin/app/components/koenig-lexical-editor.js index f2a471a463..ad1c769da6 100644 --- a/ghost/admin/app/components/koenig-lexical-editor.js +++ b/ghost/admin/app/components/koenig-lexical-editor.js @@ -678,43 +678,34 @@ export default class KoenigLexicalEditor extends Component { const multiplayerDocId = cardConfig.post.id; const multiplayerUsername = this.session.user.name; - const KGEditorComponent = ({isInitInstance}) => { - return ( -
- - - {} : this.args.updateWordCount} /> - {} : this.args.updatePostTkCount} /> - -
- ); - }; - return (
Loading editor...

}> - - + + + + +
diff --git a/ghost/admin/app/components/modal-post-history.hbs b/ghost/admin/app/components/modal-post-history.hbs index f7f8ab1261..3989c29db5 100644 --- a/ghost/admin/app/components/modal-post-history.hbs +++ b/ghost/admin/app/components/modal-post-history.hbs @@ -33,7 +33,6 @@ @lexical={{this.selectedRevision.lexical}} @cardConfig={{this.cardConfig}} @registerAPI={{this.registerSelectedEditorApi}} - @registerSecondaryAPI={{this.registerSecondarySelectedEditorApi}} /> diff --git a/ghost/admin/app/components/modal-post-history.js b/ghost/admin/app/components/modal-post-history.js index 4dfa987e2e..05aba6646e 100644 --- a/ghost/admin/app/components/modal-post-history.js +++ b/ghost/admin/app/components/modal-post-history.js @@ -31,7 +31,6 @@ export default class ModalPostHistory extends Component { super(...arguments); this.post = this.args.model.post; this.editorAPI = this.args.model.editorAPI; - this.secondaryEditorAPI = this.args.model.secondaryEditorAPI; this.toggleSettingsMenu = this.args.model.toggleSettingsMenu; } @@ -102,11 +101,6 @@ export default class ModalPostHistory extends Component { this.selectedEditor = api; } - @action - registerSecondarySelectedEditorApi(api) { - this.secondarySelectedEditor = api; - } - @action registerComparisonEditorApi(api) { this.comparisonEditor = api; @@ -136,7 +130,6 @@ export default class ModalPostHistory extends Component { updateEditor: () => { const state = this.editorAPI.editorInstance.parseEditorState(revision.lexical); this.editorAPI.editorInstance.setEditorState(state); - this.secondaryEditorAPI.editorInstance.setEditorState(state); }, closePostHistoryModal: () => { this.closeModal(); diff --git a/ghost/admin/app/controllers/lexical-editor.js b/ghost/admin/app/controllers/lexical-editor.js index 2b8342afd9..71c6331437 100644 --- a/ghost/admin/app/controllers/lexical-editor.js +++ b/ghost/admin/app/controllers/lexical-editor.js @@ -297,11 +297,6 @@ export default class LexicalEditorController extends Controller { this._timedSaveTask.perform(); } - @action - updateSecondaryInstanceModel(lexical) { - this.set('post.secondaryLexicalState', JSON.stringify(lexical)); - } - @action updateTitleScratch(title) { this.set('post.titleScratch', title); @@ -428,11 +423,6 @@ export default class LexicalEditorController extends Controller { this.editorAPI = API; } - @action - registerSecondaryEditorAPI(API) { - this.secondaryEditorAPI = API; - } - @action clearFeatureImage() { this.post.set('featureImage', null); @@ -1231,6 +1221,7 @@ export default class LexicalEditorController extends Controller { _timedSaveTask; /* Private methods -------------------------------------------------------*/ + _hasDirtyAttributes() { let post = this.post; @@ -1238,7 +1229,8 @@ export default class LexicalEditorController extends Controller { return false; } - // If the Adapter failed to save the post, isError will be true, and we should consider the post still dirty. + // if the Adapter failed to save the post isError will be true + // and we should consider the post still dirty. if (post.get('isError')) { this._leaveModalReason = {reason: 'isError', context: post.errors.messages}; return true; @@ -1253,32 +1245,37 @@ export default class LexicalEditorController extends Controller { return true; } - // Title scratch comparison + // titleScratch isn't an attr so needs a manual dirty check if (post.titleScratch !== post.title) { this._leaveModalReason = {reason: 'title is different', context: {current: post.title, scratch: post.titleScratch}}; return true; } - // Lexical and scratch comparison + // scratch isn't an attr so needs a manual dirty check let lexical = post.get('lexical'); let scratch = post.get('lexicalScratch'); - let secondaryLexical = post.get('secondaryLexicalState'); + // additional guard in case we are trying to compare null with undefined + if (scratch || lexical) { + if (scratch !== lexical) { + // lexical can dynamically set direction on loading editor state (e.g. "rtl"/"ltr") per the DOM context + // and we need to ignore this as a change from the user; see https://github.com/facebook/lexical/issues/4998 + const scratchChildNodes = scratch ? JSON.parse(scratch).root?.children : []; + const lexicalChildNodes = lexical ? JSON.parse(lexical).root?.children : []; - let lexicalChildNodes = lexical ? JSON.parse(lexical).root?.children : []; - let scratchChildNodes = scratch ? JSON.parse(scratch).root?.children : []; - let secondaryLexicalChildNodes = secondaryLexical ? JSON.parse(secondaryLexical).root?.children : []; + // // nullling is typically faster than delete + scratchChildNodes.forEach(child => child.direction = null); + lexicalChildNodes.forEach(child => child.direction = null); - lexicalChildNodes.forEach(child => child.direction = null); - scratchChildNodes.forEach(child => child.direction = null); - secondaryLexicalChildNodes.forEach(child => child.direction = null); + if (JSON.stringify(scratchChildNodes) === JSON.stringify(lexicalChildNodes)) { + return false; + } - // Compare initLexical with scratch - let isSecondaryDirty = secondaryLexical && scratch && JSON.stringify(secondaryLexicalChildNodes) !== JSON.stringify(scratchChildNodes); + this._leaveModalReason = {reason: 'lexical is different', context: {current: lexical, scratch}}; + return true; + } + } - // Compare lexical with scratch - let isLexicalDirty = lexical && scratch && JSON.stringify(lexicalChildNodes) !== JSON.stringify(scratchChildNodes); - - // New+unsaved posts always return `hasDirtyAttributes: true` + // new+unsaved posts always return `hasDirtyAttributes: true` // so we need a manual check to see if any if (post.get('isNew')) { let changedAttributes = Object.keys(post.changedAttributes()); @@ -1289,26 +1286,15 @@ export default class LexicalEditorController extends Controller { return changedAttributes.length ? true : false; } - // We've covered all the non-tracked cases we care about so fall + // we've covered all the non-tracked cases we care about so fall // back on Ember Data's default dirty attribute checks let {hasDirtyAttributes} = post; + if (hasDirtyAttributes) { this._leaveModalReason = {reason: 'post.hasDirtyAttributes === true', context: post.changedAttributes()}; - return true; } - // If either comparison is not dirty, return false, because scratch is always up to date. - if (!isSecondaryDirty || !isLexicalDirty) { - return false; - } - - // If both comparisons are dirty, consider the post dirty - if (isSecondaryDirty && isLexicalDirty) { - this._leaveModalReason = {reason: 'initLexical and lexical are different from scratch', context: {secondaryLexical, lexical, scratch}}; - return true; - } - - return false; + return hasDirtyAttributes; } _showSaveNotification(prevStatus, status, delayed) { diff --git a/ghost/admin/app/models/post.js b/ghost/admin/app/models/post.js index 835d24d0a2..1ffb06d8d0 100644 --- a/ghost/admin/app/models/post.js +++ b/ghost/admin/app/models/post.js @@ -136,9 +136,6 @@ export default Model.extend(Comparable, ValidationEngine, { scratch: null, lexicalScratch: null, titleScratch: null, - //This is used to store the initial lexical state from the - // secondary editor to get the schema up to date in case its outdated - secondaryLexicalState: null, // For use by date/time pickers - will be validated then converted to UTC // on save. Updated by an observer whenever publishedAtUTC changes. diff --git a/ghost/admin/app/templates/lexical-editor.hbs b/ghost/admin/app/templates/lexical-editor.hbs index b7ee9a4a74..bd9d5d51e7 100644 --- a/ghost/admin/app/templates/lexical-editor.hbs +++ b/ghost/admin/app/templates/lexical-editor.hbs @@ -73,7 +73,6 @@ @body={{readonly this.post.lexicalScratch}} @bodyPlaceholder={{concat "Begin writing your " this.post.displayName "..."}} @onBodyChange={{this.updateScratch}} - @updateSecondaryInstanceModel={{this.updateSecondaryInstanceModel}} @headerOffset={{editor.headerHeight}} @scrollContainerSelector=".gh-koenig-editor" @scrollOffsetBottomSelector=".gh-mobile-nav-bar" @@ -98,7 +97,6 @@ }} @postType={{this.post.displayName}} @registerAPI={{this.registerEditorAPI}} - @registerSecondaryAPI={{this.registerSecondaryEditorAPI}} @savePostTask={{this.savePostTask}} /> @@ -138,7 +136,6 @@ @updateSlugTask={{this.updateSlugTask}} @savePostTask={{this.savePostTask}} @editorAPI={{this.editorAPI}} - @secondaryEditorAPI={{this.secondaryEditorAPI}} @toggleSettingsMenu={{this.toggleSettingsMenu}} /> {{/if}} diff --git a/ghost/admin/tests/unit/controllers/editor-test.js b/ghost/admin/tests/unit/controllers/editor-test.js index acf7a5e95b..088f22391d 100644 --- a/ghost/admin/tests/unit/controllers/editor-test.js +++ b/ghost/admin/tests/unit/controllers/editor-test.js @@ -208,8 +208,7 @@ describe('Unit: Controller: lexical-editor', function () { titleScratch: 'this is a title', status: 'published', lexical: initialLexicalString, - lexicalScratch: initialLexicalString, - secondaryLexicalState: initialLexicalString + lexicalScratch: initialLexicalString })); // synthetically update the lexicalScratch as if the editor itself made the modifications on loading the initial editorState @@ -226,47 +225,5 @@ describe('Unit: Controller: lexical-editor', function () { isDirty = controller.get('hasDirtyAttributes'); expect(isDirty).to.be.true; }); - - it('dirty is false if secondaryLexical and scratch matches, but lexical is outdated', async function () { - const initialLexicalString = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Sample content","type": "extended-text","version": 1}],"direction": null,"format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; - const lexicalScratch = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Sample content","type": "extended-text","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; - const secondLexicalInstance = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Here's some new text","type": "extended-text","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; - - let controller = this.owner.lookup('controller:lexical-editor'); - controller.set('post', EmberObject.create({ - title: 'this is a title', - titleScratch: 'this is a title', - status: 'published', - lexical: initialLexicalString, - lexicalScratch: lexicalScratch, - secondaryLexicalState: secondLexicalInstance - })); - - let isDirty = controller.get('hasDirtyAttributes'); - - expect(isDirty).to.be.false; - }); - - it('dirty is true if secondaryLexical and lexical does not match scratch', async function () { - const initialLexicalString = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Sample content","type": "extended-text","version": 1}],"direction": null,"format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; - const lexicalScratch = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Sample content1234","type": "extended-text","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; - const secondLexicalInstance = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Here's some new text","type": "extended-text","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; - - let controller = this.owner.lookup('controller:lexical-editor'); - controller.set('post', EmberObject.create({ - title: 'this is a title', - titleScratch: 'this is a title', - status: 'published', - lexical: initialLexicalString, - lexicalScratch: lexicalScratch, - secondaryLexicalState: secondLexicalInstance - })); - - controller.send('updateScratch',JSON.parse(lexicalScratch)); - - let isDirty = controller.get('hasDirtyAttributes'); - - expect(isDirty).to.be.true; - }); }); });