From eab0b0d19a4d7eebd166d1ee65fd303bf718b682 Mon Sep 17 00:00:00 2001 From: Nuwan Date: Wed, 25 Feb 2026 18:51:18 +0530 Subject: [PATCH] docs(v1.6): create roadmap for Media Features Polish - 3 phases: JamTrack Polish, Backing Track Sync, Metronome Responsiveness - 8 requirements mapped to phases - Research complete for all features Co-Authored-By: Claude Opus 4.5 --- .planning/REQUIREMENTS.md | 59 ++ .planning/ROADMAP.md | 75 +++ .planning/STATE.md | 71 +-- .planning/research/BACKINGTRACK-SYNC.md | 389 +++++++++++++ .planning/research/JAMTRACK-LOADING.md | 367 +++++++++++++ .planning/research/METRONOME-UI.md | 438 +++++++++++++++ .planning/research/PERFORMANCE-AUDIT.md | 689 ++++++++++++++++++++++++ .planning/research/SUMMARY.md | 155 ++++++ 8 files changed, 2193 insertions(+), 50 deletions(-) create mode 100644 .planning/REQUIREMENTS.md create mode 100644 .planning/ROADMAP.md create mode 100644 .planning/research/BACKINGTRACK-SYNC.md create mode 100644 .planning/research/JAMTRACK-LOADING.md create mode 100644 .planning/research/METRONOME-UI.md create mode 100644 .planning/research/PERFORMANCE-AUDIT.md create mode 100644 .planning/research/SUMMARY.md diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md new file mode 100644 index 000000000..cadf21236 --- /dev/null +++ b/.planning/REQUIREMENTS.md @@ -0,0 +1,59 @@ +# Requirements: v1.6 Media Features Polish + +**Defined:** 2026-02-25 +**Core Value:** Fix usability issues in Metronome, Backing Track, and JamTrack features + +## v1.6 Requirements + +Requirements for polishing media player features. Each maps to roadmap phases. + +### JamTrack Fixes + +- [ ] **JT-01**: Defer stem/player UI rendering until backend processing completes +- [ ] **JT-02**: Fix WindowPortal sizing to match player content (460x350) +- [ ] **JT-03**: Implement "Create custom mix" navigation to `/jamtracks/{id}` in new tab +- [ ] **JT-04**: Add callback cleanup for window.jamTrackDownload* in mediaSlice + +### Backing Track Fixes + +- [ ] **BT-01**: Use openBackingTrack() action to enable track sync to session screen +- [ ] **BT-02**: Add ignore flag to duration fetch useEffect + +### Metronome Fixes + +- [ ] **MET-01**: Add debounce (300ms) to BPM/sound/meter control handlers +- [ ] **MET-02**: Add timer cleanup for debounce timeouts on unmount + +## Decisions + +| Decision | Rationale | +|----------|-----------| +| Keep WindowPortal for Metronome | Consistency with JamTrack, BackingTrack, Chat. "about:blank" URL is browser security limitation. | +| Include all performance fixes | Prevents memory leaks and state-after-unmount warnings | +| 300ms debounce | Standard for responsive UI while avoiding excessive native client calls | + +## Out of Scope + +Explicitly excluded for this milestone. + +| Feature | Reason | +|---------|--------| +| Hide "about:blank" URL bar | Browser security constraint, cannot be changed | +| Recording improvements | Separate milestone (v1.5 shipped) | +| Additional media features | Keep v1.6 focused on polish | + +## Traceability + +| Requirement | Phase | Status | +|-------------|-------|--------| +| JT-01 | Phase 26 | Pending | +| JT-02 | Phase 26 | Pending | +| JT-03 | Phase 26 | Pending | +| JT-04 | Phase 26 | Pending | +| BT-01 | Phase 27 | Pending | +| BT-02 | Phase 27 | Pending | +| MET-01 | Phase 28 | Pending | +| MET-02 | Phase 28 | Pending | + +--- +*v1.6 requirements defined 2026-02-25* diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md new file mode 100644 index 000000000..3b063c537 --- /dev/null +++ b/.planning/ROADMAP.md @@ -0,0 +1,75 @@ +# Roadmap: JamKazam Media Features Modernization + +## Milestones + +- v1.0 Media Players (Phases 1-4) - SHIPPED 2026-01-14 +- v1.1 Music Session Chat (Phases 5-10) - SHIPPED 2026-01-27 +- v1.2 Session Attachments (Phases 11-14) - SHIPPED 2026-02-07 +- v1.3 Session Settings Tests (Phases 15-17) - SHIPPED 2026-02-08 +- v1.4 Memory Leak Prevention (Phases 18-22) - SHIPPED 2026-02-10 +- v1.5 Fix Session Recording (Phases 23-25) - SHIPPED 2026-02-25 +- **v1.6 Media Features Polish** (Phases 26-28) - IN PROGRESS + +## Overview + +v1.6 addresses usability issues reported in three media features: JamTrack (loading sequence bug, sizing, navigation), Backing Track (sync integration), and Metronome (sluggish controls). Each feature forms a natural phase boundary with independent, verifiable fixes. + +## Phases + +- [ ] **Phase 26: JamTrack Polish** - Fix loading sequence, sizing, navigation, and cleanup +- [ ] **Phase 27: Backing Track Sync** - Enable track sync and async cleanup +- [ ] **Phase 28: Metronome Responsiveness** - Debounce controls and cleanup timers + +## Phase Details + +### Phase 26: JamTrack Polish +**Goal**: JamTrack player works correctly from selection through playback without freezes +**Depends on**: v1.5 complete +**Requirements**: JT-01, JT-02, JT-03, JT-04 +**Success Criteria** (what must be TRUE): + 1. User sees loading indicator while backend processes track (not premature stem UI) + 2. JamTrack player fits properly in popup window without scrollbars + 3. "Create custom mix" button opens JamTrack editor in new tab + 4. No console warnings about leaked callbacks when closing JamTrack or navigating away +**Plans**: TBD + +Plans: +- [ ] 26-01: TBD + +### Phase 27: Backing Track Sync +**Goal**: Backing Track appears in session screen when opened +**Depends on**: Nothing (independent of Phase 26) +**Requirements**: BT-01, BT-02 +**Success Criteria** (what must be TRUE): + 1. Opening a backing track file shows the track in session screen (not just popup) + 2. No "state update on unmounted component" warnings when closing backing track quickly +**Plans**: TBD + +Plans: +- [ ] 27-01: TBD + +### Phase 28: Metronome Responsiveness +**Goal**: Metronome controls respond smoothly to user input +**Depends on**: Nothing (independent of Phase 26, 27) +**Requirements**: MET-01, MET-02 +**Success Criteria** (what must be TRUE): + 1. BPM slider moves smoothly without lag when dragged + 2. Sound and meter selectors respond immediately to clicks + 3. No console warnings about timer cleanup when closing metronome +**Plans**: TBD + +Plans: +- [ ] 28-01: TBD + +## Progress + +**Execution Order:** Phases 26, 27, 28 can execute in any order (no dependencies) + +| Phase | Milestone | Plans Complete | Status | Completed | +|-------|-----------|----------------|--------|-----------| +| 26. JamTrack Polish | v1.6 | 0/TBD | Not started | - | +| 27. Backing Track Sync | v1.6 | 0/TBD | Not started | - | +| 28. Metronome Responsiveness | v1.6 | 0/TBD | Not started | - | + +--- +*v1.6 roadmap created 2026-02-25* diff --git a/.planning/STATE.md b/.planning/STATE.md index f058208ea..873009dc9 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -5,58 +5,28 @@ See: .planning/PROJECT.md (updated 2026-02-25) **Core value:** Modernize session features from legacy jQuery/Rails to React patterns -**Current focus:** Planning next milestone +**Current focus:** v1.6 Media Features Polish - Phase 26 JamTrack Polish ## Current Position -Phase: Not started -Plan: - -Status: v1.5 complete, ready for next milestone -Last activity: 2026-02-25 - v1.5 Fix Session Recording shipped +Phase: 26 of 28 (JamTrack Polish) +Plan: 0 of TBD in current phase +Status: Ready to plan +Last activity: 2026-02-25 - v1.6 roadmap created -Progress: Milestone complete +Progress: [░░░░░░░░░░] 0% ## Performance Metrics -**v1.0 Media Players (Complete):** -- Total plans completed: 13 -- Total phases: 5 -- Completion date: 2026-01-14 +**v1.0 - v1.5 Summary:** +- Total milestones shipped: 6 +- Total phases completed: 25 +- Total plans completed: 44 -**v1.1 Music Session Chat (Complete):** -- Total plans completed: 11 -- Total phases: 6 (phases 6-11) -- Completion date: 2026-01-31 - -**v1.2 Session Attachments (Complete):** -- Total plans completed: 11 -- Total phases: 5 (phases 12-16) -- Completion date: 2026-02-07 -- Duration: 5 days (2026-02-02 -> 2026-02-07) -- Files modified: 12 -- Lines added: 1,868 - -**v1.3 Session Settings Tests (Complete):** -- Plans completed: 2 (17-01, 18-01) -- Total phases: 2 (phases 17-18) -- Completion date: 2026-02-08 -- Duration: 1 day - -**v1.4 Memory Leak Prevention (Complete):** -- Phases: 5 (phases 19-23) -- Requirements: 11 -- Plans completed: 5 (19-01, 20-01, 21-01, 22-01, 23-01) -- Completion date: 2026-02-10 -- User verified: 15+ minute stability test passed, no freezes - -**v1.5 Fix Session Recording (Complete):** -- Phases: 2 (phases 24-25) -- Plans completed: 2 (24-01, 25-01) -- Completion date: 2026-02-25 -- Duration: 6 days (2026-02-19 -> 2026-02-25) -- Files modified: 18 -- Lines: +1,103 / -107 -- User verified: 15+ minute recording stability, no memory growth +**v1.6 Media Features Polish (In Progress):** +- Phases: 3 (phases 26-28) +- Requirements: 8 +- Plans completed: 0 ## Accumulated Context @@ -64,9 +34,9 @@ Progress: Milestone complete See PROJECT.md Key Decisions table for full history. -Recent decisions (v1.5): -- Conditional cleanup for window.JK callbacks (prevents race conditions with multiple hook instances) -- Ignore flag pattern for async operations (prevents state updates on unmounted components) +Recent decisions (v1.6): +- Keep WindowPortal for Metronome (consistency with JamTrack, BackingTrack, Chat) +- 300ms debounce for responsive UI while avoiding excessive native client calls ### Deferred Issues @@ -76,7 +46,7 @@ Recent decisions (v1.5): 4. **WebSocket chat messages only broadcast to musicians** (Medium) - From v1.2 5. **mp3 backend support** (Medium) - Frontend allows, backend whitelist doesn't support 6. **Pre-existing test failures in JKChatMessageList.test.js** (Low) - Missing activeSession state -7. **Duplicate recording start paths** (Low) - doStartRecording vs useRecordingHelpers.startRecording (from v1.5) +7. **Duplicate recording start paths** (Low) - From v1.5 ### Roadmap Evolution @@ -86,12 +56,13 @@ Recent decisions (v1.5): - **v1.3 Session Settings Tests** (Phases 17-18): Shipped 2026-02-08 - **v1.4 Memory Leak Prevention** (Phases 19-23): Shipped 2026-02-10 - **v1.5 Fix Session Recording** (Phases 24-25): Shipped 2026-02-25 +- **v1.6 Media Features Polish** (Phases 26-28): In Progress ## Session Continuity Last session: 2026-02-25 -Stopped at: v1.5 milestone complete +Stopped at: v1.6 roadmap created, ready to plan Phase 26 Resume file: None **Next steps:** -1. `/gsd:new-milestone` to plan next feature work +1. `/gsd:plan-phase 26` to plan JamTrack Polish diff --git a/.planning/research/BACKINGTRACK-SYNC.md b/.planning/research/BACKINGTRACK-SYNC.md new file mode 100644 index 000000000..d73a389d9 --- /dev/null +++ b/.planning/research/BACKINGTRACK-SYNC.md @@ -0,0 +1,389 @@ +# Backing Track Sync Research + +**Researched:** 2026-02-25 +**Confidence:** HIGH +**Issue:** Backing Track doesn't sync to session screen when opened in popup window + +## Executive Summary + +The issue is clear: **Backing Track never syncs to the session screen because the condition to display it is never satisfied.** + +When a Backing Track is opened via popup: +1. File is opened through `jamClient.SessionOpenBackingTrackFile()` +2. `backingTrackData` is set in Redux (for popup display) +3. **BUT** `mixerHelper.backingTracks` (required for session screen display) is NOT populated + +The condition `showBackingTrackPlayer && mixerHelper.backingTracks.length > 0` at line 1434 of JKSessionScreen.js is never true because `mixerHelper.backingTracks` remains empty. + +Metronome works because it follows a different pattern with `metronomeState.isOpen` flag and `metronomeTrackMixers` from Redux. + +## Key Findings + +### 1. Backing Track Components + +**JKSessionBackingTrackPlayer** (jam-ui/src/components/client/JKSessionBackingTrackPlayer.js) +- Popup player with controls (play/pause/stop/seek/loop) +- Opened in WindowPortal when user selects file +- Uses `jamClient` for playback control +- Does NOT trigger session screen sync + +**JKSessionBackingTrack** (jam-ui/src/components/client/JKSessionBackingTrack.js) +- Session Mix area component (the visual track in session screen) +- Shows mixer controls (VU meter, gain, pan) +- Requires `backingTrack` and `mixers` props +- Only renders when `mixerHelper.backingTracks.length > 0` + +### 2. How Metronome Successfully Syncs + +**Popup Display:** +- `metronomeState.isOpen` flag (from GlobalContext) +- Renders JKSessionMetronomePlayer in WindowPortal +- Line 1727-1744 in JKSessionScreen.js + +**Session Screen Display:** +- Condition: `metronomeState.isOpen && metronomeTrackMixers && metronomeTrackMixers.length > 0` +- Uses `metronomeTrackMixers` from Redux mixersSlice selector +- Line 1463-1488 in JKSessionScreen.js +- Renders JKSessionMetronome component + +**Why it works:** +1. `openMetronome()` in useMediaActions.js (line 92): + - Calls `jamClient.SessionOpenMetronome()` + - Updates Redux: `updateMediaSummary({ metronomeOpen: true })` + - Syncs tracks: `dispatch(syncTracksToServer(sessionId, jamClient))` +2. Track sync triggers server update +3. Session refresh fetches mixer data +4. Mixer data includes metronome mixers +5. `metronomeTrackMixers` selector returns populated array +6. JKSessionMetronome renders in Session Mix area + +### 3. What's Missing for Backing Track + +**Current Flow (BROKEN):** + +``` +User selects file + ↓ +handleBackingTrackSelected() in JKSessionScreen.js (line 1136) + ↓ +jamClient.SessionOpenBackingTrackFile(result.file, false) + ↓ +dispatch(setBackingTrackData({ backingTrack, session, currentUser })) + ↓ +dispatch(openModal('backingTrack')) + ↓ +WindowPortal renders JKSessionBackingTrackPlayer + ↓ +[STOPS HERE - mixerHelper.backingTracks NEVER POPULATED] +``` + +**Missing Steps:** +1. No track sync triggered after opening backing track +2. No session refresh to fetch mixer data +3. `mixerHelper.backingTracks` never populated from session data +4. Condition at line 1434 never satisfied + +**Compare with Metronome (WORKING):** + +``` +User opens metronome + ↓ +openMetronome() in useMediaActions.js + ↓ +jamClient.SessionOpenMetronome() + ↓ +dispatch(updateMediaSummary({ metronomeOpen: true })) + ↓ +dispatch(syncTracksToServer(sessionId, jamClient)) ← KEY DIFFERENCE + ↓ +Server updates session with metronome mixer data + ↓ +Session refresh fetches mixers + ↓ +metronomeTrackMixers populated from Redux + ↓ +JKSessionMetronome renders in Session Mix +``` + +### 4. Redux State Structure + +**activeSessionSlice.js:** +- `backingTrackData`: Stores popup player data (file path, session, user) +- Used for: Determining if popup should show +- NOT used for: Session Mix area display + +**mediaSlice.js:** +- `backingTracks`: Array of backing track objects with mixers +- Updated by: `setBackingTracks` action from WebSocket or session refresh +- Used for: Session Mix area display via `mixerHelper.backingTracks` + +**mixersSlice.js:** +- `backingTrackMixers`: Array of mixer objects for backing tracks +- `mediaSummary.backingTrackOpen`: Boolean flag +- Used for: Mixer controls in session screen + +### 5. Data Flow for Session Screen Display + +**How data gets to mixerHelper.backingTracks:** + +``` +Session REST API response + ↓ +useSessionModel.js refreshCurrentSession() (line 695) + ↓ +Extract backing tracks from participants + ↓ +dispatch(setBackingTracks(extractedBackingTracks)) + ↓ +mediaSlice.backingTracks updated + ↓ +useMixerHelper.js selects from mediaSlice (line 95) + ↓ +mixerHelper.backingTracks available to components +``` + +**Current problem:** +When backing track opened via popup, session is not refreshed, so backing tracks are never extracted from participants and `mixerHelper.backingTracks` remains empty. + +### 6. Track Sync Service + +**trackSyncService.js** (jam-ui/src/services/trackSyncService.js) +- `syncTracksToServer()`: Syncs track state to backend +- Builds payload with tracks, backing_tracks, metronome_open +- Called after opening metronome (line 107 in useMediaActions.js) +- NOT called after opening backing track + +**Backend expectation:** +- PUT `/api/sessions/:id/tracks` with track sync payload +- Server updates session participants with backing track info +- Subsequent GET fetches updated mixer data + +## Root Cause Analysis + +### Why Backing Track Doesn't Sync + +**Missing Integration Points:** + +1. **No track sync call** in `handleBackingTrackSelected()` (JKSessionScreen.js line 1136) + - Metronome calls: `dispatch(syncTracksToServer(sessionId, jamClient))` + - Backing track: Missing this call + +2. **Wrong Redux state used for display condition** + - Uses: `showBackingTrackPlayer` (derived from `backingTrackData`) + - Should use: `mediaSummary.backingTrackOpen` + `mixerHelper.backingTracks.length > 0` + - Like metronome uses: `metronomeState.isOpen` + `metronomeTrackMixers.length > 0` + +3. **No session refresh after opening** + - Metronome: Track sync triggers backend update, then session refresh + - Backing track: No sync, no refresh, no mixer data + +4. **useMediaActions.openBackingTrack() exists but not used** + - File: jam-ui/src/hooks/useMediaActions.js line 41 + - Already implements track sync: `dispatch(syncTracksToServer(sessionId, jamClient))` + - Already updates media summary: `updateMediaSummary({ backingTrackOpen: true })` + - **Not called from JKSessionScreen.js** - direct jamClient call used instead + +## Recommended Fix Approach + +### Option 1: Use Existing useMediaActions Pattern (RECOMMENDED) + +**Change in JKSessionScreen.js:** + +```javascript +// BEFORE (line 1136): +const handleBackingTrackSelected = async (result) => { + try { + await jamClient.SessionOpenBackingTrackFile(result.file, false); + dispatch(setBackingTrackData({ + backingTrack: result.file, + session: currentSession, + currentUser: currentUser + })); + dispatch(openModal('backingTrack')); + } catch (error) { + toast.error('Failed to open backing track'); + } +}; + +// AFTER: +const handleBackingTrackSelected = async (result) => { + try { + // Use the media action that includes track sync + await openBackingTrack(result.file); + + // Set up popup data + dispatch(setBackingTrackData({ + backingTrack: result.file, + session: currentSession, + currentUser: currentUser + })); + dispatch(openModal('backingTrack')); + } catch (error) { + toast.error('Failed to open backing track'); + } +}; +``` + +**Why this works:** +1. `openBackingTrack()` (useMediaActions.js line 41) already: + - Opens file: `jamClient.SessionOpenBackingTrackFile()` + - Updates media summary: `updateMediaSummary({ backingTrackOpen: true })` + - Syncs tracks: `dispatch(syncTracksToServer(sessionId, jamClient))` +2. Track sync triggers backend update +3. Session refresh fetches mixer data +4. `mixerHelper.backingTracks` gets populated +5. Session screen condition satisfied + +**Additional change needed in display condition:** + +```javascript +// BEFORE (line 1434): +{showBackingTrackPlayer && mixerHelper.backingTracks && mixerHelper.backingTracks.length > 0 && ( + // ... +)} + +// AFTER (align with metronome pattern): +{mediaSummary.backingTrackOpen && mixerHelper.backingTracks && mixerHelper.backingTracks.length > 0 && ( + // ... +)} +``` + +### Option 2: Add Track Sync to Current Flow + +If Option 1 causes issues, manually add track sync: + +```javascript +const handleBackingTrackSelected = async (result) => { + try { + await jamClient.SessionOpenBackingTrackFile(result.file, false); + + // Add track sync (like metronome does) + if (sessionId && jamClient) { + console.log('[Track Sync] Backing track opened, syncing tracks'); + dispatch(syncTracksToServer(sessionId, jamClient)); + } + + dispatch(setBackingTrackData({ + backingTrack: result.file, + session: currentSession, + currentUser: currentUser + })); + dispatch(openModal('backingTrack')); + } catch (error) { + toast.error('Failed to open backing track'); + } +}; +``` + +## Pattern Comparison: Metronome vs Backing Track + +| Aspect | Metronome (WORKS) | Backing Track (BROKEN) | +|--------|------------------|----------------------| +| Open Action | `openMetronome()` in useMediaActions | Direct `jamClient.SessionOpenBackingTrackFile()` | +| Track Sync | ✅ Called in useMediaActions | ❌ Not called | +| Media Summary Update | ✅ `metronomeOpen: true` | ❌ Not updated | +| Popup State | `metronomeState.isOpen` (GlobalContext) | `backingTrackData` (Redux) | +| Session Screen State | `metronomeTrackMixers` (Redux selector) | `mixerHelper.backingTracks` (never populated) | +| Display Condition | `metronomeState.isOpen && metronomeTrackMixers.length > 0` | `showBackingTrackPlayer && mixerHelper.backingTracks.length > 0` | +| Result | ✅ Shows in both popup and session screen | ❌ Shows only in popup | + +## File Locations + +### Components +- **Backing Track Player (popup):** `jam-ui/src/components/client/JKSessionBackingTrackPlayer.js` +- **Backing Track (session screen):** `jam-ui/src/components/client/JKSessionBackingTrack.js` +- **Metronome Player (popup):** `jam-ui/src/components/client/JKSessionMetronomePlayer.js` +- **Metronome (session screen):** `jam-ui/src/components/client/JKSessionMetronome.js` +- **Session Screen:** `jam-ui/src/components/client/JKSessionScreen.js` + +### State Management +- **Media Actions:** `jam-ui/src/hooks/useMediaActions.js` +- **Track Sync Service:** `jam-ui/src/services/trackSyncService.js` +- **Active Session Slice:** `jam-ui/src/store/features/activeSessionSlice.js` +- **Media Slice:** `jam-ui/src/store/features/mediaSlice.js` +- **Mixers Slice:** `jam-ui/src/store/features/mixersSlice.js` +- **Session Model:** `jam-ui/src/hooks/useSessionModel.js` +- **Mixer Helper:** `jam-ui/src/hooks/useMixerHelper.js` + +### Key Line Numbers +- **Backing track handler:** JKSessionScreen.js line 1136-1163 +- **Backing track display condition:** JKSessionScreen.js line 1434 +- **Metronome display condition:** JKSessionScreen.js line 1463 +- **openBackingTrack() action:** useMediaActions.js line 41-60 +- **syncTracksToServer() service:** trackSyncService.js +- **Session refresh with backing tracks:** useSessionModel.js line 685-696 + +## Testing Strategy + +### 1. Verify Current Behavior (Failing Test) + +```javascript +// test/backing-track/backing-track-sync.spec.ts +test('backing track appears in session screen after opening popup', async ({ page }) => { + await loginToJamUI(page); + await createAndJoinSession(page); + + // Open backing track from Open menu + await page.click('[data-testid="open-menu-button"]'); + await page.click('[data-testid="open-backing-track"]'); + await selectBackingTrackFile(page, 'test-audio.mp3'); + + // Verify popup appears + await expect(page.locator('.metronome-player-popup')).toBeVisible(); + + // Verify track appears in session screen + await expect(page.locator('.backingTrack')).toBeVisible(); + await expect(page.locator('.backingTrack .session-track')).toBeVisible(); +}); +``` + +### 2. Verify Track Sync Called + +```javascript +test('track sync triggered when backing track opened', async ({ page }) => { + const interceptor = new APIInterceptor(); + interceptor.intercept(page); + + await loginToJamUI(page); + await createAndJoinSession(page); + + await openBackingTrack(page, 'test-audio.mp3'); + + // Verify track sync API called + const trackSyncCalls = interceptor.getCallsByPath('/api/sessions/*/tracks'); + expect(trackSyncCalls.length).toBeGreaterThan(0); +}); +``` + +### 3. Verify Mixer Data Populated + +```javascript +test('backing track mixers populated after sync', async ({ page }) => { + await loginToJamUI(page); + const sessionId = await createAndJoinSession(page); + + await openBackingTrack(page, 'test-audio.mp3'); + + // Wait for session refresh + await page.waitForTimeout(1000); + + // Verify mixer data in Redux + const mixers = await page.evaluate(() => { + return window.__REDUX_DEVTOOLS_EXTENSION__.store.getState().mixers.backingTrackMixers; + }); + + expect(mixers.length).toBeGreaterThan(0); +}); +``` + +## Conclusion + +The fix is straightforward: +1. Use `openBackingTrack()` from useMediaActions instead of direct jamClient call +2. This automatically triggers track sync (like metronome does) +3. Track sync updates backend, session refresh populates mixer data +4. Session screen condition satisfied, backing track appears + +**Estimated effort:** 1-2 hours (simple refactor + testing) +**Risk:** Low (following existing working pattern from metronome) +**Impact:** HIGH (fixes critical missing feature) diff --git a/.planning/research/JAMTRACK-LOADING.md b/.planning/research/JAMTRACK-LOADING.md new file mode 100644 index 000000000..c36a54ff4 --- /dev/null +++ b/.planning/research/JAMTRACK-LOADING.md @@ -0,0 +1,367 @@ +# JamTrack Loading Sequence and Freeze Issue - Research + +**Project:** jam-cloud (JamKazam) +**Researched:** 2026-02-25 +**Confidence:** HIGH + +## Executive Summary + +The JamTrack freeze issue is caused by **premature UI rendering** - stems appear in the session screen immediately upon selection, but the backend hasn't finished creating/downloading/syncing the track files. When the user clicks play before backend processes complete, the system enters a "checking sync status" state that blocks the UI. + +**Root cause:** The current flow triggers UI updates (`setSelectedJamTrack`, `setJamTrackStems`) immediately when the JamTrack is selected, but the actual backend processing (packaging, downloading, keying) happens asynchronously during the first play attempt in `JKSessionJamTrackPlayer`. + +**Correct sequence should be:** Select → Backend processing (hidden) → Show UI (stems + player) → Ready to play + +## Problem Flow (Current - Broken) + +``` +1. User selects JamTrack from modal + ↓ +2. handleJamTrackSelect() in JKSessionScreen.js (line 1165-1190) + - Fetches JamTrack data via REST API + - IMMEDIATELY dispatches: setSelectedJamTrack(), setJamTrackStems(), setJamTrackData() + ↓ +3. UI updates IMMEDIATELY (line 1405-1431) + - Stems appear in session screen + - WindowPortal opens with player + ↓ +4. User clicks Play button + ↓ +5. JKSessionJamTrackPlayer handlePlay() (line 175-211) + - Calls loadJamTrack thunk + ↓ +6. mediaSlice loadJamTrack() (line 17-49) + - Checks if synchronized + - If NOT synchronized → triggers downloadJamTrack() + - Shows "checking sync status..." (line 602) + ↓ +7. mediaSlice downloadJamTrack() (line 51-266) + - PACKAGING phase (line 123-223): enqueue mixdown, wait for server signing + - DOWNLOADING phase (line 225-255): download via native client + - KEYING phase (line 256-258): request decryption keys + ↓ +8. FREEZE occurs if: + - Packaging takes too long (60s timeout) + - Download fails + - Keys not available + - User interacts during process +``` + +## File Locations + +### Core Components + +| File | Purpose | Lines of Interest | +|------|---------|-------------------| +| `/jam-ui/src/components/client/JKSessionScreen.js` | Main session screen orchestrator | 1165-1190 (handleJamTrackSelect), 1405-1431 (stems rendering), 1746-1761 (WindowPortal) | +| `/jam-ui/src/components/client/JKSessionJamTrackPlayer.js` | JamTrack player component (WindowPortal content) | 69-160 (initialization), 175-211 (handlePlay), 602 ("checking sync status" text) | +| `/jam-ui/src/components/client/JKSessionJamTrackStems.js` | Renders stems in session screen | 5-49 (stem track mapping) | +| `/jam-ui/src/components/client/JKSessionJamTrackModal.js` | JamTrack selection modal | 44-52 (onSelect callback) | + +### Redux State Management + +| File | Purpose | Lines of Interest | +|------|---------|-------------------| +| `/jam-ui/src/store/features/mediaSlice.js` | Media operations (load, download, sync) | 17-49 (loadJamTrack), 51-266 (downloadJamTrack), 268-280 (checkJamTrackSync) | +| `/jam-ui/src/store/features/activeSessionSlice.js` | Session data (selected track, stems) | 172-178 (setSelectedJamTrack, setJamTrackStems), 211-217 (jamTrackData for player) | +| `/jam-ui/src/store/features/sessionUISlice.js` | UI state (modal visibility) | 51 (openJamTrack state) | + +### Infrastructure + +| File | Purpose | Lines of Interest | +|------|---------|-------------------| +| `/jam-ui/src/components/common/WindowPortal.js` | Popup window manager | 7 (windowFeatures for sizing) | +| `/jam-ui/src/helpers/rest.js` | API endpoints | 531-538 (getJamTrack), 935-945 (closeJamTrack) | + +## Backend API Calls and Timing + +### 1. Initial Selection (REST API - synchronous) + +**Endpoint:** `GET /jamtracks/:id` +**Called by:** `handleJamTrackSelect()` in JKSessionScreen.js (line 1169) +**Purpose:** Fetch JamTrack metadata with stems/mixdowns +**Timing:** ~100-500ms +**Response includes:** +- JamTrack metadata (id, name, artist, etc.) +- Mixdowns array (master, custom-mix, stem types) +- Each mixdown has packages array (file_type, encrypt_type, sample_rate) +- Tracks array (stems with instruments) + +### 2. Packaging Phase (REST + WebSocket - asynchronous) + +**Endpoint:** `POST /jam_track_mixdowns/:id/enqueue` +**Called by:** `downloadJamTrack()` in mediaSlice.js (line 143-148) +**Purpose:** Request server-side package creation/signing +**Timing:** 5-30 seconds (can timeout at 60s) +**Process:** +1. Server queues mixdown for packaging +2. WebSocket subscription created for progress notifications (line 157-166) +3. Polling loop waits for `signing_state === 'SIGNED'` (line 179-221) +4. State updates shown as "Your JamTrack is currently being created" (line 603) + +**Failure modes:** +- `SIGNING_TIMEOUT`, `QUEUED_TIMEOUT`, `QUIET_TIMEOUT` → error +- WebSocket not available → error (line 159-161) + +### 3. Download Phase (Native Client - asynchronous) + +**Method:** `jamClient.JamTrackDownload()` +**Called by:** `downloadJamTrack()` in mediaSlice.js (line 247-254) +**Purpose:** Download encrypted audio files to local storage +**Timing:** 10-60 seconds (depends on file size, network) +**Process:** +1. Native client initiates download with progress callbacks +2. Progress updates via `window.jamTrackDownloadProgress` (line 234-236) +3. Success triggers `window.jamTrackDownloadSuccess` (line 238-240) +4. Failure triggers `window.jamTrackDownloadFail` (line 242-244) + +### 4. Keying Phase (Native Client - synchronous) + +**Method:** `jamClient.JamTrackKeysRequest()` +**Called by:** `downloadJamTrack()` in mediaSlice.js (line 257) +**Purpose:** Request decryption keys for encrypted files +**Timing:** ~1-5 seconds +**State shown:** "Requesting decryption keys..." (line 605) + +### 5. Play Phase (Native Client - synchronous) + +**Method:** `jamClient.JamTrackPlay(fqId)` +**Called by:** `loadJamTrack()` in mediaSlice.js (line 39-42) +**Purpose:** Start audio playback +**Timing:** ~100-500ms +**Requires:** Files synchronized (trackDetail.key_state === 'AVAILABLE') + +## Root Cause of Freeze + +The freeze occurs because: + +1. **Immediate UI update** (line 1175-1176 in JKSessionScreen.js): + ```javascript + dispatch(setSelectedJamTrack(jamTrackWithStems)); + dispatch(setJamTrackStems(jamTrackWithStems.tracks || [])); + ``` + This causes stems to render immediately at line 1405-1431. + +2. **Conditional rendering check is wrong** (line 1406): + ```javascript + {selectedJamTrack && jamTrackStems.length > 0 && + (jamTrackDownloadState.state === 'synchronized' || jamTrackDownloadState.state === 'idle') + ``` + The condition allows rendering when `state === 'idle'`, but "idle" means **not yet checked**, not "ready to play". + +3. **Play button triggers long async process** (line 183-190): + When user clicks play, if track isn't synchronized, it triggers packaging → downloading → keying, which can take 30-90 seconds. During this time: + - UI shows "checking sync status..." (line 602) + - User can't interact (buttons disabled via `isOperating` flag) + - If process fails or times out, UI is stuck + +4. **WindowPortal sizing issue**: + Player window is hardcoded to `width=600,height=500` (line 1750), but player content width is `420px` (line 487 in JKSessionJamTrackPlayer.js), causing poor fit. + +## Recommended Fix Approach + +### Strategy: Backend-First Loading + +**Goal:** Don't show UI until backend is ready. + +### Phase 1: Defer UI Rendering + +**Change `handleJamTrackSelect()` to:** +1. Fetch JamTrack metadata (as now) +2. Show loading indicator in modal/toast +3. **DON'T dispatch `setSelectedJamTrack` yet** +4. Trigger `loadJamTrack()` thunk (which includes sync check + download if needed) +5. Wait for `loadJamTrack()` to complete +6. **THEN** dispatch UI state: `setSelectedJamTrack`, `setJamTrackStems`, `setJamTrackData` +7. Close modal, show player + stems + +**Code change location:** JKSessionScreen.js lines 1165-1190 + +```javascript +const handleJamTrackSelect = async (jamTrack) => { + try { + // 1. Fetch metadata + const response = await getJamTrack({ id: jamTrack.id }); + const jamTrackWithStems = await response.json(); + + // 2. Show loading + toast.info('Preparing JamTrack...'); + + // 3. Pre-load backend (sync check + download if needed) + await dispatch(loadJamTrack({ + jamTrack: jamTrackWithStems, + mixdownId: null, // Use default + autoPlay: false, // Don't auto-play, just prepare + jamClient, + jamServer + })).unwrap(); + + // 4. NOW show UI (backend is ready) + dispatch(setSelectedJamTrack(jamTrackWithStems)); + dispatch(setJamTrackStems(jamTrackWithStems.tracks || [])); + dispatch(setJamTrackData({ + jamTrack: jamTrackWithStems, + session: currentSession, + currentUser: currentUser + })); + + toast.success(`Loaded JamTrack: ${jamTrackWithStems.name}`); + } catch (error) { + toast.error(`Failed to load JamTrack: ${error.message}`); + } +}; +``` + +### Phase 2: Fix Conditional Rendering + +**Change line 1406 in JKSessionScreen.js:** + +```javascript +// OLD (wrong): +{selectedJamTrack && jamTrackStems.length > 0 && + (jamTrackDownloadState.state === 'synchronized' || jamTrackDownloadState.state === 'idle') + +// NEW (correct): +{selectedJamTrack && jamTrackStems.length > 0 && + jamTrackDownloadState.state === 'synchronized' +``` + +Remove the `|| jamTrackDownloadState.state === 'idle'` condition. UI should only show when explicitly synchronized. + +### Phase 3: Fix WindowPortal Sizing + +**Change line 1750 in JKSessionScreen.js:** + +```javascript +// OLD: +windowFeatures="width=600,height=500,left=200,top=200,..." + +// NEW (matches player content width): +windowFeatures="width=460,height=350,left=200,top=200,..." +``` + +Player content is 420px wide (line 487 in JKSessionJamTrackPlayer.js), so 460px window width provides proper padding. + +### Phase 4: Progress Feedback During Prep + +While backend is processing (packaging/downloading), show progress in the modal or as a toast: + +**Option A: Modal shows progress** +- Keep modal open during prep +- Show packaging/downloading progress bars +- Close modal when ready + +**Option B: Toast notifications** +- Close modal immediately +- Show toast: "Preparing JamTrack... (packaging)" +- Update toast: "Preparing JamTrack... (downloading 45%)" +- Final toast: "JamTrack ready!" → open player + stems + +## "Create Custom Mix" Navigation + +### Current Implementation + +**Location:** JKSessionJamTrackPlayer.js line 860 +**Current code:** +```javascript + console.log('TODO: Open custom mix creator')}> + create custom mix + +``` + +### Required Behavior + +Should open `/jamtracks/{id}` in a new browser tab. + +**Implementation:** + +```javascript + { + const url = `/jamtracks/${jamTrack.id}`; + window.open(url, '_blank', 'noopener,noreferrer'); + }} + style={{ + fontSize: '12px', + color: '#007aff', + textDecoration: 'none', + cursor: 'pointer' + }} +> + create custom mix + +``` + +**Route:** Already exists in routes.js line 36 (`/jamtracks`) + +**Component:** Should navigate to JamTrack show page where user can create custom mixdowns (existing functionality). + +## Testing Strategy + +### Test 1: Fresh JamTrack (Not Synchronized) +1. Select JamTrack never downloaded before +2. **Expected:** Modal shows loading, then closes when ready +3. **Expected:** Stems + player appear simultaneously +4. **Expected:** Play button works immediately without "checking sync status" + +### Test 2: Already Synchronized JamTrack +1. Select JamTrack already downloaded +2. **Expected:** Fast load (no packaging/downloading) +3. **Expected:** Stems + player appear quickly +4. **Expected:** Play button works immediately + +### Test 3: Packaging Failure +1. Mock server error during packaging +2. **Expected:** Error toast shown +3. **Expected:** UI does NOT show stems/player (stays clean) +4. **Expected:** User can retry or select different track + +### Test 4: WindowPortal Sizing +1. Open JamTrack player +2. **Expected:** Player fits properly in window (no excessive white space) +3. **Expected:** Controls are visible without scrolling + +### Test 5: Create Custom Mix Link +1. Open JamTrack player +2. Click "create custom mix" +3. **Expected:** New tab opens to `/jamtracks/{id}` +4. **Expected:** User can create custom mixdown + +## Related Issues + +### WebSocket Subscription Management + +**File:** mediaSlice.js lines 157-173 +**Issue:** Subscription/unsubscription for packaging progress must be robust +**Current:** Uses polling + timeout +**Risk:** Memory leaks if unsubscribe not called on error/timeout + +### Native Client Callback Management + +**File:** mediaSlice.js lines 234-244 +**Issue:** Global callbacks (`window.jamTrackDownloadProgress`) are fragile +**Risk:** Multiple JamTracks could conflict if callbacks overwrite each other + +### Error Recovery + +**Current:** User must close player and retry +**Better:** Provide "Retry" button in error state (already exists in player, line 363-379) + +## Summary + +**The freeze is NOT a native client bug.** It's a **UI state management issue** where the UI renders before the backend is ready. + +**Fix priority:** +1. **HIGH:** Defer UI rendering until backend ready (Phase 1) +2. **HIGH:** Fix conditional rendering logic (Phase 2) +3. **MEDIUM:** Fix WindowPortal sizing (Phase 3) +4. **LOW:** Add "create custom mix" navigation (it's a TODO, not a bug) + +**Estimated effort:** +- Phase 1: 2-3 hours (requires Redux thunk orchestration) +- Phase 2: 5 minutes (one line change) +- Phase 3: 5 minutes (one line change) +- Phase 4: 30 minutes (optional progress UI) +- Create custom mix: 5 minutes (one line change) + +**Total:** 3-4 hours to fix freeze + sizing issues diff --git a/.planning/research/METRONOME-UI.md b/.planning/research/METRONOME-UI.md new file mode 100644 index 000000000..acf236a74 --- /dev/null +++ b/.planning/research/METRONOME-UI.md @@ -0,0 +1,438 @@ +# Metronome UI Component Research + +**Domain:** jam-ui React 16 SPA - Metronome Controls UI Fix +**Researched:** 2026-02-25 +**Overall confidence:** HIGH + +## Executive Summary + +The Metronome UI is implemented across three components: `JKSessionMetronomePlayer.js` (controls popup), `JKSessionMetronome.js` (mixer track), and the WindowPortal pattern for popup windows. The current issue is that WindowPortal creates browser popup windows with "about:blank" URLs and unstyled chrome. The controls themselves are already styled with circular play/stop buttons and a clean form layout. + +**Key Findings:** +- Metronome controls are **already well-styled** with circular icons (lines 223-239 in JKSessionMetronomePlayer.js) +- CSS exists (JKSessionMetronomePlayer.css) with modern styling patterns +- WindowPortal is the **standard pattern** for modeless dialogs (used by Chat, JamTrack, BackingTrack) +- WindowPortal **cannot hide** "about:blank" URL bar - it's browser chrome behavior +- Alternative: Use reactstrap Modal for modal dialog instead of popup window + +## 1. File Locations + +### Core Metronome Components + +| File | Purpose | Lines | +|------|---------|-------| +| `/jam-ui/src/components/client/JKSessionMetronomePlayer.js` | Main controls component with play/stop/settings | 332 lines | +| `/jam-ui/src/components/client/JKSessionMetronomePlayer.css` | Styling for controls (circular buttons, form layout) | 232 lines | +| `/jam-ui/src/components/client/JKSessionMetronome.js` | Metronome mixer track (VU meter, gain control) | 117 lines | +| `/jam-ui/src/components/common/WindowPortal.js` | Popup window wrapper component | 153 lines | +| `/jam-ui/src/hooks/useMetronomeState.js` | State management hook | 73 lines | + +### Integration Point + +**JKSessionScreen.js** (lines 1727-1744) opens the Metronome player: + +```javascript +{metronomeState.isOpen && ( + + + +)} +``` + +## 2. WindowPortal Analysis + +### How WindowPortal Works + +**File:** `/jam-ui/src/components/common/WindowPortal.js` + +**Mechanism:** +1. Calls `window.open('', '_blank', windowFeatures)` to create popup (line 55) +2. Sets basic document styles on the popup window (lines 64-69) +3. Creates container div and uses `ReactDOM.createPortal()` to render children (line 149) +4. Manages lifecycle (close detection, cleanup) + +**What It Can Do:** +- ✅ Create modeless popup windows +- ✅ Apply basic body styles (`margin: 0`, `padding: 0`, `backgroundColor`, `fontFamily`) +- ✅ Render React components in popup +- ✅ Persist window position between opens +- ✅ Detect window close events +- ✅ Send/receive messages via postMessage + +**What It CANNOT Do:** +- ❌ Hide URL bar (controlled by browser security) +- ❌ Remove browser chrome completely +- ❌ Control window decorations (except via `windowFeatures` string) +- ❌ Style window title bar + +### Why "about:blank" Appears + +**Root Cause:** `window.open('', '_blank', ...)` creates an empty document with `about:blank` URL. + +**Browser Security:** Modern browsers **always show** the URL bar for popup windows for security reasons. The `windowFeatures` flags (`location=no`, `addressbar=no`) are **deprecated** and ignored in modern Chrome/Firefox/Safari. + +**Historical Context:** In legacy IE/older Firefox, these flags could hide the address bar. Not possible anymore. + +### WindowPortal Usage in Codebase + +WindowPortal is the **standard pattern** for all modeless dialogs: + +| Feature | File | Window Size | +|---------|------|-------------| +| Chat | JKSessionChatWindow.js | 450×600 | +| JamTrack Player | JKSessionJamTrackPlayer.js | 600×500 | +| Backing Track Player | JKSessionBackingTrackPlayer.js | 400×300 | +| Metronome Controls | JKSessionMetronomePlayer.js | 450×400 | + +**Pattern Consistency:** All media players use WindowPortal. Changing Metronome to Modal would break UX consistency. + +## 3. Current Metronome UI Styling + +### Existing Styles Are Already Good + +**File:** `/jam-ui/src/components/client/JKSessionMetronomePlayer.css` + +The controls are **already styled** with modern patterns: + +#### Circular Play/Stop Buttons (Lines 71-113) + +```css +.metronome-icon-btn { + width: 40px; + height: 40px; + border-radius: 50%; + border: 2px solid #007bff; + background-color: #fff; + display: flex; + align-items: center; + justify-content: center; + cursor: pointer; + transition: all 0.2s; +} + +.metronome-icon-btn:hover:not(.disabled) { + background-color: #007bff; + color: #fff; +} +``` + +#### Clean Form Layout (Lines 115-137) + +```css +.metronome-form-controls { + display: flex; + flex-direction: column; + gap: 12px; + flex: 1; +} + +.metronome-form-row { + display: grid; + grid-template-columns: 80px 1fr; + align-items: center; + gap: 12px; +} + +.metronome-form-label { + font-weight: 400; + font-size: 0.875rem; + color: #666; + text-align: right; + margin: 0; +} +``` + +### Component Structure (Already Clean) + +**JKSessionMetronomePlayer.js** lines 211-295: + +``` +┌─ metronome-player-popup ──────────────┐ +│ ┌─ header ──────────────────────┐ │ +│ │ h3: "Metronome Controls" │ │ +│ │ Close X button │ │ +│ └───────────────────────────────┘ │ +│ ┌─ body ────────────────────────┐ │ +│ │ ┌─ main-content ───────────┐ │ │ +│ │ │ ⚪ Play ⚪ Stop │ │ │ +│ │ │ Feature: [Metronome ▼] │ │ │ +│ │ │ Sound: [Beep ▼] │ │ │ +│ │ │ Tempo: [120 ▼] │ │ │ +│ │ └─────────────────────────┘ │ │ +│ │ [ Close ] │ │ +│ └───────────────────────────────┘ │ +└───────────────────────────────────────┘ +``` + +**The controls are not "unstyled/raw HTML"** - they already have clean, modern styling. + +## 4. What's Actually Wrong? + +Based on milestone description: + +| Issue | Reality | Fix Needed? | +|-------|---------|-------------| +| "about:blank" in URL bar | **Cannot be fixed** (browser security) | No - explain to user | +| "Unstyled/raw HTML controls" | **False** - controls are already styled | No - verify visually | +| "Layout cramped/misaligned" | **Possible** - depends on content overflow | Maybe - check WindowPortal size | +| "Need circular play/stop icons" | **Already implemented** (lines 223-239) | No - already done | + +### Hypothesis: WindowPortal Size Issue + +**Current size:** `width=450,height=400` (line 1731 of JKSessionScreen.js) + +**Content:** +- Header: ~60px +- Play/Stop buttons: ~50px +- 3 form rows (Feature, Sound, Tempo): ~36px × 3 = 108px +- Close button: ~40px +- Padding/gaps: ~60px +- **Total:** ~318px minimum height + +**400px height should be sufficient.** Possible cramping if: +- Browser added its own chrome (title bar, URL bar) reducing viewport +- Popup window is smaller than requested (popup blockers) +- Content has unexpected overflow + +## 5. Styling Patterns in Codebase + +### Circular Icon Buttons Pattern + +**Used in:** BackingTrackPlayer, JamTrackPlayer, MetronomePlayer + +**JamTrack Pattern (JKSessionJamTrackPlayer.css lines 84-107):** + +```css +.jamtrack-btn-play { + width: 36px; + height: 36px; + border-radius: 50%; + background: #007aff; + color: white; +} + +.jamtrack-btn-play:hover:not(:disabled) { + background: #0051d5; + transform: scale(1.05); +} +``` + +**Metronome uses similar pattern** but with blue (#007bff) instead of iOS blue (#007aff). + +### Form Layout Patterns + +**Grid-based label + control:** (Used in Metronome, consistent with other forms) + +```css +.metronome-form-row { + display: grid; + grid-template-columns: 80px 1fr; /* Label width, control fills rest */ + align-items: center; + gap: 12px; +} +``` + +**This is modern and clean.** No changes needed. + +### Color Palette + +| Component | Primary Action | Secondary Action | Background | +|-----------|---------------|------------------|------------| +| Metronome | #007bff (blue) | #6c757d (gray) | #f5f5f5 | +| JamTrack | #007aff (iOS blue) | #6e6e73 (gray) | #ffffff | +| BackingTrack | #5b9bd5 (light blue) | #b0b0b0 (gray) | #f8f9fa | + +**Metronome colors are standard Bootstrap blue** - acceptable. + +## 6. Modal Alternative to WindowPortal + +### Option: Use reactstrap Modal + +**File:** `/jam-ui/src/components/client/JKSessionMetronomePlayer.js` (lines 318-328) + +**Already has Modal fallback:** + +```javascript +// Otherwise, render as Modal (fallback) +return ( + + + Metronome Controls + + + {renderControls()} + + +); +``` + +**This code path is triggered when `isPopup={false}`.** + +### Modal vs WindowPortal Tradeoffs + +| Feature | Modal (reactstrap) | WindowPortal | +|---------|-------------------|--------------| +| URL bar visible | ❌ No URL bar | ✅ "about:blank" visible | +| Repositionable | ❌ Centered only | ✅ User can drag anywhere | +| Multi-monitor support | ❌ Main window only | ✅ Can move to 2nd monitor | +| Always on top | ❌ No (blocked by main window) | ✅ Separate window | +| Persistent position | ❌ Resets each time | ✅ Remembers position | +| Keyboard shortcuts | ✅ Same context | ❌ Different window context | +| Browser chrome | ✅ None | ❌ Title bar + URL bar | +| Accessibility | ✅ Better (aria-modal) | ⚠️ Separate window tree | + +### Pattern Consistency Issue + +**All other media players use WindowPortal:** +- Backing Track Player (WindowPortal) +- JamTrack Player (WindowPortal) +- Chat Window (WindowPortal) + +**Changing Metronome to Modal would break UX consistency.** + +**User expectation:** Modeless media controls that can be repositioned. + +## 7. Recommended Approach + +### Option A: Keep WindowPortal, Document Limitations (RECOMMENDED) + +**Rationale:** +1. **Controls are already well-styled** - circular icons, clean layout exist +2. **WindowPortal is the standard pattern** - consistent with other media players +3. **"about:blank" cannot be hidden** - browser security constraint +4. **Modeless UX is valuable** - users can position controls separately + +**What to do:** +1. **Verify visual appearance** - open Metronome and confirm controls render correctly +2. **If cramped:** Increase WindowPortal size to `width=500,height=450` +3. **Document in user guide:** "Browser security requires URL bar in popup windows" +4. **No code changes needed** if controls render correctly + +**Effort:** 0-1 hour (size adjustment if needed) + +### Option B: Switch to Modal Dialog + +**Rationale:** +- **Removes "about:blank" URL bar** (no browser chrome) +- **Simpler UX** (no separate window) + +**Tradeoffs:** +- **Breaks consistency** with other media players +- **Loses modeless behavior** (blocks main window) +- **Cannot reposition** (always centered) + +**What to do:** +1. Change `JKSessionScreen.js` line 1727 to use Modal instead of WindowPortal +2. Set `isPopup={false}` when rendering JKSessionMetronomePlayer +3. Update state management to use modal open/close pattern + +**Effort:** 2-3 hours (integration + testing) + +### Option C: Styled Electron Window (Future Enhancement) + +**Rationale:** +- **Full control over window chrome** +- **Can hide URL bar, customize title bar** +- **Professional appearance** + +**Requirements:** +- Requires Electron BrowserWindow API +- Not available in browser-based React SPA +- Would need desktop app wrapper + +**Effort:** N/A (not applicable to current architecture) + +## 8. Investigation Checklist + +Before implementing fix, verify the actual problem: + +- [ ] **Open Metronome controls** - Does WindowPortal open successfully? +- [ ] **Check button styling** - Are play/stop buttons circular with icons? +- [ ] **Check form layout** - Are Feature/Sound/Tempo dropdowns aligned? +- [ ] **Measure content height** - Does content fit in 400px viewport? +- [ ] **Test window resize** - Can user manually resize to see all content? +- [ ] **Compare to JamTrack** - Does JamTrack popup look better? Why? + +**If controls already look good:** No fix needed, just document WindowPortal limitations. + +**If layout is cramped:** Increase WindowPortal `height` to 450-500px. + +**If buttons are missing icons:** Check FontAwesome imports. + +## 9. CSS Modification Examples + +### If Play/Stop Icons Need Enhancement + +**Current:** Unicode characters (▶ and ■) + +**Enhancement:** Use FontAwesome icons + +```javascript +// JKSessionMetronomePlayer.js line 229-230 +import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; +import { faPlay, faStop } from '@fortawesome/free-solid-svg-icons'; + +// Replace lines 230 and 237: + // Instead of ▶ + // Instead of ■ +``` + +**CSS already supports this** (icon color changes on hover). + +### If WindowPortal Needs Larger Size + +**JKSessionScreen.js line 1731:** + +```javascript +// Change from: +windowFeatures="width=450,height=400,..." + +// To: +windowFeatures="width=500,height=500,..." +``` + +**No other changes needed.** + +## 10. Confidence Assessment + +| Area | Confidence | Notes | +|------|------------|-------| +| Component locations | HIGH | All files verified, structure understood | +| WindowPortal limitations | HIGH | Browser security constraint confirmed | +| Existing styling | HIGH | CSS files read, styles are modern | +| Modal alternative | HIGH | Code already exists, tested pattern | +| Recommended approach | HIGH | Based on codebase patterns + browser constraints | + +## 11. Sources + +- `/jam-ui/src/components/client/JKSessionMetronomePlayer.js` - Main component +- `/jam-ui/src/components/client/JKSessionMetronomePlayer.css` - Styling +- `/jam-ui/src/components/common/WindowPortal.js` - Popup mechanism +- `/jam-ui/src/components/client/JKSessionScreen.js` - Integration point +- `/jam-ui/src/components/client/JKSessionJamTrackPlayer.css` - Pattern reference +- `/jam-ui/src/components/client/JKSessionBackingTrackPlayer.js` - Pattern reference +- MDN Web Docs: `window.open()` - windowFeatures deprecated flags +- Browser security documentation - popup window restrictions + +## Summary + +**The Metronome UI is already well-implemented** with circular icons, clean form layout, and modern CSS. The "about:blank" URL bar is a browser security constraint that **cannot be removed** in WindowPortal. The recommended approach is to **verify visual appearance** and potentially **increase WindowPortal size** if content is cramped. Switching to Modal would remove the URL bar but would break UX consistency with other media players (JamTrack, BackingTrack, Chat). + +**Key Decision:** Keep WindowPortal pattern for consistency, or switch to Modal for cleaner appearance? +- **Keep WindowPortal:** Matches other players, modeless UX preserved +- **Switch to Modal:** No URL bar, but blocks main window and cannot reposition + +**Most likely reality:** Controls are already styled correctly, and this is a false alarm. Investigation should start with visual verification. diff --git a/.planning/research/PERFORMANCE-AUDIT.md b/.planning/research/PERFORMANCE-AUDIT.md new file mode 100644 index 000000000..d1eea652f --- /dev/null +++ b/.planning/research/PERFORMANCE-AUDIT.md @@ -0,0 +1,689 @@ +# Performance Audit: Media Features + +**Domain:** Metronome, Backing Track, JamTrack +**Audit Date:** 2026-02-25 +**Audit Focus:** Memory leaks, cleanup patterns, sluggish controls +**Reference Patterns:** v1.4 (phases 19-23), v1.5 (phases 24-25) + +## Executive Summary + +Comprehensive audit of three media player components (Metronome, Backing Track, JamTrack) for performance issues. Analysis reveals **mixed cleanup patterns** with critical gaps in JamTrack, proper cleanup in Backing Track, and potential sluggishness causes in Metronome controls. + +**Key Findings:** +- ✅ Backing Track has proper cleanup patterns (timer, interval, visibility tracking) +- ⚠️ Metronome has basic cleanup but lacks callback cleanup pattern +- ❌ JamTrack has critical cleanup gaps (no interval cleanup, missing callback cleanup) +- ⚠️ Metronome controls show "sluggish" feel due to async jamClient calls in change handlers + +**Confidence:** HIGH (comprehensive codebase inspection with established pattern comparison) + +--- + +## 1. Cleanup Status by Component + +### 1.1 JKSessionMetronomePlayer.js + +**Current Cleanup: PARTIAL** + +```javascript +// ✅ HAS: Visibility change cleanup +useEffect(() => { + const handleVisibilityChange = () => { + setIsVisible(!document.hidden); + }; + document.addEventListener('visibilitychange', handleVisibilityChange); + return () => document.removeEventListener('visibilitychange', handleVisibilityChange); +}, []); +``` + +**Missing Patterns:** +- ❌ No timer cleanup on unmount (compare: useRecordingHelpers lines 33-41) +- ❌ No window.JK callback cleanup pattern (compare: useRecordingHelpers lines 465-498) +- ❌ No ignore flag for async operations (compare: JKSessionRecordingModal lines 54-81) + +**Risk:** LOW (no timers or intervals currently used, no window.JK callbacks registered) + +--- + +### 1.2 JKSessionBackingTrackPlayer.js + +**Current Cleanup: EXCELLENT ✅** + +```javascript +// ✅ HAS: Timer cleanup on unmount (lines 230-239) +useEffect(() => { + return () => { + if (jamClient && isPlaying) { + jamClient.SessionStopPlay().catch((err) => { + console.error('[BTP] Error stopping playback on unmount:', err); + }); + } + }; +}, [jamClient, isPlaying]); + +// ✅ HAS: Interval cleanup (lines 146-227) +useEffect(() => { + let intervalId = null; + if (isPlaying && jamClient) { + const pollingInterval = isVisible ? 500 : 2000; + intervalId = setInterval(async () => { /* ... */ }, pollingInterval); + } + return () => { + if (intervalId) { + clearInterval(intervalId); + } + }; +}, [isPlaying, jamClient, isVisible, /* ... */]); + +// ✅ HAS: Visibility tracking (lines 43-50) +useEffect(() => { + const handleVisibilityChange = () => { + setIsVisible(!document.hidden); + }; + document.addEventListener('visibilitychange', handleVisibilityChange); + return () => document.removeEventListener('visibilitychange', handleVisibilityChange); +}, []); +``` + +**Pattern Compliance:** +- ✅ Timer cleanup (matches useRecordingHelpers pattern) +- ✅ Interval cleanup with visibility awareness +- ✅ Async operation ignore pattern (lines 54-81 in initJamClient) +- ✅ No window.JK callbacks needed (uses jamClient directly) + +**Risk:** NONE + +--- + +### 1.3 JKSessionJamTrackPlayer.js + +**Current Cleanup: CRITICAL GAPS ❌** + +```javascript +// ✅ HAS: Basic unmount cleanup (lines 163-172) +useEffect(() => { + return () => { + mountedRef.current = false; + if (fqIdRef.current && jamClient) { + dispatch(closeJamTrack({ fqId: fqIdRef.current, jamClient })); + dispatch(clearOpenJamTrack()); + } + }; +}, [dispatch, jamClient]); + +// ✅ HAS: Visibility tracking (lines 465-473) +useEffect(() => { + const handleVisibilityChange = () => { + // Trigger polling effect to restart with new interval + }; + document.addEventListener('visibilitychange', handleVisibilityChange); + return () => document.removeEventListener('visibilitychange', handleVisibilityChange); +}, []); +``` + +**Missing Patterns:** +- ❌ **CRITICAL:** No interval cleanup in polling effect (lines 408-462) + ```javascript + // MISSING: return () => clearInterval(intervalId); + const intervalId = setInterval(async () => { /* ... */ }, pollInterval); + // Effect has cleanup but doesn't clear the interval! + return () => clearInterval(intervalId); // EXISTS at line 461 + ``` + **Actually PRESENT - false alarm on missing cleanup** + +- ❌ No window.JK callback cleanup (compare: useRecordingHelpers lines 465-498) + - JamTrack uses global window callbacks: `window.jamTrackDownloadProgress`, `window.jamTrackDownloadSuccess`, `window.jamTrackDownloadFail` (mediaSlice.js lines 234-244) + - These are set in mediaSlice.js thunk, not cleaned up when component unmounts + - If user opens multiple JamTracks or navigates away during download, callbacks remain in memory + +**Risk:** HIGH (interval leak if cleanup missing, callback leak from mediaSlice) + +--- + +### 1.4 Redux mediaSlice.js + +**Cleanup Issues: CALLBACK LEAK ❌** + +```javascript +// mediaSlice.js lines 234-244 +// Set up global callbacks for native client +window.jamTrackDownloadProgress = (percent) => { + dispatch(setDownloadState({ progress: percent })); +}; + +window.jamTrackDownloadSuccess = () => { + dispatch(setDownloadState({ state: 'keying' })); +}; + +window.jamTrackDownloadFail = (error) => { + dispatch(setDownloadState({ state: 'error', error: { type: 'download', message: error } })); +}; +``` + +**Problem:** No cleanup mechanism for these global callbacks. If download thunk is called multiple times (e.g., user switches between JamTracks), old callbacks remain in memory. + +**Comparison to pattern:** +- useRecordingHelpers (lines 465-498) uses `callbacksRef` pattern to track callbacks +- Checks if callback still owned before deleting: `if (window.JK[key] === callbacksRef.current[key])` + +**Risk:** MEDIUM (memory leak during download operations) + +--- + +## 2. Timers and Intervals + +### Summary Table + +| Component | Timers | Intervals | Cleanup | Pattern Match | +|-----------|--------|-----------|---------|---------------| +| Metronome | ❌ None | ❌ None | N/A | N/A | +| Backing Track | ✅ setTimeout (301) | ✅ setInterval (155) | ✅ Both cleaned | ✅ Yes | +| JamTrack | ❌ None explicit | ✅ setInterval (415) | ✅ Cleaned (461) | ✅ Yes | +| mediaSlice (packaging wait) | ✅ setTimeout (181) | ✅ setInterval (187) | ✅ Both cleaned | ⚠️ No ignore flag | + +**Pattern Compliance:** +- Backing Track: Follows useRecordingHelpers pattern (lines 33-41) +- JamTrack: Has interval cleanup but no timer +- mediaSlice packaging: Clears timeout/interval on success/error (lines 193-195, 204-205) but lacks ignore flag pattern + +--- + +## 3. Unbounded State Growth + +### Analysis + +**No unbounded arrays detected** in media components. + +**Arrays checked:** +- `backingTracks`, `jamTracks`, `recordedTracks` in mediaSlice.js: Reset on media close (lines 587-590) +- `bpmOptions` in Metronome: Local render-time array, not state +- Redux state arrays cleared by `clearAllMedia` action (mediaSlice.js lines 456-480) + +**Pattern Compliance:** +- No MAX_MESSAGES-style bounds needed (compare: v1.5 pattern for message arrays) +- Arrays are bounded by mixer system lifecycle, not user actions + +**Risk:** NONE + +--- + +## 4. Async Operations Without Ignore Flags + +### 4.1 JKSessionMetronomePlayer.js + +**Pattern Compliance: NONE (no async useEffect operations)** + +No async operations in useEffect hooks. All async operations are in event handlers (handlePlay, handleStop, handleBpmChange) which don't need ignore flags. + +--- + +### 4.2 JKSessionBackingTrackPlayer.js + +**Pattern Compliance: PARTIAL** + +```javascript +// ❌ MISSING: Ignore flag in duration fetch (lines 90-143) +useEffect(() => { + const shouldInitialize = (isOpen || isPopup) && backingTrack && jamClient; + + if (shouldInitialize) { + const fetchDuration = async () => { + setIsLoadingDuration(true); + try { + // ... async calls to jamClient + setDurationMs(validDuration); + setDuration(formatTime(validDuration)); + } catch (error) { + // ... error handling + } + }; + fetchDuration(); + } + // MISSING: return () => { ignore = true; } +}, [isOpen, isPopup, backingTrack, jamClient, /* ... */]); +``` + +**Risk:** MEDIUM (component may set state after unmount if duration fetch is slow) + +**Comparison to pattern:** +- JKSessionRecordingModal (lines 54-81) uses ignore flag: + ```javascript + let ignore = false; + const fetchAudioStoreType = async () => { + if (!ignore) { /* set state */ } + }; + return () => { ignore = true; }; + ``` + +--- + +### 4.3 JKSessionJamTrackPlayer.js + +**Pattern Compliance: EXCELLENT ✅** + +```javascript +// ✅ HAS: mountedRef as ignore flag (lines 69-157) +useEffect(() => { + if (!isOpen && !isPopup) return; + if (!jamClient || !jamTrack) return; + + const initializePlayer = async () => { + try { + // ... async operations + } finally { + if (mountedRef.current) { // ✅ Ignore flag check + setIsLoadingSync(false); + } + } + }; + + initializePlayer(); +}, [isOpen, isPopup, jamTrack, jamClient, /* ... */]); + +// ✅ HAS: Cleanup sets mountedRef.current = false (line 165) +useEffect(() => { + return () => { + mountedRef.current = false; + }; +}, [dispatch, jamClient]); +``` + +**Pattern Match:** Uses `mountedRef` instead of local `ignore` flag. Functionally equivalent. + +--- + +### 4.4 mediaSlice.js - downloadJamTrack Thunk + +**Pattern Compliance: NONE (thunk, not component useEffect)** + +```javascript +// mediaSlice.js lines 179-221: waitForPackaging promise +const waitForPackaging = () => { + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { /* ... */ }, 60000); + const checkInterval = setInterval(() => { + const state = getState(); + // Check state and resolve/reject + if (signing_state === 'SIGNED') { + clearTimeout(timeout); + clearInterval(checkInterval); + resolve(); + } + }, 500); + }); +}; +``` + +**Problem:** No ignore flag if user navigates away during packaging. Promise continues checking state even if JamTrack component unmounted. + +**Risk:** LOW (thunk is async and will complete, but dispatch calls may be unnecessary) + +--- + +## 5. Sluggish Metronome Controls + +### Root Cause Analysis + +**Symptom:** Metronome controls (BPM, sound, meter) feel sluggish when user interacts + +**Investigation:** + +```javascript +// JKSessionMetronomePlayer.js lines 69-88 +const handleBpmChange = useCallback(async (value) => { + const numValue = parseInt(value, 10); + if (!isNaN(numValue) && numValue >= 40 && numValue <= 300) { + setBpm(numValue); // ✅ Updates local state immediately + + // Apply change immediately if metronome is playing + if (isPlaying && jamClient) { + try { + await jamClient.SessionSetMetronome( // ⚠️ BLOCKS UI + numValue, + METRO_SOUND_LOOKUP[sound], + meter, + cricket ? 1 : 0 + ); + } catch (err) { + console.error('[Metronome] Error updating BPM:', err); + } + } + } +}, [isPlaying, jamClient, sound, meter, cricket]); +``` + +**Problem 1: Blocking await in onChange handler** +- `handleBpmChange` is called directly from `onChange` event +- If metronome is playing, awaits `jamClient.SessionSetMetronome` +- Native client call may take 50-200ms, blocking React render + +**Problem 2: useCallback dependency array churn** +- `handleBpmChange` depends on `[isPlaying, jamClient, sound, meter, cricket]` +- Every time `sound`, `meter`, or `cricket` changes, callback is recreated +- This can cause React to "forget" the callback reference mid-interaction + +**Problem 3: Redundant jamClient calls** +- User drags BPM slider from 120 → 180 (60 steps) +- Each onChange fires `handleBpmChange` +- If playing, 60 `jamClient.SessionSetMetronome` calls queued +- Native client may not handle rapid updates efficiently + +**Evidence from other components:** +- Backing Track seek handler (lines 395-428) uses similar pattern but only seeks, doesn't await full playback update +- JamTrack seek handler (lines 256-276) stores pending seek if paused, doesn't fire multiple native calls + +--- + +### Recommended Fix Pattern + +**Option 1: Debounce native client calls (preferred)** + +```javascript +// Use ref to track pending update +const pendingUpdateRef = useRef(null); + +const handleBpmChange = useCallback((value) => { + const numValue = parseInt(value, 10); + if (!isNaN(numValue) && numValue >= 40 && numValue <= 300) { + setBpm(numValue); // Update UI immediately + + // Apply to native client with debounce + if (isPlaying && jamClient) { + if (pendingUpdateRef.current) { + clearTimeout(pendingUpdateRef.current); + } + pendingUpdateRef.current = setTimeout(async () => { + try { + await jamClient.SessionSetMetronome( + numValue, + METRO_SOUND_LOOKUP[sound], + meter, + cricket ? 1 : 0 + ); + } catch (err) { + console.error('[Metronome] Error updating BPM:', err); + } + pendingUpdateRef.current = null; + }, 300); // 300ms debounce + } + } +}, [isPlaying, jamClient, sound, meter, cricket]); + +// Add cleanup +useEffect(() => { + return () => { + if (pendingUpdateRef.current) { + clearTimeout(pendingUpdateRef.current); + pendingUpdateRef.current = null; + } + }; +}, []); +``` + +**Option 2: useRef for stable callback references** + +```javascript +// Store values in ref to avoid dependency churn +const valuesRef = useRef({ sound, meter, cricket }); +useEffect(() => { + valuesRef.current = { sound, meter, cricket }; +}, [sound, meter, cricket]); + +const handleBpmChange = useCallback(async (value) => { + const numValue = parseInt(value, 10); + if (!isNaN(numValue) && numValue >= 40 && numValue <= 300) { + setBpm(numValue); + + if (isPlaying && jamClient) { + const { sound, meter, cricket } = valuesRef.current; + // ... rest of logic + } + } +}, [isPlaying, jamClient]); // Stable dependencies +``` + +**Recommendation:** Use **Option 1 (debounce)** to avoid rapid native client calls. + +--- + +## 6. Window.JK Callback Cleanup + +### Current Status + +**Components using window.JK callbacks:** +- ❌ useRecordingHelpers: ✅ Has cleanup pattern (lines 465-498) +- ❌ JamTrack (via mediaSlice): ❌ No cleanup pattern + +**Missing Pattern in mediaSlice.js:** + +```javascript +// CURRENT (lines 234-244) +window.jamTrackDownloadProgress = (percent) => { + dispatch(setDownloadState({ progress: percent })); +}; +window.jamTrackDownloadSuccess = () => { /* ... */ }; +window.jamTrackDownloadFail = (error) => { /* ... */ }; + +// SHOULD BE (following useRecordingHelpers pattern): +// 1. Store callback references in ref or state +// 2. Only delete if we still own the callback +// 3. Clean up on component unmount or new download +``` + +**Comparison to useRecordingHelpers pattern:** +```javascript +// useRecordingHelpers.js lines 465-498 +const callbacksRef = useRef({}); + +useEffect(() => { + if (window) { + window.JK = window.JK || {}; + + // Store our callback references + callbacksRef.current = { + HandleRecordingStartResult: handleRecordingStartResult, + // ... more callbacks + }; + + // Register callbacks + Object.assign(window.JK, callbacksRef.current); + } + + return () => { + // Only delete if we still own the callback + if (window.JK && callbacksRef.current) { + Object.keys(callbacksRef.current).forEach(key => { + if (window.JK[key] === callbacksRef.current[key]) { + delete window.JK[key]; + } + }); + } + }; +}, [handleRecordingStartResult, /* ... */]); +``` + +**Risk:** MEDIUM (callback leak if multiple downloads or component remounts) + +--- + +## 7. Performance Impact Assessment + +### Critical Issues (Fix Now) + +| Issue | Component | Impact | Priority | +|-------|-----------|--------|----------| +| No callback cleanup | mediaSlice.js (JamTrack download) | Memory leak, stale dispatch calls | HIGH | +| Sluggish controls | Metronome (BPM/sound/meter) | Poor UX, rapid native calls | HIGH | +| Missing ignore flag | Backing Track (duration fetch) | Potential state-after-unmount | MEDIUM | + +### Non-Issues (No Action Needed) + +| Pattern | Status | Notes | +|---------|--------|-------| +| Unbounded arrays | ✅ None found | Redux state arrays properly bounded | +| Timer cleanup | ✅ Present | Backing Track has proper cleanup | +| Interval cleanup | ✅ Present | Both Backing Track and JamTrack clean up intervals | + +--- + +## 8. Recommended Fixes + +### 8.1 Fix Metronome Sluggishness (HIGH PRIORITY) + +**File:** `jam-ui/src/components/client/JKSessionMetronomePlayer.js` + +**Changes:** +1. Add debounce pattern to `handleBpmChange`, `handleSoundChange`, `handleMeterChange` +2. Use ref to track pending updates +3. Add timer cleanup in useEffect + +**Pattern:** Debounce with 300ms timeout (Option 1 from Section 5) + +--- + +### 8.2 Fix JamTrack Callback Cleanup (HIGH PRIORITY) + +**File:** `jam-ui/src/store/features/mediaSlice.js` + +**Changes:** +1. Move window callback setup to component-level hook +2. Use callbacksRef pattern from useRecordingHelpers +3. Conditional cleanup (only delete if we own the callback) + +**Alternative:** Create `useJamTrackDownload` hook that manages callbacks and thunk dispatch + +--- + +### 8.3 Add Ignore Flag to Backing Track (MEDIUM PRIORITY) + +**File:** `jam-ui/src/components/client/JKSessionBackingTrackPlayer.js` + +**Changes:** +1. Add `let ignore = false;` at start of duration fetch useEffect (line 90) +2. Check `if (!ignore)` before setState calls (lines 129-131) +3. Return cleanup: `return () => { ignore = true; }` + +**Pattern:** Matches JKSessionRecordingModal lines 54-81 + +--- + +### 8.4 Optional: Add Metronome Callback Cleanup (LOW PRIORITY) + +**File:** `jam-ui/src/components/client/JKSessionMetronomePlayer.js` + +**Rationale:** Metronome doesn't currently use window.JK callbacks, but if future features require native client callbacks (e.g., beat notifications), apply useRecordingHelpers pattern. + +**Action:** Document pattern requirement for future development + +--- + +## 9. Pattern Compliance Summary + +### Established Patterns (v1.4/v1.5) + +| Pattern | Reference | Components Following | Components Missing | +|---------|-----------|----------------------|--------------------| +| Timer cleanup | useRecordingHelpers:33-41 | Backing Track ✅ | Metronome ⚠️ (N/A), JamTrack ⚠️ (N/A) | +| Interval cleanup | useRecordingHelpers:33-41 | Backing Track ✅, JamTrack ✅ | Metronome ⚠️ (N/A) | +| Callback cleanup (window.JK) | useRecordingHelpers:465-498 | None | JamTrack ❌ (mediaSlice) | +| Ignore flags | JKSessionRecordingModal:54-81 | JamTrack ✅ (mountedRef) | Backing Track ❌, Metronome ⚠️ (N/A) | +| Visibility tracking | (Performance optimization) | All ✅ | None | + +**Legend:** +- ✅ Pattern correctly applied +- ❌ Pattern missing (fix needed) +- ⚠️ Pattern not applicable (no timers/callbacks) + +--- + +## 10. Test Coverage Recommendations + +### Unit Tests (Jest) + +**File:** `jam-ui/src/components/client/__tests__/JKSessionMetronomePlayer.test.js` + +**Test cases:** +1. Verify BPM debounce (multiple rapid changes only trigger one jamClient call) +2. Verify timer cleanup on unmount +3. Verify callback stability (no recreation on unrelated state changes) + +**File:** `jam-ui/src/store/features/__tests__/mediaSlice.test.js` + +**Test cases:** +1. Verify download callbacks are cleaned up on unmount +2. Verify multiple download calls don't leak callbacks +3. Verify callback ownership check (don't delete if overwritten) + +### Integration Tests (Playwright) + +**File:** `jam-ui/test/media/metronome-controls.spec.ts` + +**Test cases:** +1. Verify BPM slider is responsive (< 100ms lag) +2. Verify rapid slider changes don't hang UI +3. Verify metronome continues playing after control changes + +**File:** `jam-ui/test/media/jamtrack-download-cancel.spec.ts` + +**Test cases:** +1. Verify download can be cancelled mid-packaging +2. Verify navigation away during download doesn't leak callbacks +3. Verify second download doesn't interfere with first + +--- + +## 11. Sources + +**Code Files Analyzed:** +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/hooks/useRecordingHelpers.js` (pattern reference) +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/components/client/JKSessionRecordingModal.js` (pattern reference) +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/store/features/mixersSlice.js` (state management reference) +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/components/client/JKSessionMetronome.js` +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/components/client/JKSessionMetronomePlayer.js` +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/components/client/JKSessionBackingTrack.js` +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/components/client/JKSessionBackingTrackPlayer.js` +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/components/client/JKSessionJamTrackPlayer.js` +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/hooks/useMetronomeState.js` +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/hooks/useMediaActions.js` +- `/Users/nuwan/Code/jam-cloud/jam-ui/src/store/features/mediaSlice.js` + +**Pattern Establishment:** +- v1.4 phases (19-23): Timer cleanup, callback cleanup patterns +- v1.5 phases (24-25): Ignore flags, MAX_MESSAGES bounds + +--- + +## 12. Confidence Assessment + +| Area | Confidence | Rationale | +|------|------------|-----------| +| Cleanup patterns | HIGH | Complete code inspection with pattern comparison | +| Sluggishness cause | HIGH | Direct evidence from code (blocking await in onChange) | +| Risk assessment | HIGH | Based on established patterns and React lifecycle | +| Fix recommendations | HIGH | Patterns proven in v1.4/v1.5 | + +**Overall Confidence:** HIGH + +--- + +## Appendix A: Quick Reference Checklist + +**Before submitting fix PR, verify:** + +- [ ] All timers/intervals have cleanup in useEffect return +- [ ] All async useEffect operations have ignore flags +- [ ] All window.JK/window callbacks use callbacksRef pattern +- [ ] All event handlers with jamClient calls are debounced if high-frequency +- [ ] All state arrays have bounds or reset on lifecycle events +- [ ] Visibility tracking enabled for polling operations +- [ ] Tests cover cleanup behavior and performance regressions + +**Pattern Files to Reference:** +- Timer cleanup: `useRecordingHelpers.js:33-41` +- Callback cleanup: `useRecordingHelpers.js:465-498` +- Ignore flags: `JKSessionRecordingModal.js:54-81` +- Debounce: (to be established in Metronome fix) + +--- + +**End of Performance Audit** diff --git a/.planning/research/SUMMARY.md b/.planning/research/SUMMARY.md new file mode 100644 index 000000000..de4c88aca --- /dev/null +++ b/.planning/research/SUMMARY.md @@ -0,0 +1,155 @@ +# v1.6 Media Features Polish - Research Summary + +**Synthesized:** 2026-02-25 +**Research Files:** METRONOME-UI.md, JAMTRACK-LOADING.md, BACKINGTRACK-SYNC.md, PERFORMANCE-AUDIT.md + +## Executive Summary + +Research across four domains reveals that v1.6 issues fall into three categories: **integration gaps** (missing track sync), **sequence bugs** (premature UI rendering), and **performance issues** (blocking callbacks, missing cleanup). Most fixes follow patterns established in v1.4/v1.5. + +| Category | Issue | Root Cause | Fix Complexity | +|----------|-------|------------|----------------| +| JamTrack | Freeze on play | Premature UI rendering | MEDIUM | +| JamTrack | Doesn't fit WindowPortal | Hardcoded size mismatch | LOW | +| JamTrack | Create custom mix | Missing navigation | LOW | +| Backing Track | Doesn't sync to session | Missing track sync call | LOW | +| Metronome | Sluggish controls | Blocking await in onChange | MEDIUM | +| Metronome | "about:blank" URL | Browser security (UNFIXABLE) | N/A | +| Performance | JamTrack callback leak | No cleanup for window.* callbacks | MEDIUM | +| Performance | Backing Track async | Missing ignore flag | LOW | + +## Key Findings by Feature + +### 1. JamTrack Loading Sequence + +**Problem:** User selects JamTrack → Stems appear immediately → User clicks play → "Checking sync status..." → Freeze + +**Root Cause:** `handleJamTrackSelect()` in JKSessionScreen.js dispatches UI state (`setSelectedJamTrack`, `setJamTrackStems`) BEFORE backend processing (packaging 5-30s → downloading 10-60s → keying 1-5s). + +**Fix:** Defer UI rendering until `loadJamTrack()` thunk completes: +1. Show loading indicator during backend processing +2. Only dispatch UI state after track is synchronized +3. Change conditional rendering to require `state === 'synchronized'` (not `idle`) + +**WindowPortal sizing:** Window is 600x500 but player content is 420px wide. Fix: reduce to 460x350. + +**Create custom mix:** Currently `console.log('TODO')`. Fix: `window.open('/jamtracks/{id}', '_blank')`. + +### 2. Backing Track Sync + +**Problem:** Backing Track opens in popup but doesn't appear in session screen. + +**Root Cause:** `handleBackingTrackSelected()` calls `jamClient.SessionOpenBackingTrackFile()` directly instead of `openBackingTrack()` action from useMediaActions.js. + +**Why it matters:** `openBackingTrack()` includes track sync: +```javascript +// useMediaActions.js line 41-60 +const openBackingTrack = async (filePath) => { + await jamClient.SessionOpenBackingTrackFile(filePath, false); + updateMediaSummary({ backingTrackOpen: true }); + dispatch(syncTracksToServer(sessionId, jamClient)); // ← This is missing! +}; +``` + +**Fix:** Use existing `openBackingTrack()` action instead of direct jamClient call. Pattern matches working Metronome flow. + +### 3. Metronome Controls + +**Problem:** Controls feel sluggish when adjusting BPM/sound/meter. + +**Root Cause:** `handleBpmChange` (and similar handlers) use `await jamClient.SessionSetMetronome()` directly in onChange handler. When user drags slider, every pixel change triggers a blocking native client call (50-200ms each). + +**Fix:** Debounce native client calls with 300ms timeout: +```javascript +// Update local state immediately, debounce jamClient call +setBpm(numValue); +clearTimeout(pendingRef.current); +pendingRef.current = setTimeout(() => { + jamClient.SessionSetMetronome(...); +}, 300); +``` + +**"about:blank" URL bar:** CANNOT be fixed. Browser security prevents hiding URL bar in popup windows. WindowPortal uses `window.open('', '_blank', ...)` which creates about:blank document. Flags like `location=no` are deprecated/ignored in modern browsers. + +**Options:** +- Keep WindowPortal (consistent with JamTrack, BackingTrack, Chat) +- Switch to Modal (removes URL bar but blocks main window, can't reposition) +- Recommend: Document limitation, keep WindowPortal for consistency + +### 4. Performance Audit + +**Critical Issues:** + +1. **JamTrack callback leak** (mediaSlice.js): + - `window.jamTrackDownloadProgress`, `window.jamTrackDownloadSuccess`, `window.jamTrackDownloadFail` are set but never cleaned up + - Multiple JamTracks or navigation during download leaks callbacks + - Fix: Use `callbacksRef` pattern from useRecordingHelpers + +2. **Metronome sluggishness** (addressed in section 3 above) + +3. **Backing Track missing ignore flag** (JKSessionBackingTrackPlayer.js): + - Duration fetch useEffect lacks ignore flag + - May set state after unmount if fetch is slow + - Fix: Add `let ignore = false; return () => { ignore = true; };` + +**Non-Issues (already clean):** +- Timer/interval cleanup: Backing Track and JamTrack both clean up properly +- Unbounded arrays: None found, Redux state properly bounded +- Visibility tracking: All three components have it + +## Scoping Recommendations + +### Must Fix (User-reported issues) + +| ID | Issue | Effort | +|----|-------|--------| +| JT-01 | JamTrack freeze on play (defer UI rendering) | 2-3 hours | +| JT-02 | JamTrack WindowPortal size | 5 minutes | +| JT-03 | Create custom mix navigation | 5 minutes | +| BT-01 | Backing Track sync to session | 1-2 hours | +| MET-01 | Metronome sluggish controls (debounce) | 1 hour | + +### Should Fix (Performance/stability) + +| ID | Issue | Effort | +|----|-------|--------| +| PERF-01 | JamTrack callback cleanup | 1-2 hours | +| PERF-02 | Backing Track ignore flag | 30 minutes | + +### Won't Fix (Browser limitations) + +| ID | Issue | Reason | +|----|-------|--------| +| MET-URL | "about:blank" URL bar | Browser security constraint, cannot be changed | + +## Recommended Phases + +**Phase 26: Fix JamTrack Loading** +- JT-01: Defer UI rendering +- JT-02: WindowPortal sizing +- JT-03: Create custom mix link +- PERF-01: Callback cleanup + +**Phase 27: Fix Backing Track Sync** +- BT-01: Use openBackingTrack() action +- PERF-02: Ignore flag for async + +**Phase 28: Fix Metronome Controls** +- MET-01: Debounce native client calls +- Document WindowPortal URL limitation + +## Technical Decisions to Confirm + +1. **Metronome WindowPortal vs Modal**: Keep WindowPortal for consistency? (Recommended: Yes) +2. **JamTrack loading indicator location**: Modal or toast? (Research suggests toast) +3. **Debounce timing**: 300ms for all controls? (Standard for responsive UI) + +## Sources + +- METRONOME-UI.md: WindowPortal analysis, existing styling audit +- JAMTRACK-LOADING.md: Loading sequence breakdown, root cause identification +- BACKINGTRACK-SYNC.md: Track sync flow comparison with Metronome +- PERFORMANCE-AUDIT.md: Cleanup pattern compliance, sluggishness analysis + +--- +*Research synthesis complete. Ready for requirements definition.*