adding new feature #1
Conversation
WalkthroughAdds activity/audit logging: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Server as Express
participant TodoCtrl as TodoController
participant ActCtrl as ActivityController
participant DB as SQLite DB
rect rgba(220,240,255,0.6)
note over User,Server: Create Todo (sync flow, async logging)
User->>Server: POST /api/todos
Server->>TodoCtrl: createTodo(req)
TodoCtrl->>DB: INSERT INTO todos (...)
DB-->>TodoCtrl: lastID, row
TodoCtrl->>ActCtrl: createActivity({todo_id, action: "CREATE", new_value})
ActCtrl->>DB: INSERT INTO activities (...)
DB-->>ActCtrl: lastID
TodoCtrl-->>Server: 201 {todo}
Server-->>User: 201 {todo}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
✨ 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. Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai generate sequence diagram |
Sequence Diagram for PRsequenceDiagram
autonumber
actor Dev as Developer
participant Repo as Git Hosting / PR
participant CI as CI Pipeline
participant Tests as Test Suite
actor Reviewer
actor Maintainer
participant Deploy as Deployment Pipeline
Dev->>Repo: Push feature branch
Dev->>Repo: Open PR (Title/Objectives, Summary)
Repo->>CI: Trigger pipeline
CI->>Tests: Build + Lint + Test
Tests-->>CI: Results (pass/fail)
CI-->>Repo: Update status checks
Reviewer->>Repo: Review changes
Repo-->>Reviewer: Diff + CI status
Reviewer-->>Repo: Approve or Request changes
alt Changes requested
Repo-->>Dev: Feedback/comments
Dev->>Repo: Push updates
Repo->>CI: Re-run pipeline
CI->>Tests: Re-execute suite
Tests-->>CI: Results
CI-->>Repo: Status checks updated
end
opt Approved
Maintainer->>Repo: Merge PR
Repo->>Deploy: Trigger release/deploy
Deploy-->>Repo: Deployment status
Repo-->>Dev: Merge/deploy notifications
end
|
✅ Actions performedSequence diagram generated. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
config/database.js (2)
37-50: Consider adding indexes for performance optimization.The activities table will likely be queried frequently by
todo_idandcreated_at. Consider adding indexes to improve query performance.Add these indexes after creating the activities table:
// After line 69, add: this.db.run(`CREATE INDEX IF NOT EXISTS idx_activities_todo_id ON activities(todo_id)`, (err) => { if (err) console.error('Error creating todo_id index:', err.message); }); this.db.run(`CREATE INDEX IF NOT EXISTS idx_activities_created_at ON activities(created_at DESC)`, (err) => { if (err) console.error('Error creating created_at index:', err.message); });
52-71: Consider using async/await or Promise chaining for better readability.The nested callbacks make the code harder to read and maintain. Consider refactoring to use async/await or Promise chaining for sequential table creation.
Here's a cleaner approach using promisified database operations:
- // Create todos table - this.db.run(createTodosTableSQL, (err) => { - if (err) { - console.error('Error creating todos table:', err.message); - reject(err); - return; - } - console.log('Todos table ready'); - - // Create activities table - this.db.run(createActivityTableSQL, (err) => { - if (err) { - console.error('Error creating activities table:', err.message); - reject(err); - return; - } - console.log('Activities table ready'); - resolve(); - }); - }); + const runSQL = (sql, tableName) => { + return new Promise((resolve, reject) => { + this.db.run(sql, (err) => { + if (err) { + console.error(`Error creating ${tableName} table:`, err.message); + reject(err); + } else { + console.log(`${tableName} table ready`); + resolve(); + } + }); + }); + }; + + runSQL(createTodosTableSQL, 'Todos') + .then(() => runSQL(createActivityTableSQL, 'Activities')) + .then(() => resolve()) + .catch(reject);controllers/activityController.js (3)
124-160: Consider making createActivity method private.The
createActivitymethod appears to be for internal use only (as noted in the comment), but it's exposed as a public method of the class.Consider prefixing with underscore to indicate it's internal:
- // Create a new activity (usually called internally) - async createActivity(activityData) { + // Create a new activity (internal use only) + async _createActivity(activityData) {
209-214: Optimize statistics queries using date functions.The date query uses SQLite's
DATE()function which might not use indexes efficiently.Consider using timestamp comparisons for better performance:
- 'SELECT COUNT(*) as today FROM activities WHERE DATE(created_at) = DATE("now")', + "SELECT COUNT(*) as today FROM activities WHERE created_at >= datetime('now', 'start of day')",
239-239: Export pattern could lead to testing difficulties.Exporting an instantiated object makes it harder to mock in tests.
Consider exporting the class and creating the instance separately:
-module.exports = new ActivityController(); +module.exports = ActivityController; +// Or if you need a singleton: +module.exports = { ActivityController, activityController: new ActivityController() };controllers/todoController.js (3)
5-24: Harden IP/user-agent capture; avoid deprecated req.connection and consider proxy/XFF.
Use req.socket.remoteAddress (not req.connection), prefer X-Forwarded-For when behind proxies, and consider redacting/storing a hash if IP/user-agent are sensitive. Also guard JSON.stringify against rare circulars.Apply this diff:
- const userIp = req ? req.ip || req.connection.remoteAddress : null; - const userAgent = req ? req.get('User-Agent') : null; + const userIp = req + ? (req.headers['x-forwarded-for']?.split(',')[0].trim() + || req.ip + || req.socket?.remoteAddress + || null) + : null; + const userAgent = req ? (req.get('User-Agent') || null) : null; @@ - old_value: oldValue ? JSON.stringify(oldValue) : null, - new_value: newValue ? JSON.stringify(newValue) : null, + old_value: oldValue ? safeStringify(oldValue) : null, + new_value: newValue ? safeStringify(newValue) : null,Add once in this file (outside the class):
function safeStringify(obj) { try { return JSON.stringify(obj); } catch { return null; } }
123-195: Change-detection is solid; consider normalizing types and not awaiting log writes.
- Normalize completed to boolean or 0/1 consistently in both old/new snapshots to ease analysis.
- Don’t await activity logging to reduce latency; treat it as best-effort.
Apply this diff:
- db.run(sql, values, async (err) => { + db.run(sql, values, (err) => { if (err) { res.status(500).json({ error: err.message }); return; } // Log the update activity - await this.logActivity( + void this.logActivity( id, 'UPDATE', changes.join(', '), - currentTodo, - { ...currentTodo, ...req.body }, + { ...currentTodo, completed: !!currentTodo.completed }, + { ...currentTodo, ...req.body, completed: completed !== undefined ? !!completed : !!currentTodo.completed }, req ); res.json({ message: 'Todo updated successfully' }); });
208-237: Delete flow looks good; add basic param validation and make logging non-blocking.
- Ensure id is a positive integer before queries; return 400 otherwise.
- Logging can be fire-and-forget.
Apply this diff:
- db.get('SELECT * FROM todos WHERE id = ?', [id], async (err, todo) => { + const numId = Number(id); + if (!Number.isInteger(numId) || numId <= 0) { + res.status(400).json({ error: 'Invalid id' }); + return; + } + db.get('SELECT * FROM todos WHERE id = ?', [numId], async (err, todo) => { @@ - db.run('DELETE FROM todos WHERE id = ?', [id], async (err) => { + db.run('DELETE FROM todos WHERE id = ?', [numId], (err) => { if (err) { res.status(500).json({ error: err.message }); return; } // Log the deletion activity - await this.logActivity( - id, + void this.logActivity( + numId, 'DELETE', `Todo "${todo.title}" was deleted`, todo, null, req ); res.json({ message: 'Todo deleted successfully' }); });server.js (1)
14-19: Enable trust proxy if you rely on req.ip behind proxies/CDNs.
This complements the improved IP capture in controllers.Add after app initialization:
app.set('trust proxy', true);routes/activityRoutes.js (1)
5-22: Add basic validation and auth for destructive endpoints.
- Validate params (id, todoId) and pagination query via express-validator/celebrate.
- Protect DELETE routes (single and bulk clear) with auth/role checks.
Example (sketch):
const { celebrate, Joi, Segments } = require('celebrate'); router.get('/todo/:todoId', celebrate({ [Segments.PARAMS]: Joi.object({ todoId: Joi.number().integer().positive().required() }) }), activityController.getActivitiesByTodoId ); router.delete('/', /* auth middleware here */, activityController.clearAllActivities );Happy to wire up validation middleware across all routes if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(5 hunks)config/database.js(2 hunks)controllers/activityController.js(1 hunks)controllers/todoController.js(4 hunks)routes/activityRoutes.js(1 hunks)server.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
routes/activityRoutes.js (1)
controllers/todoController.js (1)
activityController(2-2)
controllers/activityController.js (3)
config/database.js (1)
database(99-99)controllers/todoController.js (1)
database(1-1)server.js (1)
database(7-7)
controllers/todoController.js (1)
routes/activityRoutes.js (1)
activityController(3-3)
🔇 Additional comments (7)
README.md (3)
15-16: LGTM! Good documentation of the new activities feature.The documentation clearly describes the new activity tracking and analytics capabilities being introduced.
84-92: LGTM! Comprehensive API documentation for activities endpoints.The activities API endpoints are well-documented with appropriate query parameters and clear descriptions.
162-175: LGTM! Clear database schema documentation for the activities table.The activities table schema is properly documented with appropriate foreign key relationship to the todos table.
controllers/activityController.js (1)
26-26: SQL injection vulnerability with parameter parsing.The
parseInt()calls on Lines 26 should be done before using the values in SQL parameters to ensure type safety.Parse the values before using them:
- sql += ' ORDER BY a.created_at DESC LIMIT ? OFFSET ?'; - params.push(parseInt(limit), parseInt(offset)); + sql += ' ORDER BY a.created_at DESC LIMIT ? OFFSET ?'; + params.push(limit, offset); // Already parsed as integers aboveLikely an incorrect or invalid review comment.
controllers/todoController.js (1)
2-2: Import path is correct for sibling controller.
No action needed.server.js (1)
9-9: Routes wiring LGTM.
Importing and mounting /api/activities is correct and doesn’t interfere with existing routes.Also applies to: 22-22
routes/activityRoutes.js (1)
1-24: Router and ordering look correct.
Specific routes (/stats, /todo/:todoId) are declared before '/:id', avoiding shadowing. Exports are clean.
| async getAllActivities(req, res) { | ||
| try { | ||
| const db = database.getConnection(); | ||
| const { page = 1, limit = 50, todo_id } = req.query; | ||
| const offset = (page - 1) * limit; | ||
|
|
||
| let sql = ` | ||
| SELECT | ||
| a.*, | ||
| t.title as todo_title | ||
| FROM activities a | ||
| LEFT JOIN todos t ON a.todo_id = t.id | ||
| `; | ||
| let params = []; | ||
|
|
||
| if (todo_id) { | ||
| sql += ' WHERE a.todo_id = ?'; | ||
| params.push(todo_id); | ||
| } | ||
|
|
||
| sql += ' ORDER BY a.created_at DESC LIMIT ? OFFSET ?'; | ||
| params.push(parseInt(limit), parseInt(offset)); | ||
|
|
||
| db.all(sql, params, (err, rows) => { | ||
| if (err) { | ||
| res.status(500).json({ error: err.message }); | ||
| return; | ||
| } | ||
|
|
||
| // Get total count for pagination | ||
| let countSql = 'SELECT COUNT(*) as total FROM activities'; | ||
| let countParams = []; | ||
|
|
||
| if (todo_id) { | ||
| countSql += ' WHERE todo_id = ?'; | ||
| countParams.push(todo_id); | ||
| } | ||
|
|
||
| db.get(countSql, countParams, (err, countResult) => { | ||
| if (err) { | ||
| res.status(500).json({ error: err.message }); | ||
| return; | ||
| } | ||
|
|
||
| res.json({ | ||
| activities: rows, | ||
| pagination: { | ||
| page: parseInt(page), | ||
| limit: parseInt(limit), | ||
| total: countResult.total, | ||
| pages: Math.ceil(countResult.total / limit) | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
| } catch (error) { | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add input validation for pagination parameters.
The pagination parameters are parsed directly without validation, which could lead to negative offsets or invalid limits.
Add validation for the pagination parameters:
async getAllActivities(req, res) {
try {
const db = database.getConnection();
- const { page = 1, limit = 50, todo_id } = req.query;
+ let { page = 1, limit = 50, todo_id } = req.query;
+
+ // Validate and sanitize pagination parameters
+ page = Math.max(1, parseInt(page) || 1);
+ limit = Math.min(100, Math.max(1, parseInt(limit) || 50));
+
const offset = (page - 1) * limit;📝 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 getAllActivities(req, res) { | |
| try { | |
| const db = database.getConnection(); | |
| const { page = 1, limit = 50, todo_id } = req.query; | |
| const offset = (page - 1) * limit; | |
| let sql = ` | |
| SELECT | |
| a.*, | |
| t.title as todo_title | |
| FROM activities a | |
| LEFT JOIN todos t ON a.todo_id = t.id | |
| `; | |
| let params = []; | |
| if (todo_id) { | |
| sql += ' WHERE a.todo_id = ?'; | |
| params.push(todo_id); | |
| } | |
| sql += ' ORDER BY a.created_at DESC LIMIT ? OFFSET ?'; | |
| params.push(parseInt(limit), parseInt(offset)); | |
| db.all(sql, params, (err, rows) => { | |
| if (err) { | |
| res.status(500).json({ error: err.message }); | |
| return; | |
| } | |
| // Get total count for pagination | |
| let countSql = 'SELECT COUNT(*) as total FROM activities'; | |
| let countParams = []; | |
| if (todo_id) { | |
| countSql += ' WHERE todo_id = ?'; | |
| countParams.push(todo_id); | |
| } | |
| db.get(countSql, countParams, (err, countResult) => { | |
| if (err) { | |
| res.status(500).json({ error: err.message }); | |
| return; | |
| } | |
| res.json({ | |
| activities: rows, | |
| pagination: { | |
| page: parseInt(page), | |
| limit: parseInt(limit), | |
| total: countResult.total, | |
| pages: Math.ceil(countResult.total / limit) | |
| } | |
| }); | |
| }); | |
| }); | |
| } catch (error) { | |
| res.status(500).json({ error: 'Internal server error' }); | |
| } | |
| } | |
| async getAllActivities(req, res) { | |
| try { | |
| const db = database.getConnection(); | |
| let { page = 1, limit = 50, todo_id } = req.query; | |
| // Validate and sanitize pagination parameters | |
| page = Math.max(1, parseInt(page, 10) || 1); | |
| limit = Math.min(100, Math.max(1, parseInt(limit, 10) || 50)); | |
| const offset = (page - 1) * limit; | |
| // … rest of method remains unchanged … |
🤖 Prompt for AI Agents
In controllers/activityController.js around lines 5 to 63, the pagination query
params (page, limit) are parsed without validation which can produce negative
offsets or invalid limits; validate and normalize these inputs by parsing them
with parseInt(base 10), treating NaN or values < 1 as the default (page = 1,
limit = 50), enforce an upper bound on limit (e.g., maxLimit = 100) and use
Math.max to ensure page and limit are at least 1, then recompute offset = (page
- 1) * limit using the validated values; also validate/convert todo_id if
provided (ensure it’s an integer or ignore/return a 400) and use the validated
params in the SQL params and pagination metadata.
| // Clear all activities (admin function) | ||
| async clearAllActivities(req, res) { | ||
| try { | ||
| const db = database.getConnection(); | ||
|
|
||
| db.run('DELETE FROM activities', function(err) { | ||
| if (err) { | ||
| res.status(500).json({ error: err.message }); | ||
| return; | ||
| } | ||
| res.json({ | ||
| message: 'All activities cleared successfully', | ||
| deletedCount: this.changes | ||
| }); | ||
| }); | ||
| } catch (error) { | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Security concern: clearAllActivities lacks authorization check.
The clearAllActivities method is destructive and should require proper authorization before allowing deletion of all audit logs.
This endpoint should verify admin privileges before execution. Consider adding middleware for authorization:
// In your route definition, add authorization middleware:
router.delete('/', requireAdmin, activityController.clearAllActivities);Also consider adding a confirmation mechanism to prevent accidental deletion.
🤖 Prompt for AI Agents
In controllers/activityController.js around lines 184 to 202, the
clearAllActivities handler is missing any authorization or confirmation before
performing a destructive delete; add an authorization check at the start of the
function (e.g., verify req.user exists and that req.user.role === 'admin' or
req.user.isAdmin) and return res.status(403) if unauthorized, and require an
explicit confirmation flag in the request (for example a boolean confirm=true in
req.body or a specific admin token) before running the DELETE; alternatively
ensure the route that wires this controller uses a requireAdmin middleware
(router.delete('/', requireAdmin, activityController.clearAllActivities)) and
reject requests without the confirmation parameter to prevent accidental
deletion.
| async (err) => { | ||
| if (err) { | ||
| res.status(500).json({ error: err.message }); | ||
| return; | ||
| } | ||
| res.status(201).json({ | ||
| id: this.lastID, | ||
|
|
||
| const todoId = this.lastID; | ||
| const newTodo = { | ||
| id: todoId, | ||
| title: title.trim(), | ||
| description: description ? description.trim() : '', | ||
| completed: false, | ||
| completed: false | ||
| }; | ||
|
|
||
| // Log the creation activity | ||
| await this.logActivity( | ||
| todoId, | ||
| 'CREATE', | ||
| `Todo "${title.trim()}" was created`, | ||
| null, | ||
| newTodo, | ||
| req | ||
| ); | ||
|
|
||
| res.status(201).json({ | ||
| ...newTodo, | ||
| message: 'Todo created successfully' |
There was a problem hiding this comment.
Bug: this.lastID will be undefined due to arrow callback; use function() to access sqlite3 Statement context.
Arrow functions don’t bind their own this; you’ll never get the inserted id. Capture the controller instance to call logActivity.
Apply this diff:
- db.run(
+ const self = this;
+ db.run(
'INSERT INTO todos (title, description) VALUES (?, ?)',
[title.trim(), description ? description.trim() : ''],
- async (err) => {
+ function (err) {
if (err) {
res.status(500).json({ error: err.message });
return;
}
-
- const todoId = this.lastID;
+ const todoId = this.lastID;
const newTodo = {
id: todoId,
title: title.trim(),
description: description ? description.trim() : '',
completed: false
};
- // Log the creation activity
- await this.logActivity(
- todoId,
- 'CREATE',
- `Todo "${title.trim()}" was created`,
- null,
- newTodo,
- req
- );
-
- res.status(201).json({
- ...newTodo,
- message: 'Todo created successfully'
- });
+ // Fire-and-forget to avoid blocking main path
+ void self.logActivity(
+ todoId,
+ 'CREATE',
+ `Todo "${title.trim()}" was created`,
+ null,
+ newTodo,
+ req
+ );
+
+ res.status(201).json({ ...newTodo, message: 'Todo created successfully' });
}
);📝 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 (err) => { | |
| if (err) { | |
| res.status(500).json({ error: err.message }); | |
| return; | |
| } | |
| res.status(201).json({ | |
| id: this.lastID, | |
| const todoId = this.lastID; | |
| const newTodo = { | |
| id: todoId, | |
| title: title.trim(), | |
| description: description ? description.trim() : '', | |
| completed: false, | |
| completed: false | |
| }; | |
| // Log the creation activity | |
| await this.logActivity( | |
| todoId, | |
| 'CREATE', | |
| `Todo "${title.trim()}" was created`, | |
| null, | |
| newTodo, | |
| req | |
| ); | |
| res.status(201).json({ | |
| ...newTodo, | |
| message: 'Todo created successfully' | |
| const self = this; | |
| db.run( | |
| 'INSERT INTO todos (title, description) VALUES (?, ?)', | |
| [title.trim(), description ? description.trim() : ''], | |
| function (err) { | |
| if (err) { | |
| res.status(500).json({ error: err.message }); | |
| return; | |
| } | |
| const todoId = this.lastID; | |
| const newTodo = { | |
| id: todoId, | |
| title: title.trim(), | |
| description: description ? description.trim() : '', | |
| completed: false | |
| }; | |
| // Fire-and-forget to avoid blocking main path | |
| void self.logActivity( | |
| todoId, | |
| 'CREATE', | |
| `Todo "${title.trim()}" was created`, | |
| null, | |
| newTodo, | |
| req | |
| ); | |
| res.status(201).json({ ...newTodo, message: 'Todo created successfully' }); | |
| } | |
| ); |
🤖 Prompt for AI Agents
In controllers/todoController.js around lines 81 to 107, the DB callback uses an
arrow function so this.lastID is undefined and you lose the controller context
when calling logActivity; before invoking the DB run, capture the controller
instance (e.g., const controller = this), change the callback to an async
function(err) { ... } so you can read the sqlite Statement context via
this.lastID to set todoId, and inside the callback call
controller.logActivity(...) (not this.logActivity) so you retain the controller
reference while still using this.lastID from the Statement.
|
@coderabbitai generate unit tests |
1 similar comment
|
@coderabbitai generate unit tests |
|
Unit Test Generation failed: CodeRabbit timed out while planning changes for tests/controllers/activityController.test.js. Skipping this file. |
✅ Actions performedUnit Test Generation started... |
|
Unit Test Generation failed: CodeRabbit timed out while planning changes for tests/server.test.js. Skipping this file. |
✅ Actions performedUnit Test Generation started... |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Unit Test Generation process completed. You can view the generated tests in this PR: View PR |
✅ Actions performedUnit Test Generation started... |
|
Unit Test Generation failed: CodeRabbit timed out while planning changes for controllers/activityController.test.js. Skipping this file. |
✅ Actions performedUnit Test Generation started... |
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores