Implement optimistic updates The core of this change is that we are setting `editing=false` immediately if `gr-comment`'s `save()` method. And that we are updating the model with the new draft message right away in `comments-model`'s `saveDraft()` method. Everything else is polish. We are making use of the two draft states SAVING and ERROR such that the user and the model have a clear understanding whether the page can be closed or not. And we are hooking up with the router's new `blockNavigation` feature. The error scenario is now handled more carefully and more prominently. Screenshots: https://imgur.com/a/bMH4SDk Release-Notes: skip Google-Bug-Id: b/262228572 Change-Id: I89c5d00c0aa2e84511aa7dd86a7e75a7170b6082
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts index f4881ce..46288f4 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -29,6 +29,10 @@ isDraftOrUnsaved, isRobot, isUnsaved, + isSaving, + isError, + isSaved, + isDraft, } from '../../../types/common'; import {GrConfirmDeleteCommentDialog} from '../gr-confirm-delete-comment-dialog/gr-confirm-delete-comment-dialog'; import { @@ -63,16 +67,13 @@ import {createDiffUrl} from '../../../models/views/change'; import {userModelToken} from '../../../models/user/user-model'; import {modalStyles} from '../../../styles/gr-modal-styles'; - -const UNSAVED_MESSAGE = 'Unable to save draft'; +import {when} from 'lit/directives/when.js'; const FILE = 'FILE'; // visible for testing export const AUTO_SAVE_DEBOUNCE_DELAY_MS = 2000; -export const __testOnly_UNSAVED_MESSAGE = UNSAVED_MESSAGE; - declare global { interface HTMLElementEventMap { 'comment-editing-changed': CustomEvent<CommentEditingChangedDetail>; @@ -167,21 +168,11 @@ @property({type: String}) messagePlaceholder?: string; - /* private, but used in css rules */ - @property({type: Boolean, reflect: true}) - saving = false; - // GrReplyDialog requires the patchset level comment to always remain // editable. @property({type: Boolean, attribute: 'permanent-editing-mode'}) permanentEditingMode = false; - /** - * `saving` and `autoSaving` are separate and cannot be set at the same time. - * `saving` affects the UI state (disabled buttons, etc.) and eventually - * leaves editing mode, but `autoSaving` just happens in the background - * without the user noticing. - */ @state() autoSaving?: Promise<DraftInfo>; @@ -202,9 +193,6 @@ @state() unresolved = true; - @property({type: Boolean}) - unableToSave = false; - @property({type: Boolean, attribute: 'show-patchset'}) showPatchset = false; @@ -334,13 +322,9 @@ :host([collapsed]) { padding: var(--spacing-s) var(--spacing-m); } - :host([saving]) { - pointer-events: none; - } - :host([saving]) .actions, - :host([saving]) .robotActions, - :host([saving]) .date { - opacity: 0.5; + :host([error]) { + background-color: var(--error-background); + border-radius: var(--border-radius); } .header { align-items: center; @@ -509,8 +493,14 @@ } override render() { + if (!this.comment) return; if (isUnsaved(this.comment) && !this.editing) return; - const classes = {container: true, draft: isDraftOrUnsaved(this.comment)}; + this.toggleAttribute('saving', this.isSaving()); + this.toggleAttribute('error', this.isError()); + const classes = { + container: true, + draft: isDraftOrUnsaved(this.comment), + }; return html` <gr-endpoint-decorator name="comment"> <gr-endpoint-param name="comment" .value=${this.comment}> @@ -593,7 +583,7 @@ 'This draft is only visible to you. ' + "To publish drafts, click the 'Reply' or 'Start review' button " + "at the top of the change or press the 'a' key."; - if (this.unableToSave) { + if (this.isError()) { label += ' (Failed to save)'; tooltip = 'Unable to save draft. Please try to save again.'; } @@ -677,13 +667,19 @@ } private renderDate() { - if (!this.comment?.updated || this.collapsed) return; + if (this.isUnsaved() || this.collapsed) return; return html` <span class="date" tabindex="0" @click=${this.handleAnchorClick}> - <gr-date-formatter - withTooltip - .dateStr=${this.comment.updated} - ></gr-date-formatter> + ${when( + this.isSaved(), + () => html` + <gr-date-formatter + withTooltip + .dateStr=${this.comment!.updated} + ></gr-date-formatter> + `, + () => (this.comment as DraftInfo).__draft + )} </span> `; } @@ -719,7 +715,6 @@ class="editMessage" autocomplete="on" code="" - ?disabled=${this.saving} rows="4" .placeholder=${this.messagePlaceholder} text=${this.messageText} @@ -789,7 +784,6 @@ if (!isDraftOrUnsaved(this.comment)) return; return html` <div class="rightActions"> - ${this.autoSaving ? html`. ` : ''} ${this.renderDiscardButton()} ${this.renderEditButton()} ${this.renderCancelButton()} ${this.renderSaveButton()} ${this.renderCopyLinkIcon()} @@ -828,7 +822,7 @@ if (this.editing || this.permanentEditingMode) return; return html`<gr-button link - ?disabled=${this.saving} + ?disabled=${this.isSaving() && !this.autoSaving} class="action discard" @click=${this.discard} >Discard</gr-button @@ -837,11 +831,7 @@ private renderEditButton() { if (this.editing) return; - return html`<gr-button - link - ?disabled=${this.saving} - class="action edit" - @click=${this.edit} + return html`<gr-button link class="action edit" @click=${this.edit} >Edit</gr-button >`; } @@ -851,7 +841,7 @@ return html` <gr-button link - ?disabled=${this.saving} + ?disabled=${this.isSaving() && !this.autoSaving} class="action cancel" @click=${this.cancel} >Cancel</gr-button @@ -860,7 +850,7 @@ } private renderSaveButton() { - if (!this.editing && !this.unableToSave) return; + if (!this.editing && !this.isError()) return; return html` <gr-button link @@ -896,7 +886,6 @@ link secondary class="action show-fix" - ?disabled=${this.saving} @click=${() => this.handleShowFix()} > Show Fix @@ -1073,7 +1062,7 @@ // private, but visible for testing isSaveDisabled() { assertIsDefined(this.comment, 'comment'); - if (this.saving) return true; + if (this.isSaving() && !this.autoSaving) return true; return !this.messageText?.trimEnd(); } @@ -1169,7 +1158,7 @@ } async autoSave() { - if (this.saving || this.autoSaving) return; + if (this.isSaving() || this.autoSaving) return; if (!this.editing || !this.comment) return; if (!isDraftOrUnsaved(this.comment)) return; const messageToSave = this.messageText.trimEnd(); @@ -1177,7 +1166,7 @@ if (messageToSave === this.comment.message) return; try { - this.autoSaving = this.rawSave(messageToSave, {showToast: false}); + this.autoSaving = this.rawSave({showToast: false}); await this.autoSaving; } finally { this.autoSaving = undefined; @@ -1190,52 +1179,47 @@ } async save() { - if (!isDraftOrUnsaved(this.comment)) throw new Error('not a draft'); - // If it's an unsaved comment then it does not have a draftID yet which - // means sending another save() request will create a new draft - if (isUnsaved(this.comment) && this.saving) return; + assert(isDraftOrUnsaved(this.comment), 'can only save drafts'); + // There is a minimal chance of `isSaving()` being false between iterations + // of the below while loop. But this will be extremely rare and just lead + // to a harmless assertion error. So let's not bother. + if (this.isSaving() && !this.autoSaving) return; - try { - this.saving = true; - this.unableToSave = false; - if (this.autoSaving) { - this.comment = await this.autoSaving; + if (!this.permanentEditingMode) { + this.editing = false; + } + if (this.autoSaving) { + this.comment = await this.autoSaving; + } + // Depending on whether `messageToSave` is empty we treat this either as + // a discard or a save action. + const messageToSave = this.messageText.trimEnd(); + if (messageToSave === '') { + await this.getCommentsModel().discardDraft(id(this.comment)); + } else { + // No need to make a backend call when nothing has changed. + while (this.somethingToSave()) { + this.comment = await this.rawSave({showToast: true}); } - // Depending on whether `messageToSave` is empty we treat this either as - // a discard or a save action. - const messageToSave = this.messageText.trimEnd(); - if (messageToSave === '') { - // Don't try to discard UnsavedInfo. Nothing to do then. - if (this.comment) { - await this.getCommentsModel().discardDraft(id(this.comment)); - } - } else { - // No need to make a backend call when nothing has changed. - if ( - messageToSave !== this.comment?.message || - this.unresolved !== this.comment.unresolved - ) { - await this.rawSave(messageToSave, {showToast: true}); - } - } - if (!this.permanentEditingMode) { - this.editing = false; - } - } catch (e) { - this.unableToSave = true; - throw e; - } finally { - this.saving = false; } } + private somethingToSave() { + if (!this.comment) return false; + return ( + this.messageText.trimEnd() !== this.comment?.message || + this.unresolved !== this.comment.unresolved + ); + } + /** For sharing between save() and autoSave(). */ - private rawSave(message: string, options: {showToast: boolean}) { - if (!isDraftOrUnsaved(this.comment)) throw new Error('not a draft'); + private rawSave(options: {showToast: boolean}) { + assert(isDraftOrUnsaved(this.comment), 'can only save drafts'); + assert(!this.isSaving(), 'saving already in progress'); return this.getCommentsModel().saveDraft( { ...this.comment, - message, + message: this.messageText.trimEnd(), unresolved: this.unresolved, }, options.showToast @@ -1284,6 +1268,23 @@ ); this.closeDeleteCommentModal(); } + + isUnsaved() { + return isUnsaved(this.comment); + } + + isSaved() { + return !isDraft(this.comment) || isSaved(this.comment); + } + + /** Note that this can also mean that auto-saving is in-progress. */ + isSaving() { + return isSaving(this.comment); + } + + isError() { + return isError(this.comment); + } } declare global {