-
Notifications
You must be signed in to change notification settings - Fork 22
Standardize MatterClient access via Lit context (#421) #599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,18 +11,22 @@ import "@material/web/divider/divider"; | |
| import "@material/web/iconbutton/icon-button"; | ||
| import "@material/web/list/list"; | ||
| import "@material/web/list/list-item"; | ||
| import { consume } from "@lit/context"; | ||
| import { MatterClient } from "@matter-server/ws-client"; | ||
| import { mdiFile, mdiPlus } from "@mdi/js"; | ||
| import { LitElement, css, html, nothing } from "lit"; | ||
| import { customElement } from "lit/decorators.js"; | ||
| import { customElement, property } from "lit/decorators.js"; | ||
| import { clientContext } from "../../client/client-context.js"; | ||
| import { showAlertDialog, showPromptDialog } from "../../components/dialog-box/show-dialog-box.js"; | ||
| import { showCommissionNodeDialog } from "../../components/dialogs/commission-node-dialog/show-commission-node-dialog.js"; | ||
| import "../../components/ha-svg-icon"; | ||
| import { handleAsync } from "../../util/async-handler.js"; | ||
|
|
||
| @customElement("server-details") | ||
| export class ServerDetails extends LitElement { | ||
| public client?: MatterClient; | ||
| @consume({ context: clientContext, subscribe: true }) | ||
| @property({ attribute: false }) | ||
| public client!: MatterClient; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type lie:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: |
||
|
|
||
| protected override render() { | ||
| if (!this.client) return html``; | ||
|
|
@@ -71,7 +75,7 @@ export class ServerDetails extends LitElement { | |
| } | ||
|
|
||
| private _commissionNode() { | ||
| showCommissionNodeDialog(this.client!); | ||
| showCommissionNodeDialog(); | ||
| } | ||
|
|
||
| private async _uploadDiagnosticsDumpFile() { | ||
|
|
@@ -96,7 +100,7 @@ export class ServerDetails extends LitElement { | |
| reader.readAsText(selectedFile, "UTF-8"); | ||
| reader.onload = async () => { | ||
| try { | ||
| await this.client!.importTestNode(reader.result?.toString() ?? ""); | ||
| await this.client.importTestNode(reader.result?.toString() ?? ""); | ||
| } catch (err: any) { | ||
| showAlertDialog({ | ||
| title: "Failed to import test node", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { provide } from "@lit/context"; | ||
| import { consume, provide } from "@lit/context"; | ||
| import "@material/web/button/outlined-button"; | ||
| import "@material/web/divider/divider"; | ||
| import "@material/web/iconbutton/icon-button"; | ||
|
|
@@ -16,6 +16,7 @@ import { mdiAlertCircleOutline, mdiPencil, mdiPlay, mdiRefresh } from "@mdi/js"; | |
| import { css, html, LitElement, nothing, type TemplateResult } from "lit"; | ||
| import { customElement, property, state } from "lit/decorators.js"; | ||
| import { unsafeHTML } from "lit/directives/unsafe-html.js"; | ||
| import { clientContext } from "../client/client-context.js"; | ||
| import { clusters } from "../client/models/descriptions.js"; | ||
| import { showAlertDialog } from "../components/dialog-box/show-dialog-box.js"; | ||
| import { showAttributeWriteDialog } from "../components/dialogs/dev/show-attribute-write-dialog.js"; | ||
|
|
@@ -74,6 +75,8 @@ function clusterAttributes(attributes: { [key: string]: any }, endpoint: number, | |
|
|
||
| @customElement("matter-cluster-view") | ||
| class MatterClusterView extends LitElement { | ||
| @consume({ context: clientContext, subscribe: true }) | ||
| @property({ attribute: false }) | ||
| public client!: MatterClient; | ||
|
|
||
| @property() | ||
|
|
@@ -130,12 +133,11 @@ class MatterClusterView extends LitElement { | |
| <dashboard-header | ||
| .title=${`Node ${this.node.node_id} ${nodeHex} | Endpoint ${this.endpoint} | Cluster ${this.cluster} (${clusterName})`} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leftover
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
| .backButton=${`#node/${this.node.node_id}/${this.endpoint}`} | ||
| .client=${this.client} | ||
| ></dashboard-header> | ||
|
|
||
| <!-- node details section --> | ||
| <div class="container"> | ||
| <node-details .node=${this.node} .client=${this.client}></node-details> | ||
| <node-details .node=${this.node}></node-details> | ||
| </div> | ||
|
|
||
| <!-- Cluster commands section (if available for this cluster) --> | ||
|
|
@@ -375,8 +377,7 @@ class MatterClusterView extends LitElement { | |
| const container = this.shadowRoot?.getElementById("cluster-commands-container"); | ||
| if (container) { | ||
| const commandsElement = container.firstElementChild as any; | ||
| if (commandsElement && this.node && this.client) { | ||
| commandsElement.client = this.client; | ||
| if (commandsElement && this.node) { | ||
| commandsElement.node = this.node; | ||
| commandsElement.endpoint = this.endpoint; | ||
| commandsElement.cluster = this.cluster; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,7 @@ class MatterDashboardApp extends LitElement { | |
| this.client.startListening().then( | ||
| () => { | ||
| this._state = "connected"; | ||
| this.provider.setValue(clone(this.client)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Critical:
Failure path:
Silent, intermittent, scales with UI activity during attribute updates. Fix: don't snapshot the client. Keep the context value as the live
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — and updated further after reflection. Initial fix replaced On review, your suggested approach is the right one. Switched to a dedicated
|
||
| this._setupEventListeners(); | ||
|
Comment on lines
108
to
112
|
||
| }, | ||
| (_err: MatterError) => { | ||
|
|
@@ -198,7 +199,6 @@ class MatterDashboardApp extends LitElement { | |
| // cluster level | ||
| return html` | ||
| <matter-cluster-view | ||
| .client=${this.client} | ||
| .node=${this.client.nodes[this._route.path[0]]} | ||
| .endpoint=${parseInt(this._route.path[1], 10)} | ||
| .cluster=${parseInt(this._route.path[2], 10)} | ||
|
markvp marked this conversation as resolved.
|
||
|
|
@@ -209,28 +209,21 @@ class MatterDashboardApp extends LitElement { | |
| // endpoint level | ||
| return html` | ||
| <matter-endpoint-view | ||
| .client=${this.client} | ||
| .node=${this.client.nodes[this._route.path[0]]} | ||
| .endpoint=${parseInt(this._route.path[1], 10)} | ||
| ></matter-endpoint-view> | ||
| `; | ||
| } | ||
| if (this._route.prefix === "node") { | ||
| // node level | ||
| return html` | ||
| <matter-node-view | ||
| .client=${this.client} | ||
| .node=${this.client.nodes[this._route.path[0]]} | ||
| ></matter-node-view> | ||
| `; | ||
| return html` <matter-node-view .node=${this.client.nodes[this._route.path[0]]}></matter-node-view> `; | ||
| } | ||
| // Get device counts for conditional navigation | ||
| const { hasThreadDevices, hasWifiDevices } = this._getDeviceCounts(); | ||
|
|
||
| // Check for Thread view (#thread or #thread/123) | ||
| if (this._route.prefix === "thread" || this._route.path[0] === "thread") { | ||
| return html`<matter-network-view | ||
| .client=${this.client} | ||
| .nodes=${this.client.nodes} | ||
| .activeView=${this._activeView} | ||
| .initialSelectedNodeId=${this._initialSelectedNodeId} | ||
|
|
@@ -242,7 +235,6 @@ class MatterDashboardApp extends LitElement { | |
| // Check for WiFi view (#wifi or #wifi/123) | ||
| if (this._route.prefix === "wifi" || this._route.path[0] === "wifi") { | ||
| return html`<matter-network-view | ||
| .client=${this.client} | ||
| .nodes=${this.client.nodes} | ||
| .activeView=${this._activeView} | ||
| .initialSelectedNodeId=${this._initialSelectedNodeId} | ||
|
|
@@ -253,7 +245,6 @@ class MatterDashboardApp extends LitElement { | |
| } | ||
| // root level: server overview (nodes view) | ||
| return html`<matter-server-view | ||
| .client=${this.client} | ||
| .nodes=${this.client.nodes} | ||
| .route=${this._route} | ||
| .activeView=${this._activeView} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,11 +9,13 @@ import "@material/web/divider/divider"; | |
| import "@material/web/iconbutton/icon-button"; | ||
| import "@material/web/list/list"; | ||
| import "@material/web/list/list-item"; | ||
| import { consume } from "@lit/context"; | ||
| import { MatterClient, MatterNode, isTestNodeId } from "@matter-server/ws-client"; | ||
| import { mdiAlertCircleOutline, mdiChevronRight } from "@mdi/js"; | ||
| import { LitElement, css, html } from "lit"; | ||
| import { customElement, property } from "lit/decorators.js"; | ||
| import { guard } from "lit/directives/guard.js"; | ||
| import { clientContext } from "../client/client-context.js"; | ||
| import { DeviceType, clusters, device_types } from "../client/models/descriptions.js"; | ||
| import "../components/ha-svg-icon"; | ||
| import { formatHex, formatNodeAddress, getEffectiveFabricIndex } from "../util/format_hex.js"; | ||
|
|
@@ -48,6 +50,8 @@ export function getEndpointDeviceTypes(node: MatterNode, endpoint: number): Devi | |
|
|
||
| @customElement("matter-endpoint-view") | ||
| class MatterEndpointView extends LitElement { | ||
| @consume({ context: clientContext, subscribe: true }) | ||
| @property({ attribute: false }) | ||
| public client!: MatterClient; | ||
|
|
||
| @property() | ||
|
|
@@ -79,12 +83,11 @@ class MatterEndpointView extends LitElement { | |
| <dashboard-header | ||
| .title=${`Node ${this.node.node_id} ${nodeHex} | Endpoint ${this.endpoint}`} | ||
| .backButton=${`#node/${this.node.node_id}`} | ||
| .client=${this.client} | ||
| ></dashboard-header> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leftover
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
|
|
||
| <!-- node details section --> | ||
| <div class="container"> | ||
| <node-details .node=${this.node} .client=${this.client}></node-details> | ||
| <node-details .node=${this.node}></node-details> | ||
| </div> | ||
|
|
||
| <!-- Endpoint clusters listing --> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Inconsistent migration — settings dialog chain still prop-drills.
_openSettings(line 74) still callsshowSettingsDialog(this.client). Inside that chain:show-settings-dialog.ts:9-12— receivesclient, assignsdialog.client = clientsettings-dialog.ts:23— bare@property(no@consume)settings-dialog.ts:92— passes.client=${this.client}to<log-level-section>log-level-section.ts— bare@propertySibling dialogs (commission/log-level/binding) were migrated. Either finish the settings chain for consistency or scope-clarify in the PR description.
Also a type-lie nit on this line:
client!: MatterClient(non-null assertion) but lines 73, 165, 178, 190 all guard withif (this.client)— runtime admitsundefined. Originalclient?: MatterClientwas more honest. Either keep?:or remove the guards.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both addressed.
Settings chain:
showSettingsDialognow takes no arguments.settings-dialogandlog-level-sectionboth use@consume({ context: clientContext, subscribe: true }). The dialog is appended tomatter-dashboard-app'srenderRoot, so it sits inside the provider's shadow tree and receives context naturally — consistent with how commission/log-level/binding dialogs were already handled.Type lie:
client!: MatterClient→client?: MatterClient,@property({ attribute: false })removed (see nit below on@consume + @propertystacking).