Skip to content

Warn user when DNS is not working - #4414

Open
nicoschmdt wants to merge 2 commits into
bluerobotics:masterfrom
nicoschmdt:warn-dns
Open

Warn user when DNS is not working#4414
nicoschmdt wants to merge 2 commits into
bluerobotics:masterfrom
nicoschmdt:warn-dns

Conversation

@nicoschmdt

@nicoschmdt nicoschmdt commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

fix: #690

web icon changes to dns and has a tooltip informing the user that a DNS problem was identified. if the user opens it the DNS page will be the main one.

image

@nicoschmdt
nicoschmdt marked this pull request as draft September 4, 2026 20:12
@nicoschmdt
nicoschmdt marked this pull request as ready for review September 4, 2026 20:14
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Automated PR Review

0. Summary

  • Verdict: MINOR SUGGESTIONS ✏️

Adds a "DNS is failing" warning path: the Helper backend now classifies each site probe with an error_kind (dns / timeout) so the frontend can distinguish a name-resolution failure from a full internet outage, and the InternetTrayMenu swaps to mdi-dns plus a dedicated tooltip when DNS is broken. Clicking through the tray opens NetworkInterfaceMenu directly on the DNS Configuration tab via a new initial-page prop.

1. Correctness & Implementation Bugs

  • 1.1 [minor] core/services/helper/main.py:509 — the pending-future branch used to set error="timeout"; it now sets only error_kind=WebsiteError.TIMEOUT and leaves error=None. Non-frontend consumers of /v1.0/check_internet_access (extensions, users scripting against the API) that used error == "timeout" to detect a timeout will now see null. Given check_internet_access is a versioned public endpoint, either keep error="timeout" alongside the new error_kind, or acknowledge this as a v1 schema change. The DNS path is not affected because check_website sets both error and error_kind together (main.py:466).
  • 1.2 [minor] core/frontend/src/components/app/NetworkInterfaceMenu.vue:62initialPage is only consumed in mounted(), so subsequent changes to the parent's dns_failure while the dialog is open are ignored. This is fine today because InternetTrayMenu.vue:33 guards with v-if="show_menu" and the dialog remounts on each open, but that coupling is easy to miss on future edits. A watch: { initialPage(...) } would make the component robust in isolation.

4. Performance

  • 4.1 [nit] core/frontend/src/store/helper.ts:107decided_sites is walked three times (filter twice + some twice more via ip_sites/named_sites). Fine at 5 sites; call out only if the site list grows.

6. Code Quality & Style

  • 6.1 [minor] core/services/helper/main.py:293request_response.dns_error = isinstance(e, socket.gaierror) overwrites the flag on every hit of the (HTTPException, socket.error) branch. Works because the default is False, but reads more like a set-when-True. Consider if isinstance(e, socket.gaierror): request_response.dns_error = True for symmetry with how error_kind is set in check_website.
  • 6.2 [minor] core/frontend/src/components/app/InternetTrayMenu.vue:34:initial-page="dns_failure ? 'dns_configuration' : ''" uses the empty string as a sentinel. Prefer :initial-page="dns_failure ? 'dns_configuration' : null" (and default: null on the prop) — the "no target" case is a null, not an empty page key.
  • 6.3 [nit] core/frontend/src/store/helper.ts:104 — the DNS-classification block (10+ lines with a multi-line comment) would read more clearly extracted into a small helper (e.g. deriveDnsFailure(decided_sites)) returning boolean | null (null = "keep previous"), moving the "no else" invariant out of the middle of the action.
  • 6.4 [nit] core/frontend/src/components/app/NetworkInterfaceMenu.vue:53 — the page_selected type change from string | null to number | null is actually a latent bug fix (v-tabs without :value= on tabs emits indices). Worth calling out in the commit message so the change isn't hidden inside the DNS work.

7. Tests

  • 7.1 [minor] No unit tests cover the new error_kind classification in simple_http_request / check_website, nor the frontend dns_failure state machine. The state machine has non-trivial "keep previous value" behavior (the // No else comment) that is exactly the kind of thing a small test would pin down. ./.hooks/pre-push will not fail from missing coverage on new lines automatically, but adding a couple of pytest cases mocking socket.gaierror / socket.timeout around Helper.simple_http_request would prevent silent regressions.

8. Documentation

  • 8.1 [nit] PR title is draft: Warn user when DNS is not working while isDraft: false. Either flip the PR to a GitHub draft or drop the draft: prefix — the current combination is confusing for reviewers scanning the queue.
  • 8.2 [nit] PR body is just fix: #690. A one-paragraph summary of the classification rules (what qualifies as "DNS failure", why the "no else" branch exists) would speed up review and give future readers something more informative than the commit diff.

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.

@nicoschmdt
nicoschmdt marked this pull request as draft September 4, 2026 20:18
@nicoschmdt
nicoschmdt marked this pull request as ready for review September 14, 2026 19:23
@nicoschmdt
nicoschmdt requested a review from a team September 14, 2026 19:24
@nicoschmdt nicoschmdt changed the title draft: Warn user when DNS is not working Warn user when DNS is not working Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core: Version-chooser: Temporary failure in name resolution

1 participant