Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a batch processing dashboard with dedicated tabs for monitoring batches and files, along with a new API route for secure file downloads. The changes also include logic to ensure batch chat requests are non-streaming and comprehensive unit tests for the new functionality. Review feedback highlights a regression in routing for image and video endpoints, the need for better filename handling in download headers, significant code duplication across UI components, and opportunities for performance optimization using memoization.
| case "/v1/images/generations": | ||
| case "/v1/images/edits": | ||
| return handleImagesGenerationsRoute(request); | ||
| case "/v1/videos": | ||
| case "/v1/videos/generations": | ||
| return handleVideosGenerationsRoute(request); |
There was a problem hiding this comment.
The switch statement is missing cases for /v1/images/edits and /v1/videos which were present in the previous implementation. This will cause these requests to fall through to the default case and be handled by handleChat, which is incorrect for image and video endpoints.
| case "/v1/images/generations": | |
| case "/v1/images/edits": | |
| return handleImagesGenerationsRoute(request); | |
| case "/v1/videos": | |
| case "/v1/videos/generations": | |
| return handleVideosGenerationsRoute(request); | |
| case "/v1/images/generations": | |
| case "/v1/images/edits": | |
| return handleImagesGenerationsRoute(request); | |
| case "/v1/videos": | |
| case "/v1/videos/generations": | |
| return handleVideosGenerationsRoute(request); |
| return new Response(content as unknown as BodyInit, { | ||
| headers: { | ||
| "Content-Type": file.mimeType || "application/octet-stream", | ||
| "Content-Disposition": `attachment; filename="${filename}"`, |
There was a problem hiding this comment.
The filename in the Content-Disposition header should be properly escaped to handle special characters like double quotes. Additionally, providing the filename* parameter ensures better compatibility with non-ASCII characters across different browsers.
| "Content-Disposition": `attachment; filename="${filename}"`, | |
| "Content-Disposition": `attachment; filename="${filename.replace(/"/g, "")}"; filename*=UTF-8''${encodeURIComponent(filename)}`, |
| function relativeTime(ts: number): string { | ||
| const diffMs = Date.now() - ts * 1000; | ||
| const diffSec = Math.round(diffMs / 1000); | ||
| if (diffSec < 60) return `${diffSec}s ago`; | ||
| const diffMin = Math.round(diffSec / 60); | ||
| if (diffMin < 60) return `${diffMin}m ago`; | ||
| const diffHr = Math.round(diffMin / 60); | ||
| if (diffHr < 24) return `${diffHr}h ago`; | ||
| return `${Math.round(diffHr / 24)}d ago`; | ||
| } | ||
|
|
||
| interface BatchRecord { | ||
| id: string; | ||
| endpoint: string; | ||
| completionWindow: string; | ||
| status: string; | ||
| inputFileId: string; | ||
| outputFileId?: string | null; | ||
| errorFileId?: string | null; | ||
| createdAt: number; | ||
| inProgressAt?: number | null; | ||
| expiresAt?: number | null; | ||
| finalizingAt?: number | null; | ||
| completedAt?: number | null; | ||
| failedAt?: number | null; | ||
| expiredAt?: number | null; | ||
| cancellingAt?: number | null; | ||
| cancelledAt?: number | null; | ||
| requestCountsTotal: number; | ||
| requestCountsCompleted: number; | ||
| requestCountsFailed: number; | ||
| metadata?: Record<string, any> | null; | ||
| errors?: any | null; | ||
| model?: string | null; | ||
| usage?: any | null; | ||
| } | ||
|
|
||
| interface FileRecord { | ||
| id: string; | ||
| filename: string; | ||
| bytes: number; | ||
| purpose: string; | ||
| status?: string | null; | ||
| createdAt: number; | ||
| } | ||
|
|
||
| interface BatchDetailModalProps { | ||
| batch: BatchRecord; | ||
| files: FileRecord[]; | ||
| onClose: () => void; | ||
| } | ||
|
|
||
| const STATUS_STYLES: Record<string, string> = { | ||
| completed: "bg-emerald-500/15 text-emerald-400 border-emerald-500/25", | ||
| completed_with_failures: "bg-red-500/15 text-red-400 border-red-500/25", | ||
| failed: "bg-red-500/15 text-red-400 border-red-500/25", | ||
| in_progress: "bg-blue-500/15 text-blue-400 border-blue-500/25", | ||
| in_progress_with_failures: "bg-orange-500/15 text-orange-400 border-orange-500/25", | ||
| finalizing: "bg-violet-500/15 text-violet-400 border-violet-500/25", | ||
| finalizing_with_failures: "bg-orange-500/15 text-orange-400 border-orange-500/25", | ||
| validating: "bg-yellow-500/15 text-yellow-400 border-yellow-500/25", | ||
| cancelling: "bg-orange-500/15 text-orange-400 border-orange-500/25", | ||
| cancelled: "bg-gray-500/15 text-gray-400 border-gray-500/25", | ||
| cancelled_with_failures: "bg-red-500/15 text-red-400 border-red-500/25", | ||
| expired: "bg-gray-500/15 text-gray-400 border-gray-500/25", | ||
| }; | ||
|
|
||
| const STATUS_LABELS: Record<string, string> = { | ||
| completed_with_failures: "completed with failures", | ||
| in_progress_with_failures: "in progress (with failures)", | ||
| finalizing_with_failures: "finalizing (with failures)", | ||
| cancelled_with_failures: "cancelled with failures", | ||
| }; | ||
|
|
||
| function effectiveStatus(batch: BatchRecord): string { | ||
| const hasFailed = (batch.requestCountsFailed ?? 0) > 0; | ||
| if (!hasFailed) return batch.status; | ||
| const map: Record<string, string> = { | ||
| completed: "completed_with_failures", | ||
| in_progress: "in_progress_with_failures", | ||
| finalizing: "finalizing_with_failures", | ||
| cancelled: "cancelled_with_failures", | ||
| }; | ||
| return map[batch.status] ?? batch.status; | ||
| } | ||
|
|
||
| function StatusBadge({ batch }: { batch: BatchRecord }) { | ||
| const key = effectiveStatus(batch); | ||
| const cls = STATUS_STYLES[key] ?? "bg-gray-500/15 text-gray-400 border-gray-500/25"; | ||
| const label = STATUS_LABELS[key] ?? key.replace(/_/g, " "); | ||
| return ( | ||
| <span className={`inline-block px-2 py-0.5 rounded-md text-xs font-medium border ${cls}`}> | ||
| {label} | ||
| </span> | ||
| ); | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between BatchDetailModal.tsx, BatchListTab.tsx, and FilesListTab.tsx. Utility functions like relativeTime, effectiveStatus, and components like StatusBadge along with their associated styles and interfaces should be moved to a shared utilities or types file to improve maintainability and ensure consistency across the dashboard.
| const [searchQuery, setSearchQuery] = useState(""); | ||
| const [purposeFilter, setPurposeFilter] = useState("all"); | ||
|
|
||
| const purposes = ["all", ...Array.from(new Set(files.map((f) => f.purpose)))]; |
There was a problem hiding this comment.
This calculation runs on every render. Since it depends on the files array, it should be memoized using useMemo to avoid unnecessary computations as the list grows. Note that you will also need to add useMemo to your React imports.
| const purposes = ["all", ...Array.from(new Set(files.map((f) => f.purpose)))]; | |
| const purposes = useMemo(() => ["all", ...Array.from(new Set(files.map((f) => f.purpose)))], [files]); |
| const filtered = batches.filter((b) => { | ||
| if (statusFilter !== "all" && b.status !== statusFilter) return false; | ||
| if (searchQuery) { | ||
| const q = searchQuery.toLowerCase(); | ||
| return ( | ||
| b.id.toLowerCase().includes(q) || | ||
| b.endpoint.toLowerCase().includes(q) || | ||
| (b.model ?? "").toLowerCase().includes(q) | ||
| ); | ||
| } | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
The filtering logic for batches runs on every render. It should be wrapped in useMemo to improve performance, especially when the batches list is large or when the user is actively typing in the search box. Note that you will also need to add useMemo to your React imports.
| const filtered = batches.filter((b) => { | |
| if (statusFilter !== "all" && b.status !== statusFilter) return false; | |
| if (searchQuery) { | |
| const q = searchQuery.toLowerCase(); | |
| return ( | |
| b.id.toLowerCase().includes(q) || | |
| b.endpoint.toLowerCase().includes(q) || | |
| (b.model ?? "").toLowerCase().includes(q) | |
| ); | |
| } | |
| return true; | |
| }); | |
| const filtered = useMemo(() => batches.filter((b) => { | |
| if (statusFilter !== "all" && b.status !== statusFilter) return false; | |
| if (searchQuery) { | |
| const q = searchQuery.toLowerCase(); | |
| return ( | |
| b.id.toLowerCase().includes(q) || | |
| b.endpoint.toLowerCase().includes(q) || | |
| (b.model ?? "").toLowerCase().includes(q) | |
| ); | |
| } | |
| return true; | |
| }), [batches, statusFilter, searchQuery]); |
|
@diegosouzapw heh, code moves fast here 😄 I need to go to bed now 😢 I'll look at the merge conflict and code reviews tomorrow, unless you can pick up my changes? |
|
Thanks @hartmark for this great contribution! 🎉 It's now officially merged into the |
|
Merged manually into release/v3.7.0 via commit ed8ca4b1. Thanks again for the contribution! |
* Added GUI for OpenAI batching * Support downloading files via API * Set stream: false for chat completions in batchProcessor
* Added GUI for OpenAI batching * Support downloading files via API * Set stream: false for chat completions in batchProcessor
* Added GUI for OpenAI batching * Support downloading files via API * Set stream: false for chat completions in batchProcessor
Summary
Adds user-frontend to previous PR
Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Test coverage on affected code