-
Notifications
You must be signed in to change notification settings - Fork 0
review: add stack-mode TUI navigation and copy callbacks #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add --stack/-s flag to review all PRs in a stack with h/l navigation - Refactor TUI to use callbacks for copy operations (no exit on copy) - Show status feedback when copying to clipboard - Display PR title and navigation hints in header - Fix help text to document C for raw copy vs c/y for LLM format ztk-id: 43eabb95-282d-40d5-96db-5ef79f59e7da
ccaf1f7 to
1275e92
Compare
| // Track which PR is "current" (top of stack) | ||
| if (idx == stk.commits.len - 1) { | ||
| current_pr_index = ReviewTUIState.pr_summaries.items.len - 1; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Stack mode starts at wrong PR when top commit skipped
In stack mode, current_pr_index is only updated when idx == stk.commits.len - 1, meaning it only works if the absolute last commit in the stack has a valid open PR. If the top commit is WIP, has no PR, or has a closed PR, current_pr_index remains 0 (the bottom PR) instead of pointing to the topmost valid PR. This causes users to start navigating from the wrong PR in the stack when the top commit is skipped for any reason.
| if (copy_callback) |cb| { | ||
| cb(list.selected, false); | ||
| status_msg = "Copied to clipboard (raw)"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Clipboard success message shown when nothing copied
When the user presses c, y, or C to copy, the status message "Copied to clipboard" is set unconditionally after calling copy_callback. However, if there are no feedback items (empty list), copyCallback returns early without copying anything, but the TUI still displays the success message. This gives misleading feedback to users that content was copied when nothing actually was.
| current_items = new_items; | ||
| list = ListView.init(current_items, visible_height); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: PR index updated before callback success check
The current_pr_index is modified before checking if pr_change_callback returns valid items. If the callback returns null (e.g., due to a bounds check failure at line 1607 in cli.zig), the index remains changed while current_items and list are not updated. This causes a mismatch where renderWithPRNav displays the header for the new PR index but the list still shows items from the previous PR.
ztk-id: 43eabb95-282d-40d5-96db-5ef79f59e7da
Note
Adds
--stack/-sto review all PRs in a stack with PR navigation and refactors the TUI to use copy callbacks with inline status feedback.cmdReview):--stack/-sto fetch and display feedback for all PRs in the stack; navigate PRs withh/lor arrow keys.--pr/-pand default current-branch PR;--listprints summaries non-interactively.runReviewTUISingleand list printerprintReviewList.tui.zig):runInteractive(...)to accept acopy_callback, optionalprslist, current PR index, andpr_change_callback; removeActionreturn enum.PRInfoandrenderWithPRNavto show PR header, navigation hints, and status messages; keep session open on copy (c/y for LLM,Cfor raw).ReviewTUIStateto manage allocator-backed lists, PR context, and shared builders (buildItemsForSummary,prChangeCallback,copyCallback).--stackand copy keybindings.Written by Cursor Bugbot for commit 1275e92. This will update automatically on new commits. Configure here.