test: add e2e tests for annotation queue flows#4568
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
Replace the SendGrid-only email backend with SMTP support, keeping SendGrid as a legacy fallback for existing deployments. Changes: - email_service.py: use smtplib for SMTP (priority), SendGrid as fallback, no-op when neither is configured - env.py: add SmtpConfig (SMTP_HOST, SMTP_PORT, SMTP_USERNAME, SMTP_PASSWORD, SMTP_FROM_ADDRESS, SMTP_USE_TLS), keep SendgridConfig for backwards compatibility; update AuthFacade.email_method to check both - OSS/EE organization_service.py: use env.smtp.enabled || env.sendgrid .enabled; use configured from_address instead of hardcoded email - user_service.py: same email-enabled check update - db_manager_ee.py: remove dead sendgrid import and unused sg client - pyproject.toml: remove sendgrid dependency (imported lazily only when SENDGRID_API_KEY is set) - env example files: add SMTP vars, mark SendGrid as legacy - docs: add SMTP config table, mark SendGrid as legacy Closes Agenta-AI#4536
The breadcrumb always showed 'Auto Evals' for SDK evaluations because: 1. test.tsx normalized type='custom' to 'auto' before passing to EvalRunPreviewPage, losing the SDK type information 2. Page.tsx typeMap had no 'custom' entry 3. buildBreadcrumbs.ts hardcoded 'auto evaluation' as fallback label Fix: - Remove the custom→auto normalization in test.tsx - Add 'custom' → 'SDK Evals' entry to Page.tsx typeMap (matches the tab label in EvaluationsView.tsx) - Change buildBreadcrumbs.ts fallback from 'auto evaluation' to 'Evaluations' Closes Agenta-AI#4549
Add Playwright acceptance tests for the annotation queue feature, following the existing human-annotation test patterns. Tests cover (WEB-ACC-ANNOTATION-001 through 006): 1. Navigate to annotation queues page and verify empty state or list 2. Create a new annotation queue with testcases kind 3. Create a new annotation queue with traces kind 4. Open a queue detail page by clicking a queue row 5. Search for annotation queues by name 6. Delete an annotation queue via the actions menu Structure mirrors human-annotation/: - annotation-queue.spec.ts — entry point - index.ts — test definitions with tags - tests.ts — fixtures (navigateToAnnotations, createAnnotationQueue) - assets/types.ts — type definitions Closes Agenta-AI#4528
dosubot
Bot
added
the
size:L
|
Someone is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces SMTP email backend configuration alongside SendGrid, enriches execution error objects with type and stacktrace details, centralizes interactive row-click suppression logic across table components, adds custom evaluation type support, implements user password reset in workspace settings, and adds E2E annotation queue tests with supporting minor updates to provider mapping and evaluator navigation. ChangesSMTP Email Backend Configuration
Error Details Enrichment
Interactive Row Click Handler Centralization
Custom Evaluation Type Support
User Password Reset Feature
Annotation Queue End-to-End Tests
Supporting Updates
🎯 4 (Complex) | ⏱️ ~60 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/packages/agenta-entities/src/runnable/types.ts (1)
228-231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
StageExecutionResult.errorwithExecutionResult.errorto include the new fields.
ExecutionResult.errornow includes optionaltypeandstacktracefields (lines 207-208), butStageExecutionResult.errorwas not updated to match. Since stage results represent individual nodes in chain execution and are stored inchainResults(line 272), this inconsistency could cause type mismatches or lost error details when stage results are processed.🔧 Suggested fix
error?: { message: string code?: string + type?: string + stacktrace?: string }api/ee/src/services/organization_service.py (1)
107-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider hardcoded default sender address.
Lines 121-122 provide a hardcoded default sender
"account@hello.agenta.ai"when neither SMTP nor SendGrid from_address is configured. This creates several issues:
- SPF/DKIM failures: Emails sent from this address without proper domain configuration will likely fail authentication checks and be marked as spam or rejected.
- Unmonitored inbox: Replies will go to an address that may not be monitored.
- Inconsistency with OSS: The OSS version (oss/src/services/organization_service.py lines 172-174) raises a
ValueErrorinstead, which prevents misconfiguration.Consider aligning with the OSS behavior by raising an error when no sender is configured, forcing explicit configuration.
🔧 Proposed fix to align with OSS behavior
- from_address = env.smtp.from_address or env.sendgrid.from_address or "account@hello.agenta.ai" - + from_address = env.smtp.from_address or env.sendgrid.from_address + if not from_address: + raise ValueError("Email requires a sender email address (SMTP_FROM_ADDRESS or SENDGRID_FROM_ADDRESS)") + await email_service.send_email(
🧹 Nitpick comments (8)
web/oss/tests/playwright/acceptance/annotation-queue/index.ts (3)
45-90: ⚡ Quick winConsider adding cleanup for created test queues.
Tests 002 and 003 create annotation queues but don't delete them afterward. Over many test runs, this could accumulate test data and potentially affect test reliability or list-rendering performance. Test 006 demonstrates the deletion flow, so consider adding similar cleanup to these tests (e.g., in a try-finally block or afterEach hook).
105-126: ⚡ Quick winConsider adding cleanup for the created test queue.
Test 004 creates a queue for testing detail-page navigation but doesn't delete it. While Playwright test isolation might handle this, explicit cleanup would prevent test data accumulation and make the test more self-contained.
141-171: ⚡ Quick winConsider adding cleanup for the created test queue.
Test 005 creates a queue for testing search functionality but doesn't delete it afterward. Adding cleanup (similar to test 006's delete flow) would prevent test data pollution.
web/packages/agenta-annotation-ui/src/components/AnnotationSession/ScenarioListView.tsx (1)
1677-1678: ⚡ Quick winRemove underscore prefix from used parameter.
The parameter
_eventis prefixed with an underscore, which conventionally indicates an intentionally unused parameter. However, the parameter is used on line 1678 in theshouldIgnoreRowClickcall. Remove the underscore prefix to follow naming conventions.♻️ Rename parameter
- const handleRowClick = useCallback((_event: React.MouseEvent, record: ScenarioTableRow) => { - if (shouldIgnoreRowClick(_event)) return + const handleRowClick = useCallback((event: React.MouseEvent, record: ScenarioTableRow) => { + if (shouldIgnoreRowClick(event)) return setDrawerScenarioId(record.scenarioId) }, [])api/oss/src/utils/env.py (1)
942-942: ⚡ Quick winAlign use_tls truthiness check with global _TRUTHY constant.
The file defines a global
_TRUTHYset (line 10) used consistently across all other boolean config fields, butuse_tlsuses a hardcoded tuple("true", "1", "yes")instead. This creates inconsistency and means inputs like"t","y","on","enable", or"enabled"won't be recognized as truthy here.♻️ Proposed fix for consistency
- use_tls: bool = os.getenv("SMTP_USE_TLS", "true").lower() in ("true", "1", "yes") + use_tls: bool = os.getenv("SMTP_USE_TLS", "true").lower() in _TRUTHYapi/oss/src/services/email_service.py (2)
47-47: ⚡ Quick winUse keyword-only parameters in function signature.
The function signature should include
*to enforce keyword-only parameters for better call-site clarity and to prevent positional argument mistakes. As per coding guidelines, Python API code should prefer keyword-only parameters.♻️ Proposed refactor
-def _send_via_smtp(to_email: str, subject: str, html_content: str, from_email: str) -> None: +def _send_via_smtp(*, to_email: str, subject: str, html_content: str, from_email: str) -> None: """Send email using SMTP."""Source: Coding guidelines
74-74: ⚡ Quick winUse keyword-only parameters in function signature.
The function signature should include
*to enforce keyword-only parameters. As per coding guidelines, Python API code should prefer keyword-only parameters.♻️ Proposed refactor
-def _send_via_sendgrid(to_email: str, subject: str, html_content: str, from_email: str) -> None: +def _send_via_sendgrid(*, to_email: str, subject: str, html_content: str, from_email: str) -> None: """Send email using SendGrid (legacy fallback)."""Source: Coding guidelines
api/oss/src/services/organization_service.py (1)
172-174: 💤 Low valueConsider making the error message more specific.
The error message correctly prevents misconfiguration by raising a ValueError, but could be more helpful by mentioning the specific environment variables needed.
♻️ Optional improvement
from_address = env.smtp.from_address or env.sendgrid.from_address if not from_address: - raise ValueError("Email requires a sender email address to work.") + raise ValueError("Email requires a sender email address. Set SMTP_FROM_ADDRESS or SENDGRID_FROM_ADDRESS.")
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 899f18e1-9768-4f4a-8454-14e331ea1de3
📒 Files selected for processing (41)
api/ee/src/services/db_manager_ee.pyapi/ee/src/services/organization_service.pyapi/oss/src/services/email_service.pyapi/oss/src/services/organization_service.pyapi/oss/src/services/user_service.pyapi/oss/src/utils/env.pyapi/pyproject.tomldocs/docs/self-host/02-configuration.mdxhosting/docker-compose/ee/env.ee.dev.examplehosting/docker-compose/ee/env.ee.gh.examplehosting/docker-compose/oss/env.oss.dev.examplehosting/docker-compose/oss/env.oss.gh.examplesdks/python/agenta/sdk/middlewares/running/vault.pyweb/oss/src/components/EvalRunDetails/atoms/runInvocationAction.tsweb/oss/src/components/EvalRunDetails/components/Page.tsxweb/oss/src/components/EvalRunDetails/test.tsxweb/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/lib/helpers/buildBreadcrumbs.tsweb/oss/src/services/evaluations/invocations/api.tsweb/oss/src/services/profile/index.tsweb/oss/tests/playwright/acceptance/annotation-queue/annotation-queue.spec.tsweb/oss/tests/playwright/acceptance/annotation-queue/assets/types.tsweb/oss/tests/playwright/acceptance/annotation-queue/index.tsweb/oss/tests/playwright/acceptance/annotation-queue/tests.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 (3)
- api/ee/src/services/db_manager_ee.py
- api/pyproject.toml
- web/oss/src/components/EvaluationRunsTablePOC/actions/navigationActions.ts
| from_address = env.smtp.from_address or env.sendgrid.from_address or "account@hello.agenta.ai" | ||
|
|
There was a problem hiding this comment.
Reconsider hardcoded default sender address.
Lines 157-158 use the same problematic hardcoded default sender "account@hello.agenta.ai". This has the same issues as in send_invitation_email: SPF/DKIM failures, unmonitored replies, and inconsistency with the OSS version.
Consider raising an error when no sender is configured.
🔧 Proposed fix
- from_address = env.smtp.from_address or env.sendgrid.from_address or "account@hello.agenta.ai"
-
+ from_address = env.smtp.from_address or env.sendgrid.from_address
+ if not from_address:
+ raise ValueError("Email requires a sender email address (SMTP_FROM_ADDRESS or SENDGRID_FROM_ADDRESS)")
+
workspace_admins = await db_manager_ee.get_workspace_administrators(workspace)| def _send_via_smtp(to_email: str, subject: str, html_content: str, from_email: str) -> None: | ||
| """Send email using SMTP.""" | ||
| msg = MIMEMultipart("alternative") | ||
| msg["Subject"] = subject | ||
| msg["From"] = from_email | ||
| msg["To"] = to_email | ||
| msg.attach(MIMEText(html_content, "html")) | ||
|
|
||
| smtp_host = env.smtp.host | ||
| smtp_port = env.smtp.port | ||
|
|
||
| if env.smtp.use_tls: | ||
| server = smtplib.SMTP(smtp_host, smtp_port) | ||
| server.ehlo() | ||
| server.starttls() | ||
| server.ehlo() | ||
| else: | ||
| server = smtplib.SMTP(smtp_host, smtp_port) | ||
|
|
||
| try: | ||
| if env.smtp.username and env.smtp.password: | ||
| server.login(env.smtp.username, env.smtp.password) | ||
| server.sendmail(from_email, [to_email], msg.as_string()) | ||
| finally: | ||
| server.quit() | ||
|
|
There was a problem hiding this comment.
Add timeout to SMTP connection to prevent hangs.
The SMTP connection lacks a timeout parameter. If the SMTP server is unresponsive, the application will hang indefinitely on the request thread, degrading availability.
🔧 Proposed fix to add timeout
- if env.smtp.use_tls:
- server = smtplib.SMTP(smtp_host, smtp_port)
+ smtp_timeout = 30 # seconds
+ if env.smtp.use_tls:
+ server = smtplib.SMTP(smtp_host, smtp_port, timeout=smtp_timeout)
server.ehlo()
server.starttls()
server.ehlo()
else:
- server = smtplib.SMTP(smtp_host, smtp_port)
+ server = smtplib.SMTP(smtp_host, smtp_port, timeout=smtp_timeout)| except Exception as e: | ||
| raise HTTPException(status_code=500, detail=str(e)) |
There was a problem hiding this comment.
Refine exception handling to preserve error context.
Catching all exceptions with except Exception as e and converting to HTTPException(status_code=500, detail=str(e)) loses important error context and makes debugging harder. SMTP and SendGrid errors have different root causes (network timeouts, authentication failures, rate limits, invalid recipients, etc.) and should be handled with more granularity.
♻️ Suggested improvement
try:
if _USE_SMTP:
_send_via_smtp(to_email, subject, html_content, from_email)
else:
_send_via_sendgrid(to_email, subject, html_content, from_email)
return True
- except Exception as e:
- raise HTTPException(status_code=500, detail=str(e))
+ except smtplib.SMTPException as e:
+ log.error(f"SMTP error sending email to {to_email}: {e}", exc_info=True)
+ raise HTTPException(status_code=500, detail=f"Failed to send email via SMTP: {type(e).__name__}")
+ except Exception as e:
+ log.error(f"Unexpected error sending email to {to_email}: {e}", exc_info=True)
+ raise HTTPException(status_code=500, detail="Failed to send email")| """SMTP Email configuration""" | ||
|
|
||
| host: str | None = os.getenv("SMTP_HOST") | ||
| port: int = int(os.getenv("SMTP_PORT", "587")) |
There was a problem hiding this comment.
Guard against ValueError if SMTP_PORT contains non-numeric input.
If SMTP_PORT is set to a non-numeric string, int(os.getenv(...)) will raise ValueError and crash the application at startup. Consider wrapping in a try-except or using a helper that validates and falls back to the default.
🛡️ Proposed fix with validation
- port: int = int(os.getenv("SMTP_PORT", "587"))
+ port: int = (
+ int(port_str)
+ if (port_str := os.getenv("SMTP_PORT", "587")).isdigit()
+ else 587
+ )📝 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.
| port: int = int(os.getenv("SMTP_PORT", "587")) | |
| port: int = ( | |
| int(port_str) | |
| if (port_str := os.getenv("SMTP_PORT", "587")).isdigit() | |
| else 587 | |
| ) |
| const evaluationTypeBreadcrumb = useMemo(() => { | ||
| const typeMap: Record<string, {label: string; kind: string}> = { | ||
| auto: {label: "Auto Evals", kind: "auto"}, | ||
| custom: {label: "SDK Evals", kind: "custom"}, |
There was a problem hiding this comment.
Expand evaluationType prop union to include "custom" to avoid a TS contract break.
Line 51 adds "custom" behavior, but EvalRunPreviewPageProps still excludes it. With EvalRunTestPage now forwarding "custom", this becomes a type error at the component boundary.
Suggested fix
interface EvalRunPreviewPageProps {
runId: string
- evaluationType: "auto" | "human" | "online"
+ evaluationType: "auto" | "human" | "online" | "custom"
projectId?: string | null
}| 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.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Route this through the Fern client instead of fetchJson.
This is new frontend API code in web, but it manually builds the URL/query string and calls fetchJson directly. That bypasses the generated contract for this endpoint and the repo’s required queryParams call shape. Please add/use the matching Fern client method here instead of composing the request by hand.
As per coding guidelines, "All new frontend API code must go through the Fern-generated client, not raw axios", "Use getAgentaSdkClient({host: getAgentaApiUrl()})", and "Pass query params via {queryParams: {...}}, NOT axios's {params: {...}}."
Source: Coding guidelines
| const st = errorData.status.stacktrace | ||
| errorStacktrace = Array.isArray(st) ? st.join("\n") : st |
There was a problem hiding this comment.
Add type guard for stacktrace normalization to ensure string output.
The current logic assumes non-array values are strings, but if errorData.status.stacktrace is an object, number, or boolean, it will be assigned directly, violating the string | undefined type constraint.
🛡️ Suggested fix
const st = errorData.status.stacktrace
- errorStacktrace = Array.isArray(st) ? st.join("\n") : st
+ errorStacktrace = Array.isArray(st)
+ ? st.join("\n")
+ : typeof st === "string"
+ ? st
+ : undefined| 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.
Add type guard for inline stacktrace normalization.
Same issue as line 687: if st is not an array or string (e.g., object, number), it will be assigned directly, violating the type constraint.
🛡️ Suggested fix
- ...(st ? {stacktrace: Array.isArray(st) ? st.join("\n") : st} : {}),
+ ...(st ? {stacktrace: Array.isArray(st) ? st.join("\n") : typeof st === "string" ? st : undefined} : {}),📝 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.
| ...(st ? {stacktrace: Array.isArray(st) ? st.join("\n") : st} : {}), | |
| ...(st ? {stacktrace: Array.isArray(st) ? st.join("\n") : typeof st === "string" ? st : undefined} : {}), |
GanJiaKouN16
force-pushed
the
feat/annotation-queue-e2e-tests
branch
from
1e8500c to
cd01041
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/oss/src/services/profile/index.ts (1)
64-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis concern was already raised: route through the Fern client instead of
fetchJson.The previous review flagged that this new frontend API code manually builds the URL and calls
fetchJson, bypassing the Fern-generated contract. Additionally, the current implementation passesuser_idas a query parameter (url.searchParams.set), but theResetUserPasswordRequestDTO in the generated client showsuser_idas a body field, suggesting a potential parameter-passing mismatch.Source: Coding guidelines
🧹 Nitpick comments (1)
api/oss/src/services/user_service.py (1)
162-164: ⚡ Quick winConsider validating that the enabled backend has its corresponding from_address configured.
The fallback from
env.smtp.from_addresstoenv.sendgrid.from_addressprovides migration flexibility, but could mask configuration errors in production. For example, if SMTP is enabled butenv.smtp.from_addressis None, the code silently uses SendGrid's from_address with the SMTP backend, which may fail depending on server validation.Consider logging a warning when the fallback occurs, or validating that the enabled backend has its corresponding from_address configured.
💡 Proposed validation improvement
-from_address = env.smtp.from_address or env.sendgrid.from_address +from_address = env.smtp.from_address if env.smtp.enabled else env.sendgrid.from_address if not from_address: - raise ValueError("Email requires a sender email address to work.") + backend_name = "SMTP" if env.smtp.enabled else "SendGrid" + raise ValueError(f"Email requires a sender email address to work. Please configure {backend_name} from_address.") + +# Warn if using fallback sender +if env.smtp.enabled and not env.smtp.from_address and env.sendgrid.from_address: + log.warning("SMTP is enabled but smtp.from_address is not set; falling back to SendGrid from_address")
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c627e001-7494-4efc-851c-5db6dedbfa5c
📒 Files selected for processing (41)
api/ee/src/services/db_manager_ee.pyapi/ee/src/services/organization_service.pyapi/oss/src/services/email_service.pyapi/oss/src/services/organization_service.pyapi/oss/src/services/user_service.pyapi/oss/src/utils/env.pyapi/pyproject.tomldocs/docs/self-host/02-configuration.mdxhosting/docker-compose/ee/env.ee.dev.examplehosting/docker-compose/ee/env.ee.gh.examplehosting/docker-compose/oss/env.oss.dev.examplehosting/docker-compose/oss/env.oss.gh.examplesdks/python/agenta/sdk/middlewares/running/vault.pyweb/oss/src/components/EvalRunDetails/atoms/runInvocationAction.tsweb/oss/src/components/EvalRunDetails/components/Page.tsxweb/oss/src/components/EvalRunDetails/test.tsxweb/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/lib/helpers/buildBreadcrumbs.tsweb/oss/src/services/evaluations/invocations/api.tsweb/oss/src/services/profile/index.tsweb/oss/tests/playwright/acceptance/annotation-queue/annotation-queue.spec.tsweb/oss/tests/playwright/acceptance/annotation-queue/assets/types.tsweb/oss/tests/playwright/acceptance/annotation-queue/index.tsweb/oss/tests/playwright/acceptance/annotation-queue/tests.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 (3)
- web/oss/src/components/EvaluationRunsTablePOC/actions/navigationActions.ts
- api/pyproject.toml
- api/ee/src/services/db_manager_ee.py
✅ Files skipped from review due to trivial changes (6)
- web/oss/src/components/SharedDrawers/TraceDrawer/components/EvaluatorDetailsPopover.tsx
- web/oss/tests/playwright/acceptance/annotation-queue/annotation-queue.spec.ts
- hosting/docker-compose/oss/env.oss.dev.example
- hosting/docker-compose/ee/env.ee.dev.example
- hosting/docker-compose/oss/env.oss.gh.example
- docs/docs/self-host/02-configuration.mdx
🚧 Files skipped from review as they are similar to previous changes (27)
- web/packages/agenta-entities/src/runnable/types.ts
- web/packages/agenta-playground/src/state/execution/types.ts
- web/oss/src/components/EvalRunDetails/atoms/runInvocationAction.ts
- web/oss/src/components/pages/prompts/PromptsPage.tsx
- web/oss/src/components/pages/observability/components/SessionsTable/index.tsx
- web/oss/src/components/EvalRunDetails/test.tsx
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useEntityTableState.ts
- web/packages/agenta-annotation-ui/src/components/AnnotationSession/ScenarioListView.tsx
- web/oss/src/components/EvalRunDetails/components/Page.tsx
- web/packages/agenta-playground/src/executeWorkflowRevision.ts
- web/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsx
- web/oss/src/components/pages/observability/components/ObservabilityTable/index.tsx
- web/oss/src/components/EvaluationRunsTablePOC/components/EvaluationRunsTable/index.tsx
- api/oss/src/services/organization_service.py
- web/oss/src/services/evaluations/invocations/api.ts
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsx
- web/oss/tests/playwright/acceptance/annotation-queue/tests.ts
- api/ee/src/services/organization_service.py
- api/oss/src/utils/env.py
- api/oss/src/services/email_service.py
- hosting/docker-compose/ee/env.ee.gh.example
- sdks/python/agenta/sdk/middlewares/running/vault.py
- web/oss/src/components/pages/settings/WorkspaceManage/cellRenderers.tsx
- web/oss/src/components/InfiniteVirtualTable/hooks/useTableManager.tsx
- web/packages/agenta-entity-ui/src/shared/EntityTable.tsx
- web/oss/tests/playwright/acceptance/annotation-queue/index.ts
- web/packages/agenta-playground/src/state/execution/executionRunner.ts
2 participants
Summary
Fixes #4528 — adds Playwright e2e test coverage for annotation queue flows.
Tests added (WEB-ACC-ANNOTATION-001 through 006)
Structure
Follows the existing
human-annotation/test pattern:Fixtures
navigateToAnnotations()— navigates to/{projectURL}/annotationsand waits for the page to loadcreateAnnotationQueue({name, kind})— opens the Create Queue drawer, fills in name/kind, submits, and waits for drawer to closeCloses #4528