fix(kb): listing kb at one level #813
Conversation
rahul1841
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds optional includeItems to the collections API and updates the frontend to request and consume nested items, reshape them into a file-tree, add chevron toggles and sticky header/grid UI, fetch folder contents on expand, and apply a minor monospace header style tweak in FileUploadSkeleton. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Frontend (Knowledge Management)
participant API as Server /knowledgeBase
participant Store as UI Store
UI->>API: GET /knowledgeBase/collections?includeItems=true
API-->>API: For each collection: check access, fetch items (parallel)
API-->>UI: 200 OK { collections: [{..., items:[...]}] }
UI->>Store: buildFileTree(collection.items), compute isOpen & totalItems
Store-->>UI: Render collections with chevrons and sticky headers
sequenceDiagram
autonumber
participant UI as Frontend
participant API as Server /knowledgeBase/:id/items
participant Toast as Toast
UI->>UI: User toggles folder
alt Open folder
UI->>API: GET /knowledgeBase/:id/items?parentId=<folderId>
API-->>UI: 200 OK [items...]
UI->>UI: Populate children, set isOpen=true
else Error fetching
API-->>UI: 4xx/5xx
UI->>Toast: Show error "Failed to load folder contents"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing touches
🧪 Generate unit tests
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @rahul1841, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the knowledge base management interface to present collection contents in a flattened, expandable view. This change improves user experience by allowing direct access to all items within a collection without navigating through multiple levels, supported by necessary backend API modifications to fetch comprehensive collection data.
Highlights
- Knowledge Base Display: Implemented a new display method for knowledge base collections, allowing all items within a collection to be listed at a single level, with expand/collapse functionality for better navigation.
- API Enhancements: Modified the backend API to support fetching all items associated with a knowledge base collection, enabling the new frontend display.
- User Interface Improvements: Applied several UI adjustments, including updated styling for table headers, sticky positioning for collection headers, and integration of new icons for the expand/collapse feature.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the knowledge base listing to display items within collections on the main view. This involves frontend changes for a hierarchical display and backend modifications to provide the necessary item data. The changes are functional, but there are opportunities for improvement in performance, code duplication, and data correctness. I've provided specific suggestions to address these points.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
server/api/knowledgeBase.ts (2)
762-874: Folder cache keys are inconsistent and ineffective.You check folderCache.has(folderPath) using the full remaining path, but write with only the first segment. This nullifies caching beyond the first level and risks wrong hits.
Use a stable key of parentId + folderName (case-insensitive):
- const folderPath = pathParts.join("/") + const folderPath = pathParts.join("/") // Check cache first - if (folderCache && folderCache.has(folderPath)) { - return folderCache.get(folderPath) || null - } + const cacheKey = `${parentId ?? "root"}:${(pathParts[0] || "").toLowerCase()}` + if (folderCache && folderCache.has(cacheKey)) { + return folderCache.get(cacheKey) || null + } @@ - if (folderCache) { - folderCache.set(pathParts.slice(0, 1).join("/"), currentFolderId) - } + if (folderCache) { + folderCache.set(cacheKey, currentFolderId) + }
1556-1594: Unauthorized file read (IDOR) in GetFileContentApi.This endpoint lacks authentication/authorization checks and doesn’t verify the item belongs to the requested collection. A malicious user could fetch arbitrary file contents by ID.
Apply access checks mirroring GetFilePreviewApi and verify collection membership:
export const GetFileContentApi = async (c: Context) => { const { sub: userEmail } = c.get(JwtPayloadKey) const collectionId = c.req.param("clId") const itemId = c.req.param("itemId") - - try{ - const collectionFile = await getCollectionFileByItemId(db, itemId) + try{ + // Authn: load user + const users = await getUserByEmail(db, userEmail) + if (!users || users.length === 0) { + throw new HTTPException(404, { message: "User not found" }) + } + const user = users[0] + + // Collection existence + access + const collection = await getCollectionById(db, collectionId) + if (!collection) { + throw new HTTPException(404, { message: "Collection not found" }) + } + if (collection.ownerId !== user.id && collection.isPrivate) { + throw new HTTPException(403, { message: "You don't have access to this Collection" }) + } + + // Item existence + membership + const item = await getCollectionItemById(db, itemId) + if (!item || item.type !== "file") { + throw new HTTPException(404, { message: "File not found" }) + } + if (item.collectionId !== collectionId) { + throw new HTTPException(404, { message: "File not found in this knowledge base" }) + } + + const collectionFile = await getCollectionFileByItemId(db, itemId) if (!collectionFile) { throw new HTTPException(404, { message: "File data not found" }) } @@ const fileContent = await readFile(collectionFile.storagePath)frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
653-671: Use totalItems instead of totalCount to avoid stale field names.Other code now uses totalItems; mixing fields risks incorrect counts.
-files: updatedCl.totalCount || selectedFiles.length, +files: updatedCl.totalItems ?? selectedFiles.length, @@ -totalCount: updatedCl.totalCount, +totalCount: updatedCl.totalItems,
773-797: Same: align to totalItems after add-files.Prevent incorrect file counts post-upload.
-files: updatedCl.totalCount || 0, +files: updatedCl.totalItems || 0,
🧹 Nitpick comments (8)
frontend/src/components/FileUploadSkeleton.tsx (1)
17-17: Guard against negative skeletonCount.If processedFiles ever exceeds totalFiles, Array.from({ length: skeletonCount }) will throw. Clamp to 0.
-const skeletonCount = Math.min(4, totalFiles - processedFiles) +const skeletonCount = Math.max(0, Math.min(4, totalFiles - processedFiles))server/api/knowledgeBase.ts (1)
247-281: includeItems: parallel item fetch with per-collection access is OK; consider minor hardening.
- Current try/catch inside map prevents Promise.all rejection—good.
- Edge case: if getAccessibleCollections can return private-but-shared collections, the current owner-only rule will return empty items for them. Verify intended behavior.
If shared private collections should expose items to permitted users, replace the ownerId/isPrivate check with the same predicate used by getAccessibleCollections or an explicit “hasAccess(user, collection)” helper.
frontend/src/routes/_authenticated/knowledgeManagement.tsx (6)
459-461: Periodic progress poll includes items—consistent with above.Consider extracting a shared fetchCollections() helper to DRY.
485-502: Mapping DTO → VM is sound; confirm default open behavior.
- buildFileTree over (collection.items || []) is fine for top-level listing.
- isOpen defaults to true when items exist; verify this is desired.
If you want collections collapsed by default, set isOpen: false and expand on click only.
Also applies to: 511-513
535-537: Duplicate collection mapping logic.The DTO→VM mapping repeats in multiple places. Extract to a small mapper to avoid drift.
+// utils/mappers.ts +export const mapCollection = (collection: CollectionType & { items?: CollectionItem[] }, userEmail?: string): Collection => ({ + id: collection.id, + name: collection.name, + description: collection.description, + files: collection.totalItems || 0, + items: buildFileTree((collection.items || []).map((item) => ({ + name: item.name, + type: item.type as "file" | "folder", + totalFileCount: item.totalFileCount, + updatedAt: item.updatedAt, + id: item.id, + updatedBy: item.lastUpdatedByEmail || userEmail || "Unknown", + }))), + isOpen: (collection.items || []).length > 0, + lastUpdated: new Date(collection.updatedAt).toLocaleString("en-GB", { day: "numeric", month: "short", year: "numeric", hour: "2-digit", minute: "2-digit" }), + updatedBy: collection.lastUpdatedByEmail || "Unknown", + totalCount: collection.totalItems, + isPrivate: collection.isPrivate, +})Also applies to: 541-569
1239-1239: Duplicate Tailwind classes.Both max-w-[50%] and max-w-[90vw] are present; the latter wins. Drop one to avoid confusion.
-<div className="relative bg-white dark:bg-[#1E1E1E] w-[50%] max-w-[50%] max-w-[90vw] h-full shadow-2xl ... +<div className="relative bg-white dark:bg-[#1E1E1E] w-[50%] max-w-[90vw] h-full shadow-2xl ...
1482-1488: Invalid Tailwind class: use text-[18px] instead of text-18px.text-18px won’t be recognized by Tailwind; brackets are required for arbitrary values.
-<h2 className="text-18px font-semibold text-gray-800 dark:text-gray-200"> +<h2 className="text-[18px] font-semibold text-gray-800 dark:text-gray-200">
1473-1480: Chevron indicator placement is clear and accessible.Consider adding aria-expanded to the clickable header for a11y.
-<div - className="sticky mb-2 cursor-pointer top-0 bg-white dark:bg-[#1E1E1E] py-1" - onClick={async () => { ... }} -> +<div + className="sticky mb-2 cursor-pointer top-0 bg-white dark:bg-[#1E1E1E] py-1" + onClick={async () => { ... }} + aria-expanded={collection.isOpen} + role="button" +>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/FileUploadSkeleton.tsx(1 hunks)frontend/src/routes/_authenticated/knowledgeManagement.tsx(10 hunks)server/api/knowledgeBase.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/api/knowledgeBase.ts (2)
server/db/schema/knowledgeBase.ts (1)
collections(19-58)server/db/knowledgeBase.ts (1)
getCollectionItemsByParent(142-164)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
frontend/src/api.ts (1)
api(4-4)frontend/src/types/knowledgeBase.ts (1)
CollectionItem(20-58)frontend/src/utils/fileUtils.ts (1)
buildFileTree(162-203)
🔇 Additional comments (8)
frontend/src/components/FileUploadSkeleton.tsx (1)
22-22: Monospace header style alignment looks good.Matches the updated header styling used in Knowledge Management. No functional impact.
server/api/knowledgeBase.ts (1)
234-235: New includeItems query toggle is fine.Boolean parsing from query is clear and non-breaking.
frontend/src/routes/_authenticated/knowledgeManagement.tsx (6)
12-14: Chevron imports align with the new expand/collapse UX.No issues.
399-401: Good: collections fetch now opts into includeItems.Keeps the ongoing-upload check consistent with the new API capability.
584-584: Dependency on user email is appropriate.Ensures refetch on user change.
1274-1276: Sticky overlay header is a nice UX touch.Keeps context visible while scrolling the file list.
1440-1441: Sticky collection row wrapper works well with the chevron affordance.Good interaction design.
1540-1545: Header grid matches FileUploadSkeleton; good consistency.No issues.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
670-683: Fix inconsistent use of totalItems vs totalCount (breaks counts and completion checks).Server responses are read as totalItems in some places and totalCount in others. Normalize to totalItems with a safe fallback to avoid zeros/miscounts after create/add/delete.
Apply this diff:
@@ - files: updatedCl.totalCount || selectedFiles.length, + files: (updatedCl.totalItems ?? updatedCl.totalCount ?? selectedFiles.length), @@ - totalCount: updatedCl.totalCount, + totalCount: (updatedCl.totalItems ?? updatedCl.totalCount), @@ - files: updatedCl.totalCount || 0, + files: (updatedCl.totalItems ?? updatedCl.totalCount ?? 0), @@ - files: updatedCl.totalCount || 0, + files: (updatedCl.totalItems ?? updatedCl.totalCount ?? 0),Also applies to: 793-817, 872-895
715-717: Revoke object URLs to prevent memory leaks.Previews from URL.createObjectURL should be revoked on remove/clear/close.
Apply this diff:
@@ - const handleCloseModal = () => { + const handleCloseModal = () => { + // Clean up previews + try { + selectedFiles.forEach((f) => f.preview && URL.revokeObjectURL(f.preview)) + } catch {} setShowNewCollection(false) @@ - const handleRemoveFile = (id: string) => { - setSelectedFiles((prev) => prev.filter((f) => f.id !== id)) + const handleRemoveFile = (id: string) => { + setSelectedFiles((prev) => { + const removed = prev.find((f) => f.id === id) + try { if (removed?.preview) URL.revokeObjectURL(removed.preview) } catch {} + return prev.filter((f) => f.id !== id) + }) } @@ - const handleRemoveAllFiles = () => { - setSelectedFiles([]) + const handleRemoveAllFiles = () => { + setSelectedFiles((prev) => { + try { prev.forEach((f) => f.preview && URL.revokeObjectURL(f.preview)) } catch {} + return [] + }) }Also applies to: 719-721
♻️ Duplicate comments (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
494-521: DRY the item→FileTree mapping into a helper.The same map/build logic is repeated in 6 places. Extract a helper to reduce bugs and maintenance.
Example helper (place near other utils in this file or in fileUtils):
function toFileTree(items: CollectionItem[]) { return buildFileTree( items.map((it) => ({ id: it.id, name: it.name, type: it.type as "file" | "folder", totalFileCount: it.totalFileCount, updatedAt: it.updatedAt, updatedBy: it.lastUpdatedByEmail ?? "Unknown", })), ) }Then replace each inline map/build with:
coll.items = toFileTree(data) // or toFileTree(items)Also applies to: 551-569, 795-805, 875-885, 1474-1485, 1625-1644
1504-1504: Avoid arbitrary text-18px; use Tailwind token.Prefer text-lg or define a token in tailwind.config.
Apply this diff:
- <h2 className="text-18px font-semibold text-gray-800 dark:text-gray-200"> + <h2 className="text-lg font-semibold text-gray-800 dark:text-gray-200">
506-509: Don’t fall back to current user for updatedBy (data misattribution).If lastUpdatedByEmail is missing, use "Unknown" rather than user?.email.
Apply this diff:
- updatedBy: - item.lastUpdatedByEmail || user?.email || "Unknown", + updatedBy: item.lastUpdatedByEmail ?? "Unknown", @@ - item.lastUpdatedByEmail || user?.email || "Unknown", + item.lastUpdatedByEmail ?? "Unknown", @@ - updatedBy: - item.lastUpdatedByEmail || user?.email || "Unknown", + updatedBy: item.lastUpdatedByEmail ?? "Unknown", @@ - updatedBy: - item.lastUpdatedByEmail || user?.email || "Unknown", + updatedBy: item.lastUpdatedByEmail ?? "Unknown", @@ - updatedBy: - item.lastUpdatedByEmail || - user?.email || - "Unknown", + updatedBy: item.lastUpdatedByEmail ?? "Unknown", @@ - updatedBy: - item.lastUpdatedByEmail || - user?.email || - "Unknown", + updatedBy: item.lastUpdatedByEmail ?? "Unknown",Also applies to: 564-566, 802-804, 881-883, 1352-1356, 1634-1637
🧹 Nitpick comments (4)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (4)
1259-1259: Remove duplicate Tailwind classes.max-w is repeated; keep a single constraint.
Apply this diff:
- <div className="relative bg-white dark:bg-[#1E1E1E] w-[50%] max-w-[50%] max-w-[90vw] h-full shadow-2xl transform transition-transform duration-300 ease-in-out"> + <div className="relative bg-white dark:bg-[#1E1E1E] w-[50%] max-w-[90vw] h-full shadow-2xl transform transition-transform duration-300 ease-in-out">
1037-1037: Fix user-facing copy.Small grammar tweak.
Apply this diff:
- let errorMessage = "Failed to preview file download the file instead." + let errorMessage = "Failed to preview file. Download the file instead."
1879-1895: Tighten types for findItemByPath.Return FileNode | null instead of any.
Apply this diff:
-function findItemByPath(items: FileNode[], targetPath: string): any | null { - const findInNodes = (nodes: FileNode[], currentPath: string): any | null => { +function findItemByPath(items: FileNode[], targetPath: string): FileNode | null { + const findInNodes = (nodes: FileNode[], currentPath: string): FileNode | null => {
1460-1554: Chevron + header grid changes: good UX; minor a11y follow-up.The Plus and More icons act as buttons; wrap with a Button (or add role/button + aria-label) for keyboard/screen-reader access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/FileUploadSkeleton.tsx(1 hunks)frontend/src/routes/_authenticated/knowledgeManagement.tsx(9 hunks)server/api/knowledgeBase.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/api/knowledgeBase.ts
- frontend/src/components/FileUploadSkeleton.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
frontend/src/api.ts (1)
api(4-4)frontend/src/types/knowledgeBase.ts (1)
CollectionItem(20-58)frontend/src/utils/fileUtils.ts (1)
buildFileTree(162-219)
🔇 Additional comments (2)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
406-408: includeItems=true on collection fetches looks good.This aligns the client with the server’s expanded payload.
Also applies to: 466-468, 545-547
418-421: Upload-completion heuristic may false-positive for existing collections.Comparing totalItems ≥ uploaded count ignores pre-existing files. Consider storing a baseline count in localStorage and compare delta.
Would you like a small patch to persist baselineCount and adjust the check?
Also applies to: 478-481
4 participants
Description
Testing
Summary by CodeRabbit
New Features
Style
Bug Fixes