docs: add implementation plan for notification UI improvements
Detailed plan with bite-sized tasks for: - Notification message format improvements - Settings section collapse fix - Modal filter control repairs Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
717
docs/plans/2025-10-26-notification-ui-improvements.md
Normal file
717
docs/plans/2025-10-26-notification-ui-improvements.md
Normal file
@@ -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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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
|
||||
Reference in New Issue
Block a user