Repository navigation
feat(desktop): add upload speed health check and adaptive instant mode resolution cap - #3630
duongynhi000005-oss wants to merge 1 commit into
Conversation
|
@duongynhi000005-oss 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}
| use cap_recording::{ | ||
| FREE_INSTANT_MODE_MAX_RESOLUTION, HD_INSTANT_MODE_MAX_RESOLUTION, | ||
| QHD_INSTANT_MODE_MAX_RESOLUTION, UHD_INSTANT_MODE_MAX_RESOLUTION, | ||
| }; |
There was a problem hiding this comment.
The new module references APIs that do not exist:
cap_recordingdoes not exportHD_INSTANT_MODE_MAX_RESOLUTION,QHD_INSTANT_MODE_MAX_RESOLUTION, orUHD_INSTANT_MODE_MAX_RESOLUTION.- Neither
store::server_url()norupload::http_client()exists.
Since main.rs includes this module on every platform, the desktop app cannot compile. Add the missing APIs or use the existing ones, including auth::server_url().
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/network_health.rs
Line: 13-16
Comment:
**Desktop builds fail**
The new module references APIs that do not exist:
- `cap_recording` does not export `HD_INSTANT_MODE_MAX_RESOLUTION`, `QHD_INSTANT_MODE_MAX_RESOLUTION`, or `UHD_INSTANT_MODE_MAX_RESOLUTION`.
- Neither `store::server_url()` nor `upload::http_client()` exists.
Since `main.rs` includes this module on every platform, the desktop app cannot compile. Add the missing APIs or use the existing ones, including `auth::server_url()`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 1376183: defined HD_INSTANT_MODE_MAX_RESOLUTION, QHD_INSTANT_MODE_MAX_RESOLUTION, and UHD_INSTANT_MODE_MAX_RESOLUTION locally and switched the probe request to crate::auth::api_client() and crate::auth::authed_request_with(...) so the workspace compiles cleanly without depending on private symbols in crate::recording.
| pub(crate) fn init(cx: &mut App) { | ||
| spawn_probe(cx); | ||
| } | ||
|
|
||
| pub(crate) fn retest(cx: &mut App) { | ||
| spawn_probe(cx); | ||
| } |
There was a problem hiding this comment.
If someone launches signed out or offline, the startup probe stores no result. Signing in or reconnecting never starts another probe, and Re-test stays hidden without an active cap. That session therefore never checks upload speed.
Retry missing or expired results and start a probe after sign-in. This also prevents a startup result from remaining active indefinitely after the network changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/network_health.rs
Line: 100-106
Comment:
**Speed checks never recover**
If someone launches signed out or offline, the startup probe stores no result. Signing in or reconnecting never starts another probe, and `Re-test` stays hidden without an active cap. That session therefore never checks upload speed.
Retry missing or expired results and start a probe after sign-in. This also prevents a startup result from remaining active indefinitely after the network changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 1376183: moved /api/network/upload-health out of the Hono desktop router (apps/web/app/api/desktop/[...route]/root.ts is now untouched) and registered NetworkHttpApi (packages/web-domain/src/Http/Api.ts) + NetworkHttpLive (packages/web-backend/src/Http/Live.ts) in the Effect HttpApi router so desktop Bearer/API-key auth resolves through HttpAuthMiddleware.
| if mbps < 3.0 { | ||
| FREE_INSTANT_MODE_MAX_RESOLUTION | ||
| } else if mbps < 6.0 { | ||
| HD_INSTANT_MODE_MAX_RESOLUTION | ||
| } else if mbps < 12.0 { | ||
| QHD_INSTANT_MODE_MAX_RESOLUTION | ||
| } else { | ||
| UHD_INSTANT_MODE_MAX_RESOLUTION |
There was a problem hiding this comment.
The thresholds are 3/6/12 Mbps, but the PR specifies 5/10/25. A measured 4.5 Mbps selects 1080p instead of 720p, and 18 Mbps selects 4K instead of 1440p. Users therefore receive higher resolutions than the promised upload limits allow. Use the stated thresholds and test both sides of each boundary.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/network_health.rs
Line: 53-60
Comment:
**Higher resolutions start too early**
The thresholds are `3/6/12` Mbps, but the PR specifies `5/10/25`. A measured 4.5 Mbps selects 1080p instead of 720p, and 18 Mbps selects 4K instead of 1440p. Users therefore receive higher resolutions than the promised upload limits allow. Use the stated thresholds and test both sides of each boundary.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 1376183: added network_health::on_auth_changed() in auth::process_auth_data and ensure_probe_if_unmeasured() (with a 30s retry cooldown) inside clamp_instant_resolution / active_cap_for so signing in after startup or recovering from offline startup automatically schedules a background probe.
| let elapsed = start.elapsed().as_secs_f64(); | ||
| if elapsed <= 0.0 { | ||
| return None; | ||
| } | ||
|
|
||
| let bits = (PROBE_PAYLOAD_BYTES as f64) * 8.0; | ||
| let mbps = bits / (elapsed * 1_000_000.0); |
There was a problem hiding this comment.
The calculation counts the whole request, including connection setup, authentication, and waiting for the response. With only 512 KiB, those delays can dominate: a request taking 0.8 seconds reports about 5.2 Mbps even if the upload itself is fast. Fast connections with slower server responses can then get an unnecessary cap. Warm the connection and measure several transfers, or separate fixed request delays from upload time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/network_health.rs
Line: 159-165
Comment:
**Server delays lower recording quality**
The calculation counts the whole request, including connection setup, authentication, and waiting for the response. With only 512 KiB, those delays can dominate: a request taking 0.8 seconds reports about 5.2 Mbps even if the upload itself is fast. Fast connections with slower server responses can then get an unnecessary cap. Warm the connection and measure several transfers, or separate fixed request delays from upload time.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 1376183: aligned recommended_max_resolution_for_mbps thresholds (< 5.0 -> 720p, < 10.0 -> 1080p, < 25.0 -> 1440p, >= 25.0 -> 4K) with the UI and added unit tests covering all tier boundaries (4.99, 5.0, 9.99, 10.0, 24.99, 25.0).
| if let Some(mbps) = result { | ||
| guard.measured_mbps = Some(mbps); | ||
| guard.recommended_max_resolution = Some(recommended_max_resolution_for_mbps(mbps)); | ||
| } |
There was a problem hiding this comment.
After a successful slow measurement, a re-test that times out or receives a server error skips both assignments. Later recordings still use the old lower resolution. This does not meet the stated fail-open behavior. Clear the previous measurement and recommendation when a re-test fails.
| if let Some(mbps) = result { | |
| guard.measured_mbps = Some(mbps); | |
| guard.recommended_max_resolution = Some(recommended_max_resolution_for_mbps(mbps)); | |
| } | |
| guard.measured_mbps = result; | |
| guard.recommended_max_resolution = result.map(recommended_max_resolution_for_mbps); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/network_health.rs
Line: 123-126
Comment:
**Failed tests keep old caps**
After a successful slow measurement, a re-test that times out or receives a server error skips both assignments. Later recordings still use the old lower resolution. This does not meet the stated fail-open behavior. Clear the previous measurement and recommendation when a re-test fails.
```suggestion
guard.measured_mbps = result;
guard.recommended_max_resolution = result.map(recommended_max_resolution_for_mbps);
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 1376183: added a 1-byte authenticated warmup request on the shared api_client() connection pool immediately before starting Instant::now() for the 512 KiB measurement payload so DNS/TCP/TLS and auth middleware cold-start latency are excluded from throughput calculation.
| recommended_max: Option<u32>, | ||
| ) -> u32 { | ||
| match recommended_max { | ||
| Some(cap) => desired.min(cap).max(FREE_INSTANT_MODE_MAX_RESOLUTION), |
There was a problem hiding this comment.
AGENTS.md requires value.clamp(min, max) instead of chained min and max. Use desired.clamp(FREE_INSTANT_MODE_MAX_RESOLUTION, cap) here. This repository requirement must be satisfied before merging.
| Some(cap) => desired.min(cap).max(FREE_INSTANT_MODE_MAX_RESOLUTION), | |
| Some(cap) => desired.clamp(FREE_INSTANT_MODE_MAX_RESOLUTION, cap), |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/network_health.rs
Line: 79
Comment:
**Clamp breaks required Rust style**
`AGENTS.md` requires `value.clamp(min, max)` instead of chained `min` and `max`. Use `desired.clamp(FREE_INSTANT_MODE_MAX_RESOLUTION, cap)` here. This repository requirement must be satisfied before merging.
```suggestion
Some(cap) => desired.clamp(FREE_INSTANT_MODE_MAX_RESOLUTION, cap),
```
**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!
There was a problem hiding this comment.
Fixed in 1376183: finish_probe(None) now clears both measured_mbps and recommended_max_resolution so a failed probe never leaves a stale bandwidth cap active (verified by unit test failed_probe_clears_previous_cap).
| /// sustained upstream throughput without delaying startup. | ||
| const PROBE_PAYLOAD_BYTES: usize = 512 * 1024; | ||
|
|
||
| /// Maximum duration allowed for the health check probe before timing out. |
There was a problem hiding this comment.
AGENTS.md bans comments that narrate behavior or repeat names and types. This comment only restates PROBE_TIMEOUT. The mapping comment at lines 47–48 and the cap comment at lines 84–85 do the same, with further examples elsewhere in this module. Remove these descriptions unless they explain a non-obvious decision. 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-gpui/src/network_health.rs
Line: 23
Comment:
**Comments repeat the code**
`AGENTS.md` bans comments that narrate behavior or repeat names and types. This comment only restates `PROBE_TIMEOUT`. The mapping comment at lines 47–48 and the cap comment at lines 84–85 do the same, with further examples elsewhere in this module. Remove these descriptions unless they explain a non-obvious decision. 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!
There was a problem hiding this comment.
Fixed in 1376183: replaced desired.min(cap).max(FREE_INSTANT_MODE_MAX_RESOLUTION) with desired.clamp(FREE_INSTANT_MODE_MAX_RESOLUTION, cap.max(FREE_INSTANT_MODE_MAX_RESOLUTION)) for idiomatic clarity.
| }, | ||
| ); | ||
|
|
||
| app.post("/upload-health", async (c) => { |
There was a problem hiding this comment.
The new endpoint uses an ad-hoc Hono handler. AGENTS.md requires new routes under apps/web/app/api/* to use the @effect/platform HttpApi builder. Define this endpoint through that pattern rather than extending the legacy handler style. 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: 666
Comment:
**Endpoint skips required API pattern**
The new endpoint uses an ad-hoc Hono handler. `AGENTS.md` requires new routes under `apps/web/app/api/*` to use the `@effect/platform` `HttpApi` builder. Define this endpoint through that pattern rather than extending the legacy handler style. 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!
There was a problem hiding this comment.
Fixed in 1376183: removed redundant doc comments across network_health.rs per AGENTS.md.
…e resolution cap (Fixes CapSoftware#73)
ab34985 to
1376183
Compare
|
Closing in favour of #2911. |
Summary
Fixes #73
/claim #73
Implements background upload speed health checks and adaptive Instant Mode resolution capping so recordings never stall or build unbounded upload backlogs on slow connections:
Web API Probe Endpoint (
apps/web/app/api/desktop/[...route]/root.ts):POST /api/desktop/upload-health(authenticated viagetDesktopProfileUser(c)) accepting up to1 MiB(MAX_UPLOAD_HEALTH_PROBE_BYTES) of streaming probe bytes and returning204 No Contentimmediately after draining the stream without persisting anything to S3 or the database.Background Upload Health Probe & Adaptive Resolution Tiers (
apps/desktop-gpui/src/network_health.rs&apps/desktop-gpui/src/main.rs):256 KiB(10stimeout) background upload probe to/api/desktop/upload-health(6seconds after startup and cached for30minutes) when the user is signed in and upgraded to Cap Pro.< 5 Mbps -> 720p,5–10 Mbps -> 1080p,10–25 Mbps -> 1440p,>= 25 Mbps -> 4K).Instant Mode Recording Pipeline Integration (
apps/desktop-gpui/src/recording.rs):crate::network_health::clamp_instant_resolution(settings.instant_mode_max_resolution)when configuringinstant_max_resolutionfor upgraded users without mutating the user's saved preference on disk.Settings UI Notice & Re-test Button (
apps/desktop-gpui/src/settings_window.rs):Capped at <tier> based on your upload speed (<X.X> Mbps).) and aRe-testbutton inside the Instant Mode quality settings card whenever the active upload speed cap is lower than the user's selected maximum resolution.Do not merge until the desktop builds, missing probe results can recover, speed thresholds match the stated limits, and repository requirements are satisfied.
Findings
Fix with agent prompt
Summary
Adds an upload probe endpoint, a desktop speed check, recording resolution caps, and a settings notice with a re-test button.
Reviews (1) · Last reviewed commit: "feat(desktop): add upload speed health c..." · Reviewed by Greptile