Repository navigation
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const response = await fetch(targetUrl, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: testPayload, | ||
| signal: controller.signal, | ||
| }); |
There was a problem hiding this comment.
Both checks use browser fetch, but the packaged desktop app's connect-src excludes the default server, https://cap.so. The requests are blocked, so users see “Upload Error” and never get a speed result even when recording uploads work. Use native fetch from @tauri-apps/plugin-http, as web-api.ts does. This also avoids the development CORS mismatch for http://localhost:3002.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.ts
Line: 160-167
Comment:
**Desktop checks cannot connect**
Both checks use browser `fetch`, but the packaged desktop app's `connect-src` excludes the default server, `https://cap.so`. The requests are blocked, so users see “Upload Error” and never get a speed result even when recording uploads work. Use native `fetch` from `@tauri-apps/plugin-http`, as `web-api.ts` does. This also avoids the development CORS mismatch for `http://localhost:3002`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| try { | ||
| const { generalSettingsStore } = await import("~/store"); | ||
| await generalSettingsStore.set({ | ||
| instantModeMaxResolution: recommendedResolution, | ||
| }); |
There was a problem hiding this comment.
Every successful test replaces the user's saved instantModeMaxResolution, including while Studio mode is selected. A Pro user who chooses 4K loses that choice at the next test, at most 45 seconds later. Keep the recommendation separate from the saved preference, and apply it only through an explicit automatic-quality setting.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.ts
Line: 264-268
Comment:
**Saved quality gets replaced**
Every successful test replaces the user's saved `instantModeMaxResolution`, including while Studio mode is selected. A Pro user who chooses 4K loses that choice at the next test, at most 45 seconds later. Keep the recommendation separate from the saved preference, and apply it only through an explicit automatic-quality setting.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const startTime = performance.now(); | ||
| const response = await fetch(targetUrl, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/octet-stream", | ||
| }, | ||
| body: buffer, | ||
| signal: controller.signal, | ||
| }); | ||
|
|
||
| const durationMs = performance.now() - startTime; |
There was a problem hiding this comment.
Timing one 256 KiB request measures connection setup, server delay, and the return trip as well as upload time. A 100 Mbps upload with 200 ms of other delay reads roughly 9.5 Mbps and gets the medium setting instead of high. Because this result changes recording quality, use a warmed-up, longer upload sample or repeated samples that reduce the effect of fixed delays.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.ts
Line: 231-241
Comment:
**Delay lowers recording quality**
Timing one 256 KiB request measures connection setup, server delay, and the return trip as well as upload time. A 100 Mbps upload with 200 ms of other delay reads roughly 9.5 Mbps and gets the medium setting instead of high. Because this result changes recording quality, use a warmed-up, longer upload sample or repeated samples that reduce the effect of fixed delays.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let isRecordingActive = false; | ||
| let activeSpeedTestAbort: AbortController | null = null; | ||
|
|
||
| export function setRecordingState(active: boolean) { | ||
| isRecordingActive = active; |
There was a problem hiding this comment.
isRecordingActive is a plain variable, so disabled={isRecording()} does not update when recording starts or stops. The main indicator reads no signal in that expression, so changes to other parts of the indicator do not repair it either. Store recording state in a Solid signal so both buttons track the recording state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.ts
Line: 85-89
Comment:
**Buttons miss recording changes**
`isRecordingActive` is a plain variable, so `disabled={isRecording()}` does not update when recording starts or stops. The main indicator reads no signal in that expression, so changes to other parts of the indicator do not repair it either. Store recording state in a Solid signal so both buttons track the recording state.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const qualityLabel = () => { | ||
| const tier = speedTest().qualityTier; | ||
| const res = speedTest().recommendedResolution; | ||
| if (!tier) return "Standard (1080p)"; | ||
| if (tier === "high") return `Full (${res}p)`; | ||
| if (tier === "medium") return `Standard (${res}p)`; | ||
| return `Adapted (${res}p)`; |
There was a problem hiding this comment.
recommendedResolution is a width limit, not a video height. Appending p therefore shows “1920p” for a 1080p limit and “2160p” for a limit of roughly 1215 pixels high. Use the same width-to-label mapping as the quality settings, and choose presets consistent with those labels.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/components/NetworkHealthIndicator.tsx
Line: 40-46
Comment:
**Quality labels overstate output**
`recommendedResolution` is a width limit, not a video height. Appending `p` therefore shows “1920p” for a 1080p limit and “2160p” for a limit of roughly 1215 pixels high. Use the same width-to-label mapping as the quality settings, and choose presets consistent with those labels.
---
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!
| const speedLabel = () => { | ||
| const mbps = speedTest().speedMbps; | ||
| if (mbps === null) return "Checking..."; | ||
| return `${mbps} Mbps`; |
There was a problem hiding this comment.
When the first speed test fails but the health check succeeds, speedMbps remains null and speedLabel() keeps returning “Checking...”. The tooltip still says “Operational”, and no view displays speedTest.error. Render the error state separately from a running test, including when an older speed result exists.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/components/NetworkHealthIndicator.tsx
Line: 34-37
Comment:
**Failed tests look unfinished**
When the first speed test fails but the health check succeeds, `speedMbps` remains null and `speedLabel()` keeps returning “Checking...”. The tooltip still says “Operational”, and no view displays `speedTest.error`. Render the error state separately from a running test, including when an older speed result exists.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const json = await c.req.json().catch(() => ({})); | ||
| const payload = typeof json.payload === "string" ? json.payload : ""; | ||
| bytesReceived = Buffer.byteLength(payload, "utf8"); | ||
| } else { | ||
| const bodyBuffer = await c.req | ||
| .arrayBuffer() | ||
| .catch(() => new ArrayBuffer(0)); |
There was a problem hiding this comment.
The POST handlers turn body-read failures into empty bodies and still return success. Invalid JSON therefore returns healthy: true, and a failed speed-test body read returns HTTP 200 with zero bytes. Let those failures reach the error response, and have the client verify bytesReceived before accepting the speed result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/desktop/[...route]/health.ts
Line: 20-26
Comment:
**Failed reads count as success**
The POST handlers turn body-read failures into empty bodies and still return success. Invalid JSON therefore returns `healthy: true`, and a failed speed-test body read returns HTTP 200 with zero bytes. Let those failures reach the error response, and have the client verify `bytesReceived` before accepting the speed result.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| export const app = new Hono().use(withOptionalAuth); | ||
|
|
||
| app.get("/", (c) => { |
There was a problem hiding this comment.
The new health API uses Hono handlers. The repository guide requires new Next.js APIs under apps/web/app/api/* to use the @effect/platform HttpApi builder rather than ad-hoc handlers. Implement these endpoints with that pattern and its shared handler wiring. This repository requirement must be satisfied 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]/health.ts
Line: 4-6
Comment:
**New API skips required pattern**
The new health API uses `Hono` handlers. The repository guide requires new Next.js APIs under `apps/web/app/api/*` to use the `@effect/platform` `HttpApi` builder rather than ad-hoc handlers. Implement these endpoints with that pattern and its shared handler wiring. This repository requirement must be satisfied 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!
| @@ -0,0 +1,133 @@ | |||
| import { cx } from "cva"; | |||
There was a problem hiding this comment.
The new file NetworkHealthIndicator.tsx uses a PascalCase filename. The repository guide requires kebab-case filenames while keeping component names PascalCase. Rename the file to network-health-indicator.tsx and update its import. 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/components/NetworkHealthIndicator.tsx
Line: 1
Comment:
**Filename breaks repository convention**
The new file `NetworkHealthIndicator.tsx` uses a PascalCase filename. The repository guide requires kebab-case filenames while keeping component names PascalCase. Rename the file to `network-health-indicator.tsx` and update its import. 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!
| signalFactory = solid.createSignal; | ||
| } | ||
| } catch { | ||
| // Fallback to standalone reactive signal |
There was a problem hiding this comment.
The new “Fallback to standalone reactive signal” comment merely narrates the catch branch, which the repository's comments policy forbids. It is also inaccurate: createFallbackSignal stores a plain variable and cannot notify Solid when it changes. Remove the comment rather than describing the fallback as reactive. This repository requirement must be satisfied before merging.
| // Fallback to standalone reactive signal |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.ts
Line: 58
Comment:
**Comment promises nonexistent updates**
The new “Fallback to standalone reactive signal” comment merely narrates the catch branch, which the repository's comments policy forbids. It is also inaccurate: `createFallbackSignal` stores a plain variable and cannot notify Solid when it changes. Remove the comment rather than describing the fallback as reactive. This repository requirement must be satisfied before merging.
```suggestion
```
**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!
…reactive indicators
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| }); | ||
|
|
||
| const SpeedTestPayload = Schema.Struct({ | ||
| payload: Schema.String.pipe(Schema.minLength(1)), |
There was a problem hiding this comment.
P2: The public speed-test endpoint accepts unbounded request bodies without rate limiting.
Public POST accepts an unbounded string and scans it for byte length, with no route auth or rate limit.
Enforce a strict body-size limit and per-IP rate limit; require auth if anonymous speed tests are unnecessary.
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/desktop/health/route.ts">
<violation number="1" location="apps/web/app/api/desktop/health/route.ts:17">
<priority>P2</priority>
<title>The public speed-test endpoint accepts unbounded request bodies without rate limiting.</title>
<evidence>The new POST `/api/desktop/health` endpoint accepts `payload` as `Schema.String.pipe(Schema.minLength(1))` (line 17), with no maximum length or byte limit. Its handler then computes `Buffer.byteLength(payload.payload, "utf8")` (line 53), so request parsing and processing scale with attacker-supplied body size. The route is exported directly through `apiToHandler` (lines 64–67) and does not attach `HttpAuthMiddleware` or a rate limiter. `apiToHandler` provides the auth middleware implementation but does not globally require it (`apps/web/lib/server.ts:99-117`); protected endpoint examples explicitly opt in with `.middleware(HttpAuthMiddleware)` (`apps/web/app/api/recording/prepare/route.ts:21-41`). Thus an unauthenticated remote caller can repeatedly submit large POST bodies and consume request-processing memory and CPU. The base checkout's catch-all API route delegates only to existing `HttpLive` routes (`repository-base/apps/web/app/api/[[...route]]/route.ts:1-14`); a search of the base checkout found no equivalent desktop health/speed-test endpoint. The new route therefore adds this public, attacker-controlled parsing and byte-counting path.</evidence>
<recommendation>Set and enforce a strict maximum payload byte length (and reject oversized requests before fully parsing/retaining them), and apply an explicit per-IP rate limit to this unauthenticated endpoint. If speed testing does not need anonymous access, require the appropriate authentication middleware.</recommendation>
</violation>
</file>
|
@greptileai please re-review the latest head. |
…health on recovery
|
@greptileai please re-review. I applied both remaining changes by warming up the connection and using repeated samples with latency subtraction to avoid fixed delays skewing the speed test, and updating network health so successful tests properly clear any previous offline error status. |
| if (networkHealth().status !== "healthy") { | ||
| void runUploadHealthCheck(); | ||
| } | ||
| void runSpeedTest(); |
There was a problem hiding this comment.
The periodic retry starts runUploadHealthCheck() and runSpeedTest() together. If the GET stalls while the POST samples succeed, its later timeout replaces the recovered networkHealth with "unhealthy". Users then see “Upload Error” despite the successful upload.
Run these checks in order, or prevent an older failed check from replacing a newer success.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.ts
Line: 346-349
Comment:
**Late checks undo recovery**
The periodic retry starts `runUploadHealthCheck()` and `runSpeedTest()` together. If the GET stalls while the POST samples succeed, its later timeout replaces the recovered `networkHealth` with `"unhealthy"`. Users then see “Upload Error” despite the successful upload.
Run these checks in order, or prevent an older failed check from replacing a newer success.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| expect(speedTest().status).toBe("completed"); | ||
| expect(speedTest().error).toBeNull(); | ||
|
|
||
| // Verify network health was restored and error cleared |
There was a problem hiding this comment.
The new comments narrate the setup and assertions. The repository guide requires comments to explain non-obvious context, not repeat what the code does. Remove these comments before merging to satisfy that requirement. The same pattern appears at lines 122 and 158, and elsewhere in this test.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.test.ts
Line: 154
Comment:
**Comments repeat the test**
The new comments narrate the setup and assertions. The repository guide requires comments to explain non-obvious context, not repeat what the code does. Remove these comments before merging to satisfy that requirement. The same pattern appears at lines 122 and 158, and elsewhere in this test.
**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!
…nd remove test narrative comments
|
@greptileai please re-review. I resolved both items by sequencing the health check and speed test with timestamp guards to prevent late responses from overwriting a recovered status, and cleaned up the test comments to follow the repo guidelines. |
| await runSpeedTest(); | ||
| expect(networkHealth().status).toBe("healthy"); | ||
|
|
||
| delayedReject?.(new Error("Late network timeout")); |
There was a problem hiding this comment.
delayedReject?.(...) fails strict TypeScript checking. delayedReject starts as null, and assigning it inside the Promise callback does not change its type at this call site. TypeScript treats the callable value as never. The desktop's tsconfig.json includes this test, so it breaks typechecking. Capture the reject function with a definite-assignment declaration instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.test.ts
Line: 198
Comment:
**New test breaks typechecking**
`delayedReject?.(...)` fails strict TypeScript checking. `delayedReject` starts as `null`, and assigning it inside the Promise callback does not change its type at this call site. TypeScript treats the callable value as `never`. The desktop's `tsconfig.json` includes this test, so it breaks typechecking. Capture the reject function with a definite-assignment declaration instead.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| globalThis.fetch = vi.fn().mockReturnValue(delayedPromise); | ||
| const pendingCheck = runUploadHealthCheck(); | ||
|
|
||
| globalThis.fetch = vi |
There was a problem hiding this comment.
The test replaces globalThis.fetch before the pending health check uses it. runUploadHealthCheck() first awaits getServerUrl(), then reads the fetch function. It therefore uses the replacement mock, not delayedPromise. Rejecting that promise does not produce the late health-check failure this test should cover, leaving the race untested. Wait until the GET has entered the delayed mock before replacing it, and assert that the check stays pending until rejection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.test.ts
Line: 167-170
Comment:
**Test misses the delayed request**
The test replaces `globalThis.fetch` before the pending health check uses it. `runUploadHealthCheck()` first awaits `getServerUrl()`, then reads the fetch function. It therefore uses the replacement mock, not `delayedPromise`. Rejecting that promise does not produce the late health-check failure this test should cover, leaving the race untested. Wait until the GET has entered the delayed mock before replacing it, and assert that the check stays pending until rejection.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| await runUploadHealthCheck(); | ||
| if (!isRecordingSignal()) { | ||
| await runSpeedTest(); |
There was a problem hiding this comment.
If the page leaves while runUploadHealthCheck() is pending, cleanup clears the interval and aborts any current speed test. Once the health check finishes, this callback starts a new speed test anyway. Both startup and an already-running interval callback can upload roughly 1 MiB after monitoring has stopped. Add a stopped flag and check it after the await, before calling runSpeedTest().
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/network-health.ts
Line: 346-348
Comment:
**Uploads start after cleanup**
If the page leaves while `runUploadHealthCheck()` is pending, cleanup clears the interval and aborts any current speed test. Once the health check finishes, this callback starts a new speed test anyway. Both startup and an already-running interval callback can upload roughly 1 MiB after monitoring has stopped. Add a stopped flag and check it after the await, before calling `runSpeedTest()`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…outing, and enforce length check
|
@greptileai please re-review. I resolved all three issues |
This branch has not been deployed
1 participant
Closes #73
/claim #73
Summary
Implements an upload speed test and health check for Cap desktop in instant mode:
/api/desktop/health./api/desktop/healthand/api/desktop/health/speed-test) that stream and count bytes in memory without writing files to storage.
The PR appears safe to merge; no blocking findings remain.Summary
Adds a desktop network indicator, upload-speed recommendations, and health endpoints.
Reviews (5) · Last reviewed commit: "fix(desktop): add stopped guard to healt..." · Reviewed by Greptile