fix: link evaluator button to playground in trace drawer#4565
Open
GanJiaKouN16 wants to merge 5 commits into
Open
fix: link evaluator button to playground in trace drawer#4565GanJiaKouN16 wants to merge 5 commits into
GanJiaKouN16 wants to merge 5 commits into
Conversation
Wire the existing GenerateResetLinkModal and PasswordResetLinkModal into the Actions dropdown in the workspace members table. - Add 'Reset password' menu item for workspace members (not self) - Add resetPassword API function in profile service - Show confirmation dialog before generating the reset link - Display the generated password reset link with copy functionality Closes Agenta-AI#2572
Several tables with row-level click navigation were missing the shouldIgnoreRowClick guard, causing clicks on interactive elements (checkboxes, dropdowns, buttons) to accidentally trigger row navigation. Changes: - Consolidate shouldIgnoreRowClick with broader selector list (merges EvaluationRunsTablePOC's extra selectors: [role='button'], [role='menuitem'], [role='checkbox'], .ant-btn, etc.) - Export INTERACTIVE_ROW_SELECTORS constant for reuse - Add guard to ObservabilityTable (traces) - Add guard to SessionsTable - Add guard to PromptsPage - Add guard to TestcasesTableShell - Add guard to EntityTable - Replace partial data-ivt-stop-row-click check in ScenarioListView with full shouldIgnoreRowClick - Update useEntityTableState to use consolidated selectors - Remove duplicate shouldIgnoreRowClick from navigationActions.ts - Update EvaluationRunsTablePOC to import from shared utility Closes Agenta-AI#3254
The evaluation table was showing a generic 'too many requests' message instead of the actual provider error because: 1. executeViaFetch never checked for body-level errors on HTTP 200. The Python SDK can return HTTP 200 with a non-200 status.code embedded in the response body (WorkflowBatchResponse.status.code). This path was silently treated as success. 2. Error stacktrace/type/code were not propagated through the pipeline. Even when the HTTP error path was taken, only the message was extracted — the SDK's status.type, status.code, and status.stacktrace were dropped. Changes: - executeViaFetch: detect body-level errors on HTTP 200 by checking responseData.status.code !== 200 and return an error result - executeViaFetch: extract stacktrace (coercing string[] to string), type, and code from both HTTP-error and body-error paths - Add stacktrace and type to ExecutionResult, RunResult, and ExecuteWorkflowRevisionResult error shapes - runInvocationAction: pass stacktrace and type through to upsertStepResultWithInvocation - upsertStepResultWithInvocation: accept type field in error param No UI changes needed — InvocationCell already renders stepError.message and stepError.stacktrace when present; extractStepError already reads error.code, error.type, error.stacktrace from persisted step data. Closes Agenta-AI#3324
…iddleware
The vault middleware built env var names using f'{provider.upper()}_API_KEY'
which produces TOGETHER_AI_API_KEY for the 'together_ai' provider kind.
The actual env var is TOGETHERAI_API_KEY (no underscore), matching the
frontend (llmProviders.ts, transforms.ts), backend (env.py), and the
Daytona sandbox runner (daytona.py).
Add an explicit _PROVIDER_ENV_VAR_MAP dict (mirroring the Daytona runner
pattern) that maps each provider kind to its correct env var name, with
fallback to the original f-string pattern for any future providers.
Closes Agenta-AI#3659
… drawer The 'Open evaluator registry' button in the Trace Drawer's EvaluatorDetailsPopover navigated human evaluators to the registry page (/evaluators?tab=human&openEvaluator=...) instead of the evaluator playground. Automatic evaluators already linked correctly. Unify both evaluator types to navigate to the playground (/evaluators/playground?revisions=...), consistent with how other parts of the codebase link to evaluators (EvaluatorSection, ConfigurationView). Update button text to 'Open evaluator playground'. Closes Agenta-AI#4535
|
Someone is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
size:L
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (16)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR enriches error shapes with optional type/stacktrace fields across types and runtime, centralizes row-click suppression selectors and logic, fixes evaluator playground routing and label, adds workspace password reset UI+API, and adds a provider-kind → env-var mapping in the Python vault middleware. ChangesError metadata enrichment across execution layers
Row click suppression refactoring and consolidation
Evaluator navigation consolidation and UI fix
Workspace member password reset feature
Python SDK LLM provider environment variable mapping
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
web/packages/agenta-entities/src/runnable/types.ts (1)
207-208: ⚡ Quick winAlign stage/row error contracts with the enriched execution error shape.
ExecutionResult.errornow includestypeandstacktrace, butStageExecutionResult.errorandRowExecutionResult.errorstill omit them. This creates a type-boundary mismatch for chain/row consumers.Suggested contract alignment
export interface StageExecutionResult { @@ error?: { message: string code?: string + type?: string + stacktrace?: string } @@ } export interface RowExecutionResult { @@ error?: { message: string code?: string + type?: string + stacktrace?: string } @@ }web/oss/src/components/InfiniteVirtualTable/hooks/useTableManager.tsx (1)
33-50: ⚡ Quick winAvoid maintaining a second selector registry here.
This selector list duplicates
web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsx, so future drift can reintroduce inconsistent row-click behavior across tables. Prefer a single shared source (or re-export) forINTERACTIVE_ROW_SELECTORS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c72e940-75ba-4967-89d8-53fa47845fb4
📒 Files selected for processing (22)
sdks/python/agenta/sdk/middlewares/running/vault.pyweb/oss/src/components/EvalRunDetails/atoms/runInvocationAction.tsweb/oss/src/components/EvaluationRunsTablePOC/actions/navigationActions.tsweb/oss/src/components/EvaluationRunsTablePOC/components/EvaluationRunsTable/index.tsxweb/oss/src/components/InfiniteVirtualTable/hooks/useTableManager.tsxweb/oss/src/components/SharedDrawers/TraceDrawer/components/EvaluatorDetailsPopover.tsxweb/oss/src/components/SharedDrawers/TraceDrawer/hooks/useEvaluatorNavigation.tsweb/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsxweb/oss/src/components/pages/observability/components/ObservabilityTable/index.tsxweb/oss/src/components/pages/observability/components/SessionsTable/index.tsxweb/oss/src/components/pages/prompts/PromptsPage.tsxweb/oss/src/components/pages/settings/WorkspaceManage/cellRenderers.tsxweb/oss/src/services/evaluations/invocations/api.tsweb/oss/src/services/profile/index.tsweb/packages/agenta-annotation-ui/src/components/AnnotationSession/ScenarioListView.tsxweb/packages/agenta-entities/src/runnable/types.tsweb/packages/agenta-entity-ui/src/shared/EntityTable.tsxweb/packages/agenta-playground/src/executeWorkflowRevision.tsweb/packages/agenta-playground/src/state/execution/executionRunner.tsweb/packages/agenta-playground/src/state/execution/types.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useEntityTableState.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsx
💤 Files with no reviewable changes (1)
- web/oss/src/components/EvaluationRunsTablePOC/actions/navigationActions.ts
Comment on lines
+207
to
+213
| <GenerateResetLinkModal | ||
| open={generateResetLinkOpen} | ||
| username={user.username} | ||
| onCancel={() => setGenerateResetLinkOpen(false)} | ||
| onOk={handleResetPassword} | ||
| confirmLoading={resetLoading} | ||
| /> |
There was a problem hiding this comment.
Generate-link modal closes before async completion
Line 211 wires async generation correctly, but the modal currently calls onOk and then immediately onCancel in GenerateResetLinkModal (web/oss/src/components/pages/settings/WorkspaceManage/Modals/GenerateResetLinkModal.tsx:7-12). That makes confirmLoading at Line 212 ineffective and forces users to reopen the modal after failures.
Suggested fix (in GenerateResetLinkModal.tsx)
-const onGenerateLink = () => {
- props.onOk?.({} as any)
- props.onCancel?.({} as any)
-}
+const onGenerateLink = async () => {
+ try {
+ await props.onOk?.({} as any)
+ props.onCancel?.({} as any)
+ } catch {
+ // keep modal open so user can retry
+ }
+}
Comment on lines
+64
to
+70
| export const resetPassword = async (userId: string): Promise<string> => { | ||
| const base = getBaseUrl() | ||
| const url = new URL("api/profile/reset-password", base) | ||
| url.searchParams.set("user_id", userId) | ||
| const data = await fetchJson<string>(url, { | ||
| method: "POST", | ||
| }) |
There was a problem hiding this comment.
Route this new API call through the Fern SDK client
Line 64 introduces a new frontend API call, but it bypasses the Fern-generated client and manual URL/query handling is used instead. Please switch this to getAgentaSdkClient({host: getAgentaApiUrl()}), pass user_id via queryParams, and validate the response at the boundary with safeParseWithLogging.
Suggested direction
-export const resetPassword = async (userId: string): Promise<string> => {
- const base = getBaseUrl()
- const url = new URL("api/profile/reset-password", base)
- url.searchParams.set("user_id", userId)
- const data = await fetchJson<string>(url, {
- method: "POST",
- })
- return data
-}
+export const resetPassword = async (userId: string): Promise<string> => {
+ const client = getAgentaSdkClient({host: getAgentaApiUrl()})
+ const raw = await client.users.resetUserPassword(
+ {user_id: userId},
+ {queryParams: {user_id: userId}},
+ )
+ const parsed = safeParseWithLogging(/* zod string schema */, raw, "resetPassword")
+ if (!parsed.success) throw new Error("Invalid reset-password response")
+ return parsed.data
+}As per coding guidelines: “All new frontend API code must go through the Fern-generated client, not raw axios”, “Use getAgentaSdkClient({host: getAgentaApiUrl()})”, “Pass query params via {queryParams: {...}}”, and “Keep zod validation at the boundary of API calls using safeParseWithLogging”.
Source: Coding guidelines
Comment on lines
+64
to
+72
| export const resetPassword = async (userId: string): Promise<string> => { | ||
| const base = getBaseUrl() | ||
| const url = new URL("api/profile/reset-password", base) | ||
| url.searchParams.set("user_id", userId) | ||
| const data = await fetchJson<string>(url, { | ||
| method: "POST", | ||
| }) | ||
| return data | ||
| } |
There was a problem hiding this comment.
Promise<string> overstates the reset-password response contract
Line 64 assumes a guaranteed string return, but upstream service behavior can return no link when Sendgrid is configured (api/oss/src/services/user_service.py:130-170). This makes the current type/flow unsafe and can surface an empty/invalid link downstream.
Please align contract explicitly: either make backend always return a link string, or change frontend type/UX to handle “email sent, no link returned” as a separate success path.
Comment on lines
+718
to
+731
| if (bodyStatus && typeof bodyStatus === "object" && bodyStatus.code && bodyStatus.code !== 200) { | ||
| const traceId = extractTraceIdFromPayload(responseData) | ||
| const spanId = extractSpanIdFromPayload(responseData) | ||
| const st = bodyStatus.stacktrace | ||
| return { | ||
| executionId, | ||
| status: "error", | ||
| startedAt, | ||
| completedAt: new Date().toISOString(), | ||
| error: { | ||
| message: bodyStatus.message || "Invocation failed", | ||
| ...(bodyStatus.code ? {code: bodyStatus.code.toString()} : {}), | ||
| ...(bodyStatus.type ? {type: bodyStatus.type} : {}), | ||
| ...(st ? {stacktrace: Array.isArray(st) ? st.join("\n") : st} : {}), |
There was a problem hiding this comment.
Normalize status.code before comparing to 200.
The current check treats "200" as failure because it compares directly against numeric 200. That can incorrectly route successful responses into the error path.
Suggested fix
- const bodyStatus = responseData?.status
- if (bodyStatus && typeof bodyStatus === "object" && bodyStatus.code && bodyStatus.code !== 200) {
+ const bodyStatus = responseData?.status as
+ | {code?: number | string; message?: string; type?: string; stacktrace?: string | string[]}
+ | undefined
+ const statusCode =
+ typeof bodyStatus?.code === "string" ? Number(bodyStatus.code) : bodyStatus?.code
+ if (statusCode !== undefined && Number.isFinite(statusCode) && statusCode !== 200) {
@@
- ...(bodyStatus.code ? {code: bodyStatus.code.toString()} : {}),
+ ...(statusCode !== undefined ? {code: String(statusCode)} : {}),
GanJiaKouN16
force-pushed
the
fix/evaluator-registry-button-link
branch
from
691f759 to
01a6e6d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4535 — the "Open evaluator registry" button in the Trace Drawer linked to the wrong destination for human evaluators.
Root cause
useEvaluatorNavigation.tshad separate URL paths for human vs automatic evaluators:/evaluators/playground?revisions=...✅ (correct)/evaluators?tab=human&openEvaluator=...❌ (registry page, not playground)The button text also always said "Open evaluator registry" regardless of where it actually navigated.
Fix
/evaluators/playground?revisions=..., consistent with how the rest of the codebase links to evaluators (EvaluatorSection.tsx,ConfigurationView.tsx)OnlineEvaluationDrawer.tsxis NOT changed — its "Open evaluator registry" link (line 651) correctly points to the registry page for adding new evaluatorsChanges
useEvaluatorNavigation.ts/evaluators/playground?revisions=...EvaluatorDetailsPopover.tsxCloses #4535