fix: fixed some bugs and improved agent reasoning steps#1270
Open
Himanshvarma wants to merge 10 commits intomainfrom
Open
fix: fixed some bugs and improved agent reasoning steps#1270Himanshvarma wants to merge 10 commits intomainfrom
Himanshvarma wants to merge 10 commits intomainfrom
Conversation
Himanshvarma
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive reasoning system overhaul with structured event-based telemetry, integrates Knowledge Base search and file results throughout the stack, implements tool cooldown management with turn-based recovery, adds environment-based agentic-by-default mode, and threads timing metadata through the chat pipeline. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as ChatPage
participant UseChatStream as useChatStream
participant Backend as Server API
participant Vespa as Vespa Search
User->>Frontend: Submit chat query
Frontend->>Backend: POST /api/chat (streamTimeTakenMs tracking)
par Parallel Search Execution
Backend->>Vespa: Main search (global/tools)
Backend->>Vespa: KB search (searchVespaKnowledgeBase)
and
Backend->>Backend: Execute tools & agent logic
end
Vespa-->>Backend: Main results + KB results
Backend->>Backend: Merge results by docId (prefer KB if higher relevance)
Backend->>UseChatStream: Stream ReasoningEventPayload events (structured)
UseChatStream->>UseChatStream: Parse & accumulate via ReasoningProvider
UseChatStream-->>Frontend: timeTakenMs in ResponseMetadata
Frontend->>Frontend: Render EnhancedReasoning with ReasoningContext
alt isStreaming
Frontend->>Frontend: Show StreamingReasoning (sliding window)
else Post-Stream
Frontend->>Frontend: Show MergedReasoning (full plan+steps)
end
Frontend-->>User: Display reasoning + merged search results + timeTakenMs
sequenceDiagram
participant Agent as Message Agent
participant ToolMgr as ToolCooldownManager
participant Emitter as ReasoningEmitter
participant Client as SSE Stream
Agent->>ToolMgr: recoverExpiredTools(currentTurn)
ToolMgr-->>Agent: List of recovered tools
Agent->>Emitter: ReasoningSteps.toolRecovered(recovered)
Emitter->>Client: Send ReasoningEventPayload
Agent->>Agent: Select available tools (filtered by cooldown)
Agent->>Emitter: ReasoningSteps.toolSelected(toolName, query)
Emitter->>Client: Send ReasoningEventPayload
Agent->>Agent: Execute tool
alt Tool succeeds
Agent->>Emitter: ReasoningSteps.toolCompleted(toolName, false)
Emitter->>Client: Send success event
else Tool fails
Agent->>ToolMgr: recordFailure(toolName, error, currentTurn)
ToolMgr->>ToolMgr: Increment count, check threshold
alt Threshold reached
ToolMgr->>Agent: Returns true (cooldown activated)
Agent->>Emitter: ReasoningSteps.toolCooldownApplied(...)
else Threshold not reached
ToolMgr->>Agent: Returns false (still available)
Agent->>Emitter: ReasoningSteps.toolCompleted(toolName, true)
end
Emitter->>Client: Send failure/cooldown event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Summary of ChangesHello, 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 significantly enhances the agentic chat experience by overhauling the reasoning display, introducing a structured event system for agent activity, and implementing a robust tool cooldown mechanism. These changes, alongside bug fixes for default agentic mode settings and improved attachment handling, aim to deliver a more reliable, performant, and user-friendly conversational AI. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring of the agent reasoning and streaming architecture, moving from a freeform text-based system to a structured event-based one. This is a significant improvement for maintainability and frontend user experience. The changes also include a new tool failure cooldown mechanism, more robust context handling, and various bug fixes. My review focuses on a potentially impactful change to context chunking.
Note: Security Review did not run due to the size of the PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
server/ai/fetchModels.ts (1)
324-327:⚠️ Potential issue | 🟡 MinorUpdate log messages to match the new model version.
The log messages still reference "Claude Sonnet 4.5" and "Claude Opus 4.5", which is inconsistent with the updated display names "Claude Sonnet 4.6" and "Claude Opus 4.6" at lines 206 and 210.
📝 Proposed fix to align log messages
if (isSonnet46(id) && config.allowSonnet46) { - Logger.info("Allowing Claude Sonnet 4.5 model despite litellm_provider not being 'hosted_vllm'") + Logger.info("Allowing Claude Sonnet 4.6 model despite litellm_provider not being 'hosted_vllm'") } if (isOpus46(id) && config.allowOpus46) { - Logger.info("Allowing Claude Opus 4.5 model despite litellm_provider not being 'hosted_vllm'") + Logger.info("Allowing Claude Opus 4.6 model despite litellm_provider not being 'hosted_vllm'") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/ai/fetchModels.ts` around lines 324 - 327, The log messages referencing "Claude Sonnet 4.5" and "Claude Opus 4.5" are outdated; update the Logger.info calls that occur when allowing these models (the branches using the Sonnet/Opus checks, e.g., the isOpus46(id) check and the corresponding Sonnet check) to use the new display names "Claude Sonnet 4.6" and "Claude Opus 4.6" so they match the display name updates at the model definition lines.server/db/message.ts (1)
276-283:⚠️ Potential issue | 🟠 MajorExclude soft-deleted rows from the detailed feedback list.
This query still omits
deletedAt IS NULL, while the aggregate query above includes it. A soft-deleted message will disappear from the totals but still show up infeedbackMessages, which makes the stats inconsistent and can resurface deleted feedback text.Suggested fix
.where( and( inArray(messages.chatExternalId, chatExternalIds), eq(messages.email, email), eq(messages.workspaceExternalId, workspaceExternalId), + isNull(messages.deletedAt), eq(messages.isSummary, false), sql`${messages.feedback}->>'type' IN ('like', 'dislike')`, ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/db/message.ts` around lines 276 - 283, The feedbackMessages query is missing a filter to exclude soft-deleted rows, causing deleted messages to appear in the detailed feedback list; update the .where(...) clause that builds feedbackMessages to also require messages.deletedAt IS NULL (e.g. add eq(messages.deletedAt, null) or the equivalent isNull(messages.deletedAt) condition alongside inArray(messages.chatExternalId, ...), eq(messages.email, ...), eq(messages.workspaceExternalId, ...), eq(messages.isSummary, false), and the existing sql feedback type check) so the detailed list matches the aggregate totals and omits soft-deleted feedback.server/api/chat/tools/slack/getSlackMessages.ts (1)
206-221:⚠️ Potential issue | 🟠 MajorAddress potential fragment deduplication issues with duplicate docIds.
The change from
buildChunkFragmentId(citation.docId, idx)tocitation.docIdremoves index-based uniqueness. Fragment IDs are used for set-based deduplication downstream (message-agents.ts:1872), and ifallItemscontains duplicate docIds (possible when thread root messages appear in both initial search results and thread fetches), legitimate fragments will be incorrectly filtered out.Ensure docId uniqueness in
allItemsbefore fragment creation, or restore the index-based uniqueness guarantee.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chat/tools/slack/getSlackMessages.ts` around lines 206 - 221, The fragment ID change replaced buildChunkFragmentId(citation.docId, idx) with citation.docId which breaks downstream set-based deduplication (see message-agents.ts:1872) when allItems contains duplicate docIds; restore uniqueness by either (A) deduplicating allItems by docId before mapping to MinimalAgentFragment (use searchToCitation(item).docId to collapse duplicates) or (B) restore index-based uniqueness when creating fragments (use buildChunkFragmentId(citation.docId, idx) or append the item's index) in the fragment creation block that calls searchToCitation and answerContextMap so fragments.id remains unique for allItems.frontend/src/components/ChatBox.tsx (1)
914-931:⚠️ Potential issue | 🔴 CriticalCritical:
errorMessageused before declaration causes runtime error.Line 921 references
errorMessage, but it's not defined until line 929. This will throw aReferenceErrordue to the temporal dead zone when an upload is aborted.🐛 Proposed fix
} catch (error) { if (error instanceof Error && error.name === "AbortError") { + const abortMessage = "Upload cancelled" setSelectedFiles((prev) => prev.map((f) => f.id === selectedFile.id ? { ...f, uploading: false, - uploadError: errorMessage, + uploadError: abortMessage, } : f, ), ) return null } const errorMessage = error instanceof Error ? error.message : "Upload failed"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ChatBox.tsx` around lines 914 - 931, The catch block in ChatBox.tsx uses errorMessage before it's declared which causes a runtime ReferenceError; fix by computing the errorMessage immediately after entering the catch (e.g., declare const errorMessage = error instanceof Error ? error.message : "Upload failed") and then handle the AbortError branch using that variable when calling setSelectedFiles for the file with id selectedFile.id (the setSelectedFiles update that flips uploading to false and sets uploadError). Ensure the error instanceof Error check is reused (or repeated) as needed and remove the later declaration so no duplicate definitions remain.server/api/chat/message-agents.ts (2)
1870-1873:⚠️ Potential issue | 🟠 MajorFilter previously seen contexts with the same dedup key you store.
gatheredFragmentsKeysis populated withgetFragmentDedupKey(), but this filter still checksc.id. When the same Vespa doc comes back under a different synthetic fragment id, it survivesfilteredContexts, burns ranking tokens, and can crowd out genuinely new docs beforeaddToolFragments()drops it later.Suggested fix
- const filteredContexts = contexts.filter( - (c: MinimalAgentFragment) => !gatheredFragmentsKeys.has(c.id) - ) + const filteredContexts = contexts.filter((c: MinimalAgentFragment) => { + const key = getFragmentDedupKey(c) + return !key || !gatheredFragmentsKeys.has(key) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chat/message-agents.ts` around lines 1870 - 1873, The filter is using c.id but gatheredFragmentsKeys contains dedup keys from getFragmentDedupKey(), so replace the membership check to compare getFragmentDedupKey(c) against gatheredFragmentsKeys (or precompute a set of keys for contexts and filter by that) so previously-seen fragments are correctly excluded; update the filteredContexts creation (where contexts, MinimalAgentFragment and gatheredFragmentsKeys are referenced) to call getFragmentDedupKey for each context instead of using c.id so duplicates by dedup key are filtered before addToolFragments runs.
1643-1654:⚠️ Potential issue | 🟡 MinorDon't emit a "skipped" validation step unless you actually skip.
This branch now sends
toolValidationError, but the hook explicitly continues into normal execution. The UI will say "Skipping this step due to invalid input." even when the tool still runs and may succeed or fail later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chat/message-agents.ts` around lines 1643 - 1654, The code emits ReasoningSteps.toolValidationError (via emitReasoningEvent) even though execution continues; change this to a non-skipping event or only emit the "error/skipped" event when you actually abort execution. Specifically, in the validateToolInput branch referencing validateToolInput, emitReasoningEvent with a new/alternate step (e.g., ReasoningSteps.toolValidationWarning or similar) that indicates a validation warning, or move the ReasoningSteps.toolValidationError call to the branch where you actually skip/return; keep Logger.warn as-is but do not send the "skipped" error event unless the tool is truly skipped. Ensure the code updates use emitReasoningEvent, ReasoningSteps.toolValidationError, and validateToolInput identifiers when making the change.server/shared/types.ts (1)
744-749:⚠️ Potential issue | 🟡 MinorReject invalid
createdAtstrings instead of transforming them intoInvalid Date.
new Date("bad")still returns aDateobject, so malformed attachment metadata will pass this schema and fail later on serialization or comparison.Suggested fix
- createdAt: z.union([z.string(), z.date()]).transform((val) => { - if (typeof val === "string") { - return new Date(val) - } - return val - }), + createdAt: z.preprocess( + (val) => + val instanceof Date || typeof val === "string" ? new Date(val) : val, + z.date().refine( + (date) => !Number.isNaN(date.getTime()), + "Invalid createdAt", + ), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/types.ts` around lines 744 - 749, The createdAt schema currently transforms any string via new Date(...) which allows invalid dates; update the createdAt handling to explicitly validate parsed dates and reject bad strings instead of returning Invalid Date. In the createdAt transform (in server/shared/types.ts) parse the string into a Date, check validity via isNaN(parsed.getTime()) (or similar), and if invalid cause the Zod validation to fail (e.g., throw or return an error/refine), otherwise return the valid Date; alternatively use z.preprocess or z.string().refine + z.date() to enforce and reject malformed date strings before producing a Date object.
🧹 Nitpick comments (4)
server/api/chat/utils.ts (1)
154-202: Minor redundancy in deduplication logic.The
seenSet already prevents duplicates from being added tofileIds, making theArray.from(new Set(fileIds))calls at lines 173, 184, 195, and 201 redundant. Consider simplifying to justfileIds.slice(0, maxFiles).♻️ Optional simplification
- if (fileIds.length >= maxFiles) return Array.from(new Set(fileIds)).slice(0, maxFiles) + if (fileIds.length >= maxFiles) return fileIds.slice(0, maxFiles)Apply similar change to lines 184, 195, and 201.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chat/utils.ts` around lines 154 - 202, In collectReferencedFileIdsUntilCompaction, the seen Set already prevents duplicates so replace the redundant Array.from(new Set(fileIds)).slice(0, maxFiles) early returns (inside the attachments, fileIds, and sources loops) and the final return with simple fileIds.slice(0, maxFiles); ensure all early-return sites that currently call Array.from(new Set(fileIds)).slice(...) instead return fileIds.slice(0, maxFiles) so deduplication is only handled by seen and the result is still trimmed to MAX_FILES.frontend/src/components/StreamingReasoning.tsx (2)
104-113: Clarify the hardcodedindex={0}usage.The
indexprop is hardcoded to0for all items. If this is intentional (perhaps the index isn't used for display purposes), consider adding a comment. Otherwise, if the index matters for styling or behavior, consider using the actual index fromvisibleMainItems.📝 Option 1: Add clarifying comment
<ReasoningStepComponent step={item.step} - index={0} + index={0} // index not used for display; only step content matters isStreaming={isStreaming}📝 Option 2: Use actual index if needed
- {visibleMainItems.map((item) => ( + {visibleMainItems.map((item, idx) => ( <div key={item.key} className="reasoning-step-enter"> {item.kind === "tool" ? ( ... ) : ( <ReasoningStepComponent step={item.step} - index={0} + index={idx} isStreaming={isStreaming}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/StreamingReasoning.tsx` around lines 104 - 113, The ReasoningStepComponent call is passing a hardcoded index={0} which is ambiguous; either add a short inline comment explaining index is intentionally constant (e.g., "index not used for display/ordering") or change it to use the actual loop index from visibleMainItems (e.g., pass the iterator index variable) so styling/behavior depending on index works correctly; update the JSX where ReasoningStepComponent is rendered to reference the real index or add the clarifying comment next to the index prop to make the intent explicit.
62-70: Move CSS keyframes outside the render function.The inline
<style>tag is recreated on every render. Consider moving these keyframes to a CSS file, a global stylesheet, or using a CSS-in-JS solution with proper caching.♻️ Move to a CSS module or global styles
Create a CSS file or add to existing global styles:
/* In a CSS file or global styles */ `@keyframes` rsStepIn { from { opacity: 0; transform: translateY(6px); } to { opacity: 1; transform: translateY(0); } } .reasoning-step-enter { animation: rsStepIn 0.18s ease-out forwards; }Then remove the inline
<style>block from the component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/StreamingReasoning.tsx` around lines 62 - 70, The inline <style> block inside the StreamingReasoning component recreates the `@keyframes` rsStepIn and .reasoning-step-enter rules on every render; move those rules out of the render into a CSS file (or CSS module/global stylesheet or a cached CSS-in-JS solution) by adding the `@keyframes` rsStepIn and .reasoning-step-enter animation to your stylesheet and then remove the inline <style> tag from StreamingReasoning.tsx so the component simply uses the .reasoning-step-enter class without redefining keyframes each render.frontend/src/components/MergedReasoning.tsx (1)
30-33: Consider scrollbar visibility for accessibility.Hiding the scrollbar entirely (
scrollbarWidth: "none") may impact users who rely on visible scrollbars for navigation cues. Consider using a subtle scrollbar or keeping the default behavior.♿ Optional: Use subtle scrollbar instead of hiding
<div className="max-h-80 overflow-y-auto pr-1" - style={{ scrollbarWidth: "none", msOverflowStyle: "none" }} + style={{ scrollbarWidth: "thin" }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MergedReasoning.tsx` around lines 30 - 33, The current div in the MergedReasoning component that uses className "max-h-80 overflow-y-auto pr-1" hides the scrollbar via inline styles (scrollbarWidth: "none", msOverflowStyle: "none"), which reduces accessibility; instead, remove those inline styles and either keep the default scrollbar or implement a subtle, accessible scrollbar (e.g., use CSS rules or a Tailwind utility to make the scrollbar thin/low-contrast or add ::-webkit-scrollbar styles) so users still get a visible scroll cue while preserving the overflow-y-auto behavior in the MergedReasoning component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ReasoningContext.tsx`:
- Around line 211-235: The current buildToolGroupItems (and similarly
buildFlatItems) groups all steps by toolExecutionId across the entire array,
which pulls later non-contiguous occurrences forward; change the algorithm to
group only contiguous runs: iterate the steps in order, maintain a current block
state (currentToolId/currentDelegationId) and accumulate steps while the id
stays the same, flushing the block to items when the id changes or when a
non-tool step is encountered; when you flush, emit a single "tool" item with the
accumulated steps and key derived from that block (use the first step's
id/timestamp), and for non-tool steps emit individual "step" items; update
buildToolGroupItems to stop using the global toolBlocks map and implement the
single-pass contiguous grouping, and apply the same contiguous grouping logic to
buildFlatItems for delegationRunId and toolExecutionId.
- Around line 579-583: The component currently mounts expanded then collapses in
useEffect, causing a flash; initialize the expanded state from the isStreaming
prop and synchronize on changes instead of correcting after paint. Replace
useState(true) with useState initialized from isStreaming (e.g., useState(() =>
isStreaming)) and update the effect to setExpanded(isStreaming) (or simply
remove the effect and drive expansion directly from isStreaming changes) so the
memoized component (the ToolBlock-like component using expanded and setExpanded)
starts in the correct state and updates when isStreaming changes.
- Around line 404-417: The fallback logic only scans top-level steps for a stage
and misses earlier substeps inside the latest turn, so update the search to walk
substeps backward first: when computing latestStage (using variables lastTop,
lastTop.substeps, latestStage and enum ReasoningEventType.TurnStarted), if
lastTop.substeps exists scan its elements from end to start to find the first
substep with a .stage and use that; if none found then fall back to
lastTop.stage and finally do the existing backwards scan over steps to find a
stage. Ensure you reference lastTop.substeps[lastTop.substeps.length - 1]
replacement with a reverse loop over substeps before the top-level fallback.
- Around line 163-169: The regex in const parameterPattern is too permissive
because the case-insensitive flag makes the lookahead \n[A-Z] match lowercase
and prematurely end multi-line key/value blocks; update parameterPattern to
remove the case-insensitive 'i' flag and tighten the lookahead to a real section
boundary (e.g., require a blank line OR a newline followed by an uppercase
header token and a colon like "HeaderName:"), so the match for Parameters:
captures multi-line key/value pairs (e.g., "query: ...\nlimit: ...") until an
actual section boundary or EOF; locate parameterPattern in ReasoningContext.tsx
and replace the pattern accordingly.
In `@frontend/src/routes/_authenticated/chat.tsx`:
- Around line 228-232: The Zod schema for agentic currently forces
chatParams.agentic to always be boolean (due to .default("false")), so the state
init in isAgenticMode (which checks chatParams.agentic !== undefined and falls
back to import.meta.env.VITE_AGENTIC_BY_DEFAULT) never hits the fallback; fix by
either 1) updating the Zod schema for the agentic field (remove or change
.default("false") so agentic can be undefined/preserved) so chatParams.agentic
can be undefined and the existing isAgenticMode logic works, or 2) keep the
schema but change the initialization in
frontend/src/routes/_authenticated/chat.tsx to derive the sentinel from the raw
URL search params (e.g., check URLSearchParams.get('agentic') before Zod
parsing) and then decide the default using
import.meta.env.VITE_AGENTIC_BY_DEFAULT; target symbols: agentic Zod schema,
chatParams.agentic, isAgenticMode / setIsAgenticMode, and
VITE_AGENTIC_BY_DEFAULT.
In `@frontend/src/routes/_authenticated/index.tsx`:
- Around line 39-41: Guard parsing of the persisted AGENTIC_STATE: when reading
storedValue from localStorage (variable storedValue), wrap JSON.parse in a
try/catch and on any parse error or if the parsed result is not a boolean
(typeof result !== "boolean") return the fallback value
import.meta.env.VITE_AGENTIC_BY_DEFAULT === "true"; ensure only a boolean
payload is accepted and any malformed or non-boolean stored value falls back to
the env default.
In `@server/api/chat/agents.ts`:
- Around line 1188-1191: The log message claims the code is "Using default" but
actualModelId is being set to the invalid modelId; update the log to reflect the
real behavior (e.g., `Invalid model: ${modelId}. Retaining provided modelId`) so
it matches the assignment, or alternatively make the code match the message by
setting actualModelId to the desired default constant instead; locate the
assignment to actualModelId and the loggerWithChild({...}).error call and either
change the error text to indicate retention of modelId or change the assignment
to use the default model constant from your Models enum/label mappings.
In `@server/api/chat/chat.ts`:
- Around line 5048-5051: The code is incorrectly assigning an invalid modelId
into actualModelId (actualModelId = modelId) which prevents the later fallback
(actualModelId || config.defaultBestModel) from using the default; remove that
assignment (or set actualModelId = undefined/null) in the branch that logs the
invalid model so actualModelId remains falsy and the fallback
config.defaultBestModel is used; keep the loggerWithChild(...).error call as-is
to record the invalid model.
In `@server/api/chat/message-agents.ts`:
- Around line 4062-4068: When reconstructing agentContext you currently merge
referencedFileIds with historyFileIds expanded via expandSheetIds but only
source image attachments from the current request's attachmentMetadata; collect
historical image/file attachment IDs from previousConversationHistory (e.g.,
parse prior turns' attachmentMetadata or call
collectReferencedFileIdsUntilCompaction on previousConversationHistory including
image types) and include those IDs in the same Set used to build
allReferencedFileIds so prior images are available during context rebuild;
update the same logic used around collectReferencedFileIdsUntilCompaction,
expandSheetIds and wherever agentContext is rebuilt (also apply the same change
at the other similar block that uses referencedFileIds/attachmentMetadata) so
historical image attachments are rehydrated into agentContext.
- Around line 2320-2327: The toolCompleted event is using hookContext.status to
decide failure but earlier the code normalizes non-"success" into record.status
= "error", causing mismatch; update the emit call in the block that calls
emitReasoningEvent with ReasoningSteps.toolCompleted to use the normalized
status (e.g., record.status or the normalizedStatus variable) instead of
hookContext.status so the emitted toolCompleted reflects the same normalized
error state used elsewhere.
In `@server/api/chat/reasoning-steps.ts`:
- Around line 80-84: The displayText in turnStarted
(ReasoningEventType.TurnStarted) assumes 1-based turns but MessageAgents uses
0-based turns, so update turnStarted to treat turnNumber === 0 as the first turn
(use "Starting research.") and for subsequent turns compute a human-facing pass
number by using turnNumber + 1 in the "Starting search pass ..." message; adjust
the conditional logic in the turnStarted function accordingly.
In `@server/db/message.ts`:
- Around line 57-90: getChatMessagesUntilCompaction currently returns the
summary row and uses only createdAt as a boundary, causing correctness and race
issues; change it to fetch the summary row's id (use lastSummary[0].id), exclude
summary messages (isSummary = false) in the subsequent query, and replace the
single-column cutoff with a stable two-column boundary by returning rows where
(createdAt < cutoffCreatedAt) OR (createdAt = cutoffCreatedAt AND id <=
cutoffId) so ties are broken by id; keep the early-return to
getChatMessagesWithAuth when lastSummary is empty and still parse the final
result with selectMessageSchema.
---
Outside diff comments:
In `@frontend/src/components/ChatBox.tsx`:
- Around line 914-931: The catch block in ChatBox.tsx uses errorMessage before
it's declared which causes a runtime ReferenceError; fix by computing the
errorMessage immediately after entering the catch (e.g., declare const
errorMessage = error instanceof Error ? error.message : "Upload failed") and
then handle the AbortError branch using that variable when calling
setSelectedFiles for the file with id selectedFile.id (the setSelectedFiles
update that flips uploading to false and sets uploadError). Ensure the error
instanceof Error check is reused (or repeated) as needed and remove the later
declaration so no duplicate definitions remain.
In `@server/ai/fetchModels.ts`:
- Around line 324-327: The log messages referencing "Claude Sonnet 4.5" and
"Claude Opus 4.5" are outdated; update the Logger.info calls that occur when
allowing these models (the branches using the Sonnet/Opus checks, e.g., the
isOpus46(id) check and the corresponding Sonnet check) to use the new display
names "Claude Sonnet 4.6" and "Claude Opus 4.6" so they match the display name
updates at the model definition lines.
In `@server/api/chat/message-agents.ts`:
- Around line 1870-1873: The filter is using c.id but gatheredFragmentsKeys
contains dedup keys from getFragmentDedupKey(), so replace the membership check
to compare getFragmentDedupKey(c) against gatheredFragmentsKeys (or precompute a
set of keys for contexts and filter by that) so previously-seen fragments are
correctly excluded; update the filteredContexts creation (where contexts,
MinimalAgentFragment and gatheredFragmentsKeys are referenced) to call
getFragmentDedupKey for each context instead of using c.id so duplicates by
dedup key are filtered before addToolFragments runs.
- Around line 1643-1654: The code emits ReasoningSteps.toolValidationError (via
emitReasoningEvent) even though execution continues; change this to a
non-skipping event or only emit the "error/skipped" event when you actually
abort execution. Specifically, in the validateToolInput branch referencing
validateToolInput, emitReasoningEvent with a new/alternate step (e.g.,
ReasoningSteps.toolValidationWarning or similar) that indicates a validation
warning, or move the ReasoningSteps.toolValidationError call to the branch where
you actually skip/return; keep Logger.warn as-is but do not send the "skipped"
error event unless the tool is truly skipped. Ensure the code updates use
emitReasoningEvent, ReasoningSteps.toolValidationError, and validateToolInput
identifiers when making the change.
In `@server/api/chat/tools/slack/getSlackMessages.ts`:
- Around line 206-221: The fragment ID change replaced
buildChunkFragmentId(citation.docId, idx) with citation.docId which breaks
downstream set-based deduplication (see message-agents.ts:1872) when allItems
contains duplicate docIds; restore uniqueness by either (A) deduplicating
allItems by docId before mapping to MinimalAgentFragment (use
searchToCitation(item).docId to collapse duplicates) or (B) restore index-based
uniqueness when creating fragments (use buildChunkFragmentId(citation.docId,
idx) or append the item's index) in the fragment creation block that calls
searchToCitation and answerContextMap so fragments.id remains unique for
allItems.
In `@server/db/message.ts`:
- Around line 276-283: The feedbackMessages query is missing a filter to exclude
soft-deleted rows, causing deleted messages to appear in the detailed feedback
list; update the .where(...) clause that builds feedbackMessages to also require
messages.deletedAt IS NULL (e.g. add eq(messages.deletedAt, null) or the
equivalent isNull(messages.deletedAt) condition alongside
inArray(messages.chatExternalId, ...), eq(messages.email, ...),
eq(messages.workspaceExternalId, ...), eq(messages.isSummary, false), and the
existing sql feedback type check) so the detailed list matches the aggregate
totals and omits soft-deleted feedback.
In `@server/shared/types.ts`:
- Around line 744-749: The createdAt schema currently transforms any string via
new Date(...) which allows invalid dates; update the createdAt handling to
explicitly validate parsed dates and reject bad strings instead of returning
Invalid Date. In the createdAt transform (in server/shared/types.ts) parse the
string into a Date, check validity via isNaN(parsed.getTime()) (or similar), and
if invalid cause the Zod validation to fail (e.g., throw or return an
error/refine), otherwise return the valid Date; alternatively use z.preprocess
or z.string().refine + z.date() to enforce and reject malformed date strings
before producing a Date object.
---
Nitpick comments:
In `@frontend/src/components/MergedReasoning.tsx`:
- Around line 30-33: The current div in the MergedReasoning component that uses
className "max-h-80 overflow-y-auto pr-1" hides the scrollbar via inline styles
(scrollbarWidth: "none", msOverflowStyle: "none"), which reduces accessibility;
instead, remove those inline styles and either keep the default scrollbar or
implement a subtle, accessible scrollbar (e.g., use CSS rules or a Tailwind
utility to make the scrollbar thin/low-contrast or add ::-webkit-scrollbar
styles) so users still get a visible scroll cue while preserving the
overflow-y-auto behavior in the MergedReasoning component.
In `@frontend/src/components/StreamingReasoning.tsx`:
- Around line 104-113: The ReasoningStepComponent call is passing a hardcoded
index={0} which is ambiguous; either add a short inline comment explaining index
is intentionally constant (e.g., "index not used for display/ordering") or
change it to use the actual loop index from visibleMainItems (e.g., pass the
iterator index variable) so styling/behavior depending on index works correctly;
update the JSX where ReasoningStepComponent is rendered to reference the real
index or add the clarifying comment next to the index prop to make the intent
explicit.
- Around line 62-70: The inline <style> block inside the StreamingReasoning
component recreates the `@keyframes` rsStepIn and .reasoning-step-enter rules on
every render; move those rules out of the render into a CSS file (or CSS
module/global stylesheet or a cached CSS-in-JS solution) by adding the
`@keyframes` rsStepIn and .reasoning-step-enter animation to your stylesheet and
then remove the inline <style> tag from StreamingReasoning.tsx so the component
simply uses the .reasoning-step-enter class without redefining keyframes each
render.
In `@server/api/chat/utils.ts`:
- Around line 154-202: In collectReferencedFileIdsUntilCompaction, the seen Set
already prevents duplicates so replace the redundant Array.from(new
Set(fileIds)).slice(0, maxFiles) early returns (inside the attachments, fileIds,
and sources loops) and the final return with simple fileIds.slice(0, maxFiles);
ensure all early-return sites that currently call Array.from(new
Set(fileIds)).slice(...) instead return fileIds.slice(0, maxFiles) so
deduplication is only handled by seen and the result is still trimmed to
MAX_FILES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f7e9b65-72aa-4dba-b800-fc4780555e23
📒 Files selected for processing (25)
frontend/src/components/ChatBox.tsxfrontend/src/components/EnhancedReasoning.tsxfrontend/src/components/MergedReasoning.tsxfrontend/src/components/ReasoningContext.tsxfrontend/src/components/StreamingReasoning.tsxfrontend/src/hooks/useChatStream.tsfrontend/src/routes/_authenticated/agent.tsxfrontend/src/routes/_authenticated/chat.tsxfrontend/src/routes/_authenticated/index.tsxserver/ai/context.tsserver/ai/fetchModels.tsserver/api/chat/agent-schemas.tsserver/api/chat/agents.tsserver/api/chat/chat.tsserver/api/chat/jaf-provider.tsserver/api/chat/message-agents.tsserver/api/chat/reasoning-steps.tsserver/api/chat/tool-cooldown.tsserver/api/chat/tool-schemas.tsserver/api/chat/tools/slack/getSlackMessages.tsserver/api/chat/tools/utils.tsserver/api/chat/utils.tsserver/db/message.tsserver/db/schema/messages.tsserver/shared/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/message-agents.ts (1)
5364-5389:⚠️ Potential issue | 🟠 MajorRecord turn 0 expectations before review runs.
currentTurn > 0now skips persistence for the first turn, butturn_starthas already happened, so nothing flushes that buffer before the same turn's tool-error or turn-end review. Turn 0 reviews therefore miss theexpected_resultsthey are supposed to evaluate.Suggested fix
- if (currentTurn > 0) { + if (currentTurn >= MIN_TURN_NUMBER) {Apply the same change in the delegated-agent path.
Also applies to: 6598-6609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chat/message-agents.ts` around lines 5364 - 5389, The first-turn expectations are being buffered instead of persisted which causes turn 0 reviews to miss expected_results; update the delegated-agent path to mirror the main-agent change so that whenever extractedExpectations are collected you push them into pendingExpectations and agentContext.currentTurnArtifacts.expectations and then call recordExpectationsForTurn(currentTurn, extractedExpectations) when currentTurn >= 0 (i.e., ensure turn 0 is recorded rather than only currentTurn > 0), otherwise push to expectationBuffer only for genuinely future turns; apply this same fix in the delegated-agent block referenced around the other occurrence (the block around lines 6598-6609) so both paths use the same currentTurn handling and calls to recordExpectationsForTurn, Logger.debug, pendingExpectations, expectationBuffer, and agentContext.currentTurnArtifacts.expectations.
♻️ Duplicate comments (2)
server/api/chat/message-agents.ts (1)
4062-4068:⚠️ Potential issue | 🟠 MajorCarry forward prior image attachments when rebuilding context.
This merge only rehydrates historical non-image file ids. Follow-up questions about an image uploaded in an earlier turn still rebuild
initialAttachmentContextwithout that image, while prior documents remain available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chat/message-agents.ts` around lines 4062 - 4068, The current rebuild only adds expanded sheet ids from history, dropping prior image attachments; update the aggregation that builds allReferencedFileIds so it carries forward raw historical file ids (historyFileIds) in addition to sheet expansions. Specifically, in the block that computes allReferencedFileIds (which uses referencedFileIds, historyFileIds, and expandSheetIds), include historyFileIds directly (not only historyFileIds.flatMap(...)) so image file ids from collectReferencedFileIdsUntilCompaction are preserved when reconstructing initialAttachmentContext.frontend/src/components/ReasoningContext.tsx (1)
404-423:⚠️ Potential issue | 🟡 MinorKeep scanning older turn substeps before defaulting the progress banner.
If the newest turn has no staged substep, the fallback still only checks
steps[i].stage. That can reset the banner to"Understanding your query..."even though an earlier turn already reachedgatheringoranalyzing.🔎 Tight fallback fix
latestStage ??= lastTop.stage if (!latestStage) { - for (let i = steps.length - 1; i >= 0; i--) { - if (steps[i].stage) { - latestStage = steps[i].stage - break - } - } + for (let i = steps.length - 1; i >= 0 && !latestStage; i--) { + const step = steps[i] + if (step.type === ReasoningEventType.TurnStarted && step.substeps?.length) { + for (let j = step.substeps.length - 1; j >= 0; j--) { + if (step.substeps[j].stage) { + latestStage = step.substeps[j].stage + break + } + } + } + latestStage ??= step.stage + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ReasoningContext.tsx` around lines 404 - 423, The fallback only checks steps[i].stage and misses staged substeps in older turns, so update the fallback scanning logic (after latestStage ??= lastTop.stage) to also inspect each steps[i].substeps for a stage (mirroring the earlier loop over lastTop.substeps) before falling back to steps[i].stage; specifically, in the for loop that iterates steps from the end, check steps[i].substeps (if present) from last to first for a .stage and set latestStage to that stage, otherwise fall back to steps[i].stage, ensuring STAGE_PROGRESS_TEXT[latestStage] uses the most recent substep stage when present.
🧹 Nitpick comments (3)
frontend/src/routes/_authenticated/chat.tsx (1)
2601-2605: Consider aligning with theshouldWireClarificationpattern for robustness.The condition
message.externalId === "current-resp"works whencurrentResp.messageIdis null, but if the messageId is assigned before streaming ends, this check fails while the message is still the active streaming response. TheshouldWireClarificationcheck at line 2509 handles this more robustly.♻️ Optional: more robust check
timeTakenMs={ - message.externalId === "current-resp" + currentResp && message.externalId === (currentResp.messageId || "current-resp") ? streamTimeTakenMs : message.timeTakenMs ?? undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/_authenticated/chat.tsx` around lines 2601 - 2605, The timeTakenMs assignment should use the same robust check as shouldWireClarification instead of comparing message.externalId === "current-resp"; update the ternary that sets timeTakenMs (currently switching between streamTimeTakenMs and message.timeTakenMs) to use the shouldWireClarification(...) result (the same call/site used at line 2509) so it correctly picks streamTimeTakenMs while the response is still streaming even if currentResp.messageId becomes non-null; ensure you reference the same parameters/vars passed to shouldWireClarification when replacing the externalId comparison.frontend/src/components/ReasoningContext.tsx (2)
450-468: Expose disclosure state on the toggle buttons.These controls show and hide content but never publish
aria-expanded, which makes the new reasoning UI harder to understand in a screen reader. Addingaria-expandedhere would close that gap with almost no code.♿ Small accessibility improvement
<button type="button" onClick={() => setExpanded((e) => !e)} + aria-expanded={expanded} className="w-full flex items-center gap-2 px-3 py-2 text-left hover:bg-gray-100 dark:hover:bg-slate-700 rounded-lg transition-colors" >Apply the same addition to the
ToolBlockandAgentBlocktoggle buttons. If you want to go one step further, pair it witharia-controlson the collapsible content container.Also applies to: 591-605, 667-679
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ReasoningContext.tsx` around lines 450 - 468, The toggle button in ReasoningContext.tsx is missing the accessible disclosure state; update the button that calls setExpanded and uses the expanded variable to include aria-expanded={expanded} (and optionally add aria-controls linking to the collapsible container's id) so screen readers announce its state; apply the same aria-expanded addition to the corresponding toggle buttons in ToolBlock and AgentBlock (references: setExpanded, expanded, label/plan.goal, completedCount/total, isStreaming/hasInProgress) to ensure consistent accessibility.
496-505: Plan for React 19 JSX typing when upgrading@types/react.The repo currently uses React 19.2.1 with
@types/react18.3.27. While the globalJSXnamespace still works with@types/react18, upgrading to@types/react19 (which should align with React 19) will remove this namespace and requireReact.JSX.Elementinstead.Replace
JSX.ElementwithReact.JSX.Elementin these signatures to prepare for the@types/reactupgrade:Locations to update
ReasoningStepComponentprop type (line 504)ToolBlockprop type (line 583)AgentBlockprop type (line 640)ReasoningContextValuetype (line 745)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ReasoningContext.tsx` around lines 496 - 505, The prop/type declarations use the global JSX namespace which will break when `@types/react` is upgraded; update all occurrences of JSX.Element to React.JSX.Element (including optional null unions) — specifically change the return/prop types in ReasoningStepComponent (prop getAppIcon), ToolBlock (its getAppIcon prop), AgentBlock (its getAppIcon prop) and the ReasoningContextValue type to use React.JSX.Element | null instead of JSX.Element | null so the code remains compatible with `@types/react` 19.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/api/chat/message-agents.ts`:
- Around line 5008-5012: agentContext.runtime.emitReasoning is currently wired
directly to emitReasoningStep, so transient SSE write failures can throw and
abort agent runs; change the binding so emitReasoning is wrapped in a try/catch
that calls emitReasoningStep(payload as ReasoningEventPayload) and logs (but
does not rethrow) any errors from writeSSE, mirroring the error-swallowing
wrapper used elsewhere (e.g., emitReasoningEvent). Apply the same safe wrapper
when setting up the delegated-agent runtime (the delegated-agent runtime
assignment around lines where emitReasoning is wired) so delegated-agent
start/completion and synthesis start/completion calls won't fail the run on SSE
write failures.
- Around line 1786-1795: The dedup logic was partially migrated to
getFragmentDedupKey but elsewhere code still checks
gatheredFragmentsKeys.has(c.id), causing identical Vespa documents with new
fragment ids to bypass dedup; update places that use c.id for dedup (e.g., the
filter at the earlier check that calls gatheredFragmentsKeys.has(c.id)) to
compute the fragment key via getFragmentDedupKey(c) (and skip when it returns
falsy) and use gatheredFragmentsKeys.has(key) instead, ensuring
gatheredFragmentsKeys stores and checks the dedup keys consistently with the
loop that uses getFragmentDedupKey(fragment).
In `@server/api/chat/reasoning-steps.ts`:
- Around line 46-61: TOOL_DISPLAY is missing an entry for
XyneTools.MetadataRetrieval so that metadata retrieval uses the generic fallback
text; add a mapping for XyneTools.MetadataRetrieval to TOOL_DISPLAY (the same
object that contains entries like XyneTools.searchGlobal and
XyneTools.searchKnowledgeBase) with an appropriate executing and completed
string (e.g., "Retrieving metadata." and "Metadata retrieved.") so the module
uses the centralized copy instead of the generic fallback.
---
Outside diff comments:
In `@server/api/chat/message-agents.ts`:
- Around line 5364-5389: The first-turn expectations are being buffered instead
of persisted which causes turn 0 reviews to miss expected_results; update the
delegated-agent path to mirror the main-agent change so that whenever
extractedExpectations are collected you push them into pendingExpectations and
agentContext.currentTurnArtifacts.expectations and then call
recordExpectationsForTurn(currentTurn, extractedExpectations) when currentTurn
>= 0 (i.e., ensure turn 0 is recorded rather than only currentTurn > 0),
otherwise push to expectationBuffer only for genuinely future turns; apply this
same fix in the delegated-agent block referenced around the other occurrence
(the block around lines 6598-6609) so both paths use the same currentTurn
handling and calls to recordExpectationsForTurn, Logger.debug,
pendingExpectations, expectationBuffer, and
agentContext.currentTurnArtifacts.expectations.
---
Duplicate comments:
In `@frontend/src/components/ReasoningContext.tsx`:
- Around line 404-423: The fallback only checks steps[i].stage and misses staged
substeps in older turns, so update the fallback scanning logic (after
latestStage ??= lastTop.stage) to also inspect each steps[i].substeps for a
stage (mirroring the earlier loop over lastTop.substeps) before falling back to
steps[i].stage; specifically, in the for loop that iterates steps from the end,
check steps[i].substeps (if present) from last to first for a .stage and set
latestStage to that stage, otherwise fall back to steps[i].stage, ensuring
STAGE_PROGRESS_TEXT[latestStage] uses the most recent substep stage when
present.
In `@server/api/chat/message-agents.ts`:
- Around line 4062-4068: The current rebuild only adds expanded sheet ids from
history, dropping prior image attachments; update the aggregation that builds
allReferencedFileIds so it carries forward raw historical file ids
(historyFileIds) in addition to sheet expansions. Specifically, in the block
that computes allReferencedFileIds (which uses referencedFileIds,
historyFileIds, and expandSheetIds), include historyFileIds directly (not only
historyFileIds.flatMap(...)) so image file ids from
collectReferencedFileIdsUntilCompaction are preserved when reconstructing
initialAttachmentContext.
---
Nitpick comments:
In `@frontend/src/components/ReasoningContext.tsx`:
- Around line 450-468: The toggle button in ReasoningContext.tsx is missing the
accessible disclosure state; update the button that calls setExpanded and uses
the expanded variable to include aria-expanded={expanded} (and optionally add
aria-controls linking to the collapsible container's id) so screen readers
announce its state; apply the same aria-expanded addition to the corresponding
toggle buttons in ToolBlock and AgentBlock (references: setExpanded, expanded,
label/plan.goal, completedCount/total, isStreaming/hasInProgress) to ensure
consistent accessibility.
- Around line 496-505: The prop/type declarations use the global JSX namespace
which will break when `@types/react` is upgraded; update all occurrences of
JSX.Element to React.JSX.Element (including optional null unions) — specifically
change the return/prop types in ReasoningStepComponent (prop getAppIcon),
ToolBlock (its getAppIcon prop), AgentBlock (its getAppIcon prop) and the
ReasoningContextValue type to use React.JSX.Element | null instead of
JSX.Element | null so the code remains compatible with `@types/react` 19.
In `@frontend/src/routes/_authenticated/chat.tsx`:
- Around line 2601-2605: The timeTakenMs assignment should use the same robust
check as shouldWireClarification instead of comparing message.externalId ===
"current-resp"; update the ternary that sets timeTakenMs (currently switching
between streamTimeTakenMs and message.timeTakenMs) to use the
shouldWireClarification(...) result (the same call/site used at line 2509) so it
correctly picks streamTimeTakenMs while the response is still streaming even if
currentResp.messageId becomes non-null; ensure you reference the same
parameters/vars passed to shouldWireClarification when replacing the externalId
comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 599f8623-6409-4a76-92de-a7b32cca2462
📒 Files selected for processing (5)
frontend/src/components/ReasoningContext.tsxfrontend/src/routes/_authenticated/chat.tsxfrontend/src/routes/_authenticated/index.tsxserver/api/chat/message-agents.tsserver/api/chat/reasoning-steps.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/_authenticated/index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/api/search.ts (1)
527-550:⚠️ Potential issue | 🔴 CriticalRemove
includeKnowledgeBaseInSearchproperty—not supported inVespaQueryConfig.The property
includeKnowledgeBaseInSearchdoes not exist inVespaQueryConfigfrom@xyne/vespa-tsand causes build failureTS2353at lines 533 and 549.To include/exclude the knowledge base, use
excludedAppsinstead. WhenisSebiis true, omitApps.KnowledgeBasefromexcludedApps(or leave it unset). WhenisSebiis false, addApps.KnowledgeBasetoexcludedApps.Remove both instances of
includeKnowledgeBaseInSearch: isSebiand conditionally setexcludedAppsbased on theisSebiflag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/search.ts` around lines 527 - 550, The code is passing an unsupported includeKnowledgeBaseInSearch option to searchVespa (VespaQueryConfig); remove both includeKnowledgeBaseInSearch: isSebi entries and instead set excludedApps based on isSebi when calling searchVespa: if isSebi is true leave excludedApps unset (or omit Apps.KnowledgeBase), otherwise include Apps.KnowledgeBase in excludedApps; update both searchVespa call sites (the one that runs with Promise.all and the else branch) to use excludedApps accordingly while keeping other params (alpha, limit, requestDebug, offset, timestampRange, rankProfile) and leaving updateUserQueryHistory, groupCount, results logic unchanged.server/api/chat/message-agents.ts (1)
3221-3270:⚠️ Potential issue | 🟠 MajorKeep a terminal reasoning event for failed delegations.
agentDelegated(...)is emitted beforeexecuteCustomAgent(), but the error branch returns a plainToolResponse.error(...). Later,afterToolExecutionHook()only emitsagentCompleted(...)for successfulrunPublicAgentresults and also skips the generictoolCompleted(...)path for this tool, so failed delegations never get a closing event and the delegated reasoning block can remain stuck in a running state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chat/message-agents.ts` around lines 3221 - 3270, The failed-delegation branch returns ToolResponse.error without emitting a closing reasoning event, leaving the delegated block open; before returning when toolOutput.error is truthy (inside the run_public_agent flow where delegationRunId and delegatedAgentName are set and executeCustomAgent was called), call and await context.runtime?.emitReasoning with ReasoningSteps.agentCompleted(...) (or an equivalent failure payload) enriched with agent: delegatedAgentName, delegationRunId, parentAgent: "Main" and the error details, then return the ToolResponse.error; use the same emitReasoning pattern used in the success path (mirror the reasoningEmitter shape) so failed delegations always emit a terminal agentCompleted event.
♻️ Duplicate comments (1)
server/api/chat/message-agents.ts (1)
4062-4068:⚠️ Potential issue | 🟠 MajorHistorical image and thread context is still dropped on follow-ups.
This only rehydrates historical non-image
fileIds. PriorimageAttachmentFileIdsandthreadIdsare still sourced exclusively from the current request, so follow-up questions about an earlier uploaded image or an earlier thread-linked email will still rebuildagentContextwithout that context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chat/message-agents.ts` around lines 4062 - 4068, The code only merges non-image fileIds from history (via collectReferencedFileIdsUntilCompaction and expandSheetIds) so historical imageAttachmentFileIds and threadIds are still omitted when rebuilding agentContext; update the logic to also extract and merge historical imageAttachmentFileIds and threadIds from previousConversationHistory (either by extending collectReferencedFileIdsUntilCompaction to return those arrays or adding small helpers like collectReferencedImageAttachmentIdsUntilCompaction and collectReferencedThreadIdsUntilCompaction), then include those IDs when constructing allReferencedFileIds and when populating agentContext (ensure imageAttachmentFileIds and threadIds are combined with the current request’s values).
🧹 Nitpick comments (5)
frontend/src/lib/common.tsx (1)
140-155: Consider consolidating duplicate icon rendering branches.Lines 140-147 and 148-155 return identical JSX. These can be merged into a single condition:
♻️ Proposed consolidation
- } else if (app === Apps.KnowledgeBase && entity === SystemEntity.SystemInfo) { + } else if (app === Apps.KnowledgeBase && (entity === SystemEntity.SystemInfo || entity === "file")) { return ( <BookOpen size={size?.w || 12} className={`${classNameVal} dark:stroke-[`#F1F3F4`]`} stroke="#384049" /> ) - } else if (app === Apps.KnowledgeBase && entity === "file") { - return ( - <BookOpen - size={size?.w || 12} - className={`${classNameVal} dark:stroke-[`#F1F3F4`]`} - stroke="#384049" - /> - ) } else if (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/common.tsx` around lines 140 - 155, Two consecutive branches in the icon-rendering logic both check Apps.KnowledgeBase and return identical BookOpen JSX (the conditions using SystemEntity.SystemInfo and entity === "file"); consolidate them by combining the conditions into a single branch (e.g., check app === Apps.KnowledgeBase && (entity === SystemEntity.SystemInfo || entity === "file")) and keep the single BookOpen return to eliminate duplicate JSX while preserving classNameVal, size, and stroke props.frontend/src/components/SearchResult.tsx (1)
437-443: Type assertions in thekb_itemsbranch indicate incomplete type narrowing.The
kb_itemsbranch requires multiple inline type assertions ((result as { fileName?: string }),(result as { url?: string }),(result as { updatedAt?: number })), while other discriminated union variants (file,user,SearchResultDiscriminatedUniontype may not properly include thekb_itemsvariant with its full property set. If the discriminated union is correctly defined, TypeScript should automatically narrow the type whenresult.type === "kb_items", eliminating the need for manual assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/SearchResult.tsx` around lines 437 - 443, The kb_items branch is using inline type assertions (e.g., (result as { fileName?: string }), (result as { url?: string }), (result as { updatedAt?: number })) because TypeScript isn't narrowing result when result.type === "kb_items"; fix the SearchResultDiscriminatedUnion so it includes a proper kb_items variant with the expected properties (fileName, url, updatedAt, chunk/summary shape) or add a named interface (e.g., KbItemResult) and include it in the union, then remove the inline casts in the SearchResult.tsx branch (where title and chunkText are derived) so result is directly typed and narrows correctly when result.type === "kb_items".frontend/src/components/Autocomplete.tsx (1)
139-146: Thetitlefield access is correct; remove the invalid comparison withSearchResult.tsx.
KbFileAutocompletehas atitlefield per the schema definition (AutocompleteKbFileSchema at line 348 of server/shared/types.ts), sokbResult.titlewill render correctly. SearchResult.tsx uses a different discriminated union type (SearchResultDiscriminatedUnion from@xyne/vespa-ts) with different kb_items schema, making the field name comparison between the two components invalid.The type assertion on line 140 is redundant since AutocompleteSchema is a properly defined discriminated union—TypeScript should automatically narrow the type after the
result.type === "kb_items"check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Autocomplete.tsx` around lines 139 - 146, The code incorrectly compares field names against SearchResult.tsx and uses a redundant type assertion; inside Autocomplete.tsx where you check result.type === "kb_items" (and cast to KbFileAutocomplete), remove the invalid comparison to SearchResult and the unnecessary "as KbFileAutocomplete" cast—rely on the AutocompleteSchema discriminated union narrowing and directly use kbResult.title (or result.title) to render the title, ensuring any import/usage of KbFileAutocomplete matches the AutocompleteSchema definition.server/vespa/schemas/ticket.sd (2)
613-623: Renormalize the title-boost branch before blending it withalpha.Line 613 says the native-rank branch is “just to balance the vector score”, but
vector_scoreis a closeness score in[0, 1]whilenativeRankis a normalized text score. Multiplying the lexical side by5and0.2makes that branch several times larger than the vector branch, soalphastops being an intuitive semantic-vs-lexical dial. If that isn’t intentional, normalize the weighted native-rank sum and reuse it in both phases. (docs.vespa.ai)♻️ Suggested refactor
rank-profile title_boosted_hybrid inherits initial { - `#nativeRank`(chunk) just to balance the vector score + function title_native_rank() { + expression: ((5 * nativeRank(subject)) + (0.2 * nativeRank(description))) / 5.2 + } + first-phase { - expression: (query(alpha) * vector_score) + ((1 - query(alpha)) * (5 * nativeRank(subject)+0.2*nativeRank(description))) + expression: (query(alpha) * vector_score) + ((1 - query(alpha)) * title_native_rank) } global-phase { expression { ( (query(alpha) * vector_score) + - ((1 - query(alpha)) * (5 * nativeRank(subject) + 0.2*nativeRank(description))) + ((1 - query(alpha)) * title_native_rank) ) * doc_recency }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/vespa/schemas/ticket.sd` around lines 613 - 623, The lexical branch (nativeRank(subject)/nativeRank(description)) is scaled unequally to vector_score making query(alpha) non-intuitive; compute a normalized lexical_score by combining nativeRank(subject) and nativeRank(description) into a single weighted sum, then renormalize it back into [0,1] (e.g., divide by the sum of weights or apply min-max scaling) and replace the raw weighted expression in both first-phase and global-phase with that normalized lexical_score so that (query(alpha) * vector_score) + ((1 - query(alpha)) * lexical_score) yields a balanced, consistent alpha blend (also multiply the whole blended expression by doc_recency in global-phase as before).
612-615: Document the query tensor difference between rank profiles.Line 612 inherits
initial, which declaresquery(e)as the tensor input, while the existinghybridprofile (line 479) declaresquery(q). Although no current callers in the codebase reference either profile, ensure any future migration or adoption oftitle_boosted_hybriduses the correct tensor name. If callers switch fromhybridtotitle_boosted_hybrid, they must update query inputs fromquery(q)toquery(e), or addquery(q)to the profile definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/vespa/schemas/ticket.sd` around lines 612 - 615, The rank-profile title_boosted_hybrid currently inherits initial which uses the tensor input name query(e), while the existing hybrid profile uses query(q); update the title_boosted_hybrid profile so its query tensor name matches expected callers — either change its declaration to include query(q) in addition to or instead of query(e), or document/rename it and update any callers to pass query(e) if you intend to keep initial's naming; reference the rank-profile title_boosted_hybrid, the inherited profile initial, and the hybrid profile (which uses query(q)) when making the change so the tensor input names are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ChatBox.tsx`:
- Around line 3114-3116: The agent-mode controls are being hidden solely when
isAgenticByDefault is true, which can remove the toggle even if the effective
mode (isAgenticMode) is false; update the visibility logic in ChatBox.tsx
(around the blocks guarded by {!isAgenticByDefault && showAdvancedOptions} and
the similar block at the second location) to use an effective mode value
instead—either coerce effectiveIsAgentic = isAgenticByDefault || isAgenticMode
and hide controls only when effectiveIsAgentic is true, or ensure you set
effectiveIsAgentic to true when VITE_AGENTIC_BY_DEFAULT is set and only then
hide the Agent toggle/MCP picker; apply this change to both occurrences (the one
near the shown diff and the one at the other reported location).
In `@frontend/src/components/SearchResult.tsx`:
- Around line 447-455: The anchor in SearchResult.tsx uses (result as { url?:
string }).url which doesn't exist on KbFileResponseSchema causing broken links;
update rendering to build a valid href from existing fields instead of relying
on url — for example use a constructed path like `/kb/${result.docId}` or
`/kb/${result.docId}#${result.itemId}` (use result.docId and result.itemId when
present), and ensure the component still falls back to "#" only as a last
resort; adjust the type assertion/remove the bogus url cast and update any
usages of getIcon(result.app, result.entity, ...) to continue working with the
same result object.
In `@server/api/chat/message-agents.ts`:
- Around line 4487-4504: Assign mainRunIdRef via mainRunIdRef.current =
generateRunId() before any calls to emitReasoningStep so the initial events
(e.g., ReasoningSteps.attachmentAnalyzing and
ReasoningSteps.attachmentExtracted) carry a stable runId; update the code path
that prepares attachments (the block calling prepareInitialAttachmentContext and
then emitReasoningStep) to ensure mainRunIdRef is set first, and mirror the same
change for the other occurrence that emits reasoning steps later (the block
around the second emitReasoningStep calls). Ensure you reference mainRunIdRef,
generateRunId(), emitReasoningStep, and the ReasoningSteps.* helpers when making
the change so all early events use the generated runId.
In `@server/api/search.ts`:
- Line 295: The call sites pass an extra positional isSebi flag to functions
that don't support it (autocomplete and groupVespaSearch); fix by removing the
extra positional parameter and either upgrade `@xyne/vespa-ts` to a version that
accepts these options or implement wrapper functions (follow the searchVespa
pattern) named e.g. autocompleteWithSebi and groupVespaSearchWithSebi that
accept (query, email, ..., isSebi), handle the flag internally, build the proper
options object, and then call the underlying library's autocomplete and
groupVespaSearch methods with only the supported parameters; update the calls on
server/api/search.ts to use the new wrappers (or the upgraded library API) and
ensure no extra positional args are passed.
---
Outside diff comments:
In `@server/api/chat/message-agents.ts`:
- Around line 3221-3270: The failed-delegation branch returns ToolResponse.error
without emitting a closing reasoning event, leaving the delegated block open;
before returning when toolOutput.error is truthy (inside the run_public_agent
flow where delegationRunId and delegatedAgentName are set and executeCustomAgent
was called), call and await context.runtime?.emitReasoning with
ReasoningSteps.agentCompleted(...) (or an equivalent failure payload) enriched
with agent: delegatedAgentName, delegationRunId, parentAgent: "Main" and the
error details, then return the ToolResponse.error; use the same emitReasoning
pattern used in the success path (mirror the reasoningEmitter shape) so failed
delegations always emit a terminal agentCompleted event.
In `@server/api/search.ts`:
- Around line 527-550: The code is passing an unsupported
includeKnowledgeBaseInSearch option to searchVespa (VespaQueryConfig); remove
both includeKnowledgeBaseInSearch: isSebi entries and instead set excludedApps
based on isSebi when calling searchVespa: if isSebi is true leave excludedApps
unset (or omit Apps.KnowledgeBase), otherwise include Apps.KnowledgeBase in
excludedApps; update both searchVespa call sites (the one that runs with
Promise.all and the else branch) to use excludedApps accordingly while keeping
other params (alpha, limit, requestDebug, offset, timestampRange, rankProfile)
and leaving updateUserQueryHistory, groupCount, results logic unchanged.
---
Duplicate comments:
In `@server/api/chat/message-agents.ts`:
- Around line 4062-4068: The code only merges non-image fileIds from history
(via collectReferencedFileIdsUntilCompaction and expandSheetIds) so historical
imageAttachmentFileIds and threadIds are still omitted when rebuilding
agentContext; update the logic to also extract and merge historical
imageAttachmentFileIds and threadIds from previousConversationHistory (either by
extending collectReferencedFileIdsUntilCompaction to return those arrays or
adding small helpers like collectReferencedImageAttachmentIdsUntilCompaction and
collectReferencedThreadIdsUntilCompaction), then include those IDs when
constructing allReferencedFileIds and when populating agentContext (ensure
imageAttachmentFileIds and threadIds are combined with the current request’s
values).
---
Nitpick comments:
In `@frontend/src/components/Autocomplete.tsx`:
- Around line 139-146: The code incorrectly compares field names against
SearchResult.tsx and uses a redundant type assertion; inside Autocomplete.tsx
where you check result.type === "kb_items" (and cast to KbFileAutocomplete),
remove the invalid comparison to SearchResult and the unnecessary "as
KbFileAutocomplete" cast—rely on the AutocompleteSchema discriminated union
narrowing and directly use kbResult.title (or result.title) to render the title,
ensuring any import/usage of KbFileAutocomplete matches the AutocompleteSchema
definition.
In `@frontend/src/components/SearchResult.tsx`:
- Around line 437-443: The kb_items branch is using inline type assertions
(e.g., (result as { fileName?: string }), (result as { url?: string }), (result
as { updatedAt?: number })) because TypeScript isn't narrowing result when
result.type === "kb_items"; fix the SearchResultDiscriminatedUnion so it
includes a proper kb_items variant with the expected properties (fileName, url,
updatedAt, chunk/summary shape) or add a named interface (e.g., KbItemResult)
and include it in the union, then remove the inline casts in the
SearchResult.tsx branch (where title and chunkText are derived) so result is
directly typed and narrows correctly when result.type === "kb_items".
In `@frontend/src/lib/common.tsx`:
- Around line 140-155: Two consecutive branches in the icon-rendering logic both
check Apps.KnowledgeBase and return identical BookOpen JSX (the conditions using
SystemEntity.SystemInfo and entity === "file"); consolidate them by combining
the conditions into a single branch (e.g., check app === Apps.KnowledgeBase &&
(entity === SystemEntity.SystemInfo || entity === "file")) and keep the single
BookOpen return to eliminate duplicate JSX while preserving classNameVal, size,
and stroke props.
In `@server/vespa/schemas/ticket.sd`:
- Around line 613-623: The lexical branch
(nativeRank(subject)/nativeRank(description)) is scaled unequally to
vector_score making query(alpha) non-intuitive; compute a normalized
lexical_score by combining nativeRank(subject) and nativeRank(description) into
a single weighted sum, then renormalize it back into [0,1] (e.g., divide by the
sum of weights or apply min-max scaling) and replace the raw weighted expression
in both first-phase and global-phase with that normalized lexical_score so that
(query(alpha) * vector_score) + ((1 - query(alpha)) * lexical_score) yields a
balanced, consistent alpha blend (also multiply the whole blended expression by
doc_recency in global-phase as before).
- Around line 612-615: The rank-profile title_boosted_hybrid currently inherits
initial which uses the tensor input name query(e), while the existing hybrid
profile uses query(q); update the title_boosted_hybrid profile so its query
tensor name matches expected callers — either change its declaration to include
query(q) in addition to or instead of query(e), or document/rename it and update
any callers to pass query(e) if you intend to keep initial's naming; reference
the rank-profile title_boosted_hybrid, the inherited profile initial, and the
hybrid profile (which uses query(q)) when making the change so the tensor input
names are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31452d34-6211-4634-9554-3ec0e0898c9d
📒 Files selected for processing (9)
frontend/src/components/Autocomplete.tsxfrontend/src/components/ChatBox.tsxfrontend/src/components/SearchResult.tsxfrontend/src/lib/common.tsxserver/api/chat/message-agents.tsserver/api/search.tsserver/config.tsserver/shared/types.tsserver/vespa/schemas/ticket.sd
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
frontend/src/components/ChatBox.tsx (1)
367-368:⚠️ Potential issue | 🟠 MajorKeep an escape hatch when agentic-default is enabled.
isAgenticByDefaultonly provides the default prop value here; parents can still passfalse. In that state these guards hide both the MCP picker and the Agent toggle, so the user can get stuck in non-agent mode with no way to turn it back on. Use an effective mode (isAgenticByDefault || isAgenticMode) for both rendering and interactivity.Also applies to: 385-385, 3145-3147, 3750-3751
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ChatBox.tsx` around lines 367 - 368, The guards use isAgenticByDefault as a prop default which can hide both the MCP picker and the Agent toggle and leave the UI stuck; change rendering and interaction checks to use an effective mode (e.g., const effectiveAgentic = isAgenticByDefault || isAgenticMode) so both visibility and interactivity for the MCP picker and Agent toggle use effectiveAgentic instead of raw isAgenticByDefault; update all occurrences referenced (around symbols isAgenticByDefault, isAgenticMode and the MCP/Agent toggle render blocks noted at the duplicated locations) so parents can still flip agentic mode at runtime.
🧹 Nitpick comments (1)
server/api/search.ts (1)
610-612: Consider passingVespaSearchResponseToSearchResultfor consistency.The main
SearchApi(line 567-571) and agent search (line 453-458) passSearchSlackChannels(line 637-639) also omits it, verify whether KB search results require user-specific transformations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/search.ts` around lines 610 - 612, The call to VespaSearchResponseToSearchResult here omits the user email argument used elsewhere; update the call in the SearchApi flow to pass the current user's email as the third parameter (matching the other calls that pass email) so user-specific transformations are applied consistently, and audit the similar call in SearchSlackChannels (which also omits email) to decide whether it likewise should pass the email or intentionally omit it; modify VespaSearchResponseToSearchResult invocations (and callers passing chunkDocument) to include the email when user-specific behavior is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ChatBox.tsx`:
- Around line 1565-1588: The totalCount calculation currently sums backend
counts (fetchedTotalCount = (data.count ?? 0) + (kbData.count ?? 0)) causing
"Load More" to remain when items are duplicated; change this to use the deduped
union used for globalResults by deriving the total from the dedup map/array
instead (e.g., use byDocId.size or results.length) and call setTotalCount with
that value so totalCount reflects unique results after the docId deduplication
used by mainResults/kbResults/byDocId.
In `@frontend/src/routes/_authenticated/search.tsx`:
- Around line 423-426: When appending new paginated results in the search
handler, deduplicate items in merged before calling setResults so documents with
the same docId from /search and /search/knowledge-base aren't rendered twice;
specifically, in the branch that checks newOffset > 0, compute a filteredMerged
by removing any item whose docId already exists in prevResults (use
setResults(prev => { const existingIds = new Set(prev.map(r => r.docId)); return
[...prev, ...merged.filter(m => !existingIds.has(m.docId))]; })) and then call
setResults with that filtered array, while keeping the else branch to replace
results with merged unchanged.
- Around line 455-468: The sidebar total is using the raw endpoint counts
(data.count + kbData.count) when sumGroupCounts(mergedGroups) is zero, which
overcounts duplicate docs; instead compute a deduped total from the merged
result set (dedupe by docId) and setSearchMeta({ totalCount: dedupedTotal }) —
i.e., after mergeGroupCounts(…) and wherever the search results are merged,
produce a unique-docId count (call it dedupedTotal or uniqueCount) and use that
as the fallback/current total instead of (data.count ?? 0) + (kbData.count ??
0); update references to mergeGroupCounts, sumGroupCounts, mergedGroups and
setSearchMeta to use this deduped total.
In `@server/api/search.ts`:
- Around line 598-608: searchVespaKnowledgeBase is being called without the
computed timestampRange, causing search results to ignore the same time filter
used by groupVespaSearchKnowledgeBase; update the call to
searchVespaKnowledgeBase(decodedQuery, email, { limit: page, offset,
rankProfile: SearchModes.NativeRank, timestampRange }) so the options object
includes timestampRange (the value computed from lastUpdated) to ensure both
searchVespaKnowledgeBase and groupVespaSearchKnowledgeBase apply the same time
filtering.
In `@server/search/vespa.ts`:
- Around line 158-161: The code exports two methods that don't exist on
VespaService (searchVespaKnowledgeBase and groupVespaSearchKnowledgeBase),
causing module load failures; either upgrade the `@xyne/vespa-ts` library to a
version that exposes those methods or remove the broken exports and update
callsites. If upgrading is not possible, delete the exports
searchVespaKnowledgeBase and groupVespaSearchKnowledgeBase from the vespa.ts
module and change the usages in server/api/search.ts to call the existing
methods (e.g., searchVespa or groupVespaSearch) or add a runtime guard that
falls back to the non-KB methods; ensure all references to those two symbols are
removed or redirected so module initialization no longer tries to bind missing
methods.
---
Duplicate comments:
In `@frontend/src/components/ChatBox.tsx`:
- Around line 367-368: The guards use isAgenticByDefault as a prop default which
can hide both the MCP picker and the Agent toggle and leave the UI stuck; change
rendering and interaction checks to use an effective mode (e.g., const
effectiveAgentic = isAgenticByDefault || isAgenticMode) so both visibility and
interactivity for the MCP picker and Agent toggle use effectiveAgentic instead
of raw isAgenticByDefault; update all occurrences referenced (around symbols
isAgenticByDefault, isAgenticMode and the MCP/Agent toggle render blocks noted
at the duplicated locations) so parents can still flip agentic mode at runtime.
---
Nitpick comments:
In `@server/api/search.ts`:
- Around line 610-612: The call to VespaSearchResponseToSearchResult here omits
the user email argument used elsewhere; update the call in the SearchApi flow to
pass the current user's email as the third parameter (matching the other calls
that pass email) so user-specific transformations are applied consistently, and
audit the similar call in SearchSlackChannels (which also omits email) to decide
whether it likewise should pass the email or intentionally omit it; modify
VespaSearchResponseToSearchResult invocations (and callers passing
chunkDocument) to include the email when user-specific behavior is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b99b3694-2006-424a-a16b-b48e2e3be4f3
📒 Files selected for processing (5)
frontend/src/components/ChatBox.tsxfrontend/src/routes/_authenticated/search.tsxserver/api/search.tsserver/search/vespa.tsserver/server.ts
Aayushjshah
approved these changes
Mar 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Testing
Additional Notes
Summary by CodeRabbit
Release Notes
New Features
Improvements