fix: honor space row gaps across machines - #3740
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe expanded sidebar calculates row gaps between consecutive workspaces within the same endpoint. Scroll metrics and row rendering apply these gaps to endpoint and workspace placement. A test verifies spacing for local and remote machines with multiple workspace and worktree configurations. Suggested reviewers: Merge Risk: ⚪ Minimal · up to The expanded sidebar now applies configured gaps between Spaces on the same machine while retaining packed machine boundaries and worktree children. Current coverage validates the intended layout behavior, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
Greptile SummaryThis PR propagates configured Space row gaps into the expanded multi-machine sidebar while preserving packed machine and worktree-group boundaries.
Confidence Score: 4/5The implementation appears safe to merge, with a non-blocking opportunity to cover nonzero-gap scrolling in the regression test. The row grouping and gap application match existing sidebar behavior, and scroll metrics consume the same gaps as rendering; the only accepted concern is missing regression coverage for the overflowing, scrolled case. Files Needing Attention: src/client/shell/tests/endpoints.rs
|
| Filename | Overview |
|---|---|
| src/client/shell/endpoint_sidebar.rs | Computes machine-aware Space gaps and consistently applies them to scrolling and vertical rendering. |
| src/client/shell/tests/endpoints.rs | Covers static gap placement and packed boundaries, but not the changed nonzero-gap scrolling behavior. |
Reviews (1): Last reviewed commit: "fix: honor space row gaps across machine..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| remote.workspaces.push(third); | ||
| state.set_endpoint_snapshot(&remote_id, Box::new(remote)); | ||
|
|
||
| state.compose(100, 40).expect("combined endpoint frame"); |
There was a problem hiding this comment.
This regression test uses a 100×40 frame that fits all five workspace rows, so it only verifies unscrolled placement even though the change also affects gap-aware scroll limits and rendering after scrolling. Add a constrained-viewport case with a nonzero row gap that scrolls the expanded machine list and verifies the final scroll position and visible row coordinates; otherwise those calculations could diverge without detection.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Issue
A user sets a Space row gap while viewing Local and SSH machines. Spaces inside each expanded machine stay packed together.
Problem
The combined machine sidebar assigned zero gap to every row. The configured Space gap never reached scrolling or rendered positions.
How did we fix it?
Herdr now applies the configured gap between Spaces in the same machine. Machine boundaries and indented worktree children remain packed. Scrolling and rendering use the same calculated gaps.
Verification
The compose-path test failed before with the second Space at row 5 instead of row 6, then passed after the change. It also covers bottom scrolling and gap hitboxes. All 28 endpoint tests, local Windows lint, docs checks, maintenance tests, and the render-scale benchmark passed. PR checks passed on Ubuntu, macOS, and ConPTY; Windows hit an unrelated metadata timing-test failure.
refs #3738