Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,12 @@ Before completing non-trivial features, verify:
- [ ] Lists use `LazyVStack`/`LazyHStack` for large datasets
- [ ] Network calls cancelled on view disappear (`.task` handles this)
- [ ] Parsers have `measure {}` tests if processing large payloads
- [ ] Images use `ImageCache` (not loading inline)
- [ ] Images use `ImageCache` with appropriate `targetSize` (not loading inline)
- [ ] Search input is debounced (not firing on every keystroke)
- [ ] ForEach uses stable identity (avoid `Array(enumerated())` unless rank is needed)
- [ ] Frequently updating UI (e.g., progress) caches formatted strings

> πŸ“š See [docs/architecture.md#performance-guidelines](docs/architecture.md#performance-guidelines) for detailed patterns.

## Task Planning: Phases with Exit Criteria

Expand Down
25 changes: 20 additions & 5 deletions Core/Services/API/APICache.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,19 @@ final class APICache {
/// Maximum number of cached entries before LRU eviction kicks in.
private static let maxEntries = 50

private var cache: [String: CacheEntry] = [:]
/// Pre-allocated dictionary with initial capacity to reduce rehashing.
private var cache: [String: CacheEntry]

private init() {}
/// Timestamp of last eviction to avoid running on every access.
private var lastEvictionTime: Date = .distantPast

/// Minimum interval between automatic evictions (30 seconds).
private static let evictionInterval: TimeInterval = 30

private init() {
// Pre-allocate capacity to avoid rehashing during normal operation
self.cache = Dictionary(minimumCapacity: Self.maxEntries)
}

/// Gets cached data if available and not expired.
func get(key: String) -> [String: Any]? {
Expand All @@ -61,15 +71,20 @@ final class APICache {
/// Stores data in the cache with the specified TTL.
/// Evicts least recently used entries if cache is at capacity.
func set(key: String, data: [String: Any], ttl: TimeInterval) {
// Evict expired entries first
self.evictExpiredEntries()
let now = Date()

// Evict expired entries periodically (not on every set)
if now.timeIntervalSince(self.lastEvictionTime) > Self.evictionInterval {
self.evictExpiredEntries()
self.lastEvictionTime = now
}

// Evict LRU entries if still at capacity
while self.cache.count >= Self.maxEntries {
self.evictLeastRecentlyUsed()
}

self.cache[key] = CacheEntry(data: data, timestamp: Date(), ttl: ttl)
self.cache[key] = CacheEntry(data: data, timestamp: now, ttl: ttl)
}

/// Generates a stable, deterministic cache key from endpoint and request body.
Expand Down
16 changes: 14 additions & 2 deletions Core/Services/API/YTMusicClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ final class YTMusicClient: YTMusicClientProtocol {
"User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.0 Safari/605.1.15",
"Accept-Encoding": "gzip, deflate, br",
]
// Enable HTTP pipelining for faster sequential requests
configuration.httpShouldUsePipelining = true
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP pipelining (httpShouldUsePipelining) is deprecated and largely unsupported by modern servers, including YouTube Music's API. Most HTTP/1.1 servers don't support pipelining due to security and performance concerns, and HTTP/2 makes it obsolete with multiplexing. This setting will likely have no effect and could potentially cause issues with some proxy configurations. Consider removing this setting or documenting that it may not have the intended effect.

Suggested change
// Enable HTTP pipelining for faster sequential requests
configuration.httpShouldUsePipelining = true

Copilot uses AI. Check for mistakes.
// Increase connection pool for parallel requests
configuration.httpMaximumConnectionsPerHost = 6
// Use shared URL cache for transport-level caching
configuration.urlCache = URLCache.shared
configuration.requestCachePolicy = .useProtocolCachePolicy
// Reduce timeout for faster failure detection
configuration.timeoutIntervalForRequest = 15
configuration.timeoutIntervalForResource = 30
self.session = URLSession(configuration: configuration)
}

Expand Down Expand Up @@ -1203,8 +1213,10 @@ final class YTMusicClient: YTMusicClientProtocol {
// Handle errors back on main actor
switch result {
case let .success(data):
// Parse JSON - URLSession already decompresses gzip/deflate on a background thread,
// and JSONSerialization is very fast for typical response sizes (~5-15ms)
// Parse JSON synchronously - JSONSerialization is highly optimized
// and typically completes in <5ms even for large responses.
// The actual response parsing (in Parsers/) is more expensive
// but must happen on MainActor anyway for @Observable updates.
guard let json = try JSONSerialization.jsonObject(with: data) as? [String: Any] else {
throw YTMusicError.parseError(message: "Response is not a JSON object")
}
Expand Down
2 changes: 1 addition & 1 deletion Core/Services/WebKit/WebKitManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ final class WebKitManager: NSObject, WebKitManagerProtocol {
private var cookieDebounceTask: Task<Void, Never>?

/// Minimum interval between cookie backup operations (in seconds).
private static let cookieDebounceInterval: Duration = .seconds(2)
private static let cookieDebounceInterval: Duration = .seconds(5)

/// The YouTube Music origin URL.
static let origin = "https://music.youtube.com"
Expand Down
78 changes: 63 additions & 15 deletions Core/Utilities/ImageCache.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,16 @@ actor ImageCache {
/// Maximum disk cache size in bytes (200MB).
private static let maxDiskCacheSize: Int64 = 200 * 1024 * 1024

/// Priority levels for image loading to ensure visible images load first.
enum Priority: Sendable {
case high // Visible on screen
case low // Prefetch for upcoming content
}

Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Priority enum is defined with documentation suggesting it's used to prioritize visible images over prefetch content, but it's never actually used in the implementation. The enum has no effect on the code. Consider either implementing priority-based loading or removing this unused enum to avoid confusion.

Suggested change
/// Priority levels for image loading to ensure visible images load first.
enum Priority: Sendable {
case high // Visible on screen
case low // Prefetch for upcoming content
}

Copilot uses AI. Check for mistakes.
private let memoryCache = NSCache<NSURL, NSImage>()
private var inFlight: [URL: Task<NSImage?, Never>] = [:]
/// Tracks active prefetch task groups to allow cancellation.
private var activePrefetchTasks: [UUID: Task<Void, Never>] = [:]
private let fileManager = FileManager.default
private let diskCacheURL: URL

Expand Down Expand Up @@ -99,29 +107,69 @@ actor ImageCache {
}

/// Prefetches images with controlled concurrency to avoid network congestion.
/// Returns a prefetch ID that can be used to cancel the prefetch operation.
/// - Parameters:
/// - urls: URLs to prefetch.
/// - targetSize: Optional target size for downsampling.
/// - maxConcurrent: Maximum number of concurrent fetches (default: 4).
/// - Returns: A UUID that can be passed to `cancelPrefetch` to stop the operation.
@discardableResult
func prefetch(urls: [URL], targetSize: CGSize? = nil, maxConcurrent: Int = maxConcurrentPrefetch)
async
async -> UUID
{
await withTaskGroup(of: Void.self) { group in
var inProgress = 0
for url in urls {
// Wait for a slot if we're at capacity
if inProgress >= maxConcurrent {
await group.next()
inProgress -= 1
}

group.addTask(priority: .utility) {
_ = await self.image(for: url, targetSize: targetSize)
let prefetchId = UUID()

let task = Task(priority: .utility) {
await withTaskGroup(of: Void.self) { group in
var inProgress = 0
for url in urls {
// Check cancellation before starting new work
guard !Task.isCancelled else { break }

// Skip if already in memory cache
if self.memoryCache.object(forKey: url as NSURL) != nil {
continue
}

// Wait for a slot if we're at capacity
if inProgress >= maxConcurrent {
await group.next()
inProgress -= 1
}

group.addTask(priority: .utility) {
guard !Task.isCancelled else { return }
_ = await self.image(for: url, targetSize: targetSize)
}
inProgress += 1
}
inProgress += 1
// Wait for remaining tasks
await group.waitForAll()
}
// Wait for remaining tasks
await group.waitForAll()
}

self.activePrefetchTasks[prefetchId] = task

// Wait for completion and cleanup
await task.value
self.activePrefetchTasks.removeValue(forKey: prefetchId)

return prefetchId
}

/// Cancels an active prefetch operation.
func cancelPrefetch(id: UUID) {
if let task = activePrefetchTasks[id] {
task.cancel()
self.activePrefetchTasks.removeValue(forKey: id)
}
}

/// Cancels all active prefetch operations.
func cancelAllPrefetches() {
for (id, task) in self.activePrefetchTasks {
task.cancel()
self.activePrefetchTasks.removeValue(forKey: id)
}
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutating a dictionary while iterating over it can lead to undefined behavior. The code removes values from activePrefetchTasks while iterating, which is unsafe. Consider using removeAll() after the loop, or iterate over a copy of the keys.

Suggested change
for (id, task) in self.activePrefetchTasks {
task.cancel()
self.activePrefetchTasks.removeValue(forKey: id)
}
for (_, task) in self.activePrefetchTasks {
task.cancel()
}
self.activePrefetchTasks.removeAll()

Copilot uses AI. Check for mistakes.
}

Expand Down
25 changes: 23 additions & 2 deletions Views/macOS/CachedAsyncImage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ struct CachedAsyncImage<Content: View, Placeholder: View>: View {

@State private var image: NSImage?
@State private var isLoaded = false
@State private var loadTask: Task<Void, Never>?

/// Whether to animate the image appearance.
private var shouldAnimate: Bool {
Expand All @@ -30,10 +31,30 @@ struct CachedAsyncImage<Content: View, Placeholder: View>: View {
self.placeholder()
}
}
.onChange(of: self.url) { oldUrl, newUrl in
// Cancel previous load when URL changes
if oldUrl != newUrl {
self.loadTask?.cancel()
self.image = nil
self.isLoaded = false
}
}
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onChange handler at lines 34-41 is redundant with the .task(id:) modifier at line 42. The .task modifier already cancels and restarts when the id (url) changes, so manually cancelling in onChange and resetting state creates duplicate logic. This could lead to race conditions where both cancellations occur simultaneously. Consider removing the onChange block entirely and relying solely on .task(id:) for URL change handling.

Copilot uses AI. Check for mistakes.
.task(id: self.url) {
guard let url else { return }
self.image = await ImageCache.shared.image(for: url, targetSize: self.targetSize)
self.isLoaded = true
// Cancel any previous load task
self.loadTask?.cancel()

let task = Task {
let loadedImage = await ImageCache.shared.image(for: url, targetSize: self.targetSize)
guard !Task.isCancelled else { return }
self.image = loadedImage
self.isLoaded = true
}
self.loadTask = task
await task.value
}
.onDisappear {
self.loadTask?.cancel()
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions Views/macOS/HomeView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,9 @@ struct HomeView: View {
.staggeredAppearance(index: 0)
}

// API sections
ForEach(Array(self.viewModel.sections.enumerated()), id: \.element.id) { index, section in
// API sections - use stable id without array enumeration
ForEach(self.viewModel.sections) { section in
self.sectionView(section)
.staggeredAppearance(index: self.favoritesManager.isVisible ? index + 1 : index)
.task {
await self.prefetchImagesAsync(for: section)
}
Expand All @@ -93,6 +92,7 @@ struct HomeView: View {

ScrollView(.horizontal, showsIndicators: false) {
LazyHStack(spacing: 16) {
// Use stable ID from items, avoid enumeration for non-chart sections
if section.isChart {
ForEach(Array(section.items.enumerated()), id: \.element.id) { index, item in
HomeSectionItemCard(item: item, rank: index + 1) {
Expand All @@ -103,12 +103,12 @@ struct HomeView: View {
}
}
} else {
ForEach(Array(section.items.enumerated()), id: \.element.id) { index, item in
ForEach(section.items) { item in
HomeSectionItemCard(item: item) {
self.playItem(item, in: section, at: index)
self.playItem(item, in: section, at: 0)
}
.contextMenu {
self.contextMenuItems(for: item, in: section, at: index)
self.contextMenuItems(for: item, in: section, at: 0)
}
}
}
Expand Down
23 changes: 19 additions & 4 deletions Views/macOS/PlayerBar.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ struct PlayerBar: View {
@State private var volumeValue: Double = 1.0
@State private var isAdjustingVolume = false

/// Cached formatted progress string to avoid repeated formatting.
@State private var formattedProgress: String = "0:00"
@State private var formattedRemaining: String = "-0:00"
/// Last integer second of progress to reduce string formatting frequency.
@State private var lastProgressSecond: Int = -1

var body: some View {
GlassEffectContainer(spacing: 0) {
HStack(spacing: 0) {
Expand Down Expand Up @@ -102,6 +108,13 @@ struct PlayerBar: View {
if !self.isSeeking, self.playerService.duration > 0 {
self.seekValue = newValue / self.playerService.duration
}
// Only update formatted strings when the second changes to reduce Text view updates
let currentSecond = Int(newValue)
if currentSecond != self.lastProgressSecond {
self.lastProgressSecond = currentSecond
self.formattedProgress = self.formatTime(newValue)
self.formattedRemaining = "-\(self.formatTime(self.playerService.duration - newValue))"
}
}
.onChange(of: self.playerService.volume) { _, newValue in
// Sync local volume value when not actively adjusting
Expand Down Expand Up @@ -175,11 +188,12 @@ struct PlayerBar: View {

private var seekBarView: some View {
HStack(spacing: 10) {
// Elapsed time - show seek position while dragging, actual progress otherwise
Text(self.formatTime(self.isSeeking ? self.seekValue * self.playerService.duration : self.playerService.progress))
// Elapsed time - use cached formatted string when not seeking
Text(self.isSeeking ? self.formatTime(self.seekValue * self.playerService.duration) : self.formattedProgress)
.font(.system(size: 11))
.foregroundStyle(.secondary)
.frame(minWidth: 45, alignment: .trailing)
.monospacedDigit()

// Seek slider
Slider(value: self.$seekValue, in: 0 ... 1) { editing in
Expand All @@ -193,11 +207,12 @@ struct PlayerBar: View {
}
.controlSize(.small)

// Remaining time
Text("-\(self.formatTime(self.playerService.duration - (self.isSeeking ? self.seekValue * self.playerService.duration : self.playerService.progress)))")
// Remaining time - use cached formatted string when not seeking
Text(self.isSeeking ? "-\(self.formatTime(self.playerService.duration - self.seekValue * self.playerService.duration))" : self.formattedRemaining)
.font(.system(size: 11))
.foregroundStyle(.secondary)
.frame(minWidth: 45, alignment: .leading)
.monospacedDigit()
}
}

Expand Down
7 changes: 3 additions & 4 deletions Views/macOS/SearchView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,8 @@ struct SearchView: View {
private var resultsView: some View {
ScrollView {
LazyVStack(spacing: 0) {
ForEach(Array(self.viewModel.filteredItems.enumerated()), id: \.element.id) { index, item in
self.resultRow(item, index: index)
ForEach(self.viewModel.filteredItems) { item in
self.resultRow(item)
Divider()
.padding(.leading, 72)
}
Expand Down Expand Up @@ -327,7 +327,7 @@ struct SearchView: View {
}
}

private func resultRow(_ item: SearchResultItem, index: Int) -> some View {
private func resultRow(_ item: SearchResultItem) -> some View {
Button {
self.handleItemTap(item)
} label: {
Expand Down Expand Up @@ -386,7 +386,6 @@ struct SearchView: View {
.contentShape(Rectangle())
}
.buttonStyle(.interactiveRow(cornerRadius: 6))
.staggeredAppearance(index: min(index, 10))
.contextMenu {
self.contextMenuItems(for: item)
}
Expand Down
Loading
Loading