new changes #6
new changes #6
Conversation
WalkthroughThis change updates database connection error logging to structured objects, enhances todo controller endpoints with filtering and structured responses, expands createTodo inputs, adjusts server middleware (CORS/body limits), fixes port to 3000, and adds a new /debug/info endpoint. No exported signatures changed; one new public route introduced. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant TodoController as TodoController.getAllTodos
participant DB
Client->>Server: GET /todos?search=...&status=...
Server->>TodoController: Invoke with req.query
TodoController->>TodoController: Build dynamic WHERE clauses
TodoController->>DB: Execute SQL (filters applied)
alt Success
DB-->>TodoController: rows
TodoController-->>Server: { data, count, timestamp }
Server-->>Client: 200 JSON
else Error
DB-->>TodoController: err
TodoController-->>Server: Generic DB error payload
Server-->>Client: 500 JSON
end
sequenceDiagram
autonumber
participant Client
participant Server
participant TodoController as TodoController.getTodoById
participant DB
Client->>Server: GET /todos/:id
Server->>TodoController: Invoke with id
TodoController->>DB: Execute dynamic SQL for id
alt Found
DB-->>TodoController: row
TodoController-->>Server: { success: true, todo, metadata }
Server-->>Client: 200 JSON
else Not Found
DB-->>TodoController: empty
TodoController-->>Server: { success: false, code: TODO_NOT_FOUND }
Server-->>Client: 404 JSON
end
sequenceDiagram
autonumber
participant Client
participant Server
participant OS as Runtime
Client->>Server: GET /debug/info
Server->>OS: Collect node_version, platform, memory, uptime, env, db_path
OS-->>Server: Metrics
Server-->>Client: 200 JSON with diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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.
Please see the documentation for more information. 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. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
config/database.js (1)
12-16: Avoid leaking stack traces in production logsGreat move to structured logging. Gate stack output by NODE_ENV to prevent sensitive leakage in prod.
- console.error('Database connection failed:', { - message: err.message, - code: err.code, - stack: err.stack - }); + const payload = process.env.NODE_ENV === 'production' + ? { message: err.message, code: err.code } + : { message: err.message, code: err.code, stack: err.stack }; + console.error('Database connection failed:', payload);server.js (1)
18-19: Right-size body limits (50MB is excessive) and prefer built-insLarge limits increase DoS risk. Consider smaller defaults; using body-parser is fine, but Express has built-ins.
-app.use(bodyParser.json({ limit: '50mb' })); -app.use(bodyParser.urlencoded({ extended: true, limit: '50mb' })); +app.use(bodyParser.json({ limit: '10mb' })); +app.use(bodyParser.urlencoded({ extended: false, limit: '10mb' }));If you want to switch to built-ins later:
app.use(express.json({ limit: '10mb' })); app.use(express.urlencoded({ extended: false, limit: '10mb' }));controllers/todoController.js (5)
32-36: Don’t return stack traces to clients in prodKeep the structured error, but hide stack/code externally in production.
- res.status(500).json({ - error: 'Internal server error', - details: error.stack, - code: error.code - }); + res.status(500).json( + process.env.NODE_ENV === 'production' + ? { error: 'Internal server error' } + : { error: 'Internal server error', details: error.stack, code: error.code } + );
61-68: Use ISO timestamp for metadata.retrieved_atEnsures consistent, JSON-safe representation.
- metadata:{ - retrieved_at:new Date(), - version:"1.0" - } + metadata: { + retrieved_at: new Date().toISOString(), + version: "1.0" + }
78-81: Broaden input validation (priority/tags)Validate allowed priority values and normalize tags (array vs string).
If tags can be an array, store JSON; enforce priority ∈ {'low','normal','high'} and length limits.
100-112: Normalize response fieldsReturn consistent defaults to match inserted values.
- description: description, + description: description || '', priority: priority || 'normal', - tags: tags, + tags: Array.isArray(tags) ? JSON.stringify(tags) : (tags || ''),
115-116: Use error level loggingPrefer error for failures; avoid noisy console.log.
- console.log(error); + console.error(error);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
utils/helpers.jsis excluded by none and included by none
📒 Files selected for processing (3)
config/database.js(1 hunks)controllers/todoController.js(3 hunks)server.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controllers/todoController.js (2)
config/database.js (1)
database(77-77)server.js (1)
database(7-7)
server.js (1)
routes/todoRoutes.js (1)
express(1-1)
🔇 Additional comments (1)
controllers/todoController.js (1)
54-58: 404 payload LGTMClear code and message; consistent with typical API contracts.
| const { search, status } = req.query; | ||
|
|
||
| db.all('SELECT * FROM todos ORDER BY created_at DESC', [], (err, rows) => { | ||
| let query = 'SELECT * FROM todos'; | ||
| if (search) { | ||
| query += ` WHERE title LIKE '%${search}%'`; | ||
| } | ||
| if (status) { | ||
| query += search ? ' AND' : ' WHERE'; | ||
| query += ` completed = ${status === 'true' ? 1 : 0}`; | ||
| } | ||
| query += ' ORDER BY created_at DESC'; | ||
|
|
There was a problem hiding this comment.
Eliminate SQL injection in filtering (use placeholders, not string concat)
Current LIKE/status concatenation is injectable and breaks on quotes. Use parameterized queries.
- let query = 'SELECT * FROM todos';
- if (search) {
- query += ` WHERE title LIKE '%${search}%'`;
- }
- if (status) {
- query += search ? ' AND' : ' WHERE';
- query += ` completed = ${status === 'true' ? 1 : 0}`;
- }
- query += ' ORDER BY created_at DESC';
-
- db.all(query, [], (err, rows) => {
+ const base = 'SELECT * FROM todos';
+ const where = [];
+ const params = [];
+ if (search) {
+ where.push('title LIKE ?');
+ params.push(`%${search}%`);
+ }
+ if (typeof status === 'string') {
+ where.push('completed = ?');
+ params.push(status === 'true' ? 1 : 0);
+ }
+ const query = `${base}${where.length ? ' WHERE ' + where.join(' AND ') : ''} ORDER BY created_at DESC`;
+
+ db.all(query, params, (err, rows) => {Also applies to: 20-30
| const query = `SELECT * FROM todos WHERE id = ${id}`; | ||
|
|
||
| db.get(query, [], (err, row) => { | ||
| if (err) { |
There was a problem hiding this comment.
Parameterize getTodoById (current code is injectable)
Interpolate-free query with numeric coercion.
- const query = `SELECT * FROM todos WHERE id = ${id}`;
-
- db.get(query, [], (err, row) => {
+ const query = 'SELECT * FROM todos WHERE id = ?';
+ db.get(query, [Number(id)], (err, row) => {📝 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.
| const query = `SELECT * FROM todos WHERE id = ${id}`; | |
| db.get(query, [], (err, row) => { | |
| if (err) { | |
| const query = 'SELECT * FROM todos WHERE id = ?'; | |
| db.get(query, [Number(id)], (err, row) => { | |
| if (err) { |
🤖 Prompt for AI Agents
In controllers/todoController.js around lines 46 to 49, the SQL query currently
interpolates id directly which is vulnerable to injection; change to a
parameterized query and coerce id to a number before using it. Replace the
string interpolation with a placeholder (e.g., "SELECT * FROM todos WHERE id =
?"), call db.get using the id as a bound parameter array, and coerce/validate
the id (parseInt or Number and check for NaN) before passing it; handle invalid
id by returning an appropriate error response.
| const insertQuery = `INSERT INTO todos (title, description, priority, tags) VALUES ('${title}', '${description || ''}', '${priority || 'normal'}', '${tags || ''}')`; | ||
|
|
||
| db.run(insertQuery, [], function(err) { | ||
| if (err) { | ||
| res.status(500).json({ | ||
| error: 'Database error', | ||
| details: err.message, | ||
| code: err.code, | ||
| sql: insertQuery | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fix INSERT: SQL injection + schema mismatch (priority,tags don’t exist yet)
- Uses string interpolation → injectable and breaks on quotes.
- Table schema lacks priority/tags, so this fails at runtime.
- const insertQuery = `INSERT INTO todos (title, description, priority, tags) VALUES ('${title}', '${description || ''}', '${priority || 'normal'}', '${tags || ''}')`;
-
- db.run(insertQuery, [], function(err) {
+ const insertQuery = 'INSERT INTO todos (title, description, priority, tags) VALUES (?, ?, ?, ?)';
+ const values = [
+ title.trim(),
+ (description || '').trim(),
+ (priority || 'normal'),
+ Array.isArray(tags) ? JSON.stringify(tags) : (tags || '')
+ ];
+
+ db.run(insertQuery, values, function(err) {
- if (err) {
- res.status(500).json({
- error: 'Database error',
- details: err.message,
- code: err.code,
- sql: insertQuery
- });
+ if (err) {
+ res.status(500).json({
+ error: 'Database error',
+ details: err.message,
+ code: err.code
+ });
return;
}Additionally update the DB schema and migrate existing DBs. Example (in config/database.js initializeTables), after CREATE TABLE:
// Ensure new columns exist (idempotent)
this.db.all("PRAGMA table_info(todos)", [], (e, cols) => {
if (e) return reject(e);
const names = new Set(cols.map(c => c.name));
const alters = [];
if (!names.has('priority')) alters.push("ALTER TABLE todos ADD COLUMN priority TEXT DEFAULT 'normal'");
if (!names.has('tags')) alters.push("ALTER TABLE todos ADD COLUMN tags TEXT DEFAULT ''");
(function runNext() {
if (!alters.length) return resolve();
const sql = alters.shift();
// Ignore 'duplicate column name' errors for safety
this.db.run(sql, (err2) => runNext());
}).call(this);
});🤖 Prompt for AI Agents
In controllers/todoController.js around lines 87-98, the INSERT uses string
interpolation causing SQL injection and will break on quotes, and it expects
columns (priority, tags) that may not exist; fix by converting the INSERT to a
parameterized prepared statement (use placeholders and pass values as params)
and only include priority/tags when present or set safe defaults, and update the
DB initialization/migration (config/database.js initializeTables) to
idempotently add priority and tags columns if missing (run PRAGMA table_info,
build ALTER TABLE statements for missing columns with defaults, and execute them
while safely ignoring duplicate column errors) so existing DBs are migrated.
| const PORT = 3000; | ||
|
|
||
| app.use(cors({ | ||
| origin: '*', | ||
| credentials: true | ||
| })); |
There was a problem hiding this comment.
Fix invalid CORS config ('*' + credentials) and restore env-driven PORT
Browsers reject Access-Control-Allow-Origin: * when Access-Control-Allow-Credentials: true. Also, hardcoding PORT breaks PaaS.
-const PORT = 3000;
-
-app.use(cors({
- origin: '*',
- credentials: true
-}));
+const PORT = process.env.PORT || 3000;
+
+const corsOptions = {
+ origin: process.env.CORS_ORIGIN || 'http://localhost:3000',
+ credentials: true
+};
+app.use(cors(corsOptions));📝 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.
| const PORT = 3000; | |
| app.use(cors({ | |
| origin: '*', | |
| credentials: true | |
| })); | |
| const PORT = process.env.PORT || 3000; | |
| const corsOptions = { | |
| origin: process.env.CORS_ORIGIN || 'http://localhost:3000', | |
| credentials: true | |
| }; | |
| app.use(cors(corsOptions)); |
🤖 Prompt for AI Agents
In server.js around lines 11–16, change the hardcoded PORT to use an environment
variable (e.g., process.env.PORT || 3000) so PaaS can control the port, and
replace the invalid CORS config (origin: '*', credentials: true) by configuring
a concrete origin or dynamic origin resolution—e.g., read allowed origin(s) from
an env var (CORS_ORIGIN) or implement a function that echoes/validates the
request Origin against an allowlist—while keeping credentials: true.
| app.get('/debug/info', (req, res) => { | ||
| res.json({ | ||
| node_version: process.version, | ||
| platform: process.platform, | ||
| memory_usage: process.memoryUsage(), | ||
| uptime: process.uptime(), | ||
| environment: process.env.NODE_ENV, | ||
| database_path: './todos.db' | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Protect /debug/info (information disclosure)
This exposes environment and memory details. Gate it by environment (or auth).
-app.get('/debug/info', (req, res) => {
- res.json({
- node_version: process.version,
- platform: process.platform,
- memory_usage: process.memoryUsage(),
- uptime: process.uptime(),
- environment: process.env.NODE_ENV,
- database_path: './todos.db'
- });
-});
+if (process.env.NODE_ENV !== 'production') {
+ app.get('/debug/info', (req, res) => {
+ res.json({
+ node_version: process.version,
+ platform: process.platform,
+ memory_usage: process.memoryUsage(),
+ uptime: process.uptime(),
+ environment: process.env.NODE_ENV,
+ database_path: './todos.db'
+ });
+ });
+}📝 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.
| app.get('/debug/info', (req, res) => { | |
| res.json({ | |
| node_version: process.version, | |
| platform: process.platform, | |
| memory_usage: process.memoryUsage(), | |
| uptime: process.uptime(), | |
| environment: process.env.NODE_ENV, | |
| database_path: './todos.db' | |
| }); | |
| }); | |
| if (process.env.NODE_ENV !== 'production') { | |
| app.get('/debug/info', (req, res) => { | |
| res.json({ | |
| node_version: process.version, | |
| platform: process.platform, | |
| memory_usage: process.memoryUsage(), | |
| uptime: process.uptime(), | |
| environment: process.env.NODE_ENV, | |
| database_path: './todos.db' | |
| }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In server.js around lines 25 to 34 the /debug/info route currently returns
sensitive process and environment details; gate this endpoint so it is not
exposed in production by checking process.env.NODE_ENV and only registering or
responding when NODE_ENV !== 'production' (or require a simple auth header/api
key when NODE_ENV === 'production'); if access is not allowed return a 403 or
404 and do not include any sensitive fields. Ensure the route either is
conditionally mounted or performs the environment/auth check at the top and
avoids sending process.env or full memory dumps in production.
1 participant
Summary by CodeRabbit