From fce8dbc37fc6cb131f7149b58f278654cf4c14d9 Mon Sep 17 00:00:00 2001 From: Misaka Date: Sat, 4 Apr 2026 14:00:48 +0800 Subject: [PATCH] feat(logging-p0): add screenshot capture and browser console diagnostics for ERP errors Enhance ERP automation error diagnostics by capturing PNG screenshots on every error and forwarding browser console warnings/errors to the structured logger. Includes automatic cleanup of old screenshots aligned with the configured log retention period. Co-Authored-By: Claude Opus 4.6 --- src/main/services/erp/erp-auth.ts | 3 + src/main/services/erp/erp-error-context.ts | 51 +++++ src/main/services/erp/page-diagnostics.ts | 50 +++++ src/main/services/logger/index.ts | 5 +- src/main/services/logger/shared.ts | 32 +++ tests/unit/logger-integration.test.ts | 83 +++++++- .../services/erp/erp-error-context.test.ts | 133 +++++++++++++ .../services/erp/page-diagnostics.test.ts | 184 ++++++++++++++++++ 8 files changed, 539 insertions(+), 2 deletions(-) create mode 100644 src/main/services/erp/page-diagnostics.ts create mode 100644 tests/unit/services/erp/erp-error-context.test.ts create mode 100644 tests/unit/services/erp/page-diagnostics.test.ts diff --git a/src/main/services/erp/erp-auth.ts b/src/main/services/erp/erp-auth.ts index 609dbf8..5895791 100644 --- a/src/main/services/erp/erp-auth.ts +++ b/src/main/services/erp/erp-auth.ts @@ -2,6 +2,7 @@ import { chromium } from 'playwright' import type { ErpConfig, ErpSession } from '../../types/erp.types' import { createLogger } from '../logger' import { capturePageContext } from './erp-error-context' +import { attachPageDiagnostics, attachContextDiagnostics } from './page-diagnostics' const log = createLogger('ErpAuthService') @@ -55,6 +56,8 @@ export class ErpAuthService { }) const page = await context.newPage() + attachPageDiagnostics(page) + attachContextDiagnostics(context) // Navigate to login page (use actual login URL from Python code) const loginUrl = `${this.config.url}/yonbip/resources/uap/rbac/login/main/index.html` diff --git a/src/main/services/erp/erp-error-context.ts b/src/main/services/erp/erp-error-context.ts index ca65fde..0a948c5 100644 --- a/src/main/services/erp/erp-error-context.ts +++ b/src/main/services/erp/erp-error-context.ts @@ -6,12 +6,61 @@ */ import type { Page } from 'playwright' +import fs from 'fs' +import path from 'path' +import { getLogDir } from '../logger/shared' export interface ErpErrorContext { pageUrl?: string frameHierarchy?: Array<{ name: string; url: string }> targetSelector?: string step?: string + screenshotPath?: string +} + +/** + * Sanitize a step name for use as a filename component. + * Replaces non-alphanumeric characters with underscores and truncates. + */ +function sanitizeForFilename(step: string | undefined): string { + if (!step) return 'unknown' + return step.replace(/[^a-zA-Z0-9_-]/g, '_').slice(0, 40) +} + +/** + * Capture a screenshot of the page for error diagnostics. + * Stored as PNG under /screenshots/. + * Defensive: never throws. + */ +async function captureScreenshot(page: Page, step?: string): Promise { + try { + if (page.isClosed()) return undefined + + const screenshotDir = path.join(getLogDir(), 'screenshots') + fs.mkdirSync(screenshotDir, { recursive: true }) + + const now = new Date() + const timestamp = [ + now.getFullYear(), + String(now.getMonth() + 1).padStart(2, '0'), + String(now.getDate()).padStart(2, '0'), + '_', + String(now.getHours()).padStart(2, '0'), + String(now.getMinutes()).padStart(2, '0'), + String(now.getSeconds()).padStart(2, '0') + ].join('') + + const filename = `err_${timestamp}_${sanitizeForFilename(step)}.png` + const filePath = path.join(screenshotDir, filename) + + const buffer = await page.screenshot({ type: 'png', timeout: 5000 }) + fs.writeFileSync(filePath, buffer) + + return filePath + } catch { + // screenshot failure must not propagate + return undefined + } } /** @@ -49,5 +98,7 @@ export async function capturePageContext( ctx.step = step } + ctx.screenshotPath = await captureScreenshot(page, step) + return ctx } diff --git a/src/main/services/erp/page-diagnostics.ts b/src/main/services/erp/page-diagnostics.ts new file mode 100644 index 0000000..d4f28bd --- /dev/null +++ b/src/main/services/erp/page-diagnostics.ts @@ -0,0 +1,50 @@ +/** + * Page Diagnostics + * + * Attaches browser console and error listeners to Playwright pages + * so that ERP-side JS errors are visible in the application log. + * + * Only warning and error level console messages are captured — + * ERP (YonBIP) outputs large volumes of info-level messages that + * would drown the log. + */ + +import type { Page, BrowserContext } from 'playwright' +import { createLogger } from '../logger' + +const log = createLogger('BrowserDiagnostics') + +/** + * Attach console and error listeners to a single page. + */ +export function attachPageDiagnostics(page: Page): void { + page.on('console', (msg) => { + const type = msg.type() + if (type !== 'warning' && type !== 'error') return + + const location = msg.location() + log.error(`[Browser ${type}] ${msg.text()}`, { + pageUrl: page.url(), + consoleType: type, + location: location ? `${location.url}:${location.lineNumber}` : undefined + }) + }) + + page.on('pageerror', (error) => { + log.error(`[Browser pageerror] ${error.message}`, { + pageUrl: page.url(), + error: error.message, + stack: error.stack + }) + }) +} + +/** + * Attach diagnostics to all current and future pages in a browser context. + * Covers popups and pages opened by ERP automation. + */ +export function attachContextDiagnostics(context: BrowserContext): void { + context.on('page', (page) => { + attachPageDiagnostics(page) + }) +} diff --git a/src/main/services/logger/index.ts b/src/main/services/logger/index.ts index 8bd3ceb..4f001d7 100644 --- a/src/main/services/logger/index.ts +++ b/src/main/services/logger/index.ts @@ -13,7 +13,7 @@ import DailyRotateFile from 'winston-daily-rotate-file' import path from 'path' import { BrowserWindow } from 'electron' import { serializeError, sanitizeError } from './error-utils' -import { getLogDir, isProduction } from './shared' +import { getLogDir, isProduction, cleanupOldScreenshots } from './shared' import { IPC_CHANNELS } from '../../../shared/ipc-channels' import { getContext, run } from './request-context' @@ -212,6 +212,9 @@ export function applyLoggingConfig(config: { level: string; appRetention: number }) ) } + + // Clean up old screenshot files beyond the retention window + cleanupOldScreenshots(config.appRetention) } /** diff --git a/src/main/services/logger/shared.ts b/src/main/services/logger/shared.ts index bc14b27..82807b6 100644 --- a/src/main/services/logger/shared.ts +++ b/src/main/services/logger/shared.ts @@ -58,3 +58,35 @@ export const LOG_LEVEL_PRIORITY: Record = { export function isLoggable(level: string, threshold: string): boolean { return (LOG_LEVEL_PRIORITY[level] ?? 0) >= (LOG_LEVEL_PRIORITY[threshold] ?? 2) } + +/** + * Delete screenshot files older than the given retention period. + * Scans /screenshots/ and removes .png files whose mtime exceeds + * the retention window. Defensive: never throws. + * + * @param retentionDays - Number of days to keep screenshots + */ +export function cleanupOldScreenshots(retentionDays: number): void { + try { + const screenshotDir = path.join(getLogDir(), 'screenshots') + if (!fs.existsSync(screenshotDir)) return + + const files = fs.readdirSync(screenshotDir) + const cutoff = Date.now() - retentionDays * 24 * 60 * 60 * 1000 + + for (const file of files) { + if (!file.endsWith('.png')) continue + const filePath = path.join(screenshotDir, file) + try { + const stat = fs.statSync(filePath) + if (stat.mtimeMs < cutoff) { + fs.unlinkSync(filePath) + } + } catch { + // individual file deletion failure should not stop the loop + } + } + } catch { + // entire cleanup is best-effort + } +} diff --git a/tests/unit/logger-integration.test.ts b/tests/unit/logger-integration.test.ts index 15563bc..ac7d635 100644 --- a/tests/unit/logger-integration.test.ts +++ b/tests/unit/logger-integration.test.ts @@ -3,7 +3,10 @@ * Verifies RequestContext is properly integrated with Logger */ -import { describe, it, expect } from 'vitest' +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import fs from 'fs' +import path from 'path' +import { getLogDir } from '../../src/main/services/logger/shared' describe('Logger RequestContext Integration', () => { it('should export run from request-context', async () => { @@ -52,3 +55,81 @@ describe('Logger RequestContext Integration', () => { expect(loggerModule.createLogger).toBeDefined() }) }) + +describe('cleanupOldScreenshots', () => { + let unlinkSyncSpy: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('should delete PNG files older than retention period', async () => { + const { cleanupOldScreenshots } = await import('../../src/main/services/logger/shared') + const screenshotDir = path.join(getLogDir(), 'screenshots') + + const now = Date.now() + const oldTime = now - 20 * 24 * 60 * 60 * 1000 // 20 days ago + const recentTime = now - 5 * 24 * 60 * 60 * 1000 // 5 days ago + + vi.spyOn(fs, 'existsSync').mockImplementation((p) => { + if (typeof p === 'string' && p.includes('screenshots')) return true + return false + }) + + vi.spyOn(fs, 'readdirSync').mockReturnValue(['err_old.png', 'err_recent.png'] as any) + + vi.spyOn(fs, 'statSync').mockImplementation((p) => { + if (typeof p === 'string' && p.includes('err_old')) { + return { mtimeMs: oldTime } as any + } + return { mtimeMs: recentTime } as any + }) + + unlinkSyncSpy = vi.spyOn(fs, 'unlinkSync').mockReturnValue(undefined) + + cleanupOldScreenshots(14) + + // Only the old file should be deleted + expect(unlinkSyncSpy).toHaveBeenCalledTimes(1) + expect(unlinkSyncSpy).toHaveBeenCalledWith(path.join(screenshotDir, 'err_old.png')) + }) + + it('should not delete anything if screenshots directory does not exist', async () => { + const { cleanupOldScreenshots } = await import('../../src/main/services/logger/shared') + + vi.spyOn(fs, 'existsSync').mockReturnValue(false) + const readdirSyncSpy = vi.spyOn(fs, 'readdirSync') + + cleanupOldScreenshots(14) + + expect(readdirSyncSpy).not.toHaveBeenCalled() + }) + + it('should skip non-PNG files', async () => { + const { cleanupOldScreenshots } = await import('../../src/main/services/logger/shared') + const screenshotDir = path.join(getLogDir(), 'screenshots') + + const now = Date.now() + const oldTime = now - 20 * 24 * 60 * 60 * 1000 + + vi.spyOn(fs, 'existsSync').mockImplementation((p) => { + if (typeof p === 'string' && p.includes('screenshots')) return true + return false + }) + + vi.spyOn(fs, 'readdirSync').mockReturnValue(['notes.txt', 'data.json', 'err_old.png'] as any) + + vi.spyOn(fs, 'statSync').mockReturnValue({ mtimeMs: oldTime } as any) + unlinkSyncSpy = vi.spyOn(fs, 'unlinkSync').mockReturnValue(undefined) + + cleanupOldScreenshots(14) + + // Only the .png file should be deleted + expect(unlinkSyncSpy).toHaveBeenCalledTimes(1) + expect(unlinkSyncSpy).toHaveBeenCalledWith(path.join(screenshotDir, 'err_old.png')) + }) +}) diff --git a/tests/unit/services/erp/erp-error-context.test.ts b/tests/unit/services/erp/erp-error-context.test.ts new file mode 100644 index 0000000..65fd710 --- /dev/null +++ b/tests/unit/services/erp/erp-error-context.test.ts @@ -0,0 +1,133 @@ +/** + * Tests for ERP Error Context Capture with screenshot support + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import fs from 'fs' +import path from 'path' + +// Mock logger/shared before importing the module under test +vi.mock('../../../../src/main/services/logger/shared', () => ({ + getLogDir: vi.fn(() => '/tmp/test-logs') +})) + +// Mock fs +vi.mock('fs', () => ({ + default: { + mkdirSync: vi.fn(), + writeFileSync: vi.fn(), + existsSync: vi.fn(() => true), + readdirSync: vi.fn(() => []), + statSync: vi.fn() + } +})) + +import { capturePageContext } from '../../../../src/main/services/erp/erp-error-context' + +function createMockPage(overrides: Record = {}) { + return { + url: vi.fn(() => 'https://erp.example.com/page'), + frames: vi.fn(() => [ + { name: () => 'main', url: () => 'https://erp.example.com/main' }, + { name: () => 'forwardFrame', url: () => 'https://erp.example.com/frame' } + ]), + isClosed: vi.fn(() => false), + screenshot: vi.fn(() => Buffer.from('fake-png-data')), + ...overrides + } as any +} + +describe('capturePageContext', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(fs.mkdirSync).mockReturnValue(undefined) + vi.mocked(fs.writeFileSync).mockReturnValue(undefined) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('should capture screenshot and return screenshotPath on success', async () => { + const page = createMockPage() + + const ctx = await capturePageContext(page, '#selector', 'login.username') + + expect(ctx.screenshotPath).toBeDefined() + expect(ctx.screenshotPath).toContain('err_') + expect(ctx.screenshotPath).toContain('.png') + expect(ctx.screenshotPath).toContain('login_username') + expect(fs.writeFileSync).toHaveBeenCalledWith( + expect.stringContaining('.png'), + expect.any(Buffer) + ) + }) + + it('should return screenshotPath undefined when page is closed', async () => { + const page = createMockPage({ isClosed: vi.fn(() => true) }) + + const ctx = await capturePageContext(page) + + expect(ctx.screenshotPath).toBeUndefined() + expect(fs.writeFileSync).not.toHaveBeenCalled() + }) + + it('should still return other fields when screenshot fails', async () => { + const page = createMockPage({ + screenshot: vi.fn(() => { + throw new Error('screenshot timeout') + }) + }) + + const ctx = await capturePageContext(page, '#target', 'test-step') + + // Other fields should still be populated + expect(ctx.pageUrl).toBe('https://erp.example.com/page') + expect(ctx.frameHierarchy).toHaveLength(2) + expect(ctx.targetSelector).toBe('#target') + expect(ctx.step).toBe('test-step') + // Screenshot should be undefined due to failure + expect(ctx.screenshotPath).toBeUndefined() + }) + + it('should sanitize special characters in step name for filename', async () => { + const page = createMockPage() + + const ctx = await capturePageContext(page, undefined, 'login/user:test*step') + + expect(ctx.screenshotPath).toBeDefined() + expect(ctx.screenshotPath).toContain('login_user_test_step') + }) + + it('should use "unknown" in filename when step is not provided', async () => { + const page = createMockPage() + + const ctx = await capturePageContext(page) + + expect(ctx.screenshotPath).toBeDefined() + expect(ctx.screenshotPath).toContain('unknown') + }) + + it('should truncate long step names to 40 characters', async () => { + const page = createMockPage() + const longStep = 'a'.repeat(80) + + const ctx = await capturePageContext(page, undefined, longStep) + + expect(ctx.screenshotPath).toBeDefined() + const filename = path.basename(ctx.screenshotPath!) + // Extract step portion: err_YYYYMMDD_HHmmss_.png + const stepPart = filename.replace(/^err_\d{8}_\d{6}_/, '').replace(/\.png$/, '') + expect(stepPart.length).toBe(40) + }) + + it('should create screenshots directory if it does not exist', async () => { + const page = createMockPage() + + await capturePageContext(page) + + expect(fs.mkdirSync).toHaveBeenCalledWith(expect.stringContaining('screenshots'), { + recursive: true + }) + }) +}) diff --git a/tests/unit/services/erp/page-diagnostics.test.ts b/tests/unit/services/erp/page-diagnostics.test.ts new file mode 100644 index 0000000..1bb1655 --- /dev/null +++ b/tests/unit/services/erp/page-diagnostics.test.ts @@ -0,0 +1,184 @@ +/** + * Tests for Page Diagnostics module + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest' + +// vi.hoisted() runs before vi.mock() factories, even though both are hoisted. +const mockLog = vi.hoisted(() => ({ + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn() +})) + +vi.mock('../../../../src/main/services/logger', () => ({ + createLogger: () => mockLog +})) + +import { + attachPageDiagnostics, + attachContextDiagnostics +} from '../../../../src/main/services/erp/page-diagnostics' + +describe('page-diagnostics', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + describe('attachPageDiagnostics', () => { + it('should log console warning messages', () => { + const listeners: Record any)[]> = {} + const page = { + on: vi.fn((event: string, handler: (...args: any[]) => any) => { + if (!listeners[event]) listeners[event] = [] + listeners[event].push(handler) + }), + url: vi.fn(() => 'https://erp.example.com/page') + } as any + + attachPageDiagnostics(page) + + const consoleHandlers = listeners['console'] ?? [] + expect(consoleHandlers).toHaveLength(1) + + consoleHandlers[0]({ + type: () => 'warning', + text: () => 'Something suspicious', + location: () => ({ url: 'app.js', lineNumber: 42 }) + }) + + expect(mockLog.error).toHaveBeenCalledWith( + expect.stringContaining('Browser warning'), + expect.objectContaining({ + pageUrl: 'https://erp.example.com/page', + consoleType: 'warning' + }) + ) + }) + + it('should log console error messages', () => { + const listeners: Record any)[]> = {} + const page = { + on: vi.fn((event: string, handler: (...args: any[]) => any) => { + if (!listeners[event]) listeners[event] = [] + listeners[event].push(handler) + }), + url: vi.fn(() => 'https://erp.example.com/page') + } as any + + attachPageDiagnostics(page) + + const consoleHandlers = listeners['console'] ?? [] + consoleHandlers[0]({ + type: () => 'error', + text: () => 'Uncaught TypeError', + location: () => ({ url: 'vendor.js', lineNumber: 100 }) + }) + + expect(mockLog.error).toHaveBeenCalledWith( + expect.stringContaining('Browser error'), + expect.objectContaining({ + consoleType: 'error' + }) + ) + }) + + it('should NOT log console info messages', () => { + const listeners: Record any)[]> = {} + const page = { + on: vi.fn((event: string, handler: (...args: any[]) => any) => { + if (!listeners[event]) listeners[event] = [] + listeners[event].push(handler) + }), + url: vi.fn(() => 'https://erp.example.com/page') + } as any + + attachPageDiagnostics(page) + + const consoleHandlers = listeners['console'] ?? [] + consoleHandlers[0]({ + type: () => 'info', + text: () => 'XHR loaded', + location: () => ({ url: 'app.js', lineNumber: 10 }) + }) + + expect(mockLog.error).not.toHaveBeenCalled() + }) + + it('should NOT log console log messages', () => { + const listeners: Record any)[]> = {} + const page = { + on: vi.fn((event: string, handler: (...args: any[]) => any) => { + if (!listeners[event]) listeners[event] = [] + listeners[event].push(handler) + }), + url: vi.fn(() => 'https://erp.example.com/page') + } as any + + attachPageDiagnostics(page) + + const consoleHandlers = listeners['console'] ?? [] + consoleHandlers[0]({ + type: () => 'log', + text: () => 'debug output', + location: () => null + }) + + expect(mockLog.error).not.toHaveBeenCalled() + }) + + it('should log pageerror events', () => { + const listeners: Record any)[]> = {} + const page = { + on: vi.fn((event: string, handler: (...args: any[]) => any) => { + if (!listeners[event]) listeners[event] = [] + listeners[event].push(handler) + }), + url: vi.fn(() => 'https://erp.example.com/page') + } as any + + attachPageDiagnostics(page) + + const pageErrorHandlers = listeners['pageerror'] ?? [] + expect(pageErrorHandlers).toHaveLength(1) + + pageErrorHandlers[0](new Error('Script error on page')) + + expect(mockLog.error).toHaveBeenCalledWith( + expect.stringContaining('Browser pageerror'), + expect.objectContaining({ + pageUrl: 'https://erp.example.com/page', + error: 'Script error on page' + }) + ) + }) + }) + + describe('attachContextDiagnostics', () => { + it('should auto-attach diagnostics to new pages in the context', () => { + const contextListeners: Record any)[]> = {} + const context = { + on: vi.fn((event: string, handler: (...args: any[]) => any) => { + if (!contextListeners[event]) contextListeners[event] = [] + contextListeners[event].push(handler) + }) + } as any + + attachContextDiagnostics(context) + + const pageHandlers = contextListeners['page'] ?? [] + expect(pageHandlers).toHaveLength(1) + + const newPage = { + on: vi.fn(), + url: vi.fn(() => 'https://erp.example.com/popup') + } as any + + pageHandlers[0](newPage) + + expect(newPage.on).toHaveBeenCalledWith('console', expect.any(Function)) + expect(newPage.on).toHaveBeenCalledWith('pageerror', expect.any(Function)) + }) + }) +})