Fix loading and state of <gr-repo>

The goal was to just fix the bug reported in b/266010917: This was
caused by the component being rendered with undefined `repoConfig`,
then handlers such as `handleDescriptionTextChanged` were called
with empty values at a moment where `repoConfig` was already loaded.

The immediate fix is just not render the DOM while `repoConfig` is
undefined or `loading` is true. That could have been achieved with a
few lines of change, but using a `loading` css class along with
`display:none` is so polymer-style that we are also cleaning this up
along the way.

Release-Notes: Fix the repository configuration loading and rendering when switching tabs
Bug: Issue 301481564
Google-Bug-Id: b/266010917
Change-Id: Iabb30640319583e1d646dc68d508ab89044aba8b

2 files changed, 122 insertions, 118 deletions

diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
index 01d452afd0..391aa55b3c 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
@@ -25,7 +25,7 @@ import {
   ProjectState,
   SubmitType,
 } from '../../../constants/constants';
-import {hasOwnProperty} from '../../../utils/common-util';
+import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util';
 import {firePageError, fireTitleChange} from '../../../utils/event-util';
 import {getAppContext} from '../../../services/app-context';
 import {WebLinkInfo} from '../../../types/diff';
@@ -36,8 +36,9 @@ import {subpageStyles} from '../../../styles/gr-subpage-styles';
 import {sharedStyles} from '../../../styles/shared-styles';
 import {BindValueChangeEvent} from '../../../types/events';
 import {deepClone} from '../../../utils/deep-util';
-import {LitElement, PropertyValues, css, html} from 'lit';
+import {LitElement, PropertyValues, css, html, nothing} from 'lit';
 import {customElement, property, state} from 'lit/decorators.js';
+import {when} from 'lit/directives/when.js';
 import {subscribe} from '../../lit/subscription-controller';
 import {createSearchUrl} from '../../../models/views/search';
 
@@ -148,16 +149,6 @@ export class GrRepo extends LitElement {
           color: var(--deemphasized-text-color);
           content: ' *';
         }
-        .loading,
-        .hide {
-          display: none;
-        }
-        #loading.loading {
-          display: block;
-        }
-        #loading:not(.loading) {
-          display: none;
-        }
         #options .repositorySettings {
           display: none;
         }
@@ -185,49 +176,48 @@ export class GrRepo extends LitElement {
             >
           </div>
         </div>
-        <div id="loading" class=${this.loading ? 'loading' : ''}>
-          Loading...
-        </div>
-        <div id="loadedContent" class=${this.loading ? 'loading' : ''}>
-          ${this.renderDownloadCommands()}
-          <h2
-            id="configurations"
-            class="heading-2 ${configChanged ? 'edited' : ''}"
-          >
-            Configurations
-          </h2>
-          <div id="form">
-            <fieldset>
-              ${this.renderDescription()} ${this.renderRepoOptions()}
-              ${this.renderPluginConfig()}
-              <gr-button
-                ?disabled=${this.readOnly || !configChanged}
-                @click=${this.handleSaveRepoConfig}
-                >Save changes</gr-button
-              >
-            </fieldset>
-            <gr-endpoint-decorator name="repo-config">
-              <gr-endpoint-param
-                name="repoName"
-                .value=${this.repo}
-              ></gr-endpoint-param>
-              <gr-endpoint-param
-                name="readOnly"
-                .value=${this.readOnly}
-              ></gr-endpoint-param>
-            </gr-endpoint-decorator>
-          </div>
-        </div>
+        ${when(
+          this.loading || !this.repoConfig,
+          () => html`<div id="loading">Loading...</div>`,
+          () => html`<div id="loadedContent">
+            ${this.renderDownloadCommands()}
+            <h2
+              id="configurations"
+              class="heading-2 ${configChanged ? 'edited' : ''}"
+            >
+              Configurations
+            </h2>
+            <div id="form">
+              <fieldset>
+                ${this.renderDescription()} ${this.renderRepoOptions()}
+                ${this.renderPluginConfig()}
+                <gr-button
+                  ?disabled=${this.readOnly || !configChanged}
+                  @click=${this.handleSaveRepoConfig}
+                  >Save changes</gr-button
+                >
+              </fieldset>
+              <gr-endpoint-decorator name="repo-config">
+                <gr-endpoint-param
+                  name="repoName"
+                  .value=${this.repo}
+                ></gr-endpoint-param>
+                <gr-endpoint-param
+                  name="readOnly"
+                  .value=${this.readOnly}
+                ></gr-endpoint-param>
+              </gr-endpoint-decorator>
+            </div>
+          </div>`
+        )}
       </div>
     `;
   }
 
   private renderDownloadCommands() {
+    if (!this.schemes.length) return nothing;
     return html`
-      <div
-        id="downloadContent"
-        class=${!this.schemes || !this.schemes.length ? 'hide' : ''}
-      >
+      <div id="downloadContent">
         <h2 id="download" class="heading-2">Download</h2>
         <fieldset>
           <gr-download-commands
@@ -250,6 +240,7 @@ export class GrRepo extends LitElement {
   }
 
   private renderDescription() {
+    assertIsDefined(this.repoConfig, 'repoConfig');
     return html`
       <h3 id="Description" class="heading-3">Description</h3>
       <fieldset>
@@ -261,7 +252,7 @@ export class GrRepo extends LitElement {
           rows="4"
           monospace
           ?disabled=${this.readOnly}
-          .text=${this.repoConfig?.description ?? ''}
+          .text=${this.repoConfig.description ?? ''}
           @text-changed=${this.handleDescriptionTextChanged}
         ></gr-textarea>
       </fieldset>
@@ -723,8 +714,9 @@ export class GrRepo extends LitElement {
 
   private renderPluginConfig() {
     const pluginData = this.computePluginData();
+    if (!pluginData.length) return nothing;
     return html` <div
-      class="pluginConfig ${!pluginData || !pluginData.length ? 'hide' : ''}"
+      class="pluginConfig"
       @plugin-config-changed=${this.handlePluginConfigChanged}
     >
       <h3 class="heading-3">Plugins</h3>
@@ -760,6 +752,12 @@ export class GrRepo extends LitElement {
   // private but used in test
   async loadRepo() {
     if (!this.repo) return Promise.resolve();
+    this.repoConfig = undefined;
+    this.originalConfig = undefined;
+    this.loading = true;
+    this.weblinks = [];
+    this.schemesObj = undefined;
+    this.readOnly = true;
 
     const promises = [];
 
@@ -1119,6 +1117,7 @@ export class GrRepo extends LitElement {
 
   private handleDescriptionTextChanged(e: BindValueChangeEvent) {
     if (!this.repoConfig || this.loading) return;
+    if (this.repoConfig.description === e.detail.value) return;
     this.repoConfig = {
       ...this.repoConfig,
       description: e.detail.value,
@@ -1128,6 +1127,7 @@ export class GrRepo extends LitElement {
 
   private handleStateSelectBindValueChanged(e: BindValueChangeEvent) {
     if (!this.repoConfig || this.loading) return;
+    if (this.repoConfig.state === e.detail.value) return;
     this.repoConfig = {
       ...this.repoConfig,
       state: e.detail.value as ProjectState,
@@ -1137,6 +1137,7 @@ export class GrRepo extends LitElement {
 
   private handleSubmitTypeSelectBindValueChanged(e: BindValueChangeEvent) {
     if (!this.repoConfig || this.loading) return;
+    if (this.repoConfig.submit_type === e.detail.value) return;
     this.repoConfig = {
       ...this.repoConfig,
       submit_type: e.detail.value as SubmitType,
diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
index 38c4a5da19..ef99a346c6 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
@@ -157,14 +157,17 @@ suite('gr-repo tests', () => {
     element = await fixture(html`<gr-repo></gr-repo>`);
   });
 
-  test('render', () => {
+  test('render', async () => {
+    element.repo = REPO as RepoName;
+    await element.loadRepo();
+    await element.updateComplete;
     // prettier and shadowDom assert do not agree about span.title wrapping
     assert.shadowDom.equal(
       element,
       /* prettier-ignore */ /* HTML */ `
       <div class="gr-form-styles main read-only">
         <div class="info">
-          <h1 class="heading-1" id="Title"></h1>
+          <h1 class="heading-1" id="Title">test-repo</h1>
           <hr />
           <div>
             <a href="">
@@ -178,7 +181,7 @@ suite('gr-repo tests', () => {
                 Browse
               </gr-button>
             </a>
-            <a href="">
+            <a href="/q/project:test-repo">
               <gr-button
                 aria-disabled="false"
                 link=""
@@ -190,15 +193,7 @@ suite('gr-repo tests', () => {
             </a>
           </div>
         </div>
-        <div class="loading" id="loading">Loading...</div>
-        <div class="loading" id="loadedContent">
-          <div class="hide" id="downloadContent">
-            <h2 class="heading-2" id="download">Download</h2>
-            <fieldset>
-              <gr-download-commands id="downloadCommands">
-              </gr-download-commands>
-            </fieldset>
-          </div>
+        <div id="loadedContent">
           <h2 class="heading-2" id="configurations">Configurations</h2>
           <div id="form">
             <fieldset>
@@ -266,7 +261,7 @@ suite('gr-repo tests', () => {
                   </span>
                 </section>
                 <section
-                  class="repositorySettings"
+                  class="repositorySettings showConfig"
                   id="enableSignedPushSettings"
                 >
                   <span class="title"> Enable signed push </span>
@@ -277,7 +272,7 @@ suite('gr-repo tests', () => {
                   </span>
                 </section>
                 <section
-                  class="repositorySettings"
+                  class="repositorySettings showConfig"
                   id="requireSignedPushSettings"
                 >
                   <span class="title"> Require signed push </span>
@@ -379,9 +374,6 @@ suite('gr-repo tests', () => {
                   </span>
                 </section>
               </fieldset>
-              <div class="hide pluginConfig">
-                <h3 class="heading-3">Plugins</h3>
-              </div>
               <gr-button
                 aria-disabled="true"
                 disabled=""
@@ -398,7 +390,51 @@ suite('gr-repo tests', () => {
           </div>
         </div>
       </div>
-    `
+    `,
+      {ignoreTags: ['option']}
+    );
+  });
+
+  test('render loading', async () => {
+    element.repo = REPO as RepoName;
+    element.loading = true;
+    await element.updateComplete;
+    // prettier and shadowDom assert do not agree about span.title wrapping
+    assert.shadowDom.equal(
+      element,
+      /* prettier-ignore */ /* HTML */ `
+      <div class="gr-form-styles main read-only">
+        <div class="info">
+          <h1 class="heading-1" id="Title">test-repo</h1>
+          <hr />
+          <div>
+            <a href="">
+              <gr-button
+                aria-disabled="true"
+                disabled=""
+                link=""
+                role="button"
+                tabindex="-1"
+              >
+                Browse
+              </gr-button>
+            </a>
+            <a href="/q/project:test-repo">
+              <gr-button
+                aria-disabled="false"
+                link=""
+                role="button"
+                tabindex="0"
+              >
+                View Changes
+              </gr-button>
+            </a>
+          </div>
+        </div>
+        <div id="loading">Loading...</div>
+      </div>
+    `,
+      {ignoreTags: ['option']}
     );
   });
 
@@ -451,55 +487,22 @@ suite('gr-repo tests', () => {
     assert.isTrue(requestUpdateStub.called);
   });
 
-  test('loading displays before repo config is loaded', () => {
-    assert.isTrue(
-      queryAndAssert<HTMLDivElement>(element, '#loading').classList.contains(
-        'loading'
-      )
-    );
-    assert.isFalse(
-      getComputedStyle(queryAndAssert<HTMLDivElement>(element, '#loading'))
-        .display === 'none'
-    );
-    assert.isTrue(
-      queryAndAssert<HTMLDivElement>(
-        element,
-        '#loadedContent'
-      ).classList.contains('loading')
-    );
-    assert.isTrue(
-      getComputedStyle(
-        queryAndAssert<HTMLDivElement>(element, '#loadedContent')
-      ).display === 'none'
-    );
-  });
-
-  test('download commands visibility', async () => {
-    element.loading = false;
-    await element.updateComplete;
-    assert.isTrue(
-      queryAndAssert<HTMLDivElement>(
-        element,
-        '#downloadContent'
-      ).classList.contains('hide')
-    );
-    assert.isTrue(
-      getComputedStyle(
-        queryAndAssert<HTMLDivElement>(element, '#downloadContent')
-      ).display === 'none'
-    );
+  test('render download commands', async () => {
+    element.repo = REPO as RepoName;
+    await element.loadRepo();
     element.schemesObj = SCHEMES;
     await element.updateComplete;
-    assert.isFalse(
-      queryAndAssert<HTMLDivElement>(
-        element,
-        '#downloadContent'
-      ).classList.contains('hide')
-    );
-    assert.isFalse(
-      getComputedStyle(
-        queryAndAssert<HTMLDivElement>(element, '#downloadContent')
-      ).display === 'none'
+    const content = queryAndAssert<HTMLDivElement>(element, '#downloadContent');
+    assert.dom.equal(
+      content,
+      /* HTML */ `
+        <div id="downloadContent">
+          <h2 class="heading-2" id="download">Download</h2>
+          <fieldset>
+            <gr-download-commands id="downloadCommands"></gr-download-commands>
+          </fieldset>
+        </div>
+      `
     );
   });
 
@@ -715,9 +718,9 @@ suite('gr-repo tests', () => {
         Promise.resolve(new Response())
       );
 
-      const button = queryAll<GrButton>(element, 'gr-button')[2];
-
       await element.loadRepo();
+
+      const button = queryAll<GrButton>(element, 'gr-button')[2];
       assert.isTrue(button.hasAttribute('disabled'));
       assert.isFalse(
         queryAndAssert<HTMLHeadingElement>(