fix(chat): keep pictures when editing a queued message - #398
Merged
Conversation
Editing a queued message removed it and then recalled only its text, so
any attached picture was lost — the queued entry was already gone, and the
composer had no way to carry the image back.
A server-queued attachment carries only `{id, name, mimeType, sizeBytes}`;
the bytes live on the server. So the attachments are now read back through
their signed asset URLs before the removal — before, because the removal is
what makes the edit destructive. Web rebuilds a `File` + object URL, mobile
a data URL, matching what each composer's drafts already hold. Mobile's
local outbox path already did this and is unchanged.
An attachment that cannot be read back is reported rather than dropped
silently, and the message's text still comes back for editing.
Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
… back Adversarial review found the first pass still lost pictures in three ways. Removing a queued message prunes its attachment files server-side (ProjectionPipeline "thread.queued-message-removed"), so anything that goes wrong before the composer has the bytes is unrecoverable. - Signed asset URLs resolve asynchronously. Editing before they land — or through a network blip — restored nothing but removed the message anyway, which is the reported bug under a race. The edit now backs out and leaves the message queued, so it can be retried. - The composer's 8-image limit was ignored, so an edit could leave a draft that no longer sends. The edit is now refused up front when the pictures would not fit, rather than restoring some and dropping the rest. - On web the recalled images went to whichever composer was current after the awaits, so switching threads mid-fetch moved them to the wrong draft. They are written to the draft target captured before the awaits instead, matching the retry path; this also drops the ChatComposer handle change. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
Second review round: pinning the recalled pictures to the thread captured at click time left the text going through the live composer handle, so switching threads mid-edit split a message in two — pictures on the original thread, text on whichever thread was open by then, with the queued copy already gone. An unmounted composer dropped the text outright. Text and pictures now both go to the captured draft target. The composer handle is still called, but only while it belongs to that thread, since all it adds is input history and cursor placement. The failure message no longer promises that retrying will help: an attachment the server no longer has will never come back, and the message says so. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
omegent-app Bot
added a commit
that referenced
this pull request
Aug 15, 2026
#401 was squash-merged. That kept the code and threw the lineage away: the 23 upstream commits stopped being ancestors, so `fork/dev` read as **29 commits behind upstream when only 6 were genuinely outstanding**, and the next sync would have re-merged and re-resolved all 23 — on the same mobile files that took sixteen conflicts to land the first time. Nobody noticed for two merges. It surfaced in a deploy alert that said **"Commits (2)"** for a range that had carried 23. ## What this adds A `push`-triggered check on `fork/dev` that fails when a commit which carried a sync has fewer than two parents, and prints the `-s ours` repair in the log. Sync commits are identified by **the head branch of the PR they came from**, not by their subject, because subjects vary by merge method: ``` Merge pull request #400 from patroza/sync/upstream-2026-08-12b Merge upstream/main into fork/dev (23 commits) (#401) merge: sync upstream through b73232b ``` A merge-button commit names the branch inline, so no API call is needed; squash and rebase commits are resolved through the API, with the subject line as a fallback when that is unavailable. **Ordinary fork PRs are untouched** — they are expected to squash, and are never checked. ## Verified against the real commits | commit | what it is | result | |---|---|---| | `5e63531b1` | #401, squash-merged sync | **fails**, exit 1 | | `0bf7835cc` | #400, sync merged properly | recognised as a sync, passes (2 parents) | | `a76069bd9` | #402, an ordinary squashed PR | not flagged | The third row is the one that matters most: the guard has to stay silent on your normal workflow. ## This detects, it does not prevent Worth being explicit, since it was the first question asked: **clicking merge does not fail.** The check runs after the merge lands, because GitHub has no per-PR merge-method control, and a repository-wide setting cannot allow squash for ordinary fork PRs while requiring a merge commit for syncs. Squash merges do fire `push` — `5e63531b1` triggered Fork CI at 08:08 — so this turns a silent, weeks-later discovery into a red check within a minute. Actual prevention is `gh pr merge <n> --merge`, which is how #398, #399 and #400 all landed correctly. That is now written into the sync runbook in `AGENTS.md`. If you would rather it be enforced at the button, the next step is a `sync:upstream` label workflow that merges the PR through the API once checks pass — say the word and I will add it. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) via [T3 Chat](https://t3.chat) on Discord --------- Co-authored-by: omegent-app[bot] <306514130+omegent-app[bot]@users.noreply.github.com> Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Queue a message with a picture, then edit it: the picture was gone.
Editing removes the queued message and puts it back in the composer — but the recall carried only
text, and the composer handle had no way to take anything else. Since a user removal also prunes the message's attachment files server-side (ProjectionPipeline→thread.queued-message-removed), the picture was gone from disk, not just from the screen.The fork already did this correctly in one of three places, which is what made the intent unambiguous:
The local outbox works because its attachments already hold their bytes. A server-queued attachment carries only
{id, name, mimeType, sizeBytes}, so the two broken paths now read the bytes back through the attachment's signed asset URL before the removal, and rebuild what each composer's drafts actually hold — aFile+ object URL on web, a data URL on mobile.Nothing is removed that cannot be restored
Two adversarial review rounds (grok-4.5, gpt-5.6-sol) found the first pass still lost pictures three ways. Everything that could fail now fails before the removal:
Known, deliberate
addImagesdedupes onname + mimeType + sizeBytes, so recalling a picture the draft already holds is a no-op.Verification
pnpm typecheck(18 packages) ·pnpm test(2763 passed; the pre-existingCodexTextGenerationlaunch-args failure is unrelated) ·vp buildinapps/web·vp check --fix. 29 new tests cover the rebuild, the per-attachment URL fetch, partial failure, and the capacity guard on both platforms.Co-authored-by: Patrick Roza 42661+patroza@users.noreply.github.com
🤖 Generated with Claude Code via T3 Chat on Discord