fix(forward): clone content before forwarding to prevent ghost thread creation

[lml2468]

Summary

Fixes #86 — forwarding a file to a thread (子区) creates an unnamed ghost thread instead of sending to the target.

Root Cause

fowardMessageUI and the multi-select forward path both reuse the original MessageContent reference (despite the variable being named cloneContent). For MediaMessageContent subclasses (file/image/video), the file property retains the original File object after the initial upload — the SDK never clears it.

When chatManager.send detects content.file != null, it takes the upload path instead of direct-send. The upload task uses the target channel's type/ID for the storage path (/${channelType}/${channelID}/...). For thread targets (channelType=5), this may trigger server-side auto-create behavior, producing an unnamed ghost thread.

Additionally, sharing the same content reference across multiple forward targets causes mutation interference (upload task writes url/remoteUrl back to the content object).

Fix

• Extract cloneContentForForward() that does an encode() → decode() roundtrip to produce a clean independent instance
• Strip file (prevent unnecessary re-upload — root cause fix)
• Strip reply and mention (not relevant in forwarded context)
• Each (message, channel) pair gets its own clone, preventing cross-target mutation

Changes

File	Change
cloneContentForForward.ts	New utility: safe content cloning for forward
Conversation/index.tsx	Use cloneContentForForward in both forward paths
cloneContentForForward.test.ts	6 tests covering file stripping, reply/mention cleanup, isolation

Testing

• vitest run — 6/6 new tests pass
• No regression in existing Conversation tests (32/32 pass)
• Pre-existing TSC errors unchanged (storybook/svg imports)
• Manual: forward a file to a thread — verify file lands in the correct thread, no ghost thread created

[lml2468]

[APPROVE]

Focused, well-tested root cause fix for the ghost thread bug.

The encode() → decode() roundtrip is the right approach — it produces a type-correct independent clone using the SDK's own serialization contract, and stripping file / reply / mention post-decode covers all three mutation vectors cleanly.

✅ Highlights

• Root cause correctly identified: MediaMessageContent.file surviving upload triggers re-upload with target channel's type/ID, causing server-side auto-create
• Per-(message, channel) cloning in both forward paths prevents cross-target mutation interference
• 6 tests cover the key invariants: file stripping, context cleanup, immutability of original, and clone independence
• Clean separation — new utility is self-contained and well-documented

No blocking or non-blocking findings.

[Jerry-Xin]

This PR is relevant to octo-web and addresses the correct forwarding area, but it introduces a media-forwarding regression for local messages whose upload has not finished yet.

🔴 Blocking

🔴 Critical — cloneContentForForward() always strips file from media content after encoding, even when the media has no remoteUrl yet. For newly sent or still-uploading files/images, FileContent.encodeJSON() and ImageContent.encodeJSON() serialize url from remoteUrl; before upload completion that value is empty. The new clone then removes file, so chatManager.send() takes the direct-send path with an empty media URL, producing a broken forwarded message instead of uploading or blocking the action. The forward menu does not guard against unsent/in-progress messages, so this is reachable from the UI. See cloneContentForForward.ts, cloneContentForForward.ts, FileContent.ts, Image/index.tsx, and module.tsx.

A safe fix would only strip file when the cloned media has a usable remote URL, or reject/disable forwarding for media still in upload state. Please add a regression test for media content with file present and remoteUrl empty.

💬 Non-blocking

🟡 Warning — The tests claim image stripping coverage in the header/PR text, but the added test suite only exercises a custom file content type plus text/reply/mention/isolation. Adding a real ImageContent test would better cover the exact classes used by this component. See cloneContentForForward.test.ts.

✅ Highlights

The per-target clone placement in both single-message and multi-select forwarding paths is the right direction and avoids shared-content mutation across selected target channels.

[Jerry-Xin]

The clone-before-forward approach is the right fix direction — per-target isolation prevents cross-target mutation.

🔴 Blocking

🔴 cloneContentForForward() unconditionally strips file from MediaMessageContent after the encode→decode roundtrip (cloneContentForForward.ts:54). FileContent.encodeJSON() serializes url from this.remoteUrl || "" (FileContent.ts:33). When the original message is still uploading (or just sent locally before upload completion), remoteUrl is empty → the clone gets url: "" → file is stripped → chatManager.send takes the direct-send path with an empty media URL. Result: a broken forwarded message with no content.

Safe fix: only strip file when the clone has a usable remoteUrl, or gate the forward action to reject media still in upload state. Either way, add a regression test for media with file present and remoteUrl empty.

💬 Non-blocking

🟡 Test header comment claims 7 test cases including image stripping, but only 6 it() blocks exist and none exercise a real ImageContent. Minor doc/test gap.

🟡 check-sprint CI is red (required check) — PR is not linked to a Sprint on the project board. Needs the Sprint field set before merge.

✅ Highlights

• Per-target clone placement in both fowardMessageUI and multi-select path correctly prevents shared-reference mutation.
• Thorough JSDoc explaining the root cause, upload-path trigger, and the ghost thread mechanism.
• encode→decode roundtrip is a clean deep-clone pattern for SDK content objects.

[yujiawei]

Code Review — PR #165 (octo-web)

Summary

The fix correctly diagnoses the root cause of the ghost-thread bug on forward and addresses it cleanly. After tracing through wukongimjssdk@1.2.11 source (chat_manager.sendWithOptions, MessageContent.encode/decode), the proposed mechanism matches the SDK's actual behavior:

• chatManager.sendWithOptions branches on content instanceof MediaMessageContent && content.file != null → upload task; otherwise direct send (packet.payload = content.encode()).
• The upload task uses the target channel's type/ID when building the storage path; for channelType=5 (thread) targets the server may auto-materialize a thread → "ghost thread".
• MessageContent.encode() (SDK) embeds both mention and reply into the JSON wire payload, so a re-decode() does restore them — the explicit cleanup in cloneContentForForward is necessary, not redundant.

The new utility, the two call-site replacements, and the unit tests are all consistent with that model. No P0/P1 issues found.

---

Findings

✅ Correct: file stripping is the right root-cause fix

packages/dmworkbase/src/Components/Conversation/cloneContentForForward.ts:54-56

(cloned as any).file = undefined together with the encode→decode roundtrip is sufficient to force the SDK down the "no upload" path (chat_manager line if (!content.file)), so the wire payload uses remoteUrl and packet.channelID/channelType of the target channel — no auto-create on the storage side. Verified against FileContent.encodeJSON() (writes remoteUrl as url) and FileContent.decodeJSON() (restores remoteUrl = url). Roundtrip preserves the URL.

✅ Correct: per-target clone removes the shared-reference hazard

packages/dmworkbase/src/Components/Conversation/index.tsx:372-376 and :1979-1986

Both forward paths now call cloneContentForForward inside the per-channel loop, so the upload task's writes to content.url / content.remoteUrl for one channel cannot leak into the next channel's send packet. This is consistent with the existing vm.ts:1841-1849 comment that already calls out the "同一 content 可能被多目标复用" hazard, so the new code matches the surrounding style.

✅ Correct: stripping reply and mention on forward

cloneContentForForward.ts:57-58

Forwarding a quoted/@-message into a different channel should not carry reply.messageID (which references a message that does not exist in the target channel) or mention.uids (which point to a different channel's members). Stripping them on clone is semantically right and prevents broken references on the receiving side. Tests at cloneContentForForward.test.ts:113-131 cover this explicitly.

⚠️ Minor: PR description overstates the scope to "file/image/video"

PR body, "Root Cause" section

The body lists MediaMessageContent subclasses as "file/image/video". In this codebase only FileContent (packages/dmworkbase/src/Messages/File/FileContent.ts:5), ImageContent (packages/dmworkbase/src/Messages/Image/index.tsx:22), and VoiceContent (packages/dmworkbase/src/Messages/Voice/index.tsx:15) extend MediaMessageContent. VideoContent (packages/dmworkbase/src/Messages/Video/index.tsx:15), GifContent, and ScreenshotContent extend the plain MessageContent and therefore never trip the instanceof MediaMessageContent && file upload branch. So the ghost-thread upload-path failure mode is actually limited to file / image / voice. The fix itself is unaffected; only the PR description is slightly off. Not blocking.

💡 Suggestion: use delete instead of undefined as any

cloneContentForForward.ts:57-58

MessageContent.reply is declared non-nullable in the SDK's .d.ts (reply: Reply;), which is why the PR uses as any. The SDK encode path only checks if (this.reply) so undefined is functionally fine, but delete (cloned as any).reply is equally safe and avoids the cast. Cosmetic only.

💡 Suggestion: tighten cast for file

cloneContentForForward.ts:55

Because cloned is already narrowed by instanceof MediaMessageContent, you can drop the as any:

;(cloned as MediaMessageContent).file = undefined

MediaMessageContent.file is declared file?: File so the assignment type-checks without any. Nit.

💡 Suggestion: behavior note for unregistered content types

cloneContentForForward.ts:47

WKSDK.shared().getMessageContent(contentType) falls back to UnknownContent (SDK message_content_manager.getMessageContent) when no handler is registered. UnknownContent.contentType is hard-coded to MessageContentType.unknown (=0). If someone ever invokes cloneContentForForward on a content type the host has not registered, the clone's wire type would silently downgrade to unknown. All forward-reachable types in packages/dmworkbase/src/module.tsx:204-280 are currently registered, so this is theoretical, but a defensive check (e.g. assert the returned cloned.contentType === content.contentType and fall back to returning the original) would harden it for future plugins. Optional.

💡 Suggestion: minor doc accuracy

cloneContentForForward.ts:11-17

The header doc currently writes "用户自己发送的文件/图片/视频消息" — same minor overcounting as the PR body. Recommend trimming "/视频" to keep the doc accurate to the actual MediaMessageContent subclasses in this repo.

---

Tests

packages/dmworkbase/src/Components/Conversation/__tests__/cloneContentForForward.test.ts

The 6 tests target the exact contract the utility promises: roundtrip preserves text, file is stripped on MediaMessageContent, reply/mention are stripped, the original is not mutated, and multiple clones are independent. Using a local TestFileContent registered against the SDK's content type registry isolates the test from production content classes — sensible.

Two small additions worth considering (non-blocking):

1. A test asserting that the clone's wire payload (decodeWire(cloned)) does not include reply / mention keys after stripping (currently you only assert the in-memory properties).
2. A test that an already-uploaded FileContent (with remoteUrl set, file undefined originally) still produces a working clone — i.e. the "happy path" the bug fix actually relies on at runtime.

---

Verdict

The root cause analysis is correct, the fix is minimal and focused, and the tests exercise the new contract. No correctness, security, or data-loss concerns. The remaining items are documentation accuracy and optional cosmetic/defensive cleanups.