Documents: UI for upload #20
Conversation
📝 WalkthroughWalkthroughAdds serverless API routes proxying document collection and per-document endpoints, introduces a client-side Documents page with upload/list/preview/delete UX, renames "Tasks" → "Capabilities" and adds Documents to the sidebar, and tweaks configs backend URL trailing slash. Changes
Sequence DiagramsequenceDiagram
participant User as User / Browser
participant Client as DocumentPage (Client)
participant APIRoute as API Route (/api/document*)
participant Backend as Backend (Document Service)
User->>Client: Open Documents page
Client->>Client: Read API key from localStorage
Client->>APIRoute: GET /api/document (X-API-KEY)
APIRoute->>Backend: GET /api/v1/documents/ (forward X-API-KEY)
Backend-->>APIRoute: Return document list
APIRoute-->>Client: Return document list
Client->>Client: Render DocumentListing
User->>Client: Select file & upload
Client->>APIRoute: POST /api/document (FormData + X-API-KEY)
APIRoute->>Backend: POST /api/v1/documents/ (forward FormData & X-API-KEY)
Backend-->>APIRoute: Return created document
APIRoute-->>Client: Return created document
Client->>Client: Refresh list
User->>Client: Click document to preview
Client->>APIRoute: GET /api/document/{id} (X-API-KEY)
APIRoute->>Backend: GET /api/v1/documents/{id}?include_url=true
Backend-->>APIRoute: Return document with signed_url
APIRoute-->>Client: Return document details
Client->>Client: Render DocumentPreview
User->>Client: Confirm delete
Client->>APIRoute: DELETE /api/document/{id} (X-API-KEY)
APIRoute->>Backend: DELETE /api/v1/documents/{id} (forward X-API-KEY)
Backend-->>APIRoute: Deletion confirmation
APIRoute-->>Client: Return status
Client->>Client: Refresh list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/api/document/`[document_id]/route.ts:
- Around line 12-20: The GET handler currently always returns 200 by calling
NextResponse.json(data) regardless of the backend fetch status; change it to
propagate the backend response status by using NextResponse.json(data, { status:
response.status }) (or equivalent) so client-side code receives 4xx/5xx
statuses; locate the fetch call and the return that uses NextResponse.json in
the GET route (reference symbols: response, data, NextResponse.json) and update
the return to include the backend response.status.
- Around line 43-57: The fetch DELETE handler calls response.json()
unconditionally which throws on 204/empty bodies; update the logic around the
fetch response (the response variable in route.ts) to first check for
response.status === 204 or no body (e.g., check Content-Length or Content-Type)
and only call response.json() when a body exists—otherwise set data to null or
an empty object and return the appropriate NextResponse.json(data, { status:
response.status }) path so 204/empty responses are handled without a SyntaxError
and the status is preserved; keep using NextResponse.json to return the payload
or empty result.
In `@app/api/document/route.ts`:
- Around line 8-17: The GET handler currently always returns 200; update the
`fetch(\`\${backendUrl}/api/v1/documents/\`)` handling so that when you forward
the backend payload you use `NextResponse.json(data, { status: response.status
})` and when `!response.ok` return the backend error body with
`response.status`; ensure the catch block returns an appropriate error status
(e.g., 500) and error message. Do the same fix for the POST handler where the
success branch currently hardcodes 200—use `NextResponse.json(data, { status:
response.status })` instead to forward the backend status.
🧹 Nitpick comments (2)
app/api/document/route.ts (1)
53-61: Preserve backend success status. Returning 200 masks 201/202 from the upstream; keepingresponse.statusmakes the proxy transparent.♻️ Suggested tweak
- return NextResponse.json(data, { status: 200 }); + return NextResponse.json(data, { status: response.status });app/document/page.tsx (1)
355-366: Avoid hard-coded IST offset in date formatting. Adding +5.5h bakes in a single timezone and can double‑offset if timestamps already include TZ data. ConsiderIntl.DateTimeFormatwith a configured timeZone (if you want IST) or localtoLocaleString, and reuse one helper across both components.Also applies to: 575-586
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/api/configs/route.ts (3)
16-20: TypeScript error:erroris of typeunknown.In the catch block,
errorhas typeunknownin TypeScript strict mode. Accessingerror.messagedirectly will cause a type error.Proposed fix
} catch (error) { return NextResponse.json( - { success: false, error: error.message, data: null }, + { success: false, error: error instanceof Error ? error.message : 'Unknown error', data: null }, { status: 500 } ); }
54-59: Same TypeScript error:erroris of typeunknown.Same issue as the GET handler -
error.messageneeds type narrowing.Proposed fix
} catch (error) { console.error('Proxy error:', error); return NextResponse.json( - {error:'Failed to forward request', details:error.message}, + {error:'Failed to forward request', details: error instanceof Error ? error.message : 'Unknown error'}, {status:500} ); }
14-15: Missingresponse.okcheck before parsing JSON.The GET handler parses the response as JSON without checking if the request succeeded. If the backend returns a non-2xx status, this could lead to unexpected behavior or obscure errors.
Proposed fix
const response = await fetch(`${backendUrl}/api/v1/configs/`, { headers: { 'X-API-KEY': apiKey || '', }, }); + if (!response.ok) { + const errorData = await response.json().catch(() => ({})); + return NextResponse.json( + { success: false, error: errorData.error || `Backend returned ${response.status}`, data: null }, + { status: response.status } + ); + } + const data = await response.json(); return NextResponse.json(data);
🤖 Fix all issues with AI agents
In `@app/document/page.tsx`:
- Around line 686-701: The onError handler on the <img> is directly touching the
DOM via parent.innerHTML which risks XSS and breaks React's VDOM; change this to
use React state in the DocumentPreview component: add a boolean state (e.g.,
imageError, setImageError), update the onError to log the error and call
setImageError(true) (reference document.signed_url in the log), and
conditionally render either the <img> (when !imageError) or a safe fallback
<div> message (when imageError) instead of manipulating innerHTML.
- Around line 53-58: The useEffect calls fetchDocuments but doesn’t include it
in the dependency array, risking stale closures; fix by wrapping fetchDocuments
in useCallback (exported/defined in the same component) with its own
dependencies (e.g., apiKey, any state setters) and then include fetchDocuments
in the useEffect dependency array, or alternatively move the fetch logic
directly into the useEffect so only apiKey is required—ensure the unique
function name fetchDocuments and the useEffect that depends on apiKey are
updated accordingly.
- Around line 105-149: The handleUpload function builds FormData but never
includes the documentName field; update handleUpload to append documentName to
the FormData (e.g., formData.append('name' or 'documentName', documentName))
before the fetch so the backend receives the user-provided name, ensuring you
reference the existing formData and documentName variables and keep the rest of
the upload flow (response handling, fetchDocuments, toast calls) unchanged.
🧹 Nitpick comments (4)
app/components/Sidebar.tsx (1)
32-37: Minor:'Documents': trueinexpandedMenushas no effect.The Documents menu item (line 75) is a leaf node without a submenu, so tracking its expanded state is unnecessary. This doesn't cause any issues but adds confusion.
Proposed cleanup
const [expandedMenus, setExpandedMenus] = useState<Record<string, boolean>>({ 'Capabilities': true, 'Evaluations': true, - 'Documents': true, 'Configurations': false, });app/document/page.tsx (3)
4-6: Use@/path alias for imports instead of relative paths.Based on learnings, the codebase convention is to use
@/path aliases for imports.Proposed fix
-import { APIKey, STORAGE_KEY } from '../keystore/page'; -import Sidebar from '../components/Sidebar'; -import { useToast } from '../components/Toast'; +import { APIKey, STORAGE_KEY } from '@/app/keystore/page'; +import Sidebar from '@/app/components/Sidebar'; +import { useToast } from '@/app/components/Toast';
83-86: Remove debugconsole.logstatements before merging.Multiple debug logs are present throughout the file (lines 83-85, 130, 651, 700-701, 708, 718). These should be removed for production code.
Also applies to: 130-130, 651-651
355-369: DuplicateformatDatefunction - consider reusing fromutils.ts.The
formatDatefunction is defined twice in this file (lines 355-369 and 575-590), and a similar function already exists inapp/components/utils.ts. Consider importing and reusing the utility function to maintain DRY principles.Proposed refactor
+import { formatDate } from '@/app/components/utils'; // Then remove the local formatDate definitions in DocumentListing and DocumentPreview // and use the imported formatDate directlyNote: The existing
formatDatein utils.ts usestoLocaleString('en-GB')while this implementation formats manually. You may need to align the format or create a shared utility with the desired format.Also applies to: 575-590
| currentPage, | ||
| onPageChange, | ||
| }: DocumentListingProps) { | ||
| const formatDate = (dateString?: string) => { |
There was a problem hiding this comment.
@Prajna1999 Why don't we make this the util function for format date which shows the datetime like this - "28 Jan 2024, 14:30",
instead of what we have in the util right now which shows the datetiem like this - "28/01/2024, 2:30:00 pm"
New Features -
Some thing to note down -
For now, this is how it looks -