refactor(ucd-store): make use of result type for operations#209
refactor(ucd-store): make use of result type for operations#209
Conversation
* Updated the type name for clarity and consistency. * Adjusted the corresponding usage in the UCDStore class.
…ror type * Refactored error classes to extend from a new abstract class `BaseUCDStoreError`. * Introduced a `StoreError` type to standardize error messages and types. * Updated `UCDStore` class to return structured results in the `mirror` method. * Adjusted tests to reflect changes in error handling and class references.
🦋 Changeset detectedLatest commit: c554d2a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughStore operations were changed to return an error-first tuple ([data, error]) and errors were refactored into a hierarchical, serializable model with UCDStoreBaseError and typed payloads; internal flows (mirror, clean, analyze) were refactored for staged concurrency, per-file error handling, and directory pre-creation; CLI/playgrounds/tests were updated to the new return shapes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI / Caller
participant Store as UCDStore
participant Op as Internal Operation
User->>CLI: invoke analyze|mirror|clean|repair
CLI->>Store: call method(options)
Store->>Op: perform operation
Op-->>Store: result or throw
alt Internal returns [data, error] with error != null
Store-->>CLI: [null, StoreErrorInstance]
else Success (error == null)
Store-->>CLI: [dataArray, null]
end
CLI-->>User: render data or errors
sequenceDiagram
participant Store as UCDStore.mirror()
participant Validate as Validate versions & file lists
participant Plan as Build directory plan
participant FS as Filesystem
participant Queue as Files queue (N workers)
participant Remote as Remote API
Store->>Validate: fetch and validate per-version file lists
Validate-->>Store: file paths or throw
Store->>Plan: compute directoriesToCreate
Plan-->>FS: mkdirp all directories
FS-->>Store: done
Store->>Queue: enqueue [version,filePath] tasks
loop workers (N)
Queue->>Remote: GET file
alt Content-Type = application/json
Remote-->>Queue: raw text body
Queue->>FS: write text (preserve raw JSON)
else text/*
Remote-->>Queue: text body
Queue->>FS: write text
else
Remote-->>Queue: arrayBuffer
Queue->>FS: write binary
end
alt fetch/write fails
Queue-->>Store: record failed for version
else
Queue-->>Store: record mirrored/skipped
end
end
Store-->>Caller: [per-version results array, error|null]
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
pkg: ucd-store
…definitions * Removed the `StoreError` type and integrated its structure into individual error classes. * Updated error classes to extend `UCDStoreBaseError` for consistency. * Enhanced the `~toStoreError` method to return more detailed error information. * Adjusted tests to reflect changes in error class structure and ensure proper inheritance.
* Simplified error handling in `internal__mirror` by removing error message logging. * Updated content retrieval in `internal__mirrorFile` to directly use raw text response for non-JSON content types. * Enhanced test coverage for concurrency checks and error handling scenarios.
Preview Deployment for WebThe Web worker has been deployed successfully. Preview URL: https://preview.ucdjs.dev This preview was built from commit 6985daa 🤖 This comment will be updated automatically when you push new commits to this PR. |
Preview Deployment for ApiThe Api worker has been deployed successfully. Preview URL: https://preview.api.ucdjs.dev This preview was built from commit 6985daa 🤖 This comment will be updated automatically when you push new commits to this PR. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
* Changed return type of `clean` method to `StoreOperationResult<CleanResult[]>`. * Implemented structured error handling for uninitialized store and operational errors. * Updated tests to reflect changes in the return structure and assertions.
* Updated `analyze` method to return a `StoreOperationResult` type. * Improved error handling by returning detailed error messages. * Adjusted tests to validate the new response structure. * Ensured concurrency checks are properly handled in the clean operation.
* Added detailed error logging for failed store analysis. * Ensured that errors are properly displayed to the user. * Maintained structured output for successful analysis results.
* Update the store analysis to return a structured result. * Improve assertions for analysis success and data presence. * Log detailed results for both initial and subsequent analyses.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (21)
packages/ucd-store/src/internal/mirror.ts (2)
60-76: Pre-creation of directories is good; minor nit on capability assertion.The staged pre-creation of directories looks solid and will reduce redundant I/O. Tiny nit: you already assert capabilities at Line 55; reasserting inside the loop isn’t needed.
Apply this diff to avoid redundant checks inside the loop:
- await Promise.all([...directoriesToCreate].map(async (dir) => { - assertCapability(store.fs, ["mkdir", "exists"]); - if (!await store.fs.exists(dir)) { - await store.fs.mkdir(dir); - } - })); + await Promise.all([...directoriesToCreate].map(async (dir) => { + if (!(await store.fs.exists(dir))) { + await store.fs.mkdir(dir); + } + }));
103-117: Minor: simplify Promise mapping and remove unnecessary async wrapper.Mapping with async only to return limit(async () => ...) adds an extra microtask. You can return the limiter call directly.
Apply this diff (already included in the earlier refactor, but valid independently):
- await Promise.all(filesQueue.map(async ([version, filePath]) => { - return limit(async () => { - // ... - }); - })); + await Promise.all( + filesQueue.map(([version, filePath]) => + limit(async () => { + // ... + }), + ), + );packages/ucd-store/src/errors.ts (3)
1-1: ESLint rule name likely incorrect; consider using @typescript-eslint/explicit-function-return-type.The disabled rule id looks non-standard (“ts/explicit-function-return-type”). If the intent is to silence @typescript-eslint’s rule, use the canonical id.
-/* eslint-disable ts/explicit-function-return-type */ +/* eslint-disable @typescript-eslint/explicit-function-return-type */
3-5: Avoid ‘any’ in the abstract serializer; prefer ‘unknown’.Returning any broadens the type surface. ‘unknown’ preserves safety while still allowing narrowing at call sites.
-export abstract class UCDStoreBaseError extends Error { - abstract "~toStoreError"(): any; -} +export abstract class UCDStoreBaseError extends Error { + abstract "~toStoreError"(): unknown; +}
10-14: Optional: ensure correct prototype with Error subclassing across environments.Depending on transpilation targets, instanceof checks on custom Errors can fail without setting the prototype. Consider adding Object.setPrototypeOf(this, new.target.prototype) in constructors.
Example (apply similarly to other error classes):
constructor(message: string, data?: Record<string, unknown>) { super(message); this.name = "UCDStoreError"; this.data = data; Object.setPrototypeOf(this, new.target.prototype); }Also applies to: 29-34, 49-53, 69-81, 97-101, 113-116
packages/ucd-store/src/internal/clean.ts (1)
55-58: Surface analyze errors for better diagnostics.You throw a generic failure, but the wrapper includes structured errors. Include them to aid callers and logs.
- if (!analysisResult.success) { - throw new UCDStoreError("Failed to analyze store"); - } + if (!analysisResult.success) { + throw new UCDStoreError("Failed to analyze store", { errors: analysisResult.errors }); + }packages/ucd-store/src/types.ts (2)
75-95: StoreOperationResult shape is solid; consider readonly errors for immutability.Small API nudge: return readonly arrays to discourage mutation from consumers.
export type StoreOperationResult<TData = unknown> = { /** * Whether the operation completed successfully. */ success: false; /** * The result data from the operation (when successful). */ data?: TData; /** * Array of errors that occurred during the operation. */ - errors: StoreError[]; + errors: readonly StoreError[]; } | { success: true; data: TData; - errors: never[]; + errors: readonly never[]; };
61-74: Preserve backward compatibility for InitOptions renameNo internal references to
StoreInitOptionswere found. To avoid breaking external consumers who may have imported the old name, add a compatibility alias:• packages/ucd-store/src/types.ts (after the
InitOptionsdefinition):export interface InitOptions { /** * Whether to force overwrite existing store manifest and directories. * When true, existing manifest will be recreated even if it already exists. */ force?: boolean; /** * Whether to perform a dry run without actually creating files or directories. * This is useful for testing and debugging the initialization process. */ dryRun?: boolean; } +// Back-compat: preserve the old name +export type StoreInitOptions = InitOptions;packages/cli/src/cmd/store/analyze.ts (1)
97-99: Print expectedFileCount even when it’s 0.The current truthy check skips logging when expectedFileCount is 0. Use a nullish check to include zero.
- if (expectedFileCount) { + if (expectedFileCount != null) { console.info(` Total files expected: ${expectedFileCount}`); }packages/ucd-store/test/analyze.test.ts (1)
552-553: Clarify the inline comment about fileCount semantics.The assertion is fine; the comment is misleading. It reads as if only 2 files are present, yet expects fileCount 3. Clarify that fileCount includes orphaned files.
- expect(analysisResult.fileCount).toBe(3); // only 2 files present in the store (2 expected, 1 orphaned) + expect(analysisResult.fileCount).toBe(3); // counts present files (2 expected present + 1 orphaned)packages/ucd-store/test/mirror.test.ts (2)
331-336: Fix contradictory assertion message.The second assert checks that data is present, but the message says “Expected no versions to be mirrored.” Update the message to reflect the expectation.
- assert(mirrorResult.data != null, "Expected no versions to be mirrored"); + assert(mirrorResult.data != null, "Expected per-version results to be present despite failures");
170-179: Consider unifying the error-shape across operations (undefined vs [] for data).In mirror tests, error cases assert data is undefined, while analyze and repair use data: [] for some failures. This asymmetry forces extra branching in consumers.
If feasible, standardize on data: [] for failure cases where no per-version results are produced. For partial failures (e.g., per-version failures but global success=false), keeping data with per-version entries makes sense.
Also applies to: 192-201, 215-224, 248-257, 361-368
packages/ucd-store/test/repair.test.ts (1)
346-353: Align NOT_INITIALIZED error shape with other operations.Here, repair expects data: [] on NOT_INITIALIZED, whereas mirror expects data to be undefined. Consider converging on a single convention to simplify clients.
packages/ucd-store/test/errors.test.ts (1)
113-173: Consider adding toStoreError payload assertions for bridge unsupported operationYou validate the message formatting well. To harden against regressions, also assert the "~toStoreError" payload structure (type, operation, requiredCapabilities, availableCapabilities).
Example test you could add near this suite:
it("should convert to store error payload for bridge unsupported operation", () => { const operation = "unicode-normalization"; const requiredCapabilities = ["nfc", "nfd", "nfkc", "nfkd"]; const availableCapabilities = ["nfc", "nfd"]; const error = new UCDStoreBridgeUnsupportedOperation(operation, requiredCapabilities, availableCapabilities); const payload = error["~toStoreError"](); expect(payload).toEqual({ type: "BRIDGE_UNSUPPORTED_OPERATION", operation, message: `Operation "unicode-normalization" is not supported. Required capabilities: nfc, nfd, nfkc, nfkd. Available capabilities: nfc, nfd`, requiredCapabilities, availableCapabilities, }); });packages/ucd-store/src/store.ts (5)
326-334: De-duplicate NOT_INITIALIZED error construction via error serializerYou repeat the NOT_INITIALIZED message literal across operations. Prefer constructing UCDStoreNotInitializedError and using its "~toStoreError" to avoid message drift and ensure consistency. Also, mirror’s not-initialized branch omits data: [], unlike analyze/clean/repair.
Apply these diffs:
@@ - return { - success: false, - data: [], - errors: [{ - message: "Store is not initialized. Please initialize the store before performing operations.", - type: "NOT_INITIALIZED", - }], - }; + return { + success: false, + data: [], + errors: [new UCDStoreNotInitializedError()["~toStoreError"]()], + }; @@ - return { - success: false, - errors: [{ - message: "Store is not initialized. Please initialize the store before performing operations.", - type: "NOT_INITIALIZED", - }], - }; + return { + success: false, + data: [], + errors: [new UCDStoreNotInitializedError()["~toStoreError"]()], + }; @@ - return { - success: false, - data: [], - errors: [{ - message: "Store is not initialized. Please initialize the store before performing operations.", - type: "NOT_INITIALIZED", - }], - }; + return { + success: false, + data: [], + errors: [new UCDStoreNotInitializedError()["~toStoreError"]()], + }; @@ - return { - success: false, - data: [], - errors: [{ - message: "Store is not initialized. Please initialize the store before performing operations.", - type: "NOT_INITIALIZED", - }], - }; + return { + success: false, + data: [], + errors: [new UCDStoreNotInitializedError()["~toStoreError"]()], + };Also applies to: 398-405, 472-479, 533-540
418-453: Mirror: include data: [] in failure paths for API consistencyOther operations (analyze, clean, repair) include data: [] on failure. Mirror omits it in its failure branches, which is mildly inconsistent and can complicate consumers relying on uniform shapes.
Apply this diff:
@@ - return { - success: false, - errors: [ + return { + success: false, + data: [], + errors: [ { message: err instanceof Error ? err.message : String(err), type: "GENERIC", }, ], }; @@ - return { - success: false, - errors: [ + return { + success: false, + data: [], + errors: [ err["~toStoreError"](), ], };
317-323: Update @throws docs to reflect non-throwing wrapper semanticsanalyze now returns StoreOperationResult and does not throw. The @throws tags are now misleading.
Apply this diff to the JSDoc:
- * @returns {Promise<StoreOperationResult<AnalyzeResult[]>>} A promise that resolves to a StoreOperationResult containing an array of VersionAnalysis objects, one for each analyzed version - * - * @throws {UCDStoreVersionNotFoundError} When a specified version is not available in the store - * @throws {BridgeUnsupportedOperation} When the filesystem doesn't support required capabilities - * @throws {UCDStoreError} When other operational errors occur during analysis + * @returns {Promise<StoreOperationResult<AnalyzeResult[]>>} A promise that resolves to a StoreOperationResult containing results per version. Errors are returned via `result.errors`. + * + * Note: This method does not throw; all errors are surfaced via the `StoreOperationResult.errors` array.
389-395: Mirror JSDoc return type and throws are outdatedMirror returns StoreOperationResult<MirrorResult[]> and does not throw. Update the JSDoc for accuracy.
Apply this diff:
- * @returns {Promise<MirrorResult[]>} A promise that resolves to an array of MirrorResult objects, one for each mirrored version - * - * @throws {UCDStoreVersionNotFoundError} When a specified version is not available in the remote API - * @throws {BridgeUnsupportedOperation} When the filesystem doesn't support required capabilities (mkdir, write) - * @throws {UCDStoreError} When the concurrency parameter is less than 1 or other operational errors occur + * @returns {Promise<StoreOperationResult<MirrorResult[]>>} A promise that resolves to a StoreOperationResult with per-version mirror results. Errors are returned via `result.errors`. + * + * Note: This method does not throw; errors (including invalid concurrency, capability issues, and version checks) are surfaced in the `errors` array.
463-469: Clean JSDoc return type and throws are outdatedClean returns StoreOperationResult<CleanResult[]> and does not throw. Align the JSDoc accordingly.
Apply this diff:
- * @returns {Promise<CleanResult[]>} A promise that resolves to an array of CleanResult objects, one for each version cleaned - * - * @throws {UCDStoreVersionNotFoundError} When a specified version is not available in the store - * @throws {BridgeUnsupportedOperation} When the filesystem doesn't support required capabilities - * @throws {UCDStoreError} When the concurrency parameter is less than 1 or other operational errors occur + * @returns {Promise<StoreOperationResult<CleanResult[]>>} A promise that resolves to a StoreOperationResult with per-version clean results. Errors are returned via `result.errors`. + * + * Note: This method does not throw; errors (including invalid concurrency, capability issues, and version checks) are surfaced in the `errors` array.packages/ucd-store/test/clean.test.ts (2)
281-300: Rename test to reflect non-throwing API semanticsThe operation no longer throws on invalid concurrency; it returns a failure result. Rename for clarity.
Apply this diff:
- it("should throw if concurrency is less than 1", async () => { + it("should return failure when concurrency is less than 1", async () => {
270-279: Optional: assert uniform presence of data on failuresIn addition to checking success/data/errors here, consider adding a small helper assertion ensuring data is always present as an array for failure results too. This catches shape inconsistencies across operations (mirror currently omits data on failure).
If desired, I can add a tiny helper like:
function expectArrayData<T>(r: { data?: T[] }) { expect(Array.isArray(r.data)).toBe(true); }and use it in failure-path tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
packages/cli/src/cmd/store/analyze.ts(1 hunks)packages/ucd-store/playgrounds/node-playground.ts(1 hunks)packages/ucd-store/src/errors.ts(1 hunks)packages/ucd-store/src/index.ts(1 hunks)packages/ucd-store/src/internal/clean.ts(1 hunks)packages/ucd-store/src/internal/mirror.ts(2 hunks)packages/ucd-store/src/store.ts(10 hunks)packages/ucd-store/src/types.ts(3 hunks)packages/ucd-store/test/analyze.test.ts(13 hunks)packages/ucd-store/test/clean.test.ts(11 hunks)packages/ucd-store/test/errors.test.ts(5 hunks)packages/ucd-store/test/mirror.test.ts(6 hunks)packages/ucd-store/test/repair.test.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
PR: ucdjs/ucd#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/ucd-store/src/index.tspackages/ucd-store/src/internal/mirror.tspackages/ucd-store/test/repair.test.tspackages/ucd-store/test/clean.test.ts
📚 Learning: 2025-08-17T10:10:19.077Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/test/clean.test.ts:273-301
Timestamp: 2025-08-17T10:10:19.077Z
Learning: In UCD store clean tests, race conditions between analyze and delete phases are intentionally tested to verify robust handling of files that disappear during clean operations. The test pattern: start clean() → externally delete file during execution → verify file is marked as "skipped" rather than "failed".
Applied to files:
packages/ucd-store/src/internal/clean.tspackages/ucd-store/test/clean.test.ts
📚 Learning: 2025-08-17T10:06:17.991Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/src/internal/repair.ts:88-96
Timestamp: 2025-08-17T10:06:17.991Z
Learning: The internal__clean function in ucdjs/ucd-store can be safely called with versions: [] and a directories parameter to clean up only specific directories without processing any version files or modifying the manifest. When versions is empty, the analysis returns no results, skipping all file deletion logic, but the directory cleanup section still processes directories from the directories parameter.
Applied to files:
packages/ucd-store/src/internal/clean.tspackages/ucd-store/src/store.ts
📚 Learning: 2025-07-13T09:23:43.820Z
Learnt from: luxass
PR: ucdjs/ucd#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/ucd-store/test/repair.test.ts
🧬 Code Graph Analysis (7)
packages/ucd-store/src/internal/mirror.ts (4)
packages/ucd-store/src/store.ts (2)
versions(110-112)versions(592-601)packages/ucd-store/src/errors.ts (1)
UCDStoreVersionNotFoundError(46-62)packages/ucd-store/src/index.ts (1)
UCDStoreVersionNotFoundError(5-5)packages/ucd-store/src/internal/files.ts (1)
getExpectedFilePaths(20-38)
packages/ucd-store/src/internal/clean.ts (2)
packages/ucd-store/src/errors.ts (1)
UCDStoreError(7-23)packages/ucd-store/src/index.ts (2)
UCDStoreError(2-2)CleanResult(21-21)
packages/ucd-store/test/mirror.test.ts (2)
packages/ucd-store/src/index.ts (1)
createNodeUCDStore(10-10)tooling/test-utils/src/msw.ts (2)
mockFetch(50-76)HttpResponse(210-210)
packages/ucd-store/src/store.ts (5)
packages/ucd-store/src/types.ts (2)
InitOptions(61-73)StoreOperationResult(75-94)packages/ucd-store/src/internal/analyze.ts (3)
AnalyzeOptions(5-17)AnalyzeResult(19-60)internal__analyze(62-113)packages/ucd-store/src/internal/mirror.ts (3)
MirrorOptions(12-18)MirrorResult(20-41)internal__mirror(43-121)packages/ucd-store/src/internal/clean.ts (3)
CleanOptions(8-8)CleanResult(10-31)internal__clean(37-145)packages/ucd-store/src/internal/repair.ts (3)
RepairOptions(12-12)RepairResult(14-42)internal__repair(44-99)
packages/ucd-store/src/types.ts (1)
packages/ucd-store/src/errors.ts (1)
StoreError(133-139)
packages/ucd-store/test/errors.test.ts (2)
packages/ucd-store/src/errors.ts (4)
UCDStoreError(7-23)UCDStoreBridgeUnsupportedOperation(64-92)UCDStoreInvalidManifestError(94-110)UCDStoreNotInitializedError(112-124)packages/ucd-store/src/index.ts (2)
UCDStoreError(2-2)UCDStoreBridgeUnsupportedOperation(4-4)
packages/ucd-store/src/errors.ts (2)
packages/ucd-store/src/index.ts (4)
UCDStoreError(2-2)UCDStoreFileNotFoundError(3-3)UCDStoreVersionNotFoundError(5-5)UCDStoreBridgeUnsupportedOperation(4-4)packages/ucd-store/src/store.ts (1)
manifestPath(118-120)
🔇 Additional comments (14)
packages/ucd-store/src/internal/mirror.ts (1)
169-176: Good call: write JSON responses as raw text.Avoiding parse/stringify prevents failures on invalid JSON and unnecessary overhead. This aligns with the goal of mirroring byte-for-byte responses.
packages/ucd-store/src/index.ts (1)
4-4: Compat alias looks good.Re-exporting UCDStoreBridgeUnsupportedOperation as UCDStoreUnsupportedFeature maintains backward compatibility.
packages/ucd-store/src/internal/clean.ts (2)
45-49: Concurrency guard is appropriate.Throwing on concurrency < 1 prevents undefined limiter behavior.
67-67: Iteration over analysisResult.data matches the new wrapper shape.This aligns with StoreOperationResult from types.ts. Good adaptation.
packages/ucd-store/playgrounds/node-playground.ts (1)
75-88: Playground adapts cleanly to the new result wrapper.Good validations on success/data presence and straightforward extraction of the first analysis. Assertions read clearly and reflect the new API.
Also applies to: 94-105
packages/ucd-store/test/analyze.test.ts (1)
97-106: Tests correctly updated to assert the StoreOperationResult wrapper.Solid, consistent pattern: assert success, ensure data presence, then assert fields. Error-path expectations (UNSUPPORTED_VERSION, GENERIC) look precise and helpful.
Also applies to: 138-149, 174-179, 218-223, 249-257, 286-290, 320-327, 380-385, 400-403, 426-433, 477-483, 542-548
packages/ucd-store/test/repair.test.ts (1)
76-94: Repair tests correctly migrated to wrapper shape and cover key scenarios.Good coverage: missing/orphaned, mixed, multiple versions, intact store, dry-run, and version scoping. Assertions read clearly and match expected behaviors.
Also applies to: 123-145, 172-192, 213-221, 249-266, 283-294, 320-328
packages/ucd-store/test/errors.test.ts (4)
33-55: Great coverage of generic error serializationThe tests for UCDStoreError’s "~toStoreError" payloads (with and without data) are precise and align with the new serializable error model.
22-31: Inheritance assertions updated correctlyAsserting UCDStoreBaseError as an intermediate base for UCDStoreError, UCDStoreFileNotFoundError, and UCDStoreVersionNotFoundError looks correct and matches the refactor.
Also applies to: 58-71, 86-97
175-216: Invalid manifest error tests are spot-onConstructor behavior, message formatting, and "~toStoreError" payload (including manifestPath) are all validated correctly.
218-256: Not-initialized error tests look solid and consistentThe parameterless constructor, consistent message, and "~toStoreError" mapping to NOT_INITIALIZED are well covered.
packages/ucd-store/src/store.ts (1)
351-356: Analyze: wrapper result mapping looks goodSuccess path returns the correct wrapper shape with data and empty errors. Matches the new API design.
packages/ucd-store/test/clean.test.ts (2)
72-83: Nice adaptation to StoreOperationResult shapeThe test correctly switches to cleanResult.success and dereferences per-version data via cleanResult.data[0], matching the new public API.
316-328: Analyze() mocks aligned with wrapper contractMocked analyze() results use success/data/errors structure correctly; this keeps the clean() tests focused and deterministic.
Also applies to: 363-375
…unction * Removed unnecessary initialization of result array. * Utilized a Map to manage `MirrorResult` objects by version for improved clarity and performance. * Simplified return statement to directly return values from the Map.
* Clarified the behavior of `response.text()` regarding charset handling. * Highlighted potential issues with non-UTF-8 encoded JSON responses.
* Changed assertions in multiple test cases to expect `null` instead of `undefined` for `mirrorResult.data` when the mirror operation fails. * This improves clarity and consistency in error handling within the tests.
…defined * Changed assertions in the mirror tests to expect `undefined` instead of `null` for `mirrorResult.data`. * This aligns with the expected behavior when no versions are mirrored.
…ror function * Simplified the promise handling for file mirroring. * Improved error handling for concurrency checks. * Updated test descriptions for clarity on expected behavior.
* Replace early return with exception throwing for uninitialized store in `analyze`, `mirror`, `clean`, and `repair` methods. * This change enhances error handling consistency across methods and improves code readability.
* Eliminated unnecessary `data: []` fields in error responses for better clarity. * This change improves the consistency of error handling across the UCDStore class.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/ucd-store/test/mirror.test.ts (1)
168-179: LGTM: NOT_INITIALIZED failure path uses data: undefinedThis resolves previous null vs undefined inconsistencies noted in earlier reviews.
packages/ucd-store/test/clean.test.ts (1)
281-281: Acknowledged: Failure shape for invalid concurrency now expectsdatato be undefinedThis matches CI and the current implementation. Thanks for aligning the assertion.
Also applies to: 291-299
🧹 Nitpick comments (5)
packages/ucd-store/test/analyze.test.ts (2)
400-403: Nit: redundant empty-array assertionsYou check both deep equality with [] and length === 0. Prefer a single assertion using toHaveLength(0) for clarity and speed.
Apply this diff:
- expect(analysisResult.data).toEqual([]); - expect(analysisResult.data.length).toBe(0); + expect(analysisResult.data).toHaveLength(0);
477-483: Nit: variable naming can be less ambiguousUsing both analyzeResult and analysisResult in the same scope is easy to misread. Consider renaming the inner variable to versionAnalysis for clarity.
Apply this diff:
- const analyzeResult = await store.analyze({ checkOrphaned: true }); + const analyzeResult = await store.analyze({ checkOrphaned: true }); @@ - const analysisResult = analyzeResult.data[0]; + const versionAnalysis = analyzeResult.data[0]; @@ - expect(analysisResult.version).toBe("15.0.0"); - expect(analysisResult.isComplete).toBe(false); + expect(versionAnalysis.version).toBe("15.0.0"); + expect(versionAnalysis.isComplete).toBe(false); @@ - expect(analysisResult.fileCount).toBeLessThan(analysisResult.expectedFileCount); - expect(analysisResult.fileCount).toBe(2); // only 2 files present in the store - expect(analysisResult.expectedFileCount).toBe(3); + expect(versionAnalysis.fileCount).toBeLessThan(versionAnalysis.expectedFileCount); + expect(versionAnalysis.fileCount).toBe(2); // only 2 files present in the store + expect(versionAnalysis.expectedFileCount).toBe(3); @@ - expect(analysisResult.missingFiles).toEqual(["DerivedBidiClass.txt"]); - expect(analysisResult.orphanedFiles).toEqual([]); - expect(analysisResult.files).toEqual([ + expect(versionAnalysis.missingFiles).toEqual(["DerivedBidiClass.txt"]); + expect(versionAnalysis.orphanedFiles).toEqual([]); + expect(versionAnalysis.files).toEqual([packages/ucd-store/src/store.ts (2)
318-323: Update JSDoc: analyze no longer throws; errors are returned in the wrapperDoc still lists throws, but the implementation maps errors into StoreOperationResult and does not throw.
Apply this diff to align the documentation:
- * @returns {Promise<StoreOperationResult<AnalyzeResult[]>>} A promise that resolves to a StoreOperationResult containing an array of VersionAnalysis objects, one for each analyzed version - * - * @throws {UCDStoreVersionNotFoundError} When a specified version is not available in the store - * @throws {BridgeUnsupportedOperation} When the filesystem doesn't support required capabilities - * @throws {UCDStoreError} When other operational errors occur during analysis + * @returns {Promise<StoreOperationResult<AnalyzeResult[]>>} A StoreOperationResult containing an array of VersionAnalysis objects, one per analyzed version. + * + * Note: Errors such as NOT_INITIALIZED or UNSUPPORTED_VERSION are returned via `errors` instead of thrown.
324-368: DRY the catch blocks by centralizing error-to-wrapper mappinganalyze/mirror/clean/repair duplicate the exact error mapping. Extract to a private helper for maintainability and consistency.
Apply this diff to replace the catch body:
- } catch (err) { - if (!(err instanceof UCDStoreBaseError)) { - return { - success: false, - errors: [ - { - message: err instanceof Error ? err.message : String(err), - type: "GENERIC", - }, - ], - }; - } - - return { - success: false, - errors: [ - err["~toStoreError"](), - ], - }; - } + } catch (err) { + return this["~toFailure"](err); + }And add this helper method (outside the selected range):
private "~toFailure"<T = never>(err: unknown): StoreOperationResult<T> { if (!(err instanceof UCDStoreBaseError)) { return { success: false, errors: [ { message: err instanceof Error ? err.message : String(err), type: "GENERIC", }, ], }; } return { success: false, errors: [err["~toStoreError"]()], }; }packages/ucd-store/test/clean.test.ts (1)
363-390: Make the deletion failure deterministic (avoid relying on fs.rm semantics)Currently, the failure relies on calling
rmon a missing file while forcingexists()to return true. If the underlyingrmis configured withforce, the call may not reject, which can make the test flaky. Prefer explicitly mockingrmto fail for the target file.Insert a targeted rejection for only the problematic file (after the
existsspy, beforestore.clean()):vi.spyOn(store.fs, "exists").mockResolvedValue(true); +// Ensure only ArabicShaping.txt deletion fails; others succeed. +vi.spyOn(store.fs, "rm").mockImplementation(async (p: any) => { + if (String(p).endsWith("ArabicShaping.txt")) { + throw new Error("Simulated deletion failure"); + } + // Resolve for other files to keep "deleted" assertions passing without touching disk. + return; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/lemon-walls-hang.md(1 hunks)packages/ucd-store/src/store.ts(6 hunks)packages/ucd-store/test/analyze.test.ts(13 hunks)packages/ucd-store/test/clean.test.ts(11 hunks)packages/ucd-store/test/mirror.test.ts(6 hunks)packages/ucd-store/test/repair.test.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-13T09:23:43.820Z
Learnt from: luxass
PR: ucdjs/ucd#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/ucd-store/test/repair.test.ts
📚 Learning: 2025-08-17T10:06:17.991Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/src/internal/repair.ts:88-96
Timestamp: 2025-08-17T10:06:17.991Z
Learning: The internal__clean function in ucdjs/ucd-store can be safely called with versions: [] and a directories parameter to clean up only specific directories without processing any version files or modifying the manifest. When versions is empty, the analysis returns no results, skipping all file deletion logic, but the directory cleanup section still processes directories from the directories parameter.
Applied to files:
packages/ucd-store/src/store.ts
📚 Learning: 2025-08-17T10:10:19.077Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/test/clean.test.ts:273-301
Timestamp: 2025-08-17T10:10:19.077Z
Learning: In UCD store clean tests, race conditions between analyze and delete phases are intentionally tested to verify robust handling of files that disappear during clean operations. The test pattern: start clean() → externally delete file during execution → verify file is marked as "skipped" rather than "failed".
Applied to files:
packages/ucd-store/test/clean.test.ts
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
PR: ucdjs/ucd#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/ucd-store/test/clean.test.ts
🧬 Code Graph Analysis (3)
packages/ucd-store/test/repair.test.ts (1)
packages/ucd-store/src/index.ts (1)
createNodeUCDStore(10-10)
packages/ucd-store/src/store.ts (6)
packages/ucd-store/src/types.ts (2)
InitOptions(61-73)StoreOperationResult(75-94)packages/ucd-store/src/internal/analyze.ts (3)
AnalyzeOptions(5-17)AnalyzeResult(19-60)internal__analyze(62-113)packages/ucd-store/src/errors.ts (1)
UCDStoreNotInitializedError(112-124)packages/ucd-store/src/internal/mirror.ts (3)
MirrorOptions(11-17)MirrorResult(19-40)internal__mirror(42-113)packages/ucd-store/src/internal/clean.ts (3)
CleanOptions(8-8)CleanResult(10-31)internal__clean(37-145)packages/ucd-store/src/internal/repair.ts (3)
RepairOptions(12-12)RepairResult(14-42)internal__repair(44-99)
packages/ucd-store/test/mirror.test.ts (1)
tooling/test-utils/src/msw.ts (2)
mockFetch(50-76)HttpResponse(210-210)
🔇 Additional comments (40)
.changeset/lemon-walls-hang.md (2)
5-22: Changelog reads well; migration guidance is clear and actionableThe wrapper shape, migration steps, and new error taxonomy are communicated clearly. Calling out not-initialized and concurrency validation as wrapper failures is helpful for downstreams.
2-3: Double-check semver: API return type change is breakingSwitching core operations to return a result wrapper alters public method signatures. If
@ucdjs/ucd-storeis consumed externally, this warrants a major version bump rather than a minor.Would you like me to scan the repo for external/public usage (e.g., examples, CLI, playground, other packages) that rely on the old signatures to assess breakage?
packages/ucd-store/test/analyze.test.ts (10)
97-105: LGTM: validates wrapper success/data shape on happy pathGood pattern: assert success before dereferencing data; checks are tight and readable.
138-149: LGTM: orphaned-file scenario matches expected wrapper shapeAssertions correctly validate success, data presence, and per-version fields.
174-179: LGTM: avoids ordering assumptions by using find()Using find() to fetch specific version entries prevents order-dependent flakiness when multiple results are present.
218-223: LGTM: specific-version analysis path is correctly validatedThe test confirms both wrapper shape and that only the requested version is analyzed.
249-256: LGTM: failure shape uses data: undefined and structured errorAsserting data is undefined (not empty array) aligns with the new StoreOperationResult contract.
286-290: LGTM: API error path asserts GENERIC error typeType-only assertion keeps the test resilient to message formatting differences.
320-327: LGTM: remote store success validates wrapper and per-version fieldsConsistent pattern with local store tests; good coverage across bridges.
342-346: LGTM: empty-manifest case returns success with empty dataCorrect expectation for an initialized store with no versions.
426-433: LGTM: “no files” case retains success semanticsCorrectly expects completeness with fileCount 0 and no missing files for the version.
542-548: LGTM: combined missing+orphaned scenario is asserted comprehensivelyGood coverage of counts and lists.
packages/ucd-store/test/mirror.test.ts (9)
64-76: LGTM: wrapper assertions and per-version results are preciseValidates zero errors, success, and correct per-version counts for mirrored/skipped/failed; side-effect checks with existsSync round out the test well.
116-124: LGTM: dry-run path validates mirrored list without side effectsGood assert stack: success/data checks, counts, and file system state validation.
145-152: LGTM: force flag path correctly overwrites existing contentValidates successful mirroring and verifies new content via getFile().
181-201: LGTM: invalid concurrency returns GENERIC error with data undefinedMatches the StoreOperationResult failure contract and error mapping.
203-224: LGTM: unsupported version failure is validated with structured errorCorrectly asserts UNSUPPORTED_VERSION shape and message.
226-257: LGTM: API error mapping to GENERIC with detailed messageThe test matches the expected “Failed to fetch expected files …” message shape.
259-312: LGTM: content-type handling test covers JSON, text, and binaryGood coverage; callCount assertion ensures all branches executed.
314-341: LGTM: per-file failures stay within per-version result; top-level success remains trueAppropriate separation of per-item errors vs. top-level operation outcome.
343-368: LGTM: catastrophic error returns a single GENERIC error with no dataThe expectations align with wrapper semantics and error mapping.
packages/ucd-store/test/repair.test.ts (9)
76-94: LGTM: missing-files repair path validates restored/skipped/removedAsserts wrapper success and per-version result integrity; side-effect checks solidify behavior.
123-145: LGTM: orphan cleanup path validates removals and skipped correctlyGood granularity: checks both removed list and that expected files were preserved.
172-192: LGTM: combined repair case (restore + remove + skip) is comprehensiveBalanced coverage across restored/removed/skipped lists and success state.
213-221: LGTM: multi-version repair establishes both result entriesIndexes are safe here because repair preserves input-version ordering (via Promise.all and mapping).
249-266: LGTM: intact store produces all-skipped outcomeMatches expectations: success with empty restored/removed/failed and fully skipped.
283-299: LGTM: dry-run behavior identifies operations without applying side effectsVerifies wrapper success and ensures filesystem remains unchanged.
320-334: LGTM: specific-version repair isolates effects to the requested versionAccurate validation of selective operation scope.
344-353: LGTM: NOT_INITIALIZED failure uses data undefined and structured errorClear, deterministic assertion on error shape and message.
355-375: LGTM: invalid concurrency returns wrapper failure with GENERIC errorMessage is precise and consistent with other suites.
packages/ucd-store/src/store.ts (4)
387-435: LGTM: mirror wrapper behavior and options mapping are correct
- Validates initialization
- Applies defaults for versions/concurrency/dryRun/force
- Returns success wrapper with data on completion
452-499: LGTM: clean wrapper behavior is consistent with result-type contractError mapping aligns with other operations; options defaulting is correct.
501-547: LGTM: repair wrapper wiring and option defaults align with internal behaviorReturns wrapped results and standardizes error mapping; consistent with tests.
243-307: Init flow aligns with manifest/API logic and respects dryRun/force
- Validates API availability before proceeding
- Merges constructor and manifest versions
- Creates base path and writes manifest only when appropriate
packages/ucd-store/test/clean.test.ts (6)
72-83: LGTM: Clean wrapper usage and per-version assertions look correctGood check for success flag, data presence, and the deleted/skipped/failed breakdown.
113-129: LGTM: Orphaned files scenario validated wellAsserting that the orphaned local file is included in deleted alongside mirrored files is correct.
178-209: LGTM: Multi-version clean scenarioSolid assertions for both versions’ per-version results and deleted sets.
231-247: LGTM: Selective clean for a single version in a multi-version storeGood validation that only the targeted version is cleaned and the manifest is updated accordingly.
316-328: LGTM: Race-condition-safe test for disappearing file — correctly marked as skippedMocking analyze and then deleting the file before clean validates the “skip if already gone” behavior. This aligns with the retrieved learning about intentional race handling in clean tests.
Also applies to: 333-345
405-414: LGTM: Dry-run honors no-op on filesystem while reporting would-be deletionsThe snapshot comparison plus deleted list check is a good combination to verify behavior.
* Updated `assertCapability` to check for both `listdir` and `exists`. * Added existence check for the version directory before listing entries. * Modified test to ensure it correctly skips files not in the store and verifies file existence. * Enhanced test coverage for cleaning operations with additional files.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/ucd-store/test/clean.test.ts (4)
133-181: Nice: Race condition handling aligns with intended behavior (skipped vs. failed)Mocking analyze(), deleting the file before clean(), and asserting it lands in
skippedis exactly the robustness check we want for disappearing files.
317-325: Good fix: Failure shape for invalid concurrency now expectsdata: undefinedMatches the wrapper semantics for failure cases.
9-9: Don’t importassertfrom Vitest; use Node’s assert or switch toexpect()Vitest’s
assertis not a callable function. This will throw at runtime when used asassert(...). Replace the import and use Node’sok:-import { assert, beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { ok as assert } from "node:assert/strict";
294-305:it.todohas an implementation — convert toit.skipor make it a real testVitest disallows
it.todowith a callback body. Either enable it or keep it skipped.Apply minimal change (outside this hunk) at Line 279:
// before it.todo("should handle empty store", async () => { // after it.skip("should handle empty store", async () => {
🧹 Nitpick comments (5)
packages/ucd-store/src/store.ts (5)
131-134: Pre-check for version directory is good, but consider error semanticsThrowing
UCDStoreVersionNotFoundErrorwhen the directory is missing conflates “unknown version” with “known but not mirrored yet.” If you want to keep this UX, fine; otherwise, consider a distinct error (or message) to hint “mirror the version first.”
328-372: Avoid repeating the same try/catch result-wrapping logic
analyze,mirror,clean, andrepairall duplicate the same wrapper pattern. Extract a small helper to DRY this up.Example helper and usage (outside this hunk):
// helper async function wrapOperation<T>(fn: () => Promise<T>): Promise<StoreOperationResult<T>> { try { const data = await fn(); return { success: true, data, errors: [] as never[] }; } catch (err) { if (err instanceof UCDStoreBaseError) { return { success: false, errors: [err["~toStoreError"]()] }; } return { success: false, errors: [{ message: err instanceof Error ? err.message : String(err), type: "GENERIC" }], }; } } // then here return wrapOperation(async () => { if (!this.#initialized) throw new UCDStoreNotInitializedError(); let { checkOrphaned = false, versions = [] } = options; if (versions.length === 0) versions = this.#versions; return internal__analyze(this, { checkOrphaned, versions }); });
391-440: Update JSDoc to reflectStoreOperationResult<MirrorResult[]>The signature returns the wrapper type, but the JSDoc above still advertises
Promise<MirrorResult[]>. Align the docs to prevent consumer confusion.Apply this minimal change in the JSDoc above Line 391:
- * @returns {Promise<MirrorResult[]>} A promise that resolves to an array of MirrorResult objects, one for each mirrored version + * @returns {Promise<StoreOperationResult<MirrorResult[]>>} A promise resolving to a StoreOperationResult with per-version mirror details
456-503: Update JSDoc to reflectStoreOperationResult<CleanResult[]>Same doc mismatch as mirror: the method returns the wrapper now.
Suggested change in the JSDoc above Line 456:
- * @returns {Promise<CleanResult[]>} A promise that resolves to an array of CleanResult objects, one for each version cleaned + * @returns {Promise<StoreOperationResult<CleanResult[]>>>} StoreOperationResult with per-version clean results
505-551: Same duplication note asanalyze/mirror/clean— consider the wrapper helperReplicate the
wrapOperationhelper here to keep all four methods consistent and concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/ucd-store/src/store.ts(7 hunks)packages/ucd-store/test/clean.test.ts(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-17T10:10:19.077Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/test/clean.test.ts:273-301
Timestamp: 2025-08-17T10:10:19.077Z
Learning: In UCD store clean tests, race conditions between analyze and delete phases are intentionally tested to verify robust handling of files that disappear during clean operations. The test pattern: start clean() → externally delete file during execution → verify file is marked as "skipped" rather than "failed".
Applied to files:
packages/ucd-store/test/clean.test.ts
📚 Learning: 2025-08-17T10:06:17.991Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/src/internal/repair.ts:88-96
Timestamp: 2025-08-17T10:06:17.991Z
Learning: The internal__clean function in ucdjs/ucd-store can be safely called with versions: [] and a directories parameter to clean up only specific directories without processing any version files or modifying the manifest. When versions is empty, the analysis returns no results, skipping all file deletion logic, but the directory cleanup section still processes directories from the directories parameter.
Applied to files:
packages/ucd-store/test/clean.test.tspackages/ucd-store/src/store.ts
🧬 Code Graph Analysis (1)
packages/ucd-store/src/store.ts (6)
packages/ucd-store/src/errors.ts (2)
UCDStoreVersionNotFoundError(46-62)UCDStoreNotInitializedError(112-124)packages/ucd-store/src/types.ts (2)
InitOptions(61-73)StoreOperationResult(75-94)packages/ucd-store/src/internal/analyze.ts (3)
AnalyzeOptions(5-17)AnalyzeResult(19-60)internal__analyze(62-113)packages/ucd-store/src/internal/mirror.ts (3)
MirrorOptions(11-17)MirrorResult(19-40)internal__mirror(42-113)packages/ucd-store/src/internal/clean.ts (3)
CleanOptions(8-8)CleanResult(10-31)internal__clean(37-145)packages/ucd-store/src/internal/repair.ts (3)
RepairOptions(12-12)RepairResult(14-42)internal__repair(44-99)
🔇 Additional comments (7)
packages/ucd-store/test/clean.test.ts (7)
72-86: LGTM: Clean happy-path checks are precise and comprehensiveGood coverage: validates success, result presence, per-version fields, and exact deletions.
113-129: LGTM: Orphaned file scenario is covered wellAsserts include the orphaned file and maintain tight expectations for deleted set and counts.
203-234: LGTM: Multi-version cleaning validatedSolid assertions for both versions, with explicit contents for the deleted arrays.
256-272: LGTM: Scoped cleaning preserves manifest for untouched versionsGood check that only the targeted version is cleaned and manifest reflects the remaining version(s).
342-354: LGTM: Non-existent file deletion path correctly ends inskippedThe spy + existence check makes the intent clear; assertions align with the wrapper model.
Also applies to: 359-371
389-401: LGTM: Deletion failure path exercisedStubbing
existsto true after removing the file is a pragmatic way to provoke a failure onrm. Assertions are on-point.Also applies to: 403-416
431-440: LGTM: Dry-run behavior validated without mutating the FSDeleted list reflects “would delete” semantics while snapshots confirm no writes occurred.
I will implement a new package called "@ucdjs/shared", which contains some overall utility functions that is shared across the codebase. E.g. Shared Result type & error handling
* Updated `getFileTree`, `getFilePaths`, and `analyze` methods to use `tryCatch` for error handling. * Changed return types to `OperationResult` for better clarity on success and error states. * Adjusted tests to reflect changes in return structure and ensure proper error assertions.
* Refactor the `store.analyze` method to return a tuple of `[analyses, error]` instead of a single result object. * Update assertions in tests to check for `error` and `analyses` instead of the previous structure. * Ensure all relevant tests are updated to reflect the new return type and maintain expected behavior.
…test assertions * Refactored `internal__clean` to destructure the result of `store.analyze` into `analyses` and `error`. * Updated error handling to throw the error directly if it exists. * Modified tests to reflect the new return structure of `store.clean`, ensuring assertions check for `cleanData` and `cleanError`. * Enhanced clarity and robustness of the cleaning process.
* Updated tests to destructure results from `store.mirror()` for clarity. * Improved assertions to check for errors and data presence more explicitly. * Ensured consistency in error handling across multiple test cases.
…ta and error * Refactored `repair` method to return a tuple of repair data and error. * Updated `getFileTree` method to return a tuple of file tree data and error. * Enhanced error handling in tests to assert expected outcomes.
* Refactored the `runAnalyzeStore` function to destructure results from `store.analyze`. * Enhanced error handling by checking for `analyzeError` instead of `result.success`. * Updated logging to provide clearer error messages and analysis results.
* Changed the way mirror results are accessed by using `find` to retrieve results for versions "15.0.0" and "16.0.0". * This improves clarity in the test assertions and ensures that the correct version data is being validated.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the UCD store operations to return standardized result types for improved error handling and API consistency.
- Wraps all core operations (analyze, mirror, clean, repair) in a
tryCatchhelper that returns[data, error]tuples - Introduces structured error types extending
UCDStoreBaseErrorwith~toStoreErrorserialization methods - Updates test suites to handle the new return format with assertion-based error checking
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ucd-store/src/store.ts | Refactors core operations to return OperationResult<T, StoreError> tuples instead of throwing |
| packages/ucd-store/src/errors.ts | Introduces UCDStoreBaseError hierarchy with structured error serialization |
| packages/ucd-store/src/internal/*.ts | Updates internal operations to handle new error flow and improve mirror file handling |
| packages/ucd-store/test/*.test.ts | Converts tests from exception-based to result tuple assertions |
| packages/cli/src/cmd/store/analyze.ts | Updates CLI to handle new analyze result format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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)
packages/ucd-store/src/internal/clean.ts (1)
114-130: Danger: directories are removed recursively without checking emptiness—can delete unexpected content.The comment says “clean up empty directories,” but the code deletes them unconditionally with
{ recursive: true }. If any files remain (e.g., user-added files not covered by analysis), they will be removed. Replace with listdir+emptiness check and includelistdirin capability assertion.- // clean up empty directories (bottom-up approach) - if (!dryRun) { - assertCapability(store.fs, ["exists", "rm"]); - // sort directories by depth (deepest first) for bottom-up cleanup - const sortedDirectories = Array.from(directoriesToCheck).sort((a, b) => b.split("/").length - a.split("/").length); - for (const dirPath of sortedDirectories) { - try { - const exists = await store.fs.exists(dirPath); - if (!exists) continue; - await store.fs.rm(dirPath, { recursive: true }); - } catch { - // silently ignore directory deletion failures - } - } - } + // clean up empty directories (bottom-up approach) + if (!dryRun) { + assertCapability(store.fs, ["exists", "listdir", "rm"]); + // sort directories by depth (deepest first) for bottom-up cleanup + const sortedDirectories = Array.from(directoriesToCheck).sort( + (a, b) => b.split("/").length - a.split("/").length, + ); + for (const dirPath of sortedDirectories) { + try { + const exists = await store.fs.exists(dirPath); + if (!exists) continue; + const children = await store.fs.listdir(dirPath); + if (children.length === 0) { + await store.fs.rm(dirPath); + } + } catch { + // silently ignore directory deletion failures + } + } + }Additionally, if
directoriescan be provided externally, validate each is withinstore.basePathbefore adding todirectoriesToCheckto prevent accidental out-of-scope deletions. I can provide a smallisSubpathhelper if you want.
♻️ Duplicate comments (3)
packages/cli/src/cmd/store/analyze.ts (1)
64-74: Ensure--jsonoutputs on failures too (print JSON before error/return).Same concern as earlier review: early return on error prevents JSON output when
--jsonis used. Emit a consistent JSON shape for both success and failure.- const [analyzeData, analyzeError] = await store.analyze({ + const [analyzeData, analyzeError] = await store.analyze({ checkOrphaned: !!checkOrphaned, versions: versions || [], }); - - if (analyzeError != null) { - console.error(red(`\n❌ Error analyzing store:`)); - console.error(` ${analyzeError.message}`); - return; - } - - if (json) { - console.info(JSON.stringify(analyzeData, null, 2)); - return; - } + if (json) { + const serialize = (err: unknown) => { + const anyErr = err as any; + if (anyErr && typeof anyErr["~toStoreError"] === "function") { + return anyErr["~toStoreError"](); + } + return err + ? { type: "GENERIC" as const, message: err instanceof Error ? err.message : String(err) } + : null; + }; + console.info(JSON.stringify({ data: analyzeData, error: serialize(analyzeError) }, null, 2)); + return; + } + if (analyzeError != null) { + console.error(red(`\n❌ Error analyzing store:`)); + console.error(` ${analyzeError.message}`); + return; + }Also applies to: 75-78
packages/ucd-store/test/clean.test.ts (1)
283-310:it.todomust not have a callback—convert toit.skip(or enable it).Vitest treats
it.todoas a placeholder without implementation. Since this block has a body, change it toit.skipto keep it disabled without breaking tooling, or toit(...)to enable it.- it.todo("should handle empty store", async () => { + it.skip("should handle empty store", async () => { // ... });packages/ucd-store/src/store.ts (1)
426-431: Bug: internal__clean called without directories (TypeError risk)internal__clean constructs new Set(directories) and will throw if directories is undefined. Pass an empty array.
const result = await internal__clean(this, { versions, concurrency, dryRun, + directories: [], });
🧹 Nitpick comments (15)
packages/ucd-store/src/internal/analyze.ts (3)
74-78: Good migration to error-first tuple; add a defensive non-null check foractualFiles.Pattern is consistent with the new API. To harden against accidental
[null, null]returns, add a quick guard so we don’t passnullintoSet(...).- const [actualFiles, error] = await store.getFilePaths(version); + const [actualFiles, error] = await store.getFilePaths(version); if (error != null) { throw error; } + // Defensive: ensure successful branch always has data + if (actualFiles == null) { + throw new Error("store.getFilePaths returned null data without an error"); + }
80-103: Deterministic ordering for outputs and simpler set ops.Current loops iterate
Sets, which are insertion-ordered but derived fromexpectedFiles/actualFilesin different places;files,missingFiles, andorphanedFilesordering may fluctuate. Prefer deriving from the original arrays to preserve deterministic order and simplify logic.- const orphanedFiles: string[] = []; - const missingFiles: string[] = []; - const files: string[] = []; - - const expectedSet = new Set(expectedFiles); - const actualSet = new Set(actualFiles); - for (const expectedFile of expectedSet) { - if (!actualSet.has(expectedFile)) { - missingFiles.push(expectedFile); - } - } - - for (const actualFile of actualSet) { - const isExpected = expectedSet.has(actualFile); - if (checkOrphaned && !isExpected) { - orphanedFiles.push(actualFile); - continue; - } - - if (isExpected) { - files.push(actualFile); - } - } + const expectedSet = new Set(expectedFiles); + const actualSet = new Set(actualFiles); + + // Preserve expected order for these two + const missingFiles = expectedFiles.filter(f => !actualSet.has(f)); + const files = expectedFiles.filter(f => actualSet.has(f)); + + // Preserve actual order for orphaned (only when requested) + const orphanedFiles = checkOrphaned ? actualFiles.filter(f => !expectedSet.has(f)) : [];
104-114:fileCountincludes orphaned files—double-check intent.
fileCountis set toactualFiles.lengthwhilefilesonly include expected ones. If you intended “files found that belong to the version,” consider usingfiles.lengthinstead. If you intended “all files present under the version directory,” current code is correct—document this to avoid confusion.packages/ucd-store/src/internal/clean.ts (3)
83-103: Minor: move capability assertion out of the hot path and drop!non-null assertions.
assertCapability(store.fs, ["exists", "rm"])is inside each limited task. Move a single assertion before the loop and removeversionResult!non-null assertions for clarity.- for (const analysis of analyses) { + assertCapability(store.fs, ["exists", "rm"]); + for (const analysis of analyses) { // initialize result for this version const versionResult: CleanResult = { version: analysis.version, deleted: [], skipped: [], failed: [], }; @@ - promises.push(limit(async () => { - assertCapability(store.fs, ["exists", "rm"]); + promises.push(limit(async () => { try { const exists = await store.fs.exists(filePath); if (!exists) { console.error("File does not exist, skipping deletion:", filePath); - versionResult!.skipped.push(file); + versionResult.skipped.push(file); return; } if (!dryRun) { await store.fs.rm(filePath); } - versionResult!.deleted.push(file); + versionResult.deleted.push(file); } catch { - versionResult!.failed.push(file); + versionResult.failed.push(file); } }));
89-92: Avoid console noise in library code for expected races—use debug logger or collect telemetry.Missing files are normal in races (per tests). Consider downgrading to a debug logger or removing the
console.errorto keep library output clean.- console.error("File does not exist, skipping deletion:", filePath); + // Optional: use a debug logger here if available + // debug("Skipping deletion; file does not exist: %s", filePath);
132-143: Defensive: handle absent manifest gracefully.If
~readManifest()ever returnsnull/undefined,Object.keys(manifest)throws. Default to an empty object.- const manifest = await store["~readManifest"](); + const manifest = (await store["~readManifest"]()) ?? {};packages/ucd-store/test/clean.test.ts (2)
135-183: Good use ofvi.spyOn(store, "analyze")to simulate orphan/race scenarios.This mirrors real-world behavior (files disappearing mid-clean). As a minor enhancement, add inline comments indicating which files are expected to be deleted vs. skipped to help future readers.
Also applies to: 343-354, 391-401
431-441: Dry-run expectation clarity.Asserting that
deletedlists planned deletions while the snapshot remains unchanged is a great pattern. Consider adding a short comment in the test explaining thatdeletedreflects “would delete” for readers skimming the test.packages/ucd-store/src/store.ts (1)
249-255: Broaden file-not-found handling for non-ENOENT environmentsLimiting not-found detection to ENOENT (Node) may miss other bridges (e.g., HTTP) that surface different errors. Prefer an existence check via fs.exists before read or normalize read errors at the bridge layer.
Example refactor:
- } catch (err) { - if (err instanceof Error && err.message.includes("ENOENT")) { - throw new UCDStoreError(`File '${filePath}' does not exist in version '${version}'.`); - } - throw err; - } + } catch (err) { + const fullPath = isAbsolute(filePath) ? filePath : join(this.basePath, version, filePath); + if (await this.#fs.exists?.(fullPath) === false) { + throw new UCDStoreError(`File '${filePath}' does not exist in version '${version}'.`); + } + throw err; + }packages/ucd-store/test/analyze.test.ts (4)
256-259: Use instance and property assertions for errors (avoid deep-equality on Error)Deep-equality against
new UCDStoreVersionNotFoundError(version)can be brittle (stack/cause differences). Assert type and relevant props instead.- expect(error).toEqual(new UCDStoreVersionNotFoundError(version)); + expect(error).toBeInstanceOf(UCDStoreVersionNotFoundError); + expect(error?.message).toBe(`Version '${version}' does not exist in the store.`); + // @ts-expect-error version is a custom field on the error type + expect(error?.version).toBe(version);
314-316: HEAD mocks should not return a bodyWhile MSW tolerates it, returning JSON from a HEAD handler is semantically incorrect and can hide issues. Return an empty 200 instead.
- ["HEAD", `${UCDJS_API_BASE_URL}/api/v1/files/15.0.0`, () => { - return HttpResponse.json(stripChildrenFromEntries(MOCK_FILES)); - }], + ["HEAD", `${UCDJS_API_BASE_URL}/api/v1/files/15.0.0`, () => { + return new Response(null, { status: 200 }); + }],
404-407: Remove redundant assertion
expect(analyses).toEqual([])already implies length 0; the subsequent length assertion is redundant.- expect(analyses).toEqual([]); - expect(analyses.length).toBe(0); + expect(analyses).toEqual([]);
556-556: Correct the inline comment to match the asserted value
fileCountis asserted to be 3 (2 expected + 1 orphaned). The comment says “only 2 files present,” which is misleading.- expect(analysisResult.fileCount).toBe(3); // only 2 files present in the store (2 expected, 1 orphaned) + expect(analysisResult.fileCount).toBe(3); // 3 files present in the store (2 expected + 1 orphaned)packages/ucd-store/test/repair.test.ts (1)
353-356: Strengthen error assertions with type checksWhere you validate failure paths, also assert the specific error type to guard against message-only false positives. Import
UCDStoreNotInitializedErrorand reuse the shared errors for clarity.+import { UCDStoreNotInitializedError } from "@ucdjs/ucd-store"; expect(repairData).toBe(null); assert(repairError != null, "Expected error to be present"); expect(repairError.message).toBe("Store is not initialized. Please initialize the store before performing operations."); + expect(repairError).toBeInstanceOf(UCDStoreNotInitializedError);expect(repairData).toBe(null); assert(repairError != null, "Expected error to be present"); expect(repairError.message).toBe("Concurrency must be at least 1"); + // Optionally: expect a typed error if you introduce one for invalid concurrencyAlso applies to: 369-372
packages/ucd-store/test/mirror.test.ts (1)
230-237: Prefer explicit API error type checks when availableYou already type the mocked error as
ApiError. Consider assertingmirrorErroris an instance of your store error type (e.g.,UCDStoreError) in addition to checking the message, for stronger guarantees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/cli/src/cmd/store/analyze.ts(1 hunks)packages/ucd-store/playgrounds/node-playground.ts(1 hunks)packages/ucd-store/src/internal/analyze.ts(1 hunks)packages/ucd-store/src/internal/clean.ts(1 hunks)packages/ucd-store/src/store.ts(5 hunks)packages/ucd-store/src/types.ts(1 hunks)packages/ucd-store/test/analyze.test.ts(14 hunks)packages/ucd-store/test/clean.test.ts(12 hunks)packages/ucd-store/test/file-operations.test.ts(20 hunks)packages/ucd-store/test/mirror.test.ts(6 hunks)packages/ucd-store/test/repair.test.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ucd-store/src/types.ts
- packages/ucd-store/playgrounds/node-playground.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
PR: ucdjs/ucd#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/ucd-store/test/file-operations.test.tspackages/ucd-store/src/internal/analyze.tspackages/ucd-store/test/analyze.test.tspackages/ucd-store/test/mirror.test.tspackages/ucd-store/test/clean.test.tspackages/ucd-store/src/store.ts
📚 Learning: 2025-08-17T10:06:18.009Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/src/internal/repair.ts:88-96
Timestamp: 2025-08-17T10:06:18.009Z
Learning: The internal__clean function in ucdjs/ucd-store can be safely called with versions: [] and a directories parameter to clean up only specific directories without processing any version files or modifying the manifest. When versions is empty, the analysis returns no results, skipping all file deletion logic, but the directory cleanup section still processes directories from the directories parameter.
Applied to files:
packages/ucd-store/src/internal/clean.tspackages/ucd-store/test/clean.test.tspackages/ucd-store/src/store.ts
📚 Learning: 2025-08-17T10:10:19.096Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/test/clean.test.ts:273-301
Timestamp: 2025-08-17T10:10:19.096Z
Learning: In UCD store clean tests, race conditions between analyze and delete phases are intentionally tested to verify robust handling of files that disappear during clean operations. The test pattern: start clean() → externally delete file during execution → verify file is marked as "skipped" rather than "failed".
Applied to files:
packages/ucd-store/src/internal/clean.tspackages/ucd-store/test/analyze.test.tspackages/ucd-store/test/clean.test.ts
📚 Learning: 2025-07-13T09:23:43.820Z
Learnt from: luxass
PR: ucdjs/ucd#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/ucd-store/test/repair.test.tspackages/ucd-store/test/analyze.test.ts
📚 Learning: 2025-08-23T05:20:36.929Z
Learnt from: luxass
PR: ucdjs/ucd#209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.929Z
Learning: In the ucdjs/ucd codebase using modern Vitest, the assert export from "vitest" works as a callable function assert(condition, message) and provides TypeScript type narrowing, making it the preferred choice over expect() for runtime assertions that need compile-time type safety. The existing usage pattern assert(condition, message) throughout the test suite is working correctly.
Applied to files:
packages/ucd-store/test/analyze.test.tspackages/ucd-store/test/clean.test.ts
📚 Learning: 2025-08-23T05:20:36.929Z
Learnt from: luxass
PR: ucdjs/ucd#209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.929Z
Learning: In the ucdjs/ucd codebase, Vitest's assert import works as a callable function assert(condition, message) and provides TypeScript type narrowing, making it the preferred choice over expect() for runtime assertions that also need compile-time type safety.
Applied to files:
packages/ucd-store/test/clean.test.ts
📚 Learning: 2025-08-23T05:16:26.833Z
Learnt from: luxass
PR: ucdjs/ucd#209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:16:26.833Z
Learning: In the ucdjs/ucd codebase, the team prefers using `assert(condition, message)` over `expect(condition).toBeTruthy()` because assert provides TypeScript type narrowing while expect doesn't. This is important for maintaining type safety after runtime validation checks.
Applied to files:
packages/ucd-store/test/clean.test.ts
🧬 Code graph analysis (5)
packages/ucd-store/src/internal/clean.ts (1)
packages/ucd-store/src/errors.ts (1)
UCDStoreError(7-23)
packages/ucd-store/test/repair.test.ts (1)
packages/ucd-store/src/index.ts (1)
createNodeUCDStore(10-10)
packages/ucd-store/test/analyze.test.ts (2)
packages/ucd-store/src/errors.ts (2)
UCDStoreVersionNotFoundError(46-62)UCDStoreError(7-23)packages/ucd-store/test/__shared.ts (1)
stripChildrenFromEntries(116-121)
packages/ucd-store/test/mirror.test.ts (1)
tooling/test-utils/src/msw.ts (2)
mockFetch(50-76)HttpResponse(210-210)
packages/ucd-store/src/store.ts (9)
packages/shared/src/try-catch.ts (2)
OperationResult(3-3)tryCatch(11-27)packages/fetch/src/components.ts (1)
UnicodeTreeNode(12-12)packages/ucd-store/src/errors.ts (4)
StoreError(133-139)UCDStoreNotInitializedError(112-124)UCDStoreVersionNotFoundError(46-62)UCDStoreError(7-23)packages/shared/src/index.ts (1)
flattenFilePaths(3-3)packages/ucd-store/src/types.ts (1)
InitOptions(59-71)packages/ucd-store/src/internal/analyze.ts (3)
AnalyzeOptions(5-17)AnalyzeResult(19-60)internal__analyze(62-117)packages/ucd-store/src/internal/mirror.ts (3)
MirrorOptions(11-17)MirrorResult(19-40)internal__mirror(42-113)packages/ucd-store/src/internal/clean.ts (3)
CleanOptions(8-8)CleanResult(10-31)internal__clean(37-145)packages/ucd-store/src/internal/repair.ts (3)
RepairOptions(12-12)RepairResult(14-42)internal__repair(44-99)
🔇 Additional comments (12)
packages/ucd-store/src/internal/clean.ts (2)
45-49: Concurrency guard LGTM.
Clear and user-facing error makes misconfiguration obvious.
50-58: Tuple-based analyze integration looks good.
Immediate throw onerroris appropriate internally; upstream wrapper can catch and convert to[null, err].If you expect
internal__cleanto be invoked withversions: [](directories-only mode per prior learnings),store.analyze({ versions: [] })returns[], which is handled correctly here.packages/cli/src/cmd/store/analyze.ts (1)
80-98: LGTM: switched iteration to the data array.
Adapts cleanly to the tuple API and preserves existing UX.packages/ucd-store/test/file-operations.test.ts (3)
7-7: Assert import LGTM (matches team’s narrowing preference).
Consistent with the project’s convention to leverage type narrowing viaassert(condition, message).
49-52: Nice, tests updated to the[data, error]tuple across file operations.
- Success paths assert
error === nulland data non-null.- Failure paths assert
data === nulland validate error types/messages.
This makes the API contract explicit and improves readability.Also applies to: 117-121, 139-141, 169-173, 212-218, 342-345, 362-364, 384-386, 424-426, 553-558, 589-594, 636-649, 685-689, 706-709, 726-730, 747-751
553-558: Capability tests are precise and valuable.Asserting
BridgeUnsupportedOperationon missinglistdirand verifying spy call ordering strengthens the contract. No changes requested.Also applies to: 589-594, 636-649
packages/ucd-store/test/clean.test.ts (2)
72-83: Broad test updates to tuple API look correct.
- Consistent
[cleanData, cleanError]destructuring and assertions- Multi-version indexing is clear (
cleanData[0],cleanData[1])- Dry-run semantics validated by snapshot
- Concurrency error shape validated as
[null, err]If flakiness appears due to parallelism, consider adding
vi.useFakeTimers()or reducing concurrency in tests that mutate the same filesystem state.Also applies to: 114-131, 173-183, 205-237, 259-277, 300-310, 322-327, 359-372, 403-416, 431-441
312-327: Concurrency < 1 failure shape aligned with implementation.
Asserting[null, error]and checking the exact message keeps the contract crisp. LGTM.packages/ucd-store/src/store.ts (1)
239-247: No change needed: FileSystemBridge already handles basePath resolution
After inspecting both the Node and HTTP bridge implementations, we confirmed that:
- In the Node bridge,
read(path)usesresolveSafePath(basePath, path), which automatically joins any relativepathagainst the configuredbasePath.- In the HTTP bridge,
read(path)usesjoinURL(baseUrl, path), which similarly prefixes the base URL to relative paths.Therefore, calling
this.#fs.read(join(version, filePath))correctly resolves tobasePath/version/filePath(orbaseUrl/version/filePathfor HTTP). Explicitly prependingthis.basePathin the read call is unnecessary and can be left as-is.packages/ucd-store/test/repair.test.ts (1)
76-96: Nice coverage of restored/removed/skipped breakdownAssertions comprehensively validate per-bucket outcomes and the final on-disk state. This is a solid pattern for the rest of the suite.
packages/ucd-store/test/mirror.test.ts (2)
90-103: Good fix: avoid order assumptions by locating results by version keySwitching from array destructuring to
findremoves nondeterministic flakiness under concurrency. Nicely done.
305-313: Robust handling of simulated write failuresGreat use of
assertCapability+vi.spyOn(..., "write")to force error paths and verify failure aggregation without failing the overall operation.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/ucd-store/src/internal/clean.ts (3)
86-101: TOCTOU: treat “file disappeared during deletion” as skipped, not failed.Between exists() and rm(), another process can remove the file. Current catch marks it failed, violating the intended “skip if gone” behavior tested in clean tests.
- try { + try { const exists = await store.fs.exists(filePath); if (!exists) { - console.error("File does not exist, skipping deletion:", filePath); - versionResult!.skipped.push(file); + console.warn("File does not exist, skipping deletion:", filePath); + versionResult.skipped.push(file); return; } if (!dryRun) { await store.fs.rm(filePath); } - versionResult!.deleted.push(file); - } catch { - versionResult!.failed.push(file); + versionResult.deleted.push(file); + } catch (err: unknown) { + // If the file is already gone, treat as skipped + const code = (err as any)?.code ?? (err as any)?.cause?.code; + if (code === "ENOENT") { + versionResult.skipped.push(file); + return; + } + versionResult.failed.push(file); }If fs-bridge exposes a specific NotFound error type, switch the code check to an
instanceoftype guard accordingly.
114-130: Dangerous: recursive directory deletion may remove non-target files. Only remove empty dirs.Comment says “bottom-up cleanup,” but
rm(dirPath, { recursive: true })will delete directories even if they still contain files (including unrelated ones). Check emptiness first and remove non-recursively.- if (!dryRun) { - assertCapability(store.fs, ["exists", "rm"]); - - // sort directories by depth (deepest first) for bottom-up cleanup - const sortedDirectories = Array.from(directoriesToCheck).sort((a, b) => b.split("/").length - a.split("/").length); - - for (const dirPath of sortedDirectories) { - try { - const exists = await store.fs.exists(dirPath); - if (!exists) continue; - - await store.fs.rm(dirPath, { recursive: true }); - } catch { - // silently ignore directory deletion failures - } - } - } + if (!dryRun) { + assertCapability(store.fs, ["exists", "rm", "listdir"]); + + // sort directories by depth (deepest first) for bottom-up cleanup + const sortedDirectories = Array.from(directoriesToCheck).sort((a, b) => b.split("/").length - a.split("/").length); + + for (const dirPath of sortedDirectories) { + try { + const exists = await store.fs.exists(dirPath); + if (!exists) continue; + const entries = await store.fs.listdir(dirPath); + if (entries.length === 0) { + await store.fs.rm(dirPath); + } + } catch { + // silently ignore directory deletion failures + } + } + }
133-143: Possible null manifest; coalesce to avoid Object.keys(null).If
~readManifest()returns null/undefined,Object.keys(manifest)will throw.- const manifest = await store["~readManifest"](); + const manifest = (await store["~readManifest"]()) ?? {};packages/ucd-store/test/analyze.test.ts (1)
567-582: Invalid usage:it.todomust not have a callback — convert toit.skipVitest disallows providing an implementation to it.todo. Keep the bodies intact by skipping these tests for now.
-it.todo("should analyze store with partially missing directory structures", async () => { +it.skip("should analyze store with partially missing directory structures", async () => { // Test case where directory exists but some files within it are missing // E.g., "extracted/" directory exists but "DerivedBidiClass.txt" inside is missing }); -it.todo("should handle file system errors during analysis", async () => { +it.skip("should handle file system errors during analysis", async () => { // Test case where filesystem operations fail during analysis // E.g., permission errors, corrupted files, etc. // Should return empty array or handle gracefully }); -it.todo("should analyze empty version directory", async () => { +it.skip("should analyze empty version directory", async () => { // Test case where version directory exists but is completely empty // Different from "no files" test - this has an empty directory vs no directory });
♻️ Duplicate comments (3)
packages/cli/src/cmd/store/analyze.ts (1)
69-74: Ensure --json emits output on failures too (print JSON before error-return).This is the same control-flow issue flagged previously: returning early on error prevents
--jsonconsumers from receiving machine-readable output.- if (analyzeError != null) { - console.error(red(`\n❌ Error analyzing store:`)); - console.error(` ${analyzeError.message}`); - return; - } - - if (json) { - console.info(JSON.stringify(analyzeData, null, 2)); - return; - } + if (json) { + console.info(JSON.stringify({ + data: analyzeData ?? null, + error: analyzeError ? { name: analyzeError.name, message: analyzeError.message } : null, + }, null, 2)); + return; + } + if (analyzeError != null) { + console.error(red(`\n❌ Error analyzing store:`)); + console.error(` ${analyzeError.message}`); + return; + }Also applies to: 75-77
packages/ucd-store/test/clean.test.ts (1)
283-311: Invalid usage:it.todowith a callback — convert toit.skipTo keep the existing body without running it, mark the test as skipped.
- it.todo("should handle empty store", async () => { + it.skip("should handle empty store", async () => { const storePath = await testdir(); mockFetch([ ["GET", `${UCDJS_API_BASE_URL}/api/v1/versions/:version/file-tree`, () => { return HttpResponse.json([]); }], ]); const store = await createNodeUCDStore({ basePath: storePath, versions: ["15.0.0"], }); await store.init(); await store.mirror(); const [cleanData, cleanError] = await store.clean(); assert(cleanError === null, "Expected clean to succeed"); assert(cleanData != null, "Expected clean data to be non-null"); assert(cleanData[0] != null, "Expected first clean result to be non-null"); expect(cleanData[0].version).toBe("15.0.0"); expect(cleanData[0].skipped).toEqual([]); expect(cleanData[0].failed).toEqual([]); expect(cleanData[0].deleted).toHaveLength(0); - }); + });packages/ucd-store/src/store.ts (1)
410-434: Bug: internal__clean called without directories; can throw TypeError at runtimeThis persists from a prior review. internal__clean does new Set(directories). If directories is undefined, it throws “TypeError: undefined is not iterable”. Pass an empty array when you don’t target specific directories.
Apply this diff:
- const result = await internal__clean(this, { - versions, - concurrency, - dryRun, - }); + const result = await internal__clean(this, { + versions, + concurrency, + dryRun, + directories: [], + });
🧹 Nitpick comments (18)
packages/ucd-store/src/types.ts (1)
59-71: Add backward-compat alias and mark fields readonly
File:packages/ucd-store/src/types.ts(Lines 59–71)I ran a code‐wide search for
StoreInitOptionsand found no internal references, so introducing the alias will not conflict. Marking theforceanddryRunproperties asreadonlyclearly signals that these options shouldn’t be mutated.Suggested diff:
--- a/packages/ucd-store/src/types.ts +++ b/packages/ucd-store/src/types.ts @@ -59,7 +59,7 @@ export interface InitOptions { /** * Whether to force overwrite existing store manifest and directories. * When true, existing manifest will be recreated even if it already exists. - force?: boolean; + readonly force?: boolean; /** * Whether to perform a dry run without actually creating files or directories. * This is useful for testing and debugging the initialization process. - dryRun?: boolean; + readonly dryRun?: boolean; }Add immediately after the interface to preserve compatibility:
// Back-compat alias (deprecated; remove in next major) export type StoreInitOptions = InitOptions;packages/ucd-store/src/internal/clean.ts (3)
45-48: Strengthen the concurrency guard (handle NaN and non-integers).Using
< 1misses NaN and may pass floats to p-limit. Validate more defensively.- if (concurrency < 1) { + if (!Number.isFinite(concurrency) || concurrency < 1) { throw new UCDStoreError("Concurrency must be at least 1"); }
60-61: Avoid passing possibly-undefined to Set constructor.While most runtimes tolerate
new Set(undefined), coalesce for clarity.- const directoriesToCheck = new Set<string>(directories); + const directoriesToCheck = new Set<string>(directories ?? []);
64-66: Hoist capability assertions out of the hot path.Asserting on every file is unnecessary overhead; do it once.
Add near the limit creation:
// create the limit function to control concurrency const limit = pLimit(concurrency); + assertCapability(store.fs, ["exists", "rm"]);Then remove inside the task:
- promises.push(limit(async () => { - assertCapability(store.fs, ["exists", "rm"]); + promises.push(limit(async () => {Also applies to: 83-85
packages/cli/src/cmd/store/analyze.ts (1)
95-97: Log expectedFileCount even when zero.Guard suppresses legitimate zero. Check for undefined instead.
- if (expectedFileCount) { + if (typeof expectedFileCount === "number") { console.info(` Total files expected: ${expectedFileCount}`); }packages/ucd-store/playgrounds/node-playground.ts (2)
75-85: Rename local vars for clarity (analyses vs analyze).Use plural for arrays to avoid confusion with the method name and to improve readability.
-const [analyze, error] = await store.analyze({ +const [analyses, error] = await store.analyze({ versions: ["15.1.0"], checkOrphaned: true, }); -assert(analyze != null, "store analysis should succeed"); +assert(analyses != null, "store analysis should succeed"); assert(error == null, "store analysis should not return an error"); -assert(analyze[0], "analysis should contain data"); +assert(analyses[0], "analysis should contain data"); -const [analysis] = analyze; +const [analysis] = analyses;
95-107: Typo: newAnalyzes → newAnalyses for pluralization consistency.Minor naming fix for readability.
-const [newAnalyzes, newError] = await store.analyze({ +const [newAnalyses, newError] = await store.analyze({ versions: ["15.1.0"], checkOrphaned: true, }); -assert(newAnalyzes != null, "new store analysis should succeed"); +assert(newAnalyses != null, "new store analysis should succeed"); assert(newError == null, "new store analysis should not return an error"); -assert(newAnalyzes[0], "new analysis should contain data"); +assert(newAnalyses[0], "new analysis should contain data"); -const [newAnalysis] = newAnalyzes; +const [newAnalysis] = newAnalyses;packages/ucd-store/test/mirror.test.ts (2)
71-74: Optional: fetch per-version result by key for consistencyEven though only one version is requested here, mirroring the order-agnostic pattern improves consistency across the suite and protects against future changes that might introduce multiple versions in this test.
- expect(mirrorData[0].failed).toHaveLength(0); - expect(mirrorData[0].mirrored).toHaveLength(3); - expect(mirrorData[0].skipped).toHaveLength(0); - expect(mirrorData[0].version).toBe("15.0.0"); + const v15 = mirrorData.find((r) => r.version === "15.0.0"); + assert(v15 != null, "Expected mirror result for version 15.0.0"); + expect(v15.failed).toHaveLength(0); + expect(v15.mirrored).toHaveLength(3); + expect(v15.skipped).toHaveLength(0); + expect(v15.version).toBe("15.0.0");
239-293: Strengthen content-type coverage with explicit path assertionsYou already validate callCount and overall success. As a small robustness boost, assert the mirrored paths to ensure each content-type branch yields the expected filenames.
assert(mirrorData[0] != null, "Expected at least one version to be mirrored"); - expect(mirrorData[0].mirrored).toHaveLength(3); + expect(mirrorData[0].mirrored).toHaveLength(3); + expect(mirrorData[0].mirrored).toEqual(expect.arrayContaining([ + "ArabicShaping.txt", + "BidiBrackets.txt", + "extracted/DerivedBidiClass.txt", + ])); expect(mirrorData[0].skipped).toHaveLength(0); expect(mirrorData[0].version).toBe("15.0.0");packages/ucd-store/test/file-operations.test.ts (3)
99-121: Rename: test title says “throw” but code asserts error-return tupleThe API returns [data, error]; it doesn’t throw. Rename to reflect the contract for clarity in failures and reports.
-it("should throw error for invalid version", async () => { +it("should return error for invalid version", async () => {
529-558: Rename: capability test titles should reflect error-return semanticsThese don’t throw; they return a BridgeUnsupportedOperation error in the tuple.
-it("should throw BridgeUnsupportedOperation when getFileTree is called without listdir capability", async () => { +it("should return BridgeUnsupportedOperation error when getFileTree is called without listdir capability", async () => {
564-599: Rename: align second capability test title with tuple-based APIKeep test names consistent with the non-throwing contract.
-it("should throw BridgeUnsupportedOperation when getFilePaths is called without listdir capability", async () => { +it("should return BridgeUnsupportedOperation error when getFilePaths is called without listdir capability", async () => {packages/ucd-store/test/repair.test.ts (1)
216-229: Prevent flakiness: don’t rely on per-version result orderSimilar to the mirror tests, repair collects per-version results and may not preserve insertion order under concurrency. Fetch by version key instead of indexing.
- assert(repairData[0] != null, "Expected repair results for 15.0.0"); - assert(repairData[1] != null, "Expected repair results for 16.0.0"); - - const repair15Result = repairData[0]; - const repair16Result = repairData[1]; + const repair15Result = repairData.find((r) => r.version === "15.0.0"); + const repair16Result = repairData.find((r) => r.version === "16.0.0"); + assert(repair15Result != null, "Expected repair results for 15.0.0"); + assert(repair16Result != null, "Expected repair results for 16.0.0");packages/ucd-store/test/analyze.test.ts (2)
256-259: Make error assertions resilient: check type and fields, not deep-equality of instancesComparing to a newly constructed error instance via toEqual can be brittle. Prefer instance check + property assertions.
- expect(error).toEqual(new UCDStoreVersionNotFoundError(version)); + expect(error).toBeInstanceOf(UCDStoreVersionNotFoundError); + // @ts-expect-error – narrow after runtime assert + expect(error.version).toBe(version); + expect(error.message).toBe(`Version '${version}' does not exist in the store.`);
314-317: HEAD mocks shouldn’t return a bodyHEAD responses conventionally have no body. It’s harmless here, but tightening the mock helps catch regressions where code accidentally reads HEAD bodies.
- ["HEAD", `${UCDJS_API_BASE_URL}/api/v1/files/15.0.0`, () => { - return HttpResponse.json(stripChildrenFromEntries(MOCK_FILES)); - }], + ["HEAD", `${UCDJS_API_BASE_URL}/api/v1/files/15.0.0`, () => { + return HttpResponse.text("", { status: 200 }); + }],packages/ucd-store/test/clean.test.ts (1)
205-237: Prevent flakiness: avoid indexing for multi-version assertionsOrder of per-version results can vary when execution is concurrent. Locate results by version key.
- assert(cleanData[0] != null, "Expected clean results for 15.0.0"); - assert(cleanData[1] != null, "Expected clean results for 16.0.0"); - - const clean15Result = cleanData[0]; - const clean16Result = cleanData[1]; + const clean15Result = cleanData.find((r) => r.version === "15.0.0"); + const clean16Result = cleanData.find((r) => r.version === "16.0.0"); + assert(clean15Result != null, "Expected clean results for 15.0.0"); + assert(clean16Result != null, "Expected clean results for 16.0.0");packages/ucd-store/src/store.ts (2)
121-135: Small TOCTOU window between exists() and listdir(); consider single-step handlingBetween exists() and listdir() there’s a minor time-of-check/time-of-use gap. If a version folder is deleted between these calls, listdir() may throw. This is low risk, but you can simplify by letting listdir() fail and mapping the resulting error to UCDStoreVersionNotFoundError for a single operation.
Apply this diff to reduce the TOCTOU window:
- assertCapability(this.#fs, ["listdir", "exists"]); - if (!await this.#fs.exists(join(this.basePath, version))) { - throw new UCDStoreVersionNotFoundError(version); - } - - const entries = await this.#fs.listdir(join(this.basePath, version), true); + assertCapability(this.#fs, ["listdir"]); + let entries: UnicodeTreeNode[]; + try { + entries = await this.#fs.listdir(join(this.basePath, version), true); + } catch (e) { + throw new UCDStoreVersionNotFoundError(version); + }
138-171: Confirm FS-bridge FileEntry.path is relative and adjust childPath accordinglyI verified that FS-bridge’s
FileEntry.pathis always defined relative to the directory passed tolistdir:
- In the NodeBridge implementation,
pathis set toentry.name(no leading segments)- In HTTPBridge tests,
pathis mocked (e.g."/file1.txt"), but in real usage the server should emit paths relative to the requested directory, not fully version-scoped pathsTherefore, unconditionally calling
const childPath = join(parentPath, child.path ?? child.name);will correctly prepend
parentPathfor all entries. There is no scenario wherechild.pathalready includes the version prefix, so no double-prepends occur.Optional refinement
To guard against unexpected absolute paths (e.g. tests or custom bridges emitting a leading slash), you can add anisAbsolutecheck:- const childPath = join(parentPath, child.path ?? child.name); + const rawPath = child.path ?? child.name; + const childPath = isAbsolute(rawPath) + ? rawPath + : join(parentPath, rawPath);This is not required given FS-bridge’s documented behavior, but can harden against anomalies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/cli/src/cmd/store/analyze.ts(1 hunks)packages/ucd-store/playgrounds/node-playground.ts(1 hunks)packages/ucd-store/src/internal/analyze.ts(1 hunks)packages/ucd-store/src/internal/clean.ts(1 hunks)packages/ucd-store/src/store.ts(5 hunks)packages/ucd-store/src/types.ts(1 hunks)packages/ucd-store/test/analyze.test.ts(14 hunks)packages/ucd-store/test/clean.test.ts(12 hunks)packages/ucd-store/test/file-operations.test.ts(20 hunks)packages/ucd-store/test/mirror.test.ts(6 hunks)packages/ucd-store/test/repair.test.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
PR: ucdjs/ucd#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/ucd-store/src/internal/analyze.tspackages/ucd-store/test/file-operations.test.tspackages/ucd-store/test/clean.test.tspackages/ucd-store/src/store.ts
📚 Learning: 2025-08-17T10:10:19.096Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/test/clean.test.ts:273-301
Timestamp: 2025-08-17T10:10:19.096Z
Learning: In UCD store clean tests, race conditions between analyze and delete phases are intentionally tested to verify robust handling of files that disappear during clean operations. The test pattern: start clean() → externally delete file during execution → verify file is marked as "skipped" rather than "failed".
Applied to files:
packages/ucd-store/src/internal/clean.tspackages/ucd-store/test/clean.test.tspackages/ucd-store/test/analyze.test.ts
📚 Learning: 2025-08-17T10:06:18.009Z
Learnt from: luxass
PR: ucdjs/ucd#206
File: packages/ucd-store/src/internal/repair.ts:88-96
Timestamp: 2025-08-17T10:06:18.009Z
Learning: The internal__clean function in ucdjs/ucd-store can be safely called with versions: [] and a directories parameter to clean up only specific directories without processing any version files or modifying the manifest. When versions is empty, the analysis returns no results, skipping all file deletion logic, but the directory cleanup section still processes directories from the directories parameter.
Applied to files:
packages/ucd-store/src/internal/clean.tspackages/ucd-store/test/clean.test.tspackages/ucd-store/src/store.ts
📚 Learning: 2025-07-13T09:23:43.820Z
Learnt from: luxass
PR: ucdjs/ucd#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/ucd-store/test/repair.test.tspackages/ucd-store/test/analyze.test.ts
📚 Learning: 2025-08-23T05:20:36.929Z
Learnt from: luxass
PR: ucdjs/ucd#209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.929Z
Learning: In the ucdjs/ucd codebase using modern Vitest, the assert export from "vitest" works as a callable function assert(condition, message) and provides TypeScript type narrowing, making it the preferred choice over expect() for runtime assertions that need compile-time type safety. The existing usage pattern assert(condition, message) throughout the test suite is working correctly.
Applied to files:
packages/ucd-store/test/clean.test.tspackages/ucd-store/test/analyze.test.ts
📚 Learning: 2025-08-23T05:20:36.929Z
Learnt from: luxass
PR: ucdjs/ucd#209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.929Z
Learning: In the ucdjs/ucd codebase, Vitest's assert import works as a callable function assert(condition, message) and provides TypeScript type narrowing, making it the preferred choice over expect() for runtime assertions that also need compile-time type safety.
Applied to files:
packages/ucd-store/test/clean.test.ts
📚 Learning: 2025-08-23T05:16:26.833Z
Learnt from: luxass
PR: ucdjs/ucd#209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:16:26.833Z
Learning: In the ucdjs/ucd codebase, the team prefers using `assert(condition, message)` over `expect(condition).toBeTruthy()` because assert provides TypeScript type narrowing while expect doesn't. This is important for maintaining type safety after runtime validation checks.
Applied to files:
packages/ucd-store/test/clean.test.ts
🧬 Code graph analysis (5)
packages/ucd-store/src/internal/clean.ts (1)
packages/ucd-store/src/errors.ts (1)
UCDStoreError(7-23)
packages/ucd-store/test/mirror.test.ts (1)
tooling/test-utils/src/msw.ts (2)
mockFetch(50-76)HttpResponse(210-210)
packages/ucd-store/playgrounds/node-playground.ts (1)
packages/ucd-store/src/store.ts (1)
analyze(336-358)
packages/ucd-store/test/analyze.test.ts (3)
packages/ucd-store/src/errors.ts (2)
UCDStoreVersionNotFoundError(46-62)UCDStoreError(7-23)packages/ucd-store/src/index.ts (3)
UCDStoreVersionNotFoundError(5-5)UCDStoreError(2-2)createHTTPUCDStore(9-9)packages/ucd-store/test/__shared.ts (1)
stripChildrenFromEntries(116-121)
packages/ucd-store/src/store.ts (3)
packages/shared/src/try-catch.ts (2)
OperationResult(3-3)tryCatch(11-27)packages/ucd-store/src/errors.ts (4)
StoreError(133-139)UCDStoreNotInitializedError(112-124)UCDStoreVersionNotFoundError(46-62)UCDStoreError(7-23)packages/ucd-store/src/internal/analyze.ts (3)
AnalyzeOptions(5-17)AnalyzeResult(19-60)internal__analyze(62-117)
🔇 Additional comments (15)
packages/ucd-store/src/internal/clean.ts (1)
50-58: Ignore unnecessary refactor for empty-versions cleanupThe assumption that passing
versions: []tointernal__cleantriggers a full-store analysis is incorrect. By design:
store.analyze({ versions: [] })immediately returns[[], null], sinceinternal__analyzedoesPromise.all([])when the versions array is empty (no versions to map over).- As a result,
analysesis[], so no files are ever scheduled for deletion.- The only remaining work is the directory cleanup on the provided
directoriesarray and a no-op on the manifest removal loop (sinceversionsis empty).This matches the intended “directory-only” cleanup contract (and aligns with existing tests and repair flows). No change is needed in
clean.ts.Likely an incorrect or invalid review comment.
packages/ucd-store/src/internal/analyze.ts (1)
74-79: Good alignment with OperationResult tuple.Destructuring
[actualFiles, error]and throwing on error matches the new result type contract. No issues spotted.packages/ucd-store/test/mirror.test.ts (2)
96-103: Good fix: avoid order-dependent assertions in concurrent resultsUsing Array.prototype.find to locate per-version results eliminates flakiness from nondeterministic insertion order. This addresses earlier concurrency ordering issues.
212-237: Clear API error scenario is well-coveredGreat to see the 500 response mapped to a readable message with the version context. This will help a lot when diagnosing failures.
packages/ucd-store/test/repair.test.ts (1)
253-271: Nice intact-store coverageValidating that an intact store yields only “skipped” operations is a solid guard against accidental deletions.
packages/ucd-store/test/clean.test.ts (2)
135-183: Nice race-condition coverage for “skip” on disappearing filesThe pattern (analyze → delete external file → clean with concurrency:1) is a solid guard to ensure the file is marked as skipped, not failed. Matches the intended behavior from earlier learnings.
322-327: Clear concurrency guard assertionAsserting data is null and checking the message provides good signal for callers and keeps failure shape consistent across operations.
packages/ucd-store/src/store.ts (8)
4-5: Good: Public API now consistently typed via OperationResult/StoreErrorImporting OperationResult and StoreError here aligns the surface with the new result-wrapper pattern and improves uniformity across methods.
18-18: Nice consolidation of shared utilitiesImporting tryCatch, createPathFilter, flattenFilePaths, and safeJsonParse from a single shared entrypoint keeps this file lean and focused. No action needed.
205-223: LGTM: getFilePaths correctly composes getFileTree + flattenFilePathsError propagation via tryCatch is consistent, and the success path stays simple. No change requested.
336-358: LGTM: analyze() wraps internal flow with consistent result typingDefaults and version fallback to this.#versions look good. The method cleanly funnels the array result through tryCatch.
372-397: LGTM: mirror() matches the new result-wrapper API and preserves options behaviorParameter defaults and delegation to internal__mirror are consistent. No change requested.
436-460: LGTM: repair() aligns with the new OperationResult<RepairResult[], StoreError> surfaceOptions handling mirrors clean()/mirror() patterns. No change requested.
481-489: Good: safe JSON parse + zod schema validation for manifestThe two-phase validation (parse then schema) is solid and provides precise errors. No action needed.
205-223: OperationResult is correctly defined as tuple typesThe types in
packages/shared/src/try-catch.tsare declared as:export type OperationSuccess<T> = readonly [data: T, error: null]; export type OperationFailure<E> = readonly [data: null, error: E]; export type OperationResult<T, E> = OperationSuccess<T> | OperationFailure<E>;Since both success and failure branches are readonly 2-tuples with the same positions, destructuring via
[data, error]is safe and behaves as expected. No further changes are required.
| const filterDirectoryChildren = (children: UnicodeTreeNode[], parentPath: string): UnicodeTreeNode[] => { | ||
| const result: UnicodeTreeNode[] = []; | ||
|
|
||
| for (const child of children) { | ||
| const childPath = join(parentPath, child.path ?? child.name); | ||
| const isFiltered = this.#filter(childPath, extraFilters); | ||
|
|
||
| // fast path for files and empty directories | ||
| if (child.type === "file" || (child.type === "directory" && (!child.children || child.children.length === 0))) { | ||
| if (isFiltered) { | ||
| result.push(child); | ||
| } | ||
|
|
||
| assertCapability(this.#fs, "listdir"); | ||
| const entries = await this.#fs.listdir(join(this.basePath, version), true); | ||
| continue; | ||
| } | ||
|
|
||
| // handle directories with children | ||
| if (child.type === "directory" && child.children) { | ||
| const filteredGrandChildren = filterDirectoryChildren(child.children, childPath); | ||
|
|
||
| if (isFiltered && filteredGrandChildren.length > 0) { | ||
| result.push({ | ||
| name: child.name, | ||
| path: child.path, | ||
| type: "directory", | ||
| children: filteredGrandChildren, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Directory filtering drops valid descendants; don’t require parent dir to match filter
The current logic only includes a directory node if the directory itself matches the filters AND it has filtered descendants. If a directory doesn’t match but contains matching files, the whole subtree is dropped. Expected behavior: include a directory if any descendant matches, regardless of whether the directory path itself matches.
Apply this diff to keep directories that contain filtered descendants:
- const filterDirectoryChildren = (children: UnicodeTreeNode[], parentPath: string): UnicodeTreeNode[] => {
+ const filterDirectoryChildren = (children: UnicodeTreeNode[], parentPath: string): UnicodeTreeNode[] => {
const result: UnicodeTreeNode[] = [];
for (const child of children) {
- const childPath = join(parentPath, child.path ?? child.name);
+ // Avoid double-prepending parentPath when child.path is already full/absolute
+ const childPath = child.path ?? join(parentPath, child.name);
const isFiltered = this.#filter(childPath, extraFilters);
// fast path for files and empty directories
if (child.type === "file" || (child.type === "directory" && (!child.children || child.children.length === 0))) {
if (isFiltered) {
result.push(child);
}
continue;
}
// handle directories with children
if (child.type === "directory" && child.children) {
const filteredGrandChildren = filterDirectoryChildren(child.children, childPath);
- if (isFiltered && filteredGrandChildren.length > 0) {
+ // Include directory if it contains any filtered descendants, even if dir itself doesn't match
+ if (filteredGrandChildren.length > 0) {
result.push({
name: child.name,
path: child.path,
type: "directory",
children: filteredGrandChildren,
});
}
}
}
return result;
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/ucd-store/src/store.ts around lines 138-171, the directory filtering
currently only adds a directory when both the directory itself matches the
filter and it has matching descendants; change that so directories are added
whenever they have any filtered descendants regardless of the directory path
matching. Concretely, keep the fast-path for files/empty directories (push if
isFiltered), but for directories with children push the directory node whenever
filteredGrandChildren.length > 0 (using the same name/path/type/children shape),
removing the isFiltered requirement for including parent directories; leave
other logic unchanged.
| for (const entry of entries) { | ||
| const isFiltered = this.#filter(entry.path, extraFilters); | ||
|
|
||
| // fast path for files and empty directories | ||
| if (child.type === "file" || (child.type === "directory" && (!child.children || child.children.length === 0))) { | ||
| if (entry.type === "file" || (entry.type === "directory" && (!entry.children || entry.children.length === 0))) { | ||
| if (isFiltered) { | ||
| result.push(child); | ||
| result.push(entry); | ||
| } | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| // handle directories with children | ||
| if (child.type === "directory" && child.children) { | ||
| const filteredGrandChildren = filterDirectoryChildren(child.children, childPath); | ||
| if (entry.type === "directory" && entry.children) { | ||
| const filteredChildren = filterDirectoryChildren(entry.children, entry.path); | ||
|
|
||
| if (isFiltered && filteredGrandChildren.length > 0) { | ||
| if (isFiltered && filteredChildren.length > 0) { | ||
| result.push({ | ||
| name: child.name, | ||
| path: child.path, | ||
| name: entry.name, | ||
| path: entry.path, | ||
| type: "directory", | ||
| children: filteredGrandChildren, | ||
| children: filteredChildren, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Top-level directory gating has the same issue; allow directories that contain filtered descendants
Mirrors the inner logic bug: currently requires the directory to match to include it. This drops valid files if only children match.
Apply this diff:
- if (entry.type === "directory" && entry.children) {
- const filteredChildren = filterDirectoryChildren(entry.children, entry.path);
- if (isFiltered && filteredChildren.length > 0) {
+ if (entry.type === "directory" && entry.children) {
+ const filteredChildren = filterDirectoryChildren(entry.children, entry.path);
+ if (filteredChildren.length > 0) {
result.push({
name: entry.name,
path: entry.path,
type: "directory",
children: filteredChildren,
});
}
}📝 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.
| for (const entry of entries) { | |
| const isFiltered = this.#filter(entry.path, extraFilters); | |
| // fast path for files and empty directories | |
| if (child.type === "file" || (child.type === "directory" && (!child.children || child.children.length === 0))) { | |
| if (entry.type === "file" || (entry.type === "directory" && (!entry.children || entry.children.length === 0))) { | |
| if (isFiltered) { | |
| result.push(child); | |
| result.push(entry); | |
| } | |
| continue; | |
| } | |
| // handle directories with children | |
| if (child.type === "directory" && child.children) { | |
| const filteredGrandChildren = filterDirectoryChildren(child.children, childPath); | |
| if (entry.type === "directory" && entry.children) { | |
| const filteredChildren = filterDirectoryChildren(entry.children, entry.path); | |
| if (isFiltered && filteredGrandChildren.length > 0) { | |
| if (isFiltered && filteredChildren.length > 0) { | |
| result.push({ | |
| name: child.name, | |
| path: child.path, | |
| name: entry.name, | |
| path: entry.path, | |
| type: "directory", | |
| children: filteredGrandChildren, | |
| children: filteredChildren, | |
| }); | |
| } | |
| for (const entry of entries) { | |
| const isFiltered = this.#filter(entry.path, extraFilters); | |
| // fast path for files and empty directories | |
| if (entry.type === "file" || (entry.type === "directory" && (!entry.children || entry.children.length === 0))) { | |
| if (isFiltered) { | |
| result.push(entry); | |
| } | |
| continue; | |
| } | |
| // handle directories with children | |
| if (entry.type === "directory" && entry.children) { | |
| const filteredChildren = filterDirectoryChildren(entry.children, entry.path); | |
| if (filteredChildren.length > 0) { | |
| result.push({ | |
| name: entry.name, | |
| path: entry.path, | |
| type: "directory", | |
| children: filteredChildren, | |
| }); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/ucd-store/src/store.ts around lines 174 to 197, the top-level
directory handling currently requires the directory itself to match (isFiltered)
before including it even when some children match; change the gating so that any
directory with filteredChildren is included regardless of isFiltered.
Concretely, replace the condition "if (isFiltered && filteredChildren.length >
0)" with a check that pushes the directory whenever "filteredChildren.length >
0" (preserving the same shape: name, path, type: 'directory', children:
filteredChildren).
| async getFile(version: string, filePath: string, extraFilters?: string[]): Promise<OperationResult<string, StoreError>> { | ||
| return tryCatch(async () => { | ||
| if (!this.#initialized) { | ||
| throw new UCDStoreNotInitializedError(); | ||
| } | ||
|
|
||
| assertCapability(this.#fs, "read"); | ||
| try { | ||
| if (isAbsolute(filePath)) { | ||
| return await this.#fs.read(filePath); | ||
| if (!this.#versions.includes(version)) { | ||
| throw new UCDStoreVersionNotFoundError(version); | ||
| } | ||
|
|
||
| return await this.#fs.read(join(version, filePath)); | ||
| } catch (err) { | ||
| if (err instanceof Error && err.message.includes("ENOENT")) { | ||
| throw new UCDStoreError(`File '${filePath}' does not exist in version '${version}'.`); | ||
| if (!this.#filter(filePath, extraFilters)) { | ||
| throw new UCDStoreError(`File path "${filePath}" is filtered out by the store's filter patterns.`); | ||
| } | ||
|
|
||
| throw err; | ||
| } | ||
| assertCapability(this.#fs, "read"); | ||
| try { | ||
| let content = ""; | ||
| if (isAbsolute(filePath)) { | ||
| content = await this.#fs.read(filePath); | ||
| } else { | ||
| content = await this.#fs.read(join(version, filePath)); | ||
| } | ||
|
|
||
| return content; | ||
| } catch (err) { | ||
| if (err instanceof Error && err.message.includes("ENOENT")) { | ||
| throw new UCDStoreError(`File '${filePath}' does not exist in version '${version}'.`); | ||
| } | ||
|
|
||
| throw err; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Security: absolute-path reads and missing basePath when reading can escape store root
Current logic will:
- Read any absolute path the caller provides (e.g., “/etc/passwd”) if the FS bridge allows it.
- For relative paths, it reads join(version, filePath) but omits basePath, diverging from getFileTree’s basePath adherence.
Both allow reading outside the store or outside the configured basePath.
Apply this diff to enforce root confinement and consistent basePath usage, and to align filter checks with version-scoped paths:
-import { isAbsolute, join } from "pathe";
+import { isAbsolute, join, normalize, relative } from "pathe";
@@
- if (!this.#filter(filePath, extraFilters)) {
+ // Filter using a version-scoped path (consistent with getFileTree)
+ if (!this.#filter(join(version, filePath), extraFilters)) {
throw new UCDStoreError(`File path "${filePath}" is filtered out by the store's filter patterns.`);
}
assertCapability(this.#fs, "read");
try {
- let content = "";
- if (isAbsolute(filePath)) {
- content = await this.#fs.read(filePath);
- } else {
- content = await this.#fs.read(join(version, filePath));
- }
-
- return content;
+ const root = join(this.basePath, version);
+ const candidate = isAbsolute(filePath) ? filePath : join(root, filePath);
+ const resolved = normalize(candidate);
+ const rel = relative(root, resolved);
+ // Disallow traversal outside the version root
+ if (rel.startsWith("..") || isAbsolute(rel)) {
+ throw new UCDStoreError(`Refusing to read outside store root: ${filePath}`);
+ }
+ return await this.#fs.read(resolved);
} catch (err) {
- if (err instanceof Error && err.message.includes("ENOENT")) {
+ // Prefer errno code if available; fall back to message
+ if (
+ (err as any)?.code === "ENOENT" ||
+ (err instanceof Error && err.message.includes("ENOENT"))
+ ) {
throw new UCDStoreError(`File '${filePath}' does not exist in version '${version}'.`);
}
throw err;
}I can also wire this to a dedicated FileNotFoundError subclass if you have one (it appears in StoreError), for richer downstream handling.
📝 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.
| async getFile(version: string, filePath: string, extraFilters?: string[]): Promise<OperationResult<string, StoreError>> { | |
| return tryCatch(async () => { | |
| if (!this.#initialized) { | |
| throw new UCDStoreNotInitializedError(); | |
| } | |
| assertCapability(this.#fs, "read"); | |
| try { | |
| if (isAbsolute(filePath)) { | |
| return await this.#fs.read(filePath); | |
| if (!this.#versions.includes(version)) { | |
| throw new UCDStoreVersionNotFoundError(version); | |
| } | |
| return await this.#fs.read(join(version, filePath)); | |
| } catch (err) { | |
| if (err instanceof Error && err.message.includes("ENOENT")) { | |
| throw new UCDStoreError(`File '${filePath}' does not exist in version '${version}'.`); | |
| if (!this.#filter(filePath, extraFilters)) { | |
| throw new UCDStoreError(`File path "${filePath}" is filtered out by the store's filter patterns.`); | |
| } | |
| throw err; | |
| } | |
| assertCapability(this.#fs, "read"); | |
| try { | |
| let content = ""; | |
| if (isAbsolute(filePath)) { | |
| content = await this.#fs.read(filePath); | |
| } else { | |
| content = await this.#fs.read(join(version, filePath)); | |
| } | |
| return content; | |
| } catch (err) { | |
| if (err instanceof Error && err.message.includes("ENOENT")) { | |
| throw new UCDStoreError(`File '${filePath}' does not exist in version '${version}'.`); | |
| } | |
| throw err; | |
| } | |
| }); | |
| } | |
| // Update import to include normalize and relative | |
| import { isAbsolute, join, normalize, relative } from "pathe"; | |
| async getFile(version: string, filePath: string, extraFilters?: string[]): Promise<OperationResult<string, StoreError>> { | |
| return tryCatch(async () => { | |
| if (!this.#initialized) { | |
| throw new UCDStoreNotInitializedError(); | |
| } | |
| if (!this.#versions.includes(version)) { | |
| throw new UCDStoreVersionNotFoundError(version); | |
| } | |
| // Filter using a version-scoped path (consistent with getFileTree) | |
| if (!this.#filter(join(version, filePath), extraFilters)) { | |
| throw new UCDStoreError(`File path "${filePath}" is filtered out by the store's filter patterns.`); | |
| } | |
| assertCapability(this.#fs, "read"); | |
| try { | |
| const root = join(this.basePath, version); | |
| const candidate = isAbsolute(filePath) ? filePath : join(root, filePath); | |
| const resolved = normalize(candidate); | |
| const rel = relative(root, resolved); | |
| // Disallow traversal outside the version root | |
| if (rel.startsWith("..") || isAbsolute(rel)) { | |
| throw new UCDStoreError(`Refusing to read outside store root: ${filePath}`); | |
| } | |
| return await this.#fs.read(resolved); | |
| } catch (err) { | |
| // Prefer errno code if available; fall back to message | |
| if ( | |
| (err as any)?.code === "ENOENT" || | |
| (err instanceof Error && err.message.includes("ENOENT")) | |
| ) { | |
| throw new UCDStoreError(`File '${filePath}' does not exist in version '${version}'.`); | |
| } | |
| throw err; | |
| } | |
| }); | |
| } |
1 participant
🔗 Linked issue
📚 Description
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores