Skip to content

feat(theme): add Pierre defaults#471

Open
benvinegar wants to merge 1 commit into
mainfrom
feat/pierre-themes
Open

feat(theme): add Pierre defaults#471
benvinegar wants to merge 1 commit into
mainfrom
feat/pierre-themes

Conversation

@benvinegar

Copy link
Copy Markdown
Member

Summary

  • Add Diffs.com's Pierre themes (pierre-dark, pierre-light) as built-in selectable/configurable themes.
  • Keep Pierre themes alphabetized with the Shiki-backed theme list.
  • Preserve Pierre token colors when using Pierre syntax themes, while keeping non-Pierre color remapping behavior intact.
  • Document the new theme ids and add a patch changeset.

Testing

  • bun run format:check
  • bun run typecheck
  • bun test src/ui/themes.test.ts src/ui/diff/pierre.test.ts src/core/config.test.ts src/ui/lib/ui-lib.test.ts
  • bun run test (failed on existing session integration timeout: session CLI integration > navigate works, and comment add only focuses the session when --focus is passed; rerunning that single test also timed out)

This PR description was generated by Pi using OpenAI GPT-5 Codex (2026-06-21)

@greptile-apps

greptile-apps Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds Diffs.com's pierre-dark and pierre-light themes as first-class selectable themes in Hunk, alongside the existing Shiki-backed palette. The implementation introduces a unified BUNDLED_HIGHLIGHTER_THEME_IDS type that merges Pierre IDs alphabetically with the Shiki list, updates all consumers of the old BUNDLED_SHIKI_THEME_IDS export, and adds a bypass in normalizeHighlightedColor so Pierre's intentional token colors are preserved when those themes are active.

  • New theme IDs (pierre-dark, pierre-light) are registered with background, foreground, and diff-color metadata in shikiThemes.ts, fully covering the same data paths as existing Shiki themes.
  • Color-remap bypass in pierre.ts short-circuits the reserved-token remapping for any syntaxTheme that starts with \"pierre-\", keeping Pierre's exact palette intact while leaving non-Pierre themes unaffected.
  • Type compat: BundledShikiThemeDiffColors is preserved as a type alias so external callers aren't broken."

Confidence Score: 4/5

Safe to merge — the change is additive and well-tested, with no modifications to existing theme behavior.

The implementation correctly threads Pierre themes through every data path (backgrounds, foregrounds, diff colors, config validation, selector lists) and the new color-remap bypass is exercised by an end-to-end test. The only concern is the indexOf("plastic") insertion-index lookup, which silently degrades to wrong ordering rather than failing loudly if the anchor theme is ever removed.

src/ui/lib/shikiThemes.ts — the PIERRE_THEME_INSERTION_INDEX derivation via indexOf deserves a defensive check.

Important Files Changed

Filename Overview
src/ui/lib/shikiThemes.ts Adds BUNDLED_PIERRE_THEME_IDS, BundledPierreThemeId, BundledHighlighterThemeId, and BUNDLED_HIGHLIGHTER_THEME_IDS (alphabetically merging Pierre themes with Shiki themes at the position of "plastic"). Background, foreground, and diff color entries are added for both Pierre themes. A backward-compat type alias BundledShikiThemeDiffColors is preserved. The insertion index relies on indexOf("plastic") which silently misbehaves if that entry is removed.
src/ui/diff/pierre.ts Adds an early-return guard in normalizeHighlightedColor that skips the Pierre-to-theme-color remap when the active syntax theme starts with "pierre-", preserving Pierre's intended token colors for these themes while keeping the remap for all other palettes.
src/ui/themes.ts Migrates all references from BUNDLED_SHIKI_THEME_IDS/BundledShikiThemeId to the new BUNDLED_HIGHLIGHTER_THEME_IDS/BundledHighlighterThemeId equivalents. The buildShikiTheme function now also covers Pierre themes, correctly setting syntaxTheme: themeId so the Pierre color-bypass logic activates.
src/core/config.ts Simple rename of BUNDLED_SHIKI_THEME_IDS → BUNDLED_HIGHLIGHTER_THEME_IDS for the config validation set, correctly expanding the valid theme ID space to include Pierre themes.
src/opentui/themes.ts Propagates the rename to HUNK_DIFF_THEME_NAMES, so OpenTUI consumers automatically pick up the two Pierre theme IDs.
src/ui/diff/pierre.test.ts Adds an end-to-end integration test confirming that pierre-dark syntax highlighting produces the expected token colors — directly exercising the new bypass logic.
src/ui/themes.test.ts Expands contrast, chrome-color, and selector tests to cover the full BUNDLED_HIGHLIGHTER_THEME_IDS set, and adds a dedicated assertion block for the Pierre dark and light theme properties.
src/core/config.test.ts Adds pierre-dark to the parameterized custom-theme-base acceptance test; minor code formatting cleanup on the test block.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["BUNDLED_PIERRE_THEME_IDS\n['pierre-dark', 'pierre-light']"] --> C
    B["BUNDLED_SHIKI_THEME_IDS\n(64 themes)"] --> C
    C["BUNDLED_HIGHLIGHTER_THEME_IDS\n(merged, alphabetical via indexOf('plastic'))"] --> D
    C --> E
    C --> F

    D["config.ts\nBUILT_IN_THEME_IDS\n(validation)"]
    E["opentui/themes.ts\nHUNK_DIFF_THEME_NAMES\n(type + selector)"]
    F["themes.ts\nTHEMES[]\nbuildShikiTheme per id\nsyntaxTheme = themeId"]

    F --> G["resolveTheme()"]
    G --> H["pierre.ts\nnormalizeHighlightedColor()"]
    H -- "syntaxTheme starts with 'pierre-'" --> I["Return raw Pierre token color"]
    H -- "other theme" --> J["Remap reserved Pierre colors\nto theme-safe syntax colors"]
Loading
%%{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["BUNDLED_PIERRE_THEME_IDS\n['pierre-dark', 'pierre-light']"] --> C
    B["BUNDLED_SHIKI_THEME_IDS\n(64 themes)"] --> C
    C["BUNDLED_HIGHLIGHTER_THEME_IDS\n(merged, alphabetical via indexOf('plastic'))"] --> D
    C --> E
    C --> F

    D["config.ts\nBUILT_IN_THEME_IDS\n(validation)"]
    E["opentui/themes.ts\nHUNK_DIFF_THEME_NAMES\n(type + selector)"]
    F["themes.ts\nTHEMES[]\nbuildShikiTheme per id\nsyntaxTheme = themeId"]

    F --> G["resolveTheme()"]
    G --> H["pierre.ts\nnormalizeHighlightedColor()"]
    H -- "syntaxTheme starts with 'pierre-'" --> I["Return raw Pierre token color"]
    H -- "other theme" --> J["Remap reserved Pierre colors\nto theme-safe syntax colors"]
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/ui/lib/shikiThemes.ts:76-82
**Fragile insertion-index via `indexOf`**

`BUNDLED_SHIKI_THEME_IDS.indexOf("plastic")` returns `-1` if `"plastic"` is ever removed from the Shiki list. `Array.prototype.slice` treats negative indices as offsets from the end, so the resulting `BUNDLED_HIGHLIGHTER_THEME_IDS` would silently place Pierre themes immediately before `"vitesse-light"` rather than at the correct alphabetical position — breaking the documented "alphabetized with Shiki ids" contract without any error or failing test. A runtime assertion (`if (PIERRE_THEME_INSERTION_INDEX === -1) throw new Error(...)`) or at least a comment noting the assumption would make the failure loud instead of silent.

Reviews (1): Last reviewed commit: "feat(theme): add Pierre defaults" | Re-trigger Greptile

Comment thread src/ui/lib/shikiThemes.ts
Comment on lines +76 to +82
const PIERRE_THEME_INSERTION_INDEX = BUNDLED_SHIKI_THEME_IDS.indexOf("plastic");

export const BUNDLED_HIGHLIGHTER_THEME_IDS: readonly BundledHighlighterThemeId[] = [
...BUNDLED_SHIKI_THEME_IDS.slice(0, PIERRE_THEME_INSERTION_INDEX),
...BUNDLED_PIERRE_THEME_IDS,
...BUNDLED_SHIKI_THEME_IDS.slice(PIERRE_THEME_INSERTION_INDEX),
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Fragile insertion-index via indexOf

BUNDLED_SHIKI_THEME_IDS.indexOf("plastic") returns -1 if "plastic" is ever removed from the Shiki list. Array.prototype.slice treats negative indices as offsets from the end, so the resulting BUNDLED_HIGHLIGHTER_THEME_IDS would silently place Pierre themes immediately before "vitesse-light" rather than at the correct alphabetical position — breaking the documented "alphabetized with Shiki ids" contract without any error or failing test. A runtime assertion (if (PIERRE_THEME_INSERTION_INDEX === -1) throw new Error(...)) or at least a comment noting the assumption would make the failure loud instead of silent.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/lib/shikiThemes.ts
Line: 76-82

Comment:
**Fragile insertion-index via `indexOf`**

`BUNDLED_SHIKI_THEME_IDS.indexOf("plastic")` returns `-1` if `"plastic"` is ever removed from the Shiki list. `Array.prototype.slice` treats negative indices as offsets from the end, so the resulting `BUNDLED_HIGHLIGHTER_THEME_IDS` would silently place Pierre themes immediately before `"vitesse-light"` rather than at the correct alphabetical position — breaking the documented "alphabetized with Shiki ids" contract without any error or failing test. A runtime assertion (`if (PIERRE_THEME_INSERTION_INDEX === -1) throw new Error(...)`) or at least a comment noting the assumption would make the failure loud instead of silent.

How can I resolve this? If you propose a fix, please make it concise.

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.

1 participant