diff --git a/src/app/shared/flow-assistant/flow-assistant.html b/src/app/shared/flow-assistant/flow-assistant.html index fe814ec..debd686 100644 --- a/src/app/shared/flow-assistant/flow-assistant.html +++ b/src/app/shared/flow-assistant/flow-assistant.html @@ -105,13 +105,14 @@ Credential @for (credential of compatibleCredentials(); track credential.id) { {{ credential.label }} } @if (credentialsLoading()) { Loading credentials... } + @else if (!compatibleCredentials().length) { No credentials available } @if (credentialsError()) { {{ credentialsError() }} } @if (!credentialsLoading() && !compatibleCredentials().length) { @@ -221,35 +222,9 @@ } - @if (!credentialFormOpen()) { - } @else { -
- - Label - - - - Provider - - @for (provider of providers(); track provider) { {{ provider }} } - - - - Description (optional) - - - - {{ editingCredentialId() ? 'New value (to rotate)' : 'API key' }} - - {{ editingCredentialId() ? 'Leave empty to keep the current key.' : 'The value will not be shown again after saving.' }} - - @if (credentialsError()) {

{{ credentialsError() }}

} - -
- } + @if (credentialsError()) {

{{ credentialsError() }}

} + @if (credentialSaving()) {

Saving...

} } diff --git a/src/app/shared/flow-assistant/flow-assistant.spec.ts b/src/app/shared/flow-assistant/flow-assistant.spec.ts index 20701b3..e799eab 100644 --- a/src/app/shared/flow-assistant/flow-assistant.spec.ts +++ b/src/app/shared/flow-assistant/flow-assistant.spec.ts @@ -1,6 +1,7 @@ import { TestBed } from '@angular/core/testing'; import { AssistantService } from '@services/assistant/assistant'; import { Authorization } from '@services/authorization/authorization'; +import { NodeSettingsDialogService } from '@services/dialogs/node-settings-dialog'; import { VaultService } from '@services/vault/vault'; import { AssistantSessionStore } from '@stores/assistant-session-store'; import { EditorStateHolder } from '@stores/flow-editor'; @@ -18,7 +19,12 @@ import { FlowAssistant } from './flow-assistant'; describe('FlowAssistant model selection', () => { const MODELS_URL = '/retriever/LLM/models?provider={provider}'; - function build(options: { models: string[]; open: boolean }) { + function build(options: { + models: string[]; + open: boolean; + dialogResult?: Record | null; + credentials?: unknown[]; + }) { const assistant = { getConfig: vi.fn(() => of({ defaultProvider: '', @@ -34,10 +40,18 @@ describe('FlowAssistant model selection', () => { listProviderCatalog: vi.fn(() => of([])) }; + const vault = { + listSecrets: vi.fn(() => of(options.credentials ?? [])), + createSecret: vi.fn(() => of({ id: 'cred-1', label: 'Key', provider: 'Gemini', active: true })), + updateSecret: vi.fn(() => of({ id: 'cred-1', label: 'Key', provider: 'Gemini', active: true })) + }; + const settingsDialog = { open: vi.fn((_input: unknown) => Promise.resolve(options.dialogResult ?? null)) }; + TestBed.configureTestingModule({ providers: [ { provide: AssistantService, useValue: assistant }, - { provide: VaultService, useValue: { listSecrets: vi.fn(() => of([])) } }, + { provide: NodeSettingsDialogService, useValue: settingsDialog }, + { provide: VaultService, useValue: vault }, { provide: EditorStateHolder, useValue: { currentFlow: vi.fn(() => null), activeFlowData: vi.fn(() => null) } @@ -58,7 +72,7 @@ describe('FlowAssistant model selection', () => { const component = TestBed.createComponent(FlowAssistant).componentInstance as any; component.assistantConfig.set({ availableModelsRetrieverUrl: MODELS_URL }); - return { component, assistant }; + return { component, assistant, vault, settingsDialog }; } afterEach(() => TestBed.resetTestingModule()); @@ -109,3 +123,141 @@ describe('FlowAssistant model selection', () => { expect(component.modelsOpen()).toBe(false); }); }); + +describe('FlowAssistant credentials', () => { + const MODELS_URL = '/retriever/LLM/models?provider={provider}'; + + function build(options: { + dialogResult?: Record | null; + credentials?: unknown[]; + } = {}) { + const assistant = { + getConfig: vi.fn(() => of({ + defaultProvider: '', + defaultModel: '', + availableProvidersRetrieverUrl: '/retriever/LLM/providers', + availableModelsRetrieverUrl: MODELS_URL, + defaultPhaseModels: {}, + providerCatalogUrl: '/llm/providers' + })), + listProviders: vi.fn(() => of(['InternalOllama', 'Gemini'])), + listModels: vi.fn(() => of([])), + areModelsOpen: vi.fn(() => of(true)), + listProviderCatalog: vi.fn(() => of([])) + }; + const vault = { + listSecrets: vi.fn(() => of(options.credentials ?? [])), + createSecret: vi.fn(() => of({ id: 'cred-1', label: 'Key', provider: 'Gemini', active: true })), + updateSecret: vi.fn(() => of({ id: 'cred-1', label: 'Key', provider: 'Gemini', active: true })) + }; + const settingsDialog = { open: vi.fn((_input: unknown) => Promise.resolve(options.dialogResult ?? null)) }; + + TestBed.configureTestingModule({ + providers: [ + { provide: AssistantService, useValue: assistant }, + { provide: NodeSettingsDialogService, useValue: settingsDialog }, + { provide: VaultService, useValue: vault }, + { + provide: EditorStateHolder, + useValue: { currentFlow: vi.fn(() => null), activeFlowData: vi.fn(() => null) } + }, + { provide: Authorization, useValue: { currentUser: vi.fn(() => null) } }, + { + provide: AssistantSessionStore, + useValue: { + flowKey: vi.fn(() => 'flow-1'), + getSnapshot: vi.fn(() => null), + setSnapshot: vi.fn(), + clearSnapshot: vi.fn(), + cloneSnapshot: vi.fn((snapshot: unknown) => snapshot) + } + } + ] + }); + + const component = TestBed.createComponent(FlowAssistant).componentInstance as any; + component.providers.set(['InternalOllama', 'Gemini']); + return { component, vault, settingsDialog }; + } + + afterEach(() => TestBed.resetTestingModule()); + + it('opens a modal wherever it is asked for, without needing the credentials panel open', async () => { + // It used to set a flag whose form only rendered inside the credentials panel, so pressing Add + // credential from the picker beside the model looked like it did nothing at all. + const { component, settingsDialog } = build(); + expect(component.credentialsPanelOpen()).toBe(false); + + await component.openCredentialForm('Gemini'); + + expect(settingsDialog.open).toHaveBeenCalledTimes(1); + const dialog = settingsDialog.open.mock.calls[0][0] as any; + expect(dialog.title).toBe('Add credential'); + expect(dialog.fields.map((field: any) => field.key)).toEqual(['label', 'provider', 'description', 'value']); + expect(dialog.initial.provider).toBe('Gemini'); + }); + + it('requires a label and a key to create one, and lets the dialog enforce it', async () => { + const { settingsDialog, component } = build(); + + await component.openCredentialForm('Gemini'); + + const byKey = new Map( + (settingsDialog.open.mock.calls[0][0] as any).fields.map((field: any) => [field.key, field]) + ); + expect(byKey.get('label').required).toBe(true); + expect(byKey.get('value').required).toBe(true); + expect(byKey.get('provider').readonly).toBe(false); + }); + + it('saves what the modal returned', async () => { + const { component, vault } = build({ + dialogResult: { label: 'Prod key', provider: 'Gemini', description: 'shared', value: 'sk-1' } + }); + + await component.openCredentialForm('Gemini'); + + expect(vault.createSecret).toHaveBeenCalledWith({ + label: 'Prod key', + provider: 'Gemini', + description: 'shared', + value: 'sk-1' + }); + }); + + it('writes nothing when the modal is cancelled', async () => { + const { component, vault } = build({ dialogResult: null }); + + await component.openCredentialForm('Gemini'); + + expect(vault.createSecret).not.toHaveBeenCalled(); + expect(vault.updateSecret).not.toHaveBeenCalled(); + }); + + it('locks the provider and makes the key optional when rotating an existing one', async () => { + // The provider is what makes a credential compatible, so rotating a key must not move it; and + // an empty key means "keep the current one" rather than "no key". + const { component, settingsDialog, vault } = build({ + dialogResult: { label: 'Prod key', provider: 'Gemini', description: '', value: '' } + }); + + await component.editCredential({ id: 'cred-1', label: 'Prod key', provider: 'Gemini', active: true }); + + const dialog = settingsDialog.open.mock.calls[0][0] as any; + const byKey = new Map(dialog.fields.map((field: any) => [field.key, field])); + expect(byKey.get('provider').readonly).toBe(true); + expect(byKey.get('value').required).toBe(false); + expect(vault.updateSecret).toHaveBeenCalledWith('cred-1', { label: 'Prod key', description: undefined }); + }); + + it('offers the current provider even when the list never loaded', async () => { + const { component, settingsDialog } = build(); + component.providers.set([]); + + await component.openCredentialForm('Gemini'); + + const provider = (settingsDialog.open.mock.calls[0][0] as any).fields + .find((field: any) => field.key === 'provider'); + expect(provider.options).toEqual([{ label: 'Gemini', value: 'Gemini' }]); + }); +}); diff --git a/src/app/shared/flow-assistant/flow-assistant.ts b/src/app/shared/flow-assistant/flow-assistant.ts index 45c07f1..591b587 100644 --- a/src/app/shared/flow-assistant/flow-assistant.ts +++ b/src/app/shared/flow-assistant/flow-assistant.ts @@ -22,6 +22,7 @@ import { Flow } from '@models/flow'; import { AssistantService } from '@services/assistant/assistant'; import { CREDENTIAL_ERROR_MESSAGES, VaultService } from '@services/vault/vault'; import { Authorization } from '@services/authorization/authorization'; +import { NodeSettingField, NodeSettingsDialogService } from '@services/dialogs/node-settings-dialog'; import { AssistantSessionStore } from '@stores/assistant-session-store'; import { EditorStateHolder } from '@stores/flow-editor'; import { finalize, firstValueFrom, interval, Subscription, switchMap, take } from 'rxjs'; @@ -42,6 +43,7 @@ export class FlowAssistant implements OnInit, OnDestroy { private readonly editorState = inject(EditorStateHolder); private readonly authorization = inject(Authorization); private readonly sessionStore = inject(AssistantSessionStore); + private readonly settingsDialog = inject(NodeSettingsDialogService); private pollSubscription: Subscription | null = null; private initialized = false; private activeFlowKey: string | null = null; @@ -80,13 +82,7 @@ export class FlowAssistant implements OnInit, OnDestroy { readonly credentialsError = signal(null); readonly selectedCredentialId = signal(''); readonly credentialsPanelOpen = signal(false); - readonly credentialFormOpen = signal(false); readonly credentialSaving = signal(false); - readonly editingCredentialId = signal(null); - readonly credentialLabel = signal(''); - readonly credentialProvider = signal(''); - readonly credentialDescription = signal(''); - readonly credentialValue = signal(''); readonly advancedModelsOpen = signal(false); readonly modelPickerOpen = signal(false); readonly quickPromptsOpen = signal(true); @@ -317,35 +313,80 @@ export class FlowAssistant implements OnInit, OnDestroy { } } - openCredentialForm(provider = this.selectedProvider()) { + /** + * A modal rather than a form inside the credentials panel: this is reachable from the credential + * picker next to the model too, and there the panel it used to appear in may well be collapsed - + * so pressing Add credential looked like it did nothing at all. + */ + async openCredentialForm(provider = this.selectedProvider()) { if (this.configurationLocked()) return; - this.editingCredentialId.set(null); - this.credentialLabel.set(''); - this.credentialProvider.set(provider); - this.credentialDescription.set(''); - this.credentialValue.set(''); - this.credentialsError.set(null); - this.credentialsPanelOpen.set(true); - this.credentialFormOpen.set(true); + await this.openCredentialDialog(null, provider); } - editCredential(credential: VaultSecret) { - this.editingCredentialId.set(credential.id); - this.credentialLabel.set(credential.label); - this.credentialProvider.set(credential.provider); - this.credentialDescription.set(credential.description ?? ''); - this.credentialValue.set(''); - this.credentialsError.set(null); - this.credentialsPanelOpen.set(true); - this.credentialFormOpen.set(true); + async editCredential(credential: VaultSecret) { + await this.openCredentialDialog(credential, credential.provider); } - saveCredential() { - const label = this.credentialLabel().trim(); - const provider = this.credentialProvider().trim(); - const description = this.credentialDescription().trim(); - const value = this.credentialValue(); - const editingId = this.editingCredentialId(); + private async openCredentialDialog(editing: VaultSecret | null, provider: string) { + this.credentialsError.set(null); + + // The current provider is offered even when the list has not loaded: it is the one the user + // is adding a key for, and a select with nothing in it would be a dead end. + const providerOptions = [...new Set([...this.providers(), provider].filter((name) => !!name))] + .map((name) => ({ label: name, value: name })); + + const fields: NodeSettingField[] = [ + { key: 'label', label: 'Label', type: 'text', required: true, autofocus: true }, + { + key: 'provider', + label: 'Provider', + type: 'select', + required: true, + options: providerOptions, + // What makes a credential compatible, so rotating a key must not move it to another one. + readonly: editing != null + }, + { key: 'description', label: 'Description (optional)', type: 'text' }, + { + key: 'value', + label: editing ? 'New value (to rotate)' : 'API key', + type: 'password', + required: editing == null, + tip: editing + ? 'Leave empty to keep the current key.' + : 'The value will not be shown again after saving.' + } + ]; + + const result = await this.settingsDialog.open({ + title: editing ? `Edit ${editing.label}` : 'Add credential', + fields, + initial: { + label: editing?.label ?? '', + provider, + description: editing?.description ?? '', + value: '' + } + }); + if (!result) return; + + this.saveCredential({ + editingId: editing?.id ?? null, + label: String(result['label'] ?? '').trim(), + provider: String(result['provider'] ?? '').trim(), + description: String(result['description'] ?? '').trim(), + value: String(result['value'] ?? '') + }); + } + + private saveCredential(input: { + editingId: string | null; + label: string; + provider: string; + description: string; + value: string; + }) { + const { editingId, label, provider, description, value } = input; if (!label || !provider || (!editingId && !value.trim()) || this.credentialSaving()) return; this.credentialSaving.set(true); @@ -358,13 +399,8 @@ export class FlowAssistant implements OnInit, OnDestroy { }) : this.vault.createSecret({ label, provider, description: description || undefined, value }); - request.pipe(finalize(() => { - this.credentialSaving.set(false); - this.credentialValue.set(''); - })).subscribe({ + request.pipe(finalize(() => this.credentialSaving.set(false))).subscribe({ next: (credential) => { - this.credentialFormOpen.set(false); - this.editingCredentialId.set(null); if (credential.active && this.sameProvider(credential.provider, this.selectedProvider())) { this.selectedCredentialId.set(credential.id); } diff --git a/src/app/shared/task-execution-viewer/task-execution-viewer.html b/src/app/shared/task-execution-viewer/task-execution-viewer.html index 929b15e..9249000 100644 --- a/src/app/shared/task-execution-viewer/task-execution-viewer.html +++ b/src/app/shared/task-execution-viewer/task-execution-viewer.html @@ -334,7 +334,7 @@ Credential @for (credential of llmCredentialOptionsFor(entry); track credential.id) { @@ -343,7 +343,8 @@ } @if (llmCredentialLoadingFor(entry)) { Loading credentials... } - @if (authorizationSavingFor(entry)) { Saving credential... } + @else if (authorizationSavingFor(entry)) { Saving credential... } + @else if (!llmCredentialOptionsFor(entry).length) { No credentials available } @if (llmCredentialErrorFor(entry); as credentialError) {
@@ -354,12 +355,14 @@ @if (authorizationErrorFor(entry); as authorizationError) {
{{ authorizationError }}
} - @if (!llmCredentialLoadingFor(entry) && !llmCredentialOptionsFor(entry).length && !llmCredentialErrorFor(entry)) { -
No compatible credentials available.
+ @if (credentialFormError(); as formError) { +
{{ formError }}
}
@if (!llmCredentialLoadingFor(entry)) { - + } @if (editingAuthorizationKeys()[entry.requirement.key]) { @@ -368,31 +371,6 @@ } - @if (credentialFormOpen()) { -
-
New {{ credentialFormProvider() }} credential
- - Label - - - - Description (optional) - - - - API key - - The value will not be shown after saving. - - @if (credentialFormError(); as formError) { -
{{ formError }}
- } -
- - -
-
- } >({}); /** Satisfied requirements the user reopened to pick a different credential. */ readonly editingAuthorizationKeys = signal>({}); - readonly credentialFormOpen = signal(false); - readonly credentialFormProvider = signal(''); - readonly credentialFormLabel = signal(''); - readonly credentialFormDescription = signal(''); - readonly credentialFormValue = signal(''); readonly credentialFormSaving = signal(false); readonly credentialFormError = signal(null); readonly outputPreviewModal = signal(null); @@ -267,7 +262,6 @@ export class TaskExecutionViewerComponent implements OnDestroy { this.llmCredentialErrors.set({}); this.editingAuthorizationKeys.set({}); this.requestedCredentialProviders.clear(); - this.credentialFormOpen.set(false); this.credentialFormError.set(null); this.loadLlmProviderCapabilities(); this.activeAsideTab.set('inputs'); @@ -1023,37 +1017,53 @@ export class TaskExecutionViewerComponent implements OnDestroy { this.submitAuthorization(entry.requirement); } - openVaultCredentialForm(provider: string) { + /** + * A modal, like the copilot's: the form used to render further down the aside, so on a long run + * pressing Add credential scrolled nothing into view and looked like it did nothing. + */ + async openVaultCredentialForm(provider: string) { this.credentialFormError.set(null); - this.credentialFormProvider.set(provider); - this.credentialFormLabel.set(''); - this.credentialFormDescription.set(''); - this.credentialFormValue.set(''); - this.credentialFormOpen.set(true); + + // The provider is fixed by the requirement being answered, so it is shown rather than chosen. + const fields: NodeSettingField[] = [ + { key: 'provider', label: 'Provider', type: 'display' }, + { key: 'label', label: 'Label', type: 'text', required: true, autofocus: true }, + { key: 'description', label: 'Description (optional)', type: 'text' }, + { + key: 'value', + label: 'API key', + type: 'password', + required: true, + tip: 'The value will not be shown after saving.' + } + ]; + + const result = await this.settingsDialog.open({ + title: `Add ${provider} credential`, + fields, + initial: { provider, label: '', description: '', value: '' } + }); + if (!result) return; + + this.saveExecutionCredential( + provider, + String(result['label'] ?? '').trim(), + String(result['description'] ?? '').trim(), + String(result['value'] ?? '') + ); } - closeVaultCredentialForm() { - this.credentialFormOpen.set(false); - this.credentialFormValue.set(''); - this.credentialFormError.set(null); - } - - saveExecutionCredential() { - const provider = this.credentialFormProvider().trim(); - const label = this.credentialFormLabel().trim(); - const value = this.credentialFormValue(); + private saveExecutionCredential(provider: string, label: string, description: string, value: string) { if (!provider || !label || !value.trim() || this.credentialFormSaving()) return; this.credentialFormSaving.set(true); this.vaultService.createSecret({ provider, label, - description: this.credentialFormDescription().trim() || undefined, + description: description || undefined, value }).pipe(take(1)).subscribe({ next: (credential) => { this.credentialFormSaving.set(false); - this.credentialFormValue.set(''); - this.credentialFormOpen.set(false); this.credentialFormError.set(null); this.reloadLlmCredentials(provider); @@ -1067,7 +1077,6 @@ export class TaskExecutionViewerComponent implements OnDestroy { }, error: (error) => { this.credentialFormSaving.set(false); - this.credentialFormValue.set(''); this.credentialFormError.set(this.executionErrorMessage(error)); } });