From 712ff442dfe7c61f538278294c1f1e2c2380c8f3 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 30 Jul 2026 15:47:26 +0200 Subject: [PATCH 1/5] Select created thread instead of resetting selection Submitting the new thread form left the editor with no selection at all, dropping the reviewer back to the outline. Wait for the thread to be created instead and select the comments of its subject, highlighting what was just written. The thread id is only known once the create request has come back, so ReviewSession emits a create:thread event the preview message controller reacts to. --- .../PreviewMessageController-spec.js | 86 +++++++++++++++++++ .../controllers/PreviewMessageController.js | 30 ++++++- .../package/src/editor/views/NewThreadView.js | 6 +- package/spec/review/ReviewSession-spec.js | 51 +++++++++++ package/src/review/ReviewSession.js | 1 + 5 files changed, 168 insertions(+), 6 deletions(-) diff --git a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js index 9977e77087..1e5300948f 100644 --- a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js +++ b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js @@ -364,6 +364,92 @@ describe('PreviewMessageController', () => { })).resolves.toMatchObject({contentElementId: 1, command: {some: 'COMMAND'}}); }); + it('selects and highlights a created content element thread in iframe', async () => { + const entry = factories.entry(ScrolledEntry, {}, { + entryTypeSeed: normalizeSeed({ + contentElements: [{id: 1, permaId: 10}] + }) + }); + entry.reviewSession = factories.reviewSession(); + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow}); + + await postReadyMessageAndWaitForAcknowledgement(iframeWindow); + + return expect(new Promise(resolve => { + iframeWindow.addEventListener('message', event => { + if (event.data.type === 'SELECT') { + resolve(event.data); + } + }); + entry.reviewSession.trigger('create:thread', { + id: 7, subjectType: 'ContentElement', subjectId: 10 + }); + })).resolves.toMatchObject({ + type: 'SELECT', + payload: {id: 1, type: 'contentElementComments', highlightedThreadId: 7} + }); + }); + + it('selects and highlights a created section thread in iframe', async () => { + const entry = factories.entry(ScrolledEntry, {}, { + entryTypeSeed: normalizeSeed({ + sections: [{id: 5, permaId: 50}] + }) + }); + entry.reviewSession = factories.reviewSession(); + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow}); + + await postReadyMessageAndWaitForAcknowledgement(iframeWindow); + + return expect(new Promise(resolve => { + iframeWindow.addEventListener('message', event => { + if (event.data.type === 'SELECT') { + resolve(event.data); + } + }); + entry.reviewSession.trigger('create:thread', { + id: 3, subjectType: 'Section', subjectId: 50 + }); + })).resolves.toMatchObject({ + type: 'SELECT', + payload: {id: 5, type: 'sectionComments', highlightedThreadId: 3} + }); + }); + + it('ignores created threads of deleted subjects', async () => { + const entry = factories.entry(ScrolledEntry, {}, { + entryTypeSeed: normalizeSeed({ + contentElements: [{id: 1, permaId: 10}] + }) + }); + entry.reviewSession = factories.reviewSession(); + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow}); + + await postReadyMessageAndWaitForAcknowledgement(iframeWindow); + + // Expecting the SELECT of the known subject to arrive first pins the + // absence of one for the deleted subject. + return expect(new Promise(resolve => { + iframeWindow.addEventListener('message', event => { + if (event.data.type === 'SELECT') { + resolve(event.data); + } + }); + entry.reviewSession.trigger('create:thread', { + id: 7, subjectType: 'ContentElement', subjectId: 999 + }); + entry.reviewSession.trigger('create:thread', { + id: 8, subjectType: 'ContentElement', subjectId: 10 + }); + })).resolves.toMatchObject({ + type: 'SELECT', + payload: {id: 1, type: 'contentElementComments', highlightedThreadId: 8} + }); + }); + it('sends SELECT message to iframe on resetSelection event on model', async () => { const entry = factories.entry(ScrolledEntry, {}, { entryTypeSeed: normalizeSeed({ diff --git a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js index 5355635100..85b1fb990e 100644 --- a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js +++ b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js @@ -128,6 +128,28 @@ export const PreviewMessageController = Object.extend({ }) }); + // Passing the thread id here rather than posting + // SELECT_COMMENT_THREAD avoids depending on the preview having + // processed the new thread already: that message is dropped for + // threads the iframe does not know yet. + if (this.entry.reviewSession) { + this.listenTo(this.entry.reviewSession, 'create:thread', thread => { + const model = modelForSubject(this.entry, thread); + + if (!model) return; + + postMessage({ + type: 'SELECT', + payload: { + id: model.id, + type: thread.subjectType === 'Section' ? 'sectionComments' : + 'contentElementComments', + highlightedThreadId: thread.id + } + }) + }); + } + this.listenTo(this.entry, 'selectWidget', widget => { postMessage({ type: 'SELECT', @@ -312,9 +334,13 @@ function selectedCommentsSubjectFor(entry, payload) { } if (type === 'newThread') { - const collection = subjectType === 'Section' ? entry.sections : entry.contentElements; - return {subjectType, id: collection.findWhere({permaId: subjectId})?.id}; + return {subjectType, id: modelForSubject(entry, {subjectType, subjectId})?.id}; } return undefined; } + +function modelForSubject(entry, {subjectType, subjectId}) { + const collection = subjectType === 'Section' ? entry.sections : entry.contentElements; + return collection.findWhere({permaId: subjectId}); +} diff --git a/entry_types/scrolled/package/src/editor/views/NewThreadView.js b/entry_types/scrolled/package/src/editor/views/NewThreadView.js index 2cc80348f9..da734ae235 100644 --- a/entry_types/scrolled/package/src/editor/views/NewThreadView.js +++ b/entry_types/scrolled/package/src/editor/views/NewThreadView.js @@ -49,14 +49,12 @@ const NewThreadFormView = ReviewView.extend({ className: styles.form, renderContent() { - const {entry, subjectType, subjectId, subjectRange} = this.options; - const leave = () => entry.trigger('resetSelection'); + const {subjectType, subjectId, subjectRange} = this.options; return ( + subjectRange={subjectRange} /> ); } }); diff --git a/package/spec/review/ReviewSession-spec.js b/package/spec/review/ReviewSession-spec.js index 4c0383bf32..b4cae30c75 100644 --- a/package/spec/review/ReviewSession-spec.js +++ b/package/spec/review/ReviewSession-spec.js @@ -112,6 +112,57 @@ describe('ReviewSession', () => { ); }); + it('emits create:thread after createThread', async () => { + const request = jest.fn() + .mockResolvedValueOnce({ + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [] + }) + .mockResolvedValueOnce({ + id: 1, + subjectType: 'ContentElement', + subjectId: 10, + comments: [{id: 100, body: 'Looks good!', creatorId: 42}] + }); + + const session = new ReviewSession({entryId: 5, request}); + await session.fetch(); + + const listener = jest.fn(); + session.on('create:thread', listener); + + await session.createThread({ + subjectType: 'ContentElement', + subjectId: 10, + body: 'Looks good!' + }); + + expect(listener).toHaveBeenCalledWith( + expect.objectContaining({id: 1, subjectType: 'ContentElement'}) + ); + }); + + it('does not emit create:thread when updating a thread', async () => { + const request = jest.fn() + .mockResolvedValueOnce({ + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [{id: 1, subjectType: 'ContentElement', subjectId: 10, comments: []}] + }) + .mockResolvedValueOnce({ + id: 1, subjectType: 'ContentElement', subjectId: 10, resolved: true, comments: [] + }); + + const session = new ReviewSession({entryId: 5, request}); + await session.fetch(); + + const listener = jest.fn(); + session.on('create:thread', listener); + + await session.updateThread({threadId: 1, resolved: true}); + + expect(listener).not.toHaveBeenCalled(); + }); + it('includes subject_range in createThread request', async () => { const subjectRange = { anchor: {path: [0, 0], offset: 5}, diff --git a/package/src/review/ReviewSession.js b/package/src/review/ReviewSession.js index 79dd05e11e..00b61e4566 100644 --- a/package/src/review/ReviewSession.js +++ b/package/src/review/ReviewSession.js @@ -30,6 +30,7 @@ export class ReviewSession { this._upsertThread(thread); this.trigger('change:thread', thread); + this.trigger('create:thread', thread); } async updateThread({threadId, resolved}) { From 0447c7bd3319987af6580cb38602edc0ecbf2df0 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 30 Jul 2026 16:23:37 +0200 Subject: [PATCH 2/5] Keep unsent comment texts as drafts per subject Dismissing a popover, changing the selection in the editor or leaving the new thread view discarded whatever had been typed. Store the text as a draft on the review session instead, keyed by subject alone: a draft written about one phrase of a text block is offered again when commenting on another phrase of the same block. The session also represents the create request in flight as a pending draft, which is what disables the form and shows a spinner. Submitting therefore no longer closes the thread list form right away: it stays open until the thread has been created, or becomes editable again with the text still in it when the request failed. Drafts reach the editor sidebar and the frontend popover through the same message relay as the rest of the review state, except that the sidebar writes them directly: its message handler is disposed together with the view, which would drop the draft stored on the way out. --- .../spec/editor/views/NewThreadView-spec.js | 37 ++++ .../package/spec/review/NewThreadForm-spec.js | 188 ++++++++++++++++++ .../spec/review/ReviewMessageHandler-spec.js | 44 +++- .../spec/review/ReviewStateProvider-spec.js | 176 +++++++++++++++- .../package/spec/review/ThreadList-spec.js | 127 +++++++++++- .../spec/support/renderWithReviewState.js | 14 +- .../package/src/editor/views/ReviewView.js | 20 +- .../package/src/review/NewThreadForm.js | 66 ++++-- .../src/review/NewThreadForm.module.css | 24 +++ .../src/review/ReviewMessageHandler.js | 12 +- .../package/src/review/ReviewStateProvider.js | 101 +++++++++- .../scrolled/package/src/review/ThreadList.js | 11 +- .../package/src/review/postMessage.js | 14 ++ package/spec/review/ReviewSession-spec.js | 115 +++++++++++ package/src/review/ReviewSession.js | 54 +++++ 15 files changed, 953 insertions(+), 50 deletions(-) create mode 100644 entry_types/scrolled/package/spec/review/NewThreadForm-spec.js diff --git a/entry_types/scrolled/package/spec/editor/views/NewThreadView-spec.js b/entry_types/scrolled/package/spec/editor/views/NewThreadView-spec.js index 20242acd48..bd7e00cd21 100644 --- a/entry_types/scrolled/package/spec/editor/views/NewThreadView-spec.js +++ b/entry_types/scrolled/package/spec/editor/views/NewThreadView-spec.js @@ -8,6 +8,7 @@ import {NewThreadView} from 'editor/views/NewThreadView'; import {factories, useFakeTranslations, renderBackboneView} from 'pageflow/testHelpers'; import {useEditorGlobals} from 'support'; import {fireEvent} from '@testing-library/dom'; +import userEvent from '@testing-library/user-event'; describe('NewThreadView', () => { const {createEntry} = useEditorGlobals(); @@ -59,6 +60,42 @@ describe('NewThreadView', () => { expect(getByText('Comments')).toBeInTheDocument(); }); + // Submitting cannot reach the session here: jsdom leaves MessageEvent + // source unset for same-window posts, so the handler drops the create + // message. NewThreadForm-spec covers the form disabling itself. + it('stores the entered text as a draft when the view is closed', async () => { + const user = userEvent.setup(); + const entry = createEntry({}); + entry.reviewSession = factories.reviewSession(); + const view = buildView(entry); + + const {getByPlaceholderText} = renderBackboneView(view); + + await user.type(getByPlaceholderText('Add a comment...'), 'Half a thought'); + view.close(); + + expect(entry.reviewSession.drafts).toEqual({ + 'ContentElement:10': { + subjectType: 'ContentElement', + subjectId: 10, + body: 'Half a thought', + pending: false + } + }); + }); + + it('restores the draft stored for the subject', () => { + const entry = createEntry({}); + entry.reviewSession = factories.reviewSession(); + entry.reviewSession.setDraft({ + subjectType: 'ContentElement', subjectId: 10, body: 'Half a thought' + }); + + const {getByPlaceholderText} = renderBackboneView(buildView(entry)); + + expect(getByPlaceholderText('Add a comment...')).toHaveValue('Half a thought'); + }); + it('navigates to the comments selection tab when the back link is clicked', () => { const entry = createEntry({}); entry.reviewSession = factories.reviewSession(); diff --git a/entry_types/scrolled/package/spec/review/NewThreadForm-spec.js b/entry_types/scrolled/package/spec/review/NewThreadForm-spec.js new file mode 100644 index 0000000000..7466a86042 --- /dev/null +++ b/entry_types/scrolled/package/spec/review/NewThreadForm-spec.js @@ -0,0 +1,188 @@ +import React from 'react'; +import '@testing-library/jest-dom/extend-expect'; +import userEvent from '@testing-library/user-event'; +import {act, waitFor} from '@testing-library/react'; +import {useFakeTranslations} from 'pageflow/testHelpers'; + +import {NewThreadForm} from 'review/NewThreadForm'; +import {postReviewStateDraftsChangeMessage} from 'review/postMessage'; +import {renderWithReviewState} from 'support/renderWithReviewState'; + +const seed = { + sections: [{id: 1, permaId: 1}], + contentElements: [{id: 1, permaId: 10, sectionId: 1, typeName: 'textBlock'}] +}; + +function draft({body, pending = false}) { + return { + 'ContentElement:10': { + subjectType: 'ContentElement', subjectId: 10, body, pending + } + }; +} + +function renderForm(options = {}) { + return renderWithReviewState( + , + {seed, ...options} + ); +} + +function postDraftsChange(drafts) { + act(() => postReviewStateDraftsChangeMessage(window, drafts)); +} + +describe('NewThreadForm', () => { + // A spy left behind by a failing spec would swallow the simulated + // review state messages of the ones after it. + afterEach(() => jest.restoreAllMocks()); + + useFakeTranslations({ + 'pageflow_scrolled.review.add_comment_placeholder': 'Add a comment...', + 'pageflow_scrolled.review.send': 'Send', + 'pageflow_scrolled.review.enter_for_new_line': 'Enter for new line' + }); + + describe('draft restoring', () => { + it('starts out with the stored draft of the subject', () => { + const {getByPlaceholderText} = renderForm({ + drafts: draft({body: 'Half a thought'}) + }); + + expect(getByPlaceholderText('Add a comment...')).toHaveValue('Half a thought'); + }); + + it('starts out empty without a draft', () => { + const {getByPlaceholderText} = renderForm(); + + expect(getByPlaceholderText('Add a comment...')).toHaveValue(''); + }); + }); + + describe('draft storing', () => { + it('stores the text when the form goes away', async () => { + const user = userEvent.setup(); + const setDraft = jest.fn(); + + const {getByPlaceholderText, unmount} = renderForm({setDraft}); + + await user.type(getByPlaceholderText('Add a comment...'), 'Half a thought'); + unmount(); + + expect(setDraft).toHaveBeenCalledWith({ + subjectType: 'ContentElement', + subjectId: 10, + body: 'Half a thought' + }); + }); + + // Blank bodies make the session drop the draft. + it('stores a blank text once the input has been cleared', async () => { + const user = userEvent.setup(); + const setDraft = jest.fn(); + + const {getByPlaceholderText, unmount} = renderForm({ + drafts: draft({body: 'Never mind'}), setDraft + }); + + await user.clear(getByPlaceholderText('Add a comment...')); + unmount(); + + expect(setDraft).toHaveBeenLastCalledWith( + expect.objectContaining({body: ''}) + ); + }); + + // The form does not render again between the draft being dropped and + // the list taking it off screen, so the pending draft is the last one + // it saw. + it('does not store the text while the session is creating the thread', () => { + const setDraft = jest.fn(); + + const {unmount} = renderForm({ + drafts: draft({body: 'Looks good', pending: true}), + setDraft + }); + + unmount(); + + expect(setDraft).not.toHaveBeenCalled(); + }); + + it('does not store the text after submitting', async () => { + const user = userEvent.setup(); + const setDraft = jest.fn(); + + const {getByPlaceholderText, getByRole, unmount} = renderForm({setDraft}); + + await user.type(getByPlaceholderText('Add a comment...'), 'Looks good'); + await user.click(getByRole('button', {name: 'Send'})); + + unmount(); + + expect(setDraft).not.toHaveBeenCalled(); + }); + + it('stores the text again when creating the thread failed', async () => { + const user = userEvent.setup(); + const setDraft = jest.fn(); + + const {getByPlaceholderText, getByRole, unmount} = renderForm({setDraft}); + + await user.type(getByPlaceholderText('Add a comment...'), 'Looks good'); + await user.click(getByRole('button', {name: 'Send'})); + + postDraftsChange(draft({body: 'Looks good', pending: false})); + await waitFor(() => + expect(getByPlaceholderText('Add a comment...')).not.toBeDisabled() + ); + + await user.type(getByPlaceholderText('Add a comment...'), ' after all'); + unmount(); + + expect(setDraft).toHaveBeenCalledWith( + expect.objectContaining({body: 'Looks good after all'}) + ); + }); + }); + + describe('while the thread is being created', () => { + it('disables the input and shows a spinner', () => { + const {getByPlaceholderText, getByRole, container} = renderForm({ + drafts: draft({body: 'Looks good', pending: true}) + }); + + expect(getByPlaceholderText('Add a comment...')).toBeDisabled(); + expect(getByRole('button', {name: 'Send'})).toBeDisabled(); + expect(container.querySelector('[data-file-name="SvgSpinner"]')).not.toBeNull(); + }); + + it('ignores further submits', async () => { + const user = userEvent.setup(); + const postMessage = jest.spyOn(window.top, 'postMessage').mockImplementation(() => {}); + + const {getByRole} = renderForm({ + drafts: draft({body: 'Looks good', pending: true}) + }); + + await user.click(getByRole('button', {name: 'Send'})); + + expect(postMessage).not.toHaveBeenCalledWith( + expect.objectContaining({type: 'CREATE_COMMENT_THREAD'}), + expect.anything() + ); + + postMessage.mockRestore(); + }); + + it('is editable again once the draft is no longer pending', () => { + const {getByPlaceholderText, getByRole} = renderForm({ + drafts: draft({body: 'Looks good', pending: false}) + }); + + expect(getByPlaceholderText('Add a comment...')).not.toBeDisabled(); + expect(getByRole('button', {name: 'Send'})).toBeEnabled(); + expect(getByPlaceholderText('Add a comment...')).toHaveValue('Looks good'); + }); + }); +}); diff --git a/entry_types/scrolled/package/spec/review/ReviewMessageHandler-spec.js b/entry_types/scrolled/package/spec/review/ReviewMessageHandler-spec.js index f9419719d1..4134c66b49 100644 --- a/entry_types/scrolled/package/spec/review/ReviewMessageHandler-spec.js +++ b/entry_types/scrolled/package/spec/review/ReviewMessageHandler-spec.js @@ -6,7 +6,8 @@ function fakeReviewSession() { const session = { createThread: jest.fn().mockResolvedValue(), createComment: jest.fn().mockResolvedValue(), - updateThread: jest.fn().mockResolvedValue() + updateThread: jest.fn().mockResolvedValue(), + setDraft: jest.fn() }; Object.assign(session, BackboneEvents); @@ -135,6 +136,47 @@ describe('ReviewMessageHandler', () => { }); }); + it('calls session.setDraft on SET_COMMENT_DRAFT message from targetWindow', async () => { + const session = fakeReviewSession(); + + ReviewMessageHandler.create({session, targetWindow: window}); + + window.dispatchEvent(new MessageEvent('message', { + data: { + type: 'SET_COMMENT_DRAFT', + payload: {subjectType: 'CE', subjectId: 10, body: 'Half a thought'} + }, + origin: window.location.origin, + source: window + })); + + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(session.setDraft).toHaveBeenCalledWith({ + subjectType: 'CE', subjectId: 10, body: 'Half a thought' + }); + }); + + it('posts REVIEW_STATE_DRAFTS_CHANGE on change:drafts event', () => { + const session = fakeReviewSession(); + const postMessage = jest.fn(); + jest.spyOn(window, 'postMessage').mockImplementation(postMessage); + + ReviewMessageHandler.create({session, targetWindow: window}); + + const drafts = { + 'CE:10': {subjectType: 'CE', subjectId: 10, body: 'Half a thought', pending: false} + }; + session.trigger('change:drafts', drafts); + + expect(postMessage).toHaveBeenCalledWith( + {type: 'REVIEW_STATE_DRAFTS_CHANGE', payload: drafts}, + window.location.origin + ); + + window.postMessage.mockRestore(); + }); + it('ignores messages not from targetWindow', async () => { const session = fakeReviewSession(); const iframeWindow = {}; diff --git a/entry_types/scrolled/package/spec/review/ReviewStateProvider-spec.js b/entry_types/scrolled/package/spec/review/ReviewStateProvider-spec.js index b0ac76a525..cfb6d2833b 100644 --- a/entry_types/scrolled/package/spec/review/ReviewStateProvider-spec.js +++ b/entry_types/scrolled/package/spec/review/ReviewStateProvider-spec.js @@ -5,13 +5,17 @@ import {renderHook} from '@testing-library/react-hooks'; import { ReviewStateProvider, + useCommentDraft, useCommentThread, - useCommentThreads + useCommentThreads, + useCreateCommentThread } from 'review/ReviewStateProvider'; import { postReviewStateResetMessage, - postReviewStateThreadChangeMessage + postReviewStateThreadChangeMessage, + postReviewStateDraftsChangeMessage } from 'review/postMessage'; +import {renderHookWithReviewState} from 'support/renderWithReviewState'; function wrapper({children}) { return {children}; @@ -239,4 +243,172 @@ describe('ReviewStateProvider', () => { expect(result.current).toBeUndefined(); }); }); + + describe('useCommentDraft', () => { + const draft = { + subjectType: 'CE', subjectId: 10, body: 'Half a thought', pending: false + }; + + function renderDraftHook({initialDrafts, setDraft} = {}) { + return renderHook( + () => useCommentDraft({subjectType: 'CE', subjectId: 10}), + { + wrapper: ({children}) => ( + + {children} + + ) + } + ); + } + + it('returns undefined without a draft for the subject', () => { + const {result} = renderDraftHook(); + + expect(result.current[0]).toBeUndefined(); + }); + + it('returns drafts passed as initial drafts', () => { + const {result} = renderDraftHook({initialDrafts: {'CE:10': draft}}); + + expect(result.current[0]).toEqual(draft); + }); + + it('updates on drafts change message', async () => { + const {result, waitForNextUpdate} = renderDraftHook(); + + act(() => { + postReviewStateDraftsChangeMessage(window, {'CE:10': draft}); + }); + await waitForNextUpdate(); + + expect(result.current[0]).toEqual(draft); + }); + + it('stores a draft for the subject by posting a message', () => { + const postMessage = jest.spyOn(window.top, 'postMessage').mockImplementation(() => {}); + + const {result} = renderDraftHook(); + + result.current[1]('Half a thought'); + + expect(postMessage).toHaveBeenCalledWith( + { + type: 'SET_COMMENT_DRAFT', + payload: {subjectType: 'CE', subjectId: 10, body: 'Half a thought'} + }, + window.location.origin + ); + + postMessage.mockRestore(); + }); + + it('stores a draft through the function passed to the provider instead', () => { + const setDraft = jest.fn(); + + const {result} = renderDraftHook({setDraft}); + + result.current[1]('Half a thought'); + + expect(setDraft).toHaveBeenCalledWith({ + subjectType: 'CE', subjectId: 10, body: 'Half a thought' + }); + }); + + it('keeps drafts on reset message', async () => { + const {result, waitForNextUpdate} = renderHook( + () => ({ + draft: useCommentDraft({subjectType: 'CE', subjectId: 10})[0], + threads: useCommentThreads() + }), + { + wrapper: ({children}) => ( + + {children} + + ) + } + ); + + postReset({ + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [{id: 1, subjectType: 'CE', subjectId: 10, comments: []}] + }); + await waitForNextUpdate(); + + expect(result.current.threads).toHaveLength(1); + expect(result.current.draft).toEqual(draft); + }); + }); + + // Assembling the payload needs the entry structure the subject lives in. + describe('useCreateCommentThread', () => { + const seed = { + sections: [{id: 1, permaId: 7}], + contentElements: [{id: 1, permaId: 10, sectionId: 1, typeName: 'textBlock'}] + }; + + function renderCreateHook({drafts} = {}) { + return renderHookWithReviewState( + () => ({ + createThread: useCreateCommentThread({ + subjectType: 'ContentElement', subjectId: 10 + }), + draft: useCommentDraft({subjectType: 'ContentElement', subjectId: 10})[0], + other: useCommentDraft({subjectType: 'ContentElement', subjectId: 20})[0] + }), + {seed, drafts} + ); + } + + it('marks the draft of the subject pending right away', () => { + const {result} = renderCreateHook(); + + act(() => result.current.createThread('Looks good')); + + expect(result.current.draft).toEqual({ + subjectType: 'ContentElement', + subjectId: 10, + body: 'Looks good', + pending: true + }); + }); + + it('posts a create thread message with the section of the subject', () => { + const postMessage = jest.spyOn(window.top, 'postMessage').mockImplementation(() => {}); + + const {result} = renderCreateHook(); + + act(() => result.current.createThread('Looks good')); + + expect(postMessage).toHaveBeenCalledWith( + { + type: 'CREATE_COMMENT_THREAD', + payload: expect.objectContaining({ + subjectType: 'ContentElement', + subjectId: 10, + sectionPermaId: 7, + body: 'Looks good' + }) + }, + window.location.origin + ); + + postMessage.mockRestore(); + }); + + it('keeps drafts of other subjects', () => { + const {result} = renderCreateHook({ + drafts: { + 'ContentElement:20': { + subjectType: 'ContentElement', subjectId: 20, body: 'Elsewhere', pending: false + } + } + }); + + act(() => result.current.createThread('Looks good')); + + expect(result.current.other).toMatchObject({body: 'Elsewhere'}); + }); + }); }); diff --git a/entry_types/scrolled/package/spec/review/ThreadList-spec.js b/entry_types/scrolled/package/spec/review/ThreadList-spec.js index e5cf50009f..af6109bd50 100644 --- a/entry_types/scrolled/package/spec/review/ThreadList-spec.js +++ b/entry_types/scrolled/package/spec/review/ThreadList-spec.js @@ -1,9 +1,14 @@ import React from 'react'; import '@testing-library/jest-dom/extend-expect'; import userEvent from '@testing-library/user-event'; +import {act, waitFor} from '@testing-library/react'; import {useFakeTranslations} from 'pageflow/testHelpers'; import {ThreadList} from 'review/ThreadList'; +import { + postReviewStateDraftsChangeMessage, + postReviewStateThreadChangeMessage +} from 'review/postMessage'; import {review} from 'review/api'; import {ScrollHighlightedThreadIntoViewProvider} from 'review/scrollHighlightedThreadIntoView'; import {renderWithReviewState} from 'support/renderWithReviewState'; @@ -15,8 +20,16 @@ const seed = { contentElements: [{id: 1, permaId: 10, sectionId: 1, typeName: 'textBlock'}] }; -function renderThreadList(ui, {commentThreads = []} = {}) { - return renderWithReviewState(ui, {seed, commentThreads}); +function renderThreadList(ui, options = {}) { + return renderWithReviewState(ui, {seed, ...options}); +} + +function postDraftsChange(drafts) { + act(() => postReviewStateDraftsChangeMessage(window, drafts)); +} + +function postThreadChange(thread) { + act(() => postReviewStateThreadChangeMessage(window, thread)); } describe('ThreadList', () => { @@ -587,6 +600,116 @@ describe('ThreadList', () => { expect(getByPlaceholderText('Add a comment...')).toBeInTheDocument(); }); + + it('shows the form again when a draft exists for the subject', () => { + const {getByPlaceholderText} = renderThreadList( + , + { + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, comments: [ + {id: 10, body: 'Existing', creatorName: 'Bob', creatorId: 2} + ]} + ], + drafts: { + 'ContentElement:10': { + subjectType: 'ContentElement', subjectId: 10, body: 'Half a thought' + } + } + } + ); + + expect(getByPlaceholderText('Add a comment...')).toHaveValue('Half a thought'); + }); + + it('keeps the form open while the thread is being created', () => { + const {getByPlaceholderText} = renderThreadList( + , + { + drafts: { + 'ContentElement:10': { + subjectType: 'ContentElement', + subjectId: 10, + body: 'Looks good', + pending: true + } + } + } + ); + + expect(getByPlaceholderText('Add a comment...')).toBeDisabled(); + }); + + // The editor sidebar lists opt out of the form entirely, since new + // threads are composed in a view of their own there. + it('does not show the form for a draft when showNewForm is false', () => { + const {queryByPlaceholderText} = renderThreadList( + , + { + drafts: { + 'ContentElement:10': { + subjectType: 'ContentElement', subjectId: 10, body: 'Half a thought' + } + } + } + ); + + expect(queryByPlaceholderText('Add a comment...')).toBeNull(); + }); + + it('keeps the form open with the text on submit', async () => { + const user = userEvent.setup(); + + const {getByPlaceholderText, getByRole} = renderThreadList( + + ); + + await user.type(getByPlaceholderText('Add a comment...'), 'New thread'); + await user.click(getByRole('button', {name: 'Send'})); + + expect(getByPlaceholderText('Add a comment...')).toBeDisabled(); + expect(getByPlaceholderText('Add a comment...')).toHaveValue('New thread'); + }); + + // Storing the text on the way out would bring back the draft the + // session has just dropped and reopen the form. + it('closes the form for good after the thread has been created', async () => { + const user = userEvent.setup(); + const setDraft = jest.fn(); + + const {getByPlaceholderText, queryByPlaceholderText, getByRole} = renderThreadList( + , + {setDraft} + ); + + await user.type(getByPlaceholderText('Add a comment...'), 'New thread'); + await user.click(getByRole('button', {name: 'Send'})); + + postDraftsChange({ + 'ContentElement:10': { + subjectType: 'ContentElement', + subjectId: 10, + body: 'New thread', + pending: true + } + }); + await waitFor(() => + expect(getByPlaceholderText('Add a comment...')).toBeDisabled() + ); + setDraft.mockClear(); + + postThreadChange({ + id: 1, + subjectType: 'ContentElement', + subjectId: 10, + comments: [{id: 10, body: 'New thread', creatorName: 'Alice', creatorId: 1}] + }); + postDraftsChange({}); + + await waitFor(() => + expect(queryByPlaceholderText('Add a comment...')).toBeNull() + ); + expect(setDraft).not.toHaveBeenCalled(); + }); }); describe('replies', () => { diff --git a/entry_types/scrolled/package/spec/support/renderWithReviewState.js b/entry_types/scrolled/package/spec/support/renderWithReviewState.js index 34ca29dfcb..ad31f6435c 100644 --- a/entry_types/scrolled/package/spec/support/renderWithReviewState.js +++ b/entry_types/scrolled/package/spec/support/renderWithReviewState.js @@ -9,25 +9,27 @@ import {LocatedCommentThreadsProvider} from 'review/useLocatedCommentThreads'; // lives in), so it needs the entry scope just like any other frontend // component — review state is layered on top via the entry helper's // `wrapper` option. -export function renderWithReviewState(ui, {commentThreads = [], currentUser = null, seed = {}} = {}) { +export function renderWithReviewState(ui, {commentThreads = [], currentUser = null, drafts, setDraft, seed = {}} = {}) { return renderInEntry(ui, { seed, - wrapper: reviewStateWrapper({commentThreads, currentUser}) + wrapper: reviewStateWrapper({commentThreads, currentUser, drafts, setDraft}) }); } // Counterpart for selector hooks that read entry state and review state. -export function renderHookWithReviewState(hook, {commentThreads = [], currentUser = null, seed = {}} = {}) { +export function renderHookWithReviewState(hook, {commentThreads = [], currentUser = null, drafts, setDraft, seed = {}} = {}) { return renderHookInEntry(hook, { seed, - wrapper: reviewStateWrapper({commentThreads, currentUser}) + wrapper: reviewStateWrapper({commentThreads, currentUser, drafts, setDraft}) }); } -function reviewStateWrapper({commentThreads = [], currentUser = null} = {}) { +function reviewStateWrapper({commentThreads = [], currentUser = null, drafts, setDraft} = {}) { return function ReviewStateWrapper({children}) { return ( - + {children} diff --git a/entry_types/scrolled/package/src/editor/views/ReviewView.js b/entry_types/scrolled/package/src/editor/views/ReviewView.js index b16145381c..2aa12191be 100644 --- a/entry_types/scrolled/package/src/editor/views/ReviewView.js +++ b/entry_types/scrolled/package/src/editor/views/ReviewView.js @@ -19,13 +19,13 @@ import styles from './ReviewView.module.css'; // Base Marionette view for comment-related sidebar panels. Provides the // shared wiring: a container div, a ReviewMessageHandler bridging the // session to the preview iframe, a ReviewStateProvider seeded from the -// current session state, and an EntryStateProvider so the rendered tree -// can resolve entry structure (e.g. the section a comment subject lives -// in). Subclasses implement `renderContent(props)` to return the React -// subtree. Props are produced by `props()` (default: empty) and -// re-evaluated whenever the subclass calls `rerender()` — useful for -// re-rendering on backbone change events without requiring React -// subscription hooks inside the rendered tree. +// current session state and its drafts, and an EntryStateProvider so the +// rendered tree can resolve entry structure (e.g. the section a comment +// subject lives in). Subclasses implement `renderContent(props)` to +// return the React subtree. Props are produced by `props()` (default: +// empty) and re-evaluated whenever the subclass calls `rerender()` — +// useful for re-rendering on backbone change events without requiring +// React subscription hooks inside the rendered tree. export const ReviewView = Marionette.ItemView.extend({ template: () => `
`, @@ -41,6 +41,8 @@ export const ReviewView = Marionette.ItemView.extend({ targetWindow: window }); + this.setDraft = draft => session.setDraft(draft); + this.rerender(); }, @@ -52,7 +54,9 @@ export const ReviewView = Marionette.ItemView.extend({ rerender() { const {entry} = this.options; ReactDOM.render( - + diff --git a/entry_types/scrolled/package/src/review/NewThreadForm.js b/entry_types/scrolled/package/src/review/NewThreadForm.js index bec4dba95e..4dfe1b59ec 100644 --- a/entry_types/scrolled/package/src/review/NewThreadForm.js +++ b/entry_types/scrolled/package/src/review/NewThreadForm.js @@ -1,26 +1,23 @@ -import React, {useCallback, useState} from 'react'; - -import {useSectionPermaIdOfSubject} from 'pageflow-scrolled/entryState'; +import React, {useCallback, useEffect, useRef, useState} from 'react'; import {useI18n} from '../frontend/i18n'; -import {postCreateCommentThreadMessage} from './postMessage'; -import {useSubjectQuote} from './subjectQuote'; +import {useCommentDraft, useCreateCommentThread} from './ReviewStateProvider'; import {autoGrow, autoResize} from './autoGrow'; import {isSubmitShortcut} from './submitShortcut'; import SendIcon from './images/send.svg'; +import SpinnerIcon from '../frontend/icons/spinner.svg'; import styles from './NewThreadForm.module.css'; export function NewThreadForm({subjectType, subjectId, subjectRange, onSubmit}) { const {t} = useI18n({locale: 'ui'}); - const [body, setBody] = useState(''); - const hasText = body.trim().length > 0; - const sectionPermaId = useSectionPermaIdOfSubject({subjectType, subjectId}); + const {body, setBody, submitting} = useDraftedBody({subjectType, subjectId}); + const hasText = body.trim().length > 0; - // Recorded now since the commented text can change afterwards, leaving the - // comment without the wording it referred to. - const quote = useSubjectQuote({subjectType, subjectId, subjectRange}); + const createCommentThread = useCreateCommentThread({ + subjectType, subjectId, subjectRange + }); // preventScroll keeps focus from yanking the page to the top before the // portaled popover has been positioned by floating-ui. @@ -47,35 +44,64 @@ export function NewThreadForm({subjectType, subjectId, subjectRange, onSubmit}) } function createThread() { - if (!hasText) return; + if (!hasText || submitting) return; - postCreateCommentThreadMessage({ - subjectType, subjectId, subjectRange, sectionPermaId, body, quote - }); - setBody(''); + createCommentThread(body); if (onSubmit) onSubmit(); } return ( -
+