Refactor: Centralize State Management & remove unnecessary api call #267
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis set of changes refactors the playground feature to centralize all data fetching and state management for agents, linked accounts, apps, and app functions within a global store, removing local state and effects from individual components. Components such as Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PlaygroundPage
participant SettingsSidebar
participant AgentStore
participant API
User->>PlaygroundPage: Loads page
PlaygroundPage->>AgentStore: Reads state (agents, linked accounts, etc.)
PlaygroundPage->>SettingsSidebar: Renders with store-backed data
SettingsSidebar->>AgentStore: initializeFromProject(project)
AgentStore->>API: Fetch agents, linked accounts, apps, functions
API-->>AgentStore: Returns data
AgentStore->>SettingsSidebar: Updates state
User->>SettingsSidebar: Selects agent/linked account/app/function
SettingsSidebar->>AgentStore: Updates selection
AgentStore->>SettingsSidebar: Provides filtered data
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
jiwei-aipolabs
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
frontend/src/app/playground/page.tsx (2)
37-41:⚠️ Potential issueAvoid sending an empty
linked_account_owner_idfield
selectedLinkedAccountOwnerIdis an empty string on initial load and whenever a user explicitly clears the selection.
Many back-ends treat an empty string differently from “field not provided”, sometimes triggering validation errors.Consider constructing the body object dynamically so the field is included only when a non-empty value exists.
- body: { - linked_account_owner_id: selectedLinkedAccountOwnerId, - selected_apps: selectedApps, - selected_functions: selectedFunctions, - }, + body: { + ...(selectedLinkedAccountOwnerId && { + linked_account_owner_id: selectedLinkedAccountOwnerId, + }), + selected_apps: selectedApps, + selected_functions: selectedFunctions, + },
62-82: 🛠️ Refactor suggestionDuplicate parameter building – extract helper & ensure value presence
Exactly the same argument object is built twice for both
ACI_EXECUTE_FUNCTIONand the default branch. Apart from increasing maintenance cost, it repeats the risk noted above (emptylinked_account_owner_id).Refactor into a small helper that returns the correct payload and omits the field when not set:
- result = await executeFunction( - toolCall.toolName, - { - function_input: toolCall.args as Record<string, unknown>, - linked_account_owner_id: selectedLinkedAccountOwnerId, - }, - apiKey, - ); + const payload = { + function_input: toolCall.args as Record<string, unknown>, + ...(selectedLinkedAccountOwnerId && { + linked_account_owner_id: selectedLinkedAccountOwnerId, + }), + }; + result = await executeFunction(toolCall.toolName, payload, apiKey);
🧹 Nitpick comments (9)
frontend/src/app/playground/playground-settings.tsx (1)
28-53: Unnecessary re-executions ofinitializeDataEvery state change inside the store will create new function references, causing this effect to re-run because the functions are in the dependency array.
Wrap the store selectors inuseAgentStorewith a stable selector or list onlyactiveProjectas a dependency.-}, [ - activeProject, - initializeFromProject, - fetchLinkedAccounts, - fetchApps, - fetchAppFunctions, - getApiKey, -]); +}, [activeProject]);Alternatively use
useAgentStoreselectors that guarantee referential stability.frontend/src/app/playground/setting-app-selector.tsx (2)
36-39: Subscribing to the entire store hurts performance
useAgentStore()without a selector causes this component to re-render on any store change, even those unrelated to apps.Use a selector with shallow comparison to subscribe only to the pieces you need:
-const { selectedApps, setSelectedApps, getAvailableApps } = useAgentStore(); +const { selectedApps, setSelectedApps, getAvailableApps } = useAgentStore( + (s) => ({ + selectedApps: s.selectedApps, + setSelectedApps: s.setSelectedApps, + getAvailableApps: s.getAvailableApps, + }), + shallow, +);(Remember to import
shallowfromzustand/shallow.)
118-137: Consider memoisingappList
getAvailableApps()is invoked on every render and loops over all apps.
Wrap the transformation inuseMemowithselectedAppsand the raw list as deps to avoid unnecessary work:-const appList = getAvailableApps().map((app) => ({ +const appList = useMemo( + () => + getAvailableApps().map((app) => ({ id: app.name, name: app.display_name || app.name, icon: /* … */, -})); + })), + [getAvailableApps, selectedApps], +);Not critical, but beneficial once the app catalog grows.
frontend/src/app/playground/setting-linked-account-owner-id-selector.tsx (2)
35-40: Reset order might leave stale derived state
setSelectedLinkedAccountOwnerIdis invoked before clearing selected apps / functions.
If any listeners derive their state based on the previous owner + current apps/functions, they’ll observe a transient inconsistent state.
Setting the owner after clearing dependent selections avoids that edge case.-const resetSelectedLinkedAccountOwnerId = (value: string) => { - setSelectedLinkedAccountOwnerId(value); - setMessages([]); - setSelectedApps([]); - setSelectedFunctions([]); -}; +const resetSelectedLinkedAccountOwnerId = (value: string) => { + setMessages([]); + setSelectedApps([]); + setSelectedFunctions([]); + setSelectedLinkedAccountOwnerId(value); +};
64-71: Micro-perf: avoid recomputing unique accounts on every render
getUniqueLinkedAccounts()constructs a newMapeach call, so invoking it inside JSX causes work on every re-render.
Consider memoising the result in the store or pulling it once into a localconstbefore thereturn.frontend/src/lib/store/agent.ts (4)
3-6: Duplicate import from the same module
ProjectandAgentare imported in two separate statements from"../types/project".
Consolidating them into one keeps the import list tidy and prevents accidental circular-import headaches.-import { Project } from "../types/project"; -... -import { Agent } from "../types/project"; +import { Project, Agent } from "../types/project";
60-65: Method name shadows the imported utility, risks confusionThe store property
getApiKeyshadows thegetApiKeyfunction imported from../api/util.
While it currently works due to JavaScript’s hoisting rules, the double meaning is hard to read and easy to mis-reference.Rename the store method to something like
deriveApiKeyorgetSelectedAgentApiKeyto make intent crystal-clear.- getApiKey: (activeProject: Project) => { - const apiKey = getApiKey(activeProject, get().selectedAgent); + deriveApiKey: (activeProject: Project) => { + const apiKey = getApiKey(activeProject, get().selectedAgent);
76-87:getUniqueLinkedAccountsrecalculates on every callBecause it rebuilds a
Mapeach time, using this inside React render paths scales O(N) per render.
Consider:
- Storing the unique array once when
linkedAccountsis set, or- Returning a memoised selector via
zustand’ssubscribeWithSelector.Either eliminates redundant work.
100-121: Potential O(N²) filtering – consider pre-indexing
getAvailableAppsdoes two passes over arrays (filter+some) inside each other, leading to O(M×N) complexity.
With dozens of apps & accounts this isn’t huge, but it’s easy to optimise:const accountsByOwner = new Set( get().linkedAccounts .filter(a => !get().selectedLinkedAccountOwnerId || a.linked_account_owner_id === get().selectedLinkedAccountOwnerId) .map(a => a.app_name), ); return get().apps.filter(app => get().allowedApps.includes(app.name) && accountsByOwner.has(app.name));This converts the inner
someto O(1) lookups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/src/app/playground/page.tsx(5 hunks)frontend/src/app/playground/playground-settings.tsx(1 hunks)frontend/src/app/playground/setting-agent-selector.tsx(1 hunks)frontend/src/app/playground/setting-app-selector.tsx(4 hunks)frontend/src/app/playground/setting-function-selector.tsx(4 hunks)frontend/src/app/playground/setting-linked-account-owner-id-selector.tsx(2 hunks)frontend/src/lib/api/appfunction.ts(1 hunks)frontend/src/lib/store/agent.ts(1 hunks)frontend/src/lib/types/appfunction.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
frontend/src/app/playground/setting-function-selector.tsx (1)
frontend/src/lib/store/agent.ts (1)
useAgentStore(39-166)
frontend/src/app/playground/playground-settings.tsx (3)
frontend/src/lib/store/agent.ts (1)
useAgentStore(39-166)frontend/src/lib/api/util.ts (1)
getApiKey(3-23)frontend/src/app/playground/setting-agent-selector.tsx (1)
AgentSelector(24-95)
frontend/src/app/playground/setting-app-selector.tsx (1)
frontend/src/lib/store/agent.ts (1)
useAgentStore(39-166)
frontend/src/app/playground/setting-linked-account-owner-id-selector.tsx (1)
frontend/src/lib/store/agent.ts (1)
useAgentStore(39-166)
frontend/src/lib/store/agent.ts (4)
backend/aci/common/db/sql_models.py (4)
LinkedAccount(379-433)Agent(111-157)App(247-297)Project(64-108)frontend/src/lib/types/appfunction.ts (1)
AppFunction(1-8)frontend/src/lib/api/util.ts (1)
getApiKey(3-23)frontend/src/lib/api/appfunction.ts (1)
searchFunctions(64-107)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Format & Lint
- GitHub Check: Format, Lint, and Test
- GitHub Check: Compose Tests
🔇 Additional comments (14)
frontend/src/lib/types/appfunction.ts (1)
21-28: Good organization of type definitions!Moving the
FunctionsSearchParamsinterface to this file centralizes all app function related types in one location. This is a good practice for maintaining clean architecture and makes types easier to discover, import, and maintain.frontend/src/lib/api/appfunction.ts (1)
5-5: Clean import updateThe import change correctly reflects the type interface's new location. This maintains consistency with the type definition centralization.
frontend/src/app/playground/setting-function-selector.tsx (6)
3-3: Import cleanupGood job streamlining imports to only what's necessary after removing local state management.
16-16: Store import for centralized state managementAdding the Zustand store import aligns with the PR objective of centralizing state management.
31-36: Effective use of centralized stateNicely refactored to use the global store for all function-related state and operations, eliminating redundant local state and API calls.
38-38: Clean function filteringUsing
getAvailableAppFunctions()directly from the store simplifies the component and ensures consistent filtering logic across the application.
76-77: Loading state from global storeThe loading indicator now correctly uses the centralized
loadingFunctionsstate, maintaining consistency with the global state management approach.
104-111: Consistent UI handling based on loading stateThe component correctly handles the loading state from the store and renders the appropriate UI based on that state. This creates a consistent user experience.
frontend/src/app/playground/setting-agent-selector.tsx (4)
18-18: Added meta context accessAdding
useMetaInfoprovides access to the active project, which is necessary for getting the correct API key.
24-35: Comprehensive refactoring to use centralized stateExcellent refactoring to use the global agent store for all agent-related state and operations. The component now:
- Accesses the active project from context
- Retrieves agents and selected agent from the store
- Uses store methods for state updates and API calls
This approach centralizes state management and eliminates prop drilling.
41-42: Reset selections on agent changeGood addition to clear selected apps and functions when changing agents, preventing invalid selections across agent contexts.
48-48: Fetch app functions with the correct API keyThe code now properly fetches app functions when an agent changes, using the API key from the active project. This ensures that the functions list is always up-to-date.
frontend/src/app/playground/page.tsx (1)
106-107: Prop naming drift – ensureChatInputstill expectslinkedAccountOwnerId
ChatInputnow receiveslinkedAccountOwnerId={selectedLinkedAccountOwnerId}.
Double-check that the component’s prop name wasn’t renamed toselectedLinkedAccountOwnerIdduring the refactor; otherwise TS/ESLint will flag an unknown prop.If a rename happened, update this usage accordingly.
frontend/src/lib/store/agent.ts (1)
145-154: String-prefix check ties logic to naming convention
getAvailableAppFunctionsrelies on function names starting with"APP_NAME__".
If back-end naming ever changes (e.g. kebab-case, camelCase), the selector silently breaks.Safer alternatives:
• Comparefunc.app_name(already present onAppFunction) againstselectedApps.
• Or export a small helper that encodes the naming policy in one place.-return get().appFunctions.filter((func) => - selectedApps.some((appName) => - func.name.startsWith(`${appName.toUpperCase()}__`), - ), -); +return get().appFunctions.filter((func) => + selectedApps.includes(func.app_name), +);Would adopting
func.app_namecover all use-cases, or do you rely on cross-app prefixes elsewhere?
frontend/src/app/playground/setting-linked-account-owner-id-selector.tsx
Show resolved
Hide resolved
| fetchLinkedAccounts: async (apiKey: string) => { | ||
| try { | ||
| const accounts = await getAllLinkedAccounts(apiKey); | ||
| set((state) => ({ ...state, linkedAccounts: accounts })); | ||
| return accounts; | ||
| } catch (error) { | ||
| console.error("Failed to fetch linked accounts:", error); | ||
| throw error; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Filter out disabled linked accounts at source
fetchLinkedAccounts stores all accounts returned by the API.
Down-stream selectors (e.g. UI pickers) will then have to remember to hide disabled ones, risking leaks.
-const accounts = await getAllLinkedAccounts(apiKey);
-set((state) => ({ ...state, linkedAccounts: accounts }));
+const accounts = (await getAllLinkedAccounts(apiKey)).filter(
+ (a) => a.enabled,
+);
+set((state) => ({ ...state, linkedAccounts: accounts }));This keeps the store’s data set clean and simplifies selectors.
📝 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.
| fetchLinkedAccounts: async (apiKey: string) => { | |
| try { | |
| const accounts = await getAllLinkedAccounts(apiKey); | |
| set((state) => ({ ...state, linkedAccounts: accounts })); | |
| return accounts; | |
| } catch (error) { | |
| console.error("Failed to fetch linked accounts:", error); | |
| throw error; | |
| } | |
| }, | |
| fetchLinkedAccounts: async (apiKey: string) => { | |
| try { | |
| const accounts = (await getAllLinkedAccounts(apiKey)).filter( | |
| (a) => a.enabled, | |
| ); | |
| set((state) => ({ ...state, linkedAccounts: accounts })); | |
| return accounts; | |
| } catch (error) { | |
| console.error("Failed to fetch linked accounts:", error); | |
| throw error; | |
| } | |
| }, |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/app/playground/playground-settings.tsx (1)
33-34: Initialize store before deriving the API key
getApiKey(activeProject)is called before the store is fully initialized, yet it relies onselectedAgentbeing set.Swap the two calls so the store is ready before the API key is derived:
- initializeFromProject(activeProject); - const apiKey = getApiKey(activeProject); + initializeFromProject(activeProject); + const apiKey = getApiKey(activeProject);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/playground/playground-settings.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/app/playground/playground-settings.tsx (3)
frontend/src/lib/store/agent.ts (1)
useAgentStore(39-166)frontend/src/lib/api/util.ts (1)
getApiKey(3-23)frontend/src/app/playground/setting-agent-selector.tsx (1)
AgentSelector(24-95)
🪛 Biome (1.9.4)
frontend/src/app/playground/playground-settings.tsx
[error] 43-43: expected , but instead found catch
Remove catch
(parse)
[error] 43-43: expected , but instead found (
Remove (
(parse)
[error] 43-43: expected , but instead found {
Remove {
(parse)
[error] 44-44: expected , but instead found .
Remove .
(parse)
[error] 44-44: expected , but instead found )
Remove )
(parse)
[error] 44-44: expected , but instead found ;
Remove ;
(parse)
[error] 47-47: Expected a statement but instead found '}'.
Expected a statement here.
(parse)
[error] 50-57: Expected a statement but instead found '}, [
activeProject,
initializeFromProject,
fetchLinkedAccounts,
fetchApps,
fetchAppFunctions,
getApiKey,
])'.
Expected a statement here.
(parse)
🪛 GitHub Actions: Dev Portal Checks
frontend/src/app/playground/playground-settings.tsx
[error] 43-43: Prettier syntax error: ',' expected at line 43, column 9. Likely a syntax issue in the catch block.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Format & Lint
- GitHub Check: Compose Tests
🔇 Additional comments (5)
frontend/src/app/playground/playground-settings.tsx (5)
9-12: Imports updated for centralized state managementThe imports now include hooks for state management (
useAgentStore), notifications (toast), and project context (useMetaInfo), which aligns with the PR objective of centralizing state management.
13-16: Props interface simplifiedThe
SettingsSidebarPropsinterface has been simplified by removing thelinkedAccountsandagentsproperties, as these are now retrieved from the centralized store.
18-27: Component updated to use centralized storeThe component now uses the central store for data and operations rather than receiving agents and linked accounts as props.
65-69: Component props updated for centralized stateThe
AgentSelectorandLinkAccountOwnerIdSelectorcomponents now receive fewer props, as they retrieve agents and linked accounts from the central store.
28-42:⚠️ Potential issueFix syntax error in initialization function
There's a syntax error in the
initializeDatafunction where the function is not properly closed before a secondcatchblock appears.const initializeData = async () => { try { initializeFromProject(activeProject); const apiKey = getApiKey(activeProject); // Initialize settings data (agents, linked accounts, apps, app functions) await fetchLinkedAccounts(apiKey); await fetchApps(apiKey); await fetchAppFunctions(apiKey); - } catch (error) { - // handle error - } - }Likely an incorrect or invalid review comment.
dev-aipolabs
changed the title
…ry request
🏷️ Ticket
https://www.notion.so/make-agent-request-once-apps-and-functions-information-1df8378d6a4780f8a1dffc4bba5bb91d?pvs=4
📝 Description
Avoid redundant API request; refactor state and request logic into Zustand
When switching agents, we still need to fetch appFunctions because each agent can have a different set of enabled apps. For performance reasons, we only request the list of enabled functions and not full function definitions list. This make the response lightweight, but we could change this in the future if use cases require fetch all functions definition.
🎥 Sceenshot (if applicable)
Previous:
Now these unnecessary API call have been removed.
✅ Checklist
Summary by CodeRabbit
Refactor
New Features
Style
Chores