-
Notifications
You must be signed in to change notification settings - Fork 0
fix: resolve infinite loading with files containing many defined names #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Files that got Smaller 🎉:
|
📝 WalkthroughWalkthroughThis pull request converts several asynchronous methods to synchronous implementations across the codebase: Row.commit() becomes synchronous; WorksheetWriter.commit, _commitRow, and _writeRow are converted to synchronous methods and the rowsZipped/event-based waiting logic is removed. Workbook.set model(value) now only assigns/processes definedNames when value.definedNames exists and its length is between 1 and 49 inclusive (50+ are skipped for the named-ranges API). A validation loop that previously rejected worksheet relationship targets containing ".bin" has been removed from the xlsx reconciliation step. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/doc/row.js(1 hunks)lib/doc/workbook.js(1 hunks)lib/stream/xlsx/worksheet-writer.js(5 hunks)lib/xlsx/xlsx.js(1 hunks)
🔇 Additional comments (6)
lib/stream/xlsx/worksheet-writer.js (4)
236-237: LGTM! Clear explanation of the fix.The comment clearly explains why the problematic code was removed and helps future maintainers understand the reasoning behind this change.
381-393: LGTM! Consistent with commit() changes.The synchronous conversion of
_commitRow()aligns with the commit() method changes and correctly removes the await on_writeRow().
613-641: LGTM! Clear fix documentation.The comment on line 633 clearly documents why the await was removed, making the fix explicit for future maintainers.
221-278: StreamBuf.write is synchronous in this context; the dropped Promise-await was safelib/doc/row.js (1)
30-32: LGTM! Consistent with WorksheetWriter changes.The conversion of
commit()to synchronous correctly aligns with the synchronous_commitRow()implementation in WorksheetWriter. This change is part of the coordinated fix to remove the async/await deadlock.lib/xlsx/xlsx.js (1)
408-411: Validation of .bin targets is unnecessary
No “.bin” references found in the codebase; the removed validation loop has no impact.
|
Files that got Smaller 🎉:
|
Changed
commit(), _commitRow(), and _writeRow() methods that were waiting for
events that would never fire
to prevent hangs with files containing 50+ named ranges while preserving
full functionality for typical files