Fix inconsistent Vegas scroll transition gaps caused by plugin-baked padding#384
Conversation
Plugins that build their own ticker image via ScrollHelper.create_scrolling_image() (or that manually pad both ends for a clean standalone loop) carry a solid-black margin up to display_width wide on one or both edges. Vegas mode already adds its own configurable gap around every item, so leaving that margin in place stacked an extra, uncontrolled blank stretch on top of separator_width for whichever plugin took the ScrollHelper-capture path — producing inconsistent transition gaps between modules compared to plugins that provide content natively via get_vegas_content(). _get_scroll_helper_content() now detects and crops any such margin before handing the image to the Vegas render pipeline, so every plugin's gap is governed solely by vegas_scroll.separator_width regardless of which capture path produced its content.
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPluginAdapter now strips solid-black edge padding from scroll-helper cached images before resizing and RGB conversion. The new helper crops matching margins using ChangesScroll Padding Stripping
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 | 12 |
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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/vegas_mode/plugin_adapter.py (1)
322-377: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHeuristic can false-positive on legitimate all-black edge content.
is_solid_blackonly checks whether the exactpad_width-wide edge strip is uniformly(0,0,0). A plugin whose real content happens to render a solid-black region of that exact width at an edge (e.g. a dark-themed logo/background touching the boundary) would be silently cropped as if it were padding, since there's no way to distinguish real black pixels from baked-in padding via extrema alone.This is an inherent trade-off of the approach and may be acceptable given the PR's scope, but consider at least logging a distinguishable warning when both edges qualify simultaneously (a stronger signal of the fixed synthetic padding vs. coincidental content), so unexpected crops are easier to diagnose in the field.
Also, no unit tests were added for
_strip_scroll_padding's branch logic (invalid pad_width, no-match, all-black degenerate case, left-only/right-only crop). Given this directly affects rendered pixel output, some coverage would guard against regressions.🤖 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/vegas_mode/plugin_adapter.py` around lines 322 - 377, The _strip_scroll_padding heuristic can mistakenly crop legitimate solid-black edge content because is_solid_black only checks for an all-black pad_width strip, so add a warning in the branch where both left and right edges qualify to make unexpected crops easier to diagnose. Keep the current cropping behavior in _strip_scroll_padding, but improve the log path around the existing logger.info call so simultaneous edge matches are clearly distinguishable from ordinary one-sided padding. Also add unit tests covering _strip_scroll_padding for invalid pad_width, no-match, left-only, right-only, and the all-black degenerate case to lock down the branch logic.
🤖 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.
Nitpick comments:
In `@src/vegas_mode/plugin_adapter.py`:
- Around line 322-377: The _strip_scroll_padding heuristic can mistakenly crop
legitimate solid-black edge content because is_solid_black only checks for an
all-black pad_width strip, so add a warning in the branch where both left and
right edges qualify to make unexpected crops easier to diagnose. Keep the
current cropping behavior in _strip_scroll_padding, but improve the log path
around the existing logger.info call so simultaneous edge matches are clearly
distinguishable from ordinary one-sided padding. Also add unit tests covering
_strip_scroll_padding for invalid pad_width, no-match, left-only, right-only,
and the all-black degenerate case to lock down the branch logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a7bb235-b3f2-474a-a07a-beb2b46b063f
📒 Files selected for processing (1)
src/vegas_mode/plugin_adapter.py
…t tests Logging a double-edge match at warning level (vs. info for a single edge) makes it easy to spot an unexpected crop in the field, since two edges matching at once is a much stronger signal of genuine baked-in padding than one edge coinciding with real all-black content. Also adds test/test_vegas_plugin_adapter.py covering _strip_scroll_padding's branch logic: leading-only, trailing-only, both-edges, no-match, degenerate all-black, missing/undersized display_width, and the info-vs-warning log level.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test_vegas_plugin_adapter.py (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding type hints and a docstring to the
_solidhelper.Per coding guidelines, Python functions should use type hints and docstrings. The
_solidhelper is small but used across multiple tests.As per coding guidelines: "Use type hints for function parameters and return values" and "Use docstrings for classes and complex functions."
✨ Optional polish
-def _solid(width, height, color): - return Image.new('RGB', (width, height), color) +def _solid(width: int, height: int, color: tuple[int, int, int]) -> Image.Image: + """Create a solid-color RGB image of the given dimensions.""" + return Image.new('RGB', (width, height), color)🤖 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_plugin_adapter.py` around lines 34 - 35, The _solid helper is missing the required function documentation and type annotations. Update _solid to include type hints for width, height, and color, and add a brief docstring describing that it creates a solid RGB image for tests. Keep the change localized to the _solid function so it matches the coding guidelines used throughout the test module.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/test_vegas_plugin_adapter.py`:
- Around line 34-35: The _solid helper is missing the required function
documentation and type annotations. Update _solid to include type hints for
width, height, and color, and add a brief docstring describing that it creates a
solid RGB image for tests. Keep the change localized to the _solid function so
it matches the coding guidelines used throughout the test module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b03735ba-bf16-4da0-8843-cb5890a1a34e
📒 Files selected for processing (2)
src/vegas_mode/plugin_adapter.pytest/test_vegas_plugin_adapter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/vegas_mode/plugin_adapter.py
2 participants
Summary
Investigated a report that Vegas scroll mode's transitions between modules look inconsistent (e.g. one module needs to fully clear the screen before the next appears, while another overlaps heavily with the next module before it's even gone). Root cause: plugins that build their own ticker image via
ScrollHelper.create_scrolling_image()(or that manually pad both ends of their image for a clean standalone loop) carry a solid-black margin up todisplay_widthpixels wide on one or both edges. Vegas mode already adds its own configurable gap (vegas_scroll.separator_width) around every item, so that baked-in margin stacked an extra, uncontrolled blank stretch on top of it — but only for plugins captured via thescroll_helper.cached_imagepath, not for plugins that provide content natively viaget_vegas_content(). That created the asymmetric gaps.PluginAdapter._get_scroll_helper_content()now detects a solid-black margin up toscroll_helper.display_widthwide on each edge of the captured image and crops it off before handing the image to the Vegas render pipeline.set_scrolling_image()directly (no baked-in margin) are untouched, and a degenerate all-black image is left alone rather than cropped to nothing.Type of change
Related issues
N/A — reported directly in conversation, no tracked issue.
Test plan
EMULATOR=true python3 run.py)scripts/dev_server.py)pytest)Manually exercised
PluginAdapter._get_scroll_helper_content()/ the new_strip_scroll_padding()helper with syntheticScrollHelperimages covering: leading-pad-only (fromcreate_scrolling_image()), both leading+trailing pad, no pad (set_scrolling_image()with content starting at pixel 0), and a degenerate all-black image — all crop as expected. No repo-side plugin implementation was available to test end-to-end against real clock/stocks/weather plugins (those live in the separateledmatrix-pluginsmonorepo).Documentation
README.mdif user-facing behavior changeddocs/if developer behavior changedPlugin compatibility
The crop only fires when a plugin's own
ScrollHelper.display_width-wide edge is solid black — plugins with no such margin, or with real content touching the edge, are unaffected.Checklist
config_schema.json) — N/A, no new config key addedNotes for reviewer
The margin-detection heuristic assumes a plugin's baked-in padding is solid black (
(0, 0, 0)) and exactlyscroll_helper.display_widthwide, matchingScrollHelper.create_scrolling_image()'s behavior. If a real-world plugin pads with a non-black color or a different width, this crop won't fire for it — worth confirming against the actual stocks/weather plugin implementations inledmatrix-pluginsonce this lands.Generated by Claude Code
Summary by CodeRabbit