feat(fetch): initialize fetch package with API client and documentation#81
feat(fetch): initialize fetch package with API client and documentation#81
Conversation
- Added LICENSE file with MIT License. - Created README.md for package description and installation instructions. - Configured ESLint with @luxass/eslint-config. - Added package.json with metadata, scripts, and dependencies. - Implemented API client in src/index.ts for Unicode API. - Added TypeScript configuration files (tsconfig.json, tsconfig.build.json). - Configured build process with tsdown and Turbo. - Updated pnpm workspace to include new fetch package. - Removed @ucdjs/tsconfig dependency from utils package.
🦋 Changeset detectedLatest commit: 9d70220 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new package, Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client (@ucdjs/fetch)
participant API as Unicode API
participant Proxy as unicode-proxy.ucdjs.dev
Client->>API: GET /api/v1/unicode-proxy/{path...}
API->>Proxy: GET /{path...}
Proxy-->>API: Proxied response (file, directory, or error)
API-->>Client: Returns proxied response or error
sequenceDiagram
participant RouteHandler as API Route Handler
participant ProxyRequest as proxyRequest function
participant Proxy as unicode-proxy.ucdjs.dev
RouteHandler->>ProxyRequest: Call with path segments
ProxyRequest->>Proxy: Fetch proxied content
Proxy-->>ProxyRequest: Response or error
ProxyRequest-->>RouteHandler: Response or error
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/api/src/routes/v1_unicode-proxy.openapi.tsOops! Something went wrong! :( ESLint: 9.28.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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 (
|
- Added a `fetch` method to the client configuration for improved request handling. - Introduced a new test suite for the unicode API client, covering: * Client creation with custom base URLs. * Default client validation. * Successful and error handling for `/api/v1/unicode-versions` and `/api/v1/unicode-proxy` endpoints. * Integration scenarios for concurrent requests and custom client instances.
* Introduced `turbo.json` for both `api` and `proxy` applications. * Configured build, dev, lint, and typecheck tasks for improved development workflow.
* Simplified `.spectral.yml` to extend from `@luxass/spectral-ruleset`. * Added `@luxass/spectral-ruleset` as a dependency in `package.json`, `pnpm-lock.yaml`, and `pnpm-workspace.yaml`.
- Updated the test for handling binary data responses to use a more meaningful Uint8Array. - Modified the client GET request to parse the response as an `arrayBuffer`. - Added validation to ensure the binary data is correctly converted to a string.
* Changed workflow name from `deploy to production` to `deploy to preview` for clarity. * Ensures accurate representation of the deployment environment in the CI/CD process.
- Introduced new routes for proxying requests with up to six path segments. - Enhanced error handling to provide more informative messages for 404 errors. - Updated `package.json` to include `@luxass/utils` as a dependency.
Preview DeploymentsThe workers have been deployed successfully. API Preview URL: https://preview.api.ucdjs.dev This preview was built from commit 9d70220 🤖 This comment will be updated automatically when you push new commits to this PR. |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I will fix the tests when this gets merged into main, since they depend on the production running API. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
.github/workflows/deploy-preview.yaml (1)
137-141: Add a concurrency block to avoid piling up preview deploymentsEvery push to the PR triggers a new preview. Without
concurrency, older runs keep deploying and leave multiple Cloudflare preview versions + comment noise.concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: truePlace this at the top of the workflow or inside each job to auto-cancel superseded runs.
🧹 Nitpick comments (14)
.github/workflows/deploy-preview.yaml (1)
32-40: Matrix construction is clever but brittleThe ternary dance that produces
''placeholders and then excludes them is hard to read and scales poorly when more apps are introduced. A clearer pattern:strategy: matrix: - app: - - ${{ (github.event.inputs.app == 'api' || github.event.inputs.app == 'both' || github.event.inputs.app == null) && 'api' || '' }} - - ${{ (github.event.inputs.app == 'proxy' || github.event.inputs.app == 'both' || github.event.inputs.app == null) && 'proxy' || '' }} - exclude: - - app: '' + include: + - app: api + - app: proxyThen gate the job itself with a simple
if:to pick the desired target. This removes the sentinel value and is easier to extend.pnpm-workspace.yaml (2)
32-32: Add changelog entry for new internal rulesetAdding
@luxass/spectral-rulesetto the linting catalog is fine, but remember to call out the monorepo-wide lint ruleset change in the next release notes so downstream contributors know to install it.
63-64:openapi-typescriptin dev catalog is OKTooling-only dependency; no issues spotted.
Consider pinning to a minor (~7.8.0) if deterministic code-gen output is critical.apps/proxy/turbo.json (1)
5-13: Minor Turbo config polish
dist/**already matches nested folders;dist/**/**is redundant but harmless.- Ensure
tsconfig.jsoninapps/proxyactually setsincremental: trueandtsBuildInfoFile: ".cache/tsbuildinfo.json"; otherwise thetypechecktask’s declared output will never be produced and Turbo will rerun needlessly."typecheck": { - "outputs": [".cache/tsbuildinfo.json"] + "outputs": [".cache/tsbuildinfo.json"] // ensure tsconfig matches }.changeset/spicy-beers-pump.md (1)
5-6: Drop the “feat:” prefix from the changeset summaryChangeset summaries are already grouped by bump-type when they are assembled into the release notes. Keeping the conventional-commit prefix causes duplicate wording such as “### Features – feat: add fetch client”.
- feat: add fetch client +Add fetch clienttooling/tsconfig/base.json (1)
17-18: Keep path mappings alphabetically sorted for quick scanningSlotting the new alias in its lexicographical position (
@ucdjs/fetchcomes before@ucdjs/ucd-store) avoids hunting for entries later.- "@ucdjs/ucd-store": ["./packages/ucd-store/src/index.ts"], - "@ucdjs/fetch": ["./packages/fetch/src/index.ts"], + "@ucdjs/fetch": ["./packages/fetch/src/index.ts"], + "@ucdjs/ucd-store": ["./packages/ucd-store/src/index.ts"],packages/fetch/tsconfig.build.json (1)
1-4: Avoid compiling test files in the production buildIncluding the
testfolder in the build tsconfig bloats the output and lengthens CI times. The roottsconfig.jsonalready covers tests for IDE feedback &vitest; the build config should focus on publishable artefacts only.- "include": ["src/**/*", "*.ts", "test"] + "include": ["src/**/*", "*.ts"] + "exclude": ["test"]packages/fetch/tsconfig.json (1)
1-4: Minor glob tweak for test inclusion
"test"only includes files directly inside the folder. If you ever nest fixtures (e.g.test/utils/helpers.ts) they will be skipped by the language-service.- "include": ["src/**/*", "*.ts", "test"] + "include": ["src/**/*", "*.ts", "test/**/*"]packages/fetch/README.md (1)
6-13: Add a quick-start code snippetA one-liner example demonstrates value instantly:
A simple and efficient fetch client for api.ucdjs.dev ## Installation @@ npm install @ucdjs/fetch+## Usage
+
+```ts
+import { client } from '@ucdjs/fetch'
+
+// Get a code-point
+const { data } = await client.GET('/code-point/{value}', {
- params: { path: { value: '0041' } }
+})+console.log(data.name) // "LATIN CAPITAL LETTER A"
+```packages/fetch/turbo.json (1)
4-18: Add deterministic outputs forgenerate:client
generate:clientcurrently has nooutputs, so Turbo will rerun it on every pipeline execution, defeating caching. Declare its artifacts (e.g.src/generated/**ordist/client/**) to optimise build time."generate:client": { + "outputs": ["src/generated/**"] }apps/api/src/routes/v1_unicode-proxy.ts (1)
49-72: Consider improving path segment concatenation robustness.The current path concatenation using template literals works but could be more robust against edge cases like empty segments or special characters.
Consider using a more robust path joining approach:
- return proxyRequest(c, `${path}/${path2}`); + return proxyRequest(c, [path, path2].filter(Boolean).join('/'));This pattern would handle edge cases better and be more maintainable as the number of segments grows.
packages/fetch/src/index.ts (1)
17-17: Consider if the fetch wrapper is necessary.The fetch wrapper
fetch: (...args) => fetch(...args)appears redundant since openapi-fetch uses the global fetch by default.- fetch: (...args) => fetch(...args),Unless there's a specific reason for this wrapper, it can be removed to simplify the configuration.
packages/fetch/test/index.test.ts (1)
279-279: Improve type safety for response data assertion.The type assertion
(data as any).typebypasses TypeScript's type checking. Consider using a more specific type guard or assertion.- expect((data as any).type).toBe("directory"); + expect(data && typeof data === 'object' && 'type' in data ? data.type : undefined).toBe("directory");Or better yet, if
ProxyResponsetype is available:- expect((data as any).type).toBe("directory"); + expect((data as ProxyResponse).type).toBe("directory");apps/api/src/routes/v1_unicode-proxy.openapi.ts (1)
107-390: Consider using a route generator function for better maintainability.Given the repetitive nature of these routes, consider creating a generator function to create routes with different path segment counts.
function createUnicodeProxyRoute(segmentCount: number) { const pathSegments = Array.from({ length: segmentCount }, (_, i) => `{path${i === 0 ? '' : i + 1}}`); const pathString = `/${pathSegments.join('/')}`; const paramObject = pathSegments.reduce((acc, segment, index) => { const paramName = segment.slice(1, -1); // Remove { } const segmentNumber = index === 0 ? 'first' : ['second', 'third', 'fourth', 'fifth', 'sixth'][index]; acc[paramName] = z.string().describe(`The ${segmentNumber} path segment`); return acc; }, {} as Record<string, any>); return createRoute({ method: "get", path: pathString, tags: ["Unicode Proxy"], description: `Proxy requests to unicode-proxy.ucdjs.dev with ${segmentCount === 1 ? 'one' : ['two', 'three', 'four', 'five', 'six'][segmentCount - 2]} path segment${segmentCount === 1 ? '' : 's'}`, request: { params: z.object(paramObject), }, responses: UNICODE_PROXY_RESPONSES, }); } export const UNICODE_PROXY_ROUTE_2 = createUnicodeProxyRoute(2); export const UNICODE_PROXY_ROUTE_3 = createUnicodeProxyRoute(3); // ... etc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
.changeset/spicy-beers-pump.md(1 hunks).github/labeler.yml(1 hunks).github/workflows/deploy-preview.yaml(1 hunks)apps/api/.spectral.yml(1 hunks)apps/api/package.json(1 hunks)apps/api/src/openapi.ts(2 hunks)apps/api/src/routes/v1_unicode-proxy.openapi.ts(1 hunks)apps/api/src/routes/v1_unicode-proxy.ts(3 hunks)apps/api/turbo.json(1 hunks)apps/proxy/turbo.json(1 hunks)packages/fetch/.gitignore(1 hunks)packages/fetch/LICENSE(1 hunks)packages/fetch/README.md(1 hunks)packages/fetch/eslint.config.js(1 hunks)packages/fetch/package.json(1 hunks)packages/fetch/src/index.ts(1 hunks)packages/fetch/test/index.test.ts(1 hunks)packages/fetch/tsconfig.build.json(1 hunks)packages/fetch/tsconfig.json(1 hunks)packages/fetch/tsdown.config.ts(1 hunks)packages/fetch/turbo.json(1 hunks)packages/utils/package.json(0 hunks)pnpm-workspace.yaml(3 hunks)tooling/tsconfig/base.json(1 hunks)vitest.config.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/utils/package.json
- vitest.config.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: luxass
PR: ucdjs/ucd#20
File: packages/ucd-store/test/index.test.ts:1-7
Timestamp: 2025-05-31T07:44:02.123Z
Learning: User luxass prefers to implement functionality in separate PRs rather than including everything in a single large PR, keeping changes focused and manageable.
Learnt from: luxass
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.
Learnt from: luxass
PR: ucdjs/ucd#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.
.changeset/spicy-beers-pump.md (2)
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.
Learnt from: luxass
PR: ucdjs/ucd#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.
tooling/tsconfig/base.json (1)
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.
apps/api/src/openapi.ts (1)
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.
packages/fetch/tsconfig.json (1)
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.
packages/fetch/README.md (1)
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.
packages/fetch/tsconfig.build.json (1)
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.
packages/fetch/tsdown.config.ts (1)
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.
packages/fetch/eslint.config.js (1)
Learnt from: luxass
PR: ucdjs/ucd#20
File: packages/ucd-store/test/index.test.ts:1-7
Timestamp: 2025-05-31T07:44:02.123Z
Learning: User luxass prefers to implement functionality in separate PRs rather than including everything in a single large PR, keeping changes focused and manageable.
packages/fetch/package.json (2)
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.
Learnt from: luxass
PR: ucdjs/ucd#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.
🧬 Code Graph Analysis (2)
apps/api/src/routes/v1_unicode-proxy.ts (3)
apps/api/src/types.ts (1)
HonoEnv(4-6)apps/api/src/utils.ts (1)
createError(5-15)apps/api/src/routes/v1_unicode-proxy.openapi.ts (7)
ROOT_UNICODE_PROXY_ROUTE(4-51)UNICODE_PROXY_ROUTE(53-105)UNICODE_PROXY_ROUTE_2(107-160)UNICODE_PROXY_ROUTE_3(162-216)UNICODE_PROXY_ROUTE_4(218-273)UNICODE_PROXY_ROUTE_5(275-331)UNICODE_PROXY_ROUTE_6(333-390)
apps/api/src/routes/v1_unicode-proxy.openapi.ts (1)
apps/api/src/routes/v1_unicode-proxy.schemas.ts (1)
ProxyResponseSchema(17-20)
🪛 GitHub Check: codecov/patch
apps/api/src/routes/v1_unicode-proxy.ts
[warning] 10-10: apps/api/src/routes/v1_unicode-proxy.ts#L10
Added line #L10 was not covered by tests
[warning] 13-13: apps/api/src/routes/v1_unicode-proxy.ts#L13
Added line #L13 was not covered by tests
[warning] 41-41: apps/api/src/routes/v1_unicode-proxy.ts#L41
Added line #L41 was not covered by tests
[warning] 46-46: apps/api/src/routes/v1_unicode-proxy.ts#L46
Added line #L46 was not covered by tests
[warning] 50-51: apps/api/src/routes/v1_unicode-proxy.ts#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 55-56: apps/api/src/routes/v1_unicode-proxy.ts#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 60-61: apps/api/src/routes/v1_unicode-proxy.ts#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 65-66: apps/api/src/routes/v1_unicode-proxy.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 70-71: apps/api/src/routes/v1_unicode-proxy.ts#L70-L71
Added lines #L70 - L71 were not covered by tests
🔇 Additional comments (21)
.github/workflows/deploy-preview.yaml (2)
1-1: Double-check external references to the renamed workflowChanging the
name:field only affects the display name, but required-checks, badges, or 3rd-party dashboards might still look for the old “deploy to production” name. Verify branch-protection rules or CI status gates so the PR isn’t blocked by a missing required check.
25-28: 👍 Token scope hardened
permissions: {}removes the defaultGITHUB_TOKENscopes—good security practice.packages/fetch/.gitignore (1)
1-1: Good rule – covers generated artifactsIgnoring any “.generated” file/dir keeps transient code-gen output out of Git and CI artefacts.
No further action needed..github/labeler.yml (1)
11-14: Label looks correct – verify runner picks it upThe key is quoted (required because of the colon) and the glob path is consistent with other package rules.
Once merged, open /actions/labeler run on a test PR touchingpackages/fetch/**to ensure the label is applied.pnpm-workspace.yaml (1)
51-52: ```shell
#!/bin/bash
echo "Searching for engineStrict occurrences:"
rg -n "engineStrict" || echo "None found"echo
echo "Showing root package.json (first 200 lines):"
if [ -f package.json ]; then sed -n '1,200p' package.json; else echo "No root package.json found."; fiecho
echo "Searching for 'engines' fields in all package.json files:"
find . -type f -name package.json | while read f; do
echo "File: $f"
rg -n '"engines"' "$f" || echo "No 'engines' field in $f"
echo
done</details> <details> <summary>apps/api/package.json (2)</summary> `21-21`: **Runtime addition of `@luxass/utils` acknowledged** The util package is already in the prod catalog, so the alias is correct. Make sure tree-shaking removes unused helpers when bundling for Cloudflare Workers to keep bundle size small. --- `29-29`: **Spectral ruleset devDependency – good call** Switching to a shared ruleset will reduce duplication. ✅ </details> <details> <summary>apps/api/src/openapi.ts (2)</summary> `2-2`: **Potential OpenAPI 3.1 vs 3.0 mismatch** `getOpenAPI31Document` (from `@hono/zod-openapi`) expects a 3.1 spec, yet `openapi` is still set to `"3.0.0"`. 3.1 introduces breaking‐change fields (e.g. `components.schemas.*.examples`) that will be silently ignored or produce validation warnings when the version is downgraded. Please double-check that the downstream tooling (client generation, Swagger UI, etc.) handles this mix correctly. ```diff - openapi: "3.0.0", + openapi: "3.1.0",
31-36: Nice readability win withdedentSwitching to a tagged template literal markedly improves the multi-line description’s formatting—good call.
packages/fetch/LICENSE (1)
1-21: License file looks goodMIT text and copyright line are correct.
packages/fetch/eslint.config.js (1)
1-10: ```shell
#!/bin/bash
set -eecho "=== packages/fetch/package.json ==="
if [ -f packages/fetch/package.json ]; then
sed -n '1,200p' packages/fetch/package.json
else
echo "packages/fetch/package.json not found"
fiecho
echo "=== Root package.json ==="
if [ -f package.json ]; then
sed -n '1,200p' package.json
else
echo "package.json not found"
fi</details> <details> <summary>packages/fetch/tsdown.config.ts (1)</summary> `3-14`: **Consider aligning `exports: true` with `package.json`** `exports: true` makes `tsdown` verify the `exports` map, but `package.json` must already contain the relevant sub-paths (`"."`, `./package.json`, etc.). If they’re missing, the build will fail or emit an empty bundle. Quick sanity-check the manifest before merging. </details> <details> <summary>apps/api/turbo.json (1)</summary> `1-19`: **LGTM! Well-structured Turbo configuration.** The configuration follows Turbo best practices with appropriate task definitions, dependency management, and output specifications. </details> <details> <summary>apps/api/.spectral.yml (1)</summary> `1-2`: **Excellent simplification for maintainability.** Replacing the complex custom Spectral configuration with the external `@luxass/spectral-ruleset` centralizes rule management and reduces maintenance overhead. </details> <details> <summary>packages/fetch/package.json (1)</summary> `1-56`: **Well-structured package configuration.** The package.json follows best practices with proper ES module configuration, comprehensive scripts, and appropriate use of catalog references for monorepo dependency management. </details> <details> <summary>apps/api/src/routes/v1_unicode-proxy.ts (2)</summary> `10-38`: **Excellent refactoring that eliminates code duplication.** The `proxyRequest` function effectively consolidates the proxy logic with proper error handling, response forwarding, and caching headers. --- `10-38`: **Test coverage warnings are expected for new code.** The static analysis warnings about test coverage are expected since this is new functionality. Consider adding comprehensive tests for the proxy functionality including error scenarios and edge cases. The learnings indicate that luxass prefers to implement functionality in separate PRs. Would you like me to help create a separate testing PR to address the coverage concerns? </details> <details> <summary>packages/fetch/src/index.ts (3)</summary> `11-19`: **Well-implemented client factory with good documentation.** The `createClient` function is properly typed and documented. The configuration looks appropriate for the Unicode API. --- `33-33`: **Default client configuration is appropriate.** The pre-configured client with the production URL `https://api.ucdjs.dev` provides good developer experience out of the box. --- `35-43`: **Selective type exports are well-chosen.** The exported types cover the essential API components, and the commented-out types suggest thoughtful curation of the public API surface. </details> <details> <summary>packages/fetch/test/index.test.ts (1)</summary> `192-193`: **ESLint disable comment is appropriately used.** The ESLint disable for `node/prefer-global/buffer` is justified here since this is test code specifically testing binary data handling where the Buffer API usage is intentional and necessary. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
* Consolidated route definitions into a single configuration array. * Simplified route registration by iterating over route configurations. * Enhanced maintainability and scalability for future route additions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/src/routes/v1_unicode-proxy.openapi.ts (1)
5-43: Excellent refactoring to eliminate response schema duplication.The extraction of
errorResponseSchemaandcommonResponsessuccessfully addresses the previous review comment about code duplication. The use ofsatisfies RouteConfig["responses"]ensures type safety while maintaining flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/routes/v1_unicode-proxy.openapi.ts(1 hunks)apps/api/src/routes/v1_unicode-proxy.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: luxass
PR: ucdjs/ucd#20
File: packages/ucd-store/test/index.test.ts:1-7
Timestamp: 2025-05-31T07:44:02.123Z
Learning: User luxass prefers to implement functionality in separate PRs rather than including everything in a single large PR, keeping changes focused and manageable.
Learnt from: luxass
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.
Learnt from: luxass
PR: ucdjs/ucd#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.
apps/api/src/routes/v1_unicode-proxy.openapi.ts (1)
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.
🧬 Code Graph Analysis (1)
apps/api/src/routes/v1_unicode-proxy.openapi.ts (1)
apps/api/src/routes/v1_unicode-proxy.schemas.ts (1)
ProxyResponseSchema(17-20)
🪛 GitHub Check: codecov/patch
apps/api/src/routes/v1_unicode-proxy.ts
[warning] 22-22: apps/api/src/routes/v1_unicode-proxy.ts#L22
Added line #L22 was not covered by tests
[warning] 25-25: apps/api/src/routes/v1_unicode-proxy.ts#L25
Added line #L25 was not covered by tests
[warning] 69-69: apps/api/src/routes/v1_unicode-proxy.ts#L69
Added line #L69 was not covered by tests
[warning] 72-73: apps/api/src/routes/v1_unicode-proxy.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
🔇 Additional comments (5)
apps/api/src/routes/v1_unicode-proxy.ts (4)
1-1: LGTM! Clean import organization.The imports are well-organized, clearly separating the route constants for different path segment counts.
Also applies to: 6-18
22-50: Well-implemented proxy function with proper error handling.The
proxyRequestfunction effectively consolidates the proxy logic with appropriate error handling and path-specific error messages.Note: Static analysis indicates missing test coverage for this function. I understand from your PR comment that tests depend on the production API. Consider adding unit tests with mocked fetch responses to improve coverage.
52-64: Excellent data-driven approach for route configuration.The
routeConfigsarray provides a clean, scalable way to define routes supporting multiple path segments. This approach eliminates code duplication and makes it easy to extend or modify route definitions.
66-75: Clean and efficient dynamic route registration.The forEach loop elegantly registers all route handlers using the configuration data. The logic properly handles both the root route (no parameters) and parameterized routes.
apps/api/src/routes/v1_unicode-proxy.openapi.ts (1)
88-98: Clean and scalable route generation.The route exports efficiently leverage the
createProxyRoutehelper function, providing a maintainable way to generate routes for 0-10 path segments.
* Changed `params` type from `any` to `Record<string, z.ZodString>` for better type safety. * Updated path segment descriptions to use ordinal numbers for clarity. * Added additional examples for path segments to enhance documentation.
1 participant
Summary by CodeRabbit
New Features
@ucdjs/fetchpackage, providing a simple and efficient API client for Unicode endpoints.Bug Fixes
Documentation
Chores
Tests
Refactor