fix: address Obsidian plugin review findings - #95
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The settings tab no longer overrides display(), which is the lifecycle entry point in Obsidian (and in this repo’s mocks), so the UI may not render unless display() delegates to update().
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Obsidian settings/UI code to address plugin review findings (Obsidian 1.13+ API changes) and aligns unit tests with the updated settings-tab rendering flow.
Changes:
- Refactors the settings tab rendering to focus on
update()and documents the declarative settings API behavior. - Updates UI components for Obsidian 1.13+ (e.g.,
setDestructive()), and removes inline style toggling in favor of CSS classes in modals. - Adjusts settings unit tests to validate the updated rendering lifecycle.
File summaries
| File | Description |
|---|---|
| tests/unit/settings.test.ts | Updates tests to exercise the new settings-tab rendering path. |
| src/settings.ts | Refactors settings-tab lifecycle/rendering and updates button API usage for Obsidian 1.13+. |
| src/modals.ts | Minor UI cleanup (dropdown loop style) and switches error visibility to class-based toggling. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
381
to
386
| /** | ||
| * Obsidian's settings-tab lifecycle entry point. | ||
| * | ||
| * Keep the actual renderer in update() so existing refreshes from setting | ||
| * controls continue to work, while ensuring Obsidian invokes the renderer | ||
| * when the tab is opened (including Obsidian 1.13+). | ||
| */ | ||
| display(): void { | ||
| this.update(); | ||
| } | ||
|
|
||
| /** | ||
| * Imperative settings renderer shared by display() and in-tab refreshes. | ||
| * getSettingDefinitions() returns [] because this tab intentionally uses | ||
| * the richer imperative UI rather than declarative setting indexing. | ||
| * Obsidian's current settings-tab lifecycle renderer. Obsidian invokes | ||
| * update() when the tab opens, and controls can call it to refresh the | ||
| * imperative UI in place. | ||
| */ | ||
| update(): void { |
Comment on lines
+31
to
35
| it('should render settings options during the current update lifecycle', () => { | ||
| const container = tab.containerEl; | ||
| tab.display(); | ||
| tab.update(); | ||
|
|
||
| expect(container.classList.contains('make-it-rain-settings-container')).toBe(true); |
frostmute
added a commit
that referenced
this pull request
Sep 4, 2026
…update() PR #95 dropped the PluginSettingTab.display() override while keeping getSettingDefinitions() returning an empty array. Obsidian 1.13+ falls back to display() to render the tab imperatively when the definitions list is empty; update() alone stores definitions for the declarative path and the search index. With display() removed, the framework falls through to the base no-op, leaving the pane blank (Issue #87 regression reported in 2.1.3). display() is kept as a one-line bridge to the existing imperative renderer. update() remains the refresh entry point for in-tab state changes. Both delegate to the same renderer, so the DOM shape is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the verified Obsidian plugin review findings and updates the affected settings tests.