Repository navigation
Conversation
`git rev-parse --show-toplevel` walks up the directory tree, so opening a folder that isn't a git repository silently opened the nearest ancestor that is one. Someone whose home directory is a dotfiles repo could pick `~/projects/thing` and get their whole home directory added as a project, with no prompt: the existing "Initialize Git Repository?" dialog only fires when no ancestor is a repo at all. Resolution is unchanged — a subdirectory still resolves to the repo that contains it. getGitRoot now also reports whether the requested path is itself the work tree root, and openNew/openFromPath surface the dialog instead of deciding silently. Opening the enclosing repo stays the primary action and produces the same project as before; initializing the folder the user actually picked is now reachable as an alternative. `--show-prefix` is empty exactly at a work tree root, which keeps worktree roots (where `.git` is a file) and submodule roots correct and avoids comparing paths against git output, which would break on macOS where /tmp canonicalizes to /private/tmp, symlinks resolve to realpaths and a case-insensitive volume returns canonical casing.
📝 WalkthroughWalkthroughProject opening detects when a selected directory is nested inside a Git repository. The flow preserves the selected path and enclosing repository path. The Git initialization dialog can initialize the selected directory or open the enclosing repository. ChangesNested repository opening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InitGitDialog
participant useOpenProject
participant openFromPath
User->>InitGitDialog: Select an action for a nested folder
InitGitDialog->>useOpenProject: Confirm initialization or open enclosing repository
useOpenProject->>openFromPath: Open each unique enclosing repository
openFromPath-->>useOpenProject: Return opened projects
useOpenProject-->>InitGitDialog: Close dialog and resolve projects
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/renderer/react-query/projects/InitGitDialog.tsx (1)
13-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required component directory layout.
Move this component to
InitGitDialog/InitGitDialog.tsx. Add anInitGitDialog/index.tsbarrel export. The directInitGitDialog.tsxlayout does not meet the component organization rule.As per coding guidelines, “Use one component per file and organize components as
ComponentName/ComponentName.tsxwith anindex.tsbarrel export.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/react-query/projects/InitGitDialog.tsx` around lines 13 - 118, Reorganize the InitGitDialog component into an InitGitDialog directory by moving the existing InitGitDialog function implementation into InitGitDialog/InitGitDialog.tsx, then add InitGitDialog/index.ts as the barrel export. Update any imports or references to use the barrel export while preserving the component’s behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts`:
- Around line 1202-1205: Replace the interpolated shell commands in the worktree
setup with execFileSync("git", [...]) calls for both worktree operations near
the existing execSync calls, passing repository arguments and worktreePath as
separate arguments while preserving the current cwd and stdio options.
- Around line 1189-1193: Await the promise returned by the getGitRoot rejection
assertion in the test “throws NotGitRepoError when no ancestor is a repo.” Apply
the same await to the two later expect(...).rejects assertions in the related
tests so each rejection assertion completes before the test exits.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 893-896: Update the Git output parsing near the root-path return
to preserve the exact root value from split("\n"); remove the root.trim()
transformation while retaining prefix.trim() for the emptiness check. Ensure
openNew and openFromPath receive root paths with trailing whitespace unchanged.
---
Nitpick comments:
In `@apps/desktop/src/renderer/react-query/projects/InitGitDialog.tsx`:
- Around line 13-118: Reorganize the InitGitDialog component into an
InitGitDialog directory by moving the existing InitGitDialog function
implementation into InitGitDialog/InitGitDialog.tsx, then add
InitGitDialog/index.ts as the barrel export. Update any imports or references to
use the barrel export while preserving the component’s behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b93bbecc-0dd4-4799-bb11-2ad0514a95b6
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/renderer/react-query/projects/InitGitDialog.tsxapps/desktop/src/renderer/react-query/projects/useOpenProject.tsxapps/desktop/src/renderer/stores/git-init-dialog.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts:1158">
P3: The new getGitRoot tests compare the git-reported root against the locally joined `repoPath` with `toBe`. On Windows, `git rev-parse --show-toplevel` returns forward-slash paths while `join` produces backslashes, so these assertions would fail (and `symlinkSync` on a directory needs elevated privileges). Consider normalizing both sides through `realpathSync` or comparing with a platform-independent helper so the suite stays portable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| const result = await getGitRoot(repoPath); | ||
|
|
||
| expect(result.root).toBe(repoPath); |
There was a problem hiding this comment.
P3: The new getGitRoot tests compare the git-reported root against the locally joined repoPath with toBe. On Windows, git rev-parse --show-toplevel returns forward-slash paths while join produces backslashes, so these assertions would fail (and symlinkSync on a directory needs elevated privileges). Consider normalizing both sides through realpathSync or comparing with a platform-independent helper so the suite stays portable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts, line 1158:
<comment>The new getGitRoot tests compare the git-reported root against the locally joined `repoPath` with `toBe`. On Windows, `git rev-parse --show-toplevel` returns forward-slash paths while `join` produces backslashes, so these assertions would fail (and `symlinkSync` on a directory needs elevated privileges). Consider normalizing both sides through `realpathSync` or comparing with a platform-independent helper so the suite stays portable.</comment>
<file context>
@@ -1134,3 +1137,129 @@ describe("parsePrUrl", () => {
+
+ const result = await getGitRoot(repoPath);
+
+ expect(result.root).toBe(repoPath);
+ expect(result.isRoot).toBe(true);
+ });
</file context>
There was a problem hiding this comment.
Leaving this as is for now. CI is ubuntu-latest across every job in ci.yml, and the rest of this file already compares join()-built paths against git output the same way (the getWorktreeCreatedAt and createWorktree blocks above), so normalizing only the new tests would make them inconsistent with their neighbours. If the suite ever runs on Windows it needs a pass over the whole file rather than these nine cases.
Worth noting the production code is separator-agnostic — the root check is --show-prefix being empty, so nothing compares a joined path to git output at runtime. This is purely a test-portability point.
Trimming the toplevel line corrupts a path whose final directory name ends in whitespace, handing openNew/openFromPath a path that doesn't exist. Splitting on newline already separates the two values, so only a trailing CR needs stripping. Also await the rejects assertions in the new getGitRoot tests.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Read the toplevel and the prefix in two rev-parse calls instead of splitting one newline-delimited result: a directory name may contain a newline, which would truncate the root and open the wrong repository. Reading the toplevel with `raw` keeps stdout untrimmed, so a root whose final directory name ends in whitespace survives, and there is now a test covering that. The enclosing-repo action only appears when every selected folder has an enclosing repo. In a mixed selection it would otherwise open the nested folders and silently drop the plain ones, which are neither opened nor initialized. Also dedupe the resolved projects, since selecting a repo root together with one of its subfolders resolves to the same project twice; surface typed error results from openFromPath instead of dropping them; and track which action is running so the buttons no longer both read "Initializing...".
|
Pushed
Two I left alone, with reasoning in the threads: the Re-verified after the changes: |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
A directory name can end in a carriage return, and git round-trips that as `<name>\r\n`, so consuming the CR as part of the terminator returned a path that doesn't exist. Verified against git 2.50.1: `rev-parse --show-toplevel` ends the line with a bare LF, and a repo directory whose name ends in CR reports `.../cr\r\n`. Covered by a test alongside the trailing-whitespace one.
|
This PR has had no activity for 53 days and will close in 7 days unless there is new activity. If this contribution is still important or relevant, please leave a comment to keep it open. If it closes, you are welcome to reopen it or ask a maintainer to reopen it. Thank you for contributing! |
|
This PR was automatically closed due to inactivity to help keep the review queue manageable. This does not mean the contribution was rejected. If this contribution is still important or relevant, please reopen it or leave a comment asking a maintainer to reopen it. Thank you for contributing! |
What & why
getGitRootresolves a picked folder withgit rev-parse --show-toplevel, which walks up the directory tree. If the folder you pick isn't a git repository but any ancestor is, the app silently opens that ancestor instead.NotGitRepoErroris the only trigger for the existing "Initialize Git Repository?" dialog, and it's only thrown when no ancestor is a repo, so you're never asked.The case that hurts is a home directory tracked as a dotfiles repo. Pick
~/projects/new-thingin Add repository → Open project and Superset adds~as a project, named after your home directory, without a prompt. Drag-and-drop does the same thing throughopenFromPath.This does not change which repository gets opened. A subdirectory still resolves to the repo that contains it, and that stays the default action. The only change is that the app no longer makes the choice silently.
plans/20260601-v2-import-git-init-non-git-folder.md:100-102deferred this case on purpose:That's still the behavior.
getGitRootnow returns{ root, isRoot }, andopenNew/openFromPathshow the dialog whenisRootis false rather than writing a project row. The dialog gets a third button,Open <root>, which is the primary action and produces exactly the project you get today.Initialize Gitwas already wired toinitGitAndOpen, which inits and upserts the path verbatim, so initializing the folder you actually picked works without a router change — it just wasn't reachable whenever an ancestor repo existed.The root test is
--show-prefix, which is empty exactly when the path is the work tree root. That keeps two cases right that a filesystem or string check would get wrong:git worktreeroot.gitis a file, not a directory, sostatSync(join(p, ".git")).isDirectory()would call it "not a repo" and offer togit inita worktree Superset created itself.--show-toplevelfails on macOS three separate ways:/tmpand/varcanonicalize to/private/..., symlinks resolve to their realpath, and a case-insensitive volume returns canonical casing.Bare repos and
.gitdirectories still fail with "this operation must be run in a work tree" rather than "not a git repository", so they keep falling through as a generic error and are never offered agit init. There are tests for both.Scope is the Electron open-project path only.
packages/host-servicehas the same walk-up intryRevParseGitRoot/findByPath, butinitLocalRepoInPlacedocuments ancestor adoption as intentional idempotence, so offering "initialize here" there needs a new wire flag throughproject.create. That's a separate PR.How I tested it
bun test,bun run lintandbun run typecheckon macOS, bun 1.3.5, git 2.50.1.Nine new cases in
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.tscovering repo root, subdirectory, untracked child folder, no ancestor repo, worktree root, worktree subdirectory, symlinked repo root, bare repo and.gitdirectory.getGitRoothad no coverage before this.The subdirectory and worktree cases are the regression guards — they assert the walk-up still happens and that a worktree root is still a root.
packages/host-serviceis untouched, and the two tests that lock the walk-up semantics pass unmodified:Full desktop suite,
bun run lintandbun run typecheck:The 2 failures are pre-existing. I confirmed they fail identically on a clean
main(a316a1f) with my changes stashed:Shell Environment > getProcessEnvWithShellPath applies shell PATH and preserves string varsandcreateWorktree hook tolerance > continues when git is killed mid-hook (timeout shape) but worktree is created. Baseline there is 37 pass / 2 fail in that file; with this change it's 46 pass / 2 fail.src/no-main-process-blocking.test.tspasses withallowedCountsunedited (git.ts stays at 21, projects.ts at 6) — the change reuses the existing git client rather than adding one.I haven't captured screenshots. I don't have a working desktop app to drive over CDP, and I'd rather not put a mocked-up image in a PR description. Happy to add them if you'd like, or the dialog copy is:
Open the enclosing repository?/Users/me).Cancel·Initialize Git·Open dotfilesWhen there's no enclosing repo the dialog is unchanged, both copy and buttons.
Checklist
type(scope): subject)bun run lintandbun run typecheckpass (CI fails on lint warnings too)Summary by cubic
Ask the user to confirm when they pick a folder inside a Git repo. The dialog lets them open the enclosing repo or initialize Git in the selected folder; default stays “open enclosing,” and only shows when all selected folders are nested.
openFromPath.getGitRootnow returns{ root, isRoot }; reads--show-topleveland--show-prefixin two calls to handle names with newlines; usesrawto preserve trailing whitespace and strips only the trailing LF (preserves a trailing CR). Keeps worktrees, submodules, symlinks, and macOS path canonicalization correct..gitdirectory.Written for commit ff12eba. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes