Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
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:
📝 WalkthroughWalkthroughThe PR centralises and simplifies git URL normalization and tightens downstream handling. shared/utils/git-providers.ts adds a single-pass normalizeGitUrl that trims input, removes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 668114a4-b18a-44e2-b608-0195545392e1
📒 Files selected for processing (2)
app/composables/useRepositoryUrl.tsshared/utils/git-providers.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shared/utils/git-providers.ts (1)
301-303:⚠️ Potential issue | 🔴 CriticalPreserve explicit SSH ports; current rewrite breaks valid remotes.
Line 301 rewrites
host:portintohost/port, so URLs likessh://git@gitlab.example.com:2222/group/repo.gitare malformed and parsed incorrectly.Proposed fix
export function normalizeGitUrl(input: string): string | null { let url = input .trim() .replace(/^git\+/, '') .replace(/\.git$/, '') - .replace(/(^|\/)[^/]+?@/, '$1') // remove "user@" from "ssh://user@host.com:..." - .replace(/(\.[^./]+?):/, '$1/') // change ".com:" to ".com/" from "ssh://user@host.com:..." + if (!url) return null + + // SCP-style: git@host:owner/repo -> https://host/owner/repo + if (/^[^@/\s]+@[^:/\s]+:.+/.test(url)) { + url = url.replace(/^[^@/\s]+@([^:/\s]+):/, 'https://$1/') + } else { + // URL-style: remove optional userinfo only + url = url.replace(/(^|\/\/)[^/@]+@/, '$1') + } + + // Only rewrite host:path when no scheme exists (avoid touching :port) + if (!url.includes('://')) { + url = url.replace(/(\.[^./]+?):/, '$1/') + } + + url = url .replace(/^git:\/\//, 'https://') .replace(/^ssh:\/\//, 'https://') url = url.includes('://') ? url : `https://${url}` return url || null }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f46b42b-2208-4bfc-a7a2-009fc7c75efc
📒 Files selected for processing (1)
shared/utils/git-providers.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Quite a lot of PRs for this issue indeed 🤣 What do you think about adding a single test to handle the .git problem from the issue? Just to be sure that this doesn't change in the future. |
|
If we want to add a test, I can copy over the one I have from publint so it's tested altogether. It'll probably be more robust that way if the function is edited again. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unit/shared/utils/git-providers.spec.ts (2)
8-73: Good test coverage for the normalisation logic.The test suite comprehensively covers the URL normalisation scenarios including the
.gitsuffix removal that addresses issue#2097. The use ofexpect.soft()for multiple assertions per test is a good practice.Consider adding an edge-case test for empty/invalid input. According to the implementation summary,
normalizeGitUrlreturnsnullfor empty results. A test verifying this behaviour would prevent regressions:Suggested test addition
describe('normalizeGitUrl', () => { + it('should return null for empty or whitespace-only input', () => { + expect.soft(normalizeGitUrl('')).toBeNull() + expect.soft(normalizeGitUrl(' ')).toBeNull() + }) + it('should leave plain HTTPS URLs unchanged', () => {
55-63: Clarify test description for the colon-preservation edge case.Lines 60–62 test that a colon is preserved when a forward slash already exists after the host — this is crucial for Radicle URLs like
rad:z3nP4yT1PE3m1PxLEzr173sZtJVnT. However, the surroundingitblock description ("should convert git@github.com SSH format") doesn't clearly convey this intent.Consider splitting this into a separate test or adjusting the description to make the colon-preservation behaviour explicit:
Suggested improvement
it('should convert git@github.com SSH format', () => { expect.soft(normalizeGitUrl('git@github.com:user/repo')).toBe('https://github.com/user/repo') expect .soft(normalizeGitUrl('git@github.com:user/repo.git')) .toBe('https://github.com/user/repo') - expect - .soft(normalizeGitUrl('git@github.com/user/repo:123')) - .toBe('https://github.com/user/repo:123') + }) + + it('should preserve colon when path already contains forward slash (Radicle URLs)', () => { + expect + .soft(normalizeGitUrl('git@github.com/user/repo:123')) + .toBe('https://github.com/user/repo:123') })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00b601cc-0fcc-4e3e-bcc4-048117079e9b
📒 Files selected for processing (1)
test/unit/shared/utils/git-providers.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/shared/utils/git-providers.spec.ts (1)
9-77: Please add explicit regressions for whitespace-only input and Radiclerad:IDs.Line 9 only asserts
''. Adding these two cases will better lock in the trim behaviour and the Radicle colon handling that this PR targets.Proposed test additions
describe('normalizeGitUrl', () => { it('should return null for empty input', () => { expect.soft(normalizeGitUrl('')).toBeNull() + expect.soft(normalizeGitUrl(' \n\t ')).toBeNull() }) + it('should keep Radicle repository IDs with colon unchanged', () => { + expect + .soft( + normalizeGitUrl( + 'https://app.radicle.at/nodes/seed.radicle.at/rad:z3nP4yT1PE3m1PxLEzr173sZtJVnT', + ), + ) + .toBe( + 'https://app.radicle.at/nodes/seed.radicle.at/rad:z3nP4yT1PE3m1PxLEzr173sZtJVnT', + ) + }) + it('should leave plain HTTPS URLs unchanged', () => {As per coding guidelines "
**/*.{test,spec}.{ts,tsx}: Write unit tests for core functionality usingvitest."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ade363a-3de8-4aee-a0be-2ef6b0ea7e08
📒 Files selected for processing (1)
test/unit/shared/utils/git-providers.spec.ts
2 participants
🔗 Linked issue
fix #2097
🧭 Context
Ok so, that linked issue already has two PRs, I'm submitting another 😅 This PR uses a function that I've used in publint (and tested), and also copied to vite, which has been working well, so it might be useful to reuse the same code here.
EDIT: Looks like the code didn't handle radicle urls properly (
https://app.radicle.at/nodes/seed.radicle.at/rad:z3nP4yT1PE3m1PxLEzr173sZtJVnT, the colon) but I've updated to handle it, downside of copying snippets I guess 😬📚 Description
This PR introduces a simpler git url normalization code and handles the trailing
.gitbug.