Skip to content

fix(platform): read quoted CSV cells across line breaks - #4235

Merged
yannickmonney merged 2 commits into
mainfrom
fix/csv-import-quoted-multiline
Oct 4, 2026
Merged

yannickmonney merged 2 commits into
mainfrom
fix/csv-import-quoted-multiline

Conversation

@yannickmonney

@yannickmonney yannickmonney commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

What changed

The CSV import parser split the file on every \n before 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 a Widget with only First line, price 0 and stock 0, plus a fabricated product named Second line,12,3, also at 0/0.

  • parseCSVRecords replaces parseCSVLine plus the split('\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.
  • A quote opens a quoted field only at the start of a field, after optional whitespace, as before. Inside an unquoted cell (6" long) it is a literal character. On main it opened a quoted section mid-cell, which with record-level reading would have run into the next rows.
  • Quotes that don't pair up fail clearly (second commit, after review):
    • Two cases count: a quoted field that never closes, and text after a closing quote.
    • The record is refused by its line through the existing row-error list, with a new translated sentence, 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. …"
    • An unclosed quote is still read as a literal character for recovery, so it never swallows the rows after it. The other rows import as before.
    • A header whose quotes don't pair up refuses the file at row 1, so nothing imports under misread column names.
    • ImportRowError gains { row, quotes: 'unpaired' }, and importRowErrorMessage renders it.
  • These are unchanged:
    • field trimming;
    • blank-line skipping, now per record, so the whole-file trim() is gone;
    • ,,,, rows skipped by isBlankRecord;
    • header lower-casing;
    • positional mode;
    • row numbering (firstLine + index);
    • the Excel path.
  • New tests:
    • lib/utils/file-parsing.test.ts:
      • the issue's SheetJS sheet_to_csv fixture, asserted byte for byte against the issue's CSV;
      • commas, escaped quotes and a CRLF break inside quoted cells of a CRLF file (RS: '\r\n');
      • positional mode;
      • a control for plain rows, one-line quoted commas, and blank lines around the header;
      • the inch mark;
      • an unclosed quote: its row is refused, and the next row imports;
      • text after a closing quote, including two stray quotes that pair across lines and merge two rows: both are refused, and the rest imports;
      • a malformed header refusing the file.
    • app/hooks/use-file-import.test.ts: the fixture's bytes go as a File through parseImportFile (jsdom FileReader) and productMappers.record. The result is exactly one Widget with the whole description, price 12, stock 3 and USD.
    • app/features/shared/import/import-row-errors.test.ts: the new row error renders as Row 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 parseCSVText and append tests to the same two test files. Whichever lands second resolves a small textual conflict.

  • Both drop the whole-file trim().
  • parseCSVRecords returns every record, blank lines included. fix(platform): preserve CSV import source row numbers #4201's numbering therefore maps onto parseCSVRecords(csvText, delimiter).entries() with the same line: index + 1, kept beside values and unpairedQuotes.
  • That number is the record's position. It equals the physical line everywhere except after a multi-line cell. There, it is the row a spreadsheet shows, which is what ImportRowError documents ("the line the user sees in a spreadsheet").
  • This PR pins that: the row after a two-line cell is row 3 (rows: [2, 3]).
  • A local trial merge of the first head with fix(platform): preserve CSV import source row numbers #4201 passed both PRs' tests (comment below). The register row sits beside the import dialogs' row, away from fix(platform): preserve CSV import source row numbers #4201's insertion at the top of the table.

How I verified it

All runs were local and light, at the exact head a2276d6de5946096b5516f630c5bb630d75e3fc7. No full platform suite, workspace tsc, SAST, stack, browser or E2E was run; CI runs those.

  • Reproduced first on origin/main 8a580fcc2.
    • The new parser tests failed and the existing ones passed.
    • The product test through parseImportFile returned the issue's exact output: Widget / First line / 0 / 0 and Second line,12,3 / 0 / 0.
  • Parser and i18n: bunx vitest run --project server lib/utils/file-parsing.test.ts lib/i18n/messages.test.ts passed 38/38: parser 14, and the catalog parity and usage checks.
  • Hook, row-error mapper and alert, both import dialogs and contacts CRUD: bunx vitest --run --config vitest.ui.config.ts on use-file-import.test.ts, import-row-errors.test.ts, import-row-errors-alert.test.tsx, products-import-dialog.test.tsx, contacts-import-dialog.test.tsx and contacts-crud.test.tsx passed: 6 files, 70 tests.
  • Mutation checks. Each mutation was reverted afterwards. Each failed exactly the test written for its rule:
    1. an unclosed quote running to the end of the file;
    2. a quote opening anywhere;
    3. dropping the last line;
    4. not skipping blank lines;
    5. ignoring text after a closing quote;
    6. not flagging an unclosed quote;
    7. not refusing a malformed header.
  • Round-trip fuzz (local, not committed).
    • Setup: 3,000 seeded random tables (commas, ", "", inch marks, LF/CRLF breaks, tabs, padding) went through SheetJS sheet_to_csv with LF and CRLF record separators, then back through parseCSVWithMapper.
    • Result: every table read back exactly, and none was flagged as unpaired. main's parser fails at the first seed.
    • The harness writes CRLF inside cells of CRLF files, because SheetJS with a non-default 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.
  • Lint and format:
    • bunx oxlint --type-aware on 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.
    • commitlint on both commits: clean.
  • Scoped TypeScript: a temporary tsconfig extending tsconfig.dom.json over 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

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
@yannickmonney yannickmonney added bug Bug service: platform Platform service labels Oct 4, 2026
@yannickmonney

Copy link
Copy Markdown
Contributor Author

Compatibility with #4201, checked by a local trial merge (never pushed): this head c921061abd5f480057997212daeb491a94e3bd99 merged with #4201's head 2f6fc72d39290f049b0ed4f2029b0a6139a3be59.

Conflicts. All are textual: parseCSVText, the two appended test blocks, the two xlsx/parseImportFile test imports, and automation.md. The automation.md conflict is between #4201's row and a newer main row, not this PR's row.

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:

  • vitest --project server lib/utils/file-parsing.test.ts: 14/14. That is this PR's 12, plus fix(platform): preserve CSV import source row numbers #4201's positional-mode case with skipEmptyLines on and off, which expects rows [2, 7], error row 5, and indexes [0, 1, 2] / [1, 4, 6].
  • vitest --config vitest.ui.config.ts on use-file-import, import-row-errors, the products and contacts import dialogs, and contacts CRUD: 5 files, 68/68. These include fix(platform): preserve CSV import source row numbers #4201's SheetJS CSV/XLSX row-provenance tests, its blank-line fixtures, and this PR's multi-line product test.
  • A scoped type-check of the merged files reports no diagnostics.

So whichever PR lands second needs only this resolution. A row after a multi-line cell keeps its spreadsheet number, because #4201's line counts records once records span lines.

@yannickmonney yannickmonney left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review — REQUEST CHANGES at exact head c921061abd5f480057997212daeb491a94e3bd99.

Evidence:

  • bunx vitest run --project server lib/utils/file-parsing.test.ts passed 12/12 at this head.
  • bunx vitest run --config vitest.ui.config.ts app/hooks/use-file-import.test.ts was attempted, but suite setup failed before tests with TypeError: undefined is not an object (evaluating 'z.enum') from lib/shared/schemas/common.ts:62; this is an environment/dependency-resolution blocker, not a product-test result.\n- bunx oxlint --type-aware and bunx oxfmt --check passed 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 added reads a quote that never closes as a literal character test expects an unterminated quoted field to be accepted into data with errors: []; parseCSVRecords deliberately 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.
@yannickmonney

yannickmonney commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor Author

Re: the independent review at c921061ab (malformed quoting does not fail clearly). Addressed in a2276d6de5946096b5516f630c5bb630d75e3fc7, fix(platform): refuse CSV rows whose quotes don't pair up.

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 errors: [] is the data-integrity risk this issue is about, and it was the first decision I flagged for challenge. So the change is:

  • What counts as unpaired: a quoted field that never closes, or text after a closing quote ("Best" widget). Two stray quotes that pair across lines and merge two rows produce that text, so they are caught too.
  • What the user sees: the record is refused by its line through the existing row-error list. The new sentence common.import.unpairedQuotes is in en, de and fr; de-CH inherits de, which has no ß. The dialog shows "Row 3: The quotes in this row don't pair up. …", and the other rows import as before.
  • Recovery is unchanged: an unclosed quote is still read as a literal character, so it never swallows the rows after it.
  • A header whose quotes don't pair up refuses the file at row 1, so nothing imports under misread column names.
  • A quote inside an unquoted cell (6" long) stays a literal character. Excel reads it that way, RFC-exported files never contain it, and refusing it would reject common hand-made files. Please flag it if you want that refused too.
  • The type: ImportRowError gains { row, quotes: 'unpaired' }, and importRowErrorMessage renders it.

Tests:

  • New:
    • an unclosed quote is refused and the next row imports;
    • text after a closing quote, including the cross-line merge, is refused;
    • a malformed header refuses the file;
    • rendering, and the en/de/fr sentence with no placeholder.
  • Three new mutations each fail their test: ignoring text after a closing quote; not flagging an unclosed quote; not refusing a malformed header.
  • The SheetJS round-trip fuzz (3,000 seeded tables, LF and CRLF) still reads every valid table back exactly, with no false refusal.

Exact-head checks at a2276d6de:

  • parser plus lib/i18n/messages.test.ts: 38/38;
  • the 6 UI import suites: 70/70;
  • oxlint, oxfmt, git diff --check, conflict markers, lint:manual and commitlint: clean;
  • the scoped type-check over 2,609 files: no diagnostics.

Your z.enum setup error (corrected; an earlier version of this comment blamed the config, but you did use --config vitest.ui.config.ts). undefined is not an object (evaluating 'z.enum') is JavaScriptCore's wording, which suggests that run executed the suite on Bun's runtime. Here, bunx vitest starts vitest's #!/usr/bin/env node bin on Node v24.21.0, and bunx vitest --run --config vitest.ui.config.ts app/hooks/use-file-import.test.ts passes.

#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 row: 1 should become the header record's own line, if blank lines precede it.

@yannickmonney yannickmonney left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:207 marks EOF inside quotes as unpaired before replaying that field unquoted. The malformed record is refused, while subsequent plain records remain importable. file-parsing.ts:240 marks 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:310 refuses a malformed header before mapping any data; file-parsing.ts:330 refuses malformed data before calling the mapper. import-row-errors.ts:49 renders 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, and file-parsing.test.ts:192 cover 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, parseImportFile and 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 at 04d0b1f3175868fdaf80c16c4ac96d5f32feec96: it replaces the same row representation and loop, so the second PR must deliberately integrate values, record position and unpairedQuotes, 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 1 on 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/.

@yannickmonney
yannickmonney merged commit e3a5645 into main Oct 4, 2026
72 checks passed
@yannickmonney
yannickmonney deleted the fix/csv-import-quoted-multiline branch October 4, 2026 11:06
yannickmonney pushed a commit that referenced this pull request Oct 8, 2026
Rebuilt on main cef9c48 to resolve the conflict with #4235 (TALE-127 repair).
yannickmonney pushed a commit that referenced this pull request Oct 8, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug service: platform Platform service

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Quoted multiline CSV imports silently split one product into two zero-price products

1 participant