Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughComponentizes the knowledge-base UI via a new useCollections hook, adds multi-file document upload batching with per-file progress and modal deletion, introduces LocalStorage-backed job cache and enrichment utilities, adds skeletons and a FileExtBadge/DownloadIcon, and updates lint/config. ChangesKnowledge Base & Document Management Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eslint.config.mjs (1)
1-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve formatting drift to clear CI.
CI reports
prettier --checkfailures on this file. Please run Prettier oneslint.config.mjsso lint-and-build is clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eslint.config.mjs` around lines 1 - 58, Run Prettier to fix formatting drift in eslint.config.mjs; specifically format the file that defines the eslintConfig constant (created via defineConfig and using globalIgnores) so it passes `prettier --check` in CI — you can run your project's configured Prettier command or an editor/IDE formatter to save the file with Prettier rules and commit the formatted eslintConfig export.
🧹 Nitpick comments (13)
app/(main)/evaluations/[id]/page.tsx (1)
254-255: ⚡ Quick winConsider refactoring the long class string for better readability.
The
segmentedClassstring is over 350 characters long and difficult to read/maintain. Consider using a utility function withclsxor breaking it into logical groups:♻️ Suggested refactor using template literals
const segmentedClass = - "inline-flex items-center gap-1.5 px-3 py-1.5 text-xs font-medium transition-all cursor-pointer border border-border bg-bg-primary text-text-primary shadow-[0_1px_2px_rgba(0,0,0,0.04)] hover:bg-neutral-50 hover:shadow-[0_1px_2px_rgba(0,0,0,0.08)] data-[selected=true]:bg-accent-primary data-[selected=true]:border-accent-primary data-[selected=true]:text-white data-[selected=true]:shadow-[0_1px_2px_rgba(0,0,0,0.12)] data-[selected=true]:hover:bg-accent-hover data-[selected=true]:hover:shadow-[0_1px_2px_rgba(0,0,0,0.12)]"; + `inline-flex items-center gap-1.5 px-3 py-1.5 text-xs font-medium transition-all cursor-pointer + border border-border bg-bg-primary text-text-primary shadow-[0_1px_2px_rgba(0,0,0,0.04)] + hover:bg-neutral-50 hover:shadow-[0_1px_2px_rgba(0,0,0,0.08)] + data-[selected=true]:bg-accent-primary data-[selected=true]:border-accent-primary + data-[selected=true]:text-white data-[selected=true]:shadow-[0_1px_2px_rgba(0,0,0,0.12)] + data-[selected=true]:hover:bg-accent-hover data-[selected=true]:hover:shadow-[0_1px_2px_rgba(0,0,0,0.12)]`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/evaluations/[id]/page.tsx around lines 254 - 255, The long inline string assigned to segmentedClass is hard to read; refactor by splitting it into named logical groups (e.g., base, border, stateSelected, hover) and compose them with a class-combiner like clsx (import clsx) or join an array/template literal to build segmentedClass; locate the segmentedClass constant and replace the single 350+ char string with grouped constants (e.g., const base = "...", const selected = "...") and then set segmentedClass = clsx(base, selectedCondition && selected) or segmentedClass = [base, selectedCondition ? selected : null].filter(Boolean).join(" ") to improve readability and maintainability.app/hooks/useCollections.ts (2)
126-137: 💤 Low valueAvoid using a state setter as a synchronous read.
Lines 126–133 invoke
setSelectedCollection((prev) => …)solely to readprevand write to the localreplacementId, then returnprevunchanged. This is fragile: in React Strict Mode the updater can be called twice, which would causereplacementIdto be assigned twice (still correct here, but easy to break in the future).Mirror
selectedCollectionto a ref (similar toapiKeyRef) and read from it directly, or restructure so the replacement decision happens aftersetCollectionsresolves via an effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/hooks/useCollections.ts` around lines 126 - 137, The code uses setSelectedCollection(prev => ...) purely to read prev and set replacementId, which is unsafe in Strict Mode; instead read selectedCollection directly (or mirror it to a ref like apiKeyRef) and compute replacementId without using the state setter, then call fetchCollectionDetails(replacementId) if present; specifically update the logic around setSelectedCollection / selectedCollection / replacementId so you do not rely on the updater to synchronously read state (or move this replacement logic into an effect that runs after setCollections completes).
45-88: ⚡ Quick win
fetchCollectionDetailsanddeleteCollectionclose overcollections, causing identity churn.Both callbacks list
collectionsin their dep arrays so they're rebuilt on every collections update. BecausefetchCollectionsdepends onfetchCollectionDetails, every refresh recreates the entire chain and any effect that depends onfetchCollectionsre-runs unnecessarily.Inside
fetchCollectionDetailsthe only use ofcollectionsis the lookup at Line 51 — this can be replaced with a functionalsetSelectedCollectionthat capturesprevfrom the page state, or by reading from acollectionsRef.deleteCollection's use ofcollections.find(...)fororiginalCollectioncan be similarly relocated inside a functional update.Refactoring to drop
collectionsfrom the dep arrays prevents needless re-renders/re-fetches in downstream effects.Also applies to: 318-392
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/hooks/useCollections.ts` around lines 45 - 88, fetchCollectionDetails and deleteCollection are closing over the collections array causing identity churn; remove collections from their dependency arrays and move any direct reads of collections into functional state updaters or a stable ref. Specifically, in fetchCollectionDetails (function fetchCollectionDetails) replace the collections.find(...) lookup by using setSelectedCollection(prev => (prev if matching) || prev) or read from a collectionsRef.current that you initialize and keep in sync, then remove collections from the useCallback deps; do the same in deleteCollection by moving originalCollection = collections.find(...) into a functional updater or reading from collectionsRef.current before removing collections from its deps. Ensure you update or create a collectionsRef and keep it updated whenever collections changes so linting passes and behavior remains unchanged.app/lib/utils/knowledgeBaseCache.ts (1)
75-100: ⚡ Quick winCache entries without
collection_idare never pruned.
pruneStaleCacheonly removes entries that have acollection_idbut no longer match a live collection. Entries that never resolved (e.g., a create job that failed silently or whose job status never returned acollectionId) will sit inlocalStorageforever, andgetEntriesWithoutCollectionIdwill keep re-scanning them on every enrichment pass.Consider adding a
created_attimestamp toCacheEntryand pruning entries older than some TTL (e.g., 24h) insidepruneStaleCache, or pruning unmatched orphans afterpreFetchJobStatusescompletes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/utils/knowledgeBaseCache.ts` around lines 75 - 100, pruneStaleCache currently only removes entries whose collection_id no longer exists leaving orphaned entries without collection_id to accumulate; update pruneStaleCache (which calls readCache/writeCache) to also inspect entries missing data.collection_id and delete them if their created_at timestamp is older than a TTL (e.g., 24h), adding created_at to your CacheEntry when entries are created; alternatively, after preFetchJobStatuses completes remove unmatched orphans by jobId — ensure you reference CacheEntry, pruneStaleCache, getEntriesWithoutCollectionId, readCache and writeCache when implementing the TTL-based deletion so orphaned entries are pruned automatically.app/lib/utils/collectionEnrichment.ts (2)
65-79: ⚡ Quick winRedundant nested loop in
preFetchJobStatuses.The inner
for (const entry of getEntriesWithoutCollectionId())does not depend on the outercollection, so it re-reads LocalStorage and re-adds the same set ofjobIds for every collection that lacks a cachedjob_id. TheSetdeduplicates the writes, but the work is still O(N·M) and readslocalStorageonce per collection.Hoist the orphan lookup out of the loop:
♻️ Proposed fix
- const jobIdsToFetch = new Set<string>(); - collections.forEach((collection) => { - const cached = getCollectionDataByCollectionId(collection.id); - if (!cached.job_id) { - for (const entry of getEntriesWithoutCollectionId()) { - jobIdsToFetch.add(entry.jobId); - } - } - }); + const hasUncachedCollection = collections.some( + (collection) => !getCollectionDataByCollectionId(collection.id).job_id, + ); + const jobIdsToFetch = new Set<string>( + hasUncachedCollection + ? getEntriesWithoutCollectionId().map((e) => e.jobId) + : [], + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/utils/collectionEnrichment.ts` around lines 65 - 79, preFetchJobStatuses currently calls getEntriesWithoutCollectionId() inside the collections.forEach loop causing repeated localStorage reads and O(N·M) work; move the orphan lookup outside the loop so you first call getEntriesWithoutCollectionId() once, iterate its results to populate jobIdsToFetch (using the Set), then iterate collections to check getCollectionDataByCollectionId(collection.id) only to decide whether to skip or not; update references to jobIdsToFetch, getEntriesWithoutCollectionId, and getCollectionDataByCollectionId in preFetchJobStatuses accordingly.
81-108: 💤 Low valueAdd a typed shape for the intermediate result and consolidate with
fetchJobStatus.The mapper in
preFetchJobStatusesand the body offetchJobStatusboth callapiFetchagainst/api/collections/jobs/${jobId}with the same{ data?: JobStatusData } & JobStatusDatashape and the samejobData.collection?.id || jobData.collection_id || nullnormalization. Extract a small private helper (e.g.,normalizeJobStatus(jobId, apiKey)) and reuse it from both call sites to keep response handling in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/utils/collectionEnrichment.ts` around lines 81 - 108, preFetchJobStatuses and fetchJobStatus duplicate the same apiFetch call and response normalization; extract a small helper (e.g., normalizeJobStatus(jobId, apiKey)) that performs the apiFetch<{ data?: JobStatusData } & JobStatusData>(`/api/collections/jobs/${jobId}`, apiKey), unwraps result.data || result, normalizes collection id via jobData.collection?.id || jobData.collection_id || null, and returns a typed intermediate shape { jobId, status: string | null, collectionId: string | null }; replace the mapper in preFetchJobStatuses and the body of fetchJobStatus to call normalizeJobStatus and use its returned shape to build the JobStatusMap or single JobStatus respectively, preserving existing error handling.app/components/knowledge-base/CollectionDetail.tsx (1)
113-120: ⚡ Quick winPreview button is hardcoded to first document.
The "Preview" header button always opens
documents[0]regardless of which document the user might want to preview. If users have many documents in a collection, they cannot preview anything other than the first one from this entry point. Consider letting users click any row in the documents list to preview, or remove the misleading single "Preview" button.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/knowledge-base/CollectionDetail.tsx` around lines 113 - 120, The Preview button currently always calls onPreviewDocument(documents[0]) which wrongly previews only the first document; change the behavior to preview the currently selected document (or remove the misleading button). Implement a selection state (e.g., selectedDocumentId or selectedDocument) inside CollectionDetail and update document rows to set that selection on click, then update the header button to call onPreviewDocument(selectedDocument) (or disable/hide the button when no selection). Update references to documents and onPreviewDocument in the CollectionDetail component to use the new selection state so the preview opens the intended document.app/(main)/document/page.tsx (4)
153-157: ⚡ Quick winSequential upload loop prevents concurrent uploads.
The current implementation uploads files one at a time in sequence. While this makes progress tracking simpler, it increases total upload time for multiple files. If the sequential approach is intentional (e.g., to avoid overwhelming the server or to simplify progress UI), consider adding a comment explaining the choice.
If parallelization is acceptable, you could use
Promise.allSettledto upload concurrently while still tracking individual results.⚡ Alternative: Parallel uploads with result tracking
-let successCount = 0; -for (let i = 0; i < selectedFiles.length; i += 1) { - setCurrentUploadIndex(i); - const ok = await uploadOneFile(selectedFiles[i]); - if (ok) successCount += 1; -} +const results = await Promise.allSettled( + selectedFiles.map((file, i) => { + setCurrentUploadIndex(i); + return uploadOneFile(file); + }) +); +const successCount = results.filter(r => r.status === 'fulfilled' && r.value).length;Note: This would require rethinking progress tracking since multiple uploads would run concurrently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/document/page.tsx around lines 153 - 157, Current sequential loop over selectedFiles (using setCurrentUploadIndex, uploadOneFile, successCount) serializes uploads and slows overall throughput; either add a clarifying comment explaining that sequential uploads are intentional, or convert to concurrent uploads by mapping selectedFiles to promises that call uploadOneFile and using Promise.allSettled to await them, then compute successCount from fulfilled results; if you switch to concurrency, remove or adjust setCurrentUploadIndex usage (it no longer represents a single active index) and instead track per-file progress/state so UI updates remain correct.
183-205: ⚡ Quick winConsider adding loading state for delete operation.
The delete operation is asynchronous but doesn't show a loading indicator. If the network is slow, users may click multiple times or perceive the UI as unresponsive. Consider adding a
isDeletingDocumentstate and disabling the confirm button while deleting.⏳ Suggested loading state pattern
+const [isDeletingDocument, setIsDeletingDocument] = useState(false); const handleConfirmDelete = async () => { if (!documentToDelete) return; const documentId = documentToDelete.id; setDocumentToDelete(null); + setIsDeletingDocument(true); try { await apiFetch(`/api/document/${documentId}`, apiKey?.key ?? "", { method: "DELETE", }); // ... rest of logic } catch (error) { // ... error handling + } finally { + setIsDeletingDocument(false); } };Then pass
isDeletingDocumenttoDeleteDocumentModalto disable the confirm button.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/document/page.tsx around lines 183 - 205, Add a deletion loading state and disable the confirm button while the async delete runs: introduce a new boolean state (e.g., isDeletingDocument with setIsDeletingDocument) in the component, set it true at the start of handleConfirmDelete and false in both success and catch blocks, pass isDeletingDocument down to DeleteDocumentModal (or use it to disable the confirm button where that modal is rendered), and ensure early returns or UI updates (like setDocumentToDelete(null), setSelectedDocument(null), refetch(), toast.*) still occur while preventing duplicate requests.
66-98: ⚖️ Poor tradeoffConsider extracting file selection logic to a custom hook.
This file selection handler includes batch limit enforcement, size validation, and toast notifications—multiple responsibilities that make the page component harder to test and maintain. Consider extracting this logic into a custom hook like
useDocumentFileSelectioninapp/hooks/.🎣 Example extraction pattern
// app/hooks/useDocumentFileSelection.ts export function useDocumentFileSelection( selectedFiles: File[], setSelectedFiles: (files: File[]) => void ) { const toast = useToast(); const handleFileSelect = (event: React.ChangeEvent<HTMLInputElement>) => { // ... existing logic ... }; return { handleFileSelect }; }Based on learnings: Apply Single Responsibility Principle (SRP): Each component, hook, function, or module should do one thing and have one reason to change. Extract data fetching into hooks, business logic into utilities, and keep components focused on presentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/document/page.tsx around lines 66 - 98, Extract the file selection logic in handleFileSelect into a new custom hook (e.g., useDocumentFileSelection) and move validation and toast side-effects there: create useDocumentFileSelection(selectedFiles, setSelectedFiles) that encapsulates batch limit enforcement using MAX_DOCUMENT_UPLOAD_BATCH, size checks against MAX_DOCUMENT_SIZE_BYTES / MAX_DOCUMENT_SIZE_MB, oversized counting, and toast.error/toast.warning notifications, and return a handleFileSelect function to be used by the page component so the page keeps only presentation/state hookup (calling setSelectedFiles via the hook) while all business logic and notifications live in the hook.
273-289: 💤 Low valueMobile preview modal has hardcoded height.
The preview modal uses
h-[80vh]which is an arbitrary CSS value. This might cause layout issues on very small screens or interfere with the modal'smaxHeight="max-h-[90vh]"prop.Consider either:
- Removing the inner height and letting the modal manage it
- Using a Tailwind utility class like
h-fullto fill available space📱 Suggested adjustment
-<div className="h-[80vh]"> +<div className="h-full"> <DocumentPreview document={selectedDocument} isLoading={isLoadingDocument} /> </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/document/page.tsx around lines 273 - 289, The mobile preview modal currently forces a fixed inner height via the h-[80vh] wrapper which can conflict with the Modal's maxHeight prop and break on small screens; update the mobile block that renders <Modal> (where open uses selectedDocument/isLoadingDocument, onClose calls setSelectedDocument, and it renders <DocumentPreview document={selectedDocument} isLoading={isLoadingDocument}>) to remove the hardcoded h-[80vh] wrapper and instead allow the Modal to size the content (e.g., remove the div or replace its class with a flexible utility such as h-full), ensuring the preview fills available modal space without fixed viewport height.app/components/icons/common/DownloadIcon.tsx (1)
7-15: ⚡ Quick winAdd explicit width and height to the SVG.
The SVG element relies entirely on the
classNameprop for sizing. If a consumer forgets to pass dimensions viaclassName, the icon may not render at a visible size or may have unexpected layout behavior.📐 Proposed fix to add default dimensions
<svg className={className} + width="24" + height="24" fill="none" viewBox="0 0 24 24" stroke="currentColor" strokeWidth={2} strokeLinecap="round" strokeLinejoin="round" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/icons/common/DownloadIcon.tsx` around lines 7 - 15, The SVG in the DownloadIcon component relies solely on the className prop for sizing; add explicit width and height attributes to the <svg> element (e.g., width="24" and height="24") so the icon has sensible default dimensions when className is omitted, and keep className applied so consumers can still override sizing via CSS or a size prop if present; update the <svg> tag (the element using className, fill, viewBox, stroke, strokeWidth, strokeLinecap, strokeLinejoin) to include these default width/height attributes.app/components/document/DocumentPreview.tsx (1)
102-115: ⚡ Quick winThe conditional
downloadattribute logic is correct but subtle.The spread operator usage
{...(condition ? {} : { download: document.fname })}conditionally adds thedownloadattribute only for non-previewable files. This means:
- Previewable files (images, PDFs) open in a new tab (no download attribute)
- Non-previewable files trigger a download prompt
This is good UX, but the pattern might be unclear to future maintainers. Consider adding a brief comment explaining the intent.
💬 Suggested clarification comment
{(document.signed_url || document.object_store_url) && ( <a href={document.signed_url || document.object_store_url} + // For previewable files: open in new tab; for others: trigger download {...(isPreviewable(document.fname) ? {} : { download: document.fname })} target="_blank" rel="noopener noreferrer" className="shrink-0 inline-flex items-center gap-2 px-4 py-1.5 text-sm font-medium transition-colors bg-accent-primary text-white hover:bg-accent-hover rounded-full" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/document/DocumentPreview.tsx` around lines 102 - 115, Add a short inline comment above the anchor rendering that explains the conditional spread: when isPreviewable(document.fname) is true we intentionally omit the download attribute so previewable files (images/PDFs) open in a new tab, and when false we add download: document.fname to force a download for non-previewable files; reference the anchor that uses document.signed_url || document.object_store_url, the isPreviewable(document.fname) check, and the conditional spread `{...(isPreviewable(document.fname) ? {} : { download: document.fname })}` so future maintainers understand the UX intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/`(main)/document/page.tsx:
- Around line 125-132: Current code uses localStorage to persist file sizes
(fileSizeMap, document_file_sizes, data.data.id, file.size), which is fragile;
instead, remove the localStorage workaround and either surface "N/A" for missing
file_size or raise a backend task to return file_size. Update the logic in
page.tsx where fileSizeMap is read/written: stop reading/writing
document_file_sizes, read file_size from the document response (e.g.,
data.data.file_size) and if it is null/undefined display "N/A" in the UI (or add
a user-visible indicator), and create a follow-up issue requesting the backend
to include file_size in document responses so syncing and persistence are
handled server-side.
In `@app/`(main)/knowledge-base/page.tsx:
- Around line 240-248: The open prop currently uses
!!selectedCollection?.documents which is true for empty arrays; change the
condition to check the array length (e.g., showDocPreviewModal &&
(selectedCollection?.documents?.length ?? 0) > 0 or
!!selectedCollection?.documents?.length) so the modal only opens when documents
exist; update the JSX open expression and ensure any places that call
setShowDocPreviewModal(false) (used when closing) remain unchanged so empty
collections do not trigger the preview.
In `@app/components/document/DocumentListing.tsx`:
- Around line 79-113: The delete <span> is nested inside a <button>, blocking
keyboard access; refactor so selection and delete are two separate native
<button>s (e.g., wrap each document row in a container div keyed by doc.id,
render a primary selection <button> that calls onSelect(doc) and applies the
existing selection styling/heading/formatDate/FileExtBadge, and render a sibling
delete <button> that calls e.stopPropagation() then onDelete(doc.id) and keeps
the TrashIcon, title/aria-label and visual classes), ensure the delete button is
focusable and keyboard-activatable, remove the nested interactive element inside
the selection button, and preserve existing class names and conditional styles
used in this component (references: onSelect, onDelete, FileExtBadge,
formatDate, TrashIcon, doc.fname, doc.id).
In `@app/components/document/UploadDocumentModal.tsx`:
- Around line 101-110: The file input currently keeps its value so re-selecting
the same file won't re-trigger onChange; update the handler to clear the input
after processing by either resetting the DOM input's value (use
fileInputRef.current && (fileInputRef.current.value = '')) or resetting
event.target.value = '' at the end of onFileSelect, ensuring you null-check
fileInputRef and do this only after successful handling so the same file can be
picked again; references: fileInputRef, onFileSelect, id="document-file-upload",
ACCEPTED_DOCUMENT_TYPES, isUploading, limitReached.
In `@app/components/FileExtBadge.tsx`:
- Around line 25-29: getFileExtension currently returns an empty string for
filenames that end with a dot (e.g., "name."), causing a blank badge; update the
function (getFileExtension) to treat a trailing dot as no extension by checking
if dot === -1 OR dot === fileName.length - 1 OR the sliced extension is an empty
string, and in those cases return "FILE" instead of the empty string so the
badge falls back to "FILE".
In `@app/components/knowledge-base/CollectionsList.tsx`:
- Around line 56-94: The delete control is currently a non-interactive <span>
nested inside the row <button>, which breaks semantics and keyboard access;
extract the delete control out of the row button and render it as its own native
<button type="button"> (respecting the isOptimistic guard) rather than a <span>,
keep the existing onClick handler that calls onRequestDelete(collection.id) and
call e.stopPropagation() at the start of that handler so the row
onSelect(collection.id) doesn't fire, preserve the current className,
title/aria-label for accessibility, and ensure the row still uses onSelect for
the main button while the new delete button is a sibling element so keyboard
users can tab to and activate it.
In `@app/components/knowledge-base/DocumentPickerModal.tsx`:
- Around line 27-33: The search state (search/setSearch) persists between modal
opens causing stale filteredDocuments results; add a useEffect that watches the
modal visibility prop (e.g., open or isOpen) and call setSearch("") on close (or
on every open) to clear the query and reset filteredDocuments; update
DocumentPickerModal.tsx to reset search in response to the visibility prop
change so availableDocuments filtering is fresh each time the modal opens.
In `@app/components/knowledge-base/DocumentPreviewModal.tsx`:
- Around line 54-59: The iframe rendering previewDoc.signed_url in
DocumentPreviewModal is unsandboxed and risky; update the iframe in the
component to harden it by (1) adding a restrictive sandbox attribute (e.g., no
allow-scripts, only add specific allowances like allow-popups or allow-forms if
absolutely needed), (2) add referrerPolicy="no-referrer" and loading="lazy" to
reduce information leakage and resource impact, and (3) validate/sanitize
previewDoc.signed_url before use (ensure it's an HTTPS URL and optionally check
allowed host/origin) so only trusted signed URLs are rendered.
In `@app/hooks/useCollections.ts`:
- Around line 236-240: In the useCollections hook replace all blocking
alert(...) calls with the project's toast API: call const toast = useToast() at
the top of the hook (inside useCollections) and change the alert("Please log in
to continue") to toast.warning(...) or toast.error(...), change alert("Please
provide a name and select at least one document") to toast.warning(...), and
likewise replace alerts for create-failure, delete-status-failure, and
delete-failure with toast.error(...) (and use toast.success(...) where success
messages exist). Update every occurrence referenced (around the check in the
submit/create handler near the name/documentIds validation, and the other
reported spots at lines ~299-304, ~366-368, ~377-378, ~386-388) so the hook uses
toast methods instead of alert.
- Around line 411-420: The first useEffect's dependency array is wrong: it uses
isAuthenticated, fetchCollections, and fetchDocuments but only lists apiKey,
which causes stale closures and lint failures; change its deps to include
isAuthenticated, fetchCollections, and fetchDocuments (or include apiKey as well
if the effect needs to re-run on key change). Also fix the second effect to only
depend on apiKey (remove isAuthenticated) since it only sets apiKeyRef.current;
keep references to the functions fetchCollections and fetchDocuments (and ensure
they are properly memoized) so the effect re-runs with the latest callbacks.
- Around line 360-389: The polling function pollDeleteStatus schedules itself
with setTimeout and can continue after the component unmounts; fix this by
storing the timeout id in a ref (e.g., deletePollTimeoutRef) whenever you call
setTimeout in pollDeleteStatus and clearing that timeout in the existing cleanup
effect (and also when the job finishes or fails). Additionally guard state
updates inside pollDeleteStatus (setCollections, setSelectedCollection) by
checking a mounted flag or that apiKeyRef.current still exists before calling
them, and ensure any pending timeout is cleared on success/failure paths as well
as on unmount.
In `@app/lib/utils/knowledgeBaseCache.ts`:
- Around line 37-46: saveCollectionData currently overwrites an existing cache
entry's collection_id with undefined when called without collectionId; update
saveCollectionData to read the existing entry via readCache(), preserve its
collection_id when the optional collectionId parameter is undefined (i.e., only
set collection_id when collectionId is provided), and then write the merged
entry back with writeCache(); refer to saveCollectionData, readCache, writeCache
and the collection_id field when making the change.
---
Outside diff comments:
In `@eslint.config.mjs`:
- Around line 1-58: Run Prettier to fix formatting drift in eslint.config.mjs;
specifically format the file that defines the eslintConfig constant (created via
defineConfig and using globalIgnores) so it passes `prettier --check` in CI —
you can run your project's configured Prettier command or an editor/IDE
formatter to save the file with Prettier rules and commit the formatted
eslintConfig export.
---
Nitpick comments:
In `@app/`(main)/document/page.tsx:
- Around line 153-157: Current sequential loop over selectedFiles (using
setCurrentUploadIndex, uploadOneFile, successCount) serializes uploads and slows
overall throughput; either add a clarifying comment explaining that sequential
uploads are intentional, or convert to concurrent uploads by mapping
selectedFiles to promises that call uploadOneFile and using Promise.allSettled
to await them, then compute successCount from fulfilled results; if you switch
to concurrency, remove or adjust setCurrentUploadIndex usage (it no longer
represents a single active index) and instead track per-file progress/state so
UI updates remain correct.
- Around line 183-205: Add a deletion loading state and disable the confirm
button while the async delete runs: introduce a new boolean state (e.g.,
isDeletingDocument with setIsDeletingDocument) in the component, set it true at
the start of handleConfirmDelete and false in both success and catch blocks,
pass isDeletingDocument down to DeleteDocumentModal (or use it to disable the
confirm button where that modal is rendered), and ensure early returns or UI
updates (like setDocumentToDelete(null), setSelectedDocument(null), refetch(),
toast.*) still occur while preventing duplicate requests.
- Around line 66-98: Extract the file selection logic in handleFileSelect into a
new custom hook (e.g., useDocumentFileSelection) and move validation and toast
side-effects there: create useDocumentFileSelection(selectedFiles,
setSelectedFiles) that encapsulates batch limit enforcement using
MAX_DOCUMENT_UPLOAD_BATCH, size checks against MAX_DOCUMENT_SIZE_BYTES /
MAX_DOCUMENT_SIZE_MB, oversized counting, and toast.error/toast.warning
notifications, and return a handleFileSelect function to be used by the page
component so the page keeps only presentation/state hookup (calling
setSelectedFiles via the hook) while all business logic and notifications live
in the hook.
- Around line 273-289: The mobile preview modal currently forces a fixed inner
height via the h-[80vh] wrapper which can conflict with the Modal's maxHeight
prop and break on small screens; update the mobile block that renders <Modal>
(where open uses selectedDocument/isLoadingDocument, onClose calls
setSelectedDocument, and it renders <DocumentPreview document={selectedDocument}
isLoading={isLoadingDocument}>) to remove the hardcoded h-[80vh] wrapper and
instead allow the Modal to size the content (e.g., remove the div or replace its
class with a flexible utility such as h-full), ensuring the preview fills
available modal space without fixed viewport height.
In `@app/`(main)/evaluations/[id]/page.tsx:
- Around line 254-255: The long inline string assigned to segmentedClass is hard
to read; refactor by splitting it into named logical groups (e.g., base, border,
stateSelected, hover) and compose them with a class-combiner like clsx (import
clsx) or join an array/template literal to build segmentedClass; locate the
segmentedClass constant and replace the single 350+ char string with grouped
constants (e.g., const base = "...", const selected = "...") and then set
segmentedClass = clsx(base, selectedCondition && selected) or segmentedClass =
[base, selectedCondition ? selected : null].filter(Boolean).join(" ") to improve
readability and maintainability.
In `@app/components/document/DocumentPreview.tsx`:
- Around line 102-115: Add a short inline comment above the anchor rendering
that explains the conditional spread: when isPreviewable(document.fname) is true
we intentionally omit the download attribute so previewable files (images/PDFs)
open in a new tab, and when false we add download: document.fname to force a
download for non-previewable files; reference the anchor that uses
document.signed_url || document.object_store_url, the
isPreviewable(document.fname) check, and the conditional spread
`{...(isPreviewable(document.fname) ? {} : { download: document.fname })}` so
future maintainers understand the UX intent.
In `@app/components/icons/common/DownloadIcon.tsx`:
- Around line 7-15: The SVG in the DownloadIcon component relies solely on the
className prop for sizing; add explicit width and height attributes to the <svg>
element (e.g., width="24" and height="24") so the icon has sensible default
dimensions when className is omitted, and keep className applied so consumers
can still override sizing via CSS or a size prop if present; update the <svg>
tag (the element using className, fill, viewBox, stroke, strokeWidth,
strokeLinecap, strokeLinejoin) to include these default width/height attributes.
In `@app/components/knowledge-base/CollectionDetail.tsx`:
- Around line 113-120: The Preview button currently always calls
onPreviewDocument(documents[0]) which wrongly previews only the first document;
change the behavior to preview the currently selected document (or remove the
misleading button). Implement a selection state (e.g., selectedDocumentId or
selectedDocument) inside CollectionDetail and update document rows to set that
selection on click, then update the header button to call
onPreviewDocument(selectedDocument) (or disable/hide the button when no
selection). Update references to documents and onPreviewDocument in the
CollectionDetail component to use the new selection state so the preview opens
the intended document.
In `@app/hooks/useCollections.ts`:
- Around line 126-137: The code uses setSelectedCollection(prev => ...) purely
to read prev and set replacementId, which is unsafe in Strict Mode; instead read
selectedCollection directly (or mirror it to a ref like apiKeyRef) and compute
replacementId without using the state setter, then call
fetchCollectionDetails(replacementId) if present; specifically update the logic
around setSelectedCollection / selectedCollection / replacementId so you do not
rely on the updater to synchronously read state (or move this replacement logic
into an effect that runs after setCollections completes).
- Around line 45-88: fetchCollectionDetails and deleteCollection are closing
over the collections array causing identity churn; remove collections from their
dependency arrays and move any direct reads of collections into functional state
updaters or a stable ref. Specifically, in fetchCollectionDetails (function
fetchCollectionDetails) replace the collections.find(...) lookup by using
setSelectedCollection(prev => (prev if matching) || prev) or read from a
collectionsRef.current that you initialize and keep in sync, then remove
collections from the useCallback deps; do the same in deleteCollection by moving
originalCollection = collections.find(...) into a functional updater or reading
from collectionsRef.current before removing collections from its deps. Ensure
you update or create a collectionsRef and keep it updated whenever collections
changes so linting passes and behavior remains unchanged.
In `@app/lib/utils/collectionEnrichment.ts`:
- Around line 65-79: preFetchJobStatuses currently calls
getEntriesWithoutCollectionId() inside the collections.forEach loop causing
repeated localStorage reads and O(N·M) work; move the orphan lookup outside the
loop so you first call getEntriesWithoutCollectionId() once, iterate its results
to populate jobIdsToFetch (using the Set), then iterate collections to check
getCollectionDataByCollectionId(collection.id) only to decide whether to skip or
not; update references to jobIdsToFetch, getEntriesWithoutCollectionId, and
getCollectionDataByCollectionId in preFetchJobStatuses accordingly.
- Around line 81-108: preFetchJobStatuses and fetchJobStatus duplicate the same
apiFetch call and response normalization; extract a small helper (e.g.,
normalizeJobStatus(jobId, apiKey)) that performs the apiFetch<{ data?:
JobStatusData } & JobStatusData>(`/api/collections/jobs/${jobId}`, apiKey),
unwraps result.data || result, normalizes collection id via
jobData.collection?.id || jobData.collection_id || null, and returns a typed
intermediate shape { jobId, status: string | null, collectionId: string | null
}; replace the mapper in preFetchJobStatuses and the body of fetchJobStatus to
call normalizeJobStatus and use its returned shape to build the JobStatusMap or
single JobStatus respectively, preserving existing error handling.
In `@app/lib/utils/knowledgeBaseCache.ts`:
- Around line 75-100: pruneStaleCache currently only removes entries whose
collection_id no longer exists leaving orphaned entries without collection_id to
accumulate; update pruneStaleCache (which calls readCache/writeCache) to also
inspect entries missing data.collection_id and delete them if their created_at
timestamp is older than a TTL (e.g., 24h), adding created_at to your CacheEntry
when entries are created; alternatively, after preFetchJobStatuses completes
remove unmatched orphans by jobId — ensure you reference CacheEntry,
pruneStaleCache, getEntriesWithoutCollectionId, readCache and writeCache when
implementing the TTL-based deletion so orphaned entries are pruned
automatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6d902f7-d8ea-449b-ac64-fcfae333d61b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
CLAUDE.mdapp/(main)/document/page.tsxapp/(main)/evaluations/[id]/page.tsxapp/(main)/knowledge-base/page.tsxapp/components/FileExtBadge.tsxapp/components/Loader.tsxapp/components/document/DeleteDocumentModal.tsxapp/components/document/DocumentListing.tsxapp/components/document/DocumentListingSkeleton.tsxapp/components/document/DocumentPreview.tsxapp/components/document/DocumentPreviewSkeleton.tsxapp/components/document/UploadDocumentModal.tsxapp/components/icons/common/DownloadIcon.tsxapp/components/icons/index.tsxapp/components/knowledge-base/CollectionDetail.tsxapp/components/knowledge-base/CollectionsList.tsxapp/components/knowledge-base/CollectionsListSkeleton.tsxapp/components/knowledge-base/CreateCollectionForm.tsxapp/components/knowledge-base/DeleteCollectionModal.tsxapp/components/knowledge-base/DocumentChip.tsxapp/components/knowledge-base/DocumentPickerModal.tsxapp/components/knowledge-base/DocumentPreviewModal.tsxapp/globals.cssapp/hooks/useCollections.tsapp/lib/constants.tsapp/lib/utils/collectionEnrichment.tsapp/lib/utils/knowledgeBaseCache.tseslint.config.mjspackage.json
💤 Files with no reviewable changes (1)
- app/globals.css
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/ConfigModal.tsx (1)
131-133: ⚡ Quick winConsider adding user-facing error feedback.
When config fetching fails, the error is only logged to the console. Users have no indication that loading failed. Consider adding a toast notification to inform users of the error.
💡 Suggested improvement with toast notification
First, import the
useToasthook at the top of the file:import { useAuth } from "@/app/lib/context/AuthContext"; +import { useToast } from "@/app/hooks/useToast"; import { apiFetch } from "@/app/lib/apiClient";Then, use it in the component:
export default function ConfigModal({ isOpen, onClose, job, assistantConfig, }: ConfigModalProps) { const { activeKey, isAuthenticated } = useAuth(); + const { showToast } = useToast(); const [configVersionInfo, setConfigVersionInfo] = useState<ConfigVersionInfo | null>(null);Finally, show a toast in the error handler:
} } catch (error) { console.error("Error fetching config version info:", error); + showToast("Failed to load configuration details", "error"); } finally { setIsLoadingConfig(false); }As per coding guidelines, toast notifications should be consumed via the
useToast()hook.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/ConfigModal.tsx` around lines 131 - 133, The catch block in the config version fetch only logs errors to console (see the error handling inside ConfigModal's config fetch routine) — add user-facing feedback by importing and calling the useToast hook in the ConfigModal component, initialize const toast = useToast() (or similar) inside the component, and inside the catch (where console.error("Error fetching config version info:", error) is called) also call toast with a clear error title/message so users see a failure notification; keep the existing console.error for diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/components/ConfigModal.tsx`:
- Around line 131-133: The catch block in the config version fetch only logs
errors to console (see the error handling inside ConfigModal's config fetch
routine) — add user-facing feedback by importing and calling the useToast hook
in the ConfigModal component, initialize const toast = useToast() (or similar)
inside the component, and inside the catch (where console.error("Error fetching
config version info:", error) is called) also call toast with a clear error
title/message so users see a failure notification; keep the existing
console.error for diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9a3be92-db9a-44a0-a8fc-1afc6baed2d9
📒 Files selected for processing (15)
.coderabbit.yamlapp/(main)/configurations/prompt-editor/page.tsxapp/(main)/evaluations/[id]/page.tsxapp/components/ConfigModal.tsxapp/components/PageHeader.tsxapp/components/Sidebar.tsxapp/components/document/DocumentListing.tsxapp/components/document/DocumentListingSkeleton.tsxapp/components/knowledge-base/CollectionDetail.tsxapp/components/knowledge-base/CollectionsList.tsxapp/components/knowledge-base/CollectionsListSkeleton.tsxapp/components/knowledge-base/DocumentPickerModal.tsxapp/components/user-menu/Branding.tsxapp/hooks/useCollections.tseslint.config.mjs
✅ Files skipped from review due to trivial changes (7)
- app/components/user-menu/Branding.tsx
- .coderabbit.yaml
- app/(main)/configurations/prompt-editor/page.tsx
- app/components/Sidebar.tsx
- app/components/PageHeader.tsx
- eslint.config.mjs
- app/components/knowledge-base/CollectionsList.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- app/(main)/evaluations/[id]/page.tsx
- app/components/document/DocumentListingSkeleton.tsx
- app/components/knowledge-base/DocumentPickerModal.tsx
- app/components/knowledge-base/CollectionDetail.tsx
- app/components/document/DocumentListing.tsx
- app/components/knowledge-base/CollectionsListSkeleton.tsx
- app/hooks/useCollections.ts
vprashrex
approved these changes
May 11, 2026
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
2 participants
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.
Issue: #155
Summary
Revamps the Document and Knowledge Base experiences with a more modular, responsive UI and shared infrastructure for file handling.
CollectionsList,CollectionDetail,CreateCollectionForm,DeleteCollectionModal,DocumentPickerModal,DocumentPreviewModal,DocumentChip) backed by a newuseCollectionshook withjob-statuspolling and a localStorage-backed cache (knowledgeBaseCache.ts,collectionEnrichment.ts).FileExtBadge,DownloadIcon, listing/preview skeletons, and tweaks toPageHeader,Sidebar,ConfigModal, andglobals.cssfor visualconsistency.
MAX_DOCUMENT_UPLOAD_BATCH = 5and related upload limits..coderabbit.yaml, and tightenseslint.config.mjswith duplication/complexity rules.<Loader>and updates the evaluations detail segmented control styling.Summary by CodeRabbit
Release Notes
New Features
Improvements