Port the BannerManager to HH3#8021
Conversation
🦋 Changeset detectedLatest commit: 926f0a1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Ports the BannerManager functionality into Hardhat v-next (HH3), enabling the CLI to fetch a remote banner configuration, cache it locally, and conditionally print banner messages.
Changes:
- Added a new
BannerManagerimplementation that fetchesbanner-config-v3.json, caches it, and throttles requests/displays. - Integrated banner display into the main CLI flow (post-task) and into
init. - Added a dedicated test suite covering banner fetch/caching/throttling behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| v-next/hardhat/src/internal/cli/banner-manager.ts | New BannerManager implementation with remote fetch + caching + throttling. |
| v-next/hardhat/src/internal/cli/main.ts | Shows banner after task execution in interactive non-CI environments with a short timeout. |
| v-next/hardhat/src/internal/cli/init/init.ts | Shows banner after init flow completes (currently without a timeout / environment gating). |
| v-next/hardhat/test/internal/cli/banner-manager.ts | New tests for BannerManager behavior (currently contains an intercept path issue). |
Comments suppressed due to low confidence (6)
v-next/hardhat/src/internal/cli/init/init.ts:141
showBanner()is called without a timeout here, which means the underlyinggetRequestcan block for the default request timeout (300s) if the network is slow/unreachable. This can unnecessarily stallhardhat --initflows. Consider passing a small timeout (similar tomain.ts) and/or gating banner display behind the same interactive-environment check (!isCi()andprocess.stdout.isTTY).
try {
const { BannerManager } = await import("../banner-manager.js");
const bannerManager = await BannerManager.getInstance();
await bannerManager.showBanner();
} catch (bannerError) {
log("Error showing banner", bannerError);
}
v-next/hardhat/src/internal/cli/banner-manager.ts:177
#isBannerConfigrequiresObject.getOwnPropertyNames(value).length === 4, which will reject otherwise-valid configs if the remote JSON ever adds new fields (forward-compat break), causing banners to silently stop working. Prefer validating only the required properties and ignoring additional ones.
return (
Object.getOwnPropertyNames(value).length === 4 &&
"enabled" in value &&
v-next/hardhat/src/internal/cli/banner-manager.ts:204
readCache()trusts the on-disk JSON shape (includingbannerConfig,lastDisplayTime, andlastRequestTime) without runtime validation/sanitization. A corrupted/hand-edited cache can lead to runtime exceptions (e.g.formattedMessagesnot being an array) or NaN time math. Consider validating the parsed cache object and discarding/repairing invalid fields before constructing theBannerManager.
async function readCache(): Promise<BannerCache> {
const cacheDir = await getCacheDir();
const bannerCacheFilePath = path.join(cacheDir, BANNER_CACHE_FILE_NAME);
try {
return await readJsonFile<BannerCache>(bannerCacheFilePath);
} catch (error) {
v-next/hardhat/src/internal/cli/banner-manager.ts:187
- The numeric fields in the banner config are only checked with
typeof ... === "number". This allowsNaN,Infinity, and negative values, which can result in unexpected throttling behavior (comparisons withNaNalways being false, negative intervals effectively disabling throttling, etc.). Consider enforcingNumber.isFinite(...)and>= 0for the interval fields.
"minSecondsBetweenDisplays" in value &&
typeof value.minSecondsBetweenDisplays === "number" &&
"minSecondsBetweenRequests" in value &&
typeof value.minSecondsBetweenRequests === "number"
v-next/hardhat/src/internal/cli/banner-manager.ts:14
- The debug namespace here (
hardhat:util:banner-manager) doesn't match the surrounding CLI namespaces (e.g.hardhat:core:cli:main,hardhat:cli:init,hardhat:cli:telemetry:*). Consider renaming to ahardhat:cli:*/hardhat:core:cli:*prefix to keep debug filtering consistent across the CLI.
const log = debug("hardhat:util:banner-manager");
v-next/hardhat/test/internal/cli/banner-manager.ts:155
- Test name has a grammatical typo:
should's displayshould beshouldn't display(or similar).
it("should's display the banner more often than minSecondsBetweenDisplays", async () => {
// Store a cache file with recent lastDisplayTime and high
// minSecondsBetweenDisplays
await writeJsonFile(path.join(cacheDir, BANNER_CACHE_FILE_NAME), {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
schaable
left a comment
There was a problem hiding this comment.
LGTM. I left two minor comments, feel free to apply them if you agree.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Luis Schaab <schaable@gmail.com>
c75614b to
926f0a1
Compare
No description provided.