From 8180055be2bffe95223ee188f092886366f3dfc4 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 2 Mar 2026 17:33:11 -0800 Subject: [PATCH 1/2] fix: E2E test stability and UI performance improvements - Fix sandbox warning dialog blocking pointer events in E2E tests by dismissing it in agent-output-modal-responsive and success-log-contrast specs - Set skipSandboxWarning in test project setup localStorage cache - Add eslint-disable for empty destructuring pattern in feature-deep-link spec - Fix event-hooks-settings spec: wait for success toast before asserting dialog hidden, update empty state text, use Playwright .or() instead of Promise.race for locator matching - Remove stale eslint-disable comment in use-board-effects hook - Cap dev server log buffer at 50KB to prevent unbounded memory growth - Memoize line count calculation in DevServerLogsPanel to avoid repeated string splits on every render --- .../board-view/hooks/use-board-effects.ts | 1 - .../components/dev-server-logs-panel.tsx | 13 ++++++++++-- .../hooks/use-dev-server-logs.ts | 20 ++++++++++++++----- .../tests/features/feature-deep-link.spec.ts | 1 + .../agent-output-modal-responsive.spec.ts | 15 ++++++++++++++ .../features/success-log-contrast.spec.ts | 15 ++++++++++++++ .../settings/event-hooks-settings.spec.ts | 20 +++++++++---------- apps/ui/tests/utils/project/setup.ts | 1 + 8 files changed, 68 insertions(+), 18 deletions(-) diff --git a/apps/ui/src/components/views/board-view/hooks/use-board-effects.ts b/apps/ui/src/components/views/board-view/hooks/use-board-effects.ts index d697f2cb8..ae63d3876 100644 --- a/apps/ui/src/components/views/board-view/hooks/use-board-effects.ts +++ b/apps/ui/src/components/views/board-view/hooks/use-board-effects.ts @@ -119,7 +119,6 @@ export function useBoardEffects({ if (featuresFingerprint && !isLoading) { checkAllContexts(); } - // eslint-disable-next-line react-hooks/exhaustive-deps }, [featuresFingerprint, isLoading, checkContextExists, setFeaturesWithContext]); // Re-check context when a feature stops, completes, or errors diff --git a/apps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsx b/apps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsx index fd8f70562..cb51b451a 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsx +++ b/apps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useCallback, useState } from 'react'; +import { useEffect, useRef, useCallback, useState, useMemo } from 'react'; import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/dialog'; import { Button } from '@/components/ui/button'; import { @@ -123,10 +123,19 @@ export function DevServerLogsPanel({ } }, []); + const lineCount = useMemo(() => { + if (!logs) return 0; + // Count newlines directly instead of allocating a split array + let count = 1; + for (let i = 0; i < logs.length; i++) { + if (logs.charCodeAt(i) === 10) count++; + } + return count; + }, [logs]); + if (!worktree) return null; const formattedStartTime = formatStartedAt(startedAt); - const lineCount = logs ? logs.split('\n').length : 0; return ( !isOpen && onClose()}> diff --git a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts index 5847f5e6b..a836592ce 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts +++ b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts @@ -5,6 +5,9 @@ import { pathsEqual } from '@/lib/utils'; const logger = createLogger('DevServerLogs'); +// Maximum log buffer size (characters) - matches server-side MAX_SCROLLBACK_SIZE +const MAX_LOG_BUFFER_SIZE = 50_000; // ~50KB + export interface DevServerLogState { /** The log content (buffered + live) */ logs: string; @@ -136,13 +139,20 @@ export function useDevServerLogs({ worktreePath, autoSubscribe = true }: UseDevS }, []); /** - * Append content to logs + * Append content to logs, enforcing a maximum buffer size to prevent + * unbounded memory growth and progressive UI lag. */ const appendLogs = useCallback((content: string) => { - setState((prev) => ({ - ...prev, - logs: prev.logs + content, - })); + setState((prev) => { + let newLogs = prev.logs + content; + if (newLogs.length > MAX_LOG_BUFFER_SIZE) { + newLogs = newLogs.slice(-MAX_LOG_BUFFER_SIZE); + } + return { + ...prev, + logs: newLogs, + }; + }); }, []); // Fetch initial logs when worktreePath changes diff --git a/apps/ui/tests/features/feature-deep-link.spec.ts b/apps/ui/tests/features/feature-deep-link.spec.ts index 7083c2641..9c0fa398a 100644 --- a/apps/ui/tests/features/feature-deep-link.spec.ts +++ b/apps/ui/tests/features/feature-deep-link.spec.ts @@ -28,6 +28,7 @@ test.describe('Feature Deep Link', () => { let projectPath: string; let projectName: string; + // eslint-disable-next-line no-empty-pattern test.beforeEach(async ({}, testInfo) => { projectName = `test-project-${testInfo.workerIndex}-${Date.now()}`; projectPath = path.join(TEST_TEMP_DIR, projectName); diff --git a/apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts b/apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts index 0c0c6d02a..724d6f29b 100644 --- a/apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts +++ b/apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts @@ -100,6 +100,21 @@ test.describe('AgentOutputModal Responsive Behavior', () => { await expect(page.locator('[data-testid="board-view"]')).toBeVisible({ timeout: 10000 }); + // Dismiss sandbox warning dialog if it appears (blocks pointer events) + const sandboxAcceptBtn = page.locator('button:has-text("I Accept the Risks")'); + const sandboxVisible = await sandboxAcceptBtn + .waitFor({ state: 'visible', timeout: 2000 }) + .then(() => true) + .catch(() => false); + if (sandboxVisible) { + await sandboxAcceptBtn.click(); + await page + .locator('[role="dialog"][data-state="open"]') + .first() + .waitFor({ state: 'hidden', timeout: 3000 }) + .catch(() => {}); + } + // Wait for the verified feature card to appear const featureCard = page.locator(`[data-testid="kanban-card-${featureId}"]`); await expect(featureCard).toBeVisible({ timeout: 10000 }); diff --git a/apps/ui/tests/features/success-log-contrast.spec.ts b/apps/ui/tests/features/success-log-contrast.spec.ts index 9f16c51e0..d374085d6 100644 --- a/apps/ui/tests/features/success-log-contrast.spec.ts +++ b/apps/ui/tests/features/success-log-contrast.spec.ts @@ -109,6 +109,21 @@ test.describe('Success log output contrast', () => { await expect(page.locator('[data-testid="board-view"]')).toBeVisible({ timeout: 10000 }); + // Dismiss sandbox warning dialog if it appears (blocks pointer events) + const sandboxAcceptBtn = page.locator('button:has-text("I Accept the Risks")'); + const sandboxVisible = await sandboxAcceptBtn + .waitFor({ state: 'visible', timeout: 2000 }) + .then(() => true) + .catch(() => false); + if (sandboxVisible) { + await sandboxAcceptBtn.click(); + await page + .locator('[role="dialog"][data-state="open"]') + .first() + .waitFor({ state: 'hidden', timeout: 3000 }) + .catch(() => {}); + } + // Wait for the verified feature card to appear const featureCard = page.locator(`[data-testid="kanban-card-${featureId}"]`); await expect(featureCard).toBeVisible({ timeout: 10000 }); diff --git a/apps/ui/tests/settings/event-hooks-settings.spec.ts b/apps/ui/tests/settings/event-hooks-settings.spec.ts index 474dfda48..b589071e1 100644 --- a/apps/ui/tests/settings/event-hooks-settings.spec.ts +++ b/apps/ui/tests/settings/event-hooks-settings.spec.ts @@ -12,7 +12,7 @@ */ import { test, expect, type Page } from '@playwright/test'; -import { authenticateForTests, navigateToSettings } from '../utils'; +import { authenticateForTests, navigateToSettings, waitForSuccessToast } from '../utils'; // Timeout constants for maintainability const TIMEOUTS = { @@ -224,6 +224,9 @@ test.describe('Event Hooks Settings', () => { const addButton = dialog.locator('button:has-text("Add Endpoint")').last(); await addButton.click(); + // Wait for the success toast to confirm the save completed (including API call) + await waitForSuccessToast(page, 'Endpoint added', { timeout: 10000 }); + // Dialog should close await expect(dialog).toBeHidden({ timeout: TIMEOUTS.dialogHidden }); @@ -256,16 +259,13 @@ test.describe('Event Hooks Settings', () => { // The endpoints tab should show either existing endpoints or the empty state // The key is that it should NOT show "empty" if there are endpoints on the server - // Either we see "No endpoints configured" OR we see endpoint cards - const emptyState = page.locator('text=No endpoints configured'); + // Either we see "No ntfy endpoints configured" OR we see endpoint cards + const emptyState = page.locator('text=No ntfy endpoints configured'); const endpointCard = page.locator('[data-testid="endpoint-card"]').first(); - // One of these should be visible - await expect( - Promise.race([ - emptyState.waitFor({ state: 'visible', timeout: 5000 }).then(() => 'empty'), - endpointCard.waitFor({ state: 'visible', timeout: 5000 }).then(() => 'card'), - ]) - ).resolves.toBeDefined(); + // One of these should be visible (use Playwright's .or() to match either locator) + await expect(emptyState.or(endpointCard)).toBeVisible({ + timeout: TIMEOUTS.endpointVisible, + }); }); }); diff --git a/apps/ui/tests/utils/project/setup.ts b/apps/ui/tests/utils/project/setup.ts index 2d307f9bc..532d0d525 100644 --- a/apps/ui/tests/utils/project/setup.ts +++ b/apps/ui/tests/utils/project/setup.ts @@ -242,6 +242,7 @@ export async function setupRealProject( theme: 'dark', sidebarOpen: true, maxConcurrency: 3, + skipSandboxWarning: true, }; localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache)); From a9a0ba9ed96924329d142c9d1ce1553c8eaed44e Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 2 Mar 2026 18:22:35 -0800 Subject: [PATCH 2/2] fix: address PR review feedback - Slice log buffer at newline boundaries to avoid cutting lines in half - Add logsVersion/didTrim metadata to DevServerLogState so the panel's incremental writer detects buffer shifts and triggers a full rewrite - Extract sandbox warning dismissal into shared dismissSandboxWarningIfVisible() helper in test utils to reduce duplication --- .../components/dev-server-logs-panel.tsx | 9 +++++--- .../hooks/use-dev-server-logs.ts | 21 ++++++++++++++++--- .../agent-output-modal-responsive.spec.ts | 15 ++----------- .../features/success-log-contrast.spec.ts | 15 ++----------- apps/ui/tests/utils/components/dialogs.ts | 21 +++++++++++++++++++ 5 files changed, 49 insertions(+), 32 deletions(-) diff --git a/apps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsx b/apps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsx index cb51b451a..5c2ef48d8 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsx +++ b/apps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsx @@ -54,6 +54,8 @@ export function DevServerLogsPanel({ const { logs, + logsVersion, + didTrim, isRunning, isLoading, error, @@ -81,8 +83,9 @@ export function DevServerLogsPanel({ return; } - // If logs got shorter (e.g., cleared), rewrite all - if (logs.length < lastLogsLengthRef.current) { + // If logs got shorter (e.g., cleared) or buffer was trimmed (content shifted), + // do a full rewrite so the terminal stays in sync + if (logs.length < lastLogsLengthRef.current || didTrim) { xtermRef.current.write(logs); lastLogsLengthRef.current = logs.length; return; @@ -94,7 +97,7 @@ export function DevServerLogsPanel({ xtermRef.current.append(newContent); lastLogsLengthRef.current = logs.length; } - }, [logs, worktree?.path]); + }, [logs, logsVersion, didTrim, worktree?.path]); // Reset when panel opens with a new worktree useEffect(() => { diff --git a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts index a836592ce..2d5dd8abb 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts +++ b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts @@ -11,6 +11,10 @@ const MAX_LOG_BUFFER_SIZE = 50_000; // ~50KB export interface DevServerLogState { /** The log content (buffered + live) */ logs: string; + /** Incremented whenever logs content changes (including trim+shift) */ + logsVersion: number; + /** True when the latest append caused head truncation */ + didTrim: boolean; /** Whether the server is currently running */ isRunning: boolean; /** Whether initial logs are being fetched */ @@ -55,6 +59,8 @@ interface UseDevServerLogsOptions { export function useDevServerLogs({ worktreePath, autoSubscribe = true }: UseDevServerLogsOptions) { const [state, setState] = useState({ logs: '', + logsVersion: 0, + didTrim: false, isRunning: false, isLoading: false, error: null, @@ -126,6 +132,8 @@ export function useDevServerLogs({ worktreePath, autoSubscribe = true }: UseDevS const clearLogs = useCallback(() => { setState({ logs: '', + logsVersion: 0, + didTrim: false, isRunning: false, isLoading: false, error: null, @@ -144,13 +152,20 @@ export function useDevServerLogs({ worktreePath, autoSubscribe = true }: UseDevS */ const appendLogs = useCallback((content: string) => { setState((prev) => { - let newLogs = prev.logs + content; - if (newLogs.length > MAX_LOG_BUFFER_SIZE) { - newLogs = newLogs.slice(-MAX_LOG_BUFFER_SIZE); + const combined = prev.logs + content; + const didTrim = combined.length > MAX_LOG_BUFFER_SIZE; + let newLogs = combined; + if (didTrim) { + const slicePoint = combined.length - MAX_LOG_BUFFER_SIZE; + // Find the next newline after the slice point to avoid cutting a line in half + const firstNewlineIndex = combined.indexOf('\n', slicePoint); + newLogs = combined.slice(firstNewlineIndex > -1 ? firstNewlineIndex + 1 : slicePoint); } return { ...prev, logs: newLogs, + didTrim, + logsVersion: prev.logsVersion + 1, }; }); }, []); diff --git a/apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts b/apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts index 724d6f29b..8b48685bf 100644 --- a/apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts +++ b/apps/ui/tests/features/responsive/agent-output-modal-responsive.spec.ts @@ -13,6 +13,7 @@ import { waitForNetworkIdle, authenticateForTests, handleLoginScreenIfPresent, + dismissSandboxWarningIfVisible, } from '../../utils'; const TEST_TEMP_DIR = createTempDirPath('responsive-modal-test'); @@ -101,19 +102,7 @@ test.describe('AgentOutputModal Responsive Behavior', () => { await expect(page.locator('[data-testid="board-view"]')).toBeVisible({ timeout: 10000 }); // Dismiss sandbox warning dialog if it appears (blocks pointer events) - const sandboxAcceptBtn = page.locator('button:has-text("I Accept the Risks")'); - const sandboxVisible = await sandboxAcceptBtn - .waitFor({ state: 'visible', timeout: 2000 }) - .then(() => true) - .catch(() => false); - if (sandboxVisible) { - await sandboxAcceptBtn.click(); - await page - .locator('[role="dialog"][data-state="open"]') - .first() - .waitFor({ state: 'hidden', timeout: 3000 }) - .catch(() => {}); - } + await dismissSandboxWarningIfVisible(page); // Wait for the verified feature card to appear const featureCard = page.locator(`[data-testid="kanban-card-${featureId}"]`); diff --git a/apps/ui/tests/features/success-log-contrast.spec.ts b/apps/ui/tests/features/success-log-contrast.spec.ts index d374085d6..07e5f3ca6 100644 --- a/apps/ui/tests/features/success-log-contrast.spec.ts +++ b/apps/ui/tests/features/success-log-contrast.spec.ts @@ -13,6 +13,7 @@ import { waitForNetworkIdle, authenticateForTests, handleLoginScreenIfPresent, + dismissSandboxWarningIfVisible, } from '../utils'; /** @@ -110,19 +111,7 @@ test.describe('Success log output contrast', () => { await expect(page.locator('[data-testid="board-view"]')).toBeVisible({ timeout: 10000 }); // Dismiss sandbox warning dialog if it appears (blocks pointer events) - const sandboxAcceptBtn = page.locator('button:has-text("I Accept the Risks")'); - const sandboxVisible = await sandboxAcceptBtn - .waitFor({ state: 'visible', timeout: 2000 }) - .then(() => true) - .catch(() => false); - if (sandboxVisible) { - await sandboxAcceptBtn.click(); - await page - .locator('[role="dialog"][data-state="open"]') - .first() - .waitFor({ state: 'hidden', timeout: 3000 }) - .catch(() => {}); - } + await dismissSandboxWarningIfVisible(page); // Wait for the verified feature card to appear const featureCard = page.locator(`[data-testid="kanban-card-${featureId}"]`); diff --git a/apps/ui/tests/utils/components/dialogs.ts b/apps/ui/tests/utils/components/dialogs.ts index 059c67c05..57f9a31d8 100644 --- a/apps/ui/tests/utils/components/dialogs.ts +++ b/apps/ui/tests/utils/components/dialogs.ts @@ -2,6 +2,27 @@ import { Page, Locator } from '@playwright/test'; import { clickElement } from '../core/interactions'; import { waitForElement, waitForElementHidden } from '../core/waiting'; +/** + * Dismiss the sandbox warning dialog if it appears. + * This dialog blocks pointer events and must be accepted before interacting + * with elements behind it. + */ +export async function dismissSandboxWarningIfVisible(page: Page): Promise { + const sandboxAcceptBtn = page.locator('button:has-text("I Accept the Risks")'); + const sandboxVisible = await sandboxAcceptBtn + .waitFor({ state: 'visible', timeout: 2000 }) + .then(() => true) + .catch(() => false); + if (sandboxVisible) { + await sandboxAcceptBtn.click(); + await page + .locator('[role="dialog"][data-state="open"]') + .first() + .waitFor({ state: 'hidden', timeout: 3000 }) + .catch(() => {}); + } +} + /** * Check if the add feature dialog is visible */