This repository was archived by the owner on Oct 28, 2026. It is now read-only.
Repository navigation
[fix] Tear down Bokeh views before re-embedding a chart - #79
Merged
Merged
Conversation
sfc-gh-mbarnes
marked this pull request as ready for review
September 10, 2026 04:25
sfc-gh-lmasuch
approved these changes
Sep 11, 2026
Fixes streamlit/streamlit#15549: a hover tooltip visible when Streamlit reruns stays on screen afterwards, survives navigation to other pages, and only clears on a hard browser refresh. Clearing the container was never enough. `embed_item` builds a tree of Bokeh views, and some attach DOM outside the container: - `TooltipView._reposition` appends to `document.body.shadowRoot ?? document.body` and calls `showPopover()`, promoting the element to the browser's top layer. Nothing in Bokeh calls `hidePopover()`; the only takedown is removing the element. - The tooltip cannot clean itself up. That needs a `move_exit` to reach the hover tool, and `UIEventBus._trigger` returns early once its `hit_area` is disconnected -- which detaching the container guarantees. The model stays `visible: true` indefinitely. - Removing the element by hand does not stick: `UIElementView` observes its own element with a `ResizeObserver` and `_after_resize` calls `_reposition()`, so detaching resizes it to 0x0 and it re-appends and re-shows. It also re-anchors against a detached target, whose bounding box is zeros, which is why a stale tooltip lands at the top-left of the viewport. The tooltip is the visible symptom of a broader leak: every view, document scroll listener and ResizeObserver from a previous embed stayed alive. Measured in a real browser, registered root views grew 2, 4, 6, 8, 10, 12, 14 over six reruns, none destroyed. Bokeh already provides the teardown and the component discarded it: `embed_item` returns the `ViewManager` for what it built, and `clear()` calls `remove()` on each root, cascading through `children_views()` to the tooltip views. That return value is now kept per instance and cleared before the container is detached, so views unhook while their elements are still connected. v2's cleanup callback clears too -- previously unmounting leaked the same way, which is how a tooltip outlived page navigation. `destroyViews` is duck-typed on `clear` because `embed_item`'s return value is not documented surface: losing teardown in a future Bokeh should be a slow leak, not a broken chart. It does not, however, swallow an error from `clear()` itself -- a failed teardown surfaces rather than re-embedding over views in an unknown state. That is a deliberate call worth a reviewer's opinion, since it means a teardown exception would break rendering where today nothing is torn down at all. Regression tests assert the leak rather than the symptom, so they need no hover timing: root view count stays constant across reruns. A second test covers the reported case directly, rerunning by keyboard so the pointer never leaves the marker. Both are v2-only; v1 renders each chart in its own iframe, where the leak is confined and dies with the frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An independent review on a different model found two defects and a coverage gap,
all verified there by execution rather than reading.
**The tooltip e2e test never hit a marker.** Replaying the figure's geometry in a
real browser, the sweep at a fixed mid-canvas height landed on a marker 0 times
out of 5: y data of [6, 7, 2, 4, 5] plus axis padding clusters the markers near
the top of the canvas. The test would have taken its own
`pytest.skip("could not land the pointer on a marker")` on essentially every
run -- a silent no-op rather than an occasional flake. The app now draws one
large marker at the centre of fixed ranges with the axes hidden, so the canvas
centre is the marker, and the test asserts a tooltip appears instead of skipping
when it cannot raise one.
**A failed teardown wedged the chart for the session.** Letting an error from
`clear()` propagate was the wrong call. The caller only replaces its stored
manager from the `embed_item` that follows, so a throw meant the reference was
never replaced and every later render retried teardown on the same broken
manager. One hiccup broke the chart permanently, where the old behaviour merely
leaked. `destroyViews` now reports and swallows, degrading to the previous leaky
behaviour, which is the lesser failure.
**The wiring had no unit coverage, and it turned out to be testable.** Mocking
`./loaders` makes the v2 component reachable from a unit test -- almost certainly
why the existing tests only exercise the pure helpers. v1 needed a fresh module
per test plus its own component-lib instance from the same module graph, since
`vi.resetModules()` otherwise leaves the dispatch and the listener on different
event targets. Both now cover first render, teardown before re-embed, surviving
a failed teardown, and for v2 also unmount and two independent instances.
This closes a gap that mattered most for v1: the e2e tests skip there, because v1
renders each chart in its own iframe and the top-level page has no window.Bokeh,
so its 13 changed lines previously had no coverage of any kind. Verified all the
new tests fail with the fix neutered -- 4 of 5 in v2, 2 of 3 in v1, the
remainder being first-render invariants that should hold either way.
Also corrects the claim that teardown must precede detaching the container so
views can unhook while connected: nothing in Bokeh's remove() path branches on
that, so the order is conventional rather than load-bearing.
73 frontend tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sfc-gh-mbarnes
force-pushed
the
fix/bokeh-view-teardown
branch
from
September 11, 2026 22:25
76f744b to
c515083
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes streamlit/streamlit#15549: a Bokeh hover tooltip visible when Streamlit reruns stays on screen, survives navigation to other pages, and only clears on a hard refresh.
embed_itemreturns theViewManagerfor what it built, and this component discarded it — so every rerun orphaned the previous view tree. The tooltip is the visible symptom of a broader leak: measured across seven embeds in a real browser, root views went 2, 4, 6, 8, 10, 12, 14, liveResizeObservers 8, 16, 24, …, anddocumentscroll listeners 1, 2, 3, …. All three stay flat with the fix.Why the tooltip in particular
Three things compound, all verified against the shipped
bokeh-3.10.0.min.js:TooltipView._repositionappends the tooltip todocument.body, not the plot container, and shows it via the Popover API.hidePopoverappears zero times in Bokeh — the only takedown is removing the element.move_exitto reach the hover tool, andUIEventBus._triggerreturns early once itshit_areais disconnected, which detaching the container guarantees.UIElementViewobserves its own element with aResizeObserver, and_after_resizecalls_reposition()— so detaching resizes it to 0×0, which re-appends it. It also re-anchors against a detached target whose bounding box is zeros, which is why a stale tooltip lands top-left, as in the issue's screenshot.So sweeping
document.body > .bk-Tooltipdoes not work, and was verified not to. That's the obvious fix, and the leaked view undoes it.The fix
Keep the
ViewManager— per instance in v2'sComponentState, module-scoped in v1 — andclear()it before detaching the container. v2's cleanup callback clears too; unmounting leaked the same way, which is how a tooltip outlived page navigation.destroyViewsis duck-typed onclearand reports rather than rethrows a failure inside it.embed_item's return value isn't documented surface, so losing teardown should degrade to a leak, not a broken chart — and rethrowing was worse: the caller only replaces its stored manager from theembed_itemthat follows, so a throw meant retrying the same broken manager for the rest of the session.Worth a reviewer's attention
This changes teardown for every chart on every rerun, not just ones with tooltips. That breadth is what makes it durable rather than cosmetic, but the existing snapshot suite is the safety net for ordinary rendering.
Testing
73 frontend unit tests.
destroyViewscovers the happy path, wrong-shape no-ops, and degradation on failure. New wiring tests cover both entrypoints: first render, teardown before re-embed, surviving a failed teardown, plus unmount and two independent instances for v2. Verified they discriminate — 4 of 5 fail in v2 and 2 of 3 in v1 with the fix neutered, the rest being first-render invariants that hold either way.The wiring had no unit coverage before this; mocking
./loadersis what makes the v2 component reachable at all. That matters most for v1, whose 13 changed lines had no coverage of any kind, since the e2e tests are v2-only — v1 renders each chart in its own iframe, so the top-level page has nowindow.Bokeh.E2E, v2-only: one test asserts the leak (root view count constant across reruns, no hover timing involved), one covers the reported case, rerunning by keyboard so the pointer never leaves the marker.
Verified against the issue's actual repro — the 8-subplot
gridplotwith 10,000 points and eightHoverTools — in real Chromium: 2 stale tooltips survive teardown without the fix and 0 with it, root views4, 6, 8, 10versus flat2. That exercises teardown cascading from a GridPlot root through 8 child plots to 8 hover tools, which the single-figure tests can't show.Both e2e tests run in CI on the v2 leg (
streamlit 1.63.0 -> Custom Component v2, 77 passed) and skip on the v1 leg by design. They were also verified locally in both directions across chromium, firefox and webkit — 11 runs, no flakes — passing with the fix and failing without it (expected count '0', actual '1', the stale tooltip surviving).🤖 Generated with Claude Code