Add Bmotion tool (#12447)#12448
Open
msynk wants to merge 23 commits into
Open
Conversation
msynk
requested review from
Cyrus-Sushiant,
mhrastegari,
yasmoradi and
zahra-yousefi
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds the Bit.Bmotion Blazor animation library, its WebAssembly demo app, browser interop bridge, public animation models and services, routed demo pages, documentation, and MSTest coverage for easing, transforms, colors, transitions, and animation drivers. ChangesBit.Bmotion end-to-end implementation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (19)
src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor-37-37 (1)
37-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
IsPresent="true"defeats AnimatePresence exit animations for list items.The
IsPresentprop is hardcoded totrue, which means the exit animation will never play when items are removed from the list. AnimatePresence needsIsPresentto toggle tofalsebefore the item is removed from the DOM.Consider binding
IsPresentto a dictionary that tracks pending removals, or refactor to wrap the entire@foreachloop so that AnimatePresence can detect membership changes in_items.🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor` at line 37, The AnimatePresence usage currently hardcodes IsPresent="true", which prevents exit animations from running; change AnimatePresence/IsPresent so IsPresent is driven by state: either bind IsPresent to a boolean lookup (e.g., a Dictionary<Guid,bool> like pendingRemoval[item.Id]) and flip it to false before removing from _items, or instead move the AnimatePresence wrapper to surround the entire `@foreach` over _items so membership changes in _items are detected automatically; update the removal logic (the method that removes items) to first set pendingRemoval[item.Id] = false (or remove the item from whatever presence-tracking collection) and only after the exit animation completes actually remove the item from _items.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs-145-148 (1)
145-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRotation property handling differs between serialization methods.
In
ToCssStyleString(), when bothRotateandRotateZare set, they're merged into a singlerotate()transform usingRotateZ ?? Rotate(Line 146). However, inToJsDictionary(), both properties are added separately if set (Lines 91, 94). This creates different behavior between server-side and client-side rendering, potentially causing a visual jump when JavaScript takes over.♻️ Proposed fix to align both methods
-if (Rotate.HasValue) d["rotate"] = Rotate.Value; +if (Rotate.HasValue && !RotateZ.HasValue) d["rotate"] = Rotate.Value;Or alternatively, emit both in CSS as well (though this may be less clear):
-if (Rotate.HasValue || RotateZ.HasValue) - transforms.Add($"rotate({RotateZ ?? Rotate}deg)"); +if (Rotate.HasValue && !RotateZ.HasValue) + transforms.Add($"rotate({Rotate.Value}deg)"); +if (RotateZ.HasValue) + transforms.Add($"rotate({RotateZ.Value}deg)");🤖 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 `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` around lines 145 - 148, The server CSS and client JS disagree because ToCssStyleString() collapses Rotate and RotateZ into one rotate(...) transform while ToJsDictionary() currently adds both separately; update ToJsDictionary() so it mirrors ToCssStyleString() by adding a single rotate entry using RotateZ ?? Rotate (and avoid emitting both rotate and rotateZ keys), and ensure RotateX/RotateY handling remains unchanged; modify the logic around where rotate/RotateZ/Rotate are added in ToJsDictionary() to use the merged value (RotateZ ?? Rotate) and remove the duplicate emission of both properties.src/Bmotion/Bit.Bmotion/Components/Motion.cs-498-499 (1)
498-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Viewport.Onceprecedence is not respected on leave.Line 498 checks only
!Once. WhenViewportis set, docs sayViewport.Onceshould win; deactivation should use the effective once value (Viewport?.Once ?? Once).🤖 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 `@src/Bmotion/Bit.Bmotion/Components/Motion.cs` around lines 498 - 499, The leave handler currently checks only the local Once flag; change it to use the effective once value from Viewport if present: replace the condition that uses !Once with one that evaluates !(Viewport?.Once ?? Once) so the deactivation call in the block (await Engine.DeactivateGestureLayerAsync(_id, "inview")) respects Viewport.Once; reference symbols: WhileInView, Once, Viewport, Engine.DeactivateGestureLayerAsync, _id, "inview".src/Bmotion/Bit.Bmotion/Services/MotionValue.cs-32-33 (1)
32-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnumerate a subscriber snapshot before awaiting callbacks.
At Lines 32–33, callbacks can subscribe/unsubscribe while iterating, which can throw
InvalidOperationExceptionand stop notification flow.🤖 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 `@src/Bmotion/Bit.Bmotion/Services/MotionValue.cs` around lines 32 - 33, The loop over _subscribers in the MotionValue notification method can throw InvalidOperationException if subscribers change during iteration; fix by enumerating a snapshot (e.g., create a copy of _subscribers via ToList() or ToArray()) into a local variable and iterate over that snapshot when awaiting each subscriber (use the same local name like subscribers and await each sub(value)), ensuring you reference the same method in MotionValue that currently contains "foreach (var sub in _subscribers) await sub(value);".src/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cs-45-48 (1)
45-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent exceptions on invalid hex colors in
Parse.At Lines 45–48, malformed hex input throws instead of returning
null. That breaksLerp’s fallback behavior (return to) and can interrupt animation updates.🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cs` around lines 45 - 48, The Parse method currently assumes the hex substrings are valid and uses Convert.ToInt32(h[..2], 16) etc., which throws on malformed input; update Parse to first validate h.Length (must be >=6 and <=8) and then use safe parsing (int.TryParse or equivalent with NumberStyles.HexNumber and CultureInfo.InvariantCulture) for each substring used in Convert.ToInt32(h[..2], 16), Convert.ToInt32(h[2..4], 16), Convert.ToInt32(h[4..6], 16) and the optional alpha slice (h[6..8]); if any TryParse fails, return null so Lerp’s fallback behavior remains intact. Ensure the alpha branch still divides by 255.0 when parsing succeeds.src/Bmotion/Bit.Bmotion/Services/MotionValue.cs-67-82 (1)
67-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate transform ranges for minimum length and segment validity.
Current checks only compare lengths. With fewer than 2 points, Line 81 can index out of range; with repeated adjacent input points, Line 77 can divide by zero.
🤖 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 `@src/Bmotion/Bit.Bmotion/Services/MotionValue.cs` around lines 67 - 82, The Map function's validation is insufficient: before using inputRange and outputRange, ensure they each have at least 2 points (inputRange.Length >= 2) and that inputRange is strictly monotonic (e.g., for all i inputRange[i+1] != inputRange[i], ideally inputRange[i+1] > inputRange[i]) to avoid index issues and division by zero in the loop inside Map; if these checks fail throw ArgumentException with a clear message. Add these validations alongside the existing length-equality check (referencing inputRange, outputRange and the local Map(T v) function) so the loop and the denominator (inputRange[i + 1] - inputRange[i]) are guaranteed non-zero. Ensure error messages mention which condition failed (too few points or non-increasing/repeated input range points).src/Bmotion/Bit.Bmotion/Services/MotionValue.cs-25-25 (1)
25-25:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid fire-and-forget async work in
Valuesetter.Line 25 drops the
SetAsynctask, so subscriber exceptions become unobserved and rapid sets can interleave unpredictably. This needs an explicit update model (e.g., async-only setter path or synchronized queue).🤖 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 `@src/Bmotion/Bit.Bmotion/Services/MotionValue.cs` at line 25, The Value property's setter currently does a fire-and-forget call to SetAsync (set => _ = SetAsync(value)), causing unobserved exceptions and race conditions; change the update model by removing the async fire-and-forget from the setter: either make the property setter synchronous and call a synchronous helper (e.g., implement a SetSync method that awaits or locks and updates the backing field), or mark the setter obsolete/private and expose an explicit async API that callers must await (keep public Task SetAsync(T value) as the single update path); if concurrency is a concern, serialize updates inside SetAsync using a SemaphoreSlim or a dedicated request queue to ensure ordered, awaited execution and to surface exceptions from SetAsync to callers (refer to SetAsync and the Value property to apply these changes).src/Bmotion/Bit.Bmotion/Components/Motion.cs-112-116 (1)
112-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent
AdditionalAttributesfrom overriding the generated element id.At Lines 112–116, an incoming
idinAdditionalAttributescan replace_id, breaking the engine/interop contract that targets this element by_id.🤖 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 `@src/Bmotion/Bit.Bmotion/Components/Motion.cs` around lines 112 - 116, AdditionalAttributes can contain an "id" that overrides the generated _id; before calling builder.AddMultipleAttributes(seq++, AdditionalAttributes) remove any "id" key from AdditionalAttributes (or pass a filtered collection that excludes the "id" key) so the previously set builder.AddAttribute(seq++, "id", _id) remains authoritative; update the code around the builder.AddAttribute / builder.AddMultipleAttributes calls to filter out the "id" entry (e.g. call AddMultipleAttributes with AdditionalAttributes.Where(kvp => kvp.Key != "id") or copy and Remove("id")).src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js-124-129 (1)
124-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
unregisterElementreferences an undefined viewport observer variable.Line 128 uses
_vpObserver, but only_vpObserversexists. This throws during unregister/teardown and leaves viewport tracking state partially cleaned.Suggested patch
- if (_vpObserver && el) _vpObserver.unobserve(el); + if (el) _vpObservers.forEach(obs => obs.unobserve(el));🤖 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 `@src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js` around lines 124 - 129, The unregisterElement function uses a non-existent _vpObserver variable; fix it to use the actual _vpObservers collection: retrieve the observer for the element via _vpObservers.get(elementId), call observer.unobserve(el) if both observer and el exist, and then remove that observer entry from _vpObservers (in addition to the existing _vpRefs.delete and _runCleanup calls) so viewport tracking is fully cleaned up.src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js-218-236 (1)
218-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPan delta is always zero due state update order.
At Lines 225 and 234–235,
lastX/lastYare overwritten before delta is emitted, soOnPanMovereceivesdeltaX=0anddeltaY=0every time.Suggested patch
const onMove = (e) => { const dx = e.clientX - startX, dy = e.clientY - startY; + const deltaX = e.clientX - lastX, deltaY = e.clientY - lastY; const now = Date.now(), dt = now - lastT; if (dt > 0) { - velX = (e.clientX - lastX) / dt * 1000; - velY = (e.clientY - lastY) / dt * 1000; + velX = deltaX / dt * 1000; + velY = deltaY / dt * 1000; } - lastX = e.clientX; lastY = e.clientY; lastT = now; + lastX = e.clientX; lastY = e.clientY; lastT = now; @@ dotnetRef.invokeMethodAsync('OnPanMove', e.clientX, e.clientY, - e.clientX - lastX, e.clientY - lastY, + deltaX, deltaY, e.clientX - startX, e.clientY - startY, velX, velY);🤖 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 `@src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js` around lines 218 - 236, In onMove, the code updates lastX/lastY/lastT before calling dotnetRef.invokeMethodAsync('OnPanMove'), which makes the emitted delta values zero; compute deltaX and deltaY (e.g., const deltaX = e.clientX - lastX; const deltaY = e.clientY - lastY) and compute velX/velY using lastX/lastY/lastT first, then call OnPanMove with those computed deltaX/deltaY and velX/velY, and only after the invoke update lastX = e.clientX; lastY = e.clientY; lastT = now (or alternatively move the existing lastX/lastY/lastT assignments to after the invoke).src/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cs-70-76 (1)
70-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the cubic-bezier derivative used by Newton iteration.
At Line 73,
dbxomits thex2contribution in the final term, so the solver can converge to an incorrectuand return wrong easing values for some curves.Suggested patch
- double dbx = 3 * (1 - u) * (1 - u) * x1 + 6 * u * (1 - u) * x2 - 6 * u * (1 - u) * x1 + 3 * u * u; + double dbx = 3 * (1 - u) * (1 - u) * x1 + + 6 * u * (1 - u) * (x2 - x1) + + 3 * u * u * (1 - x2);🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cs` around lines 70 - 76, The Newton iteration's derivative calculation for the cubic-bezier is incorrect; replace the dbx assignment in the loop (the line that assigns to dbx using variables u, x1, x2) with the true derivative of x(u): dbx = 3 * ( (1 - u) * (1 - u) * x1 + 2 * (1 - u) * u * x2 + u * u ); this uses the correct contributions from x1 and x2 during the Newton step so the solver in the loop converges to the right u.src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs-49-55 (1)
49-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
SetInstantchanges can be lost before they are emitted as CSS updates.Line 51 returns
nullwhen there are no active drivers, and Line 54 clears_dirtyPropseach tick. That drops dirty-only updates created bySetInstant(Lines 147-173), so instant style changes may never reach output.Suggested fix
- if (_activeAnims.Count == 0 && !_isDragging) return null; - - _transformDirty = _isDragging; // drag always refreshes transform - _dirtyProps.Clear(); + if (_activeAnims.Count == 0 && !_isDragging && !_transformDirty && _dirtyProps.Count == 0) + return null; + + var pendingProps = _dirtyProps.Count > 0 ? _dirtyProps.ToArray() : Array.Empty<string>(); + bool pendingTransformDirty = _transformDirty; + + _transformDirty = _isDragging || pendingTransformDirty; // preserve pending instant updates + _dirtyProps.Clear(); + foreach (var prop in pendingProps) _dirtyProps.Add(prop);Also applies to: 74-74, 147-173
🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs` around lines 49 - 55, The Tick method currently returns null early and clears _dirtyProps each tick which causes SetInstant changes (created by SetInstant) to be dropped before being emitted; modify Tick (method Tick) so it only returns null when there are no active drivers, not dragging, AND _dirtyProps is empty (or alternatively move _dirtyProps.Clear to after emission), ensuring _dirtyProps is preserved until the CSS updates are emitted for SetInstant; update the logic around the early return and the placement of _dirtyProps.Clear so SetInstant-produced dirty properties survive the tick that emits them.src/Bmotion/Bit.Bmotion/Engine/InertiaDriver.cs-27-27 (1)
27-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard invalid inertia time constants before integration.
At Line 27,
_timeConstantSeccan be<= 0. Then Line 55 divides by it insideMath.Exp, which can produceNaN/Infinityand prevent convergence.Suggested fix
- _timeConstantSec = config.TimeConstant / 1000.0; + _timeConstantSec = Math.Max(config.TimeConstant / 1000.0, 0.001);Also applies to: 55-55
🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/InertiaDriver.cs` at line 27, The code assigns _timeConstantSec = config.TimeConstant / 1000.0 and later uses it inside Math.Exp causing NaN/Infinity if <= 0; add a guard in the InertiaDriver constructor (where _timeConstantSec is set) to validate config.TimeConstant > 0 and either throw ArgumentException or replace with a small positive epsilon (e.g. 1e-6) to avoid division by zero, and also add a defensive check right before the Math.Exp usage (the integration/update method that computes the exp factor) to handle non-positive _timeConstantSec by using the same epsilon or early-return behavior; reference the field _timeConstantSec and the integration/update method that computes Math.Exp so both assignment and usage are fixed.src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs-119-128 (1)
119-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNew
AnimateTocalls can strand previous completion awaiters.Line 127 overwrites
_completionSourceunconditionally. If another animation starts before the prior one finishes, the oldTaskCompletionSourcemay never complete.Suggested fix
- _completionSource = completionSource; + if (_completionSource != null && !ReferenceEquals(_completionSource, completionSource)) + _completionSource.TrySetCanceled(); + _completionSource = completionSource;🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs` around lines 119 - 128, The AnimateTo method overwrites the field _completionSource unconditionally, which can leave previously supplied TaskCompletionSource instances never completed; before assigning _completionSource in AnimateTo, check if an existing _completionSource is present and complete it (e.g., call TrySetCanceled or TrySetResult) to ensure awaiting callers aren't stranded, then assign the new completion source; update any related cleanup logic that completes _completionSource on animation end to handle the new behavior so no double-completion or race occurs.src/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cs-97-101 (1)
97-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
onCompletetask is not properly awaited/observed.At Line 100,
ContinueWith(_ => onComplete(), ...)creates a nested task chain that is dropped. Exceptions fromonCompletecan become unobserved.Suggested fix
- _ = tcs.Task.ContinueWith(_ => onComplete(), TaskScheduler.Default); + _ = Task.Run(async () => + { + await tcs.Task.ConfigureAwait(false); + await onComplete().ConfigureAwait(false); + });🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cs` around lines 97 - 101, The continuation created with tcs.Task.ContinueWith(_ => onComplete(), ...) drops the nested task so exceptions from onComplete go unobserved; fix by awaiting the continuation instead of dropping it — replace the ContinueWith call with explicit awaits (await tcs.Task; await onComplete();) after EnsureLoopRunningAsync() so the onComplete() Task is awaited and any exceptions are observed (refer to the local tcs variable, the onComplete delegate, and the EnsureLoopRunningAsync() call).src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs-30-66 (1)
30-66:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
ExitBeforeEnteris currently a no-op.The parameter is publicly documented, but the enter/exit state transitions never read it, so behavior is identical whether it is
trueorfalse. Please either implement the delayed-enter branch or remove the parameter until it is supported.🤖 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 `@src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs` around lines 30 - 66, The ExitBeforeEnter parameter is unused; update the lifecycle logic in OnParametersSet and the OnAllExitsComplete handler so that when ExitBeforeEnter is true new children do not render until exiting children finish: in OnParametersSet, when transitioning from present->not present keep current behavior, but when transitioning from not present->present check ExitBeforeEnter — if true set a flag (e.g., _deferEnter = true) and do not set _shouldRender = true, otherwise proceed with current re-enter branch (set _presenceCtx.IsExiting = false, _presenceCtx.Reset(), _shouldRender = true); in OnAllExitsComplete (the method subscribed in OnInitialized) clear _presenceCtx.IsExiting, set _deferEnter = false and set _shouldRender = true so entering children render, and ensure _prevIsPresent is updated appropriately after these transitions so state remains consistent.src/Bmotion/Bit.Bmotion/Services/AnimationControls.cs-35-41 (1)
35-41:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Complete()does not implement its documented behavior.The method claims to snap elements to end values, but it currently performs the same operation as
Stop()(snap to current/intermediate values). This breaks the public API contract and should be implemented distinctly (or the contract/docs adjusted to match actual behavior).src/Bmotion/Bit.Bmotion/Services/ScrollTracker.cs-24-56 (1)
24-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRepeated
ObserveAsynccalls accumulate observers but overwrite callback state.Each new observe call replaces
_onScrollbut keeps previous subscriptions active. This can cause duplicate event streams and unexpected callback routing over time. Either enforce single active subscription (replace old before adding new) or track callbacks per subscription key.Proposed fix (single-active-subscription semantics)
public async Task ObserveAsync(string? containerId, Func<ScrollInfo, Task> onChange) { ObjectDisposedException.ThrowIf(_disposed, this); + + foreach (var key in _subscriptionKeys) + await _interop.UnobserveScrollAsync(key); + _subscriptionKeys.Clear(); + _onScroll = onChange; var key = await _interop.ObserveScrollAsync(containerId, _dotnet!); if (key != null) _subscriptionKeys.Add(key); }🤖 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 `@src/Bmotion/Bit.Bmotion/Services/ScrollTracker.cs` around lines 24 - 56, The ObserveAsync method currently overwrites _onScroll while leaving previous subscriptions in _subscriptionKeys active; change ObserveAsync (and use the corresponding MotionInterop unobserve method) to first remove any existing subscriptions before adding the new one: call the interop unobserve function for each key in _subscriptionKeys (e.g., UnobserveScrollAsync or the appropriate counterpart on MotionInterop) and clear _subscriptionKeys, then set _onScroll to the new callback and add only the freshly returned key; ensure ObjectDisposedException checks and _dotnet usage remain unchanged and that you only keep a single active subscription key in _subscriptionKeys.src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs-64-75 (1)
64-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
OnAllExitsCompleteagainst stale callbacks after re-entry.After
Reset()on re-entry, lateNotifyExitComplete(...)calls from prior exiting children can still triggerAllExitsComplete, and this handler then forces_shouldRender = falseeven when content is present again.Proposed fix
public partial class AnimatePresence : ComponentBase { + private bool _awaitingExitCompletion; protected override void OnParametersSet() { if (_prevIsPresent && !IsPresent) { _presenceCtx.IsExiting = true; + _awaitingExitCompletion = true; _shouldRender = true; } else if (!_prevIsPresent && IsPresent) { _presenceCtx.IsExiting = false; _presenceCtx.Reset(); + _awaitingExitCompletion = false; _shouldRender = true; } _prevIsPresent = IsPresent; } private void OnAllExitsComplete() { + if (!_awaitingExitCompletion || IsPresent) return; + _awaitingExitCompletion = false; _shouldRender = false; _presenceCtx.IsExiting = false; InvokeAsync(StateHasChanged); } }🤖 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 `@src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs` around lines 64 - 75, OnAllExitsComplete must ignore stale callbacks after a re-entry: add an early-return guard at the top of OnAllExitsComplete that checks the current presence context state (for example, if (!_presenceCtx.IsExiting) return;) so that after Reset() or re-entry late NotifyExitComplete calls do not flip _shouldRender to false; leave the rest of the method (setting _shouldRender = false, _presenceCtx.IsExiting = false, InvokeAsync(StateHasChanged)) unchanged.
🟡 Minor comments (9)
src/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.css-5-7 (1)
5-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider providing a visible focus indicator for accessibility.
Removing
outline: noneonh1:focuseliminates the keyboard navigation focus indicator. Users navigating with keyboard need visual feedback when focus moves to headings. Consider replacing with a custom focus style instead of removing it entirely, e.g.,outline: 2px solid#0071c1; outline-offset: 2px;.🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.css` around lines 5 - 7, The h1:focus rule currently removes the keyboard focus indicator; update the CSS rule for h1:focus (or use h1:focus-visible) to provide a visible focus style instead of outline: none — for example set outline: 2px solid `#0071c1` and outline-offset: 2px (or a similarly high-contrast, accessible color/width) so keyboard users see clear focus on headings.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs-158-160 (1)
158-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing OutlineColor to CSS string serialization.
OutlineColoris serialized inToJsDictionary()(Line 102) but is missing fromToCssStyleString(). This creates an inconsistency where server-side rendered initial styles won't include outline color, potentially causing a flash of unstyles content.🎨 Proposed fix
if (BorderColor != null) sb.Append($"border-color:{BorderColor};"); +if (OutlineColor != null) sb.Append($"outline-color:{OutlineColor};"); if (Fill != null) sb.Append($"fill:{Fill};");🤖 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 `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` around lines 158 - 160, The CSS serializer ToCssStyleString is missing the OutlineColor property causing inconsistency with ToJsDictionary; update the ToCssStyleString method (near the lines that append BorderColor, Fill, Stroke) to append OutlineColor when set (e.g., if (OutlineColor != null) sb.Append($"outline-color:{OutlineColor};");) so server-side rendered styles include outline color just like ToJsDictionary does.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs-163-163 (1)
163-163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing BoxShadow to CSS string serialization.
BoxShadowis serialized inToJsDictionary()(Line 108) but is missing fromToCssStyleString(). This inconsistency means server-side rendered initial styles won't include box shadows.🎨 Proposed fix
if (BorderRadius != null) sb.Append($"border-radius:{BorderRadius};"); +if (BoxShadow != null) sb.Append($"box-shadow:{BoxShadow};"); if (PathLength.HasValue)🤖 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 `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` at line 163, ToCssStyleString() is missing serialization for the BoxShadow property (it’s present in ToJsDictionary()), so add a check similar to BorderRadius that appends the CSS box-shadow rule; specifically, in ToCssStyleString() add something like: if (BoxShadow != null) sb.Append($"box-shadow:{BoxShadow};"); so server-rendered CSS includes the same box shadow output as ToJsDictionary().src/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csproj-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorUpdate .NET 10.0 availability concern.
Bit.Bmotion.Tests.csprojtargetsnet10.0;net9.0;net8.0. .NET 10 (net10.0) is publicly released, so the “may not be available yet” concern is not valid. Keep/ensure CI and dev machines have a .NET 10 SDK installed (e.g., SDK version shipped with the current .NET 10 servicing, such as 10.0.301).🤖 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 `@src/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csproj` at line 4, The TargetFrameworks element in Bit.Bmotion.Tests.csproj should retain net10.0 (i.e., keep "net10.0;net9.0;net8.0") because .NET 10 is publicly released; remove any comment or conditional that treats net10.0 as unavailable and ensure the TargetFrameworks entry includes net10.0. Also update CI/dev environments to install/use a .NET 10 SDK (for example 10.0.301) so builds and tests can run against the net10.0 target.src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs-10-11 (1)
10-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix documentation/code inconsistency.
The comment on Line 10 states "Default: false" but the property initializer on Line 11 sets
Enabled = true. These are inconsistent.Since this is an opt-in options class (users only provide
LayoutOptionswhen they want layout animations), havingEnabled = trueby default makes sense. Please update the comment to match the code:- /// <summary>Enable automatic layout animations. Default: false.</summary> + /// <summary>Enable automatic layout animations. Default: true.</summary> public bool Enabled { get; set; } = true;🤖 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 `@src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs` around lines 10 - 11, The XML doc for the LayoutOptions.Enabled property is incorrect; change its summary to reflect the actual default (true) by updating the comment on the Enabled property so it reads "Default: true." Target the property named Enabled on the LayoutOptions class and only modify the <summary> text to match the initializer (public bool Enabled { get; set; } = true;).src/Bmotion/Bit.Bmotion/Models/TransitionConfig.cs-183-213 (1)
183-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProperties dictionary is shallow-copied, not deep-cloned.
Line 211 copies the
Propertiesdictionary reference rather than creating a new dictionary. If code mutates the cloned config'sProperties, it will affect the original. This may violate the stated intent on lines 178-181 to avoid "mutating the original's fields."🛡️ Proposed fix to deep-clone the Properties dictionary
DelayChildren = DelayChildren, When = When, - Properties = Properties, + Properties = Properties == null ? null : new Dictionary<string, TransitionConfig>(Properties.Select(kv => new KeyValuePair<string, TransitionConfig>(kv.Key, kv.Value.Clone()))), OnUpdate = OnUpdate,🤖 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 `@src/Bmotion/Bit.Bmotion/Models/TransitionConfig.cs` around lines 183 - 213, TransitionConfig.Clone currently shallow-copies the Properties dictionary (Properties = Properties) which lets mutations on the clone affect the original; change Clone to create a new dictionary instance and copy each entry into it, deep-cloning common mutable value types where appropriate (e.g., if a value is an array clone it with ((T[])value).Clone(), if it implements ICloneable call Clone(), otherwise copy the value reference), then assign that new dictionary to the Properties property; update the Clone method in class TransitionConfig to use this new dictionary so the cloned config has its own independent Properties collection.src/Bmotion/Bit.Bmotion/Components/Motion.cs-581-581 (1)
581-581:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTap listener activation should include
OnTapCancel.Line 581 enables tap listeners for
WhileTap/OnTapStart/OnTaponly. If onlyOnTapCancelis provided, tap events are never attached.🤖 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 `@src/Bmotion/Bit.Bmotion/Components/Motion.cs` at line 581, The tap listener activation condition in Motion.cs currently checks WhileTap, OnTapStart, and OnTap only, so add OnTapCancel to the predicate so tap handlers are enabled when only OnTapCancel is provided; update the conditional that sets d["tap"] (the line referencing WhileTap, OnTapStart, OnTap) to also include OnTapCancel.src/Bmotion/Bit.Bmotion/Services/AnimationController.cs-44-47 (1)
44-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
nullpassed toStop(params string[] properties).If a caller passes
nullexplicitly,properties.Lengththrows. This should be treated the same as “no properties”.Proposed fix
public void Stop(params string[] properties) { if (_elementId == null) return; - _engine.Stop(_elementId, properties.Length > 0 ? properties : null); + _engine.Stop(_elementId, properties is { Length: > 0 } ? properties : null); }🤖 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 `@src/Bmotion/Bit.Bmotion/Services/AnimationController.cs` around lines 44 - 47, The Stop method can throw when a caller explicitly passes null for the params array; update AnimationController.Stop to treat a null properties argument the same as no properties by checking properties == null || properties.Length == 0 before calling _engine.Stop so you pass null when there are no properties, e.g. compute a local props variable (or conditional) and call _engine.Stop(_elementId, props); keep the existing early return for _elementId and reference the Stop method and _engine.Stop call when making the change.src/Bmotion/Bit.Bmotion/Components/MotionConfig.razor-28-37 (1)
28-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
TransitionSpeedto prevent invalid negative durations.
TransitionSpeedis applied as a multiplier downstream; negative values can produce invalid transition timings. Guard this at the config boundary.Proposed fix
protected override void OnParametersSet() { + if (TransitionSpeed < 0) + throw new ArgumentOutOfRangeException(nameof(TransitionSpeed), "TransitionSpeed must be >= 0."); + _ctx.DefaultTransition = Transition; _ctx.ReduceMotion = ReduceMotion; _ctx.TransitionSpeed = TransitionSpeed; }🤖 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 `@src/Bmotion/Bit.Bmotion/Components/MotionConfig.razor` around lines 28 - 37, OnParametersSet currently assigns TransitionSpeed directly to _ctx.TransitionSpeed allowing negative values; add validation in OnParametersSet to clamp or reject negatives (e.g., if TransitionSpeed < 0 then set _ctx.TransitionSpeed = 0 or reset TransitionSpeed to 1.0) before assigning to the MotionConfigContext. Update the code that sets _ctx.TransitionSpeed in OnParametersSet to use the validated/clamped value and keep the public property TransitionSpeed unchanged except for validation here.
🧹 Nitpick comments (8)
src/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razor (1)
75-75: 💤 Low valueRemove empty
@codeblock.The empty
@code { }block serves no purpose and can be removed for cleaner code.🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razor` at line 75, Remove the unnecessary empty Razor code block by deleting the standalone "`@code` { }" declaration (the empty `@code` block in the Keyframes component) so the file no longer contains an unused code region.src/Bmotion/README.md (1)
115-123: 💤 Low valueAdd language identifier to fenced code block.
The fenced code block listing event callbacks should specify a language (e.g.,
text) to satisfy markdown linters and improve documentation consistency.📝 Suggested fix
-``` +```text OnHoverStart / OnHoverEnd OnTapStart / OnTap / OnTapCancel OnFocusStart / OnFocusEnd OnPanStart / OnPan / OnPanEnd (PanInfo) OnDragStart / OnDrag / OnDragEnd OnAnimationStart / OnAnimationComplete OnViewportEnter / OnViewportLeave ```🤖 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 `@src/Bmotion/README.md` around lines 115 - 123, Update the fenced code block that lists the event callbacks to include a language identifier (e.g., change the opening triple backticks from ``` to ```text) so markdown linters recognize it; ensure the closing triple backticks remain unchanged and no other text is modified.Source: Linters/SAST tools
src/Bmotion/Bit.Bmotion/Models/AnimationTarget.cs (2)
8-21: ⚡ Quick winConsider adding state validation.
The class design implies exactly one of
Props,Variant, orIsDisabledshould be set, but there's no enforcement. A default-constructedAnimationTargetor one created via object initializer could have all properties null/false, creating an invalid state. Consider making the constructor private and only exposing the implicit operators, or adding validation in a constructor.♻️ Proposed fix to prevent invalid states
-public sealed class AnimationTarget +public sealed class AnimationTarget { + private AnimationTarget() { } + /// <summary>Direct set of animation properties.</summary> public AnimationProps? Props { get; private init; }🤖 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 `@src/Bmotion/Bit.Bmotion/Models/AnimationTarget.cs` around lines 8 - 21, The class allows invalid combinations of Props/Variant/IsDisabled; add a single private ctor that enforces exactly one of Props != null, Variant != null, or IsDisabled == true and throws ArgumentException for invalid combos, then expose public static factory methods (e.g., AnimationTarget.FromProps(AnimationProps props), AnimationTarget.FromVariant(string variant), AnimationTarget.Disabled()) that call the private ctor to create valid instances; update or add any existing implicit operators to use these factories and leave the init-only properties private so instances can only be created via the validated ctor/factories (ensure HasProps and IsVariant remain derived properties).
29-30: 💤 Low valueClarify the semantics of
trueconversion.When
trueis passed, an emptyAnimationPropsinstance is created. Since an emptyAnimationPropshas no properties set, it's unclear what would animate. Consider whether this behavior is intentional or iftrueshould map to a different state (e.g., a default animation or be treated as invalid).🤖 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 `@src/Bmotion/Bit.Bmotion/Models/AnimationTarget.cs` around lines 29 - 30, The implicit bool->AnimationTarget conversion currently maps true to an empty AnimationProps which is ambiguous; update the implicit operator AnimationTarget(bool value) to either (preferred) initialize Props with a sensible default animation (e.g., AnimationProps.Default or new AnimationProps { Duration = X, Easing = Y } — pick existing defaults in the project or add a static Default on AnimationProps) so a true value means “enabled with default animation”, or (alternatively) make the operator throw an InvalidOperationException to force callers to supply explicit props; also update the XML comment on the implicit operator and add/adjust tests to reflect the chosen semantics.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs (1)
129-175: Keyframes are not rendered in server-side CSS.The
Keyframesdictionary (which can override single-value properties) is handled inToJsDictionary()but not inToCssStyleString(). This means keyframe-based animations won't have any server-side rendering, which could cause a flash of unstyled content (FOUC) before JavaScript initializes. Consider whether this is acceptable or if keyframes should fallback to their first value in the CSS string.🤖 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 `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` around lines 129 - 175, ToCssStyleString() currently ignores the Keyframes dictionary so server-rendered CSS can miss initial values; update ToCssStyleString() to honor Keyframes as a fallback source for any property not explicitly set (mirroring how ToJsDictionary() uses Keyframes): when computing transforms and simple CSS properties (X/Y/Z, Scale/ScaleX/ScaleY, Rotate/RotateX/RotateY/RotateZ, SkewX/SkewY, Perspective, Opacity, BackgroundColor, Color, BorderColor, Fill, Stroke, Width, Height, BorderRadius, PathLength, and any CssVars keys) check Keyframes for an entry and use its first keyframe value if the explicit property is null/empty, then format that value the same way the method currently formats explicit properties (e.g., produce translate/translate3d/scale/rotate/skew/perspective strings and apply culture-invariant numeric formatting); keep existing behavior when explicit properties exist and ensure PathLength clamping and stroke-dashoffset calculation still use the chosen (explicit or keyframe) value.src/Bmotion/Bit.Bmotion/Models/MotionVariants.cs (1)
12-16: ⚡ Quick winConsider documenting the overwrite behavior or adding validation.
The
Addmethod silently overwrites any existing variant with the same name (case-insensitive). While this might be intentional for a fluent API, it could mask configuration errors where a developer accidentally uses the same variant name twice.Consider either:
- Documenting this behavior in the XML comments
- Adding validation to prevent silent overwrites
📝 Option 1: Document the behavior
+ /// <summary> + /// Adds or updates a variant with the specified name. + /// If a variant with the same name already exists, it will be overwritten. + /// </summary> public MotionVariants Add(string name, AnimationProps props)🛡️ Option 2: Add validation
public MotionVariants Add(string name, AnimationProps props) { + if (_variants.ContainsKey(name)) + throw new ArgumentException($"A variant with the name '{name}' already exists.", nameof(name)); _variants[name] = props; return this; }🤖 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 `@src/Bmotion/Bit.Bmotion/Models/MotionVariants.cs` around lines 12 - 16, The Add method in MotionVariants currently overwrites existing entries in the _variants dictionary; change Add(string name, AnimationProps props) to validate first and prevent silent overwrites by checking whether _variants already contains the provided name (respecting the dictionary's case-insensitivity) and throwing an ArgumentException or InvalidOperationException with a clear message if it exists; alternatively (if you prefer keeping overwrite semantics) add/update the XML doc comment on the MotionVariants.Add method to explicitly state that calling Add will replace any existing variant with the same name and consider providing a separate AddOrReplace/Replace method for explicit replacement.src/Bmotion/Bit.Bmotion/Models/PanInfo.cs (1)
10-19: 💤 Low valueConsider removing redundant default initializers.
The
= new()initializers on lines 10, 13, 16, and 19 are redundant. The downstream consumer (Motion.OnPanMove) always constructsPanInfowith explicit property initialization, so these defaults are never used. Removing them simplifies the model and signals that callers must supply values.Alternatively, consider making these properties
init-only to prevent mutation after construction.♻️ Proposed cleanup
- public PointInfo Point { get; set; } = new(); + public PointInfo Point { get; set; } - public PointInfo Delta { get; set; } = new(); + public PointInfo Delta { get; set; } - public PointInfo Offset { get; set; } = new(); + public PointInfo Offset { get; set; } - public PointInfo Velocity { get; set; } = new(); + public PointInfo Velocity { get; set; }Or for immutability:
- public PointInfo Point { get; set; } = new(); + public PointInfo Point { get; init; }🤖 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 `@src/Bmotion/Bit.Bmotion/Models/PanInfo.cs` around lines 10 - 19, Remove the redundant "= new()" default initializers from the PanInfo properties and make them init-only to enforce construction-time assignment and immutability: change the Point, Delta, Offset, and Velocity properties to use "get; init;" (referencing the PanInfo class and its Point, Delta, Offset, Velocity properties) so callers like Motion.OnPanMove must supply values at construction and cannot mutate them afterward.src/Bmotion/Bit.Bmotion/Context/PresenceContext.cs (1)
22-27: 💤 Low valueCompletion counter may exceed child count if NotifyExitComplete is called for unregistered children.
The comparison on line 25 uses
>=which guards against over-counting, but ifNotifyExitCompleteis called for a child not in_childrenor after the list has been modified, the event may fire prematurely or multiple times. This is likely fine ifAnimatePresencestrictly controls the registration/notification lifecycle, but consider adding a guard to ensurechildis registered before incrementing.🛡️ Proposed defensive check
internal void NotifyExitComplete(Motion child) { + if (!_children.Contains(child)) return; _completedExits++; if (_completedExits >= _children.Count) AllExitsComplete?.Invoke(); }🤖 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 `@src/Bmotion/Bit.Bmotion/Context/PresenceContext.cs` around lines 22 - 27, NotifyExitComplete currently increments _completedExits unconditionally which can over-count if called for unregistered or duplicate children; update NotifyExitComplete(Motion child) to first verify the child is registered (e.g., if (_children.Contains(child)) { ... }) before incrementing, and guard against double-counting by either removing the child from _children when counted or tracking completed items with a HashSet (e.g., completedChildren.Add(child) and only increment when it was not already present); after the guarded increment, keep the existing check to invoke AllExitsComplete when _completedExits reaches _children.Count (or use completedChildren.Count for the HashSet approach).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7dbd8f9a-d4eb-4f6a-b476-c5d649958c7f
📒 Files selected for processing (69)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/AnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/Motion.cssrc/Bmotion/Bit.Bmotion/Components/MotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/MotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/PresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/VariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/ColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/IAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Engine/InertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/KeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/SpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/TransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/TweenDriver.cssrc/Bmotion/Bit.Bmotion/Interop/MotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/AnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/AnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/DragOptions.cssrc/Bmotion/Bit.Bmotion/Models/LayoutOptions.cssrc/Bmotion/Bit.Bmotion/Models/MotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/PanInfo.cssrc/Bmotion/Bit.Bmotion/Models/ScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/TransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/ViewportOptions.cssrc/Bmotion/Bit.Bmotion/Services/AnimationController.cssrc/Bmotion/Bit.Bmotion/Services/AnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/MotionValue.cssrc/Bmotion/Bit.Bmotion/Services/ScrollTracker.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
src/Bmotion/README.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the JavaScript dependency claim to avoid contradictory docs.
The README currently says “zero JavaScript dependencies” while also documenting an auto-loaded JS bridge. Consider rephrasing to “no manual JS wiring” or “no app-authored JS required.”
Suggested wording patch
-A Blazor-native animation library inspired by [Framer Motion](https://www.framer.com/motion/). Springs, gestures, layout animations, variants, and keyframes - **zero JavaScript dependencies**. All animation math runs in C# via WebAssembly. +A Blazor-native animation library inspired by [Framer Motion](https://www.framer.com/motion/). Springs, gestures, layout animations, variants, and keyframes - **no app-authored JavaScript required**. All animation math runs in C# via WebAssembly. @@ -The browser bridge (`BitBmotion.js`) ships as a static web asset of the package and is -imported automatically the first time an animation runs, so no manual `<script>` tag is -required. +The internal browser bridge (`BitBmotion.js`) ships as a static web asset of the package and is +imported automatically the first time an animation runs, so no manual `<script>` tag is required.Also applies to: 46-48
🤖 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 `@src/Bmotion/README.md` at line 3, The README contradicts itself by claiming "zero JavaScript dependencies" while also documenting an auto-loaded JS bridge. Rephrase the dependency claim in the file at lines 3 and also at lines 46-48 (which have similar wording) to clarify that no manual JavaScript wiring or app-authored JavaScript is required, rather than claiming zero JavaScript dependencies altogether. Use language like "no manual JS wiring" or "no app-authored JS required" to accurately reflect that internal JavaScript is auto-loaded but users do not need to write or manage JavaScript code themselves.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs-113-115 (1)
113-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce the documented
CssVarskey contract.
CssVarsis documented as requiring keys that start with--, but invalid keys are currently serialized as regular CSS properties. This can silently override unrelated styles instead of behaving as custom properties.Suggested fix
if (CssVars != null) foreach (var kv in CssVars) - d[kv.Key] = kv.Value; + { + if (!kv.Key.StartsWith("--", StringComparison.Ordinal)) + throw new ArgumentException("CssVars keys must start with '--'.", nameof(CssVars)); + d[kv.Key] = kv.Value; + } @@ if (CssVars != null) foreach (var kv in CssVars) - sb.Append($"{kv.Key}:{kv.Value};"); + { + if (!kv.Key.StartsWith("--", StringComparison.Ordinal)) + throw new ArgumentException("CssVars keys must start with '--'.", nameof(CssVars)); + sb.Append($"{kv.Key}:{kv.Value};"); + }Also applies to: 172-174
🤖 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 `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` around lines 113 - 115, The CssVars dictionary is being serialized without validating that keys follow the documented contract of starting with `--` (CSS custom property format). This can cause invalid keys to override unrelated styles. Add validation in both locations where CssVars are processed (lines 113-115 and lines 172-174) to check that each key in CssVars starts with `--` before adding it to the dictionary d. Either skip keys that don't meet this requirement or throw an appropriate exception to enforce the contract and prevent silent style overrides.src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs-20-24 (1)
20-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
Transitiondefault documentation with the actual declaration.The comment says this defaults to a snappy spring, but this property is declared nullable with no in-type default initializer.
🤖 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 `@src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs` around lines 20 - 24, The XML documentation comment for the Transition property in LayoutOptions claims it "Defaults to a snappy spring," but the property is declared as TransitionConfig? (nullable) with no initializer, meaning it has no actual default value. Update the documentation comment to accurately reflect that this property is nullable and either describe the actual default behavior when null, or remove the claim about defaulting to a snappy spring. Alternatively, if a snappy spring should truly be the default, add an initializer to provide that default value.src/Bmotion/Bit.Bmotion/Models/DragOptions.cs-21-21 (1)
21-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp
Elasticto the documented 0..1 range before serialization.
Elasticis documented as bounded, butToJsObject()currently forwards any value as-is.Suggested fix
- ["dragElastic"] = Elastic, + ["dragElastic"] = Math.Clamp(Elastic, 0d, 1d),Also applies to: 53-53
🤖 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 `@src/Bmotion/Bit.Bmotion/Models/DragOptions.cs` at line 21, The Elastic property is documented as bounded to a 0..1 range, but the ToJsObject() method does not enforce this constraint before serialization. Modify the ToJsObject() method to clamp the Elastic property value to the 0..1 range using Math.Max and Math.Min before including it in the serialized output. Apply the same clamping logic to the other property mentioned at line 53 that has similar bounded documentation.src/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cs-55-61 (1)
55-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd upfront selector validation in
AnimateAsync(string selector, ...).Line 60 forwards selector directly; null/whitespace should fail fast with a clear argument exception.
Proposed fix
public async ValueTask<AnimationControls> AnimateAsync( string selector, AnimationProps keyframes, TransitionConfig? transition = null) { + if (string.IsNullOrWhiteSpace(selector)) + throw new ArgumentException("Selector must be non-empty.", nameof(selector)); + var ids = await _interop.ResolveOrRegisterBySelectorAsync(selector); return StartAnimations(ids, keyframes, transition); }🤖 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 `@src/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cs` around lines 55 - 61, The AnimateAsync method accepts a selector string parameter but passes it directly to _interop.ResolveOrRegisterBySelectorAsync without validation. Add upfront validation in the AnimateAsync method to check if the selector parameter is null or empty/whitespace, and throw an ArgumentException with a descriptive message if validation fails. This should occur before calling ResolveOrRegisterBySelectorAsync to fail fast with clear error information.src/Bmotion/Bit.Bmotion/Services/AnimationController.cs-20-20 (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
elementIdinBindTobefore storing it.Line 20 accepts empty/whitespace IDs, which can surface later as runtime failures in engine/interop calls.
Proposed fix
- public void BindTo(string elementId) => _elementId = elementId; + public void BindTo(string elementId) + { + if (string.IsNullOrWhiteSpace(elementId)) + throw new ArgumentException("Element id must be non-empty.", nameof(elementId)); + _elementId = elementId; + }🤖 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 `@src/Bmotion/Bit.Bmotion/Services/AnimationController.cs` at line 20, The BindTo method in the AnimationController class does not validate the elementId parameter before storing it, allowing empty or whitespace values to be assigned to the _elementId field. This leads to runtime failures when the invalid ID is used later in engine or interop calls. Add validation logic at the start of the BindTo method to check if elementId is null, empty, or whitespace, and throw an appropriate exception (such as ArgumentException or ArgumentNullException) with a descriptive message if validation fails. Only assign the value to _elementId if validation passes.src/Bmotion/Bit.Bmotion/Engine/TransformComposer.cs-41-43 (1)
41-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid emitting identity
scale(1)inBuild.Line 41-43 currently adds
scale(...)even when the value is1, which contradicts the “empty when identity” contract and causes unnecessary style churn.Proposed fix
- if (t.TryGetValue("scale", out double scale)) - parts.Add($"scale({scale})"); + if (t.TryGetValue("scale", out double scale) && scale != 1) + parts.Add($"scale({scale})");🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/TransformComposer.cs` around lines 41 - 43, The TransformComposer Build method is currently adding scale(1) to the parts list even when the scale value is 1, which contradicts the contract to return empty content for identity values. Modify the condition in the if statement that checks t.TryGetValue("scale", out double scale) to additionally verify that scale is not equal to 1 before calling parts.Add. This prevents unnecessary style output when the scale transform is the identity value.
🧹 Nitpick comments (1)
src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs (1)
44-48: ⚡ Quick winAdd an identity test case for uniform scale.
Build_AllIdentityValues_ReturnsEmptyis a great guard, but it doesn’t include["scale"] = 1. Adding that case will better lock the identity contract.Suggested test addition
[TestMethod] public void Build_AllIdentityValues_ReturnsEmpty() { var t = new Dictionary<string, double> { ["x"] = 0, ["y"] = 0, ["rotate"] = 0 }; Assert.AreEqual(string.Empty, TransformComposer.Build(t)); } + +[TestMethod] +public void Build_UniformScaleIdentity_ReturnsEmpty() +{ + var t = new Dictionary<string, double> { ["scale"] = 1.0 }; + Assert.AreEqual(string.Empty, TransformComposer.Build(t)); +}🤖 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 `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs` around lines 44 - 48, The test method `Build_AllIdentityValues_ReturnsEmpty` currently only tests identity values for x, y, and rotate, but omits the scale property. Add the scale identity value (scale = 1) to the Dictionary initialization in this test method to ensure the identity contract is fully validated across all transform properties. This will strengthen the test coverage by verifying that the TransformComposer.Build method correctly returns an empty string when all properties including scale are at their identity values.
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor`:
- Around line 35-49: The AnimatePresence component with IsPresent="true" is
currently placed inside the foreach loop wrapping each individual item. When
RemoveItem is called, the item is removed from the _items collection before
IsPresent can transition to false, preventing the exit animation from playing.
Move the AnimatePresence wrapper outside the foreach loop to wrap the entire
collection, and ensure each Motion component has a unique `@key` attribute.
Alternatively, maintain a presence flag for each item in your data model and set
it to false before removing the item from the collection, allowing the exit
animation to complete before the item is dropped from the loop.
In `@src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs`:
- Around line 39-41: The initialization of _prevIsPresent and _shouldRender both
start as true, which incorrectly treats the first parameter set with
IsPresent=false as an exit transition. Initialize _prevIsPresent to false
instead of true to reflect the actual initial state where nothing is present, so
that an initial IsPresent=false parameter value is not treated as a leave
transition. This ensures the presence logic only treats actual state changes
from present to not-present as exit transitions, not the initial parameter set.
In `@src/Bmotion/Bit.Bmotion/Components/Motion.cs`:
- Around line 132-139: The Motion component creates a fresh VariantContext when
Variants are defined, but it only sets the ActiveVariant based on the local
Animate property without preserving any inherited active variant from an
ancestor component. When setting _ownVariantCtx.ActiveVariant, check if there's
an inherited active variant available (from a parent VariantContext) and use
that as a fallback when the current component doesn't explicitly override it
with its own Animate.Variant. This ensures that nested variant trees properly
inherit and propagate the active label through the component hierarchy, allowing
stagger and variant propagation to work correctly at all levels.
- Around line 379-385: The OnPointerUp method in Motion.cs deactivates the
gesture layer when WhileTap is active, but fails to invoke the OnTapCancel
callback when the user releases outside the element (isInsideElement is false).
This is the most common cancellation scenario. After the gesture layer
deactivation, add a check for when isInsideElement is false and invoke
OnTapCancel.InvokeAsync() to notify subscribers of the cancelled tap gesture.
In `@src/Bmotion/Bit.Bmotion/Context/PresenceContext.cs`:
- Around line 10-16: The _children field uses a List<Motion> which allows
duplicate registrations, but the completion tracking uses a HashSet that won't
account for duplicates, causing AllExitsComplete to fail when a child is
registered multiple times. Change the _children field from List<Motion> to
HashSet<Motion> to prevent duplicate registrations, and update the Register and
Unregister methods to work with the HashSet's Add and Remove operations
respectively. This ensures the count of children matches the count of completed
children for proper exit animation tracking.
In `@src/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cs`:
- Around line 199-255: The issue is that when momentum is true but both velocity
components are below the inertia threshold (0.5), the constraint snap-back logic
is skipped entirely because it lives under the else if (constraints != null)
branch. This allows the element to remain outside its bounds. Change the else if
to just if for the constraint recovery block so that snap-back logic executes
independently of the momentum state. This ensures that whenever constraints are
defined and the element is out of bounds (regardless of whether momentum
animations were triggered), the element will snap back to valid bounds using the
snapTransition.
- Around line 309-315: The EnsureLoopRunningAsync method sets _loopRunning to
true before calling the asynchronous StartRafLoopAsync operation, which means if
that call throws an exception, _loopRunning remains true while the loop never
actually started. This causes all future calls to return early without retrying.
Move the _loopRunning = true assignment to after the await
_interop.StartRafLoopAsync(_dotnet) call completes successfully, so the flag
only gets set if the startup succeeds.
In `@src/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cs`:
- Around line 16-20: The string interpolation format for the alpha value in the
Lerp method uses the current culture, which causes CSS color strings to have
commas as decimal separators in cultures with comma decimals (e.g.,
rgba(255,128,64,0,5) instead of rgba(255,128,64,0.5)), making the CSS invalid.
Replace the current culture-dependent formatting with invariant culture
formatting for the alpha value in the return statement at lines 16-20.
Additionally, apply the same invariant culture fix to any double.Parse calls and
string interpolations elsewhere in the file that handle color component values,
particularly at lines 57-78, to ensure all CSS color numbers consistently use
dots as decimal separators regardless of the user's system culture.
In `@src/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cs`:
- Around line 46-59: The ToCssString method is missing easing mode cases that
are supported by the Get(...) method, specifically Circ*, Back*, and Anticipate
modes. When Motion.PlayFlipAsync() uses this method for layout transitions,
these unsupported modes incorrectly fall through to the default "ease" case,
causing inconsistent behavior. Add switch cases in the ToCssString method for
all easing modes that Get(...) supports, either by mapping them to their CSS
cubic-bezier equivalents (for Circ* modes) or providing appropriate fallback
values for modes without direct CSS equivalents (Back* and Anticipate modes).
In `@src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs`:
- Around line 222-226: The ActivateGestureLayer method unconditionally calls
AnimateTo() without checking gesture priority, allowing lower-priority gestures
to override higher-priority ones that are already active. After storing the new
gesture layer in _gestureLayers[gesture], add a priority check before calling
AnimateTo() to ensure the newly activated gesture has sufficient priority to
animate to those values. Only proceed with the AnimateTo() call if the new
gesture's priority is higher than any other currently active gesture, mirroring
the priority logic already applied in DeactivateGestureLayer().
- Around line 144-151: The keyframe dispatch logic at lines 144-151 treats all
string arrays as color animations without verifying the property is actually a
color property, and the helper methods at lines 343-399 are too restrictive. To
fix: at lines 144-151, reorder or add property-type checks so that
CreateColorKeyframesDriver is only called when IsColorProp(key) returns true AND
the value is a string array; at lines 343-399, widen TryGetDoubleArray and
TryGetStringArray to accept IEnumerable<T> instead of just array types,
supporting List<double>, List<string>, int[], float[], and other common C#
numeric and string collection types so that inputs like new[] { 0, 100 },
List<double>, width = ["10px", "20px"] are properly recognized and dispatched to
the correct driver.
- Around line 157-183: The SetInstant method updates the backing dictionaries
but does not cancel any existing drivers/animators for those properties, which
will overwrite the instant values on the next tick. Before updating the
Transforms, StringValues, or NumericValues dictionaries in SetInstant, cancel
any active drivers for each property key being updated to ensure the instant
values are authoritative and not immediately overwritten by ongoing animations.
- Around line 185-210: The CancelProp method removes individual animation
drivers but fails to complete the _completionSource, unlike CancelAll(). When
Cancel(string[] properties) is called with specific property names, it removes
drivers one by one via CancelProp, but if those are the only active animations,
the _completionSource never gets resolved, causing AnimateToAwaitAsync() to hang
indefinitely. Modify the CancelProp method to check if _activeAnims is empty
after removing a driver, and if so, complete the _completionSource by calling
TrySetResult() and setting _completionSource to null, matching the behavior in
CancelAll().
In `@src/Bmotion/Bit.Bmotion/Engine/KeyframesDriver.cs`:
- Around line 61-63: When the repeat type is Mirror or Reverse, the code
reverses the _curFrames array but fails to reverse the corresponding Times
array, causing timing misalignment on non-uniform timelines. At the anchor site
(lines 61-63) where Array.Reverse is called on _curFrames within the numeric
driver's repeat logic, also reverse the Times array to mirror the timeline.
Apply the same fix at the sibling site (lines 147-149) in the color driver's
repeat logic, ensuring both the frame order and the timeline order are reversed
together to maintain correct segment timing.
In `@src/Bmotion/Bit.Bmotion/Models/TransitionConfig.cs`:
- Line 23: The EaseCubicBezier property at line 23 allows any double array
without validating the strict 4-item contract required for animation behavior.
Add validation logic to ensure that whenever EaseCubicBezier is set (either
through the property setter or during deserialization at line 127), it is either
null or contains exactly 4 finite numeric values. Implement this validation to
reject arrays of incorrect length or containing NaN/Infinity values before the
property is assigned or serialized. This prevents invalid animation
configuration from leaking into runtime.
In `@src/Bmotion/Bit.Bmotion/Models/ViewportOptions.cs`:
- Around line 34-40: The threshold switch expression in ViewportOptions.cs
treats the Amount property case-sensitively for the "some" and "all" cases, and
does not validate that parsed double values are finite, allowing invalid
thresholds like NaN or infinity to propagate. Convert Amount to lowercase before
the switch statement to ensure case-insensitive matching for "some" and "all",
and add a check using double.IsFinite() in the default case to validate the
parsed numeric value is finite before clamping and returning it; non-finite
values should default to 0.0.
In `@src/Bmotion/Bit.Bmotion/Services/AnimationControls.cs`:
- Around line 37-41: The Complete() method is not implementing its documented
contract of snapping animations to target end values. Instead, it calls
_engine.Stop(id, null) which is identical to the Stop() method behavior. Replace
the Stop() call in the Complete() method body with the appropriate engine method
call that actually implements the completion behavior (snapping to target end
values), rather than just halting the animation.
In `@src/Bmotion/Bit.Bmotion/Services/MotionValue.cs`:
- Around line 32-37: The subscriber invocation itself needs error handling, not
just the returned task. In SetSync, SetAsync, and the third affected location,
wrap the `sub(value)` call in a try-catch block to handle cases where a Func<T,
Task> subscriber throws synchronously before returning a task. If a subscriber
throws, catch the exception, log it appropriately, and continue invoking
remaining subscribers instead of allowing the exception to propagate and skip
the rest of the subscription chain.
- Around line 72-108: Both Transform method overloads (the generic
Transform<TOut> method at lines 72-76 and the Transform method with
inputRange/outputRange parameters at lines 81-108, as well as the related
overloads that apply similar patterns around lines 111-118) are not disposing of
the IDisposable returned by the Subscribe() call. This leaves the upstream
subscription active even after the derived MotionValue is disposed, causing
resource leaks. Store the result of each Subscribe() call and ensure it is
properly disposed when the derived MotionValue is disposed. You can accomplish
this by passing the subscription to the derived MotionValue instance so it can
manage the subscription lifecycle, or by returning a composite disposable that
includes both the derived value and the subscription, allowing the caller to
manage both resources together.
In `@src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js`:
- Around line 206-255: The _attachPan function doesn't track whether a pointer
is currently pressed, allowing stale startX/startY coordinates from previous
gestures to trigger pan detection during subsequent hover movements. Add a flag
variable to track the active pointer state (set to true in the onDown handler,
false in the onUp handler), and in the onMove handler, only perform pan
detection and invoke pan callbacks when this flag indicates a pointer is
actually down.
---
Minor comments:
In `@src/Bmotion/Bit.Bmotion/Engine/TransformComposer.cs`:
- Around line 41-43: The TransformComposer Build method is currently adding
scale(1) to the parts list even when the scale value is 1, which contradicts the
contract to return empty content for identity values. Modify the condition in
the if statement that checks t.TryGetValue("scale", out double scale) to
additionally verify that scale is not equal to 1 before calling parts.Add. This
prevents unnecessary style output when the scale transform is the identity
value.
In `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs`:
- Around line 113-115: The CssVars dictionary is being serialized without
validating that keys follow the documented contract of starting with `--` (CSS
custom property format). This can cause invalid keys to override unrelated
styles. Add validation in both locations where CssVars are processed (lines
113-115 and lines 172-174) to check that each key in CssVars starts with `--`
before adding it to the dictionary d. Either skip keys that don't meet this
requirement or throw an appropriate exception to enforce the contract and
prevent silent style overrides.
In `@src/Bmotion/Bit.Bmotion/Models/DragOptions.cs`:
- Line 21: The Elastic property is documented as bounded to a 0..1 range, but
the ToJsObject() method does not enforce this constraint before serialization.
Modify the ToJsObject() method to clamp the Elastic property value to the 0..1
range using Math.Max and Math.Min before including it in the serialized output.
Apply the same clamping logic to the other property mentioned at line 53 that
has similar bounded documentation.
In `@src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs`:
- Around line 20-24: The XML documentation comment for the Transition property
in LayoutOptions claims it "Defaults to a snappy spring," but the property is
declared as TransitionConfig? (nullable) with no initializer, meaning it has no
actual default value. Update the documentation comment to accurately reflect
that this property is nullable and either describe the actual default behavior
when null, or remove the claim about defaulting to a snappy spring.
Alternatively, if a snappy spring should truly be the default, add an
initializer to provide that default value.
In `@src/Bmotion/Bit.Bmotion/Services/AnimationController.cs`:
- Line 20: The BindTo method in the AnimationController class does not validate
the elementId parameter before storing it, allowing empty or whitespace values
to be assigned to the _elementId field. This leads to runtime failures when the
invalid ID is used later in engine or interop calls. Add validation logic at the
start of the BindTo method to check if elementId is null, empty, or whitespace,
and throw an appropriate exception (such as ArgumentException or
ArgumentNullException) with a descriptive message if validation fails. Only
assign the value to _elementId if validation passes.
In `@src/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cs`:
- Around line 55-61: The AnimateAsync method accepts a selector string parameter
but passes it directly to _interop.ResolveOrRegisterBySelectorAsync without
validation. Add upfront validation in the AnimateAsync method to check if the
selector parameter is null or empty/whitespace, and throw an ArgumentException
with a descriptive message if validation fails. This should occur before calling
ResolveOrRegisterBySelectorAsync to fail fast with clear error information.
In `@src/Bmotion/README.md`:
- Line 3: The README contradicts itself by claiming "zero JavaScript
dependencies" while also documenting an auto-loaded JS bridge. Rephrase the
dependency claim in the file at lines 3 and also at lines 46-48 (which have
similar wording) to clarify that no manual JavaScript wiring or app-authored
JavaScript is required, rather than claiming zero JavaScript dependencies
altogether. Use language like "no manual JS wiring" or "no app-authored JS
required" to accurately reflect that internal JavaScript is auto-loaded but
users do not need to write or manage JavaScript code themselves.
---
Nitpick comments:
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs`:
- Around line 44-48: The test method `Build_AllIdentityValues_ReturnsEmpty`
currently only tests identity values for x, y, and rotate, but omits the scale
property. Add the scale identity value (scale = 1) to the Dictionary
initialization in this test method to ensure the identity contract is fully
validated across all transform properties. This will strengthen the test
coverage by verifying that the TransformComposer.Build method correctly returns
an empty string when all properties including scale are at their identity
values.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 389d34b5-0098-418e-92a9-916633506126
📒 Files selected for processing (69)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/AnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/Motion.cssrc/Bmotion/Bit.Bmotion/Components/MotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/MotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/PresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/VariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/ColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/IAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Engine/InertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/KeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/SpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/TransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/TweenDriver.cssrc/Bmotion/Bit.Bmotion/Interop/MotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/AnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/AnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/DragOptions.cssrc/Bmotion/Bit.Bmotion/Models/LayoutOptions.cssrc/Bmotion/Bit.Bmotion/Models/MotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/PanInfo.cssrc/Bmotion/Bit.Bmotion/Models/ScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/TransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/ViewportOptions.cssrc/Bmotion/Bit.Bmotion/Services/AnimationController.cssrc/Bmotion/Bit.Bmotion/Services/AnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/MotionValue.cssrc/Bmotion/Bit.Bmotion/Services/ScrollTracker.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (2)
src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs (1)
169-176:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPath drawing output is still using the broken dash math.
Line 174/175 and Line 234/235 still emit normalized dash values that don’t reliably produce progressive SVG path drawing. This is the same unresolved issue previously reported, now duplicated in both CSS emitters (
ToCssStyleStringandToCssStyleDictionary).Also applies to: 229-236
🤖 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 `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs` around lines 169 - 176, The path drawing output contains broken dash math that does not reliably produce progressive SVG path drawing, and this issue is duplicated across two CSS emitter methods. In the ToCssStyleString method (around lines 169-176 with stroke-dasharray and stroke-dashoffset calculations), fix the normalized dash value calculations used in the stroke-dasharray and stroke-dashoffset assignments to use correct math for progressive path drawing. Then apply the same corrected formula to the duplicate code in ToCssStyleDictionary method (around lines 229-236) which has the identical stroke-dasharray and stroke-dashoffset calculations. Both methods must use consistent and correct dash math to ensure reliable progressive SVG path drawing behavior.src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs (1)
20-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the documented cubic-bezier invariant at assignment time.
Line 20 documents a strict 4-value bezier contract, but Line 22 currently accepts any array and Line 129 clones it unchanged. Please validate
EaseCubicBezieras eithernullor exactly 4 finite values to prevent invalid transition configs from reaching runtime.Proposed minimal fix
- public double[]? EaseCubicBezier { get; set; } + private double[]? easeCubicBezier; + public double[]? EaseCubicBezier + { + get => easeCubicBezier; + set + { + if (value is not null && (value.Length != 4 || value.Any(v => !double.IsFinite(v)))) + throw new ArgumentException("EaseCubicBezier must contain exactly 4 finite values [x1, y1, x2, y2]."); + easeCubicBezier = value; + } + }Also applies to: 129-129
🤖 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 `@src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs` around lines 20 - 23, The EaseCubicBezier property currently lacks validation despite its documentation requiring exactly 4 finite values in the format [x1, y1, x2, y2]. Add validation logic to the EaseCubicBezier property setter (around line 22) to ensure the assigned value is either null or an array with exactly 4 finite numeric values; reject any other input. Additionally, ensure the cloning operation at line 129 also validates the array to maintain the invariant, so invalid configs cannot persist through serialization or copying operations.
🟡 Minor comments (8)
src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor-47-48 (1)
47-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to the remove button.
At Line 47–48, the icon-only
✕button has no explicit accessible label, so screen readers may announce it ambiguously. Add anaria-label(and mirror the same change in the_listCodesnippet around Line 170–171 for consistency).Suggested fix
- <button style="background:none;border:none;color:`#ff4785`;cursor:pointer;font-size:1.1rem" - `@onclick`="@(() => RemoveItem(item.Id))">✕</button> + <button style="background:none;border:none;color:`#ff4785`;cursor:pointer;font-size:1.1rem" + aria-label="@($"Remove {item.Label}")" + title="@($"Remove {item.Label}")" + `@onclick`="@(() => RemoveItem(item.Id))">✕</button>🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor` around lines 47 - 48, The remove button displaying the ✕ icon lacks an accessible label for screen readers. Add an aria-label attribute to the button element in the RemoveItem button (around lines 47-48) with a descriptive label such as "Remove item". Apply the same aria-label attribute to the corresponding remove button in the _listCode snippet (around lines 170-171) to maintain consistency across both locations.src/Bmotion/Bit.Bmotion/Components/BmotionConfig.razor-35-35 (1)
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
TransitionSpeedagainst non-finite values.
TransitionSpeed < 0 ? 0 : TransitionSpeedstill allowsNaN/Infinity, which can break downstream duration calculations.Suggested fix
- _ctx.TransitionSpeed = TransitionSpeed < 0 ? 0 : TransitionSpeed; + _ctx.TransitionSpeed = double.IsFinite(TransitionSpeed) + ? Math.Max(0, TransitionSpeed) + : 1.0;🤖 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 `@src/Bmotion/Bit.Bmotion/Components/BmotionConfig.razor` at line 35, The guard for TransitionSpeed in the BmotionConfig.razor component only checks if the value is negative but allows non-finite values like NaN and Infinity to pass through, which can break downstream duration calculations. Modify the assignment of _ctx.TransitionSpeed to validate not only that it is non-negative but also that it is a finite number by using a method like double.IsFinite() in addition to the negative check, defaulting to 0 for any invalid values.src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs-68-76 (1)
68-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate subscriber callbacks for null.
Adding null callbacks currently defers failures to notification time; reject invalid registrations immediately.
Suggested fix
public IDisposable Subscribe(Func<T, Task> callback) { + ArgumentNullException.ThrowIfNull(callback); _subscribers.Add(callback); return new Subscription(() => _subscribers.Remove(callback)); } public IDisposable Subscribe(Action<T> callback) - => Subscribe(v => { callback(v); return Task.CompletedTask; }); + { + ArgumentNullException.ThrowIfNull(callback); + return Subscribe(v => { callback(v); return Task.CompletedTask; }); + }🤖 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 `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs` around lines 68 - 76, Both Subscribe method overloads accept null callbacks without validation, allowing the failure to be deferred until notification time instead of rejecting the invalid registration immediately. Add null validation at the start of each Subscribe overload to throw ArgumentNullException if the callback parameter is null. Apply this check to the async version Subscribe(Func<T, Task> callback) and the synchronous convenience overload Subscribe(Action<T> callback) before the callback is used or added to the collection.src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs-47-47 (1)
47-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject null
onChangeat subscription time.Failing fast here avoids deferred null callback failures during scroll events.
Suggested fix
public async Task ObserveAsync(string? containerId, Func<BmotionScrollInfo, Task> onChange) { + ArgumentNullException.ThrowIfNull(onChange); ObjectDisposedException.ThrowIf(_disposed, this);🤖 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 `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs` at line 47, Add a null guard at the start of the ObserveAsync method to validate that the onChange parameter is not null. If onChange is null, throw an ArgumentNullException with an appropriate parameter name. This prevents deferred failures when the callback is invoked during scroll events by catching the issue early at subscription time.src/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cs-7-8 (1)
7-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winXML docs contradict the actual API contract.
Line 7 says
Null = both axes, butAxisis a non-nullableBmotionDragAxis(Line 8) and defaults toBoth. Please align the comment to avoid misleading IntelliSense/API usage.🤖 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 `@src/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cs` around lines 7 - 8, The XML documentation on the Axis property in BmotionDragOptions contradicts the actual implementation. The property is non-nullable with a default value of BmotionDragAxis.Both, not nullable. Update the summary documentation for the Axis property to remove the reference to "Null = both axes" and instead clarify that the property defaults to Both when not explicitly set, reflecting the actual non-nullable contract of the property.src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs-17-22 (1)
17-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject negative
TransitionSpeedvalues.Line 21 accepts any
double; negative speeds break the documented timing contract and can silently collapse animations instead of scaling them.Suggested fix
-public double TransitionSpeed { get; set; } = 1.0; +private double _transitionSpeed = 1.0; +public double TransitionSpeed +{ + get => _transitionSpeed; + set => _transitionSpeed = value < 0 + ? throw new ArgumentOutOfRangeException(nameof(value), "TransitionSpeed must be >= 0.") + : value; +}🤖 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 `@src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs` around lines 17 - 22, The TransitionSpeed property currently accepts any double value without validation, but negative speeds are invalid according to the documented contract and will cause animations to malfunction. Add validation to the TransitionSpeed property setter to reject negative values by throwing an appropriate exception (such as ArgumentException or ArgumentOutOfRangeException) when a negative value is assigned, ensuring that only non-negative values are accepted.src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs-494-497 (1)
494-497:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Cast<string>()may throw if not all elements are strings.The check
oa[0] is stringonly verifies the first element. If subsequent elements are non-strings,Cast<string>()will throwInvalidCastException.Proposed fix
if (value is object[] oa && oa.Length > 0 && oa[0] is string) { - result = oa.Cast<string>().ToArray(); + if (oa.All(x => x is string)) + { + result = oa.Cast<string>().ToArray(); + return true; + } - return true; + return false; }🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs` around lines 494 - 497, The current code validates only the first element with `oa[0] is string`, but `Cast<string>()` will throw `InvalidCastException` if any subsequent element is not a string. Fix this by adding a validation check that ensures ALL elements in the `oa` array are strings before calling `Cast<string>()`. Use `oa.All(x => x is string)` in addition to (or instead of) the existing `oa[0] is string` check to validate the entire array before the cast operation.src/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cs-17-19 (1)
17-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the precision contract mismatch in
Num(double).Line 17 says “full round-trip precision,” but Line 19 formats with
"G6"(lossy). Either change the format to a round-trip one ("R"/"G17") or update the XML summary to match the intended compact formatting.💡 Minimal fix (doc-aligned)
- /// <summary>Formats a double as an invariant-culture string with full round-trip precision.</summary> + /// <summary>Formats a double as a compact invariant-culture string (6 significant digits).</summary> public static string Num(double value) => value.ToString("G6", CultureInfo.InvariantCulture);🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cs` around lines 17 - 19, The `Num(double)` method has a contract mismatch between its XML documentation and implementation. The documentation states it provides "full round-trip precision," but the implementation uses `"G6"` format which is lossy and not full round-trip. Either replace the format string `"G6"` with `"R"` (or `"G17"`) to achieve actual round-trip precision, or update the XML summary comment to accurately describe the lossy `"G6"` compact formatting behavior to match the intended implementation.
🧹 Nitpick comments (1)
src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor (1)
53-54: UseOnDragStartto avoid redundant per-frame status updates during drag.
OnDragfires continuously while dragging and sets_dragInfoto the same"Dragging…"value each frame, causing unnecessary renders. Move this status update toOnDragStart, which fires only once at drag initiation, keepingOnDragEndfor the"Released"state.🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor` around lines 53 - 54, The OnDrag event handler fires continuously during drag operations, causing redundant per-frame updates to _dragInfo with the "Dragging…" status and unnecessary renders. Add an OnDragStart event binding to the component and create a HandleDragStart method that sets _dragInfo to "Dragging…" once at drag initiation. Remove or refactor the HandleDrag method to eliminate the redundant status update, keeping only the necessary drag tracking logic. Keep HandleDragEnd for setting the "Released" state when the drag completes.
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razor`:
- Around line 40-46: The CopyAsync method uses InvokeVoidAsync which discards
the boolean result returned by the bmCopyToClipboard JavaScript function,
causing _copied to be set to true even when the copy operation fails. Change the
InvokeVoidAsync call to InvokeAsync<bool> to capture the actual boolean result
from bmCopyToClipboard, then only set _copied = true when the returned value is
actually true, not unconditionally.
In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 130-135: The _ownVariantCtx.InitialVariant assignment is resetting
to null when there is no local Initial variant, but it should inherit from the
parent context instead. Modify the InitialVariant assignment to follow the same
fallback pattern as ActiveVariant: when Initial?.IsVariant is false, use
VariantCtx?.InitialVariant as the fallback value instead of null. This ensures
that descendants preserve inherited initial variant labels when the current node
defines its own Variants without defining a local Initial variant.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cs`:
- Around line 45-48: The field _shouldRender is initialized to true, but when
IsPresent is false on the initial render, the OnParametersSet method does not
set _shouldRender to false for the false→false transition case, causing children
to mount despite being initially absent. Change the initialization of
_shouldRender from true to false so that the component does not render when
IsPresent is false on first render, then ensure the transition logic in
OnParametersSet (around lines 60-85) properly handles all state transitions to
update _shouldRender accordingly.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs`:
- Around line 33-35: Both keyframe driver constructors lack input validation
which can cause crashes during animation. In
src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs (lines 33-35),
add validation to ensure frames.Length is at least 2 and that the Times array
length matches frames.Length, throwing an appropriate exception if either
condition fails. Apply the identical validation logic to
src/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cs (lines 33-36) to
maintain consistent safety guarantees across both implementations and prevent
interpolation indexing failures.
- Around line 34-35: Both BmotionNumericKeyframesDriver.cs (lines 34-35) and
BmotionColorKeyframesDriver.cs (lines 34-35) are storing direct references to
the config.Times array and then mutating it during mirror/reverse operations,
which modifies the caller's original data. Clone the config.Times array
immediately after the null-coalescing assignment (before any subsequent
mutations or operations) in both files by calling .ToArray() or .Clone() on the
assigned value, so that _times holds an independent copy that can be safely
modified without affecting the caller's original configuration object.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cs`:
- Around line 49-50: The rotation assignment at line 49 in the
BmotionTransformComposer.cs file incorrectly prioritizes a zero value from
"rotateZ" over a non-zero "rotate" value. Modify the logic for assigning the rz
variable so that it only uses the "rotateZ" value when it is non-zero (excluding
the identity case), and falls back to "rotate" when "rotateZ" is absent or is
zero. This ensures that non-zero rotation values from either property are
preserved in the output.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cs`:
- Around line 36-38: All four animation driver implementations incorrectly apply
a final value when cancelled instead of freezing at the current position,
violating the documented interface contract. In the Tick method of each driver
class, remove the _apply() call from the cancellation check: in
BmotionTweenDriver.cs (lines 36-38) change the condition to just `if
(_cancelled) return true;` by removing `_apply(_to);`, in
BmotionColorTweenDriver.cs (lines 36-38) do the same by removing `_apply(_to);`,
in BmotionInertiaDriver.cs (lines 41-43) change to `if (_cancelled) return
true;` by removing `_apply(_projected);`, and in BmotionSpringDriver.cs (lines
67-69) change to `if (_cancelled) return true;` by removing `_apply(_target);`.
This ensures animations freeze at their current intermediate position when
cancelled rather than jumping to the target value.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cs`:
- Around line 51-54: The `keyframes` parameter in the `AnimateAsync` method
signature (at lines 51-54) is not being validated before use, and when
`keyframes.ToJsDictionary()` is called, it can throw if null is passed. Add an
`ArgumentNullException` validation at the beginning of the `AnimateAsync` method
body to fail fast at the API boundary. Apply the same null validation for the
`keyframes` parameter at the other affected locations (lines 74-77 and line 90)
where similar keyframes parameter usage occurs without validation.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cs`:
- Around line 20-26: The BindTo method registers new element IDs via
_engine.RegisterElement but never unregisters the previously bound element ID
before rebinding. When BindTo is called multiple times with different IDs, the
old registrations accumulate in the engine causing a leak. Before registering
the new elementId, check if _elementId is already set to a non-empty value and
unregister it via _engine.UnregisterElement. Additionally, consider implementing
IDisposable on the BmotionAnimationController class to unregister the element
when the controller's lifetime ends.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Around line 67-75: The OnScroll method does not handle exceptions that may be
thrown by the _onScroll callback, which can destabilize the interop flow and
host circuit/runtime. Wrap the await _onScroll(info) invocation in a try-catch
block to catch and handle any exceptions thrown by the callback. Log or handle
the exception appropriately to prevent it from propagating and faulting the
JS-invokable callback flow.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs`:
- Around line 96-108: The Transform method can be called with non-numeric
generic types like Guid or DateTime, and Convert.ToDouble(v) in the nested Map
function will throw a runtime exception with an unclear error message. Add an
explicit check early in the Transform method to verify that the generic type T
is numeric (such as int, double, float, decimal, etc.), and if not, throw an
ArgumentException with a clear message indicating that Transform only works with
numeric types. This ensures the error is caught and reported clearly rather than
bubbling up from Convert.ToDouble with a cryptic message.
In `@src/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.js`:
- Around line 36-48: The `_tick` function is vulnerable to exceptions from the
`invokeMethod('ComputeFrame', timestamp)` call, which can skip the
`requestAnimationFrame(_tick)` scheduling on line 47 and stall animations
indefinitely. Wrap the `invokeMethod` call and the subsequent style application
logic in a try-catch block to safely handle any exceptions, ensuring that
`requestAnimationFrame(_tick)` is always executed after the try-catch block
completes, regardless of whether an exception occurred. This guarantees the
animation loop continues even after transient frame failures.
- Around line 370-378: The observeViewport function can create duplicate
IntersectionObserver subscriptions for the same element when called multiple
times with different options, as line 377 reuses or creates a new observer
without first removing the element from any previously active observer. Before
calling _getVpObserver(margin, threshold).observe(el), you need to check if the
element is already being observed and unobserve it from its previous observer to
prevent stacking subscriptions. This ensures that only one observer subscription
exists per element and that the once behavior works correctly across option
changes.
In `@src/Bmotion/README.md`:
- Around line 54-62: Update all code examples in the README to use the actual
prefixed API names from the implementation. Replace all unprefixed component and
class names with their Bmotion-prefixed equivalents: change <Motion> to
<Bmotion>, <AnimatePresence> to <BmotionAnimatePresence>, <MotionConfig> to
<BmotionConfig>, AnimationProps to BmotionAnimationProps, TransitionConfig to
BmotionTransitionConfig, ViewportOptions to BmotionViewportOptions, Easing to
BmotionEasing, and RepeatType to BmotionRepeatType. Ensure all examples
throughout the README documentation use the correct prefixed names so users can
copy and use the code without compilation errors.
---
Minor comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor`:
- Around line 47-48: The remove button displaying the ✕ icon lacks an accessible
label for screen readers. Add an aria-label attribute to the button element in
the RemoveItem button (around lines 47-48) with a descriptive label such as
"Remove item". Apply the same aria-label attribute to the corresponding remove
button in the _listCode snippet (around lines 170-171) to maintain consistency
across both locations.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionConfig.razor`:
- Line 35: The guard for TransitionSpeed in the BmotionConfig.razor component
only checks if the value is negative but allows non-finite values like NaN and
Infinity to pass through, which can break downstream duration calculations.
Modify the assignment of _ctx.TransitionSpeed to validate not only that it is
non-negative but also that it is a finite number by using a method like
double.IsFinite() in addition to the negative check, defaulting to 0 for any
invalid values.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs`:
- Around line 17-22: The TransitionSpeed property currently accepts any double
value without validation, but negative speeds are invalid according to the
documented contract and will cause animations to malfunction. Add validation to
the TransitionSpeed property setter to reject negative values by throwing an
appropriate exception (such as ArgumentException or ArgumentOutOfRangeException)
when a negative value is assigned, ensuring that only non-negative values are
accepted.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cs`:
- Around line 17-19: The `Num(double)` method has a contract mismatch between
its XML documentation and implementation. The documentation states it provides
"full round-trip precision," but the implementation uses `"G6"` format which is
lossy and not full round-trip. Either replace the format string `"G6"` with
`"R"` (or `"G17"`) to achieve actual round-trip precision, or update the XML
summary comment to accurately describe the lossy `"G6"` compact formatting
behavior to match the intended implementation.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs`:
- Around line 494-497: The current code validates only the first element with
`oa[0] is string`, but `Cast<string>()` will throw `InvalidCastException` if any
subsequent element is not a string. Fix this by adding a validation check that
ensures ALL elements in the `oa` array are strings before calling
`Cast<string>()`. Use `oa.All(x => x is string)` in addition to (or instead of)
the existing `oa[0] is string` check to validate the entire array before the
cast operation.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cs`:
- Around line 7-8: The XML documentation on the Axis property in
BmotionDragOptions contradicts the actual implementation. The property is
non-nullable with a default value of BmotionDragAxis.Both, not nullable. Update
the summary documentation for the Axis property to remove the reference to "Null
= both axes" and instead clarify that the property defaults to Both when not
explicitly set, reflecting the actual non-nullable contract of the property.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Line 47: Add a null guard at the start of the ObserveAsync method to validate
that the onChange parameter is not null. If onChange is null, throw an
ArgumentNullException with an appropriate parameter name. This prevents deferred
failures when the callback is invoked during scroll events by catching the issue
early at subscription time.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs`:
- Around line 68-76: Both Subscribe method overloads accept null callbacks
without validation, allowing the failure to be deferred until notification time
instead of rejecting the invalid registration immediately. Add null validation
at the start of each Subscribe overload to throw ArgumentNullException if the
callback parameter is null. Apply this check to the async version
Subscribe(Func<T, Task> callback) and the synchronous convenience overload
Subscribe(Action<T> callback) before the callback is used or added to the
collection.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs`:
- Around line 169-176: The path drawing output contains broken dash math that
does not reliably produce progressive SVG path drawing, and this issue is
duplicated across two CSS emitter methods. In the ToCssStyleString method
(around lines 169-176 with stroke-dasharray and stroke-dashoffset calculations),
fix the normalized dash value calculations used in the stroke-dasharray and
stroke-dashoffset assignments to use correct math for progressive path drawing.
Then apply the same corrected formula to the duplicate code in
ToCssStyleDictionary method (around lines 229-236) which has the identical
stroke-dasharray and stroke-dashoffset calculations. Both methods must use
consistent and correct dash math to ensure reliable progressive SVG path drawing
behavior.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs`:
- Around line 20-23: The EaseCubicBezier property currently lacks validation
despite its documentation requiring exactly 4 finite values in the format [x1,
y1, x2, y2]. Add validation logic to the EaseCubicBezier property setter (around
line 22) to ensure the assigned value is either null or an array with exactly 4
finite numeric values; reject any other input. Additionally, ensure the cloning
operation at line 129 also validates the array to maintain the invariant, so
invalid configs cannot persist through serialization or copying operations.
---
Nitpick comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor`:
- Around line 53-54: The OnDrag event handler fires continuously during drag
operations, causing redundant per-frame updates to _dragInfo with the
"Dragging…" status and unnecessary renders. Add an OnDragStart event binding to
the component and create a HandleDragStart method that sets _dragInfo to
"Dragging…" once at drag initiation. Remove or refactor the HandleDrag method to
eliminate the redundant status update, keeping only the necessary drag tracking
logic. Keep HandleDragEnd for setting the "Released" state when the drag
completes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f06b4101-1a53-444c-9d4b-c29e7dde1965
📒 Files selected for processing (80)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor`:
- Around line 46-54: The description text at line 46 mentions listening to
OnDragStart, OnDrag, and OnDragEnd events, but the Bmotion component only wires
callbacks for OnDragStart and OnDragEnd (the OnDrag callback is missing). Update
the description text to accurately reflect only the events that are actually
being listened to in the component. The same mismatch also exists at lines
145-153, so both locations need to be corrected to align the event descriptions
with the actual wired callbacks.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razor`:
- Around line 80-84: The AddEvent method continuously appends to the _events
list without any size limit, causing unbounded memory growth even though only
the last 8 entries are rendered to the UI. Add logic to the AddEvent method to
cap the _events list size—after adding a new event, check if the list exceeds a
reasonable maximum (likely 8 entries to match what's displayed) and remove older
entries from the beginning of the list to maintain a fixed size. This prevents
long-running sessions from accumulating unnecessary memory.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razor`:
- Line 8: The ToString("F1") formatting at line 8 of ScrollAnimations.razor is
culture-sensitive and produces invalid CSS in locales with comma decimal
separators (e.g., "12,3%" instead of "12.3%"). Replace the ToString("F1") call
on the _progressY calculation with invariant culture formatting to ensure
CSS-valid output regardless of locale. Apply the same invariant culture
formatting fix to the identical pattern at line 86 in ScrollAnimations.razor to
maintain consistency across both locations where percentage widths are
dynamically calculated.
In `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/index.html`:
- Around line 37-47: The textarea cleanup code (document.body.removeChild(ta))
is only executed in the success path, so if ta.focus(), ta.select(), or
document.execCommand('copy') throws an exception, the textarea element remains
in the DOM. Restructure the try-catch block to include a finally block that
ensures document.body.removeChild(ta) always executes, guaranteeing cleanup
regardless of whether an exception occurs during the copy operation sequence.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cs`:
- Around line 64-76: The exit-transition bookkeeping in the condition checking
_prevIsPresent && !IsPresent has two issues: First, _presenceCtx.IsExiting is
set unconditionally even when BmotionPresenceContext.ChildCount is zero, causing
AllExitsComplete to never fire and _shouldRender to remain true indefinitely.
Second, _deferEnter is not cleared when entering exit mode, allowing stale
deferred-enter state to incorrectly persist and remount after a later exit
completes. Fix this by conditionally setting _presenceCtx.IsExiting only when
ChildCount > 0, and explicitly clear the _deferEnter flag when a new leave
occurs in the _prevIsPresent && !IsPresent branch.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs`:
- Around line 21-30: The TransitionSpeed property setter currently only
validates that the value is non-negative but does not reject NaN or Infinity
values, which can propagate invalid timing values into animation math. Modify
the validation logic in the TransitionSpeed setter to also check that the value
is finite using double.IsFinite(), rejecting any non-finite values (NaN,
positive or negative Infinity) with an appropriate exception message alongside
the existing non-negative check.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs`:
- Around line 9-33: The _nextChildIndex field continuously increments without
being reset, causing stagger delays computed in GetChildDelay to grow unbounded
in reused trees or subsequent variant cycles. Create a ResetChildrenOrder()
method that resets _nextChildIndex to 0, and ensure this method is called
whenever the parent component starts a fresh animation or variant cycle to
maintain correct per-cycle stagger ordering for RegisterChild() and
GetChildDelay().
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cs`:
- Around line 56-57: The regex patterns in the BmotionColorInterpolator class
are case-sensitive while the LooksLikeColor method accepts color functions
case-insensitively, causing uppercase inputs like RGB(...) or HSL(...) to be
detected but fail parsing. Add RegexOptions.IgnoreCase flag to the Regex.Match
calls in the Parse method for both the rgba pattern at lines 56-57 and the hsla
pattern at lines 70-71 to make them case-insensitive and properly handle color
inputs regardless of case.
- Around line 11-12: The Lerp method in BmotionColorInterpolator is parsing the
from and to color parameters on every frame tick, causing expensive regex
operations to repeat at ~60fps. Move the color parsing from the Lerp method to
the driver initialization phase in BmotionColorKeyframesDriver (around Line 62),
store the parsed color channel values, and modify Lerp to accept pre-parsed
channel data and perform only the interpolation calculation without any parsing
overhead.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cs`:
- Around line 28-30: The `frames` parameter is assigned directly to the
`_frames` field without cloning, which allows callers to mutate the original
array and affect the driver's behavior after construction. Clone the `frames`
parameter when assigning it to `_frames`, using the same cloning pattern already
applied to `_curFrames` (i.e., `_frames = (string[])frames.Clone();`), to keep
the driver's behavior deterministic and prevent external mutations from altering
`Cancel()` or `Complete()` outcomes.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cs`:
- Around line 113-120: The issue is a race condition where multiple concurrent
AnimateAsync calls on the same element can cause premature UnregisterElement
calls while other animations are still active, combined with unsynchronized
access to the _elements dictionary in BmotionAnimationEngine. Fix this by
implementing reference counting: modify RegisterElement and UnregisterElement
methods to track the number of active animations per element (increment on
register, decrement on completion), and only actually unregister when the count
reaches zero. Additionally, synchronize all access to the _elements dictionary
by either wrapping all RegisterElement, UnregisterElement, and rAF loop
dictionary accesses with a lock, or replace the dictionary with
ConcurrentDictionary to ensure thread-safe mutations across the service and
engine components.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cs`:
- Around line 32-49: Add null guards for the `props` parameter in the three
public methods AnimateAsync, AnimateAwaitAsync, and Set to prevent
NullReferenceException when null is passed as input. For each method, add a null
check for `props` at the beginning (similar to the existing check for
`_elementId`) and return early if it is null, ensuring the method does not
proceed to call `props.ToJsDictionary()` with a null value.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Around line 62-63: The ObserveAsync overload that accepts an
Action<BmotionScrollInfo> onChange parameter does not validate that onChange is
not null before wrapping it in a lambda and passing it to the other ObserveAsync
overload. Add a null check at the beginning of this ObserveAsync method to throw
an ArgumentNullException if onChange is null, ensuring fail-fast validation
instead of allowing the null to be wrapped and cause a fault later when the
callback is actually invoked.
In `@src/Bmotion/README.md`:
- Line 1: Change the README title at line 1 from "# bit Bmotion" to "#
Bit.Bmotion" to align with the consistent package and namespace naming
convention used throughout the codebase.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs`:
- Around line 126-130: The cancel contract is inconsistent: tests assume
Cancel() snaps the value to the target, but the driver implementation treats
Cancel() as "complete immediately" without writing the target value on that
tick. In src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs at
lines 126-130, remove or modify the assertion Assert.AreEqual(100.0, lastValue,
1e-5) because Cancel() does not guarantee the lastValue reaches the target—only
that the operation completes; keep Assert.IsTrue(done) to validate completion.
Apply the same fix to
src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cs at lines 111-115:
remove any assertion that expects the value to equal the target after Cancel(),
and retain only the assertion that validates completion.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb1e4d1e-4049-4d59-98ae-f53c270c2d71
📒 Files selected for processing (80)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor (1)
46-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDescription claims "real-time position feedback" but demo shows lifecycle status.
Line 46 describes the demo as providing "real-time position feedback," but the wired callbacks (
OnDragStart/OnDragEndat Lines 53-54) are lifecycle events, not continuous updates. The displayed status (_dragInfo) shows "Dragging…" and "Released"—lifecycle transitions, not position data. Real-time position feedback would require theOnDragcallback.📝 Suggested fix
- <p>Listen to <c>OnDragStart</c>, <c>OnDragEnd</c> for real-time position feedback.</p> + <p>Listen to <c>OnDragStart</c> and <c>OnDragEnd</c> for drag lifecycle feedback.</p>Apply the same fix to the snippet at Line 145:
- <p>Listen to <c>OnDragStart</c>, <c>OnDragEnd</c> for real-time position feedback.</p> + <p>Listen to <c>OnDragStart</c> and <c>OnDragEnd</c> for drag lifecycle feedback.</p>🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor` at line 46, The description text at line 46 incorrectly claims the demo provides "real-time position feedback," but the actual implementation only wires the OnDragStart and OnDragEnd lifecycle callbacks which display status transitions ("Dragging…" and "Released") rather than continuous position data. Update the text at line 46 to accurately describe that the demo demonstrates lifecycle event handling (OnDragStart/OnDragEnd) instead of real-time position feedback. Apply the same descriptive correction to the equivalent text at line 145 to maintain consistency across the demo documentation.
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razor`:
- Around line 213-221: The AddEvent method in the code snippet (lines 216-220)
is missing the event-list trimming logic that exists in the live demo at lines
80-87. After adding an event to the _events list, implement the same logic that
trims the collection to prevent unbounded growth by removing older entries when
the list exceeds the maximum size. Ensure the trimming logic from the actual
demo is included in the AddEvent method so that users copying this example will
have complete, production-ready code that prevents memory issues from
accumulating events indefinitely.
In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 576-585: The NeedsPathLengthAttr method and its helper
HasPathLength do not account for PathLength when animation targets are variant
labels (strings) rather than BmotionAnimationTarget objects with Props. The
HasPathLength method only checks t?.Props?.PathLength but variant-driven targets
store their configuration differently. Enhance the HasPathLength method to
detect PathLength in both direct property definitions (when target is a
BmotionAnimationTarget with Props) and when targets are variant labels by
checking the variant definitions or the alternate property path where
variant-based PathLength would be stored. This ensures pathLength="1" is
correctly injected for normalized draw animations regardless of whether targets
use direct properties or variant references.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionConfig.razor`:
- Around line 29-38: The _ctx object is being mutated in place in the
OnParametersSet method, but Blazor's cascading parameter system detects changes
based on object reference equality, not property mutations. Create a new
instance of BmotionConfigContext with the updated values for DefaultTransition,
ReduceMotion, and TransitionSpeed instead of modifying the existing _ctx
reference. This ensures the reference changes and cascading updates properly
propagate to descendant components when these parameter values change at
runtime.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs`:
- Around line 43-58: The _reducedMotionDetected flag is being set to true before
the interop operations complete, which prevents retries on transient failures.
Move the _reducedMotionDetected = true assignment to after the try block
successfully completes (place it after the await
_interop.WatchReducedMotionAsync(_dotnet) call). This ensures that if either the
PrefersReducedMotionAsync or WatchReducedMotionAsync calls fail, the flag
remains false and future invocations can retry the detection instead of exiting
early at the guard condition.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cs`:
- Around line 48-49: The cubic-bezier string building in the EaseCubicBezier
serialization uses direct numeric interpolation which produces culture-dependent
decimal separators (commas instead of periods in certain locales), creating
invalid WAAPI easing values. Wrap each of the four cubic-bezier control point
values (cb[0], cb[1], cb[2], cb[3]) with BmotionCssFormat.Num(...) to ensure the
output uses culture-invariant formatting regardless of the current system
locale.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs`:
- Line 14: The `Transforms` dictionary in `BmotionElementAnimationState.cs` uses
the default case-sensitive string comparer, but
`BmotionTransformComposer.IsTransformProp()` accepts keys case-insensitively.
This mismatch causes keys like "X" to be accepted but never matched during
emission since the code expects canonical lowercase keys like "x". Update the
`Transforms` dictionary initialization to use `StringComparer.OrdinalIgnoreCase`
as the key comparer to ensure case-insensitive lookups match the transform key
detection behavior.
- Around line 594-597: The numeric keyframe conversion logic in the condition
checking if value is an object array with numeric first element assumes all
array elements are convertible to double. This causes unhandled exceptions when
the array contains mixed types like [0, "bad"]. Modify the conversion to safely
attempt converting each element individually, validating that each element can
be converted to a double before processing, and either skip invalid elements or
return false if any element fails conversion. Use a try-catch block or
validation logic (such as attempting to parse each element) around the
Convert.ToDouble call in the Select operation to prevent exceptions from being
thrown during the array-wide conversion.
- Around line 86-90: The foreach loop iterating over `_activeAnims` is
vulnerable to re-entrant mutations because `driver.Tick(timestamp)` can invoke
user callbacks (via `CreateNumericDriver`) that mutate the `_activeAnims`
dictionary, causing a runtime exception. Fix this by iterating over a snapshot
of the dictionary entries instead of the dictionary itself—use `.ToList()` or
similar on `_activeAnims` to create a copy of the entries before the foreach
loop, ensuring mutations during callback execution do not affect the
enumeration.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cs`:
- Around line 26-27: The BmotionInertiaDriver has an issue where
InertiaRestDelta can be assigned a non-positive value, which breaks the decay
motion completion condition and causes the driver to remain active indefinitely.
At the assignment of _restDelta from config.InertiaRestDelta (around line
26-27), clamp the value to a positive minimum threshold using Math.Max to ensure
InertiaRestDelta is always greater than zero. The same clamping logic must also
be applied at the sibling location around lines 57-60 where InertiaRestDelta is
similarly assigned, to maintain consistency across all initialization paths.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs`:
- Around line 25-43: The Times array validation in the constructor only checks
length but does not validate that the times are monotonically increasing or
within valid range, which can cause the Interpolate method (called later at line
111) to produce invalid segment math and propagate NaN or incorrect values.
After the existing Times length validation (the check that compares
config.Times.Length to frames.Length), add additional validation to ensure that
each element in config.Times is greater than or equal to the previous element
(monotonic ascending order) and that all values fall within an acceptable range
(typically 0 to 1 for normalized keyframe times). Throw an ArgumentException
with a descriptive message if the Times array fails either of these checks.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cs`:
- Around line 50-51: Add defensive validation of the Mass value in the
BmotionSpringDriver constructor before the assignment at line 50 where _m is set
to config.Mass. Ensure that _m is always greater than 0 by clamping or
validating the config.Mass value, since any Mass value of 0 or less will cause
division by _m at line 85 to produce non-finite values (Infinity or NaN), which
will cause the rest condition check at line 88 to always fail and trap the
animation indefinitely. Alternatively, add a setter to the Mass property in
BmotionTransitionConfig to enforce that Mass is always greater than 0,
preventing invalid values from being assigned in the first place.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Around line 87-93: The OnScroll method lacks a null check for the info
parameter at the interop boundary, which means dereferencing info.ProgressX,
info.ProgressY, info.ScrollX, and info.ScrollY will throw a
NullReferenceException if info is null. Add a guard clause at the beginning of
the OnScroll method to check if info is null and return early or handle the null
case appropriately before any property assignments or further logic.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs`:
- Around line 96-99: The Transform methods do not validate their function input
parameters and can throw opaque NullReferenceException at runtime if null
arguments are passed. Add null argument checks at the beginning of the
Transform<TOut>(Func<T,TOut> fn) method (around line 96-99) and the other
Transform method in the range 108-116, throwing ArgumentNullException for null
inputs to fail fast with a clear error message instead of allowing the code to
proceed and eventually fail with an unclear NullReferenceException.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor`:
- Line 46: The description text at line 46 incorrectly claims the demo provides
"real-time position feedback," but the actual implementation only wires the
OnDragStart and OnDragEnd lifecycle callbacks which display status transitions
("Dragging…" and "Released") rather than continuous position data. Update the
text at line 46 to accurately describe that the demo demonstrates lifecycle
event handling (OnDragStart/OnDragEnd) instead of real-time position feedback.
Apply the same descriptive correction to the equivalent text at line 145 to
maintain consistency across the demo documentation.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f8223b23-5b4d-4f11-bbac-a19dc7cb2d61
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs (1)
9-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a fresh-cycle reset for stagger child indexing.
RegisterChild()only increments_nextChildIndex, so repeated mount/unmount cycles can produce inflated delays unrelated to current child order. This driftsstaggerChildrenbehavior over time.Suggested direction
public class BmotionVariantContext { private int _nextChildIndex; @@ internal int RegisterChild() => _nextChildIndex++; + internal void ResetChildrenOrder() => _nextChildIndex = 0; @@ public double GetChildDelay(int childIndex) => DelayChildren + childIndex * StaggerChildren; }Then invoke
ResetChildrenOrder()from the parent when starting a new variant cycle (fresh enter cycle), not on ordinary rerenders.🤖 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 `@src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs` around lines 9 - 40, Add a new method called ResetChildrenOrder() to the BmotionVariantContext class that resets the _nextChildIndex counter back to 0 to prevent inflated delays from accumulating across repeated mount/unmount cycles. This method should be invoked from the parent component when starting a fresh variant cycle (entering a new variant state), not during ordinary rerenders, ensuring that child stagger delays remain consistent with the actual current child order rather than drifting based on historical component lifecycle events.src/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cs (1)
46-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClaim settlement atomically before calling engine side effects.
Volatile.Read+ReleaseOnce()is still a check-then-act race:Stop()/Complete()can run_engine.Stop/_engine.Completeafter a concurrent natural settlement has already released ownership, which can interfere with newer animations on the same element IDs.🛠️ Minimal fix
- private void ReleaseOnce() - { - if (System.Threading.Interlocked.Exchange(ref _released, 1) == 0) - _release(); - } + private bool TryClaim() + => System.Threading.Interlocked.Exchange(ref _released, 1) == 0; @@ public void Stop() { - if (System.Threading.Volatile.Read(ref _released) != 0) return; - foreach (var id in _elementIds) - _engine.Stop(id, null); - ReleaseOnce(); + if (!TryClaim()) return; + try + { + foreach (var id in _elementIds) + _engine.Stop(id, null); + } + finally + { + _release(); + } } @@ public void Complete() { - if (System.Threading.Volatile.Read(ref _released) != 0) return; - foreach (var id in _elementIds) - _engine.Complete(id); - ReleaseOnce(); + if (!TryClaim()) return; + try + { + foreach (var id in _elementIds) + _engine.Complete(id); + } + finally + { + _release(); + } } @@ - internal void OnCompletionSettled() => ReleaseOnce(); + internal void OnCompletionSettled() + { + if (TryClaim()) + _release(); + }Also applies to: 72-74
🤖 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 `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cs` around lines 46 - 63, The Stop() and Complete() methods have a check-then-act race condition where the Volatile.Read check and the subsequent _engine.Stop/_engine.Complete calls are not atomic, allowing a concurrent thread to release ownership between the check and the engine side effects. Replace the non-atomic pattern (Volatile.Read check followed by ReleaseOnce call) with an atomic operation like Interlocked.CompareExchange on the _released field that both checks if the object is released and marks it as released in a single atomic step, ensuring the engine side effects only execute if we successfully claim ownership before any concurrent settlement can change the state. Apply this same atomic fix pattern to both the Stop() method and the Complete() method (referenced at line 72-74 in the comment).
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor`:
- Around line 78-84: The RemoveItem method at the anchor location (lines 78-84)
and the similar code block at the sibling location (lines 197-203) in
AnimatePresencePage.razor both set item.Present to false to trigger an exit
animation, but they do not force a render cycle before the delay and removal.
This causes the exit animation and removal to potentially collapse into a single
render, skipping the visible transition. Add a call to StateHasChanged()
immediately after setting item.Present = false at both locations to ensure the
exit animation renders visibly before the Task.Delay completes and the item is
removed from the _items list.
In `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/index.html`:
- Line 26: The dismiss element in the error UI uses an `<a>` tag without an href
attribute, which is not keyboard-focusable and causes accessibility issues.
Replace the `<a class="dismiss">` element with a semantic `<button type="button"
class="dismiss">` to ensure the dismiss functionality remains operable via
keyboard navigation while maintaining the same visual styling and emoji content.
In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 245-253: In the else if branch for `_isExiting && PresenceCtx is
not { IsExiting: true }`, you need to also reset the `_prevInheritedVariant`
field to null (in addition to the existing `_prevAnimate = null;` assignment).
This ensures that when the presence re-enters, both the animation cache and the
inherited variant cache are cleared. Without this, if Animate is null and the
inherited variant label remains unchanged, the check at Line 347 will suppress
the replay and the enter animation will not run after re-entry.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cs`:
- Around line 31-33: The variant comparison in the AreEquivalent method uses
case-sensitive comparison (StringComparison.Ordinal) when comparing a.Variant
and b.Variant, but variant resolution elsewhere in the codebase uses
case-insensitive comparison (StringComparer.OrdinalIgnoreCase). Change the
StringComparison.Ordinal parameter to StringComparison.OrdinalIgnoreCase in the
string.Equals call that compares variants to ensure consistency and prevent
animations from re-triggering due to casing-only differences that resolve to the
same variant.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cs`:
- Around line 22-37: The public properties Momentum, SnapTransition, and
SnapToOrigin defined in BmotionDragOptions are not being serialized into the
event payload, making them ineffective. In
src/Bmotion/Bit.Bmotion/Components/Bmotion.cs, locate the BuildEventFlags method
and add serialization of all three properties (Momentum, SnapTransition,
SnapToOrigin) into the runtime payload. Then in
src/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.js, ensure the JavaScript code reads
these properties from the payload and applies them to the drag behavior
consistently.
- Around line 17-21: The Elastic property in BmotionDragOptions is documented as
a normalized factor with a valid range of 0 to 1, but currently has no
validation to enforce this contract. Add validation to the Elastic property
setter to clamp the incoming value between 0 and 1 (or throw an exception if
validation is preferred), ensuring that only valid elasticity values within the
documented range can be set.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cs`:
- Around line 98-104: The RegisterElement calls on lines 98-100 are not
protected against exceptions that may occur during the task creation on line
103. If the LINQ expression calling AnimateToAwaitAsync throws synchronously
before completion/control tasks are properly established, the registered
elements will leak because no release callback will execute. Wrap the
registration and task creation logic in a try-catch or try-finally block to
ensure that if task creation fails, any already-registered elements are properly
released/cleaned up before the exception propagates. Alternatively, defer the
RegisterElement calls until after confirming the task creation will not throw
synchronously.
In `@src/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.js`:
- Around line 211-218: The tap gesture handlers (onDown and onUp) currently
respond to any pointer event, allowing right-click and middle-click to trigger
tap callbacks. Add a primary button check to the onDown and onUp event handlers
to ignore non-primary mouse buttons. In the onDown handler, check if the pointer
is primary before setting pressing to true and invoking OnPointerDown. In the
onUp handler, check if the pointer is primary before proceeding with the
pressing logic and invoking OnPointerUp. Use the pointer event's isPrimary
property or button property (0 for primary) to filter these events.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs`:
- Around line 9-40: Add a new method called ResetChildrenOrder() to the
BmotionVariantContext class that resets the _nextChildIndex counter back to 0 to
prevent inflated delays from accumulating across repeated mount/unmount cycles.
This method should be invoked from the parent component when starting a fresh
variant cycle (entering a new variant state), not during ordinary rerenders,
ensuring that child stagger delays remain consistent with the actual current
child order rather than drifting based on historical component lifecycle events.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cs`:
- Around line 46-63: The Stop() and Complete() methods have a check-then-act
race condition where the Volatile.Read check and the subsequent
_engine.Stop/_engine.Complete calls are not atomic, allowing a concurrent thread
to release ownership between the check and the engine side effects. Replace the
non-atomic pattern (Volatile.Read check followed by ReleaseOnce call) with an
atomic operation like Interlocked.CompareExchange on the _released field that
both checks if the object is released and marks it as released in a single
atomic step, ensuring the engine side effects only execute if we successfully
claim ownership before any concurrent settlement can change the state. Apply
this same atomic fix pattern to both the Stop() method and the Complete() method
(referenced at line 72-74 in the comment).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: edb4cec6-4b14-42c9-8dfe-d1a59046ec93
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (5)
src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs (1)
42-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMixed-case
Buildtest currently masks composer lookup behavior.At Line 42, constructing
twithStringComparer.OrdinalIgnoreCasemakes mixed-case lookups succeed even ifBmotionTransformComposer.Buildremains case-sensitive internally. That means this test doesn’t actually lock the end-to-end mixed-case contract it describes.Suggested test tightening
- var t = new Dictionary<string, double>(StringComparer.OrdinalIgnoreCase) + var t = new Dictionary<string, double> { ["X"] = 10, ["SCALE"] = 2.0, };🤖 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 `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs` around lines 42 - 50, The test creates the dictionary t with StringComparer.OrdinalIgnoreCase, which allows mixed-case key lookups to succeed regardless of whether BmotionTransformComposer.Build method itself handles case-insensitive lookups. This masks the actual behavior being tested. Remove the StringComparer.OrdinalIgnoreCase argument from the Dictionary constructor so that the dictionary uses the default case-sensitive comparison, and ensure BmotionTransformComposer.Build method is truly handling the mixed-case keys ("X", "SCALE") correctly and not relying on the dictionary's case-insensitive behavior.src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs (1)
90-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse driver identity checks, not only key existence, in re-entrant tick processing.
Line 93 only checks key presence. If a callback replaces the driver for the same key during this frame, the stale snapshot driver can still tick and later remove the new driver at Lines 101–102.
🔧 Safer re-entrant handling
- List<string>? completed = null; + List<(string key, IBmotionAnimationDriver driver)>? completed = null; foreach (var (key, driver) in _activeAnims.ToArray()) { - // The driver may have been removed by a re-entrant callback earlier in this loop. - if (!_activeAnims.ContainsKey(key)) continue; + // The key may still exist but point to a different driver after re-entrant mutation. + if (!_activeAnims.TryGetValue(key, out var current) || !ReferenceEquals(current, driver)) + continue; if (driver.Tick(timestamp)) - (completed ??= new List<string>()).Add(key); + (completed ??= new List<(string, IBmotionAnimationDriver)>()).Add((key, driver)); } if (completed != null) - foreach (var key in completed) + foreach (var (key, driver) in completed) { + if (!_activeAnims.TryGetValue(key, out var current) || !ReferenceEquals(current, driver)) + continue; _activeAnims.Remove(key); NotePropFinished(key, interrupted: false); // natural completion }🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs` around lines 90 - 103, The re-entrant safety check at line 93 only verifies key presence with ContainsKey(key), but does not verify the driver identity. If a re-entrant callback replaces the driver for a given key during loop iteration, the stale driver from the ToArray() snapshot can still tick and later incorrectly remove the new driver at line 101-102. Replace the ContainsKey check with a TryGetValue call that retrieves the current driver for the key and verifies it matches the stale driver from the snapshot before proceeding with the Tick call. This ensures only the actual active driver for each key is processed in this frame.src/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cs (1)
33-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim
Amountbefore parsing.At Line 33, values like
" all "/" 0.5 "can miss intended parsing and fall back unexpectedly. Trim before lowercasing/parsing.Proposed fix
- var amount = Amount?.ToLowerInvariant(); + var amount = Amount?.Trim().ToLowerInvariant();🤖 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 `@src/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cs` around lines 33 - 40, The Amount property can contain leading or trailing whitespace which causes values like " all " or " 0.5 " to fail pattern matching and numeric parsing. Modify the line where amount is assigned (currently `var amount = Amount?.ToLowerInvariant();`) to trim whitespace before converting to lowercase. Add a Trim() call after the null-conditional operator and before ToLowerInvariant() to ensure all leading and trailing whitespace is removed before the switch statement evaluation and numeric parsing logic.src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor (1)
78-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExit animation can be skipped without an intermediate render.
At Line 82 and Line 201,
Presentis flipped tofalsebut no render is forced before the delay/removal path, so the exit transition may not visibly run.Suggested patch
async Task RemoveItem(int id) { var item = _items.Find(i => i.Id == id); if (item is null || !item.Present) return; item.Present = false; // trigger the exit animation + await InvokeAsync(StateHasChanged); // render exit state before removal delay await Task.Delay(400); // let it finish before dropping the item from the list _items.Remove(item); }Apply the same fix in the
_listCodesnippet block so copied sample code matches runtime behavior.Also applies to: 197-203
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor` around lines 78 - 84, The RemoveItem method in AnimatePresencePage.razor (lines 78-84) sets item.Present to false but does not force an intermediate render before the delay, which prevents the exit animation from running visibly. After setting item.Present = false and before the Task.Delay(400) call, add a call to StateHasChanged() to force the UI to process the state change and trigger the CSS transition. Apply the same fix at lines 197-203 where this pattern is repeated, and ensure the _listCode snippet block also includes this StateHasChanged() call so the copied sample code matches the runtime behavior.src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs (1)
9-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStagger indices can drift upward across remount cycles.
RegisterChild()only increments and there is no reset/unregister flow, so remounted children can receive progressively larger delays throughGetChildDelay(...)over time.🤖 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 `@src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs` around lines 9 - 40, The RegisterChild() method increments _nextChildIndex without any corresponding decrement mechanism, causing child indices to grow unbounded across remount cycles and producing progressively larger stagger delays via GetChildDelay(). Implement an unregister mechanism (e.g., add an UnregisterChild() method) that decrements the counter when children unmount, or alternatively track the set of currently-registered child indices and only use the maximum active index rather than a monotonically-increasing counter. Ensure that SeedChildIndex(int value) and any child registration/unregistration logic work together to maintain correct stagger indices across context remounting without permanent drift.
🧹 Nitpick comments (2)
src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs (1)
45-51: 💤 Low valueConcurrent modification risk when iterating subscribers.
_subscribers.ToArray()creates a snapshot for iteration, which is good. However, if a subscriber callback synchronously callsSubscribeor disposes its subscription handle (removing from_subscribers),List<T>.Removecan throw or corrupt state during iteration on another call stack. Since this runs on a single WebAssembly UI thread (as per README), this is unlikely to cause immediate issues, but recursive/reentrant notification scenarios could still hit this.Consider using a copy-on-write pattern or deferring removals if reentrancy becomes a concern.
Also applies to: 64-70
🤖 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 `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs` around lines 45 - 51, The code iterates over a snapshot of _subscribers using ToArray(), which is safe for iteration, but a subscriber callback can synchronously call Subscribe or Dispose (removing from _subscribers) on the same instance, causing potential concurrent modification issues if reentrancy occurs. Implement a copy-on-write pattern where any modification to _subscribers creates a new list instance rather than modifying in-place, ensuring that ongoing iterations use the old snapshot while new subscriptions/removals operate on the new list. Apply this pattern to all places where subscribers are added or removed (the Subscribe method and the unsubscription/Dispose mechanism).src/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.css (1)
107-118: ⚡ Quick winAdd explicit
:focus-visiblestyling for interactive controls.Custom button controls currently define hover states but no explicit keyboard-focus treatment. Defining
:focus-visibleensures consistent, high-contrast focus indication across browsers/themes.Suggested patch
.btn-bm { @@ } .btn-bm:hover { opacity: .85; } +.btn-bm:focus-visible, +.code-toggle:focus-visible, +.code-copy:focus-visible { + outline: 2px solid var(--bm-primary); + outline-offset: 2px; +}Also applies to: 145-186
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.css` around lines 107 - 118, The `.btn-bm` button class currently lacks keyboard-focus styling, which compromises accessibility for keyboard users. Add a `:focus-visible` pseudo-class rule for the `.btn-bm` selector that provides high-contrast focus indication (e.g., similar opacity treatment or an outline style) to ensure keyboard focus is clearly visible. Apply the same `:focus-visible` treatment to all other interactive controls in the file that currently define hover states but lack explicit focus-visible styling, ensuring consistent keyboard navigation feedback across all custom interactive elements.
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/App.razor`:
- Line 4: The FocusOnNavigate component in App.razor targets only h1 elements
with its Selector attribute, but some demo pages use h2 as their top heading
instead. Update the Selector attribute on the FocusOnNavigate component to match
both h1 and h2 elements using a CSS selector that accepts multiple element
types, so focus will correctly move to the top of the page after navigation
regardless of whether pages use h1 or h2 headings.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razor`:
- Around line 60-65: The callback passed to Scroll.ObserveAsync at lines 60-65
must return a Task but the current block lambda does not, causing a compile-time
failure. Convert the callback to an async lambda that returns a Task by making
it async and awaiting the InvokeAsync call. Apply the same fix to the mirrored
code at lines 103-108 to ensure consistency.
In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 445-446: The guards checking if While* properties are non-null
(such as `if (WhileHover != null)`) prevent gesture layer deactivation when
those properties are removed while a gesture layer is active, leaving stale
styles in place. Remove these null checks from all deactivation calls in the
Bmotion class so that Engine.DeactivateGestureLayerAsync is called
unconditionally whenever cleanup occurs. This applies to all instances: the
WhileHover deactivation around lines 445-446, WhilePress around 463-464,
WhileFocus around 472-473, WhileInView around 490-491, WhileTap around 521-522,
and WhileView around 581-582. Ensure each deactivation statement executes
regardless of the current state of its corresponding While* property.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs`:
- Around line 496-512: The StopRafLoopGatedAsync method awaits
_loopStartGate.WaitAsync() without protection against the semaphore being
disposed. Since this method can be called as a fire-and-forget task, if
DisposeAsync disposes the semaphore before the pending task executes, it will
result in an unobserved ObjectDisposedException. Wrap the
_loopStartGate.WaitAsync() call in a try-catch block that catches
ObjectDisposedException and returns early, allowing graceful handling of the
case where disposal has already occurred while the task was pending.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs`:
- Around line 88-92: The if statement in BmotionNumericKeyframesDriver.cs that
handles both Mirror and Reverse repeat types currently treats them identically
by reversing frames and times on every cycle. You need to separate the logic:
for Mirror mode, keep the current behavior of reversing each cycle to alternate
playback direction, but for Reverse mode, reverse the frames and times only once
(not on every cycle) so the animation plays in reverse order repeatedly without
toggling direction. Modify the condition at line 88 to check specifically for
Mirror mode in the reversing block, and add separate handling for Reverse mode
that reverses only initially.
- Around line 38-41: The validation for config.Times entries at lines 38 and 40
uses relational operators that do not catch non-finite values like double.NaN
(since all comparisons with NaN return false). Add an explicit double.IsFinite()
check for each config.Times[i] value before performing the range [0, 1] check
and the monotonically ascending order check to ensure that NaN and infinity
values are rejected and prevent destabilization of interpolation math.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cs`:
- Around line 59-60: The _restSpeed and _restDelta field assignments in
BmotionSpringDriver.cs accept non-positive values from config.RestSpeed and
config.RestDelta multiplied by restScale, which prevents the completion gate
check from ever being satisfied, leaving the spring active indefinitely. Apply a
clamp operation (such as Math.Max) to ensure both _restSpeed and _restDelta are
assigned positive values only, guaranteeing that the termination condition can
be reached.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs`:
- Around line 190-197: The `PathSpacing` and `PathOffset` properties are not
being clamped to their documented 0–1 range before being used in CSS output. In
the file src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs at lines
190-197 (anchor), clamp both the `spacing` variable (derived from PathSpacing)
and the `offset` variable (derived from PathOffset) to the 0–1 range using the
same Math.Max(0, Math.Min(1, value)) pattern applied to the `clamped` variable.
Apply the same clamping to the corresponding `spacing` and `offset` variables at
lines 259-266 (sibling).
In `@src/Bmotion/README.md`:
- Around line 303-317: The sample component code injects a
BmotionAnimationController but does not properly dispose of it, which can leave
the bound element registered in the engine after component teardown. Implement
the IAsyncDisposable interface in the component code block and add a
DisposeAsync method that properly disposes the BmotionAnimationController. This
should be done alongside the existing OnAfterRender method to ensure the
controller's resources are released when the component is destroyed.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor`:
- Around line 78-84: The RemoveItem method in AnimatePresencePage.razor (lines
78-84) sets item.Present to false but does not force an intermediate render
before the delay, which prevents the exit animation from running visibly. After
setting item.Present = false and before the Task.Delay(400) call, add a call to
StateHasChanged() to force the UI to process the state change and trigger the
CSS transition. Apply the same fix at lines 197-203 where this pattern is
repeated, and ensure the _listCode snippet block also includes this
StateHasChanged() call so the copied sample code matches the runtime behavior.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs`:
- Around line 9-40: The RegisterChild() method increments _nextChildIndex
without any corresponding decrement mechanism, causing child indices to grow
unbounded across remount cycles and producing progressively larger stagger
delays via GetChildDelay(). Implement an unregister mechanism (e.g., add an
UnregisterChild() method) that decrements the counter when children unmount, or
alternatively track the set of currently-registered child indices and only use
the maximum active index rather than a monotonically-increasing counter. Ensure
that SeedChildIndex(int value) and any child registration/unregistration logic
work together to maintain correct stagger indices across context remounting
without permanent drift.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs`:
- Around line 90-103: The re-entrant safety check at line 93 only verifies key
presence with ContainsKey(key), but does not verify the driver identity. If a
re-entrant callback replaces the driver for a given key during loop iteration,
the stale driver from the ToArray() snapshot can still tick and later
incorrectly remove the new driver at line 101-102. Replace the ContainsKey check
with a TryGetValue call that retrieves the current driver for the key and
verifies it matches the stale driver from the snapshot before proceeding with
the Tick call. This ensures only the actual active driver for each key is
processed in this frame.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cs`:
- Around line 33-40: The Amount property can contain leading or trailing
whitespace which causes values like " all " or " 0.5 " to fail pattern matching
and numeric parsing. Modify the line where amount is assigned (currently `var
amount = Amount?.ToLowerInvariant();`) to trim whitespace before converting to
lowercase. Add a Trim() call after the null-conditional operator and before
ToLowerInvariant() to ensure all leading and trailing whitespace is removed
before the switch statement evaluation and numeric parsing logic.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs`:
- Around line 42-50: The test creates the dictionary t with
StringComparer.OrdinalIgnoreCase, which allows mixed-case key lookups to succeed
regardless of whether BmotionTransformComposer.Build method itself handles
case-insensitive lookups. This masks the actual behavior being tested. Remove
the StringComparer.OrdinalIgnoreCase argument from the Dictionary constructor so
that the dictionary uses the default case-sensitive comparison, and ensure
BmotionTransformComposer.Build method is truly handling the mixed-case keys
("X", "SCALE") correctly and not relying on the dictionary's case-insensitive
behavior.
---
Nitpick comments:
In `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.css`:
- Around line 107-118: The `.btn-bm` button class currently lacks keyboard-focus
styling, which compromises accessibility for keyboard users. Add a
`:focus-visible` pseudo-class rule for the `.btn-bm` selector that provides
high-contrast focus indication (e.g., similar opacity treatment or an outline
style) to ensure keyboard focus is clearly visible. Apply the same
`:focus-visible` treatment to all other interactive controls in the file that
currently define hover states but lack explicit focus-visible styling, ensuring
consistent keyboard navigation feedback across all custom interactive elements.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs`:
- Around line 45-51: The code iterates over a snapshot of _subscribers using
ToArray(), which is safe for iteration, but a subscriber callback can
synchronously call Subscribe or Dispose (removing from _subscribers) on the same
instance, causing potential concurrent modification issues if reentrancy occurs.
Implement a copy-on-write pattern where any modification to _subscribers creates
a new list instance rather than modifying in-place, ensuring that ongoing
iterations use the old snapshot while new subscriptions/removals operate on the
new list. Apply this pattern to all places where subscribers are added or
removed (the Subscribe method and the unsubscription/Dispose mechanism).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5112dd31-ed53-46aa-8fd6-de3495540b92
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razor (1)
60-65:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReturn the callback
TaskfromObserveAsynclambda.At Line 60 and Line 103, the callback body does not return a
Task. This was previously flagged and still appears unresolved; keep runtime code and snippet aligned by returningInvokeAsync(StateHasChanged).🐛 Proposed fix
- await Scroll.ObserveAsync(null, info => + await Scroll.ObserveAsync(null, info => { _progressX = info.ProgressX; _progressY = info.ProgressY; - InvokeAsync(StateHasChanged); + return InvokeAsync(StateHasChanged); });- await Scroll.ObserveAsync(null, info => + await Scroll.ObserveAsync(null, info => { _progressX = info.ProgressX; _progressY = info.ProgressY; - InvokeAsync(StateHasChanged); + return InvokeAsync(StateHasChanged); });Also applies to: 103-108
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razor` around lines 60 - 65, The callback lambda passed to Scroll.ObserveAsync must return a Task to match the expected delegate signature. Currently, the callback updates _progressX and _progressY and calls InvokeAsync(StateHasChanged) but does not return the result. Change the callback to return the Task result from InvokeAsync(StateHasChanged) so the callback properly completes the async operation. This fix needs to be applied in two locations where this pattern occurs in the file.
🧹 Nitpick comments (3)
src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs (1)
22-30: ⚡ Quick winAdd explicit null guards for constructor dependencies.
Line 26 dereferences
configbefore any null check, and_applycan also be null. Fail fast withArgumentNullExceptionto keep error behavior deterministic.Proposed patch
public BmotionNumericKeyframesDriver(double[] frames, BmotionTransitionConfig config, Action<double> apply) { + ArgumentNullException.ThrowIfNull(config); + ArgumentNullException.ThrowIfNull(apply); + if (frames is null || frames.Length < 2) throw new ArgumentException("Keyframe animations require at least 2 frames.", nameof(frames));🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs` around lines 22 - 30, The BmotionNumericKeyframesDriver constructor dereferences the config parameter on line 26 without first checking if it is null, and the apply parameter is never validated for null. Add explicit null guards at the start of the constructor before any parameter usage. Check both the config parameter and the apply parameter, throwing ArgumentNullException for each if they are null, to ensure deterministic error behavior and prevent null reference exceptions from occurring deeper in the code.src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cs (1)
4-146: ⚡ Quick winAdd regression tests for constructor validation paths.
The suite exercises runtime behavior well, but it does not lock in the new input-guard contracts (invalid
Timesshape/range/non-finite entries and non-finite timing values). Adding a fewAssert.ThrowsException<ArgumentException>tests will prevent these safeguards from regressing silently.🤖 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 `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cs` around lines 4 - 146, The NumericKeyframesDriverTests class exercises runtime animation behavior but lacks tests for input validation. Add new test methods to the NumericKeyframesDriverTests class that use Assert.ThrowsException<ArgumentException> to verify that the BmotionNumericKeyframesDriver constructor properly validates its inputs. Create separate tests for invalid Times array shapes (length mismatch with values), invalid Times ranges (values outside 0.0-1.0), non-finite entries in the Times array (NaN or Infinity), and non-finite Duration values in BmotionTransitionConfig. These tests will ensure the input-guard contracts are enforced and prevent future regressions of these safeguards.src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs (1)
228-263: ⚡ Quick winAdd regression tests for constructor safety guards.
The suite now exercises behavior well, but it doesn’t lock the new non-positive
Massand non-positiveRestSpeed/RestDeltasafeguards that prevent non-terminating springs. Adding these two tests will protect against regressions in termination guarantees.Suggested test additions
@@ public void Tick_InfiniteRepeat_NeverCompletes() { @@ Assert.IsFalse(done, "Infinite-repeat spring should never report completion"); } + [TestMethod] + public void Tick_NonPositiveMass_StillCompletes() + { + var driver = new BmotionSpringDriver(0, 100, new BmotionTransitionConfig + { + Stiffness = 100, + Damping = 20, + Mass = 0 + }, _ => { }); + + int ticks = RunUntilComplete(driver, 10_000); + Assert.IsTrue(ticks > 0, "Spring should settle with fallback mass when Mass <= 0."); + } + + [TestMethod] + public void Tick_NonPositiveRestThresholds_StillCompletes() + { + var driver = new BmotionSpringDriver(0, 100, new BmotionTransitionConfig + { + Stiffness = 100, + Damping = 20, + Mass = 1, + RestSpeed = 0, + RestDelta = 0 + }, _ => { }); + + int ticks = RunUntilComplete(driver, 10_000); + Assert.IsTrue(ticks > 0, "Spring should settle with clamped rest thresholds."); + } + private static int RunUntilComplete(BmotionSpringDriver driver, double maxMs = 30_000)🤖 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 `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs` around lines 228 - 263, Add two new regression test methods to the SpringDriverTests class to exercise the constructor safety guards that prevent non-terminating springs. The first test should verify that the BmotionSpringDriver constructor properly validates non-positive Mass values and prevents them from being used (either by throwing an exception or applying a minimum threshold). The second test should verify that the constructor properly validates non-positive RestSpeed and RestDelta values with similar safeguards. Both tests should ensure these validation guards are enforced and cannot be bypassed, protecting the termination guarantees of the spring driver.
🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs`:
- Around line 519-521: The `_loopStartGate.Release()` call in the finally block
is vulnerable to an `ObjectDisposedException` that can occur when the semaphore
is disposed (at line 559) while `StopRafLoopGatedAsync` is still unwinding.
Guard the `Release()` call against this disposal race by wrapping it in a
try-catch block that catches and safely handles `ObjectDisposedException` to
prevent the exception from being thrown on the unobserved task path.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs`:
- Around line 366-370: The CompleteAll() method iterates directly over
_activeAnims.Values while calling driver.Complete(), which can invoke user
callbacks that mutate _activeAnims during iteration, causing an exception.
Create a copy of the active animation drivers before iterating (for example, by
converting _activeAnims.Values to a list or array) so that any mutations to
_activeAnims by callbacks during the driver.Complete() calls do not affect the
enumeration, then iterate over this copy to call Complete() on each driver
before clearing _activeAnims.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cs`:
- Around line 29-33: The Create method in BmotionMotionVariants does not
validate its entries parameter before using it in the foreach loop, causing a
NullReferenceException if null is passed. Add a null guard check at the
beginning of the Create method that throws ArgumentNullException with an
appropriate message when entries is null, ensuring the error is clear and
descriptive rather than a generic NullReferenceException from the foreach
statement.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs`:
- Around line 196-203: The CloneProperties method does not guard against null
entries in the source dictionary. In the foreach loop where copy[kv.Key] =
kv.Value.Clone() is executed, if kv.Value is null, this will throw a
NullReferenceException. Add a null check to handle null dictionary values—either
skip null entries by assigning null directly (copy[kv.Key] = null) or by
checking kv.Value is not null before calling Clone() on it.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razor`:
- Around line 60-65: The callback lambda passed to Scroll.ObserveAsync must
return a Task to match the expected delegate signature. Currently, the callback
updates _progressX and _progressY and calls InvokeAsync(StateHasChanged) but
does not return the result. Change the callback to return the Task result from
InvokeAsync(StateHasChanged) so the callback properly completes the async
operation. This fix needs to be applied in two locations where this pattern
occurs in the file.
---
Nitpick comments:
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs`:
- Around line 22-30: The BmotionNumericKeyframesDriver constructor dereferences
the config parameter on line 26 without first checking if it is null, and the
apply parameter is never validated for null. Add explicit null guards at the
start of the constructor before any parameter usage. Check both the config
parameter and the apply parameter, throwing ArgumentNullException for each if
they are null, to ensure deterministic error behavior and prevent null reference
exceptions from occurring deeper in the code.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cs`:
- Around line 4-146: The NumericKeyframesDriverTests class exercises runtime
animation behavior but lacks tests for input validation. Add new test methods to
the NumericKeyframesDriverTests class that use
Assert.ThrowsException<ArgumentException> to verify that the
BmotionNumericKeyframesDriver constructor properly validates its inputs. Create
separate tests for invalid Times array shapes (length mismatch with values),
invalid Times ranges (values outside 0.0-1.0), non-finite entries in the Times
array (NaN or Infinity), and non-finite Duration values in
BmotionTransitionConfig. These tests will ensure the input-guard contracts are
enforced and prevent future regressions of these safeguards.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs`:
- Around line 228-263: Add two new regression test methods to the
SpringDriverTests class to exercise the constructor safety guards that prevent
non-terminating springs. The first test should verify that the
BmotionSpringDriver constructor properly validates non-positive Mass values and
prevents them from being used (either by throwing an exception or applying a
minimum threshold). The second test should verify that the constructor properly
validates non-positive RestSpeed and RestDelta values with similar safeguards.
Both tests should ensure these validation guards are enforced and cannot be
bypassed, protecting the termination guarantees of the spring driver.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b9d6152-a770-403f-bf97-f24bc1a527e9
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 706-720: In the ReconcileViewportAsync method, when sig is null
and UnobserveViewportAsync is called on line 711, the active inview styles may
remain stuck on the element. To fix this, clear any active inview styles in
addition to unobserving the viewport. Call an appropriate method (likely on the
Interop class or through some style-clearing mechanism) to remove the inview
layer styles before or after the UnobserveViewportAsync call to ensure styles
don't persist after viewport observation is disabled.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cs`:
- Around line 17-31: The Unregister method removes children from _children
without checking if this triggers the exit completion condition. During an exit
cycle, when a child is unregistered after it has already completed (exists in
_completedChildren), the AllExitsComplete event may never fire if all remaining
children have completed. After removing the child from _children in the
Unregister method, add a check to see if _completedChildren.Count is now greater
than or equal to _children.Count, and if so, invoke AllExitsComplete to properly
signal completion even when children unregister during an active exit cycle.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cs`:
- Line 77: The Complete() method at line 77 always applies the _to value, but
for tweens with Mirror or Reverse modes and odd repeat counts, the natural
terminal state is the original _from value, not _to. This creates inconsistency
between forced completion and natural completion behavior as evidenced by the
mirrored completion expectations in the test file. Modify the Complete() method
to calculate and apply the correct terminal value by checking the current tween
mode (Mirror/Reverse) and repeat count state, then apply either _from or _to
accordingly to match what the tween would naturally reach at the end of its
animation cycle.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs`:
- Around line 28-32: The validation in BmotionNumericKeyframesDriver checks only
that Duration, Delay, and RepeatDelay are finite values, but does not reject
negative Duration values, which causes the animation to never complete because
the progress value t never reaches 1. Extend the condition in the if statement
(currently checking double.IsFinite for all three properties) to also verify
that config.Duration is greater than or equal to zero, ensuring negative
durations are rejected in the same ArgumentException.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cs`:
- Line 21: The BmotionAnimationController constructor currently accepts a
BmotionAnimationEngine engine parameter without validating it, allowing null
values to be assigned to the _engine field. This causes failures to occur later
when the engine is actually used rather than at construction time. Add a null
guard at the start of the constructor that throws an ArgumentNullException if
the engine parameter is null, ensuring the dependency is validated immediately
upon object creation.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f1e61bc-5e2d-4b98-8fce-8d21048b743f
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs (1)
200-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve dictionary comparer and reject null property overrides in
CloneProperties.Line 200 recreates
Propertieswith the default comparer, which can change lookup semantics after cloning. Line 204 also preserves null values via!, pushing a broken state downstream instead of failing fast.Proposed fix
private static Dictionary<string, BmotionTransitionConfig>? CloneProperties( Dictionary<string, BmotionTransitionConfig>? source) { if (source is null) return null; - var copy = new Dictionary<string, BmotionTransitionConfig>(source.Count); + var copy = new Dictionary<string, BmotionTransitionConfig>(source.Count, source.Comparer); foreach (var kv in source) - // A null per-property override has nothing to deep-copy; preserve it as-is rather than - // dereferencing it for Clone() (which would throw a NullReferenceException). - copy[kv.Key] = kv.Value?.Clone()!; + copy[kv.Key] = kv.Value?.Clone() + ?? throw new ArgumentException( + "Properties cannot contain null transition configs.", + nameof(source)); return copy; }🤖 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 `@src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs` around lines 200 - 205, In the CloneProperties method, when creating the copy dictionary at the beginning, pass the source dictionary's Comparer to the Dictionary constructor to preserve the original lookup semantics. Additionally, at the line where kv.Value is being cloned, remove the null-forgiving operator and instead explicitly validate that kv.Value is not null, rejecting null property overrides by throwing an appropriate exception rather than allowing nulls to propagate downstream with the ! operator.
🧹 Nitpick comments (3)
src/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cs (1)
155-159: 💤 Low valueConsider stopping the rAF loop before disposing the module.
If the RAF loop is running when
DisposeAsyncis called, disposing the module while callbacks are still scheduled could leave the JS side in an inconsistent state. The JS_tickfunction will gracefully exit when_enginesis empty, but explicitly stopping ensures cleaner teardown.Suggested enhancement
public async ValueTask DisposeAsync() { - if (_moduleTask.IsValueCreated) + if (_moduleTask.IsValueCreated) + { + await StopRafLoopAsync<object>(null); await (await Module()).DisposeAsync(); + } }🤖 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 `@src/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cs` around lines 155 - 159, The DisposeAsync method disposes the module without first stopping the RAF loop, which could leave the JS side in an inconsistent state if callbacks are still scheduled. Modify the DisposeAsync method to explicitly stop the RAF loop before disposing the module by calling the appropriate stop method on the Module object before the DisposeAsync call, ensuring cleaner teardown and preventing orphaned scheduled callbacks.src/Bmotion/Bit.Bmotion.Demos/wwwroot/index.html (1)
26-26: 💤 Low valueConsider adding an explicit label for the dismiss button.
While the dismiss button correctly uses
<button type="button">for keyboard accessibility, screen reader users would benefit from an explicitaria-label="Dismiss error message"attribute since the emoji🗙may not convey clear semantics.♻️ Optional enhancement
- <button type="button" class="dismiss">🗙</button> + <button type="button" class="dismiss" aria-label="Dismiss error message">🗙</button>🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/index.html` at line 26, The dismiss button with class "dismiss" currently only uses an emoji (🗙) as its content, which may not be properly conveyed to screen reader users. Add an aria-label attribute to the button element with descriptive text such as "Dismiss error message" to provide explicit accessibility information for assistive technologies while maintaining the emoji as visual content.src/Bmotion/Bit.Bmotion/BitBmotion.cs (1)
28-55: Services are properly designed for transient disposal, but ensureBmotionAnimationControllerexamples follow the same pattern asBmotionScrollTracker.Both services correctly implement disposal interfaces (
IAsyncDisposableforBmotionScrollTracker,IDisposableforBmotionAnimationController) with proper cleanup logic:
BmotionScrollTrackerunobserves scroll subscriptions, clears state, and disposes itsDotNetObjectReferenceBmotionAnimationControllerunregisters its bound element from the engineBoth services have explicit XML documentation stating consuming components must dispose them explicitly. The demo component
ScrollAnimations.razorcorrectly implements this pattern (line 3:@implements IAsyncDisposable, line 69:await Scroll.DisposeAsync()), providing a working reference for developers.However, the more important risk is that no demo or example was found showing the disposal pattern for
BmotionAnimationController. To match the clarity and completeness ofBmotionScrollTracker's guidance, consider creating or documenting an example component that demonstrates explicitDispose()call forBmotionAnimationController.🤖 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 `@src/Bmotion/Bit.Bmotion/BitBmotion.cs` around lines 28 - 55, The AddBitBmotionServices method correctly registers BmotionAnimationController as transient, and the service properly implements IDisposable with cleanup logic. However, unlike BmotionScrollTracker which has a documented example in ScrollAnimations.razor showing explicit disposal, BmotionAnimationController lacks a corresponding demo component. Create or update an example component that demonstrates the proper disposal pattern for BmotionAnimationController by implementing IDisposable and calling Dispose() on the service instance, mirroring the pattern shown in ScrollAnimations.razor (which implements IAsyncDisposable and calls DisposeAsync() on BmotionScrollTracker). This will provide developers with clear guidance on how to properly manage the lifecycle of BmotionAnimationController.
🤖 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 `@src/Bmotion/Bit.Bmotion.Demos/Pages/Home.razor`:
- Line 54: The feature claim "Zero JS deps" on line 54 of the Home.razor file is
misleading because the library includes a JavaScript interop bridge file
(bit-bmotion.js). Replace the "Zero JS deps" text with a more accurate
description that reflects the actual dependency structure, such as "Minimal JS"
or "Lightweight JS bridge" to properly set user expectations about the runtime
dependencies.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cs`:
- Around line 39-43: The validation logic for config.Times entries is missing a
check to ensure values are finite (not NaN or Infinity). Since NaN comparisons
always return false, invalid NaN values would pass the existing range and
monotonically ascending checks, causing issues in interpolation math. Add a
double.IsFinite check for each config.Times[i] value before the existing range
[0, 1] and order validation checks in the loop that processes Times entries.
- Around line 102-108: The BmotionColorKeyframesDriver currently treats Mirror
and Reverse modes identically by reversing frames and channels every iteration,
but it should distinguish them like BmotionNumericKeyframesDriver does. Add a
private boolean field _reversed (initialized to false) to the
BmotionColorKeyframesDriver class, then modify the reversal logic in the
condition that checks _repeatType so that Mirror mode always reverses
frames/channels/times on each iteration, while Reverse mode only reverses once
(when _reversed is false) and then sets _reversed to true to prevent further
reversals on subsequent iterations. This ensures Reverse mode plays consistently
backwards after the initial reversal, while Mirror mode toggles direction each
cycle.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cs`:
- Around line 25-40: The BmotionColorTweenDriver constructor is missing finite
timing validation that exists in BmotionColorKeyframesDriver and
BmotionNumericKeyframesDriver. Add validation checks in the constructor to
ensure that config.Duration, config.Delay, and config.RepeatDelay are all finite
values (not NaN or Infinity) before they are multiplied by 1000 and stored in
_durationMs, _delayMs, and _repeatDelayMs. If any of these values are not
finite, throw an appropriate exception with a descriptive error message
explaining which timing value is invalid.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cs`:
- Around line 46-47: The Margin property is being serialized as-is in the
BmotionViewportOptions class dictionary without validation, which could result
in invalid rootMargin payload if Margin is null or whitespace. Before assigning
Margin to the dictionary key at line 46, normalize it by checking if it is null
or empty/whitespace and provide a safe default value (such as an empty string or
null depending on the JavaScript interop requirements) instead of passing the
raw Margin value.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs`:
- Around line 200-205: In the CloneProperties method, when creating the copy
dictionary at the beginning, pass the source dictionary's Comparer to the
Dictionary constructor to preserve the original lookup semantics. Additionally,
at the line where kv.Value is being cloned, remove the null-forgiving operator
and instead explicitly validate that kv.Value is not null, rejecting null
property overrides by throwing an appropriate exception rather than allowing
nulls to propagate downstream with the ! operator.
---
Nitpick comments:
In `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/index.html`:
- Line 26: The dismiss button with class "dismiss" currently only uses an emoji
(🗙) as its content, which may not be properly conveyed to screen reader users.
Add an aria-label attribute to the button element with descriptive text such as
"Dismiss error message" to provide explicit accessibility information for
assistive technologies while maintaining the emoji as visual content.
In `@src/Bmotion/Bit.Bmotion/BitBmotion.cs`:
- Around line 28-55: The AddBitBmotionServices method correctly registers
BmotionAnimationController as transient, and the service properly implements
IDisposable with cleanup logic. However, unlike BmotionScrollTracker which has a
documented example in ScrollAnimations.razor showing explicit disposal,
BmotionAnimationController lacks a corresponding demo component. Create or
update an example component that demonstrates the proper disposal pattern for
BmotionAnimationController by implementing IDisposable and calling Dispose() on
the service instance, mirroring the pattern shown in ScrollAnimations.razor
(which implements IAsyncDisposable and calls DisposeAsync() on
BmotionScrollTracker). This will provide developers with clear guidance on how
to properly manage the lifecycle of BmotionAnimationController.
In `@src/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cs`:
- Around line 155-159: The DisposeAsync method disposes the module without first
stopping the RAF loop, which could leave the JS side in an inconsistent state if
callbacks are still scheduled. Modify the DisposeAsync method to explicitly stop
the RAF loop before disposing the module by calling the appropriate stop method
on the Module object before the DisposeAsync call, ensuring cleaner teardown and
preventing orphaned scheduled callbacks.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31ed4475-6305-4ba3-b976-22972a467af8
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (1)
src/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cs (1)
17-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle “last child unregistered” as an exit-complete condition.
On Line 24, requiring
_children.Count > 0skips completion when the final child unregisters during an active exit, which can leaveBmotionAnimatePresencestuck in exiting mode.Suggested fix
internal void Unregister(Bmotion child) { - _children.Remove(child); + if (!_children.Remove(child)) return; _completedChildren.Remove(child); // A child disposed mid-exit removes itself here. If every remaining child has already // completed, NotifyExitComplete's count check won't run again, so re-evaluate now to // avoid AllExitsComplete never firing. - if (IsExiting && _children.Count > 0 && _completedChildren.Count >= _children.Count) + if (IsExiting && (_children.Count == 0 || _completedChildren.Count >= _children.Count)) AllExitsComplete?.Invoke(); }🤖 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 `@src/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cs` around lines 17 - 25, In the Unregister method, the condition on line 24 requires _children.Count to be greater than 0, which prevents the AllExitsComplete event from firing when the final child unregisters during an active exit. Modify the condition to also handle the case where _children.Count equals 0, as this indicates all children have been unregistered and the exit is complete. The check should evaluate to true either when there are remaining children that have all completed, or when there are no remaining children at all during an active exit.
🤖 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 `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 334-346: When the Animate property transitions from a non-null
value to null, the first if block (checking AreEquivalent between _prevAnimate
and Animate) executes and prevents the subsequent else if block from running,
which means inherited variant animation is deferred to a later rerender. Fix
this by removing the "else" keyword before the second if statement at line 345
(the one checking if Animate == null and variants exist), so that both the
animate-change handling and the variant fallback logic can execute in the same
update cycle when Animate becomes null.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs`:
- Around line 60-89: The EnsureReducedMotionDetectedAsync method only guards
against re-running detection after completion with the _reducedMotionDetected
check, but concurrent callers can both enter the method before the first caller
sets this flag, resulting in duplicate probe and watch setup. Add a shared
in-flight task field (such as a Task or TaskCompletionSource) or a SemaphoreSlim
to gate the detection and subscription logic so that only one caller executes
the probe and watch setup while others await the completion of that single
attempt. Check and set this synchronization primitive before the try block that
calls PrefersReducedMotionAsync, ensuring all concurrent callers serialize
through a single detection attempt rather than racing.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cs`:
- Around line 91-92: The segment progress clamping on line 92 in the
BmotionColorKeyframesDriver.cs file only clamps the upper bound using
Math.Min(segT, 1.0), allowing negative segT values to pass through to the easing
function, which can cause backward extrapolation. Replace the Math.Min call with
a proper range clamp that uses both Math.Max and Math.Min to constrain segT to
the [0, 1] range before passing it to the easing function via _eases[seg].
- Around line 27-31: The validation in the BmotionColorKeyframesDriver
constructor currently only checks if Duration, Delay, and RepeatDelay are finite
values, but this allows negative durations which prevent animation completion by
keeping t below 1.0. Enhance the validation condition to also check that
Duration, Delay, and RepeatDelay are greater than or equal to zero, rejecting
any negative values alongside the existing finite value check.
- Line 133: The Complete() method in BmotionColorKeyframesDriver is hardcoding
the application of the last frame via _frames[^1], but this is incorrect for
mirrored or reversed finite animations that naturally terminate on the first
frame. Modify the Complete() method to determine the correct natural terminal
frame based on the animation configuration (considering factors like whether the
run is mirrored, reversed, or finite) and apply that frame instead of always
applying _frames[^1]. For animations that end naturally on the first frame,
apply _frames[0], otherwise apply the appropriate terminal frame based on the
animation state.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cs`:
- Around line 70-74: The `Reverse` mode is currently treated identically to
`Mirror` mode in the conditional check at lines 70-74, causing both to swap
values on every pass. However, `Reverse` should only reverse direction once,
matching the behavior used in keyframe drivers. Separate the logic so that only
`BmotionRepeatType.Mirror` performs the swap operation within the conditional,
while `BmotionRepeatType.Reverse` uses a different approach that reverses
direction once rather than repeatedly. Additionally, the parity formula at lines
89-92 must be updated to correctly compute terminal results for reverse repeats
based on the corrected reverse behavior, not the mirrored swap behavior.
- Around line 27-31: The validation in the BmotionColorTweenDriver constructor
currently only checks if Duration, Delay, and RepeatDelay are finite values, but
does not reject negative Duration values which can prevent the tween from
reaching completion. Modify the condition in the if statement that includes the
isFinite checks to also validate that config.Duration is greater than 0,
ensuring negative duration values are rejected upfront to match behavior in
other drivers and prevent invalid tween states.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs`:
- Around line 52-53: The BmotionNumericKeyframesDriver field _frames is assigned
a direct reference to the frames parameter without cloning, which allows
external mutations of the original array to affect the driver's behavior and
make it non-deterministic. Clone the frames array for the _frames assignment on
line 52, using the same approach as the _curFrames assignment on line 53 (using
Clone() method), to ensure the driver maintains its own independent copy of the
frames data.
- Line 119: The Complete() method in BmotionNumericKeyframesDriver always
applies the last frame (_frames[^1]), but this is incorrect for finite Mirror
and Reverse repeat modes which may naturally end on _frames[0] instead. Modify
the Complete() method to determine the correct terminal frame based on the
repeat mode and frame count, then apply that terminal frame instead of always
applying the last frame.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cs`:
- Around line 38-43: The implicit operators for BmotionAnimationTarget need to
handle null inputs properly to maintain nullability semantics. In both the
implicit operator that takes BmotionAnimationProps and the implicit operator
that takes a string, add null checks at the beginning of each operator. If the
input parameter is null, return null instead of creating a new
BmotionAnimationTarget instance. This ensures that null inputs remain null and
allows downstream code like the check in Bmotion.cs to properly detect when
Animate is not explicitly set and apply the inherited variant fallback.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cs`:
- Around line 15-18: The Add method in BmotionMotionVariants class does not
validate that the props parameter is not null before storing it in the _variants
dictionary. This allows null values to be silently stored, which can cause
ResolveProps to return null for existing variant names, disabling animations.
Add a null check for the props parameter at the beginning of the Add method and
throw an appropriate exception (such as ArgumentNullException) if props is null,
ensuring only valid animation properties are stored.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cs`:
- Around line 29-33: The BmotionAnimateService constructor accepts engine and
interop parameters without null validation, which will result in unhelpful
NullReferenceExceptions if null values are passed. Add null guard clauses at the
beginning of the constructor to validate that both the engine and interop
parameters are not null, throwing an appropriate ArgumentNullException with a
descriptive message for each parameter before assigning them to the _engine and
_interop fields.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Around line 40-43: The BmotionScrollTracker constructor accepts the interop
parameter without null validation, which will cause an unhelpful
NullReferenceException later if a null value is passed. Add a null guard at the
beginning of the constructor to check if the interop parameter is null and throw
an ArgumentNullException with a descriptive message if it is, before assigning
it to the _interop field. This ensures early and clear failure with an
actionable error message.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cs`:
- Around line 104-117: The Cancel_CompletesImmediately test method only verifies
that the driver completes immediately (done is true) but fails to verify that no
new value is emitted after Cancel() is called. Add a check to the log collection
after calling driver.Cancel() and driver.Tick(150) to ensure the log size
remains unchanged or that no new value was added to the log following the
cancellation. This ensures the no-apply guarantee is validated, confirming that
Tick exits before the _apply operation when the driver is in a cancelled state.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cs`:
- Around line 17-25: In the Unregister method, the condition on line 24 requires
_children.Count to be greater than 0, which prevents the AllExitsComplete event
from firing when the final child unregisters during an active exit. Modify the
condition to also handle the case where _children.Count equals 0, as this
indicates all children have been unregistered and the exit is complete. The
check should evaluate to true either when there are remaining children that have
all completed, or when there are no remaining children at all during an active
exit.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d7c44b61-b2d3-4b28-8520-33aaa8127c9c
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs (1)
9-10:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftChild stagger indexing still has no lifecycle reset/unregister path.
Line [30] monotonically increments
_nextChildIndex, but this context has no way to release/reset indices. Combined withBmotionassigning a child index once and never unregistering on disposal (seesrc/Bmotion/Bit.Bmotion/Components/Bmotion.cs, Line [298] and Line [783]), remount cycles can accumulate larger delays from Line [40].Also applies to: 30-40
🤖 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 `@src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs` around lines 9 - 10, The `_nextChildIndex` field in BmotionVariantContext is incremented monotonically without any mechanism to reset or release indices, causing accumulated delays on remounts. Add lifecycle methods to the BmotionVariantContext class to track and cleanup child indices (such as RemoveChildIndex or similar), then ensure the Bmotion component calls these cleanup methods when it's disposed or unregistered. This will allow indices to be reused or the counter to be properly managed across remount cycles, preventing the stagger delay from accumulating unbounded.src/Bmotion/Bit.Bmotion/Components/Bmotion.cs (1)
709-726:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeactivate
inviewlayer whenWhileInViewis removed but viewport observation remains.When
WhileInViewbecomes null butOnViewportEnter/OnViewportLeaveare still subscribed, Line [730] keeps observation enabled, and Lines [722]-[725] re-observe without clearing an already-activeinviewlayer. That can leave stale styles until a future intersection change.🔧 Suggested patch
private async Task ReconcileViewportAsync() { var sig = BuildViewportSignature(); if (sig == _viewportSig) return; _viewportSig = sig; if (sig == null) { await Interop.UnobserveViewportAsync(_id); // Unobserving stops future intersect callbacks but doesn't undo an already-active // in-view layer, so clear it here to avoid leaving inview styles stuck on the element. await Engine.DeactivateGestureLayerAsync(_id, "inview"); return; } + if (WhileInView == null) + await Engine.DeactivateGestureLayerAsync(_id, "inview"); if (Viewport != null) await Interop.ObserveViewportWithOptionsAsync(_id, _dotnet!, Viewport); else await Interop.ObserveViewportAsync(_id, _dotnet!, Once); }Also applies to: 728-735
🤖 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 `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs` around lines 709 - 726, The ReconcileViewportAsync method deactivates the inview layer only when all viewport observation stops (sig == null), but it fails to deactivate the inview layer when WhileInView is specifically removed while other viewport callbacks like OnViewportEnter or OnViewportLeave remain active. When this occurs, the code re-observes at lines 720-722 without clearing the previously-active inview layer, leaving stale styles. Add logic to detect when WhileInView transitions from being set to null while viewport observation continues, and call Engine.DeactivateGestureLayerAsync to clear the inview layer in that scenario before proceeding with the re-observation.
🤖 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 `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 23-24: The Tag property can receive null or blank values which
will cause the component to fail during rendering when OpenElement is called at
line 117. Add validation logic before the OpenElement call to check if Tag is
null or empty, and normalize it to a safe default value of "div" if needed. This
ensures that OpenElement always receives a valid tag name and prevents the
component tree from crashing.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cs`:
- Around line 22-40: The BmotionInertiaDriver constructor accepts configuration
values without validating that they are finite numbers. Non-finite values (NaN
or Infinity) in TimeConstant, Delay, Power, InertiaVelocity, InertiaRestDelta,
and the optional bounds InertiaMax and InertiaMin can propagate through the
Tick() method and cause the animation to produce NaN positions or never satisfy
the completion condition. Add validation at the start of the constructor to
ensure all input values from the config parameter are finite numbers using a
check like double.IsFinite(), throwing an appropriate exception if any value is
invalid.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cs`:
- Around line 55-57: The BmotionTweenDriver incorrectly treats Reverse and
Mirror repeat types identically by swapping values each cycle. Reverse should
consistently play backward from 1→0 without swapping, while Mirror should
alternate direction. In the repeat cycle logic around line 55-57, separate the
handling so that only BmotionRepeatType.Mirror swaps _curFrom and _curTo, while
BmotionRepeatType.Reverse never swaps. Additionally, the Complete() method
around line 66 always applies _to as the terminal value, but for finite Mirror
and Reverse sequences that end at the start value, it should apply _curFrom
instead of _to. Determine the correct terminal value based on the repeat type
and the current state to ensure the animation settles at the proper end value.
- Around line 26-33: The BmotionTweenDriver constructor accepts timing
configuration values (Duration, Delay, RepeatDelay) without validating that they
are finite numbers. Add validation guards before the assignments to _durationMs,
_delayMs, and _repeatDelayMs to ensure that config.Duration, config.Delay, and
config.RepeatDelay are not NaN or Infinity. This will prevent invalid progress
calculations and prevent the driver from running indefinitely. Consider throwing
an appropriate exception if any of these values fail the finite validation.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cs`:
- Around line 44-53: The Stop() method may skip calling _release() if
_engine.Stop(id, null) throws an exception, since _released has already been
atomically claimed. Wrap the foreach loop containing _engine.Stop(id, null)
calls in a try-finally block and move the _release() call into the finally block
to ensure it always executes. Apply the same try-finally pattern to the
Complete() method where _engine.Complete(id, null) is called, so that _release()
is guaranteed to run even when engine operations throw exceptions.
In `@src/Bmotion/README.md`:
- Around line 260-267: The Bmotion component example in the README has incorrect
quote escaping in the Initial and Animate attributes. Currently,
Initial='"hidden"' and Animate='"visible"' use nested quotes which pass the
literal strings with quote characters included as values. Fix this by removing
the inner single quotes so that Initial="hidden" and Animate="visible" properly
pass the bare variant names to the component. Ensure both the Initial and
Animate attribute values follow this corrected syntax pattern.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cs`:
- Line 1: The GlobalUsings.cs file currently only contains a global using
statement for Microsoft.VisualStudio.TestTools.UnitTesting, but the test suite
references types from Bmotion engine and model namespaces without per-file using
statements. Add global using statements for the required Bmotion namespaces
(such as Bit.Bmotion and any engine/model-related namespaces used throughout the
test files) to the GlobalUsings.cs file to resolve the compilation errors.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 709-726: The ReconcileViewportAsync method deactivates the inview
layer only when all viewport observation stops (sig == null), but it fails to
deactivate the inview layer when WhileInView is specifically removed while other
viewport callbacks like OnViewportEnter or OnViewportLeave remain active. When
this occurs, the code re-observes at lines 720-722 without clearing the
previously-active inview layer, leaving stale styles. Add logic to detect when
WhileInView transitions from being set to null while viewport observation
continues, and call Engine.DeactivateGestureLayerAsync to clear the inview layer
in that scenario before proceeding with the re-observation.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs`:
- Around line 9-10: The `_nextChildIndex` field in BmotionVariantContext is
incremented monotonically without any mechanism to reset or release indices,
causing accumulated delays on remounts. Add lifecycle methods to the
BmotionVariantContext class to track and cleanup child indices (such as
RemoveChildIndex or similar), then ensure the Bmotion component calls these
cleanup methods when it's disposed or unregistered. This will allow indices to
be reused or the counter to be properly managed across remount cycles,
preventing the stagger delay from accumulating unbounded.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3579a52a-83ef-4c47-8492-a6d9e9b91f3a
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs (1)
28-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject negative
Durationto prevent non-terminating animations.The validation checks
double.IsFinite(config.Duration)but notconfig.Duration >= 0. A negative duration causes_durationMsto be negative, and the progress calculation at line 86 will never reach1.0, making the animation loop forever for finite repeat counts.🐛 Proposed fix
- if (!double.IsFinite(config.Duration) || !double.IsFinite(config.Delay) || !double.IsFinite(config.RepeatDelay)) + if (!double.IsFinite(config.Duration) || config.Duration < 0 + || !double.IsFinite(config.Delay) || !double.IsFinite(config.RepeatDelay)) // NaN/infinite timing values poison _startTime in the progress math (e.g. a NaN delay // makes _startTime NaN), pushing invalid values through _apply. Reject them up front. throw new ArgumentException( - "Duration, Delay and RepeatDelay must be finite values.", nameof(config)); + "Duration must be finite and non-negative; Delay and RepeatDelay must be finite.", nameof(config));🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs` around lines 28 - 32, The validation in the BmotionNumericKeyframesDriver constructor checks that Duration, Delay, and RepeatDelay are finite values using double.IsFinite(), but does not check that they are non-negative. Negative duration values will cause _durationMs to be negative, which prevents the progress calculation from ever reaching 1.0, resulting in infinite animation loops even with finite repeat counts. Extend the existing ArgumentException validation condition to also require that config.Duration, config.Delay, and config.RepeatDelay are all greater than or equal to zero, ensuring all timing values are both finite and non-negative.src/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cs (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd missing global usings for Bmotion engine and model namespaces (compile blocker).
The test files reference
BmotionTweenDriver,BmotionTransitionConfig,BmotionTransitionType,BmotionEasing, andBmotionRepeatTypewithout per-fileusingstatements. Without global imports forBit.Bmotion.EngineandBit.Bmotion.Models, the test project will fail to compile.🔧 Proposed fix
global using Microsoft.VisualStudio.TestTools.UnitTesting; +global using Bit.Bmotion.Engine; +global using Bit.Bmotion.Models;🤖 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 `@src/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cs` at line 1, Add global using statements for the Bmotion engine and models namespaces in the GlobalUsings.cs file. The test files reference BmotionTweenDriver, BmotionTransitionConfig, BmotionTransitionType, BmotionEasing, and BmotionRepeatType types without per-file using statements. Add global using statements for Bit.Bmotion.Engine and Bit.Bmotion.Models alongside the existing Microsoft.VisualStudio.TestTools.UnitTesting import to make these types available throughout the test project and resolve the compile errors.src/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs (1)
279-279:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
Timesarray mutation to match the correctedEaseCubicBezierpattern.At Line 279,
clone.Times![0] = 99mutates a potentially defensive-copy returned by the getter, not the clone's stored array. IfTimesreturns a defensive copy (likeEaseCubicBezierdoes at Line 278), this mutation won't actually modify the clone's internal state, and the assertion at Line 283 could pass without validating the intended deep-copy independence. Apply the same fix used forEaseCubicBezier: assign a new array with the modified value.🧪 Proposed test fix
clone.Duration = 9.9; // EaseCubicBezier's getter returns a defensive copy, so mutating the getter result would // hit a throwaway array. Assign a new array to mutate the clone's actual stored state. clone.EaseCubicBezier = [0.9, 0.2, 0.3, 0.4]; - clone.Times![0] = 99; + clone.Times = [99, 1]; Assert.AreEqual(0.3, original.Duration); // scalar untouched Assert.AreEqual(0.1, original.EaseCubicBezier![0]); // array deep-copied Assert.AreEqual(0.0, original.Times![0]); // array deep-copied🤖 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 `@src/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs` at line 279, The Times property likely returns a defensive copy similar to how EaseCubicBezier does at line 278, so directly mutating the returned array at line 279 with clone.Times![0] = 99 won't modify the clone's internal state. Instead of mutating the returned array, create a new array with the modified value and assign it to clone.Times, following the same defensive-copy pattern used for EaseCubicBezier to ensure the test properly validates deep-copy independence.
🧹 Nitpick comments (1)
src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs (1)
145-207: ⚡ Quick winDeduplicate transform/path composition to prevent future renderer drift.
ToCssStyleString()andToCssStyleDictionary()currently duplicate transform precedence and path-dash calculations. Extract shared helpers so both renderers always use the same rules.♻️ Refactor sketch
- var transforms = new List<string>(); - ... - if (PathLength.HasValue) - { - double clamped = Math.Max(0, Math.Min(1, PathLength.Value)); - double spacing = Math.Max(0, Math.Min(1, PathSpacing ?? 1.0)); - double offset = Math.Max(0, Math.Min(1, PathOffset ?? 0.0)); - ... - } + var transforms = BuildTransforms(); + var path = BuildPathDashValues(); // null when PathLength is unset + ... + if (path is not null) { ... } + private List<string> BuildTransforms() + { + // single source of truth for perspective/translate/scale/rotate/skew order + } + + private (double length, double spacing, double offset)? BuildPathDashValues() + { + // single source of truth for clamping/defaults + }Also applies to: 216-276
🤖 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 `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs` around lines 145 - 207, The ToCssStyleString() method contains transform building logic and path-dash calculations that are duplicated in ToCssStyleDictionary(). Extract two helper methods: one to build and return the transforms list (containing the logic for X/Y/Z translation, Scale vs ScaleX/ScaleY, Rotate vs RotateZ, and other rotation/skew operations), and another to calculate and return the clamped PathLength, PathSpacing, and PathOffset values. Call these new helper methods from both ToCssStyleString() and ToCssStyleDictionary() to eliminate duplication and ensure both renderers always apply the same transformation precedence and path calculation rules.
🤖 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 `@src/Bmotion/Bit.Bmotion/Bit.Bmotion.csproj`:
- Around line 9-13: Remove the global IL2091 warning suppression from the NoWarn
element in the Bit.Bmotion.csproj project file. Instead, add scoped suppressions
using UnconditionalSuppressMessage attributes or `#pragma` warning disable
directives at the specific locations in BitBmotion.cs (particularly around lines
60-64 in the JS interop helpers) where the generic DotNetObjectReference forward
declarations are used and where DynamicDependency attributes are already
present. This approach allows the trimmer to catch future trim/AOT regressions
while still suppressing IL2091 only where it is safe and actionable.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs`:
- Around line 466-497: Serialize the disposal of shared loop resources with the
loop startup to prevent race conditions. The EnsureLoopRunningAsync method uses
_loopStartGate to serialize startup, but the DisposeAsync method (around lines
560-577) must also acquire the same _loopStartGate before disposing of shared
resources like _dotnet and stopping the loop. Ensure that DisposeAsync waits on
_loopStartGate before accessing or disposing these shared resources to prevent
startup and teardown from interleaving.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cs`:
- Around line 65-70: The color string parameter in the method is not trimmed
before parsing, causing inputs with leading or trailing whitespace like " `#fff` "
to fail the StartsWith('#') check and other prefix-based parsing logic, which
forces a fallback to string interpolation. Trim the incoming color string
parameter c at the beginning of the parsing logic, before the
string.IsNullOrEmpty check, so that whitespace-padded inputs are properly
handled and the subsequent prefix checks and regex patterns can correctly
identify the color format.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs`:
- Around line 367-374: The `CompleteAll()` method snapshots
`_activeAnims.Values` for iteration but then unconditionally clears the entire
`_activeAnims` collection and resolves all batches after the loop completes,
which wipes any new animations or batches started by user callbacks during
`driver.Complete()` invocations. Snapshot both the `_activeAnims` collection and
any relevant batch collection before iterating, then only remove or clear the
items from those original snapshots rather than operating on the live
collections. This preserves any re-entrant animations or batches that callbacks
may have added during the completion process.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cs`:
- Around line 106-112: The BmotionSpringDriver currently treats Reverse repeat
type identically to Mirror on line 110, but BmotionColorTweenDriver implements
reverse-once semantics where the sequence plays forward once then backward once
and stops, unlike Mirror which continuously ping-pongs. Additionally, the
Complete() method at line 126 always applies _target as the terminal value, but
for finite mirrored or reversed sequences this is not the correct natural
endpoint. Refactor the repeat type logic to differentiate between Mirror (which
swaps _from and _target to ping-pong continuously) and Reverse (which should
only reverse once without the swap pattern), and update the Complete() method to
return the appropriate terminal value based on whether the sequence has finished
in a mirrored/reversed state, rather than always returning _target.
- Around line 40-66: Non-finite values in spring configuration inputs (such as
Stiffness, Damping, Velocity, RestSpeed, RestDelta, Delay, RepeatDelay, and
bounce-derived VisualDuration/Duration) can cause internal state variables like
_vel and _pos to become non-finite, leaving the driver running indefinitely. Add
validation at the start of the initialization block (before assigning k from
config.Stiffness) to check that all critical configuration values are finite
using double.IsFinite(), and throw an exception or apply safe defaults for any
non-finite inputs. Ensure this validation covers config.Stiffness,
config.Damping, config.Velocity, config.RestSpeed, config.RestDelta,
config.Delay, config.RepeatDelay, and duration-related values used in the
SpringFromBounce calculation.
In `@src/Bmotion/README.md`:
- Around line 260-268: In the Bmotion component example in the README, the
Initial and Animate properties use incorrect quote escaping syntax that passes
literal quote characters as part of the variant name values. Change the Initial
property from using single quotes around double-quoted content ('"hidden"') to
using only double quotes around the bare variant name (="hidden"), and similarly
change the Animate property from ('"visible"') to (="visible"). This ensures the
Bmotion component receives the actual variant names without extra quote
characters embedded in the strings.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs`:
- Around line 28-32: The validation in the BmotionNumericKeyframesDriver
constructor checks that Duration, Delay, and RepeatDelay are finite values using
double.IsFinite(), but does not check that they are non-negative. Negative
duration values will cause _durationMs to be negative, which prevents the
progress calculation from ever reaching 1.0, resulting in infinite animation
loops even with finite repeat counts. Extend the existing ArgumentException
validation condition to also require that config.Duration, config.Delay, and
config.RepeatDelay are all greater than or equal to zero, ensuring all timing
values are both finite and non-negative.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cs`:
- Line 1: Add global using statements for the Bmotion engine and models
namespaces in the GlobalUsings.cs file. The test files reference
BmotionTweenDriver, BmotionTransitionConfig, BmotionTransitionType,
BmotionEasing, and BmotionRepeatType types without per-file using statements.
Add global using statements for Bit.Bmotion.Engine and Bit.Bmotion.Models
alongside the existing Microsoft.VisualStudio.TestTools.UnitTesting import to
make these types available throughout the test project and resolve the compile
errors.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs`:
- Line 279: The Times property likely returns a defensive copy similar to how
EaseCubicBezier does at line 278, so directly mutating the returned array at
line 279 with clone.Times![0] = 99 won't modify the clone's internal state.
Instead of mutating the returned array, create a new array with the modified
value and assign it to clone.Times, following the same defensive-copy pattern
used for EaseCubicBezier to ensure the test properly validates deep-copy
independence.
---
Nitpick comments:
In `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs`:
- Around line 145-207: The ToCssStyleString() method contains transform building
logic and path-dash calculations that are duplicated in ToCssStyleDictionary().
Extract two helper methods: one to build and return the transforms list
(containing the logic for X/Y/Z translation, Scale vs ScaleX/ScaleY, Rotate vs
RotateZ, and other rotation/skew operations), and another to calculate and
return the clamped PathLength, PathSpacing, and PathOffset values. Call these
new helper methods from both ToCssStyleString() and ToCssStyleDictionary() to
eliminate duplication and ensure both renderers always apply the same
transformation precedence and path calculation rules.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eb2fa37a-0f58-4469-8f70-dc6e4407553c
📒 Files selected for processing (82)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionXY.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
Comment on lines
+9
to
+13
| <IsTrimmable>true</IsTrimmable> | ||
| <!-- The generic JS interop helpers forward DotNetObjectReference<T> where T is always a | ||
| concrete component/service type the runtime can see, so the trimmer's IL2091 advice | ||
| about propagating DynamicallyAccessedMembers annotations isn't actionable here. --> | ||
| <NoWarn>$(NoWarn);IL2091</NoWarn> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where IL2091 is suppressed and whether local suppressions exist.
rg -n --type=xml 'NoWarn|IL2091|IsTrimmable' src/Bmotion/Bit.Bmotion/Bit.Bmotion.csproj
rg -n --type=cs 'IL2091|UnconditionalSuppressMessage|`#pragma` warning disable|DynamicDependency' src/Bmotion/Bit.BmotionRepository: bitfoundation/bitplatform
Length of output: 1422
Move IL2091 suppression from project level to scoped suppressions.
Line 13 suppresses IL2091 globally while line 9 marks the package trimmable. This prevents catching future trim/AOT regressions. Instead of project-wide suppression, use UnconditionalSuppressMessage or #pragma warning disable IL2091 at specific call sites where IL2091 is safe—such as the JS interop helpers in BitBmotion.cs (lines 60-64) that already employ [DynamicDependency] attributes.
🤖 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 `@src/Bmotion/Bit.Bmotion/Bit.Bmotion.csproj` around lines 9 - 13, Remove the
global IL2091 warning suppression from the NoWarn element in the
Bit.Bmotion.csproj project file. Instead, add scoped suppressions using
UnconditionalSuppressMessage attributes or `#pragma` warning disable directives at
the specific locations in BitBmotion.cs (particularly around lines 60-64 in the
JS interop helpers) where the generic DotNetObjectReference forward declarations
are used and where DynamicDependency attributes are already present. This
approach allows the trimmer to catch future trim/AOT regressions while still
suppressing IL2091 only where it is safe and actionable.
Comment on lines
+466
to
+497
| public async ValueTask EnsureLoopRunningAsync() | ||
| { | ||
| if (_loopRunning) return; | ||
|
|
||
| // Concurrent callers (e.g. several elements registering on the same frame) can all pass the | ||
| // check above before any of them flips _loopRunning, which would start the rAF loop more | ||
| // than once. Serialize startup behind a gate so only the first caller starts it; the rest | ||
| // re-check _loopRunning after acquiring the gate and become no-ops. | ||
| await _loopStartGate.WaitAsync(); | ||
| try | ||
| { | ||
| if (_loopRunning) return; | ||
|
|
||
| // Bit.Bmotion's animation loop relies on synchronous JS→.NET interop (the JS rAF ticker | ||
| // calls ComputeFrame synchronously). That is only available on Blazor WebAssembly; on | ||
| // Blazor Server / SSR the call would throw an opaque error, so fail fast with a clear one. | ||
| if (!_interop.IsInProcess) | ||
| throw new PlatformNotSupportedException( | ||
| "Bit.Bmotion requires synchronous JS interop and is only supported on Blazor WebAssembly. " + | ||
| "It cannot run on Blazor Server or during server-side prerendering."); | ||
|
|
||
| _dotnet ??= DotNetObjectReference.Create(this); | ||
| await _interop.StartRafLoopAsync(_dotnet); | ||
| // Only flag the loop as running once startup actually succeeded; if the interop call | ||
| // throws, the flag stays false so a later call can retry instead of silently no-op'ing. | ||
| _loopRunning = true; | ||
| } | ||
| finally | ||
| { | ||
| _loopStartGate.Release(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Serialize disposal with loop startup before disposing shared loop resources.
DisposeAsync can run while EnsureLoopRunningAsync is still in-flight, which can leave startup/teardown interleaving on _dotnet and _loopStartGate. This risks teardown-time faults and inconsistent loop state.
Suggested hardening
public async ValueTask EnsureLoopRunningAsync()
{
if (_loopRunning) return;
- await _loopStartGate.WaitAsync();
+ bool entered = false;
+ try
+ {
+ await _loopStartGate.WaitAsync();
+ entered = true;
+ }
+ catch (ObjectDisposedException)
+ {
+ return;
+ }
try
{
if (_loopRunning) return;
@@
_loopRunning = true;
}
finally
{
- _loopStartGate.Release();
+ if (entered)
+ {
+ try { _loopStartGate.Release(); }
+ catch (ObjectDisposedException) { /* disposed during teardown */ }
+ }
}
}
public async ValueTask DisposeAsync()
{
+ bool gateEntered = false;
+ try
+ {
+ await _loopStartGate.WaitAsync();
+ gateEntered = true;
+
foreach (var (_, state) in _elements)
state.CancelAll();
_elements.Clear();
@@
if (_dotnet != null)
{
try { await _interop.StopRafLoopAsync(_dotnet); } catch { /* ignore during teardown */ }
try { await _interop.UnwatchReducedMotionAsync(_dotnet); } catch { /* ignore during teardown */ }
_dotnet.Dispose();
_dotnet = null;
}
- _loopStartGate.Dispose();
+ }
+ catch (ObjectDisposedException)
+ {
+ return;
+ }
+ finally
+ {
+ if (gateEntered)
+ {
+ try { _loopStartGate.Release(); } catch (ObjectDisposedException) { }
+ }
+ try { _loopStartGate.Dispose(); } catch (ObjectDisposedException) { }
+ }
}Also applies to: 560-577
🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs` around lines 466 -
497, Serialize the disposal of shared loop resources with the loop startup to
prevent race conditions. The EnsureLoopRunningAsync method uses _loopStartGate
to serialize startup, but the DisposeAsync method (around lines 560-577) must
also acquire the same _loopStartGate before disposing of shared resources like
_dotnet and stopping the loop. Ensure that DisposeAsync waits on _loopStartGate
before accessing or disposing these shared resources to prevent startup and
teardown from interleaving.
Comment on lines
+65
to
+70
| { | ||
| if (string.IsNullOrEmpty(c)) return null; | ||
|
|
||
| if (c.StartsWith('#')) | ||
| { | ||
| var h = c[1..]; |
There was a problem hiding this comment.
Trim incoming color strings before parsing.
Line 66 only checks string.IsNullOrEmpty(c) and parsing then relies on prefix checks/anchored regex; inputs like " #fff " or " rgb(0,0,0) " fail parsing and force the string-Lerp fallback to jump directly to to.
Suggested patch
internal static double[]? Parse(string c)
{
- if (string.IsNullOrEmpty(c)) return null;
+ if (string.IsNullOrWhiteSpace(c)) return null;
+ c = c.Trim();📝 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
| { | |
| if (string.IsNullOrEmpty(c)) return null; | |
| if (c.StartsWith('#')) | |
| { | |
| var h = c[1..]; | |
| { | |
| if (string.IsNullOrWhiteSpace(c)) return null; | |
| c = c.Trim(); | |
| if (c.StartsWith('#')) | |
| { | |
| var h = c[1..]; |
🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cs` around lines 65 -
70, The color string parameter in the method is not trimmed before parsing,
causing inputs with leading or trailing whitespace like " `#fff` " to fail the
StartsWith('#') check and other prefix-based parsing logic, which forces a
fallback to string interpolation. Trim the incoming color string parameter c at
the beginning of the parsing logic, before the string.IsNullOrEmpty check, so
that whitespace-padded inputs are properly handled and the subsequent prefix
checks and regex patterns can correctly identify the color format.
Comment on lines
+367
to
+374
| { | ||
| // Snapshot the drivers before iterating: driver.Complete() applies the final value, which | ||
| // can invoke a user OnUpdate callback that re-enters and mutates _activeAnims (e.g. starts | ||
| // a new animation on this element). Iterating the live Values collection would then throw. | ||
| foreach (var driver in _activeAnims.Values.ToArray()) | ||
| driver.Complete(); | ||
| _activeAnims.Clear(); | ||
| ResolveAllBatches(true); // snapped to end values = completed |
There was a problem hiding this comment.
CompleteAll() can wipe re-entrant animations started during completion callbacks.
At Line 373 and Lines 409-413, new animations/batches started from driver.Complete() callbacks are cleared/resolved as if they were part of the original completion pass. Snapshot and detach the current sets before invoking callbacks.
💡 Minimal fix
internal void CompleteAll()
{
- foreach (var driver in _activeAnims.Values.ToArray())
- driver.Complete();
- _activeAnims.Clear();
- ResolveAllBatches(true); // snapped to end values = completed
+ var drivers = _activeAnims.Values.ToArray();
+ _activeAnims.Clear();
+
+ var batches = _batches.ToArray();
+ _batches.Clear();
+
+ foreach (var driver in drivers)
+ driver.Complete();
+
+ foreach (var b in batches)
+ b.Source.TrySetResult(true);
}Also applies to: 409-414
🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs` around lines
367 - 374, The `CompleteAll()` method snapshots `_activeAnims.Values` for
iteration but then unconditionally clears the entire `_activeAnims` collection
and resolves all batches after the loop completes, which wipes any new
animations or batches started by user callbacks during `driver.Complete()`
invocations. Snapshot both the `_activeAnims` collection and any relevant batch
collection before iterating, then only remove or clear the items from those
original snapshots rather than operating on the live collections. This preserves
any re-entrant animations or batches that callbacks may have added during the
completion process.
Comment on lines
+40
to
+66
| double k = config.Stiffness; | ||
| double d = config.Damping; | ||
| if (config.Bounce.HasValue) | ||
| { | ||
| double vd = config.VisualDuration ?? config.Duration; | ||
| (k, d) = BmotionTransitionConfig.SpringFromBounce(vd, config.Bounce.Value, config.Mass); | ||
| } | ||
|
|
||
| _k = k; | ||
| _d = d; | ||
| // Mass divides the acceleration each sub-step; a value <= 0 would yield NaN/Infinity and | ||
| // trap the spring (the rest test would never pass). Fall back to the default mass of 1. | ||
| _m = config.Mass > 0 ? config.Mass : 1.0; | ||
| _vel = _initialVel = config.Velocity; | ||
| // Rest thresholds are scaled by the animation's magnitude so large-range springs (e.g. | ||
| // x: 0→1000) settle in proportion to their distance instead of chasing an absolute 0.01px/ | ||
| // 0.01px-per-sec target for many extra frames. Small ranges keep the absolute thresholds. | ||
| double range = Math.Abs(to - from); | ||
| double restScale = range > 1.0 ? range : 1.0; | ||
| // Clamp to a small positive floor: a non-positive RestSpeed/RestDelta would make the | ||
| // completion gate (Abs(vel) < restSpeed && Abs(pos-target) < restDelta) unsatisfiable, | ||
| // leaving the spring ticking forever. | ||
| const double minRest = 1e-4; | ||
| _restSpeed = Math.Max(config.RestSpeed * restScale, minRest); | ||
| _restDelta = Math.Max(config.RestDelta * restScale, minRest); | ||
| _currentDelayMs = config.Delay * 1000; | ||
| _repeatDelayMs = config.RepeatDelay * 1000; |
There was a problem hiding this comment.
Validate spring inputs before simulation math.
At Line 40 onward, non-finite Stiffness/Damping/Velocity/RestSpeed/RestDelta/Delay/RepeatDelay (and bounce-derived durations) can make _vel/_pos non-finite and leave the driver running forever.
💡 Minimal hardening
public BmotionSpringDriver(double from, double to, BmotionTransitionConfig config, Action<double> apply)
{
+ if (!double.IsFinite(config.Stiffness) || !double.IsFinite(config.Damping) ||
+ !double.IsFinite(config.Velocity) || !double.IsFinite(config.RestSpeed) ||
+ !double.IsFinite(config.RestDelta) || !double.IsFinite(config.Delay) ||
+ !double.IsFinite(config.RepeatDelay) || config.Delay < 0 || config.RepeatDelay < 0)
+ throw new ArgumentException("Spring parameters must be finite, and delays must be non-negative.", nameof(config));
+ if (config.Bounce is { } bounce && !double.IsFinite(bounce))
+ throw new ArgumentException("Bounce must be finite when provided.", nameof(config));
+ if (config.VisualDuration is { } vd && (!double.IsFinite(vd) || vd < 0))
+ throw new ArgumentException("VisualDuration must be finite and non-negative when provided.", nameof(config));
+
_pos = _from = from;🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cs` around lines 40 - 66,
Non-finite values in spring configuration inputs (such as Stiffness, Damping,
Velocity, RestSpeed, RestDelta, Delay, RepeatDelay, and bounce-derived
VisualDuration/Duration) can cause internal state variables like _vel and _pos
to become non-finite, leaving the driver running indefinitely. Add validation at
the start of the initialization block (before assigning k from config.Stiffness)
to check that all critical configuration values are finite using
double.IsFinite(), and throw an exception or apply safe defaults for any
non-finite inputs. Ensure this validation covers config.Stiffness,
config.Damping, config.Velocity, config.RestSpeed, config.RestDelta,
config.Delay, config.RepeatDelay, and duration-related values used in the
SpringFromBounce calculation.
Comment on lines
+106
to
+112
| if (_isInfinite || _iteration < _repeat) | ||
| { | ||
| if (!_isInfinite) _iteration++; | ||
| // Mirror/Reverse ping-pong back to the start; Loop replays from the origin. | ||
| if (_repeatType is BmotionRepeatType.Mirror or BmotionRepeatType.Reverse) | ||
| (_from, _target) = (_target, _from); | ||
| _pos = _from; |
There was a problem hiding this comment.
Reverse repeat handling and Complete() terminal value are inconsistent across drivers.
Line 110 currently treats Reverse like Mirror, while BmotionColorTweenDriver implements reverse-once semantics. Also, Line 126 always applies _target, which is not always the natural terminal value for finite mirrored/reversed sequences.
Also applies to: 126-126
🤖 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 `@src/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cs` around lines 106 -
112, The BmotionSpringDriver currently treats Reverse repeat type identically to
Mirror on line 110, but BmotionColorTweenDriver implements reverse-once
semantics where the sequence plays forward once then backward once and stops,
unlike Mirror which continuously ping-pongs. Additionally, the Complete() method
at line 126 always applies _target as the terminal value, but for finite
mirrored or reversed sequences this is not the correct natural endpoint.
Refactor the repeat type logic to differentiate between Mirror (which swaps
_from and _target to ping-pong continuously) and Reverse (which should only
reverse once without the swap pattern), and update the Complete() method to
return the appropriate terminal value based on whether the sequence has finished
in a mirrored/reversed state, rather than always returning _target.
Comment on lines
+260
to
+268
| <Bmotion Variants="variants" | ||
| Initial='"hidden"' | ||
| Animate='"visible"' | ||
| Transition='new BmotionTransitionConfig { StaggerChildren = 0.1 }'> | ||
| <Bmotion>Item 1</Bmotion> | ||
| <Bmotion>Item 2</Bmotion> | ||
| <Bmotion>Item 3</Bmotion> | ||
| </Bmotion> | ||
| ``` |
There was a problem hiding this comment.
Fix incorrect quote escaping in variant example—unresolved from prior review.
Lines 261–262 show:
Initial='"hidden"'
Animate='"visible"'This Razor syntax passes the literal strings "hidden" and "visible" (with quote characters included), not the bare variant names. The correct syntax must omit the inner quotes:
Initial="hidden"
Animate="visible"A prior review flagged this issue and claimed it was fixed in commits ee041c4–5a55545, but the incorrect syntax is still present. Users copying this example will encounter compilation errors or incorrect animation behavior.
🐛 Proposed fix
<Bmotion Variants="variants"
- Initial='"hidden"'
- Animate='"visible"'
+ Initial="hidden"
+ Animate="visible"
Transition='new BmotionTransitionConfig { StaggerChildren = 0.1 }'>🤖 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 `@src/Bmotion/README.md` around lines 260 - 268, In the Bmotion component
example in the README, the Initial and Animate properties use incorrect quote
escaping syntax that passes literal quote characters as part of the variant name
values. Change the Initial property from using single quotes around
double-quoted content ('"hidden"') to using only double quotes around the bare
variant name (="hidden"), and similarly change the Animate property from
('"visible"') to (="visible"). This ensures the Bmotion component receives the
actual variant names without extra quote characters embedded in the strings.
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.
closes #12447
Summary by CodeRabbit
<Bmotion>,<BmotionAnimatePresence>,<BmotionConfig>, variants, gestures/drag (including constraints), FLIP layout animation, viewport-triggered animations, scroll tracking, reduced-motion handling, and programmatic animation controls.