Skip to content

Conversation

@Gkrumbach07
Copy link
Collaborator

Summary

Fixes a race condition that occurred when deleting multiple projects in quick succession.

Problem

When deleting two projects rapidly:

  1. First deletion invalidates projectKeys.lists()
  2. This triggers a refetch of the project list from backend
  3. Second deletion happens before the refetch completes
  4. State becomes inconsistent, causing "project not found" errors

Solution

Implemented optimistic updates with proper error handling in the useDeleteProject hook:

  • Optimistic updates: Projects are immediately removed from the cache before the API call completes
  • Cancel in-flight queries: Prevents race conditions by canceling any ongoing refetches
  • Proper rollback: Restores previous state on errors (except 404s, which are fine)
  • Handles both response types: Works with paginated and legacy list query responses
  • Updates totalCount: Correctly decrements count in paginated responses
  • Idempotent deletion: Silently ignores "not found" errors (project already deleted)

Testing

  • ✅ Frontend builds successfully with no TypeScript errors
  • ✅ Linting passes without issues
  • ✅ Projects are removed from UI immediately on deletion
  • ✅ Multiple rapid deletions no longer cause race conditions
  • ✅ Error handling properly restores state on non-404 failures

Files Changed

  • components/frontend/src/services/queries/use-projects.ts

Related Issue

https://issues.redhat.com/browse/RHOAIENG-39096

Fixes a race condition that occurred when deleting multiple projects in
quick succession. The issue was caused by React Query cache invalidation
triggering a refetch while a second deletion was in progress, leading to
inconsistent state and "project not found" errors.

Changes:
- Implement optimistic updates in useDeleteProject hook
- Cancel in-flight queries before applying optimistic updates
- Add proper rollback mechanism on errors (except 404s)
- Handle both paginated and legacy list query responses
- Update totalCount in paginated responses when removing projects
- Silently ignore "not found" errors during deletion (idempotent)

Testing:
- Frontend builds successfully with no TypeScript errors
- Linting passes without issues
- Projects are removed from UI immediately on deletion
- Multiple rapid deletions no longer cause race conditions
- Error handling properly restores state on non-404 failures

Issue: https://issues.redhat.com/browse/RHOAIENG-39096

Co-Authored-By: Claude (claude-sonnet-4-5) <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 28, 2026

Claude Code Review

Summary

This PR fixes a race condition in project deletion by implementing optimistic updates. The issue occurred when multiple projects were deleted quickly - cache invalidation triggered refetches that conflicted with subsequent deletions. The solution is well-implemented and follows React Query best practices.

Issues by Severity

Major Issues

  1. Type Safety (line 126): Unsafe type assertion as { items: Project[]; totalCount?: number }. Recommend using a type guard function instead.
  2. Inconsistent Pattern: Only this mutation uses optimistic updates. Consider documenting why or applying to other delete operations like useDeleteSession.

Minor Issues

  1. Missing Error Logging: No console.error for non-404 failures, making debugging difficult
  2. No Tests: Missing unit tests for this race condition fix
  3. String-based Error Detection: Fragile error checking via string matching
  4. Complex Snapshot Logic: Map with JSON serialization adds complexity

Positive Highlights

✅ Excellent problem identification and solution
✅ Proper React Query optimistic update pattern
✅ Handles both paginated and legacy responses
✅ Idempotent deletion (404s ignored correctly)
✅ Good inline documentation
✅ Updates totalCount correctly

Recommendations

Before Merge:

  • Add type guard for response check
  • Add error logging for non-404 errors

Follow-up:

  • Add unit tests
  • Document why this uses optimistic updates
  • Check if useDeleteSession needs similar fix

Verdict

APPROVED with recommendations

Solid fix for a real issue. Type safety and logging improvements recommended but don't block merge. Great work!


Generated by Claude Code Review


🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

  1. CLAUDE.md - Master project instructions, development standards
  2. backend-development.md - Go backend, K8s integration patterns
  3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
  4. security-standards.md - Auth, RBAC, token handling
  5. k8s-client-usage.md - User token vs service account patterns
  6. error-handling.md - Consistent error patterns
  7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.

@bobbravo2 bobbravo2 added this to the v0.0.20 milestone Jan 30, 2026
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.

2 participants