fix: item linking search results (#1869)

packages/web/jest.config.js

@@ -22,4 +22,5 @@ module.exports = {
     '^.+\\.(ts|tsx)?$': 'ts-jest',
     '\\.svg$': 'svg-jest',
   },
+  testEnvironment: 'jsdom',
 }

packages/web/src/javascripts/Controllers/LinkingController.spec.ts

@@ -0,0 +1,221 @@
+import { WebApplication } from '@/Application/Application'
+import {
+  AnonymousReference,
+  ContentReferenceType,
+  ContentType,
+  FileItem,
+  FileToNoteReference,
+  InternalEventBus,
+  SNNote,
+} from '@standardnotes/snjs'
+import { FilesController } from './FilesController'
+import { ItemListController } from './ItemList/ItemListController'
+import { LinkingController } from './LinkingController'
+import { NavigationController } from './Navigation/NavigationController'
+import { SelectedItemsController } from './SelectedItemsController'
+import { SubscriptionController } from './Subscription/SubscriptionController'
+
+const createNote = (name: string, options?: Partial<SNNote>) => {
+  return {
+    title: name,
+    archived: false,
+    trashed: false,
+    uuid: String(Math.random()),
+    content_type: ContentType.Note,
+    ...options,
+  } as jest.Mocked<SNNote>
+}
+
+const createFile = (name: string, options?: Partial<FileItem>) => {
+  return {
+    title: name,
+    archived: false,
+    trashed: false,
+    uuid: String(Math.random()),
+    content_type: ContentType.File,
+    ...options,
+  } as jest.Mocked<FileItem>
+}
+
+describe('LinkingController', () => {
+  let linkingController: LinkingController
+  let application: WebApplication
+  let navigationController: NavigationController
+  let selectionController: SelectedItemsController
+  let eventBus: InternalEventBus
+
+  let itemListController: ItemListController
+  let filesController: FilesController
+  let subscriptionController: SubscriptionController
+
+  beforeEach(() => {
+    application = {} as jest.Mocked<WebApplication>
+    application.getPreference = jest.fn()
+    application.addSingleEventObserver = jest.fn()
+    application.streamItems = jest.fn()
+
+    navigationController = {} as jest.Mocked<NavigationController>
+
+    selectionController = {} as jest.Mocked<SelectedItemsController>
+
+    eventBus = {} as jest.Mocked<InternalEventBus>
+
+    itemListController = {} as jest.Mocked<ItemListController>
+    filesController = {} as jest.Mocked<FilesController>
+    subscriptionController = {} as jest.Mocked<SubscriptionController>
+
+    linkingController = new LinkingController(application, navigationController, selectionController, eventBus)
+    linkingController.setServicesPostConstruction(itemListController, filesController, subscriptionController)
+  })
+
+  describe('isValidSearchResult', () => {
+    it("should not be valid result if it doesn't match query", () => {
+      const searchQuery = 'test'
+
+      const file = createFile('anotherFile')
+
+      const isFileValidResult = linkingController.isValidSearchResult(file, searchQuery)
+
+      expect(isFileValidResult).toBeFalsy()
+    })
+
+    it('should not be valid result if item is archived or trashed', () => {
+      const searchQuery = 'test'
+
+      const archived = createFile('test', { archived: true })
+
+      const trashed = createFile('test', { trashed: true })
+
+      const isArchivedFileValidResult = linkingController.isValidSearchResult(archived, searchQuery)
+      expect(isArchivedFileValidResult).toBeFalsy()
+
+      const isTrashedFileValidResult = linkingController.isValidSearchResult(trashed, searchQuery)
+      expect(isTrashedFileValidResult).toBeFalsy()
+    })
+
+    it('should not be valid result if result is active item', () => {
+      const searchQuery = 'test'
+
+      const activeItem = createFile('test', { uuid: 'same-uuid' })
+
+      Object.defineProperty(itemListController, 'activeControllerItem', { value: activeItem })
+
+      const result = createFile('test', { uuid: 'same-uuid' })
+
+      const isFileValidResult = linkingController.isValidSearchResult(result, searchQuery)
+      expect(isFileValidResult).toBeFalsy()
+    })
+
+    it('should be valid result if it matches query even case insensitive', () => {
+      const searchQuery = 'test'
+
+      const file = createFile('TeSt')
+
+      const isFileValidResult = linkingController.isValidSearchResult(file, searchQuery)
+
+      expect(isFileValidResult).toBeTruthy()
+    })
+  })
+
+  describe('isSearchResultAlreadyLinked', () => {
+    it('should be true if active item & result are same content type & active item references result', () => {
+      const activeItem = createFile('test', {
+        uuid: 'active-item',
+        references: [
+          {
+            reference_type: ContentReferenceType.FileToFile,
+            uuid: 'result',
+          } as AnonymousReference,
+        ],
+      })
+      const result = createFile('test', { uuid: 'result', references: [] })
+
+      Object.defineProperty(itemListController, 'activeControllerItem', { value: activeItem })
+
+      const isFileAlreadyLinked = linkingController.isSearchResultAlreadyLinked(result)
+      expect(isFileAlreadyLinked).toBeTruthy()
+    })
+
+    it('should be false if active item & result are same content type & result references active item', () => {
+      const activeItem = createFile('test', {
+        uuid: 'active-item',
+        references: [],
+      })
+      const result = createFile('test', {
+        uuid: 'result',
+        references: [
+          {
+            reference_type: ContentReferenceType.FileToFile,
+            uuid: 'active-item',
+          } as AnonymousReference,
+        ],
+      })
+
+      Object.defineProperty(itemListController, 'activeControllerItem', { value: activeItem })
+
+      const isFileAlreadyLinked = linkingController.isSearchResultAlreadyLinked(result)
+      expect(isFileAlreadyLinked).toBeFalsy()
+    })
+
+    it('should be true if active item & result are different content type & result references active item', () => {
+      const activeNote = createNote('test', {
+        uuid: 'active-note',
+        references: [],
+      })
+
+      const fileResult = createFile('test', {
+        uuid: 'file-result',
+        references: [
+          {
+            reference_type: ContentReferenceType.FileToNote,
+            uuid: 'active-note',
+          } as FileToNoteReference,
+        ],
+      })
+
+      Object.defineProperty(itemListController, 'activeControllerItem', { value: activeNote })
+
+      const isFileResultAlreadyLinked = linkingController.isSearchResultAlreadyLinked(fileResult)
+      expect(isFileResultAlreadyLinked).toBeTruthy()
+    })
+
+    it('should be true if active item & result are different content type & active item references result', () => {
+      const activeFile = createNote('test', {
+        uuid: 'active-file',
+        references: [
+          {
+            reference_type: ContentReferenceType.FileToNote,
+            uuid: 'note-result',
+          } as FileToNoteReference,
+        ],
+      })
+
+      const noteResult = createFile('test', {
+        uuid: 'note-result',
+        references: [],
+      })
+
+      Object.defineProperty(itemListController, 'activeControllerItem', { value: activeFile })
+
+      const isNoteResultAlreadyLinked = linkingController.isSearchResultAlreadyLinked(noteResult)
+      expect(isNoteResultAlreadyLinked).toBeTruthy()
+    })
+
+    it('should be false if active item & result are different content type & neither references the other', () => {
+      const activeFile = createNote('test', {
+        uuid: 'active-file',
+        references: [],
+      })
+
+      const noteResult = createFile('test', {
+        uuid: 'note-result',
+        references: [],
+      })
+
+      Object.defineProperty(itemListController, 'activeControllerItem', { value: activeFile })
+
+      const isNoteResultAlreadyLinked = linkingController.isSearchResultAlreadyLinked(noteResult)
+      expect(isNoteResultAlreadyLinked).toBeFalsy()
+    })
+  })
+})

packages/web/src/javascripts/Controllers/LinkingController.tsx

@@ -342,72 +342,104 @@ export class LinkingController extends AbstractViewController {
     this.application.sync.sync().catch(console.error)
   }
 
-  getSearchResults = (searchQuery: string) => {
+  isValidSearchResult = (item: LinkableItem, searchQuery: string) => {
-    if (!searchQuery.length) {
+    const title = item instanceof SNTag ? this.application.items.getTagLongTitle(item) : item.title ?? ''
-      return {
+
-        linkedResults: [],
+    const matchesQuery = title.toLowerCase().includes(searchQuery.toLowerCase())
-        unlinkedResults: [],
+
-        shouldShowCreateTag: false,
+    const isActiveItem = this.activeItem?.uuid === item.uuid
-      }
+    const isArchivedOrTrashed = item.archived || item.trashed
+
+    const isValidSearchResult = matchesQuery && !isActiveItem && !isArchivedOrTrashed
+
+    return isValidSearchResult
+  }
+
+  isSearchResultAlreadyLinked = (item: LinkableItem) => {
+    if (!this.activeItem) {
+      return false
     }
 
-    const searchResults = naturalSort(
+    let isAlreadyLinked = false
-      this.application.items.getItems([ContentType.Note, ContentType.File, ContentType.Tag]).filter((item) => {
+
-        const title = item instanceof SNTag ? this.application.items.getTagLongTitle(item) : item.title
+    const isItemReferencedByActiveItem = this.activeItem.references.some((ref) => ref.uuid === item.uuid)
-        const matchesQuery = title?.toLowerCase().includes(searchQuery.toLowerCase())
+    const isActiveItemReferencedByItem = item.references.some((ref) => ref.uuid === this.activeItem?.uuid)
-        const isNotActiveItem = this.activeItem?.uuid !== item.uuid
+
-        const isArchivedOrTrashed = item.archived || item.trashed
+    if (this.activeItem.content_type === item.content_type) {
-        return matchesQuery && isNotActiveItem && !isArchivedOrTrashed
+      isAlreadyLinked = isItemReferencedByActiveItem
-      }),
+    } else {
+      isAlreadyLinked = isActiveItemReferencedByItem || isItemReferencedByActiveItem
+    }
+
+    return isAlreadyLinked
+  }
+
+  isSearchResultExistingTag = (result: DecryptedItemInterface<ItemContent>, searchQuery: string) =>
+    result.content_type === ContentType.Tag && result.title === searchQuery
+
+  getSearchResults = (searchQuery: string) => {
+    let unlinkedResults: LinkableItem[] = []
+    const linkedResults: ItemLink<LinkableItem>[] = []
+    let shouldShowCreateTag = false
+
+    const defaultReturnValue = {
+      linkedResults,
+      unlinkedResults,
+      shouldShowCreateTag,
+    }
+
+    if (!searchQuery.length) {
+      return defaultReturnValue
+    }
+
+    if (!this.activeItem) {
+      return defaultReturnValue
+    }
+
+    const searchableItems = naturalSort(
+      this.application.items.getItems([ContentType.Note, ContentType.File, ContentType.Tag]),
       'title',
     )
 
-    const isAlreadyLinked = (item: DecryptedItemInterface<ItemContent>) => {
+    const unlinkedTags: LinkableItem[] = []
-      if (!this.activeItem) {
+    const unlinkedNotes: LinkableItem[] = []
-        return false
+    const unlinkedFiles: LinkableItem[] = []
+
+    for (let index = 0; index < searchableItems.length; index++) {
+      const item = searchableItems[index]
+
+      if (!this.isValidSearchResult(item, searchQuery)) {
+        continue
       }
 
-      const isItemReferencedByActiveItem = this.application.items
+      if (this.isSearchResultAlreadyLinked(item)) {
-        .itemsReferencingItem(item)
+        if (linkedResults.length < 20) {
-        .some((linkedItem) => linkedItem.uuid === this.activeItem?.uuid)
+          linkedResults.push(this.createLinkFromItem(item, 'linked'))
-      const isActiveItemReferencedByItem = this.application.items
+        }
-        .itemsReferencingItem(this.activeItem)
+        continue
-        .some((linkedItem) => linkedItem.uuid === item.uuid)
-
-      if (this.activeItem.content_type === item.content_type) {
-        return isItemReferencedByActiveItem
       }
 
-      return isActiveItemReferencedByItem || isItemReferencedByActiveItem
+      if (unlinkedTags.length < 5 && item.content_type === ContentType.Tag) {
+        unlinkedTags.push(item)
+        continue
+      }
+
+      if (unlinkedNotes.length < 5 && item.content_type === ContentType.Note) {
+        unlinkedNotes.push(item)
+        continue
+      }
+
+      if (unlinkedFiles.length < 5 && item.content_type === ContentType.File) {
+        unlinkedFiles.push(item)
+        continue
+      }
     }
 
-    const prioritizeTagResult = (
+    unlinkedResults = unlinkedTags.concat(unlinkedNotes).concat(unlinkedFiles)
-      itemA: DecryptedItemInterface<ItemContent>,
-      itemB: DecryptedItemInterface<ItemContent>,
-    ) => {
-      if (itemA.content_type === ContentType.Tag && itemB.content_type !== ContentType.Tag) {
-        return -1
-      }
-      if (itemB.content_type === ContentType.Tag && itemA.content_type !== ContentType.Tag) {
-        return 1
-      }
-      return 0
-    }
 
-    const unlinkedResults = searchResults
+    shouldShowCreateTag =
-      .slice(0, 20)
+      !linkedResults.find((link) => this.isSearchResultExistingTag(link.item, searchQuery)) &&
-      .filter((item) => !isAlreadyLinked(item))
+      !unlinkedResults.find((item) => this.isSearchResultExistingTag(item, searchQuery))
-      .sort(prioritizeTagResult)
-    const linkedResults = searchResults
-      .filter(isAlreadyLinked)
-      .slice(0, 20)
-      .map((item) => this.createLinkFromItem(item, 'linked'))
-
-    const isResultExistingTag = (result: DecryptedItemInterface<ItemContent>) =>
-      result.content_type === ContentType.Tag && result.title === searchQuery
-
-    const shouldShowCreateTag =
-      !linkedResults.find((link) => isResultExistingTag(link.item)) && !unlinkedResults.find(isResultExistingTag)
 
     return {
       unlinkedResults,