Skip to content

Commit

Permalink
fix: UX improvements to FieldSelectModel and minor bug fixes (#247)
Browse files Browse the repository at this point in the history
- Fixes #238
- Fixes #244 
- Fixes #242
- Refactor components to Svelte 5
- Hides "**Recently used models**" group when there are no recently used
models.
  • Loading branch information
fmaclen authored Nov 29, 2024
1 parent f9bf8cb commit 9914980
Show file tree
Hide file tree
Showing 14 changed files with 81 additions and 51 deletions.
4 changes: 2 additions & 2 deletions src/lib/components/ButtonNew.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import LL from '$i18n/i18n-svelte';
import { Sitemap } from '$lib/sitemap';
import { generateStorageId } from '$lib/utils';
import { generateRandomId } from '$lib/utils';
import Button from './Button.svelte';
import { generateNewUrl } from './ButtonNew';
Expand All @@ -13,7 +13,7 @@
let href: string;
function setId() {
newId = generateStorageId();
newId = generateRandomId();
href = generateNewUrl(sitemap, newId);
}
Expand Down
4 changes: 2 additions & 2 deletions src/lib/components/ButtonNew.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { type Sitemap } from '$lib/sitemap';
import { generateStorageId } from '$lib/utils';
import { generateRandomId } from '$lib/utils';

export function generateNewUrl(sitemap: Sitemap, id?: string): string {
return `/${sitemap}/${id ? id : generateStorageId()}`;
return `/${sitemap}/${id ? id : generateRandomId()}`;
}
31 changes: 19 additions & 12 deletions src/lib/components/FieldSelectModel.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,26 @@
import FieldSelect from './FieldSelect.svelte';
export let isLabelVisible: boolean | undefined = true;
export let value: string | undefined;
interface Props {
isLabelVisible?: boolean;
value?: string;
}
let { isLabelVisible = true, value = $bindable() }: Props = $props();
const disabled = $derived(!$settingsStore.models?.length);
const models = $derived($settingsStore.models?.map(formatModelToSelectOption));
const lastUsedModels = $derived($settingsStore.lastUsedModels?.map(formatModelToSelectOption));
const otherModels = $derived(
models?.filter((m) => !lastUsedModels?.some((lm) => lm.value === m.value)) || []
);
type ModelOption = {
value: string;
label: string;
badge?: string | string[];
};
let disabled: boolean;
let models: ModelOption[] = [];
let lastUsedModels: ModelOption[] = [];
let otherModels: ModelOption[] = [];
function formatModelToSelectOption(model: Model): ModelOption {
const badges: string[] = [];
const modelServer = $serversStore.find((s) => s.id === model.serverId);
Expand All @@ -27,10 +33,10 @@
return { value: model.name, label: model.name, badge: badges };
}
$: disabled = !$settingsStore.models?.length;
$: models = $settingsStore.models?.map(formatModelToSelectOption);
$: lastUsedModels = $settingsStore.lastUsedModels?.map(formatModelToSelectOption);
$: otherModels = models?.filter((m) => !lastUsedModels?.some((lm) => lm.value === m.value)) || [];
// Auto-select model when there is only one available
$effect(() => {
if (!value && otherModels?.length === 1) value = otherModels[0].value;
});
</script>

<FieldSelect
Expand All @@ -40,7 +46,8 @@
label={$LL.availableModels()}
{isLabelVisible}
options={[
{ label: $LL.lastUsedModels(), options: lastUsedModels },
// Only include lastUsedModels if they exist
...(lastUsedModels?.length ? [{ label: $LL.lastUsedModels(), options: lastUsedModels }] : []),
{ label: $LL.otherModels(), options: otherModels }
]}
bind:value
Expand Down
4 changes: 3 additions & 1 deletion src/lib/connections.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { generateRandomId } from './utils';

export enum ConnectionType {
Ollama = 'ollama',
OpenAI = 'openai',
Expand Down Expand Up @@ -33,7 +35,7 @@ export function getDefaultServer(connectionType: ConnectionType): Server {
}

return {
id: crypto.randomUUID(),
id: generateRandomId(),
baseUrl,
connectionType,
modelFilter,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/utils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { formatDistanceToNow } from 'date-fns';

export function generateStorageId() {
export function generateRandomId() {
return Math.random().toString(36).substring(2, 8); // E.g. `z7avx9`
}

Expand Down
15 changes: 7 additions & 8 deletions src/routes/sessions/[id]/Prompt.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import { loadKnowledge, type Knowledge } from '$lib/knowledge';
import { knowledgeStore, serversStore } from '$lib/localStorage';
import type { Editor, Message, Session } from '$lib/sessions';
import { generateStorageId } from '$lib/utils';
import { generateRandomId } from '$lib/utils';
import KnowledgeSelect from './KnowledgeSelect.svelte';
Expand Down Expand Up @@ -97,11 +97,10 @@
role: 'user',
knowledge: a.knowledge,
content: `
CONTEXT
---
${a.knowledge.name}
---
${a.knowledge.content}
<CONTEXT>
<CONTEXT_NAME>${a.knowledge.name}</CONTEXT_NAME>
<CONTEXT_CONTENT>${a.knowledge.content}</CONTEXT_CONTENT>
</CONTEXT>
`
});
});
Expand Down Expand Up @@ -213,7 +212,7 @@ ${a.knowledge.content}
<Button
variant="outline"
on:click={() => {
attachments = [...attachments, { fieldId: generateStorageId() }];
attachments = [...attachments, { fieldId: generateRandomId() }];
}}
data-testid="knowledge-attachment"
>
Expand All @@ -239,7 +238,7 @@ ${a.knowledge.content}
<ButtonSubmit
handleSubmit={submit}
hasMetaKey={editor.isCodeEditor}
disabled={!editor.prompt || !session.model}
disabled={!editor.prompt || !session.model || editor.isCompletionInProgress}
>
{$LL.run()}
</ButtonSubmit>
Expand Down
9 changes: 4 additions & 5 deletions tests/attachments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,10 @@ test.describe('Knowledge Attachments', () => {
role: 'user',
knowledge: MOCK_KNOWLEDGE[0],
content: `
CONTEXT
---
${MOCK_KNOWLEDGE[0].name}
---
${MOCK_KNOWLEDGE[0].content}
<CONTEXT>
<CONTEXT_NAME>${MOCK_KNOWLEDGE[0].name}</CONTEXT_NAME>
<CONTEXT_CONTENT>${MOCK_KNOWLEDGE[0].content}</CONTEXT_CONTENT>
</CONTEXT>
`
});

Expand Down
4 changes: 2 additions & 2 deletions tests/docs.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { expect, test } from '@playwright/test';

import type { Model } from '$lib/settings';
import { generateRandomId } from '$lib/utils';

import {
MOCK_API_TAGS_RESPONSE,
MOCK_DEFAULT_SERVER_ID,
MOCK_KNOWLEDGE,
mockOllamaModelsResponse,
seedKnowledgeAndReload
Expand Down Expand Up @@ -37,7 +37,7 @@ test('seed data and take screenshots for README.md', async ({ page }) => {
// Stage 2 sessions
const models: Model[] = MOCK_API_TAGS_RESPONSE.models.map((model) => ({
name: model.name,
serverId: MOCK_DEFAULT_SERVER_ID
serverId: generateRandomId()
}));
await page.evaluate(
({ modelA, modelB }) =>
Expand Down
Binary file modified tests/docs.test.ts-snapshots/motd.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion tests/openai.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ test.describe('OpenAI Integration', () => {

// Simulate sending a message (you might need to adjust this based on your actual UI)
await page.locator('.prompt-editor__textarea').fill('Hello, AI!');
await mockOpenAICompletionResponse(page, MOCK_OPENAI_COMPLETION_RESPONSE_1);
await page.getByRole('button', { name: 'Run' }).click();

await page.getByLabel('Available models').click();
await expect(page.getByText('Recently used models', { exact: true })).toBeVisible();
await expect(page.getByRole('option', { name: 'gpt-3.5-turbo' })).toBeVisible();
Expand Down
26 changes: 26 additions & 0 deletions tests/servers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,4 +278,30 @@ test.describe('Servers', () => {
await expect(modelOption.last()).toContainText('llama.cpp');
await expect(modelOption.last()).not.toContainText('openai-compatible');
});

test('new connections are saved with correct serverIds', async ({ page }) => {
await mockOllamaModelsResponse(page);

// Check localStorage for correct format
const serversLocalStorage = await page.evaluate(() =>
window.localStorage.getItem('hollama-servers')
);
const servers = JSON.parse(serversLocalStorage || '[]');
expect(servers).toHaveLength(1);
expect(servers[0].id).toMatch(/^[a-z0-9]{6}$/); // Should match format from generateRandomId()

// Models aren't saved to localStorage until we load a new or existing session
await page.getByRole('link', { name: 'Sessions' }).click();
await page.getByTestId('new-session').click();
await expect(page.getByText('Write a prompt to start a new session')).toBeVisible();

// Verify the settings has the correct serverId reference
const settingsLocalStorage = await page.evaluate(() =>
window.localStorage.getItem('hollama-settings')
);
const settings = JSON.parse(settingsLocalStorage || '{}');
expect(settings.models).toBeDefined();
expect(settings.models.length).toBeGreaterThan(0);
expect(settings.models[0].serverId).toBe(servers[0].id);
});
});
21 changes: 12 additions & 9 deletions tests/session-interaction.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ test.describe('Session interaction', () => {
test('can start a new session, choose a model and stop a completion in progress', async ({
page
}) => {
const sendButton = page.getByText('Run');
const runButton = page.getByText('Run');
const stopButton = page.getByTitle('Stop completion');
const userMessage = page.locator('article', { hasText: 'You' });
const aiMessage = page.locator('article', { hasText: 'Assistant' });
Expand All @@ -184,17 +184,26 @@ test.describe('Session interaction', () => {
await chooseModel(page, MOCK_API_TAGS_RESPONSE.models[0].name);
await page.route('**/chat', () => {});
await promptTextarea.fill('Hello world!');
await sendButton.click();
await runButton.click();
await expect(userMessage).toBeVisible();
await expect(userMessage).toContainText('Hello world!');
await expect(aiMessage).toBeVisible();
await expect(aiMessage).toContainText('...');
await expect(promptTextarea).toHaveValue('');
await expect(sendButton).toBeDisabled();
await expect(runButton).toBeDisabled();
await expect(stopButton).toBeVisible();
await expect(page.getByText('Write a prompt to start a new session')).not.toBeVisible();
await expect(sessionMetadata).toHaveText(new RegExp(MOCK_API_TAGS_RESPONSE.models[0].name));

// Before the completion is stopped, make sure the Run button is disabled
// even if there is a prompt in the textarea and a model selected
await promptTextarea.fill('Hello again!');
await expect(page.locator('.field-combobox-input')).toHaveValue(
MOCK_API_TAGS_RESPONSE.models[0].name
);
await expect(runButton).toBeDisabled();

await promptTextarea.fill('');
await stopButton.click();
await expect(page.getByText('Write a prompt to start a new session')).toBeVisible();
await expect(userMessage).not.toBeVisible();
Expand Down Expand Up @@ -506,14 +515,8 @@ test.describe('Session interaction', () => {

// Attempt to navigate away
await page.getByText('Settings', { exact: true }).click();

// Check that we're still on the session page
await expect(page.getByTestId('session-id')).toBeVisible();

// Start another streamed completion
await promptTextarea.fill('Another test prompt');
await runButton.click();

// Wait for the completion to start
await expect(page.getByText('...')).toBeVisible();

Expand Down
6 changes: 1 addition & 5 deletions tests/ui.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,8 @@ test.describe('FieldSelect', () => {

// Open the dropdown
await modelCombobox.click();

// Check that the groups are visible
await expect(page.getByText('Recently used models', { exact: true })).toBeVisible();
await expect(page.getByText('Recently used models', { exact: true })).not.toBeVisible();
await expect(page.getByText('Other models', { exact: true })).toBeVisible();

// Check that all options are visible
await expect(page.getByRole('option')).toHaveCount(MOCK_API_TAGS_RESPONSE.models.length);

// Filter options
Expand Down
4 changes: 1 addition & 3 deletions tests/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,6 @@ export const MOCK_OPENAI_MODELS: OpenAI.Models.Model[] = [
{ id: 'text-davinci-003', object: 'model', created: 1669599635, owned_by: 'openai-internal' }
];

export const MOCK_DEFAULT_SERVER_ID = 'abc123';

export async function chooseFromCombobox(
page: Page,
label: string | RegExp,
Expand All @@ -176,7 +174,7 @@ export async function mockOllamaModelsResponse(page: Page) {
// Add the default server to the servers list
await page.evaluate(
(data) => window.localStorage.setItem('hollama-servers', JSON.stringify(data)),
[{ ...getDefaultServer(ConnectionType.Ollama), id: MOCK_DEFAULT_SERVER_ID }]
[{ ...getDefaultServer(ConnectionType.Ollama) }]
);

// Mock the tags response
Expand Down

0 comments on commit 9914980

Please sign in to comment.