Skip to content

Commit 0707113

Browse files
authored
Merge pull request #53 from myrk-kale/refactor/review-reader-only
refactor(tui): keep only the review workspace
2 parents 56ff4e6 + 4b0e664 commit 0707113

21 files changed

Lines changed: 321 additions & 5050 deletions

File tree

‎README.md‎

Lines changed: 15 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -73,16 +73,15 @@ To work with the agent and the reviewer side by side, start the agent through Kl
7373
klaub wrap codex # or: klaub wrap -- claude --model <model>
7474
```
7575

76-
Klaub runs the agent in a session of its own with a reviewer window next to it. Press `Ctrl+B` then `R` to open the reviewer, `Ctrl+B` then `A` to go back to the agent (lower case works too), and `Ctrl+B` then `d` to leave the session running in the background. `klaub session attach` brings it back. `Ctrl+B` then `Q` ends the session and the agent.
76+
Klaub runs the agent in a session of its own with a reviewer window next to it. Press `Shift+→` to go to the reviewer and `Shift+←` to go back to the agent. `Ctrl+B` then `d` leaves the session running in the background, and `klaub session attach` brings it back. `Ctrl+B` then `Q` ends the session and the agent.
7777

78-
### Your first review in six keys
78+
### Your first review in five keys
7979

80-
1. Open the queue with `klaub review`. The riskiest change is first.
80+
1. Open the queue with `klaub review`. The riskiest change is first, and the code of the selected one is beside it.
8181
2. Press `w` to see why it has that priority.
82-
3. Press `Enter` to read the whole function. Press `v` to flip between the old and the new version.
83-
4. Press `i` for the contract and `4` for the callers and the routes that reach the change.
84-
5. Press `c` to comment on a line, and `r` to mark a unit as reviewed.
85-
6. Press `F` to finish. Klaub saves the comments and prepares one short instruction for the agent.
82+
3. Press `Enter` to focus the code, `v` to flip between the old and the new version and `[` and `]` to jump between the changed regions.
83+
4. Press `i` for the contract and `K` for the symbol panel: its signature with the types, its documentation and its priority.
84+
5. Press `r` to mark a unit as reviewed, and `2` or `3` to read the same change as a tree or as a list of files.
8685

8786
Press `?` at any time for the key list.
8887

@@ -265,29 +264,22 @@ Every key below can be changed in `.klaub.toml` under `[keys]` ([configuration](
265264

266265
| Key | Mode |
267266
| --- | --- |
268-
| `1` or `F2` | Review, with the queue on the left |
269-
| `2` or `F3` | Review, with the structure tree on the left: modules, files and symbols |
270-
| `3` | Review, with the flat list of changed files on the left |
271-
| `F4` | Review, with the code focused (`Enter` on a list gets there too) |
272-
| `4`, `g` or `F5` | Impact: callers, entrypoints, callees |
273-
| `5` or `F6` | Comments |
274-
| `6`, `b` or `F8` | Context: brief and acceptance criteria |
275-
| `7`, `t` or `F7` | Tests |
276-
| `8` or `F10` | Follow-up: decide on the previous round's comments |
277-
| `9` | Coverage |
278-
| `0`, `f` or `F9` | Flow |
267+
| `1` or `F2` | The queue on the left, ranked by risk |
268+
| `2` or `F3` | The structure tree on the left: modules, files and symbols |
269+
| `3` | The flat list of changed files on the left |
270+
| `F4` | The code focused (`Enter` on a list gets there too) |
279271
| `Tab`, `Shift+Tab` | Next and previous mode |
280272
| `Esc` | Go back (from the code, to the list you came from) |
281273

282-
The review screen is a list on the left and the code of the selected unit on the right. The queue, the tree and the file list are three ways to fill the left side, and the code follows the cursor as you move in any of them. `K` shows or hides the panel beside the code: the symbol's signature with its types, the documentation comment it carries, the comments you wrote on it and the reasons for its priority.
274+
The screen is a list on the left and the code of the selected unit on the right. The queue, the tree and the file list are three ways to fill the left side, and the code follows the cursor as you move in any of them. `K` shows or hides the panel beside the code: the symbol's signature with its types, the documentation comment it carries and the reasons for its priority.
283275

284276
### Moving and reading
285277

286278
| Key | Action |
287279
| --- | --- |
288280
| `j`, `k` or arrows | Move |
289281
| `h`, `l` | Scroll sideways in code |
290-
| `Enter` | Open the selected unit, symbol or step |
282+
| `Enter` | Open the selected unit |
291283
| `v` | Toggle old and new |
292284
| `B` (hold) | Show the previous version while held, in supported terminals |
293285
| `[`, `]` | Previous and next changed region |
@@ -303,15 +295,11 @@ The review screen is a list on the left and the code of the selected unit on the
303295

304296
| Key | Action |
305297
| --- | --- |
306-
| `c` | Comment on the selection (`V` selects several lines) |
307298
| `w` | Why this priority, and silence a false positive |
308299
| `r` | Mark reviewed or unmark |
309300
| `m`, `,` | Bookmark a unit, jump to the next bookmark |
310301
| `n`, `P` | Next and previous unreviewed unit |
311302
| `N` | Next unreviewed P0 or P1 unit |
312-
| `e`, `d` | In Comments: edit or delete a draft |
313-
| `y`, `p`, `u`, `X` | In Follow-up: resolved, partial, unresolved, regression |
314-
| `F` | Finish the review |
315303
| `R` | Take a new snapshot after the worktree changed |
316304

317305
### Filters and tools
@@ -326,25 +314,22 @@ The review screen is a list on the left and the code of the selected unit on the
326314
| `s` | Minimum reach: local, module or global |
327315
| `M` | Next module |
328316
| `x` | Clear filters |
329-
| `X` | Run the associated tests (asks first) |
330-
| `U` | Reload test reports |
331-
| `L` | Reload the flow diagram |
332-
| `p` | In Impact: pivot on a symbol. In Flow: ask the agent for a diagram |
333317

334318
### Around the agent
335319

336320
These work after `klaub wrap`. Press the prefix (`Ctrl+B` by default), release it, then the key.
337321

338322
| Key | Action |
339323
| --- | --- |
324+
| `Shift+→` | Open the reviewer, with no prefix |
325+
| `Shift+←` | Back to the agent, with no prefix |
340326
| `R` or `r` | Open the reviewer |
341327
| `A` or `a` | Back to the agent |
342-
| `I`, `C`, `T`, `B`, `L` | Open the reviewer on impact, comments, tests, brief or flow |
343328
| `d` | Detach and leave the session running (`klaub session attach` brings it back) |
344329
| `Q` | End the session and its agent, after a `y/n` confirmation |
345330
| `?` | Print these chords |
346331

347-
An optional mouse mode (`[ui] mouse = true`) adds wheel scrolling and clicks on tabs, rows and code lines.
332+
An optional mouse mode (`[ui] mouse = true`) adds wheel scrolling and clicks on rows and code lines.
348333

349334
## Command reference
350335

‎benches/ui.rs‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ use klaub::adapters::storage::{
3333
};
3434
use klaub::app::{OpenOptions, Ports, ReviewSession};
3535
use klaub::domain::config::Config;
36-
use klaub::domain::ports::{Clipboard, IdGenerator};
36+
use klaub::domain::ports::IdGenerator;
3737
use klaub::domain::snapshot::SnapshotRequest;
3838
use klaub::tui::{App, draw, handle_key};
3939
use ratatui::Terminal;
@@ -105,12 +105,7 @@ fn app() -> (TempRepo, App) {
105105
&OpenOptions::default(),
106106
)
107107
.unwrap();
108-
let app = App::new(
109-
session,
110-
Arc::new(TreeSitterParser::new()),
111-
None,
112-
None::<Arc<dyn Clipboard>>,
113-
);
108+
let app = App::new(session, Arc::new(TreeSitterParser::new()));
114109
(repo, app)
115110
}
116111

‎docs/configuration.md‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ Every key has a default, so an empty file (or none) is valid. The schema lives i
3131
| `ui.hold_to_peek` | `false` | Hold `B` (the `peek_previous` key) while reading code to see the previous version, and let go to come back. It needs a terminal that reports key releases (the kitty keyboard protocol: kitty, Ghostty, WezTerm, recent iTerm2 and foot). Where that is missing, which includes tmux, the key says so and `v` still toggles. While it is on, the terminal sends keys as escape codes except inside text fields, which keep ordinary input so compose and `AltGr` characters still type. |
3232
| `integration.submit_mode` | `"manual"` | The only mode. Klaub never presses `Enter` in the agent. |
3333
| `integration.paste_requires_confirmation` | `true` | Putting text in the agent's input always asks first. |
34-
| `integration.tmux_prefix` | `"C-b"` | The prefix of the session chords. Moving between the agent and the reviewer needs no prefix: `Shift+←` goes to the agent and `Shift+→` to the reviewer. The rest are `Prefix+R`, `Prefix+A`, `Prefix+I`, `Prefix+C`, `Prefix+T`, `Prefix+B`, `Prefix+L`, `Prefix+Q` and `Prefix+?`. |
34+
| `integration.tmux_prefix` | `"C-b"` | The prefix of the session chords. Moving between the agent and the reviewer needs no prefix: `Shift+←` goes to the agent and `Shift+→` to the reviewer. The rest are `Prefix+R`, `Prefix+A`, `Prefix+d`, `Prefix+Q` and `Prefix+?`. |
3535

3636
## `[keys]`
3737

@@ -40,34 +40,28 @@ Moves a reader action to another single character. An action that is moved gives
4040
```toml
4141
[keys]
4242
toggle_reviewed = "e"
43-
comment = "C"
43+
explain = "E"
4444
```
4545

4646
The ids are:
4747

48-
`queue`, `structure`, `files`, `code`, `impact`, `comments`, `context`, `tests`, `followup`, `coverage`, `flow`, `next_unreviewed`, `previous_unreviewed`, `next_critical`, `toggle_reviewed`, `toggle_bookmark`, `next_bookmark`, `comment`, `explain`, `toggle_version`, `peek_previous`, `next_change`, `previous_change`, `toggle_code_scope`, `search`, `filter_high`, `filter_unreviewed`, `filter_introduced`, `filter_no_tests`, `filter_new`, `cycle_scope`, `cycle_module`, `clear_filters`, `history_back`, `history_forward`, `deeper_tree`, `shallower_tree`, `run_tests`, `reload_reports`, `reload_flow`, `refresh`, `finish`, `help`.
48+
`queue`, `structure`, `files`, `code`, `next_unreviewed`, `previous_unreviewed`, `next_critical`, `toggle_reviewed`, `toggle_bookmark`, `next_bookmark`, `explain`, `toggle_version`, `peek_previous`, `next_change`, `previous_change`, `toggle_code_scope`, `search`, `filter_high`, `filter_unreviewed`, `filter_introduced`, `filter_no_tests`, `filter_new`, `cycle_scope`, `cycle_module`, `clear_filters`, `history_back`, `history_forward`, `deeper_tree`, `shallower_tree`, `refresh`, `help`.
4949

5050
The keys that only mean something inside some modes move the same way. A moved key answers to its new character in the modes it works in, and its old character stops acting there:
5151

5252
| Id | Default | Works in |
5353
| --- | --- | --- |
54-
| `down`, `up` | `j`, `k` | every list, scrolling view and the delivery list |
54+
| `down`, `up` | `j`, `k` | every list and the code |
5555
| `scroll_left`, `scroll_right` | `h`, `l` | Code |
56-
| `select_lines` | `V` | Code |
5756
| `open_file` | `o` | Code |
5857
| `inspect_contract` | `i` | Code |
5958
| `toggle_inspector` | `K` | Code |
60-
| `pivot_impact` | `p` | Impact |
61-
| `request_flow` | `p` | Flow |
62-
| `edit_intent`, `add_criterion` | `i`, `a` | Context |
63-
| `edit_comment`, `delete_comment` | `e`, `d` | Comments |
64-
| `decide_resolved`, `decide_partial`, `decide_unresolved`, `decide_regression` | `y`, `p`, `u`, `X` | Follow-up |
6559

6660
```toml
6761
[keys]
6862
down = "z"
6963
up = "Z"
70-
decide_resolved = "Y"
64+
open_file = "O"
7165
```
7266

7367
A mode-specific key may take a letter that another mode uses, but not one that is live in the same mode, and not one a global action already has. The footer and the help show the keys in use.

‎src/adapters/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,5 @@ pub mod tmux;
1818
mod process;
1919
mod system;
2020

21-
pub(crate) use process::{ClipboardCopy, ProcessTestRunner};
21+
pub(crate) use process::ProcessTestRunner;
2222
pub(crate) use system::{RandomIds, SystemClock};

‎src/adapters/process.rs‎

Lines changed: 5 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
1-
//! Process-backed adapters: the test runner and the clipboard.
1+
//! Process-backed adapter: the test runner.
22
//!
3-
//! Both spawn a program directly, never through a shell, so a test id or a copied comment cannot
4-
//! be interpreted as shell syntax. Output is capped and sanitised before it comes back, because it
3+
//! It spawns the program directly, never through a shell, so a test id cannot be interpreted as
4+
//! shell syntax. Output is capped and sanitised before it comes back, because it
55
//! is later drawn on screen.
66
7-
use std::io::{Read, Write};
7+
use std::io::Read;
88
use std::os::unix::process::CommandExt;
99
use std::process::{Child, Command, Stdio};
1010
use std::sync::{Arc, Mutex};
1111
use std::thread::{self, JoinHandle};
1212
use std::time::{Duration, Instant};
1313

1414
use crate::domain::error::{Error, Result};
15-
use crate::domain::ports::{CancelToken, Clipboard, TestRunOutcome, TestRunRequest, TestRunner};
15+
use crate::domain::ports::{CancelToken, TestRunOutcome, TestRunRequest, TestRunner};
1616
use crate::domain::text::sanitize_terminal_text;
1717

1818
/// How much of a test run's output is kept. The tail is what the reader shows.
@@ -192,54 +192,6 @@ fn tail(text: &str) -> String {
192192
text[cut..].to_owned()
193193
}
194194

195-
/// Copies text with `pbcopy` on macOS and `wl-copy` or `xclip` elsewhere. A missing program is an
196-
/// error the reader reports, not a panic.
197-
#[derive(Debug, Default)]
198-
pub(crate) struct ClipboardCopy;
199-
200-
impl Clipboard for ClipboardCopy {
201-
fn copy(&self, text: &str) -> Result<()> {
202-
let candidates: &[(&str, &[&str])] = if cfg!(target_os = "macos") {
203-
&[("pbcopy", &[])]
204-
} else {
205-
&[("wl-copy", &[]), ("xclip", &["-selection", "clipboard"])]
206-
};
207-
let mut last = None;
208-
for (program, args) in candidates {
209-
match pipe_to(program, args, text) {
210-
Ok(()) => return Ok(()),
211-
Err(error) => last = Some(error),
212-
}
213-
}
214-
Err(last
215-
.unwrap_or_else(|| Error::invalid("clipboard", "no clipboard program is available")))
216-
}
217-
}
218-
219-
fn pipe_to(program: &str, args: &[&str], text: &str) -> Result<()> {
220-
let mut child = Command::new(program)
221-
.args(args)
222-
.stdin(Stdio::piped())
223-
.stdout(Stdio::null())
224-
.stderr(Stdio::null())
225-
.spawn()
226-
.map_err(|error| Error::io(program, error))?;
227-
if let Some(stdin) = child.stdin.as_mut() {
228-
stdin
229-
.write_all(text.as_bytes())
230-
.map_err(|error| Error::io(program, error))?;
231-
}
232-
let status = child.wait().map_err(|error| Error::io(program, error))?;
233-
if status.success() {
234-
Ok(())
235-
} else {
236-
Err(Error::invalid(
237-
"clipboard",
238-
format!("{program} exited with {status}"),
239-
))
240-
}
241-
}
242-
243195
#[cfg(test)]
244196
mod tests {
245197
use super::*;

‎src/adapters/tmux/config.rs‎

Lines changed: 4 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ pub struct SessionConfig<'a> {
2626

2727
/// The help shown by `Prefix ?`.
2828
pub const HELP_TEXT: &str = "klaub Shift+Left agent Shift+Right review (no prefix) Prefix+R review \
29-
Prefix+A agent Prefix+I impact Prefix+C comments Prefix+T tests Prefix+B brief Prefix+L flow \
30-
Prefix+d detach Prefix+Q quit (Prefix is Ctrl+B unless configured; R and A also work in lower case)";
29+
Prefix+A agent Prefix+d detach Prefix+Q quit \
30+
(Prefix is Ctrl+B unless configured; R and A also work in lower case)";
3131

3232
/// Checks a tmux key name such as `C-b`, `C-Space` or `M-a`.
3333
pub fn validate_prefix(prefix: &str) -> Result<()> {
@@ -202,35 +202,6 @@ pub fn configuration(spec: &SessionConfig<'_>) -> Result<Vec<Vec<String>>> {
202202
"10000",
203203
HELP_TEXT,
204204
]));
205-
// These open the reviewer on a specific mode. The review window is selected first, then the
206-
// function key is delivered to its pane, where the reader maps it to a mode.
207-
for (key, function_key) in [
208-
("I", "F5"),
209-
("C", "F6"),
210-
("T", "F7"),
211-
("B", "F8"),
212-
("L", "F9"),
213-
] {
214-
commands.push(command(&[
215-
"bind-key",
216-
"-T",
217-
"prefix",
218-
key,
219-
"run-shell",
220-
"-b",
221-
"#{@klaub-focus-review}",
222-
]));
223-
commands.push(command(&[
224-
"bind-key",
225-
"-T",
226-
"prefix",
227-
key,
228-
"send-keys",
229-
"-t",
230-
"review",
231-
function_key,
232-
]));
233-
}
234205
commands.push(command(&[
235206
"set-hook",
236207
"-g",
@@ -306,13 +277,8 @@ mod tests {
306277
.any(|c| c.starts_with("bind-key -T prefix Q confirm-before"))
307278
);
308279
assert!(
309-
flat.iter()
310-
.any(|c| c == "bind-key -T prefix I send-keys -t review F5"),
311-
"Prefix+I opens the impact view"
312-
);
313-
assert!(
314-
flat.iter()
315-
.any(|c| c == "bind-key -T prefix L send-keys -t review F9")
280+
!flat.iter().any(|c| c.contains("send-keys -t review")),
281+
"the reader has no screens left to open by a key"
316282
);
317283
assert!(
318284
!flat.iter().any(|c| c.contains("unbind-key")),

‎src/app/analysis.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,12 @@ pub struct Analysis {
100100
}
101101

102102
impl Analysis {
103+
/// Whether `path` is a test file by the configured patterns.
104+
#[must_use]
105+
pub fn is_test_file(&self, path: &crate::domain::path::RepoPath) -> bool {
106+
is_test_path(&self.config, path)
107+
}
108+
103109
/// The file entry containing `unit`.
104110
pub fn file_of(&self, unit: &UnitId) -> Option<&FileEntry> {
105111
self.units.get(unit).map(|u| &self.files[u.file_index])

‎src/cli/commands/review.rs‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,15 @@ use crate::adapters::parse::TreeSitterParser;
1515
use crate::adapters::storage::{
1616
FsConfigSource, FsFileReader, FsFlowRepository, JsonReviewRepository, SqliteCache,
1717
};
18-
use crate::adapters::tmux::channel::TmuxAgentChannel;
1918
use crate::app::{OpenOptions, Ports, ReviewSession};
2019
use crate::domain::i18n::{Locale, cli as t};
2120
use crate::domain::ids::SessionId;
22-
use crate::domain::ports::{AgentChannel, Clipboard, ConfigSource};
21+
use crate::domain::ports::ConfigSource;
2322
use crate::domain::snapshot::{Scope, SnapshotRequest};
2423
use crate::tui::App;
2524
use anyhow::{Context as _, Result};
2625

27-
use crate::adapters::{ClipboardCopy, ProcessTestRunner, RandomIds, SystemClock};
26+
use crate::adapters::{ProcessTestRunner, RandomIds, SystemClock};
2827
use crate::cli::args::{Format, ReviewArgs};
2928
use crate::cli::context::Context;
3029
use crate::cli::{env, logging, output};
@@ -93,8 +92,7 @@ pub(super) fn run(ctx: &Context, args: &ReviewArgs) -> Result<ExitCode> {
9392
let session = ReviewSession::open(ports, request, &config, &options)
9493
.with_context(|| t::cannot_open_review(locale, &target.worktree.display().to_string()))?;
9594

96-
let clipboard: Option<Arc<dyn Clipboard>> = Some(Arc::new(ClipboardCopy));
97-
let mut app = App::new(session, parser, target.agent, clipboard);
95+
let mut app = App::new(session, parser);
9896
if first_review_here {
9997
app = app.with_storage_notice();
10098
}
@@ -137,10 +135,9 @@ fn report_wait(session: &ReviewSession, format: Format, locale: Locale) -> Resul
137135
Ok(code)
138136
}
139137

140-
/// Where the review happens and, inside a wrapped session, the pane the feedback can be prepared in.
138+
/// Where the review happens and, inside a wrapped session, the session it belongs to.
141139
struct ReviewTarget {
142140
session_id: Option<SessionId>,
143-
agent: Option<Arc<dyn AgentChannel>>,
144141
worktree: PathBuf,
145142
}
146143

@@ -149,7 +146,6 @@ fn resolve_target(ctx: &Context, args: &ReviewArgs) -> Result<ReviewTarget> {
149146
let cwd = std::env::current_dir()?;
150147
return Ok(ReviewTarget {
151148
session_id: None,
152-
agent: None,
153149
worktree: env::find_worktree_root(&cwd).unwrap_or(cwd),
154150
});
155151
};
@@ -158,10 +154,8 @@ fn resolve_target(ctx: &Context, args: &ReviewArgs) -> Result<ReviewTarget> {
158154
.registry()
159155
.load(&id)
160156
.with_context(|| t::cannot_load_session(ctx.locale, id.as_str()))?;
161-
let channel: Arc<dyn AgentChannel> = Arc::new(TmuxAgentChannel::new(record.clone()));
162157
Ok(ReviewTarget {
163158
session_id: Some(id),
164-
agent: Some(channel),
165159
worktree: record.worktree,
166160
})
167161
}

0 commit comments

Comments
 (0)