fix: surgically remove sent content from input when editor changed mid-flight (#458)

[lml2468]

Root cause

runSendWithCleanup() in sendFlow.ts, the !isEditorUnchanged() branch (L143–158). When the editor content changed during the async send await, the code deliberately left the entire live doc untouched to avoid wiping the new draft (round-2 data-loss protection) — but the already-sent snapshot text was never surgically removed. This is the known residual of the #227 round-2 fix.

Fix

Extended SendCleanup with two optional, backward-compatible fields:

• snapshotContentSize: ProseMirror doc.content.size captured at send time
• removeSentContent(): callback that surgically deletes positions 0→snapshotContentSize from the live editor, preserving any content typed after the snapshot

The caller (MessageInput) captures snapshotContentSize before the send and implements removeSentContent via TipTap deleteRange. When the editor changed mid-flight and removeSentContent is provided, the function calls it (plus attachment ref/URL cleanup) instead of leaving the doc untouched.

Back-compat: when removeSentContent is not provided, the legacy "leave untouched" behaviour is preserved.

Contracts preserved

All 13 existing test contracts in __tests__/sendFlow.test.ts remain green; 5 new tests cover the partial-removal path.

Fixes #458

[OctoBoooot]

Review: fix: surgically remove sent content from input when editor changed mid-flight (#458) (#470)

Verdict: Request changes

This re-attempts the same fix as #469 with a different API call (editor.commands.deleteRange({from:0, to:size}) instead of tr.delete(0, size)) — but the two are functionally identical, and the data-loss blocker from #469 is unaddressed. Four reviewers (me, Steve, OctoBoooot, Jerry-Xin) blocked #469 on exactly this; the same 🔴 applies here verbatim.

Blockers

• MessageInput/index.tsx:removeSentContent — deleteRange({ from: 0, to: snapshotContentSize }) treats snapshotContentSize (a ProseMirror content.size, which counts block-boundary tokens) as a deletable document-position prefix over the current doc. That's only correct when the new draft is a brand-new block after the snapshot. The PR's own comment ("correct when the user appended new text") misses that same-paragraph append — the most common chat-input interaction — breaks it. Concrete failing input: snapshot = <p>hello</p> → content.size = 7 (para-open + hello + para-close). User appends world in the same paragraph during the in-flight send → <p>helloworld</p>. deleteRange(0, 7) deletes the para-open token + hello + the first char of the new draft, yielding a corrupted orld — destroying part of the user's new text. Prepend ([X][hello]) and edit-inside are worse; same root cause. This re-opens the round-2 "draft mangled/wiped" class the branch exists to prevent, for every non-new-block mid-flight edit. The cursor is not pinned during the await, so these cases are freely reachable. Fix direction (convergent across #469 reviewers): don't use content.size as a position — diff the live doc against the snapshot JSON to locate the actually-sent node range, or map snapshot positions through the editor transactions.
• Tests don't cover the real deletion. The 5 new sendFlow.test.ts cases mock removeSentContent as vi.fn() and only assert that it's called with snapshotContentSize — the actual deleteRange position math (the entire risk surface) has zero coverage, identical to #469. A regression test must run a real (or faithfully-faked) ProseMirror doc through both [snapshot][new paragraph] (should pass) and <p>hello</p> + same-paragraph append (currently corrupts) and assert the surviving text. Right now the suite is green while the bug is live.

Praise

• The SendCleanup interface extension remains clean and backward-compatible (both fields optional, legacy "leave untouched" path preserved when removeSentContent absent, the 13 existing contracts stay green) — and the cleanup.snapshotContentSize != null guard in sendFlow.ts correctly pairs the two new fields so a half-configured caller can't trigger the path. The orchestration is right; it's the position arithmetic in the caller that needs the non-append cases handled.

Out of scope (informational)

• Draft PR; linked #458 is already CLOSED — worth confirming the issue should reopen until this actually lands, since #469 (the prior attempt) didn't merge.

[Jerry-Xin]

Summary: This PR is relevant to octo-web, but the new fixed-range deletion can still lose user draft content, which is a blocker for this data-loss fix.

🔴 Blocking

🔴 Critical — Fixed deleteRange(0, snapshotContentSize) is not safe once the live editor has changed.
In packages/dmworkbase/src/Components/MessageInput/index.tsx:1045, the caller deletes the original snapshot size from the current document whenever isEditorUnchanged() is false. That only works if the user appended new content after the old snapshot. If the user moves the cursor and types before the old content, edits in the middle, selects/replaces the old text, or replaces it with a shorter next draft while the send is pending, this range either deletes newly typed content or addresses a to position beyond the current doc size. Concrete same-paragraph-append case: <p>hello</p> has content.size = 7 (paragraph open/close boundary tokens count); after typing new mid-flight the doc is <p>hellonew</p>, and deleteRange({ from: 0, to: 7 }) removes the paragraph-open token + hellon, leaving ew — corrupting the user's text. The prepend case ([X][hello]) is also reachable since the selection/cursor is never pinned during the await. The call path in packages/dmworkbase/src/Components/MessageInput/sendFlow.ts:175 also does not catch cleanup errors, so a ProseMirror range error would escape after a successful send.

This reintroduces the same class of data-loss risk this cleanup is designed to prevent (same root cause as #469). The surgical path should only run when the live document still contains the sent snapshot as an unchanged prefix and the range is valid; otherwise it should preserve the live editor. A robust fix tracks/maps the sent range through transactions (ProseMirror position mapping); at minimum a prefix/validity check is needed.

🔴 Test coverage — The crux logic is still untested.
The new tests in packages/dmworkbase/src/Components/MessageInput/__tests__/sendFlow.test.ts:250 only mock removeSentContent (vi.fn()) and assert it was called with 42. They do not exercise the real TipTap/ProseMirror deleteRange behavior, so the load-bearing position math has zero coverage — exactly the gap that let #469's bug through. Please add coverage against real doc states for: append-only, same-paragraph append, insert-before-snapshot (prepend), edit-middle, and replace-with-shorter-draft.

💬 Non-blocking

🟡 Warning — packages/dmworkbase/src/Components/MessageInput/sendFlow.ts:79 documents collapseExpanded as "only when the editor is cleared," but the changed-editor branch now calls it after partial deletion while preserving a new draft. That may unexpectedly collapse the composer while the user is typing the next message.

✅ Highlights

The helper preserves backward compatibility when removeSentContent is absent, and the top-attachment cleanup remains ID-scoped.

Note: this PR is currently a Draft.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

octo-web PR#470 Review Report

Reviewer: Octo-Q (automated review)
PR: #470
Head SHA: a60aacb4aff417e584f2bdf0b7e69a93819c7b5b
Scope: 3 files, +131 / -15 lines — extends SendCleanup with optional snapshotContentSize + removeSentContent to surgically delete already-sent snapshot content from the live editor when the user started a new draft during the async send.

---

1. Verification

• ✅ Interface extension backward-compatible — snapshotContentSize?: number and removeSentContent?: (size) => void are both optional on SendCleanup (sendFlow.ts:84-98). Existing callers that don't provide these fields get the legacy "leave untouched" behavior.
• ✅ Conditional guard correct — if (cleanup.removeSentContent && cleanup.snapshotContentSize != null) (sendFlow.ts:182) correctly requires both fields. != null properly rejects both undefined and null while allowing 0 (which would be the empty-doc edge case — harmless since deleteRange(0,0) is a no-op).
• ✅ Cleanup actions in !isEditorUnchanged branch — when removeSentContent runs, the branch also calls deleteEditorAttachmentRefs(), revokeEditorPreviewUrls(), and collapseExpanded?.(). This correctly mirrors the unchanged-editor branch's cleanup, minus clearEditor (replaced by the surgical delete). ✅
• ✅ snapshotContentSize capture timing — captured at index.tsx:987 BEFORE sendingRef.current = true (index.tsx:991), so the editor is fully editable and the doc size reflects the exact send-time state.
• ✅ removeSentContent closure captures live editor — (size) => editor.commands.deleteRange({ from: 0, to: size }) (index.tsx:1044-1046) operates on the TipTap editor's current state at invocation time, not the snapshot-time state. This is correct — ProseMirror positions in the unchanged prefix are stable under append-only edits.
• ✅ Return value preserved — both the new removeSentContent path and the legacy fallback return true (editor consumed), matching the pre-existing contract.
• ✅ Top attachment removal unaffected — removeTopAttachments runs before the editor-unchanged check (sendFlow.ts:155-158), so the new code doesn't alter its behavior.

2. Findings

F1 — P2: Non-append edits may cause deleteRange to silently fail or produce incorrect results

diff-scope: new (introduced by this PR's new deleteRange call).

editor.commands.deleteRange({ from: 0, to: snapshotContentSize }) assumes the user appended new content after the snapshot. For the append-only case, ProseMirror's replace algorithm correctly closes the open paragraph on the right side of the slice, preserving the new text. Verified: for <doc><p>hello World</p></doc> (snapshot="hello", snapshotContentSize=7), deleteRange(0, 7) correctly produces <doc><p> World</p></doc>.

However, if the user's edits are not pure appends:

• Mid-document insertion shifts positions after the insertion point, so deleteRange(0, snapshotContentSize) extends into new content — may delete part of the new draft, or fail entirely (ProseMirror throws ReplaceError, TipTap silently drops the command).
• Doc shrunk below snapshotContentSize (user deleted some original text) — to exceeds current doc bounds → command fails silently, falling back to legacy "leave untouched" behavior (duplicate-on-next-send residual).

Severity rationale: P2, not P1. (a) For a chat input, append-only is by far the most common edit pattern. (b) When the command fails, the fallback is the legacy behavior (already-sent text stays), which is the pre-existing residual — not a regression or data loss. (c) No content is lost in any scenario; worst case is the original duplicate-on-send residual persists.

Recommendation: Add a defensive bounds-check before calling deleteRange: if snapshotContentSize > editor.state.doc.content.size, skip the surgical delete and fall back to legacy. Document the append-only assumption in the removeSentContent JSDoc.

F2 — P2: No integration test verifying deleteRange with a real ProseMirror document

diff-scope: new (the 5 new tests all use mocked removeSentContent).

All 5 new tests in the octo-web#458 describe block (sendFlow.test.ts:248-327) use vi.fn() stubs for removeSentContent with an arbitrary snapshotContentSize: 42. They verify the orchestration contract (which functions are called with which arguments) but never exercise the actual editor.commands.deleteRange against a real TipTap document. This means:

• The core correctness property ("deleting positions [0, snapshotContentSize) from the live doc removes only the sent content") is unverified.
• Edge cases (multi-paragraph snapshots, inline attachment nodes, mentions in the snapshot) are not tested.

Recommendation: Add at least one integration test that creates a real TipTap editor with StarterKit, sets content, captures doc.content.size, appends text, calls deleteRange, and asserts the remaining text. This would catch any schema-specific surprises.

F3 — nit: snapshotContentSize of 0 (empty doc) passes the guard but is a no-op

snapshotContentSize != null allows 0. deleteRange({from: 0, to: 0}) is a no-op — harmless but wasteful. Extremely unlikely in practice (empty doc → nothing to send).

3. Suggestions

1. Bounds-check before deleteRange (addresses F1):

   removeSentContent: (size: number) => {
     if (size <= editor.state.doc.content.size) {
       editor.commands.deleteRange({ from: 0, to: size });
     }
   },

2. Integration test (addresses F2): One test with a real TipTap editor verifying the surgical delete against appended text.
3. Consider adding removeSentContent return-value check (editor.commands.deleteRange(...) returns boolean) and logging on failure for diagnostics.

4. Additional Findings

None beyond F1–F3. The code is well-documented, the back-compat contract is clean, and the PR correctly resolves the known #227 residual.

5. Data Flow Tracing

Consumed data	Upstream source	Flows correctly?
snapshotContentSize	editor.state.doc.content.size at index.tsx:987 (pre-send)	✅ captured before async send, passed through SendCleanup, consumed at sendFlow.ts:182
removeSentContent callback	Closure over editor at index.tsx:1044	✅ invokes editor.commands.deleteRange on current state (correct — positions stable under append)
isEditorUnchanged()	Compares JSON.stringify(editor.getJSON()) vs editorSnapshot at index.tsx:996	✅ unchanged from pre-PR behavior, gate correctly controls which branch runs
deleteEditorAttachmentRefs / revokeEditorPreviewUrls	Closures over consumedAttachmentAttrs at index.tsx:989	✅ now also called in the !isEditorUnchanged + removeSentContent path (correct — consumed attachments were sent)
collapseExpanded	Closure over expanded state + setExpanded at index.tsx:1029	✅ called via optional chaining ?.() in both paths

6. Blind-spot Checklist (R5)

• C1 — Double-path parity: N/A. This PR adds behavior to one branch (!isEditorUnchanged) only. The other branch (editor unchanged → clearEditor) is unaffected. No create/delete symmetry to check.
• C2 — Control-flow ordering / nested reuse: Clear. removeSentContent runs exactly once in the new branch. No double-invocation risk. The ordering (removeSentContent → deleteEditorAttachmentRefs → revokeEditorPreviewUrls → collapseExpanded) is consistent with the unchanged branch.
• C3 — Authorization boundary: N/A. No auth/permission/jail changes.
• C4 — Authorization lifecycle: N/A.
• C5 — Build ≠ runtime: N/A. Source-code change in a React component. No build artifacts, extensions, CLI, or relative-URL concerns.
• C6 — Governance/policy docs: N/A.

7. Cross-round Blocker Re-check (R6)

N/A — first review of this PR.

---

[Octo-Q] verdict: APPROVE

The fix correctly resolves the #227 residual for the common case (append-only edits during async send). The back-compat contract is clean, the interface extension is well-designed, and all 13 existing test contracts are preserved. The two P2 findings (non-append edit edge case, missing integration test) are improvements worth considering but do not block landing.

[yujiawei]

Code Review — PR #470 (octo-web)

Head SHA reviewed: a60aacb4aff417e584f2bdf0b7e69a93819c7b5b

Verdict: Changes Requested

The intent of this PR is right and the orchestration scaffolding (SendCleanup extension, back-compat fallback, attachment/id cleanup) is clean. But the core mechanism — deleteRange({ from: 0, to: snapshotContentSize }) against the current editor doc — is not a surgical removal of the sent content. It only works in one narrow case and corrupts the user's new draft in the common case this PR is meant to fix.

I verified the position math against a real ProseMirror/TipTap instance (@tiptap/core's deleteRange is literally tr.delete(from, to) with no clamping), so these are behaviors, not hypotheticals.

---

P0 — removeSentContent deletes the wrong range against a mutated document

packages/dmworkbase/src/Components/MessageInput/index.tsx:1044-1046

removeSentContent: (size: number) => {
  editor.commands.deleteRange({ from: 0, to: size });
},

size is snapshotContentSize = editor.state.doc.content.size captured before the send (index.tsx:987). It is consumed in the !isEditorUnchanged() branch (packages/dmworkbase/src/Components/MessageInput/sendFlow.ts:175-181) — i.e. precisely when the live document has already changed. ProseMirror positions are coordinates into the current tree, not stable identifiers for the old content. Deleting a stale length [0, oldSize) from a changed doc does not target the old content.

Behavior verified against real ProseMirror, sent text "hello" (snapshotContentSize = 7):

User action mid-flight	Live doc	deleteRange(0,7) result	Expected
Type world inline (cursor stays after sent text — the default, since Enter sends)	hello world	world	world
Type world inline (no space)	helloworld	orld	world
Prepend XX	XXhello	o	XX
Press Shift+Enter, type world (new paragraph)	hello¶world	world ✅	world
Edit/shorten sent text below 7 (e.g. hi)	hi	throws Position 7 out of range	—

Two distinct problems:

1. Off-by-the-block-boundary on inline continuation (the common case). Because Enter sends and Shift+Enter is rarely used, a user who keeps typing after sending produces a single merged paragraph. snapshotContentSize includes the snapshot paragraph's close token, which no longer exists as a boundary in the merged paragraph, so the delete consumes one extra inline character — the first character of the user's new draft is silently dropped. This is data loss on the exact flow #458 is meant to protect.

2. Wrong/destructive deletion on prepend and in-place edits, and an unhandled RangeError when the live doc shrinks below snapshotContentSize (the deleteRange runs after the awaited send, so the throw becomes an uncaught rejection and the cleanup silently fails).

Only the separate-new-paragraph case (Shift+Enter then type) is correct, because there the snapshot is a whole leading block and [0, blockSize) happens to map to it.

Fix direction: capture identity, not a length. Two standard options:

• Snapshot the sent range as a ProseMirror Mapping/step rebase: record the original [0, size] and map it through editor.state.tr accumulated since the snapshot to its current coordinates before deleting.
• Or anchor the sent content behind a stable marker (a ProseMirror Decoration/Mark or a tracked position via tr.mapping) captured at send time, then delete from the marker.

Either way the deletion must be against mapped current positions of the originally-sent nodes, never a stale absolute length.

---

P2 — collapseExpanded() fires while the user is actively typing

packages/dmworkbase/src/Components/MessageInput/sendFlow.ts:179

collapseExpanded?.() is now called inside the !isEditorUnchanged() branch. Entering this branch means the user is mid-draft in the composer. Collapsing the expanded composer out from under active typing is jarring, and it contradicts the field's own contract at sendFlow.ts:79-80 ("only when the editor is cleared"). Suggest not collapsing in the changed-editor branch.

---

P2 — New tests do not exercise the risky math

packages/dmworkbase/src/Components/MessageInput/__tests__/sendFlow.test.ts:257-275

The 5 new tests stub removeSentContent as a vi.fn() with a hard-coded snapshotContentSize: 42, so they only assert that the orchestrator forwards 42. They never instantiate a real editor, never mutate the doc mid-flight, and never validate the actual deleteRange outcome for inline-append / prepend / new-paragraph / shrink. The one behavior this PR adds is the one behavior left untested. Please add tests that drive a real ProseMirror doc through each of the table cases above; they would have caught the P0.

---

Summary

The data flow and back-compat design are sound, but the deletion primitive is positionally incorrect against a changed document and loses or mangles the user's new draft in the common path (and throws on shrink). This needs a mapping-based or marker-based removal plus real-editor tests before it can land.