Fix searching by author #1440

GianniCarlo · 2026-01-28T14:19:28Z

Bugfix

Search was not considering the author field

Copilot

Pull request overview

This PR fixes a bug where the search functionality was not considering the author field when filtering books. The fix adds author/details field searching to both the buildFilterPredicate method and the searchAllBooks method in LibraryService. Additionally, the PR includes new logic in SearchViewModel to handle bound books in search results.

Changes:

Extended search predicates in LibraryService to include the details (author) field in addition to the title field
Added processBoundBookResults method in SearchViewModel to replace child items of bound books with the bound book itself in search results

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
Shared/Services/LibraryService.swift	Updated two search predicates to search both title and details/author fields
BookPlayer/Search/SearchViewModel.swift	Added bound book result processing and updated to use the processed results

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-28T14:24:53Z

BookPlayer/Search/SearchViewModel.swift

+  private func processBoundBookResults(_ items: [SimpleLibraryItem]) -> [SimpleLibraryItem] {
+    var resultItems: [SimpleLibraryItem] = []
+    var addedBoundPaths: Set<String> = []
+
+    for item in items {
+      // Check if this item is inside a bound book
+      if let parentPath = item.parentFolder,
+         let parentType = libraryService.getItemProperty("type", relativePath: parentPath) as? Int16,
+         parentType == SimpleItemType.bound.rawValue {
+        // Parent is a bound book - add the parent instead (if not already added)
+        if !addedBoundPaths.contains(parentPath) {
+          if let boundItem = libraryService.getSimpleItem(with: parentPath) {
+            resultItems.append(boundItem)
+            addedBoundPaths.insert(parentPath)
+          }
+        }
+      } else {
+        // Regular item or bound book itself - add directly
+        resultItems.append(item)
+      }
+    }
+
+    return resultItems
+  }


The processBoundBookResults method lacks test coverage. Consider adding tests to verify that books inside bound folders are correctly replaced with the bound folder itself in search results, and that duplicate bound folders are not added.

Copilot · 2026-01-28T14:24:53Z

BookPlayer/Search/SearchViewModel.swift

+        // Regular item or bound book itself - add directly
+        resultItems.append(item)


Potential bug: If a bound book itself matches the search criteria AND one of its child items also matches, the bound book will be added twice to the results. The first time when the bound book is processed (line 111) and the second time when its child is processed (line 105). Consider tracking bound books added via line 111 in the addedBoundPaths set to prevent duplicates.

Suggested change

// Regular item or bound book itself - add directly

resultItems.append(item)

// Regular item or bound book itself

// If this item is itself a bound book, track it in addedBoundPaths

if let itemType = libraryService.getItemProperty("type", relativePath: item.relativePath) as? Int16,

itemType == SimpleItemType.bound.rawValue {

if !addedBoundPaths.contains(item.relativePath) {

resultItems.append(item)

addedBoundPaths.insert(item.relativePath)

}

} else {

// Non-bound items are added directly

resultItems.append(item)

}

Copilot · 2026-01-28T14:24:54Z

Shared/Services/LibraryService.swift

+          format: "title CONTAINS[cd] %@ OR details CONTAINS[cd] %@",
+          query, query


The predicate format string is inconsistent with the one in buildFilterPredicate (lines 448-452). Consider using the %K format specifier with #keyPath for type safety and consistency, similar to how it's done in buildFilterPredicate. The current implementation should be:
format: "%K CONTAINS[cd] %@ OR %K CONTAINS[cd] %@", #keyPath(LibraryItem.title), query, #keyPath(LibraryItem.details), query

Suggested change

format: "title CONTAINS[cd] %@ OR details CONTAINS[cd] %@",

query, query

format: "%K CONTAINS[cd] %@ OR %K CONTAINS[cd] %@",

#keyPath(LibraryItem.title), query,

#keyPath(LibraryItem.details), query

Copilot · 2026-01-28T14:24:54Z

BookPlayer/Search/SearchViewModel.swift

+    // Process results to handle bound books properly
+    let processedResults = processBoundBookResults(allResults)
+
    // Group results by parent folder
-    let groupedResults = groupResultsByFolder(allResults)
-    
+    let groupedResults = groupResultsByFolder(processedResults)
+
    self.searchSections = groupedResults
  }
+
+  /// When a book inside a bound folder matches, replace it with the bound folder itself.
+  /// Bound books should appear as single items, not as sections with their internal chapters.
+  private func processBoundBookResults(_ items: [SimpleLibraryItem]) -> [SimpleLibraryItem] {
+    var resultItems: [SimpleLibraryItem] = []
+    var addedBoundPaths: Set<String> = []
+
+    for item in items {
+      // Check if this item is inside a bound book
+      if let parentPath = item.parentFolder,
+         let parentType = libraryService.getItemProperty("type", relativePath: parentPath) as? Int16,
+         parentType == SimpleItemType.bound.rawValue {
+        // Parent is a bound book - add the parent instead (if not already added)
+        if !addedBoundPaths.contains(parentPath) {
+          if let boundItem = libraryService.getSimpleItem(with: parentPath) {
+            resultItems.append(boundItem)
+            addedBoundPaths.insert(parentPath)
+          }
+        }
+      } else {
+        // Regular item or bound book itself - add directly
+        resultItems.append(item)
+      }
+    }
+
+    return resultItems
+  }


The processBoundBookResults method and its usage appear unrelated to the PR description which states "Search was not considering the author field". This change handles bound book result processing but doesn't address author field searching. Consider separating unrelated changes into different PRs for better code review and version control history.

Copilot · 2026-01-28T14:24:54Z

Shared/Services/LibraryService.swift

      predicates.append(
        NSPredicate(
-          format: "%K CONTAINS[cd] %@",
+          format: "%K CONTAINS[cd] %@ OR %K CONTAINS[cd] %@",
          #keyPath(LibraryItem.title),
+          query,
+          #keyPath(LibraryItem.details),
          query
        )
      )


The added author/details search functionality lacks test coverage. Consider adding tests to verify that searching by the author field works correctly, for example by creating books with different authors and verifying that searches return the correct results based on author matches.

Copilot · 2026-01-28T14:24:54Z

Shared/Services/LibraryService.swift

      predicates.append(
        NSPredicate(
-          format: "title CONTAINS[cd] %@",
-          query
+          format: "title CONTAINS[cd] %@ OR details CONTAINS[cd] %@",
+          query, query
        )
      )
    }


The searchAllBooks method lacks test coverage. Consider adding tests to verify the search functionality works correctly, including tests that verify searching by author/details field returns the expected results.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-29T17:21:14Z

BookPlayer/Search/SearchViewModel.swift

+    // Process results to handle bound books properly
+    let processedResults = processBoundBookResults(allResults)
+
    // Group results by parent folder
-    let groupedResults = groupResultsByFolder(allResults)
-    
+    let groupedResults = groupResultsByFolder(processedResults)
+
    self.searchSections = groupedResults
  }
+
+  /// When a book inside a bound folder matches, replace it with the bound folder itself.
+  /// Bound books should appear as single items, not as sections with their internal chapters.
+  private func processBoundBookResults(_ items: [SimpleLibraryItem]) -> [SimpleLibraryItem] {
+    var resultItems: [SimpleLibraryItem] = []
+    var addedBoundPaths: Set<String> = []
+
+    for item in items {
+      // Check if this item is inside a bound book
+      if let parentPath = item.parentFolder,
+         let parentType = libraryService.getItemProperty("type", relativePath: parentPath) as? Int16,
+         parentType == SimpleItemType.bound.rawValue {
+        // Parent is a bound book - add the parent instead (if not already added)
+        if !addedBoundPaths.contains(parentPath) {
+          if let boundItem = libraryService.getSimpleItem(with: parentPath) {
+            resultItems.append(boundItem)
+            addedBoundPaths.insert(parentPath)
+          }
+        }
+      } else if item.type == .bound {
+        // Bound book itself matched - add if not already added via child match
+        if !addedBoundPaths.contains(item.relativePath) {
+          resultItems.append(item)
+          addedBoundPaths.insert(item.relativePath)
+        }
+      } else {
+        // Regular book or folder - add directly
+        resultItems.append(item)
+      }
+    }
+
+    return resultItems
+  }


The processBoundBookResults method is a significant new feature that handles bound books specially during search. However, this functionality is not mentioned in the PR description, which only states "Search was not considering the author field".

This addition changes the search behavior for bound books (treating them as single items rather than showing their internal chapters), which could be considered a separate enhancement. Consider either:

Updating the PR description to mention this bound book handling improvement, or

Moving this enhancement to a separate PR to keep changes focused

This would make the PR clearer for reviewers and help with tracking what changes were made and why.

Copilot · 2026-01-29T17:21:15Z

Shared/Services/LibraryService.swift

+    // Add search query predicate if provided (searches both title and author/details)
    if let query = query, !query.isEmpty {
      predicates.append(
        NSPredicate(
-          format: "title CONTAINS[cd] %@",
+          format: "%K CONTAINS[cd] %@ OR %K CONTAINS[cd] %@",
+          #keyPath(LibraryItem.title),
+          query,
+          #keyPath(LibraryItem.details),
          query
        )
      )


While the code change is correct, the test suite has test coverage for filterContents (see BookPlayerTests/Services/LibraryServiceTests.swift:1401-1460) but lacks test coverage for the newly modified searchAllBooks function. The existing tests only verify title-based filtering and don't test author/details field searching.

Consider adding test cases to verify:

Searching by author/details field returns the expected results

Searching returns items that match either title OR author/details

Case-insensitive searching works for author field

This would help prevent regressions and document the expected behavior of the search functionality.

Copilot · 2026-01-29T17:21:15Z

BookPlayer/Search/SearchViewModel.swift

+        if !addedBoundPaths.contains(parentPath) {
+          if let boundItem = libraryService.getSimpleItem(with: parentPath) {
+            resultItems.append(boundItem)
+            addedBoundPaths.insert(parentPath)


When a child item inside a bound book matches the search query, the code attempts to fetch and display the parent bound book instead. However, if libraryService.getSimpleItem(with: parentPath) returns nil (line 104), the matched child item is silently dropped from the search results without any fallback.

This could lead to unexpected behavior where a user searches for something, the search finds a match inside a bound book, but nothing appears in the results because the parent couldn't be fetched.

Consider adding a fallback to include the original item if the parent cannot be fetched, or at minimum add logging to track when this occurs. For example:

If the parent can't be fetched, either:

Add the original child item to results as a fallback, or

Log a warning to help diagnose why the parent fetch failed

Suggested change

addedBoundPaths.insert(parentPath)

addedBoundPaths.insert(parentPath)

} else {

// Fallback: parent could not be fetched, keep the original matched item

print("SearchViewModel: Warning - failed to load bound parent item at path \(parentPath). Using child item '\(item.title)' as fallback in search results.")

resultItems.append(item)

Copilot · 2026-01-29T17:21:15Z

BookPlayer/Search/SearchViewModel.swift

+    for item in items {
+      // Check if this item is inside a bound book
+      if let parentPath = item.parentFolder,
+         let parentType = libraryService.getItemProperty("type", relativePath: parentPath) as? Int16,


The processBoundBookResults method calls libraryService.getItemProperty for each item in the search results (line 100). This results in a separate database fetch for each item that has a parent folder, which could lead to N+1 query performance issues when processing large search result sets.

Consider optimizing this by:

Fetching all parent types in a batch query before the loop, or

Including the parent type information in the initial search query results so it's already available

This would be especially beneficial for searches that return many items with parent folders, as it would reduce the number of database round-trips.

Suggested change

for item in items {

// Check if this item is inside a bound book

if let parentPath = item.parentFolder,

let parentType = libraryService.getItemProperty("type", relativePath: parentPath) as? Int16,

// Precompute parent folder types to avoid repeated lookups (potential N+1 queries)

let parentPaths = Set(items.compactMap { $0.parentFolder })

var parentTypes: [String: Int16] = [:]

for path in parentPaths {

if let type = libraryService.getItemProperty("type", relativePath: path) as? Int16 {

parentTypes[path] = type

}

}

for item in items {

// Check if this item is inside a bound book

if let parentPath = item.parentFolder,

let parentType = parentTypes[parentPath],

Fix searching by author

b289bfd

GianniCarlo requested a review from Copilot January 28, 2026 14:19

GianniCarlo self-assigned this Jan 28, 2026

Copilot started reviewing on behalf of GianniCarlo January 28, 2026 14:19 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

address feedback

175da81

GianniCarlo requested a review from Copilot January 29, 2026 17:16

Copilot started reviewing on behalf of GianniCarlo January 29, 2026 17:16 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

		// Regular item or bound book itself - add directly
		resultItems.append(item)

		format: "title CONTAINS[cd] %@ OR details CONTAINS[cd] %@",
		query, query

Uh oh!

Fix searching by author #1440

Are you sure you want to change the base?

Fix searching by author #1440

Uh oh!

Conversation

GianniCarlo commented Jan 28, 2026

Bugfix

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants