feat(desktop): add pre-recording upload probe for instant mode#1827
feat(desktop): add pre-recording upload probe for instant mode#1827JYZ-LESLIE wants to merge 2 commits into
Conversation
superagent-security
Bot
added
the
contributor:verified
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let upload_probe_result = if matches!(inputs.mode, RecordingMode::Instant) { | ||
| match video_upload_info.as_ref() { | ||
| Some(video) => Some(run_upload_probe(&app, &video.id, user_max_instant_resolution).await), | ||
| None => None, | ||
| } | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
run_upload_probe is fully awaited before ShowCapWindow::InProgressRecording is shown (line 1076). If the probe times out, the user gets no visible response for up to UPLOAD_PROBE_TIMEOUT (15 s) after clicking record — directly breaking the "instant" promise of this mode. The probe should be launched concurrently with (or after) showing the window, with the result applied asynchronously. If the resolution must be known synchronously, the timeout should be a fraction of a second (e.g. 3–5 s) rather than 15 s.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/recording.rs
Line: 998-1005
Comment:
**Probe blocks recording UI for up to 15 seconds**
`run_upload_probe` is fully awaited before `ShowCapWindow::InProgressRecording` is shown (line 1076). If the probe times out, the user gets no visible response for up to `UPLOAD_PROBE_TIMEOUT` (15 s) after clicking record — directly breaking the "instant" promise of this mode. The probe should be launched concurrently with (or after) showing the window, with the result applied asynchronously. If the resolution must be known synchronously, the timeout should be a fraction of a second (e.g. 3–5 s) rather than 15 s.
How can I resolve this? If you propose a fix, please make it concise.| crate::api::PresignedS3PutRequest { | ||
| video_id: video_id.to_string(), | ||
| subpath: "instant-upload-health-probe.bin".to_string(), | ||
| method: PresignedS3PutRequestMethod::Put, |
There was a problem hiding this comment.
The 256 KB file at instant-upload-health-probe.bin is PUT into the video's S3 path via a real signed URL but is never explicitly deleted. Every Instant recording leaves this orphaned object behind, accumulating storage costs and cluttering the video bucket. Consider using a dedicated, separately-namespaced probe path with an S3 lifecycle rule, or delete the object after the measurement completes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/recording.rs
Line: 657-660
Comment:
**Probe payload never cleaned up from S3**
The 256 KB file at `instant-upload-health-probe.bin` is PUT into the video's S3 path via a real signed URL but is never explicitly deleted. Every Instant recording leaves this orphaned object behind, accumulating storage costs and cluttering the video bucket. Consider using a dedicated, separately-namespaced probe path with an S3 lifecycle rule, or delete the object after the measurement completes.
How can I resolve this? If you propose a fix, please make it concise.| if let Some(probe) = upload_probe_result.as_ref() { | ||
| let _ = RecordingEvent::UploadProbe { | ||
| status: probe.status, | ||
| upload_mbps: probe.upload_mbps, | ||
| recommended_max_resolution: probe.recommended_max_resolution, | ||
| applied_max_resolution: probe.applied_max_resolution, | ||
| detail: probe.detail.clone(), | ||
| } | ||
| .emit(&app); | ||
| } |
There was a problem hiding this comment.
RecordingEvent::UploadProbe is emitted immediately after show(&app).await returns. If InProgressRecording is a freshly created window, the JS may still be bootstrapping and the createEffect that subscribes to the event will not yet have run. The probe result would be silently dropped and the status badge would never appear. The resolution cap itself is correctly applied server-side, but the UI feedback is lost. Emitting the event after the first Countdown tick (when the window is guaranteed to be interactive) would be safer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/recording.rs
Line: 1083-1092
Comment:
**UploadProbe event may fire before the window's JS listener is registered**
`RecordingEvent::UploadProbe` is emitted immediately after `show(&app).await` returns. If `InProgressRecording` is a freshly created window, the JS may still be bootstrapping and the `createEffect` that subscribes to the event will not yet have run. The probe result would be silently dropped and the status badge would never appear. The resolution cap itself is correctly applied server-side, but the UI feedback is lost. Emitting the event after the first `Countdown` tick (when the window is guaranteed to be interactive) would be safer.
How can I resolve this? If you propose a fix, please make it concise.| <Show when={hasCameraInput() && disconnectedInputs.camera}> | ||
| <div | ||
| class="flex h-8 w-8 items-center justify-center" | ||
| title="Camera disconnected - recording continues without camera overlay" | ||
| > | ||
| <IconLucideVideoOff class="size-5 text-amber-11" /> | ||
| </div> | ||
| </Show> |
There was a problem hiding this comment.
After the refactor the Show when={hasCameraInput() && disconnectedInputs.camera} block is indented one level deeper than its siblings. The visual nesting in the JSX tree no longer matches the actual DOM hierarchy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/in-progress-recording.tsx
Line: 835-842
Comment:
**Camera-disconnected block is over-indented**
After the refactor the `Show when={hasCameraInput() && disconnectedInputs.camera}` block is indented one level deeper than its siblings. The visual nesting in the JSX tree no longer matches the actual DOM hierarchy.
How can I resolve this? If you propose a fix, please make it concise.
|
Correction (clean formatting): Addressed the latest Greptile feedback in commit e3a4544:
Validation notes:
|
|
/claim #73 |
1 participant
/claim #73
This implements a scoped upload-speed health check for desktop Instant recordings, and applies a conservative resolution cap before recording starts.
What changed
start_recording(Instant mode only):<4 Mbps -> 720p4-10 Mbps -> 1080p10-20 Mbps -> 1440p>=20 Mbps -> 4Kmin(user_setting, recommended)so user preference remains an upper bound.RecordingEvent::UploadProbeevent and surfaced the probe status in the in-progress recording controls UI.Behavior notes
Validation
cargois unavailable.Demo video
Greptile Summary
This PR adds a one-shot upload-speed probe to Instant mode: before recording starts, a 256 KB payload is PUT to a signed S3 URL, the measured throughput is mapped to a resolution tier, and the lower of the user's setting and the recommended tier is applied. A new
UploadProbeevent surfaces the result in the in-progress recording UI as a status badge.run_upload_probeis fully awaited beforeShowCapWindow::InProgressRecordingis shown, so the user sees no feedback for the entire probe duration on a slow/timeout path — directly at odds with Instant mode's promise.instant-upload-health-probe.binobject is never deleted after measurement, accumulating storage per recording.UploadProbeis emitted immediately aftershow(&app).await, which may return before the window's JS has registered its event listener, silently dropping the UI badge.Confidence Score: 3/5
The resolution cap and probe logic work correctly in the happy path, but the probe runs synchronously before the recording UI appears, which can stall Instant mode for up to 15 seconds on slow or failing connections.
The core correctness of the resolution math and fallback behaviour is solid. The main concern is that awaiting the probe before showing the window breaks the responsiveness of Instant mode. Additionally, each recording leaves a 256 KB orphaned file in S3, and the UploadProbe event is fired in a window where the frontend listener may not yet be active.
apps/desktop/src-tauri/src/recording.rs — specifically the ordering of run_upload_probe relative to ShowCapWindow::InProgressRecording and the missing probe file cleanup.
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(desktop): add upload probe for inst..." | Re-trigger Greptile