feat: adopt obsidian 1.11 settings APIs #1041
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughBumped Obsidian dependency to 1.11.0; extended ambient type declarations for Setting/SettingGroup/SettingTab; refactored settings UI into grouped SettingGroup-based sections with Svelte component lifecycle handling; adjusted no-scripts notice DOM targeting; added comprehensive Obsidian UI test stubs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/obsidian-stub.ts (1)
206-232: Consider storing all components in thecomponentsarray for test consistency.
addComponentstores the created component inthis.components, butaddButton,addToggle,addText, andaddDropdowndo not. This inconsistency could affect tests that inspect thecomponentsarray.🔎 Proposed fix
addButton(cb: (component: ButtonComponent) => any): this { - cb(new ButtonComponent(this.controlEl)); + const component = new ButtonComponent(this.controlEl); + this.components.push(component); + cb(component); return this; } addToggle(cb: (component: ToggleComponent) => any): this { - cb(new ToggleComponent(this.controlEl)); + const component = new ToggleComponent(this.controlEl); + this.components.push(component); + cb(component); return this; } addText(cb: (component: TextComponent) => any): this { - cb(new TextComponent(this.controlEl)); + const component = new TextComponent(this.controlEl); + this.components.push(component); + cb(component); return this; } addDropdown(cb: (component: DropdownComponent) => any): this { - cb(new DropdownComponent(this.controlEl)); + const component = new DropdownComponent(this.controlEl); + this.components.push(component); + cb(component); return this; }src/quickAddSettingsTab.ts (2)
209-230: Consider using DOM APIs instead of innerHTML.While these values are build-time constants and only render in dev builds (minimizing XSS risk), using DOM APIs would be cleaner and more consistent with Obsidian patterns.
🔎 Suggested refactor using DOM APIs
if (__DEV_GIT_BRANCH__ !== null) { const branchDiv = infoContainer.createDiv(); - branchDiv.innerHTML = `<strong>Branch:</strong> ${__DEV_GIT_BRANCH__}`; + branchDiv.createEl("strong", { text: "Branch:" }); + branchDiv.appendText(` ${__DEV_GIT_BRANCH__}`); branchDiv.style.marginBottom = "5px"; } if (__DEV_GIT_COMMIT__ !== null) { const commitDiv = infoContainer.createDiv(); - commitDiv.innerHTML = `<strong>Commit:</strong> ${__DEV_GIT_COMMIT__}`; + commitDiv.createEl("strong", { text: "Commit:" }); + commitDiv.appendText(` ${__DEV_GIT_COMMIT__}`); commitDiv.style.marginBottom = "5px"; } if (__DEV_GIT_DIRTY__ !== null) { const statusDiv = infoContainer.createDiv(); const statusText = __DEV_GIT_DIRTY__ ? "Yes (uncommitted changes)" : "No"; const statusColor = __DEV_GIT_DIRTY__ ? "var(--text-warning)" : "var(--text-success)"; - statusDiv.innerHTML = `<strong>Uncommitted changes:</strong> <span style="color: ${statusColor}">${statusText}</span>`; + statusDiv.createEl("strong", { text: "Uncommitted changes:" }); + statusDiv.createEl("span", { text: ` ${statusText}`, attr: { style: `color: ${statusColor}` } }); }
408-410: Minor inconsistency: reading fromplugin.settingsinstead ofsettingsStore.Other settings consistently use
settingsStore.getState()to read current values, but this one reads fromthis.plugin.settings.inputPrompt. Consider usingsettingsStore.getState().inputPrompt === "multi-line"for consistency.🔎 Suggested fix
.addToggle((toggle) => toggle - .setValue(this.plugin.settings.inputPrompt === "multi-line") + .setValue(settingsStore.getState().inputPrompt === "multi-line") .setTooltip("Use multi-line input prompt")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (6)
manifest.json(1 hunks)package.json(1 hunks)src/global.d.ts(2 hunks)src/gui/MacroGUIs/noScriptsFoundNotice.ts(1 hunks)src/quickAddSettingsTab.ts(2 hunks)tests/obsidian-stub.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Source code lives in
src/: core logic underengine/,services/, andutils/; Svelte UI insrc/gui; shared types insrc/types; settings entry insrc/quickAddSettingsTab.ts
Files:
src/gui/MacroGUIs/noScriptsFoundNotice.tssrc/global.d.tssrc/quickAddSettingsTab.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through theloggerutilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters ortests/obsidian-stub.ts
Files:
src/gui/MacroGUIs/noScriptsFoundNotice.tssrc/global.d.tssrc/quickAddSettingsTab.ts
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for classes and Svelte components
Files:
src/gui/MacroGUIs/noScriptsFoundNotice.tssrc/global.d.tssrc/quickAddSettingsTab.ts
tests/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place tests and stubs in
tests/directory
Files:
tests/obsidian-stub.ts
tests/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.{ts,tsx}: Co-locate specs with their source or group them undertests/feature-name
Add regression coverage for bug fixes
Files:
tests/obsidian-stub.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to src/**/*.{ts,tsx} : Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or `tests/obsidian-stub.ts`
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to src/**/*.{ts,tsx,js,jsx} : Source code lives in `src/`: core logic under `engine/`, `services/`, and `utils/`; Svelte UI in `src/gui`; shared types in `src/types`; settings entry in `src/quickAddSettingsTab.ts`
Applied to files:
src/global.d.tssrc/quickAddSettingsTab.ts
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to src/**/*.{ts,tsx} : Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or `tests/obsidian-stub.ts`
Applied to files:
src/global.d.tstests/obsidian-stub.ts
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to tests/**/*.{ts,tsx,js,jsx} : Place tests and stubs in `tests/` directory
Applied to files:
tests/obsidian-stub.ts
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to tests/**/*.{ts,tsx} : Add regression coverage for bug fixes
Applied to files:
tests/obsidian-stub.ts
🧬 Code graph analysis (1)
src/quickAddSettingsTab.ts (6)
tests/obsidian-stub.ts (6)
Setting(151-233)BaseComponent(19-31)PluginSettingTab(318-330)App(269-297)SettingGroup(235-263)TFolder(340-344)src/types/choices/IChoice.ts (1)
IChoice(3-10)src/gui/PackageManager/ExportPackageModal.ts (1)
ExportPackageModal(7-37)src/gui/PackageManager/ImportPackageModal.ts (1)
ImportPackageModal(5-27)docs/static/scripts/migrateDataviewToFrontmatter.js (1)
currentValue(285-285)src/gui/suggesters/genericTextSuggester.ts (1)
GenericTextSuggester(4-42)
🪛 ast-grep (0.40.0)
src/quickAddSettingsTab.ts
[warning] 210-210: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: branchDiv.innerHTML = <strong>Branch:</strong> ${__DEV_GIT_BRANCH__}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 216-216: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: commitDiv.innerHTML = <strong>Commit:</strong> ${__DEV_GIT_COMMIT__}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 228-228: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: statusDiv.innerHTML = <strong>Uncommitted changes:</strong> <span style="color: ${statusColor}">${statusText}</span>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 210-210: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: branchDiv.innerHTML = <strong>Branch:</strong> ${__DEV_GIT_BRANCH__}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 216-216: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: commitDiv.innerHTML = <strong>Commit:</strong> ${__DEV_GIT_COMMIT__}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 228-228: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: statusDiv.innerHTML = <strong>Uncommitted changes:</strong> <span style="color: ${statusColor}">${statusText}</span>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 GitHub Actions: Build With Lint
src/global.d.ts
[error] 1-1: ESLint: 'Setting' is defined but never used. (@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Build With Lint
src/global.d.ts
[failure] 1-1:
'Setting' is defined but never used
🔇 Additional comments (17)
package.json (1)
35-35: LGTM!The Obsidian dev dependency update to
^1.11.0aligns with the manifest'sminAppVersionbump and is required for the new settings APIs being adopted.manifest.json (1)
5-5: Breaking change: minimum Obsidian version bumped from 1.6.0 to 1.11.0.Users on Obsidian versions below 1.11.0 will no longer be able to use this plugin after the update. Consider documenting this in release notes.
tests/obsidian-stub.ts (5)
19-31: LGTM!Clean
BaseComponentbase class implementation with proper fluent API pattern.
33-149: LGTM!The component stubs (
ButtonComponent,ToggleComponent,DropdownComponent,TextComponent) properly extendBaseComponent, create appropriate DOM elements, and maintain fluent API chaining. No-op methods are appropriately stubbed for testing.
235-263: LGTM!The
SettingGroupstub correctly implements the grouped settings pattern with proper DOM structure and fluent API.
453-470: LGTM!The enhanced
Noticeclass stub withnoticeEl,containerEl, andmessageElproperties properly supports the fallback chain innoScriptsFoundNotice.ts. The aliasing ofnoticeEltomessageElis a reasonable test simplification.
484-505: LGTM!All new component stubs are properly included in the default export.
src/global.d.ts (1)
38-43: No issue found. TheaddSettingmethod insrc/global.d.ts:42correctly declares a return type ofthis, notvoid. This matches the implementation intests/obsidian-stub.ts:259-261, which also returnsthisfor method chaining. The declaration and stub are consistent.Likely an incorrect or invalid review comment.
src/quickAddSettingsTab.ts (9)
99-107: LGTM - Clean abstraction for backward compatibility.The
SettingGroupLiketype andSvelteSettingComponentwrapper provide a reasonable abstraction layer for the new Obsidian 1.11 API while maintaining compatibility with older versions.
120-163: Good lifecycle management.The
destroySettingViews()cleanup at both the start ofdisplay()and inhide()properly prevents memory leaks from orphaned Svelte components when the settings tab is re-displayed or closed.
165-185: Solid fallback pattern for backward compatibility.The runtime check
typeof SettingGroup === "function"gracefully handles environments where the new Obsidian 1.11 API isn't available, falling back to the traditional heading-based approach.
287-316: LGTM!Clean implementation of the packages setting with properly scoped modal handling.
318-394: Consistent pattern across notification and template settings.All these settings follow the same group-based pattern with proper state management through
settingsStore.
396-398: LGTM - Proper cleanup on hide.
426-454: LGTM - Clean folder path setting with suggestions.Good use of
GenericTextSuggesterfor UX improvement and proper type filtering withinstanceof TFolder.
456-513: LGTM!The remaining settings follow consistent patterns. The
this.display()calls on toggle change for online features and ribbon icon settings appropriately refresh the UI to reflect state-dependent changes.
245-251: No type mismatch exists. TheaddComponentcallback parameter is correctly typed asHTMLElementin bothtests/obsidian-stub.tsandsrc/global.d.ts, and the code properly passes it tomountView(el)andnew SvelteSettingComponent(el), both of which expectHTMLElement. The implementation is correct.Likely an incorrect or invalid review comment.
|
Note: Notice has containerEl in Obsidian 1.11 (alongside messageEl). The fallback chain messageEl ?? containerEl ?? noticeEl is intentional for compatibility with both new and older versions. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/global.d.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Source code lives in
src/: core logic underengine/,services/, andutils/; Svelte UI insrc/gui; shared types insrc/types; settings entry insrc/quickAddSettingsTab.ts
Files:
src/global.d.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through theloggerutilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters ortests/obsidian-stub.ts
Files:
src/global.d.ts
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for classes and Svelte components
Files:
src/global.d.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to src/**/*.{ts,tsx} : Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or `tests/obsidian-stub.ts`
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to src/**/*.{ts,tsx} : Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or `tests/obsidian-stub.ts`
Applied to files:
src/global.d.ts
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to src/**/*.{ts,tsx} : Prefer type-only imports in TypeScript files
Applied to files:
src/global.d.ts
🧬 Code graph analysis (1)
src/global.d.ts (1)
tests/obsidian-stub.ts (3)
Setting(151-233)BaseComponent(19-31)SettingGroup(235-263)
🔇 Additional comments (3)
src/global.d.ts (3)
1-1: LGTM! ESLint error resolved.The removal of
Settingfrom the import correctly addresses the previous lint error. Interface augmentation doesn't require importing the interface being extended within adeclare moduleblock.
34-36: LGTM! Setting interface augmentation is correct.The interface augmentation follows the correct pattern for extending the
Settinginterface with the newaddComponentmethod. The method signature correctly uses the importedBaseComponenttype.
45-47: LGTM! SettingTab interface augmentation is correct.The interface augmentation follows the correct pattern for extending the
SettingTabinterface with the newiconproperty using the importedIconNametype.
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.