New class-based Api interface and frontend proxy #143
Conversation
WalkthroughThis pull request introduces significant refactoring of the frontend API interaction and configuration management. Key changes include the creation of a new Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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: 3
🔭 Outside diff range comments (3)
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (1)
Line range hint
52-57: Add error handling for the search API call.The search functionality should handle potential API errors gracefully to prevent the UI from breaking and provide feedback to users when the search fails.
const debouncedSearch = debounce(async () => { if (input.length > 0) { - const response = await api.search(input); - searchResults = response.items; + try { + const response = await api.search(input); + searchResults = response.items; + } catch (error) { + console.error('Error during search:', error); + searchResults = []; + // TODO: Consider showing an error message to the user + } } else { searchResults = []; } }, 300);frontend/src/routes/joins/[slug]/+page.svelte (2)
Line range hint
204-214: Refactor duplicate API parameters into a reusable configuration.The API calls share many common parameters. Consider extracting these into a base configuration object to reduce duplication and improve maintainability.
// Create a base configuration const baseTimeseriesConfig = { joinId: joinTimeseries.name, startTs: data.dateRange.startTimestamp, endTs: data.dateRange.endTimestamp, metricType: 'drift', offset: '1D', algorithm: 'psi', granularity: 'percentile' }; // Use it in API calls const featureData = await api.getFeatureTimeseries({ ...baseTimeseriesConfig, featureName: seriesName, metrics: 'value' });Also applies to: 215-225, 424-434
Line range hint
424-442: Add error handling for individual distribution fetches.The distribution loading could fail for individual features while others succeed. Consider handling errors for each feature separately to prevent a single failure from affecting all distributions.
const distributionsPromises = allFeatures.map((featureName) => - api.getFeatureTimeseries({ + api.getFeatureTimeseries({ ...baseTimeseriesConfig, featureName, metrics: 'value' - }) + }).catch(error => { + console.error(`Error loading distribution for ${featureName}:`, error); + return null; + }) ); const responses = await Promise.all(distributionsPromises); -distributions = responses.filter((response) => response.isNumeric); +distributions = responses.filter((response): response is FeatureResponse => + response !== null && response.isNumeric +);
🧹 Nitpick comments (8)
frontend/src/lib/api/api.ts (3)
14-19: Clarify usage of 'data' in ApiRequestOptions
While 'data' is typed broadly as Record<string, any>, it’s used to build both query string parameters (for GET) and request body (for POST). This is convenient but can confuse maintainers. Consider adding a JSDoc comment or a more explicit naming scheme (e.g., queryData vs. bodyData) to improve readability and prevent misusage.
26-29: Validate accessToken usage
The constructor sets accessToken, but it’s only used for the Authorization header in #send. If additional token-based logic is needed in the future, ensure this field is kept in sync or provide direct setter methods for reusability (e.g., token refresh flows).
33-80: Promote naming clarity and parameter coverage
The methods getModels, getJoins, search, and getJoinTimeseries follow a clear pattern. Here are some suggestions:
• For getJoins, offset and limit are numeric, so the usage of URLSearchParams is appropriate. Consider validating offset and limit to prevent negative values or unexpected large inputs.
• search method uses a single "term" param. If the system eventually supports advanced searches with more parameters, you might want a typed object as input.
• getJoinTimeseries uses default offsets and metrics. Consider clarifying in docstrings or comments what “null” or '10h' implies for the domain.frontend/src/routes/api/[...path]/+server.ts (2)
4-8: Document the proxy approach
Your docstring is good at explaining the purpose of the proxy (CORS, consistent URL). Consider referencing the newApiclass in comments so future contributors can quickly see how everything ties together.
9-11: Handle non-GET/POST methods
Currently, only GET and POST are implemented. In case you need PUT, PATCH, or DELETE in the future, consider either rejecting them explicitly or adding fallback logic. This helps avoid 404 confusion or inconsistent behavior.Also applies to: 13-15
frontend/src/routes/joins/[slug]/+page.server.ts (1)
26-26: Validate fallback logic
If the user-provided date range is invalid, you catch the error and revert to fallback timestamps (Jan-Mar 2023). That’s a good fallback approach. Consider whether you need to document or display a “Using fallback data range” warning to the UI.frontend/README.md (1)
35-37: Add language specifier to the code block.The code block for the
.envfile content should specify a language for better syntax highlighting and documentation consistency.- ``` + ```env API_BASE_URL=http://localhost:9000 ```🧰 Tools
🪛 Markdownlint (0.37.0)
35-35: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
frontend/src/routes/joins/[slug]/+page.svelte (1)
Line range hint
424-442: Consider implementing request batching for performance optimization.Loading distributions triggers multiple parallel API requests. Consider implementing a batched API endpoint to reduce network overhead.
Would you like me to help design a batched API endpoint that can handle multiple feature requests in a single call?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
frontend/README.md(1 hunks)frontend/src/lib/api/api.ts(1 hunks)frontend/src/lib/components/NavigationBar/NavigationBar.svelte(2 hunks)frontend/src/routes/api/[...path]/+server.ts(1 hunks)frontend/src/routes/joins/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(6 hunks)frontend/src/routes/joins/[slug]/+page.svelte(5 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
frontend/README.md
35-35: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
frontend/src/lib/api/api.ts (2)
8-12: Ensure consistent base-URL handling
Currently, the optional 'base' property imposes no requirement for a trailing slash. If the user of this API sets a base without a trailing slash, subsequent path concatenations may lead to malformed URLs (e.g. "https://example.comapi/v1"). Consider normalizing the string here (e.g., adding or trimming trailing slashes) to avoid potential errors and improve developer UX.
Would you like to verify usage of this property across the codebase to ensure consistent usage of slashes?
21-24: Private class fields and encapsulation
Using private fields (#base, #fetch, #accessToken) is a neat approach to encapsulate your implementation details. Ensure you have test coverage and that no other parts of your code need direct read access.
frontend/src/routes/joins/+page.server.ts (2)
3-3: Transition to class-based API usage
Replacing individual function imports with { Api } fosters a more standardized approach across the codebase. This is a good maintainability improvement.
5-5: Ensure 'fetch' usage is properly tested
Using fetch from SvelteKit’s server context is convenient, but it may differ from the browser’s fetch, especially around credentials or SSR environment. Confirm that your test coverage includes these aspects (CORS, SSR fetch differences, etc.).
Also applies to: 8-8
frontend/src/routes/api/[...path]/+server.ts (1)
1-2: Check for environment variable consistency
You rely on env.API_BASE_URL. Validate that it’s set in all deployment target environments to avoid runtime errors.
frontend/src/routes/joins/[slug]/+page.server.ts (3)
2-2: Consistent usage of type imports
Importing { Api } from '$lib/api/api' is consistent with your updated approach. Ensure references to the older version of the API do not linger in the codebase.
13-14: Pass 'fetch' for SSR
By passing the fetch parameter, you ensure SSR compatibility. Good job. Verify that the same approach is used consistently in other load functions.
79-79: fetchInitialData must handle concurrency
Promise.all is a great approach for concurrent requests. Make sure to handle partial failures: if one request fails (e.g., getModels), do you want to continue using partial data from getJoinTimeseries?
| async getFeatureTimeseries({ | ||
| joinId, | ||
| featureName, | ||
| startTs, | ||
| endTs, | ||
| metricType = 'drift', | ||
| metrics = 'null', | ||
| offset = '10h', | ||
| algorithm = 'psi', | ||
| granularity = 'aggregates' | ||
| }: { | ||
| joinId: string; | ||
| featureName: string; | ||
| startTs: number; | ||
| endTs: number; | ||
| metricType?: string; | ||
| metrics?: string; | ||
| offset?: string; | ||
| algorithm?: string; | ||
| granularity?: string; | ||
| }): Promise<FeatureResponse> { | ||
| const params = new URLSearchParams({ | ||
| startTs: startTs.toString(), | ||
| endTs: endTs.toString(), | ||
| metricType, | ||
| metrics, | ||
| offset, | ||
| algorithm, | ||
| granularity | ||
| }); | ||
| return this.#send(`join/${joinId}/feature/${featureName}/timeseries?${params.toString()}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validations and exceptions
getFeatureTimeseries is quite similar to getJoinTimeseries. You might consider factoring out common logic (e.g., forming query parameters) for reusability. Also, ensure that featureName is validated before constructing the final URL to avoid confusing API errors if an invalid or empty featureName is passed.
| async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) { | ||
| let url = `${this.#base}/${resource}`; | ||
|
|
||
| export async function getFeatureTimeseries({ | ||
| joinId, | ||
| featureName, | ||
| startTs, | ||
| endTs, | ||
| metricType = 'drift', | ||
| metrics = 'null', | ||
| offset = '10h', | ||
| algorithm = 'psi', | ||
| granularity = 'aggregates' | ||
| }: { | ||
| joinId: string; | ||
| featureName: string; | ||
| startTs: number; | ||
| endTs: number; | ||
| metricType?: string; | ||
| metrics?: string; | ||
| offset?: string; | ||
| algorithm?: string; | ||
| granularity?: string; | ||
| }): Promise<FeatureResponse> { | ||
| const params = new URLSearchParams({ | ||
| startTs: startTs.toString(), | ||
| endTs: endTs.toString(), | ||
| metricType, | ||
| metrics, | ||
| offset, | ||
| algorithm, | ||
| granularity | ||
| }); | ||
| return get(`join/${joinId}/feature/${featureName}/timeseries?${params.toString()}`); | ||
| const method = options?.method ?? 'GET'; | ||
|
|
||
| if (method === 'GET' && options?.data) { | ||
| url += `?${new URLSearchParams(options.data)}`; | ||
| } | ||
|
|
||
| return this.#fetch(url, { | ||
| method: options?.method ?? 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` }) | ||
| }, | ||
| ...(method === 'POST' && | ||
| options?.data && { | ||
| body: JSON.stringify(options.data) | ||
| }) | ||
| }).then(async (response) => { | ||
| const text = await response.text(); | ||
| try { | ||
| return JSON.parse(text) as Data; | ||
| } catch (e) { | ||
| console.log(`Unable to parse: "${text}" for url: ${url}`); | ||
| throw e; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Augment error handling for #send
The #send method currently logs JSON parse errors but does not handle HTTP errors (4xx/5xx). Consider:
• Raising an explicit error if response.ok is false.
• Providing better fallback or retry logic.
• Exposing error messages from the response body to help with debugging.
Also, keep in mind that many requests can fail for network reasons before receiving any response. Consider implementing a try/catch around fetch calls.
| // Try with requested date range first | ||
| try { | ||
| const { joinTimeseries, model } = await fetchInitialData( | ||
| api, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate error handling
You have a try/catch block that logs an error, then calls fetchInitialData again. Ensure you consistently handle or log the underlying cause (e.g., 404 vs. network error). Otherwise, you might mask repeated issues.
Also applies to: 56-56
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/lib/api/api.ts (1)
54-116: 🛠️ Refactor suggestionRefactor duplicate code in timeseries methods.
The getJoinTimeseries and getFeatureTimeseries methods share significant common code. Consider:
- Creating a shared type for common parameters
- Extracting the common URL parameters logic
+type TimeseriesParams = { + startTs: number; + endTs: number; + metricType?: string; + metrics?: string; + offset?: string; + algorithm?: string; +}; +private createTimeseriesParams(params: TimeseriesParams): URLSearchParams { + return new URLSearchParams({ + startTs: params.startTs.toString(), + endTs: params.endTs.toString(), + metricType: params.metricType ?? 'drift', + metrics: params.metrics ?? 'null', + offset: params.offset ?? '10h', + algorithm: params.algorithm ?? 'psi' + }); +}
🧹 Nitpick comments (6)
frontend/src/lib/api/api.ts (4)
9-20: LGTM! Consider adding JSDoc comments for better documentation.The type definitions are well-structured and provide good flexibility. Consider adding JSDoc comments to document the purpose and constraints of each type and its properties.
+/** Options for configuring the Api instance */ export type ApiOptions = { + /** Base URL for API requests. Defaults to '/api/v1' */ base?: string; + /** Custom fetch implementation. Defaults to global fetch */ fetch?: typeof fetch; + /** Optional access token for authentication */ accessToken?: string; };
33-34: Address the TODO comment about project structure.Consider creating a dedicated models API client class that extends the base Api class. This would improve code organization and maintainability.
Would you like me to help create a proposal for the model-specific API client structure?
38-44: Add parameter validation for pagination.Consider adding validation for offset and limit parameters to ensure they are non-negative numbers.
async getJoins(offset: number = 0, limit: number = 10) { + if (offset < 0 || limit <= 0) { + throw new Error('Offset must be non-negative and limit must be positive'); + } const params = new URLSearchParams({ offset: offset.toString(), limit: limit.toString() }); return this.#send<JoinsResponse>(`joins?${params.toString()}`); }
46-52: Add input validation for search parameters.The search term should be validated to ensure it's not empty or just whitespace. Also, consider consistent validation with getJoins for the limit parameter.
async search(term: string, limit: number = 20) { + if (!term?.trim()) { + throw new Error('Search term cannot be empty'); + } + if (limit <= 0) { + throw new Error('Limit must be positive'); + } const params = new URLSearchParams({ term, limit: limit.toString() }); return this.#send<ModelsResponse>(`search?${params.toString()}`); }frontend/src/lib/api/api.test.ts (1)
Line range hint
1-65: Enhance test coverage for API module.Consider adding tests for:
- Timeout scenarios
- Network failures
- Invalid JSON responses
- Different HTTP methods (POST, PUT, DELETE)
- Authorization header handling
Example test cases:
it('should handle request timeout', async () => { mockFetch.mockImplementationOnce(() => new Promise(() => {})); await expect(api.getModels()).rejects.toThrow('Request timeout'); }); it('should handle network errors', async () => { mockFetch.mockRejectedValueOnce(new Error('Network failure')); await expect(api.getModels()).rejects.toThrow('Network failure'); });frontend/src/lib/types/Model/Model.test.ts (1)
Line range hint
1-200: LGTM! Consider enhancing warning logs.The tests are comprehensive and well-structured. Consider:
- Using a structured logging format for warnings
- Adding assertions for the warning messages in tests
-console.warn(`Additional fields found in ModelsResponse: ${additionalKeys.join(', ')}`); +console.warn(JSON.stringify({ + message: 'Additional fields found in response', + type: 'ModelsResponse', + fields: additionalKeys +}));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
frontend/src/lib/api/api.test.ts(4 hunks)frontend/src/lib/api/api.ts(1 hunks)frontend/src/lib/types/Model/Model.test.ts(1 hunks)
🔇 Additional comments (1)
frontend/src/lib/api/api.ts (1)
118-154:
Enhance error handling in the #send method.
The current error handling could be improved in several ways:
- Add request timeout handling
- Implement retry logic for transient failures
- Provide more detailed error information
- Add proper error logging instead of console.log
async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) {
+ const timeout = options?.timeout ?? 30000;
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), timeout);
+
let url = `${this.#base}/${resource}`;
const method = options?.method ?? 'GET';
if (method === 'GET' && options?.data) {
url += `?${new URLSearchParams(options.data)}`;
}
- return this.#fetch(url, {
- method: options?.method ?? 'GET',
- headers: {
- 'Content-Type': 'application/json',
- ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` })
- },
- ...(method === 'POST' &&
- options?.data && {
- body: JSON.stringify(options.data)
- })
- }).then(async (response) => {
+ try {
+ const response = await this.#fetch(url, {
+ method: options?.method ?? 'GET',
+ headers: {
+ 'Content-Type': 'application/json',
+ ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` })
+ },
+ signal: controller.signal,
+ ...(method === 'POST' &&
+ options?.data && {
+ body: JSON.stringify(options.data)
+ })
+ });
if (response.ok) {
const text = await response.text();
try {
if (text) {
return JSON.parse(text) as Data;
} else {
return {};
}
} catch (e) {
- console.log(`Unable to parse: "${text}" for url: ${url}`);
- throw e;
+ throw new Error(`Failed to parse response: ${e.message}`);
}
} else {
- error(response.status);
+ const errorBody = await response.text();
+ throw new Error(`HTTP ${response.status}: ${errorBody}`);
}
- });
+ } catch (e) {
+ if (e.name === 'AbortError') {
+ throw new Error(`Request timeout after ${timeout}ms`);
+ }
+ throw e;
+ } finally {
+ clearTimeout(timeoutId);
+ }
}Likely invalid or redundant comment.
|
@ken-zlai based on our discussion earlier, and should replace the need for #141 (and possibly https://github.com/zipline-ai/infrastructure/pull/5) The api diff is easier to view if you enable "Hide whitespace"
|
sean-zlai
changed the title
|
@sean-zlai Very crafty, I like this approach quite a bit. @chewy-zlai Since we proxy all api requests thru our node server, we shouldn't need any sort of public load balanced URL. Apologies since you already implemented that in the infra repo. Having that said, we can test this PR out by building and deploying this (sean/api-proxy) branch. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/routes/api/[...path]/+server.ts (1)
6-10: Enhance documentation with security considerationsWhile the current documentation explains the proxy's purpose well, it should include:
- Security implications and limitations
- Expected request/response format
- Error handling behavior
Consider expanding the documentation:
/** * Proxy calls from frontend server (ex. `http://app.zipline.ai/api`) to Scala backend (ex. `http://app:9000/api`), resolving: * - CORS issues * - Consistent URL handling whether issuing requests from browser (ex. `+page.svelte`) or frontend server (ex `+page.server.ts`) + * + * Security considerations: + * - Requests are proxied as-is, maintain validation in the backend + * - Only whitelisted API endpoints should be accessible + * + * Error handling: + * - Backend errors are passed through unchanged + * - Network errors will be thrown as-is */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/routes/api/[...path]/+server.ts(1 hunks)
🔇 Additional comments (1)
frontend/src/routes/api/[...path]/+server.ts (1)
1-17: Verify HTTP method support and content type handling
The current implementation might need to support additional HTTP methods (PUT, DELETE, etc.) and handle various content types properly.
Let's verify the API requirements:
✅ Verification successful
Let me gather more information about the API usage patterns to make a comprehensive verification.
The proxy implementation is sufficient for current API requirements
The implementation is correctly handling all necessary HTTP methods. Here's why:
- The proxy server passes through the original
requestobject which preserves all HTTP methods, headers, and content types - The API client (
frontend/src/lib/api/api.ts) shows that only GET and POST methods are currently used in the application - Content-Type handling is properly implemented in the API client with 'application/json'
- The proxy's design allows for transparent forwarding of any future HTTP methods without modification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for HTTP method usage in the codebase
echo "Checking HTTP method usage in the frontend code..."
rg -i "method: ['\"](?:GET|POST|PUT|DELETE|PATCH)" --type ts
# Check for content type handling
echo "Checking content type handling..."
rg -i "content-type" --type ts
# Check for existing API endpoints
echo "Checking API endpoint patterns..."
rg -i "fetch\(.*?/api/" --type ts
Length of output: 829
Script:
#!/bin/bash
# Check for all HTTP method handlers in the API routes
echo "Checking HTTP method handlers in API routes..."
ast-grep --pattern 'export const $METHOD: RequestHandler = $_'
# Check the API client implementation
echo "Checking API client implementation..."
cat frontend/src/lib/api/api.ts
# Check API tests
echo "Checking API tests..."
cat frontend/src/lib/api/api.test.ts
Length of output: 5972
| import { env } from '$env/dynamic/private'; | ||
| import type { RequestHandler } from './$types'; | ||
|
|
||
| const API_BASE_URL = env.API_BASE_URL ?? 'http://localhost:9000'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for API_BASE_URL environment variable
The current implementation could benefit from additional validation and type safety:
- Validate the URL format
- Ensure HTTPS in production
- Add type safety for the environment variable
Consider applying this improvement:
import { env } from '$env/dynamic/private';
import type { RequestHandler } from './$types';
+import { z } from 'zod';
+
+const ApiBaseUrlSchema = z.string().url();
+const validateApiBaseUrl = (url: string) => {
+ const parsed = ApiBaseUrlSchema.parse(url);
+ if (process.env.NODE_ENV === 'production' && !parsed.startsWith('https://')) {
+ throw new Error('API_BASE_URL must use HTTPS in production');
+ }
+ return parsed;
+};
-const API_BASE_URL = env.API_BASE_URL ?? 'http://localhost:9000';
+const API_BASE_URL = validateApiBaseUrl(env.API_BASE_URL ?? 'http://localhost:9000');📝 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.
| import { env } from '$env/dynamic/private'; | |
| import type { RequestHandler } from './$types'; | |
| const API_BASE_URL = env.API_BASE_URL ?? 'http://localhost:9000'; | |
| import { env } from '$env/dynamic/private'; | |
| import type { RequestHandler } from './$types'; | |
| import { z } from 'zod'; | |
| const ApiBaseUrlSchema = z.string().url(); | |
| const validateApiBaseUrl = (url: string) => { | |
| const parsed = ApiBaseUrlSchema.parse(url); | |
| if (process.env.NODE_ENV === 'production' && !parsed.startsWith('https://')) { | |
| throw new Error('API_BASE_URL must use HTTPS in production'); | |
| } | |
| return parsed; | |
| }; | |
| const API_BASE_URL = validateApiBaseUrl(env.API_BASE_URL ?? 'http://localhost:9000'); |
| export const GET: RequestHandler = ({ params, url, request }) => { | ||
| return fetch(`${API_BASE_URL}/api/${params.path + url.search}`, request); | ||
| }; |
There was a problem hiding this comment.
Add error handling and request validation
The current implementation needs improvements in several areas:
- Path parameter validation
- Error handling
- Request timeout
- Response transformation
Consider this safer implementation:
-export const GET: RequestHandler = ({ params, url, request }) => {
- return fetch(`${API_BASE_URL}/api/${params.path + url.search}`, request);
-};
+export const GET: RequestHandler = async ({ params, url, request }) => {
+ try {
+ // Validate path to prevent directory traversal
+ if (params.path.includes('..')) {
+ return new Response('Invalid path', { status: 400 });
+ }
+
+ const response = await Promise.race([
+ fetch(`${API_BASE_URL}/api/${params.path + url.search}`, {
+ ...request,
+ signal: AbortSignal.timeout(30000) // 30s timeout
+ }),
+ new Promise((_, reject) =>
+ setTimeout(() => reject(new Error('Request timeout')), 30000)
+ )
+ ]);
+
+ // Optionally transform response
+ const data = await response.json();
+ return new Response(JSON.stringify(data), {
+ status: response.status,
+ headers: {
+ 'Content-Type': 'application/json'
+ }
+ });
+ } catch (error) {
+ console.error('Proxy error:', error);
+ return new Response('Internal Server Error', { status: 500 });
+ }
+};📝 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.
| export const GET: RequestHandler = ({ params, url, request }) => { | |
| return fetch(`${API_BASE_URL}/api/${params.path + url.search}`, request); | |
| }; | |
| export const GET: RequestHandler = async ({ params, url, request }) => { | |
| try { | |
| // Validate path to prevent directory traversal | |
| if (params.path.includes('..')) { | |
| return new Response('Invalid path', { status: 400 }); | |
| } | |
| const response = await Promise.race([ | |
| fetch(`${API_BASE_URL}/api/${params.path + url.search}`, { | |
| ...request, | |
| signal: AbortSignal.timeout(30000) // 30s timeout | |
| }), | |
| new Promise((_, reject) => | |
| setTimeout(() => reject(new Error('Request timeout')), 30000) | |
| ) | |
| ]); | |
| // Optionally transform response | |
| const data = await response.json(); | |
| return new Response(JSON.stringify(data), { | |
| status: response.status, | |
| headers: { | |
| 'Content-Type': 'application/json' | |
| } | |
| }); | |
| } catch (error) { | |
| console.error('Proxy error:', error); | |
| return new Response('Internal Server Error', { status: 500 }); | |
| } | |
| }; |
| export const POST: RequestHandler = ({ params, url, request }) => { | ||
| return fetch(`${API_BASE_URL}/api/${params.path + url.search}`, request); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication
The POST handler duplicates the GET handler's code. Consider extracting the common logic into a shared function.
Here's a suggested refactor:
+async function proxyRequest(request: Request, params: { path: string }, url: URL) {
+ try {
+ if (params.path.includes('..')) {
+ return new Response('Invalid path', { status: 400 });
+ }
+
+ const response = await Promise.race([
+ fetch(`${API_BASE_URL}/api/${params.path + url.search}`, {
+ ...request,
+ signal: AbortSignal.timeout(30000)
+ }),
+ new Promise((_, reject) =>
+ setTimeout(() => reject(new Error('Request timeout')), 30000)
+ )
+ ]);
+
+ const data = await response.json();
+ return new Response(JSON.stringify(data), {
+ status: response.status,
+ headers: {
+ 'Content-Type': 'application/json'
+ }
+ });
+ } catch (error) {
+ console.error('Proxy error:', error);
+ return new Response('Internal Server Error', { status: 500 });
+ }
+}
+
-export const POST: RequestHandler = ({ params, url, request }) => {
- return fetch(`${API_BASE_URL}/api/${params.path + url.search}`, request);
-};
+export const GET: RequestHandler = (event) => proxyRequest(event.request, event.params, event.url);
+export const POST: RequestHandler = (event) => proxyRequest(event.request, event.params, event.url);📝 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.
| export const POST: RequestHandler = ({ params, url, request }) => { | |
| return fetch(`${API_BASE_URL}/api/${params.path + url.search}`, request); | |
| }; | |
| async function proxyRequest(request: Request, params: { path: string }, url: URL) { | |
| try { | |
| if (params.path.includes('..')) { | |
| return new Response('Invalid path', { status: 400 }); | |
| } | |
| const response = await Promise.race([ | |
| fetch(`${API_BASE_URL}/api/${params.path + url.search}`, { | |
| ...request, | |
| signal: AbortSignal.timeout(30000) | |
| }), | |
| new Promise((_, reject) => | |
| setTimeout(() => reject(new Error('Request timeout')), 30000) | |
| ) | |
| ]); | |
| const data = await response.json(); | |
| return new Response(JSON.stringify(data), { | |
| status: response.status, | |
| headers: { | |
| 'Content-Type': 'application/json' | |
| } | |
| }); | |
| } catch (error) { | |
| console.error('Proxy error:', error); | |
| return new Response('Internal Server Error', { status: 500 }); | |
| } | |
| } | |
| export const GET: RequestHandler = (event) => proxyRequest(event.request, event.params, event.url); | |
| export const POST: RequestHandler = (event) => proxyRequest(event.request, event.params, event.url); |
sean-zlai
force-pushed
the
sean/api-proxy
branch
from
3e85046 to
9f13f9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/src/lib/api/api.test.ts (1)
35-39: Enhance test coverage for API interface.While the current tests cover basic GET functionality, consider adding tests for:
- POST requests with request body
- Authorization header when accessToken is provided
- Other API methods (getJoins, search, getJoinTimeseries, getFeatureTimeseries)
Would you like me to help generate additional test cases?
frontend/README.md (1)
33-37: Improve environment configuration documentation.The .env setup instructions are helpful, but consider:
- Mentioning that
API_BASE_URLdefaults tohttp://localhost:9000if not set- Specifying the language for the code block
- ``` + ```env API_BASE_URL=http://localhost:9000 ```🧰 Tools
🪛 Markdownlint (0.37.0)
35-35: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
frontend/src/lib/api/api.ts (3)
33-33: Address TODO comment with concrete plan.The TODO comment suggests uncertainty about code organization. Consider:
- Creating a dedicated
modelsdirectory underlib/api/- Moving model-specific API calls to
lib/api/models/api.ts- Using a facade pattern to expose a unified API interface
Would you like help implementing this architectural improvement?
38-52: Reduce code duplication in API methods.The URL parameter handling is duplicated across methods. Consider extracting common logic:
private createSearchParams(params: Record<string, string | number>): string { const searchParams = new URLSearchParams(); Object.entries(params).forEach(([key, value]) => { searchParams.append(key, value.toString()); }); return searchParams.toString(); }This would simplify the API methods:
async getJoins(offset: number = 0, limit: number = 10) { const params = this.createSearchParams({ offset, limit }); return this.#send<JoinsResponse>(`joins?${params}`); }Also applies to: 54-81, 83-116
15-20: Improve type safety of ApiRequestOptions.The current type allows any value in the data object:
export type ApiRequestOptions = { method?: 'GET' | 'POST' | 'PUT' | 'DELETE'; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - data?: Record<string, any>; + data?: Record<string, string | number | boolean | null>; headers?: Record<string, string>; };frontend/src/routes/joins/[slug]/+page.svelte (3)
Line range hint
204-215: Consider extracting common API parameters.The API calls share several common parameters. Consider extracting these into a configuration object to improve maintainability and reduce duplication.
+const commonParams = { + joinId: joinTimeseries.name, + startTs: data.dateRange.startTimestamp, + endTs: data.dateRange.endTimestamp, + metricType: 'drift', + algorithm: 'psi', + granularity: 'percentile' +}; const [featureData, nullFeatureData] = await Promise.all([ api.getFeatureTimeseries({ - joinId: joinTimeseries.name, - featureName: seriesName, - startTs: data.dateRange.startTimestamp, - endTs: data.dateRange.endTimestamp, - granularity: 'percentile', - metricType: 'drift', - metrics: 'value', - offset: '1D', - algorithm: 'psi' + ...commonParams, + featureName: seriesName, + metrics: 'value', + offset: '1D' }), api.getFeatureTimeseries({ - joinId: joinTimeseries.name, - featureName: seriesName, - startTs: data.dateRange.startTimestamp, - endTs: data.dateRange.endTimestamp, - metricType: 'drift', - metrics: 'null', - offset: '1D', - algorithm: 'psi', - granularity: 'percentile' + ...commonParams, + featureName: seriesName, + metrics: 'null', + offset: '1D' }) ]);
Line range hint
424-434: Enhance error handling in loadDistributions.While basic error handling is present, consider providing more specific error feedback to users and implementing retry logic for transient failures.
try { const responses = await Promise.all(distributionsPromises); distributions = responses.filter((response) => response.isNumeric); } catch (error) { - console.error('Error loading distributions:', error); + console.error('Failed to load feature distributions:', error); + // Consider implementing retry logic for transient failures + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + // Consider showing a user-friendly error notification + notifications.error(`Failed to load distributions: ${errorMessage}`); } finally { isLoadingDistributions = false; }
Line range hint
1-724: Consider breaking down the component for better maintainability.The component handles multiple responsibilities and could benefit from being split into smaller, more focused components:
- Chart rendering logic could be moved to separate components
- Data fetching logic could be extracted into a custom store or hook
- Event handling could be centralized in a dedicated service
This would improve maintainability, testability, and reusability.
Example structure:
// stores/featureMonitoring.ts export const createFeatureMonitoringStore = () => { // Data fetching and state management logic }; // components/charts/FeatureChart.svelte // Chart rendering logic // services/chartEventHandler.ts export class ChartEventHandler { // Event handling logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (9)
frontend/README.md(1 hunks)frontend/src/lib/api/api.test.ts(4 hunks)frontend/src/lib/api/api.ts(1 hunks)frontend/src/lib/components/NavigationBar/NavigationBar.svelte(2 hunks)frontend/src/lib/types/Model/Model.test.ts(1 hunks)frontend/src/routes/api/[...path]/+server.ts(1 hunks)frontend/src/routes/joins/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(6 hunks)frontend/src/routes/joins/[slug]/+page.svelte(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/lib/components/NavigationBar/NavigationBar.svelte
- frontend/src/routes/joins/+page.server.ts
- frontend/src/routes/api/[...path]/+server.ts
- frontend/src/routes/joins/[slug]/+page.server.ts
- frontend/src/lib/types/Model/Model.test.ts
🧰 Additional context used
🪛 Markdownlint (0.37.0)
frontend/README.md
35-35: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
frontend/src/lib/api/api.test.ts (1)
9-9: LGTM! Api class instantiation is correctly configured.
The test setup properly instantiates the Api class with a mocked fetch function.
frontend/src/lib/api/api.ts (1)
118-154: 🛠️ Refactor suggestion
Enhance error handling in #send method.
The current error handling could be improved:
- Network errors aren't explicitly handled
- JSON parse errors only log to console
- Error responses might contain useful information
Consider this enhanced implementation:
async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) {
let url = `${this.#base}/${resource}`;
const method = options?.method ?? 'GET';
if (method === 'GET' && options?.data) {
url += `?${new URLSearchParams(options.data)}`;
}
- return this.#fetch(url, {
+ try {
+ const response = await this.#fetch(url, {
method: options?.method ?? 'GET',
headers: {
'Content-Type': 'application/json',
...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` })
},
...(method === 'POST' &&
options?.data && {
body: JSON.stringify(options.data)
})
- }).then(async (response) => {
- if (response.ok) {
+ });
+ if (response.ok) {
const text = await response.text();
try {
if (text) {
return JSON.parse(text) as Data;
} else {
return {};
}
} catch (e) {
- console.log(`Unable to parse: "${text}" for url: ${url}`);
- throw e;
+ throw new Error(`Invalid JSON response from ${url}: ${text}`);
}
} else {
- error(response.status);
+ const errorText = await response.text();
+ throw error(response.status, {
+ message: `API error: ${errorText || response.statusText}`
+ });
}
- });
+ } catch (e) {
+ if (e instanceof Error) {
+ throw e;
+ }
+ throw new Error(`Network error while fetching ${url}: ${e}`);
+ }
}Likely invalid or redundant comment.
frontend/src/routes/joins/[slug]/+page.svelte (1)
19-19: LGTM: Clean API integration.
The integration of the new Api class is clean and follows the singleton pattern appropriately.
Also applies to: 35-35
…d({ fetch })`), improved response parsing, and future improvements (`Authorization` header for access tokens, etc)
… `app:9000`) to fix CORS issues and provide consistent URL handling regardless of env (browser / `+page.svelte`, server load() / `+page.server.ts`, etc)
sean-zlai
force-pushed
the
sean/api-proxy
branch
from
9f13f9f to
4d6fe92
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
frontend/src/lib/api/api.test.ts (1)
Line range hint
1-65: Enhance test coverageWhile the basic functionality is tested, consider adding tests for:
- Authorization header handling
- Timeout scenarios
- Network errors
- Edge cases for parameter validation
it('should include authorization header when token is provided', async () => { const api = new Api({ fetch: mockFetch, accessToken: 'test-token' }); mockFetch.mockResolvedValueOnce({ ok: true, text: () => Promise.resolve('{}') }); await api.getModels(); expect(mockFetch).toHaveBeenCalledWith( expect.any(String), expect.objectContaining({ headers: expect.objectContaining({ 'Authorization': 'Bearer test-token' }) }) ); });
♻️ Duplicate comments (1)
frontend/src/lib/api/api.ts (1)
118-154:⚠️ Potential issueEnhance error handling in #send method
The current error handling could be improved:
- Add timeout handling
- Implement retry logic for transient failures
- Include response body in error messages
async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) { let url = `${this.#base}/${resource}`; + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 30000); const method = options?.method ?? 'GET'; if (method === 'GET' && options?.data) { url += `?${new URLSearchParams(options.data)}`; } - return this.#fetch(url, { + try { + const response = await this.#fetch(url, { + signal: controller.signal, method: options?.method ?? 'GET', headers: { 'Content-Type': 'application/json', ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` }) }, ...(method === 'POST' && options?.data && { body: JSON.stringify(options.data) }) - }).then(async (response) => { - if (response.ok) { + }); + clearTimeout(timeout); + + if (response.ok) { const text = await response.text(); try { if (text) { return JSON.parse(text) as Data; } else { return {}; } } catch (e) { console.log(`Unable to parse: "${text}" for url: ${url}`); throw e; } } else { - error(response.status); + const errorBody = await response.text(); + error(response.status, { + message: `API request failed: ${errorBody}` + }); } - }); + } catch (e) { + clearTimeout(timeout); + if (e.name === 'AbortError') { + throw new Error('Request timeout'); + } + throw e; + } }
🧹 Nitpick comments (4)
frontend/src/lib/api/api.ts (3)
9-20: LGTM! Consider strengthening type definitionsThe type definitions are well-structured. Consider these enhancements:
- Make
methodinApiRequestOptionsrequired since it has a default in#send- Replace
Record<string, any>with a more specific type fordataexport type ApiRequestOptions = { - method?: 'GET' | 'POST' | 'PUT' | 'DELETE'; + method: 'GET' | 'POST' | 'PUT' | 'DELETE'; // eslint-disable-next-line @typescript-eslint/no-explicit-any - data?: Record<string, any>; + data?: Record<string, string | number | boolean>; headers?: Record<string, string>; };
33-34: Address TODO comment about project structureConsider moving API calls to a dedicated models directory (e.g.,
src/lib/api/models/) to improve maintainability.Would you like me to help create a proposal for the API code organization structure?
38-44: Consider adding parameter validationAdd runtime validation for offset and limit parameters to ensure they are positive numbers.
async getJoins(offset: number = 0, limit: number = 10) { + if (offset < 0 || limit <= 0) { + throw new Error('Offset must be non-negative and limit must be positive'); + } const params = new URLSearchParams({ offset: offset.toString(), limit: limit.toString() }); return this.#send<JoinsResponse>(`joins?${params.toString()}`); }frontend/README.md (1)
33-37: Improve environment configuration documentation
- Add language specification to the code block
- Provide more context about the API configuration
4. Create `.env` file in frontend directory with the follow - ``` + ```env API_BASE_URL=http://localhost:9000 ``` + + Note: If `API_BASE_URL` is not defined, it will default to `http://localhost:9000`. + This configuration is used by the frontend server to proxy API requests.🧰 Tools
🪛 Markdownlint (0.37.0)
35-35: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (9)
frontend/README.md(1 hunks)frontend/src/lib/api/api.test.ts(4 hunks)frontend/src/lib/api/api.ts(1 hunks)frontend/src/lib/components/NavigationBar/NavigationBar.svelte(2 hunks)frontend/src/lib/types/Model/Model.test.ts(1 hunks)frontend/src/routes/api/[...path]/+server.ts(1 hunks)frontend/src/routes/joins/+page.server.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.server.ts(6 hunks)frontend/src/routes/joins/[slug]/+page.svelte(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/lib/components/NavigationBar/NavigationBar.svelte
- frontend/src/lib/types/Model/Model.test.ts
- frontend/src/routes/api/[...path]/+server.ts
- frontend/src/routes/joins/[slug]/+page.svelte
- frontend/src/routes/joins/[slug]/+page.server.ts
- frontend/src/routes/joins/+page.server.ts
🧰 Additional context used
🪛 Markdownlint (0.37.0)
frontend/README.md
35-35: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| async getJoinTimeseries({ | ||
| joinId, | ||
| startTs, | ||
| endTs, | ||
| metricType = 'drift', | ||
| metrics = 'null', | ||
| offset = '10h', | ||
| algorithm = 'psi' | ||
| }: { | ||
| joinId: string; | ||
| startTs: number; | ||
| endTs: number; | ||
| metricType?: string; | ||
| metrics?: string; | ||
| offset?: string; | ||
| algorithm?: string; | ||
| }) { | ||
| const params = new URLSearchParams({ | ||
| startTs: startTs.toString(), | ||
| endTs: endTs.toString(), | ||
| metricType, | ||
| metrics, | ||
| offset, | ||
| algorithm | ||
| }); | ||
|
|
||
| export async function search(term: string, limit: number = 20): Promise<ModelsResponse> { | ||
| const params = new URLSearchParams({ | ||
| term, | ||
| limit: limit.toString() | ||
| }); | ||
| return get(`search?${params.toString()}`); | ||
| } | ||
| return this.#send<JoinTimeSeriesResponse>(`join/${joinId}/timeseries?${params.toString()}`); | ||
| } | ||
|
|
||
| async getFeatureTimeseries({ | ||
| joinId, | ||
| featureName, | ||
| startTs, | ||
| endTs, | ||
| metricType = 'drift', | ||
| metrics = 'null', | ||
| offset = '10h', | ||
| algorithm = 'psi', | ||
| granularity = 'aggregates' | ||
| }: { | ||
| joinId: string; | ||
| featureName: string; | ||
| startTs: number; | ||
| endTs: number; | ||
| metricType?: string; | ||
| metrics?: string; | ||
| offset?: string; | ||
| algorithm?: string; | ||
| granularity?: string; | ||
| }) { | ||
| const params = new URLSearchParams({ | ||
| startTs: startTs.toString(), | ||
| endTs: endTs.toString(), | ||
| metricType, | ||
| metrics, | ||
| offset, | ||
| algorithm, | ||
| granularity | ||
| }); | ||
| return this.#send<FeatureResponse>( | ||
| `join/${joinId}/feature/${featureName}/timeseries?${params.toString()}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication and improve type safety
The timeseries methods have significant duplication in parameter handling. Consider:
- Creating shared types for common parameters
- Extracting parameter handling to a utility function
type TimeseriesParams = {
startTs: number;
endTs: number;
metricType?: string;
metrics?: string;
offset?: string;
algorithm?: string;
};
function createTimeseriesParams(params: TimeseriesParams): URLSearchParams {
return new URLSearchParams({
startTs: params.startTs.toString(),
endTs: params.endTs.toString(),
metricType: params.metricType ?? 'drift',
metrics: params.metrics ?? 'null',
offset: params.offset ?? '10h',
algorithm: params.algorithm ?? 'psi'
});
}| async search(term: string, limit: number = 20) { | ||
| const params = new URLSearchParams({ | ||
| term, | ||
| limit: limit.toString() | ||
| }); | ||
| return this.#send<ModelsResponse>(`search?${params.toString()}`); | ||
| } |
There was a problem hiding this comment.
Add input validation for search parameters
The search term should be validated and sanitized before being used in the query.
async search(term: string, limit: number = 20) {
+ if (!term?.trim()) {
+ throw new Error('Search term cannot be empty');
+ }
+ if (limit <= 0) {
+ throw new Error('Limit must be positive');
+ }
const params = new URLSearchParams({
- term,
+ term: term.trim(),
limit: limit.toString()
});
return this.#send<ModelsResponse>(`search?${params.toString()}`);
}📝 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.
| async search(term: string, limit: number = 20) { | |
| const params = new URLSearchParams({ | |
| term, | |
| limit: limit.toString() | |
| }); | |
| return this.#send<ModelsResponse>(`search?${params.toString()}`); | |
| } | |
| async search(term: string, limit: number = 20) { | |
| if (!term?.trim()) { | |
| throw new Error('Search term cannot be empty'); | |
| } | |
| if (limit <= 0) { | |
| throw new Error('Limit must be positive'); | |
| } | |
| const params = new URLSearchParams({ | |
| term: term.trim(), | |
| limit: limit.toString() | |
| }); | |
| return this.#send<ModelsResponse>(`search?${params.toString()}`); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/lib/api/api.ts (1)
54-116: 🛠️ Refactor suggestionReduce code duplication in timeseries methods
The
getJoinTimeseriesandgetFeatureTimeseriesmethods share significant parameter handling logic. Consider extracting common code:type BaseTimeseriesParams = { startTs: number; endTs: number; metricType?: string; metrics?: string; offset?: string; algorithm?: string; }; type JoinTimeseriesParams = BaseTimeseriesParams & { joinId: string; }; type FeatureTimeseriesParams = JoinTimeseriesParams & { featureName: string; granularity?: string; }; private createTimeseriesParams(params: BaseTimeseriesParams): URLSearchParams { return new URLSearchParams({ startTs: params.startTs.toString(), endTs: params.endTs.toString(), metricType: params.metricType ?? 'drift', metrics: params.metrics ?? 'null', offset: params.offset ?? '10h', algorithm: params.algorithm ?? 'psi' }); }
🧹 Nitpick comments (3)
frontend/src/lib/api/api.ts (3)
9-20: Consider type improvements for better type safety
- The
fetchtype could be more precise:- fetch?: typeof fetch; + fetch?: WindowOrWorkerGlobalScope['fetch'];
- The
datafield in ApiRequestOptions could use a more specific type thanany:- // eslint-disable-next-line @typescript-eslint/no-explicit-any - data?: Record<string, any>; + data?: Record<string, unknown>;
27-30: Add URL validation in constructorConsider validating the base URL to ensure it doesn't contain trailing slashes that could cause double-slash issues:
constructor(opts: ApiOptions = {}) { - this.#base = opts.base ?? '/api/v1'; + const base = opts.base ?? '/api/v1'; + this.#base = base.endsWith('/') ? base.slice(0, -1) : base; this.#fetch = opts.fetch ?? fetch; this.#accessToken = opts.accessToken; }
33-33: Address TODO comment about project organizationConsider moving model-specific API calls to a dedicated ModelApi class that extends the base Api class. This would improve organization and maintainability.
Would you like me to help create a proposal for the API organization structure?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/lib/api/api.ts(1 hunks)
🔇 Additional comments (1)
frontend/src/lib/api/api.ts (1)
22-155: Verify API usage across the codebase
Since this is a significant refactor of the API interface, let's verify its usage across the codebase.
✅ Verification successful
Let me analyze the imports and usage patterns to verify if we need more information.
API usage is consistent with the new interface
The verification shows that:
- All API usage follows the new interface pattern with proper instantiation of the
Apiclass - Direct
fetchusage is only present in the API proxy route ([...path]/+server.ts), which is expected - All API method calls (
getModels,getJoins,search,getJoinTimeseries,getFeatureTimeseries) match the new interface - Error handling is properly implemented in the consuming components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify API usage patterns across the codebase
# Find direct fetch usage that should be using the new Api class
echo "Checking for direct fetch usage..."
rg --type-add 'frontend:*.{ts,js,svelte}' -t frontend 'fetch\(' -g '!api.ts'
# Find old API function imports that should be updated
echo "Checking for old API imports..."
rg --type-add 'frontend:*.{ts,js,svelte}' -t frontend 'import.*from.*api' -g '!api.ts'
# Find potential places where error handling might need updates
echo "Checking error handling patterns..."
rg --type-add 'frontend:*.{ts,js,svelte}' -t frontend 'error\(' -g '!api.ts'
Length of output: 1502
Script:
#!/bin/bash
# Let's check the actual usage of the Api class in these files to ensure they're using the new methods correctly
echo "Checking Api class usage patterns..."
rg --type-add 'frontend:*.{ts,js,svelte}' -t frontend -A 5 'new Api\('
echo "Checking specific API method calls..."
rg --type-add 'frontend:*.{ts,js,svelte}' -t frontend -A 2 'api\.(getModels|getJoins|search|getJoinTimeseries|getFeatureTimeseries)'
Length of output: 6423
| async getJoins(offset: number = 0, limit: number = 10) { | ||
| const params = new URLSearchParams({ | ||
| offset: offset.toString(), | ||
| limit: limit.toString() | ||
| }); | ||
| return this.#send<JoinsResponse>(`joins?${params.toString()}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for pagination and search parameters
Both getJoins and search methods need input validation:
async getJoins(offset: number = 0, limit: number = 10) {
+ if (offset < 0) throw new Error('Offset must be non-negative');
+ if (limit <= 0) throw new Error('Limit must be positive');
const params = new URLSearchParams({
offset: offset.toString(),
limit: limit.toString()
});
return this.#send<JoinsResponse>(`joins?${params.toString()}`);
}
async search(term: string, limit: number = 20) {
+ if (!term?.trim()) throw new Error('Search term cannot be empty');
+ if (limit <= 0) throw new Error('Limit must be positive');
const params = new URLSearchParams({
- term,
+ term: term.trim(),
limit: limit.toString()
});
return this.#send<ModelsResponse>(`search?${params.toString()}`);
}Also applies to: 46-52
| async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) { | ||
| let url = `${this.#base}/${resource}`; | ||
|
|
||
| return get(`join/${joinId}/timeseries?${params.toString()}`); | ||
| } | ||
| const method = options?.method ?? 'GET'; | ||
|
|
||
| export async function getFeatureTimeseries({ | ||
| joinId, | ||
| featureName, | ||
| startTs, | ||
| endTs, | ||
| metricType = 'drift', | ||
| metrics = 'null', | ||
| offset = '10h', | ||
| algorithm = 'psi', | ||
| granularity = 'aggregates' | ||
| }: { | ||
| joinId: string; | ||
| featureName: string; | ||
| startTs: number; | ||
| endTs: number; | ||
| metricType?: string; | ||
| metrics?: string; | ||
| offset?: string; | ||
| algorithm?: string; | ||
| granularity?: string; | ||
| }): Promise<FeatureResponse> { | ||
| const params = new URLSearchParams({ | ||
| startTs: startTs.toString(), | ||
| endTs: endTs.toString(), | ||
| metricType, | ||
| metrics, | ||
| offset, | ||
| algorithm, | ||
| granularity | ||
| }); | ||
| return get(`join/${joinId}/feature/${featureName}/timeseries?${params.toString()}`); | ||
| if (method === 'GET' && options?.data) { | ||
| url += `?${new URLSearchParams(options.data)}`; | ||
| } | ||
|
|
||
| return this.#fetch(url, { | ||
| method: options?.method ?? 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` }) | ||
| }, | ||
| ...(method === 'POST' && | ||
| options?.data && { | ||
| body: JSON.stringify(options.data) | ||
| }) | ||
| }).then(async (response) => { | ||
| if (response.ok) { | ||
| const text = await response.text(); | ||
| try { | ||
| if (text) { | ||
| return JSON.parse(text) as Data; | ||
| } else { | ||
| // TODO: Should we return `null` here and require users to handle | ||
| return {} as Data; | ||
| } | ||
| } catch (e) { | ||
| console.log(`Unable to parse: "${text}" for url: ${url}`); | ||
| throw e; | ||
| } | ||
| } else { | ||
| error(response.status); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Enhance error handling and response processing in #send method
Several improvements are needed:
- Better error handling for non-OK responses
- Proper typing for empty responses
- Simplified headers handling
async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) {
let url = `${this.#base}/${resource}`;
const method = options?.method ?? 'GET';
+ const headers = {
+ 'Content-Type': 'application/json',
+ ...options?.headers,
+ ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` })
+ };
if (method === 'GET' && options?.data) {
url += `?${new URLSearchParams(options.data)}`;
}
return this.#fetch(url, {
method,
- headers: {
- 'Content-Type': 'application/json',
- ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` })
- },
+ headers,
...(method === 'POST' &&
options?.data && {
body: JSON.stringify(options.data)
})
}).then(async (response) => {
if (response.ok) {
const text = await response.text();
try {
if (text) {
return JSON.parse(text) as Data;
} else {
- // TODO: Should we return `null` here and require users to handle
- return {} as Data;
+ return null as unknown as Data;
}
} catch (e) {
- console.log(`Unable to parse: "${text}" for url: ${url}`);
+ const error = new Error(`Unable to parse response: "${text}" for url: ${url}`);
+ console.error(error);
+ throw error;
}
} else {
- error(response.status);
+ const errorText = await response.text();
+ throw error(response.status, {
+ message: errorText || `Request failed with status ${response.status}`
+ });
}
});
}📝 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.
| async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) { | |
| let url = `${this.#base}/${resource}`; | |
| return get(`join/${joinId}/timeseries?${params.toString()}`); | |
| } | |
| const method = options?.method ?? 'GET'; | |
| export async function getFeatureTimeseries({ | |
| joinId, | |
| featureName, | |
| startTs, | |
| endTs, | |
| metricType = 'drift', | |
| metrics = 'null', | |
| offset = '10h', | |
| algorithm = 'psi', | |
| granularity = 'aggregates' | |
| }: { | |
| joinId: string; | |
| featureName: string; | |
| startTs: number; | |
| endTs: number; | |
| metricType?: string; | |
| metrics?: string; | |
| offset?: string; | |
| algorithm?: string; | |
| granularity?: string; | |
| }): Promise<FeatureResponse> { | |
| const params = new URLSearchParams({ | |
| startTs: startTs.toString(), | |
| endTs: endTs.toString(), | |
| metricType, | |
| metrics, | |
| offset, | |
| algorithm, | |
| granularity | |
| }); | |
| return get(`join/${joinId}/feature/${featureName}/timeseries?${params.toString()}`); | |
| if (method === 'GET' && options?.data) { | |
| url += `?${new URLSearchParams(options.data)}`; | |
| } | |
| return this.#fetch(url, { | |
| method: options?.method ?? 'GET', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` }) | |
| }, | |
| ...(method === 'POST' && | |
| options?.data && { | |
| body: JSON.stringify(options.data) | |
| }) | |
| }).then(async (response) => { | |
| if (response.ok) { | |
| const text = await response.text(); | |
| try { | |
| if (text) { | |
| return JSON.parse(text) as Data; | |
| } else { | |
| // TODO: Should we return `null` here and require users to handle | |
| return {} as Data; | |
| } | |
| } catch (e) { | |
| console.log(`Unable to parse: "${text}" for url: ${url}`); | |
| throw e; | |
| } | |
| } else { | |
| error(response.status); | |
| } | |
| }); | |
| } | |
| async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) { | |
| let url = `${this.#base}/${resource}`; | |
| const method = options?.method ?? 'GET'; | |
| const headers = { | |
| 'Content-Type': 'application/json', | |
| ...options?.headers, | |
| ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` }) | |
| }; | |
| if (method === 'GET' && options?.data) { | |
| url += `?${new URLSearchParams(options.data)}`; | |
| } | |
| return this.#fetch(url, { | |
| method, | |
| headers, | |
| ...(method === 'POST' && | |
| options?.data && { | |
| body: JSON.stringify(options.data) | |
| }) | |
| }).then(async (response) => { | |
| if (response.ok) { | |
| const text = await response.text(); | |
| try { | |
| if (text) { | |
| return JSON.parse(text) as Data; | |
| } else { | |
| return null as unknown as Data; | |
| } | |
| } catch (e) { | |
| const error = new Error(`Unable to parse response: "${text}" for url: ${url}`); | |
| console.error(error); | |
| throw error; | |
| } | |
| } else { | |
| const errorText = await response.text(); | |
| throw error(response.status, { | |
| message: errorText || `Request failed with status ${response.status}` | |
| }); | |
| } | |
| }); | |
| } |
…entify root cause
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/src/lib/api/api.ts (3)
38-44:⚠️ Potential issueAdd input validation for pagination and search parameters
The input validation suggestions from the previous review are still relevant.
Also applies to: 46-52
54-116: 🛠️ Refactor suggestionReduce code duplication in timeseries methods
The timeseries methods share similar parameter handling logic and could be refactored.
118-157:⚠️ Potential issueEnhance error handling and response processing
The error handling suggestions from the previous review are still relevant. Additionally:
- The TODO comment about null handling needs to be addressed
- Error handling should be more informative
Also, consider implementing request timeout handling:
async #send<Data = unknown>(resource: string, options?: ApiRequestOptions) { let url = `${this.#base}/${resource}`; const method = options?.method ?? 'GET'; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30s timeout try { return await this.#fetch(url, { method, headers: { 'Content-Type': 'application/json', ...(this.#accessToken && { Authorization: `Bearer ${this.#accessToken}` }) }, + signal: controller.signal, ...(method === 'POST' && options?.data && { body: JSON.stringify(options.data) }) }).then(/* ... */); } finally { + clearTimeout(timeoutId); } }
🧹 Nitpick comments (3)
frontend/src/lib/api/api.ts (3)
15-20: Consider using a more specific type instead ofanyReplace the
anytype with a more specific type to improve type safety. Consider usingunknownor defining a union type of expected values.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - data?: Record<string, any>; + data?: Record<string, string | number | boolean | null>;
22-31: Add JSDoc documentation for the Api class and its constructorAdding comprehensive documentation will improve maintainability and help other developers understand the purpose and usage of this class.
+/** + * Api class for handling HTTP requests to the backend. + * Supports custom fetch implementation and configurable base URL. + */ export class Api { #base: string; #fetch: typeof fetch; #accessToken: string | undefined; + /** + * @param opts - Configuration options for the API + * @param opts.base - Base URL for API requests (defaults to '/api/v1') + * @param opts.fetch - Custom fetch implementation (defaults to global fetch) + * @param opts.accessToken - Optional authentication token + */ constructor(opts: ApiOptions = {}) {
33-34: Address the TODO comment about project structureConsider moving model-specific API calls to a dedicated ModelApi class that extends the base Api class. This would improve organization and maintainability.
Would you like me to help create a proposal for the API structure reorganization?
Summary
Apiinterfacefetchto allow relative URLs from SvelteKit load() functionsfetch(window.fetch on browser, global fetch on Node.js)Dateinstances (usingparsefrom @layerstack/utils)Authorizationheader for access tokenshttp://localhost:5173/api/*=>http://localhost:9000/api/v1/*http://app.zipline.ai/api/*=>http://app.zipline.ai:9000/api/v1/*+page.svelte) or frontend server (ex+page.server.ts)Test results
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
.envfile for configuration.Improvements
Apiclass, improving organization and encapsulation of API interactions.Documentation