Add collections route#120
Conversation
|
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 a new Fastify GET route at /content/collections, registers it on app startup, implements DB queries (with optional author/pagination), formats collection objects (including full image URLs via createImageUrl), adds tests and test mocks, and updates tsconfig to allow JS and rename ESLint config file. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Fastify
participant DB
participant S3
Client->>Fastify: GET /content/collections?author=&page=&limit=
Fastify->>DB: Query collections with authors and chapter counts
DB-->>Fastify: Collection rows
Fastify->>S3: createImageUrl for cover/profile images
S3-->>Fastify: Full image URLs
Fastify-->>Client: 200 OK with collections array
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Hey @fennifith, should the query parameters be optional or required? Because currently they are all required and I am not sure about this. Also can you look at the example (I tried adding changes as a diff with previews but I couldn't find how can I do. I also tried added as a link with highlight and it didn't work and only points to the beginning of the highlight.) |
|
@femincan I would make For the example URLs, I'd just use a fake value e.g. |
…ectly check eslint config in tsconfig
|
The referenced issue says:
To be able to display the collection card we also need the Also we are returning the author's |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/api/src/routes/content/collections.test.ts (2)
18-43: UsemockResolvedValuefor async database queries.
db.query.collections.findManyis an async function, butmockReturnValueis used instead ofmockResolvedValue. While this may work due to how the code awaits the result,mockResolvedValueis the correct semantic choice for async functions and makes the test intent clearer.♻️ Proposed fix
- vi.mocked(db.query.collections.findMany).mockReturnValue([ + vi.mocked(db.query.collections.findMany).mockResolvedValue([ { slug: "harsh-leadership-truths", // ... rest of mock data }, - ] as never); + ] as never);Apply the same change to all test cases (lines 78, 91, 139).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/content/collections.test.ts` around lines 18 - 43, The test is mocking an async DB call using vi.mocked(db.query.collections.findMany).mockReturnValue(...); replace mockReturnValue with mockResolvedValue so the promise resolves correctly: update the mock calls for db.query.collections.findMany (and any other async mocks in this file) to use mockResolvedValue instead of mockReturnValue, including the other occurrences noted (lines with similar mocks for collections.findMany at other test cases).
17-74: Consider verifying the database query receives correct parameters.The tests verify response shapes but don't assert that
findManywas called with the expected arguments (where, limit, offset). Adding such assertions would catch parameter-passing bugs.💡 Example assertion to add
expect(db.query.collections.findMany).toHaveBeenCalledWith( expect.objectContaining({ limit: 10, offset: 0, where: expect.objectContaining({ authors: { slug: undefined } }), }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/content/collections.test.ts` around lines 17 - 74, The test currently only validates the response body but doesn't assert that db.query.collections.findMany was invoked with the expected parameters; add an assertion after the request to check that db.query.collections.findMany was called with an object containing limit: 10, offset: 0 (derived from page/limit), and the expected where clause (e.g., where: expect.objectContaining({ authors: { slug: undefined } }) or the actual filter used by the route); reference the mocked function db.query.collections.findMany in the test to locate where to add this expect(...) call so parameter-passing regressions are caught.apps/api/src/routes/content/collections.ts (1)
6-11: Consider adding bounds validation for pagination parameters.The
pageandlimitparameters have no constraints, allowing negative values or excessively large limits that could cause unexpected database behavior or resource exhaustion.♻️ Proposed fix with validation constraints
const CollectionsQueryParamsSchema = Type.Object({ locale: Type.String({ default: "en" }), - page: Type.Number(), - limit: Type.Number(), + page: Type.Number({ minimum: 0 }), + limit: Type.Number({ minimum: 1, maximum: 100 }), author: Type.Optional(Type.String()), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/content/collections.ts` around lines 6 - 11, The CollectionsQueryParamsSchema allows unbounded page and limit values; update the schema (CollectionsQueryParamsSchema) to validate pagination by enforcing page >= 1 and a sane range for limit (e.g., >=1 and a reasonable maximum like 100 or 1000), and consider adding defaults for page and limit to avoid missing values; adjust the Type.Number declarations for the page and limit fields to include minimum (and maximum for limit) constraints so invalid or abusive values are rejected before reaching DB/query logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/content/collections.ts`:
- Around line 116-127: The loop building formattedCollection reads
collection.data[0] unsafely which can be undefined; update the loop in the
collections processing code (the for...of over collections that constructs
formattedCollection) to guard against missing locale data by checking if
collection.data && collection.data[0] exist (or use optional chaining) before
accessing properties like coverImage, title, and description; if data is
missing, either skip that collection or populate safe defaults (e.g.,
undefined/empty strings and chapterCount 0) so accessing
collectionData.coverImage, collectionData.title, and collectionData.description
cannot throw.
- Around line 30-64: The examples in the response schema are inconsistent with
the defined model: fix each example object inside the examples array by (1)
removing the stray "n" from the first example's description, (2) adding the
required slug field to each example (e.g., slug: "<appropriate-slug>") and (3)
renaming each author property profileImage to profileImageUrl so authors match
the schema; update the example objects that include coverUrl, title,
description, authors, chapterCount, and slug accordingly.
- Line 4: Replace the incorrect TypeBox import with the correct package import
(`@sinclair/typebox`) by updating the import of Type and Static, add a null-safety
guard before accessing collection.data[0] in the code that iterates collections
(e.g., assign const collectionData = collection.data[0]; if (!collectionData)
continue; before using collectionData), and update the schema examples to match
the schema fields used by the Type.Object/Type.String definitions (use
profileImageUrl instead of profileImage, include the required slug field, and
remove the stray typo "...n") so examples align with the declared schema.
- Around line 101-113: The query in db.query.collections.findMany currently
always supplies where: { authors: { slug: queryParams.author } }, which passes
slug: undefined when queryParams.author is absent; update the where to
conditionally include the author filter only when queryParams.author is defined
by using the Drizzle callback style (e.g., use and() and eq() on the
authors.slug column inside the where callback) so that when queryParams.author
is undefined the authors.slug predicate is omitted; ensure this change is
applied where db.query.collections.findMany is called and continue to include
the other with:, limit:, and offset: options unchanged.
---
Nitpick comments:
In `@apps/api/src/routes/content/collections.test.ts`:
- Around line 18-43: The test is mocking an async DB call using
vi.mocked(db.query.collections.findMany).mockReturnValue(...); replace
mockReturnValue with mockResolvedValue so the promise resolves correctly: update
the mock calls for db.query.collections.findMany (and any other async mocks in
this file) to use mockResolvedValue instead of mockReturnValue, including the
other occurrences noted (lines with similar mocks for collections.findMany at
other test cases).
- Around line 17-74: The test currently only validates the response body but
doesn't assert that db.query.collections.findMany was invoked with the expected
parameters; add an assertion after the request to check that
db.query.collections.findMany was called with an object containing limit: 10,
offset: 0 (derived from page/limit), and the expected where clause (e.g., where:
expect.objectContaining({ authors: { slug: undefined } }) or the actual filter
used by the route); reference the mocked function db.query.collections.findMany
in the test to locate where to add this expect(...) call so parameter-passing
regressions are caught.
In `@apps/api/src/routes/content/collections.ts`:
- Around line 6-11: The CollectionsQueryParamsSchema allows unbounded page and
limit values; update the schema (CollectionsQueryParamsSchema) to validate
pagination by enforcing page >= 1 and a sane range for limit (e.g., >=1 and a
reasonable maximum like 100 or 1000), and consider adding defaults for page and
limit to avoid missing values; adjust the Type.Number declarations for the page
and limit fields to include minimum (and maximum for limit) constraints so
invalid or abusive values are rejected before reaching DB/query logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a52417fb-4095-4083-b5c9-e5eaca4cf991
📒 Files selected for processing (5)
apps/api/src/createApp.tsapps/api/src/routes/content/collections.test.tsapps/api/src/routes/content/collections.tsapps/api/test-utils/setup.tstsconfig.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/routes/content/collections.ts (1)
6-11: Consider adding minimum value constraints for pagination parameters.
pageandlimitaccept any number, including negative values. A negativepagewould produce a negative offset (line 116), and a zero or negativelimitcould cause unexpected behavior or DB errors.♻️ Proposed fix with minimum constraints
const CollectionsQueryParamsSchema = Type.Object({ locale: Type.String({ default: "en" }), - page: Type.Number(), - limit: Type.Number(), + page: Type.Number({ minimum: 0 }), + limit: Type.Number({ minimum: 1 }), author: Type.Optional(Type.String()), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/content/collections.ts` around lines 6 - 11, The CollectionsQueryParamsSchema currently allows any numbers for page and limit which can produce negative offsets and DB errors; update the schema's page and limit entries in CollectionsQueryParamsSchema to enforce minimum values (e.g., page minimum 1 and limit minimum 1, and consider sensible defaults) so validation rejects negatives/zero, and ensure the code that computes the offset from page and limit (the offset calculation that uses page and limit) relies on these validated values before querying the DB.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/routes/content/collections.ts`:
- Around line 6-11: The CollectionsQueryParamsSchema currently allows any
numbers for page and limit which can produce negative offsets and DB errors;
update the schema's page and limit entries in CollectionsQueryParamsSchema to
enforce minimum values (e.g., page minimum 1 and limit minimum 1, and consider
sensible defaults) so validation rejects negatives/zero, and ensure the code
that computes the offset from page and limit (the offset calculation that uses
page and limit) relies on these validated values before querying the DB.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 992f474c-924b-4beb-a483-c79fae274484
📒 Files selected for processing (1)
apps/api/src/routes/content/collections.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/routes/content/collections.ts (1)
7-12: Consider adding a maximum limit and sort parameter.
The
limitparameter has no maximum, allowing clients to request very large result sets which could impact performance.Issue
#84mentions supporting asortparameter (e.g., by date), but it's not included in this schema. Was this intentionally deferred?💡 Suggested addition for limit cap and sort
const CollectionsQueryParamsSchema = Type.Object({ locale: Type.String({ default: "en" }), page: Type.Number({ minimum: 0 }), - limit: Type.Number({ minimum: 1 }), + limit: Type.Number({ minimum: 1, maximum: 100 }), author: Type.Optional(Type.String()), + sort: Type.Optional(Type.Union([Type.Literal("date"), Type.Literal("title")])), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/content/collections.ts` around lines 7 - 12, CollectionsQueryParamsSchema lacks a max cap on limit and is missing the requested sort parameter; update the schema (CollectionsQueryParamsSchema) to set a reasonable maximum for the limit property (e.g., Type.Number with minimum and maximum) to prevent unbounded requests, and add an optional sort property (e.g., Type.Optional(Type.Union([...]) or Type.String with an enum of allowed sort keys like "date:asc", "date:desc" or "createdAt:asc/desc")) so callers can request ordered results; ensure any default behavior is documented or set via Type.Default if desired and validate the chosen sort values match the backend sort handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/routes/content/collections.ts`:
- Around line 7-12: CollectionsQueryParamsSchema lacks a max cap on limit and is
missing the requested sort parameter; update the schema
(CollectionsQueryParamsSchema) to set a reasonable maximum for the limit
property (e.g., Type.Number with minimum and maximum) to prevent unbounded
requests, and add an optional sort property (e.g.,
Type.Optional(Type.Union([...]) or Type.String with an enum of allowed sort keys
like "date:asc", "date:desc" or "createdAt:asc/desc")) so callers can request
ordered results; ensure any default behavior is documented or set via
Type.Default if desired and validate the chosen sort values match the backend
sort handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b97876f-af8b-4bd1-8c2e-07fafba7a94a
📒 Files selected for processing (2)
apps/api/src/routes/content/collections.test.tsapps/api/src/routes/content/collections.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/routes/content/collections.test.ts
fennifith
left a comment
fennifith
left a comment
There was a problem hiding this comment.
Looks good! The tests are super thorough - great work :D
2 participants
TODO:
Note: While working on the PR there was an error in
tsconfig.jsonbecause of the misname of the eslint config and missingallowJsproperty. I updated the config to suppress the error.Closes: #84
Summary by CodeRabbit