Conversation
|
Warning Rate limit exceeded@gentamura has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds update and delete operations for categories and tasks across types, repositories, services, queries, routes, and tests, including new PATCH/DELETE route handlers, repository-level access checks, and in-memory fixtures plus end-to-end tests exercising update/delete flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Routes
participant Queries
participant Services
participant Repos
participant DB
rect rgb(235,245,255)
Note over Client,Routes: PATCH /<resource>/:id (update)
Client->>Routes: PATCH /categories/:categoryId (body)
Routes->>Routes: auth + parseUpdatePayload
Routes->>Queries: update({ id, userId, ...fields })
Queries->>Services: update(params)
Services->>Repos: update(repoParams)
Repos->>DB: UPDATE ... WHERE id = ? AND access_check
DB-->>Repos: updated row | null
Repos-->>Services: entity | null
Services-->>Queries: entity | null
Queries-->>Routes: entity | null
alt found
Routes-->>Client: 200 + updated entity
else not found
Routes-->>Client: 404
end
end
rect rgb(255,245,235)
Note over Client,Routes: DELETE /<resource>/:id
Client->>Routes: DELETE /tasks/:taskId
Routes->>Routes: auth
Routes->>Queries: delete({ id, userId })
Queries->>Services: delete(params)
Services->>Repos: delete(repoParams)
Repos->>DB: DELETE ... WHERE id = ? AND access_check
DB-->>Repos: rowsDeleted (0|1)
Repos-->>Services: boolean
Services-->>Queries: boolean
Queries-->>Routes: boolean
alt deleted
Routes-->>Client: 204 No Content
else not found
Routes-->>Client: 404
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![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
🧹 Nitpick comments (14)
packages/api/src/queries/task-queries.ts (1)
38-46: Task update/delete wiring looks correct; optional pass‑throughThe delegation and surface match the types; no business logic leaks. Optionally, you could forward params directly to reduce boilerplate:
- update:
return dependencies.service.update(params)- delete:
return dependencies.service.delete(params)
Keep as-is if you prefer explicit whitelisting.Also applies to: 48-53, 59-61
packages/api/src/routes/tasks.ts (1)
25-29: Harden PATCH payload validation (hasOwn, optional extra-key guard)Current logic is good. For stricter input hygiene:
- Use Object.hasOwn to avoid inherited props:
const hasName = Object.hasOwn(value, "name"), etc.- Optionally reject unknown keys to catch typos: ensure
Object.keys(value).every(k => k === "name" || k === "description" || k === "isChecked").
Behavior stays the same (trim, null handling) while tightening checks.
Would you like a small diff applying these two tweaks?Also applies to: 66-116
packages/api/src/services/category-service.ts (1)
37-47: Category service update/delete correctly delegate; consider optional no‑op guardWiring is correct and sets updatedBy. Optionally guard updates with no fields (name/kind both undefined) to avoid metadata-only writes, unless you rely on updatedAt bumps.
Also applies to: 49-56, 62-64
packages/api/src/routes/categories.ts (1)
30-33: Tighten PATCH payload parsing (hasOwn + extra-keys check)Use
Object.hasOwnfor presence checks and optionally reject unexpected keys (only allow "name" and "kind") to catch typos early. Keeps current behavior while strengthening validation.
Want a quick diff to apply the same pattern here and in tasks for consistency?Also applies to: 55-90
packages/api/src/queries/category-queries.ts (1)
39-46: Category update/delete delegation looks good; optional pass‑throughImplementation matches the service surface and guidelines. As an optional cleanup, forward
paramsdirectly to service to cut repetition (keep explicit mapping if you prefer whitelisting).Also applies to: 48-53, 59-61
packages/api/src/repositories/category-repository.ts (1)
174-203: Update logic is correct; consider guarding empty field updatesThe partial update and scoping are correct. Optionally short‑circuit when both
nameandkindare undefined to avoid metadata‑only writes, unless you intentionally want updatedAt bumps.packages/api/src/repositories/task-repository.ts (2)
106-120: Delete has the same ownership constraintDelete is limited to tasks.createdBy=userId. If category owners should be able to delete tasks in their categories (as the policy suggests), mirror the same fix here.
2-2: Operator imports for broader conditionIf you adopt the policy-aligned WHERE, you’ll need or (and possibly a categories reference). Ensure they’re imported from @listee/db.
packages/api/src/app.test.ts (4)
149-169: Category PATCH happy-path coveredTest is clear and assertions are appropriate.
Add a test where PATCH has no updatable fields (should 400) and a test updating kind to ensure route validation/domain rules are enforced.
171-190: Category DELETE happy-path coveredSuggest adding a not-found delete case to assert 404 mapping when service returns false.
278-298: Task PATCH happy-path covered; null handling worth testingCurrent in-memory implementation preserves boolean false via ?? and allows description to become null via explicit undefined check. Add tests for:
- isChecked: false
- description: null
300-316: Task DELETE happy-path coveredAlso add a not-found case to ensure DELETE /tasks/:id -> 404 when service returns false.
packages/types/src/api.ts (2)
31-37: Enforce “at least one field” at the type level (optional)To prevent empty PATCHes, define UpdateCategoryParams as an intersection with an AtLeastOne of {name, kind}.
-export interface UpdateCategoryParams { - readonly categoryId: string; - readonly userId: string; - readonly name?: string; - readonly kind?: string; -} +type AtLeastOne<T> = { [K in keyof T]-?: Required<Pick<T, K>> & Partial<Omit<T, K>> }[keyof T]; +export type UpdateCategoryParams = + { readonly categoryId: string; readonly userId: string } & + AtLeastOne<{ readonly name?: string; readonly kind?: string }>;
69-76: Apply AtLeastOne to task updates too (optional)Mirror category approach to disallow empty updates while still allowing description=null and isChecked=false.
-export interface UpdateTaskParams { - readonly taskId: string; - readonly userId: string; - readonly name?: string; - readonly description?: string | null; - readonly isChecked?: boolean; -} +export type UpdateTaskParams = + { readonly taskId: string; readonly userId: string } & + AtLeastOne<{ + readonly name?: string; + readonly description?: string | null; + readonly isChecked?: boolean; + }>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/api/src/app.test.ts(4 hunks)packages/api/src/queries/category-queries.ts(2 hunks)packages/api/src/queries/task-queries.ts(2 hunks)packages/api/src/repositories/category-repository.ts(2 hunks)packages/api/src/repositories/task-repository.ts(2 hunks)packages/api/src/routes/categories.ts(3 hunks)packages/api/src/routes/tasks.ts(3 hunks)packages/api/src/services/category-service.ts(2 hunks)packages/api/src/services/task-service.ts(2 hunks)packages/types/src/api.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
packages/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/*/src/**/*.{ts,tsx}: Avoid implicit any; TypeScript is run with strict enabled
Prefer type guards or the satisfies operator over as casts where appropriate
Prefer unknown for external inputs
Use PascalCase for types and enums
Use camelCase for variables and functions
Ensure source comments are written in English
Files:
packages/api/src/queries/task-queries.tspackages/api/src/routes/tasks.tspackages/api/src/routes/categories.tspackages/api/src/services/category-service.tspackages/api/src/repositories/category-repository.tspackages/api/src/app.test.tspackages/api/src/services/task-service.tspackages/api/src/repositories/task-repository.tspackages/api/src/queries/category-queries.tspackages/types/src/api.ts
packages/*/src/{routes,queries,services,repositories}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Maintain dependency direction routes → queries → services → repositories
Files:
packages/api/src/queries/task-queries.tspackages/api/src/routes/tasks.tspackages/api/src/routes/categories.tspackages/api/src/services/category-service.tspackages/api/src/repositories/category-repository.tspackages/api/src/services/task-service.tspackages/api/src/repositories/task-repository.tspackages/api/src/queries/category-queries.ts
packages/*/src/queries/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Queries should compose services/repositories per use case and inject dependencies via factories
Files:
packages/api/src/queries/task-queries.tspackages/api/src/queries/category-queries.ts
packages/*/src/routes/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Routes must delegate exclusively to queries and handle HTTP translation/status; avoid business logic in routes
Files:
packages/api/src/routes/tasks.tspackages/api/src/routes/categories.ts
packages/*/src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Services may depend on repositories, but keep service layer thin and extract complex domain logic into dedicated modules
Files:
packages/api/src/services/category-service.tspackages/api/src/services/task-service.ts
packages/*/src/repositories/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Repositories must not depend on services and should encapsulate external access, returning plain or domain-specific types
Files:
packages/api/src/repositories/category-repository.tspackages/api/src/repositories/task-repository.ts
packages/*/src/**/{*.test.ts,__tests__/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
packages/*/src/**/{*.test.ts,__tests__/**/*.ts}: Write test names and descriptions in English
Co-locate tests as *.test.ts or under tests/ using Bun’s test runner
Files:
packages/api/src/app.test.ts
🧬 Code graph analysis (10)
packages/api/src/queries/task-queries.ts (1)
packages/types/src/api.ts (2)
UpdateTaskParams(69-75)DeleteTaskParams(77-80)
packages/api/src/routes/tasks.ts (3)
packages/api/src/utils/validation.ts (3)
isRecord(1-3)isNonEmptyString(5-11)isBoolean(13-15)packages/api/src/routes/auth-utils.ts (1)
tryAuthenticate(7-20)packages/api/src/utils/error.ts (1)
toErrorMessage(1-21)
packages/api/src/routes/categories.ts (3)
packages/api/src/utils/validation.ts (2)
isRecord(1-3)isNonEmptyString(5-11)packages/api/src/routes/auth-utils.ts (1)
tryAuthenticate(7-20)packages/api/src/utils/error.ts (1)
toErrorMessage(1-21)
packages/api/src/services/category-service.ts (2)
packages/types/src/api.ts (2)
UpdateCategoryParams(31-36)DeleteCategoryParams(38-41)packages/db/src/schema/index.ts (1)
Category(107-107)
packages/api/src/repositories/category-repository.ts (2)
packages/types/src/api.ts (2)
UpdateCategoryRepositoryParams(107-113)DeleteCategoryRepositoryParams(115-118)packages/db/src/schema/index.ts (2)
Category(107-107)categories(59-105)
packages/api/src/app.test.ts (4)
packages/api/src/queries/category-queries.ts (1)
createCategoryQueries(11-62)packages/api/src/app.ts (1)
createApp(7-22)packages/db/src/schema/index.ts (4)
categories(59-105)tasks(109-162)Category(107-107)Task(164-164)packages/api/src/queries/task-queries.ts (1)
createTaskQueries(11-62)
packages/api/src/services/task-service.ts (1)
packages/types/src/api.ts (2)
UpdateTaskParams(69-75)DeleteTaskParams(77-80)
packages/api/src/repositories/task-repository.ts (2)
packages/types/src/api.ts (2)
UpdateTaskRepositoryParams(163-170)DeleteTaskRepositoryParams(172-175)packages/db/src/schema/index.ts (2)
Task(164-164)tasks(109-162)
packages/api/src/queries/category-queries.ts (1)
packages/types/src/api.ts (2)
UpdateCategoryParams(31-36)DeleteCategoryParams(38-41)
packages/types/src/api.ts (1)
packages/types/src/db.ts (3)
Category(11-11)Task(11-11)PaginatedResult(22-26)
🔇 Additional comments (25)
packages/api/src/routes/tasks.ts (2)
239-274: PATCH /tasks/:taskId flow is clean and layeredAuth → parse → queries.update → 404/200 responses are correct; no business logic in the route.
276-296: DELETE /tasks/:taskId semantics look goodAuth, boolean result mapping to 404/204, and error handling are all consistent.
packages/api/src/routes/categories.ts (2)
230-264: PATCH /categories/:categoryId route is solidAuth → parse → queries.update with 404/200 handling; clean separation of concerns.
266-286: DELETE /categories/:categoryId route LGTMBoolean result correctly mapped to 404/204; error handling consistent.
packages/api/src/repositories/category-repository.ts (1)
205-219: Delete and export surface LGTMScoped delete by (id, createdBy) and boolean return are appropriate; return map exposes
deleteasdeleteCategory.Also applies to: 225-227
packages/api/src/services/task-service.ts (3)
49-58: Service update: thin and correct forwardingForwarding to repository with updatedBy=userId is correct. Ensure the HTTP layer validates that at least one of name, description, or isChecked is present to avoid no-op updates.
60-65: Delete delegation looks goodDelegates to repository and returns boolean. No concerns.
71-73: Public API shape alignmentExposing update and delete on the returned service matches the expanded TaskService interface.
packages/api/src/repositories/task-repository.ts (1)
122-128: Repository surface expansion LGTMExposing update and delete aligns with TaskRepository in types.
packages/api/src/app.test.ts (4)
375-395: In-memory category update logic is correctCorrectly guards not-found, merges fields, and preserves existing values when undefined; updates updatedBy/updatedAt.
396-407: In-memory category delete logic is correctReturns boolean and prunes the item.
445-467: In-memory task update merges fields safely
- name uses ?? (no null type anyway).
- description distinguishes undefined vs null to allow clearing.
- isChecked uses ?? to preserve false.
All good.
468-479: In-memory task delete logic is correctBoolean result and removal are correct.
packages/types/src/api.ts (12)
21-23: Queries surface: additions consistentupdate/delete additions for CategoryQueries look consistent with the rest of the stack.
38-42: Delete params look goodMinimal identifiers only; matches usage.
57-59: TaskQueries additions are consistentupdate/delete signatures match CategoryQueries structure.
77-81: Delete task params are fineNo changes suggested.
107-114: Repository params include updatedByGood separation: service params don’t include updatedBy; repository params do.
115-119: DeleteCategoryRepositoryParams OKMatches service/query shapes.
126-128: CategoryRepository additions consistentSignatures align with params.
136-138: CategoryService additions consistentThin pass-through design supported.
163-170: UpdateTaskRepositoryParams shape is correctCarries updatedBy and optional fields. Good.
172-176: DeleteTaskRepositoryParams OKNo issues.
181-183: TaskRepository additions consistentOK as per interfaces.
189-191: TaskService additions consistentNo concerns.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/api/src/repositories/task-repository.ts (3)
2-2: Addsqlimport to support single-statement access checks.To harden UPDATE/DELETE (see comments below), we’ll use an EXISTS subquery. Import sql:
-import { and, categories, eq, or, tasks } from "@listee/db"; +import { and, categories, eq, or, tasks } from "@listee/db"; +import { sql } from "drizzle-orm";
14-31: Access check now matches RLS (creator OR category owner).Good fix; aligns with tasks.hasAccess policy semantics. Consider extracting a reusable predicate builder to avoid duplication across queries.
65-93: findById access path looks good; consider DRYing the predicate.Logic is correct; optionally reuse the same access predicate helper used in list/update/delete to reduce drift risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/api/src/repositories/task-repository.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
packages/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/*/src/**/*.{ts,tsx}: Avoid implicit any; TypeScript is run with strict enabled
Prefer type guards or the satisfies operator over as casts where appropriate
Prefer unknown for external inputs
Use PascalCase for types and enums
Use camelCase for variables and functions
Ensure source comments are written in English
Files:
packages/api/src/repositories/task-repository.ts
packages/*/src/{routes,queries,services,repositories}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Maintain dependency direction routes → queries → services → repositories
Files:
packages/api/src/repositories/task-repository.ts
packages/*/src/repositories/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Repositories must not depend on services and should encapsulate external access, returning plain or domain-specific types
Files:
packages/api/src/repositories/task-repository.ts
🧬 Code graph analysis (1)
packages/api/src/repositories/task-repository.ts (3)
packages/db/src/index.ts (3)
Database(131-131)and(581-581)or(581-581)packages/types/src/api.ts (5)
TaskRepository(177-183)ListTasksRepositoryParams(144-147)FindTaskRepositoryParams(149-152)UpdateTaskRepositoryParams(163-170)DeleteTaskRepositoryParams(172-175)packages/db/src/schema/index.ts (3)
tasks(109-162)categories(59-105)Task(164-164)
🔇 Additional comments (2)
packages/api/src/repositories/task-repository.ts (2)
5-5: Types import aligns with new repository surface.Matches @listee/types TaskRepository contract. No issues.
Also applies to: 10-10
169-170: Repository surface wiring looks correct.update and delete exposed as per @listee/types TaskRepository.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/api/src/app.test.ts(4 hunks)packages/api/src/queries/category-queries.ts(2 hunks)packages/api/src/repositories/category-repository.ts(2 hunks)packages/api/src/routes/categories.ts(3 hunks)packages/api/src/services/category-service.ts(2 hunks)packages/types/src/api.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/api/src/services/category-service.ts
- packages/api/src/app.test.ts
- packages/types/src/api.ts
🧰 Additional context used
📓 Path-based instructions (5)
packages/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/*/src/**/*.{ts,tsx}: Avoid implicit any; TypeScript is run with strict enabled
Prefer type guards or the satisfies operator over as casts where appropriate
Prefer unknown for external inputs
Use PascalCase for types and enums
Use camelCase for variables and functions
Ensure source comments are written in English
Files:
packages/api/src/repositories/category-repository.tspackages/api/src/queries/category-queries.tspackages/api/src/routes/categories.ts
packages/*/src/{routes,queries,services,repositories}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Maintain dependency direction routes → queries → services → repositories
Files:
packages/api/src/repositories/category-repository.tspackages/api/src/queries/category-queries.tspackages/api/src/routes/categories.ts
packages/*/src/repositories/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Repositories must not depend on services and should encapsulate external access, returning plain or domain-specific types
Files:
packages/api/src/repositories/category-repository.ts
packages/*/src/queries/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Queries should compose services/repositories per use case and inject dependencies via factories
Files:
packages/api/src/queries/category-queries.ts
packages/*/src/routes/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Routes must delegate exclusively to queries and handle HTTP translation/status; avoid business logic in routes
Files:
packages/api/src/routes/categories.ts
🧬 Code graph analysis (3)
packages/api/src/repositories/category-repository.ts (3)
packages/types/src/api.ts (2)
UpdateCategoryRepositoryParams(106-111)DeleteCategoryRepositoryParams(113-116)packages/db/src/schema/index.ts (2)
Category(107-107)categories(59-105)packages/db/src/index.ts (2)
and(581-581)eq(581-581)
packages/api/src/queries/category-queries.ts (1)
packages/types/src/api.ts (2)
UpdateCategoryParams(31-35)DeleteCategoryParams(37-40)
packages/api/src/routes/categories.ts (3)
packages/api/src/utils/validation.ts (2)
isRecord(1-3)isNonEmptyString(5-11)packages/api/src/routes/auth-utils.ts (1)
tryAuthenticate(7-20)packages/api/src/utils/error.ts (1)
toErrorMessage(1-21)
🔇 Additional comments (9)
packages/api/src/routes/categories.ts (3)
30-32: LGTM!The
UpdateCategoryPayloadinterface correctly defines the shape for partial category updates with an optionalnamefield.
221-254: LGTM with note on validation.The PATCH route handler correctly implements authentication, JSON parsing, payload validation, and error handling. The 404 response for missing categories and the 200 response with updated data follow REST conventions.
Once the validation logic in
parseUpdateCategoryPayloadis fixed to allow optional fields, verify that empty PATCH requests are handled correctly (either as no-op updates or with an appropriate error message).
256-276: LGTM!The DELETE route handler correctly implements authentication, delegates to queries, and returns appropriate HTTP status codes (404 for not found, 204 for successful deletion, 500 for errors). The implementation follows REST conventions.
packages/api/src/repositories/category-repository.ts (3)
174-199: LGTM!The
updatefunction correctly implements partial updates by:
- Always setting
updatedByandupdatedAtfor audit tracking- Conditionally updating
nameonly when provided- Properly scoping the update to both
categoryIdanduserIdfor authorization- Returning
nullwhen the category is not found or doesn't belong to the user
201-215: LGTM!The
deleteCategoryfunction correctly implements deletion with proper authorization scoping (bothcategoryIdanduserId). Returning a boolean to indicate success allows the caller to distinguish between "not found" and other errors.
217-223: LGTM!The repository correctly exposes all CRUD operations including the new
updateanddeletemethods. The implementation maintains consistency with the existing pattern.packages/api/src/queries/category-queries.ts (3)
39-45: LGTM!The
updatefunction correctly delegates to the service layer, forwarding all necessary parameters. This maintains the proper dependency direction (queries → services).
47-52: LGTM!The
deleteCategoryfunction correctly delegates to the service layer, forwarding all necessary parameters. This maintains the proper dependency direction (queries → services).
54-60: LGTM!The query layer correctly exposes all operations including the new
updateanddeletemethods, maintaining consistency with the existing pattern and the CategoryQueries interface.
gentamura
deleted the
codex/implement-update-and-delete-for-categories-and-tasks
branch
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68fb09059e9483318cfec1f51fc55fbc
Summary by CodeRabbit