feat(desktop): add Jujutsu (jj) VCS support via provider abstraction#1561
Open
andreasasprou wants to merge 9 commits intosuperset-sh:mainfrom
Open
feat(desktop): add Jujutsu (jj) VCS support via provider abstraction#1561andreasasprou wants to merge 9 commits intosuperset-sh:mainfrom
andreasasprou wants to merge 9 commits intosuperset-sh:mainfrom
Conversation
Contributor
|
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 a VCS provider abstraction (Git + Jujutsu), implements GitProvider and JjProvider, migrates project/workspace/branch flows to use getVcsProvider, and records per-project vcsType via a DB migration. All workspace lifecycle and branch/config operations are routed through the provider API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant DB
participant VcsFactory as getVcsProvider
participant VcsImpl as Git/Jj
Client->>Router: request create/open project (path)
Router->>DB: resolve mainRepoPath (worktrees → projects)
Router->>VcsFactory: getVcsProvider(mainRepoPath)
VcsFactory->>VcsImpl: select & return provider (cache)
Router->>VcsImpl: getDefaultBranch / getCurrentBranch / listBranches
VcsImpl-->>Router: branch info
Router->>DB: upsert project with vcsType
DB-->>Router: persisted project
Router-->>Client: return project (includes vcsType)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
103-103:⚠️ Potential issue | 🟡 MinorGit-specific language in user-facing warning for jj repos.
"Worktree not found in git (may have been manually removed)"will be shown verbatim to jj users. The same wording appears at line 384 in thecanDeleteWorktreehandler.🔧 Suggested update (applies to both line 103 and line 384)
- warning: "Worktree not found in git (may have been manually removed)", + warning: "Workspace not found (may have been manually removed)",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts` at line 103, The user-facing warning string "Worktree not found in git (may have been manually removed)" uses Git-specific wording and should be made neutral for jj repos; update the literal in the delete worktree handler in delete.ts and the canDeleteWorktree handler to a generic message such as "Worktree not found (may have been manually removed)". Locate the string occurrences in the delete procedure (in the handler that throws or returns that warning) and in the canDeleteWorktree function and replace the message text so both handlers present the non-Git-specific wording.
🧹 Nitpick comments (9)
packages/local-db/src/schema/zod.ts (1)
142-144: DuplicateVcsTypedefinition risks divergence.
VcsTypeis independently declared here ((typeof VCS_TYPES)[number]) and again as a plain union type inapps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/types.ts(line 4). They are compatible today, but a future addition of a new VCS kind needs to be made in both places or a type error at the DB layer may not surface as a compile error in the router layer (or vice versa).Consider having
vcs/types.tsimport and re-exportVcsTypefrom this package, or have the schema import fromvcs/types.ts, so there is a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/local-db/src/schema/zod.ts` around lines 142 - 144, There are two duplicate VcsType definitions (the const VCS_TYPES and the derived type VcsType here and a separate plain union VcsType in the other module), so pick a single source of truth and wire the other module to import it: either export VcsType (and VCS_TYPES if needed) from this package and update the other module to import/re-export that type, or remove this local VcsType and import the canonical VcsType from the other module; update all imports to reference the chosen exported symbol (VCS_TYPES and/or VcsType) so only one definition remains.apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/index.ts (2)
102-116:getRepoRootinstantiates aJjProvideron every call, even when jj is unavailable.If
jjCLI isn't installed, everygetRepoRootcall pays the cost of creating aJjProviderand catching the failed exec. Consider guarding withisJjCliAvailable()first.♻️ Proposed fix
export async function getRepoRoot(path: string): Promise<string> { - // Try jj first — it works for both colocated and pure jj repos - try { - const jjProvider = new JjProvider(); - const root = await jjProvider.getRepoRoot(path); - if (root && existsSync(join(root, ".jj"))) { - return root; + if (isJjCliAvailable()) { + try { + const jjProvider = new JjProvider(); + const root = await jjProvider.getRepoRoot(path); + if (root && existsSync(join(root, ".jj"))) { + return root; + } + } catch { + // jj not a jj repo, fall through to git } - } catch { - // jj not available or not a jj repo, fall through to git }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/index.ts` around lines 102 - 116, Change getRepoRoot to first check isJjCliAvailable() before creating a JjProvider: call await isJjCliAvailable() and only instantiate new JjProvider() and call jjProvider.getRepoRoot(path) when that returns true; otherwise skip the try/catch and directly construct/use GitProvider.getRepoRoot(path). Keep existing behavior of validating the ".jj" folder and falling back to GitProvider.getRepoRoot(path) when jj returns no root.
41-51: SynchronousexecFileSyncblocks the Electron main process.
isJjCliAvailable()usesexecFileSyncwhich blocks the main thread for up to 5 seconds ifjjis not found. While it's memoized (runs once), it can still cause a noticeable UI freeze on the first call. Consider making this async or deferring the check.♻️ Proposed async alternative
-let jjAvailabilityResult: boolean | null = null; +let jjAvailabilityPromise: Promise<boolean> | null = null; -function isJjCliAvailable(): boolean { - if (jjAvailabilityResult !== null) return jjAvailabilityResult; - - try { - execFileSync("jj", ["version"], { timeout: 5_000, stdio: "ignore" }); - jjAvailabilityResult = true; - } catch { - jjAvailabilityResult = false; - } - return jjAvailabilityResult; +async function isJjCliAvailable(): Promise<boolean> { + if (!jjAvailabilityPromise) { + jjAvailabilityPromise = (async () => { + try { + const { execFile } = await import("node:child_process"); + const { promisify } = await import("node:util"); + await promisify(execFile)("jj", ["version"], { timeout: 5_000 }); + return true; + } catch { + return false; + } + })(); + } + return jjAvailabilityPromise; }This would also require making
getVcsProviderasync, which is a larger change. If deferred, add a comment noting the potential UI freeze.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/index.ts` around lines 41 - 51, The isJjCliAvailable function blocks the Electron main thread by using execFileSync; change it to a non-blocking implementation or defer the check: replace execFileSync with an async child_process.execFile call (or spawn) and make isJjCliAvailable return a Promise<boolean> that resolves and caches jjAvailabilityResult, and update callers (e.g., getVcsProvider) to await the async result; if you prefer not to change call sites now, instead add a clear TODO comment above isJjCliAvailable explaining the current sync behavior and the potential UI freeze on first call and defer the check to a background task so it doesn't run on the critical path.apps/desktop/src/lib/trpc/routers/changes/branches.ts (1)
139-142:--unsetbypasses VCS abstraction — consider adding toVcsProviderinterface.The
setBaseBranchConfigpath uses the VCS provider, but the unset path falls back to rawsimpleGit. This will work for colocated jj repos (since.gitexists), but it breaks the abstraction. If a non-colocated jj mode is ever supported, this will silently fail.Consider adding an
unsetBaseBranchConfigmethod toVcsProvider, or at minimum, a// TODOto track this gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/branches.ts` around lines 139 - 142, The code bypasses the VcsProvider by calling simpleGit.raw to unset branch.${currentBranch}.base, breaking the VCS abstraction used by setBaseBranchConfig; add an unsetBaseBranchConfig method to the VcsProvider interface and implement it in the providers (and call VcsProvider.unsetBaseBranchConfig(currentBranch) instead of using simpleGit(input.worktreePath).raw(...)), or at minimum add a clear TODO comment next to the simpleGit usage referencing VcsProvider and setBaseBranchConfig so this gap is tracked for non-colocated jj modes.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
757-759: RedundantdetectVcsTypecall aftergetVcsProvider.
getVcsProvider(clonePath)at line 757 already callsdetectVcsTypeinternally. The explicitdetectVcsType(clonePath)at line 759 is redundant. Consider reusingvcsForClone.typeinstead.♻️ Proposed fix
const vcsForClone = getVcsProvider(clonePath); const defaultBranch = await vcsForClone.getDefaultBranch(clonePath); - const vcsType = detectVcsType(clonePath); const project = localDb .insert(projects) .values({ mainRepoPath: clonePath, name, color: getDefaultProjectColor(), defaultBranch, - vcsType, + vcsType: vcsForClone.type, })Also applies to: 767-767
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts` around lines 757 - 759, The code redundantly calls detectVcsType(clonePath) after obtaining vcsForClone from getVcsProvider(clonePath); instead reuse the provider's type property (vcsForClone.type) when setting vcsType and in the other occurrence mentioned, remove the extra detectVcsType calls and replace them with references to vcsForClone.type so you only call getVcsProvider once and read its .type for VCS type information.apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts (1)
665-672:getRepoRootbypasses thejj()wrapper and its repo lock.This directly calls
execFileAsyncinstead of going through thejj()helper. Whilejj rootis read-only and unlikely to cause lock contention, it also skips--no-pagerand--color=never. Consider using thejj()wrapper for consistency, or at least adding--no-pagerand--color=never.♻️ Proposed fix
async getRepoRoot(path: string): Promise<string> { - const env = await getJjEnv(); - const { stdout } = await execFileAsync("jj", ["root", "-R", path], { - env, - timeout: 10_000, - }); - return stdout.trim(); + const output = await jj(path, ["root"]); + return output.trim(); }Note: This would add repo-lock serialization, which may or may not be desired for a simple root lookup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts` around lines 665 - 672, getRepoRoot currently calls execFileAsync directly (and uses getJjEnv) which bypasses the jj() wrapper and its repo lock plus standard flags; change getRepoRoot to call the jj() helper instead of execFileAsync so it inherits repo-lock serialization and flags like --no-pager/--color=never (or if you intentionally want to avoid locking, at minimum include those flags when invoking jj root and keep using getJjEnv), updating references in getRepoRoot to use jj() rather than execFileAsync and preserving the timeout behavior.apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/types.ts (3)
15-16:isDetachedis redundant withbranch === null.The field comment on
branchalready states "null if detached," makingisDetacheda derived value (branch === null). Keeping both in sync is extra discipline for every call site.Either remove
isDetachedand let callers checkbranch === null, or replacebranch: string | nullwith a discriminated union:♻️ Proposed refactor (discriminated union option)
export interface ExternalWorkspace { path: string; - /** Branch name (git) or bookmark name (jj), null if detached */ - branch: string | null; - isDetached: boolean; isBare: boolean; +} & ( + | { isDetached: false; branch: string } + | { isDetached: true; branch: null } +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/types.ts` around lines 15 - 16, Remove the redundant isDetached boolean from the VCS type and rely on branch === null as the source of truth (or alternatively convert the existing type to a discriminated union where one variant has branch: string and another has branch: null), updating all references to the type and any code that reads isDetached to instead check branch === null; specifically edit the type declaration that currently includes isDetached and branch, remove isDetached, and replace usages of isDetached with (branch === null) or implement the discriminated-union variant for clearer typing across functions that consume this type.
5-5:VcsTypeis defined in two places — risk of divergence.
packages/local-db/src/schema/zod.tsalready exportsVcsTypederived fromVCS_TYPES. Redefining it here as a literal union means the two can silently diverge if a third VCS type is added to the DB schema (or to this file) without a matching update to the other.Consider importing from the canonical source instead:
♻️ Proposed refactor
-export type VcsType = "git" | "jj"; +export type { VcsType } from "@local-db/schema/zod"; // adjust alias to match tsconfig🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/types.ts` at line 5, The literal union type VcsType declared here risks diverging from the canonical VCS_TYPES-derived type in the schema; replace this local declaration by importing the canonical type instead (e.g. import the exported VcsType or derive it from the exported VCS_TYPES constant from the schema module) so both usages share the same source of truth; update any usages in this file to use the imported VcsType and remove the local `export type VcsType = "git" | "jj";`.
34-213: Inconsistent path-parameter naming acrossVcsProvidermakes the contract ambiguous for implementors.Three distinct names—
mainRepoPath,repoPath, andworkspacePath—are used across the interface, but no documentation explains their semantics or when each applies.getAheadBehindCounttakesrepoPathwhilegetDefaultBranchtakesmainRepoPath, even though both conceptually operate on the same repository root.hasUncommittedChanges/hasUnpushedCommitsuseworkspacePath(a worktree path), which is a meaningful distinction, but it isn't stated.At minimum, add a brief doc comment above
VcsProviderclarifying the naming convention (e.g., "mainRepoPath= primary git/jj repo root;repoPath= any repo or worktree path acceptable;workspacePath= a checked-out worktree/workspace"), or normalise to two explicit names (repoPath/workspacePath) with consistent usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/types.ts` around lines 34 - 213, The interface VcsProvider uses three inconsistent path parameter names (mainRepoPath, repoPath, workspacePath); update the contract by either (A) adding a concise doc comment immediately above the VcsProvider declaration that defines: "mainRepoPath = primary repository root; repoPath = any repository or worktree path acceptable to VCS calls; workspacePath = a checked-out worktree/workspace path" and clarifying which methods expect each, or (B) normalize parameters to two names (repoPath and workspacePath) and update all method signatures and their JSDoc accordingly (examples to update: createWorkspace, createWorkspaceFromExistingBranch, removeWorkspace, workspaceExists, listExternalWorkspaces, getBranchWorkspacePath, hasUncommittedChanges, hasUnpushedCommits, getAheadBehindCount, getCurrentBranch, listBranches, getDefaultBranch, refreshDefaultBranch, fetchDefaultBranch, deleteLocalBranch, checkoutBranch, safeCheckoutBranch, refExistsLocally, hasOriginRemote, branchExistsOnRemote, getRepoRoot, getBaseBranchConfig, setBaseBranchConfig) so implementors clearly know which path to pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts`:
- Around line 94-102: Remove the unused exported function isJjAvailable from
jj-provider.ts: delete the entire isJjAvailable declaration and its export so
the repository uses the single cached check isJjCliAvailable in index.ts (which
is consumed by getVcsProvider); ensure there are no remaining imports/usages of
isJjAvailable anywhere and update any barrel/index exports if they re-exported
it.
- Around line 420-457: In getAheadBehindCount, the jj revset interpolation uses
params.defaultBranch unquoted which breaks when the branch name contains revset
operators; update both revset strings used for aheadOutput and behindOutput to
quote the branch name by wrapping params.defaultBranch in double quotes inside
the revset (e.g., use ::"${params.defaultBranch}" wherever
::${params.defaultBranch} is currently used) so the revset parser treats the
branch name as a literal.
- Around line 248-272: The code spawns a hardcoded Unix '/bin/rm' (in the block
that renames workspacePath to tempPath using rename(...) and spawn(...)) which
breaks on Windows; change the spawn call to choose platform-appropriate
command/args (e.g. if process.platform === 'win32' use 'cmd' with [' /c',
'rmdir', '/s', '/q', tempPath] or PowerShell Remove-Item invocation, otherwise
use '/bin/rm' with ['-rf', tempPath]), keep detached: true, stdio: "ignore",
call child.unref(), and preserve the existing child.on("error", ...) and
child.on("exit", ...) logging; optionally add a comment mentioning the
fs.rm(tempPath, { recursive: true, force: true }) alternative if macOS hang is
resolved.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts`:
- Around line 51-52: The variable vcs is declared inside the try block via const
(from getVcsProvider(mainRepoPath)) but is later referenced in the catch,
causing a possible ReferenceError; hoist the variable declaration out of the try
(e.g., let vcs: ReturnType<typeof getVcsProvider> | undefined) before the try,
assign it inside the try by calling getVcsProvider(mainRepoPath), and update the
catch to safely check for vcs !== undefined before using it (or handle the
undefined case); apply the same hoisting/fallback approach to any other
variables in that region referenced in both try and catch (lines around
437-456).
---
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts`:
- Line 103: The user-facing warning string "Worktree not found in git (may have
been manually removed)" uses Git-specific wording and should be made neutral for
jj repos; update the literal in the delete worktree handler in delete.ts and the
canDeleteWorktree handler to a generic message such as "Worktree not found (may
have been manually removed)". Locate the string occurrences in the delete
procedure (in the handler that throws or returns that warning) and in the
canDeleteWorktree function and replace the message text so both handlers present
the non-Git-specific wording.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/changes/branches.ts`:
- Around line 139-142: The code bypasses the VcsProvider by calling
simpleGit.raw to unset branch.${currentBranch}.base, breaking the VCS
abstraction used by setBaseBranchConfig; add an unsetBaseBranchConfig method to
the VcsProvider interface and implement it in the providers (and call
VcsProvider.unsetBaseBranchConfig(currentBranch) instead of using
simpleGit(input.worktreePath).raw(...)), or at minimum add a clear TODO comment
next to the simpleGit usage referencing VcsProvider and setBaseBranchConfig so
this gap is tracked for non-colocated jj modes.
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 757-759: The code redundantly calls detectVcsType(clonePath) after
obtaining vcsForClone from getVcsProvider(clonePath); instead reuse the
provider's type property (vcsForClone.type) when setting vcsType and in the
other occurrence mentioned, remove the extra detectVcsType calls and replace
them with references to vcsForClone.type so you only call getVcsProvider once
and read its .type for VCS type information.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/index.ts`:
- Around line 102-116: Change getRepoRoot to first check isJjCliAvailable()
before creating a JjProvider: call await isJjCliAvailable() and only instantiate
new JjProvider() and call jjProvider.getRepoRoot(path) when that returns true;
otherwise skip the try/catch and directly construct/use
GitProvider.getRepoRoot(path). Keep existing behavior of validating the ".jj"
folder and falling back to GitProvider.getRepoRoot(path) when jj returns no
root.
- Around line 41-51: The isJjCliAvailable function blocks the Electron main
thread by using execFileSync; change it to a non-blocking implementation or
defer the check: replace execFileSync with an async child_process.execFile call
(or spawn) and make isJjCliAvailable return a Promise<boolean> that resolves and
caches jjAvailabilityResult, and update callers (e.g., getVcsProvider) to await
the async result; if you prefer not to change call sites now, instead add a
clear TODO comment above isJjCliAvailable explaining the current sync behavior
and the potential UI freeze on first call and defer the check to a background
task so it doesn't run on the critical path.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts`:
- Around line 665-672: getRepoRoot currently calls execFileAsync directly (and
uses getJjEnv) which bypasses the jj() wrapper and its repo lock plus standard
flags; change getRepoRoot to call the jj() helper instead of execFileAsync so it
inherits repo-lock serialization and flags like --no-pager/--color=never (or if
you intentionally want to avoid locking, at minimum include those flags when
invoking jj root and keep using getJjEnv), updating references in getRepoRoot to
use jj() rather than execFileAsync and preserving the timeout behavior.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/types.ts`:
- Around line 15-16: Remove the redundant isDetached boolean from the VCS type
and rely on branch === null as the source of truth (or alternatively convert the
existing type to a discriminated union where one variant has branch: string and
another has branch: null), updating all references to the type and any code that
reads isDetached to instead check branch === null; specifically edit the type
declaration that currently includes isDetached and branch, remove isDetached,
and replace usages of isDetached with (branch === null) or implement the
discriminated-union variant for clearer typing across functions that consume
this type.
- Line 5: The literal union type VcsType declared here risks diverging from the
canonical VCS_TYPES-derived type in the schema; replace this local declaration
by importing the canonical type instead (e.g. import the exported VcsType or
derive it from the exported VCS_TYPES constant from the schema module) so both
usages share the same source of truth; update any usages in this file to use the
imported VcsType and remove the local `export type VcsType = "git" | "jj";`.
- Around line 34-213: The interface VcsProvider uses three inconsistent path
parameter names (mainRepoPath, repoPath, workspacePath); update the contract by
either (A) adding a concise doc comment immediately above the VcsProvider
declaration that defines: "mainRepoPath = primary repository root; repoPath =
any repository or worktree path acceptable to VCS calls; workspacePath = a
checked-out worktree/workspace path" and clarifying which methods expect each,
or (B) normalize parameters to two names (repoPath and workspacePath) and update
all method signatures and their JSDoc accordingly (examples to update:
createWorkspace, createWorkspaceFromExistingBranch, removeWorkspace,
workspaceExists, listExternalWorkspaces, getBranchWorkspacePath,
hasUncommittedChanges, hasUnpushedCommits, getAheadBehindCount,
getCurrentBranch, listBranches, getDefaultBranch, refreshDefaultBranch,
fetchDefaultBranch, deleteLocalBranch, checkoutBranch, safeCheckoutBranch,
refExistsLocally, hasOriginRemote, branchExistsOnRemote, getRepoRoot,
getBaseBranchConfig, setBaseBranchConfig) so implementors clearly know which
path to pass.
In `@packages/local-db/src/schema/zod.ts`:
- Around line 142-144: There are two duplicate VcsType definitions (the const
VCS_TYPES and the derived type VcsType here and a separate plain union VcsType
in the other module), so pick a single source of truth and wire the other module
to import it: either export VcsType (and VCS_TYPES if needed) from this package
and update the other module to import/re-export that type, or remove this local
VcsType and import the canonical VcsType from the other module; update all
imports to reference the chosen exported symbol (VCS_TYPES and/or VcsType) so
only one definition remains.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts (1)
1008-1045:⚠️ Potential issue | 🟡 MinorAdd
setBaseBranchConfigcall for bulk-imported external worktrees.The
openExternalWorktreemutation (line 850) callsvcsForExt.setBaseBranchConfigafter creating a new external worktree record, which writes the git configuration for base branch tracking. TheimportAllWorktreesloop (lines 1008–1045) performs the identical database insertions but skips this call, leaving bulk-imported worktrees without the corresponding git config. Both paths should consistently set the git base branch configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around lines 1008 - 1045, The bulk-import loop in importAllWorktrees that iterates externalWorktrees creates worktree and workspace records but omits calling vcsForExt.setBaseBranchConfig, so imported worktrees lack the git base-branch config; update the loop (the block that creates worktree via localDb.insert(worktrees).values(...).returning().get() and then inserts into workspaces and calls copySupersetConfigToWorktree) to call vcsForExt.setBaseBranchConfig(project.mainRepoPath, ext.path, defaultBranch) (matching the call made in openExternalWorktree) after creating the worktree record and before/after copySupersetConfigToWorktree so imported worktrees get the same base-branch configuration as single imports.
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts (1)
198-215: Loop variablebranchshadows the outerbranchparameter.
for (const branch of commonBranches)reuses the same identifier as the destructured workspacebranchparameter. No functional bug today (the outerbranchisn't read inside the loop body), but it's a readability hazard and will be flagged byno-shadowlinting rules.♻️ Proposed rename
- for (const branch of commonBranches) { - if (branch === effectiveBaseBranch) continue; + for (const fallback of commonBranches) { + if (fallback === effectiveBaseBranch) continue; if (checkOriginRefs) { - const fallbackOriginRef = `origin/${branch}`; + const fallbackOriginRef = `origin/${fallback}`; if (await vcs.refExistsLocally(mainRepoPath, fallbackOriginRef)) { console.log( - `[workspace-init] ${reason}. Using fallback tracking ref: ${fallbackOriginRef}`, + `[workspace-init] ${reason}. Using fallback tracking ref: ${fallbackOriginRef}`, ); - return { ref: fallbackOriginRef, fallbackBranch: branch }; + return { ref: fallbackOriginRef, fallbackBranch: fallback }; } } - if (await vcs.refExistsLocally(mainRepoPath, branch)) { + if (await vcs.refExistsLocally(mainRepoPath, fallback)) { console.log( - `[workspace-init] ${reason}. Using fallback local branch: ${branch}`, + `[workspace-init] ${reason}. Using fallback local branch: ${fallback}`, ); - return { ref: branch, fallbackBranch: branch }; + return { ref: fallback, fallbackBranch: fallback }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts` around lines 198 - 215, The loop variable `branch` in `for (const branch of commonBranches)` shadows the outer `branch` parameter; rename the loop identifier (e.g., to `candidateBranch` or `commonBranch`) wherever it's used inside this block (references to `effectiveBaseBranch`, `checkOriginRefs`, `vcs.refExistsLocally(mainRepoPath, ...)`, `fallbackOriginRef`, `reason`, and the returned `fallbackBranch`) so the outer `branch` parameter is not shadowed and lint `no-shadow` warnings are resolved.apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts (1)
648-649: Inconsistent VCS provider variable naming across procedures.Three procedures use decorated names (
vcsForOpen,vcsForExt,vcsForImport) while the remaining procedures (create,createBranchWorkspace,handleNewWorktree) use plainvcs. Since each is scoped to its own closure there is no conflict — the qualifiers add noise without clarity benefit.♻️ Suggested rename
- const vcsForOpen = getVcsProvider(project.mainRepoPath); - const exists = await vcsForOpen.workspaceExists( + const vcs = getVcsProvider(project.mainRepoPath); + const exists = await vcs.workspaceExists(- const vcsForExt = getVcsProvider(project.mainRepoPath); - const exists = await vcsForExt.workspaceExists( + const vcs = getVcsProvider(project.mainRepoPath); + const exists = await vcs.workspaceExists((and the later
vcsForExt.setBaseBranchConfigcall at line 850)- const vcsForImport = getVcsProvider(project.mainRepoPath); + const vcs = getVcsProvider(project.mainRepoPath);(and all subsequent
vcsForImport.*calls)Also applies to: 710-711, 945-946
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around lines 648 - 649, The variable naming for VCS providers is inconsistent: replace scoped decorated names (vcsForOpen, vcsForExt, vcsForImport) with a single plain identifier vcs inside each procedure to reduce noise; locate calls that obtain the provider via getVcsProvider and rename the local variable to vcs and update all subsequent references in that closure (e.g., the workspaceExists call, vcsForExt.setBaseBranchConfig, and all vcsForImport.* usages) without changing other scopes or global names, ensuring each procedure (create, createBranchWorkspace, handleNewWorktree and the other affected closures) consistently uses vcs.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
759-761:detectVcsTypeis called redundantly alongsidegetVcsProvider
getVcsProvider(clonePath)at line 759 already invokesdetectVcsTypeinternally (seevcs/index.tsline 63). The explicit call on line 761 duplicates the filesystem check. Consider callingdetectVcsTypeonce and reusing the result:♻️ Proposed refactor
- const vcsForClone = getVcsProvider(clonePath); - const defaultBranch = await vcsForClone.getDefaultBranch(clonePath); - const vcsType = detectVcsType(clonePath); + const vcsType = detectVcsType(clonePath); + const vcsForClone = getVcsProvider(clonePath); + const defaultBranch = await vcsForClone.getDefaultBranch(clonePath);
getVcsProvidercaches by path, so callingdetectVcsTypefirst doesn't change caching behaviour — it just makes the data flow explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts` around lines 759 - 761, The code calls detectVcsType(clonePath) redundantly after getVcsProvider(clonePath), which already calls detectVcsType internally; remove the extra filesystem probe by either (A) call detectVcsType(clonePath) once first, store the result in a variable and pass it into getVcsProvider/assign vcsType, or (B) drop the explicit detectVcsType call and reuse the VCS type from the provider (vcsForClone) — update the block around getVcsProvider, vcsForClone and vcsType/defaultBranch so detectVcsType is only invoked once and the cached provider is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts`:
- Line 71: Replace the hardcoded git-specific user strings in workspace-init.ts
with VCS-aware messaging: find occurrences of the literal "Creating git
worktree..." and the error text that suggests running "git fetch origin …" and
change them to choose messages/commands based on the active provider (use the
JjProvider symbol to detect the jj backend). Concretely, when provider
instanceof JjProvider (or provider.type === 'jj'), show a jj-appropriate status
like "Creating worktree..." or include provider.displayName and suggest the
correct fetch command (e.g. "jj git fetch …"); otherwise keep the git wording
and "git fetch origin …". Do this where workspace initialization calls occur
(the create worktree/add workspace flow in this file) so the UI text and
suggested CLI instructions reflect the provider.
---
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts`:
- Around line 1008-1045: The bulk-import loop in importAllWorktrees that
iterates externalWorktrees creates worktree and workspace records but omits
calling vcsForExt.setBaseBranchConfig, so imported worktrees lack the git
base-branch config; update the loop (the block that creates worktree via
localDb.insert(worktrees).values(...).returning().get() and then inserts into
workspaces and calls copySupersetConfigToWorktree) to call
vcsForExt.setBaseBranchConfig(project.mainRepoPath, ext.path, defaultBranch)
(matching the call made in openExternalWorktree) after creating the worktree
record and before/after copySupersetConfigToWorktree so imported worktrees get
the same base-branch configuration as single imports.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts`:
- Line 51: The catch block references the try-scoped variable `vcs` which could
cause a ReferenceError; replace that usage with a direct call to
`getVcsProvider(mainRepoPath)` (the provider cache key) so the catch always
retrieves the cached provider instance instead of relying on the outer-scope
`vcs` variable—update the catch to call `getVcsProvider(mainRepoPath)` where
`vcs` was previously used (e.g., in error handling or cleanup) to ensure a
stable reference.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 759-761: The code calls detectVcsType(clonePath) redundantly after
getVcsProvider(clonePath), which already calls detectVcsType internally; remove
the extra filesystem probe by either (A) call detectVcsType(clonePath) once
first, store the result in a variable and pass it into getVcsProvider/assign
vcsType, or (B) drop the explicit detectVcsType call and reuse the VCS type from
the provider (vcsForClone) — update the block around getVcsProvider, vcsForClone
and vcsType/defaultBranch so detectVcsType is only invoked once and the cached
provider is used.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts`:
- Around line 648-649: The variable naming for VCS providers is inconsistent:
replace scoped decorated names (vcsForOpen, vcsForExt, vcsForImport) with a
single plain identifier vcs inside each procedure to reduce noise; locate calls
that obtain the provider via getVcsProvider and rename the local variable to vcs
and update all subsequent references in that closure (e.g., the workspaceExists
call, vcsForExt.setBaseBranchConfig, and all vcsForImport.* usages) without
changing other scopes or global names, ensuring each procedure (create,
createBranchWorkspace, handleNewWorktree and the other affected closures)
consistently uses vcs.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts`:
- Around line 198-215: The loop variable `branch` in `for (const branch of
commonBranches)` shadows the outer `branch` parameter; rename the loop
identifier (e.g., to `candidateBranch` or `commonBranch`) wherever it's used
inside this block (references to `effectiveBaseBranch`, `checkOriginRefs`,
`vcs.refExistsLocally(mainRepoPath, ...)`, `fallbackOriginRef`, `reason`, and
the returned `fallbackBranch`) so the outer `branch` parameter is not shadowed
and lint `no-shadow` warnings are resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts (1)
463-469:getCurrentBranchonly strips*, not??for conflicted bookmarks.If after a fetch the local and remote bookmarks diverge, the local bookmark will be marked as conflicting and displays in status as
main??. Thebookmarkstemplate output can carry this??suffix; the currentreplace(/\*$/, "")won't strip it, leaking"branchname??"as the returned branch name.♻️ Proposed fix
- return first?.replace(/\*$/, "") || null; + return first?.replace(/[\*\?]+$/, "") || null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts` around lines 463 - 469, The getCurrentBranch logic returns a bookmark name with trailing markers because it only strips '*' but not the conflict '??' suffix; update the cleanup step in getCurrentBranch to remove both a trailing '*' and any trailing '?' characters (e.g., strip /\*$/ and /\?+$/ or a single combined regex) from the first bookmark token (the variable named first derived from trimmed.split) and still return null when empty—ensure you reference and change the replace call that currently uses replace(/\*$/, "") to a regex that also removes the conflict markers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts`:
- Around line 617-626: The hasOriginRemote check currently treats any remote
whose name starts with "origin" as a match; change the loop in hasOriginRemote
to trim each non-empty line, split it on whitespace, take the first token (the
remote name) and compare it for exact equality to "origin" (use ===), returning
true only on an exact match; keep the existing try/catch behavior and references
to jj(mainRepoPath, ["git", "remote", "list"]) and mainRepoPath.
- Around line 527-531: The config key passed to jj is using an invalid TOML
identifier; change the argument in the jj(...) call that currently requests
"revset-aliases.trunk()" to use the quoted key form "revset-aliases.\"trunk()\""
so jj actually reads the trunk() alias; update the call site in the function
that computes the default branch (the getDefaultBranch flow / jj(...)
invocation) to request revset-aliases."trunk()" instead of
revset-aliases.trunk().
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts`:
- Around line 180-185: The branch name is passed unquoted to jj as the -r revset
(in createWorkspaceFromExistingBranch and fetchDefaultBranch), so revset
operators like ~,&,| are misparsed; update the argument construction where
params.branch is used (e.g., the args array around "--name", wsName, "-r",
params.branch in createWorkspaceFromExistingBranch and the equivalent spot in
fetchDefaultBranch) to wrap the branch string in double quotes (e.g., `'"' +
params.branch + '"'`) before pushing into the args array so jj receives the
entire revset as a single quoted argument.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-init.ts`:
- Around line 352-356: The error text for branch-fetch failures still referenced
Git-specific guidance; update the failure message that builds with
effectiveBaseBranch and sanitizedError to use VCS-agnostic wording (e.g.,
"Please check your network connection or try fetching \"${effectiveBaseBranch}\"
manually.") instead of any "git fetch" instruction; locate the string
construction in the function that assembles the error tuple (the code that
returns ["failed", "Cannot fetch branch", ...]) and replace the hard-coded Git
command with the neutral phrasing while preserving sanitizedError interpolation.
- Around line 437-458: The catch block currently calls
getVcsProvider(mainRepoPath) directly which works since getVcsProvider is
synchronous/cached, but for clarity and to avoid any future scoping regressions
hoist the provider by declaring const vcs = getVcsProvider(mainRepoPath) before
the try (or at the top of the surrounding function) and then replace
getVcsProvider(mainRepoPath).removeWorkspace(...) with
vcs.removeWorkspace(mainRepoPath, worktreePath); keep the existing
manager.wasWorktreeCreated(workspaceId), mainRepoPath, worktreePath and
cleanupError handling unchanged.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts`:
- Around line 463-469: The getCurrentBranch logic returns a bookmark name with
trailing markers because it only strips '*' but not the conflict '??' suffix;
update the cleanup step in getCurrentBranch to remove both a trailing '*' and
any trailing '?' characters (e.g., strip /\*$/ and /\?+$/ or a single combined
regex) from the first bookmark token (the variable named first derived from
trimmed.split) and still return null when empty—ensure you reference and change
the replace call that currently uses replace(/\*$/, "") to a regex that also
removes the conflict markers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/docs/content/docs/faq.mdx (1)
39-39: Consider linking to official jj installation documentation instead of single method.
brew install jjcovers Homebrew, which is the dominant path on macOS. However,jjis also available viacargo install jj-cliand pre-built binaries from the GitHub releases page. Since the app is macOS-only today this is a minor nit, but a link to the official install docs would be more future-proof.✏️ Suggested wording
-Yes. Superset auto-detects colocated jj repos and uses native jj workspaces. Requires the `jj` CLI (`brew install jj`). +Yes. Superset auto-detects colocated jj repos and uses native jj workspaces. Requires the `jj` CLI (see [jj installation docs](https://jj-vcs.github.io/jj/latest/install-and-setup/)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/content/docs/faq.mdx` at line 39, Replace the hardcoded "brew install jj" instruction in the sentence that mentions the `jj` CLI with a pointer to the official jj installation docs (link to the project's installation or GitHub releases) and briefly list alternative install methods (e.g., `cargo install jj-cli` and pre-built binaries from releases) so the line about Superset detecting colocated jj repos reads something like: mention the `jj` CLI and link to the official install page while keeping examples of Homebrew, cargo, and pre-built binaries.apps/desktop/AGENTS.md (1)
5-8: Expand the section to cover the non-obvious interaction patterns agents will need.The core rule (
getVcsProviderover direct CLI calls) is clear, but the following patterns are missing and agents are likely to get them wrong without guidance:
- jj CLI fallback —
getVcsProvidersilently returns aGitProviderwhen the jj CLI is unavailable; agents must not assume a jj repo always yields aJjProvider.detectVcsType/getRepoRoot— co-exported helpers agents may call independently before constructing a provider.VcsTypepersistence — creating or mutating a project record requires storing the detectedvcsType; agents that omit it break the schema invariant.📝 Suggested expansion
-Git and Jujutsu (jj) are supported via a `VcsProvider` interface in `src/lib/trpc/routers/workspaces/utils/vcs/`. Use `getVcsProvider(mainRepoPath)` for workspace lifecycle, status, and branch operations — never call `git.ts` functions directly for these. Git-specific utilities not in `VcsProvider` (PR handling, branch naming) are re-exported from `vcs/index.ts`. +Git and Jujutsu (jj) are supported via a `VcsProvider` interface in `src/lib/trpc/routers/workspaces/utils/vcs/`. + +**Entry points (all exported from `vcs/index.ts`):** +- `getVcsProvider(mainRepoPath)` — returns the appropriate provider; **falls back to `GitProvider` with a warning when the `jj` CLI is not installed**. Never assume the returned provider is always `JjProvider` for a jj repo. +- `detectVcsType(repoPath)` — detects the VCS type without constructing a full provider. +- `getRepoRoot(repoPath)` — resolves the canonical repo root. + +**Rules:** +- Use `getVcsProvider` for all workspace lifecycle, status, and branch operations — never invoke VCS CLIs or `git.ts` functions directly. +- Git-specific utilities not in `VcsProvider` (PR handling, branch naming) are re-exported from `vcs/index.ts`. +- When creating or mutating a project record, persist the detected VCS type using the `VcsType` zod type in the `vcs_type` column.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/AGENTS.md` around lines 5 - 8, Expand the VCS Provider Abstraction section to document three non-obvious patterns: explain that getVcsProvider(mainRepoPath) may return a GitProvider as a silent fallback when the jj CLI is not available (do not assume a JJ repo always gives JjProvider), list and describe the co-exported helpers detectVcsType and getRepoRoot and when agents may call them directly, and mandate persisting the detected VcsType (use the VcsType enum) when creating or mutating project records so schema invariants remain intact; reference getVcsProvider, GitProvider, JjProvider, detectVcsType, getRepoRoot, and VcsType in the text so readers can locate the implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docs/content/docs/faq.mdx`:
- Around line 38-40: Update the "Does it support Jujutsu (jj)?" FAQ entry to
document the fallback: state that if the `jj` CLI is not installed Superset will
automatically fall back to using the GitProvider (git worktrees) and will emit a
warning, so users with colocated jj repos may see git used instead; edit the FAQ
paragraph under the "Does it support Jujutsu (jj)?" heading to append one
concise sentence describing this fallback behavior and the presence of a warning
so users aren’t surprised.
In `@apps/docs/content/docs/overview.mdx`:
- Line 20: Replace the old Jujutsu documentation URL in the "Jujutsu (jj)" link
(currently "https://jj-vcs.github.io/jj/") with the new canonical domain
"https://docs.jj-vcs.dev/"; update the markdown link text occurrence "Jujutsu
(jj)" in apps/docs/content/docs/overview.mdx so the link points directly to the
new URL.
In `@apps/docs/content/docs/workspaces.mdx`:
- Line 32: Replace the macOS-only install hint "Requires the `jj` CLI to be
installed (`brew install jj`)" with a platform-agnostic instruction that links
to the official jj installation page; update the text around the phrase so it
directs users to the official install docs (or adds brief notes for
macOS/Linux/Windows) and replace the inline brew command with the canonical
install URL or anchor to the project's installation section to cover all
platforms.
- Line 28: Update the stale documentation link by replacing the URL string
"https://jj-vcs.github.io/jj/" with the canonical
"https://docs.jj-vcs.dev/latest/" in the line that begins "Superset
auto-detects..." and add a one-line caveat immediately after that sentence
stating: "Note: only colocated jj repos (`.jj` + `.git`) are currently
supported." so readers know non-colocated repos fall back to GitProvider.
---
Nitpick comments:
In `@apps/desktop/AGENTS.md`:
- Around line 5-8: Expand the VCS Provider Abstraction section to document three
non-obvious patterns: explain that getVcsProvider(mainRepoPath) may return a
GitProvider as a silent fallback when the jj CLI is not available (do not assume
a JJ repo always gives JjProvider), list and describe the co-exported helpers
detectVcsType and getRepoRoot and when agents may call them directly, and
mandate persisting the detected VcsType (use the VcsType enum) when creating or
mutating project records so schema invariants remain intact; reference
getVcsProvider, GitProvider, JjProvider, detectVcsType, getRepoRoot, and VcsType
in the text so readers can locate the implementations.
In `@apps/docs/content/docs/faq.mdx`:
- Line 39: Replace the hardcoded "brew install jj" instruction in the sentence
that mentions the `jj` CLI with a pointer to the official jj installation docs
(link to the project's installation or GitHub releases) and briefly list
alternative install methods (e.g., `cargo install jj-cli` and pre-built binaries
from releases) so the line about Superset detecting colocated jj repos reads
something like: mention the `jj` CLI and link to the official install page while
keeping examples of Homebrew, cargo, and pre-built binaries.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/changes/branches.ts (2)
127-130: Unsetting the base branch bypasses the VcsProvider abstraction.The set path routes through
vcs.setBaseBranchConfig, but the unset falls back to rawsimpleGit. For colocated jj repos this happens to work (.gitexists), but it breaks the abstraction boundary established in the rest of this PR. Consider addingunsetBaseBranchConfigtoVcsProvider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/branches.ts` around lines 127 - 130, The code currently unsets branch base config by calling simpleGit(...).raw(...) directly which bypasses the VcsProvider abstraction used elsewhere (vcs.setBaseBranchConfig); add an unsetBaseBranchConfig method to the VcsProvider interface and implement it in providers, then replace the direct simpleGit call in the handler (where currentBranch and input.worktreePath are available) with a call to vcs.unsetBaseBranchConfig(currentBranch, input.worktreePath) so all repos use the provider API instead of touching simpleGit directly.
14-29:getMainRepoPathmakes two sequential DB round-trips; a single join would be cleaner.The worktree→project lookup can be collapsed into one query with an inner join, avoiding a second round-trip.
♻️ Proposed refactor
-function getMainRepoPath(worktreePath: string): string { - const wt = localDb - .select() - .from(worktrees) - .where(eq(worktrees.path, worktreePath)) - .get(); - if (!wt) throw new Error(`Worktree not found: ${worktreePath}`); - const project = localDb - .select() - .from(projects) - .where(eq(projects.id, wt.projectId)) - .get(); - if (!project) - throw new Error(`Project not found for worktree: ${worktreePath}`); - return project.mainRepoPath; -} +import { innerJoin } from "drizzle-orm"; + +function getMainRepoPath(worktreePath: string): string { + const row = localDb + .select({ mainRepoPath: projects.mainRepoPath }) + .from(worktrees) + .innerJoin(projects, eq(worktrees.projectId, projects.id)) + .where(eq(worktrees.path, worktreePath)) + .get(); + if (!row) + throw new Error(`Project not found for worktree: ${worktreePath}`); + return row.mainRepoPath; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/branches.ts` around lines 14 - 29, getMainRepoPath currently does two DB round-trips (first selecting worktree, then selecting project); replace them with a single query that joins worktrees to projects and selects project.mainRepoPath in one call (use localDb.select().from(worktrees).innerJoin(projects, eq(projects.id, worktrees.projectId)).where(eq(worktrees.path, worktreePath)).get() or equivalent) and preserve the existing error handling by throwing if the joined result is null/undefined; update references to wt.projectId to use the joined result’s project fields instead.apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts (1)
399-423: Workspace path used aswithRepoLockkey — concurrent operations across workspaces of the same jj repo can race.
hasUncommittedChanges,hasUnpushedCommits, andgetCurrentBranchall passworkspacePathas the implicitlockKey. Two of these running concurrently for different workspaces of the same underlying jj repo each get their own lock, so they can run simultaneously and trigger jj store lock contention. The gracefulcatch → falsemitigates data corruption but can produce incorrect status reads.Consider resolving the main repo path (e.g., via
getRepoRoot) and using it aslockKey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts` around lines 399 - 423, The workspace-level lock key causes races when multiple workspaces share the same underlying jj repo; update hasUncommittedChanges, hasUnpushedCommits (and getCurrentBranch if present) to resolve the repository root first (e.g., call getRepoRoot(workspacePath) or equivalent) and pass that repo root as the lockKey to withRepoLock instead of workspacePath so concurrent operations for different workspaces mapping to the same repo serialize correctly; ensure getRepoRoot returns a stable repo identifier and handle errors before acquiring the lock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts`:
- Around line 698-713: setBaseBranchConfig currently swallows all errors (empty
catch) so callers like branches.ts cannot detect write failures; change the
empty catch to capture the error and propagate it (either rethrow the original
error or throw a new Error with context) so failures are visible to callers.
Specifically, in setBaseBranchConfig (which uses getJjEnv and execFileAsync),
replace catch {} with catch (err) { throw new Error(`Failed to set base branch
config for ${branch} in ${repoPath}: ${err}`) } or simply rethrow err so
branches.ts can handle/report the failure.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/types.ts`:
- Around line 199-213: Update the JSDoc for getBaseBranchConfig and
setBaseBranchConfig to correctly state how the Jj implementation stores the base
branch: change the "Jj: reads/writes from local DB (worktrees.baseBranch)" text
to "Jj: reads/writes git config (branch.<name>.base)" so the docs match the
actual behavior implemented in JjProvider.getBaseBranchConfig and
JjProvider.setBaseBranchConfig which call git -C repoPath config
branch.<name>.base.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts`:
- Around line 627-638: The current hasOriginRemote implementation uses
line.trim().startsWith("origin") which returns true for names like
"origin-backup"; update the check to parse the remote name exactly: after
getting output from jj(mainRepoPath, ["git", "remote", "list"]), for each line
do const name = line.trim().split(/\s+/)[0]; then return true only if name ===
"origin". Keep the rest of the try/catch and return semantics unchanged.
- Around line 536-543: In getDefaultBranch replace the invalid TOML key passed
to jj(...) — change the argument "revset-aliases.trunk()" to use inner quotes so
the config key is revset-aliases."trunk()" (i.e., call jj(mainRepoPath,
["config","get","revset-aliases.\"trunk()\""])); this ensures jj config get
actually reads the trunk() alias instead of failing silently and forcing the
fallback heuristic.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/changes/branches.ts`:
- Around line 127-130: The code currently unsets branch base config by calling
simpleGit(...).raw(...) directly which bypasses the VcsProvider abstraction used
elsewhere (vcs.setBaseBranchConfig); add an unsetBaseBranchConfig method to the
VcsProvider interface and implement it in providers, then replace the direct
simpleGit call in the handler (where currentBranch and input.worktreePath are
available) with a call to vcs.unsetBaseBranchConfig(currentBranch,
input.worktreePath) so all repos use the provider API instead of touching
simpleGit directly.
- Around line 14-29: getMainRepoPath currently does two DB round-trips (first
selecting worktree, then selecting project); replace them with a single query
that joins worktrees to projects and selects project.mainRepoPath in one call
(use localDb.select().from(worktrees).innerJoin(projects, eq(projects.id,
worktrees.projectId)).where(eq(worktrees.path, worktreePath)).get() or
equivalent) and preserve the existing error handling by throwing if the joined
result is null/undefined; update references to wt.projectId to use the joined
result’s project fields instead.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/vcs/jj-provider.ts`:
- Around line 399-423: The workspace-level lock key causes races when multiple
workspaces share the same underlying jj repo; update hasUncommittedChanges,
hasUnpushedCommits (and getCurrentBranch if present) to resolve the repository
root first (e.g., call getRepoRoot(workspacePath) or equivalent) and pass that
repo root as the lockKey to withRepoLock instead of workspacePath so concurrent
operations for different workspaces mapping to the same repo serialize
correctly; ensure getRepoRoot returns a stable repo identifier and handle errors
before acquiring the lock.
andreasasprou
force-pushed
the
feat/jj-vcs-support
branch
from
69a98b2 to
d3f1a1d
Compare
Introduces a VcsProvider interface that abstracts workspace lifecycle, status, branch, and ref operations for both Git and Jujutsu. Detects jj repos via `.jj` directory and routes to JjProvider which uses native jj commands (workspace add/forget, bookmark list/create/forget, etc.). - Phase 1: VcsProvider interface, GitProvider (thin git.ts wrapper), schema migration adding vcsType to projects table - Phase 2: Migrate all callers (workspace-init, create, delete, git-status, teardown, projects, branches) to use getVcsProvider() - Phase 3: JjProvider with native jj CLI commands for colocated repos Colocated repos (.jj + .git) only for now. Changes UI (staging, commit, push) continues using git commands which work in colocated mode.
- Add per-repo mutex to serialize jj CLI operations (prevents store lock contention from concurrent commands) - Add jj CLI availability check — falls back to GitProvider if jj is not installed but .jj directory exists - Fix listExternalWorkspaces/getBranchWorkspacePath to use git worktree list in colocated mode (jj CLI doesn't expose workspace paths) - Fix createWorkspaceFromExistingBranch to set bookmark on @ so getCurrentBranch() returns the expected branch name - Expand gitRefToJjRevset() to handle refs/remotes/ and refs/heads/ - Fix branchExistsOnRemote to handle potential marker characters in bookmark list output - Remove fragile parseWorkspaceList path fabrication
In jj, the working copy often has no bookmark (unlike git where HEAD always points to a branch). Fall back to the project's defaultBranch when getCurrentBranch returns null during project open and workspace creation. Also fix out-of-scope vcs variable in workspace-init error handler.
- Remove unused isJjAvailable export from jj-provider (isJjCliAvailable
in index.ts is the single source of truth)
- Add Windows compatibility for directory cleanup in removeWorkspace
(use cmd /c rmdir on win32, /bin/rm on unix)
- Quote branch names in jj revset expressions to prevent parse errors
with special characters in branch names
- Replace git-specific user-facing strings with VCS-agnostic wording
("Creating workspace..." instead of "Creating git worktree...")
Update overview, workspaces, and FAQ pages to document native jj workspace support including auto-detection, bookmark listing, and colocated mode behavior.
- Overview: mention jj support in "How it Works" - Workspaces: add concise jj section (auto-detection, bookmarks, CLI req) - FAQ: add jj Q&A, rename Git section to "Git & Jujutsu" - AGENTS.md: document VcsProvider abstraction for developers
- branches.ts: use vcs.listBranches() and vcs.getDefaultBranch() instead of simpleGit - jj-provider: workspaceExists() now checks both jj registration and disk presence - jj-provider: repo lock key uses mainRepoPath for workspace-scoped operations - types.ts: import VcsType from @superset/local-db (single source of truth) - migration: backfill null vcs_type to "git" for existing projects
- Fix JSDoc: base branch config uses git config for both git and jj (colocated) - Fix hasOriginRemote false-positive: exact match on remote name, not startsWith - Fix jj config get TOML key: quote trunk() in revset-aliases."trunk()" - Update jj documentation URLs to canonical domain (docs.jj-vcs.dev) - Add colocated-only note and link to jj installation guide
andreasasprou
force-pushed
the
feat/jj-vcs-support
branch
from
d3f1a1d to
022f986
Compare
|
Any movement on this? 👀 |
|
Any movement on this? 👀 I reimplemented it and tried running it locally. You can use it as a reference: https://github.com/NextAlone/superset/commits/feat/jj-vcs-reimpl/ |
|
would love to see this one get merged. jj is gaining in popularity |
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
4 participants
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.
Summary
Adds native Jujutsu (jj) VCS support to the desktop app via a provider abstraction pattern. Users of jj in colocated mode (
.jj+.git) now get nativejj workspaceoperations instead of git worktrees, meaning jj commands work inside workspaces and automatic rebasing is preserved.Motivation: User feedback that Superset creates git worktrees instead of jj workspaces, breaking jj commands and auto-rebasing inside workspaces.
Quick demo
jj-superset.mp4
Changes
Phase 1: VCS Abstraction Layer
vcs/types.ts—VcsProviderinterface (22 methods covering workspace lifecycle, status, branch/bookmark ops, ref checks, repo root, base branch config)vcs/git-provider.ts— Thin delegation to existinggit.tsfunctions (zero behavior changes)vcs/index.ts— Factory + detection:detectVcsType(),getVcsProvider()with caching,getRepoRoot(), jj CLI availability checkvcs_typecolumn to projects table,VcsTypezod typePhase 2: Migrate Callers (pure refactor, identical behavior)
workspace-init.ts— All git.ts imports →getVcsProvider()callscreate.ts— Workspace creation via VcsProviderdelete.ts— Uncommitted/unpushed checks, workspace removal via VcsProvidergit-status.ts— Status refresh, ahead/behind, external workspaces via VcsProviderteardown.ts— Workspace removal via VcsProviderprojects.ts— VCS-aware root detection, storesvcsTypeon projectchanges/branches.ts— Branch listing, current branch, base branch config via VcsProviderPhase 3: JjProvider Implementation
vcs/jj-provider.ts— Full jj implementation with:withRepoLock) to serialize jj CLI calls (prevents store lock contention)jj workspace add/forget/listfor workspace lifecyclejj bookmark list/create/set/forgetfor bookmark operations::@ ~ ::main@origin)gitRefToJjRevset()for transparent ref format conversiongit worktree list --porcelainfor path discovery (jj CLI doesn't expose workspace paths)Key design decisions
.gitstill exists, so Changes UI (staging, commit, push) continues working via git.jjexists but jj CLI isn't installed, falls back to GitProvider with a warningTest plan
vcsTypeset to"jj"in projects tablejj workspace listshows workspace, jj commands work insidejj workspace forgetcalled, directory cleaned upSummary by CodeRabbit
New Features
Documentation
Chores