Fix media attachment actions - #4849
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 ## Snapshots Image annotation overlayHovering a photo reveals the Lucide LineSquiggle action without moving the thumbnail.
Image editor controlsThe editor retains its existing spoiler, draw, and close controls.
|
|
@codex 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: 4f789f43da
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f789f43da
ℹ️ 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".
Photos and generic files now upload as soon as they are attached, so an in-flight upload lives in neither pendingImeta nor queuedAttachments. The normal send path did not gate on media.isUploading, so a draft that already had text could be sent mid-upload: the message published without the attachment and the descriptor landed in an already-cleared composer. Gate both submitMessage and the Send button on media.isUploading (the edit-only condition is now unconditional) and assert the disabled/enabled transition in the photo-upload e2e test. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d90da2890e
ℹ️ 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".
The main mediaController is reused across channel/draft changes, and the draft-key lifecycle only clears and restores pendingImeta and queued attachments. Since photos and generic files now upload immediately, an upload can still be in flight when the composer swaps drafts: the captured fillSlot then wrote into the now-current imetaSlots, so the previous channel's attachment could appear in -- or overwrite a slot reserved by -- the new draft. Track an upload epoch that bumps on every wholesale setPendingImeta replacement (draft/channel switch, post-send clear, edit restore). Uploads pin the epoch at start; fillSlot, onUploaded, and the native paperclip path drop their descriptor when it no longer matches, while still settling the uploading count so progress UI does not leak. The updater form of setPendingImeta appends within the current draft, so it does not bump. uploadEditedAttachment needs no epoch: it swaps by URL and no-ops when the target is no longer pending. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed8e7fdfdb
ℹ️ 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".
Two follow-on correctness issues from the immediate-upload split, neither of which changes the intended UI or behavior. Stale previews could hijack the wrong draft. The upload epoch made late completions discard their descriptors, but the old preview row and its cancel button stayed on screen. Cancelling one ran cancelUpload with the replaced draft's slotIndex, nulling whatever attachment now occupied that slot in the draft on screen. Previews now record the epoch they were created in and cancel skips the slot-nulling when it no longer matches. Videos with a missing or opaque MIME type were not queued. shouldQueueFile was MIME-only, so a .mp4 arriving as empty or application/octet-stream (no OS MIME entry, network shares, some pickers) uploaded in the foreground and blocked Send instead of taking the background path. Add a filename-extension fallback in a new videoFileType helper, with a concrete MIME type still authoritative so an image/gif named .mp4 stays an image. The same helper re-types the blob URL used for poster capture, which otherwise yields no poster for those files. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e4e250839
ℹ️ 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".
The upload epoch stopped a replaced draft's uploads from writing their descriptors, but it left their preview rows on screen and their share of uploadingCount intact. Because the normal send path now gates on media.isUploading, an immediate photo/file upload started in draft A kept both the Send button and Enter disabled in draft B until it finished or the user noticed the leftover preview and cancelled it. Fold the retire into the epoch bump: setPendingImeta's wholesale- replacement branch now calls beginNewDraftEpoch, which advances the epoch and marks every active upload canceled, drops its preview, and subtracts it from uploadingCount. Marking them canceled also keeps their completion and error paths quiet, so an upload the user has switched away from cannot raise a banner over the draft now on screen. Bump and retire are one operation so the two cannot drift apart again. Appends against the current draft still do not bump, so a concurrent legitimate upload is untouched. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51ced806c1
ℹ️ 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".
beginNewDraftEpoch cleared the live activeUploadingPreviewIds set while both state updaters still closed over it. React invokes those updaters lazily (and may replay them), so by the time they ran the set could be empty: the preview filter kept every stale row and the count subtracted zero. That reinstated the very bug the retire was added to fix — a draft boundary would leave the previous draft's preview on screen and hold the new draft's send gate closed. Snapshot the ids into a local Set and the size into a local number before clearing, so the updaters read values that cannot change underneath them. The updaters are also idempotent under replay, since filtering and Math.max clamping both settle after the first application. Two regressions cover it: retirement holds when the updaters run after the live set is cleared, and replayed updaters reach the same state. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b7d5d9d6e
ℹ️ 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".
setPendingImeta's updater form runs against the current draft, so it does not bump the upload epoch — but it compacted imetaSlots with a filter, which renumbers the array while an immediate upload still holds an index handed out by reserveSlots. Attach a photo, paste an agent or team snapshot before the upload finishes, and the snapshot became slot 0; the photo's captured fillSlot(0, ...) then overwrote it. Map the updater's result back onto the slot layout instead of replacing it: survivors stay at their existing indexes, removals become null rather than shifting their neighbours, and genuinely new descriptors append after the reserved tail where no pending fillSlot can reach them. An updater that returns its input unchanged (the snapshot-paste dedupe) leaves the slots untouched, identity included. Callers still see the compacted list, so they need no knowledge of reserved slots. Descriptors are matched on url plus sha256, since a url alone can repeat across re-uploads of identical bytes. The slot logic moves to a new imetaSlots module: it is pure, this keeps useMediaUpload.ts under the desktop file-size ratchet, and it lets the nine regressions exercise the real exported functions rather than a copy — the reported repro, appends beside a filled attachment, the same-array dedupe, removals, a full clear, digest-sensitive matching, and a reorder. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6103b24f89
ℹ️ 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".
The queued-attachment remove badge moved from always-rendered to `hidden ... group-hover:flex`, which makes it keyboard-unreachable: a `display: none` element cannot take focus, so a keyboard-only user had no way to drop a queued video from the composer before sending. The hover overlay actions had the same problem. Hide with `opacity-0` and `pointer-events-none` instead, revealing on `group-hover`, `group-focus-within`, and the button's own `focus-visible`. The controls stay in the tab order and focus reveals them before activation, while a mouse behaves exactly as before — a transparent, click-through overlay never swallows a click, and `pointer-events` does not affect keyboard focus or Enter. The intended hover-to-reveal presentation is unchanged; only the mechanism is. Both remove badges and the overlay actions now share one reveal class. Adds an e2e test that focuses the queued remove badge and activates it with Enter (it times out on `focus()` against the `display: none` version), and tightens the existing hover assertion to check opacity and pointer-events rather than DOM visibility. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d868b67aa
ℹ️ 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".
Switching the hover badges from `display: none` to `opacity-0` put them in the tab order, so their accessible names now matter. Two of the three icon-only remove badges had none: the uploaded-media one in `MediaAttachmentItem` and the non-media file-card one. A screen-reader user tabbing the composer heard an unlabeled button. Only the queued-video badge was already labeled. Add `aria-label="Remove attachment"` to both, matching the queued badge. The overlay actions (annotate, edit, spoiler) already carry names via `aria-label` or `sr-only` text. Adds one e2e test per unlabeled branch — each focuses the named button, asserts it is focused and fully revealed, then removes the attachment with Enter. Each fails if its own label is dropped: the image test needs an `image/png` upload descriptor to reach the `MediaAttachmentItem` branch, since the file-level bridge serves a PDF. No visual or pointer behavior changes. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bcc614766
ℹ️ 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".
| // draft that is now on screen. The updater form is an append against the | ||
| // *current* draft (e.g. agent-snapshot paste), so it must NOT bump. | ||
| if (typeof action !== "function") { | ||
| beginNewDraftEpoch(); |
There was a problem hiding this comment.
When setPendingImeta([...]) is used to seed another attachment set, this now always retires every active immediate upload. In MessageComposer, entering edit mode snapshots only pendingImeta and queuedAttachments, so a photo/PDF that is still uploading is not captured; if the user attaches one, chooses “Edit message” before it resolves, and then cancels the edit, the original draft comes back without that attachment. Scope this retirement to boundaries that abandon the draft, or preserve/block edit while foreground uploads are still in flight.
Useful? React with 👍 / 👎.
1 participant
Summary
Snapshots
Image annotation overlay
Image editor controls
Testing
pnpm typecheckpnpm check