Hi @alanpoon, sorry just getting around to reviewing things after my bonanza of makepad fixes.

Is this PR ready to merge in? (just wondering since you hadn't yet requested a review from me or assigned the waiting-on-review label)

if so, lmk and i'll review it asap.

hi @alanpoon, friendly bump on this. Is this ready? (aside from the conflicts)

No real rush, but i'm planning to do an alpha release soon and it'd be great to have this feature included! Plus I'd hate to see all your hard work go to waste.

Working on it.

I had Claude max do a thorough review, just out of curiosity. I was aware of some of these things and think they can be left for later, but some of them do need to be addressed now.

You don't have to fully implement replies, though I think it's basically just a one-line fix, so you might as well since it's quite trivial. But you definitely need to address points 1-5 below, as well as 6 and 7. Note that point 3, 6, 7 are all things i brought up already, related to the TimelineKind usage.

Points 9, 10, 12, 13, 14 are also pretty important to address.

🚨 Critical — real behavior bugs

1. Replies are silently dropped on every attachment send.
https://github.com/project-robius/robrix/blob/image_upload_2.0/src/sliding_sync.rs#L1721-L1723

// For now, we'll just send the attachment without reply support
// TODO: Add proper reply support for attachments
let _ = replied_to; // Suppress unused warning for now

replied_to is plumbed through 4 layers (RoomInputBar → action → room_screen → MatrixRequest::SendAttachment) and then thrown away. The "reply with an attachment" feature is visibly broken — AttachmentConfig supports .reply(Reply) and that needs to be wired.

Worse: RoomInputBarRef::handle_file_upload_confirmed (room_input_bar.rs:848) take()s replying_to and calls clear_replying_to(cx) — so the user's reply state is destroyed, not just unused. And retry in room_screen.rs hardcodes replied_to: None, so the reply cannot be recovered even once this TODO is fixed.

2. Cancel button does nothing to actually abort the upload.
https://github.com/project-robius/robrix/blob/image_upload_2.0/src/sliding_sync.rs#L1714

let _send_attachment_task = Handle::current().spawn(async move { ... });

The JoinHandle is bound to _ and dropped. TimelineUpdate::FileUploadAbortHandle is defined and plumbed all the way through to UploadProgressView::set_abort_handle() — but is never emitted. So self.abort_handle is always None when the user clicks cancel; handle.abort() never runs. The UI hides, but the upload continues, still consumes bandwidth, still eventually posts the message.

Fix: grab task.abort_handle() before letting the JoinHandle drop, then sender.send(TimelineUpdate::FileUploadAbortHandle(abort_handle)).

3. Attachment can be sent to the WRONG room.
https://github.com/project-robius/robrix/blob/image_upload_2.0/src/app.rs#L304-L314

Some(FilePreviewerAction::UploadConfirmed(file_data)) => {
    if let Some(selected_room) = &self.app_state.selected_room {
        if let Some(timeline_kind) = selected_room.timeline_kind() {
            if let Some(sender) = get_timeline_update_sender(&timeline_kind) {
                let _ = sender.send(TimelineUpdate::FileUploadConfirmed(file_data.clone()));

The target room is resolved from self.app_state.selected_room at the moment the user clicks "Upload" in the modal, not at the moment they clicked the attach button. Repro:

1. Open Room A, click attach, pick a file.
2. Switch to Room B while the modal is open (or while the background load is still running).
3. Click Upload.

Result: the file uploads to Room B silently. This is the most dangerous issue in the PR — user data goes to the wrong conversation. FileData needs to carry its originating TimelineKind from the moment the picker opens.

4. rfd::FileDialog::pick_file() blocks the UI thread.
https://github.com/project-robius/robrix/blob/image_upload_2.0/src/room/room_input_bar.rs#L641

if let Some(selected_file_path) = dialog.pick_file() {

pick_file() is synchronous — it blocks the calling thread until the user picks or cancels. This is called from handle_actions on the main UI event loop, so the entire Robrix UI freezes for as long as the native dialog is open. The code comment claims "required for non-windowed environments" but that is not accurate — rfd exposes AsyncFileDialog::pick_file().await which is the intended API for GUI apps.

5. Progress updates race with completion/error and can overwrite the error UI.
https://github.com/project-robius/robrix/blob/image_upload_2.0/src/sliding_sync.rs#L1735-L1753

Two independent tasks write to the same sender:

• the progress-forwarding task: subscriber.get() → sender.send(FileUploadUpdate { current, total }) → subscriber.next().await
• the send task: send_future.await → sender.send(FileUploadError | FileUploadComplete)

No ordering between them. A late FileUploadUpdate arriving after FileUploadError causes set_upload_progress to overwrite the error's status_label with "Uploading… N%" — the error message disappears and the user sees a stale progress bar on a dead upload. (Also possible for FileUploadComplete, but benign there because the view is hidden.)

🔴 High — architectural

6. get_timeline_update_sender acquires a backend Mutex from the UI thread.
https://github.com/project-robius/robrix/blob/image_upload_2.0/src/sliding_sync.rs#L2285-L2294

ALL_JOINED_ROOMS is a std::sync::Mutex held regularly by matrix_worker_task (14 call sites). Calling get_timeline_update_sender from app.rs on the UI thread can block the UI while the worker holds the lock. This is the symptom of the broader issue you already flagged — the root cause is the circuitous request routing:

UI modal → FilePreviewerAction::UploadConfirmed → app.rs → TimelineUpdate::FileUploadConfirmed → room_screen.rs → MatrixRequest::SendAttachment → sliding_sync.rs

Half of this indirection exists only to fetch replying_to from the RoomInputBar. Cleaner: route FilePreviewerAction::UploadConfirmed to the RoomScreen itself (it already owns the RoomInputBar and knows its TimelineKind). Then get_timeline_update_sender can be deleted and the public API stays clean. This also eliminates bug #3.

7. UploadProgressViewAction::Retry carries OwnedRoomId, not TimelineKind.
https://github.com/project-robius/robrix/blob/image_upload_2.0/src/home/upload_progress.rs#L122-L125

Already noted in the first review. Related: the RoomScreen retry guard if tl.kind.room_id() != &room_id { continue }; only compares room IDs, not thread root IDs — so retries can't distinguish main-timeline vs thread-timeline uploads within the same room. The action should carry TimelineKind.

🟠 Medium — correctness

8. No file-size limit. A user picks a 4 GB video. The entire file is std::fs::read'd into a Vec<u8>, cloned for the modal, cloned again for the MatrixRequest, then file_data.data.clone() passed to send_attachment — five+ copies in RAM. Homeservers typically cap ~50–100 MB anyway; reject early with a helpful message and consider Arc<Vec<u8>> for the fields that must survive multiple hops.

9. The generated thumbnail is dead code. image_utils::generate_thumbnail produces a JPEG thumbnail stored in FileData.thumbnail, but:

• Preview in file_upload_modal.rs:210 uses &file_data.data (the full image), not the thumbnail.
• Upload in sliding_sync.rs:1757 uses AttachmentConfig::new() — thumbnail is never passed via AttachmentInfo.

Lanczos3 resize on a large image is CPU-expensive and currently wasted. Either wire the thumbnail into AttachmentConfig and use it for the modal preview, or remove the generation entirely.

10. FileLoadedData.dimensions is set but never read. Same story — populated in open_file_picker, dropped in convert_loaded_data_to_file_data.

11. is_displayable_image claims bmp/gif/webp but the image crate features are only jpeg+png. Cargo.toml: features = ["jpeg", "png"]. imghdr::from_bytes and mime_guess will happily report image/gif / image/webp / image/bmp, is_displayable_image returns true, then thumbnail decode fails — logs an error, falls back to file icon. Works, but wastes a decode attempt. Either add the image features, or trim is_displayable_image to formats you can actually handle.

12. No duplicate upload prevention. While a file is being read in the background thread, clicking the attach button again overwrites self.pending_file_load; the old thread finishes and silently send(...)-fails. The first file the user picked is lost with no feedback. Similarly, nothing prevents starting a second upload while one is in flight — the single UploadProgressView is reset, so the first upload becomes unobservable and uncancellable.

13. UploadProgressViewAction::Cancelled has no listener in room_screen.rs. The action is emitted by the widget but nothing catches it, so the only effect of cancel is a local UI hide. Combined with bug #2, the full cancel path is an illusion.

14. AttachmentConfig::new() with no info. No AttachmentInfo::Image/Video/Audio/File with dimensions/duration/mime. Other clients see a generic blob with no thumbnail, degraded rendering. Worth tracking even if fixed in a follow-up.

🟡 Low — code quality

15. handle_file_upload_confirmed returns Option<Option<Reply>>. Outer option = "borrow_mut succeeded", inner = "has reply". Used in one place; split into two methods or collapse (since a failed borrow_mut() is semantically the same as "no reply" for the caller).

16. let _ = replied_to; // Suppress unused warning is a code smell — it ships a broken feature with a silencer. If the feature isn't ready, don't plumb the parameter; if it is, wire it up.

17. Double Cx::post_action on close (file_upload_modal.rs:260-264): Cancelled then Hide. If Upload and close both fire in the same frame, the user can get UploadConfirmed immediately followed by Cancelled — upload goes through anyway. Minor, but collapse to a single action.

18. is_tsp_signing_enabled inconsistent cfg. #[cfg(feature = "tsp")] on RoomInputBarRef but uncfg'd on RoomInputBar. Gate both or neither.

19. Debug log!("FileUploadModal: Loading image preview, data size: {} bytes, mime: {}", ...) looks like a debug-print that slipped through.

20. Label { flow: Flow.Right{wrap: true}, ... } on file_name_label is suspicious — Labels don't usually accept flow.

✅ Things that look fine

• image_utils::generate_thumbnail aspect-ratio math is correct.
• format_file_size is reasonable.
• ProgressBar SDF drawing is bounded by clamp(0.0, 1.0).
• Dependency additions are appropriate (rfd gated to desktop; image with minimal features).
• script_mod registrations are wired consistently across home/mod.rs, shared/mod.rs, app.rs, lib.rs.
• Empty-file rejection in open_file_picker is good.

Suggested priority

Block on #1–#6. Those six are user-visible data-safety, liveness, or correctness bugs. #7 is a natural companion to #6 and they're best fixed together with the routing redesign.

Also, if you're busy with other things, I can have Claude take a stab at fixing these things, they're not too complicated so it ought to be doable. Just let me know.