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.
packages/web/src/app/[domain]/chat/components/shareChatPopover.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/app/[domain]/chat/components/shareChatPopover.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/features/chat/components/chatThread/signInPromptBanner.tsx
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/web/src/app/[domain]/chat/components/shareChatPopover/index.tsx
Outdated
Show resolved
Hide resolved
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.
packages/web/src/app/[domain]/chat/components/shareChatPopover/index.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/app/[domain]/chat/components/shareChatPopover/index.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/app/[domain]/chat/components/shareChatPopover/ee/invitePanel.tsx
Show resolved
Hide resolved
packages/web/src/app/[domain]/chat/components/shareChatPopover/shareSettings.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/app/[domain]/chat/components/shareChatPopover/shareSettings.tsx
Show resolved
Hide resolved
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.
packages/web/src/app/[domain]/chat/components/shareChatPopover/shareSettings.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/app/api/(server)/ee/chat/[chatId]/searchMembers/route.ts
Show resolved
Hide resolved
packages/web/src/app/[domain]/chat/components/shareChatPopover/ee/invitePanel.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
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."Fixes #661
Fixes #479
PostHog Events
wa_chat_share_dialog_openedwa_chat_visibility_changedwa_chat_link_copiedwa_chat_users_invitedwa_chat_user_removedwa_shared_chat_viewedwa_chat_sign_in_banner_displayedwa_chat_sign_in_banner_dismissedwa_chat_sign_in_banner_clickedwa_anonymous_chats_claimedwa_chat_duplicatedwa_chat_renamedwa_chat_deletedUI 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 access control and persistence (schema changes, ownership/visibility checks, new sharing API), so mistakes could leak private chats or block legitimate access.
Overview
Adds a new chat sharing model: chats can be public via link or private but shared with specific org users (Enterprise-gated via new
chat-sharingentitlement), including a new member-search API and UI to invite/remove users and change visibility.Reworks chat permissions from
Chat.isReadonlyto ownership-based access control, adds anonymous chat ownership via a cookie-backedanonymousCreatorId, and claims anonymous chats on sign-in; write operations (sending messages, renaming, updating messages) become owner-only while shared users can view private chats and submit feedback.Introduces chat duplication (including from non-owner/read-only views), improves chat action UX with consolidated rename/duplicate/delete dialogs + loading states, and adds Open Graph metadata + dynamic OG image generation for public chat links; auth flows now preserve
callbackUrland can offer “Continue as guest” when anonymous access is enabled.Written by Cursor Bugbot for commit 8eb70a7. This will update automatically on new commits. Configure here.