Fix binding deletion when target node no longer exists#486
Conversation
When deleting a binding, the dashboard first tries to clean up the ACL on the target node. If that node was already removed, the error blocked the binding removal entirely, leaving an orphaned binding that could never be deleted. Now the ACL cleanup failure shows a prompt letting the user decide whether to proceed. If the subsequent binding removal also fails, an alert informs the user about potential inconsistent state. Fixes #474 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the dashboard’s binding-deletion flow to handle the case where a binding’s target node has already been removed/unreachable, so users can still delete the binding on the source node instead of being blocked by ACL cleanup errors.
Changes:
- Wraps ACL cleanup in error handling and prompts the user to optionally proceed with binding removal when ACL cleanup fails.
- Adds a follow-up alert intended to warn about potential inconsistent state if binding removal fails after ACL changes.
| await this.syncBindingUpdates(updatedBindings, index); | ||
| let aclCleanedUp = false; | ||
| try { | ||
| await this.removeNodeAtACLEntry(this.node!.node_id, endpoint, targetNodeId); |
There was a problem hiding this comment.
aclCleanedUp is set based solely on whether removeNodeAtACLEntry(...) throws, but setACLEntry returns per-path status results (and the server converts many failures into a result with non-zero status rather than throwing). This can mark ACL cleanup as successful and skip the confirmation prompt even when the ACL write failed. Consider having removeNodeAtACLEntry return the write results and using analyzeBatchResults to treat non-all_success outcomes as a cleanup failure (prompt the user and keep aclCleanedUp accurate).
| await this.removeNodeAtACLEntry(this.node!.node_id, endpoint, targetNodeId); | |
| const aclResults = await this.removeNodeAtACLEntry(this.node!.node_id, endpoint, targetNodeId); | |
| if (aclResults) { | |
| const analysis = analyzeBatchResults(aclResults as MatterBatchResult[]); | |
| if (analysis.status !== "all_success") { | |
| const message = | |
| "ACL cleanup failed: " + (analysis.message ?? "One or more ACL updates failed."); | |
| throw new Error(message); | |
| } | |
| } |
| try { | ||
| const updatedBindings = this.removeBindingAtIndex(rawBindings, index); | ||
| await this.syncBindingUpdates(updatedBindings, index); | ||
| } catch (bindingError) { | ||
| const errorMessage = bindingError instanceof Error ? bindingError.message : String(bindingError); | ||
| await showAlertDialog({ | ||
| title: "Binding removal failed", | ||
| text: | ||
| `Failed to remove the binding: ${errorMessage}. ` + | ||
| (aclCleanedUp | ||
| ? "The ACL on the target device was already updated. " + | ||
| "The binding and ACL may now be out of sync." | ||
| : "No changes were made."), | ||
| }); |
There was a problem hiding this comment.
The try/catch around syncBindingUpdates(...) will only run if an exception is thrown, but setNodeBinding failures are typically returned as an AttributeWriteResult[] with non-zero status (not thrown). In those cases this code will still update this.node!.attributes as if the binding was removed, and the new "Binding removal failed" alert will never appear. Suggest updating syncBindingUpdates to analyze the returned results (via analyzeBatchResults) and only mutate local state when the write outcome is all_success (otherwise surface an error to the user).
Summary
Addresses #474
🤖 Generated with Claude Code