diff --git a/docs/plans/2025-10-28-obsidian-review-bot-fixes-design.md b/docs/plans/2025-10-28-obsidian-review-bot-fixes-design.md new file mode 100644 index 0000000..8fd0259 --- /dev/null +++ b/docs/plans/2025-10-28-obsidian-review-bot-fixes-design.md @@ -0,0 +1,213 @@ +# ObsidianReviewBot Fixes Design + +**Date:** 2025-10-28 +**Status:** Approved +**PR:** https://github.com/obsidianmd/obsidian-releases/pull/8298 + +## Overview + +This design addresses all required issues identified by ObsidianReviewBot for the MCP Server plugin submission to the Obsidian community plugin repository. + +## Required Fixes + +1. **Config path documentation** - Update hardcoded `.obsidian` examples to generic alternatives +2. **Command naming** - Remove "MCP Server" from command display names +3. **File deletion API** - Replace `vault.delete()` with `app.fileManager.trashFile()` +4. **Inline styles** - Extract 90+ JavaScript style assignments to CSS with semantic class names + +## Implementation Strategy + +**Approach:** Fix-by-fix across files - Complete one type of fix across all affected files before moving to the next fix type. + +**Benefits:** +- Groups related changes together for clearer git history +- Easier to test each fix type independently +- Simpler code review with focused commits + +## Fix Order and Details + +### Fix 1: Config Path Documentation + +**Files affected:** `src/tools/index.ts` + +**Changes:** +- Line 235: Update exclude pattern example from `['.obsidian/**', '*.tmp']` to `['templates/**', '*.tmp']` +- Line 300: Same update for consistency + +**Rationale:** Obsidian's configuration directory isn't necessarily `.obsidian` - users can configure this. Examples should use generic folders rather than system directories. + +**Risk:** None - documentation only, no functional changes + +### Fix 2: Command Naming + +**Files affected:** `src/main.ts` + +**Changes:** +- Line 54: "Start MCP Server" → "Start server" +- Line 62: "Stop MCP Server" → "Stop server" +- Line 70: "Restart MCP Server" → "Restart server" + +**Note:** Command IDs remain unchanged (stable API requirement) + +**Rationale:** Obsidian plugin guidelines state command names should not include the plugin name itself. + +**Risk:** Low - purely cosmetic change to command palette display + +### Fix 3: File Deletion API + +**Files affected:** `src/tools/note-tools.ts` + +**Changes:** +- Line 162: `await this.vault.delete(existingFile)` → `await this.fileManager.trashFile(existingFile)` +- Line 546: `await this.vault.delete(file)` → `await this.fileManager.trashFile(file)` + +**Context:** +- Line 162: Overwrite conflict resolution when creating files +- Line 546: Permanent delete operation (when soft=false) + +**Rationale:** Use `app.fileManager.trashFile()` instead of direct deletion to respect user's trash preferences configured in Obsidian settings. + +**Risk:** Medium - changes deletion behavior, requires testing both scenarios + +**Testing:** +- Verify overwrite conflict resolution still works +- Verify permanent delete operation respects user preferences +- Confirm files go to user's configured trash location + +### Fix 4: Inline Styles to CSS + +**Files affected:** +- `styles.css` (add new classes) +- `src/settings.ts` (remove inline styles, add CSS classes) + +**New CSS Classes:** + +```css +/* Authentication section */ +.mcp-auth-section { margin-bottom: 20px; } +.mcp-auth-summary { + font-size: 1.17em; + font-weight: bold; + margin-bottom: 12px; + cursor: pointer; +} + +/* API key display */ +.mcp-key-display { + padding: 12px; + background-color: var(--background-secondary); + border-radius: 4px; + font-family: monospace; + font-size: 0.9em; + word-break: break-all; + user-select: all; + cursor: text; + margin-bottom: 16px; +} + +/* Tab navigation */ +.mcp-config-tabs { + display: flex; + gap: 8px; + margin-bottom: 16px; + border-bottom: 1px solid var(--background-modifier-border); +} + +.mcp-tab { + padding: 8px 16px; + border: none; + background: none; + cursor: pointer; + border-bottom: 2px solid transparent; +} + +.mcp-tab-active { + border-bottom-color: var(--interactive-accent); + font-weight: bold; +} + +/* Config display */ +.mcp-config-display { + padding: 12px; + background-color: var(--background-secondary); + border-radius: 4px; + font-size: 0.85em; + overflow-x: auto; + user-select: text; + cursor: text; + margin-bottom: 12px; +} + +/* Helper text */ +.mcp-file-path { + padding: 8px; + background-color: var(--background-secondary); + border-radius: 4px; + font-family: monospace; + font-size: 0.9em; + margin-bottom: 12px; + color: var(--text-muted); +} + +.mcp-usage-note { + font-size: 0.9em; + color: var(--text-muted); + font-style: italic; +} + +/* Additional utility classes */ +.mcp-heading { + margin-top: 24px; + margin-bottom: 12px; +} + +.mcp-container { margin-bottom: 20px; } + +.mcp-button-group { + display: flex; + gap: 8px; + margin-bottom: 12px; +} + +.mcp-label { + margin-bottom: 4px; + font-size: 0.9em; + color: var(--text-muted); +} +``` + +**Changes to settings.ts:** +- Remove all `.style.` property assignments (90+ lines) +- Add corresponding CSS class names using `.addClass()` or `className` property +- Preserve dynamic styling for tab active state (use conditional class application) + +**Rationale:** Obsidian plugin guidelines require styles to be in CSS files rather than applied via JavaScript. This improves maintainability and follows platform conventions. + +**Risk:** High - largest refactor, visual regression possible + +**Testing:** +- Build and load in Obsidian +- Verify settings panel appearance unchanged in both light and dark themes +- Test all interactive elements: collapsible sections, tabs, buttons +- Confirm responsive behavior + +## Testing Strategy + +**After each fix:** +1. Run `npm test` - ensure no test failures +2. Run `npm run build` - verify TypeScript compilation +3. Check for linting issues + +**Before final commit:** +1. Full test suite passes +2. Clean build with no warnings +3. Manual smoke test of all settings UI features +4. Visual verification in both light and dark themes + +## Success Criteria + +- All 4 ObsidianReviewBot required issues resolved +- No test regressions +- No visual regressions in settings panel +- Clean build with no TypeScript errors +- Ready for PR re-submission