fix(webui): keep embedded ActivityWatch URLs inside WebView#161
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: 88314aaf84
ℹ️ 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".
| } | ||
|
|
||
| return when (uri.host?.lowercase()) { | ||
| "localhost", "127.0.0.1", "::1" -> true |
There was a problem hiding this comment.
isEmbeddedActivityWatchUrl compares uri.host to "::1", but java.net.URI("http://[::1]:5600").host includes brackets ("[::1]"). As a result, IPv6 loopback URLs are misclassified as external and opened in the system browser, which breaks the intended in-WebView navigation/storage behavior for those links.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 696e8d5. java.net.URI.getHost() does return IPv6 with brackets — changed the match to "[::1]". Good catch.
Greptile SummaryThis PR fixes WebView navigation by replacing a
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to host classification in the WebView URL router and correctly handles all three loopback forms. The fix is logically sound: java.net.URI correctly extracts hosts for all tested forms (localhost, 127.0.0.1, [::1]), the routing logic in shouldOverrideUrlLoading is unchanged structurally, and the new helper is covered by unit tests across the full scheme/host matrix. The only exception path (silent false on parse error) is a pre-existing pattern with a minor observability gap, not a regression. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[shouldOverrideUrlLoading] --> B{URLUtil.isNetworkUrl?}
B -- No --> C[return true: block load]
B -- Yes --> D{starts with http/https?}
D -- No --> E[return false: load in WebView]
D -- Yes --> F[isEmbeddedActivityWatchUrl]
F --> G{parse URI via java.net.URI}
G -- parse error --> H[return false: open externally]
G -- OK --> I{scheme http or https?}
I -- No --> H
I -- Yes --> J{host is localhost / 127.0.0.1 / ::1?}
J -- Yes --> K[return true: keep in WebView]
J -- No --> H
Reviews (2): Last reviewed commit: "fix(webui): fix IPv6 bracket handling an..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return when (uri.host?.lowercase()) { | ||
| "localhost", "127.0.0.1", "::1" -> true | ||
| else -> false | ||
| } |
There was a problem hiding this comment.
isEmbeddedActivityWatchUrl returns true for every port on loopback, not just 5600. If the in-app WebView ever renders a page from 127.0.0.1:5600 that links to another local service on a different port (e.g., a dev server at 127.0.0.1:3000), that link will silently stay inside the WebView rather than opening in the external browser. The original check used url.contains("//localhost:") which implicitly scoped to the scheme+host+port triple; the new helper broadens that. Consider adding a port check (port == 5600, or a small set of known ports) if this widened scope is not intentional.
There was a problem hiding this comment.
The broader scope is intentional — ActivityWatch can run on non-default ports and we want to keep all loopback traffic in-WebView regardless of port. The original url.contains("//localhost:") check was actually port-agnostic too (it just required a port to be present). Leaving as-is.
| fun `treats local embedded server hosts as internal`() { | ||
| assertTrue(isEmbeddedActivityWatchUrl("http://127.0.0.1:5600/#/settings/")) | ||
| assertTrue(isEmbeddedActivityWatchUrl("http://localhost:5600/#/settings/")) | ||
| assertTrue(isEmbeddedActivityWatchUrl("http://[::1]:5600/#/settings/")) | ||
| } |
There was a problem hiding this comment.
The isEmbeddedActivityWatchUrl function explicitly allows both http and https schemes for loopback hosts, but the "internal" test only exercises http:// URLs. Adding assertTrue(isEmbeddedActivityWatchUrl("https://localhost:5600/")) and its 127.0.0.1/::1 equivalents would verify the full scheme matrix the function guards.
There was a problem hiding this comment.
Fixed in 696e8d5 — added https variants for all three loopback hosts plus a no-port case.
…rage
java.net.URI.getHost() returns IPv6 addresses with brackets ("[::1]" not
"::1"), so the loopback check was silently misclassifying IPv6 loopback
URLs as external. Fix by matching "[::1]" directly.
Also add https loopback variants and a no-port case to the unit test to
cover the full scheme matrix the function guards.
|
All review feedback addressed in 696e8d5:
The PR is ready for merge when you get a chance, @ErikBjare. |
2 participants
Summary
127.0.0.1andlocalhostas internal embedded-server hostsWhy
The Android app serves the embedded UI from
http://127.0.0.1:5600, but the current WebView override only whitelistslocalhost. That means internal navigations can get kicked out to the external browser, which uses different web storage than the in-app WebView. That matches thesettings saved but not appliedsymptom from #153.Testing
WebUIFragmentTestcoverage for internal vs external host handlingjavais not installed on Bob's VMRefs #153