fix(sheet): Tippen überschreibt den Zellinhalt by SamTV12345 · Pull Request #379 · ether/etherpad-go

SamTV12345 · 2026-07-28T19:49:59Z

Problem

Eine Zelle auswählen und lostippen hat den Text angehängt statt ersetzt. Ursache: die Grid-Zelle ist ein dauerhaft contenteditable td, der Tastendruck ging schlicht an den Cursor. Aufgefallen ist es, weil ein Undo-e2e-Test in #377 „firstsecond" statt „second" gelesen hat — der Test war falsch geschrieben, aber das Verhalten dahinter ist der eigentliche Fehler.

Excel überschreibt: der erste Tastendruck auf einer ausgewählten Zelle ersetzt den Inhalt.

Fix

Der erste druckbare Tastendruck auf einer Zelle, die ausgewählt aber nicht in Bearbeitung ist, leert sie und lässt das Zeichen in die leere Zelle fallen (kein preventDefault, der Browser fügt selbst ein). Danach greift activeEdit und weitere Zeichen hängen normal an.

Die Auswege entsprechen Excel:

F2 — Bearbeiten mit Cursor am Ende
Doppelklick — Bearbeiten an der Klickposition
IME-Komposition nimmt denselben Überschreib-Pfad, weil sie ohne druckbares keydown startet

Test

Playwright: tippen ersetzt, F2 hängt an, Doppelklick bearbeitet an Ort und Stelle. Unit-Tests gibt es dafür keine — es ist reines Browser-Verhalten in der contenteditable-Zelle, und für die View existiert keine DOM-Test-Infrastruktur.

Basis

Dritter Teil der Kette #377 (Undo/Redo) → #378 (Suchen & Ersetzen) → dieser PR. Basis wird nach dem Mergen der Vorgänger auf main gezogen.

🤖 Generated with Claude Code

qodo-free-for-open-source-projects · 2026-07-28T19:50:07Z

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis

qodo-free-for-open-source-projects · 2026-07-28T19:50:41Z

PR Summary by Qodo

Make typing overwrite selected spreadsheet cells

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Replaces selected cell content when users begin typing.
• Preserves existing content for F2, double-click, and subsequent keystrokes.
• Adds browser coverage for overwrite and edit-in-place workflows.

Diagram

graph TD
  A["Cell Input"] --> B{"Editing Active?"}
  B -- "Typing or IME: no" --> C["Clear Content"] --> D["Browser Inserts"] --> E["Commit Edit"]
  B -- "F2: no" --> F["Caret at End"] --> D
  B -- "Double-click: no" --> G["Edit in Place"] --> D
  B -- "yes" --> D

High-Level Assessment

The current approach is appropriate because it preserves the existing contenteditable cell architecture and relies on native browser insertion after adjusting content and caret state. Replacing cells with a temporary input or toggling contenteditable only during edits would require broader focus, selection, formula, and collaboration changes without improving this targeted fix.

Files changed (2) +68 / -0

Bug fix (1) +42 / -0

sheetView.tsImplement Excel-style overwrite-on-typing behavior +42/-0

Implement Excel-style overwrite-on-typing behavior

• Adds caret positioning and overwrite helpers to the DOM sheet view. Printable input and IME composition clear a selected, inactive cell, while F2 and double-click explicitly enter edit mode without clearing existing content.

ui/src/js/sheet/sheetView.ts

Tests (1) +26 / -0

sheet_excel_chrome.spec.tsCover overwrite and edit-in-place cell behavior +26/-0

Cover overwrite and edit-in-place cell behavior
• Adds a Playwright regression test verifying that typing replaces selected cell content. It also confirms that F2 appends from the end and double-click preserves content for in-place editing.
playwright/specs/sheet_excel_chrome.spec.ts

qodo-code-review · 2026-07-28T19:53:01Z

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📜 Skill insights (0)

1. AltGr typing still appends 🐞 Bug ≡ Correctness

Description

The overwrite condition rejects printable keydowns carrying Ctrl or Alt, so layouts and browsers
exposing AltGr as Ctrl+Alt skip beginOverwrite() and insert into the existing content. Common
international-layout characters therefore append instead of replacing the selected cell.

Code

ui/src/js/sheet/sheetView.ts[R364-365]

+      if (!this.activeEdit && e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey) {
+        this.beginOverwrite(td);

Relevance

⭐⭐⭐ High
Common international typing path directly violates the PR’s overwrite behavior.
PR-#320

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

A focused cell is populated with its stored raw value and remains inactive until the later input
event. The new condition excludes Ctrl/Alt printable keydowns, so AltGr events exposed as Ctrl+Alt
reach the populated contenteditable without invoking the overwrite clear.

ui/src/js/sheet/sheetView.ts[293-305]
ui/src/js/sheet/sheetView.ts[362-366]

Agent prompt

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

## Issue description
Printable AltGr-generated characters can carry both Ctrl and Alt modifier flags. The current overwrite predicate treats them as shortcuts, so they edit the existing content without first clearing it.

## Issue Context
Distinguish AltGr text input from actual Ctrl/Alt shortcuts, and add coverage for an AltGr-equivalent printable key event.

## Fix Focus Areas
- ui/src/js/sheet/sheetView.ts[362-366]
- playwright/specs/sheet_excel_chrome.spec.ts[162-186]

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

2. Double-click replaces selected word 🐞 Bug ≡ Correctness

Description

Native double-clicking in a contenteditable selects a word, but the new handler only sets
activeEdit and never collapses that selection to the clicked caret. The next character can replace
the selected word rather than edit at the click position, violating the new preservation test's
contract.

Code

ui/src/js/sheet/sheetView.ts[R308-309]

+    td.addEventListener('dblclick', () => {
+      this.activeEdit = true;

Relevance

⭐⭐⭐ High
Directly contradicts the PR’s double-click preservation intent and added test contract.
PR-#318

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The handler contains no caret or selection manipulation despite claiming to keep the caret at the
click position. The added Playwright test explicitly requires the original newer text to survive
typing after the double-click.

ui/src/js/sheet/sheetView.ts[306-310]
playwright/specs/sheet_excel_chrome.spec.ts[180-185]

Agent prompt

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

## Issue description
The double-click handler enables edit mode without overriding the contenteditable's native word selection. Subsequent typing can replace the selected word instead of inserting at the clicked position.

## Issue Context
Prevent or collapse the second-click selection while retaining the caret derived from the click coordinates. Strengthen the browser test to assert the exact resulting value and, if practical, insertion at a non-terminal position.

## Fix Focus Areas
- ui/src/js/sheet/sheetView.ts[306-310]
- playwright/specs/sheet_excel_chrome.spec.ts[180-185]

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

3. Canceled composition deletes value 🐞 Bug ≡ Correctness

Description

When an IME composition ends before producing an input value, compositionstart has already emptied
the cell. Blurring afterward compares that empty DOM value with the stored value and commits an
unintended deletion.

Code

ui/src/js/sheet/sheetView.ts[R313-314]

+    td.addEventListener('compositionstart', () => {
+      if (!this.activeEdit) this.beginOverwrite(td);

Relevance

⭐⭐ Medium
Plausible IME data-loss edge case, but cancellation semantics and remedy are browser-dependent.
PR-#318

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The composition listener synchronously clears td.textContent, while the blur listener commits
whenever the resulting DOM text differs from the model; no composition cancellation path restores
the original content.

ui/src/js/sheet/sheetView.ts[311-315]
ui/src/js/sheet/sheetView.ts[377-390]

Agent prompt

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

## Issue description
The cell is cleared as soon as IME composition starts, before replacement text is guaranteed. A canceled composition that produces no input can consequently commit the cell as empty on blur.

## Issue Context
Arm overwrite mode on `compositionstart`, clear when composition text actually arrives, and restore or preserve the original value when composition is canceled. Add browser coverage for canceling an IME composition before input.

## Fix Focus Areas
- ui/src/js/sheet/sheetView.ts[311-315]
- ui/src/js/sheet/sheetView.ts[377-390]

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

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

qodo-code-review · 2026-07-28T19:53:03Z

+      if (!this.activeEdit && e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey) {
+        this.beginOverwrite(td);


1. Altgr typing still appends 🐞 Bug ≡ Correctness

The overwrite condition rejects printable keydowns carrying Ctrl or Alt, so layouts and browsers exposing AltGr as Ctrl+Alt skip beginOverwrite() and insert into the existing content. Common international-layout characters therefore append instead of replacing the selected cell.

Agent Prompt

## Issue description Printable AltGr-generated characters can carry both Ctrl and Alt modifier flags. The current overwrite predicate treats them as shortcuts, so they edit the existing content without first clearing it. ## Issue Context Distinguish AltGr text input from actual Ctrl/Alt shortcuts, and add coverage for an AltGr-equivalent printable key event. ## Fix Focus Areas - ui/src/js/sheet/sheetView.ts[362-366] - playwright/specs/sheet_excel_chrome.spec.ts[162-186]

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

qodo-code-review · 2026-07-28T19:53:03Z

+    td.addEventListener('compositionstart', () => {
+      if (!this.activeEdit) this.beginOverwrite(td);


2. Canceled composition deletes value 🐞 Bug ≡ Correctness

When an IME composition ends before producing an input value, compositionstart has already emptied the cell. Blurring afterward compares that empty DOM value with the stored value and commits an unintended deletion.

Agent Prompt

## Issue description The cell is cleared as soon as IME composition starts, before replacement text is guaranteed. A canceled composition that produces no input can consequently commit the cell as empty on blur. ## Issue Context Arm overwrite mode on `compositionstart`, clear when composition text actually arrives, and restore or preserve the original value when composition is canceled. Add browser coverage for canceling an IME composition before input. ## Fix Focus Areas - ui/src/js/sheet/sheetView.ts[311-315] - ui/src/js/sheet/sheetView.ts[377-390]

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

qodo-code-review · 2026-07-28T19:53:03Z

+    td.addEventListener('dblclick', () => {
+      this.activeEdit = true;


3. Double-click replaces selected word 🐞 Bug ≡ Correctness

Native double-clicking in a contenteditable selects a word, but the new handler only sets activeEdit and never collapses that selection to the clicked caret. The next character can replace the selected word rather than edit at the click position, violating the new preservation test's contract.

Agent Prompt

## Issue description The double-click handler enables edit mode without overriding the contenteditable's native word selection. Subsequent typing can replace the selected word instead of inserting at the clicked position. ## Issue Context Prevent or collapse the second-click selection while retaining the caret derived from the click coordinates. Strengthen the browser test to assert the exact resulting value and, if practical, insertion at a non-terminal position. ## Fix Focus Areas - ui/src/js/sheet/sheetView.ts[306-310] - playwright/specs/sheet_excel_chrome.spec.ts[180-185]

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

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>

Selecting a cell and typing appended to what was already there, because the grid cell is a permanently contenteditable td and the keystroke just went to the caret. Excel overwrites instead, which is what everyone expects from a spreadsheet - and it is what made a two-value edit in the undo e2e test read "firstsecond". The first printable keystroke on a cell that is selected but not being edited now clears it and lets the character land in the empty cell. The escape hatches match Excel: F2 enters edit mode with the caret at the end, double-click edits in place at the click position. IME composition takes the same overwrite path, since it starts without a printable keydown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

qodo-code-review Bot reviewed Jul 28, 2026

View reviewed changes

SamTV12345 force-pushed the feat/sheet-find-replace branch from 704d10a to 5efc964 Compare July 28, 2026 19:56

SamTV12345 force-pushed the feat/sheet-type-overwrite branch from 498d3df to 1253b19 Compare July 28, 2026 19:57

SamTV12345 force-pushed the feat/sheet-find-replace branch from 5efc964 to 1b1198c Compare July 28, 2026 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(sheet): Tippen überschreibt den Zellinhalt - #379

fix(sheet): Tippen überschreibt den Zellinhalt#379
SamTV12345 wants to merge 2 commits into
feat/sheet-find-replacefrom
feat/sheet-type-overwrite

SamTV12345 commented Jul 28, 2026

Uh oh!

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

edited

Loading

Uh oh!

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

edited by qodo-code-review Bot

Loading

Uh oh!

qodo-code-review Bot commented Jul 28, 2026

Uh oh!

qodo-code-review Bot Jul 28, 2026

Uh oh!

qodo-code-review Bot Jul 28, 2026

Uh oh!

qodo-code-review Bot Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		if (!this.activeEdit && e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey) {
		this.beginOverwrite(td);

		td.addEventListener('compositionstart', () => {
		if (!this.activeEdit) this.beginOverwrite(td);

		td.addEventListener('dblclick', () => {
		this.activeEdit = true;

Uh oh!

Conversation

SamTV12345 commented Jul 28, 2026

Problem

Fix

Test

Basis

Uh oh!

qodo-free-for-open-source-projects Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

New Review Started

Uh oh!

qodo-free-for-open-source-projects Bot commented Jul 28, 2026 • edited by qodo-code-review Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Jul 28, 2026

Code Review by Qodo

Uh oh!

qodo-code-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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

edited

Loading

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

edited by qodo-code-review Bot

Loading