-
Notifications
You must be signed in to change notification settings - Fork 465
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
Add searchStarterPacks to appview #2978
base: main
Are you sure you want to change the base?
Conversation
ec2861c
to
d522841
Compare
|
||
async searchStarterPacks(req) { | ||
// @NOTE we don't store the term in the db, so we can't search by it. | ||
const { term: _term, limit, cursor } = req |
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.
Not sure what to do with the term
here, since we don't have the starter pack name stored in the appview. I think the search service will have it though, so that's why it exists.
I'm happy to change it to something better.
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.
We certainly could store the starter pack name in the db! How is it handled for post and actor search? Ideally we'd follow a similar pattern to those two if there is a pattern.
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 added a search by term with a gin index. I'm doing ilike
, not sure if it is a good usage of the index or how much we care about it.
45e0fc3
to
6d3d285
Compare
6d3d285
to
5ea8405
Compare
5ea8405
to
984c9cd
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.
Looking good! The only thing we might want to consider here is indexing the starter pack names so that some form of search is implemented in the dataplane.
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.
P.S. let's add a changeset for the @atproto/api package to this PR, so that a version of that package containing the new search lexicon can be published.
984c9cd
to
8a21be2
Compare
This adds:
app.bsky.graph.searchStarterPacks
and its implementation in thebsky
package.app.bsky.unspecced.searchStarterPacksSkeleton
, to be implemented by the search service.