Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Pull request overview
This PR implements a new filtering API endpoint that allows querying the Volunteers table with AND/OR operations on specific columns. The implementation includes both the core filtering logic and an HTTP route handler to expose this functionality.
- Adds a
filter_generalfunction with column validation and support for AND/OR operations - Creates a GET endpoint at
/api/filter-generalto expose the filtering functionality - Implements validation for parameters and returns appropriate error responses
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/lib/api/filter_general.ts | Core filtering logic with column whitelisting, AND/OR operation support, and Supabase query execution |
| src/app/api/filter-general/route.ts | HTTP route handler that validates query parameters and calls the filter_general function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/lib/api/filter_general.ts
Outdated
|
|
||
| const client = await createClient(); | ||
|
|
||
| if (values.length == 0) { |
There was a problem hiding this comment.
Use strict equality operator (===) instead of loose equality (==) to avoid type coercion issues.
| if (values.length == 0) { | |
| if (values.length === 0) { |
src/lib/api/filter_general.ts
Outdated
| return { data: null, error: "No values provided." }; | ||
| } | ||
|
|
||
| if (op == "AND") { |
There was a problem hiding this comment.
Use strict equality operator (===) instead of loose equality (==) to avoid type coercion issues.
src/lib/api/filter_general.ts
Outdated
| const result = await client.from("Volunteers").select().eq(column, value); | ||
| return result; | ||
| } | ||
| if (op == "OR") { |
There was a problem hiding this comment.
Use strict equality operator (===) instead of loose equality (==) to avoid type coercion issues.
| if (op == "OR") { | |
| if (op === "OR") { |
src/lib/api/filter_general.ts
Outdated
| if (values.length == 0) { | ||
| return { data: null, error: "No values provided." }; | ||
| } |
There was a problem hiding this comment.
The values.length check at line 24 is redundant since the route handler already validates this condition at line 10. This duplicate validation should be removed or the route handler's check should be removed to avoid redundancy.
| if (values.length == 0) { | |
| return { data: null, error: "No values provided." }; | |
| } |
src/lib/api/filter_general.ts
Outdated
| export async function filter_general( | ||
| op: string, | ||
| column: string, | ||
| values: string[] | ||
| ) { |
There was a problem hiding this comment.
Missing JSDoc documentation for this public API function. Should include descriptions of parameters (op, column, values), the return type, and examples of valid operations. See getExample.ts comments for reference style.
src/lib/api/filter_general.ts
Outdated
| import { createClient } from "../client/supabase/server"; | ||
|
|
||
| export async function filter_general( | ||
| op: string, |
There was a problem hiding this comment.
The 'op' parameter allows any string value but only 'AND' and 'OR' are valid. Consider using a union type (op: 'AND' | 'OR') to enforce valid values at compile time and improve type safety.
| op: string, | |
| op: 'AND' | 'OR', |
src/app/api/filter-general/route.ts
Outdated
| const column = searchParams.get("column"); | ||
| const values = searchParams.getAll("values"); | ||
|
|
||
| if (!op || !column || values.length == 0) { |
There was a problem hiding this comment.
Use strict equality operator (===) instead of loose equality (==) to avoid type coercion issues.
| if (!op || !column || values.length == 0) { | |
| if (!op || !column || values.length === 0) { |
ch-iv
left a comment
There was a problem hiding this comment.
We should generally aim to have written tests for every PR. Could you write some tests for the changes you've made?
src/app/api/filter-general/route.ts
Outdated
There was a problem hiding this comment.
I think the name of the directory filter-general is too generic. We want to convey more information about what this route does by giving it a more descriptive name.
src/app/api/filter-general/route.ts
Outdated
There was a problem hiding this comment.
We don't need to implement the route.
src/lib/api/filter_general.ts
Outdated
| @@ -0,0 +1,45 @@ | |||
| import { createClient } from "../client/supabase/server"; | |||
|
|
|||
| export async function filter_general( | |||
There was a problem hiding this comment.
Similar comment to route naming. This function should be given a more descriptive name. The current name doesn't communicate what this method does.
src/lib/api/filter_general.ts
Outdated
| op: string, | ||
| column: string, | ||
| values: string[] | ||
| ) { |
There was a problem hiding this comment.
The return value of this function should be typed.
src/lib/api/filter_general.ts
Outdated
| import { createClient } from "../client/supabase/server"; | ||
|
|
||
| export async function filter_general( | ||
| op: string, |
There was a problem hiding this comment.
(nit) Not a big fan of the name op. Something like matchType is more descriptive.
src/lib/api/filter_general.ts
Outdated
| import { createClient } from "../client/supabase/server"; | ||
|
|
||
| export async function filter_general( | ||
| op: string, |
There was a problem hiding this comment.
We should rely on the TypeScript type system to enforce the validity of the value of op (as well as for other arguments.)
I suggest you created either an Enum or a literal value type for op.
type MatchType = "any" | "all";then you can type the function argument as follows:
function filter_general(op: MatchType, ...)and the ts compiler will let you know if the function contract is being violated when you pass in the wrong values for op.
src/lib/api/filter_general.ts
Outdated
|
|
||
| export async function filter_general( | ||
| op: string, | ||
| column: string, |
There was a problem hiding this comment.
Same comment as for OP. This should be a type or an enum.
src/lib/api/filter_general.ts
Outdated
| const valid_columns = [ | ||
| "name_org", | ||
| "pseudonym", | ||
| "pronouns", | ||
| "email", | ||
| "phone", | ||
| "position", | ||
| "opt_in_communication", | ||
| ]; |
There was a problem hiding this comment.
We should get rid of this in favor of introducing a type or an enum for valid columns in a centralized helper in this codebase.
| "She/her", | ||
| ]); | ||
| expect(result).toEqual({ data: mockData, error: null }); | ||
| }); |
There was a problem hiding this comment.
Great work so far, one final test you should have is when the input operator is invalid (not AND/OR)
| @@ -0,0 +1,64 @@ | |||
| import { describe, it, expect, beforeEach, afterEach } from "vitest"; | |||
There was a problem hiding this comment.
You should combine unit tests and integration tests into one file, within tests/lib/api!
|
|
||
| const volunteer = result.data[0]!; | ||
| expect(volunteer.email).toBe("a@test.com"); | ||
| }); |
There was a problem hiding this comment.
You should add a test for AND operation on unique values. For example, email is unique, test what if you do AND operation on 2 emails
|
|
||
| type VolunteerRow = Database["public"]["Tables"]["Volunteers"]["Row"]; | ||
|
|
||
| export async function filter_by_general_info( |
There was a problem hiding this comment.
The function name should match the file name: getVolunteerByGeneralInfo!
66b64db to
902fc39
Compare
| type MockSupabaseClient = { | ||
| from: ReturnType<typeof vi.fn>; | ||
| select: ReturnType<typeof vi.fn>; | ||
| eq: ReturnType<typeof vi.fn>; | ||
| in: ReturnType<typeof vi.fn>; | ||
| }; |
Resolved comments from PR completed unit tests fixing vercel issue fixing vercel issue1 fixing vercel issue3 fixing vercel issue3 fixing vercel issue2 fixing vercel issue resolving integration test fixed the test resolved comments Implement filter-general function changed to real database cleaned up mock tests
708d7f4 to
3dd964e
Compare
implement this function that filters volunteers by general information (e.g. email, phone, position,etc.)
Basic testing
test case 1: AND with one unique value (op = "AND", column = "email", values = "v1@mail.com")
test case 2: AND with two values. (op = "AND", column = "pronouns", values = "He/him", "She/her")
Should return empty rows since this is filtering one column only.

test case 3: OR with two values ((op = "AND", column = "pronouns", values = "He/him", "She/her")

test case 4: Invalid op ((op = "hello", column = "email", values = "v1@mail.com")
