Fix race condition causing addressed items to briefly reappear (#67)

## Summary
- Fix race condition where addressed items with hide-addressed filter
active would disappear, briefly reappear (~100-200ms), then disappear
again
- The race occurred when success response cleared pending state before a
stale jobs refresh arrived with old data
- Now pending state is only cleared when jobs refresh confirms the
server has the updated state

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: wesm

cmd/roborev/tui.go

@@ -1399,8 +1399,24 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 			m.jobs = msg.jobs
 		}
 
-		// Apply any pending addressed changes to prevent flash during race condition
-		// (user pressed 'a' but server response arrived before addressed update completed)
+		// Clear pending addressed states that server has confirmed (data matches pending)
+		// This must happen before re-applying, so we can check the raw server state
+		for jobID, pending := range m.pendingAddressed {
+			for i := range m.jobs {
+				if m.jobs[i].ID == jobID {
+					// Check if server state matches pending state
+					// Treat nil as false (unaddressed) for comparison
+					serverState := m.jobs[i].Addressed != nil && *m.jobs[i].Addressed
+					if serverState == pending.newState {
+						delete(m.pendingAddressed, jobID)
+					}
+					break
+				}
+			}
+		}
+
+		// Apply any remaining pending addressed changes to prevent flash during race
+		// condition (server data is stale, from request sent before update completed)
 		for i := range m.jobs {
 			if pending, ok := m.pendingAddressed[m.jobs[i].ID]; ok {
 				newState := pending.newState
@@ -1557,13 +1573,15 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 			}
 			// Stale error responses are silently ignored
 		} else {
-			// Success: clear pending state if current
-			if isCurrentRequest {
-				if msg.jobID > 0 {
-					delete(m.pendingAddressed, msg.jobID)
-				} else if msg.reviewID > 0 {
-					delete(m.pendingReviewAddressed, msg.reviewID)
-				}
+			// Success handling differs by type:
+			// - For jobs (jobID > 0): don't clear here. Let the jobs refresh handler
+			//   clear it when server data confirms the update. This prevents a race
+			//   where we clear pending, then a stale jobs response arrives and briefly
+			//   shows the old state.
+			// - For review-only (no jobID): clear immediately. The race condition doesn't
+			//   apply because pendingReviewAddressed isn't affected by jobs refresh.
+			if isCurrentRequest && msg.jobID == 0 && msg.reviewID > 0 {
+				delete(m.pendingReviewAddressed, msg.reviewID)
 			}
 		}
 

cmd/roborev/tui_test.go

@@ -4240,7 +4240,7 @@ func TestTUIPendingAddressedNotClearedByStaleResponse(t *testing.T) {
 	}
 }
 
-func TestTUIPendingAddressedClearedByMatchingResponse(t *testing.T) {
+func TestTUIPendingAddressedNotClearedOnSuccess(t *testing.T) {
 	m := newTuiModel("http://localhost")
 
 	addressedFalse := false
@@ -4251,21 +4251,83 @@ func TestTUIPendingAddressedClearedByMatchingResponse(t *testing.T) {
 	// User toggles addressed to true
 	m.pendingAddressed[1] = pendingState{newState: true, seq: 1}
 
-	// Response comes back for the correct toggle to true
-	matchingMsg := tuiAddressedResultMsg{
+	// Success response comes back
+	successMsg := tuiAddressedResultMsg{
 		jobID:    1,
 		oldState: false,
-		newState: true, // This response matches what we requested
-		seq:      1,    // Matches pending seq
+		newState: true,
+		seq:      1,
 		err:      nil,
 	}
 
-	updated, _ := m.Update(matchingMsg)
+	updated, _ := m.Update(successMsg)
 	m2 := updated.(tuiModel)
 
-	// pendingAddressed should be cleared because newState matches
+	// pendingAddressed should NOT be cleared on success - it waits for jobs refresh
+	// to confirm the update. This prevents race condition where stale jobs response
+	// arrives after success and briefly shows old state.
+	if _, ok := m2.pendingAddressed[1]; !ok {
+		t.Error("pendingAddressed should NOT be cleared on success response")
+	}
+}
+
+func TestTUIPendingAddressedClearedByJobsRefresh(t *testing.T) {
+	m := newTuiModel("http://localhost")
+
+	addressedFalse := false
+	m.jobs = []storage.ReviewJob{
+		{ID: 1, Status: storage.JobStatusDone, Addressed: &addressedFalse},
+	}
+
+	// User toggles addressed to true
+	m.pendingAddressed[1] = pendingState{newState: true, seq: 1}
+
+	// Jobs refresh arrives with server data confirming the update
+	addressedTrue := true
+	jobsMsg := tuiJobsMsg{
+		jobs: []storage.ReviewJob{
+			{ID: 1, Status: storage.JobStatusDone, Addressed: &addressedTrue},
+		},
+	}
+
+	updated, _ := m.Update(jobsMsg)
+	m2 := updated.(tuiModel)
+
+	// pendingAddressed should be cleared because server data matches pending state
 	if _, ok := m2.pendingAddressed[1]; ok {
-		t.Error("pendingAddressed should be cleared when response newState matches pending state")
+		t.Error("pendingAddressed should be cleared when jobs refresh confirms update")
+	}
+}
+
+func TestTUIPendingAddressedNotClearedByStaleJobsRefresh(t *testing.T) {
+	m := newTuiModel("http://localhost")
+
+	addressedFalse := false
+	m.jobs = []storage.ReviewJob{
+		{ID: 1, Status: storage.JobStatusDone, Addressed: &addressedFalse},
+	}
+
+	// User toggles addressed to true
+	m.pendingAddressed[1] = pendingState{newState: true, seq: 1}
+
+	// Stale jobs refresh arrives with old data (from request sent before update)
+	staleJobsMsg := tuiJobsMsg{
+		jobs: []storage.ReviewJob{
+			{ID: 1, Status: storage.JobStatusDone, Addressed: &addressedFalse}, // Still false!
+		},
+	}
+
+	updated, _ := m.Update(staleJobsMsg)
+	m2 := updated.(tuiModel)
+
+	// pendingAddressed should NOT be cleared because server data doesn't match
+	if _, ok := m2.pendingAddressed[1]; !ok {
+		t.Error("pendingAddressed should NOT be cleared when jobs refresh has stale data")
+	}
+
+	// Job should still show as addressed (pending state re-applied)
+	if m2.jobs[0].Addressed == nil || !*m2.jobs[0].Addressed {
+		t.Error("Job should still show as addressed due to pending state")
 	}
 }
 
@@ -4632,3 +4694,93 @@ func TestTUIQueueNoScrollIndicatorPads(t *testing.T) {
 		t.Errorf("Queue with few jobs should maintain height, got %d lines for height %d", len(lines), m.height)
 	}
 }
+
+func TestTUIPendingReviewAddressedClearedOnSuccess(t *testing.T) {
+	// pendingReviewAddressed (for reviews without jobs) should be cleared on success
+	// because the race condition only affects jobs list refresh, not review-only items
+	m := newTuiModel("http://localhost")
+
+	// Track a review-only pending state (no job ID)
+	m.pendingReviewAddressed[42] = pendingState{newState: true, seq: 1}
+
+	// Success response for review-only (jobID=0, reviewID=42)
+	successMsg := tuiAddressedResultMsg{
+		jobID:      0, // No job - this is review-only
+		reviewID:   42,
+		reviewView: true,
+		oldState:   false,
+		newState:   true,
+		seq:        1,
+		err:        nil,
+	}
+
+	updated, _ := m.Update(successMsg)
+	m2 := updated.(tuiModel)
+
+	// pendingReviewAddressed SHOULD be cleared on success (no race condition for review-only)
+	if _, ok := m2.pendingReviewAddressed[42]; ok {
+		t.Error("pendingReviewAddressed should be cleared on success for review-only items")
+	}
+}
+
+func TestTUIPendingAddressedClearsWhenServerNilMatchesFalse(t *testing.T) {
+	// When pending newState is false and server Addressed is nil,
+	// treat nil as false and clear the pending state
+	m := newTuiModel("http://localhost")
+
+	// Job with nil Addressed (e.g., partial payload or non-done status that became done)
+	m.jobs = []storage.ReviewJob{
+		{ID: 1, Status: storage.JobStatusDone, Addressed: nil},
+	}
+
+	// User had toggled to false (unaddressed)
+	m.pendingAddressed[1] = pendingState{newState: false, seq: 1}
+
+	// Jobs refresh arrives with nil Addressed (should match false)
+	jobsMsg := tuiJobsMsg{
+		jobs: []storage.ReviewJob{
+			{ID: 1, Status: storage.JobStatusDone, Addressed: nil},
+		},
+	}
+
+	updated, _ := m.Update(jobsMsg)
+	m2 := updated.(tuiModel)
+
+	// pendingAddressed should be cleared because nil == false matches newState=false
+	if _, ok := m2.pendingAddressed[1]; ok {
+		t.Error("pendingAddressed should be cleared when server nil matches pending false")
+	}
+}
+
+func TestTUIPendingAddressedNotClearsWhenServerNilMismatchesTrue(t *testing.T) {
+	// When pending newState is true and server Addressed is nil,
+	// do NOT clear (nil != true)
+	m := newTuiModel("http://localhost")
+
+	m.jobs = []storage.ReviewJob{
+		{ID: 1, Status: storage.JobStatusDone, Addressed: nil},
+	}
+
+	// User toggled to true (addressed)
+	m.pendingAddressed[1] = pendingState{newState: true, seq: 1}
+
+	// Jobs refresh arrives with nil Addressed (doesn't match true)
+	jobsMsg := tuiJobsMsg{
+		jobs: []storage.ReviewJob{
+			{ID: 1, Status: storage.JobStatusDone, Addressed: nil},
+		},
+	}
+
+	updated, _ := m.Update(jobsMsg)
+	m2 := updated.(tuiModel)
+
+	// pendingAddressed should NOT be cleared because nil != true
+	if _, ok := m2.pendingAddressed[1]; !ok {
+		t.Error("pendingAddressed should NOT be cleared when server nil mismatches pending true")
+	}
+
+	// Job should show as addressed due to pending state re-applied
+	if m2.jobs[0].Addressed == nil || !*m2.jobs[0].Addressed {
+		t.Error("Job should show as addressed due to pending state")
+	}
+}