-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: Implement search feature through Dataproxy #2198
Conversation
BundleMonFiles updated (3)
Unchanged files (10)
Total files change +28.55KB +0.88% Final result: ✅ View report in BundleMon website ➡️ |
5717054
to
6246a5c
Compare
7fdd29f
to
d31d676
Compare
d31d676
to
4f34db2
Compare
4f34db2
to
89ef9e2
Compare
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.
approve pour débloquer si besoin
package.json
Outdated
@@ -35,6 +35,7 @@ | |||
"homepage": "https://github.com/cozy/cozy-home#readme", | |||
"dependencies": { | |||
"@sentry/react": "7.119.0", | |||
"comlink": "^4.4.1", |
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.
on fixe la version des dep pour les dep non cozy
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.
package.json
Outdated
@@ -57,6 +57,7 @@ | |||
"leaflet": "1.7.1", | |||
"localforage": "^1.10.0", | |||
"lodash": "4.17.21", | |||
"mime-types": "^2.1.35", |
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.
idem version fixe
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.
return | ||
} | ||
|
||
setIframeUrl(result.data.attributes.services[0]?.href) |
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.
on peut enlever le ?
ici, dans le doute ça évitera de set un undefined 🤷♂️
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.
Si on l'enlève ça fera un throw et donc un crash de la home (j'en parlait l'autre jour il faudra qu'on mette un ErrorBoundary quelque part)
Mais le undefined est géré plus loin on ne rend l'iframe que si iFrameUrl est truthy
return result | ||
} | ||
|
||
const value = { |
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.
memoiser value permet des renders inutiles
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.
Je ne pense pas qu'il y en ai besoin, une fois l'iframe chargée le state ne devrait plus bouger (c'est juste un pont de communication, le state est géré à l'intérieur de l'iframe).
Je propose de merger quand même, mais je vérifierai ça lundi.
setDataProxy(() => remote) | ||
} | ||
|
||
const search = async search => { |
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.
usecallback les méthodes d'un provider évite également des renders
@@ -16,7 +16,7 @@ import ResultMenuItem from './ResultMenuItem' | |||
const SearchResult = () => { | |||
const { isLoading, results } = useSearch() | |||
|
|||
if (isLoading) | |||
if (isLoading && !results?.length) |
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.
c'est dû au fait que le result peut être []
même pendant un loading ? Est-ce qu'on pourrait plutôt avoir plus simplement un tableau, ou null, si on est !loading ou loading ? C'est basé sur client.query donc on hérite du comportement de cozy-client ?
@@ -31,6 +31,8 @@ const SearchResult = () => { | |||
icon={result.icon} | |||
primaryText={result.primary} | |||
secondaryText={result.secondary} | |||
query={searchValue} | |||
highlightQuery="true" |
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.
attention on devrait probablement avoir ={true}
ici non ?
@@ -31,6 +31,8 @@ const SearchResult = () => { | |||
icon={result.icon} | |||
primaryText={result.primary} | |||
secondaryText={result.secondary} | |||
query={searchValue} |
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'aurai gardé searchValue
en nom de prop 🤔 query
ça fait penser au queryDef de cozy-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.
On a eu exactement cette discussion avec @paultranvan y a quelques jours qui a opté pour query car c'est le terme généralement utilisé. Mais j'ai pas d'objection forte pour changer le nom.
@@ -0,0 +1,117 @@ | |||
/** | |||
* Code copied and adapted from cozy-drive |
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.
est-ce la manière la plus optimisé et moderne de le faire ? De quand date le code sur cozy-drive ? Est-ce qu'il n'y a pas des outils côté Mui pour faire ça plus simplement ?
Par ailleurs, comme on utilise la même chose à deux endroits, on devrait peut-être faire un compose cozy-ui
enfin, pour verrouiller l'approche, on pourrait peut-être rajouter des tests...
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.
Par ailleurs, comme on utilise la même chose à deux endroits, on devrait peut-être faire un compose cozy-ui
Je suis allé au plus simple car j'imagine qu'on va bientôt remplacer la recherche de drive par la celle de dataproxy.
Ok pour les tests, j'ajouterai ça dans les prochaines PR
@@ -47,7 +47,9 @@ const getApplicationsList = memoize(data => { | |||
!flag(`home_hidden_apps.${app.slug.toLowerCase()}`) // can be set in the context with `home_hidden_apps: - drive - banks`for example | |||
) | |||
const dedupapps = uniqBy(apps, 'slug') | |||
const array = dedupapps.map(app => <AppTile key={app.id} app={app} />) | |||
const array = dedupapps |
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.
suite à la description "This filter should be temporary as we expect to hide the cozy-app at a
higher level on cozy-client collections" on devrait peut-être mettre aussi un commentaire dans le code
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.
Comlink will be needed to ease communication between the app and the dataproxy iframe
Mime-types will be needed to display correct thumbnail on ResultMenu items based on the searched media
We want to embed the new `cozy-data-proxy` cozy-app as an intent into cozy-home `cozy-data-proxy` is a new cozy-app that is designed to persist the Cozy's data into a single place and to make it available to other cozy-apps For now this cozy-app is meant to search local Cozy's data and serve search results through Comlink communication Related PR: cozy/cozy-web-data-proxy#1
Now that we implemented the DataProxy provider, we want to remove the stubbed search method and use the DataProxy instead for searching into the Cozy's data
We want ResultMenu items to have their icon reflecting the returned data type This implementation uses some code from cozy-drive's search
Without this, the search results would update incorectly when the user would update the search query and when the resulting ResultMenuItems change For example, a Contact result can display a PDF icon if the previous search did return a PDF file at the same result position
The DataProxy provider already check for the corresponding flag and also take into account if the `cozy-data-proxy` is not installed, so we want to use this info to display the SearchResult component instead of just the flag
With previous implementation, the search would not update when the user changes the search query
When displaying search results, each item can contain a primary text and a secondary text. Sometimes the secondary text is a file path, sometimes some app description or anything else based on the returned doctype Due to this, it is sometimes difficult to see which line did match the query So we want to highlight the searched query in the primary and/or the secondary text This implementation uses some code from cozy-drive's search
For some doctypes, the search results may not contain any secondary text This would crash the app as the SuggestionItemTextHighlighted component do not expect to receive a null value So we want to add a protection against this scenario In the future we may want to introduce some Error Boundaries around the search component
We want to hide the `cozy-data-proxy` from the user as this is a technical cozy-app that is not meant to be opened This filter should be temporary as we expect to hide the cozy-app at a higher level on cozy-client collections
89ef9e2
to
0456cb9
Compare
J'ai fais les modifs les plus simples et les importantes, je traiterai le reste dans une autre PR, je merge déjà celle-ci |
This PR implements the search feature based on the new DataProxy
The DataProxy is a new cozy-app meant to be embedded by other cozy-apps that need to access locally persisted data and heavy processes (like search indexing, maybe local AI in the future etc)
The goal is to mutualize the persisted data that will be accessed by multiple cozy-app that have all their own domain (as we use flat domain with slug for every cozy-app) and so cannot mutualize their local storage without additional mechanisms (this is why we want to implement the DataProxy)
Here cozy-home access the DataProxy through an iframe based API (using the cozy-stack intent mechanism)
The DataProxy search API takes the user's search query and return a list of documents that match the query. For now those documents can be files, contacts or apps.
Related PR: cozy/cozy-web-data-proxy#1