perf: optimize network, caching, and SwiftUI rendering#57
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements performance optimizations across network requests, caching layers, and SwiftUI rendering. The changes focus on reducing unnecessary computations, improving responsiveness, and optimizing memory usage.
Key changes include:
- Network configuration optimizations with HTTP pipelining, increased connection pool size, and reduced timeouts
- Enhanced image cache with prefetch cancellation support and memory-aware loading
- SwiftUI rendering improvements by eliminating unstable ForEach identities and caching formatted strings
- API cache optimizations with pre-allocated capacity and periodic eviction
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/playback.md | Whitespace-only formatting changes |
| docs/architecture.md | Added comprehensive performance guidelines section documenting network, caching, and SwiftUI optimization patterns |
| AGENTS.md | Updated profiling checklist with additional performance verification items |
| Views/macOS/SearchView.swift | Removed Array enumeration from ForEach to use stable identity and removed staggered appearance animation |
| Views/macOS/HomeView.swift | Removed Array enumeration for non-chart sections to optimize ForEach rendering |
| Views/macOS/PlayerBar.swift | Added cached formatted time strings with throttled updates to reduce Text view re-renders; added monospacedDigit modifier |
| Views/macOS/CachedAsyncImage.swift | Added task cancellation on URL changes and view disappearance to prevent memory buildup |
| Core/Utilities/ImageCache.swift | Added Priority enum (unused), prefetch cancellation support, and memory cache checks during prefetch |
| Core/Services/API/YTMusicClient.swift | Enabled HTTP pipelining, increased connection pool to 6, added URL cache with protocol cache policy, reduced timeouts |
| Core/Services/API/APICache.swift | Pre-allocated dictionary capacity and changed to periodic eviction (every 30s) instead of per-write |
| Core/Services/WebKit/WebKitManager.swift | Increased cookie debounce interval from 2 to 5 seconds to reduce I/O operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Core/Utilities/ImageCache.swift
Outdated
| /// 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 | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| /// 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 | |
| } |
Core/Utilities/ImageCache.swift
Outdated
| for (id, task) in self.activePrefetchTasks { | ||
| task.cancel() | ||
| self.activePrefetchTasks.removeValue(forKey: id) | ||
| } |
There was a problem hiding this comment.
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.
| for (id, task) in self.activePrefetchTasks { | |
| task.cancel() | |
| self.activePrefetchTasks.removeValue(forKey: id) | |
| } | |
| for (_, task) in self.activePrefetchTasks { | |
| task.cancel() | |
| } | |
| self.activePrefetchTasks.removeAll() |
Views/macOS/CachedAsyncImage.swift
Outdated
| .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 | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| // Enable HTTP pipelining for faster sequential requests | ||
| configuration.httpShouldUsePipelining = true |
There was a problem hiding this comment.
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.
| // Enable HTTP pipelining for faster sequential requests | |
| configuration.httpShouldUsePipelining = true |
docs/architecture.md
Outdated
|
|
||
| ```swift | ||
| let configuration = URLSessionConfiguration.default | ||
| configuration.httpShouldUsePipelining = true // Parallel requests on same connection |
There was a problem hiding this comment.
The comment says "Parallel requests on same connection" but HTTP pipelining doesn't provide parallelism - it only allows sending multiple requests without waiting for responses (sequential pipelining). True parallel requests on the same connection require HTTP/2 multiplexing, which URLSession uses automatically when available. The comment is misleading about what this setting does.
| configuration.httpShouldUsePipelining = true // Parallel requests on same connection | |
| configuration.httpShouldUsePipelining = true // Enable HTTP/1.1 request pipelining (sequential; HTTP/2 multiplexing is automatic) |
Description
Type of Change
Related Issues
Changes Made
Testing
xcodebuild test -only-testing:KasetTests)Checklist
swiftlint --strict && swiftformat .Screenshots
Additional Notes