refactor(cli): simplify download progress rendering and tests

Author: fengmk2

crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/download_progress/download.mjs

@@ -8,8 +8,12 @@ import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { gzipSync } from 'node:zlib';
 
-// Small, valid archives keep this test offline. The installed files are never run.
-function archive(files) {
+/**
+ * Small, valid archives keep this test offline. The installed files are never run.
+ * @param {Record<string, string>} files
+ * @returns {Buffer}
+ */
+function createArchive(files) {
   const blocks = [];
   for (const [name, contents] of Object.entries(files)) {
     const body = Buffer.from(contents);
@@ -30,8 +34,8 @@ const version = '99.0.0';
 const musl = process.platform === 'linux' && !process.report.getReport().header.glibcVersionRuntime;
 const platform = `${process.platform}-${process.arch}${musl ? '-musl' : ''}`;
 const nodeRoot = `node-v${version}-${platform}`;
-const nodeArchive = archive({ [`${nodeRoot}/bin/node`]: '#!/bin/sh\nexit 0\n' });
-const npmArchive = archive({
+const nodeArchive = createArchive({ [`${nodeRoot}/bin/node`]: '#!/bin/sh\nexit 0\n' });
+const npmArchive = createArchive({
   'package/package.json': JSON.stringify({
     name: 'npm',
     version,
@@ -41,7 +45,7 @@ const npmArchive = archive({
   'package/bin/npx-cli.js': '// fixture\n',
 });
 let downloads = 0;
-const server = createServer((request, response) => {
+const server = createServer(function handleRequest(request, response) {
   if (request.url.endsWith('/SHASUMS256.txt.asc')) {
     response.writeHead(404).end();
     return;
@@ -50,25 +54,30 @@ const server = createServer((request, response) => {
     response.end(`${createHash('sha256').update(nodeArchive).digest('hex')}  ${nodeRoot}.tar.gz\n`);
     return;
   }
-  const body = request.url.endsWith(`/${nodeRoot}.tar.gz`)
-    ? nodeArchive
-    : request.url === `/npm/-/npm-${version}.tgz`
-      ? npmArchive
-      : undefined;
-  if (!body) {
+  let body;
+  if (request.url.endsWith(`/${nodeRoot}.tar.gz`)) {
+    body = nodeArchive;
+  } else if (request.url === `/npm/-/npm-${version}.tgz`) {
+    body = npmArchive;
+  } else {
     response.writeHead(404).end();
     return;
   }
   downloads++;
-  if (process.argv[2] === 'known') response.setHeader('Content-Length', body.length);
+  if (process.argv[2] === 'known') {
+    response.setHeader('Content-Length', body.length);
+  }
   response.flushHeaders();
   // Exercise multiple progress redraws; only the completed screen is snapshotted.
+  const chunkSize = Math.ceil(body.length / 8);
   let offset = 0;
-  const timer = setInterval(() => {
-    const end = Math.min(offset + Math.ceil(body.length / 8), body.length);
+  const timer = setInterval(function sendChunk() {
+    const end = Math.min(offset + chunkSize, body.length);
     response.write(body.subarray(offset, end));
     offset = end;
-    if (offset === body.length) response.end();
+    if (offset === body.length) {
+      response.end();
+    }
   }, 50);
   response.on('close', () => clearInterval(timer));
 });

crates/vp_shared/src/download_progress.rs

@@ -30,48 +30,50 @@ fn style_with_width(
 }
 
 fn download_row(state: &ProgressState, message: &str, available: usize) -> Str {
+    let has_total = state.len().is_some();
+    let message_width = measure_text_width(message);
     let bytes = HumanBytes(state.pos());
-    let counts = match state.len() {
+    let mut compact_stats = match state.len() {
         Some(total) => format!("{bytes}/{}", HumanBytes(total)),
         None => format!("{bytes}"),
     };
     let speed = HumanBytes(state.per_sec() as u64);
-    let details = match state.len() {
-        Some(_) => format!("{counts} ({speed}/s, {:#})", HumanDuration(state.eta())),
-        None => format!("{counts} ({speed}/s)"),
+    let detailed_stats = if has_total {
+        format!("{compact_stats} ({speed}/s, {:#})", HumanDuration(state.eta()))
+    } else {
+        format!("{compact_stats} ({speed}/s)")
     };
-    let fixed_width = measure_text_width(message) + measure_text_width(&details) + 2;
-    if state.len().is_some() {
-        // Keep a useful bar only when the full message and statistics fit.
-        // The leading space, brackets, and trailing space use three columns
-        // in addition to the two spaces included in `fixed_width`.
-        let bar_width = available.saturating_sub(fixed_width + 3);
+    let text_width = message_width + measure_text_width(&detailed_stats);
+    if has_total {
+        // Reserve three spaces and two brackets, plus at least four bar columns.
+        let bar_width = available.saturating_sub(text_width + 5);
         if bar_width >= 4 {
             let filled = (state.fraction() * bar_width as f32) as usize;
             let remaining = bar_width - filled;
             let bar = format!("{}{}", "#".repeat(filled), if remaining > 0 { ">" } else { "" });
             return format!(
-                " {message} [{}{}] {details}",
+                " {message} [{}{}] {detailed_stats}",
                 style(bar).blue(),
                 style("-".repeat(remaining.saturating_sub(1))).white(),
             );
         }
-    } else if fixed_width <= available {
-        return format!(" {message} {details}");
+    } else if text_width + 2 <= available {
+        return format!(" {message} {detailed_stats}");
     }
 
     // On narrow terminals, omit the bar, speed, and ETA before shortening the
     // message. For very small panes, prefer a percentage to two byte counts.
-    let counts = if state.len().is_some()
-        && measure_text_width(&counts) + 2 + measure_text_width(message).min(8) > available
-    {
-        format!("{:.0}%", state.fraction() * 100.0)
+    let compact_width = measure_text_width(&compact_stats) + 2 + message_width.min(8);
+    if has_total && compact_width > available {
+        compact_stats = format!("{:.0}%", state.fraction() * 100.0);
+    }
+    let available_message_width = available.saturating_sub(measure_text_width(&compact_stats) + 2);
+    let message = truncate_str(message, available_message_width, "");
+    if message.is_empty() {
+        format!(" {compact_stats}")
     } else {
-        counts
-    };
-    let message_width = available.saturating_sub(measure_text_width(&counts) + 2);
-    let message = truncate_str(message, message_width, "");
-    if message.is_empty() { format!(" {counts}") } else { format!(" {message} {counts}") }
+        format!(" {message} {compact_stats}")
+    }
 }
 
 #[cfg(test)]
@@ -114,19 +116,9 @@ mod tests {
                         for position in [0, 5 * 1024 * 1024, 25 * 1024 * 1024, total] {
                             progress.set_elapsed(Duration::from_secs(2));
                             progress.set_position(position);
-                            for reset_speed in [false, true] {
-                                if reset_speed {
-                                    progress.reset_eta();
-                                }
-                                progress.force_draw();
-                                let screen = term.contents();
-                                let lines: Vec<_> = screen.lines().collect();
-                                assert_eq!(lines.len(), 2, "width {width}: {screen}");
-                                assert_eq!(lines[0], earlier);
-                                assert!(measure_text_width(lines[1]) <= usize::from(width));
-                                let moves = term.moves_since_last_check();
-                                assert!(!moves.contains("Up("), "width {width}: {moves}");
-                            }
+                            assert_progress_row(&progress, &term, earlier);
+                            progress.reset_eta();
+                            assert_progress_row(&progress, &term, earlier);
                         }
                     }
 
@@ -140,6 +132,18 @@ mod tests {
         }
     }
 
+    fn assert_progress_row(progress: &ProgressBar, term: &InMemoryTerm, earlier: &str) {
+        progress.force_draw();
+        let width = term.width();
+        let screen = term.contents();
+        let lines: Vec<_> = screen.lines().collect();
+        assert_eq!(lines.len(), 2, "width {width}: {screen}");
+        assert_eq!(lines[0], earlier);
+        assert!(measure_text_width(lines[1]) <= usize::from(width));
+        let moves = term.moves_since_last_check();
+        assert!(!moves.contains("Up("), "width {width}: {moves}");
+    }
+
     #[test]
     fn compact_layout_preserves_download_counts() {
         for (width, has_bar) in [(40, false), (60, false), (80, true), (138, true)] {