Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions frontend/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,17 @@ The frontend for Chronon.
```

3. Install dependencies:

```bash
npm install
```

4. Create `.env` file in frontend directory with the follow

```
API_BASE_URL=http://localhost:9000
```

### Development

To start the development server:
Expand Down
16 changes: 10 additions & 6 deletions frontend/src/lib/api/api.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { get } from './api';
import { Api } from './api';
import { error } from '@sveltejs/kit';

// Mock the fetch function
const mockFetch = vi.fn();
global.fetch = mockFetch;

const api = new Api({ fetch });

// Mock the error function from @sveltejs/kit
vi.mock('@sveltejs/kit', () => ({
error: vi.fn()
Expand All @@ -28,11 +30,13 @@ describe('API module', () => {
text: () => Promise.resolve(JSON.stringify(mockResponse))
});

const result = await get('test-path');
const result = await api.getModels();

expect(mockFetch).toHaveBeenCalledWith(`http://localhost:9000/api/v1/test-path`, {
expect(mockFetch).toHaveBeenCalledWith(`/api/v1/models`, {
method: 'GET',
headers: {}
headers: {
'Content-Type': 'application/json'
}
});
expect(result).toEqual(mockResponse);
});
Expand All @@ -43,7 +47,7 @@ describe('API module', () => {
text: () => Promise.resolve('')
});

const result = await get('empty-path');
const result = await api.getModels();

expect(result).toEqual({});
});
Expand All @@ -54,7 +58,7 @@ describe('API module', () => {
status: 404
});

await get('error-path');
await api.getModels();

expect(error).toHaveBeenCalledWith(404);
});
Expand Down
231 changes: 138 additions & 93 deletions frontend/src/lib/api/api.ts
Original file line number Diff line number Diff line change
@@ -1,113 +1,158 @@
import { error } from '@sveltejs/kit';
import type {
FeatureResponse,
JoinsResponse,
JoinTimeSeriesResponse,
ModelsResponse
} from '$lib/types/Model/Model';
import { error } from '@sveltejs/kit';
import { browser } from '$app/environment';

const apiBaseUrl = !browser
? process?.env?.API_BASE_URL || 'http://localhost:9000'
: 'http://localhost:9000';
export type ApiOptions = {
base?: string;
fetch?: typeof fetch;
accessToken?: string;
};

const base = `${apiBaseUrl}/api/v1`;
export type ApiRequestOptions = {
method?: 'GET' | 'POST' | 'PUT' | 'DELETE';
// eslint-disable-next-line @typescript-eslint/no-explicit-any
data?: Record<string, any>;
headers?: Record<string, string>;
};

async function send({ method, path }: { method: string; path: string }) {
const opts = { method, headers: {} };
export class Api {
#base: string;
#fetch: typeof fetch;
#accessToken: string | undefined;

const res = await fetch(`${base}/${path}`, opts);
if (res.ok) {
const text = await res.text();
return text ? JSON.parse(text) : {};
constructor(opts: ApiOptions = {}) {
this.#base = opts.base ?? '/api/v1';
this.#fetch = opts.fetch ?? fetch; // default to global fetch (browser and node)
this.#accessToken = opts.accessToken;
}

error(res.status);
}
// TODO: eventually move this to a model-specific file/decide on a good project structure for organizing api calls
async getModels() {
return this.#send<ModelsResponse>('models');
}

export function get(path: string) {
return send({ method: 'GET', path });
}
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()}`);
}
Comment on lines +38 to +44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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


// todo: eventually move this to a model-specific file/decide on a good project structure for organizing api calls
export async function getModels(): Promise<ModelsResponse> {
return get('models');
}
async search(term: string, limit: number = 20) {
const params = new URLSearchParams({
term,
limit: limit.toString()
});
return this.#send<ModelsResponse>(`search?${params.toString()}`);
}
Comment on lines +46 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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()}`);
}


export async function getJoins(offset: number = 0, limit: number = 10): Promise<JoinsResponse> {
const params = new URLSearchParams({
offset: offset.toString(),
limit: limit.toString()
});
return get(`joins?${params.toString()}`);
}
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()}`
);
}
Comment on lines +54 to +116
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Reduce code duplication and improve type safety

The timeseries methods have significant duplication in parameter handling. Consider:

  1. Creating shared types for common parameters
  2. 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'
  });
}


export async function 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;
}): Promise<JoinTimeSeriesResponse> {
const params = new URLSearchParams({
startTs: startTs.toString(),
endTs: endTs.toString(),
metricType,
metrics,
offset,
algorithm
});
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.error(`Unable to parse: "${text}" for url: ${url}`);
throw e;
}
} else {
const text = (await response.text?.()) ?? '';
console.error(`Failed request: "${text}" for url: ${url}`);
error(response.status);
}
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
CommandItem,
CommandEmpty
} from '$lib/components/ui/command/';
import { search } from '$lib/api/api';
import { Api } from '$lib/api/api';
import type { Model } from '$lib/types/Model/Model';
import debounce from 'lodash/debounce';
import { onDestroy, onMount } from 'svelte';
Expand Down Expand Up @@ -46,9 +46,11 @@
let searchResults: Model[] = $state([]);
let isMac: boolean | undefined = $state(undefined);

const api = new Api();

const debouncedSearch = debounce(async () => {
if (input.length > 0) {
const response = await search(input);
const response = await api.search(input);
searchResults = response.items;
} else {
searchResults = [];
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/lib/types/Model/Model.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { describe, it, expect } from 'vitest';
import * as api from '$lib/api/api';
import { Api } from '$lib/api/api';
import type { ModelsResponse, Model, JoinTimeSeriesResponse } from '$lib/types/Model/Model';

const api = new Api({ base: 'http://localhost:9000/api/v1' });

describe('Model types', () => {
it('should match ModelsResponse type', async () => {
const result = (await api.getModels()) as ModelsResponse;
Expand Down
17 changes: 17 additions & 0 deletions frontend/src/routes/api/[...path]/+server.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { env } from '$env/dynamic/private';
import type { RequestHandler } from './$types';

const API_BASE_URL = env.API_BASE_URL ?? 'http://localhost:9000';
Comment on lines +1 to +4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for API_BASE_URL environment variable

The current implementation could benefit from additional validation and type safety:

  1. Validate the URL format
  2. Ensure HTTPS in production
  3. 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.

Suggested change
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');


/**
* 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`)
*/
export const GET: RequestHandler = ({ params, url, request }) => {
return fetch(`${API_BASE_URL}/api/${params.path + url.search}`, request);
};
Comment on lines +11 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling and request validation

The current implementation needs improvements in several areas:

  1. Path parameter validation
  2. Error handling
  3. Request timeout
  4. 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.

Suggested change
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);
};
Comment on lines +15 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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);

5 changes: 3 additions & 2 deletions frontend/src/routes/joins/+page.server.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import type { PageServerLoad } from './$types';
import type { JoinsResponse } from '$lib/types/Model/Model';
import * as api from '$lib/api/api';
import { Api } from '$lib/api/api';

export const load: PageServerLoad = async (): Promise<{ joins: JoinsResponse }> => {
export const load: PageServerLoad = async ({ fetch }): Promise<{ joins: JoinsResponse }> => {
const offset = 0;
const limit = 100;
const api = new Api({ fetch });
return {
joins: await api.getJoins(offset, limit)
};
Expand Down
Loading
Loading