feat(desktop): editable attachments + data-loss fix on message edit#755
Merged
Conversation
wesbillman
force-pushed
the
fix/edit-deletes-attachment
branch
from
327c24e to
5cff4e8
Compare
Edit events carry only new content; imeta tags live on the original
event. The renderer overlays edit.content on the original but
markdown.tsx only renders <img>/<video> for URLs literally in the body.
Editing dropped the attachment markdown -> attachment vanished.
Round-trip imeta through the composer:
- New imetaMediaMarkdown.ts: stripImetaMediaLines + appendImetaMediaLines
+ imetaMediaFromTags (mime-typed {url, m} list, in tag order).
- MessageComposer strips trailing  lines on edit-load
and re-appends them on save.
- ChannelScreen projects editTargetMessage.tags via imetaMediaFromTags;
ChannelPane / MessageThreadPanel forward imetaMedia through editTarget.
Mime-typed list (not bare URLs) so videos round-trip with ![video] alt
regardless of URL suffix (markdown.tsx switches on .mp4 today).
Tests: 12 new node:test cases covering strip/append/round-trip incl.
videos without .mp4 suffix and interleaved-text preservation.
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Self-verify pass on edit-deletes-attachment fix per Beth's review: 1. Allow saving an empty caption when the message still carries imeta attachments. Previously the edit-submit guard rejected an empty trimmed body even though the imeta lines would be re-appended, making caption-less attachment edits a silent no-op. 2. `appendImetaMediaLines` now skips URLs already textually present in the body. Defends future/manual layouts where an imeta line isn't trailing (interleaved captions, reorderable UI, etc.) from double-rendering on round-trip. 3. `imetaMediaFromTags` falls back to `image/jpeg` when an imeta tag lacks a mime type, so legacy events still round-trip. Tests: +4 new (10 → 16 in this file), 262 desktop node:test pass. typecheck + check ✅. Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…aths
Extract formatImetaMediaLine({url, m}) so the mime->alt mapping
(![video] for video/*, else ![image]) lives in one place and the send
and edit paths use the exact same primitive. Behavior preserved.
Co-authored-by: summer <summer@sprout>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wesbillman
force-pushed
the
fix/edit-deletes-attachment
branch
from
5cff4e8 to
88e57dc
Compare
Editing a message with attachments now opens those attachments in the composer's existing pending-imeta row, where the user can remove them, add new ones (paperclip/drop/paste), and edit the caption — exactly like composing a fresh message. Saving emits an edit event (kind:40003) that carries a full new imeta tag set; the receiver overlays the edit's imeta tags onto the original event, leaving non-imeta tags (h, mentions) untouched. This stacks on the data-loss prevention fix from PR #755. The previous approach round-tripped imeta markdown lines through the body to keep attachments visible; now the body is text-only and attachments live purely in tags, matching the send-path data model. Changes - ImetaMedia widened from {url, m} to BlobDescriptor so the same pendingImeta state powers both send and edit. imetaMediaFromTags projects the full descriptor (sha256, size, dim, blurhash, thumb, duration, image) from the original event's imeta tags. - buildImetaTags extracted from the send path; reused for edit. formatImetaMediaLine retained for the send path's body markdown. appendImetaMediaLines removed (edit path no longer reconstitutes body lines — the user-visible body is now the source of truth). - MessageComposer edit-load now seeds media.setPendingImeta from editTarget.imetaMedia; ComposerAttachments renders them with no new UI. Edit-cancel clears the seeded imeta. Edit-submit builds mediaTags from current pendingImeta and passes through onEditSave. - onEditSave signature: (content, mediaTags?). Plumbed through ChannelPane, MessageThreadPanel, useChannelPaneHandlers, useEditMessageMutation, editMessage (tauri.ts), edit_message Tauri command, and build_message_edit (events.rs). - Empty content + empty imeta is still rejected at the Rust boundary to prevent edits from being effective deletions; empty content with attachments is allowed (caption-less media-only edits). - Receiver overlay (formatTimelineMessages): editsByTargetId now carries the edit's tags; the rendered message's tags merge non-imeta original tags with imeta tags from the edit. Optimistic update in useEditMessageMutation mirrors the same projection so local feedback is consistent until the edit event echoes back. Tests - imetaMediaMarkdown.test.mjs extended for the widened imetaMediaFromTags projection and buildImetaTags shape. - New formatTimelineMessages.test.mjs covers the imeta overlay: - no edit → original tags untouched - edit replaces imeta A,B → A,C; non-imeta preserved - edit with zero imeta strips all attachments - edit adding imeta to a previously text-only message - Desktop suite: 269 pass / 0 fail (was 262). - Rust suite (src-tauri --lib): 374 pass / 0 fail. - pnpm typecheck and pnpm check (biome + file-sizes) clean. Out of scope - Mobile parity. Mobile's edit path doesn't handle attachments today; follow-up task. Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Two BLOCKs and two CHANGEs from review of the Slack-style edit feature. BLOCK 1 — rendering bug The previous commit's edit-submit dropped the `` body lines and relied on the receiver tag overlay alone. But `markdown.tsx` only emits `<img>`/`<VideoPlayer>` for URLs literally present in the body — `imetaByUrl` (from `MessageRow.tsx`) is metadata enrichment for nodes the parser has already created, not a synthesizer. Result: post-edit body had no image lines and the renderer drew nothing. Fix: re-append `formatImetaMediaLine` per attachment to the saved body in edit-submit, exactly mirroring the send path. Send and edit now share a single body-shape model. The imeta-tag overlay still runs (so blurhash/poster on `MessageRow` come from the edited tag set), but the `<img>`/`<video>` element itself is back to being driven by the body. BLOCK 2 — entering edit destroyed draft attachments Edit-enter clobbered `pendingImeta` with the edit target's attachments without snapshotting the user's in-flight draft. Edit-exit then set `pendingImeta` to `[]`, dropping any uploads the user had staged for a fresh send. Fix: new `preEditPendingImetaRef` snapshots `media.pendingImetaRef.current` on edit-enter; the else branch restores it (alongside `preEditContentRef`) on edit-exit. CHANGE 3 — drift-proof the overlay test Extracted `applyEditOverlay` and `applyEditTagOverlay` to a sibling `applyEditOverlay.mjs` with a `.d.mts` declaration. Both `formatTimelineMessages.ts` (renderer) and `useEditMessageMutation` (post-edit cache update) now import the same source. The test file imports the same `.mjs` directly — no inlined copy, so the test can't silently drift from production. Also added a test pinning the behaviour that the edit's `h`/`e`/`p` tags do NOT leak into the merged set (only the original's non-imeta tags survive). CHANGE 4 — projection-ceiling comment Documented that `imetaMediaFromTags` drops NIP-92 `alt`/`fallback`/ `service` fields (`BlobDescriptor` doesn't carry them). Loss only fires on cross-client edits today; comment calls out the path forward if those fields ever need to be first-class in the composer. NIT — apply-on-success vs optimistic `useEditMessageMutation` cache update runs in `onSuccess`, not `onMutate`. Reworded the comment so "optimistic" doesn't mislead future readers. Files - desktop/src/features/messages/lib/applyEditOverlay.mjs (new) - desktop/src/features/messages/lib/applyEditOverlay.d.mts (new) - desktop/src/features/messages/lib/formatTimelineMessages.ts (uses .mjs helper) - desktop/src/features/messages/lib/formatTimelineMessages.test.mjs (imports the .mjs directly + 2 new tests pinning leak-prevention) - desktop/src/features/messages/lib/imetaMediaMarkdown.ts (projection-ceiling comment on imetaMediaFromTags) - desktop/src/features/messages/hooks.ts (uses applyEditTagOverlay; reworded apply-on-success comment) - desktop/src/features/messages/ui/MessageComposer.tsx (preEditPendingImetaRef stash/restore; re-append imeta lines on edit-submit) - desktop/scripts/check-file-sizes.mjs (MessageComposer 780→800; rewritten line-budget note) Validation - desktop tests: 271 pass / 0 fail (was 269; +2 from new applyEditTagOverlay leak-prevention tests) - src-tauri --lib: 374 pass / 0 fail - pnpm typecheck ✓, pnpm check (biome + file-sizes) ✓ - pnpm build ✓ (vite resolves the @/...mjs import end-to-end) - All pre-commit hooks green Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Three behavior-preserving cleanups on top of the Beth-approved
attachment-editable edit flow:
- applyEditOverlay.mjs → applyEditTagOverlay.mjs. The combined
body+tags helper was exported and tested but had no production
caller (both the renderer and the post-edit cache update only
used the tag-overlay variant). Drop it before it solidifies as
public surface; rename the module + test file to match what's
actually exported. Adds an immutability test on the way through.
- MessageComposer pre-edit save/restore refs collapse into a single
preEditSnapshotRef holding { content, pendingImeta }. The two
refs were always paired (set together on edit-mode entry, cleared
together on exit), so a single ref-of-record is more honest about
the lifecycle.
- Body+tag construction on send and edit submit (formatImetaMediaLine
loop + buildImetaTags) extract to a shared buildOutgoingMessage
helper in imetaMediaMarkdown.ts. The send/edit difference around
empty pending — undefined vs [] — is documented at the callsites
(the edit path coerces with ?? [] to keep the wipe-attachments
signal that the receiver overlay relies on).
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Clicking Edit on a message left DOM focus on the message row, so global keybinds (Delete, navigation arrows) fired against the row instead of the editor — Wes hit Delete to clear text and got navigation/weirdness. The edit-load effect already called `richText.focus()`, but it ran synchronously alongside Radix-style focus-restoration from the closing context menu. Restoration won the race and put focus back on the row. Fix: defer the focus call to the next animation frame so it lands after the menu's restoration. `focusEnd` (which the existing alias `focus` already pointed at) drops the caret at end of the loaded content. Cleanup cancels the RAF if the effect re-runs (e.g. user cancels before the frame fires) so we don't focus stale state. Files - desktop/src/features/messages/ui/MessageComposer.tsx Validation - Desktop tests: 274 pass / 0 fail - Rust src-tauri --lib: 374 pass / 0 fail - typecheck ✓, biome ✓, file-sizes ✓ (797 lines, budget 800) - All pre-commit hooks green Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Radix's FocusScope on DropdownMenuContent schedules focus-restoration via setTimeout(0), which fires AFTER the composer's RAF-deferred focusEnd. The composer wins the first paint (visible flicker), then Radix steals focus back to the EllipsisVertical trigger, leaving global keybinds (Delete) firing on the message row. Fix is scoped to the Edit menu item only: a per-mount ref flips true in the Edit onClick before onEdit fires, and DropdownMenuContent's onCloseAutoFocus reads it to call event.preventDefault() and reset. Reply / Mark unread / Delete / Copy / reactions / Escape / outside- click all leave the ref false, so default trigger-restoration runs unchanged — keyboard a11y intact for every non-Edit path. The MessageComposer.tsx RAF stays in place as belt-and-suspenders for any future trigger UI that doesn't override. Files - desktop/src/features/messages/ui/MessageActionBar.tsx (+13/-1) Validation - Desktop tests: 274 pass / 0 fail - Rust src-tauri --lib: 374 pass / 0 fail - typecheck ✓, biome ✓, file-sizes ✓ - All pre-commit hooks green Signed-off-by: Wes <wesbillman@users.noreply.github.com>
wesbillman
changed the title
Collaborator
Author
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f47c80c9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+59
to
+61
| type: entry.m ?? "image/jpeg", | ||
| sha256: entry.x ?? "", | ||
| size: entry.size ?? 0, |
There was a problem hiding this comment.
When editing an existing/cross-client media tag that lacks NIP-94 fields (valid NIP-92 can be just url plus another field), this projects it to sha256: "" and size: 0; buildImetaTags then submits x and size 0 on the kind:40003 edit. The relay rejects imeta unless x is a 64-char lowercase hex value and size is positive (crates/sprout-relay/src/handlers/imeta.rs), so merely saving a text edit on such a legacy media message fails instead of preserving the attachment state. Preserve the original valid fields or treat these entries as uneditable rather than synthesizing invalid values.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Good catch — addressed in 8a693907.
buildImetaTags now emits x only when sha256 is non-empty, and size only when it's a positive number. url and m stay unconditional (m has an image/jpeg fallback in imetaMediaFromTags, BlobDescriptor's type is always populated by the upload pipeline).
Scope clarification: this commit fixes the desktop-side structural emit. Successfully editing a legacy/cross-client sparse-imeta message also requires loosening the relay validator (crates/sprout-relay/src/handlers/imeta.rs requires url + m + x + size AND a local /media/... URL). That's tracked as a separate follow-up — a federation/cross-client edit needs the relay-side change to actually land. With this commit alone, the desktop client stops fabricating x / size 0 lines; failure mode shifts from "x must be 64-char hex" to a cleaner "missing required field" rejection.
Tests: 5 new cases in imetaMediaMarkdown.test.mjs covering sparse projection (imetaMediaFromTags with no x / no size), conditional emit (buildImetaTags omits x when sha256 empty, omits size when 0), and round-trip determinism (sparse imeta → projection → rebuild yields same tag).
Hygiene for legacy and cross-client imeta entries that arrive without a sha256 or size. Previously `buildImetaTags` emitted unconditional `x ` and `size 0` lines, which our own relay validator rejects on the way back out, breaking edits that round-trip through such entries. NIP-92 itself treats every imeta field except `url` as optional, so dropping empty `x` / zero `size` is spec-clean. `url` and `m` stay unconditional (`m` already has a fallback to `image/jpeg` in `imetaMediaFromTags`); all other fields were already conditional. The send path is unaffected for fresh uploads (sha256 + size always populated by the upload pipeline). The fix only changes behaviour for projections of legacy/cross-client tags through the edit flow. Tests - `imetaMediaFromTags: entry without x leaves sha256 empty` - `imetaMediaFromTags: entry without size leaves size 0` - `buildImetaTags: omits x line when sha256 is empty` - `buildImetaTags: omits size line when size is 0` - `round-trip: sparse imeta from legacy tags rebuilds without empty x/size` Files - desktop/src/features/messages/lib/imetaMediaMarkdown.ts (conditional x/size emission + JSDoc note) - desktop/src/features/messages/lib/imetaMediaMarkdown.test.mjs (mirror inline buildImetaTags + 5 new tests) Validation - Desktop tests: 279 pass / 0 fail (was 274; +5) - Rust src-tauri --lib: 374 pass / 0 fail - typecheck ✓, biome ✓, file-sizes ✓ - All pre-commit hooks green Signed-off-by: Wes <wesbillman@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
1 participant
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.
Summary
Fixes a data-loss bug AND lands the proper Slack-style attachment-editable edit flow for desktop. Editing a message with an image/video used to delete the attachment from the saved message; this PR makes attachments first-class state in the edit composer (add / remove / replace, alongside text), and fixes follow-up rendering, focus, and tag-shape regressions caught during testing/review.
Reported by: Will Pfleger / kalvin in #sprout-bugs (thread root
6a14995049990b2380527d0bbb90c86d08421f4c7c51efb0eefc01c709d6b0b8).What changed (in order on the branch)
1. Initial data-loss fix (commits 1–3)
Round-trip imeta media through the composer body so existing attachments survive an edit:
lines but remember the imeta-derived URLs.formatImetaMediaLine({url, m})shared between send and edit paths (mime→alt lives in one place).2. Slack-style edit flow (commits 4–5)
Attachments are now first-class composer state on edit, not just preserved metadata:
pendingImetafromeditTarget.imetaMedia. The existing attachment row renders them as removable thumbnails. Paperclip / drop / paste already work in edit mode — no new UI.markdown lines so the renderer (markdown.tsx) draws them —imetaByUrlis metadata enhancement, not a node synthesizer.ImetaMediawidened to fullBlobDescriptor(sha256/size/dim/blurhash/thumb/duration preserved).applyEditTagOverlay.mjs): on render, edit's imeta tags replace original's; non-imeta tags (h, mentions, replies) are preserved.edit_message: empty body + zero attachments rejected (was: empty body alone).3. Refactor pass (commit 6)
Behavior-preserving consolidation:
applyEditTagOverlay.mjs(+.d.mts) — single source for tag overlay, imported by both the renderer and the cache update + tests.buildOutgoingMessage(body, pendingImeta)— shared by send and edit submit paths (undefined= "omit imeta tags" for sends,[]= "wipe attachments" for edits).preEditSnapshotRef: { content, pendingImeta }collapses two refs that were always set/cleared together.4. Focus fix on edit entry (commits 7–8)
Cosmetic regression caught during local testing — pressing Delete after clicking Edit was escaping the composer:
<DropdownMenuContent>schedulessetTimeout(0)to restore focus to the menu trigger on close, which fires AFTERrequestAnimationFrame. Our edit-mode focus call landed (briefly visible flicker) and was then stolen back to the trigger button.editJustSelectedRefflipped on the EditonClick.onCloseAutoFocusreads it andpreventDefault()s only when Edit was the picked item — Reply / Mark unread / Delete / Copy / reactions / Escape / outside-click all keep their default Radix focus restoration (a11y intact).5. Sparse-imeta hygiene (commit 9)
Reviewer (Codex bot) caught an asymmetry:
buildImetaTagswas emittingxandsizelines unconditionally, so a legacy/cross-client imeta missing those NIP-94 fields would surface as literalx(empty value) andsize 0on the kind:40003 edit — both rejected by Sprout's relay validator.xonly whensha256is non-empty; emitsizeonly when> 0. Other optional fields (dim/blurhash/thumb/duration/image) were already conditional.crates/sprout-relay/src/handlers/imeta.rsrequiresurl + m + x + sizeAND a local/media/...URL). That's a separate follow-up tied to federation / cross-client interop.Steps to reproduce the bug (before this PR)
Steps to verify the fix
P0 — bugs we fixed
P1 — new capability
4. Edit a text-only message, paste an image, save. → Image renders.
5. Edit a message with an image, click X on the thumbnail in the composer, save. → Image is gone. Reload. → Still gone.
6. Edit, remove image A, add image B, save. → Only B renders.
7. Send a message with 2 images, edit, remove one, save. → Only the kept one renders.
8. Edit, clear text, keep image, save. → Allowed; renders just the image.
9. Edit, clear text AND remove all attachments. → Save is rejected (no edit-as-deletion).
P2 — interactions
10. Repeat #1, #4, #5 with a video.
11. Reactions on a message stay after edit.
12. Mentions /
p/htags survive the edit.Automated test coverage
desktop/src/features/messages/lib/imetaMediaMarkdown.test.mjs— pure helpers (extract, strip, append, round-trip, edge cases) +buildOutgoingMessagecases + sparse-imeta projection / round-trip / conditional emit.desktop/src/features/messages/lib/applyEditTagOverlay.test.mjs— tag overlay (no-edit pass-through, A,B→A,C swap with non-imeta preserved, edit-strips-all, edit-adds-to-text-only, leak-prevention).Test counts at HEAD: Desktop
node:test279 / 0 · Rustsrc-tauri --lib374 / 0.pnpm typecheck✅,pnpm check(biome + file-sizes) ✅, full pre-push hooks ✅.Out-of-scope follow-ups
crates/sprout-relay/src/handlers/imeta.rsneeds to allow optional fields and non-local URLs. Tracked separately.alt/fallback/serviceprojection — currently dropped becauseBlobDescriptordoesn't carry them. Documented atimetaMediaFromTags.MessageComposer.tsxis now ~800 lines — flagged as a future split candidate.Commits
fix(desktop): preserve attachments when editing messages— initial round-trip.fix(desktop): cover edit-attachment edge cases— caption-less guard, append dedup, legacy-mime fallback.refactor(desktop): share imeta line formatter between send and edit paths.feat(desktop): Slack-style attachment-editable edit flow.fix(desktop): address Beth review on attachment-editable edit flow— render bug + draft-loss.refactor(desktop): tighten edit-attachment helpers— overlay extraction + outgoing-message helper + ref collapse.fix(desktop): focus editor on edit-mode entry so global keybinds land.fix(desktop): suppress Radix focus-restoration when picking Edit— scoped per-Edit-item, a11y preserved on other paths.fix(desktop): omit imeta x/size lines when values absent— sparse-imeta hygiene.