chore: add demo script and update vite dependency in package.json#8
Conversation
|
Warning Rate limit exceeded@abdullahmujahidali has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds extensive README expansion with Excel/CSV import-export and styling docs, an ADR for XLSX design, a Vite-backed interactive demo (export/import/preview/re-export UI), demo tooling (vite config, script), and a minor test assertion tweak; no public API signatures were changed in library source files. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser_UI as Demo UI (index.html)
participant Cellify as Cellify Library (window exports)
participant FileSys as File/Blob API
User->>Browser_UI: Click "Export" / Drop file / Click "Import"
Browser_UI->>Cellify: call workbookToXlsxBlob / csvToWorkbook / xlsxBlobToWorkbook
Cellify-->>Browser_UI: returns Blob or Workbook object
alt Export path
Browser_UI->>FileSys: createObjectURL / download Blob
FileSys-->>User: file download starts
else Import path
Browser_UI->>Browser_UI: render preview (sheets, formulas, merges)
Browser_UI-->>User: show sheet tabs, preview, logs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
=======================================
Coverage 89.79% 89.79%
=======================================
Files 20 20
Lines 2048 2048
Branches 541 541
=======================================
Hits 1839 1839
Misses 209 209 ☔ 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: 3
🧹 Nitpick comments (1)
docs/decisions/006-xlsx-format.md (1)
54-67: Consider adding language identifier to fenced code block.For better syntax highlighting and consistency, add a language identifier to the fenced code block.
-``` +```text src/formats/xlsx/ ├── index.ts # Public exports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
README.md(4 hunks)demo/README.md(1 hunks)demo/index.html(1 hunks)demo/vite.config.js(1 hunks)docs/decisions/006-xlsx-format.md(1 hunks)docs/decisions/README.md(1 hunks)package.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
README.md (2)
src/core/Workbook.ts (1)
Workbook(58-420)tests/accessibility.test.ts (1)
describe(18-332)
demo/index.html (5)
tests/cell.test.ts (1)
describe(4-259)src/core/Workbook.ts (1)
Workbook(58-420)src/core/Cell.ts (1)
Cell(27-378)tests/workbook.test.ts (5)
describe(4-240)workbook(217-224)workbook(47-53)workbook(145-151)it(13-103)tests/accessibility.test.ts (1)
describe(18-332)
🪛 LanguageTool
docs/decisions/006-xlsx-format.md
[style] ~135-~135: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...iderations - Add streaming support for very large files - Support hyperlinks and comments...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
demo/README.md
19-19: Bare URL used
(MD034, no-bare-urls)
docs/decisions/006-xlsx-format.md
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
docs/decisions/README.md (1)
18-18: LGTM! ADR index updated correctly.The new ADR entry is properly formatted and consistent with the existing index structure.
package.json (1)
35-36: LGTM! Demo script added correctly.The new demo script is properly formatted and integrates well with the existing scripts.
demo/README.md (1)
1-42: LGTM! Clear and comprehensive demo documentation.The documentation effectively describes the demo features, setup instructions, and development workflow.
docs/decisions/006-xlsx-format.md (1)
1-138: LGTM! Comprehensive and well-structured ADR.The ADR effectively documents the XLSX implementation decisions, including rationale, trade-offs, and technical details.
README.md (1)
1-457: LGTM! Excellent comprehensive documentation.The README has been significantly enhanced with clear examples, API references, and usage patterns for all major features. The documentation is well-organized and accessible.
demo/index.html (1)
1-731: LGTM! Well-structured and functional demo.The demo provides a comprehensive interactive showcase of Cellify's capabilities:
- Clean UI with clear sections for export and import workflows
- Multiple export examples demonstrating styling, formulas, and multi-sheet support
- Robust import handling with preview, sheet navigation, and re-export options
- Good error handling and user feedback via logging
The code is well-organized and follows good practices for a demo/testing tool.
| server: { | ||
| port: 5432, | ||
| open: true, |
There was a problem hiding this comment.
Port mismatch with documentation.
The Vite config specifies port 5432, but demo/README.md (line 19) references http://localhost:5173. Ensure consistency between the configuration and documentation.
Consider updating demo/README.md to reflect the correct port:
-3. Open http://localhost:5173 in your browser (Vite will show the actual URL)
+3. Open http://localhost:5432 in your browserOr update the vite.config.js to use the default port (5173):
server: {
- port: 5432,
+ port: 5173,
open: true,
},📝 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.
| server: { | |
| port: 5432, | |
| open: true, | |
| server: { | |
| port: 5173, | |
| open: true, | |
| }, |
🤖 Prompt for AI Agents
In demo/vite.config.js around lines 11 to 13, the dev server port is set to 5432
which conflicts with demo/README.md that references http://localhost:5173;
change the port to 5173 in vite.config.js to match the README, or if you intend
to keep 5432 instead, update demo/README.md (line ~19) to reference
http://localhost:5432; ensure both files use the same port and run a quick dev
start to confirm the server opens at the documented address.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/decisions/006-xlsx-format.md (1)
135-135: Minor language refinement: "very large files" → "large files".Line 135 uses "very" as an over-used intensifier. Consider replacing with a more concrete or precise descriptor (e.g., "large files" or "files exceeding memory constraints").
Apply this diff for clarity:
- - Add streaming support for very large files + - Add streaming support for large files
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
demo/vite.config.js(1 hunks)docs/decisions/006-xlsx-format.md(1 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- demo/vite.config.js
🧰 Additional context used
🪛 LanguageTool
docs/decisions/006-xlsx-format.md
[style] ~135-~135: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...iderations - Add streaming support for very large files - Support hyperlinks and comments...
(EN_WEAK_ADJECTIVE)
🔇 Additional comments (2)
package.json (1)
35-36: Demo script addition looks good.The
"demo": "vite demo"script entry is correctly added and supports the demo tooling mentioned in the PR. The trailing comma on line 35 is appropriate for multi-line JSON formatting.docs/decisions/006-xlsx-format.md (1)
1-138: Well-structured and comprehensive ADR.The XLSX import/export ADR is thorough and clearly documents the design decisions, trade-offs, and rationale. The breakdown of module structure, import strategy, and cell type detection is particularly helpful for future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
demo/index.html (2)
928-931: Make file-type detection case-insensitive for extensionsThe current checks:
if (file.name.endsWith('.xlsx')) { ... } else if (file.name.endsWith('.csv')) { ... }will reject files like
REPORT.XLSXordata.CSV. Consider normalizing to lower case:- if (file.name.endsWith('.xlsx')) { + const lowerName = file.name.toLowerCase(); + if (lowerName.endsWith('.xlsx')) { - } else if (file.name.endsWith('.csv')) { + } else if (lowerName.endsWith('.csv')) {This makes the demo more forgiving and closer to user expectations.
1032-1036: Remove or gate debugconsole.logcalls indisplaySheet
displaySheetcurrently logs dimensions, cell count, and merges on every render:console.log('Sheet dimensions:', dims); console.log('Sheet cells size:', sheet.cells?.size || 'N/A'); console.log('Sheet merges:', sheet.merges?.length || 0);These are useful during development but can clutter the console in the shipped demo. Consider removing them or guarding behind a simple debug flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/images/demo-screenshot.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
README.md(4 hunks)demo/index.html(1 hunks)package.json(2 hunks)tests/xlsx.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (2)
tests/xlsx.test.ts (1)
1322-1322: Relaxed duration assertion is appropriateAllowing
durationMsto be>= 0keeps the invariant against negative values while avoiding flakiness in environments that may report0ms for very fast imports. The rest of the stats checks remain strict and meaningful.README.md (1)
7-33: Feature and demo documentation updates look solidThe high-level description, feature list, and new demo section (
npm run demowith screenshot) clearly communicate the library’s capabilities and how to try them. The messaging matches the XLSX/CSV support and styling/import features exercised in tests and the demo.
| if (file.name.endsWith('.xlsx')) { | ||
| result = await xlsxBlobToWorkbook(file); | ||
| } else if (file.name.endsWith('.csv')) { | ||
| const text = await file.text(); | ||
| result = { workbook: csvToWorkbook(text), stats: { sheetCount: 1 }, warnings: [] }; | ||
| } else { |
There was a problem hiding this comment.
Fix CSV import path to match csvToWorkbook return shape
Here the CSV branch treats csvToWorkbook(text) as if it returns a Workbook directly:
result = { workbook: csvToWorkbook(text), stats: { sheetCount: 1 }, warnings: [] };In the README, csvToWorkbook is documented and used as returning an object:
const { workbook } = csvToWorkbook(csvString);If the actual API is { workbook, ... }, the demo will set result.workbook to that wrapper object instead of the real workbook, breaking currentWorkbook, tab rendering, and preview.
Align the demo with the documented API, for example:
- } else if (file.name.endsWith('.csv')) {
- const text = await file.text();
- result = { workbook: csvToWorkbook(text), stats: { sheetCount: 1 }, warnings: [] };
+ } else if (file.name.endsWith('.csv')) {
+ const text = await file.text();
+ const { workbook } = csvToWorkbook(text);
+ result = { workbook, stats: { sheetCount: 1 }, warnings: [] };
}Adjust further if csvToWorkbook also returns stats/warnings, but the key point is to store the actual Workbook instance in result.workbook.
🤖 Prompt for AI Agents
In demo/index.html around lines 928 to 933, the CSV branch assigns
result.workbook to the entire return value of csvToWorkbook instead of the
actual Workbook; call csvToWorkbook(text), destructure its returned { workbook,
stats, warnings } (or at least extract workbook) and set result = { workbook,
stats: stats || { sheetCount: 1 }, warnings: warnings || [] } so result.workbook
is the real Workbook instance and stats/warnings are preserved or defaulted.
| // Warnings | ||
| const warningsEl = document.getElementById('warningsContent'); | ||
| if (warnings && warnings.length > 0) { | ||
| warningsEl.style.display = 'block'; | ||
| warningsEl.innerHTML = ` | ||
| <p class="warnings-title">⚠️ Warnings</p> | ||
| <ul>${warnings.map(w => `<li>${w.message}</li>`).join('')}</ul> | ||
| `; | ||
| } else { | ||
| warningsEl.style.display = 'none'; | ||
| } |
There was a problem hiding this comment.
Escape all user-controlled strings before inserting into HTML to avoid XSS
warnings text, sheet names, and formula text are currently interpolated into HTML/attributes via innerHTML/template literals without escaping:
warnings.map(w => \- ${w.message} `)` (warnings may include workbook-derived details).
sheetTabsbuttons rendersheet.namedirectly.- Cell
<td>usestitle="${title}"wheretitleincludesformula.formula.
If a user imports a malicious workbook, these values can contain HTML/JS and lead to XSS in the demo. You already have escapeHtml; reuse it for all dynamic content that flows into HTML.
Suggested patch:
- warningsEl.innerHTML = `
- <p class="warnings-title">⚠️ Warnings</p>
- <ul>${warnings.map(w => `<li>${w.message}</li>`).join('')}</ul>
- `;
+ warningsEl.innerHTML = `
+ <p class="warnings-title">⚠️ Warnings</p>
+ <ul>${warnings.map(w => `<li>${escapeHtml(String(w.message))}</li>`).join('')}</ul>
+ `;
@@
- tabsEl.innerHTML = workbook.sheets.map((sheet, i) =>
- `<button class="sheet-tab ${i === 0 ? 'active' : ''}" data-sheet-index="${i}">${sheet.name}</button>`
- ).join('');
+ tabsEl.innerHTML = workbook.sheets.map((sheet, i) =>
+ `<button class="sheet-tab ${i === 0 ? 'active' : ''}" data-sheet-index="${i}">${escapeHtml(String(sheet.name))}</button>`
+ ).join('');
@@
- const formula = cell?.formula;
- const title = formula ? `Formula: =${formula.formula}` : '';
+ const formula = cell?.formula;
+ const rawTitle = formula ? `Formula: =${formula.formula}` : '';
@@
- html += `<td${spanAttrs} title="${title}" style="${style}">${escapeHtml(String(value))}</td>`;
+ const safeTitle = escapeHtml(rawTitle);
+ html += `<td${spanAttrs} title="${safeTitle}" style="${style}">${escapeHtml(String(value))}</td>`;This keeps the demo robust even when opening untrusted XLSX/CSV files.
Also applies to: 1003-1007, 1096-1111, 1137-1142
🤖 Prompt for AI Agents
In demo/index.html around lines 990-1000 (also apply to 1003-1007, 1096-1111,
1137-1142): dynamic strings (warnings, sheet names, cell titles/formulas) are
inserted via innerHTML/template literals and must be escaped to prevent XSS;
update the code to reuse the existing escapeHtml function for every
user/workbook-derived value before interpolation, replace direct innerHTML
builds with element creation or set textContent where possible, and for
attributes (like title) set the escaped string via setAttribute or by assigning
the escaped value to the property so no unescaped HTML/JS is injected.
| ## CSV Import/Export | ||
|
|
||
| ```typescript | ||
| import { Workbook, sheetToCsv, csvToWorkbook } from 'cellify'; | ||
| import { sheetToCsv, csvToWorkbook } from 'cellify'; | ||
|
|
||
| // Export to CSV | ||
| const workbook = new Workbook(); | ||
| const sheet = workbook.addSheet('Data'); | ||
| sheet.cell('A1').value = 'Name'; | ||
| sheet.cell('B1').value = 'Age'; | ||
| sheet.cell('A2').value = 'Alice'; | ||
| sheet.cell('B2').value = 30; | ||
|
|
||
| const csv = sheetToCsv(sheet); | ||
| // "Name,Age\r\nAlice,30" | ||
| // "Product,Units,Price,Total\r\nLaptop,150,999.99,149998.5" | ||
|
|
||
| // Import from CSV (auto-detects delimiter) | ||
| const { workbook } = csvToWorkbook(csvString); | ||
|
|
||
| // With options | ||
| const csv = sheetToCsv(sheet, { | ||
| delimiter: ';', | ||
| includeHeaders: true, | ||
| dateFormat: 'YYYY-MM-DD', | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.js" -o -name "*.tsx" -o -name "*.jsx" | head -20Repository: abdullahmujahidali/Cellify
Length of output: 650
🏁 Script executed:
fd -t f "demo" | head -20Repository: abdullahmujahidali/Cellify
Length of output: 101
🏁 Script executed:
cat -n README.md | sed -n '163,181p'Repository: abdullahmujahidali/Cellify
Length of output: 622
🏁 Script executed:
find . -type f \( -name "*csv*" -o -name "*.ts" -o -name "*.js" \) | grep -E "(src|lib)" | head -20Repository: abdullahmujahidali/Cellify
Length of output: 650
🏁 Script executed:
cat -n src/formats/csv.reader.tsRepository: abdullahmujahidali/Cellify
Length of output: 12714
🏁 Script executed:
find . -name "index.html" -o -name "demo.html"Repository: abdullahmujahidali/Cellify
Length of output: 87
🏁 Script executed:
cat -n src/formats/csv.types.tsRepository: abdullahmujahidali/Cellify
Length of output: 5086
🏁 Script executed:
cat -n demo/index.htmlRepository: abdullahmujahidali/Cellify
Length of output: 40026
Update README to reflect actual return type of csvToWorkbook
The documentation shows:
const { workbook } = csvToWorkbook(csvString);However, csvToWorkbook() returns a Workbook directly, not an object with a workbook property. Update the README example to:
const workbook = csvToWorkbook(csvString);The demo file works around this by manually wrapping the result, but the documentation should match the actual implementation.
🤖 Prompt for AI Agents
In README.md around lines 163 to 181, the CSV import example incorrectly
destructures csvToWorkbook as returning an object ({ workbook } =
csvToWorkbook(csvString)) while the actual function returns a Workbook directly;
update the example to assign the return value to a variable (workbook =
csvToWorkbook(csvString)) and adjust any surrounding explanatory text or
examples to use that workbook variable instead of expecting a wrapped object.
1 participant
Description
Brief description of changes.
Type of Change
Related Issues
Fixes #(issue number)
Checklist
npm test)Testing
Describe how you tested these changes.
Screenshots (if applicable)
Add screenshots to help explain your changes.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.