From 2b252c8a32cd1a8fbbe654ad7ae3169a43a47abb Mon Sep 17 00:00:00 2001 From: Carson Date: Tue, 21 Apr 2026 17:54:13 -0500 Subject: [PATCH] Fix pending output observer callbacks after unbind --- branch-review-findings.md | 73 +++++++++++++++++++++++ srcts/src/shiny/bind.ts | 11 ++++ srcts/src/shiny/index.ts | 3 + srcts/src/time/__tests__/debounce.test.ts | 18 ++++++ srcts/src/time/debounce.ts | 20 ++++++- srcts/types/src/time/debounce.d.ts | 6 +- 6 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 branch-review-findings.md create mode 100644 srcts/src/time/__tests__/debounce.test.ts diff --git a/branch-review-findings.md b/branch-review-findings.md new file mode 100644 index 000000000..9b38e907c --- /dev/null +++ b/branch-review-findings.md @@ -0,0 +1,73 @@ +# Branch Review Findings + +Review date: 2026-04-21 +Branch: `resize-observer-v2` +Reviewer: Codex + +## Status + +- [ ] Finding 1: Restore the guarantee that output clientdata is flushed before the next input batch is sent +- [x] Finding 2: Prevent post-unbind observer callbacks from emitting `.clientdata_output_null_*` inputs +- [ ] Simplification: Move output-info observer setup / flush / disposal behind a dedicated module API + +## Finding 1 + +Severity: High +Status: Open + +Summary: +The new observer path no longer guarantees that output clientdata is flushed before the next input batch is sent. + +Details: +The central `sendOutputInfoFns` debouncer is still wired into `InputBatchSender.lastChanceCallback`, but the per-element `ResizeObserver` and `IntersectionObserver` handlers in `srcts/src/shiny/index.ts` debounce for 100ms and then call `doSendHiddenState()` / `doSendSize()` directly, bypassing that flush path. + +Risk: +A tab/show/layout change followed immediately by a click, brush, or other input can reach the server with stale `.clientdata_output_*` width, height, or hidden values. + +Relevant files: +- `srcts/src/shiny/index.ts` +- `srcts/src/shiny/sendOutputInfo.ts` + +Notes: +- Old behavior routed these updates through debouncers that were explicitly flushed before send. + +## Finding 2 + +Severity: Medium +Status: Fixed on branch + +Summary: +Queued observer callbacks can survive unbind and emit bogus `.clientdata_output_null_*` inputs. + +Details: +`getIdFromEl()` now returns `null` when `shiny-output-binding` has already been removed, but the callbacks created in `ensureObservers()` have no cancellation mechanism. `unbindOutputs()` disconnects the observers, but it does not cancel already-scheduled debounced callbacks; when one fires after unbind, `doSendHiddenState()` / `doSendSize()` / `doSendTheme()` will build keys from `id === null`. + +Risk: +Dynamic UI replacement or removal can send invalid clientdata inputs after the output has already been unbound. + +Relevant files: +- `srcts/src/shiny/index.ts` +- `srcts/src/shiny/bind.ts` +- `srcts/src/time/debounce.ts` + +Resolution: +Added explicit cancellation support to the shared `debounce()` helper, stored the debounced observer callbacks alongside each observer, and canceled those callbacks during `unbindOutputs()` before removing the output binding data. + +## Simplification + +Status: Open + +Summary: +Most of the new output-info state machine still lives inline in `initialize()`. + +Opportunity: +Pull observer setup, flush behavior, and disposal behind one module-owned API so that lifecycle handling is centralized and easier to reason about. + +Relevant files: +- `srcts/src/shiny/index.ts` +- `srcts/src/shiny/sendOutputInfo.ts` + +## Verification + +- `npm run build_types` passed during review +- Browser-level regression coverage for this new output-info pipeline was not run diff --git a/srcts/src/shiny/bind.ts b/srcts/src/shiny/bind.ts index b3d5371f4..83e7bd709 100644 --- a/srcts/src/shiny/bind.ts +++ b/srcts/src/shiny/bind.ts @@ -442,6 +442,17 @@ function unbindOutputs( } } + for (const key of [ + "shiny-resize-observer-callback", + "shiny-intersection-observer-callback", + "shiny-mutate-observer-callback", + ]) { + const callback = $el.data(key) as { cancel?: () => void } | undefined; + + callback?.cancel?.(); + $el.removeData(key); + } + $el.trigger({ type: "shiny:unbound", // @ts-expect-error; Can not remove info on a established, malformed Event object diff --git a/srcts/src/shiny/index.ts b/srcts/src/shiny/index.ts index b4e5f6046..3445fbffe 100644 --- a/srcts/src/shiny/index.ts +++ b/srcts/src/shiny/index.ts @@ -433,6 +433,7 @@ class ShinyClass { const ro = new ResizeObserver(() => onResize()); ro.observe(el); + $(el).data("shiny-resize-observer-callback", onResize); $(el).data("shiny-resize-observer", ro); } @@ -447,6 +448,7 @@ class ShinyClass { const io = new IntersectionObserver(() => onIntersect()); io.observe(el); + $(el).data("shiny-intersection-observer-callback", onIntersect); $(el).data("shiny-intersection-observer", io); } @@ -464,6 +466,7 @@ class ShinyClass { }); $(el).data("shiny-mutate-observer", mo); + $(el).data("shiny-mutate-observer-callback", onMutate); } } diff --git a/srcts/src/time/__tests__/debounce.test.ts b/srcts/src/time/__tests__/debounce.test.ts new file mode 100644 index 000000000..b18fff86a --- /dev/null +++ b/srcts/src/time/__tests__/debounce.test.ts @@ -0,0 +1,18 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { debounce } from "../debounce"; + +test("debounce can cancel a pending callback before it fires", async () => { + let calls = 0; + const debounced = debounce(10, () => { + calls += 1; + }) as ReturnType & { cancel?: () => void }; + + debounced(); + debounced.cancel?.(); + + await new Promise((resolve) => setTimeout(resolve, 30)); + + assert.equal(calls, 0); +}); diff --git a/srcts/src/time/debounce.ts b/srcts/src/time/debounce.ts index f7359b412..b84b3f10d 100644 --- a/srcts/src/time/debounce.ts +++ b/srcts/src/time/debounce.ts @@ -70,15 +70,21 @@ class Debouncer implements InputRatePolicy { // 900ms intervals will result in a single execution // of the underlying function, 1000ms after the 17th // call. +type DebouncedFunction void> = (( + ...args: Parameters +) => void) & { + cancel: () => void; +}; + function debounce void>( threshold: number | undefined, func: T, -): (...args: Parameters) => void { +): DebouncedFunction { let timerId: ReturnType | null = null; // Do not alter `function()` into an arrow function. // The `this` context needs to be dynamically bound - return function thisFunc(...args: Parameters) { + const debounced = function thisFunc(...args: Parameters) { if (timerId !== null) { clearTimeout(timerId); timerId = null; @@ -92,6 +98,16 @@ function debounce void>( func.apply(thisFunc, args); }, threshold); }; + + debounced.cancel = function () { + if (timerId !== null) { + clearTimeout(timerId); + timerId = null; + } + }; + + return debounced; } export { debounce, Debouncer }; +export type { DebouncedFunction }; diff --git a/srcts/types/src/time/debounce.d.ts b/srcts/types/src/time/debounce.d.ts index 1f7970161..b46c3871a 100644 --- a/srcts/types/src/time/debounce.d.ts +++ b/srcts/types/src/time/debounce.d.ts @@ -14,5 +14,9 @@ declare class Debouncer implements InputRatePolicy $clearTimer(): void; $invoke(): void; } -declare function debounce void>(threshold: number | undefined, func: T): (...args: Parameters) => void; +type DebouncedFunction void> = ((...args: Parameters) => void) & { + cancel: () => void; +}; +declare function debounce void>(threshold: number | undefined, func: T): DebouncedFunction; export { debounce, Debouncer }; +export type { DebouncedFunction };