Skip to content

feat(cli): add progress bar to generation pipeline#680

Open
shivxmsharma wants to merge 4 commits intonodejs:mainfrom
shivxmsharma:feat/cli-progress-bar
Open

feat(cli): add progress bar to generation pipeline#680
shivxmsharma wants to merge 4 commits intonodejs:mainfrom
shivxmsharma:feat/cli-progress-bar

Conversation

@shivxmsharma
Copy link
Contributor

Description

Adds a cli-progress progress bar to the generate command that tracks each target generator as it completes.

  • New src/utils/progressBar.mjs utility wraps cli-progress and writes to stderr to avoid conflicts with the existing logger (stdout)
  • Automatically disabled when stderr is not a TTY (CI environments, piped output)
  • New --no-progress flag to explicitly disable it

The bar starts after all generators are scheduled and increments once per target generator as its result collection finishes. The phase label shows the generator name:

  legacy-json [████████████████████████████████████████] 100% | 1/1

Validation

# Bar renders
node bin/cli.mjs generate -i "doc/api/*.md" -t legacy-json -o out -v 18.0.0

# Bar suppressed explicitly
node bin/cli.mjs generate -i "doc/api/*.md" -t legacy-json -o out -v 18.0.0 --no-progress

# Bar auto-suppressed in CI / piped output (stderr is not a TTY)
node bin/cli.mjs generate -i "doc/api/*.md" -t legacy-json -o out -v 18.0.0 2>/dev/null

All 312 existing tests pass.

Related Issues

Closes #58

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm test to check if all tests are passing.

@shivxmsharma shivxmsharma requested a review from a team as a code owner March 15, 2026 17:24
Copilot AI review requested due to automatic review settings March 15, 2026 17:24
@vercel
Copy link

vercel bot commented Mar 15, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
api-docs-tooling Ready Ready Preview Mar 21, 2026 10:18am

Request Review

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an optional CLI progress bar to the generation pipeline so users can see per-target generator completion while running doc-kit generate.

Changes:

  • Introduces src/utils/progressBar.mjs, a small wrapper around cli-progress that renders to stderr and auto-disables when stderr isn’t a TTY.
  • Hooks the progress bar into src/generators.mjs so it starts after scheduling and increments once each target generator’s result collection finishes.
  • Adds --no-progress to the generate command and wires it into runGenerators, plus adds the cli-progress dependency.

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/utils/progressBar.mjs New utility to create a TTY-aware progress bar that writes to stderr.
src/generators.mjs Starts/increments/stops the progress bar around target result collection.
bin/commands/generate.mjs Adds --no-progress and passes opts.progress into the generator runner.
package.json Adds cli-progress dependency.
package-lock.json Locks cli-progress and its transitive dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Member

@avivkeller avivkeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ovflowd given that the tooling is really fast, is a progress bar still needed?

errorWrap(async opts => {
const config = await setConfig(opts);
await runGenerators(config);
await runGenerators(config, opts.progress);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The progress bar should be a part of global config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I wait for @ovflowd's response or make the possible changes to this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO progress bar should be disabled/enabled behind cli arg since it's related to cli.

@shivxmsharma shivxmsharma force-pushed the feat/cli-progress-bar branch from 4f07217 to 57a244e Compare March 15, 2026 19:27
@vercel
Copy link

vercel bot commented Mar 15, 2026

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT ! and IMO it's still a good thing to have progress bar

@codecov
Copy link

codecov bot commented Mar 15, 2026

Codecov Report

❌ Patch coverage is 9.09091% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.63%. Comparing base (38c58bc) to head (b74b39e).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/utils/progressBar.mjs 0.00% 31 Missing ⚠️
src/generators.mjs 0.00% 7 Missing ⚠️
bin/commands/generate.mjs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #680      +/-   ##
==========================================
+ Coverage   74.74%   75.63%   +0.89%     
==========================================
  Files         146      150       +4     
  Lines       13255    13725     +470     
  Branches      960     1040      +80     
==========================================
+ Hits         9907    10381     +474     
+ Misses       3342     3338       -4     
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ovflowd
Copy link
Member

ovflowd commented Mar 18, 2026

cc @ovflowd given that the tooling is really fast, is a progress bar still needed?

I think a progress bar/visual indicator is good regardless. Allows us to see on what it is working. I'd argue it should be behind a feature flag such as --progress if we make it opt-in or --no-progress if we make it opt-out. I'd argue opt-out by default would be ideal, so we can disable it on CI.

It is a cool gimmick, but also genuinely brings "status" to what doc-kit is doing atm... Regardless of it being fast. What if I throw 20K md files on it?

@shivxmsharma
Copy link
Contributor Author

Hey @ovflowd, the current implementation already does this — --no-progress makes it opt-out (enabled by default), and it also auto-disables in CI via a process.stderr.isTTY check, so no flag is even needed there.

@avivkeller the progress flag is now part of the global Configuration object as well. Let me know if there's anything else to address!

Co-authored-by: Aviv Keller <me@aviv.sh>
Co-authored-by: Aviv Keller <me@aviv.sh>
@shivxmsharma
Copy link
Contributor Author

@avivkeller Committed the suggested changes!

return null;
}

return new SingleBar(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC, why this package versus other packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with cli-progress mainly because of its large user base (~4M weekly downloads) and the simple API. It does exactly what we need without any extra bloat.

await pool.destroy();

return results;
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, reverted! 👍

result = await streamingCache.getOrCollect(name, result);
}

progress?.increment({ phase: name });
Copy link
Member

@ovflowd ovflowd Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why progress?.?

}

const progress = createProgressBar({ enabled: configuration.progress });
progress?.start(generators.length, 0, { phase: 'Starting...' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why progress?.?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user disabled the progress bar, it'll be undefined

Copy link
Member

@ovflowd ovflowd Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehh, I'd argue that

if (!enabled || !process.stderr.isTTY) {
return null;
}

Shouldn't be inside createProgressBar, what's the point of calling createProgressBar just to return null? Id recommend instead of not creating the instance, simply making a mock stream.

so:

import { PassThrough } from 'node:stream';

// ...
stream: shouldEnable ? process.stdout : new PassThrough();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion, done! Now using a PassThrough stream when disabled instead of returning null. Much cleaner.

"@rollup/plugin-virtual": "^3.0.2",
"@swc/html-wasm": "^1.15.18",
"acorn": "^8.16.0",
"cli-progress": "^3.12.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last release was 3y ago, although I'm fine with not having releases that often, any particular reason why this package instead of others?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems stable and mature — no breaking changes or open security issues. The last release is just because there's nothing to fix. Open to switching if you have a preference though!

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation of a ProgrressBar only provides Progress per generator, in the future we'd probably want to expose the Progress Bar as an Progress API to each generator so they can have more granular control.

@shivxmsharma
Copy link
Contributor Author

This implementation of a ProgrressBar only provides Progress per generator, in the future we'd probably want to expose the Progress Bar as an Progress API to each generator so they can have more granular control.

Agreed, that would be a nice follow-up. Happy to work on that in a separate PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add cli-progress (Progress Bar) for CLI

5 participants