feat: add teams chat list feature - #2294
Conversation
|
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:
WalkthroughAdds organization-scoped chat snapshots: DB migration and schema updates, API changes to create/list/get/delete org shares, server-side playground routing, new org-shared UI (pages, sidebar, search, header), share dialog/org switcher updates, playground integration, and React Query hooks. Organization-Scoped Chat Sharing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/playground/src/hooks/useChats.ts (1)
7-20:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftModel org shares as a collection, not a single pair.
The dialog now renders a per-organization share list, but
Chatcan only hydrate oneorgShareId/orgShareOrganizationIdpair. After a reload, any additional org shares for the same chat will be invisible to the client until they are recreated in-session. This contract needs either anorgShares[]field or a dedicated “list org shares for chat” query that the dialog can consume.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/hooks/useChats.ts` around lines 7 - 20, The Chat type currently models organization shares as a single pair (orgShareId, orgShareOrganizationId) which loses additional org shares after reload; update the model to represent multiple org shares by replacing those two fields with an orgShares array (e.g., orgShares: { id: string; organizationId: string; sharedAt?: string }[]) or alternatively add and use a new query/function like listOrgSharesForChat(chatId) that the UI (dialog) consumes; update all usages of Chat (including the useChats hook and any serialization/deserialization/hydration logic) to read/write the orgShares collection rather than orgShareId/orgShareOrganizationId so multiple org shares persist and render correctly.
🧹 Nitpick comments (1)
apps/playground/src/components/playground/chat-header.tsx (1)
67-67: ⚡ Quick winConsider changing the default value to
false.The current default of
truemeans sharing is disabled by default, which is counterintuitive. While callers currently provide this prop explicitly, the default should represent the normal state (sharing enabled) rather than a restrictive state. This would improve code clarity and prevent potential issues if the prop is ever omitted.♻️ Proposed fix
- isShareChatDisabled = true, + isShareChatDisabled = false,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/components/playground/chat-header.tsx` at line 67, The default prop value for isShareChatDisabled in the ChatHeader component is currently true, which makes sharing disabled by default; update the default to false by changing the destructured/default assignment (isShareChatDisabled = false) in the chat-header component (look for the ChatHeader functional component or its props destructuring) so sharing is enabled when callers omit the prop; run/type-check any related usages or tests to ensure no assumptions break.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/playground/src/components/playground/org-sidebar.tsx`:
- Line 385: The Sidebar render currently concatenates className directly causing
"undefined" to appear (Sidebar element with prop className={className + "
max-md:hidden"}); update this to safely join classes using the existing cn
utility (e.g., cn(className, "max-md:hidden")) or at minimum fallback to an
empty string (e.g., (className ?? "") + " max-md:hidden") so undefined never
leaks into the rendered class attribute; modify the Sidebar usage in
org-sidebar.tsx accordingly.
In `@apps/playground/src/components/playground/share-chat-dialog.tsx`:
- Around line 151-156: The orgShareMap state (created via useState and updated
with setOrgShareMap) currently only accumulates entries and must be reset
whenever currentChatId changes; update the initialization logic to run in a
useEffect watching currentChatId (and orgShareId/orgShareOrganizationId) so that
on chat switch you reinitialize orgShareMap to only include the orgShareId for
the active chat (or {} if none) and then merge any additional shares for that
same chat, and apply the same fix to the other similar block that sets
orgShareMap later in the file.
- Around line 328-336: The tooltip currently attaches to a disabled Button
inside TooltipTrigger, making the "Share is available after the response
finishes" message unreachable when disabled; fix by moving the TooltipTrigger to
wrap a non-disabled wrapper element (e.g., a span or div) and keep the
DialogTrigger/Button inside that wrapper so the wrapper receives hover/focus
events while the Button retains disabled state; update the same pattern for the
other occurrence referenced (the block around lines with the second
TooltipTrigger/DialogTrigger) so both tooltips remain reachable when disabled.
---
Outside diff comments:
In `@apps/playground/src/hooks/useChats.ts`:
- Around line 7-20: The Chat type currently models organization shares as a
single pair (orgShareId, orgShareOrganizationId) which loses additional org
shares after reload; update the model to represent multiple org shares by
replacing those two fields with an orgShares array (e.g., orgShares: { id:
string; organizationId: string; sharedAt?: string }[]) or alternatively add and
use a new query/function like listOrgSharesForChat(chatId) that the UI (dialog)
consumes; update all usages of Chat (including the useChats hook and any
serialization/deserialization/hydration logic) to read/write the orgShares
collection rather than orgShareId/orgShareOrganizationId so multiple org shares
persist and render correctly.
---
Nitpick comments:
In `@apps/playground/src/components/playground/chat-header.tsx`:
- Line 67: The default prop value for isShareChatDisabled in the ChatHeader
component is currently true, which makes sharing disabled by default; update the
default to false by changing the destructured/default assignment
(isShareChatDisabled = false) in the chat-header component (look for the
ChatHeader functional component or its props destructuring) so sharing is
enabled when callers omit the prop; run/type-check any related usages or tests
to ensure no assumptions break.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 14bd1570-97a4-4c70-9174-607e298955ee
⛔ Files ignored due to path filters (4)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/ui/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.ts
📒 Files selected for processing (21)
apps/api/src/routes/chats.tsapps/api/src/routes/public-chat-shares.tsapps/playground/src/app/org/[orgId]/chat/[shareId]/page.tsxapps/playground/src/app/org/[orgId]/page.tsxapps/playground/src/app/page.tsxapps/playground/src/app/playground-shell.tsxapps/playground/src/components/playground/chat-header.tsxapps/playground/src/components/playground/chat-page-client.tsxapps/playground/src/components/playground/chat-sidebar.tsxapps/playground/src/components/playground/fork-chat-button.tsxapps/playground/src/components/playground/org-header.tsxapps/playground/src/components/playground/org-page-client.tsxapps/playground/src/components/playground/org-search-dialog.tsxapps/playground/src/components/playground/org-sidebar.tsxapps/playground/src/components/playground/organization-switcher.tsxapps/playground/src/components/playground/share-chat-dialog.tsxapps/playground/src/hooks/useChats.tspackages/db/migrations/1778761263_round_pandemic.sqlpackages/db/migrations/meta/1778761263_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/db/src/schema.ts
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/playground/src/components/playground/org-sidebar.tsx (1)
548-561:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHardcoded Dashboard URL — same pattern as logout redirect.
Same env-coupled issue covered for the logout redirect at lines 269-272. Centralize behind
NEXT_PUBLIC_DASHBOARD_URLso non-development environments don't always point at the production domain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/components/playground/org-sidebar.tsx` around lines 548 - 561, Replace the hardcoded NODE_ENV conditional used to build the Dashboard link inside the DropdownMenuItem anchor with a centralized env var: read the URL from process.env.NEXT_PUBLIC_DASHBOARD_URL (falling back to the previous development URL if you want a safe default), and use that value for the href on the anchor (the DropdownMenuItem/anchor that currently renders ExternalLink and "Dashboard"); keep target="_blank" and rel="noopener noreferrer". Ensure the code only references NEXT_PUBLIC_DASHBOARD_URL for non-dev/prod decisions so the dashboard URL is configurable at runtime.
🧹 Nitpick comments (2)
apps/playground/src/components/playground/org-sidebar.tsx (2)
102-116: 💤 Low value
MMM dis ambiguous for older shares; consider including year for items older than ~1 year.
format(date, "MMM d")(e.g.,Mar 5) drops the year. For shares older than ~12 months this can confuse users about whether they are seeing this year's or last year's share. Consider switching toMMM d, yyyyonce the date is older than the current year.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/components/playground/org-sidebar.tsx` around lines 102 - 116, The formatDate function currently uses format(date, "MMM d") which omits the year; update formatDate to detect if date.getFullYear() differs from now.getFullYear() (or if it's older than 12 months) and switch to format(date, "MMM d, yyyy") in that case, otherwise keep the existing short format; modify the logic inside formatDate (referencing formatDate and the format(...) call) to choose the format string conditionally and return the appropriate formatted string.
204-580: 🏗️ Heavy liftHeavy duplication with
ChatSidebar; consider extracting shared pieces.The user dropdown, ⌘K/Alt+K shortcut, logout flow, env-aware redirect URLs, virtualized history list, and grouping/format helpers all look very close to what the personal
ChatSidebaralready implements. Two near-copies will diverge over time (the logout URLs already appear in two places in this file alone). Worth extracting:
useLogout()hook (handles posthog/cookie/query-cache/redirect)useSearchShortcut()hookformatRelativeShareDate+groupSharesByDatehelpers<SharesVirtualList>row rendererDefer if time-boxed, but flagging since this is the second consumer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/components/playground/org-sidebar.tsx` around lines 204 - 580, The OrgSidebar duplicates several responsibilities from ChatSidebar; extract shared logic to avoid divergence by (1) creating a useLogout hook that encapsulates posthog.reset, clearLastUsedProjectCookiesAction, queryClient.clear and router.push (used by logout currently), (2) creating a useSearchShortcut hook that manages isMac state and the window keydown handler (replacing the useEffect that sets isMac and the handleKeyDown logic), (3) moving grouping/formatting helpers like groupSharesByDate and any formatRelativeShareDate into a shared helper module and using those in OrgSidebar (replace the shareGroups/historyRows logic), and (4) extracting the virtualized list renderer into a SharesVirtualList component (wrap List usage and row rendering currently using OrgShareRowComponent, getOrgShareRowHeight and rowProps). Update OrgSidebar to import and use useLogout, useSearchShortcut, shared helpers, and SharesVirtualList so the duplicated logic is centralized and both sidebars can reuse it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/playground/src/components/playground/org-sidebar.tsx`:
- Around line 102-146: The label mismatch comes from formatDate using an
hours-difference rule while groupSharesByDate uses calendar-day checks; change
formatDate to use the same calendar-day logic as groupSharesByDate (compare
date.toDateString() against today.toDateString(), yesterday.toDateString(), and
lastWeek boundary) and only fall back to format(date, "MMM d") for older dates
so both functions classify the same timestamps (update formatDate references and
keep groupSharesByDate unchanged).
- Around line 413-420: The Sidebar contains two menu entries linking to "/",
causing a duplicate; update the <SidebarMenuItem>/<SidebarMenuButton> for "Chat"
(the block rendering <MessageSquare ...> and <span>Chat</span>) so its Link
points to the org-scoped chat index (e.g., `/org/${organizationId}` or the app's
org chat list route) or remove this redundant menu item if duplicate behavior is
intended; ensure you reference the same organizationId/param used elsewhere in
this component (the one used to build the "New Chat" route) so the Chat link
becomes org-scoped and not "/" like the New Chat entry.
- Around line 264-275: Wrap the signOut(...) call in a try/catch and await it so
failures are handled: keep the existing onSuccess block (which calls
queryClient.clear() and router.push(...)) inside the try, and in the catch
surface an error toast (e.g., toast.error or your app’s notification helper) and
log the error so the user can retry instead of being left in a
partially-logged-out state; reference signOut, queryClient.clear, and
router.push when making the change.
- Around line 255-257: In logout (the async function named logout) guard the
posthog instance returned by usePostHog() before calling reset: check that
posthog is non-null (e.g., if (posthog) posthog.reset()) or use an optional call
so reset isn't invoked when the hook returns null; also consider wrapping
posthog.reset() in a try/catch so any thrown error won't prevent the subsequent
redirect/cache-clear logic from running. Ensure you reference the posthog
variable used inside logout and preserve the redirect/cache-clear sequence after
the guarded reset.
- Around line 255-276: The logout function currently hardcodes redirect URLs
inside the signOut onSuccess handler; replace the hardcoded development/prod
branches with an env-driven value (e.g., read process.env.NEXT_PUBLIC_CHAT_URL)
and use that for router.push so staging/preview honor the environment, and
likewise replace the hardcoded Dashboard href with
process.env.NEXT_PUBLIC_DASHBOARD_URL; update the references in the logout
function and the Dashboard link rendering to use these public env vars (no
memoization), and ensure queryClient.clear() and router.push(...) behavior
remains the same inside signOut's onSuccess.
- Line 219: The CreditsDisplay is using the hooked organization from
useOrganization() (const { organization, isLoading: isOrgLoading } =
useOrganization()) which always returns the default org; update the parent
component to pass the selectedOrganization prop into CreditsDisplay (the same
prop already forwarded to ChatSidebarSkeleton) and remove/replace reliance on
the hooked organization there so the credits reflect the currently
selectedOrganization; locate usage of CreditsDisplay and change its props to
accept selectedOrganization (or pass selectedOrganization into its existing
organization prop) and ensure any internal reference to useOrganization() in
CreditsDisplay is removed or overridden to use the passed-in
selectedOrganization instead.
- Around line 232-253: The keyboard shortcut handler in the useEffect
(handleKeyDown / isMac / setIsSearchOpen) must be updated to (1) detect editable
targets and skip handling when the event.target is an INPUT, TEXTAREA or has
isContentEditable true (so avoid calling event.preventDefault() or opening
search while typing or using IMEs) and (2) modernize platform detection by using
navigator.userAgentData?.platform when available with a fallback to
navigator.platform (e.g., const platform = navigator.userAgentData?.platform ||
navigator.platform; setIsMac(/(Mac|iPhone|iPad|iPod)/i.test(platform))). Ensure
the editable-check runs before computing or acting on the shortcut and keep
existing modifier-key logic otherwise.
---
Duplicate comments:
In `@apps/playground/src/components/playground/org-sidebar.tsx`:
- Around line 548-561: Replace the hardcoded NODE_ENV conditional used to build
the Dashboard link inside the DropdownMenuItem anchor with a centralized env
var: read the URL from process.env.NEXT_PUBLIC_DASHBOARD_URL (falling back to
the previous development URL if you want a safe default), and use that value for
the href on the anchor (the DropdownMenuItem/anchor that currently renders
ExternalLink and "Dashboard"); keep target="_blank" and rel="noopener
noreferrer". Ensure the code only references NEXT_PUBLIC_DASHBOARD_URL for
non-dev/prod decisions so the dashboard URL is configurable at runtime.
---
Nitpick comments:
In `@apps/playground/src/components/playground/org-sidebar.tsx`:
- Around line 102-116: The formatDate function currently uses format(date, "MMM
d") which omits the year; update formatDate to detect if date.getFullYear()
differs from now.getFullYear() (or if it's older than 12 months) and switch to
format(date, "MMM d, yyyy") in that case, otherwise keep the existing short
format; modify the logic inside formatDate (referencing formatDate and the
format(...) call) to choose the format string conditionally and return the
appropriate formatted string.
- Around line 204-580: The OrgSidebar duplicates several responsibilities from
ChatSidebar; extract shared logic to avoid divergence by (1) creating a
useLogout hook that encapsulates posthog.reset,
clearLastUsedProjectCookiesAction, queryClient.clear and router.push (used by
logout currently), (2) creating a useSearchShortcut hook that manages isMac
state and the window keydown handler (replacing the useEffect that sets isMac
and the handleKeyDown logic), (3) moving grouping/formatting helpers like
groupSharesByDate and any formatRelativeShareDate into a shared helper module
and using those in OrgSidebar (replace the shareGroups/historyRows logic), and
(4) extracting the virtualized list renderer into a SharesVirtualList component
(wrap List usage and row rendering currently using OrgShareRowComponent,
getOrgShareRowHeight and rowProps). Update OrgSidebar to import and use
useLogout, useSearchShortcut, shared helpers, and SharesVirtualList so the
duplicated logic is centralized and both sidebars can reuse it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc59d777-de8c-46c2-8af6-d15ac0c04797
📒 Files selected for processing (2)
apps/playground/src/components/playground/org-sidebar.tsxapps/playground/src/components/playground/share-chat-dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/playground/src/components/playground/share-chat-dialog.tsx
| await signOut({ | ||
| fetchOptions: { | ||
| onSuccess: () => { | ||
| queryClient.clear(); | ||
| router.push( | ||
| process.env.NODE_ENV === "development" | ||
| ? "http://localhost:3003/login" | ||
| : "https://chat.llmgateway.io/login", | ||
| ); | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
signOut error is not handled; user gets stuck on partially-logged-out state.
If signOut() rejects (network error, server returns 5xx), onSuccess never fires so the React Query cache is not cleared and no redirect happens, while PostHog has already been reset and project cookies cleared. Wrap the call in try/catch (or .catch) and at minimum surface an error toast so the user can retry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/playground/src/components/playground/org-sidebar.tsx` around lines 264 -
275, Wrap the signOut(...) call in a try/catch and await it so failures are
handled: keep the existing onSuccess block (which calls queryClient.clear() and
router.push(...)) inside the try, and in the catch surface an error toast (e.g.,
toast.error or your app’s notification helper) and log the error so the user can
retry instead of being left in a partially-logged-out state; reference signOut,
queryClient.clear, and router.push when making the change.
| <SidebarMenuItem> | ||
| <SidebarMenuButton asChild tooltip="Chat"> | ||
| <Link href="/"> | ||
| <MessageSquare className="h-4 w-4" /> | ||
| <span>Chat</span> | ||
| </Link> | ||
| </SidebarMenuButton> | ||
| </SidebarMenuItem> |
There was a problem hiding this comment.
"Chat" navigation links to /, same as "New Chat" — likely should be org-scoped or removed.
New Chat (line 407) and Chat (line 415) both href="/". Either this is a duplicate entry or the Chat link should point to the org-scoped chat index (e.g., /org/${organizationId} or a list view) given this is an org-scoped sidebar. Worth confirming the intended target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/playground/src/components/playground/org-sidebar.tsx` around lines 413 -
420, The Sidebar contains two menu entries linking to "/", causing a duplicate;
update the <SidebarMenuItem>/<SidebarMenuButton> for "Chat" (the block rendering
<MessageSquare ...> and <span>Chat</span>) so its Link points to the org-scoped
chat index (e.g., `/org/${organizationId}` or the app's org chat list route) or
remove this redundant menu item if duplicate behavior is intended; ensure you
reference the same organizationId/param used elsewhere in this component (the
one used to build the "New Chat" route) so the Chat link becomes org-scoped and
not "/" like the New Chat entry.
7d4d4bd to
2496b0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/chats.ts`:
- Around line 36-37: The response schema currently exposes only a single
orgShareId/orgShareOrganizationId pair (symbols: orgShareId,
orgShareOrganizationId) but chats can have multiple org shares; change the
contract to return a collection instead (e.g., an orgShares array of objects
with id and organizationId) and update the route/schema in chats.ts to emit that
array rather than the two nullable fields; also adjust any downstream code that
reads orgShareId/orgShareOrganizationId to iterate or map over orgShares (look
for usages of orgShareId and orgShareOrganizationId to update consumers).
- Around line 1203-1242: The deletion currently allows any org member to remove
a share after userHasOrganizationAccess succeeds; narrow the delete to the share
owner or require an org-admin permission: when fetching and deleting the record
(symbols: share, share.organizationId, db.update(tables.chatShare),
userHasOrganizationAccess) either add a condition enforcing
eq(tables.chatShare.userId, user.id) to the WHERE for owner-only deletion, or
replace/augment the check with an explicit admin check (e.g.,
isOrgAdmin(user.id, share.organizationId)) and only allow the deletion if that
returns true; ensure both the initial select and the subsequent update use the
same tightened predicate so non-owners cannot delete another user's chatShare by
id.
- Around line 813-814: Remove the broad .catch that swallows malformed JSON;
instead read the raw request body and only default to {} when the body is truly
empty, letting JSON parse errors propagate to the global handler. Concretely,
replace the c.req.json().catch(() => ({})) usage referenced by
shareChatSchema.parse with logic that awaits c.req.text(), uses {} if the text
is empty, and otherwise calls JSON.parse on the text (do not catch JSON.parse
errors), then pass that value into shareChatSchema.parse so organizationId is
derived correctly.
- Around line 63-65: shareChatSchema currently allows empty strings for
organizationId which then get written into chatShare and bypass access checks;
update shareChatSchema (the shareChatSchema object and its organizationId
schema) so it rejects blank strings (e.g., require min(1) or transform empty
string -> undefined) and adjust downstream logic that reads organizationId so it
receives either a real non-empty string or undefined before the access
validation and the insertion into chatShare.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d7cf252-7739-4d74-a413-fd75bcf4d781
⛔ Files ignored due to path filters (4)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/ui/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.ts
📒 Files selected for processing (21)
apps/api/src/routes/chats.tsapps/api/src/routes/public-chat-shares.tsapps/playground/src/app/org/[orgId]/chat/[shareId]/page.tsxapps/playground/src/app/org/[orgId]/page.tsxapps/playground/src/app/page.tsxapps/playground/src/app/playground-shell.tsxapps/playground/src/components/playground/chat-header.tsxapps/playground/src/components/playground/chat-page-client.tsxapps/playground/src/components/playground/chat-sidebar.tsxapps/playground/src/components/playground/fork-chat-button.tsxapps/playground/src/components/playground/org-header.tsxapps/playground/src/components/playground/org-page-client.tsxapps/playground/src/components/playground/org-search-dialog.tsxapps/playground/src/components/playground/org-sidebar.tsxapps/playground/src/components/playground/organization-switcher.tsxapps/playground/src/components/playground/share-chat-dialog.tsxapps/playground/src/hooks/useChats.tspackages/db/migrations/1778838468_yummy_randall.sqlpackages/db/migrations/meta/1778838468_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/db/src/schema.ts
✅ Files skipped from review due to trivial changes (1)
- apps/playground/src/app/org/[orgId]/page.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/playground/src/components/playground/fork-chat-button.tsx
- apps/playground/src/components/playground/org-header.tsx
- apps/playground/src/app/org/[orgId]/chat/[shareId]/page.tsx
- packages/db/src/schema.ts
- apps/playground/src/components/playground/chat-sidebar.tsx
- apps/api/src/routes/public-chat-shares.ts
- apps/playground/src/components/playground/chat-page-client.tsx
- apps/playground/src/components/playground/org-page-client.tsx
- apps/playground/src/hooks/useChats.ts
- apps/playground/src/components/playground/organization-switcher.tsx
- apps/playground/src/components/playground/share-chat-dialog.tsx
- apps/playground/src/app/playground-shell.tsx
- apps/playground/src/components/playground/org-search-dialog.tsx
- apps/playground/src/components/playground/chat-header.tsx
- apps/playground/src/components/playground/org-sidebar.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/playground/src/hooks/useChats.ts (1)
109-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale
/chatslist after share mutation.The previous implementation invalidated the global
get /chatsquery on success, which is what the sidebar/list view consumes (and that response now carriesorgSharesper chat). After this refactor, the list cache is never invalidated on share creation, so per-row share indicators (shareId,sharedAt,orgShares) can be stale until the next manual refetch.Same concern applies to
useDeleteChatShare(lines 137-156) anduseDeleteOrgChatShare(lines 158-203), which also dropped/omitted the/chatsinvalidation.♻️ Suggested fix
return api.useMutation("post", "/chats/{id}/share", { onSuccess: (_data, variables) => { + const chatsQueryKey = api.queryOptions("get", "/chats").queryKey; + void queryClient.invalidateQueries({ queryKey: chatsQueryKey }); + const chatId = variables.params?.path?.id;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/hooks/useChats.ts` around lines 109 - 135, The share/create mutation currently invalidates per-chat and org-shares queries but not the global "get /chats" list, causing stale sidebar data; update the onSuccess handlers in the POST "/chats/{id}/share" mutation (and similarly in useDeleteChatShare and useDeleteOrgChatShare) to also compute the queryKey for api.queryOptions("get", "/chats") and call queryClient.invalidateQueries({ queryKey: thatKey }) so the global chats list (which contains orgShares/shareId/sharedAt) is refreshed after share create/delete.
🧹 Nitpick comments (8)
apps/api/src/routes/chats.ts (4)
227-239: ⚡ Quick winDuplicated
orgSharesaggregation SQL — extract into a helper.The same JSON-aggregating subquery for
orgSharesis repeated verbatim inlistChats(227-239),searchChats(355-367), andgetChat(571-583). Any future tweak (e.g., addingcreatedAt, ordering, or fixing the filter condition) needs to be applied in three places. Extract it into a top-level helper that returns thesql<…>fragment.♻️ Suggested helper
const orgSharesAggregate = sql<Array<{ id: string; organizationId: string }>>`COALESCE( ( SELECT json_agg(json_build_object( 'id', cs.id, 'organizationId', cs.organization_id )) FROM chat_share cs WHERE cs.chat_id = ${tables.chat.id} AND cs.organization_id IS NOT NULL AND cs.deleted_at IS NULL ), '[]'::json )`;Also applies to: 355-367, 571-583
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/chats.ts` around lines 227 - 239, Extract the repeated JSON-aggregation SQL used for orgShares into a single top-level helper (e.g., orgSharesAggregate) that returns the sql<Array<{ id: string; organizationId: string }>> fragment, then replace the inline fragments in listChats, searchChats, and getChat with that helper; ensure the helper references tables.chat.id exactly as the inline versions do and preserves the COALESCE(... '[]'::json) behaviour and all WHERE filters (cs.chat_id = ${tables.chat.id}, cs.organization_id IS NOT NULL, cs.deleted_at IS NULL) so any future changes apply in one place.
1010-1078: ⚡ Quick winNo pagination on org shares list.
GET /org/{organizationId}/sharesreturns every active organization-scoped share with nolimit/offsetand no pagination parameters in the route schema. For a large org with many shared chats this becomes a heavy unbounded query (and unbounded JSON response) on each sidebar/page load. Consider addinglimit/offset(mirroringsearchChatsat lines 291-296) and an optional total count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/chats.ts` around lines 1010 - 1078, The listOrgShares route currently returns all matching shares without pagination; update the createRoute request schema for listOrgShares to accept optional limit and offset (e.g., z.number().int().min(0) for offset and a bounded limit), read them from c.req.valid("param"/"query") as appropriate, apply .limit(limit) and .offset(offset) to the db query (use defaults like limit=50, offset=0), and optionally run a separate COUNT(*) query with the same filters to return a total count in the JSON response alongside shares; ensure you still filter by organizationId and reuse userHasOrganizationAccess and the same where() conditions so results remain consistent.
802-825: 💤 Low valueCombine org access + status check into a single query.
Two sequential round-trips run on every successful org share:
userHasOrganizationAccessand theorganizationstatus/isPersonallookup. Both can be expressed as one query joiningorganizationwith the membership table, returning a row only when the user is a member AND the org is active AND not personal. This reduces latency on the share hot path and keeps the access decision atomic. Optional, since the two-step version is still correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/chats.ts` around lines 802 - 825, Replace the two-step access check (userHasOrganizationAccess + separate organization lookup) with a single DB query that joins the organization table to the membership table and filters for eq(tables.organization.id, organizationId), membership.userId = user.id (or equivalent membership field), eq(tables.organization.status, "active") and eq(tables.organization.isPersonal, false); if the query returns no row throw the same HTTPException(404, { message: "Organization not found" }). Locate the code around userHasOrganizationAccess, the subsequent db.select on tables.organization, and HTTPException and combine them into one atomic db.select/from/where that enforces membership and org status/isPersonal in one round trip. Ensure you remove the prior userHasOrganizationAccess call and keep the same error handling semantics.
745-748: 💤 Low valueType-narrowing filter at runtime is a no-op — simplify.
activeOrgSharesis already filtered withisNotNull(tables.chatShare.organizationId)in the SQLWHERE, so every row'sorganizationIdis guaranteed non-null. The runtime.filter((r): r is { ... } => r.organizationId !== null)exists only to satisfy TypeScript. Prefer asserting the narrower type at the select level (or via a small cast on assignment) so the runtime cost and visual noise go away.♻️ Suggested simplification
- orgShares: activeOrgShares.filter( - (r): r is { id: string; organizationId: string } => - r.organizationId !== null, - ), + orgShares: activeOrgShares as Array<{ id: string; organizationId: string }>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/chats.ts` around lines 745 - 748, The runtime type-narrowing filter on activeOrgShares (the .filter in the orgShares assignment) is unnecessary because the SQL already ensures organizationId is non-null via isNotNull(tables.chatShare.organizationId); remove the runtime filter and instead assert the narrower type where orgShares is assigned (e.g., cast activeOrgShares to the type { id: string; organizationId: string }[] or adjust the select/mapper that produces activeOrgShares) so you eliminate the no-op runtime check while keeping TypeScript satisfied; reference symbols: activeOrgShares, orgShares, and tables.chatShare.organizationId.apps/playground/src/components/playground/share-chat-dialog.tsx (3)
199-216: ⚡ Quick winTwo effects +
eslint-disabledoing overlapping work — consolidate.Lines 205-210 reset
orgShareMaponcurrentChatIdchange but rely on the current (possibly still-stale)orgSharesclosure (hence the eslint-disable for exhaustive-deps). Lines 212-216 then overwrite it again as soon asorgSharesupdates. Functionally this lands on the right state, but the dependency suppression and double-write make the data flow harder to follow. Consider folding them into a single effect keyed on[currentChatId, orgShares], and resettingcreatedShareUrlonly whencurrentChatIdactually changes (track via a ref or compare prev id).♻️ Suggested consolidation
- useEffect(() => { - setOrgShareMap( - Object.fromEntries(orgShares.map((s) => [s.organizationId, s.id])), - ); - setCreatedShareUrl(null); - }, [currentChatId]); // eslint-disable-line react-hooks/exhaustive-deps - - useEffect(() => { - setOrgShareMap( - Object.fromEntries(orgShares.map((s) => [s.organizationId, s.id])), - ); - }, [orgShares]); + const prevChatIdRef = useRef(currentChatId); + useEffect(() => { + setOrgShareMap( + Object.fromEntries(orgShares.map((s) => [s.organizationId, s.id])), + ); + if (prevChatIdRef.current !== currentChatId) { + setCreatedShareUrl(null); + prevChatIdRef.current = currentChatId; + } + }, [currentChatId, orgShares]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/components/playground/share-chat-dialog.tsx` around lines 199 - 216, Consolidate the two useEffect hooks in share-chat-dialog.tsx that set orgShareMap and reset createdShareUrl into a single effect that depends on [currentChatId, orgShares]; inside it compute and setOrgShareMap from orgShares (Object.fromEntries(orgShares.map(...))) and only reset setCreatedShareUrl(null) when currentChatId has actually changed (track the previous currentChatId with a ref or compare a saved value) so you can drop the eslint-disable and avoid the stale-closure/double-write behavior.
326-343: 💤 Low valueTooltip-on-disabled wrapper needs
tabIndexonly when disabled, but RadixTooltipTrigger asChildwill forward props to the<span>.
<TooltipTrigger asChild>passes refs and event handlers to the immediate child. Wrapping with<span className="inline-flex" tabIndex={disabled ? 0 : undefined}>makes the tooltip reachable by hover but, for keyboard users, the focus order changes only whendisabled. When the button is enabled, the span is still in the layout but not focusable — fine — yet Radix attaches its handlers to the span, not the button, which can subtly alter focus management between the two states. Consider keepingtabIndex={0}only on the wrapper and relying onaria-disabledon the button instead of the nativedisabledattribute, so the button itself remains focusable and the tooltip is always reachable. Optional polish; current behavior is acceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/components/playground/share-chat-dialog.tsx` around lines 326 - 343, The wrapper span used with TooltipTrigger should always be keyboard-focusable so Radix forwards handlers consistently: set the span's tabIndex to 0 unconditionally (keep className="inline-flex") and stop toggling focusability based on disabled; then make the Button remain focusable by replacing its native disabled prop with aria-disabled={disabled} (and preserve visual disabled styling via the variant/disabled CSS or a separate prop that doesn't remove focusability), ensuring DialogTrigger still wraps the Button and that pointer events/role remain correct for disabled interaction.
275-295: 💤 Low valueRedundant invalidation already encapsulated by
useDeleteOrgChatShare.
useDeleteOrgChatShareinuseChats.tsalready invalidates the org-shares list when itsorganizationIdarg is provided. Here the hook is instantiated with onlycurrentChatId, then the dialog re-implements the same invalidation manually. Either passorganizationIdto the hook factory (but the hook only takes one org id, so per-row deletion would still need this manual path), or, cleaner, drop the org-shares invalidation from the hook and keep it co-located here. Right now both worlds exist and one is dead code in this call site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/components/playground/share-chat-dialog.tsx` around lines 275 - 295, The code redundantly invalidates the org-shares query here while useDeleteOrgChatShare already performs that invalidation; either remove the manual invalidateQueries block in deleteOrganizationShare (the api.queryOptions / queryClient.invalidateQueries + orgSharesQueryKey code) and instantiate deleteOrgShare via useDeleteOrgChatShare passing the organizationId so the hook handles invalidation, or keep the manual invalidation here and remove the org-shares invalidation from useDeleteOrgChatShare in useChats.ts so only one place invalidates; update deleteOrganizationShare (and the useDeleteOrgChatShare call site) accordingly and keep the setOrgShareMap update as needed.apps/playground/src/hooks/useChats.ts (1)
158-203: 💤 Low valueConsider memoizing the hook factory.
useDeleteOrgChatShare(chatId?, organizationId?)closes over its arguments insideonSuccess, so each render constructs a new mutation with the latest values—that part is fine. However, parameterizing a hook by transient identifiers can be surprising to consumers. Theshare-chat-dialog.tsxconsumer callsuseDeleteOrgChatShare(currentChatId)(withoutorganizationId) and then performs its ownqueryClient.invalidateQueriesfor the org-shares list, which makes the second parameter dead in the only current caller. Consider either dropping the unusedorganizationIdparameter or having the dialog rely on the hook's built-in invalidation and remove the duplicated logic indeleteOrganizationShare.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/hooks/useChats.ts` around lines 158 - 203, The hook useDeleteOrgChatShare currently accepts organizationId and performs org-shares invalidation inside its onSuccess, but that parameter is unused by callers and causes surprising behavior; remove the organizationId parameter from the useDeleteOrgChatShare signature, delete the conditional block that computes orgSharesQueryKey and invalidates org shares in onSuccess, and update callers (e.g., the consumer that calls useDeleteOrgChatShare(currentChatId)) to stop passing organizationId and to remove the duplicate queryClient.invalidateQueries for org-shares (or rely solely on the hook if you prefer centralizing invalidation).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/playground/src/hooks/useChats.ts`:
- Around line 109-135: The share/create mutation currently invalidates per-chat
and org-shares queries but not the global "get /chats" list, causing stale
sidebar data; update the onSuccess handlers in the POST "/chats/{id}/share"
mutation (and similarly in useDeleteChatShare and useDeleteOrgChatShare) to also
compute the queryKey for api.queryOptions("get", "/chats") and call
queryClient.invalidateQueries({ queryKey: thatKey }) so the global chats list
(which contains orgShares/shareId/sharedAt) is refreshed after share
create/delete.
---
Nitpick comments:
In `@apps/api/src/routes/chats.ts`:
- Around line 227-239: Extract the repeated JSON-aggregation SQL used for
orgShares into a single top-level helper (e.g., orgSharesAggregate) that returns
the sql<Array<{ id: string; organizationId: string }>> fragment, then replace
the inline fragments in listChats, searchChats, and getChat with that helper;
ensure the helper references tables.chat.id exactly as the inline versions do
and preserves the COALESCE(... '[]'::json) behaviour and all WHERE filters
(cs.chat_id = ${tables.chat.id}, cs.organization_id IS NOT NULL, cs.deleted_at
IS NULL) so any future changes apply in one place.
- Around line 1010-1078: The listOrgShares route currently returns all matching
shares without pagination; update the createRoute request schema for
listOrgShares to accept optional limit and offset (e.g., z.number().int().min(0)
for offset and a bounded limit), read them from c.req.valid("param"/"query") as
appropriate, apply .limit(limit) and .offset(offset) to the db query (use
defaults like limit=50, offset=0), and optionally run a separate COUNT(*) query
with the same filters to return a total count in the JSON response alongside
shares; ensure you still filter by organizationId and reuse
userHasOrganizationAccess and the same where() conditions so results remain
consistent.
- Around line 802-825: Replace the two-step access check
(userHasOrganizationAccess + separate organization lookup) with a single DB
query that joins the organization table to the membership table and filters for
eq(tables.organization.id, organizationId), membership.userId = user.id (or
equivalent membership field), eq(tables.organization.status, "active") and
eq(tables.organization.isPersonal, false); if the query returns no row throw the
same HTTPException(404, { message: "Organization not found" }). Locate the code
around userHasOrganizationAccess, the subsequent db.select on
tables.organization, and HTTPException and combine them into one atomic
db.select/from/where that enforces membership and org status/isPersonal in one
round trip. Ensure you remove the prior userHasOrganizationAccess call and keep
the same error handling semantics.
- Around line 745-748: The runtime type-narrowing filter on activeOrgShares (the
.filter in the orgShares assignment) is unnecessary because the SQL already
ensures organizationId is non-null via
isNotNull(tables.chatShare.organizationId); remove the runtime filter and
instead assert the narrower type where orgShares is assigned (e.g., cast
activeOrgShares to the type { id: string; organizationId: string }[] or adjust
the select/mapper that produces activeOrgShares) so you eliminate the no-op
runtime check while keeping TypeScript satisfied; reference symbols:
activeOrgShares, orgShares, and tables.chatShare.organizationId.
In `@apps/playground/src/components/playground/share-chat-dialog.tsx`:
- Around line 199-216: Consolidate the two useEffect hooks in
share-chat-dialog.tsx that set orgShareMap and reset createdShareUrl into a
single effect that depends on [currentChatId, orgShares]; inside it compute and
setOrgShareMap from orgShares (Object.fromEntries(orgShares.map(...))) and only
reset setCreatedShareUrl(null) when currentChatId has actually changed (track
the previous currentChatId with a ref or compare a saved value) so you can drop
the eslint-disable and avoid the stale-closure/double-write behavior.
- Around line 326-343: The wrapper span used with TooltipTrigger should always
be keyboard-focusable so Radix forwards handlers consistently: set the span's
tabIndex to 0 unconditionally (keep className="inline-flex") and stop toggling
focusability based on disabled; then make the Button remain focusable by
replacing its native disabled prop with aria-disabled={disabled} (and preserve
visual disabled styling via the variant/disabled CSS or a separate prop that
doesn't remove focusability), ensuring DialogTrigger still wraps the Button and
that pointer events/role remain correct for disabled interaction.
- Around line 275-295: The code redundantly invalidates the org-shares query
here while useDeleteOrgChatShare already performs that invalidation; either
remove the manual invalidateQueries block in deleteOrganizationShare (the
api.queryOptions / queryClient.invalidateQueries + orgSharesQueryKey code) and
instantiate deleteOrgShare via useDeleteOrgChatShare passing the organizationId
so the hook handles invalidation, or keep the manual invalidation here and
remove the org-shares invalidation from useDeleteOrgChatShare in useChats.ts so
only one place invalidates; update deleteOrganizationShare (and the
useDeleteOrgChatShare call site) accordingly and keep the setOrgShareMap update
as needed.
In `@apps/playground/src/hooks/useChats.ts`:
- Around line 158-203: The hook useDeleteOrgChatShare currently accepts
organizationId and performs org-shares invalidation inside its onSuccess, but
that parameter is unused by callers and causes surprising behavior; remove the
organizationId parameter from the useDeleteOrgChatShare signature, delete the
conditional block that computes orgSharesQueryKey and invalidates org shares in
onSuccess, and update callers (e.g., the consumer that calls
useDeleteOrgChatShare(currentChatId)) to stop passing organizationId and to
remove the duplicate queryClient.invalidateQueries for org-shares (or rely
solely on the hook if you prefer centralizing invalidation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b2cb2eb8-cc44-4744-9929-ec52249023b9
⛔ Files ignored due to path filters (4)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/ui/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.ts
📒 Files selected for processing (6)
apps/api/src/routes/chats.tsapps/playground/src/components/playground/chat-header.tsxapps/playground/src/components/playground/chat-page-client.tsxapps/playground/src/components/playground/org-sidebar.tsxapps/playground/src/components/playground/share-chat-dialog.tsxapps/playground/src/hooks/useChats.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/playground/src/components/playground/chat-page-client.tsx
- apps/playground/src/components/playground/org-sidebar.tsx
Summary by CodeRabbit
New Features
Refactor
Bug Fixes