Skip to content

fix(desktop,ui): restage a selected message's attachments on edit-and-resend - #6246

Open
ggbdpq wants to merge 2 commits into
apache:mainfrom
ggbdpq:fix/5109-desktop-attachment-edit
Open

ggbdpq wants to merge 2 commits into
apache:mainfrom
ggbdpq:fix/5109-desktop-attachment-edit

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Desktop edit-and-resend refused any selected message carrying attachments: the revision copy stops before the revised turn, so no target-owned refs existed client-side and the edit failed closed. This closes that last #5109 gap by staging the edited turn's artifacts into the copy and re-attaching them through the composer plates, the same flow quotes already follow (#5274).

  • Runtime Host: a revision copy now includes the edited turn's user-uploaded artifacts (the include path built for uploads, whose sentinel turnId the turn-scoped selection misses) and the committed response carries the rewritten, target-owned stagedAttachments.
  • Shared lifecycle (@maka/ui): edit start restages the source attachments as retained pending items; a successful prepare swaps their refs for the copy's target-owned ones and re-keys the plate onto the branch child; cancel drops the edit's own entries and keeps the user's. A rewrite missing from the response keeps its source ref, so the Host's session-owned guard fails that submit loudly rather than sending a dangling reference.

Verification

Check Result
session-revision-protocol tests (decode/optional/malformed stagedAttachments) 6/6 pass; red with the implementation stashed (5/6)
revision-staged-context lifecycle tests (restage/ref swap/re-key/rewrite-missing) 17/17 pass; red with the implementation stashed (15/17)
apps/desktop full typecheck (workspace deps built) exit 0
npm run format:check (full tree) 2535 files, no fixes
npm run check:asf-headers all headers present
Renderer architecture check --base upstream/main passed; frozen shell files zero-diff
Protocol epoch guard --base upstream/main --head HEAD passed (epoch 227)
End-to-end edit of an attachment-bearing message on a packaged app not run locally (Electron binary download blocked); covered by the Host coordinator include-path tests and lifecycle tests

AI use

Implemented by GLM-5.3-Flash (ZCode); human-reviewed.

Checklist

  • tests added (protocol decode + lifecycle restage/ref-swap/cancel-survival)
  • format
  • ASF headers
  • no new dependencies
  • end-to-end run against a packaged Desktop app

Fixes #5109

…d-resend

Desktop edit-and-resend refused any selected message carrying attachments:
the copy excludes the revised turn, so no target-owned refs existed
client-side and the edit failed closed (apache#5109). The Host can now stage them:
a revision copy carries the edited turn's user-uploaded artifacts into the
new Session (the include path built for user uploads, whose sentinel turnId
the turn-scoped selection misses) and returns the rewritten refs on the
committed response.

The shared lifecycle restages the source attachments as retained pending
items at edit start, swaps them for the copy's target-owned refs when the
prepare succeeds, and re-keys the plate onto the branch child — the same
flow quotes already follow (apache#5274). A rewrite missing from the response
keeps its source ref, so the Host's session-owned guard fails that submit
loudly instead of sending a dangling reference. Cancel drops the edit's own
attachments and keeps the user's.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

First-round red was the protocol compatibility epoch guard: the new stagedAttachments response field is wire-observable (older peers reject the additional field in the strict copy-result decoder), so declaring it incompatible-free was not on the table. Bumped RUNTIME_HOST_COMPATIBILITY_EPOCH 227 -> 228 in ecfbaf69e; the guard now reports Protocol changed and the epoch moved: 227 -> 228.

…achments

Older peers reject the additional stagedAttachments field in the strict
copy-result decoder, so the wire change is observable: 227 -> 228.

Generated-by: GLM-5.3-Flash (ZCode)

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Reviewed exact head ecfbaf69e0b276465e8529b57220e630362a6a90 against main 59f61cf7: 11 files, +371/-60. This copies the selected user turn's uploaded artifacts into a revision target, adds optional stagedAttachments to the committed response, forwards them through Desktop, and replaces the previous attachment-edit refusal with composer restaging and cancellation cleanup.

Three P2 findings are attached inline:

  1. Desktop connects the new pending-item restorer to an existing AttachmentRef[] API. The normal root build fails with TS2322 at use-composer-attachments.ts:58, matching CI. With the actual shared React hook, beginning an attachment edit double-wraps the item: its displayed name/size disappear and toSubmittedAttachments() returns a pending object where an attachment reference should be. A conversion adapter in the probe preserves the original reference.
  2. Cancellation compares a complete attachment object against just its .ref, so the edit's own attachment is never removed. The actual revision lifecycle leaves source.txt alongside a newly added new.txt after cancel. A reference-key-only control fixes cancellation before preparation; after preparation the rewritten target reference also needs identity tracking, or it is retained on the source draft after the target copy is abandoned.
  3. The Host looks up an uploaded artifact ID in the filesystem-path map. The existing upload API returns ref.relativePath = record.id; copyConversationArtifacts() maps those through artifactIds, while relativePaths is keyed by the record's physical relative path. Against a real Host over UDS, editing the fixture's uploaded first-turn attachment commits without stagedAttachments. Looking in artifactIds first makes that same request return a target-owned reference. Without it, the composer has no replacement reference and the intended edit cannot submit safely.

Validation: fresh installation and dependency patches; the normal root build:test reaches Desktop and fails at the new type mismatch. The original protocol, staged-context, folder-attachment and real two-client Host revision tests pass (29 total). Additional probes use the actual shared attachment hook and revision actions, plus the existing real Host/storage fixture. Three added assertions fail at this head (hook shape and cancellation before/after prepare); the conversion-adapter control passes. Biome on all 11 changed files, ASF headers, whitespace checks, and the protocol epoch guard pass. The head merges cleanly with current main. No packaged Electron or native Desktop end-to-end run was performed, and the renderer build is blocked by the earlier type error.

P3 retry follow-up: please retain or reconstruct the new staged-reference result on the existing-target path (session-revision-coordinator.ts:770). With only the artifact-map lookup corrected in the probe, the first request returns a staged reference but an identical retry returns only the Session projection. This observation uses that lookup control; I am not counting it as an additional independently reproduced P2 on the unmodified head. The new result should remain usable across retries as well as initial creation.

Protocol: the strict response decoder accepts the new field and rejects malformed attachment shapes. Bumping compatibility epoch 227 to 228 is appropriate and passes the guard against current main. Other open PRs also propose 228; after any of them merges, use the next unused epoch and revalidate before merging this one.

CI: 39854063152 fails in the build on the same new TS2322 error reproduced locally. The description's successful Desktop typecheck claim does not hold for this head. The three P2 findings need fixing; this comment does not approve merging.

attachments: attachments.submittableAttachments ?? [],
restoreQuotes: quotes.restoreQuotes,
clearQuotes: quotes.clearQuotes,
restoreAttachments: attachments.restoreAttachments,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Adapt the pending-item restorer to the real attachment hook

RevisionStagedContext.restoreAttachments now receives PendingAttachment[], but this function accepts AttachmentRef[] and runs every argument through retainedToPending() again. The normal root build and CI both fail with TS2322 here. With the actual shared React hook, beginning an edit double-wraps the pending item: displayName/size become undefined and the submitted retained value has no top-level .ref. The lifecycle tests use a fake restorer that accepts pending items directly, so they miss this wiring failure. Provide a pending-item restore API or convert consistently without losing staging identity; this must also work when re-keying and cancelling.

// dropped; anything else on the plates was staged by the user during the
// edit and survives on the first owner key. ponytail: count-based survival
// can mirror the quote logic if duplicated user additions matter.
const editOwnedRefs = new Set(editOwnedAttachments.map((ref) => JSON.stringify(ref)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Remove the edit-owned attachments when cancelling

The Set contains JSON for full AttachmentRef objects, while the filter compares JSON for item.source.attachment.ref. Those shapes cannot match, so cancelling an edit retains every source attachment in the composer's ordinary draft. The actual lifecycle reproduces this with source.txt plus a newly staged new.txt: cancel leaves both instead of just new.txt. Comparing the same reference shape fixes the pre-prepare case, but after prepare the reference belongs to the target copy and no longer equals the source snapshot either. Track the edit-owned staging entries across the rewrite so cancel drops them while keeping files the user added.

) {
return [];
}
const relativePath = artifactCopy.relativePaths.get(attachment.ref.relativePath);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Resolve uploaded artifact IDs through the artifact-ID map

The upload result at artifact-coordinator.ts:635 stores record.id in ref.relativePath. copyConversationArtifacts() puts that key in artifactIds, not relativePaths (which is keyed by the record's filesystem path). Thus an ordinary uploaded attachment is copied but omitted from this response. A real UDS Host request using the existing first-turn upload fixture commits without stagedAttachments; checking artifactCopy.artifactIds first returns its target-owned reference. The existing conversation-copy reference rewriter already handles both maps. Use that same distinction here, otherwise the client keeps the source-owned ref and attachment edit-and-resend still fails.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Edit-and-resend revisions reject or drop structured message context across Desktop and TUI

2 participants