fix(conversation): clear draft after sending

[botshen]

Summary

• render draft mention placeholders as display names in conversation previews
• clear local and remote draft state after a message is actually sent
• refresh conversation listeners so the sidebar drops the draft label immediately

Fixes #182

Tests

• pnpm --filter @octo/base exec vitest run src/Utils/tests/draftPreview.test.ts src/Service/tests/messageContinuity.test.ts src/Components/Conversation/tests/sendContentProxy.test.ts

[Jerry-Xin]

The PR is in scope for octo-web, but the new post-send draft clearing can erase a newer draft in a realistic async send/navigation flow.

🔴 Blocking

• 🔴 Critical: clearDraftAfterSend() can wipe a draft created after the send started. MessageInput.send() calls props.onSend(...) without awaiting it at index.tsx, then clears the editor at index.tsx. The PR only clears the draft after the async send chain finishes at index.tsx. If the user sends, then types a new draft or switches conversations before the ack wait completes, dealloc() saves that newer draft via markConversationExtra() at index.tsx, but the original send later calls clearDraftAfterSend() and persists an empty draft at index.tsx. Make the clear conditional on the draft snapshot that belonged to the sent message, or track a send generation/current draft value so a later draft is not cleared. Add a regression test for “send pending, new draft saved, send resolves”.

💬 Non-blocking

• 🟡 Warning: conversationExtraUpdate() returns a Promise, but the new clear path does not await or catch it at index.tsx. That can leave the remote draft uncleared without any signal. This is not the same severity as the race above, but it weakens the PR’s remote-state guarantee.
• 🟡 Warning: I could not run the stated test command in this checkout because vitest is not available in the package environment: Command "vitest" not found.

✅ Highlights

• The PR correctly treats draft preview rendering as a small pure helper in draftPreview.ts.
• The conversation list now shows draft previews even when there is no last message, which matches the described sidebar behavior.

[lml2468]

Summary

Clears conversation drafts after successful sends and formats mention placeholders in draft previews.

Findings

• P1 packages/dmworkbase/src/Components/Conversation/index.tsx:2653 - No additional unique findings beyond the existing requested-change review; I independently verified that the draft-clearing race remains present at the PR head. Because MessageInput.send() invokes the async onSend path without awaiting it, a newer draft saved while the send is still waiting for ack can still be overwritten when clearDraftAfterSend() runs later.

Verdict

CHANGES_REQUESTED because the existing draft-clearing race is valid and still affects this PR's main behavior.

[yujiawei]

Code Review — PR #183 (octo-web)

Verdict: APPROVED — Two real user-facing bugs (#182) fixed correctly, with focused unit coverage and no regressions. No P0/P1 issues found.

Summary of changes

The PR addresses two conversation-list draft bugs:

1. Bug 1 — mention placeholders shown as raw UIDs in draft preview. New Utils/draftPreview.ts (formatDraftPreview) converts the stored @[uid:label] draft encoding into human-readable @label, with broadcast sentinels (-1/-2 → @所有人, -3 → @所有AI) reusing the shared constants from mentionRender.ts. ConversationList.lastContent now routes the draft through this formatter.
2. Bug 2 — draft not cleared after a successful send. Conversation is refactored so markConversationExtra delegates to a new updateConversationExtra(draft); a new clearDraftAfterSend() clears local remoteExtra.draft, persists draft="" to the server, and fires notifyConversationListeners(..., update) so the sidebar drops the [Draft] label immediately. It is invoked only when anyMessageSent is true.

1. Verification

• ✅ Bug 1 fix is correct. formatDraftPreview regex @\[([^:\]]+):([^\]]+)\] (draftPreview.ts:11) is equivalent to the producer/parser regex in MessageInput/index.tsx:484 (@\[([^\]:]+):([^\]]+)\]) — character-class order is irrelevant, so encode/decode round-trips correctly. Sentinel handling matches mentionRender.ts constants (MENTION_UID_LEGACY_ALL/HUMANS/AIS), so display stays consistent with the live message render path.
• ✅ Bug 2 fix is correct and defensively layered. clearDraftAfterSend (Conversation/index.tsx:1057) mutates the in-memory remoteExtra.draft before notifying listeners, so the re-render sees the empty draft without waiting for the server round-trip; the server POST then persists it. Both the [Draft] label (ConversationList/index.tsx:675) and the preview text (:377) key off remoteExtra.draft, so both clear in one pass.
• ✅ Guarded on actual send. clearDraftAfterSend() runs only inside if (anyMessageSent) (:2653). The newly added anyMessageSent = true after the fallback text send (:2649) is a genuine correctness fix — previously the plain-text fallback path never set the flag, so a text-only send would (a) not clear the draft and (b) mis-handle the trailing reply-attach branch. Good catch by the author.
• ✅ No null-deref from the lastContent reorder. Moving the remoteExtra.draft read above the !conversationWrap.lastMessage guard is safe: the SDK Conversation.remoteExtra getter lazy-initializes a ConversationExtra (wukongimjssdk esm :1832), so it is never null. Side effect is a behavioral improvement — a conversation that has a draft but no last message now shows the draft preview instead of nothing.
• ✅ Tests pass. Ran the three cited suites locally:
  • draftPreview.test.ts — 3/3 pass (member label, broadcast sentinels, malformed placeholder left unchanged).
  • messageContinuity.test.ts + sendContentProxy.test.ts — 15/15 pass (no regression in the send pipeline this PR touches).
• ✅ No build break from changed lines. tsc --noEmit reports no errors on draftPreview.ts or any of the new/modified lines in Conversation/index.tsx. (The repo surfaces pre-existing, environment-level react type-resolution errors across many untouched files; none are attributable to this PR.)

2. Issues

None blocking. No P0/P1.

3. Suggestions (non-blocking / nits)

• P2 — extra server write per send. clearDraftAfterSend always calls updateConversationExtra(""), which POSTs to conversations/.../extra on every successful send, even when no draft was ever stored. Functionally harmless, but you could skip the network call when the prior remoteExtra.draft was already empty to shave one request off the hot send path. Optional.
• Nit — formatDraftPreview label may contain :. Because the label group is [^\]]+ (greedy, colon-permitted) while uid is [^:\]]+, an input like @[u_1:a:b] yields @a:b. This matches the parser's behavior in MessageInput, so it's consistent and correct — just noting the asymmetry is intentional, not a bug.
• Nit — test coverage gap for Bug 2. The draft-preview formatting is unit-tested, but the clearDraftAfterSend / anyMessageSent behavior (the second half of the fix) has no direct test. The existing sendContentProxy/messageContinuity suites exercise the surrounding send path but don't assert the draft is cleared. A small test asserting clearDraftAfterSend zeroes remoteExtra.draft and notifies listeners would lock the regression. Optional given the change is small and the manual repro is clear.

4. Additional observations

• The fix correctly reuses mentionRender.ts constants rather than hardcoding -1/-2/-3 and the 所有人/所有AI labels, keeping the draft preview in lockstep with the message-render and dropdown paths — good adherence to the single-source-of-truth pattern already established in that module.
• No cross-file dependency hazards: all new symbols (formatDraftPreview, updateConversationExtra, clearDraftAfterSend) are self-contained and their imports (ConversationAction, WKSDK) are already present in the file.

Ship it.

[yujiawei]

Code Review — PR #183 (octo-web)

Verdict: CHANGES_REQUESTED — The draft-preview formatting (Bug 1) is correct and well-tested, but the draft-clearing logic (Bug 2) contains a real data-loss race. Updating my earlier note: I initially missed the async boundary in MessageInput.send(); after tracing it I agree with the two prior reviews that this is blocking.

1. P1 (blocking) — clearDraftAfterSend() can wipe a newer draft

MessageInput.send() fires props.onSend(...) without awaiting it (packages/dmworkbase/src/Components/MessageInput/index.tsx:1051) and then synchronously clears the editor (:1077). The draft clear, however, runs only at the end of the async send/ack chain (packages/dmworkbase/src/Components/Conversation/index.tsx:2653 → clearDraftAfterSend() at :1057), which includes sendTextAndWaitAck round-trips.

Reachable sequence (send → type next → switch away before ack):

1. User hits send → onSend is dispatched (async, still pending) and the editor is cleared.
2. User types a new draft B into the now-empty editor for the same conversation.
3. User switches conversation → dealloc() → markConversationExtra() (:1022) persists draft B.
4. The original async send finally resolves → clearDraftAfterSend() persists draft="" (:1062), wiping B both in memory (remoteExtra.draft = "") and on the server.

On a slow network the ack window is easily long enough to hit this. The fix should make the clear conditional on the draft snapshot that belonged to the sent message — e.g. capture the draft string (or a monotonically increasing send generation) at send time and only clear if remoteExtra.draft still equals that snapshot. Please add a regression test for "send pending → new draft saved → send resolves → new draft survives".

2. P2 (non-blocking)

• Unawaited remote write. clearDraftAfterSend calls updateConversationExtra("") → conversationExtraUpdate() returns a Promise that is neither awaited nor .catch-ed (Conversation/index.tsx:1062, datasource datasource.ts:193). A failed POST leaves the remote draft uncleared with no signal/retry. Worth at least a .catch log.
• Extra server write per send. The clear POSTs unconditionally even when no draft was stored. Minor; could short-circuit when the prior remoteExtra.draft was already empty.

3. What is correct (no changes needed)

• ✅ Bug 1 fix is solid. formatDraftPreview (Utils/draftPreview.ts:9) regex @\[([^:\]]+):([^\]]+)\] is equivalent to the producer/parser regex in MessageInput/index.tsx:484, so encode/decode round-trips. Broadcast sentinels (-1/-2→@所有人, -3→@所有AI) reuse the mentionRender.ts constants, keeping the preview consistent with the live message-render path. draftPreview.test.ts passes 3/3 (member label, sentinels, malformed-placeholder passthrough).
• ✅ The lastContent reorder (ConversationList/index.tsx:377) is null-safe — the SDK Conversation.remoteExtra getter lazy-initializes a ConversationExtra, never null — and is a behavioral improvement (drafts now preview even with no last message).
• ✅ The added anyMessageSent = true on the plain-text fallback path (Conversation/index.tsx:2649) is a correct fix in its own right (the fallback previously never set the flag).
• ✅ messageContinuity.test.ts + sendContentProxy.test.ts pass 15/15; tsc shows no errors on the new/changed lines.

4. Recommendation

Address the P1 race (snapshot/generation-guard the clear + regression test). The rest of the PR is good to go once that lands. The P2 items are nice-to-have in the same change.

[botshen]

Updated in 644ec3d9:

• Replaced formatDraftPreview regex replacement with a linear scanner to address the CodeQL polynomial-regex warning.
• Added a send-time draft snapshot/generation guard so a pending send cannot clear a newer draft saved or typed after the send started.
• Await/catch the remote draft-clear write and keep the sidebar refresh after the guarded clear.
• Added regression coverage for malformed/adversarial draft placeholders and the pending-send/new-draft race guard.

Tests:
pnpm --filter @octo/base exec vitest run src/Utils/__tests__/draftPreview.test.ts src/Utils/__tests__/draftLifecycle.test.ts src/Service/__tests__/messageContinuity.test.ts src/Components/Conversation/__tests__/sendContentProxy.test.ts

[Jerry-Xin]

This PR is in scope for octo-web and the implementation is directionally sound: it addresses draft preview rendering and avoids clearing newer live drafts after a send.

🔴 Blocking
None.

💬 Non-blocking
🟡 Warning — packages/dmworkbase/src/Utils/draftPreview.ts: Broadcast mention labels are still hardcoded through MENTION_LABEL_HUMANS / MENTION_LABEL_AIS, so the new draft preview is not actually locale-aware despite the test wording saying “localized labels.” This is consistent with existing mention behavior, so I would not block this PR, but it should be tracked if English UI parity matters.

🟡 Warning — packages/dmworkbase/src/Components/Conversation/index.tsx: remoteExtra.draft is cleared before conversationExtraUpdate("") succeeds, and listeners are notified even if the remote update throws. That gives immediate UI feedback, but a failed API call can leave local/sidebar state temporarily disagreeing with the server until the next sync.

✅ Highlights
The draft-clearing guard in draftLifecycle.ts is simple and correctly protects live/newer drafts from being erased by a pending send.

The mention placeholder parser in draftPreview.ts avoids regex backtracking risk and includes useful malformed-input coverage.

Verification note: I attempted the focused Vitest command, but this checkout has no available vitest binary through pnpm --filter @octo/base exec, so tests were not runnable locally here.

[lml2468]

Summary

This revision adds a pure draft lifecycle guard with unit coverage, captures the editor draft snapshot and draft-save generation before the async send path, awaits/logs draft-clear persistence, and replaces the draft preview regex with a linear parser.

Previous Findings Resolution

RESOLVED: the previous P1 draft-clearing race is fixed in the new code. MessageInput passes the formatted draft snapshot into onSend, Conversation captures sendDraftGeneration before the first async send work, markConversationExtra() increments draftSaveGeneration for saved drafts, and clearDraftAfterSend() now refuses to clear when the live editor has newer text or when a later saved draft exists. A newer draft typed while the send is pending is therefore protected by liveDraft; a newer draft saved during navigation/unload is protected by draftSavedAfterSend plus latestSavedDraft.

New Findings

P1 - Required status check check-sprint / check-sprint is failing on head SHA 644ec3d929bfc5ea2bcef377f0913cfa1a3dfb92: the workflow reports Sprint mismatch. Current sprint is Sprint W22, but linked issue Sprint is Sprint W23. This is not a source-code regression in the changed files, but it is a blocking PR status and needs the linked issue Sprint corrected and the check rerun before merge.

No new blocking source-code issues found in the updated draft lifecycle, Conversation, MessageInput, or draft preview changes.

Verdict

CHANGES_REQUESTED

[yujiawei]

Code Review — PR #183 (octo-web)

Verdict: APPROVED — The blocking data-loss race flagged in earlier reviews is now correctly fixed by the guard draft clearing race commit (644ec3d9), backed by a dedicated regression test. Both #182 bugs are resolved with no regressions. Remaining items are P2 polish only.

What changed

1. Bug 1 — mention placeholders in sidebar preview. New pure helper formatDraftPreview (Utils/draftPreview.ts) decodes @[uid:label] markers into display labels, reusing the mentionRender.ts sentinel constants.
2. Bug 2 — clear draft after a message is actually sent. clearDraftAfterSend now clears local + remote draft state and refreshes conversation listeners, but only when a generation/snapshot guard (shouldClearDraftAfterSend, Utils/draftLifecycle.ts) confirms it is safe.

1. Verification of the previously-blocking race — RESOLVED ✅

The prior CHANGES_REQUESTED reviews (against d9e0278c) correctly identified that MessageInput.send() dispatches props.onSend(...) without awaiting it (MessageInput/index.tsx:1053) and clears the editor synchronously (:1080), while the draft clear only runs at the end of the async ack chain (Conversation/index.tsx:2666 → clearDraftAfterSend :1062). That window allowed a draft typed/saved after send to be wiped.

The new commit closes this. At send time it captures both a draftSnapshot (MessageInput/index.tsx:1059) and sendDraftGeneration = this.draftSaveGeneration (Conversation/index.tsx), and markConversationExtra() now bumps draftSaveGeneration and records latestSavedDraft synchronously (:1031-1036). shouldClearDraftAfterSend (Utils/draftLifecycle.ts:8) then refuses to clear when:

• a live draft exists in the editor (liveDraft truthy), or
• a newer non-empty draft was saved after this send (draftSavedAfterSend && latestSavedDraft), or
• the remote draft no longer matches the snapshot that was actually sent.

I traced each branch of the reported sequence (send → type new draft → switch away before ack):

• Switch away: Conversation is keyed by channel.getChannelKey() (Pages/Chat/index.tsx:902), so a channel switch remounts the component and dealloc() runs on the same old instance, bumping generation to N+1 and saving the new draft. The later clearDraftAfterSend(gen=N) sees draftSavedAfterSend=true + non-empty latestSavedDraft → returns early. New draft survives. ✅
• Stay in channel: liveDraft is the freshly typed text → returns early. ✅
• Normal send (no follow-up): liveDraft="", remote draft empty or equal to snapshot → clears as intended. ✅

The keyed remount also means a pending clear can never write to a different channel's conversation, since it dereferences this.vm.currentConversation of the instance that owned the send.

Regression coverage: draftLifecycle.test.ts covers all four branches (clear / live-draft / newer-save / empty-save). Tests pass locally: 9/9 across draftLifecycle.test.ts + draftPreview.test.ts (vitest run, vitest 4.1.5).

2. Bug 1 (preview formatting) — correct ✅

formatDraftPreview is decode-equivalent to the producer regex /@\[([^\]:]+):([^\]]+)\]/g (MessageInput/index.tsx:486): both treat the first : as the uid/label split and require a non-empty uid and label, so encode→decode round-trips, including labels that themselves contain :. Broadcast sentinels (-1/-2→@所有人, -3→@所有AI) reuse the shared mentionRender constants, keeping the preview consistent with the live message render path. Malformed markers are passed through unchanged, and the indexOf-based scan is linear (the 1000×@[9 adversarial test confirms no backtracking).

The lastContent reorder in ConversationList/index.tsx:377 (check draft before lastMessage) is null-safe — the SDK remoteExtra getter lazy-initializes — and is the intended behavioral improvement (drafts preview even with no last message).

3. Non-blocking observations (P2 — optional, not required to merge)

• Unawaited remote write — addressed. The earlier P2 is fixed: updateConversationExtra("") is now await-ed inside a try/catch (Conversation/index.tsx:1077-1081), so a failed POST is logged rather than silently dropped.
• Redundant empty POST. When no draft was ever stored (remoteDraft===""), the guard still allows the clear and issues an empty conversationExtraUpdate. Harmless, but could short-circuit when the prior remote draft was already empty.
• Edited restored-draft edge case. If a conversation has a persisted draft X, the user edits the restored editor to Y and sends without ever leaving the conversation, then at clear time remoteDraft is still the stale X (drafts only re-persist on dealloc) while snapshot=Y, so the guard conservatively does not clear and the sidebar keeps showing X. This is a no-regression incomplete-clear (pre-PR, drafts were never cleared at all) and the conservative choice correctly prioritizes avoiding data loss over a cosmetic stale label. Worth a follow-up if the stale preview is observed in practice.

4. Recommendation

Ship it. The P1 race that blocked the earlier revisions is genuinely fixed and tested; the P2 items above are nice-to-haves that do not block merge.