fix(editor): stop Monaco theme leaking across editor instances#282
Merged
Conversation
Monaco themes are global, so any editor that resolved its theme differently from the others would re-color every editor in the app. The AI explain modal was the worst offender: it passed a theme id that was never registered via loadMonacoTheme, and it used the UI theme instead of the dedicated editor theme override. Add a useEditorTheme hook that resolves the effective editor theme (settings override with fallback to the current UI theme) and use it in every Monaco-based component. The AI explain modal now also registers the theme in beforeMount like the rest of the editors. Closes #281
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| render(<CellCodeEditor value="" onChange={vi.fn()} />, { | ||
| wrapper: withTheme(makeTheme("tabularis-dark")), | ||
| }); | ||
| it("follows the editor theme when it changes", () => { |
There was a problem hiding this comment.
WARNING: Test name claims reactivity but only asserts initial render value
The test title says "follows the editor theme when it changes", but the implementation never triggers a re-render after changing mockEditorTheme.current. It only verifies the initial theme passed to useEditorTheme. To actually test reactivity, render the component, update the mock, call rerender(), and then assert the new theme.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Fix test or merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (17 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k2.6-20260420 · 332,845 tokens |
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.
Closes #281
Monaco themes are global to the page, so every editor instance has to agree on which theme is active. Until now each Monaco-based component resolved the theme on its own: most used the UI theme, the SQL editor and the save-query modal honored the
editorThemesettings override, and the AI explain modal passed a theme id without ever registering it throughloadMonacoTheme. Whichever editor mounted last won, so opening the AI explanation modal re-colored every other editor in the app.Changes
useEditorThemehook that resolves the effective editor theme: theeditorThemesettings override when set, otherwise the current UI theme (with fallback if the override points to a deleted theme)SqlEditorWrapper,SqlPreview,QueryModal,AiExplainModal,AiApprovalModal,ConfigJsonModal,McpModal,McpPage,VisualExplainView,CellCodeEditor,CellDiffEditorAiExplainModalnow registers the theme inbeforeMountand reacts to theme changes while open, like the other editors already didCellCodeEditor/CellDiffEditordropped their optional ThemeContext handling (they always render inside the providers); tests updated accordinglytests/hooks/useEditorTheme.test.tsTesting
npx tsc --noEmitcleannpx vitest run: 2494 passed, 0 failed