Add pivot table feature (Phase 1)#26
Merged
Merged
Conversation
Port pivot table functionality from pivot-table-phase1 branch, cleanly separated from existing code without regressions. Key additions: - Pivot calculation pipeline (parse, group, aggregate, calculate, materialize) in sheet package - Pivot editor panel and hook for frontend - Store interface extended with setPivotDefinition/getPivotDefinition and invalidate() methods - Sheet model pivot protection (block direct edits on pivot sheets) - Context menu "Insert pivot table" option - Pivot tab creation in document layout - Comprehensive pivot tests (7 test files) - Design doc for pivot table architecture Co-Authored-By: Claude Opus 4.6 <[email protected]>
📝 WalkthroughWalkthroughAdds a Phase‑1 Pivot Table feature: design docs, pivot model types and store APIs, pivot calculation pipeline (parse, filter, group, aggregate, materialize), CRDT (Yorkie) persistence and sync, frontend UI/editor and hook, sheet integration (pivot tabs/protection), and extensive unit/e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Frontend (React)
participant Hook as usePivotTable Hook
participant CRDT as Yorkie Store
participant Engine as Sheet Engine
User->>Frontend: Insert pivot table (selection)
Frontend->>Frontend: create pivot tab (kind=pivot)
Frontend->>CRDT: setPivotDefinition(def)
CRDT-->>Frontend: persist & notify
Frontend->>Hook: update definition (add/remove fields)
Hook->>CRDT: sync definition
Hook->>Engine: calculatePivot(sourceGrid, definition)
Engine->>Engine: parse → filter → group → aggregate → materialize
Engine-->>Hook: PivotResult / Grid
Hook->>CRDT: write materialized cells to pivot tab
Frontend->>User: display updated pivot sheet
sequenceDiagram
participant UI as PivotEditorPanel
participant Hook as usePivotTable
participant Store as Yorkie Store
participant Sheet as Sheet Engine
UI->>Hook: user modifies fields / refresh
Hook->>Store: setPivotDefinition(updated)
Store-->>Hook: change event
Hook->>Sheet: build source grid & calculatePivot
Sheet->>Hook: materialized Grid
Hook->>Store: write cells to pivot tab
Store-->>UI: data update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 87.8s
Verification: verify:integrationResult: ✅ PASS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
packages/sheet/test/store/store.test.ts (1)
259-266: Also verifygetPivotDefinition()returns a defensive copy.This only proves
setPivotDefinition()clones on write. IfgetPivotDefinition()returns the stored object by reference, a caller can still mutate store state and this test stays green. Mutateresultbefore the second read to cover that path too.As per coding guidelines, "Write tests for critical business logic and edge cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheet/test/store/store.test.ts` around lines 259 - 266, The test currently only mutates the original `def` to prove cloning on write; also mutate the object returned by `getPivotDefinition()` (e.g., `result`) before calling `getPivotDefinition()` again to assert the store returns a defensive copy on read. In other words, after `const result = await store.getPivotDefinition();` change a nested field on `result` (such as `result.rowFields[0].label`) and then call `await store.getPivotDefinition()` again and assert the stored value remains unchanged; this verifies both `setPivotDefinition` and `getPivotDefinition` provide mutation safety.packages/sheet/src/model/pivot/materialize.ts (1)
22-41: Skip style-only'empty'cells when materializing.
usePivotTable.refresh()persists every cell returned from this function, so emitting{ v: '', s: { b: true } }for'empty'cells adds document churn without changing what users see. Dropping that case also lets you collapse the duplicated bold-header logic.♻️ Possible simplification
switch (pivotCell.type) { case 'rowHeader': case 'colHeader': - case 'empty': - cell = { v: pivotCell.value, s: { b: true } }; - break; case 'total': cell = { v: pivotCell.value, s: { b: true } }; break; + case 'empty': + break; case 'value': if (pivotCell.value !== '') { cell = { v: pivotCell.value }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheet/src/model/pivot/materialize.ts` around lines 22 - 41, The switch in materialize.ts currently creates a style-only cell for type 'empty' causing unnecessary writes; remove the 'empty' case and merge the bold-header handling so 'rowHeader', 'colHeader', and 'total' all set cell = { v: pivotCell.value, s: { b: true } }, while 'value' still only creates a cell when pivotCell.value !== ''. Ensure grid.set(sref, cell) remains guarded by cell !== undefined so empty cells are skipped.packages/sheet/test/sheet/pivot-calculate.test.ts (1)
27-45: Assert the total row/column in theshowTotalsscenario.This case turns on both row and column totals, but none of the assertions touch the total cells. A regression in grand-total materialization would still pass.
As per coding guidelines, "Write tests for critical business logic and edge cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheet/test/sheet/pivot-calculate.test.ts` around lines 27 - 45, The test enables showTotals for rows and columns but never asserts the grand totals; update the test that uses calculatePivot and the PivotTableDefinition (in pivot-calculate.test.ts) to include assertions for the total column and total row cells (e.g., the bottom-right grand total and the per-row/ per-column total cells) so the case verifies both row totals and column totals are materialized correctly when showTotals.rows and showTotals.columns are true.packages/frontend/src/app/spreadsheet/sheet-view.tsx (1)
152-155: Don't treat optionalTabMeta.kindas the only pivot signal.
kindis optional inpackages/frontend/src/types/worksheet.ts, but this component uses it as the sole source of truth for pivot UI. A document withroot.sheets[tabId]?.pivotTableset andkindmissing or stale will render as a normal sheet here while the engine still has pivot state. Either derive this from the stored pivot definition as a fallback or makekindmandatory for sheet tabs.As per coding guidelines, "Use type annotations in TypeScript to improve code clarity and catch errors early".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/spreadsheet/sheet-view.tsx` around lines 152 - 155, The current pivot detection uses TabMeta.kind alone (const isPivotTab = root?.tabs[tabId]?.kind === "pivot"), which misses cases where pivot state lives in root.sheets[tabId]?.pivotTable; update the isPivotTab computation in sheet-view.tsx to treat a tab as pivot if TabMeta.kind === "pivot" OR if root.sheets[tabId]?.pivotTable is present (use tabId, root, tabs, pivotTable, and TabMeta.kind as anchors), and add a TypeScript annotation for the isPivotTab variable to clarify its boolean type; alternatively, enforce TabMeta.kind as required across types if you prefer the schema change.packages/sheet/test/sheet/pivot-protection.test.ts (1)
121-130: Verify deep-clone behavior by mutating the returned definition.
expect(def).not.toBe(pivotDef)only proves the result is not the module-level fixture. A shallow clone of the sheet's cached definition would still pass here. Mutate a nested field ondefand assert a secondsheet.getPivotDefinition()call is unchanged so this covers the immutability contract callers rely on.As per coding guidelines, "Write tests for critical business logic and edge cases".♻️ Suggested test hardening
const def = sheet.getPivotDefinition(); expect(def).toEqual(pivotDef); // Verify it's a clone, not the same reference expect(def).not.toBe(pivotDef); + expect(def).toBeDefined(); + def!.showTotals.rows = false; + expect(sheet.getPivotDefinition()?.showTotals.rows).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheet/test/sheet/pivot-protection.test.ts` around lines 121 - 130, The test currently only checks reference inequality but not deep-clone immutability; after calling await sheet.loadPivotDefinition() and const def = sheet.getPivotDefinition(), mutate a nested property on def (e.g., change a nested field inside def.rows/def.settings or similar nested object) and then call sheet.getPivotDefinition() again and assert that the newly returned definition equals the original pivotDef (or at least that the mutated nested field was not changed), thereby proving getPivotDefinition() returns a deep clone rather than a shallow copy; use the existing MemStore, Sheet, pivotDef, loadPivotDefinition, and getPivotDefinition symbols to locate and implement this check.packages/sheet/src/model/pivot/calculate.ts (1)
142-146: Consider caching row index sets for performance.A new
Setis created for every row/column leaf intersection (line 144). For large pivot tables with many row and column combinations, this is O(rows × cols × indices) set constructions. Pre-building the row sets outside the inner loop would reduce redundant allocations.♻️ Optional performance improvement
// 6. Build data rows. + // Pre-build row index sets to avoid repeated Set construction + const rowSets = rowLeaves.map((leaf) => new Set(leaf.indices)); + for (const rowLeaf of rowLeaves) { const dataRow: PivotCell[] = [ { value: rowLeaf.label, type: 'rowHeader' }, ]; + const rowSet = rowSets[rowLeaves.indexOf(rowLeaf)]; if (hasColumns) { for (const colLeaf of colLeaves) { // Find intersection: record indices that belong to both row and column. - const rowSet = new Set(rowLeaf.indices); const intersection = colLeaf.indices.filter((i) => rowSet.has(i));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sheet/src/model/pivot/calculate.ts` around lines 142 - 146, The inner loop in calculate (in packages/sheet/src/model/pivot/calculate.ts) repeatedly constructs a new Set from rowLeaf.indices for each colLeaf, causing redundant allocations; fix it by precomputing a Map or array of Sets for all row leaves (e.g., build rowIndexSets keyed by rowLeaf or index position before iterating colLeaves) and then reuse rowIndexSets.get(rowLeaf) inside the colLeaves loop when computing intersection with colLeaf.indices; update references to rowLeaf.indices -> rowIndexSets.get(rowLeaf) in the intersection logic to avoid recreating Sets per iteration.packages/frontend/src/app/spreadsheet/yorkie-store.ts (1)
1205-1235: Consider extracting shared CRDT proxy reading logic.This method duplicates the proxy-reading logic found in
use-pivot-table.ts(readPivotProxy). Both manually extract fields to avoidstructuredCloneissues with Yorkie proxies. Consider extracting this to a shared utility function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/spreadsheet/yorkie-store.ts` around lines 1205 - 1235, getPivotDefinition duplicates the Yorkie CRDT proxy extraction logic already implemented as readPivotProxy in use-pivot-table.ts; extract that proxy-unwrapping logic into a shared utility (e.g., readPivotProxy or readPivotProxyFields) and replace the inline readField/Array.from(...) blocks inside getPivotDefinition with calls to the shared function, then update use-pivot-table.ts to call the same utility so both getPivotDefinition and readPivotProxy share the single implementation for rowFields, columnFields, valueFields, filterFields, and showTotals handling.packages/frontend/src/app/spreadsheet/pivot/pivot-editor-panel.tsx (1)
49-77: Addaria-expandedfor screen reader accessibility.The collapsible section button toggles visibility but doesn't communicate its state to assistive technologies.
♿ Accessibility improvement
<button type="button" className="flex w-full cursor-pointer items-center gap-1 text-xs font-semibold uppercase tracking-wide text-muted-foreground hover:text-foreground" onClick={() => setOpen(!open)} + aria-expanded={open} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/spreadsheet/pivot/pivot-editor-panel.tsx` around lines 49 - 77, The CollapsibleSection component's toggle button doesn't expose its state to assistive tech; update the button in CollapsibleSection to set aria-expanded={open} (and optionally add an aria-controls that points to the section content's id and ensure the content has that id) so screen readers know whether the section is open or closed; locate the button element in CollapsibleSection and add the aria attribute(s) tied to the open state.packages/frontend/src/app/spreadsheet/pivot/use-pivot-table.ts (1)
108-123: Potential echo loop when remote changes arrive.When a remote change arrives (Effect 2, line 100-103), it updates
definitionviasetDefinition. This triggers Effect 3 (line 108-123), which syncs the same change back to the Yorkie document. While this is likely a no-op at the CRDT level, it adds unnecessary overhead. Consider adding a flag to distinguish local vs remote updates.♻️ Optional improvement to skip re-syncing remote changes
+ const isRemoteUpdate = useRef(false); + // Reload definition when remote changes arrive (e.g. another user // modifies the pivot configuration). useEffect(() => { if (!doc) return; const unsub = doc.subscribe((event) => { if (event.type !== "remote-change") return; const root = doc.getRoot(); const ws = root.sheets?.[tabId]; if (ws?.pivotTable) { loadedTabIdRef.current = tabId; + isRemoteUpdate.current = true; setDefinition(readPivotProxy(ws.pivotTable)); } }); return unsub; }, [doc, tabId]); // Sync definition changes to Yorkie document. const definitionRef = useRef(definition); useEffect(() => { // Skip the initial load (when definitionRef.current transitions from null) const isInitialLoad = definitionRef.current === null && definition !== null; definitionRef.current = definition; if (isInitialLoad || !doc || !definition) return; + if (isRemoteUpdate.current) { + isRemoteUpdate.current = false; + return; + } // Guard: don't sync if tabId changed but definition hasn't re-loaded yet. if (loadedTabIdRef.current !== tabId) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/spreadsheet/pivot/use-pivot-table.ts` around lines 108 - 123, The sync effect (useEffect with definitionRef, doc.update, loadedTabIdRef, tabId, definition) can re-send remote-originated updates back into Yorkie; add a simple flag to skip re-sync for remote changes: introduce a ref like isRemoteUpdateRef, set it true in the effect that handles remote arrivals before calling setDefinition (where remote updates are applied), then in the sync effect (the block using definitionRef and doc.update) early-return if isRemoteUpdateRef.current is true and reset it to false after skipping so only local changes trigger doc.update; keep existing guards (loadedTabIdRef, tabId, definition) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@design/pivot-table.md`:
- Around line 264-275: The Store API docs are missing the new invalidate()
surface; update the Store interface section to include invalidate():
Promise<void> (or the correct signature) alongside setPivotDefinition and
getPivotDefinition, and mention that MemStore, YorkieStore, and ReadOnlyStore
implement it; reference the Store interface, methods
setPivotDefinition/getPivotDefinition, the new invalidate() method, and the
implementations MemStore, YorkieStore, ReadOnlyStore so readers know where the
behavior is added and used in external-write flows.
- Around line 292-307: The pivot-sheet protection section is incomplete: expand
it to document the full protection contract by listing all mutation entrypoints
that must be blocked when TabMeta.kind === "pivot" and/or Sheet.pivotDefinition
is present (e.g., Sheet.setData, any Sheet.applyMutation or cell-edit handlers,
public sheet APIs that modify cells or schema), specify the canonical shared
guard location to enforce (use the Sheet-level pivotDefinition check performed
in Sheet.setData as the model), and explicitly call out the one allowed
exception (internal materialization writes that use doc.update bypass Sheet
methods). Also describe initialization via loadPivotDefinition that sets the
cache and how callers should behave if the pivotDefinition is present
(early-return/error), so future contributors know which paths to change to
preserve protection.
In `@packages/sheet/src/model/pivot/group.ts`:
- Around line 74-79: The group key sort currently calls a localeCompare with an
undefined locale which uses host defaults and yields inconsistent ordering;
update the sort in group.ts where keys are derived (the keys =
[...groups.keys()].sort(...) block) to pass a fixed locale (e.g., 'en' or
'en-US') to localeCompare and keep the { numeric: true } option so ordering is
deterministic across environments, preserving the existing sortDir logic that
negates cmp for 'desc'.
In `@packages/sheet/src/view/worksheet.ts`:
- Line 266: After calling await this.sheet.loadPivotDefinition() in Worksheet,
short-circuit all edit-entry code paths by checking the pivot state: update
primeCellInputForSelection(), showCellInput(), and any paste/cut/edit shortcut
handlers to immediately return/do nothing when this.sheet.isPivotSheet() (or a
new pivotLoaded flag set after loadPivotDefinition resolves) is true; similarly
add the same guard where Worksheet currently primes/opens the inline editor so
pivot sheets never enter edit mode once the pivot definition is loaded.
---
Nitpick comments:
In `@packages/frontend/src/app/spreadsheet/pivot/pivot-editor-panel.tsx`:
- Around line 49-77: The CollapsibleSection component's toggle button doesn't
expose its state to assistive tech; update the button in CollapsibleSection to
set aria-expanded={open} (and optionally add an aria-controls that points to the
section content's id and ensure the content has that id) so screen readers know
whether the section is open or closed; locate the button element in
CollapsibleSection and add the aria attribute(s) tied to the open state.
In `@packages/frontend/src/app/spreadsheet/pivot/use-pivot-table.ts`:
- Around line 108-123: The sync effect (useEffect with definitionRef,
doc.update, loadedTabIdRef, tabId, definition) can re-send remote-originated
updates back into Yorkie; add a simple flag to skip re-sync for remote changes:
introduce a ref like isRemoteUpdateRef, set it true in the effect that handles
remote arrivals before calling setDefinition (where remote updates are applied),
then in the sync effect (the block using definitionRef and doc.update)
early-return if isRemoteUpdateRef.current is true and reset it to false after
skipping so only local changes trigger doc.update; keep existing guards
(loadedTabIdRef, tabId, definition) intact.
In `@packages/frontend/src/app/spreadsheet/sheet-view.tsx`:
- Around line 152-155: The current pivot detection uses TabMeta.kind alone
(const isPivotTab = root?.tabs[tabId]?.kind === "pivot"), which misses cases
where pivot state lives in root.sheets[tabId]?.pivotTable; update the isPivotTab
computation in sheet-view.tsx to treat a tab as pivot if TabMeta.kind ===
"pivot" OR if root.sheets[tabId]?.pivotTable is present (use tabId, root, tabs,
pivotTable, and TabMeta.kind as anchors), and add a TypeScript annotation for
the isPivotTab variable to clarify its boolean type; alternatively, enforce
TabMeta.kind as required across types if you prefer the schema change.
In `@packages/frontend/src/app/spreadsheet/yorkie-store.ts`:
- Around line 1205-1235: getPivotDefinition duplicates the Yorkie CRDT proxy
extraction logic already implemented as readPivotProxy in use-pivot-table.ts;
extract that proxy-unwrapping logic into a shared utility (e.g., readPivotProxy
or readPivotProxyFields) and replace the inline readField/Array.from(...) blocks
inside getPivotDefinition with calls to the shared function, then update
use-pivot-table.ts to call the same utility so both getPivotDefinition and
readPivotProxy share the single implementation for rowFields, columnFields,
valueFields, filterFields, and showTotals handling.
In `@packages/sheet/src/model/pivot/calculate.ts`:
- Around line 142-146: The inner loop in calculate (in
packages/sheet/src/model/pivot/calculate.ts) repeatedly constructs a new Set
from rowLeaf.indices for each colLeaf, causing redundant allocations; fix it by
precomputing a Map or array of Sets for all row leaves (e.g., build rowIndexSets
keyed by rowLeaf or index position before iterating colLeaves) and then reuse
rowIndexSets.get(rowLeaf) inside the colLeaves loop when computing intersection
with colLeaf.indices; update references to rowLeaf.indices ->
rowIndexSets.get(rowLeaf) in the intersection logic to avoid recreating Sets per
iteration.
In `@packages/sheet/src/model/pivot/materialize.ts`:
- Around line 22-41: The switch in materialize.ts currently creates a style-only
cell for type 'empty' causing unnecessary writes; remove the 'empty' case and
merge the bold-header handling so 'rowHeader', 'colHeader', and 'total' all set
cell = { v: pivotCell.value, s: { b: true } }, while 'value' still only creates
a cell when pivotCell.value !== ''. Ensure grid.set(sref, cell) remains guarded
by cell !== undefined so empty cells are skipped.
In `@packages/sheet/test/sheet/pivot-calculate.test.ts`:
- Around line 27-45: The test enables showTotals for rows and columns but never
asserts the grand totals; update the test that uses calculatePivot and the
PivotTableDefinition (in pivot-calculate.test.ts) to include assertions for the
total column and total row cells (e.g., the bottom-right grand total and the
per-row/ per-column total cells) so the case verifies both row totals and column
totals are materialized correctly when showTotals.rows and showTotals.columns
are true.
In `@packages/sheet/test/sheet/pivot-protection.test.ts`:
- Around line 121-130: The test currently only checks reference inequality but
not deep-clone immutability; after calling await sheet.loadPivotDefinition() and
const def = sheet.getPivotDefinition(), mutate a nested property on def (e.g.,
change a nested field inside def.rows/def.settings or similar nested object) and
then call sheet.getPivotDefinition() again and assert that the newly returned
definition equals the original pivotDef (or at least that the mutated nested
field was not changed), thereby proving getPivotDefinition() returns a deep
clone rather than a shallow copy; use the existing MemStore, Sheet, pivotDef,
loadPivotDefinition, and getPivotDefinition symbols to locate and implement this
check.
In `@packages/sheet/test/store/store.test.ts`:
- Around line 259-266: The test currently only mutates the original `def` to
prove cloning on write; also mutate the object returned by
`getPivotDefinition()` (e.g., `result`) before calling `getPivotDefinition()`
again to assert the store returns a defensive copy on read. In other words,
after `const result = await store.getPivotDefinition();` change a nested field
on `result` (such as `result.rowFields[0].label`) and then call `await
store.getPivotDefinition()` again and assert the stored value remains unchanged;
this verifies both `setPivotDefinition` and `getPivotDefinition` provide
mutation safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8257e8e-d135-4e71-ba12-6f4000549c5c
📒 Files selected for processing (31)
design/README.mddesign/pivot-table.mdpackages/frontend/src/app/documents/document-detail.tsxpackages/frontend/src/app/spreadsheet/pivot/pivot-editor-panel.tsxpackages/frontend/src/app/spreadsheet/pivot/use-pivot-table.tspackages/frontend/src/app/spreadsheet/sheet-view.tsxpackages/frontend/src/app/spreadsheet/yorkie-store.tspackages/frontend/src/components/sheet-context-menu.tsxpackages/frontend/src/types/worksheet.tspackages/sheet/src/index.tspackages/sheet/src/model/pivot/aggregate.tspackages/sheet/src/model/pivot/calculate.tspackages/sheet/src/model/pivot/group.tspackages/sheet/src/model/pivot/index.tspackages/sheet/src/model/pivot/materialize.tspackages/sheet/src/model/pivot/parse.tspackages/sheet/src/model/sheet.tspackages/sheet/src/model/types.tspackages/sheet/src/store/memory.tspackages/sheet/src/store/readonly.tspackages/sheet/src/store/store.tspackages/sheet/src/view/spreadsheet.tspackages/sheet/src/view/worksheet.tspackages/sheet/test/sheet/pivot-aggregate.test.tspackages/sheet/test/sheet/pivot-calculate.test.tspackages/sheet/test/sheet/pivot-e2e.test.tspackages/sheet/test/sheet/pivot-group.test.tspackages/sheet/test/sheet/pivot-materialize.test.tspackages/sheet/test/sheet/pivot-parse.test.tspackages/sheet/test/sheet/pivot-protection.test.tspackages/sheet/test/store/store.test.ts
- Pin locale to 'en' in pivot group sorting with Intl.Collator for deterministic ordering across environments - Cascade-delete dependent pivot tabs when source sheet is deleted, with a confirmation dialog listing affected pivots - Block edit mode entry on pivot sheets by guarding showCellInput, primeCellInputForSelection, and cut shortcut with isPivotSheet() Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/frontend/src/app/documents/document-detail.tsx (1)
421-429:⚠️ Potential issue | 🟠 MajorDelete the full pivot dependency closure, not just direct children.
SheetViewnow getsaddPivotTabfor every sheet tab, and its insert handler uses the currenttabIdassourceTabId. This scan only removes pivots whosesourceTabIdmatches the tab being deleted, so a pivot built from another pivot will survive with a broken source reference after the base tab is removed. Either walk the dependency graph transitively here, or suppress pivot creation on pivot tabs for Phase 1.Also applies to: 560-560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/documents/document-detail.tsx` around lines 421 - 429, The current deletion only collects direct pivot children by checking pivotTable.sourceTabId against tabId (using dependentPivotIds, root.tabOrder, root.sheets, pivotTable, sourceTabId), which leaves pivots-of-pivots orphaned; change the logic to compute the transitive closure of pivot dependencies instead: perform a BFS/DFS starting from the initial tabId over the pivot graph (treat each sheet id as a node and edges from a pivot tab to its sourceTabId) to collect all reachable pivot tab ids, then delete all those pivot tabs (rather than only direct children). Update the code that builds dependentPivotIds to use this traversal so any pivot built from another pivot is also removed.
🤖 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/documents/document-detail.tsx`:
- Around line 457-473: confirmPendingDelete currently recomputes dependents at
click-time and proceeds to delete, which can remove tabs the user didn't confirm
or leave zero tabs; instead, before calling deleteTabWithDependents, re-validate
that the live dependent set equals the snapshot shown in the dialog
(dependentNames stored when opening the modal) and that deleting the snapshot +
pendingDelete.tabId would not remove the last remaining tab. If the live set
differs from the snapshot or the post-delete tab count would be zero, abort (or
show an error/toast) and clear/refresh the modal; only call
deleteTabWithDependents(pendingDelete.tabId, dependentPivotIds) when the
validated sets match and the one-tab-minimum invariant holds. Ensure you
reference confirmPendingDelete, pendingDelete, doc, dependentNames,
deleteTabWithDependents, and setPendingDelete when implementing this check.
In `@packages/sheet/src/view/worksheet.ts`:
- Around line 4478-4479: The current pivot-edit guard (checking this.readOnly ||
!this.sheet || this.sheet.isPivotSheet()) is only used for inline edits; create
a shared method (e.g., canStartEdit or isEditAllowed) that encapsulates these
checks (readOnly, sheet existence, and sheet.isPivotSheet()) and replace the
inline guard in the inline editor path as well as call it from
handleMobileDoubleTap (before invoking mobileEditCallback/showCellInput) and
from finishEditing (the formula-bar edit entry path) so pivot sheets and
readOnly mode cannot start edits via mobile or formula-bar routes.
- Line 266: After calling this.sheet.loadPivotDefinition() you must clear any
active editor and reset edit state when the sheet becomes a pivot: detect when
this.isPivotSheet() returns true after loadPivotDefinition() and then hide/reset
the UI editor state (clear cellInput and any primed/formula session) so
primeCellInputForSelection() and showCellInput() won't leave a stale editor
visible; update the worksheet logic around loadPivotDefinition() to call the
same hide/reset routine used when cancelling edits (or explicitly clear
cellInput and related flags) immediately after loadPivotDefinition() if
isPivotSheet() is true.
---
Duplicate comments:
In `@packages/frontend/src/app/documents/document-detail.tsx`:
- Around line 421-429: The current deletion only collects direct pivot children
by checking pivotTable.sourceTabId against tabId (using dependentPivotIds,
root.tabOrder, root.sheets, pivotTable, sourceTabId), which leaves
pivots-of-pivots orphaned; change the logic to compute the transitive closure of
pivot dependencies instead: perform a BFS/DFS starting from the initial tabId
over the pivot graph (treat each sheet id as a node and edges from a pivot tab
to its sourceTabId) to collect all reachable pivot tab ids, then delete all
those pivot tabs (rather than only direct children). Update the code that builds
dependentPivotIds to use this traversal so any pivot built from another pivot is
also removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b79fe80e-d1bc-46b1-a41d-52a143059891
📒 Files selected for processing (3)
packages/frontend/src/app/documents/document-detail.tsxpackages/sheet/src/model/pivot/group.tspackages/sheet/src/view/worksheet.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sheet/src/model/pivot/group.ts
Collaborator
Author
|
@coderabbitai Thanks for the thorough review on the latest commit. Here's our assessment of the 3 new Major comments: #1 Stale delete confirmation — The race condition requires another client to add/remove tabs during the few seconds the dialog is open. We already re-collect dependents at confirm time, which is a reasonable safeguard for this edge case. We'll revisit if collaboration-heavy usage reveals real issues. #2 Reset editor on pivot load — #3 Mobile/formula-bar edit bypass — All three are defensive improvements for edge cases that are unlikely in Phase 1. We'll harden these paths as the pivot feature matures. |
|
Happy to close out these comments. If you'd like to track the hardening items for Phase 2/3, I can open follow-up issues for any or all of them. Just let me know! ✅ Actions performedComments resolved. Auto-approval is disabled; enable |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setPivotDefinition/getPivotDefinitionandinvalidate()for external write supportTest plan
pnpm verify:fast)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UX
Tests