Fix the findings from the RC9 test pass (#159-#163)

client/src/App.vue

@@ -155,7 +155,15 @@
             <span>{{ this.computeTransferButtonText() }}</span>
           </w-button>
         </div>
-        <w-flex class="xs1 align-self-center flex justify-start">
+        <!-- wrap: the grid cell is a fixed fraction of the row (125px at a
+             960px viewport), while the select, the integrity icon and Delete
+             together are wider than that. Without wrapping they overflowed the
+             cell and were drawn on top of the Install button in the next one -
+             two controls sharing the same pixels, so aiming for Install could
+             hit Delete (#160). Wrapping puts Delete on its own line instead.
+             At desktop widths everything still fits on one line, so this
+             changes nothing there. -->
+        <w-flex class="xs1 align-self-center flex justify-start wrap">
           <w-select
             v-if="this.options.magicmode == false"
             v-model="selectedLocalImage"
@@ -439,12 +447,30 @@ export default {
         return "";
       }
     },
+    // Shared by every path that writes the eMMC - install, magic and magic
+    // upload - so the refresh cannot be added to one and forgotten in the
+    // others.
+    //
+    // getInfo() is deliberately fetched once per page load, which is right for
+    // the version, revision and serial number but not for emmc_version: a flash
+    // is exactly what changes it. Without this the pipeline kept showing the
+    // image that was on the eMMC before, on the one screen meant to confirm the
+    // flash worked (#161).
+    async onFlashFinished() {
+      await axios.get(`/api/run_install_finished_commands`);
+      await this.getInfo();
+      this.installFinished = true;
+    },
     onSelectedFileChanged() {
       // Whatever the previous image checked out as says nothing about this one,
       // and leaving the old verdict up would leave Install enabled for a
       // filename nobody has verified yet.
       this.imageIntegrity = null;
-      if (this.$refs.integritychecker && this.selectedLocalImage) {
+      // Told unconditionally, including when the selection has been cleared -
+      // fileSelected() hides the icon for an empty name, but it only ever got
+      // called for a non-empty one, so deleting the selected image left its
+      // verdict on screen next to "Please select one" (#162).
+      if (this.$refs.integritychecker) {
         this.$refs.integritychecker.fileSelected(this.selectedLocalImage);
       }
     },
@@ -814,8 +840,7 @@ export default {
         } else if (data.state == "FINISHED") {
           if (this.previousState == "INSTALLING") {
             this.selectedLocalImage = null;
-            await axios.get(`/api/run_install_finished_commands`);
-            this.installFinished = true;
+            await this.onFlashFinished();
           } else if (this.previousState == "BACKUPING") {
             this.backupFile = "";
             this.getStatus();
@@ -829,12 +854,10 @@ export default {
             this.selectedLocalImage = data.filename;
           } else if (this.previousState == "MAGIC") {
             this.selectedRebuildImage = null;
-            await axios.get(`/api/run_install_finished_commands`);
-            this.installFinished = true;
+            await this.onFlashFinished();
           } else if (this.previousState == "UPLOADING_MAGIC") {
             this.selectedUploadImage = [];
-            await axios.get(`/api/run_install_finished_commands`);
-            this.installFinished = true;
+            await this.onFlashFinished();
           }
         } else if (data.state == "CANCELLED") {
           this.selectedGithubImage = null;

client/src/components/ProgressBar.vue

@@ -111,18 +111,22 @@ export default {
     },
     update: function() {
       let model = this.progress;
-      let timePassedSeconds = (Date.now() - model.timeStarted)/1000;
+      // Clamped: a start time in the future (the board and the browser do not
+      // share a clock) otherwise runs the elapsed count backwards too.
+      let timePassedSeconds = Math.max(0, (Date.now() - model.timeStarted)/1000);
       this.seconds = Math.floor(timePassedSeconds % 60) ;
       this.minutes = Math.floor(timePassedSeconds / (60));
       let progress = model.progress/100;
       this.bandwidth = model.bandwidth.toFixed(1);
 
       let secondsTotal = (timePassedSeconds/progress);
       let timeFinished = new Date(new Date(model.timeStarted).getTime() + secondsTotal*1000);
-      let timeRemaining = (timeFinished - Date.now())/1000;
+      // Clamped before the split, so neither part can carry the sign:
+      // Math.floor(-4 % 60) is -4, which is how "0m:-4s" reached the screen.
+      let timeRemaining = Math.max(0, (timeFinished - Date.now())/1000);
       this.secondsR = Math.floor(timeRemaining % 60);
       this.minutesR = Math.floor(timeRemaining / 60);
-      if(isNaN(this.secondsR) || this.seconds == -1){
+      if(isNaN(this.secondsR)){
         this.secondsR = 0
         this.minutesR = 0
       }
@@ -138,6 +142,14 @@ export default {
   padding: 0.35em 0;
   cursor: crosshair;
 }
+/* The three figures under the bar are spread by justify-space-between, which
+   leaves no space at all once the row is narrow: elapsed, rate and remaining
+   ran together as "3.7 MB/s3m:13s". A gap keeps them apart, and nowrap stops a
+   single figure being broken across lines (#163). */
+.wrapper {
+  gap: 0.6em;
+  white-space: nowrap;
+}
 .metrics-popup {
   position: fixed;
   width: min(820px, calc(100vw - 16px));
@@ -146,7 +158,10 @@ export default {
   z-index: 1000;
   padding: 0.4em 0.6em;
   border-radius: 6px;
-  background: rgba(20, 20, 20, 0.92);
+  /* Opaque. At 0.92 the page behind showed through the panel - the REFLASH
+     wordmark and the version line ran underneath the plots, which is exactly
+     where the numbers are read from (#163). */
+  background: #141414;
   color: #eee;
   box-shadow: 0 2px 10px rgba(0, 0, 0, 0.45);
   pointer-events: auto;

client/src/components/TheConfigUpdater.vue

@@ -18,13 +18,20 @@
       >
     </div>
     <p v-if="this.isConfigPresent">Config has been updated</p>
+    <!-- A way out that is not "click somewhere outside and hope". The dialog
+         offered only Set Serial Number, so anyone who opened it to read the
+         number had to guess that the backdrop dismisses it - and on the board's
+         own touchscreen there is not much backdrop to aim at (#163). -->
     <w-button
       xl
       outline
       class="ma1 btn"
       @click="clickUpdateConfig()"
       ><span>Set Serial Number</span></w-button
     >
+    <w-button xl outline class="ma1 btn" @click="dialog.show = false"
+      ><span>Cancel</span></w-button
+    >
   </w-dialog>
 </template>
 <script>
@@ -99,6 +106,15 @@ export default {
         }
       },
     },
+    // Tell the parent whenever the dialog goes away, however it went away -
+    // Cancel or the backdrop. Without this its `openSerialNumber` stayed true
+    // after a dismissal, so the watcher above never saw false -> true again and
+    // the button simply stopped opening the dialog.
+    "dialog.show"(is_shown) {
+      if (!is_shown) {
+        this.$emit("close");
+      }
+    },
   },
 };
 </script>

client/src/components/TheOptions.vue

@@ -58,12 +58,19 @@
         </div>
         <w-divider class="my6 mx-3"></w-divider>
         <h4>Actions</h4>
+        <!-- Both ask twice. They sit side by side and fired on a single click,
+             and the two outcomes are not equally cheap: a stray Shut down on a
+             headless board needs someone to walk over and power-cycle it. Same
+             two-click pattern as Delete, for the same reason - a native
+             confirm() blocks the page (#163). -->
         <div>
-          <w-button xl outline class="ma2" @click="$emit('reboot-board')">
-            <span>Reboot now</span>
+          <w-button xl outline class="ma2" @click="confirmAction('reboot')">
+            <span>{{ pending === "reboot" ? "Reboot?" : "Reboot now" }}</span>
           </w-button>
-          <w-button xl outline class="ma2" @click="$emit('shutdown-board')"
-            ><span>Shut down</span></w-button
+          <w-button xl outline class="ma2" @click="confirmAction('shutdown')"
+            ><span>{{
+              pending === "shutdown" ? "Shut down?" : "Shut down"
+            }}</span></w-button
           >
         </div>
       </w-flex>
@@ -84,6 +91,24 @@ export default {
       this.setOption(data);
       this.$emit("set-option", name, value);
     },
+    // First click arms the button, second one within a few seconds does it.
+    // Arming one disarms the other, so a click meant for Reboot cannot confirm
+    // a pending Shut down.
+    confirmAction(which) {
+      clearTimeout(this.pendingTimer);
+      if (this.pending !== which) {
+        this.pending = which;
+        this.pendingTimer = setTimeout(() => (this.pending = null), 4000);
+        return;
+      }
+      this.pending = null;
+      this.$emit(which === "reboot" ? "reboot-board" : "shutdown-board");
+    },
+  },
+  // A drawer that is closed and reopened must not still be holding an armed
+  // button from minutes ago.
+  beforeUnmount() {
+    clearTimeout(this.pendingTimer);
   },
   computed: mapGetters(["options"]),
   created() {
@@ -95,6 +120,9 @@ export default {
     open: Boolean,
   },
   data: () => ({
+    // "reboot", "shutdown", or null when neither is armed.
+    pending: null,
+    pendingTimer: null,
     radioItems: [
       { label: "Normal", value: 0 },
       { label: "90 degrees", value: 90 },

client/tests/unit/InstallGating.spec.js

@@ -169,3 +169,39 @@ describe('IntegrityChecker reports its verdict', () => {
     expect(w.vm.spinner_visible).toBe(false);
   });
 });
+
+// The badge describes one specific file. Clearing the selection - which is what
+// deleting the selected image does - has to clear the verdict with it, or a
+// green check from the file that is now gone sits next to "Please select one"
+// and reads as "the thing I am about to install is fine" (#162).
+describe('the integrity verdict belongs to the selected image (#162)', () => {
+  function stand(selected) {
+    const fileSelected = vi.fn();
+    const self = {
+      selectedLocalImage: selected,
+      imageIntegrity: true,
+      $refs: { integritychecker: { fileSelected } },
+    };
+    return { self, fileSelected };
+  }
+
+  it('tells the checker when the selection is cleared', () => {
+    const { self, fileSelected } = stand(null);
+
+    App.methods.onSelectedFileChanged.call(self);
+
+    expect(self.imageIntegrity).toBe(null);
+    // Called with the empty selection rather than skipped: fileSelected()
+    // hides the icon for an empty name, and skipping the call is exactly what
+    // left the old verdict on screen.
+    expect(fileSelected).toHaveBeenCalledWith(null);
+  });
+
+  it('still checks a newly selected image', () => {
+    const { self, fileSelected } = stand('fresh.img.xz');
+
+    App.methods.onSelectedFileChanged.call(self);
+
+    expect(fileSelected).toHaveBeenCalledWith('fresh.img.xz');
+  });
+})

client/tests/unit/ProgressBar.spec.js

@@ -96,3 +96,50 @@ describe('ProgressBar board metrics hover', () => {
     expect(w.vm.metricsVisible).toBe(false);
   });
 });
+
+// The countdown is a projection off a clock, so it can run backwards: a start
+// time in the future (the board and the browser do not share one), or a
+// progress figure at 100 while the estimate is still being recomputed. Neither
+// is a reason to show the user "0m:-4s".
+describe('the remaining time is never negative', () => {
+  function mountWith(progressModel) {
+    return shallowMount(ProgressBar, {
+      props: { revision: 'A8' },
+      global: {
+        mocks: { $store: { getters: { progress: progressModel } } },
+        stubs: { 'w-progress': true, 'w-flex': true, TheMetrics: true }
+      }
+    });
+  }
+
+  it('clamps a start time in the future', () => {
+    const w = mountWith({ progress: 50, bandwidth: 1.5, timeStarted: Date.now() + 60000 });
+    w.vm.update();
+    expect(w.vm.secondsR).toBeGreaterThanOrEqual(0);
+    expect(w.vm.minutesR).toBeGreaterThanOrEqual(0);
+    expect(w.vm.seconds).toBeGreaterThanOrEqual(0);
+    expect(w.vm.minutes).toBeGreaterThanOrEqual(0);
+  });
+
+  it('clamps a progress figure past 100', () => {
+    const w = mountWith({ progress: 140, bandwidth: 8, timeStarted: Date.now() - 30000 });
+    w.vm.update();
+    expect(w.vm.secondsR).toBeGreaterThanOrEqual(0);
+    expect(w.vm.minutesR).toBeGreaterThanOrEqual(0);
+  });
+
+  it('reports zero rather than NaN before any progress has been made', () => {
+    const w = mountWith({ progress: 0, bandwidth: 0, timeStarted: Date.now() });
+    w.vm.update();
+    expect(w.vm.secondsR).toBe(0);
+    expect(w.vm.minutesR).toBe(0);
+  });
+
+  it('still reports a real estimate for an ordinary transfer', () => {
+    // Half done after 60s, so about 60s left.
+    const w = mountWith({ progress: 50, bandwidth: 5, timeStarted: Date.now() - 60000 });
+    w.vm.update();
+    expect(w.vm.minutesR * 60 + w.vm.secondsR).toBeGreaterThan(50);
+    expect(w.vm.minutesR * 60 + w.vm.secondsR).toBeLessThan(70);
+  });
+});

client/tests/unit/TheOptions.spec.js

@@ -59,3 +59,57 @@ describe('TheOptions', () => {
     expect(binding[1]).toBe('screenRotation')
   })
 })
+
+// Reboot and Shut down sit next to each other and used to fire on one click.
+// The two outcomes are not equally cheap: a stray Shut down on a headless board
+// needs a person to go and power-cycle it (#163).
+describe('TheOptions destructive actions', () => {
+  it('does not reboot or shut down on the first click', () => {
+    const { wrapper } = mountOptions()
+
+    wrapper.vm.confirmAction('reboot')
+    wrapper.vm.confirmAction('shutdown')
+
+    expect(wrapper.emitted('reboot-board')).toBeUndefined()
+    expect(wrapper.emitted('shutdown-board')).toBeUndefined()
+  })
+
+  it('acts on the second click of the same button', () => {
+    const { wrapper } = mountOptions()
+
+    wrapper.vm.confirmAction('reboot')
+    wrapper.vm.confirmAction('reboot')
+
+    expect(wrapper.emitted('reboot-board')).toHaveLength(1)
+    expect(wrapper.emitted('shutdown-board')).toBeUndefined()
+  })
+
+  // Arming one has to disarm the other, or a click meant for Reboot would
+  // confirm a Shut down that was armed moments earlier - the precise mistake
+  // the confirmation exists to prevent.
+  it('arming the other button cancels the pending one', () => {
+    const { wrapper } = mountOptions()
+
+    wrapper.vm.confirmAction('reboot')
+    wrapper.vm.confirmAction('shutdown')
+    wrapper.vm.confirmAction('shutdown')
+
+    expect(wrapper.emitted('reboot-board')).toBeUndefined()
+    expect(wrapper.emitted('shutdown-board')).toHaveLength(1)
+  })
+
+  it('disarms itself after a few seconds so a stale click does not act', () => {
+    vi.useFakeTimers()
+    try {
+      const { wrapper } = mountOptions()
+
+      wrapper.vm.confirmAction('reboot')
+      vi.advanceTimersByTime(5000)
+      wrapper.vm.confirmAction('reboot')
+
+      expect(wrapper.emitted('reboot-board')).toBeUndefined()
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+})

reflash/download_test.go

@@ -63,17 +63,24 @@ func TestDownloadCancelStopsTheTransfer(t *testing.T) {
 
 	startTestDownload(t, srv.URL, "slow.img.xz", 64<<20)
 	path := filepath.Join(images_folder, "slow.img.xz")
+	// The bytes land in the partial now, and only become the real name once
+	// they are all there (#159) - so that is what says the transfer is under
+	// way.
+	part := path + ".part"
 	waitFor(t, func() bool {
-		fi, err := os.Stat(path)
+		fi, err := os.Stat(part)
 		return err == nil && fi.Size() > 0
 	})
 	cancelDownload(httptest.NewRecorder(), httptest.NewRequest("PUT", "/api/cancel_download", nil))
 
 	waitForState(t, CANCELLED)
 	waitFor(t, disconnected.Load)
-	if _, err := os.Stat(path); !os.IsNotExist(err) {
+	if _, err := os.Stat(part); !os.IsNotExist(err) {
 		t.Errorf("the cancelled download is still on the drive, stat err %v", err)
 	}
+	if _, err := os.Stat(path); !os.IsNotExist(err) {
+		t.Errorf("a cancelled download must not leave an image under the real name, stat err %v", err)
+	}
 	got, _ := os.ReadFile(mounts)
 	if !strings.HasSuffix(strings.TrimSpace(string(got)), "mounted "+MODE_RO) {
 		t.Errorf("drive not left read-only; mount calls %q", got)