[codex] Allow duplicate v2 repo clone URLs#3723
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRemoved repo_clone_url-based deduplication: the DB unique index was dropped, remote conflict lookups were disabled, migration docs and messages updated to rely solely on per-org Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HostService
participant CloudAPI
Note over HostService,CloudAPI: Old flow (pre-change)
Client->>HostService: findBackfillConflict(request)
HostService->>CloudAPI: v2Project.get / findByGitHubRemote(remote)
CloudAPI-->>HostService: conflict {id, name} or null
HostService-->>Client: { conflict: {...} } or { conflict: null }
Note over HostService,CloudAPI: New flow (post-change)
Client->>HostService: findBackfillConflict(request)
HostService-->>Client: { conflict: null }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR relaxes the
Confidence Score: 4/5Not safe to merge until the corresponding Drizzle migration to drop v2_projects_org_repo_clone_url_unique is committed. All application-level code changes are clean and well-reasoned, but the schema change is incomplete — the unique index still exists in the DB and no migration SQL file has been added to remove it. This P1 blocker must be resolved before the feature can actually work. packages/db/src/schema/schema.ts — requires a companion migration (e.g. 0037_drop_v2_projects_org_repo_clone_url_unique.sql) to be generated and committed.
|
| Filename | Overview |
|---|---|
| packages/db/src/schema/schema.ts | Removes the uniqueIndex on (organizationId, lower(repoCloneUrl)), but no corresponding Drizzle migration SQL file is committed — the live DB constraint still exists until migration 0037+ is generated and applied. |
| packages/host-service/src/trpc/router/project/project.ts | findBackfillConflict now always returns { conflict: null } as a stub for backwards-compatible clients; logic and types are correct, unused parameters appropriately removed. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.ts | Error message updated to reflect that multiple-candidate results are now expected; user is directed to settings instead of treating this as an invariant violation. |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts | Warning log updated to clarify 'migration has no project picker, linking to first' — accurately reflects expected multi-candidate behaviour post-schema change. |
| apps/desktop/plans/20260422-2100-v1-to-v2-port.md | Documentation updated to remove the repo_clone_url uniqueness guarantee and the corresponding UNIQUE_VIOLATION handling step from the migration happy-path. |
Sequence Diagram
sequenceDiagram
participant User
participant useFolderFirstImport
participant HostService as host-service (findByPath)
participant CloudAPI as cloud API (findByGitHubRemote)
participant DB as v2_projects (DB)
User->>useFolderFirstImport: pick local folder
useFolderFirstImport->>HostService: project.findByPath(repoPath)
HostService->>CloudAPI: v2Project.findByGitHubRemote(repoCloneUrl)
CloudAPI->>DB: SELECT WHERE org + lower(url)
DB-->>CloudAPI: 0, 1, or N candidates
CloudAPI-->>HostService: { candidates }
HostService-->>useFolderFirstImport: { candidates }
alt 0 candidates
useFolderFirstImport->>HostService: project.create(importLocal)
HostService->>DB: INSERT (slug unique enforced)
DB-->>HostService: success
HostService-->>useFolderFirstImport: { projectId }
useFolderFirstImport-->>User: success
else 1 candidate
useFolderFirstImport->>HostService: project.setup(import, repoPath)
HostService-->>useFolderFirstImport: { repoPath }
useFolderFirstImport-->>User: success
else N candidates (now expected)
useFolderFirstImport-->>User: error — Multiple projects use this repo, open from settings
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/db/src/schema/schema.ts
Line: 404-407
Comment:
**Missing DB migration file for dropped index**
The unique index `v2_projects_org_repo_clone_url_unique` was created in migration `0034_v2_projects_decouple_github_install_add_clone_url.sql` with:
```sql
CREATE UNIQUE INDEX "v2_projects_org_repo_clone_url_unique" ON "v2_projects" USING btree ("organization_id",lower("repo_clone_url"));
```
Removing it from `schema.ts` has no effect on the running database until a corresponding DROP INDEX migration is generated and committed. The latest migration in the repo is `0036` and contains no such drop. Until `drizzle-kit generate` produces a `0037_...sql` that drops this index and it is merged, any attempt to create a second project with the same `repo_clone_url` in the same org will still fail with a DB-level `UNIQUE_VIOLATION` — precisely the case this PR is meant to allow.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Allow duplicate v2 repo clone URLs" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
404
to
407
| (table) => [ | ||
| index("v2_projects_organization_id_idx").on(table.organizationId), | ||
| unique("v2_projects_org_slug_unique").on(table.organizationId, table.slug), | ||
| // One project per repo URL per org. NULLs don't collide (PG default) | ||
| // so empty-mode projects without a remote can still be created. | ||
| uniqueIndex("v2_projects_org_repo_clone_url_unique").on( | ||
| table.organizationId, | ||
| sql`lower(${table.repoCloneUrl})`, | ||
| ), | ||
| ], |
There was a problem hiding this comment.
The unique index v2_projects_org_repo_clone_url_unique was created in migration 0034_v2_projects_decouple_github_install_add_clone_url.sql with:
CREATE UNIQUE INDEX "v2_projects_org_repo_clone_url_unique" ON "v2_projects" USING btree ("organization_id",lower("repo_clone_url"));Removing it from schema.ts has no effect on the running database until a corresponding DROP INDEX migration is generated and committed. The latest migration in the repo is 0036 and contains no such drop. Until drizzle-kit generate produces a 0037_...sql that drops this index and it is merged, any attempt to create a second project with the same repo_clone_url in the same org will still fail with a DB-level UNIQUE_VIOLATION — precisely the case this PR is meant to allow.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/db/src/schema/schema.ts
Line: 404-407
Comment:
**Missing DB migration file for dropped index**
The unique index `v2_projects_org_repo_clone_url_unique` was created in migration `0034_v2_projects_decouple_github_install_add_clone_url.sql` with:
```sql
CREATE UNIQUE INDEX "v2_projects_org_repo_clone_url_unique" ON "v2_projects" USING btree ("organization_id",lower("repo_clone_url"));
```
Removing it from `schema.ts` has no effect on the running database until a corresponding DROP INDEX migration is generated and committed. The latest migration in the repo is `0036` and contains no such drop. Until `drizzle-kit generate` produces a `0037_...sql` that drops this index and it is merged, any attempt to create a second project with the same `repo_clone_url` in the same org will still fail with a DB-level `UNIQUE_VIOLATION` — precisely the case this PR is meant to allow.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/plans/20260422-2100-v1-to-v2-port.md (1)
91-120:⚠️ Potential issue | 🟡 MinorUpdate the "Happy-path flow" pseudo-code and race trickiness to match the new non-unique-URL model.
The header text on lines 83-89 was updated, but the rest of the section still describes the old uniqueness-on-
repo_clone_urlmodel:
- L97-98:
existing = v2Project.findByGitHubRemote(...)followed byif existing:reads as a single match. With multiple v2 projects allowed per clone URL,findByGitHubRemotenow returns 0..N candidates and the migration explicitly links to the first (seemigrate.tsL139-143). Pseudo-code should reflect that.- L118 ("Create-after-lookup race"): describes one writer "hits the unique index" on
repo_clone_url, but that index is being dropped in this PR. The remaining race is on(organization_id, slug)only, which is already covered by item 5 ("Slug uglification"). This trickiness item is now obsolete or should be rewritten to scope the race to slugs.Optional but recommended for keeping the plan doc trustworthy as the migration's source-of-truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260422-2100-v1-to-v2-port.md` around lines 91 - 120, Update the "Happy-path flow" pseudo-code to reflect that findByGitHubRemote now returns 0..N candidates (not a single match): call findByGitHubRemote, treat the result as a list, link to the first candidate if any (matching the implementation in migrate.ts where the first returned project is linked), and only create a new v2Project when the list is empty. Also revise the "Create-after-lookup race" trickiness to remove references to a unique index on repo_clone_url and instead describe the remaining race on the (organization_id, slug) uniqueness (i.e., concurrent creates may conflict on slug and must be handled by retry/ugly-slug logic already described).
🧹 Nitpick comments (1)
packages/host-service/src/trpc/router/project/project.ts (1)
41-53: Add a@deprecatedJSDoc tag to signal the endpoint's no-op state to IDE tooling and callers.The endpoint unconditionally returns
{ conflict: null }, and active callers inProjectLocationSection.tsxstill depend on this behavior. A deprecation marker will surface this in IDEs and help coordinate eventual removal alongside migration of the older settings screens.♻️ Suggested annotation
- findBackfillConflict: protectedProcedure + /** + * `@deprecated` Multiple v2 projects may share a `repo_clone_url`, so a + * matching URL is no longer a backfill/relocate conflict. This endpoint + * is retained as a no-op for older settings screens; remove once those + * callers are migrated. + */ + findBackfillConflict: protectedProcedure .input( z.object({ projectId: z.string().uuid(), repoPath: z.string().min(1), }), ) .query(() => { - // Multiple v2 projects may point at the same GitHub URL, so a matching - // repo URL is no longer a conflict. Kept for backwards-compatible - // clients while older settings screens still call the endpoint. return { conflict: null }; }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/project/project.ts` around lines 41 - 53, Add a JSDoc `@deprecated` annotation above the findBackfillConflict protectedProcedure to signal it is a no-op; update the comment into a deprecation message (e.g., "@deprecated This endpoint unconditionally returns { conflict: null } and is kept for backward compatibility with ProjectLocationSection.tsx; remove after migrating older settings screens."). Keep the existing implementation and return value unchanged, but ensure the JSDoc appears immediately above the findBackfillConflict declaration so IDEs and callers surface the deprecation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/plans/20260422-2100-v1-to-v2-port.md`:
- Around line 91-120: Update the "Happy-path flow" pseudo-code to reflect that
findByGitHubRemote now returns 0..N candidates (not a single match): call
findByGitHubRemote, treat the result as a list, link to the first candidate if
any (matching the implementation in migrate.ts where the first returned project
is linked), and only create a new v2Project when the list is empty. Also revise
the "Create-after-lookup race" trickiness to remove references to a unique index
on repo_clone_url and instead describe the remaining race on the
(organization_id, slug) uniqueness (i.e., concurrent creates may conflict on
slug and must be handled by retry/ugly-slug logic already described).
---
Nitpick comments:
In `@packages/host-service/src/trpc/router/project/project.ts`:
- Around line 41-53: Add a JSDoc `@deprecated` annotation above the
findBackfillConflict protectedProcedure to signal it is a no-op; update the
comment into a deprecation message (e.g., "@deprecated This endpoint
unconditionally returns { conflict: null } and is kept for backward
compatibility with ProjectLocationSection.tsx; remove after migrating older
settings screens."). Keep the existing implementation and return value
unchanged, but ensure the JSDoc appears immediately above the
findBackfillConflict declaration so IDEs and callers surface the deprecation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52521189-fe3a-4e96-9774-42ab0c832fef
📒 Files selected for processing (5)
apps/desktop/plans/20260422-2100-v1-to-v2-port.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.tspackages/db/src/schema/schema.tspackages/host-service/src/trpc/router/project/project.ts
💤 Files with no reviewable changes (1)
- packages/db/src/schema/schema.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx (1)
49-57: Optional: surface candidate project names in the toast.The handler receives full
MatchingProject[](withname), but onlycandidates.lengthis shown. Including a couple of names (e.g. the first 2–3 truncated) would help the user identify which existing projects are reusing this repo before they jump into Settings.♻️ Example refactor
- onMultipleProjects: ({ candidates }) => { - toast.error("Import failed", { - description: `Multiple projects use this repository (${candidates.length}). Choose the project in settings to set it up on this device.`, - action: { - label: "Open Projects", - onClick: () => navigate({ to: "/settings/projects" }), - }, - }); - }, + onMultipleProjects: ({ candidates }) => { + const preview = candidates + .slice(0, 3) + .map((c) => c.name) + .join(", "); + const suffix = candidates.length > 3 ? ", …" : ""; + toast.error("Import failed", { + description: `Multiple projects use this repository (${candidates.length}): ${preview}${suffix}. Choose the project in settings to set it up on this device.`, + action: { + label: "Open Projects", + onClick: () => navigate({ to: "/settings/projects" }), + }, + }); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx` around lines 49 - 57, The onMultipleProjects handler currently logs only the candidate count; change the toast description to include the first 2–3 project names from the candidates array (each truncated to a reasonable length, e.g., 30 chars with ellipses) alongside the count so users can see which projects conflict; in the DashboardSidebarHeader function extract names via candidates.slice(0,3).map(p => p.name) and join them (add "and X more" if candidates.length>3), then pass that composed string into toast.error's description while keeping the existing action (Open Projects) and navigate behavior.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.ts (1)
70-80: Fallback error path duplicates header copy — consider centralizing.The default-fallback message here ("Multiple projects use this repository (N). Open the project you want from settings…") is nearly identical to the toast description in
DashboardSidebarHeader.tsx. If this string ever changes (count formatting, project-name preview, settings route name), both call sites will drift. Consider exporting a small helper (e.g.formatMultipleProjectsMessage(candidates)) co-located with this hook so the header can reuse it.Not a blocker for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.ts` around lines 70 - 80, The fallback error message in useFolderFirstImport (inside the block that handles multiple candidates where const [only, ...rest] = candidates) duplicates the toast text used in DashboardSidebarHeader; extract the text generation into a shared helper like formatMultipleProjectsMessage(candidates) co-located with the useFolderFirstImport hook and replace the inline string with a call to that helper; ensure both useFolderFirstImport (the onMultipleProjects/reportError branch) and DashboardSidebarHeader import and use formatMultipleProjectsMessage so the count/preview/route wording stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.ts`:
- Around line 70-80: The fallback error message in useFolderFirstImport (inside
the block that handles multiple candidates where const [only, ...rest] =
candidates) duplicates the toast text used in DashboardSidebarHeader; extract
the text generation into a shared helper like
formatMultipleProjectsMessage(candidates) co-located with the
useFolderFirstImport hook and replace the inline string with a call to that
helper; ensure both useFolderFirstImport (the onMultipleProjects/reportError
branch) and DashboardSidebarHeader import and use formatMultipleProjectsMessage
so the count/preview/route wording stays consistent.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx`:
- Around line 49-57: The onMultipleProjects handler currently logs only the
candidate count; change the toast description to include the first 2–3 project
names from the candidates array (each truncated to a reasonable length, e.g., 30
chars with ellipses) alongside the count so users can see which projects
conflict; in the DashboardSidebarHeader function extract names via
candidates.slice(0,3).map(p => p.name) and join them (add "and X more" if
candidates.length>3), then pass that composed string into toast.error's
description while keeping the existing action (Open Projects) and navigate
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9319267-e14d-4be8-89b2-86fe0a1fea4d
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx
7 tasks
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
1 participant
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
repo_clone_urlso multiple projects can reference the same GitHub repository.Validation
bunx biome check packages/db/src/schema/schema.ts packages/host-service/src/trpc/router/project/project.ts apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport/useFolderFirstImport.ts apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/migrate.ts apps/desktop/plans/20260422-2100-v1-to-v2-port.md packages/db/drizzle/0037_drop_v2_project_repo_clone_url_unique.sql packages/db/drizzle/meta/_journal.json packages/db/drizzle/meta/0037_snapshot.jsonbun run --cwd packages/db typecheckbun run --cwd packages/host-service typecheckSummary by CodeRabbit
Bug Fixes
Changes