Conversation
This comment has been minimized.
This comment has been minimized.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces chat read-only flag with ownership and explicit sharing: adds anonymousCreatorId, ChatAccess table and migrations; introduces ownership checks, claim/duplicate/share/unshare APIs; adds anonymous-id cookie handling; owner-aware UI (sharing, duplication, OG image) and related client/server routes/components. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser UI
participant Cookie as Cookie Store
participant API as Server API
participant DB as Database
Browser->>Cookie: read sb.anonymous-id
alt cookie missing
Browser->>API: request getOrCreateAnonymousId
API->>Cookie: set sb.anonymous-id
end
Browser->>API: POST /api/chat (create chat, anon or auth)
API->>DB: INSERT Chat (creatorId or anonymousCreatorId)
DB-->>API: return chat
API-->>Browser: created chat
sequenceDiagram
participant Owner as Authenticated Owner
participant UI as Share UI
participant API as Server API
participant DB as Database
Owner->>UI: open ShareChatPopover
UI->>API: GET /api/ee/chat/{chatId}/searchMembers?query=...
API->>DB: query members excluding existing ChatAccess + current user
DB-->>API: return matches
API-->>UI: display results
Owner->>UI: invite selected users
UI->>API: POST shareChatWithUsers
API->>DB: INSERT ChatAccess rows
DB-->>API: success
API-->>UI: success response
UI->>Owner: show toast, update list
sequenceDiagram
participant Anonymous as Anonymous User
participant Browser as Browser UI
participant API as Server API
participant DB as Database
Anonymous->>Browser: creates chats (sb.anonymous-id)
Browser->>API: sign in
API->>DB: authenticate -> userId
API->>API: claimAnonymousChats (reads sb.anonymous-id)
API->>DB: UPDATE Chat SET creatorId=userId WHERE anonymousCreatorId=uuid
DB-->>API: updated rows
API-->>Browser: sign-in complete
Browser->>Anonymous: owner controls visible
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Fix all issues with AI agents
In `@packages/web/src/app/`[domain]/chat/[id]/opengraph-image.tsx:
- Line 121: The current src fallback uses process.env.NEXT_PUBLIC_DOMAIN_URL ??
'http://localhost:3000' which can leak localhost into production OG images;
update the code paths that build the placeholder URL (the JSX using creatorImage
and the similar occurrence around the logo) to avoid defaulting to
'http://localhost:3000' — instead, call generateDefaultImage() when
NEXT_PUBLIC_DOMAIN_URL is undefined (or, if NODE_ENV === 'production',
explicitly throw/log a visible error), so the component uses a safe generated
image rather than a localhost URL.
In `@packages/web/src/app/`[domain]/chat/components/chatName.tsx:
- Around line 50-83: onDeleteChat and onDuplicateChat use different domain
sources which is inconsistent; change onDeleteChat to use params.domain instead
of SINGLE_TENANT_ORG_DOMAIN so both handlers route consistently. Specifically,
in the onDeleteChat callback (where router.push is called) replace the
SINGLE_TENANT_ORG_DOMAIN reference with params.domain, keeping the existing
router.push and router.refresh behavior; ensure params is in the dependency
array if not already.
In `@packages/web/src/app/`[domain]/chat/components/deleteChatDialog.tsx:
- Around line 17-25: The handleDelete callback can leave isLoading true if
onDelete throws; update the handleDelete function to wrap the await onDelete()
call in a try/catch/finally so setIsLoading(false) is always executed (use
finally) and handle errors in catch (e.g., log or show an error state) rather
than letting the exception propagate uncaught; reference the handleDelete
function and the onDelete, setIsLoading, and onOpenChange symbols when making
this change.
In `@packages/web/src/app/`[domain]/chat/components/duplicateChatDialog.tsx:
- Around line 40-48: onSubmit currently calls onDuplicate and sets isLoading
before awaiting, but if onDuplicate rejects the subsequent setIsLoading(false)
and form.reset()/onOpenChange(false) won't run; wrap the await call in a
try/finally so setIsLoading(false) always executes, and optionally add a catch
to handle or report the error (e.g., show a toast) before rethrowing or
returning; update the onSubmit function (referencing onSubmit, formSchema,
setIsLoading, onDuplicate, form.reset, onOpenChange) to use try { const
newChatId = await onDuplicate(data.name); if (newChatId) { form.reset();
onOpenChange(false); } } finally { setIsLoading(false); }.
In `@packages/web/src/app/`[domain]/chat/components/renameChatDialog.tsx:
- Around line 40-49: onSubmit can leave isLoading true if onRename throws; wrap
the await in a try/finally so setIsLoading(false) always runs. Call onRename
inside a try block (capture its boolean result into a local success variable),
call setIsLoading(false) in finally, and only call form.reset() and
onOpenChange(false) when success is true; reference the onSubmit handler,
setIsLoading, onRename, form.reset, and onOpenChange to locate the changes.
In `@packages/web/src/app/`[domain]/chat/components/shareChatPopover.tsx:
- Line 33: The local state currentVisibility initialized with
useState(visibility) in the ShareChatPopover component can go stale when the
visibility prop changes; add a useEffect that watches the visibility prop and
calls setCurrentVisibility(visibility) to keep the local state in sync, ensuring
the popover reflects prop updates (use the existing currentVisibility,
setCurrentVisibility, and visibility identifiers).
- Around line 39-59: The handler handleVisibilityChange can leave isUpdating
true if updateChatVisibility throws; wrap the async call and subsequent logic in
a try/finally so setIsUpdating(false) always runs: in handleVisibilityChange,
setIsUpdating(true) then use try { const response = await
updateChatVisibility(...); /* existing isServiceError/else toast,
setCurrentVisibility, router.refresh */ } finally { setIsUpdating(false); }
while keeping the current references to updateChatVisibility, isServiceError,
setCurrentVisibility, toast, and router.refresh.
In `@packages/web/src/app/login/components/loginForm.tsx`:
- Around line 95-99: The guest "Continue as guest" link uses the user-controlled
callbackUrl directly (in the loginForm component), opening an open-redirect
risk; compute a safeCallbackUrl before rendering (e.g., in the LoginForm
component) by validating callbackUrl is a relative path (starts with "/" but not
"//") or by parsing it and ensuring its origin matches window.location.origin,
and otherwise fallback to "/"; then use that safeCallbackUrl in the Link href
instead of callbackUrl (referencing the existing callbackUrl,
isAnonymousAccessEnabled, and Link usage).
In `@packages/web/src/features/chat/actions.ts`:
- Around line 38-56: isOwnerOfChat currently only checks anonymous ownership
when no authenticated user is present, which misses authenticated users who
still have unclaimed anonymous chats; update isOwnerOfChat (and use
getAnonymousId) to always check the anonymousCreatorId fallback regardless of
whether a User is passed (but keep the existing authenticated check first so
createdById takes precedence), i.e., after verifying chat.createdById ===
user.id, if chat.anonymousCreatorId call getAnonymousId() and compare to
chat.anonymousCreatorId and return true on match; this ensures callers like
updateChatMessages will correctly recognize an owner's unclaimed anonymous chats
without changing the ownership precedence.
In `@packages/web/src/features/chat/components/chatThread/signInPromptBanner.tsx`:
- Around line 53-55: The banner is being hidden on every stream because
isStreaming is combined unconditionally in the render-gate; change that so
streaming only blocks the initial show. Update the conditional in
signInPromptBanner.tsx that currently uses isStreaming to instead gate it with
hasShownOnce (e.g. replace isStreaming with isStreaming && !hasShownOnce or
remove isStreaming from the top-level OR and only check (isStreaming &&
!hasShownOnce)), keeping the other checks (isAuthenticated, isOwner,
isDismissed, hasMessages, hasShownOnce) intact.
🧹 Nitpick comments (10)
packages/web/src/app/[domain]/chat/components/shareChatPopover.tsx (1)
61-66:navigator.clipboard.writeTextcan reject — consider handling the error.The Clipboard API may fail due to permissions or insecure contexts. A rejected promise here would be an unhandled rejection.
Proposed fix
const handleCopyLink = useCallback(() => { - navigator.clipboard.writeText(window.location.href); - toast({ - description: "✅ Link copied to clipboard", - }); + navigator.clipboard.writeText(window.location.href).then(() => { + toast({ + description: "✅ Link copied to clipboard", + }); + }).catch(() => { + toast({ + description: "Failed to copy link", + variant: "destructive", + }); + }); }, [toast]);packages/web/src/app/[domain]/chat/components/duplicateChatDialog.tsx (1)
23-25:formSchemais recreated on every render.Move it outside the component to avoid re-creating the schema (and a new
zodResolverreference) on each render.Proposed fix
+const formSchema = z.object({ + name: z.string().min(1), +}); + export const DuplicateChatDialog = ({ isOpen, onOpenChange, onDuplicate, currentName }: DuplicateChatDialogProps) => { const [isLoading, setIsLoading] = useState(false); - - const formSchema = z.object({ - name: z.string().min(1), - });packages/db/prisma/schema.prisma (1)
447-449: No constraint ensuring at least one creator identifier is present.Both
createdByIdandanonymousCreatorIdare nullable. A chat could end up with neither set, making it an unowned orphan. If this is intentional (e.g., during migration), consider adding an application-level invariant or a database check constraint to prevent it long-term.packages/db/prisma/migrations/20260213030000_add_chat_anonymous_creator_id/migration.sql (1)
1-2: Add an index onanonymousCreatorIdto support theclaimAnonymousChatsflow.The migration is missing an index on this column. The
claimAnonymousChatsfunction (packages/web/src/features/chat/actions.ts:356–360) filters chats byanonymousCreatorIdin anupdateManyquery, which will scan the table without an index. Add:Suggested index addition
CREATE INDEX "Chat_anonymousCreatorId_idx" ON "Chat"("anonymousCreatorId");Or, if filtering is typically combined with
orgId, consider a composite index:CREATE INDEX "Chat_orgId_anonymousCreatorId_idx" ON "Chat"("orgId", "anonymousCreatorId");packages/web/src/app/[domain]/chat/components/chatName.tsx (1)
108-124: Dialogs are rendered even for non-owners.
RenameChatDialog,DeleteChatDialog, andDuplicateChatDialogare always rendered in the DOM regardless ofisOwner. They can't be opened by non-owners (since the trigger is gated byisOwner), so this is functionally safe — but it's unnecessary DOM for the non-owner case.packages/web/src/features/chat/components/chatThread/chatThread.tsx (2)
307-323: DuplicatedonDuplicatelogic across three components.This
onDuplicatehandler (callingduplicateChat, handling errors with toast, navigating to the new chat) is nearly identical to the one inchatName.tsx(lines 68–83) andchatSidePanel.tsx(lines 120–139). Consider extracting a shareduseDuplicateChathook to reduce duplication.
22-31: Duplicate import source:next/navigationis imported on both line 22 and line 31.Consolidate these into a single import statement.
Proposed fix
-import { useRouter } from 'next/navigation'; +import { useRouter, useParams } from 'next/navigation'; ... -import { useParams } from 'next/navigation';packages/web/src/app/[domain]/chat/[id]/page.tsx (2)
28-80:generateMetadataduplicates the chat DB lookup done bygetChatInfoin the page function.Both
generateMetadata(line 38) andgetChatInfo(line 95) fetch the same chat record. Since these are raw Prisma calls (notfetch), Next.js request deduplication won't apply. Consider wrapping the chat lookup inReact.cache()to deduplicate acrossgenerateMetadataand the page function within the same request.Additionally, the
as unknown as SBChatMessage[]cast on line 53 has no runtime validation. If the stored JSON shape ever diverges fromSBChatMessage, the.find()and.partsaccess on lines 54–59 could throw at runtime with an unhelpful error.
86-90:claimAnonymousChats()result is silently discarded.If claiming fails (e.g., DB error wrapped in a service error), the page proceeds normally. This is likely intentional as a best-effort operation, but a failed claim means the user won't see ownership of their anonymous chats. Consider at minimum logging the failure.
if (session) { - await claimAnonymousChats(); + const claimResult = await claimAnonymousChats(); + if (isServiceError(claimResult)) { + console.error('Failed to claim anonymous chats:', claimResult.message); + } }packages/web/src/features/chat/actions.ts (1)
348-369:claimAnonymousChatsdoes not clearanonymousCreatorIdafter claiming.After transferring ownership via
createdById, theanonymousCreatorIdremains on the chat record. This is functionally harmless today because:
- The anonymous ID is a cookie-based UUID (collision-resistant)
isOwnerOfChatcheckscreatedByIdfirst for authenticated usersHowever, if you later add additional anonymous-ownership semantics or auditing, stale
anonymousCreatorIdvalues could cause confusion. Consider nulling the field during claim for clean data hygiene.
| {/* Avatar */} | ||
| {/* eslint-disable-next-line @next/next/no-img-element */} | ||
| <img | ||
| src={creatorImage ?? `${process.env.NEXT_PUBLIC_DOMAIN_URL ?? 'http://localhost:3000'}/placeholder_avatar.png`} |
There was a problem hiding this comment.
NEXT_PUBLIC_DOMAIN_URL fallback to http://localhost:3000 could leak into production OG images.
If NEXT_PUBLIC_DOMAIN_URL isn't set in production, the placeholder avatar and logo URLs will point to localhost:3000, producing broken images. Consider failing more visibly or using generateDefaultImage() as a fallback when the env var is missing.
Also applies to: 168-168
🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/[id]/opengraph-image.tsx at line 121, The
current src fallback uses process.env.NEXT_PUBLIC_DOMAIN_URL ??
'http://localhost:3000' which can leak localhost into production OG images;
update the code paths that build the placeholder URL (the JSX using creatorImage
and the similar occurrence around the logo) to avoid defaulting to
'http://localhost:3000' — instead, call generateDefaultImage() when
NEXT_PUBLIC_DOMAIN_URL is undefined (or, if NODE_ENV === 'production',
explicitly throw/log a visible error), so the component uses a safe generated
image rather than a localhost URL.
| const onDeleteChat = useCallback(async (): Promise<boolean> => { | ||
| const response = await deleteChat({ chatId: id }); | ||
|
|
||
| if (isServiceError(response)) { | ||
| toast({ | ||
| description: `❌ Failed to delete chat. Reason: ${response.message}` | ||
| }); | ||
| return false; | ||
| } else { | ||
| toast({ | ||
| description: `✅ Chat deleted successfully` | ||
| }); | ||
| router.push(`/${SINGLE_TENANT_ORG_DOMAIN}/chat`); | ||
| router.refresh(); | ||
| return true; | ||
| } | ||
| }, [id, toast, router]); | ||
|
|
||
| const onDuplicateChat = useCallback(async (newName: string): Promise<string | null> => { | ||
| const response = await duplicateChat({ chatId: id, newName }); | ||
|
|
||
| if (isServiceError(response)) { | ||
| toast({ | ||
| description: `❌ Failed to duplicate chat. Reason: ${response.message}` | ||
| }); | ||
| return null; | ||
| } else { | ||
| toast({ | ||
| description: `✅ Chat duplicated successfully` | ||
| }); | ||
| router.push(`/${params.domain}/chat/${response.id}`); | ||
| return response.id; | ||
| } | ||
| }, [id, toast, router, params.domain]); |
There was a problem hiding this comment.
Inconsistent domain routing between onDeleteChat and onDuplicateChat.
onDeleteChat (line 62) navigates using SINGLE_TENANT_ORG_DOMAIN, while onDuplicateChat (line 80) navigates using params.domain. This should be consistent — if params.domain is the correct approach (it likely is for multi-tenant support), then onDeleteChat should also use it. Conversely, if SINGLE_TENANT_ORG_DOMAIN is intentional (e.g., this component is only used in single-tenant contexts), then onDuplicateChat should match.
Note that in chatSidePanel.tsx, both delete (line 112) and duplicate (line 136) use SINGLE_TENANT_ORG_DOMAIN, so the params.domain usage here appears to be the outlier.
Proposed fix (align to params.domain)
- router.push(`/${SINGLE_TENANT_ORG_DOMAIN}/chat`);
+ router.push(`/${params.domain}/chat`);🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/components/chatName.tsx around lines 50 -
83, onDeleteChat and onDuplicateChat use different domain sources which is
inconsistent; change onDeleteChat to use params.domain instead of
SINGLE_TENANT_ORG_DOMAIN so both handlers route consistently. Specifically, in
the onDeleteChat callback (where router.push is called) replace the
SINGLE_TENANT_ORG_DOMAIN reference with params.domain, keeping the existing
router.push and router.refresh behavior; ensure params is in the dependency
array if not already.
| const handleDelete = useCallback(async () => { | ||
| setIsLoading(true); | ||
| const success = await onDelete(); | ||
| setIsLoading(false); | ||
|
|
||
| if (success) { | ||
| onOpenChange(false); | ||
| } | ||
| }, [onDelete, onOpenChange]); |
There was a problem hiding this comment.
Missing error handling — isLoading will stick if onDelete rejects.
Same issue as in DuplicateChatDialog: if onDelete throws, setIsLoading(false) is never reached.
Proposed fix
const handleDelete = useCallback(async () => {
setIsLoading(true);
- const success = await onDelete();
- setIsLoading(false);
-
- if (success) {
- onOpenChange(false);
+ try {
+ const success = await onDelete();
+ if (success) {
+ onOpenChange(false);
+ }
+ } finally {
+ setIsLoading(false);
}
}, [onDelete, onOpenChange]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDelete = useCallback(async () => { | |
| setIsLoading(true); | |
| const success = await onDelete(); | |
| setIsLoading(false); | |
| if (success) { | |
| onOpenChange(false); | |
| } | |
| }, [onDelete, onOpenChange]); | |
| const handleDelete = useCallback(async () => { | |
| setIsLoading(true); | |
| try { | |
| const success = await onDelete(); | |
| if (success) { | |
| onOpenChange(false); | |
| } | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }, [onDelete, onOpenChange]); |
🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/components/deleteChatDialog.tsx around
lines 17 - 25, The handleDelete callback can leave isLoading true if onDelete
throws; update the handleDelete function to wrap the await onDelete() call in a
try/catch/finally so setIsLoading(false) is always executed (use finally) and
handle errors in catch (e.g., log or show an error state) rather than letting
the exception propagate uncaught; reference the handleDelete function and the
onDelete, setIsLoading, and onOpenChange symbols when making this change.
| const onSubmit = async (data: z.infer<typeof formSchema>) => { | ||
| setIsLoading(true); | ||
| const newChatId = await onDuplicate(data.name); | ||
| setIsLoading(false); | ||
|
|
||
| if (newChatId) { | ||
| form.reset(); | ||
| onOpenChange(false); | ||
| } |
There was a problem hiding this comment.
Missing error handling — isLoading will stick if onDuplicate rejects.
If onDuplicate throws, setIsLoading(false) on Line 43 is never reached, leaving the dialog permanently in a loading state with all inputs disabled.
Proposed fix using try/finally
const onSubmit = async (data: z.infer<typeof formSchema>) => {
setIsLoading(true);
- const newChatId = await onDuplicate(data.name);
- setIsLoading(false);
-
- if (newChatId) {
- form.reset();
- onOpenChange(false);
+ try {
+ const newChatId = await onDuplicate(data.name);
+ if (newChatId) {
+ form.reset();
+ onOpenChange(false);
+ }
+ } finally {
+ setIsLoading(false);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onSubmit = async (data: z.infer<typeof formSchema>) => { | |
| setIsLoading(true); | |
| const newChatId = await onDuplicate(data.name); | |
| setIsLoading(false); | |
| if (newChatId) { | |
| form.reset(); | |
| onOpenChange(false); | |
| } | |
| const onSubmit = async (data: z.infer<typeof formSchema>) => { | |
| setIsLoading(true); | |
| try { | |
| const newChatId = await onDuplicate(data.name); | |
| if (newChatId) { | |
| form.reset(); | |
| onOpenChange(false); | |
| } | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/components/duplicateChatDialog.tsx around
lines 40 - 48, onSubmit currently calls onDuplicate and sets isLoading before
awaiting, but if onDuplicate rejects the subsequent setIsLoading(false) and
form.reset()/onOpenChange(false) won't run; wrap the await call in a try/finally
so setIsLoading(false) always executes, and optionally add a catch to handle or
report the error (e.g., show a toast) before rethrowing or returning; update the
onSubmit function (referencing onSubmit, formSchema, setIsLoading, onDuplicate,
form.reset, onOpenChange) to use try { const newChatId = await
onDuplicate(data.name); if (newChatId) { form.reset(); onOpenChange(false); } }
finally { setIsLoading(false); }.
| const onSubmit = async (data: z.infer<typeof formSchema>) => { | ||
| setIsLoading(true); | ||
| const success = await onRename(data.name); | ||
| setIsLoading(false); | ||
|
|
||
| if (success) { | ||
| form.reset(); | ||
| onOpenChange(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
isLoading gets stuck if onRename throws.
If the onRename promise rejects, setIsLoading(false) on line 43 is never reached. Combined with the !isLoading guard on onOpenChange (line 55), this permanently locks the dialog open.
Proposed fix
const onSubmit = async (data: z.infer<typeof formSchema>) => {
setIsLoading(true);
- const success = await onRename(data.name);
- setIsLoading(false);
-
- if (success) {
- form.reset();
- onOpenChange(false);
+ try {
+ const success = await onRename(data.name);
+ if (success) {
+ form.reset();
+ onOpenChange(false);
+ }
+ } finally {
+ setIsLoading(false);
}
}🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/components/renameChatDialog.tsx around
lines 40 - 49, onSubmit can leave isLoading true if onRename throws; wrap the
await in a try/finally so setIsLoading(false) always runs. Call onRename inside
a try block (capture its boolean result into a local success variable), call
setIsLoading(false) in finally, and only call form.reset() and
onOpenChange(false) when success is true; reference the onSubmit handler,
setIsLoading, onRename, form.reset, and onOpenChange to locate the changes.
| } | ||
|
|
||
| export const ShareChatPopover = ({ chatId, visibility, isAuthenticated = true }: ShareChatPopoverProps) => { | ||
| const [currentVisibility, setCurrentVisibility] = useState(visibility); |
There was a problem hiding this comment.
currentVisibility can become stale if the visibility prop changes.
The local state is initialized from the prop but never synced on subsequent renders. If the parent re-renders with a different visibility (e.g., after router.refresh()), the popover will show the old value.
Proposed fix — sync state from prop
Add a useEffect to keep local state in sync:
const [currentVisibility, setCurrentVisibility] = useState(visibility);
+
+useEffect(() => {
+ setCurrentVisibility(visibility);
+}, [visibility]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [currentVisibility, setCurrentVisibility] = useState(visibility); | |
| const [currentVisibility, setCurrentVisibility] = useState(visibility); | |
| useEffect(() => { | |
| setCurrentVisibility(visibility); | |
| }, [visibility]); |
🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/components/shareChatPopover.tsx at line
33, The local state currentVisibility initialized with useState(visibility) in
the ShareChatPopover component can go stale when the visibility prop changes;
add a useEffect that watches the visibility prop and calls
setCurrentVisibility(visibility) to keep the local state in sync, ensuring the
popover reflects prop updates (use the existing currentVisibility,
setCurrentVisibility, and visibility identifiers).
| const handleVisibilityChange = useCallback(async (newVisibility: ChatVisibility) => { | ||
| setIsUpdating(true); | ||
| const response = await updateChatVisibility({ | ||
| chatId, | ||
| visibility: newVisibility, | ||
| }); | ||
|
|
||
| if (isServiceError(response)) { | ||
| toast({ | ||
| description: `Failed to update visibility: ${response.message}`, | ||
| variant: "destructive", | ||
| }); | ||
| } else { | ||
| setCurrentVisibility(newVisibility); | ||
| toast({ | ||
| description: "✅ Chat visibility updated" | ||
| }); | ||
| router.refresh(); | ||
| } | ||
| setIsUpdating(false); | ||
| }, [chatId, toast, router]); |
There was a problem hiding this comment.
isUpdating gets stuck if updateChatVisibility throws.
Same pattern as the rename dialog — wrap in try/finally so setIsUpdating(false) always runs.
Proposed fix
const handleVisibilityChange = useCallback(async (newVisibility: ChatVisibility) => {
setIsUpdating(true);
- const response = await updateChatVisibility({
- chatId,
- visibility: newVisibility,
- });
-
- if (isServiceError(response)) {
- toast({
- description: `Failed to update visibility: ${response.message}`,
- variant: "destructive",
+ try {
+ const response = await updateChatVisibility({
+ chatId,
+ visibility: newVisibility,
});
- } else {
- setCurrentVisibility(newVisibility);
- toast({
- description: "✅ Chat visibility updated"
- });
- router.refresh();
+
+ if (isServiceError(response)) {
+ toast({
+ description: `Failed to update visibility: ${response.message}`,
+ variant: "destructive",
+ });
+ } else {
+ setCurrentVisibility(newVisibility);
+ toast({
+ description: "✅ Chat visibility updated"
+ });
+ router.refresh();
+ }
+ } finally {
+ setIsUpdating(false);
}
- setIsUpdating(false);
}, [chatId, toast, router]);🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/components/shareChatPopover.tsx around
lines 39 - 59, The handler handleVisibilityChange can leave isUpdating true if
updateChatVisibility throws; wrap the async call and subsequent logic in a
try/finally so setIsUpdating(false) always runs: in handleVisibilityChange,
setIsUpdating(true) then use try { const response = await
updateChatVisibility(...); /* existing isServiceError/else toast,
setCurrentVisibility, router.refresh */ } finally { setIsUpdating(false); }
while keeping the current references to updateChatVisibility, isServiceError,
setCurrentVisibility, toast, and router.refresh.
| {isAnonymousAccessEnabled && ( | ||
| <p className="text-sm text-muted-foreground"> | ||
| <Link className="underline" href={callbackUrl ?? "/"}>Continue as guest</Link> | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
Potential open redirect via callbackUrl in guest link.
callbackUrl originates from user-controlled query parameters. The sign-in/sign-up links on lines 87 and 91 pass it through the auth flow (which typically validates callback URLs), but the "Continue as guest" link navigates directly to callbackUrl without validation. A malicious callbackUrl (e.g., https://evil.com) would redirect the user off-site.
Consider validating that callbackUrl is a relative path before using it as the href.
Proposed fix
- <Link className="underline" href={callbackUrl ?? "/"}>Continue as guest</Link>
+ <Link className="underline" href={callbackUrl?.startsWith("/") ? callbackUrl : "/"}>Continue as guest</Link>📝 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.
| {isAnonymousAccessEnabled && ( | |
| <p className="text-sm text-muted-foreground"> | |
| <Link className="underline" href={callbackUrl ?? "/"}>Continue as guest</Link> | |
| </p> | |
| )} | |
| {isAnonymousAccessEnabled && ( | |
| <p className="text-sm text-muted-foreground"> | |
| <Link className="underline" href={callbackUrl?.startsWith("/") ? callbackUrl : "/"}>Continue as guest</Link> | |
| </p> | |
| )} |
🤖 Prompt for AI Agents
In `@packages/web/src/app/login/components/loginForm.tsx` around lines 95 - 99,
The guest "Continue as guest" link uses the user-controlled callbackUrl directly
(in the loginForm component), opening an open-redirect risk; compute a
safeCallbackUrl before rendering (e.g., in the LoginForm component) by
validating callbackUrl is a relative path (starts with "/" but not "//") or by
parsing it and ensuring its origin matches window.location.origin, and otherwise
fallback to "/"; then use that safeCallbackUrl in the Link href instead of
callbackUrl (referencing the existing callbackUrl, isAnonymousAccessEnabled, and
Link usage).
| /** | ||
| * Checks if the current user (authenticated or anonymous) is the owner of a chat. | ||
| */ | ||
| export const isOwnerOfChat = async (chat: Chat, user: User | undefined): Promise<boolean> => { | ||
| // Authenticated user owns the chat | ||
| if (user && chat.createdById === user.id) { | ||
| return true; | ||
| } | ||
|
|
||
| // Anonymous user owns the chat (check cookie-based anonymous ID) | ||
| if (!user && chat.anonymousCreatorId) { | ||
| const anonymousId = await getAnonymousId(); | ||
| if (anonymousId && chat.anonymousCreatorId === anonymousId) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
isOwnerOfChat won't match an authenticated user's unclaimed anonymous chats.
The !user guard on line 48 means anonymous ownership is only checked for unauthenticated users. If an authenticated user has anonymous chats that haven't been claimed yet (e.g., claimAnonymousChats failed or wasn't called), isOwnerOfChat returns false for their own chats.
This is mitigated by page.tsx calling claimAnonymousChats() before getChatInfo(), but other call sites (e.g., updateChatMessages from the chat API route) may not have this guarantee. If claiming silently fails (which the current code allows — see the unchecked result in page.tsx), the user could be locked out of their own chat mid-conversation.
Consider also checking the anonymous cookie for authenticated users as a fallback:
Proposed fix
export const isOwnerOfChat = async (chat: Chat, user: User | undefined): Promise<boolean> => {
// Authenticated user owns the chat
if (user && chat.createdById === user.id) {
return true;
}
- // Anonymous user owns the chat (check cookie-based anonymous ID)
- if (!user && chat.anonymousCreatorId) {
+ // Check cookie-based anonymous ID (for both anonymous users and
+ // authenticated users whose chats haven't been claimed yet)
+ if (chat.anonymousCreatorId) {
const anonymousId = await getAnonymousId();
if (anonymousId && chat.anonymousCreatorId === anonymousId) {
return true;
}
}
return false;
};📝 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.
| /** | |
| * Checks if the current user (authenticated or anonymous) is the owner of a chat. | |
| */ | |
| export const isOwnerOfChat = async (chat: Chat, user: User | undefined): Promise<boolean> => { | |
| // Authenticated user owns the chat | |
| if (user && chat.createdById === user.id) { | |
| return true; | |
| } | |
| // Anonymous user owns the chat (check cookie-based anonymous ID) | |
| if (!user && chat.anonymousCreatorId) { | |
| const anonymousId = await getAnonymousId(); | |
| if (anonymousId && chat.anonymousCreatorId === anonymousId) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| }; | |
| export const isOwnerOfChat = async (chat: Chat, user: User | undefined): Promise<boolean> => { | |
| // Authenticated user owns the chat | |
| if (user && chat.createdById === user.id) { | |
| return true; | |
| } | |
| // Check cookie-based anonymous ID (for both anonymous users and | |
| // authenticated users whose chats haven't been claimed yet) | |
| if (chat.anonymousCreatorId) { | |
| const anonymousId = await getAnonymousId(); | |
| if (anonymousId && chat.anonymousCreatorId === anonymousId) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| }; |
🤖 Prompt for AI Agents
In `@packages/web/src/features/chat/actions.ts` around lines 38 - 56,
isOwnerOfChat currently only checks anonymous ownership when no authenticated
user is present, which misses authenticated users who still have unclaimed
anonymous chats; update isOwnerOfChat (and use getAnonymousId) to always check
the anonymousCreatorId fallback regardless of whether a User is passed (but keep
the existing authenticated check first so createdById takes precedence), i.e.,
after verifying chat.createdById === user.id, if chat.anonymousCreatorId call
getAnonymousId() and compare to chat.anonymousCreatorId and return true on
match; this ensures callers like updateChatMessages will correctly recognize an
owner's unclaimed anonymous chats without changing the ownership precedence.
| if (isAuthenticated || !isOwner || isDismissed || !hasMessages || isStreaming || !hasShownOnce) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Banner hides during subsequent streaming after the first show.
Once hasShownOnce is true, the isStreaming condition on Line 53 will still hide the banner during follow-up message streams. If the user hasn't dismissed it, the banner flickers away and back each time a response streams. Consider removing isStreaming from the render-gate once hasShownOnce is true:
- if (isAuthenticated || !isOwner || isDismissed || !hasMessages || isStreaming || !hasShownOnce) {
+ if (isAuthenticated || !isOwner || isDismissed || !hasMessages || !hasShownOnce) {The isStreaming guard is already captured in the useEffect that sets hasShownOnce, so it only needs to delay the first appearance.
📝 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 (isAuthenticated || !isOwner || isDismissed || !hasMessages || isStreaming || !hasShownOnce) { | |
| return null; | |
| } | |
| if (isAuthenticated || !isOwner || isDismissed || !hasMessages || !hasShownOnce) { | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In `@packages/web/src/features/chat/components/chatThread/signInPromptBanner.tsx`
around lines 53 - 55, The banner is being hidden on every stream because
isStreaming is combined unconditionally in the render-gate; change that so
streaming only blocks the initial show. Update the conditional in
signInPromptBanner.tsx that currently uses isStreaming to instead gate it with
hasShownOnce (e.g. replace isStreaming with isStreaming && !hasShownOnce or
remove isStreaming from the top-level OR and only check (isStreaming &&
!hasShownOnce)), keeping the other checks (isAuthenticated, isOwner,
isDismissed, hasMessages, hasShownOnce) intact.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if (!user && chat.anonymousCreatorId) { | ||
| const anonymousId = await getAnonymousId(); | ||
| if (anonymousId && chat.anonymousCreatorId === anonymousId) { | ||
| return true; |
There was a problem hiding this comment.
Claimed chats remain anonymously editable
Medium Severity
_isOwnerOfChat grants owner rights to any unauthenticated request whose cookie matches chat.anonymousCreatorId, even after claimAnonymousChats sets createdById. Since claimAnonymousChats does not clear anonymousCreatorId, claimed chats can still be modified anonymously from the same browser/session cookie.
Additional Locations (1)
| }); | ||
| return false; | ||
| } else { | ||
| setSharedWithUsers(sharedWithUsers.filter(user => user.id !== userId)); |
There was a problem hiding this comment.
Concurrent share updates can overwrite user list
Low Severity
setSharedWithUsers uses captured sharedWithUsers in both unshare and share handlers. When multiple share/unshare requests resolve out of order, later updates can apply to stale state and undo earlier changes in the popover list, so displayed access can become incorrect until a full refresh.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Around line 25-35: Add a language specifier to the fenced code block showing
file name examples to satisfy markdownlint MD040; locate the triple-backtick
block that contains the "Correct/Incorrect" file name list (the block with
shareChatPopover.tsx, UserAvatar.tsx, etc.) and change the opening fence from
``` to ```text (or ```plaintext) so the block is treated as plain text.
In `@packages/db/tools/scripts/inject-user-data.ts`:
- Line 34: Remove the redundant confirmAction() invocation from the individual
scripts (e.g., inject-user-data.ts, migrate-duplicate-connections.ts,
inject-audit-data.ts); the confirmation is already handled in scriptRunner.ts
before calling selectedScript.run(prisma). Edit inject-user-data.ts to delete
the standalone confirmAction() call so the script relies on scriptRunner.ts’s
confirmation flow (refer to confirmAction() and selectedScript.run(prisma) in
scriptRunner.ts to verify the centralized confirmation behavior).
In `@packages/web/src/app/`[domain]/chat/[id]/page.tsx:
- Around line 86-90: claimAnonymousChats() is being awaited but its result/error
is ignored, so failures can leave chat ownership incorrect before getChatInfo()
runs; update the code around the session check to inspect the return value or
catch errors from claimAnonymousChats() (referencing claimAnonymousChats and
getChatInfo) and handle failures by either logging the error with context via
the existing logger and/or short-circuiting (throw or return an error page) so
getChatInfo() does not proceed on a claim failure; ensure any caught error
includes the original error details in the log and a clear message that claiming
anonymous chats failed.
- Around line 52-64: The code unsafely casts chat.messages to SBChatMessage[]
and accesses properties that can throw at runtime (this runs in
generateMetadata); replace the direct cast and blind property access by
defensively validating chat.messages is an array and each item matches the
minimal shape before use: check Array.isArray(chat.messages), filter/map items
where typeof m === 'object' && m?.role === 'user' && Array.isArray(m.parts',
then find a part where typeof p === 'object' && p.type === 'text' && typeof
p.text === 'string'; use optional chaining and fallbacks to produce description
(keep 'A chat on Sourcebot' or 'Untitled chat' when validation fails). Reference
symbols: chat.messages, SBChatMessage, firstUserMessage, description,
generateMetadata.
In `@packages/web/src/app/`[domain]/chat/components/shareChatPopover/index.tsx:
- Around line 76-96: The onShareChatWithUsers callback closes over
sharedWithUsers causing stale updates; change the
setSharedWithUsers([...sharedWithUsers, ...users]) call to the functional
updater form setSharedWithUsers(prev => [...prev, ...users]) to avoid race
conditions, and update the useCallback dependency array to remove
sharedWithUsers (keeping chatId and toast only) so the hook doesn't depend on
the mutable array reference.
- Around line 58-73: The onUnshareChatWithUser callback captures sharedWithUsers
and can suffer from a stale-closure when removals happen rapidly; update
setSharedWithUsers to use the functional updater form (prev => prev.filter(u =>
u.id !== userId)) so each removal derives from the latest state, and remove
sharedWithUsers from the useCallback dependency array (keep chatId and toast) to
avoid recreating the callback unnecessarily.
In
`@packages/web/src/app/`[domain]/chat/components/shareChatPopover/invitePanel.tsx:
- Around line 183-187: The click handler sets isInviting via setIsInviting(true)
but never resets it if onShareChatWithUsers throws; update the onClick async
function that calls onShareChatWithUsers(selectedUsers) to use a try/finally:
setIsInviting(true) before the try, await onShareChatWithUsers(...) inside the
try, and call setIsInviting(false) in the finally block so isInviting is always
cleared even on errors.
In
`@packages/web/src/app/`[domain]/chat/components/shareChatPopover/shareSettings.tsx:
- Around line 153-159: The onValueChange handler for the Select currently sets
setIsVisibilityUpdating(true) then awaits onVisibilityChange but does not guard
against exceptions, so setIsVisibilityUpdating(false) can be skipped; wrap the
await call in a try/finally (or try/catch/finally) inside the onValueChange
callback in shareSettings.tsx so that setIsVisibilityUpdating(false) always runs
regardless of whether onVisibilityChange(value as ChatVisibility) throws,
preserving proper UI state for the Select component.
- Around line 45-50: handleCopyLink currently calls
navigator.clipboard.writeText without handling its returned promise; update the
handleCopyLink function to be async (or return a promise) and await
navigator.clipboard.writeText(window.location.href) inside a try/catch, first
checking for navigator.clipboard existence, then show the existing success toast
on resolve and a failure toast (including a brief error message) on reject;
ensure you reference handleCopyLink and navigator.clipboard.writeText and keep
the toast dependency usage intact.
In `@packages/web/src/features/chat/actions.ts`:
- Around line 544-551: The prisma.chatAccess.delete call in the action that
removes a user's access can throw Prisma P2025 if the record is already gone;
update the code in the relevant action to either use
prisma.chatAccess.deleteMany({ where: { chatId, userId } }) so deletion is
idempotent (returns count without throwing) or wrap the
prisma.chatAccess.delete(...) call in a try/catch and ignore P2025 errors;
reference the prisma.chatAccess.delete usage in this action to locate and
replace/guard it accordingly.
🧹 Nitpick comments (10)
packages/db/prisma/schema.prisma (1)
467-479: Consider adding an index onuserIdforChatAccess.The unique constraint on
(chatId, userId)supports lookups bychatIdfirst. However, queries like "find all chats shared with user X" (e.g.,_hasSharedAccessinactions.tsuseschatId_userIdwhich is fine, but a future chat-listing feature would filter byuserIdalone) would benefit from an explicit@@index([userId]). At scale this could become a sequential scan.Suggested addition
@@unique([chatId, userId]) + @@index([userId]) }packages/web/src/app/api/(server)/chat/[chatId]/searchMembers/route.ts (1)
82-105: Preferselectoverincludeto avoid fetching sensitive columns likehashedPassword.
include: { user: true }fetches allUsercolumns (includinghashedPassword) into server memory, even though onlyid,name, andimageare used in the response mapping. Usingselectreduces the data transferred from the DB and avoids holding sensitive fields in memory.Suggested change
const members = await prisma.userToOrg.findMany({ where: { orgId: org.id, userId: { notIn: Array.from(excludeUserIds), }, user: { OR: [ { name: { contains: query, mode: 'insensitive' } }, { email: { contains: query, mode: 'insensitive' } }, ], }, }, - include: { - user: true, + select: { + userId: true, + user: { + select: { + id: true, + email: true, + name: true, + image: true, + }, + }, }, });packages/web/src/app/[domain]/chat/components/shareChatPopover/invitePanel.tsx (1)
49-52: ForwardAbortSignalfrom react-query'squeryFncontext to enable request cancellation.
@tanstack/react-querypasses a signal via thequeryFncontext, which can cancel in-flight requests when the query key changes (e.g., during rapid typing). Currently the signal parameter accepted bysearchChatShareableMembersis unused.Suggested fix
const { data: searchResults, isPending, isError } = useQuery<SearchChatShareableMembersResponse>({ queryKey: ['search-chat-shareable-members', chatId, debouncedSearchQuery], - queryFn: () => unwrapServiceError(searchChatShareableMembers({ chatId, query: debouncedSearchQuery})) + queryFn: ({ signal }) => unwrapServiceError(searchChatShareableMembers({ chatId, query: debouncedSearchQuery}, signal)) })packages/web/src/features/chat/actions.ts (2)
250-266: Inconsistent authorization: usescreatedByIddirectly instead of_isOwnerOfChat.
updateChatVisibilitycheckschat.createdById !== user.idwhileupdateChatMessages,updateChatName, etc., use_isOwnerOfChat. Since this function useswithAuthV2(authenticated only), the result is the same today, but the inconsistency makes it fragile if_isOwnerOfChatlogic evolves (e.g., to support co-owners or delegated admin). The same applies todeleteChat(line 348).Suggested fix
- // Only the creator can change visibility - if (chat.createdById !== user.id) { + const isOwner = await _isOwnerOfChat(chat, user); + if (!isOwner) { return notFound(); }
370-391:claimAnonymousChatsdoesn't clearanonymousCreatorIdafter claiming.After transferring ownership by setting
createdById, theanonymousCreatorIdremains. While the current_isOwnerOfChatlogic guards against misuse (the!usercheck), clearing it would be cleaner and more defensive against future changes.Suggested addition
const result = await prisma.chat.updateMany({ where: { orgId: org.id, anonymousCreatorId: anonymousId, createdById: null, }, data: { createdById: user.id, + anonymousCreatorId: null, }, });CLAUDE.md (1)
68-85:apiHandlerwrapper used without introduction.The
apiHandlerfunction appears in the API route examples (Lines 68, 99, 171) but is never described — no import path, purpose, or explanation of what it provides (e.g., error boundary, logging). Consider adding a brief note or import statement so developers know where it comes from.packages/web/src/app/[domain]/chat/[id]/page.tsx (1)
92-96: Independent data fetches could be parallelized.
getConfiguredLanguageModelsInfo,getRepos,getSearchContexts, andgetChatInfo(Lines 92-95) are independent of each other (onlyclaimAnonymousChatsneeded to precedegetChatInfo). Running them withPromise.allwould reduce waterfall latency.Proposed refactor
- const languageModels = await getConfiguredLanguageModelsInfo(); - const repos = await getRepos(); - const searchContexts = await getSearchContexts(params.domain); - const chatInfo = await getChatInfo({ chatId: params.id }); - const chatHistory = session ? await getUserChatHistory() : []; + const [languageModels, repos, searchContexts, chatInfo, chatHistory] = await Promise.all([ + getConfiguredLanguageModelsInfo(), + getRepos(), + getSearchContexts(params.domain), + getChatInfo({ chatId: params.id }), + session ? getUserChatHistory() : Promise.resolve([]), + ]);packages/web/src/app/[domain]/chat/components/shareChatPopover/shareSettings.tsx (2)
52-60:getInitialsis duplicated ininvitePanel.tsx.The exact same helper exists in
invitePanel.tsx(visible in the relevant code snippets). Extract it to a shared utility to keep things DRY.
186-194: Placeholderhref="#"on the info link.The
@todo: link to docscomment (Line 187) leaves a dead link in the shipped UI. Clicking it scrolls to the top of the page, which is confusing. Consider hiding the link entirely until the docs URL is available, or usinghrefwith a real URL.Do you want me to open an issue to track adding the documentation link?
packages/web/src/app/[domain]/chat/components/shareChatPopover/index.tsx (1)
98-106: Hardcoded 100ms timeout for view reset is fragile.The delay relies on the popover's close animation completing within 100ms. If the animation duration changes or varies across devices, the view could reset visibly before or after the animation ends. Consider using the popover's
onCloseAutoFocusor anonAnimationEndcallback instead, or at minimum make the delay a named constant.
| ``` | ||
| // Correct | ||
| shareChatPopover.tsx | ||
| userAvatar.tsx | ||
| apiClient.ts | ||
|
|
||
| // Incorrect | ||
| ShareChatPopover.tsx | ||
| UserAvatar.tsx | ||
| share-chat-popover.tsx | ||
| ``` |
There was a problem hiding this comment.
Fenced code block missing language specifier.
The code block on Line 25 lacks a language identifier, which was flagged by markdownlint (MD040). Since this block shows file naming examples (not executable code), specifying text or plaintext would satisfy the linter.
Proposed fix
-```
+```text
// Correct
shareChatPopover.tsx🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@CLAUDE.md` around lines 25 - 35, Add a language specifier to the fenced code
block showing file name examples to satisfy markdownlint MD040; locate the
triple-backtick block that contains the "Correct/Incorrect" file name list (the
block with shareChatPopover.tsx, UserAvatar.tsx, etc.) and change the opening
fence from ``` to ```text (or ```plaintext) so the block is treated as plain
text.
|
|
||
| console.log(`Injecting ${mockUsers.length} mock users for organization: ${org.name} (${org.domain})`); | ||
|
|
||
| confirmAction(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find all confirmAction calls in the scripts directory
rg -n 'confirmAction' packages/db/tools/scripts/Repository: sourcebot-dev/sourcebot
Length of output: 586
🏁 Script executed:
# Check scriptRunner.ts to see the flow around line 39
rg -B 5 -A 5 'selectedScript.run' packages/db/tools/Repository: sourcebot-dev/sourcebot
Length of output: 616
🏁 Script executed:
# Look at the full scriptRunner.ts to understand the confirmation flow
fd 'scriptRunner.ts' packages/db/tools/ -x cat -n {}Repository: sourcebot-dev/sourcebot
Length of output: 1973
Remove redundant confirmAction() call — confirmation already happens in scriptRunner.
confirmAction() is called in scriptRunner.ts (line 39) before invoking selectedScript.run(prisma) (line 45). Calling it again inside the script means the operator must confirm twice. This pattern exists in all three scripts: migrate-duplicate-connections.ts, inject-audit-data.ts, and inject-user-data.ts. Remove the call from individual scripts and rely on the runner's confirmation.
Proposed fix
- confirmAction();
-
const createdUsers: { id: string; email: string | null; name: string | null }[] = [];📝 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.
| confirmAction(); |
🤖 Prompt for AI Agents
In `@packages/db/tools/scripts/inject-user-data.ts` at line 34, Remove the
redundant confirmAction() invocation from the individual scripts (e.g.,
inject-user-data.ts, migrate-duplicate-connections.ts, inject-audit-data.ts);
the confirmation is already handled in scriptRunner.ts before calling
selectedScript.run(prisma). Edit inject-user-data.ts to delete the standalone
confirmAction() call so the script relies on scriptRunner.ts’s confirmation flow
(refer to confirmAction() and selectedScript.run(prisma) in scriptRunner.ts to
verify the centralized confirmation behavior).
| const chatName = chat.name ?? 'Untitled chat'; | ||
| const messages = chat.messages as unknown as SBChatMessage[]; | ||
| const firstUserMessage = messages.find(m => m.role === 'user'); | ||
|
|
||
| let description = 'A chat on Sourcebot'; | ||
| if (firstUserMessage) { | ||
| const textPart = firstUserMessage.parts.find(p => p.type === 'text'); | ||
| if (textPart && textPart.type === 'text') { | ||
| description = textPart.text.length > 160 | ||
| ? textPart.text.substring(0, 160).trim() + '...' | ||
| : textPart.text; | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsafe cast of JSON column to typed array.
chat.messages as unknown as SBChatMessage[] (Line 53) and the subsequent property access chain (m.role, m.parts, p.type, p.text) will throw at runtime if the stored JSON doesn't match the expected shape — e.g., a legacy chat or corrupted row. Since this runs in generateMetadata it would break the page load entirely.
Consider defensive access:
Proposed fix
- const messages = chat.messages as unknown as SBChatMessage[];
- const firstUserMessage = messages.find(m => m.role === 'user');
-
let description = 'A chat on Sourcebot';
- if (firstUserMessage) {
- const textPart = firstUserMessage.parts.find(p => p.type === 'text');
- if (textPart && textPart.type === 'text') {
- description = textPart.text.length > 160
- ? textPart.text.substring(0, 160).trim() + '...'
- : textPart.text;
+ try {
+ const messages = chat.messages as unknown as SBChatMessage[];
+ const firstUserMessage = messages.find(m => m.role === 'user');
+ if (firstUserMessage) {
+ const textPart = firstUserMessage.parts.find(p => p.type === 'text');
+ if (textPart && textPart.type === 'text') {
+ description = textPart.text.length > 160
+ ? textPart.text.substring(0, 160).trim() + '...'
+ : textPart.text;
+ }
}
- }
+ } catch {
+ // Fall back to default description if messages are malformed
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const chatName = chat.name ?? 'Untitled chat'; | |
| const messages = chat.messages as unknown as SBChatMessage[]; | |
| const firstUserMessage = messages.find(m => m.role === 'user'); | |
| let description = 'A chat on Sourcebot'; | |
| if (firstUserMessage) { | |
| const textPart = firstUserMessage.parts.find(p => p.type === 'text'); | |
| if (textPart && textPart.type === 'text') { | |
| description = textPart.text.length > 160 | |
| ? textPart.text.substring(0, 160).trim() + '...' | |
| : textPart.text; | |
| } | |
| } | |
| const chatName = chat.name ?? 'Untitled chat'; | |
| let description = 'A chat on Sourcebot'; | |
| try { | |
| const messages = chat.messages as unknown as SBChatMessage[]; | |
| const firstUserMessage = messages.find(m => m.role === 'user'); | |
| if (firstUserMessage) { | |
| const textPart = firstUserMessage.parts.find(p => p.type === 'text'); | |
| if (textPart && textPart.type === 'text') { | |
| description = textPart.text.length > 160 | |
| ? textPart.text.substring(0, 160).trim() + '...' | |
| : textPart.text; | |
| } | |
| } | |
| } catch { | |
| // Fall back to default description if messages are malformed | |
| } |
🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/[id]/page.tsx around lines 52 - 64, The
code unsafely casts chat.messages to SBChatMessage[] and accesses properties
that can throw at runtime (this runs in generateMetadata); replace the direct
cast and blind property access by defensively validating chat.messages is an
array and each item matches the minimal shape before use: check
Array.isArray(chat.messages), filter/map items where typeof m === 'object' &&
m?.role === 'user' && Array.isArray(m.parts', then find a part where typeof p
=== 'object' && p.type === 'text' && typeof p.text === 'string'; use optional
chaining and fallbacks to produce description (keep 'A chat on Sourcebot' or
'Untitled chat' when validation fails). Reference symbols: chat.messages,
SBChatMessage, firstUserMessage, description, generateMetadata.
| // Claim any anonymous chats created by this user before they signed in. | ||
| // This must happen before getChatInfo so the chat ownership is updated. | ||
| if (session) { | ||
| await claimAnonymousChats(); | ||
| } |
There was a problem hiding this comment.
claimAnonymousChats result is silently discarded.
The comment on Line 87 states this must happen before getChatInfo so ownership is correct, but if claimAnonymousChats returns a service error, the page proceeds as if nothing happened. This could leave the user unable to see their own chat as "owned."
Consider checking the result or at minimum logging the failure:
Proposed fix
if (session) {
- await claimAnonymousChats();
+ const claimResult = await claimAnonymousChats();
+ if (isServiceError(claimResult)) {
+ console.error('Failed to claim anonymous chats:', claimResult.message);
+ }
}📝 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.
| // Claim any anonymous chats created by this user before they signed in. | |
| // This must happen before getChatInfo so the chat ownership is updated. | |
| if (session) { | |
| await claimAnonymousChats(); | |
| } | |
| // Claim any anonymous chats created by this user before they signed in. | |
| // This must happen before getChatInfo so the chat ownership is updated. | |
| if (session) { | |
| const claimResult = await claimAnonymousChats(); | |
| if (isServiceError(claimResult)) { | |
| console.error('Failed to claim anonymous chats:', claimResult.message); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/[id]/page.tsx around lines 86 - 90,
claimAnonymousChats() is being awaited but its result/error is ignored, so
failures can leave chat ownership incorrect before getChatInfo() runs; update
the code around the session check to inspect the return value or catch errors
from claimAnonymousChats() (referencing claimAnonymousChats and getChatInfo) and
handle failures by either logging the error with context via the existing logger
and/or short-circuiting (throw or return an error page) so getChatInfo() does
not proceed on a claim failure; ensure any caught error includes the original
error details in the log and a clear message that claiming anonymous chats
failed.
| const onUnshareChatWithUser = useCallback(async (userId: string) => { | ||
| const response = await unshareChatWithUser({ chatId, userId }); | ||
| if (isServiceError(response)) { | ||
| toast({ | ||
| description: `Failed to remove invited user: ${response.message}`, | ||
| variant: "destructive", | ||
| }); | ||
| return false; | ||
| } else { | ||
| setSharedWithUsers(sharedWithUsers.filter(user => user.id !== userId)); | ||
| toast({ | ||
| description: "✅ Access removed" | ||
| }); | ||
| return true; | ||
| } | ||
| }, [chatId, toast, sharedWithUsers]); |
There was a problem hiding this comment.
Stale closure in setSharedWithUsers — rapid removals can lose updates.
sharedWithUsers is captured from the closure at callback creation time. If two removals fire in quick succession (the second starts before the first's setSharedWithUsers triggers a re-render), the second removal operates on stale state, potentially re-adding the first removed user.
Use the functional updater form to derive from the latest state:
Proposed fix
} else {
- setSharedWithUsers(sharedWithUsers.filter(user => user.id !== userId));
+ setSharedWithUsers(prev => prev.filter(user => user.id !== userId));
toast({
description: "✅ Access removed"
});
return true;
}
- }, [chatId, toast, sharedWithUsers]);
+ }, [chatId, toast]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onUnshareChatWithUser = useCallback(async (userId: string) => { | |
| const response = await unshareChatWithUser({ chatId, userId }); | |
| if (isServiceError(response)) { | |
| toast({ | |
| description: `Failed to remove invited user: ${response.message}`, | |
| variant: "destructive", | |
| }); | |
| return false; | |
| } else { | |
| setSharedWithUsers(sharedWithUsers.filter(user => user.id !== userId)); | |
| toast({ | |
| description: "✅ Access removed" | |
| }); | |
| return true; | |
| } | |
| }, [chatId, toast, sharedWithUsers]); | |
| const onUnshareChatWithUser = useCallback(async (userId: string) => { | |
| const response = await unshareChatWithUser({ chatId, userId }); | |
| if (isServiceError(response)) { | |
| toast({ | |
| description: `Failed to remove invited user: ${response.message}`, | |
| variant: "destructive", | |
| }); | |
| return false; | |
| } else { | |
| setSharedWithUsers(prev => prev.filter(user => user.id !== userId)); | |
| toast({ | |
| description: "✅ Access removed" | |
| }); | |
| return true; | |
| } | |
| }, [chatId, toast]); |
🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/components/shareChatPopover/index.tsx
around lines 58 - 73, The onUnshareChatWithUser callback captures
sharedWithUsers and can suffer from a stale-closure when removals happen
rapidly; update setSharedWithUsers to use the functional updater form (prev =>
prev.filter(u => u.id !== userId)) so each removal derives from the latest
state, and remove sharedWithUsers from the useCallback dependency array (keep
chatId and toast) to avoid recreating the callback unnecessarily.
| const onShareChatWithUsers = useCallback(async (users: SessionUser[]) => { | ||
| if (users.length === 0) { | ||
| return false; | ||
| } | ||
| const response = await shareChatWithUsers({ chatId, userIds: users.map(user => user.id) }); | ||
|
|
||
| if (isServiceError(response)) { | ||
| toast({ | ||
| description: `Failed to share with ${users.length} user${users.length > 1 ? 's' : ''}`, | ||
| variant: "destructive", | ||
| }); | ||
| return false; | ||
| } else { | ||
| setSharedWithUsers([...sharedWithUsers, ...users]); | ||
| toast({ | ||
| description: `✅ Invited ${users.length} user${users.length > 1 ? 's' : ''}` | ||
| }); | ||
| setView('main'); | ||
| return true; | ||
| } | ||
| }, [chatId, toast, sharedWithUsers]); |
There was a problem hiding this comment.
Same stale-closure issue in onShareChatWithUsers.
Same pattern — sharedWithUsers from the closure is spread into the new array, so a share-then-remove (or two rapid shares) can produce an incorrect list.
Proposed fix
} else {
- setSharedWithUsers([...sharedWithUsers, ...users]);
+ setSharedWithUsers(prev => [...prev, ...users]);
toast({
description: `✅ Invited ${users.length} user${users.length > 1 ? 's' : ''}`
});
setView('main');
return true;
}
- }, [chatId, toast, sharedWithUsers]);
+ }, [chatId, toast]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onShareChatWithUsers = useCallback(async (users: SessionUser[]) => { | |
| if (users.length === 0) { | |
| return false; | |
| } | |
| const response = await shareChatWithUsers({ chatId, userIds: users.map(user => user.id) }); | |
| if (isServiceError(response)) { | |
| toast({ | |
| description: `Failed to share with ${users.length} user${users.length > 1 ? 's' : ''}`, | |
| variant: "destructive", | |
| }); | |
| return false; | |
| } else { | |
| setSharedWithUsers([...sharedWithUsers, ...users]); | |
| toast({ | |
| description: `✅ Invited ${users.length} user${users.length > 1 ? 's' : ''}` | |
| }); | |
| setView('main'); | |
| return true; | |
| } | |
| }, [chatId, toast, sharedWithUsers]); | |
| const onShareChatWithUsers = useCallback(async (users: SessionUser[]) => { | |
| if (users.length === 0) { | |
| return false; | |
| } | |
| const response = await shareChatWithUsers({ chatId, userIds: users.map(user => user.id) }); | |
| if (isServiceError(response)) { | |
| toast({ | |
| description: `Failed to share with ${users.length} user${users.length > 1 ? 's' : ''}`, | |
| variant: "destructive", | |
| }); | |
| return false; | |
| } else { | |
| setSharedWithUsers(prev => [...prev, ...users]); | |
| toast({ | |
| description: `✅ Invited ${users.length} user${users.length > 1 ? 's' : ''}` | |
| }); | |
| setView('main'); | |
| return true; | |
| } | |
| }, [chatId, toast]); |
🤖 Prompt for AI Agents
In `@packages/web/src/app/`[domain]/chat/components/shareChatPopover/index.tsx
around lines 76 - 96, The onShareChatWithUsers callback closes over
sharedWithUsers causing stale updates; change the
setSharedWithUsers([...sharedWithUsers, ...users]) call to the functional
updater form setSharedWithUsers(prev => [...prev, ...users]) to avoid race
conditions, and update the useCallback dependency array to remove
sharedWithUsers (keeping chatId and toast only) so the hook doesn't depend on
the mutable array reference.
| onClick={async () => { | ||
| setIsInviting(true); | ||
| await onShareChatWithUsers(selectedUsers); | ||
| setIsInviting(false); | ||
| }} |
There was a problem hiding this comment.
isInviting will remain true forever if onShareChatWithUsers throws.
Wrap in try/finally so the loading state always resets.
Suggested fix
onClick={async () => {
setIsInviting(true);
- await onShareChatWithUsers(selectedUsers);
- setIsInviting(false);
+ try {
+ await onShareChatWithUsers(selectedUsers);
+ } finally {
+ setIsInviting(false);
+ }
}}📝 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.
| onClick={async () => { | |
| setIsInviting(true); | |
| await onShareChatWithUsers(selectedUsers); | |
| setIsInviting(false); | |
| }} | |
| onClick={async () => { | |
| setIsInviting(true); | |
| try { | |
| await onShareChatWithUsers(selectedUsers); | |
| } finally { | |
| setIsInviting(false); | |
| } | |
| }} |
🤖 Prompt for AI Agents
In
`@packages/web/src/app/`[domain]/chat/components/shareChatPopover/invitePanel.tsx
around lines 183 - 187, The click handler sets isInviting via
setIsInviting(true) but never resets it if onShareChatWithUsers throws; update
the onClick async function that calls onShareChatWithUsers(selectedUsers) to use
a try/finally: setIsInviting(true) before the try, await
onShareChatWithUsers(...) inside the try, and call setIsInviting(false) in the
finally block so isInviting is always cleared even on errors.
| const handleCopyLink = useCallback(() => { | ||
| navigator.clipboard.writeText(window.location.href); | ||
| toast({ | ||
| description: "✅ Link copied to clipboard", | ||
| }); | ||
| }, [toast]); |
There was a problem hiding this comment.
navigator.clipboard.writeText can reject — unhandled promise.
The Clipboard API can throw (e.g., document not focused, permissions denied, or unsupported in older browsers). An unhandled rejection will silently fail with no user feedback.
Proposed fix
const handleCopyLink = useCallback(() => {
- navigator.clipboard.writeText(window.location.href);
- toast({
- description: "✅ Link copied to clipboard",
- });
+ navigator.clipboard.writeText(window.location.href)
+ .then(() => {
+ toast({
+ description: "✅ Link copied to clipboard",
+ });
+ })
+ .catch(() => {
+ toast({
+ description: "Failed to copy link",
+ variant: "destructive",
+ });
+ });
}, [toast]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleCopyLink = useCallback(() => { | |
| navigator.clipboard.writeText(window.location.href); | |
| toast({ | |
| description: "✅ Link copied to clipboard", | |
| }); | |
| }, [toast]); | |
| const handleCopyLink = useCallback(() => { | |
| navigator.clipboard.writeText(window.location.href) | |
| .then(() => { | |
| toast({ | |
| description: "✅ Link copied to clipboard", | |
| }); | |
| }) | |
| .catch(() => { | |
| toast({ | |
| description: "Failed to copy link", | |
| variant: "destructive", | |
| }); | |
| }); | |
| }, [toast]); |
🤖 Prompt for AI Agents
In
`@packages/web/src/app/`[domain]/chat/components/shareChatPopover/shareSettings.tsx
around lines 45 - 50, handleCopyLink currently calls
navigator.clipboard.writeText without handling its returned promise; update the
handleCopyLink function to be async (or return a promise) and await
navigator.clipboard.writeText(window.location.href) inside a try/catch, first
checking for navigator.clipboard existence, then show the existing success toast
on resolve and a failure toast (including a brief error message) on reject;
ensure you reference handleCopyLink and navigator.clipboard.writeText and keep
the toast dependency usage intact.
| <Select | ||
| value={visibility} | ||
| onValueChange={async (value) => { | ||
| setIsVisibilityUpdating(true); | ||
| await onVisibilityChange(value as ChatVisibility); | ||
| setIsVisibilityUpdating(false); | ||
| }} |
There was a problem hiding this comment.
Missing try/finally — isVisibilityUpdating can get stuck on true if the callback throws.
If onVisibilityChange throws (rather than returning false), setIsVisibilityUpdating(false) on Line 158 is never reached, permanently disabling the selector.
Proposed fix
onValueChange={async (value) => {
setIsVisibilityUpdating(true);
- await onVisibilityChange(value as ChatVisibility);
- setIsVisibilityUpdating(false);
+ try {
+ await onVisibilityChange(value as ChatVisibility);
+ } finally {
+ setIsVisibilityUpdating(false);
+ }
}}📝 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.
| <Select | |
| value={visibility} | |
| onValueChange={async (value) => { | |
| setIsVisibilityUpdating(true); | |
| await onVisibilityChange(value as ChatVisibility); | |
| setIsVisibilityUpdating(false); | |
| }} | |
| <Select | |
| value={visibility} | |
| onValueChange={async (value) => { | |
| setIsVisibilityUpdating(true); | |
| try { | |
| await onVisibilityChange(value as ChatVisibility); | |
| } finally { | |
| setIsVisibilityUpdating(false); | |
| } | |
| }} |
🤖 Prompt for AI Agents
In
`@packages/web/src/app/`[domain]/chat/components/shareChatPopover/shareSettings.tsx
around lines 153 - 159, The onValueChange handler for the Select currently sets
setIsVisibilityUpdating(true) then awaits onVisibilityChange but does not guard
against exceptions, so setIsVisibilityUpdating(false) can be skipped; wrap the
await call in a try/finally (or try/catch/finally) inside the onValueChange
callback in shareSettings.tsx so that setIsVisibilityUpdating(false) always runs
regardless of whether onVisibilityChange(value as ChatVisibility) throws,
preserving proper UI state for the Select component.
| await prisma.chatAccess.delete({ | ||
| where: { | ||
| chatId_userId: { | ||
| chatId, | ||
| userId, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
prisma.chatAccess.delete throws P2025 if the record doesn't exist.
If the ChatAccess record was already removed (race condition, stale UI), this will throw an unhandled Prisma error. Use deleteMany (which returns count 0 silently) or wrap in a try/catch.
Suggested fix
- await prisma.chatAccess.delete({
- where: {
- chatId_userId: {
- chatId,
- userId,
- },
- },
- });
+ await prisma.chatAccess.deleteMany({
+ where: {
+ chatId,
+ userId,
+ },
+ });📝 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.
| await prisma.chatAccess.delete({ | |
| where: { | |
| chatId_userId: { | |
| chatId, | |
| userId, | |
| }, | |
| }, | |
| }); | |
| await prisma.chatAccess.deleteMany({ | |
| where: { | |
| chatId, | |
| userId, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@packages/web/src/features/chat/actions.ts` around lines 544 - 551, The
prisma.chatAccess.delete call in the action that removes a user's access can
throw Prisma P2025 if the record is already gone; update the code in the
relevant action to either use prisma.chatAccess.deleteMany({ where: { chatId,
userId } }) so deletion is idempotent (returns count without throwing) or wrap
the prisma.chatAccess.delete(...) call in a try/catch and ignore P2025 errors;
reference the prisma.chatAccess.delete usage in this action to locate and
replace/guard it accordingly.
| }); | ||
|
|
||
| return { success: true }; | ||
| }) |
There was a problem hiding this comment.
Chat sharing gates enforced only in UI
Medium Severity
shareChatWithUsers, getSharedWithUsersForChat, and unshareChatWithUser do not enforce plan or experiment checks server-side. The UI disables sharing with hasEntitlement and EXPERIMENT_ASK_GH_ENABLED, but these server actions can still be invoked directly, allowing unauthorized chat-sharing operations despite feature gating.
Additional Locations (2)
| description: `✅ Chat deleted successfully` | ||
| }); | ||
| router.push(`/${SINGLE_TENANT_ORG_DOMAIN}/chat`); | ||
| router.refresh(); |
There was a problem hiding this comment.
Delete chat redirects to wrong tenant
Medium Severity
After deleting a chat, navigation uses SINGLE_TENANT_ORG_DOMAIN instead of the current route domain. In multi-tenant routes, router.push sends users to the wrong org path, causing incorrect post-delete navigation and potentially a not-found page.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/web/src/app/`[domain]/chat/components/shareChatPopover/shareSettings.tsx:
- Around line 138-146: The removal click handler can leave user.id in
removingUserIds if onRemoveSharedWithUser throws; wrap the await call in a
try/finally so setRemovingUserIds always removes user.id afterwards. Concretely,
inside the onClick handler that references setRemovingUserIds and
onRemoveSharedWithUser (and user.id), add try { await
onRemoveSharedWithUser(user.id); } finally { setRemovingUserIds(prev => { const
next = new Set(prev); next.delete(user.id); return next; }); } so the
spinner/button state is cleared regardless of errors.
In `@packages/web/src/app/api/`(server)/ee/chat/[chatId]/searchMembers/route.ts:
- Around line 36-50: The two guard branches return StatusCodes.FORBIDDEN while
using ErrorCode.NOT_FOUND, which is inconsistent; pick one approach and make
codes match: either (A) keep StatusCodes.FORBIDDEN and replace
ErrorCode.NOT_FOUND with a matching forbidden error code (e.g.,
ErrorCode.FORBIDDEN) in the serviceErrorResponse calls, or (B) if the intent is
to hide the endpoint, change StatusCodes.FORBIDDEN to StatusCodes.NOT_FOUND and
keep ErrorCode.NOT_FOUND. Update both branches that reference
env.EXPERIMENT_ASK_GH_ENABLED and hasEntitlement('chat-sharing') so
serviceErrorResponse uses consistent status and error code pairs (adjust
StatusCodes.* and ErrorCode.* accordingly).
🧹 Nitpick comments (3)
packages/web/src/app/api/(server)/ee/chat/[chatId]/searchMembers/route.ts (1)
104-120: Unbounded query results for large organizations.The comment on Line 26 acknowledges this is non-paginated, but
findManywith notakelimit on acontainssearch (especially with an empty default query on Line 13) will return every org member minus exclusions. For organizations with thousands of members, this could be slow and transfer a large payload.Consider adding a reasonable
takelimit (e.g., 25–50) to cap results.Proposed fix
const members = await prisma.userToOrg.findMany({ where: { orgId: org.id, userId: { notIn: Array.from(excludeUserIds), }, user: { OR: [ { name: { contains: query, mode: 'insensitive' } }, { email: { contains: query, mode: 'insensitive' } }, ], }, }, include: { user: true, }, + take: 50, });packages/web/src/app/[domain]/chat/components/shareChatPopover/ee/invitePanel.tsx (2)
124-133: Side effect inside a state updater function.
setSearchQuery('')on Line 130 is called inside thesetSelectedUsersupdater callback. State updater functions should be pure — calling another state setter inside one is an anti-pattern in React that can lead to unpredictable render batching behavior.Proposed fix — move the side effect outside the updater
onClick={() => { - setSelectedUsers(prev => { - const isSelected = prev.some(u => u.id === user.id); - if (isSelected) { - return prev.filter(u => u.id !== user.id); - } else { - setSearchQuery(''); - return [...prev, user]; - } - }); + const isSelected = selectedUsers.some(u => u.id === user.id); + if (isSelected) { + setSelectedUsers(prev => prev.filter(u => u.id !== user.id)); + } else { + setSelectedUsers(prev => [...prev, user]); + setSearchQuery(''); + } }}
36-44: DuplicatedgetInitialshelper across components.This exact function is duplicated in
shareSettings.tsx(Lines 57–65). Consider extracting it into a shared utility.
| onClick={async () => { | ||
| setRemovingUserIds(prev => new Set(prev).add(user.id)); | ||
| await onRemoveSharedWithUser(user.id); | ||
| setRemovingUserIds(prev => { | ||
| const next = new Set(prev); | ||
| next.delete(user.id); | ||
| return next; | ||
| }); | ||
| }} |
There was a problem hiding this comment.
Same try/finally concern for user removal handler.
If onRemoveSharedWithUser throws, the user ID remains in removingUserIds permanently — the spinner stays and the button remains disabled.
Proposed fix
onClick={async () => {
setRemovingUserIds(prev => new Set(prev).add(user.id));
- await onRemoveSharedWithUser(user.id);
- setRemovingUserIds(prev => {
- const next = new Set(prev);
- next.delete(user.id);
- return next;
- });
+ try {
+ await onRemoveSharedWithUser(user.id);
+ } finally {
+ setRemovingUserIds(prev => {
+ const next = new Set(prev);
+ next.delete(user.id);
+ return next;
+ });
+ }
}}🤖 Prompt for AI Agents
In
`@packages/web/src/app/`[domain]/chat/components/shareChatPopover/shareSettings.tsx
around lines 138 - 146, The removal click handler can leave user.id in
removingUserIds if onRemoveSharedWithUser throws; wrap the await call in a
try/finally so setRemovingUserIds always removes user.id afterwards. Concretely,
inside the onClick handler that references setRemovingUserIds and
onRemoveSharedWithUser (and user.id), add try { await
onRemoveSharedWithUser(user.id); } finally { setRemovingUserIds(prev => { const
next = new Set(prev); next.delete(user.id); return next; }); } so the
spinner/button state is cleared regardless of errors.
| if (env.EXPERIMENT_ASK_GH_ENABLED === 'true') { | ||
| return serviceErrorResponse({ | ||
| statusCode: StatusCodes.FORBIDDEN, | ||
| errorCode: ErrorCode.NOT_FOUND, | ||
| message: "This API is not enabled with this experiment.", | ||
| }) | ||
| } | ||
|
|
||
| if (!hasEntitlement('chat-sharing')) { | ||
| return serviceErrorResponse({ | ||
| statusCode: StatusCodes.FORBIDDEN, | ||
| errorCode: ErrorCode.NOT_FOUND, | ||
| message: "Chat sharing is not enabled for your license", | ||
| }) | ||
| } |
There was a problem hiding this comment.
Mismatched HTTP status code and error code.
Both guards return StatusCodes.FORBIDDEN (403) but use ErrorCode.NOT_FOUND. This is confusing for API consumers — a 403 response with a "not found" error code sends mixed signals. If the intent is to hide the endpoint's existence, use 404 consistently; if the intent is to convey "forbidden," use a corresponding error code.
Proposed fix
if (env.EXPERIMENT_ASK_GH_ENABLED === 'true') {
return serviceErrorResponse({
- statusCode: StatusCodes.FORBIDDEN,
- errorCode: ErrorCode.NOT_FOUND,
+ statusCode: StatusCodes.NOT_FOUND,
+ errorCode: ErrorCode.NOT_FOUND,
message: "This API is not enabled with this experiment.",
})
}
if (!hasEntitlement('chat-sharing')) {
return serviceErrorResponse({
- statusCode: StatusCodes.FORBIDDEN,
- errorCode: ErrorCode.NOT_FOUND,
+ statusCode: StatusCodes.NOT_FOUND,
+ errorCode: ErrorCode.NOT_FOUND,
message: "Chat sharing is not enabled for your license",
})
}📝 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 (env.EXPERIMENT_ASK_GH_ENABLED === 'true') { | |
| return serviceErrorResponse({ | |
| statusCode: StatusCodes.FORBIDDEN, | |
| errorCode: ErrorCode.NOT_FOUND, | |
| message: "This API is not enabled with this experiment.", | |
| }) | |
| } | |
| if (!hasEntitlement('chat-sharing')) { | |
| return serviceErrorResponse({ | |
| statusCode: StatusCodes.FORBIDDEN, | |
| errorCode: ErrorCode.NOT_FOUND, | |
| message: "Chat sharing is not enabled for your license", | |
| }) | |
| } | |
| if (env.EXPERIMENT_ASK_GH_ENABLED === 'true') { | |
| return serviceErrorResponse({ | |
| statusCode: StatusCodes.NOT_FOUND, | |
| errorCode: ErrorCode.NOT_FOUND, | |
| message: "This API is not enabled with this experiment.", | |
| }) | |
| } | |
| if (!hasEntitlement('chat-sharing')) { | |
| return serviceErrorResponse({ | |
| statusCode: StatusCodes.NOT_FOUND, | |
| errorCode: ErrorCode.NOT_FOUND, | |
| message: "Chat sharing is not enabled for your license", | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@packages/web/src/app/api/`(server)/ee/chat/[chatId]/searchMembers/route.ts
around lines 36 - 50, The two guard branches return StatusCodes.FORBIDDEN while
using ErrorCode.NOT_FOUND, which is inconsistent; pick one approach and make
codes match: either (A) keep StatusCodes.FORBIDDEN and replace
ErrorCode.NOT_FOUND with a matching forbidden error code (e.g.,
ErrorCode.FORBIDDEN) in the serviceErrorResponse calls, or (B) if the intent is
to hide the endpoint, change StatusCodes.FORBIDDEN to StatusCodes.NOT_FOUND and
keep ErrorCode.NOT_FOUND. Update both branches that reference
env.EXPERIMENT_ASK_GH_ENABLED and hasEntitlement('chat-sharing') so
serviceErrorResponse uses consistent status and error code pairs (adjust
StatusCodes.* and ErrorCode.* accordingly).
| userId, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Unshare fails on already removed access
Low Severity
unshareChatWithUser calls prisma.chatAccess.delete without checking whether the share row exists. If access was already removed (for example by a concurrent action), Prisma throws and sew converts it into an unexpected error instead of a normal not-found style response.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| {isAnonymousAccessEnabled && ( | ||
| <p className="text-sm text-muted-foreground"> | ||
| <Link className="underline" href={callbackUrl ?? "/"}>Continue as guest</Link> | ||
| </p> |
There was a problem hiding this comment.
Guest link trusts unsanitized callback URL
Low Severity
The new “Continue as guest” link uses callbackUrl directly as href. Because callbackUrl comes from query params and is not validated here, the login page can render a link to an arbitrary external destination.
1 participant
Overview
From cursor:
Note
Enables sharing chats beyond simple public/private links by introducing per-user access grants and a new
chat-sharingentitlement, plus related UI to manage visibility and invite/remove org members from access.Reworks chat ownership/editability: removes
Chat.isReadonly, tracks anonymous chat ownership via a cookie-backedanonymousCreatorId, claims anonymous chats on sign-in, and tightens write operations (sending messages, renaming, updating messages) to owner-only while allowing shared users to view private chats and leave feedback.Adds chat duplication (including from read-only/shared views) and consolidates chat action UI (rename/duplicate/delete with loading states). Also adds public-chat metadata/OG image generation for better link previews, and improves auth UX by preserving
callbackUrlacross login/signup and optionally offering “Continue as guest.”Todo:
Fixes #661
Fixes #479
UI screenshots:
Share Dialog - Main View
Visibility Dropdown
Invite Users View
Remove User
Enterprise Feature Disabled
Unauthenticated User
Chat Actions Menu
Duplicating Chats
Anonymous access
Open graph images
Summary by CodeRabbit
New Features
Bug Fixes
Refactors
Note
High Risk
Touches core chat authorization and persistence (DB schema + API/server actions) and introduces new sharing surfaces; mistakes could leak private chats or incorrectly block access.
Overview
Enables richer chat sharing and link previews: public chat pages now emit OpenGraph/Twitter metadata and a dynamic OG image, and chat owners get a new Share popover to toggle visibility and (Enterprise) invite/remove specific org members.
Reworks chat permissions and ownership by removing
Chat.isReadonlyand enforcing owner-only writes (sending messages, renaming, updating messages, changing visibility), while allowing private chats to be viewed when explicitly shared via newChatAccessrows.Improves anonymous chat flow by tracking anonymous ownership via a cookie-backed
anonymousCreatorId, claiming those chats on sign-in, and surfacing better auth UX (preservecallbackUrl, optional “Continue as guest”, sign-in prompt). Adds chat duplication (including from read-only/shared views) and consolidates chat actions UI (rename/duplicate/delete) with loading states.Written by Cursor Bugbot for commit 908666e. This will update automatically on new commits. Configure here.