Add Bmotion tool (#12447)#12448
Open
msynk wants to merge 5 commits into
Open
Conversation
msynk
requested review from
Cyrus-Sushiant,
mhrastegari,
yasmoradi and
zahra-yousefi
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA Blazor WebAssembly demo app and Bit.Bmotion library are added. The library introduces animation models, contexts, engine/runtime code, interop, components, and services. The demo app adds routed sample pages, shared layout, CSS, startup wiring, and tests/documentation cover the new APIs and drivers. ChangesBit.Bmotion end-to-end implementation
Sequence Diagram(s)sequenceDiagram
participant Motion
participant AnimationEngine
participant MotionInterop
participant BitBmotionJS
Motion->>AnimationEngine: AnimateToAsync and gesture updates
AnimationEngine->>MotionInterop: StartRafLoopAsync / ApplyStylesAsync
MotionInterop->>BitBmotionJS: invoke JS module functions
BitBmotionJS->>AnimationEngine: ComputeFrame(timestamp)
BitBmotionJS->>Motion: OnPointer*, OnPan*, OnIntersect callbacks
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ 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
Comment on lines
+35
to
+49
| @foreach (var item in _items) | ||
| { | ||
| <AnimatePresence IsPresent="true" @key="item.Id"> | ||
| <Motion Tag="div" | ||
| Style="display:flex;justify-content:space-between;align-items:center;background:rgba(108,71,255,.12);border:1px solid rgba(108,71,255,.25);border-radius:8px;padding:.6rem 1rem;" | ||
| Initial="@(new AnimationProps { Opacity = 0, X = -30 })" | ||
| Animate="@(new AnimationProps { Opacity = 1, X = 0 })" | ||
| Exit="@(new AnimationProps { Opacity = 0, X = 30 })" | ||
| Transition="@TransitionConfig.Spring(stiffness:300, damping:25)"> | ||
| <span>@item.Label</span> | ||
| <button style="background:none;border:none;color:#ff4785;cursor:pointer;font-size:1.1rem" | ||
| @onclick="@(() => RemoveItem(item.Id))">✕</button> | ||
| </Motion> | ||
| </AnimatePresence> | ||
| } |
There was a problem hiding this comment.
List item exit animation is bypassed by per-item AnimatePresence usage.
On Line 37, each item uses <AnimatePresence IsPresent="true">, so removed items are dropped from the loop before IsPresent can transition to false. This prevents the intended exit lifecycle for list removals. Use a single parent AnimatePresence around the collection and key each Motion item, or keep a presence flag per item and flip it before removal.
🤖 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
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.
Comment on lines
+39
to
+41
| private readonly PresenceContext _presenceCtx = new(); | ||
| private bool _shouldRender = true; | ||
| private bool _prevIsPresent = true; |
There was a problem hiding this comment.
Initial IsPresent=false is treated like an exit.
Because _shouldRender and _prevIsPresent both start as true, the first parameter set with IsPresent = false enters the leave path and keeps rendering children even though nothing was previously present.
Suggested fix
- private bool _shouldRender = true;
- private bool _prevIsPresent = true;
+ private bool _shouldRender;
+ private bool _prevIsPresent;
@@
protected override void OnInitialized()
{
+ _shouldRender = IsPresent;
+ _prevIsPresent = IsPresent;
_presenceCtx.AllExitsComplete += OnAllExitsComplete;
}Also applies to: 53-60
🤖 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 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.
Comment on lines
+132
to
+139
| if (Variants != null) | ||
| { | ||
| _ownVariantCtx ??= new VariantContext(); | ||
| _ownVariantCtx.ActiveVariant = Animate?.IsVariant == true ? Animate.Variant : null; | ||
| _ownVariantCtx.InitialVariant = Initial?.IsVariant == true ? Initial.Variant : null; | ||
| _ownVariantCtx.Variants = Variants; | ||
| _ownVariantCtx.StaggerChildren = Transition?.StaggerChildren ?? 0; | ||
| _ownVariantCtx.DelayChildren = Transition?.DelayChildren ?? 0; |
There was a problem hiding this comment.
Nested variant trees lose the inherited active label.
When this component defines Variants, it creates a fresh VariantContext but only copies local explicit variant targets. If the active label came from an ancestor, descendants see null, so nested variant propagation and stagger stop at this node.
Suggested fix
_ownVariantCtx ??= new VariantContext();
- _ownVariantCtx.ActiveVariant = Animate?.IsVariant == true ? Animate.Variant : null;
- _ownVariantCtx.InitialVariant = Initial?.IsVariant == true ? Initial.Variant : null;
+ _ownVariantCtx.ActiveVariant = Animate?.IsVariant == true
+ ? Animate.Variant
+ : VariantCtx?.ActiveVariant;
+ _ownVariantCtx.InitialVariant = Initial?.IsVariant == true
+ ? Initial.Variant
+ : VariantCtx?.InitialVariant;
_ownVariantCtx.Variants = Variants;🤖 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 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.
Comment on lines
+379
to
+385
| [JSInvokable] | ||
| public async Task OnPointerUp(bool isInsideElement) | ||
| { | ||
| if (WhileTap != null) | ||
| await Engine.DeactivateGestureLayerAsync(_id, "tap"); | ||
| if (isInsideElement) await OnTap.InvokeAsync(); | ||
| } |
There was a problem hiding this comment.
Release-outside never raises OnTapCancel.
OnPointerUp(false) clears the tap layer, but it does not invoke the public cancel callback. That drops the most common cancellation path: press inside, release outside.
Suggested fix
public async Task OnPointerUp(bool isInsideElement)
{
if (WhileTap != null)
await Engine.DeactivateGestureLayerAsync(_id, "tap");
- if (isInsideElement) await OnTap.InvokeAsync();
+ if (isInsideElement)
+ await OnTap.InvokeAsync();
+ else
+ await OnTapCancel.InvokeAsync();
}📝 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
| [JSInvokable] | |
| public async Task OnPointerUp(bool isInsideElement) | |
| { | |
| if (WhileTap != null) | |
| await Engine.DeactivateGestureLayerAsync(_id, "tap"); | |
| if (isInsideElement) await OnTap.InvokeAsync(); | |
| } | |
| [JSInvokable] | |
| public async Task OnPointerUp(bool isInsideElement) | |
| { | |
| if (WhileTap != null) | |
| await Engine.DeactivateGestureLayerAsync(_id, "tap"); | |
| if (isInsideElement) | |
| await OnTap.InvokeAsync(); | |
| else | |
| await OnTapCancel.InvokeAsync(); | |
| } |
🤖 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 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.
Comment on lines
+10
to
+16
| private readonly List<Motion> _children = new(); | ||
|
|
||
| /// <summary>True while the children are playing their exit animation.</summary> | ||
| public bool IsExiting { get; internal set; } | ||
|
|
||
| internal void Register(Motion child) => _children.Add(child); | ||
| internal void Unregister(Motion child) => _children.Remove(child); |
There was a problem hiding this comment.
Prevent duplicate child registration in presence tracking.
Using List<Motion> plus unconditional Register allows duplicates. With completion tracked in a HashSet, _completedChildren.Count may never reach _children.Count, so AllExitsComplete can fail to fire.
Proposed fix
- private readonly List<Motion> _children = new();
+ private readonly HashSet<Motion> _children = new();
- internal void Register(Motion child) => _children.Add(child);
+ internal void Register(Motion child) => _children.Add(child);
internal void Unregister(Motion child) => _children.Remove(child);Also applies to: 28-29
🤖 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 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.
Comment on lines
+34
to
+40
| double threshold = Amount switch | ||
| { | ||
| "some" => 0.0, | ||
| "all" => 1.0, | ||
| _ => double.TryParse(Amount, System.Globalization.NumberStyles.Any, | ||
| System.Globalization.CultureInfo.InvariantCulture, out var v) | ||
| ? Math.Clamp(v, 0, 1) : 0.0, |
There was a problem hiding this comment.
Harden Amount parsing (case + non-finite values).
Lines 34–40 treat "some"/"all" case-sensitively and can propagate non-finite parsed values. That can produce wrong thresholds for valid user intent and invalid runtime thresholds.
Proposed fix
- double threshold = Amount switch
+ var amount = Amount?.Trim();
+ double threshold = amount?.ToLowerInvariant() switch
{
"some" => 0.0,
"all" => 1.0,
- _ => double.TryParse(Amount, System.Globalization.NumberStyles.Any,
+ _ => double.TryParse(amount, System.Globalization.NumberStyles.Float,
System.Globalization.CultureInfo.InvariantCulture, out var v)
- ? Math.Clamp(v, 0, 1) : 0.0,
+ && double.IsFinite(v)
+ ? Math.Clamp(v, 0d, 1d) : 0.0,
};📝 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
| double threshold = Amount switch | |
| { | |
| "some" => 0.0, | |
| "all" => 1.0, | |
| _ => double.TryParse(Amount, System.Globalization.NumberStyles.Any, | |
| System.Globalization.CultureInfo.InvariantCulture, out var v) | |
| ? Math.Clamp(v, 0, 1) : 0.0, | |
| var amount = Amount?.Trim(); | |
| double threshold = amount?.ToLowerInvariant() switch | |
| { | |
| "some" => 0.0, | |
| "all" => 1.0, | |
| _ => double.TryParse(amount, System.Globalization.NumberStyles.Float, | |
| System.Globalization.CultureInfo.InvariantCulture, out var v) | |
| && double.IsFinite(v) | |
| ? Math.Clamp(v, 0d, 1d) : 0.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/Models/ViewportOptions.cs` around lines 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.
Comment on lines
+37
to
+41
| public void Complete() | ||
| { | ||
| foreach (var id in _elementIds) | ||
| _engine.Stop(id, null); | ||
| } |
There was a problem hiding this comment.
Complete() currently behaves exactly like Stop() despite a different contract.
Lines 37-41 call the same _engine.Stop(id, null) path as Stop(), so “snap to target end values” is not implemented as documented.
🤖 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/AnimationControls.cs` around lines 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.
Comment on lines
+32
to
+37
| public void SetSync(T value) | ||
| { | ||
| _value = value; | ||
| foreach (var sub in _subscribers.ToArray()) | ||
| _ = ObserveAsync(sub(value)); | ||
| } |
There was a problem hiding this comment.
Wrap subscriber invocation itself, not just the returned task.
ObserveAsync(...) only helps after a Task exists. Subscribe(Action<T>) and any Func<T, Task> that throws before returning a task will escape from sub(value), fault SetSync/SetAsync, and skip later subscribers.
Also applies to: 46-50, 63-64
🤖 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 - 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.
Comment on lines
+72
to
+108
| public MotionValue<TOut> Transform<TOut>(Func<T, TOut> fn) where TOut : struct | ||
| { | ||
| var derived = new MotionValue<TOut>($"{_id}_t", fn(_value)); | ||
| Subscribe(async v => await derived.SetAsync(fn(v))); | ||
| return derived; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Map from an input range to an output range using linear interpolation. | ||
| /// </summary> | ||
| public MotionValue<double> Transform(double[] inputRange, double[] outputRange) | ||
| { | ||
| if (inputRange.Length != outputRange.Length) | ||
| throw new ArgumentException("inputRange and outputRange must have the same length."); | ||
| if (inputRange.Length < 2) | ||
| throw new ArgumentException("inputRange and outputRange must contain at least 2 points."); | ||
| for (int i = 0; i < inputRange.Length - 1; i++) | ||
| if (inputRange[i + 1] <= inputRange[i]) | ||
| throw new ArgumentException("inputRange must be strictly increasing (no repeated or decreasing points)."); | ||
|
|
||
| double Map(T v) | ||
| { | ||
| double x = Convert.ToDouble(v); | ||
| for (int i = 0; i < inputRange.Length - 1; i++) | ||
| { | ||
| if (x >= inputRange[i] && x <= inputRange[i + 1]) | ||
| { | ||
| double t = (x - inputRange[i]) / (inputRange[i + 1] - inputRange[i]); | ||
| return outputRange[i] + t * (outputRange[i + 1] - outputRange[i]); | ||
| } | ||
| } | ||
| return x < inputRange[0] ? outputRange[0] : outputRange[^1]; | ||
| } | ||
|
|
||
| var derived = new MotionValue<double>($"{_id}_tr", Map(_value)); | ||
| Subscribe(async v => await derived.SetAsync(Map(v))); | ||
| return derived; |
There was a problem hiding this comment.
Dispose the upstream transform subscription with the derived value.
Both Transform overloads drop the IDisposable returned by Subscribe(...). After the derived MotionValue is disposed, the source still holds its callback and continues updating an object the caller considers dead.
Also applies to: 111-118
🤖 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 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.
Comment on lines
+206
to
+255
| function _attachPan(el, dotnetRef, cleanups) { | ||
| const PAN_THRESHOLD = 3; // pixels before pan is detected | ||
| let panning = false; | ||
| let startX, startY, lastX, lastY, lastT; | ||
| let velX = 0, velY = 0; | ||
|
|
||
| const onDown = (e) => { | ||
| if (e.button !== 0 && e.pointerType !== 'touch') return; | ||
| startX = lastX = e.clientX; startY = lastY = e.clientY; | ||
| lastT = Date.now(); velX = velY = 0; panning = false; | ||
| el.setPointerCapture(e.pointerId); | ||
| }; | ||
|
|
||
| const onMove = (e) => { | ||
| const dx = e.clientX - startX, dy = e.clientY - startY; | ||
| const now = Date.now(), dt = now - lastT; | ||
| const deltaX = e.clientX - lastX, deltaY = e.clientY - lastY; | ||
| if (dt > 0) { | ||
| velX = deltaX / dt * 1000; | ||
| velY = deltaY / dt * 1000; | ||
| } | ||
|
|
||
| if (!panning && Math.sqrt(dx * dx + dy * dy) >= PAN_THRESHOLD) { | ||
| panning = true; | ||
| dotnetRef.invokeMethodAsync('OnPanStart_'); | ||
| } | ||
| if (panning) { | ||
| dotnetRef.invokeMethodAsync('OnPanMove', | ||
| e.clientX, e.clientY, | ||
| deltaX, deltaY, | ||
| dx, dy, | ||
| velX, velY); | ||
| } | ||
|
|
||
| lastX = e.clientX; lastY = e.clientY; lastT = now; | ||
| }; | ||
|
|
||
| const onUp = () => { if (panning) { panning = false; dotnetRef.invokeMethodAsync('OnPanEnd_'); } }; | ||
|
|
||
| el.addEventListener('pointerdown', onDown); | ||
| el.addEventListener('pointermove', onMove); | ||
| el.addEventListener('pointerup', onUp); | ||
| el.addEventListener('pointercancel', onUp); | ||
| cleanups.push(() => { | ||
| el.removeEventListener('pointerdown', onDown); | ||
| el.removeEventListener('pointermove', onMove); | ||
| el.removeEventListener('pointerup', onUp); | ||
| el.removeEventListener('pointercancel', onUp); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Gate pan detection behind an active pointer.
_attachPan never records whether a pointer is currently down. After one completed gesture, ordinary hover pointermove events can reuse the stale startX/startY and trigger OnPanStart_ without any pressed button.
💡 Minimal fix
function _attachPan(el, dotnetRef, cleanups) {
const PAN_THRESHOLD = 3; // pixels before pan is detected
+ let tracking = false;
let panning = false;
let startX, startY, lastX, lastY, lastT;
let velX = 0, velY = 0;
const onDown = (e) => {
if (e.button !== 0 && e.pointerType !== 'touch') return;
+ tracking = true;
startX = lastX = e.clientX; startY = lastY = e.clientY;
lastT = Date.now(); velX = velY = 0; panning = false;
el.setPointerCapture(e.pointerId);
};
const onMove = (e) => {
+ if (!tracking) return;
const dx = e.clientX - startX, dy = e.clientY - startY;
const now = Date.now(), dt = now - lastT;
const deltaX = e.clientX - lastX, deltaY = e.clientY - lastY;
@@
- const onUp = () => { if (panning) { panning = false; dotnetRef.invokeMethodAsync('OnPanEnd_'); } };
+ const onUp = () => {
+ if (!tracking) return;
+ tracking = false;
+ if (panning) { panning = false; dotnetRef.invokeMethodAsync('OnPanEnd_'); }
+ };🤖 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 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.
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
Motioncomponent,AnimatePresence, and globalMotionConfig.