-
Notifications
You must be signed in to change notification settings - Fork 123
Console 1382 member pagination and search #7299
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
base: main
Are you sure you want to change the base?
Changes from all commits
5825c22
475e7fe
68af504
546b550
abbd0e4
e1a5546
64578a0
8e820f2
5a12547
a2e9037
dea8fc1
a2d73a4
69f6939
c28e2a5
cf07f1a
c7a399f
2628b1b
555e5f1
91975f6
7d69bc4
041045e
650eeef
f9c17da
7df7e4b
b6b8419
c6c914e
5e91797
e45c665
5efef97
e1f56a6
34a0dc9
3f201d6
e1d5da1
a2c0ae1
1e83941
7a6af7b
dd3166d
688f9b6
0148d24
8a9afb0
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 |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { type MigrationExecutor } from '../pg-migrator'; | ||
|
|
||
| export default { | ||
| name: '2025.11.25T00-00-00.members-search.ts', | ||
| run: ({ sql }) => sql` | ||
| -- The order was wrong. This was sorting by org_id, user_id, then created_at... | ||
| DROP INDEX IF EXISTS "organization_member_pagination_idx"; | ||
|
|
||
| -- Replace "organization_member_pagination_idx" with a new index in the correct order | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS "organization_member_pagination" | ||
| ON "organization_member" ( | ||
| "organization_id" DESC | ||
| , "created_at" DESC | ||
| , "user_id" DESC | ||
| ); | ||
| `, | ||
| } satisfies MigrationExecutor; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,21 +148,31 @@ export class OrganizationMembers { | |
|
|
||
| async getPaginatedOrganizationMembersForOrganization( | ||
| organization: Organization, | ||
| args: { first: number | null; after: string | null }, | ||
| args: { first: number | null; after: string | null; searchTerm: string | null }, | ||
| ) { | ||
| this.logger.debug( | ||
| 'Find paginated organization members for organization. (organizationId=%s)', | ||
| organization.id, | ||
| ); | ||
|
|
||
| const first = args.first; | ||
| const first = args.first ? Math.min(args.first, 50) : 50; | ||
| const cursor = args.after ? decodeCreatedAtAndUUIDIdBasedCursor(args.after) : null; | ||
| const searchTerm = args.searchTerm?.trim() ?? ''; | ||
| const searching = searchTerm.length > 0; | ||
|
|
||
| const query = sql` | ||
| SELECT | ||
| ${organizationMemberFields(sql`"om"`)} | ||
| FROM | ||
| "organization_member" AS "om" | ||
| ${ | ||
| searching | ||
| ? sql` | ||
| JOIN "users" as "u" | ||
| ON "om"."user_id" = "u"."id" | ||
| ` | ||
| : sql`` | ||
| } | ||
| WHERE | ||
| "om"."organization_id" = ${organization.id} | ||
| ${ | ||
|
|
@@ -178,11 +188,12 @@ export class OrganizationMembers { | |
| ` | ||
| : sql`` | ||
| } | ||
| ${searching ? sql`AND "u"."display_name" || ' ' || "u"."email" ILIKE ${'%' + searchTerm + '%'}` : sql``} | ||
| ORDER BY | ||
| "om"."organization_id" DESC | ||
| , "om"."created_at" DESC | ||
| , "om"."user_id" DESC | ||
|
Comment on lines
193
to
195
Contributor
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. How does this affect usage of existing indices?
Collaborator
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. We have an index already that handles this: But this is important for the pagination -- so that the last element in the list has a cursor with the oldest timestamp
Contributor
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. Now I get it, so in case of, we would need to update the index as well. 😢 Can be a follow-up PR, though!
Collaborator
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've modified this index. I was going back and forth on how to make this query as efficient as possible without introducing something like elastic search and I think this is now as good as it gets. The trigram index wasn't working out for a few reasons:
We could take the opposite route of first filtering a user by the display name / email, but it's much more efficient to filter the list how we have it for now... Maybe if a single organization had millions of users then it'd be different. When we do hit performance limits with our data, or if the returned ordering becomes a more critical feature, then we should shift to cloning this data to something like elasticsearch. |
||
| , "om"."user_id" DESC | ||
| ${first ? sql`LIMIT ${first + 1}` : sql``} | ||
| LIMIT ${first + 1} | ||
| `; | ||
|
|
||
| const result = await this.pool.any<unknown>(query); | ||
|
|
||
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.
filters
MembersFilter
Does singular make more sense here?
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.
It's named after
Query.memberslike how we name inputsThere 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.
So
MembersFilterandQuery.members(filter:)MembersFiltersandQuery.members(filters:)?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 see
XFilterandXFilterInputforinputname and bothfilter:andfiltersin the codebase.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.
For inputs we also kind of have the convention to append
Inputto the name.If in doubt on how to name things, I would always recommend to look for similar fields that are tagged with
@tag(name: "public"), as everything that is part of the public API schema, in theory, is consistent and follows how we want the GraphQL schema to be exposed.Having that said I am also guilty of my own feedback 😆

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.
Since this is part of the private GraphQL API for now, I am okay with handling it later though. Just make sure to create a follow up task