fix(ci): pin submodule to SDK v0.6.0, enable submodule checkout, format#6
Conversation
Unblocks the three broken status checks on `main`: * **Build (CS0246)**: `lib/dotnet-sdk` gitlink now pins to `v0.6.0` (`63bf1cb`), which contains `ResQ.Simulation.Engine` and `ResQ.Mavlink*` — the prior pin was an orphan commit unreachable on the SDK remote. * **CI checkout**: bump reusable workflow to `resq-software/.github@109c36b5` (PR #18) so `actions/checkout` can take `submodules: recursive`, wired into both `dotnet-ci.yml` and the CodeQL job in `security-scan.yml`. * **Format check**: apply `dotnet format` repo-wide (10 source/test files). Aligned-column initializers became one-entry-per-line to satisfy `dotnet format --verify-no-changes --severity warn`. * **Build (Vite.AspNetCore 2.x)**: Dependabot auto-merged the 1.12.0 → 2.4.1 bump (#3), which dropped the `Vite.AspNetCore.Extensions` namespace. Use `using Vite.AspNetCore` to match the v2.x API. Local verification: `dotnet build -c Release` green, 82/82 tests pass, `dotnet format --verify-no-changes` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request applies formatting and whitespace normalization across multiple source files and test files, updates a GitHub Actions workflow reference, and bumps a dependency submodule commit hash. The functional logic and control flow remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily consists of widespread formatting cleanup, specifically removing extra whitespace in assignments and switch expressions across the codebase. It also includes a subproject update and a namespace adjustment for Vite. Feedback highlights several technical concerns: a thread-safety issue with _simTime access, an architectural suggestion to use Dependency Injection for complex services, and potential null reference exceptions in SimulationService and VizFrameBuilder.
| // Build and broadcast frame outside the lock to avoid holding it during async I/O. | ||
| var snapshot = GetSnapshot(); | ||
| var frame = _frameBuilder.Build(snapshot, _simTime); | ||
| var frame = _frameBuilder.Build(snapshot, _simTime); |
There was a problem hiding this comment.
The variable _simTime is accessed here without a lock, but it is modified under a lock in the background loop (line 215) and in the Reset method (line 165). Since double operations are not guaranteed to be atomic in C#, this can lead to torn reads or inconsistent values when the simulation is reset or updated from another thread. It is safer to capture the current simulation time while holding the lock in the main loop.
| _terrain = new TerrainNoiseService(); | ||
| _weather = new UpdatableWeatherSystem(new WeatherConfig()); | ||
| _world = new SimulationWorld(new SimulationConfig(), _terrain, _weather); | ||
| _swarm = new SwarmController(_terrain); |
There was a problem hiding this comment.
The constructor directly instantiates several complex dependencies (TerrainNoiseService, UpdatableWeatherSystem, SimulationWorld, and SwarmController). This violates the Dependency Inversion Principle and makes the service difficult to unit test in isolation. Consider registering these as services in the dependency injection container and injecting them through the constructor.
| var weatherMode = mode.ToLowerInvariant() switch | ||
| { | ||
| "steady" => WeatherMode.Steady, | ||
| "steady" => WeatherMode.Steady, |
There was a problem hiding this comment.
The mode parameter is used with ToLowerInvariant() on line 125 (context). If mode is null (which can happen if it is missing from the API request body), this will throw a NullReferenceException. Consider adding a null check or using a null-safe approach before calling ToLowerInvariant().
| Radius: h.Radius, | ||
| Id: h.Id, | ||
| Type: h.Type, | ||
| Center: h.Center.Length == 3 ? [h.Center[0], h.Center[1], h.Center[2]] : [0f, 0f, 0f], |
There was a problem hiding this comment.
If h.Center is null (which can happen if the configuration binder encounters a null value in the JSON), accessing h.Center.Length will throw a NullReferenceException. Using a pattern match or null-conditional operator would be safer.
Center: h.Center is { Length: 3 } ? [h.Center[0], h.Center[1], h.Center[2]] : [0f, 0f, 0f],The `security.yml` workflow calls `security-scan.yml` directly (separate from the `gates` path), so it needs its own `submodules: recursive` to give CodeQL's autobuild the SDK submodule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five keyboard-bound camera framings for demo work (item #6 of the polish plan): Shift+1 OVERVIEW top-down 45° hero shot of the whole swarm Shift+2 TACTICAL 45° oblique at mesh altitude, sim-game angle Shift+3 COCKPIT FPV follow of the selected drone Shift+4 GROUND operator eye-level (1.8 m), looking up Shift+5 INVESTOR toggles the 90s scripted dolly (also Ctrl+Shift+R) Swarm-aware: framings compute swarm centroid + extent from the last VizFrame, so the same keybinds work for single drone, swarm-5, or multi-agency-sar without per-scenario tuning. After a preset lands, user input (orbit, zoom, WASD fly) continues from the new pose. Changes: * `client/cameraPresets.ts` (new) — `CameraPresets` class with the five framings and a shared swarm-bounds helper. * `client/cameraControl.ts` — adds public `UnityCamera.setPose(pos, target)`. Releases scripted and follow modes; resyncs orbit yaw/pitch/distance from the new quaternion so subsequent user input feels continuous. * `client/app.ts` — instantiate + bind `Shift+Digit1..5` before the existing switch; checks shiftKey first so it can't fire the wrong handler. * `client/controls.ts` — early-return on shifted digits so Shift+1 doesn't also run the `single` scenario. Verified: tsc clean, build/format green, 82 / 82 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five keyboard-bound camera framings for demo work (item #6 of the polish plan): Shift+1 OVERVIEW top-down 45° hero shot of the whole swarm Shift+2 TACTICAL 45° oblique at mesh altitude, sim-game angle Shift+3 COCKPIT FPV follow of the selected drone Shift+4 GROUND operator eye-level (1.8 m), looking up Shift+5 INVESTOR toggles the 90s scripted dolly (also Ctrl+Shift+R) Swarm-aware: framings compute swarm centroid + extent from the last VizFrame, so the same keybinds work for single drone, swarm-5, or multi-agency-sar without per-scenario tuning. After a preset lands, user input (orbit, zoom, WASD fly) continues from the new pose. Changes: * `client/cameraPresets.ts` (new) — `CameraPresets` class with the five framings and a shared swarm-bounds helper. * `client/cameraControl.ts` — adds public `UnityCamera.setPose(pos, target)`. Releases scripted and follow modes; resyncs orbit yaw/pitch/distance from the new quaternion so subsequent user input feels continuous. * `client/app.ts` — instantiate + bind `Shift+Digit1..5` before the existing switch; checks shiftKey first so it can't fire the wrong handler. * `client/controls.ts` — early-return on shifted digits so Shift+1 doesn't also run the `single` scenario. Verified: tsc clean, build/format green, 82 / 82 tests passing. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Unblocks the three broken status checks on
main— all pre-existed PR #5 and are unrelated to it.What was broken
gates / .NET CI / Build(FAIL) — viz's.csprojreferences SDK types (SimulatedDrone,WeatherSystem,SimulationWorld,ITerrain,IWeatherSystem,SurfaceType,WeatherConfig) and projects (ResQ.Simulation.Engine,ResQ.Mavlink*) that don't exist at the current submodule pin5f80948…— an orphan commit not reachable on dotnet-sdk remote.gates / .NET CI / Format check(FAIL) — 10 files faileddotnet format --verify-no-changes(aligned-column dict initializers + parameter alignment).scan / CodeQL (csharp)+gates / Security / CodeQL (csharp)(FAIL) — CodeQL's autobuild cascaded from docs(AGENTS): add git-hooks contract + dotnet pre-push #1.gates / .NET CI / Build(latent) — Dependabot auto-mergedVite.AspNetCore1.12.0 → 2.4.1 (Bump the nuget group with 2 updates #3) which is a major-version bump that removed theVite.AspNetCore.Extensionsnamespace.resq-software/.github's reusable workflow didn't take asubmodulesinput.What this PR changes
lib/dotnet-sdkgitlink5f80948…(orphan) →63bf1cb…(SDKv0.6.0).github/workflows/ci.ymlresq-software/.github@109c36b5(PR #18); addsubmodules: recursivesrc/ResQ.Viz.Web/Program.csVite.AspNetCore.Extensions→Vite.AspNetCore(v2.x API)dotnet formatapplied; aligned-column initializers broken out one-per-lineLocal verification
Notes
v0.6.0is the last tagged release before dotnet-sdk'schore: sync from monorepo @3b483ebwhich removed the simulation/mavlink projects in favor of a slimmerResQ.SimulationAPI. Once viz is ready to migrate to that API, the submodule can be bumped forward.chore/license-headers-and-hooks) rebases cleanly on this.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Style
Chores