Conversation
📊 Benchmark resultsComparing with 2453221
|
src/utils/init/config-manual.ts
Outdated
| provider: 'manual', | ||
| repo_path: repoPath, | ||
| provider: repoData.provider ?? 'manual', | ||
| repo_path: repoData.repo ?? repoPath, |
There was a problem hiding this comment.
[small bug] I think this is dead code- we'll be prompting users for an SSH that never gets send via network request ie repoData.repo ("owner/name")
Should we do something like so only the manual case will prompt the user
const repoPath = repoData.repo ?? (await getRepoPath({ repoData }))
repo_path: repoPath,
* ci: create integration test sites in testing account This [mechanism already exists](https://github.com/netlify/cli/blob/b80b98f85929803fc35a08458c9327dc7ef63de0/tests/integration/utils/create-live-test-site.ts#L22-L36) but wasn't being used here, so it was falling back to the first account the user has access to that is returned by the accounts API. The `netlify-integration-testing` account is properly configured to host our various integration test sites, e.g. to avoid being rate limited. * ci: try DEBUG_TESTS=1 * Revert "ci: try DEBUG_TESTS=1" This reverts commit 2d186bf. * chore: bump deploy integration test timeout and concurrency
* feat: begin integrating `@netlify/dev` * chore: add deps * fix(deps): bump netlify packages to dedupe with @netlify/dev * chore: update deps * chore: fix lint issue * chore: add debug logging * chore: add comment Co-authored-by: Philippe Serhal <philippe.serhal@netlify.com> * chore: cap site name length in tests * chore: update snapshot --------- Co-authored-by: Philippe Serhal <philippe.serhal@netlify.com>
* feat: deprecate sites:create-template * chore: format * chore: lint * docs: build docs * fix: claude says this is how we fix the PR title lint issue * fix: lol now claude says this will work * remove this * remove unneeded issuePrefixes that claude added * fix: remove a bit more dead code * fix: remove sites:create-template placeholder cmd --------- Co-authored-by: Philippe Serhal <philippe.serhal@netlify.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request modifies the manual repository configuration initialization flow and adds comprehensive test coverage. The production change in Estimated code review effort🎯 2 (Simple) | ⏱️ ~13 minutes RationaleThe production code modification is minimal (2 lines) with straightforward fallback logic. The test additions total 287 lines across two files following consistent patterns for validating provider-specific behavior and URL parsing. While substantial in line count, the tests apply repetitive assertions across similar scenarios (multiple providers and URL formats), which reduces per-file cognitive load. No complex structural changes or heterogeneous logic modifications are present. 🚥 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 unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/utils/get-repo-data.test.ts`:
- Around line 8-119: The tests are self-fulfilling because they construct
RepoData literals and assert those same values; update each spec to call the
real getRepoData function instead of building RepoData objects so
parsing/normalization is exercised. Replace the mockRepoData literals with calls
to getRepoData(...) (or the exported function that returns RepoData) using the
raw git URL/branch/name inputs from each case (e.g.,
'git@github.com:ownername/test.git', 'https://gitlab.com/...', custom host,
etc.), then assert on returnedRepo.httpsUrl, returnedRepo.provider and
returnedRepo.repo to validate behavior. Ensure you import getRepoData and keep
the same expected strings in assertions.
In `@tests/unit/utils/init/config-manual.test.ts`:
- Around line 41-45: The test is failing due to an unsafe direct cast of the
Netlify `config` property; locate the object literal assigned to
`netlify.config` in tests/unit/utils/init/config-manual.test.ts and change the
cast so the provided `{ plugins: [] }` is first cast to `unknown` and then to
the required `CachedConfig['config']` (or BaseCommand['netlify']['config']) type
instead of a direct cast; this preserves the intended test shape while
satisfying TypeScript's structural checks for `build` and other required
properties referenced by `CachedConfig['config']`.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/utils/init/config-manual.tstests/unit/utils/get-repo-data.test.tstests/unit/utils/init/config-manual.test.ts
| describe('getRepoData', () => { | ||
| describe('RepoData structure for different Git providers', () => { | ||
| it('should construct correct httpsUrl for GitHub SSH URLs', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'ownername', | ||
| repo: 'ownername/test', | ||
| url: 'git@github.com:ownername/test.git', | ||
| branch: 'main', | ||
| provider: 'github', | ||
| httpsUrl: 'https://github.com/ownername/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://github.com/ownername/test') | ||
| expect(mockRepoData.provider).toBe('github') | ||
| expect(mockRepoData.repo).toBe('ownername/test') | ||
| }) | ||
|
|
||
| it('should construct correct httpsUrl for GitLab SSH URLs', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'ownername', | ||
| repo: 'ownername/test', | ||
| url: 'git@gitlab.com:ownername/test.git', | ||
| branch: 'main', | ||
| provider: 'gitlab', | ||
| httpsUrl: 'https://gitlab.com/ownername/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://gitlab.com/ownername/test') | ||
| expect(mockRepoData.provider).toBe('gitlab') | ||
| expect(mockRepoData.repo).toBe('ownername/test') | ||
| }) | ||
|
|
||
| it('should construct correct httpsUrl for GitHub HTTPS URLs', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'ownername', | ||
| repo: 'ownername/test', | ||
| url: 'https://github.com/ownername/test.git', | ||
| branch: 'main', | ||
| provider: 'github', | ||
| httpsUrl: 'https://github.com/ownername/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://github.com/ownername/test') | ||
| expect(mockRepoData.provider).toBe('github') | ||
| expect(mockRepoData.repo).toBe('ownername/test') | ||
| }) | ||
|
|
||
| it('should construct correct httpsUrl for GitLab HTTPS URLs', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'ownername', | ||
| repo: 'ownername/test', | ||
| url: 'https://gitlab.com/ownername/test.git', | ||
| branch: 'main', | ||
| provider: 'gitlab', | ||
| httpsUrl: 'https://gitlab.com/ownername/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://gitlab.com/ownername/test') | ||
| expect(mockRepoData.provider).toBe('gitlab') | ||
| expect(mockRepoData.repo).toBe('ownername/test') | ||
| }) | ||
|
|
||
| it('should use host as provider for unknown Git hosts', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'user', | ||
| repo: 'user/test', | ||
| url: 'git@custom-git.example.com:user/test.git', | ||
| branch: 'main', | ||
| provider: 'custom-git.example.com', | ||
| httpsUrl: 'https://custom-git.example.com/user/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://custom-git.example.com/user/test') | ||
| expect(mockRepoData.provider).toBe('custom-git.example.com') | ||
| expect(mockRepoData.repo).toBe('user/test') | ||
| }) | ||
| }) | ||
|
|
||
| describe('provider field mapping', () => { | ||
| it('should map github.com to "github" provider', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'user', | ||
| repo: 'user/test', | ||
| url: 'git@github.com:user/test.git', | ||
| branch: 'main', | ||
| provider: 'github', | ||
| httpsUrl: 'https://github.com/user/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.provider).toBe('github') | ||
| }) | ||
|
|
||
| it('should map gitlab.com to "gitlab" provider', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'user', | ||
| repo: 'user/test', | ||
| url: 'git@gitlab.com:user/test.git', | ||
| branch: 'main', | ||
| provider: 'gitlab', | ||
| httpsUrl: 'https://gitlab.com/user/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.provider).toBe('gitlab') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Tests are self-fulfilling; exercise getRepoData instead.
These cases construct RepoData literals and assert the same values, so they don’t validate parsing/normalization behavior. Consider driving the tests through getRepoData (with mocked repo info) and asserting its output to catch regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/utils/get-repo-data.test.ts` around lines 8 - 119, The tests are
self-fulfilling because they construct RepoData literals and assert those same
values; update each spec to call the real getRepoData function instead of
building RepoData objects so parsing/normalization is exercised. Replace the
mockRepoData literals with calls to getRepoData(...) (or the exported function
that returns RepoData) using the raw git URL/branch/name inputs from each case
(e.g., 'git@github.com:ownername/test.git', 'https://gitlab.com/...', custom
host, etc.), then assert on returnedRepo.httpsUrl, returnedRepo.provider and
returnedRepo.repo to validate behavior. Ensure you import getRepoData and keep
the same expected strings in assertions.
| netlify: { | ||
| api: mockApi as NetlifyAPI, | ||
| cachedConfig: { configPath: '/test/netlify.toml' } as BaseCommand['netlify']['cachedConfig'], | ||
| config: { plugins: [] } as BaseCommand['netlify']['config'], | ||
| repositoryRoot: '/test', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(config-manual\.test\.ts|base-command\.ts)"Repository: netlify/cli
Length of output: 127
🏁 Script executed:
cat -n tests/unit/utils/init/config-manual.test.ts | sed -n '35,50p'Repository: netlify/cli
Length of output: 608
🏁 Script executed:
head -100 src/commands/base-command.ts | cat -nRepository: netlify/cli
Length of output: 4256
🏁 Script executed:
cat -n src/commands/base-command.ts | sed -n '100,250p'Repository: netlify/cli
Length of output: 6552
🏁 Script executed:
cat -n src/commands/types.tsRepository: netlify/cli
Length of output: 108
🏁 Script executed:
rg -t ts "type NetlifyOptions|interface NetlifyOptions" --max-count 5Repository: netlify/cli
Length of output: 109
🏁 Script executed:
cat -n src/commands/types.d.tsRepository: netlify/cli
Length of output: 1910
🏁 Script executed:
rg -t ts "type NormalizedCachedConfigConfig|interface NormalizedCachedConfigConfig" -A 15Repository: netlify/cli
Length of output: 14867
🏁 Script executed:
rg -t ts "type CachedConfig|interface CachedConfig" src/lib/build.js -A 20Repository: netlify/cli
Length of output: 111
🏁 Script executed:
fd build src/lib -t f | head -10Repository: netlify/cli
Length of output: 71
🏁 Script executed:
rg "type CachedConfig|interface CachedConfig" --max-count 5Repository: netlify/cli
Length of output: 154
🏁 Script executed:
rg "export interface CachedConfig" -A 30 src/lib/build.tsRepository: netlify/cli
Length of output: 1068
🏁 Script executed:
rg "export interface CachedConfig" -A 60 src/lib/build.ts | head -80Repository: netlify/cli
Length of output: 2080
🏁 Script executed:
rg "export interface CachedConfig" -A 150 src/lib/build.ts | grep -A 5 "plugins"Repository: netlify/cli
Length of output: 499
🏁 Script executed:
cat -n tests/unit/utils/init/config-manual.test.ts | head -80Repository: netlify/cli
Length of output: 3052
🏁 Script executed:
cat tsconfig.json | head -50Repository: netlify/cli
Length of output: 94
🏁 Script executed:
cat tsconfig.base.json | head -80Repository: netlify/cli
Length of output: 403
🏁 Script executed:
cd /tmp && cat > check_types.ts << 'EOF'
import type { CachedConfig } from '/repo/src/lib/build'
import type BaseCommand from '/repo/src/commands/base-command'
// This is what the test is trying to do
const testConfig: BaseCommand['netlify']['config'] = { plugins: [] }
EOF
npx tsc --noEmit check_types.ts 2>&1Repository: netlify/cli
Length of output: 300
🏁 Script executed:
rg "export interface CachedConfig" -A 200 src/lib/build.ts | tail -50Repository: netlify/cli
Length of output: 1462
Fix the failing type assertion on config.
Line 44 attempts an invalid direct cast. The config property requires a CachedConfig['config'] object with required properties like build, but only provides { plugins: [] }. Use a two-step cast through unknown:
Proposed fix
- config: { plugins: [] } as BaseCommand['netlify']['config'],
+ config: { plugins: [] } as unknown as BaseCommand['netlify']['config'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| netlify: { | |
| api: mockApi as NetlifyAPI, | |
| cachedConfig: { configPath: '/test/netlify.toml' } as BaseCommand['netlify']['cachedConfig'], | |
| config: { plugins: [] } as BaseCommand['netlify']['config'], | |
| repositoryRoot: '/test', | |
| netlify: { | |
| api: mockApi as NetlifyAPI, | |
| cachedConfig: { configPath: '/test/netlify.toml' } as BaseCommand['netlify']['cachedConfig'], | |
| config: { plugins: [] } as unknown as BaseCommand['netlify']['config'], | |
| repositoryRoot: '/test', |
🧰 Tools
🪛 GitHub Actions: Typecheck
[error] 44-44: TypeScript error TS2352: Conversion of type '{ plugins: never[]; }' to type 'NormalizedCachedConfigConfig' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first. (During npm run typecheck/tsc)
🪛 GitHub Check: typecheck
[failure] 44-44:
Conversion of type '{ plugins: never[]; }' to type 'NormalizedCachedConfigConfig' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/utils/init/config-manual.test.ts` around lines 41 - 45, The test
is failing due to an unsafe direct cast of the Netlify `config` property; locate
the object literal assigned to `netlify.config` in
tests/unit/utils/init/config-manual.test.ts and change the cast so the provided
`{ plugins: [] }` is first cast to `unknown` and then to the required
`CachedConfig['config']` (or BaseCommand['netlify']['config']) type instead of a
direct cast; this preserves the intended test shape while satisfying
TypeScript's structural checks for `build` and other required properties
referenced by `CachedConfig['config']`.
4 participants
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #3771
Also not described in the issue was that we weren't displaying the correct webhook for gitlab users to add manually since we were passing through
manualinstead ofgitlab- now it actually showsConfigure the following webhook for your repository: https://api.netlify.com/hooks/gitlabas it should (and was in the code, just wasn't being exercised).For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)
