feat: add URL download speed test functionality - #240
Open
b1n23 wants to merge 4 commits into
Open
Conversation
- Implement multi-threaded URL download testing with 2/4/8 thread options - Add 9 preset global download servers (Cloudflare, OVH, Linode, Vultr, DataPacket) - Support custom URL input with validation - Add comprehensive HTTP headers for CDN compatibility - Extend database schema for URL download scheduling - Add TestTypeURLDownload constant and related types - Fix switch statement duplicate cases in speedtest handlers
Frontend improvements: - Add URL download test UI in ServerList component - Custom URL input with thread selection (2/4/8 threads) - Configurable timeout with default 30 seconds - Display URL download results in dashboard with proper server names - Add URL download scheduling support in ScheduleManager - Show test configuration (threads, timeout) in schedule display - Update SpeedHistoryChart to support URL download metrics - Fix field name consistency (useUrlDownload) UI enhancements: - Improve schedule display format: "hostname (X threads, Ys timeout) - URL Download" - Fix URL wrapping issues in schedule details - Update columns to display URL download server information
- Add comprehensive HTTP client timeouts to prevent goroutine leaks - Configure transport-level timeouts (dial, TLS handshake, response header) - Set client timeout slightly longer than context for graceful cancellation - Add connection pool management (MaxIdleConns, IdleConnTimeout) - Ensure all network phases have independent timeout protection This prevents goroutine leaks when HTTP connections hang or timeout, ensuring workers exit within timeout+15s maximum.
📝 WalkthroughWalkthroughAdds a new ChangesURL Download Execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Main
participant SpeedTestService
participant UrlDownloadRunner
participant HTTPDownloadServers
participant ResultHandler
Browser->>Main: Configure URL, threads, and timeout
Main->>SpeedTestService: Start url_download test
SpeedTestService->>UrlDownloadRunner: RunTest with options
UrlDownloadRunner->>HTTPDownloadServers: Execute concurrent HTTP downloads
HTTPDownloadServers-->>UrlDownloadRunner: Bytes, TTFB, and worker status
UrlDownloadRunner-->>SpeedTestService: Progress and result
SpeedTestService->>ResultHandler: SaveResult
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/speedtest/ScheduleManager.tsx (1)
284-330: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Create Schedule" isn't disabled/guarded for
url_downloadwithout a server or custom URL.
requiresServerSelectiononly covers"iperf"/"librespeed", so fortestType === "url_download"with no server selected and nocustomUrl,isMissingServer/isCreateDisabledstayfalseandhandleCreateSchedule's early guard (lines 323-329) never fires. The request still goes out and gets rejected server-side pervalidateScheduleServerIDs's"url_download schedules require a download URL or server ID"check, so there's no data-integrity risk, but the button/UX won't reflect the missing-input state the way it does for iperf3/LibreSpeed, and the user gets a raw backend error instead of the friendly inline message.🐛 Proposed fix
const requiresServerSelection = testType === "iperf" || testType === "librespeed"; const isMissingServer = requiresServerSelection && selectedServers.length === 0; + const isMissingUrlDownloadTarget = + testType === "url_download" && selectedServers.length === 0 && !customUrl?.trim(); const isMissingTime = scheduleType === "exact" && exactTimes.length === 0; - const isCreateDisabled = isMissingServer || isMissingTime; + const isCreateDisabled = isMissingServer || isMissingUrlDownloadTarget || isMissingTime;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/speedtest/ScheduleManager.tsx` around lines 284 - 330, Update the schedule validation around requiresServerSelection, isMissingServer, renderButtonContent, and handleCreateSchedule so url_download requires either a selected server or a non-empty customUrl. Preserve the existing iperf3/LibreSpeed behavior, expose the same disabled state and friendly prompt for missing URL/server input, and prevent submission before the backend request.
🧹 Nitpick comments (5)
internal/speedtest/url_download.go (2)
79-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThreads/timeout validation duplicated with
internal/database/schedule.go.The
2/4/8threads and1-300timeout checks here mirrorvalidateScheduleServerIDsininternal/database/schedule.go. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/speedtest/url_download.go` around lines 79 - 93, The download threads and timeout validation in the URL download setup duplicates the rules in validateScheduleServerIDs. Consolidate these shared constraints into a reusable validation symbol and update the validation flow here and in validateScheduleServerIDs to call it, preserving the existing defaults, accepted thread values, timeout range, and error behavior.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated URL-download validation rules (threads 2/4/8, timeout 1-300s) across two files. Extracting a shared validator would prevent the two copies from silently drifting apart if the constraints ever change.
internal/speedtest/url_download.go#L79-93: extract the threads/timeout checks into a shared helper (e.g.types.ValidateURLDownloadOptions(threads, timeout int) error) and call it here.internal/database/schedule.go#L28-42: call the same shared helper instead of re-implementing the2/4/8and1-300checks inline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/speedtest/url_download.go` at line 1, Extract the duplicated URL-download threads and timeout validation from the URL-download validation flow and the schedule validation flow into one shared helper, such as types.ValidateURLDownloadOptions(threads, timeout int) error. Replace both inline checks with calls to this helper, preserving the allowed thread values (2, 4, or 8) and timeout range (1–300 seconds).internal/database/schedule.go (1)
28-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates threads/timeout validation from
internal/speedtest/url_download.go.Same
2/4/8and1-300constraints are re-implemented inRunTest. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/schedule.go` around lines 28 - 42, Remove the duplicated DownloadThreads and DownloadTimeout validation from the schedule validation block, and reuse the existing validation from the URL-download implementation in internal/speedtest/url_download.go. Keep the download URL/server ID validation in place and ensure invalid thread or timeout values still produce the established validation errors through the shared path.web/src/types/types.ts (1)
32-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the new
TestTypeinstead of a duplicated literal union.
testTypehere re-declares the exact same four-value literal union thatTestType(lines 7-14) already expresses. Any future test type addition now needs to be kept in sync across two separate declarations.♻️ Proposed refactor
- testType: "speedtest" | "iperf3" | "librespeed" | "url_download"; + testType: TestType;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/types/types.ts` around lines 32 - 43, Update the SpeedTestResult.testType property to reference the existing TestType declaration from this types module instead of repeating the four-value literal union, preserving the same allowed values and keeping future additions centralized.web/src/components/speedtest/ServerList.tsx (1)
299-329: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse the shared server-fetch helper here.
web/src/api/speedtest.ts#getServersalready wraps this/servers?testType=...request, so reusing it would avoid duplicating the same fetch/error-handling logic inServerList.tsx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/speedtest/ServerList.tsx` around lines 299 - 329, Replace the duplicated request logic in fetchUrlDownloadServers with the shared getServers helper from web/src/api/speedtest.ts, passing the url_download test type and preserving the existing state update and error propagation behavior. Keep the useEffect’s toast handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/speedtest/url_download.go`:
- Around line 41-53: When resolving a builtin server in
UrlDownloadRunner.RunTest, capture server.Host in opts.ServerHost alongside
opts.ServerName. Update the builtin-IDC branch of SaveResult to assign
serverHost from opts.ServerHost instead of result.Server, preserving the real
hostname in stored results.
- Around line 104-331: The download worker coordination must not close errChan
until all workers have exited, and ttfb access must be synchronized. Update the
completion, timeout, and cancellation handling around done and errChan so
timeout paths stop or defer draining without closing the channel while workers
may still send; close it only after wg.Wait completes. Protect the ttfb write in
firstByteOnce and its later read with the existing mutex, or otherwise ensure
the read occurs after worker completion.
In `@web/src/components/Main.tsx`:
- Around line 309-323: Update the url_download progress initialization in
Main.tsx to set TestProgress.isUrlDownload to true alongside the existing type
flags, ensuring progress-display components can identify URL download tests.
- Around line 332-364: Remove the selected server host fallback from the
downloadUrl assignment in the speed test mutation. For url_download tests, use
only customUrl or selectedServers[0]?.url; when both are absent, leave
downloadUrl undefined so the backend can resolve the URL from server IDs.
---
Outside diff comments:
In `@web/src/components/speedtest/ScheduleManager.tsx`:
- Around line 284-330: Update the schedule validation around
requiresServerSelection, isMissingServer, renderButtonContent, and
handleCreateSchedule so url_download requires either a selected server or a
non-empty customUrl. Preserve the existing iperf3/LibreSpeed behavior, expose
the same disabled state and friendly prompt for missing URL/server input, and
prevent submission before the backend request.
---
Nitpick comments:
In `@internal/database/schedule.go`:
- Around line 28-42: Remove the duplicated DownloadThreads and DownloadTimeout
validation from the schedule validation block, and reuse the existing validation
from the URL-download implementation in internal/speedtest/url_download.go. Keep
the download URL/server ID validation in place and ensure invalid thread or
timeout values still produce the established validation errors through the
shared path.
In `@internal/speedtest/url_download.go`:
- Around line 79-93: The download threads and timeout validation in the URL
download setup duplicates the rules in validateScheduleServerIDs. Consolidate
these shared constraints into a reusable validation symbol and update the
validation flow here and in validateScheduleServerIDs to call it, preserving the
existing defaults, accepted thread values, timeout range, and error behavior.
- Line 1: Extract the duplicated URL-download threads and timeout validation
from the URL-download validation flow and the schedule validation flow into one
shared helper, such as types.ValidateURLDownloadOptions(threads, timeout int)
error. Replace both inline checks with calls to this helper, preserving the
allowed thread values (2, 4, or 8) and timeout range (1–300 seconds).
In `@web/src/components/speedtest/ServerList.tsx`:
- Around line 299-329: Replace the duplicated request logic in
fetchUrlDownloadServers with the shared getServers helper from
web/src/api/speedtest.ts, passing the url_download test type and preserving the
existing state update and error propagation behavior. Keep the useEffect’s toast
handling unchanged.
In `@web/src/types/types.ts`:
- Around line 32-43: Update the SpeedTestResult.testType property to reference
the existing TestType declaration from this types module instead of repeating
the four-value literal union, preserving the same allowed values and keeping
future additions centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a9e9b56-13bd-4911-a977-48788c0183c1
📒 Files selected for processing (15)
internal/database/schedule.gointernal/server/handlers.gointernal/speedtest/result_handler.gointernal/speedtest/speedtest.gointernal/speedtest/url_download.gointernal/speedtest/url_download_servers.gointernal/types/types.goweb/src/components/Main.tsxweb/src/components/speedtest/ScheduleManager.tsxweb/src/components/speedtest/ServerList.tsxweb/src/components/speedtest/SpeedHistoryChart.tsxweb/src/components/speedtest/SpeedTestTab.tsxweb/src/components/speedtest/columns.tsxweb/src/types/speedtest.tsweb/src/types/types.ts
Comment on lines
+104
to
+331
| startTime := time.Now() | ||
| var ttfb time.Duration | ||
| var totalBytes int64 | ||
| var contentLength int64 // Total expected size from Content-Length header | ||
| var mu sync.Mutex | ||
| var wg sync.WaitGroup | ||
| var firstByteOnce sync.Once | ||
| errChan := make(chan error, threads) | ||
|
|
||
| // Progress reporting goroutine | ||
| progressDone := make(chan struct{}) | ||
| go func() { | ||
| ticker := time.NewTicker(1 * time.Second) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-progressDone: | ||
| return | ||
| case <-ticker.C: | ||
| mu.Lock() | ||
| elapsed := time.Since(startTime).Seconds() | ||
| currentSpeed := 0.0 | ||
| if elapsed > 0 { | ||
| currentSpeed = float64(totalBytes) * 8 / elapsed / 1000000 // Mbps | ||
| } | ||
|
|
||
| // Calculate real progress | ||
| progress := 50.0 // Default fallback | ||
| if contentLength > 0 { | ||
| // Use actual download progress | ||
| progress = float64(totalBytes) / float64(contentLength) * 100.0 | ||
| if progress > 100.0 { | ||
| progress = 100.0 | ||
| } | ||
| } else { | ||
| // Estimate based on elapsed time vs timeout | ||
| timeProgress := (elapsed / float64(timeout)) * 100.0 | ||
| if timeProgress > 95.0 { | ||
| progress = 95.0 // Cap at 95% for time-based estimation | ||
| } else { | ||
| progress = timeProgress | ||
| } | ||
| } | ||
| mu.Unlock() | ||
|
|
||
| if r.progressCallback != nil { | ||
| r.progressCallback(types.SpeedUpdate{ | ||
| Type: "download", | ||
| ServerName: opts.ServerName, | ||
| Speed: currentSpeed, | ||
| Progress: progress, | ||
| IsComplete: false, | ||
| IsScheduled: opts.IsScheduled, | ||
| TestType: "url_download", | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| // Launch download workers | ||
| for i := 0; i < threads; i++ { | ||
| wg.Add(1) | ||
| go func(workerID int) { | ||
| defer wg.Done() | ||
|
|
||
| req, err := http.NewRequestWithContext(testCtx, "GET", downloadURL, nil) | ||
| if err != nil { | ||
| errChan <- fmt.Errorf("worker %d: failed to create request: %w", workerID, err) | ||
| return | ||
| } | ||
|
|
||
| // Set User-Agent to avoid 403 errors from CDNs like Cloudflare | ||
| req.Header.Set("User-Agent", "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36") | ||
| req.Header.Set("Accept", "*/*") | ||
| req.Header.Set("Accept-Encoding", "gzip, deflate") | ||
| req.Header.Set("Connection", "keep-alive") | ||
|
|
||
| // Set Referer header to the root URL (scheme + host) | ||
| if parsedURL, err := url.Parse(downloadURL); err == nil { | ||
| referer := fmt.Sprintf("%s://%s/", parsedURL.Scheme, parsedURL.Host) | ||
| req.Header.Set("Referer", referer) | ||
| } | ||
|
|
||
| // Configure HTTP client with timeouts to prevent goroutine leaks | ||
| // Client timeout is slightly longer than context to allow graceful cancellation | ||
| client := &http.Client{ | ||
| Timeout: time.Duration(timeout+5) * time.Second, | ||
| Transport: &http.Transport{ | ||
| DialContext: (&net.Dialer{ | ||
| Timeout: 10 * time.Second, | ||
| KeepAlive: 30 * time.Second, | ||
| }).DialContext, | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| ResponseHeaderTimeout: 10 * time.Second, | ||
| ExpectContinueTimeout: 1 * time.Second, | ||
| MaxIdleConns: 100, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| }, | ||
| } | ||
|
|
||
| workerStart := time.Now() | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| errChan <- fmt.Errorf("worker %d: request failed: %w", workerID, err) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| // Record TTFB for the first worker | ||
| firstByteOnce.Do(func() { | ||
| ttfb = time.Since(workerStart) | ||
| log.Debug(). | ||
| Int("worker", workerID). | ||
| Dur("ttfb", ttfb). | ||
| Msg("First byte received") | ||
|
|
||
| // Get Content-Length from the first successful response | ||
| if resp.ContentLength > 0 { | ||
| mu.Lock() | ||
| contentLength = resp.ContentLength * int64(threads) // Multiply by thread count | ||
| mu.Unlock() | ||
| log.Debug(). | ||
| Int64("content_length", resp.ContentLength). | ||
| Int("threads", threads). | ||
| Int64("total_expected", contentLength). | ||
| Msg("Content-Length detected") | ||
| } | ||
| }) | ||
|
|
||
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusPartialContent { | ||
| errChan <- fmt.Errorf("worker %d: HTTP %d", workerID, resp.StatusCode) | ||
| return | ||
| } | ||
|
|
||
| // Read response body and discard to memory | ||
| buf := make([]byte, 32*1024) | ||
| for { | ||
| select { | ||
| case <-testCtx.Done(): | ||
| return | ||
| default: | ||
| n, err := resp.Body.Read(buf) | ||
| if n > 0 { | ||
| mu.Lock() | ||
| totalBytes += int64(n) | ||
| mu.Unlock() | ||
| } | ||
| if err != nil { | ||
| if err == io.EOF { | ||
| return | ||
| } | ||
| log.Debug(). | ||
| Err(err). | ||
| Int("worker", workerID). | ||
| Msg("Read error") | ||
| return | ||
| } | ||
| } | ||
| } | ||
| }(i) | ||
| } | ||
|
|
||
| // Don't wait indefinitely for goroutines - return before context deadline | ||
| // Use a separate goroutine to wait for cleanup | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| wg.Wait() | ||
| close(done) | ||
| }() | ||
|
|
||
| // Calculate time buffer: return 500ms before timeout to avoid handler 504 | ||
| deadline, hasDeadline := testCtx.Deadline() | ||
| if !hasDeadline { | ||
| deadline = time.Now().Add(time.Duration(timeout) * time.Second) | ||
| } | ||
| timeBuffer := 500 * time.Millisecond | ||
| returnDeadline := deadline.Add(-timeBuffer) | ||
|
|
||
| // Create timer to return before actual timeout | ||
| returnTimer := time.NewTimer(time.Until(returnDeadline)) | ||
| defer returnTimer.Stop() | ||
|
|
||
| // Wait for either completion or near-timeout | ||
| var errors []error | ||
| select { | ||
| case <-done: | ||
| // All workers completed normally | ||
| close(progressDone) | ||
| close(errChan) | ||
| for err := range errChan { | ||
| errors = append(errors, err) | ||
| } | ||
| case <-returnTimer.C: | ||
| // About to hit deadline - return immediately with partial results | ||
| // Don't wait for goroutines to finish to avoid handler timeout | ||
| close(progressDone) | ||
| close(errChan) | ||
| for err := range errChan { | ||
| errors = append(errors, err) | ||
| } | ||
| // Workers will cleanup in background | ||
| go func() { | ||
| <-done // Wait for cleanup to finish | ||
| }() | ||
| case <-testCtx.Done(): | ||
| // Fallback: context cancelled/timeout - return immediately | ||
| close(progressDone) | ||
| close(errChan) | ||
| for err := range errChan { | ||
| errors = append(errors, err) | ||
| } | ||
| go func() { | ||
| <-done | ||
| }() | ||
| } | ||
|
|
||
| // Calculate results | ||
| elapsed := time.Since(startTime).Seconds() | ||
| if elapsed == 0 { | ||
| elapsed = 0.001 | ||
| } | ||
|
|
||
| downloadSpeed := float64(totalBytes) * 8 / elapsed / 1000000 // Mbps | ||
| latency := fmt.Sprintf("%.2fms", float64(ttfb.Microseconds())/1000.0) | ||
|
|
||
| // Check timeout status | ||
| wasTimeout := ctx.Err() == context.DeadlineExceeded || testCtx.Err() == context.DeadlineExceeded |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files of interest ---'
git ls-files 'internal/speedtest/*' 'internal/**/result_handler.go' 'internal/**/schedule.go' 'internal/**/handler.go' 'internal/**/server.go' | sed -n '1,200p'
echo
echo '--- search for recover() and panic handling ---'
rg -n "recover\(|panic\(" internal -g'*.go'
echo
echo '--- locate RunTest and callers ---'
rg -n "RunTest\(" internal -g'*.go'
echo
echo '--- outline speedtest files ---'
ast-grep outline internal/speedtest/url_download.go
ast-grep outline internal/speedtest/*.go 2>/dev/null || trueRepository: autobrr/netronome
Length of output: 910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- RunTest and call sites ---'
rg -n "RunTest\(" internal -g'*.go'
echo
echo '--- recover/panic in speedtest and server paths ---'
rg -n "recover\(|panic\(" internal/server internal/speedtest -g'*.go'
echo
echo '--- relevant slices of url_download.go ---'
nl -ba internal/speedtest/url_download.go | sed -n '1,360p'
echo
echo '--- result_handler.go ---'
nl -ba internal/speedtest/result_handler.go | sed -n '1,240p'Repository: autobrr/netronome
Length of output: 1655
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- recover() search across repo ---'
rg -n "recover\(" . -g'*.go' || true
echo
echo '--- url_download.go selected lines ---'
nl -ba internal/speedtest/url_download.go | sed -n '140,340p'
echo
echo '--- speedtest.go runner dispatch ---'
nl -ba internal/speedtest/speedtest.go | sed -n '100,190p'Repository: autobrr/netronome
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- recover() / panic() across repo ---'
rg -n "recover\(|panic\(" . -g'*.go' || true
echo
echo '--- url_download.go around the worker/error paths ---'
sed -n '150,340p' internal/speedtest/url_download.go | cat -nRepository: autobrr/netronome
Length of output: 7197
Don’t close errChan before the workers finish
- The timeout/cancel branches close and drain
errChanwhile workers can still send errors, which can panic on a send to a closed channel and take the process down. ttfbis also written by the first worker and read later without synchronization; guard it with the existing mutex or read it only afterwg.Wait().
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 131-131: ineffectual assignment to progress
(ineffassign)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/speedtest/url_download.go` around lines 104 - 331, The download
worker coordination must not close errChan until all workers have exited, and
ttfb access must be synchronized. Update the completion, timeout, and
cancellation handling around done and errChan so timeout paths stop or defer
draining without closing the channel while workers may still send; close it only
after wg.Wait completes. Protect the ttfb write in firstByteOnce and its later
read with the existing mutex, or otherwise ensure the read occurs after worker
completion.
s0up4200
reviewed
Jul 23, 2026
s0up4200
left a comment
s0up4200
left a comment
Collaborator
There was a problem hiding this comment.
Hi! Thanks for the PR. Was this implemented with an LLM? If so, which one, and in what harness?
Have you tested it yourself yet?
Please either address or mark the coderabbit feedbacks as resolved when you can 👍
s0up4200
added
enhancement
Author
Hi! Thanks for the review.
Let me know if anything else is needed. |
- Store actual hostname instead of display name for built-in servers - Add panic recovery to prevent crashes - Fix URL validation and progress state flags - Add schedule validation for URL downloads
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/speedtest/result_handler.go (1)
54-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPersist the hostname separately from the port.
u.Hostincludes an optional port, so custom URLs likehttps://example.com:8443/filecurrently storeexample.com:8443inserverHost, while other tests use the hostname-only value. Useu.Hostname()forserverHost; keepu.Hostin theserverIDif the port needs to affect uniqueness.Suggested fix
- host := u.Host + host := u.Hostname() serverHost = &host serverID = fmt.Sprintf("url-custom-%s", u.Host)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/speedtest/result_handler.go` around lines 54 - 58, Update the custom download URL handling around url.Parse in the result handler to assign u.Hostname() to serverHost, keeping the hostname separate from any port. Preserve u.Host in serverID so ports continue to distinguish custom server identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/speedtest/result_handler.go`:
- Around line 54-58: Update the custom download URL handling around url.Parse in
the result handler to assign u.Hostname() to serverHost, keeping the hostname
separate from any port. Preserve u.Host in serverID so ports continue to
distinguish custom server identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7ec9ef4-c8a8-44e4-afba-b3960dc2b0aa
📒 Files selected for processing (4)
internal/speedtest/result_handler.gointernal/speedtest/url_download.goweb/src/components/Main.tsxweb/src/components/speedtest/ScheduleManager.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/components/Main.tsx
- web/src/components/speedtest/ScheduleManager.tsx
- internal/speedtest/url_download.go
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
2 participants
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
Why
Testing
pnpm -C web lintpnpm -C web buildScreenshots (if UI)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes