Fix org settings modal: use same data source as button for name and i…#1434
Fix org settings modal: use same data source as button for name and i…#1434sidneyswift merged 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✨ 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: 2
🧹 Nitpick comments (1)
hooks/useOrgSettings.ts (1)
53-56: RedundantsetIsLoading(false)call.At this point in the execution flow,
isLoadinghasn't been set totrueyet (that happens insidefetchOrgDetails). This makes the code harder to reason about and suggests unclear state transitions.Consider restructuring so loading state is only toggled when an async operation is actually involved, or add a comment explaining the intent:
if (!selectedOrg) { - setIsLoading(false); return; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/useOrgSettings.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (.cursor/rules/component-design.mdc)
**/*.{tsx,ts,jsx,js}: Use semantic HTML elements appropriate to the component's role
Ensure keyboard navigation and focus management in UI components
Provide proper ARIA roles/states and test with screen readers
Start with semantic HTML first, then augment with ARIA if needed
Support both controlled and uncontrolled state in components
Wrap a single HTML element per component (no multiple root elements)
Favor composition over inheritance in component design
Expose clear APIs via props/slots for component customization
UseasChildpattern for flexible element types in components
Support polymorphism withasprop when appropriate
Use@radix-ui/react-*primitives for behavior and accessibility
Useclass-variance-authority(CVA) for component variants
Use@radix-ui/react-slotfor implementingasChildpattern
Use@radix-ui/react-use-controllable-statefor state management
Follow shadcn/ui component structure and naming conventions
Usecn()utility combiningclsxandtailwind-mergefor class merging
Export both component and variant functions (e.g.,Button, buttonVariants)
Use@radix-ui/react-iconsfor UI component icons (not Lucide React)
Uselucide-reactfor application icons and illustrations
Useframer-motionfor animations and transitions
Usenext-themesfor theme management
Usetailwindcss-animatefor CSS animations
Usesonnerfor toast notifications
Useembla-carousel-reactfor carousels
Usereact-resizable-panelsfor resizable layouts
ProvidedefaultValueandonValueChangeprops for state management
Usedata-stateattribute for visual component states (open/closed, loading, etc.)
Usedata-slotattribute for component identification
Use kebab-case naming for data attributes (e.g.,data-slot="form-field")
Follow class order in Tailwind: base → variants → conditionals → user overrides
Define component variants outside components to avoid recreation on re-render
Implement focus trapping in modal/dialog components
Store...
Files:
hooks/useOrgSettings.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/component-design.mdc)
**/*.{tsx,ts}: Extend native HTML attributes usingReact.ComponentProps<"element">pattern
Export component prop types with explicitComponentNamePropsnaming convention
Files:
hooks/useOrgSettings.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/stagehand.mdc)
**/*.{ts,tsx}: Plan instructions using Stagehandobservemethod before executing actions (e.g.,const results = await page.observe("Click the sign in button");)
Cache the results ofobserveto avoid unexpected DOM changes
Keepactactions atomic and specific (e.g., "Click the sign in button" not "Sign in to the website"). Avoid actions that are more than one step.
Use variable substitution with thevariablesparameter inactfor dynamic form filling (e.g.,action: "Enter Name: %name%", variables: { name: "John Doe" })
Always use Zod schemas for structured data extraction with theextractmethod (e.g.,schema: z.object({ text: z.string() }))
Wrap array extractions in a single object when extracting multiple items usingextract(e.g.,schema: z.object({ buttons: z.array(z.string()) }))
Use explicitinstructionandschemaparameters when callingextractmethod
Import Stagehand types from@browserbasehq/stagehandpackage (e.g.,import { Stagehand, Page, BrowserContext } from "@browserbasehq/stagehand";)
Initialize Stagehand withnew Stagehand()constructor and callawait stagehand.init()before using page automation methods
Implement main automation logic in functions that accept{ page, context, stagehand }parameters
Provide specific instructions to agents instead of vague ones (e.g., "Navigate to products page and filter by 'Electronics'" not "Do some stuff on this page")
Break down complex agent tasks into smaller steps for better execution
Use error handling with try/catch blocks when executing agent tasks
Combine agents for navigation with traditional methods for precise data extraction
**/*.{ts,tsx}: Create single-responsibility components with obvious data flow
Use strict TypeScript types with zero 'any' types
Follow Next.js optimization guides for performance
Files:
hooks/useOrgSettings.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx,js}: Write minimal code - use only the absolute minimum code needed
Ensure code is self-documenting through precise naming with verbs for functions and nouns for variables
Add short comments only when necessary to clarify non-obvious logic
Implement built-in security practices for authentication and data handling
Consider if code can be split into smaller functions before implementing
Avoid unnecessary abstractions during implementation
Ensure code clarity is suitable for understanding by junior developers
Files:
hooks/useOrgSettings.ts
hooks/**/*.ts
⚙️ CodeRabbit configuration file
hooks/**/*.ts: For custom hooks, ensure:
- Single responsibility per hook
- Return consistent interface
- Handle edge cases and errors
- Use proper dependency arrays
- Keep hooks focused and reusable
- Follow naming convention (use prefix)
- DRY: Extract common hook logic into shared utilities
- KISS: Avoid complex hook compositions, prefer simple state management
Files:
hooks/useOrgSettings.ts
🧬 Code graph analysis (1)
hooks/useOrgSettings.ts (1)
lib/consts.ts (1)
NEW_API_BASE_URL(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vercel Agent Review
- GitHub Check: Run evaluations
🔇 Additional comments (1)
hooks/useOrgSettings.ts (1)
83-158: LGTM!The callback functions are well-structured with proper
useCallbackusage and correct dependency arrays. The hook maintains single responsibility and exposes a clear, consistent interface.
| const { userData } = useUserProvider(); | ||
| const { data: organizations } = useAccountOrganizations(); |
There was a problem hiding this comment.
Unused userData variable.
The userData is destructured from useUserProvider() but is never referenced anywhere in this hook. This violates the clean code principle of avoiding dead code and creates unnecessary coupling to the UserProvider context.
const useOrgSettings = (orgId: string | null) => {
- const { userData } = useUserProvider();
const { data: organizations } = useAccountOrganizations();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { userData } = useUserProvider(); | |
| const { data: organizations } = useAccountOrganizations(); | |
| const { data: organizations } = useAccountOrganizations(); |
🤖 Prompt for AI Agents
In hooks/useOrgSettings.ts around lines 23–24, the destructured userData from
useUserProvider() is never used; remove the unused variable by either deleting
the call to useUserProvider() entirely if there are no side effects needed, or
change the line to not destructure userData (e.g., call the hook without
extracting userData) so the dead variable is eliminated and lint warnings are
resolved.
| const fetchOrgDetails = async () => { | ||
| setIsLoading(true); | ||
| try { | ||
| const response = await fetch(`${NEW_API_BASE_URL}/api/accounts/${orgId}`); | ||
| if (response.ok) { | ||
| const data = await response.json(); | ||
| setOrgData(data.data); | ||
| setName(data.data?.name || ""); | ||
| setImage(data.data?.image || ""); | ||
| setInstruction(data.data?.instruction || ""); | ||
| setKnowledges(data.data?.knowledges || []); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error fetching org details:", error); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchOrg(); | ||
| }, [orgId]); | ||
| fetchOrgDetails(); | ||
| }, [orgId, organizations]); |
There was a problem hiding this comment.
Missing cleanup for async operation creates potential race condition.
If orgId or organizations change while a fetch is in progress, the previous request continues and may complete after a newer request, potentially setting stale data. Consider using an AbortController for cleanup.
Additionally, non-ok responses (e.g., 404, 500) are silently ignored, which can make debugging difficult.
+ const abortController = new AbortController();
+
const fetchOrgDetails = async () => {
setIsLoading(true);
try {
- const response = await fetch(`${NEW_API_BASE_URL}/api/accounts/${orgId}`);
+ const response = await fetch(`${NEW_API_BASE_URL}/api/accounts/${orgId}`, {
+ signal: abortController.signal,
+ });
if (response.ok) {
const data = await response.json();
setOrgData(data.data);
setInstruction(data.data?.instruction || "");
setKnowledges(data.data?.knowledges || []);
+ } else {
+ console.error("Failed to fetch org details:", response.status);
}
} catch (error) {
+ if (error instanceof Error && error.name === "AbortError") return;
console.error("Error fetching org details:", error);
} finally {
setIsLoading(false);
}
};
fetchOrgDetails();
+
+ return () => {
+ abortController.abort();
+ };
}, [orgId, organizations]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchOrgDetails = async () => { | |
| setIsLoading(true); | |
| try { | |
| const response = await fetch(`${NEW_API_BASE_URL}/api/accounts/${orgId}`); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| setOrgData(data.data); | |
| setName(data.data?.name || ""); | |
| setImage(data.data?.image || ""); | |
| setInstruction(data.data?.instruction || ""); | |
| setKnowledges(data.data?.knowledges || []); | |
| } | |
| } catch (error) { | |
| console.error("Error fetching org details:", error); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }; | |
| fetchOrg(); | |
| }, [orgId]); | |
| fetchOrgDetails(); | |
| }, [orgId, organizations]); | |
| const abortController = new AbortController(); | |
| const fetchOrgDetails = async () => { | |
| setIsLoading(true); | |
| try { | |
| const response = await fetch(`${NEW_API_BASE_URL}/api/accounts/${orgId}`, { | |
| signal: abortController.signal, | |
| }); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| setOrgData(data.data); | |
| setInstruction(data.data?.instruction || ""); | |
| setKnowledges(data.data?.knowledges || []); | |
| } else { | |
| console.error("Failed to fetch org details:", response.status); | |
| } | |
| } catch (error) { | |
| if (error instanceof Error && error.name === "AbortError") return; | |
| console.error("Error fetching org details:", error); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }; | |
| fetchOrgDetails(); | |
| return () => { | |
| abortController.abort(); | |
| }; | |
| }, [orgId, organizations]); |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| if (!selectedOrg) { | ||
| setIsLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When a selected organization is not found in the organizations list, only isLoading is reset to false without clearing other state variables, leaving stale organization data (name, image) displayed in the UI.
View Details
📝 Patch Details
diff --git a/hooks/useOrgSettings.ts b/hooks/useOrgSettings.ts
index e6e0504c..870b1f8d 100644
--- a/hooks/useOrgSettings.ts
+++ b/hooks/useOrgSettings.ts
@@ -51,6 +51,11 @@ const useOrgSettings = (orgId: string | null) => {
);
if (!selectedOrg) {
+ setOrgData(null);
+ setName("");
+ setImage("");
+ setInstruction("");
+ setKnowledges([]);
setIsLoading(false);
return;
}
Analysis
Stale organization data displayed when selected organization not found in list
What fails: When navigating to an organization that's not in the user's organizations list, the useOrgSettings hook clears isLoading but leaves name, image, instruction, and knowledges state intact, causing the UI to display the previously viewed organization's data instead of clearing it.
How to trigger it:
- View Organization A settings - loads successfully with name and image populated
- Change permissions or context so Organization B is not in the user's organizations list
- Navigate to Organization B settings
- The component renders Organization A's name and image instead of a clean/empty state
Result: UI displays stale data from Organization A while attempting to load Organization B
Expected: When selectedOrg is not found in the organizations list (lines 52-55), all state variables should be cleared to match the behavior at lines 40-44. This prevents stale data from being displayed.
Root cause: Inconsistent state clearing logic - lines 40-44 clear all state when organizations is null/undefined, but lines 52-55 only reset isLoading when the organization is not found in the list. This asymmetry causes stale data to persist in the UI.
![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.
ℹ️ 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".
| if (!orgId || !organizations) { | ||
| setOrgData(null); | ||
| setName(""); | ||
| setImage(""); | ||
| setInstruction(""); |
There was a problem hiding this comment.
The guard now returns when organizations is falsy (lines 39‑43), clearing the form state and leaving isLoading false, so the modal renders an empty, saveable form and never calls /api/accounts/${orgId} whenever the organizations query is still loading or has errored. In those cases org settings can no longer load and a user could overwrite existing data with blanks, whereas previously the hook fetched as soon as orgId was set independent of the org list.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const useOrgSettings = (orgId: string | null) => { | ||
| const { userData } = useUserProvider(); |
There was a problem hiding this comment.
YAGNI principle
- actual:
userDatais unused in this file. - required: delete unused variables.
2 participants
…mage
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.