Add UI architecture documentation of android and detail of the FrontendScreen#2965
Add UI architecture documentation of android and detail of the FrontendScreen#2965TimoPtr wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughA new documentation section titled "WebView communication architecture" is added to the Android architecture guide, detailing the communication flow between Home Assistant Frontend JS and internal Android components, including architecture overview and component responsibilities. This is a documentation-only addition with no code or behavior changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/android/architecture.md (1)
119-119: Consider clarifying the arrow label for consistency.The label
"FrontendJsHandler"appears to reference an interface or type, while other labels in the diagram use method names or actions (e.g.,"getExternalAuth()","onMessageReceived()"). Consider making this label more descriptive to match the pattern, such as"implements FrontendJsHandler"or"via FrontendJsHandler".📝 Suggested clarification
- JsBridge -->|"FrontendJsHandler"| MsgHandler + JsBridge -->|"via FrontendJsHandler"| MsgHandleror
- JsBridge -->|"FrontendJsHandler"| MsgHandler + JsBridge -->|"implements FrontendJsHandler"| MsgHandler🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/android/architecture.md` at line 119, The diagram arrow label "FrontendJsHandler" is ambiguous compared to other action/method labels; update the label between JsBridge and MsgHandler to a descriptive phrase like "implements FrontendJsHandler" or "via FrontendJsHandler" so it matches the pattern of method/action labels (reference: JsBridge, FrontendJsHandler, MsgHandler).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/android/architecture.md`:
- Line 92: The two broken links in the sentence referencing external
authentication and external bus need updating: replace the paths
`/docs/api/native-app-integration/frontend/external-authentication` and
`/docs/api/native-app-integration/frontend/external-bus` with the correct
`/docs/frontend/external-authentication` and `/docs/frontend/external-bus`
respectively so the WebView paragraph links point to the actual documentation
pages.
---
Nitpick comments:
In `@docs/android/architecture.md`:
- Line 119: The diagram arrow label "FrontendJsHandler" is ambiguous compared to
other action/method labels; update the label between JsBridge and MsgHandler to
a descriptive phrase like "implements FrontendJsHandler" or "via
FrontendJsHandler" so it matches the pattern of method/action labels (reference:
JsBridge, FrontendJsHandler, MsgHandler).
|
@jpelgrom do we wait for it to be use before merging this? |
|
Mermaid styling seems to need some tweaks for dark mode...
Yes I'd wait until this is actually how the main WebView works, we might tweak it while developing. |
|
From what I understand, this isn't ready for merge yet, hence setting as draft. |
Exactly. I'll handle the merge, once the migration is going to be over. We are merging small bits at the time in debug only in the app. I'm going to probably keep this PR for a while and might update it along the way. The progress can be tracked in home-assistant/android#6424 |
TimoPtr
force-pushed
the
android/webview_architecture
branch
from
47b9b01 to
32c179b
Compare
TimoPtr
changed the title
jpelgrom
left a comment
jpelgrom
left a comment
There was a problem hiding this comment.
The updated doc helps a lot in making it clear what you want to achieve 👍
What I'm missing is:
- The introduction of UseCase
Comparing this to something like https://developer.android.com/topic/architecture or https://github.com/android/nowinandroid/blob/main/docs/ArchitectureLearningJourney.md, the language feels very theoretical and not practical, while this should be a practical document.
| image: "img/default-social.png", | ||
| mermaid: { | ||
| theme: { light: "neutral", dark: "forest" }, | ||
| theme: { light: "neutral", dark: "dark" }, |
There was a problem hiding this comment.
This change along with the other dependency changes should be submitted separately so others who do more work with Docusaurus will also notice it
| @@ -0,0 +1,446 @@ | |||
| --- | |||
| title: "UI architecture (MVI‑like)" | |||
There was a problem hiding this comment.
| title: "UI architecture (MVI‑like)" | |
| title: "UI architecture" |
| sidebar_label: "UI architecture" | ||
| --- | ||
|
|
||
| This page describes the **MVI‑like** pattern we use for screens, and then shows how the dashboard (the frontend screen) applies it. New UI should follow the same pattern. |
There was a problem hiding this comment.
This suggests a generic architecture for any screen, not just frontend. If that is intended I think it should be referenced near the top of the architecture page (## Application architecture) to avoid readers thinking we follow 100% what Google suggests.
|
|
||
| ## The pattern | ||
|
|
||
| A screen often has many concurrent input sources — the user, system callbacks (permissions, file chooser), timers, and external channels — that all have to be reduced into **one** coherent UI. The pattern keeps that predictable and testable: all inputs flow _in_ as intents, and are turned into a few well‑defined outputs — durable **state**, one‑shot **events**, and imperative **actions**. |
There was a problem hiding this comment.
Formatting note for here and many paragraphs following: the style (guide) used uses bold for UI strings or sometimes at the start of a sentence or by exception. Emphases can be added using italicized text but sparingly. This page is full of bold and italicized text and reads like LLM output.
|
|
||
| --- | ||
|
|
||
| ## The frontend screen |
There was a problem hiding this comment.
Considering the size of the page, and the intention to have the text above be more generic, I'd suggest splitting this into another (sub)page
|
|
||
| VM -->|"delegates"| Handlers | ||
| VM -->|"delegates"| Managers | ||
| VM -->|"delegates"| Repos |
There was a problem hiding this comment.
This shows as the top item of the section now, it should be the other way around with repositories at the bottom to match the previous graphs!
|
|
||
| ### At a glance | ||
|
|
||
| | Pattern role | Frontend implementation | File(s) | |
There was a problem hiding this comment.
For file(s), please provide links
| | State | `FrontendViewState` (`LoadServer`, `Loading`, `Content`, `Error`, `Insecure`, `SecurityLevelRequired`) | `frontend/FrontendViewState.kt` | | ||
| | Events | `FrontendEvent` | `frontend/navigation/FrontendEvent.kt` | | ||
| | Actions | `WebViewAction` | `frontend/WebViewAction.kt` | | ||
| | Request/response prompts | `SingleSlotQueue` | `common/util/SingleSlotQueue.kt` | |
There was a problem hiding this comment.
This is the common util pattern, shouldn't I expect to see the type used with the prompt here?
| - creates the platform `WebView` in an `AndroidView` and applies a baseline **`defaultSettings()`** — JavaScript and DOM storage on, zoom controls off, a minimum font size, the Home Assistant user‑agent appended, and a **transparent background** (so the launch screen/theme shows through until the frontend paints); | ||
| - applies the **night‑mode** preference via `WebSettingsCompat.setForceDark` — a fallback for forked or older system WebViews that don't follow the app theme on their own; | ||
| - shows a **placeholder** instead of a real WebView under Compose previews / screenshot tests, and reports `onWebViewCreationFailed` when the system WebView provider can't be instantiated (which the ViewModel turns into an unrecoverable `Error` state); | ||
| - routes the **back button** to `WebView.goBack()`, falling back to the nav host once the WebView's history is empty. | ||
|
|
||
| On top of that baseline, `FrontendScreen.configureForFrontend(...)` layers the **frontend‑specific** setup: it attaches the two clients, enables first‑ and third‑party **cookies** (`CookieManager`), toggles `mediaPlaybackRequiresUserGesture` from the "autoplay video" preference, registers a **download listener**, and installs a multi‑pointer **swipe listener** feeding `onGesture`. |
There was a problem hiding this comment.
Is this detail level really relevant when describing the architecture of the webview setup and how it maps onto the general architecture?
| | `FrontendEventHandler` | Compose UI | Emitting each `FrontendEvent` invokes the right host callback (navigate, snackbar, …). | | ||
| | `FrontendScreen` | Screenshot | Visual regression only (no logic). | | ||
|
|
||
| ## What to fix in the code to match this doc |
There was a problem hiding this comment.
This section is for an issue, not documentation. The documentation could have a basic note that not all screens match the architecture and to link to one or more issues, and welcome contributions after discussion.
3 participants
Proposed change
WIP
Type of change
Checklist
Additional information
Summary by CodeRabbit