Repository navigation
Implement constructors page, remove temp-data - #7
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Constructors landing page (with season selector + constructor cards) and removes the in-repo CSV/mock dataset by switching the Next.js /api/f1/* routes to proxy an upstream backend (via API_SERVER_URL).
Changes:
- Implement
/constructorsUI (explorer, cards, hook) and update nav/home copy from “Teams” to “Constructors”. - Extend the client data layer with
fetchConstructorsByYearand related types + tests. - Remove
temp-data/*CSVs and the server-side CSV/domain repository, updating API routes to proxy upstream responses instead.
Reviewed changes
Copilot reviewed 32 out of 40 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| temp-data/status.csv | Removed CSV dataset file (no longer used). |
| temp-data/sprint_results.csv | Removed CSV dataset file (no longer used). |
| temp-data/seasons.csv | Removed CSV dataset file (no longer used). |
| temp-data/drivers.csv | Removed CSV dataset file (no longer used). |
| temp-data/constructors.csv | Removed CSV dataset file (no longer used). |
| temp-data/circuits.csv | Removed CSV dataset file (no longer used). |
| src/types/championship.ts | Adds constructors season response/card types. |
| src/lib/services/f1DataClient.ts | Adds fetchConstructorsByYear API client call. |
| src/lib/services/f1DataClient.spec.ts | Updates cache test to include constructors fetch. |
| src/lib/server/f1Repository.ts | Deletes in-repo CSV-backed dataset repository. |
| src/lib/server/domainService.ts | Deletes server-side domain aggregation/service layer. |
| src/lib/server/csv.ts | Deletes CSV parsing utilities (no longer needed). |
| src/hooks/useConstructorsSeason.ts | New hook to load constructors season payloads and manage UI state. |
| src/components/constructors/ConstructorsExplorer.tsx | New constructors season explorer UI (year select, states, grid). |
| src/components/constructors/ConstructorsExplorer.spec.tsx | Tests for constructors explorer loading/refetch/error states. |
| src/components/constructors/ConstructorCard.tsx | New constructor card component linking to constructor detail page. |
| src/components/NavBar.tsx | Updates nav link from /teams to /constructors. |
| src/components/NavBar.spec.tsx | Updates navbar tests for constructors link + active state. |
| src/app/teams/page.tsx | Removes the old placeholder Teams page. |
| src/app/page.tsx | Updates homepage category/copy to “Constructors”. |
| src/app/constructors/page.tsx | Adds constructors index page rendering the explorer. |
| src/app/constructors/[id]/page.tsx | Renames “Team” wording to “Constructor” on detail placeholder page. |
| src/app/api/f1/standings/route.ts | Switches standings API route to upstream proxy. |
| src/app/api/f1/standings/route.spec.ts | Updates tests to validate proxy behavior + query validation. |
| src/app/api/f1/seasons/route.ts | Switches seasons API route to upstream proxy. |
| src/app/api/f1/seasons/route.spec.ts | Updates tests for upstream proxy + upstream unavailable handling. |
| src/app/api/f1/results/route.ts | Switches results API route to upstream proxy. |
| src/app/api/f1/events/route.ts | Switches events API route to upstream proxy. |
| src/app/api/f1/drivers/[id]/route.ts | Switches driver detail API route to upstream proxy. |
| src/app/api/f1/constructors/route.ts | Switches constructors API route to upstream proxy; supports legacy season param. |
| src/app/api/f1/constructors/route.spec.ts | Adds test coverage for constructors route proxy + validation. |
| src/app/api/f1/constructors/[id]/route.ts | Switches constructor detail API route to upstream proxy. |
Comment on lines
+27
to
+32
| {payload ? ( | ||
| <p className="text-muted text-sm"> | ||
| {payload.data.constructors.length} constructors in{' '} | ||
| {payload.data.year} | ||
| </p> | ||
| ) : null} |
There was a problem hiding this comment.
While a new season is loading, payload still contains the previous season’s data, so the “X constructors in YEAR” summary can briefly show the wrong year/count. Consider hiding this summary while isLoading is true, or clearing payload at the start of a refetch to avoid displaying stale season info.
| const hasSeasonParam = request.nextUrl.searchParams.has('season'); | ||
| const hasYearParam = request.nextUrl.searchParams.has('year'); | ||
| const seasonParam = request.nextUrl.searchParams.get('season'); | ||
| const yearParam = request.nextUrl.searchParams.get('year'); |
There was a problem hiding this comment.
If both season and year query params are provided (e.g. ?season=2024&year=2023), the handler silently prefers season and ignores year. That ambiguity can make debugging harder for API consumers; consider returning a 400 when both are present, or clearly prioritizing one parameter in the API contract/message.
Suggested change
| const yearParam = request.nextUrl.searchParams.get('year'); | |
| const yearParam = request.nextUrl.searchParams.get('year'); | |
| if (hasSeasonParam && hasYearParam) { | |
| return errorJson({ | |
| code: 'invalid_query', | |
| message: 'Specify either "season" or "year", but not both.', | |
| status: 400, | |
| }); | |
| } |
Comment on lines
+9
to
+31
| let upstreamUrl: URL; | ||
|
|
||
| try { | ||
| upstreamUrl = new URL(`${getBackendBaseUrl()}/api/f1/seasons`); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'server_misconfigured', | ||
| message: 'API server is not configured.', | ||
| status: 500, | ||
| }); | ||
| } | ||
|
|
||
| try { | ||
| const upstreamResponse = await fetch(upstreamUrl.toString(), { | ||
| method: 'GET', | ||
| cache: 'no-store', | ||
| }); | ||
|
|
||
| const upstreamBody = await upstreamResponse.text(); | ||
| const contentType = upstreamResponse.headers.get('content-type'); | ||
| const headers = new Headers(); | ||
|
|
||
| if (contentType) { |
There was a problem hiding this comment.
This route duplicates the same upstream-proxy URL building that exists elsewhere (e.g. /api/f1/championships uses buildBackendApiUrl). Consider using the shared helper from src/lib/backend.ts to reduce repetition and keep URL construction consistent (normalization, query handling, etc.).
Comment on lines
9
to
+58
| export async function GET(request: NextRequest) { | ||
| const season = parseIntegerQuery(request.nextUrl.searchParams.get('season')); | ||
|
|
||
| if (season === null) { | ||
| return errorJson({ | ||
| code: 'invalid_query', | ||
| message: 'A valid integer season query parameter is required.', | ||
| status: 400, | ||
| }); | ||
| } | ||
|
|
||
| const events = getEventsBySeason(season); | ||
| let upstreamUrl: URL; | ||
|
|
||
| if (!events) { | ||
| try { | ||
| upstreamUrl = new URL(`${getBackendBaseUrl()}/api/f1/events`); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'season_not_found', | ||
| message: 'Requested season was not found.', | ||
| status: 404, | ||
| code: 'server_misconfigured', | ||
| message: 'API server is not configured.', | ||
| status: 500, | ||
| }); | ||
| } | ||
|
|
||
| return okJson({ season, events }); | ||
| upstreamUrl.searchParams.set('season', String(season)); | ||
|
|
||
| try { | ||
| const upstreamResponse = await fetch(upstreamUrl.toString(), { | ||
| method: 'GET', | ||
| cache: 'no-store', | ||
| }); | ||
|
|
||
| const upstreamBody = await upstreamResponse.text(); | ||
| const contentType = upstreamResponse.headers.get('content-type'); | ||
| const headers = new Headers(); | ||
|
|
||
| if (contentType) { | ||
| headers.set('content-type', contentType); | ||
| } | ||
|
|
||
| return new NextResponse(upstreamBody, { | ||
| status: upstreamResponse.status, | ||
| headers, | ||
| }); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'upstream_unavailable', | ||
| message: 'Failed to reach upstream API server.', | ||
| status: 502, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This route’s behavior changed to proxy an upstream API (including misconfiguration and upstream-unavailable error handling), but there is no accompanying route.spec.ts here (unlike other proxied routes such as championships, seasons, standings). Adding tests would help prevent regressions in query validation, status passthrough, and error mapping.
Comment on lines
21
to
+82
| export async function GET(request: NextRequest) { | ||
| const season = parseIntegerQuery(request.nextUrl.searchParams.get('season')); | ||
| const sessionType = parseSessionType( | ||
| request.nextUrl.searchParams.get('sessionType'), | ||
| ); | ||
|
|
||
| if (season === null) { | ||
| return errorJson({ | ||
| code: 'invalid_query', | ||
| message: 'A valid integer season query parameter is required.', | ||
| status: 400, | ||
| }); | ||
| } | ||
|
|
||
| if (sessionType === null) { | ||
| return errorJson({ | ||
| code: 'invalid_query', | ||
| message: 'sessionType must be one of race, sprint, or all.', | ||
| status: 400, | ||
| }); | ||
| } | ||
|
|
||
| const result = getResultsBySeason({ season, sessionType }); | ||
| let upstreamUrl: URL; | ||
|
|
||
| if (!result) { | ||
| try { | ||
| upstreamUrl = new URL(`${getBackendBaseUrl()}/api/f1/results`); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'season_not_found', | ||
| message: 'Requested season was not found.', | ||
| status: 404, | ||
| code: 'server_misconfigured', | ||
| message: 'API server is not configured.', | ||
| status: 500, | ||
| }); | ||
| } | ||
|
|
||
| return okJson(result); | ||
| upstreamUrl.searchParams.set('season', String(season)); | ||
| upstreamUrl.searchParams.set('sessionType', sessionType); | ||
|
|
||
| try { | ||
| const upstreamResponse = await fetch(upstreamUrl.toString(), { | ||
| method: 'GET', | ||
| cache: 'no-store', | ||
| }); | ||
|
|
||
| const upstreamBody = await upstreamResponse.text(); | ||
| const contentType = upstreamResponse.headers.get('content-type'); | ||
| const headers = new Headers(); | ||
|
|
||
| if (contentType) { | ||
| headers.set('content-type', contentType); | ||
| } | ||
|
|
||
| return new NextResponse(upstreamBody, { | ||
| status: upstreamResponse.status, | ||
| headers, | ||
| }); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'upstream_unavailable', | ||
| message: 'Failed to reach upstream API server.', | ||
| status: 502, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This route’s behavior changed to proxy an upstream API (including sessionType validation and upstream error mapping), but there is no route.spec.ts covering the new proxy logic. Consider adding tests similar to standings/route.spec.ts to verify query validation, upstream URL construction, and passthrough of upstream status/body.
Comment on lines
9
to
+62
| export async function GET( | ||
| _request: NextRequest, | ||
| context: { params: Promise<{ id: string }> }, | ||
| ) { | ||
| const params = await context.params; | ||
| const constructorId = Number(params.id); | ||
|
|
||
| if (!Number.isInteger(constructorId)) { | ||
| return errorJson({ | ||
| code: 'invalid_path', | ||
| message: 'Constructor id must be an integer.', | ||
| status: 400, | ||
| }); | ||
| } | ||
|
|
||
| const constructor = getConstructorById(constructorId); | ||
| let upstreamUrl: URL; | ||
|
|
||
| if (!constructor) { | ||
| try { | ||
| upstreamUrl = new URL( | ||
| `${getBackendBaseUrl()}/api/f1/constructors/${constructorId}`, | ||
| ); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'constructor_not_found', | ||
| message: 'Constructor was not found.', | ||
| status: 404, | ||
| code: 'server_misconfigured', | ||
| message: 'API server is not configured.', | ||
| status: 500, | ||
| }); | ||
| } | ||
|
|
||
| return okJson(constructor); | ||
| try { | ||
| const upstreamResponse = await fetch(upstreamUrl.toString(), { | ||
| method: 'GET', | ||
| cache: 'no-store', | ||
| }); | ||
|
|
||
| const upstreamBody = await upstreamResponse.text(); | ||
| const contentType = upstreamResponse.headers.get('content-type'); | ||
| const headers = new Headers(); | ||
|
|
||
| if (contentType) { | ||
| headers.set('content-type', contentType); | ||
| } | ||
|
|
||
| return new NextResponse(upstreamBody, { | ||
| status: upstreamResponse.status, | ||
| headers, | ||
| }); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'upstream_unavailable', | ||
| message: 'Failed to reach upstream API server.', | ||
| status: 502, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This route now proxies the upstream constructor-detail endpoint and has custom validation/error mapping, but there’s no test coverage for it. Consider adding a route.spec.ts next to this file to cover invalid id handling, misconfiguration (missing API_SERVER_URL), and upstream passthrough behavior.
Comment on lines
9
to
+60
| export async function GET( | ||
| _request: NextRequest, | ||
| context: { params: Promise<{ id: string }> }, | ||
| ) { | ||
| const params = await context.params; | ||
| const driverId = Number(params.id); | ||
|
|
||
| if (!Number.isInteger(driverId)) { | ||
| return errorJson({ | ||
| code: 'invalid_path', | ||
| message: 'Driver id must be an integer.', | ||
| status: 400, | ||
| }); | ||
| } | ||
|
|
||
| const driver = getDriverById(driverId); | ||
| let upstreamUrl: URL; | ||
|
|
||
| if (!driver) { | ||
| try { | ||
| upstreamUrl = new URL(`${getBackendBaseUrl()}/api/f1/drivers/${driverId}`); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'driver_not_found', | ||
| message: 'Driver was not found.', | ||
| status: 404, | ||
| code: 'server_misconfigured', | ||
| message: 'API server is not configured.', | ||
| status: 500, | ||
| }); | ||
| } | ||
|
|
||
| return okJson(driver); | ||
| try { | ||
| const upstreamResponse = await fetch(upstreamUrl.toString(), { | ||
| method: 'GET', | ||
| cache: 'no-store', | ||
| }); | ||
|
|
||
| const upstreamBody = await upstreamResponse.text(); | ||
| const contentType = upstreamResponse.headers.get('content-type'); | ||
| const headers = new Headers(); | ||
|
|
||
| if (contentType) { | ||
| headers.set('content-type', contentType); | ||
| } | ||
|
|
||
| return new NextResponse(upstreamBody, { | ||
| status: upstreamResponse.status, | ||
| headers, | ||
| }); | ||
| } catch { | ||
| return errorJson({ | ||
| code: 'upstream_unavailable', | ||
| message: 'Failed to reach upstream API server.', | ||
| status: 502, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This route now proxies the upstream driver-detail endpoint and has custom validation/error mapping, but there’s no test coverage for it. Consider adding a route.spec.ts to verify invalid id handling, upstream passthrough, and error responses (misconfigured backend / upstream unavailable).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.