feat(tui): auto-approve rebased changes with no interdiff

a code review tool

When a previously approved change is rebased without conflicts, the
interdiff is empty. Instead of showing an empty review view, silently
re-approve at the new commit ID.

melse.tngl.sh 1 month ago 3f18ccc8 a28ea420

+154 -77

2 changed files

expand all

lib

tui

code_review_view.ml

lens_tui.ml

+129 -67

lib/tui/code_review_view.ml

··· 90 90 path)) 91 91 ;; 92 92 93 - let render_footer ~(flavor : Catppuccin.Flavor.t) ~reviewed_count ~total_count ~is_interdiff = 93 + let render_footer 94 + ~(flavor : Catppuccin.Flavor.t) 95 + ~reviewed_count 96 + ~total_count 97 + ~is_interdiff 98 + ~diff_focused 99 + = 94 100 let shortcut = shortcut ~flavor in 95 101 let c color = Catppuccin.color ~flavor color in 96 102 let progress = ··· 107 113 ] 108 114 else View.none 109 115 in 110 - View.hcat 111 - [ shortcut "j/k" "select file" 112 - ; View.text " " 113 - ; shortcut "Shift+R" "mark reviewed" 114 - ; View.text " " 115 - ; shortcut "PgDn/PgUp" "scroll diff" 116 - ; View.text " " 117 - ; shortcut "q/esc" "back" 118 - ; progress 119 - ; interdiff_indicator 120 - ] 116 + let sp = View.text " " in 117 + let shortcuts = 118 + if diff_focused 119 + then 120 + [ shortcut "Tab" "files" 121 + ; sp 122 + ; shortcut "j/k" "scroll" 123 + ; sp 124 + ; shortcut "h/l" "pan" 125 + ; sp 126 + ; shortcut "R" "reviewed" 127 + ; sp 128 + ; shortcut "q" "back" 129 + ] 130 + else 131 + [ shortcut "Tab" "diff" 132 + ; sp 133 + ; shortcut "j/k" "select" 134 + ; sp 135 + ; shortcut "R" "reviewed" 136 + ; sp 137 + ; shortcut "q" "back" 138 + ] 139 + in 140 + View.hcat (shortcuts @ [ progress; interdiff_indicator ]) 121 141 ;; 122 142 123 143 let render_approval_prompt ~(flavor : Catppuccin.Flavor.t) ~(dimensions : Dimensions.t) = ··· 191 211 | [] -> View.text "No diff content" 192 212 | line_views -> View.vcat line_views) 193 213 in 194 - let%sub ~view, ~inject, ~less_keybindings_handler:_, ~is_at_bottom:_, ~stuck_to_bottom:_ 214 + let%sub ~view, ~inject, ~less_keybindings_handler, ~is_at_bottom:_, ~stuck_to_bottom:_ 195 215 = 196 216 Bonsai_tui_scroller.component ~dimensions diff_content_view graph 197 217 in ··· 204 224 (let%arr inject in 205 225 fun _path -> inject (Scroll_to { top = 0; bottom = 0 })) 206 226 graph; 207 - view, inject 227 + view, inject, less_keybindings_handler 208 228 ;; 209 229 210 230 (* --- Main component --- *) ··· 266 286 let selected_file_idx, set_selected_file_idx = Bonsai.state 0 graph in 267 287 let file_list_offset, set_file_list_offset = Bonsai.state 0 graph in 268 288 let showing_approval_prompt, set_showing_approval_prompt = Bonsai.state false graph in 289 + let diff_focused, set_diff_focused = Bonsai.state false graph in 290 + let h_offset, set_h_offset = Bonsai.state 0 graph in 269 291 (* Reset when the change being reviewed changes *) 270 292 Bonsai.Edge.on_change 271 293 change_id 272 294 ~trigger:`After_display 273 295 ~equal:[%equal: Change_id.t] 274 296 ~callback: 275 - (let%arr set_selected_file_idx and set_file_list_offset and set_showing_approval_prompt in 297 + (let%arr set_selected_file_idx 298 + and set_file_list_offset 299 + and set_showing_approval_prompt 300 + and set_diff_focused 301 + and set_h_offset in 276 302 fun _id -> 277 303 let open Effect.Let_syntax in 278 304 let%bind () = set_selected_file_idx 0 in 279 305 let%bind () = set_file_list_offset 0 in 280 - set_showing_approval_prompt false) 306 + let%bind () = set_showing_approval_prompt false in 307 + let%bind () = set_diff_focused false in 308 + set_h_offset 0) 281 309 graph; 282 310 let selected_file_path = 283 311 let%arr selected_file_idx and file_paths in ··· 296 324 ; height = dimensions.height - pad_h - footer_height - pad_h 297 325 } 298 326 in 299 - let diff_scroll_view, diff_inject = 327 + let diff_scroll_view, diff_inject, diff_less_handler = 300 328 diff_pane ~dimensions:diff_dims ~change_id ~prev_tip ~commit_id ~selected_file_path graph 301 329 in 330 + (* Reset horizontal scroll when file changes *) 331 + Bonsai.Edge.on_change 332 + selected_file_path 333 + ~trigger:`After_display 334 + ~equal:[%equal: string option] 335 + ~callback: 336 + (let%arr set_h_offset in 337 + fun _path -> set_h_offset 0) 338 + graph; 302 339 (* Keyboard handler *) 303 340 let visible_height = 304 341 let%arr dimensions in ··· 317 354 and reviewed_files 318 355 and set_reviewed_files 319 356 and diff_inject 357 + and diff_less_handler 320 358 and file_paths 321 - and num_files in 359 + and num_files 360 + and diff_focused 361 + and set_diff_focused 362 + and h_offset 363 + and set_h_offset in 322 364 let select_file idx = 323 365 let open Effect.Let_syntax in 324 366 let%bind () = set_selected_file_idx idx in 325 - (* Adjust scroll offset to keep selection in view *) 326 367 if idx < file_list_offset 327 368 then set_file_list_offset idx 328 369 else if idx >= file_list_offset + visible_height 329 370 then set_file_list_offset (idx - visible_height + 1) 330 371 else Effect.Ignore 331 372 in 373 + let mark_reviewed () = 374 + match List.nth file_paths selected_file_idx with 375 + | None -> Effect.Ignore 376 + | Some path -> 377 + let open Effect.Let_syntax in 378 + let new_reviewed = Set.add reviewed_files path in 379 + let%bind () = set_reviewed_files new_reviewed in 380 + let all_reviewed = 381 + List.for_all file_paths ~f:(fun p -> Set.mem new_reviewed p) 382 + in 383 + if all_reviewed 384 + then set_showing_approval_prompt true 385 + else ( 386 + let next_unreviewed = 387 + let after = 388 + List.findi file_paths ~f:(fun i p -> 389 + i > selected_file_idx && not (Set.mem new_reviewed p)) 390 + in 391 + match after with 392 + | Some (idx, _) -> Some idx 393 + | None -> 394 + List.findi file_paths ~f:(fun _i p -> not (Set.mem new_reviewed p)) 395 + |> Option.map ~f:fst 396 + in 397 + match next_unreviewed with 398 + | Some idx -> select_file idx 399 + | None -> set_showing_approval_prompt true) 400 + in 332 401 fun (event : Event.t) -> 333 402 if showing_approval_prompt 334 403 then ( ··· 341 410 | Key_press { key = Escape; mods = [] } 342 411 | Key_press { key = ASCII 'q'; mods = [] } 343 412 | Key_press { key = Backspace; mods = [] } -> on_exit 344 - | Key_press { key = ASCII 'j'; mods = [] } 345 - | Key_press { key = Arrow `Down; mods = [] } -> 346 - if num_files = 0 347 - then Effect.Ignore 348 - else select_file (Int.min (selected_file_idx + 1) (num_files - 1)) 349 - | Key_press { key = ASCII 'k'; mods = [] } 350 - | Key_press { key = Arrow `Up; mods = [] } -> 351 - select_file (Int.max (selected_file_idx - 1) 0) 352 - | Key_press { key = ASCII 'R'; mods = [] } -> 353 - (match List.nth file_paths selected_file_idx with 354 - | None -> Effect.Ignore 355 - | Some path -> 356 - let open Effect.Let_syntax in 357 - let new_reviewed = Set.add reviewed_files path in 358 - let%bind () = set_reviewed_files new_reviewed in 359 - let all_reviewed = 360 - List.for_all file_paths ~f:(fun p -> Set.mem new_reviewed p) 361 - in 362 - if all_reviewed 363 - then set_showing_approval_prompt true 364 - else ( 365 - let next_unreviewed = 366 - let after = 367 - List.findi file_paths ~f:(fun i p -> 368 - i > selected_file_idx && not (Set.mem new_reviewed p)) 369 - in 370 - match after with 371 - | Some (idx, _) -> Some idx 372 - | None -> 373 - List.findi file_paths ~f:(fun _i p -> not (Set.mem new_reviewed p)) 374 - |> Option.map ~f:fst 375 - in 376 - match next_unreviewed with 377 - | Some idx -> select_file idx 378 - | None -> set_showing_approval_prompt true)) 379 - | Key_press { key = Page `Down; mods = [] } 380 - | Key_press { key = ASCII 'd'; mods = [ Ctrl ] } -> diff_inject Down_half_screen 381 - | Key_press { key = Page `Up; mods = [] } 382 - | Key_press { key = ASCII 'u'; mods = [ Ctrl ] } -> diff_inject Up_half_screen 383 - | Key_press { key = ASCII 'G'; mods = [] } -> diff_inject Bottom 384 - | Key_press { key = ASCII 'g'; mods = [] } -> diff_inject Top 385 - | Key_press { key = ASCII 'j'; mods = [ Ctrl ] } 386 - | Key_press { key = Arrow `Down; mods = [ Shift ] } -> diff_inject Down 387 - | Key_press { key = ASCII 'k'; mods = [ Ctrl ] } 388 - | Key_press { key = Arrow `Up; mods = [ Shift ] } -> diff_inject Up 389 - | _ -> Effect.Ignore) 413 + | Key_press { key = Tab; mods = [] } -> set_diff_focused (not diff_focused) 414 + | Key_press { key = ASCII 'R'; mods = [] } -> mark_reviewed () 415 + | _ -> 416 + if diff_focused 417 + then ( 418 + match event with 419 + | Key_press { key = ASCII 'h'; mods = [] } 420 + | Key_press { key = Arrow `Left; mods = [] } -> 421 + set_h_offset (Int.max 0 (h_offset - 4)) 422 + | Key_press { key = ASCII 'l'; mods = [] } 423 + | Key_press { key = Arrow `Right; mods = [] } -> 424 + set_h_offset (h_offset + 4) 425 + | _ -> diff_less_handler event) 426 + else ( 427 + match event with 428 + | Key_press { key = ASCII 'j'; mods = [] } 429 + | Key_press { key = Arrow `Down; mods = [] } -> 430 + if num_files = 0 431 + then Effect.Ignore 432 + else select_file (Int.min (selected_file_idx + 1) (num_files - 1)) 433 + | Key_press { key = ASCII 'k'; mods = [] } 434 + | Key_press { key = Arrow `Up; mods = [] } -> 435 + select_file (Int.max (selected_file_idx - 1) 0) 436 + | Mouse { kind = Scroll _; _ } -> diff_less_handler event 437 + | Key_press { key = Page `Down; mods = [] } 438 + | Key_press { key = ASCII 'd'; mods = [ Ctrl ] } -> 439 + diff_inject Down_half_screen 440 + | Key_press { key = Page `Up; mods = [] } 441 + | Key_press { key = ASCII 'u'; mods = [ Ctrl ] } -> 442 + diff_inject Up_half_screen 443 + | _ -> Effect.Ignore)) 390 444 in 391 445 (* Compose final view *) 392 446 let view = ··· 398 452 and diff_scroll_view 399 453 and file_paths 400 454 and prev_tip 455 + and h_offset 456 + and diff_focused 401 457 and flavor = Catppuccin.flavor graph in 402 458 let c color = Catppuccin.color ~flavor color in 403 459 let content_width = dimensions.width - (2 * pad_w) in ··· 413 469 ~reviewed_files 414 470 file_paths 415 471 in 472 + let separator_color = if diff_focused then c Blue else c Overlay0 in 416 473 let separator = 417 474 View.vcat 418 475 (List.init content_height ~f:(fun _ -> 419 - View.text ~attrs:[ Attr.fg (c Overlay0) ] "\xe2\x94\x82")) 476 + View.text ~attrs:[ Attr.fg separator_color ] "\xe2\x94\x82")) 477 + in 478 + let diff_view = 479 + if h_offset > 0 then View.crop ~l:h_offset diff_scroll_view 480 + else diff_scroll_view 420 481 in 421 482 let footer = 422 483 render_footer ··· 424 485 ~reviewed_count:(Set.length reviewed_files) 425 486 ~total_count:(List.length file_paths) 426 487 ~is_interdiff:(Option.is_some prev_tip) 488 + ~diff_focused 427 489 in 428 - let main_content = View.hcat [ file_list; separator; diff_scroll_view ] in 490 + let main_content = View.hcat [ file_list; separator; diff_view ] in 429 491 let bg = 430 492 View.rectangle 431 493 ~attrs:[ Attr.bg (c Base) ]

+25 -10

lib/tui/lens_tui.ml

··· 71 71 match%sub route with 72 72 | Log -> 73 73 let enter_review = 74 - let%arr set_route in 74 + let%arr set_route and set_review_generation and review_generation in 75 75 fun (change : Jj.Change.t) -> 76 76 let open Effect.Let_syntax in 77 77 let%bind review_info = ··· 96 96 if already_approved 97 97 then Lwt.return `Already_approved 98 98 else 99 - let+ prev = 99 + let* prev = 100 100 Lens_backend.Store.latest_approval_by 101 101 store 102 102 change.change_id 103 103 user_id 104 104 in 105 - let prev_tip = 106 - match prev with 107 - | Some (a : Lens_backend.Store.Approval.t) 108 - when not (Commit_id.equal a.commit_id change.commit_id) -> 109 - Some a.commit_id 110 - | _ -> None 111 - in 112 - `Should_review prev_tip 105 + match prev with 106 + | Some (a : Lens_backend.Store.Approval.t) 107 + when not (Commit_id.equal a.commit_id change.commit_id) -> 108 + (* Previous approval exists at a different commit — check interdiff *) 109 + let+ interdiff_files = 110 + Jj.fetch_interdiff_file_paths 111 + ~from_tip:a.commit_id 112 + ~to_tip:change.commit_id 113 + in 114 + if List.is_empty interdiff_files 115 + then `Auto_approve 116 + else `Should_review (Some a.commit_id) 117 + | _ -> Lwt.return (`Should_review None) 113 118 else Lwt.return `Not_reviewer) 114 119 in 115 120 match review_info with ··· 120 125 ; commit_id = change.commit_id 121 126 ; prev_tip 122 127 }) 128 + | `Auto_approve -> 129 + let%bind () = 130 + Effect.of_lwt_thunk (fun () -> 131 + Lens_backend.Store.approve 132 + store 133 + change.change_id 134 + user_id 135 + change.commit_id) 136 + in 137 + set_review_generation (review_generation + 1) 123 138 | `Already_approved | `Not_reviewer -> Effect.Ignore 124 139 in 125 140 let ~view, ~handler =