a73x

f3d6c51a

One table for the dashboard's keys, and an issue side that can answer

a73x   2026-08-14 06:17

Commit message
One table for the dashboard's keys, and an issue side that can answer

Key handling was 57 `KeyCode::` arms across two files, nested by mode and
pane, with the footer describing them written separately as string
literals. Nothing connected the two, so nothing could notice when they
disagreed. Both failure modes were live: `c` meant comment on a patch,
Event History on an issue, and — gated on `pane == Pane::Detail` —
nothing at all one pane over; and the footer advertised `c:events` for a
key that had by then become the comment key.

The bindings are now data in `src/tui/keys.rs`: context -> key -> action,
where context is the (ViewMode, Pane) pair that already governed dispatch
with ListMode folded in where it genuinely changes what a key means.
Dispatch is one lookup. A key the context does not bind comes back as
`None` and the reader is told, rather than falling into the same silent
`_ => Continue` that "bound but gated out" fell into.

Everything else derives from the table:

- The footer is generated, so help cannot drift from the bindings. It
  leads with `?:keys` and `q:quit`, because one line cannot hold this
  surface and its first duty is to name the key that can.
- `?` opens an overlay listing the current pane's bindings — the
  discoverability fix for keys (`o`, `w`, `Ctrl-E`, `u`, `P`) that were
  never advertised anywhere.
- Tests assert what the old structure could not express: no context binds
  a key twice, no bound context is unreachable, no context is a room with
  no door, every pair has its partner. Both invariants were shown to bite
  by planting a violation.

`ViewMode::CommitList`/`CommitDetail` and `KeyAction::OpenCommitBrowser`
are now `EventHistory`/`EventDetail` and `OpenEventHistory`, after the
pane they have always drawn.

The dashboard can now comment on an issue (`c`, from either issue pane)
through the same `$EDITOR` suspend the patch side uses, against
`issue::comment` — the same call `git-collab issue comment` makes. Event
History moves to `e`: comment has the stronger claim on `c` on a surface
that is a review surface by decision, and `e` is unbound everywhere and
matches the pane's own name.

Triage: close and reopen belong here, labels do not. `C` closes with a
reason composed in `$EDITOR` — an empty buffer abandons it, because this
DAG is append-only — and reopens a closed issue. Closing is the terminal
move of the review loop, and withholding it meant quitting the dashboard
to run one command. A label is free text from an open set, so its input
is a completion problem the dashboard cannot solve; hand-typing them from
a modal line is how a repository ends up with `bug`, `Bug` and `bgu`.

Found while wiring reopen: the `a` filter offered "closed" and "all" for
issues it could never show. Closing archives the ref and the dashboard
loaded with `list_issues`, which excludes archived — so two of the three
filter settings were structurally empty. `issue list -a` and the web UI
have always used the `_with_archived` listers; the dashboard now does
too, and `visible_issues` does the narrowing.

Two of the new pty tests waited on needles that a cell-diffing redraw can
never emit, and so burned the 20s pty timeout in silence while still
passing. Both now wait on text drawn into blank cells; the file went from
20.7s to 3.1s.

Fixes 02d3eb34. Fixes d7158619.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/tui/events.rs
Old New
@@ -12,6 +12,7 @@ use crate::error::Error;
12 use crate::event::ReviewVerdict; 12 use crate::event::ReviewVerdict;
13 use crate::issue as issue_mod; 13 use crate::issue as issue_mod;
14 use crate::patch as patch_mod; 14 use crate::patch as patch_mod;
15 use crate::state::IssueStatus;
15 16
16 use super::state::{App, InputMode, KeyAction, ViewMode}; 17 use super::state::{App, InputMode, KeyAction, ViewMode};
17 use super::widgets::{ui, RowTarget}; 18 use super::widgets::{ui, RowTarget};
@@ -108,42 +109,11 @@ pub(crate) fn run_loop(
108 } 109 }
109 continue; 110 continue;
110 } 111 }
111 InputMode::ReviewVerdict => { 112 // The verdict prompt is a binding table like any other
112 let verdict = match key.code { 113 // pane's — three keys and a way out — so it is dispatched
113 KeyCode::Char('a') => Some(ReviewVerdict::Approve), 114 // below rather than intercepted here, and its footer is
114 KeyCode::Char('r') => Some(ReviewVerdict::RequestChanges), 115 // generated from the same table.
115 KeyCode::Char('c') => Some(ReviewVerdict::Comment), 116 InputMode::ReviewVerdict | InputMode::Normal => {}
116 _ => None,
117 };
118 app.input_mode = InputMode::Normal;
119 if let Some(verdict) = verdict {
120 submit_review(terminal, app, repo, verdict);
121 }
122 continue;
123 }
124 InputMode::Normal => {}
125 }
126
127 // Handle keys that need repo access or are run_loop-specific
128 // before delegating to handle_key
129 if app.mode == ViewMode::Details {
130 match key.code {
131 KeyCode::Char('/') => {
132 app.input_mode = InputMode::Search;
133 app.search_query.clear();
134 continue;
135 }
136 KeyCode::Char('n') => {
137 app.input_mode = InputMode::CreateTitle;
138 app.input_buf.clear();
139 app.create_title.clear();
140 continue;
141 }
142 KeyCode::Char('o') => {
143 return checkout_and_leave(app, repo);
144 }
145 _ => {}
146 }
147 } 117 }
148 118
149 match app.handle_key(key.code, key.modifiers) { 119 match app.handle_key(key.code, key.modifiers) {
@@ -164,7 +134,7 @@ pub(crate) fn run_loop(
164 open_patch_detail(app, repo, patch); 134 open_patch_detail(app, repo, patch);
165 } 135 }
166 } 136 }
167 KeyAction::OpenCommitBrowser => { 137 KeyAction::OpenEventHistory => {
168 if let Some(ref_name) = app.selected_ref_name() { 138 if let Some(ref_name) = app.selected_ref_name() {
169 match crate::dag::walk_events(repo, &ref_name) { 139 match crate::dag::walk_events(repo, &ref_name) {
170 Ok(events) => { 140 Ok(events) => {
@@ -173,7 +143,7 @@ pub(crate) fn run_loop(
173 if !app.event_history.is_empty() { 143 if !app.event_history.is_empty() {
174 app.event_list_state.select(Some(0)); 144 app.event_list_state.select(Some(0));
175 } 145 }
176 app.mode = ViewMode::CommitList; 146 app.mode = ViewMode::EventHistory;
177 app.scroll = 0; 147 app.scroll = 0;
178 } 148 }
179 Err(e) => { 149 Err(e) => {
@@ -183,6 +153,9 @@ pub(crate) fn run_loop(
183 } 153 }
184 } 154 }
185 KeyAction::Comment => submit_comment(terminal, app, repo), 155 KeyAction::Comment => submit_comment(terminal, app, repo),
156 KeyAction::CommentOnIssue => submit_issue_comment(terminal, app, repo),
157 KeyAction::CloseOrReopenIssue => close_or_reopen_issue(terminal, app, repo),
158 KeyAction::Review(verdict) => submit_review(terminal, app, repo, verdict),
186 KeyAction::ToggleResolve => toggle_resolve(app, repo), 159 KeyAction::ToggleResolve => toggle_resolve(app, repo),
187 KeyAction::ShowAnswers => show_answers(app, repo), 160 KeyAction::ShowAnswers => show_answers(app, repo),
188 KeyAction::Checkout => return checkout_and_leave(app, repo), 161 KeyAction::Checkout => return checkout_and_leave(app, repo),
@@ -326,6 +299,97 @@ fn create_issue(app: &mut App, repo: &Repository, body: &str) {
326 app.create_title.clear(); 299 app.create_title.clear();
327 } 300 }
328 301
302 /// Comment on the issue under the cursor.
303 ///
304 /// The same `$EDITOR` round trip the patch side has used since `094b055b`,
305 /// against `issue::comment` — which is exactly what `git-collab issue comment
306 /// <id> -b …` calls, so the dashboard is not a second way of writing the same
307 /// event. Until this existed the dashboard could read an issue and not answer
308 /// it, and the key that looked like the answer opened the Event History.
309 fn submit_issue_comment(
310 terminal: &mut Terminal<CrosstermBackend<io::Stdout>>,
311 app: &mut App,
312 repo: &Repository,
313 ) {
314 let Some(issue) = app.selected_issue().cloned() else {
315 app.status_msg = Some("No issue is selected to comment on.".to_string());
316 return;
317 };
318 let short = app.issue_abbrev.of(&issue.id).to_string();
319 let seed = comment_seed(
320 &format!("Commenting on issue {}: {}", short, issue.title),
321 Vec::new(),
322 );
323
324 match compose(terminal, &seed) {
325 Ok(Composed::Body(body)) => match issue_mod::comment(repo, &issue.id, &body) {
326 Ok(_) => {
327 app.reload(repo);
328 app.status_msg = Some(format!("Comment added on issue {}", short));
329 }
330 Err(e) => app.status_msg = Some(format!("Comment failed: {}", e)),
331 },
332 Ok(Composed::Aborted) => {
333 app.status_msg = Some("Aborting comment: empty message.".to_string())
334 }
335 Err(e) => app.status_msg = Some(e.to_string()),
336 }
337 }
338
339 /// Close the issue under the cursor, or reopen it if it is already closed.
340 ///
341 /// One key, because it is one decision and the row on screen already says
342 /// which way it goes. Closing composes a reason in `$EDITOR` and an empty
343 /// buffer abandons it — the same abort `git commit` gives, for the same
344 /// reason: this DAG is append-only and a `C` pressed by accident is permanent.
345 /// Reopening asks nothing, because `issue::reopen` records no reason and
346 /// because it is itself the undo of a close.
347 fn close_or_reopen_issue(
348 terminal: &mut Terminal<CrosstermBackend<io::Stdout>>,
349 app: &mut App,
350 repo: &Repository,
351 ) {
352 let Some(issue) = app.selected_issue().cloned() else {
353 app.status_msg = Some("No issue is selected.".to_string());
354 return;
355 };
356 let short = app.issue_abbrev.of(&issue.id).to_string();
357
358 if issue.status == IssueStatus::Closed {
359 match issue_mod::reopen(repo, &issue.id) {
360 Ok(_) => {
361 app.reload(repo);
362 app.status_msg = Some(format!("Issue {} reopened", short));
363 }
364 Err(e) => app.status_msg = Some(format!("Reopen failed: {}", e)),
365 }
366 return;
367 }
368
369 let seed = comment_seed(
370 &format!(
371 "Closing issue {}: {}\n# What is written here is recorded as the reason.",
372 short, issue.title
373 ),
374 Vec::new(),
375 );
376 match compose(terminal, &seed) {
377 Ok(Composed::Body(reason)) => {
378 match issue_mod::close(repo, &issue.id, Some(reason.trim())) {
379 Ok(_) => {
380 app.reload(repo);
381 app.status_msg = Some(format!("Issue {} closed", short));
382 }
383 Err(e) => app.status_msg = Some(format!("Close failed: {}", e)),
384 }
385 }
386 Ok(Composed::Aborted) => {
387 app.status_msg = Some("Aborting close: empty message.".to_string())
388 }
389 Err(e) => app.status_msg = Some(e.to_string()),
390 }
391 }
392
329 fn submit_comment( 393 fn submit_comment(
330 terminal: &mut Terminal<CrosstermBackend<io::Stdout>>, 394 terminal: &mut Terminal<CrosstermBackend<io::Stdout>>,
331 app: &mut App, 395 app: &mut App,
src/tui/keys.rs
Old New
@@ -0,0 +1,889 @@
1 //! Every key this dashboard binds, in one table.
2 //!
3 //! Before this, key handling was fifty-seven `KeyCode::` arms spread across
4 //! two files and nested by mode and pane, and the footer describing them was a
5 //! separate set of string literals. Nothing connected the two, so nothing
6 //! could notice when they disagreed — and they did. `c` meant "comment" on a
7 //! patch, "open the Event History" on an issue, and nothing at all one pane
8 //! over, where it fell through a `pane == Detail` guard into silence.
9 //!
10 //! So the bindings are data here, and everything else is derived from them:
11 //!
12 //! - [`lookup`] is the dispatcher. A key that is not in the table for the
13 //! current context is *unbound*, and the reader is told so; there is no
14 //! longer a silent fall-through.
15 //! - [`footer`] and [`overlay`] are generated. Help cannot drift from the
16 //! bindings because it is not written down anywhere else. This is the move
17 //! `tests/cli_surface_test.rs` made when it took clap's command tree as the
18 //! oracle for command citations instead of a hand-kept list.
19 //! - The tests at the bottom assert what the old structure could not express:
20 //! that no context binds a key twice, and that no context is bound but
21 //! unreachable.
22 //!
23 //! Text entry is deliberately *not* in the table. In [`InputMode::Search`] and
24 //! [`InputMode::CreateTitle`] every printable key is data rather than a
25 //! command, so there is nothing to bind and nothing to advertise; those two
26 //! are intercepted before dispatch in `events.rs`.
27
28 use crossterm::event::KeyCode;
29
30 use crate::state::IssueStatus;
31
32 use super::state::{App, InputMode, ListMode, Pane, StatusFilter, ViewMode};
33
34 // ── Contexts ────────────────────────────────────────────────────────────────
35
36 /// Where a key is being pressed.
37 ///
38 /// The (`ViewMode`, `Pane`) pair that already governed dispatch, with
39 /// `ListMode` folded in where it genuinely changes what a key means: the
40 /// issue and patch lists share a `ViewMode` and a `Pane` and disagree about
41 /// `c`, `e`, `p` and `u`, so treating them as one context is what let those
42 /// disagreements be settled by an `if` buried in an arm.
43 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
44 pub(crate) enum Context {
45 /// The issue list, with focus on the list.
46 IssueList,
47 /// The issue list, with focus on the detail pane beside it.
48 IssueDetail,
49 /// The patch list, with focus on the list.
50 PatchList,
51 /// The patch list, with focus on the summary pane beside it. Reachable
52 /// only when there is no patch to open — `Tab` opens the patch itself
53 /// otherwise — but reachable, so it is bound.
54 PatchSummary,
55 /// One patch, its revisions and its diff: the review surface.
56 PatchDetail,
57 /// An issue's event stream, listed.
58 EventHistory,
59 /// One event from that stream.
60 EventDetail,
61 /// The modal prompt `R` opens, waiting for a verdict.
62 ReviewVerdict,
63 /// The `?` overlay, listing the context behind it.
64 Help,
65 }
66
67 impl Context {
68 /// Every context, densely indexed. The inventory the invariant tests walk,
69 /// and nothing the program itself needs — dispatch always has an [`App`]
70 /// to ask.
71 ///
72 /// The match in [`Context::ordinal`] is exhaustive, so a new variant
73 /// cannot be added without giving it an index, and `all_contexts_are_listed`
74 /// checks that index is this array's.
75 #[cfg(test)]
76 pub(crate) const ALL: [Context; 9] = [
77 Context::IssueList,
78 Context::IssueDetail,
79 Context::PatchList,
80 Context::PatchSummary,
81 Context::PatchDetail,
82 Context::EventHistory,
83 Context::EventDetail,
84 Context::ReviewVerdict,
85 Context::Help,
86 ];
87
88 #[cfg(test)]
89 fn ordinal(self) -> usize {
90 match self {
91 Context::IssueList => 0,
92 Context::IssueDetail => 1,
93 Context::PatchList => 2,
94 Context::PatchSummary => 3,
95 Context::PatchDetail => 4,
96 Context::EventHistory => 5,
97 Context::EventDetail => 6,
98 Context::ReviewVerdict => 7,
99 Context::Help => 8,
100 }
101 }
102
103 /// What the overlay calls this pane.
104 pub(crate) fn title(self) -> &'static str {
105 match self {
106 Context::IssueList => "Issues — list",
107 Context::IssueDetail => "Issues — detail",
108 Context::PatchList => "Patches — list",
109 Context::PatchSummary => "Patches — detail",
110 Context::PatchDetail => "Patch detail",
111 Context::EventHistory => "Event History",
112 Context::EventDetail => "Event detail",
113 Context::ReviewVerdict => "Review verdict",
114 Context::Help => "Keys",
115 }
116 }
117
118 /// The context a key pressed right now belongs to.
119 pub(crate) fn of(app: &App) -> Context {
120 if app.show_help {
121 return Context::Help;
122 }
123 if app.input_mode == InputMode::ReviewVerdict {
124 return Context::ReviewVerdict;
125 }
126 Context::surface(app)
127 }
128
129 /// The context underneath any overlay — the one the `?` list is about.
130 pub(crate) fn surface(app: &App) -> Context {
131 match app.mode {
132 ViewMode::PatchDetail => Context::PatchDetail,
133 ViewMode::EventHistory => Context::EventHistory,
134 ViewMode::EventDetail => Context::EventDetail,
135 ViewMode::Details => match (app.list_mode, &app.pane) {
136 (ListMode::Issues, Pane::ItemList) => Context::IssueList,
137 (ListMode::Issues, Pane::Detail) => Context::IssueDetail,
138 (ListMode::Patches, Pane::ItemList) => Context::PatchList,
139 (ListMode::Patches, Pane::Detail) => Context::PatchSummary,
140 },
141 }
142 }
143 }
144
145 // ── Keys ────────────────────────────────────────────────────────────────────
146
147 /// A key as a binding names it.
148 ///
149 /// Control is part of the binding; Shift is not. The terminal has already
150 /// folded Shift into the character by the time this sees it, which is why `R`
151 /// and `r` are two different keys in the table rather than one key and a
152 /// modifier.
153 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
154 pub(crate) struct Key {
155 pub(crate) code: KeyCode,
156 pub(crate) ctrl: bool,
157 }
158
159 const fn k(c: char) -> Key {
160 Key {
161 code: KeyCode::Char(c),
162 ctrl: false,
163 }
164 }
165
166 const fn ctrl(c: char) -> Key {
167 Key {
168 code: KeyCode::Char(c),
169 ctrl: true,
170 }
171 }
172
173 const fn c(code: KeyCode) -> Key {
174 Key { code, ctrl: false }
175 }
176
177 impl Key {
178 /// How help prints it.
179 pub(crate) fn name(self) -> String {
180 let base = match self.code {
181 KeyCode::Char(' ') => "Space".to_string(),
182 KeyCode::Char(ch) => ch.to_string(),
183 KeyCode::Enter => "Enter".to_string(),
184 KeyCode::Tab => "Tab".to_string(),
185 KeyCode::BackTab => "Shift-Tab".to_string(),
186 KeyCode::Esc => "Esc".to_string(),
187 KeyCode::Backspace => "Backspace".to_string(),
188 KeyCode::Up => "Up".to_string(),
189 KeyCode::Down => "Down".to_string(),
190 KeyCode::Left => "Left".to_string(),
191 KeyCode::Right => "Right".to_string(),
192 KeyCode::PageUp => "PgUp".to_string(),
193 KeyCode::PageDown => "PgDn".to_string(),
194 KeyCode::Home => "Home".to_string(),
195 KeyCode::End => "End".to_string(),
196 KeyCode::Delete => "Del".to_string(),
197 KeyCode::Insert => "Ins".to_string(),
198 KeyCode::F(n) => format!("F{}", n),
199 other => format!("{:?}", other),
200 };
201 if self.ctrl {
202 format!("Ctrl-{}", base)
203 } else {
204 base
205 }
206 }
207 }
208
209 // ── Actions ─────────────────────────────────────────────────────────────────
210
211 /// What a key means. One variant per distinct meaning, which is why moving
212 /// down has four of them: the footer used to say `j/k:navigate` on a pane
213 /// where `j` scrolled, because "move down" was one arm that did different
214 /// things and one string that described only the first of them.
215 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
216 pub(crate) enum Action {
217 Quit,
218 /// Leave this view for the one behind it.
219 Back,
220 SelectNext,
221 SelectPrev,
222 ScrollDown,
223 ScrollUp,
224 ScrollPageDown,
225 ScrollPageUp,
226 CursorDown,
227 CursorUp,
228 CursorPageDown,
229 CursorPageUp,
230 /// Move the view a row without moving the cursor: the only way through a
231 /// line that wrapped taller than the pane.
232 ScrollRowDown,
233 ScrollRowUp,
234 EventNext,
235 EventPrev,
236 OpenEvent,
237 TogglePane,
238 OpenSelectedPatch,
239 OpenLinkedPatch,
240 SwitchList,
241 CycleStatusFilter,
242 ToggleUnresolvedOnly,
243 Reload,
244 BeginSearch,
245 BeginCreateIssue,
246 CommentOnIssue,
247 CloseOrReopenIssue,
248 OpenEventHistory,
249 CommentOnPatch,
250 BeginReviewVerdict,
251 ToggleResolve,
252 ShowAnswers,
253 Checkout,
254 NextRevision,
255 PrevRevision,
256 ToggleInterdiff,
257 ToggleWrap,
258 ShowHelp,
259 CloseHelp,
260 VerdictApprove,
261 VerdictRequestChanges,
262 VerdictComment,
263 CancelVerdict,
264 }
265
266 // ── The table ───────────────────────────────────────────────────────────────
267
268 /// What help calls a binding. Some labels depend on what the key would do
269 /// next — `a` cycles a filter, so it has to say which way — and those are a
270 /// function of the app rather than a literal, so that the footer stays right
271 /// without anyone maintaining a parallel `match`.
272 #[derive(Clone, Copy)]
273 pub(crate) enum Label {
274 Fixed(&'static str),
275 OfApp(fn(&App) -> &'static str),
276 }
277
278 impl Label {
279 fn text(self, app: &App) -> &'static str {
280 match self {
281 Label::Fixed(s) => s,
282 Label::OfApp(f) => f(app),
283 }
284 }
285 }
286
287 pub(crate) struct Entry {
288 /// The keys that mean this here. More than one when they are aliases —
289 /// `j` and `Down` — and the first is the one help prints.
290 pub(crate) keys: &'static [Key],
291 pub(crate) action: Action,
292 pub(crate) label: Label,
293 /// Whether the one-line footer advertises it. The overlay lists all of it.
294 pub(crate) in_footer: bool,
295 /// Another action whose key is printed alongside this one's, for the pairs
296 /// that read as one entry: `j/k`, `[/]`. Still generated from the table, so
297 /// rebinding either half moves the help with it.
298 pub(crate) pairs_with: Option<Action>,
299 }
300
301 /// Shorthand for the common shape: one key, one action, one fixed label.
302 const fn e(keys: &'static [Key], action: Action, label: &'static str, in_footer: bool) -> Entry {
303 Entry {
304 keys,
305 action,
306 label: Label::Fixed(label),
307 in_footer,
308 pairs_with: None,
309 }
310 }
311
312 /// Bound in every context that is not a modal prompt.
313 ///
314 /// Kept out of the per-context tables so that `q` cannot come to mean
315 /// something else on one pane by accident — and the duplicate check below
316 /// tests these against every context, so it cannot come to mean something else
317 /// on purpose either without the test saying so.
318 const COMMON: &[Entry] = &[
319 e(&[k('?')], Action::ShowHelp, "keys", true),
320 e(&[k('q')], Action::Quit, "quit", true),
321 e(&[ctrl('c')], Action::Quit, "quit", false),
322 ];
323
324 fn filter_label(app: &App) -> &'static str {
325 match app.status_filter {
326 StatusFilter::Open => "show all",
327 StatusFilter::All => "closed",
328 StatusFilter::Closed => "open only",
329 }
330 }
331
332 fn list_label(app: &App) -> &'static str {
333 match app.list_mode {
334 ListMode::Issues => "patches",
335 ListMode::Patches => "issues",
336 }
337 }
338
339 /// `C` is one key because closing and reopening are one decision, and the
340 /// issue on screen already says which way it goes.
341 fn close_label(app: &App) -> &'static str {
342 match app.selected_issue() {
343 Some(issue) if issue.status == IssueStatus::Closed => "reopen",
344 _ => "close",
345 }
346 }
347
348 /// Down and up, with their arrow-key aliases. Named once so that a rebinding
349 /// moves every pane's navigation together, and so that the footer's `j/k` is
350 /// these keys rather than two characters someone typed into a string.
351 const DOWN: &[Key] = &[k('j'), c(KeyCode::Down)];
352 const UP: &[Key] = &[k('k'), c(KeyCode::Up)];
353
354 const fn nav_pair(next: Action, prev: Action, label: &'static str) -> [Entry; 2] {
355 [
356 Entry {
357 keys: DOWN,
358 action: next,
359 label: Label::Fixed(label),
360 in_footer: true,
361 pairs_with: Some(prev),
362 },
363 Entry {
364 keys: UP,
365 action: prev,
366 label: Label::Fixed(label),
367 in_footer: false,
368 pairs_with: None,
369 },
370 ]
371 }
372
373 const ISSUE_NAV: [Entry; 2] = nav_pair(Action::SelectNext, Action::SelectPrev, "navigate");
374 const SCROLL_NAV: [Entry; 2] = nav_pair(Action::ScrollDown, Action::ScrollUp, "scroll");
375
376 /// The keys the two issue contexts share. Everything about an issue is
377 /// addressed by the row under the cursor, and the row under the cursor does
378 /// not change when focus moves to the pane beside it — which is exactly the
379 /// assumption the old `pane == Pane::Detail` guard got wrong.
380 const ISSUE_VERBS: &[Entry] = &[
381 e(&[k('c')], Action::CommentOnIssue, "comment", true),
382 e(&[k('e')], Action::OpenEventHistory, "events", true),
383 e(&[k('p')], Action::OpenLinkedPatch, "patch", true),
384 Entry {
385 keys: &[k('P')],
386 action: Action::SwitchList,
387 label: Label::OfApp(list_label),
388 in_footer: true,
389 pairs_with: None,
390 },
391 Entry {
392 keys: &[k('a')],
393 action: Action::CycleStatusFilter,
394 label: Label::OfApp(filter_label),
395 in_footer: true,
396 pairs_with: None,
397 },
398 Entry {
399 keys: &[k('C')],
400 action: Action::CloseOrReopenIssue,
401 label: Label::OfApp(close_label),
402 in_footer: true,
403 pairs_with: None,
404 },
405 e(&[k('/')], Action::BeginSearch, "search", true),
406 e(&[k('r')], Action::Reload, "refresh", true),
407 e(&[k('n')], Action::BeginCreateIssue, "new issue", true),
408 e(&[k('o')], Action::Checkout, "check out the linked patch", false),
409 e(
410 &[c(KeyCode::PageDown)],
411 Action::ScrollPageDown,
412 "page down the detail",
413 false,
414 ),
415 e(
416 &[c(KeyCode::PageUp)],
417 Action::ScrollPageUp,
418 "page up the detail",
419 false,
420 ),
421 // Esc is "back" wherever there is something behind. On the top-level
422 // lists there is not, so it keeps the meaning it has always had here.
423 e(&[c(KeyCode::Esc)], Action::Quit, "quit", false),
424 ];
425
426 /// The same, for the patch list. `u` lives here and only here: issues carry no
427 /// unresolved count, so on that pane it would narrow nothing while looking
428 /// like it had.
429 const PATCH_LIST_VERBS: &[Entry] = &[
430 e(&[k('u')], Action::ToggleUnresolvedOnly, "unresolved", true),
431 Entry {
432 keys: &[k('P')],
433 action: Action::SwitchList,
434 label: Label::OfApp(list_label),
435 in_footer: true,
436 pairs_with: None,
437 },
438 Entry {
439 keys: &[k('a')],
440 action: Action::CycleStatusFilter,
441 label: Label::OfApp(filter_label),
442 in_footer: true,
443 pairs_with: None,
444 },
445 e(&[k('/')], Action::BeginSearch, "search", true),
446 e(&[k('r')], Action::Reload, "refresh", true),
447 e(&[k('n')], Action::BeginCreateIssue, "new issue", true),
448 e(&[k('o')], Action::Checkout, "check out this patch", false),
449 e(
450 &[c(KeyCode::PageDown)],
451 Action::ScrollPageDown,
452 "page down the detail",
453 false,
454 ),
455 e(
456 &[c(KeyCode::PageUp)],
457 Action::ScrollPageUp,
458 "page up the detail",
459 false,
460 ),
461 // Esc is "back" wherever there is something behind. On the top-level
462 // lists there is not, so it keeps the meaning it has always had here.
463 e(&[c(KeyCode::Esc)], Action::Quit, "quit", false),
464 ];
465
466 const PATCH_DETAIL: &[Entry] = &[
467 Entry {
468 keys: DOWN,
469 action: Action::CursorDown,
470 label: Label::Fixed("line"),
471 in_footer: true,
472 pairs_with: Some(Action::CursorUp),
473 },
474 Entry {
475 keys: UP,
476 action: Action::CursorUp,
477 label: Label::Fixed("line"),
478 in_footer: false,
479 pairs_with: None,
480 },
481 e(&[k('c')], Action::CommentOnPatch, "comment", true),
482 e(&[k('R')], Action::BeginReviewVerdict, "review", true),
483 e(&[k('x')], Action::ToggleResolve, "resolve", true),
484 e(&[k('a')], Action::ShowAnswers, "answers", true),
485 e(&[c(KeyCode::Esc)], Action::Back, "back", true),
486 Entry {
487 keys: &[k('[')],
488 action: Action::PrevRevision,
489 label: Label::Fixed("revision"),
490 in_footer: true,
491 pairs_with: Some(Action::NextRevision),
492 },
493 Entry {
494 keys: &[k(']')],
495 action: Action::NextRevision,
496 label: Label::Fixed("revision"),
497 in_footer: false,
498 pairs_with: None,
499 },
500 e(&[k('d')], Action::ToggleInterdiff, "interdiff", true),
501 e(&[k('o')], Action::Checkout, "checkout", true),
502 e(&[k('w')], Action::ToggleWrap, "wrap", true),
503 e(&[ctrl('e')], Action::ScrollRowDown, "scroll a row", false),
504 e(&[ctrl('y')], Action::ScrollRowUp, "scroll a row back", false),
505 e(
506 &[c(KeyCode::PageDown)],
507 Action::CursorPageDown,
508 "page down",
509 false,
510 ),
511 e(&[c(KeyCode::PageUp)], Action::CursorPageUp, "page up", false),
512 ];
513
514 const EVENT_HISTORY: &[Entry] = &[
515 Entry {
516 keys: DOWN,
517 action: Action::EventNext,
518 label: Label::Fixed("navigate"),
519 in_footer: true,
520 pairs_with: Some(Action::EventPrev),
521 },
522 Entry {
523 keys: UP,
524 action: Action::EventPrev,
525 label: Label::Fixed("navigate"),
526 in_footer: false,
527 pairs_with: None,
528 },
529 e(&[c(KeyCode::Enter)], Action::OpenEvent, "detail", true),
530 e(&[c(KeyCode::Esc)], Action::Back, "back", true),
531 ];
532
533 const EVENT_DETAIL: &[Entry] = &[
534 e(&[c(KeyCode::Esc)], Action::Back, "back", true),
535 e(
536 &[c(KeyCode::PageDown)],
537 Action::ScrollPageDown,
538 "page down",
539 false,
540 ),
541 e(&[c(KeyCode::PageUp)], Action::ScrollPageUp, "page up", false),
542 ];
543
544 const REVIEW_VERDICT: &[Entry] = &[
545 e(&[k('a')], Action::VerdictApprove, "approve", true),
546 e(
547 &[k('r')],
548 Action::VerdictRequestChanges,
549 "request-changes",
550 true,
551 ),
552 e(&[k('c')], Action::VerdictComment, "comment", true),
553 e(&[c(KeyCode::Esc)], Action::CancelVerdict, "cancel", true),
554 ];
555
556 const HELP: &[Entry] = &[
557 Entry {
558 keys: &[c(KeyCode::Esc), k('?')],
559 action: Action::CloseHelp,
560 label: Label::Fixed("close"),
561 in_footer: true,
562 pairs_with: None,
563 },
564 e(&[k('q')], Action::Quit, "quit", true),
565 ];
566
567 /// The bindings of one context, in footer order.
568 ///
569 /// Ordered by what the reader reaches for, because the footer is one line and
570 /// a narrow terminal clips the tail. `?` and `q` lead: one line cannot hold
571 /// this surface, so its first duty is to name the key that can, and the second
572 /// is the way out.
573 pub(crate) fn entries(context: Context) -> Vec<&'static Entry> {
574 let own: Vec<&'static Entry> = match context {
575 Context::IssueList => ISSUE_NAV
576 .iter()
577 .chain(TAB_PANE.iter())
578 .chain(ISSUE_VERBS.iter())
579 .collect(),
580 Context::IssueDetail => SCROLL_NAV
581 .iter()
582 .chain(TAB_PANE.iter())
583 .chain(ISSUE_VERBS.iter())
584 .collect(),
585 Context::PatchList => ISSUE_NAV
586 .iter()
587 .chain(OPEN_PATCH.iter())
588 .chain(PATCH_LIST_VERBS.iter())
589 .collect(),
590 Context::PatchSummary => SCROLL_NAV
591 .iter()
592 .chain(TAB_PANE.iter())
593 .chain(PATCH_LIST_VERBS.iter())
594 .collect(),
595 Context::PatchDetail => PATCH_DETAIL.iter().collect(),
596 Context::EventHistory => EVENT_HISTORY.iter().collect(),
597 Context::EventDetail => SCROLL_NAV.iter().chain(EVENT_DETAIL.iter()).collect(),
598 Context::ReviewVerdict => REVIEW_VERDICT.iter().collect(),
599 Context::Help => HELP.iter().collect(),
600 };
601 let mut all: Vec<&'static Entry> = common(context).iter().collect();
602 all.extend(own);
603 all
604 }
605
606 const TAB_PANE: [Entry; 1] = [Entry {
607 keys: &[c(KeyCode::Tab), c(KeyCode::Enter)],
608 action: Action::TogglePane,
609 label: Label::Fixed("pane"),
610 in_footer: true,
611 pairs_with: None,
612 }];
613
614 /// In the patch list, `Enter` opens the patch rather than moving focus: the
615 /// pane beside the list is a summary of the thing `Enter` opens, so stopping
616 /// there would be a step to nowhere.
617 const OPEN_PATCH: [Entry; 1] = [Entry {
618 keys: &[c(KeyCode::Enter), c(KeyCode::Tab)],
619 action: Action::OpenSelectedPatch,
620 label: Label::Fixed("view patch"),
621 in_footer: true,
622 pairs_with: None,
623 }];
624
625 fn common(context: Context) -> &'static [Entry] {
626 match context {
627 // Modal prompts. `q` in a verdict prompt is a verdict-shaped mistake,
628 // and the overlay's own `q` is bound below.
629 Context::Help | Context::ReviewVerdict => &[],
630 _ => COMMON,
631 }
632 }
633
634 // ── Dispatch ────────────────────────────────────────────────────────────────
635
636 /// What this key means here, or `None` if it means nothing here.
637 ///
638 /// `None` is a real answer, and the caller owes the reader a sentence for it.
639 /// That is the whole difference from the structure this replaces, where a key
640 /// with no binding and a key whose binding was gated out both arrived at the
641 /// same `_ => KeyAction::Continue`.
642 pub(crate) fn lookup(context: Context, key: Key) -> Option<Action> {
643 entries(context)
644 .into_iter()
645 .find(|entry| entry.keys.contains(&key))
646 .map(|entry| entry.action)
647 }
648
649 // ── Generated help ──────────────────────────────────────────────────────────
650
651 /// The key names printed for one entry, `j/k` and `[/]` included.
652 fn hint_keys(context: Context, entry: &Entry) -> String {
653 let mut names = vec![entry.keys[0].name()];
654 if let Some(partner) = entry.pairs_with {
655 if let Some(other) = entries(context)
656 .into_iter()
657 .find(|candidate| candidate.action == partner)
658 {
659 names.push(other.keys[0].name());
660 }
661 }
662 names.join("/")
663 }
664
665 /// Whether some other entry already prints this one as the tail of its pair,
666 /// so `j/k` and `[/]` appear once rather than twice.
667 fn is_a_partner(context: Context, entry: &Entry) -> bool {
668 entries(context)
669 .into_iter()
670 .any(|other| other.pairs_with == Some(entry.action))
671 }
672
673 /// The one-line footer for wherever the reader is.
674 pub(crate) fn footer(app: &App) -> String {
675 let context = Context::of(app);
676 let mut items: Vec<String> = Vec::new();
677 for entry in entries(context) {
678 if !entry.in_footer {
679 continue;
680 }
681 items.push(format!(
682 "{}:{}",
683 hint_keys(context, entry),
684 entry.label.text(app)
685 ));
686 }
687 items.join(" ")
688 }
689
690 /// Every binding of the context behind the overlay, as `(keys, meaning)`.
691 ///
692 /// `surface` rather than `of`, because the list the overlay draws is the list
693 /// for the pane it is covering — the overlay's own two keys are on its footer.
694 pub(crate) fn overlay(app: &App) -> Vec<(String, String)> {
695 let context = Context::surface(app);
696 entries(context)
697 .into_iter()
698 .filter(|entry| !is_a_partner(context, entry))
699 .map(|entry| {
700 let keys = if entry.pairs_with.is_some() {
701 hint_keys(context, entry)
702 } else {
703 entry
704 .keys
705 .iter()
706 .map(|key| key.name())
707 .collect::<Vec<_>>()
708 .join(" / ")
709 };
710 (keys, entry.label.text(app).to_string())
711 })
712 .collect()
713 }
714
715 #[cfg(test)]
716 mod tests {
717 use super::*;
718 use crate::abbrev::Abbrev;
719
720 fn app() -> App {
721 App::new(Vec::new(), Vec::new(), Abbrev::minimal(), Abbrev::minimal())
722 }
723
724 /// The invariant the old structure could not express.
725 ///
726 /// Two keys meaning the same thing is fine — `j` and `Down` are aliases.
727 /// One key meaning two things is the bug this whole table exists to make
728 /// impossible, and it was live: `c` was comment, Event History, and
729 /// silence, depending on where you stood.
730 #[test]
731 fn no_context_binds_a_key_twice() {
732 for context in Context::ALL {
733 let mut seen: Vec<(Key, Action)> = Vec::new();
734 for entry in entries(context) {
735 for key in entry.keys {
736 if let Some((_, other)) = seen.iter().find(|(k, _)| k == key) {
737 panic!(
738 "{:?} binds {} twice: {:?} and {:?}",
739 context,
740 key.name(),
741 other,
742 entry.action
743 );
744 }
745 seen.push((*key, entry.action));
746 }
747 }
748 }
749 }
750
751 /// A binding in a context nothing can reach is a binding nobody can press.
752 /// Every context in the table has to be one `Context::of` can produce.
753 #[test]
754 fn every_context_is_reachable() {
755 for context in Context::ALL {
756 let mut app = app();
757 match context {
758 Context::IssueList => {
759 app.list_mode = ListMode::Issues;
760 app.pane = Pane::ItemList;
761 }
762 Context::IssueDetail => {
763 app.list_mode = ListMode::Issues;
764 app.pane = Pane::Detail;
765 }
766 Context::PatchList => {
767 app.list_mode = ListMode::Patches;
768 app.pane = Pane::ItemList;
769 }
770 Context::PatchSummary => {
771 app.list_mode = ListMode::Patches;
772 app.pane = Pane::Detail;
773 }
774 Context::PatchDetail => app.mode = ViewMode::PatchDetail,
775 Context::EventHistory => app.mode = ViewMode::EventHistory,
776 Context::EventDetail => app.mode = ViewMode::EventDetail,
777 Context::ReviewVerdict => app.input_mode = InputMode::ReviewVerdict,
778 Context::Help => app.show_help = true,
779 }
780 assert_eq!(
781 Context::of(&app),
782 context,
783 "no state produces {:?}, so its bindings are unreachable",
784 context
785 );
786 }
787 }
788
789 /// `ALL` is what the other tests iterate, so it has to be all of them.
790 #[test]
791 fn all_contexts_are_listed() {
792 for (index, context) in Context::ALL.iter().enumerate() {
793 assert_eq!(
794 context.ordinal(),
795 index,
796 "Context::ALL and Context::ordinal disagree about {:?}",
797 context
798 );
799 }
800 }
801
802 /// No context is a room with no door. Every one of them can be left, by
803 /// `Esc`, by `q`, or — for the overlay — by the key that opened it.
804 #[test]
805 fn every_context_has_a_way_out() {
806 for context in Context::ALL {
807 let ways: Vec<Action> = entries(context)
808 .into_iter()
809 .map(|entry| entry.action)
810 .filter(|action| {
811 matches!(
812 action,
813 Action::Quit | Action::Back | Action::CloseHelp | Action::CancelVerdict
814 )
815 })
816 .collect();
817 assert!(!ways.is_empty(), "{:?} cannot be left", context);
818 }
819 }
820
821 /// Every binding says what it does, because the overlay prints it.
822 #[test]
823 fn every_binding_is_labelled() {
824 let app = app();
825 for context in Context::ALL {
826 for entry in entries(context) {
827 assert!(
828 !entry.keys.is_empty(),
829 "{:?} has a binding with no key",
830 context
831 );
832 assert!(
833 !entry.label.text(&app).is_empty(),
834 "{:?} binds {} to nothing it can name",
835 context,
836 entry.keys[0].name()
837 );
838 }
839 }
840 }
841
842 /// A pair's partner has to be in the same context, or the footer prints
843 /// half of a `j/k`.
844 #[test]
845 fn every_pair_has_its_partner() {
846 for context in Context::ALL {
847 for entry in entries(context) {
848 if let Some(partner) = entry.pairs_with {
849 assert!(
850 entries(context)
851 .into_iter()
852 .any(|other| other.action == partner),
853 "{:?} pairs {} with {:?}, which is not bound here",
854 context,
855 entry.keys[0].name(),
856 partner
857 );
858 }
859 }
860 }
861 }
862
863 /// The footer is generated, so this is a property of the table rather than
864 /// of a string literal: what it prints is what the keys do.
865 #[test]
866 fn the_footer_names_the_bindings_it_has() {
867 let mut app = app();
868 app.list_mode = ListMode::Issues;
869 let text = footer(&app);
870 assert!(text.starts_with("?:keys"), "{}", text);
871 assert!(text.contains("c:comment"), "{}", text);
872 assert!(text.contains("e:events"), "{}", text);
873 assert!(!text.contains("c:events"), "{}", text);
874 }
875
876 /// The same key, two panes, one meaning.
877 #[test]
878 fn c_means_comment_on_both_issue_contexts() {
879 for context in [Context::IssueList, Context::IssueDetail] {
880 assert_eq!(lookup(context, k('c')), Some(Action::CommentOnIssue));
881 }
882 }
883
884 /// And nothing that is not bound comes back as if it were.
885 #[test]
886 fn an_unbound_key_looks_up_to_nothing() {
887 assert_eq!(lookup(Context::IssueList, k('Z')), None);
888 }
889 }
src/tui/mod.rs
Old New
@@ -1,4 +1,5 @@
1 mod events; 1 mod events;
2 mod keys;
2 mod state; 3 mod state;
3 mod tips; 4 mod tips;
4 mod widgets; 5 mod widgets;
@@ -17,8 +18,10 @@ use self::events::run_loop;
17 use self::state::App; 18 use self::state::App;
18 19
19 pub fn run(repo: &Repository) -> Result<(), Error> { 20 pub fn run(repo: &Repository) -> Result<(), Error> {
20 let issues = app_state::list_issues(repo)?; 21 // Archived included, and narrowed by the status filter on the way to the
21 let patches = app_state::list_patches(repo)?; 22 // screen — see the note in `App::reload`.
23 let issues = app_state::list_issues_with_archived(repo)?;
24 let patches = app_state::list_patches_with_archived(repo)?;
22 // A separate query from the two above, and that is the point: those list 25 // A separate query from the two above, and that is the point: those list
23 // what the dashboard can display, this counts what the ids have to be 26 // what the dashboard can display, this counts what the ids have to be
24 // unique against. 27 // unique against.
@@ -326,7 +329,7 @@ mod tests {
326 ); 329 );
327 } 330 }
328 331
329 /// Create sample event history for testing commit browser 332 /// Create sample event history for the Event History panes
330 fn make_test_event_history() -> Vec<(Oid, crate::event::Event)> { 333 fn make_test_event_history() -> Vec<(Oid, crate::event::Event)> {
331 let oid1 = Oid::from_str("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap(); 334 let oid1 = Oid::from_str("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap();
332 let oid2 = Oid::from_str("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb").unwrap(); 335 let oid2 = Oid::from_str("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb").unwrap();
@@ -523,39 +526,49 @@ mod tests {
523 526
524 // ── handle_key tests for 'c' key ───────────────────────────────────── 527 // ── handle_key tests for 'c' key ─────────────────────────────────────
525 528
526 #[test] 529 /// The Event History is on `e` now, and on both issue panes rather than
527 fn test_handle_key_c_in_detail_pane_returns_open_commit_browser() { 530 /// only the one with focus.
528 let mut app = make_app(3, 0); 531 ///
529 app.pane = Pane::Detail; 532 /// This test used to assert `c`. It was not wrong about the code; it was
530 app.list_state.select(Some(0)); 533 /// wrong about what the code should do, which is why it passed for as long
531 let result = app.handle_key( 534 /// as it did — `c` had already become the comment key on the patch pane,
532 crossterm::event::KeyCode::Char('c'), 535 /// and nothing here could see that.
533 crossterm::event::KeyModifiers::empty(), 536 #[test]
534 ); 537 fn test_handle_key_e_opens_the_event_history() {
535 assert_eq!(result, KeyAction::OpenCommitBrowser); 538 for pane in [Pane::ItemList, Pane::Detail] {
536 } 539 let mut app = make_app(3, 0);
537 540 app.pane = pane;
538 #[test] 541 app.list_state.select(Some(0));
539 fn test_handle_key_c_in_item_list_pane_is_noop() { 542 let result = app.handle_key(
540 let mut app = make_app(3, 0); 543 crossterm::event::KeyCode::Char('e'),
541 app.pane = Pane::ItemList; 544 crossterm::event::KeyModifiers::empty(),
542 app.list_state.select(Some(0)); 545 );
543 let result = app.handle_key( 546 assert_eq!(result, KeyAction::OpenEventHistory);
544 crossterm::event::KeyCode::Char('c'), 547 }
545 crossterm::event::KeyModifiers::empty(),
546 );
547 assert_eq!(result, KeyAction::Continue);
548 } 548 }
549 549
550 /// With no issue under the cursor there is nothing to comment on or to
551 /// show the history of — but the reader is told that, rather than left to
552 /// guess whether the key exists.
550 #[test] 553 #[test]
551 fn test_handle_key_c_no_selection_is_noop() { 554 fn test_no_selection_says_there_is_no_issue() {
552 let mut app = make_app(0, 0); 555 for key in ['c', 'e', 'C'] {
553 app.pane = Pane::Detail; 556 let mut app = make_app(0, 0);
554 let result = app.handle_key( 557 app.pane = Pane::Detail;
555 crossterm::event::KeyCode::Char('c'), 558 let result = app.handle_key(
556 crossterm::event::KeyModifiers::empty(), 559 crossterm::event::KeyCode::Char(key),
557 ); 560 crossterm::event::KeyModifiers::empty(),
558 assert_eq!(result, KeyAction::Continue); 561 );
562 assert_eq!(result, KeyAction::Continue);
563 assert!(
564 app.status_msg
565 .as_deref()
566 .is_some_and(|m| m.contains("No issue")),
567 "`{}` with nothing selected said {:?}",
568 key,
569 app.status_msg
570 );
571 }
559 } 572 }
560 573
561 #[test] 574 #[test]
@@ -568,14 +581,14 @@ mod tests {
568 assert_eq!(result, KeyAction::Quit); 581 assert_eq!(result, KeyAction::Quit);
569 } 582 }
570 583
571 // ── CommitList navigation tests ────────────────────────────────────── 584 // ── Event history navigation tests ──────────────────────────────────────
572 585
573 #[test] 586 #[test]
574 fn test_commit_list_navigate_down() { 587 fn test_commit_list_navigate_down() {
575 let mut app = make_app(3, 0); 588 let mut app = make_app(3, 0);
576 app.event_history = make_test_event_history(); 589 app.event_history = make_test_event_history();
577 app.event_list_state.select(Some(0)); 590 app.event_list_state.select(Some(0));
578 app.mode = ViewMode::CommitList; 591 app.mode = ViewMode::EventHistory;
579 592
580 app.handle_key( 593 app.handle_key(
581 crossterm::event::KeyCode::Char('j'), 594 crossterm::event::KeyCode::Char('j'),
@@ -589,7 +602,7 @@ mod tests {
589 let mut app = make_app(3, 0); 602 let mut app = make_app(3, 0);
590 app.event_history = make_test_event_history(); 603 app.event_history = make_test_event_history();
591 app.event_list_state.select(Some(2)); 604 app.event_list_state.select(Some(2));
592 app.mode = ViewMode::CommitList; 605 app.mode = ViewMode::EventHistory;
593 606
594 app.handle_key( 607 app.handle_key(
595 crossterm::event::KeyCode::Char('k'), 608 crossterm::event::KeyCode::Char('k'),
@@ -603,7 +616,7 @@ mod tests {
603 let mut app = make_app(3, 0); 616 let mut app = make_app(3, 0);
604 app.event_history = make_test_event_history(); 617 app.event_history = make_test_event_history();
605 app.event_list_state.select(Some(2)); 618 app.event_list_state.select(Some(2));
606 app.mode = ViewMode::CommitList; 619 app.mode = ViewMode::EventHistory;
607 620
608 app.handle_key( 621 app.handle_key(
609 crossterm::event::KeyCode::Down, 622 crossterm::event::KeyCode::Down,
@@ -617,7 +630,7 @@ mod tests {
617 let mut app = make_app(3, 0); 630 let mut app = make_app(3, 0);
618 app.event_history = make_test_event_history(); 631 app.event_history = make_test_event_history();
619 app.event_list_state.select(Some(0)); 632 app.event_list_state.select(Some(0));
620 app.mode = ViewMode::CommitList; 633 app.mode = ViewMode::EventHistory;
621 634
622 app.handle_key( 635 app.handle_key(
623 crossterm::event::KeyCode::Up, 636 crossterm::event::KeyCode::Up,
@@ -631,7 +644,7 @@ mod tests {
631 let mut app = make_app(3, 0); 644 let mut app = make_app(3, 0);
632 app.event_history = make_test_event_history(); 645 app.event_history = make_test_event_history();
633 app.event_list_state.select(Some(1)); 646 app.event_list_state.select(Some(1));
634 app.mode = ViewMode::CommitList; 647 app.mode = ViewMode::EventHistory;
635 648
636 let result = app.handle_key( 649 let result = app.handle_key(
637 crossterm::event::KeyCode::Esc, 650 crossterm::event::KeyCode::Esc,
@@ -646,7 +659,7 @@ mod tests {
646 #[test] 659 #[test]
647 fn test_commit_list_q_quits() { 660 fn test_commit_list_q_quits() {
648 let mut app = make_app(3, 0); 661 let mut app = make_app(3, 0);
649 app.mode = ViewMode::CommitList; 662 app.mode = ViewMode::EventHistory;
650 let result = app.handle_key( 663 let result = app.handle_key(
651 crossterm::event::KeyCode::Char('q'), 664 crossterm::event::KeyCode::Char('q'),
652 crossterm::event::KeyModifiers::empty(), 665 crossterm::event::KeyModifiers::empty(),
@@ -654,21 +667,21 @@ mod tests {
654 assert_eq!(result, KeyAction::Quit); 667 assert_eq!(result, KeyAction::Quit);
655 } 668 }
656 669
657 // ── CommitDetail tests ─────────────────────────────────────────────── 670 // ── Event detail tests ───────────────────────────────────────────────
658 671
659 #[test] 672 #[test]
660 fn test_commit_list_enter_opens_detail() { 673 fn test_commit_list_enter_opens_detail() {
661 let mut app = make_app(3, 0); 674 let mut app = make_app(3, 0);
662 app.event_history = make_test_event_history(); 675 app.event_history = make_test_event_history();
663 app.event_list_state.select(Some(1)); 676 app.event_list_state.select(Some(1));
664 app.mode = ViewMode::CommitList; 677 app.mode = ViewMode::EventHistory;
665 678
666 let result = app.handle_key( 679 let result = app.handle_key(
667 crossterm::event::KeyCode::Enter, 680 crossterm::event::KeyCode::Enter,
668 crossterm::event::KeyModifiers::empty(), 681 crossterm::event::KeyModifiers::empty(),
669 ); 682 );
670 assert_eq!(result, KeyAction::Continue); 683 assert_eq!(result, KeyAction::Continue);
671 assert_eq!(app.mode, ViewMode::CommitDetail); 684 assert_eq!(app.mode, ViewMode::EventDetail);
672 assert_eq!(app.scroll, 0); 685 assert_eq!(app.scroll, 0);
673 } 686 }
674 687
@@ -677,13 +690,13 @@ mod tests {
677 let mut app = make_app(3, 0); 690 let mut app = make_app(3, 0);
678 app.event_history = make_test_event_history(); 691 app.event_history = make_test_event_history();
679 app.event_list_state = ListState::default(); 692 app.event_list_state = ListState::default();
680 app.mode = ViewMode::CommitList; 693 app.mode = ViewMode::EventHistory;
681 694
682 app.handle_key( 695 app.handle_key(
683 crossterm::event::KeyCode::Enter, 696 crossterm::event::KeyCode::Enter,
684 crossterm::event::KeyModifiers::empty(), 697 crossterm::event::KeyModifiers::empty(),
685 ); 698 );
686 assert_eq!(app.mode, ViewMode::CommitList); 699 assert_eq!(app.mode, ViewMode::EventHistory);
687 } 700 }
688 701
689 #[test] 702 #[test]
@@ -691,7 +704,7 @@ mod tests {
691 let mut app = make_app(3, 0); 704 let mut app = make_app(3, 0);
692 app.event_history = make_test_event_history(); 705 app.event_history = make_test_event_history();
693 app.event_list_state.select(Some(0)); 706 app.event_list_state.select(Some(0));
694 app.mode = ViewMode::CommitDetail; 707 app.mode = ViewMode::EventDetail;
695 app.scroll = 5; 708 app.scroll = 5;
696 709
697 let result = app.handle_key( 710 let result = app.handle_key(
@@ -699,7 +712,7 @@ mod tests {
699 crossterm::event::KeyModifiers::empty(), 712 crossterm::event::KeyModifiers::empty(),
700 ); 713 );
701 assert_eq!(result, KeyAction::Continue); 714 assert_eq!(result, KeyAction::Continue);
702 assert_eq!(app.mode, ViewMode::CommitList); 715 assert_eq!(app.mode, ViewMode::EventHistory);
703 assert_eq!(app.scroll, 0); 716 assert_eq!(app.scroll, 0);
704 assert_eq!(app.event_history.len(), 3); 717 assert_eq!(app.event_history.len(), 3);
705 } 718 }
@@ -707,7 +720,7 @@ mod tests {
707 #[test] 720 #[test]
708 fn test_commit_detail_scroll() { 721 fn test_commit_detail_scroll() {
709 let mut app = make_app(3, 0); 722 let mut app = make_app(3, 0);
710 app.mode = ViewMode::CommitDetail; 723 app.mode = ViewMode::EventDetail;
711 app.scroll = 0; 724 app.scroll = 0;
712 725
713 app.handle_key( 726 app.handle_key(
@@ -730,7 +743,7 @@ mod tests {
730 #[test] 743 #[test]
731 fn test_commit_detail_page_scroll() { 744 fn test_commit_detail_page_scroll() {
732 let mut app = make_app(3, 0); 745 let mut app = make_app(3, 0);
733 app.mode = ViewMode::CommitDetail; 746 app.mode = ViewMode::EventDetail;
734 app.scroll = 0; 747 app.scroll = 0;
735 748
736 app.handle_key( 749 app.handle_key(
@@ -748,7 +761,7 @@ mod tests {
748 #[test] 761 #[test]
749 fn test_commit_detail_q_quits() { 762 fn test_commit_detail_q_quits() {
750 let mut app = make_app(3, 0); 763 let mut app = make_app(3, 0);
751 app.mode = ViewMode::CommitDetail; 764 app.mode = ViewMode::EventDetail;
752 let result = app.handle_key( 765 let result = app.handle_key(
753 crossterm::event::KeyCode::Char('q'), 766 crossterm::event::KeyCode::Char('q'),
754 crossterm::event::KeyModifiers::empty(), 767 crossterm::event::KeyModifiers::empty(),
@@ -761,25 +774,25 @@ mod tests {
761 #[test] 774 #[test]
762 fn test_c_ignored_in_commit_list_mode() { 775 fn test_c_ignored_in_commit_list_mode() {
763 let mut app = make_app(3, 0); 776 let mut app = make_app(3, 0);
764 app.mode = ViewMode::CommitList; 777 app.mode = ViewMode::EventHistory;
765 let result = app.handle_key( 778 let result = app.handle_key(
766 crossterm::event::KeyCode::Char('c'), 779 crossterm::event::KeyCode::Char('c'),
767 crossterm::event::KeyModifiers::empty(), 780 crossterm::event::KeyModifiers::empty(),
768 ); 781 );
769 assert_eq!(result, KeyAction::Continue); 782 assert_eq!(result, KeyAction::Continue);
770 assert_eq!(app.mode, ViewMode::CommitList); 783 assert_eq!(app.mode, ViewMode::EventHistory);
771 } 784 }
772 785
773 #[test] 786 #[test]
774 fn test_c_ignored_in_commit_detail_mode() { 787 fn test_c_ignored_in_commit_detail_mode() {
775 let mut app = make_app(3, 0); 788 let mut app = make_app(3, 0);
776 app.mode = ViewMode::CommitDetail; 789 app.mode = ViewMode::EventDetail;
777 let result = app.handle_key( 790 let result = app.handle_key(
778 crossterm::event::KeyCode::Char('c'), 791 crossterm::event::KeyCode::Char('c'),
779 crossterm::event::KeyModifiers::empty(), 792 crossterm::event::KeyModifiers::empty(),
780 ); 793 );
781 assert_eq!(result, KeyAction::Continue); 794 assert_eq!(result, KeyAction::Continue);
782 assert_eq!(app.mode, ViewMode::CommitDetail); 795 assert_eq!(app.mode, ViewMode::EventDetail);
783 } 796 }
784 797
785 // ── handle_key basic tests ─────────────────────────────────────────── 798 // ── handle_key basic tests ───────────────────────────────────────────
@@ -973,7 +986,8 @@ mod tests {
973 let buf = render_app(&mut app); 986 let buf = render_app(&mut app);
974 assert_buffer_contains(&buf, "j/k:navigate"); 987 assert_buffer_contains(&buf, "j/k:navigate");
975 assert_buffer_contains(&buf, "Tab:pane"); 988 assert_buffer_contains(&buf, "Tab:pane");
976 assert_buffer_contains(&buf, "c:events"); 989 assert_buffer_contains(&buf, "e:events");
990 assert_buffer_contains(&buf, "c:comment");
977 } 991 }
978 992
979 #[test] 993 #[test]
@@ -981,7 +995,7 @@ mod tests {
981 let mut app = make_app(3, 0); 995 let mut app = make_app(3, 0);
982 app.event_history = make_test_event_history(); 996 app.event_history = make_test_event_history();
983 app.event_list_state.select(Some(0)); 997 app.event_list_state.select(Some(0));
984 app.mode = ViewMode::CommitList; 998 app.mode = ViewMode::EventHistory;
985 let buf = render_app(&mut app); 999 let buf = render_app(&mut app);
986 assert_buffer_contains(&buf, "Event History"); 1000 assert_buffer_contains(&buf, "Event History");
987 assert_buffer_contains(&buf, "Issue Open"); 1001 assert_buffer_contains(&buf, "Issue Open");
@@ -994,7 +1008,7 @@ mod tests {
994 let mut app = make_app(3, 0); 1008 let mut app = make_app(3, 0);
995 app.event_history = make_test_event_history(); 1009 app.event_history = make_test_event_history();
996 app.event_list_state.select(Some(0)); 1010 app.event_list_state.select(Some(0));
997 app.mode = ViewMode::CommitDetail; 1011 app.mode = ViewMode::EventDetail;
998 let buf = render_app(&mut app); 1012 let buf = render_app(&mut app);
999 assert_buffer_contains(&buf, "Event Detail"); 1013 assert_buffer_contains(&buf, "Event Detail");
1000 assert_buffer_contains(&buf, "aaaaaaa"); 1014 assert_buffer_contains(&buf, "aaaaaaa");
@@ -1005,7 +1019,7 @@ mod tests {
1005 #[test] 1019 #[test]
1006 fn test_render_commit_list_footer() { 1020 fn test_render_commit_list_footer() {
1007 let mut app = make_app(3, 0); 1021 let mut app = make_app(3, 0);
1008 app.mode = ViewMode::CommitList; 1022 app.mode = ViewMode::EventHistory;
1009 let buf = render_app(&mut app); 1023 let buf = render_app(&mut app);
1010 assert_buffer_contains(&buf, "Esc:back"); 1024 assert_buffer_contains(&buf, "Esc:back");
1011 } 1025 }
@@ -1013,7 +1027,7 @@ mod tests {
1013 #[test] 1027 #[test]
1014 fn test_render_commit_detail_footer() { 1028 fn test_render_commit_detail_footer() {
1015 let mut app = make_app(3, 0); 1029 let mut app = make_app(3, 0);
1016 app.mode = ViewMode::CommitDetail; 1030 app.mode = ViewMode::EventDetail;
1017 let buf = render_app(&mut app); 1031 let buf = render_app(&mut app);
1018 assert_buffer_contains(&buf, "Esc:back"); 1032 assert_buffer_contains(&buf, "Esc:back");
1019 assert_buffer_contains(&buf, "j/k:scroll"); 1033 assert_buffer_contains(&buf, "j/k:scroll");
@@ -1036,14 +1050,14 @@ mod tests {
1036 app.list_state.select(Some(0)); 1050 app.list_state.select(Some(0));
1037 1051
1038 let action = app.handle_key( 1052 let action = app.handle_key(
1039 crossterm::event::KeyCode::Char('c'), 1053 crossterm::event::KeyCode::Char('e'),
1040 crossterm::event::KeyModifiers::empty(), 1054 crossterm::event::KeyModifiers::empty(),
1041 ); 1055 );
1042 assert_eq!(action, KeyAction::OpenCommitBrowser); 1056 assert_eq!(action, KeyAction::OpenEventHistory);
1043 1057
1044 app.event_history = make_test_event_history(); 1058 app.event_history = make_test_event_history();
1045 app.event_list_state.select(Some(0)); 1059 app.event_list_state.select(Some(0));
1046 app.mode = ViewMode::CommitList; 1060 app.mode = ViewMode::EventHistory;
1047 app.scroll = 0; 1061 app.scroll = 0;
1048 1062
1049 app.handle_key( 1063 app.handle_key(
@@ -1056,7 +1070,7 @@ mod tests {
1056 crossterm::event::KeyCode::Enter, 1070 crossterm::event::KeyCode::Enter,
1057 crossterm::event::KeyModifiers::empty(), 1071 crossterm::event::KeyModifiers::empty(),
1058 ); 1072 );
1059 assert_eq!(app.mode, ViewMode::CommitDetail); 1073 assert_eq!(app.mode, ViewMode::EventDetail);
1060 1074
1061 app.handle_key( 1075 app.handle_key(
1062 crossterm::event::KeyCode::Char('j'), 1076 crossterm::event::KeyCode::Char('j'),
@@ -1068,7 +1082,7 @@ mod tests {
1068 crossterm::event::KeyCode::Esc, 1082 crossterm::event::KeyCode::Esc,
1069 crossterm::event::KeyModifiers::empty(), 1083 crossterm::event::KeyModifiers::empty(),
1070 ); 1084 );
1071 assert_eq!(app.mode, ViewMode::CommitList); 1085 assert_eq!(app.mode, ViewMode::EventHistory);
1072 assert_eq!(app.scroll, 0); 1086 assert_eq!(app.scroll, 0);
1073 1087
1074 app.handle_key( 1088 app.handle_key(
@@ -1192,18 +1206,24 @@ mod tests {
1192 assert_eq!(result, KeyAction::Continue); 1206 assert_eq!(result, KeyAction::Continue);
1193 } 1207 }
1194 1208
1209 /// `p` opens the linked patch from either issue pane, for the same reason
1210 /// `c` comments from either: the issue under the cursor does not change
1211 /// when focus moves to the pane beside the list. This test asserted the
1212 /// opposite, which is the `pane == Pane::Detail` guard written down twice.
1195 #[test] 1213 #[test]
1196 fn test_p_key_noop_in_item_list_pane() { 1214 fn test_p_opens_the_linked_patch_from_either_pane() {
1197 let mut app = test_app(); 1215 for pane in [Pane::ItemList, Pane::Detail] {
1198 app.patches[0].fixes = Some("i1".into()); 1216 let mut app = test_app();
1199 app.pane = Pane::ItemList; 1217 app.patches[0].fixes = Some("i1".into());
1200 app.list_state.select(Some(0)); 1218 app.pane = pane;
1219 app.list_state.select(Some(0));
1201 1220
1202 let result = app.handle_key( 1221 let result = app.handle_key(
1203 crossterm::event::KeyCode::Char('p'), 1222 crossterm::event::KeyCode::Char('p'),
1204 crossterm::event::KeyModifiers::empty(), 1223 crossterm::event::KeyModifiers::empty(),
1205 ); 1224 );
1206 assert_eq!(result, KeyAction::Continue); 1225 assert_eq!(result, KeyAction::OpenPatchDetail);
1226 }
1207 } 1227 }
1208 1228
1209 #[test] 1229 #[test]
@@ -2265,12 +2285,12 @@ mod tests {
2265 app.event_history = make_test_event_history(); 2285 app.event_history = make_test_event_history();
2266 app.event_list_state.select(Some(0)); 2286 app.event_list_state.select(Some(0));
2267 2287
2268 app.mode = ViewMode::CommitList; 2288 app.mode = ViewMode::EventHistory;
2269 let buf = render_app_sized(&mut app, 120, 24); 2289 let buf = render_app_sized(&mut app, 120, 24);
2270 assert_buffer_contains(&buf, "2026-01-01 00:00"); 2290 assert_buffer_contains(&buf, "2026-01-01 00:00");
2271 assert_buffer_lacks(&buf, "2026-01-01T00:00:00Z"); 2291 assert_buffer_lacks(&buf, "2026-01-01T00:00:00Z");
2272 2292
2273 app.mode = ViewMode::CommitDetail; 2293 app.mode = ViewMode::EventDetail;
2274 let buf = render_app_sized(&mut app, 120, 24); 2294 let buf = render_app_sized(&mut app, 120, 24);
2275 assert_buffer_contains(&buf, "2026-01-01 00:00"); 2295 assert_buffer_contains(&buf, "2026-01-01 00:00");
2276 assert_buffer_lacks(&buf, "2026-01-01T00:00:00Z"); 2296 assert_buffer_lacks(&buf, "2026-01-01T00:00:00Z");
@@ -2514,4 +2534,199 @@ mod tests {
2514 text 2534 text
2515 ); 2535 );
2516 } 2536 }
2537
2538 // ── One key, one meaning (02d3eb34 / d7158619) ───────────────────────
2539
2540 /// A key that means nothing here has to say so.
2541 ///
2542 /// The failure this replaces is not "the key did nothing" — it is that the
2543 /// reader cannot tell "nothing is bound" from "the binding is broken".
2544 /// `c` from the issue list pane was the live instance: no message, no
2545 /// beep, no hint.
2546 #[test]
2547 fn an_unbound_key_says_so() {
2548 let mut app = make_app(3, 0);
2549 app.handle_key(
2550 crossterm::event::KeyCode::Char('z'),
2551 crossterm::event::KeyModifiers::empty(),
2552 );
2553 let msg = app.status_msg.clone().unwrap_or_default();
2554 assert!(
2555 msg.contains('z') && msg.contains('?'),
2556 "an unbound key must name itself and point at the help key, got {:?}",
2557 app.status_msg
2558 );
2559 }
2560
2561 /// `c` is the review surface's comment key. It cannot also be the key that
2562 /// opens the Event History one pane over.
2563 #[test]
2564 fn c_on_an_issue_does_not_open_the_event_history() {
2565 let mut app = make_app(3, 0);
2566 app.pane = Pane::Detail;
2567 app.list_state.select(Some(0));
2568 let result = app.handle_key(
2569 crossterm::event::KeyCode::Char('c'),
2570 crossterm::event::KeyModifiers::empty(),
2571 );
2572 assert_ne!(
2573 result,
2574 KeyAction::OpenEventHistory,
2575 "`c` must mean comment everywhere it is bound"
2576 );
2577 }
2578
2579 /// The same key, from the list pane, where it used to fall through the
2580 /// `pane == Detail` guard into silence. The issue under the cursor is the
2581 /// same issue whichever pane has focus, so the binding is the same too.
2582 #[test]
2583 fn c_comments_on_the_issue_from_either_pane() {
2584 for pane in [Pane::ItemList, Pane::Detail] {
2585 let mut app = make_app(3, 0);
2586 app.pane = pane;
2587 app.list_state.select(Some(0));
2588 let result = app.handle_key(
2589 crossterm::event::KeyCode::Char('c'),
2590 crossterm::event::KeyModifiers::empty(),
2591 );
2592 assert_eq!(result, KeyAction::CommentOnIssue);
2593 }
2594 }
2595
2596 /// The footer is generated, so the key that opens the help overlay is in
2597 /// it by construction rather than by anyone remembering.
2598 #[test]
2599 fn the_footer_offers_the_help_key() {
2600 let mut app = make_app(3, 0);
2601 let buf = render_app_sized(&mut app, 200, 24);
2602 assert_buffer_contains(&buf, "?:keys");
2603 }
2604
2605 /// And it no longer advertises `c` as the way to the Event History.
2606 #[test]
2607 fn the_footer_does_not_still_call_c_the_event_key() {
2608 let mut app = make_app(3, 0);
2609 let buf = render_app_sized(&mut app, 200, 24);
2610 let text = buffer_to_string(&buf);
2611 assert!(
2612 !text.contains("c:events"),
2613 "the footer still names the old binding:\n{}",
2614 text
2615 );
2616 }
2617
2618 /// `?` lists the current pane's keys — including the ones the footer has
2619 /// no room for. That is the point of it: this surface grew `o`, `w`,
2620 /// `Ctrl-E` and the rest without anywhere to advertise them.
2621 #[test]
2622 fn question_mark_lists_the_keys_the_footer_cannot_fit() {
2623 let mut app = make_app(3, 0);
2624 app.mode = ViewMode::PatchDetail;
2625
2626 let footer_only = buffer_to_string(&render_app(&mut app));
2627 assert!(
2628 !footer_only.contains("Ctrl-e"),
2629 "eighty columns should not have fitted Ctrl-e:\n{}",
2630 footer_only
2631 );
2632
2633 app.handle_key(
2634 crossterm::event::KeyCode::Char('?'),
2635 crossterm::event::KeyModifiers::empty(),
2636 );
2637 let text = buffer_to_string(&render_app(&mut app));
2638 assert!(text.contains("Ctrl-e"), "no Ctrl-e in the overlay:\n{}", text);
2639 assert!(
2640 text.contains("Patch detail"),
2641 "the overlay does not say which pane it is about:\n{}",
2642 text
2643 );
2644 }
2645
2646 /// The overlay is about the pane underneath it, not about itself.
2647 #[test]
2648 fn the_overlay_lists_the_pane_it_covers() {
2649 let mut app = make_app(3, 0);
2650 app.show_help = true;
2651 let text = buffer_to_string(&render_app_sized(&mut app, 100, 30));
2652 assert!(text.contains("check out"), "{}", text);
2653 assert!(text.contains("Issues"), "{}", text);
2654 }
2655
2656 /// And it closes again, on `?`, on `Esc`, and on anything else — an
2657 /// overlay covering what the reader wants is one that should go away.
2658 #[test]
2659 fn the_overlay_closes_on_any_key() {
2660 for key in ['?', 'x', 'j'] {
2661 let mut app = make_app(3, 0);
2662 app.show_help = true;
2663 app.handle_key(
2664 crossterm::event::KeyCode::Char(key),
2665 crossterm::event::KeyModifiers::empty(),
2666 );
2667 assert!(!app.show_help, "`{}` left the overlay up", key);
2668 }
2669 let mut app = make_app(3, 0);
2670 app.show_help = true;
2671 app.handle_key(
2672 crossterm::event::KeyCode::Esc,
2673 crossterm::event::KeyModifiers::empty(),
2674 );
2675 assert!(!app.show_help);
2676 }
2677
2678 /// A key that is not a verdict abandons the prompt and says so, rather
2679 /// than cancelling it silently and leaving the reader wondering whether
2680 /// `R` worked.
2681 #[test]
2682 fn a_non_verdict_key_abandons_the_prompt_out_loud() {
2683 let mut app = make_app(3, 0);
2684 app.input_mode = InputMode::ReviewVerdict;
2685 app.handle_key(
2686 crossterm::event::KeyCode::Char('z'),
2687 crossterm::event::KeyModifiers::empty(),
2688 );
2689 assert_eq!(app.input_mode, InputMode::Normal);
2690 assert!(
2691 app.status_msg
2692 .as_deref()
2693 .is_some_and(|m| m.contains("abandoned")),
2694 "{:?}",
2695 app.status_msg
2696 );
2697 }
2698
2699 /// The verdict prompt's own footer is generated from the same table, so
2700 /// it cannot come to name a key the prompt does not take.
2701 #[test]
2702 fn the_verdict_prompt_footer_is_generated() {
2703 let mut app = make_app(3, 0);
2704 app.input_mode = InputMode::ReviewVerdict;
2705 let buf = render_app(&mut app);
2706 assert_buffer_contains(&buf, "Review verdict:");
2707 assert_buffer_contains(&buf, "a:approve");
2708 assert_buffer_contains(&buf, "r:request-changes");
2709 assert_buffer_contains(&buf, "Esc:cancel");
2710 }
2711
2712 /// `u` is a patch-list key. Pressed on the issue list it is unbound, and
2713 /// being unbound now means being told — the property that separates "this
2714 /// pane has no such filter" from "the filter is broken".
2715 #[test]
2716 fn u_on_the_issue_pane_says_it_does_nothing_there() {
2717 let mut app = make_app(3, 0);
2718 app.list_mode = ListMode::Issues;
2719 app.handle_key(
2720 crossterm::event::KeyCode::Char('u'),
2721 crossterm::event::KeyModifiers::empty(),
2722 );
2723 assert!(!app.unresolved_only);
2724 assert!(
2725 app.status_msg
2726 .as_deref()
2727 .is_some_and(|m| m.contains("does nothing")),
2728 "{:?}",
2729 app.status_msg
2730 );
2731 }
2517 } 2732 }
src/tui/state.rs
Old New
@@ -4,8 +4,10 @@ use git2::{Oid, Repository};
4 use ratatui::widgets::ListState; 4 use ratatui::widgets::ListState;
5 5
6 use crate::abbrev::Abbrev; 6 use crate::abbrev::Abbrev;
7 use crate::event::ReviewVerdict;
7 use crate::state::{self, IssueState, IssueStatus, PatchState, PatchStatus}; 8 use crate::state::{self, IssueState, IssueStatus, PatchState, PatchStatus};
8 9
10 use super::keys::{self, Action, Context, Key};
9 use super::tips::Tips; 11 use super::tips::Tips;
10 use super::widgets::{DetailRow, DisplayRow, RowTarget}; 12 use super::widgets::{DetailRow, DisplayRow, RowTarget};
11 13
@@ -18,22 +20,38 @@ pub(crate) enum Pane {
18 #[derive(Debug, PartialEq)] 20 #[derive(Debug, PartialEq)]
19 pub(crate) enum ViewMode { 21 pub(crate) enum ViewMode {
20 Details, 22 Details,
21 CommitList, 23 /// The issue's event stream, listed. Titled `Event History` on screen; it
22 CommitDetail, 24 /// was called `CommitList` until the name was read as "a list of commits"
25 /// by someone who then concluded the wrong thing about the key that opens
26 /// it.
27 EventHistory,
28 /// One event from that stream, in full.
29 EventDetail,
23 PatchDetail, 30 PatchDetail,
24 } 31 }
25 32
33 /// What the key handler needs the event loop to do.
34 ///
35 /// Everything the [`App`] can do to itself it has already done by the time
36 /// this is returned; these are the things that need the repository, the
37 /// terminal, or the loop's own control flow.
26 #[derive(Debug, PartialEq)] 38 #[derive(Debug, PartialEq)]
27 pub(crate) enum KeyAction { 39 pub(crate) enum KeyAction {
28 Continue, 40 Continue,
29 Quit, 41 Quit,
30 Reload, 42 Reload,
31 OpenCommitBrowser, 43 OpenEventHistory,
44 /// Comment on the issue under the cursor.
45 CommentOnIssue,
46 /// Close the issue under the cursor, or reopen it if it is closed.
47 CloseOrReopenIssue,
32 OpenPatchDetail, 48 OpenPatchDetail,
33 OpenPatchDetailDirect(usize), // index into visible_patches 49 OpenPatchDetailDirect(usize), // index into visible_patches
34 /// Comment on the row under the cursor: inline when it is a line of the 50 /// Comment on the row under the cursor: inline when it is a line of the
35 /// diff, on the patch's thread otherwise. 51 /// diff, on the patch's thread otherwise.
36 Comment, 52 Comment,
53 /// A verdict was chosen at the `R` prompt.
54 Review(ReviewVerdict),
37 /// Claim, or withdraw the claim, that the comment under the cursor was 55 /// Claim, or withdraw the claim, that the comment under the cursor was
38 /// answered. 56 /// answered.
39 ToggleResolve, 57 ToggleResolve,
@@ -173,6 +191,12 @@ pub(crate) struct App {
173 /// Events that landed since `tips` was taken. Never acted on by itself — 191 /// Events that landed since `tips` was taken. Never acted on by itself —
174 /// it raises a banner, and the reader reloads when they are ready. 192 /// it raises a banner, and the reader reloads when they are ready.
175 pub(crate) new_events: usize, 193 pub(crate) new_events: usize,
194 /// Whether the `?` overlay is up.
195 ///
196 /// An overlay rather than a `ViewMode`, because it is drawn over a pane
197 /// and lists that pane's keys: the thing underneath has to still be the
198 /// current surface while it is up.
199 pub(crate) show_help: bool,
176 } 200 }
177 201
178 impl App { 202 impl App {
@@ -226,6 +250,7 @@ impl App {
226 loaded_at: Local::now(), 250 loaded_at: Local::now(),
227 tips: Tips::default(), 251 tips: Tips::default(),
228 new_events: 0, 252 new_events: 0,
253 show_help: false,
229 } 254 }
230 } 255 }
231 256
@@ -439,249 +464,206 @@ impl App {
439 .find(|p| p.fixes.as_deref() == Some(&issue.id)) 464 .find(|p| p.fixes.as_deref() == Some(&issue.id))
440 } 465 }
441 466
467 /// The issue under the cursor, whichever of the two issue panes has focus.
468 ///
469 /// The selection is a property of the list, not of which pane is
470 /// highlighted, and that is why `c` is bound in both: the old `pane ==
471 /// Pane::Detail` guard treated focus as if it decided what the key had to
472 /// work with.
473 pub(crate) fn selected_issue(&self) -> Option<&IssueState> {
474 let idx = self.list_state.selected()?;
475 self.visible_issues().get(idx).copied()
476 }
477
478 /// Turn a keypress into what it means here, and do it.
479 ///
480 /// One lookup in one table, in place of fifty-seven `KeyCode::` arms
481 /// nested by mode and pane. A key the current context does not bind
482 /// arrives here as `None`, and says so — the structure this replaces sent
483 /// "not bound" and "bound but gated out" to the same silent
484 /// `_ => KeyAction::Continue`, which is how `c` came to do nothing at all
485 /// on the issue list without anyone being able to tell.
442 pub(crate) fn handle_key(&mut self, code: KeyCode, modifiers: KeyModifiers) -> KeyAction { 486 pub(crate) fn handle_key(&mut self, code: KeyCode, modifiers: KeyModifiers) -> KeyAction {
443 // Handle PatchDetail mode 487 let key = Key {
444 if self.mode == ViewMode::PatchDetail { 488 code,
445 match code { 489 ctrl: modifiers.contains(KeyModifiers::CONTROL),
446 KeyCode::Esc => { 490 };
447 self.mode = ViewMode::Details; 491 let context = Context::of(self);
448 self.pane = Pane::ItemList; 492 match keys::lookup(context, key) {
449 self.current_patch = None; 493 Some(action) => self.apply(action),
450 self.patch_diff.clear(); 494 None => self.unbound(context, key),
451 self.patch_lines.clear();
452 self.patch_rows.clear();
453 self.patch_revision_idx = 0;
454 self.patch_interdiff_mode = false;
455 self.reset_patch_view();
456 return KeyAction::Continue;
457 }
458 KeyCode::Char('q') => return KeyAction::Quit,
459 KeyCode::Char('c') if modifiers.contains(KeyModifiers::CONTROL) => {
460 return KeyAction::Quit;
461 }
462 // Rows, not lines, and the cursor stays where it is — the only
463 // way through a line that wrapped taller than the pane.
464 KeyCode::Char('e') if modifiers.contains(KeyModifiers::CONTROL) => {
465 self.scroll_rows(1);
466 return KeyAction::Continue;
467 }
468 KeyCode::Char('y') if modifiers.contains(KeyModifiers::CONTROL) => {
469 self.scroll_rows(-1);
470 return KeyAction::Continue;
471 }
472 KeyCode::Char('j') | KeyCode::Down => {
473 self.move_cursor(1);
474 return KeyAction::Continue;
475 }
476 KeyCode::Char('k') | KeyCode::Up => {
477 self.move_cursor(-1);
478 return KeyAction::Continue;
479 }
480 KeyCode::PageDown => {
481 self.move_cursor(20);
482 return KeyAction::Continue;
483 }
484 KeyCode::PageUp => {
485 self.move_cursor(-20);
486 return KeyAction::Continue;
487 }
488 // ── The review loop ──────────────────────────────────────
489 KeyCode::Char('c') => return KeyAction::Comment,
490 KeyCode::Char('R') => {
491 self.input_mode = InputMode::ReviewVerdict;
492 return KeyAction::Continue;
493 }
494 KeyCode::Char('x') => return KeyAction::ToggleResolve,
495 KeyCode::Char('a') => return KeyAction::ShowAnswers,
496 KeyCode::Char('o') => return KeyAction::Checkout,
497 KeyCode::Char(']') => {
498 if let Some(ref patch) = self.current_patch {
499 let max = patch.revisions.len().saturating_sub(1);
500 if self.patch_revision_idx < max {
501 self.patch_revision_idx += 1;
502 self.reset_patch_view();
503 return KeyAction::Reload; // signal to regenerate diff
504 }
505 }
506 return KeyAction::Continue;
507 }
508 KeyCode::Char('[') => {
509 if self.patch_revision_idx > 0 {
510 self.patch_revision_idx -= 1;
511 self.reset_patch_view();
512 return KeyAction::Reload; // signal to regenerate diff
513 }
514 return KeyAction::Continue;
515 }
516 KeyCode::Char('d') => {
517 self.patch_interdiff_mode = !self.patch_interdiff_mode;
518 self.reset_patch_view();
519 return KeyAction::Reload; // signal to regenerate diff
520 }
521 // The escape hatch for reading `+`/`-` in column. Deliberately
522 // not reset by `reset_patch_view`: a reader who turned wrapping
523 // off did not mean "until the next revision".
524 KeyCode::Char('w') => {
525 self.patch_wrap = !self.patch_wrap;
526 self.rewrap();
527 return KeyAction::Continue;
528 }
529 _ => return KeyAction::Continue,
530 }
531 } 495 }
496 }
532 497
533 // Handle CommitDetail mode first 498 /// What happens when a key means nothing here.
534 if self.mode == ViewMode::CommitDetail { 499 ///
535 match code { 500 /// A sentence, not silence. The reader is told which key they pressed,
536 KeyCode::Esc => { 501 /// that this pane does not use it, and where the pane's keys are — because
537 self.mode = ViewMode::CommitList; 502 /// the failure being fixed is not that the key did nothing, it is that the
538 self.scroll = 0; 503 /// reader could not tell "nothing is bound" from "the binding is broken".
539 return KeyAction::Continue; 504 fn unbound(&mut self, context: Context, key: Key) -> KeyAction {
540 } 505 match context {
541 KeyCode::Char('q') => return KeyAction::Quit, 506 // Any key dismisses the overlay: it is covering the thing the
542 KeyCode::Char('c') if modifiers.contains(KeyModifiers::CONTROL) => { 507 // reader wants, so getting rid of it is the one universally right
543 return KeyAction::Quit; 508 // answer, and it is a visible effect rather than silence.
544 } 509 Context::Help => {
545 KeyCode::Char('j') | KeyCode::Down => { 510 self.show_help = false;
546 self.scroll = self.scroll.saturating_add(1); 511 KeyAction::Continue
547 return KeyAction::Continue;
548 }
549 KeyCode::Char('k') | KeyCode::Up => {
550 self.scroll = self.scroll.saturating_sub(1);
551 return KeyAction::Continue;
552 }
553 KeyCode::PageDown => {
554 self.scroll = self.scroll.saturating_add(20);
555 return KeyAction::Continue;
556 }
557 KeyCode::PageUp => {
558 self.scroll = self.scroll.saturating_sub(20);
559 return KeyAction::Continue;
560 }
561 _ => return KeyAction::Continue,
562 } 512 }
563 } 513 // A modal prompt that swallowed the key would leave the reader in
564 514 // it with no sign of why. Cancelling and saying so is the honest
565 // Handle CommitList mode 515 // reading of "that was not one of the three".
566 if self.mode == ViewMode::CommitList { 516 Context::ReviewVerdict => {
567 match code { 517 self.input_mode = InputMode::Normal;
568 KeyCode::Esc => { 518 self.status_msg = Some(format!(
569 self.event_history.clear(); 519 "{} is not a verdict — review abandoned. a approves, r requests changes, \
570 self.event_list_state = ListState::default(); 520 c comments.",
571 self.mode = ViewMode::Details; 521 key.name()
572 self.scroll = 0; 522 ));
573 return KeyAction::Continue; 523 KeyAction::Continue
574 } 524 }
575 KeyCode::Char('q') => return KeyAction::Quit, 525 _ => {
576 KeyCode::Char('c') if modifiers.contains(KeyModifiers::CONTROL) => { 526 self.status_msg = Some(format!(
577 return KeyAction::Quit; 527 "{} does nothing on {} — press ? for this pane's keys.",
578 } 528 key.name(),
579 KeyCode::Char('j') | KeyCode::Down => { 529 context.title()
580 let len = self.event_history.len(); 530 ));
581 if len > 0 { 531 KeyAction::Continue
582 let current = self.event_list_state.selected().unwrap_or(0);
583 let new = (current + 1).min(len - 1);
584 self.event_list_state.select(Some(new));
585 }
586 return KeyAction::Continue;
587 }
588 KeyCode::Char('k') | KeyCode::Up => {
589 if !self.event_history.is_empty() {
590 let current = self.event_list_state.selected().unwrap_or(0);
591 let new = current.saturating_sub(1);
592 self.event_list_state.select(Some(new));
593 }
594 return KeyAction::Continue;
595 }
596 KeyCode::Enter => {
597 if self.event_list_state.selected().is_some() {
598 self.mode = ViewMode::CommitDetail;
599 self.scroll = 0;
600 }
601 return KeyAction::Continue;
602 }
603 _ => return KeyAction::Continue,
604 } 532 }
605 } 533 }
534 }
606 535
607 // Normal Details mode handling 536 /// Carry out one meaning.
608 match code { 537 ///
609 KeyCode::Char('q') | KeyCode::Esc => KeyAction::Quit, 538 /// Exhaustive over [`Action`] on purpose: a new binding cannot be added to
610 KeyCode::Char('c') if modifiers.contains(KeyModifiers::CONTROL) => KeyAction::Quit, 539 /// the table and left doing nothing, because this will not compile until
611 KeyCode::Char('P') => { 540 /// it is given a meaning here.
612 // Toggle between Issues and Patches list mode 541 fn apply(&mut self, action: Action) -> KeyAction {
613 self.list_mode = match self.list_mode { 542 match action {
614 ListMode::Issues => ListMode::Patches, 543 Action::Quit => KeyAction::Quit,
615 ListMode::Patches => ListMode::Issues, 544 Action::Back => {
616 }; 545 self.go_back();
617 self.scroll = 0;
618 self.pane = Pane::ItemList;
619 self.mode = ViewMode::Details;
620 KeyAction::Continue 546 KeyAction::Continue
621 } 547 }
622 KeyCode::Char('c') => { 548
623 // Open commit browser: only when in detail pane with an item selected 549 // ── Moving ──────────────────────────────────────────────────
624 if self.pane == Pane::Detail 550 Action::SelectNext => {
625 && self.list_mode == ListMode::Issues 551 self.move_selection(1);
626 && self.list_state.selected().is_some() 552 KeyAction::Continue
627 {
628 KeyAction::OpenCommitBrowser
629 } else {
630 KeyAction::Continue
631 }
632 } 553 }
633 KeyCode::Char('p') => { 554 Action::SelectPrev => {
634 // Open patch detail: only when in detail pane with a linked patch (issues mode) 555 self.move_selection(-1);
635 if self.pane == Pane::Detail 556 KeyAction::Continue
636 && self.list_mode == ListMode::Issues
637 && self.linked_patch_for_selected().is_some()
638 {
639 KeyAction::OpenPatchDetail
640 } else {
641 KeyAction::Continue
642 }
643 } 557 }
644 KeyCode::Char('j') | KeyCode::Down => { 558 Action::ScrollDown => {
645 if self.pane == Pane::ItemList { 559 self.scroll = self.scroll.saturating_add(1);
646 self.move_selection(1);
647 } else {
648 self.scroll = self.scroll.saturating_add(1);
649 }
650 KeyAction::Continue 560 KeyAction::Continue
651 } 561 }
652 KeyCode::Char('k') | KeyCode::Up => { 562 Action::ScrollUp => {
653 if self.pane == Pane::ItemList { 563 self.scroll = self.scroll.saturating_sub(1);
654 self.move_selection(-1);
655 } else {
656 self.scroll = self.scroll.saturating_sub(1);
657 }
658 KeyAction::Continue 564 KeyAction::Continue
659 } 565 }
660 KeyCode::PageDown => { 566 Action::ScrollPageDown => {
661 self.scroll = self.scroll.saturating_add(20); 567 self.scroll = self.scroll.saturating_add(20);
662 KeyAction::Continue 568 KeyAction::Continue
663 } 569 }
664 KeyCode::PageUp => { 570 Action::ScrollPageUp => {
665 self.scroll = self.scroll.saturating_sub(20); 571 self.scroll = self.scroll.saturating_sub(20);
666 KeyAction::Continue 572 KeyAction::Continue
667 } 573 }
668 KeyCode::Tab | KeyCode::Enter => { 574 Action::CursorDown => {
669 // In Patches list mode, entering detail pane opens patch detail directly 575 self.move_cursor(1);
670 if self.list_mode == ListMode::Patches && self.pane == Pane::ItemList { 576 KeyAction::Continue
671 if let Some(idx) = self.patch_list_state.selected() { 577 }
672 let visible_len = self.visible_patches().len(); 578 Action::CursorUp => {
673 if idx < visible_len { 579 self.move_cursor(-1);
674 return KeyAction::OpenPatchDetailDirect(idx); 580 KeyAction::Continue
675 } 581 }
676 } 582 Action::CursorPageDown => {
583 self.move_cursor(20);
584 KeyAction::Continue
585 }
586 Action::CursorPageUp => {
587 self.move_cursor(-20);
588 KeyAction::Continue
589 }
590 Action::ScrollRowDown => {
591 self.scroll_rows(1);
592 KeyAction::Continue
593 }
594 Action::ScrollRowUp => {
595 self.scroll_rows(-1);
596 KeyAction::Continue
597 }
598 Action::EventNext => {
599 let len = self.event_history.len();
600 if len > 0 {
601 let current = self.event_list_state.selected().unwrap_or(0);
602 self.event_list_state.select(Some((current + 1).min(len - 1)));
603 }
604 KeyAction::Continue
605 }
606 Action::EventPrev => {
607 if !self.event_history.is_empty() {
608 let current = self.event_list_state.selected().unwrap_or(0);
609 self.event_list_state.select(Some(current.saturating_sub(1)));
610 }
611 KeyAction::Continue
612 }
613 Action::OpenEvent => {
614 if self.event_list_state.selected().is_some() {
615 self.mode = ViewMode::EventDetail;
616 self.scroll = 0;
677 } 617 }
618 KeyAction::Continue
619 }
620 Action::TogglePane => {
678 self.pane = match self.pane { 621 self.pane = match self.pane {
679 Pane::ItemList => Pane::Detail, 622 Pane::ItemList => Pane::Detail,
680 Pane::Detail => Pane::ItemList, 623 Pane::Detail => Pane::ItemList,
681 }; 624 };
682 KeyAction::Continue 625 KeyAction::Continue
683 } 626 }
684 KeyCode::Char('a') => { 627
628 // ── Opening ─────────────────────────────────────────────────
629 Action::OpenSelectedPatch => match self.patch_list_state.selected() {
630 Some(idx) if idx < self.visible_patches().len() => {
631 KeyAction::OpenPatchDetailDirect(idx)
632 }
633 // Nothing to open: fall back to moving focus, which is what
634 // the pane beside an empty list is for.
635 _ => self.apply(Action::TogglePane),
636 },
637 Action::OpenLinkedPatch => {
638 if self.linked_patch_for_selected().is_some() {
639 KeyAction::OpenPatchDetail
640 } else {
641 self.status_msg =
642 Some("No patch is linked to this issue yet.".to_string());
643 KeyAction::Continue
644 }
645 }
646 Action::OpenEventHistory => {
647 if self.selected_item_id().is_some() {
648 KeyAction::OpenEventHistory
649 } else {
650 self.status_msg = Some("No issue is selected.".to_string());
651 KeyAction::Continue
652 }
653 }
654
655 // ── Filtering the lists ─────────────────────────────────────
656 Action::SwitchList => {
657 self.list_mode = match self.list_mode {
658 ListMode::Issues => ListMode::Patches,
659 ListMode::Patches => ListMode::Issues,
660 };
661 self.scroll = 0;
662 self.pane = Pane::ItemList;
663 self.mode = ViewMode::Details;
664 KeyAction::Continue
665 }
666 Action::CycleStatusFilter => {
685 self.status_filter = self.status_filter.next(); 667 self.status_filter = self.status_filter.next();
686 let count = self.visible_count(); 668 let count = self.visible_count();
687 let state = match self.list_mode { 669 let state = match self.list_mode {
@@ -691,9 +673,7 @@ impl App {
691 state.select(if count > 0 { Some(0) } else { None }); 673 state.select(if count > 0 { Some(0) } else { None });
692 KeyAction::Continue 674 KeyAction::Continue
693 } 675 }
694 // Patches only: issues carry no unresolved count, so on that pane 676 Action::ToggleUnresolvedOnly => {
695 // this would silently do nothing to a list the user is looking at.
696 KeyCode::Char('u') if self.list_mode == ListMode::Patches => {
697 self.unresolved_only = !self.unresolved_only; 677 self.unresolved_only = !self.unresolved_only;
698 // The row under the cursor may have just left the list; the 678 // The row under the cursor may have just left the list; the
699 // status filter resets the selection for the same reason. 679 // status filter resets the selection for the same reason.
@@ -702,8 +682,129 @@ impl App {
702 .select(if count > 0 { Some(0) } else { None }); 682 .select(if count > 0 { Some(0) } else { None });
703 KeyAction::Continue 683 KeyAction::Continue
704 } 684 }
705 KeyCode::Char('r') => KeyAction::Reload, 685 Action::Reload => KeyAction::Reload,
706 _ => KeyAction::Continue, 686 Action::BeginSearch => {
687 self.input_mode = InputMode::Search;
688 self.search_query.clear();
689 KeyAction::Continue
690 }
691 Action::BeginCreateIssue => {
692 self.input_mode = InputMode::CreateTitle;
693 self.input_buf.clear();
694 self.create_title.clear();
695 KeyAction::Continue
696 }
697
698 // ── Writing ─────────────────────────────────────────────────
699 Action::CommentOnIssue => {
700 if self.selected_item_id().is_some() {
701 KeyAction::CommentOnIssue
702 } else {
703 self.status_msg = Some("No issue is selected to comment on.".to_string());
704 KeyAction::Continue
705 }
706 }
707 Action::CloseOrReopenIssue => {
708 if self.selected_item_id().is_some() {
709 KeyAction::CloseOrReopenIssue
710 } else {
711 self.status_msg = Some("No issue is selected.".to_string());
712 KeyAction::Continue
713 }
714 }
715 Action::CommentOnPatch => KeyAction::Comment,
716 Action::BeginReviewVerdict => {
717 self.input_mode = InputMode::ReviewVerdict;
718 KeyAction::Continue
719 }
720 Action::ToggleResolve => KeyAction::ToggleResolve,
721 Action::ShowAnswers => KeyAction::ShowAnswers,
722 Action::Checkout => KeyAction::Checkout,
723 Action::VerdictApprove => self.verdict(ReviewVerdict::Approve),
724 Action::VerdictRequestChanges => self.verdict(ReviewVerdict::RequestChanges),
725 Action::VerdictComment => self.verdict(ReviewVerdict::Comment),
726 Action::CancelVerdict => {
727 self.input_mode = InputMode::Normal;
728 self.status_msg = Some("Review abandoned.".to_string());
729 KeyAction::Continue
730 }
731
732 // ── The patch detail pane's own view state ──────────────────
733 Action::NextRevision => {
734 if let Some(ref patch) = self.current_patch {
735 let max = patch.revisions.len().saturating_sub(1);
736 if self.patch_revision_idx < max {
737 self.patch_revision_idx += 1;
738 self.reset_patch_view();
739 return KeyAction::Reload; // signal to regenerate diff
740 }
741 }
742 KeyAction::Continue
743 }
744 Action::PrevRevision => {
745 if self.patch_revision_idx > 0 {
746 self.patch_revision_idx -= 1;
747 self.reset_patch_view();
748 return KeyAction::Reload; // signal to regenerate diff
749 }
750 KeyAction::Continue
751 }
752 Action::ToggleInterdiff => {
753 self.patch_interdiff_mode = !self.patch_interdiff_mode;
754 self.reset_patch_view();
755 KeyAction::Reload // signal to regenerate diff
756 }
757 // The escape hatch for reading `+`/`-` in column. Deliberately
758 // not reset by `reset_patch_view`: a reader who turned wrapping
759 // off did not mean "until the next revision".
760 Action::ToggleWrap => {
761 self.patch_wrap = !self.patch_wrap;
762 self.rewrap();
763 KeyAction::Continue
764 }
765
766 // ── The overlay ─────────────────────────────────────────────
767 Action::ShowHelp => {
768 self.show_help = true;
769 KeyAction::Continue
770 }
771 Action::CloseHelp => {
772 self.show_help = false;
773 KeyAction::Continue
774 }
775 }
776 }
777
778 fn verdict(&mut self, verdict: ReviewVerdict) -> KeyAction {
779 self.input_mode = InputMode::Normal;
780 KeyAction::Review(verdict)
781 }
782
783 /// Leave the view on screen for the one behind it.
784 fn go_back(&mut self) {
785 match self.mode {
786 ViewMode::PatchDetail => {
787 self.mode = ViewMode::Details;
788 self.pane = Pane::ItemList;
789 self.current_patch = None;
790 self.patch_diff.clear();
791 self.patch_lines.clear();
792 self.patch_rows.clear();
793 self.patch_revision_idx = 0;
794 self.patch_interdiff_mode = false;
795 self.reset_patch_view();
796 }
797 ViewMode::EventDetail => {
798 self.mode = ViewMode::EventHistory;
799 self.scroll = 0;
800 }
801 ViewMode::EventHistory => {
802 self.event_history.clear();
803 self.event_list_state = ListState::default();
804 self.mode = ViewMode::Details;
805 self.scroll = 0;
806 }
807 ViewMode::Details => {}
707 } 808 }
708 } 809 }
709 810
@@ -765,10 +866,16 @@ impl App {
765 866
766 pub(crate) fn reload(&mut self, repo: &Repository) { 867 pub(crate) fn reload(&mut self, repo: &Repository) {
767 self.rebaseline(repo); 868 self.rebaseline(repo);
768 if let Ok(issues) = state::list_issues(repo) { 869 // Archived objects included, because the status filter offers "closed"
870 // and "all" and a filter has to be able to reach what it names.
871 // Closing archives the ref, so loading with `list_issues` made those
872 // two settings of `a` structurally empty — the same lie the footer was
873 // telling, in a different place. `issue list -a` and the web UI have
874 // always used these listers; `visible_issues` does the narrowing.
875 if let Ok(issues) = state::list_issues_with_archived(repo) {
769 self.issues = issues; 876 self.issues = issues;
770 } 877 }
771 if let Ok(patches) = state::list_patches(repo) { 878 if let Ok(patches) = state::list_patches_with_archived(repo) {
772 self.patches = patches; 879 self.patches = patches;
773 } 880 }
774 // Refreshing the lists without refreshing the widths would leave the 881 // Refreshing the lists without refreshing the widths would leave the
src/tui/widgets.rs
Old New
@@ -1,12 +1,12 @@
1 use git2::{Oid, Repository}; 1 use git2::{Oid, Repository};
2 use ratatui::prelude::*; 2 use ratatui::prelude::*;
3 use ratatui::widgets::{Block, Borders, List, ListItem, Paragraph, Wrap}; 3 use ratatui::widgets::{Block, Borders, Clear, List, ListItem, Paragraph, Wrap};
4 4
5 use crate::abbrev::Abbrev; 5 use crate::abbrev::Abbrev;
6 use crate::event::{Action, ReviewVerdict}; 6 use crate::event::{Action, ReviewVerdict};
7 use crate::state::{IssueState, IssueStatus, PatchState, PatchStatus}; 7 use crate::state::{IssueState, IssueStatus, PatchState, PatchStatus};
8 8
9 use super::state::{App, InputMode, ListMode, Pane, StatusFilter, ViewMode}; 9 use super::state::{App, InputMode, ListMode, Pane, ViewMode};
10 10
11 /// A stored timestamp as the dashboard shows it: `YYYY-MM-DD HH:MM`, the same 11 /// A stored timestamp as the dashboard shows it: `YYYY-MM-DD HH:MM`, the same
12 /// rendering the web settled on in `b2a57996`. 12 /// rendering the web settled on in `b2a57996`.
@@ -80,7 +80,7 @@ pub(crate) fn action_type_label(action: &Action) -> &str {
80 } 80 }
81 } 81 }
82 82
83 /// Render one raw event for the commit browser. 83 /// Render one raw event for the Event History pane.
84 /// 84 ///
85 /// `issue_abbrev` governs the issue ids in the payload. The event's own commit 85 /// `issue_abbrev` governs the issue ids in the payload. The event's own commit
86 /// oid, and the `target` of an edit or a delete, are git object ids naming an 86 /// oid, and the `target` of an edit or a delete, are git object ids naming an
@@ -454,6 +454,71 @@ pub(crate) fn ui(frame: &mut Frame, app: &mut App, repo: Option<&Repository>) {
454 render_list(frame, app, panes[0]); 454 render_list(frame, app, panes[0]);
455 render_detail(frame, app, panes[1], repo); 455 render_detail(frame, app, panes[1], repo);
456 render_footer(frame, app, footer_area); 456 render_footer(frame, app, footer_area);
457 if app.show_help {
458 render_help(frame, app, main_area);
459 }
460 }
461
462 /// Every key of the pane underneath, generated from the binding table.
463 ///
464 /// This surface grew `c`, `R`, `x`, `a`, `w`, `o`, `n`, `u`, `P`, `Ctrl-E` and
465 /// `Ctrl-Y` without ever telling anyone, because the only place they were
466 /// advertised was a one-line footer that could not hold them. Being generated
467 /// is what makes this list free and what stops it going stale: there is
468 /// nothing here to update when a binding changes.
469 fn render_help(frame: &mut Frame, app: &App, area: Rect) {
470 let context = super::keys::Context::surface(app);
471 let rows = super::keys::overlay(app);
472 // The key column is right-aligned to the widest key, so the box has to be
473 // sized against that width and the widest label, not against the widest
474 // row: the two maxima are rarely on the same line.
475 let keys_width = rows
476 .iter()
477 .map(|(keys, _)| keys.chars().count())
478 .max()
479 .unwrap_or(0);
480 let label_width = rows
481 .iter()
482 .map(|(_, label)| label.chars().count())
483 .max()
484 .unwrap_or(0);
485 // ` <keys> <label>` inside a border on each side.
486 let width = (keys_width + label_width + 4)
487 .max(context.title().chars().count() + 9) as u16;
488 let width = width.min(area.width);
489 let height = (rows.len() as u16 + 2).min(area.height);
490
491 // Centred, and clipped by the pane rather than overflowing it.
492 let x = area.x + (area.width.saturating_sub(width)) / 2;
493 let y = area.y + (area.height.saturating_sub(height)) / 2;
494 let popup = Rect {
495 x,
496 y,
497 width,
498 height,
499 };
500
501 let lines: Vec<Line> = rows
502 .iter()
503 .map(|(keys, label)| {
504 Line::from(vec![
505 Span::styled(
506 format!(" {:>width$} ", keys, width = keys_width),
507 Style::default()
508 .fg(Color::Yellow)
509 .add_modifier(Modifier::BOLD),
510 ),
511 Span::raw(label.clone()),
512 ])
513 })
514 .collect();
515
516 frame.render_widget(Clear, popup);
517 let block = Block::default()
518 .borders(Borders::ALL)
519 .title(format!("Keys — {}", context.title()))
520 .border_style(Style::default().fg(Color::Yellow));
521 frame.render_widget(Paragraph::new(Text::from(lines)).block(block), popup);
457 } 522 }
458 523
459 /// When the screen was loaded, and whether anything has happened since. 524 /// When the screen was loaded, and whether anything has happened since.
@@ -653,8 +718,8 @@ fn render_detail(frame: &mut Frame, app: &mut App, area: Rect, repo: Option<&Rep
653 return; 718 return;
654 } 719 }
655 720
656 // Handle commit browser modes 721 // The Event History panes
657 if app.mode == ViewMode::CommitList { 722 if app.mode == ViewMode::EventHistory {
658 let items: Vec<ListItem> = app 723 let items: Vec<ListItem> = app
659 .event_history 724 .event_history
660 .iter() 725 .iter()
@@ -687,7 +752,7 @@ fn render_detail(frame: &mut Frame, app: &mut App, area: Rect, repo: Option<&Rep
687 return; 752 return;
688 } 753 }
689 754
690 if app.mode == ViewMode::CommitDetail { 755 if app.mode == ViewMode::EventDetail {
691 let content = if let Some(idx) = app.event_list_state.selected() { 756 let content = if let Some(idx) = app.event_list_state.selected() {
692 if let Some((oid, evt)) = app.event_history.get(idx) { 757 if let Some((oid, evt)) = app.event_history.get(idx) {
693 format_event_detail(oid, evt, &app.issue_abbrev) 758 format_event_detail(oid, evt, &app.issue_abbrev)
@@ -1344,10 +1409,8 @@ fn render_footer(frame: &mut Frame, app: &App, area: Rect) {
1344 return; 1409 return;
1345 } 1410 }
1346 InputMode::ReviewVerdict => { 1411 InputMode::ReviewVerdict => {
1347 let para = Paragraph::new( 1412 let para = Paragraph::new(format!(" Review verdict: {}", super::keys::footer(app)))
1348 " Review verdict: a:approve r:request-changes c:comment Esc:cancel", 1413 .style(Style::default().bg(Color::Blue).fg(Color::White));
1349 )
1350 .style(Style::default().bg(Color::Blue).fg(Color::White));
1351 frame.render_widget(para, area); 1414 frame.render_widget(para, area);
1352 return; 1415 return;
1353 } 1416 }
@@ -1362,39 +1425,15 @@ fn render_footer(frame: &mut Frame, app: &App, area: Rect) {
1362 return; 1425 return;
1363 } 1426 }
1364 1427
1365 let text = match app.mode { 1428 // Generated from the binding table, never written down twice. This line
1366 ViewMode::CommitList => " j/k:navigate Enter:detail Esc:back q:quit".to_string(), 1429 // was a set of string literals kept beside the arms they described, which
1367 ViewMode::CommitDetail => " j/k:scroll Esc:back q:quit".to_string(), 1430 // is how it came to advertise `c:events` for a key that had by then also
1368 // Ordered by what a reviewer reaches for, because this is one line and 1431 // become the comment key one pane over.
1369 // a narrow terminal clips the tail: the review loop first, then the 1432 //
1370 // way out, then the things you can also get to by other means. 1433 // It is one line and a narrow terminal clips the tail, so the table's
1371 ViewMode::PatchDetail => { 1434 // order is its priority order — and `?:keys` leads, because when a line
1372 " j/k:line c:comment R:review x:resolve a:answers Esc:back q:quit \ 1435 // cannot hold the surface its first duty is to name the key that can.
1373 [/]:revision d:interdiff o:checkout w:wrap" 1436 let text = format!(" {}", super::keys::footer(app));
1374 .to_string()
1375 }
1376 ViewMode::Details => {
1377 let list_hint = match app.list_mode {
1378 ListMode::Issues => "P:patches",
1379 ListMode::Patches => "P:issues",
1380 };
1381 let filter_hint = match app.status_filter {
1382 StatusFilter::Open => "a:show all",
1383 StatusFilter::All => "a:closed",
1384 StatusFilter::Closed => "a:open only",
1385 };
1386 let mode_hint = match app.list_mode {
1387 ListMode::Issues => " c:events p:patch",
1388 // Advertised only where it does something: issues carry no
1389 // unresolved count, so `u` is inert on that pane.
1390 ListMode::Patches => " Enter:view patch u:unresolved",
1391 };
1392 format!(
1393 " j/k:navigate Tab:pane {} {}{} /:search r:refresh q:quit",
1394 list_hint, filter_hint, mode_hint
1395 )
1396 }
1397 };
1398 let para = Paragraph::new(text).style(Style::default().bg(Color::DarkGray).fg(Color::White)); 1437 let para = Paragraph::new(text).style(Style::default().bg(Color::DarkGray).fg(Color::White));
1399 frame.render_widget(para, area); 1438 frame.render_widget(para, area);
1400 } 1439 }
tests/tui_review_test.rs
Old New
@@ -538,3 +538,237 @@ fn no_editor_configured_says_so() {
538 "the dashboard hung or died with no editor configured" 538 "the dashboard hung or died with no editor configured"
539 ); 539 );
540 } 540 }
541
542 /// A captured screen with its spacing removed.
543 ///
544 /// ratatui redraws only the cells that changed and skips the rest with a
545 /// cursor move, which `screen_text` strips — so a run of spaces between two
546 /// words can vanish from the capture while both words are genuinely on screen.
547 /// Comparing without spaces asks what these tests actually mean: is this text
548 /// on the screen, in this order.
549 fn squeezed(bytes: &[u8]) -> String {
550 screen_text(bytes).replace(' ', "")
551 }
552
553 // ── The issue side of the review loop (d7158619 / 02d3eb34) ─────────────────
554
555 /// The report that opened `d7158619`: a reader opened the dashboard on an
556 /// issue and could not answer it. The CLI could — `git-collab issue comment
557 /// <id> -b …` — so this is the binding and the target resolution, not the
558 /// write, and it goes through the same `$EDITOR` suspend the patch side uses.
559 #[test]
560 fn c_comments_on_the_issue_under_the_cursor() {
561 let repo = TestRepo::new("Reader", "reader@example.com");
562 let id = repo.issue_open("something worth answering");
563 let editor = editor_writing(&repo, "issue-comment.sh", "answering it from the dashboard");
564
565 repo.run_dashboard_driven(100, 30, &[("EDITOR", &editor)], |dash| {
566 dash.press("c", "Comment added on issue");
567 dash.send("q");
568 });
569
570 let shown = repo.run_ok(&["issue", "show", &id]);
571 assert!(
572 shown.contains("answering it from the dashboard"),
573 "the editor's body did not reach the issue:\n{}",
574 shown
575 );
576 }
577
578 /// And from the list pane, where the old `pane == Pane::Detail` guard sent `c`
579 /// into silence. The dashboard opens focused on the list, so this is the press
580 /// the reporter actually made.
581 #[test]
582 fn c_comments_from_the_list_pane_where_it_used_to_do_nothing() {
583 let repo = TestRepo::new("Reader", "reader@example.com");
584 let id = repo.issue_open("pressed from the list");
585 let editor = editor_writing(&repo, "list-comment.sh", "no tab press was needed");
586
587 repo.run_dashboard_driven(100, 30, &[("EDITOR", &editor)], |dash| {
588 // No `Tab`: focus is on the list, which is where `c` used to be inert.
589 dash.press("c", "Comment added on issue");
590 dash.send("q");
591 });
592
593 let shown = repo.run_ok(&["issue", "show", &id]);
594 assert!(
595 shown.contains("no tab press was needed"),
596 "`c` on the list pane recorded nothing:\n{}",
597 shown
598 );
599 }
600
601 /// The other half of the collision: the Event History is still reachable, on
602 /// `e`.
603 #[test]
604 fn e_opens_the_event_history() {
605 let repo = TestRepo::new("Reader", "reader@example.com");
606 repo.issue_open("has a history");
607
608 let out = repo.run_dashboard_driven(100, 30, &[], |dash| {
609 // The pane's rows rather than its title: see `squeezed`.
610 dash.press("e", "Issue Open |");
611 dash.send("q");
612 });
613 assert!(
614 squeezed(&out.stdout).contains("EventHistory"),
615 "`e` did not open the Event History:\n{}",
616 screen_text(&out.stdout)
617 );
618 }
619
620 /// An empty buffer abandons an issue comment, the way it does everywhere else
621 /// this program asks for prose, and the reader is told.
622 #[test]
623 fn an_empty_issue_comment_aborts_and_records_nothing() {
624 let repo = TestRepo::new("Reader", "reader@example.com");
625 let id = repo.issue_open("left alone");
626 let editor = repo.write_script("noop.sh", "#!/bin/sh\nexit 0\n");
627
628 let out = repo.run_dashboard_driven(100, 30, &[("EDITOR", &editor)], |dash| {
629 dash.press("c", "Aborting");
630 dash.send("q");
631 });
632
633 let shown = repo.run_ok(&["issue", "show", &id]);
634 assert!(
635 !shown.contains("--- Comments ---"),
636 "an aborted comment was recorded anyway:\n{}",
637 shown
638 );
639 let text = screen_text(&out.stdout);
640 assert!(
641 text.contains("Aborting"),
642 "nothing said the comment was abandoned:\n{}",
643 text
644 );
645 }
646
647 /// Triage, decided rather than deferred again: `C` closes the issue under the
648 /// cursor with a reason composed in `$EDITOR`.
649 #[test]
650 fn shift_c_closes_the_issue_with_a_reason() {
651 let repo = TestRepo::new("Reader", "reader@example.com");
652 let id = repo.issue_open("done with this one");
653 let editor = editor_writing(&repo, "close-reason.sh", "fixed by the patch above");
654
655 repo.run_dashboard_driven(100, 30, &[("EDITOR", &editor)], |dash| {
656 dash.press("C", "closed");
657 dash.send("q");
658 });
659
660 let shown = repo.run_ok(&["issue", "show", &id]);
661 assert!(
662 shown.contains("[closed]"),
663 "the issue is not closed:\n{}",
664 shown
665 );
666 assert!(
667 shown.contains("fixed by the patch above"),
668 "the reason did not reach the event:\n{}",
669 shown
670 );
671 }
672
673 /// The `a` filter offers "closed" and "all", so it has to be able to show a
674 /// closed issue.
675 ///
676 /// It could not: closing archives the ref, and the dashboard loaded issues
677 /// with `state::list_issues`, which excludes archived ones. So `a` cycled
678 /// through two settings that could only ever produce an empty list — a filter
679 /// naming a state it was structurally unable to reach. `issue list -a` and the
680 /// web UI both use the `_with_archived` listers for exactly this reason; the
681 /// dashboard now does too. Found by trying to reopen from the dashboard.
682 #[test]
683 fn the_status_filter_can_reach_a_closed_issue() {
684 let repo = TestRepo::new("Reader", "reader@example.com");
685 // One word: a title is drawn into cells that were blank, so it survives
686 // the capture whole, while a needle like `(all)` cannot — the `(` of
687 // `Issues (open)` does not change when the word after it does, so it is
688 // never re-emitted and never appears in the stream.
689 let id = repo.issue_open("shut-but-not-gone");
690 repo.issue_close(&id);
691
692 let out = repo.run_dashboard_driven(100, 30, &[], |dash| {
693 dash.press("a", "shut-but-not-gone");
694 dash.send("q");
695 });
696 assert!(
697 squeezed(&out.stdout).contains("shut-but-not-gone"),
698 "the closed issue is unreachable from the dashboard:\n{}",
699 screen_text(&out.stdout)
700 );
701 }
702
703 /// The same key the other way. Reopening asks for nothing, because it records
704 /// no reason and because it is itself the undo of a close.
705 #[test]
706 fn shift_c_reopens_a_closed_issue() {
707 let repo = TestRepo::new("Reader", "reader@example.com");
708 let id = repo.issue_open("closed too early");
709 repo.issue_close(&id);
710
711 repo.run_dashboard_driven(100, 30, &[], |dash| {
712 // Closed issues are hidden by the default filter, so show them first.
713 // No wait between the two: a pty hands keystrokes to the program in
714 // the order they were written, and the wait that matters is the one
715 // on the write `C` makes.
716 dash.send("a");
717 dash.press("C", "reopened");
718 dash.send("q");
719 });
720
721 let shown = repo.run_ok(&["issue", "show", &id]);
722 assert!(
723 shown.contains("[open]"),
724 "the issue did not reopen:\n{}",
725 shown
726 );
727 }
728
729 // ── The bindings, on a real screen (02d3eb34) ───────────────────────────────
730
731 /// A key this pane does not bind says so. The failure this replaces is not
732 /// that the key did nothing; it is that the reader could not tell "nothing is
733 /// bound here" from "the binding is broken".
734 #[test]
735 fn an_unbound_key_says_so_on_screen() {
736 let repo = TestRepo::new("Reader", "reader@example.com");
737 repo.issue_open("look at me");
738
739 let out = repo.run_dashboard_driven(100, 30, &[], |dash| {
740 dash.press("z", "does nothing");
741 dash.send("q");
742 });
743 let text = screen_text(&out.stdout);
744 assert!(
745 text.contains("does nothing") && text.contains('?'),
746 "an unbound key left no trace on screen:\n{}",
747 text
748 );
749 }
750
751 /// `?` lists this pane's keys. The footer is one line and this surface has
752 /// long since outgrown it — `o`, `w`, `Ctrl-E` and the rest have never been
753 /// advertised anywhere.
754 #[test]
755 fn question_mark_lists_this_panes_keys() {
756 let repo = TestRepo::new("Reader", "reader@example.com");
757 repo.issue_open("look at me");
758
759 let out = repo.run_dashboard_driven(100, 30, &[], |dash| {
760 dash.press("?", "Keys");
761 dash.send("q");
762 });
763 let text = squeezed(&out.stdout);
764 assert!(
765 text.contains("Keys"),
766 "no key overlay appeared:\n{}",
767 screen_text(&out.stdout)
768 );
769 assert!(
770 text.contains("checkoutthelinkedpatch"),
771 "the overlay does not list the keys the footer cannot fit:\n{}",
772 screen_text(&out.stdout)
773 );
774 }