diff --git a/.superpowers/sdd/final-fix-3-report.md b/.superpowers/sdd/final-fix-3-report.md new file mode 100644 index 0000000..1d20233 --- /dev/null +++ b/.superpowers/sdd/final-fix-3-report.md @@ -0,0 +1,32 @@ +# Final Fix 3 Report + +## Status + +Completed the two final re-review fixes for the Discord-style project workbench UI. + +## Fixes + +- Made ObjectInspector and CustomLinkChannel clipboard handlers await `navigator.clipboard.writeText`, show success only after resolution, and show distinct unavailable or failure feedback otherwise. +- Added focused ObjectInspector coverage for delayed success, rejection, and a missing clipboard API, plus CustomLinkChannel unavailable-clipboard coverage. +- Tightened ServerLogin validation for malformed HTTP slash forms, credentials, missing/repeated/invalid hostname labels, whitespace including encoded whitespace, and unsupported protocols while preserving localhost, IPv4, domain, port, and protocol-less normalization. +- Added unit and Playwright coverage for the requested malformed server-address cases. + +## TDD Evidence + +1. Added the regression tests before production changes. +2. Ran the targeted test set and observed 10 expected failures: six malformed addresses were accepted and clipboard actions reported false success. +3. Implemented the minimal validation and asynchronous clipboard handling. +4. Re-ran the targeted test set successfully: 3 files, 26 tests passed. + +## Verification + +From `apps/web`: + +- `npm test -- --run` passed: 8 files, 40 tests. +- `npm run build` passed. +- `npx playwright test` passed: 6 tests. +- `git diff --check` passed. + +## Concerns + +None. Existing untracked SDD artifacts were left untouched. diff --git a/apps/web/e2e/project-workbench.spec.ts b/apps/web/e2e/project-workbench.spec.ts index 33818a3..55f6ac3 100644 --- a/apps/web/e2e/project-workbench.spec.ts +++ b/apps/web/e2e/project-workbench.spec.ts @@ -25,17 +25,19 @@ test('login and project channel workbench flow', async ({ page }) => { await expect(page.getByText('Open external channel')).toBeVisible(); }); -test('rejects malformed server addresses', async ({ page }) => { - await page.goto('/'); +for (const server of ['https://user:secret@example.com', 'http:////example.com', 'http://example..com', 'http://-bad.com']) { + test(`rejects malformed server address: ${server}`, async ({ page }) => { + await page.goto('/'); - await page.getByLabel('Server IP or domain').fill('https://user:secret@example.com'); - await page.getByLabel('Email or username').fill('david@example.com'); - await page.getByLabel('Password').fill('secret'); - await page.getByRole('button', { name: 'Log in' }).click(); + await page.getByLabel('Server IP or domain').fill(server); + await page.getByLabel('Email or username').fill('david@example.com'); + await page.getByLabel('Password').fill('secret'); + await page.getByRole('button', { name: 'Log in' }).click(); - await expect(page.getByText('Enter a valid HTTP or HTTPS server address.')).toBeVisible(); - await expect(page.getByLabel('Server IP or domain')).toBeVisible(); -}); + await expect(page.getByText('Enter a valid HTTP or HTTPS server address.')).toBeVisible(); + await expect(page.getByLabel('Server IP or domain')).toBeVisible(); + }); +} test('mobile workbench keeps project navigation and inspector compact', async ({ page }) => { await page.setViewportSize({ width: 390, height: 844 }); diff --git a/apps/web/src/features/auth/ServerLogin.svelte b/apps/web/src/features/auth/ServerLogin.svelte index 09b367d..32cc241 100644 --- a/apps/web/src/features/auth/ServerLogin.svelte +++ b/apps/web/src/features/auth/ServerLogin.svelte @@ -18,10 +18,19 @@ } const candidate = /^https?:\/\//i.test(value) ? value : `http://${value}`; + if (!/^https?:\/\/(?![\\/])/i.test(candidate) || hasCredentials(candidate)) { + return { error: 'Enter a valid HTTP or HTTPS server address.' }; + } try { const parsed = new URL(candidate); - if (!parsed.hostname || parsed.username || parsed.password || !['http:', 'https:'].includes(parsed.protocol)) { + if ( + !parsed.hostname || + parsed.username || + parsed.password || + !['http:', 'https:'].includes(parsed.protocol) || + !isValidHostname(parsed.hostname) + ) { return { error: 'Enter a valid HTTP or HTTPS server address.' }; } return { apiBase: candidate }; @@ -49,6 +58,21 @@ return /^[a-z][a-z\d+.-]*:(?!\d)/i.test(value); } + function hasCredentials(value: string): boolean { + const authority = value.replace(/^https?:\/\//i, '').split(/[/?#]/, 1)[0]; + return authority.includes('@'); + } + + function isValidHostname(hostname: string): boolean { + if (hostname === 'localhost') return true; + if (/^\[[\da-f:.]+\]$/i.test(hostname)) return true; + if (/^\d+(?:\.\d+){3}$/.test(hostname)) { + return hostname.split('.').every((segment) => Number(segment) <= 255); + } + + return hostname.split('.').every((label) => /^[a-z\d](?:[a-z\d-]{0,61}[a-z\d])?$/i.test(label)); + } + function submitLogin() { error = ''; const normalized = normalizeServer(server); diff --git a/apps/web/src/features/auth/ServerLogin.test.ts b/apps/web/src/features/auth/ServerLogin.test.ts index 73e8c4a..0343ba7 100644 --- a/apps/web/src/features/auth/ServerLogin.test.ts +++ b/apps/web/src/features/auth/ServerLogin.test.ts @@ -40,6 +40,24 @@ describe('ServerLogin', () => { }); }); + it.each([ + ['localhost', 'http://localhost'], + ['localhost:8080', 'http://localhost:8080'], + ['192.168.1.20:3000', 'http://192.168.1.20:3000'], + ['example.com:8443', 'http://example.com:8443'], + ['https://example.com', 'https://example.com'], + ])('accepts and normalizes a valid server value: %s', async (server, apiBase) => { + 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(onLogin).toHaveBeenCalledWith({ apiBase, account: 'david@example.com' }); + }); + it('shows an error when account or password is missing', async () => { render(ServerLogin, { props: { apiBase: 'http://localhost:8080' } }); @@ -57,6 +75,12 @@ describe('ServerLogin', () => { ['https://user:secret@example.com', 'Enter a valid HTTP or HTTPS server address.'], ['http://', 'Enter a valid HTTP or HTTPS server address.'], ['ftp://example.com', 'Enter a valid HTTP or HTTPS server address.'], + ['http:////example.com', 'Enter a valid HTTP or HTTPS server address.'], + ['http://example..com', 'Enter a valid HTTP or HTTPS server address.'], + ['http://-bad.com', 'Enter a valid HTTP or HTTPS server address.'], + ['http://bad-.com', 'Enter a valid HTTP or HTTPS server address.'], + ['http://bad_host.com', 'Enter a valid HTTP or HTTPS server address.'], + ['http://bad!.com', '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 } }); diff --git a/apps/web/src/features/workbench/ObjectInspector.svelte b/apps/web/src/features/workbench/ObjectInspector.svelte index 9232111..6fbe503 100644 --- a/apps/web/src/features/workbench/ObjectInspector.svelte +++ b/apps/web/src/features/workbench/ObjectInspector.svelte @@ -4,17 +4,31 @@ export let item: InspectorItem | null; let activeTab: 'discussion' | 'properties' | 'more' = 'discussion'; let copyFeedback = ''; + let copyFailed = false; let currentItem: InspectorItem | null = null; $: if (item !== currentItem) { currentItem = item; copyFeedback = ''; + copyFailed = false; } - function copyLink() { + async function copyLink() { const link = new URL(`?item=${encodeURIComponent(item?.title ?? '')}`, window.location.href).toString(); - void navigator.clipboard?.writeText(link); - copyFeedback = 'Link copied to clipboard.'; + if (!navigator.clipboard?.writeText) { + copyFailed = true; + copyFeedback = 'Clipboard access is unavailable.'; + return; + } + + try { + await navigator.clipboard.writeText(link); + copyFailed = false; + copyFeedback = 'Link copied to clipboard.'; + } catch { + copyFailed = true; + copyFeedback = 'Unable to copy link.'; + } } @@ -42,7 +56,7 @@ {:else} {#if copyFeedback} -

{copyFeedback}

+

{copyFeedback}

{/if} {/if} diff --git a/apps/web/src/features/workbench/ObjectInspector.test.ts b/apps/web/src/features/workbench/ObjectInspector.test.ts new file mode 100644 index 0000000..b586131 --- /dev/null +++ b/apps/web/src/features/workbench/ObjectInspector.test.ts @@ -0,0 +1,73 @@ +import '@testing-library/jest-dom/vitest'; +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/svelte'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import ObjectInspector from './ObjectInspector.svelte'; + +const item = { + title: 'Review navigation', + type: 'Task', + description: 'Check the project navigation flow.', + properties: [], +}; +const originalClipboard = Object.getOwnPropertyDescriptor(navigator, 'clipboard'); + +function setClipboard(writeText: Clipboard['writeText']) { + Object.defineProperty(navigator, 'clipboard', { + configurable: true, + value: { writeText } as Clipboard, + }); +} + +afterEach(() => { + cleanup(); + if (originalClipboard) { + Object.defineProperty(navigator, 'clipboard', originalClipboard); + } else { + Reflect.deleteProperty(navigator, 'clipboard'); + } +}); + +describe('ObjectInspector', () => { + async function openMoreActions() { + await fireEvent.click(screen.getByRole('button', { name: 'More' })); + } + + it('shows copy success only after clipboard writing resolves', async () => { + let resolveWrite: (() => void) | undefined; + const writeText = vi.fn( + () => + new Promise((resolve) => { + resolveWrite = resolve; + }), + ); + setClipboard(writeText); + render(ObjectInspector, { props: { item } }); + + await openMoreActions(); + await fireEvent.click(screen.getByRole('button', { name: 'Copy link' })); + + expect(screen.queryByRole('status')).not.toBeInTheDocument(); + resolveWrite?.(); + await waitFor(() => expect(screen.getByRole('status')).toHaveTextContent('Link copied to clipboard.')); + }); + + it('reports a copy failure when clipboard writing rejects', async () => { + setClipboard(vi.fn().mockRejectedValue(new Error('Permission denied'))); + render(ObjectInspector, { props: { item } }); + + await openMoreActions(); + await fireEvent.click(screen.getByRole('button', { name: 'Copy link' })); + + await waitFor(() => expect(screen.getByRole('status')).toHaveTextContent('Unable to copy link.')); + }); + + it('reports unavailable clipboard access without showing success', async () => { + Reflect.deleteProperty(navigator, 'clipboard'); + render(ObjectInspector, { props: { item } }); + + await openMoreActions(); + await fireEvent.click(screen.getByRole('button', { name: 'Copy link' })); + + expect(screen.getByRole('status')).toHaveTextContent('Clipboard access is unavailable.'); + }); +}); diff --git a/apps/web/src/features/workbench/ProjectWorkbench.test.ts b/apps/web/src/features/workbench/ProjectWorkbench.test.ts index 0247755..1fda28b 100644 --- a/apps/web/src/features/workbench/ProjectWorkbench.test.ts +++ b/apps/web/src/features/workbench/ProjectWorkbench.test.ts @@ -1,6 +1,6 @@ import '@testing-library/jest-dom/vitest'; -import { fireEvent, render, screen, within } from '@testing-library/svelte'; -import { describe, expect, it } from 'vitest'; +import { fireEvent, render, screen, waitFor, within } from '@testing-library/svelte'; +import { describe, expect, it, vi } from 'vitest'; import ProjectWorkbench from './ProjectWorkbench.svelte'; describe('ProjectWorkbench', () => { @@ -88,6 +88,11 @@ describe('ProjectWorkbench', () => { }); it('provides Copy link feedback and disables unavailable archive actions', async () => { + const originalClipboard = Object.getOwnPropertyDescriptor(navigator, 'clipboard'); + Object.defineProperty(navigator, 'clipboard', { + configurable: true, + value: { writeText: vi.fn().mockResolvedValue(undefined) } as Clipboard, + }); render(ProjectWorkbench, { props: { currentUser: { account: 'david@example.com' } } }); const inspector = screen.getByLabelText('Object inspector'); @@ -96,7 +101,13 @@ describe('ProjectWorkbench', () => { await fireEvent.click(within(inspector).getByRole('button', { name: 'More' })); await fireEvent.click(within(inspector).getByRole('button', { name: 'Copy link' })); - expect(within(inspector).getByRole('status')).toHaveTextContent('Link copied to clipboard.'); + await waitFor(() => expect(within(inspector).getByRole('status')).toHaveTextContent('Link copied to clipboard.')); expect(within(inspector).getByRole('button', { name: 'Archive is unavailable in this preview' })).toBeDisabled(); + + if (originalClipboard) { + Object.defineProperty(navigator, 'clipboard', originalClipboard); + } else { + Reflect.deleteProperty(navigator, 'clipboard'); + } }); }); diff --git a/apps/web/src/features/workbench/channels/CustomLinkChannel.svelte b/apps/web/src/features/workbench/channels/CustomLinkChannel.svelte index 0457feb..57a3ff2 100644 --- a/apps/web/src/features/workbench/channels/CustomLinkChannel.svelte +++ b/apps/web/src/features/workbench/channels/CustomLinkChannel.svelte @@ -3,13 +3,22 @@ export let channel: WorkbenchChannel; let copyStatus = ''; + let copyFailed = false; async function copyUrl() { + if (!navigator.clipboard?.writeText) { + copyFailed = true; + copyStatus = 'Copy unavailable in this browser.'; + return; + } + try { - await navigator.clipboard?.writeText(channel.url ?? ''); + await navigator.clipboard.writeText(channel.url ?? ''); + copyFailed = false; copyStatus = 'URL copied.'; } catch { - copyStatus = 'Copy unavailable in this browser.'; + copyFailed = true; + copyStatus = 'Unable to copy URL.'; } } @@ -20,6 +29,6 @@ Open external channel {#if copyStatus} -

{copyStatus}

+

{copyStatus}

{/if} diff --git a/apps/web/src/features/workbench/channels/CustomLinkChannel.test.ts b/apps/web/src/features/workbench/channels/CustomLinkChannel.test.ts new file mode 100644 index 0000000..8f8e845 --- /dev/null +++ b/apps/web/src/features/workbench/channels/CustomLinkChannel.test.ts @@ -0,0 +1,36 @@ +import '@testing-library/jest-dom/vitest'; +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/svelte'; +import { afterEach, describe, expect, it } from 'vitest'; +import CustomLinkChannel from './CustomLinkChannel.svelte'; + +const channel = { + id: 'project-custom-link', + projectID: 1, + type: 'custom_link' as const, + title: 'Roadmap Board', + icon: 'link', + url: 'https://example.com/roadmap', + sortOrder: 1, +}; +const originalClipboard = Object.getOwnPropertyDescriptor(navigator, 'clipboard'); + +afterEach(() => { + cleanup(); + if (originalClipboard) { + Object.defineProperty(navigator, 'clipboard', originalClipboard); + } else { + Reflect.deleteProperty(navigator, 'clipboard'); + } +}); + +describe('CustomLinkChannel', () => { + it('reports unavailable clipboard access without showing URL copied', async () => { + Reflect.deleteProperty(navigator, 'clipboard'); + render(CustomLinkChannel, { props: { channel } }); + + await fireEvent.click(screen.getByRole('button', { name: 'Copy URL' })); + + await waitFor(() => expect(screen.getByText('Copy unavailable in this browser.')).toBeInTheDocument()); + expect(screen.queryByText('URL copied.')).not.toBeInTheDocument(); + }); +}); diff --git a/apps/web/src/index.css b/apps/web/src/index.css index cfad6a2..ea3f080 100644 --- a/apps/web/src/index.css +++ b/apps/web/src/index.css @@ -253,6 +253,7 @@ button:disabled { cursor: not-allowed; opacity: 0.55; } .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; } +.copy-feedback.copy-error { color: var(--danger); } .object-inspector { align-content: start;