419 lines
17 KiB
Markdown
419 lines
17 KiB
Markdown
---
|
|
phase: 14-performance-optimization
|
|
plan: 03
|
|
type: execute
|
|
wave: 2
|
|
depends_on: ["14-01"]
|
|
files_modified:
|
|
- frontend/src/components/track-list/track-list.ts
|
|
- frontend/src/components/queue-panel/queue-panel.ts
|
|
- frontend/src/components/cover-grid/cover-grid.ts
|
|
- frontend/src/store/queue-store.ts
|
|
- frontend/src/store/library-store.ts
|
|
- frontend/src/store/controllers/library-controller.ts
|
|
autonomous: true
|
|
requirements: [PERF-RENDER-01, PERF-RENDER-02]
|
|
|
|
must_haves:
|
|
truths:
|
|
- "Scrolling does not create new arrow function closures per rendered item"
|
|
- "Queue store notifications are batched via queueMicrotask (matching library store pattern)"
|
|
- "Library store subscribers can subscribe to specific data types (tracks, albums, etc.) and only update when their data changes"
|
|
artifacts:
|
|
- path: "frontend/src/components/track-list/track-list.ts"
|
|
provides: "Bound method references instead of inline arrow closures in renderTrackRow"
|
|
contains: "this.onTrackRowClick"
|
|
- path: "frontend/src/components/queue-panel/queue-panel.ts"
|
|
provides: "Bound method references instead of inline closures in renderTrackItem"
|
|
contains: "this.onQueueTrackClick"
|
|
- path: "frontend/src/store/queue-store.ts"
|
|
provides: "queueMicrotask-based notification batching"
|
|
contains: "queueMicrotask"
|
|
- path: "frontend/src/store/library-store.ts"
|
|
provides: "Granular subscription by data type"
|
|
contains: "subscribeToTracks"
|
|
key_links:
|
|
- from: "frontend/src/store/library-store.ts"
|
|
to: "frontend/src/store/controllers/library-controller.ts"
|
|
via: "Granular subscription replacing blanket subscribe"
|
|
pattern: "subscribeTo"
|
|
- from: "frontend/src/store/queue-store.ts"
|
|
to: "notify"
|
|
via: "queueMicrotask batching"
|
|
pattern: "queueMicrotask"
|
|
---
|
|
|
|
<objective>
|
|
Eliminate per-item closure allocation during scroll rendering and reduce unnecessary component re-renders by adding notification batching and granular store subscriptions.
|
|
|
|
Purpose: Every scroll frame, `renderTrackRow` and `renderTrackItem` create new arrow function closures for click, dblclick, contextmenu, and dragstart handlers. This causes GC pressure during rapid scrolling. Additionally, the queue store notifies synchronously (not batched), and the library store sends blanket notifications for any data change even if the subscribing component only cares about one data type.
|
|
|
|
Output: Scroll rendering is GC-friendly with stable function references; store notifications are batched and granular.
|
|
</objective>
|
|
|
|
<execution_context>
|
|
@/home/caleb/.config/opencode/get-shit-done/workflows/execute-plan.md
|
|
@/home/caleb/.config/opencode/get-shit-done/templates/summary.md
|
|
</execution_context>
|
|
|
|
<context>
|
|
@.planning/PROJECT.md
|
|
@.planning/ROADMAP.md
|
|
@.planning/STATE.md
|
|
@frontend/src/components/track-list/track-list.ts
|
|
@frontend/src/components/queue-panel/queue-panel.ts
|
|
@frontend/src/store/queue-store.ts
|
|
@frontend/src/store/library-store.ts
|
|
@frontend/src/store/controllers/library-controller.ts
|
|
</context>
|
|
|
|
<interfaces>
|
|
<!-- Current renderTrackRow pattern (track-list.ts ~line 1530) -->
|
|
```typescript
|
|
private renderTrackRow = (track: library.Track, index: number): unknown => {
|
|
// Creates new closures every render:
|
|
// @click=${(e: MouseEvent) => this.onTrackRowClick(e, track, index)}
|
|
// @dblclick=${() => this.onTrackRowDblClick(track)}
|
|
// @contextmenu=${(e: MouseEvent) => this.onTrackContextMenu(e, track)}
|
|
// @dragstart=${(e: DragEvent) => this.onTrackDragStart(e, track)}
|
|
};
|
|
```
|
|
|
|
<!-- Current queue store notification (no batching) -->
|
|
```typescript
|
|
private notify(): void {
|
|
for (const sub of this.subscribers) {
|
|
sub();
|
|
}
|
|
}
|
|
```
|
|
|
|
<!-- Current library store subscription (blanket) -->
|
|
```typescript
|
|
// LibraryController subscribes to ALL store changes
|
|
hostConnected(): void {
|
|
this.unsubscribe = libraryStore.subscribe(() => {
|
|
this.host.requestUpdate();
|
|
});
|
|
}
|
|
```
|
|
|
|
<!-- Library store already has queueMicrotask batching -->
|
|
```typescript
|
|
private notify(): void {
|
|
if (this.notifyScheduled) return;
|
|
this.notifyScheduled = true;
|
|
queueMicrotask(() => {
|
|
this.notifyScheduled = false;
|
|
for (const sub of this.subscribers) {
|
|
sub();
|
|
}
|
|
});
|
|
}
|
|
```
|
|
</interfaces>
|
|
|
|
<tasks>
|
|
|
|
<task type="auto">
|
|
<name>Task 1: Eliminate per-item closure allocation in render hot paths</name>
|
|
<files>
|
|
frontend/src/components/track-list/track-list.ts
|
|
frontend/src/components/queue-panel/queue-panel.ts
|
|
frontend/src/components/cover-grid/cover-grid.ts
|
|
</files>
|
|
<action>
|
|
Replace inline arrow function closures in renderItem callbacks with event delegation or stable bound references. The key insight: lit-virtualizer's renderItem is called for each visible item on every scroll frame — closures created here are immediately GC pressure.
|
|
|
|
**track-list.ts — renderTrackRow:**
|
|
|
|
The current pattern creates 5 new closures per row per render: `@click`, `@dblclick`, `@contextmenu`, `@dragstart`, and the fav-icon `@click`.
|
|
|
|
Convert to **data-attribute event delegation**:
|
|
|
|
1. Add `data-index="${index}"` to the `.track-row` div.
|
|
|
|
2. Instead of per-row `@click`, `@dblclick`, `@contextmenu`, `@dragstart` closures, use a single delegated event handler pattern. Add stable (bound once) event handlers on the `lit-virtualizer` element itself in `firstUpdated()`:
|
|
|
|
```typescript
|
|
override firstUpdated() {
|
|
// ... existing firstUpdated code ...
|
|
|
|
const virt = this.virtualizer;
|
|
if (virt) {
|
|
virt.addEventListener('click', this.onDelegatedClick);
|
|
virt.addEventListener('dblclick', this.onDelegatedDblClick);
|
|
virt.addEventListener('contextmenu', this.onDelegatedContextMenu);
|
|
// dragstart delegated on track-row (draggable="true" is on each row)
|
|
}
|
|
}
|
|
```
|
|
|
|
3. Create stable bound methods that extract the index from `data-index`:
|
|
|
|
```typescript
|
|
private onDelegatedClick = (e: MouseEvent) => {
|
|
const row = (e.target as HTMLElement).closest('.track-row') as HTMLElement | null;
|
|
if (!row) return;
|
|
const idx = Number(row.dataset.index);
|
|
const track = this.cachedSortedTracks[idx];
|
|
if (!track) return;
|
|
|
|
// Check if click was on fav-icon
|
|
const favEl = (e.target as HTMLElement).closest('.fav-icon');
|
|
if (favEl) {
|
|
e.stopPropagation();
|
|
void this.favCtrl.toggleFavorite(track.FilePath);
|
|
return;
|
|
}
|
|
|
|
this.onTrackRowClick(e, track, idx);
|
|
};
|
|
|
|
private onDelegatedDblClick = (e: MouseEvent) => {
|
|
const row = (e.target as HTMLElement).closest('.track-row') as HTMLElement | null;
|
|
if (!row) return;
|
|
const idx = Number(row.dataset.index);
|
|
const track = this.cachedSortedTracks[idx];
|
|
if (track) this.onTrackRowDblClick(track);
|
|
};
|
|
|
|
private onDelegatedContextMenu = (e: MouseEvent) => {
|
|
const row = (e.target as HTMLElement).closest('.track-row') as HTMLElement | null;
|
|
if (!row) return;
|
|
const idx = Number(row.dataset.index);
|
|
const track = this.cachedSortedTracks[idx];
|
|
if (track) this.onTrackContextMenu(e, track);
|
|
};
|
|
```
|
|
|
|
4. For `@dragstart`: Keep it inline on the row element but use the data-index delegation pattern. Since `draggable="true"` must be on the individual row, the dragstart event naturally targets the row. Add a single delegated handler:
|
|
|
|
```typescript
|
|
private onDelegatedDragStart = (e: DragEvent) => {
|
|
const row = (e.target as HTMLElement).closest('.track-row') as HTMLElement | null;
|
|
if (!row) return;
|
|
const idx = Number(row.dataset.index);
|
|
const track = this.cachedSortedTracks[idx];
|
|
if (track) this.onTrackDragStart(e, track);
|
|
};
|
|
```
|
|
|
|
5. Update `renderTrackRow` to remove ALL inline closures:
|
|
```typescript
|
|
private renderTrackRow = (track: library.Track, index: number): unknown => {
|
|
// ... classMap, isFav, etc. remain the same ...
|
|
return html`
|
|
<div
|
|
class=${classMap({ 'track-row': true, active, selected })}
|
|
draggable="true"
|
|
data-index=${index}
|
|
>
|
|
<!-- fav icon — click handled by delegation -->
|
|
<div class=${classMap({ 'fav-icon': true, favorited: isFav })}>
|
|
<wa-icon name=${this.favCtrl.iconName} variant=${favVariant}></wa-icon>
|
|
</div>
|
|
${/* column rendering stays the same */}
|
|
</div>
|
|
`;
|
|
};
|
|
```
|
|
|
|
6. Add `@dragend` as a single stable handler on the virtualizer too (it's already `this.onTrackDragEnd` which is stable).
|
|
|
|
**CRITICAL:** Event delegation must work through shadow DOM. Since the virtualizer and its children are all within the same shadow root, `event.target.closest('.track-row')` works correctly. But verify with `composedPath()` if needed.
|
|
|
|
**queue-panel.ts — renderTrackItem:**
|
|
|
|
Apply the same event delegation pattern:
|
|
1. Add `data-index="${track.position}"` (or the loop index) to each `.track-item`
|
|
2. Register delegated handlers on the virtualizer in firstUpdated
|
|
3. Extract item index via `closest('.track-item')?.dataset.index`
|
|
4. Remove all inline closures from the template
|
|
|
|
**cover-grid.ts — renderAlbumCard/renderGridEntry:**
|
|
|
|
The cover grid already uses some delegation (it reads `data-index` for some operations). Verify that all click handlers on album cards use delegation. If any inline closures remain in `renderGridEntry` or `renderAlbumCard`, convert them.
|
|
|
|
**Key rule:** After this change, `renderTrackRow` and `renderTrackItem` should create ZERO new function objects. Every handler reference should be stable (either a bound class method or a property arrow function defined once in the class body).
|
|
</action>
|
|
<verify>
|
|
`cd frontend && npx vite build --mode development 2>&1 | tail -5` builds without errors.
|
|
Functional test: (1) Click a track → plays correctly, (2) Double-click → plays, (3) Right-click → context menu appears with correct track, (4) Drag a track → drag image shows, drop works, (5) Click favorite icon → toggles correctly, (6) Multi-select with Shift/Ctrl → works, (7) Queue panel: click, dblclick, drag, context menu all work.
|
|
</verify>
|
|
<done>renderTrackRow and renderTrackItem create zero inline closures. All event handling uses delegation via data-index attributes and stable bound handlers.</done>
|
|
</task>
|
|
|
|
<task type="auto">
|
|
<name>Task 2: Add notification batching to queue store and granular subscriptions to library store</name>
|
|
<files>
|
|
frontend/src/store/queue-store.ts
|
|
frontend/src/store/library-store.ts
|
|
frontend/src/store/controllers/library-controller.ts
|
|
</files>
|
|
<action>
|
|
**queue-store.ts — Add queueMicrotask batching:**
|
|
|
|
The library store already uses `queueMicrotask` batching (added in Phase 8). The queue store does NOT — it calls all subscribers synchronously on every state change. This means rapid queue mutations (e.g., adding multiple tracks) trigger multiple synchronous re-renders.
|
|
|
|
Add the same batching pattern from library-store:
|
|
|
|
```typescript
|
|
private notifyScheduled = false;
|
|
|
|
private notify(): void {
|
|
if (this.notifyScheduled) return;
|
|
this.notifyScheduled = true;
|
|
queueMicrotask(() => {
|
|
this.notifyScheduled = false;
|
|
for (const sub of this.subscribers) {
|
|
sub();
|
|
}
|
|
});
|
|
}
|
|
```
|
|
|
|
This coalesces multiple synchronous `notify()` calls into a single subscriber notification per microtask tick. Safe because Lit's `requestUpdate()` already deduplicates internally, but this prevents the overhead of even invoking all subscriber callbacks multiple times.
|
|
|
|
**library-store.ts — Add granular data-type subscriptions:**
|
|
|
|
Currently `subscribe()` registers a callback that fires on ANY store change (tracks, albums, artists, genres, cover size, loading state). This means:
|
|
- Track list component gets notified when albums change (unnecessary requestUpdate)
|
|
- Album grid gets notified when genres change (unnecessary requestUpdate)
|
|
- All components get notified when any loading flag changes
|
|
|
|
Add type-specific subscriptions alongside the existing blanket `subscribe()`:
|
|
|
|
```typescript
|
|
type DataType = 'tracks' | 'albums' | 'artists' | 'genres' | 'coverSize';
|
|
|
|
private typedSubscribers = new Map<DataType, Set<Subscriber>>();
|
|
|
|
subscribeTo(type: DataType, callback: Subscriber): () => void {
|
|
if (!this.typedSubscribers.has(type)) {
|
|
this.typedSubscribers.set(type, new Set());
|
|
}
|
|
const subs = this.typedSubscribers.get(type)!;
|
|
subs.add(callback);
|
|
return () => subs.delete(callback);
|
|
}
|
|
|
|
private notifyType(type: DataType): void {
|
|
const subs = this.typedSubscribers.get(type);
|
|
if (subs) {
|
|
for (const sub of subs) {
|
|
sub();
|
|
}
|
|
}
|
|
}
|
|
```
|
|
|
|
Then update the data access methods to use `notifyType`:
|
|
- `getTracks()` finally block: call `notifyType('tracks')` instead of `notify()`
|
|
- `getAlbums()` finally block: call `notifyType('albums')` instead of `notify()`
|
|
- `getArtists()` finally block: call `notifyType('artists')` instead of `notify()`
|
|
- `getGenres()` finally block: call `notifyType('genres')` instead of `notify()`
|
|
- `setCoverSize()`: call `notifyType('coverSize')` instead of `notify()`
|
|
- `invalidate()`: Keep calling `notify()` (blanket) since invalidation affects everything
|
|
|
|
Wait — this is tricky. The `notify()` method uses `queueMicrotask` batching. If we have both typed and blanket notifications in the same microtask, we need to be careful.
|
|
|
|
**Simpler approach:** Instead of typed subscriptions on the store, make `LibraryController` smarter. The controller already has access to `cachedTracks`, `cachedAlbums`, etc. On each store notification, the controller can CHECK if the data it cares about actually changed before calling `requestUpdate()`:
|
|
|
|
```typescript
|
|
// In LibraryController
|
|
hostConnected(): void {
|
|
// Track the references we last saw
|
|
let lastTracks = libraryStore.getCachedTracks();
|
|
let lastAlbums = libraryStore.getCachedAlbums();
|
|
|
|
this.unsubscribe = libraryStore.subscribe(() => {
|
|
const newTracks = libraryStore.getCachedTracks();
|
|
const newAlbums = libraryStore.getCachedAlbums();
|
|
const newArtists = libraryStore.getCachedArtists();
|
|
const newGenres = libraryStore.getCachedGenres();
|
|
|
|
// Only request update if data this host cares about changed
|
|
// Since we don't know what the host uses, check all and requestUpdate
|
|
// if ANY changed. But crucially, skip if loading state just toggled.
|
|
if (newTracks !== lastTracks ||
|
|
newAlbums !== lastAlbums ||
|
|
newArtists !== this.lastArtists ||
|
|
newGenres !== this.lastGenres) {
|
|
lastTracks = newTracks;
|
|
lastAlbums = newAlbums;
|
|
// ... etc
|
|
this.host.requestUpdate();
|
|
}
|
|
});
|
|
}
|
|
```
|
|
|
|
Actually, this is still checking everything. The real win is: **don't call requestUpdate when only loading state changed**. The loading state toggling on/off during eagerFetch causes 8+ unnecessary requestUpdate calls across all components.
|
|
|
|
**Refined approach for library-store.ts:**
|
|
Add a `changeGeneration` counter. Increment it only when actual data changes (not loading flags):
|
|
|
|
```typescript
|
|
private changeGen = 0;
|
|
|
|
// In getTracks, getAlbums, etc. — after setting this.tracks = tracks:
|
|
this.changeGen++;
|
|
|
|
// In invalidate — after clearing caches:
|
|
this.changeGen++;
|
|
```
|
|
|
|
Then in `notify()`, also expose the generation. In the controller:
|
|
|
|
```typescript
|
|
hostConnected(): void {
|
|
let lastGen = libraryStore.changeGeneration;
|
|
this.unsubscribe = libraryStore.subscribe(() => {
|
|
const gen = libraryStore.changeGeneration;
|
|
if (gen !== lastGen) {
|
|
lastGen = gen;
|
|
this.host.requestUpdate();
|
|
}
|
|
});
|
|
}
|
|
```
|
|
|
|
Add a public `get changeGeneration(): number` to the store.
|
|
|
|
This means: loading flag changes trigger `notify()` but subscribers skip the update because `changeGeneration` hasn't changed. Only when actual data arrives (or is invalidated) do components re-render.
|
|
|
|
Use this approach. It's simpler, backward-compatible, and eliminates the biggest source of unnecessary re-renders.
|
|
</action>
|
|
<verify>
|
|
`cd frontend && npx vite build --mode development 2>&1 | tail -5` builds without errors.
|
|
Functional test: (1) App starts → all views load data correctly, (2) Trigger a library scan → views update when scan completes, (3) Queue operations (add, remove, reorder) work without lag, (4) Rapid queue additions don't cause visual stuttering.
|
|
</verify>
|
|
<done>Queue store uses queueMicrotask batching. Library store has changeGeneration counter. LibraryController skips requestUpdate when only loading state changed. Result: fewer unnecessary component re-renders during data loading.</done>
|
|
</task>
|
|
|
|
</tasks>
|
|
|
|
<verification>
|
|
After both tasks:
|
|
1. Build succeeds
|
|
2. All click/dblclick/contextmenu/drag interactions work on track list and queue panel
|
|
3. Selection (click, Shift+click, Ctrl+click) still works
|
|
4. Queue operations are responsive
|
|
5. Library scan invalidation still triggers view updates
|
|
6. No regressions in any view's functionality
|
|
</verification>
|
|
|
|
<success_criteria>
|
|
- renderTrackRow creates 0 inline closures (all delegation)
|
|
- renderTrackItem creates 0 inline closures (all delegation)
|
|
- queue-store.ts contains queueMicrotask batching
|
|
- library-store.ts has changeGeneration counter
|
|
- LibraryController checks changeGeneration before requestUpdate
|
|
- All existing interactions work correctly
|
|
</success_criteria>
|
|
|
|
<output>
|
|
After completion, create `.planning/phases/14-performance-optimization/14-03-SUMMARY.md`
|
|
</output>
|