Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
useCloseAddRepositoryModal,
useResolveNewProjectModal,
} from "renderer/stores/add-repository-modal";
import { GitInitConfirmDialog } from "./components/GitInitConfirmDialog";
import { NewProjectModal } from "./components/NewProjectModal";

export function AddRepositoryModals() {
Expand All @@ -12,16 +13,19 @@ export function AddRepositoryModals() {
const resolveNewProject = useResolveNewProjectModal();

return (
<NewProjectModal
open={active.kind === "new-project"}
onOpenChange={(open) => {
if (!open) close();
}}
onSuccess={(result) => {
toast.success("Project created.");
resolveNewProject({ projectId: result.projectId });
}}
onError={(message) => toast.error(`Create failed: ${message}`)}
/>
<>
<NewProjectModal
open={active.kind === "new-project"}
onOpenChange={(open) => {
if (!open) close();
}}
onSuccess={(result) => {
toast.success("Project created.");
resolveNewProject({ projectId: result.projectId });
}}
onError={(message) => toast.error(`Create failed: ${message}`)}
/>
<GitInitConfirmDialog />
</>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import {
AlertDialog,
AlertDialogContent,
AlertDialogDescription,
AlertDialogFooter,
AlertDialogHeader,
AlertDialogTitle,
} from "@superset/ui/alert-dialog";
import { Button } from "@superset/ui/button";
import { getBaseName } from "renderer/lib/pathBasename";
import { useGitInitConfirmStore } from "renderer/stores/git-init-confirm";

/**
* Confirms initializing git in a folder the user picked to import that isn't a
* git repo yet. Driven imperatively by `useGitInitConfirmStore.request()` from
* the folder-first import flow; mounted once via AddRepositoryModals.
*/
export function GitInitConfirmDialog() {
const isOpen = useGitInitConfirmStore((s) => s.isOpen);
const repoPath = useGitInitConfirmStore((s) => s.repoPath);
const resolve = useGitInitConfirmStore((s) => s.resolve);

const folderName = repoPath ? getBaseName(repoPath) : "this folder";

return (
<AlertDialog
open={isOpen}
onOpenChange={(open) => {
if (!open) resolve(false);
}}
>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle>Initialize git repository?</AlertDialogTitle>
<AlertDialogDescription asChild>
<p>
<span className="font-medium text-foreground select-text cursor-text">
{folderName}
</span>{" "}
isn't a git repository yet. Initialize git here and import it?
</p>
</AlertDialogDescription>
</AlertDialogHeader>
<AlertDialogFooter>
<Button variant="outline" onClick={() => resolve(false)}>
Cancel
</Button>
<Button onClick={() => resolve(true)}>Initialize &amp; import</Button>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { GitInitConfirmDialog } from "./GitInitConfirmDialog";
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,24 @@ const selectDirectoryMock = mock(async () => ({
canceled: false,
path: repoPath,
}));
const findByPathMock = mock(async () => ({
candidates: [] as { id: string; name: string }[],
cloudErrors: [] as (typeof cloudError)[],
}));
const findByPathMock = mock(
async (): Promise<{
candidates: { id: string; name: string }[];
cloudErrors: (typeof cloudError)[];
needsGitInit?: boolean;
}> => ({
candidates: [],
cloudErrors: [],
}),
);
const setupMock = mock(async () => setupResult);
const createMock = mock(async () => ({
projectId: "created-project",
repoPath,
mainWorkspaceId: "workspace-created",
}));
const finalizeSetupMock = mock(() => undefined);
const requestGitInitMock = mock(async () => false);

mock.module("react", () => ({
useCallback: <T extends (...args: never[]) => unknown>(callback: T) =>
Expand Down Expand Up @@ -63,6 +70,10 @@ mock.module(
}),
);

mock.module("renderer/stores/git-init-confirm", () => ({
useRequestGitInitConfirm: () => requestGitInitMock,
}));

const { useFolderFirstImport } = await import("./useFolderFirstImport");

describe("useFolderFirstImport", () => {
Expand All @@ -73,10 +84,12 @@ describe("useFolderFirstImport", () => {
setupMock,
createMock,
finalizeSetupMock,
requestGitInitMock,
]) {
fn.mockClear();
}
findByPathMock.mockResolvedValue({ candidates: [], cloudErrors: [] });
requestGitInitMock.mockResolvedValue(false);
});

it("reports cloud lookup errors instead of creating a duplicate local import when no candidates exist", async () => {
Expand All @@ -97,4 +110,51 @@ describe("useFolderFirstImport", () => {
expect(setupMock).not.toHaveBeenCalled();
expect(finalizeSetupMock).not.toHaveBeenCalled();
});

it("imports with init after the user confirms a non-git folder", async () => {
findByPathMock.mockResolvedValue({
candidates: [],
cloudErrors: [],
needsGitInit: true,
});
requestGitInitMock.mockResolvedValue(true);
const onError = mock(() => undefined);

const result = await useFolderFirstImport({ onError }).start();

expect(requestGitInitMock).toHaveBeenCalledWith(repoPath);
expect(createMock).toHaveBeenCalledWith({
name: "octocat",
mode: { kind: "importLocal", repoPath, initIfNeeded: true },
});
expect(finalizeSetupMock).toHaveBeenCalledWith(hostUrl, {
projectId: "created-project",
repoPath,
mainWorkspaceId: "workspace-created",
});
expect(result).toEqual({
projectId: "created-project",
repoPath,
mainWorkspaceId: "workspace-created",
});
expect(onError).not.toHaveBeenCalled();
});

it("does nothing when the user cancels the git-init confirmation", async () => {
findByPathMock.mockResolvedValue({
candidates: [],
cloudErrors: [],
needsGitInit: true,
});
requestGitInitMock.mockResolvedValue(false);
const onError = mock(() => undefined);

const result = await useFolderFirstImport({ onError }).start();

expect(result).toBeNull();
expect(requestGitInitMock).toHaveBeenCalledWith(repoPath);
expect(createMock).not.toHaveBeenCalled();
expect(finalizeSetupMock).not.toHaveBeenCalled();
expect(onError).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
useFinalizeProjectSetup,
} from "renderer/react-query/projects";
import { useLocalHostService } from "renderer/routes/_authenticated/providers/LocalHostServiceProvider";
import { useRequestGitInitConfirm } from "renderer/stores/git-init-confirm";

export interface UseFolderFirstImportResult {
start: () => Promise<ProjectSetupResult | null>;
Expand All @@ -26,6 +27,7 @@ export function useFolderFirstImport(options?: {
const { activeHostUrl } = hostService;
const finalizeSetup = useFinalizeProjectSetup();
const selectDirectory = electronTrpc.window.selectDirectory.useMutation();
const requestGitInit = useRequestGitInitConfirm();
const { onError, onMultipleProjects } = options ?? {};

const start = useCallback(async (): Promise<ProjectSetupResult | null> => {
Expand Down Expand Up @@ -54,6 +56,20 @@ export function useFolderFirstImport(options?: {
let candidates: MatchingProject[];
try {
const response = await client.project.findByPath.query({ repoPath });

// Folder isn't a git repo yet: offer to `git init` it, then import
// via the create path with init enabled.
if ("needsGitInit" in response && response.needsGitInit) {
const confirmed = await requestGitInit(repoPath);
if (!confirmed) return null;
const result = await client.project.create.mutate({
name: getBaseName(repoPath),
mode: { kind: "importLocal", repoPath, initIfNeeded: true },
});
finalizeSetup(activeHostUrl, result);
return result;
}

candidates = response.candidates;
if (candidates.length === 0 && response.cloudErrors.length > 0) {
const first = response.cloudErrors[0];
Expand Down Expand Up @@ -107,6 +123,7 @@ export function useFolderFirstImport(options?: {
hostService,
onError,
onMultipleProjects,
requestGitInit,
selectDirectory,
]);

Expand Down
47 changes: 47 additions & 0 deletions apps/desktop/src/renderer/stores/git-init-confirm.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { create } from "zustand";
import { devtools } from "zustand/middleware";

interface GitInitConfirmState {
isOpen: boolean;
repoPath: string | null;
/**
* Opens the confirm dialog and resolves `true` if the user agrees to
* `git init` the folder, `false` if they cancel/dismiss. Only one request
* can be in flight at a time — a second call resolves the prior request to
* `false` before opening fresh. Safe today because there is a single global
* dialog instance (rendered by AddRepositoryModals).
*/
request: (repoPath: string) => Promise<boolean>;
resolve: (confirmed: boolean) => void;
}

// Module-level resolver so the pending promise isn't stored in zustand state.
// The store drives the dialog's open/close UI; the resolver bridges the
// imperative request() call back to its caller.
let pendingResolve: ((confirmed: boolean) => void) | null = null;

export const useGitInitConfirmStore = create<GitInitConfirmState>()(
devtools(
(set) => ({
isOpen: false,
repoPath: null,
request: (repoPath) => {
pendingResolve?.(false);
return new Promise<boolean>((resolve) => {
pendingResolve = resolve;
set({ isOpen: true, repoPath });
});
},
resolve: (confirmed) => {
const resolve = pendingResolve;
pendingResolve = null;
set({ isOpen: false, repoPath: null });
resolve?.(confirmed);
},
}),
{ name: "git-init-confirm" },
),
);

export const useRequestGitInitConfirm = () =>
useGitInitConfirmStore((state) => state.request);
23 changes: 21 additions & 2 deletions packages/host-service/src/trpc/router/project/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ import {
cloneRepoInto,
cloneTemplateInto,
initEmptyRepo,
initLocalRepoInPlace,
type ResolvedRepo,
resolveLocalRepo,
tryRevParseGitRoot,
} from "./utils/resolve-repo";
import { templateUrlFor } from "./utils/templates";

Expand Down Expand Up @@ -196,11 +198,28 @@ export async function createFromClone(
});
}

/**
* Resolve an existing repo, or — when `initIfNeeded` and the folder isn't a git
* repo yet — `git init` it in place first. The init branch only runs after the
* UI has confirmed intent with the user.
*/
async function resolveOrInitLocalRepo(
repoPath: string,
initIfNeeded: boolean,
): Promise<ResolvedRepo> {
if (!initIfNeeded) return resolveLocalRepo(repoPath);
const root = await tryRevParseGitRoot(repoPath);
return root ? resolveLocalRepo(root) : initLocalRepoInPlace(repoPath);
}
Comment on lines +206 to +213

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

initIfNeeded now lets file paths slip through as repo imports.

Before this change, createFromImportLocal always went through resolveLocalRepo(repoPath), which rejected non-directories. With the new probe-first flow, passing /repo/src/file.ts plus initIfNeeded: true resolves /repo and imports it successfully. Please validate the original repoPath before tryRevParseGitRoot so the mutation stays folder-only.

Also applies to: 219-222

🤖 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 `@packages/host-service/src/trpc/router/project/handlers.ts` around lines 206 -
213, The change allows file paths to be converted into repos because
resolveOrInitLocalRepo calls tryRevParseGitRoot before validating the original
repoPath; fix resolveOrInitLocalRepo (and the analogous call at the other site)
by first verifying repoPath is a directory (e.g., fs.stat/exists + isDirectory)
and, if not a directory, immediately call resolveLocalRepo(repoPath) so
non-directory paths are rejected as before; only if repoPath is a directory and
initIfNeeded is true should you call tryRevParseGitRoot and fall back to
initLocalRepoInPlace(repoPath).


export async function createFromImportLocal(
ctx: HostServiceContext,
args: { name: string; repoPath: string },
args: { name: string; repoPath: string; initIfNeeded?: boolean },
): Promise<CreateResult> {
const resolved = await resolveLocalRepo(args.repoPath);
const resolved = await resolveOrInitLocalRepo(
args.repoPath,
args.initIfNeeded ?? false,
);
return persistFromResolved(ctx, {
name: args.name,
resolved,
Expand Down
24 changes: 23 additions & 1 deletion packages/host-service/src/trpc/router/project/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import {
type ResolvedRepo,
resolveLocalRepo,
resolveMatchingSlug,
tryRevParseGitRoot,
validateDirectoryPath,
} from "./utils/resolve-repo";

export const projectRouter = router({
Expand Down Expand Up @@ -162,7 +164,22 @@ export const projectRouter = router({
}),
)
.query(async ({ ctx, input }) => {
const resolved = await resolveLocalRepo(input.repoPath);
// Detect "folder isn't a git repo yet" without throwing, so the import
// UI can offer to `git init` it (create importLocal + initIfNeeded)
// instead of dead-ending on a BAD_REQUEST. Additive optional field —
// repo paths never carry needsGitInit, so existing callers are
// unaffected.
const root = await tryRevParseGitRoot(input.repoPath);
if (root === null) {
validateDirectoryPath(input.repoPath, "Path"); // 400 on missing / not-a-dir
return {
candidates: [],
cloudErrors: [] as { url: string; message: string }[],
needsGitInit: true as const,
};
}

const resolved = await resolveLocalRepo(root);
Comment on lines +172 to +182

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate input.repoPath before probing Git.

git rev-parse --show-toplevel succeeds for file paths inside a worktree, so this branch now accepts something like /repo/src/index.ts and resolves the containing repo instead of rejecting it as "Path is not a directory". That changes the import contract from "pick a folder" to "pick anything inside a repo" and can import the wrong project from an accidental file selection.

🤖 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 `@packages/host-service/src/trpc/router/project/project.ts` around lines 172 -
182, The code calls tryRevParseGitRoot(input.repoPath) before validating that
input.repoPath is a directory, which lets file paths inside a repo be accepted;
move or add the directory validation so the path is confirmed a directory before
probing Git. Specifically, invoke validateDirectoryPath(input.repoPath, "Path")
(or a direct fs.stat check for directory) prior to calling tryRevParseGitRoot,
and only call tryRevParseGitRoot/resolveLocalRepo if the path is a directory;
preserve the existing return shape (candidates/cloudErrors/needsGitInit) when
validation fails.

const gitRoot = resolved.repoPath;

const expectedParsed =
Expand Down Expand Up @@ -391,6 +408,10 @@ export const projectRouter = router({
z.object({
kind: z.literal("importLocal"),
repoPath: z.string().min(1),
// When set, `git init` a non-git folder in place before
// importing. The UI sets this only after confirming intent
// with the user (see findByPath's needsGitInit).
initIfNeeded: z.boolean().optional().default(false),
}),
z.object({
kind: z.literal("template"),
Expand Down Expand Up @@ -423,6 +444,7 @@ export const projectRouter = router({
return createFromImportLocal(ctx, {
name: input.name,
repoPath: input.mode.repoPath,
initIfNeeded: input.mode.initIfNeeded,
});
}
}),
Expand Down
Loading
Loading