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.

Sign up for GitHub

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

Restyling #188

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5fb6d51
commit
Anchel123 Sep 30, 2024
b652d8b
remove header border
Anchel123 Sep 30, 2024
a80b174
restyle
Anchel123 Oct 28, 2024
ec11a2a
add hover card and fix show path and search
Anchel123 Oct 29, 2024
ecbef2a
Fix Typo in Chat Component
gkorland Nov 1, 2024
e7c8d76
replace localhost in ip
Anchel123 Nov 5, 2024
706b487
Merge branch 'restyling' of https://github.com/FalkorDB/code-graph in…
Anchel123 Nov 5, 2024
fbe93b2
improve styling and add multi paths messages
Anchel123 Nov 5, 2024
3d0ad9e
change colors and restyle chat
Anchel123 Nov 5, 2024
98f3cb6
fix type err
Anchel123 Nov 5, 2024
56ce407
fix alert on gibberish insert in search
Anchel123 Nov 5, 2024
d80b8f3
open suggestions only when there is suggestions and add submit in sea…
Anchel123 Nov 6, 2024
9920291
add submit in search and change selection box style
Anchel123 Nov 6, 2024
8f85cdd
remove commit list and change the chat style
Anchel123 Nov 11, 2024
1426b64
final fix
Anchel123 Nov 12, 2024
6c72394
commit
Anchel123 Nov 12, 2024
96e29aa
commit
Anchel123 Nov 12, 2024
e447054
fix search input
Anchel123 Nov 12, 2024
938dec2
commit
Anchel123 Nov 14, 2024
679b236
fix commit list
Anchel123 Nov 14, 2024
d1de315
commit
Anchel123 Nov 14, 2024
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
View file
Open in desktop

This file was deleted.

27 changes: 27 additions & 0 deletions app/api/chat/[graph]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
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(`http://127.0.0.1:5000/chat`, {
method: 'POST',
body: JSON.stringify({ repo: graphName, msg}),
headers: {
"Content-Type": 'application/json'
}
})

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

const json = await result.json()

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

Enhance API call resilience

The API call implementation lacks timeout handling and retry mechanism, which could lead to poor user experience during network issues or server delays.

Consider implementing these improvements:

+const TIMEOUT_MS = 10000
+const MAX_RETRIES = 3
+
 try {
-    const result = await fetch(`http://127.0.0.1:5000/chat`, {
+    const controller = new AbortController()
+    const timeoutId = setTimeout(() => controller.abort(), TIMEOUT_MS)
+    
+    let lastError: Error | null = null
+    for (let attempt = 0; attempt < MAX_RETRIES; attempt++) {
+        try {
+            const result = await fetch(`${env.CHAT_API_URL}/chat`, {
                 method: 'POST',
                 body: JSON.stringify({ repo: graphName, msg}),
                 headers: {
                     "Content-Type": 'application/json'
-                }
+                },
+                signal: controller.signal
             })
+            clearTimeout(timeoutId)

             if (!result.ok) {
-                throw new Error(await result.text())
+                const errorText = await result.text()
+                throw new Error(`Server error: ${result.status} - ${errorText}`)
             }

             const json = await result.json()
+            return NextResponse.json({ result: json }, { status: 200 })
+        } catch (err) {
+            lastError = err as Error
+            if (err.name === 'AbortError') {
+                throw new Error('Request timeout')
+            }
+            // Only retry on network errors
+            if (!err.message.startsWith('Server error:')) {
+                await new Promise(resolve => setTimeout(resolve, 1000 * attempt))
+                continue
+            }
+            throw err
+        }
+    }
+    throw lastError || new Error('Max retries reached')
📝 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
try {
const result = await fetch(`http://127.0.0.1:5000/chat`, {
method: 'POST',
body: JSON.stringify({ repo: graphName, msg}),
headers: {
"Content-Type": 'application/json'
}
})
if (!result.ok) {
throw new Error(await result.text())
}
const json = await result.json()
const TIMEOUT_MS = 10000
const MAX_RETRIES = 3
try {
const controller = new AbortController()
const timeoutId = setTimeout(() => controller.abort(), TIMEOUT_MS)
let lastError: Error | null = null
for (let attempt = 0; attempt < MAX_RETRIES; attempt++) {
try {
const result = await fetch(`${env.CHAT_API_URL}/chat`, {
method: 'POST',
body: JSON.stringify({ repo: graphName, msg}),
headers: {
"Content-Type": 'application/json'
},
signal: controller.signal
})
clearTimeout(timeoutId)
if (!result.ok) {
const errorText = await result.text()
throw new Error(`Server error: ${result.status} - ${errorText}`)
}
const json = await result.json()
return NextResponse.json({ result: json }, { status: 200 })
} catch (err) {
lastError = err as Error
if (err.name === 'AbortError') {
throw new Error('Request timeout')
}
// Only retry on network errors
if (!err.message.startsWith('Server error:')) {
await new Promise(resolve => setTimeout(resolve, 1000 * attempt))
continue
}
throw err
}
}
throw lastError || new Error('Max retries reached')

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.

}
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

Add rate limiting to prevent abuse

The chat endpoint is vulnerable to abuse as it lacks rate limiting. This could lead to excessive load on both your server and the chat service.

Consider implementing rate limiting using a Redis-based solution or Next.js middleware:

// app/api/chat/[graph]/middleware.ts
import { rateLimit } from '@/lib/rate-limit'
import { NextResponse } from 'next/server'

const limiter = rateLimit({
  interval: 60 * 1000, // 1 minute
  uniqueTokenPerInterval: 500,
  limit: 10, // requests per interval
})

export async function middleware(request: Request) {
  try {
    await limiter.check(request)
    return NextResponse.next()
  } catch {
    return new NextResponse('Too Many Requests', { status: 429 })
  }
}

57 changes: 42 additions & 15 deletions app/api/repo/[graph]/[node]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,51 @@
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 {
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

Ensure consistent parsing of nodeId.

In the GET function, nodeId is parsed using parseInt(params.node), whereas in the POST function, it's used directly from params.node. For consistency and to prevent potential type-related bugs, consider parsing nodeId in both functions using parseInt.

Apply this diff to make parsing consistent in the POST function:

-    const nodeId = params.node;
+    const nodeId = parseInt(params.node);

Also applies to: 23-24


const result = await fetch(`http://127.0.0.1:5000/get_neighbors?repo=${graphId}&node_id=${nodeId}`, {
method: 'GET',
})

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.

🛠️ Refactor suggestion

Check response status before processing in GET function.

In the GET function, the response from the fetch call is not checked for success before parsing it as JSON. If the response is not OK, attempting to parse it may throw an error. Consider checking result.ok and handling error responses appropriately.

Apply this diff to add response status check:

     const result = await fetch(`http://127.0.0.1:5000/get_neighbors?repo=${graphId}&node_id=${nodeId}`, {
         method: 'GET',
     })
+    if (!result.ok) {
+        throw new Error(await result.text())
+    }

This aligns error handling in the GET function with the POST function and ensures consistent behavior.

📝 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
const result = await fetch(`http://127.0.0.1:5000/get_neighbors?repo=${graphId}&node_id=${nodeId}`, {
method: 'GET',
})
const json = await result.json()
return NextResponse.json({ result: json }, { status: 200 })
} catch (err) {
const result = await fetch(`http://127.0.0.1:5000/get_neighbors?repo=${graphId}&node_id=${nodeId}`, {
method: 'GET',
})
if (!result.ok) {
throw new Error(await result.text())
}
const json = await result.json()
return NextResponse.json({ result: json }, { status: 200 })
} catch (err) {

}
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(`http://127.0.0.1:5000/find_paths`, {
method: 'POST',
headers: {
'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