with the skill
## Summary
The import logic has a bug that will make the feature report the wrong number every time, so I'd hold off merging. It's a small fix.
## 🔴 [blocking] `forEach(async ...)` isn't awaited, so `imported` is returned as 0
```ts
rows.forEach(async (row) => {
...
await db.contacts.insert(contact);
imported++;
});
return { imported, total: rows.length };
```
`Array.prototype.forEach` ignores the promises returned by an async callback. Each callback runs until its first `await`, then `forEach` moves on. The function reaches `return` before any insert has finished, so the UI will show "Imported 0 of 3 contacts" even when every row is saved. The inserts do still run in the background, which can make a quick local test look fine if you checked the DB rather than the returned count.
## 🔴 [blocking] Errors become unhandled promise rejections
Show the rest (53 more lines)
Nothing catches errors inside the callback. If `toContact(row)` throws on a malformed row, or `insert` fails (duplicate key, constraint violation, connection error), the rejection is detached from the caller. On Node 15+ an unhandled rejection crashes the process by default. The caller also can't tell that anything went wrong.
## 🟡 [important] Unbounded concurrency
Even once awaited correctly, `rows.map(...)` with `Promise.all` fires every insert at once. A 50k-row file would exhaust the connection pool and starve other requests. Process rows sequentially, in bounded batches, or with a bulk insert.
## 🟡 [important] Decide what partial failure means
"Imported N of M" implies some rows can fail, but the result doesn't say which ones or why. Users will want to know which rows to fix. Also decide whether the import should be all-or-nothing (a transaction) or best-effort.
## Suggested fix (best-effort, sequential)
```ts
export interface ImportResult {
imported: number;
total: number;
failed: { index: number; error: string }[];
}
export async function importContacts(rows: CsvRow[], db: Db): Promise<ImportResult> {
let imported = 0;
const failed: ImportResult["failed"] = [];
for (const [index, row] of rows.entries()) {
try {
await db.contacts.insert(toContact(row));
imported++;
} catch (err) {
failed.push({ index, error: err instanceof Error ? err.message : String(err) });
}
}
return { imported, total: rows.length, failed };
}
```
If throughput matters, chunk the rows (e.g. 100 at a time) with `Promise.allSettled`, or use a bulk insert if `Db` supports one. If you'd rather it be all-or-nothing, wrap the loop in a transaction and let it throw.
## 💡 [suggestion] Tests
"Tested locally with a 3-row file" isn't enough here, and a basic test would have caught the first bug. Cases I'd want:
- All rows succeed, so `imported === total`.
- One row fails in `toContact` or `insert`, so it's counted in `failed` and the rest still import.
- Empty input returns `{ imported: 0, total: 0, failed: [] }`.
- Ideally, a larger input that checks concurrency stays bounded.
## ❓ Questions
- Should duplicates (same email) be skipped, updated, or reported as failures?
- Is there a max file or row size enforced upstream? I only see this one file, so I can't tell whether `csv.ts` or the upload handler limit it.
## Verdict
🔄 **Request changes.** The first two items must be fixed before merge. The rest can be handled here or in a follow-up. Happy to look at the updated version or the tests.