Conversation
There was a problem hiding this comment.
Pull request overview
Adds integration library search (Jellyfin + Audiobookshelf) and expands exported debug logs with detailed storage usage breakdown.
Changes:
- Add searchable UI + query handling to Jellyfin and Audiobookshelf library screens.
- Add Audiobookshelf search API call and pass search term to Jellyfin items endpoint.
- Add storage breakdown details to the debug transferable export.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| BookPlayer/Settings/Sections/DebugFileTransferable.swift | Adds a storage breakdown section to debug export output. |
| BookPlayer/Jellyfin/Network/JellyfinConnectionService.swift | Extends item fetch API to accept an optional search term. |
| BookPlayer/Jellyfin/Library Screen/JellyfinLibraryViewModel.swift | Adds debounced search query state and refetch-on-search behavior. |
| BookPlayer/Jellyfin/Library Screen/JellyfinLibraryView.swift | Adds conditional .searchable and wraps grid layout in a ScrollView. |
| BookPlayer/Jellyfin/Library Screen/GridLayout/JellyfinLibraryGridView.swift | Switches grid implementation to LazyVGrid. |
| BookPlayer/AudiobookShelf/Network/AudiobookShelfConnectionService.swift | Adds a dedicated search endpoint call. |
| BookPlayer/AudiobookShelf/Library Screen/GridLayout/AudiobookShelfLibraryGridView.swift | Switches grid implementation to LazyVGrid. |
| BookPlayer/AudiobookShelf/Library Screen/AudiobookShelfLibraryViewModel.swift | Adds debounced search query state and switches between list vs search requests. |
| BookPlayer/AudiobookShelf/Library Screen/AudiobookShelfLibraryView.swift | Adds conditional .searchable and wraps grid layout in a ScrollView. |
| BookPlayer/AudiobookShelf/Library Screen/AudiobookShelfLibraryItem.swift | Adds Codable models for Audiobookshelf search response parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private func onSearchQueryChanged() { | ||
| guard let folderID else { return } | ||
| fetchTask?.cancel() | ||
| fetchTask = nil | ||
| items = [] | ||
| nextStartItemIndex = 0 | ||
| totalItems = Int.max | ||
| fetchFolderItems(folderID: folderID) | ||
| } |
There was a problem hiding this comment.
When the search query changes, the list is reset but selectedItems is not cleared. This can leave stale selections for items no longer visible and may enable actions (download/delete) on outdated IDs. Consider clearing selectedItems (and potentially exiting edit mode) when resetting items for a new search.
| private func onSearchQueryChanged() { | ||
| guard let libraryID else { return } | ||
| fetchTask?.cancel() | ||
| fetchTask = nil | ||
| items = [] | ||
| nextPage = 0 | ||
| totalItems = Int.max | ||
|
|
||
| if searchQuery.isEmpty { | ||
| fetchLibraryItems(libraryID: libraryID) | ||
| } else { | ||
| searchLibraryItems(libraryID: libraryID, query: searchQuery) | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the Jellyfin view model, changing the search query resets items but does not clear selectedItems. This can keep selections around for items that are no longer part of the current result set. Clear selectedItems (and potentially reset editMode) when starting a new search / returning to the full list.
| private struct ConditionalSearchableModifier: ViewModifier { | ||
| let isSearchable: Bool | ||
| @Binding var text: String | ||
|
|
||
| func body(content: Content) -> some View { | ||
| if isSearchable { | ||
| content.searchable(text: $text, placement: .navigationBarDrawer(displayMode: .always)) | ||
| } else { | ||
| content | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There are now two near-identical conditional searchable modifiers (ConditionalSearchableModifier here and JellyfinSearchableModifier in JellyfinLibraryView). Consider extracting a single shared View extension / modifier to reduce duplication and keep behavior consistent across integrations.
| libraryRepresentation += remoteOnlyInfo | ||
| } | ||
|
|
||
| libraryRepresentation += file.getStorageBreakdown() |
There was a problem hiding this comment.
The storage breakdown does a full recursive enumeration across multiple directories and is executed synchronously while building the debug export string. On large libraries this can be slow and may block the caller (potentially the UI). Consider computing this off the main thread (e.g., make the breakdown async and await it from a Task) or gating it behind an explicit 'include storage breakdown' flag.
| /// Get the size of a folder's contents | ||
| private func getFolderSize(_ url: URL, skipHidden: Bool) -> Int64 { | ||
| let fm = FileManager.default | ||
| guard fm.fileExists(atPath: url.path) else { return 0 } | ||
|
|
||
| var options: FileManager.DirectoryEnumerationOptions = [] | ||
| if skipHidden { | ||
| options.insert(.skipsHiddenFiles) | ||
| } | ||
|
|
||
| guard let enumerator = fm.enumerator( | ||
| at: url, | ||
| includingPropertiesForKeys: [.fileSizeKey, .isRegularFileKey], | ||
| options: options | ||
| ) else { return 0 } | ||
|
|
||
| var size: Int64 = 0 | ||
| for case let fileURL as URL in enumerator { | ||
| guard let values = try? fileURL.resourceValues(forKeys: [.fileSizeKey, .isRegularFileKey]), | ||
| values.isRegularFile == true else { continue } | ||
| size += Int64(values.fileSize ?? 0) | ||
| } | ||
| return size | ||
| } |
There was a problem hiding this comment.
The storage breakdown does a full recursive enumeration across multiple directories and is executed synchronously while building the debug export string. On large libraries this can be slow and may block the caller (potentially the UI). Consider computing this off the main thread (e.g., make the breakdown async and await it from a Task) or gating it behind an explicit 'include storage breakdown' flag.
| in folderID: String, | ||
| startIndex: Int?, | ||
| limit: Int?, | ||
| sortBy: JellyfinLayout.SortBy | ||
| sortBy: JellyfinLayout.SortBy, | ||
| searchTerm: String? = nil | ||
| ) async throws -> (items: [JellyfinLibraryItem], nextStartIndex: Int, maxCountItems: Int) { |
There was a problem hiding this comment.
Consider normalizing searchTerm inside the service (e.g., trimming whitespace and treating an empty string as nil). As written, passing \"\" would enable isRecursive and send an empty searchTerm, which can unintentionally broaden the query and increase server/load time.
| let parameters = Paths.GetItemsParameters( | ||
| startIndex: startIndex, | ||
| limit: limit, | ||
| isRecursive: false, | ||
| isRecursive: searchTerm != nil, | ||
| searchTerm: searchTerm, | ||
| sortOrder: sortOrder, |
There was a problem hiding this comment.
Consider normalizing searchTerm inside the service (e.g., trimming whitespace and treating an empty string as nil). As written, passing \"\" would enable isRecursive and send an empty searchTerm, which can unintentionally broaden the query and increase server/load time.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let parameters = Paths.GetItemsParameters( | ||
| startIndex: startIndex, | ||
| limit: limit, | ||
| isRecursive: false, | ||
| isRecursive: searchTerm != nil, | ||
| searchTerm: searchTerm, | ||
| sortOrder: sortOrder, | ||
| parentID: folderID, | ||
| fields: [.sortName], |
There was a problem hiding this comment.
isRecursive is toggled based on searchTerm != nil, which treats searchTerm: "" (or whitespace-only) as a “search” and will change query behavior while also sending an effectively empty search term. Consider normalizing searchTerm (trim + convert empty to nil) and basing both isRecursive and searchTerm off the normalized value.
| private func searchLibraryItems(libraryID: String, query: String) { | ||
| fetchTask = Task { @MainActor in | ||
| defer { self.fetchTask = nil } | ||
|
|
||
| do { | ||
| let items = try await connectionService.searchItems( | ||
| in: libraryID, | ||
| query: query, | ||
| limit: 100 | ||
| ) | ||
|
|
||
| self.totalItems = items.count | ||
| self.items = items | ||
| } catch is CancellationError { |
There was a problem hiding this comment.
Search results are capped at 100 via limit: 100, and totalItems is set to items.count, which will be incorrect/incomplete when the library has more than 100 matching items. If the API supports it, either remove the hard cap (pass nil) or implement paging for search and only set totalItems from a server-provided total (or clearly distinguish “loaded count” vs “total matches”).
| func getStorageBreakdown() -> String { | ||
| var info = "\n\n--- Storage Breakdown ---\n" | ||
| let fm = FileManager.default | ||
|
|
||
| let processedURL = DataManager.getProcessedFolderURL() | ||
| let artworkCacheURL = ArtworkService.cacheDirectoryURL | ||
| let backupURL = DataManager.getBackupFolderURL() | ||
| let inboxURL = DataManager.getInboxFolderURL() | ||
| let dbBackupURL = DataManager.getDatabaseBackupFolderURL() | ||
| let syncTasksSwiftDataURL = DataManager.getSyncTasksSwiftDataURL() |
There was a problem hiding this comment.
getStorageBreakdown() recursively enumerates multiple directories and files; if this runs on the main actor/thread during debug export creation, it can noticeably block UI (especially for large libraries). Consider computing this off the main thread (e.g., in a detached task) and/or caching results for the lifetime of the export generation.
| /// Get the size of a folder's contents | ||
| private func getFolderSize(_ url: URL, skipHidden: Bool) -> Int64 { | ||
| let fm = FileManager.default | ||
| guard fm.fileExists(atPath: url.path) else { return 0 } | ||
|
|
||
| var options: FileManager.DirectoryEnumerationOptions = [] | ||
| if skipHidden { | ||
| options.insert(.skipsHiddenFiles) | ||
| } | ||
|
|
||
| guard let enumerator = fm.enumerator( | ||
| at: url, | ||
| includingPropertiesForKeys: [.fileSizeKey, .isRegularFileKey], | ||
| options: options | ||
| ) else { return 0 } | ||
|
|
||
| var size: Int64 = 0 | ||
| for case let fileURL as URL in enumerator { | ||
| guard let values = try? fileURL.resourceValues(forKeys: [.fileSizeKey, .isRegularFileKey]), | ||
| values.isRegularFile == true else { continue } | ||
| size += Int64(values.fileSize ?? 0) | ||
| } | ||
| return size | ||
| } |
There was a problem hiding this comment.
getStorageBreakdown() recursively enumerates multiple directories and files; if this runs on the main actor/thread during debug export creation, it can noticeably block UI (especially for large libraries). Consider computing this off the main thread (e.g., in a detached task) and/or caching results for the lifetime of the export generation.
| .modifier(JellyfinSearchableModifier(isSearchable: viewModel.isSearchable, text: $viewModel.searchQuery)) | ||
| .searchPresentationToolbarBehavior(.avoidHidingContent) |
There was a problem hiding this comment.
The conditional searchable modifier is duplicated with the Audiobookshelf screen (same logic, different type name). Consolidating this into a shared helper (e.g., a single ConditionalSearchableModifier in a common UI utilities file) reduces duplication and helps keep search behavior consistent between integrations.
|
|
||
| private struct JellyfinSearchableModifier: ViewModifier { | ||
| let isSearchable: Bool | ||
| @Binding var text: String | ||
|
|
||
| func body(content: Content) -> some View { | ||
| if isSearchable { | ||
| content.searchable(text: $text, placement: .navigationBarDrawer(displayMode: .always)) | ||
| } else { | ||
| content | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The conditional searchable modifier is duplicated with the Audiobookshelf screen (same logic, different type name). Consolidating this into a shared helper (e.g., a single ConditionalSearchableModifier in a common UI utilities file) reduces duplication and helps keep search behavior consistent between integrations.
| public func searchItems( | ||
| in libraryId: String, | ||
| query: String, | ||
| limit: Int? = nil | ||
| ) async throws -> [AudiobookShelfLibraryItem] { | ||
| guard let connection else { | ||
| throw URLError(.userAuthenticationRequired) | ||
| } |
There was a problem hiding this comment.
This introduces new network behavior and response decoding for Audiobookshelf search, but there’s no accompanying test coverage shown here. Adding unit tests that validate URL construction (path + query items), authorization header presence, and decoding/mapping behavior would help prevent regressions (especially since this is user-facing search).
| let decoder = JSONDecoder() | ||
| let searchResponse = try decoder.decode(AudiobookShelfSearchResponse.self, from: data) | ||
|
|
||
| return searchResponse.book.compactMap { AudiobookShelfLibraryItem(apiItem: $0.libraryItem) } | ||
| } |
There was a problem hiding this comment.
This introduces new network behavior and response decoding for Audiobookshelf search, but there’s no accompanying test coverage shown here. Adding unit tests that validate URL construction (path + query items), authorization header presence, and decoding/mapping behavior would help prevent regressions (especially since this is user-facing search).
Purpose