Use switchMap when requesting change comments
Currently there is a race condition if the changeNum changes several
times in the short sequence. With introduction of switchMap, only the
latest change will end up updating the comments. Preventing the issues
where the comments are shown for the wrong change.
Additionally combine multiply separate modifyState calls. Each
individual call adds a an async boundary due to
https://cs.opensource.google/gerrit/gerrit/gerrit/+/master:polygerrit-ui/app/models/base/model.ts;l=49
Which makes it hard to reason about the state consistency.
Race condition is also possible between a manual call to
reloadAllComments (happens on reply), and RxJS subscribe based paths.
In order to avoid this issue, we introduce reloadAllComments$ manual
trigger. This way manual reload request will be resolved via the same
switchMap mechanism. This means that reloadAllComments no longer returns
a promise that resolves on completion, but awaiting that promise was not
used in the codebase.
gr-message-list_test needed fixing, since the reloading requires
changeNum to be set in the comments-model. Therefore we update it to
interact with ChangeViewModel.
As a bonus some documentation comments have been added to
comments-model.ts
Google-Bug-Id: b/328632912
Release-Notes: skip
Change-Id: Ib32318f12fa7c8812d3587bbf780d45816f8aff1
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
index df04f68..68fbe8b 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
@@ -33,7 +33,12 @@
import {GrButton} from '../../shared/gr-button/gr-button';
import {PaperToggleButtonElement} from '@polymer/paper-toggle-button';
import {testResolver} from '../../../test/common-test-setup';
-import {commentsModelToken} from '../../../models/comments/comments-model';
+import {TEST_PROJECT_NAME} from '../../../test/test-data-generators';
+import {
+ ChangeChildView,
+ changeViewModelToken,
+} from '../../../models/views/change';
+import {GerritView} from '../../../services/router/router-model';
const author = {
_account_id: 42 as AccountId,
@@ -138,9 +143,12 @@
element = await fixture<GrMessagesList>(
html`<gr-messages-list></gr-messages-list>`
);
- await testResolver(commentsModelToken).reloadComments(
- 0 as NumericChangeId
- );
+ testResolver(changeViewModelToken).setState({
+ view: GerritView.CHANGE,
+ childView: ChangeChildView.OVERVIEW,
+ changeNum: 123 as NumericChangeId,
+ repo: TEST_PROJECT_NAME,
+ });
element.messages = messages;
await element.updateComplete;
});
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index f26a565..41b47ef 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -7,7 +7,6 @@
import {
CommentInfo,
NumericChangeId,
- PatchSetNum,
RevisionId,
UrlEncodedCommentId,
RobotCommentInfo,
@@ -34,7 +33,14 @@
import {deepEqual} from '../../utils/deep-util';
import {select} from '../../utils/observable-util';
import {define} from '../dependency';
-import {combineLatest, forkJoin, from, Observable, of} from 'rxjs';
+import {
+ BehaviorSubject,
+ combineLatest,
+ forkJoin,
+ from,
+ Observable,
+ of,
+} from 'rxjs';
import {fire, fireAlert} from '../../utils/event-util';
import {CURRENT} from '../../utils/patch-set-util';
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
@@ -69,9 +75,20 @@
drafts?: {[path: string]: DraftInfo[]};
// Ported comments only affect `CommentThread` properties, not individual
// comments.
- /** undefined means 'still loading' */
+ /**
+ * Comments ported from earlier patchsets.
+ *
+ * This only considers current patchset (right side), not the base patchset
+ * (left-side).
+ *
+ * undefined means 'still loading'
+ */
portedComments?: {[path: string]: CommentInfo[]};
- /** undefined means 'still loading' */
+ /**
+ * Drafts ported from earlier patchsets.
+ *
+ * undefined means 'still loading'
+ */
portedDrafts?: {[path: string]: DraftInfo[]};
/**
* If a draft is discarded by the user, then we temporarily keep it in this
@@ -216,7 +233,7 @@
return nextState;
}
-/** Adds or updates a draft. */
+/** Adds or updates a draft in the state. */
export function setDraft(state: CommentState, draft: DraftInfo): CommentState {
const nextState = {...state};
assert(!!draft.path, 'draft without path');
@@ -235,6 +252,12 @@
return nextState;
}
+/** Removes a draft from the state.
+ *
+ * Removed draft is stored in discardedDrafts for potential undo operation.
+ * discardedDrafts however is only a client-side cache and such drafts are not
+ * retained in the server.
+ */
export function deleteDraft(
state: CommentState,
draft: DraftInfo
@@ -254,6 +277,10 @@
}
export const commentsModelToken = define<CommentsModel>('comments-model');
+/**
+ * Model that maintains the state of all comments and drafts for the current
+ * change in the context of change-view.
+ */
export class CommentsModel extends Model<CommentState> {
public readonly commentsLoading$ = select(
this.state$,
@@ -415,6 +442,8 @@
}
);
+ public readonly reloadAllComments$ = new BehaviorSubject(undefined);
+
public thread$(id: UrlEncodedCommentId) {
return select(this.threads$, threads => threads.find(t => t.rootId === id));
}
@@ -423,8 +452,6 @@
private changeNum?: NumericChangeId;
- private patchNum?: PatchSetNum;
-
private drafts: {[path: string]: DraftInfo[]} = {};
private draftToastTask?: DelayedTask;
@@ -464,9 +491,8 @@
this.subscriptions.push(
this.drafts$.subscribe(x => (this.drafts = x ?? {}))
);
- this.subscriptions.push(
- this.changeModel.patchNum$.subscribe(x => (this.patchNum = x))
- );
+ // Patchset-level draft should always exist when opening reply dialog.
+ // If there are none, create an empty one.
this.subscriptions.push(
combineLatest([
this.draftsLoading$,
@@ -478,41 +504,55 @@
})
);
this.subscriptions.push(
- this.changeViewModel.changeNum$.subscribe(changeNum => {
- this.changeNum = changeNum;
- this.setState({...initialState});
- this.reloadAllComments();
- })
+ combineLatest([this.changeViewModel.changeNum$, this.reloadAllComments$])
+ .pipe(
+ switchMap(([changeNum, _]) => {
+ this.changeNum = changeNum;
+ this.setState({...initialState});
+ if (!changeNum) return of([undefined, undefined, undefined]);
+ return forkJoin([
+ this.restApiService.getDiffComments(changeNum),
+ this.restApiService.getDiffRobotComments(changeNum),
+ this.restApiService.getDiffDrafts(changeNum),
+ ]);
+ })
+ )
+ .subscribe(([comments, robotComments, drafts]) => {
+ this.reportRobotCommentStats(robotComments);
+ this.modifyState(s => {
+ s = setComments(s, comments);
+ s = setRobotComments(s, robotComments);
+ return setDrafts(s, drafts);
+ });
+ })
);
+ // When the patchset selection changes update information about comments
+ // ported from earlier patchsets.
this.subscriptions.push(
- combineLatest([
- this.changeModel.changeNum$,
- this.changeModel.patchNum$,
- ]).subscribe(([changeNum, patchNum]) => {
- this.changeNum = changeNum;
- this.patchNum = patchNum;
- this.reloadAllPortedComments();
- })
+ combineLatest([this.changeModel.changeNum$, this.changeModel.patchNum$])
+ .pipe(
+ switchMap(([changeNum, patchNum]) => {
+ this.changeNum = changeNum;
+ if (!changeNum) return of([undefined, undefined]);
+ const revision = patchNum ?? (CURRENT as RevisionId);
+ return forkJoin([
+ this.restApiService.getPortedComments(changeNum, revision),
+ this.restApiService.getPortedDrafts(changeNum, revision),
+ ]);
+ })
+ )
+ .subscribe(([portedComments, portedDrafts]) =>
+ this.modifyState(s => {
+ s = setPortedComments(s, portedComments);
+ return setPortedDrafts(s, portedDrafts);
+ })
+ )
);
}
// Note that this does *not* reload ported comments.
- async reloadAllComments() {
- if (!this.changeNum) return;
- await Promise.all([
- this.reloadComments(this.changeNum),
- this.reloadRobotComments(this.changeNum),
- this.reloadDrafts(this.changeNum),
- ]);
- }
-
- async reloadAllPortedComments() {
- if (!this.changeNum) return;
- if (!this.patchNum) return;
- await Promise.all([
- this.reloadPortedComments(this.changeNum, this.patchNum),
- this.reloadPortedDrafts(this.changeNum, this.patchNum),
- ]);
+ reloadAllComments() {
+ this.reloadAllComments$.next(undefined);
}
// visible for testing
@@ -520,19 +560,6 @@
this.setState(reducer({...this.getState()}));
}
- async reloadComments(changeNum: NumericChangeId): Promise<void> {
- const comments = await this.restApiService.getDiffComments(changeNum);
- this.modifyState(s => setComments(s, comments));
- }
-
- async reloadRobotComments(changeNum: NumericChangeId): Promise<void> {
- const robotComments = await this.restApiService.getDiffRobotComments(
- changeNum
- );
- this.reportRobotCommentStats(robotComments);
- this.modifyState(s => setRobotComments(s, robotComments));
- }
-
private reportRobotCommentStats(obj?: PathToRobotCommentsInfoMap) {
if (!obj) return;
const comments = Object.values(obj).flat();
@@ -561,33 +588,6 @@
);
}
- async reloadDrafts(changeNum: NumericChangeId): Promise<void> {
- const drafts = await this.restApiService.getDiffDrafts(changeNum);
- this.modifyState(s => setDrafts(s, drafts));
- }
-
- async reloadPortedComments(
- changeNum: NumericChangeId,
- patchNum = CURRENT as RevisionId
- ): Promise<void> {
- const portedComments = await this.restApiService.getPortedComments(
- changeNum,
- patchNum
- );
- this.modifyState(s => setPortedComments(s, portedComments));
- }
-
- async reloadPortedDrafts(
- changeNum: NumericChangeId,
- patchNum = CURRENT as RevisionId
- ): Promise<void> {
- const portedDrafts = await this.restApiService.getPortedDrafts(
- changeNum,
- patchNum
- );
- this.modifyState(s => setPortedDrafts(s, portedDrafts));
- }
-
async restoreDraft(draftId: UrlEncodedCommentId) {
const found = this.discardedDrafts?.find(d => id(d) === draftId);
if (!found) throw new Error('discarded draft not found');
