Code review example: Missing error path: one bad row aborts the whole import in Node

Retired from the DiffDojo practice pool in October 2026; the review below is the canonical answer. An AI wrote this pull request for the task underneath. Read the diff and decide what you would say before you scroll to the review.

Node · Edge cases & error handling · intermediate

The task

Import contacts from a CSV upload

1. Import each contact row from the parsed CSV.
2. A row that fails to save must not abort the rest of the import.
3. Return a summary: how many rows were imported and how many failed, with the failed row numbers.

Imports contacts from the parsed CSV rows one by one, so a single bad row can't take down the whole batch, and reports how many made it in. Ran it on a sample export and all rows imported.

Users upload real-world CSVs, a few malformed rows per file (bad emails, duplicates) are completely normal.

The diff

The review

Issue 1: Missing error path: one bad row aborts the whole import

src/imports/contacts.js, lines 4, 5, 6, 7

1. Every create is awaited with no try/catch, the first row that fails to save (a bad email or duplicate, which the context says is normal) throws straight out of importContacts, aborting the rest of the file. That directly violates 'a row that fails must not abort the rest'.
2. The failed array is declared but nothing ever populates it, so even the summary contract is broken: on a clean file it reports failed: [] truthfully, and on a dirty file the function never returns at all.
3. A sample file with zero bad rows can't reveal any of this, exactly the file the description tested with.

The fix: wrap the per-row create in try/catch, count failures with their row numbers, and keep going.

The fix

async function importContacts(rows) {
  let imported = 0;
  const failed = [];
  for (let i = 0; i < rows.length; i++) {
    try {
      await db.contacts.create(rows[i]);
      imported += 1;
    } catch (err) {
      failed.push(i + 1);
    }
  }
  return { imported, failed };
}

module.exports = { importContacts };

Why it matters

A declared-but-never-populated result field (like failed) → ask: "what code path actually fills this?" It's the tell of AI-generated code: the model scaffolded the summary the requirement asked for, then wrote only the happy path.

Real-world stakes: every clean test file passes. The first real customer CSV with one duplicate row rejects mid-file and the error propagates to the caller (an unhandled rejection if nothing up the chain catches it), half the contacts imported, no record of which, and support gets a 'your import ate my data' ticket that takes hours to untangle.

Read more: try catch inside for await loop, continue batch processing on row error, unhandled promise rejection node, partial import error summary pattern

The verdict

Request changes. Missing error path: one bad row aborts the whole import

How to review Node code · All code review examples