docs: add design document for ObsidianReviewBot fixes
This commit is contained in:
213
docs/plans/2025-10-28-obsidian-review-bot-fixes-design.md
Normal file
213
docs/plans/2025-10-28-obsidian-review-bot-fixes-design.md
Normal file
@@ -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
|
||||
Reference in New Issue
Block a user