Typechecking fixes #77
Conversation
…in auth and song modules
There was a problem hiding this comment.
Pull request overview
This PR addresses various TypeScript typechecking issues in the backend codebase by:
- Upgrading Next.js from 16.0.8 to 16.0.10
- Removing unnecessary @nbw/database dependency from @nbw/song package
- Adding TypeScript configuration options for better type checking
- Creating type declarations for the @nbw/thumbnail/node module
- Fixing test assertions and mock configurations
- Correcting OAuth strategy configurations
Changes:
- Updated Next.js and related dependencies to version 16.0.10
- Added
moduleResolution: "node"andnoEmit: trueto backend tsconfig.json - Created type declaration file for @nbw/thumbnail/node module
- Fixed test mocks to use
jest.spyOninstead of direct mock manipulation - Changed
toThrowErrortotoThrowin multiple test files - Fixed GitHub strategy configuration from
redirect_uritocallbackURL - Added type assertions to resolve type incompatibilities
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| bun.lock | Updated Next.js from 16.0.8 to 16.0.10 and removed @nbw/database dependency from @nbw/song |
| apps/backend/tsconfig.json | Added moduleResolution and noEmit compiler options |
| apps/backend/src/types/thumbnail.d.ts | Created type declarations for @nbw/thumbnail/node module |
| apps/backend/src/song/song.service.spec.ts | Changed mock setup to use jest.spyOn |
| apps/backend/src/song/song.controller.spec.ts | Added explicit type casting with as unknown as |
| apps/backend/src/song/song.controller.ts | Added conditional check for sortField |
| apps/backend/src/lib/GetRequestUser.spec.ts | Changed toThrowError to toThrow |
| apps/backend/src/auth/strategies/google.strategy.spec.ts | Changed toThrowError to toThrow |
| apps/backend/src/auth/strategies/github.strategy.ts | Fixed callbackURL property and added any cast |
| apps/backend/src/auth/strategies/github.strategy.spec.ts | Changed toThrowError to toThrow |
| apps/backend/src/auth/strategies/discord.strategy/index.ts | Added const assertion to prompt property |
| apps/backend/src/auth/strategies/discord.strategy/discord.strategy.spec.ts | Changed toThrowError to toThrow |
| apps/backend/src/auth/strategies/JWT.strategy.spec.ts | Changed toThrowError to toThrow |
| apps/backend/src/auth/auth.service.ts | Added type casts for JWT payload and options |
| apps/backend/src/auth/auth.service.spec.ts | Added explicit parameter type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const sortField = query.sort | ||
| ? sortFieldMap.get(query.sort) ?? 'createdAt' | ||
| : 'createdAt'; |
There was a problem hiding this comment.
The conditional check query.sort ? sortFieldMap.get(query.sort) ?? 'createdAt' : 'createdAt' is redundant. The original code sortFieldMap.get(query.sort) ?? 'createdAt' already handles the case where query.sort is undefined or null correctly, since Map.get(undefined) returns undefined and the nullish coalescing operator (??) will fall back to 'createdAt'. The added ternary check doesn't add value and makes the code more verbose.
| const sortField = query.sort | |
| ? sortFieldMap.get(query.sort) ?? 'createdAt' | |
| : 'createdAt'; | |
| const sortField = sortFieldMap.get(query.sort) ?? 'createdAt'; |
| this.jwtService.signAsync( | ||
| payload as object, | ||
| { | ||
| secret: this.JWT_SECRET, | ||
| expiresIn: this.JWT_EXPIRES_IN, | ||
| } as any, | ||
| ), | ||
| this.jwtService.signAsync( | ||
| payload as object, | ||
| { | ||
| secret: this.JWT_REFRESH_SECRET, | ||
| expiresIn: this.JWT_REFRESH_EXPIRES_IN, | ||
| } as any, | ||
| ), |
There was a problem hiding this comment.
The cast payload as object is unnecessary since TokenPayload is already an object type with defined properties. This cast reduces type safety without providing benefit. Similarly, casting the options object to any masks potential type issues with the JWT service configuration. Consider reviewing the JWT service types to ensure proper type compatibility rather than using type assertions.
| super({ | ||
| clientID: GITHUB_CLIENT_ID, | ||
| clientSecret: GITHUB_CLIENT_SECRET, | ||
| redirect_uri: `${SERVER_URL}/v1/auth/github/callback`, | ||
| callbackURL: `${SERVER_URL}/v1/auth/github/callback`, | ||
| scope: 'user:read,user:email', | ||
| state: false, | ||
| }); | ||
| } as any); |
There was a problem hiding this comment.
The change from redirect_uri to callbackURL is correct and fixes a typo in the configuration property name. However, casting the entire configuration object to any masks other potential type issues. Consider checking the passport-github types to ensure all configuration properties are correctly typed rather than bypassing type checking with as any.
|
|
||
| mockSongModel.aggregate.mockReturnValue(mockAggregate as any); | ||
| mockSongModel.populate.mockResolvedValue(songList); | ||
| jest.spyOn(songModel, 'aggregate').mockReturnValue(mockAggregate as any); |
There was a problem hiding this comment.
The test setup now uses jest.spyOn(songModel, 'aggregate') but the assertions still reference mockSongModel.aggregate and mockSongModel.populate. Since songModel is injected from the mocked mockSongModel, the assertions should be updated to use songModel instead of mockSongModel for consistency, or the spy should be on mockSongModel.
|
|
||
| mockSongModel.aggregate.mockReturnValue(mockAggregate as any); | ||
| mockSongModel.populate.mockResolvedValue(songList); | ||
| jest.spyOn(songModel, 'aggregate').mockReturnValue(mockAggregate as any); |
There was a problem hiding this comment.
Similar to the previous test, the setup uses jest.spyOn(songModel, 'aggregate') but the assertions reference mockSongModel.aggregate and mockSongModel.populate. The assertions should be updated to use songModel for consistency with the spy setup.
No description provided.