feat: use r2 for data storage #15
Conversation
WalkthroughThis pull request updates the category route to fetch data using an environment variable instead of a traditional fetch call. It improves error handling by throwing an exception when data is absent and introduces a caching mechanism. Additionally, it adds a new R2 bucket property to the environment configuration and updates the wrangler configuration to include the EMOJI_DATA bucket for different environments. It also modifies several routes to implement a structured caching approach. Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 0
🧹 Nitpick comments (2)
src/routes/v1_categories.ts (2)
13-40: Consider refactoring duplicate code between route handlers.Both route handlers share nearly identical code for fetching and parsing categories. Consider extracting this common logic into a reusable function to improve maintainability.
import { cache, createError } from "../utils"; import { ALL_CATEGORIES_ROUTE, GET_CATEGORY_ROUTE } from "./v1_categories.openapi"; +async function fetchCategories(c) { + const version = c.req.param("version"); + + const res = await c.env.EMOJI_DATA.get(`v${version}/groups.json`); + + if (res == null) { + throw new HTTPException(500, { + message: "failed to fetch categories", + }); + } + + const data = await res.json(); + + const result = z.array(EmojiCategorySchema).safeParse(data); + + if (!result.success) { + return createError(c, 500, "failed to parse categories"); + } + + return result.data; +} export const V1_CATEGORIES_ROUTER = new OpenAPIHono<HonoContext>().basePath("/api/v1/categories/:version"); V1_CATEGORIES_ROUTER.use(versionMiddleware); V1_CATEGORIES_ROUTER.openapi(ALL_CATEGORIES_ROUTE, async (c) => { - const version = c.req.param("version"); - - const res = await c.env.EMOJI_DATA.get(`v${version}/groups.json`); - - if (res == null) { - throw new HTTPException(500, { - message: "failed to fetch categories", - }); - } - - const data = await res.json(); - - const result = z.array(EmojiCategorySchema).safeParse(data); - - if (!result.success) { - return createError(c, 500, "failed to parse categories"); - } - - const categories = result.data; + const categories = await fetchCategories(c); cache(c, 3600, true); return c.json( categories, 200, ); }); V1_CATEGORIES_ROUTER.openapi(GET_CATEGORY_ROUTE, async (c) => { const version = c.req.param("version"); const categorySlug = c.req.param("category"); - const res = await c.env.EMOJI_DATA.get(`v${version}/groups.json`); - - if (res == null) { - throw new HTTPException(500, { - message: "failed to fetch categories", - }); - } - - const data = await res.json(); - - const result = z.array(EmojiCategorySchema).safeParse(data); - - if (!result.success) { - return createError(c, 500, "failed to parse categories"); - } - - const category = result.data.find((c) => c.slug === categorySlug); + const categories = await fetchCategories(c); + const category = categories.find((c) => c.slug === categorySlug); cache(c, 3600, true); return c.json( category, 200, ); });Also applies to: 42-70
46-52: Consider consistent error handling approach.While this code now uses
HTTPExceptionfor R2 fetch errors, it still usescreateErrorfor parsing errors (line 59). Consider usingHTTPExceptionconsistently throughout the codebase for better error handling uniformity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/v1_categories.ts(5 hunks)worker-configuration.d.ts(1 hunks)wrangler.jsonc(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/routes/v1_categories.ts (1)
src/utils.ts (1) (1)
cache(36-46)
🔇 Additional comments (9)
worker-configuration.d.ts (1)
8-8: Good addition of R2 bucket type for emoji dataAdding the
EMOJI_DATA: R2Bucketproperty to the Cloudflare environment interface properly types the new R2 bucket storage that will replace the previous fetching mechanism. This change aligns with the PR objective to use R2 for data storage.wrangler.jsonc (3)
16-21: Good addition of R2 bucket configuration.The R2 bucket configuration is properly set up with a clear binding name
EMOJI_DATAthat will be used for storing emoji data, aligning with the PR's objective to use R2 for data storage.
34-39: Environment-specific bucket naming is a good practice.Using a separate bucket name (
mojis-preview) for the preview environment follows best practices for environment isolation, preventing potential data corruption between environments.
51-56: Consistent bucket configuration across environments.The production environment R2 bucket configuration is consistent with the root configuration, both using the
mojisbucket. This ensures proper resource management.src/routes/v1_categories.ts (5)
3-3: Good improvement in error handling.Importing
HTTPExceptionfrom Hono allows for more standardized error responses.
6-6: Added caching capability.Including the
cacheutility enables response caching, which will improve API performance.
16-22: Changed data source from remote fetch to R2 storage.The implementation now uses Cloudflare R2 storage instead of remote fetching, which should provide better performance and reliability. Good use of null check for error handling.
34-35: Added response caching.Setting a 1-hour cache with immutable flag will significantly improve performance for frequently accessed data.
64-65: Good caching implementation for single category route.Implementing the same caching strategy for the single category route ensures consistent performance improvements across the API.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/routes/v1_categories.ts (3)
22-28: Refactor to eliminate duplicated data fetching and error handling.The same data fetching and error handling code is duplicated in both route handlers. Consider extracting this logic into a reusable function.
+ async function fetchCategories(c) { + const version = c.req.param("version"); + const res = await c.env.EMOJI_DATA.get(`v${version}/groups.json`); + + if (res == null) { + throw new HTTPException(500, { + message: "failed to fetch categories", + }); + } + + const data = await res.json(); + const result = z.array(EmojiCategorySchema).safeParse(data); + + if (!result.success) { + return createError(c, 500, "failed to parse categories"); + } + + return result.data; + } V1_CATEGORIES_ROUTER.openapi(ALL_CATEGORIES_ROUTE, async (c) => { - const version = c.req.param("version"); - - const res = await c.env.EMOJI_DATA.get(`v${version}/groups.json`); - - if (res == null) { - throw new HTTPException(500, { - message: "failed to fetch categories", - }); - } - - const data = await res.json(); - - const result = z.array(EmojiCategorySchema).safeParse(data); - - if (!result.success) { - return createError(c, 500, "failed to parse categories"); - } - - const categories = result.data; + const categories = await fetchCategories(c); return c.json( categories, 200, ); }); V1_CATEGORIES_ROUTER.openapi(GET_CATEGORY_ROUTE, async (c) => { const categorySlug = c.req.param("category"); - const version = c.req.param("version"); - - const res = await c.env.EMOJI_DATA.get(`v${version}/groups.json`); - - if (res == null) { - throw new HTTPException(500, { - message: "failed to fetch categories", - }); - } - - const data = await res.json(); - - const result = z.array(EmojiCategorySchema).safeParse(data); - - if (!result.success) { - return createError(c, 500, "failed to parse categories"); - } - - const category = result.data.find((c) => c.slug === categorySlug); + const categories = await fetchCategories(c); + const category = categories.find((c) => c.slug === categorySlug);Also applies to: 50-56
24-27: Consider consistent error handling across the codebase.This file uses
HTTPExceptionfor error handling while other files (likegateway_github.ts) usecreateError. Consider standardizing the error handling approach across the application.Also applies to: 52-55
46-72: Optimize data storage for individual category retrieval.Currently, the GET_CATEGORY_ROUTE handler fetches all categories and then finds the specific one. Consider storing individual categories as separate objects in R2 to enable direct retrieval of a specific category without loading all data.
If restructuring storage isn't feasible now, consider adding a comment indicating this as a future optimization:
// TODO: Optimize by storing individual categories as separate objects // to enable direct retrieval without loading all categories
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/cache.ts(0 hunks)src/routes/gateway_github.ts(1 hunks)src/routes/random-emoji.ts(1 hunks)src/routes/v1_categories.ts(3 hunks)src/utils.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/cache.ts
- src/utils.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/routes/gateway_github.ts (1)
src/types.ts (1) (1)
HonoContext(4-6)
src/routes/v1_categories.ts (1)
src/routes/v1_categories.openapi.ts (1) (1)
ALL_CATEGORIES_ROUTE(15-41)
🔇 Additional comments (3)
src/routes/gateway_github.ts (1)
3-4: LGTM! Well-implemented caching middleware.The implementation of the global cache middleware for GitHub emojis is well-structured. Using the wildcard path ensures that all routes in this router benefit from caching, and the cache configuration with a 1-hour expiration is appropriate for this type of data.
Also applies to: 9-12
src/routes/v1_categories.ts (2)
14-17: LGTM! Well-implemented caching middleware.The implementation of the global cache middleware for category routes is properly structured with appropriate cache settings.
22-22: LGTM! Good transition to R2 for data storage.The change from HTTP fetch to R2 bucket (
c.env.EMOJI_DATA.get()) aligns well with the PR objective of using R2 for data storage, which should improve performance and reliability.Also applies to: 50-50
|
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/routes/v1_categories.test.ts (2)
10-35: Consider adding test data cleanupThe
beforeAllimplementation correctly populates the R2 bucket with test data necessary for the tests. This is a good practice for test isolation.However, consider adding an
afterAllhook to clean up the test data after tests complete, which would ensure complete test isolation between test runs.+ import { beforeAll, afterAll, describe, expect, it } from "vitest"; // After all the tests + afterAll(async () => { + await env.EMOJI_DATA.delete("v15.1/groups.json"); + });
11-34: Add error handling to the data initializationThe current implementation doesn't handle potential errors during R2 data initialization. Consider adding try/catch to properly handle and report setup failures.
beforeAll(async () => { + try { await env.EMOJI_DATA.put("v15.1/groups.json", JSON.stringify([ { // ... existing data }, ])); + } catch (error) { + console.error("Failed to initialize test data:", error); + throw error; + } });
📜 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 (6)
package.json(2 hunks)src/routes/v1_categories.ts(3 hunks)test/routes/v1_categories.test.ts(1 hunks)tsconfig.json(1 hunks)vitest.config.ts(1 hunks)worker-configuration-test.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- worker-configuration-test.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/v1_categories.ts
🔇 Additional comments (9)
tsconfig.json (1)
10-10: New Type Declaration AddedThe new type entry
"./worker-configuration-test.d.ts"has been correctly added to the"types"array. This enhances type checking and IntelliSense and aligns with the broader effort for improved Cloudflare bindings.vitest.config.ts (1)
9-9: Good addition of isolatedStorage for test reliability.Adding
isolatedStorage: trueis appropriate when working with R2 storage in tests. This ensures each test worker has its own isolated storage environment, preventing test interference and potential flaky tests when multiple tests interact with storage buckets simultaneously.test/routes/v1_categories.test.ts (3)
7-7: Good addition of thebeforeAllimportThe import of
beforeAllis correctly added to support the test data setup.
63-77: The existing test for valid version now depends on seeded dataThis test previously might have used mocked responses, but now it will use the data we've seeded in the
beforeAllhook. The test looks good, but it's worth noting this architectural change from mocked responses to a more integration-test-like approach using the R2 bucket.
79-92: Test specifically validates the seeded dataThis test now provides targeted validation for the "smileys-emotion" category that was seeded in the
beforeAllhook. The implementation aligns well with the new data storage approach.package.json (4)
5-5: Updated Package Manager Version
ThepackageManagerfield has been updated from[email protected]to[email protected]. This is a minor version bump which should be backwards compatible, but please verify that none of the project scripts rely on version-specific behaviors in pnpm.
21-21: Dependency Upgrade for @scalar/hono-api-reference
The version for@scalar/hono-api-referencehas been bumped from^0.7.1to^0.7.2. Ensure that you review the changelog for any potential backward-incompatible changes and that it integrates smoothly with your existing codebase.
26-26: Dev Dependency Update: @cloudflare/vitest-pool-workers
The dev dependency for@cloudflare/vitest-pool-workershas been updated from^0.8.1to^0.8.2. This minor version update generally indicates bug fixes or minor improvements. Please run your tests to confirm the upgrade does not introduce any issues.
34-34: Dev Dependency Update: Wrangler
Thewranglerversion has been upgraded from^4.1.0to^4.2.0, which is likely necessary to support the new R2 integration. Please verify that this upgrade works seamlessly with your new R2 configurations and that any new features or breaking changes in wrangler are properly handled by your project scripts and configuration.
Summary by CodeRabbit
New Features
Bug Fixes
Chores