feat(theme): support raw Shiki syntax scopes#570
Conversation
Greptile SummaryThis PR replaces Hunk's indirect syntax-color approach (inferring semantic roles from Pierre output colors) with direct Shiki/TextMate scope overrides registered as content-addressed derived themes. A compatibility adapter (
Confidence Score: 4/5Safe to merge; the core highlighting path, cache invalidation, and legacy-compat adapter all work correctly. The design is sound: content-addressed theme names keep highlight caches coherent, the legacy adapter is well-isolated, and the boundary normalization in config.ts prevents double-expansion in the happy path. All four comments are style and documentation concerns with no blocking correctness issues. README.md drops the backward-compatibility note for the old [custom_theme.syntax] table. pierre.ts themeRenderCacheKey and themes.ts buildCustomTheme deserve inline comments explaining the cache-separation contract and the two-pass normalization design. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["TOML config"] -->|"readCustomTheme"| B["resolveSyntaxScopeOverrides"]
B --> C["CustomThemeConfig { syntaxScopes }"]
C -->|"buildCustomTheme"| D["AppTheme { syntaxScopeOverrides }"]
D -->|"syntaxHighlightThemeName"| E{"overrides?"}
E -- yes --> F["hunk-custom-XXXX"]
E -- no --> G["Base theme name"]
F --> H["registerCustomTheme"]
H --> I["Pierre / Shiki"]
G --> I
I --> J["HAST nodes"]
J --> K["RenderSpan[]"] --> L["TUI / Pager"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["TOML config"] -->|"readCustomTheme"| B["resolveSyntaxScopeOverrides"]
B --> C["CustomThemeConfig { syntaxScopes }"]
C -->|"buildCustomTheme"| D["AppTheme { syntaxScopeOverrides }"]
D -->|"syntaxHighlightThemeName"| E{"overrides?"}
E -- yes --> F["hunk-custom-XXXX"]
E -- no --> G["Base theme name"]
F --> H["registerCustomTheme"]
H --> I["Pierre / Shiki"]
G --> I
I --> J["HAST nodes"]
J --> K["RenderSpan[]"] --> L["TUI / Pager"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Keep the source-accurate base theme and pass exact TextMate selectors through unchanged. | ||
| // The diff highlighter registers that derived palette with Pierre by content hash. | ||
| syntaxTheme: baseTheme.syntaxTheme, | ||
| syntaxScopeOverrides: resolveSyntaxScopeOverrides(customTheme.syntax, customTheme.syntaxScopes), | ||
| }; |
There was a problem hiding this comment.
resolveSyntaxScopeOverrides silently no-ops for config-sourced themes
config.ts / readCustomTheme already calls resolveSyntaxScopeOverrides(legacySyntax, exactSyntaxScopes) and stores the merged result in customTheme.syntaxScopes, deliberately never setting customTheme.syntax. When buildCustomTheme receives that object, the call here is resolveSyntaxScopeOverrides(undefined, mergedScopes), which just returns mergedScopes unchanged.
The call is not wrong — it correctly handles programmatic callers that construct a CustomThemeConfig with both syntax and syntaxScopes set directly — but without a comment it looks like redundant work and may invite "cleanup" that removes the config-side normalization. A brief note explaining the two-pass contract (config.ts normalises at the parse boundary; themes.ts defends against direct callers) would help future maintainers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/themes.ts
Line: 294-298
Comment:
**Double-resolution of `resolveSyntaxScopeOverrides` silently no-ops for config-sourced themes**
`config.ts / readCustomTheme` already calls `resolveSyntaxScopeOverrides(legacySyntax, exactSyntaxScopes)` and stores the merged result in `customTheme.syntaxScopes`, deliberately never setting `customTheme.syntax`. When `buildCustomTheme` receives that object, the call here is `resolveSyntaxScopeOverrides(undefined, mergedScopes)`, which just returns `mergedScopes` unchanged.
The call is not wrong — it correctly handles programmatic callers that construct a `CustomThemeConfig` with both `syntax` and `syntaxScopes` set directly — but without a comment it looks like redundant work and may invite "cleanup" that removes the config-side normalization. A brief note explaining the two-pass contract (`config.ts` normalises at the parse boundary; `themes.ts` defends against direct callers) would help future maintainers.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
$body
This comment was generated by Pi using GPT-5.6 Sol
| async function buildCustomSyntaxTheme( | ||
| name: string, | ||
| baseThemeName: string, | ||
| scopeOverrides: Record<string, string>, | ||
| ) { | ||
| const baseTheme = await resolvePierreTheme(baseThemeName); | ||
| const scopeSettings = Object.entries(scopeOverrides).map(([scope, foreground]) => ({ | ||
| scope, | ||
| settings: { foreground }, | ||
| })); | ||
|
|
||
| return { | ||
| ...baseTheme, | ||
| name, | ||
| settings: [...(baseTheme.settings ?? []), ...scopeSettings], | ||
| } satisfies ThemeRegistrationResolved; | ||
| } |
There was a problem hiding this comment.
scopeSettings is appended after baseTheme.settings. In Shiki's TextMate scoring, selector specificity is determined by the number of matching scope-path components, and position only breaks ties at equal specificity. A user who writes "comment" = "#ffffff" to recolor all comments will be silently overridden for any token where the base theme carries a more specific rule such as comment.line.number-sign.python.
This is technically correct TextMate behaviour, but the README currently says only "later rules win when matching selectors have equal specificity" — it doesn't warn that base-theme selectors with higher specificity will always win regardless of position. A note in the docs or in a config-validation warning would prevent confusing misconfiguration reports like the one this PR closes (#557).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/syntaxHighlightTheme.ts
Line: 22-38
Comment:
**User broad-selector overrides cannot beat more-specific base-theme rules**
`scopeSettings` is appended after `baseTheme.settings`. In Shiki's TextMate scoring, selector specificity is determined by the number of matching scope-path components, and position only breaks ties at equal specificity. A user who writes `"comment" = "#ffffff"` to recolor all comments will be silently overridden for any token where the base theme carries a more specific rule such as `comment.line.number-sign.python`.
This is technically correct TextMate behaviour, but the README currently says only "later rules win when matching selectors have equal specificity" — it doesn't warn that base-theme selectors with higher specificity will always win regardless of position. A note in the docs or in a config-validation warning would prevent confusing misconfiguration reports like the one this PR closes (#557).
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Documented this specificity rule in e3f0d4f. The README now states that a more-specific base selector beats a broader override and directs users to add the grammar-specific selector when needed.
Responded by Pi using OpenAI GPT-5.3.
This comment was generated by Pi using OpenAI GPT-5.3
|
Addressed the Greptile review in
Focused theme/highlighting tests, typecheck, lint, and formatting all pass. Responded by Pi using OpenAI GPT-5.3. This comment was generated by Pi using OpenAI GPT-5.3 |
1 participant
Summary
[custom_theme.syntax_scopes]for exact Shiki/TextMate selector-to-color overrides[custom_theme.syntax]roles through an isolated compatibility adapterWhy
custom_theme.syntax.commentcould not affect TypeScript comments because Hunk inferred semantic roles from a small set of Pierre output colors, and Pierre emitted comments using an unmapped color. Applying overrides as native Shiki rules preserves token identity and avoids maintaining another language-scope taxonomy in the main highlighting path.Closes #557.
Configuration
The previous semantic table remains supported during a documented migration window and will be removed in the next major release. Exact scope entries take precedence over translated legacy entries with the same selector.
Validation
bun run typecheckbun run lintbun run format:checkbun run test:tty-smoke: 9 passedbun test: 1,222 passed, 14 skipped; the unrelated direct-file atomic-save PTY test remains flaky locally and is reproducible frommainRelated work
PR #478 adds full VS Code/Shiki theme-file loading. Scope overlays and full theme files are complementary, but both touch theme registration and should be reconciled before merging independently.
This PR description was generated by Pi using OpenAI GPT-5.3