Skip to content

Add Shiki syntax highlighting with CMake support#5

Open
bjoernschroeder wants to merge 4 commits intomainfrom
bas/dark_mode_code_highlighting
Open

Add Shiki syntax highlighting with CMake support#5
bjoernschroeder wants to merge 4 commits intomainfrom
bas/dark_mode_code_highlighting

Conversation

@bjoernschroeder
Copy link
Copy Markdown
Collaborator

@bjoernschroeder bjoernschroeder commented Apr 20, 2026

Summary

  • Add Shiki-based syntax highlighting to diff views (Catppuccin Latte theme, 22 languages)
  • Highlight old/new file streams separately to preserve multi-line token context
  • Add CMake language detection for .cmake files and CMakeLists.txt
  • Expose data-language attribute on diff content element for client-side highlighting
  • Add 24 unit tests for the syntax highlight module
  • Add difftypp binary to gitignore
  • Update README with syntax highlighting and CMake docs

Replace highlight.js with Shiki (TextMate grammars, Catppuccin Latte theme)
for VSCode-quality syntax highlighting in diff views. Highlights old/new
file streams separately to preserve multi-line token context.

Supports 22 languages: Go, C/C++, CMake, Python, TypeScript, Rust, Java,
CSS, HTML, JSON, YAML, XML, Markdown, Bash, SQL, TOML, Dockerfile, Makefile,
and more. Adds CMake detection for .cmake files and CMakeLists.txt.

Includes 24 unit tests for the syntax highlight module.
@bjoernschroeder bjoernschroeder requested a review from Gamezar April 20, 2026 03:36
@Gamezar
Copy link
Copy Markdown
Owner

Gamezar commented Apr 21, 2026

Review — Shiki Syntax Highlighting + CMake

Overview

Client-side Shiki highlighting for diff views. Old/new streams tokenized separately to preserve multi-line context. Adds CMake detection. 22 lang grammars lazy-imported. 24-test vitest suite.

Correctness

  • Context lines double-highlighted. Pushed into both oldLines and newLines; new-stream run overwrites old-stream innerHTML on same row. Wasted work, last-write-wins. Fix: assign context rows to one stream only.
  • Silent token-count mismatch. result.tokens.forEach guards i >= stream.length but fewer tokens than rows = trailing rows keep server HTML with no warning. Add console.warn in dev.
  • CSS comment misleading. .diff-line-content span[style] { font-style: inherit } claims to "ensure styles render" but actually strips Shiki's italic output. Then .italic { font-style: italic } re-adds via class Shiki never emits (Shiki uses inline style). Net: italics disabled globally. State intent or remove.
  • DEFAULT_TEXT_COLOR hardcoded to #4c4f69 (catppuccin-latte base). Theme swap = stale. Add // tied to catppuccin-latte note.

Style

  • TS follows project rules (no semis, single quotes). Good.
  • // fail silently in catch masks real bugs — console.warn preferred.
  • shiki/core + lazy lang imports = good discipline. But built diff.js is ~2.2MB single bundle — confirm esbuild config intends all 22 grammars ship on every diff page.

Template / Server

  • SelectedFileLanguage already wired at internal/server/server.go:1073. PR cleanly exposes via data-language. Good.
  • layout.html adds stray blank line — drop.

Tests

  • 24 tests, solid pure-fn + DOM coverage. jsdom env confirmed.
  • Go side: 3 CMake cases in TestDetectLanguageComplete. Sufficient.
  • Missing: no test for context-line double-highlight or token-count mismatch.

Security

  • escapeHtml covers & < > ". Single-quote not escaped — OK because attrs use double quotes and token content goes to text nodes. Safe.

Performance

  • Highlighter created + disposed per page load. No cross-file caching. Nav-heavy sessions re-parse grammar each click — consider module-level memo.
  • Whole-block tokenize of both streams = two full-file highlights per view. Large diffs may block main thread. No measurements.

Risks

  • Bundle size regression (verify delta).
  • Italic suppression may surprise users.
  • Silent grammar-load failure = hard to diagnose.

Verdict

Solid feature, clean Go-side change. Fix context-line double-highlight and clarify italic CSS before merge. Confirm bundle-size impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants