Conversation
📝 WalkthroughWalkthroughThe PR introduces a comprehensive test suite for the Maui.FreakyEffects library by adding a new net10.0 test project with unit tests covering animations, event arguments, gesture recognition, and utility classes. Additionally, animation classes are refactored to use asynchronous method variants, and a parameter ordering bug in TouchTrackingRect is corrected. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Maui.FreakyEffects/Maui.FreakyEffects/Shared/Skeleton/BeatAnimation.cs (1)
19-23:StopAnimationuses an unsafe direct cast inconsistent with other animation classes.
HorizontalShakeAnimationandVerticalShakeAnimationboth guard withif (bindable is View self)inStopAnimation, butBeatAnimation(andFadeAnimation) perform a direct(View)bindablecast without a type guard, which would throw anInvalidCastExceptionif ever called with a non-Viewbindable. Consider aligning with the safer pattern used elsewhere.♻️ Suggested fix
protected override async Task StopAnimation(BindableObject bindable) { - var self = (View)bindable; - await self.ScaleToAsync(1, 100); + if (bindable is View self) + await self.ScaleToAsync(1, 100); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maui.FreakyEffects/Maui.FreakyEffects/Shared/Skeleton/BeatAnimation.cs` around lines 19 - 23, BeatAnimation.StopAnimation currently does a direct cast (var self = (View)bindable) which can throw; change it to match the safe pattern used in HorizontalShakeAnimation/VerticalShakeAnimation by using an "if (bindable is View self)" guard and only call self.ScaleToAsync(1, 100) inside that block (no cast or exception). Also mirror the same guard pattern in FadeAnimation.StopAnimation if present to keep consistency across animation classes.Maui.FreakyEffects/Maui.FreakyEffects.Tests/Skeleton/AnimationConstructorTests.cs (1)
74-80:AnimationTypes_HasFiveMemberswill break on any future enum addition.If this is intentionally guarding the public API surface, add a comment explaining that. Otherwise, consider removing it since
AnimationTypes_HasExpectedValues(lines 65-72) already validates all five named members.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maui.FreakyEffects/Maui.FreakyEffects.Tests/Skeleton/AnimationConstructorTests.cs` around lines 74 - 80, The test AnimationTypes_HasFiveMembers is brittle because it asserts a hardcoded enum count; either remove this test or convert it to an explicit API-guard with a clear comment. Update the test file by either deleting the AnimationTypes_HasFiveMembers method (since AnimationTypes_HasExpectedValues already checks the named members) or keep it but add a comment above the AnimationTypes_HasFiveMembers test explaining it intentionally guards the public API surface and must be updated when enum members are added; ensure references to the test name AnimationTypes_HasFiveMembers and the complementary test AnimationTypes_HasExpectedValues are preserved so reviewers can see which approach you chose.Maui.FreakyEffects/Maui.FreakyEffects.Tests/TouchTracking/TouchTrackingRectTests.cs (1)
8-38: Consider consolidating the four constructor-property tests.All four tests construct the same
TouchTrackingRect(10f, 20f, 30f, 40f)and each assert a single property. A single test with four assertions (or a[Theory]with[MemberData]) avoids the repeated construction and keeps the constructor contract in one place.♻️ Proposed consolidation
- [Fact] - public void Constructor_SetsLeft() - { - var rect = new TouchTrackingRect(10f, 20f, 30f, 40f); - Assert.Equal(10f, rect.Left); - } - - [Fact] - public void Constructor_SetsTop() - { - var rect = new TouchTrackingRect(10f, 20f, 30f, 40f); - Assert.Equal(20f, rect.Top); - } - - [Fact] - public void Constructor_SetsRight() - { - var rect = new TouchTrackingRect(10f, 20f, 30f, 40f); - Assert.Equal(30f, rect.Right); - } - - [Fact] - public void Constructor_SetsBottom() - { - var rect = new TouchTrackingRect(10f, 20f, 30f, 40f); - Assert.Equal(40f, rect.Bottom); - } + [Fact] + public void Constructor_SetsAllProperties() + { + var rect = new TouchTrackingRect(10f, 20f, 30f, 40f); + + Assert.Equal(10f, rect.Left); + Assert.Equal(20f, rect.Top); + Assert.Equal(30f, rect.Right); + Assert.Equal(40f, rect.Bottom); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maui.FreakyEffects/Maui.FreakyEffects.Tests/TouchTracking/TouchTrackingRectTests.cs` around lines 8 - 38, Replace the four separate Fact tests (Constructor_SetsLeft, Constructor_SetsTop, Constructor_SetsRight, Constructor_SetsBottom) with a single consolidated test that constructs TouchTrackingRect(10f, 20f, 30f, 40f) once and asserts all four properties (Left, Top, Right, Bottom) in that test (e.g., a new Fact named Constructor_InitializesAllProperties or use a Theory+MemberData if preferred); update or remove the four original test methods so there’s one test validating the constructor initializes all properties.Maui.FreakyEffects/Maui.FreakyEffects.sln (1)
10-11: LGTM — test project correctly wired into the solution.The project entry and all four configuration mappings (Debug/Release × ActiveCfg/Build.0) are in order.
Nit: the project GUID
{A1B2C3D4-E5F6-7890-ABCD-EF1234567890}is clearly hand-crafted rather than auto-generated. This is functionally fine but could collide with another hand-crafted GUID in a different branch. Consider letting Visual Studio ordotnet sln addgenerate a proper random GUID.Also applies to: 26-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maui.FreakyEffects/Maui.FreakyEffects.sln` around lines 10 - 11, The solution contains a hand-crafted project GUID ({A1B2C3D4-E5F6-7890-ABCD-EF1234567890}) in the Project(...) entry which risks collisions; replace that GUID with a proper randomly-generated GUID by removing and re-adding the test project (or using Visual Studio/dotnet sln add to re-add it) so the Project(...) line for "Maui.FreakyEffects.Tests" receives an auto-generated GUID; ensure the same change is applied to the duplicate entries referenced on lines 26-29.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@Maui.FreakyEffects/Maui.FreakyEffects.Tests/SkiaScene/TouchGestureRecognizerTests.cs`:
- Around line 69-83: The test fails because
TouchGestureRecognizer.ProcessTouchEvent calls DetectPinchAndPanGestures(type)
before removing the touch, so OnPan is fired during the Released event; update
the test in
TouchGestureRecognizerTests.ProcessTouchEvent_Released_RemovesTouchFromTracking
to expect OnPan to have fired by changing the assertion to Assert.Equal(1,
panCount) and adjust the comment to state "Pan fires on Released (1 touch)" (or
alternatively remove the final Moved event and reword the comment) so the test
reflects that DetectPinchAndPanGestures triggers OnPan on Released.
---
Duplicate comments:
In `@Maui.FreakyEffects/Maui.FreakyEffects/Shared/Skeleton/FadeAnimation.cs`:
- Around line 19-23: StopAnimation currently does an unsafe direct cast
((View)bindable) which can throw; update StopAnimation in FadeAnimation to use
safe pattern matching (e.g., if (bindable is View self) { await
self.FadeToAsync(1, 100); } else return) similar to the change suggested for
BeatAnimation.StopAnimation so the method returns early when bindable is not a
View.
---
Nitpick comments:
In `@Maui.FreakyEffects/Maui.FreakyEffects.sln`:
- Around line 10-11: The solution contains a hand-crafted project GUID
({A1B2C3D4-E5F6-7890-ABCD-EF1234567890}) in the Project(...) entry which risks
collisions; replace that GUID with a proper randomly-generated GUID by removing
and re-adding the test project (or using Visual Studio/dotnet sln add to re-add
it) so the Project(...) line for "Maui.FreakyEffects.Tests" receives an
auto-generated GUID; ensure the same change is applied to the duplicate entries
referenced on lines 26-29.
In
`@Maui.FreakyEffects/Maui.FreakyEffects.Tests/Skeleton/AnimationConstructorTests.cs`:
- Around line 74-80: The test AnimationTypes_HasFiveMembers is brittle because
it asserts a hardcoded enum count; either remove this test or convert it to an
explicit API-guard with a clear comment. Update the test file by either deleting
the AnimationTypes_HasFiveMembers method (since AnimationTypes_HasExpectedValues
already checks the named members) or keep it but add a comment above the
AnimationTypes_HasFiveMembers test explaining it intentionally guards the public
API surface and must be updated when enum members are added; ensure references
to the test name AnimationTypes_HasFiveMembers and the complementary test
AnimationTypes_HasExpectedValues are preserved so reviewers can see which
approach you chose.
In
`@Maui.FreakyEffects/Maui.FreakyEffects.Tests/TouchTracking/TouchTrackingRectTests.cs`:
- Around line 8-38: Replace the four separate Fact tests (Constructor_SetsLeft,
Constructor_SetsTop, Constructor_SetsRight, Constructor_SetsBottom) with a
single consolidated test that constructs TouchTrackingRect(10f, 20f, 30f, 40f)
once and asserts all four properties (Left, Top, Right, Bottom) in that test
(e.g., a new Fact named Constructor_InitializesAllProperties or use a
Theory+MemberData if preferred); update or remove the four original test methods
so there’s one test validating the constructor initializes all properties.
In `@Maui.FreakyEffects/Maui.FreakyEffects/Shared/Skeleton/BeatAnimation.cs`:
- Around line 19-23: BeatAnimation.StopAnimation currently does a direct cast
(var self = (View)bindable) which can throw; change it to match the safe pattern
used in HorizontalShakeAnimation/VerticalShakeAnimation by using an "if
(bindable is View self)" guard and only call self.ScaleToAsync(1, 100) inside
that block (no cast or exception). Also mirror the same guard pattern in
FadeAnimation.StopAnimation if present to keep consistency across animation
classes.
Comment on lines
+69
to
+83
| [Fact] | ||
| public void ProcessTouchEvent_Released_RemovesTouchFromTracking() | ||
| { | ||
| var recognizer = new TouchGestureRecognizer(); | ||
| int panCount = 0; | ||
| recognizer.OnPan += (_, _) => panCount++; | ||
|
|
||
| recognizer.ProcessTouchEvent(1, TouchActionType.Pressed, new SKPoint(0, 0)); | ||
| recognizer.ProcessTouchEvent(1, TouchActionType.Released, new SKPoint(5, 5)); | ||
|
|
||
| // After release, a move with the same id should do nothing | ||
| recognizer.ProcessTouchEvent(1, TouchActionType.Moved, new SKPoint(10, 10)); | ||
|
|
||
| Assert.Equal(0, panCount); // Pan fires on Released (1 touch), but not on Move after release | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "TouchGestureRecognizer.cs" --exclude "*Tests*" -type fRepository: FreakyAli/Maui.FreakyEffects
Length of output: 242
🏁 Script executed:
fd "TouchGestureRecognizer.cs" --exclude "*Tests*"Repository: FreakyAli/Maui.FreakyEffects
Length of output: 290
🏁 Script executed:
cat "Maui.FreakyEffects/Maui.FreakyEffects/Shared/SkiaScene/SkiaScene.TouchManipulation/TouchGestureRecognizer.cs"Repository: FreakyAli/Maui.FreakyEffects
Length of output: 4889
Test will fail — OnPan fires on Released with 1 touch.
The implementation calls DetectPinchAndPanGestures(type) on line 72 (in TouchGestureRecognizer.cs) before removing the touch from the dictionary. When exactly 1 touch remains, this invokes OnPan (line 120).
In this test, the Released event triggers OnPan, incrementing panCount to 1. The assertion Assert.Equal(0, panCount) will fail.
Either update the assertion to Assert.Equal(1, panCount), or remove the final Moved event (lines 80–81) and reword the comment to clarify that OnPan fires on Released.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Maui.FreakyEffects/Maui.FreakyEffects.Tests/SkiaScene/TouchGestureRecognizerTests.cs`
around lines 69 - 83, The test fails because
TouchGestureRecognizer.ProcessTouchEvent calls DetectPinchAndPanGestures(type)
before removing the touch, so OnPan is fired during the Released event; update
the test in
TouchGestureRecognizerTests.ProcessTouchEvent_Released_RemovesTouchFromTracking
to expect OnPan to have fired by changing the assertion to Assert.Equal(1,
panCount) and adjust the comment to state "Pan fires on Released (1 touch)" (or
alternatively remove the final Moved event and reword the comment) so the test
reflects that DetectPinchAndPanGestures triggers OnPan on Released.
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Tests