fix: add morgan HTTP request logging middleware#232
Open
pericharlabindhumadhavi-data wants to merge 2 commits into
Open
fix: add morgan HTTP request logging middleware#232pericharlabindhumadhavi-data wants to merge 2 commits into
pericharlabindhumadhavi-data wants to merge 2 commits into
Conversation
|
@pericharlabindhumadhavi-data is attempting to deploy a commit to the xthxr's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughserver.js imports the morgan package and registers an Express middleware using a custom log format that records method, URL, status, content-length, and response time for each incoming HTTP request. No other routing, authentication, or exported API surface was changed. ChangesHTTP Request Logging
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server.js (1)
86-86: ⚡ Quick winConsider environment-based configuration and log streaming for production.
Morgan defaults to stdout, which may not integrate well with production log aggregation systems. Consider:
- Different formats for development vs. production
- Streaming logs to a file or external service in production
- Conditional enabling based on NODE_ENV
💡 Example environment-based configuration
-app.use(morgan('combined')); +// Use 'dev' format in development, 'combined' in production +if (process.env.NODE_ENV === 'production') { + // In production, consider streaming to a file or log service + app.use(morgan('combined', { + skip: (req, res) => res.statusCode < 400 // Only log errors in production + })); +} else { + app.use(morgan('dev')); // Colorized, concise format for development +}For production file logging:
const fs = require('fs'); const path = require('path'); // Create a write stream (in append mode) const accessLogStream = fs.createWriteStream( path.join(__dirname, 'access.log'), { flags: 'a' } ); app.use(morgan('combined', { stream: accessLogStream }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.js` at line 86, The morgan middleware is currently always configured with app.use(morgan('combined')), which writes to stdout and isn’t suitable for production log aggregation; update the middleware to configure logging based on NODE_ENV (use a verbose/console-friendly format like 'dev' in development and 'combined' in production), conditionally enable morgan only when needed, and in production stream logs to a persistent sink by creating a write stream via fs.createWriteStream (e.g., accessLogStream) or by wiring morgan’s stream option to your external logging service; modify the existing app.use(morgan('combined')) call to choose format and stream based on process.env.NODE_ENV and ensure errors creating the stream are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.js`:
- Line 86: The app is using morgan('combined') which logs PII (client IP and
user-agent); replace this with a privacy-safe logging format by updating the
app.use(morgan('combined')) call to use either a built-in minimal format ('tiny'
or 'common') or a custom morgan format string that omits :remote-addr and
:user-agent (e.g., include only :method :url :status :res[content-length]
:response-time), or register custom tokens to anonymize IPs before logging;
update the call to morgan in the same location (the app.use(...) invocation) and
ensure any retention/consent policy is documented if you must keep IPs.
---
Nitpick comments:
In `@server.js`:
- Line 86: The morgan middleware is currently always configured with
app.use(morgan('combined')), which writes to stdout and isn’t suitable for
production log aggregation; update the middleware to configure logging based on
NODE_ENV (use a verbose/console-friendly format like 'dev' in development and
'combined' in production), conditionally enable morgan only when needed, and in
production stream logs to a persistent sink by creating a write stream via
fs.createWriteStream (e.g., accessLogStream) or by wiring morgan’s stream option
to your external logging service; modify the existing
app.use(morgan('combined')) call to choose format and stream based on
process.env.NODE_ENV and ensure errors creating the stream are handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Author
|
Hey @xthxr, this one is a small 2-line fix whenever you get a moment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
morganwas already installed inpackage.jsonbut was never imported or registered as middleware inserver.js. This meant no HTTP access logs were being generated in production, making it impossible to debug request-level issues without adding extra instrumentation.The fix adds two lines to
server.js:const morgan = require('morgan');at the top with other importsapp.use(morgan('combined'));early in the middleware chain before route handlersEvery HTTP request will now produce a structured log line like:
GET /api/user/links 200 45msRelated Issue
Fixes #204
Type of Change
Checklist
Notes
No new dependencies introduced —
morganwas already present inpackage.json. Fix is exactly two lines.Summary by CodeRabbit