Skip to content

Address architectural issues #1–#7 from codebase review - #48

Merged
veillette merged 1 commit into
mainfrom
claude/codebase-architecture-review-vCe5N
Apr 12, 2026
Merged

veillette merged 1 commit into
mainfrom
claude/codebase-architecture-review-vCe5N

Conversation

@veillette

Copy link
Copy Markdown
Collaborator

#1 – Centralize element labels in ElementRegistry
Add a label: TReadOnlyProperty<string> field to ElementDescriptor so the
registry is the single source of truth for element display names. Export
getElementLabel(). Remove the duplicate buildTypeLabels()/TYPE_LABELS
map from EditContainerNode and the now-unused StringManager import there.

#2 – Rename continuousSpectrumSource → ContinuousSpectrumSource
Fix the lone camelCase type key to match the PascalCase convention used by
all other element types. Add a case "continuousSpectrumSource": fall-through
shim in elementSerialization.ts so scenes saved by older versions continue to
load without data loss.

#3 – Replace 4-arg buildEditControls signature with EditControlContext bag
Introduce EditControlContext { signConvention, useCurvatureDisplay } in
EditControlsResult.ts. Update ElementDescriptor, the exported
buildEditControls() functions in ElementRegistry/EditControlFactory, and the
call site in EditContainerNode. Registry entries that previously prefixed
unused params with _ now destructure only what they need from the bag.
Adding a new display preference requires a single interface change, not
touching every builder signature.

#4 – Replace instanceof checks in OpticsScene.simulate() with interfaces
Add IAcquirable and ICompound to OpticsTypes.ts, with corresponding type
guards (isAcquirable, isCompound). DetectorElement implements IAcquirable;
FiberOpticElement implements ICompound. OpticsScene.simulate() uses the
guards instead of instanceof DetectorElement / instanceof FiberOpticElement,
removing its dependency on those concrete classes.

#5 – Make OpticsScene.getElement() O(1)
Add a _elementById: Map<string, OpticalElement> maintained by addElement /
removeElement / clearElements. getElement() is now a single Map lookup
instead of a full array scan.

#6 – Fix SpatialIndex.getAllElements() O(n²) dedup
Replace the all.includes(el) linear scan inside a loop with a
Set<OpticalElement> accumulator, reducing the fast-path dedup from O(n²)
to O(n).

#7 – Extract syncBidirectional helper in SimScreenView
Replace three near-identical let blockXSync = false bidirectional-sync
patterns with a single generic syncBidirectional(propA, propB) helper.
The helper also writes propA's initial value to propB, removing the
separate initialization lines that preceded two of the three blocks.

https://claude.ai/code/session_01H65WqMzMK88Jkra4gAjhE7

#1 – Centralize element labels in ElementRegistry
Add a `label: TReadOnlyProperty<string>` field to ElementDescriptor so the
registry is the single source of truth for element display names.  Export
`getElementLabel()`.  Remove the duplicate `buildTypeLabels()`/`TYPE_LABELS`
map from EditContainerNode and the now-unused StringManager import there.

#2 – Rename continuousSpectrumSource → ContinuousSpectrumSource
Fix the lone camelCase type key to match the PascalCase convention used by
all other element types.  Add a `case "continuousSpectrumSource":` fall-through
shim in elementSerialization.ts so scenes saved by older versions continue to
load without data loss.

#3 – Replace 4-arg buildEditControls signature with EditControlContext bag
Introduce `EditControlContext { signConvention, useCurvatureDisplay }` in
EditControlsResult.ts.  Update ElementDescriptor, the exported
buildEditControls() functions in ElementRegistry/EditControlFactory, and the
call site in EditContainerNode.  Registry entries that previously prefixed
unused params with `_` now destructure only what they need from the bag.
Adding a new display preference requires a single interface change, not
touching every builder signature.

#4 – Replace instanceof checks in OpticsScene.simulate() with interfaces
Add IAcquirable and ICompound to OpticsTypes.ts, with corresponding type
guards (isAcquirable, isCompound).  DetectorElement implements IAcquirable;
FiberOpticElement implements ICompound.  OpticsScene.simulate() uses the
guards instead of `instanceof DetectorElement` / `instanceof FiberOpticElement`,
removing its dependency on those concrete classes.

#5 – Make OpticsScene.getElement() O(1)
Add a `_elementById: Map<string, OpticalElement>` maintained by addElement /
removeElement / clearElements.  getElement() is now a single Map lookup
instead of a full array scan.

#6 – Fix SpatialIndex.getAllElements() O(n²) dedup
Replace the `all.includes(el)` linear scan inside a loop with a
`Set<OpticalElement>` accumulator, reducing the fast-path dedup from O(n²)
to O(n).

#7 – Extract syncBidirectional helper in SimScreenView
Replace three near-identical `let blockXSync = false` bidirectional-sync
patterns with a single generic `syncBidirectional(propA, propB)` helper.
The helper also writes propA's initial value to propB, removing the
separate initialization lines that preceded two of the three blocks.

https://claude.ai/code/session_01H65WqMzMK88Jkra4gAjhE7
@veillette
veillette merged commit 0b7767a into main Apr 12, 2026
1 check passed
@veillette
veillette deleted the claude/codebase-architecture-review-vCe5N branch April 15, 2026 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants