Skip to content

Conversation

@Raubzeug
Copy link
Contributor

@Raubzeug Raubzeug commented Nov 7, 2025

This is needed to unify yql syntax highlight. For now it will use textmate grammar from monaco-yql-languages, which is generated by yql backend team.

Stand

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
378 374 0 2 2
Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

  1. Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
  2. Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: 🔺

Current: 66.08 MB | Main: 47.46 MB
Diff: +18.62 MB (39.23%)

⚠️ Bundle size increased. Please review.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • 📊 indicates links to detailed reports.
  • 🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.
## Key Changes
  • New Implementation: Created shikiHighlighter.ts with singleton pattern, lazy loading, and efficient caching of languages/themes
  • Component Refactor: YDBSyntaxHighlighter now uses async rendering with loading states and proper cleanup
  • Custom Themes: Implemented YQL-specific light/dark themes matching Monaco editor colors
  • Dependency Update: Replaced react-syntax-highlighter (and its types) with shiki 3.15.0
  • Bundle Optimization: Dynamic import of Shiki library reduces initial bundle (though overall bundle increased by 18.62 MB due to Shiki's size)

Implementation Quality

The implementation follows good patterns:

  • Proper singleton pattern prevents multiple Shiki instances
  • Lazy loading defers Shiki import until first use
  • Language and theme caching avoids redundant loading
  • Effect cleanup prevents memory leaks with cancellation tokens
  • Loading state provides good UX during async highlighting

Notable Observations

  • The 39% bundle increase (18.62 MB) is expected given Shiki's comprehensive language support and WASM-based highlighting engine
  • The migration maintains backward compatibility - existing component usage patterns remain unchanged
  • The effect re-runs on every text change which may cause frequent re-highlighting in interactive scenarios

Confidence Score: 5/5

  • This PR is safe to merge with no blocking issues
  • The implementation is well-structured with proper error handling, memory management, and caching. The migration is complete and maintains API compatibility. The bundle size increase is acceptable given the feature requirements. No logic errors, security issues, or breaking changes detected.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/components/SyntaxHighlighter/shikiHighlighter.ts 5/5 New core implementation using Shiki with lazy loading, singleton pattern, and efficient caching of languages/themes
src/components/SyntaxHighlighter/YDBSyntaxHighlighter.tsx 4/5 Replaced react-syntax-highlighter with async Shiki rendering, added loading state, proper cleanup on unmount. Minor: effect runs on every text change which may impact performance
src/components/SyntaxHighlighter/themes.ts 5/5 Complete rewrite with Shiki theme definitions matching Monaco colors for YQL light/dark themes
package.json 5/5 Replaced react-syntax-highlighter with shiki 3.15.0, removed unused type definitions

Sequence Diagram

sequenceDiagram
    participant Component as YDBSyntaxHighlighter
    participant Hook as useEffect
    participant Highlighter as shikiHighlighter
    participant Shiki as Shiki Library
    participant Cache as Language/Theme Cache

    Component->>Hook: Mount with text, language, themeType
    Hook->>Highlighter: highlightCode(text, language, themeType)
    
    alt First use of Shiki
        Highlighter->>Shiki: Dynamic import('shiki')
        Shiki-->>Highlighter: createHighlighter instance
        Note over Highlighter: Cache highlighter promise
    end
    
    Highlighter->>Cache: Check if language loaded
    alt Language not in cache
        Highlighter->>Shiki: loadLanguage(yql or bundled)
        Shiki-->>Highlighter: Language loaded
        Highlighter->>Cache: Add to loadedLanguages
    end
    
    Highlighter->>Cache: Check if theme loaded
    alt Theme not in cache
        Highlighter->>Shiki: loadTheme(custom or bundled)
        Shiki-->>Highlighter: Theme loaded
        Highlighter->>Cache: Add to loadedThemes
    end
    
    Highlighter->>Shiki: codeToHtml(code, lang, theme)
    Shiki-->>Highlighter: HTML string with syntax highlighting
    Highlighter-->>Hook: Return HTML
    Hook->>Component: setHighlightedHtml(html)
    Component->>Component: Render with dangerouslySetInnerHTML
    
    Note over Component: On text/language/theme change
    Component->>Hook: Re-run effect
    Hook->>Highlighter: highlightCode (cached instances)
    Note over Cache: Reuses cached language/theme
    Highlighter-->>Component: New highlighted HTML
Loading

builtinFunctions,
keywords,
typeKeywords,
} from 'monaco-yql-languages/build/yql/yql.keywords';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These constants are not valid yet, and it won't be undated in future. Soon it will be removed from the library.

@astandrik astandrik requested a review from Copilot November 10, 2025 09:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR migrates the syntax highlighting implementation from react-syntax-highlighter to shiki. The change improves performance and maintainability by:

  • Replacing Prism-based highlighting with the modern Shiki library
  • Implementing custom YQL themes based on Monaco VS themes
  • Using lazy loading for languages and themes with a singleton highlighter instance

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/components/SyntaxHighlighter/YDBSyntaxHighlighter.tsx Replaced react-syntax-highlighter component with async shiki-based highlighting
src/components/SyntaxHighlighter/YDBSyntaxHighlighter.scss Added new content wrapper styles for shiki-generated HTML
src/components/SyntaxHighlighter/shikiHighlighter.ts New module implementing lazy-loaded shiki highlighter with caching
src/components/SyntaxHighlighter/themes.ts Replaced Prism themes with custom TextMate theme registrations for YQL
src/components/SyntaxHighlighter/types.ts Updated to use shiki's BundledLanguage/BundledTheme types
src/components/SyntaxHighlighter/yql.ts Removed Prism YQL language definition (replaced by TextMate grammar)
src/components/TruncatedQuery/TruncatedQuery.tsx Added className prop to YDBSyntaxHighlighter
src/components/TruncatedQuery/TruncatedQuery.scss Added padding override for pre elements
package.json Removed react-syntax-highlighter dependencies, added shiki 3.15.0
package-lock.json Updated dependency tree for shiki and its transitive dependencies

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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