Skip to content

feat(composer): compose spoiler, annotation, and tooltip controls#1491

Open
tellaho wants to merge 3 commits into
mainfrom
tho/spoiler-formatting-ingress
Open

feat(composer): compose spoiler, annotation, and tooltip controls#1491
tellaho wants to merge 3 commits into
mainfrom
tho/spoiler-formatting-ingress

Conversation

@tellaho

@tellaho tellaho commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Category: improvement
User Impact: Users can mark text and individual image/video attachments as spoilers independently, annotate uploaded images, and get composer tooltips that dismiss correctly from the final combined composer UI.

Problem: The composer’s previous spoiler model coupled text spoilering to pending media, so marking text as spoiler could also hide every attached image/video. In parallel, uploaded-image annotation and tooltip-dismissal fixes touched the same composer attachment/lightbox surfaces, making the intended combined behavior easy to lose during conflict resolution.

Solution: This PR is now the collapsed composer stack for #1491 + #1488 + #1272. It keeps text spoiler as a text-only formatting action, moves media spoiler control into each attachment’s annotation-capable lightbox, preserves uploaded-image draw/save/revert behavior, and applies tooltip dismissal to the controls that survive in the final combined UI.

Final composed behavior

  • Text spoiler stays text-only in the formatting toolbar.
  • Media spoiler is per attachment from the lightbox action bar.
  • Uploaded images use the annotation-capable MediaAttachmentItem path with view/edit modes.
  • The spoiler toggle is hidden while editing/drawing so the image remains unblurred for annotation.
  • Tooltip dismissal applies across the final composer controls: remove, revert, spoiler, edit, attach, mention, emoji, text-spoiler, and formatting toggles.
  • SimpleImageLightbox was extended additively with optional actions; existing consumers such as ViewImageToolPreview are unchanged.

Stack / merge history

This branch intentionally carries the combined composer stack so the merge order is traceable in GitHub history instead of relying on code comments:

  1. feat(composer): compose spoiler, annotation, and tooltip controls #1491 — split text vs. per-media spoiler semantics.
  2. feat(uploads) add image annotation tools to composer uploads #1488 — merged into this branch; annotation remains the media path, with the per-attachment spoiler toggle hosted in the annotation lightbox.
  3. fix(composer): dismiss composer tooltips when cursor leaves the trigger #1272 — merged into this branch; tooltip dismissal is applied to the final composed UI.

#1487 and #1490 stayed independent solo PRs against main.

Validation

  • biome check
  • tsc
  • unit tests ✅
  • build
  • cargo check
  • e2e: spoiler + composer-tooltip-dismiss + composer-image-draw ✅

Screenshots

Lightbox view-mode action bar with spoiler / draw / close:

image

Composer thumbnail after per-attachment spoiler:

image

Notes

@tellaho tellaho changed the title separate text and media spoiler controls chore(spoilers) separate text and media spoiler controls Jul 3, 2026
@tellaho tellaho marked this pull request as ready for review July 3, 2026 08:50
@tellaho tellaho changed the title chore(spoilers) separate text and media spoiler controls chore(spoilers): move spoiler controls Jul 3, 2026
@tellaho

tellaho commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator Author

CI note, corrected after rebase: Desktop Smoke E2E had repeatedly failed the same pre-existing scroll-history assertion by half a pixel (observed 28.5px vs allowed 28px) after several rekicks, while spoiler-specific checks stayed green. Latest main now carries the scroll tolerance fix (32px with rationale), so after rebasing this PR no longer includes an unrelated scroll-test diff.

@tellaho tellaho force-pushed the tho/spoiler-formatting-ingress branch from 427be3e to 9dc2d08 Compare July 3, 2026 18:12
Move the text spoiler toggle into the expanded formatting toolbar and
make it text-only — it no longer mirrors onto pending media
attachments. Media spoilers are now toggled per image/video via a new
control in the attachment lightbox's top-right cluster, next to the
close button and backed by the existing per-URL spoilered set.

The lightbox toggle is placed left of where the upcoming image
edit/draw control (PR #1488) will sit, with a note to hide it while
edit mode is active once that feature lands.

Co-authored-by: Taylor Ho <taylorkmho@gmail.com>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
@tellaho tellaho force-pushed the tho/spoiler-formatting-ingress branch from 9dc2d08 to f32d8fd Compare July 3, 2026 18:23
tellaho added a commit that referenced this pull request Jul 4, 2026
…image-canvas

* origin/tho/spoiler-formatting-ingress:
  feat(composer): split text and media spoiler controls

Stacks #1488 on top of #1491: annotation (MediaAttachmentItem) stays the
media path, and #1491's per-attachment spoiler toggle now lives in that
lightbox's view-mode action bar. Text spoiler remains text-only; the
superseded AttachmentMediaLightbox is removed.

Co-authored-by: Taylor Ho <taylorkmho@gmail.com>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
tellaho added a commit that referenced this pull request Jul 4, 2026
…tooltip-pointer-events

Stacks #1272 on top of #1488 (whose branch now contains #1491): the
tooltip dismiss-on-leave sweep is applied to the final combined composer
UI. The toolbar spoiler button removed by #1491 stays removed, and the
annotation MediaAttachmentItem's tooltips adopt disableHoverableContent.

Co-authored-by: Taylor Ho <taylorkmho@gmail.com>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
#1272)

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: npub1223z34hd7vtwc6qj4s7flsxkj644nlre2nthu7lrrmkumhu3xddsrx9r6w <52a228d6edf316ec6812ac3c9fc0d696ab59fc7954d77e7be31eedcddf91335b@sprout-oss.stage.blox.sqprod.co>
@tellaho tellaho changed the title chore(spoilers): move spoiler controls feat(composer): compose spoiler, annotation, and tooltip controls Jul 4, 2026

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the full diff (18 files, +1406/−274) as the collapsed #1491 + #1488 + #1272 stack. Traced the upload/cancel accounting in useMediaUpload, the new Rust IPC command's validation path, and the lightbox key/remount trick. The architecture is sound and the sharp edges are handled well:

  • uploadEditedAttachment / revertAttachment preview-count and finishUpload bookkeeping balances on success, cancel, and error paths (cancel and onUploadError each own their decrement — verified against cancelUpload).
  • Keying MediaAttachmentItem by originalUrl ?? attachment.url is a neat trick: edit-save and revert both preserve the key, so the open dialog never remounts. Correct in both directions.
  • fetch_media_bytes reuses the exact SSRF/origin/size-cap/MIME validation as the download commands — no new attack surface beyond the existing envelope.
  • Spoiler-membership migration across URL swaps (useAttachmentEditing) is small and covered by the e2e spec.
  • The pruning effect on pendingImeta and the same-callback batched slot-swap + originals re-key are consistent (React batches both setters), so revert state can't observe a torn intermediate.

Findings, none blocking:

  1. Dead code: SimpleImageLightbox's new actions prop has no consumer. After the collapse, ComposerAttachments builds its own DialogPrimitive-based lightbox, and the only remaining SimpleImageLightbox consumer (ViewImageToolPreview) doesn't pass actions. The PR body still describes this as an additive extension for the composer, but the final composed code never uses it — suggest reverting SimpleImageLightbox.tsx to main unless a follow-up needs it.

  2. Perf nit: fetch_media_bytes returns Vec<u8> as a JSON number array over IPC. With the 50 MiB cap that's a worst-case ~150 MB JSON payload to deserialize into number[]. tauri::ipc::Response would send raw bytes. Fine to ship as-is for typical image sizes; worth a follow-up if large images feel slow to enter edit mode.

  3. UX nit: Escape during save exits edit mode but doesn't abort the in-flight upload — the save still completes and swaps the attachment after the user apparently backed out. Low stakes (the swap is revertable), just noting it's a known race.

  4. Revert affordance is lost across a draft/channel round-trip — the originals map is in-memory and pruned when the attachment leaves pendingImeta. The code comment documents this as intentional; agreed it's the right scope, flagging only so it's a recorded decision.

  5. Several of the new comment blocks (e.g. the 8-line tooltip rationale in MessageComposerToolbar, the multi-line lightbox-close rationale) restate design justification that lives in the PR body. House style here trends much leaner — consider trimming to one line each.

CI: all 26 checks green, including both Desktop E2E Integration shards which run the three new/updated specs. Approving — #1 is the only change I'd actually like to see before merge, and it's a deletion.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants