fix: random not able to exit piano roll hybrid mode issue when switching between different MIDI regions (introduced a region dragging start threshold)

2026-05-01 20:34:21 -07:00
parent cbd2fc60a0
commit d5795c5281
6 changed files with 234 additions and 36 deletions
@@ -439,8 +439,9 @@ const MainContent: React.FC<MainContentProps> = ({
      }
    }

-    // If piano roll is visible, set this region as the active region
-    if (showPianoRoll) {
+    // Keep the active piano roll region in sync when that same region is updated.
+    // Do not switch the editor to an unrelated region from generic move/resize updates.
+    if (showPianoRoll && activeRegionId === regionId) {
      setActiveRegionId(regionId);

      if (DEBUG_MODE.MAIN_CONTENT) {
@@ -0,0 +1,97 @@
+import React from 'react';
+import { describe, it, expect, beforeAll, beforeEach, vi } from 'vitest';
+import { render, fireEvent } from '@testing-library/react';
+import RegionItem from './RegionItem';
+import { KGMidiRegion } from '../../core/region/KGMidiRegion';
+import { KGMainContentState } from '../../core/state/KGMainContentState';
+
+vi.mock('../../stores/projectStore', () => ({
+  useProjectStore: () => ({
+    selectedRegionIds: [],
+    timeSignature: { numerator: 4, denominator: 4 },
+    bpm: 120,
+  }),
+}));
+
+describe('RegionItem', () => {
+  beforeAll(() => {
+    Object.defineProperty(HTMLCanvasElement.prototype, 'getContext', {
+      value: vi.fn(() => ({
+        clearRect: vi.fn(),
+        fillRect: vi.fn(),
+      })),
+    });
+
+    class ResizeObserverMock {
+      observe() {}
+      unobserve() {}
+      disconnect() {}
+    }
+
+    vi.stubGlobal('ResizeObserver', ResizeObserverMock);
+  });
+
+  beforeEach(() => {
+    KGMainContentState.instance().setActiveTool('pointer');
+  });
+
+  const renderRegion = (props: Partial<React.ComponentProps<typeof RegionItem>> = {}) => {
+    const midiRegion = new KGMidiRegion('midi-1', 'track-1', 0, 'Test Region', 0, 4);
+
+    return render(
+      <RegionItem
+        id="midi-1"
+        name="Test Region"
+        style={{ left: '0px', width: '120px', position: 'absolute' }}
+        onClick={vi.fn()}
+        onDragStart={vi.fn()}
+        onDrag={vi.fn()}
+        onDragEnd={vi.fn()}
+        midiRegion={midiRegion}
+        {...props}
+      />
+    );
+  };
+
+  it('treats small pointer jitter as a click', () => {
+    const onClick = vi.fn();
+    const onDragStart = vi.fn();
+    const onDrag = vi.fn();
+    const onDragEnd = vi.fn();
+
+    const { container } = renderRegion({ onClick, onDragStart, onDrag, onDragEnd });
+    const region = container.querySelector('.track-region');
+
+    expect(region).toBeTruthy();
+
+    fireEvent.mouseDown(region!, { clientX: 100, clientY: 100 });
+    fireEvent.mouseMove(document, { clientX: 102, clientY: 102 });
+    fireEvent.mouseUp(document, { clientX: 102, clientY: 102 });
+
+    expect(onClick).toHaveBeenCalledWith('midi-1');
+    expect(onDragStart).not.toHaveBeenCalled();
+    expect(onDrag).not.toHaveBeenCalled();
+    expect(onDragEnd).not.toHaveBeenCalled();
+  });
+
+  it('starts a drag after crossing the movement threshold', () => {
+    const onClick = vi.fn();
+    const onDragStart = vi.fn();
+    const onDrag = vi.fn();
+    const onDragEnd = vi.fn();
+
+    const { container } = renderRegion({ onClick, onDragStart, onDrag, onDragEnd });
+    const region = container.querySelector('.track-region');
+
+    expect(region).toBeTruthy();
+
+    fireEvent.mouseDown(region!, { clientX: 100, clientY: 100 });
+    fireEvent.mouseMove(document, { clientX: 110, clientY: 100 });
+    fireEvent.mouseUp(document, { clientX: 110, clientY: 100 });
+
+    expect(onClick).not.toHaveBeenCalled();
+    expect(onDragStart).toHaveBeenCalledWith('midi-1', 100, 100);
+    expect(onDrag).toHaveBeenCalledWith('midi-1', 10, 0);
+    expect(onDragEnd).toHaveBeenCalledWith('midi-1');
+  });
+});
@@ -9,6 +9,8 @@ import { KGAudioRegion } from '../../core/region/KGAudioRegion';
 import { useProjectStore } from '../../stores/projectStore';
 import { KGMainContentState } from '../../core/state/KGMainContentState';

+const DRAG_START_THRESHOLD_PX = 4;
+
 interface RegionItemProps {
  id: string;
  name: string;
@@ -73,7 +75,7 @@ const RegionItem: React.FC<RegionItemProps> = ({
  // Use refs to track states for immediate access
  const isResizingRef = useRef<boolean>(false);
  const isDraggingRef = useRef<boolean>(false);
-  const hasMovedRef = useRef<boolean>(false);
+  const isPendingDragRef = useRef<boolean>(false);

  // Canvas ref for note visualization
  const canvasRef = useRef<HTMLCanvasElement | null>(null);
@@ -391,7 +393,7 @@ const RegionItem: React.FC<RegionItemProps> = ({
    const activeTool = KGMainContentState.instance().getActiveTool();
    if (activeTool === 'pencil') {
      // Still allow click events to pass through for region selection
-      if (!hasMovedRef.current && onClick) {
+      if (onClick) {
        if (DEBUG_MODE.REGION_ITEM) {
          console.log(`REGION CLICKED (pencil mode): regionId=${id}`);
        }
@@ -404,7 +406,7 @@ const RegionItem: React.FC<RegionItemProps> = ({
    e.preventDefault();

    // Reset movement tracking
-    hasMovedRef.current = false;
+    isPendingDragRef.current = false;

    // Store initial mouse position
    initialMousePosRef.current = { x: e.clientX, y: e.clientY };
@@ -423,21 +425,12 @@ const RegionItem: React.FC<RegionItemProps> = ({
        onResizeStart(id, resizeEdge, e.clientX);
      }
    } else {
-      // Start dragging
+      // Wait for actual pointer movement before promoting this gesture to a drag.
      if (DEBUG_MODE.REGION_ITEM) {
-        console.log(`DRAG START: regionId=${id}`);
+        console.log(`PENDING REGION INTERACTION: regionId=${id}`);
      }

-      setIsDragging(true);
-      isDraggingRef.current = true;
-
-      // Change cursor to grabbing during drag
-      setCursor('grabbing');
-
-      // Call the onDragStart callback if provided
-      if (onDragStart) {
-        onDragStart(id, e.clientX, e.clientY);
-      }
+      isPendingDragRef.current = true;
    }

    // Add global event listeners for mouse move and up
@@ -447,9 +440,6 @@ const RegionItem: React.FC<RegionItemProps> = ({

  // Handle global mouse move for resize or drag
  const handleGlobalMouseMove = (e: MouseEvent) => {
-    // Set the hasMovedRef to true as soon as there's movement
-    hasMovedRef.current = true;
-
    if (isResizingRef.current) {
      // Handle resize
      if (DEBUG_MODE.REGION_ITEM) {
@@ -463,16 +453,35 @@ const RegionItem: React.FC<RegionItemProps> = ({
      if (onResize) {
        onResize(id, resizeEdge, deltaX);
      }
-    } else if (isDraggingRef.current) {
+    } else if (isDraggingRef.current || isPendingDragRef.current) {
+      const deltaX = e.clientX - initialMousePosRef.current.x;
+      const deltaY = e.clientY - initialMousePosRef.current.y;
+      const movedEnough = Math.hypot(deltaX, deltaY) >= DRAG_START_THRESHOLD_PX;
+
+      if (!isDraggingRef.current) {
+        if (!movedEnough) {
+          return;
+        }
+
+        if (DEBUG_MODE.REGION_ITEM) {
+          console.log(`DRAG START: regionId=${id}`);
+        }
+
+        isPendingDragRef.current = false;
+        setIsDragging(true);
+        isDraggingRef.current = true;
+        setCursor('grabbing');
+
+        if (onDragStart) {
+          onDragStart(id, initialMousePosRef.current.x, initialMousePosRef.current.y);
+        }
+      }
+
      // Handle drag
      if (DEBUG_MODE.REGION_ITEM) {
        console.log(`DRAG MOVE: regionId=${id}, trackIndex=${trackIndex}`);
      }

-      // Calculate delta from initial position
-      const deltaX = e.clientX - initialMousePosRef.current.x;
-      const deltaY = e.clientY - initialMousePosRef.current.y;
-
      // Call the onDrag callback if provided
      if (onDrag) {
        onDrag(id, deltaX, deltaY);
@@ -511,16 +520,15 @@ const RegionItem: React.FC<RegionItemProps> = ({
      if (onDragEnd) {
        onDragEnd(id);
      }
-
-      // If there was no movement, treat it as a click
-      if (!hasMovedRef.current && onClick) {
-        if (DEBUG_MODE.REGION_ITEM) {
-          console.log(`REGION CLICKED: regionId=${id}`);
-        }
-        onClick(id);
+    } else if (isPendingDragRef.current && onClick) {
+      if (DEBUG_MODE.REGION_ITEM) {
+        console.log(`REGION CLICKED: regionId=${id}`);
      }
+      onClick(id);
    }

+    isPendingDragRef.current = false;
+
    // Remove global event listeners
    document.removeEventListener('mousemove', handleGlobalMouseMove);
    document.removeEventListener('mouseup', handleGlobalMouseUp);
@@ -492,6 +492,13 @@ const TrackGridItem: React.FC<TrackGridItemProps> = ({
    currentDragTop.current = null;
    currentDragRegion.current = null;
    
+    if (finalBarNumber === region.barNumber && finalTrackIndex === region.trackIndex) {
+      if (DEBUG_MODE.TRACK_GRID_ITEM) {
+        console.log(`Skipping no-op drag update for region ${regionId}`);
+      }
+      return;
+    }
+
    // Notify parent about drag end with final values
    if (onRegionDragEnd) {
      onRegionDragEnd(regionId, finalBarNumber, finalTrackIndex);
@@ -387,6 +387,13 @@ const TrackGridPanel: React.FC<TrackGridPanelProps> = ({
    const region = regions.find(r => r.id === regionId);
    if (!region) return;

+    if (finalBarNumber === region.barNumber && finalTrackIndex === region.trackIndex) {
+      if (DEBUG_MODE.TRACK_GRID_PANEL) {
+        console.log(`Skipping no-op move for region ${regionId}`);
+      }
+      return;
+    }
+    
    // Get the target track
    const targetTrack = tracks[finalTrackIndex];
    if (!targetTrack) return;
@@ -0,0 +1,78 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { act } from '@testing-library/react';
+import { KGMidiTrack } from '../core/track/KGMidiTrack';
+
+const mockProject = {
+  getTimeSignature: () => ({ numerator: 4, denominator: 4 }),
+  getMaxBars: () => 32,
+  getBarWidthMultiplier: () => 1,
+  getTracks: () => [new KGMidiTrack('Track 1', 0, 'acoustic_grand_piano')],
+  getBpm: () => 120,
+  getKeySignature: () => 'C major',
+  getName: () => 'Test Project',
+  getSelectedMode: () => 'major',
+  getIsLooping: () => false,
+  getLoopingRange: () => null,
+};
+
+const mockCore = {
+  getCurrentProject: () => mockProject,
+  setPlayheadUpdateCallback: vi.fn(),
+  setPlaybackStateChangeCallback: vi.fn(),
+  getSelectedItems: () => [],
+  onSelectionChanged: vi.fn(),
+  canUndo: () => false,
+  canRedo: () => false,
+  getUndoDescription: () => '',
+  getRedoDescription: () => '',
+  setOnCommandHistoryChanged: vi.fn(),
+  executeCommand: vi.fn(),
+  clearSelectedItems: vi.fn(),
+  getStatus: () => 'Ready',
+  getPlayheadPosition: () => 0,
+  getIsPlaying: () => false,
+};
+
+vi.mock('../core/KGCore', () => ({
+  KGCore: {
+    instance: () => mockCore,
+  },
+}));
+
+vi.mock('../core/config/ConfigManager', () => ({
+  ConfigManager: {
+    instance: () => ({
+      getIsInitialized: () => true,
+      get: () => false,
+    }),
+  },
+}));
+
+describe('projectStore piano roll state', () => {
+  beforeEach(() => {
+    vi.resetModules();
+  });
+
+  it('clears hybrid state when opening a MIDI region', async () => {
+    const { useProjectStore } = await import('./projectStore');
+
+    act(() => {
+      useProjectStore.getState().openHybridMode('midi-a', 'audio-a');
+    });
+
+    let state = useProjectStore.getState();
+    expect(state.pianoRollMode).toBe('hybrid');
+    expect(state.activeRegionId).toBe('midi-a');
+    expect(state.hybridAudioRegionId).toBe('audio-a');
+
+    act(() => {
+      useProjectStore.getState().openMidiPianoRoll('midi-b');
+    });
+
+    state = useProjectStore.getState();
+    expect(state.showPianoRoll).toBe(true);
+    expect(state.pianoRollMode).toBe('midi-edit');
+    expect(state.activeRegionId).toBe('midi-b');
+    expect(state.hybridAudioRegionId).toBeNull();
+  });
+});