packages/sheet/src/model/worksheet/sheet.ts (1)

2664-2670: ⚠️ Potential issue | 🟠 Major

Cross-sheet cache can still go stale through public mutation APIs.

Line 2666 says to invalidate when formulas change, but setCell() (Line 797) and setGrid() (Line 804) can mutate formulas without calling invalidateCrossSheetCache(). If cache was already built, later recalculateCrossSheetFormulas() can run against stale membership.

💡 Suggested fix

 async setCell(ref: Ref, cell: Cell): Promise<void> {
+  this.invalidateCrossSheetCache();
   await this.store.set(this.normalizeRefToAnchor(ref), cell);
 }

 async setGrid(grid: Grid): Promise<void> {
+  this.invalidateCrossSheetCache();
   await this.store.setGrid(grid);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sheet/src/model/worksheet/sheet.ts` around lines 2664 - 2670,
setCell() and setGrid() can mutate formulas without invalidating the cross-sheet
formula cache, so calls to recalculateCrossSheetFormulas() may run against stale
data; update the public mutation APIs (setCell and setGrid) to call
invalidateCrossSheetCache() whenever they change cell contents (or
conservatively on any successful mutation) so the cache is cleared when formulas
are added/removed/modified; reference the methods setCell, setGrid,
invalidateCrossSheetCache, and recalculateCrossSheetFormulas when making the
change.

packages/sheet/src/model/worksheet/sheet.ts (1)

1169-1173: Defer cache invalidation until after no-op guards.

Line 1169 and Line 3051 invalidate even when the operation exits early (no-op move / blocked move / already-sorted filter). This causes unnecessary cache rebuilds.

♻️ Suggested refactor

 private async moveCells(
   axis: Axis,
   src: number,
   count: number,
   dst: number,
   options?: { skipPostRecalculate?: boolean },
 ): Promise<void> {
-  this.invalidateCrossSheetCache();
   // No-op if source and destination are the same
   if (dst >= src && dst <= src + count) {
     return;
   }

   for (const [anchorSref, span] of this.merges) {
     const anchor = parseRef(anchorSref);
     if (isMergeSplitByMove(anchor, span, axis, src, count)) {
       return;
     }
   }

+  this.invalidateCrossSheetCache();
   await this.store.moveCells(axis, src, count, dst);

 async sortFilterByColumn(
   col: number,
   direction: 'asc' | 'desc',
 ): Promise<boolean> {
-  this.invalidateCrossSheetCache();
   if (!this.filterRange || !this.isColumnInFilter(col)) {
     return false;
   }
   ...
   const alreadySorted = desired.every((row, i) => row === dataStart + i);
   if (alreadySorted) {
     return true;
   }

+  this.invalidateCrossSheetCache();
   // Build row mapping: oldRow → newRow.
   const rowMap = new Map<number, number>();

Also applies to: 1175-1179, 3051-3054, 3104-3107

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/sheet/src/model/worksheet/sheet.ts` around lines 1169 - 1173, The
call to this.invalidateCrossSheetCache is happening before early-return/no-op
guards, causing unnecessary cache rebuilds; move the invalidateCrossSheetCache
invocation so it runs only after the method determines the operation will
proceed (i.e., after no-op checks/blocked-move checks/already-sorted checks).
Update each location that currently calls this.invalidateCrossSheetCache early
(the occurrences around the move/filtered/sort handlers—e.g., the moveRows,
filterRows, and sortRows code paths) to defer the invalidation until after the
guard that returns on no-op, so the method returns without touching the cache
when no change occurs. Ensure behavior remains identical when an actual mutation
occurs.

packages/frontend/tests/app/spreadsheet/yorkie-cross-sheet.integration.ts (1)

6-6: Consider adding a descriptive skip message for better test output.

When tests are skipped due to missing YORKIE_RPC_ADDR, the test output doesn't explain why. Adding context helps developers understand the skip reason.

-const shouldRun = Boolean(process.env.YORKIE_RPC_ADDR);
+const shouldRun = Boolean(process.env.YORKIE_RPC_ADDR);
+const skipReason = shouldRun ? false : "YORKIE_RPC_ADDR not set - skipping Yorkie integration tests";

Then use { skip: skipReason } in test options.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/frontend/tests/app/spreadsheet/yorkie-cross-sheet.integration.ts` at
line 6, The test currently uses shouldRun = Boolean(process.env.YORKIE_RPC_ADDR)
without explaining skips; define a descriptive skipReason (e.g.,
"YORKIE_RPC_ADDR not set - integration tests requiring Yorkie RPC are skipped"),
set shouldRun = Boolean(process.env.YORKIE_RPC_ADDR) as before, and pass the
skip option into the test(s) or suite (use { skip: skipReason } where tests are
registered) so skipped tests show the reason; update references to shouldRun and
the test registration to use the new skipReason when skipping.