Repository navigation
fix(platform): read quoted CSV cells across line breaks - #4235
Conversation
The import parser split the file on every line break before reading quotes, so a spreadsheet export of a cell holding a line break became two records: the product kept the first line with price and stock 0, and a second zero-price product was named after the rest of the row. Read the text record by record instead: a quoted field holds the delimiter, "" for a quote and line breaks. A quote opens a field only at its start (an inch mark mid-cell stays literal), and a quote that never closes reads as a literal character, so an unbalanced quote cannot swallow the rows after it. Plain rows, one-line quoted commas, trimming, blank-line skipping and the Excel path are unchanged. Closes #3580
|
Compatibility with #4201, checked by a local trial merge (never pushed): this head Conflicts. All are textual: The parser resolution is the mapping described in the PR body: const rows: CSVParseOutput['rows'] = [];
for (const [index, values] of parseCSVRecords(csvText, delimiter).entries()) {
if (skipEmptyLines && isBlankLine(values)) continue;
rows.push({ values, line: index + 1 });
}Results on the merge:
So whichever PR lands second needs only this resolution. A row after a multi-line cell keeps its spreadsheet number, because #4201's |
yannickmonney
left a comment
There was a problem hiding this comment.
Independent review — REQUEST CHANGES at exact head c921061abd5f480057997212daeb491a94e3bd99.
Evidence:
bunx vitest run --project server lib/utils/file-parsing.test.tspassed 12/12 at this head.bunx vitest run --config vitest.ui.config.ts app/hooks/use-file-import.test.tswas attempted, but suite setup failed before tests withTypeError: undefined is not an object (evaluating 'z.enum')fromlib/shared/schemas/common.ts:62; this is an environment/dependency-resolution blocker, not a product-test result.\n-bunx oxlint --type-awareandbunx oxfmt --checkpassed on the four changed TypeScript files.\n- The SheetJS-shaped multiline fixture and RFC 4180 LF/CRLF/comma/escaped-quote cases are covered and pass in the parser suite; the baseline parser visibly splits on every LF, matching #3580’s reproduction.\n\nFinding: malformed quoting does not fail clearly. The addedreads a quote that never closes as a literal charactertest expects an unterminated quoted field to be accepted intodatawitherrors: [];parseCSVRecordsdeliberately converts it back to unquoted text. Other malformed forms such as a closing quote followed by non-delimiter text are likewise accepted. This conflicts with TALE-126’s explicit requirement that malformed quoting fail clearly without silently splitting or merging rows. Please add parser-level validation/error reporting for malformed quote structure while retaining valid multiline RFC 4180 behavior, and add a regression asserting the error.\n\nUnrun proof: no CI was rerun or cancelled, and no browser, E2E, stack, container, or full-workspace suite was run. GitHub would not allow the formal request-changes state because the authenticated account is the PR author, so this is recorded as a review comment instead.
Review of #4235: a quote that never closed was read as a literal and the row imported, and text after a closing quote was accepted, so a malformed row could land misread with no error. Mark a record whose quotes don't pair up (a quoted field that never closes, or text after a closing quote) and refuse it by line with the translated common.import.unpairedQuotes sentence, keeping the recovery that stops it from swallowing the rows after it. A header with such quotes refuses the file at row 1. Valid RFC 4180 records, including multi-line cells, read as before.
|
Re: the independent review at On the cited requirement: the text of TALE-126 and #3580 doesn't state one about malformed quoting. Its acceptance is to parse quoted records across lines, preserve newline, comma and escaped-quote content, give the fixture exactly one product at 12/3, and keep the simple controls passing. The finding stands on its merits, though. A misread row landing with
Tests:
Exact-head checks at
Your #4201 trial merge, redone at this head: parser 16/16, the UI import suites 74/74, and the merged type-check is clean. One follow-up for whichever PR lands second: under #4201's numbering, the header refusal's |
yannickmonney
left a comment
There was a problem hiding this comment.
ACCEPT — independent round-2 re-review (TALE-573 / TALE-126 / #3580)
Exact reviewed head: a2276d6de5946096b5516f630c5bb630d75e3fc7, checked at entry and immediately before posting. Reviewer agent af98a247-0225-43ff-b469-5b5b5702b4e0, run 57f1fc94-370e-4604-a59c-eeeb702d67d5, distinct from implementation agent #3 (4aec6e61). I read reply 5976871911 and the interdiff from c921061ab.
The prior malformed-quote finding is resolved; no blocking finding in this review scope:
file-parsing.ts:207marks EOF inside quotes as unpaired before replaying that field unquoted. The malformed record is refused, while subsequent plain records remain importable.file-parsing.ts:240marks non-whitespace after a closing quote as unpaired. The cited cross-line malformed pairing is refused rather than accepted as a fabricated product.file-parsing.ts:310refuses a malformed header before mapping any data;file-parsing.ts:330refuses malformed data before calling the mapper.import-row-errors.ts:49renders the new localized reason through the existing row-error list. EN/DE/FR catalog and rendering checks pass.- The regressions at
file-parsing.test.ts:158,file-parsing.test.ts:172, andfile-parsing.test.ts:192cover unterminated fields, trailing text (including cross-line pairing), and malformed headers. Independent additional probes cover LF/CRLF recovery, three malformed final-record variants, and the CSV FileReader path. - Valid multiline, commas, escaped quotes, positional and simple CSV controls pass. Through actual SheetJS CSV and XLSX bytes, jsdom FileReader,
parseImportFileand the production product mapper, the fixture yields exactly one Widget, complete multiline description, price 12, stock 3, USD, row 2, no errors. Existing Excel line-accounting tests pass. The Excel implementation is unchanged. - Current-head row-accounting assertions remain green, including spreadsheet-record numbering after a multiline cell (
[2, 3]). This is not a claim to fix #3581. I inspected #4201 at04d0b1f3175868fdaf80c16c4ac96d5f32feec96: it replaces the same row representation and loop, so the second PR must deliberately integratevalues, record position andunpairedQuotes, retain malformed-header refusal, and use the header record position when leading blanks exist. Keep both sets of regression tests. No fresh combined-head trial merge was run; the authors reported trial-merge evidence is not independently rerun evidence here.
Independently observed verification
Node v24.21.0, existing dependencies, one worker, 2 GiB Node heap:
bunx vitest run --project server --maxWorkers 1 lib/utils/file-parsing.test.ts lib/i18n/messages.test.ts: 38/38 pass.bunx vitest run --config vitest.ui.config.ts --maxWorkers 1on import hook, products dialog, contacts dialog, row-error mapper and alert: 65/65 pass; contacts CRUD separately: 5/5 pass.- Output-only independent probe configuration/suite: 8/8 pass. Total: 116 targeted tests pass, excluding the duplicate initial Bun-runtime parser run. Node rerun has a Storybook extensionless-import warning, no test failures. The import suites no longer encounter the previous round runtime/setup error when run on Node.
git diff c921061ab a2276d6de --check: clean. Tracked working tree: clean. Disk: 201 GiB free initially, 185 GiB at final evidence capture, above the 20 GiB floor.
Limitations and handoff
Correction to round 1: malformed quoting is not an explicit requirement in TALE-126/#3580. The data-integrity concern was valid and is now addressed on its merits; this re-review task explicitly asks to verify it. A literal inch mark inside an unquoted cell remains accepted by design, not a new strict-RFC rejection requirement. A malformed cross-line record is rejected as a unit; this does not promise reconstruction of every intended row from ambiguous invalid CSV.
Unrun: full suites, local lint/format/typecheck/SAST, browser/visual/E2E, real file-picker interaction, database import, author mutation/fuzz checks, and a combined #4201 trial merge. These are not claimed as independently observed. Passive CI snapshot: six Candidate source / Resolve source checks pending; no green-CI claim. No CI rerun/cancellation, push, merge, source changes or card moves.
This ACCEPT supersedes my c921061ab malformed-quote objection only for the exact head above. It is an independent source/behavior verdict, not a protected-human/native-task approval or merge authorization. The shared GitHub principal is also the PR author, so this is posted as a COMMENT review record, not self-approval. TALE-359 retains final CI, overlap integration and merge clearance. A changed head requires scoped revalidation.
Evidence: /agent/output/eac1002d-02ef-4f89-ba2e-8c987f337f33/.
A quoted cell's line breaks (LF or CRLF) belong to the cell, so a multi-line record is one spreadsheet row. parseCSVRecords counted every \n inside quotes, so the rows after such a cell were numbered one too high ([2,4] where #4235 expects [2,3]), and the quote-recovery replay recounted the same breaks. Number each record by its position instead: blank lines still count as rows, a malformed header keeps its own row, and the rows after an unclosed quote keep theirs. Format the parser and hook tests with oxfmt, and move the KNOW-B11 row into the coverage table.
What changed
The CSV import parser split the file on every
\nbefore it looked at quotes (lib/utils/file-parsing.ts,parseCSVText). A spreadsheet's CSV export of a cell holding a line break became two records, with no error. For the issue's fixture, products imported aWidgetwith onlyFirst line, price 0 and stock 0, plus a fabricated product namedSecond line,12,3, also at 0/0.parseCSVRecordsreplacesparseCSVLineplus thesplit('\n'). It reads the text record by record (RFC 4180): a quoted field holds the delimiter,""for a quote, and LF or CRLF line breaks.6" long) it is a literal character. Onmainit opened a quoted section mid-cell, which with record-level reading would have run into the next rows.common.import.unpairedQuotes(en/de/fr; de-CH inherits de, which has noß). For example: "Row 3: The quotes in this row don't pair up. …"ImportRowErrorgains{ row, quotes: 'unpaired' }, andimportRowErrorMessagerenders it.trim()is gone;,,,,rows skipped byisBlankRecord;firstLine + index);lib/utils/file-parsing.test.ts:sheet_to_csvfixture, asserted byte for byte against the issue's CSV;RS: '\r\n');app/hooks/use-file-import.test.ts: the fixture's bytes go as aFilethroughparseImportFile(jsdomFileReader) andproductMappers.record. The result is exactly oneWidgetwith the whole description, price 12, stock 3 and USD.app/features/shared/import/import-row-errors.test.ts: the new row error renders asRow N: …, and en/de/fr each carry the sentence.tests/manual/reference/automation.md: one row registers that coverage. The real picker, the database import and the dialog stay manual.Overlap with #4201 (CSV source row numbers, #3581)
Both PRs touch
parseCSVTextand append tests to the same two test files. Whichever lands second resolves a small textual conflict.trim().parseCSVRecordsreturns every record, blank lines included. fix(platform): preserve CSV import source row numbers #4201's numbering therefore maps ontoparseCSVRecords(csvText, delimiter).entries()with the sameline: index + 1, kept besidevaluesandunpairedQuotes.ImportRowErrordocuments ("the line the user sees in a spreadsheet").rows: [2, 3]).How I verified it
All runs were local and light, at the exact head
a2276d6de5946096b5516f630c5bb630d75e3fc7. No full platform suite, workspacetsc, SAST, stack, browser or E2E was run; CI runs those.origin/main8a580fcc2.parseImportFilereturned the issue's exact output:Widget/First line/ 0 / 0 andSecond line,12,3/ 0 / 0.bunx vitest run --project server lib/utils/file-parsing.test.ts lib/i18n/messages.test.tspassed 38/38: parser 14, and the catalog parity and usage checks.bunx vitest --run --config vitest.ui.config.tsonuse-file-import.test.ts,import-row-errors.test.ts,import-row-errors-alert.test.tsx,products-import-dialog.test.tsx,contacts-import-dialog.test.tsxandcontacts-crud.test.tsxpassed: 6 files, 70 tests.","", inch marks, LF/CRLF breaks, tabs, padding) went through SheetJSsheet_to_csvwith LF and CRLF record separators, then back throughparseCSVWithMapper.main's parser fails at the first seed.RS: '\r\n'leaves a cell holding a bare LF unquoted. That output is not valid RFC 4180 and reads as two records, as before.bunx oxlint --type-awareon the changed TypeScript files: clean.oxfmt --check: clean.git diff --check: clean.bun scripts/check-conflict-markers.ts: clean.bun run lint:manual: ok, 5 trees, 1,446 boxes.tsconfig.dom.jsonover the changed files, both import dialogs, the row-error helpers and the platform's ambient declarations (2,609 files) reported no diagnostics.No schema, migration, permission or dependency change. The only new user-visible string is
common.import.unpairedQuotes, in en, de and fr. The issue's acceptance holds: records parse across physical lines; embedded newline, comma and escaped-quote content is preserved; the fixture yields exactly one product at price 12 and stock 3; and the simple CSV controls still pass.Closes #3580