perf: speed up syntax-highlight row building#177
Conversation
Greptile SummaryThis PR adds three module-level caches to speed up syntax-highlight row building in
Confidence Score: 4/5Safe to merge after fixing the normalizedColorCache keying bug; without the fix, switching between dark themes produces incorrect syntax highlight colors. One confirmed P1 logic bug: normalizedColorCache is keyed by theme.appearance instead of theme.id, causing color normalization to return stale results when switching among the three dark themes (graphite, midnight, ember). All other caching additions are correct and the performance rationale is sound. Score is 4 because the P1 fix is a targeted two-line change. src/ui/diff/pierre.ts — specifically the normalizedColorCache declaration (lines 163–166) and the normalizeHighlightedColor function (lines 173–191). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildSplitRows / buildStackRows] --> B[makeSplitCell / makeStackCell]
B --> C{highlightedLine?}
C -- undefined --> D[cleanDiffLine plain-text fallback]
C -- HAST node --> E[flattenHighlightedLine]
E --> F{flattenedHighlightedLineCache hit?
key: theme.id + emphasisBg}
F -- yes --> G[return cached RenderSpan array]
F -- no --> H[visit HAST tree recursively]
H --> I[parseStyleValue]
I --> J{parsedStyleValueCache hit?}
J -- yes --> K[return cached Map]
J -- no --> L[parse CSS string into Map and cache]
H --> M[normalizeHighlightedColor]
M --> N{normalizedColorCache hit?
key: theme.appearance ONLY ⚠️}
N -- yes --> O[return cached color — may be wrong theme's color!]
N -- no --> P[resolve via RESERVED_PIERRE_TOKEN_COLORS
+ theme.syntaxColors → cache]
H --> Q[mergeSpan → build spans array]
Q --> R[store spans in flattenedHighlightedLineCache]
G --> S[RenderSpan rows returned to renderer]
Reviews (1): Last reviewed commit: "perf: speed up syntax-highlight row buil..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const normalizedColorCache = { | ||
| dark: new Map<string, string>(), | ||
| light: new Map<string, string>(), | ||
| }; | ||
| // The expensive part after highlighting is walking Pierre's HAST line tree and flattening it | ||
| // into terminal spans. The same highlighted line objects are reused when files remount or when | ||
| // we build both split and stack rows, so memoize flattened spans by line node + theme/background. | ||
| const flattenedHighlightedLineCache = new WeakMap<HastNode, Map<string, RenderSpan[]>>(); | ||
|
|
||
| /** Remap Pierre token hues that collide with diff add/remove semantics into theme-safe syntax colors. */ | ||
| function normalizeHighlightedColor(color: string | undefined, theme: AppTheme) { | ||
| if (!color) { | ||
| return color; | ||
| } | ||
|
|
||
| const cached = normalizedColorCache[theme.appearance].get(color); | ||
| if (cached) { | ||
| return cached; | ||
| } | ||
|
|
||
| const normalized = color.trim().toLowerCase(); | ||
| const reserved = | ||
| RESERVED_PIERRE_TOKEN_COLORS[theme.appearance][ | ||
| normalized as keyof (typeof RESERVED_PIERRE_TOKEN_COLORS)[typeof theme.appearance] | ||
| ]; | ||
| if (!reserved) { | ||
| return color; | ||
| } | ||
|
|
||
| return theme.syntaxColors[reserved]; | ||
| const resolvedColor = reserved ? theme.syntaxColors[reserved] : color; | ||
| normalizedColorCache[theme.appearance].set(color, resolvedColor); | ||
| return resolvedColor; | ||
| } |
There was a problem hiding this comment.
normalizedColorCache keyed by appearance causes stale colors across dark themes
The cache maps theme.appearance ("dark" or "light") to a color map, but the resolved color comes from theme.syntaxColors[reserved] — which differs per theme id, not just per appearance. There are three dark themes with distinct keyword/string colors: graphite (#c4d0da / #d8c6ef), midnight (#8ed4ff / #c7b4ff), and ember (#ffb47f / #ffd3a8).
If the app first renders with the graphite theme, #ff6762 (Pierre's dark keyword indicator) gets cached as "#c4d0da" (graphite's keyword color). When the user switches to midnight, normalizedColorCache["dark"].get("#ff6762") returns "#c4d0da" instead of midnight's "#8ed4ff". The sibling flattenedHighlightedLineCache correctly uses theme.id in its composite key (line 214), so flattened spans are properly recomputed per theme — but normalizeHighlightedColor then poisons those freshly-walked spans with the wrong color.
Fix: key by theme.id instead of theme.appearance:
const normalizedColorCache = new Map<string, Map<string, string>>();
function normalizeHighlightedColor(color: string | undefined, theme: AppTheme) {
if (!color) {
return color;
}
let cacheForTheme = normalizedColorCache.get(theme.id);
if (!cacheForTheme) {
cacheForTheme = new Map<string, string>();
normalizedColorCache.set(theme.id, cacheForTheme);
}
const cached = cacheForTheme.get(color);
if (cached !== undefined) {
return cached;
}
const normalized = color.trim().toLowerCase();
const reserved =
RESERVED_PIERRE_TOKEN_COLORS[theme.appearance][
normalized as keyof (typeof RESERVED_PIERRE_TOKEN_COLORS)[typeof theme.appearance]
];
const resolvedColor = reserved ? theme.syntaxColors[reserved] : color;
cacheForTheme.set(color, resolvedColor);
return resolvedColor;
}There was a problem hiding this comment.
Good catch — I changed the reserved-token remap cache to key by theme.id instead of appearance, and added a regression test that reuses the same dark highlighted output across graphite, midnight, and ember to make sure theme switches keep their own keyword/string colors.
This comment was generated by Pi using OpenAI o3
1 participant
Summary
src/ui/diff/pierre.tsBenchmark note
Notes
src/ui/diff/pierre.tsValidation
bun run typecheckbun test src/ui/diff/pierre.test.ts src/ui/AppHost.reload.test.tsxThis PR description was generated by Pi using OpenAI o3