Conversation
🦋 Changeset detectedLatest commit: 1bc63d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@luxass has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis change refactors the Unicode Character Database (UCD) file utilities by extracting mirroring, validation, and filesystem adapter logic into dedicated modules. It updates the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MirrorModule as mirrorUCDFiles
participant Internal as internal_mirrorUnicodeVersion
participant FS as FSAdapter
participant API as Unicode Proxy API
User->>MirrorModule: mirrorUCDFiles(options)
MirrorModule->>API: Fetch file list for each version
MirrorModule->>Internal: internal_mirrorUnicodeVersion(version, options)
Internal->>FS: Ensure output directory
Internal->>API: Download filtered files
API-->>Internal: Return file data
Internal->>FS: Write files to disk
Internal-->>MirrorModule: Return files/errors
MirrorModule-->>User: Return MirrorResult
sequenceDiagram
participant User
participant ValidateModule as validateUCDFiles
participant FS as FSAdapter
participant API as Unicode Proxy API
User->>ValidateModule: validateUCDFiles(version, options)
ValidateModule->>API: Fetch expected file list
ValidateModule->>FS: Read local directory (recursive)
ValidateModule->>ValidateModule: Compare local files to expected
ValidateModule-->>User: Return missingFiles, notRequiredFiles
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
packages/utils/src/ucd-files/fs-adapter.ts (1)
6-9: Doc-comment is stale – adapter now implements more thanreadFileThe JSDoc still states “it only implements the readFile method” but the object now exposes six methods.
Update the description to avoid confusion for consumers and future maintainers.packages/utils/test/ucd-files/fs-adapter.test.ts (1)
24-35: Prefervi.resetAllMocks()after dynamic importAfter importing the spied
readFile, subsequent tests could receive residual mock state.
Callingvi.resetAllMocks()(or reading the mock insidebeforeEach) keeps isolation tight..changeset/silent-suits-guess.md (1)
5-5: Consider expanding the changeset summaryA one-line “refactor ucd-files” note gives little context for consumers scanning the changelog.
Briefly list the extracted modules (fs-adapter,mirror,validate) or the newrecursive readdirfeature so downstream users understand the impact.packages/utils/test/ucd-files/mirror.test.ts (1)
185-192: Mockreaddirsignature should acceptrecursive
mirrorUCDFilesmay invokefs.readdir(path, true).
Define the mock as(path, _recursive?) => …to avoid unexpectedTypeErroronce the function is fixed:- readdir: vi.fn().mockResolvedValue([]), + readdir: vi.fn().mockResolvedValue([]) as unknown as FSAdapter["readdir"],(Or use
vi.fn<FSAdapter["readdir"]>().)
This keeps the mock type-safe and forward-compatible.packages/utils/src/ucd-files/validate.ts (1)
56-69: JSDoc parameter list is out of sync.
@paramtags should describe bothversionandoptions, otherwise typedoc/ESLint complain (see static-analysis hint).🧰 Tools
🪛 GitHub Check: build
[warning] 56-56:
Expected @param names to be "version, options". Got "options"packages/utils/src/ucd-files/mirror.ts (1)
73-80: Unconditionally instantiating a default FS adapter is wasteful.
await createDefaultFSAdapter()runs even when the caller already suppliedoptions.fs, incurring an unnecessary dynamicimport("node:fs/promises").- fs: await createDefaultFSAdapter(), + fs: options.fs ?? await createDefaultFSAdapter(),A small optimisation, but worthwhile inside tight loops or serverless functions.
packages/utils/src/ucd-files/internal.ts (1)
135-161: Potentially overwhelming concurrency when mirroring many files.All fetches are fired in parallel; mirroring a full Unicode version (~700 files) can hammer the proxy and consume memory.
Consider throttling withp-limit/ a simple semaphore.packages/utils/test/ucd-files/validate.test.ts (1)
249-270: Spy expectation misses therecursiveargument
FSAdapter.readdirwas updated to accept a secondrecursiveparameter.
The implementation calls:readdir(path, true)but the assertion only expects the path. Align the test:
-expect(mockFs.readdir).toHaveBeenCalledWith(`${testdirPath}/v16.0.0`); +expect(mockFs.readdir).toHaveBeenCalledWith( + `${testdirPath}/v16.0.0`, + true +);(or use
expect.any(Boolean)if you don’t want to couple to the exact flag).
This fixes the CI failure reported at line 269.🧰 Tools
🪛 GitHub Check: build
[failure] 269-269: packages/utils/test/ucd-files/validate.test.ts > validateUCDFiles > configuration options > should use custom filesystem adapter
AssertionError: expected "spy" to be called with arguments: [ Array(1) ]Received:
1st spy call:
[
"/home/runner/work/ucd/ucd/.vitest-testdirs/vitest-validate-configuration-options-should-use-custom-filesystem-adapter/v16.0.0",
- true,
]Number of calls: 1
❯ packages/utils/test/ucd-files/validate.test.ts:269:30
🪛 GitHub Actions: CI
[error] 269-269: Test failure: expected 'spy' to be called with arguments [ Array(1) ] in 'should use custom filesystem adapter'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/silent-suits-guess.md(1 hunks)packages/utils/src/types.ts(1 hunks)packages/utils/src/ucd-files.ts(1 hunks)packages/utils/src/ucd-files/fs-adapter.ts(1 hunks)packages/utils/src/ucd-files/internal.ts(1 hunks)packages/utils/src/ucd-files/mirror.ts(1 hunks)packages/utils/src/ucd-files/validate.ts(1 hunks)packages/utils/test/ucd-files/fs-adapter.test.ts(1 hunks)packages/utils/test/ucd-files/mirror.test.ts(1 hunks)packages/utils/test/ucd-files/validate.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/utils/src/ucd-files/mirror.ts (3)
packages/utils/src/types.ts (1)
FSAdapter(5-12)packages/utils/src/ucd-files/fs-adapter.ts (1)
createDefaultFSAdapter(13-58)packages/utils/src/ucd-files/internal.ts (1)
internal_mirrorUnicodeVersion(14-87)
packages/utils/test/ucd-files/fs-adapter.test.ts (1)
packages/utils/src/ucd-files/fs-adapter.ts (2)
createDefaultFSAdapter(13-58)readFile(18-20)
packages/utils/src/ucd-files/validate.ts (3)
packages/utils/src/types.ts (1)
FSAdapter(5-12)packages/utils/src/ucd-files/fs-adapter.ts (1)
createDefaultFSAdapter(13-58)packages/utils/src/ucd-files/internal.ts (1)
internal__flattenFilePaths(191-205)
🪛 GitHub Check: build
packages/utils/test/ucd-files/validate.test.ts
[failure] 481-481: packages/utils/test/ucd-files/validate.test.ts > validateUCDFiles > edge cases > should handle empty file list from API
AssertionError: expected [] to include 'SomeFile.txt'
❯ packages/utils/test/ucd-files/validate.test.ts:481:39
[failure] 269-269: packages/utils/test/ucd-files/validate.test.ts > validateUCDFiles > configuration options > should use custom filesystem adapter
AssertionError: expected "spy" to be called with arguments: [ Array(1) ]
Received:
1st spy call:
[
"/home/runner/work/ucd/ucd/.vitest-testdirs/vitest-validate-configuration-options-should-use-custom-filesystem-adapter/v16.0.0",
- true,
]
Number of calls: 1
❯ packages/utils/test/ucd-files/validate.test.ts:269:30
[failure] 108-108: packages/utils/test/ucd-files/validate.test.ts > validateUCDFiles > basic validation functionality > should identify extra files that are not required
AssertionError: expected [] to include 'ExtraFile.txt'
❯ packages/utils/test/ucd-files/validate.test.ts:108:39
packages/utils/src/ucd-files/validate.ts
[warning] 56-56:
Expected @param names to be "version, options". Got "options"
🪛 GitHub Actions: CI
packages/utils/test/ucd-files/validate.test.ts
[error] 108-108: Test failure: expected [] to include 'ExtraFile.txt' in 'should identify extra files that are not required'
[error] 269-269: Test failure: expected 'spy' to be called with arguments [ Array(1) ] in 'should use custom filesystem adapter'
[error] 481-481: Test failure: expected [] to include 'SomeFile.txt' in 'should handle empty file list from API'
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
packages/utils/src/ucd-files.ts (1)
1-10: Barrel re-exports look clean.No issues spotted.
luxass
changed the base branch from
luxass/06-14-feat-add-validateucdfiles-function-and-update-fsadapter-interface-with-exists-and-readdir-methods
to
graphite-base/56
…lidateUCDFiles to handle directories
luxass
force-pushed
the
luxass/06-14-refactor-ucd-files-impl
branch
2 times, most recently
from
9b767d9 to
b7ed1db
Compare
…est to reflect changes
luxass
force-pushed
the
luxass/06-14-refactor-ucd-files-impl
branch
from
b7ed1db to
ef300d3
Compare
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
packages/utils/test/ucd-files/mirror.test.ts (2)
206-208:⚠️ Potential issue
mkdirexpectation uses avprefix that no other assertion relies on – likely a mismatch with the actual implementationAll other assertions in this suite expect paths like
16.0.0/UnicodeData.txt(novprefix).
Expecting the adapter to be called with${testdirPath}/v16.0.0is therefore inconsistent and will turn into a false-negative ifmirrorUCDFilescorrectly mirrors to<basePath>/<version>.-expect(mockFs.mkdir).toHaveBeenCalledWith(`${testdirPath}/v16.0.0`, { recursive: true }); +expect(mockFs.mkdir).toHaveBeenCalledWith(`${testdirPath}/16.0.0`, { recursive: true });
406-409:⚠️ Potential issueStubbed file download returns JSON instead of plain text
mirrorUCDFilespresumably writes the raw file contents. Returning JSON here (HttpResponse.json(...)) yields aContent-Type: application/jsonresponse and a stringified body, which is an unrealistic contract and could mask encoding bugs.-["GET https://unicode-proxy.ucdjs.dev/16.0.0/ucd/level1/level2/level3/deep-file.txt", () => { - return HttpResponse.json({ name: "deep-file.txt", path: "deep-file.txt" }); -}], +["GET https://unicode-proxy.ucdjs.dev/16.0.0/ucd/level1/level2/level3/deep-file.txt", () => + new HttpResponse("dummy-file-content"), +],
♻️ Duplicate comments (3)
packages/utils/src/ucd-files/fs-adapter.ts (1)
47-50:⚠️ Potential issue
readdirstill relies on a non-existentrecursiveoption – implement real recursion.
fs.promises.readdirsilently ignores therecursiveflag on every currently-released Node.js version (≤ v22).
Callers will believe they received a deep listing while in fact only one level is returned, breakingvalidateUCDFilesand anything else that depends on it.- async readdir(dirPath, recursive = false) { - return fsModule.readdir(dirPath, { - recursive, - }); - }, + async readdir(dirPath, recursive = false) { + if (!recursive) { + // Shallow case stays cheap. + return fsModule.readdir(dirPath); + } + + const out: string[] = []; + const stack: string[] = [dirPath]; + + while (stack.length) { + const current = stack.pop()!; + const entries = await fsModule.readdir(current, { withFileTypes: true }); + for (const entry of entries) { + const full = `${current}/${entry.name}`; + if (entry.isDirectory()) { + stack.push(full); + } else { + out.push(path.relative(dirPath, full)); + } + } + } + return out; + },Don’t forget to cover the recursive branch in unit tests.
packages/utils/src/ucd-files/internal.ts (1)
8-9:⚠️ Potential issueDownload host is still hard-wired –
apiUrl/ client setting is ignored for file downloads.Even though the high-level API allows a custom
apiUrl, actual file bytes are always fetched fromhttps://unicode-proxy.ucdjs.dev. This is the same issue flagged earlier; it prevents users from mirroring from self-hosted endpoints or during tests.Expose the base URL as an argument (e.g. pass it through
mirrorOptions) or reuse the already-configured REST client’s base to keep list & file fetches consistent.Also applies to: 139-140
packages/utils/src/ucd-files/validate.ts (1)
142-147: 🛠️ Refactor suggestionCatching everything and returning empty results hides real failures.
The function now swallows all errors again and reports “no problems” (
[]). Callers lose any context, and CI cannot fail on genuine issues.Either re-throw the enriched error or return
{ missingFiles, notRequiredFiles, error }.
🧹 Nitpick comments (4)
packages/utils/src/ucd-files/validate.ts (1)
56-69: JSDoc parameter mismatch.
@paramlistsoptions.versionetc., but the actual function signature is(version, options). Update the comment or the tags to avoid TS/ESDoc tooling warnings.🧰 Tools
🪛 GitHub Check: build
[warning] 56-56:
Expected @param names to be "version, options". Got "options"packages/utils/test/ucd-files/validate.test.ts (2)
42-46: Factor the API URL into a shared helper to reduce duplicationThe literal
"GET https://unicode-api.luxass.dev/api/v1/unicode-files/16.0.0"
is repeated in almost every test. Centralising the base URL (e.g.const DEFAULT_API = "https://unicode-api.luxass.dev"or a tinymockUnicodeFiles(version, payload)helper) will:
- make future URL changes trivial,
- eliminate typo-risk,
- shorten individual test cases.
No functional impact, just tidier & easier to maintain.
155-159: Redundant assertion
expect(result.missingFiles).not.toContain("emojis/emoji-data.txt")is implied by the immediatetoEqual([])that follows. Dropping the first line tightens the test without losing coverage.-// Should not include emoji files in missing files since they're filtered out -expect(result.missingFiles).not.toContain("emojis/emoji-data.txt"); -expect(result.missingFiles).toEqual([]); +expect(result.missingFiles).toEqual([]);packages/utils/test/ucd-files/mirror.test.ts (1)
185-193: CustomFSAdaptermocks are incomplete – add missing methods to avoid runtime failures
mirrorUCDFilesmay invoke additional adapter methods such asstat,unlink, orchmod. Any un-mocked call will throwTypeError: fn is not a functionand cause brittle, misleading failures when the implementation evolves.Recommend defining the adapter with full coverage or using
vi.fn()&Proxyto auto-stub unknown keys.Also applies to: 308-315
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/silent-suits-guess.md(1 hunks)packages/utils/src/types.ts(1 hunks)packages/utils/src/ucd-files.ts(1 hunks)packages/utils/src/ucd-files/fs-adapter.ts(1 hunks)packages/utils/src/ucd-files/internal.ts(1 hunks)packages/utils/src/ucd-files/mirror.ts(1 hunks)packages/utils/src/ucd-files/validate.ts(1 hunks)packages/utils/test/ucd-files/fs-adapter.test.ts(1 hunks)packages/utils/test/ucd-files/mirror.test.ts(1 hunks)packages/utils/test/ucd-files/validate.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/silent-suits-guess.md
- packages/utils/src/types.ts
- packages/utils/test/ucd-files/fs-adapter.test.ts
- packages/utils/src/ucd-files/mirror.ts
🧰 Additional context used
🧠 Learnings (1)
packages/utils/src/ucd-files/fs-adapter.ts (1)
Learnt from: luxass
PR: ucdjs/ucd#6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
🧬 Code Graph Analysis (1)
packages/utils/src/ucd-files/validate.ts (3)
packages/utils/src/types.ts (1)
FSAdapter(5-12)packages/utils/src/ucd-files/fs-adapter.ts (1)
createDefaultFSAdapter(13-58)packages/utils/src/ucd-files/internal.ts (1)
internal__flattenFilePaths(191-205)
🪛 GitHub Check: codecov/patch
packages/utils/src/ucd-files/internal.ts
[warning] 143-149: packages/utils/src/ucd-files/internal.ts#L143-L149
Added lines #L143 - L149 were not covered by tests
[warning] 155-160: packages/utils/src/ucd-files/internal.ts#L155-L160
Added lines #L155 - L160 were not covered by tests
packages/utils/src/ucd-files/fs-adapter.ts
[warning] 28-31: packages/utils/src/ucd-files/fs-adapter.ts#L28-L31
Added lines #L28 - L31 were not covered by tests
[warning] 37-46: packages/utils/src/ucd-files/fs-adapter.ts#L37-L46
Added lines #L37 - L46 were not covered by tests
packages/utils/src/ucd-files/validate.ts
[warning] 87-88: packages/utils/src/ucd-files/validate.ts#L87-L88
Added lines #L87 - L88 were not covered by tests
🪛 GitHub Check: build
packages/utils/src/ucd-files/validate.ts
[warning] 56-56:
Expected @param names to be "version, options". Got "options"
🔇 Additional comments (2)
packages/utils/src/ucd-files.ts (1)
1-9: Barrel file looks good.Re-exports are clean and maintain original public surface.
packages/utils/test/ucd-files/mirror.test.ts (1)
47-52: Network calls for file downloads are not mocked – tests may hit the real networkEach test only stubs the file-list endpoint (
…/unicode-files/<version>).
mirrorUCDFilessubsequently downloads every individual file (e.g.…/unicode-proxy.ucdjs.dev/<version>/ucd/<path>).
Without corresponding mocks Vitest will attempt real HTTP requests, making the suite flaky and dramatically slower offline/CI.Consider adding a catch-all MSW handler or explicit mocks for the per-file URLs, e.g.:
mockFetch([ // existing list mock … ['GET https://unicode-proxy.ucdjs.dev/**', () => new HttpResponse('file', { status: 200 })], ]);Also applies to: 69-78, 95-100, 114-119, 134-139, 160-165, 185-193, 213-219, 340-344
1 participant
This PR refactors the entire UCD Files part of the utils package.
Summary by CodeRabbit
Refactor
New Features
Tests
Chores