Skip to content
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

Implement fuse #91

Merged
merged 17 commits into from
Nov 23, 2024
Merged
Show file tree
Hide file tree
Changes from 14 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
1 change: 1 addition & 0 deletions apps/web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"@trpc/react-query": "11.0.0-rc.441",
"@trpc/server": "11.0.0-rc.441",
"dayjs": "^1.11.13",
"fuse.js": "^7.0.0",
"geist": "^1.3.0",
"lucide-react": "^0.436.0",
"next": "^14.2.4",
Expand Down
12 changes: 11 additions & 1 deletion apps/web/src/app/(pages)/(dashboard)/roles/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import { useEffect, useState } from "react";
import { z } from "zod";

import {

Check warning on line 6 in apps/web/src/app/(pages)/(dashboard)/roles/page.tsx

View workflow job for this annotation

GitHub Actions / lint

Imports "ReviewType", "WorkEnvironmentType" and "WorkTermType" are only used as type
ReviewType,
WorkEnvironment,
WorkEnvironmentType,
Expand All @@ -22,6 +22,9 @@
searchParams,
}: {
searchParams?: {
workTerm?: WorkTermType;
workEnvironment?: WorkEnvironmentType;
search?: string;
cycle?: WorkTermType;
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like we want to get rid of cycle and term now since you renamed them in the new search params.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think we got rid of workTerm and workEnvironment in favor of cycle and term but I might be wrong. This might an artifact of a merge. @suxls would recommend removing workTerm and workEnvironment from the searchParams object and testing.

term?: WorkEnvironmentType;
};
Expand Down Expand Up @@ -60,19 +63,26 @@
variant: "destructive",
});
}
}, [toast, mounted]);

Check warning on line 66 in apps/web/src/app/(pages)/(dashboard)/roles/page.tsx

View workflow job for this annotation

GitHub Actions / lint

React Hook useEffect has missing dependencies: 'validationResult.error.issues' and 'validationResult.success'. Either include them or remove the dependency array
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll have to update the dependency array to include the validationResult based on that warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


const reviews = api.review.list.useQuery({
search: searchParams?.search,
options: validationResult.success ? validationResult.data : {},
});

const [selectedReview, setSelectedReview] = useState<ReviewType | undefined>(
reviews.data ? reviews.data[0] : undefined,
);

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

in general we want to avoid this kind of useEffect, but I'm assuming there was some kind of bug due to the fact that reviews are updated based on the child SearchFilter component?

if (reviews.data) {
setSelectedReview(reviews.data[0]);
}
}, [reviews.data]);

return (
<>
<SearchFilter />
<SearchFilter search={searchParams?.search} />
{reviews.data && (
<div className="mb-8 grid h-[70dvh] w-4/5 grid-cols-5 gap-4 lg:w-3/4">
<div className="col-span-2 gap-3 overflow-scroll pr-4">
Expand Down
8 changes: 6 additions & 2 deletions apps/web/src/app/_components/search/search-filter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,15 @@ const formSchema = z.object({

export type SearchFilterFormType = typeof formSchema;

export default function SearchFilter() {
interface SearchFilterProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

i know this is so my inner fundies 1 ta but what are our thoughts on starting to us js doc to lightly document the functions. Espesh since dev turnover may be on the rise (oml we are getting old). Up to rishi hes TL i'm just throwing this out there ❤️ 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree with this to make it easier for everyone to read

Copy link
Collaborator

Choose a reason for hiding this comment

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

YES pls document with JSDoc 🙏🏻 I remember we were doing it for a bit at the start of Spring but that fizzled out lol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yessir

search?: string;
}

export default function SearchFilter({ search }: SearchFilterProps) {
const form = useForm<z.infer<typeof formSchema>>({
resolver: zodResolver(formSchema),
defaultValues: {
searchText: "",
searchText: search ?? "",
},
});

Expand Down
18 changes: 16 additions & 2 deletions packages/api/src/router/review.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { TRPCRouterRecord } from "@trpc/server";
import Fuse from "fuse.js";
import { z } from "zod";

import { and, desc, eq } from "@cooper/db";
Expand All @@ -10,6 +11,7 @@ export const reviewRouter = {
list: publicProcedure
.input(
z.object({
search: z.string().optional(),
options: z
.object({
cycle: z.enum(["SPRING", "FALL", "SUMMER"]).optional(),
Expand All @@ -18,18 +20,30 @@ export const reviewRouter = {
.optional(),
}),
)
.query(({ ctx, input }) => {
.query(async ({ ctx, input }) => {
const { options } = input;

const conditions = [
options?.cycle && eq(Review.workTerm, options.cycle),
options?.term && eq(Review.workEnvironment, options.term),
].filter(Boolean);

return ctx.db.query.Review.findMany({
const reviews = await ctx.db.query.Review.findMany({
orderBy: desc(Review.id),
where: conditions.length > 0 ? and(...conditions) : undefined,
});

if (!input.search) {
return reviews;
}

const fuseOptions = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe abstract the fuzzy searching out to a helper? I might implement a custom linting rule that makes sure that every function should be 4 or less lines of code 😡 /s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will implement that on a separate branch

keys: ["reviewHeadline", "textReview", "location"],
};

const fuse = new Fuse(reviews, fuseOptions);

return fuse.search(input.search).map((result) => result.item);
}),

getByRole: publicProcedure
Expand Down
13 changes: 7 additions & 6 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.