Skip to content

refactor: split interactive_structure_editor.ts into focused mixins (TB-ARCH-2) - #209

Closed
timurbazhirov wants to merge 1 commit into
fix/scope-basis-unit-conversion-to-touched-atomsfrom
refactor/split-interactive-editor-mixins
Closed

refactor: split interactive_structure_editor.ts into focused mixins (TB-ARCH-2)#209
timurbazhirov wants to merge 1 commit into
fix/scope-basis-unit-conversion-to-touched-atomsfrom
refactor/split-interactive-editor-mixins

Conversation

@timurbazhirov

Copy link
Copy Markdown
Member

Summary

  • interactive_structure_editor.ts was 1460 lines, flagged by the TB-persona review (TB-ARCH-2) as "still the largest file in src/mixins/ by a wide margin." Extracts the two most cleanly-separable feature areas into their own composed mixins:
    • MarqueeSelectionMixin (src/mixins/marquee_selection.ts) — rubber-band marquee selection: updateMarqueeState_, showMarqueeOverlay_/updateMarqueeOverlay_/hideMarqueeOverlay_, getAtomsInScreenRect_, finishMarqueeSelection_. Fully self-contained, moved verbatim.
    • GroupTransformMixin (src/mixins/group_transform.ts) — group-transform pivot/centroid math (attachPivotToSelection_, computeCentroid_, moved verbatim) plus four newly-extracted helpers: handleGroupPivotChange_ and commitGroupGizmoDrag_ (the group-specific halves of the TransformControls change/mouseUp listeners — initializeEditor() now calls into these instead of inlining the logic), captureGroupDragStartPositions_ (also de-duplicates an identical inline snapshot that was repeated in beginAtomDrag_), and applyGroupDragDelta_ (the group half of the direct-drag pointer-move handler).
    • A small shared interactive_editor_constants.ts holds DRAG_THRESHOLD_PX, needed by both the base mixin's atom-drag threshold and the new marquee mixin's own activation threshold.
  • Both new mixins are composed into Wave in wave.js immediately before InteractiveStructureEditorMixin (whose constructor still runs last and calls initializeEditor()/initializeSelectionRaycaster(), by which point the two new mixins' constructors have already set their own state fields to defaults).
  • Deliberately not split: beginAtomDrag_/endAtomDrag_/cancelAtomDrag_ (the direct-drag lifecycle) stay in the base mixin. They're a genuinely unified state machine covering all 4 drag shapes (single/group × direct/gizmo) at once, and this round's Esc-cancel regression tests are concentrated exactly there — forcing a single/group split would trade real, hard-won correctness coverage for a readability preference. See plan/interactive-editor-spec-plan.md's "Chained PR stack" section for the full writeup.
  • Base file: 1460 → 1211 lines. Fourth and last PR in the post-review chained-PR stack (stacked on #208).

Test plan

  • Pure refactor — zero test file changes. All 181 pre-existing tests pass unchanged, which is the primary evidence of behavioral equivalence for a change like this.
  • tsc --noEmit surfaced a real narrowing gap the split introduced (group_transform.ts's fields no longer share a class body with transformControls_/transformDragStartPosition_, so TS couldn't narrow selectionPivot_'s nullability through a compound guard referencing them) — fixed by capturing selectionPivot_ in a local before the guard, not suppressed.
  • Live-browser smoke test against a real (non-jsdom, non-mocked) WebGL Wave instance: marquee-select correctly selects both atoms via a screen-space rectangle; group direct-drag moves both atoms by an identical delta; gizmo group-rotate preserves the centroid and resets the pivot's quaternion after commit.
  • npm run lint clean (0 errors; only pre-existing no-explicit-any warnings matching this file's established convention).
  • npm run build clean.

🤖 Generated with Claude Code

…TB-ARCH-2)

Extract marquee rubber-band selection into a new MarqueeSelectionMixin
and group-transform pivot/centroid math plus the group-only halves of
the TransformControls change/mouseUp listeners into a new
GroupTransformMixin, composed alongside InteractiveStructureEditorMixin
in wave.js. The direct-drag lifecycle (beginAtomDrag_/endAtomDrag_/
cancelAtomDrag_) stays in the base mixin - it's a genuinely unified
state machine covering all 4 drag shapes at once, and this round's
Esc-cancel regression tests are concentrated exactly there, so forcing
a single/group split there would trade real correctness coverage for
a readability preference.

Base file: 1460 -> 1211 lines. Pure refactor: all 181 pre-existing
tests pass unchanged, and behavioral equivalence was further verified
live against a real (non-jsdom) WebGL Wave instance - marquee-select,
group direct-drag, and gizmo group-rotate all independently confirmed
correct.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copy link
Copy Markdown
Member Author

Closing — consolidated into #216 (then #214 on top).

Verified contained: this PR's head e918f0cf is a direct ancestor of claude/uiux-p2-figure-export.

Worth noting for TB-ARCH-2's own sake: interactive_structure_editor.ts has grown again since — U-13 adds the edit-mode touch remap to it — so the split this PR started is still worth finishing, just as follow-up work rather than as a PR waiting on the same review.

The branch refactor/split-interactive-editor-mixins stays on the remote.


Generated by Claude Code

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.

1 participant