In FileSystem.js, special-case the cp command by sdarwin · Pull Request #2264 · isomorphic-git/isomorphic-git

sdarwin · 2025-12-12T16:58:00Z

The Quick Start Guide uses LightningFS. That filesystem doesn't support cp. The new submodule feature added the cp command for testing purposes. It's only for preliminary test case setup, not during the real api calls, or in production. Therefore if an end-user has a filesystem such as LightningFS which is missing cp, they shouldn't be adversely affected.

FileSystem.js already special-cases the rm command. In this PR, handle cp the same way.

The Quick Start Guide was failing on 1.36.0 with a "bind" error. That should be solved here. Going back to 1.35, there are different errors about "Missing Buffer dependency". That's connected to the "Migrate from BrowserFS to ZenFS" commits, and are a separate problem.

Please test with the Quick Start Guide. If the "bind" error is solved, and it falls back to 1.35 "Missing Buffer dependency", at least that's progress, and it might be enough to solve end-user issues.

Summary by CodeRabbit

Refactor
- Improved internal handling and consistency of file-copy operations across different file system implementations (promise-based and callback-style).
- Treats copy as a distinct special case alongside remove to ensure reliable binding behavior.
- No public API changes; external interfaces remain unchanged.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-12T16:58:11Z

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

cp handling in the FileSystem class was moved out of the generic commands list and implemented as a special case in bindFs, with distinct binding for promise-based and callback-based filesystem implementations; rm remains a special case as before. No public API changes.

Changes

Cohort / File(s)	Summary
Command binding refactor `src/models/FileSystem.js`	Removed `cp` from the generic commands list; added dedicated `bindFs` logic to bind `cp` as a special case — directly assigned for promise-based FS, wrapped with `pify` for callback-style FS. `rm` handling unchanged aside from contextual comments.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Verify cp is correctly bound for both promise-based and callback-based filesystem implementations.
Check that pify wrapper behavior preserves expected cp semantics (errors, options, callback signatures).
Run existing tests covering cp and rm paths to ensure no regressions.

Poem

🐰 I hopped through code to move a clip,
Made cp its own narrow strip.
Promise or callback, both can play,
A tidy bind to lead the way.
Cheers — the filesystem skips and sips.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'In FileSystem.js, special-case the cp command' directly and clearly describes the main change in the pull request, which is to add special-case handling for the cp command in FileSystem.js, mirroring existing rm handling.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5cd05f9 and 1a48971.

📒 Files selected for processing (1)

src/models/FileSystem.js (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/models/FileSystem.js (1)

src/utils/rmRecursive.js (1)

rmRecursive (11-28)

🔇 Additional comments (2)

src/models/FileSystem.js (2)

21-23: LGTM! Comment accurately reflects special-case handling.

The updated comment correctly documents that both rm and cp are excluded from the generic commands list and require special-case handling.

48-59: The conditional binding correctly prevents binding errors for filesystems without cp support.

The implementation properly handles the cp method by only binding it when fs.cp exists (lines 50 and 55), preventing "bind" errors on filesystems like LightningFS that lack this method. The pattern mirrors the rm handling with appropriate conditional checks.

Note that unlike rm (which falls back to rmRecursive), cp has no fallback. However, this is intentional: _cp is only used in test setup code (FixtureFSSubmodule.js) with filesystems that support the cp method, so the lack of fallback is not a concern.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

jcubic

Looks good.

We really need to restore browser tests.

jcubic · 2025-12-12T18:49:09Z

src/models/FileSystem.js

+    if (fs.cp) target._cp = fs.cp.bind(fs)
+  } else {
+    if (fs.cp) target._cp = pify(fs.cp.bind(fs))
+  }


There is already an if statement that checks isPromiseFs(fs) can you put your wrappers there?

jcubic

After a second glance, this can be improved a bit.

Let's not duplicate code.

sdarwin · 2025-12-12T19:24:15Z

can you put your wrappers there?

done.

jcubic · 2025-12-13T14:26:11Z

NPM revoked all access tokens in December, and I'm unable to add a new token on Azure DevOps. I don't have permissions.

I will try to publish by hand locally.

We really need to migrate the GitHub actions now.

sdarwin · 2025-12-13T15:11:05Z

NPM revoked all access tokens in December,

isomorphic-git-bot says "Invalid npm token".

Well, probably log into npm, generate tokens, put them in Azure or GHA, or wherever they are used.

jcubic · 2025-12-13T19:16:14Z

Yes, NPM revoked all the NPM tokens a few days ago. They announced it a few months ago; there was a banner on the website. I thought that I could change the token because I had access settings. I remove the token but can't add a new one.

The Azure DevOps file is confusing. I have no idea what it's doing based on a file. Maybe GitHub Actions has a more straightforward flow.

jcubic · 2025-12-13T19:18:07Z

We need to fully migrate to GitHub Actions if we want to go forward with the project.

isomorphic-git-bot · 2025-12-14T08:33:13Z

🎉 This PR is included in version 1.36.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

jcubic · 2025-12-14T12:08:11Z

Billy (original author) replied on LinkedIn and gave me the access to Azure DevOps. It will work for now.

This was referenced Dec 12, 2025

TypeError: Cannot read properties of undefined (reading 'bind') when cloning #2262

Closed

v1.36.0 breaks in-browser cloning due to undefined fs property access #2263

Closed

jcubic approved these changes Dec 12, 2025

View reviewed changes

jcubic reviewed Dec 12, 2025

View reviewed changes

jcubic requested changes Dec 12, 2025

View reviewed changes

In FileSystem.js, special-case the cp command

1a48971

sdarwin force-pushed the submodules2 branch from 5cd05f9 to 1a48971 Compare December 12, 2025 18:59

jcubic merged commit dd54d92 into isomorphic-git:main Dec 13, 2025
4 checks passed

isomorphic-git-bot added the released label Dec 14, 2025

Uh oh!

Comments

Conversation

sdarwin commented Dec 12, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other AI code review bot(s) detected

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

jcubic left a comment

Choose a reason for hiding this comment

Uh oh!

jcubic Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

jcubic left a comment

Choose a reason for hiding this comment

Uh oh!

sdarwin commented Dec 12, 2025

Uh oh!

jcubic commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sdarwin commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcubic commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcubic commented Dec 13, 2025

Uh oh!

isomorphic-git-bot commented Dec 14, 2025

Uh oh!

jcubic commented Dec 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sdarwin commented Dec 12, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 12, 2025 •

edited

Loading

jcubic commented Dec 13, 2025 •

edited

Loading

sdarwin commented Dec 13, 2025 •

edited

Loading

jcubic commented Dec 13, 2025 •

edited

Loading