Skip to content

feat: add CSV to PDF operation - #179

Closed
slegarraga wants to merge 2 commits into
mithun-srinivas:mainfrom
slegarraga:feat/excel-to-pdf-172
Closed

slegarraga wants to merge 2 commits into
mithun-srinivas:mainfrom
slegarraga:feat/excel-to-pdf-172

Conversation

@slegarraga

@slegarraga slegarraga commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes #172

Rescoped from Excel/CSV to CSV only after review, removing the vulnerable xlsx dependency while keeping the existing paginated table layout.

What

  • Adds src/operations/csv-to-pdf/ as a self-contained client-side operation.
  • Parses CSV with a small inline parser supporting quoted fields, escaped quotes, CRLF, and embedded commas/newlines.
  • Renders one paginated PDF table with jsPDF: repeated headers on page breaks, content-aware capped column widths, and ellipsis truncation.
  • Uses a plain reduce loop for width aggregation to avoid spreading very large arrays.
  • Reports honest limits: values only; formatting, merged cells and styles do not carry over.

Verification

  • npm ci
  • npm run lint
  • npm test — includes parser coverage for quoted fields/escaped quotes, CRLF, embedded delimiters/newlines, blank-row filtering, and empty-input rejection.
  • npm run build
  • npm run format:check
  • Real fixture conversion through the helper produced a valid %PDF-1.7 blob with the expected row count.
  • Confirmed xlsx and SheetJS are absent from package.json, package-lock.json, source, and build output.

New src/operations/excel-to-pdf/ plugin: parses .xlsx/.csv fully client-side
with SheetJS and lays each sheet out as a paginated landscape table via jsPDF.

- Multi-sheet workbooks: one titled section per sheet, empty sheets skipped
- Column widths derived from content, long cells truncated with ellipsis
- Header row bold with rule, repeated after page breaks
- Values only: formatting, merged cells and styles don't carry over (stated
  in the tool's result note per the issue's honesty requirement)
- Empty workbook or all-empty sheets fail with a clear error

Verified: real .xlsx (2 sheets) and .csv fixtures converted through the
actual helper code to valid %PDF-1.3 output; eslint clean; vite build green
with the operation embedded in the bundle.
@vercel

vercel Bot commented Aug 22, 2026

Copy link
Copy Markdown

@slegarraga is attempting to deploy a commit to the Mithun Srinivas' projects Team on Vercel.

A member of the Team first needs to authorize it.

The CI run for mithun-srinivas#179 failed with 'Timed out waiting 15000ms from
config.webServer': the build step inside the webServer command can exceed
15s on cold caches (our PR adds the xlsx dependency, growing the bundle).
60s gives headroom; local runs are unaffected because
reuseExistingServer skips this path outside CI.

Verified locally: npx playwright test → 5 passed (33.2s) with the new
timeout in place.
@mithun-srinivas

Copy link
Copy Markdown
Owner

Thanks for this @slegarraga, the layout code is genuinely nice: the paginated table, the repeated header row on page breaks, the ellipsis truncation, and the honest "values only" note are all exactly right.

The one thing I want to change before merging is the dependency. Pulling in xlsx (SheetJS) is the sticking point: the newest version published to npm is 0.18.5, and it carries two known advisories with no fixed release available on npm, prototype pollution (CVE-2023-30533) and a ReDoS (CVE-2024-22363). The patched builds only live on SheetJS's own CDN, which we can't use without breaking the zero-network guarantee. For a privacy and security focused project, I would rather not ship a package we can't patch.

So let's rescope this to CSV only and drop SheetJS entirely. CSV covers the common case, needs no dependency, and keeps the whole thing tiny. The jsPDF table layout you already wrote can stay exactly as is; only the parsing step changes.

A small inline CSV parser (handles quoted fields and escaped quotes) is all you need in place of XLSX.read:

function parseCsv(text) {
  const rows = []
  let row = [], field = '', inQuotes = false
  for (let i = 0; i < text.length; i++) {
    const c = text[i]
    if (inQuotes) {
      if (c === '"' && text[i + 1] === '"') { field += '"'; i++ }
      else if (c === '"') inQuotes = false
      else field += c
    } else if (c === '"') inQuotes = true
    else if (c === ',') { row.push(field); field = '' }
    else if (c === '\n') { row.push(field); rows.push(row); row = []; field = '' }
    else if (c !== '\r') field += c
  }
  if (field.length || row.length) { row.push(field); rows.push(row) }
  return rows.filter(r => r.some(cell => cell.trim() !== ''))
}

Then the helper becomes roughly: const text = await file.text(); const rows = parseCsv(text) and feed rows into your existing single-table layout (no more per-sheet loop). A few follow-on tidy-ups:

  • Rename the operation folder to csv-to-pdf and update meta.js (id, name, description) to say CSV, plus the Dropzone accept=".csv".
  • Drop xlsx from package.json and the lockfile.
  • Small edge case worth fixing while you're in there: columnWidths uses Math.max(...rows.map(...)), which can blow the call stack on a very large CSV. A plain reduce loop avoids that.

Everything else you did carries straight over. Thanks again, this is close!

@mithun-srinivas mithun-srinivas mentioned this pull request Aug 24, 2026
3 tasks
@slegarraga slegarraga changed the title feat: add Excel/CSV to PDF operation feat: add CSV to PDF operation Aug 25, 2026
@slegarraga

Copy link
Copy Markdown
Contributor Author

Thank you for such a thoughtful review! I rescoped this to CSV-only and removed SheetJS entirely. The parser now handles quoted fields, escaped quotes, CRLF, embedded delimiters/newlines, and blank-row filtering; column widths use reduce loops instead of spread aggregation. I added focused Vitest coverage, verified lint/format/build, converted a real fixture through the helper, and confirmed xlsx is absent from the manifest, lockfile, source, and build output. Ready for another look!

@mithun-srinivas

Copy link
Copy Markdown
Owner

Closing this one as superseded. We decided to take #172 in a CSV-only direction to avoid the SheetJS dependency and its unpatched advisories, and there's now a CSV-only implementation in #188 that takes that approach. Rather than have two PRs on the same issue, I'm closing this in favor of that one.

Thanks a lot for the original work here @slegarraga, the jsPDF table layout you wrote was solid and it's what pointed the way. You've got other PRs of yours still in flight (#177, #181), so nothing lost. Appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CSV to PDF

2 participants