Bug fixes - #21
Bug fixes#21
Conversation
WalkthroughThis PR introduces schema migrations (Project.servers JSON field), refactors DTOs to use static factory methods, adds comprehensive Swagger/error response type documentation across controllers, implements optional password updates for admin users, enhances endpoint status tracking with linting integration, and extends authentication to include password flags in JWT claims. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![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 (16)
.spectral.yaml (1)
59-59: Address the unresolved FIX comment.Line 59 contains a comment "# FIX: Use a more precise 'given' and allow for '$ref'" that suggests incomplete refinement of the
success-response-content-definedrule. The current JSONPath expression excludes 204 and 205 responses but the comment implies further improvements may be needed.Is this a known limitation, or should the rule logic be refined as part of this PR?
src/common/dto/message-response.dto.ts (1)
3-5: Simple success-response DTO is fine
MessageResponseDtois straightforward and suitable for standard “message-only” responses. Consider markingmessageasreadonlyor usingmessage!: string;if you enforce strict property initialization elsewhere, but that’s optional.src/projects/spec-reconciliation/spec-reconciliation.service.ts (1)
106-116: Helper for metadata‑stripped comparison is sound, small type/refactor nitsThe
getComparableOperationguard against non-object / array values is safe and avoids runtime errors. Two minor, optional refinements you might consider later:
- Narrow the return type from
objectto something likeRecord<string, unknown>for clearer typing.- If equality sensitivity ever becomes an issue, swap
JSON.stringifyfor a stable deep-equality helper to avoid key-order concerns (today this is just a perf/robustness nicety).prisma/seed.ts (1)
140-145: Servers seed shape matches new JSON array modelUsing
serversas an array of{ url, description }objects for Nova and Apollo aligns with the new schema and should seed correctly. If this pattern grows, you might later extract shared server definitions/constants, but it’s not necessary now.Also applies to: 356-361
src/teams/dto/team.dto.ts (1)
4-4: Static factory forTeamDtois a clean decoupling from Prisma model
TeamDto.from(team)correctly mapsid,name,createdAt, andupdatedAt, and droppingimplements Teamreduces coupling to the Prisma type. As a small optional enhancement, you could make the constructorprivateto enforce factory usage across the codebase.Also applies to: 17-23
src/projects/dto/update-access.dto.ts (1)
12-62: Nested ACL validation DTOs are structured correctly, with minor optional tighteningThe nested DTOs (
AccessUserValidationDto,AccessTeamValidationDto,ProjectAccessControlListInputDto) are wired appropriately for Swagger and class-validator (ids required, arrays +ValidateNested+Typeset up correctly).Two optional improvements to consider:
- For
profileImage,createdAt, andupdatedAt, if you care about format, you may want to add validators (e.g.,@IsString()and/or@IsUrl()/@IsDateString()), relying on@IsOptional()to skip validation when absent/null.- If these classes are ever reused outside this file, promoting them to
export classmight make future refactors easier.src/auth/dto/login-response.dto.ts (1)
3-9: LoginResponseDto is correct; consider simplifying the example JWTThe DTO shape (
access_token: stringwith Swagger metadata) is appropriate for the login response. Static analysis tools may keep flagging the long example JWT as a potential secret even though it’s clearly non-sensitive.To reduce noise, consider replacing the example with a shorter obviously-placeholder value like
"eyJhbGciOi...<snip>...signature"or"JWT_TOKEN_HERE".src/projects/spec-builder/openapi-spec.builder.ts (1)
43-46: LGTM! Defensive array handling for schema migration.The updated logic properly handles the migration from
serverUrltoserversarray with appropriate defensive checks. The fallback to an empty array is correct per OpenAPI 3.0 specification.Consider adding runtime validation for server object structure to prevent invalid OpenAPI specs:
servers: project.servers && Array.isArray(project.servers) && project.servers.length > 0 ? project.servers.filter((s: any) => s && typeof s === 'object' && s.url) : [],This would ensure each server object has at minimum a
urlproperty, as required by OpenAPI 3.0 specification.src/projects/endpoints/dto/openapi-operation.dto.ts (1)
1-10: Optional property vsApiPropertydefault “required” flag
summaryis optional in TypeScript (summary?: string), butApiPropertymarks it as required in the OpenAPI schema by default. Even though you overwrite this schema inswagger.ts, you could avoid minor confusion by marking it optional:-import { ApiProperty } from '@nestjs/swagger'; +import { ApiPropertyOptional } from '@nestjs/swagger'; - @ApiProperty() - summary?: string; + @ApiPropertyOptional() + summary?: string;(or
@ApiProperty({ required: false })if you prefer to keepApiProperty).src/projects/dto/project-access-details.dto.ts (1)
1-37: Access-details DTO structure looks good; consider a small Swagger consistency tweakThe DTO shapes for owners/viewers/deniedUsers are clear and align well with
SanitizedUserDtoandTeamDto. As an optional consistency improvement with other DTOs, you might switch to theisArraypattern used elsewhere:- @ApiProperty({ - type: [SanitizedUserDto], - description: 'List of users granted this access level.', - }) + @ApiProperty({ + type: () => SanitizedUserDto, + isArray: true, + description: 'List of users granted this access level.', + })(and similarly for
teams/deniedUsers), but the current form is functionally fine.src/common/pipes/image-file.pipe.ts (1)
1-28: Image validation pipe is solid; consider more specific error semanticsThe size and MIME-type checks are clean and reusable. Two optional refinements you might consider:
- Use a more specific HTTP error for oversized files:
-import { BadRequestException, Injectable, PipeTransform } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + PipeTransform, + PayloadTooLargeException, +} from '@nestjs/common'; - if (file.size > MAX_FILE_SIZE) { - throw new BadRequestException( + if (file.size > MAX_FILE_SIZE) { + throw new PayloadTooLargeException( `File size exceeds the limit of ${MAX_FILE_SIZE / 1024 / 1024}MB.`, );
- If some endpoints require an image file, you may want a variant that throws when
!fileinstead of silently returningundefined, to fail fast on missing uploads.Both are optional; the current implementation is functionally correct.
src/swagger.ts (1)
27-96: Document the rationale for inline schema definitions.The inline schema definitions for OpenAPI objects (ParameterObject, RequestBodyObject, ResponseObject, OpenApiOperationDto) are manually defined here rather than using DTOs with decorators. While this provides fine-grained control, it adds maintenance burden.
Consider adding a comment explaining why these schemas are defined inline rather than through standard DTO decorators, especially since they override any class-based definitions.
src/projects/dto/project-summary.dto.ts (1)
20-21: Optional: Remove redundant type specification.The
type: 'string'in the ApiProperty decorator is redundant since Swagger can automatically infer the type from the TypeScript type annotationstring | null.- @ApiProperty({ nullable: true, type: 'string' }) + @ApiProperty({ nullable: true }) description: string | null;src/projects/dto/project-server.dto.ts (1)
5-12: Type guard should validatedescriptiontype when present.The type guard validates
urlis a string but doesn't validate thatdescriptionis a string when present. This could allow non-string description values to slip through.function isValidServerObject(obj: unknown): obj is { url: string; description?: string } { return ( typeof obj === 'object' && obj !== null && 'url' in obj && - typeof (obj as Record<string, unknown>).url === 'string' + typeof (obj as Record<string, unknown>).url === 'string' && + (!('description' in obj) || + typeof (obj as Record<string, unknown>).description === 'string') ); }src/locking/locking.gateway.ts (1)
72-85: Consider joining the room before emitting initial state to prevent missed updates.There's a subtle race condition: if a lock update occurs between
getLockStatus()(line 75) andclient.join()(line 81), the client receives stale initial state and misses the broadcast. While this window is very small, joining the room first would eliminate it:@SubscribeMessage('subscribeToResource') async handleSubscribe(client: AuthenticatedSocket, resourceId: string): Promise<void> { if (typeof resourceId === 'string' && resourceId) { + await client.join(`resource:${resourceId}`); const currentLock = this.lockingService.getLockStatus(resourceId); client.emit('lock_updated', { resourceId: resourceId, lock: currentLock, }); - - await client.join(`resource:${resourceId}`); this.logger.info( `Client ${client.id} subscribed to resource ${resourceId} and received initial state.`, ); } }This ensures the client is part of the room before any state is read, so any concurrent updates are also received.
src/projects/endpoints/endpoints.service.ts (1)
277-283: Consider parallelizing the user fetches.The two
fetchUsercalls execute sequentially. Since they're independent, usingPromise.allwould reduce latency.if (currentStatus === newStatus) { + const [creator, updatedBy] = await Promise.all([ + this.fetchUser(endpoint.creatorId), + this.fetchUser(endpoint.updatedById), + ]); return new EndpointDto({ ...endpoint, - creator: await this.fetchUser(endpoint.creatorId), - updatedBy: await this.fetchUser(endpoint.updatedById), + creator, + updatedBy, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (76)
.gitignore(1 hunks).spectral.yaml(2 hunks)package.json(2 hunks)prisma/migrations/20251124153948_add_project_servers/migration.sql(1 hunks)prisma/schema.prisma(1 hunks)prisma/seed.ts(2 hunks)src/admin/teams/admin-teams.controller.ts(3 hunks)src/admin/teams/admin-teams.service.ts(2 hunks)src/admin/users/admin-users.controller.ts(5 hunks)src/admin/users/admin-users.service.ts(3 hunks)src/admin/users/dto/create-user.dto.ts(1 hunks)src/admin/users/dto/update-user.dto.ts(2 hunks)src/audit/audit.controller.ts(2 hunks)src/audit/audit.events.ts(1 hunks)src/audit/dto/audit-log.dto.ts(2 hunks)src/auth/auth.controller.ts(2 hunks)src/auth/dto/login-response.dto.ts(1 hunks)src/auth/dto/me.dto.ts(1 hunks)src/auth/dto/user.dto.ts(2 hunks)src/auth/strategies/jwt.strategy.ts(2 hunks)src/changelog/changelog.controller.ts(2 hunks)src/changelog/changelog.listener.ts(1 hunks)src/changelog/dto/changelog.dto.ts(1 hunks)src/common/adapters/socket-io.adapter.ts(2 hunks)src/common/decorators/api-paginated-response.decorator.ts(1 hunks)src/common/dto/error-response.dto.ts(1 hunks)src/common/dto/message-response.dto.ts(1 hunks)src/common/dto/paginated-response.dto.ts(1 hunks)src/common/files/files.service.ts(1 hunks)src/common/filters/all-exceptions.filter.ts(5 hunks)src/common/interfaces/api-response.interface.ts(1 hunks)src/common/pipes/image-file.pipe.ts(1 hunks)src/locking/dto/lock.dto.ts(1 hunks)src/locking/locking.controller.ts(2 hunks)src/locking/locking.gateway.ts(5 hunks)src/locking/locking.service.ts(5 hunks)src/main.ts(2 hunks)src/mock-server/mock-server.controller.ts(2 hunks)src/notifications/dto/notification.dto.ts(1 hunks)src/notifications/notification.controller.ts(3 hunks)src/notifications/notification.service.ts(2 hunks)src/profile/profile.controller.ts(2 hunks)src/projects/dto/create-project.dto.ts(2 hunks)src/projects/dto/project-access-control-list.dto.ts(1 hunks)src/projects/dto/project-access-details.dto.ts(1 hunks)src/projects/dto/project-detail.dto.ts(4 hunks)src/projects/dto/project-server.dto.ts(1 hunks)src/projects/dto/project-summary.dto.ts(3 hunks)src/projects/dto/update-access.dto.ts(1 hunks)src/projects/dto/update-project.dto.ts(1 hunks)src/projects/endpoints/dto/create-endpoint.dto.ts(2 hunks)src/projects/endpoints/dto/endpoint.dto.ts(3 hunks)src/projects/endpoints/dto/openapi-operation.dto.ts(1 hunks)src/projects/endpoints/dto/update-endpoint-status.dto.ts(1 hunks)src/projects/endpoints/dto/update-endpoint.dto.ts(2 hunks)src/projects/endpoints/endpoints.controller.ts(7 hunks)src/projects/endpoints/endpoints.service.ts(7 hunks)src/projects/environments/dto/environment.dto.ts(1 hunks)src/projects/environments/environments.controller.ts(5 hunks)src/projects/notes/dto/note.dto.ts(1 hunks)src/projects/notes/notes.controller.ts(5 hunks)src/projects/projects.controller.ts(9 hunks)src/projects/projects.service.ts(12 hunks)src/projects/schema-components/dto/schema-component.dto.ts(1 hunks)src/projects/schema-components/schema-components.controller.ts(6 hunks)src/projects/secrets/dto/secret.dto.ts(1 hunks)src/projects/secrets/secrets.controller.ts(5 hunks)src/projects/spec-builder/openapi-spec.builder.ts(1 hunks)src/projects/spec-reconciliation/spec-reconciliation.service.ts(2 hunks)src/swagger.ts(2 hunks)src/teams/dto/team.dto.ts(2 hunks)src/teams/teams.controller.ts(2 hunks)src/teams/teams.service.ts(1 hunks)src/users/dto/sanitized-user.dto.ts(1 hunks)src/users/users.controller.ts(3 hunks)src/users/users.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (43)
src/auth/dto/login-response.dto.ts (1)
src/auth/dto/login.dto.ts (1)
LoginDto(4-14)
src/audit/audit.events.ts (2)
src/audit/dto/audit-log-query.dto.ts (1)
AuditLogQueryDto(6-24)src/audit/audit.listener.ts (1)
AuditListener(8-32)
src/projects/dto/project-access-details.dto.ts (2)
src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)src/teams/dto/team.dto.ts (1)
TeamDto(4-25)
src/auth/strategies/jwt.strategy.ts (1)
src/users/users.types.ts (1)
UserWithTeams(9-11)
src/swagger.ts (18)
src/projects/endpoints/dto/openapi-operation.dto.ts (1)
OpenApiOperationDto(6-10)src/audit/dto/audit-log.dto.ts (1)
AuditLogDto(11-44)src/auth/dto/user.dto.ts (1)
UserDto(6-40)src/changelog/dto/changelog.dto.ts (1)
ChangelogDto(11-30)src/notifications/dto/notification.dto.ts (1)
NotificationDto(19-52)src/projects/dto/project-access-control-list.dto.ts (1)
ProjectAccessControlListDto(4-24)src/projects/dto/project-access-details.dto.ts (2)
ProjectAccessDetailsResponseDto(19-37)ProjectAccessControlListResponseDto(5-17)src/projects/dto/project-detail.dto.ts (1)
ProjectDetailDto(32-99)src/projects/dto/project-link.dto.ts (1)
ProjectLinkDto(4-14)src/projects/dto/project-summary.dto.ts (1)
ProjectSummaryDto(13-48)src/projects/endpoints/dto/endpoint.dto.ts (1)
EndpointDto(13-55)src/projects/endpoints/dto/endpoint-summary.dto.ts (1)
EndpointSummaryDto(16-55)src/projects/environments/dto/environment.dto.ts (1)
EnvironmentDto(4-27)src/projects/notes/dto/note.dto.ts (1)
NoteDto(18-41)src/projects/schema-components/dto/schema-component.dto.ts (1)
SchemaComponentDto(5-32)src/projects/secrets/dto/secret.dto.ts (1)
SecretDto(4-31)src/teams/dto/team.dto.ts (1)
TeamDto(4-25)src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)
src/common/decorators/api-paginated-response.decorator.ts (1)
src/common/dto/paginated-response.dto.ts (1)
PaginationMetaDto(3-18)
src/users/dto/sanitized-user.dto.ts (1)
src/users/users.types.ts (1)
SanitizedUser(3-7)
src/mock-server/mock-server.controller.ts (1)
src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/auth/dto/me.dto.ts (2)
src/users/users.types.ts (1)
UserWithTeams(9-11)src/auth/dto/user.dto.ts (1)
UserDto(6-40)
src/admin/teams/admin-teams.controller.ts (1)
src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/projects/notes/notes.controller.ts (1)
src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/projects/notes/dto/note.dto.ts (1)
src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)
src/teams/teams.controller.ts (2)
src/common/decorators/api-paginated-response.decorator.ts (1)
ApiPaginatedResponse(12-30)src/teams/dto/team.dto.ts (1)
TeamDto(4-25)
src/common/dto/error-response.dto.ts (1)
src/common/interfaces/api-response.interface.ts (1)
RequestMeta(3-12)
src/users/users.service.ts (1)
src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)
src/admin/users/admin-users.service.ts (1)
src/audit/audit.events.ts (2)
AuditEvent(8-8)AuditLogEvent(57-62)
src/locking/locking.service.ts (1)
src/locking/dto/lock.dto.ts (1)
LockDto(3-18)
src/projects/environments/environments.controller.ts (1)
src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/projects/secrets/secrets.controller.ts (2)
src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)src/projects/secrets/dto/secret.dto.ts (1)
SecretDto(4-31)
src/profile/profile.controller.ts (2)
src/types/fastify.d.ts (1)
UploadedFile(7-13)src/common/interceptors/file.interceptor.ts (1)
UploadedFile(91-96)
src/users/users.controller.ts (4)
src/common/decorators/api-paginated-response.decorator.ts (1)
ApiPaginatedResponse(12-30)src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)src/common/dto/pagination-search-query.dto.ts (1)
PaginationSearchQueryDto(5-15)src/common/dto/message-response.dto.ts (1)
MessageResponseDto(3-6)
src/auth/dto/user.dto.ts (1)
src/teams/dto/team.dto.ts (1)
TeamDto(4-25)
src/projects/projects.controller.ts (3)
src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)src/common/decorators/api-paginated-response.decorator.ts (1)
ApiPaginatedResponse(12-30)src/projects/dto/project-summary.dto.ts (1)
ProjectSummaryDto(13-48)
src/notifications/dto/notification.dto.ts (1)
src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)
src/projects/endpoints/dto/update-endpoint.dto.ts (1)
src/projects/endpoints/dto/openapi-operation.dto.ts (1)
OpenApiOperationDto(6-10)
src/changelog/dto/changelog.dto.ts (1)
src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)
src/projects/schema-components/schema-components.controller.ts (1)
src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/projects/endpoints/dto/create-endpoint.dto.ts (1)
src/projects/endpoints/dto/openapi-operation.dto.ts (1)
OpenApiOperationDto(6-10)
src/projects/dto/project-summary.dto.ts (2)
src/projects/dto/project-server.dto.ts (1)
ProjectServerDto(14-47)src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)
src/projects/dto/update-project.dto.ts (1)
src/projects/dto/create-project.dto.ts (1)
CreateProjectDto(27-53)
src/admin/teams/admin-teams.service.ts (1)
src/teams/dto/team.dto.ts (1)
TeamDto(4-25)
src/changelog/changelog.controller.ts (3)
src/common/decorators/api-paginated-response.decorator.ts (1)
ApiPaginatedResponse(12-30)src/changelog/dto/changelog.dto.ts (1)
ChangelogDto(11-30)src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/projects/endpoints/endpoints.service.ts (2)
src/projects/endpoints/dto/endpoint.dto.ts (1)
EndpointDto(13-55)src/common/exceptions/spec-linting.exception.ts (1)
SpecLintingException(12-28)
src/admin/users/admin-users.controller.ts (7)
src/auth/dto/user.dto.ts (1)
UserDto(6-40)src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)src/types/fastify.d.ts (1)
UploadedFile(7-13)src/common/interceptors/file.interceptor.ts (1)
UploadedFile(91-96)src/common/decorators/api-paginated-response.decorator.ts (1)
ApiPaginatedResponse(12-30)src/common/dto/pagination-search-query.dto.ts (1)
PaginationSearchQueryDto(5-15)src/auth/decorators/current-user.decorator.ts (1)
CurrentUser(7-10)
src/projects/endpoints/endpoints.controller.ts (3)
src/common/decorators/api-paginated-response.decorator.ts (1)
ApiPaginatedResponse(12-30)src/projects/endpoints/dto/endpoint-summary.dto.ts (1)
EndpointSummaryDto(16-55)src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/common/filters/all-exceptions.filter.ts (2)
src/config/config.type.ts (1)
AllConfigTypes(8-13)src/types/fastify.d.ts (1)
FastifyRequest(16-24)
src/audit/dto/audit-log.dto.ts (1)
src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)
src/notifications/notification.controller.ts (6)
src/common/dto/message-response.dto.ts (1)
MessageResponseDto(3-6)src/auth/decorators/current-user.decorator.ts (1)
CurrentUser(7-10)src/auth/dto/user.dto.ts (1)
UserDto(6-40)src/common/decorators/api-paginated-response.decorator.ts (1)
ApiPaginatedResponse(12-30)src/notifications/dto/notification.dto.ts (1)
NotificationDto(19-52)src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/locking/locking.controller.ts (4)
src/locking/dto/lock.dto.ts (1)
LockDto(3-18)src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)src/auth/decorators/current-user.decorator.ts (1)
CurrentUser(7-10)src/auth/dto/user.dto.ts (1)
UserDto(6-40)
src/audit/audit.controller.ts (3)
src/common/decorators/api-paginated-response.decorator.ts (1)
ApiPaginatedResponse(12-30)src/audit/dto/audit-log.dto.ts (1)
AuditLogDto(11-44)src/common/dto/error-response.dto.ts (1)
ErrorResponseDto(24-39)
src/teams/teams.service.ts (1)
src/teams/dto/team.dto.ts (1)
TeamDto(4-25)
src/projects/endpoints/dto/endpoint.dto.ts (2)
src/projects/endpoints/dto/openapi-operation.dto.ts (1)
OpenApiOperationDto(6-10)src/users/dto/sanitized-user.dto.ts (1)
SanitizedUserDto(4-21)
src/auth/auth.controller.ts (6)
src/auth/decorators/current-user.decorator.ts (1)
CurrentUser(7-10)src/auth/dto/user.dto.ts (1)
UserDto(6-40)src/auth/dto/login.dto.ts (1)
LoginDto(4-14)src/auth/dto/login-response.dto.ts (1)
LoginResponseDto(3-10)src/auth/dto/me.dto.ts (1)
MeDto(7-18)src/users/users.types.ts (1)
UserWithTeams(9-11)
🪛 Gitleaks (8.29.1)
src/auth/dto/login-response.dto.ts
[high] 7-7: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Project
|
|
||
| @Post() | ||
| @UseInterceptors(FileInterceptor) | ||
| @UseInterceptors(FileInterceptor) |
There was a problem hiding this comment.
Duplicate @UseInterceptors(FileInterceptor) decorator.
The FileInterceptor is applied twice on the create method, which will cause the interceptor to execute twice per request. This could lead to unexpected behavior or performance issues.
Remove the duplicate decorator:
@Post()
@UseInterceptors(FileInterceptor)
- @UseInterceptors(FileInterceptor)
@ApiConsumes('multipart/form-data')📝 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.
| @UseInterceptors(FileInterceptor) | |
| @UseInterceptors(FileInterceptor) | |
| @Post() | |
| @UseInterceptors(FileInterceptor) | |
| @ApiConsumes('multipart/form-data') |
🤖 Prompt for AI Agents
In src/admin/users/admin-users.controller.ts around lines 53 to 54, the
FileInterceptor decorator is applied twice on the create method causing the
interceptor to run twice; remove the duplicate @UseInterceptors(FileInterceptor)
so only a single decorator remains (ensure the remaining one is correct and
imports are unchanged).
| // Serve index.html for non-API paths (e.g. /auth/callback, /projects/123) | ||
| if (this.nodeEnv !== 'test' && !url.startsWith(`/${this.globalPrefix}`)) { | ||
| const response = ctx.getResponse<FastifyReply>(); | ||
| const filePath = join(process.cwd(), 'public', 'index.html'); | ||
| const stream = createReadStream(filePath); | ||
|
|
||
| stream.on('error', () => { | ||
| this.handleApiError(exception, host); | ||
| }); | ||
|
|
||
| response.type('text/html').send(stream); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Stream error handler may not prevent partial response.
The stream.on('error') callback is registered after creating the stream but before error events fire. However, since response.send(stream) is called immediately, if the file exists but errors during streaming (e.g., permissions, disk I/O), headers may already be sent before handleApiError is invoked, leading to incomplete/corrupt responses.
Consider checking file existence before streaming or using a more robust file-serving approach:
+ const filePath = join(process.cwd(), 'public', 'index.html');
+ const stream = createReadStream(filePath);
+
+ stream.on('open', () => {
+ response.type('text/html').send(stream);
+ });
+
stream.on('error', () => {
this.handleApiError(exception, host);
});
-
- response.type('text/html').send(stream);
return;This ensures the stream is sendable before committing the response.
📝 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.
| // Serve index.html for non-API paths (e.g. /auth/callback, /projects/123) | |
| if (this.nodeEnv !== 'test' && !url.startsWith(`/${this.globalPrefix}`)) { | |
| const response = ctx.getResponse<FastifyReply>(); | |
| const filePath = join(process.cwd(), 'public', 'index.html'); | |
| const stream = createReadStream(filePath); | |
| stream.on('error', () => { | |
| this.handleApiError(exception, host); | |
| }); | |
| response.type('text/html').send(stream); | |
| return; | |
| } | |
| // Serve index.html for non-API paths (e.g. /auth/callback, /projects/123) | |
| if (this.nodeEnv !== 'test' && !url.startsWith(`/${this.globalPrefix}`)) { | |
| const response = ctx.getResponse<FastifyReply>(); | |
| const filePath = join(process.cwd(), 'public', 'index.html'); | |
| const stream = createReadStream(filePath); | |
| stream.on('open', () => { | |
| response.type('text/html').send(stream); | |
| }); | |
| stream.on('error', () => { | |
| this.handleApiError(exception, host); | |
| }); | |
| return; | |
| } |
| return { | ||
| ...existingLock, | ||
| expiresAt: new Date(existingLock.expiresAt).toISOString(), | ||
| }; |
There was a problem hiding this comment.
Inconsistent expiresAt format between REST and WebSocket responses.
The acquireLock method returns LockDto with expiresAt as an ISO string (e.g., "2025-11-06T12:00:00.000Z"), but broadcastLockUpdate (lines 61, 79) sends the raw Lock object where expiresAt is an epoch number. This creates an inconsistency for clients consuming both REST and WebSocket APIs.
Consider converting expiresAt to ISO format before broadcasting, or create a helper method to standardize the conversion:
+ private toLockDto(lock: Lock): LockDto {
+ return {
+ userId: lock.userId,
+ username: lock.username,
+ expiresAt: new Date(lock.expiresAt).toISOString(),
+ };
+ }
+
acquireLock(resourceId: string, user: UserDto): LockDto {
// ... existing logic ...
this.gateway.broadcastLockUpdate(resourceId, existingLock);
- return {
- ...existingLock,
- expiresAt: new Date(existingLock.expiresAt).toISOString(),
- };
+ return this.toLockDto(existingLock);
}Then update broadcastLockUpdate signature to accept LockDto | null and convert before calling.
Also applies to: 81-84
🤖 Prompt for AI Agents
In src/locking/locking.service.ts around lines 61-65 (and also affecting 79 and
81-84), the REST response converts existingLock.expiresAt to an ISO string but
broadcastLockUpdate is sending the raw Lock with an epoch number, causing
inconsistent expiresAt formats between REST and WebSocket; fix by creating a
small helper that maps a Lock to LockDto (converting expiresAt to new
Date(lock.expiresAt).toISOString()), update acquireLock to use that helper,
change broadcastLockUpdate signature to accept LockDto | null (or call the
helper before invoking it) and ensure every call to broadcastLockUpdate passes
the standardized LockDto so both REST and WebSocket clients receive
ISO-formatted expiresAt.
| path: '/', | ||
| httpOnly: true, | ||
| secure: isProduction, | ||
| sameSite: 'none', | ||
| maxAge: 7 * 24 * 60 * 60, | ||
| }, |
There was a problem hiding this comment.
Potential cookie configuration issue in non-production environments.
Setting sameSite: 'none' without secure: true will cause modern browsers to reject the cookie. Since secure is only true when isProduction is true (line 114), this configuration will fail in development and staging environments.
Consider one of these approaches:
Option 1: Enforce secure with sameSite none
cookie: {
path: '/',
httpOnly: true,
- secure: isProduction,
+ secure: isProduction || true, // sameSite: 'none' requires secure: true
sameSite: 'none',
maxAge: 7 * 24 * 60 * 60,
},Option 2: Make sameSite conditional
cookie: {
path: '/',
httpOnly: true,
secure: isProduction,
- sameSite: 'none',
+ sameSite: isProduction ? 'none' : 'lax',
maxAge: 7 * 24 * 60 * 60,
},Option 3: Always use secure (if localhost dev uses HTTPS)
cookie: {
path: '/',
httpOnly: true,
- secure: isProduction,
+ secure: true,
sameSite: 'none',
maxAge: 7 * 24 * 60 * 60,
},📝 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.
| path: '/', | |
| httpOnly: true, | |
| secure: isProduction, | |
| sameSite: 'none', | |
| maxAge: 7 * 24 * 60 * 60, | |
| }, | |
| path: '/', | |
| httpOnly: true, | |
| secure: isProduction, | |
| sameSite: isProduction ? 'none' : 'lax', | |
| maxAge: 7 * 24 * 60 * 60, | |
| }, |
🤖 Prompt for AI Agents
In src/main.ts around lines 112 to 117, the cookie is configured with sameSite:
'none' while secure is only true in production, which will cause browsers to
reject the cookie in non-HTTPS environments; change the configuration so
sameSite and secure are consistent — either make sameSite conditional (e.g., use
'none' only when isProduction, otherwise 'lax' or 'strict'), or ensure secure is
true whenever sameSite is 'none' (for example enable secure for localhost dev
over HTTPS or add an explicit isLocalhost flag). Update the cookie options to
derive sameSite and secure from environment flags so they remain compatible.
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.