Addressing PR comments#4
Merged
Merged
Conversation
- Updated all 6 database endpoints with get_optional_user dependency - Added logging for authenticated requests - Routes: list_databases, get_database, upload_database, delete_database, get_database_schema, get_database_tables - Part of Task 4: Infrastructure Chunk 1
- Added get_optional_user dependency to query endpoints - Added logging for authenticated query requests - Routes: query_database, get_cache_stats, clear_cache, get_database_tables_info - Part of Task 5: Infrastructure Chunk 1
- Match backend Pydantic schemas exactly - Database types: DatabaseCreate, DatabaseResponse, SchemaResponse - Query types: QueryRequest, QueryResponse - Upload, delete, tables, cache response types - Part of Task 6: Infrastructure Chunk 2
- For use in Server Components and Server Actions - Clerk JWT auth integration via auth() helper - Full database CRUD operations - Query execution and cache management - Type-safe with TypeScript API types - Part of Task 7: Infrastructure Chunk 2
- For use in Client Components with 'use client' directive - Token passed explicitly from useAuth hook - Mirrors server-side API with browser runtime - Full database CRUD, query, and cache operations - Part of Task 8: Infrastructure Chunk 2
- Wraps client-side API with automatic Clerk auth - Convenient hook for Client Components - All database, query, and cache operations - Type-safe with useCallback memoization - Part of Task 9: Infrastructure Chunk 2
- Add rewrites to proxy /api/v1/* to backend - Uses BACKEND_URL from environment (http://localhost:8000) - Enables client-side API calls via relative paths - Part of Task 10: Infrastructure Chunk 3
- Renamed frontend/src/proxy.ts → frontend/src/middleware.ts - Exclude /api/v1/* from Clerk middleware (handled by backend) - Allows backend auth to process API requests directly - Part of Task 11: Infrastructure Chunk 3
- File validation for .db, .sqlite, .sql, .csv - Auto-populate display name from filename - Loading states and error handling - Uses useApi hook with Clerk auth - Part of Task 12: Chunk 4
- Removed 'use client' directive - Made page async to fetch data server-side - Uses getDatabases from server API - Passes data and error to DatabasesView - Part of Task 13: Chunk 5
- View button navigates to database detail page - Delete button with confirmation dialog - Shows status, name, description, type, stats - Hover effects and loading states - Part of Task 14: Chunk 5
- Real data from server-side API - Upload modal integration - Delete functionality with optimistic updates - Search/filter functionality - Empty states for no databases or search results - Part of Task 15: Chunk 5
- Removed 'use client' directive - Fetch databases server-side via getDatabases - Pass real data to DashboardHome component - Error handling for API failures - Part of Task 16: Chunk 6
- Accept real database data and compute dynamic stats - Wire quick actions to dashboard routes - Show connected databases from API data - Add error banner for dashboard data load failures - Part of Task 17: Chunk 6
- QueryInterface supports database selection and NL question input - QueryResults renders SQL, metadata, and tabular output - Uses useApi hook for authenticated query execution - Implements Tasks 18-19 in Chunk 7
- Convert /dashboard/chat page to Server Component with database fetch - Replace ChatPage module with QueryInterface-driven UX - Add empty/error states when databases are unavailable - Implements Task 20 in Chunk 7
- Convert overview route to Server Component - Fetch database metadata and schema from backend API - Render stats, metadata, and discovered table list - Implements Task 21 in Chunk 8
- Convert database chat route to Server Component - Fetch selected database and preselect it in QueryInterface - Add route-level error handling for missing database - Implements Task 22 in Chunk 8
- Convert schema route to Server Component - Fetch live schema data from backend API - Render per-table column metadata and constraints - Implements Task 23 in Chunk 9
- Add explicit page-level typings for server-fetched databases - Fix schema constraint rendering token in JSX - Improve api-client header typing for strict TypeScript - Replace loose any types in API contracts with unknown - Keep upload callback lint-safe without behavior changes
… issues - Refine query submit button styling in QueryInterface - Remove effect-based mounted flag in gradient chat input - Use resolvedTheme directly and fix icon/button accessibility details
- Convert db detail index to server-side redirect to overview - Replace static detail layout with real database metadata sidebar - Implement ERD page using live schema relationships - Implement settings page with live database metadata
- Rename middleware.ts to proxy.ts for Next.js 16 file convention - Keep API bypass and dashboard protection logic unchanged
- Add active-state tab navigation for database detail sections - Move tab rendering into client nav component for pathname awareness - Align DatabaseTablesResponse with backend /tables payload
… and layout styling
Owner
Author
|
/opencode |
Owner
Author
There was a problem hiding this comment.
Pull request overview
This PR wires the Next.js dashboard UI to real backend database/query endpoints, adds shared API typing + clients, and introduces optional Clerk JWT verification on the FastAPI side for authenticated requests.
Changes:
- Added typed server/client API clients in the frontend (plus a
useApihook) and updated dashboard pages to fetch real database/schema/query data. - Implemented optional Clerk JWT verification middleware in the backend and added request logging using the decoded user info when present.
- Expanded dashboard UI components for database listing, upload, schema/ERD views, and query execution/results rendering.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/types/api.ts | Adds TS interfaces mirroring backend schemas for DB/query/cache responses. |
| frontend/src/proxy.ts | Clerk middleware hook updated to bypass /api/v1/* proxy routes. |
| frontend/src/modules/dashboard/index.tsx | Dashboard overview now renders real database stats + navigation actions. |
| frontend/src/modules/dashboard/Databases.tsx | Databases page updated to show real DBs, search, upload modal, delete. |
| frontend/src/modules/dashboard/DashboardLayoutClient.tsx | Extracts dashboard layout into a client wrapper with pathname animation keying. |
| frontend/src/modules/dashboard/ChatPage.tsx | Chat page now driven by available databases + error state. |
| frontend/src/modules/dashboard/chat/ChatInput.tsx | Swaps old input for new AiChatInput component. |
| frontend/src/lib/api.ts | Adds server-side API client using Clerk server auth token. |
| frontend/src/lib/api-client.ts | Adds client-side API client that accepts a Clerk token from callers. |
| frontend/src/hooks/use-api.ts | Adds useApi hook to wrap client API calls with Clerk token fetching. |
| frontend/src/components/ui/textarea.tsx | Introduces reusable Textarea UI component. |
| frontend/src/components/ui/popover.tsx | Introduces Radix popover wrapper component. |
| frontend/src/components/ui/gradient-ai-chat-input.tsx | Adds animated gradient chat input component with attachments UI. |
| frontend/src/components/ui/dialog.tsx | Adds Radix dialog wrapper components (shadcn-style). |
| frontend/src/components/ui/command.tsx | Adds command palette components (cmdk + dialog). |
| frontend/src/components/ui/button.tsx | Reworks Button to shadcn-style variants + forwardRef. |
| frontend/src/components/ui/ai-chat-input.tsx | Adds compact chat input with upload + send/stop icon. |
| frontend/src/components/query/QueryResults.tsx | Adds query results stats, SQL rendering, and tabular results display. |
| frontend/src/components/query/QueryInterface.tsx | Adds database selection + question submission + backend query execution. |
| frontend/src/components/database/DatabaseUploadModal.tsx | Adds upload modal with validation and backend upload call. |
| frontend/src/components/database/DatabaseCard.tsx | Adds database card UI with view + delete actions. |
| frontend/src/app/globals.css | Updates base styles and removes global focus-visible styles. |
| frontend/src/app/dashboard/page.tsx | Converts dashboard page to server component fetching DBs via server API client. |
| frontend/src/app/dashboard/layout.tsx | Switches to server layout rendering DashboardLayoutClient and forces dynamic. |
| frontend/src/app/dashboard/databases/page.tsx | Converts databases page to server component fetching DB list. |
| frontend/src/app/dashboard/databases/[dbId]/settings/page.tsx | Implements server-rendered settings view using backend DB fetch. |
| frontend/src/app/dashboard/databases/[dbId]/schema/page.tsx | Implements server-rendered schema table using backend schema fetch. |
| frontend/src/app/dashboard/databases/[dbId]/page.tsx | Redirects to overview via server redirect(). |
| frontend/src/app/dashboard/databases/[dbId]/overview/page.tsx | Implements server-rendered overview using DB + schema fetch. |
| frontend/src/app/dashboard/databases/[dbId]/layout.tsx | Server layout for DB detail pages + new DetailNav. |
| frontend/src/app/dashboard/databases/[dbId]/erd/page.tsx | Implements simple ERD relationships listing from schema foreign keys. |
| frontend/src/app/dashboard/databases/[dbId]/chat/page.tsx | Adds per-database query page with preselected database in QueryInterface. |
| frontend/src/app/dashboard/databases/[dbId]/_components/DetailNav.tsx | Client-side nav that highlights active detail tab. |
| frontend/src/app/dashboard/chat/page.tsx | Converts main chat page to server component fetching DB list. |
| frontend/next.config.ts | Adds rewrite proxying /api/v1/* to BACKEND_URL. |
| frontend/.env.example | Documents BACKEND_URL for frontend. |
| backend/requirements.txt | Adds pyjwt[crypto] dependency for JWT verification. |
| backend/main.py | Moves DB init to FastAPI lifespan + adds root endpoint metadata response. |
| backend/api/schemas/database_schemas.py | Updates schema models (incl. SchemaResponse field rename). |
| backend/api/routers/queries.py | Adds optional auth dependency + logging and minor formatting changes. |
| backend/api/routers/databases.py | Adds optional auth dependency + logging and minor formatting changes. |
| backend/api/middleware/auth.py | Adds Clerk JWKS-based JWT verification helpers and dependencies. |
| backend/api/middleware/init.py | Exposes auth dependencies for import convenience. |
| backend/api/config/settings.py | Adds Clerk JWKS URL + issuer settings. |
| backend/.env.example | Adds Clerk JWKS/issuer example values. |
| .github/workflows/opencode.yml | Adds workflow to run OpenCode on /oc comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+97
to
99
| outline: none !important; | ||
| } | ||
|
|
There was a problem hiding this comment.
The global * rule disables outlines (outline: none !important) and the dedicated *:focus-visible styles were removed, which will eliminate keyboard focus indicators across the app. This is a major accessibility regression; restore focus-visible styling (or provide an equivalent per-component focus ring) and avoid forcing outline: none globally.
Suggested change
| outline: none !important; | |
| } | |
| } | |
| *:focus-visible { | |
| outline: 2px solid var(--ring); | |
| outline-offset: 2px; | |
| } |
Comment on lines
+70
to
+75
| if (!response.ok) { | ||
| const error = await response.json().catch(() => ({ | ||
| detail: response.statusText, | ||
| })); | ||
| throw new Error(error.detail || `API Error: ${response.status}`); | ||
| } |
There was a problem hiding this comment.
apiFetch assumes error.detail is a string, but the API error shape can be an object (your APIError type allows detail: string | Record<...>). Throwing new Error(object) will produce an unhelpful "[object Object]" message; stringify/normalize detail (and consider handling non-JSON error bodies) before throwing.
| <Clock size={14} /> | ||
| <span className="text-[11px] font-medium uppercase tracking-[0.08em]">Execution Time</span> | ||
| </div> | ||
| <div className="text-[20px] font-bold text-[#f0f0f0]">{result.execution_time_ms ?? "N/A"}ms</div> |
There was a problem hiding this comment.
Rendering execution time as {result.execution_time_ms ?? "N/A"}ms will display N/Ams when the value is missing. Consider conditionally appending the ms suffix only when execution_time_ms is a number, otherwise render just N/A.
Suggested change
| <div className="text-[20px] font-bold text-[#f0f0f0]">{result.execution_time_ms ?? "N/A"}ms</div> | |
| <div className="text-[20px] font-bold text-[#f0f0f0]"> | |
| {result.execution_time_ms !== null && result.execution_time_ms !== undefined | |
| ? `${result.execution_time_ms}ms` | |
| : "N/A"} | |
| </div> |
Comment on lines
+110
to
+116
| style={{ | ||
| minHeight: "40px", | ||
| maxHeight: "120px", | ||
| height: "auto", | ||
| outline: "none !important", | ||
| boxShadow: "none !important", | ||
| }} |
There was a problem hiding this comment.
Inline styles don’t support !important. Setting outline: "none !important" / boxShadow: "none !important" results in invalid CSS values that browsers may ignore, leading to inconsistent visuals. Remove !important from inline styles and rely on class-based styles (or a CSS rule) instead.
Comment on lines
+13
to
+21
| export default async function DatabaseDetailLayout({ | ||
| children, | ||
| params, | ||
| }: { | ||
| children: React.ReactNode; | ||
| params: Promise<{ dbId: string }>; | ||
| }) { | ||
| const router = useRouter(); | ||
| const pathname = usePathname(); | ||
| const params = useParams(); | ||
| const dbId = params.dbId; | ||
| const { dbId } = await params; | ||
| const id = parseInt(dbId, 10); |
There was a problem hiding this comment.
In Next.js App Router, params is passed as a plain object, not a Promise. Typing it as Promise<{ dbId: string }> and awaiting it works at runtime (since await on a non-Promise is a no-op), but it’s misleading and defeats Next’s built-in PageProps typing. Prefer params: { dbId: string } and remove the await.
Comment on lines
+6
to
+8
| params: Promise<{ dbId: string }>; | ||
| }) { | ||
| const { dbId } = await params; |
There was a problem hiding this comment.
params is typed as Promise<{ dbId: string }> and awaited, but Next.js App Router passes params as a plain object. This is misleading and bypasses Next’s page prop typings; prefer params: { dbId: string } and remove the await.
Suggested change
| params: Promise<{ dbId: string }>; | |
| }) { | |
| const { dbId } = await params; | |
| params: { dbId: string }; | |
| }) { | |
| const { dbId } = params; |
Comment on lines
+6
to
+8
| params: Promise<{ dbId: string }>; | ||
| }) { | ||
| const { dbId } = await params; |
There was a problem hiding this comment.
params is treated as a Promise and awaited, but in Next.js App Router it’s provided as a plain object. Keeping the Promise type here is misleading; change to params: { dbId: string } and remove the await.
Suggested change
| params: Promise<{ dbId: string }>; | |
| }) { | |
| const { dbId } = await params; | |
| params: { dbId: string }; | |
| }) { | |
| const { dbId } = params; |
Comment on lines
+7
to
+9
| params: Promise<{ dbId: string }>; | ||
| }) { | ||
| const { dbId } = await params; |
There was a problem hiding this comment.
params is typed as a Promise and awaited, but Next.js App Router passes params as a plain object. This makes the component signature inaccurate and can mask type issues; prefer params: { dbId: string } and remove the await.
Suggested change
| params: Promise<{ dbId: string }>; | |
| }) { | |
| const { dbId } = await params; | |
| params: { dbId: string }; | |
| }) { | |
| const { dbId } = params; |
Comment on lines
+16
to
+18
| params: Promise<{ dbId: string }>; | ||
| }) { | ||
| const { dbId } = await params; |
There was a problem hiding this comment.
params is declared as Promise<{ dbId: string }> and awaited, but Next.js App Router passes params as a plain object. This is misleading and defeats Next’s built-in typing; use params: { dbId: string } and drop the await.
Suggested change
| params: Promise<{ dbId: string }>; | |
| }) { | |
| const { dbId } = await params; | |
| params: { dbId: string }; | |
| }) { | |
| const { dbId } = params; |
Comment on lines
1
to
6
| import { clerkMiddleware, createRouteMatcher } from "@clerk/nextjs/server"; | ||
| import { NextResponse } from "next/server"; | ||
|
|
||
| const isProtectedRoute = createRouteMatcher(["/dashboard(.*)"]); | ||
|
|
||
| export default clerkMiddleware(async (auth, req) => { |
There was a problem hiding this comment.
Next.js only executes Middleware from middleware.{ts,js} at the project root or under src/. This file is named src/proxy.ts, and there’s no middleware.ts in the repo, so this Clerk middleware likely never runs (meaning /dashboard may not actually be protected). Rename/move this to frontend/src/middleware.ts (or add a root middleware.ts that re-exports it) so Next picks it up.
Owner
Author
|
/opencode |
Owner
Author
|
/opencode |
|
ProviderModelNotFoundError |
Owner
Author
|
/opencode |
|
ProviderModelNotFoundError |
Owner
Author
|
/claude |
Owner
Author
|
@claude[agent] hey |
Owner
Author
|
@claude[agent] should i merge this ? |
Copilot stopped work on behalf of
SumitPatel-HQ due to an error
March 30, 2026 22:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.