Yodan/connect eval service - #29
Open
yodanBargida wants to merge 14 commits into
Open
Conversation
…o yodan/connect-eval-service
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (54)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
|
coderabbit review --dir agent |
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/src/components/tables/TableList.tsx (1)
189-196: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty state is misleading when all rows were filtered out by the Spider2 toggle.
datais post-filtered, so hiding Spider2 tables on a Spider2-only result set renders "No tables found — Click 'Add Table' to get started". Branch onrawData?.lengthto show a "no tables match the current filters" message instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/tables/TableList.tsx` around lines 189 - 196, Update the empty-state branch in the TableList rendering to distinguish an empty raw dataset from a non-empty rawData set whose post-filtered data is empty. Use rawData?.length to show a “no tables match the current filters” message for filtered-out results, while preserving the existing noData/Add Table message when no tables exist at all.frontend/src/components/monitoring/RunHistoryTable.tsx (1)
403-424: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFiltering is applied after paging state and after the empty check.
Two consequences: (1) if every fetched run is excluded,
runs.lengthis still non-zero so theEmptySlatebranch is skipped and an empty table body renders; (2)pageis never clamped whenexcludeTableIdschanges, so a user on page 3 can land pasttotalPagesand see no rows.🐛 Proposed fix
const visibleRuns = excludeTableIds?.size ? runs.filter((r) => !r.table_id || !excludeTableIds.has(r.table_id)) : runs; - const paged = visibleRuns.slice(page * pageSize, (page + 1) * pageSize); - const totalPages = Math.ceil(visibleRuns.length / pageSize); + const totalPages = Math.ceil(visibleRuns.length / pageSize); + const safePage = Math.min(page, Math.max(totalPages - 1, 0)); + const paged = visibleRuns.slice(safePage * pageSize, (safePage + 1) * pageSize); + + useEffect(() => { + setPage(0); + }, [excludeTableIds]); if (isLoading) return ( <div className="run-history-loading"> <Spinner size={24} /> </div> ); - if (!runs.length) + if (!visibleRuns.length)Note:
excludeTableIdsis a memoizedSetinfrontend/src/pages/EvaluationsPage.tsx, so the effect dependency is stable there; usesafePagein the pagination controls/label too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/monitoring/RunHistoryTable.tsx` around lines 403 - 424, Base the empty-state check, pagination, and page count on filtered runs by moving the visibleRuns computation before those branches. Clamp the current page when visibleRuns or totalPages changes, derive a safePage value, and use it for slicing plus all pagination controls and page labels; ensure an entirely excluded result renders EmptySlate rather than an empty table.frontend/src/pages/SandboxPage.tsx (1)
11-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLoad
.score-ring--runningin SandboxPage.
EvaluationTab.cssis only imported fromEvaluationTab.tsx, whileSandboxPage.tsxmounts the same.score-ring score-ring--runningmarkup without a matching stylesheet. Add this modifier toglobals.cssor importEvaluationTab.cssfromSandboxPage.tsx; otherwise the running score ring renders unstyled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/SandboxPage.tsx` around lines 11 - 22, Make the running ScoreRing markup in SandboxPage render with its required styles by either adding the .score-ring--running rules to globals.css or importing EvaluationTab.css from SandboxPage.tsx. Preserve the existing ScoreRing status handling and avoid duplicating styles across both locations.backend/app/routers/evaluation.py (1)
493-516: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
table_namesaccumulation.
table_namesis built inside the loop at Lines 494-496 and then unconditionally reassigned at Line 514. Drop the loop-side accumulation.♻️ Proposed cleanup
all_production_questions: list[GoldenQuestion] = [] - table_names = [] for table in prod_tables: - table_names.append(table.name) qs = session.exec(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/routers/evaluation.py` around lines 493 - 516, Remove the loop-side table_names initialization and append operation in the production-table processing block. Keep the later schema-qualified table_names comprehension unchanged, as it is the value used after the loop.agent/src/agent/nodes/query_builder.py (1)
13-36: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUser feedback and skill prompts are computed but never sent to the LLM.
feedback_str(including any appended skill-prompt text) is built but the corresponding chain input field is commented out, and the new query_builder prompt template has no{{feedback_str}}placeholder either. This silently breaks the "user rejects → regenerate with feedback" flow and the loaded-skills injection.Suggested fix
response = await chain.ainvoke( { "schema_plan": state.get("schema_plan"), "user_query": state.get("user_query"), - # "feedback_str": feedback_str, + "feedback_str": feedback_str, } )Also add a
{{feedback_str}}placeholder to thetext2sql/query_builderprompt inagent/scripts/upload_all_prompts.pyso the fed value is actually used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/src/agent/nodes/query_builder.py` around lines 13 - 36, Pass the computed feedback_str into the chain.ainvoke input in the query-builder node, and update the text2sql/query_builder prompt definition in upload_all_prompts.py to include a {{feedback_str}} placeholder so rejection feedback and loaded-skill prompts reach the LLM.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/scripts/upload_all_prompts.py`:
- Around line 49-53: Update the constraints in the prompt-building logic near
the flat JSON schema to remove the stale references to an "english" key and
"both keys." Replace them with wording that consistently requires a valid flat
object mapping each extracted Hebrew location to its standard English
translation, and an empty object when no locations are found.
- Line 161: Update the geographic-distance instruction in the prompt content
handled by upload_all_prompts.py to name Trino’s function as
to_spherical_geography(), replacing toSphericalGeography(). Preserve the
surrounding WGS84 and spherical geography guidance.
In `@agent/src/agent/nodes/refiner.py`:
- Around line 126-134: Extract the duplicated datetime/date-to-ISO conversion
logic into a shared json_serial helper in
agent/src/agent/utils/serialization.py, preserving the TypeError behavior for
unsupported values. In agent/src/agent/nodes/refiner.py lines 126-134 and
agent/src/agent/nodes/finalizer.py lines 33-44, import and reuse the shared
helper, removing each local json_serial definition.
- Around line 159-173: The build_refiner_schema_context function must restore a
bounded schema-context representation for table_profiles instead of serializing
the entire profile blob. Apply the existing or appropriate profile-size/token
limit before json.dumps, while preserving the schema_plan fallback and “No
schema context available.” behavior.
In `@agent/src/agent/nodes/schema_explorer.py`:
- Around line 326-351: Update the Trino fallback in async function
get_table_profile to run execute_query_sync via await asyncio.to_thread, passing
the DESCRIBE SQL as its argument. Preserve the existing success handling, JSON
response, and exception logging while ensuring the event loop is not blocked.
In `@backend/app/config.py`:
- Around line 41-42: Verify the startup paths controlled by RUN_SEED and
RUN_INFRA_INIT are idempotent and safe to execute on every backend restart,
including production deployments with default configuration. Confirm repeated
seeding, OpenMetadata pipeline creation, and catalog verification do not
duplicate, corrupt, or otherwise disrupt existing resources; update those paths
if necessary to ensure safe re-runs.
In `@backend/app/main.py`:
- Line 136: Protect query execution by moving query.router from api_router to
private_router in backend/app/main.py so get_current_user applies; additionally,
in backend/app/routers/query.py around the query route, add an explicit
authentication dependency and reject non-read-only SQL statements, allowing only
read-only queries.
In `@backend/app/routers/evaluation.py`:
- Around line 382-390: The additional_tables contract uses inconsistent table
identifier formats across evaluation and orchestration callers. Introduce or
reuse one shared formatter that produces catalog.schema.table identifiers, then
apply it to the baseline table_names construction in evaluation.py, both
production and spider2 branches in orchestration.py, the regression path in
evaluation.py, and the single-table and candidate paths in evaluation.py; ensure
every caller sends the same fully qualified format.
- Around line 691-791: The evaluation post-processing logic is duplicated across
routers and should have one shared implementation. In
backend/app/routers/evaluation.py lines 691-791, extract the body of
_run_evaluation_pipeline into a shared service function, including regression
handling, alerts, and metric persistence, then call it from
_run_evaluation_pipeline. In backend/app/routers/orchestration.py lines 75-169,
replace the duplicated per-table loop body with that shared function. Move
REGRESSION_BLOCK_DELTA, REGRESSION_WARNING_DELTA, LOW_SCORE_THRESHOLD, and
_create_alert from backend/app/routers/evaluation.py lines 665-689 into the
shared module, and remove the duplicate definitions from
backend/app/routers/orchestration.py lines 187-205.
- Line 41: The router modules have a circular module-level import. In
backend/app/routers/evaluation.py:41-41, remove the top-level
get_orchestration_report import and import it inside the get_run_report handler
at its use site. In backend/app/routers/orchestration.py:35-40, retain the
execute_single_table_eval import only with the evaluation-side deferred;
otherwise move it into _run_full_pipeline, matching _run_dataset_pipeline’s
deferred-import pattern.
- Around line 193-206: Update the failure_breakdown construction in the
evaluation response handling so fixed counter keys cannot overwrite category
entries from eval_resp.failure_analysis.categories. Preserve both values by
merging without clobbering existing category keys or by placing fixed counters
under a distinct namespace, while keeping run.failure_breakdown populated with
all breakdown details.
- Around line 754-763: Update the low-score alert condition in
execute_single_table_eval to require a successful run status in addition to
score < LOW_SCORE_THRESHOLD. Preserve the existing alert creation for genuinely
low-scoring successful runs, while skipping it when the run is failed.
In `@backend/app/routers/orchestration.py`:
- Around line 322-326: Move the long-running request in _run_dataset_pipeline
out of FastAPI’s shared BackgroundTasks/AnyIO threadpool by dispatching dataset
evaluations through a dedicated worker or queue. Preserve the existing
evaluation payload, endpoint, and 600-second timeout while ensuring concurrent
runs cannot consume the shared sync-route worker slots.
- Around line 469-477: Extract the run-label fallback logic from the
orchestration endpoint into a shared helper, then update both this endpoint and
evaluation.py’s get_run to use it. Ensure identical records produce the same
labels, including table names, triggered-by dataset labels, and the Production
Baseline/Production Regression/Unknown cases.
- Around line 347-353: Validate dataset_name in trigger_dataset_run against an
explicit allowlist of supported datasets before creating the run or scheduling
background work. Reject unsupported values with the established client-error
response, and keep _run_dataset_pipeline reachable only for allowed dataset
names.
In `@backend/app/routers/query.py`:
- Around line 47-48: Replace the unbounded cur.fetchall() call in the query
handler with a server-side capped fetch using a defined MAX_ROWS limit, and
determine whether additional rows remain to set a truncation flag. Include that
flag in the API response while preserving the existing column extraction and
returned rows.
- Around line 33-41: Update the TRINO_ENABLED guard in execute_query to return
QueryResponse with success=False when Trino execution is disabled, while
preserving the existing error message and empty-result fields. Update
test_execute_query_disabled to assert the disabled execution is unsuccessful.
- Around line 43-71: Update the query execution handler in
backend/app/routers/query.py to delegate SQL execution to
core.trino.execute_query_sync instead of manually managing the Trino connection
and cursor. Preserve the existing QueryResponse success/error fields and
execution-time calculation while removing the local get_trino_connection,
cursor, fetch, and close logic.
In `@backend/app/spider2_questions.json`:
- Around line 1-1702: Remove the stale spider2_questions.json golden-question
file from the repository. Do not modify sync_om_metadata.py or add replacement
data, since no remaining references depend on this file.
In `@backend/app/sync_om_metadata.py`:
- Around line 90-92: Update the module docstring’s “Run with” command to
reference the file’s current backend/app/sync_om_metadata.py location while
preserving the existing optional flags.
- Around line 617-647: The table-selection loop in the question processing flow
should track when no extracted ref_tables match and the lowest-id catalog table
is used as a fallback. Add a separate fallback counter or equivalent summary
metric, increment it only for this fallback path, and include it in the run
summary alongside the existing inserted/skipped/failed counts.
- Around line 458-467: Parallelize the per-instance Spider2-Snow gold SQL
fetches in the loop around `_fetch_spider2_snow_gold_sql` using a small
`ThreadPoolExecutor`, submitting independent `sf_` instance requests and
collecting their results before updating metadata. Preserve the existing
`_fetch_spider2_snow_gold_sql` error handling and result mapping, while limiting
concurrency to avoid excessive GitHub requests.
- Around line 681-692: The tables_with_zero check currently performs one
GoldenQuestion query per table; replace this N+1 loop with a single query that
selects GoldenQuestion.table_id for all table IDs, build the returned ID set,
and identify tables whose IDs are absent while preserving the existing fully
qualified names and warning behavior.
- Around line 85-86: Align the environment-variable name used by the
documentation, runtime error, and lookup in the sync metadata flow. Update the
lookup associated with the OM_JWT_TOKEN variable to use the single canonical
name established elsewhere in the application, and ensure the docstring and
missing-variable error report that same name consistently.
In `@backend/tests/test_api.py`:
- Around line 263-283: Add resource-cleanup assertions to
test_execute_query_failure, verifying the mocked cursor and Trino connection are
closed after mock_cur.execute raises. Use the existing mock_cur and mock_conn
symbols and preserve all current response assertions.
In `@frontend/src/components/monitoring/RunHistoryTable.tsx`:
- Around line 396-400: Update the refetchInterval callback in RunHistoryTable so
it returns 5_000 while any run is running, but returns false when no run is
running, stopping unconditional polling and preserving manual refresh behavior.
In `@frontend/src/components/tables/EvaluationTab.tsx`:
- Around line 19-29: Remove the local ScoreRing definition from
frontend/src/components/tables/EvaluationTab.tsx lines 19-29 and import the
shared component from frontend/src/components/common/EvalUI.tsx. In
frontend/src/pages/SandboxPage.tsx lines 11-22, remove its duplicate ScoreRing
and import the same shared component; move the .score-ring styles into the
shared component’s styling so they load for both usages.
In `@frontend/src/pages/EvaluationsPage.tsx`:
- Around line 420-433: Update handleLaunch’s triggerDatasetMut.onError callback
to display an error through App.useApp()’s message.error, using the
server-provided detail as RunTriggerPanel does, while preserving the existing
runningDataset and runningRunId reset behavior.
- Around line 194-206: Replace the repeated `owner_id === 'spider2'` checks in
`filteredTables`, the other `EvaluationsPage` location, and `TableList` with a
shared `isSpider2Table` constant or predicate. Centralize the dataset-name
literal and owner-field access/casting there, then reuse it while preserving the
existing Spider2 filtering behavior.
- Around line 503-520: Keep the refetchInterval callback pure by only returning
false for completed or failed runs, without updating React state. Add an effect
tied to the running-run-status query data that clears runningDataset and
runningRunId when the run reaches a terminal status.
In `@scripts/generate_trino_catalogs.py`:
- Around line 102-126: Make get_question_referenced_db_ids() resilient to GitHub
fetch, HTTP, and parsing-related failures by catching the external-fetch
exception, logging a warning, and returning None as a no-filter fallback. Update
main() to treat referenced_db_ids is None as “all discovered non-denied
databases,” preserving the prior catalog-generation behavior while retaining
filtering when the fetch succeeds.
- Around line 102-126: The Spider2-Snow JSONL fetch and sf_ filtering are
duplicated across two files; extract them into one shared core helper. In
scripts/generate_trino_catalogs.py, update get_question_referenced_db_ids() to
call that helper and retain only this script’s uppercasing/set construction. In
backend/app/sync_om_metadata.py, refactor fetch_spider2_snow_questions() to use
the same helper while keeping gold-SQL downloading and translation local; update
both files as specified: scripts/generate_trino_catalogs.py lines 102-126 and
backend/app/sync_om_metadata.py lines 488-532.
---
Outside diff comments:
In `@agent/src/agent/nodes/query_builder.py`:
- Around line 13-36: Pass the computed feedback_str into the chain.ainvoke input
in the query-builder node, and update the text2sql/query_builder prompt
definition in upload_all_prompts.py to include a {{feedback_str}} placeholder so
rejection feedback and loaded-skill prompts reach the LLM.
In `@backend/app/routers/evaluation.py`:
- Around line 493-516: Remove the loop-side table_names initialization and
append operation in the production-table processing block. Keep the later
schema-qualified table_names comprehension unchanged, as it is the value used
after the loop.
In `@frontend/src/components/monitoring/RunHistoryTable.tsx`:
- Around line 403-424: Base the empty-state check, pagination, and page count on
filtered runs by moving the visibleRuns computation before those branches. Clamp
the current page when visibleRuns or totalPages changes, derive a safePage
value, and use it for slicing plus all pagination controls and page labels;
ensure an entirely excluded result renders EmptySlate rather than an empty
table.
In `@frontend/src/components/tables/TableList.tsx`:
- Around line 189-196: Update the empty-state branch in the TableList rendering
to distinguish an empty raw dataset from a non-empty rawData set whose
post-filtered data is empty. Use rawData?.length to show a “no tables match the
current filters” message for filtered-out results, while preserving the existing
noData/Add Table message when no tables exist at all.
In `@frontend/src/pages/SandboxPage.tsx`:
- Around line 11-22: Make the running ScoreRing markup in SandboxPage render
with its required styles by either adding the .score-ring--running rules to
globals.css or importing EvaluationTab.css from SandboxPage.tsx. Preserve the
existing ScoreRing status handling and avoid duplicating styles across both
locations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fcc587d4-0505-4b5b-9f35-e8d93b4cc834
⛔ Files ignored due to path filters (2)
backend/uv.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (54)
agent/scripts/upload_all_prompts.pyagent/src/agent/nodes/finalizer.pyagent/src/agent/nodes/query_builder.pyagent/src/agent/nodes/refiner.pyagent/src/agent/nodes/schema_explorer.pyagent/src/agent/routers/chat.pybackend/app/config.pybackend/app/infra_init.pybackend/app/main.pybackend/app/routers/__init__.pybackend/app/routers/agent.pybackend/app/routers/evaluation.pybackend/app/routers/flags.pybackend/app/routers/orchestration.pybackend/app/routers/query.pybackend/app/services/evaluator.pybackend/app/services/flag_service.pybackend/app/services/langfuse_client.pybackend/app/services/scheduler.pybackend/app/services/trino_client.pybackend/app/spider2_questions.jsonbackend/app/sync_om_metadata.pybackend/pyproject.tomlbackend/tests/test_api.pycore/src/core/models/models.pycore/src/core/trino.pydocker-compose.ymlfrontend/nginx.conffrontend/package.jsonfrontend/src/api/orchestration.tsfrontend/src/components/JsonTreeView.tsxfrontend/src/components/flags/FlagEditor.tsxfrontend/src/components/flags/ModeCard.tsxfrontend/src/components/monitoring/RunHistoryTable.cssfrontend/src/components/monitoring/RunHistoryTable.tsxfrontend/src/components/tables/EvaluationTab.cssfrontend/src/components/tables/EvaluationTab.tsxfrontend/src/components/tables/TableList.cssfrontend/src/components/tables/TableList.tsxfrontend/src/config/flagsConfig.tsfrontend/src/hooks/useEvaluations.tsfrontend/src/index.cssfrontend/src/pages/AgentTestingPage.module.cssfrontend/src/pages/ControlCenterPage.tsxfrontend/src/pages/EvaluationsPage.cssfrontend/src/pages/EvaluationsPage.tsxfrontend/src/pages/FlagsPage.cssfrontend/src/pages/SandboxPage.tsxfrontend/src/tests/components.test.tsxfrontend/tests/agent-testing.spec.tsfrontend/tests/real-agent.spec.tsinfra/trino/etc/jvm.configscripts/generate_trino_catalogs.pytext2sql_test
Comment on lines
+126
to
+134
| def json_serial(obj): | ||
| if isinstance(obj, (datetime.datetime, datetime.date)): | ||
| return obj.isoformat() | ||
| raise TypeError("Type %s not serializable" % type(obj)) | ||
|
|
||
| if esca_write_enabled: | ||
| try: | ||
| payload_data = {"columns": result.columns, "rows": result.rows} | ||
| payload = json.dumps(payload_data, default=str).encode() | ||
| payload = json.dumps(payload_data, default=json_serial).encode() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated json_serial datetime helper across two files.
The same local json_serial(obj) function (datetime/date → ISO string, else raise TypeError) is defined independently in both files for the same purpose (making ESCA JSON payloads datetime-safe). Extract it once into a shared utility module.
agent/src/agent/nodes/refiner.py#L126-L134: replace the localjson_serialdefinition with an import from a shared helper (e.g.agent/src/agent/utils/serialization.py::json_serial).agent/src/agent/nodes/finalizer.py#L33-L44: same — replace the localjson_serialdefinition with an import of the shared helper.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 133-133: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload_data, default=json_serial)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
📍 Affects 2 files
agent/src/agent/nodes/refiner.py#L126-L134(this comment)agent/src/agent/nodes/finalizer.py#L33-L44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/src/agent/nodes/refiner.py` around lines 126 - 134, Extract the
duplicated datetime/date-to-ISO conversion logic into a shared json_serial
helper in agent/src/agent/utils/serialization.py, preserving the TypeError
behavior for unsupported values. In agent/src/agent/nodes/refiner.py lines
126-134 and agent/src/agent/nodes/finalizer.py lines 33-44, import and reuse the
shared helper, removing each local json_serial definition.
Comment on lines
+19
to
29
| function ScoreRing({ score, status }: { score: number; status?: string }) { | ||
| if (status === 'running') { | ||
| return ( | ||
| <div className="score-ring score-ring--running" title="Evaluation in progress…"> | ||
| <Spinner size={14} color="#f59e0b" /> | ||
| </div> | ||
| ); | ||
| } | ||
| const pct = Math.round(score * 100); | ||
| const cls = pct >= 50 ? 'score-ring--high' : 'score-ring--low'; | ||
| return <div className={`score-ring ${cls}`}>{pct}%</div>; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicated ScoreRing component across two files. Both files define an identical ScoreRing that gained the same status === 'running' spinner branch; the shared root cause is a missing common component, which also explains the CSS-locality question on the Sandbox page.
frontend/src/components/tables/EvaluationTab.tsx#L19-L29: remove the local definition and importScoreRingfromfrontend/src/components/common/EvalUI.tsx.frontend/src/pages/SandboxPage.tsx#L11-L22: remove the local copy and import the same shared component, moving.score-ring*styles alongside it so they load wherever it is used.
📍 Affects 2 files
frontend/src/components/tables/EvaluationTab.tsx#L19-L29(this comment)frontend/src/pages/SandboxPage.tsx#L11-L22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/tables/EvaluationTab.tsx` around lines 19 - 29,
Remove the local ScoreRing definition from
frontend/src/components/tables/EvaluationTab.tsx lines 19-29 and import the
shared component from frontend/src/components/common/EvalUI.tsx. In
frontend/src/pages/SandboxPage.tsx lines 11-22, remove its duplicate ScoreRing
and import the same shared component; move the .score-ring styles into the
shared component’s styling so they load for both usages.
Comment on lines
+420
to
+433
| const handleLaunch = (datasetName: string) => { | ||
| setRunningDataset(datasetName); | ||
| triggerDatasetMut.mutate(datasetName, { | ||
| onSuccess: (run) => { | ||
| setRunningRunId(run.id); | ||
| // Switch to history tab so the user can watch progress | ||
| onLaunch?.(); | ||
| }, | ||
| onError: () => { | ||
| setRunningDataset(null); | ||
| setRunningRunId(null); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Surface dataset-run failures to the user.
onError silently resets local state, so a failed trigger looks like nothing happened. RunTriggerPanel already uses App.useApp()'s message.error with the server detail; mirror that here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/pages/EvaluationsPage.tsx` around lines 420 - 433, Update
handleLaunch’s triggerDatasetMut.onError callback to display an error through
App.useApp()’s message.error, using the server-provided detail as
RunTriggerPanel does, while preserving the existing runningDataset and
runningRunId reset behavior.
Comment on lines
+503
to
+520
| useQuery({ | ||
| queryKey: ['running-run-status', runningRunId], | ||
| queryFn: () => { | ||
| if (!runningRunId) return null; | ||
| return orchestrationApi.getRun(runningRunId); | ||
| }, | ||
| enabled: !!runningRunId, | ||
| refetchInterval: (query) => { | ||
| const data = query.state.data as any; | ||
| if (data && (data.status === 'completed' || data.status === 'failed')) { | ||
| // Run has finished, clear states | ||
| setRunningDataset(null); | ||
| setRunningRunId(null); | ||
| return false; | ||
| } | ||
| return 3000; // poll every 3 seconds | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't mutate React state inside refetchInterval.
refetchInterval is evaluated by the query observer during render/subscription updates, so calling setRunningDataset/setRunningRunId there is a side effect in a render path (can trigger "Cannot update a component while rendering" warnings and re-entrant renders). Keep the interval callback pure and clear state from an effect on the query data.
♻️ Proposed fix
- useQuery({
+ const { data: runningRun } = useQuery({
queryKey: ['running-run-status', runningRunId],
queryFn: () => {
if (!runningRunId) return null;
return orchestrationApi.getRun(runningRunId);
},
enabled: !!runningRunId,
- refetchInterval: (query) => {
- const data = query.state.data as any;
- if (data && (data.status === 'completed' || data.status === 'failed')) {
- // Run has finished, clear states
- setRunningDataset(null);
- setRunningRunId(null);
- return false;
- }
- return 3000; // poll every 3 seconds
- },
+ refetchInterval: (query) => {
+ const status = (query.state.data as any)?.status;
+ return status === 'completed' || status === 'failed' ? false : 3000;
+ },
});
+
+ useEffect(() => {
+ const status = (runningRun as any)?.status;
+ if (status === 'completed' || status === 'failed') {
+ setRunningDataset(null);
+ setRunningRunId(null);
+ }
+ }, [runningRun]);📝 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
| useQuery({ | |
| queryKey: ['running-run-status', runningRunId], | |
| queryFn: () => { | |
| if (!runningRunId) return null; | |
| return orchestrationApi.getRun(runningRunId); | |
| }, | |
| enabled: !!runningRunId, | |
| refetchInterval: (query) => { | |
| const data = query.state.data as any; | |
| if (data && (data.status === 'completed' || data.status === 'failed')) { | |
| // Run has finished, clear states | |
| setRunningDataset(null); | |
| setRunningRunId(null); | |
| return false; | |
| } | |
| return 3000; // poll every 3 seconds | |
| }, | |
| }); | |
| const { data: runningRun } = useQuery({ | |
| queryKey: ['running-run-status', runningRunId], | |
| queryFn: () => { | |
| if (!runningRunId) return null; | |
| return orchestrationApi.getRun(runningRunId); | |
| }, | |
| enabled: !!runningRunId, | |
| refetchInterval: (query) => { | |
| const status = (query.state.data as any)?.status; | |
| return status === 'completed' || status === 'failed' ? false : 3000; | |
| }, | |
| }); | |
| useEffect(() => { | |
| const status = (runningRun as any)?.status; | |
| if (status === 'completed' || status === 'failed') { | |
| setRunningDataset(null); | |
| setRunningRunId(null); | |
| } | |
| }, [runningRun]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/pages/EvaluationsPage.tsx` around lines 503 - 520, Keep the
refetchInterval callback pure by only returning false for completed or failed
runs, without updating React state. Add an effect tied to the running-run-status
query data that clears runningDataset and runningRunId when the run reaches a
terminal status.
Comment on lines
+102
to
+126
| def get_question_referenced_db_ids() -> set[str]: | ||
| """ | ||
| Fetch the Spider2-Snow sf_ question set and return the distinct db_id | ||
| values it references (uppercased, matching Snowflake's SHOW DATABASES | ||
| casing). Used to skip generating Trino catalogs for databases that will | ||
| never have a single golden question -- no point ingesting/syncing them | ||
| at all. | ||
| """ | ||
| import requests as _requests | ||
| url = "https://raw.githubusercontent.com/xlang-ai/Spider2/main/spider2-snow/spider2-snow.jsonl" | ||
| resp = _requests.get(url, timeout=15) | ||
| resp.raise_for_status() | ||
| db_ids = set() | ||
| for line in resp.text.splitlines(): | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| q = json.loads(line) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| if q.get("instance_id", "").startswith("sf_") and q.get("db_id"): | ||
| db_ids.add(q["db_id"].upper()) | ||
| return db_ids | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled GitHub fetch failure aborts the whole catalog-generation run.
get_question_referenced_db_ids() has no try/except around the network call; any transient GitHub outage or non-200 response makes main() crash via an unhandled exception, even after Snowflake enumeration already succeeded (line 153-156). This introduces a new external-service hard dependency for a script that previously only needed Snowflake connectivity. Consider wrapping the fetch in try/except with a sensible fallback (e.g. log a warning and treat all discovered Snowflake DBs as "referenced" so catalog generation can still proceed) rather than failing the entire run.
🛡️ Proposed fix
import requests as _requests
url = "https://raw.githubusercontent.com/xlang-ai/Spider2/main/spider2-snow/spider2-snow.jsonl"
- resp = _requests.get(url, timeout=15)
- resp.raise_for_status()
+ try:
+ resp = _requests.get(url, timeout=15)
+ resp.raise_for_status()
+ except Exception as exc:
+ logger.warning(
+ "Could not fetch golden-question db_id set from GitHub (%s); "
+ "will not filter target databases by referenced questions.", exc
+ )
+ return NoneAnd in main(), treat referenced_db_ids is None as "no filtering" (fall back to prior behavior of generating catalogs for all non-denied databases).
Also applies to: 158-159
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 116-116: for loop variable line overwritten by assignment target
(PLW2901)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate_trino_catalogs.py` around lines 102 - 126, Make
get_question_referenced_db_ids() resilient to GitHub fetch, HTTP, and
parsing-related failures by catching the external-fetch exception, logging a
warning, and returning None as a no-filter fallback. Update main() to treat
referenced_db_ids is None as “all discovered non-denied databases,” preserving
the prior catalog-generation behavior while retaining filtering when the fetch
succeeds.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Duplicated Spider2-Snow JSONL fetch/filter logic across two files in the same repo. Both get_question_referenced_db_ids() and fetch_spider2_snow_questions() independently fetch spider2-snow.jsonl from the same hardcoded GitHub URL, filter to instance_id starting with sf_, and extract db_id. Unlike the cross-repo justification sync_om_metadata.py gives for duplicating the evaluation service's Spider2SnowDownloader, these two live in the same repo and already share the core package — extracting a single helper (e.g. core.spider2.get_sf_questions() / get_referenced_db_ids()) would prevent the two copies from drifting if the URL or filter criteria ever change.
scripts/generate_trino_catalogs.py#L102-L126: replaceget_question_referenced_db_ids()'s body with a call into the shared helper, keeping only the uppercasing/set-building specific to this script.backend/app/sync_om_metadata.py#L488-L532: extract the JSONL-fetch +sf_-filter portion offetch_spider2_snow_questions()into the same shared helper, keeping the gold-SQL-download/translation steps local.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 116-116: for loop variable line overwritten by assignment target
(PLW2901)
📍 Affects 2 files
scripts/generate_trino_catalogs.py#L102-L126(this comment)backend/app/sync_om_metadata.py#L488-L532
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate_trino_catalogs.py` around lines 102 - 126, The Spider2-Snow
JSONL fetch and sf_ filtering are duplicated across two files; extract them into
one shared core helper. In scripts/generate_trino_catalogs.py, update
get_question_referenced_db_ids() to call that helper and retain only this
script’s uppercasing/set construction. In backend/app/sync_om_metadata.py,
refactor fetch_spider2_snow_questions() to use the same helper while keeping
gold-SQL downloading and translation local; update both files as specified:
scripts/generate_trino_catalogs.py lines 102-126 and
backend/app/sync_om_metadata.py lines 488-532.
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
| all_production_questions.extend(qs) | ||
|
|
||
| all_questions_payload = [] | ||
| from app.routers.evaluation import _build_questions_payload |
Collaborator
There was a problem hiding this comment.
להעלות את כל אימפורטים
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 by CodeRabbit
New Features
Bug Fixes
Tests