refactor: initial separation of useMetaInfo - #480
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change removes the centralized Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant RequiredAuthProvider
participant OrgProvider
participant ProjectProvider
participant SidebarProvider
participant Children
App->>RequiredAuthProvider: Render
RequiredAuthProvider->>OrgProvider: Provide org context
OrgProvider->>ProjectProvider: Provide project context
ProjectProvider->>SidebarProvider: Provide sidebar context
SidebarProvider->>Children: Render app content
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
frontend/src/components/context/org-context.tsx (3)
17-22: Allownullin the setter signature
useState<OrgMemberInfoClass | null>()returns a setter that legally acceptsnull.
Typing the context setter as(o: OrgMemberInfoClass) => voidforbidsnulland forces unnecessary casts downstream.-type OrgCtx = { +type OrgCtx = { orgs: OrgMemberInfoClass[]; activeOrg: OrgMemberInfoClass | null; - setActiveOrg: (o: OrgMemberInfoClass) => void; + setActiveOrg: (o: OrgMemberInfoClass | null) => void; reloadOrgs: () => Promise<void>; };
31-44:fetchOrgsloop may hammer the auth endpointWith a hard-coded
whileloop +setTimeout(800ms)you can hit the auth endpoint 5 × quickly on a cold start.
Consider leveraging React-Query’s built-in retry/backoff instead of rolling your own polling.
93-101: Minor rerender optimisation
value={{ orgs, activeOrg, setActiveOrg, reloadOrgs }}is recreated every render causing all consumers to re-render even when nothing changed.
Wrap it inuseMemo(or at leastuseCallbackfor functions) to avoid unnecessary updates.frontend/src/components/context/project-context.tsx (1)
18-23: Setter should acceptnullfor symmetry with state-setActiveProject: (p: Project) => void; +setActiveProject: (p: Project | null) => void;frontend/src/components/context/metainfo.tsx (1)
21-30: Redundant type assertions
userClass,activeOrg,activeProject,accessTokenare already typed precisely.
Theascasts hide potential nulls and may mask future refactors. Prefer narrowing earlier rather than casting here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/app/layout.tsx(2 hunks)frontend/src/components/context/loading-guard.tsx(1 hunks)frontend/src/components/context/metainfo.tsx(1 hunks)frontend/src/components/context/org-context.tsx(1 hunks)frontend/src/components/context/project-context.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/components/context/org-context.tsx (1)
frontend/src/components/context/loading-guard.tsx (1)
LoadingGuard(12-31)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Format, Lint, and Test
- GitHub Check: Format & Lint
- GitHub Check: Compose Tests
🔇 Additional comments (3)
frontend/src/components/context/loading-guard.tsx (1)
12-31: LGTM – simple, focused componentThe guard does exactly what it says on the tin and stays client-only. No functional issues spotted.
frontend/src/components/context/org-context.tsx (1)
71-88: Edge-case: zero organisations → infinite skeleton
LoadingGuardremains active while!activeOrg.
If the user legitimately belongs to zero orgs the UI never leaves the loading state.
Handle the “no organisations” path explicitly (e.g. redirect to onboarding or render an empty-state component).frontend/src/components/context/project-context.tsx (1)
38-43:useProjectscalled withundefinedorgIdWhen
activeOrgisnullthe hook is invoked withundefined; ifuseProjectsdoes not guard against this it may fire an unnecessary request or error.
Confirm the hook’s behaviour or gate the call behind!!activeOrg.
| <QueryClientProvider client={queryClient}> | ||
| <RequiredAuthProvider authUrl={process.env.NEXT_PUBLIC_AUTH_URL!}> | ||
| <MetaInfoProvider> | ||
| <SidebarProvider> | ||
| <AppSidebar /> | ||
| <SidebarInset> | ||
| <main className="w-full h-full mr-2 border rounded-lg border-gray-400 border-opacity-30 bg-white"> | ||
| <Header /> | ||
| {children} | ||
| <Analytics /> | ||
| </main> | ||
| </SidebarInset> | ||
| </SidebarProvider> | ||
| </MetaInfoProvider> | ||
| <OrgProvider> | ||
| <ProjectProvider> | ||
| <SidebarProvider> | ||
| <AppSidebar /> | ||
| <SidebarInset> | ||
| <main className="w-full h-full mr-2 border rounded-lg border-gray-400 border-opacity-30 bg-white"> | ||
| <Header /> | ||
| {children} | ||
| <Analytics /> | ||
| </main> | ||
| </SidebarInset> | ||
| </SidebarProvider> | ||
| </ProjectProvider> | ||
| </OrgProvider> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provider order looks correct but be wary of “permanent skeleton” states
Because OrgProvider ➔ ProjectProvider each use LoadingGuard, any failure / empty list will result in an infinite skeleton with no obvious error.
Consider adding an error boundary or a timeout–based fallback so the user is not left staring at a spinner forever if the API keeps returning 0 orgs / 0 projects.
🤖 Prompt for AI Agents
In frontend/src/app/layout.tsx around lines 102 to 117, the nested OrgProvider
and ProjectProvider both use LoadingGuard, which can cause an infinite loading
spinner if the API returns no organizations or projects. To fix this, wrap these
providers or their children with an error boundary component or implement a
timeout-based fallback UI that displays an error message or alternative content
after a certain loading duration, preventing the user from being stuck on a
permanent skeleton state.
| initialData: () => { | ||
| const cache = localStorage.getItem("orgs_cache"); | ||
| if (cache) { | ||
| try { | ||
| return JSON.parse(cache) as OrgMemberInfoClass[]; | ||
| } catch { | ||
| return []; | ||
| } | ||
| } | ||
| return []; | ||
| }, |
There was a problem hiding this comment.
LocalStorage cache is not namespaced by user
"orgs_cache" is shared across all logged-in users on the same browser.
A second account will momentarily see another user’s organisation list until the query resolves – a potential data-leak.
-const cache = localStorage.getItem("orgs_cache");
+const cache = localStorage.getItem(`orgs_cache_${userClass?.userId ?? "anon"}`);Remember to write with the same key in the later useEffect.
📝 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.
| initialData: () => { | |
| const cache = localStorage.getItem("orgs_cache"); | |
| if (cache) { | |
| try { | |
| return JSON.parse(cache) as OrgMemberInfoClass[]; | |
| } catch { | |
| return []; | |
| } | |
| } | |
| return []; | |
| }, | |
| initialData: () => { | |
| const cache = localStorage.getItem(`orgs_cache_${userClass?.userId ?? "anon"}`); | |
| if (cache) { | |
| try { | |
| return JSON.parse(cache) as OrgMemberInfoClass[]; | |
| } catch { | |
| return []; | |
| } | |
| } | |
| return []; | |
| }, |
🤖 Prompt for AI Agents
In frontend/src/components/context/org-context.tsx around lines 58 to 68, the
localStorage key "orgs_cache" is not namespaced by user, causing potential data
leaks between users on the same browser. Fix this by including a unique user
identifier (e.g., user ID or username) in the localStorage key when reading the
cache in initialData and also update the corresponding useEffect that writes to
localStorage to use the same namespaced key. This ensures each user's data is
isolated in localStorage.
| return ( | ||
| <LoadingGuard | ||
| isLoading={!activeOrg || !accessToken || isFetching || !activeProject} | ||
| > | ||
| <ProjectContext.Provider | ||
| value={{ | ||
| projects, | ||
| activeProject, | ||
| setActiveProject, | ||
| reloadActiveProject, | ||
| }} | ||
| > | ||
| {children} | ||
| </ProjectContext.Provider> | ||
| </LoadingGuard> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same infinite-skeleton risk as OrgProvider
If an organisation has zero projects the guard never resolves.
Provide an explicit empty-state instead.
🤖 Prompt for AI Agents
In frontend/src/components/context/project-context.tsx around lines 66 to 81,
the LoadingGuard causes an infinite loading state if the active organization has
zero projects because the guard condition never resolves. To fix this, add an
explicit check for the projects array length and provide a clear empty-state UI
or message when there are no projects, so the LoadingGuard can resolve and avoid
the infinite skeleton loading.
| useEffect(() => { | ||
| if (!activeOrg || !projects.length) return; | ||
| const key = `activeProject_${activeOrg.orgId}`; | ||
| const saved = localStorage.getItem(key); | ||
| setActiveProject(projects.find((p) => p.id === saved) ?? projects[0]); | ||
| }, [projects, activeOrg]); | ||
|
|
||
| useEffect(() => { | ||
| if (activeOrg && activeProject) { | ||
| localStorage.setItem( | ||
| `activeProject_${activeOrg.orgId}`, | ||
| activeProject.id, | ||
| ); | ||
| } | ||
| }, [activeProject, activeOrg]); | ||
|
|
There was a problem hiding this comment.
LocalStorage key should include userId as well
activeProject_${orgId} is unique per org but not per user.
A shared browser profile will leak the previously selected project between accounts.
-const key = `activeProject_${activeOrg.orgId}`;
+const key = `activeProject_${activeOrg.orgId}_${userClass?.userId ?? "anon"}`;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/context/project-context.tsx around lines 46 to 61,
the localStorage key for activeProject uses only the orgId, which can cause
project selection to leak between users sharing the same browser. Modify the key
to include the userId along with orgId to ensure it is unique per user and
organization. Update both the getItem and setItem calls to use this combined key
format.
| if (!accessToken || !activeOrg || !activeProject || !userClass) { | ||
| throw new Error( | ||
| "useMetaInfo: Context not ready - ensure you're using it within the proper providers", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Throwing errors for “not ready” feels harsh
Many components will call useMetaInfo unconditionally and now risk crashing the tree until data hydrates.
Instead of throwing, consider returning {ready:false} or keep the old provider-level guard to avoid propagating error boundaries everywhere.
🤖 Prompt for AI Agents
In frontend/src/components/context/metainfo.tsx around lines 14 to 18, the
current code throws an error if required context values are missing, which can
cause the component tree to crash before data is ready. Instead of throwing an
error, modify the hook to return an object indicating readiness, such as {ready:
false}, when context is not fully available. This approach avoids crashing and
allows components to handle loading states gracefully without relying on error
boundaries.
…/split-org-project-context
…/split-org-project-context
…/split-org-project-context
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| while (!orgs.length && retry < 5) { | ||
| await refreshAuthInfo(); | ||
| await new Promise((r) => setTimeout(r, 1000)); | ||
| orgs = userClass.getOrgs(); |
There was a problem hiding this comment.
Inconsistent null checking when calling getOrgs(). Line 34 uses optional chaining (?.) while line 39 directly accesses the method. If userClass is null during retry attempts, this will cause a runtime error. Should use optional chaining consistently: orgs = userClass?.getOrgs();
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
😱 Found 1 issue. Time to roll up your sleeves! 😱 Need help? Join our Discord for support! |
1 participant
🏷️ Ticket
[link the issue or ticket you are addressing in this PR here, or use the Development
section on the right sidebar to link the issue]
📝 Description
🎥 Demo (if applicable)
📸 Screenshots (if applicable)
✅ Checklist
Summary by CodeRabbit
New Features
Refactor