Apply Bmotion improvements (#12582)#12584
Merged
Merged
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ 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:
WalkthroughThis PR replaces the old Bmotion* API surface with Bm* models, rewrites the Bmotion component and engine for the new contracts, updates programmatic services and JS interop, migrates demos and docs, and expands tests across the new behaviors. ChangesBmotion library overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant Bmotion
participant BmotionChildContentRewriter
participant BmotionAnimationEngine
participant bit_bmotion_js as bit-bmotion.js
Consumer->>Bmotion: render with Initial/Animate/Exit
Bmotion->>BmotionChildContentRewriter: Render(child content, injection plan)
BmotionChildContentRewriter-->>Bmotion: injected id/style/pathLength
Bmotion->>BmotionAnimationEngine: AnimateToAsync(BmProps, BmTransition)
BmotionAnimationEngine->>bit_bmotion_js: registerElement / playWaapiAnimation / observeScroll
bit_bmotion_js-->>BmotionAnimationEngine: completion / BmScrollInfo
BmotionAnimationEngine-->>Bmotion: OnAnimationComplete(BmProps?)
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs (1)
161-226: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize overlapping WAAPI starts per element.
AnimateToAsync/AnimateToAwaitAsynccan interleave at the await points, so two calls for the same element/properties can both register WAAPI plans and leave competing compositor animations running until a later interrupt. Add a per-element gate or cancel any existing plan before adding the new one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs` around lines 161 - 226, AnimateToAsync and AnimateToAwaitAsync can race at their await points and register overlapping WAAPI plans for the same element, leaving competing compositor animations active. Add per-element serialization in BmotionAnimationEngine, or cancel/replace any existing pending plan before a new one is registered, so only one animation path can proceed for a given elementId at a time. Use the AnimateToAsync, AnimateToAwaitAsync, and InterruptWaapiOverlapsAsync flow to place the gate or preemption logic where the overlap can occur.
🧹 Nitpick comments (11)
src/Bmotion/README.md (1)
168-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign playback-control naming across the public API.
BmotionexposesPause()/Resume()/SetPlaybackRate(), whileBmAnimationControlsusesPause()/Play()/SetSpeed()for the same operations. The mismatch makes the surface harder to learn and document consistently.🤖 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 168 - 181, Align the playback-control API names between Bmotion and BmAnimationControls so the public surface is consistent and easier to document. Update the instance-methods example and the related control method names around Bmotion/BmAnimationControls so the same operation uses the same verb across both types, especially for resume/play and playback-rate/speed. Make the naming choice consistent in the referenced Bmotion API and any corresponding BmAnimationControls members so users see one set of terms throughout the docs and API.src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor (1)
61-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
@using Bit.Bmotionin the new_switchCodesnippet.Every other embedded snippet in this file (
_presenceCodeat line 140,_listCodeat line 189) starts with@using Bit.Bmotion, but the new_switchCode(lines 93-109) omits it. If a user copies this Presence Switch sample as shown,Bmotion,BmotionPresenceSwitch,Bm, andBmEasewon't resolve without a global using already present in their project.📝 Proposed fix
private const string _switchCode = """ + `@using` Bit.Bmotion + <BmotionPresenceSwitch Item="_page" Context="pageNumber">🤖 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 61 - 111, Add the missing Bit.Bmotion import to the embedded Presence Switch sample in _switchCode so the snippet is self-contained like the other demo snippets. Update the _switchCode constant in AnimatePresencePage.razor to include the same `@using` Bit.Bmotion header used by _presenceCode and _listCode, ensuring Bmotion, BmotionPresenceSwitch, Bm, and BmEase resolve when the sample is copied.src/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razor (1)
48-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTab switcher isn't keyboard accessible.
Tabs are plain
<div>s driven only by@onclick; there's notabindex, ARIArole, or@onkeydownhandling for Enter/Space, so keyboard-only users can't switch tabs in this new demo.♿ Proposed fix
<div style="position:relative;padding-bottom:6px;cursor:pointer;font-weight:600;color:@(t == _activeTab ? "`#fff`" : "`#888`")" - `@onclick`="@(() => _activeTab = t)"> + role="tab" + tabindex="0" + aria-selected="@(t == _activeTab ? "true" : "false")" + `@onclick`="@(() => _activeTab = t)" + `@onkeydown`="@(e => { if (e.Key is "Enter" or " ") _activeTab = 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/Bit.Bmotion.Demos/Pages/LayoutPage.razor` around lines 48 - 63, The tab switcher in LayoutPage.razor uses clickable divs only, so keyboard users cannot activate tabs. Update the tab item markup in the _tabs foreach to make each tab focusable and semantic by adding an appropriate role and tabindex, then handle Enter/Space with `@onkeydown` to set _activeTab the same way `@onclick` does. Keep the active state styling tied to _activeTab so the existing Bmotion underline and label styling still work.src/Bmotion/Bit.Bmotion/Models/BmScrollOptions.cs (2)
55-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNumeric edge values aren't validated to be within [0, 1].
The doc comment says an edge is "
start(0),center(0.5),end(1) or a 0–1 number", butParseEdgeonly checks that a numeric edge is finite, not that it's within[0, 1]. Values like"1.5 end"or"-0.2 start"will silently pass through and can produce out-of-range scroll progress downstream.🛠️ Proposed fix
_ => double.TryParse(edge, System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var v) && double.IsFinite(v) - ? v + ? (v is >= 0 and <= 1 + ? v + : throw new ArgumentException( + $"Numeric edge '{edge}' in scroll offset '{offset}' must be within [0, 1].")) : throw new ArgumentException( $"Unknown edge '{edge}' in scroll offset '{offset}'. Use start, center, end or a 0-1 number."),🤖 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/BmScrollOptions.cs` around lines 55 - 65, ParseEdge currently accepts any finite numeric edge, but it should only allow values in the documented 0–1 range. Update the ParseEdge helper in BmScrollOptions so the numeric branch validates that the parsed value is finite and between 0 and 1 inclusive, and throws the same ArgumentException otherwise. Keep the existing handling for the named tokens start, center, and end unchanged.
38-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMalformed
Offsetarray silently falls back to defaults instead of erroring.If
Offsethas any length other than 2 (e.g. a typo leaving 1 or 3 entries), the code silently discards it and uses["start end", "end start"]instead of surfacing the mistake, unlike the per-string validation inParseOffset/ParseEdgewhich does throw on malformed input.🛠️ Proposed fix
- var offset = Offset is { Length: 2 } ? Offset : ["start end", "end start"]; + var offset = Offset switch + { + null => new[] { "start end", "end start" }, + { Length: 2 } => Offset, + _ => throw new ArgumentException( + $"Offset must contain exactly 2 entries, got {Offset.Length}.", nameof(Offset)), + };🤖 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/BmScrollOptions.cs` at line 38, The Offset handling in BmScrollOptions currently falls back to the default pair when the array length is not exactly 2, which hides malformed input. Update the Offset logic in the scroll options parsing path so that any non-null Offset with a length other than 2 throws a validation error instead of silently substituting ["start end", "end start"]. Keep the existing ParseOffset and ParseEdge validation behavior consistent by surfacing the mistake early in BmScrollOptions.src/Bmotion/Bit.Bmotion/Models/BmProps.cs (3)
125-130: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated CssVars key-filtering logic across three methods.
The identical
if (!kv.Key.StartsWith("--")) continue;filter loop is repeated verbatim inToJsDictionary,ToCssStyleString, andToCssStyleDictionary. Consider extracting a shared helper (e.g.,IEnumerable<KeyValuePair<string,string>> ValidCssVars()) to avoid triple maintenance. Also, silently dropping invalid keys with no exception/log could hide a developer typo (e.g., forgetting the--prefix) until they notice missing styling in production.Also applies to: 210-215, 292-297
🤖 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/BmProps.cs` around lines 125 - 130, The same CssVars key filter is duplicated in ToJsDictionary, ToCssStyleString, and ToCssStyleDictionary, so extract the shared "--" validation into a helper such as ValidCssVars() and have all three methods use it. Also, make invalid CssVars keys visible by adding a clear warning or exception path instead of silently skipping them, so typos like a missing "--" prefix are easier to catch. Use the existing BmProps methods and CssVars iteration as the single place to centralize this logic.
191-200: 🔒 Security & Privacy | 🔵 TrivialRaw string props are concatenated into the style string/CssVars with no escaping.
The doc comment on Lines 13-17 already flags CSS-injection risk for string-valued properties, but there's no enforcement: a value containing
;or}(e.g., fromBackgroundColor,BoxShadow, or aCssVarsvalue bound to external input) can terminate its own declaration and inject arbitrary additional CSS into the element's inlinestyle, potentially enabling overlay/clickjacking-style attacks if a consumer ignores the documented warning. Consider a lightweight guard (reject/encode values containing;,{,}, or newlines) as defense-in-depth beyond the doc warning.Also applies to: 273-282, 210-215, 292-297
🤖 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/BmProps.cs` around lines 191 - 200, The style-building logic in BmProps concatenates raw string values directly into the inline style/CssVars output, so add defense-in-depth validation/escaping for string-valued properties and CSS variable values before appending them. Update the style assembly paths in the BmProps methods that build the CSS string so values like BackgroundColor, BoxShadow, and CssVars entries reject or sanitize unsafe characters such as semicolons, braces, and newlines, using the existing CssStr helper or a shared sanitizer to keep the fix consistent across all affected concatenation sites.
158-186: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated transform-composition logic between
ToCssStyleStringandToCssStyleDictionary.The transform list construction (translate/scale/rotate/skew/perspective ordering and zero-value elision) is duplicated almost line-for-line between the two methods, differing only in the value-accessor (
CssNumvsNum) and output sink. This duplication already requires the "prefer non-zero rotateZ" comment to be copy-pasted (lines 176-179 and 260-263) — any future fix to the transform ordering must be applied twice or the two representations will silently diverge.Also applies to: 242-269
🤖 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/BmProps.cs` around lines 158 - 186, The transform-composition logic is duplicated in both ToCssStyleString and ToCssStyleDictionary, so refactor the shared translate/scale/rotate/skew/perspective building into a single helper used by both methods. Keep the existing behavior consistent in that helper, including value precedence, zero-value elision, and transform ordering, and ensure the “prefer non-zero rotateZ” rule is implemented once instead of being copy-pasted across both call sites. Use the existing symbols ToCssStyleString, ToCssStyleDictionary, and BmotionCssFormat to locate the duplicated code.src/Bmotion/Tests/Bit.Bmotion.Tests/Models/BmApiTests.cs (1)
238-249: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReal
Task.Delaymakes velocity test timing-dependent.Relying on wall-clock delay to build up velocity is a common source of CI flakiness under load. Consider injecting/mocking a clock or time provider into
BmValueso the test can control elapsed time deterministically instead of sleeping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Models/BmApiTests.cs` around lines 238 - 249, The velocity test is timing-dependent because it relies on a real Task.Delay to accumulate elapsed time, which can make BmApiTests.Value_TracksVelocity_AndJumpResetsIt flaky. Update BmValue to use an injectable/mocked clock or time provider for velocity calculations, then adjust Value_TracksVelocity_AndJumpResetsIt to advance time deterministically through that dependency instead of sleeping. Keep the assertions around GetVelocity and Jump, but drive the elapsed time through the new clock abstraction so the test is stable in CI.src/Bmotion/Bit.Bmotion/Components/Bmotion.cs (1)
961-989: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMotion-value bindings churn and allocate on the animation hot path.
Two related concerns:
ReconcileValueBindingsshort-circuits viaReferenceEquals(_boundValues, Values), but the parameter's own XML-doc example (Values='new() { ["x"] = _x, ... }') passes a fresh dictionary literal on every parent render, defeating this check and forcing a full dispose/resubscribe + immediate re-seed on every render that touches this component.- Each per-value subscription callback and the initial seed call allocates a new
Dictionary<string, object?>(lines 983-984, 986) - this runs every time a boundBmValuechanges, i.e. potentially every animation frame.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs` around lines 961 - 989, The motion-value binding path in ReconcileValueBindings is redoing work on every parent render and allocating on every value update. Adjust the binding reconciliation so it does not rely on ReferenceEquals(_boundValues, Values) when Values may be recreated each render, and preserve subscriptions unless the actual contents change. Also remove the per-callback/per-seed Dictionary<string, object?> allocations inside the Value.Subscribe path by reusing a writable payload or batching updates through Engine.SetInstant in a lower-allocation way.src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs (1)
189-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider surfacing swallowed
OnFrameexceptions for observability.The empty
catchsilently discards any exception from the user callback. Reasonable to prevent loop breakage, but with zero visibility a brokenOnUpdate/OnFrameconsumer fails silently forever.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs` around lines 189 - 195, The empty catch around the OnFrame callback in BmotionElementAnimationState silently hides user callback failures, so add observability without breaking the animation loop. Update the OnFrame invocation guard to catch the exception and report it through the engine’s existing logging/error-reporting path, or at minimum rethrow/forward it after preventing loop disruption, so failures in OnUpdate/OnFrame consumers are visible while preserving the ComputeFrame fault handling behavior.
🤖 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/LayoutPage.razor`:
- Around line 39-43: The LayoutId demo and the `_layoutIdCode` snippet describe
`BmLayout.Position`, but `Bmotion` is never given a `Layout` value, so the
sample does not match the documented behavior. Update the `Bmotion` usage in the
`LayoutPage` demo and the generated code string so the `tab-underline` example
explicitly sets the `Layout` parameter to the position-only semantics, keeping
the sample and docs aligned.
In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 724-736: The DragSnapToOrigin branch in Bmotion should end the
drag before animating back to origin, because AnimateToAsync is currently
running without the cleanup that Engine.EndDragAsync provides for _isDragging.
Update the drag handling in Bmotion so the DragSnapToOrigin path first calls
Engine.EndDragAsync with momentum disabled, then performs the snap animation to
x/y = 0 using the existing DragTransition config, mirroring the cleanup already
done in the non-snap path.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionChildContentRewriter.cs`:
- Line 322: The raw-markup branch in BmotionChildContentRewriter is writing
injection.Id directly into an attribute string, so it must be encoded before
being appended. Update the AddMarkupContent path in BmotionChildContentRewriter
to HTML-encode the Id value before building the id attribute, ensuring quotes or
other special characters cannot break out of the attribute. Keep the fix local
to the injection.Id append logic so the rest of the markup rewriting behavior
stays unchanged.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionPresenceSwitch.cs`:
- Around line 110-125: Clear the exit flag before removing the completed entry
in OnEntryExited, since BmotionPresenceContext.Unregister may trigger
AllExitsComplete during disposal while the item is still marked exiting. Update
the exit-completion flow in BmotionPresenceSwitch.OnEntryExited so the matching
Entry is no longer considered active/exiting before _entries.Remove(entry) and
before the OnExitComplete callback path runs, preventing the same outgoing item
from reaching the completion handler twice.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs`:
- Around line 18-19: The BmotionConfigContext playback-rate comment mixes the
positive-rate rule with the zero case, which incorrectly implies durations are
divided by zero for instant mode. Update the documentation on the playback-rate
property to describe the normal behavior for rates greater than zero separately
from the special case where 0 means instant, and keep the wording clear in the
property comment/summary.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs`:
- Around line 278-303: The Blazor Server instant path in
BmotionElementAnimationState.ApplyValues/the per-key transition selection drops
callback and override behavior because the ForceInstant-built
BmotionTransitionConfig never carries transition.OnUpdate or
transition.Properties[key].OnUpdate. Update the instant fallback so it preserves
the original transition’s OnUpdate and any per-key Properties entry before
CreateNumericDriver and related driver setup consume perKey, ensuring the
zero-duration snap still notifies motion-value tracking and respects
key-specific overrides.
- Around line 853-859: Register the WAAPI plan as soon as playback begins in
SuperviseOffloadAsync by adding it through AddWaapiPlan before awaiting
PlayWaapiAnimationAsync, so InterruptWaapiOverlapsAsync can see in-flight plans
and cancel overlaps correctly. If startup fails, remove the plan again to roll
back the early registration. Use the existing AddWaapiPlan and
PlayWaapiAnimationAsync flow in BmotionElementAnimationState to keep _waapiPlans
accurate throughout compositor startup.
In `@src/Bmotion/Bit.Bmotion/Models/BmTransition.cs`:
- Around line 68-81: The ValueEquals method in BmTransition is treating OnUpdate
as part of transition equality, which causes inline callbacks recreated during
rerenders to look like changes. Update BmTransition.ValueEquals so it compares
only animation-defining state (Delay, Repeat, StaggerChildren, DelayChildren,
Properties) and excludes OnUpdate from the equality check; if callback changes
need handling, do that separately from transition change detection.
In `@src/Bmotion/Bit.Bmotion/Services/BmAnimationControls.cs`:
- Around line 40-57: SetSpeed in BmAnimationControls is racing with Stop,
Complete, and OnCompletionSettled because it only checks _released with
Volatile.Read and then calls _engine.SetPlaybackRate unguarded. Update SetSpeed
to use the same exclusive access pattern as Stop/Complete/ReleaseOnce by
synchronizing around the release/state claim and the engine calls, so
playback-rate changes cannot touch ids that have already been released or
reassigned. Also ensure the related paths that claim/release ownership use the
same _sync/CompareExchange pattern consistently.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Around line 150-158: Update BmotionScrollTracker so the TargetProgressValue
motion state stays in sync with TargetProgress in the update path. In the method
that assigns TargetProgress and pushes values to _progressXValue,
_progressYValue, _scrollXValue, and _scrollYValue, make sure
_targetProgressValue is also cleared when info.TargetProgress is null, not only
updated when it has a value. Use the existing TargetProgress assignment and
_targetProgressValue symbol to locate the logic and apply the null-reset in the
same branch.
---
Outside diff comments:
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs`:
- Around line 161-226: AnimateToAsync and AnimateToAwaitAsync can race at their
await points and register overlapping WAAPI plans for the same element, leaving
competing compositor animations active. Add per-element serialization in
BmotionAnimationEngine, or cancel/replace any existing pending plan before a new
one is registered, so only one animation path can proceed for a given elementId
at a time. Use the AnimateToAsync, AnimateToAwaitAsync, and
InterruptWaapiOverlapsAsync flow to place the gate or preemption logic where the
overlap can occur.
---
Nitpick comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor`:
- Around line 61-111: Add the missing Bit.Bmotion import to the embedded
Presence Switch sample in _switchCode so the snippet is self-contained like the
other demo snippets. Update the _switchCode constant in
AnimatePresencePage.razor to include the same `@using` Bit.Bmotion header used by
_presenceCode and _listCode, ensuring Bmotion, BmotionPresenceSwitch, Bm, and
BmEase resolve when the sample is copied.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razor`:
- Around line 48-63: The tab switcher in LayoutPage.razor uses clickable divs
only, so keyboard users cannot activate tabs. Update the tab item markup in the
_tabs foreach to make each tab focusable and semantic by adding an appropriate
role and tabindex, then handle Enter/Space with `@onkeydown` to set _activeTab the
same way `@onclick` does. Keep the active state styling tied to _activeTab so the
existing Bmotion underline and label styling still work.
In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 961-989: The motion-value binding path in ReconcileValueBindings
is redoing work on every parent render and allocating on every value update.
Adjust the binding reconciliation so it does not rely on
ReferenceEquals(_boundValues, Values) when Values may be recreated each render,
and preserve subscriptions unless the actual contents change. Also remove the
per-callback/per-seed Dictionary<string, object?> allocations inside the
Value.Subscribe path by reusing a writable payload or batching updates through
Engine.SetInstant in a lower-allocation way.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs`:
- Around line 189-195: The empty catch around the OnFrame callback in
BmotionElementAnimationState silently hides user callback failures, so add
observability without breaking the animation loop. Update the OnFrame invocation
guard to catch the exception and report it through the engine’s existing
logging/error-reporting path, or at minimum rethrow/forward it after preventing
loop disruption, so failures in OnUpdate/OnFrame consumers are visible while
preserving the ComputeFrame fault handling behavior.
In `@src/Bmotion/Bit.Bmotion/Models/BmProps.cs`:
- Around line 125-130: The same CssVars key filter is duplicated in
ToJsDictionary, ToCssStyleString, and ToCssStyleDictionary, so extract the
shared "--" validation into a helper such as ValidCssVars() and have all three
methods use it. Also, make invalid CssVars keys visible by adding a clear
warning or exception path instead of silently skipping them, so typos like a
missing "--" prefix are easier to catch. Use the existing BmProps methods and
CssVars iteration as the single place to centralize this logic.
- Around line 191-200: The style-building logic in BmProps concatenates raw
string values directly into the inline style/CssVars output, so add
defense-in-depth validation/escaping for string-valued properties and CSS
variable values before appending them. Update the style assembly paths in the
BmProps methods that build the CSS string so values like BackgroundColor,
BoxShadow, and CssVars entries reject or sanitize unsafe characters such as
semicolons, braces, and newlines, using the existing CssStr helper or a shared
sanitizer to keep the fix consistent across all affected concatenation sites.
- Around line 158-186: The transform-composition logic is duplicated in both
ToCssStyleString and ToCssStyleDictionary, so refactor the shared
translate/scale/rotate/skew/perspective building into a single helper used by
both methods. Keep the existing behavior consistent in that helper, including
value precedence, zero-value elision, and transform ordering, and ensure the
“prefer non-zero rotateZ” rule is implemented once instead of being copy-pasted
across both call sites. Use the existing symbols ToCssStyleString,
ToCssStyleDictionary, and BmotionCssFormat to locate the duplicated code.
In `@src/Bmotion/Bit.Bmotion/Models/BmScrollOptions.cs`:
- Around line 55-65: ParseEdge currently accepts any finite numeric edge, but it
should only allow values in the documented 0–1 range. Update the ParseEdge
helper in BmScrollOptions so the numeric branch validates that the parsed value
is finite and between 0 and 1 inclusive, and throws the same ArgumentException
otherwise. Keep the existing handling for the named tokens start, center, and
end unchanged.
- Line 38: The Offset handling in BmScrollOptions currently falls back to the
default pair when the array length is not exactly 2, which hides malformed
input. Update the Offset logic in the scroll options parsing path so that any
non-null Offset with a length other than 2 throws a validation error instead of
silently substituting ["start end", "end start"]. Keep the existing ParseOffset
and ParseEdge validation behavior consistent by surfacing the mistake early in
BmScrollOptions.
In `@src/Bmotion/README.md`:
- Around line 168-181: Align the playback-control API names between Bmotion and
BmAnimationControls so the public surface is consistent and easier to document.
Update the instance-methods example and the related control method names around
Bmotion/BmAnimationControls so the same operation uses the same verb across both
types, especially for resume/play and playback-rate/speed. Make the naming
choice consistent in the referenced Bmotion API and any corresponding
BmAnimationControls members so users see one set of terms throughout the docs
and API.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Models/BmApiTests.cs`:
- Around line 238-249: The velocity test is timing-dependent because it relies
on a real Task.Delay to accumulate elapsed time, which can make
BmApiTests.Value_TracksVelocity_AndJumpResetsIt flaky. Update BmValue to use an
injectable/mocked clock or time provider for velocity calculations, then adjust
Value_TracksVelocity_AndJumpResetsIt to advance time deterministically through
that dependency instead of sleeping. Keep the assertions around GetVelocity and
Jump, but drive the elapsed time through the new clock abstraction so the test
is stable in CI.
🪄 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: 18780f9e-be09-4031-b318-9f70f8d41d1e
📒 Files selected for processing (75)
src/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/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionChildContentRewriter.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionLayoutGroup.cssrc/Bmotion/Bit.Bmotion/Components/BmotionPresenceSwitch.cssrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmEaseFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/Bm.cssrc/Bmotion/Bit.Bmotion/Models/BmDrag.cssrc/Bmotion/Bit.Bmotion/Models/BmDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmEase.cssrc/Bmotion/Bit.Bmotion/Models/BmKeyframes.cssrc/Bmotion/Bit.Bmotion/Models/BmLayout.cssrc/Bmotion/Bit.Bmotion/Models/BmPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmPoint.cssrc/Bmotion/Bit.Bmotion/Models/BmPresenceMode.cssrc/Bmotion/Bit.Bmotion/Models/BmProps.cssrc/Bmotion/Bit.Bmotion/Models/BmRepeat.cssrc/Bmotion/Bit.Bmotion/Models/BmScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmScrollOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmSequence.cssrc/Bmotion/Bit.Bmotion/Models/BmStagger.cssrc/Bmotion/Bit.Bmotion/Models/BmStringKeyframes.cssrc/Bmotion/Bit.Bmotion/Models/BmTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmTransition.cssrc/Bmotion/Bit.Bmotion/Models/BmVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmViewport.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionLayoutRegistry.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Components/ChildContentRewriterTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/BmApiTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
💤 Files with no reviewable changes (8)
- src/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cs
- src/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cs
- src/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cs
- src/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cs
- src/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cs
- src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs
- src/Bmotion/Bit.Bmotion/Models/BmotionEasing.cs
- src/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs (1)
161-228: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRealize overlapping WAAPI plans before the rAF fallback starts
state.AnimateTo(...)only cancels active rAF drivers; it does not clear_waapiPlans. A concurrent caller can still register a compositor plan during the await gap, leaving the WAAPI path and the rAF driver owning the same property at once. Fold in aRealizeWaapiPlans(values.Keys)/cancel sweep before starting the rAF branch, or move that overlap handling into the state method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs` around lines 161 - 228, The rAF fallback path in AnimateToAsync and AnimateToAwaitAsync still allows stale or concurrent WAAPI plans to remain active because state.AnimateTo(...) only cancels rAF drivers. Add a WAAPI realization/cancel sweep for the same property keys before starting the rAF branch, using the existing overlap handling around InterruptWaapiOverlapsAsync and the WAAPI plan registration flow, or move that cleanup into the state-level animation method so only one owner drives each property.
🧹 Nitpick comments (1)
src/Bmotion/Tests/Bit.Bmotion.Tests/Models/BmApiTests.cs (1)
238-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftWall-clock dependence makes this test flaky.
TrackVelocityzeroes velocity when the gap between sets is>= 0.25s. Since this test relies onawait Task.Delay(30)and then assertsGetVelocity() > 0, a scheduling stall on a loaded CI agent (delay overshooting ~250ms) would reset velocity to0and fail the assertion spuriously. Consider injecting a clock/time source intoBmValue<T>so velocity math is deterministic under test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Models/BmApiTests.cs` around lines 238 - 249, The Value_TracksVelocity_AndJumpResetsIt test is timing-dependent because it relies on Task.Delay and real wall-clock behavior. Make BmValue<T>.TrackVelocity use an injectable clock/time source instead of hard-coded elapsed time so tests can control the interval deterministically, then update BmApiTests.Value_TracksVelocity_AndJumpResetsIt to use the testable time source rather than sleeping and relying on GetVelocity() staying positive.
🤖 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/ReorderPage.razor`:
- Around line 15-25: The reorder demo in ReorderPage.razor only exposes pointer
drag through the .grip handle inside BmotionReorderGroup, so add an accessible
keyboard fallback for moving items. Update the track row markup in the
BmotionReorderGroup Context="track" template to include move up/down buttons or
arrow-key handling, and wire those actions to the _tracks collection so keyboard
users can reorder items without dragging.
In `@src/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.js`:
- Around line 239-243: The keyboard tap handler in onKeyDown currently triggers
activation without stopping the browser’s default Space behavior, so focused
non-button elements can still scroll the page. Update onKeyDown in the
bit-bmotion.js keyboard interaction logic to call e.preventDefault() when
isTapKey(e) passes and before invoking OnPointerDown through dotnetRef, while
preserving the existing repeat and keyPressing guards. Keep the fix scoped to
the tap-key path so native non-tap key behavior is unchanged.
---
Outside diff comments:
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cs`:
- Around line 161-228: The rAF fallback path in AnimateToAsync and
AnimateToAwaitAsync still allows stale or concurrent WAAPI plans to remain
active because state.AnimateTo(...) only cancels rAF drivers. Add a WAAPI
realization/cancel sweep for the same property keys before starting the rAF
branch, using the existing overlap handling around InterruptWaapiOverlapsAsync
and the WAAPI plan registration flow, or move that cleanup into the state-level
animation method so only one owner drives each property.
---
Nitpick comments:
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Models/BmApiTests.cs`:
- Around line 238-249: The Value_TracksVelocity_AndJumpResetsIt test is
timing-dependent because it relies on Task.Delay and real wall-clock behavior.
Make BmValue<T>.TrackVelocity use an injectable clock/time source instead of
hard-coded elapsed time so tests can control the interval deterministically,
then update BmApiTests.Value_TracksVelocity_AndJumpResetsIt to use the testable
time source rather than sleeping and relying on GetVelocity() staying positive.
🪄 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: c3d48596-f2b9-40d3-b5dd-5fbdcba523cc
📒 Files selected for processing (42)
src/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/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ReorderPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionChildContentRewriter.cssrc/Bmotion/Bit.Bmotion/Components/BmotionPresenceGroup.cssrc/Bmotion/Bit.Bmotion/Components/BmotionPresenceSwitch.cssrc/Bmotion/Bit.Bmotion/Components/BmotionReorderGroup.cssrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmEaseFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionStringMixer.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/Bm.cssrc/Bmotion/Bit.Bmotion/Models/BmDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmDragControls.cssrc/Bmotion/Bit.Bmotion/Models/BmDragElastic.cssrc/Bmotion/Bit.Bmotion/Models/BmPresenceMode.cssrc/Bmotion/Bit.Bmotion/Models/BmProps.cssrc/Bmotion/Bit.Bmotion/Models/BmScrollOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmTransition.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/README.mdsrc/Bmotion/Bit.Bmotion/Services/BmAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/StringMixerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/BmApiTests.cs
✅ Files skipped from review due to trivial changes (2)
- src/Bmotion/Bit.Bmotion/Models/BmDragElastic.cs
- src/Bmotion/Bit.Bmotion/README.md
🚧 Files skipped from review as they are similar to previous changes (18)
- src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs
- src/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cs
- src/Bmotion/Bit.Bmotion/Models/BmTransition.cs
- src/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razor
- src/Bmotion/Bit.Bmotion/Models/BmProps.cs
- src/Bmotion/Bit.Bmotion/Components/BmotionChildContentRewriter.cs
- src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs
- src/Bmotion/Bit.Bmotion/Models/BmDragConstraints.cs
- src/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cs
- src/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razor
- src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs
- src/Bmotion/Bit.Bmotion/Services/BmAnimationControls.cs
- src/Bmotion/Bit.Bmotion/Components/BmotionPresenceSwitch.cs
- src/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cs
- src/Bmotion/README.md
- src/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razor
- src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs
- src/Bmotion/Bit.Bmotion/Components/Bmotion.cs
msynk
force-pushed
the
12582-bmotion-improvements
branch
from
ca2d922 to
afa847f
Compare
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
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 #12582
Summary by CodeRabbit