Add failed request diagnostics#750
Merged
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09949728a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds a lightweight request-failure diagnostics pipeline that records recent API failures and surfaces them in the Settings → Diagnostics screen and the exported diagnostics text, improving post-mortem troubleshooting.
Changes:
- Introduces
RequestDiagnostics(actor) andFailedRequestmodel to record recent failed API requests. - Hooks API error paths to record failures and exposes them in
DiagnosticsView(UI + export). - Extends
NetworkReport.exportText(...)to include failed-request export lines.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Ruddarr/Views/Settings/DiagnosticsView.swift | Adds a “Failed Requests” section and includes failed requests in the shared diagnostics export. |
| Ruddarr/Utilities/RequestDiagnostics.swift | New actor + data model for recording and snapshotting recent failed requests. |
| Ruddarr/Utilities/NetworkReport.swift | Extends diagnostics export to accept and append failed-request lines. |
| Ruddarr/Dependencies/API/API.swift | Records API failures to RequestDiagnostics when requests throw. |
Comments suppressed due to low confidence (2)
Ruddarr/Views/Settings/DiagnosticsView.swift:169
- This adds user-facing diagnostics output (new Failed Requests section and inclusion in the shared export), but the PR doesn’t include the required release-note updates. Per AGENTS.md, user-facing changes must update CHANGELOG.md (Unreleased) and keep TestFlight/WhatToTest.en-US.txt in sync with Unreleased.
if candidate.hasHostname {
networkDiagnosticsRow("Resolved IPs", candidateAddressesText(candidate))
}
if candidate.probeDescription != nil {
Ruddarr/Views/Settings/DiagnosticsView.swift:366
- Even with masking enabled, the displayed/exported failed-request URL will include the full query string because
maskedURL(...)preservespercentEncodedQuery. That can leak sensitive query parameters in “masked” diagnostics (e.g. tokens embedded in URLs).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Ruddarr/Views/Settings/DiagnosticsView.swift:117
- This adds a user-facing Diagnostics screen section (“Failed Requests”). Per AGENTS.md, user-facing changes must include a
CHANGELOG.mdentry underUnreleased, andTestFlight/WhatToTest.en-US.txtshould matchUnreleased. Those updates aren’t included in this PR diff.
requestRow(request)
}
} header: {
Text(verbatim: "Failed Requests")
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Ruddarr/Views/Settings/DiagnosticsView.swift:116
- This adds a new user-facing Diagnostics section (“Failed Requests”). Per AGENTS.md, user-facing changes should be reflected in
CHANGELOG.md(Unreleased) and mirrored inTestFlight/WhatToTest.en-US.txt, but those files don’t appear to include an entry for this change.
Text(verbatim: "Failed Requests")
Comment on lines
110
to
114
| if !failedRequests.isEmpty { | ||
| Section { | ||
| networkDiagnosticsRow("IPv4 Address", mask.list(report.deviceV4.map { mask.ip(NetworkInterfaces.string(fromIPv4: $0.address)) })) | ||
| networkDiagnosticsRow("IPv4 Subnets", report.subnetRowsV4(mask)) | ||
|
|
||
| if !report.deviceV6.isEmpty { | ||
| networkDiagnosticsRow("IPv6 Address", mask.list(report.deviceV6.map { mask.ip(NetworkInterfaces.string(fromIPv6Bytes: $0.address)) })) | ||
| networkDiagnosticsRow("IPv6 Subnets", report.subnetRowsV6(mask)) | ||
| ForEach(failedRequests) { request in | ||
| requestRow(request) | ||
| } |
Comment on lines
+48
to
+54
| private static func deviceSection(_ report: NetworkReport) -> FieldSection { | ||
| FieldSection("Device", [ | ||
| DiagnosticRow("Connection", .plain(report.connection)), | ||
| DiagnosticRow("Low Data Mode", .plain(report.constrained ? "on" : "off"), appearance: .exportOnly), | ||
| DiagnosticRow("Local Network", .plain(report.localNetworkDenied ? "denied" : "allowed")), | ||
| DiagnosticRow("Tailscale", .plain(report.tailnetUp ? "up" : "down")), | ||
| ]) |
| .truncationMode(.middle) | ||
|
|
||
| if let detail = request.detail, !detail.isEmpty { | ||
| Text(verbatim: masked ? maskURLs(in: detail) : detail) |
| lines.append(" URL: \(mask.url(request.url))") | ||
|
|
||
| if let detail = request.detail, !detail.isEmpty { | ||
| lines.append(" Error: \(mask.masked ? maskURLs(in: detail) : detail)") |
…mplify the diagnostics model
…buffer, and gate seeding to DEBUG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Ruddarr/Views/Settings/DiagnosticsView.swift:113
- This PR adds a new user-facing “Failed Requests” diagnostics section, but there’s no corresponding entry in CHANGELOG.md (Unreleased) and TestFlight/WhatToTest.en-US.txt currently doesn’t mention it. Repo guidance requires updating both for user-facing changes.
Text(verbatim: "Failed Requests")
Comment on lines
+83
to
+87
| } catch let error as CancellationError { | ||
| throw error | ||
| } catch API.Error.notConnectedToInternet { | ||
| throw API.Error.notConnectedToInternet | ||
| } catch { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
Ruddarr/Dependencies/API/API.swift:87
API.requestcurrently rethrowsAPI.Error.notConnectedToInternetbefore the diagnostics recording block, so these transport failures never appear in the new “Failed Requests” diagnostics (and won’t be included in the shared export) even though the PR description says transport failures are captured. Let this error fall through to the recording catch so it’s recorded while still being rethrown unchanged.
} catch let error as CancellationError {
throw error
} catch API.Error.notConnectedToInternet {
throw API.Error.notConnectedToInternet
} catch {
Ruddarr/Views/Settings/DiagnosticsView.swift:114
- This is a user-facing change (new “Failed Requests” section under Settings → Diagnostics). Per AGENTS.md, user-facing changes must add a
CHANGELOG.mdentry underUnreleasedand keepTestFlight/WhatToTest.en-US.txtin sync withUnreleased. Those files don’t currently mention this feature, so the release notes need updating before merge.
@ViewBuilder
func requestsSection(_ requests: [FailedRequest]) -> some View {
if !requests.isEmpty {
Section {
ForEach(requests) { request in
requestRow(request)
}
} header: {
Text(verbatim: "Failed Requests")
}
Ruddarr/Utilities/DiagnosticsExport.swift:10
DiagnosticsExport.text(from:masked:)is now the single source for the shareable diagnostics format (including the new Failed Requests block and masking behavior), but there are no tests asserting the exported text structure/content. Adding a focused test would help prevent accidental regressions to the export format and privacy masking.
enum DiagnosticsExport {
static func text(from report: DiagnosticsReport, masked: Bool) -> String {
var lines: [String] = ["# Ruddarr Diagnostics"]
if let version = Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String,
let build = Bundle.main.object(forInfoDictionaryKey: "CFBundleVersion") as? String {
lines.append("Version: \(version) (\(build))")
}
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
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.
Adds a Failed Requests section to the Diagnostics screen (Settings → Diagnostics) listing recent failed HTTP requests, and refactors the screen and its shareable
.txtexport to render from a single model.Failed request capture
API.requestnow records failures intoRequestDiagnostics— an in-memory, session-scoped ring buffer (most recent 50; consecutive duplicates collapsed).Single-source diagnostics model
.txtexport both render from oneDiagnosticsReport(DiagnosticsModel.swift) — adding a diagnostic is a singleDiagnosticRow, and masking is centralized inDiagnosticValue.NetworkReport.exportText/AppDiagnostics.exportLines/ view-side export code that had to be hand-kept in sync. Screen visuals are unchanged; the export is slightly cleaner (separate[Device]/[Network]blocks).Tests
RequestDiagnosticsTestscovers newest-first ordering, consecutive-duplicate collapsing, the 50-entry cap, and blank-instance handling. The store is decoupled fromAPI.Error(mapping moved toRequestDiagnostics+API.swift) so it fits the Foundation-only Tests target.