feat(config): add unwrapSingleRootDir#15
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. 📝 WalkthroughWalkthroughRemoves the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sync
participant Fetch
participant Cache
participant Materialize
participant Targets
Sync->>Fetch: request source (git/archive) with source config
Fetch-->>Sync: FetchResult { repoDir, cleanup, fromCache }
Sync->>Cache: check manifest & rulesSha256 (includes unwrapSingleRootDir)
alt cache valid (rules match)
Sync->>Materialize: materialize from cache (unwrapSingleRootDir passed)
else cache miss or rules differ
Sync->>Materialize: download/build cache layout (unwrapSingleRootDir passed)
end
Materialize->>Targets: applyTargetDir (uses resolved sourceDir after unwrap)
Targets-->>Materialize: confirm symlink/copy applied
Materialize->>Cache: write manifest & toc
Sync-->>Cache: persist docs.lock / stats
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
commit: |
There was a problem hiding this comment.
Pull request overview
This pull request implements a new unwrapSingleRootDir feature that automatically flattens single-root directory structures during materialization, and removes the depth configuration option as a breaking change.
Changes:
- Removed
depthconfig option and hardcoded git clone depth to 1 - Added
unwrapSingleRootDirboolean option for recursive directory unwrapping during materialization - Enhanced sync output to show cache status ("Restoring from cache" vs "Downloading repo")
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config.ts | Removed depth from source and defaults interfaces, added unwrapSingleRootDir to source interfaces |
| src/config-schema.ts | Updated Zod schemas to remove depth and add unwrapSingleRootDir |
| src/sync.ts | Updated rules hash computation to include all materialization-affecting parameters including unwrapSingleRootDir |
| src/materialize.ts | Implemented recursive unwrap prefix resolution and path normalization during materialization |
| src/targets.ts | Added single-level unwrap logic for target directory application |
| src/git/fetch-source.ts | Hardcoded git depth to 1, added fromCache flag to track whether git archive or cached clone was used |
| docs.config.schema.json | Removed depth from JSON schema, added unwrapSingleRootDir |
| README.md | Updated documentation to remove depth and document unwrapSingleRootDir |
| tests/sync-materialize.test.js | Added tests for unwrap functionality and re-materialization trigger |
| tests/edge-cases.test.js | Updated to verify depth is rejected as unknown field |
| tests/edge-cases-validation.test.js | Removed tests for depth validation, added rejection test |
| tests/integration-real-repos.test.js | Refactored to verify lock file instead of checking returned plan object |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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/git/fetch-source.ts (1)
252-264:⚠️ Potential issue | 🟠 MajorThe shallow fetch strategy for commit refs is problematic but the suggested fix won't work reliably.
When
isCommitRefis true, the code fetches only the default branch at--depth 1. This fails if the commit is on a different branch or no longer in the default branch history. However, pushingparams.resolvedCommit(the commit SHA) directly togit fetchis not reliably supported across standard Git servers—it requires server-side config (uploadpack.allowReachableSHA1InWant) that defaults tofalse.There's also an inconsistency:
cloneRepofetches all branches for commit refs (no--single-branchrestriction), butcloneOrUpdateReporestricts the cache update to the default branch only. The fallback error handling masks the issue by falling back to a full re-clone when checkout fails, defeating the purpose of caching.Instead of the suggested fix, either fetch all branches when
isCommitRefis true, or remove the--depthlimit for commit refs to ensure the commit is available.
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
11-51: Consider reducing redundant CI work in the matrix job.The
precheckjob runs lint, typecheck, test, build, and size on Node 22. Thebuildjob then repeats all these steps across the full matrix (up to 9 combinations on PRs). This results in significant redundancy.Consider having the matrix job focus only on cross-platform/cross-version validation (test + build), while keeping lint, typecheck, audit, and size checks only in
precheck:♻️ Suggested optimization for build job steps
- name: Install dependencies run: pnpm install --frozen-lockfile - - name: Audit dependencies - run: pnpm audit --audit-level=high - - - name: Lint - run: pnpm lint - - - name: Typecheck - run: pnpm typecheck - - name: Test run: pnpm test - name: Build run: pnpm build - - - name: Size limit - run: pnpm sizesrc/errors.ts (1)
3-9: Type guard acceptsnumbercode, butgetErrnoCodeignores it.
isErrnoExceptionallowscodeto bestring | number | undefined, butNodeJS.ErrnoException.codeis typed asstring | undefined. The numeric check on line 8 is unnecessary since Node.js errno codes are always strings (e.g.,"ENOENT").This doesn't cause bugs since
getErrnoCodecorrectly filters for strings only, but removing the numeric check would align the guard with the actual Node.js type:♻️ Optional simplification
export const isErrnoException = (error: unknown): error is ErrnoException => typeof error === "object" && error !== null && "code" in error && - (typeof (error as ErrnoException).code === "string" || - typeof (error as ErrnoException).code === "number" || - (error as ErrnoException).code === undefined); + (typeof (error as ErrnoException).code === "string" || + (error as ErrnoException).code === undefined);src/cli/parse-args.ts (1)
20-27: Consider consolidating the redundant fields.
ParsedArgsnow contains both individual fields (command,options,positionals) and aparsedobject with equivalent data (parsed.command,parsed.options,parsed.args). This duplication could lead to maintenance overhead and potential inconsistencies.If the
CliCommandinterface is the preferred consumer contract, consider removing the redundant individual fields over time:♻️ Potential future simplification
export type ParsedArgs = { - command: Command | null; - options: CliOptions; - positionals: string[]; rawArgs: string[]; help: boolean; parsed: CliCommand; };src/sync.ts (1)
6-12: Consider moving DocsCacheResolvedSource intosrc/types/.It’s now shared across modules; relocating it would align with the shared-type guideline and keep imports consistent.
As per coding guidelines: "Place shared types in
src/types/and import them viaimport type".
Summary
Implements unwrapSingleRootDir feature to flatten single-root directories.
Changes
depthconfig option, use hardcoded default valueunwrapSingleRootDirboolean option to unwrap nested root directories recursivelyBreaking Changes
Removed
depthconfiguration option; uses hardcoded depth valueSummary by CodeRabbit
New Features
Bug Fixes
Removed Features
Documentation
Tests
Chores