fix(vegas): restore live plugin-update refresh dropped by sync refactor#395
Conversation
Investigating a user report that Vegas scroll mode doesn't update scores or game status. Root cause: PR #299 (Mar 28) added a mechanism so a live score change reached the ticker within a few seconds instead of waiting for a full scroll cycle -- _tick_plugin_updates_for_vegas() diffed plugin_last_update timestamps to detect which plugins got fresh data and called coordinator.mark_plugin_updated() for each, and should_recompose() checked has_pending_updates_for_visible_segments() to trigger an immediate hot-swap. PR #330 (May 14, multi-display wireless sync) refactored both call sites while adding sync support and silently deleted this entire mechanism -- not just gated it behind the new sync-mode deferral it legitimately needed, but removed it outright. The result: VegasModeCoordinator. mark_plugin_updated() and StreamManager.has_pending_updates_for_visible_ segments() have been fully implemented but never called from anywhere since. Vegas mode's only remaining freshness sources are a 5s content cache TTL (fine) and full recompose at cycle boundaries, which depending on min/max_cycle_duration can be minutes away -- so live scores/status can sit stale far longer than a user would expect from a "live" ticker. Fix: - Restored _tick_plugin_updates_for_vegas() in display_controller.py, wired as the Vegas coordinator's update callback in place of the plain _tick_plugin_updates(). Diffs plugin_last_update before/after the tick and calls vegas_coordinator.mark_plugin_updated(plugin_id) for each plugin that actually got new data (rather than returning the list, since the callback interface no longer consumes a return value). - Restored the has_pending_updates_for_visible_segments() check in render_pipeline.should_recompose(), positioned after (not instead of) the sync-mode early return PR #330 added, so standalone installations regain immediate refresh while synced leader/follower pairs correctly keep deferring hot-swaps to cycle boundaries as PR #330 intended. Test plan: - Added test_display_controller_vegas_tick.py and test_vegas_render_pipeline_recompose.py -- neither area had any prior test coverage, which is very likely why this regression went unnoticed for ~2.5 months. - Verified both new test files fail against the pre-fix code (swapped in the current main versions of both files) with exactly the expected errors -- AttributeError for the deleted method, and the recompose assertion returning False instead of True -- then pass against the fix. - Confirmed the sync-mode deferral this restoration must not break still holds: test_sync_active_defers_pending_updates_to_cycle_boundary. - Full related suite (test_vegas_plugin_adapter, test_vegas_config, test_display_controller_plugin_toggle, test_display_controller_ optimizations, test_plugin_system): 108 passed, 1 pre-existing failure unrelated to this change (test_circuit_breaker, stale mock signature). - Full CI plugin-safety suite (test_harness, test_visual_rendering, test_plugin_matrix): 52 passed, 2 pre-existing skips.
📝 WalkthroughWalkthroughVegas plugin update ticks now notify the coordinator when plugin timestamps advance. Standalone rendering also recomposes when visible segments have pending updates, while synchronized rendering preserves cycle-based recomposition behavior. Regression tests cover both paths and their edge cases. ChangesVegas update flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/display_controller.py (1)
834-868: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the return annotation and narrow the exception boundary.
The new method lacks a
-> Nonereturn annotation, andexcept Exceptioncan silently suppress unrelated programming errors frommark_plugin_updated(). Keep per-plugin isolation, but catch only documented recoverable exceptions, including theRuntimeErrorintentionally covered by the test.As per coding guidelines, Python code must use type hints and catch specific exceptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/display_controller.py` around lines 834 - 868, Add the -> None return annotation to _tick_plugin_updates_for_vegas. Preserve per-plugin isolation while replacing the broad Exception handler around vc.mark_plugin_updated(plugin_id) with only the documented recoverable exceptions, including RuntimeError covered by the test.Source: Coding guidelines
test/test_display_controller_vegas_tick.py (2)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new test helper.
Add parameter and return annotations to
_make_controller()so the dynamically constructed controller and timestamp mapping have an explicit contract.As per coding guidelines, Python functions should use type hints for parameters and return values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_display_controller_vegas_tick.py` around lines 19 - 24, Update the _make_controller test helper with explicit type annotations for plugin_last_update, vegas_coordinator, and its returned DisplayController, using the project’s existing timestamp and coordinator types where available. Preserve the helper’s current construction and initialization behavior.Source: Coding guidelines
27-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for callback wiring.
These tests call
_tick_plugin_updates_for_vegas()directly, so Line 508 could regress back to_tick_plugin_updateswhile every test here still passes. Add a focused test with a fake coordinator asserting thatset_update_callback()receives the Vegas-aware bound method.The PR objective explicitly includes restoring the coordinator callback wiring.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_display_controller_vegas_tick.py` around lines 27 - 76, Add a focused regression test for the controller’s coordinator callback wiring, using a fake coordinator to capture set_update_callback and asserting it receives the Vegas-aware bound method _tick_plugin_updates_for_vegas rather than _tick_plugin_updates. Keep the test independent of direct invocation tests and verify the callback is registered during controller setup.test/test_vegas_render_pipeline_recompose.py (2)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new test helper.
Add parameter and return annotations to
_make_pipeline()to make its test fixture contract explicit.As per coding guidelines, Python functions should use type hints for parameters and return values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_vegas_render_pipeline_recompose.py` around lines 25 - 30, Update the test helper _make_pipeline to add a type annotation for its optional sync_manager parameter and an explicit return annotation describing the returned pipeline and stream_manager tuple, preserving its existing behavior.Source: Coding guidelines
25-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the production-shaped standalone sync state.
All standalone cases use
sync_manager=None, but production creates a standaloneDisplaySyncManager. Add a test with an inactive/standalone manager (or equivalent fake) so a mistakenis not Nonecheck cannot suppress standalone recomposition while these tests remain green.The supplied controller context creates a sync manager even for standalone deployments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_vegas_render_pipeline_recompose.py` around lines 25 - 30, Extend _make_pipeline or add a focused test to construct the standalone pipeline with an inactive DisplaySyncManager instead of sync_manager=None. Exercise the standalone recomposition path and assert recomposition still occurs, ensuring the implementation does not rely on a non-None manager to distinguish synchronized deployments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/display_controller.py`:
- Around line 847-861: Update the plugin update flow around DisplayController’s
plugin_last_update snapshot and _tick_plugin_updates call to use a new locked
PluginManager helper that atomically snapshots prior timestamps, performs the
update, computes changed plugin IDs, and returns them. Ensure all reads,
mutations, and iteration of plugin_last_update occur under the same lock, then
use the helper’s returned IDs for the Vegas notification path.
---
Nitpick comments:
In `@src/display_controller.py`:
- Around line 834-868: Add the -> None return annotation to
_tick_plugin_updates_for_vegas. Preserve per-plugin isolation while replacing
the broad Exception handler around vc.mark_plugin_updated(plugin_id) with only
the documented recoverable exceptions, including RuntimeError covered by the
test.
In `@test/test_display_controller_vegas_tick.py`:
- Around line 19-24: Update the _make_controller test helper with explicit type
annotations for plugin_last_update, vegas_coordinator, and its returned
DisplayController, using the project’s existing timestamp and coordinator types
where available. Preserve the helper’s current construction and initialization
behavior.
- Around line 27-76: Add a focused regression test for the controller’s
coordinator callback wiring, using a fake coordinator to capture
set_update_callback and asserting it receives the Vegas-aware bound method
_tick_plugin_updates_for_vegas rather than _tick_plugin_updates. Keep the test
independent of direct invocation tests and verify the callback is registered
during controller setup.
In `@test/test_vegas_render_pipeline_recompose.py`:
- Around line 25-30: Update the test helper _make_pipeline to add a type
annotation for its optional sync_manager parameter and an explicit return
annotation describing the returned pipeline and stream_manager tuple, preserving
its existing behavior.
- Around line 25-30: Extend _make_pipeline or add a focused test to construct
the standalone pipeline with an inactive DisplaySyncManager instead of
sync_manager=None. Exercise the standalone recomposition path and assert
recomposition still occurs, ensuring the implementation does not rely on a
non-None manager to distinguish synchronized deployments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11685edd-a606-4d1f-b292-94a14654b3be
📒 Files selected for processing (4)
src/display_controller.pysrc/vegas_mode/render_pipeline.pytest/test_display_controller_vegas_tick.pytest/test_vegas_render_pipeline_recompose.py
1 participant
Summary
Investigating a user report that Vegas scroll mode doesn't update scores or
game status.
Root cause: PR #299
(Mar 28, "refresh scroll buffer on live score updates") added a mechanism so
a live score change reached the ticker within a few seconds instead of
waiting for a full scroll cycle:
_tick_plugin_updates_for_vegas()indisplay_controller.pydiffedplugin_last_updatetimestamps to detect which plugins got fresh data oneach tick, and called
coordinator.mark_plugin_updated()for each.render_pipeline.should_recompose()checkedstream_manager.has_pending_updates_for_visible_segments()to trigger animmediate hot-swap.
PR #330 (May 14,
multi-display wireless sync) refactored both call sites while adding sync
support, and silently deleted this entire mechanism — not gated behind
the new sync-mode deferral it legitimately needed, but removed outright.
Result:
VegasModeCoordinator.mark_plugin_updated()andStreamManager.has_pending_updates_for_visible_segments()are both stillfully implemented and correct — just orphaned, called from nowhere, for the
past ~2.5 months. Vegas mode's only remaining freshness sources are a 5s
content cache TTL (fine, imperceptible) and a full recompose at cycle
boundaries, which depending on
min_cycle_duration/max_cycle_durationcanbe minutes away. So a live score genuinely can sit stale on screen far
longer than a "live" ticker should — matching the reported symptom exactly.
Fix
_tick_plugin_updates_for_vegas(), wired as the Vegascoordinator's update callback in place of the plain
_tick_plugin_updates(). Diffsplugin_last_updatebefore/after the tickand calls
vegas_coordinator.mark_plugin_updated(plugin_id)for eachplugin that actually got new data (rather than returning a list for a
caller to act on, since the callback interface no longer consumes a
return value the way it might have originally).
has_pending_updates_for_visible_segments()check inshould_recompose(), positioned after (not instead of) the sync-modeearly return feat(sync): multi-display wireless sync — extend scrolling across two LED matrices #330 added — so standalone installations regain immediate
refresh, while synced leader/follower pairs correctly keep deferring
hot-swaps to cycle boundaries exactly as feat(sync): multi-display wireless sync — extend scrolling across two LED matrices #330 intended (that part of feat(sync): multi-display wireless sync — extend scrolling across two LED matrices #330
was a legitimate, deliberate design choice, not a bug).
Test plan
test_display_controller_vegas_tick.pyandtest_vegas_render_pipeline_recompose.py— neither area had any priortest coverage, which is very likely why this regression went unnoticed
for two and a half months.
the current
mainversions of both touched files) with exactly theexpected errors —
AttributeErrorfor the deleted method, and therecompose assertion returning
Falseinstead ofTrue— then passagainst the fix.
holds:
test_sync_active_defers_pending_updates_to_cycle_boundary.test_vegas_plugin_adapter,test_vegas_config,test_display_controller_plugin_toggle,test_display_controller_optimizations,test_plugin_system): 108passed, 1 pre-existing failure unrelated to this change
(
test_circuit_breaker, stale mock signature — same one alreadydocumented in prior PRs' test plans).
test_harness,test_visual_rendering,test_plugin_matrix): 52 passed, 2 pre-existing skips.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests