V2 #14
V2 #14
Conversation
…, and development workflows
…nfiguration files
… token generation
…d user session management
…improved readability
… configuration and add openapi.yml to .gitignore
Summary of ChangesHello @muneebhashone, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request, titled 'V2', introduces significant architectural changes and new features aimed at improving modularity, developer experience, and operational capabilities. The core of the changes revolves around a new plugin-based application bootstrapping system, a robust session management solution, and comprehensive documentation for AI-assisted development. It also includes a new CLI tool for code generation and enhanced observability features. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and impressive overhaul of the TypeScript Backend Toolkit, refactoring it into a more modular, plugin-based architecture. The additions of comprehensive session management, observability features (logging and metrics), a CLI for code generation, and extensive documentation are all excellent improvements that will greatly enhance the developer experience and robustness of the toolkit.
The code is generally well-structured and follows modern best practices. However, there are several inconsistencies and outdated references within the newly added documentation and rule files (.mdc), which could lead to confusion. Additionally, there are a few areas where type safety could be improved by avoiding as any, and some generated code templates could be made more consistent.
My review focuses on addressing these inconsistencies to ensure the documentation and generated code are clear, accurate, and aligned with the new architecture. Great work on this major refactoring!
| ```typescript | ||
| const file = req.file; // For single file | ||
| const files = req.files; // For multiple files | ||
| const url = (req.file as any).location; // S3 URL |
There was a problem hiding this comment.
Using as any to access the location property on req.file bypasses TypeScript's type safety. The file object provided by multer-s3 has a specific type that includes the location property.
You should use a type assertion to the correct type, which is Express.MulterS3.File. This ensures type safety and improves code clarity and maintainability.
Additionally, another as any is used on line 296 (const error = new Error('Not found') as any;). This is also an anti-pattern. A better approach is to create a custom HttpError class that extends Error and includes a statusCode property, or to consistently use the successResponse helper for client-side errors like NOT_FOUND, as shown in other examples.
const url = (req.file as Express.MulterS3.File).location; // S3 URL
| - `.env` - Local development (gitignored) | ||
| - `.env.local` - Local production build (gitignored) | ||
| - `.env.production` - Production environment (gitignored) | ||
| - [src/config/config.service.ts](mdc:src/config/config.service.ts) - Type-safe config with Zod validation |
There was a problem hiding this comment.
This documentation file refers to src/config/config.service.ts, which has been removed and replaced by src/config/env.ts in this pull request. All references to the old configuration file throughout this document should be updated to point to the new env.ts file to avoid confusion.
- [src/config/env.ts](mdc:src/config/env.ts) - Type-safe config with Zod validation
.cursor/rules/new-module.mdc
Outdated
| import * as service from './module.service'; | ||
|
|
||
| export const list = async (req: Request, res: Response) => { | ||
| const { page = 1, limit = 10, search } = req.query; | ||
|
|
||
| const result = await service.findAll({ | ||
| page: Number(page), | ||
| limit: Number(limit), | ||
| search: search as string, | ||
| }); | ||
|
|
||
| return res.status(200).json(result); | ||
| }; | ||
|
|
||
| export const getById = async (req: Request, res: Response) => { | ||
| const { id } = req.params; | ||
|
|
||
| const item = await service.findById(id); | ||
|
|
||
| if (!item) { | ||
| return res.status(404).json({ message: 'Item not found' }); | ||
| } | ||
|
|
||
| return res.status(200).json(item); | ||
| }; | ||
|
|
||
| export const create = async (req: Request, res: Response) => { | ||
| const data = req.body; | ||
|
|
||
| const item = await service.create(data); | ||
|
|
||
| return res.status(201).json(item); | ||
| }; | ||
|
|
||
| export const update = async (req: Request, res: Response) => { | ||
| const { id } = req.params; | ||
| const data = req.body; | ||
|
|
||
| const item = await service.update(id, data); | ||
|
|
||
| if (!item) { | ||
| return res.status(404).json({ message: 'Item not found' }); | ||
| } | ||
|
|
||
| return res.status(200).json(item); | ||
| }; | ||
|
|
||
| export const remove = async (req: Request, res: Response) => { | ||
| const { id } = req.params; | ||
|
|
||
| const deleted = await service.remove(id); | ||
|
|
||
| if (!deleted) { | ||
| return res.status(404).json({ message: 'Item not found' }); | ||
| } | ||
|
|
||
| return res.status(200).json({ message: 'Item deleted successfully' }); | ||
| }; |
There was a problem hiding this comment.
The controller template in this guide is inconsistent with the patterns defined in controllers.mdc. Specifically:
- Namespace Import: It uses
import * as service from './module.service';, butcontrollers.mdcexplicitly advises against this, recommending named imports instead (import { findAll } from './module.service';). - Direct Response Handling: It uses
res.status(200).json(result);directly, which contradicts the rule to always use thesuccessResponse()helper for formatting responses.
To maintain consistency across the toolkit's documentation and promote best practices, this template should be updated to align with the patterns in controllers.mdc.
Here is a corrected example for the list function:
import type { Request, Response } from 'express';
import { successResponse } from '../../utils/api.utils';
import { findAll } from './module.service';
export const list = async (req: Request, res: Response) => {
const { page = 1, limit = 10, search } = req.query;
const result = await findAll({
page: Number(page),
limit: Number(limit),
search: search as string,
});
return successResponse(res, undefined, result);
};| ``` | ||
|
|
||
| - Security is auto-detected in OpenAPI by presence of `canAccess()` middleware | ||
| - JWT payload available as `req.jwtPayload` in controller (via `canAccess()`) |
There was a problem hiding this comment.
This documentation is contradictory. It states that the JWT payload is available as req.jwtPayload, but other documentation files (controllers.mdc) and the implementation in extract-jwt.ts correctly use req.user. To ensure consistency and prevent confusion, this line should be updated to reference req.user.
- JWT payload available as `req.user` in controller (via `canAccess()`)
| ❌ DON'T use `.openapi()` method in schema files | ||
| ❌ DON'T wrap schemas in request/response objects (that's for routers) |
There was a problem hiding this comment.
There are significant contradictions between the rules in this file and the patterns demonstrated in new-module.mdc:
.openapi()Usage: This file states "DON'T use.openapi()method in schema files", but thenew-module.mdcguide actively uses it in its Zod schema examples.- Schema Structure: This file advises against wrapping schemas in request/response objects, but
new-module.mdcshows exactly that pattern (e.g.,export const listSchema = { request: { query: QuerySchema }, response: { ... } };).
These conflicting guidelines will be very confusing for developers. The rules should be unified to reflect a single, consistent approach for defining schemas across the toolkit.
| - File upload handling | ||
| - Common routing mistakes | ||
|
|
||
| **[schemas.mdc](schemas.mdc)** - _Applies to: `_.schema.ts`\* |
There was a problem hiding this comment.
The asterisk * at the end of the file glob is being interpreted as a Markdown emphasis (italics) marker, which makes the documentation slightly harder to read. To ensure it's rendered as a literal character, it should be escaped with a backslash.
This same issue occurs on lines 36, 52, 63, and 71.
| **[schemas.mdc](schemas.mdc)** - _Applies to: `_.schema.ts`\* | |
| **[schemas.mdc](schemas.mdc)** - _Applies to: `_.schema.ts`\*_ |
| Logs use Pino logger from [src/lib/logger.service.ts](mdc:src/lib/logger.service.ts): | ||
|
|
||
| ```typescript | ||
| import { logger } from '@/lib/logger.service'; |
There was a problem hiding this comment.
The documentation contains outdated paths and import statements for the logger service. The logger has been moved from src/lib/logger.service.ts to src/observability/logger.ts as part of the refactoring.
Updating these paths will ensure the development guide is accurate and prevent confusion for developers following the documentation.
Logs use Pino logger from [src/observability/logger.ts](mdc:src/observability/logger.ts):
```typescript
import { logger } from '@/observability/logger';
| Check email sending logs: | ||
|
|
||
| ```typescript | ||
| import { logger } from '@/lib/logger.service'; |
There was a problem hiding this comment.
The import path for the logger service is outdated. It has been moved from src/lib/logger.service.ts to src/observability/logger.ts. This should be updated to reflect the new project structure.
import { logger } from '@/observability/logger';
| export const delete${className} = async (${moduleName}Id: MongoIdSchemaType): Promise<void> => { | ||
| const ${moduleName} = await ${className}.findByIdAndDelete(${moduleName}Id.id); | ||
| if (!${moduleName}) { | ||
| throw new Error("${className} not found"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The generated service methods have inconsistent signatures for handling document IDs. The delete${className} service expects an object { id: string } (as MongoIdSchemaType), while get${className}ById and update${className} expect a simple string for the ID.
For better consistency and predictability, all services that operate on a single document by its ID should accept the ID as a simple string. This simplifies the service layer and the corresponding controller logic.
I've suggested a change for delete${className}. You would also need to update the controller call in handleDelete${className} (line 333) from await delete${className}({ id: req.params.id }); to await delete${className}(req.params.id);.
export const delete${className} = async (${moduleName}Id: string): Promise<void> => {
const ${moduleName} = await ${className}.findByIdAndDelete(${moduleName}Id);
if (!${moduleName}) {
throw new Error("${className} not found");
}
};
src/main.ts
Outdated
| const serverAdapter = new ExpressAdapter(); | ||
| serverAdapter.setBasePath('/admin/queues'); | ||
|
|
||
| console.log(getRegisteredQueues()); |
There was a problem hiding this comment.
This console.log statement appears to be for debugging purposes. In a production or library environment, it's better to use the configured logger (logger.debug or logger.info) to ensure logs are handled consistently and can be filtered by level. This also prevents noisy output in production environments.
| console.log(getRegisteredQueues()); | |
| logger.debug({ queues: getRegisteredQueues() }, 'Registered Queues'); |
…customization steps
1 participant
No description provided.