Fix CLI port mismatch and centralize local dev URLs #1983
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| import * as p from '@clack/prompts'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| const mockProfileManager = vi.hoisted(() => ({ | ||
| getProfile: vi.fn().mockReturnValue(undefined), | ||
| addProfile: vi.fn(), | ||
| setActiveProfile: vi.fn(), | ||
| checkCredentialExists: vi.fn().mockResolvedValue(false), | ||
| })); | ||
|
|
||
| vi.mock('@clack/prompts'); | ||
|
|
||
| vi.mock('../../utils/profiles', async () => { | ||
| const actual = await vi.importActual('../../utils/profiles'); | ||
| return { | ||
| ...actual, | ||
| ProfileManager: vi.fn(() => mockProfileManager), | ||
| }; | ||
| }); | ||
|
|
||
| import { profileAddCommand } from '../../commands/profile'; | ||
| import { LOCAL_REMOTE } from '../../utils/profiles'; | ||
|
|
||
| describe('profileAddCommand', () => { | ||
| beforeEach(() => { | ||
| vi.spyOn(console, 'log').mockImplementation(() => {}); | ||
| vi.spyOn(process, 'exit').mockImplementation(() => { | ||
| throw new Error('process.exit called'); | ||
| }); | ||
| vi.clearAllMocks(); | ||
| mockProfileManager.checkCredentialExists.mockResolvedValue(false); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| describe('Local remote type', () => { | ||
| it('should create profile with LOCAL_REMOTE URLs without URL prompts', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('local'); | ||
| vi.mocked(p.text).mockResolvedValueOnce('development'); // environment | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); // switch profile | ||
|
|
||
| await profileAddCommand('test-local'); | ||
|
|
||
| expect(mockProfileManager.addProfile).toHaveBeenCalledWith('test-local', { | ||
| remote: { api: LOCAL_REMOTE.api, manageUi: LOCAL_REMOTE.manageUi }, | ||
| credential: 'none', | ||
| environment: 'development', | ||
| }); | ||
| }); | ||
|
|
||
| it('should not prompt for credential when Local is selected', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('local'); | ||
| vi.mocked(p.text).mockResolvedValueOnce('development'); // environment | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); | ||
|
|
||
| await profileAddCommand('test-local'); | ||
|
|
||
| // p.text called once (environment only), not for API URL, Manage UI URL, or credential | ||
| expect(p.text).toHaveBeenCalledTimes(1); | ||
| expect(p.text).toHaveBeenCalledWith( | ||
| expect.objectContaining({ message: 'Environment name:' }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should skip credential keychain warning for local profiles', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('local'); | ||
| vi.mocked(p.text).mockResolvedValueOnce('development'); | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); | ||
|
|
||
| await profileAddCommand('test-local'); | ||
|
|
||
| expect(mockProfileManager.checkCredentialExists).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should default environment to development for local', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('local'); | ||
| vi.mocked(p.text).mockResolvedValueOnce('development'); | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); | ||
|
|
||
| await profileAddCommand('my-local'); | ||
|
|
||
| expect(p.text).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: 'Environment name:', | ||
| initialValue: 'development', | ||
| }) | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Cloud remote type', () => { | ||
| it('should create profile with cloud remote string', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('cloud'); | ||
| vi.mocked(p.text) | ||
| .mockResolvedValueOnce('production') // environment | ||
| .mockResolvedValueOnce('inkeep-my-cloud'); // credential | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); | ||
|
|
||
| await profileAddCommand('my-cloud'); | ||
|
|
||
| expect(mockProfileManager.addProfile).toHaveBeenCalledWith('my-cloud', { | ||
| remote: 'cloud', | ||
| credential: 'inkeep-my-cloud', | ||
| environment: 'production', | ||
| }); | ||
| }); | ||
|
|
||
| it('should default environment to production for cloud', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('cloud'); | ||
| vi.mocked(p.text).mockResolvedValueOnce('production').mockResolvedValueOnce('inkeep-test'); | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); | ||
|
|
||
| await profileAddCommand('test-cloud'); | ||
|
|
||
| expect(p.text).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: 'Environment name:', | ||
| initialValue: 'production', | ||
| }) | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Custom remote type', () => { | ||
| it('should prompt for URLs with no initialValue', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('custom'); | ||
| vi.mocked(p.text) | ||
| .mockResolvedValueOnce('https://api.staging.example.com') // api URL | ||
| .mockResolvedValueOnce('https://manage.staging.example.com') // manage UI URL | ||
| .mockResolvedValueOnce('staging') // environment | ||
| .mockResolvedValueOnce('inkeep-staging'); // credential | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); | ||
|
|
||
| await profileAddCommand('staging'); | ||
|
|
||
| expect(mockProfileManager.addProfile).toHaveBeenCalledWith('staging', { | ||
| remote: { | ||
| api: 'https://api.staging.example.com', | ||
| manageUi: 'https://manage.staging.example.com', | ||
| }, | ||
| credential: 'inkeep-staging', | ||
| environment: 'staging', | ||
| }); | ||
| }); | ||
|
|
||
| it('should default environment to production for custom', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('custom'); | ||
| vi.mocked(p.text) | ||
| .mockResolvedValueOnce('https://api.example.com') | ||
| .mockResolvedValueOnce('https://manage.example.com') | ||
| .mockResolvedValueOnce('production') | ||
| .mockResolvedValueOnce('inkeep-prod'); | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); | ||
|
|
||
| await profileAddCommand('prod'); | ||
|
|
||
| expect(p.text).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: 'Environment name:', | ||
| initialValue: 'production', | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should check keychain for credential and warn if missing', async () => { | ||
| mockProfileManager.checkCredentialExists.mockResolvedValueOnce(false); | ||
| vi.mocked(p.select).mockResolvedValueOnce('custom'); | ||
| vi.mocked(p.text) | ||
| .mockResolvedValueOnce('https://api.example.com') | ||
| .mockResolvedValueOnce('https://manage.example.com') | ||
| .mockResolvedValueOnce('production') | ||
| .mockResolvedValueOnce('inkeep-prod'); | ||
| vi.mocked(p.confirm).mockResolvedValueOnce(false); | ||
|
|
||
| await profileAddCommand('prod'); | ||
|
|
||
| expect(mockProfileManager.checkCredentialExists).toHaveBeenCalledWith('inkeep-prod'); | ||
| }); | ||
| }); | ||
| }); | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import * as p from '@clack/prompts'; | ||
| import chalk from 'chalk'; | ||
| import { LOCAL_REMOTE, type Profile, ProfileError, ProfileManager } from '../utils/profiles'; | ||
|
|
||
| const profileManager = new ProfileManager(); | ||
|
|
||
|
|
@@ -75,7 +75,8 @@ export async function profileAddCommand(name?: string): Promise<void> { | |
| message: 'Remote type:', | ||
| options: [ | ||
| { value: 'cloud', label: 'Inkeep Cloud', hint: 'Default cloud deployment' }, | ||
| { value: 'local', label: 'Local', hint: 'Local development (localhost)' }, | ||
| { value: 'custom', label: 'Custom', hint: 'Self-hosted or staging deployment' }, | ||
| ], | ||
| }); | ||
|
|
||
|
|
@@ -88,13 +89,14 @@ export async function profileAddCommand(name?: string): Promise<void> { | |
|
|
||
| if (remoteType === 'cloud') { | ||
| remote = 'cloud'; | ||
| } else if (remoteType === 'local') { | ||
| remote = { ...LOCAL_REMOTE }; | ||
| } else { | ||
| const api = await p.text({ | ||
| message: 'Agents API URL:', | ||
| placeholder: 'https://your-agents-api.example.com', | ||
| validate: (value) => { | ||
| if (!value?.trim()) return 'URL is required'; | ||
| try { | ||
| new URL(value); | ||
| return undefined; | ||
|
|
@@ -111,9 +113,9 @@ export async function profileAddCommand(name?: string): Promise<void> { | |
|
|
||
| const manageUi = await p.text({ | ||
| message: 'Manage UI URL:', | ||
| placeholder: 'https://your-manage-ui.example.com', | ||
| validate: (value) => { | ||
| if (!value?.trim()) return 'URL is required'; | ||
| try { | ||
| new URL(value); | ||
| return undefined; | ||
|
|
@@ -134,11 +136,12 @@ export async function profileAddCommand(name?: string): Promise<void> { | |
| }; | ||
| } | ||
|
|
||
| // Cloud and custom (self-hosted/staging) default to 'production'; only local dev defaults to 'development' | ||
| const envDefault = remoteType === 'local' ? 'development' : 'production'; | ||
|
Contributor
There was a problem hiding this comment. 🟡 Minor: Custom profiles default to 'production' — verify this is intentional Issue: The environment default is now Why: Users setting up a staging environment (the hint says "Self-hosted or staging deployment") may be surprised their profile defaults to Fix: If intentional, consider either:
Refs:
|
||
| const environment = await p.text({ | ||
| message: 'Environment name:', | ||
| placeholder: envDefault, | ||
| initialValue: envDefault, | ||
| validate: (value) => { | ||
| if (!value) return 'Environment is required'; | ||
| return undefined; | ||
|
|
@@ -151,20 +154,28 @@ export async function profileAddCommand(name?: string): Promise<void> { | |
| } | ||
|
|
||
| // Generate credential reference name | ||
| let credential: string; | ||
|
|
||
| if (remoteType === 'local') { | ||
| credential = 'none'; | ||
| } else { | ||
| const credentialDefault = `inkeep-${profileName}`; | ||
| const credentialInput = await p.text({ | ||
| message: 'Credential reference:', | ||
| placeholder: credentialDefault, | ||
| initialValue: credentialDefault, | ||
| validate: (value) => { | ||
| if (!value) return 'Credential reference is required'; | ||
| return undefined; | ||
| }, | ||
| }); | ||
|
|
||
| if (p.isCancel(credentialInput)) { | ||
| p.cancel('Profile creation cancelled'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| credential = credentialInput; | ||
| } | ||
|
|
||
| // Create the profile | ||
|
|
@@ -179,12 +190,14 @@ export async function profileAddCommand(name?: string): Promise<void> { | |
| console.log(); | ||
| console.log(chalk.green('✓'), `Profile '${chalk.cyan(profileName)}' created successfully.`); | ||
|
|
||
| // Check if credential exists and warn if not (skip for 'none') | ||
| if (credential !== 'none') { | ||
| const credentialExists = await profileManager.checkCredentialExists(credential); | ||
| if (!credentialExists) { | ||
| console.log(); | ||
| console.log(chalk.yellow('⚠'), `Credential '${credential}' not found in keychain.`); | ||
| console.log(chalk.gray(' Run "inkeep login" to authenticate and store credentials.')); | ||
| } | ||
| } | ||
|
|
||
| // Ask if user wants to switch to this profile | ||
|
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Consider: Add explicit assertion for no
initialValueon Custom URL promptsIssue: The test title says "should prompt for URLs with no initialValue" but doesn't explicitly assert this. The test relies on the mock call sequence implicitly.
Why: An explicit assertion would make the test intention clearer and catch regressions if someone accidentally adds
initialValueto Custom prompts.Fix: Add an assertion like:
Refs: