refactor: complete mode system refactoring to eliminate code duplication

.claude/settings.local.json

@@ -83,7 +83,8 @@
       "Bash(touch:*)",
       "Bash(open http://localhost:5173/debug-multiple.html)",
       "Bash(open http://localhost:5173/debug-focus-test.html)",
-      "Bash(open http://localhost:5175/debug.html)"
+      "Bash(open http://localhost:5175/debug.html)",
+      "Bash(git add:*)"
     ]
   }
 }

Memory_Bank.md

@@ -123,6 +123,45 @@ None
 **Next Steps:**
 Ready for pull request creation. Implementation successfully addresses GitHub Issue #126 by eliminating the need to commit version.js changes after local development builds.
 
+## Issue #130 - Fix Manual Harmonics Visibility When Zoomed - 2025-01-08
+
+**Task Reference:** GitHub Issue #130 and Task Assignment Prompt Task_Issue_130.md
+**Problem:** Manual harmonics were placed at the center of the overall time period instead of the center of the currently visible (zoomed) time period, making them invisible to users when zoomed in.
+
+**Root Cause Analysis:**
+- In `ManualHarmonicModal.js` (lines 88-89), when no cursor position was available, anchor time was calculated using `(state.config.timeMin + state.config.timeMax) / 2` 
+- This used the **full time range** rather than the **visible time range** when zoomed
+- Users had to zoom out to see newly created manual harmonics, degrading user experience
+
+**Solution Implemented:**
+1. **Exported existing infrastructure:** Modified `calculateVisibleDataRange()` function in `src/components/table.js` from private to exported function to enable reuse
+2. **Enhanced modal interface:** Updated `showManualHarmonicModal()` signature to accept GramFrame instance parameter for access to zoom state
+3. **Created helper function:** Added `calculateVisibleTimePeriodCenter(state, instance)` in `ManualHarmonicModal.js` that:
+   - Returns full time range center when zoom level = 1.0 (backward compatibility)
+   - Calculates visible time range center using `calculateVisibleDataRange()` when zoomed
+4. **Updated anchor calculation:** Modified `addHarmonic()` function to use zoom-aware center calculation when no cursor position available
+
+**Key Code Changes:**
+- `src/components/table.js`: Exported `calculateVisibleDataRange()` function
+- `src/modes/harmonics/HarmonicsMode.js`: Updated modal call to pass instance parameter
+- `src/modes/harmonics/ManualHarmonicModal.js`: 
+  - Added import for `calculateVisibleDataRange`
+  - Added `calculateVisibleTimePeriodCenter()` helper function  
+  - Updated modal signature and anchor time calculation logic
+
+**Testing and Verification:**
+- All harmonics mode tests pass (10/10 tests successful)
+- TypeScript compilation successful with no errors
+- Build process successful with no issues
+- No regression in existing functionality confirmed
+
+**Technical Decisions:**
+- Leveraged existing `calculateVisibleDataRange()` infrastructure rather than duplicating zoom logic
+- Maintained backward compatibility by preserving behavior when zoom level = 1.0
+- Used dependency injection pattern to pass GramFrame instance to modal for clean separation of concerns
+
+**Result:** Manual harmonics are now positioned at the center of the visible viewport when zoomed, immediately visible to users without requiring zoom out operation.
+
 ## Issue #88 Phase A-B - Refactor Large Files and Improve Module Boundaries - 2025-01-06
 
 **Task Reference:** GitHub Issue #88 - Refactor Large Files and Improve Module Boundaries

src/components/ColorPicker.js

@@ -33,7 +33,7 @@ const COLOR_PALETTE = [
 export function createColorPicker(state) {
   const container = document.createElement('div')
   container.className = 'gram-frame-color-picker'
-  container.style.display = (state.mode === 'harmonics' || state.mode === 'analysis') ? 'block' : 'none'
+  container.style.display = 'block'
 
   // Label
   const label = document.createElement('div')

src/components/table.js

@@ -149,7 +149,7 @@ export function setupSpectrogramImage(instance, imageUrl) {
  */
 export function updateSVGLayout(instance) {
   const { naturalWidth, naturalHeight } = instance.state.imageDetails
-  const margins = instance.state.axes.margins
+  const margins = instance.state.margins
 
   if (!naturalWidth || !naturalHeight) {
     return
@@ -208,7 +208,7 @@ export function updateSVGLayout(instance) {
 export function applyZoomTransform(instance) {
   const { level, centerX, centerY } = instance.state.zoom
   const { naturalWidth, naturalHeight } = instance.state.imageDetails
-  const margins = instance.state.axes.margins
+  const margins = instance.state.margins
 
   if (!instance.spectrogramImage) {
     return
@@ -273,7 +273,7 @@ export function renderAxes(instance) {
   instance.axesGroup.innerHTML = ''
 
   const { naturalWidth, naturalHeight } = instance.state.imageDetails
-  const margins = instance.state.axes.margins
+  const margins = instance.state.margins
 
   if (!naturalWidth || !naturalHeight) {
     return
@@ -297,7 +297,7 @@ export function renderAxes(instance) {
 export function calculateVisibleDataRange(instance) {
   const { timeMin, timeMax, freqMin, freqMax } = instance.state.config
   const { naturalWidth, naturalHeight } = instance.state.imageDetails
-  const margins = instance.state.axes.margins
+  const margins = instance.state.margins
   const zoomLevel = instance.state.zoom.level
 
   if (zoomLevel === 1.0) {

src/core/FocusManager.js

@@ -20,10 +20,8 @@ let registeredInstances = new Set()
 export function registerInstance(instance) {
   registeredInstances.add(instance)
 
-  // If this is the first instance, make it focused by default
-  if (registeredInstances.size === 1 && !currentFocusedInstance) {
-    setFocusedInstance(instance)
-  }
+  // Don't auto-focus the first instance - let user explicitly interact to focus
+  // This prevents unwanted focus behavior when multiple instances exist on a page
 }
 
 /**

src/core/events.js

@@ -25,7 +25,7 @@ function screenToDataWithZoom(instance, event) {
   const svgCoords = screenToSVGCoordinates(screenX, screenY, instance.svg, instance.state.imageDetails)
 
   // Convert to data coordinates (accounting for margins and zoom)
-  const margins = instance.state.axes.margins
+  const margins = instance.state.margins
   const zoomLevel = instance.state.zoom.level
   const { naturalWidth, naturalHeight } = instance.state.imageDetails
 

src/core/initialization/ModeInitialization.js

@@ -30,7 +30,7 @@ export function initializeModeInfrastructure(instance) {
   // Initialize all modes using factory
   const availableModes = ModeFactory.getAvailableModes()
   availableModes.forEach(modeName => {
-    instance.modes[modeName] = ModeFactory.createMode(modeName, instance, instance.state)
+    instance.modes[modeName] = ModeFactory.createMode(modeName, instance)
   })
 }
 

src/core/keyboardControl.js

@@ -88,6 +88,7 @@ function handleGlobalKeyboardEvent(event) {
     return // No selection
   }
 
+
   // Prevent default browser behavior
   event.preventDefault()
   event.stopPropagation()
@@ -157,13 +158,15 @@ function moveSelectedMarker(instance, markerId, movement) {
     return
   }
 
+
   // Convert current marker position to SVG coordinates
   const currentSVG = dataToSVGCoordinates(
     marker.freq, 
     marker.time, 
     instance.state.config,
     instance.state.imageDetails,
-    instance.state.rate
+    instance.state.rate,
+    instance.state.margins
   )
 
   // Apply movement in SVG space
@@ -179,7 +182,7 @@ function moveSelectedMarker(instance, markerId, movement) {
     instance.state.config,
     instance.state.imageDetails,
     instance.state.rate,
-    instance.state.axes.margins
+    instance.state.margins
   )
 
   // Update marker position
@@ -239,7 +242,7 @@ function moveSelectedHarmonicSet(instance, harmonicSetId, movement) {
     // Convert current anchor time to SVG coordinates
     const { naturalHeight } = instance.state.imageDetails
     const { timeMin, timeMax } = instance.state.config
-    const margins = instance.state.axes.margins
+    const margins = instance.state.margins
 
     // Calculate current anchor position in SVG space
     const normalizedTime = 1.0 - (harmonicSet.anchorTime - timeMin) / (timeMax - timeMin)
@@ -290,12 +293,12 @@ function moveSelectedHarmonicSet(instance, harmonicSetId, movement) {
  * @param {Config} config - Configuration object
  * @param {ImageDetails} imageDetails - Image dimensions
  * @param {number} rate - Rate scaling factor
+ * @param {AxesMargins} margins - Axes margins
  * @returns {SVGCoordinates} SVG coordinates {x, y}
  */
-function dataToSVGCoordinates(freq, time, config, imageDetails, rate) {
+function dataToSVGCoordinates(freq, time, config, imageDetails, rate, margins) {
   const { freqMin, freqMax, timeMin, timeMax } = config
   const { naturalWidth, naturalHeight } = imageDetails
-  const margins = { left: 60, top: 15 } // Use default margins
 
   // Convert frequency back to raw frequency space for positioning
   const rawFreq = freq * rate

src/core/state.js

@@ -58,13 +58,11 @@ export const initialState = {
     width: 0,
     height: 0
   },
-  axes: {
-    margins: {
-      left: 60,    // Space for time axis labels
-      bottom: 50,  // Space for frequency axis labels  
-      right: 15,   // Small right margin
-      top: 15      // Small top margin
-    }
+  margins: {
+    left: 60,    // Space for time axis labels
+    bottom: 50,  // Space for frequency axis labels  
+    right: 15,   // Small right margin
+    top: 15      // Small top margin
   },
   // Simple zoom state for transform-based zoom
   zoom: {

src/modes/BaseMode.js

@@ -7,11 +7,9 @@ export class BaseMode {
   /**
    * Constructor for base mode
    * @param {GramFrame} instance - GramFrame instance
-   * @param {GramFrameState} state - GramFrame state object
    */
-  constructor(instance, state) {
+  constructor(instance) {
     this.instance = instance
-    this.state = state
   }
 
   /**
@@ -179,4 +177,27 @@ export class BaseMode {
     // Default implementation - override in subclasses
     return {}
   }
+
+  /**
+   * Get viewport configuration for coordinate transformations
+   * @returns {ViewportConfig} Viewport configuration object
+   */
+  getViewport() {
+    return {
+      margins: this.instance.state.margins,
+      imageDetails: this.instance.state.imageDetails,
+      config: this.instance.state.config,
+      zoom: this.instance.state.zoom
+    }
+  }
+
+  /**
+   * Update cursor style for drag operations
+   * @param {string} style - Cursor style ('crosshair', 'grab', 'grabbing')
+   */
+  updateCursorStyle(style) {
+    if (this.instance.spectrogramImage) {
+      this.instance.spectrogramImage.style.cursor = style
+    }
+  }
 }

src/modes/ModeFactory.js

@@ -13,24 +13,23 @@ export class ModeFactory {
    * Create a mode instance based on mode name
    * @param {ModeType} modeName - Name of the mode
    * @param {GramFrame} instance - GramFrame instance
-   * @param {GramFrameState} state - GramFrame state object
    * @returns {BaseMode} Mode instance
    * @throws {Error} If mode name is invalid or mode class is not available
    */
-  static createMode(modeName, instance, state) {
+  static createMode(modeName, instance) {
     try {
       switch (modeName) {
         case 'analysis':
-          return new AnalysisMode(instance, state)
+          return new AnalysisMode(instance)
 
         case 'harmonics':
-          return new HarmonicsMode(instance, state)
+          return new HarmonicsMode(instance)
 
         case 'doppler':
-          return new DopplerMode(instance, state)
+          return new DopplerMode(instance)
 
         case 'pan':
-          return new PanMode(instance, state)
+          return new PanMode(instance)
 
         default:
           throw new Error(`Invalid mode name: ${modeName}. Valid modes are: analysis, harmonics, doppler, pan`)
@@ -42,7 +41,7 @@ export class ModeFactory {
         stack: error instanceof Error ? error.stack : undefined,
         modeName,
         instanceType: instance?.constructor?.name,
-        stateExists: !!state
+        stateExists: !!instance?.state
       })
 
       // In test environments, throw the error to fail fast
@@ -52,7 +51,7 @@ export class ModeFactory {
 
       // Fallback to base mode to prevent complete failure in production
       console.warn(`Falling back to BaseMode for "${modeName}" due to error`)
-      return new BaseMode(instance, state)
+      return new BaseMode(instance)
     }
   }