Address architectural issues #1–#7 from codebase review

src/OpticsLabStrings.ts

@@ -36,9 +36,12 @@ export const ELEMENT_TYPE_SINGLE_RAY = "SingleRay";
 export const ELEMENT_TYPE_ARC_SOURCE = "ArcSource";
 /**
  * Serialization type key for {@link ContinuousSpectrumSource}.
- * Note: intentionally camelCase (historical; changing would break saved scenes).
+ * Canonical value is PascalCase.  Scenes saved with the old camelCase value
+ * ("continuousSpectrumSource") are migrated transparently by a shim in
+ * elementSerialization.ts — the shim maps the old key to the new constructor
+ * so existing saved files continue to load without modification.
  */
-export const ELEMENT_TYPE_CONTINUOUS_SPECTRUM_SOURCE = "continuousSpectrumSource";
+export const ELEMENT_TYPE_CONTINUOUS_SPECTRUM_SOURCE = "ContinuousSpectrumSource";
 
 /** Serialization type key for {@link SegmentMirror} (flat mirror). */
 export const ELEMENT_TYPE_SEGMENT_MIRROR = "Mirror";

src/common/model/detectors/DetectorElement.ts

@@ -29,6 +29,7 @@ import {
 import { MIN_RAY_LENGTH_SQ } from "../optics/OpticsConstants.js";
 import type {
   ElementCategory,
+  IAcquirable,
   IntersectionResult,
   RayInteractionResult,
   SimulationRay,
@@ -40,7 +41,7 @@ export const DETECTOR_MAX_HITS = 2000;
 
 export type DetectorHit = { t: number; brightness: number };
 
-export class DetectorElement extends BaseSegmentElement {
+export class DetectorElement extends BaseSegmentElement implements IAcquirable {
   public readonly type = ELEMENT_TYPE_DETECTOR;
   public readonly category: ElementCategory = ELEMENT_CATEGORY_BLOCKER;
 

src/common/model/fiber/FiberOpticElement.ts

@@ -35,6 +35,7 @@ import { ELEMENT_TYPE_FIBER_CORE_GLASS, ELEMENT_TYPE_FIBER_OPTIC } from "../../.
 import { Glass, type GlassPathPoint } from "../glass/Glass.js";
 import type { Point } from "../optics/Geometry.js";
 import type {
+  ICompound,
   IntersectionResult,
   OpticalElement,
   RayCallConfig,
@@ -255,7 +256,7 @@ function buildRibbonPath(samples: Array<{ point: Point; tangent: Point }>, r: nu
 
 // ── Model class ───────────────────────────────────────────────────────────────
 
-export class FiberOpticElement extends Glass {
+export class FiberOpticElement extends Glass implements ICompound {
   public override readonly type = ELEMENT_TYPE_FIBER_OPTIC;
 
   /** Start endpoint (anchor, does not affect tangent direction). */

src/common/model/optics/OpticsScene.ts

@@ -34,13 +34,11 @@ import {
   RAY_DENSITY_MIN,
 } from "../../../OpticsLabConstants.js";
 import { VIEW_MODE_OBSERVER, VIEW_MODE_RAYS } from "../../../OpticsLabStrings.js";
-import { DetectorElement } from "../detectors/DetectorElement.js";
-import { FiberOpticElement } from "../fiber/FiberOpticElement.js";
 import { ARCHETYPE_ELEMENT_STATE, deserializeElement, LIVE_ELEMENT_STATE_KEY } from "./elementSerialization.js";
 import type { Point } from "./Geometry.js";
 import { point } from "./Geometry.js";
 import OpticalElementPhetioObject from "./OpticalElementPhetioObject.js";
-import type { Observer, OpticalElement, ViewMode } from "./OpticsTypes.js";
+import { isAcquirable, isCompound, type Observer, type OpticalElement, type ViewMode } from "./OpticsTypes.js";
 import { RayTracer, type RayTracerConfig, type TraceResult } from "./RayTracer.js";
 
 // ── Scene Settings ───────────────────────────────────────────────────────────
@@ -107,6 +105,9 @@ export class OpticsScene extends PhetioObject {
   /** Undo/redo history for add/remove element commands. */
   public readonly history: CommandHistory = new CommandHistory();
 
+  /** O(1) element lookup by id. Kept in sync with opticalElementsGroup. */
+  private readonly _elementById = new Map<string, OpticalElement>();
+
   private cachedResult: TraceResult | null = null;
   private dirty = true;
 
@@ -253,6 +254,7 @@ export class OpticsScene extends PhetioObject {
         id: element.id,
         [LIVE_ELEMENT_STATE_KEY]: element,
       });
+      this._elementById.set(element.id, element);
     };
 
     if (recordHistory) {
@@ -283,6 +285,7 @@ export class OpticsScene extends PhetioObject {
         return false;
       }
       this.opticalElementsGroup.disposeElement(wrapper);
+      this._elementById.delete(elementId);
       return true;
     };
 
@@ -302,7 +305,7 @@ export class OpticsScene extends PhetioObject {
   }
 
   public getElement(elementId: string): OpticalElement | undefined {
-    return this.getElementsArray().find((e) => e.id === elementId);
+    return this._elementById.get(elementId);
   }
 
   public getAllElements(): ReadonlyArray<OpticalElement> {
@@ -311,6 +314,7 @@ export class OpticsScene extends PhetioObject {
 
   public clearElements(): void {
     this.opticalElementsGroup.clear();
+    this._elementById.clear();
   }
 
   /**
@@ -515,14 +519,14 @@ export class OpticsScene extends PhetioObject {
 
   public simulate(): TraceResult {
     const elements = this.getElementsArray();
-    const anyAcquiring = elements.some((el) => el instanceof DetectorElement && el.isAcquiring);
+    const anyAcquiring = elements.some((el) => isAcquirable(el) && el.isAcquiring);
 
     if (!(anyAcquiring || this.dirty) && this.cachedResult) {
       return this.cachedResult;
     }
 
     for (const el of elements) {
-      if (el instanceof DetectorElement) {
+      if (isAcquirable(el)) {
         el.clearHits();
       }
     }
@@ -538,9 +542,7 @@ export class OpticsScene extends PhetioObject {
     };
 
     // Expand elements that expose multiple physics objects (e.g. fiber optic core + cladding).
-    const physicsElements = elements.flatMap((el) =>
-      el instanceof FiberOpticElement ? el.getPhysicsElements() : [el],
-    );
+    const physicsElements = elements.flatMap((el) => (isCompound(el) ? el.getPhysicsElements() : [el]));
     const tracer = new RayTracer(physicsElements, config);
     this.cachedResult = tracer.trace();
     this.dirty = false;

src/common/model/optics/OpticsTypes.ts

@@ -117,13 +117,49 @@ export interface ISerializable {
   serialize(): Record<string, unknown>;
 }
 
+/**
+ * An element that accumulates measurements across simulation frames.
+ *
+ * Implemented by DetectorElement. OpticsScene uses this interface instead of
+ * `instanceof DetectorElement` so that future detector-like elements do not
+ * require changes inside the core scene logic.
+ */
+export interface IAcquirable {
+  /** True while the element is actively accumulating a histogram pass. */
+  readonly isAcquiring: boolean;
+  /** Discard all hit data accumulated so far (called at the start of each trace). */
+  clearHits(): void;
+}
+
+/**
+ * An element that expands into multiple physics objects for ray tracing.
+ *
+ * Implemented by FiberOpticElement (outer cladding + inner core). The tracer
+ * receives the expanded list so it handles both boundaries automatically.
+ * OpticsScene uses this interface instead of `instanceof FiberOpticElement`.
+ */
+export interface ICompound {
+  /** Return the concrete physics elements that this composite element comprises. */
+  getPhysicsElements(): OpticalElement[];
+}
+
 // ── Type guard helpers ───────────────────────────────────────────────────────
 
 /** Returns true when the element actively emits rays (non-empty emitRays). */
 export function isEmitter(element: OpticalElement): element is OpticalElement & IEmitter {
   return element.category === ELEMENT_CATEGORY_LIGHT_SOURCE;
 }
 
+/** Returns true when the element implements IAcquirable (accumulates measurements). */
+export function isAcquirable(element: OpticalElement): element is OpticalElement & IAcquirable {
+  return "isAcquiring" in element && "clearHits" in element;
+}
+
+/** Returns true when the element implements ICompound (expands to multiple physics objects). */
+export function isCompound(element: OpticalElement): element is OpticalElement & ICompound {
+  return "getPhysicsElements" in element;
+}
+
 // ── Base Optical Element ─────────────────────────────────────────────────────
 
 export interface OpticalElement extends IEmitter, IIntersectable, ISerializable {

src/common/model/optics/SpatialIndex.ts

@@ -111,15 +111,14 @@ export class SpatialIndex {
 
   /** Collect all finite-bounded elements (for the fast path with few elements). */
   private getAllElements(): OpticalElement[] {
-    const all: OpticalElement[] = [...this.unbounded];
+    // Use a Set for O(1) deduplication instead of the previous O(n²) all.includes() scan.
+    const seen = new Set<OpticalElement>(this.unbounded);
     for (const list of this.grid.values()) {
       for (const el of list) {
-        if (!all.includes(el)) {
-          all.push(el);
-        }
+        seen.add(el);
       }
     }
-    return all;
+    return [...seen];
   }
 
   /** Collect elements in a single grid cell into the result set. */

src/common/model/optics/elementSerialization.ts

@@ -173,6 +173,9 @@ export function deserializeElement(obj: Record<string, unknown>): OpticalElement
       assignElementId(el, obj["id"]);
       return el;
     }
+    // Legacy camelCase key written by versions prior to the PascalCase rename.
+    // Both spellings construct the same element; no data migration needed.
+    case "continuousSpectrumSource":
     case ELEMENT_TYPE_CONTINUOUS_SPECTRUM_SOURCE: {
       const el = new ContinuousSpectrumSource(
         asPoint(obj["p1"], "p1"),

src/common/view/EditContainerNode.ts

@@ -19,7 +19,6 @@ import { HBox, Node, Text } from "scenerystack/scenery";
 import { CloseButton, TrashButton } from "scenerystack/scenery-phet";
 import { FlatAppearanceStrategy, Panel } from "scenerystack/sun";
 import { Tandem } from "scenerystack/tandem";
-import { StringManager } from "../../i18n/StringManager.js";
 import OpticsLabColors from "../../OpticsLabColors.js";
 import {
   FONT_BOLD_12PX,
@@ -33,42 +32,12 @@ import opticsLab from "../../OpticsLabNamespace.js";
 import type { SignConvention } from "../../preferences/OpticsLabPreferencesModel.js";
 import type { OpticalElement } from "../model/optics/OpticsTypes.js";
 import { buildEditControls } from "./EditControlFactory.js";
+import { getElementLabel } from "./ElementRegistry.js";
 
 // ── Constants ─────────────────────────────────────────────────────────────────
 
 const TITLE_FONT = FONT_BOLD_12PX;
 
-// Human-readable labels for each element type string.
-// Keys must match the `type` field on each model class.
-function buildTypeLabels(): Partial<Record<string, TReadOnlyProperty<string>>> {
-  const c = StringManager.getInstance().getComponentStrings();
-  return {
-    ArcSource: c.arcSourceStringProperty,
-    PointSource: c.pointSourceStringProperty,
-    Beam: c.beamSourceStringProperty,
-    DivergentBeam: c.divergentBeamSourceStringProperty,
-    SingleRay: c.singleRayStringProperty,
-    continuousSpectrumSource: c.continuousSpectrumStringProperty,
-    IdealLens: c.idealLensStringProperty,
-    IdealMirror: c.idealMirrorStringProperty,
-    SphericalLens: c.sphericalLensStringProperty,
-    CircleGlass: c.circleGlassStringProperty,
-    Glass: c.glassPrismStringProperty,
-    PlaneGlass: c.halfPlaneGlassStringProperty,
-    Mirror: c.flatMirrorStringProperty,
-    ArcMirror: c.arcMirrorStringProperty,
-    ParabolicMirror: c.parabolicMirrorStringProperty,
-    BeamSplitter: c.beamSplitterStringProperty,
-    Blocker: c.lineBlockerStringProperty,
-    Detector: c.detectorStringProperty,
-    Aperture: c.apertureStringProperty,
-    TransmissionGrating: c.transmissionGratingStringProperty,
-    ReflectionGrating: c.reflectionGratingStringProperty,
-    Track: c.trackStringProperty,
-  };
-}
-const TYPE_LABELS = buildTypeLabels();
-
 // ── EditContainerNode ────────────────────────────────────────────────────────
 
 export class EditContainerNode extends Node {
@@ -196,7 +165,10 @@ export class EditContainerNode extends Node {
     };
 
     // ── Title ──────────────────────────────────────────────────────────────
-    const typeLabel: TReadOnlyProperty<string> | string = TYPE_LABELS[element.type] ?? element.type;
+    // Label comes from the single registry source of truth; falls back to the
+    // raw type string when the element is not registered (should never happen
+    // in production, but keeps the panel usable during development).
+    const typeLabel: TReadOnlyProperty<string> | string = getElementLabel(element.type) ?? element.type;
     const titleText = new Text(typeLabel, { font: TITLE_FONT, fill: OpticsLabColors.overlayValueFillProperty });
 
     const dismissBtn = new CloseButton({
@@ -215,12 +187,10 @@ export class EditContainerNode extends Node {
     });
 
     // ── Type-specific controls ─────────────────────────────────────────────
-    const { controls, refreshCallback } = buildEditControls(
-      element,
-      triggerRebuild,
-      this._signConventionProperty.value,
-      this._useCurvatureDisplayProperty.value,
-    );
+    const { controls, refreshCallback } = buildEditControls(element, triggerRebuild, {
+      signConvention: this._signConventionProperty.value,
+      useCurvatureDisplay: this._useCurvatureDisplayProperty.value,
+    });
     this._refreshCallback = refreshCallback;
 
     // ── Assemble panel — dismiss left, title, controls, trash right ─────────

src/common/view/EditControlFactory.ts

@@ -13,14 +13,13 @@
  */
 
 import opticsLab from "../../OpticsLabNamespace.js";
-import type { SignConvention } from "../../preferences/OpticsLabPreferencesModel.js";
 import type { OpticalElement } from "../model/optics/OpticsTypes.js";
 import { buildEditControls as buildEditControlsFromRegistry } from "./ElementRegistry.js";
-import type { EditControlsResult } from "./edit-controls/EditControlsResult.js";
+import type { EditControlContext, EditControlsResult } from "./edit-controls/EditControlsResult.js";
 
-// Re-export so callers that previously imported EditControlsResult from this
-// file continue to work without any import-path changes.
-export type { EditControlsResult } from "./edit-controls/EditControlsResult.js";
+// Re-export so callers that previously imported these types from this file
+// continue to work without any import-path changes.
+export type { EditControlContext, EditControlsResult } from "./edit-controls/EditControlsResult.js";
 
 /**
  * Build the property controls appropriate for the given optical element.
@@ -30,10 +29,9 @@ export type { EditControlsResult } from "./edit-controls/EditControlsResult.js";
 export function buildEditControls(
   element: OpticalElement,
   triggerRebuild: () => void,
-  signConvention: SignConvention,
-  useCurvatureDisplay: boolean,
+  context: EditControlContext,
 ): EditControlsResult {
-  return buildEditControlsFromRegistry(element, triggerRebuild, signConvention, useCurvatureDisplay);
+  return buildEditControlsFromRegistry(element, triggerRebuild, context);
 }
 
 opticsLab.register("buildEditControls", buildEditControls);