Skip to content

Fix Shift+wheel vertical scroll leak#207

Merged
benvinegar merged 1 commit intomainfrom
fix/shift-wheel-vertical-leak
Apr 18, 2026
Merged

Fix Shift+wheel vertical scroll leak#207
benvinegar merged 1 commit intomainfrom
fix/shift-wheel-vertical-leak

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

  • stop Shift+wheel horizontal scrolling from leaking a one-line vertical scroll
  • zero the intercepted wheel delta before OpenTUI's ScrollBox remaps it and restore the preserved viewport as a final guard
  • add regression coverage for native horizontal wheel events and document the fix in the changelog

Testing

  • bun run typecheck
  • bun test src/ui/AppHost.interactions.test.tsx --test-name-pattern 'shift plus'

This PR description was generated by Pi using GPT-5

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 18, 2026

Greptile Summary

This PR fixes a one-line vertical scroll leak that occurred when using Shift+wheel horizontal scrolling in some terminals. The fix zeros event.scroll.delta before OpenTUI's ScrollBox can remap it into vertical movement, then restores the preserved scroll position and resets acceleration state in a microtask as a final guard. A regression test and changelog entry accompany the change.

Confidence Score: 5/5

Safe to merge; the single finding is a minor style inconsistency that does not affect correctness under normal conditions.

All findings are P2. The scrollAcceleration.reset() call without optional chaining is inconsistent with the adjacent resetScrollAccumulators?.() pattern but is unlikely to throw in practice since scrollAcceleration is a typed, stable property. The fix is well-scoped, covered by a new regression test, and the changelog is correctly updated.

No files require special attention.

Important Files Changed

Filename Overview
src/ui/components/panes/DiffPane.tsx Zeros the wheel delta before OpenTUI's ScrollBox can remap it, then restores viewport position; scrollAcceleration.reset() lacks optional chaining unlike the adjacent resetScrollAccumulators?.() call.
src/ui/AppHost.interactions.test.tsx Adds a regression test for Shift+wheel-right not advancing vertical position; loop structure and assertions look correct.
CHANGELOG.md Changelog entry added under the correct version block; no issues.

Sequence Diagram

sequenceDiagram
    participant User
    participant DiffPane
    participant OpenTUI as OpenTUI ScrollBox

    User->>DiffPane: Shift+wheel (left/right/up+shift/down+shift)
    DiffPane->>DiffPane: Capture preservedScrollTop/Left
    DiffPane->>DiffPane: Call onScrollCodeHorizontally(±1)
    DiffPane->>DiffPane: Zero event.scroll.delta = 0
    DiffPane->>OpenTUI: (event propagates with delta=0)
    OpenTUI-->>OpenTUI: Remap delta → no vertical movement
    DiffPane->>DiffPane: queueMicrotask(restore viewport)
    Note over DiffPane: scrollTo({x, y}), reset acceleration
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/ui/components/panes/DiffPane.tsx
Line: 231-234

Comment:
**Inconsistent optional chaining on internal APIs**

`scrollAcceleration.reset()` is called without optional chaining, while `resetScrollAccumulators` is accessed via `as unknown` cast with `?.()` precisely because it may not exist. If `scrollAcceleration` is also absent on the current scroll box object (e.g., a different runtime version of OpenTUI, or a test mock), this will throw a `TypeError` inside the microtask — an uncatchable silent failure. For consistency with the pattern already used on the next line, consider guarding it the same way:

```suggestion
        currentScrollBox.scrollAcceleration?.reset();
        (
          currentScrollBox as unknown as { resetScrollAccumulators?: () => void }
        ).resetScrollAccumulators?.();
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(scroll): stop Shift+wheel vertical l..." | Re-trigger Greptile

Comment on lines +231 to +234
currentScrollBox.scrollAcceleration.reset();
(
currentScrollBox as unknown as { resetScrollAccumulators?: () => void }
).resetScrollAccumulators?.();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Inconsistent optional chaining on internal APIs

scrollAcceleration.reset() is called without optional chaining, while resetScrollAccumulators is accessed via as unknown cast with ?.() precisely because it may not exist. If scrollAcceleration is also absent on the current scroll box object (e.g., a different runtime version of OpenTUI, or a test mock), this will throw a TypeError inside the microtask — an uncatchable silent failure. For consistency with the pattern already used on the next line, consider guarding it the same way:

Suggested change
currentScrollBox.scrollAcceleration.reset();
(
currentScrollBox as unknown as { resetScrollAccumulators?: () => void }
).resetScrollAccumulators?.();
currentScrollBox.scrollAcceleration?.reset();
(
currentScrollBox as unknown as { resetScrollAccumulators?: () => void }
).resetScrollAccumulators?.();
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/components/panes/DiffPane.tsx
Line: 231-234

Comment:
**Inconsistent optional chaining on internal APIs**

`scrollAcceleration.reset()` is called without optional chaining, while `resetScrollAccumulators` is accessed via `as unknown` cast with `?.()` precisely because it may not exist. If `scrollAcceleration` is also absent on the current scroll box object (e.g., a different runtime version of OpenTUI, or a test mock), this will throw a `TypeError` inside the microtask — an uncatchable silent failure. For consistency with the pattern already used on the next line, consider guarding it the same way:

```suggestion
        currentScrollBox.scrollAcceleration?.reset();
        (
          currentScrollBox as unknown as { resetScrollAccumulators?: () => void }
        ).resetScrollAccumulators?.();
```

How can I resolve this? If you propose a fix, please make it concise.

@benvinegar benvinegar force-pushed the fix/shift-wheel-vertical-leak branch from ace5027 to cc7ca82 Compare April 18, 2026 15:54
@benvinegar benvinegar merged commit 3214793 into main Apr 18, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant