fix: remove residual sent text from input box after send (#458)

[lml2468]

Root Cause

When the user typed a new draft while the previous send was still in flight (slow upload / ack latency), the !isEditorUnchanged() branch in runSendWithCleanup deliberately left the entire live doc untouched to avoid wiping the new draft — but the already-sent text portion was never surgically removed. This is the known residual of the #227 round-2 snapshot-aware cleanup.

The fix direction was identified in MrAladdin's root-cause analysis: remove only the submitted snapshot range while preserving the new draft.

Fix

Extend SendCleanup with two optional, backward-compatible fields:

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

The caller (MessageInput/index.tsx) now:

1. Captures snapshotContentSize from editor.state.doc.content.size before the send
2. Implements removeSentContent via a ProseMirror tr.delete(0, snapshotContentSize) — removes the old sent portion, keeps the new draft

When the editor changed mid-flight and removeSentContent is provided, runSendWithCleanup calls it (plus attachment ref/URL cleanup) instead of leaving the doc untouched.

Backward Compatibility

• Both new fields are optional on SendCleanup
• Callers that don't provide removeSentContent get the original "leave as-is" behavior
• All 13 existing test contracts remain green
• 6 new tests cover the partial-removal path

Test Coverage

New test describe block: runSendWithCleanup — octo-web#458: partial editor cleanup when draft changed mid-flight

Test	What it verifies
calls removeSentContent (not clearEditor) when editor changed AND removeSentContent is provided	New path: surgical removal, no full clear
falls back to leave-as-is when editor changed but removeSentContent is NOT provided	Backward compat for older callers
does NOT call removeSentContent when editor is unchanged (normal full-clear path)	Normal path unaffected
still removes consumed top attachments by id when using removeSentContent path	Top attachments still handled correctly
does NOT call removeSentContent when send resolved false (preserve draft for retry)	Send failure → preserve everything
calls collapseExpanded via removeSentContent path when editor changed mid-flight	Collapse still works in new path

Files Changed

• packages/dmworkbase/src/Components/MessageInput/sendFlow.ts — interface + logic
• packages/dmworkbase/src/Components/MessageInput/index.tsx — caller integration
• packages/dmworkbase/src/Components/MessageInput/__tests__/sendFlow.test.ts — 6 new tests

Fixes #458

[Jerry-Xin]

The PR is relevant to octo-web, but the new ProseMirror range cleanup can delete part of the user’s new draft in the common same-paragraph typing case.

🔴 Blocking

🔴 Critical — tr.delete(0, snapshotContentSize) is not safe for appended text in the same paragraph.
In index.tsx, snapshotContentSize is doc.content.size, which includes node boundary sizes. If the sent draft was hello in one paragraph, its content size is the paragraph node size, not just the text length. While the send is pending, typing the next draft normally appends inside that same paragraph because the editor was not cleared. Deleting 0 -> snapshotContentSize in the live hellonew document crosses into the new text, so it can remove the first character of the new draft, or all of it for short cases. This reintroduces the data-loss class the send cleanup was designed to avoid.

The tests in sendFlow.test.ts only verify that the callback is invoked; they do not exercise an actual Tiptap/ProseMirror document or same-paragraph append behavior. This needs a real editor transaction test or a safer cleanup strategy that maps/removes the exact snapshot content without consuming post-snapshot edits.

💬 Non-blocking

🟡 Warning — sendFlow.ts still documents collapseExpanded as “only when the editor is cleared,” but the new changed-editor branch calls it after partial cleanup. If that behavior is intentional, update the contract comment; otherwise it may collapse the composer while the user is actively typing the next draft.

✅ Highlights

The pure runSendWithCleanup contract remains backward-compatible for callers that do not provide removeSentContent, and the top-attachment cleanup still stays id-scoped.

[OctoBoooot]

Review: fix: remove residual sent text from input box after send (#458) (#469)

Verdict: Request changes

Good problem framing and a clean, backward-compatible interface extension — but the core surgical-removal logic has a positional assumption that re-opens the round-2 data-loss class for non-append mid-flight edits, and that logic is the one part the tests don't actually exercise.

The orchestration layer (sendFlow.ts) is right: removeSentContent is opt-in, the unchanged-editor full-clear path and the send-failed preserve-for-retry path are untouched, and the 6 new tests cover the call-vs-don't-call decision matrix well. The SendCleanup extension is properly optional/backward-compatible.

Major

• MessageInput/index.tsx:removeSentContent — tr.delete(0, snapshotContentSize) treats snapshotContentSize (a ProseMirror content.size, which includes block-boundary tokens) as a flat prefix length over the current doc. That's only correct if the new draft is a brand-new block appended after the snapshot's blocks. It breaks for the most common case — the user keeps typing in the same paragraph. Concrete failing input: snapshot = <p>hello</p> (content.size = 7: para-open + hello + para-close → snapshotContentSize counts the open boundary). During the in-flight send the user appends world in that same paragraph → <p>hello world</p>. tr.delete(0, 7) then deletes the paragraph-open token + hello + the leading space of world, corrupting the block structure and eating part of the user's new draft. Prepending or editing inside the snapshot range is even worse — same root cause. Net: this re-opens the round-2 "draft mangled/wiped" class for any non-new-block edit, which is the bulk of real mid-flight typing. Suggested direction: don't treat content.size as a deletable prefix — diff the live doc against the snapshot JSON to find the actually-sent node range, or map snapshot positions through the transaction, rather than assuming 0→size is a stable, block-aligned prefix.

Minor

• __tests__/sendFlow.test.ts — all 6 new tests mock removeSentContent as vi.fn(), so they verify when it's invoked but never run the real tr.delete(0, snapshotContentSize). The actual ProseMirror deletion — the part with the positional risk above — has zero coverage. A test that builds a real (or realistically-faked) editor doc with [snapshot][appended draft] AND [prepended draft][snapshot] and asserts the surviving content would both prove the append case and surface the prepend bug. The currentSize <= snapshotContentSize guard is also only reached by the real path, so it's currently untested too.

Praise

• The SendCleanup extension is exemplary backward-compat design: both new fields optional, older callers fall through to the original "leave as-is" behavior, and the existing 13 test contracts stay green. The comment block rewrite that turns the old "accepted residual, deferred" note into "now fixed via removeSentContent" keeps the file's design history honest rather than just deleting the old caveat.
• Linking MrAladdin's root-cause analysis and framing the fix as "remove only the submitted snapshot range while preserving the new draft" shows the right intent — the goal is exactly right; it's the position-arithmetic implementation of that goal that needs the non-append case handled.

Out of scope (informational)

• PR is a draft — merge waits for ready-for-review. Body has Fixes #458 so the linked-issue gate should clear.

[mochashanyao]

[Octo-Q · automated review]

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

---

octo-web PR#469 Review Report

Reviewer: Octo-Q (automated review)
PR: #469
Head SHA: 46f43bbd21de17ea4187454c1dacead4ee01f095
Title: fix: remove residual sent text from input box after send (#458)
Files: sendFlow.ts, index.tsx, sendFlow.test.ts (+164 / -16)

---

1. 验证结论

• ✅ Interface extension backward-compatible — snapshotContentSize? and removeSentContent? are both optional on SendCleanup. Existing callers without these fields get the old "leave as-is" behavior. (sendFlow.ts:81-97)
• ✅ !isEditorUnchanged() branch logic correct — When editor changed AND removeSentContent provided, calls surgical removal + attachment ref cleanup + preview URL revocation + collapse. Falls back to no-op when not provided. (sendFlow.ts:167-177)
• ✅ snapshotContentSize capture — Read from editor.state.doc.content.size synchronously before sendingRef.current = true, before any async work. Value is stable at send-time. (index.tsx:983)
• ✅ removeSentContent closure safety — Reads editor.state fresh inside the callback (not captured at send time), so doc and tr reflect current state when called. editor.view.dispatch() targets the live view. (index.tsx:1042-1053)
• ✅ currentSize <= snapshotContentSize guard — Prevents invalid tr.delete(0, N) where N > current doc size. Catches the case where user deleted content during await and new doc is smaller. (index.tsx:1048)
• ✅ Re-entry protection — sendingRef.current guard at index.tsx:923 prevents concurrent sends; the removeSentContent path cannot race with a second send.
• ✅ Test coverage — 6 new tests cover: new surgical path, backward compat fallback, normal path unaffected, attachment cleanup in new path, send-failure preservation, and collapse in new path. All 19 total tests use mock callbacks for orchestration verification.
• ✅ Single production call-site — runSendWithCleanup is only called from MessageInput/index.tsx:991. No other production callers found via grep.

2. 发现的问题

P2-1: Position-based deletion assumes append-only editing during send await

File: index.tsx:1042-1053 (removeSentContent callback)
Diff-scope: new (introduced by this PR)

tr.delete(0, snapshotContentSize) deletes ProseMirror positions 0→snapshotContentSize from the live doc. This correctly removes the sent portion only when the user's new draft was appended after the snapshot content. The assumption:

live doc = [snapshot content (0→snapshotContentSize)] [new draft (snapshotContentSize→end)]

Edge case: If the user edits mid-document during the send await (e.g. selects text at position 5-10 and replaces it with different text), the live doc structure is no longer [sent][new] but a mix. The position-based delete would then remove positions 0→snapshotContentSize, potentially destroying parts of the user's edits while leaving parts of the already-sent text.

Concrete scenario:

1. User types "Hello World" (doc size = 13), sends
2. During await, user selects "World" (pos 7-12), deletes it, types "Next" (pos 7-11)
3. Live doc = "Hello Next" (size = 12), which is < snapshotContentSize=13
4. Guard catches this: 12 <= 13 → returns early, nothing deleted ✓

But if user types MORE:

1. User types "Hello" (size=7), sends
2. During await, selects "llo" (pos 3-6), replaces with "LP NEW DRAFT TEXT" (pos 3-21)
3. Live doc = "HeLP NEW DRAFT TEXT" (size=21), > snapshotContentSize=7
4. tr.delete(0, 7) removes "HeLP NE" → user loses first 5 chars of new draft ❌

Mitigating factors:

• Chat input UX is overwhelmingly append-typed (cursor at end, continue typing)
• sendingRef.current prevents re-entry, limiting the window for complex edits
• The currentSize <= snapshotContentSize guard catches doc-shrinkage cases
• The isEditorUnchanged() JSON comparison catches formatting-only changes

Severity rationale: R2: new code introduced by this PR. R1 assessment: production-reachable but requires a non-typical editing pattern (mid-document select-replace during the brief send await window). For the dominant append-typed case, the fix is correct and a significant UX improvement. The edge case produces data loss (worse than pre-fix residual text) but is low-probability in a chat-input context. P2 — not P1 because the trigger requires deliberate non-append editing during an async send window, which is atypical for chat UX.

Suggestion: Consider adding a comment in the removeSentContent callback documenting the append-only assumption, so future maintainers know the limitation. Alternatively, a future enhancement could diff the snapshot against the current doc to compute the precise deletion range rather than relying on position offsets.

P2-2: snapshotContentSize field on SendCleanup interface is dead code

File: sendFlow.ts:91-93 (interface declaration), index.tsx:1038 (value set)
Diff-scope: new (introduced by this PR)

snapshotContentSize is declared as an optional field on SendCleanup and set by the caller in index.tsx:1038, but never read by runSendWithCleanup. The function only checks cleanup.removeSentContent (truthiness) and calls it. The actual deletion range computation happens inside the removeSentContent callback closure in index.tsx, which already captures snapshotContentSize directly.

This field on the interface serves no functional purpose. It's not necessarily harmful, but it could mislead future readers into thinking runSendWithCleanup uses it for validation or logging.

Suggestion: Either remove it from the interface (it's already captured in the closure), or document that it's metadata-only / reserved for future use.

3. 建议

1. Add a brief code comment to removeSentContent in index.tsx noting the append-only assumption, so the edge case is documented for future maintenance.
2. Remove the unused snapshotContentSize field from the SendCleanup interface, or add a JSDoc note explaining it's informational.

4. 额外发现

• No error handling in removeSentContent: If editor.view.dispatch() throws (extremely unlikely with a valid transaction), deleteEditorAttachmentRefs() and revokeEditorPreviewUrls() in sendFlow.ts:172-173 would not execute, causing minor memory leaks (unreleased File refs and object URLs). Given that valid ProseMirror transactions don't throw, this is a theoretical concern, not a blocking issue.

5. 数据流回溯

Consumed Data	Upstream Source	Verified Flow
snapshotContentSize	editor.state.doc.content.size at index.tsx:983	✅ Captured synchronously before async send; stable value; used in closure at index.tsx:1042-1053
editor.state in removeSentContent	Live editor instance from useEditor hook	✅ Read fresh inside callback, not captured at send time; reflects current doc at callback invocation
editor.view.dispatch in removeSentContent	TipTap editor view from same hook	✅ Same live reference; valid as long as component is mounted (guaranteed by synchronous callback chain)
isEditorUnchanged()	JSON.stringify(editor.getJSON()) === editorSnapshot at index.tsx:996	✅ Compares current editor JSON to send-time snapshot; correctly detects any content/format change
cleanup.removeSentContent truthiness	sendFlow.ts:170	✅ Checks optional field presence; routes to surgical or leave-as-is path

6. 盲点 checklist (R5)

• C1 — 双路径 parity: N/A. No symmetric add/remove/subscribe/unsubscribe paths. The PR extends an existing branching path with an opt-in sub-branch.
• C2 — control-flow ordering / 嵌套复用: clear. removeSentContent is called at exactly one site (sendFlow.ts:171), with no nesting or re-entry. sendingRef.current guard prevents concurrent sends. No security controls to bypass.
• C3 — 授权边界: N/A. Client-side editor cleanup; no authorization boundary involved.
• C4 — 授权生命周期 / 容器-成员状态级联: N/A. No auth changes.
• C5 — build/note ≠ 运行期: clear. Pure TypeScript logic change in React component; no build artifacts, extensions, CLI, or relative-path concerns. Runtime behavior matches source.
• C6 — 治理/策略文档自洽性: N/A. No documentation/policy changes.

7. 跨轮 blocker 复检 (R6)

N/A — automated review review, no prior rounds.

---

[Octo-Q] verdict: APPROVE

Rationale: No P0/P1 findings. The fix correctly addresses the residual-text issue (#458) for the dominant append-typed chat input scenario. Two P2 findings are noted: (1) position-based deletion assumes append-only editing — a documented limitation with low-probability trigger in chat context, and (2) a dead snapshotContentSize field on the interface. Neither blocks landing. The backward-compatible design, clean orchestration in runSendWithCleanup, and solid test coverage (6 new tests covering all branches) give confidence in the change.

[yujiawei]

Code Review — PR #469 (octo-web)

Verdict: Changes Requested — the surgical-removal math reintroduces a data-loss bug in the most common scenario this PR is meant to fix.

Summary

The intent is right: when the editor changed mid-send, remove only the already-sent snapshot range instead of leaving it (residual text) or clearing everything (round-2 data loss). But the implementation deletes a stale absolute position range computed before the await, and that range no longer maps to "the sent content" once the user has typed into the live document. I verified the behavior empirically against prosemirror-model (same schema family as @tiptap/pm).

Spec compliance

The diff implements the stated direction from the #458 root-cause analysis (remove the submitted snapshot range, keep the new draft), the two new SendCleanup fields are optional/backward-compatible, and the orchestration change in sendFlow.ts is sound and well-tested. No scope creep. The problem is not what was built but that the position arithmetic is incorrect, so the fix does not actually hold.

Findings

P0 — tr.delete(0, snapshotContentSize) deletes part of the new draft (data loss) — packages/dmworkbase/src/Components/MessageInput/index.tsx:1042

const snapshotContentSize = editor.state.doc.content.size; // captured before await
// ...
editor.view.dispatch(tr.delete(0, snapshotContentSize));   // run after await

snapshotContentSize is doc.content.size of the snapshot, which includes node boundary tokens (e.g. the paragraph open/close). When the user keeps typing after pressing Enter, the new text is appended inside the same paragraph (the cursor stays in that paragraph), so the live doc is one paragraph, not [sent paragraph][new paragraph]. Deleting the absolute range 0 → snapshotContentSize then eats the paragraph boundary plus the leading characters of the new draft.

Verified against ProseMirror:

Snapshot (sent)	Live doc (after user types)	delete(0, S) result	Expected
hello	hello world (same line)	world	world (leading space lost)
hi	hiX (same line)	`` (empty)	X (X destroyed)
hello	XYhello (prepended)	o	XY (new draft destroyed, sent text kept)
hello	hello\nworld (new paragraph)	world	world ✅ (only the new-paragraph case is correct)

So the callback only behaves correctly when the new draft happens to start in a brand-new top-level block. For the ordinary "user keeps typing on the same line" flow — the exact #458 repro — it silently deletes the first character(s) of the user's new draft, and for a prepend it deletes the whole new draft while leaving the sent text behind (re-creating both the data-loss and the duplicate-send bug).

The root cause is using a raw absolute offset captured before the await. ProseMirror positions taken before intervening edits must be remapped through the transaction/step history (tr.mapping.map(...)) before reuse, or the sent range must be tracked with position-stable markers rather than a numeric content.size.

P1 — if (currentSize <= snapshotContentSize) return aborts cleanup after a net deletion — index.tsx:1044

The guard compares absolute sizes. If the user selects and deletes a chunk of the pending text and then types a shorter new draft during the await, currentSize < snapshotContentSize, the function early-returns, and the already-sent text lingers in the box — the bug is unresolved for that flow. Size comparison is not a valid proxy for "is there a new draft to protect"; isEditorUnchanged() already established the doc changed.

P1 — the new tests do not cover the real deletion math — packages/dmworkbase/src/Components/MessageInput/__tests__/sendFlow.test.ts:64

removeSentContent is wired as a vi.fn() that only flips a flag. All 6 new tests assert that removeSentContent is called (orchestration in sendFlow.ts), never what it does. The actual tr.delete(...) arithmetic in index.tsx has zero coverage, which is why the off-by-boundary defect ships green. This is precisely the "ProseMirror-position tests" the original deferred-follow-up comment said this fix would need. Please add a test that builds a real EditorState / TipTap doc and asserts the surviving document for: (a) same-paragraph append, (b) prepend, (c) new-paragraph append, (d) an inline attachment node at the start of the new draft, (e) net-deletion-then-type.

P2 — collapseExpanded() now fires on the changed-editor path — sendFlow.ts:176

The interface doc says collapse is "only when the editor is cleared" (sendFlow.ts:79). On the new branch the user's draft is still active, so an older send completing can collapse the composer out from under the user mid-typing. Recommend collapsing only on the full-clear path, or gating it on the post-removal editor actually being empty.

Coverage notes (what I did and didn't verify)

• Verified: the four ProseMirror position scenarios above (empirically), single caller of runSendWithCleanup (index.tsx:991), the backward-compat fallback path, and the sendFlow.ts orchestration logic (correct).
• Not verified: whether isEditorUnchanged() flags non-text mutations (e.g. attachment-only changes), behavior with inline atom nodes (mentions/pasted images) at the snapshot boundary, and any IME/composition edge during the await. The inline-atom case is likely also affected by the same boundary error and should be in the added tests.

Recommendation

Replace the absolute delete(0, snapshotContentSize) with a position-mapped removal: capture the sent range, map it through tr.mapping (or track it via stable ProseMirror positions/marks), and delete the mapped range; drop the size-comparison guard in favor of the already-available "changed" signal; then add real-document tests covering same-line append, prepend, and inline-atom cases. The orchestration layer in sendFlow.ts can stay as-is.