Conversation
…y with tests - Introduced PasteOptions and SearchOptions interfaces in range.types.ts for enhanced paste and search functionalities. - Implemented copyRange, pasteRange, cutRange, and duplicateRange methods in the Workbook class with corresponding tests. - Added comprehensive tests for copy/paste operations, including handling of values, styles, and transposition. - Developed data import/export helpers with tests for fromArray, fromObjects, toArray, and toObjects methods. - Implemented row and column operations with tests for insert, delete, move, and edge cases. - Created search functionality with tests for find, findAll, replace, and replaceAll methods.
WalkthroughAdds sheet-level features: search & replace, range clipboard (copy/cut/paste/duplicate), row/column insert/delete/move, data import/export helpers, range selection and Sheet Operations toolbar in the demo, and comprehensive tests validating these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/UI
participant Sheet as Sheet Core
participant Cells as Cell Storage
participant Clipboard as Clipboard State
rect rgb(220,245,220)
note over UI,Sheet: Search & Replace Flow
UI->>Sheet: find(query, options)
activate Sheet
Sheet->>Cells: iterateRange / match cells
Cells-->>Sheet: matched cells
Sheet-->>UI: result (Cell | null / matches[])
UI->>Sheet: replace(search, replacement, options)
Sheet->>Cells: update matching cells (replaceInCell)
Cells-->>Sheet: cells updated
Sheet-->>UI: replacement summary
deactivate Sheet
end
rect rgb(220,230,245)
note over UI,Clipboard: Copy/Cut/Paste Flow
UI->>Sheet: copyRange(range) / cutRange(range)
activate Sheet
Sheet->>Cells: snapshot values/styles/formulas for range
Cells-->>Sheet: range snapshot
Sheet->>Clipboard: store snapshot + metadata (coords, isRange)
Clipboard-->>Sheet: stored
Sheet-->>UI: copied/cut ack
UI->>Sheet: pasteRange(target, options)
Sheet->>Clipboard: retrieve snapshot
Clipboard-->>Sheet: snapshot
Sheet->>Cells: apply snapshot to target (respect transpose/valuesOnly/stylesOnly)
Cells-->>Sheet: target updated
Sheet-->>UI: paste complete
deactivate Sheet
end
sequenceDiagram
participant UI as User/UI
participant Sheet as Sheet Core
participant Cells as Cell Storage
rect rgb(245,220,220)
note over UI,Cells: Row/Column Insert/Delete/Move
UI->>Sheet: insertRow(index, count)
activate Sheet
Sheet->>Cells: identify & shift affected rows/cols
Cells-->>Sheet: shifted data
Sheet->>Cells: create empty rows/cols at index
Cells-->>Sheet: rows/cols inserted
Sheet-->>UI: operation complete
deactivate Sheet
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)demo/app.js (1)
src/core/Sheet.ts (4)
🪛 ast-grep (0.40.3)src/core/Sheet.ts[warning] 1524-1524: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns. (regexp-from-variable) 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
+ Coverage 92.52% 92.83% +0.31%
==========================================
Files 22 22
Lines 2862 3240 +378
Branches 795 921 +126
==========================================
+ Hits 2648 3008 +360
- Misses 214 232 +18 ☔ 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: 8
🧹 Nitpick comments (3)
demo/index.html (2)
62-67: Add proper labels for accessibility.The search and replace input fields lack associated
<label>elements, which are important for screen reader users. Consider adding visible labels or at minimumaria-labelattributes.🔎 Suggested improvement for accessibility
- <input type="text" id="searchInput" placeholder="Search..." class="toolbar-input"> + <label for="searchInput" class="sr-only">Search text</label> + <input type="text" id="searchInput" placeholder="Search..." class="toolbar-input" aria-label="Search text"> <button class="toolbar-btn" id="btnFind" title="Find">🔍 Find</button> <button class="toolbar-btn" id="btnFindNext" title="Find Next">Next</button> - <input type="text" id="replaceInput" placeholder="Replace with..." class="toolbar-input"> + <label for="replaceInput" class="sr-only">Replace text</label> + <input type="text" id="replaceInput" placeholder="Replace with..." class="toolbar-input" aria-label="Replace with text">You'll also need to add a CSS class for visually hidden labels:
.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0, 0, 0, 0); white-space: nowrap; border-width: 0; }
52-99: Consider adding keyboard shortcuts documentation.The toolbar hints mention using Ctrl+C/X/V but don't document other keyboard shortcuts that might be available. Consider adding more comprehensive keyboard navigation documentation for better user experience.
src/core/Sheet.ts (1)
2221-2255: ClarifytoObjectsbehavior whenrangeandheaderRoware both specified.The
headerRowparameter is treated as an absolute row index, not relative to the range. If a user specifiesrange: 'A5:C10'with the defaultheaderRow: 0, headers will be read from row 0 (outside the range) and data from rows 1-10.This might be intentional but could surprise users. Consider either:
- Making
headerRowrelative to the range's start row, or- Adding a JSDoc comment clarifying the behavior
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
CHANGELOG.mddemo/app.jsdemo/index.htmldemo/styles.csssrc/core/Sheet.tssrc/types/index.tssrc/types/range.types.tstests/copy-paste.test.tstests/data-helpers.test.tstests/row-column.test.tstests/search.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
tests/copy-paste.test.ts (1)
demo/app.js (22)
workbook(71-71)workbook(103-103)workbook(181-181)workbook(235-235)workbook(279-279)workbook(339-339)sheet(74-74)sheet(107-107)sheet(182-182)sheet(282-282)sheet(342-342)sheet(532-532)sheet(834-834)sheet(960-960)sheet(992-992)sheet(1041-1041)sheet(1109-1109)sheet(1131-1131)sheet(1146-1146)sheet(1161-1161)sheet(1176-1176)sheet(1210-1210)
tests/search.test.ts (1)
src/core/Sheet.ts (1)
cells(347-351)
demo/app.js (1)
src/core/Sheet.ts (2)
cell(178-205)cellCount(340-342)
src/core/Sheet.ts (2)
src/types/index.ts (4)
SearchOptions(71-71)RangeDefinition(64-64)parseRangeReference(76-76)PasteOptions(70-70)src/types/range.types.ts (4)
SearchOptions(263-276)RangeDefinition(7-12)parseRangeReference(22-48)PasteOptions(251-258)
⏰ 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)
- GitHub Check: Build WASM
🔇 Additional comments (10)
CHANGELOG.md (1)
10-54: Excellent documentation of new features!The changelog entry for version 0.3.0 is comprehensive and well-structured, clearly documenting all the new sheet operations functionality including search/replace, clipboard operations, row/column manipulation, and data import/export helpers. The format follows Keep a Changelog conventions and provides clear descriptions with method signatures.
demo/styles.css (1)
1044-1155: LGTM! Well-organized toolbar styling.The new CSS for the Sheet Operations Toolbar is well-structured and follows existing conventions. The use of existing CSS variables ensures visual consistency, and the class naming is clear and maintainable. The selection and highlight states are properly styled with appropriate specificity.
src/types/index.ts (1)
70-71: LGTM! Type exports follow existing patterns.The new type exports for
PasteOptionsandSearchOptionsare properly integrated into the existing type export structure and maintain consistency with the established patterns in this file.tests/search.test.ts (1)
1-219: Excellent test coverage for search functionality!This test suite is comprehensive and well-structured, covering all major use cases including exact/partial matches, regex, case sensitivity, range-limited searches, and different search targets (values, formulas, both). The test organization with clear describe blocks and descriptive test names makes it easy to understand what's being tested.
tests/copy-paste.test.ts (1)
1-186: LGTM! Comprehensive copy/paste test coverage.The test suite thoroughly covers all clipboard operations including copy, cut, paste with various options (valuesOnly, stylesOnly, transpose), duplicate, and clear. Edge cases like empty clipboard and formula preservation are properly tested.
tests/row-column.test.ts (1)
1-302: Excellent test coverage for row/column operations!This test suite comprehensively covers all row and column manipulation operations including insert, delete, and move. The tests properly verify data shifting, style preservation, and configuration handling. Edge cases like zero-count operations and same-position moves are appropriately tested.
src/types/range.types.ts (1)
260-276: Clarify regex priority over query in SearchOptions documentation.The implementation handles both
queryandregexfields by prioritizingregexwhen both are provided (see theif-else ifstructure in the matching logic at lines 1502-1512). However, this priority behavior is not documented in theSearchOptionsinterface JSDoc. Add a note to the interface or method documentation explicitly stating thatregextakes precedence if both fields are present.tests/data-helpers.test.ts (1)
1-259: Solid test coverage for data import/export helpers.The test suite provides good coverage for the happy paths of the new data helper methods. The round-trip tests at lines 227-258 are particularly valuable for ensuring data integrity.
Consider adding tests for edge cases:
- Sparse arrays with
undefinedelements- Objects with
nullorundefinedproperty values- Very large datasets (performance sanity check)
- Unicode/special characters in headers and values
demo/app.js (1)
761-814: Well-structured range selection implementation.The selection range utilities (
getSelectionRange,isCellSelected,updateSelectionHighlight,getSelectionA1) are cleanly implemented with proper normalization of start/end coordinates.src/core/Sheet.ts (1)
2264-2294:appendRowandappendRowsare well-implemented.Clean implementation that correctly handles empty sheets and calculates the next available row from dimensions.
| insertRow(rowIndex: number, count: number = 1): this { | ||
| if (count <= 0) return this; | ||
|
|
||
| // Disable events during restructuring | ||
| const wasEventsEnabled = this._eventsEnabled; | ||
| this._eventsEnabled = false; | ||
|
|
||
| try { | ||
| // Collect cells that need to be shifted | ||
| const cellsToShift: { key: string; cell: Cell; newRow: number }[] = []; | ||
|
|
||
| for (const [key, cell] of this._cells) { | ||
| if (cell.row >= rowIndex) { | ||
| cellsToShift.push({ key, cell: cell.clone(), newRow: cell.row + count }); | ||
| } | ||
| } | ||
|
|
||
| // First, delete all old cells | ||
| for (const { key } of cellsToShift) { | ||
| this._cells.delete(key); | ||
| } | ||
|
|
||
| // Then add cells at new positions | ||
| for (const { cell, newRow } of cellsToShift) { | ||
| const newCell = new Cell(newRow, cell.col, cell.value); | ||
| if (cell.style) newCell.style = cell.style; | ||
| if (cell.formula) newCell.setFormula('=' + cell.formula.formula, cell.formula.result); | ||
| if (cell.hyperlink) newCell.setHyperlink(cell.hyperlink.target, cell.hyperlink.tooltip); | ||
| if (cell.comment) newCell.setComment(cell.comment.text as string, cell.comment.author); | ||
| newCell._onChange = this.handleCellChange.bind(this); | ||
| this._cells.set(cellKey(newRow, cell.col), newCell); | ||
| } | ||
|
|
||
| // Shift row configurations | ||
| const rowConfigs = new Map<number, RowConfig>(); | ||
| for (const [idx, config] of this._rows) { | ||
| if (idx >= rowIndex) { | ||
| rowConfigs.set(idx + count, config); | ||
| } else { | ||
| rowConfigs.set(idx, config); | ||
| } | ||
| } | ||
| this._rows = rowConfigs; | ||
|
|
||
| this.recalculateDimensions(); | ||
| } finally { | ||
| this._eventsEnabled = wasEventsEnabled; | ||
| } | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /dev/null && find . -name "Sheet.ts" -path "*/src/core/*" | head -5Repository: abdullahmujahidali/Cellify
Length of output: 119
🏁 Script executed:
# First, let me find the correct path for Sheet.ts
find . -name "Sheet.ts" -type f 2>/dev/null | head -10Repository: abdullahmujahidali/Cellify
Length of output: 89
🏁 Script executed:
# Get an overview of the file structure
git ls-files | grep -i sheet | head -20Repository: abdullahmujahidali/Cellify
Length of output: 135
🏁 Script executed:
# List the directory structure
git ls-files | grep "src/core" | head -20Repository: abdullahmujahidali/Cellify
Length of output: 143
🏁 Script executed:
# Search for insertRow, insertColumn, deleteRow, deleteColumn methods
rg "insertRow|insertColumn|deleteRow|deleteColumn" -t typescript --no-heading | head -30Repository: abdullahmujahidali/Cellify
Length of output: 104
🏁 Script executed:
# Also search for _merges, _autoFilter, _conditionalFormats definitions
rg "_merges|_autoFilter|_conditionalFormats" -t typescript -B 2 -A 2 | head -100Repository: abdullahmujahidali/Cellify
Length of output: 104
🏁 Script executed:
# Check file size first
wc -l ./src/core/Sheet.tsRepository: abdullahmujahidali/Cellify
Length of output: 94
🏁 Script executed:
# Search for the methods without typescript flag
rg "insertRow|insertColumn|deleteRow|deleteColumn" ./src/core/Sheet.ts -B 2 -A 10Repository: abdullahmujahidali/Cellify
Length of output: 3310
🏁 Script executed:
# Search for _merges, _autoFilter, _conditionalFormats
rg "_merges|_autoFilter|_conditionalFormats" ./src/core/Sheet.ts -B 2 -A 2Repository: abdullahmujahidali/Cellify
Length of output: 2213
🏁 Script executed:
# Read specific lines around insertRow, insertColumn, etc.
rg -n "^\s*(insertRow|insertColumn|deleteRow|deleteColumn)\(" ./src/core/Sheet.tsRepository: abdullahmujahidali/Cellify
Length of output: 323
🏁 Script executed:
# Read insertRow method (lines 1721-1779)
sed -n '1721,1779p' ./src/core/Sheet.tsRepository: abdullahmujahidali/Cellify
Length of output: 2037
🏁 Script executed:
# Read insertColumn method (lines 1780-1836)
sed -n '1780,1836p' ./src/core/Sheet.tsRepository: abdullahmujahidali/Cellify
Length of output: 1950
🏁 Script executed:
# Read deleteRow method (lines 1837-1906)
sed -n '1837,1906p' ./src/core/Sheet.tsRepository: abdullahmujahidali/Cellify
Length of output: 2423
🏁 Script executed:
# Read deleteColumn method (lines 1907-1976)
sed -n '1907,1976p' ./src/core/Sheet.tsRepository: abdullahmujahidali/Cellify
Length of output: 2363
🏁 Script executed:
# Also check if there are any merge/autoFilter/conditionalFormat update calls in these methods
rg -n "this\._merges|this\._autoFilter|this\._conditionalFormats" ./src/core/Sheet.ts | grep -E "(172[0-9]|173[0-9]|174[0-9]|175[0-9]|176[0-9]|177[0-9]|178[0-9]|179[0-9]|180[0-9]|181[0-9]|182[0-9]|183[0-9]|184[0-9]|185[0-9]|186[0-9]|187[0-9]|188[0-9]|189[0-9]|190[0-9]|191[0-9]|192[0-9]|193[0-9]|194[0-9]|195[0-9])"Repository: abdullahmujahidali/Cellify
Length of output: 52
🏁 Script executed:
# Verify what recalculateDimensions does - does it handle merge ranges?
rg -n "recalculateDimensions" ./src/core/Sheet.ts -A 15 | head -40Repository: abdullahmujahidali/Cellify
Length of output: 1125
Row/column insertion and deletion don't update merge ranges, autoFilter, or conditional formatting ranges.
When inserting or deleting rows/columns, the following sheet metadata is not updated:
- Merge ranges (
_merges) - Merged cell boundaries should shift or expand - AutoFilter range (
_autoFilter) - Filter range should expand/contract - Conditional formatting ranges (
_conditionalFormats) - CF ranges should shift
For example, if a merge spans A1:B5 and you insert a row at index 3, the merge should extend to A1:B6, but it remains unchanged. This leads to incorrect behavior on export or when users interact with merged cells after structural changes.
Consider adding helper methods to shift these ranges:
private shiftMerges(rowIndex: number, count: number, isInsertion: boolean): void {
// Update _merges based on insertion/deletion
}
private shiftAutoFilter(rowIndex: number, count: number, isInsertion: boolean): void {
// Update _autoFilter based on insertion/deletion
}
private shiftConditionalFormats(rowIndex: number, count: number, isInsertion: boolean): void {
// Update _conditionalFormats based on insertion/deletion
}🤖 Prompt for AI Agents
In src/core/Sheet.ts around lines 1721-1771, insertRow currently moves cell data
and row configs but does not update merge ranges (_merges), autoFilter
(_autoFilter) or conditional formatting ranges (_conditionalFormats), causing
those ranges to remain incorrect after structural changes; add private helper
methods shiftMerges, shiftAutoFilter and shiftConditionalFormats that accept
(index: number, count: number, isInsertion: boolean) and call them from
insertRow (with isInsertion = true) so merges that start/end at/after rowIndex
are shifted or expanded, the autoFilter range is expanded/shifted if it
overlaps/starts at/after rowIndex, and each conditional format range is adjusted
similarly (for deletions handle collapsing/shortening ranges and removing
invalid ranges); after shifting ranges keep existing semantics (preserve ranges
before the index, shift indices >= rowIndex by +count on insert, -count on
delete, and drop ranges that become empty).
|
1 participant
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.