feat(desktop): upload speed test and health check for Instant mode - #2322
Open
stffinfcti wants to merge 3 commits into
Open
stffinfcti wants to merge 3 commits into
stffinfcti wants to merge 3 commits into
Conversation
Adds a small status indicator in the bottom-right of the main window that measures upload throughput and reports upload health: - New authed endpoint POST /api/desktop/upload-health accepts a bounded (<=1MiB) probe body and echoes receivedBytes so the client can verify the upload was not truncated; GET acts as a liveness check. - A desktop UploadHealthMonitor probes on startup and on demand, measures Mbps from a 512KiB upload (minus one measured RTT), and emits uploadHealthChanged events to the frontend. - Probes never run during a recording: a fresh probe refuses to start while recording is pending/active and any in-flight probe is aborted the moment recording begins. - Instant mode recordings cap their max output width by the measured upload speed (<4 Mbps -> 1280, <10 -> 1920, <25 -> 2560) when a fresh measurement exists. - On failure the indicator flags the problem and links to support. Closes CapSoftware#73
- restore macOS SystemDiagnostics/MacOSVersionInfo in the committed bindings; they had drifted to the Linux shape after regeneration - a 404 from /api/desktop/upload-health now maps to the hidden 'unknown' state so older self-hosted web builds don't raise a false alarm - wire refresh_upload_health to a retry button on the failed indicator Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+205
to
+210
| if recording_in_progress(app).await { | ||
| return false; | ||
| } | ||
| match crate::auth::AuthStore::get(app) { | ||
| Ok(Some(_)) => monitor.start_probe(app), | ||
| _ => false, |
Contributor
There was a problem hiding this comment.
begin_probe releases the app read lock after checking the recording state, then installs the probe under a separate lock. If a recording becomes pending between those operations, recording_started finds no probe to cancel, and the later active transition does not retry cancellation. The 512 KiB probe can therefore run during an Instant recording and compete with its live upload.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/upload_health.rs
Line: 205-210
Comment:
**Probe can outlive cancellation**
`begin_probe` releases the app read lock after checking the recording state, then installs the probe under a separate lock. If a recording becomes pending between those operations, `recording_started` finds no probe to cancel, and the later active transition does not retry cancellation. The 512 KiB probe can therefore run during an Instant recording and compete with its live upload.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+326
to
+331
| match inner.status.state { | ||
| UploadHealthState::Healthy | UploadHealthState::Degraded => { | ||
| inner.status.upload_mbps.and_then(recommended_max_width) | ||
| } | ||
| _ => None, | ||
| } |
Contributor
There was a problem hiding this comment.
A failed recheck retains the last measured upload_mbps, but changes the state to failed and marks the result fresh for ten minutes. This branch only accepts healthy or degraded, so a transient failure prevents that retained slow measurement from limiting subsequent Instant recordings. Those recordings can use the user's higher resolution until another probe runs or the cache expires.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/upload_health.rs
Line: 326-331
Comment:
**Failures disable cached cap**
A failed recheck retains the last measured `upload_mbps`, but changes the state to `failed` and marks the result fresh for ten minutes. This branch only accepts `healthy` or `degraded`, so a transient failure prevents that retained slow measurement from limiting subsequent Instant recordings. Those recordings can use the user's higher resolution until another probe runs or the cache expires.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| @@ -0,0 +1,130 @@ | |||
| import * as shell from "@tauri-apps/plugin-shell"; | |||
Contributor
There was a problem hiding this comment.
The repository naming directive requires TypeScript component files to use kebab-case while keeping the component symbol PascalCase. This new file is named UploadHealthIndicator.tsx; it must be renamed and its import updated before merging.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/components/UploadHealthIndicator.tsx
Line: 1
Comment:
**Component filename violates convention**
The repository naming directive requires TypeScript component files to use kebab-case while keeping the component symbol PascalCase. This new file is named `UploadHealthIndicator.tsx`; it must be renamed and its import updated before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/capsoftware/cap/blob/main/CLAUDE.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!
Comment on lines
+1
to
+35
| import { Hono } from "hono"; | ||
| import { withAuth } from "../../utils"; | ||
|
|
||
| export const app = new Hono().use(withAuth); | ||
|
|
||
| export const MAX_UPLOAD_PROBE_BYTES = 1024 * 1024; | ||
|
|
||
| app.get("/", (c) => c.json({ ok: true })); | ||
|
|
||
| app.post("/", async (c) => { | ||
| const contentLength = Number(c.req.header("content-length") ?? 0); | ||
| if (contentLength > MAX_UPLOAD_PROBE_BYTES) | ||
| return c.json({ error: "Probe payload too large" }, { status: 413 }); | ||
|
|
||
| const body = c.req.raw.body; | ||
| if (body === null) return c.json({ receivedBytes: 0 }); | ||
|
|
||
| let receivedBytes = 0; | ||
| const reader = body.getReader(); | ||
| try { | ||
| for (;;) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
| receivedBytes += value.byteLength; | ||
| if (receivedBytes > MAX_UPLOAD_PROBE_BYTES) { | ||
| await reader.cancel(); | ||
| return c.json({ error: "Probe payload too large" }, { status: 413 }); | ||
| } | ||
| } | ||
| } finally { | ||
| reader.releaseLock(); | ||
| } | ||
|
|
||
| return c.json({ receivedBytes }); | ||
| }); |
Contributor
There was a problem hiding this comment.
The repository directive requires new Next.js endpoints under apps/web/app/api/* to use the @effect/platform HttpApi builder and prohibits ad-hoc handlers. This endpoint introduces a standalone Hono handler instead, so it must be moved to the prescribed API group and layer pattern before merging.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/desktop/[...route]/upload-health.ts
Line: 1-35
Comment:
**Route bypasses required pattern**
The repository directive requires new Next.js endpoints under `apps/web/app/api/*` to use the `@effect/platform` `HttpApi` builder and prohibits ad-hoc handlers. This endpoint introduces a standalone Hono handler instead, so it must be moved to the prescribed API group and layer pattern before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/capsoftware/cap/blob/main/CLAUDE.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!
- begin_probe: hold the App read lock across the recording check and probe install so a pending transition (which always runs recording_started under the write lock) can no longer slip between them and let a probe run into a recording - instant_resolution_cap: a failed recheck retains the last measured speed, so keep enforcing it instead of letting a transient error lift the cap - rename UploadHealthIndicator.tsx to upload-health-indicator.tsx per the kebab-case filename convention - move /api/desktop/upload-health off the ad-hoc Hono handler onto the @effect/platform HttpApi group/layer pattern (DesktopApiContract + HttpApiBuilder + apiToHandler), preserving auth, the 413 byte cap, and early-abort streaming semantics Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the speed test and health check from #73 for the Tauri v2 desktop app.
POST /api/desktop/upload-health, which streams and counts request bytes (hard-capped at 1 MiB) and returnsreceivedBytes. Nothing is written to object storage, so probes leave no orphaned files.GETon the same route covers reachability/auth. A 404 is treated as "endpoint not deployed" rather than a failure.start_recordingappliesmin(user setting, recommended)for Instant mode only.UploadHealthIndicatorin the main window shows checking / healthy / degraded / failed with the measured Mbps and a manual re-check.Why this approach
Existing submissions leave gaps this avoids: probes awaited inline before the recording UI appears (stalling Instant mode for the full probe timeout), payloads written through signed S3 URLs that orphan objects in storage, unauthenticated probe endpoints accepting arbitrary bodies, recording-state flags that stay stuck after a failed stop, and in-flight probes that keep running after recording starts. Here the probe only ever runs in the background against a dedicated byte-counting route — authenticated, bounded, no storage writes — and recording start is a pure cache read plus cancellation.
Testing
cargo test -p cap-desktop upload_health --lib— 3 passed (tier mapping, checking-state reporting, failure preserving last measured speed)bunx biome checkon all touched TypeScript/TSX files — cleancargo fmt --all --check— clean@algora-pbc /claim #73
Closes #73
Bounty payout: @stffinfcti
This PR is not safe to merge until probe startup is synchronized with recording admission, retained measurements continue to gate quality after transient failures, and the explicit repository requirements are satisfied.
Findings
Fix with agent prompt
Summary
This PR adds an authenticated upload-health endpoint, a desktop upload-speed monitor and status indicator, and throughput-based Instant recording resolution caps.
Reviews (1) · Last reviewed commit: "fix(desktop): revert bindings drift, add..."