Skip to content

fix: resolve relative paths in working tree diffs using workspace root#110

Open
Straffern wants to merge 2 commits intojnsahaj:mainfrom
Straffern:fix-working-tree-path-resolution
Open

fix: resolve relative paths in working tree diffs using workspace root#110
Straffern wants to merge 2 commits intojnsahaj:mainfrom
Straffern:fix-working-tree-path-resolution

Conversation

@Straffern
Copy link

@Straffern Straffern commented Jan 14, 2026

This pull request improves how file paths are resolved when reading files from the working tree, ensuring that relative paths are interpreted correctly with respect to the workspace or repository root. The main change is the introduction of a get_workspace_root method to the VcsBackend trait, with implementations for both Git and JJ backends. This change ensures consistent and correct file access, especially in cases where the current working directory may differ from the repository root.

Disclaimer: I do not know Rust. I have never coded in Rust. The clanker wrote this. I have created/applied a .patch in my NixOS config and it seems to work nicely.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed working-tree file path resolution so relative paths are resolved against the repository workspace, preventing wrong-file reads.
    • Ensured the editor opens the correct file (using repository-root–based paths) when launching from subdirectories, including when opening at a specific line.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

📝 Walkthrough

Walkthrough

Added a runtime workspace-root accessor to VcsBackend and implemented it for Git and Jj. Diff code now resolves working-tree file paths against that workspace root; Jj backend now discovers and stores the workspace root during initialization. (47 words)

Changes

Cohort / File(s) Summary
VCS Backend Trait
src/vcs/backend.rs
Added fn get_workspace_root(&self) -> PathBuf to expose the repository workspace root at runtime.
Git Backend Implementation
src/vcs/git.rs
Implemented get_workspace_root() using the repository workdir() or a fallback (cwd/"."), returning a PathBuf.
Jj Backend Implementation
src/vcs/jj.rs
JjBackend::new now locates the workspace root via find_workspace_dir() and loads the workspace from it; stores workspace root and implements get_workspace_root(). Constructor signature updated to new(path: &Path).
Diff Command Consumers
src/command/diff/git.rs, src/command/diff/app.rs
Resolve working-tree filenames by joining the backend-provided workspace root with the filename before reading files or launching the editor (use absolute/full path instead of raw filename).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant DiffCmd as Diff Command
    participant Vcs as VcsBackend
    participant FS as Filesystem
    participant Editor

    rect rgba(200,230,255,0.5)
    User->>DiffCmd: request diff / open file
    DiffCmd->>Vcs: get_workspace_root()
    Vcs-->>DiffCmd: workspace_root (PathBuf)
    DiffCmd->>FS: read file at workspace_root + filename
    FS-->>DiffCmd: file contents
    DiffCmd->>Editor: launch editor with full path
    Editor-->>User: editor opens file
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 I dug up roots beneath my paws,

I stitched the paths without a pause,
From git to jj I hop and cheer,
Full paths now open—editor near! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ 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 accurately summarizes the main change: implementing workspace root path resolution for working tree diffs to fix relative path issues.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b8fd44a and a451725.

📒 Files selected for processing (5)
  • src/command/diff/app.rs
  • src/command/diff/git.rs
  • src/vcs/backend.rs
  • src/vcs/git.rs
  • src/vcs/jj.rs
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/vcs/git.rs
  • src/command/diff/app.rs
  • src/vcs/jj.rs
  • src/command/diff/git.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/vcs/backend.rs (2)
src/vcs/git.rs (1)
  • get_workspace_root (787-792)
src/vcs/jj.rs (1)
  • get_workspace_root (986-988)
🔇 Additional comments (2)
src/vcs/backend.rs (2)

1-2: LGTM!

The PathBuf import is correctly added alongside the existing Path import and is required for the new trait method signature.


133-136: Trait method definition is correct; the reported duplicate path bug could not be verified.

The get_workspace_root() method is well-documented and correctly defined. Both implementations (Git and JJ) return valid workspace root paths, and the usage sites correctly join these with relative filenames from diff outputs (e.g., src/foo.rs).

However, the reported bug about duplicate path segments (e.g., .../api/api/lib/...) could not be confirmed in the code structure. Filenames are correctly sourced from VCS backends as relative paths, and the path joining logic appears sound. If this bug occurs in practice, it may require runtime debugging or examination of actual editor invocation to diagnose.

✏️ Tip: You can disable this entire section by setting review_details to false in your review 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.

❤️ Share

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

@jnsahaj
Copy link
Owner

jnsahaj commented Jan 14, 2026

Hi, the change looks good but I'm curious about the issue you faced, which prompted you to make this change.
Can you describe that?

@Straffern
Copy link
Author

Straffern commented Jan 14, 2026

Hi, the change looks good but I'm curious about the issue you faced, which prompted you to make this change. Can you describe that?

Yeah, I am working from a monorepo and thus often want to execute lumen from within a subdir of the repo root.
eg. lumen diff
Previously to this fix, I would get an error that said I was not currently in a jj repo (or maybe it said git, but I'm actively using jj and using jj revsets with lumen).

@Straffern
Copy link
Author

Hmm. There might be a bug with this.. When I try and open a file, from lumen diff (e keybinding).
I'm running lumen diff from /home/user/work/monorepo/api
expected:
Opens neovim for file: /home/user/work/monorepo/api/lib/pn/some_file.ex

What actually happens:
neovim tries to open /home/user/work/monorepo/api/api/lib/pn/some_file.ex (does not exist )

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.

2 participants