packages/sheet/src/model/core/coordinates.ts (1)

442-443: Make single-item return path clone-consistent in mergeOverlapping.

Line 442 returns a shallow array copy for length === 1, while later paths clone ranges. Returning [cloneRange(ranges[0])] keeps mutability behavior consistent.

♻️ Proposed refactor

 export function mergeOverlapping(ranges: Ranges): Ranges {
-  if (ranges.length <= 1) return [...ranges];
+  if (ranges.length === 0) return [];
+  if (ranges.length === 1) return [cloneRange(ranges[0])];

🤖 Prompt for AI Agents

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

In `@packages/sheet/src/model/core/coordinates.ts` around lines 442 - 443, The
single-item early return in mergeOverlapping currently returns a shallow
reference [...ranges] which is inconsistent with later paths that return cloned
ranges; change the early-return to return a cloned single item by using
cloneRange(ranges[0]) so callers always receive independent Range objects;
locate mergeOverlapping and replace the return [...ranges] for the length === 1
case with an array containing cloneRange(ranges[0]) to ensure consistent
immutability behavior.

packages/sheet/src/model/core/coordinates.test.ts (1)

120-152: Add parseRanges edge-case coverage for empty/invalid segments.

Given this suite already defines serialization behavior for empty ranges (Line 110-112), please also assert parse behavior for '' and malformed comma segments (e.g., 'A1,,B2') to prevent regressions.

🧪 Suggested tests

   describe('parseRanges', () => {
+    it('should return empty array for empty string', () => {
+      expect(parseRanges('')).toEqual([]);
+    });
+
+    it('should throw for empty range segments', () => {
+      expect(() => parseRanges('A1,,B2')).toThrow();
+    });
+
     it('should parse comma-separated ranges', () => {
       const result = parseRanges('A1:B2,C3:D4');
       expect(result).toEqual([r(1, 1, 2, 2), r(3, 3, 4, 4)]);
     });

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/src/model/core/coordinates.test.ts` around lines 120 - 152,
Add tests covering parseRanges edge cases: assert parseRanges('') returns []
(empty array) and parseRanges('A1,,B2') treats empty/malformed segments
gracefully (e.g., ignores empty segment and returns ranges for A1 and B2) and
that round-tripping via toSrngFromRanges on the parsed result yields the
expected normalized string ('' -> '' and 'A1,,B2' -> 'A1,B2'). Update the
existing describe blocks that exercise parseRanges and toSrngFromRanges to
include these assertions so regressions for empty/invalid segments are caught.

packages/sheet/src/model/core/coordinates.ts (1)

444-462: Consider edge case: non-contiguous ranges with same coordinates but different order may not merge correctly.

The mergeOverlapping function sorts ranges by top-left corner (a[0].r, then a[0].c) and performs a single-pass merge. However, for rectangles that overlap but don't share the same starting row, a single pass may miss merges. Consider whether multi-pass or union-find would be more robust for complex overlapping scenarios.

For example, three ranges where A overlaps B, and B overlaps C, but after sorting A comes before C — if A and B merge first, the result may not properly merge with C if C was processed before the merged result.

That said, for typical multi-selection use cases (disjoint Ctrl+click regions), this implementation should work correctly.

🤖 Prompt for AI Agents

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

In `@packages/sheet/src/model/core/coordinates.ts` around lines 444 - 462,
mergeOverlapping can miss transitive overlaps because sorting by top-left and
one-pass merging (using isIntersect/isAdjacent + mergeRanges) doesn't guarantee
that after merging two ranges the result will be compared with earlier/skipped
ranges; change mergeOverlapping to compute transitive unions: either (A) run the
merge pass repeatedly until no changes (keep using sorted, cloneRange,
isIntersect/isAdjacent, mergeRanges) or (B) build connectivity by checking all
pairs and unioning overlapping/adjacent ranges (union-find), then for each
connected component merge its member ranges with mergeRanges; use the existing
helpers (isIntersect, isAdjacent, mergeRanges, cloneRange) to form group merges
so all transitive merges are captured.

packages/sheet/src/view/overlay.ts (1)

454-504: Extract the per-range painter before these paths drift.

The row/column/cell drawing branches are now duplicated in both the simple and frozen renderers. A small helper that paints one range for a given scroll context would keep future selection-style changes from diverging between the two paths.

As per coding guidelines, "Keep functions small and focused on a single responsibility".

Also applies to: 644-707

🤖 Prompt for AI Agents

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

In `@packages/sheet/src/view/overlay.ts` around lines 454 - 504, The selection
rendering logic inside renderSelectionsSimple duplicates per-range painting for
'row', 'column', and cell cases and should be extracted into a small helper
(e.g., paintRangeForScroll or paintSelectionRange) that takes (ctx, range,
scroll, selectionType, rowDim, colDim, port) and uses the existing helpers
toBoundingRect and expandBoundingRect and theme colors from getThemeColor; move
the fillStyle/strokeStyle/lineWidth + fillRect/strokeRect calls for the three
branches into that helper and replace each per-range block in
renderSelectionsSimple (and the similar code in the frozen renderer around lines
644-707) with a single call to the new helper to avoid duplication and keep
functions focused.