Keep fallback probes out of the rolling three-hour average - #1
Open
CodeAThing wants to merge 1 commit into
Open
Conversation
`threeHourSamples` filtered on the source strings "FAST CDN" and "fallback probe", but no code path ever produced either name. The values actually written are "Fast.com", "FAST probe fallback" and "Cloudflare fallback", so the filter matched nothing and every fallback measurement was averaged in. That matters because the fallback probes are not comparable to the Fast.com run: they download a fixed 2-8 MB payload instead of Fast.com's adaptive test, so they report a much lower figure. A single Cloudflare fallback could halve the reported three-hour average. Replace the source string with a `SpeedSource` enum so the comparison cannot silently drift again, and derive the filter from `isFallback`. The raw values match the strings written by 1.0.x, and the enum decodes an unrecognised source as `.fastDotCom` rather than throwing, because `loadHistory()` uses `try?` and would otherwise discard the whole file. While in the same paths: - Cancelling a run (which happens on every interval change) no longer falls through to the fallback probes, and no longer surfaces "Speed test failed: CancellationError" in the popover. - When both Fast.com and the fallback fail, the reported error now names both causes instead of only the last one. - `webView.stopLoading()` moved to a `defer`, so an abandoned run cannot keep downloading on the cancellation path.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
What changed
threeHourSamplesfilters out fallback measurements by comparingsourceagainst
"FAST CDN"and"fallback probe". Neither string is producedanywhere in the codebase -- the values actually written are
"Fast.com","FAST probe fallback"and"Cloudflare fallback", so the filter matchednothing and every fallback result was averaged in.
I replaced the raw
Stringwith aSpeedSourceenum so the comparisoncannot silently drift again, and derived the filter from
isFallback.Three smaller fixes in the same paths:
through to the fallback probes, and no longer writes
"Speed test failed: CancellationError" into the popover.
both causes instead of only the last one.
webView.stopLoading()moved into adefer, so an abandoned run cannotkeep downloading on the cancellation path.
Why
The fallback probes are not comparable to the Fast.com run: they pull a fixed
2-8 MB payload instead of Fast.com's adaptive test, so they report a much
lower figure. One Cloudflare fallback in a quiet three-hour window can halve
the average shown in the popover.
The README already describes fallback results as labelled in the history, so I
read the exclusion as the intended behaviour rather than a change of policy.
Happy to drop the enum and just fix the two string literals if you'd rather
keep the diff minimal.
How it was validated
xcodebuild -project FastBar/FastBar.xcodeproj -scheme FastBar -configuration Debug -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO build-- succeeds, no new warnings.git diff --checkis clean.Ran the app on a Mac and confirmed measurements still record and render in the
menu bar and the popover.
Since there's no test target yet, I checked the encoding contract by compiling
the real
SpeedMonitor.swiftagainst a small harness:speed-history.jsonwritten by 1.0.1 decodes unchanged, and re-encoding itis byte-identical -- the enum's raw values are the strings already on disk.
sourcedecodes as.fastDotComrather than throwing.loadHistory()usestry?, so a throw there would silently discard theuser's entire history.
before the fix it was 47.4.
If a test target would be welcome I'm glad to open a follow-up PR that adds one
and moves these checks into it.