chore: more elaborate response log when API request fails #1000
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes in this pull request enhance error handling across several functions within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
More templates
commit: |
|
A new canary is available for testing. You can install this latest build in your project with: pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add [email protected]
pnpm add @uploadthing/[email protected] |
📦 Bundle size comparison
|
![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
🧹 Outside diff range and nitpick comments (1)
packages/uploadthing/src/internal/logger.ts (1)
56-65: LGTM! Consider adding documentation and improving error handling.The
logHttpClientErrorfunction is well-implemented and consistent with the existing codebase. It provides a flexible way to log HTTP client errors with different levels of detail based on the error type.Consider adding JSDoc comments to explain the function's purpose, parameters, and return value. This will improve code maintainability and make it easier for other developers to understand and use the function.
The JSON parsing in the "ResponseError" case could benefit from more robust error handling. Consider wrapping the
jsonparsing in a try-catch block to handle potential parsing errors gracefully. Here's a suggested improvement:export const logHttpClientError = (message: string) => (err: HttpClientError.HttpClientError | HttpBody.HttpBodyError) => err._tag === "ResponseError" ? Effect.flatMap( Effect.tryPromise(() => err.response.json()), (json) => Effect.logError(`${message} (${err.response.status})`).pipe( Effect.annotateLogs("response", json), ), ).pipe( Effect.catchAll((parseError) => Effect.logError(`${message} (${err.response.status})`).pipe( Effect.annotateLogs("error", "Failed to parse response JSON"), Effect.annotateLogs("parseError", parseError), ), ), ) : Effect.logError(message).pipe(Effect.annotateLogs("error", err));This change ensures that even if JSON parsing fails, we still log the error message and status code, along with information about the parsing failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/uploadthing/src/internal/handler.ts (4 hunks)
- packages/uploadthing/src/internal/logger.ts (2 hunks)
- packages/uploadthing/src/sdk/index.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/src/sdk/index.ts
🧰 Additional context used
🔇 Additional comments (7)
packages/uploadthing/src/internal/logger.ts (2)
1-1: LGTM! Imports are correctly added and utilized.The new imports for
HttpBodyandHttpClientErrorfrom "@effect/platform" are appropriately added and used in thelogHttpClientErrorfunction. This maintains consistency with the existing codebase's use of the Effect library.
Line range hint
1-65: Summary: Enhanced error logging aligns with PR objectivesThe addition of the
logHttpClientErrorfunction successfully addresses the PR's objective of providing more elaborate response logging when API requests fail. This implementation:
- Integrates seamlessly with the existing logging infrastructure.
- Provides detailed error information for both HTTP client errors and response body parsing errors.
- Maintains consistency with the codebase's use of the Effect library for logging and error handling.
These changes will significantly improve debugging capabilities for API request failures, making it easier to identify and resolve issues in production environments.
packages/uploadthing/src/internal/handler.ts (5)
34-38: LGTM: Import changes enhance error logging capabilitiesThe addition of
logHttpClientErrorimport aligns well with the PR objective of improving error logging. The grouping with other logging-related imports maintains good code organization.
386-387: Excellent: Improved error handling inhandleCallbackRequestThe use of
logHttpClientErrorhere centralizes error logging and provides more detailed information when registering the callback result fails. This change enhances maintainability and aligns perfectly with the PR's objective of improving API request failure logging.
620-620: Great: Consistent error handling inhandleUploadActionThe use of
logHttpClientErrorfor metadata registration errors maintains consistency with the earlier changes. This further centralizes error logging and improves the overall error handling approach in the file.
657-663: Excellent: Enhanced error logging for dev stream callback requestsThe use of
logHttpClientErrorhere maintains consistency with earlier changes. The addition of context (hook, signature, payload) to the error log significantly enhances debugging capabilities for development-related issues.
Line range hint
1-693: Summary: Excellent improvements in error logging across the fileThe changes in this file consistently implement the use of
logHttpClientErrorfor various API request failure scenarios. This enhancement aligns perfectly with the PR's objective of providing more elaborate response logs. The consistent application of this logging approach across different functions (handleCallbackRequest,handleUploadAction, and dev stream error handling) improves code maintainability and debugging capabilities.These modifications will significantly aid in troubleshooting API-related issues by providing more detailed and context-rich error logs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/uploadthing/src/internal/logger.ts (2)
62-80: LGTM with a suggestion: Consider adding error handling for body extraction.The
logHttpClientResponsefunction is well-implemented, providing flexibility in logging level and response body format. However, consider adding error handling for the body extraction process.Consider wrapping the body extraction in a try-catch or using
Effect.tryPromiseto handle potential errors:return (response: HttpClientResponse.HttpClientResponse) => Effect.tryPromise({ try: () => mixin !== "None" ? response[mixin]() : Promise.resolve(), catch: (error) => new Error(`Failed to extract response body: ${error}`), }).pipe( Effect.flatMap(() => Effect.logWithLevel(level, `${message} (${response.status})`).pipe( Effect.annotateLogs("response", response), ) ), Effect.catchAll((error) => Effect.logError(`Error logging response: ${error.message}`).pipe( Effect.annotateLogs("error", error), Effect.annotateLogs("response", response), ) ) );This change would ensure that any errors during body extraction are caught and logged, providing more robust error handling.
82-87: LGTM with suggestions: Consider improving error type handling.The
logHttpClientErrorfunction effectively differentiates between ResponseError and other error types, and reuseslogHttpClientResponsefor consistency. However, there's room for improvement in error type narrowing and handling.Consider the following improvements:
- Use type predicates for more precise error type checking:
const isResponseError = ( err: HttpClientError.HttpClientError | HttpBody.HttpBodyError ): err is HttpClientError.ResponseError => err._tag === "ResponseError"; export const logHttpClientError = (message: string) => (err: HttpClientError.HttpClientError | HttpBody.HttpBodyError) => isResponseError(err) ? logHttpClientResponse(message, { level: "Error" })(err.response) : Effect.logError(message).pipe( Effect.annotateLogs("error", err), Effect.annotateLogs("errorType", err._tag) );
- Consider adding more specific error handling for different error types:
export const logHttpClientError = (message: string) => (err: HttpClientError.HttpClientError | HttpBody.HttpBodyError) => { if (isResponseError(err)) { return logHttpClientResponse(message, { level: "Error" })(err.response); } else if (err._tag === "RequestError") { return Effect.logError(`${message}: Request failed`).pipe( Effect.annotateLogs("error", err), Effect.annotateLogs("errorType", "RequestError") ); } else { return Effect.logError(`${message}: Unknown error`).pipe( Effect.annotateLogs("error", err), Effect.annotateLogs("errorType", err._tag) ); } };These changes would provide more precise error handling and logging, making debugging easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/uploadthing/src/internal/handler.ts (3 hunks)
- packages/uploadthing/src/internal/logger.ts (2 hunks)
- packages/uploadthing/src/sdk/index.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/src/sdk/index.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/uploadthing/src/internal/logger.ts (2)
1-5: LGTM: New imports and type definition look good.The new imports from "@effect/platform" and the
HttpClientResponseMixinMethodtype definition are appropriate for the new logging functions introduced in this file.Also applies to: 60-60
Line range hint
1-87: Summary: Good implementation with room for minor improvements.Overall, the changes in this file significantly enhance the error handling and logging capabilities, aligning well with the PR objectives. The new
logHttpClientResponseandlogHttpClientErrorfunctions provide more elaborate and flexible response logging, which will improve debugging.Some suggestions for improvements have been made:
- Adding error handling for body extraction in
logHttpClientResponse.- Improving error type handling and narrowing in
logHttpClientError.These suggestions, if implemented, would further enhance the robustness and precision of the logging system.
packages/uploadthing/src/internal/handler.ts (1)
34-39: Centralized logging functions imported successfullyThe addition of
logHttpClientError,logHttpClientResponse,withLogFormat, andwithMinimalLogLevelimports improves the codebase by centralizing logging logic, enhancing maintainability and readability.
| ), | ||
| ), | ||
| onFalse: () => | ||
| metadataRequest.pipe( | ||
| Effect.tapBoth({ | ||
| onSuccess: logHttpClientResponse("Registerred metadata"), |
There was a problem hiding this comment.
Typo in log message: 'Registerred' should be 'Registered'
Apply this diff to correct the typo:
-onSuccess: logHttpClientResponse("Registerred metadata"),
+onSuccess: logHttpClientResponse("Registered metadata"),📝 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.
| onSuccess: logHttpClientResponse("Registerred metadata"), | |
| onSuccess: logHttpClientResponse("Registered metadata"), |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/uploadthing/src/internal/handler.ts (4)
387-388: Improved error handling inhandleCallbackRequestThe use of
logHttpClientErrorfor error logging is a good improvement. It centralizes the error handling logic and makes the code more consistent.Consider making the error message more specific:
- logHttpClientError("Failed to register callback result"), + logHttpClientError("Failed to register callback result in handleCallbackRequest"),This will make it easier to identify the source of the error in logs.
624-629: Enhanced logging inhandleUploadActionThe use of
logHttpClientResponseandlogHttpClientErrorwithin atapBotheffect is a good improvement. It ensures comprehensive logging for both success and failure cases without interfering with the main function flow.Consider making the success message more specific:
- onSuccess: logHttpClientResponse("Registered metadata", { + onSuccess: logHttpClientResponse("Successfully registered metadata in handleUploadAction", {This will provide more context in the logs, making it easier to trace the flow of operations.
641-648: Improved logging for callback request forwardingThe use of
logHttpClientResponseandlogHttpClientErrorwithin atapBotheffect for callback request forwarding is a good improvement. It ensures comprehensive logging for both success and failure cases.Consider making the error message more specific:
- "Failed to forward callback request from dev stream", + "Failed to forward callback request from dev stream in handleUploadAction",This will provide more context in the logs, making it easier to identify the source of the error.
658-661: Enhanced logging for metadata registrationThe use of
logHttpClientResponseandlogHttpClientErrorwithin atapBotheffect for metadata registration is a good improvement. It ensures comprehensive logging for both success and failure cases.Consider making both messages more specific:
- onSuccess: logHttpClientResponse("Registered metadata"), - onFailure: logHttpClientError("Failed to register metadata"), + onSuccess: logHttpClientResponse("Successfully registered metadata in handleUploadAction"), + onFailure: logHttpClientError("Failed to register metadata in handleUploadAction"),This will provide more context in the logs, making it easier to trace the flow of operations and identify the source of any errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/uploadthing/src/internal/handler.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/uploadthing/src/internal/handler.ts (3)
34-39: Improved error handling with centralized loggingThe addition of
logHttpClientErrorandlogHttpClientResponseimports from the "./logger" module is a positive change. This centralization of logging functions will lead to more consistent error handling and logging across the codebase, making it easier to maintain and debug.
Line range hint
1-686: Overall improvements in error handling and loggingThe changes in this file significantly improve error handling and logging by consistently implementing centralized logging functions. This will make the code more maintainable and easier to debug. Great job on these improvements!
Remember to address the typo in the "Registered metadata" log message as mentioned in the previous comment.
625-625:⚠️ Potential issueTypo in log message persists
The typo in the log message still exists:
- onSuccess: logHttpClientResponse("Registerred metadata", { + onSuccess: logHttpClientResponse("Registered metadata", {This typo was mentioned in a previous review. Please ensure to correct it in this update.
|
A new canary is available for testing. You can install this latest build in your project with: pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add [email protected]
pnpm add @uploadthing/[email protected] |
just attaching
responseas a log annotation doesn't include the response body if it hasn't been consumed yet. this PR adds a helper that unwraps body for better debug infoSummary by CodeRabbit
New Features
Bug Fixes