Shift chart and pivot table ranges on row/column changes#147
Conversation
Add parseColumnLabel, shiftColumnLabel, shiftA1Range, moveColumnLabel, and moveA1Range as pure building-block functions for chart and pivot table range shifting. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
When rows or columns are inserted, deleted, or moved, chart sourceRange, xAxisColumn, and seriesColumns are now updated to reflect the structural change. Previously only the chart anchor position was shifted. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Pivot table sourceRange is now updated when structural changes occur in both MemStore and Yorkie document layers. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Chart and pivot sourceRange references point to data on other tabs. The previous implementation only shifted ranges within the same worksheet, missing pivot tables (always on a separate tab) and charts referencing other tabs. Now shiftCells/moveCells iterates all tabs to update chart/pivot ranges whose sourceTabId matches the modified tab. Also filters chart range shifts by sourceTabId to avoid incorrect updates. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds cross-tab data range update functionality to the spreadsheet system. When a worksheet is shifted or moved structurally, the YorkieStore now invokes new helper functions that automatically adjust chart and pivot table data ranges if they reference the modified source tab. The sheets library exports new coordinate-parsing and range-transformation utilities to support these updates. Changes
Sequence DiagramsequenceDiagram
participant Client
participant YorkieStore
participant Worksheet Structure
participant Charts/Pivots
Client->>YorkieStore: shiftCells() / moveCells()
YorkieStore->>YorkieStore: applyYorkieWorksheetShift() / applyYorkieWorksheetMove()
YorkieStore->>Worksheet Structure: shiftCrossTabDataRanges() / moveCrossTabDataRanges()
Worksheet Structure->>Worksheet Structure: Iterate through all sheets
alt sourceTabId matches modified tab
Worksheet Structure->>Worksheet Structure: Transform chart sourceRange via shiftA1Range/moveA1Range
Worksheet Structure->>Worksheet Structure: Update xAxisColumn via shiftColumnLabel/moveColumnLabel
Worksheet Structure->>Worksheet Structure: Update seriesColumns entries
Worksheet Structure->>Charts/Pivots: Apply updated ranges
else sourceTabId does not match
Worksheet Structure->>Charts/Pivots: Leave ranges unchanged
end
Worksheet Structure-->>YorkieStore: Complete
YorkieStore-->>Client: Operation finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Verification: verify:selfResult: ✅ PASS in 123.1s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
shiftA1Range previously returned null when either range endpoint fell in the deleted zone. For chart/pivot data ranges, this is too aggressive — deleting the first few rows of a chart range should shrink it, not lose it entirely. Now clamps deleted endpoints to the deletion boundary so ranges survive partial deletions. Also removes incorrect pivot shift from MemStore (single-tab store cannot do cross-tab sourceTabId checks). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/sheets/src/model/worksheet/shifting.ts (1)
499-500: Silent passthrough for non-range strings may mask bugs.
if (parts.length !== 2) return range;silently returns the input unchanged for single-cell refs (e.g."A1"), comma-joined multi-ranges (e.g."A1:B2,C1:D2"), or an empty string. A log/throw or at least a doc comment on the expected input format would make misuse easier to detect. Not critical since current callers always passstart:endstrings, but worth documenting.packages/sheets/test/sheet/shifting.test.ts (1)
519-530: Consider adding a test for moves that split a range.The current
moveA1Rangetests only exercise ranges that lie entirely inside the moved block or entirely outside it. Adding a case where the move covers only one endpoint of the range (e.g.moveA1Range('C1:D10', 'column', 3, 1, 10)) would lock down the behavior for the inversion edge case called out inshifting.ts.it('should keep range valid when move splits the range', () => { // Moving col C out while D stays — current output "I1:C10" is inverted. const result = moveA1Range('C1:D10', 'column', 3, 1, 10); // Define the expected semantics; at minimum, start <= end. const [start, end] = result.split(':'); // e.g. expect(parseRef(start).c).toBeLessThanOrEqual(parseRef(end).c); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheets/test/sheet/shifting.test.ts` around lines 519 - 530, Add a unit test for the split-move edge case to ensure moveA1Range preserves valid (non-inverted) ranges when the move covers only one endpoint; specifically call moveA1Range('C1:D10', 'column', 3, 1, 10) and assert the returned "start:end" has start <= end by parsing both endpoints (use parseRef or the existing A1 parser used elsewhere in tests) and checking the column (and/or row) of the start is <= the end, plus any additional semantic expectations you want to lock down.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/src/app/spreadsheet/yorkie-worksheet-structure.ts`:
- Around line 393-398: The pivotTable.sourceRange is left unchanged when
shiftA1Range(ws.pivotTable.sourceRange, axis, index, count) returns null,
leaving the pivot pointing at deleted data; update the block handling
ws.pivotTable (use ws.pivotTable, shiftA1Range, pivotTable.sourceRange) so that
if shifted is null you either set ws.pivotTable.sourceRange = undefined/null or
remove ws.pivotTable entirely (choose the same behavior used for charts
elsewhere), and ensure any dependent state/notifications are updated accordingly
so the worksheet no longer references a stale range.
- Around line 118-150: The shiftOneChartRange function leaves chart.sourceRange
and chart.xAxisColumn stale when shiftA1Range or shiftColumnLabel return null;
update shiftOneChartRange so that when shiftA1Range(chart.sourceRange, ...)
returns null you clear/unset chart.sourceRange (or mark the chart for deletion),
and similarly when shiftColumnLabel returns null you remove/unset
chart.xAxisColumn (not just skip), while continuing to filter nulls out of
chart.seriesColumns; apply the same consistent behavior for
pivotTable.sourceRange (use shiftA1Range and clear/unset pivotTable.sourceRange
or drop the pivotTable when it becomes invalid) so
sourceRange/xAxisColumn/seriesColumns handling is consistent across
shiftOneChartRange and pivotTable updates.
In `@packages/sheets/src/model/worksheet/shifting.ts`:
- Around line 551-564: moveA1Range can produce inverted ranges when moveRef
remaps endpoints independently; after computing start =
moveRef(parseRef(parts[0]), ...) and end = moveRef(parseRef(parts[1]), ...),
detect if the moved axis coordinate of start is greater than end and swap that
axis' values so the returned "start:end" is normalized; use the same
parsed/returned ref structures (from parseRef/moveRef/toSref) to swap either
column indices for axis === 'column' or row indices for axis === 'row' before
calling toSref, preserving other coordinates.
---
Nitpick comments:
In `@packages/sheets/test/sheet/shifting.test.ts`:
- Around line 519-530: Add a unit test for the split-move edge case to ensure
moveA1Range preserves valid (non-inverted) ranges when the move covers only one
endpoint; specifically call moveA1Range('C1:D10', 'column', 3, 1, 10) and assert
the returned "start:end" has start <= end by parsing both endpoints (use
parseRef or the existing A1 parser used elsewhere in tests) and checking the
column (and/or row) of the start is <= the end, plus any additional semantic
expectations you want to lock down.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8afa84e3-f63d-4585-a802-f582851c9670
📒 Files selected for processing (7)
packages/frontend/src/app/spreadsheet/yorkie-store.tspackages/frontend/src/app/spreadsheet/yorkie-worksheet-structure.tspackages/sheets/src/index.tspackages/sheets/src/model/core/coordinates.tspackages/sheets/src/model/worksheet/shifting.tspackages/sheets/src/store/memory.tspackages/sheets/test/sheet/shifting.test.ts
| function shiftOneChartRange( | ||
| chart: SheetChart, | ||
| axis: Axis, | ||
| index: number, | ||
| count: number, | ||
| ): void { | ||
| if (chart.sourceRange) { | ||
| const shifted = shiftA1Range(chart.sourceRange, axis, index, count); | ||
| if (shifted) { | ||
| chart.sourceRange = shifted; | ||
| } | ||
| } | ||
|
|
||
| if (axis === 'column') { | ||
| if (chart.xAxisColumn) { | ||
| const shifted = shiftColumnLabel(chart.xAxisColumn, index, count); | ||
| if (shifted) { | ||
| chart.xAxisColumn = shifted; | ||
| } | ||
| } | ||
|
|
||
| if (chart.seriesColumns) { | ||
| const result: string[] = []; | ||
| for (const col of chart.seriesColumns) { | ||
| const shifted = shiftColumnLabel(col, index, count); | ||
| if (shifted) { | ||
| result.push(shifted); | ||
| } | ||
| } | ||
| chart.seriesColumns = result; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale chart config when a referenced range/column is fully deleted.
shiftA1Range and shiftColumnLabel return null when the entire range / the referenced column is deleted, but this branch is simply skipped:
chart.sourceRangekeeps pointing at data rows/columns that no longer exist.chart.xAxisColumnkeeps the old label even when that column was deleted — whileseriesColumnscorrectly filters null entries, so behavior is inconsistent between the two fields.
Consider clearing (or unsetting) the field, or dropping the chart entirely, when the source becomes invalid — the same question applies to pivotTable.sourceRange below. Whatever the decision, it should at least be consistent across sourceRange / xAxisColumn / seriesColumns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/app/spreadsheet/yorkie-worksheet-structure.ts` around
lines 118 - 150, The shiftOneChartRange function leaves chart.sourceRange and
chart.xAxisColumn stale when shiftA1Range or shiftColumnLabel return null;
update shiftOneChartRange so that when shiftA1Range(chart.sourceRange, ...)
returns null you clear/unset chart.sourceRange (or mark the chart for deletion),
and similarly when shiftColumnLabel returns null you remove/unset
chart.xAxisColumn (not just skip), while continuing to filter nulls out of
chart.seriesColumns; apply the same consistent behavior for
pivotTable.sourceRange (use shiftA1Range and clear/unset pivotTable.sourceRange
or drop the pivotTable when it becomes invalid) so
sourceRange/xAxisColumn/seriesColumns handling is consistent across
shiftOneChartRange and pivotTable updates.
| if (ws.pivotTable?.sourceTabId === sourceTabId && ws.pivotTable.sourceRange) { | ||
| const shifted = shiftA1Range(ws.pivotTable.sourceRange, axis, index, count); | ||
| if (shifted) { | ||
| ws.pivotTable.sourceRange = shifted; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same stale-range concern for pivotTable.sourceRange.
When shiftA1Range(...) returns null (the pivot's source range was fully deleted), the pivot keeps its original sourceRange, which will now point at non-existent data. Recommend either clearing sourceRange or removing pivotTable altogether on full deletion — same question as for charts above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/app/spreadsheet/yorkie-worksheet-structure.ts` around
lines 393 - 398, The pivotTable.sourceRange is left unchanged when
shiftA1Range(ws.pivotTable.sourceRange, axis, index, count) returns null,
leaving the pivot pointing at deleted data; update the block handling
ws.pivotTable (use ws.pivotTable, shiftA1Range, pivotTable.sourceRange) so that
if shifted is null you either set ws.pivotTable.sourceRange = undefined/null or
remove ws.pivotTable entirely (choose the same behavior used for charts
elsewhere), and ensure any dependent state/notifications are updated accordingly
so the worksheet no longer references a stale range.
| export function moveA1Range( | ||
| range: string, | ||
| axis: Axis, | ||
| src: number, | ||
| count: number, | ||
| dst: number, | ||
| ): string { | ||
| const parts = range.split(':'); | ||
| if (parts.length !== 2) return range; | ||
|
|
||
| const start = moveRef(parseRef(parts[0]), axis, src, count, dst); | ||
| const end = moveRef(parseRef(parts[1]), axis, src, count, dst); | ||
| return toSref(start) + ':' + toSref(end); | ||
| } |
There was a problem hiding this comment.
moveA1Range can produce inverted ranges when a move splits the range.
When the move block covers only part of a range's span, the two endpoints are remapped independently and can cross over, producing an invalid start:end string.
Example: moveA1Range('C1:D10', 'column', 3, 1, 10) yields "I1:C10" because:
- start
c=3→remapIndex(3,3,1,10) = 9("I") - end
c=4→remapIndex(4,3,1,10) = 3("C")
Downstream parsers that call parseRange on this will happily accept it but produce a negative-sized range. At minimum the result should be normalized so start <= end on the moved axis; ideally the semantics of a split move should be defined (e.g., clamp to the surviving contiguous block, or preserve the bounding span).
🔧 Suggested minimum fix — normalize endpoints so the range is never inverted
export function moveA1Range(
range: string,
axis: Axis,
src: number,
count: number,
dst: number,
): string {
const parts = range.split(':');
if (parts.length !== 2) return range;
const start = moveRef(parseRef(parts[0]), axis, src, count, dst);
const end = moveRef(parseRef(parts[1]), axis, src, count, dst);
- return toSref(start) + ':' + toSref(end);
+ const normStart = {
+ r: Math.min(start.r, end.r),
+ c: Math.min(start.c, end.c),
+ };
+ const normEnd = {
+ r: Math.max(start.r, end.r),
+ c: Math.max(start.c, end.c),
+ };
+ return toSref(normStart) + ':' + toSref(normEnd);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sheets/src/model/worksheet/shifting.ts` around lines 551 - 564,
moveA1Range can produce inverted ranges when moveRef remaps endpoints
independently; after computing start = moveRef(parseRef(parts[0]), ...) and end
= moveRef(parseRef(parts[1]), ...), detect if the moved axis coordinate of start
is greater than end and swap that axis' values so the returned "start:end" is
normalized; use the same parsed/returned ref structures (from
parseRef/moveRef/toSref) to swap either column indices for axis === 'column' or
row indices for axis === 'row' before calling toSref, preserving other
coordinates.
When a move splits a range (e.g. moving one endpoint's column far away), the two endpoints can cross over producing an invalid inverted range like "I1:C10". Now normalizes start/end so the result is always a valid range. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
chart-pivot-range-shift (#147), table-row-splitting (#145), and table-copy-paste (#140) are all merged. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 participant
Summary
shiftA1Range,moveA1Range,shiftColumnLabel,moveColumnLabelutility functions topackages/sheetsfor shifting A1-notation range strings and column labelssourceRange,xAxisColumn, andseriesColumnswhen rows/columns are inserted, deleted, or movedsourceRangewhen rows/columns are inserted, deleted, or movedsourceTabIdmatches the modified tab (pivot tables always live on a separate tab from their source data)Test plan
parseColumnLabel,shiftColumnLabel,shiftA1Range,moveColumnLabel,moveA1Rangepnpm verify:fastpasses (lint + all unit tests)sourceRangeexpandedsourceRangeupdatedxAxisColumn→ verify column removed from config🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests