Skip to content

Assert list_state is selected in Detail movement methods#52

Merged
ooloth merged 4 commits into
mainfrom
claude/issue-48
May 16, 2026
Merged

Assert list_state is selected in Detail movement methods#52
ooloth merged 4 commits into
mainfrom
claude/issue-48

Conversation

@ooloth
Copy link
Copy Markdown
Owner

@ooloth ooloth commented May 15, 2026

Adds precondition assertions to all four Detail-arm movement methods (move_up, move_down, move_page_up, move_page_down) so that an unselected list_state in the Detail screen panics immediately rather than silently computing movement from index 0.

Since apply_enter_action always calls ds.select(Some(0)) when entering the detail screen, an unselected state here is a programmer error, not a recoverable case.

Closes #48


Generated by Claude Code

Adds precondition assertions to all four Detail-arm movement methods
(move_up, move_down, move_page_up, move_page_down) so that an unselected
list_state in the Detail screen panics immediately rather than silently
computing movement from index 0.

Closes #48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds precondition assertions to App’s Screen::Detail movement methods to fail fast when DetailView.list_state is unexpectedly unselected, instead of silently defaulting to index 0.

Changes:

  • Added assert!(view.list_state.selected().is_some(), ...) to move_up, move_down, move_page_up, and move_page_down in the Screen::Detail arms.
  • Intended to treat an unselected Detail list state as a programmer error (per issue #48).
Comments suppressed due to low confidence (2)

ui/tui/src/state/mod.rs:82

  • Same issue as in move_up: apply_enter_action leaves DetailView.list_state unselected when item_count == 0 (ui/tui/src/state/update.rs:297-306). With the new unconditional assert, move_down will panic instead of being a no-op for an empty detail list. Consider asserting only when len > 0, or ensure the detail screen always selects an index.
                assert!(
                    view.list_state.selected().is_some(),
                    "list_state must be selected in Detail screen"
                );
                let sel = view.list_state.selected().unwrap_or(0);

ui/tui/src/state/mod.rs:120

  • This assertion will also fire for an empty detail list, but apply_enter_action can construct Screen::Detail with list_state left unselected when item_count == 0 (ui/tui/src/state/update.rs:297-306). If empty groups are allowed, move_page_up should probably no-op rather than panic; otherwise make the invariant true by always selecting when entering Detail.
                assert!(
                    view.list_state.selected().is_some(),
                    "list_state must be selected in Detail screen"
                );
                let sel = view.list_state.selected().unwrap_or(0);
                view.list_state.select(Some(sel.saturating_sub(PAGE)));

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ui/tui/src/state/mod.rs
ooloth added 3 commits May 15, 2026 19:46
The file was removed in aa09a73 when it was moved to dotfiles, but
workflows/src/implement.rs still references it via include_str! at
compile time, breaking the build. Restoring it as a short-term fix
until implement.rs is updated to read the prompt from a runtime path.
The four Detail-arm movement methods (move_up, move_down,
move_page_up, move_page_down) had no test coverage. Adds a
#[cfg(test)] module in state/mod.rs with eight rstest cases covering
interior movement and boundary clamping for each method.

These tests were surfaced by the copilot review of PR #52, which
questioned whether the new precondition asserts could fire on real
user input. The asserts are correct — an empty Detail list is
unreachable in production — but the gap in coverage made the asserts
unverifiable by the suite.
@ooloth ooloth merged commit f463b50 into main May 16, 2026
1 check failed
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.

Assert list_state is selected in Detail movement methods instead of silently defaulting to 0

3 participants