Conversation
Greptile SummaryThis PR fixes a one-line vertical scroll leak that occurred when using Shift+wheel horizontal scrolling in some terminals. The fix zeros Confidence Score: 5/5Safe to merge; the single finding is a minor style inconsistency that does not affect correctness under normal conditions. All findings are P2. The No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
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
Prompt To Fix All With AIThis 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 |
| currentScrollBox.scrollAcceleration.reset(); | ||
| ( | ||
| currentScrollBox as unknown as { resetScrollAccumulators?: () => void } | ||
| ).resetScrollAccumulators?.(); |
There was a problem hiding this 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:
| 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.ace5027 to
cc7ca82
Compare
Summary
Testing
This PR description was generated by Pi using GPT-5