Uh oh!
There was an error while loading. Please reload this page.
fix(tables): sniff CSV delimiter and capture the real header row on import - #5888
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Headers are taken from the parser’s New Extensive tests cover delimiter edge cases, header derivation, and stream replay/destroy behavior. Reviewed by Cursor Bugbot for commit 35156cc. Configure here. |
Greptile SummaryAdds consistent, quote-aware CSV delimiter detection and authoritative header capture across table-import paths.
Confidence Score: 5/5The PR appears safe to merge. No blocking failures remain in the fixes related to the previous review threads. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[CSV upload or stored file] --> B[Read bounded file prefix]
B --> C[Trial-parse delimiter candidates]
C --> D[Select delimiter by width and consistency]
D --> E[Replay complete stream]
E --> F[csv-parse header callback]
F --> G[Schema inference and row import]
Reviews (8): Last reviewed commit: "test(tables): use sleep helper instead o..." | Re-trigger Greptile |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
waleedlatif1
commented
Jul 23, 2026
waleedlatif1
commented
Jul 23, 2026
@cursor review |
…mport Table CSV import derived the delimiter purely from the file extension, so semicolon-delimited exports (European-locale Excel) parsed as a single column. Header derivation also read Object.keys(rows[0]), so with relax_column_count a ragged/sparse first data row dropped its trailing columns from schema inference. - Add detectCsvDelimiter: trial-parse the head against ,/;/tab/| and pick the candidate with the most columns (tie-break on row-width consistency), quote-aware so separators inside quoted cells don't skew the guess; extension is now only the fallback - Add sniffCsvDelimiterFromStream: memory-bounded (64KB) peek-then-replay for the streaming import paths so multi-GB files sniff without buffering - Capture the true header row via the csv-parse columns callback on every path, fixing dropped columns when the first row is short - Wire into the create route, append/replace route, background runner, client dialog preview, and parseFileRows (copilot)
… buffer Addresses review findings on the delimiter sniffer: - Rank candidates by row-width consistency first (modal column count), then column count. Ranking on column count alone let a comma that appears in a semicolon file's unquoted header win over the real separator; the wide-but-ragged split now loses to the uniform one. - Move the partial-trailing-line trim into detectCsvDelimiter so every path (stream, client preview, parseFileRows) prepares the sniff sample identically and can't disagree on the delimiter. - Cap the stream peeker's detection copy at the sniff window via Buffer.concat's length arg and replay the buffered chunks by reference, so an oversized source chunk can't break the bounded-memory contract.
a5d29bc to
a6cc15cComparewaleedlatif1
commented
Jul 23, 2026
waleedlatif1
commented
Jul 23, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Addresses round-2 review findings: - Score each delimiter candidate by modalWidth * consistency instead of consistency alone. Consistency-only let a separator that appears uniformly inside values (e.g. a pipe once per row) beat a real delimiter whose rows are legitimately ragged; the product rewards a split that is both wide and uniform, with width breaking ties. Genuinely symmetric files (two valid columns under either separator) fall back to the global-default candidate order. - Dedupe the captured header row to match the parser's record keys. csv-parse collapses duplicate column names onto one key (last value wins), so reporting the raw duplicates made schema inference invent a phantom empty column and could fail mapping validation. dedupeHeaders keeps first-occurrence order, case-sensitively, matching the emitted keys.
waleedlatif1
commented
Jul 23, 2026
waleedlatif1
commented
Jul 23, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
detectCsvDelimiter always trimmed to the last newline, so a complete file with no trailing newline lost its final data row from scoring — leaving the header alone and letting a header-widening separator beat the real one. It now takes a `complete` flag: the trim runs only for truncated prefixes, where a mid-record cut must be dropped. The stream sniffer passes it from `exhausted`, and the buffered callers (client preview, parseFileRows) pass it when the sample covers the whole file.
waleedlatif1
commented
Jul 23, 2026
waleedlatif1
commented
Jul 23, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Hoist the csv-parse options out of the multi-line parse() call so the `// double-cast-allowed` annotation sits directly above the `as unknown as` token — the strict boundary audit only matches an annotation within three lines of the cast. Also simplify the stream sniffer's completeness check to `exhausted` (the loop only ends early on end-of-stream, so it already means the whole file fit the sniff window).
The stream sniffer's read loop stopped as soon as the buffered size reached the sniff window, so a file whose size is exactly the window never triggered the extra read that observes end-of-stream — it was judged a truncated prefix and dropped its final newline-less row, disagreeing with the buffered callers (which mark that size complete). Read while size <= window so the boundary case sees EOF and `exhausted` (hence `complete`) is set correctly. Regression test added.
waleedlatif1
commented
Jul 23, 2026
waleedlatif1
commented
Jul 23, 2026
@cursor review |
waleedlatif1
commented
Jul 23, 2026
waleedlatif1
commented
Jul 23, 2026
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 35156cc. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
waleedlatif1
commented
Jul 23, 2026
waleedlatif1
commented
Jul 23, 2026
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 35156cc. Configure here.
Summary
,;\t|), quote-aware, with the extension only as a fallback.Object.keys(rows[0]); withrelax_column_counta ragged/sparse first data row dropped its trailing columns from schema inference and mapping. The true header row is now captured via thecsv-parsecolumnscallback on every path — fixes the "sparse csv didn't work" report.detectCsvDelimiter(trial-parse each candidate, pick most columns, tie-break on row-width consistency) mirrors PapaParse/csv.Sniffer, andsniffCsvDelimiterFromStream(64KB peek-then-replay) keeps the streaming paths memory-bounded on multi-GB files.parseFileRows(copilot).Type of Change
Testing
alarms-exportfile: comma → 1 column, semicolon → 34 columns / 1249 records; sniffer detects;check:api-validationpassesChecklist