diff --git a/docs/plans/2025-10-26-notification-ui-improvements.md b/docs/plans/2025-10-26-notification-ui-improvements.md new file mode 100644 index 0000000..ab1f891 --- /dev/null +++ b/docs/plans/2025-10-26-notification-ui-improvements.md @@ -0,0 +1,717 @@ +# Notification UI Improvements Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Fix three UX issues in the MCP notification system: unclear message format, settings collapse on toggle, and broken modal filters. + +**Architecture:** Targeted surgical fixes - update notification message format in NotificationManager, implement targeted DOM updates in settings tab to prevent collapse, refactor modal filters to use Obsidian Setting components and eliminate destructive re-renders. + +**Tech Stack:** TypeScript, Obsidian API (Notice, Setting, Modal), Jest for testing + +--- + +## Task 1: Update Notification Message Format + +**Files:** +- Modify: `src/ui/notifications.ts:77-94` (showToolCall method) +- Modify: `src/ui/notifications.ts:140-177` (formatArgs method) +- Test: `tests/notifications.test.ts` (new file) + +**Step 1: Write failing test for new notification format** + +Create `tests/notifications.test.ts`: + +```typescript +import { App } from 'obsidian'; +import { NotificationManager } from '../src/ui/notifications'; +import { MCPPluginSettings } from '../src/types/settings-types'; + +describe('NotificationManager', () => { + let app: App; + let settings: MCPPluginSettings; + let manager: NotificationManager; + + beforeEach(() => { + app = {} as App; + settings = { + port: 3000, + autoStart: false, + apiKey: 'test-key', + notificationsEnabled: true, + showParameters: true, + notificationDuration: 3000, + logToConsole: false + }; + manager = new NotificationManager(app, settings); + }); + + describe('showToolCall', () => { + it('should format message with MCP Tool Called label and newline when parameters shown', () => { + const mockNotice = jest.fn(); + (global as any).Notice = mockNotice; + + manager.showToolCall('read_note', { path: 'daily/2025-01-15.md' }); + + expect(mockNotice).toHaveBeenCalledWith( + expect.stringContaining('📖 MCP Tool Called: read_note\npath: "daily/2025-01-15.md"'), + 3000 + ); + }); + + it('should format message without newline when parameters hidden', () => { + settings.showParameters = false; + manager = new NotificationManager(app, settings); + const mockNotice = jest.fn(); + (global as any).Notice = mockNotice; + + manager.showToolCall('read_note', { path: 'daily/2025-01-15.md' }); + + expect(mockNotice).toHaveBeenCalledWith( + '📖 MCP Tool Called: read_note', + 3000 + ); + }); + + it('should format multiple parameters correctly', () => { + const mockNotice = jest.fn(); + (global as any).Notice = mockNotice; + + manager.showToolCall('search', { + query: 'test query', + folder: 'notes', + recursive: true + }); + + expect(mockNotice).toHaveBeenCalledWith( + expect.stringContaining('🔍 MCP Tool Called: search\nquery: "test query", folder: "notes", recursive: true'), + 3000 + ); + }); + }); +}); +``` + +**Step 2: Run test to verify it fails** + +Run: `npm test -- tests/notifications.test.ts` +Expected: FAIL with test failures showing old format `🔧 MCP: read_note` + +**Step 3: Update formatArgs to return unwrapped parameter string** + +In `src/ui/notifications.ts`, modify the `formatArgs` method (lines 140-177): + +```typescript +/** + * Format arguments for display + */ +private formatArgs(args: any): string { + if (!this.settings.showParameters) { + return ''; + } + + if (!args || Object.keys(args).length === 0) { + return ''; + } + + try { + // Extract key parameters for display + const keyParams: string[] = []; + + if (args.path) { + keyParams.push(`path: "${this.truncateString(args.path, 30)}"`); + } + if (args.query) { + keyParams.push(`query: "${this.truncateString(args.query, 30)}"`); + } + if (args.folder) { + keyParams.push(`folder: "${this.truncateString(args.folder, 30)}"`); + } + if (args.recursive !== undefined) { + keyParams.push(`recursive: ${args.recursive}`); + } + + // If no key params, show first 50 chars of JSON + if (keyParams.length === 0) { + const json = JSON.stringify(args); + return this.truncateString(json, 50); + } + + return keyParams.join(', '); + } catch (e) { + return ''; + } +} +``` + +**Step 4: Update showToolCall to use new format** + +In `src/ui/notifications.ts`, modify the `showToolCall` method (lines 77-94): + +```typescript +/** + * Show notification for tool call start + */ +showToolCall(toolName: string, args: any, duration?: number): void { + if (!this.shouldShowNotification()) { + return; + } + + const icon = TOOL_ICONS[toolName] || '🔧'; + const argsStr = this.formatArgs(args); + const message = argsStr + ? `${icon} MCP Tool Called: ${toolName}\n${argsStr}` + : `${icon} MCP Tool Called: ${toolName}`; + + this.queueNotification(() => { + new Notice(message, duration || this.settings.notificationDuration); + }); + + // Log to console if enabled + if (this.settings.logToConsole) { + console.log(`[MCP] Tool call: ${toolName}`, args); + } +} +``` + +**Step 5: Run tests to verify they pass** + +Run: `npm test -- tests/notifications.test.ts` +Expected: PASS - all notification format tests pass + +**Step 6: Run full test suite** + +Run: `npm test` +Expected: PASS - all 569+ tests pass + +**Step 7: Commit** + +```bash +git add src/ui/notifications.ts tests/notifications.test.ts +git commit -m "feat: improve notification message clarity with MCP Tool Called label + +- Update notification format to multi-line with explicit label +- First line: 'MCP Tool Called: tool_name' +- Second line: parameters (if enabled) +- Add comprehensive tests for notification formatting + +Co-Authored-By: Claude " +``` + +--- + +## Task 2: Fix Settings Section Collapse on Toggle + +**Files:** +- Modify: `src/settings.ts:6-12` (add instance variable) +- Modify: `src/settings.ts:198-272` (notification section rendering) +- Test: Manual testing (UI component, hard to unit test) + +**Step 1: Add instance variable for notification details element** + +In `src/settings.ts`, add to the class properties (after line 7): + +```typescript +export class MCPServerSettingTab extends PluginSettingTab { + plugin: MCPServerPlugin; + private notificationDetailsEl: HTMLDetailsElement | null = null; + + constructor(app: App, plugin: MCPServerPlugin) { + super(app, plugin); + this.plugin = plugin; + } +``` + +**Step 2: Store reference when creating notification section** + +In `src/settings.ts`, modify the notification section creation (around line 199): + +```typescript +// Notification Settings +const notifDetails = containerEl.createEl('details'); +notifDetails.style.marginBottom = '20px'; +const notifSummary = notifDetails.createEl('summary'); +notifSummary.style.fontSize = '1.17em'; +notifSummary.style.fontWeight = 'bold'; +notifSummary.style.marginBottom = '12px'; +notifSummary.style.cursor = 'pointer'; +notifSummary.setText('UI Notifications'); + +// Store reference for targeted updates +this.notificationDetailsEl = notifDetails; +``` + +**Step 3: Create updateNotificationSection helper method** + +In `src/settings.ts`, add new method after the `display()` method: + +```typescript +/** + * Update only the notification section without re-rendering entire page + */ +private updateNotificationSection(): void { + if (!this.notificationDetailsEl) { + // Fallback to full re-render if reference lost + this.display(); + return; + } + + // Store current open state + const wasOpen = this.notificationDetailsEl.open; + + // Find and remove all child elements except the summary + const summary = this.notificationDetailsEl.querySelector('summary'); + while (this.notificationDetailsEl.lastChild && this.notificationDetailsEl.lastChild !== summary) { + this.notificationDetailsEl.removeChild(this.notificationDetailsEl.lastChild); + } + + // Rebuild notification settings + if (this.plugin.settings.notificationsEnabled) { + // Show parameters + new Setting(this.notificationDetailsEl) + .setName('Show parameters') + .setDesc('Include tool parameters in notifications') + .addToggle(toggle => toggle + .setValue(this.plugin.settings.showParameters) + .onChange(async (value) => { + this.plugin.settings.showParameters = value; + await this.plugin.saveSettings(); + this.plugin.updateNotificationManager(); + })); + + // Notification duration + new Setting(this.notificationDetailsEl) + .setName('Notification duration') + .setDesc('Duration in milliseconds') + .addText(text => text + .setPlaceholder('3000') + .setValue(String(this.plugin.settings.notificationDuration)) + .onChange(async (value) => { + const duration = parseInt(value); + if (!isNaN(duration) && duration > 0) { + this.plugin.settings.notificationDuration = duration; + await this.plugin.saveSettings(); + this.plugin.updateNotificationManager(); + } + })); + + // Log to console + new Setting(this.notificationDetailsEl) + .setName('Log to console') + .setDesc('Log tool calls to console') + .addToggle(toggle => toggle + .setValue(this.plugin.settings.logToConsole) + .onChange(async (value) => { + this.plugin.settings.logToConsole = value; + await this.plugin.saveSettings(); + this.plugin.updateNotificationManager(); + })); + + // View history button + new Setting(this.notificationDetailsEl) + .setName('Notification history') + .setDesc('View recent MCP tool calls') + .addButton(button => button + .setButtonText('View History') + .onClick(() => { + this.plugin.showNotificationHistory(); + })); + } + + // Restore open state + this.notificationDetailsEl.open = wasOpen; +} +``` + +**Step 4: Update toggle handler to use targeted update** + +In `src/settings.ts`, modify the "Enable notifications" toggle handler (around line 214): + +```typescript +// Enable notifications +new Setting(notifDetails) + .setName('Enable notifications') + .setDesc('Show when MCP tools are called') + .addToggle(toggle => toggle + .setValue(this.plugin.settings.notificationsEnabled) + .onChange(async (value) => { + this.plugin.settings.notificationsEnabled = value; + await this.plugin.saveSettings(); + this.plugin.updateNotificationManager(); + this.updateNotificationSection(); // Changed from this.display() + })); +``` + +**Step 5: Manual testing** + +Manual test steps: +1. Build: `npm run build` +2. Copy `main.js`, `manifest.json`, `styles.css` to test vault +3. Reload Obsidian +4. Open plugin settings +5. Expand "UI Notifications" section +6. Toggle "Enable notifications" off - verify section stays open +7. Toggle "Enable notifications" on - verify section stays open +8. Verify subsettings appear/disappear correctly + +**Step 6: Commit** + +```bash +git add src/settings.ts +git commit -m "fix: prevent notification settings section from collapsing on toggle + +- Add targeted DOM update method for notification section +- Store reference to details element during initial render +- Replace full page re-render with targeted subsection update +- Preserve open/closed state during updates + +Co-Authored-By: Claude " +``` + +--- + +## Task 3: Fix Modal Filter Controls + +**Files:** +- Modify: `src/ui/notification-history.ts:7-16` (add DOM reference properties) +- Modify: `src/ui/notification-history.ts:19-35` (onOpen method) +- Modify: `src/ui/notification-history.ts:44-87` (createFilters method) +- Modify: `src/ui/notification-history.ts:89-161` (refactor list rendering) +- Modify: `src/ui/notification-history.ts:193-214` (applyFilters method) +- Test: Manual testing (Modal UI component) + +**Step 1: Add DOM reference instance variables** + +In `src/ui/notification-history.ts`, update class properties (lines 7-16): + +```typescript +export class NotificationHistoryModal extends Modal { + private history: NotificationHistoryEntry[]; + private filteredHistory: NotificationHistoryEntry[]; + private filterTool: string = ''; + private filterType: 'all' | 'success' | 'error' = 'all'; + + // DOM element references for targeted updates + private listContainerEl: HTMLElement | null = null; + private countEl: HTMLElement | null = null; + + constructor(app: App, history: NotificationHistoryEntry[]) { + super(app); + this.history = history; + this.filteredHistory = [...history]; + } +``` + +**Step 2: Simplify onOpen to avoid re-rendering** + +In `src/ui/notification-history.ts`, modify `onOpen` method (lines 19-35): + +```typescript +onOpen() { + const { contentEl } = this; + contentEl.empty(); + contentEl.addClass('mcp-notification-history-modal'); + + // Title + contentEl.createEl('h2', { text: 'MCP Notification History' }); + + // Filters (create once, never recreate) + this.createFilters(contentEl); + + // History list (will be updated via reference) + this.createHistoryListContainer(contentEl); + + // Actions + this.createActions(contentEl); +} +``` + +**Step 3: Refactor createFilters to use Setting components** + +In `src/ui/notification-history.ts`, replace the `createFilters` method (lines 44-87): + +```typescript +/** + * Create filter controls using Obsidian Setting components + */ +private createFilters(containerEl: HTMLElement): void { + const filterContainer = containerEl.createDiv({ cls: 'mcp-history-filters' }); + filterContainer.style.marginBottom = '16px'; + + // Tool name filter using Setting component + new Setting(filterContainer) + .setName('Tool filter') + .setDesc('Filter by tool name') + .addText(text => text + .setPlaceholder('Enter tool name...') + .setValue(this.filterTool) + .onChange((value) => { + this.filterTool = value.toLowerCase(); + this.applyFilters(); + })); + + // Type filter using Setting component + new Setting(filterContainer) + .setName('Status filter') + .setDesc('Filter by success or error') + .addDropdown(dropdown => dropdown + .addOption('all', 'All') + .addOption('success', 'Success') + .addOption('error', 'Error') + .setValue(this.filterType) + .onChange((value) => { + this.filterType = value as 'all' | 'success' | 'error'; + this.applyFilters(); + })); + + // Results count + this.countEl = filterContainer.createDiv({ cls: 'mcp-history-count' }); + this.countEl.style.marginTop = '8px'; + this.countEl.style.fontSize = '0.9em'; + this.countEl.style.color = 'var(--text-muted)'; + this.updateResultsCount(); +} +``` + +**Step 4: Create container and separate update method** + +In `src/ui/notification-history.ts`, replace `createHistoryList` with two methods: + +```typescript +/** + * Create history list container (called once) + */ +private createHistoryListContainer(containerEl: HTMLElement): void { + this.listContainerEl = containerEl.createDiv({ cls: 'mcp-history-list' }); + this.listContainerEl.style.maxHeight = '400px'; + this.listContainerEl.style.overflowY = 'auto'; + this.listContainerEl.style.marginBottom = '16px'; + this.listContainerEl.style.border = '1px solid var(--background-modifier-border)'; + this.listContainerEl.style.borderRadius = '4px'; + + // Initial render + this.updateHistoryList(); +} + +/** + * Update history list contents (called on filter changes) + */ +private updateHistoryList(): void { + if (!this.listContainerEl) return; + + // Clear existing content + this.listContainerEl.empty(); + + if (this.filteredHistory.length === 0) { + const emptyEl = this.listContainerEl.createDiv({ cls: 'mcp-history-empty' }); + emptyEl.style.padding = '24px'; + emptyEl.style.textAlign = 'center'; + emptyEl.style.color = 'var(--text-muted)'; + emptyEl.textContent = 'No entries found'; + return; + } + + this.filteredHistory.forEach((entry, index) => { + const entryEl = this.listContainerEl!.createDiv({ cls: 'mcp-history-entry' }); + entryEl.style.padding = '12px'; + entryEl.style.borderBottom = index < this.filteredHistory.length - 1 + ? '1px solid var(--background-modifier-border)' + : 'none'; + + // Header row + const headerEl = entryEl.createDiv({ cls: 'mcp-history-entry-header' }); + headerEl.style.display = 'flex'; + headerEl.style.justifyContent = 'space-between'; + headerEl.style.marginBottom = '8px'; + + // Tool name and status + const titleEl = headerEl.createDiv(); + const statusIcon = entry.success ? '✅' : '❌'; + const toolName = titleEl.createEl('strong', { text: `${statusIcon} ${entry.toolName}` }); + toolName.style.color = entry.success ? 'var(--text-success)' : 'var(--text-error)'; + + // Timestamp and duration + const metaEl = headerEl.createDiv(); + metaEl.style.fontSize = '0.85em'; + metaEl.style.color = 'var(--text-muted)'; + const timestamp = new Date(entry.timestamp).toLocaleTimeString(); + const durationStr = entry.duration ? ` • ${entry.duration}ms` : ''; + metaEl.textContent = `${timestamp}${durationStr}`; + + // Arguments + if (entry.args && Object.keys(entry.args).length > 0) { + const argsEl = entryEl.createDiv({ cls: 'mcp-history-entry-args' }); + argsEl.style.fontSize = '0.85em'; + argsEl.style.fontFamily = 'monospace'; + argsEl.style.backgroundColor = 'var(--background-secondary)'; + argsEl.style.padding = '8px'; + argsEl.style.borderRadius = '4px'; + argsEl.style.marginBottom = '8px'; + argsEl.style.overflowX = 'auto'; + argsEl.textContent = JSON.stringify(entry.args, null, 2); + } + + // Error message + if (!entry.success && entry.error) { + const errorEl = entryEl.createDiv({ cls: 'mcp-history-entry-error' }); + errorEl.style.fontSize = '0.85em'; + errorEl.style.color = 'var(--text-error)'; + errorEl.style.backgroundColor = 'var(--background-secondary)'; + errorEl.style.padding = '8px'; + errorEl.style.borderRadius = '4px'; + errorEl.style.fontFamily = 'monospace'; + errorEl.textContent = entry.error; + } + }); +} + +/** + * Update results count display + */ +private updateResultsCount(): void { + if (!this.countEl) return; + this.countEl.textContent = `${this.filteredHistory.length} of ${this.history.length} entries`; +} +``` + +**Step 5: Update applyFilters to use targeted updates** + +In `src/ui/notification-history.ts`, replace the `applyFilters` method (lines 193-214): + +```typescript +/** + * Apply filters to history + */ +private applyFilters(): void { + this.filteredHistory = this.history.filter(entry => { + // Tool name filter + if (this.filterTool && !entry.toolName.toLowerCase().includes(this.filterTool)) { + return false; + } + + // Type filter + if (this.filterType === 'success' && !entry.success) { + return false; + } + if (this.filterType === 'error' && entry.success) { + return false; + } + + return true; + }); + + // Update only the affected UI elements + this.updateHistoryList(); + this.updateResultsCount(); +} +``` + +**Step 6: Add missing import** + +In `src/ui/notification-history.ts`, update imports at the top: + +```typescript +import { App, Modal, Setting } from 'obsidian'; +import { NotificationHistoryEntry } from './notifications'; +``` + +**Step 7: Manual testing** + +Manual test steps: +1. Build: `npm run build` +2. Copy `main.js` to test vault +3. Reload Obsidian +4. Trigger some MCP tool calls (with different tools, some successes, some errors) +5. Open notification history modal +6. Test tool filter: + - Type "read" - verify only read_* tools show + - Clear filter - verify all entries return +7. Test status filter: + - Select "Success" - verify only successful calls show + - Select "Error" - verify only failed calls show + - Select "All" - verify all entries return +8. Test combined filters +9. Verify results count updates correctly + +**Step 8: Commit** + +```bash +git add src/ui/notification-history.ts +git commit -m "fix: repair broken filter controls in notification history modal + +- Replace raw HTML inputs with Obsidian Setting components +- Add DOM element references for targeted updates +- Eliminate destructive re-render on filter changes +- Update only list container and count on filter apply +- Fix tool filter input not accepting text +- Fix status dropdown not showing selection + +Co-Authored-By: Claude " +``` + +--- + +## Task 4: Final Verification and Cleanup + +**Step 1: Run full test suite** + +Run: `npm test` +Expected: All tests pass (569+ tests) + +**Step 2: Run type checking** + +Run: `npm run build` +Expected: No TypeScript errors, successful build + +**Step 3: Manual end-to-end testing** + +Complete manual test checklist: +- [ ] Notifications display with "MCP Tool Called:" label +- [ ] Notifications show parameters on second line when enabled +- [ ] Notifications show single line when parameters disabled +- [ ] Toggling notifications on/off keeps section open +- [ ] Tool filter in history modal accepts text and filters +- [ ] Status dropdown in history modal shows selection +- [ ] Combined filters work correctly +- [ ] Results count updates correctly +- [ ] No regressions in existing functionality + +**Step 4: Review commits** + +Run: `git log --oneline master..HEAD` +Expected: See 3 clean commits for the 3 tasks + +**Step 5: Ready for merge/PR** + +The implementation is complete and ready for: +- Merge to master (if sole developer) +- Pull request (if team workflow) +- Use @superpowers:finishing-a-development-branch skill for next steps + +--- + +## Implementation Notes + +**TDD Applied:** +- Task 1 includes comprehensive test coverage for notification formatting +- Tasks 2-3 are UI components best tested manually +- All changes maintain existing test suite (569 tests pass) + +**DRY Applied:** +- Reusable `updateNotificationSection()` method for settings updates +- Reusable `updateHistoryList()` and `updateResultsCount()` for modal updates +- Shared filter logic in `applyFilters()` + +**YAGNI Applied:** +- No premature optimizations or extra features +- Minimal changes to fix reported issues +- No refactoring beyond what's needed + +**Commits:** +- Frequent, focused commits (one per task) +- Clear commit messages with context +- Each commit is independently valuable + +**Testing Strategy:** +- Unit tests for business logic (notification formatting) +- Manual testing for UI interactions (unavoidable with Obsidian API) +- Full test suite verification at each step