Skip to content

Conversation

@tiye
Copy link
Member

@tiye tiye commented Sep 17, 2025

finbr expanded to br with finish and open.

Summary by CodeRabbit

  • New Features

    • Introduced a Branch command group with finish and open subcommands.
    • finish safely handles uncommitted changes, verifies merge into main, updates main, and deletes the feature branch with clear status messages.
    • open launches the repository’s remote page in your browser, navigating to the current branch when supported.
    • Added a JWT decode CLI to pretty-print JWT header and payload.
  • Documentation

    • Added Branch Operations section to README (duplicated in the diff).
  • Refactor

    • Reorganized CLI into a nested Branch command structure.
  • Style

    • Standardized console message formatting.

@tiye tiye requested a review from a team September 17, 2025 17:04
@coderabbitai
Copy link

coderabbitai bot commented Sep 17, 2025

Walkthrough

Adds a new branch ("br") subcommand group with finish and open; implements an expanded finish_branch flow (stash, fetch, merge checks, switch/pull, delete, restore) and open_remote_repository; introduces a JWT CLI module; adds webbrowser and jsonwebtoken deps; updates formatting and README docs (duplicated section).

Changes

Cohort / File(s) Summary of Changes
CLI args restructuring
src/args.rs
Replaces FinishBranch with Branch subcommand group (#[argh(subcommand, name = "br")] InspectForBranch) and adds BranchCommand enum with Finish and Open variants; removes InspectForFinishBranch; adds Jwt import.
CLI dispatch & wiring
src/main.rs
Wires new BranchCommand variants: Finishgit::finish_branch, Opengit::open_remote_repository; adds mod jwt and Jwt handling; converts many prints to brace-style formatting.
Git branch operations
src/git.rs
Substantially expands finish_branch (dirty-state stash/restore, fetch, multiple merge verification strategies, checkout/pull main, delete branch, detailed messaging, many helpers). Adds public pub fn open_remote_repository() -> Result<(), String> to parse origin, normalize URL, and open browser.
JWT utility module
src/jwt.rs
New CLI JWT decoding module: InspectForJwt, JwtCommand, DecodeArgs, handle_jwt_command; decodes JWT header/payload (signature validation disabled) and prints JSON.
Documentation
README.md
Adds "Branch Operations" section documenting br finish and br open; section appears duplicated in diff.
Formatting & minor edits
src/dir_marks.rs, src/tags.rs
Formatting-only changes: switch to named brace placeholders and whitespace/indent adjustments; no logic changes.
Dependencies
Cargo.toml
Adds webbrowser = "0.8" and jsonwebtoken = "9.3.1" under [dependencies].

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant CLI as CLI (argh)
  participant Main as main.rs
  participant Git as git.rs

  rect rgb(240,248,255)
  note over User,Main: br finish
  User->>CLI: inspect br finish
  CLI-->>Main: BranchCommand::Finish
  Main->>Git: finish_branch()
  Git->>Git: check_and_handle_dirty_state()
  Git->>Git: fetch_remote()
  Git->>Git: is_branch_merged()
  alt merged
    Git->>Git: checkout_branch(main)
    Git->>Git: pull_main_branch()
    Git->>Git: delete_branch(feature)
    Git->>Git: restore_stashed_changes()
    Git-->>Main: Ok
  else not merged / error
    Git-->>Main: Err
  end
  Main-->>User: status / error output
  end
Loading
sequenceDiagram
  autonumber
  actor User
  participant CLI as CLI (argh)
  participant Main as main.rs
  participant Git as git.rs
  participant Browser as System Browser

  rect rgb(245,255,250)
  note over User,Browser: br open
  User->>CLI: inspect br open
  CLI-->>Main: BranchCommand::Open
  Main->>Git: open_remote_repository()
  Git->>Git: read current branch
  Git->>Git: parse .git/config (origin URL)
  Git->>Git: normalize to web URL (trim .git, SSH→HTTPS)
  Git->>Git: append /tree/<branch> for GitHub
  Git->>Browser: open URL
  Browser-->>Git: result
  Git-->>Main: Ok/Err
  Main-->>User: status / error output
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • get a finish branch command #4 — Implements finish_branch and CLI wiring for finishing branches; directly related to the br command restructuring and expanded finish flow.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “refine branch subcommand” concisely reflects the primary change of restructuring and enhancing the branch (br) subcommand with new finish and open actions, matching the PR’s focus on expanding the CLI’s branch operations.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch finbr

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (14)
src/tags.rs (2)

61-71: Fix range header to reflect the displayed window, not the full repository span.

Use tags_to_display.first()/last() instead of all_tags.first()/last() so the “from … to …” dates match what’s shown.

Apply this diff:

-  if !options.all && total_tags > tags_to_display.len() {
-    if let (Some(first_tag), Some(last_tag)) = (all_tags.first(), all_tags.last()) {
+  if !options.all && total_tags > tags_to_display.len() {
+    if let (Some(first_tag), Some(last_tag)) = (tags_to_display.first(), tags_to_display.last()) {
       println!(
         "Showing the last {} of {} tags (from {} to {}). Use --all to see all.",
         tags_to_display.len(),
         total_tags,
         first_tag.date,
         last_tag.date
       );
       println!(); // Add a blank line for separation
     }
   }

74-79: Micro-cleanup: compute max width functionally.

Equivalent, simpler, and avoids a mutable loop.

-  let mut max_tag_width = 0;
-  for tag in &tags_to_display {
-    if tag.name.len() > max_tag_width {
-      max_tag_width = tag.name.len();
-    }
-  }
+  let max_tag_width = tags_to_display.iter().map(|t| t.name.len()).max().unwrap_or(0);
src/dir_marks.rs (1)

94-94: Align message formatting for consistency.

removed (Line 91) still uses positional {} while nearby changes adopt named placeholders. Consider updating it for uniformity.

-      println!("removed `{}`\t{}\t{}", target.kwd, target.path, target.description);
+      println!("removed `{}`\t{}\t{}", target.kwd, target.path, target.description);
+      // Or, for consistency with new style (requires temporary bindings or inline vars):
+      let kwd = &target.kwd;
+      let path = &target.path;
+      let description = &target.description;
+      println!("removed `{kwd}`\t{path}\t{description}");
src/args.rs (2)

96-103: Add short help text for br.

A brief doc comment improves in br -h.

-#[derive(FromArgs, PartialEq, Debug)]
+#[derive(FromArgs, PartialEq, Debug)]
 /// command for branch operations
 #[argh(subcommand, name = "br")]
 pub struct InspectForBranch {

Add a doc string:

-/// command for branch operations
+/// Git branch operations (use `in br finish` or `in br open`)

111-120: Improve -h output: add doc strings; consider long alias.

  • Add doc comments to show descriptions in help.
  • If you want both br and branch, argh doesn’t support aliases; define a second wrapper with name = "branch" that delegates to the same enum.
 #[derive(FromArgs, PartialEq, Debug)]
 #[argh(subcommand, name = "finish")]
-pub struct InspectForBranchFinish {}
+/// Delete the current branch after verifying it is already merged into main
+pub struct InspectForBranchFinish {}
 
 #[derive(FromArgs, PartialEq, Debug)]
 #[argh(subcommand, name = "open")]
-pub struct InspectForBranchOpen {}
+/// Open the remote repository (and current branch on GitHub) in your browser
+pub struct InspectForBranchOpen {}
src/git.rs (2)

76-90: Minor: redundant import and mixed languages.

Inside the function, the extra use std::process::Command; is redundant (already at file top). Also consider English-only comments for consistency.

-  use std::fs;
-  use std::process::Command;
+  use std::fs;

9-9: Unify error types across public functions.

finish_branch returns Result<(), Box<dyn Error>> while open_remote_repository returns Result<(), String>. Pick one (anyhow::Result<()> is convenient) for a cleaner API.

Also applies to: 76-76

src/main.rs (7)

25-25: Prefer Display over Debug for IP output

IpAddr implements Display; {ip} prints cleaner than {ip:?}.

-          println!("{name}:\t{ip:?}");
+          println!("{name}:\t{ip}");

31-31: Tidy CLI text: avoid tab padding

Tabs render inconsistently. Use a single space.

-        println!("{my_local_ip}\t\t(copied to clipboard)");
+        println!("{my_local_ip} (copied to clipboard)");

65-69: Use Path.display() for paths; keep UID as-is

For CWD, prefer Display formatting to avoid debug quotes. UID can remain {v:?} unless UserId implements Display in your target platforms.

-          print!("\t{v:?}");
+          print!("\t{}", v.display());

If process.user_id()’s type implements Display, you may also switch {v:?} to {v}.


83-83: Tidy CLI text: avoid tab padding

Match the earlier output style.

-      println!("{dir_str}\t\t(copied to clipboard)");
+      println!("{dir_str} (copied to clipboard)");

121-133: Propagate errors with ? instead of exiting

Since main returns Result<(), String>, prefer ? with map_err to standardize error handling and avoid abrupt exit(1).

-    Branch(options) => match options.subcommand {
-      BranchCommand::Finish(_) => {
-        if let Err(e) = git::finish_branch() {
-          eprintln!("Error finishing branch: {e}");
-          std::process::exit(1);
-        }
-      }
-      BranchCommand::Open(_) => {
-        if let Err(e) = git::open_remote_repository() {
-          eprintln!("Error opening remote repository: {e}");
-          std::process::exit(1);
-        }
-      }
-    }
+    Branch(options) => match options.subcommand {
+      BranchCommand::Finish(_) => {
+        git::finish_branch().map_err(|e| format!("Error finishing branch: {e}"))?;
+      }
+      BranchCommand::Open(_) => {
+        git::open_remote_repository().map_err(|e| format!("Error opening remote repository: {e}"))?;
+      }
+    }

129-132: Cross‑platform browser open for br open

git::open_remote_repository calls the macOS-only open. Consider using a cross‑platform opener (e.g., the opener crate) so br open works on Linux (xdg-open) and Windows (start).

In src/git.rs:

// Cargo.toml: opener = "0.7"
use opener::open;

open(&web_url).map_err(|e| format!("Failed to open browser: {e}"))?;

136-139: Make ShowTags consistent: propagate error with ?

Aligns with the suggested pattern above and existing uses like show_file_size.

-      if let Err(e) = tags::show_tags(&options) {
-        eprintln!("Error showing tags: {e}");
-        std::process::exit(1);
-      }
+      tags::show_tags(&options).map_err(|e| format!("Error showing tags: {e}"))?;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 76eaaba and 3b6216f.

📒 Files selected for processing (6)
  • README.md (2 hunks)
  • src/args.rs (2 hunks)
  • src/dir_marks.rs (4 hunks)
  • src/git.rs (4 hunks)
  • src/main.rs (5 hunks)
  • src/tags.rs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main.rs (2)
src/git.rs (2)
  • finish_branch (9-74)
  • open_remote_repository (76-159)
src/tags.rs (1)
  • show_tags (11-86)
🔇 Additional comments (8)
README.md (2)

139-153: Clarify platform support for br open (or make it cross‑platform).

Code currently uses the macOS open tool. Either document macOS-only behavior here, or adopt a cross‑platform launcher (see my code suggestion in src/git.rs).


97-111: Update example after tag-range header fix (if applied).

Once src/tags.rs uses the displayed range for “from … to …”, ensure the example mirrors that behavior.

src/dir_marks.rs (3)

85-85: LGTM (named formatting).


106-106: LGTM (error message with inline var).


221-221: LGTM (const interpolation).

src/args.rs (2)

20-22: Top-level wiring for Branch looks good.


104-110: LGTM (subcommands).

src/main.rs (1)

11-11: Confirm args wiring — OK

TopLevelInspection present; InspectionCommand includes Branch(InspectForBranch); InspectForBranch is #[argh(subcommand, name = "br")] and delegates to BranchCommand; BranchCommand has Finish(InspectForBranchFinish) and Open(InspectForBranchOpen) with argh names "finish" and "open"; no FinishBranch variant found.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (5)
src/git.rs (5)

45-46: Clarify messages: check against origin/

to avoid confusion.

You already use remote-first in helpers; reflect that in the user-facing text.

-println!("🔍 Checking if branch '{current_branch_name}' has been merged into '{main_branch}'...");
+println!("🔍 Checking if branch '{current_branch_name}' has been merged into 'origin/{main_branch}'...");

-println!("✅ Branch '{current_branch_name}' has been merged into '{main_branch}'");
+println!("✅ Branch '{current_branch_name}' has been merged into 'origin/{main_branch}'");

Also applies to: 64-65


174-183: Make browser opening cross‑platform.

“open” is macOS-only. Use a cross-platform approach or a small dependency.

Option A (no new dep):

-  // 使用系统默认浏览器打开 URL
-  let result = Command::new("open")
-    .arg(&web_url)
-    .output()
-    .map_err(|e| format!("Failed to open browser: {e}"))?;
-
-  if !result.status.success() {
-    let stderr = String::from_utf8_lossy(&result.stderr);
-    return Err(format!("Failed to open URL: {stderr}"));
-  }
+  // Open default browser across platforms
+  let status = if cfg!(target_os = "macos") {
+    Command::new("open").arg(&web_url).status()
+  } else if cfg!(target_os = "windows") {
+    Command::new("cmd").args(["/C", "start", "", &web_url]).status()
+  } else {
+    Command::new("xdg-open").arg(&web_url).status()
+  }.map_err(|e| format!("Failed to open browser: {e}"))?;
+  if !status.success() {
+    return Err("Failed to open URL".to_string());
+  }

Option B (preferred):

  • Cargo.toml: add dependency webbrowser = "0.8"
  • Replace the whole block with:
webbrowser::open(&web_url).map_err(|e| format!("Failed to open browser: {e}"))?;

30-32: Reorder: stash only after verifying merge, just before checkout.

Current flow can create a stray stash if the branch isn’t merged. Move dirty-state handling to right before switching to main.

-  // Check for dirty working directory
-  let has_stashed_changes = check_and_handle_dirty_state()?;
+  // (moved) Handle dirty working directory only after merge verification, before checkout

   // Detect the main branch (main or master)
   let main_branch = detect_main_branch(&repo)?;
   println!("🎯 Detected main branch: {main_branch}");

   // First fetch to get the latest remote history BEFORE checking merge status
   println!("📡 Fetching latest changes from remote...");
   if let Err(e) = fetch_remote() {
     println!("⚠️  Warning: Failed to fetch from remote: {e}");
     println!("   Continuing with local state, but results may not be accurate");
   }

   // Check if the current branch has been merged into main BEFORE switching branches
   println!("🔍 Checking if branch '{current_branch_name}' has been merged into '{main_branch}'...");
   …
-  // Now it's safe to switch to main branch
+  // Now it's safe to switch to main branch
+  // Check for dirty working directory right before switching
+  let has_stashed_changes = check_and_handle_dirty_state()?;
   println!("🔄 Switching to {main_branch} branch...");
   if let Err(e) = checkout_branch(&repo, &main_branch) {
     println!("❌ Failed to switch to {main_branch} branch: {e}");
     println!("   Staying on current branch '{current_branch_name}' for safety");
     return Err(format!("Failed to switch to {main_branch} branch").into());
   }

Also applies to: 66-74


104-147: Don’t parse .git/config; use libgit2 to get remote URL.

Manual parsing breaks with worktrees/alternate gitdirs. Query the remote via git2.

-pub fn open_remote_repository() -> Result<(), String> {
-  use std::fs;
-  use std::process::Command;
+pub fn open_remote_repository() -> Result<(), String> {
+  use std::process::Command;

   // 获取当前分支名
   let repo = Repository::open(".").map_err(|e| format!("Failed to open repository: {e}"))?;
   …
-  // 读取 .git/config 文件
-  let config_content = fs::read_to_string(".git/config").map_err(|e| format!("Failed to read .git/config: {e}"))?;
-
-  // 查找 remote "origin" 部分的 url
-  let mut in_origin_section = false;
-  let mut remote_url = None;
-
-  for line in config_content.lines() {
-    let line = line.trim();
-    if line == "[remote \"origin\"]" {
-      in_origin_section = true;
-      continue;
-    }
-    if line.starts_with('[') && line != "[remote \"origin\"]" {
-      in_origin_section = false;
-      continue;
-    }
-    if in_origin_section && line.starts_with("url = ") {
-      let url = line.strip_prefix("url = ").unwrap();
-      remote_url = Some(url.to_string());
-      break;
-    }
-  }
-
-  let url = remote_url.ok_or("No remote origin URL found in .git/config")?;
+  // Use libgit2 to resolve origin URL
+  let remote = repo.find_remote("origin").map_err(|e| format!("Failed to get remote 'origin': {e}"))?;
+  let url = remote.url().ok_or("No remote origin URL found")?.to_string();

216-225: Make checkout robust when only a remote branch exists.

Create local tracking branch from origin/ if the local branch is missing.

-fn checkout_branch(repo: &Repository, branch_name: &str) -> Result<(), Box<dyn std::error::Error>> {
-  let branch = repo.find_branch(branch_name, BranchType::Local)?;
-  let branch_ref = branch.get();
-  let commit = branch_ref.peel_to_commit()?;
-
-  repo.checkout_tree(commit.as_object(), None)?;
-  repo.set_head(&format!("refs/heads/{branch_name}"))?;
-
-  Ok(())
-}
+fn checkout_branch(repo: &Repository, branch_name: &str) -> Result<(), Box<dyn std::error::Error>> {
+  if let Ok(local) = repo.find_branch(branch_name, BranchType::Local) {
+    let commit = local.get().peel_to_commit()?;
+    repo.checkout_tree(commit.as_object(), None)?;
+    repo.set_head(&format!("refs/heads/{branch_name}"))?;
+    return Ok(());
+  }
+  // Fallback: create local from origin/<branch_name>
+  let remote_ref = format!("refs/remotes/origin/{branch_name}");
+  let rref = repo.find_reference(&remote_ref)?;
+  let commit = rref.peel_to_commit()?;
+  repo.branch(branch_name, &commit, false)?;
+  let new_local = repo.find_branch(branch_name, BranchType::Local)?;
+  new_local.set_upstream(Some(&format!("origin/{branch_name}")))?;
+  repo.checkout_tree(commit.as_object(), None)?;
+  repo.set_head(&format!("refs/heads/{branch_name}"))?;
+  Ok(())
+}
🧹 Nitpick comments (3)
src/git.rs (3)

465-477: Avoid potential panic when shortening commit hashes.

Slicing assumes ≥8 chars; be defensive.

-            println!("     {}. {} - {}", i + 1, &commit_hash.trim()[..8], msg);
+            let trimmed = commit_hash.trim();
+            let short: String = trimmed.chars().take(8).collect();
+            println!("     {}. {} - {}", i + 1, short, msg);

490-506: Prefer fast‑forward updates to avoid merge commits.

Use --ff-only to prevent unintended merge commits on main during automation.

-  println!("🔄 Merging origin/{main_branch}...");
-  let status = Command::new("git").args(["merge", &format!("origin/{main_branch}")]).status()?;
+  println!("🔄 Updating {main_branch} with fast-forward only...");
+  let status = Command::new("git").args(["merge", "--ff-only", &format!("origin/{main_branch}")]).status()?;

105-107: Remove redundant import inside function.

Command is already imported at module scope.

-  use std::process::Command;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b6216f and 7c1602f.

📒 Files selected for processing (1)
  • src/git.rs (4 hunks)
🔇 Additional comments (1)
src/git.rs (1)

75-81: Confirm: proceed with deletion even if pulling main fails?

Continuing after a failed pull can leave local main stale. Intentional?

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
Cargo.toml (1)

22-22: Consider updating to the latest version of webbrowser crate.

The latest version of the webbrowser crate is 1.0.5, but this PR adds version 0.8. The newer version likely includes bug fixes and improvements.

Apply this diff to update to the latest version:

-webbrowser = "0.8"
+webbrowser = "1.0.5"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c1602f and 265322a.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • Cargo.toml (1 hunks)
  • README.md (2 hunks)
  • src/git.rs (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/git.rs (1)
src/main.rs (2)
  • git (3-3)
  • main (14-136)
🔇 Additional comments (5)
src/git.rs (5)

104-177: LGTM! Cross-platform browser functionality properly implemented.

The open_remote_repository function correctly addresses the cross-platform browser opening issue that was flagged in previous reviews. The implementation using the webbrowser crate is the right approach.


30-32: Verify the reordered flow matches the previous recommendations.

The flow now checks for dirty state before verifying merge status, which was flagged in previous reviews as potentially problematic.

Based on the past review comment, the recommended order was:

  1. Fetch from origin
  2. Check if branch is merged into origin/
  3. Only then handle dirty state and switch branches

Please verify that Lines 37-62 properly implement this recommended sequence, or apply the previously suggested reordering.


206-224: LGTM! Robust checkout implementation.

The checkout_branch function now properly handles the case where only remote branches exist by creating local tracking branches.


118-145: Use libgit2 instead of manual .git/config parsing.

This implementation manually parses .git/config which can break with worktrees and alternate gitdirs.

Apply this diff to use libgit2 for origin URL retrieval:

-  // 读取 .git/config 文件
-  let config_content = fs::read_to_string(".git/config").map_err(|e| format!("Failed to read .git/config: {e}"))?;
-
-  // 查找 remote "origin" 部分的 url
-  let mut in_origin_section = false;
-  let mut remote_url = None;
-
-  for line in config_content.lines() {
-    let line = line.trim();
-
-    if line == "[remote \"origin\"]" {
-      in_origin_section = true;
-      continue;
-    }
-
-    if line.starts_with('[') && line != "[remote \"origin\"]" {
-      in_origin_section = false;
-      continue;
-    }
-
-    if in_origin_section && line.starts_with("url = ") {
-      let url = line.strip_prefix("url = ").unwrap();
-      remote_url = Some(url.to_string());
-      break;
-    }
-  }
-
-  let url = remote_url.ok_or("No remote origin URL found in .git/config")?;
+  // Resolve origin URL via libgit2
+  let remote = repo.find_remote("origin").map_err(|e| format!("Failed to get remote 'origin': {e}"))?;
+  let url = remote.url().ok_or("No remote origin URL found")?.to_string();

37-54: Merge checks use origin/

with local-main fallback

src/git.rs uses origin/{main_branch} in the merge checks and commands (branch --merged ≈line 340, rev-list ≈363 & 450, branch -r --merged ≈384, merge-base ≈415, merge ≈490) and falls back to local {main_branch} where needed — no change required.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
README.md (1)

119-153: Docs align with behavior; optionally clarify “main” is checked against remote.

Suggest explicitly stating the merge check is performed against origin/

to avoid confusion with stale local main.

src/jwt.rs (1)

45-46: Trim direct input to handle trailing whitespace/newlines.

Apply this diff:

-  } else {
-    args.input
-  };
+  } else {
+    args.input.trim().to_string()
+  };
src/git.rs (2)

44-56: Optional: clarify the remote in the log message.

You check against origin/

in helpers; reflect that in the message to reduce confusion.

Apply this diff:

-  println!("🔍 Checking if branch '{current_branch_name}' has been merged into '{main_branch}'...");
+  println!("🔍 Checking if branch '{current_branch_name}' has been merged into 'origin/{main_branch}'...");

108-110: Discover repo from subdirectories for robustness.

Use Repository::discover(".") to support being run from subdirs.

Apply this diff:

-  let repo = Repository::open(".").map_err(|e| format!("Failed to open repository: {e}"))?;
+  let repo = Repository::discover(".").map_err(|e| format!("Failed to open repository: {e}"))?;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 265322a and ccbb0c1.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • Cargo.toml (1 hunks)
  • README.md (2 hunks)
  • src/args.rs (3 hunks)
  • src/git.rs (4 hunks)
  • src/jwt.rs (1 hunks)
  • src/main.rs (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
src/main.rs (3)
src/git.rs (2)
  • finish_branch (9-102)
  • open_remote_repository (104-177)
src/tags.rs (1)
  • show_tags (11-86)
src/jwt.rs (1)
  • handle_jwt_command (30-34)
🔇 Additional comments (10)
src/args.rs (3)

21-24: LGTM: New top-level subcommands wired cleanly.

Branch (br) grouping and Jwt addition are consistent with argh usage.


98-112: LGTM: br command group and subcommands are well-structured.

Good separation via InspectForBranch + BranchCommand; names map predictably to CLI.


115-121: LGTM: Subcommand stubs for finish and open.

Doc comments present; no args required, matching current handlers.

README.md (1)

98-111: LGTM: Tags example matches CLI output formatting.

src/git.rs (4)

206-224: LGTM: Robust checkout with remote fallback and upstream linkage.

Handles missing local main/master gracefully.


30-32: Reorder: verify merge before stashing to avoid stray stashes.

Currently you may create a stash and then abort on “not merged,” leaving an unexpected stash. Verify merge first, then stash right before switching branches.

Apply these diffs:

@@
-  // Check for dirty working directory
-  let has_stashed_changes = check_and_handle_dirty_state()?;
+  // Defer dirty working directory check until after merge verification
@@
-  println!("✅ Branch '{current_branch_name}' has been merged into '{main_branch}'");
+  println!("✅ Branch '{current_branch_name}' has been merged into '{main_branch}'");
+
+  // Check for dirty working directory just before switching
+  let has_stashed_changes = check_and_handle_dirty_state()?;
@@
   // Now it's safe to switch to main branch
   println!("🔄 Switching to {main_branch} branch...");

Also applies to: 64-74, 91-98


104-106: Don’t parse .git/config manually; resolve origin via libgit2.

Worktrees/alternate gitdirs break hardcoded paths; use repo.find_remote("origin").

Apply these diffs:

@@
-pub fn open_remote_repository() -> Result<(), String> {
-  use std::fs;
+pub fn open_remote_repository() -> Result<(), String> {
@@
-  // 读取 .git/config 文件
-  let config_content = fs::read_to_string(".git/config").map_err(|e| format!("Failed to read .git/config: {e}"))?;
-
-  // 查找 remote "origin" 部分的 url
-  let mut in_origin_section = false;
-  let mut remote_url = None;
-
-  for line in config_content.lines() {
-    let line = line.trim();
-
-    if line == "[remote \"origin\"]" {
-      in_origin_section = true;
-      continue;
-    }
-
-    if line.starts_with('[') && line != "[remote \"origin\"]" {
-      in_origin_section = false;
-      continue;
-    }
-
-    if in_origin_section && line.starts_with("url = ") {
-      let url = line.strip_prefix("url = ").unwrap();
-      remote_url = Some(url.to_string());
-      break;
-    }
-  }
-
-  let url = remote_url.ok_or("No remote origin URL found in .git/config")?;
+  // Resolve origin URL via libgit2
+  let remote = repo.find_remote("origin").map_err(|e| format!("Failed to get remote 'origin': {e}"))?;
+  let url = remote.url().ok_or("No remote origin URL found")?.to_string();

Also applies to: 118-146


173-175: webbrowser dependency version valid: Cargo.toml specifies webbrowser = "0.8" and Cargo.lock resolves to v0.8.15, which exists on crates.io—no changes needed.

src/main.rs (2)

122-135: LGTM: br subcommand routing.

Finish/open handlers are correctly dispatched with clear error paths.


142-145: LGTM: JWT command delegated cleanly.

Error surfaced and process exits non‑zero on failure.

@NoEgAm NoEgAm merged commit 279e913 into main Oct 13, 2025
2 checks passed
@NoEgAm NoEgAm deleted the finbr branch October 13, 2025 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants