fix: deduplicate listing collection items #775
Conversation
naSim087
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. WalkthroughReplaces array-based collection state updates with Map-based dedupe/merge keyed by collection ID across fetch, create, add-files, edit, and upload flows; preserves per-collection UI state (isOpen, items), normalizes lastUpdated/updatedBy/files, and improves folder-loading error handling and messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant UI as KnowledgeManagement UI
participant S as CollectionsState (Map)
participant API as Backend
U->>UI: Load / Refresh
UI->>API: fetchCollections()
API-->>UI: collections[]
UI->>S: mergeFetchedCollections(collections[]) /* dedupe by id, preserve isOpen/items */
S-->>UI: collections[] (deduped)
U->>UI: Create / Add files / Edit / Upload
UI->>API: submitChange(...)
API-->>UI: updatedCollection
UI->>S: mergeSingleCollection(updatedCollection)
S-->>UI: collections[] (updated)
sequenceDiagram
autonumber
participant U as User
participant UI as KnowledgeManagement UI
participant API as Backend
participant S as CollectionsState (Map)
U->>UI: Expand folder
UI->>API: fetchFolderContents(folderId)
alt success
API-->>UI: children[]
UI->>S: insertChildren(folderId, children with isOpen=false, lastUpdated, updatedBy)
S-->>UI: updated tree
else error
API--x UI: error
UI->>UI: console.error + toast("Could not load folder")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–70 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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 @naSim087, 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 addresses a critical UI bug where knowledge base collections were appearing as duplicates, particularly after creation or updates. The changes refactor the collection state management to ensure uniqueness and consistency in the displayed list of collections.
Highlights
- Collection Deduplication Logic: Implemented a Map-based deduplication strategy across several setCollections calls (e.g., on initial fetch, after creating a new collection, after adding files, and after editing a collection). This ensures that collections are uniquely identified by their ID, preventing duplicate entries in the UI.
- UI Rendering Filter: Added a filter to the collection rendering logic to temporarily hide the collection currently being uploaded, preventing it from appearing twice during the upload process.
- React Key Optimization: Updated the React list key for collections from 'index' to 'collection.id', which is a best practice for stable list rendering and helps React correctly identify and update list items, especially important with dynamic data and deduplication.
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 fixes a bug where collections were listed multiple times in the UI. The fix involves using a Map to deduplicate collections by their ID when updating the state, which is a solid approach. The changes correctly preserve UI state like whether a collection is open. I've identified a potential runtime error and an opportunity to refactor duplicated code for better maintainability. Overall, good work on fixing the duplication bug.
![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
🧹 Nitpick comments (8)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (8)
483-504: TypefetchedCollectionstoCollection[]for compile-time safety.Prevents accidental field drift (like
totalCountvstotalItems).- const fetchedCollections = data.map((collection: CollectionType) => ({ + const fetchedCollections: Collection[] = data.map((collection: CollectionType) => ({ id: collection.id, name: collection.name, description: collection.description, files: collection.totalItems || 0, items: [], isOpen: false,
505-526: Strongly type the dedup Map and loop.Avoids
anyleaks and future type regressions.- const collectionsMap = new Map() + const collectionsMap = new Map<string, Collection>() ... - fetchedCollections.forEach((col: any) => { + fetchedCollections.forEach((col: Collection) => {
628-640: Comment and typing nits: it’s Map-based, not Set-based; also type the Map.Minor clarity/readability improvement.
- // Use Set-based approach to prevent duplicates + // Use Map-based approach to prevent duplicates setCollections((prev) => { - const collectionsMap = new Map() + const collectionsMap = new Map<string, Collection>()
1214-1225: Avoid name-based filtering during uploads; prefer stable IDs.When adding files to an existing collection, filtering by name can hide a different collection that (rarely) shares the same name. Using an ID is more robust. This requires tracking the uploading collection ID in state (and localStorage) alongside the name.
I can provide a small patch to:
- extend the persisted upload state with
uploadingCollectionId?: string- set it in
handleAddFilesToCollection- prefer ID-based filtering if available, else fallback to name
Would you like me to draft that?
1230-1260: Minimize direct nested mutations for safer state updates.This block mutates
colland nested nodes beforesetCollections. It works because you cloned the top-level array, but nested references remain shared. Consider immutable updates (return new objects) to reduce accidental shared state bugs.If you want, I can refactor this toggle path to a pure update (
map+ structural sharing) without deep mutations.
391-393: Trim collection names before comparison to be defensive.Whitespace differences could bypass the equality check.
- collection.name.toLowerCase() === - uploadingCollectionName.toLowerCase(), + collection.name.trim().toLowerCase() === + uploadingCollectionName.trim().toLowerCase(),
120-122: Nit: copy tweaks are fine.“Loading document…” string changes are harmless.
197-199: Nit: copy tweaks are fine.“Select a document to view” formatting looks good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
frontend/src/types/knowledgeBase.ts (2)
Collection(1-18)CollectionItem(20-58)frontend/src/utils/fileUtils.ts (3)
FileNode(140-149)uploadFileBatch(195-234)buildFileTree(151-192)frontend/src/api.ts (1)
api(4-4)
🔇 Additional comments (7)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (7)
1309-1430: LGTM: folder lazy-load with Map-merge and consistent shape.The tree toggle fetch builds children with
files,lastUpdated,updatedBy, and preservesisOpenconsistently. Looks good.
1552-1553: LGTM: clearer modal title with folder context.The “Add files to … / …” header improves clarity.
678-686: LGTM: reusing the creation modal for add-files flow.Passing
foldercontext and pre-filling the name is clean.
722-726: LGTM: propagate parentId to uploads to preserve hierarchy.Upload API call correctly includes
parentIdwhen targeting a folder.
744-777: LGTM: post-upload refresh uses Map-merge and rebuilds tree.Merges without duplication and preserves UI state.
891-919: LGTM: update-by-ID with Map-merge prevents duplicate entries after rename.Good dedup and state preservation.
29-32: Type-only imports: good.Using
import typeprevents runtime bloat.
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)
856-887: Use totalItems after delete to keep counts accurate.- files: updatedCl.totalCount || 0, + files: updatedCl.totalItems || 0,
633-650: Creation flow: use totalItems instead of totalCount.
Ensures correct initial file count and canonical field.- files: updatedCl.totalCount || selectedFiles.length, + files: updatedCl.totalItems || selectedFiles.length, ... - totalCount: updatedCl.totalCount, + totalCount: updatedCl.totalItems,
♻️ Duplicate comments (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
525-550: Type the Map and loop; consider extracting a reusable merge-dedup helper.
Reimplements the same pattern found elsewhere; typing prevents regressions.Apply:
- const collectionsMap = new Map() + const collectionsMap = new Map<string, Collection>() ... - fetchedCollections.forEach((col: any) => { + fetchedCollections.forEach((col: Collection) => {Optional helper (outside this hunk):
function mergeCollections(prev: Collection[], next: Collection[]): Collection[] { const m = new Map<string, Collection>() prev.forEach((c) => m.set(c.id, c)) next.forEach((c) => { const existing = m.get(c.id) m.set( c.id, existing ? { ...c, isOpen: existing.isOpen, items: existing.items } : c, ) }) return Array.from(m.values()) }
410-458: Use totalItems, add typing, and sort before set to ensure correct counts and stable order.
Current mapping still reads legacy totalCount and uses untyped Map; the returned array isn’t sorted here.Apply:
- const fetchedCollections = data.map( + const fetchedCollections: Collection[] = data.map( (collection: CollectionType) => ({ id: collection.id, name: collection.name, description: collection.description, - files: collection.totalCount || 0, + files: collection.totalItems || 0, items: [], isOpen: false, 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.totalCount, + totalCount: collection.totalItems, isPrivate: collection.isPrivate, }), ) // Use Map to deduplicate by ID setCollections((prev) => { - const collectionsMap = new Map() + const collectionsMap = new Map<string, Collection>() // First add existing collections (to preserve any state like isOpen) prev.forEach((col) => collectionsMap.set(col.id, col)) // Then add/update with fetched collections - fetchedCollections.forEach((col: any) => { + fetchedCollections.forEach((col: Collection) => { const existing = collectionsMap.get(col.id) if (existing) { // Preserve certain states while updating data collectionsMap.set(col.id, { ...col, isOpen: existing.isOpen, items: existing.items, }) } else { collectionsMap.set(col.id, col) } }) - - return Array.from(collectionsMap.values()) + const sorted = sortCollectionsByUpdated( + Array.from(collectionsMap.values()), + ) + return sorted })
772-809: Guard against spreading undefined and use totalItems.
Spreading collectionsMap.get(...) can throw if missing; also keep to totalItems to avoid zero/incorrect counts.Apply:
- const collectionsMap = new Map() + const collectionsMap = new Map<string, Collection>() ... - const updatedCollection = { - ...collectionsMap.get(addingToCollection.id), - files: updatedCl.totalCount || 0, + const base = + collectionsMap.get(addingToCollection.id) ?? { + id: addingToCollection.id, + name: addingToCollection.name, + description: addingToCollection.description ?? null, + files: 0, + items: [], + isOpen: false, + lastUpdated: "", + updatedBy: "Unknown", + totalCount: 0, + isPrivate: false, + } + const updatedCollection: Collection = { + ...base, + files: updatedCl.totalItems || 0, items: buildFileTree( items.map((item: CollectionItem) => ({ name: item.name, type: item.type as "file" | "folder", totalFileCount: item.totalFileCount, updatedAt: item.updatedAt, id: item.id, updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", })), ), lastUpdated: new Date(updatedCl.updatedAt).toLocaleString("en-GB", { day: "numeric", month: "short", year: "numeric", hour: "2-digit", minute: "2-digit", }), updatedBy: updatedCl.lastUpdatedByEmail || "Unknown", }
🧹 Nitpick comments (5)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (5)
477-495: Avoid parsing localized strings for sorting; keep a stable numeric sort key.
Current regex on toLocaleString is brittle across locales/browsers. Prefer storing updatedAt (ISO) or a numeric sortTs alongside lastUpdated and sort by that.Example (outside this hunk): add sortTs: Date.parse(updatedAt) when mapping; then implement:
- return collections.sort((a, b) => { /* parse lastUpdated */ }) + return [...collections].sort((a, b) => (b as any).sortTs - (a as any).sortTs)I can provide a scoped patch that wires sortTs in all mapping sites if you want.
652-668: Comment says “Set-based” but code uses Map; also type the Map.- // Use Set-based approach to prevent duplicates + // Use Map-based approach to prevent duplicates - setCollections((prev) => { - const collectionsMap = new Map() + setCollections((prev) => { + const collectionsMap = new Map<string, Collection>()
923-954: Type the Map in update flow; optional.- setCollections((prev) => { - const collectionsMap = new Map() + setCollections((prev) => { + const collectionsMap = new Map<string, Collection>()
1117-1131: Include files count for children (parity with list tree).
Not strictly required for the doc viewer tree, but keeps data uniform.- n.children = items.map( + n.children = items.map( (item: CollectionItem) => ({ id: item.id, name: item.name, type: item.type as "file" | "folder", + files: item.totalFileCount, lastUpdated: item.updatedAt, updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", isOpen: false, children: item.type === "folder" ? [] : undefined, }), )
1250-1467: Optional UX: show a placeholder card for the in-progress collection rather than hiding it.
Reduces surprise when users don’t see the just-created collection during upload.I can sketch a lightweight “uploading…” card keyed by uploadingCollectionName if you want it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
frontend/src/types/knowledgeBase.ts (2)
Collection(1-18)CollectionItem(20-58)frontend/src/api.ts (1)
api(4-4)frontend/src/utils/fileUtils.ts (3)
FileNode(140-149)uploadFileBatch(195-234)buildFileTree(151-192)
🔇 Additional comments (10)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (10)
29-32: Types import: LGTM.
120-122: UX copy: LGTM.
197-199: Empty-state text: LGTM.
391-393: Case-insensitive match: LGTM.
503-524: Initial fetch mapping: LGTM (uses totalItems correctly).
706-714: Add-files modal opener: LGTM.
750-754: Pass parentId for nested uploads: LGTM.
1250-1261: Filtering out the uploading collection to avoid duplicates: LGTM.
Prevents transient double entries during in-flight uploads.
1391-1462: Folder toggle fetch/build path: LGTM.
Uses children lastUpdated/files fields consistently and handles errors.
1588-1588: Modal title path segment: LGTM.
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 (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
612-629: New collection: switch to totalItems.Align with API contract to avoid silent zeros when totalCount alias is dropped.
const newCollection: Collection = { id: updatedCl.id, name: updatedCl.name, description: updatedCl.description, - files: updatedCl.totalCount || selectedFiles.length, + files: updatedCl.totalItems || selectedFiles.length, lastUpdated: new Date(updatedCl.updatedAt).toLocaleString("en-GB", { day: "numeric", month: "short", year: "numeric", hour: "2-digit", minute: "2-digit", }), updatedBy: updatedCl.lastUpdatedByEmail || user?.email || "Unknown", items: [], isOpen: false, - totalCount: updatedCl.totalCount, + totalCount: updatedCl.totalItems, isPrivate: updatedCl.isPrivate, }
258-259: Fix TS6133: remove unused openDropdown state.This breaks the build. Remove the state and its only usage in handleCloseModal.
- const [openDropdown, setOpenDropdown] = useState<string | null>(null) + // removed unused openDropdown stateAlso remove the reset call:
- setOpenDropdown(null) + // removed: openDropdown no longer used
827-858: Standardize totals here too (follow-ups).Elsewhere you moved to totalItems; mirror that in delete refresh to keep fields consistent.
Would you like me to push a patch for this block as well?
♻️ Duplicate comments (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
895-922: DRY: extract a shared merge/dedup helper.This Map-based state merge duplicates logic found in fetch and upload flows. Extract to a reusable mergeCollections(prev, fetched) to reduce drift.
412-461: Use totalItems consistently and type the Map.Avoid mixing legacy totalCount; prefer totalItems everywhere. Also add Map generics to prevent any and tighten types.
- const fetchedCollections = data.map( + const fetchedCollections = data.map( (collection: CollectionType) => ({ id: collection.id, name: collection.name, description: collection.description, - files: collection.totalCount || 0, + files: collection.totalItems || 0, items: [], isOpen: false, 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.totalCount, + totalCount: collection.totalItems, isPrivate: collection.isPrivate, }), ) - // Use Map to deduplicate by ID + // Use Map to deduplicate by ID setCollections((prev) => { - const collectionsMap = new Map() + const collectionsMap = new Map<string, Collection>() // First add existing collections (to preserve any state like isOpen) prev.forEach((col) => collectionsMap.set(col.id, col)) // Then add/update with fetched collections - fetchedCollections.forEach((col: any) => { + fetchedCollections.forEach((col: Collection) => { const existing = collectionsMap.get(col.id) if (existing) { // Preserve certain states while updating data collectionsMap.set(col.id, { ...col, isOpen: existing.isOpen, items: existing.items, }) } else { collectionsMap.set(col.id, col) } }) return Array.from(collectionsMap.values()) })
747-781: Possible TypeError: spreading undefined from Map.get().collectionsMap.get(addingToCollection.id) can be undefined; spreading it will throw. Also standardize on totalItems here.
- // Update the specific collection - const updatedCollection = { - ...collectionsMap.get(addingToCollection.id), - files: updatedCl.totalCount || 0, - items: buildFileTree( + // Update the specific collection + const existing = collectionsMap.get(addingToCollection.id) as Collection | undefined + const updatedCollection: Collection = { + ...(existing ?? ({} as Collection)), + id: existing?.id ?? addingToCollection.id, + name: existing?.name ?? addingToCollection.name, + description: existing?.description ?? null, + isOpen: existing?.isOpen ?? false, + files: updatedCl.totalItems || 0, + items: buildFileTree( items.map((item: CollectionItem) => ({ name: item.name, type: item.type as "file" | "folder", totalFileCount: item.totalFileCount, updatedAt: item.updatedAt, id: item.id, updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", })), ), lastUpdated: new Date(updatedCl.updatedAt).toLocaleString("en-GB", { day: "numeric", month: "short", year: "numeric", hour: "2-digit", minute: "2-digit", }), updatedBy: updatedCl.lastUpdatedByEmail || "Unknown", - } + totalCount: updatedCl.totalItems, + isPrivate: existing?.isPrivate, + } collectionsMap.set(addingToCollection.id, updatedCollection)
🧹 Nitpick comments (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
631-643: Comment mismatch and missing generics.The block uses a Map, not a Set; also add Map generics for type safety.
- // Use Set-based approach to prevent duplicates + // Use Map-based approach to prevent duplicates setCollections((prev) => { - const collectionsMap = new Map() + const collectionsMap = new Map<string, Collection>() // Add existing collections prev.forEach((col) => collectionsMap.set(col.id, col)) // Add/update new collection collectionsMap.set(newCollection.id, newCollection) return Array.from(collectionsMap.values()) })
1217-1261: Guard item fetch with error handling.Add try/catch and response.ok checks; prevents crashes on non-2xx and improves UX.
- if (coll) { + if (coll) { coll.isOpen = !coll.isOpen if (coll.isOpen) { - const response = await api.cl[":id"].items.$get({ - param: { id: collection.id }, - }) - const data = await response.json() - coll.items = buildFileTree( - data.map((item: CollectionItem) => ({ - name: item.name, - type: item.type as "file" | "folder", - totalFileCount: item.totalFileCount, - updatedAt: item.updatedAt, - id: item.id, - updatedBy: - item.lastUpdatedByEmail || - user?.email || - "Unknown", - })), - ) + try { + const response = await api.cl[":id"].items.$get({ + param: { id: collection.id }, + }) + if (!response.ok) { + throw new Error(`Failed to fetch items: ${response.status}`) + } + const data = await response.json() + coll.items = buildFileTree( + data.map((item: CollectionItem) => ({ + name: item.name, + type: item.type as "file" | "folder", + totalFileCount: item.totalFileCount, + updatedAt: item.updatedAt, + id: item.id, + updatedBy: + item.lastUpdatedByEmail || + user?.email || + "Unknown", + })), + ) + } catch (err) { + console.error("Failed to load collection items", err) + showToast("Error", "Failed to load collection items", true) + } } else { // coll.items = []; // This would clear the items, maybe not desired } setCollections(updatedCollections) }
1084-1098: Optional: include files count in child nodes.If the Simple tree shows file counts, pass files: item.totalFileCount as in the list view for consistency.
- n.children = items.map( + n.children = items.map( (item: CollectionItem) => ({ id: item.id, name: item.name, type: item.type as "file" | "folder", + files: item.totalFileCount, lastUpdated: item.updatedAt, updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", isOpen: false, children: item.type === "folder" ? [] : undefined, }), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (4)
frontend/src/types/knowledgeBase.ts (2)
Collection(1-18)CollectionItem(20-58)frontend/src/utils/fileUtils.ts (3)
FileNode(151-160)uploadFileBatch(206-245)buildFileTree(162-203)frontend/src/api.ts (1)
api(4-4)frontend/src/components/ui/dropdown-menu.tsx (4)
DropdownMenu(188-188)DropdownMenuTrigger(189-189)DropdownMenuContent(190-190)DropdownMenuItem(191-191)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/routes/_authenticated/knowledgeManagement.tsx
[error] 258-258: TypeScript error TS6133: 'openDropdown' is declared but its value is never read. (Command: bunx tsc -b)
🔇 Additional comments (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
485-506: Fetch mapping looks correct.Good: uses totalItems, preserves UI state fields, and normalizes metadata.
1217-1227: UI duplicate filter looks good.Filtering the in-progress uploading collection by name avoids transient duplicates during creation. Nicely contained to the view.
393-401: Case-insensitive name match: LGTM.Robust check for existingCollection during periodic progress polling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (6)
678-678: Inconsistent API field usage detectedLine 678 still uses
updatedCl.totalCountinstead of the expectedupdatedCl.totalItemsfield from the API.Based on past review comments, all
totalCountreferences should be replaced withtotalItems. Apply this fix:- files: updatedCl.totalCount || selectedFiles.length, + files: updatedCl.totalItems || selectedFiles.length,
689-689: Fix API field referenceSimilar to above, this should use
totalItemsinstead oftotalCount:- totalCount: updatedCl.totalCount, + totalCount: updatedCl.totalItems,
816-839: Critical: Handle potential undefined Map.get() resultThe existing review comment is still valid - spreading the result of
collectionsMap.get(addingToCollection.id)can cause a TypeError if the key doesn't exist, sinceMap.get()returnsundefinedfor missing keys.Apply the suggested fix:
- const updatedCollection = { - ...collectionsMap.get(addingToCollection.id), + const existingCollection = collectionsMap.get(addingToCollection.id); + if (existingCollection) { + const updatedCollection = { + ...existingCollection,
818-818: Fix API field consistencyReplace
totalCountwithtotalItems:- files: updatedCl.totalCount || 0, + files: updatedCl.totalItems || 0,
894-894: Another totalCount reference to fixContinue fixing API field references:
- files: updatedCl.totalCount || 0, + files: updatedCl.totalItems || 0,
1371-1398: Important: Fix field mapping for loaded childrenThe existing review comment correctly identifies that children nodes use
updatedAtinstead oflastUpdated, which will cause UI display issues. Apply the suggested fix:- n.children = items.map( - (item: CollectionItem) => ({ - id: item.id, - name: item.name, - type: item.type as "file" | "folder", - lastUpdated: item.updatedAt, - updatedBy: - item.lastUpdatedByEmail || - user?.email || - "Unknown", - isOpen: false, - children: - item.type === "folder" - ? [] - : undefined, - }), - ) + n.children = items.map( + (item: CollectionItem) => ({ + id: item.id, + name: item.name, + type: item.type as "file" | "folder", + files: item.totalFileCount, + lastUpdated: item.updatedAt, + updatedBy: + item.lastUpdatedByEmail || + user?.email || + "Unknown", + isOpen: false, + children: + item.type === "folder" + ? [] + : undefined, + }), + )
🧹 Nitpick comments (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
273-276: Minor formatting: Remove extra empty linesThese extra empty lines seem unintentional and can be removed for cleaner formatting.
- const [isFileTreeCollapsed, setIsFileTreeCollapsed] = useState(true) - - // Chat visibility state based on zoom level - const [isChatHidden, setIsChatHidden] = useState(false) - + const [isFileTreeCollapsed, setIsFileTreeCollapsed] = useState(true) + // Chat visibility state based on zoom level + const [isChatHidden, setIsChatHidden] = useState(false)
1044-1045: Minor: Trailing comma inconsistencyThe
false,parameter has a trailing comma that could be removed for consistency:- false, + false
1723-1723: Minor: Trailing whitespaceThere appears to be trailing whitespace or an empty line that could be cleaned up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx(26 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
frontend/src/types/knowledgeBase.ts (1)
CollectionItem(20-58)frontend/src/utils/fileUtils.ts (1)
buildFileTree(162-219)frontend/src/api.ts (1)
api(4-4)
🔇 Additional comments (19)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (19)
291-291: Good SSR guard implementationThe guard for server-side rendering is properly implemented to prevent window-related errors.
317-330: Event listener management looks solidThe event listener setup and cleanup for zoom detection is well implemented with proper cleanup in the effect's return function.
Also applies to: 333-333
407-407: Good API consistency in parameter namingUsing
includeItems: "true"consistently across all API calls maintains good parameter naming consistency.Also applies to: 467-467, 551-551
494-526: LGTM: Clean implementation of deduplication logicThe implementation correctly uses a Map-based approach to prevent duplicates and preserve UI state. The collection mapping logic is well-structured.
556-590: Good deduplication implementation for initial data fetchThe Map-based approach correctly handles deduplication during the initial collections fetch, maintaining consistency with the PR objectives.
693-704: Excellent Map-based deduplication patternThis is a clean implementation of Set/Map-based deduplication that prevents duplicate listings while preserving existing UI state. The pattern correctly addresses the PR objective.
809-842: Well-implemented Map-based state update with UI preservationThe Map-based approach correctly preserves existing UI state (
isOpen,items) while updating the collection data. This properly addresses the deduplication issue mentioned in the PR objectives.
956-983: Good Map-based collection update patternThe Map-based approach for updating collection names is well-implemented and consistent with the overall deduplication strategy.
1011-1014: Simple and effective collection removalThe collection removal logic is straightforward and correct - using
filterto remove the deleted collection from state.
1100-1114: Improved error handling implementationThe enhanced error handling with better error message extraction and consistent toast messaging is a good improvement.
1216-1227: Clean button layout with proper line formattingThe multi-line button properties are well-formatted and the conditional title text is properly implemented.
1267-1278: Improved SVG icon formattingThe SVG icon is now properly formatted across multiple lines for better readability while maintaining functionality.
1288-1292: Good backdrop stylingThe backdrop styling and click handler for the chat overlay are properly implemented.
1457-1466: Good button formatting and disabled state handlingThe button formatting across multiple lines is clean and the disabled state properly prevents actions during uploads.
1472-1494: Well-structured upload progress displayThe upload progress display with file counts and batch information provides good user feedback during the upload process.
1534-1543: Clean chevron icon formattingThe chevron icons are properly formatted with consistent styling and good visual hierarchy.
1559-1567: Good conditional interaction handlingThe conditional logic properly disables interactions during uploads while maintaining the existing dropdown functionality.
1580-1582: Consistent disabled state patternThe disabled state handling in dropdown menu items follows the same pattern used elsewhere in the component.
1891-1904: Good props organization for CollectionFileUploadThe props are well-organized and clearly show all the necessary data and callbacks being passed to the upload component.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
686-686: Use totalItems instead of totalCount (prevents wrong files count).These paths still use totalCount; backend and earlier mappings use totalItems. This causes incorrect file counts and inconsistent UI.
- files: updatedCl.totalCount || selectedFiles.length, + files: updatedCl.totalItems || selectedFiles.length, @@ - totalCount: updatedCl.totalCount, + totalCount: updatedCl.totalItems, @@ - files: updatedCl.totalCount || 0, + files: updatedCl.totalItems || 0, @@ - files: updatedCl.totalCount || 0, + files: updatedCl.totalItems || 0,Search for any remaining totalCount usages in this file:
#!/bin/bash rg -n -C2 '\btotalCount\b' --glob 'frontend/src/routes/_authenticated/knowledgeManagement.tsx'Also applies to: 707-707, 836-836, 912-912
494-529: Extract a single “API → UI collection” mapper to remove duplication.The data shaping logic is repeated across fetch, upload-complete refresh, create, add-files, and delete-item flows. Centralize mapping to reduce drift and keep totalItems/files/lastUpdated consistent.
If helpful, I can push a helper like mapApiCollectionToUi(collection, {items?, openOverride?}). Want me to draft it?
Also applies to: 558-592, 676-709, 827-860, 907-935
827-860: Fix potential TypeError when spreading an undefined collection.Spreading collectionsMap.get(addingToCollection.id) will throw if the key is missing.
- setCollections((prev) => { - const collectionsMap = new Map() - // Add existing collections - prev.forEach((col) => collectionsMap.set(col.id, col)) - // Update the specific collection - const updatedCollection = { - ...collectionsMap.get(addingToCollection.id), - files: updatedCl.totalCount || 0, - items: buildFileTree( - items.map((item: CollectionItem) => ({ - name: item.name, - type: item.type as "file" | "folder", - totalFileCount: item.totalFileCount, - updatedAt: item.updatedAt, - id: item.id, - updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", - })), - ), - lastUpdated: new Date(updatedCl.updatedAt).toLocaleString("en-GB", { - day: "numeric", - month: "short", - year: "numeric", - hour: "2-digit", - minute: "2-digit", - }), - updatedBy: updatedCl.lastUpdatedByEmail || "Unknown", - } - collectionsMap.set(addingToCollection.id, updatedCollection) - return Array.from(collectionsMap.values()) - }) + setCollections((prev) => { + const collectionsMap = new Map(prev.map((c) => [c.id, c])) + const existing = collectionsMap.get(addingToCollection.id) + if (!existing) { + // Key missing; construct safely from backend response + collectionsMap.set(addingToCollection.id, { + id: updatedCl.id, + name: updatedCl.name, + description: updatedCl.description, + files: updatedCl.totalItems || 0, + items: buildFileTree( + items.map((item: CollectionItem) => ({ + name: item.name, + type: item.type as "file" | "folder", + totalFileCount: item.totalFileCount, + updatedAt: item.updatedAt, + id: item.id, + updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", + })), + ), + lastUpdated: new Date(updatedCl.updatedAt).toLocaleString("en-GB", { + day: "numeric", + month: "short", + year: "numeric", + hour: "2-digit", + minute: "2-digit", + }), + updatedBy: updatedCl.lastUpdatedByEmail || "Unknown", + isPrivate: updatedCl.isPrivate, + totalCount: updatedCl.totalItems, + isOpen: true, + }) + } else { + const updatedCollection = { + ...existing, + files: updatedCl.totalItems || 0, + items: buildFileTree( + items.map((item: CollectionItem) => ({ + name: item.name, + type: item.type as "file" | "folder", + totalFileCount: item.totalFileCount, + updatedAt: item.updatedAt, + id: item.id, + updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", + })), + ), + lastUpdated: new Date(updatedCl.updatedAt).toLocaleString("en-GB", { + day: "numeric", + month: "short", + year: "numeric", + hour: "2-digit", + minute: "2-digit", + }), + updatedBy: updatedCl.lastUpdatedByEmail || "Unknown", + } + collectionsMap.set(addingToCollection.id, updatedCollection) + } + return Array.from(collectionsMap.values()) + })
🧹 Nitpick comments (6)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (6)
494-531: Preserve per-collection UI state when refreshing after upload.setCollections(updatedCollections) overwrites isOpen/items for all collections. Merge with previous state to avoid collapsing user-opened trees.
- setCollections(updatedCollections) + setCollections((prev) => { + const prevMap = new Map(prev.map((c) => [c.id, c])) + return data.map( + (collection: CollectionType & { items?: CollectionItem[] }) => { + const existing = prevMap.get(collection.id) + const isNewlyUploaded = + collection.name.toLowerCase() === + uploadingCollectionName.toLowerCase() + return { + id: collection.id, + name: collection.name, + description: collection.description, + files: collection.totalItems || 0, + items: + existing?.items && existing.items.length > 0 + ? existing.items + : buildFileTree( + (collection.items || []).map((item: CollectionItem) => ({ + name: item.name, + type: item.type as "file" | "folder", + totalFileCount: item.totalFileCount, + updatedAt: item.updatedAt, + id: item.id, + updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", + })), + ), + isOpen: + isNewlyUploaded || + (existing?.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, + } + }, + ) + })
1401-1416: Include files count for children in SimpleFileTree overlay.Overlay folder expansion omits files: item.totalFileCount; the list view includes it. Add for consistency.
- n.children = items.map( - (item: CollectionItem) => ({ + n.children = items.map( + (item: CollectionItem) => ({ id: item.id, name: item.name, type: item.type as "file" | "folder", + files: item.totalFileCount, lastUpdated: item.updatedAt, updatedBy: item.lastUpdatedByEmail || user?.email || "Unknown", isOpen: false, children: item.type === "folder" ? [] : undefined, }), )
1306-1313: Remove duplicate Tailwind class (double max-w).Duplicate max-w classes can cause unpredictable styling.
- <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">
744-759: Revoke object URLs to avoid memory leaks from previews.You create URL.createObjectURL for each selected file but never revoke it on removal/clear.
const handleRemoveFile = (id: string) => { - setSelectedFiles((prev) => prev.filter((f) => f.id !== id)) + setSelectedFiles((prev) => { + const target = prev.find((f) => f.id === id) + if (target?.preview) URL.revokeObjectURL(target.preview) + return prev.filter((f) => f.id !== id) + }) } const handleRemoveAllFiles = () => { - setSelectedFiles([]) + setSelectedFiles((prev) => { + prev.forEach((f) => f.preview && URL.revokeObjectURL(f.preview)) + return [] + }) }
609-616: Also revoke previews when closing modal.Ensure previews are cleaned up when closing.
const handleCloseModal = () => { + setSelectedFiles((prev) => { + prev.forEach((f) => f.preview && URL.revokeObjectURL(f.preview)) + return [] + }) setShowNewCollection(false) setAddingToCollection(null) setTargetFolder(null) setCollectionName("") - setSelectedFiles([]) setOpenDropdown(null) }
711-723: Update comment: this is Map-based, not Set-based.The comment says “Set-based approach” but the code uses Map. Align to avoid confusion.
- // Use Set-based approach to prevent duplicates + // Use Map-based approach to prevent duplicates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx(27 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
frontend/src/types/knowledgeBase.ts (1)
CollectionItem(20-58)frontend/src/utils/fileUtils.ts (1)
buildFileTree(162-219)frontend/src/api.ts (1)
api(4-4)
🔇 Additional comments (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (1)
1118-1132: Nice defensive error handling on failed content fetch.Clear, user-friendly messaging and proper JSON/text fallbacks.
* fix:duplicate collection listing * fix:listing of duplicate collection while updating a collection * fix ai raised issue * fix:raised issue * after new collection made level 1 will be shown --------- Co-authored-by: Rahul Kumar <rahul2004kumar14@gmail.com>
5 participants
Description
earlier while adding a new collection , ui listed same collection twice
fix
removed the duplicate listing bug using the map
Testing
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Style