refactor: implement general improvements #455
Conversation
|
📝 WalkthroughWalkthroughAdds path normalization utilities and applies normalized path handling across filtering, shared exports, test utilities, and tests; removes a noisy runtime log and stops appending a trailing newline in a mock file handler. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{test,spec}.?(c|m)[jt]s?(x)📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-06-09T05:10:32.105ZApplied to files:
📚 Learning: 2026-01-08T03:33:34.184ZApplied to files:
📚 Learning: 2025-05-04T11:52:22.858ZApplied to files:
📚 Learning: 2026-01-08T03:33:34.184ZApplied to files:
📚 Learning: 2025-06-28T08:01:22.596ZApplied to files:
📚 Learning: 2025-06-14T05:20:20.149ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (3)
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 |
github-actions
bot
added
the
pkg: test-utils
luxass
force-pushed
the
general-improvements
branch
from
e63019c to
84fb905
Compare
🌏 Preview Deployments
Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
Added functions to normalize paths and patterns for matching, ensuring leading slashes are handled correctly. Updated tests to verify behavior with leading slashes and typical project filters.
Introduces `normalizePathForFiltering` and `normalizeTreeForFiltering` to standardize file paths and maintain directory structure. Enhances test coverage for these new functionalities.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
github-actions
bot
added
pkg: cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/shared/test/files.test.ts:
- Around line 162-285: The test file has two new describe blocks for
normalizePathForFiltering and normalizeTreeForFiltering accidentally nested
inside the preceding it("should match directories with trailing slash", ...)
test; fix this by closing that it block (and its parent
describe("findFileByPath", ...) if needed) before starting the new describe
blocks so describe("normalizePathForFiltering", ...) and
describe("normalizeTreeForFiltering", ...) are top-level (or direct children of
the intended describe); locate the trailing-slash test (the it that references
matching directories with trailing slash) and add the missing closing brace(s)
to terminate the it block (and outer describe if unclosed) so the subsequent
describe blocks are moved out of the it scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/cli/src/cmd/lockfile/info.tspackages/shared/src/files.tspackages/shared/src/filter.tspackages/shared/src/index.tspackages/shared/test/files.test.tspackages/shared/test/filter.test.tspackages/test-utils/src/fs-bridges/memory-fs-bridge.tspackages/test-utils/src/mock-store/handlers/files.tspackages/test-utils/test/fs-bridge/memory-fs-bridge.test.ts
💤 Files with no reviewable changes (1)
- packages/cli/src/cmd/lockfile/info.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{test,spec}.?(c|m)[jt]s?(x)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.?(c|m)[jt]s?(x): Use test file pattern: **/.{test,spec}.?(c|m)[jt]s?(x)
Import test utilities from path aliases (#test-utils/, #internal/test-utils/conditions) rather than @ucdjs/test-utils to avoid build dependencies and cyclic dependency issues
Use mockStoreApi() utility from #test-utils/mock-store with MSW-based mocking for UCD API
Use testdir() from vitest-testdirs to create temporary test directories for filesystem testing
Files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.tspackages/shared/test/filter.test.tspackages/shared/test/files.test.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: luxass
Repo: ucdjs/ucd PR: 6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
Learnt from: luxass
Repo: ucdjs/ucd PR: 20
File: packages/ucd-store/test/index.test.ts:1-7
Timestamp: 2025-05-31T07:44:02.123Z
Learning: User luxass prefers to implement functionality in separate PRs rather than including everything in a single large PR, keeping changes focused and manageable.
Learnt from: luxass
Repo: ucdjs/ucd PR: 85
File: packages/fetch/package.json:37-38
Timestamp: 2025-06-28T08:01:22.596Z
Learning: In the ucdjs/ucd project, relative paths in npm scripts within packages (like `../../apps/api/.generated/openapi.json` in packages/fetch/package.json) resolve correctly even when run via Turborepo from the repo root, contrary to initial concerns about working directory changes.
📚 Learning: 2025-05-04T11:52:22.858Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
Applied to files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.tspackages/shared/src/files.tspackages/shared/src/index.tspackages/test-utils/src/fs-bridges/memory-fs-bridge.tspackages/shared/test/filter.test.tspackages/shared/test/files.test.ts
📚 Learning: 2026-01-08T03:33:34.184Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T03:33:34.184Z
Learning: Implement File System Bridge pattern using ucdjs/fs-bridge abstraction layer to support different storage backends (Node.js, HTTP, in-memory)
Applied to files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.tspackages/test-utils/src/fs-bridges/memory-fs-bridge.ts
📚 Learning: 2026-01-08T03:33:34.184Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T03:33:34.184Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Import test utilities from path aliases (#test-utils/*, #internal/test-utils/conditions) rather than ucdjs/test-utils to avoid build dependencies and cyclic dependency issues
Applied to files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.tspackages/shared/test/filter.test.tspackages/shared/test/files.test.ts
📚 Learning: 2026-01-08T03:33:34.184Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T03:33:34.184Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Use testdir() from vitest-testdirs to create temporary test directories for filesystem testing
Applied to files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.ts
📚 Learning: 2025-06-14T05:20:20.149Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/validate.ts:115-130
Timestamp: 2025-06-14T05:20:20.149Z
Learning: Node.js 22 supports the `recursive` option in `fs.readdir()` (introduced in Node.js 18.17.0), which returns a flattened string array of all descendant files and directories when `recursive: true` is passed.
Applied to files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.tspackages/shared/test/filter.test.ts
📚 Learning: 2025-06-14T05:20:24.527Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/fs-adapter.ts:47-51
Timestamp: 2025-06-14T05:20:24.527Z
Learning: The `recursive` option for `fs.readdir()` was introduced in Node.js v20.1.0 and is available in Node.js 22. When `recursive: true` is passed, it will recursively traverse directories and return all file paths as a flat string array.
Applied to files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.ts
📚 Learning: 2025-06-14T05:20:24.527Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 56
File: packages/utils/src/ucd-files/fs-adapter.ts:47-51
Timestamp: 2025-06-14T05:20:24.527Z
Learning: In Node.js 22, the `recursive` option for `fs.readdir()` is supported and will recursively find all files and return them in a string array when set to true.
Applied to files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.ts
📚 Learning: 2025-07-09T15:05:57.763Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 97
File: packages/utils/src/fs-bridge/node.ts:40-50
Timestamp: 2025-07-09T15:05:57.763Z
Learning: The `parentPath` property on `fs.Dirent` was added in Node.js v20.12, contrary to previous assumptions about it not being part of the Node.js API.
Applied to files:
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.tspackages/test-utils/src/fs-bridges/memory-fs-bridge.ts
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Applied to files:
packages/shared/src/files.tspackages/shared/src/index.tspackages/shared/src/filter.tspackages/test-utils/src/mock-store/handlers/files.tspackages/test-utils/src/fs-bridges/memory-fs-bridge.tspackages/shared/test/filter.test.tspackages/shared/test/files.test.ts
📚 Learning: 2026-01-08T03:33:34.184Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T03:33:34.184Z
Learning: Applies to apps/api/src/routes/**/*.{ts,tsx} : Organize API routes by version and type: /api/v1/files, /api/v1/versions, /api/v1/schemas, /_tasks, /.well-known/
Applied to files:
packages/shared/src/files.ts
📚 Learning: 2025-07-13T09:23:43.820Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 107
File: apps/api/src/routes/v1_files.ts:18-32
Timestamp: 2025-07-13T09:23:43.820Z
Learning: UNICODE_STABLE_VERSION from luxass/unicode-utils-new refers to a specific Unicode version string (not a dynamic value), and the validation logic in the files API correctly checks the normalized version (after "latest" substitution) against UNICODE_VERSION_METADATA rather than the mappedVersion from resolveUCDVersion().
Applied to files:
packages/shared/src/files.tspackages/shared/test/files.test.ts
📚 Learning: 2026-01-08T03:33:34.184Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T03:33:34.184Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Use test file pattern: **/*.{test,spec}.?(c|m)[jt]s?(x)
Applied to files:
packages/shared/test/filter.test.ts
📚 Learning: 2025-06-28T08:01:22.596Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 85
File: packages/fetch/package.json:37-38
Timestamp: 2025-06-28T08:01:22.596Z
Learning: In the ucdjs/ucd project, relative paths in npm scripts within packages (like `../../apps/api/.generated/openapi.json` in packages/fetch/package.json) resolve correctly even when run via Turborepo from the repo root, contrary to initial concerns about working directory changes.
Applied to files:
packages/shared/test/filter.test.ts
🧬 Code graph analysis (4)
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.ts (1)
tooling/moonbeam/src/esm-loader.mjs (1)
entries(44-44)
packages/shared/src/files.ts (2)
packages/shared/src/index.ts (2)
normalizePathForFiltering(8-8)normalizeTreeForFiltering(8-8)tooling/moonbeam/src/esm-loader.mjs (1)
entries(44-44)
packages/shared/test/filter.test.ts (4)
packages/shared/src/filter.ts (1)
filterTreeStructure(229-235)packages/shared/src/index.ts (1)
filterTreeStructure(13-13)packages/utils/src/index.ts (1)
filterTreeStructure(1-1)packages/ucd-store/src/store.ts (1)
tree(165-167)
packages/shared/test/files.test.ts (1)
packages/shared/src/files.ts (2)
normalizePathForFiltering(26-42)normalizeTreeForFiltering(63-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: typecheck
- GitHub Check: lint
- GitHub Check: Agent
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (11)
packages/test-utils/test/fs-bridge/memory-fs-bridge.test.ts (1)
136-241: LGTM! Path normalization applied consistently.The test expectations have been correctly updated to use absolute paths with leading slashes for all entries and trailing slashes for directories. This aligns with the PR's path normalization objectives.
packages/shared/test/filter.test.ts (2)
38-40: Good coverage for leading-slash path handling.The new test cases verify that filters correctly handle paths with leading slashes, ensuring compatibility with the normalized path format.
684-750: LGTM! Test expectations updated for normalized paths.All path values in the tree structure assertions now use absolute paths with leading slashes, consistent with the path normalization approach introduced in this PR.
packages/shared/src/index.ts (1)
8-8: LGTM! Public API extended with normalization utilities.The new exports (
normalizePathForFilteringandnormalizeTreeForFiltering) appropriately extend the public API surface to expose the path normalization functionality introduced in this PR.packages/test-utils/src/mock-store/handlers/files.ts (1)
144-159: No evidence found of the described changes.The review comment references removal of trailing newline appending and an
X-Content-Warningheader, but neither exist in the codebase. A comprehensive search found no references toX-Content-Warninganywhere in the repository, and the current implementation at lines 144-159 simply returns content without modification. The test suite (files.test.ts) contains no checks for these headers or trailing newline behavior. This appears to be an incorrect or outdated comment that does not reflect the actual code changes.Likely an incorrect or invalid review comment.
packages/shared/src/filter.ts (2)
144-168: LGTM! Well-designed path normalization helpers.The
normalizeForMatchingandnormalizePatternsfunctions provide clean, consistent path normalization for cross-platform glob matching. The logic correctly handles:
- Platform-specific separators (backslash → forward slash)
- Relative path prefixes (
./)- Leading slashes (for relative pattern matching)
- Trailing slashes (for directory patterns)
Applying the same normalization to both patterns and input paths ensures matching behavior is consistent regardless of path format.
182-196: Consistent application of normalization throughout the filter pipeline.The normalization is correctly applied at all key points:
- Include and exclude patterns are normalized before use
- Input paths are normalized before matching
- Directory pattern expansion operates on normalized patterns
This ensures the picomatch engine receives consistently formatted inputs.
packages/shared/src/files.ts (2)
44-83: Clean recursive tree normalization implementation.The
normalizeTreeForFilteringfunction correctly:
- Recursively processes all nodes in the tree
- Preserves the structure (including children for directories)
- Applies path normalization consistently
The implementation handles both files and directories appropriately, and the documentation clearly explains the purpose and usage.
5-42: Clean implementation of path normalization with excellent test coverage.The
normalizePathForFilteringfunction correctly handles version-relative path normalization across different Unicode versions. Tests confirm thathasUCDFolderPathproperly determines which versions include the "ucd/" folder in their structure—verified with versions 1.1.0 (no "ucd/" folder), 16.0.0, and 17.0.0 (both with "ucd/" folders). The function handles all edge cases: leading/trailing slashes, version prefixes, conditional "ucd/" stripping, and already-normalized paths. The recursivenormalizeTreeForFilteringimplementation preserves tree structure while normalizing all paths, making it suitable for filter pattern matching.packages/test-utils/src/fs-bridges/memory-fs-bridge.ts (2)
16-24: Simple and effective path formatting helper.The
formatEntryPathfunction clearly documents and implements FSEntry schema requirements:
- Leading slash for all paths (parity with node/http bridges)
- Trailing slash for directories
The implementation is straightforward and uses well-tested utilities from
@luxass/utils/path.
209-293: Consistent application of path formatting across all entry points.The
formatEntryPathhelper is correctly applied in all locations where FSEntry objects are created:
- Non-recursive directory entries (lines 209, 261)
- Recursive nested directories (lines 230, 293)
- File entries (lines 251, 281)
This ensures all paths returned from the memory FS bridge conform to the FSEntry schema consistently.
There was a problem hiding this comment.
Pull request overview
This pull request implements general improvements focused on path normalization consistency across the codebase. The changes standardize path formatting in file system operations to ensure leading slashes for all paths and trailing slashes for directories, bringing the memory-fs-bridge implementation in line with node/http bridges.
Changes:
- Standardized path formatting in memory-fs-bridge to use leading/trailing slashes consistently
- Added path normalization utilities for filtering operations to handle version-prefixed paths
- Enhanced path filter to handle absolute paths (with leading slashes) matching relative glob patterns
- Removed unnecessary newline addition logic from mock store file handler
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test-utils/src/fs-bridges/memory-fs-bridge.ts | Added formatEntryPath helper to ensure consistent path formatting with leading/trailing slashes |
| packages/test-utils/test/fs-bridge/memory-fs-bridge.test.ts | Updated test expectations to reflect new path format with leading slashes and trailing slashes for directories |
| packages/test-utils/src/mock-store/handlers/files.ts | Removed automatic newline addition logic and related warning header |
| packages/shared/src/filter.ts | Added normalizeForMatching and normalizePatterns to strip leading slashes and normalize paths for pattern matching |
| packages/shared/src/files.ts | Added normalizePathForFiltering and normalizeTreeForFiltering utilities to strip version/UCD prefixes from paths |
| packages/shared/test/filter.test.ts | Added test cases for leading slash path handling |
| packages/shared/test/files.test.ts | Added comprehensive tests for new normalization functions |
| packages/shared/src/index.ts | Exported new normalization utility functions |
| packages/cli/src/cmd/lockfile/info.ts | Removed debug console.error statement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🔗 Linked issue
📚 Description
This PR implements some general improvements. Mostly around filtering since we have made some changes to paths recently.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.