Feat/history integration #35
Conversation
…into feat/history-integration
…into feat/history-integration
✅ Deploy Preview for vatruzhka ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @IlinJoy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust history feature, enabling the application to persist and display a log of all executed API requests. By integrating with Supabase, the system now dynamically fetches and renders past request details, enhancing user experience by providing a comprehensive overview of their API interactions. This change moves away from static mock data, making the history functionality fully operational and data-driven. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully integrates request history with a Supabase backend. The changes are well-structured, introducing new utility functions for fetching and saving history, and updating components to use real data instead of mocks. However, I've identified a few critical issues related to error handling and data fetching that could lead to runtime crashes. Additionally, there are some high-severity bugs in response handling and URL generation. I've also included some medium-severity suggestions to improve code clarity and maintainability, such as fixing a typo and reorganizing the Supabase client setup. Please review the detailed comments for suggestions on how to address these points.
| const locale = useLocale(); | ||
| const routeUrl = generateRouteUrl(method, url, locale, body, headers); | ||
|
|
||
| const parsedHeaders = parse(HeadersSchema, JSON.parse(headers)); |
There was a problem hiding this comment.
Parsing the headers string with JSON.parse and then with valibot's parse can throw an exception if the headers string is not valid JSON or if the parsed object does not conform to the HeadersSchema. This would crash the component and stop the list of history items from rendering. You should wrap this logic in a try...catch block to handle potential parsing errors gracefully, for example by defaulting to an empty headers object.
| const parsedHeaders = parse(HeadersSchema, JSON.parse(headers)); | |
| let parsedHeaders: Record<string, string> = {}; | |
| try { | |
| parsedHeaders = parse(HeadersSchema, JSON.parse(headers)); | |
| } catch (error) { | |
| console.error('Failed to parse history item headers:', error); | |
| } |
| if (proxyResponse.error) { | ||
| throw new Error(proxyResponse.error); | ||
| } | ||
|
|
||
| if (!result.ok) { | ||
| throw new Error(`HTTP ${result.status.toString()}: ${result.statusText}`); | ||
| } | ||
|
|
||
| set({ | ||
| response: { | ||
| status: proxyResponse.status, | ||
| statusText: getReasonPhrase(proxyResponse.status), | ||
| headers: proxyResponse.headers, | ||
| body: proxyResponse.body, | ||
| timestamp, | ||
| duration, | ||
| requestSize, | ||
| responseSize, | ||
| }, | ||
| response: { ...dataForSave, statusText: getReasonPhrase(proxyResponse.status), error: error?.message }, | ||
| isLoading: false, | ||
| }); |
There was a problem hiding this comment.
Throwing an error when result.ok is false causes the execution to jump to the catch block. The catch block then sets a generic error response, losing all the details from the actual HTTP response (like status code, headers, and body) that have already been received and parsed. This prevents the user from seeing the actual error response from the server. Non-2xx responses are valid HTTP responses and should be handled gracefully by setting them in the state, not by throwing an exception that leads to loss of information.
| if (proxyResponse.error) { | |
| throw new Error(proxyResponse.error); | |
| } | |
| if (!result.ok) { | |
| throw new Error(`HTTP ${result.status.toString()}: ${result.statusText}`); | |
| } | |
| set({ | |
| response: { | |
| status: proxyResponse.status, | |
| statusText: getReasonPhrase(proxyResponse.status), | |
| headers: proxyResponse.headers, | |
| body: proxyResponse.body, | |
| timestamp, | |
| duration, | |
| requestSize, | |
| responseSize, | |
| }, | |
| response: { ...dataForSave, statusText: getReasonPhrase(proxyResponse.status), error: error?.message }, | |
| isLoading: false, | |
| }); | |
| if (proxyResponse.error) { | |
| throw new Error(proxyResponse.error); | |
| } | |
| set({ | |
| response: { ...dataForSave, statusText: getReasonPhrase(proxyResponse.status), error: error?.message }, | |
| isLoading: false, | |
| }); |
| @@ -0,0 +1,8 @@ | |||
| export const formateTimestamp = (locale: string, timestamp: string) => | |||
There was a problem hiding this comment.
There is a typo in the function name formateTimestamp. It should be formatTimestamp. This should be corrected here and in all places where it's used for consistency and to avoid confusion.
| export const formateTimestamp = (locale: string, timestamp: string) => | |
| export const formatTimestamp = (locale: string, timestamp: string) => |
|
|
||
| import { createClient } from '@/utils/supabase/client'; | ||
|
|
||
| export const supabase = createClient(); |
There was a problem hiding this comment.
This file creates and exports a singleton supabase client instance. However, the filename history-insert.ts is very specific and doesn't reflect that it's providing a general-purpose client instance. This instance is also used in AuthProvider. To improve code organization and clarity, it's better to define and export this singleton from a more general file, like @/utils/supabase/client.ts. Other parts of the app can then import the singleton from a single, clear source.
Type of PR 🍩
Changes Introduced 🥯
Self-Check ✅