feat(editor): reach the AI provider settings from the app menu - #422
Merged
Conversation
The dialog's open state was a useState inside LeftPanel's ChatStripPanel, which mounts only under `mode === "edit" && chatOpen`. Nothing outside that component could open it, so an "AI Providers" row in the wordmark menu would have been dead in Media, in Rec, and in Edit with the chat panel collapsed. Lift it the way the shortcuts dialog already is: ProviderSettingsContext owns `isProviderSettingsOpen` / `openProviderSettings` / `closeProviderSettings`, `ProviderSettingsDialog` is the one mount in App.tsx beside ShortcutsConfigDialog, and the panel's five call sites ask the context instead of a local setter. The refresh that hung off the old onClose moves with it: connecting a provider is what makes the composer usable, and the panel now watches the falling edge of the lifted state for the same event. The panel's own entry points stay. Two doors to one room is right here — the menu row is the discoverable path, not a replacement. Fixes #420
Contributor
|
Warning Review limit reached
Next review available in: 37 minutes Limit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
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 |
16 tasks
EtienneLescot
added a commit
that referenced
this pull request
Aug 20, 2026
Review follow-ups to #422, which lifted the AI provider dialog out of the chat panel and mounted it above the mode switch. Four of these are real bugs, three of them older than that PR and only made easier to reach by it. ModalShell dialogs never took focus, and the editor's window-level keydown handler only skips input/textarea/contentEditable targets. The app menu closes without restoring focus — right for a pointer user — so opening AI settings from it left `document.activeElement` on the body and every editor shortcut live underneath the backdrop: Delete destroyed the selected region, Ctrl+O stacked a second aria-modal dialog emitting a duplicate `id="modal-title"`, `?` stacked the shortcuts dialog. The handler now bails while a dialog owns the screen, and ModalShell moves focus into the dialog as it opens, which is also what announces it to a screen reader. Escape in the provider connect form left the dialog entirely, discarding a half-typed API key, instead of stepping back to the grid: ProviderSettings and ModalShell both listened on document and both fired, so the `mode === "form"` branch was dead. ModalShell grows a `closeOnEscape` opt-out for a dialog that handles the key itself. Disconnecting a provider went on showing it as CONNECTED until the dialog was closed and reopened. `snapshot` is not optional on the disconnect result, so the `?? refreshSnapshot()` fallback never ran — and refreshSnapshot is the only thing that calls setSnapshot. ProviderSettingsContext becomes EditorDialogsContext, holding a `section` rather than a boolean per dialog, split into a section context and an actions context whose value never changes identity. That is what stops NewEditorShell — timeline, preview, transport — re-rendering twice per dialog interaction just to hold an opener, and it is the shape the settings unification #420 describes needs anyway: the next section is a member of the union, not a third context and a third provider around App.tsx's editor branch. The panel's re-read on close loses its ref: "not open" already covers the mount and every close in one effect. It is also now tested, which it was not — the PR said as much — along with the fact that opening the dialog must not refresh. Also drops the dead `onActiveProviderChanged` prop and unexports ProviderSettings (ProviderSettingsDialog is its only caller), and corrects two comments the new menu row falsified: AppMenu's claim that every label is a common.actions or shortcuts key, and llm-providers.md's list of the panel's doors, which was missing the quick-pick popover's "full settings" row.
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
1 participant
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.
Summary
The app menu added in #414 lists Keyboard Shortcuts, Check for Updates and About OpenScreen, and deliberately left out the AI provider settings — the app's other settings surface, and the obvious place to look for it. It was left out because it could not have worked:
ProviderSettingswas mounted insideLeftPanel's chat strip, whose open state was a plainuseStatethere, and that panel renders only undermode === "edit" && chatOpen. A menu row would have been dead in Media, dead in Rec, and dead in Edit with the chat panel collapsed — worse than no row, because the user learns the menu lies.So the dialog is lifted the way the shortcuts dialog already was:
src/contexts/ProviderSettingsContext.tsxownsisProviderSettingsOpen/openProviderSettings/closeProviderSettings, mirroringShortcutsContext'sisConfigOpen/openConfig/closeConfig.ProviderSettingsDialog— inProviderSettings.tsx, next to the dialog it binds — is the one mount, inApp.tsxbesideShortcutsConfigDialog, lazily, above the mode switch. The dialog itself stays a plainopen/onClosecomponent rather than mounting itself: the settings unification this is step one of will want to render it as a section of a larger dialog, and a self-mounting dialog cannot be one.The panel's own entry points stay. Two doors to one room is right here: configuring the AI where you use it is good UX, and the menu row is the discoverable path, not a replacement.
The label is
editor.providerSettings.title— the dialog's own heading, not a new key. That is the rule the rest of the menu follows (its three current labels are all pre-existingcommon.actions/shortcutskeys shared with the native menu inelectron/main.ts), and it is what stops the menu from drifting from what it opens after a copy edit. All 13 locales already carry it, so no translation files change.One behavioural detail moved with the dialog. Its old
onClosealso ranrefreshLlm()— connecting a provider is what makes the composer usable and what fills the model pill, and the panel reads that over the bridge. There is noonCloseto hang that off any more, so the panel watches the falling edge of the lifted state, which is the same event. It is auseRefcomparison rather than a bare effect on[isProviderSettingsOpen], so it does not also fire on mount.Related issue
Fixes #420
The issue's "Bigger picture" — one Settings dialog with a sidebar of sections (General / Shortcuts / AI / Devices / About) — is deliberately not in here. This is its prerequisite either way, and the PR is scoped to the four steps the issue asks for.
Type of change
Release impact
Desktop impact
Screenshots / video
No screenshot: the browser pane in this session would not composite frames, so the run below was read back through the accessibility tree and the DOM instead. The app menu, in the editor:
Check for Updates is absent there because the browser shim answers the update veto
false— the same branch a Store/Flathub/Snap/Nix build takes.Testing
npm run test— 2008 passed, 5 skipped, 169 files.npm run lint,npx tsc --noEmit,npx tsc -p tsconfig.test.json --noEmit,npm run i18n:check(13 locales),npm run docs:check.src/components/ai-edition/ProviderSettings.test.tsxis new and is about reach, not about the dialog's screens: it renders the App.tsx shape (provider + dialog mount + top bar) with the real translations, and pins that the menu row opens the dialog, that it opens in Media and Rec, that the row's text and the dialog's heading are compared to each other rather than to a literal (so a copy edit moves both or fails), and that the same holds in French.EditorTopBar.test.tsxgains the row's presence and its routing, and its "offers shortcuts and about" case is now an exact-name match: the echoing translator renders both settings rows as a…titlekey, so the old/title/regex would have matched two items and pinned neither.?windowType=editor(npm run dev, browser-shim mode) and read the DOM back at each step: the row opens the dialog (#modal-title= "AI settings", full provider grid) in Media, in Rec, and in Edit with the chat panel collapsed — the three states where it would have been dead — with[aria-label="AI editor"]absent in each, confirming the panel that used to own it was not mounted. Then, with the panel open: the panel's gear and the welcome card's CTA both still open it,document.querySelectorAll('[role="dialog"]').lengthis 1 (one mount, not two), and the dialog's own close button returns it to 0.Not verified: the
refreshLlmpath on close, which needs a provider actually connected — the browser shim has no credential store.technical-documentation/testing/manual-e2e-checklist.mdcovers it, along with the three modes and the "only one dialog appears" case.Docs:
llm-providers.mdgains the context row and a note that the dialog now has two doors and one mount; the manual checklist's app-menu section counts four rows instead of three.