fix(vegas): stop the width cap emitting fragments and stale windows - #446
Open
ChuckBuilds wants to merge 1 commit into
Open
fix(vegas): stop the width cap emitting fragments and stale windows#446ChuckBuilds wants to merge 1 commit into
ChuckBuilds wants to merge 1 commit into
Conversation
Two defects in the rotation that narrows an oversized plugin to its width budget. Both were found while investigating "cut off early / starts in the middle" reports and are the reason the cap is no longer on by default; they still bite anyone who sets one. A rotation's last window was whatever happened to be left over. Windows are placed by walking forward from the previous one, with nothing looking at the remainder, so a 1,840px stocks ticker against a 1,536px budget split 1,492 + 348 -- every other appearance showed seven seconds and cut. Absorb a remainder below half a budget into the window before it. That overruns the budget by at most half, which is the better trade: the budget guards against one plugin holding the panel for minutes, not against a 20% overshoot. The floor is measured against the budget rather than the panel because snapping to item boundaries already lands an ordinary window short of it -- a 512px budget over 182px-pitch items yields 348px windows, so an absolute floor merges windows that were never fragments. The stored offset also outlived the content it was recorded against. It was a pixel column, reused verbatim after the plugin re-rendered, so once anything ahead of it changed width the window pointed at unrelated items -- observed as news refreshing 9,793px -> 9,505px mid-rotation. Track the rotation as an index into the strip's item boundaries instead, since the Nth boundary survives a digit appearing in a price, and record alongside it what the offset indexes into: a row list, a boundary list, or a column in a gapless image. A mismatch restarts the rotation rather than reinterpreting the number, which also closes the case where one plugin's row index was read back as a pixel column after its content changed from several rows to one wide strip. Replaying the four plugins that actually hit the cap on a live 512px panel: no window is now a fragment, none exceeds 1.5 budgets, and every rotation still covers the whole strip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
📝 WalkthroughWalkthroughPluginAdapter now tracks rotation offsets with content-shape metadata. It validates and resets stale state, merges undersized trailing windows, and uses boundary indices for discrete single-image crops. Tests cover width changes, content changes, stale offsets, and row-to-pixel transitions. ChangesRotation state and cropping
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 | 8 |
| 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.
Owner
Author
|
@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.
Actionable comments posted: 1
🤖 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/vegas_mode/plugin_adapter.py`:
- Around line 538-548: Update the multi-row window construction in the rotation
path around _resume_offset so a trailing suffix shorter than budget // 2 is
merged into the preceding window, accounting for _row_gap() when calculating the
combined height. Preserve the permitted maximum of 1.5 * budget, and add a
regression test covering the 450px, 450px, and 100px rows with zero gaps.
🪄 Autofix
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 Plus
Run ID: cfcb4273-7331-4e60-856a-b9efc5ae1270
📒 Files selected for processing (2)
src/vegas_mode/plugin_adapter.pytest/test_vegas_density.py
Comment on lines
538
to
+548
| if len(images) == 1: | ||
| return [self._crop_to_budget(images[0], budget, plugin_id, mode)] | ||
|
|
||
| shape = ('rows', len(images)) | ||
| if mode == 'truncate': | ||
| # Ordered content: always show from the top. Deliberately does not | ||
| # advance the offset, so the same opening items appear every time | ||
| # rather than the viewer being shown the middle of a ranked list. | ||
| start = 0 | ||
| else: | ||
| start = self._item_offsets.get(plugin_id, 0) % len(images) | ||
| start = self._resume_offset(plugin_id, shape) % len(images) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Merge a short final multi-row window.
_merge_trailing_runt() only applies to _crop_to_budget(). A multi-row rotation can still return a final window smaller than budget // 2.
For example, with zero row gaps, rows of 450px, 450px, and 100px against a 512px budget produce a 100px final pass. Include the short suffix with the preceding row window when it is below the runt floor. Account for _row_gap() and keep the permitted 1.5-budget maximum. Add a multi-row regression test.
🤖 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 538 - 548, Update the
multi-row window construction in the rotation path around _resume_offset so a
trailing suffix shorter than budget // 2 is merged into the preceding window,
accounting for _row_gap() when calculating the combined height. Preserve the
permitted maximum of 1.5 * budget, and add a regression test covering the 450px,
450px, and 100px rows with zero gaps.
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
1 participant
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.
Follow-up to #445, which called these out but left them. Both are in the rotation that narrows an oversized plugin to its width budget. They no longer fire at the default settings once #445 lands, but they still bite anyone who sets a cap.
1. The last window was whatever happened to be left over
Windows are placed by walking forward from the previous one, with nothing looking at the remainder. On a live 512px panel that split an 1,840px stocks ticker into 1,492 + 348 against a 1,536px budget — every other appearance showed seven seconds and cut, which reads as the display failing rather than as a rotation.
A remainder below half a budget is now absorbed into the window before it. That overruns the budget by at most half, which is the better trade: the budget guards against one plugin holding the panel for minutes, not against a 20% overshoot.
The floor is measured against the budget rather than the panel, and that distinction turned out to matter — snapping to item boundaries already lands an ordinary window short of the budget (a 512px budget over 182px-pitch items yields 348px windows), so an absolute floor merges windows that were never fragments. My first attempt used one screenful and the test caught it.
2. The offset outlived the content it was recorded against
The stored offset was a pixel column, reused verbatim after the plugin re-rendered. Once anything ahead of it changed width — a digit in a price, a shorter headline — the window pointed at unrelated items. Observed on the same panel as news refreshing 9,793px → 9,505px mid-rotation while its column kept advancing, which quietly breaks the "everything is seen eventually" promise for any ticker that refreshes.
Rotation is now an index into the strip's item boundaries, since the Nth boundary survives items changing width. Recorded alongside it is what the offset indexes into:
A mismatch restarts the rotation rather than reinterpreting the number. That also closes the latent unit collision from #445's description — one plugin's row index being read back as a pixel column after its content changed from several rows to one wide strip.
Verification
Replaying the four plugins that actually hit the cap on the live panel, at their logged widths:
No window is a fragment, none exceeds 1.5 budgets, and every rotation still covers the whole strip.
10 regression tests added across
TestTrailingRuntWindowandTestOffsetOutlivesItsContent, including the reported stocks case by its real numbers. 227 tests pass across the vegas and display-controller suites; the fulltest/run is 2372 passed with one pre-existing unrelated failure (test_install_lowmem.py::TestDiskBackedTmpdir, which fails identically on a clean tree whereverTMPDIRis set).Note this branch is cut from
main, not from #445 — they touch different files and can land in either order.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
Bug Fixes
Tests