Vibe/threadrefactor | Add vector RAG and document outline rendering #600
Conversation
…tant” can be scope-filtered)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/pro-web/components/routes/thread/thread-popup.tsx (1)
178-186: Unsafe property access on untyped metadata.Line 181 accesses
currentThread?.metadata?.documents?.lengthbutmetadatais typed asunknown. This works at runtime due to optional chaining but bypasses TypeScript's type checking.Consider using the
getThreadMarkdownFromMetadatautility's pattern or the typed interface suggested earlier:- !currentThread?.metadata?.documents?.length || !messageTitle + !(threadMeta as ThreadMetadata | null)?.documents?.length || !messageTitleapps/pro-web/components/routes/chat/chat-options.tsx (1)
61-76: Fix memo deps so share URLs update with subthread changes.
useMemonow depends onisSubThread/pair, but the dependency array is still[thread]. Ifpairchanges whilethreadstays stable, the URL can be stale and share the wrong subthread. Consider including the new inputs (or dropping the memo).✅ Suggested fix
- // biome-ignore lint/correctness/useExhaustiveDependencies: <explanation> - const url = useMemo( + const url = useMemo( () => !thread.slug ? '/' : urlBuilders.profilesThreadUrl({ type: 'user', threadSlug: thread.slug, threadQuestionSlug: isSubThread ? (pair as any).userMessage.slug : undefined, category: thread.chatbot.department?.name || 'general', chatbot: toSlug(thread.chatbot.name), usernameSlug: thread?.user?.slug, domain: canonicalDomain, }), - [thread], + [thread, canonicalDomain, isSubThread, pair], )apps/pro-web/lib/metadata.ts (1)
90-102: Verify role casing convention to prevent metadata defaults.The role comparisons at lines 94 and 101 use uppercase literals (
'USER','ASSISTANT') without case normalization. If roles are stored in lowercase or mixed case in the database (similar to how the codebase normalizes other data), these checks will fail silently and metadata will default to "not found."Confirm the actual casing convention for the
rolefield in the message schema. If roles are lowercase, either normalize the comparisons using.toUpperCase()or ensure consistent casing at the storage layer.apps/pro-web/lib/utils.ts (1)
88-97: Guard role before lowercasing.
message.role.toLowerCase()will throw if a role is ever missing/undefined. Consider optional chaining to keep this resilient.🛠️ Suggested fix
- if (message.role.toLowerCase() === 'user') { + if (message.role?.toLowerCase() === 'user') {
🤖 Fix all issues with AI agents
In `@apps/pro-web/components/routes/chat/chat-list/message-pair-accordion.tsx`:
- Around line 276-355: The URL slug logic in the accordion open/close block
(using isOpen, activeThreadQuestionSlug, userQuestionSlug and navigateTo in
message-pair-accordion.tsx) should guard on slug equality: when opening (isOpen
&& !activeThreadQuestionSlug) only call navigateTo to set the question slug if
activeThreadQuestionSlug !== userQuestionSlug (i.e., the slug actually changed),
and when closing (!isOpen && activeThreadQuestionSlug) only navigate to remove
the slug if activeThreadQuestionSlug === userQuestionSlug (so closing a
non-active accordion doesn't clear another message's slug); update both the
'profiles' and 'org'/'pro' branches to apply these conditions before calling
navigateTo.
In `@apps/pro-web/components/routes/thread/thread-component.tsx`:
- Around line 58-61: The code uses m.role.toLowerCase() which will throw if
m.role is null/undefined; update the logic that computes threadTitle
(referencing threadTitle and the array filter callback using m.role) to safely
normalize role by using optional chaining or a fallback string (e.g., treat
missing role as empty string) before calling toLowerCase(), and keep the rest of
the expression unchanged so you still filter for 'user' roles and access message
content.
In `@apps/pro-web/components/routes/thread/user-thread-panel.tsx`:
- Around line 563-566: The code building the optimistic thread's messages uses
thread.messages.filter((msg) => msg.role.toLowerCase() === 'user') which will
throw if msg.role is null/undefined; update the predicate to guard/normalize
role before calling toLowerCase (e.g., check typeof msg.role === 'string' or use
a safe normalization like (msg.role || '').toLowerCase()) so the filter for user
messages in the optimistic thread creation does not crash.
In `@apps/pro-web/lib/hooks/use-mb-chat.tsx`:
- Around line 1119-1169: The derived scopeTitleSnapshot is computed but never
used—replace uses of scopeTitleSnapshotForMeta with the computed
scopeTitleSnapshot when building mb_op_meta and when setting opMetaRef so the
fallback title is propagated; update the enrichedChatOptions construction (and
the parallel body in the else branch) and any opMetaRef assignment in
use-mb-chat.tsx to reference scopeTitleSnapshot (or a single unified variable
like scopeTitleSnapshotFinal) instead of scopeTitleSnapshotForMeta so both
caller-provided and derived values are consistently included in mb_op_meta and
opMetaRef.
In `@apps/pro-web/lib/hooks/use-workspace-chat.tsx`:
- Around line 946-953: The scoped history fetch call to
selectScopeConversationWindow is missing docNodeId so it won't match the new
storage shape; update the call site where you pass { threadId, scopeType,
scopeId, jwt } to include docNodeId (use the local docNodeId variable) so the
function can match using docNodeId + bare scopeId, and apply the same change to
the other identical call site in this file where selectScopeConversationWindow
is invoked (the one that mirrors the heading-thread branch).
♻️ Duplicate comments (4)
apps/pro-web/components/routes/browse/browse-list-item.tsx (2)
19-76: Duplicate initialUrl effects still present.This matches the previously reported duplicate-effect issue (two effects both writing
initialUrl). Please consolidate to a single effect.
146-148: Visibility comment still conflicts with behavior.Returning
nullunmounts the component; the comment says it stays mounted. This matches the previously flagged mismatch.apps/pro-web/services/hasura/hasura.service.ts (1)
792-798: Retry failure is swallowed and falls through to a generic error.If the retry fails, the code logs
retryErrand then throws the generic error for the original attempt, which obscures the real failure cause.apps/pro-web/lib/hooks/use-workspace-chat.tsx (1)
974-981: Avoid thescopedHistory as anycast.This bypasses the expected
MessageDBshape and hides real mismatches.
🧹 Nitpick comments (11)
apps/pro-web/components/routes/thread/thread-popup.tsx (2)
124-130: Consider creating a typed interface for thread metadata.The
biome-ignorecomments suppress linting foranycasts. While the runtime checks inpickStringprovide safety, defining a partial interface for expected metadata fields would improve type safety and IDE support.♻️ Suggested type definition
interface ThreadMetadata { docNodeId?: string doc_node_id?: string documentMarkdown?: string markdown?: string docMarkdown?: string content?: string documents?: Array<{ content?: string markdown?: string docMarkdown?: string }> }Then use
const threadMeta = (currentThread?.metadata as ThreadMetadata | null) ?? null
151-163: Type safety concerns withroleproperty access.Multiple
anycasts are used to access.roleon messages. IfMessagefrommb-genqlhas aroleproperty, prefer a type guard. If not, consider a union type for the messages array.♻️ Suggested type guard approach
function hasRole(m: AiMessage | Message): m is AiMessage | (Message & { role: string }) { return 'role' in m && typeof (m as any).role === 'string' } // Usage: if (!message || !hasRole(message) || message.role.toLowerCase() !== 'assistant') { // ... }#!/bin/bash # Check if Message type from mb-genql includes a role property ast-grep --pattern 'export type Message = { $$$ }' # Also check the schema for message table structure rg -n "role" packages/mb-genql/src/ --type ts -A2 -B2 | head -50apps/pro-web/components/routes/chat/chat-message.tsx (1)
176-180: Avoidanycast formessageto preserve type safety.Passing
message as anyhides mismatches; better to alignChatMessageActionsprop types with the actual message union and passmessagedirectly. As per coding guidelines, avoidany.♻️ Proposed change (plus align ChatMessageActions prop types)
- message={message as any} + message={message}apps/pro-web/components/routes/chat/chat-message-renderer.tsx (1)
50-67: Avoidanycasts in render branches.The explicit
message as anymasks contract drift; prefer updating the target component prop types (or a shared message union) and passmessagedirectly. As per coding guidelines, avoidany.♻️ Proposed change (plus align prop types)
- message={message as any} + message={message} ... - message={message as any} + message={message}apps/pro-web/lib/threads.ts (1)
11-14: ReplaceanyinMessagePairwith a minimal message shape.Using
anyhere makes downstream consumers brittle and hides mistakes. Consider a minimal shared interface for the fields you actually use.♻️ Suggested refactor
+interface MessageLike { + role: string + content: string + id?: string + messageId?: string +} + export interface MessagePair { - userMessage: any - chatGptMessage: any[] + userMessage: MessageLike + chatGptMessage: MessageLike[] }apps/pro-web/components/routes/chat/chat-message-actions.tsx (2)
36-38: Define a typed message shape and normalize access once.The repeated
(message as any)casts and unguarded content usage risk propagatingundefinedinto clipboard/documents. A small normalization helper reduces casts and lets you add a single content guard. It also makes it easy to normalize role checks when locating the preceding user message.♻️ Suggested refactor
+interface MessageLike { + id?: string + messageId?: string + content?: string + role?: string +} + export function ChatMessageActions({ message, @@ }: ChatMessageActionsProps) { + const normalized = message as MessageLike + const normalizedMessageId = normalized.messageId ?? normalized.id ?? '' + const normalizedContent = normalized.content ?? '' + const normalizedRole = normalized.role?.toLowerCase() + - const messageId = (message as any)?.messageId || (message as any).id + const messageId = normalizedMessageId @@ - copyToClipboard((message as any).content) + if (!normalizedContent) return + copyToClipboard(normalizedContent) @@ - content: (message as any).content, + content: normalizedContent, @@ - content: (message as any).content, + content: normalizedContent, @@ - const normalizedMessageContent = ((message as any).content || '').trim() + const normalizedMessageContent = normalizedContent.trim() @@ - if (currentContent.includes((message as any).content)) { + if (normalizedContent && currentContent.includes(normalizedContent)) { @@ -${(message as any).content}` - : (message as any).content +${normalizedContent}` + : normalizedContent @@ - const messageTitle = toSlug((message as any).content) + const messageTitle = toSlug(normalizedContent)Also applies to: 78-113, 212-219, 253-295, 322-323
341-345: Avoid inline arrow handlers in JSX.Extract a stable handler (e.g.,
useCallback) so the prop reference doesn’t churn on every render.♻️ Suggested refactor
+ const handleAddToActiveDocument = useCallback(() => { + ;(onAddToActiveDocument ?? onAddToActiveDocumentHandler)(messageId) + }, [onAddToActiveDocument, onAddToActiveDocumentHandler, messageId]) @@ - onClick={() => - (onAddToActiveDocument ?? onAddToActiveDocumentHandler)( - (message as any)?.messageId || (message as any).id, - ) - } + onClick={handleAddToActiveDocument}apps/pro-web/lib/helpers/ai-helpers.ts (1)
320-334: Tightenanyin reasoning helpers.These helpers only need a small subset of fields;
anyremoves safety and IntelliSense. Consider a narrow interface forparts/thinking.♻️ Suggested refactor
+interface ReasoningPart { + type?: string + reasoning?: string +} +interface ReasoningMessageLike { + parts?: ReasoningPart[] + thinking?: string | null +} - -export function hasReasoning(message: any): boolean { +export function hasReasoning(message: ReasoningMessageLike): boolean { return Boolean( - message.parts?.some((part: any) => part.type === 'reasoning') || + message.parts?.some((part) => part.type === 'reasoning') || message?.thinking, ) } @@ -export function extractReasoningContent( - message: any, -): string | null | undefined { +export function extractReasoningContent( + message: ReasoningMessageLike, +): string | null | undefined { if (message.parts?.length) { const reasoningPart = message.parts.find( - (part: any) => part.type === 'reasoning', + (part) => part.type === 'reasoning', )apps/pro-web/services/hasura/hasura.service.ts (1)
726-740: AvoidanyinsaveNewMessageparam handling.The types already allow
docNodeId/doc_node_id, so theanycasts are avoidable and reduce safety.
As per coding guidelines, avoidany.♻️ Suggested refactor
export async function saveNewMessage( params: Partial<SaveNewMessageParams> & { doc_node_id?: string | null }, ) { - const { jwt, docNodeId, doc_node_id, ...object } = params as any + const { jwt, docNodeId, doc_node_id, ...object } = params const client = getHasuraClient({ jwt }) // During migration, callers may pass either camelCase or snake_case. // Only send the camelCase field to Hasura (GraphQL naming is camelCase here). - const canonicalDocNodeId: string | null = (docNodeId ?? - doc_node_id ?? - null) as any + const canonicalDocNodeId: string | null = docNodeId ?? doc_node_id ?? nullapps/pro-web/components/routes/chat/chat-list/message-pair-accordion.tsx (2)
415-415: Avoid inlineonTogglehandler for a stable reference.This keeps handler identity stable and aligns with the no‑inline‑handlers guideline.
♻️ Suggested refactor
+ const isFirstQuestion = index === 0 + const handleToggle = useCallback( + (open: boolean) => toggleThreadQuestionUrl(open, isFirstQuestion), + [toggleThreadQuestionUrl, isFirstQuestion], + ) @@ - onToggle={(open) => toggleThreadQuestionUrl(open, index === 0)} + onToggle={handleToggle}As per coding guidelines, prefer stable function references for JSX handlers.
456-516: Replaceanycasts and stabilize message keys/ids.The
anyaccesses bypass type safety andFragmentkeys can end up undefined. Consider a lightweight message‑like type plusgetMessageStableIdfor keys/ids. IfMessageRenderertypes are too narrow, widen them to the actual message shape instead of casting toany.♻️ Suggested refactor
type ThreadLike = { slug?: string | null chatbot?: { name?: string | null } | null thread?: { user?: { username?: string | null } | null } | null } +type ChatMessageLike = { + id?: string | null + slug?: string | null + threadId?: string | null + content?: string | null +} + +const asChatMessageLike = (v: unknown): ChatMessageLike | undefined => + isObject(v) ? (v as ChatMessageLike) : undefined @@ - {currentThread && shouldShowUserMessage && ( + {currentThread && shouldShowUserMessage && ( <ChatOptions - threadId={(pair.userMessage as any).threadId} + threadId={asChatMessageLike(pair.userMessage)?.threadId} thread={currentThread} isBrowse={false} pair={pair} /> )} @@ - {(pair.chatGptMessage[0] as any)?.content ? ( + {asChatMessageLike(pair.chatGptMessage[0])?.content ? ( <div className="flex-1 px-1 space-y-2 overflow-hidden text-left"> <ShortMessage - content={(pair.chatGptMessage[0] as any)?.content} + content={asChatMessageLike(pair.chatGptMessage[0])?.content} /> </div> ) : ( '' )} @@ - ? pair.chatGptMessage.map((message) => ( - <Fragment key={(message as any).id}> + ? pair.chatGptMessage.map((message, messageIndex) => { + const messageKey = getMessageStableId( + asChatMessageLike(message), + `${accordionId}-response-${messageIndex}`, + ) + return ( + <Fragment key={messageKey}> {!isThread && ( <AttachmentCards userAttachments={userAttachments} isAccordionFocused={isAccordionFocused} /> )} <MessageRenderer - id={(message as any).slug} - message={message as any} + id={asChatMessageLike(message)?.slug ?? messageKey} + message={message} sendMessageFromResponse={props.sendMessageFn} onConvertToWorkspaceDocument={onConvertToWorkspaceDocument} /> </Fragment> - )) + ) + }) : ''}As per coding guidelines, avoid
anyand prefer explicit message shapes.
| // OPEN: go to question URL (if not already) | ||
| if (isOpen && !activeThreadQuestionSlug) { | ||
| if (route.kind === 'profiles') { | ||
| const usernameSlug = | ||
| route.usernameSlug || | ||
| firstParam((params as Record<string, unknown>)?.userSlug) || | ||
| '' | ||
| if (!usernameSlug) return | ||
|
|
||
| navigateTo({ | ||
| urlType: 'profilesThreadQuestionUrl', | ||
| shallow: true, | ||
| navigationParams: { | ||
| type: route.type, | ||
| usernameSlug, | ||
| category, | ||
| domain, | ||
| chatbot, | ||
| threadSlug, | ||
| threadQuestionSlug: userQuestionSlug, | ||
| }, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if (!threadQuestionSlug && isOpen) { | ||
| navigateTo({ | ||
| urlType: isProfile | ||
| ? 'profilesThreadQuestionUrl' | ||
| : 'threadQuestionUrl', | ||
| shallow: true, | ||
| navigationParams: isProfile | ||
| ? { | ||
| type: 'user', | ||
| ...navigationParts, | ||
| } | ||
| : { | ||
| type: isOrg ? 'org' : isBot ? 'bot' : 'pro', | ||
| ...navigationParts, | ||
| }, | ||
| }) | ||
| if (route.kind === 'org' || route.kind === 'pro') { | ||
| navigateTo({ | ||
| urlType: 'threadQuestionUrl', | ||
| shallow: true, | ||
| navigationParams: { | ||
| type: route.type, | ||
| category, | ||
| domain, | ||
| chatbot, | ||
| threadSlug, | ||
| threadQuestionSlug: userQuestionSlug, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| return | ||
| } | ||
| if (threadQuestionSlug && !isOpen) { | ||
| navigateTo({ | ||
| urlType: isProfile ? 'profilesThreadUrl' : 'threadUrl', | ||
| shallow: true, | ||
| navigationParams: isProfile | ||
| ? { | ||
| type: 'user', | ||
| ...navigationParts, | ||
| } | ||
| : { | ||
| type: isOrg ? 'org' : isBot ? 'bot' : 'pro', | ||
| ...navigationParts, | ||
| }, | ||
| }) | ||
|
|
||
| // CLOSE: go back to thread URL (remove question slug) | ||
| if (!isOpen && activeThreadQuestionSlug) { | ||
| if (route.kind === 'profiles') { | ||
| const usernameSlug = | ||
| route.usernameSlug || | ||
| firstParam((params as Record<string, unknown>)?.userSlug) || | ||
| '' | ||
| if (!usernameSlug) return | ||
|
|
||
| navigateTo({ | ||
| urlType: 'profilesThreadUrl', | ||
| shallow: true, | ||
| navigationParams: { | ||
| type: route.type, | ||
| usernameSlug, | ||
| category, | ||
| domain, | ||
| chatbot, | ||
| threadSlug, | ||
| }, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if (route.kind === 'org' || route.kind === 'pro') { | ||
| navigateTo({ | ||
| urlType: 'threadUrl', | ||
| shallow: true, | ||
| navigationParams: { | ||
| type: route.type, | ||
| category, | ||
| domain, | ||
| chatbot, | ||
| threadSlug, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
URL slug can get out of sync when switching accordions.
If a slug is already present, opening a different accordion won’t update the URL, and closing a non‑active accordion can clear the slug for another message. Guard on slug equality and update when it changes.
🐛 Proposed fix
- // OPEN: go to question URL (if not already)
- if (isOpen && !activeThreadQuestionSlug) {
+ const isActiveSlug = activeThreadQuestionSlug === userQuestionSlug
+
+ // OPEN: go to question URL (if not already)
+ if (isOpen && (!activeThreadQuestionSlug || !isActiveSlug)) {
if (route.kind === 'profiles') {
const usernameSlug =
route.usernameSlug ||
firstParam((params as Record<string, unknown>)?.userSlug) ||
''
if (!usernameSlug) return
@@
- // CLOSE: go back to thread URL (remove question slug)
- if (!isOpen && activeThreadQuestionSlug) {
+ // CLOSE: go back to thread URL (remove question slug)
+ if (!isOpen && isActiveSlug) {
if (route.kind === 'profiles') {
const usernameSlug =
route.usernameSlug ||
firstParam((params as Record<string, unknown>)?.userSlug) ||
''
if (!usernameSlug) return🤖 Prompt for AI Agents
In `@apps/pro-web/components/routes/chat/chat-list/message-pair-accordion.tsx`
around lines 276 - 355, The URL slug logic in the accordion open/close block
(using isOpen, activeThreadQuestionSlug, userQuestionSlug and navigateTo in
message-pair-accordion.tsx) should guard on slug equality: when opening (isOpen
&& !activeThreadQuestionSlug) only call navigateTo to set the question slug if
activeThreadQuestionSlug !== userQuestionSlug (i.e., the slug actually changed),
and when closing (!isOpen && activeThreadQuestionSlug) only navigate to remove
the slug if activeThreadQuestionSlug === userQuestionSlug (so closing a
non-active accordion doesn't clear another message's slug); update both the
'profiles' and 'org'/'pro' branches to apply these conditions before calling
navigateTo.
| const threadId = thread.threadId | ||
| const threadTitle = ( | ||
| thread.thread ? thread.thread.messages : thread.messages | ||
| ).filter((m) => m.role === 'user')[0]?.content | ||
| ).filter((m) => m.role.toLowerCase() === 'user')[0]?.content |
There was a problem hiding this comment.
Guard role normalization to avoid runtime errors.
If m.role can be null/undefined, toLowerCase() will throw. Consider optional chaining or a fallback.
🔧 Safer normalization
- ).filter((m) => m.role.toLowerCase() === 'user')[0]?.content
+ ).filter((m) => m.role?.toLowerCase() === 'user')[0]?.content📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const threadId = thread.threadId | |
| const threadTitle = ( | |
| thread.thread ? thread.thread.messages : thread.messages | |
| ).filter((m) => m.role === 'user')[0]?.content | |
| ).filter((m) => m.role.toLowerCase() === 'user')[0]?.content | |
| const threadId = thread.threadId | |
| const threadTitle = ( | |
| thread.thread ? thread.thread.messages : thread.messages | |
| ).filter((m) => m.role?.toLowerCase() === 'user')[0]?.content |
🤖 Prompt for AI Agents
In `@apps/pro-web/components/routes/thread/thread-component.tsx` around lines 58 -
61, The code uses m.role.toLowerCase() which will throw if m.role is
null/undefined; update the logic that computes threadTitle (referencing
threadTitle and the array filter callback using m.role) to safely normalize role
by using optional chaining or a fallback string (e.g., treat missing role as
empty string) before calling toLowerCase(), and keep the rest of the expression
unchanged so you still filter for 'user' roles and access message content.
| // ? These messages doesn't go to the allMessages directly, allMessages filters the true content. | ||
| messages: [thread.messages.filter((msg) => msg.role === 'user')[0]], | ||
| messages: [ | ||
| thread.messages.filter((msg) => msg.role.toLowerCase() === 'user')[0], | ||
| ], |
There was a problem hiding this comment.
Guard msg.role before toLowerCase() to avoid crashes.
If role is nullable, this can throw while building the optimistic thread.
🔧 Safer normalization
- thread.messages.filter((msg) => msg.role.toLowerCase() === 'user')[0],
+ thread.messages.filter((msg) => msg.role?.toLowerCase() === 'user')[0],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ? These messages doesn't go to the allMessages directly, allMessages filters the true content. | |
| messages: [thread.messages.filter((msg) => msg.role === 'user')[0]], | |
| messages: [ | |
| thread.messages.filter((msg) => msg.role.toLowerCase() === 'user')[0], | |
| ], | |
| // ? These messages doesn't go to the allMessages directly, allMessages filters the true content. | |
| messages: [ | |
| thread.messages.filter((msg) => msg.role?.toLowerCase() === 'user')[0], | |
| ], |
🤖 Prompt for AI Agents
In `@apps/pro-web/components/routes/thread/user-thread-panel.tsx` around lines 563
- 566, The code building the optimistic thread's messages uses
thread.messages.filter((msg) => msg.role.toLowerCase() === 'user') which will
throw if msg.role is null/undefined; update the predicate to guard/normalize
role before calling toLowerCase (e.g., check typeof msg.role === 'string' or use
a safe normalization like (msg.role || '').toLowerCase()) so the filter for user
messages in the optimistic thread creation does not crash.
| let scopeTitleSnapshot: string | null = scope?.scopeTitleSnapshot ?? null | ||
|
|
||
| // If caller didn't provide it, derive it safely (only for heading threads) | ||
| if ( | ||
| !scopeTitleSnapshot && | ||
| scopeTypeForMeta === 'heading_thread' && | ||
| scopeIdForMeta | ||
| ) { | ||
| const effectiveSections = scope?.sections?.length | ||
| ? scope.sections | ||
| : scope?.fullMarkdown | ||
| ? parseMarkdownSections(scope.fullMarkdown) | ||
| : [] | ||
|
|
||
| const match = effectiveSections.find((s) => s.id === scopeIdForMeta) | ||
| scopeTitleSnapshot = match?.title?.trim() || null | ||
| } | ||
|
|
||
| const enrichedChatOptions: ChatRequestOptions | undefined = | ||
| chatRequestOptions | ||
| ? ({ | ||
| ...chatRequestOptions, | ||
| body: { | ||
| // biome-ignore lint/suspicious/noExplicitAny: passthrough metadata | ||
| ...((chatRequestOptions as any).body || {}), | ||
| mb_op_meta: { | ||
| scope_type: scopeTypeForMeta, | ||
| scope_id: scopeIdForMeta, | ||
| doc_node_id: docNodeIdForMeta, | ||
| scope_title_snapshot: scopeTitleSnapshotForMeta, | ||
| doc_context_included: docContextIncluded, | ||
| doc_context_policy: docContextIncluded | ||
| ? DOC_CONTEXT_POLICY | ||
| : null, | ||
| heading_content_version: headingContentVersion ?? null, | ||
| }, | ||
| }, | ||
| } as ChatRequestOptions) | ||
| : ({ | ||
| body: { | ||
| mb_op_meta: { | ||
| scope_type: scopeTypeForMeta, | ||
| scope_id: scopeIdForMeta, | ||
| doc_node_id: docNodeIdForMeta, | ||
| scope_title_snapshot: scopeTitleSnapshotForMeta, | ||
| doc_context_included: docContextIncluded, | ||
| doc_context_policy: docContextIncluded | ||
| ? DOC_CONTEXT_POLICY | ||
| : null, | ||
| heading_content_version: headingContentVersion ?? null, | ||
| }, |
There was a problem hiding this comment.
Derived scopeTitleSnapshot is never propagated.
You compute scopeTitleSnapshot when the caller doesn’t supply one, but mb_op_meta and opMetaRef still use scopeTitleSnapshotForMeta, so the derived value is discarded.
🐛 Suggested fix
- const scopeTitleSnapshotForMeta: string | null =
- scope?.scopeTitleSnapshot ?? null
+ const scopeTitleSnapshotForMeta: string | null =
+ scope?.scopeTitleSnapshot ?? null
@@
- let scopeTitleSnapshot: string | null = scope?.scopeTitleSnapshot ?? null
+ let scopeTitleSnapshot: string | null = scope?.scopeTitleSnapshot ?? null
@@
- scope_title_snapshot: scopeTitleSnapshotForMeta,
+ scope_title_snapshot: scopeTitleSnapshot,
@@
- scope_title_snapshot: scopeTitleSnapshotForMeta,
+ scope_title_snapshot: scopeTitleSnapshot,
@@
- scopeTitleSnapshot: scopeTitleSnapshotForMeta,
+ scopeTitleSnapshot: scopeTitleSnapshot,Also applies to: 1236-1244
🤖 Prompt for AI Agents
In `@apps/pro-web/lib/hooks/use-mb-chat.tsx` around lines 1119 - 1169, The derived
scopeTitleSnapshot is computed but never used—replace uses of
scopeTitleSnapshotForMeta with the computed scopeTitleSnapshot when building
mb_op_meta and when setting opMetaRef so the fallback title is propagated;
update the enrichedChatOptions construction (and the parallel body in the else
branch) and any opMetaRef assignment in use-mb-chat.tsx to reference
scopeTitleSnapshot (or a single unified variable like scopeTitleSnapshotFinal)
instead of scopeTitleSnapshotForMeta so both caller-provided and derived values
are consistently included in mb_op_meta and opMetaRef.
| try { | ||
| if (threadId && jwt && scopeType === 'heading_thread' && scopeId) { | ||
| const window = await selectScopeConversationWindow({ | ||
| threadId, | ||
| scopeType, | ||
| scopeId, | ||
| jwt, | ||
| }) |
There was a problem hiding this comment.
Scoped history fetch omits docNodeId, so new-scheme rows won’t match.
selectScopeConversationWindow relies on docNodeId to match the new storage shape (docNodeId + bare scopeId). Without it, heading-thread history will only match legacy namespaced rows.
🐛 Suggested fix
if (threadId && jwt && scopeType === 'heading_thread' && scopeId) {
const window = await selectScopeConversationWindow({
threadId,
scopeType,
scopeId,
+ docNodeId,
jwt,
})
@@
{
scopeType,
scopeId: scopeId ?? null,
- docNodeId: docRootId ?? null,
+ docNodeId,
fullMarkdown: currentContent,
sections: operationOriginalSectionsRef.current,
},Also applies to: 1011-1018
🤖 Prompt for AI Agents
In `@apps/pro-web/lib/hooks/use-workspace-chat.tsx` around lines 946 - 953, The
scoped history fetch call to selectScopeConversationWindow is missing docNodeId
so it won't match the new storage shape; update the call site where you pass {
threadId, scopeType, scopeId, jwt } to include docNodeId (use the local
docNodeId variable) so the function can match using docNodeId + bare scopeId,
and apply the same change to the other identical call site in this file where
selectScopeConversationWindow is invoked (the one that mirrors the
heading-thread branch).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/pro-web/lib/hooks/use-mb-chat.tsx`:
- Around line 432-446: The debug console.log inside normalizeToolCall is logging
every tool call; remove the console.log('normalizeToolCall object —> ', {
toolCall }) or wrap it so it only runs when dev mode is enabled (e.g., check
appConfig.features.devMode or similar config flag before logging). Update the
normalizeToolCall function to either delete the log line or gate it behind the
dev-mode condition so production traffic doesn't emit this debug output.
♻️ Duplicate comments (3)
apps/pro-web/lib/hooks/use-mb-chat.tsx (3)
658-673:scopeTitleSnapshotis commented out and not persisted to DB.The
scopeTitleSnapshotfield is commented out at line 672, meaning it won't be persisted with the message metadata. If this is intentional, consider removing the commented line. If it should be included, uncomment it.This relates to the existing review comment about
scopeTitleSnapshotnot being propagated correctly from the computed value.
1118-1168: DerivedscopeTitleSnapshotis never used inmb_op_meta.The code computes a fallback
scopeTitleSnapshotat lines 1118-1134 when the caller doesn't provide one, butmb_op_metaat lines 1143-1153 and 1158-1168 still usesscopeTitleSnapshotForMeta(the original input value). The derived value is discarded.This was flagged in a previous review. The fix should replace
scopeTitleSnapshotForMetawithscopeTitleSnapshotin themb_op_metaobjects.
1235-1251:nextOpMetaalso usesscopeTitleSnapshotForMetainstead of computed value.At line 1241,
nextOpMetaassignsscopeTitleSnapshot: scopeTitleSnapshotForMetainstead of the computedscopeTitleSnapshot, making the derivation logic at lines 1118-1134 ineffective for the operation metadata ref.Part of the same issue flagged in the previous review about
scopeTitleSnapshotpropagation.
🧹 Nitpick comments (2)
apps/pro-web/lib/hooks/use-mb-chat.tsx (2)
116-153: Consider extracting inline types to a shared module.The
UiMode,ActionType,IntentSource,DocContextPolicy, andOperationMetatypes are defined inline within the component. Per the AI summary, similar types exist inapps/pro-web/types/operation-meta.ts. Consider importing from the shared module to ensure consistency and avoid drift.
856-867: Consider gating the debug console.log.Similar to
normalizeToolCall, theconsole.log('Tool call:', toolCall)at line 857 will output in production. Consider gating behindappConfig.features.devModefor consistency with other debug logs in this file.♻️ Proposed fix
onToolCall({ toolCall }: { toolCall: ToolCall<string, unknown> }) { - console.log('Tool call:', toolCall) + if (appConfig.features.devMode) { + console.log('Tool call:', toolCall) + } if (appConfig.features.devMode) {
| function normalizeToolCall(toolCall: ToolCall<string, unknown>): AiToolCall { | ||
| console.log('normalizeToolCall object —> ', { toolCall }) | ||
| const id = toolCall.toolCallId ?? '' | ||
| const name = toolCall.toolName ?? 'unknown_tool' | ||
| const args = toolCall.args ?? {} | ||
|
|
||
| return { | ||
| // Keep original fields too (helps debugging; harmless) | ||
| ...toolCall, | ||
| // Your internal shape | ||
| toolCallId: String(id), | ||
| toolName: name, | ||
| args: safeParseArgs(args), | ||
| } as AiToolCall | ||
| } |
There was a problem hiding this comment.
Remove or gate the debug console.log.
Line 433 contains a console.log('normalizeToolCall object —> ', { toolCall }) that will log on every tool call in production. Consider removing it or gating behind appConfig.features.devMode.
🐛 Proposed fix
function normalizeToolCall(toolCall: ToolCall<string, unknown>): AiToolCall {
- console.log('normalizeToolCall object —> ', { toolCall })
+ if (appConfig.features.devMode) {
+ console.log('normalizeToolCall object —> ', { toolCall })
+ }
const id = toolCall.toolCallId ?? ''🤖 Prompt for AI Agents
In `@apps/pro-web/lib/hooks/use-mb-chat.tsx` around lines 432 - 446, The debug
console.log inside normalizeToolCall is logging every tool call; remove the
console.log('normalizeToolCall object —> ', { toolCall }) or wrap it so it only
runs when dev mode is enabled (e.g., check appConfig.features.devMode or similar
config flag before logging). Update the normalizeToolCall function to either
delete the log line or gate it behind the dev-mode condition so production
traffic doesn't emit this debug output.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/pro-web/lib/hooks/use-mb-chat.tsx (1)
627-706: Persist null scope fields instead of empty strings.
normalizeScopeIdintentionally returns null for doc_root, butopMetaForDbconverts nulls to'', which can break scoped queries/history. Prefer nulls (and allow null in the record type) so doc_root/heading_thread semantics remain intact.🐛 Proposed fix
- const opMetaForDb: Record< - string, - string | boolean | number | undefined - > = { + const opMetaForDb: Record< + string, + string | boolean | number | null | undefined + > = { uiMode: opMetaNow.uiMode, scopeType: opMetaNow.scopeType, - scopeId: opMetaNow.scopeId || '', - docNodeId: opMetaNow.docNodeId || '', + scopeId: opMetaNow.scopeId ?? null, + docNodeId: opMetaNow.docNodeId ?? null, actionType: opMetaNow.actionType, excludeFromQChain: opMetaNow.excludeFromQChain, ragEligible: opMetaNow.ragEligible, mutationAllowed: opMetaNow.mutationAllowed, mutationApplied: opMetaNow.mutationApplied, intentSource: opMetaNow.intentSource, docContextIncluded: opMetaNow.docContextIncluded, - headingContentVersion: opMetaNow.headingContentVersion || '', - docContextPolicy: opMetaNow.docContextPolicy || undefined, - // scopeTitleSnapshot: opMetaNow.scopeTitleSnapshot || '', + headingContentVersion: opMetaNow.headingContentVersion ?? null, + docContextPolicy: opMetaNow.docContextPolicy ?? null, + // scopeTitleSnapshot: opMetaNow.scopeTitleSnapshot ?? null, }To confirm schema/type expectations for nullability, you can scan the repo for scope-related column definitions and types:
#!/bin/bash rg -n "scope_id|doc_node_id|heading_content_version|scope_title_snapshot" -g'*.{sql,ts,tsx,yaml}' rg -n "SaveNewMessageParams" -g'*.ts'
♻️ Duplicate comments (2)
apps/pro-web/lib/hooks/use-mb-chat.tsx (2)
416-446: Gate the normalizeToolCall debug log.
The console.log will fire on every tool call in production; please remove it or guard behind dev mode.🐛 Proposed fix
- console.log('normalizeToolCall object —> ', { toolCall }) + if (appConfig.features.devMode) { + console.log('normalizeToolCall object —> ', { toolCall }) + }
1119-1166: Use the derived scopeTitleSnapshot in mb_op_meta and opMetaRef.
A fallback snapshot is computed but the original value is still used, so the derived title is dropped. Propagate the computed snapshot consistently.🐛 Proposed fix
- const scopeTitleSnapshotForMeta: string | null = - scope?.scopeTitleSnapshot ?? null ... - scope_title_snapshot: scopeTitleSnapshotForMeta, + scope_title_snapshot: scopeTitleSnapshot, ... - scope_title_snapshot: scopeTitleSnapshotForMeta, + scope_title_snapshot: scopeTitleSnapshot, ... - scopeTitleSnapshot: scopeTitleSnapshotForMeta, + scopeTitleSnapshot: scopeTitleSnapshot,Also applies to: 1238-1262
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/pro-web/lib/hooks/use-mb-chat.tsx`:
- Around line 668-686: opMetaForDb is storing empty strings for scopeId,
docNodeId, and headingContentVersion which breaks database _isNull queries;
update the assignment in use-mb-chat where opMetaForDb is built (references:
opMetaForDb and opMetaNow) to set those fields to null instead of '' (or omit
the keys when missing), and adjust the value type to allow null so the stored
rows preserve proper null semantics for _isNull filters.
In `@apps/pro-web/lib/hooks/use-workspace-chat.rag.test.tsx`:
- Around line 248-254: The test is passing a string ('section-1') as the third
argument to latestCtx.handleWorkspaceEdit where the function expects a
selectionRange of shape { start, end } | null; change the call in the test to
pass a valid selectionRange object (or null) instead of the string—e.g., provide
{ start: 0, end: 0 } or a range that corresponds to 'section-1'—so
handleWorkspaceEdit receives the correct type.
In `@CHAT_REFACTOR.md`:
- Around line 287-334: Duplicate and out-of-order Markdown headings in
CHAT_REFACTOR.md (e.g., "Suggested implementation order", "User Request",
"Handoff summary") are triggering markdownlint MD024/MD001; locate each repeated
heading block (noted around lines 287, 684-688, 848-855, 938-944, 1104-1123) and
either rename or demote duplicates and adjust heading levels so they increment
by only one step (e.g., H2 -> H3, H3 -> H4) to restore a single unique heading
instance and correct hierarchical order across the document; ensure any
references or TOC entries match the new names/levels.
- Around line 21-25: Fix the minor typos and hyphenation in the requirements
text around the heading "### 5) In our current chat threads the first question
and answer is not collapsible form the very top" and the surrounding paragraphs
that mention "third level subsection" and "level two subsections": correct
"form" to "from", standardize hyphenation (e.g., "level-two" and "level-three"
where used as compound adjectives), and clean punctuation/spacing in the
sentence about moving the 'x' to the top corner so the paragraph reads clearly
and consistently with other headings.
♻️ Duplicate comments (3)
apps/pro-web/lib/hooks/use-mb-chat.tsx (2)
426-455: Gate tool-call debug logs behind dev mode.
console.loginnormalizeToolCallandonToolCallwill run on every tool call in production. Please gate or remove.🔧 Suggested fix
function normalizeToolCall(toolCall: ToolCall<string, unknown>): AiToolCall { - console.log('normalizeToolCall object —> ', { toolCall }) + if (appConfig.features.devMode) { + console.log('normalizeToolCall object —> ', { toolCall }) + } @@ onToolCall({ toolCall }: { toolCall: ToolCall<string, unknown> }) { - console.log('Tool call:', toolCall) + if (appConfig.features.devMode) { + console.log('Tool call:', toolCall) + }Also applies to: 869-879
1129-1182: DerivedscopeTitleSnapshotisn’t propagated.You compute
scopeTitleSnapshotbut still sendscopeTitleSnapshotForMeta, so the derived fallback is dropped.🔧 Suggested fix
- scope_title_snapshot: scopeTitleSnapshotForMeta, + scope_title_snapshot: scopeTitleSnapshot, @@ - scope_title_snapshot: scopeTitleSnapshotForMeta, + scope_title_snapshot: scopeTitleSnapshot, @@ - scopeTitleSnapshot: scopeTitleSnapshotForMeta, + scopeTitleSnapshot: scopeTitleSnapshot,Also applies to: 1252-1267
apps/pro-web/lib/hooks/use-workspace-chat.tsx (1)
944-953: IncludedocNodeIdin scoped history + pass fallback docNodeId to MBChat.The scoped history fetch still omits
docNodeId, so new-scheme rows (doc_node_id+ barescope_id) won’t match. Also, the scope payload passesdocRootId ?? null, which drops thedocumentKeyfallback.🔧 Suggested fix
- if (threadId && jwt && scopeType === 'heading_thread' && scopeId) { - const window = await selectScopeConversationWindow({ - threadId, - scopeType, - scopeId, - jwt, - }) + if (threadId && jwt && scopeType === 'heading_thread' && scopeId) { + const normalizedScopeId = + docNodeId && scopeId.startsWith(`${docNodeId}::`) + ? scopeId.slice(`${docNodeId}::`.length) + : scopeId + const window = await selectScopeConversationWindow({ + threadId, + scopeType, + scopeId: normalizedScopeId, + docNodeId, + jwt, + }) @@ - docNodeId: docRootId ?? null, + docNodeId: docNodeId ?? null,Also applies to: 1011-1016
🧹 Nitpick comments (1)
apps/pro-web/lib/hooks/use-mb-chat.tsx (1)
329-329: Avoid shadowingDOC_CONTEXT_POLICY.There’s already a top-level constant; re-declaring inside the provider is redundant and makes it easier to diverge. Consider reusing the outer constant.
| const opMetaForDb: Record< | ||
| string, | ||
| string | boolean | number | undefined | ||
| > = { | ||
| uiMode: opMetaNow.uiMode, | ||
| scopeType: opMetaNow.scopeType, | ||
| scopeId: opMetaNow.scopeId || '', | ||
| docNodeId: opMetaNow.docNodeId || '', | ||
| actionType: opMetaNow.actionType, | ||
| excludeFromQChain: opMetaNow.excludeFromQChain, | ||
| ragEligible: opMetaNow.ragEligible, | ||
| mutationAllowed: opMetaNow.mutationAllowed, | ||
| mutationApplied: opMetaNow.mutationApplied, | ||
| intentSource: opMetaNow.intentSource, | ||
| docContextIncluded: opMetaNow.docContextIncluded, | ||
| headingContentVersion: opMetaNow.headingContentVersion || '', | ||
| docContextPolicy: opMetaNow.docContextPolicy || undefined, | ||
| // scopeTitleSnapshot: opMetaNow.scopeTitleSnapshot || '', | ||
| } |
There was a problem hiding this comment.
Store nulls for scope/doc fields to preserve query semantics.
Empty strings for scopeId, docNodeId, and headingContentVersion break _isNull filters, so doc_root history/RAG lookups can miss new rows. Prefer null (or omit the keys).
✅ Suggested fix
- const opMetaForDb: Record<
- string,
- string | boolean | number | undefined
- > = {
+ const opMetaForDb: Record<
+ string,
+ string | boolean | number | null | undefined
+ > = {
uiMode: opMetaNow.uiMode,
scopeType: opMetaNow.scopeType,
- scopeId: opMetaNow.scopeId || '',
- docNodeId: opMetaNow.docNodeId || '',
+ scopeId: opMetaNow.scopeId ?? null,
+ docNodeId: opMetaNow.docNodeId ?? null,
@@
- headingContentVersion: opMetaNow.headingContentVersion || '',
- docContextPolicy: opMetaNow.docContextPolicy || undefined,
+ headingContentVersion: opMetaNow.headingContentVersion ?? null,
+ docContextPolicy: opMetaNow.docContextPolicy ?? null,
}🤖 Prompt for AI Agents
In `@apps/pro-web/lib/hooks/use-mb-chat.tsx` around lines 668 - 686, opMetaForDb
is storing empty strings for scopeId, docNodeId, and headingContentVersion which
breaks database _isNull queries; update the assignment in use-mb-chat where
opMetaForDb is built (references: opMetaForDb and opMetaNow) to set those fields
to null instead of '' (or omit the keys when missing), and adjust the value type
to allow null so the stored rows preserve proper null semantics for _isNull
filters.
| await act(async () => { | ||
| // 2. Act: Trigger "Rewrite" | ||
| await latestCtx.handleWorkspaceEdit( | ||
| 'Rewrite this', | ||
| 'rewrite', | ||
| 'section-1', | ||
| ) |
There was a problem hiding this comment.
Pass a valid selectionRange to handleWorkspaceEdit.
A string is passed where { start, end } | null is expected, which can leak an invalid type into state.
✅ Suggested fix
- await latestCtx.handleWorkspaceEdit(
- 'Rewrite this',
- 'rewrite',
- 'section-1',
- )
+ await latestCtx.handleWorkspaceEdit(
+ 'Rewrite this',
+ 'rewrite',
+ null,
+ )🤖 Prompt for AI Agents
In `@apps/pro-web/lib/hooks/use-workspace-chat.rag.test.tsx` around lines 248 -
254, The test is passing a string ('section-1') as the third argument to
latestCtx.handleWorkspaceEdit where the function expects a selectionRange of
shape { start, end } | null; change the call in the test to pass a valid
selectionRange object (or null) instead of the string—e.g., provide { start: 0,
end: 0 } or a range that corresponds to 'section-1'—so handleWorkspaceEdit
receives the correct type.
| that will show the headings and conversations around those sections. We can also slightly indent this third level subsection. The conversations around the level two subsections will also be below the level 3 subsection accordions so that people can follow the level 2 subsection conversations more easily. | ||
|
|
||
| ### 5) In our current chat threads the first question and answer is not collapsible form the very top | ||
|
|
||
| title section, but instead has a 'x' mark to close the entire thread. We should just add the collapse/expand arrow at the top header section and move the 'x' to the very top corner in a blank row section at the very top of the thread component. Please draft a build plan with references to specific files we need to update to implement these changes in our current codebase. |
There was a problem hiding this comment.
Fix minor typos in the requirement list.
Spelling and hyphenation tweaks will keep the doc clean.
✏️ Suggested edits
- that will show the headings and conversations around those sections. We can also slightly indent this third level subsection.
+ that will show the headings and conversations around those sections. We can also slightly indent this third-level subsection.
@@
- title section, but instead has a 'x' mark to close the entire thread.
+ title section, but instead has a 'x' mark to close the entire thread.
@@
- first question and answer is not collapsible form the very top
+ first question and answer is not collapsible from the very top🧰 Tools
🪛 LanguageTool
[grammar] ~21-~21: Use a hyphen to join words.
Context: .... We can also slightly indent this third level subsection. The conversations arou...
(QB_NEW_EN_HYPHEN)
[grammar] ~23-~23: Ensure spelling is correct
Context: ... question and answer is not collapsible form the very top title section, but instea...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@CHAT_REFACTOR.md` around lines 21 - 25, Fix the minor typos and hyphenation
in the requirements text around the heading "### 5) In our current chat threads
the first question and answer is not collapsible form the very top" and the
surrounding paragraphs that mention "third level subsection" and "level two
subsections": correct "form" to "from", standardize hyphenation (e.g.,
"level-two" and "level-three" where used as compound adjectives), and clean
punctuation/spacing in the sentence about moving the 'x' to the top corner so
the paragraph reads clearly and consistently with other headings.
| ### 7) Suggested implementation order (fastest path) | ||
|
|
||
| 1. Locate current thread UI entrypoint (where X + first Q/A are rendered). | ||
| 2. Add docNodeId support (types first; DB/ORM if needed). | ||
| 3. Implement outline builder (or adapt existing). | ||
| 4. Update thread fetch to return outline + messages grouped. | ||
| 5. Implement recursive accordion renderer with required message ordering. | ||
| 6. Add L2 horizontal row + See more. | ||
| 7. Add top chrome row + collapsible title. | ||
| 8. Update offline cache + add tests. | ||
| “Files to update” starter checklist (copy/paste into your ticket) Use this as your working list—replace paths once rg reveals the exact names. | ||
|
|
||
| - Schema/ORM | ||
| - prisma/schema.prisma (Message: add docNodeId; optional DocumentNode | ||
| model) | ||
|
|
||
| - prisma/migrations/\* (backfill) | ||
|
|
||
| - Server | ||
| - src/server/api/routers/thread.ts (thread fetch includes outline + grouping) | ||
| - src/server/services/threadService.ts (joins + grouping) | ||
| - src/server/services/documentOutline.ts (outline build/load) | ||
| - Shared types | ||
| - packages/shared/src/types/chat.ts | ||
| - Client state | ||
| - src/stores/threadStore.ts (expandedNodeIds, seeMoreByNodeId) | ||
| - src/lib/indexeddb/threadCache.ts (if present) | ||
| - UI | ||
| - src/components/chat/ThreadView.tsx (top chrome row + title accordion) | ||
| - src/components/chat/ThreadNodeAccordion.tsx (new) (recursive renderer) | ||
| - src/components/chat/MessageList.tsx (render by docNodeId groups) | ||
|
|
||
| <!-- Page 8 --> | ||
|
|
||
| - src/components/ui/accordion.tsx (if tweaks needed) | ||
| - src/styles/chatThread.css or Tailwind classes in components | ||
| If you paste the output of these two commands (just the filenames) I can turn the “likely” file list above into an exact, repo-accurate set of file paths and a step-by-step PR breakdown: | ||
|
|
||
| ```bash | ||
| rg -n --files | rg -i "thread|chat|message|accordion|document" | ||
| rg -n "ThreadView|ChatThread|DocumentThread|XMarkIcon|Accordion" src packages | ||
| ``` | ||
|
|
||
| According to a document from the project knowledge base (these PDF exports don’t include a publish date), here’s a fastest-path build plan with specific file touchpoints to implement the nested markdown-header accordions + updated thread header behavior. | ||
|
|
||
| ### 7) Suggested implementation order (fastest path) | ||
|
|
||
| ### 1) Locate the current thread UI entrypoint (where the X + first Q/A are rendered) |
There was a problem hiding this comment.
Resolve duplicate headings + heading level jump to satisfy markdownlint.
The document repeats several headings (e.g., multiple “Suggested implementation order” / “User Request”) and also skips heading levels around the “Handoff summary.” Rename or demote duplicates and keep level increments by one to avoid MD024/MD001 violations.
Also applies to: 684-688, 848-855, 938-944, 1104-1123
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
332-332: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
In `@CHAT_REFACTOR.md` around lines 287 - 334, Duplicate and out-of-order Markdown
headings in CHAT_REFACTOR.md (e.g., "Suggested implementation order", "User
Request", "Handoff summary") are triggering markdownlint MD024/MD001; locate
each repeated heading block (noted around lines 287, 684-688, 848-855, 938-944,
1104-1123) and either rename or demote duplicates and adjust heading levels so
they increment by only one step (e.g., H2 -> H3, H3 -> H4) to restore a single
unique heading instance and correct hierarchical order across the document;
ensure any references or TOC entries match the new names/levels.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/pro-web/services/hasura/hasura.service.ts`:
- Around line 726-741: Remove the redundant explicit doc_node_id in the
saveNewMessage signature and eliminate both uses of "as any": change the
parameter to params: Partial<SaveNewMessageParams> (so doc_node_id still comes
from the type), destructure with const { jwt, docNodeId, doc_node_id, ...object
} = params (no as any), and replace the canonicalDocNodeId declaration with an
explicit type string | null, e.g. const canonicalDocNodeId: string | null =
(docNodeId ?? doc_node_id ?? null) as string | null (or without the final cast
if inference suffices).
♻️ Duplicate comments (1)
apps/pro-web/services/hasura/hasura.service.ts (1)
831-837: Retry failure still swallowed after logging.
Line 831 logsretryErrbut falls through to the generic error, masking the retry cause.🐛 Suggested fix
} catch (retryErr) { console.error('Error saving new message (retry failed):', retryErr) + throw new Error('Failed to save new message (retry without optional fields also failed).') }
| export async function saveNewMessage( | ||
| params: Partial<SaveNewMessageParams> & { doc_node_id?: string | null }, | ||
| ) { | ||
| const { jwt, docNodeId, doc_node_id, ...object } = params as any | ||
| const client = getHasuraClient({ jwt }) | ||
|
|
||
| // During migration, callers may pass either camelCase or snake_case. | ||
| // Only send the camelCase field to Hasura (GraphQL naming is camelCase here). | ||
| const canonicalDocNodeId: string | null = (docNodeId ?? | ||
| doc_node_id ?? | ||
| null) as any | ||
| const insertObject = | ||
| canonicalDocNodeId == null | ||
| ? object | ||
| : { ...object, docNodeId: canonicalDocNodeId } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect SaveNewMessageParams definition
rg -n -C5 "SaveNewMessageParams" apps/pro-web/services/hasura/hasura.service.type.tsRepository: bitcashorg/masterbots
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Also check if SaveNewMessageParams is defined elsewhere
fd -e ts -e tsx . apps/pro-web | xargs rg "type SaveNewMessageParams|interface SaveNewMessageParams" 2>/dev/nullRepository: bitcashorg/masterbots
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Check how saveNewMessage is called to understand the flexibility needs
rg -n "saveNewMessage\(" apps/pro-web --type ts --type tsx -B2 -A2 | head -80Repository: bitcashorg/masterbots
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Read the hasura.service.type.ts file to find SaveNewMessageParams
cat apps/pro-web/services/hasura/hasura.service.type.ts | grep -n "SaveNewMessageParams" -A 20Repository: bitcashorg/masterbots
Length of output: 730
🏁 Script executed:
#!/bin/bash
# Search for saveNewMessage function calls across the codebase
rg "saveNewMessage" apps/pro-web --type ts -B 2 -A 2Repository: bitcashorg/masterbots
Length of output: 1149
🏁 Script executed:
#!/bin/bash
# Also check the full saveNewMessage implementation to understand current type handling
sed -n '726,750p' apps/pro-web/services/hasura/hasura.service.tsRepository: bitcashorg/masterbots
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Find HasuraServiceParams definition
rg -n "interface HasuraServiceParams|type HasuraServiceParams" apps/pro-web --type ts -A 10Repository: bitcashorg/masterbots
Length of output: 914
🏁 Script executed:
#!/bin/bash
# Check what properties exist in HasuraServiceParams
grep -n "HasuraServiceParams" apps/pro-web/services/hasura/hasura.service.type.ts -A 10Repository: bitcashorg/masterbots
Length of output: 1538
Remove as any and redundant type union in saveNewMessage.
Line 734–736 uses unnecessary as any which should be replaced with the explicit type as string | null. Additionally, the function parameter includes a redundant doc_node_id?: string | null that already exists in SaveNewMessageParams. Line 729's as any on the destructure can be removed by eliminating the redundant parameter.
♻️ Suggested refactor
export async function saveNewMessage(
- params: Partial<SaveNewMessageParams> & { doc_node_id?: string | null },
+ params: Partial<SaveNewMessageParams>,
) {
- const { jwt, docNodeId, doc_node_id, ...object } = params as any
+ const { jwt, docNodeId, doc_node_id, ...object } = params as Partial<SaveNewMessageParams>
const client = getHasuraClient({ jwt })
// During migration, callers may pass either camelCase or snake_case.
// Only send the camelCase field to Hasura (GraphQL naming is camelCase here).
- const canonicalDocNodeId: string | null = (docNodeId ??
- doc_node_id ??
- null) as any
+ const canonicalDocNodeId = (docNodeId ?? doc_node_id ?? null) as string | null
const insertObject =
canonicalDocNodeId == null
? object
: { ...object, docNodeId: canonicalDocNodeId }Per coding guidelines, avoid any and prefer specific types.
🤖 Prompt for AI Agents
In `@apps/pro-web/services/hasura/hasura.service.ts` around lines 726 - 741,
Remove the redundant explicit doc_node_id in the saveNewMessage signature and
eliminate both uses of "as any": change the parameter to params:
Partial<SaveNewMessageParams> (so doc_node_id still comes from the type),
destructure with const { jwt, docNodeId, doc_node_id, ...object } = params (no
as any), and replace the canonicalDocNodeId declaration with an explicit type
string | null, e.g. const canonicalDocNodeId: string | null = (docNodeId ??
doc_node_id ?? null) as string | null (or without the final cast if inference
suffices).
AndlerRL
left a comment
AndlerRL
left a comment
There was a problem hiding this comment.
Woah, a lots of changes to do in the observations! Definitely there is a consistent issue between logics and steps for the RAG system. There are parts that aren't connected, others connected but with wrong structure and other with the structure and connected but not well referenced (not grabbing the actual variables to make the system work).
We need to improve this code since it has a lot of code repetitions, overkills strategies, cumbersome validations and not so clear flows and flows that violates the Modular Development that we are following.
I left my comments to take them into account, along with the coderabbitai feedbvacks that we should take has the " 👀 " reaction from me on it.
Conceptually, the logic makes sense, I could see how the RAG is connecting to the thread documents and how these document sections are related with the root doc, parent sections to be later embedded and pushed into the simple rag system, creating relationships and context modular and optimize the AI responses. While this concept is correct, the application of it went a bit away from the expected structure and logic. Is making his own logic while "connecting" with the actual, tagging it as "legacy" where it shouldn't because this RAG system need to be built according to the current architecture, not creating a new one and threat everything as "old".
I will be creating a development plan to gather ALL the feedbacks and do a structured fix of them.
@coderabbitai after reviewing and giving this review, proceed to gather also all the feedbacks and create a development plan, to recap all the observations in order.
| $$; | ||
|
|
||
| -- 3) Hasura-facing wrapper (text -> vector cast) | ||
| CREATE OR REPLACE FUNCTION public.search_assistant_answers_gql( |
There was a problem hiding this comment.
We can remove this.
@coderabbitai elaborate an issue to track the deletion of this table. The GraphQL table is overkill since from the main table we get the graphql.
There was a problem hiding this comment.
🧩 Analysis chain
@AndlerRL, I'll create an issue to track the removal of the redundant GraphQL wrapper function.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue to track deletion of search_assistant_answers_gql wrapper
gh issue create \
--repo bitcashorg/masterbots \
--title "Remove redundant search_assistant_answers_gql wrapper function" \
--body "## Context
The \`search_assistant_answers_gql\` function and \`assistant_answer_hits_view\` were introduced in migration \`1766629067789_assistant_answers_fn\` as a Hasura-facing wrapper to convert text embeddings to vector format for similarity search.
## Issue
This GraphQL wrapper is overkill since Hasura can expose the main \`assistant_answers\` table directly via GraphQL, eliminating the need for an intermediate view and wrapper function.
## Proposed Changes
Remove the following from the database:
- \`public.assistant_answer_hits_view\` (view)
- \`public.search_assistant_answers_gql\` (function)
Consider whether \`public.search_assistant_answers\` core function should be retained for server-side SQL calls or if it can also be removed in favor of direct table queries.
## References
- PR: `#600`
- Discussion: https://github.com/bitcashorg/masterbots/pull/600#discussion_r2699695376
- Migration file: \`apps/hasura/migrations/masterbots/1766629067789_assistant_answers_fn/up.sql\`
- Requested by: `@AndlerRL`" \
--assignee AndlerRL
Length of output: 51
@AndlerRL, I've created issue #602 to track the removal of the redundant GraphQL wrapper function: #602
The issue documents:
- The redundancy of
search_assistant_answers_gqlandassistant_answer_hits_view - The rationale that direct Hasura GraphQL access to the main table is sufficient
- References to this PR and comment discussion
- Assigned to you for follow-up
🧠 Learnings used
Learnt from: AndlerRL
Repo: bitcashorg/masterbots PR: 542
File: apps/web/components/layout/profile/layout.tsx:67-68
Timestamp: 2025-08-29T20:34:24.433Z
Learning: AndlerRL prefers skeleton loading states over conditional rendering for better UX when components are waiting for data to load. Components should show loading indicators rather than blank spaces.
Learnt from: AndlerRL
Repo: bitcashorg/masterbots PR: 0
File: :0-0
Timestamp: 2025-11-21T19:47:50.195Z
Learning: For repo bitcashorg/masterbots (apps/pro-web), AndlerRL prefers integrating TanStack Query into React Context Hooks wherever they fit to manage shared server state and side effects, with a longer-term plan to align broader state management with TanStack Query and potentially replace React Context hooks. Captured from PR `#584` discussion on Nov 21, 2025.
| ON public.assistant_answers (thread_id, scope_type, scope_id, turn_index DESC); | ||
|
|
||
| -- 3) New scoped retrieval function (keep existing search_assistant_answers_gql unchanged) | ||
| CREATE OR REPLACE FUNCTION public.search_assistant_answers_scoped_gql( |
There was a problem hiding this comment.
We have to remove this too. _gql tables and functions are redundant. Graphql use by default the SQL tables and functions and converts it for Graphql system if necessary.
@coderabbitai proceed to include this into the new issue that tracks the removal of these tables and functions.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing issues related to _gql removal or cleanup
gh issue list --repo bitcashorg/masterbots --search "_gql redundant OR remove _gql OR cleanup _gql" --limit 20 --json number,title,state,body
Length of output: 2
🏁 Script executed:
#!/bin/bash
# Also search for issues that might mention "graphql" and "functions" or "tables"
gh issue list --repo bitcashorg/masterbots --search "graphql functions tables remove OR graphql redundant" --limit 20 --json number,title,state,body
Length of output: 2
🏁 Script executed:
#!/bin/bash
# Search all open issues to find any that might be tracking this cleanup
gh issue list --repo bitcashorg/masterbots --state open --limit 50 --json number,title,body | jq '.[] | select(.title | test("gql|GraphQL|function|cleanup"; "i")) | {number, title, body}'
Length of output: 16162
🏁 Script executed:
#!/bin/bash
# Update issue `#602` to include the scoped function
gh issue edit 602 --repo bitcashorg/masterbots --body "## Context
The \`search_assistant_answers_gql\` function and \`assistant_answer_hits_view\` were introduced in migration \`1766629067789_assistant_answers_fn\` as a Hasura-facing wrapper to convert text embeddings to vector format for similarity search.
Additionally, the \`search_assistant_answers_scoped_gql\` function was introduced in migration \`1766957388317__op_meta_and_scoped_rag\` as another GraphQL wrapper for scoped RAG retrieval.
## Issue
These GraphQL wrappers are overkill since Hasura can expose the main \`assistant_answers\` table directly via GraphQL, eliminating the need for intermediate views and wrapper functions.
## Proposed Changes
Remove the following from the database:
- \`public.assistant_answer_hits_view\` (view)
- \`public.search_assistant_answers_gql\` (function)
- \`public.search_assistant_answers_scoped_gql\` (function)
Consider whether \`public.search_assistant_answers\` core function should be retained for server-side SQL calls or if it can also be removed in favor of direct table queries.
## References
- PR: `#600`
- Discussion (original): https://github.com/bitcashorg/masterbots/pull/600#discussion_r2699695376
- Discussion (scoped function): https://github.com/bitcashorg/masterbots/pull/600#discussion_r2699742431
- Migration files:
- \`apps/hasura/migrations/masterbots/1766629067789_assistant_answers_fn/up.sql\`
- \`apps/hasura/migrations/masterbots/1766957388317__op_meta_and_scoped_rag/up.sql\`
- Requested by: `@AndlerRL`"
Length of output: 51
@AndlerRL, I've updated issue #602 to include search_assistant_answers_scoped_gql in the tracking list. The issue now covers all three redundant GraphQL entities:
public.assistant_answer_hits_view(view)public.search_assistant_answers_gql(function)public.search_assistant_answers_scoped_gql(function)
I've also added references to this migration file (1766957388317__op_meta_and_scoped_rag/up.sql) and this discussion thread to the issue description.
You can view the updated issue here: #602
🧠 Learnings used
Learnt from: AndlerRL
Repo: bitcashorg/masterbots PR: 542
File: apps/web/components/layout/profile/layout.tsx:67-68
Timestamp: 2025-08-29T20:34:24.433Z
Learning: AndlerRL prefers skeleton loading states over conditional rendering for better UX when components are waiting for data to load. Components should show loading indicators rather than blank spaces.
Learnt from: AndlerRL
Repo: bitcashorg/masterbots PR: 0
File: :0-0
Timestamp: 2025-11-21T19:47:50.195Z
Learning: For repo bitcashorg/masterbots (apps/pro-web), AndlerRL prefers integrating TanStack Query into React Context Hooks wherever they fit to manage shared server state and side effects, with a longer-term plan to align broader state management with TanStack Query and potentially replace React Context hooks. Captured from PR `#584` discussion on Nov 21, 2025.
| const embeddingText = vectorToPgText(embedding) | ||
|
|
||
| // 4) compute next turn index per thread (keep existing behavior; do NOT scope-partition here) | ||
| const queryMaxTurnIndex = /* GraphQL */ ` |
There was a problem hiding this comment.
For performance and system optimization, it is better to use DrizzleORM to connect directly to the DB, so these side-effects for RAG capabilities doesn't affect UX
| } | ||
| } | ||
|
|
||
| async function hasuraGql<TData>( |
There was a problem hiding this comment.
We can remove this. We have to use mb-drizzle instead for RAG actions.
| args.push('doc_node_id: $docNodeId') | ||
| } | ||
|
|
||
| return /* GraphQL */ ` |
There was a problem hiding this comment.
We need to replace here too to use DrizzleORM instead.
| ? (error as any).message | ||
| : String(error) | ||
|
|
||
| if ( |
There was a problem hiding this comment.
Improve this guard and the overall catch block... Why it has a variable alphabet here... ?
| * key mismatches (naming conventions can differ between tables/filters/codegen). | ||
| * This preserves runtime behavior and prevents TS excess-property failures. | ||
| */ | ||
| function buildScopeWhere(args: { |
There was a problem hiding this comment.
If this is used to query documents to get the relations (no immediate chat relation), we are good however, if we are not doing it, and we are building this while we are interacting with the user, then it is better to use Drizzle ORM. Revise the usage of this.
There was a problem hiding this comment.
OK this is a duplicated. The select-scope-conversation-window.ts is doing the same... We definitely we want Drizzle and remove this one.
| } | ||
| } | ||
|
|
||
| export async function selectScopeConversationWindow(args: { |
There was a problem hiding this comment.
use-mb-chat.tsx and use-workspace-chat.tsx has a conflict with this logic. We rather want to simplify this and remove this one and use Drizzle instead.
|
|
||
| export interface ChatMessageProps extends React.ComponentProps<'div'> { | ||
| message: Message & Partial<MBMessage> | ||
| message: any |
| const fb = asThreadLike(fallbackThread) | ||
|
|
||
| return ( | ||
| ft?.slug ?? ft?.thread?.slug ?? fb?.slug ?? asString(paramThreadSlug) // (useParams can be string | string[]) |
There was a problem hiding this comment.
Simplify this. we may not need this due we already have a strong active Thread validation. If the thread doesn't have a slug, the issue might be somewhere else.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
@coderabbitai after reviewing and giving this review, proceed to gather also all the feedbacks and create a development plan, to recap all the observations in order. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 266036
Executive summary of issues to address
Proposed multi-PR development plan (prioritized, with acceptance criteria)
Phase 1 — Types and shared utilities consolidation (P0)
Phase 2 — Server actions/services and RAG pipeline (P1)
Phase 3 — UI components and hooks (P1)
Phase 4 — Hooks and observability (P1)
Phase 5 — Tests, docstrings, and CI polish (P2)
Phase 6 — Documentation and flags (P2)
Sequencing and rollout
Key acceptance checks (spotlight)
Open questions / decisions
If you approve, I’ll open a single tracking issue and start with Phase 0 (schema/index + metadata) and Phase 1 (types/shared) branches. Development plan for PR
Would you like me to open a tracking issue and kick off Phase 0 and Phase 1? ✏️ Learnings added
🧠 Learnings used💡 Use |
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.