fix(hooks): find formatter configs in parent dirs#364
fix(hooks): find formatter configs in parent dirs#364tsubasakong wants to merge 2 commits intoaffaan-m:mainfrom
Conversation
📝 WalkthroughWalkthroughThe post-edit-format hook now discovers formatter configurations through marker-based detection by walking parent directories, supporting multiple Biome and Prettier config file variants instead of relying solely on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/hooks/post-edit-format.js (1)
39-64:⚠️ Potential issue | 🟠 MajorKeep walking past
package.json-only directories.Line 55 returns on the first marker, so a nested
package.jsonstill masks a shared root.prettierrc/biome.json. In a workspace likepackages/foo/package.jsonplus a repo-root formatter config,detectFormatter()only inspectspackages/fooand formatting is skipped.🐛 Suggested fix
-const PROJECT_ROOT_MARKERS = ['package.json', ...BIOME_CONFIGS, ...PRETTIER_CONFIGS]; +const PROJECT_CONFIG_MARKERS = [...BIOME_CONFIGS, ...PRETTIER_CONFIGS]; function findProjectRoot(startDir) { let dir = startDir; + let packageRoot = null; while (true) { - if (PROJECT_ROOT_MARKERS.some(marker => fs.existsSync(path.join(dir, marker)))) { + if (PROJECT_CONFIG_MARKERS.some(marker => fs.existsSync(path.join(dir, marker)))) { return dir; } + + if (!packageRoot && fs.existsSync(path.join(dir, 'package.json'))) { + packageRoot = dir; + } const parentDir = path.dirname(dir); if (parentDir === dir) break; dir = parentDir; } - return startDir; + return packageRoot || startDir; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/post-edit-format.js` around lines 39 - 64, findProjectRoot currently returns immediately when it sees any PROJECT_ROOT_MARKERS (including package.json), which causes nested package.json dirs to mask a repo-level formatter config; change findProjectRoot so it prefers formatter configs (PRETTIER_CONFIGS or BIOME_CONFIGS) over package.json: while walking up, if you find any PRETTIER_CONFIGS or BIOME_CONFIGS file return that dir; if you find package.json record it as a fallback but continue walking; after reaching filesystem root return the recorded package.json dir if any, otherwise return startDir. Update the logic in findProjectRoot and reference PROJECT_ROOT_MARKERS, PRETTIER_CONFIGS, BIOME_CONFIGS, and the findProjectRoot function name when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hooks/hooks.test.js`:
- Around line 716-726: The test only writes a Unix `npx` shim
(fs.writeFileSync(path.join(binDir, 'npx'), ...)) but the hook script
(scripts/hooks/post-edit-format.js) resolves `npx.cmd` on Windows; update the
test to create the Windows shim as well (e.g., write a corresponding `npx.cmd`
file in binDir that writes the same logFile) or write both `npx` and `npx.cmd`
unconditionally so runScript and post-edit-format.js find the shim on all
platforms; modify the fs.writeFileSync calls that create the shim(s) in the test
near runScript to include the Windows variant (ensure executable/mode behavior
is handled appropriately).
---
Outside diff comments:
In `@scripts/hooks/post-edit-format.js`:
- Around line 39-64: findProjectRoot currently returns immediately when it sees
any PROJECT_ROOT_MARKERS (including package.json), which causes nested
package.json dirs to mask a repo-level formatter config; change findProjectRoot
so it prefers formatter configs (PRETTIER_CONFIGS or BIOME_CONFIGS) over
package.json: while walking up, if you find any PRETTIER_CONFIGS or
BIOME_CONFIGS file return that dir; if you find package.json record it as a
fallback but continue walking; after reaching filesystem root return the
recorded package.json dir if any, otherwise return startDir. Update the logic in
findProjectRoot and reference PROJECT_ROOT_MARKERS, PRETTIER_CONFIGS,
BIOME_CONFIGS, and the findProjectRoot function name when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fce34e9-c905-4a02-884e-9fa98d6c312b
📒 Files selected for processing (2)
scripts/hooks/post-edit-format.jstests/hooks/hooks.test.js
| fs.writeFileSync( | ||
| path.join(binDir, 'npx'), | ||
| `#!/usr/bin/env node\nconst fs = require('fs');\nfs.writeFileSync(${JSON.stringify(logFile)}, JSON.stringify({ cwd: process.cwd(), args: process.argv.slice(2) }));\n`, | ||
| { mode: 0o755 } | ||
| ); | ||
|
|
||
| try { | ||
| const stdinJson = JSON.stringify({ tool_input: { file_path: targetFile } }); | ||
| const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson, { | ||
| PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}`, | ||
| }); |
There was a problem hiding this comment.
Make the formatter shim work on Windows too.
Lines 716-726 only create an npx stub, but scripts/hooks/post-edit-format.js resolves npx.cmd on Windows. The hook will miss this shim there, so the new assertion on logFile becomes platform-dependent.
🪟 Suggested fix
+ const shimJs = path.join(binDir, 'npx-shim.js');
+ fs.writeFileSync(
+ shimJs,
+ `const fs = require('fs');\nfs.writeFileSync(${JSON.stringify(logFile)}, JSON.stringify({ cwd: process.cwd(), args: process.argv.slice(2) }));\n`
+ );
fs.writeFileSync(
path.join(binDir, 'npx'),
- `#!/usr/bin/env node\nconst fs = require('fs');\nfs.writeFileSync(${JSON.stringify(logFile)}, JSON.stringify({ cwd: process.cwd(), args: process.argv.slice(2) }));\n`,
+ `#!/usr/bin/env node\nrequire(${JSON.stringify(shimJs)});\n`,
{ mode: 0o755 }
);
+ fs.writeFileSync(
+ path.join(binDir, 'npx.cmd'),
+ `@echo off\r\nnode "%~dp0\\npx-shim.js" %*\r\n`,
+ { mode: 0o755 }
+ );As per coding guidelines, "Ensure cross-platform compatibility for Windows, macOS, and Linux via Node.js scripts".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fs.writeFileSync( | |
| path.join(binDir, 'npx'), | |
| `#!/usr/bin/env node\nconst fs = require('fs');\nfs.writeFileSync(${JSON.stringify(logFile)}, JSON.stringify({ cwd: process.cwd(), args: process.argv.slice(2) }));\n`, | |
| { mode: 0o755 } | |
| ); | |
| try { | |
| const stdinJson = JSON.stringify({ tool_input: { file_path: targetFile } }); | |
| const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson, { | |
| PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}`, | |
| }); | |
| const shimJs = path.join(binDir, 'npx-shim.js'); | |
| fs.writeFileSync( | |
| shimJs, | |
| `const fs = require('fs');\nfs.writeFileSync(${JSON.stringify(logFile)}, JSON.stringify({ cwd: process.cwd(), args: process.argv.slice(2) }));\n` | |
| ); | |
| fs.writeFileSync( | |
| path.join(binDir, 'npx'), | |
| `#!/usr/bin/env node\nrequire(${JSON.stringify(shimJs)});\n`, | |
| { mode: 0o755 } | |
| ); | |
| fs.writeFileSync( | |
| path.join(binDir, 'npx.cmd'), | |
| `@echo off\r\nnode "%~dp0\\npx-shim.js" %*\r\n`, | |
| { mode: 0o755 } | |
| ); | |
| try { | |
| const stdinJson = JSON.stringify({ tool_input: { file_path: targetFile } }); | |
| const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson, { | |
| PATH: `${binDir}${path.delimiter}${process.env.PATH || ''}`, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hooks/hooks.test.js` around lines 716 - 726, The test only writes a
Unix `npx` shim (fs.writeFileSync(path.join(binDir, 'npx'), ...)) but the hook
script (scripts/hooks/post-edit-format.js) resolves `npx.cmd` on Windows; update
the test to create the Windows shim as well (e.g., write a corresponding
`npx.cmd` file in binDir that writes the same logFile) or write both `npx` and
`npx.cmd` unconditionally so runScript and post-edit-format.js find the shim on
all platforms; modify the fs.writeFileSync calls that create the shim(s) in the
test near runScript to include the Windows variant (ensure executable/mode
behavior is handled appropriately).
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
Description
Summary\n- let post-edit-format treat formatter config files as project-root markers, not just package.json\n- add broader Prettier config variants to the shared detection list\n- add a hook test covering config-only repos with nested files\n\nCloses #362.\n
Validation:
Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesChecklist
node tests/run-all.js)