Repository navigation
feat(desktop): add upload speed test resolution scaling and startup health check (#73) - #3188
khoap1220-hue wants to merge 1 commit into
Conversation
|
Someone is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| app_state.was_camera_only_recording = true; | ||
| } | ||
| } | ||
| crate::upload_health::cancel_in_flight_probe(&app).await; |
There was a problem hiding this comment.
On Linux, this cancellation runs after clean_capture::prepare returns. That function already sets RecordingState::Pending and waits for the user's shortcut. A probe uploading when preparation starts can keep using the connection throughout that wait. Cancel the probe when the recording first becomes pending, including this earlier path.
Knowledge Base Used: Desktop recording orchestration
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/recording.rs
Line: 2074
Comment:
**Linux cancels probes too late**
On Linux, this cancellation runs after `clean_capture::prepare` returns. That function already sets `RecordingState::Pending` and waits for the user's shortcut. A probe uploading when preparation starts can keep using the connection throughout that wait. Cancel the probe when the recording first becomes pending, including this earlier path.
**Knowledge Base Used:** [Desktop recording orchestration](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/desktop-recording-orchestration.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return c.json({ | ||
| ok: true, | ||
| receivedBytes, | ||
| storageReady: true, |
There was a problem hiding this comment.
storageReady is always true, even though this handler only reads and discards the request body. If the user's upload bucket is unavailable or its credentials stop working, the desktop still reports “Upload ready.” Check the selected upload destination before reporting readiness, or clearly limit this result to web-server reachability. The GET/HEAD handler has the same issue.
Knowledge Base Used: Web API and domain services
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/desktop/[...route]/root.ts
Line: 737-740
Comment:
**Broken storage looks healthy**
`storageReady` is always `true`, even though this handler only reads and discards the request body. If the user's upload bucket is unavailable or its credentials stop working, the desktop still reports “Upload ready.” Check the selected upload destination before reporting readiness, or clearly limit this result to web-server reachability. The GET/HEAD handler has the same issue.
**Knowledge Base Used:** [Web API and domain services](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/web-api-and-domain-services.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let message = match state { | ||
| UploadHealthState::Healthy => format!( | ||
| "Upload ready ({upload_mbps:.1} Mbps) · Recording at {quality_label}" | ||
| ), | ||
| UploadHealthState::Degraded => format!( | ||
| "Slow upload ({upload_mbps:.1} Mbps) · Recording scaled to {quality_label} for reliability" |
There was a problem hiding this comment.
quality_label comes from the measured speed, but recording uses the smaller of that recommendation and the user's ceiling. A free user above 8 Mbps sees “Recording at 1080p” while recording is capped at 720p. Build the displayed label from the effective ceiling, or describe it as a recommendation instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/upload_health.rs
Line: 355-360
Comment:
**Recording quality is overstated**
`quality_label` comes from the measured speed, but recording uses the smaller of that recommendation and the user's ceiling. A free user above 8 Mbps sees “Recording at 1080p” while recording is capped at 720p. Build the displayed label from the effective ceiling, or describe it as a recommendation instead.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let head_req = client | ||
| .head(&url) | ||
| .header("Authorization", format!("Bearer {bearer_token}")) | ||
| .timeout(RTT_PROBE_TIMEOUT) | ||
| .send(); |
There was a problem hiding this comment.
These raw requests skip apply_env_headers, which supplies x-vercel-protection-bypass. If the desktop targets a protected preview with VITE_VERCEL_AUTOMATION_BYPASS_SECRET configured, ordinary API calls can work while both health requests are rejected. Reuse the shared request path so the HEAD and POST probes send the same deployment headers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/upload_health.rs
Line: 239-243
Comment:
**Preview health checks fail**
These raw requests skip `apply_env_headers`, which supplies `x-vercel-protection-bypass`. If the desktop targets a protected preview with `VITE_VERCEL_AUTOMATION_BYPASS_SECRET` configured, ordinary API calls can work while both health requests are rejected. Reuse the shared request path so the HEAD and POST probes send the same deployment headers.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if let Some(received) = received_bytes | ||
| && received != payload_bytes | ||
| { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
A missing receivedBytes value passes this check and still produces a speed. That skips the promised check that the full payload arrived before bandwidth is calculated. Require received_bytes == Some(payload_bytes) and add a missing-field test so an incomplete response cannot change recording quality.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/upload_health.rs
Line: 126-130
Comment:
**Missing byte counts pass**
A missing `receivedBytes` value passes this check and still produces a speed. That skips the promised check that the full payload arrived before bandwidth is calculated. Require `received_bytes == Some(payload_bytes)` and add a missing-field test so an incomplete response cannot change recording quality.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.|
|
||
| let effective_duration = match rtt_estimate { | ||
| Some(rtt) if post_elapsed > rtt => { | ||
| (post_elapsed - rtt).max(MIN_EFFECTIVE_UPLOAD_WINDOW) |
There was a problem hiding this comment.
The new Duration subtraction violates the repository guide's explicit requirement to use saturating_sub for Duration and Instant. Replace post_elapsed - rtt with post_elapsed.saturating_sub(rtt). This repository requirement must be satisfied before merging.
| (post_elapsed - rtt).max(MIN_EFFECTIVE_UPLOAD_WINDOW) | |
| post_elapsed.saturating_sub(rtt).max(MIN_EFFECTIVE_UPLOAD_WINDOW) |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/upload_health.rs
Line: 134
Comment:
**Time subtraction breaks repository rules**
The new `Duration` subtraction violates the repository guide's explicit requirement to use `saturating_sub` for `Duration` and `Instant`. Replace `post_elapsed - rtt` with `post_elapsed.saturating_sub(rtt)`. This repository requirement must be satisfied before merging.
```suggestion
post_elapsed.saturating_sub(rtt).max(MIN_EFFECTIVE_UPLOAD_WINDOW)
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| app.on(["GET", "HEAD"], "/upload-health", withAuth, async (c) => { | ||
| return c.json({ | ||
| ok: true, | ||
| storageReady: true, | ||
| timestamp: Date.now(), | ||
| }); | ||
| }); | ||
|
|
||
| app.post("/upload-health", withAuth, async (c) => { |
There was a problem hiding this comment.
The new /upload-health handlers use ad-hoc Hono callbacks. The repository guide requires new endpoints under apps/web/app/api/* to use the Effect HttpApi pattern and its shared handler. Add this endpoint through that pattern rather than extending the legacy callbacks. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/desktop/[...route]/root.ts
Line: 694-702
Comment:
**New API skips required pattern**
The new `/upload-health` handlers use ad-hoc Hono callbacks. The repository guide requires new endpoints under `apps/web/app/api/*` to use the Effect `HttpApi` pattern and its shared handler. Add this endpoint through that pattern rather than extending the legacy callbacks. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return cached_or_default(app).await; | ||
| } | ||
|
|
||
| // Step 2: Upload 256 KiB test probe to /api/desktop/upload-health |
There was a problem hiding this comment.
The new comments narrate obvious operations, including “Step 2: Upload 256 KiB test probe” and “Truncated upload must be rejected.” The repository guide bans comments that simply describe the code. Remove these and the similar recording-guard comment; retain explanations of non-obvious decisions such as the RTT trade-off. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/upload_health.rs
Line: 255
Comment:
**Comments repeat the code**
The new comments narrate obvious operations, including “Step 2: Upload 256 KiB test probe” and “Truncated upload must be rejected.” The repository guide bans comments that simply describe the code. Remove these and the similar recording-guard comment; retain explanations of non-obvious decisions such as the RTT trade-off. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks a lot for putting this together, the upload speed test and health check are a thoughtful idea. We're closing this one because it targets the legacy Tauri desktop app (apps/desktop), which we've replaced with the new native GPUI app and are no longer taking feature changes for. Automatically scaling recording resolution based on measured upload speed is also a product decision we're still working through for the new app, alongside a couple of related proposals. If you'd like to pursue it there, please open an issue first so we can agree on the behaviour before any code. Thanks again for the contribution. |
|
Thanks @richiemcilroy for the clarification regarding |
Closes #73
/claim #73
Summary
Implements the startup upload health check and upload speed probe for the Cap desktop app (Tauri v2 + SolidJS), automatically scaling Instant recording resolution (with_max_output_size) based on measured upload bandwidth while strictly preventing speed probes from running during active or pending recordings.
Key Changes
efresh_upload_health_status Tauri commands + UploadHealthUpdated event.
eceivedBytes == PROBE_PAYLOAD_BYTES before computing upload_mbps.
oute.ts):
Fix the Linux pending-recording cancellation and false storage-readiness result before merging, and satisfy the repository requirements.
Findings
Fix with agent prompt
Summary
Adds startup and periodic upload probes, speed-based Instant recording caps, and a desktop status badge.
Reviews (1) · Last reviewed commit: "feat(desktop): add upload speed test res..." · Reviewed by Greptile