✨ add follow camera tracking animation#189
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesTrackingAnimation feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
src/Spillgebees.Blazor.Map.Docs/Samples/CameraFollow/CameraFollowExample.razor.cs (1)
273-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
TrainMotionChoiceaboveDisposeAsyncor moveDisposeAsyncto the end.This new enum keeps the component out of the required
.razor.csmember order. As per coding guidelines,**/*.razor.cs: Member order in C# components: [Inject] → [CascadingParameter] → [Parameter] first, then all other public members before private, with nested types and DisposeAsync last; keep fields/consts at the top of their group.🤖 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/Spillgebees.Blazor.Map.Docs/Samples/CameraFollow/CameraFollowExample.razor.cs` around lines 273 - 278, The new TrainMotionChoice nested enum is out of the required component member order in CameraFollowExample; move TrainMotionChoice so it appears before DisposeAsync, or move DisposeAsync to the very end of the type. Keep the .razor.cs ordering rule in mind for the component: injected/cascading/parameter members first, then other public members, then private members, with nested types and DisposeAsync last.Source: Coding guidelines
🤖 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/Spillgebees.Blazor.Map.Assets/src/engine/follow.ts`:
- Around line 275-276: `applyTrackMove` is updating `lastMoveAt` for every
camera correction, which keeps the movement timer fresh even when
`positionMoved` is false. Update the tracking flow in `follow.ts` so
`lastMoveAt` only advances on real entity position changes, and keep
bearing/zoom/pitch drift corrections separate from the discrete move timestamp.
Apply the same adjustment in the related `matchheading`/tracking logic around
the other affected block so `shouldAnimateTrackMove(...)` still allows
`camera.trackingAnimation` for the next true jump.
In `@src/Spillgebees.Blazor.Map/Models/Follow/MapFollowCameraOptions.cs`:
- Around line 20-28: MapFollowCameraOptions only exposes the new 8-parameter
primary constructor, which breaks callers compiled against the older 7-parameter
shape. Update MapFollowCameraOptions to preserve binary compatibility by keeping
the existing constructor signature available and exposing TrackingAnimation
through a separate compat path, such as an explicit overload or init-only
property, while leaving the older parameter order and defaults intact.
---
Nitpick comments:
In
`@src/Spillgebees.Blazor.Map.Docs/Samples/CameraFollow/CameraFollowExample.razor.cs`:
- Around line 273-278: The new TrainMotionChoice nested enum is out of the
required component member order in CameraFollowExample; move TrainMotionChoice
so it appears before DisposeAsync, or move DisposeAsync to the very end of the
type. Keep the .razor.cs ordering rule in mind for the component:
injected/cascading/parameter members first, then other public members, then
private members, with nested types and DisposeAsync last.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bbbbc847-15ee-42ab-b479-a963a5dda0c8
📒 Files selected for processing (40)
src/Spillgebees.Blazor.Map.Assets/src/engine/follow.test.tssrc/Spillgebees.Blazor.Map.Assets/src/engine/follow.tssrc/Spillgebees.Blazor.Map.Assets/src/engine/ops.tssrc/Spillgebees.Blazor.Map.Assets/tests/browser/integration/engine-follow.spec.tssrc/Spillgebees.Blazor.Map.Docs/Samples/CameraFollow/CameraFollowExample.razorsrc/Spillgebees.Blazor.Map.Docs/Samples/CameraFollow/CameraFollowExample.razor.cssrc/Spillgebees.Blazor.Map.Docs/Samples/TrainTracking/TrainSampleState.cssrc/Spillgebees.Blazor.Map.Docs/Samples/TrainTracking/TrainTrackingExample.razor.cssrc/Spillgebees.Blazor.Map.IntegrationTests/Pages/EngineFollowFunctionalTest.razorsrc/Spillgebees.Blazor.Map.IntegrationTests/StressQuery.cssrc/Spillgebees.Blazor.Map.Tests/Engine/CameraFollowOpSerializationTests.cssrc/Spillgebees.Blazor.Map.Tests/Engine/EngineOpsSerializationTests.cssrc/Spillgebees.Blazor.Map.Tests/Engine/MapFollowCoordinatorTests.cssrc/Spillgebees.Blazor.Map.Tests/Engine/MotionFrameEncoderTests.cssrc/Spillgebees.Blazor.Map/Components/DisplayMapControl.razor.cssrc/Spillgebees.Blazor.Map/Components/GeolocateMapControl.cssrc/Spillgebees.Blazor.Map/Components/Layers/GeoJsonSource.cssrc/Spillgebees.Blazor.Map/Components/Layers/TrackedEntityLayer.cssrc/Spillgebees.Blazor.Map/Components/LegendMapControl.razor.cssrc/Spillgebees.Blazor.Map/Components/MapCircle.cssrc/Spillgebees.Blazor.Map/Components/MapControls.cssrc/Spillgebees.Blazor.Map/Components/MapFeatures.cssrc/Spillgebees.Blazor.Map/Components/MapMarker.cssrc/Spillgebees.Blazor.Map/Components/MapOverlay.cssrc/Spillgebees.Blazor.Map/Components/MapOverlays.cssrc/Spillgebees.Blazor.Map/Components/MapPolyline.cssrc/Spillgebees.Blazor.Map/Components/MapSources.cssrc/Spillgebees.Blazor.Map/Components/NavigationMapControl.cssrc/Spillgebees.Blazor.Map/Components/OverlayMapControl.razor.cssrc/Spillgebees.Blazor.Map/Engine/EngineJson.cssrc/Spillgebees.Blazor.Map/Engine/EngineOps.cssrc/Spillgebees.Blazor.Map/Engine/EngineStyleJson.cssrc/Spillgebees.Blazor.Map/Engine/MapEngineChannel.cssrc/Spillgebees.Blazor.Map/Engine/MapFollowCoordinator.cssrc/Spillgebees.Blazor.Map/Models/Controls/MapControlOptions.cssrc/Spillgebees.Blazor.Map/Models/Display/MapDisplayState.cssrc/Spillgebees.Blazor.Map/Models/Follow/MapFollowCameraOptions.cssrc/Spillgebees.Blazor.Map/Models/Follow/MapFollowOptions.cssrc/Spillgebees.Blazor.Map/Models/Legends/MapLegendItemTemplateContext.cssrc/Spillgebees.Blazor.Map/PublicAPI.Shipped.txt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Spillgebees.Blazor.Map/Components/SgbMap.razor.cs (1)
633-647: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrain active control sync before tearing down map resources.
MarkDisposing()blocks new syncs, but an already-entered sync can pass Line 676 and still run_controls.Sync()whileDisposeAsync()continues toDisposeMapAsync()andRouter.Dispose(). Wait on the control-sync semaphore after marking disposal and before tearing down shared resources.Proposed fix
public async ValueTask DisposeAsync() { MarkDisposing(); + await DrainControlSyncAsync(); _readyTcs.TrySetResult(false); _subscribedDisplay?.Changed -= HandleDisplayChanged;+private async ValueTask DrainControlSyncAsync() +{ + if (!TryEnterControlSyncDrain()) + { + return; + } + + try + { + await _controlSyncLock.WaitAsync(); + _controlSyncLock.Release(); + } + finally + { + ExitControlSync(); + } +} + +private bool TryEnterControlSyncDrain() +{ + lock (_controlSyncStateLock) + { + if (_controlSyncLockDisposed) + { + return false; + } + + _controlSyncUsers++; + return true; + } +}Also applies to: 666-687
🤖 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/Spillgebees.Blazor.Map/Components/SgbMap.razor.cs` around lines 633 - 647, Drain any in-flight control sync before disposing shared map resources in DisposeAsync. After MarkDisposing() blocks new syncs, wait for the control-sync semaphore in the SgbMap component so an already-entered sync cannot continue into _controls.Sync() while MapEngineJs.DisposeMapAsync() and Router.Dispose() are running. Keep the ordering in DisposeAsync so the drain happens before teardown, and preserve the existing cleanup flow afterward.
🧹 Nitpick comments (1)
src/Spillgebees.Blazor.Map/Components/SgbMap.razor.cs (1)
690-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
DisposeAsynclast in the component partial.The new private control-sync helpers are placed after
DisposeAsync; move helpers before disposal and leaveDisposeAsyncat the end of the component. As per coding guidelines, "**/*.razor.cs: Member order in C# components: [Inject] → [CascadingParameter] → [Parameter] first, then all other public members before private, with nested types and DisposeAsync last; keep fields/consts at the top of their group."🤖 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/Spillgebees.Blazor.Map/Components/SgbMap.razor.cs` around lines 690 - 745, The component member order is off: the new private control-sync helpers in SgbMap should be moved before the disposal method so DisposeAsync remains last in the partial. Relocate MarkDisposing, IsControlSyncDisposing, TryEnterControlSync, ExitControlSync, and DisposeControlSyncLockIfIdle to the private-member area above DisposeAsync, keeping DisposeAsync at the end of the component.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/Spillgebees.Blazor.Map/Components/SgbMap.razor.cs`:
- Around line 633-647: Drain any in-flight control sync before disposing shared
map resources in DisposeAsync. After MarkDisposing() blocks new syncs, wait for
the control-sync semaphore in the SgbMap component so an already-entered sync
cannot continue into _controls.Sync() while MapEngineJs.DisposeMapAsync() and
Router.Dispose() are running. Keep the ordering in DisposeAsync so the drain
happens before teardown, and preserve the existing cleanup flow afterward.
---
Nitpick comments:
In `@src/Spillgebees.Blazor.Map/Components/SgbMap.razor.cs`:
- Around line 690-745: The component member order is off: the new private
control-sync helpers in SgbMap should be moved before the disposal method so
DisposeAsync remains last in the partial. Relocate MarkDisposing,
IsControlSyncDisposing, TryEnterControlSync, ExitControlSync, and
DisposeControlSyncLockIfIdle to the private-member area above DisposeAsync,
keeping DisposeAsync at the end of the component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b270a34e-2b37-436a-b66c-fc5f57f61b26
📒 Files selected for processing (5)
src/Spillgebees.Blazor.Map.Assets/src/engine/follow.test.tssrc/Spillgebees.Blazor.Map.Assets/src/engine/follow.tssrc/Spillgebees.Blazor.Map.Docs/Samples/CameraFollow/CameraFollowExample.razor.cssrc/Spillgebees.Blazor.Map.Tests/Components/SgbMapOverlayContentTests.cssrc/Spillgebees.Blazor.Map/Components/SgbMap.razor.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Spillgebees.Blazor.Map.Assets/src/engine/follow.ts
- src/Spillgebees.Blazor.Map.Docs/Samples/CameraFollow/CameraFollowExample.razor.cs
- src/Spillgebees.Blazor.Map.Assets/src/engine/follow.test.ts
1 participant
Allow setting a tracking animation for the camera follow mode on top of the engage animation. This allows for smooth camera tracking when entities jump from position to position.
Summary by CodeRabbit
New Features
Bug Fixes
Tests