Add logger utility and integrate with error handling and request logging #62
Conversation
- Introduced a Winston logger for structured logging. - Updated errorHandler to log errors using the new logger. - Integrated Morgan middleware for HTTP request logging. - Added type definitions for Morgan and Winston in package.json and bun.lock.
WalkthroughThis pull request introduces HTTP request and error logging by integrating Morgan middleware with Winston logger. New dependencies (morgan, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/utils/apiError.ts (1)
43-43: Enrich error logs with request context.The error logging lacks contextual information. Consider including request details (method, path, user info) to aid debugging.
🔎 Proposed enhancement
- logger.error('ERROR 🔥', err); + logger.error('ERROR 🔥', { + error: err instanceof Error ? err.message : String(err), + stack: err instanceof Error ? err.stack : undefined, + method: req.method, + path: req.path, + statusCode: status, + });src/utils/logger.ts (1)
3-17: Consider environment-specific log levels and format consistency.The logger has two concerns:
- The
httplevel logs all HTTP requests, which may be too verbose for production environments.- The console transport overrides the base format (timestamp + JSON) with colorize + simple, which loses timestamps in console output.
🔎 Proposed improvements
const logger = winston.createLogger({ - level: "http", + level: process.env.NODE_ENV === 'production' ? 'info' : 'http', format: winston.format.combine( winston.format.timestamp(), winston.format.json() ), transports: [ new winston.transports.Console({ format: winston.format.combine( + winston.format.timestamp({ format: 'YYYY-MM-DD HH:mm:ss' }), winston.format.colorize(), - winston.format.simple() + winston.format.printf(info => `${info.timestamp} ${info.level}: ${info.message}`) ) }) ] });src/app.ts (1)
24-24: Consider environment-specific Morgan formats.The
combinedformat logs extensive details suitable for development but may be overly verbose for production environments.🔎 Proposed enhancement
-app.use(morgan('combined', { stream: new LoggerStream()})); +const morganFormat = process.env.NODE_ENV === 'production' ? 'common' : 'combined'; +app.use(morgan(morganFormat, { stream: new LoggerStream() }));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
package.jsonsrc/app.tssrc/utils/apiError.tssrc/utils/logger.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/apiError.ts (1)
src/utils/logger.ts (1)
logger(25-25)
🔇 Additional comments (3)
src/utils/apiError.ts (1)
2-2: LGTM!The logger import is correctly added and follows the module structure.
src/app.ts (1)
10-11: LGTM!The logger and morgan imports are correctly added and properly integrated into the application.
package.json (1)
55-55: Dependencies are current and secure.Verification confirms:
- morgan
^1.10.1is at the latest version (1.10.1)- winston
^3.19.0is at the latest version (3.19.0)- @types/morgan
^1.9.10is at the latest version (1.9.10)No known security advisories for any of these packages. Historical CVE-2019-5413 (morgan < 1.9.1) does not affect the project since morgan 1.10.1 is in use.
| class LoggerStream { | ||
| write(message: string) { | ||
| logger.info(message.substring(0, message.lastIndexOf('\n'))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type safety and fix edge case in LoggerStream.
The LoggerStream implementation has two issues:
- Missing type annotations for the class
- Line 21:
lastIndexOf('\n')returns -1 if no newline exists, causingsubstring(0, -1)to return an empty string
🔎 Proposed fix
-class LoggerStream {
- write(message: string) {
- logger.info(message.substring(0, message.lastIndexOf('\n')));
+class LoggerStream {
+ write(message: string): void {
+ logger.info(message.trim());
}
}Note: Using trim() is simpler and handles all trailing whitespace, matching the pattern already used in src/utils/logger.ts line 21.
📝 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.
| class LoggerStream { | |
| write(message: string) { | |
| logger.info(message.substring(0, message.lastIndexOf('\n'))); | |
| } | |
| } | |
| class LoggerStream { | |
| write(message: string): void { | |
| logger.info(message.trim()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/app.ts around lines 19 to 23, the LoggerStream class lacks TypeScript
type annotations and its write method uses message.substring(0,
message.lastIndexOf('\n')) which returns an empty string when lastIndexOf
returns -1; change the class to include proper types (e.g., export class
LoggerStream implements NodeJS.WritableStream-like interface or at least
annotate write(message: string): void) and replace the substring logic with a
safe trim of trailing whitespace (e.g., use message.trim() or
message.replace(/\s+$/, '')) before calling logger.info so it never passes an
empty string from a -1 index.
| logger.stream = { | ||
| write: (message: any) => { | ||
| logger.info(message.trim()); | ||
| } | ||
| } as any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace type assertions with proper TypeScript interfaces.
The as any type assertion bypasses type safety. Define a proper interface for the stream object.
🔎 Proposed fix
+interface LoggerStream {
+ write(message: string): void;
+}
+
-logger.stream = {
- write: (message: any) => {
+logger.stream = {
+ write: (message: string) => {
logger.info(message.trim());
}
-} as any;
+} as LoggerStream;📝 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.
| logger.stream = { | |
| write: (message: any) => { | |
| logger.info(message.trim()); | |
| } | |
| } as any; | |
| interface LoggerStream { | |
| write(message: string): void; | |
| } | |
| logger.stream = { | |
| write: (message: string) => { | |
| logger.info(message.trim()); | |
| } | |
| } as LoggerStream; |
🤖 Prompt for AI Agents
In src/utils/logger.ts around lines 19 to 23, the logger.stream assignment uses
an unsafe "as any" cast; define a proper TypeScript interface (e.g., interface
LogStream { write(message: string): void }) or reuse an existing type for stream
objects, update the logger.stream property to be typed with that interface, and
remove the "as any" assertion so the write implementation (calling
logger.info(message.trim())) conforms to the declared type.
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.