Add the selected patchset for Checks as a URL parameter

Since we have a dedicated URL parameter for the checks patchset, we stop
mirroring the files patchset over to the checks patchset. That was only
a one-way sync and likely a bit confusing. It was also a stop-gap, such
that users could point to checks for a specific patchset using the files
patchset.

Release-Notes: skip
Google-Bug-Id: b/235185477
Change-Id: Ief5b2274c0cb8dbe63ede260f3455bee0387a4e6

4 files changed, 49 insertions, 38 deletions

diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 26fa0cb0d2..a4e26fe281 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -1246,13 +1246,16 @@ export class GrChecksResults extends LitElement {
   }
 
   private onPatchsetSelected(e: CustomEvent<{value: string}>) {
-    const patchset = Number(e.detail.value);
-    assert(!isNaN(patchset), 'selected patchset must be a number');
-    this.getChecksModel().setPatchset(patchset as PatchSetNumber);
+    let patchset: number | undefined = Number(e.detail.value);
+    assert(Number.isInteger(patchset), `patchset must be integer: ${patchset}`);
+    if (patchset === this.latestPatchsetNumber) patchset = undefined;
+    this.getChecksModel().updateStateSetPatchset(
+      patchset as PatchSetNumber | undefined
+    );
   }
 
   private goToLatestPatchset() {
-    this.getChecksModel().setPatchset(undefined);
+    this.getChecksModel().updateStateSetPatchset(undefined);
   }
 
   private createAttemptDropdownItems() {
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 10c2f58277..a35ca40163 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -21,6 +21,7 @@ import {
   RepoName,
   UrlEncodedCommentId,
   PARENT,
+  PatchSetNumber,
 } from '../../../types/common';
 import {AppElement, AppElementParams} from '../../gr-app-types';
 import {LocationChangeEventDetail} from '../../../types/events';
@@ -1398,6 +1399,10 @@ export class GrRouter implements Finalizable, NavigationService {
 
     const tab = queryMap.get('tab');
     if (tab) state.tab = tab;
+    const checksPatchset = Number(queryMap.get('checksPatchset'));
+    if (Number.isInteger(checksPatchset) && checksPatchset > 0) {
+      state.checksPatchset = checksPatchset as PatchSetNumber;
+    }
     const filter = queryMap.get('filter');
     if (filter) state.filter = filter;
     const attempt = stringToAttemptChoice(queryMap.get('attempt'));
diff --git a/polygerrit-ui/app/models/checks/checks-model.ts b/polygerrit-ui/app/models/checks/checks-model.ts
index 385aec3bb3..2b92529928 100644
--- a/polygerrit-ui/app/models/checks/checks-model.ts
+++ b/polygerrit-ui/app/models/checks/checks-model.ts
@@ -138,11 +138,6 @@ export interface ChecksProviderState {
 }
 
 interface ChecksState {
-  /**
-   * This is the patchset number selected by the user. The *latest* patchset
-   * can be picked up from the change model.
-   */
-  patchsetNumberSelected?: PatchSetNumber;
   /** Checks data for the latest patchset. */
   pluginStateLatest: {
     [name: string]: ChecksProviderState;
@@ -202,9 +197,10 @@ export class ChecksModel extends Model<ChecksState> implements Finalizable {
 
   private subscriptions: Subscription[] = [];
 
+  // TODO: Maybe consider migrating usages to `changeViewModel` directly.
   public checksSelectedPatchsetNumber$ = select(
-    this.state$,
-    state => state.patchsetNumberSelected
+    this.changeViewModel.checksPatchset$,
+    ps => ps
   );
 
   public checksSelectedAttemptNumber$ = select(
@@ -219,10 +215,12 @@ export class ChecksModel extends Model<ChecksState> implements Finalizable {
 
   public checksLatest$ = select(this.state$, state => state.pluginStateLatest);
 
-  public checksSelected$ = select(this.state$, state =>
-    state.patchsetNumberSelected
-      ? state.pluginStateSelected
-      : state.pluginStateLatest
+  public checksSelected$ = select(
+    combineLatest([this.state$, this.changeViewModel.checksPatchset$]),
+    ([state, ps]) => {
+      const checksPs = ps ? ChecksPatchset.SELECTED : ChecksPatchset.LATEST;
+      return this.getPluginState(state, checksPs);
+    }
   );
 
   public aPluginHasRegistered$ = select(
@@ -380,7 +378,6 @@ export class ChecksModel extends Model<ChecksState> implements Finalizable {
     readonly pluginsModel: PluginsModel
   ) {
     super({
-      patchsetNumberSelected: undefined,
       pluginStateLatest: {},
       pluginStateSelected: {},
     });
@@ -399,19 +396,6 @@ export class ChecksModel extends Model<ChecksState> implements Finalizable {
       this.checkToPluginMap$.subscribe(map => {
         this.checkToPluginMap = map;
       }),
-      combineLatest([
-        this.routerModel.routerPatchNum$,
-        this.changeModel.latestPatchNum$,
-      ]).subscribe(([routerPs, latestPs]) => {
-        this.latestPatchNum = latestPs;
-        if (latestPs === undefined) {
-          this.setPatchset(undefined);
-        } else if (typeof routerPs === 'number') {
-          this.setPatchset(routerPs);
-        } else {
-          this.setPatchset(latestPs);
-        }
-      }),
       this.firstLoadCompleted$
         .pipe(
           filter(completed => !!completed),
@@ -649,8 +633,10 @@ export class ChecksModel extends Model<ChecksState> implements Finalizable {
     this.setState(nextState);
   }
 
-  updateStateSetPatchset(patchsetNumberSelected?: PatchSetNumber) {
-    this.updateState({patchsetNumberSelected});
+  updateStateSetPatchset(num?: PatchSetNumber) {
+    this.changeViewModel.updateState({
+      checksPatchset: num === this.latestPatchNum ? undefined : num,
+    });
   }
 
   updateStateSetAttempt(attemptNumberSelected: AttemptChoice) {
@@ -661,10 +647,6 @@ export class ChecksModel extends Model<ChecksState> implements Finalizable {
     this.changeViewModel.updateState({filter: runFilterRegexp});
   }
 
-  setPatchset(num?: PatchSetNumber) {
-    this.updateStateSetPatchset(num === this.latestPatchNum ? undefined : num);
-  }
-
   reload(pluginName: string) {
     this.reloadSubjects[pluginName].next();
   }
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts
index 4c97f605d9..826ec6695b 100644
--- a/polygerrit-ui/app/models/views/change.ts
+++ b/polygerrit-ui/app/models/views/change.ts
@@ -9,6 +9,7 @@ import {
   RevisionPatchSetNum,
   BasePatchSetNum,
   ChangeInfo,
+  PatchSetNumber,
 } from '../../api/rest-api';
 import {GerritView} from '../../services/router/router-model';
 import {UrlEncodedCommentId} from '../../types/common';
@@ -25,22 +26,34 @@ import {ViewState} from './base';
 
 export interface ChangeViewState extends ViewState {
   view: GerritView.CHANGE;
+
   changeNum: NumericChangeId;
   project: RepoName;
   edit?: boolean;
   patchNum?: RevisionPatchSetNum;
   basePatchNum?: BasePatchSetNum;
   commentId?: UrlEncodedCommentId;
-  forceReload?: boolean;
-  openReplyDialog?: boolean;
   tab?: string;
+
+  /** Checks related view state */
+
+  /** selected patchset for check runs (undefined=latest) */
+  checksPatchset?: PatchSetNumber;
   /** regular expression for filtering check runs */
   filter?: string;
-  /** selected attempt for selected check runs */
+  /** selected attempt for check runs (undefined=latest) */
   attempt?: AttemptChoice;
 
+  /** State properties that trigger one-time actions */
+
+  /** for scrolling a Change Log message into view in gr-change-view */
   messageHash?: string;
+  /** for logging where the user came from */
   usp?: string;
+  /** triggers all change related data to be reloaded */
+  forceReload?: boolean;
+  /** triggers opening the reply dialog */
+  openReplyDialog?: boolean;
 }
 
 /**
@@ -85,6 +98,9 @@ export function createChangeUrl(
   }
   let suffix = `${range}`;
   const queries = [];
+  if (state.checksPatchset && state.checksPatchset > 0) {
+    queries.push(`checksPatchset=${state.checksPatchset}`);
+  }
   if (state.attempt) {
     if (state.attempt !== 'latest') queries.push(`attempt=${state.attempt}`);
   }
@@ -126,6 +142,11 @@ export const changeViewModelToken =
 export class ChangeViewModel extends Model<ChangeViewState | undefined> {
   public readonly tab$ = select(this.state$, state => state?.tab);
 
+  public readonly checksPatchset$ = select(
+    this.state$,
+    state => state?.checksPatchset
+  );
+
   public readonly attempt$ = select(this.state$, state => state?.attempt);
 
   public readonly filter$ = select(this.state$, state => state?.filter);