Skip to content
Draft
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions packages/services/api/src/modules/organization/module.graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,14 @@ export default gql`
appDeployments: [String!]
}

input MembersFilter {
"""
Part of a user's email or username that is used to filter the list of
members.
"""
searchTerm: String
}

type Organization {
"""
Unique UUID of the organization
Expand All @@ -881,8 +889,11 @@ export default gql`
name: String! @deprecated(reason: "Use the 'slug' field instead.")
owner: Member! @tag(name: "public")
me: Member!
members(first: Int @tag(name: "public"), after: String @tag(name: "public")): MemberConnection!
@tag(name: "public")
members(
first: Int @tag(name: "public")
after: String @tag(name: "public")
filters: MembersFilter
): MemberConnection! @tag(name: "public")
invitations(
first: Int @tag(name: "public")
after: String @tag(name: "public")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1221,7 +1221,7 @@ export class OrganizationManager {

async getPaginatedOrganizationMembersForOrganization(
organization: Organization,
args: { first: number | null; after: string | null },
args: { first: number | null; after: string | null; searchTerm?: string | null },
) {
await this.session.assertPerformAction({
action: 'member:describe',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 ?? '';
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}
${
Expand All @@ -178,11 +188,12 @@ export class OrganizationMembers {
`
: sql``
}
${searching ? sql`AND to_tsvector("u"."display_name" || ' ' || "u"."email") @@ to_tsquery('simple', ${searchTerm + ':*'})` : sql``}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The current full-text search implementation has a few issues:

  1. Bug with spaces: to_tsquery will fail with a syntax error if searchTerm contains spaces (e.g., "John Doe").
  2. Potential for errors: If searchTerm contains tsquery operators like &, |, !, (, ), it can lead to syntax errors.
  3. Inconsistent configuration: The to_tsvector function is called without specifying a configuration, so it uses the database's default (e.g., 'english'), while to_tsquery explicitly uses 'simple'. This can lead to search misses.

I suggest using plainto_tsquery, which is safer as it doesn't interpret operators and handles multiple words by AND-ing them. This also requires specifying the 'simple' configuration in to_tsvector for consistency. This change fixes the bug at the cost of removing prefix matching, which is a reasonable trade-off for correctness and stability.

For performance, you should also add a GIN index on the expression used in the WHERE clause:

CREATE INDEX on users using gin(to_tsvector('simple', display_name || ' ' || email));
Suggested change
${searching ? sql`AND to_tsvector("u"."display_name" || ' ' || "u"."email") @@ to_tsquery('simple', ${searchTerm + ':*'})` : sql``}
${searching ? sql`AND to_tsvector('simple', "u"."display_name" || ' ' || "u"."email") @@ plainto_tsquery('simple', ${searchTerm})` : sql``}

ORDER BY
"om"."organization_id" DESC
, "om"."created_at" DESC
, "om"."user_id" DESC
, "om"."user_id" DESC
${first ? sql`LIMIT ${first + 1}` : sql``}
LIMIT ${first + 1}
`;

const result = await this.pool.any<unknown>(query);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export const Organization: Pick<
.getPaginatedOrganizationMembersForOrganization(organization, {
first: args.first ?? null,
after: args.after ?? null,
searchTerm: args.filters?.searchTerm,
});
},
invitations: async (organization, args, { injector }) => {
Expand Down
102 changes: 32 additions & 70 deletions packages/web/app/src/components/organization/members/list.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useCallback, useMemo, useState } from 'react';
import { MoreHorizontalIcon, MoveDownIcon, MoveUpIcon } from 'lucide-react';
import { ChangeEvent, useCallback, useState } from 'react';
import { MoreHorizontalIcon } from 'lucide-react';
import type { IconType } from 'react-icons';
import { FaGithub, FaGoogle, FaOpenid, FaUserLock } from 'react-icons/fa';
import { useMutation } from 'urql';
Expand All @@ -20,13 +20,15 @@ import {
DropdownMenuItem,
DropdownMenuTrigger,
} from '@/components/ui/dropdown-menu';
import { Input } from '@/components/ui/input';
import { Link } from '@/components/ui/link';
import { SubPageLayout, SubPageLayoutHeader } from '@/components/ui/page-content-layout';
import * as Sheet from '@/components/ui/sheet';
import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from '@/components/ui/tooltip';
import { useToast } from '@/components/ui/use-toast';
import { FragmentType, graphql, useFragment } from '@/gql';
import * as GraphQLSchema from '@/gql/graphql';
import { useRouter } from '@tanstack/react-router';
import { MemberInvitationButton } from './invitations';
import { MemberRolePicker } from './member-role-picker';

Expand Down Expand Up @@ -94,7 +96,6 @@ const OrganizationMemberRow_MemberFragment = graphql(`
function OrganizationMemberRow(props: {
organization: FragmentType<typeof OrganizationMembers_OrganizationFragment>;
member: FragmentType<typeof OrganizationMemberRow_MemberFragment>;
refetchMembers(): void;
}) {
const organization = useFragment(OrganizationMembers_OrganizationFragment, props.organization);
const member = useFragment(OrganizationMemberRow_MemberFragment, props.member);
Expand Down Expand Up @@ -286,7 +287,7 @@ const OrganizationMembers_OrganizationFragment = graphql(`
owner {
id
}
members {
members(filters: { searchTerm: $searchTerm }) {
edges {
node {
id
Expand All @@ -313,53 +314,41 @@ export function OrganizationMembers(props: {
}) {
const organization = useFragment(OrganizationMembers_OrganizationFragment, props.organization);
const members = organization.members?.edges?.map(edge => edge.node);
const [orderDirection, setOrderDirection] = useState<'asc' | 'desc' | null>(null);
const [sortByKey, setSortByKey] = useState<'name' | 'role'>('name');

const sortedMembers = useMemo(() => {
if (!members) {
return [];
}

if (!orderDirection) {
return members ?? [];
}

const sorted = [...members].sort((a, b) => {
if (sortByKey === 'name') {
return a.user.displayName.localeCompare(b.user.displayName);
}

if (sortByKey === 'role') {
return (a.role?.name ?? 'Select role').localeCompare(b.role?.name ?? 'Select role') ?? 0;
}
const router = useRouter();

return 0;
});

return orderDirection === 'asc' ? sorted : sorted.reverse();
}, [members, orderDirection, sortByKey]);

const updateSorting = useCallback(
(newSortBy: 'name' | 'role') => {
if (newSortBy === sortByKey) {
setOrderDirection(
orderDirection === 'asc' ? 'desc' : orderDirection === 'desc' ? null : 'asc',
);
} else {
setSortByKey(newSortBy);
setOrderDirection('asc');
}
const onChange = useCallback(
(e: ChangeEvent<HTMLInputElement>) => {
void router.navigate({
search: {
...router.latestLocation.search,
search: e.target.value === '' ? undefined : e.target.value,
},
// don't write to history
replace: true,
});
},
[sortByKey, orderDirection],
[router],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve performance and avoid sending a request on every keystroke, the search input should be debounced. This prevents excessive network requests while the user is typing. You can achieve this by using a setTimeout within the onChange handler. You'll also need to import useRef from React.

  const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);

  const onChange = useCallback(
    (e: ChangeEvent<HTMLInputElement>) => {
      if (timeoutRef.current) {
        clearTimeout(timeoutRef.current);
      }
      const value = e.target.value;
      timeoutRef.current = setTimeout(() => {
        void router.navigate({
          search: {
            ...router.latestLocation.search,
            search: value === '' ? undefined : value,
          },
          // don't write to history
          replace: true,
        });
      }, 300); // 300ms debounce
    },
    [router],
  );


const initialValue =
'search' in router.latestLocation.search &&
typeof router.latestLocation.search.search === 'string'
? router.latestLocation.search.search
: '';

return (
<SubPageLayout>
<SubPageLayoutHeader
subPageTitle="List of organization members"
description="Manage the members of your organization and their permissions."
>
<Input
className="w-[200px] grow cursor-text"
placeholder="Filter by field name"
onChange={onChange}
defaultValue={initialValue}
/>
{organization.viewerCanManageInvitations && (
<MemberInvitationButton
refetchInvitations={props.refetchMembers}
Expand All @@ -373,45 +362,18 @@ export function OrganizationMembers(props: {
<th
colSpan={2}
className="relative cursor-pointer select-none py-3 text-left text-sm font-semibold"
onClick={() => updateSorting('name')}
>
Member
<span className="inline-block">
{sortByKey === 'name' ? (
orderDirection === 'asc' ? (
<MoveUpIcon className="relative top-[3px] size-4" />
) : orderDirection === 'desc' ? (
<MoveDownIcon className="relative top-[3px] size-4" />
) : null
) : null}
</span>
</th>
<th
className="relative w-[300px] cursor-pointer select-none py-3 text-center align-middle text-sm font-semibold"
onClick={() => updateSorting('role')}
>
<th className="relative w-[300px] cursor-pointer select-none py-3 text-center align-middle text-sm font-semibold">
Assigned Role
<span className="inline-block">
{sortByKey === 'role' ? (
orderDirection === 'asc' ? (
<MoveUpIcon className="relative top-[3px] size-4" />
) : orderDirection === 'desc' ? (
<MoveDownIcon className="relative top-[3px] size-4" />
) : null
) : null}
</span>
</th>
<th className="w-12 py-3 text-right text-sm font-semibold" />
</tr>
</thead>
<tbody className="divide-y-[1px] divide-gray-500/20">
{sortedMembers.map(node => (
<OrganizationMemberRow
key={node.id}
refetchMembers={props.refetchMembers}
organization={props.organization}
member={node}
/>
{members.map(node => (
<OrganizationMemberRow key={node.id} organization={props.organization} member={node} />
))}
</tbody>
</table>
Expand Down
6 changes: 5 additions & 1 deletion packages/web/app/src/pages/organization-members.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { QueryError } from '@/components/ui/query-error';
import { FragmentType, graphql, useFragment } from '@/gql';
import { useRedirect } from '@/lib/access/common';
import { cn } from '@/lib/utils';
import { organizationMembersRoute } from '../router';

const OrganizationMembersPage_OrganizationFragment = graphql(`
fragment OrganizationMembersPage_OrganizationFragment on Organization {
Expand Down Expand Up @@ -106,7 +107,7 @@ function PageContent(props: {
}

const OrganizationMembersPageQuery = graphql(`
query OrganizationMembersPageQuery($organizationSlug: String!) {
query OrganizationMembersPageQuery($organizationSlug: String!, $searchTerm: String) {
organization: organizationBySlug(organizationSlug: $organizationSlug) {
...OrganizationMembersPage_OrganizationFragment
viewerCanSeeMembers
Expand All @@ -119,10 +120,13 @@ function OrganizationMembersPageContent(props: {
page: SubPage;
onPageChange(page: SubPage): void;
}) {
const search = organizationMembersRoute.useSearch();

const [query, refetch] = useQuery({
query: OrganizationMembersPageQuery,
variables: {
organizationSlug: props.organizationSlug,
searchTerm: search.search || undefined,
},
});

Expand Down
5 changes: 3 additions & 2 deletions packages/web/app/src/router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -431,9 +431,10 @@ const organizationSettingsRoute = createRoute({

const OrganizationMembersRouteSearch = z.object({
page: z.enum(['list', 'roles', 'invitations']).catch('list').default('list'),
search: z.string().optional(),
});

const organizationMembersRoute = createRoute({
export const organizationMembersRoute = createRoute({
getParentRoute: () => organizationRoute,
path: 'view/members',
validateSearch(search) {
Expand All @@ -445,7 +446,7 @@ const organizationMembersRoute = createRoute({
const navigate = useNavigate({ from: organizationMembersRoute.fullPath });
const onPageChange = useCallback(
(newPage: z.infer<typeof OrganizationMembersRouteSearch>['page']) => {
void navigate({ search: { page: newPage } });
void navigate({ search: { page: newPage, search: '' } });
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For consistency, it's better to use undefined to clear the search parameter instead of an empty string. This ensures the search parameter is completely removed from the URL, which is how it's handled in the search input component. This improves code consistency and maintainability.

Suggested change
void navigate({ search: { page: newPage, search: '' } });
void navigate({ search: { page: newPage, search: undefined } });

},
[navigate],
);
Expand Down
Loading