Skip to content

Commit 604f904

Browse files
authored
fix(cli): prevent download progress from overwriting terminal output (#2741)
Node.js and package-manager downloads can erase earlier terminal output when the progress display moves the cursor upward. Both downloaders now fit the entire progress display to one terminal row. When space is limited, the display uses compact statistics and shortens the download name as needed. The layout uses the current terminal width on each redraw and clears the row when the download ends. **Narrow terminal fix** The video compares `7c51cc1` with `6bfe6c4` at 40, 60, 80, and 138 columns. It shows downloads with and without a known total size. https://github.com/user-attachments/assets/bb0f62bb-1e95-4c50-a9ed-e080af2b4a05 **Original terminal output fix** This video compares the reported recording with the first fix in `7c51cc1`. https://github.com/user-attachments/assets/af47dc70-1193-4a79-a98d-3de5b6c35232
1 parent d239360 commit 604f904

12 files changed

Lines changed: 414 additions & 36 deletions

File tree

‎Cargo.lock‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
import assert from 'node:assert/strict';
2+
import { spawn } from 'node:child_process';
3+
import { createHash } from 'node:crypto';
4+
import { once } from 'node:events';
5+
import { mkdtempSync, rmSync } from 'node:fs';
6+
import { createServer } from 'node:http';
7+
import { tmpdir } from 'node:os';
8+
import { join } from 'node:path';
9+
import { gzipSync } from 'node:zlib';
10+
11+
/**
12+
* Small, valid archives keep this test offline. The installed files are never run.
13+
* @param {Record<string, string>} files
14+
* @returns {Buffer}
15+
*/
16+
function createArchive(files) {
17+
const blocks = [];
18+
for (const [name, contents] of Object.entries(files)) {
19+
const body = Buffer.from(contents);
20+
const header = Buffer.alloc(512);
21+
header.write(name);
22+
header.write('0000755\0', 100);
23+
header.write(body.length.toString(8).padStart(11, '0') + '\0', 124);
24+
header.fill(' ', 148, 156);
25+
header.write('0', 156);
26+
const checksum = header.reduce((sum, byte) => sum + byte, 0);
27+
header.write(checksum.toString(8).padStart(6, '0') + '\0 ', 148);
28+
blocks.push(header, body, Buffer.alloc((512 - (body.length % 512)) % 512));
29+
}
30+
return gzipSync(Buffer.concat([...blocks, Buffer.alloc(1024)]));
31+
}
32+
33+
const version = '99.0.0';
34+
const musl = process.platform === 'linux' && !process.report.getReport().header.glibcVersionRuntime;
35+
const platform = `${process.platform}-${process.arch}${musl ? '-musl' : ''}`;
36+
const nodeRoot = `node-v${version}-${platform}`;
37+
const nodeArchive = createArchive({ [`${nodeRoot}/bin/node`]: '#!/bin/sh\nexit 0\n' });
38+
const npmArchive = createArchive({
39+
'package/package.json': JSON.stringify({
40+
name: 'npm',
41+
version,
42+
bin: { npm: 'bin/npm-cli.js', npx: 'bin/npx-cli.js' },
43+
}),
44+
'package/bin/npm-cli.js': '// fixture\n',
45+
'package/bin/npx-cli.js': '// fixture\n',
46+
});
47+
let downloads = 0;
48+
const server = createServer(function handleRequest(request, response) {
49+
if (request.url.endsWith('/SHASUMS256.txt.asc')) {
50+
response.writeHead(404).end();
51+
return;
52+
}
53+
if (request.url.endsWith('/SHASUMS256.txt')) {
54+
response.end(`${createHash('sha256').update(nodeArchive).digest('hex')} ${nodeRoot}.tar.gz\n`);
55+
return;
56+
}
57+
let body;
58+
if (request.url.endsWith(`/${nodeRoot}.tar.gz`)) {
59+
body = nodeArchive;
60+
} else if (request.url === `/npm/-/npm-${version}.tgz`) {
61+
body = npmArchive;
62+
} else {
63+
response.writeHead(404).end();
64+
return;
65+
}
66+
downloads++;
67+
if (process.argv[2] === 'known') {
68+
response.setHeader('Content-Length', body.length);
69+
}
70+
response.flushHeaders();
71+
// Exercise multiple progress redraws; only the completed screen is snapshotted.
72+
const chunkSize = Math.ceil(body.length / 8);
73+
let offset = 0;
74+
const timer = setInterval(function sendChunk() {
75+
const end = Math.min(offset + chunkSize, body.length);
76+
response.write(body.subarray(offset, end));
77+
offset = end;
78+
if (offset === body.length) {
79+
response.end();
80+
}
81+
}, 50);
82+
response.on('close', () => clearInterval(timer));
83+
});
84+
server.listen(0, '127.0.0.1');
85+
await once(server, 'listening');
86+
const mirror = `http://127.0.0.1:${server.address().port}`;
87+
// A separate home prevents mock installs from changing the runner's shared runtime seed.
88+
const home = mkdtempSync(join(tmpdir(), 'vp-download-progress-'));
89+
try {
90+
for (const tool of ['node', 'npm']) {
91+
console.log(`Before ${tool} download: preserve this output.`);
92+
const child = spawn('vp', ['env', 'install', `${tool}@${version}`], {
93+
stdio: 'inherit',
94+
env: { ...process.env, VP_HOME: home, VP_NODE_DIST_MIRROR: mirror, npm_config_registry: mirror },
95+
});
96+
const [code, signal] = await once(child, 'exit');
97+
assert.equal(signal, null);
98+
assert.equal(code, 0);
99+
console.log(`After ${tool} download.`);
100+
}
101+
assert.equal(downloads, 2);
102+
} finally {
103+
server.close();
104+
server.closeAllConnections();
105+
rmSync(home, { recursive: true, force: true });
106+
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
[[case]]
2+
name = "download_progress_known_length"
3+
vp = "global"
4+
# The mock Node distribution uses Unix tar archives.
5+
skip-platforms = ["windows"]
6+
steps = [["node", "download.mjs", "known"]]
7+
8+
[[case]]
9+
name = "download_progress_unknown_length"
10+
vp = "global"
11+
skip-platforms = ["windows"]
12+
steps = [["node", "download.mjs", "unknown"]]
13+
14+
[[case]]
15+
name = "download_progress_non_tty"
16+
vp = "global"
17+
skip-platforms = ["windows"]
18+
steps = [{ argv = ["node", "download.mjs", "known"], tty = false }]
19+
20+
[[case]]
21+
name = "download_progress_ci"
22+
vp = "global"
23+
skip-platforms = ["windows"]
24+
steps = [{ argv = ["node", "download.mjs", "known"], envs = [["CI", "1"]] }]
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# download_progress_ci
2+
3+
## `CI=1 node download.mjs known`
4+
5+
```
6+
Before node download: preserve this output.
7+
VITE+ - The Unified Toolchain for the Web
8+
9+
Installing Node.js <version>...
10+
Installed Node.js <version>
11+
After node download.
12+
Before npm download: preserve this output.
13+
VITE+ - The Unified Toolchain for the Web
14+
15+
Installing npm <version>...
16+
Installed npm <version>
17+
After npm download.
18+
```
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# download_progress_known_length
2+
3+
## `node download.mjs known`
4+
5+
```
6+
Before node download: preserve this output.
7+
VITE+ - The Unified Toolchain for the Web
8+
9+
Installing Node.js <version>...
10+
Installed Node.js <version>
11+
After node download.
12+
Before npm download: preserve this output.
13+
VITE+ - The Unified Toolchain for the Web
14+
15+
Installing npm <version>...
16+
Installed npm <version>
17+
After npm download.
18+
```
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# download_progress_non_tty
2+
3+
## `node download.mjs known`
4+
5+
```
6+
Before node download: preserve this output.
7+
Installing Node.js <version>...
8+
Installed Node.js <version>
9+
After node download.
10+
Before npm download: preserve this output.
11+
Installing npm <version>...
12+
Installed npm <version>
13+
After npm download.
14+
```
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# download_progress_unknown_length
2+
3+
## `node download.mjs unknown`
4+
5+
```
6+
Before node download: preserve this output.
7+
VITE+ - The Unified Toolchain for the Web
8+
9+
Installing Node.js <version>...
10+
Installed Node.js <version>
11+
After node download.
12+
Before npm download: preserve this output.
13+
VITE+ - The Unified Toolchain for the Web
14+
15+
Installing npm <version>...
16+
Installed npm <version>
17+
After npm download.
18+
```

‎crates/vp_js_runtime/src/download.rs‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use std::{fs::File, time::Duration};
77

88
use backon::{ExponentialBuilder, Retryable};
99
use futures_util::StreamExt;
10-
use indicatif::{ProgressBar, ProgressStyle};
10+
use indicatif::ProgressBar;
1111
use serde::de::DeserializeOwned;
1212
use sha2::{Digest, Sha256};
1313
use tokio::{
@@ -50,13 +50,8 @@ pub async fn download_file(
5050
let is_ci = vp_shared::EnvConfig::get().is_ci;
5151
let progress = if vp_shared::is_stderr_terminal() && !is_ci {
5252
let pb = ProgressBar::new_spinner();
53-
pb.set_style(
54-
ProgressStyle::default_spinner()
55-
.template("{msg}\n{spinner:.green} [{elapsed_precise}] {bytes} ({bytes_per_sec})")
56-
.expect("valid spinner template"),
57-
);
53+
pb.set_style(vp_shared::download_progress::download_style(message));
5854
pb.enable_steady_tick(Duration::from_millis(100));
59-
pb.set_message(message.to_string());
6055
Some(pb)
6156
} else {
6257
None
@@ -130,15 +125,6 @@ pub async fn download_file(
130125
pb.set_position(if is_resumed { resume_from } else { 0 });
131126
if let Some(size) = total_size {
132127
pb.set_length(size);
133-
pb.set_style(
134-
ProgressStyle::default_bar()
135-
.template(
136-
"{msg}\n{spinner:.green} [{elapsed_precise}] [{bar:40.blue/white}] \
137-
{bytes}/{total_bytes} ({bytes_per_sec}, {eta})",
138-
)
139-
.expect("valid progress bar template")
140-
.progress_chars("#>-"),
141-
);
142128
}
143129
}
144130

‎crates/vp_pm_cli/src/request.rs‎

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use std::{
66
use backon::{ExponentialBuilder, Retryable};
77
use flate2::read::GzDecoder;
88
use futures_util::stream::StreamExt;
9-
use indicatif::{ProgressBar, ProgressStyle};
9+
use indicatif::ProgressBar;
1010
use reqwest::{Response, StatusCode};
1111
use serde::de::DeserializeOwned;
1212
use sha1::Sha1;
@@ -140,7 +140,7 @@ impl HttpClient {
140140

141141
/// Download a file to a specified path
142142
///
143-
/// The optional `message` is displayed above a progress bar (e.g. "Downloading
143+
/// The optional `message` is displayed alongside a progress bar (e.g. "Downloading
144144
/// pnpm v10.0.0..."), shown only on a TTY and outside CI so piped/non-interactive
145145
/// output stays clean. Pass `None` for downloads that shouldn't surface progress
146146
/// (e.g. small metadata probes).
@@ -149,7 +149,7 @@ impl HttpClient {
149149
///
150150
/// * `url` - The URL of the file to download
151151
/// * `target_path` - The path where the file will be saved
152-
/// * `message` - Optional message shown above the progress bar
152+
/// * `message` - Optional message shown alongside the progress bar
153153
///
154154
/// # Returns
155155
///
@@ -175,15 +175,8 @@ impl HttpClient {
175175
&& !is_ci
176176
{
177177
let pb = ProgressBar::new_spinner();
178-
pb.set_style(
179-
ProgressStyle::default_spinner()
180-
.template(
181-
"{msg}\n{spinner:.green} [{elapsed_precise}] {bytes} ({bytes_per_sec})",
182-
)
183-
.expect("valid spinner template"),
184-
);
178+
pb.set_style(vp_shared::download_progress::download_style(message));
185179
pb.enable_steady_tick(Duration::from_millis(100));
186-
pb.set_message(message.to_string());
187180
Some(pb)
188181
} else {
189182
None
@@ -204,15 +197,6 @@ impl HttpClient {
204197
pb.set_position(0);
205198
if let Some(size) = response.content_length() {
206199
pb.set_length(size);
207-
pb.set_style(
208-
ProgressStyle::default_bar()
209-
.template(
210-
"{msg}\n{spinner:.green} [{elapsed_precise}] [{bar:40.blue/white}] \
211-
{bytes}/{total_bytes} ({bytes_per_sec}, {eta})",
212-
)
213-
.expect("valid progress bar template")
214-
.progress_chars("#>-"),
215-
);
216200
}
217201
}
218202
Self::write_response_to_file(response, target_path, progress.as_ref()).await

‎crates/vp_shared/Cargo.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ test-utils = ["dep:temp-env", "dep:tempfile"]
1818
directories = { workspace = true }
1919
nix = { workspace = true, features = ["fs", "poll", "term"] }
2020
console = { workspace = true }
21+
indicatif = { workspace = true }
2122
cow-utils = { workspace = true }
2223
serde = { workspace = true }
2324
# use `preserve_order` feature to preserve the order of the fields in `package.json`
@@ -41,6 +42,7 @@ rustls = { workspace = true }
4142
webpki-root-certs = { workspace = true }
4243

4344
[dev-dependencies]
45+
indicatif = { workspace = true, features = ["in_memory"] }
4446
serial_test = { workspace = true }
4547
tempfile = { workspace = true }
4648
# Enables the `test-utils` feature for this crate's own tests and doctests

0 commit comments

Comments
 (0)