Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request migrates the codebase from fs-extra to native Node.js fs/promises APIs, updates configuration and dependency versions, and substantially revises architectural and procedural planning documentation to reflect refined module organization, error handling, and testing patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR migrates the CLI codebase away from fs-extra toward Node’s built-in node:fs / node:fs/promises, updates related tests/config, and bumps several internal/tooling dependencies.
Changes:
- Replace
fs-extrausage withnode:fs/promisesAPIs (readFile,writeFile,cp, etc.) across CLI/build/template code and tests. - Update TypeScript configs to remove
fs-extratypes and adjust test compiler options. - Bump package/tooling versions and remove
fs-extra/@types/fs-extrafrom direct dependencies.
Reviewed changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Removes fs-extra typings and reformats compiler options. |
| tests/tsconfig.json | Adds esModuleInterop and reformats. |
| tests/file-utils.test.ts | Migrates tests from fs-extra to node:fs/promises. |
| tests/create-shape.test.ts | Migrates test file IO and cleanup to node:fs/promises. |
| tests/create-preset.test.ts | Migrates test file IO and cleanup to node:fs/promises. |
| tests/create-plugin.test.ts | Migrates test file IO and cleanup to node:fs/promises. |
| src/utils/template-utils.ts | Replaces template copy from fs-extra to cp. |
| src/utils/file-utils.ts | Replaces fs-extra usage with readFile/writeFile, existsSync, and mkdir. |
| src/create/shape/create-shape.ts | Replaces template copy from fs-extra to cp. |
| src/create/preset/create-preset.ts | Replaces template copy from fs-extra to cp. |
| src/create/plugin/create-plugin.ts | Replaces template copy from fs-extra to cp. |
| src/cli.ts | Replaces fs-extra JSON read with readFile + JSON.parse. |
| src/build/build.ts | Replaces dynamic fs-extra path check with existsSync and static node:path. |
| src/build/build-tsc.ts | Replaces fs-extra checks/reads with existsSync + readFile. |
| src/build/build-prettier.ts | Replaces fs-extra reads/writes/exists with Node built-ins. |
| src/build/build-diststats.ts | Replaces fs-extra folder ops with Node built-ins. |
| src/build/build-distfiles.ts | Replaces fs-extra reads/writes/copy with Node built-ins. |
| package.json | Removes fs-extra deps, bumps internal/tooling versions, bumps package version and pnpm. |
| pnpm-lock.yaml | Reflects dependency removals/bumps. |
| files/empty-project/webpack.config.js | Replaces fs-extra JSON read with readFile + JSON.parse. |
| files/empty-project/package.json | Bumps template devDependency versions. |
| .planning/codebase/TESTING.md | Updates analysis date/content (but still references fs-extra in places). |
| .planning/codebase/STRUCTURE.md | Updates analysis date/content. |
| .planning/codebase/STACK.md | Updates analysis date/content (but still lists fs-extra as critical). |
| .planning/codebase/INTEGRATIONS.md | Updates analysis date/content (but still references fs-extra). |
| .planning/codebase/CONVENTIONS.md | Updates analysis date/content (but still includes fs-extra example). |
| .planning/codebase/CONCERNS.md | Updates analysis date/content. |
| .planning/codebase/ARCHITECTURE.md | Updates analysis date/content (but still says core depends on fs-extra). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
src/build/build-tsc.ts (1)
19-23: Minor cleanup: reusetsconfigPathfor read.You already compute
tsconfigPath; reusing it forreadFileavoids duplicate path composition and keeps the block tighter.♻️ Suggested small refactor
const tsconfigPath = path.join(basePath, file); if (existsSync(tsconfigPath)) { - const data = await readFile(path.join(basePath, file)); + const data = await readFile(tsconfigPath); return data.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/build-tsc.ts` around lines 19 - 23, Reuse the previously computed tsconfigPath instead of recomputing the path: when checking existsSync(tsconfigPath) and then calling readFile(...), change the readFile call to use readFile(tsconfigPath) so you avoid duplicating path.join(basePath, file); update the block around tsconfigPath, existsSync and readFile accordingly..planning/codebase/STRUCTURE.md (1)
7-38: Add language specifier to the directory tree code block.For consistency and proper rendering, specify a language (e.g.,
textorplaintext) for the directory tree.-``` +```text [project-root]/ ├── src/ # TypeScript source for CLI and build tasks🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/codebase/STRUCTURE.md around lines 7 - 38, The fenced code block showing the directory tree in .planning/codebase/STRUCTURE.md lacks a language specifier; update the opening fence from ``` to ```text (or ```plaintext) so the tree renders consistently, i.e., modify the code block that begins with the directory tree under the [project-root]/ listing to include the language specifier..planning/codebase/TESTING.md (1)
33-36: Add language specifier to fenced code block.The code block should specify a language for proper syntax highlighting and linting compliance.
-``` +```text tests/ ├── <feature>.test.ts # top-level test file for a single module/feature<details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.planning/codebase/TESTING.md around lines 33 - 36, Update the fenced code
block in TESTING.md to include a language specifier (e.g.,text orbash)
so the snippet "tests/ ├── .test.ts # top-level test file for a
single module/feature" is fenced astext tests/ ...which enables proper
syntax highlighting and satisfies linting rules for fenced code blocks.</details> </blockquote></details> <details> <summary>tests/create-plugin.test.ts (1)</summary><blockquote> `11-15`: **Consider improving error handling in tests.** The current pattern swallows errors from `createPluginTemplate` with only `console.error`, allowing the test to continue. If `createPluginTemplate` fails, the subsequent `readFile` will throw a different error, obscuring the root cause. Consider removing the try/catch to let test failures surface directly, or use `expect` assertions for error cases: ```typescript // Option 1: Let errors propagate naturally await createPluginTemplate("foo", "Foo", "", destDir); // Option 2: If errors are expected in some scenarios await expect(createPluginTemplate(...)).resolves.not.toThrow(); ``` Also applies to: 27-31 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/create-plugin.test.ts` around lines 11 - 15, The test currently catches errors from createPluginTemplate("foo", "Foo", "", destDir) and only logs them, which hides failures and causes later reads (e.g., readFile) to throw unrelated errors; remove the try/catch so the promise can reject and fail the test, or replace the catch with an explicit assertion such as await expect(createPluginTemplate("foo","Foo","", destDir)).resolves.not.toThrow(); apply the same change to the other occurrence around the second createPluginTemplate call so failures surface clearly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.planning/codebase/ARCHITECTURE.md:
- Around line 31-32: Update the Core/Template Layer dependency list in
ARCHITECTURE.md to remove the obsoletefs-extraentry and instead reference
the nativenode:fs/promisesAPI; keep the other entries (lookpath,
child_processexec) and the "Used by: Command actions insrc/create/*" note
unchanged so the document matches the current implementation.- Around line 59-62: The architecture doc incorrectly documents using fs.copy
(fs-extra) for copying project-specific files; update the text to reference the
native Node API cp from node:fs/promises instead. Specifically, change the
bullet that mentions "copy project-specific files via fs.copy" to say "copy
project-specific files via cp (node:fs/promises)" and ensure any mention of the
utility function name(s) such as copyEmptyTemplateFiles, replaceTokensInFile,
runInstall, and runBuild remains intact so readers can locate the implementation
when they search the docs.In @.planning/codebase/CONCERNS.md:
- Line 26: The recommendation currently suggests fs-extra
(fs.readJSON/fs.writeJSON) which contradicts the project's migration away from
fs-extra; update the guidance to use native fs/promises APIs in the example for
updatePackageFile: read the file at pkgPath with fs.readFile and JSON.parse to
get pkg, set pkg.name = packageName (and other fields), then write back using
fs.writeFile with JSON.stringify(pkg, null, 2) plus a trailing newline and UTF-8
encoding; mention these specific symbols (updatePackageFile, pkgPath,
packageName) so the example aligns with the native fs approach.In @.planning/codebase/CONVENTIONS.md:
- Around line 56-59: The example import block in CONVENTIONS.md still shows the
removed fs-extra package; update the example to use native Node APIs and reflect
the intended import order: third-party packages, Node built-ins (using the node:
namespace), then local relative imports. Replace theimport fs from "fs-extra"
line with the appropriate native import such asimport { promises as fs } from "node:fs"orimport fs from "node:fs"(orimport path from "node:path"as
already shown), and ensure the example still includes the local importimport { camelize, capitalize, dash } from "../../utils/string-utils.js";to demonstrate
final ordering.In @.planning/codebase/INTEGRATIONS.md:
- Line 26: Update the filesystem integration note to remove the reference to
fs-extraand state that the CLI now uses native Node.js filesystem APIs;
update the sentence that mentionssrc/utils/file-utils.ts,src/create/*
helpers and examples likesrc/create/plugin/create-plugin.ts(which used
files/create-plugin) to indicate those create/template helpers now rely on the
nativefs/fs/promisesimplementations instead offs-extra, and adjust any
wording so the planning doc accurately reflects the migration.In @.planning/codebase/STACK.md:
- Line 50: Update the STACK.md entry that still lists
fs-extra^11.x: remove
that bullet or replace it to state that the project no longer depends on
fs-extraand now uses native node:fs/promises APIs (and optionally note the
example filesrc/utils/file-utils.tsand package.json no longer reference
fs-extra) so the stack documentation matches the codebase change.In @.planning/codebase/TESTING.md:
- Around line 46-47: The docs still reference fs-extra APIs (fs.ensureDir,
fs.remove, and import fs from "fs-extra") though the codebase now uses native
node:fs/promises; update all occurrences mentioned (lines referencing
fs.ensureDir, fs.remove, "actualfs-extraoperations", "usingfs-extra", and
the example import block) to use the native API names and import pattern:
replace import fs from "fs-extra" with import { mkdir, rm, readFile, writeFile }
from "node:fs/promises" and change calls to use mkdir (with { recursive: true })
for ensureDir, rm for remove, and readFile/writeFile for file IO, adjusting
example templates and explanatory text accordingly so the docs match the
migrated codebase.In
@src/build/build-distfiles.ts:
- Around line 94-96: The writeFile calls in buildDistFiles (the lines writing
package.json into dist/cjs, dist/esm, and dist/browser) can fail with ENOENT if
those directories don't exist; before each writeFile (or once for distPath),
ensure the directories exist by creating the parent directories with mkdir
(fs.promises.mkdir) using { recursive: true } for "cjs", "esm", and "browser"
(or for distPath itself) so writeFile won't error when --dist is run without
prior TypeScript compilation.In
@src/utils/file-utils.ts:
- Around line 50-64: getDestinationDir currently calls mkdir(destPath)
unconditionally which throws EEXIST for an existing empty directory and loses
the original "ensureDir" behavior for nested paths; update the function to
either (a) only call mkdir when !existsSync(destPath) or (b) call
mkdir(destPath, { recursive: true }) so parents are created and no error is
thrown if the directory already exists, and ensure the function returns destPath
at the end. Reference: getDestinationDir, destPath, existsSync, mkdir.In
@tests/create-preset.test.ts:
- Around line 10-21: The test currently catches and swallows errors from
createPresetTemplate("foo", "Foo", "", destDir); which masks failures; remove
the empty catch (or rethrow the caught error) and instead wrap the code that
reads and asserts package.json (the JSON parse + expect(pkgInfo.name).toBe(...))
in a try block with the cleanup rm(destDir, { recursive: true, force: true })
moved into a finally block so any error from createPresetTemplate or the
assertions fails the test while cleanup always runs; reference
createPresetTemplate, readFile/pkgPath/pkgInfo/expect, and rm when applying the
change.In
@tests/create-shape.test.ts:
- Around line 10-21: The test currently swallows scaffold failures and doesn't
guarantee cleanup: when calling createShapeTemplate(...) in
tests/create-shape.test.ts, remove the silent catch that only console.errors and
instead let the error propagate (or rethrow) so the test fails where the
scaffold fails, and move the rm(destDir, { recursive: true, force: true })
cleanup into a finally block to ensure the temporary directory is removed on
both success and failure; apply the same try/finally (or
try/catch+rethrow+finally) pattern to the second test block (lines 27-38) that
also calls createShapeTemplate so both tests fail on scaffold errors and always
clean up.In
@tests/file-utils.test.ts:
- Around line 11-107: The test suite is performing async filesystem work inside
describe callbacks which must be synchronous; move all await calls
(mkdir(baseDir), writeFile(...) calls, replaceTokensInFiles(...),
replaceTokensInFile(...), and getDestinationDir(...) setup) into appropriate
beforeAll or beforeEach hooks and keep describe callbacks synchronous; make the
mkdir call idempotent by calling mkdir(baseDir, { recursive: true }) in a
beforeAll that runs before any file writes; keep the assertions and afterAll
cleanup (rm(...)) as-is.
Nitpick comments:
In @.planning/codebase/STRUCTURE.md:
- Around line 7-38: The fenced code block showing the directory tree in
.planning/codebase/STRUCTURE.md lacks a language specifier; update the opening
fence fromtotext (or ```plaintext) so the tree renders consistently,
i.e., modify the code block that begins with the directory tree under the
[project-root]/ listing to include the language specifier.In @.planning/codebase/TESTING.md:
- Around line 33-36: Update the fenced code block in TESTING.md to include a
language specifier (e.g.,text orbash) so the snippet "tests/ ├──
.test.ts # top-level test file for a single module/feature" is fenced
astext tests/ ...which enables proper syntax highlighting and satisfies
linting rules for fenced code blocks.In
@src/build/build-tsc.ts:
- Around line 19-23: Reuse the previously computed tsconfigPath instead of
recomputing the path: when checking existsSync(tsconfigPath) and then calling
readFile(...), change the readFile call to use readFile(tsconfigPath) so you
avoid duplicating path.join(basePath, file); update the block around
tsconfigPath, existsSync and readFile accordingly.In
@tests/create-plugin.test.ts:
- Around line 11-15: The test currently catches errors from
createPluginTemplate("foo", "Foo", "", destDir) and only logs them, which hides
failures and causes later reads (e.g., readFile) to throw unrelated errors;
remove the try/catch so the promise can reject and fail the test, or replace the
catch with an explicit assertion such as await
expect(createPluginTemplate("foo","Foo","", destDir)).resolves.not.toThrow();
apply the same change to the other occurrence around the second
createPluginTemplate call so failures surface clearly.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `971d75d5-f051-404f-a7e4-b8af13e3e9b5` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 04a8c7b46aa134fae5ef75e5a9e1b67c9195042b and 0cedb62c6d00b3fbaa225ed70ba484e5de1f62d5. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (27)</summary> * `.planning/codebase/ARCHITECTURE.md` * `.planning/codebase/CONCERNS.md` * `.planning/codebase/CONVENTIONS.md` * `.planning/codebase/INTEGRATIONS.md` * `.planning/codebase/STACK.md` * `.planning/codebase/STRUCTURE.md` * `.planning/codebase/TESTING.md` * `files/empty-project/package.json` * `files/empty-project/webpack.config.js` * `package.json` * `src/build/build-distfiles.ts` * `src/build/build-diststats.ts` * `src/build/build-prettier.ts` * `src/build/build-tsc.ts` * `src/build/build.ts` * `src/cli.ts` * `src/create/plugin/create-plugin.ts` * `src/create/preset/create-preset.ts` * `src/create/shape/create-shape.ts` * `src/utils/file-utils.ts` * `src/utils/template-utils.ts` * `tests/create-plugin.test.ts` * `tests/create-preset.test.ts` * `tests/create-shape.test.ts` * `tests/file-utils.test.ts` * `tests/tsconfig.json` * `tsconfig.json` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Chores
Documentation