Fix reactivity#278
Draft
MackLiao wants to merge 18 commits intoBrentLab:devfrom
Draft
Conversation
…debar The comparison module's sidebar_controls @render.ui re-rendered all input widgets (top_n, effect_threshold, pvalue_threshold, facet_by) whenever the active dataset selection changed. Recreating the input widgets caused Shiny to emit spurious change events, invalidating the threshold reactive.calc wrappers and re-triggering _topn_data() 2-5x per user action. Per the performance report, ~78% of all _topn_data queries were intra-burst duplicates (264 fired vs ~50 needed). Move the four input widgets out of the dynamic @render.ui and into the static comparison_sidebar_ui module UI, seeded with DEFAULT_* constants. The dynamic block is replaced with a small empty_state_message render that emits only the empty-state banner. Inputs now have stable DOM identity across dataset toggles, so no spurious change events fire. Also adds DEFAULT_FACET_BY="binding" to queries.py so all four sidebar defaults live in one place, and routes the empty-state banner through components.empty_state(compact=True) per the CLAUDE.md convention. Verified against tmp/time_log.txt: comparison/_topn_data re-run factor drops from 2.4x avg / 4.2x worst to 1.0x exactly, and total DB time on the operation falls from 73.68s to 6.34s (~12x speedup).
The profile logger's log file was hardcoded to tfbpshiny_profile.log in the
project root, which forced an extra cp step before every analysis run that
expected tmp/time_log.txt. Add a TFBPSHINY_PROFILE_LOG_FILE env var that
overrides the path so captures can land directly where the analyzer reads.
Also emit one INFO line at startup naming both logger destinations
("Logger destinations — shiny: handler=... file=... | profiler: ..."), so
env-var configuration is verifiable from the first lines of stdout instead
of having to grep for output files. Defaults are unchanged: profile log
still goes to tfbpshiny_profile.log when no env var is set.
…ebars Apply the same static-input restructure to the binding and perturbation modules that was used for the comparison sidebar in 67e9d46. Both modules had identical bug patterns: a sidebar_controls @render.ui that recreated input_radio_buttons("col_preference") and input_radio_buttons("corr_type") whenever active_*_datasets() changed. The recreated widgets emitted spurious change events, invalidating the corr_type / col_preference reactive.calc wrappers and re-triggering downstream calcs unnecessarily. Per the latest profile capture, binding/scatter ran at 6x re-run factor and perturbation/scatter at 15x re-run factor — same root cause as comparison/_topn_data, but the bursts are smaller in absolute time so they were not the headline of the original report. The fix: - Move col_preference and corr_type radio buttons from the dynamic @render.ui in server/sidebar.py into the static module UI in ui.py, seeded with new DEFAULT_CORR_TYPE / DEFAULT_COL_PREFERENCE constants in each module's queries.py (mirroring the comparison/queries.py pattern). - Replace sidebar_controls with a small empty_state_message that emits only the empty-state banner when no datasets are active, routed through components.empty_state(compact=True). Verified: 64 unit tests still pass, all four module imports clean. Expect re-run factor on binding/scatter and perturbation/scatter to drop to 1.0x in the next profile capture, plus likely downstream improvement on binding/_all_corr_data and perturbation/_all_corr_data which depend on the same reactive wrappers.
Profile capture showed select_datasets/_matrix_data recomputing up to 5x per user action (10x re-run factor on the worst burst — 280 queries fired where ~28 would have sufficed). Root cause: the prior 0.3s debounce on _settled_datasets only coalesced keyboard-mash speed clicks. Normal mouse toggling has a click cadence of ~400-700ms, so each click fell outside the debounce window and fired a full N-dataset query matrix recompute. Bumping to 1.0s catches the typical multi-toggle session at the cost of ~0.7s extra latency before the matrix updates after the user stops clicking. Expected impact: collapses the 3-10x re-run bursts down to ~1x and saves ~12s of wasted DB time per session given the previous capture. Note: there is still a natural 2.0x floor inside _matrix_data because each pair fires two profile_span events (one per restricted-sample query). Eliminating that requires per-dataset / per-pair @reactive.calc memoization (PDF report rec BrentLab#2) — left as a separate larger change.
…race
Toggle-OFF fires two reactive writes in succession from
dataset_row._on_toggle:
1. toggle_state.set(...) - debounced 1s by _settled_datasets
2. dataset_filters.set(...) - clears the dataset's filters,
immediately invalidates _matrix_data
Because _matrix_data was reading dataset_filters() directly (no debounce),
the second write triggered a recompute with the pre-toggle dataset list
(still containing the just-toggled-off dataset, since _settled_datasets
hadn't yet expired) but the post-toggle filters. 1s later, when the
debounce expired, the matrix recomputed AGAIN with the correct list. UX
manifestation: the matrix flickering back and forth before settling, plus
asyncio CancelledError tracebacks from Shiny canceling the first
in-flight recompute when the second invalidation arrived.
Toggle-ON does not exhibit the race because it does not fire the second
filter write.
Fix: introduce a parallel _settled_filters debounced calc and have
_matrix_data read from it. Both invalidation sources now share the same
1s settle window, so toggle-off filter-clear and dataset-list updates
coalesce into a single matrix recompute. Other dataset_filters()
consumers (modal handlers, filter button styling) keep instant response.
…/perturbation
The previous fix moved the binding/perturbation SIDEBAR inputs to a static
declaration, but the regulator dropdown lives in the WORKSPACE and was
still being recreated by a @render.ui every time _all_corr_data
invalidated. Each recreation fired a spurious input.selected_regulator
change event, cascading into 6× and 15× re-runs of the downstream
scatter/regulator_plot queries (per the new analyzer capture).
Same root cause as the comparison sidebar fix and the prior binding/
perturbation sidebar fix: a dynamic @render.ui returning a freshly
constructed input widget. Same fix template, applied at the workspace
layer this time:
- Declare ui.input_selectize("selected_regulator", ...) statically in
binding/ui.py and perturbation/ui.py.
- Replace @render.ui regulator_selector with @reactive.effect
_update_regulator_choices that calls ui.update_selectize(...) to push
new choices and preserve the user's selection across data refreshes.
The pattern follows select_datasets/server/sidebar.py's existing use of
ui.update_selectize. The widget DOM persists across invalidations, so
spurious change events disappear and downstream calcs only re-run on
genuine user actions or genuine data changes.
Expected impact in the next profile capture:
- binding/scatter: 6× → 1× re-run factor
- binding/regulator_plot: 3× → 1× re-run factor
- perturbation/scatter: 15× → 1× re-run factor
- perturbation/regulator_plot: 15× → 1× re-run factor
UX side effect: when no datasets are selected the regulator dropdown is
visible-but-empty rather than absent. The empty-state messaging in the
sidebar already explains the user needs to select datasets, so this
should not be confusing.
…rrelation queries The previous regulator_selector fix used a @reactive.effect to push choices via ui.update_selectize. Effects run eagerly regardless of which page is mounted, so every dataset toggle on the Selection page caused both binding and perturbation modules to evaluate _all_corr_data() — firing the expensive correlation queries even when the user wasn't on those pages. That swamped the Selection page's debounce work and made toggle-off feel laggy again. Move the choices-push into a hidden @render.ui placeholder that is only mounted when the binding/perturbation workspace is active. The render machinery is lazy: when the placeholder isn't in the DOM, _all_corr_data isn't read, so no correlation queries fire. The placeholder returns an invisible span purely as a side-effect carrier; the real work (ui.update_selectize) happens inside the render function. The static input_selectize widget in workspace_ui still owns the DOM identity, so spurious change events stay eliminated when the user IS on the binding/perturbation page. Net effect: - Selection-page dataset toggles: no longer trigger binding/perturbation correlation queries (regression fix). - Binding/perturbation page interactions: still benefit from the static widget DOM (no spurious change events on _all_corr_data invalidation).
Shiny's default output suspension policy pauses @render.ui evaluation when the output is detected as hidden. Today the workspace_region in app.py wholesale-replaces its child output, so the trigger is either fully unmounted (correctly inactive) or fully mounted (correctly firing). But if a future layout change wraps the workspace in navset_tab, conditional_panel, or any display:none toggle, Shiny may classify the trigger as "hidden" and silently stop firing — breaking the regulator choices update with no error. Decorating the render with @output(suspend_when_hidden=False) opts out of that default and pins the side-effect to fire whenever its reactive deps invalidate AND the output is registered, regardless of visibility heuristics. The added comment also warns future contributors not to "clean up" by converting back to @reactive.effect, which would re-introduce the eager-execution regression that 1820565 fixed. Defensive change suggested by the multi-perspective review of the preceding three commits. No behavior change today; safety net for future layout work.
…ings Two cleanups from the tech-debt audit of the reactivity-fix branch: 1. Extract _SETTLED_WINDOW_SEC = 1.0 module constant in select_datasets/server/workspace.py and reference it from both _settled_datasets and _settled_filters debounce decorators. The two windows MUST stay equal for the toggle-off coalesce property to hold; making the coupling syntactically visible prevents accidental drift if either is retuned later. 2. Replace the stale "Render ... sidebar controls" summary in all three correlation-style module sidebar servers (comparison, binding, perturbation) with an accurate description. After the static-input refactor, these servers no longer render the controls — the static UI does. They only expose reactive accessors and render the empty-state banner. No behavior change.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
86818d3 refactor: extract _SETTLED_WINDOW_SEC and update stale sidebar docstrings
c9048ee fix: pin _regulator_choices_trigger with suspend_when_hidden=False
1820565 fix: convert regulator-choices effect to lazy render to stop eager correlation queries
82c7654 fix: stabilize regulator_selector DOM via update_selectize in binding/perturbation
79fe931 fix: debounce dataset_filters read in _matrix_data to fix toggle-off race
20c74d5 perf: bump _settled_datasets debounce 0.3s → 1.0s
9e2da44 fix: eliminate spurious reactivity bursts in binding/perturbation sidebars
64d6165 feat: TFBPSHINY_PROFILE_LOG_FILE env var and startup destination log
67e9d46 fix: eliminate spurious _topn_data reactivity bursts in comparison sidebar