Refine agent visibility controls and browsing filters#1609
Refine agent visibility controls and browsing filters#1609ivaavimusic wants to merge 4 commits intorecoupable:mainfrom
Conversation
|
@ivaavimusic is attempting to deploy a commit to the Recoupable Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors agent form state management by extracting form initialization logic into a dedicated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a04a94f268
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| useEffect(() => { | ||
| if (open) { | ||
| setCurrentSharedEmails(agent.shared_emails || []); | ||
| form.reset({ | ||
| title: agent.title, |
There was a problem hiding this comment.
The new reset effect now depends on the entire agent object, so while the dialog is open any background query refresh that replaces agent (for example React Query refetch on window focus) will call form.reset(...) and discard unsaved edits. This is a user-facing data-loss regression in AgentEditDialog because typing into the form can be silently wiped mid-edit; the reset should be keyed to opening the dialog (or a stable identity change like agent.id) rather than every object refresh.
Useful? React with 👍 / 👎.
| <Switch | ||
| id="isPrivate" | ||
| checked={isPrivate} |
There was a problem hiding this comment.
This switch is rendered without an associated <label>/aria-label, so assistive technologies will announce an unlabeled control and users cannot tell it changes agent visibility. The previous implementation had an explicit label, so this introduces an accessibility regression for screen-reader users in the create/edit form.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/Agents/AgentEditDialog.tsx (1)
46-62:⚠️ Potential issue | 🟠 MajorClear
shareEmailswhen the agent is saved as public.
finalShareEmailsignoresvalues.isPrivate, so flipping a private agent to public still PATCHescurrentSharedEmails. That leaves stale sharing metadata attached to a public template.🧹 Suggested fix
- const finalShareEmails = values.shareEmails && values.shareEmails.length > 0 - ? [...currentSharedEmails, ...values.shareEmails] - : currentSharedEmails; + const finalShareEmails = values.isPrivate + ? [...currentSharedEmails, ...(values.shareEmails ?? [])] + : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Agents/AgentEditDialog.tsx` around lines 46 - 62, finalShareEmails currently concatenates currentSharedEmails with values.shareEmails regardless of privacy state, so when values.isPrivate is false (agent made public) stale currentSharedEmails are still sent; update the logic in AgentEditDialog where finalShareEmails is computed to clear shareEmails when values.isPrivate is false (i.e., set finalShareEmails = [] if !values.isPrivate), otherwise preserve the existing merge behavior (using currentSharedEmails and values.shareEmails), and ensure the PATCH body uses this corrected finalShareEmails field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/Agents/Agents.tsx`:
- Around line 38-66: Wrap the two visibility buttons in an accessible group
(e.g., add role="group" and an aria-label or aria-labelledby) and mark each
toggle button with aria-pressed reflecting the current state so assistive tech
can announce which is active; specifically, update the Public and Private button
elements in Agents.tsx to include aria-pressed={!isPrivate} for the Public
button and aria-pressed={isPrivate} for the Private button (keep existing
onClick handlers like togglePrivate and the isPrivate state logic intact) and
ensure the container element has a clear aria-label describing the control.
In `@components/Agents/AgentVisibilityControl.tsx`:
- Around line 13-34: The Switch with id "isPrivate" is currently unlabeled for
assistive tech; give it an accessible name that reflects the Public/Private
mapping by either setting aria-label dynamically (e.g., "Private" when checked,
"Public" when unchecked) or wire aria-labelledby to one of the visible spans and
ensure that the labeled span text updates with the isPrivate state; update the
Switch props (id "isPrivate", checked={isPrivate}) to include that aria
attribute so screen readers announce the correct state while keeping the
existing form.setValue handler intact.
---
Outside diff comments:
In `@components/Agents/AgentEditDialog.tsx`:
- Around line 46-62: finalShareEmails currently concatenates currentSharedEmails
with values.shareEmails regardless of privacy state, so when values.isPrivate is
false (agent made public) stale currentSharedEmails are still sent; update the
logic in AgentEditDialog where finalShareEmails is computed to clear shareEmails
when values.isPrivate is false (i.e., set finalShareEmails = [] if
!values.isPrivate), otherwise preserve the existing merge behavior (using
currentSharedEmails and values.shareEmails), and ensure the PATCH body uses this
corrected finalShareEmails field.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20f5cec7-8619-4c70-b4ce-7aae99778032
📒 Files selected for processing (8)
components/Agents/AgentEditDialog.tsxcomponents/Agents/AgentVisibilityControl.tsxcomponents/Agents/Agents.tsxcomponents/Agents/CreateAgentDialog.tsxcomponents/Agents/CreateAgentForm.tsxcomponents/Agents/PrivacySection.tsxcomponents/Agents/TagSelector.tsxcomponents/Agents/useAgentForm.ts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| @@ -117,5 +128,3 @@ const AgentEditDialog: React.FC<AgentEditDialogProps> = ({ agent }) => { | |||
| }; | |||
|
|
|||
| export default AgentEditDialog; | |||
There was a problem hiding this comment.
Single Responsibility Principle
- actual: this file is longer than 100 lines of code.
- required: break this monolithic component into smaller sub-components so each file is less than 100 lines of code.
There was a problem hiding this comment.
I’ll break this down into smaller pieces under 100 lines each.
components/Agents/Agents.tsx
Outdated
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| if (isPrivate) togglePrivate(); | ||
| }} | ||
| className={cn( | ||
| "inline-flex items-center justify-center whitespace-nowrap rounded-md px-3 py-1 text-sm font-medium transition-all cursor-pointer", | ||
| !isPrivate | ||
| ? "bg-background text-foreground shadow" | ||
| : "text-muted-foreground hover:text-foreground" | ||
| )} | ||
| > | ||
| Public | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| if (!isPrivate) togglePrivate(); | ||
| }} | ||
| className={cn( | ||
| "inline-flex items-center justify-center whitespace-nowrap rounded-md px-3 py-1 text-sm font-medium transition-all cursor-pointer", | ||
| isPrivate | ||
| ? "bg-background text-foreground shadow" | ||
| : "text-muted-foreground hover:text-foreground" | ||
| )} | ||
| > | ||
| Private | ||
| </button> |
There was a problem hiding this comment.
SRP / OCP
- actual: button component defined within general Agents component.
- required: new button component file.
There was a problem hiding this comment.
Understood. I’ll extract that button into its own component file.
components/Agents/PrivacySection.tsx
Outdated
| <p className="text-sm text-muted-foreground"> | ||
| Add email addresses for the people who should be able to view this private agent. | ||
| </p> |
There was a problem hiding this comment.
Open question: Why add this in PrivacySection instead of within EmailShareInput?
There was a problem hiding this comment.
Yes, fair point. I placed it in PrivacySection because I treated that component as the container for all private-sharing behavior, but I agree the more specific ownership is EmailShareInput. I’ll move it there.
components/Agents/TagSelector.tsx
Outdated
| import { useAgentData } from "./useAgentData"; | ||
| import { CreateAgentFormData } from "./schemas"; | ||
|
|
||
| const FALLBACK_TAGS = [ |
There was a problem hiding this comment.
Why are you adding Fallback tags?
There was a problem hiding this comment.
I added the fallback tags to preserve the category options locally when the backing tag data was sparse, mainly so the modal remained testable and closer to hosted behavior during local UI work. That said, I agree this introduces hardcoded behavior in the UI, so I’ll remove it from this PR.
There was a problem hiding this comment.
Hooks need to go in hooks directory.
- actual: components/Agents/useAgentForm.ts
- required: hooks/useAgentForm.ts
There was a problem hiding this comment.
Agreed. I’ll move this hook into the hooks directory.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
components/Agents/PrivacySection.tsx (1)
26-33: Minor: Inconsistent JSX indentation.The
EmailShareInputcomponent and its props have extra leading spaces compared to the surrounding JSX. While functionally correct, aligning the indentation would improve readability.✨ Aligned indentation
{isPrivate && ( <EmailShareInput - emails={form.watch("shareEmails") ?? []} - existingSharedEmails={existingSharedEmails} - onEmailsChange={(emails) => { - form.setValue("shareEmails", emails, { shouldDirty: true, shouldValidate: true }); - }} - onExistingEmailsChange={onExistingEmailsChange} - /> + emails={form.watch("shareEmails") ?? []} + existingSharedEmails={existingSharedEmails} + onEmailsChange={(emails) => { + form.setValue("shareEmails", emails, { shouldDirty: true, shouldValidate: true }); + }} + onExistingEmailsChange={onExistingEmailsChange} + /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Agents/PrivacySection.tsx` around lines 26 - 33, The JSX for EmailShareInput is misaligned with extra leading spaces; open the component block containing EmailShareInput and normalize indentation so the tag and all its props (EmailShareInput, emails prop using form.watch("shareEmails"), existingSharedEmails, onEmailsChange handler that calls form.setValue(...), and onExistingEmailsChange) align with the surrounding JSX structure—ensure each prop is indented consistently (same level as neighboring components) for readable, consistent formatting.hooks/useAgentForm.ts (1)
24-30: Simplify the useEffect - the else-if branch is unreachable.Given that
defaultValues.shareEmailsis always initialized toinitialValues?.shareEmails ?? []on line 18,form.getValues("shareEmails")will never be falsy. The condition on line 27 is effectively dead code.♻️ Simplified effect
useEffect(() => { if (!isPrivate) { form.setValue("shareEmails", []); - } else if (!form.getValues("shareEmails")) { - form.setValue("shareEmails", []); } }, [isPrivate, form]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useAgentForm.ts` around lines 24 - 30, The else-if branch in the useEffect is unreachable because defaultValues.shareEmails is always initialized (via initialValues?.shareEmails ?? []), so simplify the effect: keep the check on isPrivate and call form.setValue("shareEmails", []) when !isPrivate, and remove the redundant else-if that checks form.getValues("shareEmails"); update the useEffect that references isPrivate, form.setValue, and form.getValues accordingly.hooks/useEditAgentTemplate.ts (2)
22-24: Consider deduplicating emails before sending to API.The merge of
currentSharedEmailsandvalues.shareEmailscould theoretically produce duplicates if the same email appears in both arrays. While the current UI flow makes this unlikely (per context, form always resetsshareEmailsto[]), adding deduplication would be a defensive measure against future edge cases.♻️ Optional deduplication
const finalShareEmails = values.isPrivate - ? [...currentSharedEmails, ...(values.shareEmails ?? [])] + ? [...new Set([...currentSharedEmails, ...(values.shareEmails ?? [])])] : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useEditAgentTemplate.ts` around lines 22 - 24, The merged email array assigned to finalShareEmails in useEditAgentTemplate.ts can contain duplicates from currentSharedEmails and values.shareEmails; update the assignment so when values.isPrivate is true you deduplicate the combined list (e.g., use a Set or filter to preserve first-occurrence order) before sending to the API, while keeping the existing isPrivate branch behavior (and ensure the deduplication happens only when computing finalShareEmails).
29-31: Consider guarding against undefineduserIdbefore making the request.If
userDatahasn't loaded yet,userIdwill beundefined, and the API will reject with a 400 error (per the PATCH handler atapp/api/agent-templates/route.ts:154-156). While the mutation will throw and error handling will catch it, the user experience would be clearer with an early guard or by disabling the submit action until user data is available.🛡️ Proposed early validation
return useMutation({ mutationFn: async (values: CreateAgentFormData) => { + if (!userData?.id) { + throw new Error("User not authenticated"); + } + const finalShareEmails = values.isPrivate ? [...currentSharedEmails, ...(values.shareEmails ?? [])] : []; const res = await fetch("/api/agent-templates", { method: "PATCH", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ id: agent.id, - userId: userData?.id, + userId: userData.id,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useEditAgentTemplate.ts` around lines 29 - 31, The request body may include an undefined userId when userData hasn't loaded; update the mutation in useEditAgentTemplate (the code that builds body: JSON.stringify({ id: agent.id, userId: userData?.id, ... })) to guard early: if userData?.id is falsy, abort the request and surface a clear error (or return early) so no PATCH is sent with undefined userId; alternatively ensure the submit action is disabled until userData is present—change the check in the mutation handler that constructs the body to validate userData?.id and handle the missing user id before calling fetch.components/Agents/AgentsVisibilityFilter.tsx (1)
3-11: Refactor to a value-change API to support controlled + uncontrolled usage.The current
togglePrivatecontract couples this component to flip semantics and forces click guards in the view layer. Prefer a standardvalue/defaultValue + onValueChange(next)shape so the component can be reused cleanly in both controlled and uncontrolled modes.♻️ Proposed refactor
+import { useState } from "react"; import { cn } from "@/lib/utils"; interface AgentsVisibilityFilterProps { - isPrivate: boolean; - togglePrivate: () => void; + isPrivate?: boolean; + defaultIsPrivate?: boolean; + onIsPrivateChange?: (next: boolean) => void; } const AgentsVisibilityFilter = ({ - isPrivate, - togglePrivate, + isPrivate: controlledIsPrivate, + defaultIsPrivate = false, + onIsPrivateChange, }: AgentsVisibilityFilterProps) => { + const [uncontrolledIsPrivate, setUncontrolledIsPrivate] = useState(defaultIsPrivate); + const isPrivate = controlledIsPrivate ?? uncontrolledIsPrivate; + const setIsPrivate = (next: boolean) => { + if (controlledIsPrivate === undefined) setUncontrolledIsPrivate(next); + onIsPrivateChange?.(next); + }; + return ( <div role="group" aria-label="Agent visibility filter" className="inline-flex h-9 items-center justify-center rounded-lg bg-muted p-1 text-muted-foreground" > <button type="button" aria-pressed={!isPrivate} - onClick={() => { - if (isPrivate) togglePrivate(); - }} + onClick={() => setIsPrivate(false)} className={cn( "inline-flex items-center justify-center whitespace-nowrap rounded-md px-3 py-1 text-sm font-medium transition-all cursor-pointer", !isPrivate ? "bg-background text-foreground shadow" : "text-muted-foreground hover:text-foreground" )} > Public </button> <button type="button" aria-pressed={isPrivate} - onClick={() => { - if (!isPrivate) togglePrivate(); - }} + onClick={() => setIsPrivate(true)} className={cn( "inline-flex items-center justify-center whitespace-nowrap rounded-md px-3 py-1 text-sm font-medium transition-all cursor-pointer", isPrivate ? "bg-background text-foreground shadow" : "text-muted-foreground hover:text-foreground" )} > Private </button> </div> ); };As per coding guidelines: "Support both controlled and uncontrolled state in components" and "Provide
defaultValueandonValueChangeprops for state management."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Agents/AgentsVisibilityFilter.tsx` around lines 3 - 11, Change the component API from isPrivate + togglePrivate to a value/defaultValue + onValueChange(next: boolean) shape: update the AgentsVisibilityFilterProps to accept value?: boolean, defaultValue?: boolean, and onValueChange?: (next: boolean) => void, then inside AgentsVisibilityFilter use the controlled pattern (use the passed value when defined otherwise keep internal state initialized from defaultValue) and call onValueChange(next) whenever the visibility changes; replace any togglePrivate usages/click handlers to compute the next boolean and call onValueChange(next) (or update internal state when uncontrolled). Reference: AgentsVisibilityFilter and AgentsVisibilityFilterProps (rename togglePrivate → onValueChange, isPrivate → value/defaultValue) and ensure event handlers use the new onValueChange(next) contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/Agents/AgentsVisibilityFilter.tsx`:
- Around line 3-11: Change the component API from isPrivate + togglePrivate to a
value/defaultValue + onValueChange(next: boolean) shape: update the
AgentsVisibilityFilterProps to accept value?: boolean, defaultValue?: boolean,
and onValueChange?: (next: boolean) => void, then inside AgentsVisibilityFilter
use the controlled pattern (use the passed value when defined otherwise keep
internal state initialized from defaultValue) and call onValueChange(next)
whenever the visibility changes; replace any togglePrivate usages/click handlers
to compute the next boolean and call onValueChange(next) (or update internal
state when uncontrolled). Reference: AgentsVisibilityFilter and
AgentsVisibilityFilterProps (rename togglePrivate → onValueChange, isPrivate →
value/defaultValue) and ensure event handlers use the new onValueChange(next)
contract.
In `@components/Agents/PrivacySection.tsx`:
- Around line 26-33: The JSX for EmailShareInput is misaligned with extra
leading spaces; open the component block containing EmailShareInput and
normalize indentation so the tag and all its props (EmailShareInput, emails prop
using form.watch("shareEmails"), existingSharedEmails, onEmailsChange handler
that calls form.setValue(...), and onExistingEmailsChange) align with the
surrounding JSX structure—ensure each prop is indented consistently (same level
as neighboring components) for readable, consistent formatting.
In `@hooks/useAgentForm.ts`:
- Around line 24-30: The else-if branch in the useEffect is unreachable because
defaultValues.shareEmails is always initialized (via initialValues?.shareEmails
?? []), so simplify the effect: keep the check on isPrivate and call
form.setValue("shareEmails", []) when !isPrivate, and remove the redundant
else-if that checks form.getValues("shareEmails"); update the useEffect that
references isPrivate, form.setValue, and form.getValues accordingly.
In `@hooks/useEditAgentTemplate.ts`:
- Around line 22-24: The merged email array assigned to finalShareEmails in
useEditAgentTemplate.ts can contain duplicates from currentSharedEmails and
values.shareEmails; update the assignment so when values.isPrivate is true you
deduplicate the combined list (e.g., use a Set or filter to preserve
first-occurrence order) before sending to the API, while keeping the existing
isPrivate branch behavior (and ensure the deduplication happens only when
computing finalShareEmails).
- Around line 29-31: The request body may include an undefined userId when
userData hasn't loaded; update the mutation in useEditAgentTemplate (the code
that builds body: JSON.stringify({ id: agent.id, userId: userData?.id, ... }))
to guard early: if userData?.id is falsy, abort the request and surface a clear
error (or return early) so no PATCH is sent with undefined userId; alternatively
ensure the submit action is disabled until userData is present—change the check
in the mutation handler that constructs the body to validate userData?.id and
handle the missing user id before calling fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 917c403c-2831-47b4-9378-5e49ea309f67
📒 Files selected for processing (10)
components/Agents/AgentEditDialog.tsxcomponents/Agents/AgentVisibilityControl.tsxcomponents/Agents/Agents.tsxcomponents/Agents/AgentsVisibilityFilter.tsxcomponents/Agents/CreateAgentDialog.tsxcomponents/Agents/EmailShareInput.tsxcomponents/Agents/PrivacySection.tsxcomponents/Agents/TagSelector.tsxhooks/useAgentForm.tshooks/useEditAgentTemplate.ts
✅ Files skipped from review due to trivial changes (4)
- components/Agents/EmailShareInput.tsx
- components/Agents/TagSelector.tsx
- components/Agents/Agents.tsx
- components/Agents/AgentVisibilityControl.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- components/Agents/CreateAgentDialog.tsx
- components/Agents/AgentEditDialog.tsx
2 participants
Summary
This PR refines how public and private agent visibility is managed and viewed.
Changes
Why
The previous visibility controls were harder to scan and took up too much space in the modal. This makes visibility feel like a normal form field while improving browsing on the agents page.
Risk
Low to medium. The change is mostly UI and state orchestration around existing visibility behavior.
Summary by CodeRabbit
New Features
Improvements