fix(Outline, SelectiveBloom): stop unnecessary effect reconstruction and re-runs

[kvvasuu]

Found while manually verifying #359 with Outline/SelectiveBloom live: SelectiveBloom's useMemo had props in its deps, so the whole SelectiveBloomEffect got reconstructed on every selection change. Separately, both effects defaulted selection/lights to = [], a fresh array on every render, causing the declarative-selection effect to re-run on every unrelated render, whether or not <Selection> was even in use.

Extracted the shared selection-sync logic (declarative array mode + Selection/Select context mode) into a single useSelectionSync hook, used by both effects - removes the duplication and fixes both bugs in one place.

Test plan: 4 new tests (2 per effect) - no reconstruction/re-run on unrelated renders, correct selection sync, and the layer-ordering.

[Copilot]

🟡 Not ready to approve

The new selection/lights syncing paths can pass nullish ref resolutions into postprocessing APIs, which can cause runtime crashes and should be guarded before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Refactors Outline and SelectiveBloom to avoid unnecessary effect reconstruction and selection-sync re-runs by centralizing selection/layer synchronization into a shared useSelectionSync hook, and adds regression tests to validate the new behavior.

Changes:

• Added EMPTY_ARRAY and extracted shared selection/layer synchronization into useSelectionSync.
• Updated Outline and SelectiveBloom to use the shared hook and stabilize memoization behavior.
• Added new Vitest coverage for selection syncing and avoiding unnecessary reconstructions/re-runs.

File summaries

File	Description
src/util.tsx	Adds EMPTY_ARRAY and introduces useSelectionSync for shared selection/layer syncing.
src/effects/SelectiveBloom.tsx	Stops unnecessary SelectiveBloomEffect reconstruction and switches to shared selection sync.
src/effects/Outline.tsx	Switches Outline’s selection/layer syncing to the shared hook.
src/tests/SelectiveBloom.test.tsx	Adds tests for reconstruction avoidance, selection sync behavior, and layer changes.
src/tests/Outline.test.tsx	Adds tests to ensure no unnecessary selection resets and correct declarative selection behavior.

Review details

Suppressed comments (1)

src/effects/SelectiveBloom.tsx:91

• Same as the add loop: resolveRef(light) can be null for ref-typed lights, so removeLight should also guard against nullish resolved refs to avoid cleanup-time crashes.

      lights.forEach((light) => removeLight(resolveRef(light), effect))

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The wrappers now drop previously-forwarded effect option props (removal of ...props) while their public prop types still advertise broader option support, creating a likely silent API regression.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

src/effects/Outline.tsx:45

• OutlineProps is typed as the full OutlineEffect options object (ConstructorParameters[2]), but the wrapper now only forwards the explicit subset listed here (no ...props). This can silently drop valid OutlineEffect options passed by consumers and is a behavioral/API change from the previous ...props forwarding.

      new OutlineEffect(scene, camera, {
        blendFunction,
        patternTexture,
        patternScale,
        edgeStrength,

src/effects/SelectiveBloom.tsx:53

• SelectiveBloomProps still accepts all keys from BloomEffectOptions, but the SelectiveBloomEffect constructor options are now hard-coded to the explicit list below (no ...props forwarding). Any BloomEffectOptions fields not enumerated here will be silently ignored, which is a behavioral/API change from the previous implementation that forwarded ...props.

  const effect = useMemo(() => {
    const instance = new SelectiveBloomEffect(scene, camera, {
      blendFunction: BlendFunction.ADD,
      luminanceThreshold,
      luminanceSmoothing,
      mipmapBlur,

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.