Skip to content

Fix binding deletion when target node no longer exists #486

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.

Sign up for GitHub

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

Merged
Apollon77 merged 1 commit into from
Apr 2, 2026
Merged

Fix binding deletion when target node no longer exists #486

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions packages/dashboard/src/components/dialogs/binding/node-binding-dialog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { clientContext } from "../../../client/client-context.js";
import { handleAsync } from "../../../util/async-handler.js";
import { analyzeBatchResults, type MatterBatchResult } from "../../../util/matter-status.js";
import { preventDefault } from "../../../util/prevent_default.js";
import { showAlertDialog, showPromptDialog } from "../../dialog-box/show-dialog-box.js";
import {
AccessControlEntryDataTransformer,
AccessControlEntryStruct,
Expand Down Expand Up @@ -70,9 +71,39 @@ export class NodeBindingDialog extends LitElement {
const targetNodeId = rawBindings[index].node;
const endpoint = rawBindings[index].endpoint;
if (targetNodeId === undefined || endpoint === undefined) return;
await this.removeNodeAtACLEntry(this.node!.node_id, endpoint, targetNodeId);
const updatedBindings = this.removeBindingAtIndex(rawBindings, index);
await this.syncBindingUpdates(updatedBindings, index);
let aclCleanedUp = false;
try {
await this.removeNodeAtACLEntry(this.node!.node_id, endpoint, targetNodeId);
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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);
}
}

Copilot uses AI. Check for mistakes.
aclCleanedUp = true;
} catch (aclError) {
const errorMessage = aclError instanceof Error ? aclError.message : String(aclError);
const proceed = await showPromptDialog({
title: "ACL cleanup failed",
text:
`Could not clean up ACL on target node ${targetNodeId}: ${errorMessage}. ` +
"The target node may no longer exist or be unreachable. " +
"Do you want to remove the binding anyway? " +
"Note: The target device may retain an outdated ACL entry.",
confirmText: "Remove binding",
});
if (!proceed) return;
}
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."),
});
Comment on lines +91 to +104
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
return;
}
} catch (error) {
this.handleBindingDeletionError(error);
}
Expand Down
Loading