feat(kb) : indexing kb with vespa frontend #1007
Conversation
frontend part of indexing
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 upload-status metadata and UploadStatus enum; migrates UploadStatus imports to shared/types; propagates uploadStatus/statusMessage/retryCount through knowledge management and file-tree builders; makes collection items non-selectable when indexing/failed; adds per-node upload status UI and onRetry callback; makes upload progress widget draggable; minor layout/typography tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as CollectionNavigation
participant State as Selection/Navigation
User->>UI: Click item
UI->>UI: isItemNonSelectable(item)?
alt Non-selectable
UI-->>User: ignore click, show tooltip
else Selectable
UI->>State: select or navigate
State-->>User: update selection / open folder
end
sequenceDiagram
autonumber
actor User
participant Tree as FileTree/FileNodeComponent
participant Parent as ParentComponent
User->>Tree: Click "Retry" (retryCount >= threshold)
alt onRetry provided
Tree->>Parent: onRetry(node, path)
Note right of Parent: retry flow (TODO)
else
Tree-->>User: no-op
end
sequenceDiagram
autonumber
actor User
participant Widget as UploadProgressWidget
User->>Widget: mousedown (start drag)
Widget->>Widget: capture offset
loop dragging
User->>Widget: mousemove
Widget->>Widget: update (x,y) within bounds
end
User->>Widget: mouseup (end drag)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Summary of ChangesHello @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 introduces significant frontend enhancements for the knowledge base feature, primarily focusing on providing real-time feedback on the indexing status of uploaded content. It integrates visual indicators for various upload states, intelligently disables interactions with items undergoing processing or those that have failed, and improves the usability of the upload progress widget by making it draggable. These changes aim to create a more transparent and intuitive experience for users managing their knowledge base content. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces UI enhancements to reflect the indexing status of knowledge base items, making the interface more informative for the user. Items that are pending, processing, or have failed indexing are now visually disabled and non-interactive. A key architectural improvement is the refactoring of the UploadStatus enum to a shared location. My review focuses on improving code quality by addressing code duplication, the use of any types, magic numbers, and hardcoded layout values. I've also identified a critical issue where a TODO for an incomplete feature has been left in the code, a potentially build-breaking import path, and some minor inconsistencies in UI styling.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (14)
frontend/src/components/UploadProgressWidget.tsx (5)
21-33: Guard window usage to avoid SSR/hydration crashesInitializing state with window.* during render can break in SSR or tests. Compute after mount or guard.
Apply this diff:
- const [position, setPosition] = useState<Position>(() => { - const padding = 6 - const widgetWidth = 480 - const widgetHeight = 150 // collapsed height - return { - x: window.innerWidth - widgetWidth - padding, - y: window.innerHeight - widgetHeight - padding - } - }) + const [position, setPosition] = useState<Position>({ x: 6, y: 6 }) + useEffect(() => { + const padding = 6 + const widgetWidth = 480 + const widgetHeight = 150 + if (typeof window !== 'undefined') { + setPosition({ + x: Math.max(padding, window.innerWidth - widgetWidth - padding), + y: Math.max(padding, window.innerHeight - widgetHeight - padding), + }) + } + }, [])
46-79: Use Pointer Events and rAF for smoother, mobile-friendly draggingMouse-only listeners miss touch; frequent setState on mousemove can jank. Prefer pointerdown/move/up and throttle via requestAnimationFrame.
Apply this diff skeleton (keeps behavior but modernizes input):
- useEffect(() => { - const handleMouseMove = (e: MouseEvent) => { ... setPosition(...) } - const handleMouseUp = () => { setIsDragging(false) } - if (isDragging) { - document.addEventListener('mousemove', handleMouseMove) - document.addEventListener('mouseup', handleMouseUp) - } - return () => { ...removeEventListener... } - }, [isDragging, dragOffset, isExpanded]) + useEffect(() => { + let raf = 0 + const onMove = (clientX: number, clientY: number) => { + const newX = clientX - dragOffset.x + const newY = clientY - dragOffset.y + const padding = 6 + const widgetWidth = 480 + const widgetHeight = isExpanded ? 400 : 150 + const maxX = window.innerWidth - widgetWidth - padding + const maxY = window.innerHeight - widgetHeight - padding + cancelAnimationFrame(raf) + raf = requestAnimationFrame(() => { + setPosition({ + x: Math.max(padding, Math.min(newX, maxX)), + y: Math.max(padding, Math.min(newY, maxY)), + }) + }) + } + const handlePointerMove = (e: PointerEvent) => { if (isDragging) onMove(e.clientX, e.clientY) } + const handlePointerUp = () => setIsDragging(false) + document.addEventListener('pointermove', handlePointerMove) + document.addEventListener('pointerup', handlePointerUp) + return () => { + document.removeEventListener('pointermove', handlePointerMove) + document.removeEventListener('pointerup', handlePointerUp) + cancelAnimationFrame(raf) + } + }, [isDragging, dragOffset, isExpanded])
53-59: Clamp using actual element size and handle viewport resizeHardcoding 480x150/400 can misclamp if styles change or content grows. Measure via ref and re-clamp on window resize.
Apply this diff:
- const widgetWidth = 480 - const widgetHeight = isExpanded ? 400 : 150 + const rect = widgetRef.current?.getBoundingClientRect() + const widgetWidth = rect?.width ?? 480 + const widgetHeight = rect?.height ?? (isExpanded ? 400 : 150)And add a resize effect:
+ useEffect(() => { + const onResize = () => { + const padding = 6 + const rect = widgetRef.current?.getBoundingClientRect() + const w = rect?.width ?? 480 + const h = rect?.height ?? (isExpanded ? 400 : 150) + setPosition(p => ({ + x: Math.max(padding, Math.min(p.x, window.innerWidth - w - padding)), + y: Math.max(padding, Math.min(p.y, window.innerHeight - h - padding)), + })) + } + window.addEventListener('resize', onResize) + return () => window.removeEventListener('resize', onResize) + }, [isExpanded])Also applies to: 81-94
114-123: Prefer transform for positioning; reduces layout/repaint costleft/top triggers layout each move; transform: translate3d(...) is smoother.
Apply this diff:
- style={{ - left: `${position.x}px`, - top: `${position.y}px`, - }} + style={{ + transform: `translate3d(${position.x}px, ${position.y}px, 0)`, + }}
159-177: Add accessible progress semanticsExpose progress via aria for screen readers.
Apply this diff:
- <div className="w-full bg-gray-200 dark:bg-gray-700 rounded-full h-2 mb-3"> + <div + className="w-full bg-gray-200 dark:bg-gray-700 rounded-full h-2 mb-3" + role="progressbar" + aria-label="Upload progress" + aria-valuemin={0} + aria-valuemax={100} + aria-valuenow={progressPercentage} + >server/scripts/kbFileStatusMigration.ts (3)
29-45: Safer null/empty handling for enum/text columnsIf uploadStatus is a Postgres enum, comparing to '' can error; prefer NULLIF/COALESCE to avoid type issues.
Apply this diff pattern (works for text columns; add casts if enum):
- statusMessage: sql`CASE - WHEN ${collectionItems.statusMessage} IS NULL OR ${collectionItems.statusMessage} = '' - THEN 'Successfully uploaded' - ELSE ${collectionItems.statusMessage} - END`, - uploadStatus: sql`CASE - WHEN ${collectionItems.uploadStatus} IS NULL OR ${collectionItems.uploadStatus} = '' - THEN ${UploadStatus.COMPLETED} - ELSE ${collectionItems.uploadStatus} - END`, + statusMessage: sql`COALESCE(NULLIF(${collectionItems.statusMessage}, ''), 'Successfully uploaded')`, + uploadStatus: sql`COALESCE(NULLIF(${collectionItems.uploadStatus}, ''), ${UploadStatus.COMPLETED})`,Repeat for collections block. If the column is an enum, cast the empty string comparison and default to the enum type, e.g.
NULLIF(${collections.uploadStatus}::text, '')::upload_status.Also applies to: 96-112
114-126: Batching without ORDER BY may thrash; add a stable orderLIMIT in a subquery without ORDER BY can update arbitrary rows; add ORDER BY id to improve predictability and reduce rechecks.
Apply this diff:
- LIMIT ${BATCH_SIZE} + ORDER BY ${collectionItems.id} + LIMIT ${BATCH_SIZE}And similarly for collections.
Also applies to: 47-59
285-287: Avoid auto-running on import; gate main() behind direct-execution checkCalling main() on import can break tests/tools that import this module.
Apply this diff (ESM-safe):
-// Run the migration -main() +// Run the migration if executed directly +if (import.meta.url === `file://${process.argv[1]}`) { + // eslint-disable-next-line no-console + main() +}frontend/src/utils/fileUtils.ts (1)
4-4: Import UploadStatus as a type to prevent bundling the enum.UploadStatus is only used in types here. Switch to a type‑only import.
-import { FileType, MIME_TYPE_MAPPINGS, EXTENSION_MAPPINGS, UploadStatus } from "shared/types" +import { FileType, MIME_TYPE_MAPPINGS, EXTENSION_MAPPINGS } from "shared/types" +import type { UploadStatus } from "shared/types"frontend/src/components/CollectionNavigation.tsx (2)
58-63: Revisit non‑selectable rule for failed items.You currently block interaction for "failed" as well as "pending/processing". Product-wise, failed items might be selectable for retry/requeue flows. Confirm desired behavior.
309-320: Reduce TooltipProvider nesting.You instantiate TooltipProvider per row. Wrap the list once with a single provider for lighter DOM and fewer providers.
Also applies to: 604-614
frontend/src/components/FileTree.tsx (1)
148-172: Consider hoisting TooltipProvider.Multiple per‑row providers add overhead. Wrap the FileTree once with a single TooltipProvider.
Also applies to: 186-210
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
1823-1826: Wire up onRetry.Hook this to a backend endpoint that requeues indexing for
node.id, then refresh the affected collection’s items (similar to your delete flow). I can draft the handler once you confirm the endpoint path/signature.
462-480: DRY the "map CollectionItem -> FileNode" transformation.You repeat the same mapping in several places. Extract a small helper like
toFileNode(item, fallbackEmail)to avoid divergence.Also applies to: 626-638, 821-836, 941-949, 1553-1572, 1687-1702, 1856-1876
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/src/components/CollectionNavigation.tsx(9 hunks)frontend/src/components/FileTree.tsx(10 hunks)frontend/src/components/UploadProgressWidget.tsx(4 hunks)frontend/src/routes/_authenticated/agent.tsx(1 hunks)frontend/src/routes/_authenticated/knowledgeManagement.tsx(11 hunks)frontend/src/types/knowledgeBase.ts(2 hunks)frontend/src/utils/fileUtils.ts(4 hunks)server/api/knowledgeBase.ts(2 hunks)server/db/schema/knowledgeBase.ts(1 hunks)server/queue/fileProcessor.ts(2 hunks)server/scripts/kbFileStatusMigration.ts(1 hunks)server/shared/types.ts(1 hunks)server/types.ts(0 hunks)server/worker.ts(1 hunks)
💤 Files with no reviewable changes (1)
- server/types.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-23T07:43:59.494Z
Learnt from: rahul1841
PR: xynehq/xyne#582
File: frontend/src/routes/_authenticated/agent.tsx:1654-1654
Timestamp: 2025-06-23T07:43:59.494Z
Learning: In the agent test interface dropdown (frontend/src/routes/_authenticated/agent.tsx), the agent selection dropdown specifically uses `allAgentsList` only and should only check `allAgentsList.length > 0` for visibility. It does not need to check other agent lists like `madeByMeAgentsList` or `sharedToMeAgentsList` because the dropdown is designed to show agents from the "all agents" tab only.
Applied to files:
frontend/src/routes/_authenticated/agent.tsx
📚 Learning: 2025-09-15T12:50:44.501Z
Learnt from: naSim087
PR: xynehq/xyne#828
File: frontend/src/components/GoogleDriveNavigation.tsx:211-216
Timestamp: 2025-09-15T12:50:44.501Z
Learning: In GoogleDriveNavigation.tsx, folder navigation from search results is not intended functionality - users should not be able to navigate into searched folders.
Applied to files:
frontend/src/components/CollectionNavigation.tsx
🧬 Code graph analysis (3)
frontend/src/components/CollectionNavigation.tsx (1)
frontend/src/components/ui/tooltip.tsx (4)
TooltipProvider(28-28)Tooltip(28-28)TooltipTrigger(28-28)TooltipContent(28-28)
frontend/src/components/FileTree.tsx (2)
frontend/src/utils/fileUtils.ts (1)
FileNode(134-146)frontend/src/components/ui/tooltip.tsx (4)
TooltipProvider(28-28)Tooltip(28-28)TooltipTrigger(28-28)TooltipContent(28-28)
frontend/src/components/UploadProgressWidget.tsx (1)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(41-47)
🔇 Additional comments (7)
frontend/src/routes/_authenticated/agent.tsx (1)
2724-2726: Typography tweak looks goodHeader size change to text-[32px] is safe; no logic impact.
server/scripts/kbFileStatusMigration.ts (2)
10-12: Import path change LGTMSwitch to "@/shared/types" aligns enum reuse across layers.
241-259: API helpers look goodTyped updates with UploadStatus and retryCount with NOW() are clean.
Also applies to: 264-283
frontend/src/types/knowledgeBase.ts (1)
60-62: New display fields look good.Optional fields for uploadStatus/statusMessage/retryCount align with UI needs.
frontend/src/utils/fileUtils.ts (1)
143-146: Propagation of upload metadata into FileNode and buildFileTree is consistent.No issues found with typing or mapping.
If there are sources that populate only file nodes (not folders), confirm folders won’t inadvertently show stale statuses. If needed, gate assignment by
file.type === "file".Also applies to: 156-159, 189-191
frontend/src/components/CollectionNavigation.tsx (1)
603-614: Same tooltip fix for main list.Apply the status‑aware tooltip here too.
- <TooltipContent> - <p>Indexing is in progress</p> - </TooltipContent> + <TooltipContent> + <p> + {item.uploadStatus === "failed" + ? (item.statusMessage || "Indexing failed") + : "Indexing is in progress"} + </p> + </TooltipContent>frontend/src/routes/_authenticated/knowledgeManagement.tsx (1)
633-636: Same cast removal as above.Refer to earlier comment; apply the same edit across these occurrences.
Also applies to: 830-833, 944-946, 1564-1566, 1697-1700, 1871-1873
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/src/types/knowledgeBase.ts (1)
1-1: Consider using a type-only import.Since
UploadStatusis used purely for type annotations in this file, marking it asimport typeavoids bundling the enum at runtime and clarifies intent.Apply this diff:
-import { UploadStatus } from "shared/types" +import type { UploadStatus } from "shared/types"frontend/src/components/FileTree.tsx (2)
58-90: Tighten theuploadStatusprop type.The component accepts
uploadStatus: string, but it should use theUploadStatustype for type safety and autocomplete.Apply this diff:
+import type { UploadStatus } from "shared/types" + // Reusable upload status indicator component const UploadStatusIndicator = ({ uploadStatus, statusMessage }: { - uploadStatus: string + uploadStatus: UploadStatus statusMessage?: string }) => {
236-255: Consider gating retry on upload status.The retry button appears when
retryCount > 3, but doesn't check if the item is actually in afailedstate. This could show retry for items that succeeded or are still processing.Optionally tighten the condition:
- {(node.retryCount ?? 0) > 3 && onRetry && ( + {onRetry && node.uploadStatus === "failed" && (node.retryCount ?? 0) > 3 && (And consider extracting the magic number:
const MAX_RETRIES_FOR_UI = 3 // Then use: (node.retryCount ?? 0) > MAX_RETRIES_FOR_UIfrontend/src/components/CollectionNavigation.tsx (2)
58-62: Use theUploadStatustype for the parameter.The function accepts
{ uploadStatus?: string }, but should use theUploadStatustype for consistency and type safety.Apply this diff:
+import type { UploadStatus } from "shared/types" + // Helper function to check if an item should be non-selectable based on upload status -function isItemNonSelectable(item: { uploadStatus?: string }): boolean { +function isItemNonSelectable(item: { uploadStatus?: UploadStatus }): boolean {
64-78: Consider making the tooltip message dynamic.The tooltip always shows "Indexing is in progress", even for failed items. Optionally, accept
uploadStatusandstatusMessageprops to show context-aware messages.Example:
const IndexingTooltip = ({ + uploadStatus, + statusMessage, +}: { + uploadStatus?: UploadStatus + statusMessage?: string +}) => { return ( <TooltipProvider> <Tooltip> <TooltipTrigger asChild> <AlertOctagon className="w-4 h-4 ml-2 text-gray-500" /> </TooltipTrigger> <TooltipContent> - <p>Indexing is in progress</p> + <p>{statusMessage || (uploadStatus === "failed" ? "Indexing failed" : "Indexing is in progress")}</p> </TooltipContent> </Tooltip> </TooltipProvider> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/CollectionNavigation.tsx(9 hunks)frontend/src/components/FileTree.tsx(10 hunks)frontend/src/components/UploadProgressWidget.tsx(4 hunks)frontend/src/types/knowledgeBase.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/CollectionNavigation.tsx (1)
frontend/src/components/ui/tooltip.tsx (4)
TooltipProvider(28-28)Tooltip(28-28)TooltipTrigger(28-28)TooltipContent(28-28)
frontend/src/components/UploadProgressWidget.tsx (1)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(41-47)
frontend/src/components/FileTree.tsx (1)
frontend/src/utils/fileUtils.ts (1)
FileNode(134-146)
🔇 Additional comments (16)
frontend/src/components/UploadProgressWidget.tsx (8)
1-4: LGTM! Imports are appropriate.The new imports (useEffect, useRef, Loader2) are all used in the implementation and properly organized.
9-18: Good refactor: constants extracted as suggested.The layout constants are now properly defined at the top of the component, addressing the previous review feedback about hardcoded values. The Position interface is clear and well-typed.
27-35: LGTM! Drag state properly initialized.The initial position calculation correctly places the widget at the bottom-right corner with appropriate padding, and all drag-related state is properly typed and initialized.
49-80: LGTM! Drag constraints and event handling are correct.The mousemove/mouseup handlers properly constrain the widget within viewport bounds with appropriate padding, dynamically adjust for expanded/collapsed states, and correctly clean up event listeners.
82-92: LGTM! Position adjustment on expand/collapse prevents overflow.This effect correctly recalculates bounds and adjusts the widget position when expanding/collapsing to prevent viewport overflow. Using the updater function pattern with setPosition is appropriate.
244-246: LGTM! Loader2 icon improves upload status feedback.Replacing the static indicator with an animated Loader2 spinner for pending/uploading states provides better visual feedback to users. The size and styling are consistent with other status icons.
188-218: LGTM! Tab styling is consistent and accessible.The updated tab styles with rounded borders and visible active state borders provide clear visual feedback. The styling is consistently applied across all three tabs.
38-47: Dragging still starts from the entire widget, stealing clicks from buttons.This implementation still captures mousedown on the entire widget container (line 122), which interferes with the expand/collapse button, cancel button, and tab buttons. The previous review comment specifically requested limiting drag initiation to the header or filtering out interactive elements.
Apply the previously suggested fix to limit drag to the header and ignore non-primary clicks:
const handleMouseDown = (e: React.MouseEvent) => { + if (e.button !== 0) return // left-click only + const target = e.target as HTMLElement + const interactive = ['BUTTON','INPUT','TEXTAREA','A','SELECT','LABEL','SVG','PATH'] + if (interactive.includes(target.tagName)) return if (widgetRef.current) { const rect = widgetRef.current.getBoundingClientRect()And update the container/header to scope the drag handler:
- <div + <div ref={widgetRef} - className="fixed z-50 bg-gray-50 dark:bg-gray-700 border border-gray-200 dark:border-gray-700 rounded-3xl shadow-lg cursor-move select-none" + className={`fixed z-50 bg-gray-50 dark:bg-gray-700 border border-gray-200 dark:border-gray-700 rounded-3xl shadow-lg ${isDragging ? 'select-none cursor-grabbing' : ''}`} style={{ left: `${position.x}px`, top: `${position.y}px`, width: `${WIDGET_WIDTH}px`, - cursor: isDragging ? 'grabbing' : 'grab' }} - onMouseDown={handleMouseDown} > {/* Header */} - <div className="p-4 rounded-t-3xl"> + <div className="p-4 rounded-t-3xl cursor-move" onMouseDown={handleMouseDown}>frontend/src/types/knowledgeBase.ts (1)
60-62: LGTM!The new optional fields are well-typed and appropriately optional, allowing gradual adoption across collection items. They propagate correctly to
Filevia the extends relationship.frontend/src/components/FileTree.tsx (4)
10-13: LGTM!Icon imports follow the recommended pattern for lucide-react, using named imports for tree-shaking.
99-99: LGTM!The
onRetryprop is correctly typed, optional, and propagated consistently through the component tree.
182-208: LGTM!Upload status indicators are cleanly integrated for both folders and files, eliminating the code duplication flagged in previous reviews.
240-255: LGTM!The retry button implementation correctly stops event propagation and triggers the callback with the node and path.
frontend/src/components/CollectionNavigation.tsx (3)
2-10: LGTM!New imports are used in the IndexingTooltip component and non-selectable item indicators.
229-324: LGTM!Non-selectable items in search results are comprehensively handled: interactions are disabled, styling is muted, and a tooltip provides context.
406-614: LGTM!Non-selectable items in the current view are comprehensively handled: folder navigation is gated, checkboxes are disabled, styling is muted, and tooltips provide context. The chevron for folders correctly appears only for selectable items.
4 participants
Description
Testing
Summary by CodeRabbit
New Features
Improvements
UI Polish
Chores