fix(frontend): Credentials disabling onboarding Run button - #11244
Conversation
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors the agent onboarding step 5 run flow by introducing new components, extracting credential management logic, and implementing a custom hook for centralized state management. Changes authentication handling to expose the user object directly instead of a boolean flag. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as page.tsx
participant Hook as useOnboardingRunStep
participant API as Queries
participant Provider as Onboarding Provider
User->>Page: Load step 5
Page->>Hook: useOnboardingRunStep()
Hook->>API: Fetch storeAgent + GraphMeta
API-->>Hook: Agent data
Hook->>Hook: Compute initial inputs
Hook->>Provider: Update agentInput state
Hook-->>Page: Return state & handlers
Page->>Page: Render SelectedAgentCard + RunAgentHint
alt Input not shown
Page->>User: Show RunAgentHint
else Input shown
Page->>Page: Render AgentOnboardingCredentials
Page->>Page: Render input fields
end
User->>Page: Click "New run"
Page->>Hook: handleNewRun()
Hook->>API: Add agent to library
Hook->>API: Execute graph
Hook->>Provider: Update run ID & count
Hook->>User: Navigate to congrats
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes introduce significant structural modifications: a new custom hook managing complex state and side effects, multiple new components with prop drilling, credential management logic extraction and reorganization, and integration changes across multiple files. The heterogeneity of changes (new components, hook creation, state refactoring, authentication API adjustment) and dense logic in useOnboardingRunStep and page.tsx refactoring demand separate reasoning paths. Authentication changes introduce behavioral shifts requiring careful verification. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
✅ Deploy Preview for auto-gpt-docs canceled.
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
|
You are above your monthly Qodo Merge usage quota. For more information, please visit here. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/AgentOnboardingCredentials/AgentOnboardingCredentials.tsx (1)
46-63: Still a race: aggregate child “loaded” signals before enabling Run.Each CredentialsInput calls onLoaded independently; forwarding
onLoadingChange(!loaded)per child can flip the parent to “loaded” while other inputs are still initializing. We need per-field aggregation (Set of loaded keys) and only mark loaded when all fields are loaded. Also initialize validity and propagate defaults on first render to avoidcredentialsValidstaying true with missing creds.Apply this diff:
- import { useState } from "react"; + import { useEffect, useMemo, useState } from "react"; @@ - const fields = getCredentialFields(props.agent); - const required = Object.keys(fields || {}).length > 0; + const fields = getCredentialFields(props.agent); + const fieldEntries = useMemo(() => Object.entries(fields || {}), [fields]); + const fieldKeys = useMemo(() => fieldEntries.map(([k]) => k), [fieldEntries]); + const required = fieldKeys.length > 0; @@ - if (!required) return null; + if (!required) return null; @@ - function handleSelectCredentials(key: string, value: Credential) { + function handleSelectCredentials(key: string, value: Credential) { const updated = { ...inputCredentials, [key]: value }; setInputCredentials(updated); - const sanitized: Record<string, CredentialsMetaInput> = {}; + const sanitized: Record<string, CredentialsMetaInput> = {}; for (const [k, v] of Object.entries(updated)) { - if (v) sanitized[k] = v; + if (v != null) sanitized[k] = v; } props.onCredentialsChange(sanitized); const isValid = !required || areAllCredentialsSet(fields, updated); props.onValidationChange(isValid); } - if (!required) return null; + // Track which credential inputs finished loading and propagate a single parent loading state + const [loadedKeys, setLoadedKeys] = useState<Set<string>>(new Set()); + useEffect(() => { + // Reset when field set changes + setLoadedKeys(new Set()); + // Assume loading until all children report loaded + props.onLoadingChange(required); + }, [required, fieldKeys.length]); + useEffect(() => { + props.onLoadingChange(loadedKeys.size !== fieldKeys.length); + }, [loadedKeys, fieldKeys.length]); + + // Initialize defaults and initial validity once fields are known + useEffect(() => { + if (!required) return; + const defaults: Record<string, Credential> = {}; + for (const [k, schema] of fieldEntries) { + defaults[k] = getSchemaDefaultCredentials(schema); + } + setInputCredentials(defaults); + const sanitized: Record<string, CredentialsMetaInput> = {}; + for (const [k, v] of Object.entries(defaults)) { + if (v != null) sanitized[k] = v; + } + props.onCredentialsChange(sanitized); + props.onValidationChange(areAllCredentialsSet(fields, defaults)); + }, [required, fieldEntries, fields]); @@ - {Object.entries(fields).map(([key, inputSubSchema]) => ( + {fieldEntries.map(([key, inputSubSchema]) => ( <div key={key} className="mt-4"> <CredentialsInput schema={inputSubSchema} selectedCredentials={ inputCredentials[key] ?? getSchemaDefaultCredentials(inputSubSchema) } onSelectCredentials={(value) => handleSelectCredentials(key, value)} siblingInputs={props.siblingInputs} - onLoaded={(loaded) => props.onLoadingChange(!loaded)} + onLoaded={(loaded) => + setLoadedKeys((prev) => { + const next = new Set(prev); + if (loaded) next.add(key); + else next.delete(key); + return next; + }) + } /> </div> ))}Also applies to: 48-59
🧹 Nitpick comments (6)
autogpt_platform/frontend/src/app/(platform)/library/agents/[id]/components/AgentRunsView/components/CredentialsInputs/CredentialsInputs.tsx (1)
134-139: Consider optimizing useEffect dependenciesThe
useEffectdepends on the entirecredentialsobject, which may trigger unnecessary re-renders if any property changes. Consider depending only oncredentials?.isLoadingto reduce re-render frequency.Apply this diff to optimize the dependency array:
// Report loaded state to parent useEffect(() => { if (onLoaded) { onLoaded(Boolean(credentials && credentials.isLoading === false)); } - }, [credentials, onLoaded]); + }, [credentials?.isLoading, onLoaded]);autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/RunAgentHint.tsx (1)
20-26: Consider using a button element for better accessibilityThe clickable area is implemented as a
<div>withonClick. For better accessibility, consider using a<button>element, which provides built-in keyboard navigation and screen reader support.- <div + <button + type="button" onClick={props.handleNewRun} className={cn( "mt-16 flex h-[68px] w-[330px] items-center justify-center rounded-xl border-2 border-violet-700 bg-neutral-50", "cursor-pointer transition-all duration-200 ease-in-out hover:bg-violet-50", )} > {/* SVG and content */} - </div> + </button>autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/AgentOnboardingCredentials/AgentOnboardingCredentials.tsx (2)
27-27: Duplicate required-check early-returns.You return
nulltwice for!required. Remove the second guard at Line 44 to avoid dead code.Also applies to: 44-44
33-36: Sanitization should exclude only null/undefined, not all falsy.Using
if (v)drops valid falsy values (e.g., empty string if allowed). Usev != null. This is also reflected in the diff above.autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/useOnboardingRunStep.tsx (1)
19-28: Avoid duplicating query data in local state; reset credentials when agent changes.
agentandstoreAgentmirror query results via effects, risking brief nulls/staleness and extra renders. Prefer deriving from queries directly.- Also, when
currentAgentVersionchanges, resetinputCredentials/credentialsValid/credentialsLoadedto avoid carrying credentials across agents.Minimal changes:
- const [agent, setAgent] = useState<GraphMeta | null>(null); - const [storeAgent, setStoreAgent] = useState<StoreAgentDetails | null>(null); + // Derive from queries to avoid duplication + const agent: GraphMeta | null = + graphMetaQuery.data?.status === 200 ? (graphMetaQuery.data.data as GraphMeta) : null; + const storeAgent: StoreAgentDetails | null = + storeAgentQuery.data?.status === 200 ? storeAgentQuery.data.data : null; @@ -useEffect(() => { - if (storeAgentQuery.data && storeAgentQuery.data.status === 200) { - setStoreAgent(storeAgentQuery.data.data); - } -}, [storeAgentQuery.data]); +// no-op: derived above @@ -useEffect(() => { - if ( - graphMetaQuery.data && - graphMetaQuery.data.status === 200 && - onboarding.state - ) { - const graphMeta = graphMetaQuery.data.data as GraphMeta; - setAgent(graphMeta); - const update = computeInitialAgentInputs( - graphMeta, - (onboarding.state.agentInput as unknown as InputValues) || null, - ); - onboarding.updateState({ agentInput: update }); - } -}, [graphMetaQuery.data]); +useEffect(() => { + if (agent && onboarding.state) { + const update = computeInitialAgentInputs( + agent, + (onboarding.state.agentInput as unknown as InputValues) || null, + ); + onboarding.updateState({ agentInput: update }); + } +}, [agent]); + +// Reset credentials on agent switch +useEffect(() => { + setInputCredentials({}); + setCredentialsValid(true); + setCredentialsLoaded(false); +}, [currentAgentVersion]);Please confirm
SelectedAgentCardcan handle a transientnullif you keep local state instead of deriving directly.Also applies to: 36-43, 48-53, 54-71
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/page.tsx (1)
121-125: Type safety: avoidas unknown as InputValues.Double casts hide mismatches between the schema and
InputValues. Prefer narrowing inuseOnboardingRunStepsoagentInputis already typed, or assert per-key where needed.I can adjust types in
useOnboardingRunStepsoagentInputis returned asInputValues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/4-agent/page.tsx(2 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/AgentOnboardingCredentials/AgentOnboardingCredentials.tsx(1 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/AgentOnboardingCredentials/helpers.ts(1 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/AgentOnboardingCredentials/types.ts(1 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/RunAgentHint.tsx(1 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/SelectedAgentCard.tsx(1 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/helpers.ts(3 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/page.tsx(4 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/useOnboardingRunStep.tsx(1 hunks)autogpt_platform/frontend/src/app/(no-navbar)/onboarding/components/StarRating.tsx(1 hunks)autogpt_platform/frontend/src/app/(platform)/library/agents/[id]/components/AgentRunsView/components/CredentialsInputs/CredentialsInputs.tsx(2 hunks)autogpt_platform/frontend/src/app/(platform)/login/page.tsx(2 hunks)autogpt_platform/frontend/src/app/(platform)/login/useLoginPage.ts(1 hunks)
⏰ 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). (3)
- GitHub Check: types
- GitHub Check: chromatic
- GitHub Check: Check PR Status
🔇 Additional comments (14)
autogpt_platform/frontend/src/app/(platform)/login/page.tsx (1)
17-17: LGTM! Correctly adapts to the new hook API.The changes properly consume the updated
useLoginPagehook by destructuringuserinstead ofisLoggedIn(line 17) and using it in the loading condition (line 33). The logic remains sound: when a user is present, the loading screen displays while theuseEffectin the hook redirects to the home page.Also applies to: 33-33
autogpt_platform/frontend/src/app/(platform)/login/useLoginPage.ts (1)
165-165: The review comment is incorrect—the code is already consistent.The
useLoginPagehook already returnsuser(notisLoggedIn), and the single consumer (login/page.tsx) correctly destructures and usesuser. Line 30 of the page component checksif (isUserLoading || user)directly without expectingisLoggedIn. No breaking change exists; the implementation and consumer are aligned.Likely an incorrect or invalid review comment.
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/4-agent/page.tsx (1)
15-15: LGTM: Import reorderingThe import statement reordering and blank line addition are stylistic changes with no functional impact.
Also applies to: 27-27
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/helpers.ts (2)
8-29: LGTM: Improved state preservationThe addition of the
existingInputsparameter allows preserving user input across re-renders, preventing data loss. The logic correctly prioritizes existing values before falling back to defaults.
31-57: LGTM: Clearer parameter namingThe parameter renames (
credentialsRequired→credentialsValid,credentialsSatisfied→credentialsLoaded) better express the intent: credentials must be both valid and fully loaded before allowing a run.autogpt_platform/frontend/src/app/(no-navbar)/onboarding/components/StarRating.tsx (1)
49-49: LGTM: Minor spacing adjustmentThe reduced top margin improves visual alignment of the numeric rating with the star icons.
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/AgentOnboardingCredentials/types.ts (1)
1-6: LGTM: Clean type definitionThe
AgentCredentialsFieldstype provides clear type safety for credential field schemas used throughout the onboarding flow.autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/SelectedAgentCard.tsx (2)
1-52: LGTM: Clean component structureThe component correctly handles the null case with a skeleton loader and uses proper formatting for numeric values with
toLocaleString.
19-23: SmartImage safely handles undefined src; the practical risk is low, but best-practice bounds checking would be prudentWhile
props.storeAgent.agent_image[0]accesses the array without bounds checking, SmartImage safely handles undefined by checking if src exists and only rendering the Image component ifsrcis truthy. Ifagent_imagewere an empty array,undefinedwould be passed to SmartImage, which would display a skeleton placeholder instead of crashing.Since
agent_imageis defined as a required field (string[]with no optional marker) inStoreAgentDetails, the backend data contract likely guarantees a non-empty array. However, as a defensive programming practice, consider adding an explicit bounds check:src={props.storeAgent.agent_image?.[0]}This ensures graceful handling if the backend contract assumption ever changes.
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/RunAgentHint.tsx (1)
1-45: LGTM: Clean presentational componentThe component is well-structured with clear visual hierarchy and appropriate styling. The inline SVG is simple and self-contained.
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/components/AgentOnboardingCredentials/helpers.ts (2)
5-19: LGTM: Robust credential field extractionThe function correctly handles null agents and missing schema properties, with appropriate defensive checks before the type assertion.
21-27: LGTM: Clear credential validation logicThe validation correctly verifies that all required credential fields have truthy values, with safe handling of null/undefined inputs.
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/useOnboardingRunStep.tsx (1)
111-155: Run flow looks solid.Happy path, error handling, and onboarding updates are correct. No blockers here.
autogpt_platform/frontend/src/app/(no-navbar)/onboarding/5-run/page.tsx (1)
60-61: Null handling forSelectedAgentCard.On the first “ready” render,
storeAgentmay still benullif you keep local state in the hook. EnsureSelectedAgentCardacceptsnullor derivestoreAgentdirectly from the query in the hook (see earlier comment).
kcze
changed the title
Run button
2bf53c5 to
db7363a
Compare
github-project-automation
Bot
moved this from 🆕 Needs initial review
to ✅ Done
in AutoGPT development kanban
2 participants
Changes 🏗️
The onboarding
Runbutton is disabled sometimes when an agent requiring credentials is selected. We think this can be because the credentials load async by a sub-component (<CredentialsInputs />), and there wasn't a way for the parent component to know whether they loaded or not.Checklist 📋
For code changes:
Summary by CodeRabbit
New Features
Improvements