feat: implement cell change notifications and event types#18
feat: implement cell change notifications and event types#18abdullahmujahidali merged 4 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a typed sheet event system, per-cell change callbacks, undo/redo, sorting and filtering (types, API, and tests); introduces release automation (publish workflow + release script), updates CI/build scripts (removes WASM publish step), and adds docs and a CHANGELOG. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Sheet
participant Cell
participant Listener as EventListener
participant Buffer as ChangeBuffer
App->>Sheet: setValue(address, value)
Sheet->>Cell: getOrCreateCell(address)
Cell->>Cell: capture oldValue/oldStyle, apply change
alt actual change occurred
Cell->>Cell: invoke _onChange(changeType, old)
Cell-->>Sheet: notify handleCellChange(changeRecord)
Sheet->>Buffer: append changeRecord (id, ts)
alt events enabled
Sheet->>Listener: emit specific event
Sheet->>Listener: emit wildcard '*' event
end
end
App->>Sheet: getChanges()
Sheet-->>App: return buffered changeRecords
App->>Sheet: commitChanges()
Sheet->>Buffer: clear buffered changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 #18 +/- ##
==========================================
+ Coverage 91.57% 92.52% +0.94%
==========================================
Files 22 22
Lines 2446 2862 +416
Branches 667 795 +128
==========================================
+ Hits 2240 2648 +408
- Misses 206 214 +8 ☔ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/Cell.ts (1)
358-367: Add_onChangenotification toclear()method for proper change tracking.The
clear()method resets all cell properties but doesn't trigger_onChange, which means changes are not recorded. This is problematic becauseSheet.clearRange()relies onclear()to reset cells, and the public API allows directcell.clear()calls (as shown in documentation and demo code). UnlikedeleteCell()which handles the entire deletion event separately,clear()is a content-clearing operation that should emit change notifications likesetValue(),setFormula(), andsetStyle()do. Without this, cleared cells disappear from change tracking.
🧹 Nitpick comments (6)
scripts/release.sh (2)
69-74: Consider the order of operations for push vs. publish.If
npm publishfails (e.g., auth issue, network), the git tag will already be pushed to the remote. This could leave the repository in an inconsistent state where a version tag exists but the package wasn't published.Consider either:
- Publishing before pushing the tag, or
- Adding guidance in error recovery for this scenario
🔎 Alternative: publish before pushing tag
-echo -e "${GREEN}📤 Pushing to remote...${NC}" -git push origin main -git push origin "v${NEW_VERSION}" - -echo -e "${GREEN}📦 Publishing to npm...${NC}" -npm publish +echo -e "${GREEN}📦 Publishing to npm...${NC}" +npm publish + +echo -e "${GREEN}📤 Pushing to remote...${NC}" +git push origin main +git push origin "v${NEW_VERSION}"
56-58: Hardcoded repository URL limits portability.The GitHub URL is hardcoded. If the repository is ever renamed or forked, this will produce incorrect changelog links.
🔎 Proposed improvement: derive URL from git remote
+# Get repository URL from git remote +REPO_URL=$(git remote get-url origin | sed 's/\.git$//' | sed 's|git@github.com:|https://github.com/|') + sed -i.bak "/^\[${CURRENT_VERSION}\]:/i\\ -[${NEW_VERSION}]: https://github.com/abdullahmujahidali/Cellify/compare/v${CURRENT_VERSION}...v${NEW_VERSION} +[${NEW_VERSION}]: ${REPO_URL}/compare/v${CURRENT_VERSION}...v${NEW_VERSION} " CHANGELOG.md.github/workflows/publish.yml (1)
44-57: Updatesoftprops/action-gh-releaseto v2.The action version
@v1is outdated. Version 2 is the current stable release with improved features and compatibility.🔎 Proposed fix
- name: Create GitHub Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with: generate_release_notes: truesrc/core/Cell.ts (1)
169-180: Consider adding a no-op check forapplyStyleto avoid spurious events.
applyStylealways fires the_onChangecallback, even when the merge produces an identical style. For frequently called style operations, this could emit unnecessary events.🔎 Proposed optimization
applyStyle(style: Partial<CellStyle>): this { const oldStyle = this._style; if (!this._style) { this._style = { ...style }; } else { this._style = this.mergeStyles(this._style, style); } - if (this._onChange) { + if (this._onChange && this._style !== oldStyle) { this._onChange(this, 'style', oldStyle); } return this; }Note: Since
mergeStylesalways creates a new object,this._style !== oldStylewill always be true after the merge. A deep equality check would be needed for true change detection, but that may be overkill given the performance cost. The current behavior is acceptable.src/core/Sheet.ts (2)
189-192: Consider caching the bound handler to reduce memory allocation.Each cell creation calls
.bind(this), creating a new function instance per cell. For sheets with many cells, this could be optimized by binding once.🔎 Proposed optimization
Add a cached bound handler in the constructor:
+ private _boundHandleCellChange: (cell: Cell, changeType: 'value' | 'style' | 'formula', oldValue?: CellValue | CellStyle) => void; + constructor(name: string) { this._name = name; + this._boundHandleCellChange = this.handleCellChange.bind(this); }Then use it in the cell method:
- cell._onChange = this.handleCellChange.bind(this); + cell._onChange = this._boundHandleCellChange;
997-1013: Consider adding error isolation for event handlers.If a handler throws an exception, subsequent handlers (including wildcard listeners) won't execute. This could cause subtle bugs where some listeners miss events due to an unrelated handler's error.
🔎 Proposed fix with error isolation
private emit<K extends keyof SheetEventMap>(eventType: K, event: SheetEventMap[K]): void { // Emit to specific listeners const listeners = this._eventListeners.get(eventType); if (listeners) { for (const handler of listeners) { - handler(event); + try { + handler(event); + } catch (error) { + console.error(`Error in ${eventType} event handler:`, error); + } } } // Emit to wildcard listeners const wildcardListeners = this._eventListeners.get('*'); if (wildcardListeners) { for (const handler of wildcardListeners) { - handler(event); + try { + handler(event); + } catch (error) { + console.error(`Error in wildcard event handler:`, error); + } } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/ci.yml(1 hunks).github/workflows/publish.yml(1 hunks)CHANGELOG.md(1 hunks)docs-site/docs/guides/events.md(1 hunks)package.json(1 hunks)scripts/release.sh(1 hunks)src/core/Cell.ts(5 hunks)src/core/Sheet.ts(5 hunks)src/core/index.ts(1 hunks)src/types/event.types.ts(1 hunks)src/types/index.ts(1 hunks)tests/events.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/events.test.ts (3)
src/core/index.ts (1)
Workbook(5-5)src/types/event.types.ts (4)
CellChangeEvent(21-33)CellStyleChangeEvent(38-50)CellAddedEvent(74-82)CellDeletedEvent(87-97)src/types/index.ts (4)
CellChangeEvent(85-85)CellStyleChangeEvent(86-86)CellAddedEvent(88-88)CellDeletedEvent(89-89)
src/core/Cell.ts (2)
src/core/Sheet.ts (1)
cell(169-196)src/types/index.ts (2)
CellValue(43-43)CellStyle(18-18)
src/core/Sheet.ts (2)
src/types/event.types.ts (7)
SheetEventHandler(112-112)ChangeRecord(129-150)SheetEventMap(117-124)CellChangeEvent(21-33)CellStyleChangeEvent(38-50)CellAddedEvent(74-82)CellDeletedEvent(87-97)src/types/index.ts (9)
SheetEventHandler(91-91)ChangeRecord(93-93)SheetEventMap(92-92)CellValue(43-43)CellStyle(18-18)CellChangeEvent(85-85)CellStyleChangeEvent(86-86)CellAddedEvent(88-88)CellDeletedEvent(89-89)
🪛 actionlint (1.7.9)
.github/workflows/publish.yml
45-45: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (23)
.github/workflows/ci.yml (1)
16-16: LGTM!The Node.js version matrix change from
[18, 20, 22]to[20, 22]correctly aligns with theenginesconstraint inpackage.json(^20.19.0 || >=22.12.0)..github/workflows/publish.yml (1)
1-42: LGTM!The workflow structure is well-designed:
- Proper trigger on version tags
- Registry URL configured for npm authentication
- Complete test → build → verify → publish pipeline
- Secrets properly referenced
package.json (1)
37-43: LGTM!The changes correctly:
- Remove WASM build from
prepublishOnlyto streamline the publish process- Add release automation scripts that properly delegate to
scripts/release.sh- Default
releasescript correctly relies on the shell script's default (patch)src/core/index.ts (1)
2-2: LGTM!The new
CellChangeCallbacktype export follows the established pattern and properly exposes the event callback type as part of the public API for the cell change notification feature.src/types/index.ts (1)
82-94: LGTM!The new event type exports are well-organized and consistent with the existing export patterns in the file. All types align with the definitions in
event.types.ts.CHANGELOG.md (1)
1-83: LGTM!The changelog follows the Keep a Changelog format correctly and accurately documents both the new event system features (Unreleased) and the initial v0.1.0 release. The documented APIs align with the implementation.
docs-site/docs/guides/events.md (1)
1-236: LGTM!Comprehensive and well-structured documentation for the event system. The examples are practical and the integration patterns for Yjs and Liveblocks correctly demonstrate echo prevention using
setEventsEnabled(false)during remote update application. The documented type interfaces align with the definitions inevent.types.ts.tests/events.test.ts (1)
1-310: LGTM!Comprehensive test coverage for the event system including edge cases like unchanged values, unsubscription, enable/disable behavior, and change tracking. The wildcard listener test at lines 141-157 effectively validates event ordering and emission for the full lifecycle of cell operations.
src/core/Cell.ts (5)
15-22: LGTM!Well-defined callback type that cleanly separates the change type from the payload. The optional
oldValueparameter accommodates both value and style changes.
46-50: LGTM!Appropriate use of
@internalJSDoc tag to indicate this is not part of the public API. The underscore prefix convention reinforces this.
91-101: LGTM!Value setter correctly captures
oldValuebefore mutation, clears any existing formula, and only notifies when the value actually changes (oldValue !== val).
126-136: LGTM!Formula setter correctly captures the old value and always notifies, which is appropriate since setting a formula is always a meaningful change regardless of whether the formula text is identical.
157-164: LGTM!Style setter correctly captures
oldStyleand only notifies when the reference changes, which is appropriate for style object replacement.src/types/event.types.ts (1)
1-150: LGTM!Well-designed event type system with:
- Proper discriminated union via the
typeproperty on each event interface- Base
SheetEventinterface for common properties (sheetName,timestamp)- Generic
SheetEventHandler<T>for type-safe handlersSheetEventMapenabling type-safeon()/off()method signaturesChangeRecordfor change tracking with flexible optional fieldsThe
RangeChangeEventinterface is defined but not yet used—this provides good extensibility for future batch operations.src/core/Sheet.ts (9)
14-22: LGTM!The type imports are properly structured using
import typefor type-only imports, which is a TypeScript best practice for avoiding runtime overhead.
133-137: LGTM!The event system state is well-encapsulated with appropriate data structures:
MapwithSetfor efficient listener management (O(1) operations, automatic deduplication), and a simple counter for unique change IDs.
259-268: LGTM!Good design choice to emit
cellDeletedbefore removing from the map, allowing event handlers to access the cell's data. The boolean return value is a useful addition for callers to know if deletion occurred.
795-801: LGTM!The
onmethod correctly manages listener registration with type-safe generics. Using aSetensures the same handler isn't registered multiple times for the same event type.
810-816: LGTM!The
offmethod correctly handles unsubscription with graceful handling when the handler isn't found, which is the expected behavior for event systems.
833-843: LGTM!Simple and effective toggle for bulk operations. The fluent interface (
return this) maintains API consistency.
865-877: LGTM!The change tracking API is clean and follows immutability principles with the
readonlyreturn type. The example in the JSDoc clearly demonstrates the intended usage pattern for syncing changes.
950-962: LGTM!The
emitCellAddedimplementation correctly emits events only when enabled and doesn't record to the change buffer—appropriate since newly created cells are empty.
889-945: The implementation stores the computed value for formula changes by design. TheChangeRecordinterface distinguishes formula changes via thetypefield ('formula' vs 'value') but doesn't include separate formula text fields. If consumers need the formula string, they can access it directly viacell.formularather than from the change record.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/core/Sheet.ts (1)
1817-1842: Capture timestamp once to ensure event and change record have identical timestamps.
Date.now()is called twice (lines 1827 and 1838), which could result in slightly different timestamps for the same deletion operation. This issue was previously flagged in an earlier review.🔎 Proposed fix
private emitCellDeleted(cell: Cell): void { if (!this._eventsEnabled) return; + const timestamp = Date.now(); + const event: CellDeletedEvent = { type: 'cellDeleted', sheetName: this._name, address: cell.address, row: cell.row, col: cell.col, value: cell.value, - timestamp: Date.now(), + timestamp, }; // Record the change this._changes.push({ id: `${this._name}-${++this._changeIdCounter}`, type: 'delete', address: cell.address, row: cell.row, col: cell.col, oldValue: cell.value, - timestamp: Date.now(), + timestamp, }); this.emit('cellDeleted', event); }
🧹 Nitpick comments (2)
tests/sorting.test.ts (1)
259-269: Consider adding a test for sorting with mixed data types.The empty sheet test is good. Consider adding a test case for sorting a column with mixed data types (e.g., strings and numbers together) to verify the behavior is consistent and documented.
CHANGELOG.md (1)
36-36: Minor: Consider hyphenating compound adjectives.Per grammar conventions, "single column sorting" and "single column filtering" could be hyphenated as "single-column sorting" and "single-column filtering" when used as compound adjectives.
🔎 Suggested fix
- - `sheet.sort(column, options)` for single column sorting + - `sheet.sort(column, options)` for single-column sorting- - `sheet.filter(column, criteria)` for single column filtering + - `sheet.filter(column, criteria)` for single-column filteringAlso applies to: 48-48
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)docs-site/docs/guides/events.md(1 hunks)docs-site/docs/guides/sorting-filtering.md(1 hunks)src/core/Sheet.ts(6 hunks)src/types/index.ts(2 hunks)src/types/range.types.ts(1 hunks)tests/filtering.test.ts(1 hunks)tests/sorting.test.ts(1 hunks)tests/undo-redo.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs-site/docs/guides/sorting-filtering.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/types/range.types.ts (1)
src/types/index.ts (1)
FilterCriteria(70-70)
src/core/Sheet.ts (4)
src/types/index.ts (12)
SheetEventHandler(92-92)ChangeRecord(94-94)RangeDefinition(64-64)parseRangeReference(74-74)CellValue(43-43)FilterCriteria(70-70)SheetEventMap(93-93)CellStyle(18-18)CellChangeEvent(86-86)CellStyleChangeEvent(87-87)CellAddedEvent(89-89)CellDeletedEvent(90-90)src/types/event.types.ts (7)
SheetEventHandler(112-112)ChangeRecord(129-150)SheetEventMap(117-124)CellChangeEvent(21-33)CellStyleChangeEvent(38-50)CellAddedEvent(74-82)CellDeletedEvent(87-97)src/types/range.types.ts (3)
RangeDefinition(7-12)parseRangeReference(22-48)FilterCriteria(251-280)src/core/Cell.ts (4)
Cell(36-430)value(80-86)value(91-102)isEmpty(344-353)
🪛 GitHub Actions: CI
src/core/Sheet.ts
[error] 1223-1223: TS2345: Argument of type 'CellValue' is not assignable to parameter of type 'string | number | boolean | Date | null'.
🪛 GitHub Check: Test (Node 20)
src/core/Sheet.ts
[failure] 1223-1223:
Argument of type 'CellValue' is not assignable to parameter of type 'string | number | boolean | Date | null'.
🪛 LanguageTool
CHANGELOG.md
[grammar] ~36-~36: Use a hyphen to join words.
Context: ...sheet.sort(column, options) for single column sorting - `sheet.sortBy(columns...
(QB_NEW_EN_HYPHEN)
[grammar] ~48-~48: Use a hyphen to join words.
Context: ...eet.filter(column, criteria)for single column filtering -sheet.filterBy(fil...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (21)
src/types/index.ts (1)
70-70: LGTM!The new type exports for
FilterCriteriaand the event system types follow the existing pattern and are cleanly organized with appropriateexport typedeclarations.Also applies to: 83-95
src/types/range.types.ts (1)
248-280: Well-structured filter criteria interface.The interface is cleanly organized with logical groupings. One minor observation: the
customfunction acceptsDateas a value type, butequals,notEquals,in, andnotIndon't includeDatein their type unions. If filtering by exact date values is expected, consider addingDateto those types for consistency.docs-site/docs/guides/events.md (2)
1-7: LGTM!Comprehensive documentation covering the event system, change tracking, undo/redo, and integration patterns with collaboration libraries. The examples are practical and demonstrate best practices like disabling events during bulk/remote operations.
235-270: Good integration example with echo prevention.The Yjs integration example correctly demonstrates disabling events before applying remote changes to prevent infinite loops. This is a common pitfall in real-time collaboration implementations.
tests/sorting.test.ts (2)
1-3: LGTM!Comprehensive test suite covering sorting functionality including edge cases like null values, date sorting, style preservation, and empty sheets.
108-123: Good test for style preservation during sort.This test correctly verifies that cell styles move with their data during sorting, which is important for maintaining data integrity.
CHANGELOG.md (1)
1-7: LGTM!Well-structured changelog following Keep a Changelog format with proper semantic versioning links.
tests/undo-redo.test.ts (2)
1-3: LGTM!Comprehensive undo/redo test suite with excellent coverage of edge cases including history limits, batch operations, and multi-cell scenarios.
199-225: Good test for history limit enforcement.This test correctly verifies FIFO behavior where the oldest undo entries are dropped when the limit is exceeded, and that the user can only undo back to the oldest retained change.
tests/filtering.test.ts (1)
1-381: Excellent test coverage for filtering functionality!The test suite is comprehensive and well-organized, covering:
- All filter types (equals, contains, numeric comparisons, value lists, empty checks)
- String operations with case insensitivity
- Multi-column filtering
- Header row handling
- Filter clearing (all and per-column)
- Property accessors (activeFilters, filteredRows)
- Edge cases (empty sheets, column index vs letter notation)
The test structure is clean, assertions are appropriate, and the tests provide strong coverage for the filtering feature.
src/core/Sheet.ts (11)
724-850: Solid sorting implementation with proper event management.The sort method correctly:
- Handles both column letters and indices
- Preserves header rows when specified
- Clones cells before reordering to maintain data integrity
- Disables events during the reordering operation to avoid event spam
- Properly wires change callbacks on newly created cells
- Recalculates dimensions after sorting
The implementation is well-structured and handles edge cases appropriately.
852-971: Multi-column sorting correctly implements cascading comparisons.The sortBy method properly:
- Handles multiple sort columns with individual options (descending, numeric)
- Cascades through sort criteria until finding a difference
- Follows the same event-disabling pattern as the single-column sort
- Preserves all cell properties during reordering
The implementation is consistent with the single-column sort and correctly handles the multi-criteria sorting logic.
973-1005: Value comparison logic is well-implemented and deterministic.The compareValues method handles all value types appropriately:
- Nulls consistently sort to the end
- Numeric values support both number types and parseable strings
- Date comparison uses proper timestamp comparison
- String comparison leverages localeCompare for locale-aware sorting
1111-1150: Filter clearing logic correctly manages state.Both clearFilter and clearColumnFilter properly:
- Manage the _activeFilters map
- Show previously hidden rows
- Clear the _filteredRows set
- clearColumnFilter intelligently re-applies remaining filters after removing one
1173-1215: Filter application correctly implements AND logic for multi-column filters.The applyFilters method properly:
- Resets filter state by showing all previously filtered rows
- Applies AND logic across multiple column filters (all criteria must match)
- Manages the _filteredRows set and row visibility states
1226-1326: Filter criteria matching comprehensively handles all filter types.The matchesCriteria method correctly implements:
- Custom predicate functions (aside from the type issue at line 1223)
- Empty/non-empty checks with null, undefined, and empty string handling
- Case-insensitive string comparisons for equals/notEquals
- String operations (contains, startsWith, endsWith) with case-insensitivity
- Numeric comparisons with type coercion for string numbers
- Range checks (between, notBetween)
- Value list operations (in, notIn) with case-insensitive matching
- Sensible default (returns true if no criteria matched)
The logic aligns well with the comprehensive test coverage in filtering.test.ts.
1456-1495: Change tracking provides clean API for monitoring sheet modifications.The change tracking methods provide:
- Read-only access to pending changes via getChanges()
- Commit mechanism to clear changes after syncing
- Count accessor for quick checks
The readonly return type on getChanges prevents external modification of the change buffer, which is a good defensive practice.
1527-1570: Undo implementation correctly handles both single and batched changes.The undo method properly:
- Guards against empty undo stack
- Sets _isUndoRedoOperation flag to prevent recursive recording
- Handles batch changes by undoing in reverse order
- Moves the change to the redo stack
- Uses try/finally to ensure the flag is always reset
1585-1625: Redo implementation mirrors undo with correct forward ordering.The redo method properly:
- Guards against empty redo stack
- Handles batch changes in forward order (opposite of undo)
- Moves the change back to the undo stack
- Uses the same flag pattern as undo to prevent recursive recording
1665-1710: Batch operation cleverly groups multiple changes into single undo unit.The batch method:
- Captures all changes made during the batch execution
- Creates a composite ChangeRecord that represents the batch
- Stores individual changes in _batchChanges property for granular undo/redo
- Allows undoing multiple operations as a single user action
This is a well-designed feature for grouping related operations.
1712-1795: Cell change handler correctly manages tracking, undo/redo, and events.The handleCellChange method properly:
- Guards against disabled events
- Uses a single timestamp for consistency
- Tracks changes in the _changes array for monitoring
- Manages the undo/redo stacks with proper flag checking
- Clears redo stack when new changes occur (correct UX behavior)
- Enforces maximum undo history limit
- Emits strongly-typed events
This is the integration point that makes the event and undo systems work together seamlessly.
| describe('undo/redo with events disabled', () => { | ||
| it('should still track changes when events are disabled', () => { | ||
| const workbook = new Workbook(); | ||
| const sheet = workbook.addSheet('Test'); | ||
|
|
||
| sheet.setEventsEnabled(false); | ||
| sheet.cell('A1').value = 'Hello'; | ||
| sheet.setEventsEnabled(true); | ||
|
|
||
| // No undo available because events were disabled | ||
| expect(sheet.canUndo).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the test file
echo "=== Looking for test file ==="
find . -name "*undo*redo*.test.ts" -o -name "*undo*.test.ts" 2>/dev/null | head -20
# Look for events documentation
echo "=== Looking for events documentation ==="
find . -path "*/guides/*events*" -o -path "*/docs/*events*" 2>/dev/null | head -20Repository: abdullahmujahidali/Cellify
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Read the test file around lines 304-316
echo "=== Test file (lines 300-320) ==="
sed -n '300,320p' ./tests/undo-redo.test.ts
# Check the events documentation
echo ""
echo "=== Events documentation (first 100 lines) ==="
head -100 ./docs-site/docs/guides/events.md
# Search for mentions of undo, history, or tracking in events.md
echo ""
echo "=== Searching for undo/history/tracking mentions ==="
grep -n -i "undo\|history\|tracking" ./docs-site/docs/guides/events.mdRepository: abdullahmujahidali/Cellify
Length of output: 3886
🏁 Script executed:
#!/bin/bash
# Get more of the events.md file, focusing on the "Disabling Events" section
echo "=== Events documentation (lines 100-200) ==="
sed -n '100,200p' ./docs-site/docs/guides/events.md
# Also check if there's any mention of setEventsEnabled and undo together
echo ""
echo "=== Searching for setEventsEnabled mentions ==="
grep -n "setEventsEnabled" ./docs-site/docs/guides/events.mdRepository: abdullahmujahidali/Cellify
Length of output: 2163
🏁 Script executed:
#!/bin/bash
# Check if there's more context about the relationship between events and undo
echo "=== Full Disabling Events section ==="
sed -n '95,115p' ./docs-site/docs/guides/events.md
# Let's also check the test description again and surrounding context
echo ""
echo "=== Test context (lines 303-320) ==="
sed -n '303,320p' ./tests/undo-redo.test.tsRepository: abdullahmujahidali/Cellify
Length of output: 916
Document that disabling events disables undo/redo tracking.
The test confirms that when events are disabled via setEventsEnabled(false), changes are not tracked in the undo history. This trade-off should be documented in the events guide so users understand the behavior when using this performance optimization for bulk operations. Additionally, the test description is misleading—consider clarifying it to say "should not track changes in undo history when events are disabled" to match the actual assertion.
🤖 Prompt for AI Agents
In tests/undo-redo.test.ts around lines 304 to 316, the test name is misleading
and the repository docs don’t state that disabling events also disables
undo/redo tracking; change the test description to "should not track changes in
undo history when events are disabled" to match the assertion, and update the
events guide (e.g. docs/events.md or the appropriate events documentation file)
to add a short note that calling setEventsEnabled(false) disables event-based
undo/redo tracking so bulk operations won’t be recorded in the undo history.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.