Fix System.OverflowException in CustomScrollViewerEx during Mouse Wheel scroll#4497
Open
4yinn wants to merge 2 commits into
Open
Fix System.OverflowException in CustomScrollViewerEx during Mouse Wheel scroll#44974yinn wants to merge 2 commits into
4yinn wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds early-return guards in CustomScrollViewerEx.OnMouseWheel to avoid division-by-zero when layout sizes are non-positive, and adds a test (MouseWheelTest) plus a test-project ProjectReference to invoke the handler via reflection and assert it does not throw. ChangesMouseWheel Handler Robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs (1)
91-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset animation state on guard-path returns.
SetIsAnimating(this, true)is set before guard checks, but both guard returns exit without resetting it. That can leave animation state stuck astrueand suppress offset state updates inOnScrollChanged.Suggested fix
- ScrollViewerBehavior.SetIsAnimating(this, true); - if (Direction == Orientation.Vertical) { + if (ActualHeight <= 0) + return; + + ScrollViewerBehavior.SetIsAnimating(this, true); + if (ScrollableHeight > 0) { e.Handled = true; } - - if (ActualHeight <= 0) - return;else { + if (ActualWidth <= 0) + return; + + ScrollViewerBehavior.SetIsAnimating(this, true); + if (ScrollableWidth > 0) { e.Handled = true; } - - if (ActualHeight <= 0) - return;Also applies to: 133-134
🤖 Prompt for 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. In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs` around lines 91 - 101, The handler sets ScrollViewerBehavior.SetIsAnimating(this, true) before guard checks but returns on two guard paths without resetting it; update the method in CustomScrollViewerEx (the scroll event handler that calls SetIsAnimating) so that you either move the SetIsAnimating(this, true) call to after the guard checks or ensure you call ScrollViewerBehavior.SetIsAnimating(this, false) before each early return (including the guard near the Vertical Orientation block and the other guard around lines ~133-134) so the animation flag is never left true when exiting early.
🤖 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 `@Flow.Launcher.Test/MouseWheelTest.cs`:
- Around line 26-34: The reflection lookup for CustomScrollViewerEx's
"OnMouseWheel" is not asserted before Invoke, which causes a confusing
null-reference if the method isn't found; add an explicit null assertion (e.g.
Assert.IsNotNull or Assert.NotNull) for onMouseWheelMethod after the GetMethod
call with a clear message like "OnMouseWheel method not found on
CustomScrollViewerEx" so failures are diagnostic, then proceed to call
onMouseWheelMethod.Invoke(scrollView, new object[] { e }) inside the existing
Assert.DoesNotThrow block.
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 133-136: In CustomScrollViewerEx (the mouse wheel/scroll handling
code that computes WheelChange using e.Delta * (ViewportWidth / 1.5) /
ActualWidth), change the early-return guard to validate ActualWidth instead of
ActualHeight (i.e., if (ActualWidth <= 0) return;) so you never divide by zero
when computing WheelChange; update the check in the same method where
WheelChange is declared to prevent the infinite offset on horizontal scrolling.
---
Outside diff comments:
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 91-101: The handler sets ScrollViewerBehavior.SetIsAnimating(this,
true) before guard checks but returns on two guard paths without resetting it;
update the method in CustomScrollViewerEx (the scroll event handler that calls
SetIsAnimating) so that you either move the SetIsAnimating(this, true) call to
after the guard checks or ensure you call
ScrollViewerBehavior.SetIsAnimating(this, false) before each early return
(including the guard near the Vertical Orientation block and the other guard
around lines ~133-134) so the animation flag is never left true when exiting
early.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e62e2923-df1f-435a-97a3-52643a176be2
📒 Files selected for processing (3)
Flow.Launcher.Test/Flow.Launcher.Test.csprojFlow.Launcher.Test/MouseWheelTest.csFlow.Launcher/Resources/Controls/CustomScrollViewerEx.cs
Comment on lines
+26
to
+34
| var onMouseWheelMethod = typeof(CustomScrollViewerEx).GetMethod( | ||
| "OnMouseWheel", | ||
| BindingFlags.NonPublic | BindingFlags.Instance | ||
| ); | ||
|
|
||
| Assert.DoesNotThrow(() => | ||
| { | ||
| onMouseWheelMethod.Invoke(scrollView, new object[] { e }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert reflected method lookup before invocation.
Add an explicit null assertion for onMouseWheelMethod so failures are diagnostic (method not found) instead of a generic null-reference during invoke.
Suggested fix
var onMouseWheelMethod = typeof(CustomScrollViewerEx).GetMethod(
"OnMouseWheel",
BindingFlags.NonPublic | BindingFlags.Instance
);
+ Assert.That(onMouseWheelMethod, Is.Not.Null, "OnMouseWheel method was not found via reflection.");
Assert.DoesNotThrow(() =>
{
onMouseWheelMethod.Invoke(scrollView, new object[] { e });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var onMouseWheelMethod = typeof(CustomScrollViewerEx).GetMethod( | |
| "OnMouseWheel", | |
| BindingFlags.NonPublic | BindingFlags.Instance | |
| ); | |
| Assert.DoesNotThrow(() => | |
| { | |
| onMouseWheelMethod.Invoke(scrollView, new object[] { e }); | |
| }); | |
| var onMouseWheelMethod = typeof(CustomScrollViewerEx).GetMethod( | |
| "OnMouseWheel", | |
| BindingFlags.NonPublic | BindingFlags.Instance | |
| ); | |
| Assert.That(onMouseWheelMethod, Is.Not.Null, "OnMouseWheel method was not found via reflection."); | |
| Assert.DoesNotThrow(() => | |
| { | |
| onMouseWheelMethod.Invoke(scrollView, new object[] { e }); | |
| }); |
🤖 Prompt for 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.
In `@Flow.Launcher.Test/MouseWheelTest.cs` around lines 26 - 34, The reflection
lookup for CustomScrollViewerEx's "OnMouseWheel" is not asserted before Invoke,
which causes a confusing null-reference if the method isn't found; add an
explicit null assertion (e.g. Assert.IsNotNull or Assert.NotNull) for
onMouseWheelMethod after the GetMethod call with a clear message like
"OnMouseWheel method not found on CustomScrollViewerEx" so failures are
diagnostic, then proceed to call onMouseWheelMethod.Invoke(scrollView, new
object[] { e }) inside the existing Assert.DoesNotThrow block.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
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 `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 133-134: In OnMouseWheel of CustomScrollViewerEx the method sets
the animating flag (IsAnimating) earlier but returns early when ActualWidth <= 0
without clearing it, which can leave IsAnimating stuck; modify OnMouseWheel to
reset IsAnimating (or the animating field) to false before the early return so
the animation state is cleared and OnScrollChanged can continue syncing offsets.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea371989-0f85-4582-a0d5-ae6ec2cb5aef
📒 Files selected for processing (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs
Comment on lines
+133
to
+134
| if (ActualWidth <= 0) | ||
| return; |
Contributor
There was a problem hiding this comment.
Reset animation state before guard returns.
OnMouseWheel sets animating true at Line 91, but this new early return exits without resetting it. That can leave IsAnimating stuck and block OnScrollChanged from syncing offsets.
Proposed minimal fix
protected override void OnMouseWheel(MouseWheelEventArgs e)
{
var Direction = GetDirection();
ScrollViewerBehavior.SetIsAnimating(this, true);
if (Direction == Orientation.Vertical)
{
if (ScrollableHeight > 0)
{
e.Handled = true;
}
- if (ActualHeight <= 0)
- return;
+ if (ActualHeight <= 0)
+ {
+ ScrollViewerBehavior.SetIsAnimating(this, false);
+ return;
+ }
@@
else
{
if (ScrollableWidth > 0)
{
e.Handled = true;
}
- if (ActualWidth <= 0)
- return;
+ if (ActualWidth <= 0)
+ {
+ ScrollViewerBehavior.SetIsAnimating(this, false);
+ return;
+ }🤖 Prompt for 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.
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs` around lines 133 -
134, In OnMouseWheel of CustomScrollViewerEx the method sets the animating flag
(IsAnimating) earlier but returns early when ActualWidth <= 0 without clearing
it, which can leave IsAnimating stuck; modify OnMouseWheel to reset IsAnimating
(or the animating field) to false before the early return so the animation state
is cleared and OnScrollChanged can continue syncing offsets.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs">
<violation number="1" location="Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs:133">
P1: Animation state `IsAnimating` is set to `true` before dimension guard-clauses, so an early return when `ActualWidth <= 0` or `ActualHeight <= 0` leaves the flag permanently stuck. `ScrollToValue` is the only place in the file that resets it to `false`, and it is never reached on these early-exit paths. When stuck true, `OnScrollChanged` stops updating `LastVerticalLocation`/`LastHorizontalLocation`, leading to stale offsets and incorrect subsequent wheel behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+133
to
+134
| if (ActualWidth <= 0) | ||
| return; |
Contributor
There was a problem hiding this comment.
P1: Animation state IsAnimating is set to true before dimension guard-clauses, so an early return when ActualWidth <= 0 or ActualHeight <= 0 leaves the flag permanently stuck. ScrollToValue is the only place in the file that resets it to false, and it is never reached on these early-exit paths. When stuck true, OnScrollChanged stops updating LastVerticalLocation/LastHorizontalLocation, leading to stale offsets and incorrect subsequent wheel behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs, line 133:
<comment>Animation state `IsAnimating` is set to `true` before dimension guard-clauses, so an early return when `ActualWidth <= 0` or `ActualHeight <= 0` leaves the flag permanently stuck. `ScrollToValue` is the only place in the file that resets it to `false`, and it is never reached on these early-exit paths. When stuck true, `OnScrollChanged` stops updating `LastVerticalLocation`/`LastHorizontalLocation`, leading to stale offsets and incorrect subsequent wheel behavior.</comment>
<file context>
@@ -130,7 +130,7 @@ protected override void OnMouseWheel(MouseWheelEventArgs e)
}
- if (ActualHeight <= 0)
+ if (ActualWidth <= 0)
return;
</file context>
Suggested change
| if (ActualWidth <= 0) | |
| return; | |
| if (ActualWidth <= 0) | |
| { | |
| ScrollViewerBehavior.SetIsAnimating(this, false); | |
| return; | |
| } |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the OverflowException reported in #4481, which occurs when the user scrolls on a ScrollViewer that has not been fully rendered yet or whose ActualHeight is 0.
Root Cause
The issue originates from the following calculation inside OnMouseWheel:
When ActualHeight is 0, the result becomes double.Infinity.
This value is later passed into the animation logic, which eventually converts it into a TimeSpan using TimeSpan.FromTicks. Since Infinity cannot be represented as a valid duration, a System.OverflowException is thrown.
Changes
Added a guard clause in CustomScrollViewerEx.cs to return early when:
Prevented invalid Infinity/NaN scroll offsets from reaching the animation pipeline.
Ensured the animation logic only receives finite values.
Testing
Added MouseWheelTest.cs to simulate a MouseWheelEvent on a CustomScrollViewerEx with zero dimensions.
Verified that:
without the fix, the scroll logic attempts to process an infinite offset;
with the fix, the method exits safely without triggering an exception.
I’m also open to feedback if you think there’s a cleaner or more robust approach for handling this scenario.
Summary by cubic
Fixes an OverflowException in
CustomScrollViewerExwhen scrolling before layout completes by exiting early on zero width/height and preventing infinite deltas from reaching animation. Adds an STA unit test to cover zero-size scroll scenarios.Summary of changes
MouseWheelTestto simulate wheel scroll on a zero-size control; test project now referencesFlow.Launcher.MouseWheelTestensures no exception is thrown on zero-size control.Release Note
Fixes a crash when using the mouse wheel to scroll before the window finishes rendering.
Written for commit 4fa1f3d. Summary will update on new commits.
Review in cubic