refactor(sp2): tier-1 architectural debt — supply chain, types, UX, renderer fallback - #7
Merged
Merged
Conversation
…ccess Three cleanups that close long-standing supply-chain and type-safety gaps in one pass. cargo deny lands a license allowlist (MIT, Apache-2.0, BSD-3-Clause, ISC, Unicode-3.0, Zlib) plus advisory + multiple-version + wildcard policies. gravitas-wasm gained a license declaration matching its sibling crate. The path dep on gravitas-core got a version pin to satisfy the wildcard policy without a workspace special-case. Result: all four checks (advisories, bans, licenses, sources) pass; copyleft contamination through transitive deps now fails loud. Clippy pedantic + nursery turned on at the gravitas-core crate root. Allow list carries justifications inline: physics formulas keep the quotient-rule grouping (suspicious_operation_groupings flagged the metric derivative quotient as a false positive); single-letter GR indices stay; published constants keep their canonical form. Five genuine improvements landed: ISCO uses cbrt instead of powf(1/3); Planck law uses exp_m1 for small argument accuracy; shadow tests use hypot instead of (x*x + y*y).sqrt; Christoffel finite-difference loops use iter_mut.enumerate; Butcher-tableau accumulators carry a local too_many_arguments allow with a one-line note about why struct-grouping the (k_i, s_i) pairs hides the integrator structure. noUncheckedIndexedAccess on. The flag exposed 95 sites across 20 files. None was a real out-of-bounds bug; all were untyped invariants the compiler could not see (a length-2 then arr[1], iteration over a known-length array, touch-event indexing). Remediation prefers destructuring with explicit length guards and ?? defaults at the read site rather than non-null assertions, so reviewers see the proof at the point of use. FFI exception mapping on integrate_ray_relativistic. The function silently returned the malformed input on a length mismatch; now it returns Result<Vec<f64>, JsValue> with a specific error message and also rejects non-positive or non-finite tolerance.
Three small UX hardening pieces that close the most user-visible gaps in the operator surface. prefers-reduced-motion gates the cinematic auto-orbit entry. WCAG 2.2 SC 2.3.3 requires non-essential motion to be opt-out; the auto-orbit camera is decorative and now silently no-ops when the media query matches. Implementation goes in startCinematic itself rather than a component wrapper so every entry path (button, keyboard shortcut, URL param) is covered without manual wiring. useReducedMotion hook reads the same media query reactively for any component that wants to disable Framer animations or skip a transition. SSR-safe: returns false on the server render and reconciles on mount. useSimulationMode hook lands a three-state machine (interactive / cinematic / transitioning) with 500ms transition windows. The hook is parallel to the existing isCinematic boolean for now; later code can migrate, but the new surface is what new features should consume because it eliminates the both-controls-active race the old boolean admits. Telemetry numeric values now pad to fixed character counts so the horizontal columns stop jittering when integer-part width changes (horizon 9.50 -> 12.00 used to shift the redshift column).
…static page Five renderer-hardening fixes that close the fallback chain for browsers and adapters that don't expose the production feature set. WebGPU rgba16float now goes through selectColorFormat(adapter), which checks float32-filterable before committing to the HDR format. Adapters without the feature get rgba8unorm with a console warning rather than a silent attachment failure that leaves the canvas blank. The choice is captured once at init in this.hdrFormat and reused at every texture creation. WebGL2 EXT_color_buffer_float now records the extension's availability on the renderer (hasFloatFramebuffer). Safari 16 and below return null silently; downstream framebuffer paths can branch on this instead of attaching a half-float buffer to a context that won't render to it. shader-cache.ts caches the last-good GPUShaderModule per pipeline ID and reuses it when a subsequent compileWithFallback call surfaces errors. The user-visible failure mode shifts from a frozen canvas to a HUD warning plus the previous render output. Pipelines wire it in incrementally; this PR lands the helper, callsites follow. Adaptive resolution moves to an explicit hysteresis band: down-shift trigger is now FPS <= 55 (was < 60), up-shift is FPS >= 75; the 56-74 zone is a dead band where neither timer accumulates and brief drift out of the band only bleeds counters down rather than hard-resetting. This eliminates the 60-FPS-boundary oscillation that the prior single-threshold design produced. The matching property test updates its FPS range to match the new threshold. StaticFallback component lands as the terminal node in the WebGPU -> WebGL2 -> static fallback chain. Renders a plain explanatory page with browser-suggestion links instead of a blank canvas when the user agent exposes neither GPU API.
📝 WalkthroughWalkthroughThis pull request introduces stricter Rust dependency management via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b7207c9e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.device.createTexture({ | ||
| size: [width, height, 1], | ||
| format: "rgba16float", | ||
| format: this.hdrFormat, |
There was a problem hiding this comment.
When selectColorFormat falls back hdrFormat to "rgba8unorm", these history textures are still bound as ATAA output storage textures, but the ATAA shader declares binding 4 as texture_storage_2d<rgba16float, write> (src/shaders/postprocess/ataa.wgsl.ts). On adapters without float32-filterable, this creates a format mismatch at bind/pipeline validation time and breaks the WebGPU render path exactly in the fallback scenario this change is trying to support.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
src/components/fallback/StaticFallback.tsx (1)
20-35: Add keyboard-visible focus styles on support links.Line 20 and Line 29 currently style hover state only. Add
focus-visiblestyles so keyboard users get clear navigation feedback.♿ Suggested tweak
<a - className="underline hover:text-white" + className="underline hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-black" href="https://www.google.com/chrome/" rel="noopener noreferrer" target="_blank" ><a - className="underline hover:text-white" + className="underline hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-black" href="https://www.mozilla.org/firefox/" rel="noopener noreferrer" target="_blank" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/fallback/StaticFallback.tsx` around lines 20 - 35, The Chrome/Firefox anchor elements in StaticFallback (the <a> anchors inside the StaticFallback component) only define hover styles; add keyboard-visible focus styles by updating their className to include appropriate focus-visible utilities (e.g., focus-visible:outline-none + focus-visible:ring or focus-visible:underline and focus-visible:ring-offset to match the hover feedback) so keyboard users see a clear ring/underline when tabbing to the links; ensure both anchor elements (the ones linking to chrome and firefox) get the same focus-visible classes for consistent keyboard navigation feedback.physics-engine/gravitas-core/src/physics/spectrum.rs (1)
35-38: Optional: make step count integer-defined to avoid float rounding sensitivity.At Line 35,
round() as usizeis fine for current constants, but using integer nm iteration avoids future off-by-one surprises if the range/step changes.Optional refactor sketch
- let start: f64 = 380.0e-9; - let end: f64 = 780.0e-9; - let step: f64 = 2.0e-9; - let n_steps = ((end - start) / step).round() as usize; - - for i in 0..=n_steps { - let lambda = step.mul_add(i as f64, start); + const START_NM: usize = 380; + const END_NM: usize = 780; + const STEP_NM: usize = 2; + let step = STEP_NM as f64 * 1.0e-9; + + for l_nm in (START_NM..=END_NM).step_by(STEP_NM) { + let lambda = l_nm as f64 * 1.0e-9;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@physics-engine/gravitas-core/src/physics/spectrum.rs` around lines 35 - 38, The current n_steps uses floating-point round() which can cause off-by-one when ranges/step change; change the logic that computes n_steps to derive an integer count deterministically (e.g. compute raw_steps = (end - start) / step, take floor/trunc and cast to usize) and then iterate 0..=n_steps computing lambda with step.mul_add(i as f64, start); also guard the final lambda with a check (lambda <= end + f64::EPSILON) or clamp to end to avoid producing a value slightly past end. Update the variables n_steps and lambda in spectrum.rs accordingly.src/rendering/bloom.ts (1)
545-550: Consider eliminating the intermediateviewportarray.While the nullish coalescing operators satisfy
noUncheckedIndexedAccess, they guard against impossibleundefinedvalues sinceviewportis a 4-element array literal (line 461) used only once. This adds unnecessary runtime checks and obscures intent.♻️ Three cleaner alternatives
Option 1 (recommended): Direct call — eliminates allocation and type complexity
- // Save current viewport - // Optimization: Avoid gl.getParameter(gl.VIEWPORT) which causes pipeline stall - // We know the viewport matches our canvas dimensions - const viewport = [0, 0, this.width, this.height]; - // Bind quad buffer gl.bindBuffer(gl.ARRAY_BUFFER, this.quadBuffer); // === PASS 1: Extract bright pixels === ... // === PASS 3: Combine with original scene === gl.bindFramebuffer(gl.FRAMEBUFFER, null); - gl.viewport( - viewport[0] ?? 0, - viewport[1] ?? 0, - viewport[2] ?? 0, - viewport[3] ?? 0, - ); + gl.viewport(0, 0, this.width, this.height);Option 2: Use tuple type for stricter type checking
- const viewport = [0, 0, this.width, this.height]; + const viewport: [number, number, number, number] = [0, 0, this.width, this.height]; // ... - gl.viewport( - viewport[0] ?? 0, - viewport[1] ?? 0, - viewport[2] ?? 0, - viewport[3] ?? 0, - ); + gl.viewport(viewport[0], viewport[1], viewport[2], viewport[3]);Option 3: Use destructuring
- const viewport = [0, 0, this.width, this.height]; + const [x, y, w, h] = [0, 0, this.width, this.height]; // ... - gl.viewport( - viewport[0] ?? 0, - viewport[1] ?? 0, - viewport[2] ?? 0, - viewport[3] ?? 0, - ); + gl.viewport(x, y, w, h);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rendering/bloom.ts` around lines 545 - 550, The viewport array allocation and per-index nullish checks are unnecessary; replace the gl.viewport call that uses the local viewport array with a direct call passing the four numeric values (e.g., gl.viewport(0, 0, canvasWidth, canvasHeight) or the four literals used when creating the viewport) to remove the intermediate viewport variable and the nullish coalescing, or alternatively destructure the viewport tuple (const [x,y,w,h] = viewport; gl.viewport(x,y,w,h)) — update the call site referencing gl.viewport and remove the redundant viewport array creation so there are no runtime ?? checks around guaranteed elements.physics-engine/gravitas-core/src/lib.rs (1)
5-42: Consider narrowing the broadest#![allow(...)]lints over time.Good move enabling
pedantic/nursery; as a follow-up, scoping policy-heavy allows (e.g., doc/must-use related) closer to modules/functions would preserve stronger lint signal for new code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@physics-engine/gravitas-core/src/lib.rs` around lines 5 - 42, The crate currently applies a very broad set of Clippy allows at the top level via the crate attribute #![allow(...)] which silences useful lints globally (e.g., clippy::doc_markdown, clippy::missing_errors_doc, clippy::must_use_candidate, clippy::float_cmp); refine this by removing the most policy-heavy allows from the crate root and instead apply them narrowly where required (for example add per-module or per-function attributes like #[allow(clippy::doc_markdown)] or #[allow(clippy::must_use_candidate)] immediately above metric::kerr, geodesic::geodesic, or specific functions that intentionally violate the lint), keep only universally justified allows at the crate level, and add a short TODO comment to each narrowed allow explaining why it is necessary so future PRs can incrementally tighten lint coverage.src/performance/benchmark.ts (1)
159-161: Fail fast on preset-index invariant violations.Line 160 and Line 243 currently return silently. If this invariant is ever broken, benchmark flow can degrade quietly. Consider cancelling/finishing explicitly so callers get deterministic behavior.
Suggested fail-safe pattern
const currentPreset = this.PRESETS_TO_TEST[this.currentPresetIndex]; - if (!currentPreset) return null; + if (!currentPreset) { + this.state = "cancelled"; + this.reset(); + return null; + } ... const presetName = this.PRESETS_TO_TEST[this.currentPresetIndex]; - if (!presetName) return; + if (!presetName) { + this.state = "cancelled"; + this.reset(); + return; + }Also applies to: 242-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/performance/benchmark.ts` around lines 159 - 161, The silent returns when the preset invariant fails (checking this.PRESETS_TO_TEST[this.currentPresetIndex]) should be replaced with an explicit fail-fast action: detect the missing currentPreset and either throw a clear error or call the benchmark cancellation/finish routine so callers get deterministic behavior; update the checks around PRESETS_TO_TEST and currentPresetIndex (the lines referencing currentPreset) to invoke the existing cancel/finish method (or throw a descriptive Error like "Invalid preset index: currentPresetIndex") instead of returning null so the benchmark flow cannot continue silently.src/hooks/useCamera.ts (1)
658-666: Consider reusinguseReducedMotionhere to avoid duplicated preference logic.This gate is correct, but the media-query logic now exists in two places (
useCameraanduseReducedMotion). Consolidating to one source keeps behavior aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCamera.ts` around lines 658 - 666, The media-query check in useCamera duplicates logic in useReducedMotion; replace the direct window.matchMedia block with the shared hook by importing and calling useReducedMotion (or the exported selector) and early-return when it indicates reduced motion, preserving the current early-exit behavior for cinematic auto-orbit; ensure the import name matches the existing export from useReducedMotion and remove the window.matchMedia branch to avoid duplicate preference logic.src/rendering/adaptive-resolution.ts (1)
104-105: UsetargetFPSto derive hysteresis thresholds instead of hardcoding them.Line 104 and Line 105 hardcode values that bypass
ResolutionConfig.targetFPS, which makes configuration partially misleading.Proposed refactor
- const DOWN_FPS = 55; - const UP_FPS = 75; + const DOWN_FPS = Math.max(1, this.config.targetFPS - 5); + const UP_FPS = Math.max(DOWN_FPS + 1, this.config.targetFPS + 15);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rendering/adaptive-resolution.ts` around lines 104 - 105, The hardcoded DOWN_FPS and UP_FPS bypass ResolutionConfig.targetFPS; replace those constants with values computed from the configured targetFPS (ResolutionConfig.targetFPS) so hysteresis follows config. For example, compute DOWN_FPS and UP_FPS from targetFPS using clear multipliers (e.g., DOWN_FPS = Math.floor(targetFPS * 0.9) and UP_FPS = Math.ceil(targetFPS * 1.1) or similar), and use those computed variables everywhere the code currently references the hardcoded DOWN_FPS/UP_FPS identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/Telemetry.tsx`:
- Around line 20-27: The fixedWidth function can return strings longer than
totalWidth because padStart doesn't truncate; update fixedWidth to build the
formatted string (keep sign + absolute value toFixed(decimals)) and if
formatted.length > totalWidth then truncate deterministically (e.g. take the
rightmost totalWidth characters, preserving least-significant digits and sign
when possible) otherwise use padStart to left‑pad to totalWidth; modify the
implementation of function fixedWidth to perform this length check and
truncation so columns never grow beyond totalWidth.
In `@src/hooks/useSimulationMode.ts`:
- Around line 23-30: The transition timers created in useSimulationMode
(transitionTimer ref and clearPending helper) aren’t cleared on component
unmount, which can lead to stale setTimeout callbacks updating unmounted
components; add a useEffect cleanup in useSimulationMode that returns () =>
clearPending() so transitionTimer.current is cleared when the hook’s consumer
unmounts, referencing the existing transitionTimer ref and clearPending function
to cancel any pending timeouts.
In `@src/performance/gpu-timer.ts`:
- Around line 116-117: The loop that inspects this.pendingQueries stops on an
invalid head entry (the code that does "const query = this.pendingQueries[0]; if
(!query) break;"), which can leave a bad first slot and block later queries;
change the behavior to drop the invalid head and continue processing by removing
the bad entry (e.g., use this.pendingQueries.shift()) and continue the loop so
subsequent queries are handled; update the logic in the method that iterates
pendingQueries (the loop referencing this.pendingQueries[0] and query) to shift
and continue instead of breaking.
In `@src/rendering/shader-cache.ts`:
- Around line 6-7: The global cache Map named "cache" (Map<string,
CachedModule>) can return shader modules compiled for a different GPUDevice
(keyed only by pipelineId); change it to be device-scoped—e.g. use a
WeakMap<GPUDevice, Map<string, CachedModule>> (or Map<GPUDevice,...>) so each
GPUDevice has its own inner Map keyed by pipelineId/CachedModule. Update all
accesses that read/write "cache" (the lookup/fallback logic around pipelineId
and the fallback at line 34) to first select the device-specific map (creating
it if missing) and then get/set by pipelineId so modules are never reused across
devices and garbage collection of device entries is possible.
In `@src/rendering/webgpu/renderer.ts`:
- Around line 346-352: When history textures are destroyed/recreated in
initTextures(), existing ATAA bind groups still reference old texture views and
will fail; update initTextures() (or right after history texture recreation) to
invalidate and rebuild the ATAA bind groups by clearing this.ataaBindGroups
(e.g., set this.ataaBindGroups = [] or recreate bind groups) whenever
historyTextures (hist0/hist1) are recreated and before the code that lazily
creates bind groups (the block checking this.ataaBindGroups.length === 0 and
this.ataaPipeline) runs so new bind groups reference the new textures.
- Around line 191-199: The ATAA shader expects a rgba16float storage texture but
the renderer's selectColorFormat() can fall back to rgba8unorm, causing
bind-group validation errors; fix by making the renderer and shader agree on
storage format: either (A) make the shader format-agnostic/parameterized (update
src/shaders/postprocess/ataa.wgsl.ts to support both float and unorm storage or
accept a compile-time specialization) and compile the shader with the matching
storage format, or (B) in the renderer ensure selectColorFormat() and the
history texture creation (where this.hdrFormat is used in createTexture) choose
a storage-compatible format and compile/create a separate ATAA pipeline when
falling back to rgba8unorm so the shader signature matches the texture format
used at runtime. Ensure the code paths that create the pipeline/ bind-group
(pipeline creation code that uses the ATAA shader) and the texture creation
(this.hdrFormat in createTexture) use the same agreed format.
- Around line 56-61: The selectColorFormat function should stop using
adapter.features.has("float32-filterable") and instead probe actual rgba16float
support by attempting to create a small GPUTexture on a GPUDevice; change the
function signature to accept a GPUDevice, call device.createTexture with size
[1,1,1], format "rgba16float" and usage flags GPUTextureUsage.STORAGE_BINDING |
GPUTextureUsage.TEXTURE_BINDING, destroy the test texture if creation succeeds
and return "rgba16float", and catch failures to return "rgba8unorm" as the safe
fallback (update all callers of selectColorFormat to pass the GPUDevice).
---
Nitpick comments:
In `@physics-engine/gravitas-core/src/lib.rs`:
- Around line 5-42: The crate currently applies a very broad set of Clippy
allows at the top level via the crate attribute #![allow(...)] which silences
useful lints globally (e.g., clippy::doc_markdown, clippy::missing_errors_doc,
clippy::must_use_candidate, clippy::float_cmp); refine this by removing the most
policy-heavy allows from the crate root and instead apply them narrowly where
required (for example add per-module or per-function attributes like
#[allow(clippy::doc_markdown)] or #[allow(clippy::must_use_candidate)]
immediately above metric::kerr, geodesic::geodesic, or specific functions that
intentionally violate the lint), keep only universally justified allows at the
crate level, and add a short TODO comment to each narrowed allow explaining why
it is necessary so future PRs can incrementally tighten lint coverage.
In `@physics-engine/gravitas-core/src/physics/spectrum.rs`:
- Around line 35-38: The current n_steps uses floating-point round() which can
cause off-by-one when ranges/step change; change the logic that computes n_steps
to derive an integer count deterministically (e.g. compute raw_steps = (end -
start) / step, take floor/trunc and cast to usize) and then iterate 0..=n_steps
computing lambda with step.mul_add(i as f64, start); also guard the final lambda
with a check (lambda <= end + f64::EPSILON) or clamp to end to avoid producing a
value slightly past end. Update the variables n_steps and lambda in spectrum.rs
accordingly.
In `@src/components/fallback/StaticFallback.tsx`:
- Around line 20-35: The Chrome/Firefox anchor elements in StaticFallback (the
<a> anchors inside the StaticFallback component) only define hover styles; add
keyboard-visible focus styles by updating their className to include appropriate
focus-visible utilities (e.g., focus-visible:outline-none + focus-visible:ring
or focus-visible:underline and focus-visible:ring-offset to match the hover
feedback) so keyboard users see a clear ring/underline when tabbing to the
links; ensure both anchor elements (the ones linking to chrome and firefox) get
the same focus-visible classes for consistent keyboard navigation feedback.
In `@src/hooks/useCamera.ts`:
- Around line 658-666: The media-query check in useCamera duplicates logic in
useReducedMotion; replace the direct window.matchMedia block with the shared
hook by importing and calling useReducedMotion (or the exported selector) and
early-return when it indicates reduced motion, preserving the current early-exit
behavior for cinematic auto-orbit; ensure the import name matches the existing
export from useReducedMotion and remove the window.matchMedia branch to avoid
duplicate preference logic.
In `@src/performance/benchmark.ts`:
- Around line 159-161: The silent returns when the preset invariant fails
(checking this.PRESETS_TO_TEST[this.currentPresetIndex]) should be replaced with
an explicit fail-fast action: detect the missing currentPreset and either throw
a clear error or call the benchmark cancellation/finish routine so callers get
deterministic behavior; update the checks around PRESETS_TO_TEST and
currentPresetIndex (the lines referencing currentPreset) to invoke the existing
cancel/finish method (or throw a descriptive Error like "Invalid preset index:
currentPresetIndex") instead of returning null so the benchmark flow cannot
continue silently.
In `@src/rendering/adaptive-resolution.ts`:
- Around line 104-105: The hardcoded DOWN_FPS and UP_FPS bypass
ResolutionConfig.targetFPS; replace those constants with values computed from
the configured targetFPS (ResolutionConfig.targetFPS) so hysteresis follows
config. For example, compute DOWN_FPS and UP_FPS from targetFPS using clear
multipliers (e.g., DOWN_FPS = Math.floor(targetFPS * 0.9) and UP_FPS =
Math.ceil(targetFPS * 1.1) or similar), and use those computed variables
everywhere the code currently references the hardcoded DOWN_FPS/UP_FPS
identifiers.
In `@src/rendering/bloom.ts`:
- Around line 545-550: The viewport array allocation and per-index nullish
checks are unnecessary; replace the gl.viewport call that uses the local
viewport array with a direct call passing the four numeric values (e.g.,
gl.viewport(0, 0, canvasWidth, canvasHeight) or the four literals used when
creating the viewport) to remove the intermediate viewport variable and the
nullish coalescing, or alternatively destructure the viewport tuple (const
[x,y,w,h] = viewport; gl.viewport(x,y,w,h)) — update the call site referencing
gl.viewport and remove the redundant viewport array creation so there are no
runtime ?? checks around guaranteed elements.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4111590-0077-47b0-92fd-cacbc8cdaeaf
📒 Files selected for processing (39)
physics-engine/deny.tomlphysics-engine/gravitas-core/src/geodesic/mod.rsphysics-engine/gravitas-core/src/lib.rsphysics-engine/gravitas-core/src/metric/kerr.rsphysics-engine/gravitas-core/src/physics/shadow.rsphysics-engine/gravitas-core/src/physics/spectrum.rsphysics-engine/gravitas-core/src/tensor/christoffel.rsphysics-engine/gravitas-core/src/tensor/metric_tensor.rsphysics-engine/gravitas-core/tests/normalize.rsphysics-engine/gravitas-wasm/Cargo.tomlphysics-engine/gravitas-wasm/src/lib.rsscripts/__tests__/seo-provenance.test.tsscripts/seo-provenance.tssrc/__tests__/integration/adaptive-systems.test.tssrc/__tests__/integration/feature-performance-impact.test.tssrc/__tests__/performance/validation.test.tssrc/__tests__/physics/advanced-physics.test.tssrc/__tests__/rendering/adaptive-resolution.test.tssrc/__tests__/types/features.test.tssrc/components/fallback/StaticFallback.tsxsrc/components/spacetime/EmbeddingDiagram.tsxsrc/components/spacetime/SpacetimeCanvas.tsxsrc/components/ui/Telemetry.tsxsrc/hooks/useAnimation.tssrc/hooks/useCamera.tssrc/hooks/useReducedMotion.tssrc/hooks/useSimulationMode.tssrc/performance/benchmark.tssrc/performance/gpu-timer.tssrc/performance/monitor.tssrc/rendering/adaptive-resolution.tssrc/rendering/bloom.tssrc/rendering/shader-cache.tssrc/rendering/webgl/renderer.tssrc/rendering/webgpu/renderer.tssrc/types/webgpu.tssrc/utils/cpu-optimizations.tssrc/workers/physics.worker.tstsconfig.json
Comment on lines
+20
to
+27
| function fixedWidth( | ||
| value: number, | ||
| totalWidth: number, | ||
| decimals: number, | ||
| ): string { | ||
| const sign = value < 0 ? "-" : " "; | ||
| const body = Math.abs(value).toFixed(decimals); | ||
| return (sign + body).padStart(totalWidth, " "); |
There was a problem hiding this comment.
fixedWidth can still overflow and reintroduce HUD jitter.
On Line 27, padStart does not constrain overlong values. If magnitude grows past the allocated width, this returns longer strings and columns shift again.
Proposed hardening
function fixedWidth(
value: number,
totalWidth: number,
decimals: number,
): string {
+ if (!Number.isFinite(value)) {
+ return "---".padStart(totalWidth, " ");
+ }
const sign = value < 0 ? "-" : " ";
const body = Math.abs(value).toFixed(decimals);
- return (sign + body).padStart(totalWidth, " ");
+ const formatted = (sign + body).padStart(totalWidth, " ");
+ if (formatted.length <= totalWidth) return formatted;
+ // Fallback to compact scientific notation when width is exceeded.
+ return value
+ .toExponential(Math.max(0, decimals - 1))
+ .slice(0, totalWidth)
+ .padStart(totalWidth, " ");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/Telemetry.tsx` around lines 20 - 27, The fixedWidth
function can return strings longer than totalWidth because padStart doesn't
truncate; update fixedWidth to build the formatted string (keep sign + absolute
value toFixed(decimals)) and if formatted.length > totalWidth then truncate
deterministically (e.g. take the rightmost totalWidth characters, preserving
least-significant digits and sign when possible) otherwise use padStart to
left‑pad to totalWidth; modify the implementation of function fixedWidth to
perform this length check and truncation so columns never grow beyond
totalWidth.
Comment on lines
+23
to
+30
| const transitionTimer = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| const clearPending = () => { | ||
| if (transitionTimer.current) { | ||
| clearTimeout(transitionTimer.current); | ||
| transitionTimer.current = null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/hooks/useSimulationMode.tsRepository: steeltroops-ai/blackhole-simulation
Length of output: 2586
Clear transition timers on unmount to prevent stale timeout callbacks.
The setTimeout callbacks at lines 38 and 51 will execute even if the component unmounts before they fire, triggering state updates on an unmounted component. Add a useEffect cleanup to clear the timer on unmount:
useEffect(() => {
return () => clearPending();
}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useSimulationMode.ts` around lines 23 - 30, The transition timers
created in useSimulationMode (transitionTimer ref and clearPending helper)
aren’t cleared on component unmount, which can lead to stale setTimeout
callbacks updating unmounted components; add a useEffect cleanup in
useSimulationMode that returns () => clearPending() so transitionTimer.current
is cleared when the hook’s consumer unmounts, referencing the existing
transitionTimer ref and clearPending function to cancel any pending timeouts.
Comment on lines
116
to
+117
| const query = this.pendingQueries[0]; | ||
| if (!query) break; |
There was a problem hiding this comment.
Drop invalid head entries instead of stopping collection.
On Line 117, break can leave a bad first slot in place and block all later query processing indefinitely. Prefer shifting it out and continuing.
Suggested fix
- if (!query) break;
+ if (!query) {
+ this.pendingQueries.shift();
+ continue;
+ }📝 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
| const query = this.pendingQueries[0]; | |
| if (!query) break; | |
| const query = this.pendingQueries[0]; | |
| if (!query) { | |
| this.pendingQueries.shift(); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/performance/gpu-timer.ts` around lines 116 - 117, The loop that inspects
this.pendingQueries stops on an invalid head entry (the code that does "const
query = this.pendingQueries[0]; if (!query) break;"), which can leave a bad
first slot and block later queries; change the behavior to drop the invalid head
and continue processing by removing the bad entry (e.g., use
this.pendingQueries.shift()) and continue the loop so subsequent queries are
handled; update the logic in the method that iterates pendingQueries (the loop
referencing this.pendingQueries[0] and query) to shift and continue instead of
breaking.
Comment on lines
+6
to
+7
| const cache = new Map<string, CachedModule>(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In WebGPU, can a GPUShaderModule created from one GPUDevice be used to create pipelines on a different GPUDevice?
💡 Result:
No, a GPUShaderModule created from one GPUDevice cannot be used to create pipelines on a different GPUDevice.
Citations:
- 1: How to better render to multiple canvases? webgpu/webgpufundamentals#73
- 2: https://developer.mozilla.org/en-US/docs/Web/API/GPUShaderModule
- 3: https://developer.mozilla.org/en-US/docs/Web/API/GPUDevice/createShaderModule
- 4: https://developer.mozilla.org/docs/Web/API/GPUDevice/createComputePipeline
- 5: http://developer.mozilla.org/en-US/docs/Web/API/GPURenderPipeline
🏁 Script executed:
cat -n src/rendering/shader-cache.ts | head -50Repository: steeltroops-ai/blackhole-simulation
Length of output: 1965
Scope cache entries per GPUDevice to prevent invalid shader-module reuse.
The cache is keyed only by pipelineId (line 6), so a fallback at line 34 can return a shader module compiled on a different GPUDevice—which violates WebGPU's device-scoped resource ownership and will fail after device recreation or loss.
Use a device-scoped cache structure instead:
🔧 Proposed fix
interface CachedModule {
module: GPUShaderModule;
compiledAt: number;
}
-const cache = new Map<string, CachedModule>();
+const cache = new WeakMap<GPUDevice, Map<string, CachedModule>>();
+
+function getDeviceCache(device: GPUDevice): Map<string, CachedModule> {
+ let deviceCache = cache.get(device);
+ if (!deviceCache) {
+ deviceCache = new Map<string, CachedModule>();
+ cache.set(device, deviceCache);
+ }
+ return deviceCache;
+}
@@
export async function compileWithFallback(
device: GPUDevice,
pipelineId: string,
source: string,
hudWarn: HudWarn,
): Promise<GPUShaderModule> {
+ const deviceCache = getDeviceCache(device);
const candidate = device.createShaderModule({ code: source });
const info = await candidate.getCompilationInfo();
const errors = info.messages.filter((m) => m.type === "error");
if (errors.length === 0) {
- cache.set(pipelineId, { module: candidate, compiledAt: Date.now() });
+ deviceCache.set(pipelineId, { module: candidate, compiledAt: Date.now() });
return candidate;
}
@@
- const lastGood = cache.get(pipelineId);
+ const lastGood = deviceCache.get(pipelineId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rendering/shader-cache.ts` around lines 6 - 7, The global cache Map named
"cache" (Map<string, CachedModule>) can return shader modules compiled for a
different GPUDevice (keyed only by pipelineId); change it to be
device-scoped—e.g. use a WeakMap<GPUDevice, Map<string, CachedModule>> (or
Map<GPUDevice,...>) so each GPUDevice has its own inner Map keyed by
pipelineId/CachedModule. Update all accesses that read/write "cache" (the
lookup/fallback logic around pipelineId and the fallback at line 34) to first
select the device-specific map (creating it if missing) and then get/set by
pipelineId so modules are never reused across devices and garbage collection of
device entries is possible.
Comment on lines
+56
to
+61
| function selectColorFormat(adapter: GPUAdapter): GPUTextureFormat { | ||
| // float32-filterable implies rgba16float renderable on every adapter that | ||
| // exposes it. Adapters without it can still render to rgba16float in | ||
| // practice, but the safe assumption is to downgrade. | ||
| if (adapter.features.has("float32-filterable")) { | ||
| return "rgba16float"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In WebGPU, does adapter.features.has("float32-filterable") guarantee support for creating rgba16float textures with STORAGE_BINDING | TEXTURE_BINDING, and what is the recommended way to probe rgba16float compatibility?
💡 Result:
No, adapter.features.has("float32-filterable") does not guarantee support for creating rgba16float textures with STORAGE_BINDING | TEXTURE_BINDING. The "float32-filterable" feature specifically enables linear filtering (beyond nearest) for 32-bit float formats like rgba32float, but rgba16float is a core format with baseline support for TEXTURE_BINDING on all WebGPU devices. STORAGE_BINDING for rgba16float is also core-supported (read-only and write-only access), as listed in the WebGPU specification's plain color formats table and confirmed in implementation details. The recommended way to probe rgba16float compatibility (or any texture format/usages) is to attempt creating the texture with device.createTexture using the desired format and usages (e.g., GPUTextureUsage.STORAGE_BINDING | GPUTextureUsage.TEXTURE_BINDING). If unsupported, it throws a GPUValidationError (per spec validation rules in createTexture). Catch the error to detect lack of support. No adapter-level probe exists; runtime validation via creation attempt is standard. Example code: try { const texture = device.createTexture({ size: [1, 1], format: 'rgba16float', usage: GPUTextureUsage.STORAGE_BINDING | GPUTextureUsage.TEXTURE_BINDING }); // Supported } catch (error) { if (error instanceof GPUValidationError) { // Not supported } }
Citations:
- 1: https://webgpufundamentals.org/webgpu/lessons/webgpu-limits-and-features.html
- 2: https://webgpufundamentals.org/webgpu/lessons/webgpu-textures.html
- 3: https://webgpu.rocks/reference/dictionary/gpustoragetexturebindinglayout/
- 4: Texture Format RGBA32Float Does Not Support Storage Texture Access StorageTextureAccess::ReadWrite gpuweb/gpuweb#4650
- 5: Compat: Validate out storage texture formats that are not supported in GLES gpuweb/gpuweb#4413
- 6: https://developer.mozilla.org/en-US/docs/Web/API/GPUDevice/createTexture
- 7: https://docs.w3cub.com/dom/gpudevice/createtexture
- 8: https://github.com/greggman/webgpu-feature-detector
🏁 Script executed:
sed -n '56,100p' src/rendering/webgpu/renderer.tsRepository: steeltroops-ai/blackhole-simulation
Length of output: 1664
🏁 Script executed:
rg -i "rgba16float|float32-filterable" --type ts --type js -B 2 -A 2Repository: steeltroops-ai/blackhole-simulation
Length of output: 3019
Replace the float32-filterable check with a direct texture format-compatibility probe.
The code incorrectly assumes float32-filterable indicates rgba16float support. Per WebGPU specification, rgba16float is a core format guaranteed on all devices; float32-filterable only controls linear filtering for 32-bit float formats and is unrelated to rgba16float availability. Since shaders hardcode rgba16float (e.g., texture_storage_2d<rgba16float, write> in compute.wgsl and ataa.wgsl), using this feature as a fallback trigger causes unnecessary quality degradation and potential texture format mismatch errors on adapters without the feature.
Replace with a direct texture creation probe:
Suggested approach
function selectColorFormat(device: GPUDevice): GPUTextureFormat {
try {
const testTexture = device.createTexture({
size: [1, 1, 1],
format: "rgba16float",
usage: GPUTextureUsage.STORAGE_BINDING | GPUTextureUsage.TEXTURE_BINDING,
});
testTexture.destroy();
return "rgba16float";
} catch {
return "rgba8unorm";
}
}Note: This requires passing device instead of adapter to the function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rendering/webgpu/renderer.ts` around lines 56 - 61, The selectColorFormat
function should stop using adapter.features.has("float32-filterable") and
instead probe actual rgba16float support by attempting to create a small
GPUTexture on a GPUDevice; change the function signature to accept a GPUDevice,
call device.createTexture with size [1,1,1], format "rgba16float" and usage
flags GPUTextureUsage.STORAGE_BINDING | GPUTextureUsage.TEXTURE_BINDING, destroy
the test texture if creation succeeds and return "rgba16float", and catch
failures to return "rgba8unorm" as the safe fallback (update all callers of
selectColorFormat to pass the GPUDevice).
Comment on lines
+191
to
199
| format: this.hdrFormat, | ||
| usage: | ||
| GPUTextureUsage.STORAGE_BINDING | GPUTextureUsage.TEXTURE_BINDING, | ||
| }), | ||
| this.device.createTexture({ | ||
| size: [width, height, 1], | ||
| format: "rgba16float", | ||
| format: this.hdrFormat, | ||
| usage: | ||
| GPUTextureUsage.STORAGE_BINDING | GPUTextureUsage.TEXTURE_BINDING, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ATAA shader storage texture format declarations and renderer texture formats.
set -euo pipefail
echo "== ATAA WGSL storage texture declarations =="
rg -n --no-heading 'texture_storage_2d<|@binding\\(' src/shaders/postprocess/ataa.wgsl
echo
echo "== Renderer texture format selection and usage =="
rg -n --no-heading 'selectColorFormat|hdrFormat|createTexture\\(|format:' src/rendering/webgpu/renderer.tsRepository: steeltroops-ai/blackhole-simulation
Length of output: 255
🏁 Script executed:
#!/bin/bash
# Verify ATAA shader storage texture format declarations and renderer texture formats (fixed regex)
set -euo pipefail
echo "== ATAA WGSL storage texture declarations =="
rg 'texture_storage_2d' src/shaders/postprocess/ataa.wgsl -A 2 || echo "No matches found"
echo ""
echo "== Renderer hdrFormat definition and usage =="
rg 'hdrFormat|selectColorFormat' src/rendering/webgpu/renderer.ts -B 2 -A 2 || echo "No matches found"
echo ""
echo "== Renderer createTexture calls with format =="
rg 'createTexture\(' src/rendering/webgpu/renderer.ts -A 4 || echo "No matches found"
echo ""
echo "== Check if ataa.wgsl exists and its size =="
ls -lh src/shaders/postprocess/ataa.wgsl 2>/dev/null || echo "File not found"Repository: steeltroops-ai/blackhole-simulation
Length of output: 2023
🏁 Script executed:
#!/bin/bash
# Search for shader files and understand the format selection logic
set -euo pipefail
echo "== All WGSL shader files in project =="
find . -name "*.wgsl" -type f 2>/dev/null | head -20
echo ""
echo "== Postprocess shader directory =="
ls -la src/shaders/postprocess/ 2>/dev/null || echo "Directory not found"
echo ""
echo "== Full selectColorFormat function =="
rg 'function selectColorFormat' src/rendering/webgpu/renderer.ts -A 15 || echo "Not found"
echo ""
echo "== Search for rgba8unorm in entire codebase =="
rg 'rgba8unorm' . --type ts --type wgsl 2>/dev/null | head -20 || echo "No rgba8unorm found"
echo ""
echo "== Search for history texture or ATAA references =="
rg -i 'history.*texture|ataa' src/rendering/webgpu/renderer.ts --type ts -B 2 -A 2 || echo "No matches"Repository: steeltroops-ai/blackhole-simulation
Length of output: 3840
🏁 Script executed:
#!/bin/bash
# Read the actual ATAA shader file (TypeScript wrapped WGSL)
cat -n src/shaders/postprocess/ataa.wgsl.ts | head -100Repository: steeltroops-ai/blackhole-simulation
Length of output: 3899
Fix storage texture format compatibility between ATAA shader and renderer fallback path.
The ATAA shader (src/shaders/postprocess/ataa.wgsl.ts, line 9) hardcodes texture_storage_2d<rgba16float, write> for the output storage texture. However, the renderer's selectColorFormat() function falls back to rgba8unorm on devices that don't support the float32-filterable feature. When history textures are created with this fallback format but the shader expects rgba16float, bind-group layout validation will fail at runtime.
Either make the shader format-agnostic by using a parameterized storage format, or update the renderer to create separate pipelines/shaders for the rgba8unorm fallback path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rendering/webgpu/renderer.ts` around lines 191 - 199, The ATAA shader
expects a rgba16float storage texture but the renderer's selectColorFormat() can
fall back to rgba8unorm, causing bind-group validation errors; fix by making the
renderer and shader agree on storage format: either (A) make the shader
format-agnostic/parameterized (update src/shaders/postprocess/ataa.wgsl.ts to
support both float and unorm storage or accept a compile-time specialization)
and compile the shader with the matching storage format, or (B) in the renderer
ensure selectColorFormat() and the history texture creation (where
this.hdrFormat is used in createTexture) choose a storage-compatible format and
compile/create a separate ATAA pipeline when falling back to rgba8unorm so the
shader signature matches the texture format used at runtime. Ensure the code
paths that create the pipeline/ bind-group (pipeline creation code that uses the
ATAA shader) and the texture creation (this.hdrFormat in createTexture) use the
same agreed format.
Comment on lines
+346
to
+352
| const [hist0, hist1] = this.historyTextures; | ||
| if ( | ||
| this.ataaBindGroups.length === 0 && | ||
| this.ataaPipeline && | ||
| hist0 && | ||
| hist1 | ||
| ) { |
There was a problem hiding this comment.
Invalidate ATAA bind groups when textures are recreated.
this.ataaBindGroups is only created when length === 0, but initTextures() destroys/recreates history textures. After a resize, existing bind groups can still point to destroyed views and fail at runtime.
💡 Proposed fix
private initTextures(width: number, height: number) {
if (!this.device) return;
// Destroy old textures if they exist
[this.computeTexture, ...this.historyTextures].forEach((tex) => {
if (tex) tex.destroy();
});
+ // Force ATAA bind groups to be rebuilt against fresh texture views.
+ this.ataaBindGroups = [];
+ this.currentHistoryIndex = 0; public resize(width: number, height: number) {
if (this.width !== width || this.height !== height) {
this.width = width;
this.height = height;
this.initTextures(width, height);
// Invalidate bind groups to force recreation
this.computeBindGroup = null;
this.renderBindGroup = null;
+ this.ataaBindGroups = [];
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rendering/webgpu/renderer.ts` around lines 346 - 352, When history
textures are destroyed/recreated in initTextures(), existing ATAA bind groups
still reference old texture views and will fail; update initTextures() (or right
after history texture recreation) to invalidate and rebuild the ATAA bind groups
by clearing this.ataaBindGroups (e.g., set this.ataaBindGroups = [] or recreate
bind groups) whenever historyTextures (hist0/hist1) are recreated and before the
code that lazily creates bind groups (the block checking
this.ataaBindGroups.length === 0 and this.ataaPipeline) runs so new bind groups
reference the new textures.
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.
Three commits closing four long-standing audit findings under a single tier-1
debt umbrella. Each commit is independently reviewable and revertable.
Group 2B (chore): cargo deny license allowlist + advisory + multi-version
policy; clippy pedantic + nursery on at gravitas-core (justified allows for
GR notation; five real numerical improvements landed); noUncheckedIndexedAccess
on (95 sites remediated, none were real out-of-bounds bugs); FFI exception
mapping on integrate_ray_relativistic.
Group 2C (feat): reduced-motion media-query gate at the cinematic entry,
useReducedMotion + useSimulationMode hooks, fixed-width numeric formatter
in the Telemetry HUD so columns stop jittering when integer-part width
changes.
Group 2D (feat): WebGPU rgba16float capability probe, WebGL2 EXT_color_buffer_float
return-value check, shader-cache helper for last-good fallback, adaptive
resolution hysteresis band (55/75 FPS thresholds with bleed-down on the
middle band), StaticFallback component for the WebGPU -> WebGL2 -> static
chain.
Test plan
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements
Tests
Chores