feat(speedtest): add result URL support for LibreSpeed#216
Open
s0up4200 wants to merge 6 commits into
Open
Conversation
Capture the shareable link returned by librespeed-cli --share, persist it in a new result_url column, and render it as a clickable icon in the results table. The --share flag is opt-in via share_results config.
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements opt-in result sharing for LibreSpeed speed tests by adding a ChangesLibrespeed Result Sharing Feature
Sequence DiagramsequenceDiagram
participant Runner as LibrespeedRunner
participant CLI as librespeed-cli
participant Extract as extractJSONArray
participant Sanitize as sanitizeResultURL
Runner->>CLI: execCmd with --share flag (if config.ShareResults)
CLI-->>Runner: stdout with JSON array, stderr
Runner->>Extract: extract JSON from stdout
Extract-->>Runner: JSON slice from first [
Runner->>Runner: unmarshal JSON to librespeedResult
Runner->>Sanitize: sanitizeResultURL(librespeedResult.Share)
Sanitize-->>Runner: normalized http/https URL or empty string
Runner-->>Runner: set Result.ResultURL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (1)
internal/speedtest/librespeed_test.go (1)
32-58: Consolidate the share-flag tests into a single table-driven test.Line 32 through Line 58 add two near-identical cases that differ only by
ShareResultsand expectation. A table-driven test will keep this path easier to extend and maintain.♻️ Proposed refactor
-func TestBuildArgsIncludesShareWhenEnabled(t *testing.T) { - runner := NewLibrespeedRunner(config.LibrespeedConfig{ - ServersPath: "/tmp/local-servers.json", - ShareResults: true, - }) - - args := runner.buildArgs(&types.TestOptions{ - IsPublicServer: true, - ServerIDs: []string{"123"}, - }) - - assert.Contains(t, args, "--share") -} - -func TestBuildArgsExcludesShareWhenDisabled(t *testing.T) { - runner := NewLibrespeedRunner(config.LibrespeedConfig{ - ServersPath: "/tmp/local-servers.json", - ShareResults: false, - }) - - args := runner.buildArgs(&types.TestOptions{ - IsPublicServer: true, - ServerIDs: []string{"123"}, - }) - - assert.NotContains(t, args, "--share") -} +func TestBuildArgsShareFlag(t *testing.T) { + tests := []struct { + name string + share bool + shouldShare bool + }{ + {name: "includes --share when enabled", share: true, shouldShare: true}, + {name: "excludes --share when disabled", share: false, shouldShare: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + runner := NewLibrespeedRunner(config.LibrespeedConfig{ + ServersPath: "/tmp/local-servers.json", + ShareResults: tt.share, + }) + + args := runner.buildArgs(&types.TestOptions{ + IsPublicServer: true, + ServerIDs: []string{"123"}, + }) + + if tt.shouldShare { + assert.Contains(t, args, "--share") + } else { + assert.NotContains(t, args, "--share") + } + }) + } +}Based on learnings: Applies to **/*_test.go : Prefer table-driven tests for new backend logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/speedtest/librespeed_test.go` around lines 32 - 58, The two near-identical tests TestBuildArgsIncludesShareWhenEnabled and TestBuildArgsExcludesShareWhenDisabled should be consolidated into a single table-driven test: create a slice of cases that set config.LibrespeedConfig.ShareResults to true/false with an expectedContains bool, construct the runner via NewLibrespeedRunner, call runner.buildArgs with the same TestOptions, and for each case assert that args contains "--share" when expectedContains is true and does not contain it when false; keep references to the same TestOptions (IsPublicServer/serverIDs) and use t.Run with a descriptive name per case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/speedtest/librespeed.go`:
- Line 154: The code is assigning external data librespeedResult.Share directly
into ResultURL; validate and sanitize this URL before persisting by parsing
librespeedResult.Share with net/url (url.Parse), verify the scheme is http or
https and the Host is non-empty, reject or clear the value if parsing fails or
the scheme/host are invalid, and optionally re-encode with url.URL.String() to
ensure a normalized safe form; update the assignment to use the
validated/sanitized value (the place where ResultURL is set from
librespeedResult.Share).
In `@web/src/components/speedtest/columns.tsx`:
- Around line 76-84: The icon-only result links in columns.tsx (the anchor using
row.original.resultUrl and the ExternalLink icon) lack a reliable accessible
name; add an aria-label (e.g., aria-label="View result on LibreSpeed") to those
<a> elements or include visually hidden text inside the anchor so screen readers
can identify the action; update both occurrences (the one referencing
row.original.resultUrl and the second occurrence around lines 176-184) to
include the aria-label or hidden text while keeping the existing title and
stopPropagation behavior.
---
Nitpick comments:
In `@internal/speedtest/librespeed_test.go`:
- Around line 32-58: The two near-identical tests
TestBuildArgsIncludesShareWhenEnabled and TestBuildArgsExcludesShareWhenDisabled
should be consolidated into a single table-driven test: create a slice of cases
that set config.LibrespeedConfig.ShareResults to true/false with an
expectedContains bool, construct the runner via NewLibrespeedRunner, call
runner.buildArgs with the same TestOptions, and for each case assert that args
contains "--share" when expectedContains is true and does not contain it when
false; keep references to the same TestOptions (IsPublicServer/serverIDs) and
use t.Run with a descriptive name per case.
🪄 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: 23a6a50a-4f22-4d26-96e6-9cbcd93eea05
📒 Files selected for processing (13)
config/config.tomlinternal/config/config.gointernal/database/migrations/postgres/021_add_result_url_postgres.sqlinternal/database/migrations/sqlite/021_add_result_url.sqlinternal/database/speedtest.gointernal/speedtest/librespeed.gointernal/speedtest/librespeed_test.gointernal/speedtest/result_handler.gointernal/speedtest/result_handler_test.gointernal/speedtest/types.gointernal/types/types.goweb/src/components/speedtest/columns.tsxweb/src/types/types.ts
…n test Validate librespeed share URLs with net/url before persisting (reject non-http(s) or missing host). Add aria-label to icon-only result links for screen readers. Consolidate share flag tests into table-driven form.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/speedtest/librespeed.go (1)
338-350: Consider trimming whitespace for defensive parsing.While JSON unmarshaling typically doesn't introduce leading/trailing whitespace, adding
strings.TrimSpacewould make the function more robust against malformed CLI output.♻️ Optional hardening
func sanitizeResultURL(raw string) string { - if raw == "" { + raw = strings.TrimSpace(raw) + if raw == "" { return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/speedtest/librespeed.go` around lines 338 - 350, The sanitizeResultURL function should defensively trim leading/trailing whitespace before parsing to avoid url.Parse errors on malformed CLI output; update sanitizeResultURL (and add strings import if missing) to call strings.TrimSpace(raw) and then parse the trimmed string, preserving the same scheme/host checks and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/speedtest/librespeed.go`:
- Around line 338-350: The sanitizeResultURL function should defensively trim
leading/trailing whitespace before parsing to avoid url.Parse errors on
malformed CLI output; update sanitizeResultURL (and add strings import if
missing) to call strings.TrimSpace(raw) and then parse the trimmed string,
preserving the same scheme/host checks and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de91d21c-0797-4bfc-9058-18566a30833c
📒 Files selected for processing (3)
internal/speedtest/librespeed.gointernal/speedtest/librespeed_test.goweb/src/components/speedtest/columns.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/speedtest/librespeed_test.go
- web/src/components/speedtest/columns.tsx
librespeed-cli prints telemetry error text to stdout before the JSON when --share is used. Find the JSON array start before unmarshaling.
…ipped on upgrade) develop already ships 021_packetloss_history_summary_index, and the migrator applies migrations by position relative to the applied count (it never reads migration names back). The duplicate 021_add_result_url sorted before the already-applied packetloss index, so on existing databases it was silently skipped: result_url was never created and every SaveSpeedTest/GetSpeedTests then failed on the missing column (speed test history broke after upgrade). Fresh installs applied all migrations in order, masking the bug in CI. Renumber to 022 for both dialects so the new migration always sorts last and is applied on upgrade. Add a migrations test rejecting duplicate version prefixes, plus a DB round-trip test for result_url. Also fixes other review findings for this PR: - speedtest: read librespeed-cli output from stdout instead of CombinedOutput so stderr telemetry errors (e.g. an IPv6 endpoint with '[') can't corrupt JSON parsing; extract the JSON-array trimming into a tested helper. - speedtest: replace the tautological result-URL test with one that exercises DefaultResultHandler.SaveResult; pin sanitizeResultURL normalization cases. - web: keep server-name ellipsis when a result link is present (move truncate to a shrinkable child of the inline-flex container; was clipping without '…'). - docs: document NETRONOME__LIBRESPEED_SHARE_RESULTS.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/librespeed.go`:
- Line 133: The log call currently emits raw librespeed-cli stdout/stderr
(log.Error().Err(err).Str("stdout", stdout.String()).Str("stderr",
stderr.String()).Msg("librespeed-cli failed")), which can leak
client-identifying JSON and share URLs; update the error handling in the
function that invokes the LibreSpeed CLI to stop logging full stdout/stderr —
either remove those .Str("stdout", ...) and .Str("stderr", ...) fields entirely,
or replace them with a sanitized/limited value (e.g., a short truncated string
or a redacted/sanitizer function that strips URLs and known client fields) and
keep the Err(err) and contextual message; ensure the change touches the same
variables (stdout, stderr) and the LibreSpeed invocation site so no raw payloads
are written to logs.
- Line 134: Wrap the returned error using %w so callers can unwrap the original
error (replace fmt.Errorf("...%v...", err, ...) with fmt.Errorf("...: %w", err)
in the function that invokes the librespeed-cli return), and remove any logging
that prints raw librespeed-cli stdout/stderr (do not log
strings.TrimSpace(stderr.String()) or stdout contents); instead log a
non-sensitive message such as the command exit status or an anonymized failure
note from the same function (identify and update the error return and the
logging statements around the librespeed-cli invocation in
internal/speedtest/librespeed.go).
🪄 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: cf76f7df-11d2-4aed-95d6-8f0d0bd1011c
📒 Files selected for processing (11)
README.mdinternal/database/migrations/migrations_test.gointernal/database/migrations/postgres/022_add_result_url_postgres.sqlinternal/database/migrations/sqlite/022_add_result_url.sqlinternal/database/speedtest_integration_test.gointernal/speedtest/librespeed.gointernal/speedtest/librespeed_test.gointernal/speedtest/result_handler_test.gointernal/types/types.goweb/src/components/speedtest/columns.tsxweb/src/types/types.ts
💤 Files with no reviewable changes (2)
- internal/database/migrations/postgres/022_add_result_url_postgres.sql
- internal/database/migrations/sqlite/022_add_result_url.sql
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- web/src/types/types.ts
- internal/database/speedtest_integration_test.go
- internal/types/types.go
- web/src/components/speedtest/columns.tsx
- internal/speedtest/result_handler_test.go
| cmd.Stderr = &stderr | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| log.Error().Err(err).Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli failed") |
There was a problem hiding this comment.
Avoid logging raw LibreSpeed stdout/stderr payloads.
Line 133 and Line 139 log full process output. That output can include client-identifying JSON fields and share URLs, which is a privacy/compliance risk in logs.
Suggested hardening
- log.Error().Err(err).Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli failed")
+ log.Error().
+ Err(err).
+ Int("stdout_bytes", stdout.Len()).
+ Int("stderr_bytes", stderr.Len()).
+ Str("stderr", strings.TrimSpace(stderr.String())).
+ Msg("librespeed-cli failed")
@@
- log.Debug().Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli output")
+ log.Debug().
+ Int("stdout_bytes", stdout.Len()).
+ Int("stderr_bytes", stderr.Len()).
+ Msg("librespeed-cli output captured")Also applies to: 139-139
🤖 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/librespeed.go` at line 133, The log call currently emits
raw librespeed-cli stdout/stderr (log.Error().Err(err).Str("stdout",
stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli failed")),
which can leak client-identifying JSON and share URLs; update the error handling
in the function that invokes the LibreSpeed CLI to stop logging full
stdout/stderr — either remove those .Str("stdout", ...) and .Str("stderr", ...)
fields entirely, or replace them with a sanitized/limited value (e.g., a short
truncated string or a redacted/sanitizer function that strips URLs and known
client fields) and keep the Err(err) and contextual message; ensure the change
touches the same variables (stdout, stderr) and the LibreSpeed invocation site
so no raw payloads are written to logs.
|
|
||
| if err := cmd.Run(); err != nil { | ||
| log.Error().Err(err).Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli failed") | ||
| return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, strings.TrimSpace(stderr.String())) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify non-wrapping fmt.Errorf usages where an error variable is formatted with %v/%s.
rg -nP --type=go 'fmt\.Errorf\([^)]*%[vs][^)]*,\s*err\b'Repository: autobrr/netronome
Length of output: 1178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the error site and nearby stdout/stderr handling
sed -n '110,170p' internal/speedtest/librespeed.go
# Also show any logging around where stdout/stderr is handled, if farther away
rg -n 'stdout|stderr|TrimSpace\(stderr\.String\(\)\)|TrimSpace\(stdout\.String\(\)\)|librespeed-cli' internal/speedtest/librespeed.go -nRepository: autobrr/netronome
Length of output: 4013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show LibrespeedResult definition
rg -n "type LibrespeedResult|LibrespeedResult struct" internal/speedtest/librespeed.go
sed -n '170,260p' internal/speedtest/librespeed.go
sed -n '260,420p' internal/speedtest/librespeed.goRepository: autobrr/netronome
Length of output: 6431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the LibrespeedResult struct and related types near the top of the file
sed -n '1,120p' internal/speedtest/librespeed.go
# Confirm which client-identifying fields exist in the JSON struct
rg -n --fixed-strings 'json:"ip"' internal/speedtest/librespeed.go
rg -n --fixed-strings 'json:"hostname"' internal/speedtest/librespeed.go
rg -n --fixed-strings 'json:"location"' internal/speedtest/librespeed.go
rg -n 'Share|share' internal/speedtest/librespeed.goRepository: autobrr/netronome
Length of output: 3476
Fix error wrapping and stop logging sensitive librespeed output
- Wrap
errwith%w(line 134) so callers can unwrap the original failure. - Don’t log raw
librespeed-clistdout/stderr(lines 133 and 139): the CLI JSON includesclientdetails (ip,hostname,city/region/country,loc, etc.) andshare, which can leak into logs.
Suggested fix
- return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, strings.TrimSpace(stderr.String()))
+ return nil, fmt.Errorf("librespeed-cli failed: %w: %s", err, strings.TrimSpace(stderr.String()))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, strings.TrimSpace(stderr.String())) | |
| return nil, fmt.Errorf("librespeed-cli failed: %w: %s", err, strings.TrimSpace(stderr.String())) |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 134-134: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
🤖 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/librespeed.go` at line 134, Wrap the returned error using
%w so callers can unwrap the original error (replace fmt.Errorf("...%v...", err,
...) with fmt.Errorf("...: %w", err) in the function that invokes the
librespeed-cli return), and remove any logging that prints raw librespeed-cli
stdout/stderr (do not log strings.TrimSpace(stderr.String()) or stdout
contents); instead log a non-sensitive message such as the command exit status
or an anonymized failure note from the same function (identify and update the
error return and the logging statements around the librespeed-cli invocation in
internal/speedtest/librespeed.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
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.
Captures the shareable link from
librespeed-cli --share, stores it in a newresult_urlcolumn, and shows it as a clickable icon next to the server name in the results table.The
--shareflag is opt-in since it sends telemetry to librespeed.org. Enable it by settingshare_results = trueunder[speedtest.librespeed]in your config, or via theLIBRESPEED_SHARE_RESULTS=trueenv var. Disabled by default.Closes #159
Summary by CodeRabbit
New Features
Database
Documentation
Tests