feat: enhance loading and error handling across components#11
feat: enhance loading and error handling across components#11
Conversation
- Introduced LoadingSpinner and ErrorState components for consistent loading and error feedback. - Updated DocumentDialog, AppSidebar, and various chat components to utilize new loading and error handling mechanisms. - Improved user experience by providing visual feedback during data fetching and error states. - Refactored useQuery hooks to include loading and error states for better data management.
WalkthroughAdds reusable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as UI Component
participant H as Data Hook
participant Q as Convex Query
U->>C: open view / navigate
C->>H: call hook()
H->>Q: fetch request
Q-->>H: { data?, isLoading, isError, error }
H-->>C: { data, isLoading, isError, error }
alt isLoading
C-->>U: render LoadingSpinner (compact/centered)
else isError
C-->>U: render ErrorState(title, error)
else data empty
C-->>U: render empty placeholder
else data present
C-->>U: render main content
end
sequenceDiagram
autonumber
participant U as User
participant D as DocumentDialog
participant Q as getDocument
participant S as Storage/Server
U->>D: open dialog (id)
D->>Q: fetch document
Q-->>D: { doc, isLoading, isError, error }
alt isLoading
D-->>U: Dialog + LoadingSpinner
else isError
D-->>U: Dialog + ErrorState("Couldn't load document")
else success
D->>D: resolve preview URL by type
alt needs download URL
D->>S: generateDownloadUrl(key)
S-->>D: url
else external/url/site
D-->>D: validate/use key as URL
else youtube
D-->>D: build embed URL
end
alt preview available
D-->>U: render viewer (img/object/iframe)
else preview error
D-->>U: render ErrorState("Unable to preview")
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (6)
src/routes/chat.$chatId.lazy.tsx (1)
47-56: Type-safety issue: chatId union vs cast; also gate the query with enabled
- The component compares
chatId !== "new"butchatIdwas cast toId<"chats">earlier. This loses the union type and weakens type safety.- Align with other hooks by adding
enabled: chatId !== "new"for clarity and to avoid unnecessary fetches.const { data: queryChat, isError: isChatError, error: chatError, } = useQuery({ - ...convexQuery( + ...convexQuery( api.chats.queries.get, chatId !== "new" ? { chatId: chatId as Id<"chats"> } : "skip" ), + enabled: chatId !== "new", });Additionally, update the chatId typing (outside the selected range) to preserve the union and satisfy the project’s strict Id typing guideline:
// At Line 38 (outside selected range) const rawChatId = params.chatId as string; const chatId: Id<"chats"> | "new" = rawChatId === "new" ? "new" : (rawChatId as Id<"chats">);src/components/chat/panels/projects/document-list.tsx (1)
47-55: Early return hides loading & error statesWhile
isLoadingProjectDocsis true (or the query errored),projectDocumentsisundefined, so this block executes and the user sees “No documents added yet” instead of a spinner or error.- if (!projectDocuments || projectDocuments.projectDocuments.length === 0) { + if (!isLoadingProjectDocs && + !isProjectDocumentsError && + (!projectDocuments || projectDocuments.projectDocuments.length === 0)) {Move the loading / error checks above this guard or incorporate them into the condition so the correct UI is shown.
src/components/chat/input/document-list.tsx (1)
105-112: Duplicate / earlyreturn nullhides spinner & error UIThe first null-return executes while data is still loading (
documentsisundefined), so the new loading/error components never render.
Remove the premature check and keep a single “no documents” guard after the loading/error blocks.- if (!documents?.length) return null; ... - if (!documents?.length) return null;Place one “no documents” check after Line 134 instead.
src/components/chat/messages/index.tsx (1)
20-41: Missing deps ⇒ stale UI on error changes
useMemoomitsisError,error,isStreamError, andstreamErrorfrom its dependency list, so entering/leaving error states won’t trigger a re-render.- }, [isLoading, isEmpty, streamStatus, chatId, userLoadable]); + }, [ + isLoading, + isEmpty, + isError, + error, + isStreamError, + streamError, + streamStatus, + chatId, + userLoadable, + ]);Also applies to: 113-116
src/components/app-sidebar/index.tsx (1)
232-237: “No matching chats” never shownThe guard
!isSearchingmakes this branch skip when searching, so users get no feedback for an empty search result set.- {displayHistoryChats.length === 0 && !isSearching && ( + {displayHistoryChats.length === 0 && (Then keep the conditional text selection (
isSearching ? ...) as-is.src/components/chat/messages/user-message.tsx (1)
283-292: Show utils bar on focus for keyboard users, not only on hoverAdd a focus-visible path to avoid hiding controls from non-pointer users.
- <div className="opacity-0 flex gap-2 group-hover/message:opacity-100 transition-opacity"> + <div className="opacity-0 flex gap-2 group-hover/message:opacity-100 group-focus-within/message:opacity-100 transition-opacity">
🧹 Nitpick comments (28)
src/hooks/chats/use-chats.ts (1)
149-150: isSearching semantics: clearer and less brittleUsing
debouncedQuery.trim().length > 0is a cleaner signal than comparing two strings. Good change.If you want the UI to reflect “user is typing” immediately (before debounce triggers), consider also deriving a non-debounced flag for UX (optional).
src/components/ui/loading-spinner.tsx (1)
4-8: Make layout styling extensible without breaking current usesCurrently
classNameapplies to the icon. If callers need to style the wrapper, consider a non-breaking addition:containerClassName.interface LoadingSpinnerProps { sizeClassName?: string; className?: string; label?: string; + containerClassName?: string; } export function LoadingSpinner({ sizeClassName = "h-5 w-5", className, label, + containerClassName, }: LoadingSpinnerProps) { return ( - <div className="flex items-center justify-center gap-2"> + <div className={cn("flex items-center justify-center gap-2", containerClassName)}> <Loader2 aria-label="loading" className={cn( "animate-spin text-muted-foreground", sizeClassName, className )} /> - <p className="text-muted-foreground">{label}</p> + {label ? <p className="text-muted-foreground">{label}</p> : null} </div> ); }Also applies to: 10-14
src/hooks/chats/use-documents.ts (1)
12-18: Avoid duplicated skip conditions (skiparg vsenabled)You’re using both
chatId !== "new" ? { chatId } : "skip"andenabled: chatId !== "new". One is sufficient. Using both is harmless but redundant and can introduce divergence if conditions drift.Pick one approach (recommended: keep the
skipsentinel only for consistency with other hooks in this PR) and drop the other.src/routes/auth.tsx (1)
4-4: Consistent loading UI via LoadingSpinnerGood replacement; this standardizes the auth loading state with the rest of the app.
Optionally add a label for clarity/accessibility:
- <LoadingSpinner sizeClassName="w-8 h-8" /> + <LoadingSpinner sizeClassName="w-8 h-8" label="Checking authentication…" />Also applies to: 20-20
src/components/chat/panels/projects/chat-list.tsx (2)
18-25: React Query state destructuring looks goodState exposure is clear. Note: React Query’s error is typed as unknown; ensure
ErrorStatecan accept unknown or cast appropriately where passed.
27-33: Loading state handling is clear and accessibleSpinner + label is appropriate. Consider adding aria-busy on the container for a11y.
- <div className="flex items-center justify-center py-10"> + <div className="flex items-center justify-center py-10" aria-busy="true" aria-live="polite">src/routes/chat.$chatId.lazy.tsx (1)
62-78: Good explicit error UI; consider adding loading handlingThe error state is well-structured and consistent. Optionally add a loading spinner for the chat fetch to avoid brief content flashes while resolving the chat.
src/components/ui/accordion.tsx (1)
153-155: Avoid running rotation animation when icon is hiddenCurrently the wrapper still animates even if the icon isn’t rendered. Minor optimization and avoids layout quirks by conditionally rendering the wrapper too.
- <motion.div - animate={{ rotate: isOpen ? 0 : 180 }} - transition={{ duration: 0.25 }} - className="items-center flex justify-center " - > - {showIcon && ( - <ChevronDownIcon className="text-muted-foreground pointer-events-none size-4" /> - )} - </motion.div> + {showIcon && ( + <motion.div + animate={{ rotate: isOpen ? 0 : 180 }} + transition={{ duration: 0.25 }} + className="items-center flex justify-center " + > + <ChevronDownIcon className="text-muted-foreground pointer-events-none size-4" /> + </motion.div> + )}src/components/chat/panels/projects/list.tsx (1)
44-53: Loading state UX is solid; add aria-busy for a11yMinor accessibility enhancement.
- <div className="flex flex-1 items-center justify-center"> + <div className="flex flex-1 items-center justify-center" aria-busy="true" aria-live="polite">src/routes/settings/profile.tsx (1)
40-48: Pass a non-undefined error to ErrorState
ErrorStatereceiveserror={userError}, but whenisErrorUseris true anduserErroris stillundefined, the component will not get any details.
Consider usingerror: userError ?? new Error("Unknown error")(or omit the prop) to avoid passingundefined.src/components/chat/input/toolbar/index.tsx (1)
324-329: Include the underlying error for debuggability
ErrorStatecan show richer context if theerrorprop is supplied, mirroring other usages in the codebase.- <ErrorState + <ErrorState density="compact" title="Failed to load project" + error={/* isProjectError object here */} className="h-9" />src/components/topnav.tsx (1)
101-105: Spinner markup produces duplicate textInside a dropdown-sized area the
<LoadingSpinner>already renders its own label; the sibling<span>repeats it.
Consider removing the extra<span>to avoid redundant content.src/components/chat/input/toolbar/projects-dropdown.tsx (1)
59-112: Minor: inconsistency in loading UIThe dropdown renders both
LoadingSpinnerand a text node, but elsewhere in the codebase the spinner component already contains the label. Align with the rest of the app for consistency:- <DropdownMenuItem> - <LoadingSpinner - className="h-4 w-4" - label="Loading projects..." - /> - <span className="text-muted-foreground"> - Loading projects... - </span> - </DropdownMenuItem> + <DropdownMenuItem> + <LoadingSpinner className="h-4 w-4" label="Loading projects..." /> + </DropdownMenuItem>src/components/document-dialog.tsx (3)
24-35: Redundantenabled+"skip"usage
enabled: !!documentDialogOpenalready prevents the query from firing when the dialog is closed.
Returning"skip"in the query key is therefore unnecessary noise.
55-65:"file"handled twice
case "file"in this switch already fetches a download URL; the subsequent default‐branch check fordocument.type === "file"will never be reached.
Removing the duplicate simplifies control-flow and avoids an extra needless request if the tag ever mis-matches.Also applies to: 75-83
180-189: Unreachable in-dialog error stateThe component returns early for
isDocumentError || documentError(lines 114-132), so this inner error block is never rendered.
Delete to avoid dead code and accidental divergence in future edits.src/components/ui/error-state.tsx (1)
64-71: Early null-return can hide explicit titlesIf the caller passes a custom
titlebut neithererrornordescription, the earlyreturn nullprevents the component from rendering despite valid props.Consider dropping the guard or checking
titleseparately.src/components/chat/messages/user-message.tsx (5)
32-41: Good adoption of explicit loading/error states in DocumentButton; minor cleanupsNicely aligns with the PR goal. Consider two small tweaks:
- Rely on
isDocumentError(it implieserrorexists) instead ofisDocumentError || documentError.- Use consistent title-case microcopy.
- if (isDocumentError || documentError) { + if (isDocumentError) { return ( <div className="py-2 max-w-xs"> <ErrorState density="compact" - title="error loading attached documents" + title="Error loading attached documents" error={documentError} showDescription={false} /> </div> ); }Also applies to: 43-52, 54-65
43-52: Spinner copy/style consistencyConsider tightening copy to match other places and singular/plural cases.
- <LoadingSpinner - sizeClassName="h-4 w-4" - label="Fetching attached documents" - /> + <LoadingSpinner sizeClassName="h-4 w-4" label="Loading document…" />
67-74: Guard against missing doc name to avoid empty buttonIf the query returns no data or an unnamed doc, provide a safe fallback label.
- {documentData?.name} + {documentData?.name ?? "View document"}
85-92: EditingDocumentList loading/error handling looks solidThe loading spinner and ErrorState simplify UX and match the PR’s consistency goal. Consider matching microcopy tone and adding a label for the loading state.
- <div className="px-2 py-1 text-xs text-muted-foreground flex items-center gap-2"> - <LoadingSpinner sizeClassName="h-3 w-3" /> Loading… + <div className="px-2 py-1 text-xs text-muted-foreground flex items-center gap-2"> + <LoadingSpinner sizeClassName="h-3 w-3" label="Loading documents…" /> </div>Also applies to: 102-116
229-251: Optional: avoid N queries by batching document fetchesRendering one
useQueryperDocumentButtoncan fan out requests for messages with many attachments. Consider prefetching via a singlegetMultiplefor all file_ids incontentand pass names down (or render directly) to reduce request overhead and spinner churn.Example approach:
- Extract all file_ids from
contentin auseMemo.useQuery({...convexQuery(api.documents.queries.getMultiple, { documentIds })}).- Build a
Map<Id<"documents">, Document>and render names inline, avoiding per-item queries.Also applies to: 242-246
temp.md (4)
5-8: Fix bare URL and clarify phrasingFormat the URL and tighten the copy for readability.
-- update these - tool-streaming, because of vibz mcp as it one shots the generation. so we need to live stream to the user all the changes. - https://github.com/0bs-chat/zerobs/tree/feat/message-queue : the message queue function. +- Update tool streaming. Vibz MCP currently one-shots generation, so we need to live-stream updates to the user. +- <https://github.com/0bs-chat/zerobs/tree/feat/message-queue> — the message queue function.
9-11: Consistency and capitalization (OAuth, MCP, UI); improve readability-- vibe coding (better auth -> convex cloud migration -> streaming tool calls -> convex oauth integration -> revamp mcp templates to pass along the env vars) - custom ui for vibz mcp. (like artifacts, we will replace the panel content with the ui for vibz)(preview, dashboard (convex dashboard), code (vs code)) +- Vibe coding (Better Auth → Convex Cloud migration → streaming tool calls → Convex OAuth integration → revamp MCP templates to pass along env vars). + Custom UI for Vibz MCP (like Artifacts): replace the panel content with the Vibz UI (Preview, Dashboard (Convex dashboard), Code (VS Code)).
12-13: Grammar and style nits (capitalize proper nouns; UX; “etc.” with period)-- migrate to better auth (when i get the green light from mantra after better-auth integrates.) +- Migrate to Better Auth (when I get the green light from Mantra after Better Auth integrates). ... -- improve ux overall with loading states and whatnot. +- Improve UX overall with loading states and related polish. -- google integration (the code is already there just need to setup oauth) +- Google integration (the code is already there; just need to set up OAuth). -- business related mcp with ability to autofill connection info (like auto fetching api key/oauth key for the headers in mcp using oauth, etc to reduce friction) +- Business‑related MCP with ability to autofill connection info (e.g., auto‑fetching API key/OAuth key for MCP request headers using OAuth, etc.) to reduce friction.Also applies to: 15-15, 21-23
25-27: Clarify actionable items-- need message queue system -- immediate send, wait for file to be processed check on the frontend instead of backend +- Implement message queue system. +- Immediate send: perform “wait for file to be processed” check on the frontend instead of the backend.src/components/chat/panels/projects/details.tsx (2)
52-61: Spinner sizing and a11y nitPrefer
sizeClassNamefor sizing (avoids conflicting Tailwind classes), and add status/a11y attributes to the wrapper.- if (isLoadingProject) { - return ( - <div className="flex flex-1 items-center justify-center py-8"> - <LoadingSpinner - className="h-6 w-6" - label="Loading project details..." - /> - </div> - ); - } + if (isLoadingProject) { + return ( + <div + className="flex flex-1 items-center justify-center py-8" + role="status" + aria-busy="true" + aria-live="polite" + > + <LoadingSpinner + sizeClassName="h-6 w-6" + label="Loading project details..." + /> + </div> + ); + }
63-73: Tighten error condition and unify layout with loading
- Rely on
isProjectError(React Query’s source of truth) and avoid the redundant|| projectError.- Match the loading view’s centered layout. The current
className=" "looks accidental.- if (isProjectError || projectError) { + if (isProjectError) { return ( - <div className=" "> + <div className="flex flex-1 items-center justify-center py-8"> <ErrorState className="h-full py-2.5" title="Error loading project details" error={projectError} /> </div> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/components/app-sidebar/index.tsx(8 hunks)src/components/chat/input/document-list.tsx(3 hunks)src/components/chat/input/toolbar/index.tsx(3 hunks)src/components/chat/input/toolbar/projects-dropdown.tsx(3 hunks)src/components/chat/messages/index.tsx(3 hunks)src/components/chat/messages/user-message.tsx(7 hunks)src/components/chat/panels/mcp/index.tsx(2 hunks)src/components/chat/panels/projects/chat-list.tsx(2 hunks)src/components/chat/panels/projects/details.tsx(3 hunks)src/components/chat/panels/projects/document-list.tsx(5 hunks)src/components/chat/panels/projects/list.tsx(3 hunks)src/components/document-dialog.tsx(5 hunks)src/components/topnav.tsx(4 hunks)src/components/ui/accordion.tsx(2 hunks)src/components/ui/error-state.tsx(1 hunks)src/components/ui/loading-spinner.tsx(1 hunks)src/hooks/chats/use-chats.ts(2 hunks)src/hooks/chats/use-documents.ts(1 hunks)src/hooks/chats/use-mcp.ts(2 hunks)src/hooks/chats/use-messages.ts(2 hunks)src/hooks/chats/use-stream.ts(3 hunks)src/routes/__root.tsx(4 hunks)src/routes/auth.tsx(2 hunks)src/routes/chat.$chatId.lazy.tsx(3 hunks)src/routes/settings/apiKeys.tsx(3 hunks)src/routes/settings/profile.tsx(2 hunks)temp.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/convex_rules.mdc)
**/*.ts: You can use the helper TypeScript type Id imported from './_generated/dataModel' to get the type of the id for a given table.
If you need to define a Record, provide the correct key and value types (e.g., Record<Id<'users'>, string>).
Be strict with types, particularly around ids of documents. Use Id<'table'> rather than string for document ids.
Always use as const for string literals in discriminated union types.
When using the Array type, always define arrays as const array: Array = [...];
When using the Record type, always define records as const record: Record<KeyType, ValueType> = {...};
Files:
src/hooks/chats/use-documents.tssrc/hooks/chats/use-mcp.tssrc/hooks/chats/use-stream.tssrc/hooks/chats/use-messages.tssrc/hooks/chats/use-chats.ts
🧠 Learnings (17)
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Try to use as few calls from actions to queries and mutations as possible to avoid race conditions.
Applied to files:
src/hooks/chats/use-documents.tssrc/components/chat/panels/projects/list.tsxsrc/hooks/chats/use-mcp.tssrc/routes/chat.$chatId.lazy.tsxsrc/routes/settings/apiKeys.tsxsrc/hooks/chats/use-messages.tssrc/components/chat/panels/projects/document-list.tsxsrc/components/chat/input/document-list.tsxsrc/components/chat/panels/projects/details.tsxsrc/components/chat/input/toolbar/projects-dropdown.tsxsrc/components/document-dialog.tsxtemp.md
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Use ctx.db.replace to fully replace an existing document. This method will throw an error if the document does not exist.
Applied to files:
src/hooks/chats/use-documents.ts
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Convex queries do NOT support .delete(). Instead, .collect() the results, iterate, and call ctx.db.delete(row._id) on each result.
Applied to files:
src/hooks/chats/use-documents.tssrc/components/chat/panels/projects/details.tsxsrc/components/chat/messages/user-message.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Use .unique() to get a single document from a query. This method will throw an error if there are multiple documents.
Applied to files:
src/hooks/chats/use-documents.tssrc/components/chat/input/document-list.tsxsrc/components/chat/messages/user-message.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/* : Convex uses file-based routing; organize files with public query, mutation, or action functions within the convex/ directory.
Applied to files:
src/routes/settings/profile.tsxsrc/routes/auth.tsxsrc/components/chat/input/document-list.tsxsrc/components/chat/panels/projects/details.tsxsrc/components/chat/input/toolbar/projects-dropdown.tsxsrc/components/document-dialog.tsxtemp.md
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Use ctx.runQuery to call a query, ctx.runMutation to call a mutation, and ctx.runAction to call an action. Only call an action from another action if you need to cross runtimes (e.g., from V8 to Node). Otherwise, pull out shared code into a helper async function.
Applied to files:
src/components/chat/panels/projects/list.tsxsrc/hooks/chats/use-mcp.tssrc/routes/settings/apiKeys.tsxsrc/components/chat/panels/projects/document-list.tsxsrc/components/chat/input/document-list.tsxsrc/components/chat/panels/projects/details.tsxsrc/components/chat/input/toolbar/projects-dropdown.tsxsrc/components/document-dialog.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Use internalQuery, internalMutation, and internalAction to register internal functions (private, only callable by other Convex functions). Use query, mutation, and action to register public functions (exposed to the public Internet). Do NOT use query, mutation, or action for sensitive internal functions.
Applied to files:
src/components/chat/panels/projects/list.tsxsrc/routes/settings/apiKeys.tsxsrc/components/chat/panels/projects/document-list.tsxsrc/components/chat/input/document-list.tsxsrc/components/chat/panels/projects/details.tsxsrc/components/chat/input/toolbar/projects-dropdown.tsxsrc/components/document-dialog.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : ALWAYS use the new function syntax for Convex functions (using query/mutation/internalQuery/internalMutation/action/internalAction with args, returns, and handler).
Applied to files:
src/hooks/chats/use-mcp.tssrc/components/chat/panels/projects/document-list.tsxsrc/components/chat/input/document-list.tsxsrc/components/chat/panels/projects/details.tsxsrc/components/chat/input/toolbar/projects-dropdown.tsxsrc/components/document-dialog.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : All ctx.runQuery, ctx.runMutation, and ctx.runAction calls take a FunctionReference. Do NOT pass the callee function directly.
Applied to files:
src/components/chat/panels/projects/document-list.tsxsrc/components/chat/panels/projects/details.tsxsrc/components/document-dialog.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Use the api object from convex/_generated/api.ts to call public functions and the internal object to call internal functions.
Applied to files:
src/components/chat/input/document-list.tsxsrc/components/chat/panels/projects/details.tsxsrc/components/chat/input/toolbar/projects-dropdown.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/schema.ts : Always import schema definition functions from convex/server in convex/schema.ts.
Applied to files:
src/components/chat/input/document-list.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Never use ctx.db inside of an action. Actions don't have access to the database.
Applied to files:
src/components/chat/input/document-list.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Always add "use node"; to the top of files containing actions that use Node.js built-in modules.
Applied to files:
src/components/chat/input/document-list.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Convex storage stores items as Blob objects. You must convert all items to/from a Blob when using Convex storage.
Applied to files:
src/components/chat/input/document-list.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : When using ctx.runQuery, ctx.runMutation, or ctx.runAction to call a function in the same file, specify a type annotation on the return value to work around TypeScript circularity limitations.
Applied to files:
src/components/chat/panels/projects/details.tsxsrc/components/chat/input/toolbar/projects-dropdown.tsxsrc/components/document-dialog.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : ALWAYS include argument and return validators for all Convex functions (query, internalQuery, mutation, internalMutation, action, internalAction). If a function doesn't return anything, include returns: v.null() as its output validator.
Applied to files:
src/components/chat/panels/projects/details.tsx
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : When using async iteration, don't use .collect() or .take(n) on the result of a query. Use for await (const row of query) instead.
Applied to files:
src/components/chat/messages/user-message.tsx
🧬 Code Graph Analysis (13)
src/components/chat/panels/projects/chat-list.tsx (2)
src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)src/components/ui/error-state.tsx (1)
ErrorState(115-115)
src/routes/settings/profile.tsx (2)
src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)src/components/ui/error-state.tsx (1)
ErrorState(115-115)
src/components/chat/panels/projects/list.tsx (2)
src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)src/components/ui/error-state.tsx (1)
ErrorState(115-115)
src/routes/chat.$chatId.lazy.tsx (1)
src/components/ui/error-state.tsx (1)
ErrorState(115-115)
src/routes/auth.tsx (1)
src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)
src/routes/settings/apiKeys.tsx (2)
src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)src/components/ui/error-state.tsx (1)
ErrorState(115-115)
src/components/chat/input/document-list.tsx (2)
src/store/chatStore.ts (1)
documentDialogOpenAtom(43-45)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)
src/components/chat/input/toolbar/index.tsx (3)
src/components/ui/button.tsx (1)
Button(59-59)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)src/components/ui/error-state.tsx (1)
ErrorState(115-115)
src/components/chat/panels/mcp/index.tsx (2)
src/hooks/chats/use-mcp.ts (1)
useMCPs(42-143)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)
src/components/chat/panels/projects/details.tsx (2)
src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)src/components/ui/error-state.tsx (1)
ErrorState(115-115)
src/components/chat/input/toolbar/projects-dropdown.tsx (3)
src/components/ui/error-state.tsx (1)
ErrorState(115-115)src/components/ui/dropdown-menu.tsx (2)
DropdownMenuItem(248-248)DropdownMenuSeparator(252-252)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(10-28)
src/components/ui/loading-spinner.tsx (1)
src/lib/utils.ts (1)
cn(4-6)
src/components/ui/error-state.tsx (1)
src/lib/utils.ts (1)
cn(4-6)
🪛 Biome (2.1.2)
src/components/topnav.tsx
[error] 58-58: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 LanguageTool
temp.md
[style] ~23-~23: In American English, abbreviations like “etc.” require a period.
Context: ...key for the headers in mcp using oauth, etc to reduce friction) - need message que...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
temp.md
7-7: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
src/hooks/chats/use-mcp.ts (1)
43-48: Verify callsites for expanded return shape and data consistencyWe’ve confirmed two usages of
useMCPs()in the repo:
src/components/chat/panels/mcp/mcp-dialog.tsx
OnlyhandleCreateandvalidateMCPare destructured here—no impact from the addedisLoading/isError/errorfields.src/components/chat/panels/mcp/index.tsx
DestructuresisErroranderror, then callsgetAllMCPs()for the data.Destructuring extra fields is non-breaking, but please verify that:
getAllMCPs()always returns an array (notnull) to prevent rendering issues when there’s no data.- Consumers handle the new
isLoadingstate where applicable.src/hooks/chats/use-chats.ts (1)
129-134: Solid: expose search loading/error state directly from the queryThis matches the new loading/error handling pattern and is consistent with other hooks. No functional issues spotted.
src/components/chat/panels/projects/chat-list.tsx (1)
8-9: LGTM: consistent UI imports for loading/errorImports align with shared UI patterns introduced in this PR.
src/routes/chat.$chatId.lazy.tsx (1)
27-27: LGTM: ErrorState importMatches the shared error UI introduced in this PR.
src/components/ui/accordion.tsx (2)
112-113: Prop addition is sensible and backwards compatibleOptional
showIconincreases flexibility without breaking callers.
116-116: DefaultshowIcon= true is appropriateSane default preserves existing visuals.
src/hooks/chats/use-messages.ts (1)
25-36: LGTM: explicit loading/error exposure from queryClear and consistent with consumers like ChatMessages. Good use of
enabledwith the"new"sentinel.src/components/chat/panels/projects/list.tsx (2)
5-6: LGTM: shared UI importsConsistent with the cross-app loading/error pattern.
21-26: React Query state destructuring is clearGood naming and exposure for downstream conditional UI.
src/routes/settings/apiKeys.tsx (1)
310-329: Loading / error handling looks solidNice, the new spinner and
ErrorStatebranches make the UX clearer and the conditions (isLoading,isError || error) are correct.src/hooks/chats/use-stream.ts (1)
182-184: Good additionExposing aggregated
isErroranderrorflags from the hook simplifies consumers.src/components/chat/panels/projects/details.tsx (2)
6-7: New loading/error UI imports — LGTMImports are correct and align with the PR’s goal of consistent async UX.
24-29: Good: explicit query state exposureClear naming and explicit
isLoading,isError, anderrorimprove readability and UX handling.
| <Badge | ||
| key={doc._id} | ||
| variant="secondary" | ||
| className="flex items-center gap-1.5 pr-1 cursor-pointer" | ||
| variant="default" | ||
| className="group/document flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center" | ||
| onClick={() => handlePreview(doc._id)} | ||
| > | ||
| <Icon className={`${IconClassName} h-3 w-3`} /> | ||
| <span className="max-w-32 truncate">{doc.name}</span> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-4 w-4 p-0.5 hover:bg-muted-foreground/20" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onRemove(doc._id); | ||
| }} | ||
| > | ||
| <XIcon className="w-3 h-3" /> | ||
| </Button> | ||
| <div className="relative h-4 w-4"> | ||
| <Icon | ||
| className={`${IconClassName} h-4 w-4 group-hover/document:opacity-0 transition duration-300`} | ||
| /> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="absolute inset-0 h-4 w-4 opacity-0 group-hover/document:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onRemove(doc._id); | ||
| }} | ||
| > | ||
| <XIcon className="w-3 h-3" /> | ||
| </Button> | ||
| </div> | ||
| <span className="max-w-32 truncate text-xs cursor-pointer"> | ||
| {doc.name} | ||
| </span> | ||
| </Badge> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the document chip and remove button keyboard- and screen reader-accessible
Badge is clickable but not a native button; the icon-only remove button lacks a label; remove control is hover-only. Add role/tabIndex + keyboard handlers and aria-labels. Also expose visibility on focus.
- <Badge
+ <Badge
key={doc._id}
variant="default"
- className="group/document flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center"
- onClick={() => handlePreview(doc._id)}
+ className="group/document flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center focus:outline-none focus-visible:ring-2 focus-visible:ring-offset-2"
+ role="button"
+ tabIndex={0}
+ aria-label={`Preview document ${doc.name}`}
+ onClick={() => handlePreview(doc._id)}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ handlePreview(doc._id);
+ }
+ }}
>
<div className="relative h-4 w-4">
<Icon
- className={`${IconClassName} h-4 w-4 group-hover/document:opacity-0 transition duration-300`}
+ className={`${IconClassName} h-4 w-4 group-hover/document:opacity-0 group-focus-within/document:opacity-0 transition duration-300`}
/>
<Button
variant="ghost"
size="icon"
- className="absolute inset-0 h-4 w-4 opacity-0 group-hover/document:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300"
+ className="absolute inset-0 h-4 w-4 opacity-0 group-hover/document:opacity-100 group-focus-within/document:opacity-100 focus:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300"
+ aria-label={`Remove document ${doc.name}`}
+ title="Remove"
onClick={(e) => {
e.stopPropagation();
onRemove(doc._id);
}}
>
<XIcon className="w-3 h-3" />
</Button>
</div>📝 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.
| <Badge | |
| key={doc._id} | |
| variant="secondary" | |
| className="flex items-center gap-1.5 pr-1 cursor-pointer" | |
| variant="default" | |
| className="group/document flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center" | |
| onClick={() => handlePreview(doc._id)} | |
| > | |
| <Icon className={`${IconClassName} h-3 w-3`} /> | |
| <span className="max-w-32 truncate">{doc.name}</span> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="h-4 w-4 p-0.5 hover:bg-muted-foreground/20" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onRemove(doc._id); | |
| }} | |
| > | |
| <XIcon className="w-3 h-3" /> | |
| </Button> | |
| <div className="relative h-4 w-4"> | |
| <Icon | |
| className={`${IconClassName} h-4 w-4 group-hover/document:opacity-0 transition duration-300`} | |
| /> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="absolute inset-0 h-4 w-4 opacity-0 group-hover/document:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onRemove(doc._id); | |
| }} | |
| > | |
| <XIcon className="w-3 h-3" /> | |
| </Button> | |
| </div> | |
| <span className="max-w-32 truncate text-xs cursor-pointer"> | |
| {doc.name} | |
| </span> | |
| </Badge> | |
| <Badge | |
| key={doc._id} | |
| variant="default" | |
| className="group/document flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center focus:outline-none focus-visible:ring-2 focus-visible:ring-offset-2" | |
| role="button" | |
| tabIndex={0} | |
| aria-label={`Preview document ${doc.name}`} | |
| onClick={() => handlePreview(doc._id)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| handlePreview(doc._id); | |
| } | |
| }} | |
| > | |
| <div className="relative h-4 w-4"> | |
| <Icon | |
| className={`${IconClassName} h-4 w-4 group-hover/document:opacity-0 group-focus-within/document:opacity-0 transition duration-300`} | |
| /> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="absolute inset-0 h-4 w-4 opacity-0 group-hover/document:opacity-100 group-focus-within/document:opacity-100 focus:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300" | |
| aria-label={`Remove document ${doc.name}`} | |
| title="Remove" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onRemove(doc._id); | |
| }} | |
| > | |
| <XIcon className="w-3 h-3" /> | |
| </Button> | |
| </div> | |
| <span className="max-w-32 truncate text-xs cursor-pointer"> | |
| {doc.name} | |
| </span> | |
| </Badge> |
🤖 Prompt for AI Agents
In src/components/chat/messages/user-message.tsx between lines 133 and 158, the
Badge component is clickable but not keyboard accessible and lacks proper ARIA
roles, and the remove Button is icon-only without an aria-label and only visible
on hover. To fix this, add role="button" and tabIndex={0} to the Badge,
implement keyboard event handlers for Enter and Space keys to trigger
handlePreview, add an aria-label to the remove Button describing its action, and
ensure the remove Button is visible on keyboard focus as well as hover by
adjusting its CSS classes accordingly.
| isSearching, | ||
| isLoadingSearch, | ||
| isSearchError, | ||
| searchError, | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Return type consistency for empty results
searchResults: searchResults || [] is pragmatic. If api.chats.queries.search returns chats, you may make the fallback explicit to keep strict typing happy in consumers:
- searchResults: searchResults || [],
+ searchResults: (searchResults ?? []) as Array<Doc<"chats">>,If unsure of the query’s return type, locate the Convex definition to confirm:
🏁 Script executed:
#!/bin/bash
# Try to find the search query and its return signature
fd -t f chats convex
rg -n --no-heading "queries\.search" -A 8 -B 4Length of output: 854
🏁 Script executed:
#!/bin/bash
# Locate the Convex query definition for `search`
rg -n "export const search" -A 8 -B 4Length of output: 610
🏁 Script executed:
#!/bin/bash
# Show the full definition of the `search` query to inspect its return
sed -n '80,140p' convex/chats/queries.tsLength of output: 457
Enforce explicit typed fallback for searchResults
Verified that api.chats.queries.search returns Array<Doc<"chats">>, so we should use a nullish-coalescing fallback and cast to that type to keep consumers strictly typed.
Locations to update:
- src/hooks/chats/use-chats.ts (in the object returned by your hook)
Suggested diff:
return {
- searchResults: searchResults || [],
+ searchResults: (searchResults ?? []) as Array<Doc<"chats">>,
isSearching,
isLoadingSearch,
isSearchError,
searchError,
};📝 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.
| isSearching, | |
| isLoadingSearch, | |
| isSearchError, | |
| searchError, | |
| }; | |
| return { | |
| searchResults: (searchResults ?? []) as Array<Doc<"chats">>, | |
| isSearching, | |
| isLoadingSearch, | |
| isSearchError, | |
| searchError, | |
| }; |
🤖 Prompt for AI Agents
In src/hooks/chats/use-chats.ts around lines 155 to 159, the returned object
from the hook includes searchResults without an explicit typed fallback. Fix
this by applying a nullish-coalescing operator to searchResults and casting it
to Array<Doc<"chats">> to ensure strict typing for consumers. Update the
returned object to include searchResults ?? [] as Array<Doc<"chats">>.
shivamhwp
changed the title
- Removed redundant loading and error handling logic from DocumentDialog and other components. - Consolidated document download and opening functions for better clarity and maintainability. - Enhanced user feedback during document loading and error states across various chat components. - Updated LoadingSpinner and ErrorState components for consistent usage throughout the application.
Deploying zerobsv2 with
|
| Latest commit: |
b5adc1c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e327fed4.zerobsv2.pages.dev |
| Branch Preview URL: | https://feature-ux-improvements.zerobsv2.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
src/routes/settings/apiKeys.tsx (2)
245-258: Add error handling to toggle/remove mutationsThese mutations currently assume success. Failures will silently do nothing beyond server logs, which contradicts the PR goal of improved error handling.
Wrap both in try/catch and surface failures via toasts:
const handleToggle = async (config: ApiKeyConfig, enabled: boolean) => { - await updateApiKey({ - key: config.key, - enabled, - }); - toast.success( - `${config.title} ${enabled ? "enabled" : "disabled"} successfully` - ); + try { + await updateApiKey({ + key: config.key, + enabled, + }); + toast.success( + `${config.title} ${enabled ? "enabled" : "disabled"} successfully` + ); + } catch (error) { + toast.error(`Failed to ${enabled ? "enable" : "disable"} ${config.title}`); + console.error(error); + } }; const handleRemove = async (config: ApiKeyConfig) => { - await removeApiKey({ key: config.key }); - toast.success(`${config.title} removed successfully`); + try { + await removeApiKey({ key: config.key }); + toast.success(`${config.title} removed successfully`); + } catch (error) { + toast.error(`Failed to remove ${config.title}`); + console.error(error); + } };
154-169: Align UI with the existing OPENAI_API_KEYThe backend and
convex/langchain/models.tsexclusively use"OPENAI_API_KEY". To avoid user confusion and ensure the correct key is stored, update the UI copy and icon to reference OpenAI (Option A).• Keep
key: "OPENAI_API_KEY"unchanged.
• Update insrc/routes/settings/apiKeys.tsx:
- Change
titlefrom"OpenRouter API Key"to"OpenAI API Key".- Point the link at
https://platform.openai.com/api-keysand link text to “OpenAI”.- Swap the icon from
<Icons.OpenRouter />to<Icons.OpenAI />.Suggested diff:
@@ src/routes/settings/apiKeys.tsx - title: "OpenRouter API Key", + title: "OpenAI API Key", description: ( <> - Required for GPT models and AI-powered features. Get your API key from{" "} - <ExternalLink href="https://openrouter.ai/settings/keys"> - OpenRouter - </ExternalLink> + Required for GPT models and AI-powered features. Get your API key from{" "} + <ExternalLink href="https://platform.openai.com/api-keys"> + OpenAI + </ExternalLink> </> ), - icon: <Icons.OpenRouter />, + icon: <Icons.OpenAI />,src/routes/__root.tsx (1)
105-111: Use the providedopenvalue inonOpenChangeto avoid stale stateThe
SidebarProvider’sonOpenChangecallback supplies the next open state, so toggling via!sidebarOpencan capture a stale value. Update to pass the new value directly:• File:
src/routes/__root.tsx
Lines: 105–111- <SidebarProvider - className="…" - open={sidebarOpen} - onOpenChange={() => { - setSidebarOpen(!sidebarOpen); - }} - > + <SidebarProvider + className="…" + open={sidebarOpen} + onOpenChange={(open: boolean) => { + setSidebarOpen(open); + }} + >
♻️ Duplicate comments (1)
src/routes/__root.tsx (1)
76-83: Resolved prior issue: state updates moved out of renderThis addresses the earlier feedback about mutating state during render on settings routes. Thanks for fixing it.
🧹 Nitpick comments (4)
src/routes/settings/apiKeys.tsx (3)
27-31: Lazy-load favicons and mark as decorative to avoid redundant screen reader outputThese provider icons are purely decorative (the provider name is rendered right next to them). To improve a11y and slightly reduce network/paint cost, consider lazy-loading and using empty alt text (or aria-hidden). Optionally, suppress the Referer header for privacy with referrerPolicy.
Apply this diff to each img tag:
- <img - src="https://www.google.com/s2/favicons?sz=256&domain_url=https%3A%2F%2Fopenai.com%2F" - alt="OpenAI" - className="w-4 h-4 dark:invert" - /> + <img + src="https://www.google.com/s2/favicons?sz=256&domain_url=https%3A%2F%2Fopenai.com%2F" + alt="" + aria-hidden="true" + loading="lazy" + decoding="async" + referrerPolicy="no-referrer" + className="w-4 h-4 dark:invert" + />Repeat similarly for Google, Exa, and OpenRouter icons (keeping provider-specific classes intact).
Also applies to: 36-40, 45-49, 54-58
280-285: Disable toggle when no key value is setPrevent enabling a provider with no stored key to avoid backend errors and confusing UX.
<Switch id={`toggle-${config.key}`} checked={isEnabled} onCheckedChange={(checked) => handleToggle(config, checked)} + disabled={!existingKey?.value} />
131-141: Ensure masked key truncates properly and doesn’t blow layoutThe mask can be very long and may overflow. Add min-w-0 to the flex child and apply truncate to the value span for reliable ellipsis in flex layouts.
- <div className="flex items-center justify-between p-3 bg-input/80 rounded-md"> - <div className="flex items-center gap-1 truncate"> + <div className="flex items-center justify-between p-3 bg-input/80 rounded-md"> + <div className="flex items-center gap-1 min-w-0"> <span className="text-sm font-medium">Current Value:</span> - <span className="text-sm font-mono"> + <span className="text-sm font-mono truncate"> {config.isPassword ? maskKey(existingKey.value) : existingKey.value} </span> </div>src/routes/__root.tsx (1)
76-83: Small polish: tighten effect dependencies and callsYou can remove sidebarOpen from deps and set both states unconditionally when entering settings. This avoids an extra effect run caused by toggling sidebarOpen to false.
- useEffect(() => { - if (!isSettingsRoute) return; - sidebarOpen && setSidebarOpen(false); - setResizePanelOpen(false); - }, [isSettingsRoute, sidebarOpen, setSidebarOpen, setResizePanelOpen]); + useEffect(() => { + if (!isSettingsRoute) return; + setSidebarOpen(false); + setResizePanelOpen(false); + }, [isSettingsRoute, setSidebarOpen, setResizePanelOpen]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/components/chat/input/toolbar/index.tsx(3 hunks)src/components/chat/panels/projects/details.tsx(3 hunks)src/components/chat/panels/projects/document-list.tsx(5 hunks)src/routes/__root.tsx(3 hunks)src/routes/chat.$chatId.lazy.tsx(3 hunks)src/routes/settings/apiKeys.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/chat/input/toolbar/index.tsx
- src/components/chat/panels/projects/details.tsx
- src/components/chat/panels/projects/document-list.tsx
- src/routes/chat.$chatId.lazy.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/routes/__root.tsx (4)
src/routes/index.tsx (1)
Route(3-5)src/store/chatStore.ts (1)
resizePanelOpenAtom(36-36)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/app-sidebar/index.tsx (1)
AppSidebar(35-246)
🔇 Additional comments (6)
src/routes/__root.tsx (6)
9-9: Adopted shared LoadingSpinnerGood move to standardize the loading UI.
12-12: Prepare store for right panel visibility controlImporting resizePanelOpenAtom here aligns the root route with panel state management.
17-17: Import useEffect to handle route-driven UI side-effectsRequired for the settings route effect added below.
25-38: Head metadata formatting/branding changes look fineSwitching to double quotes and updating the title to "0bs" is consistent and non-functional otherwise.
Also applies to: 42-52, 56-59, 63-63
87-87: LGTM on spinner usageStandardized spinner with size class keeps the loading screen consistent with the rest of the app.
112-118: Hide AppSidebar on settings routesConditional rendering here matches the effect that closes these panels; TopNav remains visible. Looks correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (4)
src/hooks/chats/use-documents.ts (1)
111-118: Potential crash if chat.documents is undefinedSpreading possibly-undefined chat.documents will throw. Default to [] to be safe.
- await updateChatMutation({ - chatId, - updates: { - documents: [...chat.documents, ...documentIds], - }, - }); + await updateChatMutation({ + chatId, + updates: { + documents: [...(chat.documents ?? []), ...documentIds], + }, + });src/components/chat/input/document-list.tsx (1)
105-121: Fix: Loading/error branches are unreachable due to early return orderingThe early return on Line 105 exits before loading/error UI, so users never see the spinner or error state. Reorder checks and remove the duplicate empty check.
- if (!documents?.length) return null; - - const selectedModel = models.find((m) => m.model_name === model); - const modalities = selectedModel?.modalities; - - if (!documents?.length) return null; - - if (isLoadingDocuments) { - return ( - <div className="flex items-center justify-start p-2"> - <div className="text-sm text-muted-foreground flex items-center gap-2"> - <LoadingSpinner sizeClassName="h-4 w-4" /> - Loading attached documents... - </div> - </div> - ); - } - - if (isDocumentsError || documentsError) { + if (isLoadingDocuments) { + return ( + <div className="flex items-center justify-start p-2"> + <div className="text-sm text-muted-foreground flex items-center gap-2"> + <LoadingSpinner sizeClassName="h-4 w-4" /> + Loading attached documents... + </div> + </div> + ); + } + + if (isDocumentsError || documentsError) { return ( <div className="flex items-center justify-start p-2"> <ErrorState title="Error loading documents" showDescription={false} className="p-2" density="compact" /> </div> ); } + + if (!documents?.length) return null; + + const selectedModel = models.find((m) => m.model_name === model); + const modalities = selectedModel?.modalities;src/components/chat/messages/user-message.tsx (1)
25-31: Type safety: use Id<'documents'> for fileId instead of stringAligns with project guidelines and removes the need for unsafe casts.
-const DocumentButton = ({ - fileId, - setDocumentDialogOpen, -}: { - fileId: string; - setDocumentDialogOpen: (id: Id<"documents"> | undefined) => void; -}) => { +const DocumentButton = ({ + fileId, + setDocumentDialogOpen, +}: { + fileId: Id<"documents">; + setDocumentDialogOpen: (id: Id<"documents"> | undefined) => void; +}) => { @@ } = useQuery({ ...convexQuery(api.documents.queries.get, { - documentId: fileId as Id<"documents">, + documentId: fileId, }), }); @@ - <Button - className="py-6 cursor-pointer" - onClick={() => setDocumentDialogOpen(fileId as Id<"documents">)} - > + <Button + className="py-6 cursor-pointer" + onClick={() => setDocumentDialogOpen(fileId)} + >Also applies to: 38-41, 68-73
src/components/app-sidebar/index.tsx (1)
232-237: Fix empty-state condition so “No matching chats found” can showThe current guard
!isSearchingprevents the search empty-state from rendering. Let the conditional depend only on the list being empty and choose the label based on isSearching.- {displayHistoryChats.length === 0 && !isSearching && ( - <div className="text-sm text-muted-foreground text-center py-2"> - {isSearching - ? "No matching chats found" - : "No chat history"} - </div> - )} + {displayHistoryChats.length === 0 && ( + <div className="text-sm text-muted-foreground text-center py-2"> + {isSearching ? "No matching chats found" : "No chat history"} + </div> + )}
♻️ Duplicate comments (3)
src/components/topnav.tsx (1)
44-46: Rules of Hooks violation: conditional return before hooksReturning
<Navigate />conditionally means subsequent hooks (useEffect,useParams) aren’t called on that render. This breaks hook ordering between renders.Remove the early return:
- if (isErrorUser) { - return <Navigate to="/auth" />; - }Add a side-effect redirect instead (place after hooks are declared), ensuring you don’t loop on the auth route and clearing stale user state:
useEffect(() => { if (!isErrorUser) return; // Optional: Only redirect on Unauthorized errors (see earlier comment) // if (!isUnauthorized(userError)) return; // Clear any stale user data before redirecting setUser(undefined); // Avoid redirect loops if TopNav also renders on the auth route if (location.pathname !== "/auth") { navigate({ to: "/auth" }); } // Optionally include search/hash as needed }, [isErrorUser, navigate, location.pathname, setUser /*, userError*/]);While here, update the dependency array of the user-sync effect to include
setUser(stable but satisfies lint):useEffect(() => { if (user) setUser(user); }, [user, setUser]); ```<!-- review_comment_end --> </blockquote></details> <details> <summary>src/components/chat/messages/user-message.tsx (2)</summary><blockquote> `136-161`: **Accessibility: Make document chip and remove button keyboard- and screen reader-accessible** Same issue previously flagged: clickable Badge without role/tabIndex and icon-only remove without label. ```diff - <Badge + <Badge key={doc._id} variant="default" - className="group/document flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center" + className="group/document flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center focus:outline-none focus-visible:ring-2 focus-visible:ring-offset-2" + role="button" + tabIndex={0} + aria-label={`Preview document ${doc.name}`} onClick={() => handlePreview(doc._id)} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handlePreview(doc._id); + } + }} > <div className="relative h-4 w-4"> <Icon - className={`${IconClassName} h-4 w-4 group-hover/document:opacity-0 transition duration-300`} + className={`${IconClassName} h-4 w-4 group-hover/document:opacity-0 group-focus-within/document:opacity-0 transition duration-300`} /> <Button variant="ghost" size="icon" - className="absolute inset-0 h-4 w-4 opacity-0 group-hover/document:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300" + className="absolute inset-0 h-4 w-4 opacity-0 group-hover/document:opacity-100 group-focus-within/document:opacity-100 focus:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300" + aria-label={`Remove document ${doc.name}`} + title="Remove" onClick={(e) => { e.stopPropagation(); onRemove(doc._id); }} > <XIcon className="w-3 h-3" /> </Button> </div>
265-265: Fix layout regression: min-w-[100vw] causes overflowReplace with responsive width that respects parent constraints.
- className="bg-transparent resize-none border-none min-w-[100vw] max-w-full ring-0 focus-visible:ring-0 focus-visible:ring-offset-0 outline-none focus-visible:outline-none text-base" + className="bg-transparent resize-none border-none w-full min-w-0 max-w-full ring-0 focus-visible:ring-0 focus-visible:ring-offset-0 outline-none focus-visible:outline-none text-base"
🧹 Nitpick comments (11)
src/components/topnav.tsx (3)
14-19: Remove<Navigate />import if switching to imperative navigationIf you move the auth-redirect into an effect (recommended below),
<Navigate />becomes unused. Drop it to prevent lint failures.import { - Navigate, useLocation, useNavigate, useParams, } from "@tanstack/react-router";
36-40: Gate auth redirect on Unauthorized errors; also captureerrorfrom the queryRight now every fetch error redirects to /auth. That can misroute on transient/network errors. Capture the
errorobject so you can conditionally redirect only for 401/Unauthorized. Consider also tuning retry behavior.Apply within this block:
const { data: user, isError: isErrorUser, + error: userError, isLoading: isLoadingUser, } = useQuery({ ...convexQuery(api.auth.getUser, {}), });Then, in an effect (see separate comment), redirect only when
userErrorindicates Unauthorized. Please confirm the error shape from convex + tanstack-query in your setup (e.g., HTTP status, code, or message) before gating. If Unauthorized isn’t distinguishable, at least setretry: falseon this query to avoid thrashing on auth failures.Verification suggestions:
- Check what
userErrorlooks like for an expired session vs. a network outage.- If it exposes an HTTP status, gate on 401 and keep the TopNav rendered otherwise (you could show an inline error or nothing).
If you want, I can wire the conditional retry function once you confirm the error shape.
102-115: Add an aria-label to the Settings button for better a11yModeToggle already has an sr-only label; mirror that for Settings to improve screen reader support.
{!resizePanelOpen ? ( <Button variant="ghost" size="icon" className="cursor-pointer" + aria-label="Open settings" onClick={() => { navigate({ to: "/settings/profile" }); }} > <Settings2Icon className="h-6 w-6" /> </Button> ) : null}src/hooks/chats/use-documents.ts (2)
35-41: Add onError handling for the mutation to provide user feedbackSilent failures here can leave UI state stale. Add onError to surface issues to the user.
- updateChatInputMutation({ - chatId: chatId, - updates: { - documents: filteredDocuments, - }, - }); + updateChatInputMutation( + { + chatId, + updates: { + documents: filteredDocuments, + }, + }, + { + onError: (err) => { + toast("Failed to remove document", { + description: String(err), + }); + }, + }, + );
73-96: Optional: parallelize uploads with bounded concurrencyUploads are fully sequential; for many files this increases latency. Consider parallelizing with a small concurrency limit (e.g., 3–5) to reduce total time while avoiding server overload.
If you want, I can provide a throttled Promise pool snippet tailored to this hook.
Also applies to: 98-108
src/components/chat/input/document-list.tsx (1)
45-66: Accessibility: Make badge keyboard-focusable and label the remove buttonThe badge is clickable but not keyboard accessible; the icon-only remove button lacks an aria-label and isn’t focus-visible.
- <Badge - variant="default" - className="group flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center" - onClick={handlePreview} - > + <Badge + variant="default" + className="group/document flex gap-1.5 py-1 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 rounded-md transition duration-300 items-center justify-center focus:outline-none focus-visible:ring-2 focus-visible:ring-offset-2" + role="button" + tabIndex={0} + aria-label={`Preview document ${doc.name}`} + onClick={handlePreview} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handlePreview(); + } + }} + > <div className="relative h-4 w-4"> <Icon - className={`${IconClassName} h-4 w-4 group-hover:opacity-0 transition duration-300`} + className={`${IconClassName} h-4 w-4 group-hover/document:opacity-0 group-focus-within/document:opacity-0 transition duration-300`} /> <Button variant="ghost" size="icon" - className="absolute inset-0 h-4 w-4 opacity-0 group-hover:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300" + className="absolute inset-0 h-4 w-4 opacity-0 group-hover/document:opacity-100 group-focus-within/document:opacity-100 focus:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300" + aria-label={`Remove document ${doc.name}`} + title="Remove" onClick={handleRemove} > <XIcon className="w-3 h-3" /> </Button> </div>Also applies to: 50-61
src/components/document-dialog.tsx (2)
49-91: Optional: guard against setting state after unmount or document switchThe async preview loader can update state after the dialog closes or the doc changes. Add a cancellation flag.
useEffect(() => { - setPreviewUrl(null); + let cancelled = false; + setPreviewUrl(null); const loadPreviewUrl = async () => { if (!document) return; try { switch (tag) { @@ - setPreviewUrl(url); + if (!cancelled) setPreviewUrl(url); break; } @@ - setPreviewUrl(document.key as string); + if (!cancelled) setPreviewUrl(document.key as string); break; } @@ - setPreviewUrl(`https://www.youtube.com/embed/${document.key}`); + if (!cancelled) + setPreviewUrl(`https://www.youtube.com/embed/${document.key}`); break; } default: if (["file", "text", "github"].includes(document.type)) { const url = await generateDownloadUrl({ documentId: document._id!, }); - setPreviewUrl(url); + if (!cancelled) setPreviewUrl(url); break; } else { - setPreviewUrl(document.key as string); + if (!cancelled) setPreviewUrl(document.key as string); } break; } } catch (e) { - setPreviewUrl(null); + if (!cancelled) setPreviewUrl(null); } }; loadPreviewUrl(); - }, [document, tag, generateDownloadUrl]); + return () => { + cancelled = true; + }; + }, [document, tag, generateDownloadUrl]);
122-127: Nit: use sizeClassName for spinner sizing for consistencyLoadingSpinner supports sizeClassName specifically for icon sizing.
- <LoadingSpinner className="h-8 w-8" /> + <LoadingSpinner sizeClassName="h-8 w-8" />src/components/chat/messages/user-message.tsx (1)
110-118: Fix typo: double space in error titleMinor polish.
- <ErrorState - title="Error loading attached documents" - error={documentsError} - /> + <ErrorState title="Error loading attached documents" error={documentsError} />src/components/app-sidebar/index.tsx (2)
79-86: Optional: prevent repeated loadMore triggersIntersectionObserver will call loadMore each time the sentinel re-enters view. Unobserve after the first trigger and re-observe once status flips back to "CanLoadMore".
const observer = new IntersectionObserver( (entries) => { const [entry] = entries; if (entry.isIntersecting) { - loadMore(15); + observer.unobserve(loadMoreElement); + loadMore(15); } },
155-163: Nit: add accessible label to the clear-search buttonHelps screen readers discover the control.
<Button variant="ghost" size="icon" onClick={() => setSearchQuery("")} className="flex items-center justify-center size-5" tabIndex={-1} + aria-label="Clear search" + title="Clear" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
services/mcps(1 hunks)src/components/app-sidebar/index.tsx(8 hunks)src/components/chat/input/document-list.tsx(3 hunks)src/components/chat/input/toolbar/index.tsx(3 hunks)src/components/chat/messages/user-message.tsx(7 hunks)src/components/chat/panels/projects/details.tsx(3 hunks)src/components/chat/panels/projects/document-list.tsx(5 hunks)src/components/document-dialog.tsx(5 hunks)src/components/topnav.tsx(4 hunks)src/components/ui/accordion.tsx(2 hunks)src/hooks/chats/use-chats.ts(2 hunks)src/hooks/chats/use-documents.ts(1 hunks)src/hooks/chats/use-mcp.ts(2 hunks)src/hooks/chats/use-messages.ts(2 hunks)src/routes/chat.$chatId.lazy.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/mcps
🚧 Files skipped from review as they are similar to previous changes (8)
- src/hooks/chats/use-messages.ts
- src/hooks/chats/use-chats.ts
- src/components/chat/panels/projects/details.tsx
- src/components/chat/input/toolbar/index.tsx
- src/routes/chat.$chatId.lazy.tsx
- src/components/ui/accordion.tsx
- src/hooks/chats/use-mcp.ts
- src/components/chat/panels/projects/document-list.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/convex_rules.mdc)
**/*.ts: You can use the helper TypeScript type Id imported from './_generated/dataModel' to get the type of the id for a given table.
If you need to define a Record, provide the correct key and value types (e.g., Record<Id<'users'>, string>).
Be strict with types, particularly around ids of documents. Use Id<'table'> rather than string for document ids.
Always use as const for string literals in discriminated union types.
When using the Array type, always define arrays as const array: Array = [...];
When using the Record type, always define records as const record: Record<KeyType, ValueType> = {...};
Files:
src/hooks/chats/use-documents.ts
🧠 Learnings (1)
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Use the api object from convex/_generated/api.ts to call public functions and the internal object to call internal functions.
Applied to files:
src/components/chat/input/document-list.tsx
🧬 Code Graph Analysis (5)
src/components/chat/messages/user-message.tsx (4)
convex/_generated/dataModel.d.ts (1)
Id(48-49)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/ui/error-state.tsx (1)
ErrorState(122-122)src/components/ui/autosize-textarea.tsx (1)
AutosizeTextarea(65-119)
src/components/app-sidebar/index.tsx (5)
src/hooks/chats/use-chats.ts (1)
useSearchChats(125-160)src/components/ui/sidebar.tsx (4)
SidebarContent(703-703)SidebarGroup(705-705)SidebarGroupLabel(708-708)SidebarGroupContent(707-707)src/components/ui/accordion.tsx (2)
AccordionTrigger(234-234)AccordionContent(235-235)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/ui/error-state.tsx (1)
ErrorState(122-122)
src/components/document-dialog.tsx (3)
convex/documents/mutations.ts (1)
generateDownloadUrl(22-39)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/ui/error-state.tsx (1)
ErrorState(122-122)
src/components/topnav.tsx (2)
src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/theme-provider.tsx (1)
ModeToggle(69-86)
src/components/chat/input/document-list.tsx (3)
src/store/chatStore.ts (1)
documentDialogOpenAtom(43-45)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/ui/error-state.tsx (1)
ErrorState(122-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
src/components/topnav.tsx (3)
24-24: LGTM: standardized spinner importConsistent with the new UI primitives. No issues.
59-59: LGTM: shortcut comment clarityComment accurately describes the scope of the shortcut. No code issues.
97-101: LGTM: unobtrusive loading indicatorCompact spinner placement and sizing align with the design system. Accessible via
aria-labelon the icon.src/hooks/chats/use-documents.ts (1)
12-17: Conditional query gating via convexQuery "skip" looks correctPassing "skip" when chatId is "new" cleanly disables the chat query. No issues here.
src/components/document-dialog.tsx (1)
224-235: Security review: sandbox flags for external iframesThe combination of allow-same-origin and allow-scripts grants substantial capability to embedded content. If you don’t need script execution, consider dropping allow-scripts; if you keep scripts, consider removing allow-same-origin to enforce opaque origin. Verify the UX trade-offs.
Would you like me to audit typical URLs and recommend the safest viable sandbox set for your use cases?
…r and useDocuments hooks - Removed unnecessary line breaks and adjusted formatting for better readability. - Enhanced observer logic in AppSidebar to unobserve the loadMoreElement when it is intersecting. - Updated useRemoveDocument and useUploadDocuments hooks for consistent code style.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/app-sidebar/index.tsx (1)
233-239: Fix unreachable “No matching chats found” message (wrong condition).The empty-state for search results is guarded by
!isSearching, so “No matching chats found” will never render during a search. This contradicts both the intent and the AI summary, and it suppresses useful feedback to users.Apply this diff to show:
- “No matching chats found” only when searching, not loading, not error, and both pinned and history search lists are empty.
- “No chat history” only when not searching and history is empty.
- {displayHistoryChats.length === 0 && !isSearching && ( - <div className="text-sm text-muted-foreground text-center py-2"> - {isSearching - ? "No matching chats found" - : "No chat history"} - </div> - )} + {isSearching && + !isLoadingSearch && + !isSearchError && + displayHistoryChats.length === 0 && + displayPinnedChats.length === 0 && ( + <div className="text-sm text-muted-foreground text-center py-2"> + No matching chats found + </div> + )} + {!isSearching && displayHistoryChats.length === 0 && ( + <div className="text-sm text-muted-foreground text-center py-2"> + No chat history + </div> + )}
🧹 Nitpick comments (6)
src/components/app-sidebar/index.tsx (6)
141-154: Add accessible name to the search input and drop redundant inline style.Improve a11y by providing an explicit accessible name; the inline transparent background is redundant with the Tailwind classes.
<Input placeholder="Search chats" className="flex-1 bg-transparent border-none focus:ring-0 focus:ring-offset-0 focus-visible:ring-0 focus-visible:ring-offset-0 outline-none text-base px-0" type="text" + aria-label="Search chats" value={searchQuery} onChange={(e) => setSearchQuery(e.target.value)} onKeyDown={(e) => { if (e.key === "Escape") { setSearchQuery(""); e.currentTarget.blur(); } }} - style={{ backgroundColor: "transparent" }} />
156-164: Add accessible name to the clear-search button.Icon-only controls need an accessible label for screen readers.
<Button variant="ghost" size="icon" onClick={() => setSearchQuery("")} className="flex items-center justify-center size-5" tabIndex={-1} + aria-label="Clear search" + title="Clear search" > <XIcon className="w-4 h-4" /> </Button>
181-188: AccordionTrigger without children: add an accessible label (or make the whole header the trigger).
AccordionTriggeris rendered without children, so the control has no accessible name and only the chevron is clickable. At minimum, add an aria-label. Alternatively, consider making the entire header (icon + “Pinned”) the trigger for a larger tap target.- <AccordionTrigger className="cursor-pointer p-0 mr-2" /> + <AccordionTrigger + className="cursor-pointer p-0 mr-2" + aria-label="Toggle pinned chats" + />
171-176: Avoid mixing controlled and uncontrolled Accordion props.
valuemakes the Accordion controlled;defaultValueis ignored and can be removed for clarity.<Accordion type="single" collapsible - defaultValue="pinned" className="w-full" value={pinnedChatsAccordionOpen ? "pinned" : ""}
84-84: Replace magic number 15 with a named constant.Removes duplication, clarifies intent, and centralizes changes to page size.
- loadMore(15); + loadMore(PAGE_SIZE);- onClick={() => loadMore(15)} + onClick={() => loadMore(PAGE_SIZE)}Add this near the top of the module (e.g., after imports):
const PAGE_SIZE = 15;Also applies to: 228-228
218-221: Optionally surface error details to users.Consider displaying the underlying error message (e.g.,
searchError?.message) inErrorStateto aid troubleshooting. If you adopt this, also destructuresearchErrorfromuseSearchChats()above.Would you like me to wire this up and ensure the ErrorState component’s API is respected?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/components/app-sidebar/index.tsx(9 hunks)src/hooks/chats/use-documents.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/chats/use-documents.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/app-sidebar/index.tsx (6)
src/hooks/chats/use-chats.ts (1)
useSearchChats(125-160)src/store/chatStore.ts (1)
pinnedChatsAccordionOpenAtom(56-59)src/components/ui/sidebar.tsx (4)
SidebarContent(703-703)SidebarGroup(705-705)SidebarGroupLabel(708-708)SidebarGroupContent(707-707)src/components/ui/accordion.tsx (2)
AccordionTrigger(234-234)AccordionContent(235-235)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/ui/error-state.tsx (1)
ErrorState(122-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
src/components/app-sidebar/index.tsx (2)
79-86: Good call: unobserving the target prevents duplicate auto-loads.Unobserving the load-more element before invoking
loadMore(15)avoids duplicate fetches caused by repeated intersections. Dependency handling and cleanup look correct.Also applies to: 91-91
23-24: LGTM: Consistent loading and error primitives integrated.Using shared
LoadingSpinnerandErrorStatealigns with the PR’s goal of consistent UX across components.
- Removed unnecessary icons from the Pinned and Previous Chats sections in the AppSidebar component for a cleaner UI. - Adjusted the SidebarGroupLabel to streamline the display of section titles.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ui/accordion.tsx (1)
129-134: Add type='button' and wire up ARIA to make the accordion accessible and form-safe.
- Without
type="button", the trigger can submit a surrounding form.- Add
aria-expanded/aria-controlson the trigger, andid/aria-labelledbyon the content for screen readers.Apply this diff:
@@ - const isOpen = expandedItems.has(accordionItem.value); + const isOpen = expandedItems.has(accordionItem.value); + const triggerId = `accordion-trigger-${accordionItem.value}`; + const contentId = `accordion-content-${accordionItem.value}`; @@ - <motion.button + <motion.button data-slot="accordion-trigger" className={cn( "focus-visible:border-ring focus-visible:ring-ring/50 flex flex-1 items-start justify-between gap-4 rounded-md py-4 text-left text-sm font-medium transition-all outline-none hover:underline focus-visible:ring-[3px] disabled:pointer-events-none disabled:opacity-50", className )} data-state={isOpen ? "open" : "closed"} onClick={handleClick} + id={triggerId} + aria-controls={contentId} + aria-expanded={isOpen} + type="button" {...props} > @@ - const isOpen = expandedItems.has(accordionItem.value); + const isOpen = expandedItems.has(accordionItem.value); + const triggerId = `accordion-trigger-${accordionItem.value}`; + const contentId = `accordion-content-${accordionItem.value}`; @@ - <motion.div + <motion.div data-slot="accordion-content" className="overflow-hidden text-sm" initial="collapsed" animate="open" exit="collapsed" + id={contentId} + role="region" + aria-labelledby={triggerId} variants={{ open: { opacity: 1, height: "auto" }, collapsed: { opacity: 0, height: 0 }, }}Also applies to: 139-147, 180-186, 185-199
src/components/chat/messages/index.tsx (1)
113-113: Include missing error-related flags in theuseMemodependency arrayThe
mainContentmemo relies onisError,error,isStreamError, andstreamErrorto decide what to render. These four flags are not listed in the current dependency array, so the memo won’t update when they change—leading to stale or “frozen” error UI. A quick script confirms all four are missing.• File:
src/components/chat/messages/index.tsx(around line 113)
• Hook:useMemo(… , [/* dependencies */])formainContentApply this diff to ensure the memo updates correctly:
- }, [isLoading, isEmpty, streamStatus, chatId, userLoadable]); + }, [ + isLoading, + isEmpty, + isError, + error, + isStreamError, + streamError, + streamStatus, + chatId, + userLoadable, + ]);
♻️ Duplicate comments (2)
src/routes/__root.tsx (1)
79-87: Good fix: moved layout state updates out of render and into an effect.This resolves the earlier “state mutation during render” issue for settings routes and prevents render loops. Nice job.
src/components/topnav.tsx (1)
18-23: Fix conditional hook usage caused by early return.Returning before subsequent hooks (e.g., useEffect below) violates React’s rules of hooks. Navigate via an effect instead and keep all hooks unconditional. This also addresses the same concern noted previously.
Apply these diffs:
@@ -import { - Navigate, - useLocation, - useNavigate, - useParams, -} from "@tanstack/react-router"; +import { useLocation, useNavigate, useParams } from "@tanstack/react-router"; @@ - if (isErrorUser) { - return <Navigate to="/auth" />; - } + useEffect(() => { + if (isErrorUser) { + navigate({ to: "/auth" }); + } + }, [isErrorUser, navigate]);#!/bin/bash # Audit for other early <Navigate /> returns that may conditionally skip hooks. # Shows surrounding context to verify ordering. rg -nP -C3 --type=tsx '\breturn\s*<Navigate\b'Also applies to: 49-51
🧹 Nitpick comments (22)
src/components/ui/accordion.tsx (6)
108-114: Broaden Trigger props to accept native button attributes (disabled, aria-*, id).You already spread
...propsonto a button but the type doesn’t allow native attributes, which can break consumers (e.g.,disabled). Extend from the intrinsic button props.Apply this diff:
-interface AccordionTriggerProps { - className?: string; - children?: React.ReactNode; - onClick?: React.MouseEventHandler<HTMLButtonElement>; - showIcon?: boolean; -} +interface AccordionTriggerProps + extends React.ButtonHTMLAttributes<HTMLButtonElement> { + showIcon?: boolean; +}
29-37: Let Accordion pass through div attributes safely.
AccordionPropsdoesn’t extend HTML div attributes but you spread...propsto a div. Extend from the intrinsic div props to avoid type holes and enable id/aria/class usage.Apply this diff:
-interface AccordionProps { +interface AccordionProps extends React.HTMLAttributes<HTMLDivElement> { type?: "single" | "multiple"; collapsible?: boolean; value?: string | string[]; defaultValue?: string | string[]; onValueChange?: (value: string | string[]) => void; - className?: string; - children?: React.ReactNode; + className?: string; // optional: redundant, but keep explicit if you prefer + children?: React.ReactNode; // ditto }
102-107: Same pass-through typing for AccordionItem.
AccordionItemEnhancedforwards...propsto a div. LetAccordionItemPropsextend HTML div attributes for correctness and flexibility.Apply this diff:
-interface AccordionItemProps { +interface AccordionItemProps extends React.HTMLAttributes<HTMLDivElement> { value: string; className?: string; children?: React.ReactNode; }
163-167: Same pass-through typing for AccordionContent.
AccordionContentspreads...propsto the animated div; extend from HTML div attributes to type that properly.Apply this diff:
-interface AccordionContentProps { +interface AccordionContentProps extends React.HTMLAttributes<HTMLDivElement> { className?: string; children?: React.ReactNode; }
150-151: Chevron rotation direction—confirm intended UX.Currently the chevron points down when open and up when closed (
isOpen ? 0 : 180). Many designs do the opposite. If you want “rotate up when open,” flip the values.Apply this diff if that’s the intended behavior:
- animate={{ rotate: isOpen ? 0 : 180 }} + animate={{ rotate: isOpen ? 180 : 0 }}
153-153: Trim trailing whitespace in className.Minor cleanup; there’s a trailing space at the end of the className value.
Apply this diff:
- className="items-center flex justify-center " + className="items-center flex justify-center"src/routes/__root.tsx (3)
110-116: Use the next open value from onOpenChange instead of toggling via captured state.Relying on the captured sidebarOpen can desync state if multiple updates fire quickly. Prefer the callback’s next open value.
Apply this diff:
- <SidebarProvider + <SidebarProvider className="font-sans h-svh relative before:content-[''] before:fixed before:inset-0 before:bg-[url('/images/noise.png')] before:opacity-50 before:pointer-events-none before:z-[-1]" open={sidebarOpen} - onOpenChange={() => { - setSidebarOpen(!sidebarOpen); - }} + onOpenChange={(open) => { + setSidebarOpen(open); + }} >
117-123: Centralize settings-route layout decisions (optional).TopNav hides itself via CSS on settings while Root always renders it. Consider conditionally rendering TopNav here (like AppSidebar) to avoid mounting work on settings pages and to keep route-specific layout logic in one place.
Potential adjustment:
- {!isSettingsRoute && <AppSidebar />} - <TopNav /> + {!isSettingsRoute && <AppSidebar />} + {!isSettingsRoute && <TopNav />}
74-78: Nit: hoist static publicRoutes outside the component.This avoids re-allocating the array every render and makes intent clearer. Also consider broadening the check if you introduce nested auth routes (e.g., startsWith("/auth")).
Would you like me to send a follow-up patch that hoists the constant and, if needed, adapts the check for nested auth paths?
src/components/topnav.tsx (4)
53-57: Add setUser to effect deps.The setter is stable, but including it clarifies intent and silences strict-lint configurations.
- }, [user]); + }, [user, setUser]);
41-47: Auth error handling nuance (optional).Redirecting on any query error may bounce users to /auth on transient network failures. If possible, gate on an “unauthorized”/auth-specific error code, otherwise surface a non-blocking ErrorState.
Want me to update this to inspect error types from convex/react-query (e.g., unauthorized vs network) and only redirect on auth failures?
113-125: Accessibility: add an aria-label to the Settings button.Screen reader users won’t get a name from the icon alone.
- <Button + <Button variant="ghost" size="icon" className="cursor-pointer" onClick={() => { navigate({ to: "/settings/profile" }); }} + aria-label="Open settings" > <Settings2Icon className="h-6 w-6" /> </Button>
85-88: Optional: avoid duplicate settings-route visibility logic.Root decides sidebar visibility for settings routes; TopNav also computes and hides itself. Consider relying on the root to conditionally render TopNav (or lifting a shared helper) to keep this concern in one place.
src/components/chat/input/document-list.tsx (5)
45-49: Improve a11y: title on Badge, aria-label on Remove, and keyboard-visible state.Add accessible labels and ensure the remove button becomes visible on keyboard focus, not only on hover.
@@ - <Badge + <Badge variant="default" className="group/badge flex gap-1.5 py-1.5 cursor-pointer bg-accent/65 hover:bg-accent/100 hover:text-accent-foreground text-accent-foreground/90 transition duration-300 items-center justify-center" + title={doc.name} onClick={handlePreview} > @@ - <Button + <Button variant="ghost" size="icon" - className="absolute inset-0 h-4 w-4 opacity-0 group-hover/badge:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300" + aria-label={`Remove document ${doc.name}`} + title={`Remove document ${doc.name}`} + className="absolute inset-0 h-4 w-4 opacity-0 group-hover/badge:opacity-100 group-focus-within/badge:opacity-100 focus-visible:opacity-100 hover:bg-muted-foreground/20 cursor-pointer transition duration-300 focus-visible:ring-1 focus-visible:ring-ring focus-visible:outline-none" onClick={handleRemove} > - <XIcon className="w-3 h-3" /> + <XIcon className="w-3 h-3" aria-hidden="true" /> </Button>Also applies to: 54-61, 60-60
115-118: Minor: Use LoadingSpinner’s label prop instead of a separate text node.Keeps semantics consistent and reduces DOM.
- <LoadingSpinner sizeClassName="h-4 w-4" /> - Loading attached documents... + <LoadingSpinner sizeClassName="h-4 w-4" label="Loading attached documents..." />
123-134: Simplify error condition and optionally surface the message.
isErroralready reflects presence oferror. Consider relying on it and optionally pass a compact description.- if (isDocumentsError || documentsError) { + if (isDocumentsError) { return ( <div className="flex items-center justify-start p-2"> <ErrorState title="Error loading documents" - showDescription={false} + description={documentsError instanceof Error ? documentsError.message : undefined} + showDescription={Boolean(documentsError)} className="p-2" density="compact" /> </div> ); }
12-13: Optional: centralize fetching via auseDocumentshook for consistency.For parity with the rest of the codebase and to avoid repeating query wiring, consider replacing the inline
useQueryblock with a dedicated hook (e.g., from@/hooks/chats/use-documents) that returns{ documents, isLoadingDocuments, isDocumentsError, documentsError }.-import { useRemoveDocument } from "@/hooks/chats/use-documents"; +import { useRemoveDocument, useDocuments } from "@/hooks/chats/use-documents"; @@ - } = useQuery({ - ...convexQuery(api.documents.queries.getMultiple, { documentIds }), - enabled: documentIds.length > 0, - }); + } = useDocuments(documentIds);Note: If you adopt this, retain the post-error empty-docs guard added in the previous comment.
Also applies to: 79-86
14-14: Tiny micro-optimization: memoize modalities lookup.Not critical, but avoids re-running
findon each render.-import React, { useCallback } from "react"; +import React, { useCallback, useMemo } from "react"; @@ - const selectedModel = models.find((m) => m.model_name === model); - const modalities = selectedModel?.modalities; + const modalities = useMemo( + () => models.find((m) => m.model_name === model)?.modalities, + [model] + );Also applies to: 107-109
src/components/chat/messages/index.tsx (4)
15-16: Destructured hook return looks good; consider clearer naming.The expanded shape matches the updated hook. Consider renaming
errortomessagesErrorlocally to avoid confusion withstreamErrorin the same scope.- const { isLoading, isEmpty, isError, error, isStreamError, streamError } = + const { isLoading, isEmpty, isError, error: messagesError, isStreamError, streamError } = useMessages({ chatId });And update usages accordingly (e.g.,
error={messagesError}).
43-46: Use LoadingSpinner’s label prop to avoid duplicate, misaligned text.Slight simplification and better a11y by keeping the label tied to the spinner component.
- <div className="flex items-center justify-center h-full"> - <LoadingSpinner /> - <div className="text-muted-foreground">Loading messages...</div> - </div> + <div className="flex items-center justify-center h-full"> + <LoadingSpinner label="Loading messages..." /> + </div>
50-62: Guard general error branch on isError only to prevent stale-object flicker.React Query ensures
erroris meaningful only whenisErroris true. Checkingerroralone can render the error state during refetches or intermediate states if a stale error object lingers.- if (isError || error) { + if (isError) { return ( <div className="flex items-center justify-center w-full h-full"> <ErrorState className="max-w-4xl" title="Failed to load messages" - error={error} + error={error} description="Please try again later." density="comfy" /> </div> ); }
64-77: Similarly, gate streaming error UI on isStreamError only.
streamErrorshould be treated as payload; the boolean flag should control the branch to avoid false positives if a stale value exists.- if (isStreamError || streamError) { + if (isStreamError) { return ( <div className="flex items-center justify-center w-full h-full"> <ErrorState className="max-w-4xl" density="comfy" description="Unable to load messages, this might be due to either a network issue or a server error." title="Error loading messages" error={streamError} showIcon={false} /> </div> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/components/app-sidebar/index.tsx(8 hunks)src/components/chat/input/document-list.tsx(3 hunks)src/components/chat/messages/index.tsx(3 hunks)src/components/chat/panels/projects/details.tsx(3 hunks)src/components/chat/panels/projects/list.tsx(3 hunks)src/components/topnav.tsx(4 hunks)src/components/ui/accordion.tsx(1 hunks)src/routes/__root.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/chat/panels/projects/details.tsx
- src/components/chat/panels/projects/list.tsx
- src/components/app-sidebar/index.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-01T07:34:38.459Z
Learnt from: CR
PR: 0bs-chat/zerobs#0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-08-01T07:34:38.459Z
Learning: Applies to convex/**/*.{ts,js} : Use the api object from convex/_generated/api.ts to call public functions and the internal object to call internal functions.
Applied to files:
src/components/chat/input/document-list.tsx
🧬 Code graph analysis (4)
src/routes/__root.tsx (3)
src/store/chatStore.ts (1)
resizePanelOpenAtom(36-36)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/app-sidebar/index.tsx (1)
AppSidebar(35-245)
src/components/chat/messages/index.tsx (3)
src/store/chatStore.ts (1)
userLoadableAtom(11-11)src/hooks/chats/use-messages.ts (1)
useMessages(20-66)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)
src/components/topnav.tsx (2)
src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/theme-provider.tsx (1)
ModeToggle(69-86)
src/components/chat/input/document-list.tsx (3)
src/store/chatStore.ts (1)
documentDialogOpenAtom(43-45)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/ui/error-state.tsx (1)
ErrorState(122-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
src/components/ui/accordion.tsx (1)
115-122: Prop reordering is safe; default remains explicit.Moving
showIconto the front withshowIcon = truekeeps behavior clear and non-breaking for named props. No issues spotted.src/routes/__root.tsx (2)
9-9: Spinner migration looks good.Adopting the shared LoadingSpinner here aligns the route’s loading UI with the rest of the app.
92-92: Minor UI: spinner sizing reads well against full-screen container.The sizeClassName choice keeps the loading state visually balanced. No action needed.
src/components/topnav.tsx (1)
107-111: Nice: lightweight loading affordance in the control cluster.The compact LoadingSpinner integrates cleanly without shifting layout. No changes needed.
src/components/chat/input/document-list.tsx (2)
7-8: LGTM: central loading/error UI adoption.Good use of the shared
LoadingSpinnerandErrorStatecomponents for consistent UX.
85-86: Good: follows Convex rule to use generated api object.Query is properly issued via
api.documents.queries.getMultiple. Matches the team guideline in retrieved learnings.src/components/chat/messages/index.tsx (2)
2-2: Nice addition: centralizing error UI via ErrorState.Import looks correct and aligns with the PR goal of consistent error handling across components.
11-11: Loading spinner integration is consistent with shared UI.Good reuse of the shared LoadingSpinner component.
| const { | ||
| data: documents, | ||
| isLoading: isLoadingDocuments, | ||
| isError: isDocumentsError, | ||
| error: documentsError, | ||
| } = useQuery({ | ||
| ...convexQuery(api.documents.queries.getMultiple, { documentIds }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix early null-return that makes Loading/Error UI unreachable; also gate the query when no attachments.
if (!documents?.length) return null; at Line 105 short-circuits before checking isLoadingDocuments/isDocumentsError, so the spinner/error never render. There’s also a duplicate guard at Lines 110-111. Move the empty-docs guard to after the loading/error blocks and add enabled to the query so we don’t fetch (or show a spinner) when documentIds is empty.
Apply:
@@
- } = useQuery({
- ...convexQuery(api.documents.queries.getMultiple, { documentIds }),
- });
+ } = useQuery({
+ ...convexQuery(api.documents.queries.getMultiple, { documentIds }),
+ // Avoid fetching when there are no attachments
+ enabled: documentIds.length > 0,
+ });
@@
- if (!documents?.length) return null;
@@
- if (!documents?.length) return null;
@@
if (isLoadingDocuments) {
@@
if (isDocumentsError || documentsError) {
@@
}
+
+ // No attached documents to render
+ if (!documents?.length) return null;Also applies to: 105-106, 110-111, 112-135
🤖 Prompt for AI Agents
In src/components/chat/input/document-list.tsx around lines 79 to 86 and
affecting 105-106, 110-111, 112-135: the useQuery currently always runs and the
component returns early when documents is empty which prevents loading/error UI
from ever showing; also there are duplicate guards. Update the useQuery call to
include an enabled flag that only runs when documentIds exists and has length >
0 (e.g., enabled: Boolean(documentIds?.length)), remove the duplicate empty-docs
guards, and move the `if (!documents?.length) return null` check to after
explicit checks for isLoadingDocuments and isDocumentsError so the spinner and
error UI render while the query is pending or failed.
- Removed unused GearIcon import from TopNav component. - Simplified imports in AppSidebar by removing unnecessary icons and loading spinner. - Adjusted layout properties for SidebarGroup and SidebarGroupLabel for better UI consistency.
- Removed outdated UX improvement note from temp.md. - Updated mcps submodule to commit 2a33ac72eaa99ca4b87a2e85991523696de5dd1a. - Made adjustments to the TopNav component by removing unused settings button and mode toggle for a cleaner interface.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/topnav.tsx (1)
47-49: Early return with breaks hook order — move redirect into an effectThis conditional return causes hooks below (useEffect/useParams) to be skipped on error renders, violating the Rules of Hooks (Biome errors at Lines 51, 58, 64). Redirect via useEffect instead. This also mirrors the earlier review feedback.
Apply:
- if (isErrorUser) { - return <Navigate to="/auth" />; - } + // Redirect to auth on user fetch error without breaking hooks order + useEffect(() => { + if (isErrorUser) { + navigate({ to: "/auth" }); + } + }, [isErrorUser, navigate]);
🧹 Nitpick comments (6)
src/components/topnav.tsx (6)
18-23: Remove unused Navigate import after redirect refactorOnce the early return is removed, Navigate is no longer needed.
-import { - Navigate, - useLocation, - useNavigate, - useParams, -} from "@tanstack/react-router"; +import { useLocation, useNavigate, useParams } from "@tanstack/react-router";
39-45: Harden auth query behavior to avoid thrash and unnecessary refetchAuth failures can loop retries and trigger repeated redirects. Consider turning off retries and giving the user doc a short cache window.
- } = useQuery({ - ...convexQuery(api.auth.getUser, {}), - }); + } = useQuery({ + ...convexQuery(api.auth.getUser, {}), + retry: false, + staleTime: 60_000, + });
63-81: Guard global Cmd/Ctrl+I against text inputs/contentEditablePrevents accidental panel toggles while typing in inputs or rich editors (Cmd/Ctrl+I is a common “italic” shortcut).
const handleKeyDown = (event: KeyboardEvent) => { - if ( + const target = event.target as HTMLElement | null; + if (target && (target.isContentEditable || ["INPUT", "TEXTAREA"].includes(target.tagName))) { + return; + } + if ( !event.repeat && event.key === "i" && (event.metaKey || event.ctrlKey) ) {
120-129: Use functional updater to avoid stale state on atom toggleMinor, but the functional form is safer and consistent with your keybinding handler.
- onClick={() => { - setResizePanelOpen(!resizePanelOpen); - setSelectedArtifact(undefined); - }} + onClick={() => { + setResizePanelOpen(prev => !prev); + setSelectedArtifact(undefined); + }}
119-136: UX: Consider disabling (not hiding) the panel toggle when an artifact/MCP is selectedHiding controls shifts layout and removes affordance; disabling with a tooltip preserves layout and sets user expectation.
Example (conceptual):
- {isOnChatRoute && ( + {isOnChatRoute && ( <Button variant="ghost" size="icon" - className={`${selectedArtifact || selectedVibzMcp ? "hidden" : ""}`} + className="" + disabled={!!(selectedArtifact || selectedVibzMcp)} aria-label="Toggle right panel"
83-139: Optional: Avoid nav flicker while user is loadingIf you notice a brief topbar flash before redirect, you can hide the bar during initial auth resolution without breaking hook order (return occurs after all hooks).
export function TopNav() { ... - return ( + if (isLoadingUser) return null; + return ( <div
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/components/chat/input/toolbar/index.tsx(3 hunks)src/components/chat/messages/user-message.tsx(2 hunks)src/components/topnav.tsx(1 hunks)src/components/ui/accordion.tsx(1 hunks)temp.md(0 hunks)
💤 Files with no reviewable changes (1)
- temp.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/chat/messages/user-message.tsx
- src/components/chat/input/toolbar/index.tsx
- src/components/ui/accordion.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/topnav.tsx (5)
src/store/chatStore.ts (5)
resizePanelOpenAtom(36-36)selectedArtifactAtom(53-53)sidebarOpenAtom(34-34)selectedVibzMcpAtom(54-54)userAtom(10-10)convex/_generated/api.js (2)
api(21-21)api(21-21)src/components/ui/sidebar.tsx (1)
SidebarTrigger(724-724)src/components/ui/button.tsx (1)
Button(59-59)src/components/theme-provider.tsx (1)
ModeToggle(69-86)
🪛 Biome (2.1.2)
src/components/topnav.tsx
[error] 51-51: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 58-58: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 64-64: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (1)
src/components/topnav.tsx (1)
51-55: Scripts are running… [waiting for results]
- Added stats.html to .gitignore to prevent tracking of generated files. - Reformatted code in DocumentDialog for improved readability and consistency. - Simplified user error handling in TopNav by moving error check to a more logical position.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
src/components/document-dialog.tsx (7)
24-34: Confirm conditional query behavior; consider adding enabled to avoid accidental fetchesYou rely on convexQuery(..., "skip"), which is fine if the wrapper handles the sentinel. To be safe with TanStack Query semantics and avoid any accidental fetches when the dialog is closed, add an explicit enabled flag keyed to documentDialogOpen.
} = useQuery({ ...convexQuery( api.documents.queries.get, documentDialogOpen ? { documentId: documentDialogOpen } : "skip", ), + enabled: !!documentDialogOpen, });If convexQuery already guarantees no-op when passed "skip", this remains a no-op safety net. Please verify current wrapper behavior.
97-105: Broaden download support (image/pdf/audio/video) and guard with try/catchUsers often want to download images/PDFs/media too. Also add basic error handling around the mutation.
const handleDownload = async () => { - if (!document || tag !== "file") return; - const url = await generateDownloadUrl({ - documentId: document._id!, - }); - if (url) { - window.open(url, "_blank"); - } + if (!document) return; + // Skip external-only resources + if (tag === "url" || tag === "site" || tag === "youtube") return; + try { + const url = await generateDownloadUrl({ + documentId: document._id!, + }); + if (url) { + const w = window.open(url, "_blank", "noopener,noreferrer"); + if (w) w.opener = null; + } + } catch (e) { + console.error("Failed to generate download url", e); + } }; ... - {tag === "file" ? ( + {(tag === "file" || tag === "pdf" || tag === "image" || tag === "audio" || tag === "video") ? ( <Button onClick={handleDownload}>Download</Button> ) : tag === "url" || tag === "site" || tag === "youtube" ? ( <Button onClick={handleOpen}> Open {tag === "youtube" ? "in YouTube" : "in Browser"} </Button> ) : null} ```<!-- review_comment_end --> Also applies to: 261-267 --- `172-176`: **Show “0 Bytes” correctly when size is zero** The current truthy check hides size 0. Use a null/undefined check instead. ```diff - {document?.size && ( + {document?.size != null && ( <div className="text-sm text-muted-foreground"> Size: {formatBytes(document.size)} </div> )} ```<!-- review_comment_end --> --- `202-212`: **Add title for PDF object for a11y** Mirror the file fallback’s title and improve accessibility. ```diff <object + title="PDF preview" data={previewUrl} className="absolute inset-0 w-full h-full" type="application/pdf" > ```<!-- review_comment_end --> --- `224-237`: **Revisit iframe sandbox policy for external sites** Sandboxing with allow-same-origin + allow-scripts reduces isolation. If the content is untrusted and you don’t need DOM access, consider dropping allow-same-origin for stronger origin isolation. If you want links to work, add allow-popups (and optionally allow-top-navigation-by-user-activation). You may also want to add referrerPolicy="no-referrer". Example: ```tsx <iframe title="Website preview" src={previewUrl} className="absolute inset-0 w-full h-full" sandbox="allow-scripts allow-popups allow-top-navigation-by-user-activation" referrerPolicy="no-referrer" />Please verify UX expectations (do links inside the frame need to open?) and adjust accordingly.
40-47: Minor naming consistency: use lowerCamelCase for iconClassNameRenaming improves consistency with typical React prop naming.
- const { - icon: Icon, - className: IconClassName, - tag, - } = document + const { + icon: Icon, + className: iconClassName, + tag, + } = document ? getDocTagInfo(document) : { icon: () => null, className: "", tag: "" };- <Icon className={`${IconClassName} h-8 w-8`} /> + <Icon className={`${iconClassName} h-8 w-8`} /> ```<!-- review_comment_end --> Also applies to: 160-161 --- `189-257`: **Optional: show a lightweight spinner while generating preview URLs** Between document load and previewUrl resolution (signed URL generation), the preview area is blank. Surface a small inline spinner for better perceived performance. ```diff - const { mutateAsync: generateDownloadUrl } = useMutation({ + const { mutateAsync: generateDownloadUrl, isPending: isGeneratingPreview } = useMutation({ mutationFn: useConvexMutation(api.documents.mutations.generateDownloadUrl), });- {previewUrl && ( + {previewUrl ? ( <div className="flex-grow relative min-h-0 bg-muted rounded-md border overflow-hidden"> {(() => { if (tag === "image") { return ( <img src={previewUrl} alt={documentName} className="absolute inset-0 w-full h-full object-contain" /> ); } else if (tag === "pdf") { return ( <object data={previewUrl} className="absolute inset-0 w-full h-full" type="application/pdf" > <div className="absolute inset-0 flex items-center justify-center text-muted-foreground"> PDF preview not supported in your browser. Please download the file to view it. </div> </object> ); } else if (tag === "youtube") { return ( <iframe title="YouTube video player" src={previewUrl} className="absolute inset-0 w-full h-full" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowFullScreen /> ); } else if (tag === "url" || tag === "site") { return ( <iframe title="Website preview" src={previewUrl} className="absolute inset-0 w-full h-full" sandbox="allow-same-origin allow-scripts" style={{ transform: "scale(0.95)", transformOrigin: "top left", width: "105.3%", // 100/0.95 to compensate for scale height: "105.3%", }} /> ); } else if (tag === "file") { // fallback for unknown file types return ( <object title="File preview" data={previewUrl} className="absolute inset-0 w-full h-full" type="application/octet-stream" > <div className="absolute inset-0 flex items-center justify-center text-muted-foreground"> File preview not supported. Please download the file to view it. </div> </object> ); } return null; })()} </div> - )} + ) : !previewError ? ( + <div className="flex-grow flex items-center justify-center bg-muted rounded-md border"> + <LoadingSpinner className="h-5 w-5" label="Preparing preview..." /> + </div> + ) : null} ```<!-- review_comment_end --> <!-- file_end --> Also applies to: 36-38 </blockquote></details> <details> <summary>src/components/topnav.tsx (4)</summary><blockquote> `31-36`: **Also wire a setter for `selectedVibzMcp` (to clear it when opening the right panel)** Hotkey and button clear `selectedArtifact` before opening the panel, but not `selectedVibzMcp`. Add its setter for consistency. ```diff - const selectedVibzMcp = useAtomValue(selectedVibzMcpAtom); + const selectedVibzMcp = useAtomValue(selectedVibzMcpAtom); + const setSelectedVibzMcp = useSetAtom(selectedVibzMcpAtom);
52-71: Hotkey: unify selection clearing and dependency listWhen toggling open via Ctrl/Cmd+I, clear both
selectedArtifactandselectedVibzMcpso the panel opens in a clean state. Also include the new setter in the effect deps.useEffect(() => { if (!isOnChatRoute) return; const handleKeyDown = (event: KeyboardEvent) => { if ( !event.repeat && event.key === "i" && (event.metaKey || event.ctrlKey) ) { event.preventDefault(); setResizePanelOpen((open) => { - if (!open) setSelectedArtifact(undefined); + if (!open) { + setSelectedArtifact(undefined); + setSelectedVibzMcp(undefined); + } return !open; }); } }; window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); - }, [isOnChatRoute, setResizePanelOpen, setSelectedArtifact]); + }, [isOnChatRoute, setResizePanelOpen, setSelectedArtifact, setSelectedVibzMcp]);
112-129: Use functional state update and clear both selections when toggling via buttonFunctional updates prevent stale-closure issues. Clear both selections to mirror the hotkey behavior.
{isOnChatRoute && ( <Button variant="ghost" size="icon" className={`${selectedArtifact || selectedVibzMcp ? "hidden" : ""}`} aria-label="Toggle right panel" onClick={() => { - setResizePanelOpen(!resizePanelOpen); - setSelectedArtifact(undefined); + setResizePanelOpen((prev) => !prev); + setSelectedArtifact(undefined); + setSelectedVibzMcp(undefined); }} >Accessibility nitpick (optional):
- Add
aria-pressed={resizePanelOpen}oraria-expanded={resizePanelOpen}andaria-controlsif the right panel has an id.
77-83: Tame complex className strings with a utility for readabilityLong template strings are hard to diff and reason about. Consider using a
cn/clsxhelper or a variant factory (e.g., cva) to compose conditional classes.Example (illustrative):
import { cn } from "@/lib/utils"; <div className={cn( "fixed right-0 z-50 flex w-full items-center justify-between bg-transparent py-2 px-1.5 pointer-events-none text-foreground/70", isSettingsRoute && "hidden" )} />Also applies to: 97-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.gitignore(2 hunks)src/components/document-dialog.tsx(1 hunks)src/components/topnav.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/document-dialog.tsx (6)
src/store/chatStore.ts (1)
documentDialogOpenAtom(43-45)convex/documents/mutations.ts (1)
generateDownloadUrl(22-39)src/lib/helper.tsx (1)
getDocTagInfo(136-174)src/components/ui/loading-spinner.tsx (1)
LoadingSpinner(11-35)src/components/ui/error-state.tsx (1)
ErrorState(122-122)src/lib/utils.ts (1)
formatBytes(8-16)
src/components/topnav.tsx (3)
src/store/chatStore.ts (5)
resizePanelOpenAtom(36-36)selectedArtifactAtom(53-53)sidebarOpenAtom(34-34)selectedVibzMcpAtom(54-54)userAtom(10-10)convex/_generated/api.js (2)
api(21-21)api(21-21)src/components/theme-provider.tsx (1)
ModeToggle(69-86)
🔇 Additional comments (2)
src/components/topnav.tsx (2)
72-74: Past concern resolved: hooks are no longer conditionalThe
<Navigate />early return comes after all hooks. Hook order remains stable across renders. Good fix relative to the earlier review.
46-51: Clear stale user on auth error in TopNavApply the following update in
src/components/topnav.tsxto ensure the global store doesn’t retain a signed-in user when the auth query fails:useEffect(() => { - if (user) { - setUser(user); - } + if (user) { + setUser(user); + } else if (isErrorUser) { + // Clear stale user on auth failure + setUser(undefined as any); + } }, [user, isErrorUser, setUser]);• Verify that
userAtominsrc/store/chatStore.tspermitsundefined(e.g.atom<Doc<"users"> | undefined>(undefined)); if it doesn’t, update its type signature accordingly so clearing it won’t cause type errors.
• This change prevents the UI from displaying a stale “signed-in” state when authentication fails.
| const { data: user, isError: isErrorUser } = useQuery({ | ||
| ...convexQuery(api.auth.getUser, {}), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid logging users out on any fetch error — redirect only on auth errors
isErrorUser is true for all failures (network blips, 5xx, timeouts). Redirecting unconditionally to /auth will kick users out spuriously. Capture error from the query and gate the redirect on an auth-specific condition. Also consider basic query hygiene (retry, staleTime).
Apply this diff to capture the error and add query options:
- const { data: user, isError: isErrorUser } = useQuery({
- ...convexQuery(api.auth.getUser, {}),
- });
+ const {
+ data: user,
+ error: userError,
+ isError: isErrorUser,
+ } = useQuery({
+ ...convexQuery(api.auth.getUser, {}),
+ // Avoid looping on auth errors; tolerate brief transient failures.
+ retry: (failureCount, err) => !isUnauthorized(err) && failureCount < 2,
+ staleTime: 5 * 60 * 1000,
+ });Add this helper in the module (outside the component) to detect auth errors (adjust to your error shape as needed):
function isUnauthorized(err: unknown) {
const e = err as any;
return e?.response?.status === 401 || e?.status === 401 || e?.code === "UNAUTHENTICATED";
}And update the redirect check below (see lines 72–74 comment).
| if (isErrorUser) { | ||
| return <Navigate to="/auth" />; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Scope the redirect to unauthorized errors only
Keep the component-level <Navigate />, but only when the error indicates an auth failure.
Apply this diff:
- if (isErrorUser) {
- return <Navigate to="/auth" />;
- }
+ if (isErrorUser && isUnauthorized(userError)) {
+ return <Navigate to="/auth" />;
+ }📝 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.
| if (isErrorUser) { | |
| return <Navigate to="/auth" />; | |
| } | |
| if (isErrorUser && isUnauthorized(userError)) { | |
| return <Navigate to="/auth" />; | |
| } |
🤖 Prompt for AI Agents
In src/components/topnav.tsx around lines 72-74, the component currently
redirects to /auth for any error; change this so the <Navigate /> is only
returned when the error explicitly indicates an authorization failure (e.g.,
error.status === 401 or an isUnauthorized flag from your auth/error hook).
Update the conditional from isErrorUser to check for unauthorized (for example:
isErrorUser && (userError?.status === 401 || userError?.code === 'UNAUTHORIZED'
|| isUnauthorized)), leaving other errors to render their normal error UI or
messages.
- Added new dependencies for @dnd-kit components to support drag-and-drop functionality. - Updated versions for @tanstack/react-query and @tanstack/query-core for improved performance and features.
- Removed unused imports and error handling in DocumentDialog for cleaner code. - Added preview error state management in DocumentDialog to enhance user feedback. - Updated TopNav to include retry logic for user query and improved keydown event handling. - Simplified settings button rendering in TopNav for better accessibility.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/topnav.tsx (1)
45-49: Prevent stale user after logout/401; clear the store and optionally redirectIf the request 401s, the userAtom will retain the previous (stale) user because we only set on truthy user. Clear on falsy user and consider redirecting to /auth on explicit unauthorized errors via an effect. If a higher-level route guard already handles the redirect, still clear the store here.
useEffect(() => { if (user) { setUser(user); } -}, [user, setUser]); + else { + // Clear stale user (e.g., after logout or 401). + setUser(undefined); + } +}, [user, setUser]); + +// Redirect only on explicit unauthorized errors. +useEffect(() => { + if (isErrorUser && isUnauthorized(userError)) { + setUser(undefined); + navigate({ to: "/auth", replace: true }); + } +}, [isErrorUser, userError, navigate, setUser]);src/components/document-dialog.tsx (1)
44-89: Make preview URL loading race-safe and include video/audio in signed-URL pathPrevent stale updates when switching documents quickly and add support for video/audio. Also fix the comment to reflect the broader set of stored objects that require a signed URL.
useEffect(() => { - setPreviewUrl(null); - setPreviewError(null); - const loadPreviewUrl = async () => { + setPreviewUrl(null); + setPreviewError(null); + let cancelled = false; + const loadPreviewUrl = async () => { if (!document) return; try { switch (tag) { case "image": case "pdf": + case "video": + case "audio": case "file": { - // Only files need download URL + // Stored objects need a signed download URL const url = await generateDownloadUrl({ documentId: document._id!, }); - setPreviewUrl(url); + if (!cancelled) setPreviewUrl(url); break; } case "url": case "site": { - setPreviewUrl(document.key as string); + if (!cancelled) setPreviewUrl(document.key as string); break; } case "youtube": { - setPreviewUrl(`https://www.youtube.com/embed/${document.key}`); + if (!cancelled) + setPreviewUrl(`https://www.youtube.com/embed/${document.key}`); break; } default: if (["file", "text", "github"].includes(document.type)) { const url = await generateDownloadUrl({ documentId: document._id!, }); - setPreviewUrl(url); + if (!cancelled) setPreviewUrl(url); break; } else { - setPreviewUrl(document.key as string); + if (!cancelled) setPreviewUrl(document.key as string); } break; } } catch (error) { - setPreviewError( - `Failed to load preview: ${error instanceof Error ? error.message : "Unknown error"}`, - ); + if (!cancelled) { + setPreviewError( + `Failed to load preview: ${error instanceof Error ? error.message : "Unknown error"}`, + ); + } } }; loadPreviewUrl(); - }, [document, tag, generateDownloadUrl]); + return () => { + cancelled = true; + }; + }, [document, tag, generateDownloadUrl]);
🧹 Nitpick comments (9)
src/components/topnav.tsx (4)
34-43: Harden unauthorized detection and expose error/isError from the queryThe current isUnauthorized check is brittle (string search in message) and the query doesn’t expose error/isError for downstream handling. Make the guard robust across common error shapes and surface error/isError so you can react to 401s deterministically.
- function isUnauthorized(err: unknown) { - return err instanceof Error && err.message.includes("401"); - } + function isUnauthorized(err: unknown): boolean { + if (!err) return false; + const e: any = err; + const status = e?.status ?? e?.response?.status ?? e?.statusCode; + const code = e?.code ?? e?.data?.code ?? e?.response?.data?.code; + const message: string = + typeof e?.message === "string" ? e.message : ""; + return ( + status === 401 || + code === "UNAUTHENTICATED" || + code === "UNAUTHORIZED" || + /(^|[^0-9])401([^0-9]|$)/.test(message) || + /unauth/i.test(message) + ); + } - const { data: user } = useQuery({ + const { + data: user, + error: userError, + isError: isErrorUser, + } = useQuery({ ...convexQuery(api.auth.getUser, {}), // Avoid looping on auth errors; tolerate brief transient failures. retry: (failureCount, err) => !isUnauthorized(err) && failureCount < 2, staleTime: 5 * 60 * 1000, });
57-75: Don’t hijack typing: ignore hotkey when focus is in inputs/contentEditableThe global Ctrl/Cmd+I listener currently fires even when the user is typing in an input/textarea or a contentEditable. Add a guard to avoid disrupting text entry.
useEffect(() => { if (!isOnChatRoute) return; const handleKeyDown = (event: KeyboardEvent) => { - if ( + if ( !event.repeat && event.key === "i" && (event.metaKey || event.ctrlKey) ) { + // Ignore when user is typing in inputs or contentEditable fields. + const target = event.target as HTMLElement | null; + if (target?.closest("input, textarea, [contenteditable='true']")) { + return; + } event.preventDefault(); setResizePanelOpen((open) => { if (!open) setSelectedArtifact(undefined); return !open; }); } };
116-126: Use functional state update and clear artifact only on open; add aria-pressedAlign the click toggle with the keyboard handler (clear only when opening) and avoid potential stale reads by using the functional updater. aria-pressed improves a11y for toggle buttons.
- {isOnChatRoute && ( - <Button + {isOnChatRoute && ( + <Button variant="ghost" size="icon" - className={`${selectedArtifact || selectedVibzMcp ? "hidden" : ""}`} + className={`${selectedArtifact || selectedVibzMcp ? "hidden" : ""}`} aria-label="Toggle right panel" - onClick={() => { - setResizePanelOpen(!resizePanelOpen); - setSelectedArtifact(undefined); - }} + aria-pressed={resizePanelOpen} + onClick={() => { + setResizePanelOpen((prev) => { + const next = !prev; + if (next && !prev) setSelectedArtifact(undefined); + return next; + }); + }} >
77-83: Add navigation landmark semantics for accessibilityExpose the top bar as a navigation landmark for better screen reader navigation.
- <div + <div role="navigation" aria-label="Top navigation" className={`fixed right-0 py-2 flex items-center w-full bg-transparent justify-between pointer-events-none text-foreground/70 z-50 px-1.5 ${isSettingsRoute ? "hidden" : ""}`} >src/components/document-dialog.tsx (5)
96-105: Handle download failures to surface errors to usersWrap generateDownloadUrl and window.open in try/catch and display a friendly error via previewError.
const handleDownload = async () => { if (!document || tag !== "file") return; - const url = await generateDownloadUrl({ - documentId: document._id!, - }); - if (url) { - const w = window.open(url, "_blank", "noopener,noreferrer"); - if (w) w.opener = null; - } + try { + const url = await generateDownloadUrl({ documentId: document._id! }); + if (url) { + const w = window.open(url, "_blank", "noopener,noreferrer"); + if (w) w.opener = null; + } + } catch (e) { + setPreviewError( + `Failed to download: ${e instanceof Error ? e.message : "Unknown error"}`, + ); + } };
23-29: Expose loading/error from useQuery so the dialog can render proper statesYou’re already standardizing loading/error across the app. Surface these here to avoid silent failures or infinite spinners when the document fetch fails.
- const { data: document } = useQuery({ + const { + data: document, + error: documentError, + isError: isDocumentError, + isLoading: isDocumentLoading, + } = useQuery({ ...convexQuery( api.documents.queries.get, documentDialogOpen ? { documentId: documentDialogOpen } : "skip", ), enabled: !!documentDialogOpen, });
153-227: Render standardized ErrorState and a LoadingSpinner; add video/audio previews; add iframe referrer policyUse the new primitives for consistency, preview video/audio when available, and reduce referrer leakage from iframes.
- {previewError ? ( - <div className="flex-grow flex items-center justify-center bg-muted rounded-md border"> - <div className="text-center text-muted-foreground"> - {previewError} - </div> - </div> - ) : previewUrl ? ( + {isDocumentError ? ( + <div className="flex-grow flex items-center justify-center bg-muted rounded-md border"> + <ErrorState + density="comfy" + title="Unable to load document" + error={documentError} + /> + </div> + ) : previewError ? ( + <div className="flex-grow flex items-center justify-center bg-muted rounded-md border"> + <ErrorState + density="comfy" + title="Unable to preview document" + error={previewError} + /> + </div> + ) : previewUrl ? ( <div className="flex-grow relative min-h-0 bg-muted rounded-md border overflow-hidden"> {(() => { if (tag === "image") { return ( <img src={previewUrl} alt={documentName} className="absolute inset-0 w-full h-full object-contain" /> ); } else if (tag === "pdf") { return ( <object data={previewUrl} className="absolute inset-0 w-full h-full" type="application/pdf" > <div className="absolute inset-0 flex items-center justify-center text-muted-foreground"> PDF preview not supported in your browser. Please download the file to view it. </div> </object> ); + } else if (tag === "video") { + return ( + <video + src={previewUrl} + className="absolute inset-0 w-full h-full" + controls + /> + ); + } else if (tag === "audio") { + return ( + <audio + src={previewUrl} + className="absolute inset-x-0 bottom-4 w-[95%] mx-auto" + controls + /> + ); } else if (tag === "youtube") { return ( <iframe title={`YouTube video: ${documentName}`} src={previewUrl} className="absolute inset-0 w-full h-full" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" + referrerPolicy="no-referrer" allowFullScreen /> ); } else if (tag === "url" || tag === "site") { return ( <iframe title={`Website preview: ${documentName}`} src={previewUrl} className="absolute inset-0 w-full h-full" sandbox="allow-same-origin allow-scripts" + referrerPolicy="no-referrer" style={{ transform: "scale(0.95)", transformOrigin: "top left", width: "105.3%", // 100/0.95 to compensate for scale height: "105.3%", }} /> ); } else if (tag === "file") { // fallback for unknown file types return ( <object data={previewUrl} className="absolute inset-0 w-full h-full" type="application/octet-stream" > <div className="absolute inset-0 flex items-center justify-center text-muted-foreground"> File preview not supported. Please download the file to view it. </div> </object> ); } return null; })()} </div> - ) : null} + ) : ( + <div + className="flex-grow flex items-center justify-center bg-muted rounded-md border" + aria-busy + > + <LoadingSpinner /> + </div> + )}Note: You’ll need to import the shared primitives if not already:
import { Button } from "@/components/ui/button"; +import { ErrorState } from "@/components/ui/error-state"; +import { LoadingSpinner } from "@/components/ui/loading-spinner";
146-150: Show size when it’s 0 bytes as wellGuard on defined-ness rather than truthiness so 0 B is rendered.
- {document?.size && ( + {typeof document?.size === "number" && ( <div className="text-sm text-muted-foreground"> Size: {formatBytes(document.size)} </div> )}
131-137: Minor: Add file type to the heading for clarity (optional)Showing the tag or a human label next to the name improves context when the preview fails.
- <DialogTitle className="flex items-center gap-2"> + <DialogTitle className="flex items-center gap-2"> <Icon className={`${IconClassName} h-8 w-8`} /> - <span>{documentName}</span> + <span>{documentName}</span> + {tag && <span className="text-muted-foreground text-sm">({tag})</span>} </DialogTitle>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/components/document-dialog.tsx(2 hunks)src/components/topnav.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/document-dialog.tsx (4)
src/store/chatStore.ts (1)
documentDialogOpenAtom(44-46)convex/documents/mutations.ts (1)
generateDownloadUrl(22-39)src/lib/helper.tsx (1)
getDocTagInfo(136-174)src/lib/utils.ts (1)
formatBytes(8-16)
src/components/topnav.tsx (3)
src/store/chatStore.ts (5)
resizePanelOpenAtom(37-37)selectedArtifactAtom(54-54)sidebarOpenAtom(35-35)selectedVibzMcpAtom(55-55)userAtom(11-11)convex/_generated/api.js (2)
api(21-21)api(21-21)src/components/theme-provider.tsx (1)
ModeToggle(69-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
src/components/topnav.tsx (1)
51-55: Route detection: verify chat route assumptionUsing useParams({ strict: false }) and !!params.chatId is fine if chatId is reliably present only on chat routes. If params shape changes or chat routes get nested differently, consider a route match check for resilience (e.g., useMatch({ to: "/chat/$chatId", fuzzy: false })).
src/components/document-dialog.tsx (1)
107-124: LGTM on reverse tabnabbing hardeningNice use of "noopener,noreferrer" and clearing opener in both branches.
- Added URL validation in DocumentDialog to ensure only safe links are processed. - Improved error handling for invalid URLs in DocumentDialog's preview functionality. - Updated TopNav to navigate to the authentication page on user query errors. - Refactored code for better readability and consistency across components.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
src/components/topnav.tsx (1)
76-78: Redirect only on auth errors; avoid logging users out on transient failuresCurrently any fetch error triggers a redirect to /auth. This will bounce users on network blips, 5xx, timeouts, etc. Capture the query error and gate the redirect on an auth-specific check.
Apply this diff to scope the redirect and plumb the error through:
-const { data: user, isError: isErrorUser } = useQuery({ +const { + data: user, + error: userError, + isError: isErrorUser, +} = useQuery({ ...convexQuery(api.auth.getUser, {}), // Avoid looping on auth errors; tolerate brief transient failures. retry: (failureCount, err) => !isUnauthorized(err) && failureCount < 2, staleTime: 5 * 60 * 1000, }); -if (isErrorUser) { - return <Navigate to="/auth" />; -} +if (isErrorUser && isUnauthorized(userError)) { + return <Navigate to="/auth" />; +}If you prefer to avoid early returns entirely, navigate in an effect instead and render normal UI while the redirect is in flight. I can provide that variant if helpful.
src/components/document-dialog.tsx (2)
94-95: Validate URLs in the default caseThe default case sets
document.keydirectly as the preview URL without validation. This could be a security risk if the key contains malicious URLs.} else { - setPreviewUrl(document.key as string); + const externalUrl = document.key as string; + if (isValidExternalUrl(externalUrl)) { + setPreviewUrl(externalUrl); + } else { + setPreviewError("Invalid or unsafe URL"); + } }
54-105: Add cleanup to prevent state updates after unmountThe
useEffectfor loading preview URLs should include a cleanup mechanism to prevent setting state after the component unmounts or when the document changes rapidly.useEffect(() => { setPreviewUrl(null); setPreviewError(null); + let cancelled = false; const loadPreviewUrl = async () => { if (!document) return; try { switch (tag) { case "image": case "pdf": case "file": { // Only files need download URL const url = await generateDownloadUrl({ documentId: document._id, }); - setPreviewUrl(url); + if (!cancelled) setPreviewUrl(url); break; } case "url": case "site": { const externalUrl = document.key as string; if (isValidExternalUrl(externalUrl)) { - setPreviewUrl(externalUrl); + if (!cancelled) setPreviewUrl(externalUrl); } else { - setPreviewError("Invalid or unsafe URL"); + if (!cancelled) setPreviewError("Invalid or unsafe URL"); } break; } case "youtube": { - setPreviewUrl(`https://www.youtube.com/embed/${document.key}`); + if (!cancelled) setPreviewUrl(`https://www.youtube.com/embed/${document.key}`); break; } default: if (["file", "text", "github"].includes(document.type)) { const url = await generateDownloadUrl({ documentId: document._id, }); - setPreviewUrl(url); + if (!cancelled) setPreviewUrl(url); break; } else { - setPreviewUrl(document.key as string); + if (!cancelled) setPreviewUrl(document.key as string); } break; } } catch (error) { - setPreviewError( - `Failed to load preview: ${error instanceof Error ? error.message : "Unknown error"}`, - ); + if (!cancelled) { + setPreviewError( + `Failed to load preview: ${error instanceof Error ? error.message : "Unknown error"}`, + ); + } } }; loadPreviewUrl(); + return () => { + cancelled = true; + }; }, [document, tag, generateDownloadUrl]);
🧹 Nitpick comments (20)
src/components/ui/tooltip-button.tsx (4)
18-23: Type safety: Use React’s MouseEventHandler for onClick.onClick is currently typed as a no-arg function, which loses event typing and blocks access to event params. Use React.MouseEventHandler for correctness.
- onClick: () => void; + onClick: React.MouseEventHandler<HTMLButtonElement>;
31-31: A11y: Ensure an accessible name even when ariaLabel isn’t provided.For an icon-only button, the accessible name must be present. If ariaLabel is omitted, fallback to tooltip for aria-label to avoid a silent control.
- aria-label={ariaLabel} + aria-label={ariaLabel ?? tooltip}
27-33: Prevent accidental form submissions and pointer-cursor on disabled.
- Add type="button" to avoid unintended form submissions when this button is placed inside a form.
- Don’t force cursor-pointer when disabled; either remove the class or switch to cursor-not-allowed conditionally (addressed in the main fix).
- variant="ghost" - size="icon" + variant="ghost" + size="icon" + type="button" - className="cursor-pointer"
24-25: LiftTooltipProviderto a global provider to eliminate redundant mountsWe’ve found multiple local mounts of
TooltipProviderwithout an app-level wrapper, which leads to unnecessary nesting and run-time overhead. It’s safe to assume a single, top-levelTooltipProviderexists and remove the per-component wrappers (or, if you prefer local defaults, document that nesting is supported).Locations to update:
- src/components/ui/tooltip-button.tsx (lines 24–25):
<TooltipProvider>around each button- src/components/ui/tooltip.tsx (lines 23–25): provider wrapped around the primitive root
- src/components/ui/sidebar.tsx (line 131): provider around sidebar tooltips
- src/components/chat/messages/ai-message/index.tsx (lines 64–65): provider per AI message
- src/components/chat/messages/utils-bar/copy-button.tsx (lines 27–28): provider per copy button
Suggested refactor:
- In your root provider (e.g.
src/components/theme-provider.tsxorApp.tsx), wrap the app in a single<TooltipProvider>:export default function ThemeProvider({ children }) { return ( + <TooltipProvider delayDuration={0}> {/* existing theming/context providers */} {children} + </TooltipProvider> ); }- Remove the local
<TooltipProvider>wrappers from the files listed above.- (Optional) In components that still include a provider, add a comment:
// Nesting TooltipProvider is supported but not required if a global provider existssrc/components/chat/messages/utils-bar/index.ts (4)
24-31: Track mutation with a stable key and surface status/error for the new loading/error UX.Adding a mutationKey improves React Query devtools visibility and deduping. Also, consider exposing isPending/error so callers can hook into LoadingSpinner/ErrorState.
Apply this focused diff:
- const { mutateAsync: updateChatMutation } = useMutation({ - mutationFn: useConvexMutation(api.chats.mutations.update), - }); + const { + mutateAsync: updateChatMutation, + isPending: isUpdatingChatModel, + error: updateChatError, + } = useMutation({ + mutationKey: ["chats.update"], + mutationFn: useConvexMutation(api.chats.mutations.update), + });And return these in the hook result (see lines 70-75 suggestion).
53-68: Drop unnecessary optional chaining and add error handling around model updates and regeneration.
- useNavigateBranch returns a function; optional chaining isn’t needed.
- Wrap updateChatMutation/regenerate so callers can render ErrorState on failure.
Apply this diff:
- const handleRegenerate = async ( + const handleRegenerate = async ( input: MessageWithBranchInfo, model?: string, ) => { - navigateBranch?.(input.depth, "next", input.totalBranches); - // If the model is provided, update the chat with the new model or - if (model) { - await updateChatMutation({ - chatId: input.message.chatId, - updates: { model }, - }); - } - await regenerate({ - messageId: input.message._id, - }); + try { + navigateBranch(input.depth, "next", input.totalBranches); + if (model) { + await updateChatMutation({ + chatId: input.message.chatId, + updates: { model }, + }); + } + await regenerate({ messageId: input.message._id }); + } catch (err) { + // Bubble up so the caller can render ErrorState + throw err; + } };
70-75: Expose mutation status/error so callers can plug into LoadingSpinner/ErrorState.Given the PR’s UX goals, returning isUpdatingChatModel and updateChatError will let consumers show spinners and error UIs during regenerate-with-model-change flows.
Apply this diff (pairs with the change suggested on lines 24-31):
return { handleBranch, handleRegenerate, navigate, navigateBranch, + isUpdatingChatModel, + updateChatError, };
33-51: Optional: EnhancehandleBranchwith error propagation and confirm non-nullchatIdI’ve verified that
MessageWithBranchInfocomes fromconvex/chatMessages/helpers.tsand that itsmessageproperty is aMessageNode(anOmit<Doc<"chatMessages">,…>) which always includes a non-nullablechatId: Id<"chats">(nochatId?in the repo) so you don’t need any runtime guard for undefined there. To align with the PR’sErrorStatepattern and ensure any failures bubble up properly, you can optionally wrap yourbranchChat/chatcalls in atry/catch(andawaitthechatcall).• Location:
src/components/chat/messages/utils-bar/index.ts(around lines 33–51)- const handleBranch = async ( + const handleBranch = async ( input: MessageWithBranchInfo, model?: string, editedContent?: { text?: string; documents?: Id<"documents">[] }, ) => { - const result = await branchChat({ - chatId: input.message.chatId, - branchFrom: input.message._id, - ...(model && { model }), - ...(editedContent && { editedContent }), - }); - if (result?.newChatId) { - // Model is already persisted by branchChat; start chat without forwarding model - chat({ - chatId: result.newChatId, - }); - navigateToChat(navigate, result.newChatId); - } + try { + const result = await branchChat({ + chatId: input.message.chatId, + branchFrom: input.message._id, + ...(model && { model }), + ...(editedContent && { editedContent }), + }); + if (result?.newChatId) { + // Model is already persisted by branchChat; start chat without forwarding model + await chat({ chatId: result.newChatId }); + navigateToChat(navigate, result.newChatId); + } + } catch (err) { + // Bubble up so the caller can render ErrorState + throw err; + } };This change is purely optional but will make sure any branch/chat failures surface correctly in your UI.
src/components/chat/messages/utils-bar/user-utils-bar.tsx (6)
114-130: Await mutation before closing edit UI to avoid race conditions.
onDone?.()fires immediately; navigation andchat()run in a.then, so the UI may close before the update/branch is applied. MakehandleSubmitasync, await the mutation (and optional chat), then callonDone.- const handleSubmit = (applySame: boolean) => { + const handleSubmit = async (applySame: boolean) => { if (!editedText) return; if (applySame === false) { navigateBranch?.(input.depth, "next", input.totalBranches); } - updateMessage({ + await updateMessage({ id: input.message._id as Id<"chatMessages">, updates: { text: editedText, documents: editedDocuments || [] }, applySame: applySame, - }).then(() => { - if (applySame === false) { - chat({ chatId: input.message.chatId }); - } - }); - onDone?.(); + }); + if (applySame === false) { + await chat({ chatId: input.message.chatId }); + } + onDone?.(); };
58-68: Reset file input after upload to allow re-selecting the same file; consider accept filter.Without resetting, picking the same file again won't trigger
onChange. Also, consider anacceptattribute (if you want to constrain types).const handleFileInputChange = useCallback( async (e: React.ChangeEvent<HTMLInputElement>) => { if (e.target.files && e.target.files.length > 0) { const uploadedIds = await handleFileUpload(e.target.files); if (uploadedIds && onDocumentsChange) { onDocumentsChange([...(editedDocuments || []), ...uploadedIds]); } } + // Allow selecting the same file again + e.target.value = ""; }, [handleFileUpload, editedDocuments, onDocumentsChange], );<input type="file" ref={fileInputRef} className="hidden" multiple + /* e.g., accept common doc types; adjust as needed */ + accept=".pdf,.txt,.md,.doc,.docx,.csv,.json,image/*" onChange={handleFileInputChange} />Also applies to: 140-146
84-90: Polish: indicate copy-drop intent during dragover.Setting
dropEffectimproves UX and communicates intent to the OS.(e: React.DragEvent<HTMLDivElement>) => { e.preventDefault(); + if (e.dataTransfer) e.dataTransfer.dropEffect = "copy"; if (!isDragActive) setIsDragActive(true); },
147-151: Add aria-labels to icon-only buttons for accessibility.Tooltip text isn't announced by screen readers; add
ariaLabel.<TooltipButton onClick={() => setEditing?.(null)} icon={<X className="h-4 w-4 text-foreground/70 cursor-pointer " />} tooltip="Cancel" + ariaLabel="Cancel" /><TooltipButton onClick={() => fileInputRef.current?.click()} icon={<Paperclip className="h-4 w-4 text-foreground/70" />} tooltip="Attach files" + ariaLabel="Attach files" />Also applies to: 196-201
101-112: Optional: memoize and centralize text extraction.The IIFE runs every render and duplicates content-parsing logic seen elsewhere. Consider a small utility (and
useMemo) to extract text safely and reuse across components.Example:
const copyText = useMemo(() => { const content = input?.message.message.content; if (!content) return ""; if (Array.isArray(content)) { const entry = (content as { type: string; text?: string }[]).find(e => e.type === "text"); return entry?.text ?? ""; } return typeof content === "string" ? content : ""; }, [input?.message.message.content]);
24-27: Consider a discriminated union forMessageContent.If only known kinds are expected, narrow
typeto a union (e.g.,"text" | "image" | "tool"). This improves type safety incopyTextand other consumers.interface MessageContentText { type: "text"; text: string; } type MessageContent = MessageContentText /* | MessageContentImage | ... */;src/components/topnav.tsx (3)
125-128: Prefer functional updater for consistency and to avoid stale closuresThis click handler uses the current value from the render, which is fine, but the keyboard shortcut uses a functional updater. Align the two and avoid potential stale toggles after rapid state changes.
onClick={() => { - setResizePanelOpen(!resizePanelOpen); + setResizePanelOpen((prev) => !prev); setSelectedArtifact(undefined); }}
120-135: Improve accessibility: indicate toggle state with aria-pressedExpose the expanded/collapsed state of the right panel to assistive tech.
<Button variant="ghost" size="icon" className={`${selectedArtifact || selectedVibzMcp ? "hidden" : ""}`} aria-label="Toggle right panel" + aria-pressed={resizePanelOpen} onClick={() => { setResizePanelOpen((prev) => !prev); setSelectedArtifact(undefined); }} >
49-53: Optional: clear user in store on confirmed auth failureIf you gate the redirect to only unauthorized errors, consider also clearing the user atom so downstream consumers don’t briefly read stale user data after an auth failure.
Example (add alongside your other effects):
useEffect(() => { // Requires userError from the query destructure if (isErrorUser && isUnauthorized(userError)) { setUser(undefined as any); // adjust typing if userAtom excludes undefined } }, [isErrorUser, userError, setUser]);If userAtom’s type excludes undefined, I can help adjust its typing to include undefined safely.
src/components/document-dialog.tsx (3)
254-260: Expand download button visibilityCurrently, the Download button only appears for
tag === "file". Consider showing it for other downloadable types like PDFs and images.- {tag === "file" ? ( + {["file", "pdf", "image", "video", "audio", "text", "github"].includes(tag) ? ( <Button onClick={handleDownload}>Download</Button> ) : tag === "url" || tag === "site" || tag === "youtube" ? ( <Button onClick={handleOpen}> Open {tag === "youtube" ? "in YouTube" : "in Browser"} </Button> ) : null}
176-249: Consider adding video and audio preview supportThe preview rendering doesn't handle video or audio tags that might be returned from
getDocTagInfo.Add cases for video and audio preview:
} else if (tag === "youtube") { return ( <iframe title={`YouTube video: ${documentName}`} src={previewUrl} className="absolute inset-0 w-full h-full" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowFullScreen /> ); +} else if (tag === "video") { + return ( + <video + src={previewUrl} + controls + className="absolute inset-0 w-full h-full object-contain" + > + <div className="absolute inset-0 flex items-center justify-center text-muted-foreground"> + Video preview not supported in your browser. + </div> + </video> + ); +} else if (tag === "audio") { + return ( + <div className="absolute inset-0 flex items-center justify-center"> + <audio src={previewUrl} controls /> + </div> + ); } else if (tag === "url" || tag === "site") {
168-174: Consider using the ErrorState component for consistencyThe PR introduces an
ErrorStatecomponent for consistent error handling across the app, but this component renders error messages directly without using it.+import { ErrorState } from "@/components/ui/error-state"; {previewError ? ( <div className="flex-grow flex items-center justify-center bg-muted rounded-md border"> - <div className="text-center text-muted-foreground"> - {previewError} - </div> + <ErrorState + density="comfy" + title="Unable to preview document" + error={new Error(previewError)} + /> </div> ) : previewUrl ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/components/chat/messages/utils-bar/index.ts(1 hunks)src/components/chat/messages/utils-bar/user-utils-bar.tsx(2 hunks)src/components/document-dialog.tsx(2 hunks)src/components/topnav.tsx(2 hunks)src/components/ui/tooltip-button.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/convex_rules.mdc)
**/*.ts: You can use the helper TypeScript type Id imported from './_generated/dataModel' to get the type of the id for a given table.
If you need to define a Record, provide the correct key and value types (e.g., Record<Id<'users'>, string>).
Be strict with types, particularly around ids of documents. Use Id<'table'> rather than string for document ids.
Always use as const for string literals in discriminated union types.
When using the Array type, always define arrays as const array: Array = [...];
When using the Record type, always define records as const record: Record<KeyType, ValueType> = {...};
Files:
src/components/chat/messages/utils-bar/index.ts
🧬 Code graph analysis (5)
src/components/chat/messages/utils-bar/index.ts (4)
convex/_generated/dataModel.d.ts (1)
Id(48-49)convex/langchain/index.ts (3)
regenerate(302-322)branchChat(324-442)chat(61-300)src/hooks/chats/use-messages.ts (2)
useNavigateBranch(76-102)MessageWithBranchInfo(17-17)convex/chatMessages/helpers.ts (1)
MessageWithBranchInfo(18-23)
src/components/document-dialog.tsx (4)
src/store/chatStore.ts (1)
documentDialogOpenAtom(44-46)convex/documents/mutations.ts (1)
generateDownloadUrl(22-39)src/lib/helper.tsx (1)
getDocTagInfo(136-174)src/lib/utils.ts (1)
formatBytes(8-16)
src/components/ui/tooltip-button.tsx (2)
src/components/ui/tooltip.tsx (4)
TooltipProvider(59-59)Tooltip(59-59)TooltipTrigger(59-59)TooltipContent(59-59)src/components/ui/button.tsx (1)
Button(59-59)
src/components/topnav.tsx (1)
src/store/chatStore.ts (5)
resizePanelOpenAtom(37-37)selectedArtifactAtom(54-54)sidebarOpenAtom(35-35)selectedVibzMcpAtom(55-55)userAtom(11-11)
src/components/chat/messages/utils-bar/user-utils-bar.tsx (6)
src/components/chat/messages/utils-bar/index.ts (1)
useMessageActions(23-76)src/hooks/chats/use-documents.ts (1)
useUploadDocuments(59-146)src/components/ui/tooltip-button.tsx (1)
TooltipButton(11-41)src/components/chat/messages/utils-bar/action-dropdown.tsx (1)
ActionDropdown(12-102)src/components/chat/messages/utils-bar/branch-navigation.tsx (1)
BranchNavigation(19-51)src/components/chat/messages/utils-bar/copy-button.tsx (1)
CopyButton(11-49)
🔇 Additional comments (6)
src/components/ui/tooltip-button.tsx (1)
8-8: Alias configuration validated
Yourtsconfig.jsondefines
"baseUrl": ".""paths": { "@/*": ["./src/*"] }and we confirmed imports like
from "@/components/ui/button"resolve throughout the codebase (includingsrc/components/ui/tooltip-button.tsx) without errors. No further changes needed.src/components/chat/messages/utils-bar/index.ts (1)
14-20: Nice extraction of a typed navigation helper.Centralizing route construction with strict Id<"chats"> typing reduces repetition and avoids accidental stringly-typed params. LGTM.
src/components/chat/messages/utils-bar/user-utils-bar.tsx (2)
6-13: Verify lucide icon import: usePaperclip, notPaperclipIcon.lucide-react typically exports
Paperclip. IfPaperclipIconisn't available in your version, this will fail at build-time. Replace the import and usage.import { Check, CheckCheck, GitBranch, Pencil, RefreshCcw, X, - PaperclipIcon, + Paperclip, } from "lucide-react";- icon={<PaperclipIcon className="h-4 w-4 text-foreground/70" />} + icon={<Paperclip className="h-4 w-4 text-foreground/70" />}Also applies to: 196-201
205-208: Guard against potentially undefinednavigateBranchinstead of using non-null assertion.
useMessageActions()returnsnavigateBranchfrom a hook; if it can be undefined in some contexts, the non-null assertion here can crash. Guard before renderingBranchNavigation.- <div className={`flex flex-row items-center gap-1 self-start opacity-70`}> - <BranchNavigation item={input} navigateBranch={navigateBranch!} /> + <div className={`flex flex-row items-center gap-1 self-start opacity-70`}> + {navigateBranch && ( + <BranchNavigation item={input} navigateBranch={navigateBranch} /> + )}src/components/topnav.tsx (1)
55-74: Keyboard toggle effect looks solidUnregisters reliably on dependency change, guards by route, and uses a functional updater to avoid stale state. Good job.
src/components/document-dialog.tsx (1)
18-25: Good security measure for URL validationThe
isValidExternalUrlhelper properly restricts external URLs to http/https schemes only, preventing XSS attacks through javascript: or data: URLs. This is a critical security control.
- Updated DocumentDialog to remove unnecessary line and improve code clarity. - Refined TopNav's error handling for unauthorized access with additional checks. - Improved drag event handling in UserUtilsBar for better user interaction. - Enhanced TooltipButton to provide a disabled state with appropriate styling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/components/topnav.tsx (2)
38-47: Broaden unauthorized detection to cover statusCode variantsSome stacks expose 401 as
statusCode(or onresponse). Cheap to include.return ( e?.response?.status === 401 || e?.status === 401 || + e?.statusCode === 401 || + e?.response?.statusCode === 401 || e?.code === "UNAUTHENTICATED" || e?.code === "UNAUTHORIZED" || (e instanceof Error && /\b401\b/.test(e.message)) );
83-85: Redirect only on 401; wire query error into the conditionUnconditionally redirecting on any fetch error will log users out for transient/network issues. Gate the redirect on an auth error and plumb the query’s
error.- const { data: user, isError: isErrorUser } = useQuery({ + const { + data: user, + error: userError, + isError: isErrorUser, + } = useQuery({ ...convexQuery(api.auth.getUser, {}), // Avoid looping on auth errors; tolerate brief transient failures. retry: (failureCount, err) => !isUnauthorized(err) && failureCount < 2, staleTime: 5 * 60 * 1000, });- if (isErrorUser) { - return <Navigate to="/auth" />; - } + if (isErrorUser && isUnauthorized(userError)) { + return <Navigate to="/auth" />; + }src/components/document-dialog.tsx (2)
111-120: Allow downloads for all file-based types, not just "file"Images, PDFs, video/audio, etc. should be downloadable via the same signed URL.
const handleDownload = async () => { - if (!document || tag !== "file") return; + if (!document) return; + const downloadable = + ["file", "pdf", "image", "video", "audio", "text", "github"].includes(tag) || + ["file", "pdf", "image", "video", "audio", "text", "github"].includes(document.type); + if (!downloadable) return; const url = await generateDownloadUrl({ documentId: document._id, }); if (url) { const w = window.open(url, "_blank", "noopener,noreferrer"); if (w) w.opener = null; } };
54-104: Make preview loading race-safe and support video/audio signed URLsAvoid setting state after unmount or rapid doc switches, and add video/audio handling consistent with images/PDFs.
useEffect(() => { setPreviewUrl(null); setPreviewError(null); - const loadPreviewUrl = async () => { + let cancelled = false; + const loadPreviewUrl = async () => { if (!document) return; try { switch (tag) { case "image": case "pdf": + case "video": + case "audio": case "file": { - // Only files need download URL + // File-based types use a signed download URL const url = await generateDownloadUrl({ documentId: document._id, }); - setPreviewUrl(url); + if (!cancelled) setPreviewUrl(url); break; } case "url": case "site": { const externalUrl = document.key as string; if (isValidExternalUrl(externalUrl)) { - setPreviewUrl(externalUrl); + if (!cancelled) setPreviewUrl(externalUrl); } else { - setPreviewError("Invalid or unsafe URL"); + if (!cancelled) setPreviewError("Invalid or unsafe URL"); } break; } case "youtube": { - setPreviewUrl(`https://www.youtube.com/embed/${document.key}`); + const id = encodeURIComponent(String(document.key)); + if (!cancelled) setPreviewUrl(`https://www.youtube.com/embed/${id}`); break; } default: if (["file", "text", "github"].includes(document.type)) { const url = await generateDownloadUrl({ documentId: document._id, }); - setPreviewUrl(url); + if (!cancelled) setPreviewUrl(url); break; } else { - setPreviewUrl(document.key as string); + const maybeUrl = String(document.key ?? ""); + if (isValidExternalUrl(maybeUrl)) { + if (!cancelled) setPreviewUrl(maybeUrl); + } else { + if (!cancelled) setPreviewError("Unsupported preview source"); + } } break; } } catch (error) { - setPreviewError( + if (!cancelled) setPreviewError( `Failed to load preview: ${error instanceof Error ? error.message : "Unknown error"}`, ); } }; loadPreviewUrl(); -}, [document, tag, generateDownloadUrl]); + return () => { cancelled = true; }; +}, [document, tag, generateDownloadUrl]);
🧹 Nitpick comments (6)
src/components/topnav.tsx (2)
132-135: Use functional updater to avoid stale toggles and coalesce side-effectAlign this onClick handler with the keyboard handler above; prevents stale reads and guarantees
setSelectedArtifact(undefined)only when opening.- onClick={() => { - setResizePanelOpen(!resizePanelOpen); - setSelectedArtifact(undefined); - }} + onClick={() => { + setResizePanelOpen((open) => { + if (!open) setSelectedArtifact(undefined); + return !open; + }); + }}
56-60: Optional: clear stale user on unauthorizedPrevents UI from briefly rendering with a stale user when an auth error occurs.
useEffect(() => { if (user) { setUser(user); } }, [user, setUser]); + +useEffect(() => { + // If we start gating redirect on isUnauthorized(userError) + // (see comment above), also clear user here. + if (isErrorUser /* && isUnauthorized(userError) */) { + setUser(undefined); + } +}, [isErrorUser, setUser]);src/components/document-dialog.tsx (4)
81-84: Sanitize YouTube key with encodeURIComponentPrevents malformed IDs from breaking the URL.
- case "youtube": { - setPreviewUrl(`https://www.youtube.com/embed/${document.key}`); + case "youtube": { + const id = encodeURIComponent(String(document.key)); + setPreviewUrl(`https://www.youtube.com/embed/${id}`); break; }- const w = window.open( - `https://youtube.com/watch?v=${document.key}`, + const w = window.open( + `https://youtube.com/watch?v=${encodeURIComponent(String(document.key))}`, "_blank", "noopener,noreferrer", );Also applies to: 131-135
63-69: Align comment with behaviorThe comment says “Only files need download URL” but the code fetches URLs for image/pdf/file; tweak for accuracy.
- // Only files need download URL + // File-based types use a signed download URL
1-6: Render a shared ErrorState for preview failures (and import it)Keeps error UI consistent with the PR’s new primitives.
import { Dialog, DialogContent, DialogHeader, DialogTitle, } from "@/components/ui/dialog"; +import { ErrorState } from "@/components/ui/error-state";- {previewError ? ( - <div className="flex-grow flex items-center justify-center bg-muted rounded-md border"> - <div className="text-center text-muted-foreground"> - {previewError} - </div> - </div> - ) : previewUrl ? ( + {previewError ? ( + <div className="flex-grow flex items-center justify-center bg-muted rounded-md border"> + <ErrorState density="comfy" title="Unable to preview document" error={previewError} /> + </div> + ) : previewUrl ? (Also applies to: 167-173
173-249: Show a LoadingSpinner while resolving the preview URLImproves perceived responsiveness and aligns with the PR’s loading UX.
- ) : null} + ) : ( + <div className="flex-grow flex items-center justify-center bg-muted rounded-md border"> + <div className="text-muted-foreground"> + {/* Replace with your shared spinner */} + Loading preview… + </div> + </div> + )}If you have a shared spinner component:
+import { LoadingSpinner } from "@/components/ui/loading-spinner";- <div className="text-muted-foreground"> - {/* Replace with your shared spinner */} - Loading preview… - </div> + <LoadingSpinner />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/components/chat/messages/utils-bar/user-utils-bar.tsx(2 hunks)src/components/document-dialog.tsx(2 hunks)src/components/topnav.tsx(2 hunks)src/components/ui/tooltip-button.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ui/tooltip-button.tsx
- src/components/chat/messages/utils-bar/user-utils-bar.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/document-dialog.tsx (6)
src/store/chatStore.ts (1)
documentDialogOpenAtom(44-46)convex/documents/mutations.ts (1)
generateDownloadUrl(22-39)src/lib/helper.tsx (1)
getDocTagInfo(136-174)src/components/ui/dialog.tsx (4)
Dialog(131-131)DialogContent(133-133)DialogHeader(136-136)DialogTitle(139-139)src/lib/utils.ts (1)
formatBytes(8-16)src/components/ui/button.tsx (1)
Button(59-59)
src/components/topnav.tsx (2)
src/store/chatStore.ts (5)
resizePanelOpenAtom(37-37)selectedArtifactAtom(54-54)sidebarOpenAtom(35-35)selectedVibzMcpAtom(55-55)userAtom(11-11)convex/_generated/api.js (2)
api(21-21)api(21-21)
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores