-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add request deduplication to API client #84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| import { test, expect } from '@playwright/test' | ||
|
|
||
| test.describe('API Request Deduplication', () => { | ||
| test('should not make duplicate concurrent requests on homepage', async ({ page }) => { | ||
| const requests: { url: string; timestamp: number }[] = [] | ||
|
|
||
| // Track all API requests | ||
| page.on('request', (request) => { | ||
| const url = request.url() | ||
| if (url.includes('/products') || url.includes('/categories')) { | ||
| requests.push({ | ||
| url: url.replace(/^https?:\/\/[^/]+/, ''), // Remove base URL for easier comparison | ||
| timestamp: Date.now(), | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| await page.goto('/') | ||
|
|
||
| // Wait for page to fully load | ||
| await expect(page.locator('[data-testid="product-card"]').first()).toBeVisible({ | ||
| timeout: 10000, | ||
| }) | ||
|
|
||
| // Wait a bit more for any delayed requests | ||
| await page.waitForTimeout(2000) | ||
|
|
||
| // Group requests by URL | ||
| const requestsByUrl = requests.reduce( | ||
| (acc, req) => { | ||
| acc[req.url] = (acc[req.url] || 0) + 1 | ||
| return acc | ||
| }, | ||
| {} as Record<string, number> | ||
| ) | ||
|
|
||
| // Check for specific duplicate patterns from issue #80 | ||
| const productsWithSummary = Object.entries(requestsByUrl).filter(([url]) => | ||
| url.includes('/products?include_delivery_summary=true') | ||
| ) | ||
| const categoriesRequests = Object.entries(requestsByUrl).filter(([url]) => | ||
| url.includes('/api/categories') | ||
| ) | ||
| const productsSorted = Object.entries(requestsByUrl).filter( | ||
| ([url]) => url.includes('/api/products?') && url.includes('sort=') | ||
| ) | ||
|
|
||
| // Assert: Each unique endpoint should be called exactly once | ||
| productsWithSummary.forEach(([url, count]) => { | ||
| expect(count, `Expected 1 request to ${url}, but found ${count}`).toBe(1) | ||
| }) | ||
|
|
||
| categoriesRequests.forEach(([url, count]) => { | ||
| expect(count, `Expected 1 request to ${url}, but found ${count}`).toBe(1) | ||
| }) | ||
|
|
||
| productsSorted.forEach(([url, count]) => { | ||
| expect(count, `Expected 1 request to ${url}, but found ${count}`).toBe(1) | ||
| }) | ||
|
|
||
| // Log all requests for debugging if test fails | ||
| console.log('All API requests:', JSON.stringify(requestsByUrl, null, 2)) | ||
| }) | ||
|
|
||
| test('should not make duplicate concurrent requests on product detail page', async ({ page }) => { | ||
| const requests: { url: string; timestamp: number }[] = [] | ||
|
|
||
| // Track all API requests | ||
| page.on('request', (request) => { | ||
| const url = request.url() | ||
| if (url.includes('/products')) { | ||
| requests.push({ | ||
| url: url.replace(/^https?:\/\/[^/]+/, ''), | ||
| timestamp: Date.now(), | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| // Navigate to homepage first | ||
| await page.goto('/') | ||
| await expect(page.locator('[data-testid="product-card"]').first()).toBeVisible({ | ||
| timeout: 10000, | ||
| }) | ||
|
|
||
| // Clear request tracking before navigating to detail page | ||
| requests.length = 0 | ||
|
|
||
| // Click on first product to go to detail page | ||
| await page.locator('[data-testid="product-card"]').first().click() | ||
| await page.waitForURL(/\/products\/\d+/) | ||
|
|
||
| // Wait for detail page to fully load | ||
| await expect(page.locator('[data-testid="product-title"]')).toBeVisible() | ||
| await page.waitForTimeout(2000) | ||
|
|
||
| // Group requests by URL | ||
| const requestsByUrl = requests.reduce( | ||
| (acc, req) => { | ||
| acc[req.url] = (acc[req.url] || 0) + 1 | ||
| return acc | ||
| }, | ||
| {} as Record<string, number> | ||
| ) | ||
|
|
||
| // Check for duplicate product detail requests (issue #80: GET /products/{id} called 2x) | ||
| const productDetailRequests = Object.entries(requestsByUrl).filter( | ||
| ([url]) => url.match(/\/products\/\d+$/) // Match /products/{id} without query params | ||
| ) | ||
|
|
||
| // Assert: Product detail endpoint should be called exactly once | ||
| productDetailRequests.forEach(([url, count]) => { | ||
| expect(count, `Expected 1 request to ${url}, but found ${count}`).toBe(1) | ||
| }) | ||
|
|
||
| // Log all requests for debugging if test fails | ||
| console.log('Product detail page API requests:', JSON.stringify(requestsByUrl, null, 2)) | ||
| }) | ||
|
|
||
| test('should deduplicate rapid navigation requests', async ({ page }) => { | ||
| const requests: Map<string, number[]> = new Map() | ||
|
|
||
| // Track all API requests with timestamps | ||
| page.on('request', (request) => { | ||
| const url = request.url() | ||
| if (url.includes('/products') || url.includes('/categories')) { | ||
| const cleanUrl = url.replace(/^https?:\/\/[^/]+/, '') | ||
| if (!requests.has(cleanUrl)) { | ||
| requests.set(cleanUrl, []) | ||
| } | ||
| requests.get(cleanUrl)!.push(Date.now()) | ||
| } | ||
| }) | ||
|
|
||
| // Trigger rapid navigation that might cause duplicate requests | ||
| await page.goto('/') | ||
| await expect(page.locator('[data-testid="product-card"]').first()).toBeVisible({ | ||
| timeout: 10000, | ||
| }) | ||
|
|
||
| // Navigate to a product and back quickly | ||
| await page.locator('[data-testid="product-card"]').first().click() | ||
| await page.waitForURL(/\/products\/\d+/) | ||
| await page.goBack() | ||
| await page.waitForURL('/') | ||
| await page.waitForTimeout(1000) | ||
|
|
||
| // Check for concurrent duplicate requests (requests within 100ms of each other) | ||
| requests.forEach((timestamps, url) => { | ||
| const sortedTimestamps = [...timestamps].sort((a, b) => a - b) | ||
| for (let i = 1; i < sortedTimestamps.length; i++) { | ||
| const timeDiff = sortedTimestamps[i] - sortedTimestamps[i - 1] | ||
| expect( | ||
| timeDiff, | ||
| `Found concurrent duplicate requests to ${url} (${timeDiff}ms apart). Deduplication should prevent this.` | ||
| ).toBeGreaterThan(100) | ||
| } | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,44 +2,114 @@ import { Category, DeliveryOption, Product } from './types' | |||||
|
|
||||||
| const API_BASE_URL = import.meta.env.VITE_API_BASE_URL || 'http://localhost:8001' | ||||||
|
|
||||||
| const DEDUPE_INTERVAL_MS = 2000 | ||||||
|
|
||||||
| type InFlightEntry = { | ||||||
| promise: Promise<unknown> | ||||||
| start: number | ||||||
| timer?: ReturnType<typeof globalThis.setTimeout> | ||||||
| } | ||||||
|
|
||||||
| class ApiClient { | ||||||
| private inFlight = new Map<string, InFlightEntry>() | ||||||
|
|
||||||
| // Normalize endpoint into a stable key: METHOD + normalized URL (sorted query) | ||||||
| private buildKey(endpoint: string, method: string): string { | ||||||
| const url = new globalThis.URL(endpoint, API_BASE_URL) | ||||||
| if (url.search) { | ||||||
| const sorted = new globalThis.URLSearchParams( | ||||||
| Array.from(url.searchParams.entries()).sort(([a], [b]) => a.localeCompare(b)) | ||||||
| ) | ||||||
| url.search = sorted.toString() | ||||||
| } | ||||||
| const pathAndQuery = url.pathname + (url.search ? `?${url.search}` : '') | ||||||
| return `${method.toUpperCase()} ${pathAndQuery}` | ||||||
| } | ||||||
|
|
||||||
| // Shared JSON request with GET-deduping | ||||||
| private async request<T>(endpoint: string, options: globalThis.RequestInit = {}): Promise<T> { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary
Suggested change
|
||||||
| const response = await fetch(`${API_BASE_URL}${endpoint}`, { | ||||||
| const method = (options.method || 'GET').toString().toUpperCase() | ||||||
| const fullUrl = `${API_BASE_URL}${endpoint}` | ||||||
|
|
||||||
| // GET-only dedupe | ||||||
| if (method === 'GET') { | ||||||
| const key = this.buildKey(endpoint, method) | ||||||
| const now = Date.now() | ||||||
| const existing = this.inFlight.get(key) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential race condition: If two identical GET requests are initiated simultaneously before either is added to the |
||||||
| if (existing && now - existing.start < DEDUPE_INTERVAL_MS) { | ||||||
| return existing.promise as Promise<T> | ||||||
| } | ||||||
|
|
||||||
| const promise = (async () => { | ||||||
| const response = await fetch(fullUrl, { | ||||||
| headers: { | ||||||
| 'Content-Type': 'application/json', | ||||||
| ...options.headers, | ||||||
| }, | ||||||
| ...options, | ||||||
| method, | ||||||
| }) | ||||||
| if (!response.ok) { | ||||||
| throw new Error(`API Error: ${response.status} ${response.statusText}`) | ||||||
| } | ||||||
| return (await response.json()) as T | ||||||
| })() | ||||||
|
|
||||||
| const entry: InFlightEntry = { promise, start: now } | ||||||
| // Failsafe cleanup in case finally doesn't run (page nav etc.) | ||||||
| entry.timer = globalThis.setTimeout(() => { | ||||||
| const current = this.inFlight.get(key) | ||||||
| if (current && current.start === entry.start) this.inFlight.delete(key) | ||||||
| }, DEDUPE_INTERVAL_MS) | ||||||
| this.inFlight.set(key, entry) | ||||||
|
|
||||||
| try { | ||||||
| return await promise | ||||||
| } finally { | ||||||
| const current = this.inFlight.get(key) | ||||||
| if (current && current.start === entry.start) { | ||||||
| if (current.timer) globalThis.clearTimeout(current.timer) | ||||||
| this.inFlight.delete(key) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Non-GET: no dedupe | ||||||
| const response = await fetch(fullUrl, { | ||||||
| headers: { | ||||||
| 'Content-Type': 'application/json', | ||||||
| ...options.headers, | ||||||
| }, | ||||||
| ...options, | ||||||
| method, | ||||||
| }) | ||||||
|
|
||||||
| if (!response.ok) { | ||||||
| throw new Error(`API Error: ${response.status} ${response.statusText}`) | ||||||
| } | ||||||
|
|
||||||
| return response.json() | ||||||
| } | ||||||
|
|
||||||
| // Public, typed JSON getter to allow components to use the client directly | ||||||
| getJSON<T>(endpoint: string, options: Omit<globalThis.RequestInit, 'method'> = {}): Promise<T> { | ||||||
| return this.request<T>(endpoint, { ...options, method: 'GET' }) | ||||||
| } | ||||||
|
|
||||||
| async getCategories(): Promise<Category[]> { | ||||||
| return this.request<Category[]>('/api/categories') | ||||||
| return this.getJSON<Category[]>('/api/categories') | ||||||
| } | ||||||
|
|
||||||
| async getDeliveryOptions(): Promise<DeliveryOption[]> { | ||||||
| return this.request<DeliveryOption[]>('/api/delivery-options') | ||||||
| return this.getJSON<DeliveryOption[]>('/api/delivery-options') | ||||||
| } | ||||||
|
|
||||||
| async getProducts( | ||||||
| params: { | ||||||
| categoryId?: string | ||||||
| deliveryOptionId?: string | ||||||
| sort?: string | ||||||
| } = {} | ||||||
| params: { categoryId?: string; deliveryOptionId?: string; sort?: string } = {} | ||||||
| ): Promise<Product[]> { | ||||||
| const searchParams = new globalThis.URLSearchParams() | ||||||
| if (params.categoryId) searchParams.set('categoryId', params.categoryId) | ||||||
| if (params.deliveryOptionId) searchParams.set('deliveryOptionId', params.deliveryOptionId) | ||||||
| if (params.sort) searchParams.set('sort', params.sort) | ||||||
|
|
||||||
| return this.request<Product[]>(`/api/products?${searchParams.toString()}`) | ||||||
| return this.getJSON<Product[]>(`/api/products?${searchParams.toString()}`) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type safety issue: Storing
Promise<unknown>but casting toPromise<T>when reusing can lead to type mismatches. If two different parts of the code request the same endpoint with different expected return types within the deduplication window, the second caller will receive incorrectly typed data from the first request's promise, potentially causing runtime errors.