fix: use windowsHide for git child processes on Windows#36
fix: use windowsHide for git child processes on Windows#36700software wants to merge 2 commits intonode-modules:masterfrom
Conversation
When running under pm2 there is no visible console; without windowsHide, Windows spawns flashing cmd windows for each execSync. close node-modules#35 Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces windowsHide: true to execSync options in index.js to prevent command windows from appearing on Windows. The review feedback suggests extending this change to line-diff.js for completeness and recommends including the cwd option in getUserNameSync to ensure consistency with other git commands.
| * Prevents obnoxious cmd window popping up on Windows when process runs via pm2 | ||
| * instead of via an already visible terminal. | ||
| */ | ||
| const baseExecOptions = { windowsHide: true }; |
There was a problem hiding this comment.
The PR description states that windowsHide: true should be set on all execSync options. However, line-diff.js (which is required at the end of this file) also uses execSync but has not been updated in this PR. To ensure the fix is comprehensive, please apply this change to line-diff.js as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
index.js (2)
47-47: Add regression assertions forexecSyncoptions.The current stubs validate command strings/return values but not the second
execSyncargument, so this PR’s core behavior (windowsHide: true) is not protected by tests. Please add assertions that verify options includewindowsHide: truefor both paths.Also applies to: 122-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` at line 47, The tests currently stub execSync but only assert the command string/return value; update the execSync stubs to also assert the second argument includes windowsHide: true (i.e., that options object merges/inherits baseExecOptions with windowsHide: true). Locate the places referencing execSync and baseExecOptions in index.js (both call paths mentioned) and add assertions in each stub handler that the options parameter has windowsHide: true (use an include/deep-equal check against { ...baseExecOptions, windowsHide: true } or verify options.windowsHide === true) so both execution branches are protected.
121-124: Usethis.cwdhere to keep repo resolution consistent.
getLastCommitSyncruns git commands withcwd: this.cwd, butgetUserNameSyncstill doesn’t. For instances created with a custom directory, this can readuser.namefrom the wrong context.🔧 Suggested patch
getUserNameSync() { - return execSync(`git ${this.gitDirStr} config user.name`, baseExecOptions) + return execSync(`git ${this.gitDirStr} config user.name`, { + ...baseExecOptions, + cwd: this.cwd, + }) .toString() .trim(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 121 - 124, getUserNameSync currently calls execSync with baseExecOptions but not this.cwd, causing repo resolution mismatch; update the execSync invocation inside getUserNameSync so it runs with cwd: this.cwd (e.g., merge or pass an options object that includes cwd: this.cwd along with baseExecOptions) while keeping the existing git command using this.gitDirStr, so behavior matches getLastCommitSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@index.js`:
- Line 47: The tests currently stub execSync but only assert the command
string/return value; update the execSync stubs to also assert the second
argument includes windowsHide: true (i.e., that options object merges/inherits
baseExecOptions with windowsHide: true). Locate the places referencing execSync
and baseExecOptions in index.js (both call paths mentioned) and add assertions
in each stub handler that the options parameter has windowsHide: true (use an
include/deep-equal check against { ...baseExecOptions, windowsHide: true } or
verify options.windowsHide === true) so both execution branches are protected.
- Around line 121-124: getUserNameSync currently calls execSync with
baseExecOptions but not this.cwd, causing repo resolution mismatch; update the
execSync invocation inside getUserNameSync so it runs with cwd: this.cwd (e.g.,
merge or pass an options object that includes cwd: this.cwd along with
baseExecOptions) while keeping the existing git command using this.gitDirStr, so
behavior matches getLastCommitSync.
700software
force-pushed
the
fix/windows-hide-exec-sync
branch
from
38de22d to
054db00
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/index-branch.test.js (1)
50-52: Also assertcwdin the deleted-branch fallback stub.This test currently checks
windowsHideonly, while the branch-path calls also depend oncwd. Adding thecwdassertion would make this test consistent and catch more regressions.Suggested patch
cp.execSync = (cmd, opts) => { assert.strictEqual(opts && opts.windowsHide, true, cmd); + assert.strictEqual(opts && opts.cwd, process.cwd(), cmd); if (cmd.includes('log -1 --pretty=format:')) return Buffer.from(fields.join(splitCharacter));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index-branch.test.js` around lines 50 - 52, The stub for cp.execSync in the deleted-branch fallback currently only asserts opts.windowsHide; update that stub (cp.execSync) to also assert the provided opts.cwd equals the expected working directory used elsewhere in this test (the repo path variable used for branch-path calls, e.g., repoPath or the test's cwd variable) so the fallback call validates both windowsHide and cwd before returning the stubbed Buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/index-branch.test.js`:
- Around line 50-52: The stub for cp.execSync in the deleted-branch fallback
currently only asserts opts.windowsHide; update that stub (cp.execSync) to also
assert the provided opts.cwd equals the expected working directory used
elsewhere in this test (the repo path variable used for branch-path calls, e.g.,
repoPath or the test's cwd variable) so the fallback call validates both
windowsHide and cwd before returning the stubbed Buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 867251c9-6060-4232-a3c2-35027b0bd739
📒 Files selected for processing (5)
index.jsline-diff.jstest/index-branch.test.jstest/index-config.test.jstest/line-diff.mock.test.js
✅ Files skipped from review due to trivial changes (1)
- index.js
1 participant
Problem I am facing
When
last-commit-logruns under pm2 on Windows, there is no visible console. EachexecSynccan spawn a flashing cmd window, which interferes with typing and is annoying!Solution
Set
windowsHide: trueon allchild_process.execSyncoptions (shared viabaseExecOptions).Issue
Closes #35
Summary by CodeRabbit