Add Drizzle migrations and expose category/task endpoints #1
Conversation
WalkthroughAdds project configs and CI, a comprehensive .gitignore, Drizzle ORM config plus migrations and snapshots (categories, profiles, tasks with RLS and indexes), a central Next.js API dispatcher wiring DB/auth/services, and thin Next.js route modules that forward requests to that dispatcher. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Route as Next.js Route
participant Dispatcher as dispatchToListeeApi
participant Hono as Hono Fetch Handler
participant Service as Service/Queries
participant DB as PostgreSQL (RLS)
Client->>Route: HTTP Request (/api/...)
Route->>Dispatcher: forward Request
Dispatcher->>Dispatcher: strip "/api" prefix\nrebuild Request URL
Dispatcher->>Hono: honoFetchHandler(new Request)
Hono->>Service: invoke services/queries (with auth)
Service->>DB: SQL queries/mutations
DB-->>Service: results (RLS applied)
Service-->>Hono: Response
Hono-->>Dispatcher: Response
Dispatcher-->>Route: Response
Route-->>Client: HTTP Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
package.json (1)
1-33: Consider adding an "engines" field to specify Node.js version requirements.Adding an
enginesfield helps ensure consistent runtime environments across development and production.Add an engines field to specify the required Node.js version:
{ "name": "listee-api", "version": "0.1.0", "private": true, + "engines": { + "node": ">=20.0.0" + }, "scripts": {tsconfig.json (1)
1-27: LGTM: TypeScript configuration is well-structured for Next.js 15.The configuration properly enables strict mode, path aliasing, and Next.js plugin support. The setup aligns with Next.js 15 best practices.
Consider updating the
targetto a more recent ECMAScript version if your deployment environment supports it:"compilerOptions": { - "target": "ES2017", + "target": "ES2022",ES2022 provides better performance and modern features while maintaining compatibility with Node.js 20+.
.gitignore (1)
34-34: Consider allowing .env.example files to be committed.The pattern
.env*will ignore all environment files including.env.example, which is commonly committed as a template for required environment variables.Apply this diff to allow example files:
# env files (can opt-in for committing if needed) -.env* +.env +.env.local +.env.*.local +!.env.exampleThis allows
.env.exampleto be committed while still ignoring actual environment files with secrets.src/app/api/healthz/route.ts (1)
1-1: Consider using consistent import style across route files.This file uses a relative import
"../handler", whilesrc/app/api/tasks/[taskId]/route.tsuses the path alias"@/app/api/handler". Using the path alias consistently improves maintainability.Apply this diff for consistency:
-import { dispatchToListeeApi } from "../handler"; +import { dispatchToListeeApi } from "@/app/api/handler";drizzle.config.ts (1)
1-20: Configuration is sound; consider adding URL format validation.The Drizzle configuration correctly validates the presence of
POSTGRES_URLand uses Next.js environment loading. The structure is appropriate for this use case.Optionally, consider validating the URL format to catch configuration errors early:
const databaseUrl = process.env.POSTGRES_URL; if (databaseUrl === undefined || databaseUrl.length === 0) { throw new Error("POSTGRES_URL is not set."); } + +try { + new URL(databaseUrl); +} catch { + throw new Error("POSTGRES_URL is not a valid URL."); +}src/app/api/handler.ts (2)
29-48: Consider resource cleanup strategy for database connections.The database connection and all dependencies are instantiated at module load, which is a singleton pattern. While this is common in Next.js, consider whether graceful shutdown or connection pooling cleanup is needed.
For production deployments:
- Verify that the database driver (@listee/db) handles connection pooling and cleanup automatically
- Consider implementing a cleanup handler for graceful shutdowns in containerized environments
- Monitor connection pool metrics to ensure connections are properly managed
If explicit cleanup is needed, you may need to expose a shutdown function and hook it into the Next.js server lifecycle.
16-27: Optional: Handle edge cases instripApiPrefix.The current implementation works for standard cases but could handle edge cases more defensively.
Consider these enhancements:
const stripApiPrefix = (pathname: string): string => { + // Normalize the pathname to handle double slashes + const normalized = pathname.replace(/\/+/g, '/'); + - if (!pathname.startsWith(API_PREFIX)) { - return pathname; + if (!normalized.startsWith(API_PREFIX)) { + return normalized; } - const stripped = pathname.slice(API_PREFIX.length); + const stripped = normalized.slice(API_PREFIX.length); if (stripped.length === 0) { return "/"; } return stripped; };This handles cases like
//api/categoriesor/api//categoriesmore robustly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.gitignore(1 hunks)biome.json(1 hunks)drizzle.config.ts(1 hunks)drizzle/0000_gigantic_colleen_wing.sql(1 hunks)drizzle/meta/0000_snapshot.json(1 hunks)drizzle/meta/_journal.json(1 hunks)next.config.ts(1 hunks)package.json(1 hunks)src/app/api/categories/[categoryId]/route.ts(1 hunks)src/app/api/categories/[categoryId]/tasks/route.ts(1 hunks)src/app/api/handler.ts(1 hunks)src/app/api/healthz/route.ts(1 hunks)src/app/api/tasks/[taskId]/route.ts(1 hunks)src/app/api/users/[userId]/categories/route.ts(1 hunks)tsconfig.json(1 hunks)
🔇 Additional comments (9)
next.config.ts (1)
1-7: LGTM: Minimal configuration is appropriate for initial setup.The empty Next.js configuration is valid for a new project. The TypeScript-based config file aligns with Next.js 15 best practices.
src/app/api/healthz/route.ts (1)
3-9: LGTM: Health check endpoint properly supports both GET and HEAD methods.Implementing both GET and HEAD methods for health checks is a best practice, allowing lightweight availability checks via HEAD requests.
biome.json (1)
22-25: The configuration is valid and correct for Biome 2.2.0.The
domainsconfiguration at the linter level is the proper syntax for Biome 2.2.0, where domain values can be "all" (enable all rules), "recommended" (enable only recommended rules), or "none" (disable the domain). Your biome.json configuration matches the official documentation pattern exactly.Likely an incorrect or invalid review comment.
drizzle/meta/_journal.json (1)
8-8: The review comment is based on incorrect timestamp analysis and should be ignored.The timestamp
1761059388883converts to October 21, 2025 at 15:09:48 UTC, which is 2 days in the past (relative to today, October 23, 2025). The original claim that this represents October 22, 2027 is incorrect. There is no system clock issue, no timezone misconfiguration, and no concern about migration ordering—the timestamp is valid and recent.Likely an incorrect or invalid review comment.
src/app/api/tasks/[taskId]/route.ts (1)
1-5: I need to gather more context about the intended scope of this endpoint before determining if the review comment's concern is valid.I've gathered information about Next.js 15 API conventions. Now let me search the codebase to understand whether this endpoint needs additional HTTP methods or is intentionally read-only.
Now let me search the codebase for test files or usage patterns to understand if CRUD operations are expected.
Based on my verification, I can now provide the rewritten review comment:
LGTM: Clean delegation pattern for API routing.
The route handler correctly delegates to the centralized
dispatchToListeeApifunction and follows Next.js 15 app router conventions. By default, unsupported methods return a 405 Method Not Allowed response, so the GET-only implementation is valid. Whether additional HTTP methods (PUT, PATCH, DELETE) should be added depends on your business requirements for this endpoint. Standard REST practice for[taskId]endpoints typically includes update and delete operations, but if this is intentionally read-only, the current implementation is appropriate.src/app/api/categories/[categoryId]/tasks/route.ts (1)
1-9: LGTM – Clean delegation pattern.The route handlers correctly delegate to the centralized API handler. This thin routing layer is appropriate for Next.js 15 and maintains consistency with the other route modules in this PR.
src/app/api/users/[userId]/categories/route.ts (1)
1-9: LGTM – Consistent with established pattern.The implementation correctly follows the centralized dispatch pattern used throughout the API routes.
src/app/api/categories/[categoryId]/route.ts (1)
1-5: LGTM – Follows established routing pattern.drizzle/0000_gigantic_colleen_wing.sql (1)
49-92: <!-- [scratchpad_update]
[task review_performance_concern_1/1]
Verification confirmed - RLS with EXISTS subqueries IS a known performance issue.[observations_updated]
- Web search confirms: "RLS policies in combination with subqueries are unfortunately not each other friends performance-wise" (12-1)
- Supabase official docs (15-1) explicitly state: "Make sure you've added indexes on any columns used within the Policies which are not already indexed"
- The review comment's concern about performance is valid and well-documented
- The recommendation to add indexes IS a best practice per Supabase and industry guidance
[analysis_updated]
- Review comment is NOT incorrect; it's actually following documented best practices
- The EXISTS subquery pattern in task policies DOES have performance implications at scale (11-23, 11-24)
- Adding composite index on
categories(id, created_by)would be beneficial for the EXISTS lookups- However, the initial script output showed there IS an index on categories but it's not optimally designed for this use case
[finding]
- The review comment is CORRECT and follows best practices
- The concern is legitimate: RLS with EXISTS subqueries needs careful index planning
- Current migration has
categories_system_name_idxon (created_by,name) but should have or add support for the specific pattern in the EXISTS query- The review comment's suggestions are all appropriate: monitoring, adding better index, denormalization, query analysis
[scratchpad_end] -->
Based on my verification, the review comment is accurate and aligns with documented PostgreSQL and Supabase best practices for RLS performance optimization.
The RLS policies with EXISTS subqueries do present potential performance concerns at scale.
RLS policies in combination with subqueries are unfortunately not each other friends performance-wise. Make sure you've added indexes on any columns used within the Policies which are not already indexed (or primary keys).
The current migration includes
categories_system_name_idxon(created_by, name)with a restrictive WHERE clause for system categories only. However, the EXISTS subquery checks bothcategories.idANDcategories.created_byacross all categories—a pattern not optimally covered by the existing index. Depending on the way that you construct your RLS policies, performance can vary significantly. In the best case, Postgres can optimize the RLS policy to be as cheap as an additional WHERE clause on your query, or a simple function call before a write. In a much worse case, it can cause a sub-query, per row returned from a database query, which results in queries scaling exponentially.The suggestions in the review comment—adding an index on
categories(id, created_by), denormalization, and query plan analysis—are all legitimate optimization strategies recommended by the PostgreSQL community for this exact pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/api/categories/[categoryId]/route.ts (1)
3-13: Implementation follows the established pattern correctly.The thin proxy pattern is consistent with other routes in the codebase, and the delegation to
dispatchToListeeApiworks as intended.Optional: Consider adopting Next.js 15 route handler conventions.
While the current implementation works (because
dispatchToListeeApiforwards the full URL), Next.js 15 route handlers with dynamic segments should include the context parameter. This would make the code more idiomatic and future-proof:-export async function GET(request: Request): Promise<Response> { +export async function GET( + request: Request, + context: { params: Promise<{ categoryId: string }> } +): Promise<Response> { return await dispatchToListeeApi(request); } -export async function PATCH(request: Request): Promise<Response> { +export async function PATCH( + request: Request, + context: { params: Promise<{ categoryId: string }> } +): Promise<Response> { return await dispatchToListeeApi(request); } -export async function DELETE(request: Request): Promise<Response> { +export async function DELETE( + request: Request, + context: { params: Promise<{ categoryId: string }> } +): Promise<Response> { return await dispatchToListeeApi(request); }Note: If you adopt this pattern, consider applying it consistently across all dynamic routes (e.g.,
tasks/[taskId]/route.ts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/api/categories/[categoryId]/route.ts(1 hunks)src/app/api/tasks/[taskId]/route.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/api/tasks/[taskId]/route.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/api/categories/[categoryId]/route.ts (4)
src/app/api/tasks/[taskId]/route.ts (3)
GET(3-5)PATCH(7-9)DELETE(11-13)src/app/api/categories/[categoryId]/tasks/route.ts (1)
GET(3-5)src/app/api/users/[userId]/categories/route.ts (1)
GET(3-5)src/app/api/handler.ts (1)
dispatchToListeeApi(50-59)
🔇 Additional comments (1)
src/app/api/categories/[categoryId]/route.ts (1)
1-1: LGTM!The import is correct and follows the project's path alias conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
18-18: Consider pinning reusable workflows to a semantic version tag instead of@main.The workflow references external reusable workflows from
listee-ci@main, which means the CI behavior is coupled to the main branch of that repository. This introduces implicit dependencies and potential brittleness if that repository's main branch changes unexpectedly.For stability and reproducibility, consider pinning to a semantic version tag (e.g.,
@v1.0.0) or a commit SHA instead:- lint: - uses: listee-dev/listee-ci/.github/workflows/lint.yml@main + lint: + uses: listee-dev/listee-ci/.github/workflows/lint.yml@v1.0.0If
@mainis intentional for staying up-to-date with the latest workflows, document this decision.Also applies to: 21-21, 24-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
11-14: Permissions configuration looks appropriate.The explicit permissions (contents, pull-requests, id-token with write access) align with common CI use cases for code updates, PR commenting, and OIDC workflows.
gentamura
force-pushed
the
feature-project-api-init
branch
from
bd94836 to
7ae7bdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/ci.yml
[medium] 17-18: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
3-9: LGTM! Workflow triggers are appropriately configured to run on main pushes and all PR branches.
11-14: LGTM! Permissions are appropriately scoped for the workflow's needs.
20-28: LGTM! The reusable workflow pattern is clean and follows the DRY principle, delegating lint, typecheck, and test responsibilities to centralized CI workflows.
gentamura
force-pushed
the
feature-project-api-init
branch
from
5beadaf to
f29ad6c
Compare
gentamura
force-pushed
the
feature-project-api-init
branch
from
f29ad6c to
c9c6c94
Compare
Summary
Summary by CodeRabbit
New Features
Chores