feat(tasks): [#8] Implement task CRUD API endpoints#22
Open
feat(tasks): [#8] Implement task CRUD API endpoints#22
Conversation
- Define BoardColumn and Task entities strictly mapping to the SQL schema - Create BoardColumnsModule with full CRUD and project ownership validation - Create TasksModule with full CRUD, column validation, and auto-ordering - Protect all new endpoints using JwtAuthGuard - Implement ParseIntPipe for all ID parameters to match SERIAL database columns
📝 WalkthroughWalkthroughThis PR introduces two new feature modules (BoardColumns and Tasks) to the NestJS backend application. Each module includes JWT-protected CRUD controllers, services with project access verification, DTOs with validation, TypeORM entities with relational mappings, and unit tests. The Projects module is updated to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant JwtAuthGuard
participant Service
participant Database
Client->>Controller: POST /board-columns (with JWT token)
Controller->>JwtAuthGuard: Validate token
JwtAuthGuard->>JwtAuthGuard: Extract userId
JwtAuthGuard->>Controller: Auth success, inject userId
Controller->>Service: create(createBoardColumnDto, userId)
Service->>Database: Load Project (verify access)
Database->>Service: Project data
Service->>Service: verifyProjectAccess()
alt User owns project
Service->>Database: Load last column order
Database->>Service: order value
Service->>Service: Calculate next order
Service->>Database: Create & save BoardColumn
Database->>Service: BoardColumn entity
Service->>Controller: Return created BoardColumn
Controller->>Client: 201 Created + BoardColumn JSON
else User denied access
Service->>Controller: ForbiddenException
Controller->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
apps/backend/src/tasks/dto/create-task.dto.ts (1)
16-18: Consider adding@IsPositive()to reject invalid IDs earlier.While
verifyColumnAccessin the service will catch non-existent columns, adding@IsPositive()would fail-fast at validation forcolumnId <= 0, avoiding an unnecessary database lookup.♻️ Proposed enhancement
-import { IsNotEmpty, IsOptional, IsString, IsNumber } from 'class-validator'; +import { IsNotEmpty, IsOptional, IsString, IsNumber, IsPositive } from 'class-validator'; export class CreateTaskDto { `@IsString`() `@IsNotEmpty`() title: string; `@IsString`() `@IsOptional`() description?: string; `@IsNumber`() `@IsOptional`() order?: number; `@IsNumber`() + `@IsPositive`() `@IsNotEmpty`() columnId: number; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/tasks/dto/create-task.dto.ts` around lines 16 - 18, Add fast-fail validation to the DTO by decorating the columnId property with `@IsPositive`() so invalid IDs (<= 0) are rejected before service calls; update the CreateTaskDto's columnId field (which currently has `@IsNumber`() and `@IsNotEmpty`()) to also include `@IsPositive`() to prevent unnecessary verifyColumnAccess DB lookups.apps/backend/src/tasks/tasks.service.ts (1)
77-83: Stub implementations need completion or removal.
findAll()andfindOne()return placeholder strings instead of actual data. If these endpoints are exposed via the controller, they will return unexpected responses.Should I help implement these methods with proper query logic and access verification, or should they be removed if not required by the current scope?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/tasks/tasks.service.ts` around lines 77 - 83, Replace the placeholder returns in TasksService.findAll and TasksService.findOne with real data operations: use the service's data access layer (e.g., injected repository/prisma client used elsewhere in this service) to query tasks, apply request-scoped filters (ownerId, teamId, permissions) and paging/ordering for findAll, and validate access/throw NotFound/Forbidden for findOne when the task doesn't exist or the caller lacks access; if these methods are not needed, remove the controller routes that call findAll/findOne or delete the methods entirely to avoid exposing stub responses.
🤖 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/backend/src/board-columns/board-columns.controller.spec.ts`:
- Around line 9-12: The test registers the concrete BoardColumnsService which
depends on TypeORM repositories and causes module compilation to fail; instead,
in the Test.createTestingModule call provide a mocked service using { provide:
BoardColumnsService, useValue: mockBoardColumnsService } (replace the providers:
[BoardColumnsService] entry), where mockBoardColumnsService is an object with
jest.fn() stubs for the methods the BoardColumnsController tests exercise (e.g.,
the controller's create/find/update/delete handlers); this keeps the test
isolated from repositories and allows the module to compile when instantiating
BoardColumnsController.
In `@apps/backend/src/board-columns/board-columns.controller.ts`:
- Around line 43-46: The findOne controller method currently calls
boardColumnsService.findOne(id) without user context so ownership checks can't
run; update the BoardColumnsController.findOne to accept the request user (e.g.,
add `@Req`() req: Request or `@User`() user param) and pass req.user.userId into the
service call (change to boardColumnsService.findOne(id, userId)); ensure the
service method signature (boardColumnsService.findOne) is updated to accept and
enforce the userId for authorization if not already implemented.
In `@apps/backend/src/board-columns/board-columns.service.spec.ts`:
- Around line 7-12: The test fails because BoardColumnsService depends on two
injected repositories (BoardColumn and Project); update the
Test.createTestingModule setup to provide mocks for those repositories (use the
Nest testing pattern with getRepositoryToken(BoardColumn) and
getRepositoryToken(Project) or equivalent providers) so dependency injection can
resolve them, then retrieve BoardColumnsService as before; ensure the mock
providers are included alongside BoardColumnsService in the module's providers
array to prevent compile-time resolution errors.
In `@apps/backend/src/board-columns/board-columns.service.ts`:
- Around line 56-63: The current order assignment in the create flow (reading
createDto.order and using columnsRepository.findOne to fetch the lastColumn) is
race-prone under concurrent creates; fix by making order allocation atomic —
either wrap the create logic in a DB transaction and acquire a lock on the
project's columns (e.g., select the project's columns with FOR UPDATE before
computing lastColumn.order and inserting) in the service method that handles
creation (the method that reads createDto and calls columnsRepository.findOne),
or add a unique constraint on (project_id, order) and implement a small retry
loop that catches unique-constraint violations and recomputes the next order
before retrying the insert; update the board-columns service create flow (where
createDto and columnsRepository are used) to use one of these approaches.
- Around line 77-79: The findOne method in BoardColumnsService is still a
scaffold that returns a string; replace it with the real lookup that fetches the
BoardColumn by id, enforces ownership using the same access-check path as
update/delete (reuse the existing check/permission logic used in update() and
remove()), and throw/not-found or forbidden errors as appropriate; ensure it
returns the actual entity (or DTO) instead of a string and update the controller
call to pass req.user.userId into findOne so ownership can be validated.
In `@apps/backend/src/board-columns/dto/create-board-column.dto.ts`:
- Around line 8-14: Replace the loose numeric validators on the DTO with
integer-specific validators: change the decorators on the order and projectId
properties in CreateBoardColumnDto from `@IsNumber`() to `@IsInt`(), import `@IsInt`
and `@Min` from class-validator, add `@Min`(0) on order (since it’s optional) and
`@Min`(1) on projectId to enforce positive IDs, and update the imports to remove
`@IsNumber` and include `@IsInt` and `@Min` so runtime validation matches the INT DB
schema.
In `@apps/backend/src/tasks/entities/task.entity.ts`:
- Around line 20-21: The Task entity's description column is declared nullable
but the property is typed as string; change the property type for description to
string | null to match the Column({ nullable: true }) decorator and
strictNullChecks. Locate the description property in the Task entity (the
`@Column`({ nullable: true }) description declaration) and update its TypeScript
type to string | null; ensure any usages handle possible null values or add
appropriate null-checks where used.
In `@apps/backend/src/tasks/tasks.controller.spec.ts`:
- Around line 9-12: The test currently instantiates the real TasksService which
requires repository dependencies; change the Test.createTestingModule setup to
provide a mocked TasksService instead of the real provider: in the module config
replace providers: [TasksService] with a provider object for TasksService using
useValue (or useClass) that supplies mocked method implementations used by
TasksController tests (e.g., mock methods like create, findAll, etc.), then
retrieve the controller from the compiled TestingModule as before so controller
unit tests run without injecting Task/BoardColumn repositories.
In `@apps/backend/src/tasks/tasks.controller.ts`:
- Around line 31-39: The findAll and findOne controller methods currently don't
scope queries to the authenticated user; update findAll and findOne to accept
the request object via `@Request`() (extract userId from req.user) and pass userId
into tasksService.findAll(...) and tasksService.findOne(...), and then update
the corresponding service methods (tasksService.findAll and
tasksService.findOne) to require a userId parameter and enforce
ownership/validation (reuse the same pattern as verifyTaskAccess used by
update/remove to check project/task ownership and throw if not authorized) so
controllers only return tasks for the authenticated user.
In `@apps/backend/src/tasks/tasks.service.spec.ts`:
- Around line 7-12: The test fails because TasksService depends on two injected
repositories (the `@InjectRepository`(Task) and `@InjectRepository`(BoardColumn)
injections used by TasksService) which are not provided in the
Test.createTestingModule providers; add provider mocks for those repositories
using getRepositoryToken(Task) and getRepositoryToken(BoardColumn) and supply
simple mock objects (e.g., objects with jest.fn() stubs for methods used by
TasksService) when building the testing module before calling module.compile();
update the providers array to include { provide: getRepositoryToken(Task),
useValue: taskRepoMock } and { provide: getRepositoryToken(BoardColumn),
useValue: boardColumnRepoMock } so module.get<TasksService>(TasksService) can
resolve successfully.
---
Nitpick comments:
In `@apps/backend/src/tasks/dto/create-task.dto.ts`:
- Around line 16-18: Add fast-fail validation to the DTO by decorating the
columnId property with `@IsPositive`() so invalid IDs (<= 0) are rejected before
service calls; update the CreateTaskDto's columnId field (which currently has
`@IsNumber`() and `@IsNotEmpty`()) to also include `@IsPositive`() to prevent
unnecessary verifyColumnAccess DB lookups.
In `@apps/backend/src/tasks/tasks.service.ts`:
- Around line 77-83: Replace the placeholder returns in TasksService.findAll and
TasksService.findOne with real data operations: use the service's data access
layer (e.g., injected repository/prisma client used elsewhere in this service)
to query tasks, apply request-scoped filters (ownerId, teamId, permissions) and
paging/ordering for findAll, and validate access/throw NotFound/Forbidden for
findOne when the task doesn't exist or the caller lacks access; if these methods
are not needed, remove the controller routes that call findAll/findOne or delete
the methods entirely to avoid exposing stub responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8240ef97-d971-4a51-8de9-43405d16c44b
📒 Files selected for processing (19)
apps/backend/src/app.module.tsapps/backend/src/board-columns/board-columns.controller.spec.tsapps/backend/src/board-columns/board-columns.controller.tsapps/backend/src/board-columns/board-columns.module.tsapps/backend/src/board-columns/board-columns.service.spec.tsapps/backend/src/board-columns/board-columns.service.tsapps/backend/src/board-columns/dto/create-board-column.dto.tsapps/backend/src/board-columns/dto/update-board-column.dto.tsapps/backend/src/board-columns/entities/board-column.entity.tsapps/backend/src/projects/entities/project.entity.tsapps/backend/src/projects/projects.controller.tsapps/backend/src/tasks/dto/create-task.dto.tsapps/backend/src/tasks/dto/update-task.dto.tsapps/backend/src/tasks/entities/task.entity.tsapps/backend/src/tasks/tasks.controller.spec.tsapps/backend/src/tasks/tasks.controller.tsapps/backend/src/tasks/tasks.module.tsapps/backend/src/tasks/tasks.service.spec.tsapps/backend/src/tasks/tasks.service.ts
Comment on lines
+9
to
+12
| const module: TestingModule = await Test.createTestingModule({ | ||
| controllers: [BoardColumnsController], | ||
| providers: [BoardColumnsService], | ||
| }).compile(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether BoardColumnsService has injected constructor dependencies
rg -n -C2 'class BoardColumnsService|constructor\(|InjectRepository|Repository<' apps/backend/src/board-columns/board-columns.service.ts
rg -n -C2 'createTestingModule|providers:|BoardColumnsService|useValue' apps/backend/src/board-columns/board-columns.controller.spec.tsRepository: Zafar7645/syncup
Length of output: 1143
Mock BoardColumnsService in this controller unit test.
BoardColumnsService has constructor dependencies on two TypeORM repositories (BoardColumn and Project). Registering the concrete service as a provider will cause module compilation to fail due to missing repository providers. Use a useValue mock to keep this unit test isolated from the database layer.
🔧 Proposed fix
beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
controllers: [BoardColumnsController],
- providers: [BoardColumnsService],
+ providers: [
+ {
+ provide: BoardColumnsService,
+ useValue: {
+ create: jest.fn(),
+ findAll: jest.fn(),
+ findOne: jest.fn(),
+ update: jest.fn(),
+ remove: jest.fn(),
+ },
+ },
+ ],
}).compile();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const module: TestingModule = await Test.createTestingModule({ | |
| controllers: [BoardColumnsController], | |
| providers: [BoardColumnsService], | |
| }).compile(); | |
| const module: TestingModule = await Test.createTestingModule({ | |
| controllers: [BoardColumnsController], | |
| providers: [ | |
| { | |
| provide: BoardColumnsService, | |
| useValue: { | |
| create: jest.fn(), | |
| findAll: jest.fn(), | |
| findOne: jest.fn(), | |
| update: jest.fn(), | |
| remove: jest.fn(), | |
| }, | |
| }, | |
| ], | |
| }).compile(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/board-columns/board-columns.controller.spec.ts` around lines
9 - 12, The test registers the concrete BoardColumnsService which depends on
TypeORM repositories and causes module compilation to fail; instead, in the
Test.createTestingModule call provide a mocked service using { provide:
BoardColumnsService, useValue: mockBoardColumnsService } (replace the providers:
[BoardColumnsService] entry), where mockBoardColumnsService is an object with
jest.fn() stubs for the methods the BoardColumnsController tests exercise (e.g.,
the controller's create/find/update/delete handlers); this keeps the test
isolated from repositories and allows the module to compile when instantiating
BoardColumnsController.
Owner
Author
There was a problem hiding this comment.
All tests will be handled in a separate issue for tests.
There was a problem hiding this comment.
@Zafar7645, understood! Would you like me to open a follow-up GitHub issue to track the test improvements (mocking BoardColumnsService and TasksService in their respective controller specs, and any other test coverage work) so it doesn't get lost?
🧠 Learnings used
Learnt from: Zafar7645
Repo: Zafar7645/syncup PR: 12
File: apps/backend/src/users/users.service.spec.ts:15-17
Timestamp: 2025-10-19T16:44:42.665Z
Learning: In the syncup repository (apps/backend/src/users/users.service.ts), the UsersService is scaffolding for future implementation and currently contains no methods or business logic. Basic "should be defined" smoke tests are sufficient for empty scaffolding services.
Learnt from: Zafar7645
Repo: Zafar7645/syncup PR: 16
File: apps/backend/src/auth/auth.service.spec.ts:6-6
Timestamp: 2026-02-07T16:08:28.564Z
Learning: In the syncup monorepo, use bcryptjs instead of native bcrypt in the backend (apps/backend) because Webpack bundling with shared libs breaks with node-gyp. Import from 'bcryptjs' where password hashing is used and ensure existing bcrypt usage is replaced accordingly. Expect roughly a 30% performance trade-off; if performance becomes critical, profile and consider alternatives, but default to bcryptjs to avoid bundling issues.
Comment on lines
+43
to
+46
| @Get(':id') | ||
| findOne(@Param('id', ParseIntPipe) id: number) { | ||
| return this.boardColumnsService.findOne(id); | ||
| } |
There was a problem hiding this comment.
Add user context to findOne for ownership checks.
This handler should pass req.user.userId to the service, otherwise per-user authorization cannot be enforced consistently on single-column reads.
Suggested change
`@Get`(':id')
- findOne(`@Param`('id', ParseIntPipe) id: number) {
- return this.boardColumnsService.findOne(id);
+ findOne(
+ `@Param`('id', ParseIntPipe) id: number,
+ `@Request`() req: { user: { userId: number; email: string } },
+ ) {
+ return this.boardColumnsService.findOne(id, req.user.userId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/board-columns/board-columns.controller.ts` around lines 43 -
46, The findOne controller method currently calls
boardColumnsService.findOne(id) without user context so ownership checks can't
run; update the BoardColumnsController.findOne to accept the request user (e.g.,
add `@Req`() req: Request or `@User`() user param) and pass req.user.userId into the
service call (change to boardColumnsService.findOne(id, userId)); ensure the
service method signature (boardColumnsService.findOne) is updated to accept and
enforce the userId for authorization if not already implemented.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check findOne in controller
rg -n "findOne" apps/backend/src/board-columns/board-columns.controller.ts -A5 -B2
# Check findOne in service
rg -n "findOne" apps/backend/src/board-columns/board-columns.service.ts -A10 -B2
Length of output: 1996
@Zafar7645, it looks like the fix hasn't landed in the branch yet. The controller's findOne on Line 44 still lacks the @Request() parameter and still calls boardColumnsService.findOne(id) without userId. Likewise, the service's findOne on Line 77 is still a stub with no ownership check:
// board-columns.controller.ts — still unfixed
findOne(`@Param`('id', ParseIntPipe) id: number) {
return this.boardColumnsService.findOne(id);
}
// board-columns.service.ts — still a stub
findOne(id: number) {
return `This action returns a #${id} boardColumn`;
}Here's what both files need:
Controller (board-columns.controller.ts):
`@Get`(':id')
findOne(
`@Param`('id', ParseIntPipe) id: number,
`@Request`() req: { user: { userId: number; email: string } },
) {
return this.boardColumnsService.findOne(id, req.user.userId);
}Service (board-columns.service.ts):
async findOne(id: number, userId: number): Promise<BoardColumn> {
return this.verifyColumnAccess(id, userId);
}verifyColumnAccess already performs the existence and ownership checks, so the service implementation is straightforward. Could you push the changes?
🧠 Learnings used
Learnt from: Zafar7645
Repo: Zafar7645/syncup PR: 16
File: apps/backend/src/auth/auth.service.spec.ts:6-6
Timestamp: 2026-02-07T16:08:28.564Z
Learning: In the syncup monorepo, use bcryptjs instead of native bcrypt in the backend (apps/backend) because Webpack bundling with shared libs breaks with node-gyp. Import from 'bcryptjs' where password hashing is used and ensure existing bcrypt usage is replaced accordingly. Expect roughly a 30% performance trade-off; if performance becomes critical, profile and consider alternatives, but default to bcryptjs to avoid bundling issues.
Comment on lines
+7
to
+12
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [BoardColumnsService], | ||
| }).compile(); | ||
|
|
||
| service = module.get<BoardColumnsService>(BoardColumnsService); |
There was a problem hiding this comment.
Test will fail: Missing repository mocks for injected dependencies.
BoardColumnsService requires @InjectRepository(BoardColumn) and @InjectRepository(Project) (see board-columns.service.ts:15-19). Without providing mocks, module.compile() will throw a dependency resolution error.
🐛 Proposed fix to add repository mocks
import { Test, TestingModule } from '@nestjs/testing';
import { BoardColumnsService } from '@/board-columns/board-columns.service';
+import { getRepositoryToken } from '@nestjs/typeorm';
+import { BoardColumn } from '@/board-columns/entities/board-column.entity';
+import { Project } from '@/projects/entities/project.entity';
describe('BoardColumnsService', () => {
let service: BoardColumnsService;
beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
- providers: [BoardColumnsService],
+ providers: [
+ BoardColumnsService,
+ {
+ provide: getRepositoryToken(BoardColumn),
+ useValue: {
+ findOne: jest.fn(),
+ find: jest.fn(),
+ create: jest.fn(),
+ save: jest.fn(),
+ merge: jest.fn(),
+ remove: jest.fn(),
+ },
+ },
+ {
+ provide: getRepositoryToken(Project),
+ useValue: {
+ findOne: jest.fn(),
+ },
+ },
+ ],
}).compile();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(async () => { | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [BoardColumnsService], | |
| }).compile(); | |
| service = module.get<BoardColumnsService>(BoardColumnsService); | |
| import { Test, TestingModule } from '@nestjs/testing'; | |
| import { BoardColumnsService } from '@/board-columns/board-columns.service'; | |
| import { getRepositoryToken } from '@nestjs/typeorm'; | |
| import { BoardColumn } from '@/board-columns/entities/board-column.entity'; | |
| import { Project } from '@/projects/entities/project.entity'; | |
| describe('BoardColumnsService', () => { | |
| let service: BoardColumnsService; | |
| beforeEach(async () => { | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [ | |
| BoardColumnsService, | |
| { | |
| provide: getRepositoryToken(BoardColumn), | |
| useValue: { | |
| findOne: jest.fn(), | |
| find: jest.fn(), | |
| create: jest.fn(), | |
| save: jest.fn(), | |
| merge: jest.fn(), | |
| remove: jest.fn(), | |
| }, | |
| }, | |
| { | |
| provide: getRepositoryToken(Project), | |
| useValue: { | |
| findOne: jest.fn(), | |
| }, | |
| }, | |
| ], | |
| }).compile(); | |
| service = module.get<BoardColumnsService>(BoardColumnsService); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/board-columns/board-columns.service.spec.ts` around lines 7
- 12, The test fails because BoardColumnsService depends on two injected
repositories (BoardColumn and Project); update the Test.createTestingModule
setup to provide mocks for those repositories (use the Nest testing pattern with
getRepositoryToken(BoardColumn) and getRepositoryToken(Project) or equivalent
providers) so dependency injection can resolve them, then retrieve
BoardColumnsService as before; ensure the mock providers are included alongside
BoardColumnsService in the module's providers array to prevent compile-time
resolution errors.
Owner
Author
There was a problem hiding this comment.
All tests will be handled in a separate issue for tests.
There was a problem hiding this comment.
@Zafar7645, understood! Would you like me to open a follow-up GitHub issue to track the test fixes (adding proper repository mocks for BoardColumnsService, TasksService, and related spec files) so it doesn't get lost?
🧠 Learnings used
Learnt from: Zafar7645
Repo: Zafar7645/syncup PR: 12
File: apps/backend/src/users/users.service.spec.ts:15-17
Timestamp: 2025-10-19T16:44:42.665Z
Learning: In the syncup repository (apps/backend/src/users/users.service.ts), the UsersService is scaffolding for future implementation and currently contains no methods or business logic. Basic "should be defined" smoke tests are sufficient for empty scaffolding services.
Learnt from: Zafar7645
Repo: Zafar7645/syncup PR: 16
File: apps/backend/src/auth/auth.service.spec.ts:6-6
Timestamp: 2026-02-07T16:08:28.564Z
Learning: In the syncup monorepo, use bcryptjs instead of native bcrypt in the backend (apps/backend) because Webpack bundling with shared libs breaks with node-gyp. Import from 'bcryptjs' where password hashing is used and ensure existing bcrypt usage is replaced accordingly. Expect roughly a 30% performance trade-off; if performance becomes critical, profile and consider alternatives, but default to bcryptjs to avoid bundling issues.
Comment on lines
+56
to
+63
| let order = createDto.order; | ||
| if (order === undefined) { | ||
| const lastColumn = await this.columnsRepository.findOne({ | ||
| where: { projectId: createDto.projectId }, | ||
| order: { order: 'DESC' }, | ||
| }); | ||
| order = lastColumn ? lastColumn.order + 1 : 0; | ||
| } |
There was a problem hiding this comment.
Order assignment is race-prone under concurrent creates.
Two concurrent requests can read the same last order and insert duplicate positions. Consider a transaction + lock (or a unique (project_id, order) constraint with retry) to keep ordering deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/board-columns/board-columns.service.ts` around lines 56 -
63, The current order assignment in the create flow (reading createDto.order and
using columnsRepository.findOne to fetch the lastColumn) is race-prone under
concurrent creates; fix by making order allocation atomic — either wrap the
create logic in a DB transaction and acquire a lock on the project's columns
(e.g., select the project's columns with FOR UPDATE before computing
lastColumn.order and inserting) in the service method that handles creation (the
method that reads createDto and calls columnsRepository.findOne), or add a
unique constraint on (project_id, order) and implement a small retry loop that
catches unique-constraint violations and recomputes the next order before
retrying the insert; update the board-columns service create flow (where
createDto and columnsRepository are used) to use one of these approaches.
Comment on lines
+77
to
+79
| findOne(id: number) { | ||
| return `This action returns a #${id} boardColumn`; | ||
| } |
There was a problem hiding this comment.
findOne is still a scaffold stub and breaks endpoint correctness.
This currently returns a string instead of the actual column and does not enforce ownership. It should use the same access check path as update/delete.
Suggested change
- findOne(id: number) {
- return `This action returns a #${id} boardColumn`;
+ async findOne(id: number, userId: number) {
+ return await this.verifyColumnAccess(id, userId);
}Downstream impact: apps/backend/src/board-columns/board-columns.controller.ts findOne must pass req.user.userId.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/board-columns/board-columns.service.ts` around lines 77 -
79, The findOne method in BoardColumnsService is still a scaffold that returns a
string; replace it with the real lookup that fetches the BoardColumn by id,
enforces ownership using the same access-check path as update/delete (reuse the
existing check/permission logic used in update() and remove()), and
throw/not-found or forbidden errors as appropriate; ensure it returns the actual
entity (or DTO) instead of a string and update the controller call to pass
req.user.userId into findOne so ownership can be validated.
Comment on lines
+8
to
+14
| @IsNumber() | ||
| @IsOptional() | ||
| order?: number; | ||
|
|
||
| @IsNumber() | ||
| @IsNotEmpty() | ||
| projectId: number; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current validators and integer schema expectations
rg -n -C2 'class CreateBoardColumnDto|@IsNumber|@IsInt|order|projectId' apps/backend/src/board-columns/dto/create-board-column.dto.ts
rg -n -C2 'CREATE TABLE board_columns|project_id|\"order\"| INT' database/init.sqlRepository: Zafar7645/syncup
Length of output: 533
🏁 Script executed:
#!/bin/bash
# Search for the board_columns table schema definition
rg -n 'CREATE TABLE board_columns' database/init.sql -A 20Repository: Zafar7645/syncup
Length of output: 799
🏁 Script executed:
#!/bin/bash
# Check for global validation pipes or transformers
rg -n 'ValidationPipe|ClassSerializerInterceptor|TransformOptions|transform:' apps/backend/src/main.ts apps/backend/src/app.module.ts --max-count=20
# Also check if there's a common DTO base class or validation config
fd -t f name '*validation*' 'apps/backend/src/' | head -20Repository: Zafar7645/syncup
Length of output: 315
🏁 Script executed:
#!/bin/bash
# Read the full ValidationPipe configuration
cat -n apps/backend/src/main.ts | head -20
# Check if CreateBoardColumnDto uses any `@Type` or `@Transform` decorators
rg -n '@Type|@Transform' apps/backend/src/board-columns/dto/create-board-column.dto.tsRepository: Zafar7645/syncup
Length of output: 818
🏁 Script executed:
#!/bin/bash
# Check if `@Min` constraints make sense
# order: 0 is typical starting value for ordering; projectId: 1 is typical for auto-increment IDs
# Search for other DTOs with similar number validation patterns
rg -n '@IsNumber\(\)' apps/backend/src --type ts | head -30Repository: Zafar7645/syncup
Length of output: 342
Replace @IsNumber() with @IsInt() to enforce integer validation for ID and order fields.
@IsNumber() accepts decimal values (e.g., 3.14), but the database schema requires INT. This mismatch causes validation to pass but persistence to fail. Use @IsInt() with appropriate @Min() bounds:
🔧 Proposed fix
-import { IsNotEmpty, IsNumber, IsString, IsOptional } from 'class-validator';
+import { IsInt, IsNotEmpty, IsOptional, IsString, Min } from 'class-validator';
@@
- `@IsNumber`()
+ `@IsInt`()
+ `@Min`(0)
`@IsOptional`()
order?: number;
@@
- `@IsNumber`()
+ `@IsInt`()
+ `@Min`(1)
`@IsNotEmpty`()
projectId: number;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/board-columns/dto/create-board-column.dto.ts` around lines 8
- 14, Replace the loose numeric validators on the DTO with integer-specific
validators: change the decorators on the order and projectId properties in
CreateBoardColumnDto from `@IsNumber`() to `@IsInt`(), import `@IsInt` and `@Min` from
class-validator, add `@Min`(0) on order (since it’s optional) and `@Min`(1) on
projectId to enforce positive IDs, and update the imports to remove `@IsNumber`
and include `@IsInt` and `@Min` so runtime validation matches the INT DB schema.
Comment on lines
+20
to
+21
| @Column({ nullable: true }) | ||
| description: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nullable mapping and TS strict null configuration
rg -n -C2 'description|nullable: true' apps/backend/src/tasks/entities/task.entity.ts
fd 'tsconfig.*json$' -t f | xargs -r rg -n -C1 '"strictNullChecks"|"strict"'Repository: Zafar7645/syncup
Length of output: 521
Align TypeScript type with nullable column.
The description property is marked nullable: true but typed as string. With strictNullChecks enabled in the backend's tsconfig, this creates a type-safety mismatch. The property type should be string | null to reflect the database schema.
🔧 Proposed fix
`@Column`({ nullable: true })
- description: string;
+ description: string | null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Column({ nullable: true }) | |
| description: string; | |
| `@Column`({ nullable: true }) | |
| description: string | null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/tasks/entities/task.entity.ts` around lines 20 - 21, The
Task entity's description column is declared nullable but the property is typed
as string; change the property type for description to string | null to match
the Column({ nullable: true }) decorator and strictNullChecks. Locate the
description property in the Task entity (the `@Column`({ nullable: true })
description declaration) and update its TypeScript type to string | null; ensure
any usages handle possible null values or add appropriate null-checks where
used.
Comment on lines
+9
to
+12
| const module: TestingModule = await Test.createTestingModule({ | ||
| controllers: [TasksController], | ||
| providers: [TasksService], | ||
| }).compile(); |
There was a problem hiding this comment.
Test will fail: TasksService dependencies are not satisfied.
Providing TasksService directly requires its repository dependencies (Task, BoardColumn). For controller unit tests, mock the service instead.
🐛 Proposed fix to mock TasksService
import { Test, TestingModule } from '@nestjs/testing';
import { TasksController } from '@/tasks/tasks.controller';
import { TasksService } from '@/tasks/tasks.service';
describe('TasksController', () => {
let controller: TasksController;
beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
controllers: [TasksController],
- providers: [TasksService],
+ providers: [
+ {
+ provide: TasksService,
+ useValue: {
+ create: jest.fn(),
+ findAll: jest.fn(),
+ findOne: jest.fn(),
+ update: jest.fn(),
+ remove: jest.fn(),
+ },
+ },
+ ],
}).compile();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const module: TestingModule = await Test.createTestingModule({ | |
| controllers: [TasksController], | |
| providers: [TasksService], | |
| }).compile(); | |
| const module: TestingModule = await Test.createTestingModule({ | |
| controllers: [TasksController], | |
| providers: [ | |
| { | |
| provide: TasksService, | |
| useValue: { | |
| create: jest.fn(), | |
| findAll: jest.fn(), | |
| findOne: jest.fn(), | |
| update: jest.fn(), | |
| remove: jest.fn(), | |
| }, | |
| }, | |
| ], | |
| }).compile(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/tasks/tasks.controller.spec.ts` around lines 9 - 12, The
test currently instantiates the real TasksService which requires repository
dependencies; change the Test.createTestingModule setup to provide a mocked
TasksService instead of the real provider: in the module config replace
providers: [TasksService] with a provider object for TasksService using useValue
(or useClass) that supplies mocked method implementations used by
TasksController tests (e.g., mock methods like create, findAll, etc.), then
retrieve the controller from the compiled TestingModule as before so controller
unit tests run without injecting Task/BoardColumn repositories.
Comment on lines
+31
to
+39
| @Get() | ||
| findAll() { | ||
| return this.tasksService.findAll(); | ||
| } | ||
|
|
||
| @Get(':id') | ||
| findOne(@Param('id', ParseIntPipe) id: number) { | ||
| return this.tasksService.findOne(id); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SERVICE_FILE="$(fd -t f 'tasks.service.ts$' apps/backend/src | head -n1)"
echo "Inspecting: ${SERVICE_FILE}"
# Verify method signatures and nearby logic
rg -n -C3 '\bfindAll\s*\(|\bfindOne\s*\(|\bverify\w+Access\s*\(|\buserId\b|\bprojectId\b' "$SERVICE_FILE"Repository: Zafar7645/syncup
Length of output: 2464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full controller file
CONTROLLER_FILE="apps/backend/src/tasks/tasks.controller.ts"
echo "=== Full controller file ==="
cat -n "$CONTROLLER_FILE"
echo -e "\n=== Check for guards or decorators ==="
rg -n '@UseGuard|@Request\(\)|@Param.*userId|extractUser|getCurrentUser' "$CONTROLLER_FILE"
echo -e "\n=== Check decorator usage at method level ==="
rg -n -B2 'findAll|findOne' "$CONTROLLER_FILE" | head -30Repository: Zafar7645/syncup
Length of output: 2374
Add user scoping to read endpoints to prevent data leakage.
findAll and findOne do not extract or pass user context, unlike create, update, and remove. If these endpoints are implemented without user scoping, they will expose all tasks across all projects instead of only the authenticated user's tasks. Update both the controller methods to extract userId via @Request() and the service methods to accept and validate user ownership (following the same pattern as verifyTaskAccess used in update and remove).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/tasks/tasks.controller.ts` around lines 31 - 39, The findAll
and findOne controller methods currently don't scope queries to the
authenticated user; update findAll and findOne to accept the request object via
`@Request`() (extract userId from req.user) and pass userId into
tasksService.findAll(...) and tasksService.findOne(...), and then update the
corresponding service methods (tasksService.findAll and tasksService.findOne) to
require a userId parameter and enforce ownership/validation (reuse the same
pattern as verifyTaskAccess used by update/remove to check project/task
ownership and throw if not authorized) so controllers only return tasks for the
authenticated user.
Comment on lines
+7
to
+12
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [TasksService], | ||
| }).compile(); | ||
|
|
||
| service = module.get<TasksService>(TasksService); |
There was a problem hiding this comment.
Test will fail: Missing repository mocks for injected dependencies.
TasksService requires @InjectRepository(Task) and @InjectRepository(BoardColumn) (see tasks.service.ts:15-18). Without providing mocks, module.compile() will throw a dependency resolution error.
🐛 Proposed fix to add repository mocks
import { Test, TestingModule } from '@nestjs/testing';
import { TasksService } from '@/tasks/tasks.service';
+import { getRepositoryToken } from '@nestjs/typeorm';
+import { Task } from '@/tasks/entities/task.entity';
+import { BoardColumn } from '@/board-columns/entities/board-column.entity';
describe('TasksService', () => {
let service: TasksService;
beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
- providers: [TasksService],
+ providers: [
+ TasksService,
+ {
+ provide: getRepositoryToken(Task),
+ useValue: {
+ findOne: jest.fn(),
+ create: jest.fn(),
+ save: jest.fn(),
+ merge: jest.fn(),
+ remove: jest.fn(),
+ },
+ },
+ {
+ provide: getRepositoryToken(BoardColumn),
+ useValue: {
+ findOne: jest.fn(),
+ },
+ },
+ ],
}).compile();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(async () => { | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [TasksService], | |
| }).compile(); | |
| service = module.get<TasksService>(TasksService); | |
| import { Test, TestingModule } from '@nestjs/testing'; | |
| import { TasksService } from '@/tasks/tasks.service'; | |
| import { getRepositoryToken } from '@nestjs/typeorm'; | |
| import { Task } from '@/tasks/entities/task.entity'; | |
| import { BoardColumn } from '@/board-columns/entities/board-column.entity'; | |
| describe('TasksService', () => { | |
| let service: TasksService; | |
| beforeEach(async () => { | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [ | |
| TasksService, | |
| { | |
| provide: getRepositoryToken(Task), | |
| useValue: { | |
| findOne: jest.fn(), | |
| create: jest.fn(), | |
| save: jest.fn(), | |
| merge: jest.fn(), | |
| remove: jest.fn(), | |
| }, | |
| }, | |
| { | |
| provide: getRepositoryToken(BoardColumn), | |
| useValue: { | |
| findOne: jest.fn(), | |
| }, | |
| }, | |
| ], | |
| }).compile(); | |
| service = module.get<TasksService>(TasksService); | |
| }); | |
| it('should be defined', () => { | |
| expect(service).toBeDefined(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/tasks/tasks.service.spec.ts` around lines 7 - 12, The test
fails because TasksService depends on two injected repositories (the
`@InjectRepository`(Task) and `@InjectRepository`(BoardColumn) injections used by
TasksService) which are not provided in the Test.createTestingModule providers;
add provider mocks for those repositories using getRepositoryToken(Task) and
getRepositoryToken(BoardColumn) and supply simple mock objects (e.g., objects
with jest.fn() stubs for methods used by TasksService) when building the testing
module before calling module.compile(); update the providers array to include {
provide: getRepositoryToken(Task), useValue: taskRepoMock } and { provide:
getRepositoryToken(BoardColumn), useValue: boardColumnRepoMock } so
module.get<TasksService>(TasksService) can resolve successfully.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8 and #21
Summary by CodeRabbit