Repository navigation
Conversation
…tion tier, and startup health check (CapSoftware#73) - Add `upload_probe` module in `crates/recording` classifying upload throughput (`SpeedProbeSample`, `AdaptiveUploadTier`) into adaptive resolution caps (`1920`, `1280`, `854`) and `StudioQuality` presets while preventing speed probes during active recordings (`should_run_speed_probe`). - Extend `RecordingDefaults::with_upload_speed_mbps` to apply adaptive quality and resolution limits prior to recording startup. - Add `/api/upload/health-check` Web App API route (`apps/web/app/api/upload/health-check/route.ts`) and unit tests to validate startup screen-recording upload probes and surface user-facing support indicators on failure. Fixes CapSoftware#73 Signed-off-by: Lucas Oliveira <kalopsia3445@gmail.com>
|
@frds3445 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
| // Validates the desktop startup screen-recording upload probe payload and returns diagnostic telemetry. | ||
| export async function POST(request: Request): Promise<Response> { | ||
| try { | ||
| const buffer = await request.arrayBuffer() |
There was a problem hiding this comment.
P2: Unauthenticated upload health-check buffers arbitrary request bodies without a size or rate limit
Public POST handler buffers the entire request body with request.arrayBuffer() and applies no maximum size or rate limit.
Require auth, enforce a small max probe size from Content-Length or a bounded stream, and add per-IP rate limiting before buffering.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="apps/web/app/api/upload/health-check/route.ts">
<violation number="1" location="apps/web/app/api/upload/health-check/route.ts:14">
<priority>P2</priority>
<title>Unauthenticated upload health-check buffers arbitrary request bodies without a size or rate limit</title>
<evidence>The new POST handler calls `const buffer = await request.arrayBuffer()` at line 14 and only checks that the result is at least `MIN_PROBE_BYTES = 16` (lines 9 and 17). There is no maximum size, Content-Length validation, streamed read bound, authentication, or rate-limit call. The Next.js proxy matcher excludes all `/api` paths (`apps/web/proxy.ts:253`), so no middleware protects the route, and unlike the existing upload sub-routes (`new Hono().use(withAuth)` in `apps/web/app/api/upload/[...route]/multipart.ts:52` and `signed.ts:39`) this handler has no auth wrapper. The base checkout had no `health-check` route (`repository-base/apps/web/app/api/upload/[...route]/route.ts:11-17` mounts only comment-media, multipart, signed, and recording-complete), so this public body-consuming sink is newly reachable. An unauthenticated client can send large or chunked POST bodies repeatedly, causing the server process to allocate and process memory proportional to the supplied body. The repository already treats unauthenticated body/memory-consuming routes as DoS-sensitive and rate-limits them (for example `apps/web/lib/rate-limit.ts:155` documents memory DoS for the unauthenticated Loom route, and `apps/web/app/api/analytics/track/route.ts:55` applies `isRateLimited`).</evidence>
<recommendation>Require authentication for the health-check route (or prove the desktop client cannot be driven anonymously), enforce a small explicit maximum probe size by checking Content-Length and reading with a bounded stream, and add per-IP rate limiting through `isRateLimited`/the Vercel Firewall before any body is buffered. Reject oversized or chunked bodies without Content-Length instead of calling `request.arrayBuffer()` unconditionally.</recommendation>
</violation>
</file>
| pub fn with_upload_speed_mbps(mut self, mbps: f64) -> Self { | ||
| let tier = crate::upload_probe::AdaptiveUploadTier::from_mbps(mbps); | ||
| self.instant_mode_max_resolution = tier.max_instant_resolution(); | ||
| self.studio_recording_quality = tier.studio_quality(); | ||
| self |
There was a problem hiding this comment.
with_upload_speed_mbps has no callers, and nothing in the desktop app sends the new probe request or reads its result. Recording still uses saved settings without measuring upload speed, so users on slow connections never receive the advertised lower resolution. Wire the probe into startup and apply its recommendations when choosing recording settings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/defaults.rs
Line: 89-93
Comment:
**Upload checks never run**
`with_upload_speed_mbps` has no callers, and nothing in the desktop app sends the new probe request or reads its result. Recording still uses saved settings without measuring upload speed, so users on slow connections never receive the advertised lower resolution. Wire the probe into startup and apply its recommendations when choosing recording settings.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const healthy: UploadHealthCheckResponse = { | ||
| ok: true, | ||
| status: "healthy", | ||
| bytesReceived, | ||
| serverTimestampMs: Date.now(), | ||
| message: "Upload health check succeeded.", | ||
| } | ||
| return Response.json(healthy, { status: 200 }) |
There was a problem hiding this comment.
POST reports healthy for any body of at least 16 bytes without checking storage. A missing bucket, invalid storage credentials, or an unavailable storage host therefore gets the same success response as working uploads. Check the user's upload destination before reporting readiness; receiving bytes at the web server does not establish storage connectivity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/upload/health-check/route.ts
Line: 29-36
Comment:
**Broken uploads look healthy**
`POST` reports healthy for any body of at least 16 bytes without checking storage. A missing bucket, invalid storage credentials, or an unavailable storage host therefore gets the same success response as working uploads. Check the user's upload destination before reporting readiness; receiving bytes at the web server does not establish storage connectivity.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| export async function POST(request: Request): Promise<Response> { | ||
| try { | ||
| const buffer = await request.arrayBuffer() | ||
| const bytesReceived = buffer.byteLength | ||
|
|
||
| if (bytesReceived < MIN_PROBE_BYTES) { |
There was a problem hiding this comment.
Anonymous uploads can exhaust memory
If a self-hosted instance has no upstream body limit, an anonymous caller can send oversized bodies and exhaust the web server's memory. POST reads the entire request before checking its size and never enforces a maximum. Require authentication and stop reading once a small probe-size limit is reached; checking Content-Length alone would not cover streamed requests.
How this was verified: The public route buffers the complete body without authentication or an upper bound, while the shared request matcher excludes API routes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/upload/health-check/route.ts
Line: 12-17
Comment:
**Anonymous uploads can exhaust memory**
If a self-hosted instance has no upstream body limit, an anonymous caller can send oversized bodies and exhaust the web server's memory. `POST` reads the entire request before checking its size and never enforces a maximum. Require authentication and stop reading once a small probe-size limit is reached; checking `Content-Length` alone would not cover streamed requests.
**How this was verified:** The public route buffers the complete body without authentication or an upper bound, while the shared request matcher excludes API routes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const MIN_PROBE_BYTES = 16 | ||
|
|
||
| // Validates the desktop startup screen-recording upload probe payload and returns diagnostic telemetry. | ||
| export async function POST(request: Request): Promise<Response> { |
There was a problem hiding this comment.
Required API setup is bypassed
The new route exports standalone request handlers instead of using the required HttpApi endpoint pattern. AGENTS.md requires new API routes to use HttpApi and export the handler returned by apiToHandler(ApiLive). Move both methods into that pattern 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/upload/health-check/route.ts
Line: 12
Comment:
**Required API setup is bypassed**
The new route exports standalone request handlers instead of using the required `HttpApi` endpoint pattern. `AGENTS.md` requires new API routes to use `HttpApi` and export the handler returned by `apiToHandler(ApiLive)`. Move both methods into that pattern 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.| import { GET, POST } from "./route" | ||
|
|
||
| describe("Cap desktop upload health-check API route", () => { | ||
| it("accepts valid upload probe buffers and rejects undersized payloads", async () => { |
There was a problem hiding this comment.
New files break formatting rules
Both route.test.ts and route.ts use spaces for indentation. AGENTS.md explicitly requires tabs for new files and edits. Format both files with the repository's formatter 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/upload/health-check/route.test.ts
Line: 5
Comment:
**New files break formatting rules**
Both `route.test.ts` and `route.ts` use spaces for indentation. `AGENTS.md` explicitly requires tabs for new files and edits. Format both files with the repository's formatter 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!
| } | ||
|
|
||
| impl AdaptiveUploadTier { | ||
| // Classifies measured upload throughput in Mbps into an adaptive recording quality tier. |
There was a problem hiding this comment.
The new comments narrate what the following functions do. AGENTS.md bans comments that restate behavior and reserves them for non-obvious context. Remove these comments before merging. The same pattern appears above with_upload_speed_mbps and POST, and elsewhere in the new module.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/upload_probe.rs
Line: 25
Comment:
**Comments repeat the code**
The new comments narrate what the following functions do. `AGENTS.md` bans comments that restate behavior and reserves them for non-obvious context. Remove these comments before merging. The same pattern appears above `with_upload_speed_mbps` and `POST`, and elsewhere in the new module.
**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!
…-IP rate limiting
…ed probe Integrate /api/upload/health-check with @effect/platform HttpApi, HttpAuthMiddleware, and Storage readiness checks, and wire startup speed probing into desktop-gpui and RecordingDefaults.
Summary
Implements upload speed probing, adaptive recording resolution recommendations, and startup upload health verification for Cap (#73):
crates/recording/src/upload_probe.rs&defaults.rs: Probes effective upload throughput, classifies link quality, and recommends optimal resolution/FPS presets so recordings degrade gracefully on constrained connections instead of stalling uploads.apps/web/app/api/upload/health-check/route.ts&route.test.ts: Adds a lightweight authenticated upload health-check endpoint with unit tests verifying readiness and storage connectivity.Fixes #73
/claim #73
Do not merge until the desktop integration, storage check, unsafe body reader, and explicit repository requirements are addressed.
Findings
Fix with agent prompt
Summary
Adds upload-tier calculations, a method for applying them to recording defaults, and a web endpoint with unit tests.
Reviews (1) · Last reviewed commit: "feat(recording): add pre-recording uploa..." · Reviewed by Greptile