From bdd33decf4b8fa1482b824d494fb0234009f949d Mon Sep 17 00:00:00 2001 From: Costa Tsaousis Date: Mon, 18 May 2026 11:38:32 +0300 Subject: [PATCH 1/2] Add multi-consumer pause-reason API to SDK root nodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `paused` attribute on the SDK root used to be set directly by each independent caller — a UI hover toggle, an open modal, a long-running export. Whichever caller flipped last won, and the "last flipped" was often the wrong one: e.g., when the cursor moved from the topology canvas onto an open actor modal, the topology hover-end fired and wrote `paused: false`, releasing the modal's still-active pause. Updates resumed underneath the modal, re-fetched data, and the tables / mini-topology jittered every tick. Fix: the SDK root now owns a private `pauseReasons` Set and exposes `addPauseReason(reasonId)` / `removePauseReason(reasonId)`. The `paused` attribute is recomputed as `blurReason || pauseReasons.size > 0` whenever a reason is added / removed OR when the blur sources (`blurred`, `autofetchOnWindowBlur`) change. Reasons compose: hover-end removes its own id and leaves the modal's id in place, so the aggregate stays true while the modal is open. Idempotent on the same reason id, no-op for empty/missing ids, fully cleared on `destroy()`. 8 new tests in `makeNode.test.js` cover the aggregation, blur combination, idempotency, and the original clobber-bug regression (remove-one keeps aggregate true while another remains). Downstream: `@netdata/cloud-frontend`'s `usePauseSDK` hook moves from its own module-level Set to this SDK API in a parallel PR. --- src/sdk/makeNode.js | 37 +++++++++++++++++++++ src/sdk/makeNode.test.js | 71 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/src/sdk/makeNode.js b/src/sdk/makeNode.js index 95b27642..cd25d5de 100644 --- a/src/sdk/makeNode.js +++ b/src/sdk/makeNode.js @@ -179,11 +179,46 @@ export default ({ sdk, parent = null, attributes: initialAttributes }) => { onAttributeChange("timezone", tz => updateIntls(tz, getAttribute("locale"))) onAttributeChange("locale", locale => updateIntls(getAttribute("timezone"), locale)) + // Multi-consumer pause registry. Each independent caller (a UI + // hover handler, an open modal, a long-running export, etc.) can + // register a reason id when it wants the SDK to hold updates and + // unregister it when done. The `paused` attribute is recomputed as + // the union of all reasons + the blur fallback, so consumers + // compose without overwriting each other's intent. Without this, + // every caller writing `paused` directly on its own intent could + // clobber another caller's still-active pause (e.g., a hover-end + // releasing the pause while a modal is still open). + const pauseReasons = new Set() + const computeAggregatePaused = () => { + const blurReason = !getAttribute("autofetchOnWindowBlur") && getAttribute("blurred") + return Boolean(blurReason) || pauseReasons.size > 0 + } + const applyAggregatePaused = () => { + updateAttribute("paused", computeAggregatePaused()) + } + const addPauseReason = reasonId => { + if (!reasonId) return + pauseReasons.add(reasonId) + applyAggregatePaused() + } + const removePauseReason = reasonId => { + if (!reasonId) return + if (!pauseReasons.delete(reasonId)) return + applyAggregatePaused() + } + // The aggregate also reacts to blur-source attribute changes + // automatically — callers that have already registered a reason + // don't have to subscribe separately to recompute when window + // focus flips. + onAttributeChange("blurred", applyAggregatePaused) + onAttributeChange("autofetchOnWindowBlur", applyAggregatePaused) + const destroy = () => { if (parent) parent.removeChild(getId()) listeners.offAll() attributeListeners.offAll() + pauseReasons.clear() setTimeout(() => (parent = null), 2000) destroyIntls() } @@ -220,6 +255,8 @@ export default ({ sdk, parent = null, attributes: initialAttributes }) => { formatTime, formatDate, formatXAxis, + addPauseReason, + removePauseReason, } return instance diff --git a/src/sdk/makeNode.test.js b/src/sdk/makeNode.test.js index efbaef05..99601015 100644 --- a/src/sdk/makeNode.test.js +++ b/src/sdk/makeNode.test.js @@ -238,6 +238,77 @@ describe("makeNode", () => { }) }) + describe("pause reasons", () => { + it("starts with no reasons and paused=false (assuming blur is off)", () => { + expect(node.getAttribute("paused")).toBeUndefined() + }) + + it("addPauseReason flips paused to true", () => { + node.addPauseReason("hover") + expect(node.getAttribute("paused")).toBe(true) + }) + + it("removePauseReason after the only reason flips paused back to false", () => { + node.addPauseReason("hover") + node.removePauseReason("hover") + expect(node.getAttribute("paused")).toBe(false) + }) + + it("composes multiple reasons — removing one keeps paused true while others remain", () => { + node.addPauseReason("hover") + node.addPauseReason("modal") + expect(node.getAttribute("paused")).toBe(true) + node.removePauseReason("hover") + // The bug this fix targets: hover-end MUST NOT clobber the + // still-active modal pause. Without the multi-consumer + // aggregate this would have flipped to false here. + expect(node.getAttribute("paused")).toBe(true) + node.removePauseReason("modal") + expect(node.getAttribute("paused")).toBe(false) + }) + + it("is idempotent — same reason id added twice acts as one", () => { + node.addPauseReason("hover") + node.addPauseReason("hover") + node.removePauseReason("hover") + expect(node.getAttribute("paused")).toBe(false) + }) + + it("ignores empty/missing reason ids", () => { + node.addPauseReason("") + node.addPauseReason(null) + node.addPauseReason(undefined) + expect(node.getAttribute("paused")).toBeUndefined() + }) + + it("recomputes when the blur attributes flip — window blur fires pause without an explicit reason", () => { + // autofetchOnWindowBlur=false means blur should pause; setting + // blurred=true should make the aggregate true. + node.updateAttribute("autofetchOnWindowBlur", false) + node.updateAttribute("blurred", true) + expect(node.getAttribute("paused")).toBe(true) + node.updateAttribute("blurred", false) + expect(node.getAttribute("paused")).toBe(false) + }) + + it("blur and explicit reasons combine — un-blurring keeps paused true if a reason is still registered", () => { + node.updateAttribute("autofetchOnWindowBlur", false) + node.updateAttribute("blurred", true) + node.addPauseReason("modal") + expect(node.getAttribute("paused")).toBe(true) + node.updateAttribute("blurred", false) + expect(node.getAttribute("paused")).toBe(true) + node.removePauseReason("modal") + expect(node.getAttribute("paused")).toBe(false) + }) + + it("autofetchOnWindowBlur=true disables the blur reason — paused stays false while blurred", () => { + node.updateAttribute("autofetchOnWindowBlur", true) + node.updateAttribute("blurred", true) + expect(node.getAttribute("paused")).toBe(false) + }) + }) + describe("inheritance", () => { it("inherits parent attributes", () => { mockParent.getAttributes.mockReturnValue({ From 7a4957a92d29b5907e0a6ffb0ebadcc55e1d7498 Mon Sep 17 00:00:00 2001 From: novykh Date: Tue, 19 May 2026 12:35:32 +0300 Subject: [PATCH 2/2] Small changes after review. --- src/sdk/makeNode.js | 13 ------------- src/sdk/makeNode.test.js | 5 ----- 2 files changed, 18 deletions(-) diff --git a/src/sdk/makeNode.js b/src/sdk/makeNode.js index cd25d5de..4bf5c1a2 100644 --- a/src/sdk/makeNode.js +++ b/src/sdk/makeNode.js @@ -179,15 +179,6 @@ export default ({ sdk, parent = null, attributes: initialAttributes }) => { onAttributeChange("timezone", tz => updateIntls(tz, getAttribute("locale"))) onAttributeChange("locale", locale => updateIntls(getAttribute("timezone"), locale)) - // Multi-consumer pause registry. Each independent caller (a UI - // hover handler, an open modal, a long-running export, etc.) can - // register a reason id when it wants the SDK to hold updates and - // unregister it when done. The `paused` attribute is recomputed as - // the union of all reasons + the blur fallback, so consumers - // compose without overwriting each other's intent. Without this, - // every caller writing `paused` directly on its own intent could - // clobber another caller's still-active pause (e.g., a hover-end - // releasing the pause while a modal is still open). const pauseReasons = new Set() const computeAggregatePaused = () => { const blurReason = !getAttribute("autofetchOnWindowBlur") && getAttribute("blurred") @@ -206,10 +197,6 @@ export default ({ sdk, parent = null, attributes: initialAttributes }) => { if (!pauseReasons.delete(reasonId)) return applyAggregatePaused() } - // The aggregate also reacts to blur-source attribute changes - // automatically — callers that have already registered a reason - // don't have to subscribe separately to recompute when window - // focus flips. onAttributeChange("blurred", applyAggregatePaused) onAttributeChange("autofetchOnWindowBlur", applyAggregatePaused) diff --git a/src/sdk/makeNode.test.js b/src/sdk/makeNode.test.js index 99601015..fb227d87 100644 --- a/src/sdk/makeNode.test.js +++ b/src/sdk/makeNode.test.js @@ -259,9 +259,6 @@ describe("makeNode", () => { node.addPauseReason("modal") expect(node.getAttribute("paused")).toBe(true) node.removePauseReason("hover") - // The bug this fix targets: hover-end MUST NOT clobber the - // still-active modal pause. Without the multi-consumer - // aggregate this would have flipped to false here. expect(node.getAttribute("paused")).toBe(true) node.removePauseReason("modal") expect(node.getAttribute("paused")).toBe(false) @@ -282,8 +279,6 @@ describe("makeNode", () => { }) it("recomputes when the blur attributes flip — window blur fires pause without an explicit reason", () => { - // autofetchOnWindowBlur=false means blur should pause; setting - // blurred=true should make the aggregate true. node.updateAttribute("autofetchOnWindowBlur", false) node.updateAttribute("blurred", true) expect(node.getAttribute("paused")).toBe(true)