Skip to content

Fix incident selection synchronization issues#138

Merged
clcollins merged 1 commit intomainfrom
fix_incident_selection
Mar 13, 2026
Merged

Fix incident selection synchronization issues#138
clcollins merged 1 commit intomainfrom
fix_incident_selection

Conversation

@clcollins
Copy link
Owner

Problem Summary

The srep binary had multiple bugs where selectedIncident didn't properly sync with the highlighted table row when navigating or returning from views. This caused user actions to operate on the wrong incident ID.

Root Causes Fixed

1. Missing re-sync after Escape

When returning from incident view (pressing Escape), the code cleared selectedIncident but didn't re-establish which incident is now under the cursor.

2. Pointer aliasing to slice elements

Using &m.incidentList[i] created pointers that became invalid when the slice was reallocated.

3. Race condition in async message handling

Late-arriving gotIncidentMsg could overwrite selectedIncident when user had navigated away.

4. Stale cache with no TTL

Cached incident data never invalidated based on age or list updates.

5. Boundary condition when viewing

Scrolling table out of bounds while viewing didn't clear selection properly.

Changes Made

Critical Synchronization Fixes

1. Add re-sync after returning from incident view (msgHandlers.go:400)

  • Call syncSelectedIncidentToHighlightedRow() after Escape key
  • Ensures selection matches current table cursor position

2. Fix pointer aliasing (model.go:219-221)

  • Copy incident struct instead of storing pointer to slice element
  • Prevents dangling pointers when incidentList is reallocated

3. Add race condition guard (tui.go:246-253)

  • Use explicit boolean check in gotIncidentMsg handler
  • Prevents late messages from overwriting current selection

4. Invalidate cache on timestamp changes (tui.go:434-453)

  • Compare LastStatusChangeAt to detect stale cache entries
  • Invalidate when incidents update, not just when they disappear

5. Improve cache freshness detection (model.go:200-241)

  • Compare cached timestamp with list data
  • Use fresh data for basic info while preserving expensive API data
  • Create copies to avoid pointer issues

6. Fix boundary conditions (model.go:177-184)

  • Clear selection when scrolling out of bounds
  • Add debug logging

7. Add defensive nil check (tui.go:526-530)

  • Check selectedIncident before accessing in loginMsg handler

Additional Improvements

Refactored incident selection logic

  • getHighlightedIncident()syncSelectedIncidentToHighlightedRow()
  • Single source of truth for incident selection
  • Called after all navigation events (Up/Down/Top/Bottom/Escape)
  • Handles caching, staleness detection, and pointer safety

UI refinements

  • Improved help layout (3 columns, help key at top)
  • Action log table styling matches main table
  • Better window size handling for small terminals

Tests Added

  • TestEscapeKeySyncsToHighlightedRow: Verifies Escape key re-sync behavior
  • TestSelectedIncidentSurvivesListUpdate: Verifies pointer aliasing fix

Testing

All tests pass:

PASS
ok  	github.com/clcollins/srepd/pkg/tui	0.172s

Verification

Manual testing should verify:

  1. Escape key returns to table and selects correct incident
  2. Navigation (Up/Down) updates selection correctly
  3. Actions (open, login, note) operate on correct incident
  4. Cache invalidates when incident status changes
  5. No panics when scrolling past table bounds

Fixes multiple reported issues with incident selection bugs.

This commit addresses multiple bugs where selectedIncident didn't properly
sync with the highlighted table row when navigating or returning from views,
causing user actions to operate on the wrong incident ID.

## Critical Fixes

1. **Add re-sync after Escape key** (msgHandlers.go:400)
   - Call syncSelectedIncidentToHighlightedRow() when returning from incident view
   - Ensures selectedIncident matches the current table cursor position

2. **Fix pointer aliasing** (model.go:219-221)
   - Copy incident struct instead of storing pointer to slice element
   - Prevents dangling pointers when incidentList is reallocated

3. **Add race condition guard** (tui.go:246-253)
   - Use explicit boolean check in gotIncidentMsg handler
   - Prevents late-arriving messages from overwriting when user navigated away

4. **Invalidate cache on timestamp changes** (tui.go:434-453)
   - Compare LastStatusChangeAt to detect stale cache entries
   - Invalidate cache when incidents update, not just when they disappear

5. **Improve cache freshness detection** (model.go:200-241)
   - Compare cached incident timestamp with list data
   - Use fresh list data for basic info while preserving expensive API data
   - Create copies to avoid pointer issues

6. **Fix boundary conditions** (model.go:177-184)
   - Clear selection when scrolling out of bounds regardless of viewing state
   - Add debug logging for troubleshooting

7. **Add defensive nil check** (tui.go:526-530)
   - Check selectedIncident before accessing in loginMsg handler

## Additional Improvements

- Refactored getHighlightedIncident() into syncSelectedIncidentToHighlightedRow()
  - Now a single source of truth for incident selection
  - Always called after navigation events (Up/Down/Top/Bottom/Escape)
  - Handles caching, staleness detection, and pointer safety

- Added comprehensive tests
  - TestEscapeKeySyncsToHighlightedRow: Verifies Escape key re-sync
  - TestSelectedIncidentSurvivesListUpdate: Verifies pointer aliasing fix

- UI refinements
  - Improved help layout (3 columns, help at top)
  - Action log table styling matches main table
  - Better window size handling

All tests pass successfully.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@clcollins clcollins merged commit 7fd18d2 into main Mar 13, 2026
1 of 2 checks passed
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