Fix notification/deep-link URL being discarded on cold start (#4145)#4740
Draft
dimdal wants to merge 5 commits into
Draft
Fix notification/deep-link URL being discarded on cold start (#4145)#4740dimdal wants to merge 5 commits into
dimdal wants to merge 5 commits into
Conversation
…t#4145) On cold start webview starts blank, so loadActiveURLIfNeeded() (fired from viewWillAppear + HA-connect notification) races open(inline:) and loads the default server URL instead of the notification's URL. Track the explicitly requested URL (pendingOpenInlineURL); give it priority in loadActiveURLIfNeeded over restored/default; clear on didFinish. Wins every load ordering, so URL no longer discarded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lank - Clear pendingOpenInlineURL on non-cancelled didFail/didFailProvisional so a permanently-failing URL doesn't get re-forced on later active-URL loads. Cancelled failures keep it (a superseding load must not lose it). - didFinish: don't clear on about:blank (loaded by showNoActiveURLError), which would drop the pending URL before the real navigation runs. - Note baseIsEqual same-server semantics on prioritizedInlineURL. - Add tests for real-failure clear and cancelled-keeps-pending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… too Move the pendingOpenInlineURL clear out of the URLError cast in didFail so any non-cancelled committed-navigation failure clears it, not just URLError. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…LRequest The earlier tests lived in WebViewControllerTests.swift, which has no project/target reference (orphaned upstream) — CI never compiled or ran them. Move them to WebViewControllerPendingURLTests.swift, registered in Tests-App, so CI runs them (7 tests, incl. the cold-start race regression). Extract activeURLRequest(for:) from loadActiveURLIfNeeded so the closure stays under the cyclomatic_complexity limit. No behavior change. Verified locally on Xcode 26.3 / iOS 26.3 sim: 7/7 pass; disabling the fix branch fails only the cold-start integration test (red→green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR addresses a cold-start race where loadActiveURLIfNeeded() could overwrite an explicit notification/deep-link navigation by introducing a “pending inline URL” concept and prioritizing it during initial loads.
Changes:
- Add
pendingOpenInlineURLto persist an explicitopen(inline:)request across cold-start loads. - Refactor
loadActiveURLIfNeeded()URL selection intoactiveURLRequest(for:)with prioritization logic. - Add a dedicated XCTest suite validating prioritization and failure/clearing behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/App/WebView/WebViewControllerPendingURLTests.swift | Adds tests for pending inline URL prioritization and clearing behavior. |
| Sources/App/Frontend/WebView/WebViewController.swift | Introduces pendingOpenInlineURL state with rationale. |
| Sources/App/Frontend/WebView/WebViewController+WebKitDelegates.swift | Clears/retains pending URL based on navigation outcomes and special-cases about:blank. |
| Sources/App/Frontend/WebView/WebViewController+URLLoading.swift | Centralizes active-URL request selection and adds prioritizedInlineURL. |
| Sources/App/Frontend/WebView/WebViewController+Navigation.swift | Records the pending URL when open(inline:) is invoked. |
| HomeAssistant.xcodeproj/project.pbxproj | Registers the new test file in the test target. |
Comment on lines
39
to
+47
| func webView(_ webView: WKWebView, didFail navigation: WKNavigation!, withError error: Error) { | ||
| refreshControl.endRefreshing() | ||
|
|
||
| // A committed navigation truly failed; stop forcing the requested URL so the default can take | ||
| // over. Only on a *real* failure — a `.cancelled` error just means a newer load superseded an | ||
| // in-flight one, and clearing then would revive the cold-start race (#4145). | ||
| if !error.isCancelled { | ||
| pendingOpenInlineURL = nil | ||
| } |
Comment on lines
+101
to
+106
| // the explicit navigation has landed; stop forcing it on subsequent active-URL loads (#4145). | ||
| // skip about:blank, which `showNoActiveURLError()` loads — clearing then would drop the | ||
| // pending URL before the real navigation gets a chance to run. | ||
| if webView.url?.absoluteString != "about:blank" { | ||
| pendingOpenInlineURL = nil | ||
| } |
Comment on lines
+109
to
+122
| if let currentURL = webView.url, currentURL.path.count > 1 { | ||
| // Preserve the current path when the base URL changes (e.g., switching between internal/external) | ||
| var components = URLComponents(url: webviewURL, resolvingAgainstBaseURL: true) | ||
| components?.path = currentURL.path | ||
| if let query = currentURL.query { | ||
| // Preserve external_auth if present, add other query items | ||
| var queryItems = components?.queryItems ?? [] | ||
| let currentQueryItems = URLComponents(url: currentURL, resolvingAgainstBaseURL: false)? | ||
| .queryItems ?? [] | ||
| for item in currentQueryItems where item.name != "external_auth" { | ||
| queryItems.append(item) | ||
| } | ||
| components?.queryItems = queryItems | ||
| } |
Comment on lines
+104
to
+106
| if webView.url?.absoluteString != "about:blank" { | ||
| pendingOpenInlineURL = nil | ||
| } |
Comment on lines
+78
to
+81
| let wasCatalyst = Current.isCatalyst | ||
| // Take the synchronous load path (skips the async connectivity sync used on iOS). | ||
| Current.isCatalyst = true | ||
| defer { Current.isCatalyst = wasCatalyst } |
Comment on lines
+36
to
+41
| /// A URL requested explicitly via `open(inline:)` (notification tap or deep link) that must take | ||
| /// priority over the automatic active-URL load. On cold start the webview begins blank, so | ||
| /// `loadActiveURLIfNeeded()` — fired from `viewWillAppear` and the HA-connect notification — would | ||
| /// otherwise race with and overwrite this navigation with the default server URL, discarding the | ||
| /// requested URL (#4145). Cleared once a navigation finishes (`didFinish`). | ||
| var pendingOpenInlineURL: URL? |
Member
|
Please provide a screen recording showing before and after |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4740 +/- ##
=======================================
Coverage ? 45.54%
=======================================
Files ? 275
Lines ? 16606
Branches ? 0
=======================================
Hits ? 7563
Misses ? 9043
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
home-assistant
Bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
…d binding - Expand the pendingOpenInlineURL doc comment to state the real clearing conditions (didFinish except about:blank; genuine failures; kept on .cancelled). - Replace the unused 'if let query' binding in activeURLRequest with a nil-check (behavior unchanged; silences the unused-value warning). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Summary
Fixes #4145. Tapping a notification with a
url(or aURIaction targeting the Home Assistant frontend) navigates correctly when the app is already running, but on a cold start the URL is discarded and the app just opens to its default page.Root cause — a cold-start race. On cold start the webview starts blank (
webView.url == nil). The notification tap drivesopen(inline:)→load(notificationURL), butloadActiveURLIfNeeded()— fired independently fromviewWillAppearandHomeAssistantAPI.didConnectNotification— loads the default server URL wheneverwebView.url == nil. Whicheverload()lands last wins, and it is usually the default, so the notification URL is lost. When the app is already runningwebView.urlis non-nil,loadActiveURLIfNeeded()early-returns, andopen(inline:)always wins — which matches the reports (works when open, and one reporter seeing it work intermittently is the race surfacing).Fix. Mirror the existing
initialURLrestoration mechanism with apendingOpenInlineURL:open(inline:)records the explicitly requested URL before loading it.loadActiveURLIfNeeded()gives that pending URL priority over the restored/default URL (new pure helperprioritizedInlineURL(...)).didFinishclears it once the navigation lands.Because the pending URL is re-asserted while the webview is still blank and only cleared on a successful navigation, it wins in every ordering of the two loads, so the URL is no longer discarded. Deep links benefit too, since they share the same
open(inline:)path.Scope: this covers URLs that resolve to the HA frontend (server URLs and relative paths such as
/frigate/review/...), which is the path in the issue. The separateaction: URI→ external browser path (openURLInBrowser) is not touched here.Added unit tests in
WebViewControllerPendingURLTests(registered in theTests-Apptarget so CI compiles and runs them) covering the priority helper,open(inline:)recording the pending URL, the cold-start race itself, and pending-URL clearing on navigation failure (kept on cancellation).Screenshots
No UI change — this is a navigation/timing fix, so there is nothing visual to capture. Behaviour: with the app force-closed, tapping a notification carrying a
urlnow lands on that URL instead of the default dashboard.Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes
🤖 Generated with Claude Code