fix/feedback-modal: changed mechanism of form filling #757
Conversation
MayankBansal2004
requested review from
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. WalkthroughThis PR mixes formatting cleanups with several functional updates: a refactor of EnhancedReasoning parsing/rendering, a new MCP Tools management modal and update flow, expanded chat route params and feedback handling via actionable toasts, an admin endpoint to delete user data, and enhanced knowledge-base search and API adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant UI as MCP Integrations UI
participant API as Admin API
participant Conn as Connector Tools Service
Admin->>UI: Click "Manage Tools"
UI->>API: GET /admin/connector/{clientId}/tools
API-->>UI: Tools list
UI->>UI: Select/deselect tools (state tracked)
Admin->>UI: Close modal
UI->>UI: Diff initial vs current selections
UI->>API: POST /admin/tools/update_status (changes)
API->>Conn: Update tool enablement
API-->>UI: Success/Failure
UI->>Admin: Show toast and refresh list
sequenceDiagram
autonumber
actor User
participant Chat as Chat UI
participant API as Message Feedback API
participant Toast as Toast System
participant Modal as Feedback Modal
User->>Chat: Click Like/Dislike
Chat->>Chat: Optimistically update feedbackMap
Chat->>API: POST /message/feedback
alt success & Like
API-->>Chat: OK
Chat->>Toast: Success toast (2s)
else success & Dislike
API-->>Chat: OK
Chat->>Toast: Toast with "Yes" action
User-->>Toast: Click "Yes"
Toast->>Chat: Trigger pending feedback
Chat->>Modal: Open detailed feedback modal
else failure
API-->>Chat: Error
Chat->>Chat: Revert optimistic update
Chat->>Toast: Destructive error toast
end
sequenceDiagram
autonumber
actor Admin as Admin
participant API as AdminDeleteUserData
participant Vespa as Vespa Data Store
participant Jobs as Sync Jobs Service
Admin->>API: DELETE user data (payload)
API->>API: Validate admin & parse payload
API->>Vespa: Delete user documents
alt clear sync jobs requested
API->>Jobs: Clear related jobs
end
API-->>Admin: { success: true } or 500
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 (10)
🚧 Files skipped from review as they are similar to previous changes (8)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-07-29T09:29:29.401ZApplied to files:
🧬 Code graph analysis (2)frontend/src/routes/_authenticated/chat.tsx (3)
frontend/src/utils/fileUtils.ts (2)
🔇 Additional comments (8)
✨ 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 @MayankBansal2004, 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 primarily refines the user feedback mechanism, ensuring the feedback modal is presented more contextually after a 'dislike' action, with an optional follow-up form. Beyond this functional change, a substantial portion of the PR focuses on code quality improvements through extensive formatting and refactoring across various frontend components and backend utilities. These changes aim to enhance code readability, maintainability, and consistency throughout the codebase.
Highlights
- Feedback Mechanism Update: The feedback modal now exclusively appears upon 'dislike' actions, with form submission being optional and triggered via a toast notification, streamlining the user feedback flow.
- Extensive Code Formatting: Numerous frontend and backend files received significant formatting adjustments, including line breaks, quote consistency, and improved readability for function signatures and object structures.
- Enhanced Reasoning Display Logic: Adjustments were made to how reasoning steps are parsed and displayed within the UI, including refinements to iteration summaries and progress state tracking.
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 introduces a significant improvement to the user feedback mechanism by making the detailed feedback form optional, appearing only after a user dislikes a response and clicks an action in a toast notification. This is a great UX enhancement. The PR also includes a large number of formatting changes across the codebase, which improve code consistency and readability. I've identified one minor issue related to JSX structure which has been fixed. Overall, the changes are positive and well-implemented.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (24)
server/utils/encryption.ts (2)
10-20: Enforce non-empty, 256‑bit base64 key before assignment.Currently an empty string passes validation and fails later at runtime; also assign the key only after validation.
constructor(key: string) { - this.key = key - - if ( - key && - Buffer.from(key, this.encoding).byteLength !== - this.encryptionKeyByteLength - ) { - throw new Error("Encryption key must be base64-encoded and 256-bit long.") - } + if (!key) { + throw new Error("Encryption key is required.") + } + const keyBuf = Buffer.from(key, "base64") + if (keyBuf.byteLength !== this.encryptionKeyByteLength) { + throw new Error("Encryption key must be base64-encoded and 256-bit long.") + } + this.key = key }
44-60: Harden input validation in decrypt (types and lengths).Validate field types and ensure IV=12 bytes and authTag=16 bytes for AES‑GCM.
public decrypt(encryptedStr: string): string { - const { ciphertext, iv, authTag } = JSON.parse(encryptedStr) + const { ciphertext, iv, authTag } = JSON.parse(encryptedStr) - if (!ciphertext || !iv || !authTag) { + if ( + typeof ciphertext !== "string" || + typeof iv !== "string" || + typeof authTag !== "string" + ) { throw new Error("Invalid encrypted string format.") } + const ivBuf = Buffer.from(iv, this.encoding) + const tagBuf = Buffer.from(authTag, this.encoding) + if (ivBuf.byteLength !== 12 || tagBuf.byteLength !== 16) { + throw new Error("Invalid IV or auth tag length.") + } + const decipher = crypto.createDecipheriv( this.algo, Buffer.from(this.key, this.encoding), - Buffer.from(iv, this.encoding), + ivBuf, ) - decipher.setAuthTag(Buffer.from(authTag, this.encoding)) + decipher.setAuthTag(tagBuf)frontend/src/routes/_authenticated/admin/integrations/google.tsx (1)
251-256: Fix scopes field to be a controlled string and validate emptiness correctly.
valueis an array while<input>expects a string; validation with!valuelets empty arrays pass. Convert to a comma-separated string for display and validate by length.- <Input + <Input id="scopes" type="text" - value={field.state.value} - onChange={(e) => field.handleChange(e.target.value.split(","))} + value={ + Array.isArray(field.state.value) + ? field.state.value.join(",") + : (field.state.value ?? "") + } + onChange={(e) => + field.handleChange( + e.target.value + .split(",") + .map((s) => s.trim()) + .filter(Boolean), + ) + } placeholder="Enter OAuth scopes" />Also tighten the validator just above:
- onChange: ({ value }) => (!value ? "scopes are required" : undefined), + onChange: ({ value }) => + Array.isArray(value) + ? value.length === 0 ? "Scopes are required" : undefined + : !String(value || "").trim() + ? "Scopes are required" + : undefined,server/api/admin.ts (1)
1006-1082: Enforce admin-role check in AdminDeleteUserData
Add a verification that the caller’s role is “admin” (not just that their account exists) before proceeding—e.g. immediately afterconst { sub } = c.get(JwtPayloadKey)in server/api/admin.ts:1006.frontend/src/components/PdfViewer.tsx (1)
464-471: Retry button does not reload the document (infinite spinner risk).Clicking Retry only toggles state but never re-triggers the load effect (the effect depends on sourceKey/initialPage, not currentPage). Result: loading=true with no new fetch.
Apply this diff to add a reload key and use it in the effect + button:
@@ const [currentVisiblePage, setCurrentVisiblePage] = useState<number>(1) const observerRef = useRef<IntersectionObserver | null>(null) + const [reloadKey, setReloadKey] = useState(0) @@ - }, [sourceKey, initialPage]) + }, [sourceKey, initialPage, reloadKey]) @@ <button onClick={() => { setError(null) - setLoading(true) - // Re-trigger the effect by changing a dependency - setCurrentPage(initialPage) + setReloadKey((k) => k + 1) }}server/search/vespa.ts (1)
846-902: Escape YQL values to prevent injection/parse errors.Raw IDs are interpolated into YQL (clId/docId). A single quote in an ID breaks the query and poses injection risk. Use existing escapeYqlValue.
- const collectionCondition = `(${collectionIds.map((id: string) => `clId contains '${id.trim()}'`).join(" or ")})` + const collectionCondition = `(${collectionIds + .map((id: string) => `clId contains '${escapeYqlValue(id.trim())}'`) + .join(" or ")})` - const folderCondition = `(${clVespaIds.map((id: string) => `docId contains '${id.trim()}'`).join(" or ")})` + const folderCondition = `(${clVespaIds + .map((id: string) => `docId contains '${escapeYqlValue(id.trim())}'`) + .join(" or ")})` - const fileCondition = `(${clVespaIds.map((id: string) => `docId contains '${id.trim()}'`).join(" or ")})` + const fileCondition = `(${clVespaIds + .map((id: string) => `docId contains '${escapeYqlValue(id.trim())}'`) + .join(" or ")})`frontend/src/lib/common.tsx (2)
135-142: Fix unreachable fallback and incorrect icon for SystemInfo.Because of the
|| entity === SystemEntity.SystemInfo, the GitHub branch renders for anySystemInfo(except the earlier KnowledgeBase case), making the PlugZap fallback unreachable. Scope the GitHub condition to MCP only and let otherSystemInfofall through to the intended fallback.Apply this diff:
- } else if ( - (app === Apps.Github && entity === ConnectorType.MCP) || - entity === SystemEntity.SystemInfo - ) { - return <Github size={size?.w || 12} className={classNameVal} /> - } else if (entity === SystemEntity.SystemInfo) { - return <PlugZap size={size?.w || 12} className={classNameVal} /> // Fallback for other MCPs + } else if (app === Apps.Github && entity === ConnectorType.MCP) { + return <Github size={size?.w || 12} className={classNameVal} /> + } else if (entity === SystemEntity.SystemInfo) { + return <PlugZap size={size?.w || 12} className={classNameVal} /> // Fallback for other MCPs
156-160: Add a Tailwind safelist or switch to inline style for dynamic minHeight
Thefrontend/tailwind.config.jscontains nosafelist, somin-h-[${minHeight}px]will be purged at build time and won’t generate any CSS. Either convert to an inline style:- <div className={`min-h-[${minHeight}px] w-full flex items-center justify-center`}> + <div style={{ minHeight }} className="w-full flex items-center justify-center">or add a safelist entry in
tailwind.config.js, for example:module.exports = { // … safelist: [ { pattern: /^min-h-\[\d+px\]$/, }, ], };frontend/src/hooks/useChatStream.ts (3)
201-219: Close EventSource before refresh to avoid leaks and double streams.
es.close()is commented out inonerror, so the old connection can persist while a new one is created after refresh.Apply this diff:
- es.onerror = async () => { - // es.close() + es.onerror = async () => { + try { es.close() } catch {} if (!triedRefresh) { triedRefresh = true // Attempt token refresh
758-759: Remove double-encoding of messageId.
URLSearchParams.appendalready encodes; wrapping withencodeURIComponentyields%25-escaped IDs server-side.Apply this diff:
- url.searchParams.append("messageId", encodeURIComponent(messageId)) + url.searchParams.append("messageId", messageId)
905-912: Notify subscribers on retry image-citation updates.Without a notify, UI may not react to new image citations during retries.
Apply this diff:
eventSource.addEventListener( ChatSSEvents.ImageCitationUpdate, (event) => { const imageCitation: ImageCitation = JSON.parse(event.data) streamState.imageCitations = imageCitation + notifySubscribers(retryStreamKey) }, )frontend/src/components/EnhancedReasoning.tsx (2)
75-86: Fix citation indexing and avoid dropping unmatched refsIndex into citationMap using a parsed number and keep the original “[n]” when no URL/mapping exists. Prevents TS index-type errors and preserves text fidelity.
Apply:
- if (citationMap) { - return text.replace(textToCitationIndex, (match, num) => { - const index = citationMap[num] - const url = citationUrls[index] - return typeof index === "number" && url ? `[[${index + 1}]](${url})` : "" // Remove citation if no mapping found - }) - } else { - return text.replace(textToCitationIndex, (match, num) => { - const url = citationUrls[num - 1] - return url ? `[[${num}]](${url})` : "" // Remove citation if no URL found - }) - } + if (citationMap) { + return text.replace(textToCitationIndex, (match, num) => { + const n = Number.parseInt(num, 10) + if (Number.isNaN(n)) return match + const index = citationMap[n] + const url = typeof index === "number" ? citationUrls[index] : undefined + return url ? `[[${(index as number) + 1}]](${url})` : match + }) + } else { + return text.replace(textToCitationIndex, (match, num) => { + const n = Number.parseInt(num, 10) + if (Number.isNaN(n)) return match + const url = citationUrls[n - 1] + return url ? `[[${n}]](${url})` : match + }) + }
278-281: Avoid Map key ‘undefined’ for fallback stepsFallback steps don’t have stepId; using
step.stepId!stores them under an undefined key and overwrites prior entries.- // Add consolidated summary as a regular step - steps.push(step) - stepMap.set(step.stepId!, step) + // Add fallback step with a stable id + const fallbackId = `fallback_${lineIndex}` + step.stepId = fallbackId + steps.push(step) + stepMap.set(fallbackId, step)frontend/src/utils/fileUtils.ts (1)
95-116: Deduplicate by path, not just filenameCurrent logic drops files with the same name from different folders. Use webkitRelativePath (fallback to name) for the dedupe key.
- // Create a map to track files by name for deduplication - const fileMap = new Map<string, File>() + // Track by relative path (fallback to name) for deduplication + const fileMap = new Map<string, File>() let duplicateCount = 0 @@ - validFiles.forEach((file) => { - if (!fileMap.has(file.name)) { - fileMap.set(file.name, file) - } else { - duplicateCount++ - } - }) + const getKey = (f: File) => + (f as any).webkitRelativePath && (f as any).webkitRelativePath.length > 0 + ? (f as any).webkitRelativePath + : f.name + validFiles.forEach((file) => { + const key = getKey(file) + if (!fileMap.has(key)) fileMap.set(key, file) + else duplicateCount++ + })frontend/src/routes/_authenticated/chat.tsx (3)
260-318: Fix XSS risk: sanitize generated HTML and restrict link protocols.jsonToHtmlMessage builds raw and returns a string that’s injected via dangerouslySetInnerHTML. DOMPurify is applied before parsing (to the JSON string), not to the final HTML, so malicious URLs like javascript: can slip through.
- Sanitize the final HTML string.
- Enforce allowed protocols (http, https, mailto, tel) when building anchors.
Apply:
@@ -const jsonToHtmlMessage = (jsonString: string): string => { +const jsonToHtmlMessage = (jsonString: string): string => { try { const parts = JSON.parse(jsonString) as Array<ParsedMessagePart> @@ - return parts + const html = parts .map((part, index) => { let htmlPart = "" if (part.type === "text") { htmlPart = part.value } else if ( part.type === "pill" && part.value && typeof part.value === "object" ) { @@ htmlPart = renderToStaticMarkup( React.createElement(Pill, { newRef: referenceForPill }), ) } else if (part.type === "link" && typeof part.value === "string") { - const url = part.value - // Create a simple anchor tag string for links - // Ensure it has similar styling to how it's created in ChatBox - // The text of the link will be the URL itself - htmlPart = `<a href="${url}" target="_blank" rel="noopener noreferrer" class="text-blue-600 dark:text-blue-400 underline hover:text-blue-800 dark:hover:text-blue-300 cursor-pointer">${url}</a>` + const raw = part.value.trim() + const isSafeUrl = (u: string) => { + try { + const p = new URL(u, window.location.origin) + return ["http:", "https:", "mailto:", "tel:"].includes(p.protocol) + } catch { + return false + } + } + const safeUrl = isSafeUrl(raw) ? raw : "#" + htmlPart = `<a href="${safeUrl}" target="_blank" rel="noopener noreferrer nofollow" class="text-blue-600 dark:text-blue-400 underline hover:text-blue-800 dark:hover:text-blue-300 cursor-pointer">${safeUrl}</a>` } @@ - .join("") - .trimEnd() + .join("") + .trimEnd() + // Sanitize the final HTML (restrict URIs to http(s)/mailto/tel) + return DOMPurify.sanitize(html, { + ALLOWED_URI_REGEXP: /^(?:(?:https?|mailto|tel):)/i, + } as any) } catch (error) { return jsonString } }And remove pre-parse sanitize at render site:
- __html: jsonToHtmlMessage(DOMPurify.sanitize(message)), + __html: jsonToHtmlMessage(message),Also applies to: 2576-2581
811-877: Don’t lose previous feedback on failure; restore prior state.If the POST fails, you delete the key, potentially erasing an existing like/dislike. Capture the previous value and restore it on error.
const handleFeedback = async ( messageId: string, feedback: MessageFeedback, ) => { if (!messageId) return - - // Optimistically update the UI - setFeedbackMap((prev) => ({ - ...prev, - [messageId]: feedback, - })) + // Capture previous to enable proper rollback + const prevFeedback = feedbackMap[messageId] + // Optimistically update + setFeedbackMap((prev) => ({ ...prev, [messageId]: feedback })) @@ } catch (error) { console.error("Failed to submit feedback:", error) - // Revert optimistic update on error - setFeedbackMap((prev) => { - const newMap = { ...prev } - delete newMap[messageId] - return newMap - }) + // Revert to previous value precisely + setFeedbackMap((prev) => { + if (prevFeedback !== undefined) { + return { ...prev, [messageId]: prevFeedback } + } + const { [messageId]: _, ...rest } = prev + return rest + })
889-934: Enhanced feedback: also restore previous value on error.Same rollback issue exists here; preserve the pre-submit value.
if (!pendingFeedback) return const { messageId } = pendingFeedback + const prevFeedback = feedbackMap[messageId] @@ } catch (error) { console.error("Failed to submit enhanced feedback", error) - // Revert optimistic update on error - setFeedbackMap((prev) => { - const newMap = { ...prev } - delete newMap[messageId] - return newMap - }) + setFeedbackMap((prev) => { + if (prevFeedback !== undefined) { + return { ...prev, [messageId]: prevFeedback } + } + const { [messageId]: _, ...rest } = prev + return rest + })server/api/knowledgeBase.ts (2)
1464-1481: Incorrect collection membership check in GetFilePreviewApi.Walking parentId to compare against a collectionId (different domains) is wrong. parentId relates to collection_items.id, not collections.id. This can return false 404s and/or allow cross-collection leakage.
Apply this diff to verify by collectionId directly:
- // Verify item belongs to this Collection by traversing up the hierarchy - let currentItem = item - let belongsToCollection = false - while (currentItem.parentId) { - if (currentItem.parentId === collectionId) { - belongsToCollection = true - break - } - const parent = await getCollectionItemById(db, currentItem.parentId) - if (!parent) break - currentItem = parent - } - - if (!belongsToCollection) { + // Verify item belongs to this Collection + if (item.collectionId !== collectionId) { throw new HTTPException(404, { message: "File not found in this knowledge base", }) }
1516-1536: GetFileContentApi misses auth and collection checks (direct file exfiltration risk).This endpoint serves file bytes without verifying user access or collection membership. A user who knows an itemId could fetch content from another user’s private collection.
Apply this diff to mirror the checks from GetFilePreviewApi and validate 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) + // Authn: get user + const users = await getUserByEmail(db, userEmail) + if (!users || users.length === 0) { + throw new HTTPException(404, { message: "User not found" }) + } + const user = users[0] + + // Authz: get collection and check 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", + }) + } + + // Validate item and membership + const item = await getCollectionItemById(db, itemId) + if (!item || item.type !== "file" || item.collectionId !== collectionId) { + throw new HTTPException(404, { message: "File not found" }) + } + + const collectionFile = await getCollectionFileByItemId(db, itemId) if (!collectionFile) { throw new HTTPException(404, { message: "File data not found" }) } // Read file content if (!collectionFile.storagePath) { throw new HTTPException(404, { message: "File storage path not found" }) } const fileContent = await readFile(collectionFile.storagePath)server/db/knowledgeBase.ts (1)
214-287: Fix collection totalItems undercount on folder delete.When deleting a folder, totalItems is decremented by only direct children + 1, not all descendants, causing drift in collection counters.
Apply this diff to count all descendants and update totals accurately:
- if (item.type === "folder") { - const markDescendantsAsDeleted = async ( - parentId: string, - ): Promise<number> => { + if (item.type === "folder") { + const markDescendantsAsDeleted = async ( + parentId: string, + ): Promise<{ filesDeleted: number; itemsDeleted: number }> => { const children = await trx .select() .from(collectionItems) .where( and( eq(collectionItems.parentId, parentId), isNull(collectionItems.deletedAt), ), ) - // We only want to decrement file counts for parents - let filesDeleted = children.filter((c) => c.type === "file").length + // Track both file-only and total item counts + let filesDeleted = children.filter((c) => c.type === "file").length + let itemsDeleted = children.length // Mark children as deleted if (children.length > 0) { await trx .update(collectionItems) .set({ deletedAt: sql`NOW()`, updatedAt: sql`NOW()`, }) .where( and( eq(collectionItems.parentId, parentId), isNull(collectionItems.deletedAt), ), ) // Recursively delete descendants of folder children for (const child of children) { if (child.type === "folder") { - filesDeleted += await markDescendantsAsDeleted(child.id) + const res = await markDescendantsAsDeleted(child.id) + filesDeleted += res.filesDeleted + itemsDeleted += res.itemsDeleted } } } - return filesDeleted + return { filesDeleted, itemsDeleted } } // Mark the folder itself as deleted await trx .update(collectionItems) .set({ deletedAt: sql`NOW()`, updatedAt: sql`NOW()`, }) .where(eq(collectionItems.id, itemId)) - // Mark all descendants as deleted and get count of files only - const descendantFilesCount = await markDescendantsAsDeleted(itemId) + // Mark all descendants as deleted and get counts + const { filesDeleted: descendantFilesCount, itemsDeleted } = + await markDescendantsAsDeleted(itemId) - // Get direct children count for collection total (includes both files and folders) - const directChildren = await trx - .select() - .from(collectionItems) - .where( - and( - eq(collectionItems.parentId, itemId), - isNull(collectionItems.deletedAt), - ), - ) - - // Update collection total count (all descendants + the folder itself) - await updateCollectionTotalCount( - trx, - item.collectionId, - -(directChildren.length + 1), - ) + // Update collection total count: all descendants + the folder itself + await updateCollectionTotalCount(trx, item.collectionId, -(itemsDeleted + 1)) // Update parent folder counts (decrement only file count from parent) if (item.parentId) { await updateParentFolderCounts(trx, item.parentId, -descendantFilesCount) }Also applies to: 268-287
server/ai/prompts.ts (3)
1271-1275: Inconsistent sortDirection type (boolean vs "asc" | "desc" | null).Earlier bullets specify booleans while later output schema requires "asc" | "desc" | null. This will cause inconsistent LLM outputs and downstream parsing errors.
Unify to string enum everywhere:
- "sortDirection": <boolean> or null + "sortDirection": "<'asc' | 'desc' | null>" @@ - "sortDirection": <boolean if applicable otherwise null> + "sortDirection": "<'asc' | 'desc' | null>" @@ - "sortDirection": <boolean or null>, + "sortDirection": "<'asc' | 'desc' | null>",Also applies to: 1287-1294, 1306-1315
207-209: Hard-coded date in instructions ("2024-11-10").This becomes stale and conflicts with getDateForAI() usage elsewhere.
- - **If the temporal expression specifies an exact period** (e.g., "2 months ago," "last quarter"): Set the end date to the current date (2024-11-10). + - **If the temporal expression specifies an exact period** (e.g., "2 months ago," "last quarter"): Set the end date to the current date (${getDateForAI()}).
1940-1945: Residual “=======” separator inside prompt.Looks like a merge artifact; it may confuse the model.
-======= -If NO relevant items are found in Retrieved Context or context doesn't match query: +If NO relevant items are found in Retrieved Context or context doesn't match query:server/api/chat/utils.ts (1)
454-461: Import Zod schemas as values or swap to type aliases for allz.infer<typeof …>
typeof Schemaneeds a runtime import; type-only imports will fail.- Replace every
z.infer<typeof XxxSchema>with the existing TS type alias (e.g.Xxx) or importXxxSchemanormally.- Rerun
to verify no remaining misuses.rg -nP '\bz\.infer<\s*typeof\s+[A-Z]\w+\s*>' --type=ts
🧹 Nitpick comments (53)
server/utils/encryption.ts (2)
26-42: Include envelope metadata (version) to future-proof ciphertexts.Without versioning, algo/format changes are hard to manage.
- return JSON.stringify({ + return JSON.stringify({ + v: 1, ciphertext: enc, iv: iv.toString(this.encoding), authTag: authTag, })
61-77: Avoid blocking the event loop with scryptSync; clarify salt/rotation strategy.Use async crypto.scrypt to prevent latency spikes; also confirm whether tying salt to the encryption key is acceptable given key rotation (hashes would become unverifiable).
- public hashOneWayScrypt(str: string): string { + public async hashOneWayScrypt(str: string): Promise<string> { // Use a fixed salt derived from the encryption key const salt = crypto .createHash("sha256") .update(this.key + "api_key_salt_scrypt") .digest() .subarray(0, 32) as crypto.BinaryLike // Use scrypt with secure parameters - const hash = crypto.scryptSync(str, salt, 32, { - N: 16384, // CPU/memory cost parameter - r: 8, // Block size parameter - p: 1, // Parallelization parameter - }) - - return hash.toString(this.encoding) + const hash: Buffer = await new Promise((resolve, reject) => + crypto.scrypt( + str, + salt, + 32, + { N: 16384, r: 8, p: 1 }, + (err, derivedKey) => (err ? reject(err) : resolve(derivedKey as Buffer)), + ), + ) + return hash.toString(this.encoding) }Follow-up:
- If key rotation is expected, consider a per-item random salt stored alongside the hash and a versioned format, or a stable server-wide salt independent of the encryption key.
frontend/src/components/Sidebar.tsx (4)
219-224: Use cn() for className consistency on Knowledge Management linkOther Links use cn(). Keep styling consistent and slightly cleaner.
- <Link - to="/knowledgeManagement" - className={`flex w-8 h-8 items-center justify-center hover:bg-[#D8DFE680] dark:hover:bg-gray-700 rounded-md mt-[10px] ${ - location.pathname.includes("/knowledgeManagement") - ? "bg-[#D8DFE680] dark:bg-gray-700" - : "" - }`} - > + <Link + to="/knowledgeManagement" + className={cn( + "flex w-8 h-8 items-center justify-center hover:bg-[#D8DFE680] dark:hover:bg-gray-700 rounded-md mt-[10px]", + location.pathname.includes("/knowledgeManagement") && + "bg-[#D8DFE680] dark:bg-gray-700", + )} + >
37-47: Type the role prop as UserRole (avoid string default)role is compared against UserRole enum; typing it as string with a "" default risks subtle bugs.
-export const Sidebar = ({ - className = "", - photoLink = "", - role = "", - isAgentMode = false, -}: { - className?: string - photoLink?: string - role?: string - isAgentMode?: boolean -}) => { +export const Sidebar = ({ + className = "", + photoLink = "", + role, + isAgentMode = false, +}: { + className?: string + photoLink?: string + role?: UserRole + isAgentMode?: boolean +}) => {
77-104: Drop BOOKMARK_BUTTON exception in click-outside handlerBased on prior learning for this repo: HistoryModal is inside the sidebar container, so clicks on its bookmark buttons won’t trigger outside-closes. The extra exception adds complexity without benefit.
- const isBookmarkButton = target.closest(`.${CLASS_NAMES.BOOKMARK_BUTTON}`) if ( !isSidebarClick && !isHistoryModalClick && !isChatInput && !isSearchArea && !isReferenceBox && !isAtMentionArea && - !isBookmarkButton && showHistory ) { setShowHistory(false) }Note: This suggestion leverages a retrieved learning from June 24, 2025 about Sidebar/HistoryModal click handling.
Also applies to: 91-91, 99-99
191-193: Capitalize tooltip label and remove placeholderMinor UX polish; aligns with other tooltip titles.
- <Tip side="right" info="agent" />{" "} - {/* Placeholder: Update this tooltip info */} + <Tip side="right" info="Agent" />frontend/src/routes/_authenticated/admin/integrations/google.tsx (4)
369-386: Make file extension check case-insensitive and resilient to missing MIME.Some browsers set an empty
value.type; also extensions can be uppercase. Recommend normalizing both before the check.onChange: ({ value }) => { if (!value) return "File is required" - // Check file type - if ( - value.type !== "application/json" && - !value.name?.endsWith(".json") - ) { + // Check file type/extension (case-insensitive; tolerate missing MIME) + const type = (value.type || "").toLowerCase() + const name = (value.name || "").toLowerCase() + if (type !== "application/json" && !name.endsWith(".json")) { return "File must be a JSON file" }
492-509: Apply the same robust JSON file check in the update flow.Mirror the case-insensitive, MIME-tolerant check here to keep behaviors consistent between create/update.
onChange: ({ value }) => { if (!value) return "File is required" - // Check file type - if ( - value.type !== "application/json" && - !value.name?.endsWith(".json") - ) { + // Check file type/extension (case-insensitive; tolerate missing MIME) + const type = (value.type || "").toLowerCase() + const name = (value.name || "").toLowerCase() + if (type !== "application/json" && !name.endsWith(".json")) { return "File must be a JSON file" }
134-135: Correct error message: this path is for OAuth creation, not file upload.- `Failed to upload file: ${response.status} ${response.statusText} - ${errorText}`, + `Failed to create OAuth integration: ${response.status} ${response.statusText} - ${errorText}`,
278-280: Remove unused state (and the ts-ignore).
selectedFileisn’t used. Dropping it avoids dead code and the need for@ts-ignore.- //@ts-ignore - const [selectedFile, setSelectedFile] = useState<File | null>(null) + // (removed unused selectedFile state)server/api/admin.ts (2)
875-877: Consider using a more type-safe approach for header validation.The current filter might silently drop headers with non-string values. Consider a more explicit validation approach that could warn about invalid headers.
Apply this refactor for better error handling:
- ([k, v]) => - typeof k === "string" && typeof v === "string" && v.trim() !== "", + ([k, v]) => { + if (typeof k !== "string" || typeof v !== "string") { + loggerWithChild({ email: sub }).warn( + `Invalid header type - key: ${typeof k}, value: ${typeof v}` + ); + return false; + } + return v.trim() !== ""; + },
1150-1150: Remove trailing comma for consistency.- } + }frontend/src/routes/_authenticated/admin/integrations/mcp.tsx (1)
339-345: Using Date.now() as key is not ideal for React lists.While Date.now() provides uniqueness most of the time, it could theoretically produce duplicates if called in rapid succession. Consider using a more robust unique ID generation method.
Consider using a more robust ID generator:
+let headerIdCounter = 0; +const generateHeaderId = () => ++headerIdCounter; onClick={() => field.pushValue({ - u_id: Date.now() /* using Date.now() here so that we can keep this as key for our list */, + u_id: generateHeaderId(), key: "", value: "", }) }frontend/src/components/Dashboard.tsx (1)
3607-3609: Trim trailing space in className.Minor nit: remove the extra space after
font-displayto avoid churn in diffs.- <h1 className="text-3xl tracking-wider font-display "> + <h1 className="text-3xl tracking-wider font-display">frontend/src/components/ClFileUpload.tsx (2)
75-79: Type the error callbacks to match DOM API.Use
DOMException | Errorfor better fidelity with WebKit directory APIs. Optional hardening per your prior preference.- file?: ( - success: (file: File) => void, - error: (err: Error) => void, - ) => void + file?: ( + success: (file: File) => void, + error: (err: DOMException | Error) => void, + ) => void @@ - readEntries: ( - success: (entries: FileSystemEntry[]) => void, - error: (err: Error) => void, - ) => void + readEntries: ( + success: (entries: FileSystemEntry[]) => void, + error: (err: DOMException | Error) => void, + ) => voidAlso applies to: 83-87
94-117: Guard against missing entry.file to prevent a hanging Promise.If
entry.isFileis true butentry.fileis undefined, the Promise never settles. Add a fast-resolve guard.- if (entry.isFile) { - entry.file?.( + if (entry.isFile) { + if (!entry.file) { + resolve([]) + return + } + entry.file( (file: File) => {frontend/src/components/PdfViewer.tsx (2)
509-511: Clamp page input to valid bounds.parseInt(...) || 1 will send 1 when user types anything > totalPages. Clamp to [1,totalPages] to avoid unexpected jumps.
- onChange={(e) => goToPage(parseInt(e.target.value) || 1)} + onChange={(e) => { + const n = Number(e.target.value) || 1 + const clamped = Math.max(1, Math.min(totalPages, n)) + goToPage(clamped) + }}
273-276: Avoid global ID collisions for multiple viewers on the page.DOM lookups use hard-coded ids (e.g., pdf-page-1). If multiple PdfViewer instances render, IDs collide. Prefix with an instanceId.
Example:
- const instanceId = useMemo(() => Math.random().toString(36).slice(2), [])
- Use
id={pdf-page-${instanceId}-${pageNum}}and query with that prefix.Also applies to: 586-593
frontend/src/index.css (1)
119-127: Prefer a design token over hard-coded #fc7878.Using a CSS variable keeps theming consistent and future-proofs color tweaks.
Apply this diff to the changed lines:
- input[type="checkbox"]:checked { - background-color: #fc7878; - border-color: #fc7878; + input[type="checkbox"]:checked { + background-color: var(--checkbox-accent, #fc7878); + border-color: var(--checkbox-accent, #fc7878); } - .dark input[type="checkbox"]:checked { - background-color: #fc7878; - border-color: #fc7878; + .dark input[type="checkbox"]:checked { + background-color: var(--checkbox-accent, #fc7878); + border-color: var(--checkbox-accent, #fc7878); } - input[type="checkbox"]:checked::after { - content: ""; + input[type="checkbox"]:checked::after { + content: "";Also add the token definition (outside the changed hunk, in
:rootand optionally.dark)::root { --checkbox-accent: #fc7878; } /* .dark { --checkbox-accent: #fc7878; } // if you want a different dark value later */Also applies to: 129-131
server/db/schema/agents.ts (1)
92-104: Optional: consider forward-compatible default.If you plan to migrate fully to the map shape, defaulting to {} (for the record arm) avoids shape drift for new rows parsed solely via Zod.
- appIntegrations: z + appIntegrations: z .union([ z.array(z.string()), // Legacy format z.record( z.object({ // New AppSelectionMap format itemIds: z.array(z.string()), selectedAll: z.boolean(), }), ), ]) - .optional() - .default([]), + .optional() + .default([] as unknown as Record<string, { itemIds: string[]; selectedAll: boolean }>),frontend/src/components/DocxViewer.tsx (2)
160-171: Reduce post-render DOM styling; rely on CSS to avoid layout thrash.You already declare equivalent CSS in the style tag below; the imperative width/table/cell/img style writes duplicate work on every render.
- const sections = containerRef.current.querySelectorAll( - "section.docx-preview, section.docx-viewer", - ) - sections.forEach((el) => { - const section = el as HTMLElement - section.style.removeProperty("width") - section.style.removeProperty("max-width") - section.style.removeProperty("min-width") - section.style.width = "100%" - section.style.maxWidth = "100%" - section.style.minWidth = "auto" - }) - - const wrappers = containerRef.current.querySelectorAll( - ".docx, .docx-wrapper, .docx-preview-wrapper", - ) - wrappers.forEach((el) => { - const wrapper = el as HTMLElement - wrapper.style.removeProperty("width") - wrapper.style.removeProperty("max-width") - wrapper.style.removeProperty("min-width") - wrapper.style.width = "100%" - wrapper.style.maxWidth = "100%" - wrapper.style.minWidth = "auto" - }) - - const tables = containerRef.current.querySelectorAll("table") - tables.forEach((table) => { - const tableEl = table as HTMLElement - tableEl.style.width = "100%" - tableEl.style.maxWidth = "100%" - tableEl.style.minWidth = "auto" - tableEl.style.tableLayout = "fixed" // Better for responsive tables - - const cells = tableEl.querySelectorAll("td, th") - cells.forEach((cell) => { - const cellEl = cell as HTMLElement - cellEl.style.overflowWrap = "break-word" - cellEl.style.whiteSpace = "normal" - cellEl.style.maxWidth = "0" // Forces text wrapping - cellEl.style.minWidth = "100px" - cellEl.style.boxSizing = "border-box" - cellEl.style.padding = "5px" - }) - }) - - const images = containerRef.current.querySelectorAll("img") - images.forEach((img) => { - const imgEl = img as HTMLElement - imgEl.style.maxWidth = "100%" - imgEl.style.height = "auto" - imgEl.style.width = "auto" - }) + // Styling handled via CSS rules injected below; avoid runtime style mutations.Also applies to: 174-184, 187-206, 209-216
239-245: Minor a11y: announce loading state.Expose progress to AT users.
- <div className="absolute inset-0 flex items-center justify-center bg-white/90 dark:bg-[#1E1E1E]/90 z-10"> + <div className="absolute inset-0 flex items-center justify-center bg-white/90 dark:bg-[#1E1E1E]/90 z-10" role="status" aria-live="polite" aria-busy="true"> <div className="text-center"> <div className="animate-spin rounded-full h-12 w-12 border-b-2 border-gray-600 dark:border-gray-300 mx-auto mb-4"></div> <p className="text-gray-600 dark:text-gray-300"> Loading document... </p> </div> </div>server/shared/types.ts (1)
482-482: Status type tweak is benign; consider centralizing.Switching quote style is no-op. Optionally, export a shared
type AgentReasoningStatus = "in_progress" | "completed" | "failed"to reuse across emitters/parsers.frontend/src/routes/_authenticated/knowledgeManagement.tsx (1)
1013-1027: Minor: avoid pre-expanding child nodes by defaultNew children are initialized with isOpen: true. Consider defaulting to false to match typical tree UX unless there’s a reason to auto-expand.
- isOpen: true, + isOpen: false,frontend/src/components/EnhancedReasoning.tsx (4)
118-121: Use Markdown list markers for parameter bullets“• ” won’t render as a list in Markdown. Use “- ” to get proper list semantics and spacing.
- const result = `**Parameters:**\n\n${formattedItems.map((item: string) => `• ${item}`).join("\n\n")}\n\n` + const result = `**Parameters:**\n\n${formattedItems.map((item: string) => `- ${item}`).join("\n")}\n`
717-721: Tiny copy fix: singular/plural secondsPrevents “1 seconds”.
- text: `The search was completed in ${duration} seconds`, + text: `The search was completed in ${duration} second${duration === 1 ? "" : "s"}`,
1005-1008: Use functional state update for collapse toggleAvoids stale closures and removes unnecessary dependency.
- const toggleCollapsed = useCallback( - () => setIsCollapsed(!isCollapsed), - [isCollapsed], - ) + const toggleCollapsed = useCallback(() => { + setIsCollapsed((v) => !v) + }, [])
1019-1021: Add aria-expanded to the toggle buttonMinor a11y improvement for screen readers.
- <button - onClick={toggleCollapsed} + <button + onClick={toggleCollapsed} + aria-expanded={!isCollapsed}server/db/schema/knowledgeBase.ts (1)
79-80: Type column is stringly; consider enum for safetyIf possible, switch
varchar("type")to a pgEnum to enforce folder/file at the DB layer and speed up comparisons.I can provide a small migration to introduce
item_type_enumand backfill.frontend/src/utils/fileUtils.ts (1)
31-37: Guard against non-string entries in cleanupPreviewUrlsDefensive: filter falsy and ensure string to avoid revoke errors when called with mixed arrays.
-export const cleanupPreviewUrls = (previews: string[]) => { - previews.forEach((url) => { - if (url) { - URL.revokeObjectURL(url) - } - }) -} +export const cleanupPreviewUrls = (previews: Array<string | undefined | null>) => { + previews.filter((u): u is string => typeof u === "string" && !!u) + .forEach((url) => URL.revokeObjectURL(url)) +}frontend/src/routes/_authenticated/chat.tsx (1)
2148-2466: Virtualization refactor looks solid; minor optional tweaks.
- Good use of measureElement and ref forwarding.
- Optional: consider increasing overscan slightly when enabling images/mermaid-heavy content to reduce pop-in during fast scrolls.
server/api/knowledgeBase.ts (3)
726-731: Fix folderCache key bug in ensureFolderPath (cache miss).You check the cache using the full folderPath (joined pathParts), but store only the first segment as the key. This defeats caching for nested paths and can cause redundant DB lookups and transactions during batch uploads.
Apply this diff:
- const folderPath = pathParts.join("/") + const folderPath = pathParts.join("/") // Check cache first if (folderCache && folderCache.has(folderPath)) { return folderCache.get(folderPath) || null } ... - if (folderCache) { - folderCache.set(pathParts.slice(0, 1).join("/"), currentFolderId) - } + if (folderCache) { + folderCache.set(folderPath, currentFolderId) + }Also applies to: 758-799, 809-810
1035-1040: Reduce per-file DB roundtrips for duplicate checks.getCollectionItemsByParent is called for every file. For large batches this becomes O(N) queries. Consider memoizing by targetParentId within the request or preloading once per parent to cut DB calls.
975-989: Harden path handling for user-supplied folder structure.Only filenames are validated. Folder names from paths aren't validated (chars/length/depth). Add validation to pathParts to prevent odd Unicode control chars, overly deep trees, and names like “.”/“..”.
Also applies to: 1010-1019, 726-735
server/api/chat/chat.ts (2)
6407-6416: Validate and bound enhanced feedback payload.Add basic input validation (length caps, option limits) to avoid oversized payloads and log noise. Keep it backend-safe regardless of UI gating.
Example (local to handler):
+ const FeedbackSchema = z.object({ + messageId: z.string().min(1), + type: z.enum(["like", "dislike"]), + customFeedback: z.string().trim().max(1000).optional(), + selectedOptions: z.array(z.string().trim().max(100)).max(10).optional(), + shareChat: z.boolean().optional(), + }) - const requestData = await c.req.valid("json") + const requestData = FeedbackSchema.parse(await c.req.valid("json"))Also applies to: 6454-6553
6521-6525: Avoid logging share tokens.Share token is effectively an access secret; avoid logging it at info level.
- loggerWithChild({ email: email }).info( - `Share token generated for feedback submission: ${shareToken}`, - { messageId, chatId: chat.externalId }, - ) + loggerWithChild({ email: email }).info( + `Share token generated for feedback submission`, + { messageId, chatId: chat.externalId }, + )server/ai/prompts.ts (5)
1072-1093: Nested template interpolation: verify brace/backtick pairing and simplify for maintainability.The ternary nesting is valid but hard to read and easy to break later. Consider extracting the optional sections into small helpers to compose the final string.
Apply this refactor to reduce nesting noise:
+const prevClassBlock = (pc?: QueryRouterLLMResponse | null) => + pc ? `**Previous Query Classification:** ${JSON.stringify(pc, null, 2)}` : "" + +const chainBreakBlock = (cbc?: ChainBreakClassifications | null) => + cbc + ? `**Chain Break Classifications (Previous Conversation Chains):** +${JSON.stringify(cbc, null, 2)} + +**IMPORTANT - Chain Context Integration:** +... (keep existing bullets) ...` + : "" return ` ... - ${ - previousClassification - ? `**Previous Query Classification:** ${JSON.stringify(previousClassification, null, 2)} - ${ - chainBreakClassifications - ? `**Chain Break Classifications (Previous Conversation Chains):** - ${JSON.stringify(chainBreakClassifications, null, 2)} -... - : "" - } -... - : "" - } + ${prevClassBlock(previousClassification)} + ${chainBreakBlock(chainBreakClassifications)} ... `
195-198: Typo: “goo for better query rewrites”.Fix minor typo for clarity.
- - Do not answer if you do not have valid context and goo for better query rewrites + - Do not answer if you do not have valid context and go for better query rewrites
1463-1467: Typo: “CONCREATE REFERENCE”.Minor grammar fix in user-facing prompt.
- b) It's an instruction or command that doesn't have any CONCREATE REFERENCE. + b) It's an instruction or command that doesn't have any CONCRETE REFERENCE.
636-638: Spelling in comment: “Baseline Reasoing Prompt JSON”.Low-impact cleanup.
-// Baseline Reasoing Prompt JSON +// Baseline Reasoning Prompt JSON
2246-2251: Wording: “Do not respond within following the JSON format.”Clarify instruction.
-Do not respond within following the JSON format. +Do not respond outside the JSON format.server/api/chat/agents.ts (6)
1116-1168: Sanitize credential logs and handle invalid JSON headers robustly.You already avoid logging headers; good. Add a narrower catch with explicit fallback and mask potential tokens in error context.
-} catch (error) { - loggerWithChild({ email: sub }).error( - error, - `Failed to parse connector credentials for connectorId: ${connectorId}. Using empty headers.`, - ) - loadedHeaders = {} -} +} catch (error) { + loggerWithChild({ email: sub }).error( + error, + `Failed to parse connector credentials for connectorId: ${connectorId}. Using empty headers.`, + { connectorId } // avoid echoing credentials + ) + loadedHeaders = {} +}
1981-2001: Don’t inject “No results found” pseudo-context into planningContext.Adding synthetic “No X found …” as a fragment can bias synthesis and leak into answers.
- const context = { - id: "", - content: `No ${appName} found within the specified date range (${fromDate} to ${toDate}). No further action needed - this simply means there was no activity during this time period.`, - source: {} as any, - confidence: 0, - } - gatheredFragments.push(context) + // Record internally if needed, but don't add to gatheredFragments + await logAndStreamReasoning({ + type: AgentReasoningStepType.LogMessage, + message: `No ${appName} found from ${fromDate} to ${toDate}.`, + })
2537-2559: Duplicate DB updates for error message in SSE error callback.
addErrMessageToMessageis called twice; remove the second call.- await stream.writeSSE({ - event: ChatSSEvents.Error, - data: errFromMap, - }) - await addErrMessageToMessage(lastMessage, errFromMap) + await stream.writeSSE({ + event: ChatSSEvents.Error, + data: errFromMap, + })
4533-4546: Duplicate DB updates for error message in AgentMessageApi error callback.Same duplication here; keep only one update.
- await stream.writeSSE({ - event: ChatSSEvents.Error, - data: errFromMap, - }) - await addErrMessageToMessage(lastMessage, errFromMap) + await stream.writeSSE({ + event: ChatSSEvents.Error, + data: errFromMap, + })
775-777: Typos in logs: “mesage”.Minor polish.
- "First mesage of the conversation, successfully created the chat" + "First message of the conversation, successfully created the chat"Also applies to: 2920-2921, 3626-3627
1156-1167: Add retries/backoff when connecting to external MCP clients.Transient network issues will otherwise fail the whole flow.
If you have a common retry util, wrap
client.connect(...)with 2–3 attempts and exponential backoff (e.g., 250ms, 750ms, 1500ms). I can draft a small helper if helpful.Also applies to: 1170-1176
server/api/chat/utils.ts (4)
921-930: Make “recent count” configurable.
Hardcoding 2 is inflexible. Add an optional param with default 2; use it in slice and log.-export function getRecentChainBreakClassifications( - messages: SelectMessage[], -): ChainBreakClassification[] { +export function getRecentChainBreakClassifications( + messages: SelectMessage[], + limit = 2, +): ChainBreakClassification[] { const chainBreaks = extractChainBreakClassifications(messages) - const recentChainBreaks = chainBreaks.slice(0, 2) // limit to the last 2 chain breaks + const recentChainBreaks = chainBreaks.slice(0, limit) // last N chain breaks getLoggerWithChild(Subsystem.Chat)().info( - `[ChainBreak] Found ${recentChainBreaks.length} recent chain breaks`, + `[ChainBreak] Found ${recentChainBreaks.length} recent chain breaks (limit=${limit})`, ) return recentChainBreaks }
932-979: Don’t assume strict user/assistant alternation; scan backwards for previous user message.
Current index-2 logic breaks with system/tool/assistant-only inserts.- // Skip if this is the first user message (no previous user message available) - if (index < 2) return - - // Get the previous user message - const previousUserMessage = messages[index - 2] - if ( - !previousUserMessage || - previousUserMessage.messageRole !== "user" || - !previousUserMessage.queryRouterClassification - ) - return + // Find previous user message (robust to non-alternating roles) + let prevIdx = index - 1 + while (prevIdx >= 0 && messages[prevIdx].messageRole !== "user") prevIdx-- + if (prevIdx < 0) return + const previousUserMessage = messages[prevIdx] + if (!previousUserMessage.queryRouterClassification) return ... - const prevClassification = parseQueryRouterClassification( - previousUserMessage.queryRouterClassification, - index - 2, - ) + const prevClassification = parseQueryRouterClassification( + previousUserMessage.queryRouterClassification, + prevIdx, + ) ... - chainBreaks.push({ - messageIndex: index - 2, + chainBreaks.push({ + messageIndex: prevIdx, classification: prevClassification, query: previousUserMessage.message || "", })
971-973: Avoid logging full message bodies.
Could leak PII; log IDs/snippets instead.- `[ChainBreak] Chain break detected: "${previousUserMessage.message}" → "${message.message}"`, + `[ChainBreak] Chain break detected: prevUserIdx=${prevIdx}, currIdx=${index}`,
981-999: Consider slimming the prompt payload.
Classification objects can be large; include only fields the model needs to keep prompts lean.- classification: chainBreak.classification, + classification: { + isFollowUp: chainBreak.classification.isFollowUp, + intent: chainBreak.classification.intent, + // add other minimal fields as required + },server/api/knowledgeBase/search.ts (2)
107-110: Fetching accessible collections once is fine; consider reusing across sections.
Minor micro-opt if called twice.
181-182: Caution: ilike on path with %term% won’t use the index.
If path search is rare/optional, OK; else consider trigram index or TSV.
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 (1)
server/api/chat/chat.ts (1)
374-377: Return 404 (not 500) when a chat trace is not foundYou already note “Return 404” in the comment but throw 500. This breaks client semantics and observability.
- if (!trace) { - // Return 404 if the trace is not found for the given IDs - throw new HTTPException(500, { message: "Chat trace not found" }) - } + if (!trace) { + // Return 404 if the trace is not found for the given IDs + throw new HTTPException(404, { message: "Chat trace not found" }) + }
🧹 Nitpick comments (7)
server/api/chat/chat.ts (7)
6091-6126: Normalize destructuring to include offset/intent (consistency with MessageApi)Here you read
offset/intentviaparsed?.filters?.…while above you destructure them. Aligning styles reduces branching and keeps both paths symmetric.- const { - apps, - count, - endTime, - entities, - sortDirection, - startTime, - } = parsed?.filters || {} + const { + apps, + count, + endTime, + entities, + sortDirection, + startTime, + offset, + intent, + } = parsed?.filters || {} classification = { direction: parsed.temporalDirection, type: parsed.type, filterQuery: parsed.filterQuery, isFollowUp: parsed.isFollowUp, filters: { apps: apps, entities: entities as Entity[], endTime, sortDirection, startTime, count, - offset: parsed?.filters?.offset || 0, - intent: parsed?.filters?.intent || {}, + offset: offset || 0, + intent: intent || {}, }, } as QueryRouterLLMResponse
3998-4003: Avoid shadowing ‘chat’; use the outer variable so catch blocks have context
let chat: SelectChatis declared at function scope and again inside the try. The inner declaration hides the outer, so the top-level catch can’t access the created chat for error reporting.- let chat: SelectChat + let chat: SelectChat let assistantMessageId: string | null = null let streamKey: string | null = null- let chat: SelectChat + // reuse outer `chat` to keep it available to catch blocksAlso applies to: 4081-4081
6584-6585: Guard metrics label to avoid ‘undefined’ feedback dimensionWhen removing feedback,
feedbackcan be falsy; incrementing the metric with an undefined label can pollute series.- likeDislikeCount.inc({ email: email, feedback: feedback }) + if (feedback === "like" || feedback === "dislike") { + likeDislikeCount.inc({ email, feedback }) + }
6550-6550: Reuse the top-level Logger instead of redeclaringYou redeclare
const Logger = getLogger(Subsystem.Chat)inside this function while a module-level Logger already exists. Prefer one.- const Logger = getLogger(Subsystem.Chat) + // use module-level Logger
6604-6604: Same here: avoid redeclaring LoggerFollow the module-level logger for consistency.
- const Logger = getLogger(Subsystem.Chat) + // use module-level Logger
6661-6674: Optional: cap customFeedback length and sanitizeTo keep payloads bounded and reduce log noise, cap/sanitize
customFeedback.- if (customFeedback && customFeedback.trim()) { - feedbackData.feedback.push(customFeedback.trim()) - } + if (customFeedback && customFeedback.trim()) { + const fb = customFeedback.trim().slice(0, 1000) // cap length + feedbackData.feedback.push(fb) + }
6677-6735: Optional: handle share token race with unique constraint or retryTwo concurrent requests could both insert a share row. Consider a unique index on (chatId, messageId) and retry on conflict.
Would you like a migration snippet to add a unique constraint on (chatId, messageId) and switch this insert to an upsert?
📜 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)
server/api/chat/chat.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: MayankBansal2004
PR: xynehq/xyne#532
File: frontend/src/routes/_authenticated/chat.tsx:1238-1242
Timestamp: 2025-06-16T08:36:57.488Z
Learning: MayankBansal2004 prefers to keep working implementations even when security improvements are suggested, but appreciates being made aware of potential security considerations for future reference.
Learnt from: MayankBansal2004
PR: xynehq/xyne#757
File: server/db/schema/knowledgeBase.ts:151-183
Timestamp: 2025-09-02T07:03:18.651Z
Learning: MayankBansal2004 prefers to keep formatting changes separate from functional database improvements, focusing PRs on their stated objectives rather than mixing concerns.
📚 Learning: 2025-08-11T08:44:08.412Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#719
File: server/api/chat/chat.ts:4023-4029
Timestamp: 2025-08-11T08:44:08.412Z
Learning: In server/api/chat/chat.ts and similar files, MayankBansal2004 prefers not to add explicit type definitions for metadata.usage structures when the receiving array is already typed and default values ensure type safety through the || 0 pattern.
Applied to files:
server/api/chat/chat.ts
🔇 Additional comments (1)
server/api/chat/chat.ts (1)
4863-4887: Robust defaulting for filters looks goodUsing
parsed?.filters || {}with sane fallbacks foroffsetandintentprevents crashes and keeps pagination stable.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/routes/_authenticated/chat.tsx (1)
870-925: Enhanced feedback: missing response.ok check can show false “Success”The enhanced submit path parses JSON and toasts success without verifying response.ok, leading to a false-positive toast and stale optimistic state on server errors.
Apply this diff to validate the response and also restore previous feedback value on failure:
const handleEnhancedFeedbackSubmit = async (data: { type: MessageFeedback customFeedback?: string selectedOptions?: string[] shareChat?: boolean }) => { if (!pendingFeedback) return const { messageId } = pendingFeedback - // Optimistically update the UI + // Snapshot previous for rollback and optimistically update the UI + const prevFeedback = feedbackMap[messageId] setFeedbackMap((prev) => ({ ...prev, [messageId]: data.type, })) try { const response = await api.message.feedback.enhanced.$post({ json: { messageId, type: data.type, customFeedback: data.customFeedback, selectedOptions: data.selectedOptions, shareChat: data.shareChat, }, }) + if (!response.ok) { + throw new Error("Failed to submit enhanced feedback") + } const result = await response.json() let successMessage = "Feedback submitted." if (data.shareChat && result.shareToken) { successMessage += " Chat has been shared for improvement purposes." } else if (data.shareChat && !result.shareToken) { successMessage += " Feedback submitted, but share token generation failed." } toast({ title: "Success", description: successMessage, duration: 2000 }) } catch (error) { console.error("Failed to submit enhanced feedback", error) // Revert optimistic update on error - setFeedbackMap((prev) => { - const newMap = { ...prev } - delete newMap[messageId] - return newMap - }) + setFeedbackMap((prev) => { + const next = { ...prev } + if (typeof prevFeedback === "undefined") { + delete next[messageId] + } else { + next[messageId] = prevFeedback + } + return next + }) toast({ title: "Error", description: "Failed to submit feedback. Please try again.", variant: "destructive", }) } finally { setPendingFeedback(null) setFeedbackModalOpen(false) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/Sidebar.tsx(1 hunks)frontend/src/index.css(3 hunks)frontend/src/routes/_authenticated/chat.tsx(3 hunks)frontend/src/routes/_authenticated/knowledgeManagement.tsx(10 hunks)server/api/chat/chat.ts(1 hunks)server/api/knowledgeBase.ts(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- server/api/chat/chat.ts
- frontend/src/index.css
- frontend/src/components/Sidebar.tsx
- server/api/knowledgeBase.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: MayankBansal2004
PR: xynehq/xyne#765
File: server/api/chat/chat.ts:5706-5712
Timestamp: 2025-09-04T08:53:33.954Z
Learning: MayankBansal2004 prefers to defer technical improvements like input validation and type safety when the current implementation is working, even when acknowledging potential benefits. He takes a pragmatic approach of "if it's not broken, don't fix it" and will revisit improvements when he has bandwidth.
Learnt from: MayankBansal2004
PR: xynehq/xyne#532
File: frontend/src/routes/_authenticated/chat.tsx:1238-1242
Timestamp: 2025-06-16T08:36:57.488Z
Learning: MayankBansal2004 prefers to keep working implementations even when security improvements are suggested, but appreciates being made aware of potential security considerations for future reference.
Learnt from: MayankBansal2004
PR: xynehq/xyne#757
File: server/db/schema/knowledgeBase.ts:151-183
Timestamp: 2025-09-02T07:03:18.671Z
Learning: MayankBansal2004 prefers to keep formatting changes separate from functional database improvements, focusing PRs on their stated objectives rather than mixing concerns.
Learnt from: MayankBansal2004
PR: xynehq/xyne#765
File: server/api/chat/chat.ts:5612-5614
Timestamp: 2025-09-03T09:59:50.071Z
Learning: MayankBansal2004 prefers to defer technical debt improvements when the current implementation is functional, especially when there are frontend abstractions in place (like "hiding modelid"). He acknowledges suggestions but prioritizes keeping working code over immediate refactoring.
📚 Learning: 2025-06-10T05:40:04.427Z
Learnt from: naSim087
PR: xynehq/xyne#525
File: frontend/src/routes/_authenticated/admin/integrations/slack.tsx:141-148
Timestamp: 2025-06-10T05:40:04.427Z
Learning: In frontend/src/routes/_authenticated/admin/integrations/slack.tsx, the ConnectAction enum and related connectAction state (lines 141-148, 469-471) are intentionally kept for future development, even though they appear unused in the current implementation.
Applied to files:
frontend/src/routes/_authenticated/chat.tsx
📚 Learning: 2025-09-02T07:02:37.452Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#757
File: server/api/chat/agents.ts:31-41
Timestamp: 2025-09-02T07:02:37.452Z
Learning: MayankBansal2004 correctly identifies when suggestions are out of scope for formatting-only PRs and prefers to keep PRs focused on their intended changes rather than expanding scope to include unrelated improvements.
Applied to files:
frontend/src/routes/_authenticated/knowledgeManagement.tsx
📚 Learning: 2025-06-16T08:36:57.488Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#532
File: frontend/src/routes/_authenticated/chat.tsx:1238-1242
Timestamp: 2025-06-16T08:36:57.488Z
Learning: MayankBansal2004 prefers to keep working implementations even when security improvements are suggested, but appreciates being made aware of potential security considerations for future reference.
Applied to files:
frontend/src/routes/_authenticated/knowledgeManagement.tsx
📚 Learning: 2025-09-02T07:03:18.671Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#757
File: server/db/schema/knowledgeBase.ts:151-183
Timestamp: 2025-09-02T07:03:18.671Z
Learning: MayankBansal2004 prefers to keep formatting changes separate from functional database improvements, focusing PRs on their stated objectives rather than mixing concerns.
Applied to files:
frontend/src/routes/_authenticated/knowledgeManagement.tsx
📚 Learning: 2025-09-03T09:59:50.071Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#765
File: server/api/chat/chat.ts:5612-5614
Timestamp: 2025-09-03T09:59:50.071Z
Learning: MayankBansal2004 prefers to defer technical debt improvements when the current implementation is functional, especially when there are frontend abstractions in place (like "hiding modelid"). He acknowledges suggestions but prioritizes keeping working code over immediate refactoring.
Applied to files:
frontend/src/routes/_authenticated/knowledgeManagement.tsx
🧬 Code graph analysis (2)
frontend/src/routes/_authenticated/chat.tsx (3)
frontend/src/api.ts (1)
api(4-4)frontend/src/hooks/use-toast.ts (1)
toast(188-188)frontend/src/components/ui/toast.tsx (1)
ToastAction(126-126)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
frontend/src/types/knowledgeBase.ts (2)
Collection(1-18)CollectionItem(20-58)frontend/src/utils/fileUtils.ts (2)
FileNode(140-149)uploadFileBatch(211-258)
🔇 Additional comments (8)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (7)
29-32: LGTM - Clean import formattingThe multiline import formatting for the knowledgeBase types improves readability.
120-122: LGTM - Consistent multiline text formattingThe formatting change maintains consistency with the text formatting pattern used elsewhere in the component.
197-199: LGTM - Consistent multiline text formattingThe formatting change maintains consistency with the text formatting pattern used elsewhere in the component.
624-627: LGTM - Improved function signature formattingThe multiline formatting for the function parameters enhances readability.
668-672: LGTM - Consistent multiline function call formattingThe multiline formatting for the
uploadFileBatchcall improves readability and maintains consistency.
1016-1030: LGTM - Improved object mapping formattingThe multiline formatting for the children mapping enhances readability while preserving the same functionality.
1475-1476: LGTM - Empty string formatting consistencyUsing
""instead of''maintains consistency with the codebase's string literal style.frontend/src/routes/_authenticated/chat.tsx (1)
127-127: ToastAction import looks correctAligned with the new actionable toast flow. No concerns.
2 participants
Description
feedback modal only opens now on dislike with the toast to make the form filling optional
Testing
tested locally.
Additional Notes
Summary by CodeRabbit