Standardize MatterClient access via Lit context (#421)#599
Standardize MatterClient access via Lit context (#421)#599markvp wants to merge 2 commits intomatter-js:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Standardizes how the dashboard accesses the shared MatterClient by providing it via @lit/context from matter-dashboard-app, removing widespread .client prop drilling and simplifying dialog helper APIs.
Changes:
- Add a
clientContextprovider inmatter-dashboard-appand consume it across dashboard views/components. - Convert dialogs/components to
@consume({ context: clientContext, subscribe: true })and stop passing.clientthrough templates. - Remove
MatterClientparameters from dialog “show*Dialog” helpers and rely on context inside the dialogs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dashboard/src/pages/network/update-connections-dialog.ts | Switch dialog to consume MatterClient from clientContext instead of a passed .client prop. |
| packages/dashboard/src/pages/network/network-details.ts | Stop passing .client into <update-connections-dialog>. |
| packages/dashboard/src/pages/matter-server-view.ts | Consume MatterClient from context; stop passing .client into children like header/server-details. |
| packages/dashboard/src/pages/matter-node-view.ts | Consume MatterClient from context; remove .client prop drilling to header/node-details. |
| packages/dashboard/src/pages/matter-network-view.ts | Consume MatterClient from context; remove .client prop drilling to header. |
| packages/dashboard/src/pages/matter-endpoint-view.ts | Consume MatterClient from context; remove .client prop drilling to node-details/header. |
| packages/dashboard/src/pages/matter-dashboard-app.ts | Remove .client passing into routed views, relying on context instead. |
| packages/dashboard/src/pages/matter-cluster-view.ts | Consume MatterClient from context and rely on context for cluster command panels. |
| packages/dashboard/src/pages/components/server-details.ts | Consume MatterClient from context; update commission/import flows to use context-based dialogs. |
| packages/dashboard/src/pages/components/node-details.ts | Consume MatterClient from context; update binding dialog invocation signature. |
| packages/dashboard/src/pages/components/header.ts | Consume MatterClient from context; open settings dialog without passing client. |
| packages/dashboard/src/pages/cluster-commands/base-cluster-commands.ts | Base class now consumes MatterClient from context for all cluster command panels. |
| packages/dashboard/src/components/dialogs/settings/show-log-level-dialog.ts | Remove client arg; create dialog that consumes client via context. |
| packages/dashboard/src/components/dialogs/settings/log-level-dialog.ts | Consume MatterClient from context inside dialog. |
| packages/dashboard/src/components/dialogs/commission-node-dialog/show-commission-node-dialog.ts | Remove client arg; create dialog that consumes client via context. |
| packages/dashboard/src/components/dialogs/commission-node-dialog/commission-node-dialog.ts | Consume MatterClient from context inside dialog. |
| packages/dashboard/src/components/dialogs/binding/show-node-binding-dialog.ts | Remove client arg; create dialog that consumes client via context. |
|
fixes #421 |
Replace manual `.client` prop drilling across 17 dashboard components with a `ContextProvider` in `matter-dashboard-app` and `@consume` decorators in all consumers, so components receive the client automatically without explicit parent-to-child bindings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
provider.setValue is now called immediately after startListening resolves so consumers receive a non-undefined client before the first nodes_changed event. network-details was the only consumer missing subscribe: true, leaving it with a stale client if the provider value changed after initial render. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
markvp
force-pushed
the
feat/421-standardize-client-context
branch
from
264b4c9 to
dd7d943
Compare
| this.client.startListening().then( | ||
| () => { | ||
| this._state = "connected"; | ||
| this.provider.setValue(clone(this.client)); | ||
| this._setupEventListeners(); |
Apollon77
left a comment
Apollon77
left a comment
There was a problem hiding this comment.
Review summary
Great direction — context replaces the prop-drilling chain cleanly. Build/lint pass. Two correctness concerns and several consistency follow-ups before merge.
🔴 Critical
- Clone-induced
msgIddivergence → silent command collisions (see inline onmatter-dashboard-app.ts:111)
🟡 Important
- Settings dialog chain not migrated — partial standardization (inline on
header.ts) - Leftover
.client=bindings on now-consuming children (inline on cluster/endpoint/node-view + network-details) network-details.tssubscribe: false → truenot justified by code reads (inline)- Type lies:
client?: MatterClient→client!: MatterClientpaired with runtimeif (this.client)guards (inline on header + server-details)
⚪ Nits (no in-diff anchor)
1. ContextProvider(this, { context: clientContext, initialValue: this.client }) (matter-dashboard-app.ts:51) — field initializer runs during element construction; dashboard.client = client in entrypoint/main.ts runs after construction. So initialValue is always undefined. Reads as if seeding a value; doesn't. Drop initialValue or move provider creation into a setter/firstUpdated after assignment.
2. @consume({subscribe:true}) @property({attribute:false}) stacking — @property is technically redundant; @consume already triggers requestUpdate on subscribed value changes. Harmless but noise. Inconsistent across files (some have it, matter-cluster-view.ts etc. didn't originally). Pick one and unify.
3. packages/dashboard/src/util/clone_class.ts (not in diff but now load-bearing for context propagation):
- Uses
anyand lacks return type — violates CLAUDE.md "avoid type casts / use generics":export const clone = <T extends object>(orig: T): T => Object.assign(Object.create(Object.getPrototypeOf(orig)), orig);
- Shallow
Object.assignskips ES private fields (#x), accessor-backed state, and non-enumerable own properties. IfMatterClientever grows a#field, clone silently loses it. - If you fix the critical msgId concern by removing the clone-on-publish pattern (recommended), this file may become unused — preferable.
The msgId issue is the only blocker — others are polish to deliver the "standardize" promise the title makes.
| this.client.startListening().then( | ||
| () => { | ||
| this._state = "connected"; | ||
| this.provider.setValue(clone(this.client)); |
There was a problem hiding this comment.
🔴 Critical: clone(this.client) causes msgId divergence → silent command collisions.
MatterClient.msgId is a TS-private primitive (packages/ws-client/src/client.ts:55) incremented by sendCommand (client.ts:371). result_futures (client.ts:46) is a shared object reference. The shallow Object.assign(Object.create(proto), orig) in util/clone_class.ts copies msgId by value but shares result_futures by reference.
Failure path:
- Original sends
startListening→original.msgId = V. setValue(clone-A)published;clone-A.msgId = V. UI sends command →clone-A.msgId = V+1, writesresult_futures["V+1"] = {clone-A's resolve}.nodes_changedfires (frequent — every attribute storm) →setValue(clone-B)made fromoriginalwhosemsgIdis stillV→clone-B.msgId = V.- UI sends another command via
clone-B→clone-B.msgId = V+1, writesresult_futures["V+1"] = {clone-B's resolve}, overwriting clone-A's pending entry. - Server reply for the first command resolves clone-B's promise. Clone-A's promise hangs until
commandTimeout(default 5 minutes) and rejects withCommandTimeoutError.
Silent, intermittent, scales with UI activity during attribute updates.
Fix: don't snapshot the client. Keep the context value as the live MatterClient ref and trigger consumer re-renders via a separate signal — e.g. a tickContext that increments, a custom event on the host, or a Lit reactive controller. Same applies to the two setValue(clone(...)) calls in _setupEventListeners.
There was a problem hiding this comment.
Fixed — and updated further after reflection.
Initial fix replaced clone(this.client) with provider.setValue(this.client, true), which solved the identity problem but used force: true as an escape hatch rather than a proper design.
On review, your suggested approach is the right one. Switched to a dedicated tickContext:
clientContextis set once afterstartListening()resolves — a stable reference that never changes, so noforceor subscription needed.tickContextis a plain incrementing number, published on everynodes_changed/server_info_updatedevent. Consumers subscribe to this for re-renders.- The two concerns are now explicitly separated: stable ref for data, tick for change signal.
clone_class.ts is deleted. settings-dialog and log-level-dialog drop their client consume entirely — neither uses this.client directly after log-level-section migrated to context.
| public client?: MatterClient; | ||
| @consume({ context: clientContext, subscribe: true }) | ||
| @property({ attribute: false }) | ||
| public client!: MatterClient; |
There was a problem hiding this comment.
🟡 Inconsistent migration — settings dialog chain still prop-drills.
_openSettings (line 74) still calls showSettingsDialog(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@property
Sibling 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 with if (this.client) — runtime admits undefined. Original client?: MatterClient was more honest. Either keep ?: or remove the guards.
There was a problem hiding this comment.
Both addressed.
Settings chain: showSettingsDialog now takes no arguments. settings-dialog and log-level-section both use @consume({ context: clientContext, subscribe: true }). The dialog is appended to matter-dashboard-app's renderRoot, 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 + @property stacking).
| public client?: MatterClient; | ||
| @consume({ context: clientContext, subscribe: true }) | ||
| @property({ attribute: false }) | ||
| public client!: MatterClient; |
There was a problem hiding this comment.
Type lie: client!: MatterClient paired with if (!this.client) return html\`;on line 32. The runtime guard says it can beundefined; the !says it can't. Originalclient?: MatterClientwas correct. Pick a stance — the codebase is inconsistent (this file asserts,header.ts` guards).
There was a problem hiding this comment.
Fixed: client!: MatterClient → client?: MatterClient, @property({ attribute: false }) removed. The one access site not covered by the existing if (!this.client) return html``` guard — the importTestNodecall inside areader.onload` closure — was updated to use optional chaining.
| @@ -130,12 +133,11 @@ class MatterClusterView extends LitElement { | |||
| <dashboard-header | |||
| .title=${`Node ${this.node.node_id} ${nodeHex} | Endpoint ${this.endpoint} | Cluster ${this.cluster} (${clusterName})`} | |||
There was a problem hiding this comment.
Leftover .client= binding to flag — line 114 (the "Not found" branch) still passes .client=${this.client} to <dashboard-header>, even though <dashboard-header> now consumes from context. The success branch was cleaned up here; the not-found branch was missed. Drop for consistency.
| .title=${`Node ${this.node.node_id} ${nodeHex} | Endpoint ${this.endpoint}`} | ||
| .backButton=${`#node/${this.node.node_id}`} | ||
| .client=${this.client} | ||
| ></dashboard-header> |
There was a problem hiding this comment.
Leftover .client= binding — line 66 ("Not found" branch) still passes .client=${this.client} to <dashboard-header>. Success branch cleaned, not-found missed. Drop.
| .client=${this.client} | ||
| backButton="#" | ||
| ></dashboard-header> | ||
| <dashboard-header .title=${`Node ${this.node.node_id} ${nodeHex}`} backButton="#"></dashboard-header> |
There was a problem hiding this comment.
Leftover .client= binding — line 53 ("Node not found" branch) still passes .client=${this.client} to <dashboard-header>. Drop for consistency with the success branch.
| public threadEdgePairs: Map<string, ThreadEdgePair> = new Map(); | ||
|
|
||
| @consume({ context: clientContext }) | ||
| @consume({ context: clientContext, subscribe: true }) |
There was a problem hiding this comment.
🟡 subscribe: false → true is not justified by code reads in this file. Only client access is this.client?.serverInfo?.fabric_index (line 124). serverInfo is a getter that reads through to the shared connection.serverInfo object, mutated in-place on server_info_updated. All other reactive data flows in via props (nodes, borderRouters, etc.). Subscribing here only adds redundant re-renders on every nodes_changed (frequent). Revert to subscribe: false unless there's a specific case I'm missing.
Also: lines 1084 and ~1162 — there are still leftover .client=${this.client} bindings on <update-connections-dialog> in some branches even though that child now consumes from context. Drop them.
There was a problem hiding this comment.
Both addressed.
subscribe: reverted to subscribe: false (the default, so the option is just dropped). You're right — serverInfo is a getter over a mutated-in-place object and all other reactive data flows in via props; no subscription is warranted here.
Leftover .client=: the binding at line 1084 on <update-connections-dialog> in the border-router branch is removed. The instance at line ~1161 (node branch) never had it — that one was already clean.
|
Addressing the review body nits: 1. 2. 3. |
Summary
ContextProviderinmatter-dashboard-appthat broadcasts theMatterClientinstance to all descendant components via@lit/context.clientprop drilling to@consume({ context: clientContext, subscribe: true })decoratorsclientargument fromshowCommissionNodeDialog(),showLogLevelDialog(), andshowNodeBindingDialog()helpers — dialogs now receive the client from context automaticallyTest plan
Manual validation performed against a live server instance (
npm run server -- --storage-path data) with a diagnostics dump imported as test nodes:http://localhost:5580— server overview renders correctlydashboard-header→showLogLevelDialogcontext path)server-details.importTestNode)#node/<id>) renders node details correctly (matter-node-view,node-details)#node/<id>/<endpoint>) renders endpoint and cluster list (matter-endpoint-view,node-details)#node/<id>/<endpoint>/<cluster>) renders attributes and command panel (matter-cluster-view,base-cluster-commands)showCommissionNodeDialog)Build and lint:
npm run format— passesnpm run lint— 0 errors, 0 warningsnpm run build— passesCloses #421
🤖 Generated with Claude Code