Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds git worktree–aware session picking (new picker mode, toggle, and worktree-derived rows), renames the CLI crate from Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant CLI as "wisp CLI\n(main.rs)"
participant Git as "git helpers\n(git.rs)"
participant Tmux as "wisp-tmux"
participant Core as "wisp-core\n(view)"
participant UI as "wisp-ui"
User->>CLI: run `wisp popup --worktree` or press Ctrl+w
CLI->>Git: git_repo_root(path)
Git-->>CLI: repo root or None
alt repo root found
CLI->>Git: git_worktree_list(repo_root)
Git-->>CLI: WorktreeInfo[]
CLI->>Tmux: request current sessions
Tmux-->>CLI: session list
CLI->>Core: derive_session_list_with_worktrees(sessions, worktrees)
Core-->>CLI: enriched SessionListItems
else no repo
CLI->>Core: derive_session_list()
Core-->>CLI: session list
end
CLI->>UI: render picker with PickerMode
UI-->>User: display items (worktree items marked "W")
User->>UI: select item or toggle mode
UI->>CLI: selection / ToggleWorktreeMode
CLI->>Tmux: create/switch session if needed
CLI->>Core: re-derive session list
Core-->>CLI: updated list
UI-->>User: updated display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Line 37: Replace the unsupported runner label "os: macos-13" in the release
workflow matrix with a valid GitHub-hosted macOS Intel runner such as
"macos-15-intel" (or "macos-26-intel") so the macOS Intel job can run; locate
the matrix entry that currently contains the string os: macos-13 and update that
value accordingly in .github/workflows/release.yml.
In `@crates/wisp-bin/src/git.rs`:
- Around line 61-65: git_repo_root currently returns the .git directory path
because it directly returns resolve_git_dir(path); change it to compute and
return the actual repository root by taking the result of resolve_git_dir(path),
handling both a .git directory and a .git file (worktree/submodule): if
resolve_git_dir returns a directory, return its parent directory as the repo
root; if it returns a file, read the file's first line like "gitdir: <path>",
resolve that path relative to the file's parent if necessary, then return the
parent of the resolved gitdir; ensure you use the existing resolve_git_dir and
git_repo_root symbols to locate and update the logic.
In `@crates/wisp-bin/src/main.rs`:
- Around line 333-336: The UI branch hardcodes PickerMode::AllSessions when
calling run_surface, so the popup path drops the --worktree picker mode; change
the third argument to use the picker mode from the parsed CLI mode (e.g.,
replace PickerMode::AllSessions with mode.picker_mode() or equivalent accessor
on mode) when calling run_surface in ParsedCli::Ui (and apply the same change to
the other occurrence around the 626-635 block) so the internal ui command
threads the requested PickerMode through load_runtime_config() -> run_surface().
- Around line 686-697: The Worktree rows (SessionListItemKind::Worktree) are
being treated like sessions during activation, so activate_filter_selection()
calls switch_or_attach_session(&item.session_id) and tries to attach to a
non-existent session; update activate_filter_selection() to detect
SessionListItemKind::Worktree and instead call the codepath that
creates/switches a session from the worktree path (use the same logic you use
when building rows from derive_session_list_with_worktrees/worktree_path), i.e.
if item.kind == SessionListItemKind::Worktree then invoke the function that
creates or switches to a session based on worktree_path (or implement a small
helper to create a session from worktree_path and then switch to it), otherwise
keep the existing switch_or_attach_session(&item.session_id) behavior for normal
session rows. Ensure references to item.worktree_path (or equivalent) are used
and no attempt is made to attach to item.session_id for pure worktree rows.
- Around line 1084-1114: The ToggleWorktreeMode handler is rebuilding
session_items from the stale immutable `state` captured earlier and never
reapplies the active sort; before calling derive_session_list_with_worktrees or
derive_session_list in the UiIntent::ToggleWorktreeMode branch, reload the
current domain state (i.e., obtain the up-to-date state used elsewhere rather
than the earlier-captured `state`) and use that fresh state when calling
derive_session_list_with_worktrees / derive_session_list, and after constructing
session_items call apply_session_sort(&mut session_items, ...) (or the existing
session-sorting helper) to reapply the active sort so renamed/killed sessions
and sort mode are correct.
In `@crates/wisp-core/src/candidate.rs`:
- Around line 97-104: The worktree candidate is being given a Directory id so
deduplicate_candidates() merges worktrees with directories; add a new
CandidateId::Worktree(PathBuf) variant (or otherwise include CandidateKind in
the dedupe key) and use it when constructing worktree candidates (replace id:
CandidateId::Directory(...) with CandidateId::Worktree(...) in the constructor
seen in candidate.rs), and update deduplicate_candidates() to treat
CandidateId::Worktree as distinct from Directory so worktree-specific
metadata/search_terms are preserved.
In `@crates/wisp-core/src/view.rs`:
- Around line 248-293: The worktree exclusion currently checks only exact
equality against session_paths, causing duplicates when a session path is a
subdirectory of a worktree; update the check inside the worktrees loop so it
treats a worktree as matched when any session path equals or starts_with the
worktree path (mirror the earlier matching logic `path == worktree_path ||
path.starts_with(worktree_path)`). In other words, replace the single
`session_paths.contains(worktree.path.as_path())` test with an any()-style check
over `session_paths` that returns true if `p == worktree.path.as_path()` or
`p.starts_with(worktree.path.as_path())`, referencing the existing symbols
`session_paths`, `worktrees`, and `worktree.path`.
In `@crates/wisp-zoxide/tests/integration.rs`:
- Around line 18-28: The function write_fake_zoxide_script currently takes a
&PathBuf which triggers clippy's ptr_arg; change its signature to accept &Path
instead and update call sites accordingly so it works with borrowed paths (use
the existing staging variable logic unchanged). Ensure you import
std::path::Path at the top if not already present and keep the internal uses
(with_extension, fs::write, fs::metadata, permissions.set_mode,
fs::set_permissions, fs::rename) the same, converting any PathBuf-specific
operations to work with &Path where needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1341a3a5-f1bc-4172-a95c-5ed7c161f289
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/release.ymlAGENTS.mdCargo.tomlLICENSEREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-config/Cargo.tomlcrates/wisp-config/src/lib.rscrates/wisp-core/Cargo.tomlcrates/wisp-core/src/candidate.rscrates/wisp-core/src/lib.rscrates/wisp-core/src/preview.rscrates/wisp-core/src/view.rscrates/wisp-fuzzy/Cargo.tomlcrates/wisp-preview/Cargo.tomlcrates/wisp-status/Cargo.tomlcrates/wisp-tmux/Cargo.tomlcrates/wisp-ui/Cargo.tomlcrates/wisp-ui/src/lib.rscrates/wisp-zoxide/Cargo.tomlcrates/wisp-zoxide/tests/integration.rsdocs/architecture.mddocs/config.schema.tomldocs/configuration.md
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/wisp-core/src/view.rs (1)
29-41:⚠️ Potential issue | 🟠 MajorIndex the worktree path in picker search.
picker_search_text()still skipspath_hint. Later in this file, standalone worktree rows use onlyfile_name()as their label, so~/src/fooand~/tmp/foostop being searchable by parent path once the branch matches.Suggested fix
pub fn picker_search_text(&self) -> String { + let path_hint = match self.kind { + SessionListItemKind::Worktree | SessionListItemKind::WorktreeSession => { + self.path_hint.as_deref() + } + SessionListItemKind::Session => None, + }; + [ Some(self.label.as_str()), + path_hint, self.active_window_label.as_deref(), self.command_hint.as_deref(), self.git_branch.as_ref().map(|branch| branch.name.as_str()), self.worktree_branch.as_deref(), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/wisp-core/src/view.rs` around lines 29 - 41, picker_search_text currently omits the worktree path (path_hint) so worktree rows that only show file_name() become unsearchable by their parent path; update the picker_search_text function to include self.path_hint (e.g., add self.path_hint.as_deref() into the array of Option<&str> items alongside label, active_window_label, command_hint, git_branch, and worktree_branch) so the worktree path is indexed for searches.crates/wisp-ui/src/lib.rs (1)
261-330:⚠️ Potential issue | 🟠 MajorRender worktree rows with their path hint.
The non-compact branch always fills the secondary column from
active_window_label, butSessionListItemKind::Worktreerows arrive with that field empty andpath_hintpopulated. The result is a row that only shows the basename plus maybe a branch, which makes same-named worktrees ambiguous.Suggested fix
- let title = pad_text( - &truncate_text( - item.active_window_label.as_deref().unwrap_or_default(), - title_width, - ), - title_width, - ); + let detail = match item.kind { + SessionListItemKind::Worktree => item.path_hint.as_deref().unwrap_or_default(), + _ => item.active_window_label.as_deref().unwrap_or_default(), + }; + let title = pad_text(&truncate_text(detail, title_width), title_width);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/wisp-ui/src/lib.rs` around lines 261 - 330, Rows for worktrees currently use active_window_label to populate the secondary column, but worktree items put their info in path_hint, leaving that column empty; update the title construction so that when building title (the variable created from active_window_label) you fall back to item.path_hint (or use item.active_window_label.as_deref().or(item.path_hint.as_deref())) before calling truncate_text/pad_text; adjust the reference in the non-compact branch where title_width and title are computed and ensure SessionListItemKind::Worktree / item.kind and item.path_hint are used so worktree rows show the path hint.crates/wisp-bin/src/main.rs (1)
1047-1050:⚠️ Potential issue | 🟠 MajorRename/close paths still aren't worktree-aware.
Pure
SessionListItemKind::Worktreerows use synthetic IDs likeworktree:/…, so these branches can callrename_session/kill_sessionon sessions that do not exist. Even for real worktree-backed sessions, the refresh logic here is session-only (derive_session_list/retain), so worktree mode drops the repo-scoped rows after rename or kill. Gate these actions to real sessions and rebuild through the currentpicker_mode.Also applies to: 1072-1091
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/wisp-bin/src/main.rs` around lines 1047 - 1050, The rename/kill flow is acting on synthetic worktree rows (SessionListItemKind::Worktree with ids like "worktree:/...") and then rebuilding the list with a hardcoded Some("default"), which drops repo-scoped rows; fix by gating calls to tmux.rename_session / tmux.kill_session to only operate on real tmux sessions (check the SessionListItemKind variant or a real session id before calling tmux.*), and when reloading use the current picker_mode instead of Some("default") so derive_session_list/retain rebuilds respect worktree vs session modes; apply the same guard-and-rebuild change to the other similar block (lines ~1072-1091) that uses load_domain_state, derive_session_list, apply_session_sort, and retain.
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
37-37:⚠️ Potential issue | 🟠 MajorUse a supported macOS Intel runner label.
Line 37 still uses
macos-13, which GitHub-hosted runners no longer accept, so thex86_64-apple-darwinrelease leg will never start.Suggested fix
- - os: macos-13 + - os: macos-15-intel target: x86_64-apple-darwin build-tool: cargo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml at line 37, The workflow uses the deprecated runner label "os: macos-13" (which prevents the x86_64-apple-darwin release leg from running); update the runner label in the release jobs/matrix where "os: macos-13" appears to a supported macOS Intel label such as "macos-12" or "macos-latest" so the x86_64-apple-darwin job can start — locate the "os: macos-13" entry in the release matrix and replace it accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/wisp-bin/src/git.rs`:
- Around line 105-107: The current logic in the git parsing code (in the block
handling head.strip_prefix("ref: ") inside the function that parses HEAD
contents) uses reference.rsplit('/').next() which trims hierarchical refs like
"refs/heads/feature/demo" down to "demo"; change this to preserve the full ref
name by returning reference.to_owned() (or reference.to_string()) so the
complete "refs/heads/..." string is retained and won't overwrite the full name
parsed from git worktree list.
In `@crates/wisp-bin/src/main.rs`:
- Around line 1118-1147: In UiIntent::ToggleWorktreeMode when you rebuild
session_items and update pending_branch_names/deferred_branch_status, also
restart the git-status workers so branch_status_updates is re-created for the
new list: drop or replace the old branch_status_updates receiver and call
spawn_git_status_workers(pending_branch_names.clone(), /* other args */) to
obtain a fresh branch_status_updates (and worker handles if any), assigning it
back to the same variable so status updates (sync/dirty) are computed for the
rebuilt list.
- Around line 613-623: create_session_from_worktree_path currently uses only the
Path::file_name as session_name which can collide globally; change session name
generation to include a unique identifier derived from worktree_path (e.g., full
canonicalized path, repository root + branch/worktree id, or a hash of
worktree_path) before calling tmux.create_or_switch_session so names are
globally unique. Update create_session_from_worktree_path to canonicalize or
normalize worktree_path, compute a stable short hash or include parent
components, and use that combined string as session_name (preserving a readable
prefix if desired) when calling
tmux.create_or_switch_session(worktree_session_name, worktree_path). Ensure
non-UTF8 paths are handled as before (to_str fallback) and that resulting
session_name meets tmux naming constraints.
---
Outside diff comments:
In `@crates/wisp-bin/src/main.rs`:
- Around line 1047-1050: The rename/kill flow is acting on synthetic worktree
rows (SessionListItemKind::Worktree with ids like "worktree:/...") and then
rebuilding the list with a hardcoded Some("default"), which drops repo-scoped
rows; fix by gating calls to tmux.rename_session / tmux.kill_session to only
operate on real tmux sessions (check the SessionListItemKind variant or a real
session id before calling tmux.*), and when reloading use the current
picker_mode instead of Some("default") so derive_session_list/retain rebuilds
respect worktree vs session modes; apply the same guard-and-rebuild change to
the other similar block (lines ~1072-1091) that uses load_domain_state,
derive_session_list, apply_session_sort, and retain.
In `@crates/wisp-core/src/view.rs`:
- Around line 29-41: picker_search_text currently omits the worktree path
(path_hint) so worktree rows that only show file_name() become unsearchable by
their parent path; update the picker_search_text function to include
self.path_hint (e.g., add self.path_hint.as_deref() into the array of
Option<&str> items alongside label, active_window_label, command_hint,
git_branch, and worktree_branch) so the worktree path is indexed for searches.
In `@crates/wisp-ui/src/lib.rs`:
- Around line 261-330: Rows for worktrees currently use active_window_label to
populate the secondary column, but worktree items put their info in path_hint,
leaving that column empty; update the title construction so that when building
title (the variable created from active_window_label) you fall back to
item.path_hint (or use
item.active_window_label.as_deref().or(item.path_hint.as_deref())) before
calling truncate_text/pad_text; adjust the reference in the non-compact branch
where title_width and title are computed and ensure
SessionListItemKind::Worktree / item.kind and item.path_hint are used so
worktree rows show the path hint.
---
Duplicate comments:
In @.github/workflows/release.yml:
- Line 37: The workflow uses the deprecated runner label "os: macos-13" (which
prevents the x86_64-apple-darwin release leg from running); update the runner
label in the release jobs/matrix where "os: macos-13" appears to a supported
macOS Intel label such as "macos-12" or "macos-latest" so the
x86_64-apple-darwin job can start — locate the "os: macos-13" entry in the
release matrix and replace it accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 562543fa-09ee-4aa0-b427-95a936f57314
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/release.ymlAGENTS.mdCargo.tomlLICENSEREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-config/Cargo.tomlcrates/wisp-config/src/lib.rscrates/wisp-core/Cargo.tomlcrates/wisp-core/src/candidate.rscrates/wisp-core/src/lib.rscrates/wisp-core/src/preview.rscrates/wisp-core/src/view.rscrates/wisp-fuzzy/Cargo.tomlcrates/wisp-preview/Cargo.tomlcrates/wisp-status/Cargo.tomlcrates/wisp-tmux/Cargo.tomlcrates/wisp-ui/Cargo.tomlcrates/wisp-ui/src/lib.rscrates/wisp-zoxide/Cargo.tomlcrates/wisp-zoxide/tests/integration.rsdocs/architecture.mddocs/config.schema.tomldocs/configuration.md
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
crates/wisp-bin/src/git.rs (1)
105-106:⚠️ Potential issue | 🟡 MinorNormalize
HEADrefs the same way asgit_worktree_list().This returns
refs/heads/feature/demo, whilegit_worktree_list()storesfeature/demo. The mismatch makes session rows and worktree rows render/search the same branch differently, and the extra prefix needlessly burns branch-column width.Suggested fix
if let Some(reference) = head.strip_prefix("ref: ") { - return Some(reference.to_string()); + return Some( + reference + .strip_prefix("refs/heads/") + .unwrap_or(reference) + .to_owned(), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/wisp-bin/src/git.rs` around lines 105 - 106, The HEAD reference returned from head.strip_prefix("ref: ") should be normalized to match git_worktree_list() by removing the "refs/heads/" prefix; update the code path that currently returns Some(reference.to_string()) to first check for and strip the "refs/heads/" prefix (e.g., via starts_with/strip_prefix) and return the shortened branch name (e.g., "feature/demo") so session rows and worktree rows use the same branch format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/wisp-bin/Cargo.toml`:
- Around line 23-25: Remove the stale binstall override entry for
package.metadata.binstall.overrides.aarch64-pc-windows-msvc from Cargo.toml
because the release workflow doesn't produce an aarch64 Windows asset; delete
the entire override block that sets pkg-fmt = "zip" and bin-dir = "{ bin }.exe"
so only supported targets (e.g., x86_64-pc-windows-msvc) remain in
package.metadata.binstall.overrides and avoid advertising a non-existent ARM
Windows binary.
In `@crates/wisp-bin/src/git.rs`:
- Around line 175-193: worktree_repo_root currently calls
state.current_session_id(None) which picks the first client instead of the
invoking client; change worktree_repo_root to accept the caller's client id (or
Option<ClientId>) and call state.current_session_id(Some(client_id)) (or pass
through the Option) so the session lookup is scoped to the invoking client, and
update all callers of worktree_repo_root accordingly; ensure this matches how
you already pass an explicit client id to derive_session_list* so both helpers
use the same explicit client identifier.
In `@crates/wisp-bin/src/main.rs`:
- Around line 632-640: The current early startup path in
session_items_for_picker_mode calls enrich_worktree_items_with_git_status
synchronously, causing a git status shell-out per worktree and blocking UI;
instead, return the derived items immediately (from
derive_session_list_with_worktrees) and enqueue their paths into the existing
background worker pipeline to compute sync/dirty state async (the same approach
used elsewhere—apply the same change for the identical code at the other
location around lines 1639-1655). Concretely: remove the synchronous call to
enrich_worktree_items_with_git_status in session_items_for_picker_mode, ensure
the returned Vec<SessionListItem> includes whatever placeholder state your UI
expects, and push the worktree paths (from worktrees or items) to the background
worker (the worker API used elsewhere) to update items when git status results
arrive. Ensure you reference the functions session_items_for_picker_mode,
derive_session_list_with_worktrees, and enrich_worktree_items_with_git_status
when making the change so the code paths are clear.
- Around line 618-627: Replace the unstable
std::collections::hash_map::DefaultHasher use for session_name generation with a
deterministic hasher so the hash suffix for normalized_path remains stable
across Rust versions; locate the block that creates normalized_path,
instantiates DefaultHasher, calls normalized_path.hash(&mut hasher), and formats
session_name (symbols: normalized_path, hasher, DefaultHasher, session_name,
sanitize_session_name) and switch to a stable hashing implementation (for
example use a fixed algorithm from a crate such as fxhash::FxHasher or
twox_hash::XxHash64 or a cryptographic digest like sha1/sha256) so the
hasher.finish() yields consistent output between releases, preserving the
existing u32 truncation in the session_name format.
In `@crates/wisp-core/src/candidate.rs`:
- Around line 87-109: The public API exposes Candidate::worktree and the
CandidateMetadata::Worktree variant while the WorktreeMetadata type remains
private inside the candidate module, so either re-export WorktreeMetadata (e.g.
pub use candidate::WorktreeMetadata;) from the crate root so downstream crates
can construct the metadata, or restrict the worktree API by changing
Candidate::worktree (and/or the CandidateMetadata::Worktree variant) to
crate-private (pub(crate)) so the public surface no longer requires an
inaccessible type; update visibility consistently for WorktreeMetadata and the
Candidate::worktree constructor to resolve the mismatch.
In `@crates/wisp-core/src/view.rs`:
- Around line 192-199: The current find_worktree_for_path function picks the
first prefix match from worktree_map; instead, iterate all entries in
worktree_map and select the matching WorktreeInfo whose key (wt_path) is the
longest matching prefix of path (i.e., where path == wt_path or
path.starts_with(wt_path)), then return that deepest match; update
find_worktree_for_path to compare prefix lengths (e.g., byte-length or component
count of wt_path) and choose the max-length matching entry from worktree_map
before mapping to the WorktreeInfo.
---
Duplicate comments:
In `@crates/wisp-bin/src/git.rs`:
- Around line 105-106: The HEAD reference returned from head.strip_prefix("ref:
") should be normalized to match git_worktree_list() by removing the
"refs/heads/" prefix; update the code path that currently returns
Some(reference.to_string()) to first check for and strip the "refs/heads/"
prefix (e.g., via starts_with/strip_prefix) and return the shortened branch name
(e.g., "feature/demo") so session rows and worktree rows use the same branch
format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73409274-6505-47b2-9010-f8cc1bb13bb5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/release.ymlAGENTS.mdCargo.tomlLICENSEREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-config/Cargo.tomlcrates/wisp-config/src/lib.rscrates/wisp-core/Cargo.tomlcrates/wisp-core/src/candidate.rscrates/wisp-core/src/lib.rscrates/wisp-core/src/preview.rscrates/wisp-core/src/view.rscrates/wisp-fuzzy/Cargo.tomlcrates/wisp-preview/Cargo.tomlcrates/wisp-status/Cargo.tomlcrates/wisp-tmux/Cargo.tomlcrates/wisp-ui/Cargo.tomlcrates/wisp-ui/src/lib.rscrates/wisp-zoxide/Cargo.tomlcrates/wisp-zoxide/tests/integration.rsdocs/architecture.mddocs/config.schema.tomldocs/configuration.md
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 37-42: Add an additional macOS/Intel matrix entry so Intel Macs
get a build: in the GitHub Actions matrix where the current entries list os,
target and build-tool (e.g., the existing target value "aarch64-apple-darwin"),
add a new matrix object with os set to an Intel macOS runner (e.g.,
"macos-15-intel"), target set to "x86_64-apple-darwin", and build-tool set to
"cargo" so the workflow produces an x86_64 macOS binary alongside the existing
aarch64 build.
In `@crates/wisp-bin/src/git.rs`:
- Around line 67-93: The extraneous call to resolve_git_dir(path)? inside
repo_root_at is unused and should be removed; update the repo_root_at function
to delete the dangling resolve_git_dir(path)? invocation so the function returns
based on the parsed .git pointer and the resolved_git_dir.exists() check (or, if
intended as validation, replace it with resolve_git_dir(&resolved_git_dir)?
instead of calling with path) to eliminate the dead code while preserving the
existing resolved_git_dir logic.
In `@crates/wisp-bin/src/main.rs`:
- Around line 630-645: session_items_for_picker_mode currently returns an empty
Vec when PickerMode::Worktree is selected but git::worktree_repo_root(...)
yields None, which hides the reason from the UI; update
session_items_for_picker_mode to propagate that state by either returning a
Result<Vec<SessionListItem>, PickerError> (create a simple PickerError variant
like NotInGitRepo) or by returning a Vec containing a sentinel SessionListItem
(e.g., SessionListItem::Info or an Item with a special flag/message) that the UI
can render as "not in a git repository" when git::worktree_repo_root returns
None; make changes around the PickerMode::Worktree branch and the callers of
session_items_for_picker_mode so the UI can display the explanatory message
instead of an empty picker.
In `@crates/wisp-ui/src/lib.rs`:
- Around line 362-385: The early return currently skips horizontal centering
when inner.height == 1; change the logic in the single-line centering block (the
condition around lines, inner.height, line_width) to perform horizontal
centering whenever line_width < usize::from(inner.width), and compute
vertical_pad as 0 when inner.height == 1 (or use usize::from(inner.height)/2
guarded). Build centered_spans and centered_text the same way, push vertical_pad
empty lines (zero if height==1), render via
Paragraph::new(Text::from(centered_text)).render(inner, buffer) and remove the
early return so single-line text is centered horizontally even when inner.height
== 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e88bebd-85f9-4221-815d-8a9d25f66494
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/release.ymlAGENTS.mdCargo.tomlLICENSEREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-config/Cargo.tomlcrates/wisp-config/src/lib.rscrates/wisp-core/Cargo.tomlcrates/wisp-core/src/candidate.rscrates/wisp-core/src/lib.rscrates/wisp-core/src/preview.rscrates/wisp-core/src/view.rscrates/wisp-fuzzy/Cargo.tomlcrates/wisp-preview/Cargo.tomlcrates/wisp-status/Cargo.tomlcrates/wisp-tmux/Cargo.tomlcrates/wisp-ui/Cargo.tomlcrates/wisp-ui/src/lib.rscrates/wisp-zoxide/Cargo.tomlcrates/wisp-zoxide/tests/integration.rsdocs/architecture.mddocs/config.schema.tomldocs/configuration.md
Comment on lines
+37
to
+42
| - os: macos-26 | ||
| target: aarch64-apple-darwin | ||
| build-tool: cargo | ||
| - os: windows-latest | ||
| target: x86_64-pc-windows-msvc | ||
| build-tool: cargo |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding an x86_64-apple-darwin target for Intel Mac users.
The matrix only includes aarch64-apple-darwin (ARM), leaving Intel Mac users without a prebuilt binary. If Intel Mac support is desired, add a matrix entry:
- os: macos-15-intel
target: x86_64-apple-darwin
build-tool: cargoIf this omission is intentional (e.g., Intel Macs are being phased out), no action needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 37 - 42, Add an additional
macOS/Intel matrix entry so Intel Macs get a build: in the GitHub Actions matrix
where the current entries list os, target and build-tool (e.g., the existing
target value "aarch64-apple-darwin"), add a new matrix object with os set to an
Intel macOS runner (e.g., "macos-15-intel"), target set to
"x86_64-apple-darwin", and build-tool set to "cargo" so the workflow produces an
x86_64 macOS binary alongside the existing aarch64 build.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/wisp-ui/src/lib.rs (2)
357-391:⚠️ Potential issue | 🟠 MajorDon't center the live query field by default.
render_boxed_paragraph()is also used for the picker/sidebar input at Lines 164-169 and 207-212. With unconditional single-line centering here, the filter text recenters on every keystroke instead of behaving like an input, which makes the primary interaction jump around. Keep centering opt-in for static content and leave editable fields left-aligned.Suggested direction
-fn render_boxed_paragraph(area: Rect, buffer: &mut Buffer, title: &str, text: Text<'_>) { +fn render_boxed_paragraph( + area: Rect, + buffer: &mut Buffer, + title: &str, + text: Text<'_>, + center_single_line: bool, +) { @@ - if lines == 1 { + if center_single_line && lines == 1 { @@ - let vertical_pad = if inner.height > 1 { - usize::from(inner.height) / 2 - } else { - 0 - }; + let vertical_pad = usize::from(inner.height.saturating_sub(1)) / 2;- render_boxed_paragraph( - chunks[0], - buffer, - model.title.as_str(), - Text::from(model.query.as_str()), - ); + render_boxed_paragraph( + chunks[0], + buffer, + model.title.as_str(), + Text::from(model.query.as_str()), + false, + ); @@ - render_boxed_paragraph( - body_chunks[1], - buffer, - "Preview", - ansi_preview_text(preview), - ); + render_boxed_paragraph( + body_chunks[1], + buffer, + "Preview", + ansi_preview_text(preview), + true, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/wisp-ui/src/lib.rs` around lines 357 - 391, render_boxed_paragraph currently unconditionally centers single-line text, which causes the editable picker/sidebar input to recenter on each keystroke; modify render_boxed_paragraph to accept a boolean flag (e.g., center_single_line: bool) and only perform the single-line horizontal/vertical centering when that flag is true, otherwise render the Paragraph left-aligned as now; update the call sites that render the live query / picker/sidebar input to pass false and any static uses that should remain centered to pass true, leaving all other rendering logic intact (use the function name render_boxed_paragraph to locate and change the signature and callers).
858-944: 🧹 Nitpick | 🔵 TrivialAdd a direct
Ctrl-Wkeyboard assertion.The new translation branch at Lines 139-141 isn't exercised by the keyboard tests shown here yet. One assertion here would stop the default worktree toggle from regressing silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/wisp-ui/src/lib.rs` around lines 858 - 944, Add a test assertion in the translates_supported_keys test to cover the new Ctrl-W branch: call translate_key with KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL) and assert it returns Some(UiIntent::ToggleWorktree). This ensures the translate_key function's new branch (Lines ~139-141) mapping Ctrl-W to UiIntent::ToggleWorktree is exercised and won't regress; update the translates_supported_keys test near the other Control-char assertions (e.g., alongside Ctrl-J/Ctrl-R) to include this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 20: Add an explicit workspace-level publish guard so member crates don't
inherit a publishable default: update the Cargo.toml workspace metadata (near
the existing repository = "https://github.com/Pajn/wisp" entry) to include
publish = false at the workspace level. Ensure the new publish = false is placed
in the workspace metadata block that applies to members so that crates without
their own publish setting are prevented from being published accidentally.
In `@crates/wisp-bin/Cargo.toml`:
- Around line 14-21: The binstall configuration uses pkg-fmt = "tgz" under
package.metadata.binstall which makes generated URLs end in .tgz while the
release action produces .tar.gz; update package.metadata.binstall.pkg-fmt from
"tgz" to "tar.gz" (and ensure any per-target overrides like
package.metadata.binstall.overrides.x86_64-pc-windows-msvc remain correct) so
URLs match the taiki-e/upload-rust-binary-action output and avoid 404s when
downloading the wisp binaries.
In `@crates/wisp-bin/src/git.rs`:
- Around line 39-46: The parsing currently checks for exact "locked" and "bare"
tokens which misses cases like "locked <reason>" and "prunable <reason>" and
causes state to leak; change the checks in the git worktree porcelain parsing to
use strip_prefix("locked") or strip_prefix("locked ") (e.g., if let Some(_) =
line.strip_prefix("locked")) to set current_locked = true, and use
strip_prefix("prunable") or strip_prefix("prunable ") to detect prunable entries
and, when prunable is observed, reset all per-record state variables
(current_path, current_branch, current_locked, etc.) to None/false so the
prunable worktree is skipped and does not affect subsequent records.
In `@crates/wisp-core/src/preview.rs`:
- Around line 138-141: Add a regression unit test that constructs a worktree
candidate via Candidate::worktree(...) with a known full_path and preview_key,
converts it to a preview request (so the match arm that yields
CandidateMetadata::Worktree runs), and asserts the resulting
PreviewRequest::Directory has path == full_path (and optionally key ==
preview_key); this ensures the CandidateMetadata::Worktree →
PreviewRequest::Directory mapping in the code handling (the match arm producing
PreviewRequest::Directory) cannot regress.
In `@crates/wisp-core/src/view.rs`:
- Around line 291-297: The worktree row currently leaves git_branch as None for
detached worktrees so the status pipeline never updates them; change the
git_branch assignment to always be Some(GitBranchStatus { name:
worktree.branch.clone().unwrap_or_default(), sync: GitBranchSync::Unknown,
dirty: false }) so detached worktrees have an empty/placeholder branch name but
a GitBranchStatus object that the pipeline (in crates/wisp-bin) can mutate;
update the code that constructs the row (the worktree -> git_branch assignment
near where worktree_path and worktree_branch are set) to use GitBranchStatus and
GitBranchSync::Unknown with worktree.branch.clone().unwrap_or_default().
---
Outside diff comments:
In `@crates/wisp-ui/src/lib.rs`:
- Around line 357-391: render_boxed_paragraph currently unconditionally centers
single-line text, which causes the editable picker/sidebar input to recenter on
each keystroke; modify render_boxed_paragraph to accept a boolean flag (e.g.,
center_single_line: bool) and only perform the single-line horizontal/vertical
centering when that flag is true, otherwise render the Paragraph left-aligned as
now; update the call sites that render the live query / picker/sidebar input to
pass false and any static uses that should remain centered to pass true, leaving
all other rendering logic intact (use the function name render_boxed_paragraph
to locate and change the signature and callers).
- Around line 858-944: Add a test assertion in the translates_supported_keys
test to cover the new Ctrl-W branch: call translate_key with
KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL) and assert it returns
Some(UiIntent::ToggleWorktree). This ensures the translate_key function's new
branch (Lines ~139-141) mapping Ctrl-W to UiIntent::ToggleWorktree is exercised
and won't regress; update the translates_supported_keys test near the other
Control-char assertions (e.g., alongside Ctrl-J/Ctrl-R) to include this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c86eb5c6-ec36-4679-9d2f-14ec40ec1fe1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/release.ymlAGENTS.mdCargo.tomlLICENSEREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-config/Cargo.tomlcrates/wisp-config/src/lib.rscrates/wisp-core/Cargo.tomlcrates/wisp-core/src/candidate.rscrates/wisp-core/src/lib.rscrates/wisp-core/src/preview.rscrates/wisp-core/src/view.rscrates/wisp-fuzzy/Cargo.tomlcrates/wisp-preview/Cargo.tomlcrates/wisp-status/Cargo.tomlcrates/wisp-tmux/Cargo.tomlcrates/wisp-ui/Cargo.tomlcrates/wisp-ui/src/lib.rscrates/wisp-zoxide/Cargo.tomlcrates/wisp-zoxide/tests/integration.rsdocs/architecture.mddocs/config.schema.tomldocs/configuration.md
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 3-7: The workflow allows manual workflow_dispatch runs from any
ref but the create-release and upload-binaries jobs must only run for tag refs;
update both jobs (create-release and upload-binaries) to include an if guard
that checks the ref is a v* tag (e.g. if: startsWith(github.ref, 'refs/tags/v'))
so manual runs from branches like main are skipped and releases only execute
when github.ref is a tag matching the v* pattern.
In `@crates/wisp-bin/src/git.rs`:
- Around line 63-67: The #[allow(dead_code)] on the git_repo_root function is
unnecessary because it is used by worktree_repo_root; remove the attribute above
the function declaration (the #[allow(dead_code)] line) so git_repo_root is
declared without the dead_code suppression, leaving the function signature pub
fn git_repo_root(path: &Path) -> Option<PathBuf> { ... } unchanged and ensuring
no other references to the attribute remain.
- Around line 5-61: The #[allow(dead_code)] attribute on the git_worktree_list
function is unnecessary since the function is used; remove the
#[allow(dead_code)] line immediately above the pub fn git_worktree_list(...)
declaration so the function is compiled without the dead-code suppression, then
run cargo check/build to verify no other warnings or issues remain.
- Around line 159-174: The parsing currently only reads the ahead count in
branch_status_for_directory and treats branches with behind>0 as Pushed; update
the GitBranchSync semantics by adding a new variant (e.g., NeedsPull or Behind)
to the GitBranchSync enum and modify the parsing in branch_status_for_directory
to also parse the behind value from the "# branch.ab +<ahead> -<behind>" line
(use the existing strip_prefix and parts logic to extract the '-' value), set
GitBranchSync::NeedsPull when behind>0 and ahead==0 (preserve NotPushed when
ahead>0 regardless of behind), and add unit tests for
branch_status_for_directory to cover ahead-only, behind-only, both, and neither
cases; alternatively, if you prefer to keep the current behavior, add a doc
comment to GitBranchSync and branch_status_for_directory clearly stating that
only "needs push" is detected and add tests asserting that behavior.
In `@crates/wisp-core/src/view.rs`:
- Around line 302-303: The worktree projection currently returns raw iteration
order (items) causing inconsistent picker order; in the function that builds the
projection (see derive_session_list and the code using state.sessions /
worktrees and the local items vector), sort items before returning using the
same comparator used for the default “recent” ordering (reuse the existing
session ordering comparator or function used by
derive_session_list/current/previous/recent ranking) so worktrees adopt the same
current/previous/recent ranking instead of map insertion order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40041cdf-dfb5-4dd1-a68e-a3ae16fe2cfa
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/release.ymlAGENTS.mdCargo.tomlLICENSEREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-config/Cargo.tomlcrates/wisp-config/src/lib.rscrates/wisp-core/Cargo.tomlcrates/wisp-core/src/candidate.rscrates/wisp-core/src/lib.rscrates/wisp-core/src/preview.rscrates/wisp-core/src/view.rscrates/wisp-fuzzy/Cargo.tomlcrates/wisp-preview/Cargo.tomlcrates/wisp-status/Cargo.tomlcrates/wisp-tmux/Cargo.tomlcrates/wisp-ui/Cargo.tomlcrates/wisp-ui/src/lib.rscrates/wisp-zoxide/Cargo.tomlcrates/wisp-zoxide/tests/integration.rsdocs/architecture.mddocs/config.schema.tomldocs/configuration.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack
Summary by CodeRabbit
New Features
Documentation
Chores