fix(csv): quote values with carriage returns and use correct MIME type#29783
fix(csv): quote values with carriage returns and use correct MIME type#29783PedroHenrique0713 wants to merge 1 commit into
Conversation
…ME type Two bugs in packages/lib/csvUtils.ts: 1. sanitizeValue() did not quote values containing a bare carriage return (\r). Per RFC 4180, CR is a valid record separator in CSV — an unquoted \r splits the field across two records, silently corrupting the export. Values with \n or \r\n were already quoted; only lone \r was missed. Used by BookingsCsvDownload and UserListTable CSV exports. 2. downloadAsCsv() created the Blob with type 'text/plain' instead of 'text/csv'. The wrong MIME type can cause some browsers to open the content inline rather than triggering a .csv download. The codebase's own FileUploader already recognises 'text/csv' as the correct type.
|
Welcome to Cal.diy, @PedroHenrique0713! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
📝 WalkthroughWalkthroughUpdated CSV escaping to quote values containing carriage returns, alongside commas and line breaks. Added tests for 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/lib/csvUtils.test.ts (1)
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrevent test pollution by restoring stubbed globals.
vi.stubGlobalis used in this test file to overwrite the globalURLanddocumentobjects. If Vitest is not configured to automatically unstub globals in your setup, these overridden objects will leak into other tests running in the same environment, potentially breaking tests that expect a full standarddocumentorURLobject. Addvi.unstubAllGlobals()to the cleanup hook to ensure isolation.♻️ Proposed fix
afterEach(() => { vi.restoreAllMocks(); + vi.unstubAllGlobals(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/csvUtils.test.ts` around lines 4 - 6, Update the test file’s afterEach cleanup hook to call vi.unstubAllGlobals() alongside vi.restoreAllMocks(), ensuring globals stubbed by tests are restored after each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/lib/csvUtils.test.ts`:
- Around line 4-6: Update the test file’s afterEach cleanup hook to call
vi.unstubAllGlobals() alongside vi.restoreAllMocks(), ensuring globals stubbed
by tests are restored after each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 903efb13-7d58-4ad6-938e-9218874ba90d
📒 Files selected for processing (2)
packages/lib/csvUtils.test.tspackages/lib/csvUtils.ts
What does this PR do?
Fixes two bugs in
packages/lib/csvUtils.ts, which powers the Bookings CSV export (BookingsCsvDownload) and the Users CSV export (UserListTable):sanitizeValue()did not quote values containing a bare carriage return (\r). Per RFC 4180, CR is a valid record separator in CSV. An unquoted\rsplits the field across two records, silently corrupting the export. Values with\nor\r\nwere already quoted; only lone\rwas missed.downloadAsCsv()created the Blob withtype: "text/plain"instead oftext/csv. The wrong MIME type can cause some browsers to open the content inline rather than triggering a.csvdownload. The codebase's ownFileUploaderalready recognisestext/csvas the correct type.Visual Demo (For contributors especially)
N/A — no visual change. The fix prevents CSV corruption for values containing
\r(e.g. user notes, attendee names with legacy line endings) and ensures the download is recognised as CSV by all browsers.Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
TZ=UTC npx vitest run packages/lib/csvUtils.test.ts— 10 tests, all pass.\rquoting,\nquoting,\r\nquoting, comma quoting, double-quote doubling, and the Blob MIME type assertion.\rtest fails onmain(expected hello\rworld to be hellorworld) and passes with this fix.Checklist