Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions playwright/specs/sheet_excel_chrome.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,48 @@ test.describe('Sheet Excel chrome', () => {
await expect(cell(page, 1, 0)).toHaveText('b');
});

test('find selects the matching cell, replace all rewrites every match', async ({ page }) => {
const padId = `xl-find-${Date.now()}`;
await openSheet(page, padId);
await commitCell(page, 0, 0, 'alpha'); // A1
await commitCell(page, 1, 0, 'beta'); // A2
await commitCell(page, 2, 0, 'alphabet'); // A3

await cell(page, 0, 0).click(); // start on the first match
await page.keyboard.press('Control+f');
const dialog = page.locator('.sheet-find');
await expect(dialog).toBeVisible();

await dialog.locator('input[type=text]').first().fill('alpha');
// Find Next searches forward from the current cell, so A1 is skipped and
// 'alphabet' in A3 comes next.
await dialog.locator('button', { hasText: 'Find Next' }).click();
await expect(cell(page, 2, 0)).toHaveClass(/sheet-sel-focus/);
await expect(dialog.locator('.sheet-find-status')).toContainText('2 cells found');

// Past the last match it wraps around to A1.
await dialog.locator('button', { hasText: 'Find Next' }).click();
await expect(cell(page, 0, 0)).toHaveClass(/sheet-sel-focus/);

await page.keyboard.press('Escape');
await expect(dialog).toBeHidden();

await cell(page, 0, 1).click(); // focus back on the grid before the shortcut
await page.keyboard.press('Control+h');
await dialog.locator('input[type=text]').first().fill('alpha');
await dialog.locator('input[type=text]').nth(1).fill('gamma');
await dialog.locator('button', { hasText: 'Replace All' }).click();
await expect(cell(page, 0, 0)).toHaveText('gamma');
await expect(cell(page, 2, 0)).toHaveText('gammabet');
await expect(cell(page, 1, 0)).toHaveText('beta'); // untouched

// The sweep is one undo step.
await page.keyboard.press('Escape');
await page.locator('.sheet-toolbar button[title="Undo (Ctrl+Z)"]').click();
await expect(cell(page, 0, 0)).toHaveText('alpha');
await expect(cell(page, 2, 0)).toHaveText('alphabet');
});

test('selected cell highlights its row and column headers', async ({ page }) => {
const padId = `xl-headhl-${Date.now()}`;
await openSheet(page, padId);
Expand Down
91 changes: 91 additions & 0 deletions ui/src/js/sheet/findReplace.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { describe, it, expect } from 'vitest';
import { findAll, findNext, matches, replaceAll, replaceInRaw, type RawCell } from './findReplace';

const CELLS: RawCell[] = [
{ row: 0, col: 0, raw: 'Apple' },
{ row: 0, col: 1, raw: 'pineapple' },
{ row: 1, col: 0, raw: 'Banana' },
{ row: 2, col: 3, raw: '=SUM(A1:A2)' },
{ row: 3, col: 0, raw: 'apple pie apple' },
];

describe('matches', () => {
it('is case-insensitive by default and substring-based', () => {
expect(matches('Apple', 'apple')).toBe(true);
expect(matches('pineapple', 'APPLE')).toBe(true);
expect(matches('Apple', 'apple', { matchCase: true })).toBe(false);
});

it('honours whole-cell matching', () => {
expect(matches('Apple', 'apple', { wholeCell: true })).toBe(true);
expect(matches('pineapple', 'apple', { wholeCell: true })).toBe(false);
});

it('never matches an empty query', () => {
expect(matches('anything', '')).toBe(false);
});
});

describe('findAll / findNext', () => {
it('returns matches in row-major order', () => {
expect(findAll(CELLS, 'apple')).toEqual([
{ row: 0, col: 0 },
{ row: 0, col: 1 },
{ row: 3, col: 0 },
]);
});

it('searches formula text, like Excel looking in formulas', () => {
expect(findAll(CELLS, 'sum')).toEqual([{ row: 2, col: 3 }]);
});

it('steps forward from the current cell and wraps around', () => {
expect(findNext(CELLS, 'apple', { row: 0, col: 0 })).toEqual({ row: 0, col: 1 });
expect(findNext(CELLS, 'apple', { row: 0, col: 1 })).toEqual({ row: 3, col: 0 });
expect(findNext(CELLS, 'apple', { row: 3, col: 0 })).toEqual({ row: 0, col: 0 }); // wrap
expect(findNext(CELLS, 'nothing', { row: 0, col: 0 })).toBeNull();
});

it('finds the only match even when standing on it', () => {
expect(findNext(CELLS, 'banana', { row: 1, col: 0 })).toEqual({ row: 1, col: 0 });
});
});

describe('replaceInRaw', () => {
it('replaces every occurrence and keeps the surrounding casing', () => {
expect(replaceInRaw('apple pie apple', 'apple', 'pear')).toBe('pear pie pear');
expect(replaceInRaw('Apple and APPLE', 'apple', 'pear')).toBe('pear and pear');
expect(replaceInRaw('Apple and APPLE', 'apple', 'pear', { matchCase: true })).toBe('Apple and APPLE');
});

it('swaps the whole content in whole-cell mode', () => {
expect(replaceInRaw('Apple', 'apple', 'Pear', { wholeCell: true })).toBe('Pear');
expect(replaceInRaw('pineapple', 'apple', 'Pear', { wholeCell: true })).toBe('pineapple');
});

it('leaves non-matching content untouched', () => {
expect(replaceInRaw('Banana', 'apple', 'pear')).toBe('Banana');
});

it('does not loop forever when the replacement contains the query', () => {
expect(replaceInRaw('a a', 'a', 'aa')).toBe('aa aa');
});
});

describe('replaceAll', () => {
it('returns only the cells that actually change', () => {
expect(replaceAll(CELLS, 'apple', 'pear')).toEqual([
{ row: 0, col: 0, raw: 'pear' },
{ row: 0, col: 1, raw: 'pinepear' },
{ row: 3, col: 0, raw: 'pear pie pear' },
]);
});

it('rewrites formulas too', () => {
expect(replaceAll(CELLS, 'A1:A2', 'B1:B2')).toEqual([{ row: 2, col: 3, raw: '=SUM(B1:B2)' }]);
});

it('returns nothing when the query matches nothing', () => {
expect(replaceAll(CELLS, 'kiwi', 'x')).toEqual([]);
});
});
75 changes: 75 additions & 0 deletions ui/src/js/sheet/findReplace.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Pure find/replace over cell contents. Like Excel's default "Look in:
// Formulas", the search runs against the raw cell content, so a formula is
// found by its text and a replacement rewrites the formula itself.

export interface FindOptions {
matchCase?: boolean;
// Excel's "Match entire cell contents": the query must be the whole content
// instead of appearing somewhere inside it.
wholeCell?: boolean;
}

export interface CellRef {
row: number;
col: number;
}

export interface RawCell extends CellRef {
raw: string;
}

const fold = (s: string, matchCase: boolean | undefined): string => (matchCase ? s : s.toLowerCase());

export function matches(raw: string, query: string, opts: FindOptions = {}): boolean {
if (query === '') return false;
const hay = fold(raw, opts.matchCase);
const needle = fold(query, opts.matchCase);
return opts.wholeCell ? hay === needle : hay.includes(needle);
}

// findAll returns every matching cell in row-major order — the order Excel
// searches in, and the order Find Next steps through.
export function findAll(cells: RawCell[], query: string, opts: FindOptions = {}): CellRef[] {
return cells
.filter((c) => matches(c.raw, query, opts))
.sort((a, b) => a.row - b.row || a.col - b.col)
.map(({ row, col }) => ({ row, col }));
}

// findNext returns the first match strictly after `from` in row-major order,
// wrapping around to the beginning. null when nothing matches at all.
export function findNext(cells: RawCell[], query: string, from: CellRef, opts: FindOptions = {}): CellRef | null {
const hits = findAll(cells, query, opts);
if (hits.length === 0) return null;
return hits.find((h) => h.row > from.row || (h.row === from.row && h.col > from.col)) ?? hits[0];
}

// replaceInRaw rewrites every occurrence in one cell. In whole-cell mode the
// content is swapped outright, matching Excel.
export function replaceInRaw(raw: string, query: string, replacement: string, opts: FindOptions = {}): string {
if (!matches(raw, query, opts)) return raw;
if (opts.wholeCell) return replacement;
if (opts.matchCase) return raw.split(query).join(replacement);
// Case-insensitive: walk the folded string so the untouched parts keep their
// original casing (a plain regex would need the query escaped anyway).
const hay = raw.toLowerCase();
const needle = query.toLowerCase();
let out = '';
let i = 0;
for (let at = hay.indexOf(needle); at !== -1; at = hay.indexOf(needle, i)) {
out += raw.slice(i, at) + replacement;
i = at + needle.length;
Comment on lines +55 to +61

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Unicode replacement corrupts text 🐞 Bug ≡ Correctness

Case-insensitive replacement uses offsets from raw.toLowerCase() to slice the original string, but
Unicode lowercasing can change UTF-16 length. Replacing x in İX, for example, produces İXz
instead of İz, silently corrupting cell contents or formulas.
Agent Prompt
## Issue description
Case-insensitive replacement derives match offsets from a lowercased string and applies them to the original string. Unicode case conversion can change string length, causing incorrect slicing and corrupted cell contents.

## Issue Context
For example, JavaScript lowercases `İX` to `i\u0307x`; the folded `x` offset is therefore not its offset in the original string. Add regression coverage for this and multiple matches after length-changing folds.

## Fix Focus Areas
- ui/src/js/sheet/findReplace.ts[21-63]
- ui/src/js/sheet/findReplace.test.ts[54-73]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}
return out + raw.slice(i);
}

// replaceAll produces the cells whose content actually changes, so the caller
// can emit one op per changed cell and nothing for the rest.
export function replaceAll(cells: RawCell[], query: string, replacement: string, opts: FindOptions = {}): RawCell[] {
const out: RawCell[] = [];
for (const c of cells) {
const next = replaceInRaw(c.raw, query, replacement, opts);
if (next !== c.raw) out.push({ row: c.row, col: c.col, raw: next });
}
return out.sort((a, b) => a.row - b.row || a.col - b.col);
}
50 changes: 49 additions & 1 deletion ui/src/js/sheet/sheetEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import { FormulaEngine } from './formulaEngine';
import { DomSheetView } from './sheetView';
import { SheetPresence, effectiveCells, type PresenceFrame } from './sheetPresence';
import { rangeToTSV, rangeToCSV, parseTSV, parseCSV, pasteOps, fillOps } from './sheetClipboard';
import { normalize, selCells, selIsSingle, type Selection } from './sheetSelection';
import { normalize, selCells, selFromSingle, selIsSingle, type Selection } from './sheetSelection';
import { createToolbar, type ToolbarCallbacks, type ToolbarElement } from './sheetToolbar';
import { createSheetTabs } from './sheetTabs';
import { sortRangeOps, distinctValues, hiddenRowsForFilter } from './sheetSortFilter';
import { createFormulaBar, type FormulaBarHandle } from './sheetFormulaBar';
import { createFindDialog } from './sheetFindDialog';
import { findAll, findNext, matches, replaceAll, replaceInRaw } from './findReplace';
import { rangeRefA1 } from './a1';
import { mergeProps } from './styleCss';
import { formatValue } from './format';
Expand Down Expand Up @@ -394,6 +396,7 @@ export function startSheetEditor(root: HTMLElement): void {
undo: () => doHistory('undo'),
redo: () => doHistory('redo'),
history: () => ({ canUndo: collab?.canUndo() ?? false, canRedo: collab?.canRedo() ?? false }),
openFind: (mode: 'find' | 'replace') => findDialog.open(readOnly ? 'find' : mode),
clear: (what: 'all' | 'formats' | 'contents') => {
if (readOnly || !collab) return;
blurActiveCell();
Expand Down Expand Up @@ -504,6 +507,7 @@ export function startSheetEditor(root: HTMLElement): void {
root.appendChild(toolbar);
root.appendChild(formulaBar.el);
root.appendChild(gridHost);
root.appendChild(findDialog.el);

const setActiveSheet = (id: string): void => {
if (id === activeSheetId) return;
Expand Down Expand Up @@ -650,6 +654,44 @@ export function startSheetEditor(root: HTMLElement): void {
for (const op of pasteOps(grid, { row: r0, col: c0 }, activeSheetId, collab.rev)) collab.applyLocal(op);
});
};
// --- Find & Replace ---------------------------------------------------
// Searches the raw cell content (Excel's default "Look in: Formulas"), so a
// formula is found by its text and a replacement rewrites the formula.
const findDialog = createFindDialog({
readOnly: false, // re-checked per action: `readOnly` is only known after the handshake
findNext: (query, opts) => {
const cells = cellsOfActive();
const hit = findNext(cells, query, selection.focus, opts);
if (hit) view?.setSelection(selFromSingle(hit.row, hit.col));
Comment on lines +663 to +665

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Hidden merged matches selected 🐞 Bug ≡ Correctness

Find searches covered cells whose contents are retained but whose DOM elements are hidden by a
merge, then selects the hidden coordinate and reports success without a visible focus outline.
Replace and Replace All can consequently mutate hidden covered-cell content that users cannot
inspect until unmerging.
Agent Prompt
## Issue description
Find and replace treat covered coordinates inside merged ranges as independently visible cells. A covered match is selected even though its DOM cell is hidden, and replacement can silently modify retained hidden content.

## Issue Context
Merged-cell application intentionally retains covered cell contents, while the view sets covered table cells to `display: none`. Define merged ranges as one searchable logical cell, normally by excluding non-anchor covered coordinates from Find, Replace, and Replace All.

## Fix Focus Areas
- ui/src/js/sheet/sheetEditor.ts[657-692]
- ui/src/js/sheet/findReplace.test.ts[29-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

return { found: hit !== null, total: findAll(cells, query, opts).length };
},
replace: (query, replacement, opts) => {
const here = selection.focus;
const raw = rawValue(here.row, here.col);
let replaced = false;
if (!readOnly && collab && matches(raw, query, opts)) {
collab.applyLocal({
type: 'setCell', sheet: activeSheetId, baseRev: collab.rev,
row: here.row, col: here.col, raw: replaceInRaw(raw, query, replacement, opts),
});
replaced = true;
}
const hit = findNext(cellsOfActive(), query, here, opts);
if (hit) view?.setSelection(selFromSingle(hit.row, hit.col));
return { replaced, total: findAll(cellsOfActive(), query, opts).length };
},
replaceAll: (query, replacement, opts) => {
if (readOnly || !collab) return 0;
blurActiveCell();
const changed = replaceAll(cellsOfActive(), query, replacement, opts);
// One tick, so the whole sweep is a single undo step.
for (const c of changed) {
collab.applyLocal({ type: 'setCell', sheet: activeSheetId, baseRev: collab.rev, row: c.row, col: c.col, raw: c.raw });
}
return changed.length;
},
});

// Undo/redo this client's own edits. Blur first so a half-typed cell does not
// get committed over the restored value by the blur handler.
const doHistory = (which: 'undo' | 'redo'): void => {
Expand Down Expand Up @@ -686,6 +728,12 @@ export function startSheetEditor(root: HTMLElement): void {
doPaste();
return;
}
// Ctrl+F / Ctrl+H open the dialog (Ctrl+H only when it can replace).
if (mod && !editingNow() && (e.key === 'f' || e.key === 'F' || e.key === 'h' || e.key === 'H')) {
e.preventDefault();
findDialog.open(e.key.toLowerCase() === 'h' && !readOnly ? 'replace' : 'find');
return;
}
// Ctrl+Z / Ctrl+Y (and Ctrl+Shift+Z) — only outside cell editing, where the
// browser's own text undo still owns the keystroke.
if (mod && !editingNow() && !readOnly) {
Expand Down
Loading
Loading