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 <noreply@anthropic.com>
This commit is contained in:
Nuwan
parent
daaa37a84e
commit
eab0b0d19a
|
|
@ -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*
|
||||
|
|
@ -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*
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
@ -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
|
||||
<a onClick={() => console.log('TODO: Open custom mix creator')}>
|
||||
create custom mix
|
||||
</a>
|
||||
```
|
||||
|
||||
### Required Behavior
|
||||
|
||||
Should open `/jamtracks/{id}` in a new browser tab.
|
||||
|
||||
**Implementation:**
|
||||
|
||||
```javascript
|
||||
<a
|
||||
onClick={() => {
|
||||
const url = `/jamtracks/${jamTrack.id}`;
|
||||
window.open(url, '_blank', 'noopener,noreferrer');
|
||||
}}
|
||||
style={{
|
||||
fontSize: '12px',
|
||||
color: '#007aff',
|
||||
textDecoration: 'none',
|
||||
cursor: 'pointer'
|
||||
}}
|
||||
>
|
||||
create custom mix
|
||||
</a>
|
||||
```
|
||||
|
||||
**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
|
||||
|
|
@ -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 && (
|
||||
<WindowPortal
|
||||
title="Metronome Controls"
|
||||
onClose={handleMetronomeClose}
|
||||
windowFeatures="width=450,height=400,left=200,top=200,menubar=no,toolbar=no,status=no,scrollbars=yes,resizable=yes,location=no, addressbar=no"
|
||||
windowId="metronome-controls"
|
||||
>
|
||||
<JKSessionMetronomePlayer
|
||||
isOpen={metronomeState.isOpen}
|
||||
onClose={handleMetronomeClose}
|
||||
metronomeState={metronomeState}
|
||||
jamClient={jamClient}
|
||||
session={currentSession}
|
||||
currentUser={currentUser}
|
||||
isPopup={true}
|
||||
/>
|
||||
</WindowPortal>
|
||||
)}
|
||||
```
|
||||
|
||||
## 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 (
|
||||
<Modal isOpen={isOpen} toggle={handleClose} className="metronome-player-modal">
|
||||
<ModalHeader toggle={handleClose}>
|
||||
Metronome Controls
|
||||
</ModalHeader>
|
||||
<ModalBody>
|
||||
{renderControls()}
|
||||
</ModalBody>
|
||||
</Modal>
|
||||
);
|
||||
```
|
||||
|
||||
**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:
|
||||
<FontAwesomeIcon icon={faPlay} /> // Instead of <span>▶</span>
|
||||
<FontAwesomeIcon icon={faStop} /> // Instead of <span>■</span>
|
||||
```
|
||||
|
||||
**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.
|
||||
|
|
@ -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**
|
||||
|
|
@ -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.*
|
||||
Loading…
Reference in New Issue