Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: improve perf inside search #2663

Closed
wants to merge 9 commits into from
2 changes: 1 addition & 1 deletion .bundlemonrc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
},
{
"path": "services/qualificationMigration/drive.js",
"maxSize": "242 KB"
"maxSize": "244 KB"
},
{
"path": "vendors/drive.<hash>.<hash>.min.css",
Expand Down
11 changes: 9 additions & 2 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"no-console": 1,
"no-param-reassign": "error",
"react-hooks/exhaustive-deps": "error"

},
"globals": {
"fixture": false
Expand All @@ -13,5 +12,13 @@
"react": {
"version": "detect"
}
}
},
"overrides": [
{
"files": ["*.spec.js[x]"],
"rules": {
"react/display-name": "off"
}
}
]
}
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## ✨ Features

* Upgrade [email protected] to be able to findAll in FileCollection
* Upgrade [email protected] to be able to call onSelect function
* Improve speed of search suggestion by querying fewer data
* Update cozy-stack-client and cozy-pouch-link to sync with cozy-client version
* Update cozy-ui
- Modify Viewers to handle [68.0.0 BC](https://github.com/cozy/cozy-ui/releases/tag/v68.0.0)
Expand All @@ -12,7 +15,7 @@

* Improve cozy-bar implementation to fix UI bugs in Amirale
* Fix navigation through mobile Flagship on Note creation and opening
* Remove unused contacts permissions on Photos
* Remove unused contacts permissions on Photos

## 🔧 Tech

Expand Down
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,9 @@
"classnames": "2.3.1",
"copy-text-to-clipboard": "1.0.4",
"cozy-authentication": "2.8.3",
"cozy-bar": "7.19.1",
"cozy-bar": "7.20.0",
"cozy-ci": "0.4.1",
"cozy-client": "^32.2.8",
"cozy-client": "^32.7.0",
"cozy-client-js": "0.19.0",
"cozy-device-helper": "^2.1.0",
"cozy-doctypes": "1.82.2",
Expand All @@ -125,12 +125,12 @@
"cozy-intent": "^1.17.1",
"cozy-keys-lib": "4.1.9",
"cozy-logger": "1.9.0",
"cozy-pouch-link": "^32.2.8",
"cozy-pouch-link": "^32.7.0",
"cozy-realtime": "3.14.4",
"cozy-scanner": "2.0.2",
"cozy-scripts": "^6.3.8",
"cozy-sharing": "4.1.5",
"cozy-stack-client": "^32.2.5",
"cozy-stack-client": "^32.7.0",
"cozy-ui": "^68.4.0",
"date-fns": "1.30.1",
"diacritics": "1.3.0",
Expand Down
31 changes: 31 additions & 0 deletions src/drive/web/modules/queries.js
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,37 @@ export const buildEncryptionByIdQuery = id => ({
}
})

/**
* Provide selector and options to fetch files
*
* @returns {{options: {MangoQueryOptions}, selector: object}} A minimalist file list
*/
export const prepareSuggestionQuery = () => {
const selector = {
_id: {
$gt: null
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@trollepierre after the merge of cozy/cozy-client#1225 you should be able to remove this selector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const options = {
partialFilter: {
_id: {
$ne: TRASH_DIR_ID
},
path: {
// this predicate is necessary until the trashed attribute is more reliable
$or: [{ $exists: false }, { $regex: '^(?!/.cozy_trash)' }]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: as this predicate on the path shouldn't be necessary if the trash was more reliable, it would be worth addding a comment here to mention it

},
trashed: {
$or: [{ $exists: false }, { $eq: false }]
}
},
fields: ['_id', 'trashed', 'dir_id', 'name', 'path', 'type', 'mime'],
indexedFields: ['_id'],
limit: 1000
}
return { selector, options }
}

export {
buildDriveQuery,
buildRecentQuery,
Expand Down
86 changes: 51 additions & 35 deletions src/drive/web/modules/services/components/SuggestionProvider.jsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
/* global cozy */
import React from 'react'
import FuzzyPathSearch from '../FuzzyPathSearch'
import { withClient } from 'cozy-client'

import { TYPE_DIRECTORY, makeNormalizedFile } from './helpers'
import { getIconUrl } from './iconContext'
import { DOCTYPE_FILES } from 'drive/lib/doctypes'
import { prepareSuggestionQuery } from '../../queries'

class SuggestionProvider extends React.Component {
componentDidMount() {
const { intent } = this.props
this.hasIndexedFiles = false
this.hasIndexFilesBeenLaunched = false

// re-attach the message listener for the intent to receive the suggestion requests
window.addEventListener('message', event => {
Expand All @@ -22,8 +23,25 @@ class SuggestionProvider extends React.Component {
})
}

/**
* Provide Suggestions to calling Intent
*
* This method called when intent provide query will indexFiles once
* to fill FuzzyPathSearch. Then will re-post message to the intent
* with updated search results containing `files` as `suggestions`
* for SearchBar need.
*
* ⚠️ For note file, we don't provide url to open, but onSelect method
* to be called on click. Less API calls expected. But a note will be opened
* slower. See helpers.js
*
* @param query - Query to find file
* @param id
* @param intent - Intent calling
* @returns {Promise<void>} nothing
*/
async provideSuggestions(query, id, intent) {
if (!this.hasIndexedFiles) {
if (!this.hasIndexFilesBeenLaunched) {
await this.indexFiles()
}

Expand All @@ -38,52 +56,50 @@ class SuggestionProvider extends React.Component {
title: result.name,
subtitle: result.path,
term: result.name,
onSelect: 'open:' + result.url,
onSelect: result.onSelect || 'open:' + result.url,
icon: result.icon
}))
},
intent.attributes.client
)
}

// fetches pretty much all the files and preloads FuzzyPathSearch
/**
* Fetches all files without trashed and preloads FuzzyPathSearch
*
* Using _find route (from findAll) improves performance:
* - using partial index to reduce amount of data fetched
* - removing trashed data directly
*
* Also, this method:
* - set first the `hasIndexFilesBeenLaunched` to prevent multiple calls
* - removes orphan file
* - normalize file to match <SearchBar> expectation
* - preloads FuzzyPathSearch
*
* @returns {Promise<void>} nothing
*/
async indexFiles() {
const { client } = this.props
// TODO: fix me
// eslint-disable-next-line no-async-promise-executor
return new Promise(async resolve => {
const resp = await cozy.client.fetchJSON(
'GET',
`/data/io.cozy.files/_all_docs?include_docs=true`
)
const files = resp.rows
// TODO: fix me
// eslint-disable-next-line no-prototype-builtins
.filter(row => !row.doc.hasOwnProperty('views'))
.map(row => ({ id: row.id, ...row.doc }))
this.hasIndexFilesBeenLaunched = true

const folders = files.filter(file => file.type === TYPE_DIRECTORY)
const { selector, options } = prepareSuggestionQuery()
const files = await client
.collection(DOCTYPE_FILES)
.findAll(selector, options)

const notInTrash = file =>
!file.trashed && !/^\/\.cozy_trash/.test(file.path)
const notOrphans = file =>
folders.find(folder => folder._id === file.dir_id) !== undefined
const folders = files.filter(file => file.type === TYPE_DIRECTORY)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we can benefits from http2 multiplexing, I'm wondering if we should not make two requests:

  • one for the files
  • one for the folders

We'll have the almost the same number of round trips (1 or 2) but we'll have a few of them parallelized. @paultranvan what do you think?

Copy link
Contributor

@paultranvan paultranvan Aug 16, 2022

Choose a reason for hiding this comment

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

I assume that, most of the time, the number of files will be order of magnitudes higher than the number of folders. So, the folder query might end up sooner than the files one, which is good, as we want to display the folders first. The downside is that we have now 2 indexes to compute instead of one, which might slow down the search when we make a query with non-indexed docs.
I think what you propose is still relevant either way, as it should allow to get partial (i.e. folders) results faster and I think faster user response is more important than the overall performance.
That being said, I also think paginated mango queries is sub-optimal for this use-case :/ I look forward for view-base approach and/or pouchdb exploration


const normalizedFilesPrevious = files
.filter(notInTrash)
.filter(notOrphans)
const notOrphans = file =>
folders.find(folder => folder._id === file.dir_id) !== undefined

const normalizedFiles = await Promise.all(
normalizedFilesPrevious.map(
async file =>
await makeNormalizedFile(client, folders, file, getIconUrl)
)
)
const normalizedFilesPrevious = files.filter(notOrphans)

this.fuzzyPathSearch = new FuzzyPathSearch(normalizedFiles)
this.hasIndexedFiles = true
resolve()
})
const normalizedFiles = normalizedFilesPrevious.map(file =>
makeNormalizedFile(client, folders, file, getIconUrl)
)

this.fuzzyPathSearch = new FuzzyPathSearch(normalizedFiles)
}

render() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import React from 'react'
import { render, waitFor } from '@testing-library/react'
import SuggestionProvider from './SuggestionProvider'
import { dummyFile, dummyNote } from 'test/dummies/dummyFile'

const parentFolder = dummyFile({ _id: 'id-file' })
const folder = dummyFile({ dir_id: 'id-file' })
const note = dummyNote({
dir_id: 'id-file',
name: 'name.cozy-note'
})
const mockFindAll = jest.fn().mockReturnValue([parentFolder, folder, note])
const mockClient = {
collection: jest.fn().mockReturnValue({ findAll: mockFindAll })
}
const mockIntentAttributesClient = 'intent-attributes-client'

jest.mock('cozy-client', () => ({
...jest.requireActual('cozy-client'),
withClient: Component => () => {
const intent = {
_id: 'id_intent',
attributes: { client: mockIntentAttributesClient }
}
return <Component client={mockClient} intent={intent}></Component>
}
}))
jest.mock('./iconContext', () => ({ getIconUrl: () => 'iconUrl' }))

describe('SuggestionProvider', () => {
let events = {}
let event

beforeEach(() => {
window.addEventListener = jest.fn((event, callback) => {
events[event] = callback
})
window.parent.postMessage = jest.fn()
event = {
origin: mockIntentAttributesClient,
data: { query: 'name', id: 'id' }
}
})

it('should query all files to display fuzzy suggestion', () => {
// Given
render(<SuggestionProvider />)

// When
events.message(event)

// Then
expect(mockClient.collection).toHaveBeenCalledWith('io.cozy.files')
expect(mockFindAll).toHaveBeenCalledWith(
{
_id: {
$gt: null
}
},
{
fields: ['_id', 'trashed', 'dir_id', 'name', 'path', 'type', 'mime'],
indexedFields: ['_id'],
limit: 1000,
partialFilter: {
_id: { $ne: 'io.cozy.files.trash-dir' },
path: { $or: [{ $exists: false }, { $regex: '^(?!/.cozy_trash)' }] },
trashed: { $or: [{ $exists: false }, { $eq: false }] }
}
}
)
})

it('should provide onSelect with open url when file is not a note + and function when it is a note', async () => {
// Given
render(<SuggestionProvider />)

// When
events.message(event)

// Then
await waitFor(() => {
expect(window.parent.postMessage).toHaveBeenCalledWith(
{
id: 'id',
suggestions: [
{
icon: 'iconUrl',
id: 'id-file',
onSelect: 'open:http://localhost/#/folder/id-file',
subtitle: '/path',
term: 'name',
title: 'name'
},
{
icon: 'iconUrl',
id: 'id-file',
onSelect: expect.any(Function),
subtitle: '/path',
term: 'name.cozy-note',
title: 'name.cozy-note'
}
],
type: 'intent-id_intent:data'
},
'intent-attributes-client'
)
})
})
})
20 changes: 17 additions & 3 deletions src/drive/web/modules/services/components/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,33 @@ import { models } from 'cozy-client'

export const TYPE_DIRECTORY = 'directory'

export const makeNormalizedFile = async (client, folders, file, getIconUrl) => {
/**
* Normalize file for Front usage in <AutoSuggestion> component inside <SearchBar>
*
* To reduce API call, the fetching of Note URL has been delayed
* inside an onSelect function called only if provided to <SearchBar>
* see https://github.com/cozy/cozy-drive/pull/2663#discussion_r938671963
*
* @param client - cozy client instance
* @param folders - all the folders returned by API
* @param file - file to normalize
* @param getIconUrl - method to get icon url
* @returns {{path: (*|string), name, icon, id, url: string, onSelect: (function(): Promise<string>)}} file with
*/
export const makeNormalizedFile = (client, folders, file, getIconUrl) => {
const isDir = file.type === TYPE_DIRECTORY
const dirId = isDir ? file._id : file.dir_id
const urlToFolder = `${window.location.origin}/#/folder/${dirId}`

let path, url
let path, url, onSelect
if (isDir) {
path = file.path
url = urlToFolder
} else {
const parentDir = folders.find(folder => folder._id === file.dir_id)
path = parentDir && parentDir.path ? parentDir.path : ''
if (models.file.isNote(file)) {
url = await models.note.fetchURL(client, file)
onSelect = () => models.note.fetchURL(client, file)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acezard : here is the call of the function (unique, in cozy)

Copy link
Contributor

Choose a reason for hiding this comment

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

What do we do with the return of this fetchURL() method?

Because if we click on the suggestion, it'll call onSelect: cozy/cozy-bar@7b7491d#diff-bd7d5c5cdb7c2b6d78ffe2d7d7045e90beba76f929eff8695969c06a4d1accc7R237

Then onSelect make the call to fetchURL() but we do we do with the result? There is no window.location or else, how does it works to open the note?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

la définition de isNote me dérange un peu :

export const isNote = file => {
  if (
    file &&
    file.name &&
    file.name.endsWith('.cozy-note') &&
    file.type === FILE_TYPE &&
    file.metadata &&
    file.metadata.content !== undefined &&
    file.metadata.schema !== undefined &&
    file.metadata.title !== undefined &&
    file.metadata.version !== undefined
  )
    return true
  return false
}

mais comme je récupère moins de données, les notes ne sont pas considérées comme telles (par cette ligne du helper et aussi le getIconUrl plus bas).

Capture d’écran 2022-08-18 à 10 22 32

Il faut choisir :

  • ajouter les metadata
  • changer de condition isNote

Je vais partir sur l'ajout des metadata, en espérant qu'elles ne soient pas trop lourdes

Copy link
Contributor Author

@trollepierre trollepierre Aug 18, 2022

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

j'ai ajouté un nouveau commit pour fixer ceci.

Copy link
Contributor

@paultranvan paultranvan Aug 18, 2022

Choose a reason for hiding this comment

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

metadata peut être assez volumineux, ça contient les qualifications, les infos EXIF, etc : https://github.com/cozy/cozy-doctypes/blob/master/docs/io.cozy.files_metadata.md
C'est un peu dommage de devoir rapatrier tout le bloc juste pour savoir si c'est une note. Un file.mime === 'text/vnd.cozy.note+markdown' n'est pas suffisant @Crash-- ?

Et sur un autre sujet, ça m'embête de voir que :

  1. les metadata des notes (content, schema, title, version) sont à la racine de metadata. J'aurais trouvé mieux de grouper tout ça dans un objet metadata.notes, ça devient un sacré bazard les metadata.
  2. Je ne vois nul part documenté ces attributs : https://github.com/cozy/cozy-doctypes/blob/master/docs/io.cozy.files_metadata.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

et un deuxième, car le postMessage serialize la fonction onSelect, je suis donc obligé de réaliser le fetchUrl directement depuis la cozy bar

Copy link
Contributor

Choose a reason for hiding this comment

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

les metadata des notes (content, schema, title, version) sont à la racine de metadata. J'aurais trouvé mieux de grouper tout ça dans un objet metadata.notes, ça devient un sacré bazard les metadata.

Je ne sais même pas si metatada se révèle être le bon endroit pour mettre le contenu de la note au final. Il me semble que ce n'est pas la première fois qu'on nous remonte une incompréhension / souci avec. Mais il est vrai que d'avoir tout ce qui est propre à la note scopé quelque part serait plus simple d'utilisation...

Je ne vois nul part documenté ces attributs : https://github.com/cozy/cozy-doctypes/blob/master/docs/io.cozy.files_metadata.md
cozy/cozy-doctypes#208

C'est un peu dommage de devoir rapatrier tout le bloc juste pour savoir si c'est une note. Un file.mime === 'text/vnd.cozy.note+markdown' n'est pas suffisant @Crash-- ?

L'idée était d'être sur qu'on n'essaye pas d'ouvrir l'éditeur avec du contenu qu'il ne sait pas gérer. On peut pas juste sélectionner [metadata.content, metadata.schema...]?

} else {
url = `${urlToFolder}/file/${file._id}`
}
Expand All @@ -26,6 +39,7 @@ export const makeNormalizedFile = async (client, folders, file, getIconUrl) => {
name: file.name,
path,
url,
onSelect,
icon: getIconUrl(file)
}
}
Loading