From ffbd8c192b78e7f3745d278043ca18f5df733b40 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Mon, 7 Sep 2026 12:19:50 +0200 Subject: [PATCH] Open Add credential as a modal, and close the empty credential select Add credential set a flag whose form rendered only inside the credentials panel, further down the aside. Pressed from the credential picker beside the model - where you actually notice a key is missing - it scrolled nothing into view and looked like it did nothing at all. Both entry points now open the shared settings dialog, which is hosted at the app root and so works wherever it is asked for. The dialog carries the rules the inline forms carried by hand: a label and a key are required to create one, while rotating an existing key leaves it optional because empty there means "keep the current one", and the provider is read-only because it is what makes a credential compatible - rotating must not move it. The current provider is offered even when the provider list never loaded, so the select cannot be a dead end. A credential select with nothing in it is now disabled and says "No credentials available" instead of opening onto an empty list. That replaces the viewer's separate amber note, which said the same thing a second time in a second place. Co-Authored-By: Claude Opus 5 (1M context) --- .../shared/flow-assistant/flow-assistant.html | 33 +--- .../flow-assistant/flow-assistant.spec.ts | 158 +++++++++++++++++- .../shared/flow-assistant/flow-assistant.ts | 108 ++++++++---- .../task-execution-viewer.html | 38 +---- .../task-execution-viewer.ts | 61 ++++--- 5 files changed, 274 insertions(+), 124 deletions(-) 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)); } });