fix(agentPathSupport): path referred folder considered as integration #1021
Conversation
…rce to that agent
naSim087
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThreaded path-derived collection identifiers through the chat/agent pipeline: added extractItemIdsFromPath, introduced PathExtractedInfo, updated multiple function signatures to accept actualModelId and pathExtractedInfo, and replaced prior folderIds/ids usage with path-derived collectionFileIds/collectionFolderIds/collectionIds where applicable. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as agents.ts
participant UT as utils.extractItemIdsFromPath
participant C as chat.ts (Understand...)
participant KB as Knowledge Base
U->>A: Send message (+ optional pathRefId)
A->>UT: extractItemIdsFromPath(pathRefId)
UT-->>A: pathExtractedInfo {collectionFileIds, collectionFolderIds, collectionIds}
A->>C: UnderstandMessageAndAnswer(message, email, actualModelId, pathExtractedInfo)
activate C
Note over C: source = pathExtractedInfo ? (folder→file→collection) : selectedItems[KB]
C->>KB: Fetch/process docs from chosen source
KB-->>C: Context chunks / metadata
C-->>A: Answer (stream or non-stream)
A-->>U: Final/streamed response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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. Comment |
Summary of ChangesHello @naSim087, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the 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 refactors the AgentMessageApi to support knowledge base paths by introducing a new extractItemIdsFromPath utility and modifying how context is passed to the RAG pipeline. The changes correctly separate path-based context extraction from message-based extraction. My review includes suggestions to improve code quality and maintainability. I've pointed out areas with code duplication that could be refactored into a helper function, inconsistencies in naming conventions and property access that should be standardized, and an opportunity to improve type safety by avoiding the any type. Addressing these points will make the code more robust and easier to maintain.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
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)
1212-1226: RenamecollectionfileIdstocollectionFileIdsthroughout
Update thepathExtractedInfotype to usecollectionFileIds(PascalCase) and replace all occurrences ofcollectionfileIdswithcollectionFileIdsin bothserver/api/chat/chat.tsandserver/api/chat/agents.ts.
🧹 Nitpick comments (2)
server/api/chat/utils.ts (1)
718-752: Add.limit(1)to DB lookups to avoid unnecessary scansBoth selects fetch a single row; limit the query explicitly for performance and clarity.
const [item] = await db .select({ id: collectionItems.id, type: collectionItems.type }) .from(collectionItems) .where( and( eq(collectionItems.vespaDocId, vespaId), isNull(collectionItems.deletedAt), ), ) + .limit(1) if (item) { // ... } else { // If not found in collectionItems, check collections table const [collection] = await db .select({ id: collections.id }) .from(collections) .where( and( eq(collections.vespaDocId, vespaId), isNull(collections.deletedAt), ), ) + .limit(1)server/api/chat/agents.ts (1)
3536-3546: Add explicit type annotation to pathExtractedInfoImport the exported type from utils and annotate to lock in the shape:
- const pathExtractedInfo = isValidPath + // import type { PathExtractedInfo } from "@/api/chat/utils" + const pathExtractedInfo: PathExtractedInfo = isValidPath ? await extractItemIdsFromPath(ids) : { collectionFileIds: [], collectionFolderIds: [], collectionIds: [] }getRecordBypath returns the Vespa doc ID (string) as expected by extractItemIdsFromPath—no further verification needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api/chat/agents.ts(3 hunks)server/api/chat/chat.ts(11 hunks)server/api/chat/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/api/chat/chat.ts (1)
server/shared/types.ts (1)
Apps(38-38)
server/api/chat/utils.ts (2)
server/db/schema/knowledgeBase.ts (2)
collectionItems(69-140)collections(20-66)server/logger/index.ts (2)
getLoggerWithChild(192-200)Subsystem(15-15)
server/api/chat/agents.ts (2)
server/api/chat/utils.ts (2)
extractFileIdsFromMessage(513-695)extractItemIdsFromPath(696-767)server/api/chat/chat.ts (1)
pathExtractedInfo(3832-3836)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
server/api/chat/agents.ts (1)
124-125: Import addition looks goodextractItemIdsFromPath is correctly imported from ./utils and used below.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/api/chat/agents.ts(3 hunks)server/api/chat/chat.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/api/chat/chat.ts (1)
server/shared/types.ts (1)
Apps(38-38)
server/api/chat/agents.ts (2)
server/api/chat/utils.ts (2)
extractFileIdsFromMessage(513-695)extractItemIdsFromPath(696-767)server/api/chat/chat.ts (1)
pathExtractedInfo(3832-3836)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
server/api/chat/agents.ts (3)
124-124: LGTM!The import of
extractItemIdsFromPathis correctly added and aligns with its usage later in the file.
3536-3545: LGTM!The refactored extraction logic correctly separates concerns:
extractFileIdsFromMessagehandles message-embedded context (pills, links, folders) without path infoextractItemIdsFromPathhandles path-derived collection context when a valid path is providedThe object literals use the correct field name
collectionFileIds(capital F) matching the type definition.
4306-4307: LGTM!The parameters passed to
UnderstandMessageAndAnswerare correct:
actualModelIdprovides the model configurationpathExtractedInfo(constructed earlier with correct field names) enables path-based collection filtering
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
server/api/chat/utils.ts (1)
761-764: [Duplicate] Logger argument order still inverted.This issue was flagged in a previous review but not yet fixed. The logger call passes the message before the error, which breaks the project's convention and can drop stack/metadata.
Apply this diff:
getLoggerWithChild(Subsystem.Chat)().error( + error, `Error extracting item IDs from pathRefId: ${vespaId}`, - error, )server/api/chat/chat.ts (1)
1349-1376: Path-derived KB IDs are still droppedWhen
pathExtractedInfois present we now return its raw IDs fromgetCollectionSource, but we still immediately run them through the legacycl-/clfd-/clf-prefix parsing. IDs coming fromextractItemIdsFromPathdo not carry those prefixes, so every branch falls through and we end up pushing zero IDs intoagentSpecificCollectionSelections, meaning path-based folder/file selections remain broken. The same regression exists in the duplicated blocks at Line 2607-2634 and Line 3171-3198.We need to short-circuit the prefix parsing whenever
pathExtractedInfois populated and push its arrays straight intocollectionIds/collectionFolderIds/collectionFileIdsinstead.const collectionIds: string[] = [] const collectionFolderIds: string[] = [] const collectionFileIds: string[] = [] - const source = getCollectionSource(pathExtractedInfo, selectedItems) - - for (const itemId of source) { - if (itemId.startsWith("cl-")) { - // Entire collection - remove cl- prefix - collectionIds.push(itemId.replace(/^cl[-_]/, "")) - } else if (itemId.startsWith("clfd-")) { - // Collection folder - remove clfd- prefix - collectionFolderIds.push(itemId.replace(/^clfd[-_]/, "")) - } else if (itemId.startsWith("clf-")) { - // Collection file - remove clf- prefix - collectionFileIds.push(itemId.replace(/^clf[-_]/, "")) - } - } + const hasPathCollections = + pathExtractedInfo && + (pathExtractedInfo.collectionFolderIds.length || + pathExtractedInfo.collectionFileIds.length || + pathExtractedInfo.collectionIds.length) + + if (hasPathCollections) { + collectionIds.push(...pathExtractedInfo!.collectionIds) + collectionFolderIds.push(...pathExtractedInfo!.collectionFolderIds) + collectionFileIds.push(...pathExtractedInfo!.collectionFileIds) + } else { + const source = selectedItems[Apps.KnowledgeBase] || [] + for (const itemId of source) { + if (itemId.startsWith("cl-")) { + collectionIds.push(itemId.replace(/^cl[-_]/, "")) + } else if (itemId.startsWith("clfd-")) { + collectionFolderIds.push(itemId.replace(/^clfd[-_]/, "")) + } else if (itemId.startsWith("clf-")) { + collectionFileIds.push(itemId.replace(/^clf[-_]/, "")) + } + } + }Apply the same fix in the corresponding blocks inside
generatePointQueryTimeExpansionandgenerateMetadataQueryAnswerso path-based selections flow all the way through.
🧹 Nitpick comments (1)
server/api/chat/utils.ts (1)
758-758: Empty else block—clarify intent or remove.Line 758 contains an empty
elseblock. If no action is needed for unrecognized prefixes, consider adding a comment explaining why (e.g., "silently ignore unrecognized prefixes") or remove the block entirely for clarity.Apply this diff if no action is intended:
if (collection) { collectionIds.push(`cl-${collection.id}`) // Keep the original prefixed ID } - } else { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api/chat/agents.ts(3 hunks)server/api/chat/chat.ts(11 hunks)server/api/chat/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/api/chat/utils.ts (2)
server/db/schema/knowledgeBase.ts (2)
collectionItems(69-140)collections(20-66)server/logger/index.ts (2)
getLoggerWithChild(192-200)Subsystem(15-15)
server/api/chat/agents.ts (1)
server/api/chat/utils.ts (2)
extractFileIdsFromMessage(513-695)extractItemIdsFromPath(696-773)
server/api/chat/chat.ts (1)
server/shared/types.ts (1)
Apps(38-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
server/api/chat/agents.ts (2)
3536-3545: Path extraction logic looks correct.The extraction flow properly separates message-based context (
extractedInfo) from path-based context (pathExtractedInfo). The conditional extraction based onisMsgWithContextandisValidPathensures that each type of context is only extracted when applicable, with appropriate empty defaults when not.
4295-4308: UnderstandMessageAndAnswer signature and PathExtractedInfo casing are correct
The function in server/api/chat/chat.ts acceptsmodelId?followed bypathExtractedInfo?: PathExtractedInfo, and thePathExtractedInfotype definescollectionFileIds,collectionFolderIds, andcollectionIdswith proper casing.server/api/chat/utils.ts (1)
720-757: Verify ID prefixing inutils.tsmatches downstream search filters
Path‐extracted IDs are built asclf-<item.id>,clfd-<item.id>,cl-<item.id>, then later stripped back to raw UUIDs before calling the Vespa query layer. Confirm whether Vespa expects:
- Vespa document IDs (original
vespaDocId, e.g.clf-12345…), in which case useitem.vespaDocIdwhen prefixing inutils.ts, or- Raw Postgres UUIDs, in which case drop the
clf-/clfd-/cl-prefixes inutils.tsto avoid passing invalid identifiers.
2 participants
Description
if the path is passed to AgentMessageApi , while doing the vespa call we replace the kb integration with the path referenced folder or file
Testing
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes