-
Notifications
You must be signed in to change notification settings - Fork 65
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
Changes from 6 commits
3a27c1b
f1e5acf
1133fb9
8d4abc3
298411d
ba47d2e
4dcbc83
db97874
1cd1efc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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 | ||
|
||
|
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 => { | ||
|
@@ -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() | ||
} | ||
|
||
|
@@ -38,52 +56,48 @@ 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 options = prepareSuggestionQuery() | ||
const files = await client.collection(DOCTYPE_FILES).findAll(null, 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
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() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
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(null, { | ||
fields: [ | ||
'_id', | ||
'trashed', | ||
'dir_id', | ||
'name', | ||
'path', | ||
'type', | ||
'mime', | ||
'metadata' | ||
], | ||
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' | ||
) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Une chose est sûre, c'est qu'on peut pas garder
metadata
au complet. Soit on arrive à aller lire quelques champs particuliers, soit on change la façon de checker si c'est une note. Carmetadata
sur les notes, ça peut vite peser des centaines de ko par note.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
j'ai recréé un helper isNote, car les metadata peuvent être volumineuses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I like this idea.
You can add to the select(['metadata.title', 'metadata.version']) . title + version are not so big. So the helper can be if(metata.title && metadata.version && file.endsWith... && ...) .
See a test query with metadata.datetime to see the result.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only one note in my local Drive.
When adding:
as fields, the size of request increase
from 650ko / 39,87 ko transferred in 538ms
to 657,97 ko / 41,61ko transferred in 578ms.
That is quite huge, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what I suggested. I suggested to only add version + title. Not schema + content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect
PR updated