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

Restyling #188

Merged
merged 21 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from 19 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
6 changes: 0 additions & 6 deletions .env.local.template

This file was deleted.

29 changes: 29 additions & 0 deletions app/api/chat/[graph]/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { NextRequest, NextResponse } from "next/server"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Move hardcoded URL to environment variable

The server URL should not be hardcoded in the code. Consider using environment variables for better configuration management across different environments.

Add this to your .env file:

+CHAT_API_URL=http://127.0.0.1:5000

Then modify the imports:

import { NextRequest, NextResponse } from "next/server"
+import { env } from "@/env.mjs"

Committable suggestion skipped: line range outside the PR's diff.



export async function POST(request: NextRequest, { params }: { params: { graph: string } }) {

const graphName = params.graph
const msg = request.nextUrl.searchParams.get('msg')

Comment on lines +4 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation and type safety

The route handler lacks proper input validation and type definitions.

Consider implementing these improvements:

+interface ChatRequest {
+  repo: string;
+  msg: string;
+}
+
+interface ChatResponse {
+  result: unknown;
+}
+
 export async function POST(request: NextRequest, { params }: { params: { graph: string } }) {
     const graphName = params.graph
     const msg = request.nextUrl.searchParams.get('msg')
+    
+    if (!msg?.trim()) {
+        return NextResponse.json(
+            { message: 'Message is required' },
+            { 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.

Suggested change
export async function POST(request: NextRequest, { params }: { params: { graph: string } }) {
const graphName = params.graph
const msg = request.nextUrl.searchParams.get('msg')
interface ChatRequest {
repo: string;
msg: string;
}
interface ChatResponse {
result: unknown;
}
export async function POST(request: NextRequest, { params }: { params: { graph: string } }) {
const graphName = params.graph
const msg = request.nextUrl.searchParams.get('msg')
if (!msg?.trim()) {
return NextResponse.json(
{ message: 'Message is required' },
{ status: 400 }
)
}

try {
const result = await fetch(`${process.env.BEAKEND_URL}/chat`, {
method: 'POST',
body: JSON.stringify({ repo: graphName, msg }),
headers: {
"Authorization": process.env.SECRET_TOKEN!,
"Content-Type": 'application/json'
}
})

if (!result.ok) {
throw new Error(await result.text())
}

const json = await result.json()

return NextResponse.json({ result: json }, { status: 200 })
} catch (err) {
return NextResponse.json({ message: (err as Error).message }, { status: 400 })
}
Comment on lines +25 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and add logging

The current error handling is too generic and lacks proper logging, making it difficult to debug issues in production.

Consider implementing these improvements:

-    return NextResponse.json({ message: (err as Error).message }, { status: 400 })
+    console.error('[ChatAPI] Error:', err)
+    
+    if (err.message === 'Request timeout') {
+        return NextResponse.json(
+            { error: 'Request timed out', code: 'TIMEOUT' },
+            { status: 408 }
+        )
+    }
+    
+    if (err.message.startsWith('Server error:')) {
+        return NextResponse.json(
+            { error: 'Chat service error', code: 'SERVER_ERROR' },
+            { status: 502 }
+        )
+    }
+    
+    return NextResponse.json(
+        { error: 'Internal server error', code: 'INTERNAL_ERROR' },
+        { status: 500 }
+    )

Committable suggestion skipped: line range outside the PR's diff.

}
61 changes: 46 additions & 15 deletions app/api/repo/[graph]/[node]/route.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,55 @@
import { FalkorDB, Graph } from "falkordb";
import { NextRequest, NextResponse } from "next/server";

export async function GET(request: NextRequest, { params }: { params: { graph: string, node: string } }) {

const nodeId = parseInt(params.node);

const nodeId = parseInt(params.node);
const graphId = params.graph;
try {

const result = await fetch(`${process.env.BEAKEND_URL}/get_neighbors?repo=${graphId}&node_id=${nodeId}`, {
method: 'GET',
headers: {
"Authorization": process.env.SECRET_TOKEN!,
}
})

const json = await result.json()

return NextResponse.json({ result: json }, { status: 200 })
} catch (err) {
return NextResponse.json({ massage: (err as Error).message }, { status: 400 })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct typo in error response property name from 'massage' to 'message'.

In both the GET and POST functions, the error response uses the property massage instead of message. This typo could lead to confusion when handling errors on the client side.

Apply this diff to fix the typo:

-        return NextResponse.json({ massage: (err as Error).message }, { status: 400 })
+        return NextResponse.json({ message: (err as Error).message }, { status: 400 })
-        return NextResponse.json({ massage: (err as Error).message }, { status: 200 })
+        return NextResponse.json({ message: (err as Error).message }, { status: 400 })

Note: Also updated the status code in the POST function's catch block to return an appropriate error status.

Also applies to: 49-49

}

export async function POST(request: NextRequest, { params }: { params: { graph: string, node: string } }) {

const nodeId = params.node;
const graphId = params.graph;
const targetId = request.nextUrl.searchParams.get('targetId')

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential null value of targetId.

The targetId is retrieved from the query parameters and used with a non-null assertion (!) in Number(targetId!). If targetId is null, this will cause a runtime error. It's safer to check if targetId exists before using it.

Apply this diff to add a null check for targetId:

     const targetId = request.nextUrl.searchParams.get('targetId')
+    if (!targetId) {
+        return NextResponse.json({ message: 'targetId is required' }, { status: 400 })
+    }

Also applies to: 37-37

try {

const db = await FalkorDB.connect({url: process.env.FALKORDB_URL || 'falkor://localhost:6379',});
const graph = db.selectGraph(graphId);
const result = await fetch(`${process.env.BEAKEND_URL}/find_paths`, {
method: 'POST',
headers: {
"Authorization": process.env.SECRET_TOKEN!,
'Content-Type': 'application/json'
},
body: JSON.stringify({
repo: graphId,
src: Number(nodeId),
dest: Number(targetId!)
})
})

// Get node's neighbors
const q_params = {nodeId: nodeId};
const query = `MATCH (src)-[e]-(n)
WHERE ID(src) = $nodeId
RETURN collect(distinct { label:labels(n)[0], id:ID(n), name: n.name } ) as nodes,
collect( { src: ID(startNode(e)), id: ID(e), dest: ID(endNode(e)), type: type(e) } ) as edges`;
if (!result.ok) {
throw new Error(await result.text())
}

let res: any = await graph.query(query, { params: q_params });
let nodes = res.data[0]['nodes'];
let edges = res.data[0]['edges'];
const json = await result.json()

return NextResponse.json({ id: graphId, nodes: nodes, edges: edges }, { status: 200 })
return NextResponse.json({ result: json }, { status: 200 })
} catch (err) {
return NextResponse.json({ massage: (err as Error).message }, { status: 200 })
}
}
Comment on lines +53 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Return appropriate error status code in POST function's catch block.

In the POST function, when an error occurs, the catch block returns a status code of 200. To accurately indicate an error to the client, this should be changed to an appropriate error status code, such as 400 or 500.

Apply this diff to fix the status code:

-        return NextResponse.json({ message: (err as Error).message }, { status: 200 })
+        return NextResponse.json({ message: (err as Error).message }, { 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.

Suggested change
return NextResponse.json({ massage: (err as Error).message }, { status: 200 })
}
return NextResponse.json({ message: (err as Error).message }, { status: 400 })
}

Loading
Loading