Skip to content

Commit 38c7f79

Browse files
authored
fix(monaco): correctly handle sparse color map updates (#1169)
1 parent baf9aee commit 38c7f79

File tree

2 files changed

+134
-3
lines changed

2 files changed

+134
-3
lines changed

packages/monaco/src/index.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,9 @@ export function shikiToMonaco(
8787
const ret = highlighter.setTheme(themeName)
8888
const theme = themeMap.get(themeName)
8989
colorMap.length = ret.colorMap.length
90-
ret.colorMap.forEach((color, i) => {
91-
colorMap[i] = color
92-
})
90+
for (let i = 0; i < ret.colorMap.length; i++) {
91+
colorMap[i] = ret.colorMap[i]
92+
}
9393
colorStyleToScopeMap.clear()
9494
theme?.rules.forEach((rule) => {
9595
const c = normalizeColor(rule.foreground)

packages/monaco/test/repro.test.ts

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
import { describe, expect, it, vi } from 'vitest'
2+
import { shikiToMonaco } from '../src/index'
3+
4+
describe('shikiToMonaco', () => {
5+
it('updates color map when theme changes', async () => {
6+
const highlighter = {
7+
getLoadedThemes: () => ['theme1', 'theme2'],
8+
getTheme: (id: string) => {
9+
if (id === 'theme1') {
10+
return {
11+
type: 'dark',
12+
colors: { 'editor.foreground': '#000000' },
13+
rules: [
14+
{ token: 'keyword', foreground: '#000000' },
15+
],
16+
}
17+
}
18+
else { // id === 'theme2'
19+
return {
20+
type: 'light',
21+
colors: {},
22+
rules: [{ token: 'string', foreground: '#000000' }],
23+
}
24+
}
25+
},
26+
setTheme: (id: string) => {
27+
if (id === 'theme1') {
28+
return { colorMap: ['#000000', '#ff0000'] }
29+
}
30+
else {
31+
// theme2 has sparse color map at index 0
32+
const map = [] as string[]
33+
map[1] = '#ffffff' // index 0 is empty
34+
return { colorMap: map }
35+
}
36+
},
37+
getLoadedLanguages: () => ['javascript'],
38+
getLanguage: (_lang: string) => ({
39+
tokenizeLine2: (line: string, stack: any) => {
40+
return {
41+
tokens: new Uint32Array([
42+
0, // startIndex
43+
0, // metadata: color index 0 (all 0s)
44+
]),
45+
ruleStack: stack,
46+
}
47+
},
48+
}),
49+
} as any
50+
51+
const monaco = {
52+
editor: {
53+
defineTheme: vi.fn(),
54+
setTheme: vi.fn(),
55+
create: vi.fn(),
56+
},
57+
languages: {
58+
register: vi.fn(),
59+
getLanguages: () => [{ id: 'javascript' }],
60+
setTokensProvider: vi.fn(),
61+
},
62+
} as any
63+
64+
shikiToMonaco(highlighter, monaco)
65+
66+
// Initial theme should be set (first one)
67+
// monaco.editor.setTheme is replaced by shikiToMonaco, so we can't check it as a spy directly
68+
// expect(monaco.editor.setTheme).toHaveBeenCalledWith('vitesse-dark')
69+
70+
// Capture the tokens provider
71+
const setTokensProviderCall = monaco.languages.setTokensProvider.mock.calls[0]
72+
const tokensProvider = setTokensProviderCall[1]
73+
74+
// Tokenize a simple string
75+
const state = tokensProvider.getInitialState()
76+
const code = 'const a = 1'
77+
78+
// Tokenize with first theme
79+
const result1 = tokensProvider.tokenize(code, state)
80+
const scopes1 = result1.tokens.map((t: any) => t.scopes)
81+
82+
// Switch theme
83+
monaco.editor.setTheme('theme2')
84+
85+
// Tokenize with second theme
86+
const result2 = tokensProvider.tokenize(code, state)
87+
const scopes2 = result2.tokens.map((t: any) => t.scopes)
88+
89+
// The scopes should be correct for the second theme.
90+
// Since we don't know the exact scopes, we can check if they are consistent with what we expect
91+
// or at least different if the themes are very different (though scopes might be same if they map to same textmate scopes).
92+
// But `shikiToMonaco` resolves scopes based on colors.
93+
// If `vitesse-dark` and `vitesse-light` have different colors for the same token,
94+
// `shikiToMonaco` might generate different scopes (because it generates scopes like `token.color-hex`).
95+
// Wait, looking at `index.ts`:
96+
// `const scope = color ? (findScopeByColorAndStyle(color, fontStyle) || '') : ''`
97+
// And `findScopeByColorAndStyle` looks up in `colorStyleToScopeMap`.
98+
// `colorStyleToScopeMap` maps `color|fontStyle` -> `rule.token`.
99+
// `rule.token` is the TextMate scope (e.g. `keyword.control`).
100+
101+
// So if both themes map `keyword` to some color, `scopes` should be `keyword`.
102+
// The issue is that it renders "incorrect color".
103+
// Monaco renders color based on the scope.
104+
// If `shikiToMonaco` returns the correct scope (e.g. `keyword`), Monaco will look up that scope in the theme's rules and apply the color.
105+
// Since `monaco.editor.setTheme` also calls `_setTheme` (the original monaco one), Monaco should have the correct theme loaded.
106+
107+
// So if `scopes` are correct, the color should be correct.
108+
// Unless `scopes` are WRONG.
109+
// Why would `scopes` be wrong?
110+
// If `colorMap` has wrong colors, `normalizeColor(colorMap[colorIdx])` returns wrong color.
111+
// Then `findScopeByColorAndStyle` looks up wrong color.
112+
// If that wrong color exists in `colorStyleToScopeMap` (which is updated for the new theme), it might find a scope that happens to have that color, or find nothing.
113+
// If it finds nothing, scope is empty string.
114+
115+
// So if the bug exists, `scopes2` might be empty or wrong.
116+
117+
// We expect scopes to be non-empty for keywords.
118+
// 'const' is a keyword.
119+
expect(scopes1[0]).not.toBe('')
120+
expect(scopes2[0]).toBe('') // Should be empty because theme2 has no color at index 0
121+
122+
// Also, if the bug is that it uses the OLD color map:
123+
// `colorIdx` from `tokenizeLine2` refers to the NEW theme's color map (presumably).
124+
// If `colorMap` (the array) still has OLD colors, then `colorMap[colorIdx]` gives a color from the OLD theme at that index.
125+
// But `colorStyleToScopeMap` is updated to the NEW theme.
126+
// So we look up an OLD color in the NEW theme's map.
127+
// Likely we won't find it, so scope becomes empty.
128+
129+
// So the failure mode is likely empty scopes or incorrect scopes.
130+
})
131+
})

0 commit comments

Comments
 (0)