feat: add draft version support and update emoji version schema #3
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 0 minutes and 10 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 (4)
WalkthroughThis update disables the version-checking logic in the categories router and enhances the emoji version endpoints. The changes add an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CategoriesRouter
Client->>CategoriesRouter: HTTP Request (e.g. GET /v1/categories?version=invalid)
Note right of CategoriesRouter: Version validation removed
CategoriesRouter-->>Client: Forwards request to next middleware
sequenceDiagram
participant Client
participant VersionsRouter
participant Utils
participant Validator
Client->>VersionsRouter: HTTP GET /v1/draft?draft=true
VersionsRouter->>Utils: getAvailableVersions()
Utils->>Validator: Validate data with EmojiLock schema
Validator-->>Utils: Returns validated emoji versions
alt Draft version found
Utils-->>VersionsRouter: Draft version data
VersionsRouter-->>Client: 200 OK (Draft emoji version)
else No draft version available
VersionsRouter-->>Client: 404 Not Found (ApiErrorSchema)
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Nitpick comments (5)
src/utils.ts (1)
18-34: Consider improving error messages.The schema validation is correctly implemented, but the error messages could be more descriptive.
- throw new Error("failed to fetch emoji data"); + throw new Error(`Failed to fetch emoji data: ${res.status} ${res.statusText}`); - throw new Error("invalid emoji data schema"); + throw new Error(`Invalid emoji data schema: ${result.error.message}`);src/schemas.ts (1)
36-42: Consider reusing theEmojiVersionschema.To maintain consistency and reduce duplication, consider reusing the
EmojiVersionschema in theEMOJI_LOCK_SCHEMA.- versions: z.array( - z.object({ - emoji_version: z.string().nullable(), - unicode_version: z.string().nullable(), - draft: z.boolean(), - }), - ), + versions: z.array(EmojiVersion.omit({ + // Remove OpenAPI metadata as it's not needed in the lock file + })),src/routes/v1_versions.ts (2)
10-18: Improve filtering logic and error handling.Consider these improvements:
- The filtering logic could be more concise using optional chaining.
- Add specific error handling for when no versions are available.
Apply this diff to improve the code:
- const draft = c.req.query("draft"); + const includeDrafts = c.req.query("draft") !== "true"; let versions = await getAvailableVersions(); - if (draft === "true") { - versions = versions.filter((version) => !version.draft); + if (!versions?.length) { + return createError(c, 404, "no versions available"); } + versions = includeDrafts ? versions : versions.filter((version) => !version.draft); return c.json(versions, 200);
44-56: Make the code more concise.The filtering and error handling logic could be more concise using optional chaining and array destructuring.
Apply this diff to improve the code:
V1_VERSIONS_ROUTER.openapi(DRAFT_EMOJI_VERSIONS_ROUTE, async (c) => { try { - const versions = (await getAvailableVersions()).filter((version) => version.draft); - - if (versions[0] == null) { + const [draftVersion] = (await getAvailableVersions())?.filter((version) => version.draft) ?? []; + + if (!draftVersion) { return createError(c, 404, "no draft versions available"); } - return c.json(versions[0], 200); + return c.json(draftVersion, 200); } catch { return createError(c, 500, "failed to fetch emoji data"); } });src/routes/v1_versions.openapi.ts (1)
43-71: Extract common parameter definition.The draft parameter definition is duplicated. Consider extracting it into a reusable constant.
Apply this diff to improve the code:
+const DRAFT_PARAMETER = { + in: "query", + name: "draft", + required: false, + description: "Whether to include draft versions", + schema: { + type: "string", + enum: ["true", "false"], + }, +} as const; export const ALL_EMOJI_VERSIONS_ROUTE = createRoute({ // ... - parameters: [ - { - in: "query", - name: "draft", - required: false, - description: "Whether to include draft versions", - schema: { - type: "boolean", - }, - }, - ], + parameters: [DRAFT_PARAMETER], // ... }); export const LATEST_EMOJI_VERSIONS_ROUTE = createRoute({ // ... - parameters: [ - { - in: "query", - name: "draft", - required: false, - description: "Whether to include draft versions", - schema: { - type: "boolean", - }, - }, - ], + parameters: [DRAFT_PARAMETER], // ... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/routes/v1_categories.ts(2 hunks)src/routes/v1_versions.openapi.ts(2 hunks)src/routes/v1_versions.ts(1 hunks)src/schemas.ts(2 hunks)src/types.ts(2 hunks)src/utils.ts(3 hunks)
🔇 Additional comments (7)
src/types.ts (2)
2-2: LGTM!The import statement is correctly structured and aligns with the new type definition.
14-14: LGTM!The
EmojiLocktype is correctly defined and aligns with the schema changes.src/routes/v1_categories.ts (2)
3-3: LGTM!The import statement is correctly updated to remove unused imports.
15-19:❓ Verification inconclusive
Verify the implications of disabling version validation.
While commenting out the version validation aligns with the PR objectives to support draft versions, please ensure that:
- This is not a temporary change (commented code should be removed if permanent).
- The removal of version validation doesn't lead to unintended acceptance of invalid versions.
Run this script to check for version validation in other routes:
🏁 Script executed:
#!/bin/bash # Description: Check if version validation is handled elsewhere # Look for version validation patterns in route files rg -A 5 'version.*param|param.*version' src/routes/Length of output: 599
Confirm the impact of disabling version validation
Based on our verification, the version validation logic in
src/routes/v1_categories.tshas been commented out without any alternative check elsewhere. The code still rejects requests missing a version parameter, but it no longer verifies whether the provided version (e.g., "latest" or any other value) is valid.Please ensure that:
- If this change is intended for draft version support, remove the commented-out code to avoid confusion.
- The absence of the additional version validation does not open up the possibility of processing unintended or invalid version values.
- If this is a temporary measure, add a clear TODO or FIXME comment to revisit the validation logic before the final release.
src/utils.ts (1)
3-4: LGTM!The import statements are correctly updated to support the new schema validation.
src/schemas.ts (1)
4-16: LGTM!The schema updates are well-structured with:
- Clear descriptions and examples for each property
- Correct nullable types for version fields
- Proper OpenAPI documentation
src/routes/v1_versions.openapi.ts (1)
74-104: LGTM!The route definition is well-structured with appropriate response schemas and error handling.
| try { | ||
| const draft = c.req.query("draft"); | ||
|
|
||
| let versions = await getAvailableVersions(); | ||
|
|
||
| if (draft === "true") { | ||
| versions = versions.filter((version) => !version.draft); | ||
| } | ||
|
|
||
| if (versions[0] == null) { | ||
| return createError(c, 500, "failed to fetch emoji data"); | ||
| } | ||
|
|
||
| if (!res.ok) { | ||
| return c.json(versions[0], 200); | ||
| } catch { | ||
| return createError(c, 500, "failed to fetch emoji data"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication and improve consistency.
The draft parameter handling is duplicated from ALL_EMOJI_VERSIONS_ROUTE. Consider extracting the common logic into a utility function.
Apply this diff to improve the code:
+async function getFilteredVersions(draft: string | null) {
+ const includeDrafts = draft !== "true";
+ const versions = await getAvailableVersions();
+ return includeDrafts ? versions : versions.filter((version) => !version.draft);
+}
V1_VERSIONS_ROUTER.openapi(LATEST_EMOJI_VERSIONS_ROUTE, async (c) => {
try {
- const draft = c.req.query("draft");
-
- let versions = await getAvailableVersions();
-
- if (draft === "true") {
- versions = versions.filter((version) => !version.draft);
- }
+ const versions = await getFilteredVersions(c.req.query("draft"));
if (versions[0] == null) {
return createError(c, 500, "failed to fetch emoji data");
}
return c.json(versions[0], 200);
} catch {
return createError(c, 500, "failed to fetch emoji data");
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const draft = c.req.query("draft"); | |
| let versions = await getAvailableVersions(); | |
| if (draft === "true") { | |
| versions = versions.filter((version) => !version.draft); | |
| } | |
| if (versions[0] == null) { | |
| return createError(c, 500, "failed to fetch emoji data"); | |
| } | |
| if (!res.ok) { | |
| return c.json(versions[0], 200); | |
| } catch { | |
| return createError(c, 500, "failed to fetch emoji data"); | |
| } | |
| async function getFilteredVersions(draft: string | null) { | |
| const includeDrafts = draft !== "true"; | |
| const versions = await getAvailableVersions(); | |
| return includeDrafts ? versions : versions.filter((version) => !version.draft); | |
| } | |
| V1_VERSIONS_ROUTER.openapi(LATEST_EMOJI_VERSIONS_ROUTE, async (c) => { | |
| try { | |
| const versions = await getFilteredVersions(c.req.query("draft")); | |
| if (versions[0] == null) { | |
| return createError(c, 500, "failed to fetch emoji data"); | |
| } | |
| return c.json(versions[0], 200); | |
| } catch { | |
| return createError(c, 500, "failed to fetch emoji data"); | |
| } | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/routes/v1_versions.test.ts (1)
42-55: Add negative test case for draft versions.Consider adding a test case for when no draft versions are available to verify the 404 response.
it("should return 404 when no draft versions are available", async () => { const request = new Request("https://api.mojis.dev/api/v1/versions/draft"); const ctx = createExecutionContext(); const response = await worker.fetch(request, env, ctx); await waitOnExecutionContext(ctx); expect(response.status).toBe(404); const data = await response.json(); expect(data).toMatchObject({ message: expect.any(String), status: 404, path: "/api/v1/versions/draft", timestamp: expect.any(String), }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/routes/v1_versions.openapi.ts(2 hunks)src/schemas.ts(2 hunks)src/types.ts(2 hunks)test/index.test.ts(1 hunks)test/routes/v1_versions.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
🔇 Additional comments (8)
test/index.test.ts (1)
10-10: LGTM!The URL update correctly reflects the new API domain while maintaining the test's purpose of verifying 404 responses.
src/schemas.ts (2)
3-16: LGTM! Schema changes are well-structured and documented.The schema updates appropriately support draft versions with:
- Clear property descriptions and examples
- Nullable version fields for draft versions
- Consistent OpenAPI documentation
35-43: LGTM! Lock schema updated consistently.The
EMOJI_LOCK_SCHEMAhas been correctly updated to match the new version structure.src/routes/v1_versions.openapi.ts (4)
4-13: LGTM! Draft parameter is well-defined.The parameter definition follows the suggested string enum approach for better type safety.
19-21: LGTM! Route updated consistently.The all versions route correctly:
- Includes the draft parameter
- Uses the updated schema
Also applies to: 26-26
46-48: LGTM! Latest version route updated consistently.The latest version route correctly:
- Includes the draft parameter
- Uses the updated schema
- Has an accurate description
Also applies to: 53-53, 56-57
69-99: LGTM! Draft versions route is well-defined.The new route includes:
- Appropriate path and method
- Clear description
- Proper error responses (404 for no drafts)
- Consistent error handling
test/routes/v1_versions.test.ts (1)
12-12: Ensure consistent API URLs across tests.The base URL "https://mojis.dev" differs from "https://api.mojis.dev" used in index.test.ts.
Update the URLs to maintain consistency:
- const request = new Request("https://mojis.dev/api/v1/versions"); + const request = new Request("https://api.mojis.dev/api/v1/versions");And similarly for other test cases.
Also applies to: 28-28, 43-43, 58-58
Summary by CodeRabbit
New Features
Refactor