fix: surface actual provider error messages in evaluation table#4563
Open
GanJiaKouN16 wants to merge 3 commits into
Open
fix: surface actual provider error messages in evaluation table#4563GanJiaKouN16 wants to merge 3 commits into
GanJiaKouN16 wants to merge 3 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
|
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
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughEnrich error payloads with optional ChangesError Enrichment & Richer Error Details
Row-Click Handler Consolidation
Reset-Password Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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: 3
🧹 Nitpick comments (3)
web/oss/src/services/profile/index.ts (1)
64-72: ⚡ Quick winConsider using the Fern-generated client for this new endpoint.
The coding guidelines specify that new frontend API code should go through the Fern-generated client rather than raw fetch utilities. A generated
__resetUserPasswordmethod already exists in the API client (seeweb/packages/agenta-api-client/src/generated/api/resources/users/client/Client.ts).While this implementation follows the existing pattern in this file for consistency, migrating to the Fern client would align with guidelines and provide type-safe request/response handling.
♻️ Example using Fern client
import {getAgentaSdkClient, getAgentaApiUrl} from "`@/oss/lib/api/client`" export const resetPassword = async (userId: string): Promise<string> => { const client = getAgentaSdkClient({host: getAgentaApiUrl()}) const response = await client.users.resetUserPassword({user_id: userId}) return response as string }Source: Coding guidelines
web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsx (1)
232-250: ⚡ Quick winConsider memoizing the joined selector string for better performance.
The current implementation splits
INTERACTIVE_ROW_SELECTORSback into an array (line 190), merges with custom selectors (lines 232-235), then callstarget.closest()multiple times (lines 248-250). Each row click results in O(n × m) closest calls where n = selector count and m = DOM depth.You could optimize by memoizing the joined string and calling
closest()once:const interactiveSelectorString = useMemo( () => interactiveSelectors.join(", "), [interactiveSelectors], ) // Then in buildRowHandlers: const isInteractive = Boolean(target.closest(interactiveSelectorString))This reduces complexity to O(n + m): one join operation (memoized) plus one closest call per click.
web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useEntityTableState.ts (1)
190-190: ⚡ Quick winAvoid parsing selector lists with a literal
", "delimiter.Line 190 is brittle to formatting changes in
INTERACTIVE_ROW_SELECTORSand can silently break click-ignore behavior. Prefer whitespace-tolerant parsing (or sharing an exported array constant fromuseTableManager).Suggested fix
-const DEFAULT_INTERACTIVE_SELECTORS = INTERACTIVE_ROW_SELECTORS.split(", ") +const DEFAULT_INTERACTIVE_SELECTORS = INTERACTIVE_ROW_SELECTORS + .split(/\s*,\s*/) + .filter(Boolean)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 91522e23-c6e2-49ca-808e-8a894dc27908
📒 Files selected for processing (19)
web/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/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
+211
to
+215
| error: { | ||
| message: errorMessage, | ||
| ...(result.error?.stacktrace ? {stacktrace: result.error.stacktrace} : {}), | ||
| ...(result.error?.type ? {type: result.error.type} : {}), | ||
| }, |
There was a problem hiding this comment.
Forward result.error.code when persisting invocation failures.
The failure payload currently strips code, so UI/error tooling cannot show provider/HTTP code consistently.
Proposed fix
error: {
message: errorMessage,
+ ...(result.error?.code ? {code: result.error.code} : {}),
...(result.error?.stacktrace ? {stacktrace: result.error.stacktrace} : {}),
...(result.error?.type ? {type: result.error.type} : {}),
},📝 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.
Suggested change
| error: { | |
| message: errorMessage, | |
| ...(result.error?.stacktrace ? {stacktrace: result.error.stacktrace} : {}), | |
| ...(result.error?.type ? {type: result.error.type} : {}), | |
| }, | |
| error: { | |
| message: errorMessage, | |
| ...(result.error?.code ? {code: result.error.code} : {}), | |
| ...(result.error?.stacktrace ? {stacktrace: result.error.stacktrace} : {}), | |
| ...(result.error?.type ? {type: result.error.type} : {}), | |
| }, |
| references?: InvocationReferences | ||
| outputs?: unknown | ||
| error?: {message: string; stacktrace?: string} | ||
| error?: {message: string; stacktrace?: string; type?: string} |
There was a problem hiding this comment.
Include error.code in the persistence contract.
This boundary type currently blocks status-code propagation, so HTTP/provider code can be lost before UI rendering.
Proposed fix
- error?: {message: string; stacktrace?: string; type?: string}
+ error?: {message: string; code?: string; stacktrace?: string; type?: string}📝 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.
Suggested change
| error?: {message: string; stacktrace?: string; type?: string} | |
| error?: {message: string; code?: string; stacktrace?: string; type?: string} |
Comment on lines
+718
to
+719
| if (bodyStatus && typeof bodyStatus === "object" && bodyStatus.code && bodyStatus.code !== 200) { | ||
| const traceId = extractTraceIdFromPayload(responseData) |
There was a problem hiding this comment.
Normalize status.code before comparing to 200.
If the backend sends "200" (string), this branch marks a success payload as an error.
Proposed fix
- const bodyStatus = responseData?.status
- if (bodyStatus && typeof bodyStatus === "object" && bodyStatus.code && bodyStatus.code !== 200) {
+ const bodyStatus = responseData?.status
+ const bodyStatusCode =
+ bodyStatus && typeof bodyStatus === "object"
+ ? Number((bodyStatus as Record<string, unknown>).code)
+ : NaN
+ if (bodyStatus && typeof bodyStatus === "object" && Number.isFinite(bodyStatusCode) && bodyStatusCode !== 200) {📝 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.
Suggested change
| if (bodyStatus && typeof bodyStatus === "object" && bodyStatus.code && bodyStatus.code !== 200) { | |
| const traceId = extractTraceIdFromPayload(responseData) | |
| const bodyStatus = responseData?.status | |
| const bodyStatusCode = | |
| bodyStatus && typeof bodyStatus === "object" | |
| ? Number((bodyStatus as Record<string, unknown>).code) | |
| : NaN | |
| if (bodyStatus && typeof bodyStatus === "object" && Number.isFinite(bodyStatusCode) && bodyStatusCode !== 200) { | |
| const traceId = extractTraceIdFromPayload(responseData) |
GanJiaKouN16
force-pushed
the
fix/evaluation-error-display
branch
from
971e50e to
585c6ad
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useEntityTableState.ts (1)
187-235: ⚡ Quick winUse
closest()with the selector string directly instead of splitting and iterating.The current implementation splits
INTERACTIVE_ROW_SELECTORSback into an array and iterates with.some(), callingclosest()for each selector. However,Element.closest()natively accepts comma-separated selector strings, so you can pass the string directly and avoid both the split and the iteration.Issues with the current approach:
- Less efficient: multiple
closest()calls instead of one- Less idiomatic:
closest()is designed to work with selector strings- Fragile: assumes
", "join format; future selectors containing commas would break the split- Unnecessary complexity: splitting a string that was just joined
Suggested refactor:
♻️ Cleaner implementation using string concatenation
-/** - * Default selectors for interactive elements that should not trigger row click. - * Uses the consolidated selector string from useTableManager for consistency. - */ -const DEFAULT_INTERACTIVE_SELECTORS = INTERACTIVE_ROW_SELECTORS.split(", ") - // ============================================================================ // HOOK // ============================================================================ @@ -230,10 +225,12 @@ // Combine selectors for interactive elements - const interactiveSelectors = useMemo( - () => [...DEFAULT_INTERACTIVE_SELECTORS, ...excludeClickSelectors], + const combinedSelectors = useMemo( + () => + excludeClickSelectors.length > 0 + ? `${INTERACTIVE_ROW_SELECTORS}, ${excludeClickSelectors.join(", ")}` + : INTERACTIVE_ROW_SELECTORS, [excludeClickSelectors], ) // Build row handlers for click events @@ -245,10 +242,7 @@ // Check if clicking on interactive elements const target = event.target as HTMLElement - const isInteractive = interactiveSelectors.some((selector) => - target.closest(selector), - ) + const isInteractive = Boolean(target.closest(combinedSelectors)) if (isInteractive) return onRowClick(record) @@ -259,7 +253,7 @@ minHeight: rowHeight, } as CSSProperties, } }, - [onRowClick, rowHeight, interactiveSelectors], + [onRowClick, rowHeight, combinedSelectors], )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 370a82f9-d793-4a5d-bb7e-1853b8563062
📒 Files selected for processing (19)
web/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/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
🚧 Files skipped from review as they are similar to previous changes (15)
- web/oss/src/services/profile/index.ts
- web/packages/agenta-playground/src/executeWorkflowRevision.ts
- web/oss/src/services/evaluations/invocations/api.ts
- web/oss/src/components/pages/observability/components/SessionsTable/index.tsx
- web/packages/agenta-annotation-ui/src/components/AnnotationSession/ScenarioListView.tsx
- web/packages/agenta-entity-ui/src/shared/EntityTable.tsx
- web/packages/agenta-entities/src/runnable/types.ts
- web/oss/src/components/EvaluationRunsTablePOC/components/EvaluationRunsTable/index.tsx
- web/oss/src/components/pages/prompts/PromptsPage.tsx
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsx
- web/oss/src/components/pages/settings/WorkspaceManage/cellRenderers.tsx
- web/packages/agenta-playground/src/state/execution/executionRunner.ts
- web/packages/agenta-playground/src/state/execution/types.ts
- web/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsx
- web/oss/src/components/InfiniteVirtualTable/hooks/useTableManager.tsx
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 #3324 — the evaluation table was showing a generic "too many requests" or "Request failed" message instead of the actual provider error (e.g. "OpenAI rate limit exceeded").
Root cause
Two bugs in the execution data pipeline:
executeViaFetchignored body-level errors on HTTP 200. The Python SDK returnsWorkflowBatchResponsewithstatus.code=424(and the real error instatus.message) for downstream API errors (429→424). When the HTTP status matchesstatus.code, Path A catches it correctly. But when the SDK returns HTTP 200 with a non-200status.codeembedded in the response body, the code unconditionally returnedstatus: "success"— the error was silently dropped.Error metadata was not propagated. Even when the HTTP error path worked, only
status.messagewas extracted. The SDK'sstatus.type,status.code, andstatus.stacktracewere all dropped before reachingInvocationCell.Changes
agenta-playground/.../executionRunner.tsresponseData.status.code !== 200). Extractstacktrace,type,codefrom both HTTP-error and body-error paths. Coercestring[]stacktrace tostring.agenta-entities/.../runnable/types.tstype?andstacktrace?toExecutionResult.erroragenta-playground/.../types.tstype?andstacktrace?toRunResult.erroragenta-playground/.../executeWorkflowRevision.tstype?andstacktrace?toExecuteWorkflowRevisionResult.errorEvalRunDetails/.../runInvocationAction.tsstacktraceandtypethrough toupsertStepResultWithInvocationevaluations/invocations/api.tstype?in error paramNo UI changes needed
InvocationCellalready rendersstepError.messageandstepError.stacktracewhen present.extractStepErroralready readserror.code,error.type,error.stacktracefrom persisted step data. The fix is purely in the data pipeline — the fields were being dropped before reaching storage.Testing
Closes #3324