feat: add threaded comments support (Excel 2019+)#28
Conversation
Code ReviewI found 2 issues during the review: Issue 1: Korean comments in code blocks (CLAUDE.md violation)File: The code blocks contain Korean comments, which violates the project guidelines. According to CLAUDE.md:
While the Korean prose in this documentation file is fine, the comments within the code examples must be in English. Please translate all inline comments in these code blocks to English. For example:
Issue 2: Missing person list relationship in workbook_rels (bug)File: The person list is written to the file but the workbook relationship is missing, which will cause Excel to fail to discover the person list. The save code writes The read path expects this relationship (see lines 361-391) and only uses a hardcoded fallback path. The round-trip tests pass because of this fallback, but actual Excel will not use it. Suggested fix: Add the person relationship to // Add person relationship to workbook_rels
if \!self.workbook_rels.relationships.iter().any(|r| {
r.rel_type == sheetkit_xml::threaded_comment::REL_TYPE_PERSON
}) {
let rid = crate::sheet::next_rid(&self.workbook_rels.relationships);
self.workbook_rels.relationships.push(Relationship {
id: rid,
rel_type: sheetkit_xml::threaded_comment::REL_TYPE_PERSON.to_string(),
target: "persons/person.xml".to_string(),
target_mode: None,
});
} |
Nebu1eto
left a comment
There was a problem hiding this comment.
Review finding (blocking): threaded comment/person IDs are generated from a process-local counter. After process restart, IDs restart from the same sequence, which can collide with IDs already present in an existing workbook and produce duplicate IDs.
Relevant path: crates/sheetkit-core/src/threaded_comment.rs (generate_guid).
Please switch to collision-resistant UUID generation and ensure uniqueness against existing IDs in the target workbook.
Nebu1eto
left a comment
There was a problem hiding this comment.
Detailed review notes (blocking):
Threaded comment API is comprehensive, but ID generation strategy is unsafe.
Blocking concern:
- GUID-like IDs are generated from a process-local monotonic counter.
- After process restart, IDs start again from the same sequence and can collide with IDs already present in a workbook.
Relevant area:
crates/sheetkit-core/src/threaded_comment.rs(generate_guid).
Expected fix:
- Use collision-resistant UUID generation.
- Before insert, enforce uniqueness against existing comment/person IDs in the workbook.
Also consider:
- Returning a not-found error for delete/resolve operations when ID does not exist (currently silent success path through workbook API).
- Validating
cellreference format on add to fail early.
Implement threaded comments with conversation-style threads, replies, author tracking via shared person list, and resolved/done state. XML layer: ThreadedComments, Person serde types with OOXML namespaces. Core layer: add/get/delete/resolve threaded comments, person management. I/O layer: read/write threadedComment and person.xml parts with content type overrides and worksheet/workbook relationships. Node.js bindings: 7 napi methods + TypeScript wrapper methods. Tests: 24 Rust tests + 10 Node.js tests including file round-trip. Docs: EN and KO guide sections with Rust and TypeScript examples. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d artifacts Allow Korean comments in code blocks within Korean documentation while requiring English for variable names, string literals, and data values. Add biome and gitignore exclusions for tsdown-generated *2.js CJS chunks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace process-local counter-based ID generation with uuid crate v4
random UUIDs in {XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX} format matching
Excel conventions. New IDs are checked against existing person and
comment IDs before insertion to guarantee uniqueness. Also adds
ThreadedCommentNotFound error variant, cell reference validation on add,
and comprehensive tests for ID format, uniqueness, and error cases.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b30a377 to
35e91a1
Compare
Summary
xl/threadedComments/threadedComment{N}.xmlandxl/persons/person.xmlwith proper content types and relationshipsaddThreadedComment,getThreadedComments,getThreadedCommentsByCell,deleteThreadedComment,resolveThreadedComment,addPerson,getPersonsChanges
XML Layer (
sheetkit-xml)threaded_comment.rs:ThreadedComments,ThreadedComment,PersonList,Personserde types with OOXML namespace constantsCore Layer (
sheetkit-core)threaded_comment.rs: business logic for add/get/delete/resolve comments, person management with auto-creation, GUID generationworkbook/io.rs: read threaded comments from worksheet rels and person list from workbook rels; write both parts with content type overridesworkbook/mod.rs: addedsheet_threaded_commentsandperson_listfields to Workbookworkbook/sheet_ops.rs: maintain threaded comments vector in sync across new/delete/copy/stream operationsworkbook/features.rs: 7 public API methods + 10 integration testsNode.js Layer (
packages/sheetkit)types.rs, 7 napi methods inlib.rsindex.tsDocumentation
docs/{en,ko}/guide/advanced.mdTest plan
cargo build/test/clippy/fmt,pnpm build/test, biome checkGenerated with Claude Code