fix: resolve workbench final review findings

This commit is contained in:
2026-07-19 19:59:38 +08:00
parent 6341a6bd8b
commit ba4ca16375
13 changed files with 357 additions and 44 deletions

View File

@@ -0,0 +1,27 @@
# Final Review Fix Report
## Status
Completed the final UI review fixes for the Discord-style project workbench.
## Fixed
- Cleared the selected inspector item on project and channel changes, with a Project A1 to Project A2 regression test.
- Reworked the mobile breakpoint into a compact horizontal project rail with bounded channel, content, and inspector regions; added a 390px Playwright assertion.
- Prevented inspector grid tracks from stretching compact controls.
- Added structured styles for all rendered channel rows, cards, sessions, metrics, and custom-link controls.
- Replaced the inert task status with a local, accessible checkbox completion control and coverage.
- Validated empty and malformed server values before normalization and added visible login connection status text.
- Disabled unfinished shell commands and converted inert tags and recent sessions to non-interactive list content.
- Rendered sorted channel icons, strengthened per-channel template coverage, and added copy feedback plus focus styling.
## Verification
- `npm test -- --run`: 6 files, 17 tests passed.
- `npm run build`: passed.
- `npx playwright test`: 2 Chromium tests passed.
- `git diff --check`: passed.
## Concern
- Vite continues to report the pre-existing notice that no Svelte config is present; it did not affect tests or the production build.

View File

@@ -24,3 +24,26 @@ test('login and project channel workbench flow', async ({ page }) => {
await page.getByRole('button', { name: 'Ops Dashboard' }).click();
await expect(page.getByText('Open external channel')).toBeVisible();
});
test('mobile workbench keeps project navigation and inspector compact', async ({ page }) => {
await page.setViewportSize({ width: 390, height: 844 });
await page.goto('/');
await page.getByLabel('Server IP or domain').fill('localhost:8080');
await page.getByLabel('Email or username').fill('david@example.com');
await page.getByLabel('Password').fill('secret');
await page.getByRole('button', { name: 'Log in' }).click();
const rail = page.getByLabel('Project list');
const sidebar = page.getByLabel('Project channels');
const stage = page.getByLabel('Channel content');
const inspector = page.getByLabel('Object inspector');
await expect(rail).toHaveCSS('grid-auto-flow', 'column');
await expect(rail).toHaveCSS('max-height', '76px');
await expect(inspector).toHaveCSS('max-height', '288px');
const [sidebarBox, stageBox] = await Promise.all([sidebar.boundingBox(), stage.boundingBox()]);
expect(sidebarBox?.y).toBeLessThan(stageBox?.y ?? 0);
expect((stageBox?.y ?? 0) - (sidebarBox?.y ?? 0)).toBeLessThanOrEqual(248);
});

View File

@@ -6,25 +6,44 @@
let account = '';
let password = '';
let error = '';
let connectionStatus = 'Not connected. Enter a server address to continue.';
function normalizeServer(value: string) {
function normalizeServer(value: string): { apiBase?: string; error?: string } {
const trimmed = value.trim();
if (trimmed.startsWith('http://') || trimmed.startsWith('https://')) {
return trimmed;
if (!trimmed) {
return { error: 'Enter a server address.' };
}
const candidate = /^https?:\/\//i.test(trimmed) ? trimmed : `http://${trimmed}`;
try {
const parsed = new URL(candidate);
if (!parsed.hostname || !['http:', 'https:'].includes(parsed.protocol)) {
return { error: 'Enter a valid HTTP or HTTPS server address.' };
}
return { apiBase: candidate };
} catch {
return { error: 'Enter a valid HTTP or HTTPS server address.' };
}
return `http://${trimmed}`;
}
function submitLogin() {
error = '';
const normalized = normalizeServer(server);
if (normalized.error) {
error = normalized.error;
connectionStatus = 'Check the server address before signing in.';
return;
}
if (!account.trim() || !password.trim()) {
error = 'Enter an account and password.';
return;
}
const normalized = normalizeServer(server);
server = normalized;
onLogin({ apiBase: normalized, account: account.trim() });
server = normalized.apiBase ?? server;
connectionStatus = 'Connection settings are ready.';
onLogin({ apiBase: server, account: account.trim() });
}
</script>
@@ -56,6 +75,8 @@
<p class="form-error" aria-live="polite">{error}</p>
{/if}
<p class="connection-status" role="status">{connectionStatus}</p>
<button type="submit">Log in</button>
<p class="login-note">The server address is remembered on this device.</p>
</form>

View File

@@ -11,6 +11,7 @@ describe('ServerLogin', () => {
expect(screen.getByLabelText('Email or username')).toBeInTheDocument();
expect(screen.getByLabelText('Password')).toBeInTheDocument();
expect(screen.getByRole('button', { name: 'Log in' })).toBeInTheDocument();
expect(screen.getByRole('status')).toHaveTextContent('Not connected. Enter a server address to continue.');
});
it('normalizes server address and emits login details', async () => {
@@ -46,4 +47,20 @@ describe('ServerLogin', () => {
expect(screen.getByText('Enter an account and password.')).toBeInTheDocument();
});
it.each([
['', 'Enter a server address.'],
['http:// bad-server', 'Enter a valid HTTP or HTTPS server address.'],
])('rejects an invalid server value: %s', async (server, error) => {
const onLogin = vi.fn();
render(ServerLogin, { props: { apiBase: 'http://localhost:8080', onLogin } });
await fireEvent.input(screen.getByLabelText('Server IP or domain'), { target: { value: server } });
await fireEvent.input(screen.getByLabelText('Email or username'), { target: { value: 'david@example.com' } });
await fireEvent.input(screen.getByLabelText('Password'), { target: { value: 'secret' } });
await fireEvent.click(screen.getByRole('button', { name: 'Log in' }));
expect(screen.getByText(error)).toBeInTheDocument();
expect(onLogin).not.toHaveBeenCalled();
});
});

View File

@@ -1,5 +1,5 @@
import '@testing-library/jest-dom/vitest';
import { fireEvent, render, screen } from '@testing-library/svelte';
import { fireEvent, render, screen, within } from '@testing-library/svelte';
import { describe, expect, it } from 'vitest';
import ChannelContent from './ChannelContent.svelte';
import { getProjectWorkspace } from './mockData';
@@ -7,15 +7,25 @@ import { getProjectWorkspace } from './mockData';
describe('ChannelContent', () => {
const workspace = getProjectWorkspace(1);
it('renders different templates for system and custom channels', () => {
for (const type of ['overview', 'inbox', 'tasks', 'ai_sessions', 'notes_sources', 'cron', 'custom_link'] as const) {
it('renders a distinctive heading or control for every channel type', () => {
const expectedContent = {
overview: { role: 'heading', name: 'Overview' },
inbox: { role: 'heading', name: 'Inbox' },
tasks: { role: 'checkbox', name: 'Mark Confirm homepage information architecture complete' },
ai_sessions: { role: 'heading', name: 'AI Sessions' },
notes_sources: { role: 'heading', name: 'Notes & Sources' },
cron: { role: 'heading', name: 'Cron Plans' },
custom_link: { role: 'link', name: 'Open external channel' },
} as const;
for (const type of Object.keys(expectedContent) as Array<keyof typeof expectedContent>) {
const channel = workspace.channels.find((item) => item.type === type);
if (!channel) throw new Error(`missing ${type}`);
render(ChannelContent, { props: { workspace, channel, onInspect: () => {} } });
const { unmount } = render(ChannelContent, { props: { workspace, channel, onInspect: () => {} } });
expect(screen.getByRole(expectedContent[type].role, { name: expectedContent[type].name })).toBeInTheDocument();
unmount();
}
expect(screen.getByText('Open external channel')).toBeInTheDocument();
});
it('sends selected task details to inspector', async () => {
@@ -36,4 +46,19 @@ describe('ChannelContent', () => {
await fireEvent.click(screen.getByRole('button', { name: 'Inspect Confirm homepage information architecture' }));
expect(inspectedTitle).toBe('Confirm homepage information architecture');
});
it('toggles a task completion state accessibly', async () => {
const channel = workspace.channels.find((item) => item.type === 'tasks');
if (!channel) throw new Error('missing task channel');
render(ChannelContent, { props: { workspace, channel, onInspect: () => {} } });
const completion = screen.getByRole('checkbox', { name: 'Mark Confirm homepage information architecture complete' });
expect(completion).not.toBeChecked();
await fireEvent.click(completion);
expect(completion).toBeChecked();
expect(within(completion.closest('article') as HTMLElement).getByText('Completed')).toBeInTheDocument();
});
});

View File

@@ -7,6 +7,18 @@
export let tags: string[];
export let recentSessions: AISessionItem[];
export let onSelectChannel: (channelID: string) => void;
const channelIcons: Record<WorkbenchChannel['icon'], string> = {
home: '\u2302',
mail: '\u2709',
list: '\u2630',
sparkles: '\u2726',
file: '\u25A4',
clock: '\u25F7',
link: '\u21AA',
};
$: sortedChannels = [...channels].sort((left, right) => left.sortOrder - right.sortOrder);
</script>
<aside class="channel-sidebar" aria-label="Project channels">
@@ -15,23 +27,24 @@
<p>Current project</p>
<h2>{project.name}</h2>
</div>
<button aria-label="Project settings">&#9881;</button>
<button aria-label="Project settings" disabled>&#9881;</button>
</header>
<section class="tag-row" aria-label="Project tags">
<ul class="tag-row" aria-label="Project tags">
{#each tags as tag}
<button type="button">#{tag}</button>
<li>#{tag}</li>
{/each}
</section>
</ul>
<section class="channel-group">
{#each channels as channel}
{#each sortedChannels as channel}
<button
class:active={channel.id === selectedChannelID}
aria-pressed={channel.id === selectedChannelID}
aria-label={`${channel.title}${channel.count ? ` ${channel.count}` : ''}`}
on:click={() => onSelectChannel(channel.id)}
>
<span class="channel-icon" aria-hidden="true">{channelIcons[channel.icon]}</span>
<span>{channel.title}</span>
{#if channel.count !== undefined}
<small>{channel.count}</small>
@@ -42,11 +55,13 @@
<section class="recent-sessions" aria-label="Recent sessions">
<h3>Recent sessions</h3>
{#each recentSessions as session}
<button type="button">
<span>{session.title}</span>
<small>{session.updatedAt}</small>
</button>
{/each}
<ul>
{#each recentSessions as session}
<li>
<span>{session.title}</span>
<small>{session.updatedAt}</small>
</li>
{/each}
</ul>
</section>
</aside>

View File

@@ -7,7 +7,7 @@
</script>
<nav class="project-rail" aria-label="Project list">
<button class="rail-logo" aria-label="Dashboard">SA</button>
<button class="rail-logo" aria-label="Dashboard" disabled>SA</button>
{#each projects as project}
<button
class:active={project.id === selectedProjectID}
@@ -22,5 +22,5 @@
{/if}
</button>
{/each}
<button class="add-project" aria-label="Create project">+</button>
<button class="add-project" aria-label="Create project" disabled>+</button>
</nav>

View File

@@ -16,10 +16,12 @@
$: selectedChannel = workspace.channels.find((channel) => channel.id === selectedChannelID) ?? workspace.channels[0];
function selectProject(projectID: number) {
selectedItem = null;
selectedProjectID = projectID;
}
function selectChannel(channelID: string) {
selectedItem = null;
selectedChannelID = channelID;
}

View File

@@ -51,4 +51,22 @@ describe('ProjectWorkbench', () => {
expect(within(inspector).getByText('Owner')).toBeInTheDocument();
expect(within(inspector).getByText('David')).toBeInTheDocument();
});
it('clears the inspector when the project or channel changes', async () => {
render(ProjectWorkbench, { props: { currentUser: { account: 'david@example.com' } } });
const inspector = screen.getByLabelText('Object inspector');
await fireEvent.click(screen.getByRole('button', { name: 'Work Plan 8' }));
await fireEvent.click(screen.getByRole('button', { name: 'Inspect Confirm homepage information architecture' }));
expect(within(inspector).getByRole('heading', { name: 'Confirm homepage information architecture' })).toBeInTheDocument();
await fireEvent.click(screen.getByRole('button', { name: 'Project A2' }));
expect(within(inspector).getByRole('heading', { name: 'Inspector' })).toBeInTheDocument();
expect(within(inspector).queryByRole('heading', { name: 'Confirm homepage information architecture' })).not.toBeInTheDocument();
await fireEvent.click(screen.getByRole('button', { name: 'Work Plan 8' }));
await fireEvent.click(screen.getByRole('button', { name: 'Inspect Confirm homepage information architecture' }));
await fireEvent.click(screen.getByRole('button', { name: 'Message Flow 36' }));
expect(within(inspector).getByRole('heading', { name: 'Inspector' })).toBeInTheDocument();
});
});

View File

@@ -9,8 +9,8 @@
<input aria-label="Global search" placeholder="Search projects, tasks, notes, AI sessions" />
</label>
<div class="topbar-actions">
<button aria-label="Back">&#8592;</button>
<button aria-label="Forward">&#8594;</button>
<button aria-label={`Account ${account}`}>{account.slice(0, 2).toUpperCase()}</button>
<button aria-label="Back" disabled>&#8592;</button>
<button aria-label="Forward" disabled>&#8594;</button>
<button aria-label={`Account ${account}`} disabled>{account.slice(0, 2).toUpperCase()}</button>
</div>
</header>

View File

@@ -2,15 +2,24 @@
import type { WorkbenchChannel } from '../types';
export let channel: WorkbenchChannel;
let copyStatus = '';
function copyUrl() {
void navigator.clipboard?.writeText(channel.url ?? '');
async function copyUrl() {
try {
await navigator.clipboard?.writeText(channel.url ?? '');
copyStatus = 'URL copied.';
} catch {
copyStatus = 'Copy unavailable in this browser.';
}
}
</script>
<section class="channel-page custom-link-channel">
<header class="channel-header"><p>External resource</p><h1>{channel.title}</h1></header>
<p>{channel.url}</p>
<a href={channel.url} target="_blank" rel="noreferrer">Open external channel</a>
<button type="button" on:click={copyUrl}>Copy URL</button>
<a class="custom-link" href={channel.url} target="_blank" rel="noreferrer">Open external channel</a>
<button class="copy-url-button" type="button" on:click={copyUrl}>Copy URL</button>
{#if copyStatus}
<p class="copy-feedback" aria-live="polite">{copyStatus}</p>
{/if}
</section>

View File

@@ -4,18 +4,41 @@
export let tasks: WorkTask[];
export let onInspect: (item: InspectorItem) => void;
let taskStates: WorkTask[] = [];
let sourceTasks: WorkTask[] | undefined;
$: if (tasks !== sourceTasks) {
sourceTasks = tasks;
taskStates = tasks.map((task) => ({ ...task }));
}
function inspectTask(task: WorkTask) {
onInspect({ title: task.title, type: 'Task', description: task.summary, properties: [{ label: 'Status', value: task.completed ? 'Completed' : 'Open' }, { label: 'Owner', value: task.owner }, { label: 'Due', value: task.due }, { label: 'Tag', value: task.tag }] });
}
function toggleTask(taskID: string) {
taskStates = taskStates.map((task) => task.id === taskID ? { ...task, completed: !task.completed } : task);
}
</script>
<section class="channel-page tasks-channel">
<header class="channel-header"><p>Work Plan</p><h1>Tasks</h1></header>
<div class="task-list">
{#each tasks as task}
{#each taskStates as task}
<article class:completed={task.completed} class="task-card">
<span class="task-check" aria-hidden="true">{task.completed ? 'Done' : 'Open'}</span>
<div><h2>{task.title}</h2><p>{task.summary}</p><small>{task.owner} · {task.due} · #{task.tag}</small></div>
<input
class="task-check"
type="checkbox"
aria-label={`Mark ${task.title} complete`}
checked={task.completed}
on:change={() => toggleTask(task.id)}
/>
<div>
<h2>{task.title}</h2>
<p>{task.summary}</p>
<small>{task.owner} / {task.due} / #{task.tag}</small>
<span class="task-status">{task.completed ? 'Completed' : 'Open'}</span>
</div>
<button aria-label={`Inspect ${task.title}`} on:click={() => inspectTask(task)}>Inspect</button>
</article>
{/each}

View File

@@ -20,7 +20,8 @@
body { margin: 0; min-width: 320px; min-height: 100vh; }
button, input, textarea, select { font: inherit; }
button { cursor: pointer; }
button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); outline-offset: 2px; }
button:focus-visible, input:focus-visible, a:focus-visible { outline: 2px solid var(--primary); outline-offset: 2px; }
button:disabled { cursor: not-allowed; opacity: 0.55; }
.login-page {
min-height: 100vh;
@@ -65,6 +66,20 @@ button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); o
padding: 9px 12px;
}
.connection-status,
.login-note,
.form-error,
.channel-header p,
.channel-page small,
.channel-page > p,
.object-inspector > p {
margin: 0;
color: var(--muted);
font-size: 0.875rem;
}
.form-error { color: var(--danger); }
.workbench-shell {
min-height: 100vh;
display: grid;
@@ -81,6 +96,12 @@ button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); o
padding: 0 16px;
}
.brand-mark { font-weight: 700; }
.search-box { display: grid; gap: 4px; min-width: 0; }
.search-box span { color: var(--muted); font-size: 0.75rem; }
.topbar-actions { display: flex; gap: 8px; }
.topbar-actions button { width: 34px; height: 34px; border: 1px solid var(--border); border-radius: 6px; background: var(--panel); }
.workbench-body {
min-height: 0;
display: grid;
@@ -116,6 +137,8 @@ button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); o
background: #eff6ff;
}
.project-rail small { color: var(--muted); font-size: 0.6875rem; }
.channel-sidebar,
.object-inspector,
.channel-stage {
@@ -124,18 +147,23 @@ button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); o
}
.channel-group,
.recent-sessions,
.task-list,
.record-list {
.record-list,
.message-list,
.file-list,
.cron-list {
display: grid;
gap: 8px;
}
.channel-group button,
.recent-sessions button,
.task-card,
.record-row,
.cron-row {
.cron-row,
.message-row,
.file-row,
.conversation-summary,
.metric-card {
width: 100%;
border: 1px solid var(--border);
border-radius: var(--radius);
@@ -145,9 +173,11 @@ button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); o
.task-card,
.record-row,
.cron-row {
.cron-row,
.message-row,
.file-row {
display: grid;
grid-template-columns: auto 1fr auto;
grid-template-columns: auto minmax(0, 1fr) auto;
gap: 12px;
align-items: start;
}
@@ -157,9 +187,72 @@ button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); o
background: var(--panel-muted);
}
.task-check { width: 18px; height: 18px; margin: 3px 0 0; accent-color: var(--primary); }
.task-status { display: inline-block; margin-top: 6px; color: var(--muted); font-size: 0.75rem; }
.channel-sidebar header { display: flex; align-items: start; justify-content: space-between; gap: 12px; }
.channel-sidebar header p,
.channel-sidebar h2,
.channel-sidebar h3 { margin: 0; }
.channel-sidebar h2 { font-size: 1rem; }
.channel-sidebar h3 { color: var(--muted); font-size: 0.8125rem; font-weight: 600; }
.channel-sidebar header button { width: 32px; height: 32px; border: 1px solid var(--border); border-radius: 6px; background: var(--panel-muted); }
.tag-row,
.recent-sessions ul { display: flex; flex-wrap: wrap; gap: 6px; margin: 0; padding: 0; list-style: none; }
.tag-row li { border: 1px solid var(--border); border-radius: 999px; padding: 3px 7px; color: var(--muted); font-size: 0.75rem; }
.channel-group button { display: grid; grid-template-columns: 18px minmax(0, 1fr) auto; gap: 8px; align-items: center; text-align: left; }
.channel-group small { color: var(--muted); }
.channel-icon { color: var(--muted); text-align: center; }
.recent-sessions { display: grid; gap: 8px; }
.recent-sessions li { display: grid; gap: 2px; min-width: 0; padding: 7px 8px; border: 1px solid var(--border); border-radius: 6px; background: var(--panel-muted); }
.recent-sessions span { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; font-size: 0.8125rem; }
.recent-sessions small { color: var(--muted); font-size: 0.6875rem; }
.channel-stage { background: var(--background); }
.channel-page { align-content: start; }
.channel-header { border-bottom: 1px solid var(--border); padding-bottom: 12px; }
.channel-header h1,
.channel-page h2 { margin: 0; }
.channel-header h1 { font-size: 1.25rem; }
.channel-page h2 { font-size: 0.9375rem; }
.channel-page h2 + p { margin: 4px 0; }
.metric-list { display: grid; grid-template-columns: repeat(auto-fit, minmax(140px, 1fr)); gap: 10px; }
.metric-card { display: grid; gap: 4px; text-align: left; }
.metric-card strong { font-size: 1.5rem; }
.metric-card span { color: var(--muted); font-size: 0.8125rem; }
.message-row { grid-template-columns: minmax(0, 1fr) auto; align-items: center; }
.file-row { grid-template-columns: auto minmax(0, 1fr) auto; align-items: center; }
.file-row > span { min-width: 40px; border: 1px solid var(--border); border-radius: 4px; padding: 3px 5px; color: var(--muted); font-size: 0.6875rem; text-align: center; text-transform: uppercase; }
.message-row p,
.cron-row p,
.conversation-summary p { margin: 4px 0; }
.message-row button,
.file-row button,
.task-card > button,
.cron-row button,
.conversation-summary button,
.copy-url-button,
.object-inspector button { border: 1px solid var(--border); border-radius: 6px; background: var(--panel); padding: 6px 9px; }
.session-layout { display: grid; grid-template-columns: minmax(180px, 0.7fr) minmax(0, 1.3fr); gap: 12px; }
.session-list { display: grid; align-content: start; gap: 6px; }
.session-list button { display: grid; gap: 3px; border: 1px solid var(--border); border-radius: 6px; background: var(--panel); padding: 9px; text-align: left; }
.session-list button.active { border-color: var(--primary); background: #eff6ff; }
.session-list small { color: var(--muted); }
.conversation-summary { align-content: start; }
.custom-link { color: var(--primary-strong); font-weight: 600; width: max-content; }
.copy-url-button { width: max-content; }
.copy-feedback { margin: 0; color: var(--success); font-size: 0.8125rem; }
.object-inspector {
align-content: start;
grid-auto-rows: min-content;
}
.inspector-tabs {
display: grid;
grid-template-columns: repeat(3, 1fr);
align-content: start;
grid-auto-rows: min-content;
gap: 4px;
}
@@ -170,17 +263,21 @@ button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); o
@media (max-width: 920px) {
.workspace-topbar {
grid-template-columns: 1fr;
grid-template-columns: minmax(0, 1fr) auto;
height: auto;
padding: 12px;
}
.search-box { grid-column: 1 / -1; grid-row: 2; }
.topbar-actions { grid-column: 2; grid-row: 1; }
.workbench-shell {
grid-template-rows: auto 1fr;
}
.workbench-body {
grid-template-columns: 1fr;
align-content: start;
}
.project-rail,
@@ -189,4 +286,40 @@ button:focus-visible, input:focus-visible { outline: 2px solid var(--primary); o
border-right: 0;
border-bottom: 1px solid var(--border);
}
.project-rail {
grid-template-columns: none;
grid-auto-flow: column;
grid-auto-columns: 48px;
align-items: center;
max-height: 76px;
overflow-x: auto;
overflow-y: hidden;
padding: 10px 12px;
}
.project-rail button { min-width: 48px; min-height: 48px; }
.channel-sidebar {
align-content: start;
max-height: 220px;
padding: 12px;
}
.channel-group,
.tag-row,
.recent-sessions ul {
grid-auto-flow: column;
grid-auto-columns: minmax(132px, max-content);
grid-template-columns: none;
flex-wrap: nowrap;
overflow-x: auto;
padding-bottom: 2px;
}
.channel-group button { min-width: 160px; }
.recent-sessions li { min-width: 160px; }
.channel-stage { max-height: 36vh; overflow: auto; }
.object-inspector { max-height: 288px; overflow: auto; }
.session-layout { grid-template-columns: 1fr; }
}