refactor: support configuration in manifest#395
Conversation
|
|
Warning Rate limit exceeded@luxass has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 4 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 (1)
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. WalkthroughThe PR changes the UCD store manifest to map versions to objects with an expectedFiles array, updating schemas, manifest read/write APIs, API routes, mock-store handlers, CI workflow, and many tests to the new manifest shape and related call-sites. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🌏 Preview Deployments
Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
📋 OpenAPI Schema AnalysisSummarySchema Components
Overall Status
Detailed Changes📋 Schema Components ChangesModified Schemas (1):
🤖 This comment is automatically updated when you push new commits. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request refactors the UCD store manifest structure to support per-version configuration metadata. Previously, the manifest was a simple key-value mapping where each version string mapped to itself (e.g., "15.0.0": "15.0.0"). The new structure maps version strings to configuration objects containing metadata like expectedFiles, which will track the expected file paths for each version.
Key changes:
- Updated manifest schema to use object values with
expectedFilesarray instead of simple string-to-string mapping - Modified manifest generation script to populate the
expectedFilesarray by traversing Unicode.org directories - Changed API endpoint to read manifest from Cloudflare R2 bucket instead of scraping Unicode.org
- Updated all test files across ucd-store and ucd-store-v2 packages to reflect new manifest structure
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/schemas/src/fs.ts |
Updated UCDStoreManifestSchema to map versions to metadata objects with expectedFiles array |
packages/ucd-store/src/store.ts |
Updated manifest writing to use new object structure with expectedFiles |
packages/ucd-store-v2/src/core/manifest.ts |
Changed writeManifest signature to accept UCDStoreManifest object instead of version array |
packages/ucd-store-v2/src/store.ts |
Updated conflict resolution to build manifest objects with expectedFiles |
packages/ucd-store-v2/src/setup/bootstrap.ts |
Modified bootstrap to create manifest objects with expectedFiles |
packages/ucd-store-v2/src/operations/sync.ts |
Updated sync operation to write manifest with new structure |
apps/api/src/routes/v1_files/routes.ts |
Refactored to fetch manifest from R2 bucket instead of scraping Unicode.org; removed unused import |
apps/api/src/routes/v1_files/openapi.ts |
Updated OpenAPI example to show new manifest structure with expectedFiles arrays |
apps/api/src/routes/v1_versions/openapi.ts |
Removed unnecessary as const from middleware array |
apps/api/src/routes/.well-known/openapi.ts |
Removed unnecessary as const from middleware array |
.github/workflows/refresh-manifest.yaml |
Updated matrix configuration and removed redundant conditional checks |
packages/test-utils/src/mock-store/handlers/files.ts |
Updated mock handler to return new manifest structure |
packages/test-utils/test/mock-store/mock-store.test.ts |
Updated test assertions to expect new manifest structure |
| Multiple test files in ucd-store and ucd-store-v2 | Updated all test fixtures to use new manifest structure with expectedFiles |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor the manifest retrieval process to list version directories under the `manifest/` prefix. This change allows for fetching `manifest.json` files for each version, improving the structure and accessibility of versioned manifests. The code now tracks the latest upload time for the `Last-Modified` header, enhancing cache control.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ucd-store/src/store.ts (1)
460-469: Don’t clobber per-version manifest config when rewriting (currently always resetsexpectedFiles).
~writeManifest(versions)reconstructs the manifest from scratch and setsexpectedFiles: []for every version, losing any config read from an existing.ucd-store.json. This is especially risky becauseinit()merges versions and may rewrite the manifest.One minimal fix is to pass the existing manifest into
~writeManifestand preserve entries when present:- async "~writeManifest"(versions: string[]): Promise<void> { + async "~writeManifest"(versions: string[], existing: UCDStoreManifest = {}): Promise<void> { assertCapability(this.#fs, "write"); const manifestData: UCDStoreManifest = {}; for (const version of versions) { - manifestData[version] = { expectedFiles: [] }; + manifestData[version] = existing[version] ?? { expectedFiles: [] }; } await this.#fs.write(this.#manifestPath, JSON.stringify(manifestData, null, 2)); }…and in
init(), keep the full parsed manifest (not just keys) and pass it when rewriting.Based on learnings, preserving config is key to making the “manifest supports configuration” refactor actually stick across store lifecycle.
packages/ucd-store-v2/src/core/manifest.ts (1)
10-34: Update JSDoc to describe the actual manifest structure.The JSDoc still states the manifest "maps each version string to itself," but the code writes
{ version: { expectedFiles: [] } }. Update the description to match the actual shape. Parent directories are created automatically by the Node.js bridge implementation, so the note about directory existence is unnecessary./** - * Write a UCD store manifest file listing the provided versions. - * - * The manifest is a simple JSON object that maps each version string to - * itself. This function serializes the manifest and writes it to the - * specified manifestPath using the provided filesystem bridge. + * Write a UCD store manifest file. + * + * The manifest is a JSON object mapping each version string to a config object + * containing version metadata (e.g., `{ version: { expectedFiles: [] } }`). * * @param {FileSystemBridge} fs - Filesystem bridge to use for writing * @param {string} manifestPath - Path where manifest should be written * @param {UCDStoreManifest} manifest - Manifest object mapping version strings * @returns {Promise<void>} A promise that resolves once the manifest has been written * @throws {Error} If the filesystem bridge does not support writing or if the write operation fails */packages/ucd-store-v2/test/core/manifest.test.ts (1)
45-60: Test name and scope are misleading—verify directory creation works with Node.js FS bridge.The test "should create parent directories if they don't exist" only validates the memory FS behavior, where directories are implicit in the Map structure. The Node.js FS bridge handles parent directory creation inside its
write()method (packages/fs-bridge/src/bridges/node.ts:119-121), but this test doesn't verify that behavior. Add an integration test using NodeFileSystemBridge with a deeply nested manifest path, or document in the writeManifest JSDoc that parent directories are created by the underlying bridge's write implementation.
🧹 Nitpick comments (4)
.github/workflows/refresh-manifest.yaml (1)
14-17: Document sync requirement between inputs and matrix more explicitly.The "Keep in sync" comment on line 14 documents a manual synchronization point between the
inputs.base_url.options(lines 16–17) and thematrix.base_urlarray on line 29. While the arrays currently match, manual sync comments are fragile and easy to miss during future updates.Consider extracting the base URL list to a YAML anchor or env variable to eliminate duplication:
env: BASE_URLS: '["https://api.ucdjs.dev", "https://preview.api.ucdjs.dev"]' # Then use ${{ env.BASE_URLS }} in both placesAlternatively, document more explicitly why synchronization is necessary and add a pre-commit hook or CI check to detect drift.
packages/schemas/src/fs.ts (1)
4-16: Schema change looks correct; consider extracting a manifest-entry schema to reuse + simplify defaults.Optional cleanup for readability/reuse (and to avoid double-defaulting):
+const UCDStoreManifestEntrySchema = z.object({ + /** + * List of expected file paths for this version. + * Defaults to an empty array when not provided. + */ + expectedFiles: z.array(z.string()).default([]), +}); + export const UCDStoreManifestSchema = z.record( z.string(), - z - .object({ - /** - * List of expected file paths for this version. - * Defaults to an empty array when not provided. - */ - expectedFiles: z.array(z.string()).default([]), - }) - .default({ - expectedFiles: [], - }), + UCDStoreManifestEntrySchema, ).meta({Also applies to: 19-31
packages/ucd-store-v2/test/operations/sync.test.ts (1)
31-35: Optional: reduce repetition with a smallmanifestFromVersions()helper.+const manifestFromVersions = (versions: string[]) => + Object.fromEntries(versions.map((v) => [v, { expectedFiles: [] }]));Then reuse it for
initialFilesand expectedJSON.stringify(...).Also applies to: 59-65, 75-80, 106-112, 122-127, 154-159, 169-173, 222-227, 237-242, 263-267, 276-281, 300-305
packages/ucd-store/test/core/init.test.ts (1)
218-232: Consider distinguishing “invalid JSON” vs “schema mismatch” in v1 store error messaging (test currently encodes the old wording).Right now the test asserts
"store manifest is not a valid JSON"for a schema mismatch ([]). If you decide to align v1 with v2’s clearer"does not match expected schema", update this test accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/refresh-manifest.yaml(2 hunks)apps/api/src/routes/.well-known/openapi.ts(1 hunks)apps/api/src/routes/v1_files/openapi.ts(2 hunks)apps/api/src/routes/v1_files/routes.ts(1 hunks)apps/api/src/routes/v1_versions/openapi.ts(2 hunks)packages/schemas/src/fs.ts(1 hunks)packages/test-utils/src/mock-store/handlers/files.ts(1 hunks)packages/test-utils/test/mock-store/mock-store.test.ts(2 hunks)packages/ucd-store-v2/src/core/manifest.ts(1 hunks)packages/ucd-store-v2/src/operations/sync.ts(1 hunks)packages/ucd-store-v2/src/setup/bootstrap.ts(1 hunks)packages/ucd-store-v2/src/store.ts(1 hunks)packages/ucd-store-v2/test/core/manifest.test.ts(13 hunks)packages/ucd-store-v2/test/operations/sync.test.ts(12 hunks)packages/ucd-store-v2/test/setup/bootstrap.test.ts(1 hunks)packages/ucd-store-v2/test/setup/verify.test.ts(6 hunks)packages/ucd-store/src/store.ts(1 hunks)packages/ucd-store/test/core/init.test.ts(7 hunks)packages/ucd-store/test/file-operations/file-paths.test.ts(3 hunks)packages/ucd-store/test/file-operations/file-tree.test.ts(10 hunks)packages/ucd-store/test/file-operations/get-file.test.ts(4 hunks)packages/ucd-store/test/maintenance/analyze.test.ts(11 hunks)packages/ucd-store/test/maintenance/clean.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{test,spec}.?(c|m)[jt]s?(x)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.?(c|m)[jt]s?(x): Use Vitest for all tests with workspace-aware configuration
Test utilities must be imported from #test-utils/* aliases instead of @ucdjs/test-utils package to avoid build step requirements and cyclic dependency issues
Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API
Use testdir() from vitest-testdirs to create temporary test directories for filesystem testing
Run tests from the repository root using vitest run --project= instead of changing directory to individual packages
Files:
packages/ucd-store/test/maintenance/clean.test.tspackages/test-utils/test/mock-store/mock-store.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store-v2/test/setup/verify.test.tspackages/ucd-store-v2/test/core/manifest.test.tspackages/ucd-store-v2/test/setup/bootstrap.test.tspackages/ucd-store/test/core/init.test.ts
apps/api/src/routes/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Organize routes by version (v1_files, v1_versions) in the Hono API
Files:
apps/api/src/routes/v1_versions/openapi.tsapps/api/src/routes/v1_files/openapi.tsapps/api/src/routes/v1_files/routes.ts
apps/api/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
apps/api/src/**/*.ts: Auto-generate OpenAPI spec from Zod schemas using @hono/zod-openapi in the API app
Always runpnpm build:openapiafter changing API routes or Zod schemas to regenerate the OpenAPI spec
Serve Scalar API documentation at the root (/) and organize API docs site using Nuxt/Docus
Files:
apps/api/src/routes/v1_versions/openapi.tsapps/api/src/routes/v1_files/openapi.tsapps/api/src/routes/v1_files/routes.ts
packages/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement file system bridge pattern using @ucdjs/fs-bridge to support multiple storage backends (Node.js, HTTP, in-memory)
Files:
packages/ucd-store/src/store.tspackages/ucd-store-v2/src/store.tspackages/ucd-store-v2/src/setup/bootstrap.tspackages/ucd-store-v2/src/core/manifest.tspackages/schemas/src/fs.tspackages/test-utils/src/mock-store/handlers/files.tspackages/ucd-store-v2/src/operations/sync.ts
packages/test-utils/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Set up MSW (Mock Service Worker) globally via test-utils for HTTP mocking across all tests
Files:
packages/test-utils/src/mock-store/handlers/files.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Learnt from: luxass
Repo: ucdjs/ucd PR: 11
File: .changeset/gold-news-lay.md:2-3
Timestamp: 2025-05-09T11:50:44.881Z
Learning: The ucdjs/ucd project is in beta phase, and uses a more flexible approach to semantic versioning during this pre-1.0 state. Features may use patch bumps rather than strictly following semver conventions.
Learnt from: luxass
Repo: ucdjs/ucd PR: 107
File: apps/api/src/routes/v1_files.ts:18-32
Timestamp: 2025-07-13T09:23:43.820Z
Learning: UNICODE_STABLE_VERSION from luxass/unicode-utils-new refers to a specific Unicode version string (not a dynamic value), and the validation logic in the files API correctly checks the normalized version (after "latest" substitution) against UNICODE_VERSION_METADATA rather than the mappedVersion from resolveUCDVersion().
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/test-utils/test/mock-store/mock-store.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store/src/store.tsapps/api/src/routes/v1_files/openapi.tspackages/ucd-store-v2/test/setup/verify.test.tspackages/ucd-store-v2/test/core/manifest.test.tspackages/ucd-store-v2/test/setup/bootstrap.test.tsapps/api/src/routes/.well-known/openapi.tspackages/ucd-store-v2/src/store.tspackages/ucd-store-v2/src/setup/bootstrap.tspackages/ucd-store-v2/src/core/manifest.tsapps/api/src/routes/v1_files/routes.tspackages/schemas/src/fs.tspackages/ucd-store/test/core/init.test.tspackages/test-utils/src/mock-store/handlers/files.tspackages/ucd-store-v2/src/operations/sync.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/test-utils/test/mock-store/mock-store.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store-v2/test/setup/verify.test.tspackages/ucd-store-v2/test/core/manifest.test.tspackages/ucd-store/test/core/init.test.tspackages/test-utils/src/mock-store/handlers/files.ts
📚 Learning: 2025-08-17T10:06:18.009Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 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/test/maintenance/clean.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/src/store.ts
📚 Learning: 2025-08-17T10:10:19.096Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 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/test/maintenance/clean.test.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Test utilities must be imported from #test-utils/* aliases instead of ucdjs/test-utils package to avoid build step requirements and cyclic dependency issues
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/test-utils/test/mock-store/mock-store.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store-v2/test/setup/verify.test.tspackages/ucd-store-v2/test/core/manifest.test.tspackages/ucd-store-v2/test/setup/bootstrap.test.tspackages/ucd-store/test/core/init.test.ts
📚 Learning: 2025-10-14T07:15:35.199Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 326
File: packages/shared/src/ucd-config.ts:34-58
Timestamp: 2025-10-14T07:15:35.199Z
Learning: In packages/shared/src/ucd-config.ts, the `getDefaultUCDEndpointConfig` function uses a build-time define `__UCD_ENDPOINT_DEFAULT_CONFIG__` that is replaced at build time by the bundler. The `??` operator is evaluated at build time, so the final code contains no runtime branching. Therefore, runtime unit tests for this function are not valuable—the build process itself handles the injection and fallback logic.
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store-v2/test/setup/verify.test.tspackages/ucd-store-v2/test/setup/bootstrap.test.tspackages/ucd-store/test/core/init.test.tspackages/test-utils/src/mock-store/handlers/files.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to packages/*/src/**/*.ts : Implement file system bridge pattern using ucdjs/fs-bridge to support multiple storage backends (Node.js, HTTP, in-memory)
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store/test/file-operations/file-tree.test.tspackages/ucd-store/src/store.tspackages/ucd-store-v2/test/setup/verify.test.tspackages/ucd-store-v2/test/core/manifest.test.tspackages/ucd-store-v2/src/setup/bootstrap.tspackages/ucd-store-v2/src/core/manifest.tsapps/api/src/routes/v1_files/routes.tspackages/schemas/src/fs.tspackages/ucd-store/test/core/init.test.tspackages/test-utils/src/mock-store/handlers/files.tspackages/ucd-store-v2/src/operations/sync.ts
📚 Learning: 2025-07-13T09:23:43.820Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 107
File: apps/api/src/routes/v1_files.ts:18-32
Timestamp: 2025-07-13T09:23:43.820Z
Learning: UNICODE_STABLE_VERSION from luxass/unicode-utils-new refers to a specific Unicode version string (not a dynamic value), and the validation logic in the files API correctly checks the normalized version (after "latest" substitution) against UNICODE_VERSION_METADATA rather than the mappedVersion from resolveUCDVersion().
Applied to files:
packages/ucd-store/test/maintenance/clean.test.tspackages/ucd-store/test/file-operations/get-file.test.tspackages/ucd-store/test/maintenance/analyze.test.tspackages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store/test/file-operations/file-tree.test.tsapps/api/src/routes/v1_files/openapi.tspackages/ucd-store-v2/test/setup/verify.test.tspackages/ucd-store-v2/test/core/manifest.test.tspackages/ucd-store-v2/test/setup/bootstrap.test.tsapps/api/src/routes/v1_files/routes.tspackages/ucd-store/test/core/init.test.tspackages/test-utils/src/mock-store/handlers/files.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to packages/test-utils/src/**/*.ts : Set up MSW (Mock Service Worker) globally via test-utils for HTTP mocking across all tests
Applied to files:
packages/test-utils/test/mock-store/mock-store.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store-v2/test/setup/verify.test.tspackages/test-utils/src/mock-store/handlers/files.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to apps/api/src/routes/**/*.ts : Organize routes by version (v1_files, v1_versions) in the Hono API
Applied to files:
apps/api/src/routes/v1_versions/openapi.tsapps/api/src/routes/.well-known/openapi.tsapps/api/src/routes/v1_files/routes.tspackages/test-utils/src/mock-store/handlers/files.ts
📚 Learning: 2025-06-28T08:01:22.596Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 85
File: packages/fetch/package.json:37-38
Timestamp: 2025-06-28T08:01:22.596Z
Learning: In the ucdjs/ucd project, relative paths in npm scripts within packages (like `../../apps/api/.generated/openapi.json` in packages/fetch/package.json) resolve correctly even when run via Turborepo from the repo root, contrary to initial concerns about working directory changes.
Applied to files:
packages/ucd-store/test/file-operations/file-paths.test.ts
📚 Learning: 2025-06-29T11:20:13.668Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 87
File: packages/worker-shared/tsconfig.build.json:1-4
Timestamp: 2025-06-29T11:20:13.668Z
Learning: In the ucdjs/ucd project, the packages use tsdown instead of tsc for building libraries. The tsconfig.build.json files are primarily for IDE experience and type checking, not for direct compilation, so including "test" directories in these configs doesn't affect build output.
Applied to files:
packages/ucd-store/test/file-operations/file-paths.test.tspackages/ucd-store-v2/test/operations/sync.test.tspackages/ucd-store/test/file-operations/file-tree.test.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to packages/client/src/**/*.ts : The ucdjs/client package types must be auto-generated from the OpenAPI spec, not manually written
Applied to files:
packages/ucd-store-v2/test/operations/sync.test.tsapps/api/src/routes/.well-known/openapi.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to packages/ucdjs-internal/shared/** : ucdjs-internal/shared package may not follow semantic versioning as it is only for internal use, requiring coordinated updates in dependent packages
Applied to files:
packages/ucd-store-v2/test/operations/sync.test.ts
📚 Learning: 2025-08-23T05:20:36.940Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.940Z
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-v2/test/setup/verify.test.tspackages/ucd-store-v2/test/setup/bootstrap.test.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to apps/api/src/**/*.ts : Always run `pnpm build:openapi` after changing API routes or Zod schemas to regenerate the OpenAPI spec
Applied to files:
apps/api/src/routes/.well-known/openapi.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to apps/api/src/**/*.ts : Auto-generate OpenAPI spec from Zod schemas using hono/zod-openapi in the API app
Applied to files:
apps/api/src/routes/v1_files/routes.ts
🧬 Code graph analysis (5)
packages/ucd-store-v2/test/core/manifest.test.ts (2)
packages/ucd-store-v2/src/core/manifest.ts (2)
writeManifest(23-34)readManifest(50-85)packages/ucd-store/src/store.ts (4)
fs(99-101)manifestPath(138-140)versions(130-132)versions(460-469)
packages/ucd-store-v2/src/setup/bootstrap.ts (2)
packages/ucd-store-v2/src/core/manifest.ts (1)
writeManifest(23-34)packages/ucd-store/src/store.ts (4)
fs(99-101)manifestPath(138-140)versions(130-132)versions(460-469)
packages/ucd-store-v2/src/core/manifest.ts (1)
packages/ucd-store/src/store.ts (1)
manifestPath(138-140)
packages/test-utils/src/mock-store/handlers/files.ts (1)
packages/ucd-store/src/store.ts (2)
versions(130-132)versions(460-469)
packages/ucd-store-v2/src/operations/sync.ts (1)
packages/ucd-store-v2/src/core/manifest.ts (1)
writeManifest(23-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (21)
.github/workflows/refresh-manifest.yaml (2)
28-29: Verify GitHub Actions matrix dynamic computation syntax.Line 29 uses GitHub Actions' dynamic matrix feature with a conditional expression inside
fromJson():base_url: ${{ github.event_name == 'schedule' && fromJson('["https://api.ucdjs.dev", "https://preview.api.ucdjs.dev"]') || fromJson(format('["{0}"]', inputs.base_url)) }}This syntax conditionally returns either a two-element array (for scheduled runs) or a single-element array (for manual dispatch). The logic appears sound, but GitHub Actions' matrix interpolation has historically had edge cases with complex expressions.
Please confirm that this dynamic matrix syntax works correctly in your CI environment by verifying that:
- Scheduled runs generate two jobs (one per base_url)
- Manual dispatch runs generate one job with the selected base_url
- The
inputs.base_urlvariable is correctly passed through in dispatch modeIf you encounter matrix expansion issues, consider simplifying by pre-computing the matrix in a separate setup job or using a reusable workflow.
47-97: Verify that manifest generation and upload steps run unconditionally.Per the enriched summary, conditional gating (
if:clauses) on the "generate manifest" and "Create and upload version tar files" steps have been removed. The provided code shows no visibleif:conditions on either step, suggesting they now run unconditionally for all event types (schedule and workflow_dispatch).This aligns with the PR objective to enable automatic manifest refresh and deployment. However, verify that:
- The unconditional execution of both steps does not cause unintended side effects (e.g., unnecessary uploads on failed runs)
- Both steps have appropriate error handling (the tar upload step has error handling; verify manifest generation does too)
- Scheduled runs (2× per week) do not create excessive load or manifest churn
The tar upload section (lines 73–77) correctly uses
${{ matrix.base_url }}to target the appropriate endpoint for each matrix job, which supports the dynamic base_url logic. Confirm post-merge that manifest refreshes occur as expected both on schedule and on manual dispatch.packages/ucd-store/test/file-operations/file-tree.test.ts (1)
35-35: LGTM! Test fixtures updated to new manifest structure.All test fixture data correctly updated from version-string mappings to version-object mappings with
expectedFilesarrays, aligning with the refactored manifest schema.Also applies to: 103-103, 125-125, 155-155, 202-202, 370-370, 390-390, 414-414, 454-454, 505-505
packages/ucd-store/test/file-operations/get-file.test.ts (1)
40-40: LGTM! Test fixtures updated to new manifest structure.All test fixture data correctly updated to use the new object-based manifest entries with
expectedFilesarrays.Also applies to: 60-60, 80-80, 101-101
apps/api/src/routes/v1_files/openapi.ts (2)
52-52: Minor formatting adjustment.The middleware array formatting was adjusted (removed trailing comma context from diff). This is a non-functional change.
63-89: Verify OpenAPI spec regeneration.The example payload correctly demonstrates the new manifest structure where each version maps to an object with
expectedFilesarrays. Ensure the OpenAPI spec was regenerated after these schema changes.As per coding guidelines, always run
pnpm build:openapiafter changing API routes or Zod schemas to regenerate the OpenAPI spec. Please confirm this was done.apps/api/src/routes/.well-known/openapi.ts (1)
17-17: LGTM! Type assertion adjustment.Removing
as constfrom the middleware array loosens the type from a readonly tuple to a regular array. This aligns with similar changes in other route files and has no runtime impact.apps/api/src/routes/v1_versions/openapi.ts (1)
18-18: LGTM! Consistent type assertion changes.Removing
as constfrom middleware arrays in both route definitions aligns with similar changes across other route files. These are non-functional typing adjustments with no runtime impact.Also applies to: 77-77
apps/api/src/routes/v1_files/routes.ts (2)
14-14: LGTM! Clear constant definition.The
STORE_MANIFEST_KEYconstant provides a clear, reusable reference for the manifest object key in the bucket.
16-36: Verify bucket binding and fallback behavior.The refactor from HTML scraping to bucket-based manifest retrieval is a significant improvement. However, several aspects need verification:
Empty manifest fallback (line 27): Returning an empty manifest
{}when the object is not found may silently mask configuration issues. Consider returning a 404 or 503 instead, or ensure this is the intended behavior for new deployments.Cache duration: The 1-hour cache (
max-age=3600) is significantly shorter than the 7-day cache used elsewhere. Verify this is appropriate for manifest data.Bucket binding: Ensure the
UCD_BUCKETbinding is properly configured in all deployment environments (dev, staging, production).#!/bin/bash # Verify bucket binding configuration and cache consistency # Check for bucket binding references in wrangler/deployment configs rg -nP '\bUCD_BUCKET\b' -g 'wrangler.toml' -g '*.config.{ts,js}' -g '.env*' # Check cache duration constants for consistency rg -nP 'MAX_AGE.*SECONDS|max-age=\d+' apps/api/src/packages/ucd-store/test/file-operations/file-paths.test.ts (1)
34-34: LGTM! Test fixtures updated to new manifest structure.All test fixture data correctly updated to use the new object-based manifest entries with
expectedFilesarrays.Also applies to: 66-66, 92-92
packages/ucd-store-v2/src/setup/bootstrap.ts (1)
67-69: LGTM! Bootstrap logic updated for new manifest structure.The manifest writing now correctly creates a version-to-object mapping where each version initializes with an empty
expectedFilesarray. This aligns with the updatedwriteManifestsignature inpackages/ucd-store-v2/src/core/manifest.tsthat expects aUCDStoreManifestobject instead of a version array.packages/test-utils/src/mock-store/handlers/files.ts (1)
68-71: Mock manifest shape update matches new schema.packages/test-utils/test/mock-store/mock-store.test.ts (1)
338-354: Tests updated correctly for{ expectedFiles }manifest entries.Also applies to: 357-373
packages/ucd-store-v2/test/setup/bootstrap.test.ts (1)
214-217: Manifest structure assertion updated appropriately.packages/ucd-store-v2/test/setup/verify.test.ts (1)
27-31: Verify test fixtures updated correctly for new manifest shape.Also applies to: 50-53, 74-79, 96-101, 141-144, 163-166
packages/ucd-store/test/maintenance/clean.test.ts (1)
199-204: Manifest JSON expectations updated and still follow test-utils import conventions.Also applies to: 253-258, 277-281
packages/ucd-store/test/maintenance/analyze.test.ts (1)
78-81: All analyze fixtures/mocks aligned to new manifest entry object shape.Also applies to: 121-124, 162-166, 200-204, 236-239, 267-270, 298-302, 360-362, 412-416, 446-449, 511-514
packages/ucd-store-v2/test/operations/sync.test.ts (1)
31-35: Good migration of sync tests to the new manifest shape.Also applies to: 59-65, 75-80, 106-112, 122-127, 154-159, 169-173, 222-227, 237-242, 263-267, 276-281, 300-305
packages/ucd-store/test/core/init.test.ts (1)
15-19: Test updates are consistent with the new manifest schema.Also applies to: 156-160, 180-184, 218-232, 259-263, 275-281, 283-289, 304-310, 312-317, 329-336, 338-345, 356-363
packages/ucd-store-v2/test/core/manifest.test.ts (1)
15-27: LGTM: coverage is updated and matches the new manifest API.Also applies to: 34-44, 50-60, 67-76, 82-103, 128-133, 140-151, 156-181, 247-259, 265-279, 288-299, 305-321, 323-329, 341-348
Updated the manifest fetching logic to return early if the object is not found, preventing unnecessary errors. This change improves error handling and ensures that the application behaves correctly when a manifest for a specific version is unavailable.
Modified the `sync` function to ensure that when writing the manifest, it retains the `expectedFiles` for existing versions instead of overwriting them with empty arrays. This change enhances the integrity of the manifest data during synchronization.
… handling Introduces a new function `readManifestOrDefault` that attempts to read the UCD store manifest from disk. If the read operation fails due to reasons such as a missing file, invalid JSON, or schema validation failure, it returns a provided default manifest. This enhances the robustness of manifest handling in the application. Additionally, updates the `handleVersionConflict` function to utilize `readManifestOrDefault`, ensuring that it can gracefully handle scenarios where the manifest may not be available.
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 (1)
.github/workflows/refresh-manifest.yaml (1)
9-17: Add validation or make base_url required for manual triggers.The
base_urlinput is markedrequired: false, but the matrix expression on line 29 provides no fallback when it's not supplied during a manual workflow dispatch. If a user manually triggers the workflow without selecting abase_url,github.event.inputs.base_urlwill be an empty string, causing the matrix to evaluate to[""]and fail at the HTTP request step.Either make the input
required: trueor add defensive logic to ensure a valid URL is selected before the workflow proceeds.Consider this fix:
base_url: description: 'URL to trigger manifest refresh' - required: false + required: true type: choice # NOTE: Keep in sync with matrix.base_url below options: - 'https://api.ucdjs.dev' - 'https://preview.api.ucdjs.dev'Alternatively, provide a default option:
base_url: description: 'URL to trigger manifest refresh' required: false type: choice # NOTE: Keep in sync with matrix.base_url below + default: 'https://api.ucdjs.dev' options: - 'https://api.ucdjs.dev' - 'https://preview.api.ucdjs.dev'
🧹 Nitpick comments (2)
packages/ucd-store-v2/src/core/manifest.ts (1)
19-19: Complete the JSDoc description.The parameter description on line 19 is incomplete: "Manifest object mapping version strings" doesn't specify what versions map to. Consider completing it to clarify the manifest structure.
Apply this diff to clarify the JSDoc:
- * @param {UCDStoreManifest} manifest - Manifest object mapping version strings + * @param {UCDStoreManifest} manifest - Manifest object mapping version strings to configuration objectsapps/api/src/routes/v1_files/routes.ts (1)
51-53: Consider logging when manifest objects are missing.Silently skipping missing manifest objects with early return reduces observability. Adding a console.error or console.warn here would help diagnose why expected versions aren't appearing in the aggregated manifest.
Apply this diff:
if (!object) { + console.error(`[v1_files]: manifest not found for version ${version} at key ${key}`); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/refresh-manifest.yaml(2 hunks)apps/api/src/routes/v1_files/routes.ts(1 hunks)packages/ucd-store-v2/src/core/manifest.ts(2 hunks)packages/ucd-store-v2/src/operations/sync.ts(1 hunks)packages/ucd-store-v2/src/store.ts(3 hunks)packages/ucd-store-v2/test/core/manifest.test.ts(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ucd-store-v2/src/operations/sync.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/api/src/routes/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Organize routes by version (v1_files, v1_versions) in the Hono API
Files:
apps/api/src/routes/v1_files/routes.ts
apps/api/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
apps/api/src/**/*.ts: Auto-generate OpenAPI spec from Zod schemas using @hono/zod-openapi in the API app
Always runpnpm build:openapiafter changing API routes or Zod schemas to regenerate the OpenAPI spec
Serve Scalar API documentation at the root (/) and organize API docs site using Nuxt/Docus
Files:
apps/api/src/routes/v1_files/routes.ts
packages/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement file system bridge pattern using @ucdjs/fs-bridge to support multiple storage backends (Node.js, HTTP, in-memory)
Files:
packages/ucd-store-v2/src/store.tspackages/ucd-store-v2/src/core/manifest.ts
**/*.{test,spec}.?(c|m)[jt]s?(x)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.?(c|m)[jt]s?(x): Use Vitest for all tests with workspace-aware configuration
Test utilities must be imported from #test-utils/* aliases instead of @ucdjs/test-utils package to avoid build step requirements and cyclic dependency issues
Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API
Use testdir() from vitest-testdirs to create temporary test directories for filesystem testing
Run tests from the repository root using vitest run --project= instead of changing directory to individual packages
Files:
packages/ucd-store-v2/test/core/manifest.test.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Learnt from: luxass
Repo: ucdjs/ucd PR: 11
File: .changeset/gold-news-lay.md:2-3
Timestamp: 2025-05-09T11:50:44.881Z
Learning: The ucdjs/ucd project is in beta phase, and uses a more flexible approach to semantic versioning during this pre-1.0 state. Features may use patch bumps rather than strictly following semver conventions.
Learnt from: luxass
Repo: ucdjs/ucd PR: 107
File: apps/api/src/routes/v1_files.ts:18-32
Timestamp: 2025-07-13T09:23:43.820Z
Learning: UNICODE_STABLE_VERSION from luxass/unicode-utils-new refers to a specific Unicode version string (not a dynamic value), and the validation logic in the files API correctly checks the normalized version (after "latest" substitution) against UNICODE_VERSION_METADATA rather than the mappedVersion from resolveUCDVersion().
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Applied to files:
apps/api/src/routes/v1_files/routes.tspackages/ucd-store-v2/src/store.tspackages/ucd-store-v2/src/core/manifest.tspackages/ucd-store-v2/test/core/manifest.test.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to apps/api/src/routes/**/*.ts : Organize routes by version (v1_files, v1_versions) in the Hono API
Applied to files:
apps/api/src/routes/v1_files/routes.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to packages/*/src/**/*.ts : Implement file system bridge pattern using ucdjs/fs-bridge to support multiple storage backends (Node.js, HTTP, in-memory)
Applied to files:
apps/api/src/routes/v1_files/routes.tspackages/ucd-store-v2/src/store.tspackages/ucd-store-v2/test/core/manifest.test.ts
📚 Learning: 2025-07-13T09:23:43.820Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 107
File: apps/api/src/routes/v1_files.ts:18-32
Timestamp: 2025-07-13T09:23:43.820Z
Learning: UNICODE_STABLE_VERSION from luxass/unicode-utils-new refers to a specific Unicode version string (not a dynamic value), and the validation logic in the files API correctly checks the normalized version (after "latest" substitution) against UNICODE_VERSION_METADATA rather than the mappedVersion from resolveUCDVersion().
Applied to files:
apps/api/src/routes/v1_files/routes.tspackages/ucd-store-v2/test/core/manifest.test.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to apps/api/src/**/*.ts : Auto-generate OpenAPI spec from Zod schemas using hono/zod-openapi in the API app
Applied to files:
apps/api/src/routes/v1_files/routes.ts
📚 Learning: 2025-05-04T11:52:22.858Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
Applied to files:
packages/ucd-store-v2/src/store.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to **/scripts/**/*.ts : Use tsx --import ucdjs/moonbeam/register for running build scripts that import workspace packages
Applied to files:
packages/ucd-store-v2/src/store.ts
📚 Learning: 2025-08-17T10:06:18.009Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 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-v2/src/core/manifest.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Test utilities must be imported from #test-utils/* aliases instead of ucdjs/test-utils package to avoid build step requirements and cyclic dependency issues
Applied to files:
packages/ucd-store-v2/test/core/manifest.test.ts
📚 Learning: 2025-08-23T05:20:36.940Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 209
File: packages/ucd-store/test/clean.test.ts:9-9
Timestamp: 2025-08-23T05:20:36.940Z
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-v2/test/core/manifest.test.ts
🧬 Code graph analysis (1)
packages/ucd-store-v2/test/core/manifest.test.ts (2)
packages/ucd-store-v2/src/core/manifest.ts (3)
writeManifest(23-34)readManifest(50-85)readManifestOrDefault(100-109)packages/ucd-store/src/store.ts (4)
fs(99-101)manifestPath(138-140)versions(130-132)versions(460-469)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (windows-latest)
- GitHub Check: build-apps
🔇 Additional comments (8)
.github/workflows/refresh-manifest.yaml (1)
14-14: Sync-note comments improve maintainability.The synchronization comments linking
inputs.base_url.options(line 14) andmatrix.base_url(line 28) are well-placed and help prevent drift between the two. The matrix expression correctly usesgithub.event.inputs.base_urlto access the workflow dispatch input, addressing the concern from the previous review.Also applies to: 28-29
packages/ucd-store-v2/src/store.ts (1)
173-176: LGTM! Manifest preservation correctly implemented.The merge and overwrite paths now properly preserve existing per-version configuration by reading the current manifest and only defaulting to
{ expectedFiles: [] }for versions not already present. This addresses the data loss concern raised in the previous review.Also applies to: 181-184
packages/ucd-store-v2/src/core/manifest.ts (1)
87-109: LGTM! Clean fallback implementation.The new
readManifestOrDefaultfunction provides a safe way to read manifests with a default fallback. The implementation is clean, the JSDoc is comprehensive, and it follows a common error-handling pattern.packages/ucd-store-v2/test/core/manifest.test.ts (1)
6-6: LGTM! Comprehensive test coverage for new manifest structure.The test suite has been thoroughly updated to reflect the new manifest object shape. Key improvements:
- All
writeManifestandreadManifesttests correctly use and expect the{ expectedFiles: [] }structure- New
readManifestOrDefaulttest suite provides excellent coverage of success cases, error fallbacks, and edge cases- Integration tests verify round-trip behavior with the new structure
- Test patterns are consistent throughout
Also applies to: 15-17, 22-26, 281-402, 411-413, 420-420
apps/api/src/routes/v1_files/routes.ts (4)
16-21: LGTM! Proper infrastructure validation.The bucket binding check with 502 response and error logging is the correct way to handle missing infrastructure dependencies.
26-29: Verify the empty manifest handling strategy.Returning 200 with an empty manifest when no versions are found could mask configuration issues or incomplete deployments. Consider whether a 503 (Service Unavailable) might be more appropriate to signal that the service isn't fully initialized, or confirm this graceful degradation is intentional.
69-71: Verify the cache duration aligns with manifest update frequency.The manifest cache is set to 1 hour (3600 seconds), while file content uses 7 days (MAX_AGE_ONE_WEEK_SECONDS). Confirm this shorter cache duration is intentional based on how frequently manifests are expected to be updated.
16-78: Update the route definition to document theLast-Modifiedresponse header, then regenerate the OpenAPI spec.The handler now returns a
Last-Modifiedheader based on the latest manifest upload time, but this header is not documented in the route definition atapps/api/src/routes/v1_files/openapi.ts. Add aheaderssection to the 200 response ofGET_UCD_STOREto document this header, then runpnpm build:openapito regenerate the spec.
Updated the method of extracting the relative path from the object key by using `slice` instead of `replace`. This change enhances clarity and performance when handling version directories under the `manifest/` prefix.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Added logic to fetch the current store manifest and compare local versions with remote versions. Implemented a summary report for uploaded, skipped, and failed versions, improving visibility into the manifest refresh process.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api/src/routes/v1_files/routes.ts (1)
55-57: Add schema validation for bucket manifest data.The JSON data from the bucket is parsed without runtime validation. The type assertion
object.json<UCDStoreManifest[typeof version]>()only provides compile-time typing, not runtime guarantees. Consider using Zod validation to ensure the data matches the expected schema before serving it.
🧹 Nitpick comments (2)
apps/api/src/routes/v1_files/routes.ts (1)
23-29: Consider handling pagination forbucket.list()results.R2's
list()method returns up to 1000 objects by default. If the number of manifest versions grows, some versions could be silently omitted. ChecklistResult.truncatedand iterate with cursors if needed.- const listResult = await bucket.list({ prefix: STORE_MANIFEST_PREFIX }); + const objects: R2Object[] = []; + let cursor: string | undefined; + do { + const listResult = await bucket.list({ prefix: STORE_MANIFEST_PREFIX, cursor }); + objects.push(...listResult.objects); + cursor = listResult.truncated ? listResult.cursor : undefined; + } while (cursor); - if (!listResult.objects.length) { + if (!objects.length) { console.error("[v1_files]: no manifest versions found in bucket"); return c.json({} satisfies UCDStoreManifest, 200); }Then update the loop at line 34 to iterate over
objectsinstead oflistResult.objects.packages/ucd-store-v2/src/store.ts (1)
162-168: Consider passing the manifest object to eliminate redundant reads.The manifest is already read on line 91, but
handleVersionConflictre-reads it on lines 172 and 180. You could pass the full manifest object as a parameter instead of justmanifestVersionsto avoid the redundant filesystem access.Example signature change:
export async function handleVersionConflict( strategy: VersionConflictStrategy, providedVersions: string[], - manifestVersions: string[], + existingManifest: UCDStoreManifest, fs: FileSystemBridge, manifestPath: string, ): Promise<string[]> {Then use
existingManifestdirectly instead of callingreadManifestOrDefault.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/refresh-manifest.yaml(4 hunks)apps/api/src/routes/v1_files/routes.ts(1 hunks)packages/ucd-store-v2/src/store.ts(2 hunks)packages/ucd-store-v2/src/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ucd-store-v2/src/types.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/api/src/routes/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Organize routes by version (v1_files, v1_versions) in the Hono API
Files:
apps/api/src/routes/v1_files/routes.ts
apps/api/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
apps/api/src/**/*.ts: Auto-generate OpenAPI spec from Zod schemas using @hono/zod-openapi in the API app
Always runpnpm build:openapiafter changing API routes or Zod schemas to regenerate the OpenAPI spec
Serve Scalar API documentation at the root (/) and organize API docs site using Nuxt/Docus
Files:
apps/api/src/routes/v1_files/routes.ts
packages/*/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement file system bridge pattern using @ucdjs/fs-bridge to support multiple storage backends (Node.js, HTTP, in-memory)
Files:
packages/ucd-store-v2/src/store.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Use mockStoreApi() utility from #test-utils/mock-store for MSW-based mocking of the UCD API
Learnt from: luxass
Repo: ucdjs/ucd PR: 11
File: .changeset/gold-news-lay.md:2-3
Timestamp: 2025-05-09T11:50:44.881Z
Learning: The ucdjs/ucd project is in beta phase, and uses a more flexible approach to semantic versioning during this pre-1.0 state. Features may use patch bumps rather than strictly following semver conventions.
📚 Learning: 2025-06-09T05:10:32.105Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 45
File: packages/ucd-store/src/download.ts:24-24
Timestamp: 2025-06-09T05:10:32.105Z
Learning: In the ucd-store package refactor, picomatch was moved from direct usage in download.ts to internal usage within the createPathFilter function in filter.ts. The pattern format is still picomatch-compatible, so JSDoc comments referencing picomatch pattern format remain correct.
Applied to files:
apps/api/src/routes/v1_files/routes.tspackages/ucd-store-v2/src/store.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to apps/api/src/routes/**/*.ts : Organize routes by version (v1_files, v1_versions) in the Hono API
Applied to files:
apps/api/src/routes/v1_files/routes.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to packages/*/src/**/*.ts : Implement file system bridge pattern using ucdjs/fs-bridge to support multiple storage backends (Node.js, HTTP, in-memory)
Applied to files:
apps/api/src/routes/v1_files/routes.tspackages/ucd-store-v2/src/store.ts
📚 Learning: 2025-07-13T09:23:43.820Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 107
File: apps/api/src/routes/v1_files.ts:18-32
Timestamp: 2025-07-13T09:23:43.820Z
Learning: UNICODE_STABLE_VERSION from luxass/unicode-utils-new refers to a specific Unicode version string (not a dynamic value), and the validation logic in the files API correctly checks the normalized version (after "latest" substitution) against UNICODE_VERSION_METADATA rather than the mappedVersion from resolveUCDVersion().
Applied to files:
apps/api/src/routes/v1_files/routes.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to apps/api/src/**/*.ts : Auto-generate OpenAPI spec from Zod schemas using hono/zod-openapi in the API app
Applied to files:
apps/api/src/routes/v1_files/routes.ts
📚 Learning: 2025-05-04T11:52:22.858Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 6
File: packages/cli/src/cmd/codegen/fields.ts:68-82
Timestamp: 2025-05-04T11:52:22.858Z
Learning: User prefers to address the issue with Node.js API compatibility (`Dirent.parentPath` not being part of Node's API and the `recursive: true` option for `readdir` being only available in Node 21+) in a separate PR rather than the current one.
Applied to files:
packages/ucd-store-v2/src/store.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to **/scripts/**/*.ts : Use tsx --import ucdjs/moonbeam/register for running build scripts that import workspace packages
Applied to files:
packages/ucd-store-v2/src/store.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to **/*.{test,spec}.?(c|m)[jt]s?(x) : Test utilities must be imported from #test-utils/* aliases instead of ucdjs/test-utils package to avoid build step requirements and cyclic dependency issues
Applied to files:
packages/ucd-store-v2/src/store.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to packages/client/src/**/*.ts : The ucdjs/client package types must be auto-generated from the OpenAPI spec, not manually written
Applied to files:
packages/ucd-store-v2/src/store.ts
📚 Learning: 2025-07-20T05:37:40.565Z
Learnt from: luxass
Repo: ucdjs/ucd PR: 131
File: tooling/eslint-plugin/package.json:0-0
Timestamp: 2025-07-20T05:37:40.565Z
Learning: In the ucdjs/ucd project, internal tooling packages (private packages in the tooling/ directory) export TypeScript files directly without requiring a build step, unlike published packages which use tsdown to build and export from ./dist/. Examples include ucdjs/tsdown-config and ucdjs/eslint-plugin.
Applied to files:
packages/ucd-store-v2/src/store.ts
📚 Learning: 2025-12-08T02:24:42.703Z
Learnt from: CR
Repo: ucdjs/ucd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T02:24:42.703Z
Learning: Applies to packages/*/tsconfig.json : Use ucdjs-tooling/tsconfig for TypeScript configuration in all packages
Applied to files:
packages/ucd-store-v2/src/store.ts
🧬 Code graph analysis (2)
apps/api/src/routes/v1_files/routes.ts (4)
apps/api/src/routes/v1_files/openapi.ts (1)
GET_UCD_STORE(43-103)apps/api/src/lib/errors.ts (1)
badGateway(203-224)packages/schemas/src/fs.ts (1)
UCDStoreManifest(38-38)packages/ucd-store/src/store.ts (2)
versions(130-132)versions(460-469)
packages/ucd-store-v2/src/store.ts (1)
packages/ucd-store-v2/src/core/manifest.ts (2)
readManifestOrDefault(100-109)writeManifest(23-34)
🪛 GitHub Check: zizmor
.github/workflows/refresh-manifest.yaml
[warning] 58-58:
code injection via template expansion
[warning] 59-59:
code injection via template expansion
[warning] 135-135:
code injection via template expansion
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: build-apps
🔇 Additional comments (11)
.github/workflows/refresh-manifest.yaml (5)
28-29: Matrix configuration correctly addresses workflow_dispatch routing.The conditional logic properly uses
github.event.inputs.base_urland correctly routes schedule events to both URLs and workflow_dispatch to the selected URL. Past concerns about matrix configuration have been resolved.
55-60: Fetch step correctly handles manifest retrieval with graceful fallback.The new step appropriately fetches the current manifest before upload, with
|| echo "{}"providing graceful degradation if the fetch fails. This enables the subsequent checksum comparison logic.However, static analysis flags code injection via template expansion on lines 58-59. While the risk is minimal because
matrix.base_urlcomes from a constrained choice list, consider using explicit quoting for defense-in-depth:- curl -s "${{ matrix.base_url }}/api/v1/files/.ucd-store.json" -o current-store.json || echo "{}" > current-store.json + curl -s '${{ matrix.base_url }}/api/v1/files/.ucd-store.json' -o current-store.json || echo "{}" > current-store.jsonThis makes the intent explicit and helps static analysis tools understand the values are not user-controlled.
84-97: Verify SHA256 comparison strategy aligns with manifest structure.The checksum comparison logic uses canonical JSON (
jq -cS) to determine if a version needs uploading. However, the strategy comparesmanifest.jsoncontent against the remote stored version object. Per the PR context, the manifest shape has changed to map versions to objects withexpectedFilesarrays.Confirm that:
- The local generated
manifest.jsonstructure matches the remote stored version object structure (including the newexpectedFilesfield).- No transformations are applied to the manifest between generation and storage that would cause identical content to have different checksums.
- The comparison correctly handles cases where only
expectedFileshas changed.If there are structural misalignments, the skip logic may fail to detect true changes or skip legitimate uploads.
128-173: Comprehensive job summary reporting improves workflow visibility.The GitHub Actions job summary is well-structured with clear status counts, conditional sections for details, and proper markdown formatting. This significantly improves visibility into manifest refresh operations.
Note: Static analysis also flags code injection on line 135 (
${{ matrix.base_url }}). Apply the same quoting fix suggested for line 58 to maintain consistency:- echo "**Target:** \`${{ matrix.base_url }}\`" + echo "**Target:** \`${{ matrix.base_url }}\`"Actually, since this is markdown output (not shell interpolation), the quoting is not applicable here. The template expansion is safe in this context. You may want to add a
# shellcheck disable=SC2086comment if shellcheck reports warnings.
175-180: Error handling strategy is sound.The workflow correctly fails only when uploads actually fail, not when versions are skipped due to matching checksums. This distinction allows the workflow to process versions incrementally without false negatives.
apps/api/src/routes/v1_files/routes.ts (4)
14-14: LGTM!The constant is well-named and clearly defines the prefix for manifest objects in the bucket.
16-21: LGTM!Good defensive check for the bucket binding with appropriate error logging and 502 response.
31-40: LGTM!The version extraction logic correctly uses
.slice()for prefix removal and properly handles the path structure.
69-77: Verify cache duration consistency with route middleware.The route middleware in
openapi.tssetscacheControl: max-age=${MAX_AGE_ONE_WEEK_SECONDS}(7 days), but this handler explicitly setsCache-Control: public, max-age=3600(1 hour). The handler's header will take precedence. If the 1-hour cache is intentional (since the manifest is aggregated dynamically), consider updating the middleware configuration to match, or add a comment explaining the intentional override.packages/ucd-store-v2/src/store.ts (2)
172-175: Data loss issue resolved correctly.The merge strategy now preserves existing per-version configuration (like
expectedFiles) when combining manifest and provided versions, which addresses the critical data loss concern from the previous review.
180-183: Overwrite strategy correctly preserves per-version config.The overwrite strategy now preserves existing per-version configuration while replacing the version list, which is the correct behavior and addresses the data loss concern.
Updated the `refresh-manifest.yaml` workflow to consistently use the `BASE_URL` environment variable instead of directly referencing `matrix.base_url`. This change improves readability and maintainability of the script.
🔗 Linked issue
resolves #394
📚 Description
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.