From 03ac4901ed62c58d1a2a3a3f894b8112fba5e7b7 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Tue, 8 Sep 2026 11:41:41 +0200 Subject: [PATCH] Put the provider/model picker above the dialog that opens it The picker (node-settings-dialog) shared --z-modal with every other dialog guest, so opening it from "Evaluate impact with LLM" - itself a modal - tied on z-index with the report behind it and lost on DOM order: it rendered, and its backdrop even blocked clicks, but neither was visible. It read as a button that did nothing. Named the layer this actually is - --z-dialog-over-modal, the same one the confirmation dialog already needed and had defined ad hoc as --z-confirm - and moved the picker onto it. While chasing this, closed a real silence next to it: with no LLM provider published at all, the picker answered null and the caller treated that like a dismissal, so the button did nothing for a second, unrelated reason. It now throws with a message, and Simulate surfaces it as a notification instead of swallowing it. Co-Authored-By: Claude Sonnet 5 --- .../llm-descriptor-settings.spec.ts | 11 +++++++++-- .../llm-descriptor-settings.ts | 5 +++-- .../node-settings-dialog/node-settings-dialog.html | 2 +- .../task-execution-viewer.html | 4 +++- .../task-execution-viewer/task-execution-viewer.ts | 14 +++++++++++++- src/styles.css | 9 ++++++++- 6 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/app/shared/llm-descriptor-settings/llm-descriptor-settings.spec.ts b/src/app/shared/llm-descriptor-settings/llm-descriptor-settings.spec.ts index 77cdb97..177e329 100644 --- a/src/app/shared/llm-descriptor-settings/llm-descriptor-settings.spec.ts +++ b/src/app/shared/llm-descriptor-settings/llm-descriptor-settings.spec.ts @@ -72,10 +72,17 @@ describe('openLLMDescriptorSettings', () => { expect(open.mock.calls[0][0].initial.seed).toBeUndefined(); }); - it('answers null when no provider is published at all', async () => { + it('says so, instead of opening nothing, when no provider is published at all', async () => { const { service, open } = dialog({ provider: 'x', model: 'y' }); - expect(await openLLMDescriptorSettings(service, retriever([]), { title: 'Simulation Settings' })).toBeNull(); + await expect(openLLMDescriptorSettings(service, retriever([]), { title: 'Simulation Settings' })) + .rejects.toThrow('No LLM provider is published'); expect(open).not.toHaveBeenCalled(); }); + + it('answers null when the dialog is dismissed', async () => { + const { service } = dialog(null); + + expect(await openLLMDescriptorSettings(service, retriever(), { title: 'Simulation Settings' })).toBeNull(); + }); }); diff --git a/src/app/shared/llm-descriptor-settings/llm-descriptor-settings.ts b/src/app/shared/llm-descriptor-settings/llm-descriptor-settings.ts index 277c4d4..d7773a7 100644 --- a/src/app/shared/llm-descriptor-settings/llm-descriptor-settings.ts +++ b/src/app/shared/llm-descriptor-settings/llm-descriptor-settings.ts @@ -48,7 +48,8 @@ export type LLMDescriptorSettingsRequest = { * They ask the same question, so they ask it with the same window - the provider list, the models * that follow from the chosen provider, and the same optional parameters. * - * Returns null when the dialog is dismissed, or when no provider is published at all. + * Returns null when the dialog is dismissed. Throws when no provider is published at all: the + * caller has a place to say so, and answering null there made the button that opened it look broken. */ export async function openLLMDescriptorSettings( settingsDialog: NodeSettingsDialogService, @@ -57,7 +58,7 @@ export async function openLLMDescriptorSettings( ): Promise { const providerOptions = await loadOptions(fieldRetriever, 'providers', {}, PROVIDER_RETRIEVER_URL); if (!providerOptions.length) { - return null; + throw new Error('No LLM provider is published by the server, so there is no model to choose.'); } const inherited = request.initialDescriptor ?? null; diff --git a/src/app/shared/node-settings-dialog/node-settings-dialog.html b/src/app/shared/node-settings-dialog/node-settings-dialog.html index 0e52792..fd5453d 100644 --- a/src/app/shared/node-settings-dialog/node-settings-dialog.html +++ b/src/app/shared/node-settings-dialog/node-settings-dialog.html @@ -1,5 +1,5 @@ @if (state()) { -
+
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 b7bc485..ff7f4a7 100644 --- a/src/app/shared/task-execution-viewer/task-execution-viewer.html +++ b/src/app/shared/task-execution-viewer/task-execution-viewer.html @@ -527,7 +527,9 @@ [executionId]="execution()!.id" [annotatedNodeCount]="biasAnnotatedNodeCount()" [blockedReason]="biasExperimentBlockedReason()" - (startExperimentRequested)="openBiasedRerunDialog()" /> + [comparableVariant]="canCompareBiasExecution()" + (startExperimentRequested)="openBiasedRerunDialog()" + (compareRequested)="openCompareDialog()" /> }
diff --git a/src/app/shared/task-execution-viewer/task-execution-viewer.ts b/src/app/shared/task-execution-viewer/task-execution-viewer.ts index c2eb8be..5229630 100644 --- a/src/app/shared/task-execution-viewer/task-execution-viewer.ts +++ b/src/app/shared/task-execution-viewer/task-execution-viewer.ts @@ -1132,7 +1132,19 @@ export class TaskExecutionViewerComponent implements OnDestroy { const executionId = this.execution()?.id; if (!executionId || !this.canSimulateExecution()) return; - const simulator = await this.openSimulationSettings(); + let simulator: LLMDescriptor | null; + try { + simulator = await this.openSimulationSettings(); + } catch (error) { + // No provider to choose from, or the list could not be loaded. Silence here reads as a dead + // button, which is what the picker is opened by. + this.notifications.show( + error instanceof Error ? error.message : 'Unable to load the providers to simulate with.', + 'error', + 6000 + ); + return; + } if (!simulator) return; this.simulateInProgress.set(true); diff --git a/src/styles.css b/src/styles.css index 47de21d..a5e2102 100644 --- a/src/styles.css +++ b/src/styles.css @@ -65,7 +65,14 @@ --space-5: 20px; --z-modal-backdrop: 10020; --z-modal: 10021; - --z-confirm: 10031; + /* + * Dialogs that are opened from inside another dialog: a confirmation, and the provider/model + * picker. At --z-modal they tied with the modal that opened them and lost on DOM order, so the + * picker for "Evaluate impact with LLM" opened behind the report it was asked for and read as a + * button that did nothing. + */ + --z-dialog-over-modal: 10031; + --z-confirm: var(--z-dialog-over-modal); } /* ── Semantic surface colours (hard-coded) ──────────────────────