Skip to content

ALL-202: Add delete action for curation rows in the curation review table#213

Merged
christabone merged 4 commits intomainfrom
all-202
Apr 16, 2026
Merged

ALL-202: Add delete action for curation rows in the curation review table#213
christabone merged 4 commits intomainfrom
all-202

Conversation

@christabone
Copy link
Copy Markdown
Contributor

TL;DR

Adds row-level deletion for prepared curation candidates, including confirmation UI, backend persistence, and cleanup of dependent candidate state.

Summary

  • Added a new DELETE endpoint for candidate removal in curation workspace sessions.
  • Reused and refactored existing prepared-session child cleanup to delete per-candidate dependent rows for drafts, evidence anchors, validation snapshots, and candidate-level action logs.
  • Added candidate_deleted action-log typing and response payload support.
  • Wired delete controls through EntityTagRow, EntityTagTable, and CurationWorkspacePage with explicit confirm dialog and autosave-before-delete behavior.
  • Added backend and frontend tests for deletion confirmation, service delegation, workspace persistence, and dependent-row cleanup.

Test Plan

  • docker compose -f docker-compose.test.yml run --rm backend-unit-tests bash -lc "python -m pytest tests/unit/api/test_curation_workspace_api.py tests/unit/lib/curation_workspace/test_session_service.py -v --tb=short"
  • docker compose -f docker-compose.test.yml run --rm backend-integration-tests bash -lc "python -m pytest tests/integration/test_curation_workspace_sessions_api.py -v --tb=short"
  • cd frontend && npm ci && npm run test -- --run src/features/curation/entityTable/EntityTagRow.test.tsx src/features/curation/entityTable/EntityTagTable.test.tsx src/pages/CurationWorkspacePage.test.tsx

@claude
Copy link
Copy Markdown

claude bot commented Apr 14, 2026

PR Review: ALL-202 — Add delete action for curation rows in the curation review table

Summary

This PR adds row-level deletion for curation candidates in the workspace. Changes span the full stack:

  • Backend: New DELETE /sessions/{session_id}/candidates/{candidate_id} endpoint, delete_candidate service function with child-row cleanup (drafts, evidence anchors, validation snapshots, action logs), and a CANDIDATE_DELETED action type.
  • Backend refactor: Extracted _delete_session_validation_snapshots and _delete_candidate_children helpers from _clear_prepared_session_children, reused by both the new delete and the existing session-clear paths.
  • Frontend: Delete button on EntityTagRow, confirmation dialog in EntityTagTable, deleteCurationCandidate service call, and workspace-level handleDeleteTag with autosave-before-delete.
  • Tests: Unit and integration tests covering the API delegation, service logic (child cleanup, session state updates, cursor repositioning), confirmation dialog behavior, and full workspace page flow.

BLOCKING Issues

None found.

  • No fallback patterns: No silent error swallowing, backwards-compatibility shims, or graceful degradation patterns introduced.
  • No dead code: The refactoring of _clear_prepared_session_children cleanly replaces inline SQL with calls to the new shared helpers. All new types, functions, and imports are referenced.

Other Issues

No critical or warning-level issues.

Suggestions (non-blocking):

  1. session_service.py:3292-3295 — The in-memory action_log_entries filter uses an and condition (candidate_id != X and draft_id != Y) which is broader than the DB delete in _delete_candidate_children (which only filters on candidate_id). In practice these are equivalent because a draft maps 1:1 to a candidate, but the asymmetry could confuse future readers. Consider aligning the in-memory filter to match the DB delete (filter on candidate_id only).

  2. session_service.py:3325-3329build_action_log_entry(action_log_row) is checked for None, but the input is guaranteed non-None (just created two lines above), and build_action_log_entry only returns None for None input. The check is harmless but tests an impossible state. Minor — no action needed.

What looks good

  • Clean extraction of _delete_candidate_children / _delete_session_validation_snapshots avoids duplication between single-candidate delete and full session clear.
  • _current_candidate_id_after_delete has clear, predictable cursor-repositioning logic (keep current > next by order > previous by order).
  • Confirmation dialog properly prevents dismiss during pending delete (deletePending guard on close/cancel).
  • Autosave flush before delete prevents data loss on the active draft.
  • Comprehensive test coverage across all layers: API delegation, service unit tests with DB assertions, integration test with child-row verification, frontend dialog confirmation flow, and full page-level workspace test.

Overall Assessment

Clean, well-structured PR. The feature is complete end-to-end with appropriate test coverage. No blocking issues. Approved.


Need help? Comment @claude followed by your request.

@christabone
Copy link
Copy Markdown
Contributor Author

@claude Please review these recent changes.

  • Updated PR head: 0989c26
  • Context: addressed prior PR feedback

@claude
Copy link
Copy Markdown

claude bot commented Apr 14, 2026

Re-review: ALL-202 — Add delete action for curation rows (after 0989c26)

Previous feedback status

The new commit 0989c26 ("Align curation delete action-log filtering") addresses both suggestions from my prior review:

  1. Action-log in-memory filter alignment — Removed the and action_log_entry.draft_id != candidate.draft.id clause, so the in-memory filter now matches the DB delete (filter on candidate_id only). Resolved.
  2. Unnecessary None check on build_action_log_entry — Removed the if action_log_entry is None guard and its associated HTTPException. Resolved.

BLOCKING Issues

None found.

  • No fallback patterns, silent error swallowing, or backwards-compatibility shims introduced.
  • No dead code — all new imports, types, functions, and exports are referenced. The refactoring of _clear_prepared_session_children cleanly delegates to _delete_session_validation_snapshots and _delete_candidate_children with no leftover inline SQL.

Other Issues

No critical or warning-level issues.

Overall Assessment

Clean PR, both prior suggestions addressed. The feature is complete end-to-end with good test coverage across all layers. Approved.


Need help? Comment @claude followed by your request.

# Conflicts:
#	backend/tests/unit/api/test_auth_api.py
#	backend/tests/unit/api/test_auth_api_endpoints.py
#	backend/tests/unit/api/test_chat_execute_flow_endpoint.py
#	backend/tests/unit/api/test_flow_evidence_export.py
#	backend/tests/unit/api/test_flows_api.py
#	backend/tests/unit/api/test_flows_endpoints.py
@christabone
Copy link
Copy Markdown
Contributor Author

@claude Please review these recent changes.

  • Updated PR head: d5931ac
  • Context: addressed prior PR feedback

@claude
Copy link
Copy Markdown

claude bot commented Apr 16, 2026

Re-review: ALL-202 — Add curation row delete action (after d5931ac)

Previous feedback status

Both suggestions from my first review were addressed in 0989c26 (confirmed in my second review). No outstanding issues carried forward.

New commits since last review

Commit Description Assessment
30e35df Fix delete action follow-up CI regressions Test expectation in useEvidenceNavigation.test.ts updated to use expect.objectContaining() with subsection_title: null, aligning with the EvidenceAnchor type that includes this optional field. Correct fix.
d5931ac Merge remote-tracking branch origin/main into all-202 Merge commit bringing in main (including ALL-204 reranking changes). No conflict artifacts in the diff.

BLOCKING Issues

None found.

  • No fallback patterns: The ?? null usages (e.g., response.session.current_candidate_id ?? null, state.tags.find(...) ?? null) handle genuinely optional/nullable values, not silent degradation. The catch in EntityTagTable.handleConfirmDelete logs the error and lets the page-level setTableError (set before re-throw in CurationWorkspacePage.handleDeleteTag) surface it to the user — not error swallowing.
  • No dead code: All imports, types, functions, and helpers introduced by this PR are referenced. The refactored _clear_prepared_session_children cleanly delegates to the shared _delete_session_validation_snapshots and _delete_candidate_children helpers with no leftover inline SQL.

Other Issues

No critical, warning, or suggestion-level issues.

Overall Assessment

Clean merge of main with a minor CI test fix. No new concerns introduced. The full PR remains well-structured with comprehensive test coverage across all layers. Approved.


Need help? Comment @claude followed by your request.

@christabone christabone merged commit 2458e4f into main Apr 16, 2026
16 checks passed
@christabone christabone deleted the all-202 branch April 16, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant