Add delete run API and Run Summary control - #300
Conversation
- Transactional SQLite delete for all run-scoped tables in FK-safe order
- DELETE /v1/simulations/runs/{run_id} with ownership check (RUN_FORBIDDEN)
- UI: deleteRun client, confirm + Delete run next to Export, state cleanup
Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🚅 Deployed to the social_agent_simulation_p-pr-300 environment in agent-simulation-platform
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a complete end-to-end feature for deleting simulation runs across the database, API, and UI layers. Changes include a new database deletion method with cascading deletes, an authenticated API endpoint with authorization checks, frontend UI with a delete button and confirmation prompt, comprehensive state cleanup on the client, and full test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/api/test_simulation_run_delete.py`:
- Around line 43-83: Add two new test cases in the TestDeleteSimulationRun
suite: one that calls DELETE /v1/simulations/runs/{run_id} with a non-existent
run_id (e.g., "run-delete-missing-...") and asserts a 404 status and error code
"RUN_NOT_FOUND", and another that calls DELETE with an invalid run_id (e.g.,
empty string or malformed ID) and asserts a 400 status and appropriate
validation error code (e.g., "INVALID_INPUT"); use the existing
simulation_client fixture and mirror the structure of
test_delete_run_returns_204_and_get_returns_404 and
test_delete_run_returns_403_when_app_user_mismatch to locate where to add the
new tests in the TestDeleteSimulationRun class and ensure GET behavior is not
affected.
- Around line 12-40: The helper _insert_completed_run currently opens the SQLite
connection and calls conn.commit() and conn.close() manually; change it to use a
context manager (with sqlite3.connect(temp_db) as conn:) so the connection is
always closed (and committed on successful exit) even if an exception occurs,
and move the conn.execute(...) and commit logic inside that with block while
removing the explicit conn.close() call.
In `@ui/components/details/RunSummary.tsx`:
- Around line 53-62: Replace the fire-and-forget delete with an async handler
that uses an is-prefix boolean state (e.g., isDeleting) in the RunSummary
component: set isDeleting = true before calling onDeleteRun(), await
onDeleteRun() inside a try/catch, show user feedback on error (toast/alert) in
the catch, and always set isDeleting = false in finally; update
handleDeleteRunClick to be async and use the new state, and update the delete
button to disable while isDeleting (and optionally show a spinner) so users get
loading/error feedback.
In `@ui/hooks/useSimulationPageState.ts`:
- Around line 491-536: handleDeleteRun currently calls deleteRun(runId) and only
performs UI cleanup on success, so if deleteRun throws the error propagates and
callers ignore it; wrap the deleteRun call in a try/catch inside handleDeleteRun
(function name: handleDeleteRun) and on catch populate a new error state keyed
by runId (e.g., runDeletionErrorByRunId via setRunDeletionErrorByRunId) with the
caught error and then rethrow (or return a failure indicator) so the caller
(RunSummary) can display the error; do not perform the state-cleanup steps when
deleteRun fails.
In `@ui/openapi.json`:
- Around line 1542-1555: The OpenAPI spec lacks documented 403/404/400 responses
for the run endpoints; update the FastAPI route decorator in
simulation/api/routes/runs.py (the route that raises RUN_NOT_FOUND,
RUN_FORBIDDEN, and INVALID_RUN_ID) to include a responses={...} mapping that
explicitly documents 403 (RUN_FORBIDDEN), 404 (RUN_NOT_FOUND) and 400
(INVALID_RUN_ID) with appropriate descriptions and JSON schema/$ref to your
existing error schema (e.g., HTTPError or HTTPValidationError), then regenerate
the OpenAPI (ui/openapi.json) so the UI client types include these error
responses.
🪄 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
Run ID: 25881e3a-d6cf-4619-98e2-436190fdb772
⛔ Files ignored due to path filters (2)
docs/plans/2026-03-24_delete_run_a7f3e2/images/after/start_screen_after_delete.pngis excluded by!**/*.pngdocs/plans/2026-03-24_delete_run_a7f3e2/images/before/summary_with_export_and_delete.pngis excluded by!**/*.png
📒 Files selected for processing (16)
db/repositories/interfaces.pydb/repositories/run_repository.pydb/services/run_deletion_sqlite.pysimulation/api/errors.pysimulation/api/routes/runs.pysimulation/api/services/run_delete_service.pysimulation/core/engine.pytests/api/test_simulation_run_delete.pyui/app/page.tsxui/components/details/DetailsPanel.tsxui/components/details/RunSummary.tsxui/components/run-detail/RunDetailContext.tsxui/hooks/useSimulationPageState.tsui/lib/api/simulation.tsui/openapi.jsonui/types/api.generated.ts
| def _insert_completed_run( | ||
| temp_db: str, | ||
| *, | ||
| run_id: str, | ||
| app_user_id: str | None, | ||
| ) -> None: | ||
| conn = sqlite3.connect(temp_db) | ||
| conn.execute( | ||
| """ | ||
| INSERT INTO runs ( | ||
| run_id, app_user_id, created_at, total_turns, total_agents, | ||
| feed_algorithm, metric_keys, started_at, status, completed_at | ||
| ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| """, | ||
| ( | ||
| run_id, | ||
| app_user_id, | ||
| "2026-01-01T00:00:00", | ||
| 1, | ||
| 1, | ||
| "chronological", | ||
| None, | ||
| "2026-01-01T00:00:00", | ||
| "completed", | ||
| "2026-01-01T00:00:01", | ||
| ), | ||
| ) | ||
| conn.commit() | ||
| conn.close() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using context manager for SQLite connection.
The helper function manually calls conn.commit() and conn.close(). Using a context manager ensures the connection is properly closed even if an exception occurs.
♻️ Suggested improvement
def _insert_completed_run(
temp_db: str,
*,
run_id: str,
app_user_id: str | None,
) -> None:
- conn = sqlite3.connect(temp_db)
- conn.execute(
- """
- INSERT INTO runs (
- run_id, app_user_id, created_at, total_turns, total_agents,
- feed_algorithm, metric_keys, started_at, status, completed_at
- ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
- """,
- (
- run_id,
- app_user_id,
- "2026-01-01T00:00:00",
- 1,
- 1,
- "chronological",
- None,
- "2026-01-01T00:00:00",
- "completed",
- "2026-01-01T00:00:01",
- ),
- )
- conn.commit()
- conn.close()
+ with sqlite3.connect(temp_db) as conn:
+ conn.execute(
+ """
+ INSERT INTO runs (
+ run_id, app_user_id, created_at, total_turns, total_agents,
+ feed_algorithm, metric_keys, started_at, status, completed_at
+ ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
+ """,
+ (
+ run_id,
+ app_user_id,
+ "2026-01-01T00:00:00",
+ 1,
+ 1,
+ "chronological",
+ None,
+ "2026-01-01T00:00:00",
+ "completed",
+ "2026-01-01T00:00:01",
+ ),
+ )
+ conn.commit()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/api/test_simulation_run_delete.py` around lines 12 - 40, The helper
_insert_completed_run currently opens the SQLite connection and calls
conn.commit() and conn.close() manually; change it to use a context manager
(with sqlite3.connect(temp_db) as conn:) so the connection is always closed (and
committed on successful exit) even if an exception occurs, and move the
conn.execute(...) and commit logic inside that with block while removing the
explicit conn.close() call.
| class TestDeleteSimulationRun: | ||
| def test_delete_run_returns_204_and_get_returns_404( | ||
| self, | ||
| simulation_client, | ||
| temp_db, | ||
| ): | ||
| """After delete, run is gone from list and GET returns 404.""" | ||
| client, _ = simulation_client | ||
| run_id = f"run-delete-{uuid.uuid4().hex[:12]}" | ||
| _insert_completed_run(temp_db, run_id=run_id, app_user_id=_MOCK_APP_USER_ID) | ||
|
|
||
| assert any( | ||
| r["run_id"] == run_id for r in client.get("/v1/simulations/runs").json() | ||
| ) | ||
|
|
||
| delete = client.delete(f"/v1/simulations/runs/{run_id}") | ||
| assert delete.status_code == 204 | ||
|
|
||
| get_run = client.get(f"/v1/simulations/runs/{run_id}") | ||
| assert get_run.status_code == 404 | ||
| assert get_run.json()["error"]["code"] == "RUN_NOT_FOUND" | ||
|
|
||
| listed = client.get("/v1/simulations/runs").json() | ||
| assert not any(r["run_id"] == run_id for r in listed) | ||
|
|
||
| def test_delete_run_returns_403_when_app_user_mismatch( | ||
| self, | ||
| simulation_client, | ||
| temp_db, | ||
| ): | ||
| """Cannot delete another user's run when runs.app_user_id is set.""" | ||
| client, _ = simulation_client | ||
| run_id = f"run-delete-403-{uuid.uuid4().hex[:12]}" | ||
| _insert_completed_run(temp_db, run_id=run_id, app_user_id="other-app-user-id") | ||
|
|
||
| delete = client.delete(f"/v1/simulations/runs/{run_id}") | ||
| assert delete.status_code == 403 | ||
| assert delete.json()["error"]["code"] == "RUN_FORBIDDEN" | ||
|
|
||
| get_run = client.get(f"/v1/simulations/runs/{run_id}") | ||
| assert get_run.status_code == 200 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good test coverage for happy path and authorization.
The tests correctly verify:
- Successful deletion returns 204 and subsequent GET returns 404
- Authorization check returns 403 when
app_user_iddoesn't match - The run remains accessible after a failed authorization check
Consider adding tests for:
- 404 when attempting to delete a non-existent run
- 400 for invalid
run_idformat (e.g., empty string)
Would you like me to generate additional test cases for the 404 (not found) and 400 (invalid ID) scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/api/test_simulation_run_delete.py` around lines 43 - 83, Add two new
test cases in the TestDeleteSimulationRun suite: one that calls DELETE
/v1/simulations/runs/{run_id} with a non-existent run_id (e.g.,
"run-delete-missing-...") and asserts a 404 status and error code
"RUN_NOT_FOUND", and another that calls DELETE with an invalid run_id (e.g.,
empty string or malformed ID) and asserts a 400 status and appropriate
validation error code (e.g., "INVALID_INPUT"); use the existing
simulation_client fixture and mirror the structure of
test_delete_run_returns_204_and_get_returns_404 and
test_delete_run_returns_403_when_app_user_mismatch to locate where to add the
new tests in the TestDeleteSimulationRun class and ensure GET behavior is not
affected.
| const handleDeleteRunClick = (): void => { | ||
| if ( | ||
| !window.confirm( | ||
| 'Delete this run permanently? This cannot be undone.', | ||
| ) | ||
| ) { | ||
| return; | ||
| } | ||
| void onDeleteRun(); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding error handling and loading state for the delete operation.
The current implementation uses fire-and-forget (void onDeleteRun()) without error handling or loading feedback. If the API call fails, users won't know the deletion failed, and they might navigate away thinking the run was deleted.
Consider:
- Adding a loading state to disable the button during deletion
- Catching errors and displaying feedback (toast, alert, etc.)
- Following the
is-prefixconvention for boolean state per project learnings
🛡️ Example improvement with loading/error handling
+ const [isDeleting, setIsDeleting] = useState(false);
+
const handleDeleteRunClick = (): void => {
if (
!window.confirm(
'Delete this run permanently? This cannot be undone.',
)
) {
return;
}
- void onDeleteRun();
+ setIsDeleting(true);
+ Promise.resolve(onDeleteRun())
+ .catch((err) => {
+ console.error('Failed to delete run:', err);
+ alert('Failed to delete run. Please try again.');
+ })
+ .finally(() => setIsDeleting(false));
};And update the button:
<button
type="button"
className="text-red-700 hover:text-red-900"
onClick={handleDeleteRunClick}
+ disabled={isDeleting}
>
- Delete run
+ {isDeleting ? 'Deleting...' : 'Delete run'}
</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/components/details/RunSummary.tsx` around lines 53 - 62, Replace the
fire-and-forget delete with an async handler that uses an is-prefix boolean
state (e.g., isDeleting) in the RunSummary component: set isDeleting = true
before calling onDeleteRun(), await onDeleteRun() inside a try/catch, show user
feedback on error (toast/alert) in the catch, and always set isDeleting = false
in finally; update handleDeleteRunClick to be async and use the new state, and
update the delete button to disable while isDeleting (and optionally show a
spinner) so users get loading/error feedback.
| const handleDeleteRun = useCallback(async (): Promise<void> => { | ||
| const runId: string | null = selectedRunId; | ||
| if (runId === null) { | ||
| return; | ||
| } | ||
| await deleteRun(runId); | ||
| clearRunDetailsAutoRetryState(runId); | ||
| setRuns((prev) => prev.filter((r) => r.runId !== runId)); | ||
| setRunConfigs((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[runId]; | ||
| return next; | ||
| }); | ||
| setFallbackTurns((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[runId]; | ||
| return next; | ||
| }); | ||
| setTurnsLoadingByRunId((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[runId]; | ||
| return next; | ||
| }); | ||
| setTurnsErrorByRunId((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[runId]; | ||
| return next; | ||
| }); | ||
| setRunDetailsLoadingByRunId((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[runId]; | ||
| return next; | ||
| }); | ||
| setRunDetailsErrorByRunId((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[runId]; | ||
| return next; | ||
| }); | ||
| turnsFetchInFlightRef.current.delete(runId); | ||
| lastTurnsFetchAttemptAtMsRef.current.delete(runId); | ||
| loadedTurnsRunIdsRef.current.delete(runId); | ||
| turnsRequestIdRef.current.delete(runId); | ||
| runDetailsRequestIdRef.current.delete(runId); | ||
| setSelectedRunId(null); | ||
| setSelectedTurn(null); | ||
| }, [selectedRunId, clearRunDetailsAutoRetryState]); |
There was a problem hiding this comment.
Unhandled errors will leave UI state unchanged while user believes deletion succeeded.
If deleteRun(runId) throws (network error, 403, etc.), the error propagates but the caller (RunSummary) currently ignores it. The user sees the confirmation prompt, clicks "OK", and if the API fails silently, the run remains visible but the user may not realize the deletion failed.
The state cleanup logic (lines 497-535) is thorough, but it only executes on success. Consider either:
- Handling errors in this hook and exposing error state
- Ensuring callers handle the thrown error (currently they don't)
For now, the UI wiring in RunSummary.tsx should catch and display errors, as noted in that file's review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/hooks/useSimulationPageState.ts` around lines 491 - 536, handleDeleteRun
currently calls deleteRun(runId) and only performs UI cleanup on success, so if
deleteRun throws the error propagates and callers ignore it; wrap the deleteRun
call in a try/catch inside handleDeleteRun (function name: handleDeleteRun) and
on catch populate a new error state keyed by runId (e.g.,
runDeletionErrorByRunId via setRunDeletionErrorByRunId) with the caught error
and then rethrow (or return a failure indicator) so the caller (RunSummary) can
display the error; do not perform the state-cleanup steps when deleteRun fails.
| "responses": { | ||
| "204": { | ||
| "description": "Successful Response" | ||
| }, | ||
| "422": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/HTTPValidationError" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Validation Error" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
OpenAPI spec is missing documented error responses for 403 and 404.
The backend route (simulation/api/routes/runs.py) returns 404 (RUN_NOT_FOUND), 403 (RUN_FORBIDDEN), and 400 (INVALID_RUN_ID) responses, but the OpenAPI spec only documents 204 and 422. This may cause confusion for API consumers and incomplete type generation for the UI client.
Consider adding explicit response documentation in the FastAPI route decorator to auto-generate complete OpenAPI specs:
responses={
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/openapi.json` around lines 1542 - 1555, The OpenAPI spec lacks documented
403/404/400 responses for the run endpoints; update the FastAPI route decorator
in simulation/api/routes/runs.py (the route that raises RUN_NOT_FOUND,
RUN_FORBIDDEN, and INVALID_RUN_ID) to include a responses={...} mapping that
explicitly documents 403 (RUN_FORBIDDEN), 404 (RUN_NOT_FOUND) and 400
(INVALID_RUN_ID) with appropriate descriptions and JSON schema/$ref to your
existing error schema (e.g., HTTPError or HTTPValidationError), then regenerate
the OpenAPI (ui/openapi.json) so the UI client types include these error
responses.
Made-with: Cursor
| def delete_run(self, run_id: str) -> None: | ||
| """Delete a run and all FK-dependent rows in one transaction.""" | ||
| with self._transaction_provider.run_transaction() as c: | ||
| deleted = delete_run_and_dependents(cast(sqlite3.Connection, c), run_id) |
There was a problem hiding this comment.
let's match the interface of the other functions here, i.e., taking a conn object and not doing the cast.
Made-with: Cursor
|
may be blocked by #285 (to refactor exception handling) |
| @@ -0,0 +1,44 @@ | |||
| """Transactional deletion of a simulation run and all dependent SQLite rows. | |||
There was a problem hiding this comment.
also probably should just be a SQLite adapter, following the patterns that other logic uses.
| @@ -0,0 +1,34 @@ | |||
| """Delete persisted simulation run (API layer).""" | |||
There was a problem hiding this comment.
should this just be a part of simulation/core/services/command_service.py?
Overview
Users need to remove simulation runs from storage and from the UI. Today the API exposes list/get for runs but no delete; the Run Summary header only has Export Run. This work adds a transactional DB delete, a
DELETE /v1/simulations/runs/{run_id}endpoint, a thin API client, and a Delete run control with confirmation. On success, the client clearsselectedRunId/selectedTurnlike Start New Run so the app returns to the default start screen.Problem / motivation
There was no way to remove a persisted run from SQLite or from the sidebar/details UI without manual DB edits.
Solution
Implement ordered transactional deletes for all run-scoped tables, expose a DELETE route with
RUN_NOT_FOUND/RUN_FORBIDDEN/INVALID_RUN_IDerror codes, regenerate OpenAPI, and wire a confirm + delete flow on the Run Summary that prunes local caches and clears selection.Happy Flow
selectedRunIdis set (useSimulationPageState).RunSummarywith Export Run and Delete run (same header row).window.confirm.DELETE /v1/simulations/runs/{run_id}viadeleteRuninui/lib/api/simulation.ts.run_id, enforces ownership whenruns.app_user_idis set, deletes dependent rows in one transaction, then therunsrow. Returns 204 on success; 404RUN_NOT_FOUND; 403RUN_FORBIDDEN; 400INVALID_RUN_ID.runsstate, prunes per-run caches, then setsselectedRunIdandselectedTurntonulllikehandleStartNewRun.StartScreenViewagain (isStartScreen).Data Flow
RunSummary→onDeleteRun(context) →handleDeleteRun→deleteRun(runId)→DELETE /v1/simulations/runs/{run_id}→delete_simulation_run→SimulationEngine.delete_run→SQLiteRunRepository.delete_run→delete_run_and_dependents(ordered SQL DELETEs) → 204.Changes
db/services/run_deletion_sqlite.py: FK-safe delete sequence for all run-scoped tablesdb/repositories/interfaces.py,db/repositories/run_repository.py:delete_runon repositorysimulation/core/engine.py:delete_runsimulation/api/errors.py:ApiRunForbiddenErrorsimulation/api/services/run_delete_service.py: ownership + delete orchestrationsimulation/api/routes/runs.py:DELETE /v1/simulations/runs/{run_id}tests/api/test_simulation_run_delete.py: API + repo coverageui/lib/api/simulation.ts:deleteRunui/hooks/useSimulationPageState.ts:handleDeleteRuncache pruningui/components/run-detail/RunDetailContext.tsx,ui/app/page.tsx,ui/components/details/DetailsPanel.tsx,ui/components/details/RunSummary.tsx: UI wiringui/openapi.json,ui/types/api.generated.ts: regenerated from FastAPIManual Verification
uv run pytest tests/api/ -q— all pass (75 passed, 4 skipped)cd ui && npm run lint:all— passDISABLE_AUTH=1/NEXT_PUBLIC_DISABLE_AUTH=true; start API and UI; select run → Summary → Delete run → cancel leaves run; confirm removes run and shows start screen.State
Made with Cursor
Summary by CodeRabbit
Release Notes