feat(csv): choose comma, semicolon or tab delimiters - #760
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cevheri
left a comment
There was a problem hiding this comment.
This is the best-shaped PR of the batch, and the hard part of #688 is the part you got right.
Its third item, "leave the RFC 4180 quoting untouched, only the separator becomes a parameter", is in tension with itself: the quoting predicate has to start testing the CHOSEN delimiter, or a semicolon inside a field in a semicolon file breaks the row. You resolved it the right way, by taking , out of NEEDS_QUOTING and adding text.includes(delimiter), and then you pinned the consequence: toCsv([{ amount: "1,5" }], ..., ";") asserts the comma stays BARE. That is the whole point of the issue, the European decimal comma, and it is the assertion most people would have missed. Putting , back into NEEDS_QUOTING turns two cases red, so it holds.
The parse side matches it: "Snow;""quote""" covers a quoted field carrying both the delimiter and an escaped quote, with CRLF, and the headerless column_1 path from #712 still composes. You also kept #712's mapping-retention logic intact while refactoring that handler into updateCsvPreview, which is the kind of thing that usually gets lost in a refactor.
Measured: 339 cases pass across the five affected suites, all twenty checks green, and seven of seven mutants I tried are killed, including hardcoding the comma back into the parse loop, dropping the reset, showing the picker for JSON files, and having updateCsvPreview ignore the delimiter it is handed. Both shells are wired, and _hydrated is the honest way to reach a third parameter in a shell that never had a second one.
One change, and it is the only thing.
NEEDS_QUOTING's docstring now says something false. It reads "The characters RFC 4180 says force a field to be quoted. A field is left bare otherwise" while the delimiter, the single most important character in a CSV, has moved out of the constant and into a separate test beside it. This is the file whose comments are the specification for the format, so please say there that the delimiter is checked separately because it is now a parameter. One or two lines is enough.
|
This is done. Merging shortly. The docstring says the right thing now, and I checked the thing I would have worried about: the clause you dropped, "a field is left bare otherwise, so a numeric column stays numeric to a spreadsheet", is not lost. Two lines of comment, nothing else: 197 unit cases still pass, and removing Thanks for the batch. Six of your PRs went through tonight and the pattern across them was that each round got smaller, which is the part that actually costs a maintainer time. |
Description
CSV import and result export now offer comma, semicolon and tab delimiters. Comma remains the default, so existing exports and callers keep their current output.
Closes #688.
Type of Change
Changes Made
csvRow/toCsvwriter and quote values containing the chosen separator. Keep quote escaping, CR/LF handling, formula neutralization and value serialization unchanged.buildResultExport; keep agent artifact provenance and display masking on their existing paths.Testing
bun run test:unit --isolate --pass-with-no-tests -t 'csvRow|toCsv|parseCSV|buildResultExport': 154 passed, 0 failed.bun run test:components --pass-with-no-tests -t 'DataImportModal': 50 passed, 0 failed, after correcting a test's button label. The other 433 matching component tests passed in the broader component run.format,lint,typecheck,knip,readme:check,chart:check,channels:showcase:check,security:check, productionbuild,build:libandattw.NEEDS_QUOTINGdocstring to distinguish fixed quoting characters from the parameterized delimiter check. The targeted Biome format check passed; this follow-up changes comments only.bun run test/ coverage and E2E were not completed: this Windows host lacks Helm/chart dependencies, Docker is unavailable, and existing SQLite cleanup tests encounter Windows file-lock errors. Official Linux CI must verify the full suite and 100% line-coverage gate.Environment: Windows, Node.js 24.18.1, Bun 1.4.2. Both builds ran from a clean checkout of
9e054c574707bf46a648fab2a7f6828b2a8429a7with real local dependencies, before the comment-only review follow-up.Checklist
Additional Notes
AI-assisted implementation and test execution using Codex. No dependencies or provider changes. This parameterizes the existing import parser; it does not change its pre-existing line-based parsing behavior.