fix(vegas): stop the width cap emitting fragments and stale windows - #446
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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: 4 (Complex) | ~40 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
|
@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
The floor only guarded the single-image path. I had reasoned the row
path could not produce a runt because it wraps, which is wrong: wrapping
only helps when the row wrapped to actually fits. Rows of 450, 450 and
100 against a 512px budget give the 100 a pass of its own -- two seconds
against nine, which is the symptom this branch exists to remove.
Reproduced before changing anything:
pass 1: 450px pass 2: 450px pass 3: 100px
A window may now overrun the budget while it is still shorter than the
floor, bounded at the same 1.5 budgets the single-image path allows, so
the short row is carried with its neighbour instead of standing alone.
pass 1: 450px pass 2: 450px pass 3: 550px
A next row too wide to absorb within that cap still leaves a short
window standing -- rows of 900 and 100 keep alternating. Merging them
would mean a window of nearly two budgets, and the rule that always
shows an oversized first row already makes the same trade.
Three regression tests: the reported shape, that the overrun stays
bounded when a row cannot be absorbed, and that absorbing never drops a
row from the rotation. The single-image path is untouched -- the four
plugins that actually hit the cap on a live panel replay identically.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
Valid, and it caught a wrong assumption of mine rather than just a missing call. I had reasoned the multi-row path couldn't produce a runt because it wraps ( Two seconds against nine — the symptom this branch exists to remove. Fix. A window may now overrun the budget while it is still shorter than the floor, bounded at the same 1.5 budgets the single-image path allows, so the short row is carried with its neighbour rather than standing alone: I went with a floor on the window rather than a trailing-suffix merge, because with a wrapping rotation there is no "final" window to merge into — the sequence is endless, and the short pass isn't necessarily at the end. Enforcing "no window below the floor while it can be fixed within the cap" covers the reported shape and the general one. One case it deliberately doesn't fix. When the next row is too wide to absorb within the cap, a short window still stands — rows of 900 and 100 keep alternating. Merging them would mean a window of nearly two budgets, and the rule that always shows an oversized first row already makes the same trade. Pinned in a test so it stays a decision rather than drift. Verification
|
1 participant
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