feat(config): enable unwrapSingleRootDir by default#23
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. 📝 WalkthroughWalkthroughDefault for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cache/materialize.ts (1)
172-179:⚠️ Potential issue | 🔴 CriticalPass
unwrapSingleRootDirtocomputeManifestHashcall to align withmaterializeSource.The
computeManifestHashcall (line 587–596 in sync.ts) omits theunwrapSingleRootDirparameter whilematerializeSourceexplicitly passes it. Whensource.unwrapSingleRootDirisfalse, this causes a mismatch: the materialized manifest is generated without unwrapping, but the hash is computed with unwrapping enabled (the default). This triggers false cache invalidation and unnecessary re-materialization.Add
unwrapSingleRootDir: source.unwrapSingleRootDirto thecomputeManifestHashparameters.
There was a problem hiding this comment.
Pull request overview
This PR changes the default value of unwrapSingleRootDir from false to true, which will automatically flatten single-directory hierarchies in materialized output for a cleaner structure.
Changes:
- Updated default configuration to enable
unwrapSingleRootDirby default - Updated documentation to reflect the new default value
- Adjusted test expectations to account for unwrapped directory structures
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/config/index.ts | Changed unwrapSingleRootDir default from false to true in DEFAULT_CONFIG |
| src/cache/materialize.ts | Updated fallback default value for unwrapSingleRootDir parameter from false to true |
| README.md | Updated documentation table to indicate new default value of true |
| tests/sync-materialize.test.js | Adjusted test expectations for unwrapped paths; removed explicit unwrapSingleRootDir: true setting since it's now the default; added assertion for disabled unwrap case |
| tests/sync-include-exclude.test.js | Updated path assertions to expect unwrapped structure when single root directory exists |
| tests/edge-cases-security.test.js | Updated deeply nested directory test to expect fully unwrapped path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/source-id.ts`:
- Line 1: The current INVALID_ID_PATTERN only denies a small set of characters
and misses control/whitespace characters and Windows reserved names/trailing
dots/spaces; update validation to (1) expand the denylist to include control
characters and whitespace (e.g., \u0000-\u001F and \u007F and \s), (2) normalize
the ID by trimming trailing spaces/dots before checking reserved names, and (3)
explicitly reject Windows device names (CON, PRN, AUX, NUL, COM1..COM9,
LPT1..LPT9) case-insensitively; locate and modify the INVALID_ID_PATTERN and the
ID validation logic (the function/main validation path that uses
INVALID_ID_PATTERN and the checks around lines referenced 24-27) to perform
normalization first then apply the reserved-name check and the stricter pattern
rejection.
🧹 Nitpick comments (3)
tests/edge-cases-validation.test.js (1)
39-40: Tighten the error regex to avoid false positives.The alternation currently passes if either
sources.0.idor the generic phrase matches, which can let unrelated error messages slip through. Consider requiring the ID label and the invalid-character wording.♻️ Suggested update
- /sources\.0\.id|path separators|reserved characters/i, + /sources\.0\.id.*(path separators|reserved characters)/i,- /sources\.0\.id|path separators|reserved characters/i, + /sources\.0\.id.*(path separators|reserved characters)/i,Also applies to: 50-51
tests/edge-cases.test.js (2)
36-37: Make invalid‑ID error expectations more specific.The current alternation can pass on partial matches. Requiring the ID label plus the invalid‑character wording makes the test assert the intended message.
♻️ Suggested update
- /sources\.0\.id|path separators|reserved characters/i, + /sources\.0\.id.*(path separators|reserved characters)/i,- /sources\.0\.id|path separators|reserved characters/i, + /sources\.0\.id.*(path separators|reserved characters)/i,Also applies to: 49-50
129-130: Tighten the blank‑ID error regex.
/sources\.0\.id|blank/iwill pass if either token appears. Consider requiring the ID label plus “blank”.♻️ Suggested update
-await assert.rejects(() => loadConfig(configPath), /sources\.0\.id|blank/i); +await assert.rejects(() => loadConfig(configPath), /sources\.0\.id.*blank/i);
Summary
Enable unwrap single root directory by default for cleaner materialized output structure.
Changes
unwrapSingleRootDirchanged fromfalsetotrueSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation