Eng 337 search route #177
Eng 337 search route #177
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces new API route handlers for generating and searching embeddings, implements generic embedding utilities with OpenAI support, and adds robust, type-safe Supabase database utilities for entity and batch operations. The changes include comprehensive validation, error handling, and CORS support across all new endpoints and utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EmbeddingsAPI
participant Utils
participant OpenAI
Client->>EmbeddingsAPI: POST /api/embeddings (input, model, ...)
EmbeddingsAPI->>Utils: generic_embedding(input, model, provider, dimensions)
Utils->>OpenAI: Request embedding
OpenAI-->>Utils: Embedding vector(s)
Utils-->>EmbeddingsAPI: Embedding vector(s)
EmbeddingsAPI-->>Client: 200 OK (embeddings)
sequenceDiagram
participant Client
participant SearchAPI
participant Utils
participant Supabase
Client->>SearchAPI: POST /api/supabase/rpc/search-content (queryText/queryEmbedding, ...)
alt queryEmbedding missing
SearchAPI->>Utils: generic_embedding(queryText, ...)
Utils->>OpenAI: Request embedding
OpenAI-->>Utils: Embedding vector
Utils-->>SearchAPI: Embedding vector
end
SearchAPI->>Supabase: Call RPC (match_content_embeddings or match_embeddings_for_subset_nodes)
Supabase-->>SearchAPI: Matching results
SearchAPI-->>Client: 200 OK (results)
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (18)
apps/website/app/api/supabase/insert/person/route.ts (1)
102-102: Consider enabling OPTIONS handler for CORS support.The commented OPTIONS handler suggests this endpoint may need CORS support. If this API will be called from browser clients, consider uncommenting it.
- // If you need an OPTIONS handler for this route: - // export const OPTIONS = defaultOptionsHandler; + export const OPTIONS = defaultOptionsHandler;apps/website/app/api/embeddings/route.ts (3)
2-2: Remove unused import.The
OpenAIimport is not used anywhere in this file since the embedding generation is handled by thegeneric_embeddingutility function.-import OpenAI from "openai";
6-12: Remove unused parameter from RequestBody type.The
encoding_formatparameter is destructured in the request body but never used in the embedding generation.type RequestBody = { input: string | string[]; model?: string; dimensions?: number; provider?: string; - encoding_format?: "float" | "base64"; };
45-58: Improve error handling around embedding generation.The current code checks if
embeddings === undefinedafter callinggeneric_embedding, but this approach doesn't handle potential exceptions that might be thrown by the function. Consider wrapping the embedding call in a try-catch block for more robust error handling.- const embeddings = await generic_embedding( - input, - model, - provider, - dimensions, - ); - if (embeddings === undefined) - response = NextResponse.json( - { - error: "Failed to generate embeddings.", - }, - { status: 500 }, - ); - else response = NextResponse.json(embeddings, { status: 200 }); + try { + const embeddings = await generic_embedding( + input, + model, + provider, + dimensions, + ); + if (embeddings === undefined) { + response = NextResponse.json( + { + error: "Failed to generate embeddings.", + }, + { status: 500 }, + ); + } else { + response = NextResponse.json(embeddings, { status: 200 }); + } + } catch (embeddingError) { + console.error("Error generating embeddings:", embeddingError); + response = NextResponse.json( + { + error: "Failed to generate embeddings.", + details: embeddingError instanceof Error ? embeddingError.message : "Unknown embedding error", + }, + { status: 500 }, + ); + }apps/website/app/api/supabase/insert/document/route.ts (1)
18-32: Remove unused destructured fields from validation function.The validation function destructures
source_local_id,url,metadata,created, andlast_modifiedbut doesn't validate them. Consider removing these unused destructured fields for cleaner code.const validateDocument: ItemValidator<DocumentDataInput> = (data) => { const { space_id, - source_local_id, - url, - metadata, - created, - last_modified, author_id, } = data; if (!space_id) return "Missing required space_id field."; if (!author_id) return "Missing required author_id field."; return null; };apps/website/app/api/supabase/insert/space/route.ts (2)
24-53: Consider using a separate validator function for consistency.This route uses inline validation within the helper function, which is inconsistent with other routes that use separate validator functions. Consider extracting the validation logic into a separate
validateSpacefunction to maintain consistency across the codebase.+const validateSpace: ItemValidator<SpaceDataInput> = (data) => { + const { name, url, platform_id } = data; + + if (!name || typeof name !== "string" || name.trim() === "") { + return "Missing or invalid name."; + } + if (!url || typeof url !== "string" || url.trim() === "") { + return "Missing or invalid URL."; + } + if ( + platform_id === undefined || + platform_id === null || + typeof platform_id !== "number" + ) { + return "Missing or invalid platform_id."; + } + return null; +}; const processAndGetOrCreateSpace = async ( supabasePromise: ReturnType<typeof createClient>, data: SpaceDataInput, ): Promise<GetOrCreateEntityResult<SpaceRecord>> => { const { name, url, platform_id } = data; - // --- Start of validation --- - if (!name || typeof name !== "string" || name.trim() === "") { - return { - entity: null, - error: "Missing or invalid name.", - created: false, - status: 400, - }; - } - // ... rest of validation logic - // --- End of validation ---
99-105: Remove redundant validation in POST handler.The POST handler performs basic validation that is already covered more thoroughly in the helper function. This creates redundancy and potential inconsistency. Consider removing this validation or moving all validation to a separate validator function.
export const POST = async (request: NextRequest): Promise<NextResponse> => { const supabasePromise = createClient(); try { const body: SpaceDataInput = await request.json(); - // Minimal validation here, more detailed in the helper - if (!body || typeof body !== "object") { - return createApiResponse(request, { - error: "Invalid request body: expected a JSON object.", - status: 400, - }); - } const result = await processAndGetOrCreateSpace(supabasePromise, body);apps/website/app/api/supabase/insert/platform/route.ts (2)
38-45: Remove redundant validation logic.The null checks for
platformNameandplatformUrlare redundant since the previous conditional logic ensures these values are either both set or the function returns early.This validation block can be removed:
- if (!platformName || !platformUrl) { - return { - error: "Platform name or URL could not be derived.", - entity: null, - created: false, - status: 400, - }; - }
65-70: Improve input validation specificity.The current validation only checks for string type but doesn't validate URL format or provide specific guidance.
Consider more comprehensive URL validation:
- if (!url || typeof url !== "string") { + if (!url || typeof url !== "string" || url.trim().length === 0) { return createApiResponse(request, { - error: "Missing or invalid url in request body.", + error: "Missing, empty, or invalid url in request body. Please provide a valid URL string.", status: 400, }); }apps/website/app/api/supabase/insert/account/route.ts (1)
81-92: Consolidate validation with helper function logic.The validation logic here duplicates what's already in
getOrCreateAccount. Consider removing the validation from the helper function and keeping it here for better separation of concerns.This approach centralizes validation in the route handler while keeping business logic in the helper function.
apps/website/app/api/supabase/insert/content/route.ts (2)
15-16: Consider reducing coupling by avoiding exports of internal types.Exporting
ContentDataInputandContentRecordtypes increases coupling between modules. Consider whether these types need to be exported or if they should remain internal to this route.If these types are only used internally, remove the
exportkeywords to reduce module coupling.
18-53: Consider moving validation function to utility module.The exported
inputValidationfunction suggests it might be used elsewhere. If so, consider moving it to a shared utility module for better organization and reusability.Move validation functions to a dedicated validation utility module if they're shared across routes.
apps/website/app/api/supabase/insert/content-embedding/batch/route.ts (1)
59-59: Remove unsafe non-null assertion.The non-null assertion
embeddingItemsSet!is unsafe sinceby_model[table_name]could theoretically be undefined.Add proper validation or use optional chaining:
- const embeddingItemsSet = by_model[table_name]; + const embeddingItemsSet = by_model[table_name]; + if (!embeddingItemsSet) continue; // Skip if no items for this model const results = await processAndInsertBatch< // ... existing code ... >( supabase, - embeddingItemsSet!, + embeddingItemsSet,apps/website/app/api/supabase/insert/content-embedding/route.ts (3)
64-66: Consider using strict equality for consistency.While the current comparison works correctly for numbers, using strict equality would be more consistent with the codebase style.
- if (vector.length != table_size) { + if (vector.length !== table_size) {
102-104: Consider using strict equality for consistency.Same as above, prefer strict equality for consistency.
- if (vector.length != table_size) { + if (vector.length !== table_size) {
241-245: Address TODO: Replace with generic name.The TODO comment indicates the route name should be replaced with something more generic.
Would you like me to suggest a more generic name for this error handler call or create an issue to track this task?
apps/website/app/utils/supabase/dbUtils.ts (2)
22-31: Simplify the return statement logic.The function correctly handles undefined cases, but the logic can be simplified since accessing a non-existent key in an object returns
undefinedanyway.Apply this diff to simplify the return statement:
export function get_known_embedding( model: string, dimensions: number, provider: string, ): EmbeddingTableData | undefined { const embedding_name = known_embeddings[`${provider || "openai"}-${model}-${dimensions}`]; - return known_embedding_tables[embedding_name || ""]; + return embedding_name ? known_embedding_tables[embedding_name] : undefined; }
1-440: Well-architected database utility module!This module demonstrates excellent design patterns:
- Type Safety: Extensive use of TypeScript generics ensures compile-time safety
- DRY Principle: Common patterns are abstracted into reusable functions
- Error Recovery: Sophisticated handling of race conditions and constraint violations
- Extensibility: The processor/validator pattern in batch operations allows for flexible data transformation
Consider documenting the race condition handling strategy in a README or architectural decision record (ADR) as it's a valuable pattern that other developers should understand and potentially reuse elsewhere in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
apps/website/app/api/embeddings/route.ts(1 hunks)apps/website/app/api/supabase/insert/account/route.ts(1 hunks)apps/website/app/api/supabase/insert/agents/route.ts(1 hunks)apps/website/app/api/supabase/insert/content-embedding/batch/route.ts(1 hunks)apps/website/app/api/supabase/insert/content-embedding/route.ts(1 hunks)apps/website/app/api/supabase/insert/content/batch/route.ts(1 hunks)apps/website/app/api/supabase/insert/content/route.ts(1 hunks)apps/website/app/api/supabase/insert/document/route.ts(1 hunks)apps/website/app/api/supabase/insert/person/route.ts(1 hunks)apps/website/app/api/supabase/insert/platform/route.ts(1 hunks)apps/website/app/api/supabase/insert/space/route.ts(1 hunks)apps/website/app/api/supabase/rpc/search-content/route.ts(1 hunks)apps/website/app/utils/supabase/apiUtils.ts(1 hunks)apps/website/app/utils/supabase/dbUtils.ts(1 hunks)apps/website/app/utils/supabase/server.ts(1 hunks)apps/website/app/utils/supabase/types.gen.ts(1 hunks)apps/website/package.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/website/app/utils/supabase/server.ts (1)
apps/website/app/utils/supabase/types.gen.ts (1)
Database(9-645)
apps/website/app/utils/supabase/apiUtils.ts (1)
apps/website/app/utils/llm/cors.ts (1)
cors(12-39)
apps/website/app/utils/supabase/dbUtils.ts (1)
apps/website/app/utils/supabase/types.gen.ts (3)
Database(9-645)TablesInsert(676-697)Tables(649-674)
🪛 Biome (1.9.4)
apps/website/app/api/supabase/rpc/search-content/route.ts
[error] 63-64: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (14)
apps/website/package.json (1)
17-17:✅ Verification successful
Verify package versions for security and currency.
The new dependencies
@supabase/ssrandopenaishould be verified to ensure they are the latest stable versions and free from security vulnerabilities.Also applies to: 20-20
🌐 Web query:
What are the latest stable versions of @supabase/ssr and openai npm packages, and are there any known security vulnerabilities in versions ^0.6.1 and ^4.98.0 respectively?💡 Result:
As of May 23, 2025, the latest stable versions of the
@supabase/ssrandopenainpm packages are:
- @supabase/ssr: Version 0.6.1
- openai: Version 4.98.0
Regarding security vulnerabilities, there are no known issues in these versions.
Dependencies Verified: Up-to-Date & Secure
Both
@supabase/ssr@^0.6.1andopenai@^4.98.0are the latest stable releases as of May 23, 2025, and have no known security vulnerabilities. No further action is needed.apps/website/app/utils/supabase/server.ts (1)
5-46: Well-implemented Supabase SSR client creation.This implementation follows best practices for server-side Supabase client creation:
- Proper environment variable validation with clear error messages
- Correct use of Next.js cookies API with async/await
- Appropriate cookie handling for SSR with documented error handling in
setAll- Type safety with the Database schema
The try-catch block in
setAllis correctly implemented to handle the known limitation when called from Server Components.apps/website/app/api/supabase/insert/agents/route.ts (1)
1-71: Well-structured API route with comprehensive validation.This implementation demonstrates excellent practices:
- Proper input validation with clear error messages
- Type-safe operations using generated Supabase types
- Consistent use of shared utilities for database operations and API responses
- Comprehensive error handling with appropriate HTTP status codes
- Clean separation of concerns with the
getOrCreateAgentByTypehelper functionThe validation logic properly checks for missing, empty, or invalid agent types before proceeding with database operations.
apps/website/app/api/supabase/insert/content/batch/route.ts (1)
34-61: Well-structured batch insertion handler.The POST handler correctly validates the input as an array, uses the generic batch processing utility, and provides comprehensive error handling. The API response structure is consistent with the established pattern.
apps/website/app/api/supabase/insert/document/route.ts (1)
34-70: Excellent foreign key error handling.The
createDocumentfunction properly handles foreign key constraint violations and provides meaningful error messages for bothspace_idandauthor_idconstraints. The implementation follows the established pattern effectively.apps/website/app/api/supabase/insert/account/route.ts (1)
54-73: Excellent foreign key constraint error handling.The detailed error handling for foreign key constraints provides clear, actionable feedback to API consumers by parsing constraint names and providing context-specific error messages.
apps/website/app/utils/supabase/types.gen.ts (1)
1-803: Well-structured type definitions for the database schema.The auto-generated types provide comprehensive type safety for database operations. The schema design demonstrates good practices with proper foreign key relationships and consistent naming conventions.
apps/website/app/api/supabase/rpc/search-content/route.ts (2)
40-45: Proper null safety check with early return.Good defensive programming by validating the model information before proceeding with embedding operations.
99-106: Excellent optimization for empty array case.Good performance optimization by returning early when
subsetPlatformIdsis empty, avoiding unnecessary RPC calls. The detailed logging also helps with debugging.apps/website/app/utils/supabase/apiUtils.ts (2)
81-90: Good error handling for missing API key.The code properly checks for the API key and logs an error if missing. The conditional OpenAI client initialization is a good defensive approach.
117-125: Excellent timeout handling implementation.Great use of
Promise.racepattern for implementing request timeout. The 30-second timeout is reasonable for API calls, and the error message is clear.apps/website/app/api/supabase/insert/content-embedding/route.ts (1)
174-193: Excellent foreign key error handling.Great approach to providing specific, user-friendly error messages for foreign key violations. The check for target_id constraint helps users understand exactly what went wrong.
apps/website/app/utils/supabase/dbUtils.ts (2)
213-440: Well-designed batch operation utilities!The batch operation functions (
InsertValidatedBatch,validateAndInsertBatch, andprocessAndInsertBatch) are well-structured with:
- Consistent error handling patterns
- Proper partial error tracking
- Good separation of concerns between validation and processing
- Comprehensive logging for debugging
The implementation provides a solid foundation for batch database operations.
103-171: Excellent error handling implementation!The error handling in
getOrCreateEntityis particularly well-designed:
- Proper handling of race conditions with unique constraint violations (error code 23505)
- Clear foreign key constraint error messages (error code 23503)
- Appropriate HTTP status codes for different error scenarios
- Comprehensive logging for debugging
This provides a robust foundation for database operations.
| if (agent_response.error || agent_response.entity === null) | ||
| return agent_response as any as GetOrCreateEntityResult<PersonRecord>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unsafe type casting.
The type casting as any as GetOrCreateEntityResult<PersonRecord> bypasses TypeScript's type safety and could lead to runtime errors if the Agent response structure doesn't match the expected Person result structure.
Consider creating a proper error response instead:
- if (agent_response.error || agent_response.entity === null)
- return agent_response as any as GetOrCreateEntityResult<PersonRecord>;
+ if (agent_response.error || agent_response.entity === null) {
+ return {
+ entity: null,
+ error: agent_response.error || "Failed to create Agent for Person",
+ details: agent_response.details,
+ created: false,
+ status: agent_response.status || 500,
+ };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (agent_response.error || agent_response.entity === null) | |
| return agent_response as any as GetOrCreateEntityResult<PersonRecord>; | |
| if (agent_response.error || agent_response.entity === null) { | |
| return { | |
| entity: null, | |
| error: agent_response.error || "Failed to create Agent for Person", | |
| details: agent_response.details, | |
| created: false, | |
| status: agent_response.status || 500, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/website/app/api/supabase/insert/person/route.ts around lines 32 to 33,
avoid using unsafe double type casting with 'as any as
GetOrCreateEntityResult<PersonRecord>' which bypasses TypeScript's type safety.
Instead, create and return a proper error response object that conforms to the
GetOrCreateEntityResult<PersonRecord> type, ensuring the error and null entity
cases are handled explicitly and safely without forcing the type cast.
| return getOrCreateEntity<"Person">( | ||
| supabase, | ||
| "Person", | ||
| "id, name, email, orcid, type", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify field selection matches table schema.
The field selection includes "id, name, email, orcid, type" but the Person table may not have a type field since that's typically in the Agent table.
🏁 Script executed:
#!/bin/bash
# Description: Check if Person table has a 'type' field in the database schema
# Expected: Person table schema should not include 'type' field
ast-grep --pattern $'Person": {
$$$
"type": $_,
$$$
}'Length of output: 61
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for Prisma schema..."
PRISMA_PATH=$(fd --hidden --max-depth 2 --glob "schema.prisma" || true)
if [[ -n "$PRISMA_PATH" ]]; then
echo "Found Prisma schema at: $PRISMA_PATH"
echo "Extracting Person model..."
rg -n "model Person" -A5 "$PRISMA_PATH" || true
else
echo "No Prisma schema.prisma found."
fi
echo
echo "Looking for SQL migrations..."
SQL_FILES=$(fd --hidden --glob "*.sql" || true)
if [[ -n "$SQL_FILES" ]]; then
for f in $SQL_FILES; do
grep -HinR "create table.*person" -n "$f" || true
done
else
echo "No .sql migration files found."
fi
echo
echo "Searching TypeScript interfaces or Zod schemas for Person..."
rg -n "interface Person" || true
rg -n "type Person" || true
rg -n "z\\.object\\(.*Person" -A3 || trueLength of output: 5693
#!/bin/bash
set -e
FILES=(
"packages/database/supabase/migrations/20250504202930_content_tables.sql"
"packages/database/supabase/schemas/agent.sql"
)
for f in "${FILES[@]}"; do
if [[ -f "$f" ]]; then
echo "----- $f -----"
sed -n '/CREATE TABLE.*"Person"/,/^);/p' "$f" | nl
echo
else
echo "File not found: $f"
fi
doneThis will display the column definitions for the Person table so we can verify whether a type column exists.
#!/bin/bash
set -e
FILES=(
"packages/database/supabase/migrations/20250504202930_content_tables.sql"
"packages/database/supabase/schemas/agent.sql"
)
for f in "${FILES[@]}"; do
if [[ -f "$f" ]]; then
echo "----- $f -----"
# Print column definitions for Person table
sed -n '/CREATE TABLE.*"Person"/,/);/p' "$f"
echo
else
echo "File not found: $f"
fi
done🏁 Script executed:
#!/bin/bash
set -e
echo "---- content_tables migration Person table ----"
sed -n '/CREATE TABLE "Person"/,/);/p' packages/database/supabase/migrations/20250504202930_content_tables.sql || echo "Not found"
echo
echo "---- agent schema Person table ----"
sed -n '/CREATE TABLE IF NOT EXISTS public."Person"/,/);/p' packages/database/supabase/schemas/agent.sql || echo "Not found"Length of output: 841
Remove invalid type field from Person selection
The Person table schema (in both migrations and agent.sql) defines only id, name, orcid, and email—there is no type column. Update your field list in apps/website/app/api/supabase/insert/person/route.ts (line 37) accordingly:
- "id, name, email, orcid, type",
+ "id, name, email, orcid",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "id, name, email, orcid, type", | |
| "id, name, email, orcid", |
🤖 Prompt for AI Agents
In apps/website/app/api/supabase/insert/person/route.ts at line 37, the field
selection includes "type" which does not exist in the Person table schema.
Remove "type" from the selected fields so that only valid columns "id, name,
email, orcid" are included to match the actual Person table structure.
| return validateAndInsertBatch<"Content">( | ||
| supabase, | ||
| contentItems, | ||
| "Content", // Table name | ||
| "*", // Select query (can be more specific, e.g., "id, text, scale, ...") | ||
| "Content", // Entity name for logging | ||
| inputValidation, | ||
| null, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the purpose of the null parameter.
The null parameter passed as the last argument to validateAndInsertBatch needs clarification. Consider adding a comment explaining what this parameter represents or using a named constant if it has semantic meaning.
🏁 Script executed:
#!/bin/bash
# Description: Check the signature of validateAndInsertBatch to understand the null parameter
ast-grep --pattern 'function validateAndInsertBatch($$$) {
$$$
}'
ast-grep --pattern 'validateAndInsertBatch<$_>($$$)'Length of output: 952
🏁 Script executed:
#!/bin/bash
# Locate definition and imports of validateAndInsertBatch
rg "validateAndInsertBatch" -n .Length of output: 326
🏁 Script executed:
#!/bin/bash
# Extract signature and JSDoc for validateAndInsertBatch
rg -n 'export async function validateAndInsertBatch' -C 10 apps/website/app/utils/supabase/dbUtils.tsLength of output: 898
Clarify the null as the output validator
The final null you pass is the outputValidator (per the function signature). To make the intent explicit, consider one of the following:
- Use a named constant (e.g.
const noOutputValidator: ItemValidator<…> | null = null) - Add an inline comment (
null /* no output validation */)
Locations:
- apps/website/app/api/supabase/insert/content/batch/route.ts (lines 23–31)
🤖 Prompt for AI Agents
In apps/website/app/api/supabase/insert/content/batch/route.ts around lines 23
to 32, the last argument passed as null to validateAndInsertBatch represents the
outputValidator but this is not clear. To fix this, either define a named
constant like noOutputValidator with the type ItemValidator or add an inline
comment next to null explaining it means no output validation, e.g., null /* no
output validation */. This will clarify the purpose of the null parameter for
future readers.
| ) { | ||
| return { | ||
| ...result, | ||
| error: `Invalid platform_id: No Space record found for ID ${platform_id}.`, | ||
| }; |
There was a problem hiding this comment.
Fix incorrect error message for platform_id foreign key constraint.
The error message incorrectly states "No Space record found" when it should reference the Platform table since platform_id is a foreign key to the Platform table.
return {
...result,
- error: `Invalid platform_id: No Space record found for ID ${platform_id}.`,
+ error: `Invalid platform_id: No Platform record found for ID ${platform_id}.`,
};🤖 Prompt for AI Agents
In apps/website/app/api/supabase/insert/space/route.ts around lines 82 to 86,
the error message incorrectly says "No Space record found" for an invalid
platform_id, but platform_id references the Platform table. Update the error
message to correctly state that no Platform record was found for the given
platform_id to accurately reflect the foreign key constraint.
| if (lowerCaseURL.includes("roamresearch.com")) { | ||
| platformName = "roamresearch"; | ||
| platformUrl = "https://roamresearch.com"; | ||
| } else { | ||
| console.warn("Could not determine platform from URL:", url); | ||
| return { | ||
| error: "Could not determine platform from URL.", | ||
| entity: null, | ||
| created: false, | ||
| status: 400, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limited platform support and improve URL parsing logic.
The current implementation only supports "roamresearch.com", which significantly limits the API's usefulness. Additionally, the logic could be more maintainable.
Consider refactoring to support multiple platforms using a configuration-driven approach:
- if (lowerCaseURL.includes("roamresearch.com")) {
- platformName = "roamresearch";
- platformUrl = "https://roamresearch.com";
- } else {
- console.warn("Could not determine platform from URL:", url);
- return {
- error: "Could not determine platform from URL.",
- entity: null,
- created: false,
- status: 400,
- };
- }
+ const platformConfigs = {
+ "roamresearch.com": { name: "roamresearch", url: "https://roamresearch.com" },
+ "obsidian.md": { name: "obsidian", url: "https://obsidian.md" },
+ // Add more platforms as needed
+ };
+
+ const matchedPlatform = Object.entries(platformConfigs).find(([domain]) =>
+ lowerCaseURL.includes(domain)
+ );
+
+ if (matchedPlatform) {
+ const [, config] = matchedPlatform;
+ platformName = config.name;
+ platformUrl = config.url;
+ } else {
+ console.warn("Could not determine platform from URL:", url);
+ return {
+ error: "Could not determine platform from URL.",
+ entity: null,
+ created: false,
+ status: 400,
+ };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (lowerCaseURL.includes("roamresearch.com")) { | |
| platformName = "roamresearch"; | |
| platformUrl = "https://roamresearch.com"; | |
| } else { | |
| console.warn("Could not determine platform from URL:", url); | |
| return { | |
| error: "Could not determine platform from URL.", | |
| entity: null, | |
| created: false, | |
| status: 400, | |
| }; | |
| } | |
| const platformConfigs = { | |
| "roamresearch.com": { name: "roamresearch", url: "https://roamresearch.com" }, | |
| "obsidian.md": { name: "obsidian", url: "https://obsidian.md" }, | |
| // Add more platforms as needed | |
| }; | |
| const matchedPlatform = Object.entries(platformConfigs).find( | |
| ([domain]) => lowerCaseURL.includes(domain) | |
| ); | |
| if (matchedPlatform) { | |
| const [, config] = matchedPlatform; | |
| platformName = config.name; | |
| platformUrl = config.url; | |
| } else { | |
| console.warn("Could not determine platform from URL:", url); | |
| return { | |
| error: "Could not determine platform from URL.", | |
| entity: null, | |
| created: false, | |
| status: 400, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/website/app/api/supabase/insert/platform/route.ts around lines 25 to 36,
the code only supports "roamresearch.com" and uses a simple string inclusion
check for URL parsing. Refactor this by creating a configuration object or array
that maps platform names to their base URLs, then iterate or search this
configuration to determine the platform based on the input URL. This will make
the code more maintainable and easily extendable to support multiple platforms
without hardcoding each one.
| const results = await processAndInsertBatch< | ||
| // any table for type checking purposes only | ||
| "ContentEmbedding_openai_text_embedding_3_small_1536", | ||
| ApiInputEmbeddingItem, | ||
| ApiOutputEmbeddingRecord | ||
| >( | ||
| supabase, | ||
| embeddingItemsSet!, | ||
| table_name, | ||
| "*", // Select all fields, adjust if needed for ContentEmbeddingRecord | ||
| "ContentEmbedding", | ||
| inputProcessing, | ||
| outputProcessing, | ||
| ); |
There was a problem hiding this comment.
Fix hardcoded table name in generic type annotation.
The type annotation uses a specific table name (ContentEmbedding_openai_text_embedding_3_small_1536) which doesn't match the dynamic table_name variable.
The hardcoded table name creates a type safety issue:
const results = await processAndInsertBatch<
- // any table for type checking purposes only
- "ContentEmbedding_openai_text_embedding_3_small_1536",
+ keyof Database["public"]["Tables"], // Use proper table key type
ApiInputEmbeddingItem,
ApiOutputEmbeddingRecord
>(Alternatively, if the generic type system can't handle dynamic table names, consider using a type assertion or refactor the utility function.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const results = await processAndInsertBatch< | |
| // any table for type checking purposes only | |
| "ContentEmbedding_openai_text_embedding_3_small_1536", | |
| ApiInputEmbeddingItem, | |
| ApiOutputEmbeddingRecord | |
| >( | |
| supabase, | |
| embeddingItemsSet!, | |
| table_name, | |
| "*", // Select all fields, adjust if needed for ContentEmbeddingRecord | |
| "ContentEmbedding", | |
| inputProcessing, | |
| outputProcessing, | |
| ); | |
| const results = await processAndInsertBatch< | |
| keyof Database["public"]["Tables"], // Use proper table key type | |
| ApiInputEmbeddingItem, | |
| ApiOutputEmbeddingRecord | |
| >( | |
| supabase, | |
| embeddingItemsSet!, | |
| table_name, | |
| "*", // Select all fields, adjust if needed for ContentEmbeddingRecord | |
| "ContentEmbedding", | |
| inputProcessing, | |
| outputProcessing, | |
| ); |
🤖 Prompt for AI Agents
In apps/website/app/api/supabase/insert/content-embedding/batch/route.ts around
lines 52 to 65, the generic type annotation for the table name is hardcoded as
"ContentEmbedding_openai_text_embedding_3_small_1536" which conflicts with the
dynamic table_name variable. To fix this, replace the hardcoded table name in
the generic type parameter with a more generic or dynamic type that aligns with
the actual table_name variable or use a type assertion to bypass the strict type
check. Alternatively, refactor the processAndInsertBatch function to better
support dynamic table names in its type system.
|
|
||
| const globalResults: ApiOutputEmbeddingRecord[] = []; | ||
| const partial_errors = []; | ||
| let created = true; // TODO: Maybe transmit from below |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix hardcoded created flag and improve logic.
The created variable is hardcoded to true but the TODO comment suggests it should be derived from the batch processing results.
Properly track creation status:
- let created = true; // TODO: Maybe transmit from below
+ let hasAnyCreated = false;
for (const table_name of Object.keys(by_model)) {
const embeddingItemsSet = by_model[table_name];
const results = await processAndInsertBatch<
// ... existing code ...
);
if (results.error || results.data === undefined)
return { ...results, data: undefined };
globalResults.push(...results.data);
+ if (results.status === 201) hasAnyCreated = true;
if (results.partial_errors !== undefined)
partial_errors.push(...results.partial_errors);
}
return {
data: globalResults,
partial_errors,
- status: created ? 201 : 200,
+ status: hasAnyCreated ? 201 : 200,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let created = true; // TODO: Maybe transmit from below | |
| // Replace the hard-coded flag with a real tracker | |
| - let created = true; // TODO: Maybe transmit from below | |
| + let hasAnyCreated = false; | |
| for (const table_name of Object.keys(by_model)) { | |
| const embeddingItemsSet = by_model[table_name]; | |
| const results = await processAndInsertBatch< | |
| // ... existing code ... | |
| ); | |
| if (results.error || results.data === undefined) | |
| return { ...results, data: undefined }; | |
| globalResults.push(...results.data); | |
| + if (results.status === 201) hasAnyCreated = true; | |
| if (results.partial_errors !== undefined) | |
| partial_errors.push(...results.partial_errors); | |
| } | |
| return { | |
| data: globalResults, | |
| partial_errors, | |
| - status: created ? 201 : 200, | |
| + status: hasAnyCreated ? 201 : 200, | |
| }; |
🤖 Prompt for AI Agents
In apps/website/app/api/supabase/insert/content-embedding/batch/route.ts at line
49, the variable 'created' is currently hardcoded to true, which does not
reflect the actual creation status. Update the code to derive 'created' from the
batch processing results, such as checking if the insert operation succeeded or
if new records were created, and assign the boolean accordingly instead of a
fixed true value.
| try { | ||
| embeddingItems.reduce((acc, item, index) => { | ||
| if (!item?.model) { | ||
| throw new Error(`Element ${index} undefined or does not have a model`); | ||
| } | ||
| if (acc[item.model] === undefined) { | ||
| acc[item.model] = []; | ||
| } | ||
| acc[item.model]!.push(item); | ||
| return acc; | ||
| }, by_model); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| return { | ||
| status: 400, | ||
| error: error.message, | ||
| }; | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify error handling in grouping logic.
The try-catch around the reduce operation adds unnecessary complexity. The validation can be handled more elegantly using array methods.
Replace the complex error handling with cleaner validation:
- const by_model: { [key: string]: ApiInputEmbeddingItem[] } = {};
- try {
- embeddingItems.reduce((acc, item, index) => {
- if (!item?.model) {
- throw new Error(`Element ${index} undefined or does not have a model`);
- }
- if (acc[item.model] === undefined) {
- acc[item.model] = [];
- }
- acc[item.model]!.push(item);
- return acc;
- }, by_model);
- } catch (error) {
- if (error instanceof Error) {
- return {
- status: 400,
- error: error.message,
- };
- }
- throw error;
- }
+ // Validate all items have models first
+ const invalidIndex = embeddingItems.findIndex(item => !item?.model);
+ if (invalidIndex !== -1) {
+ return {
+ status: 400,
+ error: `Element ${invalidIndex} undefined or does not have a model`,
+ };
+ }
+
+ // Group by model - now safe since all items are validated
+ const by_model = embeddingItems.reduce((acc, item) => {
+ const model = item.model!; // Safe due to validation above
+ if (!acc[model]) {
+ acc[model] = [];
+ }
+ acc[model].push(item);
+ return acc;
+ }, {} as { [key: string]: ApiInputEmbeddingItem[] });🤖 Prompt for AI Agents
In apps/website/app/api/supabase/insert/content-embedding/batch/route.ts between
lines 26 and 45, remove the try-catch block around the reduce function and
instead validate the embeddingItems array before the reduce call using array
methods like every or some to check for missing or invalid model properties. If
validation fails, return the error response immediately. This simplifies the
error handling by separating validation from grouping logic.
| async function openai_embedding( | ||
| input: string | string[], | ||
| model: string, | ||
| dimensions?: number, | ||
| ): Promise<number[] | number[][] | undefined> { | ||
| let options: OpenAI.EmbeddingCreateParams = { | ||
| model, | ||
| input, | ||
| }; | ||
| if (dimensions) { | ||
| options = { ...options, ...{ dimensions } }; | ||
| } | ||
|
|
||
| const embeddingsPromise = openai!.embeddings.create(options); | ||
| const timeoutPromise = new Promise<never>((_, reject) => | ||
| setTimeout( | ||
| () => reject(new Error("OpenAI API request timeout")), | ||
| OPENAI_REQUEST_TIMEOUT_MS, | ||
| ), | ||
| ); | ||
|
|
||
| const response = await Promise.race([embeddingsPromise, timeoutPromise]); | ||
| const embeddings = response.data.map((d) => d.embedding); | ||
| if (Array.isArray(input)) return embeddings; | ||
| else return embeddings[0]; | ||
| } |
There was a problem hiding this comment.
Critical: Add null check before using OpenAI client.
The non-null assertion openai! on line 116 is risky. If the API key is missing, openai will be null and this will throw a runtime error.
Add a null check at the beginning of the function:
async function openai_embedding(
input: string | string[],
model: string,
dimensions?: number,
): Promise<number[] | number[][] | undefined> {
+ if (!openai) {
+ throw new Error("OpenAI client not initialized. Check OPENAI_API_KEY.");
+ }
let options: OpenAI.EmbeddingCreateParams = {
model,
input,
};
if (dimensions) {
options = { ...options, ...{ dimensions } };
}
- const embeddingsPromise = openai!.embeddings.create(options);
+ const embeddingsPromise = openai.embeddings.create(options);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function openai_embedding( | |
| input: string | string[], | |
| model: string, | |
| dimensions?: number, | |
| ): Promise<number[] | number[][] | undefined> { | |
| let options: OpenAI.EmbeddingCreateParams = { | |
| model, | |
| input, | |
| }; | |
| if (dimensions) { | |
| options = { ...options, ...{ dimensions } }; | |
| } | |
| const embeddingsPromise = openai!.embeddings.create(options); | |
| const timeoutPromise = new Promise<never>((_, reject) => | |
| setTimeout( | |
| () => reject(new Error("OpenAI API request timeout")), | |
| OPENAI_REQUEST_TIMEOUT_MS, | |
| ), | |
| ); | |
| const response = await Promise.race([embeddingsPromise, timeoutPromise]); | |
| const embeddings = response.data.map((d) => d.embedding); | |
| if (Array.isArray(input)) return embeddings; | |
| else return embeddings[0]; | |
| } | |
| async function openai_embedding( | |
| input: string | string[], | |
| model: string, | |
| dimensions?: number, | |
| ): Promise<number[] | number[][] | undefined> { | |
| if (!openai) { | |
| throw new Error("OpenAI client not initialized. Check OPENAI_API_KEY."); | |
| } | |
| let options: OpenAI.EmbeddingCreateParams = { | |
| model, | |
| input, | |
| }; | |
| if (dimensions) { | |
| options = { ...options, ...{ dimensions } }; | |
| } | |
| const embeddingsPromise = openai.embeddings.create(options); | |
| const timeoutPromise = new Promise<never>((_, reject) => | |
| setTimeout( | |
| () => reject(new Error("OpenAI API request timeout")), | |
| OPENAI_REQUEST_TIMEOUT_MS, | |
| ), | |
| ); | |
| const response = await Promise.race([embeddingsPromise, timeoutPromise]); | |
| const embeddings = response.data.map((d) => d.embedding); | |
| if (Array.isArray(input)) return embeddings; | |
| else return embeddings[0]; | |
| } |
🤖 Prompt for AI Agents
In apps/website/app/utils/supabase/apiUtils.ts around lines 103 to 128, the code
uses the OpenAI client with a non-null assertion which can cause a runtime error
if the client is null. Add a null check at the start of the openai_embedding
function to verify that the OpenAI client is initialized; if it is null or
undefined, throw an appropriate error or handle the case gracefully before
proceeding with the API call.
| export async function getOrCreateEntity< | ||
| TableName extends keyof Database["public"]["Tables"], | ||
| >( | ||
| supabase: SupabaseClient<Database, "public", Database["public"]>, | ||
| tableName: keyof Database["public"]["Tables"], | ||
| selectQuery: string, | ||
| matchCriteria: Record<string, any>, | ||
| insertData: TablesInsert<TableName>, // Flexible insert data | ||
| entityName: string = tableName, | ||
| ): Promise<GetOrCreateEntityResult<Tables<TableName>>> { |
There was a problem hiding this comment.
Fix type inconsistency for the tableName parameter.
The tableName parameter should use the generic TableName type for consistency with the rest of the function signature. This ensures better type safety.
Apply this diff to fix the type inconsistency:
export async function getOrCreateEntity<
TableName extends keyof Database["public"]["Tables"],
>(
supabase: SupabaseClient<Database, "public", Database["public"]>,
- tableName: keyof Database["public"]["Tables"],
+ tableName: TableName,
selectQuery: string,
matchCriteria: Record<string, any>,
insertData: TablesInsert<TableName>, // Flexible insert data
entityName: string = tableName,
): Promise<GetOrCreateEntityResult<Tables<TableName>>> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getOrCreateEntity< | |
| TableName extends keyof Database["public"]["Tables"], | |
| >( | |
| supabase: SupabaseClient<Database, "public", Database["public"]>, | |
| tableName: keyof Database["public"]["Tables"], | |
| selectQuery: string, | |
| matchCriteria: Record<string, any>, | |
| insertData: TablesInsert<TableName>, // Flexible insert data | |
| entityName: string = tableName, | |
| ): Promise<GetOrCreateEntityResult<Tables<TableName>>> { | |
| export async function getOrCreateEntity< | |
| TableName extends keyof Database["public"]["Tables"], | |
| >( | |
| supabase: SupabaseClient<Database, "public", Database["public"]>, | |
| tableName: TableName, | |
| selectQuery: string, | |
| matchCriteria: Record<string, any>, | |
| insertData: TablesInsert<TableName>, // Flexible insert data | |
| entityName: string = tableName, | |
| ): Promise<GetOrCreateEntityResult<Tables<TableName>>> { | |
| // …rest of implementation… | |
| } |
🤖 Prompt for AI Agents
In apps/website/app/utils/supabase/dbUtils.ts around lines 52 to 61, the
tableName parameter is typed as keyof Database["public"]["Tables"] instead of
the generic TableName, causing a type inconsistency. Change the type of the
tableName parameter to TableName to align with the generic type used elsewhere
in the function signature, improving type safety and consistency.
|
Hey Marc-Antoine, I wanted to remove almost all the inline comments and a few console.log. Now its up to you which of these seem right to you. This is what came to mind while I was just glancing over, I have not looked it from a reviewer's pov. |
|
@maparent Can you clarify what the purpose of this PR is? I was under the impression ENG-337 would just be adding the search route. But this PR contains 3k line changes over 18 files. I'm guessing this is based off |
|
It should come after it, and starts from its tip. It is a small isolated PR once you compare it to eng-294-create-supabase-routes-squashed instead of main. (I should have found a way to tell code rabbit to start from there, but it re-reviewed everything, which is why I ignored all comments but one.) |
|
But otherwise: Expecting eng-294-create-supabase-routes-squashed to be merged first, then this. |
Using |
maparent
changed the base branch from
main
to
eng-294-create-supabase-routes-squashed
|
Ah, did not know I could do that, thanks! |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/website/app/api/supabase/rpc/search-content/route.ts (2)
63-65: Simplify condition using optional chaining.The static analysis tool correctly identified that this condition can be simplified for better readability.
Apply this diff to use optional chaining:
if ( - newEmbedding !== undefined && - newEmbedding.length && + newEmbedding?.length && !Array.isArray(newEmbedding[0]) ) {🧰 Tools
🪛 Biome (1.9.4)
[error] 63-64: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
125-183: Robust POST handler with comprehensive error handling.The handler demonstrates excellent practices with proper try-catch blocks, specific error status codes, and detailed error messages. The CORS integration is also well-implemented.
Consider reducing logging verbosity for production environments:
- console.log("[API Route] POST: Parsed request body:", body); + console.log("[API Route] POST: Request received");You might want to make detailed logging conditional based on environment variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/website/app/api/supabase/rpc/search-content/route.ts(1 hunks)apps/website/app/utils/supabase/apiUtils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/website/app/utils/supabase/apiUtils.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/website/app/api/supabase/rpc/search-content/route.ts
[error] 63-64: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
apps/website/app/api/supabase/rpc/search-content/route.ts (3)
1-22: LGTM! Well-structured imports and type definitions.The imports are appropriate for the functionality, and the type definitions demonstrate good TypeScript practices with comprehensive parameter coverage and proper database type references.
87-123: Excellent RPC logic with proper conditional handling.The implementation correctly handles the mutual exclusivity between
currentDocumentIdandsubsetPlatformIds, includes a smart optimization for empty arrays, and properly routes to the appropriate RPC function based on parameters.
185-188: LGTM! Proper CORS preflight support.The OPTIONS handler correctly implements CORS preflight support with appropriate status code and response handling.
maparent
force-pushed
the
ENG-337-search-route
branch
from
d32d5dc to
502ef31
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
502ef31 to
fbae81f
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
fbae81f to
461110f
Compare
maparent
force-pushed
the
eng-294-create-supabase-routes-squashed
branch
from
ec4ae5e to
4916281
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
32f63d1 to
ccddee0
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
ccddee0 to
4097a85
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
0f23c3b to
70042e2
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
70042e2 to
2d70fdb
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
c46314a to
251319e
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
251319e to
a08f820
Compare
maparent
force-pushed
the
eng-294-create-supabase-routes-squashed
branch
from
2d9a054 to
351bed8
Compare
maparent
force-pushed
the
ENG-337-search-route
branch
from
a08f820 to
f08b15e
Compare
|
RE: In addition, the PR includes several unresolved Coderabbit comments and needs refactoring to align with our style guide. I understand this PR was handed off midstream and the requirements may not have been clear from the start. Given all that, I’m going to close this PR so we can start fresh. Let’s sync up to clarify the goals and requirements and open a clean branch with a tighter scope. 💪 |
Summary by CodeRabbit