feat: Implement CSV import/export functionality#5
Conversation
- Added CSV import/export capabilities with auto-detection of delimiters. - Introduced new API methods: `csvToWorkbook`, `csvToSheet`, `sheetToCsv`, and `sheetsToCsv`. - Created comprehensive tests for CSV import and export functionalities. - Documented CSV format options and implementation decisions in ADR-005. - Updated README to reflect new features and usage examples.
WalkthroughAdds RFC 4180–aware CSV import and export: new reader and writer modules with delimiter auto-detection, type inference, configurable options and defaults, new types and exports, documentation (ADR + README) and an extensive test suite. Changes
// Sequence Diagram(s) omitted (changes do not meet diagram criteria). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/csv.test.ts (2)
🔇 Additional comments (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
==========================================
+ Coverage 84.71% 88.45% +3.73%
==========================================
Files 7 10 +3
Lines 772 1022 +250
Branches 180 260 +80
==========================================
+ Hits 654 904 +250
Misses 118 118 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/csv.test.ts (1)
163-309: Consider adding date detection tests.The test suite comprehensively covers most import functionality, but there's no test for date/datetime detection mentioned in the ADR (lines 43-44 in 005-csv-format.md). Consider adding tests for the
detectDatesoption with ISO date patterns.it('should detect dates when enabled', () => { const csv = '2024-01-15,2024-12-31T23:59:59'; const workbook = csvToWorkbook(csv, { detectDates: true }); const sheet = workbook.getSheetByIndex(0)!; expect(sheet.cell('A1').value).toBeInstanceOf(Date); expect(sheet.cell('B1').value).toBeInstanceOf(Date); }); it('should not detect dates when disabled', () => { const csv = '2024-01-15'; const workbook = csvToWorkbook(csv, { detectDates: false }); const sheet = workbook.getSheetByIndex(0)!; expect(typeof sheet.cell('A1').value).toBe('string'); });src/formats/csv.reader.ts (1)
299-336: Consider handling European number formats more explicitly.The comment on line 320 mentions handling different decimal formats, but the implementation only removes commas as thousands separators. European number formats using periods for thousands (e.g.,
1.000.000) won't be correctly parsed.Since the validation regex on line 331 requires a standard format, these European numbers will correctly remain as strings. However, consider either:
- Explicitly documenting that only Anglo-Saxon number formats (comma thousands, period decimal) are supported
- Or implementing heuristic detection for European formats (period thousands, comma decimal)
src/formats/csv.writer.ts (1)
86-97: Potential ambiguity when multiple sheets share the same name
sheetsToCsvusessheet.nameas theMapkey, so later sheets with the same name silently overwrite earlier ones.If duplicate names are possible in your model, consider either:
- Throwing or warning on duplicates, or
- Accepting a custom key function / suffix strategy.
If sheet names are guaranteed unique, a brief comment to that effect would clarify the intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)docs/decisions/005-csv-format.md(1 hunks)docs/decisions/README.md(1 hunks)src/formats/csv.reader.ts(1 hunks)src/formats/csv.types.ts(1 hunks)src/formats/csv.writer.ts(1 hunks)src/formats/index.ts(1 hunks)src/index.ts(1 hunks)tests/csv.test.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/formats/csv.types.ts (1)
src/formats/index.ts (5)
CsvExportOptions(3-3)CsvImportOptions(4-4)CsvImportResult(5-5)DEFAULT_CSV_EXPORT_OPTIONS(9-9)DEFAULT_CSV_IMPORT_OPTIONS(10-10)
src/formats/csv.reader.ts (2)
src/formats/csv.types.ts (3)
CsvImportOptions(73-149)DEFAULT_CSV_IMPORT_OPTIONS(194-208)CsvImportResult(154-174)src/core/Sheet.ts (1)
rows(549-551)
tests/csv.test.ts (2)
src/formats/csv.writer.ts (3)
sheetToCsv(32-65)sheetToCsvBuffer(74-77)sheetsToCsv(86-97)src/formats/csv.reader.ts (3)
csvToWorkbook(30-37)csvToSheet(47-113)csvBufferToWorkbook(122-127)
src/formats/csv.writer.ts (1)
src/formats/csv.types.ts (2)
CsvExportOptions(8-68)DEFAULT_CSV_EXPORT_OPTIONS(179-189)
🔇 Additional comments (17)
docs/decisions/README.md (1)
17-17: LGTM!The ADR index entry is properly formatted and consistent with existing entries.
docs/decisions/005-csv-format.md (2)
6-71: LGTM!The technical decisions are well-documented with clear rationale. The RFC 4180 compliance, auto-detection strategy, and API design align with the implementation.
73-96: LGTM!Good documentation of both benefits and limitations. The acknowledgment of CSV's lossy nature and missing streaming support sets appropriate expectations.
src/index.ts (1)
46-47: LGTM!The public API export is clean and follows the existing pattern. The comment appropriately indicates future Excel format support.
README.md (2)
78-78: LGTM!The feature list addition accurately reflects the new CSV import/export capability.
86-114: LGTM!The CSV documentation is clear and comprehensive. The code examples demonstrate both import and export workflows correctly, and the feature list accurately describes the implementation capabilities.
tests/csv.test.ts (3)
12-134: LGTM!Excellent test coverage for CSV export functionality. The tests cover empty sheets, various data types, RFC 4180 escaping rules, custom options, and range exports.
136-161: LGTM!The tests for buffer export and multi-sheet export provide appropriate coverage for these wrapper functions.
311-416: LGTM!Excellent test coverage for import functions and roundtrip integrity. The tests verify that data survives the export/import cycle, including special characters that require escaping.
src/formats/csv.types.ts (2)
73-174: LGTM!The import options and result types are comprehensive and well-documented. The option names are clear, and the default values are sensible.
179-189: Verify theincludeHeadersdefault value.The default export options include
includeHeaders: true, but this option doesn't appear to be implemented in the export logic (as flagged in the previous comment about lines 33-37). Verify whether this default should be removed or if the feature needs to be implemented.src/formats/csv.reader.ts (4)
30-127: LGTM!The public API functions are well-structured. The delegation pattern keeps the code DRY, and the buffer handling correctly deals with UTF-8 decoding and BOM removal.
170-258: LGTM!The CSV parser correctly implements RFC 4180 with proper handling of quoted fields, escaped quotes (doubled), and multi-line values. The state machine approach is clear and handles edge cases correctly.
263-294: LGTM!The type conversion logic is well-structured with appropriate fallbacks. The conditional detection based on options provides good user control.
341-403: The ADR does not imply datetime support. Line 43 specifies "ISO date | Date |2024-01-15→ Date object"—a date-only format. The "Negative" section explicitly notes "Limited date format detection," and "Future Enhancements" mentions additional date patterns, not datetime support. The current implementation correctly aligns with the ADR's date-only scope.Likely an incorrect or invalid review comment.
src/formats/index.ts (1)
1-17: CSV barrel exports look clean and consistentRe-exports are well-organized (types vs values) and match the intended CSV API surface; this should integrate cleanly with the existing top-level index.
src/formats/csv.writer.ts (1)
99-204: Value formatting and escaping cover key edge casesThe helpers handle nulls, numbers (including NaN/Infinity), booleans, dates (ISO/locale/custom), and rich text, and
escapeFieldcorrectly quotes fields per RFC 4180 (delimiter/quote/newline) with proper quote doubling and regex escaping. This is a solid foundation for CSV export.
| export function sheetToCsv(sheet: Sheet, options: CsvExportOptions = {}): string { | ||
| const opts = { ...DEFAULT_CSV_EXPORT_OPTIONS, ...options }; | ||
| const { delimiter, rowDelimiter, quoteChar, quoteAllFields, includeBom } = opts; | ||
|
|
||
| // Determine range to export | ||
| const range = options.range | ||
| ? parseRangeReference(options.range) | ||
| : sheet.dimensions; | ||
|
|
||
| if (!range) { | ||
| // Empty sheet | ||
| return includeBom ? UTF8_BOM : ''; | ||
| } | ||
|
|
||
| const rows: string[] = []; | ||
|
|
||
| // Build CSV rows | ||
| for (let rowIdx = range.startRow; rowIdx <= range.endRow; rowIdx++) { | ||
| const rowValues: string[] = []; | ||
|
|
||
| for (let colIdx = range.startCol; colIdx <= range.endCol; colIdx++) { | ||
| const cell = sheet.getCell(rowIdx, colIdx); | ||
| const value = cell?.value ?? null; | ||
| const formatted = formatValue(value, opts); | ||
| const escaped = escapeField(formatted, delimiter, quoteChar, quoteAllFields); | ||
| rowValues.push(escaped); | ||
| } | ||
|
|
||
| rows.push(rowValues.join(delimiter)); | ||
| } | ||
|
|
||
| const csv = rows.join(rowDelimiter); | ||
| return includeBom ? UTF8_BOM + csv : csv; | ||
| } |
There was a problem hiding this comment.
includeHeaders option is currently ignored in export loop
CsvExportOptions defines includeHeaders, but sheetToCsv always exports every row in range, regardless of this flag. Callers setting includeHeaders: false will still get the first row, which is surprising given the option name and JSDoc.
Consider wiring the flag into the row iteration so the first row of the exported range is treated as the “header” row and skipped when includeHeaders is false, e.g.:
-export function sheetToCsv(sheet: Sheet, options: CsvExportOptions = {}): string {
- const opts = { ...DEFAULT_CSV_EXPORT_OPTIONS, ...options };
- const { delimiter, rowDelimiter, quoteChar, quoteAllFields, includeBom } = opts;
+export function sheetToCsv(sheet: Sheet, options: CsvExportOptions = {}): string {
+ const opts = { ...DEFAULT_CSV_EXPORT_OPTIONS, ...options };
+ const { delimiter, rowDelimiter, quoteChar, quoteAllFields, includeBom, includeHeaders } = opts;
@@
- // Build CSV rows
- for (let rowIdx = range.startRow; rowIdx <= range.endRow; rowIdx++) {
+ // Build CSV rows
+ const startRow = includeHeaders ? range.startRow : range.startRow + 1;
+ for (let rowIdx = startRow; rowIdx <= range.endRow; rowIdx++) {(Behavior would be “first row of the exported range is considered the header”.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function sheetToCsv(sheet: Sheet, options: CsvExportOptions = {}): string { | |
| const opts = { ...DEFAULT_CSV_EXPORT_OPTIONS, ...options }; | |
| const { delimiter, rowDelimiter, quoteChar, quoteAllFields, includeBom } = opts; | |
| // Determine range to export | |
| const range = options.range | |
| ? parseRangeReference(options.range) | |
| : sheet.dimensions; | |
| if (!range) { | |
| // Empty sheet | |
| return includeBom ? UTF8_BOM : ''; | |
| } | |
| const rows: string[] = []; | |
| // Build CSV rows | |
| for (let rowIdx = range.startRow; rowIdx <= range.endRow; rowIdx++) { | |
| const rowValues: string[] = []; | |
| for (let colIdx = range.startCol; colIdx <= range.endCol; colIdx++) { | |
| const cell = sheet.getCell(rowIdx, colIdx); | |
| const value = cell?.value ?? null; | |
| const formatted = formatValue(value, opts); | |
| const escaped = escapeField(formatted, delimiter, quoteChar, quoteAllFields); | |
| rowValues.push(escaped); | |
| } | |
| rows.push(rowValues.join(delimiter)); | |
| } | |
| const csv = rows.join(rowDelimiter); | |
| return includeBom ? UTF8_BOM + csv : csv; | |
| } | |
| export function sheetToCsv(sheet: Sheet, options: CsvExportOptions = {}): string { | |
| const opts = { ...DEFAULT_CSV_EXPORT_OPTIONS, ...options }; | |
| const { delimiter, rowDelimiter, quoteChar, quoteAllFields, includeBom, includeHeaders } = opts; | |
| // Determine range to export | |
| const range = options.range | |
| ? parseRangeReference(options.range) | |
| : sheet.dimensions; | |
| if (!range) { | |
| // Empty sheet | |
| return includeBom ? UTF8_BOM : ''; | |
| } | |
| const rows: string[] = []; | |
| // Build CSV rows | |
| const startRow = includeHeaders ? range.startRow : range.startRow + 1; | |
| for (let rowIdx = startRow; rowIdx <= range.endRow; rowIdx++) { | |
| const rowValues: string[] = []; | |
| for (let colIdx = range.startCol; colIdx <= range.endCol; colIdx++) { | |
| const cell = sheet.getCell(rowIdx, colIdx); | |
| const value = cell?.value ?? null; | |
| const formatted = formatValue(value, opts); | |
| const escaped = escapeField(formatted, delimiter, quoteChar, quoteAllFields); | |
| rowValues.push(escaped); | |
| } | |
| rows.push(rowValues.join(delimiter)); | |
| } | |
| const csv = rows.join(rowDelimiter); | |
| return includeBom ? UTF8_BOM + csv : csv; | |
| } |
🤖 Prompt for AI Agents
In src/formats/csv.writer.ts around lines 32 to 65, the
CsvExportOptions.includeHeaders flag is ignored — the function always exports
every row in the provided range. Use the resolved opts.includeHeaders value to
treat the first row of the exported range as a header and skip it when
includeHeaders is false (e.g. start the row loop at range.startRow + 1 or
conditionally continue when rowIdx === range.startRow && !opts.includeHeaders).
Ensure skipping the header still respects includeBom and handles the case where
skipping leaves no data (return BOM or empty string accordingly).
| export function sheetToCsvBuffer(sheet: Sheet, options: CsvExportOptions = {}): Uint8Array { | ||
| const csv = sheetToCsv(sheet, options); | ||
| return new TextEncoder().encode(csv); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the csv.writer.ts file to understand the implementation
find . -type f -name "csv.writer.ts" | head -5Repository: abdullahmujahidali/Cellify
Length of output: 97
🏁 Script executed:
# Let's also search for CsvExportOptions definition
rg "CsvExportOptions" --type ts -A 5 | head -50Repository: abdullahmujahidali/Cellify
Length of output: 2426
🏁 Script executed:
# Let's read the csv.types.ts file to see the full CsvExportOptions definition
cat -n src/formats/csv.types.tsRepository: abdullahmujahidali/Cellify
Length of output: 5263
🏁 Script executed:
# Read csv.writer.ts to see the full sheetToCsv implementation
cat -n src/formats/csv.writer.ts | head -100Repository: abdullahmujahidali/Cellify
Length of output: 3433
🏁 Script executed:
# Search for tests related to CSV encoding
rg "encoding" --type ts -i | grep -i "csv\|test" | head -20Repository: abdullahmujahidali/Cellify
Length of output: 304
🏁 Script executed:
# Check if encoding is used anywhere else in the codebase
rg "encoding" --type ts -B 2 -A 2Repository: abdullahmujahidali/Cellify
Length of output: 710
🏁 Script executed:
# Find test files related to CSV
find . -type f \( -name "*test*" -o -name "*spec*" \) -path "*csv*" | head -10Repository: abdullahmujahidali/Cellify
Length of output: 89
🏁 Script executed:
# Search for test files more broadly
fd -e test.ts -e spec.ts -e test.js -e spec.js | grep -i csvRepository: abdullahmujahidali/Cellify
Length of output: 87
🏁 Script executed:
# Read the CSV test file
cat -n tests/csv.test.tsRepository: abdullahmujahidali/Cellify
Length of output: 15811
encoding option is not honored in sheetToCsvBuffer (always UTF‑8)
CsvExportOptions.encoding allows 'utf-8' | 'utf-16' | 'ascii', but sheetToCsvBuffer always uses TextEncoder, which only produces UTF‑8 and doesn't accept an encoding parameter. This silently ignores non‑UTF‑8 encodings and can mislead callers.
At minimum, consider enforcing UTF‑8 here (and/or tightening the type/docs), e.g.:
export function sheetToCsvBuffer(sheet: Sheet, options: CsvExportOptions = {}): Uint8Array {
- const csv = sheetToCsv(sheet, options);
- return new TextEncoder().encode(csv);
+ const encoding = options.encoding ?? DEFAULT_CSV_EXPORT_OPTIONS.encoding;
+ if (encoding !== 'utf-8') {
+ throw new Error(
+ `sheetToCsvBuffer currently supports only 'utf-8' encoding (got '${encoding}').`
+ );
+ }
+
+ const csv = sheetToCsv(sheet, options);
+ return new TextEncoder().encode(csv);
}Alternatively, implement real non‑UTF‑8 encoding support (Node Buffer, iconv, etc.) or drop those encodings from the options type.
Committable suggestion skipped: line range outside the PR's diff.
| "compilerOptions": { | ||
| "target": "ES2022", | ||
| "lib": ["ES2022"], | ||
| "lib": ["ES2022", "DOM"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check usage of DOM-specific APIs beyond TextEncoder/TextDecoder
rg -n "Blob|File|FormData|XMLHttpRequest|fetch|localStorage|sessionStorage|document|window|navigator" --type=ts src/Repository: abdullahmujahidali/Cellify
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check TextEncoder/TextDecoder usage specifically
rg -n "TextEncoder|TextDecoder" --type=ts src/Repository: abdullahmujahidali/Cellify
Length of output: 223
🏁 Script executed:
#!/bin/bash
# View current tsconfig.json
cat tsconfig.jsonRepository: abdullahmujahidali/Cellify
Length of output: 785
🏁 Script executed:
#!/bin/bash
# Check if there's any package.json to understand TypeScript version
cat package.json | grep -A 5 typescriptRepository: abdullahmujahidali/Cellify
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Check for any DOM type references in the source files
rg -n "DOM|TextEncoder|TextDecoder" src/ tsconfig.jsonRepository: abdullahmujahidali/Cellify
Length of output: 398
Remove DOM lib from lib array in tsconfig.json—it is not necessary.
TextEncoder and TextDecoder, the only DOM-related features used in this codebase (csv.writer.ts and csv.reader.ts), are included in the ES2022 standard library, which is already specified. The DOM lib adds unnecessary browser-specific APIs (Blob, File, FormData, XMLHttpRequest, etc.) with zero actual usage in the codebase. Since TypeScript 5.3.0 includes these APIs in ES2022, you can safely change line 4 to "lib": ["ES2022"].
🤖 Prompt for AI Agents
In tsconfig.json around line 4, the compiler "lib" currently includes "DOM"
which is unnecessary; remove "DOM" so the lib array is only ["ES2022"]. Edit
that line to drop "DOM" (leave "ES2022") to avoid pulling in browser-only types
while keeping TextEncoder/TextDecoder from ES2022.
2 participants
csvToWorkbook,csvToSheet,sheetToCsv, andsheetsToCsv.Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.