feat(ai): edit AI providers on settings sub-pages instead of stacked modals - #1824
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Deploying quickadd with
|
| Latest commit: |
c9212e3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6a83a48e.quickadd.pages.dev |
| Branch Preview URL: | https://feat-ai-settings-subpage.quickadd.pages.dev |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (42)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…modals Co-authored-by: Christian Bager Bach Houmann <christian@bagerbach.com>
Co-authored-by: Christian Bager Bach Houmann <christian@bagerbach.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Co-authored-by: Christian Bager Bach Houmann <christian@bagerbach.com>
e14e0b1 to
c9212e3
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e14e0b1637
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function aiPageSignature(providers: readonly AIProvider[]): string { | ||
| return JSON.stringify(providers.map((p) => [p.id, p.name, p.endpoint])); |
There was a problem hiding this comment.
When a user types in either the provider Name or Endpoint field, every keystroke changes this signature; the store subscription in QuickAddSettingsTab immediately calls this.update(), which rebuilds and re-renders the active settings page. This can replace the input currently receiving the event, causing focus/cursor loss after each character and potentially invalidating the provider page path during a rename. Update the parent entry after leaving the provider page, or exclude live-edited fields from the rebuild signature.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked in Obsidian 1.13.7; this doesn't happen. SettingTab.update() → refreshCurrentPage re-displays only the root tab or a declarative page. The provider page is an imperative SettingPage, so it is never re-rendered. Live check while typing a rename into Name: document.activeElement === nameInput stays true after each input, and the inline title, the window title, and (after Back) the list label all update. The page stack holds the page object rather than its name path, so Back works after a rename too. Regression tests for rename and title sync are in AIProviderSettingPage.test.ts.
| export function createAIAssistantPage(app: App): SettingDefinitionPage<SettingsKey> { | ||
| return { | ||
| type: "page", | ||
| name: AI_ASSISTANT_PAGE_NAME, | ||
| desc: "Providers, models, and defaults for AI commands.", | ||
| displayValue: () => describeProviderCount(storedProviders().length), | ||
| visible: () => !settingsStore.getState().disableOnlineFeatures, | ||
| items: [createProvidersList(app), createDefaultsGroup(app)], |
There was a problem hiding this comment.
The new provider add/edit/delete workflow is exposed only through declarative/custom settings pages, while the added tests invoke those pages through the Obsidian stub; a repo-wide search found no command or API entrypoint with inspectable state and deterministic logging that can exercise this flow through obsidian:e2e. Add that seam so the replacement workflow can be verified in the required real-Obsidian CLI path.
AGENTS.md reference: AGENTS.md:L103-L107
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Added in 8df21d7: a command quickadd:openAIAssistantSettings ("Open AI Assistant settings") that opens Settings → QuickAdd → AI Assistant, so the flow is reachable from the Obsidian CLI (obsidian command id=quickadd:openAIAssistantSettings). Verified live: it opens as one modal on the QuickAdd tab with the AI Assistant page showing. All provider state is inspectable in data.json/app.plugins.plugins.quickadd.settings.ai, and every flow in the PR table was exercised in real Obsidian through obsidian:e2e eval. Unit tests cover the command's navigation order and its behavior while AI is disabled.
Co-authored-by: Christian Bager Bach Houmann <christian@bagerbach.com>
…izes Co-authored-by: Christian Bager Bach Houmann <christian@bagerbach.com>
2 participants
Summary
AI provider editing moves out of three stacked modals (sparkles button → AI Assistant settings → Edit providers → provider editor) into Obsidian 1.13 settings sub-pages:
Settings → QuickAdd → AI & online → AI Assistant → provider
type: "page"entry. It shows "N providers" on its row and is hidden while Disable AI & online features is on. It holds a Providers list (native+add button, or an "Add provider" row on mobile; Delete/Backspace deletes with confirmation; each entry shows a live "N retired" value with a warning status) and a Defaults group (default model, prompt template folder, show assistant, confirm tool calls, default system prompt). Because the page is declarative, Obsidian's settings search indexes it; "default model", "provider", and "API key" all land here.SettingPagewith a connection group (name, endpoint, type, API key, Test connection), Model sync (source, auto-sync with Sync now and the last-synced status line, Browse), a Models list (filter,+add, retired row with Remove retired, Retired badges, newest first), and Delete provider. The delete action also works on touch, since Obsidian draws no trash button on page entries.settingsStore, and providers are addressed by stable id, so a background sync never overwrites edits.syncStoredProvidersyncs a detached copy and merges the result into the provider as it is when the request returns.app.setting.navigateToSearchResult, the path its own settings search takes; if that API ever disappears, a Notice tells the user where to go.quickadd:openAIAssistantSettings) opens the page from the palette or the Obsidian CLI. While AI is disabled, it explains why instead of opening the page.Number.MAX_SAFE_INTEGER(a 400-digit input used to becomeInfinity, which is stored asnull).AIAssistantSettingsModal,AIAssistantProvidersModal, and their snapshot/restore and race-tracking code.Screenshots (Obsidian 1.13.7, throwaway vault)
Before: three stacked modals
After: entry under AI & online
AI Assistant page
Provider page after a live Test connection (key linked from the keychain)
Models: filter, add, and retired cleanup
Retired badge and Delete provider
Mobile (Obsidian's
emulateMobile, 400 px)Verification in real Obsidian 1.13.7 (isolated e2e vault, live OpenAI API)
data.json:apiKeyRefset,apiKeyempty✓ Connected. The provider lists 64 models.(green); without a key: red✗ … HTTP 401 … (No API key is linked.)Synced from the models.dev directory: already up to date.· statusLast synced just now · 29 models.Not synced yet., thenSynced from the provider's models endpoint: 28 new, 0 updated.·57 modelsdata.json:defaultModel: "gpt-5.2",defaultModelRef: {providerId: "openai", name: "gpt-5.2"}1 retired⚠ → Remove retired → confirm → badge and row gone; back on the list the warning is gone; stored retired = 0data.jsonCustom (custom)/Custom (custom-2); after deleting one, the other relabels toCustomquickadd:openAIAssistantSettingscommanddev:errors→No errors captured.with the debugger attachedAlso reviewed in dark theme and at mobile width; keyboard focus rings are visible.
Tests
New regression tests:
AIProviderSettingPage.test.ts,aiAssistantSettingsPage.test.ts,aiSettingsState.test.ts,modelSyncService.syncStoredProvider.test.ts, andquickAddSettingsTab.aiPage.test.ts. They port every behavior the deleted modal tests pinned: sort order, filter, delete by name after model objects are replaced, retired names captured at click time, Test connection using the provider API, endpoint edits clearing stale status, Sync now counting only the list on screen, and Add model validation. They add coverage for pre-migration data withoutai.providers(the tab is constructed before migrations run), duplicate-id repair, page-signature scoping (a sync never rebuilds the page), live retired counts, default-model refresh after a sync, delete-from-page, titles after a rename, and navigation with its fallback.ChoiceView.aiEntry.test.tschecks that both buttons call the navigation.pnpm run build-with-lint✅ (one pre-existing warning inmodelsDirectory.fetch.test.ts) ·pnpm run check(svelte-check) ✅ 0 errors ·pnpm run test: 460 files, 5925 tests passed.Release / migration notes
app.setting.navigateToSearchResultfor the two entry buttons only, with a fallback Notice.docs/.../Images/AI_Assistant_Setup.gif(re-recorded in docs: retake outdated screenshots and demo GIFs #1822) still shows the old modal flow and should be re-recorded against these pages. The text around it is updated.AIAssistant.md,QuickAddAPI.md), along with the model-not-found hint inaiHelpers.ts.Note
Replace stacked AI provider modals with editable settings sub-pages
syncStoredProviderin modelSyncService.ts, which merges discovered models into the current provider state without overwriting concurrent edits.storedProviderstolerates pre-migration settings without a provider array.resolveModelInputOrThrowerror guidance now points to Settings → QuickAdd → AI Assistant; removed provider-list CSS selectors and theAIAssistantProvidersModalsync test file; ChoiceView requires the newopenAISettingsprop.Macroscope summarized 8df21d7.
Summary by CodeRabbit