feat: add OpenOptions for partial workbook reading#19
Conversation
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Nebu1eto
left a comment
There was a problem hiding this comment.
Review finding (blocking): OpenOptions sheets filtering currently swaps unselected worksheets to WorksheetXml::default() during open, and save then writes those empty worksheets back. This makes open_with_options(...).save(...) data-destructive for skipped sheets.
Relevant path: crates/sheetkit-core/src/workbook/io.rs (sheet parse branch that uses WorksheetXml::default(), and full worksheet write path).
Suggested fix: preserve raw XML bytes for unparsed sheets (or forbid save when selective parsing was used) so skipped sheets round-trip without data loss.
Nebu1eto
left a comment
There was a problem hiding this comment.
Detailed review notes (blocking):
OpenOptions is a valuable addition, but there is a data-loss path today.
Blocking concern:
- With selective sheet parsing (
sheetsoption), unselected worksheets are replaced byWorksheetXml::default()during open. - A subsequent save writes those default/empty worksheets back, which destroys data on skipped sheets.
Relevant area:
crates/sheetkit-core/src/workbook/io.rs(sheet parse branch + normal worksheet serialization path).
Expected behavior for safety:
- Either preserve raw XML bytes for unparsed sheets and round-trip them unchanged, or
- mark workbook as partially loaded and reject save unless explicitly forced.
Please add a regression test for: open_with_options(select subset) -> save -> reopen -> skipped sheet data remains intact.
…safety) Add OpenOptions struct with sheet_rows, sheets, max_unzip_size, and max_zip_entries fields. Workbook::open_with_options and open_from_buffer_with_options accept these options to control parsing. - sheet_rows: truncate rows per sheet during open for faster partial reads - sheets: skip XML parsing for unselected sheets (empty WorksheetXml) - max_unzip_size / max_zip_entries: reject archives exceeding safety limits - New error variants: ZipSizeExceeded, ZipEntryCountExceeded - Node.js: optional options parameter added to open/openSync/openBuffer/openBufferSync - TypeScript wrapper updated with JsOpenOptions type - 17 Rust tests + 13 Node.js tests covering all options and combinations - EN and KO API reference docs updated Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When opening with selective sheet parsing (sheets option), unparsed sheets were replaced with WorksheetXml::default(), destroying their data on save. Now the raw XML bytes are stored and written back directly during save, preserving all sheet content unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
32d9445 to
7606c64
Compare
Summary
OpenOptionsstruct (Rust) /JsOpenOptionstype (TypeScript) for controlling workbook parsing at open timesheetRows). Rows beyond the limit are discarded during parsing for a real performance benefit.ZipSizeExceeded,ZipEntryCountExceededopen()/openSync()/openBuffer()/openBufferSync()methods accept an optional options parameter -- fully backward compatibleWorkbook::open_with_options(path, &opts)andWorkbook::open_from_buffer_with_options(data, &opts)Test plan
cargo build --workspacepassescargo test --workspacepasses (1253 core + 178 xml + 7 integration + 1 facade + 5 doc = 1444 total)cargo clippy --workspacepasses (no warnings)cargo fmt --checkpassespnpm buildpasses (napi + typecheck + tsdown)pnpm vitest runpasses (267 tests including 13 new open options tests)pnpm biome check .passespnpm tsc --noEmitpasses.claude/files committedGenerated with Claude Code