Skip to content

feat(sheet): Suchen & Ersetzen - #378

Open
SamTV12345 wants to merge 1 commit into
mainfrom
feat/sheet-find-replace
Open

feat(sheet): Suchen & Ersetzen#378
SamTV12345 wants to merge 1 commit into
mainfrom
feat/sheet-find-replace

Conversation

@SamTV12345

Copy link
Copy Markdown
Member

Was

Ctrl+F / Ctrl+H öffnen einen Excel-artigen Dialog mit Find Next, Replace, Replace All sowie Match case und Entire cell. Im Ribbon gibt es dazu „Find & Select" in der Editing-Gruppe.

Gesucht wird im Rohinhalt der Zelle — das ist Excels Standard „Look in: Formulas": eine Formel wird über ihren Text gefunden, und ein Ersetzen schreibt die Formel um (=SUM(A1:A2)=SUM(B1:B2)). Find Next läuft zeilenweise ab der aktuellen Zelle und springt am Ende wieder an den Anfang.

Replace All schreibt eine setCell-Op pro geänderter Zelle innerhalb eines Ticks — damit ist der ganze Durchlauf dank #377 genau ein Undo-Schritt. Ein Playwright-Test hält das fest.

Beim Ersetzen ohne Groß-/Kleinschreibungs-Beachtung läuft die Ersetzung über den gefalteten String statt über eine Regex: die unberührten Teile behalten ihre Schreibweise, und die Suchanfrage muss nirgends escaped werden.

Die View bekommt setSelection(), damit ein Treffer wie ein Klick ausgewählt und in den sichtbaren Bereich gescrollt wird — inklusive der üblichen Benachrichtigungen an Editor und Formelleiste.

Tests

14 Unit-Tests für die reine Logik (Groß-/Kleinschreibung, ganze Zelle, Zeilenreihenfolge, Wrap-Around, mehrfaches Vorkommen pro Zelle, Ersetzung die die Suchanfrage enthält, Formeltext) plus ein Playwright-Test über Tastatur- und Dialogpfad inklusive Undo des Replace-All. 180 Vitest-Tests grün.

Nicht enthalten

Platzhalter (*, ?) in der Suche, Suche über alle Blätter, und „Find All" als Trefferliste — der Dialog nennt stattdessen die Trefferzahl.

Hinweis zur Basis

Basiert auf #377 (Undo/Redo), weil der Replace-All-Test dessen Undo braucht. Sobald #377 gemergt ist, stelle ich die Basis auf main um.

🤖 Generated with Claude Code

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 28, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Unicode replacement corrupts text 🐞 Bug ≡ Correctness
Description
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.
Code

ui/src/js/sheet/findReplace.ts[R55-61]

+  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;
Relevance

⭐⭐⭐ High

Deterministic Unicode corruption is a local correctness bug; similar parsing edge-case fixes were
accepted.

PR-#320

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The implementation computes at and i against the lowercased value but passes those positions to
raw.slice(). Because lowercase conversion can expand characters such as U+0130, those positions
are not guaranteed to address the same characters in raw.

ui/src/js/sheet/findReplace.ts[49-63]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


2. Hidden merged matches selected 🐞 Bug ≡ Correctness
Description
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.
Code

ui/src/js/sheet/sheetEditor.ts[R663-665]

+      const cells = cellsOfActive();
+      const hit = findNext(cells, query, selection.focus, opts);
+      if (hit) view?.setSelection(selFromSingle(hit.row, hit.col));
Relevance

⭐⭐ Medium

Plausible merged-cell UX bug, but no close acceptance or rejection precedent exists.

PR-#352

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
cellsOfActive() returns every stored cell, and the new callbacks search that unfiltered list.
Merge application retains covered contents, while rendering hides every covered non-anchor cell and
applies the focus class only to the exact selected coordinate.

ui/src/js/sheet/sheetEditor.ts[124-132]
lib/sheet/apply.go[100-111]
ui/src/js/sheet/sheetView.ts[423-435]
ui/src/js/sheet/sheetView.ts[503-511]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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



Informational

3. Off-grid matches remain invisible ✓ Resolved 🐞 Bug ≡ Correctness
Description
Find searches every stored workbook coordinate, but DomSheetView only renders rows 0–199 and
columns 0–51, so an out-of-range hit updates selection state without any visible cell or scroll
target. Such coordinates are valid and can be created by inserting a row before row 199 or a column
before column 51 because structural operations have no matching upper bound.
Code

ui/src/js/sheet/sheetEditor.ts[R664-665]

+      const hit = findNext(cells, query, selection.focus, opts);
+      if (hit) view?.setSelection(selFromSingle(hit.row, hit.col));
Relevance

⭐ Low

The team explicitly rejected preventing structural operations from creating cells beyond the fixed
rendered grid.

PR-#328

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The editor renders a fixed 200-by-52 grid while cellsOfActive() returns all stored coordinates.
setSelection() tolerates a missing table cell through optional chaining, and server validation
permits structural operations to shift cells beyond those rendered limits.

ui/src/js/sheet/sheetEditor.ts[73-74]
ui/src/js/sheet/sheetEditor.ts[124-132]
ui/src/js/sheet/sheetView.ts[370-375]
lib/sheet/op.go[115-121]
lib/sheet/apply.go[118-123]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Find can return stored cells outside the fixed 200-by-52 rendered grid. Selecting such a result reports success but cannot show or scroll to the matching cell.
## Issue Context
Coordinates are zero-based, so row 200 and column 52 are already outside the view. Either expand/virtualize the view to include valid stored coordinates or consistently constrain data and searches to supported bounds; add coverage for structurally shifted boundary cells.
## Fix Focus Areas
- ui/src/js/sheet/sheetEditor.ts[73-74]
- ui/src/js/sheet/sheetEditor.ts[657-692]
- ui/src/js/sheet/sheetView.ts[367-375]
- playwright/specs/sheet_excel_chrome.spec.ts[204-239]

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


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 28, 2026

Copy link
Copy Markdown

PR Summary by Qodo

Add Excel-style find and replace to sheets

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Adds Find Next, Replace, and Replace All for raw cell contents.
• Supports case-sensitive, entire-cell, row-major, and wrap-around searches.
• Exposes ribbon and keyboard entry points with single-step undo coverage.
Diagram

sequenceDiagram
  actor User
  participant Entry as Ribbon / Shortcuts
  participant Dialog as Find Dialog
  participant Editor as Sheet Editor
  participant Logic as Find Logic
  participant View as Grid View
  participant Collab as Collaboration
  User->>Entry: Open find or replace
  Entry->>Dialog: Select mode
  Dialog->>Editor: Submit action
  Editor->>Logic: Search raw cells
  Logic-->>Editor: Return hits or changes
  alt Find Next
    Editor->>View: Select and reveal hit
  else Replace
    Editor->>Collab: Apply setCell operations
  end
Loading
High-Level Assessment

The current separation is appropriate: pure find/replace functions remain independently testable, the dialog owns only UI state, and the editor coordinates selection and collaborative mutations. Inlining the logic into the dialog or using regex-based replacement would increase coupling and escaping complexity without a meaningful benefit.

Files changed (7) +452 / -2

Enhancement (5) +324 / -2
findReplace.tsImplement pure raw-cell find and replace operations +75/-0

Implement pure raw-cell find and replace operations

• Introduces reusable matching, find-all, find-next, per-cell replacement, and replace-all functions. Searches operate on raw values, preserve untouched casing, and return results in row-major order.

ui/src/js/sheet/findReplace.ts

sheetEditor.tsIntegrate find and replace with sheet editing +49/-1

Integrate find and replace with sheet editing

• Wires dialog actions to active-sheet raw cells, programmatic selection, and collaborative setCell operations. Adds Ctrl+F and Ctrl+H shortcuts while restricting replacement in read-only sessions.

ui/src/js/sheet/sheetEditor.ts

sheetFindDialog.tsAdd the Excel-style find and replace dialog +180/-0

Add the Excel-style find and replace dialog

• Creates the dialog UI for Find Next, Replace, Replace All, Match case, and Entire cell. It manages find/replace modes, keyboard behavior, and result status messages through editor callbacks.

ui/src/js/sheet/sheetFindDialog.ts

sheetToolbar.tsExpose Find & Select in the Editing ribbon group +10/-1

Expose Find & Select in the Editing ribbon group

• Adds a search icon, toolbar callback, and Find & Select menu with Find and Replace commands.

ui/src/js/sheet/sheetToolbar.ts

sheetView.tsSupport programmatic selection and match scrolling +10/-0

Support programmatic selection and match scrolling

• Adds setSelection so editor-driven navigation updates selection observers, rerenders the grid, and scrolls the focused cell into view.

ui/src/js/sheet/sheetView.ts

Tests (2) +128 / -0
sheet_excel_chrome.spec.tsCover find, replace-all, and grouped undo end to end +37/-0

Cover find, replace-all, and grouped undo end to end

• Adds a browser test for Ctrl+F and Ctrl+H dialog flows, match selection, replacement results, and status counts. It verifies that Replace All is reverted by one undo action.

playwright/specs/sheet_excel_chrome.spec.ts

findReplace.test.tsTest raw-cell search and replacement semantics +91/-0

Test raw-cell search and replacement semantics

• Adds unit coverage for case handling, entire-cell matching, row-major ordering, wrap-around, repeated replacements, formula text, and no-match behavior.

ui/src/js/sheet/findReplace.test.ts

@qodo-code-review

qodo-code-review Bot commented Jul 28, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Unicode replacement corrupts text 🐞 Bug ≡ Correctness
Description
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.
Code

ui/src/js/sheet/findReplace.ts[R55-61]

+  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;
Relevance

⭐⭐⭐ High

Deterministic Unicode corruption is a local correctness bug; similar parsing edge-case fixes were
accepted.

PR-#320

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The implementation computes at and i against the lowercased value but passes those positions to
raw.slice(). Because lowercase conversion can expand characters such as U+0130, those positions
are not guaranteed to address the same characters in raw.

ui/src/js/sheet/findReplace.ts[49-63]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


2. Hidden merged matches selected 🐞 Bug ≡ Correctness
Description
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.
Code

ui/src/js/sheet/sheetEditor.ts[R663-665]

+      const cells = cellsOfActive();
+      const hit = findNext(cells, query, selection.focus, opts);
+      if (hit) view?.setSelection(selFromSingle(hit.row, hit.col));
Relevance

⭐⭐ Medium

Plausible merged-cell UX bug, but no close acceptance or rejection precedent exists.

PR-#352

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
cellsOfActive() returns every stored cell, and the new callbacks search that unfiltered list.
Merge application retains covered contents, while rendering hides every covered non-anchor cell and
applies the focus class only to the exact selected coordinate.

ui/src/js/sheet/sheetEditor.ts[124-132]
lib/sheet/apply.go[100-111]
ui/src/js/sheet/sheetView.ts[423-435]
ui/src/js/sheet/sheetView.ts[503-511]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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



Informational

3. Off-grid matches remain invisible ✓ Resolved 🐞 Bug ≡ Correctness
Description
Find searches every stored workbook coordinate, but DomSheetView only renders rows 0–199 and
columns 0–51, so an out-of-range hit updates selection state without any visible cell or scroll
target. Such coordinates are valid and can be created by inserting a row before row 199 or a column
before column 51 because structural operations have no matching upper bound.
Code

ui/src/js/sheet/sheetEditor.ts[R664-665]

+      const hit = findNext(cells, query, selection.focus, opts);
+      if (hit) view?.setSelection(selFromSingle(hit.row, hit.col));
Relevance

⭐ Low

The team explicitly rejected preventing structural operations from creating cells beyond the fixed
rendered grid.

PR-#328

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The editor renders a fixed 200-by-52 grid while cellsOfActive() returns all stored coordinates.
setSelection() tolerates a missing table cell through optional chaining, and server validation
permits structural operations to shift cells beyond those rendered limits.

ui/src/js/sheet/sheetEditor.ts[73-74]
ui/src/js/sheet/sheetEditor.ts[124-132]
ui/src/js/sheet/sheetView.ts[370-375]
lib/sheet/op.go[115-121]
lib/sheet/apply.go[118-123]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Find can return stored cells outside the fixed 200-by-52 rendered grid. Selecting such a result reports success but cannot show or scroll to the matching cell.

## Issue Context
Coordinates are zero-based, so row 200 and column 52 are already outside the view. Either expand/virtualize the view to include valid stored coordinates or consistently constrain data and searches to supported bounds; add coverage for structurally shifted boundary cells.

## Fix Focus Areas
- ui/src/js/sheet/sheetEditor.ts[73-74]
- ui/src/js/sheet/sheetEditor.ts[657-692]
- ui/src/js/sheet/sheetView.ts[367-375]
- playwright/specs/sheet_excel_chrome.spec.ts[204-239]

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


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment on lines +55 to +61
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;

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

Comment on lines +663 to +665
const cells = cellsOfActive();
const hit = findNext(cells, query, selection.focus, opts);
if (hit) view?.setSelection(selFromSingle(hit.row, hit.col));

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

@SamTV12345
SamTV12345 force-pushed the feat/sheet-find-replace branch from 704d10a to 5efc964 Compare July 28, 2026 19:56
@SamTV12345
SamTV12345 changed the base branch from feat/sheet-undo-redo to main July 28, 2026 19:56
@SamTV12345
SamTV12345 enabled auto-merge (squash) July 28, 2026 19:56
Ctrl+F / Ctrl+H open an Excel-style dialog with Find Next, Replace and Replace
All, plus Match case and Entire cell; the ribbon gets a Find & Select menu in
the Editing group.

The search runs against the raw cell content, which is Excel default "Look in:
Formulas": a formula is found by its text and a replacement rewrites the
formula. Find Next walks row-major from the current cell and wraps around.
Replace All emits one setCell op per changed cell within a single tick, so the
whole sweep is one undo step.

Case-insensitive replacement is done by walking the folded string instead of a
regex, so untouched parts keep their original casing and no query needs
escaping.

The view gains setSelection() so a hit can be selected and scrolled into view
the same way a click would, notifications included.

Not covered: wildcards in the query, searching across all sheets, and Find All
as a result list - the dialog reports the match count instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@SamTV12345
SamTV12345 force-pushed the feat/sheet-find-replace branch from 5efc964 to 1b1198c Compare July 28, 2026 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant