first commit#199
first commit#199manzano-bjf wants to merge 1 commit intoStarShopCr:mainfrom manzano-bjf:feat/any_breaking_guarantess
Conversation
WalkthroughThe PR removes escrow/store features, Redis, and various role-specific/user extra fields. It migrates IDs from UUID/string to numeric across entities, DTOs, controllers, and services, switches ownership checks from wallet-based to id-based, tightens TypeScript types (strict mode), simplifies middleware by removing logging/latency, drops caching in products, and updates docs/tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API (AuthController)
participant AuthSvc as AuthService
participant UserSvc as UserService
participant JWT as JWT
Client->>API: POST /auth/register { walletAddress, role, name?, email? }
API->>AuthSvc: registerWithWallet(data)
AuthSvc->>UserSvc: createUser({ walletAddress, role, name, email })
UserSvc-->>AuthSvc: User { id (number), ... }
AuthSvc->>JWT: sign({ id, walletAddress, role[] })
JWT-->>AuthSvc: token, exp
AuthSvc-->>API: { user { id, walletAddress, name, email, role[] }, token, expiresIn }
API-->>Client: 200 { success: true, data: { user, token, expiresIn } }
sequenceDiagram
autonumber
actor Client
participant API as API (UsersController)
participant UserSvc as UserService
Client->>API: PUT /users/update/:id
API->>API: ParseIntPipe on :id
API->>UserSvc: updateUser(id:number, { name?, email? })
UserSvc-->>API: Updated User { id:number, ... }
API-->>Client: 200 { success: true, data: { id, walletAddress, name, email, role[] } }
sequenceDiagram
autonumber
actor Seller
participant API as API (OffersService)
participant Repo as OfferRepo
participant BR as BuyerRequestRepo
Seller->>API: update(offerId, dto, sellerId:number)
API->>Repo: findOne(offerId)
Repo-->>API: Offer { sellerId }
API->>API: authorize sellerId === request.sellerId
API->>Repo: save(updatedOffer)
Repo-->>API: Offer
API-->>Seller: Offer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (41)
docker-compose.yml (1)
1-17: Remove remaining Redis cache references
The codebase still includes Redis-specific logic—imports insrc/cache/*, thecache.module,cache.service, interceptors, and theREDIS_URLentry insrc/config/env.ts. Remove or refactor all Redis/cache code and configuration to complete the Redis removal.src/modules/notifications/dto/notification.dto.ts (1)
21-25: Convert userId in UserNotificationDto to a numeric type.Update
userIdfromstringtonumberand replace@IsString()with@IsNumber()(adding@Type(() => Number)if using class-transformer) to align with the numeric IDs used in your entities.src/modules/products/services/product.service.ts (2)
170-172: Validate sort order before type assertion.The code casts
order.toUpperCase()to'ASC' | 'DESC'without validation. If a user provides an invalid sort value likename:invalid, this could lead to unexpected behavior or SQL errors.Apply this diff to add validation:
if (sort) { const [field, order] = sort.split(':'); + const sortOrder = order?.toUpperCase(); + if (sortOrder !== 'ASC' && sortOrder !== 'DESC') { + throw new Error('Invalid sort order. Use ASC or DESC'); + } - queryBuilder.orderBy(`product.${field}`, order.toUpperCase() as 'ASC' | 'DESC'); + queryBuilder.orderBy(`product.${field}`, sortOrder as 'ASC' | 'DESC'); } else { queryBuilder.orderBy('product.createdAt', 'DESC'); }
170-172: Validate field name to prevent SQL injection.The code dynamically interpolates the
fieldvalue into the SQL query without validation. A malicious user could providesort=constructor:ASCor other field names that could cause errors or potentially expose sensitive data through error messages.Apply this diff to add field validation:
if (sort) { const [field, order] = sort.split(':'); + const allowedFields = ['name', 'price', 'createdAt', 'category']; + if (!allowedFields.includes(field)) { + throw new Error(`Invalid sort field. Allowed fields: ${allowedFields.join(', ')}`); + } const sortOrder = order?.toUpperCase(); if (sortOrder !== 'ASC' && sortOrder !== 'DESC') { throw new Error('Invalid sort order. Use ASC or DESC'); } queryBuilder.orderBy(`product.${field}`, sortOrder as 'ASC' | 'DESC'); } else { queryBuilder.orderBy('product.createdAt', 'DESC'); }src/app.module.ts (2)
46-47: Coding guideline violation: Direct process.env access in TypeORM configuration.Lines 46, 47, 67, and 68 access
process.envdirectly, violating the coding guideline: "Do not access environment variables via process.env directly; import and use the config object from src/config/env".Based on coding guidelines.
Refactor to use the config object:
+import { config } from './config'; + @Module({ imports: [ ConfigModule.forRoot({ isGlobal: true }), ScheduleModule.forRoot(), TypeOrmModule.forRoot({ type: 'postgres', - url: process.env.DATABASE_URL, - ssl: process.env.DB_SSL === 'true' ? { rejectUnauthorized: false } : undefined, + url: config.database.url || `postgresql://${config.database.username}:${config.database.password}@${config.database.host}:${config.database.port}/${config.database.name}`, + ssl: config.database.ssl ? { rejectUnauthorized: false } : undefined, entities: [ // ... entities ], - synchronize: process.env.NODE_ENV !== 'production', - logging: process.env.NODE_ENV === 'development', + synchronize: config.database.synchronize, + logging: config.database.logging, }),Note: You may need to add a
urlfield to the database config insrc/config/index.tsor construct it from existing fields.Also applies to: 67-68
40-84: Remove or update lingering escrow tests and docs
The filesrc/modules/escrow/tests/escrow.integration.spec.tsstill contains references to removed routes (/escrow/...) and entities (EscrowAccount,Milestone)—delete or refactor these tests, and update any documentation that mentions escrow functionality.src/config/index.ts (1)
1-29: Refactor config to use ENV from src/config/env.ts
Import { ENV } from './env' in src/config/index.ts and replace all direct process.env accessors with the corresponding ENV properties (e.g., ENV.PORT, ENV.JWT_SECRET, etc.).src/modules/files/tests/file.controller.spec.ts (2)
14-14: Replaceanytype with explicit type.The
responseObjvariable is typed asany, which violates the coding guideline to avoid theanytype. Use an explicit type orunknowninstead.As per coding guidelines.
Apply this diff:
- let responseObj: any; + let responseObj: { status: jest.Mock; json: jest.Mock };
87-97: Remove invalidroleproperty from mockUserThe User entity (
src/modules/users/entities/user.entity.ts) doesn’t define arolefield—use the existinguserRolesrelation instead. Omitrole: [Role.USER]from this mock for consistency.src/modules/orders/dto/order.dto.ts (1)
6-6: Migrate Order and OrderItem ID fields to numeric
- In src/modules/orders/entities/order.entity.ts (line 23) change
@PrimaryGeneratedColumn('uuid') id: string
to
@PrimaryGeneratedColumn() id: number- In src/modules/orders/entities/order-item.entity.ts (lines 8, 14) change
@PrimaryGeneratedColumn('uuid') id: string
and
@Column({ type: 'uuid' }) product_id: string
to use numeric types (e.g.@PrimaryGeneratedColumn() id: numberand@Column({ type: 'int' }) product_id: number)- Update DTO at src/modules/orders/dto/order.dto.ts (lines 6, 9, 23) so that both
idandproduct_idare typed asnumberto match the migrated entitiessrc/modules/wishlist/tests/wishlist.controller.spec.ts (2)
114-131: Add error case tests for removeFromWishlist.The test suite only covers the success scenario. Per coding guidelines, both success and error cases should be tested.
Consider adding test cases for:
- Product not found in wishlist
- User not found
- Database errors during removal
Example test structure:
it('should throw NotFoundException when product is not in wishlist', async () => { const userId = 'user-id'; const productId = 'product-id'; const req = mockRequest({ user: { id: userId, walletAddress: 'test-wallet', role: [Role.USER], }, }) as unknown as AuthRequest; jest .spyOn(service, 'removeFromWishlist') .mockRejectedValueOnce(new NotFoundException('Product not found in wishlist')); await expect(controller.removeFromWishlist(productId, req)).rejects.toThrow(NotFoundException); });
133-150: Add error case tests for getWishlist.The test suite only covers the success scenario. Per coding guidelines, both success and error cases should be tested.
Consider adding test cases for:
- Empty wishlist
- User not found
- Database errors during retrieval
Example test structure:
it('should return empty array when user has no wishlist items', async () => { const userId = 'user-id'; const req = mockRequest({ user: { id: userId, walletAddress: 'test-wallet', role: [Role.USER], }, }) as unknown as AuthRequest; jest.spyOn(service, 'getWishlist').mockResolvedValueOnce([]); const result = await controller.getWishlist(req); expect(result.wishlist).toEqual([]); }); it('should handle service errors gracefully', async () => { const userId = 'user-id'; const req = mockRequest({ user: { id: userId, walletAddress: 'test-wallet', role: [Role.USER], }, }) as unknown as AuthRequest; jest .spyOn(service, 'getWishlist') .mockRejectedValueOnce(new Error('Database error')); await expect(controller.getWishlist(req)).rejects.toThrow('Database error'); });src/common/filters/http-exception.filter.ts (3)
12-12: Add explicit return type and replaceanytype.The
catchmethod violates coding guidelines:
- Missing explicit return type (should be
: void)- Parameter
exceptionusesanytype instead ofunknownor a specific typeAs per coding guidelines.
Apply this diff:
- catch(exception: any, host: ArgumentsHost) { + catch(exception: unknown, host: ArgumentsHost): void {Note: You'll need to add type guards when accessing properties on
exceptionthroughout the method since it's nowunknown.
29-31: Replaceanytype assertion with a proper interface.The type assertion
(exceptionResponse as any)violates coding guidelines. Define an interface for the exception response shape.As per coding guidelines.
Apply this diff to define and use a proper interface:
+interface ExceptionResponse { + message?: string; + [key: string]: any; +} + @Catch() export class HttpExceptionFilter implements ExceptionFilter {Then update line 30:
- } else if (typeof exceptionResponse === 'object' && exceptionResponse !== null) { - message = (exceptionResponse as any).message || exception.message || message; - } + } else if (typeof exceptionResponse === 'object' && exceptionResponse !== null) { + const typedResponse = exceptionResponse as ExceptionResponse; + message = typedResponse.message || (exception as Error).message || message; + }
40-43: Avoid directprocess.envaccess; use the central config.Import
ENVfrom your config and replace the inline check:+import { ENV } from '../../config'; … - ...(process.env.NODE_ENV === 'development' && { + ...(ENV.NODE_ENV === 'development' && { error: exception.stack, timestamp: new Date().toISOString(), }),This ensures you’re not reaching into
process.envdirectly but using the shared environment config.src/common/interceptors/response.interceptor.ts (1)
28-31: Removeanytype annotation.The explicit
anytype onformattedResponseviolates coding guidelines. Use theStandardResponseinterface suggested in the previous comment.As per coding guidelines.
Apply this diff:
- const formattedResponse: any = { + const formattedResponse: StandardResponse = { success: true, data, };src/modules/offers/services/offers.service.ts (2)
98-98: Replace console.log with proper logging.The coding guidelines explicitly prohibit
console.log. Use NestJS Logger instead for proper structured logging.Apply this diff to use proper logging:
+import { Injectable, NotFoundException, ForbiddenException, BadRequestException, Logger } from '@nestjs/common'; @Injectable() export class OffersService { + private readonly logger = new Logger(OffersService.name); + constructor(Then replace the console.log:
- console.log(`Offer #${offerId} accepted by buyer #${buyerId}. Other pending offers rejected.`); + this.logger.log(`Offer #${offerId} accepted by buyer #${buyerId}. Other pending offers rejected.`);As per coding guidelines.
121-121: Replace console.log with proper logging.Same issue as in the
acceptmethod. Use the NestJS Logger for structured logging instead ofconsole.log.Apply this diff:
- console.log(`Offer #${offerId} rejected by buyer #${buyerId}.`); + this.logger.log(`Offer #${offerId} rejected by buyer #${buyerId}.`);As per coding guidelines.
src/middleware/auth.middleware.ts (2)
5-8: Add explicit return type for the middleware function.The function lacks an explicit return type annotation.
As per coding guidelines, add a return type:
-export const authMiddleware = (req: Request, res: Response, next: NextFunction) => { +export const authMiddleware = (req: Request, res: Response, next: NextFunction): void => { // Implementación del middleware next(); };
10-17: Add explicit return types for the middleware factory and returned function.Both the factory function and the returned middleware lack explicit return type annotations.
As per coding guidelines, add return types:
-export const requireRole = (roleName: Role) => { - return (req: AuthenticatedRequest, res: Response, next: NextFunction) => { +export const requireRole = (roleName: Role): ((req: AuthenticatedRequest, res: Response, next: NextFunction) => void) => { + return (req: AuthenticatedRequest, res: Response, next: NextFunction): void => { if (!req.user || !req.user.role.includes(roleName)) { return res.status(403).json({ message: 'Forbidden' }); } next(); }; };src/modules/auth/services/auth.service.ts (4)
48-54: Replace directprocess.envaccess andconsole.logwith proper config and logging.Lines 49 and 52 violate coding guidelines:
- Direct
process.env.NODE_ENVaccess (should use the config object fromsrc/config/env)console.logusage (should use NestJS Logger)As per coding guidelines:
+import { Logger } from '@nestjs/common'; + +// In the class: +private readonly logger = new Logger(AuthService.name); + // In verifyStellarSignature: if ( - process.env.NODE_ENV === 'development' && + config.nodeEnv === 'development' && signature === 'base64-encoded-signature-string-here' ) { - console.log('Development mode: Bypassing signature verification for testing'); + this.logger.warn('Development mode: Bypassing signature verification for testing'); return true; }Note: Ensure
configexports anodeEnvproperty.
60-64: Replaceconsole.errorwith proper logging.Line 62 uses
console.error, which violates coding guidelines.Use NestJS Logger instead:
} catch (error) { - console.error('Signature verification error:', error); + this.logger.error('Signature verification error:', error); return false; }
70-75: Add explicit return type for the method.The method lacks an explicit return type annotation.
As per coding guidelines:
async registerWithWallet(data: { walletAddress: string; role: 'buyer' | 'seller'; name?: string; email?: string; - }): Promise<{ user: User; token: string; expiresIn: number }> { + }): Promise<{ user: User; token: string; expiresIn: number }> {Wait, the return type is already present in the provided code. Let me re-check the annotated code...
Actually, looking at the code again, line 75 shows the return type is present. This is correct.
[analysis]
Return type is present; no issue here.
[actions]
Review the JWT token generation duplication instead
-->
163-174: Accept numericidparameter and validate input.The method accepts a
stringbut immediately converts it tonumberusingNumber(id), which can returnNaNfor invalid inputs. This creates a type inconsistency and potential runtime issue.Change the signature to accept
numberdirectly, or validate the string conversion:Option 1 (preferred): Accept
numberdirectly:- async getUserById(id: string): Promise<User> { + async getUserById(id: number): Promise<User> { const user = await this.userRepository.findOne({ - where: { id: Number(id) }, + where: { id }, relations: ['userRoles', 'userRoles.role'], });Option 2: Validate string conversion:
async getUserById(id: string): Promise<User> { + const numericId = Number(id); + if (isNaN(numericId)) { + throw new BadRequestError('Invalid user ID format'); + } const user = await this.userRepository.findOne({ - where: { id: Number(id) }, + where: { id: numericId }, relations: ['userRoles', 'userRoles.role'], });src/modules/shared/middleware/session.middleware.ts (1)
8-8: Replace directprocess.envaccess with the config object.Line 8 accesses
process.env.JWT_SECRETdirectly, which violates coding guidelines.As per coding guidelines, import and use the config object:
+import { config } from '../../../config'; + -const jwtService = new JwtService({ secret: process.env.JWT_SECRET }); +const jwtService = new JwtService({ secret: config.jwtSecret });src/modules/reviews/dto/review.dto.ts (1)
8-16: Type inconsistency between DTOs in this file.
ReviewResponseDTO.userIdis nownumber(line 10), butCreateReviewDto.userIdis still typed as optionalstring(line 40). Similarly,ReviewResponseDTO.productIdisnumber(line 11) whileCreateReviewDto.productIdisstring(line 27).This mismatch will require manual type coercion and creates confusion about the actual types used by the API.
Consider aligning all ID fields to use consistent numeric types:
export class CreateReviewDto { - @IsString() + @IsNumber() @IsNotEmpty() - productId: string; + productId: number; @IsNumber() @Min(1) @Max(5) rating: number; @IsString() @IsNotEmpty() comment: string; - @IsString() + @IsNumber() @IsOptional() - userId?: string; + userId?: number; }src/types/auth-request.type.ts (1)
4-12: Type inconsistency betweenAppUser.idandAuthenticatedRequest.user.id.
AppUser.idis typed asstring(line 5), butAuthenticatedRequest.user.idis nowstring | number(line 16). SinceAuthenticatedRequest.userappears to represent the same user entity, this creates a confusing type mismatch.If the migration goal is to move fully to numeric IDs, consider:
- Updating
AppUser.idto also bestring | numberfor consistency- Or documenting that
AppUserrepresents the database entity (string ID) whileAuthenticatedRequest.userrepresents the JWT payload (transitional type)export interface AppUser { - id: string; + id: string | number; walletAddress: string; name?: string; email?: string; role: Role[]; createdAt?: Date; updatedAt?: Date; }src/modules/buyer-requests/entities/buyer-request.entity.ts (1)
29-30: Add missing migration for buyer_requests.userId type change
No migration file was found for the change to theuserIdcolumn on thebuyer_requeststable—please create and apply a migration to update its type.src/modules/auth/controllers/role.controller.ts (1)
19-25: Add explicit type for req parameter and provide return type documentation.The
reqparameter is untyped (implicitany), which violates the coding guideline against usingany. It should be typed asAuthenticatedRequestor a proper Express request type with user.Apply this diff:
+import { AuthenticatedRequest } from '../../shared/middleware/auth.middleware'; + @Get('my-roles') @UseGuards(JwtAuthGuard) - async getMyRoles(@Req() req): Promise<{ roles: string[] }> { + async getMyRoles(@Req() req: AuthenticatedRequest): Promise<{ roles: string[] }> { const roles = await this.roleService.getUserRoles(req.user.id); return { roles: roles.map((role) => role.name) }; }As per coding guidelines
src/modules/shared/middleware/auth.middleware.ts (1)
78-88: Fix requireRole to avoid instantiating dependencies with null.Line 82 creates an
AuthMiddlewareinstance withnullfor both constructor parameters (new AuthMiddleware(null, null)) just to callmapRoleToEnum. This is fragile and will cause runtime errors ifmapRoleToEnumor any other method tries to access those dependencies.Extract
mapRoleToEnumto a standalone utility function:Create a new utility file (e.g.,
src/utils/role-mapper.ts):import { Role } from '../types/role'; export function mapRoleToEnum(roleName: string): Role { switch (roleName.toLowerCase()) { case 'admin': return Role.ADMIN; case 'seller': return Role.SELLER; case 'buyer': case 'user': return Role.USER; default: return Role.USER; } }Then update this file:
+import { mapRoleToEnum } from '../../../utils/role-mapper'; + @Injectable() export class AuthMiddleware implements NestMiddleware { constructor( private readonly jwtService: JwtService, private readonly roleService: RoleService ) {} - public mapRoleToEnum(roleName: string): Role { - switch (roleName.toLowerCase()) { - case 'admin': - return Role.ADMIN; - case 'seller': - return Role.SELLER; - case 'buyer': - case 'user': - return Role.USER; - default: - return Role.USER; - } - } async use(req: AuthenticatedRequest, res: Response, next: NextFunction): Promise<void> { try { // ... (rest of the method) const userRoles = await this.roleService.getUserRoles(decoded.id); - req.user = { ...decoded, role: userRoles.map((role) => this.mapRoleToEnum(role.name)) }; + req.user = { ...decoded, role: userRoles.map((role) => mapRoleToEnum(role.name)) }; // ... } } } export const requireRole = ( roleName: 'buyer' | 'seller' | 'admin' ): ((req: AuthenticatedRequest, res: Response, next: NextFunction) => void) => { return (req: AuthenticatedRequest, res: Response, next: NextFunction) => { - const requiredRole = new AuthMiddleware(null, null).mapRoleToEnum(roleName); + const requiredRole = mapRoleToEnum(roleName); if (!req.user || !req.user.role.includes(requiredRole)) { throw new ReferenceError('Insufficient permissions'); } next(); }; };src/modules/reviews/services/review.service.ts (1)
95-114: Potential authorization bypass: type mismatch in ownership check.Line 104 performs an ownership check:
if (review.userId !== userId). Ifreview.userIdis stored as a string in the database but the parameteruserIdis a number, this comparison will use loose equality semantics that might not work as expected, or it will always fail the check.Ensure both sides of the comparison have the same type. If the Review entity stores
userIdas a number, this should work correctly. If it's stored as a string, you need to convert the parameter:async deleteReview(userId: number, reviewId: string): Promise<boolean> { const review = await this.repository.findOne({ where: { id: reviewId }, }); if (!review) { throw new NotFoundError(`Review with ID ${reviewId} not found`); } - if (review.userId !== userId) { + if (Number(review.userId) !== userId) { throw new BadRequestError('You can only delete your own reviews'); }Or better yet, ensure consistent types across the codebase so no conversion is needed.
src/modules/users/entities/user.entity.ts (1)
16-46: Add migration to drop removed user columns
TypeORM doesn’t automatically removepayoutWallet,sellerOnchainRegistered,country,location,buyerData,sellerDataor thestoresrelation—create a new migration insrc/migrationsto drop these now-deleted columns.src/modules/auth/tests/auth.service.spec.ts (1)
200-210: registerWithWallet missing signature verificationThe test in src/modules/auth/tests/auth.service.spec.ts lines 200–210 expects an UnauthorizedError on invalid signature, but AuthService.registerWithWallet neither accepts nor verifies any signature and thus can’t throw here. Implement signature verification in registerWithWallet or adjust/remove the invalid-signature test.
src/modules/auth/services/role.service.ts (4)
7-7: Remove duplicate type definition.The
RoleNametype is already defined and exported fromsrc/modules/auth/types/role.types.ts(visible in relevant code snippets). This local duplicate violates DRY principles and can lead to inconsistencies if the canonical type is updated.Apply this diff:
-type RoleName = 'buyer' | 'seller' | 'admin'; +import { RoleName } from '../types/role.types';
44-53: Inconsistent parameter type for userId.The
assignRoleToUsermethod acceptsuserId: stringand parses it to number (line 50), while related methods likeremoveRoleFromUser(line 55) acceptuserId: numberdirectly. This inconsistency forces unnecessary type conversions and creates confusion about the expected type.Apply this diff to align with numeric ID usage throughout the PR:
-async assignRoleToUser(userId: string, roleName: string): Promise<void> { +async assignRoleToUser(userId: number, roleName: string): Promise<void> { const role = await this.findByName(roleName as RoleName); if (!role) { throw new Error(`Role ${roleName} not found`); } await this.userRoleRepository.save({ - userId: parseInt(userId), + userId, roleId: role.id, }); }
7-7: Remove duplicate type definition.The
RoleNametype is already defined insrc/modules/auth/types/role.types.ts. This local redefinition creates maintenance overhead and potential drift.Apply this diff to use the imported type:
+import { RoleName } from '../types/role.types'; -type RoleName = 'buyer' | 'seller' | 'admin';
44-53: Standardize userId parameter type to number.The method accepts
userId: stringbut immediately converts it to a number withparseInt. This is inconsistent with other methods in the class (removeRoleFromUser,hasRole,hasAnyRole) that acceptnumberdirectly.Apply this diff to align with the numeric ID migration:
-async assignRoleToUser(userId: string, roleName: string): Promise<void> { +async assignRoleToUser(userId: number, roleName: string): Promise<void> { const role = await this.findByName(roleName as RoleName); if (!role) { throw new Error(`Role ${roleName} not found`); } await this.userRoleRepository.save({ - userId: parseInt(userId), + userId, roleId: role.id, }); }src/modules/buyer-requests/services/buyer-requests.service.ts (1)
252-284: Add explicit return type.The private method
mapToResponseDtolacks an explicit return type annotation, which violates the coding guideline requiring explicit return types for all functions.As per coding guidelines.
Apply this diff:
-private mapToResponseDto(buyerRequest: BuyerRequest): BuyerRequestResponseDto { +private mapToResponseDto(buyerRequest: BuyerRequest): BuyerRequestResponseDto {Note: The return type is already present in the code, but ensure it's consistently applied if any similar methods exist.
src/modules/users/services/user.service.ts (2)
79-90: Replaceany[]return type with explicit type.The return type
Promise<any[]>violates the coding guideline "Do not use the any type; use unknown or specific types instead." Define an explicit return type for orders or use the Order entity type.As per coding guidelines.
Example fix:
- async getUserOrders(id: string): Promise<any[]> { + async getUserOrders(id: string): Promise<Order[]> {(Import the Order entity at the top of the file if not already present.)
92-103: Replaceany[]return type with explicit type.The return type
Promise<any[]>violates the coding guideline "Do not use the any type; use unknown or specific types instead." Define an explicit return type for wishlist items or use the Wishlist entity type.As per coding guidelines.
Example fix:
- async getUserWishlist(id: string): Promise<any[]> { + async getUserWishlist(id: string): Promise<Wishlist[]> {(Import the Wishlist entity at the top of the file if not already present.)
src/modules/users/controllers/user.controller.ts (1)
76-79: Do not access environment variables directly; use config object.Direct access to
process.env.NODE_ENVviolates the coding guideline: "Do not access environment variables via process.env directly; import and use the config object from src/config/env."As per coding guidelines.
Import and use the config object:
+import { config } from '../../../config/env'; + ... res.cookie('token', result.token, { httpOnly: true, - secure: process.env.NODE_ENV === 'production', + secure: config.NODE_ENV === 'production', sameSite: 'strict', maxAge: result.expiresIn * 1000, });
🧹 Nitpick comments (18)
src/middleware/async-handler.ts (1)
3-9: Good type safety improvement with the generic parameter.The migration from
anyto a generic type parameterT = unknownaligns well with the coding guidelines and improves type safety at call sites. The explicitRequestHandlerreturn type and typed parameters are also appropriate.Consider adding JSDoc for developer experience.
While the function is straightforward, a brief JSDoc comment would help developers understand the purpose and usage of this utility, especially regarding error propagation to Express error handlers.
Example JSDoc:
+/** + * Wraps an async Express route handler to automatically catch and forward errors. + * @template T - The promise return type of the handler function + * @param fn - Async function to wrap + * @returns Express RequestHandler that catches promise rejections + */ export const asyncHandler = <T = unknown>( fn: (req: Request, res: Response, next: NextFunction) => Promise<T> ): RequestHandler => {tsconfig.json (1)
18-22: LGTM! Strict mode enables comprehensive type safety.Enabling
strict: trueis an excellent change that enforces type discipline across the codebase and aligns with the PR's migration to stricter types.Note:
strictNullChecks,noImplicitAny, andstrictBindCallApplyare already enabled bystrict: true, making lines 20-22 redundant. You may optionally remove them for cleaner configuration:"skipLibCheck": true, "strict": true, - "strictNullChecks": true, - "noImplicitAny": true, - "strictBindCallApply": true, "forceConsistentCasingInFileNames": false,README.md (1)
483-502: Update README audit command for preciseanydetection
In README.md (line 499), replace the existing grep audit with:rg '\b(:\s*any\b|<any>|as\s+any\b|any\[\])' src --type ts -g '!*.spec.ts' || echo "No 'any' types found"This uses word boundaries to avoid false positives and excludes test files (current codebase has zero matches).
src/modules/products/services/product.service.ts (1)
47-50: Consider clarifying the dual repository pattern.The service initializes two separate repositories (
this.repositoryfromAppDataSourceandthis.productRepositoryfromDatabaseAppDataSource). This creates potential confusion about which repository to use in which context. Different methods use different repositories (e.g.,createusesthis.repositorywhilecreateProductusesthis.productRepository), which could lead to inconsistencies.Consider documenting why two data sources are needed or consolidating to a single repository if both point to the same database.
package.json (2)
8-8: Remove duplicate coverage script.Both
test:coverage(line 8) andtest:cov(line 23) run identical commands (jest --coverage). This duplication can cause confusion and maintenance issues.Apply this diff to remove the duplicate:
"test:coverage": "jest --coverage", "dev": "nest start --watch", "build": "nest build", "start": "nest start", "start:dev": "nest start --watch", "start:debug": "nest start --debug --watch", "start:prod": "node dist/main", "test:crypto": "node test-crypto.js", "docs:validate": "swagger-cli validate openapi.yaml", "docs:build": "redoc-cli build openapi.yaml -o static-docs.html", "typeorm": "typeorm", "lint": "eslint . --ext .ts", "lint:fix": "eslint . --ext .ts --fix", "format": "prettier --write \"src/**/*.ts\"", "prepare": "husky install", - "test:cov": "jest --coverage", "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand",Also applies to: 23-23
57-57: Consider upgrading to Express v5.The project uses Express v4.21.2 while NestJS v11 officially supports Express v5 as the default HTTP adapter. Express v5 has been stable since September 2024 and includes routing improvements and security updates.
If there are no specific compatibility concerns, consider upgrading:
- "express": "^4.21.2", + "express": "^5.0.1",Note: Review the Express v4→v5 migration guide for breaking changes in route matching (wildcard syntax, optional parameters). Based on learnings.
src/modules/files/tests/file.controller.spec.ts (1)
16-42: Consider using NestJS testing utilities for proper dependency injection.The current setup manually instantiates
FileServiceandFileController, then assigns the service via type assertion. This approach is brittle and doesn't follow NestJS testing best practices.Consider refactoring to use
Test.createTestingModule():+import { Test, TestingModule } from '@nestjs/testing'; + describe('FileController', () => { let fileController: FileController; let fileService: jest.Mocked<FileService>; let mockRequest: Partial<AuthenticatedRequest>; let mockResponse: Partial<Response>; let responseObj: any; - beforeEach(() => { + beforeEach(async () => { // Reset mocks jest.clearAllMocks(); - fileService = new FileService() as jest.Mocked<FileService>; - - fileController = new FileController(); - (fileController as unknown as { fileService: FileService }).fileService = fileService; + const module: TestingModule = await Test.createTestingModule({ + controllers: [FileController], + providers: [ + { + provide: FileService, + useValue: { + uploadFile: jest.fn(), + getUserFiles: jest.fn(), + getFileById: jest.fn(), + deleteFile: jest.fn(), + getFilesByType: jest.fn(), + }, + }, + ], + }).compile(); + + fileController = module.get<FileController>(FileController); + fileService = module.get<FileService>(FileService) as jest.Mocked<FileService>; responseObj = { status: jest.fn().mockReturnThis(), json: jest.fn().mockReturnThis(), }; mockResponse = responseObj; // Setup basic mock request mockRequest = { user: { id: 'user-uuid', walletAddress: '0x123', role: [Role.USER], }, fileProvider: 'cloudinary', fileType: FileType.IMAGE, params: {}, }; });src/modules/wishlist/tests/wishlist.controller.spec.ts (1)
77-112: Align test fixtures with entity types
- Use numeric values for
userIdandproductIdin mocks to matchid: numberonUserandProductentities (you may castproductIdto string for controller parameters).- Keep
walletAddressin the mock sinceAuthRequest.userandUserstill require it; remove in a follow-up once those types are updated.src/modules/auth/services/auth.service.ts (1)
90-97: Extract duplicate JWT token generation logic into a helper method.The JWT token generation logic is duplicated in lines 90-97 and 124-130. This violates the DRY principle and makes maintenance harder.
Extract into a private helper method:
private generateToken(user: User, role: string): { token: string; expiresIn: number } { const token = sign( { id: user.id, walletAddress: user.walletAddress, role }, config.jwtSecret, { expiresIn: '1h' } ); return { token, expiresIn: 3600 }; }Then use it in both places:
- const role = updatedUser.userRoles?.[0]?.role?.name || 'buyer'; - const token = sign( - { id: updatedUser.id, walletAddress: updatedUser.walletAddress, role }, - config.jwtSecret, - { - expiresIn: '1h', - } - ); - - return { user: updatedUser, token, expiresIn: 3600 }; + const role = updatedUser.userRoles?.[0]?.role?.name || 'buyer'; + const { token, expiresIn } = this.generateToken(updatedUser, role); + return { user: updatedUser, token, expiresIn };Apply the same pattern at lines 124-130.
Also applies to: 124-130
src/modules/auth/dto/auth-response.dto.ts (1)
102-110: Verify consistency of UserResponseDto.data with UserDto.The
UserResponseDto.datais an inline object type that includesid: numberand timestamp fields (createdAt,updatedAt), but it doesn't directly reference or extendUserDto. This creates potential for drift between the two definitions if one is updated without the other.Consider refactoring to compose the type from
UserDto:data: { - id: number; - walletAddress: string; - name: string; - email: string; - role: string; - createdAt: Date; - updatedAt: Date; + createdAt: Date; + updatedAt: Date; + } & UserDto;Or create a shared interface that both can reference to reduce duplication and maintain consistency.
src/modules/reviews/services/review.service.ts (1)
20-61: Harmonize UserService.getUserById to use numeric IDs
InReviewService.createReview(src/modules/reviews/services/review.service.ts:36), we’re convertinguserId: numbertostringforuserService.getUserById(String(userId))becausegetUserByIdcurrently takesid: string. Since both theUser.idandReview.userIdfields arenumber, update thegetUserByIdsignature toasync getUserById(id: number): Promise<User>insrc/modules/users/services/user.service.tsand remove theString()cast here. This will keep types consistent across services.src/middleware/userValidation.middleware.ts (2)
9-9: Note: Duplicate type alias across middleware files.
ClassType<T>here is identical toConstructor<T>insrc/middleware/validation.middleware.ts(line 5). Consider consolidating these type aliases into a shared types file to reduce duplication and ensure consistency.For example, create
src/middleware/types.ts:export type Constructor<T> = new (...args: unknown[]) => T;Then import it in both files:
import { Constructor } from './types';
20-20: Note: Inconsistent validation error status codes.This middleware returns 422 (Unprocessable Entity) for validation errors, while
src/middleware/validation.middleware.tsreturns 400 (Bad Request) for similar validation failures. Consider standardizing on one status code for consistency, unless the distinction is intentional (e.g., 422 for DTO validation vs. 400 for express-validator chains).src/modules/buyer-requests/services/buyer-requests.service.ts (2)
140-151: Consider extracting ownership validation to reduce duplication.The ownership check pattern (fetch entity, verify existence, verify ownership) is repeated across
update,remove, andcloseRequestmethods. This duplication increases maintenance burden and the risk of inconsistent error handling.Consider extracting to a private helper method:
private async verifyOwnership(id: number, userId: number): Promise<BuyerRequest> { const buyerRequest = await this.buyerRequestRepository.findOne({ where: { id }, }); if (!buyerRequest) { throw new NotFoundException('Buyer request not found'); } if (buyerRequest.userId !== userId) { throw new ForbiddenException('You can only modify your own buyer requests'); } return buyerRequest; }Then simplify the methods:
async update( id: number, updateBuyerRequestDto: UpdateBuyerRequestDto, userId: number ): Promise<BuyerRequestResponseDto> { const buyerRequest = await this.verifyOwnership(id, userId); // Check if request is still open if (buyerRequest.status !== BuyerRequestStatus.OPEN) { throw new ForbiddenException('Cannot update closed or fulfilled requests'); } // ... rest of update logic }Also applies to: 190-201, 300-302
140-151: Consider extracting duplicate ownership validation logic.The ownership check pattern (fetch entity → check existence → verify ownership) is repeated in
update,remove, andcloseRequestmethods. Extracting this into a private helper would reduce duplication and ensure consistent error messages.Consider adding a private helper method:
private async validateOwnership(id: number, userId: number): Promise<BuyerRequest> { const buyerRequest = await this.buyerRequestRepository.findOne({ where: { id }, }); if (!buyerRequest) { throw new NotFoundException('Buyer request not found'); } if (buyerRequest.userId !== userId) { throw new ForbiddenException('You can only modify your own buyer requests'); } return buyerRequest; }Then simplify the methods:
async update( id: number, updateBuyerRequestDto: UpdateBuyerRequestDto, userId: number ): Promise<BuyerRequestResponseDto> { const buyerRequest = await this.validateOwnership(id, userId); // Check if request is still open if (buyerRequest.status !== BuyerRequestStatus.OPEN) { throw new ForbiddenException('Cannot update closed or fulfilled requests'); } // ... rest of logic } async remove(id: number, userId: number): Promise<void> { const buyerRequest = await this.validateOwnership(id, userId); await this.buyerRequestRepository.remove(buyerRequest); }Also applies to: 190-201
src/modules/users/services/user.service.ts (2)
46-55: Type inconsistency: string parameter parsed to number.The method accepts
id: stringbut immediately parses it to a number withparseInt(id). Since the controller now usesParseIntPipeto receive numeric IDs, this service method should acceptid: numberdirectly to eliminate redundant conversions and improve type safety.Apply this diff:
- async getUserById(id: string): Promise<User> { + async getUserById(id: number): Promise<User> { const user = await this.userRepository.findOne({ - where: { id: parseInt(id) }, + where: { id }, relations: ['userRoles', 'userRoles.role'], });Then update the call site in the controller at line 151 to pass the numeric
userIddirectly instead of converting it back to a string.
79-81: Align parameter types with ID migration.Both
getUserOrdersandgetUserWishlistacceptid: stringbut parse it to a number withparseInt(id). Consistent with the PR's migration to numeric IDs, these methods should acceptid: numberdirectly to avoid redundant parsing.Apply this diff:
- async getUserOrders(id: string): Promise<any[]> { + async getUserOrders(id: number): Promise<Order[]> { const user = await this.userRepository.findOne({ - where: { id: parseInt(id) }, + where: { id }, relations: ['orders'], }); ... - async getUserWishlist(id: string): Promise<any[]> { + async getUserWishlist(id: number): Promise<Wishlist[]> { const user = await this.userRepository.findOne({ - where: { id: parseInt(id) }, + where: { id }, relations: ['wishlist'], });Also applies to: 92-94
src/modules/users/controllers/user.controller.ts (1)
151-151: Redundant type conversion from number to string.The
userIdparameter is parsed to a number viaParseIntPipe, but then converted back to a string withString(userId)when callinguserService.getUserById. This indicates a design inconsistency. The service method should acceptnumberdirectly to align with the ID migration.After updating
userService.getUserByIdto acceptnumber(as suggested in the service file review), remove the string conversion:- const user = await this.userService.getUserById(String(userId)); + const user = await this.userService.getUserById(userId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (58)
README.md(2 hunks)docker-compose.yml(1 hunks)package.json(2 hunks)src/app.module.ts(2 hunks)src/common/filters/http-exception.filter.ts(1 hunks)src/common/interceptors/response.interceptor.ts(1 hunks)src/config/index.ts(1 hunks)src/dtos/ProductVariantDTO.ts(1 hunks)src/dtos/UserDTO.ts(0 hunks)src/main.ts(0 hunks)src/middleware/async-handler.ts(1 hunks)src/middleware/auth.middleware.ts(1 hunks)src/middleware/paramValidation.middleware.ts(1 hunks)src/middleware/userValidation.middleware.ts(1 hunks)src/middleware/validateRequest.middleware.ts(1 hunks)src/middleware/validation.middleware.ts(1 hunks)src/modules/auth/auth.module.ts(0 hunks)src/modules/auth/controllers/auth.controller.ts(3 hunks)src/modules/auth/controllers/role.controller.ts(1 hunks)src/modules/auth/dto/auth-response.dto.ts(5 hunks)src/modules/auth/dto/auth.dto.ts(1 hunks)src/modules/auth/entities/user-role.entity.ts(1 hunks)src/modules/auth/index.ts(0 hunks)src/modules/auth/middleware/authorize-roles.middleware.ts(1 hunks)src/modules/auth/middleware/jwt-auth.middleware.ts(1 hunks)src/modules/auth/services/auth.service.ts(9 hunks)src/modules/auth/services/role.service.ts(1 hunks)src/modules/auth/strategies/jwt.strategy.ts(1 hunks)src/modules/auth/tests/auth.service.spec.ts(4 hunks)src/modules/buyer-requests/dto/buyer-request-response.dto.ts(1 hunks)src/modules/buyer-requests/entities/buyer-request.entity.ts(1 hunks)src/modules/buyer-requests/services/buyer-requests.service.ts(4 hunks)src/modules/files/tests/file.controller.spec.ts(1 hunks)src/modules/files/tests/file.service.spec.ts(0 hunks)src/modules/files/tests/test-utils.ts(0 hunks)src/modules/notifications/dto/notification.dto.ts(1 hunks)src/modules/offers/services/offers.service.ts(2 hunks)src/modules/orders/dto/order.dto.ts(1 hunks)src/modules/orders/entities/order-item.entity.ts(0 hunks)src/modules/orders/entities/order.entity.ts(0 hunks)src/modules/products/products.module.ts(1 hunks)src/modules/products/services/product.service.ts(3 hunks)src/modules/reviews/controllers/review.controller.ts(2 hunks)src/modules/reviews/dto/review.dto.ts(1 hunks)src/modules/reviews/entities/review.entity.ts(1 hunks)src/modules/reviews/services/review.service.ts(3 hunks)src/modules/shared/middleware/auth.middleware.ts(3 hunks)src/modules/shared/middleware/session.middleware.ts(1 hunks)src/modules/shared/types/auth-request.type.ts(0 hunks)src/modules/users/controllers/user.controller.ts(5 hunks)src/modules/users/entities/user.entity.ts(1 hunks)src/modules/users/services/user.service.ts(5 hunks)src/modules/wishlist/common/types/auth-request.type.ts(0 hunks)src/modules/wishlist/tests/wishlist.controller.spec.ts(4 hunks)src/types/auth-request.type.ts(2 hunks)src/types/express.d.ts(0 hunks)src/types/global-response.type.ts(2 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (11)
- src/types/express.d.ts
- src/modules/files/tests/test-utils.ts
- src/modules/files/tests/file.service.spec.ts
- src/modules/auth/auth.module.ts
- src/modules/orders/entities/order.entity.ts
- src/modules/wishlist/common/types/auth-request.type.ts
- src/modules/shared/types/auth-request.type.ts
- src/modules/auth/index.ts
- src/main.ts
- src/modules/orders/entities/order-item.entity.ts
- src/dtos/UserDTO.ts
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.ts
📄 CodeRabbit inference engine (.cursorrules)
src/**/*.ts: Do not access environment variables via process.env directly; import and use the config object from src/config/env
Provide explicit return types for all functions
Do not use the any type; use unknown or specific types instead
Create interfaces for complex object shapes
Type all function parameters explicitly
Do not use console.log; use proper logging (e.g., NestJS Logger)
Remove unused imports
Do not leave commented-out code in commits
Wrap risky operations in try-catch and handle errors appropriately
Add JSDoc comments for complex logic
Files:
src/modules/wishlist/tests/wishlist.controller.spec.tssrc/types/global-response.type.tssrc/modules/auth/controllers/role.controller.tssrc/common/filters/http-exception.filter.tssrc/modules/auth/middleware/authorize-roles.middleware.tssrc/modules/files/tests/file.controller.spec.tssrc/types/auth-request.type.tssrc/middleware/auth.middleware.tssrc/modules/reviews/controllers/review.controller.tssrc/config/index.tssrc/middleware/validateRequest.middleware.tssrc/middleware/userValidation.middleware.tssrc/modules/auth/strategies/jwt.strategy.tssrc/dtos/ProductVariantDTO.tssrc/modules/reviews/entities/review.entity.tssrc/middleware/paramValidation.middleware.tssrc/modules/auth/dto/auth.dto.tssrc/modules/auth/dto/auth-response.dto.tssrc/modules/auth/middleware/jwt-auth.middleware.tssrc/modules/products/products.module.tssrc/modules/shared/middleware/auth.middleware.tssrc/middleware/async-handler.tssrc/app.module.tssrc/modules/buyer-requests/services/buyer-requests.service.tssrc/modules/products/services/product.service.tssrc/modules/reviews/dto/review.dto.tssrc/modules/shared/middleware/session.middleware.tssrc/modules/notifications/dto/notification.dto.tssrc/modules/buyer-requests/entities/buyer-request.entity.tssrc/modules/users/controllers/user.controller.tssrc/modules/buyer-requests/dto/buyer-request-response.dto.tssrc/modules/auth/services/role.service.tssrc/modules/offers/services/offers.service.tssrc/modules/auth/entities/user-role.entity.tssrc/common/interceptors/response.interceptor.tssrc/modules/users/entities/user.entity.tssrc/modules/reviews/services/review.service.tssrc/modules/users/services/user.service.tssrc/modules/orders/dto/order.dto.tssrc/middleware/validation.middleware.tssrc/modules/auth/controllers/auth.controller.tssrc/modules/auth/tests/auth.service.spec.tssrc/modules/auth/services/auth.service.ts
src/**/*.spec.ts
📄 CodeRabbit inference engine (.cursorrules)
src/**/*.spec.ts: Create tests for new features
Mock external dependencies in tests
Test both success and error cases
Ensure tests are deterministic (no flakiness)
Files:
src/modules/wishlist/tests/wishlist.controller.spec.tssrc/modules/files/tests/file.controller.spec.tssrc/modules/auth/tests/auth.service.spec.ts
src/modules/**
📄 CodeRabbit inference engine (.cursorrules)
Follow NestJS module structure within src/modules/[module-name]/
Files:
src/modules/wishlist/tests/wishlist.controller.spec.tssrc/modules/auth/controllers/role.controller.tssrc/modules/auth/middleware/authorize-roles.middleware.tssrc/modules/files/tests/file.controller.spec.tssrc/modules/reviews/controllers/review.controller.tssrc/modules/auth/strategies/jwt.strategy.tssrc/modules/reviews/entities/review.entity.tssrc/modules/auth/dto/auth.dto.tssrc/modules/auth/dto/auth-response.dto.tssrc/modules/auth/middleware/jwt-auth.middleware.tssrc/modules/products/products.module.tssrc/modules/shared/middleware/auth.middleware.tssrc/modules/buyer-requests/services/buyer-requests.service.tssrc/modules/products/services/product.service.tssrc/modules/reviews/dto/review.dto.tssrc/modules/shared/middleware/session.middleware.tssrc/modules/notifications/dto/notification.dto.tssrc/modules/buyer-requests/entities/buyer-request.entity.tssrc/modules/users/controllers/user.controller.tssrc/modules/buyer-requests/dto/buyer-request-response.dto.tssrc/modules/auth/services/role.service.tssrc/modules/offers/services/offers.service.tssrc/modules/auth/entities/user-role.entity.tssrc/modules/users/entities/user.entity.tssrc/modules/reviews/services/review.service.tssrc/modules/users/services/user.service.tssrc/modules/orders/dto/order.dto.tssrc/modules/auth/controllers/auth.controller.tssrc/modules/auth/tests/auth.service.spec.tssrc/modules/auth/services/auth.service.ts
src/modules/**/tests/**/*.controller.spec.ts
📄 CodeRabbit inference engine (.cursorrules)
Include controller unit tests named [module].controller.spec.ts under tests/
Files:
src/modules/wishlist/tests/wishlist.controller.spec.tssrc/modules/files/tests/file.controller.spec.ts
src/modules/**/controllers/**/*.controller.ts
📄 CodeRabbit inference engine (.cursorrules)
Handle HTTP in controllers only
Files:
src/modules/auth/controllers/role.controller.tssrc/modules/reviews/controllers/review.controller.tssrc/modules/users/controllers/user.controller.tssrc/modules/auth/controllers/auth.controller.ts
src/modules/**/entities/**/*.entity.ts
📄 CodeRabbit inference engine (.cursorrules)
Define persistence entities in entities/*.entity.ts
Files:
src/modules/reviews/entities/review.entity.tssrc/modules/buyer-requests/entities/buyer-request.entity.tssrc/modules/auth/entities/user-role.entity.tssrc/modules/users/entities/user.entity.ts
src/modules/**/dto/**/*.dto.ts
📄 CodeRabbit inference engine (.cursorrules)
src/modules/**/dto/**/*.dto.ts: Use DTOs for data validation
Use class-validator decorators on DTOs
Files:
src/modules/auth/dto/auth.dto.tssrc/modules/auth/dto/auth-response.dto.tssrc/modules/reviews/dto/review.dto.tssrc/modules/notifications/dto/notification.dto.tssrc/modules/buyer-requests/dto/buyer-request-response.dto.tssrc/modules/orders/dto/order.dto.ts
src/modules/**/[a-zA-Z0-9-]*.module.ts
📄 CodeRabbit inference engine (.cursorrules)
Provide a module file per feature module (e.g., [module].module.ts)
Files:
src/modules/products/products.module.ts
src/modules/**/services/**/*.service.ts
📄 CodeRabbit inference engine (.cursorrules)
Place business logic in services
Files:
src/modules/buyer-requests/services/buyer-requests.service.tssrc/modules/products/services/product.service.tssrc/modules/auth/services/role.service.tssrc/modules/offers/services/offers.service.tssrc/modules/reviews/services/review.service.tssrc/modules/users/services/user.service.tssrc/modules/auth/services/auth.service.ts
src/modules/**/tests/**/*.service.spec.ts
📄 CodeRabbit inference engine (.cursorrules)
Include service unit tests named [module].service.spec.ts under tests/
Files:
src/modules/auth/tests/auth.service.spec.ts
🧠 Learnings (3)
📚 Learning: 2025-09-29T16:06:54.363Z
Learnt from: CR
PR: StarShopCr/StarShop-Backend#0
File: .cursorrules:0-0
Timestamp: 2025-09-29T16:06:54.363Z
Learning: Applies to src/modules/**/dto/**/*.dto.ts : Use DTOs for data validation
Applied to files:
src/middleware/userValidation.middleware.tssrc/modules/auth/dto/auth.dto.ts
📚 Learning: 2025-09-29T16:06:54.363Z
Learnt from: CR
PR: StarShopCr/StarShop-Backend#0
File: .cursorrules:0-0
Timestamp: 2025-09-29T16:06:54.363Z
Learning: Applies to src/modules/**/dto/**/*.dto.ts : Use class-validator decorators on DTOs
Applied to files:
src/middleware/userValidation.middleware.tssrc/modules/auth/dto/auth.dto.tssrc/middleware/validation.middleware.ts
📚 Learning: 2025-09-29T16:06:54.363Z
Learnt from: CR
PR: StarShopCr/StarShop-Backend#0
File: .cursorrules:0-0
Timestamp: 2025-09-29T16:06:54.363Z
Learning: Ensure code follows NestJS patterns and is production-ready per the quality checklist
Applied to files:
README.md
🧬 Code graph analysis (7)
src/middleware/validateRequest.middleware.ts (1)
src/middleware/validation.middleware.ts (1)
validateRequest(7-22)
src/middleware/paramValidation.middleware.ts (1)
src/middleware/validateRequest.middleware.ts (1)
validateRequest(8-17)
src/modules/buyer-requests/services/buyer-requests.service.ts (2)
src/modules/buyer-requests/dto/create-buyer-request.dto.ts (1)
CreateBuyerRequestDto(14-54)src/modules/buyer-requests/dto/buyer-request-response.dto.ts (1)
BuyerRequestResponseDto(3-22)
src/modules/users/controllers/user.controller.ts (4)
src/modules/wishlist/controller/wishlist.controller.ts (1)
UseGuards(16-47)src/modules/auth/dto/auth.dto.ts (1)
UpdateUserDto(54-70)src/dtos/UserDTO.ts (1)
UpdateUserDto(30-43)src/utils/errors.ts (1)
UnauthorizedError(18-22)
src/modules/auth/services/role.service.ts (1)
src/modules/auth/types/role.types.ts (1)
RoleName(1-1)
src/middleware/validation.middleware.ts (1)
src/middleware/validateRequest.middleware.ts (1)
validateRequest(8-17)
src/modules/auth/services/auth.service.ts (2)
src/config/index.ts (1)
config(1-29)src/modules/auth/types/role.types.ts (1)
RoleName(1-1)
| POSTGRES_USER: user | ||
| POSTGRES_PASSWORD: password | ||
| POSTGRES_DB: starshop |
There was a problem hiding this comment.
Security risk: hardcoded credentials in version control.
The PostgreSQL credentials are hardcoded with weak values (user/password). This poses a security risk if the compose file is used in non-local environments or committed to public repositories.
Apply this diff to use environment variables with safe defaults:
environment:
- POSTGRES_USER: user
- POSTGRES_PASSWORD: password
- POSTGRES_DB: starshop
+ POSTGRES_USER: ${POSTGRES_USER:-user}
+ POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-password}
+ POSTGRES_DB: ${POSTGRES_DB:-starshop}Then create a .env.example file with placeholders and add .env to .gitignore.
📝 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.
| POSTGRES_USER: user | |
| POSTGRES_PASSWORD: password | |
| POSTGRES_DB: starshop | |
| environment: | |
| - POSTGRES_USER: user | |
| - POSTGRES_PASSWORD: password | |
| POSTGRES_USER: ${POSTGRES_USER:-user} | |
| POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-password} | |
| POSTGRES_DB: ${POSTGRES_DB:-starshop} |
🤖 Prompt for AI Agents
In docker-compose.yml around lines 7 to 9, the PostgreSQL credentials are
hardcoded (POSTGRES_USER/POSTGRES_PASSWORD/POSTGRES_DB) which is a security
risk; change these to reference environment variables (e.g., POSTGRES_USER,
POSTGRES_PASSWORD, POSTGRES_DB) with safe defaults using parameter expansion,
update any docs to instruct using a .env file, add a new .env.example with
placeholder values for these variables (do not include real secrets), and add
.env to .gitignore so runtime secrets are not committed to version control.
| Milestone, | ||
| ], | ||
| synchronize: false, | ||
| synchronize: process.env.NODE_ENV !== 'production', |
There was a problem hiding this comment.
Critical: Dynamic synchronize poses data loss risk.
Similar to the issue in src/config/index.ts, enabling synchronize: process.env.NODE_ENV !== 'production' will cause TypeORM to automatically alter the database schema in all non-production environments, risking data loss and making it difficult to test migrations.
This setting should be false in all environments, with explicit migrations handling schema changes:
- synchronize: process.env.NODE_ENV !== 'production',
+ synchronize: false,If this change was intentional, ensure:
- All developers understand the risk
- Staging uses
NODE_ENV=productionor a separate config - Documentation warns about potential data loss
- Migration strategy is planned before production deployment
📝 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.
| synchronize: process.env.NODE_ENV !== 'production', | |
| synchronize: false, |
🤖 Prompt for AI Agents
In src/app.module.ts around line 67, the TypeORM config uses synchronize:
process.env.NODE_ENV !== 'production', which risks automatic schema changes and
data loss; change this to always set synchronize: false and ensure migrations
are applied explicitly (use TypeORM migrations runner or your migration
service), update any environment/config logic to drive migrations instead of
schema sync, and add/adjust docs and dev onboarding to explain that schema
changes must be performed via migrations and how to run them (ensure staging
uses safe env/config).
|
|
||
| @Injectable() | ||
| export class ResponseInterceptor implements NestInterceptor { | ||
| intercept(context: ExecutionContext, next: CallHandler): Observable<any> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace Observable<any> with a specific type.
The return type Observable<any> violates coding guidelines. Define an interface for the response format.
As per coding guidelines.
Apply this diff to add a response interface:
+interface StandardResponse<T = any> {
+ success: boolean;
+ data?: T;
+ token?: string;
+ message?: string;
+}
+
@Injectable()
export class ResponseInterceptor implements NestInterceptor {
- intercept(context: ExecutionContext, next: CallHandler): Observable<any> {
+ intercept(context: ExecutionContext, next: CallHandler): Observable<StandardResponse> {📝 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.
| intercept(context: ExecutionContext, next: CallHandler): Observable<any> { | |
| import { Injectable, NestInterceptor, ExecutionContext, CallHandler } from '@nestjs/common'; | |
| import { Observable } from 'rxjs'; | |
| interface StandardResponse<T = any> { | |
| success: boolean; | |
| data?: T; | |
| token?: string; | |
| message?: string; | |
| } | |
| @Injectable() | |
| export class ResponseInterceptor implements NestInterceptor { | |
| intercept(context: ExecutionContext, next: CallHandler): Observable<StandardResponse> { | |
| // existing implementation... | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/common/interceptors/response.interceptor.ts around line 13, the method
signature uses the generic Observable<any>; define a concrete response interface
(e.g., a Response<T> with fields like data, status, message or whatever the
project standard requires) in this file or a shared types file, import it, and
replace Observable<any> with Observable<Response<any>> or make the interceptor
generic (Observable<Response<T>>) if appropriate; update the intercept
implementation/return mapping to build and return objects matching the new
interface and add any necessary imports/exports.
| password: process.env.DB_PASSWORD || 'password', | ||
| name: process.env.DB_DATABASE || 'starshop', | ||
| synchronize: false, | ||
| synchronize: process.env.NODE_ENV !== 'production', |
There was a problem hiding this comment.
Critical: Dynamic synchronize in non-production poses data loss risk.
Changing synchronize from false to process.env.NODE_ENV !== 'production' enables TypeORM auto-synchronization in development, staging, and any non-production environment. This will automatically alter the database schema on application startup to match entity definitions, which can lead to:
- Data loss if columns are removed or renamed
- Unintended schema changes in shared development/staging databases
- Difficulty reproducing production-like conditions in staging
Consider using explicit database migrations instead of auto-sync, even in development:
- synchronize: process.env.NODE_ENV !== 'production',
+ synchronize: false,If auto-sync is intentional for local development only, document this clearly and ensure staging environments use NODE_ENV=production or have a separate configuration.
📝 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.
| synchronize: process.env.NODE_ENV !== 'production', | |
| synchronize: false, |
🤖 Prompt for AI Agents
In src/config/index.ts around line 10, the synchronize setting is currently set
to process.env.NODE_ENV !== 'production', which enables TypeORM auto-schema sync
in non-production and risks data loss; change this to a safe default
(synchronize: false), remove auto-sync for staging, and instead drive schema
changes with explicit migrations; if you still want auto-sync for local dev
only, make it conditional on a dedicated env var (e.g., TYPEORM_SYNC=true) and
document that local-only behavior and ensure staging uses NODE_ENV=production or
TYPEORM_SYNC unset.
| export interface AttributeValueDTO { | ||
| attributeId: string; // UUID or numeric depending on schema | ||
| valueId: string; // references attribute value entity | ||
| value?: string; // optional human-readable value | ||
| } |
There was a problem hiding this comment.
ID types inconsistent with PR migration to numeric IDs.
The comment mentions "UUID or numeric depending on schema," but attributeId and valueId are typed as string. According to the PR objectives, IDs are being migrated from UUID/string to numeric across DTOs and entities. These fields should be typed as number to align with the migration.
Apply this diff to fix the type inconsistency:
export interface AttributeValueDTO {
- attributeId: string; // UUID or numeric depending on schema
- valueId: string; // references attribute value entity
+ attributeId: number;
+ valueId: number;
value?: string; // optional human-readable value
}📝 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.
| export interface AttributeValueDTO { | |
| attributeId: string; // UUID or numeric depending on schema | |
| valueId: string; // references attribute value entity | |
| value?: string; // optional human-readable value | |
| } | |
| export interface AttributeValueDTO { | |
| attributeId: number; | |
| valueId: number; | |
| value?: string; // optional human-readable value | |
| } |
🤖 Prompt for AI Agents
In src/dtos/ProductVariantDTO.ts around lines 1 to 5, the AttributeValueDTO
fields attributeId and valueId are typed as string while the project is
migrating IDs to numeric; change attributeId and valueId types from string to
number, keep value as optional string, and update any related
imports/usages/tests to accept numeric IDs to maintain type consistency across
DTOs and entities.
| const userId = Number(req.user.id); | ||
| if (!userId) { | ||
| throw new BadRequestError('User ID is required'); | ||
| } |
There was a problem hiding this comment.
Incorrect validation logic for numeric user ID.
Using Number() coercion followed by a falsy check is problematic:
Number("0")returns0, which is falsy, so a valid user ID of0would incorrectly throw "User ID is required"Number("invalid")returnsNaN, which is truthy, so invalid input would pass this check and cause issues downstream
Apply this diff to properly validate the numeric ID:
- const userId = Number(req.user.id);
- if (!userId) {
+ const userId = Number(req.user.id);
+ if (isNaN(userId)) {
throw new BadRequestError('User ID is required');
}📝 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.
| const userId = Number(req.user.id); | |
| if (!userId) { | |
| throw new BadRequestError('User ID is required'); | |
| } | |
| const userId = Number(req.user.id); | |
| if (isNaN(userId)) { | |
| throw new BadRequestError('User ID is required'); | |
| } |
🤖 Prompt for AI Agents
In src/modules/reviews/controllers/review.controller.ts around lines 16-19, the
current Number() coercion plus a falsy check wrongly rejects valid 0 IDs and
lets NaN pass; replace it with explicit numeric validation: convert req.user.id
to a number (or use parseInt) and then check Number.isInteger(userId) (and
optionally userId >= 0) or use Number.isFinite and not NaN; if the check fails
throw BadRequestError('User ID is required' or 'Invalid user ID').
| const userId = Number(req.user.id); | ||
| if (!userId) { | ||
| throw new BadRequestError('User ID is required'); | ||
| } |
There was a problem hiding this comment.
Incorrect validation logic for numeric user ID (duplicate issue).
Same validation issue as in createReview: Number() coercion with falsy check will fail for user ID 0 and pass for NaN.
Apply this diff:
- const userId = Number(req.user.id);
- if (!userId) {
+ const userId = Number(req.user.id);
+ if (isNaN(userId)) {
throw new BadRequestError('User ID is required');
}🤖 Prompt for AI Agents
In src/modules/reviews/controllers/review.controller.ts around lines 81 to 84,
the code uses Number(req.user.id) with a falsy check which rejects valid userId
0 and may accept NaN; replace the validation with a proper numeric check such
as: convert to number, then verify it's a finite integer (e.g.,
Number.isInteger(userId) or !Number.isNaN(userId) && isFinite(userId)) and only
throw BadRequestError if the check fails, so 0 is allowed and non-numeric values
are rejected.
| @PrimaryGeneratedColumn('uuid') | ||
| id: string; | ||
|
|
||
| @Column({ name: 'userId', type: 'uuid' }) | ||
| userId: string; | ||
| @Column({ name: 'userId' }) | ||
| userId: number; |
There was a problem hiding this comment.
Inconsistent primary key strategy: UUID for Review but numeric foreign keys.
The Review entity uses @PrimaryGeneratedColumn('uuid') for its own id (line 16) but now references numeric userId (line 20). This mixed approach can complicate:
- Indexing and join performance (UUID vs numeric)
- API consistency (some IDs are UUIDs, others are numbers)
- Migration complexity
Consider standardizing on either all numeric IDs or all UUIDs across entities for consistency.
🤖 Prompt for AI Agents
In src/modules/reviews/entities/review.entity.ts around lines 16 to 20, the
Review entity currently uses a UUID primary key but a numeric userId foreign key
which creates inconsistent ID strategies; fix by choosing one ID strategy and
making types consistent: either switch the Review.id to a numeric
@PrimaryGeneratedColumn() to match numeric userId, or change userId to a UUID
string (e.g., @Column({ type: 'uuid', name: 'userId' }) userId: string) and
update the relation decorator to reference the User entity's uuid PK. After
changing types, update the TypeORM relation definitions, adjust any
DTOs/services/controllers that assume number vs string, regenerate or add DB
migrations for the altered column type, and run tests to ensure joins and APIs
use the consistent ID format.
| @Column({ name: 'userId' }) | ||
| userId: number; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify database migration for userId type change and unique constraint.
The userId column type changed from UUID to numeric, which impacts:
- The existing
@Unique(['userId', 'productId'])constraint (line 14) must be dropped and recreated - Existing foreign key relationships need updating
- Data migration is required if the table contains records
Run the following script to check for related migrations and verify entity key types:
🏁 Script executed:
#!/bin/bash
# Description: Verify migration files and entity key types for reviews
# Check for review-related migration files
echo "=== Review Migration Files ==="
fd -e ts -e js migration | xargs rg -l "review" -i
# Verify User and Product primary key types
echo -e "\n=== User Entity Primary Key ==="
rg -A2 "class User" src/modules/users/entities/user.entity.ts | rg -A1 "@PrimaryGeneratedColumn"
echo -e "\n=== Product Entity Primary Key ==="
rg -A2 "class Product" src/modules/products/entities/product.entity.ts | rg -A1 "@PrimaryGeneratedColumn"Length of output: 183
Add missing DB migration for review entity type changes and constraints
- Create a migration to alter
review.userIdandreview.productIdfromUUIDtointeger. - Drop and recreate the
UNIQUE(userId, productId)index. - Update foreign key definitions to reference the numeric
users.idandproducts.id. - Include data migration logic to convert existing UUID references to their integer counterparts.
| const currentUserId = req.user?.id; | ||
| const currentUserRole = req.user?.role?.[0]; | ||
|
|
||
| // Check if user is updating their own profile or is admin | ||
| if (walletAddress !== currentUserWalletAddress && currentUserRole !== 'admin') { | ||
| if (userId !== currentUserId && currentUserRole !== 'admin') { | ||
| throw new UnauthorizedError('You can only update your own profile'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify req.user.id type consistency.
The code compares userId (number from ParseIntPipe) with currentUserId (from req.user?.id). Ensure that the JWT payload and request typing define req.user.id as number to prevent type coercion issues in the strict equality check.
Run the following script to check the AuthRequest interface and user type definitions:
🏁 Script executed:
#!/bin/bash
# Description: Verify req.user.id is typed as number in AuthRequest/JWT payload
# Search for AuthRequest interface definition
ast-grep --pattern $'interface AuthRequest {
$$$
}'
# Search for Request user property extensions
rg -nP --type=ts -C5 'interface.*Request|declare.*Express' src/Length of output: 8210
Convert req.user.id to a number before strict comparison
req.user.id is typed as string | number (and in middleware as string), so comparing it directly to the numeric userId will always fail. Cast it to a number (e.g.
const currentUserId = Number(req.user?.id);) or enforce a numeric id in your JWT payload.
🤖 Prompt for AI Agents
In src/modules/users/controllers/user.controller.ts around lines 109 to 115, the
code compares numeric userId to req.user.id which can be a string, so the strict
comparison will fail; convert req.user.id to a number before comparing (e.g.
assign currentUserId = Number(req.user?.id)), validate that conversion (handle
NaN by treating as unauthorized or throwing a BadRequest/UnauthorizedError),
then use the numeric currentUserId in the existing ownership/admin check so the
comparison works correctly.
1 participant
Summary by CodeRabbit
Refactor
Documentation
Chores