Skip to content

feat(ui): route mouse-wheel to preview in stacked (below) layout (#1626 follow-up) - #1782

Open
jdidion wants to merge 3 commits into
asheshgoplani:mainfrom
jdidion:feat/stacked-mouse-routing
Open

feat(ui): route mouse-wheel to preview in stacked (below) layout (#1626 follow-up)#1782
jdidion wants to merge 3 commits into
asheshgoplani:mainfrom
jdidion:feat/stacked-mouse-routing

Conversation

@jdidion

@jdidion jdidion commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Note: This PR was largely generated by Claude and has not been completely reviewed by me (the human). Feel free to defer review until this warning is removed.

Follow-up to the merged #1626 (preview-below orientation), addressing a CodeRabbit review finding on that PR.

What

In the below/stacked preview layout, the mouse wheel always moved the session-list cursor — it never scrolled the preview. Wheel-to-preview routing was dual-layout only (X-based: "is the wheel over the right column?"). Stacked mode has no right column — the preview is the lower region — so scroll input never reached it.

This adds Y-based routing for stacked mode: a wheel event at or below the horizontal divider scrolls the preview (previewScrollOffset), matching the dual layout's X-based behaviour. Single layout is unchanged (no preview target).

How

  • stackedPreviewTopY() computes the divider's screen-Y by mirroring renderStackedLayout's height split — it reuses the shared stackedListHeight helper plus a new contentChromeTop() that both this and the render path use, so the routing Y can't drift from what's actually drawn.
  • The wheel handler switches on layout mode: dual routes by X (unchanged), stacked routes by Y (new), single falls through to list-scroll.

Scope

This is the wheel-scroll half of CodeRabbit's finding. Divider resize in stacked mode stays on the < / > keys (added in #1626), not a mouse drag: the existing divider-drag path (isOnDivider / setPreviewPctFromMouseX) is X-axis only, and a parallel Y-drag would duplicate fragile layout math for redundant polish (the keyboard path already covers stacked resize). Flagged in the commit as a possible follow-up if you want mouse-drag parity.

Verification

go build ./..., go vet, gofmt clean; internal/ui layout/preview/mouse tests pass.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved mouse-wheel behavior when viewing stacked layouts so wheel input scrolls the PREVIEW region rather than moving the session list cursor.
    • PREVIEW scrolling now responds to cursor position within the preview pane (using the pane’s true on-screen location).
    • Preview area detection now correctly accounts for the title/help/header rows and optional update/maintenance/debug banners.

CodeRabbit review on asheshgoplani#1626 (merged): in the below/stacked preview layout, the
mouse wheel always moved the session-list cursor because wheel-to-preview
routing was dual-layout-only (X-based: "is the wheel over the right column").
Stacked mode has no right column — the preview is the LOWER region — so scroll
input never reached it.

Add Y-based routing for stacked mode: a wheel event at or below the horizontal
divider scrolls the preview (previewScrollOffset), matching the dual layout's
X-based behaviour. New stackedPreviewTopY() computes the divider Y by mirroring
renderStackedLayout's height split (via the shared stackedListHeight helper +
a new contentChromeTop() that both this and the render path use, so the Y can't
drift from what's drawn). Single layout unchanged (no preview target).

Scope: this addresses the wheel-scroll half of the review. Divider RESIZE in
stacked mode stays on the `<`/`>` keys (added in asheshgoplani#1626) rather than a mouse
drag — the X-axis divider-drag path (isOnDivider/setPreviewPctFromMouseX) is
X-only, and a parallel Y-drag would duplicate fragile layout math for redundant
polish. Noted for a possible follow-up.

Build + go vet + gofmt clean; UI layout/preview tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 577d0059-0fa5-430e-ab16-d74aad93bfd5

📥 Commits

Reviewing files that changed from the base of the PR and between 7debcdf and 7acaa3d.

📒 Files selected for processing (1)
  • internal/ui/home.go
💤 Files with no reviewable changes (1)
  • internal/ui/home.go

📝 Walkthrough

Walkthrough

Adds layout-aware helpers for locating the stacked PREVIEW pane and routes mouse-wheel events there in stacked layouts, while preserving existing dual and single layout behavior.

Changes

Stacked PREVIEW wheel routing

Layer / File(s) Summary
Preview geometry helpers
internal/ui/home.go
Computes the main content origin and stacked PREVIEW pane position using update, maintenance, and debug layout flags.
Preview wheel routing
internal/ui/home.go
Routes stacked-layout wheel events within the PREVIEW region to previewScrollOffset; dual-layout X-based routing and single-layout list scrolling remain unchanged.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: asheshgoplani

🚥 Pre-merge checks | ✅ 5 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Remote_parity ⚠️ Warning preview_scroll_test only covers local sessions; renderRemotePreview ignores previewScrollOffset, and no t.Skip documents RemoteSession N/A. Add a RemoteSession wheel/preview-scroll test (or a documented t.Skip if intentionally N/A) and, if needed, apply previewScrollOffset in remote preview rendering.
Test_coverage_per_surface ⚠️ Warning TUI feature added stacked-layout wheel routing, but tests only cover dual and single layouts; no test exercises stacked wheel behavior or the new Y helper. Add internal/ui tests for stacked/below wheel events over preview vs list and the divider Y helper/offset reset path; no web/CLI/remote surfaces are applicable.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and accurately describes the main change to mouse-wheel routing in stacked layout.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jdidion
jdidion marked this pull request as ready for review July 27, 2026 18:18
@jdidion
jdidion requested a review from asheshgoplani as a code owner July 27, 2026 18:18
@github-actions

Copy link
Copy Markdown

👋 Thanks for opening this — a couple of intake fields would help the maintainer's validation pipeline (and any reviewer) understand it faster. Nothing here blocks you and nothing is closing: this is a heads-up, and editing the description re-runs this check automatically.

  • Missing required sections: ## What problem does this solve?, ## Why this change, ## User impact, ## AI disclosure, ## What actually bothered you, ## Checklist.
  • No AI-disclosure box is checked — pick exactly one (Human-written / AI-assisted / AI-authored). AI is welcome here with equal standing; this is a signal, not a filter.
  • ## What actually bothered you is empty — one real sentence of human intent is the one field intake cannot accept blank. If an agent opened this PR, quote what the human asked for.

The full field-level contract is in .github/INTAKE.md. AI-authored PRs are welcome here with equal standing.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@internal/ui/home.go`:
- Line 1102: Remove the unused panelTitleLines constant declaration from the
surrounding function in internal/ui/home.go, leaving the remaining panel layout
logic unchanged.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5460e40b-60a6-49cc-bd3a-98db6b289c82

📥 Commits

Reviewing files that changed from the base of the PR and between 580e772 and 7debcdf.

📒 Files selected for processing (1)
  • internal/ui/home.go

Comment thread internal/ui/home.go Outdated
jdidion and others added 2 commits July 27, 2026 11:22
…olangci)

The golangci `unused` linter flagged home.go:1102 — panelTitleLines was declared
in stackedPreviewTopY but never used (stackedListHeight already accounts for the
panel title internally). Dead code; removed. Verified with golangci-lint run
locally (0 issues).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
No code change. The prior push (golangci green) hit a flaky internal/session
-race failure (TestIssue1421 / -race timing) unrelated to this PR's one-line
home.go change, which passed the Full test suite on the push before it. Empty
commit to re-run the gate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jdidion

jdidion commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

@asheshgoplani heads-up on CI, and a request to re-run when you get a chance.

The code side is done and golangci is green. But the Full test suite (PR gate) (-race) has gone red on three runs of this branch, and I'm fairly confident it's pre-existing -race flakiness, not this PR:

  • This PR is a one-line change on top of the merged feat(ui): add preview-below orientation toggle for portrait terminals #1626 (removes an unused const in home.go; the substantive change is the ~60-line stacked-mouse-wheel routing, which touches only internal/ui).
  • The gate passed the Full suite on an earlier push of this same branch, then failed on a subsequent push, and failed again on an empty re-trigger commit (no code change at all).
  • The failing packages change every runinternal/tmux (softkill), then internal/session, then internal/statedb + internal/session — which is the signature of resource-contention flakiness under -race, not a deterministic failure in one test. None of these packages are touched by this PR.
  • main's own go-test runs show the same: mostly green, with intermittent failures (e.g. a failure on 2026-07-26). So the gate flakes on main too, independent of this branch.
  • I couldn't reproduce a matching failure locally (my box hits an unrelated env-specific bind: invalid argument in TestIssue1421_CleanStaleSSHSockets, which wouldn't fail on your Linux runners).

Could you re-run the gate (you have admin; I can't re-run from the fork)? If it's a known-flaky suite you already manage, happy to defer to whatever your standard is. And if you'd rather I split anything out or adjust scope, just say.

Same disclosure as before: largely Claude-generated, diff reviewed by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant