Conversation
🦋 Changeset detectedLatest commit: d712bd3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@luxass has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 46 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughThis change unifies the Unicode Character Database store implementation by replacing the separate Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Factory
participant UCDStore
participant FileSystemBridge
participant HTTPClient
CLI->>Factory: createLocalUCDStore(options)
Factory->>UCDStore: new UCDStore({ mode: "local", ...options })
UCDStore->>FileSystemBridge: initializeLocalStore()
FileSystemBridge-->>UCDStore: Filesystem operations
UCDStore-->>CLI: Ready store instance
CLI->>Factory: createRemoteUCDStore(options)
Factory->>UCDStore: new UCDStore({ mode: "remote", ...options })
UCDStore->>HTTPClient: initializeRemoteStore()
HTTPClient-->>UCDStore: Fetch versions/metadata
UCDStore-->>CLI: Ready store instance
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
* Replaced `LocalUCDStore` and `RemoteUCDStore` with a single `UCDStore` class. * Updated playgrounds to use `NodeFileSystemBridge` and `HTTPFileSystemBridge`. * Removed unused types and imports related to legacy store implementations. * Adjusted tests to reflect the new unified store structure. * Cleaned up package.json to remove references to deleted types.
…ore initialization * Refactored `UCDStore` to use `DEFAULT_BASE_URL` and `DEFAULT_PROXY_URL` constants. * Updated constructor to accept options for `baseUrl` and `proxyUrl`. * Added new factory functions `createLocalUCDStore` and `createRemoteUCDStore` for better store initialization. * Removed legacy test files for local and remote UCD store implementations.
…ote store creation functions * Updated `clean`, `init`, `repair`, and `status` commands to use `createLocalUCDStore` and `createRemoteUCDStore`. * Improved clarity and specificity in store creation logic. * Adjusted `AnalyzeResult` and `CleanResult` types for better structure and clarity.
0ab3887 to
4c82460
Compare
* Changed `FilterFn` to `PathFilter` for better clarity. * Updated `getFileTree`, `getFile`, `getFilePaths`, and `processFileStructure` methods to accept `extraFilters` for improved filtering capabilities.
There was a problem hiding this comment.
Pull Request Overview
This PR merges the LocalUCDStore and RemoteUCDStore into a single UCDStore class driven by a mode option, replaces the custom memfs implementation with a new FileSystemBridge API, and updates all references and tests accordingly.
- Replace
FSAdapterandmemfswithFileSystemBridgeand dynamic imports from@ucdjs/utils/fs-bridge - Refactor validate/mirror/internal logic to use the new bridge, removing
ensureDirand adding explicitexistschecks - Delete legacy store classes and tests, introduce
createLocalUCDStore/createRemoteUCDStore, and update CLI/playgrounds
Reviewed Changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tooling/tsconfig/base.json | Updated path aliases to point at fs-bridge modules |
| packages/utils/tsdown.config.ts | Adjusted documentation entries for source files |
| packages/utils/src/ucd-files/validate.ts | Switched to FileSystemBridge, dynamic import, and replaced ensureDir logic |
| packages/utils/src/ucd-files/internal.ts | Updated internal mirror logic to use FileSystemBridge |
| packages/utils/src/memfs.ts & src/types.ts | Removed legacy in-memory FS adapter and shared types |
| packages/utils/test/memfs.test.ts | Removed legacy memfs tests |
| packages/ucd-store/src/{local,remote,store}.ts | Deleted old store implementations, exported new constructors |
| packages/cli/src/cmd/store/*.ts | Updated CLI commands to use createLocalUCDStore/createRemoteUCDStore |
Comments suppressed due to low confidence (4)
packages/utils/tsdown.config.ts:9
- [nitpick] The documentation entry includes only
fs-bridge.ts; consider adding entries for./src/fs-bridge/node.tsand./src/fs-bridge/http.tsso those modules are covered in generated docs.
"./src/fs-bridge.ts",
packages/utils/src/ucd-files/validate.ts:223
- Removing the
{ recursive: true }option may prevent creating nested directories. Consider adding{ recursive: true }to ensure deep paths are created or retain a directory-existence check.
await fs.mkdir(versionOutputDir);
packages/utils/src/ucd-files/internal.ts:26
- This
mkdircall no longer includes the recursive option, which can fail for nestedversionOutputDir. Add{ recursive: true }or verify parent directories exist.
await fs.mkdir(versionOutputDir);
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
packages/ucd-store/src/store.ts (1)
269-271: Fix filter mutation in processFileStructureThis method has the same filter mutation issue as
getFile(). The extra filters should not permanently modify the instance's filter.Apply a similar fix here to avoid side effects across method calls.
🧹 Nitpick comments (1)
packages/ucd-store/playgrounds/local-playground.ts (1)
10-15: Consider using the new factory function for consistency.The store creation uses
createUCDStoredirectly, which works but is inconsistent with the CLI commands that use the specific factory functionscreateLocalUCDStore. For consistency with the broader refactor pattern, consider:-const store = await createUCDStore({ - mode: "local", +const store = await createLocalUCDStore({ basePath, versions: ["15.0.0", "16.0.0", "17.0.0"], fs: NodeFileSystemBridge, });This would also eliminate the need to explicitly specify the mode and align with the factory function pattern used elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
packages/cli/src/cmd/store/clean.ts(2 hunks)packages/cli/src/cmd/store/init.ts(2 hunks)packages/cli/src/cmd/store/repair.ts(2 hunks)packages/cli/src/cmd/store/status.ts(3 hunks)packages/ucd-store/playgrounds/local-playground.ts(1 hunks)packages/ucd-store/playgrounds/remote-playground.ts(1 hunks)packages/ucd-store/src/index.ts(1 hunks)packages/ucd-store/src/local.ts(0 hunks)packages/ucd-store/src/remote.ts(0 hunks)packages/ucd-store/src/store.ts(2 hunks)packages/ucd-store/test/local.test.ts(0 hunks)packages/ucd-store/test/remote.test.ts(0 hunks)packages/ucd-store/test/store.test.ts(1 hunks)packages/utils/package.json(0 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/memfs.ts(0 hunks)packages/utils/src/types.ts(0 hunks)packages/utils/src/ucd-files/internal.ts(5 hunks)packages/utils/src/ucd-files/mirror.ts(3 hunks)packages/utils/src/ucd-files/validate.ts(6 hunks)packages/utils/test/memfs.test.ts(0 hunks)packages/utils/test/ucd-files/mirror.test.ts(4 hunks)packages/utils/test/ucd-files/validate.test.ts(3 hunks)packages/utils/tsdown.config.ts(0 hunks)tooling/tsconfig/base.json(1 hunks)
💤 Files with no reviewable changes (9)
- packages/utils/tsdown.config.ts
- packages/utils/package.json
- packages/ucd-store/test/local.test.ts
- packages/utils/test/memfs.test.ts
- packages/utils/src/memfs.ts
- packages/ucd-store/test/remote.test.ts
- packages/ucd-store/src/remote.ts
- packages/utils/src/types.ts
- packages/ucd-store/src/local.ts
🧰 Additional context used
🧬 Code Graph Analysis (10)
packages/utils/test/ucd-files/validate.test.ts (1)
packages/utils/src/fs-bridge.ts (1)
FileSystemBridge(19-94)
packages/cli/src/cmd/store/init.ts (2)
packages/ucd-store/src/index.ts (2)
createRemoteUCDStore(3-3)createLocalUCDStore(2-2)packages/ucd-store/src/store.ts (2)
createRemoteUCDStore(355-367)createLocalUCDStore(337-349)
packages/cli/src/cmd/store/clean.ts (2)
packages/ucd-store/src/index.ts (2)
createRemoteUCDStore(3-3)createLocalUCDStore(2-2)packages/ucd-store/src/store.ts (2)
createRemoteUCDStore(355-367)createLocalUCDStore(337-349)
packages/utils/test/ucd-files/mirror.test.ts (1)
packages/utils/src/fs-bridge.ts (1)
FileSystemBridge(19-94)
packages/utils/src/ucd-files/mirror.ts (1)
packages/utils/src/fs-bridge.ts (1)
FileSystemBridge(19-94)
packages/utils/src/ucd-files/internal.ts (1)
packages/utils/src/fs-bridge.ts (1)
FileSystemBridge(19-94)
packages/ucd-store/playgrounds/local-playground.ts (1)
packages/ucd-store/src/store.ts (2)
createUCDStore(318-320)UCDStore(83-310)
packages/cli/src/cmd/store/status.ts (2)
packages/ucd-store/src/index.ts (2)
createRemoteUCDStore(3-3)createLocalUCDStore(2-2)packages/ucd-store/src/store.ts (2)
createRemoteUCDStore(355-367)createLocalUCDStore(337-349)
packages/utils/src/ucd-files/validate.ts (1)
packages/utils/src/fs-bridge.ts (1)
FileSystemBridge(19-94)
packages/ucd-store/test/store.test.ts (3)
packages/utils/src/fs-bridge.ts (1)
FileSystemBridge(19-94)packages/ucd-store/src/index.ts (2)
createLocalUCDStore(2-2)createUCDStore(4-4)packages/ucd-store/src/store.ts (4)
createLocalUCDStore(337-349)DEFAULT_BASE_URL(80-80)DEFAULT_PROXY_URL(81-81)createUCDStore(318-320)
🪛 GitHub Check: codecov/patch
packages/cli/src/cmd/store/init.ts
[warning] 3-3: packages/cli/src/cmd/store/init.ts#L3
Added line #L3 was not covered by tests
[warning] 50-50: packages/cli/src/cmd/store/init.ts#L50
Added line #L50 was not covered by tests
[warning] 53-53: packages/cli/src/cmd/store/init.ts#L53
Added line #L53 was not covered by tests
[warning] 56-56: packages/cli/src/cmd/store/init.ts#L56
Added line #L56 was not covered by tests
[warning] 60-60: packages/cli/src/cmd/store/init.ts#L60
Added line #L60 was not covered by tests
packages/cli/src/cmd/store/clean.ts
[warning] 4-4: packages/cli/src/cmd/store/clean.ts#L4
Added line #L4 was not covered by tests
[warning] 42-42: packages/cli/src/cmd/store/clean.ts#L42
Added line #L42 was not covered by tests
[warning] 45-45: packages/cli/src/cmd/store/clean.ts#L45
Added line #L45 was not covered by tests
[warning] 48-48: packages/cli/src/cmd/store/clean.ts#L48
Added line #L48 was not covered by tests
[warning] 52-52: packages/cli/src/cmd/store/clean.ts#L52
Added line #L52 was not covered by tests
packages/cli/src/cmd/store/status.ts
[warning] 4-4: packages/cli/src/cmd/store/status.ts#L4
Added line #L4 was not covered by tests
[warning] 42-42: packages/cli/src/cmd/store/status.ts#L42
Added line #L42 was not covered by tests
[warning] 45-45: packages/cli/src/cmd/store/status.ts#L45
Added line #L45 was not covered by tests
[warning] 48-48: packages/cli/src/cmd/store/status.ts#L48
Added line #L48 was not covered by tests
[warning] 52-52: packages/cli/src/cmd/store/status.ts#L52
Added line #L52 was not covered by tests
[warning] 77-77: packages/cli/src/cmd/store/status.ts#L77
Added line #L77 was not covered by tests
packages/cli/src/cmd/store/repair.ts
[warning] 3-3: packages/cli/src/cmd/store/repair.ts#L3
Added line #L3 was not covered by tests
[warning] 43-43: packages/cli/src/cmd/store/repair.ts#L43
Added line #L43 was not covered by tests
[warning] 46-46: packages/cli/src/cmd/store/repair.ts#L46
Added line #L46 was not covered by tests
[warning] 49-49: packages/cli/src/cmd/store/repair.ts#L49
Added line #L49 was not covered by tests
[warning] 53-53: packages/cli/src/cmd/store/repair.ts#L53
Added line #L53 was not covered by tests
packages/ucd-store/src/store.ts
[warning] 117-122: packages/ucd-store/src/store.ts#L117-L122
Added lines #L117 - L122 were not covered by tests
[warning] 125-127: packages/ucd-store/src/store.ts#L125-L127
Added lines #L125 - L127 were not covered by tests
[warning] 129-130: packages/ucd-store/src/store.ts#L129-L130
Added lines #L129 - L130 were not covered by tests
[warning] 132-132: packages/ucd-store/src/store.ts#L132
Added line #L132 was not covered by tests
[warning] 134-135: packages/ucd-store/src/store.ts#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 137-142: packages/ucd-store/src/store.ts#L137-L142
Added lines #L137 - L142 were not covered by tests
[warning] 146-147: packages/ucd-store/src/store.ts#L146-L147
Added lines #L146 - L147 were not covered by tests
[warning] 150-150: packages/ucd-store/src/store.ts#L150
Added line #L150 was not covered by tests
[warning] 152-160: packages/ucd-store/src/store.ts#L152-L160
Added lines #L152 - L160 were not covered by tests
🔇 Additional comments (29)
packages/utils/test/ucd-files/validate.test.ts (3)
1-1: LGTM: Correct interface migration.The import change from
FSAdaptertoFileSystemBridgealigns with the filesystem abstraction refactoring.
249-262: LGTM: Mock object correctly implements FileSystemBridge interface.The mock filesystem object properly satisfies the
FileSystemBridgeinterface with all required methods (read, write, listdir, mkdir, stat, exists, rm).
330-343: LGTM: Consistent interface implementation.The second mock object also correctly implements the
FileSystemBridgeinterface, maintaining consistency with the first mock.packages/cli/src/cmd/store/clean.ts (2)
4-4: LGTM: Correct import update for new factory functions.The import change aligns with the store unification refactoring, replacing the generic
createUCDStorewith specific factory functions.
41-54: LGTM: Store creation logic properly updated.The conditional logic correctly uses
createRemoteUCDStorevscreateLocalUCDStorebased on the remote flag, and the option key has been properly renamed toglobalFilters.packages/utils/src/index.ts (1)
19-19: To ensure we catch any remaining usages ofFilterFn, let’s rerun the search usinggrep(which supports excluding directories out of the box):#!/bin/bash echo "Searching for any references to FilterFn in .ts files…" grep -R "FilterFn" -n --include="*.ts" --exclude-dir=node_modules --exclude-dir=dist . echo "Searching specifically for imports of FilterFn…" grep -R "import.*FilterFn" -n --include="*.ts" --exclude-dir=node_modules --exclude-dir=dist .tooling/tsconfig/base.json (1)
12-14: LGTM: Path mappings support new filesystem bridge structure.The new path mappings correctly support the filesystem bridge abstraction with separate mappings for the base module, node implementation, and HTTP implementation.
packages/cli/src/cmd/store/repair.ts (2)
3-3: LGTM: Consistent import update across CLI commands.The import change matches the pattern used in other CLI commands and correctly adopts the new factory functions.
41-55: LGTM: Store creation logic consistent with other CLI commands.The conditional store creation logic and option key renaming (
globalFilters) is identical to the clean command, maintaining consistency across the CLI interface.packages/cli/src/cmd/store/status.ts (3)
4-4: LGTM! Import updated for new factory functions.The import correctly replaces the generic
createUCDStorewith the new mode-specific factory functionscreateLocalUCDStoreandcreateRemoteUCDStore.
42-46: Store creation logic correctly updated for new API.The conditional store creation properly uses the new factory functions based on the
remoteflag, and the option name has been correctly updated fromfilterstoglobalFiltersto match the new API.Also applies to: 48-53
77-77: Good fix! Removed incorrect "bytes" suffix.The console output for "Is Complete" correctly removes the erroneous "bytes" suffix since this field is a boolean value.
packages/cli/src/cmd/store/init.ts (2)
3-3: LGTM! Import updated consistently with other CLI commands.The import change aligns with the new factory function approach used across all CLI store commands.
50-54: Store creation correctly uses new factory functions.The conditional logic properly calls the appropriate factory function based on the
remoteflag, and the option rename fromfilterstoglobalFiltersis consistent with the new API.Also applies to: 56-61
packages/utils/src/ucd-files/mirror.ts (3)
1-1: LGTM! Updated to use new filesystem bridge abstraction.The import correctly changes from the old
FSAdapterto the newFileSystemBridgeinterface.
24-24: Type correctly updated to match new filesystem abstraction.The
MirrorOptionsinterface properly updates thefsproperty type to useFileSystemBridge.
81-83: Good improvement! Async import with proper error handling.The change from synchronous
createFileSystem({ type: "node" })to asynchronous dynamic import is well-implemented with a descriptive error message for import failures.packages/utils/test/ucd-files/mirror.test.ts (3)
1-1: LGTM! Import updated to use external filesystem bridge package.The import correctly changes to use
FileSystemBridgefrom the@ucdjs/utils/fs-bridgepackage instead of the local types.
197-197: Mock filesystem objects correctly updated for new interface.The type annotations properly update from
FSAdaptertoFileSystemBridge, and the mock implementations align with the new interface requirements.Also applies to: 326-326
212-212: Test expectation correctly updated for simplified mkdir API.The removal of the
{ recursive: true }option from themkdirexpectation aligns with the simplifiedFileSystemBridge.mkdirmethod signature that no longer accepts options.packages/ucd-store/playgrounds/local-playground.ts (2)
5-6: LGTM! Imports updated for unified store approach.The imports correctly bring in the
NodeFileSystemBridgeand the unifiedUCDStoretype and factory function.
17-18: Assertions correctly updated for unified store.The type assertion properly changes to
UCDStoreand the new mode assertion ensures the store is configured correctly.packages/ucd-store/playgrounds/remote-playground.ts (2)
4-10: LGTM! Clean migration to unified store API.The refactoring to use
createUCDStorewith explicit mode configuration aligns well with the PR objectives of consolidating the store implementations.
21-22: ```shell
#!/bin/bashShow the synchronous hasVersion implementation in store.ts
echo "🔍 hasVersion implementation in packages/ucd-store/src/store.ts"
rg -n "hasVersion" -C5 packages/ucd-store/src/store.tsDisplay the test file to see how hasVersion is used and asserted
echo -e "\n🔍 Content of packages/ucd-store/test/store.test.ts"
sed -n '1,200p' packages/ucd-store/test/store.test.ts</details> <details> <summary>packages/utils/src/ucd-files/internal.ts (1)</summary> `26-26`: ```shell #!/bin/bash # Display the import of fs and context around mkdir in internal.ts echo "=== Import and fs identifier ===" head -n 30 packages/utils/src/ucd-files/internal.ts echo -e "\n=== mkdir usage around line 26 ===" sed -n '20,40p' packages/utils/src/ucd-files/internal.tspackages/ucd-store/src/index.ts (1)
1-14: Well-structured API exports for the unified store.The reorganization of exports provides a clean API surface with:
- Dedicated factory functions for each store mode
- Consolidated type exports
- Clear separation of concerns
This aligns perfectly with the PR objectives.
packages/utils/src/ucd-files/validate.ts (2)
70-72: Good error handling for dynamic filesystem imports.The dynamic import pattern with explicit error handling ensures clear failure messages if the filesystem bridge cannot be loaded.
Also applies to: 194-196
223-223: ```bash
#!/bin/bashDisplay the top of validate.ts to locate basePath declaration and function signature
sed -n '1,100p' packages/utils/src/ucd-files/validate.ts
</details> <details> <summary>packages/ucd-store/src/store.ts (1)</summary> `355-367`: **Clarify why remote store needs FileSystemBridge** The `createRemoteUCDStore` function requires a `FileSystemBridge`, which is counterintuitive since remote operations typically use HTTP, not filesystem operations. Based on the code at line 246, it appears the remote mode uses the filesystem bridge for caching (`this.#fs.read()`). If this is intentional, consider: 1. Renaming to clarify its caching purpose 2. Using a more appropriate default like an HTTP-based bridge for remote mode 3. Adding documentation to explain this requirement </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
* Removed `fs-extra` from dependencies in `package.json`, `pnpm-lock.yaml`, and `pnpm-workspace.yaml`. * Updated comments in `mirror.ts` and `validate.ts` to reflect the change to a NodeJS bridge for file operations. * Ensured that file existence checks in `internal.ts` and `validate.ts` are awaited for proper asynchronous handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/heavy-camels-sink.md (1)
7-7: Grammar nit: “Since we using” → “Since we are using”-Merge LocalUCDStore & RemoteUCDStore into a single UCDStore class which handles everything. Since we using the new fs-bridge exposed from `@ucdjs/utils` we can easily do this. +Merge LocalUCDStore & RemoteUCDStore into a single UCDStore class that handles everything. Since we are using the new fs-bridge exposed by `@ucdjs/utils`, we can easily do this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.changeset/heavy-camels-sink.md(1 hunks)packages/ucd-store/package.json(0 hunks)packages/utils/src/ucd-files/internal.ts(5 hunks)packages/utils/src/ucd-files/mirror.ts(3 hunks)packages/utils/src/ucd-files/validate.ts(6 hunks)pnpm-workspace.yaml(0 hunks)
💤 Files with no reviewable changes (2)
- pnpm-workspace.yaml
- packages/ucd-store/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/utils/src/ucd-files/internal.ts
- packages/utils/src/ucd-files/mirror.ts
- packages/utils/src/ucd-files/validate.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/heavy-camels-sink.md
[grammar] ~7-~7: An auxiliary verb seems to be missing from this progressive structure. Did you mean “we're using”, “we are using”, or “we were using”?
Context: ...e class which handles everything. Since we using the new fs-bridge exposed from `@ucdjs/...
(PRP_VBG)
* Combined filter logic to streamline the application of extra filters. * Updated `analyze` method to throw an error indicating it is not implemented yet.

This PR is merging the LocalUCDStore and RemoteUCDStore into a single UCDStore class. The
createUCDStorefunction is also changed to now take in some options, which is where you define the mode.It uses the newly added filesystem bridge under the hood.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores