feat(api): align wildcard and ucd-store routes #460
Conversation
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request centralizes HTTP response handling for file and directory operations by introducing new utility functions in a shared library. These utilities replace duplicated inline logic across multiple route handlers for constructing response headers, determining file types, and handling file/directory responses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 |
github-actions
bot
added
pkg: cli
🌏 Preview Deployments
Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
luxass
force-pushed
the
feat/align-store-files-edp
branch
from
c21c43c to
a874554
Compare
Refactor file and directory response logic by introducing dedicated handlers `handleFileResponse` and `handleDirectoryResponse`. This improves code organization and readability. Additionally, utility functions for determining file extensions and checking directory listings have been added.
luxass
force-pushed
the
feat/align-store-files-edp
branch
from
a874554 to
3ef4be8
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
github-actions
bot
removed
pkg: cli
There was a problem hiding this comment.
Pull request overview
This pull request refactors file handling logic by extracting common functionality from the wildcard and ucd-store routes into shared utility functions. This reduces code duplication and improves maintainability.
Changes:
- Extracted file and directory response handling logic into shared utility functions in
apps/api/src/lib/files.ts - Removed duplicated code from both route handlers (
$wildcard.tsandfiles.ts) - Cleaned up console logging statements and eslint-disable directives
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/api/src/lib/files.ts | Added new utility functions: buildDirectoryHeaders, buildFileHeaders, handleFileResponse, handleDirectoryResponse, determineFileExtension, isHtmlFile, and isDirectoryListing to centralize file handling logic |
| apps/api/src/routes/v1_files/$wildcard.ts | Removed duplicated helper functions and replaced with calls to shared utilities; removed unused Entry type import |
| apps/api/src/ucd-store/routes/files.ts | Replaced inline file handling logic with shared utility function calls; removed route-specific console logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| export async function handleFileResponse( | ||
| c: any, |
There was a problem hiding this comment.
The parameter c has type any which bypasses type checking. Consider using Context<HonoEnv> imported from "hono" for better type safety.
| } | ||
|
|
||
| export function handleDirectoryResponse( | ||
| c: any, |
There was a problem hiding this comment.
The parameter c has type any which bypasses type checking. Consider using Context<HonoEnv> imported from "hono" for better type safety.
| console.log(`[file-handler]: HEAD request, calculated size: ${actualSize}`); | ||
| return c.newResponse(null, 200, headers); | ||
| } | ||
|
|
||
| const headers: Record<string, string> = { | ||
| "Content-Type": contentType, | ||
| ...baseHeaders, | ||
| [UCD_STAT_TYPE_HEADER]: "file", | ||
| }; | ||
|
|
||
| const cd = response.headers.get("Content-Disposition"); | ||
| if (cd) headers["Content-Disposition"] = cd; | ||
|
|
||
| console.log(`[file-handler]: binary file, streaming without buffering`); |
There was a problem hiding this comment.
The log messages now use a generic [file-handler] prefix instead of route-specific prefixes like [v1_files] or [ucd-store]. This makes it harder to trace which route generated the log entry. Consider either passing a logger prefix/context as a parameter to these functions, or using the route-specific logging before calling these shared handlers.
| export function buildDirectoryHeaders( | ||
| files: Entry[], | ||
| baseHeaders: Record<string, string>, | ||
| ): Record<string, string> { | ||
| return { | ||
| ...baseHeaders, | ||
| [UCD_STAT_TYPE_HEADER]: "directory", | ||
| [UCD_STAT_CHILDREN_HEADER]: `${files.length}`, | ||
| [UCD_STAT_CHILDREN_FILES_HEADER]: `${files.filter((f) => f.type === "file").length}`, | ||
| [UCD_STAT_CHILDREN_DIRS_HEADER]: `${files.filter((f) => f.type === "directory").length}`, | ||
| }; | ||
| } | ||
|
|
||
| export function buildFileHeaders( | ||
| contentType: string, | ||
| baseHeaders: Record<string, string>, | ||
| response: Response, | ||
| actualContentLength: number, | ||
| ): Record<string, string> { | ||
| const headers: Record<string, string> = { | ||
| "Content-Type": contentType, | ||
| ...baseHeaders, | ||
| [UCD_STAT_TYPE_HEADER]: "file", | ||
| [UCD_STAT_SIZE_HEADER]: `${actualContentLength}`, | ||
| "Content-Length": `${actualContentLength}`, | ||
| }; | ||
|
|
||
| const cd = response.headers.get("Content-Disposition"); | ||
| if (cd) headers["Content-Disposition"] = cd; | ||
|
|
||
| return headers; | ||
| } | ||
|
|
||
| export interface FileResponseOptions { | ||
| contentType: string; | ||
| baseHeaders: Record<string, string>; | ||
| response: Response; | ||
| isHeadRequest: boolean; | ||
| } | ||
|
|
||
| export async function handleFileResponse( | ||
| c: any, | ||
| options: FileResponseOptions, | ||
| ): Promise<Response> { | ||
| const { contentType, baseHeaders, response, isHeadRequest } = options; | ||
|
|
||
| if (isHeadRequest) { | ||
| const blob = await response.blob(); | ||
| const actualSize = blob.size; | ||
| const headers = buildFileHeaders(contentType, baseHeaders, response, actualSize); | ||
| console.log(`[file-handler]: HEAD request, calculated size: ${actualSize}`); | ||
| return c.newResponse(null, 200, headers); | ||
| } | ||
|
|
||
| const headers: Record<string, string> = { | ||
| "Content-Type": contentType, | ||
| ...baseHeaders, | ||
| [UCD_STAT_TYPE_HEADER]: "file", | ||
| }; | ||
|
|
||
| const cd = response.headers.get("Content-Disposition"); | ||
| if (cd) headers["Content-Disposition"] = cd; | ||
|
|
||
| console.log(`[file-handler]: binary file, streaming without buffering`); | ||
|
|
||
| return c.newResponse(response.body, 200, headers); | ||
| } | ||
|
|
||
| export interface DirectoryResponseOptions { | ||
| files: Entry[]; | ||
| baseHeaders: Record<string, string>; | ||
| } | ||
|
|
||
| export function handleDirectoryResponse( | ||
| c: any, | ||
| options: DirectoryResponseOptions, | ||
| ): Response { | ||
| const { files, baseHeaders } = options; | ||
| const headers = buildDirectoryHeaders(files, baseHeaders); | ||
| return c.json(files, 200, headers); | ||
| } | ||
|
|
||
| export function determineFileExtension(leaf: string): string { | ||
| return leaf.includes(".") ? leaf.split(".").pop()!.toLowerCase() : ""; | ||
| } | ||
|
|
||
| export function isHtmlFile(extName: string): boolean { | ||
| return HTML_EXTENSIONS.includes(`.${extName}`); | ||
| } | ||
|
|
||
| export function isDirectoryListing(contentType: string, extName: string): boolean { | ||
| return contentType.includes("text/html") && !isHtmlFile(extName); | ||
| } |
There was a problem hiding this comment.
The newly extracted functions handleFileResponse, handleDirectoryResponse, determineFileExtension, isHtmlFile, isDirectoryListing, buildDirectoryHeaders, and buildFileHeaders lack test coverage. Consider adding unit tests for these functions to ensure they behave correctly in isolation, especially for edge cases like empty file extensions, different content types, and header handling.
| export function buildDirectoryHeaders( | ||
| files: Entry[], | ||
| baseHeaders: Record<string, string>, | ||
| ): Record<string, string> { | ||
| return { | ||
| ...baseHeaders, | ||
| [UCD_STAT_TYPE_HEADER]: "directory", | ||
| [UCD_STAT_CHILDREN_HEADER]: `${files.length}`, | ||
| [UCD_STAT_CHILDREN_FILES_HEADER]: `${files.filter((f) => f.type === "file").length}`, | ||
| [UCD_STAT_CHILDREN_DIRS_HEADER]: `${files.filter((f) => f.type === "directory").length}`, | ||
| }; | ||
| } | ||
|
|
||
| export function buildFileHeaders( | ||
| contentType: string, | ||
| baseHeaders: Record<string, string>, | ||
| response: Response, | ||
| actualContentLength: number, | ||
| ): Record<string, string> { | ||
| const headers: Record<string, string> = { | ||
| "Content-Type": contentType, | ||
| ...baseHeaders, | ||
| [UCD_STAT_TYPE_HEADER]: "file", | ||
| [UCD_STAT_SIZE_HEADER]: `${actualContentLength}`, | ||
| "Content-Length": `${actualContentLength}`, | ||
| }; | ||
|
|
||
| const cd = response.headers.get("Content-Disposition"); | ||
| if (cd) headers["Content-Disposition"] = cd; | ||
|
|
||
| return headers; | ||
| } | ||
|
|
||
| export interface FileResponseOptions { | ||
| contentType: string; | ||
| baseHeaders: Record<string, string>; | ||
| response: Response; | ||
| isHeadRequest: boolean; | ||
| } | ||
|
|
||
| export async function handleFileResponse( | ||
| c: any, | ||
| options: FileResponseOptions, | ||
| ): Promise<Response> { | ||
| const { contentType, baseHeaders, response, isHeadRequest } = options; | ||
|
|
||
| if (isHeadRequest) { | ||
| const blob = await response.blob(); | ||
| const actualSize = blob.size; | ||
| const headers = buildFileHeaders(contentType, baseHeaders, response, actualSize); | ||
| console.log(`[file-handler]: HEAD request, calculated size: ${actualSize}`); | ||
| return c.newResponse(null, 200, headers); | ||
| } | ||
|
|
||
| const headers: Record<string, string> = { | ||
| "Content-Type": contentType, | ||
| ...baseHeaders, | ||
| [UCD_STAT_TYPE_HEADER]: "file", | ||
| }; | ||
|
|
||
| const cd = response.headers.get("Content-Disposition"); | ||
| if (cd) headers["Content-Disposition"] = cd; | ||
|
|
||
| console.log(`[file-handler]: binary file, streaming without buffering`); | ||
|
|
||
| return c.newResponse(response.body, 200, headers); | ||
| } | ||
|
|
||
| export interface DirectoryResponseOptions { | ||
| files: Entry[]; | ||
| baseHeaders: Record<string, string>; | ||
| } | ||
|
|
||
| export function handleDirectoryResponse( | ||
| c: any, | ||
| options: DirectoryResponseOptions, | ||
| ): Response { | ||
| const { files, baseHeaders } = options; | ||
| const headers = buildDirectoryHeaders(files, baseHeaders); | ||
| return c.json(files, 200, headers); | ||
| } |
There was a problem hiding this comment.
The newly added utility functions lack documentation. Consider adding JSDoc comments to describe their purpose, parameters, return values, and usage examples, especially for handleFileResponse, handleDirectoryResponse, buildDirectoryHeaders, and buildFileHeaders which are public exports.
1 participant
🔗 Linked issue
📚 Description
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.