fix: resolve linter warnings and improve TypeScript types#196
fix: resolve linter warnings and improve TypeScript types#196respp wants to merge 1 commit intoStarShopCr:mainfrom
Conversation
- Replace 'any' types with specific TypeScript types (Record<string, unknown>, etc.) - Add explicit return types to functions missing them - Fix test mocks and jest spies with proper typing - Improve middleware and decorator type safety - Resolve 56 warnings (from 245 to 189 - 23% reduction) - Maintain 0 errors for clean codebase Addresses GitHub issue StarShopCr#176: Linter errors preventing clean contributions
WalkthroughWidespread TypeScript type tightening (any→unknown), explicit method return types, and refined controller response shapes. Escrow: removed fund endpoint, simplified DTOs, removed repository/blockchain dependencies, added EscrowModule export, and changed BlockchainService.fund signature. Offers: added wallet-ownership validations and refactored confirmPurchase into a transactional flow. Numerous test typings updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor B as Buyer
participant API as OffersController
participant S as OffersService
participant V as Ownership Validators
participant DB as DataSource (Transaction)
B->>API: POST /offers/:id/confirm-purchase
API->>S: confirmPurchase(offerId, user)
S->>V: validateBuyerWalletOwnership(user, offerId)
V-->>S: ok or throws
S->>DB: beginTransaction()
DB->>S: transaction manager
S->>DB: mark offer as purchased
alt BuyerRequest linked
S->>DB: update BuyerRequest status
end
S->>DB: commit
S-->>API: result
API-->>B: 200 { success, data }
rect rgba(255, 245, 230, 0.6)
note right of V: New precondition checks<br/>(wallet ownership)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (18)
src/modules/seller/services/seller.service.ts (1)
92-92: Critical: Method signature mismatch causes compilation error.Line 92 calls
extractPayoutWalletFromXdr(submitRegisterDto.signedXdr)with an argument, but the method signature at line 140 was changed to accept no parameters. This will cause a TypeScript compilation error.Apply this diff to fix the call site:
- const mockPayoutWallet = this.extractPayoutWalletFromXdr(submitRegisterDto.signedXdr); + const mockPayoutWallet = this.extractPayoutWalletFromXdr();Note: Since this is a mock implementation that returns a hardcoded value and doesn't use the
signedXdrparameter anyway, removing the argument is appropriate. However, when implementing the actual Soroban integration (as indicated by the TODO comments), you'll need to revisit this to properly extract the payout wallet from the signed XDR.src/modules/buyer-requests/tests/buyer-requests.service.spec.ts (1)
69-69: Critical: Undefined variablerepository.Line 69 assigns to
repository, but this variable is not declared in the test suite scope. The AI summary mentions "Removed a local Jest repository mock declaration," but the assignment and usage remain throughout the test file.Add the missing variable declaration after line 10:
describe('BuyerRequestsService', () => { let service: BuyerRequestsService; + let repository: jest.Mocked<Repository<BuyerRequest>>; const mockQueryBuilder = {src/modules/escrow/examples/offers-escrow-integration.example.ts (1)
51-51: Replace console.log with NestJS Logger.The coding guidelines require using proper logging (e.g., NestJS Logger) instead of
console.logfor all files matchingsrc/**/*.ts. Even though this is an example file, it should demonstrate best practices.As per coding guidelines.
Apply this pattern throughout the file (also applies to lines 53, 80, 127):
+import { Injectable, Logger } from '@nestjs/common'; -import { Injectable } from '@nestjs/common';+ private readonly logger = new Logger(OffersEscrowIntegrationService.name); + constructor(private readonly escrowService: EscrowService) {}- console.log(`Escrow account created for offer ${offerId} with ${defaultMilestones.length} milestones`); + this.logger.log(`Escrow account created for offer ${offerId} with ${defaultMilestones.length} milestones`);- console.error('Failed to create escrow account:', error); + this.logger.error('Failed to create escrow account:', error);src/modules/escrow/services/escrow.service.ts (2)
87-87: Replace console.log with proper logging.The coding guidelines prohibit
console.log. Use NestJS Logger instead.Apply this diff:
+import { Injectable, NotFoundException, ForbiddenException, BadRequestException, Logger } from '@nestjs/common'; + @Injectable() export class EscrowService { + private readonly logger = new Logger(EscrowService.name); + constructor(- console.log(`Funds released: ${milestone.amount} to seller ${escrowAccount.sellerId} for milestone ${milestoneId}`); + this.logger.log(`Funds released: ${milestone.amount} to seller ${escrowAccount.sellerId} for milestone ${milestoneId}`);As per coding guidelines.
301-326: Fix missing injections and updatefundEscrowcall
- Constructor only injects
escrowRepository,milestoneRepository, andofferRepository—add@InjectRepository(Transaction) private txRepository: Repository<Transaction>andprivate blockchain: BlockchainService(or rename to match existing property) sothis.txRepoandthis.blockchainresolve.- The call
this.blockchain.fund(amountStr, dto.signer, id)no longer matchesasync fund(): Promise<string>—either remove the arguments or restore the previous signature; if the entire fund flow is deprecated, removefundEscrow.- Adjust or remove the related tests in
escrow.service.spec.tsto align with these changes.src/modules/escrow/services/blockchain.service.ts (1)
7-11: Fix blockchain.fund signature mismatchIn
src/modules/escrow/services/blockchain.service.tsthefund()method now takes no parameters, but insrc/modules/escrow/services/escrow.service.ts(line 310) it’s called with three arguments (amountStr, dto.signer, id). Align the method signature and its call—either restore the parameters onfund(...)or update the call at line 310 to use no arguments.src/modules/attributes/services/attributes.service.spec.ts (1)
46-47: Remove undefined variable references.These lines reference
attributeRepositoryandattributeValueRepositoryvariables that are not declared anywhere in the test file, causing compilation errors. The repositories are already properly mocked viagetRepositoryTokenin the module providers (lines 34-41), and these variables are never used in any test assertions.Apply this diff to remove the unused variable assignments:
service = module.get<AttributeService>(AttributeService) - attributeRepository = module.get<Repository<Attribute>>(getRepositoryToken(Attribute)) - attributeValueRepository = module.get<Repository<AttributeValue>>(getRepositoryToken(AttributeValue)) })src/modules/coupons/tests/coupon.integration.spec.ts (1)
77-95: Incorrect return type annotation.The function is annotated to return
Promise<Coupon>, but it actually returns a supertestTestobject (which resolves to aResponse, not aCoupon). While the function calls are currently not using the return value, this type annotation is misleading and could cause issues if the return value is used in the future.Apply this diff to fix the return type:
- const createTestCoupon = async (code: string): Promise<Coupon> => { + const createTestCoupon = async (code: string): Promise<void> => { const createCouponDto = { code, type: CouponType.PERCENTAGE, value: 10, minPurchaseAmount: 50, maxDiscountAmount: 100, startDate: new Date(), endDate: new Date(Date.now() + 86400000), usageLimit: 100, created_by: 'admin1', }; - return request(app.getHttpServer()) + await request(app.getHttpServer()) .post('/coupons') .set('Authorization', `Bearer ${authToken}`) .send(createCouponDto) .expect(201); };src/modules/auth/middleware/jwt-auth.middleware.ts (2)
45-51: Type assertion loses important type information.Casting the user object to
Record<string, unknown>is overly broad and loses all the specific type information about the properties being set (id, email, walletAddress, name, role). This makes downstream code that accessesreq.user.id,req.user.email, etc., lose type checking.Instead of casting to
Record<string, unknown>, properly typereq.userby:
- Extending the Express Request type in
src/types/express.d.ts, or- Using the
AuthenticatedRequestinterface consistentlySince the middleware imports from
src/types/role, the Express type augmentation should be defined there or in a related types file. Example:// In src/types/express.d.ts or similar declare global { namespace Express { interface Request { user?: { id: number; email: string; walletAddress: string; name?: string; role: Role[]; }; } } }Then remove the type assertion entirely:
- req.user = { + req.user = { id: user.id, email: user.email, walletAddress: user.walletAddress, name: user.name, role: user.userRoles?.map((ur) => ur.role.name as Role) || [decoded.role as Role], - } as Record<string, unknown>; + };
45-51: Type assertion defeats type safety.Asserting the user object as
Record<string, unknown>loses all type information and creates a mismatch with the expectedAuthenticatedRequest.userinterface. The constructed object has a well-defined shape withid,walletAddress,name, androle: Role[], but casting toRecord<string, unknown>erases this information.This defeats the purpose of TypeScript's type checking and may mask type errors in downstream code that expects the proper user shape.
Remove the type assertion and let TypeScript infer the correct type, or if there's a type mismatch, fix it properly:
req.user = { id: user.id, email: user.email, walletAddress: user.walletAddress, name: user.name, role: user.userRoles?.map((ur) => ur.role.name as Role) || [decoded.role as Role], - } as Record<string, unknown>; + };If removing the assertion causes type errors, those errors indicate real type mismatches that should be resolved by:
- Ensuring
req.usertype declaration matches this object shape- Or defining a proper interface for this user object and using it consistently
src/cache/interceptors/cache.interceptor.ts (1)
54-66: Type safety issue: accessing properties onunknowntyped parameters.The
metadataandrequestparameters are typed asunknown, but lines 55-62 directly access properties without type guards or type assertions. TypeScript should report errors for these property accesses.Apply type assertions or define interfaces to safely access properties:
- private generateCacheKey(metadata: unknown, request: unknown): string { - const { key, entity, action } = metadata; + private generateCacheKey(metadata: unknown, request: unknown): string { + const { key, entity, action } = metadata as { key?: string; entity?: string; action?: string }; // Extract parameters from request const params = { - query: request.query, - params: request.params, - body: request.body, - user: request.user?.id, // Include user ID if authenticated + query: (request as any).query, + params: (request as any).params, + body: (request as any).body, + user: (request as any).user?.id, // Include user ID if authenticated };Alternatively, define proper interfaces for better type safety:
interface CacheMetadata { key?: string; entity?: string; action?: string; } interface CacheRequest { query?: unknown; params?: unknown; body?: unknown; user?: { id: unknown }; } private generateCacheKey(metadata: unknown, request: unknown): string { const { key, entity, action } = metadata as CacheMetadata; const req = request as CacheRequest; const params = { query: req.query, params: req.params, body: req.body, user: req.user?.id, }; return this.cacheService['generateKey'](entity || 'default', action || key, params); }src/modules/auth/dto/auth.dto.ts (4)
136-136: Replaceanytype withunknownor a specific interface.The
buyerDataproperty is typed asany, which violates the coding guideline: "Do not use the any type; use unknown or specific types instead."Consider defining a specific interface or using
unknown:@IsRoleSpecificData({ message: 'buyerData is only allowed for buyers' }) @IsObject() @IsOptional() - buyerData?: any; + buyerData?: unknown;Or better, define a proper interface:
interface BuyerData { preferences?: string[]; // Add other buyer-specific fields } // Then use: buyerData?: BuyerData;As per coding guidelines.
145-145: Replaceanytype withunknownor a specific interface.The
sellerDataproperty is typed asany, which violates the coding guideline: "Do not use the any type; use unknown or specific types instead."Consider defining a specific interface or using
unknown:@IsRoleSpecificData({ message: 'sellerData is only allowed for sellers' }) @IsObject() @IsOptional() - sellerData?: any; + sellerData?: unknown;Or better, define a proper interface:
interface SellerData { businessName?: string; categories?: string[]; rating?: number; // Add other seller-specific fields } // Then use: sellerData?: SellerData;As per coding guidelines.
192-192: Replaceanytype withunknownor a specific interface.The
buyerDataproperty inUpdateUserDtois typed asany, which violates the coding guideline: "Do not use the any type; use unknown or specific types instead."Apply the same fix as suggested for
RegisterUserDto.buyerDatato maintain consistency.As per coding guidelines.
201-201: Replaceanytype withunknownor a specific interface.The
sellerDataproperty inUpdateUserDtois typed asany, which violates the coding guideline: "Do not use the any type; use unknown or specific types instead."Apply the same fix as suggested for
RegisterUserDto.sellerDatato maintain consistency.As per coding guidelines.
src/modules/auth/guards/roles.guard.ts (1)
29-43: Return type mismatch in error paths.The middleware declares a
voidreturn type, but lines 33 and 37 usereturn res.status(...).json(...). Sinceres.json()returnsResponse<unknown>, this creates a type mismatch.Apply this pattern to fix the type mismatch:
if (!user || !user.role) { - return res.status(403).json({ success: false, message: 'Access denied. Role required.' }); + res.status(403).json({ success: false, message: 'Access denied. Role required.' }); + return; } if (!user.role.includes(requiredRole)) { - return res - .status(403) - .json({ success: false, message: `Access denied. ${requiredRole} role required.` }); + res.status(403).json({ success: false, message: `Access denied. ${requiredRole} role required.` }); + return; }src/middleware/validation.middleware.ts (1)
31-40: Type inconsistency:voidreturn with response object.The function is typed to return
void(line 31), but line 34 returns the result ofres.status(400).json(...), which is aResponseobject. This inconsistency can cause type-checking confusion.Consider using an explicit
voidcast or restructuring the early return to avoid returning the response object.Apply this diff:
const { id } = req.params; if (!id || isNaN(Number(id))) { - return res.status(400).json({ + res.status(400).json({ status: 'error', message: 'Invalid ID parameter', }); + return; }src/modules/products/controllers/product.controller.ts (1)
28-34: Add explicit return type to getAllProducts method.The method lacks an explicit return type, which violates the coding guideline requiring explicit return types for all functions.
Apply this diff to add the return type:
async getAllProducts( @Query('page') page: string = '1', @Query('limit') limit: string = '10', @Query('search') search?: string, @Query('category') category?: string, @Query('sort') sort?: string - ) { + ): Promise<{ success: boolean; data: unknown }> {Note: Replace
unknownwith the actual return type fromProductService.getAllProducts()for better type safety.As per coding guidelines
🧹 Nitpick comments (22)
src/modules/stores/services/store.service.ts (2)
20-58: Good type improvement, but consider a more specific interface.Changing from
anytoRecord<string, unknown>improves type safety. However, since you're accessing specific properties (businessDescription,categories,phone,website), consider defining an interface forsellerDatato provide better type checking and IDE support.Example interface:
interface SellerRegistrationData { businessDescription?: string; categories?: string[]; phone?: string; website?: string; }Then update the signature:
- async createDefaultStore(sellerId: number, sellerData: Record<string, unknown>): Promise<Store> { + async createDefaultStore(sellerId: number, sellerData: SellerRegistrationData): Promise<Store> {
213-233: Define a specific interface for the return type.While
Record<string, unknown>is better thanany, the return shape is actually fixed and known. Define a specific interface to provide better type safety for consumers of this method.interface StoreStats { id: number; name: string; status: StoreStatus; rating: number; reviewCount: number; createdAt: Date; verifiedAt: Date | null; }Then update the signature:
- async getStoreStats(storeId: number, sellerId: number): Promise<Record<string, unknown>> { + async getStoreStats(storeId: number, sellerId: number): Promise<StoreStats> {src/modules/files/middlewares/file-upload.middleware.ts (2)
25-32: Consider using Express error handling pattern.The middleware currently sends an error response directly when
uploadMiddlewarefails. In Express/NestJS applications, it's typically better to pass errors to the next error-handling middleware usingnext(err)so that centralized error handlers can process them consistently.Apply this diff to align with Express error handling conventions:
uploadMiddleware(req, res, (err): void => { if (err) { - res.status(400).json({ - status: 'error', - message: err.message, - }); - return; + next(err); + return; }Alternatively, if you need to preserve the 400 status and specific response format, you could wrap the error:
if (err) { const fileError = new Error(err.message); (fileError as any).status = 400; next(fileError); return; }
58-67: Consider consistent error handling pattern.Similar to the
uploadFilemiddleware, this middleware sends error responses directly. For consistency with Express/NestJS error handling conventions, consider passing errors tonext()so centralized error handlers can process them uniformly.Apply this diff:
export const validateFileExists = (req: Request, res: Response, next: NextFunction): void => { if (!req.file) { - res.status(400).json({ - status: 'error', - message: 'No file uploaded', - }); - return; + const error = new Error('No file uploaded'); + (error as any).status = 400; + next(error); + return; } next(); };src/modules/buyer-requests/tests/buyer-requests.service.spec.ts (1)
49-49: Good type safety improvement, but consider using a proper User type.Changing
as anytoas Record<string, unknown>aligns with the coding guidelines. However, since this mock has a specific shape withid,name, andwalletAddress, consider importing and using the actualUserentity type or creating a typed test fixture for better type safety.If a User entity exists, apply this pattern:
- } as Record<string, unknown>, + } as Partial<User>,src/modules/stores/dto/store.dto.ts (2)
25-30: Consider extracting inline object types to interfaces.Multiple inline object type definitions appear throughout the DTOs:
socialMediaobject (lines 25-30)coordinatesobject (lines 62-65)- Business hours day objects repeated 7 times (lines 72, 77, 82, 87, 92, 97, 102)
Extracting these to dedicated interfaces would improve type reusability and maintainability.
Example refactor:
+interface SocialMediaLinks { + facebook?: string; + twitter?: string; + instagram?: string; + linkedin?: string; +} + +interface Coordinates { + latitude?: number; + longitude?: number; +} + +interface DayHours { + open: string; + close: string; + closed: boolean; +} + export class ContactInfoDto { @ApiPropertyOptional({ description: 'Store phone number' }) @IsOptional() @IsString() phone?: string; @ApiPropertyOptional({ description: 'Store email address' }) @IsOptional() @IsString() email?: string; @ApiPropertyOptional({ description: 'Store website URL' }) @IsOptional() @IsUrl() website?: string; @ApiPropertyOptional({ description: 'Social media links' }) @IsOptional() @IsObject() - socialMedia?: { - facebook?: string; - twitter?: string; - instagram?: string; - linkedin?: string; - }; + socialMedia?: SocialMediaLinks; }Apply similar changes for
coordinatesand business hours objects.Also applies to: 62-65, 72-72, 77-77, 82-82, 87-87, 92-92, 97-97, 102-102
24-30: Consider strengthening validation for nested object structures.The
@IsObject()decorator only verifies that a value is an object, but doesn't validate its internal structure. For nested objects with specific shapes (socialMedia, coordinates, business hours), consider either:
- Extracting them to separate DTO classes and using
@ValidateNested()with@Type(), or- Creating custom validators that check the expected properties
This would prevent invalid nested structures from passing validation.
Example for coordinates:
export class CoordinatesDto { @IsOptional() @IsNumber() latitude?: number; @IsOptional() @IsNumber() longitude?: number; } export class AddressDto { // ... other properties ... @ApiPropertyOptional({ description: 'Geographic coordinates' }) @IsOptional() @ValidateNested() @Type(() => CoordinatesDto) coordinates?: CoordinatesDto; }Also applies to: 61-65, 71-72, 76-77, 81-82, 86-87, 91-92, 96-97, 101-102
src/modules/escrow/examples/offers-escrow-integration.example.ts (2)
63-83: Replaceanytypes with specific types.The coding guidelines prohibit the
anytype in favor ofunknownor specific types. This function has two issues:
- Return type
Promise<any>at line 67 should be specific (likely the return type ofescrowService.releaseFunds).- Type assertions
as anyat line 72 and 115 suggest a type mismatch that should be properly resolved.As per coding guidelines.
async handleMilestoneCompletion( milestoneId: string, sellerId: number, notes?: string, - ): Promise<any> { + ): Promise<unknown> { try { const result = await this.escrowService.releaseFunds( { milestoneId, - type: 'milestone' as any, + type: 'milestone', notes, }, sellerId,Note: If the DTO type definition for
releaseFundsdoesn't accept'milestone'as a valid type, update the DTO definition rather than usingas any.
88-95: Consider typing thedatafield more specifically.The return type includes
data?: any, which could be more specific. Consider using the actual return type fromescrowService.releaseFundsorunknownif the type varies.As per coding guidelines.
- ): Promise<{ approved: boolean; released?: boolean; data?: any }> { + ): Promise<{ approved: boolean; released?: boolean; data?: unknown }> {src/modules/coupons/tests/coupon.controller.spec.ts (2)
84-84: Use specificCoupontype instead ofRecord<string, unknown>.While replacing
anywithRecord<string, unknown>improves type safety, using the specificCoupontype would be more precise and provide better type checking. ThemockCouponobject already has a well-defined Coupon shape.Apply this diff:
- jest.spyOn(service, 'createCoupon').mockResolvedValue(mockCoupon as Record<string, unknown>); + jest.spyOn(service, 'createCoupon').mockResolvedValue(mockCoupon as unknown as Coupon);
118-118: Use specificCoupontype instead ofRecord<string, unknown>.Similarly, use the specific
Coupontype here for better type precision.Apply this diff:
- jest.spyOn(service, 'validateCoupon').mockResolvedValue(mockCoupon as Record<string, unknown>); + jest.spyOn(service, 'validateCoupon').mockResolvedValue(mockCoupon as unknown as Coupon);src/modules/disputes/tests/dispute.service.spec.ts (2)
7-13: Consider using a more specific mock repository type.While
Record<string, unknown>is an improvement overany, a more precise type likePartial<Repository<OrderItem>>would provide better type safety and IDE support for the repository methods.Apply this diff:
-const mockOrderItemRepo = (): Record<string, unknown> => ({ +const mockOrderItemRepo = (): Partial<Repository<OrderItem>> => ({ findOne: jest.fn(), save: jest.fn(), manager: { getRepository: jest.fn().mockReturnValue({ findOne: jest.fn() }), }, });
14-18: Consider using a more specific mock repository type.Similarly, use
Partial<Repository<Dispute>>for better type precision.Apply this diff:
-const mockDisputeRepo = (): Record<string, unknown> => ({ +const mockDisputeRepo = (): Partial<Repository<Dispute>> => ({ findOne: jest.fn(), create: jest.fn(), save: jest.fn(), });src/modules/disputes/controllers/dispute.controller.ts (1)
12-19: Explicit return type added; consider creating a specific response DTO.The explicit return type
Promise<Record<string, unknown>>follows the coding guidelines. However, if the dispute service returns a consistent shape, consider creating a specific response DTO interface for better type safety and API documentation.For example, you could create:
interface DisputeResponseDto { id: string; // other properties returned by startDispute }Then update the return type:
async startDispute( @Body('orderItemId') orderItemId: string, @Body('reason') reason: string, @Req() req: AuthenticatedRequest ): Promise<DisputeResponseDto> { const buyer = req.user; return this.disputeService.startDispute(orderItemId, buyer, reason); }src/cache/tests/cache.service.spec.ts (1)
8-9: Consider more specific mock types for better type safety.While
Record<string, unknown>is an improvement overany, it doesn't capture the structure of the mocks and may lose Jest's type inference for mock assertions. Consider defining explicit mock types:- let mockCacheManager: Record<string, unknown>; - let mockConfigService: Record<string, unknown>; + let mockCacheManager: { + get: jest.Mock; + set: jest.Mock; + del: jest.Mock; + reset: jest.Mock; + }; + let mockConfigService: { + get: jest.Mock; + };This provides better IDE autocomplete and type-checking for mock methods throughout the test file while maintaining the same runtime behavior.
src/middleware/validation.middleware.ts (1)
22-22: Remove or translate Spanish comments.The Spanish comments (lines 22, 26) should be removed or translated to English for consistency across the codebase.
Apply this diff:
- return; // ✅ Solo return vacío + return; // Early return on validation failure- next(); // ✅ Deja continuar la cadena de middleware + next(); // Continue to next middlewareAlso applies to: 26-26
src/common/interceptors/response.interceptor.ts (1)
14-76: Type refinement improves safety, but consider edge cases.The return type
Observable<Record<string, unknown>>correctly reflects the interceptor's formatting behavior. The logic properly wraps raw responses in a standard format with{ success: true, data: ... }.However, the type guard at line 28 (
typeof data === 'object') will be true for arrays andnull. While the current logic handles this correctly (arrays and null will be wrapped as data), consider adding a more precise check if you want to distinguish between object literals and other object types.If you want stricter checks:
- if (data && typeof data === 'object' && 'success' in data) { + if (data && typeof data === 'object' && !Array.isArray(data) && 'success' in data) {src/dtos/UserDTO.ts (1)
92-92: Consider defining explicit interfaces for buyer and seller data.The properties
buyerDataandsellerDataare now typed asRecord<string, unknown>instead ofany, which improves type safety. However, if the structure of buyer and seller data is known (e.g., specific fields likepreferences,businessName,categoriesfrom the API examples), consider defining explicit interfaces:interface BuyerData { preferences?: string[]; // other known fields } interface SellerData { businessName?: string; categories?: string[]; // other known fields }This would provide better autocomplete, type checking, and documentation.
Also applies to: 101-101
src/cache/cache.service.ts (1)
157-169: LGTM! Type safety improved.The return type change from
Record<string, any>toRecord<string, unknown>aligns with the PR's typing improvements.For even stronger type safety, consider defining a specific interface for the return type:
+interface CacheStats { + prefix: string; + debugMode: boolean; + timestamp: string; +} + -async getStats(): Promise<Record<string, unknown>> { +async getStats(): Promise<CacheStats> {This would provide IntelliSense support and catch potential typos at compile time.
src/modules/products/controllers/product.controller.ts (3)
96-100: Extract authorization logic into a reusable guard.Authorization checks for product ownership are duplicated in
updateProduct(lines 96-100) anddeleteProduct(lines 126-130). This should be extracted into a reusable guard or decorator.Create a resource ownership guard:
// src/modules/shared/guards/product-ownership.guard.ts import { Injectable, CanActivate, ExecutionContext, ForbiddenException } from '@nestjs/common'; import { ProductService } from '../../products/services/product.service'; @Injectable() export class ProductOwnershipGuard implements CanActivate { constructor(private readonly productService: ProductService) {} async canActivate(context: ExecutionContext): Promise<boolean> { const request = context.switchToHttp().getRequest(); const userId = request.user?.id; const productId = parseInt(request.params.id); const product = await this.productService.getProductById(productId); if (product.sellerId !== userId) { throw new ForbiddenException('Not authorized to access this product'); } // Attach product to request to avoid re-fetching request.product = product; return true; } }Then simplify the controller methods:
@Put(':id') - @UseGuards(JwtAuthGuard, RolesGuard) + @UseGuards(JwtAuthGuard, RolesGuard, ProductOwnershipGuard) @Roles('seller', 'admin') async updateProduct(@Param('id') id: string, @Body() updateProductDto: UpdateProductDto, @Request() req: AuthenticatedRequest): Promise<{ success: boolean; data: Product }> { - const userId = req.user.id; - if (!userId) { - throw new BadRequestError('User not authenticated'); - } - - const product = await this.productService.getProductById(parseInt(id)); - - if (product.sellerId !== userId) { - throw new ForbiddenError('Not authorized to update this product'); - } - const updatedProduct = await this.productService.updateProduct(parseInt(id), updateProductDto); return { success: true, data: updatedProduct }; }
51-51: Add error handling forparseIntoperations.Multiple
parseIntcalls throughout the file (lines 36-37, 51, 78, 96, 103, 126, 132) lack error handling. If the input is not a valid number,parseIntreturnsNaN, which will cause unexpected behavior downstream.Consider using NestJS's built-in
ParseIntPipefor automatic validation and error handling:@Get(':id') @ApiOperation({ summary: 'Get product by ID' }) @ApiResponse({ status: 200, description: 'Product retrieved successfully' }) @ApiResponse({ status: 404, description: 'Product not found' }) - async getProductById(@Param('id') id: string): Promise<{ success: boolean; data: Product }> { + async getProductById(@Param('id', ParseIntPipe) id: number): Promise<{ success: boolean; data: Product }> { - const product = await this.productService.getProductById(parseInt(id)); + const product = await this.productService.getProductById(id); return { success: true, data: product }; }Apply similar changes to other methods that use
@Param('id')and importParseIntPipe:import { Controller, Get, Post, Put, Delete, Param, Body, Query, UseGuards, Request, + ParseIntPipe, } from '@nestjs/common';
62-65: Extract the authenticated user via a custom decorator and remove redundant manual checks
Manualreq.user?.idchecks after@UseGuards(JwtAuthGuard)are unnecessary—JwtAuthGuardalready throws on missing/invalid tokens and setsrequest.user. Define aCurrentUserdecorator insrc/modules/shared/decorators/current-user.decorator.tsand replace all@Request() req+ guard-check boilerplate with@CurrentUser() user.Example decorator:
import { createParamDecorator, ExecutionContext } from '@nestjs/common'; export const CurrentUser = createParamDecorator( (_: unknown, ctx: ExecutionContext) => ctx.switchToHttp().getRequest().user, );Usage before vs. after:
- async createProduct( - @Body() dto: CreateProductDto, - @Request() req: any, - ) { - const userId = req.user?.id; - if (!userId) throw new BadRequestError('User not authenticated'); + async createProduct( + @Body() dto: CreateProductDto, + @CurrentUser() user: { id: number; role: string }, + ) { // user.id is guaranteed const product = await this.productService.createProduct({ ...dto, - sellerId: Number(userId), + sellerId: user.id, });Apply the same refactor in
updateProductanddeleteProductto eliminate repeated guards checks.
📜 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 (57)
src/cache/cache.module.ts(1 hunks)src/cache/cache.service.ts(5 hunks)src/cache/controllers/cache.controller.ts(4 hunks)src/cache/decorators/cache.decorator.ts(3 hunks)src/cache/interceptors/cache.interceptor.ts(3 hunks)src/cache/tests/cache.service.spec.ts(1 hunks)src/common/decorators/api-response.decorator.ts(3 hunks)src/common/filters/http-exception.filter.ts(2 hunks)src/common/interceptors/response.interceptor.spec.ts(1 hunks)src/common/interceptors/response.interceptor.ts(2 hunks)src/dtos/ProductVariantDTO.ts(1 hunks)src/dtos/UserDTO.ts(3 hunks)src/middleware/async-handler.ts(1 hunks)src/middleware/async.middleware.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(2 hunks)src/modules/attributes/controllers/attributes.controller.ts(8 hunks)src/modules/attributes/services/attributes.service.spec.ts(1 hunks)src/modules/attributes/services/attributes.service.ts(1 hunks)src/modules/auth/decorators/roles.decorator.ts(1 hunks)src/modules/auth/decorators/seller-wallet-ownership.decorator.ts(1 hunks)src/modules/auth/decorators/wallet-ownership.decorator.ts(1 hunks)src/modules/auth/dto/auth.dto.ts(1 hunks)src/modules/auth/guards/roles.guard.ts(2 hunks)src/modules/auth/middleware/authorize-roles.middleware.ts(1 hunks)src/modules/auth/middleware/jwt-auth.middleware.ts(2 hunks)src/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.ts(1 hunks)src/modules/buyer-requests/tests/buyer-requests.service.spec.ts(1 hunks)src/modules/coupons/tests/coupon.controller.spec.ts(2 hunks)src/modules/coupons/tests/coupon.integration.spec.ts(1 hunks)src/modules/coupons/tests/coupon.service.spec.ts(6 hunks)src/modules/disputes/controllers/dispute.controller.ts(1 hunks)src/modules/disputes/tests/dispute.service.spec.ts(1 hunks)src/modules/escrow/controllers/escrow.controller.ts(0 hunks)src/modules/escrow/escrow.module.ts(1 hunks)src/modules/escrow/examples/offers-escrow-integration.example.ts(1 hunks)src/modules/escrow/services/blockchain.service.ts(1 hunks)src/modules/escrow/services/escrow.service.ts(4 hunks)src/modules/escrow/tests/escrow.integration.spec.ts(0 hunks)src/modules/escrow/tests/escrow.service.spec.ts(0 hunks)src/modules/escrows/services/escrow.service.spec.ts(1 hunks)src/modules/files/middlewares/file-upload.middleware.ts(1 hunks)src/modules/offers/services/offers.service.ts(0 hunks)src/modules/offers/tests/offer.service.spec.ts(0 hunks)src/modules/offers/tests/offers.controller.spec.ts(0 hunks)src/modules/offers/tests/offers.integration.spec.ts(0 hunks)src/modules/products/controllers/product.controller.ts(4 hunks)src/modules/seller/services/seller.service.ts(1 hunks)src/modules/seller/tests/seller.service.spec.ts(0 hunks)src/modules/stores/dto/store.dto.ts(1 hunks)src/modules/stores/entities/store.entity.ts(0 hunks)src/modules/stores/services/store.service.ts(2 hunks)src/modules/users/tests/user-update-api.spec.ts(0 hunks)src/modules/wishlist/tests/wishlist.service.spec.ts(4 hunks)
💤 Files with no reviewable changes (10)
- src/modules/seller/tests/seller.service.spec.ts
- src/modules/stores/entities/store.entity.ts
- src/modules/escrow/tests/escrow.integration.spec.ts
- src/modules/offers/tests/offers.integration.spec.ts
- src/modules/offers/tests/offer.service.spec.ts
- src/modules/escrow/tests/escrow.service.spec.ts
- src/modules/users/tests/user-update-api.spec.ts
- src/modules/offers/tests/offers.controller.spec.ts
- src/modules/escrow/controllers/escrow.controller.ts
- src/modules/offers/services/offers.service.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/common/interceptors/response.interceptor.tssrc/modules/disputes/controllers/dispute.controller.tssrc/modules/buyer-requests/tests/buyer-requests.service.spec.tssrc/modules/escrow/escrow.module.tssrc/cache/interceptors/cache.interceptor.tssrc/modules/auth/decorators/seller-wallet-ownership.decorator.tssrc/dtos/ProductVariantDTO.tssrc/modules/escrow/services/blockchain.service.tssrc/middleware/auth.middleware.tssrc/middleware/validateRequest.middleware.tssrc/middleware/validation.middleware.tssrc/common/decorators/api-response.decorator.tssrc/cache/controllers/cache.controller.tssrc/modules/coupons/tests/coupon.controller.spec.tssrc/modules/auth/guards/roles.guard.tssrc/modules/auth/decorators/roles.decorator.tssrc/modules/coupons/tests/coupon.service.spec.tssrc/modules/coupons/tests/coupon.integration.spec.tssrc/middleware/userValidation.middleware.tssrc/modules/auth/middleware/jwt-auth.middleware.tssrc/middleware/async-handler.tssrc/modules/stores/services/store.service.tssrc/modules/auth/decorators/wallet-ownership.decorator.tssrc/modules/files/middlewares/file-upload.middleware.tssrc/modules/seller/services/seller.service.tssrc/cache/decorators/cache.decorator.tssrc/cache/cache.module.tssrc/modules/escrow/examples/offers-escrow-integration.example.tssrc/dtos/UserDTO.tssrc/middleware/paramValidation.middleware.tssrc/common/interceptors/response.interceptor.spec.tssrc/modules/disputes/tests/dispute.service.spec.tssrc/modules/products/controllers/product.controller.tssrc/cache/tests/cache.service.spec.tssrc/modules/attributes/services/attributes.service.tssrc/middleware/async.middleware.tssrc/modules/auth/middleware/authorize-roles.middleware.tssrc/common/filters/http-exception.filter.tssrc/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.tssrc/cache/cache.service.tssrc/modules/wishlist/tests/wishlist.service.spec.tssrc/modules/stores/dto/store.dto.tssrc/modules/attributes/services/attributes.service.spec.tssrc/modules/auth/dto/auth.dto.tssrc/modules/escrow/services/escrow.service.tssrc/modules/escrows/services/escrow.service.spec.tssrc/modules/attributes/controllers/attributes.controller.ts
src/modules/**
📄 CodeRabbit inference engine (.cursorrules)
Follow NestJS module structure within src/modules/[module-name]/
Files:
src/modules/disputes/controllers/dispute.controller.tssrc/modules/buyer-requests/tests/buyer-requests.service.spec.tssrc/modules/escrow/escrow.module.tssrc/modules/auth/decorators/seller-wallet-ownership.decorator.tssrc/modules/escrow/services/blockchain.service.tssrc/modules/coupons/tests/coupon.controller.spec.tssrc/modules/auth/guards/roles.guard.tssrc/modules/auth/decorators/roles.decorator.tssrc/modules/coupons/tests/coupon.service.spec.tssrc/modules/coupons/tests/coupon.integration.spec.tssrc/modules/auth/middleware/jwt-auth.middleware.tssrc/modules/stores/services/store.service.tssrc/modules/auth/decorators/wallet-ownership.decorator.tssrc/modules/files/middlewares/file-upload.middleware.tssrc/modules/seller/services/seller.service.tssrc/modules/escrow/examples/offers-escrow-integration.example.tssrc/modules/disputes/tests/dispute.service.spec.tssrc/modules/products/controllers/product.controller.tssrc/modules/attributes/services/attributes.service.tssrc/modules/auth/middleware/authorize-roles.middleware.tssrc/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.tssrc/modules/wishlist/tests/wishlist.service.spec.tssrc/modules/stores/dto/store.dto.tssrc/modules/attributes/services/attributes.service.spec.tssrc/modules/auth/dto/auth.dto.tssrc/modules/escrow/services/escrow.service.tssrc/modules/escrows/services/escrow.service.spec.tssrc/modules/attributes/controllers/attributes.controller.ts
src/modules/**/controllers/**/*.controller.ts
📄 CodeRabbit inference engine (.cursorrules)
Handle HTTP in controllers only
Files:
src/modules/disputes/controllers/dispute.controller.tssrc/modules/products/controllers/product.controller.tssrc/modules/attributes/controllers/attributes.controller.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/buyer-requests/tests/buyer-requests.service.spec.tssrc/modules/coupons/tests/coupon.controller.spec.tssrc/modules/coupons/tests/coupon.service.spec.tssrc/modules/coupons/tests/coupon.integration.spec.tssrc/common/interceptors/response.interceptor.spec.tssrc/modules/disputes/tests/dispute.service.spec.tssrc/cache/tests/cache.service.spec.tssrc/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.tssrc/modules/wishlist/tests/wishlist.service.spec.tssrc/modules/attributes/services/attributes.service.spec.tssrc/modules/escrows/services/escrow.service.spec.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/buyer-requests/tests/buyer-requests.service.spec.tssrc/modules/coupons/tests/coupon.service.spec.tssrc/modules/disputes/tests/dispute.service.spec.tssrc/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.tssrc/modules/wishlist/tests/wishlist.service.spec.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/escrow/escrow.module.ts
src/modules/**/services/**/*.service.ts
📄 CodeRabbit inference engine (.cursorrules)
Place business logic in services
Files:
src/modules/escrow/services/blockchain.service.tssrc/modules/stores/services/store.service.tssrc/modules/seller/services/seller.service.tssrc/modules/attributes/services/attributes.service.tssrc/modules/escrow/services/escrow.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/coupons/tests/coupon.controller.spec.ts
src/modules/**/tests/**/*.integration.spec.ts
📄 CodeRabbit inference engine (.cursorrules)
Include integration tests named [module].integration.spec.ts under tests/
Files:
src/modules/coupons/tests/coupon.integration.spec.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/stores/dto/store.dto.tssrc/modules/auth/dto/auth.dto.ts
🧠 Learnings (2)
📚 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/validation.middleware.tssrc/dtos/UserDTO.tssrc/modules/stores/dto/store.dto.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 DTOs for data validation
Applied to files:
src/middleware/validation.middleware.tssrc/middleware/userValidation.middleware.tssrc/dtos/UserDTO.tssrc/modules/auth/dto/auth.dto.ts
🧬 Code graph analysis (9)
src/middleware/auth.middleware.ts (5)
src/modules/auth/guards/roles.guard.ts (1)
requireRole(28-44)src/modules/shared/middleware/auth.middleware.ts (2)
requireRole(80-90)AuthenticatedRequest(11-23)src/modules/shared/types/auth-request.type.ts (1)
AuthenticatedRequest(4-18)src/types/express.d.ts (1)
AuthenticatedRequest(55-57)src/types/auth-request.type.ts (1)
AuthenticatedRequest(14-26)
src/middleware/validateRequest.middleware.ts (1)
src/middleware/validation.middleware.ts (1)
validateRequest(5-28)
src/middleware/validation.middleware.ts (1)
src/middleware/validateRequest.middleware.ts (1)
validateRequest(4-15)
src/modules/coupons/tests/coupon.service.spec.ts (1)
src/modules/coupons/entities/coupon.entity.ts (1)
CouponUsage(13-13)
src/modules/auth/decorators/wallet-ownership.decorator.ts (1)
src/modules/auth/guards/wallet-ownership.guard.ts (2)
WalletOwnershipConfig(6-24)WALLET_OWNERSHIP_METADATA_KEY(4-4)
src/modules/files/middlewares/file-upload.middleware.ts (1)
src/modules/files/index.ts (1)
FileType(2-2)
src/middleware/paramValidation.middleware.ts (1)
src/middleware/validation.middleware.ts (1)
paramValidators(30-41)
src/cache/tests/cache.service.spec.ts (1)
src/modules/notifications/tests/mocks/config.mock.ts (1)
mockConfigService(1-11)
src/modules/attributes/controllers/attributes.controller.ts (4)
src/modules/attributes/dto/create-attribute.dto.ts (1)
CreateAttributeDto(18-40)src/modules/attributes/dto/attribute-response.dto.ts (2)
AttributeResponseDto(18-43)PaginatedAttributesResponseDto(45-62)src/modules/attributes/dto/get-attributes-query.dto.ts (1)
GetAttributesQueryDto(5-35)src/modules/attributes/dto/update-attribute.dto.ts (1)
UpdateAttributeDto(4-16)
| async invalidateEntity(entity: string): Promise<{ message: string }> { | ||
| await this.cacheService.invalidateEntity(entity); | ||
| return { message: `Cache invalidated for entity: ${entity}` }; | ||
| } |
There was a problem hiding this comment.
Missing @Param decorator for route parameter.
The invalidateEntity method uses entity: string parameter but lacks the @Param('entity') decorator to bind it to the route parameter :entity (line 35). This will cause entity to be undefined at runtime.
Apply this diff:
+ async invalidateEntity(@Param('entity') entity: string): Promise<{ message: string }> {
- async invalidateEntity(entity: string): Promise<{ message: string }> {Import Param from @nestjs/common:
-import { Controller, Post, Get, Delete, UseGuards, HttpCode, HttpStatus } from '@nestjs/common';
+import { Controller, Post, Get, Delete, UseGuards, HttpCode, HttpStatus, Param } from '@nestjs/common';📝 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.
| async invalidateEntity(entity: string): Promise<{ message: string }> { | |
| await this.cacheService.invalidateEntity(entity); | |
| return { message: `Cache invalidated for entity: ${entity}` }; | |
| } | |
| ++ b/src/cache/controllers/cache.controller.ts | |
| @@ -1,7 +1,7 @@ | |
| import { Controller, Post, Get, Delete, UseGuards, HttpCode, HttpStatus, Param } from '@nestjs/common'; | |
| import { CacheService } from '../services/cache.service'; | |
| @Controller('cache') | |
| export class CacheController { | |
| @@ -38,7 +38,7 @@ export class CacheController { | |
| @HttpCode(HttpStatus.OK) | |
| async invalidateEntity(@Param('entity') entity: string): Promise<{ message: string }> { | |
| await this.cacheService.invalidateEntity(entity); | |
| return { message: `Cache invalidated for entity: ${entity}` }; | |
| } |
🤖 Prompt for AI Agents
In src/cache/controllers/cache.controller.ts around lines 41 to 44, the route
handler parameter entity is missing the @Param('entity') decorator and thus will
be undefined at runtime; import Param from @nestjs/common at the top of the file
and annotate the method signature parameter with @Param('entity') so NestJS
binds the :entity route param to the entity argument, keeping the method return
and logic unchanged.
| async invalidateAction(entity: string, action: string): Promise<{ message: string }> { | ||
| await this.cacheService.invalidateAction(entity, action); | ||
| return { message: `Cache invalidated for entity: ${entity}, action: ${action}` }; | ||
| } |
There was a problem hiding this comment.
Missing @Param decorators for route parameters.
The invalidateAction method uses entity: string and action: string parameters but lacks the @Param decorators to bind them to the route parameters :entity and :action (line 46). This will cause both parameters to be undefined at runtime.
Apply this diff:
+ async invalidateAction(@Param('entity') entity: string, @Param('action') action: string): Promise<{ message: string }> {
- async invalidateAction(entity: string, action: string): Promise<{ message: string }> {Ensure Param is imported from @nestjs/common (see previous comment).
📝 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.
| async invalidateAction(entity: string, action: string): Promise<{ message: string }> { | |
| await this.cacheService.invalidateAction(entity, action); | |
| return { message: `Cache invalidated for entity: ${entity}, action: ${action}` }; | |
| } | |
| async invalidateAction( | |
| @Param('entity') entity: string, | |
| @Param('action') action: string | |
| ): Promise<{ message: string }> { | |
| await this.cacheService.invalidateAction(entity, action); | |
| return { message: `Cache invalidated for entity: ${entity}, action: ${action}` }; | |
| } |
🤖 Prompt for AI Agents
In src/cache/controllers/cache.controller.ts around lines 52 to 55, the
invalidateAction method parameters are not bound to route params; add
@Param('entity') entity: string and @Param('action') action: string to the
method signature so NestJS injects the route values, and ensure Param is
imported from '@nestjs/common' at the top of the file.
| } else if (exception.message) { | ||
| message = exception.message; |
There was a problem hiding this comment.
Type safety issue: accessing message on unknown type.
Line 34 accesses exception.message when exception is typed as unknown and has already been checked to not be an HttpException. TypeScript will flag this as an error since unknown doesn't have a message property.
Apply this diff to fix the type safety issue:
- } else if (exception.message) {
- message = exception.message;
+ } else if ((exception as { message?: string }).message) {
+ message = (exception as { message?: string }).message;
}📝 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.
| } else if (exception.message) { | |
| message = exception.message; | |
| } else if ((exception as { message?: string }).message) { | |
| message = (exception as { message?: string }).message; | |
| } |
🤖 Prompt for AI Agents
In src/common/filters/http-exception.filter.ts around lines 34-35, avoid
accessing exception.message on an unknown-typed value; narrow the type first
(e.g., check instanceof Error and use exception.message, or check typeof
exception === 'object' && exception !== null && 'message' in exception then
coerce to string) and assign message only after that safe check so TypeScript no
longer complains about accessing a property on unknown.
| @@ -2,13 +2,13 @@ import { Request, Response, NextFunction } from 'express'; | |||
| import { AuthenticatedRequest } from '../types/auth-request.type'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify AuthenticatedRequest interface consistency across the codebase.
Multiple AuthenticatedRequest interface definitions exist with slightly different shapes:
src/types/auth-request.type.ts:id: string, includesfileProvider/fileTypesrc/modules/shared/types/auth-request.type.ts:id: string | number, includesbuyerData/sellerDatasrc/modules/shared/middleware/auth.middleware.ts: Similar to shared typessrc/types/express.d.ts: Minimaluser: Express.User
This duplication can lead to type inconsistencies and runtime issues when middleware and controllers expect different shapes.
Run the following script to find all AuthenticatedRequest definitions and their usage:
🏁 Script executed:
#!/bin/bash
# Find all AuthenticatedRequest interface definitions
echo "=== AuthenticatedRequest interface definitions ==="
rg -n -A 15 "interface AuthenticatedRequest"
echo -e "\n=== AuthenticatedRequest imports ==="
rg -n "import.*AuthenticatedRequest"Length of output: 5259
Consolidate AuthenticatedRequest into a single shared definition.
Multiple AuthenticatedRequest interfaces currently exist across the codebase (in src/types/auth-request.type.ts, src/types/express.d.ts, src/modules/shared/types/auth-request.type.ts, and src/modules/shared/middleware/auth.middleware.ts), resulting in divergent shapes and import paths. Unify these into one canonical interface (for example, in src/types/auth-request.type.ts), remove the duplicate declarations, and update all imports to reference that single source.
🤖 Prompt for AI Agents
In src/middleware/auth.middleware.ts around line 2, the project imports an
AuthenticatedRequest type that is duplicated in multiple places; consolidate by
keeping a single canonical interface in src/types/auth-request.type.ts, delete
the other duplicate declarations (src/types/express.d.ts,
src/modules/shared/types/auth-request.type.ts,
src/modules/shared/middleware/auth.middleware.ts), export the canonical
interface from src/types/auth-request.type.ts, and update all files to import
AuthenticatedRequest from that single path; after changes, run the TypeScript
build to fix any import paths and adjust any shape differences to match the
canonical interface.
| export const paramValidationMiddleware = (validators: Record<string, unknown>): ((req: Request, res: Response, next: NextFunction) => Promise<void>) => { | ||
| return validateRequest(Object.values(validators)); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify validators typing is compatible with express-validator.
The validators parameter is typed as Record<string, unknown>, but express-validator returns ValidationChain instances (e.g., from param().isInt()). The loose unknown typing may allow non-validator objects to be passed, leading to runtime errors when validateRequest tries to call .run(req) on them.
Consider constraining the type to Record<string, ValidationChain> or a similar express-validator-compatible interface.
🏁 Script executed:
#!/bin/bash
# Description: Check if ValidationChain type is imported or used elsewhere
rg -nP --type=ts 'ValidationChain|express-validator' -g '!node_modules' -g '**/middleware/*.ts' -A2 -B2Length of output: 754
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/middleware/validateRequest.middleware.tsLength of output: 574
Constrain validators to ValidationChain
Change the signature in src/middleware/paramValidation.middleware.ts from
export const paramValidationMiddleware = (validators: Record<string, unknown>) => …to
import { ValidationChain } from 'express-validator';
export const paramValidationMiddleware = (validators: Record<string, ValidationChain>) => …and update validateRequest to accept ValidationChain[] instead of unknown[] so only valid express-validator chains are passed.
🤖 Prompt for AI Agents
In src/middleware/paramValidation.middleware.ts around lines 10 to 12, the
validators parameter is typed too loosely as Record<string, unknown>; import
ValidationChain from 'express-validator' and change the signature to accept
Record<string, ValidationChain>, then update validateRequest’s signature to
accept ValidationChain[] (instead of unknown[]) so Object.values(validators) is
a ValidationChain[]; ensure TypeScript types are updated where validateRequest
is declared/used to reflect the new ValidationChain[] parameter.
| import { Test, TestingModule } from '@nestjs/testing'; | ||
| import { EscrowService } from './escrow.service'; | ||
| import { getRepositoryToken } from '@nestjs/typeorm'; | ||
| import { Repository, DataSource } from 'typeorm'; | ||
| import { DataSource } from 'typeorm'; | ||
| import { Escrow } from '../entities/escrow.entity'; | ||
| import { Milestone, MilestoneStatus } from '../entities/milestone.entity'; | ||
| import { ForbiddenException, NotFoundException, BadRequestException } from '@nestjs/common'; | ||
| import { ForbiddenException, BadRequestException } from '@nestjs/common'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify module path consistency.
This test file is located in src/modules/escrows/ (plural), while other escrow-related files in the PR are in src/modules/escrow/ (singular). Ensure this path is correct and not a typo, as it may indicate a structural inconsistency in the module organization.
Run the following script to check the directory structure:
🏁 Script executed:
#!/bin/bash
# Check for escrow vs escrows directories
echo "=== Escrow directories ==="
fd -t d "escrow" src/modules/
echo -e "\n=== Files in escrows (plural) ==="
fd -t f . src/modules/escrows/ 2>/dev/null || echo "Directory does not exist"
echo -e "\n=== Files in escrow (singular) ==="
fd -t f . src/modules/escrow/ 2>/dev/null || echo "Directory does not exist"Length of output: 1567
Consolidate duplicate ‘escrow’ modules
The repository contains both src/modules/escrow/ and src/modules/escrows/, each defining controllers, services, entities, and tests for escrow functionality. This duplication causes confusion and potential import conflicts. Refactor to a single module directory (choose singular or plural), merge duplicate files, and update all imports accordingly.
🤖 Prompt for AI Agents
In src/modules/escrows/services/escrow.service.spec.ts lines 1-8, the project
has duplicated escrow modules under src/modules/escrow/ and
src/modules/escrows/, causing confusion and import conflicts; choose one
canonical directory name (singular or plural), consolidate files by merging
controllers, services, entities and tests into that single module directory,
remove the other directory, update all import paths across the codebase and
tests to reference the chosen module, and run tests to ensure no broken imports
remain.
| @ApiResponse({ status: 200, description: 'Product retrieved successfully' }) | ||
| @ApiResponse({ status: 404, description: 'Product not found' }) | ||
| async getProductById(@Param('id') id: string) { | ||
| async getProductById(@Param('id') id: string): Promise<{ success: boolean; data: Product }> { |
There was a problem hiding this comment.
🧩 Analysis chain
Missing import for Product type.
The Product type is referenced in the return type but is not imported. This will cause a TypeScript compilation error.
Add the import at the top of the file:
import { ApiTags, ApiOperation, ApiResponse, ApiBearerAuth } from '@nestjs/swagger';
import { ProductService } from '../services/product.service';
+import { Product } from '../entities/product.entity';
import { JwtAuthGuard } from '../../shared/guards/jwt-auth.guard';Note: Adjust the import path based on where your Product entity/type is defined (e.g., ../entities/product.entity or ../types/product.type).
🏁 Script executed:
#!/bin/bash
# Find the Product entity/type definition to verify the correct import path
fd -e ts -e d.ts "product" src/modules/products | xargs rg -l "^(export )?(class|interface|type) Product\b"Length of output: 47
Add missing Product import
Product is used in the return type but not imported, causing a TypeScript error. Add this at the top of product.controller.ts:
import { ApiTags, ApiOperation, ApiResponse, ApiBearerAuth } from '@nestjs/swagger';
import { ProductService } from '../services/product.service';
+import { Product } from '../entities/product.entity';
import { JwtAuthGuard } from '../../shared/guards/jwt-auth.guard';📝 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.
| async getProductById(@Param('id') id: string): Promise<{ success: boolean; data: Product }> { | |
| import { ApiTags, ApiOperation, ApiResponse, ApiBearerAuth } from '@nestjs/swagger'; | |
| import { ProductService } from '../services/product.service'; | |
| import { Product } from '../entities/product.entity'; | |
| import { JwtAuthGuard } from '../../shared/guards/jwt-auth.guard'; |
🤖 Prompt for AI Agents
In src/modules/products/controllers/product.controller.ts around line 50 the
Product type is referenced in the method return type but not imported; add an
import for Product at the top of the file pointing to the file that defines the
Product type (for example the product model/entity file under
src/modules/products/models or src/modules/products/entities, e.g. import {
Product } from 'src/modules/products/models/product.model'), then save so the
TypeScript compiler can resolve the type.
| @ApiOperation({ summary: 'Create a new product' }) | ||
| @ApiResponse({ status: 201, description: 'Product created successfully' }) | ||
| async createProduct(@Body() createProductDto: any, @Request() req: any) { | ||
| async createProduct(@Body() createProductDto: Record<string, unknown>, @Request() req: Record<string, unknown>): Promise<{ success: boolean; data: Product }> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace Record<string, unknown> with proper DTO classes.
Using Record<string, unknown> for DTOs and the request object defeats the purpose of TypeScript's type safety and NestJS's validation pipes.
Issues with current approach:
- No compile-time validation of DTO structure
- Manual field extraction and validation (lines 67-70) is error-prone
req.user?.idtype is unsafe (unknown)- Missing runtime validation via
class-validatordecorators
Create a proper DTO class:
// src/modules/products/dto/create-product.dto.ts
import { IsString, IsNotEmpty, IsNumber, IsOptional, IsArray } from 'class-validator';
import { Type } from 'class-transformer';
export class CreateProductDto {
@IsString()
@IsNotEmpty()
name: string;
@IsString()
@IsNotEmpty()
description: string;
@IsNumber()
@Type(() => Number)
price: number;
@IsString()
@IsNotEmpty()
category: string;
@IsArray()
@IsOptional()
images?: string[];
}For the request object, use NestJS's typed Request:
+import { Request as ExpressRequest } from 'express';
+
+interface AuthenticatedRequest extends ExpressRequest {
+ user: { id: number; role: string };
+}
- async createProduct(@Body() createProductDto: Record<string, unknown>, @Request() req: Record<string, unknown>): Promise<{ success: boolean; data: Product }> {
+ async createProduct(@Body() createProductDto: CreateProductDto, @Request() req: AuthenticatedRequest): Promise<{ success: boolean; data: Product }> {
- const userId = req.user?.id;
+ const userId = req.user.id;
if (!userId) {
throw new BadRequestError('User not authenticated');
}
- const { name, description, price, category, images } = createProductDto;
- if (!name || !description || !price || !category) {
- throw new BadRequestError('Missing required fields');
- }
const product = await this.productService.createProduct({
- name,
- description,
- price,
- category,
- images,
+ ...createProductDto,
sellerId: Number(userId),
});As per coding guidelines
Based on learnings
| @ApiOperation({ summary: 'Update a product' }) | ||
| @ApiResponse({ status: 200, description: 'Product updated successfully' }) | ||
| async updateProduct(@Param('id') id: string, @Body() updateProductDto: any, @Request() req: any) { | ||
| async updateProduct(@Param('id') id: string, @Body() updateProductDto: Record<string, unknown>, @Request() req: Record<string, unknown>): Promise<{ success: boolean; data: Product }> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace Record<string, unknown> with proper DTO classes (same issue as createProduct).
The same typing issues identified in createProduct apply here. Use a dedicated UpdateProductDto class with class-validator decorators and a properly typed AuthenticatedRequest.
Create an update DTO:
// src/modules/products/dto/update-product.dto.ts
import { IsString, IsNumber, IsOptional, IsArray } from 'class-validator';
import { Type } from 'class-transformer';
export class UpdateProductDto {
@IsString()
@IsOptional()
name?: string;
@IsString()
@IsOptional()
description?: string;
@IsNumber()
@Type(() => Number)
@IsOptional()
price?: number;
@IsString()
@IsOptional()
category?: string;
@IsArray()
@IsOptional()
images?: string[];
}Then update the method signature:
- async updateProduct(@Param('id') id: string, @Body() updateProductDto: Record<string, unknown>, @Request() req: Record<string, unknown>): Promise<{ success: boolean; data: Product }> {
+ async updateProduct(@Param('id') id: string, @Body() updateProductDto: UpdateProductDto, @Request() req: AuthenticatedRequest): Promise<{ success: boolean; data: Product }> {
- const userId = req.user?.id;
+ const userId = req.user.id;
if (!userId) {
throw new BadRequestError('User not authenticated');
}
const product = await this.productService.getProductById(parseInt(id));
if (product.sellerId !== userId) {
throw new ForbiddenError('Not authorized to update this product');
}
- const { name, description, price, category, images } = updateProductDto;
- const updatedProduct = await this.productService.updateProduct(parseInt(id), {
- name,
- description,
- price,
- category,
- images,
- });
+ const updatedProduct = await this.productService.updateProduct(parseInt(id), updateProductDto);As per coding guidelines
Based on learnings
🤖 Prompt for AI Agents
In src/modules/products/controllers/product.controller.ts around line 90, the
updateProduct method currently uses Record<string, unknown> for the body and
request types; replace these with a proper UpdateProductDto and an
AuthenticatedRequest type: create src/modules/products/dto/update-product.dto.ts
(using class-validator decorators as shown in the review), import
UpdateProductDto and AuthenticatedRequest into the controller, change the method
signature to async updateProduct(@Param('id') id: string, @Body()
updateProductDto: UpdateProductDto, @Request() req: AuthenticatedRequest):
Promise<{ success: boolean; data: Product }>, and ensure the controller is wired
to use validation (e.g., ValidationPipe globally or on the route) so the DTO is
validated.
| @ApiOperation({ summary: 'Delete a product' }) | ||
| @ApiResponse({ status: 200, description: 'Product deleted successfully' }) | ||
| async deleteProduct(@Param('id') id: string, @Request() req: any) { | ||
| async deleteProduct(@Param('id') id: string, @Request() req: Record<string, unknown>): Promise<{ success: boolean; message: string }> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use typed request interface for better type safety.
The req: Record<string, unknown> type loses all type information about the authenticated user, requiring unsafe optional chaining (req.user?.id).
Apply the same AuthenticatedRequest interface suggested for other methods:
- async deleteProduct(@Param('id') id: string, @Request() req: Record<string, unknown>): Promise<{ success: boolean; message: string }> {
+ async deleteProduct(@Param('id') id: string, @Request() req: AuthenticatedRequest): Promise<{ success: boolean; message: string }> {
- const userId = req.user?.id;
+ const userId = req.user.id;
if (!userId) {
throw new BadRequestError('User not authenticated');
}As per coding guidelines
📝 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.
| async deleteProduct(@Param('id') id: string, @Request() req: Record<string, unknown>): Promise<{ success: boolean; message: string }> { | |
| async deleteProduct( | |
| @Param('id') id: string, | |
| @Request() req: AuthenticatedRequest | |
| ): Promise<{ success: boolean; message: string }> { | |
| const userId = req.user.id; | |
| if (!userId) { | |
| throw new BadRequestError('User not authenticated'); | |
| } | |
| // …rest of implementation… |
🤖 Prompt for AI Agents
In src/modules/products/controllers/product.controller.ts around line 120, the
controller method types the request as Record<string, unknown> which strips user
typing and forces unsafe optional chaining; change the parameter to use the
shared AuthenticatedRequest interface (import it from its module if not already
imported), update the method signature to async deleteProduct(@Param('id') id:
string, @Request() req: AuthenticatedRequest): Promise<{ success: boolean;
message: string }>, and adjust any req.user usages to the properly typed fields
(e.g., req.user.id) relying on the interface rather than optional chaining.
1 participant
🐛 Fix: Resolve Linter Warnings (Issue #176)
📊 Summary
🔧 Changes Made
anytypes with specific TypeScript types🧪 Testing
📁 Files Modified
58 files updated with type improvements
Closes #176
Summary by CodeRabbit
Breaking Changes
Improvements
Chores