feat: implement multi-site bibliography API endpoint#25
Conversation
Review Summary by QodoImplement multi-site bibliography API endpoint with filtering
WalkthroughsDescription• Implement multi-site bibliography API endpoint with siteId filtering • Add filter utility function for site-specific bibliography items • Include input validation and comprehensive error handling • Configure HTTP caching headers for performance optimization Diagramflowchart LR
A["GET /api/bibliography?siteId"] --> B["Validate siteId parameter"]
B --> C["Fetch bibliography.json"]
C --> D["Filter by siteId"]
D --> E["Return filtered items with cache headers"]
File Changes1. web/src/lib/api/filter.ts
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-site customInfo support across parsing, generation, types, and frontend; introduces a siteId-filtering utility and a GET /api/bibliography endpoint that returns site-scoped bibliography data with ETag caching; adds generated TypeScript data, tests, and Vitest config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as API
participant Data as bibliographyData
participant Filter as filterBySiteId
participant Crypto as WebCrypto
Client->>API: GET /api/bibliography?siteId=site.example
API->>API: validate siteId (regex)
API->>Data: read bibliographyData (in-memory)
API->>Filter: filterBySiteId(items, siteId)
Filter-->>API: filtered items
API->>API: attach item.customInfo[siteId] to each item
API->>Crypto: compute ETag from JSON
Crypto-->>API: ETag
API-->>Client: 200 JSON + ETag + Cache-Control
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying kotetsu with
|
| Latest commit: |
2788931
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9cb695a0.kotetsu.pages.dev |
| Branch Preview URL: | https://issue-24-api.kotetsu.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web/src/routes/api/bibliography/`+server.ts:
- Around line 36-41: The catch block around the fetch/response.json in the GET
handler should use SvelteKit's isHttpError() instead of instanceof HttpError;
import isHttpError from '@sveltejs/kit' and replace any instanceof HttpError
checks with isHttpError(err) so HTTP errors are detected correctly and rethrown
(or handled) while other errors return a 500 — update the error handling around
the fetch('/data/bibliography.json') and response.json() calls accordingly to
use isHttpError(err) and rethrow the original error when true.
🧹 Nitpick comments (2)
web/src/routes/api/bibliography/+server.ts (2)
43-43: Type assertion without runtime validation.
response.json()returnsany; the annotationBibliographyItem[]is a compile-time-only assertion. Ifbibliography.jsonhas a schema mismatch (e.g., after a pipeline change), malformed data silently passes through to clients. For a static file you control this is low-risk, but consider adding a lightweight runtime check (e.g.,Array.isArray(fullData)) as a safety net.
53-58: RedundantContent-Typeheader.SvelteKit's
json()helper already setsContent-Type: application/json. The explicit header on Line 56 is unnecessary.♻️ Proposed fix
return json(filtered, { headers: { - 'Cache-Control': 'public, max-age=3600, stale-while-revalidate=86400', - 'Content-Type': 'application/json' + 'Cache-Control': 'public, max-age=3600, stale-while-revalidate=86400' } });
| // 2. Fetch full data from static file | ||
| const response = await fetch('/data/bibliography.json'); | ||
|
|
||
| if (!response.ok) { | ||
| throw error(500, { message: 'Failed to load bibliography data' }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n web/src/routes/api/bibliography/+server.tsRepository: Rindrics/kotetsu
Length of output: 2176
🌐 Web query:
SvelteKit HttpError export from @sveltejs/kit 2.x documentation
💡 Result:
In SvelteKit 2.x, HttpError is a TypeScript type/interface exported from @sveltejs/kit (it represents the object produced by calling error(...)). You typically import it as a type:
import type { HttpError } from '@sveltejs/kit';Docs describe it as the object returned by error, with:
status: number(400–599)body: App.Error[1]
If you need to detect these thrown errors (e.g. in hooks or try/catch), use SvelteKit’s type guard:
import { isHttpError } from '@sveltejs/kit';(Also documented as an export from @sveltejs/kit.) [2]
Note: internally SvelteKit relies on instanceof checks against its HttpError implementation, which is why having multiple bundled copies of @sveltejs/kit can cause issues; this is discussed in SvelteKit’s changelog/PR notes. [3]
Sources:
[1] @sveltejs/kit module docs (HttpError section) https://svelte.nodejs.cn/docs/kit/%40sveltejs-kit
[2] SvelteKit “modules” docs (exports list includes isHttpError) https://kit.svelte.js.cn/docs/modules
[3] PR discussion mentioning HttpError instanceof checks https://svelte-changelog.dev/pull/sveltejs/kit/13971
Use isHttpError() to check for SvelteKit errors in the catch block.
Wrapping fetch and response.json() in a try/catch is good practice, but the proposed fix should use SvelteKit's isHttpError() function instead of instanceof HttpError to avoid bundling issues. Update the imports and error handling:
♻️ Corrected fix
-import { json, error } from '@sveltejs/kit';
+import { json, error, isHttpError } from '@sveltejs/kit';- const response = await fetch('/data/bibliography.json');
-
- if (!response.ok) {
- throw error(500, { message: 'Failed to load bibliography data' });
- }
-
- const fullData: BibliographyItem[] = await response.json();
+ let fullData: BibliographyItem[];
+ try {
+ const response = await fetch('/data/bibliography.json');
+ if (!response.ok) {
+ throw error(500, { message: 'Failed to load bibliography data' });
+ }
+ fullData = await response.json();
+ } catch (e) {
+ if (isHttpError(e)) throw e;
+ throw error(500, { message: 'Failed to load bibliography data' });
+ }🤖 Prompt for AI Agents
In `@web/src/routes/api/bibliography/`+server.ts around lines 36 - 41, The catch
block around the fetch/response.json in the GET handler should use SvelteKit's
isHttpError() instead of instanceof HttpError; import isHttpError from
'@sveltejs/kit' and replace any instanceof HttpError checks with
isHttpError(err) so HTTP errors are detected correctly and rethrown (or handled)
while other errors return a 500 — update the error handling around the
fetch('/data/bibliography.json') and response.json() calls accordingly to use
isHttpError(err) and rethrow the original error when true.
Code Review by Qodo
1. Worker script file missing
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| import type { RequestHandler } from './$types'; | ||
| import type { BibliographyItem } from '$lib/types'; | ||
| import { json, error } from '@sveltejs/kit'; | ||
| import { filterBySiteId } from '$lib/api/filter'; | ||
|
|
||
| /** | ||
| * GET /api/bibliography?siteId=<site-id> | ||
| * | ||
| * Returns filtered bibliography data for a specific site. | ||
| * | ||
| * Query Parameters: | ||
| * - siteId (required): The site identifier (alphanumeric, underscore, dot) | ||
| * | ||
| * Response: | ||
| * - 200: JSON array of BibliographyItem objects | ||
| * - 400: Missing or invalid siteId parameter | ||
| * - 404: siteId not found in database | ||
| * - 500: Failed to load bibliography data | ||
| * | ||
| * Example: | ||
| * GET /api/bibliography?siteId=akirahayashi_com | ||
| */ | ||
| export const GET: RequestHandler = async ({ url, fetch }) => { | ||
| // 1. Validate siteId parameter | ||
| const siteId = url.searchParams.get('siteId'); | ||
|
|
||
| if (!siteId) { | ||
| throw error(400, { message: 'Missing required parameter: siteId' }); | ||
| } | ||
|
|
||
| // Validate siteId format: alphanumeric, underscore, dot only | ||
| if (!/^[a-zA-Z0-9_.]+$/.test(siteId)) { | ||
| throw error(400, { message: 'Invalid siteId format' }); | ||
| } | ||
|
|
||
| // 2. Fetch full data from static file | ||
| const response = await fetch('/data/bibliography.json'); | ||
|
|
||
| if (!response.ok) { | ||
| throw error(500, { message: 'Failed to load bibliography data' }); | ||
| } | ||
|
|
||
| const fullData: BibliographyItem[] = await response.json(); | ||
|
|
||
| // 3. Filter by siteId | ||
| const filtered = filterBySiteId(fullData, siteId); | ||
|
|
||
| if (filtered.length === 0) { | ||
| throw error(404, { message: `siteId not found: ${siteId}` }); | ||
| } | ||
|
|
||
| // 4. Return with cache headers | ||
| return json(filtered, { | ||
| headers: { | ||
| 'Cache-Control': 'public, max-age=3600, stale-while-revalidate=86400', | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
1. Worker script file missing 📎 Requirement gap ✓ Correctness
• The PR implements /api/bibliography as a SvelteKit route but does not add the required Cloudflare Workers script at /infrastructure/workers/bibliography-api.ts. • Without the Worker script, the intended edge API implementation cannot be deployed or reviewed for correctness/security.
Agent Prompt
## Issue description
The compliance checklist requires a Cloudflare Worker script at `/infrastructure/workers/bibliography-api.ts`, but the PR currently implements the endpoint as a SvelteKit route only.
## Issue Context
The multi-site bibliography API is intended to run at the edge via Cloudflare Workers.
## Fix Focus Areas
- web/src/routes/api/bibliography/+server.ts[1-59]
- infrastructure/index.ts[1-25]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (!siteId) { | ||
| throw error(400, { message: 'Missing required parameter: siteId' }); | ||
| } | ||
|
|
||
| // Validate siteId format: alphanumeric, underscore, dot only | ||
| if (!/^[a-zA-Z0-9_.]+$/.test(siteId)) { | ||
| throw error(400, { message: 'Invalid siteId format' }); | ||
| } | ||
|
|
||
| // 2. Fetch full data from static file | ||
| const response = await fetch('/data/bibliography.json'); | ||
|
|
||
| if (!response.ok) { | ||
| throw error(500, { message: 'Failed to load bibliography data' }); | ||
| } | ||
|
|
||
| const fullData: BibliographyItem[] = await response.json(); | ||
|
|
||
| // 3. Filter by siteId | ||
| const filtered = filterBySiteId(fullData, siteId); | ||
|
|
||
| if (filtered.length === 0) { | ||
| throw error(404, { message: `siteId not found: ${siteId}` }); | ||
| } |
There was a problem hiding this comment.
3. Error payload uses message 📎 Requirement gap ✓ Correctness
• The 400 (missing/invalid siteId) and 404 (unknown siteId) errors use { message: ... } rather
than the defined { "error": ... } payload shape.
• This makes client handling inconsistent with the ticket-defined contract and can break consumers
expecting the specified error schema.
Agent Prompt
## Issue description
The endpoint returns 400/404 JSON payloads using a `message` key, but compliance requires a defined error payload shape (e.g., `{ "error": "..." }`).
## Issue Context
External clients may parse the error JSON and depend on the exact key.
## Fix Focus Areas
- web/src/routes/api/bibliography/+server.ts[27-34]
- web/src/routes/api/bibliography/+server.ts[48-50]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| import type { RequestHandler } from './$types'; | ||
| import type { BibliographyItem } from '$lib/types'; | ||
| import { json, error } from '@sveltejs/kit'; | ||
| import { filterBySiteId } from '$lib/api/filter'; | ||
|
|
||
| /** | ||
| * GET /api/bibliography?siteId=<site-id> | ||
| * | ||
| * Returns filtered bibliography data for a specific site. | ||
| * | ||
| * Query Parameters: | ||
| * - siteId (required): The site identifier (alphanumeric, underscore, dot) | ||
| * | ||
| * Response: | ||
| * - 200: JSON array of BibliographyItem objects | ||
| * - 400: Missing or invalid siteId parameter | ||
| * - 404: siteId not found in database | ||
| * - 500: Failed to load bibliography data | ||
| * | ||
| * Example: | ||
| * GET /api/bibliography?siteId=akirahayashi_com | ||
| */ | ||
| export const GET: RequestHandler = async ({ url, fetch }) => { | ||
| // 1. Validate siteId parameter | ||
| const siteId = url.searchParams.get('siteId'); | ||
|
|
||
| if (!siteId) { | ||
| throw error(400, { message: 'Missing required parameter: siteId' }); | ||
| } | ||
|
|
||
| // Validate siteId format: alphanumeric, underscore, dot only | ||
| if (!/^[a-zA-Z0-9_.]+$/.test(siteId)) { | ||
| throw error(400, { message: 'Invalid siteId format' }); | ||
| } | ||
|
|
||
| // 2. Fetch full data from static file | ||
| const response = await fetch('/data/bibliography.json'); | ||
|
|
||
| if (!response.ok) { | ||
| throw error(500, { message: 'Failed to load bibliography data' }); | ||
| } | ||
|
|
||
| const fullData: BibliographyItem[] = await response.json(); | ||
|
|
||
| // 3. Filter by siteId | ||
| const filtered = filterBySiteId(fullData, siteId); | ||
|
|
||
| if (filtered.length === 0) { | ||
| throw error(404, { message: `siteId not found: ${siteId}` }); | ||
| } | ||
|
|
||
| // 4. Return with cache headers | ||
| return json(filtered, { | ||
| headers: { | ||
| 'Cache-Control': 'public, max-age=3600, stale-while-revalidate=86400', | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
5. Pulumi lacks worker route 📎 Requirement gap ⛯ Reliability
• The infrastructure-as-code currently provisions a Cloudflare Pages domain but does not deploy a Worker or bind the /api/bibliography route. • Without Pulumi configuration for the Worker and route mapping, the API deployment is manual/undefined and does not meet the required exposure/configuration criteria.
Agent Prompt
## Issue description
Pulumi does not currently deploy a Worker or bind the `/api/bibliography` route, which is required for the multi-site bibliography API.
## Issue Context
The endpoint is intended to be exposed via Cloudflare Workers at `/api/bibliography`.
## Fix Focus Areas
- infrastructure/index.ts[1-25]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Rindrics
force-pushed
the
issue-24-api
branch
from
0af3c64 to
ff09d29
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/scripts/generate-data.ts (2)
1-3:⚠️ Potential issue | 🟡 MinorStale comment: still references
bibliography.json.The doc comment on line 2 says "Generate bibliography.json" but the script now generates
bibliography.ts.Proposed fix
/** - * Build script to generate bibliography.json from bib and yaml files + * Build script to generate bibliography.ts from bib and yaml files * Run with: npx tsx scripts/generate-data.ts */
113-113:⚠️ Potential issue | 🟡 MinorStale log message: still says
bibliography.json.- console.log('Generating bibliography.json...'); + console.log('Generating bibliography data...');
🤖 Fix all issues with AI agents
In `@docs/issues/pr-25.md`:
- Around line 20-28: The "filterBySiteId hardcoding" issue is stale because the
current implementation of filterBySiteId already uses item.customInfo?.[siteId];
remove or mark the issue resolved in the docs to avoid confusion by deleting the
stale entry or adding a short note referencing the updated implementation
(filterBySiteId function) and the relevant server handler (bibliography +server
handler) so reviewers know the bug was fixed; ensure the docs/no-opting issue
list no longer claims hardcoded akirahayashi_com.
- Line 1: Remove the accidental Japanese prompt string "それぞれ英語の conventional
commit message を示しつつ直して" from the document content and replace it with proper
English documentation showing conventional commit examples (e.g., "feat: add X",
"fix: correct Y", "docs: update Z") and a brief one-line explanation of the
format; locate the stray prompt by searching for that exact phrase and update
the surrounding text to be user-facing documentation in English rather than an
instruction.
In `@web/scripts/generate-data.ts`:
- Around line 125-131: The generated TypeScript module (created in
generate-data.ts via writeFileSync to outputPath as bibliography.ts) is missing
during CI type-checks and breaks imports in +page.ts/+server.ts; add a
declaration file named bibliography.d.ts alongside where bibliography.ts will be
generated that declares export const bibliographyData: BibliographyItem[]
(importing BibliographyItem from your types) so the TS compiler sees the symbol
during pnpm check, or alternatively ensure your CI runs npx tsx
scripts/generate-data.ts before pnpm check or commit a minimal stub
bibliography.ts exporting an empty array.
In `@web/src/routes/api/bibliography/`+server.ts:
- Line 4: CI is failing because the generated module imported as
bibliographyData (import { bibliographyData } from '$lib/data/bibliography')
does not exist during type checking; fix by either updating the CI pipeline to
run the generator (run web/scripts/generate-data.ts before pnpm check/build so
web/src/lib/data/bibliography.ts is created) or add a committed type
stub/declaration (e.g., declare module '$lib/data/bibliography' exporting
bibliographyData) to satisfy TypeScript during CI; choose one approach and apply
it so the import in +server.ts resolves during the check step.
🧹 Nitpick comments (5)
web/src/lib/parsers/yaml.ts (1)
14-41:validateCustomInfotype guard doesn't reject unknown fields.The type guard asserts
value is CustomInfoFullbut only checks known fields, allowing objects with extra properties to pass through. This is generally acceptable since downstreamtoFrontendInfoingenerate-data.tsexplicitly picks only the safe fields (tags,review,readDate). However, if this guard is ever used in a context where the object is passed through directly, unexpected fields could leak.web/src/routes/api/bibliography/+server.ts (3)
39-41: 404 for empty results is ambiguous.A valid
siteIdformat with no matching entries returns 404, which is indistinguishable from a truly unknown/non-existent site. Consider whether returning an empty 200 array would be more appropriate for "valid site, no entries" vs a true "site not found" check against a known list of site IDs.
50-55:Content-Typeheader is redundant.SvelteKit's
json()helper already setsContent-Type: application/json. Explicitly setting it is harmless but unnecessary.Proposed fix
return json(transformed, { headers: { - 'Cache-Control': 'public, max-age=3600, stale-while-revalidate=86400', - 'Content-Type': 'application/json' + 'Cache-Control': 'public, max-age=3600, stale-while-revalidate=86400' } });
44-47: Transform logic duplicated with+page.ts.Lines 44–47 mirror the same spread-and-extract pattern in
web/src/routes/+page.ts(lines 10–14). Consider extracting a shared helper (e.g.,transformForSite(items, siteId)) to keep the transformation in one place.web/src/routes/+page.ts (1)
6-6:DEFAULT_SITE_IDis a magic string — consider sharing it.This constant is defined locally and the same value
'akirahayashi_com'implicitly drives the API endpoint behavior as well. If this default is project-wide, extracting it to a shared config/constants module would prevent drift.
| // Write TypeScript module | ||
| const outputPath = join(outputDir, 'bibliography.ts'); | ||
| const moduleContent = `import type { BibliographyItem } from '../types'; | ||
|
|
||
| export const bibliographyData: BibliographyItem[] = ${JSON.stringify(items, null, 2)}; | ||
| `; | ||
| writeFileSync(outputPath, moduleContent); |
There was a problem hiding this comment.
Generated .ts module is the root cause of CI pipeline failures.
The script writes bibliography.ts to src/lib/data/, which is then imported by +page.ts and +server.ts. Since this file is generated at build time and likely .gitignored, CI's pnpm check fails because the module doesn't exist yet.
Options to fix:
- Run the generation script before type-checking in CI (e.g., add
npx tsx scripts/generate-data.tsstep beforepnpm check). - Commit a placeholder/stub
bibliography.tswith an empty array export. - Add a type declaration file (
bibliography.d.ts) so type-checking passes without the generated file.
Option 3 is the most robust as it decouples type-checking from data generation:
// web/src/lib/data/bibliography.d.ts
import type { BibliographyItem } from '../types';
export declare const bibliographyData: BibliographyItem[];🤖 Prompt for AI Agents
In `@web/scripts/generate-data.ts` around lines 125 - 131, The generated
TypeScript module (created in generate-data.ts via writeFileSync to outputPath
as bibliography.ts) is missing during CI type-checks and breaks imports in
+page.ts/+server.ts; add a declaration file named bibliography.d.ts alongside
where bibliography.ts will be generated that declares export const
bibliographyData: BibliographyItem[] (importing BibliographyItem from your
types) so the TS compiler sees the symbol during pnpm check, or alternatively
ensure your CI runs npx tsx scripts/generate-data.ts before pnpm check or commit
a minimal stub bibliography.ts exporting an empty array.
ff09d29 to
bc827bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@web/package.json`:
- Around line 31-32: The package.json currently lists incompatible versions
"vite": "^7.2.6" and "vitest": "^1.6.0"; update the vitest dependency to a 3.2+
release (e.g., change "vitest" to "^3.2.0") so it supports Vite 7, then
reinstall/update the lockfile (npm/yarn/pnpm) and run the test suite; if
tests/configs reference Vitest APIs, adjust config files (vite/vitest config or
test setup) for any 3.x breaking changes.
In `@web/src/routes/`+page.ts:
- Around line 8-15: The current load() maps bibliographyData and assigns
customInfo: item.customInfo?.[DEFAULT_SITE_ID], but that runtime object is
CustomInfoFull and may include the sensitive memo field; update the mapping in
load() to destructure the site-specific custom info and exclude memo (e.g.,
const { memo, ...frontend } = item.customInfo?.[DEFAULT_SITE_ID] || {}) and
assign customInfo to the frontend object (typed as CustomInfoFrontend |
undefined) so memo is not serialized to the client; refer to load,
bibliographyData, DEFAULT_SITE_ID, CustomInfoFrontend and the memo property when
making the change.
In `@web/src/routes/api/bibliography/`+server.test.ts:
- Around line 4-43: The tests define mockBibliographyData but never inject
it—because the GET handler imports bibliographyData from $lib/data/bibliography
at module scope, tests exercise the real module; update the test to mock that
module: use vitest's vi.mock to replace $lib/data/bibliography with an object
exporting bibliographyData (use the mockBibliographyData array) before importing
or invoking the GET handler so the handler (GET) reads the mocked
bibliographyData instead of the real/generated data.
In `@web/vitest.config.ts`:
- Around line 1-9: The Vitest config lacks SvelteKit path alias resolution so
imports using SvelteKit’s $lib will fail; update the config returned by
defineConfig in web/vitest.config.ts to enable SvelteKit aliasing by either
importing and applying the Vite config from `@sveltejs/kit/vite` (or its plugin)
or by adding a resolve.alias entry that maps '$lib' to 'src/lib' (ensure the
change is applied inside the exported defineConfig({ test: {...}, resolve: {
alias: [...] } }) object so tests can resolve $lib imports).
🧹 Nitpick comments (2)
web/src/lib/api/__tests__/filter.test.ts (1)
103-117: siteId regex is duplicated — drift risk with the server handler.This test defines its own regex
/^[a-zA-Z0-9_.]+$/rather than importing or sharing the pattern from the server handler. If the validation regex in+server.tschanges, this test will silently pass with stale rules. Consider extracting the regex into a shared constant.web/src/routes/+page.ts (1)
6-6: Consider sharingDEFAULT_SITE_IDas a constant.This value is also conceptually used in the API route handler's logic. Extracting it to a shared module would avoid drift.
- Change BibliographyItem.customInfo from single site to per-site map
- Update parseCustomInfo to return all sites' metadata (not just one)
- Support multiple sites via customInfo[siteId] structure
- Update filterBySiteId to properly filter by site association
- Transform API responses to extract site-specific customInfo
- Update page loader to use bibliography module directly
- Remove unused merge.ts (replaced by inline transformation)
Enables proper multi-site bibliography API support.
- Change error response key from 'message' to 'error' - Use json() with status option instead of error() for consistency - Ensures external clients can reliably parse error payloads Affected endpoints: - GET /api/bibliography?siteId= (400 and 404 responses)
- Ensure bibliography.ts is generated before type checking - Fixes CI failures when checking out fresh repository
- Generate ETag from response content using SHA-256 hash - Enable conditional requests and cache validation - Supports external site builds with proper cache control Endpoints affected: - GET /api/bibliography?siteId= (200 responses)
- Add comprehensive JSDoc for GET /api/bibliography endpoint - Document response body structure with all fields - Clarify single-site limitation and use cases - Document error response format - Add cache control and ETag behavior - Explain security rationale for memo field exclusion - Update filterBySiteId documentation for clarity Helps external consumers understand API contract and limitations.
Tests cover: - Missing or invalid siteId parameter validation (400) - Non-existent siteId handling (404) - Proper filtering by siteId (200) - memo field exclusion for security - ETag header generation - Cache-Control header configuration - Valid/invalid siteId format acceptance Includes vitest setup with test and test:ui scripts.
Rindrics
force-pushed
the
issue-24-api
branch
from
bc827bc to
1b185bf
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/scripts/generate-data.ts (1)
1-3:⚠️ Potential issue | 🟡 MinorStale references to
bibliography.json.The doc comment (Line 2) and the console.log at Line 113 still reference
bibliography.json, but the script now generatesbibliography.ts.Proposed fix
/** - * Build script to generate bibliography.json from bib and yaml files + * Build script to generate bibliography.ts from bib and yaml files * Run with: npx tsx scripts/generate-data.ts */At Line 113:
- console.log('Generating bibliography.json...'); + console.log('Generating bibliography.ts...');
🧹 Nitpick comments (4)
web/src/lib/data/bibliography.ts (1)
1-60: Generated data file looks consistent with the type system.This file appears to be output from
generate-data.ts. If so, consider adding a generated-file header comment (e.g.,// AUTO-GENERATED — do not edit manually) to prevent accidental hand-edits and to help tooling (linters, formatters) skip it.web/src/routes/api/bibliography/+server.ts (2)
66-82: Double JSON serialization and redundantContent-Typeheader.
json(transformed)on Line 76 internally callsJSON.stringifyagain, meaning the response body is serialized twice (Line 72 for the ETag, and again insidejson()). Also, SvelteKit'sjson()helper already setsContent-Type: application/json, making Line 80 redundant.Proposed optimization
const responseBody = JSON.stringify(transformed); const etag = `"${createHash('sha256').update(responseBody).digest('hex').slice(0, 16)}"`; - return json(transformed, { + return new Response(responseBody, { headers: { 'ETag': etag, - 'Cache-Control': 'public, max-age=3600, stale-while-revalidate=86400', - 'Content-Type': 'application/json' + 'Cache-Control': 'public, max-age=3600, stale-while-revalidate=86400', + 'Content-Type': 'application/json' } });
45-83: NoIf-None-Matchhandling despite ETag generation.The endpoint computes and returns an ETag but never checks the incoming
If-None-Matchrequest header. Without that check, the ETag provides no bandwidth savings — the full response is always sent. Consider returning a304 Not Modifiedwhen the client's ETag matches.Proposed addition after ETag computation
const etag = `"${createHash('sha256').update(responseBody).digest('hex').slice(0, 16)}"`; + const ifNoneMatch = url.searchParams // wrong — use request headers + // Access request headers via the event: + // In SvelteKit, destructure `request` from the event: + // export const GET: RequestHandler = async ({ url, request }) => { + // const ifNoneMatch = request.headers.get('if-none-match'); + // if (ifNoneMatch === etag) { + // return new Response(null, { status: 304 }); + // }Specifically, update the handler signature and add the conditional:
-export const GET: RequestHandler = async ({ url }) => { +export const GET: RequestHandler = async ({ url, request }) => {Then, after computing the ETag:
+ if (request.headers.get('if-none-match') === etag) { + return new Response(null, { status: 304 }); + }web/src/routes/+page.ts (1)
6-6: Consider extractingDEFAULT_SITE_IDto a shared config.This constant is hardcoded here. If other parts of the codebase (or tests) need to reference the default site, having it in a single shared location would prevent drift.
Add test suite for filterBySiteId function covering: - Filtering entries by siteId - Excluding entries without siteId metadata - Handling different siteIds - Non-existent siteId behavior - Field preservation - siteId format validation
- Remove web/src/lib/data/ from .gitignore - Include generated bibliography.ts in repository - Ensures TypeScript imports resolve during CI type checks - Reduces CI runtime by avoiding regeneration step
- Update vitest dependency from ^1.6.0 to ^3.2.0 - Resolves compatibility issue with Vite 7.2.6 - Enables latest test runner features and improvements
- Add resolve.alias configuration for $lib → src/lib - Enables tests to import using SvelteKit's \$lib alias - Resolves module import failures during test execution - Tests can now use same import paths as application code
…bility) - Replace node:crypto with Web Crypto API's crypto.subtle.digest - Generates SHA-256 hash using platform-native APIs - Resolves 'crypto not found' error during Cloudflare Pages deployment - Enables deployment to serverless/edge environments The Web Crypto API is universally supported in all JavaScript runtimes including Node.js, Deno, Cloudflare Workers, and browsers.
Rindrics
force-pushed
the
issue-24-api
branch
from
bf8832a to
defcddf
Compare
- Add request parameter to GET handler - Check If-None-Match header and return 304 Not Modified when ETag matches - Enables HTTP cache validation and bandwidth savings - Clients can now use conditional requests for efficient data fetching 304 responses bypass response body transmission when data hasn't changed.
- Create web/src/lib/config/constants.ts for application-wide constants - Move DEFAULT_SITE_ID from +page.ts to constants file - Enables single source of truth for default site identifier - Prevents drift if other modules need to reference default site Future improvements can add more constants to this file.
Rindrics
force-pushed
the
issue-24-api
branch
from
defcddf to
2788931
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const ifNoneMatch = request.headers.get('if-none-match'); | ||
| if (ifNoneMatch === etag) { | ||
| return new Response(null, { status: 304 }); | ||
| } |
There was a problem hiding this comment.
🟡 If-None-Match comparison doesn't handle multiple ETags per HTTP spec
The If-None-Match header comparison at web/src/routes/api/bibliography/+server.ts:81 uses strict string equality (ifNoneMatch === etag), but per RFC 7232 Section 3.2, the If-None-Match header can contain a comma-separated list of ETags (e.g., "abc", "def") or the wildcard *.
Root Cause and Impact
The HTTP spec defines If-None-Match as "*" / 1#entity-tag, meaning it can contain multiple ETags. The current code:
const ifNoneMatch = request.headers.get('if-none-match');
if (ifNoneMatch === etag) {
return new Response(null, { status: 304 });
}This fails when:
- A client sends multiple ETags:
If-None-Match: "abc123", "def456"— the strict equality check against a single ETag like"abc123"will fail because the full header string doesn't match. - A client sends the wildcard
*— this should match any ETag but won't with strict equality.
Impact: Cache validation will fail in these cases, causing the server to return a full 200 response with the body instead of a 304 Not Modified. This wastes bandwidth but doesn't cause incorrect behavior. In practice, most browsers send a single ETag for simple GET requests, so the impact is limited.
| const ifNoneMatch = request.headers.get('if-none-match'); | |
| if (ifNoneMatch === etag) { | |
| return new Response(null, { status: 304 }); | |
| } | |
| const ifNoneMatch = request.headers.get('if-none-match'); | |
| if (ifNoneMatch === '*' || ifNoneMatch?.split(',').some((tag) => tag.trim() === etag)) { | |
| return new Response(null, { status: 304 }); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores