From ec35ff73da6c1c0bdeb4c87bcd7838d81e83972a Mon Sep 17 00:00:00 2001 From: JoJowAkaGioGiow Date: Wed, 29 Jul 2026 16:21:01 -0300 Subject: [PATCH 01/12] feat: add custom themes --- CHANGELOG.md | 5 + frontend/DESIGN.md | 9 +- frontend/src/components/TerminalView.tsx | 19 +- .../settings/AppearanceSettings.tsx | 187 +++++++++--- frontend/src/components/ui/sonner.tsx | 2 +- frontend/src/index.css | 11 +- frontend/src/lib/api-types.ts | 10 + frontend/src/lib/rpc.ts | 10 + frontend/src/lib/themes.test.ts | 66 +++++ frontend/src/lib/themes.ts | 154 ++++++++++ frontend/src/providers/settings.tsx | 111 +++++-- frontend/src/themes/dark.json | 43 +++ frontend/src/themes/light.json | 43 +++ internal/themes/bundled.go | 157 ++++++++++ internal/themes/themes.go | 271 ++++++++++++++++++ internal/themes/themes_test.go | 114 ++++++++ main.go | 2 + 17 files changed, 1119 insertions(+), 95 deletions(-) create mode 100644 frontend/src/lib/themes.test.ts create mode 100644 frontend/src/lib/themes.ts create mode 100644 frontend/src/themes/dark.json create mode 100644 frontend/src/themes/light.json create mode 100644 internal/themes/bundled.go create mode 100644 internal/themes/themes.go create mode 100644 internal/themes/themes_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b531f9d80..3db536a43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Appearance can import custom color themes.** lich still ships the bundled + Light and Dark themes, but Settings › Appearance now accepts theme JSON files + saved under the app config directory. Imported themes can recolor both the + interface tokens and the terminal palette, can be selected independently for + the app and terminal, and can be removed without touching the bundled themes. - **Every worktree gets a dev-server port of its own.** Two worktrees of one project both ran `pnpm dev` and both wanted the same port, so the second one lost — and the setup script that had just installed the dependencies had no diff --git a/frontend/DESIGN.md b/frontend/DESIGN.md index 04146d87e..48f195558 100644 --- a/frontend/DESIGN.md +++ b/frontend/DESIGN.md @@ -27,9 +27,9 @@ state first. ### Color -Zinc, achromatic, by decision — there is **no brand accent hue**. The palette is defined once in -`src/index.css` (`@theme` + `:root`/`.dark`, oklch). Never hardcode a hex or oklch value in a component; always -go through a token. +The bundled light/dark themes are zinc and achromatic — there is **no brand accent hue** by default. Palettes are +theme JSON data applied to the tokens mapped in `src/index.css` (`@theme` + CSS variables). Never hardcode a hex or +oklch value in a component; always go through a token. Surface roles (dark values shown; light is the mirror): @@ -120,7 +120,8 @@ Short specs; the code is the detail. All follow the idiom above. ## Fixed by decision — do not "fix" these -- **Zinc, no accent hue.** Adding a brand color is a design change, not a tweak. +- **Bundled themes stay zinc, no accent hue.** A chromatic palette belongs in a custom theme JSON, not in component + classes. - **The layout.** Top tabs · left session sidebar · terminal · right dock · footer. The reskin is skin, not structure. - **Semantic color** and the **diff language badges** — they carry meaning. diff --git a/frontend/src/components/TerminalView.tsx b/frontend/src/components/TerminalView.tsx index 0158dd5c9..9badc2f9c 100644 --- a/frontend/src/components/TerminalView.tsx +++ b/frontend/src/components/TerminalView.tsx @@ -21,7 +21,6 @@ import { copyToastMessage, COPY_TOAST_DURATION_MS } from "@/lib/terminal/copy-to import { computeGrid } from "@/lib/terminal/term-fit" import { mouseEncodingSequence } from "@/lib/terminal/term-modes" import { useSettings } from "@/providers/settings" -import type { ResolvedTheme } from "@/providers/settings" import type { SessionKind } from "@/lib/session/sessions" import "@xterm/xterm/css/xterm.css" @@ -106,13 +105,6 @@ function fitTerminal(term: Terminal, container: HTMLElement): void { } } -// Light matches the app's light-gray canvas (--background) rather than pure -// white, which glared; the dark foreground keeps CLI output high-contrast. -const TERMINAL_COLORS: Record = { - dark: { background: "#06070f", foreground: "#e5e7eb" }, - light: { background: "#e8e8ea", foreground: "#1f2328" }, -} - // ensureFontLoaded blocks until a font is available. The renderer measures // cell metrics at open, so bundled fonts (@font-face) must be loaded first; // system fonts resolve as no-ops and failures fall back to monospace. @@ -176,6 +168,7 @@ export function TerminalView({ visible, }: TerminalViewProps) { const { font, terminalFontSize, resolvedTerminalTheme } = useSettings() + const terminalColors = resolvedTerminalTheme.terminal const containerRef = useRef(null) const liveRef = useRef(null) // False until the mount effect's async setup builds the first terminal. @@ -195,11 +188,11 @@ export function TerminalView({ const visibleRef = useRef(visible) const fontRef = useRef(font) const fontSizeRef = useRef(terminalFontSize) - const themeRef = useRef(resolvedTerminalTheme) + const themeRef = useRef(terminalColors) visibleRef.current = visible fontRef.current = font fontSizeRef.current = terminalFontSize - themeRef.current = resolvedTerminalTheme + themeRef.current = terminalColors // In-terminal search (Ctrl+F). The open flag mirrors into a ref so the // terminal's key handler — wired once at creation — reads the live value. @@ -247,7 +240,7 @@ export function TerminalView({ cursorBlink: true, scrollback: SCROLLBACK_LINES, allowProposedApi: true, - theme: TERMINAL_COLORS[themeRef.current], + theme: themeRef.current, }) const serialize = new SerializeAddon() term.loadAddon(serialize) @@ -600,7 +593,7 @@ export function TerminalView({ useEffect(() => { const live = liveRef.current if (live) { - live.term.options.theme = TERMINAL_COLORS[resolvedTerminalTheme] + live.term.options.theme = resolvedTerminalTheme.terminal } }, [resolvedTerminalTheme]) @@ -612,7 +605,7 @@ export function TerminalView({
{searchOpen && (
diff --git a/frontend/src/components/settings/AppearanceSettings.tsx b/frontend/src/components/settings/AppearanceSettings.tsx index 42e18e458..5a6905b2b 100644 --- a/frontend/src/components/settings/AppearanceSettings.tsx +++ b/frontend/src/components/settings/AppearanceSettings.tsx @@ -1,14 +1,15 @@ import { CaseSensitive, Minus, - Monitor, - Moon, Plus, SquareTerminal, - Sun, + Trash2, + Upload, ZoomIn, ZoomOut, } from "lucide-react" +import { useRef } from "react" +import { toast } from "sonner" import { DEFAULT_TERMINAL_FONT_SIZE, DEFAULT_ZOOM, @@ -17,47 +18,38 @@ import { TERMINAL_FONT_SIZE_STEP, ZOOM_MAX, ZOOM_MIN, - THEMES, - TERMINAL_THEMES, ZOOM_STEP, useSettings, } from "@/providers/settings" import type { TerminalTheme, Theme } from "@/providers/settings" import { Stepper } from "@/components/common/Stepper" -import { SegmentedControl } from "./SegmentedControl" import { SettingBlock, SettingGroup } from "./SettingBlock" import { FontSetting } from "./FontSetting" - -// How each theme value presents itself. Built off THEMES/TERMINAL_THEMES so the -// picker cannot drift from what the provider persists — adding a theme there -// is what puts it on screen, and forgetting an entry here is a type error. -const THEME_LABELS: Record = { - system: { label: "System", icon: }, - light: { label: "Light", icon: }, - dark: { label: "Dark", icon: }, -} - -const TERMINAL_THEME_LABELS: Record = { - match: { label: "Match app", icon: }, - light: { label: "Light", icon: }, - dark: { label: "Dark", icon: }, -} - -const THEME_OPTIONS = THEMES.map((value) => ({ value, ...THEME_LABELS[value] })) - -const TERMINAL_THEME_OPTIONS = TERMINAL_THEMES.map((value) => ({ - value, - ...TERMINAL_THEME_LABELS[value], -})) +import { Button } from "@/components/ui/button" +import { + Select, + SelectContent, + SelectGroup, + SelectItem, + SelectLabel, + SelectTrigger, + SelectValue, +} from "@/components/ui/select" +import { customTheme } from "@/lib/themes" +import { errorText } from "@/lib/utils" // Appearance holds every look-and-feel control, split into an Interface group // (theme, zoom) and a Terminal group (background, text size, font) so the two // concerns read apart instead of as one flat list. The group label supplies the // context, so the block titles drop their "Interface"/"Terminal" prefix. export function AppearanceSettings() { + const fileRef = useRef(null) const { + themes, theme, setTheme, + importTheme, + removeTheme, zoom, setZoom, terminalFontSize, @@ -65,17 +57,98 @@ export function AppearanceSettings() { terminalTheme, setTerminalTheme, } = useSettings() + const removableTheme = customTheme(themes, theme) + + const onImportTheme = async (file: File | undefined) => { + if (!file) { + return + } + try { + const imported = await importTheme(await file.text()) + toast.success(`Imported theme: ${imported.name}`) + } catch (error) { + toast.error(`Theme import failed: ${errorText(error)}`) + } finally { + if (fileRef.current) { + fileRef.current.value = "" + } + } + } + + const onRemoveTheme = async () => { + if (!removableTheme) { + return + } + try { + await removeTheme(removableTheme.id) + toast.success(`Removed theme: ${removableTheme.name}`) + } catch (error) { + toast.error(`Theme removal failed: ${errorText(error)}`) + } + } return ( <> - - + +
+ + void onImportTheme(event.currentTarget.files?.[0])} + /> + + +
} - title="Appearance" - description="Background color of the terminal. Match app keeps it in sync with the interface theme so text stays legible." + title="Theme" + description="Match app keeps the terminal in sync with the interface theme so text stays legible." > - + onValueChange={(value) => value && setTerminalTheme(value as TerminalTheme)} + > + + + + + + Automatic + Match app + + + Bundled + {themes + .filter((item) => item.origin === "bundled") + .map((item) => ( + + {item.name} + + ))} + + {themes.some((item) => item.origin === "custom") && ( + + Custom + {themes + .filter((item) => item.origin === "custom") + .map((item) => ( + + {item.name} + + ))} + + )} + + + terminal: Record +} + /** internal/patchnotes.Group — one "### Added/Changed/Fixed" block of a release. */ export interface PatchNotesGroup { label: string diff --git a/frontend/src/lib/rpc.ts b/frontend/src/lib/rpc.ts index fc71d347b..670de2fe3 100644 --- a/frontend/src/lib/rpc.ts +++ b/frontend/src/lib/rpc.ts @@ -20,6 +20,7 @@ import type { PullRequestSummary, StoredProject, StoredSession, + ThemeDefinition, Worktree, } from "./api-types" @@ -256,3 +257,12 @@ export const Providers = { /** Every known provider with its install state (binary found on PATH). */ Detect: () => call("providers.Detect", []), } + +export const Themes = { + /** Bundled themes plus user-imported themes from the config dir. */ + List: () => call("themes.List", []), + /** Import a raw theme JSON document into the config dir. */ + Import: (raw: string) => call("themes.Import", [raw]), + /** Remove a user-imported theme. Bundled themes are rejected by the backend. */ + Remove: (id: string) => call("themes.Remove", [id]), +} diff --git a/frontend/src/lib/themes.test.ts b/frontend/src/lib/themes.test.ts new file mode 100644 index 000000000..da4f247c1 --- /dev/null +++ b/frontend/src/lib/themes.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from "vitest" +import type { ThemeDefinition } from "./api-types" +import { + APP_COLOR_TOKENS, + applyAppTheme, + BUNDLED_THEMES, + mergeThemes, + resolveTerminalTheme, + resolveTheme, + sanitizeTerminalThemePreference, + sanitizeThemePreference, +} from "./themes" + +function customTheme(id: string): ThemeDefinition { + const base = BUNDLED_THEMES[0] + return { + ...base, + id, + name: "Custom", + origin: "custom", + app: { ...base.app, background: "#101010" }, + terminal: { ...base.terminal, background: "#111111" }, + } +} + +describe("themes", () => { + it("keeps bundled themes first and merges custom themes", () => { + const themes = mergeThemes([customTheme("custom")]) + expect(themes.map((theme) => theme.id)).toEqual(["light", "dark", "custom"]) + }) + + it("resolves system to the bundled light or dark theme", () => { + expect(resolveTheme("system", BUNDLED_THEMES, false).id).toBe("light") + expect(resolveTheme("system", BUNDLED_THEMES, true).id).toBe("dark") + }) + + it("falls back to light when a stored custom theme is missing", () => { + expect(resolveTheme("missing", BUNDLED_THEMES, false).id).toBe("light") + }) + + it("resolves terminal match to the app theme", () => { + const appTheme = customTheme("custom") + expect(resolveTerminalTheme("match", appTheme, BUNDLED_THEMES)).toBe(appTheme) + expect(resolveTerminalTheme("dark", appTheme, BUNDLED_THEMES).id).toBe("dark") + }) + + it("sanitizes stored theme preferences", () => { + expect(sanitizeThemePreference("custom-theme")).toBe("custom-theme") + expect(sanitizeThemePreference("System")).toBe("system") + expect(sanitizeTerminalThemePreference("match")).toBe("match") + expect(sanitizeTerminalThemePreference("bad value")).toBe("match") + }) + + it("applies every app token as a CSS variable", () => { + const values = new Map() + const root = { + style: { + setProperty: (key: string, value: string) => values.set(key, value), + }, + } as unknown as HTMLElement + applyAppTheme(BUNDLED_THEMES[0], root) + for (const token of APP_COLOR_TOKENS) { + expect(values.get(`--${token}`)).toBe(BUNDLED_THEMES[0].app[token]) + } + }) +}) diff --git a/frontend/src/lib/themes.ts b/frontend/src/lib/themes.ts new file mode 100644 index 000000000..35b23fe70 --- /dev/null +++ b/frontend/src/lib/themes.ts @@ -0,0 +1,154 @@ +import type { ThemeDefinition } from "@/lib/api-types" +import darkTheme from "@/themes/dark.json" +import lightTheme from "@/themes/light.json" + +export const SYSTEM_THEME = "system" +export const MATCH_TERMINAL_THEME = "match" +export const DEFAULT_THEME = SYSTEM_THEME +export const DEFAULT_TERMINAL_THEME = MATCH_TERMINAL_THEME +export const LIGHT_THEME_ID = "light" +export const DARK_THEME_ID = "dark" + +export type Theme = typeof SYSTEM_THEME | string +export type TerminalTheme = typeof MATCH_TERMINAL_THEME | string +export type ResolvedTheme = ThemeDefinition + +export const APP_COLOR_TOKENS = [ + "background", + "foreground", + "card", + "card-foreground", + "popover", + "popover-foreground", + "primary", + "primary-foreground", + "secondary", + "secondary-foreground", + "muted", + "muted-foreground", + "accent", + "accent-foreground", + "destructive", + "border", + "input", + "ring", + "chart-1", + "chart-2", + "chart-3", + "chart-4", + "chart-5", + "sidebar", + "sidebar-foreground", + "sidebar-primary", + "sidebar-primary-foreground", + "sidebar-accent", + "sidebar-accent-foreground", + "sidebar-border", + "sidebar-ring", +] as const + +const BUNDLED = [lightTheme, darkTheme].map((theme) => + normalizeTheme(theme as unknown as ThemeDefinition, "bundled"), +) +const BUNDLED_ORDER = new Map(BUNDLED.map((theme, index) => [theme.id, index])) + +export const BUNDLED_THEMES = mergeThemes(BUNDLED) + +export function mergeThemes( + themes: readonly ThemeDefinition[] | null | undefined, +): ThemeDefinition[] { + const byId = new Map() + for (const theme of BUNDLED) { + byId.set(theme.id, normalizeTheme(theme, "bundled")) + } + for (const theme of themes ?? []) { + const normalized = normalizeTheme(theme) + byId.set(normalized.id, normalized) + } + return [...byId.values()].sort((a, b) => { + if (a.origin !== b.origin) { + return a.origin === "bundled" ? -1 : 1 + } + if (a.origin === "bundled") { + return (BUNDLED_ORDER.get(a.id) ?? 0) - (BUNDLED_ORDER.get(b.id) ?? 0) + } + return a.name.localeCompare(b.name) + }) +} + +export function resolveTheme( + selected: Theme, + themes: readonly ThemeDefinition[], + systemDark: boolean, +): ThemeDefinition { + if (selected === SYSTEM_THEME) { + return findTheme(themes, systemDark ? DARK_THEME_ID : LIGHT_THEME_ID) + } + return findTheme(themes, selected) +} + +export function resolveTerminalTheme( + selected: TerminalTheme, + appTheme: ThemeDefinition, + themes: readonly ThemeDefinition[], +): ThemeDefinition { + if (selected === MATCH_TERMINAL_THEME) { + return appTheme + } + return findTheme(themes, selected) +} + +export function applyAppTheme(theme: ThemeDefinition, root: HTMLElement): void { + for (const token of APP_COLOR_TOKENS) { + root.style.setProperty(`--${token}`, theme.app[token]) + } +} + +export function themeExists(themes: readonly ThemeDefinition[], id: string): boolean { + return themes.some((theme) => theme.id === id) +} + +export function customTheme( + themes: readonly ThemeDefinition[], + id: string, +): ThemeDefinition | null { + const theme = themes.find((item) => item.id === id) + return theme?.origin === "custom" ? theme : null +} + +export function sanitizeThemePreference(raw: string | null): Theme { + return safePreference(raw) ?? DEFAULT_THEME +} + +export function sanitizeTerminalThemePreference(raw: string | null): TerminalTheme { + return safePreference(raw) ?? DEFAULT_TERMINAL_THEME +} + +function findTheme(themes: readonly ThemeDefinition[], id: string): ThemeDefinition { + return ( + themes.find((theme) => theme.id === id) ?? + themes.find((theme) => theme.id === LIGHT_THEME_ID) ?? + normalizeTheme(lightTheme as unknown as ThemeDefinition, "bundled") + ) +} + +function safePreference(raw: string | null): string | null { + if (!raw || raw.length > 64) { + return null + } + return /^[a-z0-9][a-z0-9._-]{0,63}$/.test(raw) ? raw : null +} + +function normalizeTheme( + theme: ThemeDefinition, + origin: ThemeDefinition["origin"] = theme.origin, +): ThemeDefinition { + return { + id: theme.id, + name: theme.name, + scheme: theme.scheme, + origin, + app: { ...theme.app }, + terminal: { ...theme.terminal }, + } +} diff --git a/frontend/src/providers/settings.tsx b/frontend/src/providers/settings.tsx index 6bba77bda..edc230397 100644 --- a/frontend/src/providers/settings.tsx +++ b/frontend/src/providers/settings.tsx @@ -10,7 +10,24 @@ import { type Hotkeys, } from "@/lib/hotkeys" import { zoomIntent } from "@/lib/terminal/zoom-keys" -import { parseBoolPref, parseEnumPref, parseNumberPref, readPref, writePref } from "@/lib/prefs" +import { parseBoolPref, parseNumberPref, readPref, writePref } from "@/lib/prefs" +import { Themes as ThemeRPC } from "@/lib/rpc" +import { + applyAppTheme, + BUNDLED_THEMES, + DEFAULT_TERMINAL_THEME, + DEFAULT_THEME, + mergeThemes, + resolveTerminalTheme, + resolveTheme, + sanitizeTerminalThemePreference, + sanitizeThemePreference, +} from "@/lib/themes" +import type { TerminalTheme, Theme, ResolvedTheme } from "@/lib/themes" +import type { ThemeDefinition } from "@/lib/api-types" + +export type { TerminalTheme, Theme, ResolvedTheme } from "@/lib/themes" +export { DEFAULT_TERMINAL_THEME, DEFAULT_THEME } from "@/lib/themes" const FONT_STORAGE_KEY = "lich.terminal.font" const TERMINAL_FONT_SIZE_STORAGE_KEY = "lich.terminal.fontSize" @@ -23,20 +40,6 @@ const CONTEXT_USAGE_STORAGE_KEY = "lich.footer.contextUsage" // fontconfig, so it must be offered explicitly alongside the system fonts. export const DEFAULT_FONT = "FiraCode Nerd Font Mono" -// THEMES drives both the persisted value and the Appearance picker options. -// "system" follows the OS color scheme live. -export const THEMES = ["system", "light", "dark"] as const -export type Theme = (typeof THEMES)[number] -export const DEFAULT_THEME: Theme = "system" - -// TERMINAL_THEMES: "match" tracks the resolved app theme; the others force it. -export const TERMINAL_THEMES = ["match", "light", "dark"] as const -export type TerminalTheme = (typeof TERMINAL_THEMES)[number] -export const DEFAULT_TERMINAL_THEME: TerminalTheme = "match" - -// A theme resolved to a concrete color scheme (system/match already applied). -export type ResolvedTheme = "light" | "dark" - export const ZOOM_MIN = 0.5 export const ZOOM_MAX = 2 export const ZOOM_STEP = 0.1 @@ -72,10 +75,10 @@ const readTerminalFontSize = (): number => clampTerminalFontSize, ) -const readTheme = (): Theme => parseEnumPref(readPref(THEME_STORAGE_KEY), THEMES, DEFAULT_THEME) +const readTheme = (): Theme => sanitizeThemePreference(readPref(THEME_STORAGE_KEY)) const readTerminalTheme = (): TerminalTheme => - parseEnumPref(readPref(TERMINAL_THEME_STORAGE_KEY), TERMINAL_THEMES, DEFAULT_TERMINAL_THEME) + sanitizeTerminalThemePreference(readPref(TERMINAL_THEME_STORAGE_KEY)) const readZoom = (): number => parseNumberPref(readPref(ZOOM_STORAGE_KEY), DEFAULT_ZOOM, clampZoom) @@ -92,7 +95,11 @@ interface SettingsValue { /** Color theme applied to the whole app via the `.dark` class on . */ theme: Theme setTheme: (theme: Theme) => void - /** Theme resolved to a concrete scheme (system already mapped to the OS). */ + /** Bundled and imported themes available to the app. */ + themes: ThemeDefinition[] + importTheme: (raw: string) => Promise + removeTheme: (id: string) => Promise + /** Theme resolved to concrete colors (system already mapped to the OS). */ resolvedTheme: ResolvedTheme /** UI zoom factor applied to the whole app (1 = 100%). */ zoom: number @@ -116,13 +123,30 @@ const SettingsContext = createContext(null) export function SettingsProvider({ children }: { children: ReactNode }) { const [font, setFontState] = useState(readFont) const [terminalFontSize, setTerminalFontSizeState] = useState(readTerminalFontSize) + const [themes, setThemes] = useState(BUNDLED_THEMES) const [theme, setThemeState] = useState(readTheme) - const [resolvedTheme, setResolvedTheme] = useState("light") + const [resolvedTheme, setResolvedTheme] = useState(BUNDLED_THEMES[0]) const [zoom, setZoomState] = useState(readZoom) const [terminalTheme, setTerminalThemeState] = useState(readTerminalTheme) const [hotkeys, setHotkeys] = useState(loadHotkeys) const [showContextUsage, setShowContextUsageState] = useState(readContextUsage) + useEffect(() => { + let cancelled = false + ThemeRPC.List() + .then((loaded) => { + if (!cancelled) { + setThemes(mergeThemes(loaded)) + } + }) + .catch((error) => { + console.warn("[settings] failed to load custom themes", error) + }) + return () => { + cancelled = true + } + }, []) + const setFont = useCallback((next: string) => { setFontState(next) writePref(FONT_STORAGE_KEY, next) @@ -160,6 +184,35 @@ export function SettingsProvider({ children }: { children: ReactNode }) { writePref(TERMINAL_THEME_STORAGE_KEY, next) }, []) + const importTheme = useCallback( + async (raw: string) => { + const imported = await ThemeRPC.Import(raw) + setThemes((prev) => mergeThemes([...prev, imported])) + setTheme(imported.id) + return imported + }, + [setTheme], + ) + + const removeTheme = useCallback(async (id: string) => { + await ThemeRPC.Remove(id) + setThemes((prev) => mergeThemes(prev.filter((item) => item.id !== id))) + setThemeState((prev) => { + if (prev !== id) { + return prev + } + writePref(THEME_STORAGE_KEY, DEFAULT_THEME) + return DEFAULT_THEME + }) + setTerminalThemeState((prev) => { + if (prev !== id) { + return prev + } + writePref(TERMINAL_THEME_STORAGE_KEY, DEFAULT_TERMINAL_THEME) + return DEFAULT_TERMINAL_THEME + }) + }, []) + const setHotkey = useCallback((id: HotkeyId, combo: Combo) => { setHotkeys((prev) => { const next = { ...prev, [id]: combo } @@ -181,20 +234,22 @@ export function SettingsProvider({ children }: { children: ReactNode }) { writePref(CONTEXT_USAGE_STORAGE_KEY, next) }, []) - // Toggle the `.dark` class on and track the resolved scheme. For - // "system", follow the OS scheme and keep following it live. + // Apply the resolved theme's CSS variables and toggle `.dark` for existing + // dark variants. For "system", follow the OS scheme and keep following it + // live. useEffect(() => { const media = window.matchMedia("(prefers-color-scheme: dark)") const apply = () => { - const dark = theme === "dark" || (theme === "system" && media.matches) - document.documentElement.classList.toggle("dark", dark) - setResolvedTheme(dark ? "dark" : "light") + const resolved = resolveTheme(theme, themes, media.matches) + applyAppTheme(resolved, document.documentElement) + document.documentElement.classList.toggle("dark", resolved.scheme === "dark") + setResolvedTheme(resolved) } apply() if (theme !== "system") return media.addEventListener("change", apply) return () => media.removeEventListener("change", apply) - }, [theme]) + }, [theme, themes]) // Scale the app by moving the root font size: every Tailwind spacing and type // utility resolves in rem (--spacing is 0.25rem, --text-* are rem), so one @@ -251,8 +306,7 @@ export function SettingsProvider({ children }: { children: ReactNode }) { } }, [zoomBy, setZoom]) - const resolvedTerminalTheme: ResolvedTheme = - terminalTheme === "match" ? resolvedTheme : terminalTheme + const resolvedTerminalTheme = resolveTerminalTheme(terminalTheme, resolvedTheme, themes) return ( 128 || strings.ContainsAny(trimmed, ";{}") { + return fmt.Errorf("%s.%s is not a safe CSS color value", group, key) + } + return nil +} + +func cloneTheme(theme Theme) Theme { + return Theme{ + ID: theme.ID, + Name: theme.Name, + Scheme: theme.Scheme, + Origin: theme.Origin, + App: cloneMap(theme.App), + Terminal: cloneMap(theme.Terminal), + } +} + +func cloneMap(in map[string]string) map[string]string { + out := make(map[string]string, len(in)) + for key, value := range in { + out[key] = value + } + return out +} diff --git a/internal/themes/themes_test.go b/internal/themes/themes_test.go new file mode 100644 index 000000000..fc683be53 --- /dev/null +++ b/internal/themes/themes_test.go @@ -0,0 +1,114 @@ +package themes + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" +) + +func customTheme(id string) Theme { + theme := cloneTheme(bundledThemes[0]) + theme.ID = id + theme.Name = "Custom" + theme.Origin = OriginCustom + return theme +} + +func rawTheme(t *testing.T, theme Theme) string { + t.Helper() + data, err := json.Marshal(theme) + if err != nil { + t.Fatalf("marshal theme: %v", err) + } + return string(data) +} + +func TestListIncludesBundledThemes(t *testing.T) { + s := NewInDir(t.TempDir()) + themes, err := s.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(themes) != 2 { + t.Fatalf("got %d themes, want 2", len(themes)) + } + if themes[0].ID != "light" || themes[0].Origin != OriginBundled { + t.Fatalf("first bundled theme = %#v", themes[0]) + } + if themes[1].ID != "dark" || themes[1].Origin != OriginBundled { + t.Fatalf("second bundled theme = %#v", themes[1]) + } +} + +func TestImportPersistsCustomTheme(t *testing.T) { + dir := t.TempDir() + s := NewInDir(dir) + imported, err := s.Import(rawTheme(t, customTheme("solar"))) + if err != nil { + t.Fatalf("Import: %v", err) + } + if imported.ID != "solar" || imported.Origin != OriginCustom { + t.Fatalf("imported = %#v", imported) + } + if _, err := os.Stat(filepath.Join(dir, "solar.json")); err != nil { + t.Fatalf("stat saved theme: %v", err) + } + listed, err := s.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(listed) != 3 || listed[2].ID != "solar" { + t.Fatalf("listed themes = %#v", listed) + } +} + +func TestImportRejectsReservedID(t *testing.T) { + s := NewInDir(t.TempDir()) + _, err := s.Import(rawTheme(t, customTheme("dark"))) + if err == nil { + t.Fatal("Import reserved id succeeded") + } +} + +func TestImportRejectsMissingToken(t *testing.T) { + theme := customTheme("missing-token") + delete(theme.App, "background") + s := NewInDir(t.TempDir()) + _, err := s.Import(rawTheme(t, theme)) + if err == nil { + t.Fatal("Import without required token succeeded") + } +} + +func TestRemoveDeletesOnlyCustomTheme(t *testing.T) { + dir := t.TempDir() + s := NewInDir(dir) + if _, err := s.Import(rawTheme(t, customTheme("removable"))); err != nil { + t.Fatalf("Import: %v", err) + } + if err := s.Remove("removable"); err != nil { + t.Fatalf("Remove custom: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, "removable.json")); !os.IsNotExist(err) { + t.Fatalf("saved theme still exists: %v", err) + } + if err := s.Remove("light"); err == nil { + t.Fatal("Remove bundled theme succeeded") + } +} + +func TestListSkipsInvalidCustomTheme(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "bad.json"), []byte("{"), 0o600); err != nil { + t.Fatalf("write bad theme: %v", err) + } + s := NewInDir(dir) + themes, err := s.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(themes) != 2 { + t.Fatalf("got %d themes, want bundled only", len(themes)) + } +} diff --git a/main.go b/main.go index 4f5bf9dd9..a955346ca 100644 --- a/main.go +++ b/main.go @@ -25,6 +25,7 @@ import ( "github.com/omartelo/lich/internal/store" "github.com/omartelo/lich/internal/system" "github.com/omartelo/lich/internal/terminal" + "github.com/omartelo/lich/internal/themes" ) // The frontend is embedded into the binary and served over the loopback @@ -102,6 +103,7 @@ func main() { dispatcher.Register("store", db) dispatcher.Register("system", system.New(env)) dispatcher.Register("providers", providers.New()) + dispatcher.Register("themes", themes.New()) dispatcher.Deny("store.Close") term.Mount("/rpc/", dispatcher) term.Mount("/events", hub) From a7d9aec46a891c1aced134a8ced1ee67fae6116a Mon Sep 17 00:00:00 2001 From: JoJowAkaGioGiow Date: Thu, 30 Jul 2026 15:40:50 -0300 Subject: [PATCH 02/12] fix(themes): improve custom theme management --- docs/themes.md | 79 ++++++ frontend/DESIGN.md | 1 + .../settings/AppearanceSettings.tsx | 128 +++++++--- frontend/src/lib/themes.test.ts | 13 + frontend/src/lib/themes.ts | 47 +--- internal/themes/bundled.go | 42 +--- internal/themes/themes_test.go | 228 ++++++++++++++++++ 7 files changed, 430 insertions(+), 108 deletions(-) create mode 100644 docs/themes.md diff --git a/docs/themes.md b/docs/themes.md new file mode 100644 index 000000000..9b4ef26cc --- /dev/null +++ b/docs/themes.md @@ -0,0 +1,79 @@ +# Theme JSON + +lich ships bundled `light` and `dark` themes and accepts user-imported JSON themes. +Imported themes are stored under the user config directory: + +- Production: `/lich/themes/.json` +- Dev mode (`LICH_DEV`): `/lich/themes-dev/.json` + +The file name is managed by lich on import. A stored custom theme whose file name +does not match `.json` is ignored. + +## Import Shape + +```json +{ + "id": "tokyo-night", + "name": "Tokyo Night", + "scheme": "dark", + "app": { + "background": "oklch(0.18 0.05 276)", + "foreground": "oklch(0.92 0.03 270)", + "card": "oklch(0.22 0.05 276)", + "card-foreground": "oklch(0.92 0.03 270)", + "popover": "oklch(0.22 0.05 276)", + "popover-foreground": "oklch(0.92 0.03 270)", + "primary": "oklch(0.75 0.13 255)", + "primary-foreground": "oklch(0.14 0.04 276)", + "secondary": "oklch(0.27 0.05 276)", + "secondary-foreground": "oklch(0.88 0.04 270)", + "muted": "oklch(0.25 0.04 276)", + "muted-foreground": "oklch(0.65 0.05 270)", + "accent": "oklch(0.32 0.07 276)", + "accent-foreground": "oklch(0.92 0.03 270)", + "destructive": "oklch(0.7 0.18 25)", + "border": "oklch(0.95 0.02 270 / 12%)", + "input": "oklch(0.95 0.02 270 / 16%)", + "ring": "oklch(0.75 0.13 255)", + "chart-1": "oklch(0.75 0.13 255)", + "chart-2": "oklch(0.78 0.13 150)", + "chart-3": "oklch(0.82 0.13 85)", + "chart-4": "oklch(0.73 0.15 320)", + "chart-5": "oklch(0.76 0.14 25)", + "sidebar": "oklch(0.16 0.04 276)", + "sidebar-foreground": "oklch(0.92 0.03 270)", + "sidebar-primary": "oklch(0.75 0.13 255)", + "sidebar-primary-foreground": "oklch(0.14 0.04 276)", + "sidebar-accent": "oklch(0.27 0.05 276)", + "sidebar-accent-foreground": "oklch(0.92 0.03 270)", + "sidebar-border": "oklch(0.95 0.02 270 / 12%)", + "sidebar-ring": "oklch(0.75 0.13 255)" + }, + "terminal": { + "background": "#1a1b26", + "foreground": "#c0caf5", + "cursor": "#c0caf5", + "selectionBackground": "#33467c" + } +} +``` + +`origin` is optional in imported JSON. The backend overwrites it with `custom` +before saving. + +## Validation Rules + +- `id` is required, must match `^[a-z0-9][a-z0-9._-]{0,63}$`, and cannot be + `light`, `dark`, `system`, or `match`. +- `name` is required and cannot be blank. +- `scheme` must be `light` or `dark`. +- `app` is required and must include every token shown in the example. +- `terminal.background` and `terminal.foreground` are required. +- Optional terminal tokens are: `cursor`, `cursorAccent`, `selectionBackground`, + `selectionForeground`, `black`, `red`, `green`, `yellow`, `blue`, `magenta`, + `cyan`, `white`, `brightBlack`, `brightRed`, `brightGreen`, `brightYellow`, + `brightBlue`, `brightMagenta`, `brightCyan`, `brightWhite`. +- Unknown app or terminal tokens are rejected. +- Color values are CSS color strings, cannot be blank, cannot exceed 128 + characters, and cannot contain `;`, `{`, or `}`. + diff --git a/frontend/DESIGN.md b/frontend/DESIGN.md index 48f195558..69817e20e 100644 --- a/frontend/DESIGN.md +++ b/frontend/DESIGN.md @@ -30,6 +30,7 @@ state first. The bundled light/dark themes are zinc and achromatic — there is **no brand accent hue** by default. Palettes are theme JSON data applied to the tokens mapped in `src/index.css` (`@theme` + CSS variables). Never hardcode a hex or oklch value in a component; always go through a token. +The importable theme JSON contract lives in `docs/themes.md`. Surface roles (dark values shown; light is the mirror): diff --git a/frontend/src/components/settings/AppearanceSettings.tsx b/frontend/src/components/settings/AppearanceSettings.tsx index 5a6905b2b..428380210 100644 --- a/frontend/src/components/settings/AppearanceSettings.tsx +++ b/frontend/src/components/settings/AppearanceSettings.tsx @@ -1,5 +1,6 @@ import { CaseSensitive, + ChevronDown, Minus, Plus, SquareTerminal, @@ -8,7 +9,7 @@ import { ZoomIn, ZoomOut, } from "lucide-react" -import { useRef } from "react" +import { useRef, useState } from "react" import { toast } from "sonner" import { DEFAULT_TERMINAL_FONT_SIZE, @@ -22,6 +23,8 @@ import { useSettings, } from "@/providers/settings" import type { TerminalTheme, Theme } from "@/providers/settings" +import type { ThemeDefinition } from "@/lib/api-types" +import { ConfirmDialog } from "@/components/ConfirmDialog" import { Stepper } from "@/components/common/Stepper" import { SettingBlock, SettingGroup } from "./SettingBlock" import { FontSetting } from "./FontSetting" @@ -35,7 +38,8 @@ import { SelectTrigger, SelectValue, } from "@/components/ui/select" -import { customTheme } from "@/lib/themes" +import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip" +import { customThemes } from "@/lib/themes" import { errorText } from "@/lib/utils" // Appearance holds every look-and-feel control, split into an Interface group @@ -44,6 +48,8 @@ import { errorText } from "@/lib/utils" // context, so the block titles drop their "Interface"/"Terminal" prefix. export function AppearanceSettings() { const fileRef = useRef(null) + const [themePendingRemoval, setThemePendingRemoval] = useState(null) + const [importedThemesOpen, setImportedThemesOpen] = useState(false) const { themes, theme, @@ -57,7 +63,8 @@ export function AppearanceSettings() { terminalTheme, setTerminalTheme, } = useSettings() - const removableTheme = customTheme(themes, theme) + const importedThemes = customThemes(themes) + const hasCustomThemes = importedThemes.length > 0 const onImportTheme = async (file: File | undefined) => { if (!file) { @@ -76,12 +83,13 @@ export function AppearanceSettings() { } const onRemoveTheme = async () => { - if (!removableTheme) { + if (!themePendingRemoval) { return } try { - await removeTheme(removableTheme.id) - toast.success(`Removed theme: ${removableTheme.name}`) + await removeTheme(themePendingRemoval.id) + toast.success(`Removed theme: ${themePendingRemoval.name}`) + setThemePendingRemoval(null) } catch (error) { toast.error(`Theme removal failed: ${errorText(error)}`) } @@ -114,16 +122,14 @@ export function AppearanceSettings() { ))} - {themes.some((item) => item.origin === "custom") && ( + {hasCustomThemes && ( Custom - {themes - .filter((item) => item.origin === "custom") - .map((item) => ( - - {item.name} - - ))} + {importedThemes.map((item) => ( + + {item.name} + + ))} )} @@ -139,16 +145,78 @@ export function AppearanceSettings() { Import JSON -
+ {hasCustomThemes && ( +
+ + {importedThemesOpen && ( +
+ {importedThemes.map((item) => ( +
+ + + {item.name} + + {item.id} + + + + setThemePendingRemoval(item)} + /> + } + > + + + {`Remove ${item.name}`} + +
+ ))} +
+ )} +
+ )} + setThemePendingRemoval(null)} + title="Remove imported theme?" + description={ + <> + Delete {themePendingRemoval?.name}? This + removes the imported theme file from lich. + + } + > + + ))} - {themes.some((item) => item.origin === "custom") && ( + {hasCustomThemes && ( Custom - {themes - .filter((item) => item.origin === "custom") - .map((item) => ( - - {item.name} - - ))} + {importedThemes.map((item) => ( + + {item.name} + + ))} )} diff --git a/frontend/src/lib/themes.test.ts b/frontend/src/lib/themes.test.ts index da4f247c1..6340a01b0 100644 --- a/frontend/src/lib/themes.test.ts +++ b/frontend/src/lib/themes.test.ts @@ -4,6 +4,7 @@ import { APP_COLOR_TOKENS, applyAppTheme, BUNDLED_THEMES, + customThemes, mergeThemes, resolveTerminalTheme, resolveTheme, @@ -29,6 +30,11 @@ describe("themes", () => { expect(themes.map((theme) => theme.id)).toEqual(["light", "dark", "custom"]) }) + it("returns only imported custom themes", () => { + const themes = mergeThemes([customTheme("custom-a"), customTheme("custom-b")]) + expect(customThemes(themes).map((theme) => theme.id)).toEqual(["custom-a", "custom-b"]) + }) + it("resolves system to the bundled light or dark theme", () => { expect(resolveTheme("system", BUNDLED_THEMES, false).id).toBe("light") expect(resolveTheme("system", BUNDLED_THEMES, true).id).toBe("dark") @@ -63,4 +69,11 @@ describe("themes", () => { expect(values.get(`--${token}`)).toBe(BUNDLED_THEMES[0].app[token]) } }) + + it("keeps bundled themes on the same app token set", () => { + const tokens = [...APP_COLOR_TOKENS].sort() + for (const theme of BUNDLED_THEMES) { + expect(Object.keys(theme.app).sort()).toEqual(tokens) + } + }) }) diff --git a/frontend/src/lib/themes.ts b/frontend/src/lib/themes.ts index 35b23fe70..f69703883 100644 --- a/frontend/src/lib/themes.ts +++ b/frontend/src/lib/themes.ts @@ -13,45 +13,12 @@ export type Theme = typeof SYSTEM_THEME | string export type TerminalTheme = typeof MATCH_TERMINAL_THEME | string export type ResolvedTheme = ThemeDefinition -export const APP_COLOR_TOKENS = [ - "background", - "foreground", - "card", - "card-foreground", - "popover", - "popover-foreground", - "primary", - "primary-foreground", - "secondary", - "secondary-foreground", - "muted", - "muted-foreground", - "accent", - "accent-foreground", - "destructive", - "border", - "input", - "ring", - "chart-1", - "chart-2", - "chart-3", - "chart-4", - "chart-5", - "sidebar", - "sidebar-foreground", - "sidebar-primary", - "sidebar-primary-foreground", - "sidebar-accent", - "sidebar-accent-foreground", - "sidebar-border", - "sidebar-ring", -] as const - const BUNDLED = [lightTheme, darkTheme].map((theme) => normalizeTheme(theme as unknown as ThemeDefinition, "bundled"), ) const BUNDLED_ORDER = new Map(BUNDLED.map((theme, index) => [theme.id, index])) +export const APP_COLOR_TOKENS = Object.keys(BUNDLED[0].app) export const BUNDLED_THEMES = mergeThemes(BUNDLED) export function mergeThemes( @@ -104,16 +71,8 @@ export function applyAppTheme(theme: ThemeDefinition, root: HTMLElement): void { } } -export function themeExists(themes: readonly ThemeDefinition[], id: string): boolean { - return themes.some((theme) => theme.id === id) -} - -export function customTheme( - themes: readonly ThemeDefinition[], - id: string, -): ThemeDefinition | null { - const theme = themes.find((item) => item.id === id) - return theme?.origin === "custom" ? theme : null +export function customThemes(themes: readonly ThemeDefinition[]): ThemeDefinition[] { + return themes.filter((theme) => theme.origin === "custom") } export function sanitizeThemePreference(raw: string | null): Theme { diff --git a/internal/themes/bundled.go b/internal/themes/bundled.go index 58eb089a6..9b94b8255 100644 --- a/internal/themes/bundled.go +++ b/internal/themes/bundled.go @@ -8,39 +8,15 @@ func set(keys ...string) map[string]struct{} { return out } -var appTokens = set( - "background", - "foreground", - "card", - "card-foreground", - "popover", - "popover-foreground", - "primary", - "primary-foreground", - "secondary", - "secondary-foreground", - "muted", - "muted-foreground", - "accent", - "accent-foreground", - "destructive", - "border", - "input", - "ring", - "chart-1", - "chart-2", - "chart-3", - "chart-4", - "chart-5", - "sidebar", - "sidebar-foreground", - "sidebar-primary", - "sidebar-primary-foreground", - "sidebar-accent", - "sidebar-accent-foreground", - "sidebar-border", - "sidebar-ring", -) +func keySet(values map[string]string) map[string]struct{} { + out := make(map[string]struct{}, len(values)) + for key := range values { + out[key] = struct{}{} + } + return out +} + +var appTokens = keySet(bundledThemes[0].App) var terminalTokens = set( "background", diff --git a/internal/themes/themes_test.go b/internal/themes/themes_test.go index fc683be53..ef4190803 100644 --- a/internal/themes/themes_test.go +++ b/internal/themes/themes_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "os" "path/filepath" + "reflect" + "strings" "testing" ) @@ -71,6 +73,63 @@ func TestImportRejectsReservedID(t *testing.T) { } } +func TestImportRejectsMalformedJSON(t *testing.T) { + s := NewInDir(t.TempDir()) + if _, err := s.Import("{"); err == nil { + t.Fatal("Import malformed JSON succeeded") + } +} + +func TestImportRejectsInvalidID(t *testing.T) { + tests := []struct { + name string + id string + }{ + {name: "empty", id: ""}, + {name: "uppercase", id: "Bad"}, + {name: "leading dash", id: "-bad"}, + {name: "too long", id: strings.Repeat("a", 65)}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := NewInDir(t.TempDir()) + if _, err := s.Import(rawTheme(t, customTheme(tt.id))); err == nil { + t.Fatal("Import invalid id succeeded") + } + }) + } +} + +func TestImportRejectsInvalidMetadata(t *testing.T) { + tests := []struct { + name string + mutate func(*Theme) + }{ + { + name: "missing name", + mutate: func(theme *Theme) { + theme.Name = " " + }, + }, + { + name: "invalid scheme", + mutate: func(theme *Theme) { + theme.Scheme = "dim" + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + theme := customTheme("invalid-metadata") + tt.mutate(&theme) + s := NewInDir(t.TempDir()) + if _, err := s.Import(rawTheme(t, theme)); err == nil { + t.Fatal("Import invalid metadata succeeded") + } + }) + } +} + func TestImportRejectsMissingToken(t *testing.T) { theme := customTheme("missing-token") delete(theme.App, "background") @@ -81,6 +140,85 @@ func TestImportRejectsMissingToken(t *testing.T) { } } +func TestImportRejectsInvalidColorValues(t *testing.T) { + tests := []struct { + name string + mutate func(*Theme) + }{ + { + name: "unknown app color", + mutate: func(theme *Theme) { + theme.App["unknown"] = "#fff" + }, + }, + { + name: "empty app color", + mutate: func(theme *Theme) { + theme.App["background"] = " " + }, + }, + { + name: "unsafe app color", + mutate: func(theme *Theme) { + theme.App["foreground"] = "red; color: blue" + }, + }, + { + name: "unknown terminal color", + mutate: func(theme *Theme) { + theme.Terminal["unknown"] = "#fff" + }, + }, + { + name: "empty terminal color", + mutate: func(theme *Theme) { + theme.Terminal["cursor"] = " " + }, + }, + { + name: "unsafe terminal color", + mutate: func(theme *Theme) { + theme.Terminal["cursor"] = "red{}" + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + theme := customTheme("invalid-color") + tt.mutate(&theme) + s := NewInDir(t.TempDir()) + if _, err := s.Import(rawTheme(t, theme)); err == nil { + t.Fatal("Import invalid color succeeded") + } + }) + } +} + +func TestImportRejectsMissingTerminalRequiredColors(t *testing.T) { + for _, key := range []string{"background", "foreground"} { + t.Run(key, func(t *testing.T) { + theme := customTheme("missing-terminal") + delete(theme.Terminal, key) + s := NewInDir(t.TempDir()) + if _, err := s.Import(rawTheme(t, theme)); err == nil { + t.Fatal("Import without required terminal color succeeded") + } + }) + } +} + +func TestImportReturnsDirectoryError(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "themes") + if err := os.WriteFile(path, []byte("not a directory"), 0o600); err != nil { + t.Fatalf("write dir blocker: %v", err) + } + s := NewInDir(path) + if _, err := s.Import(rawTheme(t, customTheme("dir-error"))); err == nil { + t.Fatal("Import with file as themes dir succeeded") + } +} + func TestRemoveDeletesOnlyCustomTheme(t *testing.T) { dir := t.TempDir() s := NewInDir(dir) @@ -98,6 +236,30 @@ func TestRemoveDeletesOnlyCustomTheme(t *testing.T) { } } +func TestListSortsCustomThemesByName(t *testing.T) { + dir := t.TempDir() + s := NewInDir(dir) + zeta := customTheme("zeta") + zeta.Name = "Zeta" + alpha := customTheme("alpha") + alpha.Name = "alpha" + if _, err := s.Import(rawTheme(t, zeta)); err != nil { + t.Fatalf("Import zeta: %v", err) + } + if _, err := s.Import(rawTheme(t, alpha)); err != nil { + t.Fatalf("Import alpha: %v", err) + } + themes, err := s.List() + if err != nil { + t.Fatalf("List: %v", err) + } + got := []string{themes[0].ID, themes[1].ID, themes[2].ID, themes[3].ID} + want := []string{"light", "dark", "alpha", "zeta"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("theme order = %v, want %v", got, want) + } +} + func TestListSkipsInvalidCustomTheme(t *testing.T) { dir := t.TempDir() if err := os.WriteFile(filepath.Join(dir, "bad.json"), []byte("{"), 0o600); err != nil { @@ -112,3 +274,69 @@ func TestListSkipsInvalidCustomTheme(t *testing.T) { t.Fatalf("got %d themes, want bundled only", len(themes)) } } + +func TestListSkipsFilenameMismatch(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "wrong.json"), []byte(rawTheme(t, customTheme("right"))), 0o600); err != nil { + t.Fatalf("write mismatched theme: %v", err) + } + s := NewInDir(dir) + themes, err := s.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(themes) != 2 { + t.Fatalf("got %d themes, want bundled only", len(themes)) + } +} + +func TestListReturnsDirectoryError(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "themes") + if err := os.WriteFile(path, []byte("not a directory"), 0o600); err != nil { + t.Fatalf("write dir blocker: %v", err) + } + s := NewInDir(path) + if _, err := s.List(); err == nil { + t.Fatal("List with file as themes dir succeeded") + } +} + +func TestBundledThemesAreCloned(t *testing.T) { + s := NewInDir(t.TempDir()) + first, err := s.List() + if err != nil { + t.Fatalf("List: %v", err) + } + first[0].App["background"] = "changed" + first[0].Terminal["foreground"] = "changed" + + second, err := s.List() + if err != nil { + t.Fatalf("List again: %v", err) + } + if second[0].App["background"] == "changed" || second[0].Terminal["foreground"] == "changed" { + t.Fatalf("bundled theme was mutated: %#v", second[0]) + } +} + +func TestBundledThemesMatchFrontendJSON(t *testing.T) { + for _, theme := range bundledThemes { + t.Run(theme.ID, func(t *testing.T) { + data, err := os.ReadFile(filepath.Join("..", "..", "frontend", "src", "themes", theme.ID+".json")) + if err != nil { + t.Fatalf("read frontend theme: %v", err) + } + var frontend Theme + if err := json.Unmarshal(data, &frontend); err != nil { + t.Fatalf("parse frontend theme: %v", err) + } + if !reflect.DeepEqual(theme, frontend) { + t.Fatalf("backend bundled theme differs from frontend JSON\nbackend: %#v\nfrontend: %#v", theme, frontend) + } + if !reflect.DeepEqual(keySet(theme.App), appTokens) { + t.Fatalf("app tokens for %s = %#v, want %#v", theme.ID, keySet(theme.App), appTokens) + } + }) + } +} From 4c9e02702c23d7791847d37e3ad78f43720d7e4d Mon Sep 17 00:00:00 2001 From: JoJowAkaGioGiow Date: Thu, 30 Jul 2026 17:02:45 -0300 Subject: [PATCH 03/12] feat(themes): add import template download --- docs/themes.md | 2 +- .../settings/AppearanceSettings.tsx | 32 +++++++++++++++++-- frontend/src/lib/themes.test.ts | 15 +++++++++ frontend/src/lib/themes.ts | 14 ++++++++ 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/docs/themes.md b/docs/themes.md index 9b4ef26cc..f2f70abba 100644 --- a/docs/themes.md +++ b/docs/themes.md @@ -1,6 +1,7 @@ # Theme JSON lich ships bundled `light` and `dark` themes and accepts user-imported JSON themes. +The Appearance settings can download a valid starter template. Imported themes are stored under the user config directory: - Production: `/lich/themes/.json` @@ -76,4 +77,3 @@ before saving. - Unknown app or terminal tokens are rejected. - Color values are CSS color strings, cannot be blank, cannot exceed 128 characters, and cannot contain `;`, `{`, or `}`. - diff --git a/frontend/src/components/settings/AppearanceSettings.tsx b/frontend/src/components/settings/AppearanceSettings.tsx index 428380210..db30774ce 100644 --- a/frontend/src/components/settings/AppearanceSettings.tsx +++ b/frontend/src/components/settings/AppearanceSettings.tsx @@ -1,6 +1,7 @@ import { CaseSensitive, ChevronDown, + Download, Minus, Plus, SquareTerminal, @@ -39,7 +40,7 @@ import { SelectValue, } from "@/components/ui/select" import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip" -import { customThemes } from "@/lib/themes" +import { customThemes, THEME_TEMPLATE_FILENAME, themeTemplateJSON } from "@/lib/themes" import { errorText } from "@/lib/utils" // Appearance holds every look-and-feel control, split into an Interface group @@ -95,6 +96,17 @@ export function AppearanceSettings() { } } + const onDownloadThemeTemplate = () => { + const url = URL.createObjectURL(new Blob([themeTemplateJSON()], { type: "application/json" })) + const link = document.createElement("a") + link.href = url + link.download = THEME_TEMPLATE_FILENAME + document.body.append(link) + link.click() + link.remove() + URL.revokeObjectURL(url) + } + return ( <> @@ -143,8 +155,24 @@ export function AppearanceSettings() { /> + + + } + > + + + Download Theme Template +
{hasCustomThemes && (
diff --git a/frontend/src/lib/themes.test.ts b/frontend/src/lib/themes.test.ts index 6340a01b0..48cacca51 100644 --- a/frontend/src/lib/themes.test.ts +++ b/frontend/src/lib/themes.test.ts @@ -10,6 +10,8 @@ import { resolveTheme, sanitizeTerminalThemePreference, sanitizeThemePreference, + THEME_TEMPLATE_FILENAME, + themeTemplateJSON, } from "./themes" function customTheme(id: string): ThemeDefinition { @@ -76,4 +78,17 @@ describe("themes", () => { expect(Object.keys(theme.app).sort()).toEqual(tokens) } }) + + it("builds a valid import template", () => { + const template = JSON.parse(themeTemplateJSON()) as Partial + + expect(THEME_TEMPLATE_FILENAME).toBe("lich-theme-template.json") + expect(template.id).toBe("my-theme") + expect(template.name).toBe("My Theme") + expect(template.scheme).toBe("light") + expect(template.origin).toBeUndefined() + expect(Object.keys(template.app ?? {}).sort()).toEqual([...APP_COLOR_TOKENS].sort()) + expect(template.terminal?.background).toBeTruthy() + expect(template.terminal?.foreground).toBeTruthy() + }) }) diff --git a/frontend/src/lib/themes.ts b/frontend/src/lib/themes.ts index f69703883..de321c4be 100644 --- a/frontend/src/lib/themes.ts +++ b/frontend/src/lib/themes.ts @@ -12,6 +12,7 @@ export const DARK_THEME_ID = "dark" export type Theme = typeof SYSTEM_THEME | string export type TerminalTheme = typeof MATCH_TERMINAL_THEME | string export type ResolvedTheme = ThemeDefinition +type ThemeImportTemplate = Omit const BUNDLED = [lightTheme, darkTheme].map((theme) => normalizeTheme(theme as unknown as ThemeDefinition, "bundled"), @@ -20,6 +21,7 @@ const BUNDLED_ORDER = new Map(BUNDLED.map((theme, index) => [theme.id, index])) export const APP_COLOR_TOKENS = Object.keys(BUNDLED[0].app) export const BUNDLED_THEMES = mergeThemes(BUNDLED) +export const THEME_TEMPLATE_FILENAME = "lich-theme-template.json" export function mergeThemes( themes: readonly ThemeDefinition[] | null | undefined, @@ -75,6 +77,18 @@ export function customThemes(themes: readonly ThemeDefinition[]): ThemeDefinitio return themes.filter((theme) => theme.origin === "custom") } +export function themeTemplateJSON(): string { + const base = BUNDLED[0] + const template = { + id: "my-theme", + name: "My Theme", + scheme: base.scheme, + app: { ...base.app }, + terminal: { ...base.terminal }, + } satisfies ThemeImportTemplate + return `${JSON.stringify(template, null, 2)}\n` +} + export function sanitizeThemePreference(raw: string | null): Theme { return safePreference(raw) ?? DEFAULT_THEME } From 71da2afb4b667953750d8b40081dcf827d35efc6 Mon Sep 17 00:00:00 2001 From: JoJowAkaGioGiow Date: Thu, 30 Jul 2026 17:34:58 -0300 Subject: [PATCH 04/12] feat: adapted template to be more intuitive --- docs/themes.md | 3 +- .../settings/AppearanceSettings.tsx | 1 + frontend/src/lib/themes.test.ts | 11 +++- frontend/src/lib/themes.ts | 66 +++++++++++++++++-- 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/docs/themes.md b/docs/themes.md index f2f70abba..835902746 100644 --- a/docs/themes.md +++ b/docs/themes.md @@ -1,7 +1,8 @@ # Theme JSON lich ships bundled `light` and `dark` themes and accepts user-imported JSON themes. -The Appearance settings can download a valid starter template. +The Appearance settings can download a valid dark starter template that uses +only hex colors. Imported themes are stored under the user config directory: - Production: `/lich/themes/.json` diff --git a/frontend/src/components/settings/AppearanceSettings.tsx b/frontend/src/components/settings/AppearanceSettings.tsx index db30774ce..866f1f510 100644 --- a/frontend/src/components/settings/AppearanceSettings.tsx +++ b/frontend/src/components/settings/AppearanceSettings.tsx @@ -105,6 +105,7 @@ export function AppearanceSettings() { link.click() link.remove() URL.revokeObjectURL(url) + toast.success("Template download started!") } return ( diff --git a/frontend/src/lib/themes.test.ts b/frontend/src/lib/themes.test.ts index 48cacca51..92a0d7c81 100644 --- a/frontend/src/lib/themes.test.ts +++ b/frontend/src/lib/themes.test.ts @@ -83,12 +83,17 @@ describe("themes", () => { const template = JSON.parse(themeTemplateJSON()) as Partial expect(THEME_TEMPLATE_FILENAME).toBe("lich-theme-template.json") - expect(template.id).toBe("my-theme") - expect(template.name).toBe("My Theme") - expect(template.scheme).toBe("light") + expect(template.id).toBe("purple-night") + expect(template.name).toBe("Purple Night") + expect(template.scheme).toBe("dark") expect(template.origin).toBeUndefined() expect(Object.keys(template.app ?? {}).sort()).toEqual([...APP_COLOR_TOKENS].sort()) expect(template.terminal?.background).toBeTruthy() expect(template.terminal?.foreground).toBeTruthy() + expect(Object.values({ ...template.app, ...template.terminal }).every(isHexColor)).toBe(true) }) }) + +function isHexColor(value: unknown): boolean { + return typeof value === "string" && /^#[0-9a-fA-F]{3,8}$/.test(value) +} diff --git a/frontend/src/lib/themes.ts b/frontend/src/lib/themes.ts index de321c4be..532e876da 100644 --- a/frontend/src/lib/themes.ts +++ b/frontend/src/lib/themes.ts @@ -78,13 +78,67 @@ export function customThemes(themes: readonly ThemeDefinition[]): ThemeDefinitio } export function themeTemplateJSON(): string { - const base = BUNDLED[0] const template = { - id: "my-theme", - name: "My Theme", - scheme: base.scheme, - app: { ...base.app }, - terminal: { ...base.terminal }, + id: "purple-night", + name: "Purple Night", + scheme: "dark", + app: { + background: "#15111f", + foreground: "#f4efff", + card: "#1f1930", + "card-foreground": "#f4efff", + popover: "#241d38", + "popover-foreground": "#f4efff", + primary: "#b794f6", + "primary-foreground": "#15111f", + secondary: "#2d2442", + "secondary-foreground": "#efe7ff", + muted: "#261f36", + "muted-foreground": "#b8a9d9", + accent: "#3a2d57", + "accent-foreground": "#f4efff", + destructive: "#ff5c8a", + border: "#4a3a68", + input: "#554178", + ring: "#c4a7ff", + "chart-1": "#b794f6", + "chart-2": "#8be9fd", + "chart-3": "#50fa7b", + "chart-4": "#ffb86c", + "chart-5": "#ff79c6", + sidebar: "#1b1628", + "sidebar-foreground": "#f4efff", + "sidebar-primary": "#b794f6", + "sidebar-primary-foreground": "#15111f", + "sidebar-accent": "#2d2442", + "sidebar-accent-foreground": "#f4efff", + "sidebar-border": "#4a3a68", + "sidebar-ring": "#c4a7ff", + }, + terminal: { + background: "#15111f", + foreground: "#f4efff", + cursor: "#c4a7ff", + cursorAccent: "#15111f", + selectionBackground: "#4a3a68", + selectionForeground: "#ffffff", + black: "#15111f", + red: "#ff5c8a", + green: "#50fa7b", + yellow: "#ffb86c", + blue: "#8be9fd", + magenta: "#bd93f9", + cyan: "#80ffea", + white: "#f4efff", + brightBlack: "#6f5a95", + brightRed: "#ff7aa8", + brightGreen: "#69ff94", + brightYellow: "#ffd18a", + brightBlue: "#a4f0ff", + brightMagenta: "#d5b7ff", + brightCyan: "#a0fff1", + brightWhite: "#ffffff", + }, } satisfies ThemeImportTemplate return `${JSON.stringify(template, null, 2)}\n` } From 943e1228b00150730aa574e78ee74959ff3fab08 Mon Sep 17 00:00:00 2001 From: martelo <125408621+omartelo@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:27:04 -0300 Subject: [PATCH 05/12] fix: disable the Merge button when GitHub will not merge (#132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge gate read `mergeable` alone, which reports only whether the branch conflicts. A pull request missing a required review, waiting on a required check, sitting behind its base branch, or one GitHub had not finished recomputing after a push all passed the gate — the button was offered, gh refused, and the toast said "it has conflicts or an unmet requirement" without naming which. Carry gh's `mergeStateStatus` — GitHub's own answer to "can this merge right now" — through to the panel and gate the button on it, with the reason on the hover. UNSTABLE stays mergeable: a failing or pending check that no rule requires does not block a merge on GitHub, and the checks readout already shows it red. The gate lives in `lib/pulls/merge-gate.ts` so the node-only suite can test it; an answer with no `mergeStateStatus` falls back to the old `mergeable` check rather than blocking a merge GitHub would have taken. --- CHANGELOG.md | 8 ++ .../src/components/pulls/PullRequestView.tsx | 12 +-- frontend/src/lib/api-types.ts | 3 + frontend/src/lib/pulls/merge-gate.test.ts | 89 +++++++++++++++++++ frontend/src/lib/pulls/merge-gate.ts | 45 ++++++++++ internal/project/pr.go | 10 ++- internal/project/pr_test.go | 4 + 7 files changed, 160 insertions(+), 11 deletions(-) create mode 100644 frontend/src/lib/pulls/merge-gate.test.ts create mode 100644 frontend/src/lib/pulls/merge-gate.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c86d06de..caf534850 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The Merge button says no before the click, not after.** Merging a pull + request GitHub would not take — a required review missing, a required check + still running, the base branch moved on, or GitHub simply still working out + whether the branch merges — offered the button anyway and answered with a + toast that named none of those reasons. The button is now disabled whenever + GitHub reports it cannot merge, and hovering it says which of them it is. A + failing check that no rule requires still merges: the checks readout shows it + red, and the call stays yours. - **Settings › Font lists your installed fonts on Windows.** The picker read the font list from fontconfig, which Windows does not have, so it offered nothing beyond the bundled default and whatever was already selected. It now reads the diff --git a/frontend/src/components/pulls/PullRequestView.tsx b/frontend/src/components/pulls/PullRequestView.tsx index 0c3ec695b..6c46bedc8 100644 --- a/frontend/src/components/pulls/PullRequestView.tsx +++ b/frontend/src/components/pulls/PullRequestView.tsx @@ -11,6 +11,7 @@ import { } from "lucide-react" import { ProjectService, System } from "@/lib/rpc" import type { MergeMethod, PullRequestDetail } from "@/lib/api-types" +import { mergeBlockedReason } from "@/lib/pulls/merge-gate" import { cn, errorText } from "@/lib/utils" import { Markdown } from "@/components/Markdown" import { Button } from "@/components/ui/button" @@ -72,16 +73,7 @@ export function PullRequestView({ const [edit, setEdit] = useState(null) const [tab, setTab] = useState("overview") const commitCount = detail.commits?.length ?? 0 - // A pull request the list reached by number may be over already, in which - // case there is nothing to merge and gh would refuse anyway. - const blocked = - detail.state !== "OPEN" - ? `Pull request is ${detail.state.toLowerCase()}` - : detail.isDraft - ? "Pull request is a draft" - : detail.mergeable === "CONFLICTING" - ? `Conflicts with ${detail.baseRefName}` - : null + const blocked = mergeBlockedReason(detail) const merge = async (method: MergeMethod, subject = "", body = "") => { setMerging(true) diff --git a/frontend/src/lib/api-types.ts b/frontend/src/lib/api-types.ts index 6d41acd39..6b2acd451 100644 --- a/frontend/src/lib/api-types.ts +++ b/frontend/src/lib/api-types.ts @@ -93,6 +93,9 @@ export interface PullRequestDetail { isDraft: boolean /** gh: MERGEABLE | CONFLICTING | UNKNOWN */ mergeable: string + /** GitHub's own "can this merge right now": CLEAN | UNSTABLE | BLOCKED | + * BEHIND | DIRTY | DRAFT | HAS_HOOKS | UNKNOWN. See mergeBlockedReason. */ + mergeStateStatus: string /** gh's aggregate verdict: APPROVED | CHANGES_REQUESTED | REVIEW_REQUIRED; * "" where the repository requires no review. */ reviewDecision: string diff --git a/frontend/src/lib/pulls/merge-gate.test.ts b/frontend/src/lib/pulls/merge-gate.test.ts new file mode 100644 index 000000000..3dbab6a81 --- /dev/null +++ b/frontend/src/lib/pulls/merge-gate.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it } from "vitest" +import { mergeBlockedReason } from "./merge-gate" +import type { PullRequestDetail } from "@/lib/api-types" + +const detail = (over: Partial = {}): PullRequestDetail => ({ + number: 130, + url: "https://github.com/o/l/pull/130", + title: "feat: something", + body: "", + state: "OPEN", + isDraft: false, + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: "", + baseRefName: "main", + headRefName: "quiet-willow", + changedFiles: 1, + isCrossRepository: false, + checks: { total: 0, passed: 0, failed: 0, pending: 0 }, + checkRuns: null, + commits: null, + ...over, +}) + +describe("mergeBlockedReason", () => { + it("lets a clean pull request through", () => { + expect(mergeBlockedReason(detail())).toBeNull() + }) + + it("lets a merge over a non-required red check through", () => { + expect(mergeBlockedReason(detail({ mergeStateStatus: "UNSTABLE" }))).toBeNull() + }) + + it("lets a repository with merge hooks through", () => { + expect(mergeBlockedReason(detail({ mergeStateStatus: "HAS_HOOKS" }))).toBeNull() + }) + + it("blocks a pull request that is already over", () => { + expect(mergeBlockedReason(detail({ state: "MERGED" }))).toBe("Pull request is merged") + expect(mergeBlockedReason(detail({ state: "CLOSED" }))).toBe("Pull request is closed") + }) + + it("blocks a draft before reading the merge state", () => { + expect(mergeBlockedReason(detail({ isDraft: true, mergeStateStatus: "DRAFT" }))).toBe( + "Pull request is a draft", + ) + }) + + it("names the base branch on a conflict", () => { + const reason = mergeBlockedReason( + detail({ mergeStateStatus: "DIRTY", mergeable: "CONFLICTING", baseRefName: "develop" }), + ) + expect(reason).toBe("Conflicts with develop") + }) + + it("blocks an unmet review or required check", () => { + expect(mergeBlockedReason(detail({ mergeStateStatus: "BLOCKED" }))).toBe( + "GitHub requires a review or a status check that has not passed", + ) + }) + + it("blocks a branch the base has moved past", () => { + expect(mergeBlockedReason(detail({ mergeStateStatus: "BEHIND" }))).toBe( + "Base branch has moved — update this branch first", + ) + }) + + // The state that produced the flat "not mergeable" toast: GitHub had not + // finished recomputing after a push, and the old gate only looked at + // mergeable === CONFLICTING. + it("blocks while GitHub is still recomputing", () => { + expect(mergeBlockedReason(detail({ mergeStateStatus: "UNKNOWN", mergeable: "UNKNOWN" }))).toBe( + "GitHub is still working out whether this can merge", + ) + }) + + it("blocks a state this build has never seen", () => { + expect(mergeBlockedReason(detail({ mergeStateStatus: "SOMETHING_NEW" }))).toBe( + "GitHub will not merge this pull request yet", + ) + }) + + it("falls back to mergeable when no merge state came back", () => { + expect(mergeBlockedReason(detail({ mergeStateStatus: "" }))).toBeNull() + expect(mergeBlockedReason(detail({ mergeStateStatus: "", mergeable: "CONFLICTING" }))).toBe( + "Conflicts with main", + ) + }) +}) diff --git a/frontend/src/lib/pulls/merge-gate.ts b/frontend/src/lib/pulls/merge-gate.ts new file mode 100644 index 000000000..ef01255c5 --- /dev/null +++ b/frontend/src/lib/pulls/merge-gate.ts @@ -0,0 +1,45 @@ +import type { PullRequestDetail } from "@/lib/api-types" + +// The merge states GitHub reports that it will still merge from. Everything +// else it refuses, and gh's refusal reaches the screen as one flat sentence +// (internal/project/gherror.go) that names none of the reasons below — so the +// button has to answer this itself, before the click. +// +// UNSTABLE is in: a failing or pending check that no rule requires does not +// block a merge on GitHub, and the checks readout already shows it red. Whether +// to merge over it is the reader's call, not the button's. +const MERGEABLE_STATES = new Set(["CLEAN", "UNSTABLE", "HAS_HOOKS"]) + +// Why GitHub refuses each of the states it will not merge from. Keyed by gh's +// mergeStateStatus. DRAFT is absent because isDraft says it first and says it +// better; DIRTY is absent because its reason names the base branch. +const BLOCKED_REASONS: Record = { + BLOCKED: "GitHub requires a review or a status check that has not passed", + BEHIND: "Base branch has moved — update this branch first", + UNKNOWN: "GitHub is still working out whether this can merge", +} + +// mergeBlockedReason names why a pull request cannot be merged right now, or +// returns null when it can. The order is the order a reader would ask in: is +// there anything to merge, is it ready to be merged, will GitHub take it. +export function mergeBlockedReason(detail: PullRequestDetail): string | null { + if (detail.state !== "OPEN") { + return `Pull request is ${detail.state.toLowerCase()}` + } + if (detail.isDraft) { + return "Pull request is a draft" + } + // Older answers, and any state gh adds after this build, carry no + // mergeStateStatus. Fall back to the coarse field rather than blocking a + // merge GitHub would have taken. + if (detail.mergeStateStatus === "") { + return detail.mergeable === "CONFLICTING" ? `Conflicts with ${detail.baseRefName}` : null + } + if (MERGEABLE_STATES.has(detail.mergeStateStatus)) { + return null + } + if (detail.mergeStateStatus === "DIRTY") { + return `Conflicts with ${detail.baseRefName}` + } + return BLOCKED_REASONS[detail.mergeStateStatus] ?? "GitHub will not merge this pull request yet" +} diff --git a/internal/project/pr.go b/internal/project/pr.go index ca7377376..0a21bc65a 100644 --- a/internal/project/pr.go +++ b/internal/project/pr.go @@ -65,7 +65,7 @@ func runGH(timeout time.Duration, dir, token string, args ...string) ([]byte, er } // prViewFields is the gh `pr view --json` selection backing the Pulls panel. -const prViewFields = "number,url,state,title,body,isDraft,mergeable,reviewDecision,baseRefName,headRefName,isCrossRepository,statusCheckRollup,changedFiles,commits" +const prViewFields = "number,url,state,title,body,isDraft,mergeable,mergeStateStatus,reviewDecision,baseRefName,headRefName,isCrossRepository,statusCheckRollup,changedFiles,commits" // prSelector renders the pull request argument gh's subcommands take ahead of // their flags. Zero means "the branch checked out at path" — gh's own default, @@ -101,6 +101,12 @@ type PRDetail struct { State string `json:"state"` IsDraft bool `json:"isDraft"` Mergeable string `json:"mergeable"` // gh: MERGEABLE | CONFLICTING | UNKNOWN + // MergeStateStatus is GitHub's own answer to "can this merge right now" — + // CLEAN | UNSTABLE | BLOCKED | BEHIND | DIRTY | DRAFT | HAS_HOOKS | UNKNOWN. + // Mergeable alone cannot gate the button: it says nothing about a required + // review or a required check, and it reads UNKNOWN while GitHub recomputes + // after a push. + MergeStateStatus string `json:"mergeStateStatus"` // ReviewDecision is gh's aggregate verdict — APPROVED | CHANGES_REQUESTED | // REVIEW_REQUIRED — and "" where the repository requires no review. Who // reviewed is a different field (latestReviews) this does not carry. @@ -137,6 +143,7 @@ type ghPRView struct { Body string `json:"body"` IsDraft bool `json:"isDraft"` Mergeable string `json:"mergeable"` + MergeStateStatus string `json:"mergeStateStatus"` ReviewDecision string `json:"reviewDecision"` BaseRefName string `json:"baseRefName"` HeadRefName string `json:"headRefName"` @@ -201,6 +208,7 @@ func parsePRDetail(out []byte, openOnly bool) (*PRDetail, error) { State: v.State, IsDraft: v.IsDraft, Mergeable: v.Mergeable, + MergeStateStatus: v.MergeStateStatus, ReviewDecision: v.ReviewDecision, BaseRefName: v.BaseRefName, HeadRefName: v.HeadRefName, diff --git a/internal/project/pr_test.go b/internal/project/pr_test.go index df72efaba..567a3ee9f 100644 --- a/internal/project/pr_test.go +++ b/internal/project/pr_test.go @@ -18,6 +18,7 @@ func TestParsePRDetail(t *testing.T) { "body": "makes the badge actionable", "isDraft": false, "mergeable": "MERGEABLE", + "mergeStateStatus": "UNSTABLE", "baseRefName": "main", "headRefName": "quiet-willow", "changedFiles": 6, @@ -42,6 +43,9 @@ func TestParsePRDetail(t *testing.T) { if pr.Mergeable != "MERGEABLE" || pr.BaseRefName != "main" || pr.HeadRefName != "quiet-willow" { t.Errorf("wrong refs/mergeable: %+v", pr) } + if pr.MergeStateStatus != "UNSTABLE" { + t.Errorf("wrong merge state status: %q", pr.MergeStateStatus) + } if pr.Checks.Passed != 2 || pr.Checks.Total != 2 || pr.Checks.Failed != 0 { t.Errorf("wrong checks: %+v", pr.Checks) } From de153c90f8e080a5a731949a8fbe496521defc80 Mon Sep 17 00:00:00 2001 From: martelo <125408621+omartelo@users.noreply.github.com> Date: Thu, 30 Jul 2026 20:31:07 -0300 Subject: [PATCH 06/12] feat: read and write pull request reviews inside the diff (#130) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: read and write pull request reviews inside the diff Pulls only ever showed the parts of a pull request that travel one way: the checks rollup, the commits, the changed files, and the two writes that end it. The conversation was missing entirely, so a review that asked for a change was invisible here and reading it meant the browser — which is the thing this screen set out to stop. The whole review now happens in the diff. A thread opens inline, in a gap between two lines of the file it is about. The gap is a CodeMirror block widget that owns nothing but an empty element; React fills it through a portal. That split is the point: a comment box holds text somebody is typing and the editor rebuilds its DOM whenever it likes, so the widget is compared by a key that outlives a rebuild and the text lives outside it. Slots arrive through a StateEffect rather than through the view's extensions — resolving a thread must not rebuild the editor under the cursor. Comments of your own are written the same way and held back. The diff's context menu grows "Comment on lines N-M" beside "Inject lines N-M" — the same gesture, the two destinations it has always had to choose between: the session next door, or GitHub. Each comment waits with the others until Submit review sends them as one call, with a verdict and a summary above them. That is what makes it a review rather than a pile of separate remarks, and it is why the pending state is a store backed by localStorage: the panel refetches on focus and while checks run, a collapsed file destroys its editor, and an app update reloads the page. A review written over an afternoon has to survive all three. Everything a line cannot hold goes to a new Conversation tab, in the order it was said: the verdicts, the comments on the pull request itself, and the threads whose lines the branch has since rewritten — GitHub nulls their anchor, so no diff can show them. Settled threads fold themselves away behind a count wherever they render. Behind it: - One `gh api graphql` read brings back reviews, comments and threads together. gh's own `pr view` cannot answer this: --json reviews has a review's body but never the threads under it, and no REST endpoint reports whether a thread was resolved. - A thread carries two identifiers because GitHub needs both — the node id resolves it, the comment's database id is what a reply is addressed to. - A submitted review is one REST call carrying every line comment. Its body goes to gh through a temporary file: --field cannot express an array of objects, and widening every gh call site with a stdin argument to serve this one call is the worse trade. - ApprovePullRequest is gone. Approving is that same submit with no body and no comments, so the button keeps its single click and grows into "Submit review N" the moment a comment is waiting. - A reply posts immediately; a new comment always waits. It is GitHub's own split between answering something already said and starting a review. Deliberately new-file lines only for a comment of your own: a comment on a deleted line is a thread GitHub anchors on the other side of the diff, and that needs a gesture of its own. Verified by driving a headless instance over CDP, since the frontend suite is node-only and cannot render a widget: a draft planted in the store reappears inside the right file's diff after a reload, a real drag-selection produces the menu item, and the composer it opens files the comment as pending with its line span intact. Co-authored-by: Claude * fix: give TooltipTrigger its ref back, and key skeleton bars by position Two React warnings the Pulls screen printed on every visit. Neither was new — the review work just made them loud enough to read. TooltipTrigger was a plain function component, so the ref base-ui hands a trigger nested inside another part went nowhere. The session card does exactly that (a TooltipTrigger inside a ContextMenuTrigger), which is the composition ui/button.tsx already forwards for, and the same trap the frontend's CLAUDE.md names: the only sign is a console warning, while the part quietly never registers its node. SkeletonLines keyed each bar by its width class. The widths are ragged on purpose and repeat, so two bars claimed the same key and React folded them into one — a placeholder stack shorter than the text it stands in for. Keyed by position now; the list is static, so the index is the identity. The Pulls screen's console is empty after both. * feat: let any review thread fold away, not only a settled one A resolved thread could be folded to a line and opened again; an open one could not, so the thread you had just read and dealt with kept as much of the diff as the argument it started — and the only way to get the code back was to resolve something that was not resolved. Folding is reading, not an action on the thread, so it takes no control of its own: the label is the toggle, the way a file's diff header already works, with the chevron saying which way it goes. The two kinds now differ in one thing only, where they start — open by default, closed when settled — and either can be moved from there. Closed, a card is its header alone: it drops the body's padding and carries the comment count, which is the only thing left saying how much is under that line. * fix: keep the line numbers level with their code past a thread Every line below an inline thread showed the number of the line above it: a comment on 839 left the next line reading 841 where GitHub reads 840, and the drift held for the rest of the file. Two causes, both about the height of the gap the widget opens. The gutter draws nothing for a block widget unless it is handed a marker for one, so it had one cell fewer than the content had blocks and every number below sat one line high. An empty marker through lineNumberWidgetMarker buys the widget a cell of its own height and nothing else. That alone left 12px of drift per thread, because the cards inside the gap carried a vertical margin. CodeMirror measures a block widget by the element's own box, and a child's margin falls outside it — height the gutter is never told about. The spacing is the slot's own padding now, and the rule is written where someone would otherwise put the margin back. Verified against the reported shape — a thread on an added line that replaced a deleted one — by pairing every gutter cell with the content block at the same offset: the orphan cell is gone, 840 sits beside 840, and folding the card (which changes its height) keeps them level, since that part CodeMirror re-measures on its own. * fix: stop a discarded comment from moving the one being edited A review pass over the inline-review work, carrying two behaviour fixes and the cleanup around them. Editing a pending comment addressed it by its index in the review. Open the editor on one comment, discard an earlier one, and the indices shift under the open box: saving wrote the text over whichever comment had moved into that slot. The editor holds the comment itself now — the store replaces comments rather than mutating them, so the reference is stable for exactly as long as the comment is. A file's header counted every unresolved thread on it, including the ones the diff cannot anchor: an outdated thread, or one on a line outside every hunk. The header read three where expanding the file showed one. isAnchored already answered this and had no caller outside its own test; the count uses it. PullsFiles built a fresh review object per file on every render, and that object is the identity of each file's CodeMirror decorations — so every render of the screen re-dispatched a transaction into every mounted editor. On a wide pull request that is the shape of the freeze in #92. The per-file objects are memoised, and draftsOnFile takes the comments rather than the whole review, so writing the review summary no longer reads as the line comments having moved. The rest is duplication and magic values: the "t:" slot prefix is one constant behind isThreadSlot, the byline copied across three components is one Byline, the comment field's class is shared with the submit dialog, DiffSelection.range drops a null case the selection can never produce, and CommentOnPullRequest returns its error directly. buildSlots is exported and covered — the sort and the bounds are what stand between an unordered slot list and a RangeSetBuilder that throws on the diff. With it: a thread with no comments left in it, a clear that finds no draft, a non-object inside a stored one, and the array identity the memoisation rests on. * fix: stop a base branch's rules from killing the Merge button GitHub reports mergeStateStatus BLOCKED for any base branch carrying a protection rule or a ruleset. It means "a rule applies here", not "I will not take this" — and a ruleset whose commit-message pattern can only be evaluated against the merge commit leaves an approved pull request with green checks and no conflicts sitting at BLOCKED for good, while GitHub's own button merges it. The gate read BLOCKED as a refusal, so the Merge button was dead on every pull request in a governed repository. Measured on a real one: approved, three checks passed, MERGEABLE, no conflicts, no classic branch protection, one ruleset with a Conventional Commits pattern — and BLOCKED, for an account holding admin on the repository. Invert the gate: name the refusals and let everything else through, rather than allow-listing the states GitHub merges from. A conflict, a base branch that has moved, a draft, a pull request already over, and BLOCKED where reviewDecision says the review the rule wants is still missing. Everything else is offered, and gh's own refusal reaches the screen if GitHub does refuse after all. Two more states go the same way. UNKNOWN is GitHub still recomputing after a push, which is waiting rather than refusing; a state this build has never seen is not evidence of anything. Blocking on either is how a button dies quietly. A merge that is never tried cannot report why it failed, which is worse than the toast this gate was written to replace. The four tests that asserted the old reading assert its inverse, each carrying the reason: the contract changed. The changelog entry described the behaviour being corrected and had not shipped, so it is rewritten rather than answered with a second entry. * style: wrap the merge gate's blocked-review assertions as biome does Formatting only. The frontend gate was reported clean off a truncated biome run, and CI caught what the truncation hid. * feat: offer the merge methods the base branch actually accepts A repository ruleset can pin a branch to one merge method, and squash alone is the common setting. Nothing in a pull request's own fields says so — mergeStateStatus answers whether this merge can happen, never which methods the branch would take — so the menu offered all three and GitHub refused two of them after the click, with a sentence that named none. Read the base branch's rules through `repos/{owner}/{repo}/rules/branches/{branch}` and build the menu from what comes back. The read is keyed by the branch rather than by the pull request, which is what the answer is about: every pull request onto the same base shares one. It is not polled and does not refetch on focus — a ruleset is a repository setting somebody changes once a quarter, and all it narrows is a menu. Everything unknown widens rather than narrows: no ruleset, an unreadable answer, a failed call, or a ruleset naming only methods lich cannot ask gh for. A menu missing an option lich could have offered says nothing about why it is missing; one that offers an option GitHub turns down at least answers with gh's own refusal. The endpoint returns [] for a branch it has no rules for and for a branch that does not exist, so a wrong path escape and an ungoverned branch are indistinguishable from outside — which is also why the degradation is safe: either reads as "no rule known". Merging over red CI now says so before the click. A failing check that no rule requires does not block a merge on GitHub, and lich offers it for the same reason — the call is the reader's. It offered it in silence, though, with the red a line away in the status row. The button carries the count of failing checks, and its hover says GitHub will take the merge anyway. * fix: size a dropdown menu by its own text, not by its trigger Every menu carried w-(--anchor-width), so it was exactly as wide as the button that opened it. That is a select's behaviour — a select's popup stands in for its field and should match it, which is why ui/select.tsx keeps it — but a menu is sized by what it says. Hanging off a small control, the items wrapped: "Squash and merge, edit message…" came out over three lines under a Merge button, and New Session and the pull request filter did the same. The popup sizes to its content now, with min-w-32 as the floor, and an item is one line: a command that wraps reads as two. Items also carry a little more room around the text, which is the other half of what made the narrow menus hard to read. Both call sites that already fought the default keep working: the notifications menu pins its own width and truncates inside each item, and the New Session menu's cap is wider than the provider names ever reach. * fix: keep a price refresh from outliving the test that started it CI failed in internal/pricing with "TempDir RemoveAll cleanup: unlinkat /tmp/TestUnknownModelIsUnpricedNotFree…/001: directory not empty", on a commit that touched only the frontend. Rate answers a miss and spawns the refresh that may teach it, and the refresh ends by writing the cache. TestUnknownModelIsUnpricedNotFree is made of misses, so it returned with that goroutine still running: the write landed inside the temp directory's own removal. The test that fails is whichever one loses the race, never the one that leaked — which is why it read as unrelated to the change under it. Proven rather than guessed: a probe that waits after the assertions found the goroutine writing prices.json into the directory the cleanup was about to remove. It would not reproduce here at -count=200, with -race, or across 40 cold processes — the window is the microseconds between RemoveAll's readdir and its rmdir, which a loaded runner widens and this machine does not. Rate's goroutine joins a WaitGroup, and awaitRefresh drains it. Nothing in the app waits — the caller already had its answer, and the refresh only teaches the next turn — so this exists for the suite, where a component still writing to disk after its owner is gone is a flake looking for a test to fail. The sibling tests already avoided this by hand: the one below reaches for lookup instead of Rate, with a comment saying why. That worked per-test and left the next one to rediscover it. TestARefreshDoes NotOutliveItsTest locks the drain itself, so deleting the WaitGroup fails a test rather than a future CI run. Verified with the package at -race -count=200, and the whole backend suite at -race. --------- Co-authored-by: Claude --- CHANGELOG.md | 55 +++- .../src/components/common/SkeletonLines.tsx | 8 +- frontend/src/components/diff/FileDiff.tsx | 199 ++++++++++--- frontend/src/components/diff/InjectMenu.tsx | 39 ++- frontend/src/components/diff/ReviewPanel.tsx | 8 +- frontend/src/components/diff/ReviewSlots.tsx | 115 +++++++ frontend/src/components/diff/useDiffEditor.ts | 47 ++- frontend/src/components/pulls/Byline.tsx | 24 ++ frontend/src/components/pulls/CommentBox.tsx | 89 ++++++ .../src/components/pulls/PullRequestView.tsx | 259 +++++++++++++--- frontend/src/components/pulls/Pulls.tsx | 9 + .../components/pulls/PullsConversation.tsx | 155 ++++++++++ frontend/src/components/pulls/PullsFiles.tsx | 58 +++- .../src/components/pulls/ReviewThread.tsx | 253 ++++++++++++++++ .../components/pulls/SubmitReviewDialog.tsx | 102 +++++++ frontend/src/components/ui/dropdown-menu.tsx | 12 +- frontend/src/components/ui/tooltip.tsx | 13 +- frontend/src/lib/api-types.ts | 78 +++++ frontend/src/lib/codemirror-comment.ts | 120 -------- frontend/src/lib/codemirror-threads.test.ts | 59 ++++ frontend/src/lib/codemirror-threads.ts | 147 +++++++++ frontend/src/lib/codemirror.ts | 50 +++- frontend/src/lib/git/diff.test.ts | 30 ++ frontend/src/lib/git/diff.ts | 20 ++ frontend/src/lib/prefs.ts | 4 + .../lib/pulls/conversation-timeline.test.ts | 86 ++++++ .../src/lib/pulls/conversation-timeline.ts | 68 +++++ frontend/src/lib/pulls/merge-gate.test.ts | 85 +++++- frontend/src/lib/pulls/merge-gate.ts | 80 +++-- .../lib/pulls/pending-review-store.test.ts | 218 ++++++++++++++ .../src/lib/pulls/pending-review-store.ts | 196 ++++++++++++ frontend/src/lib/pulls/review-slots.test.ts | 127 ++++++++ frontend/src/lib/pulls/review-slots.ts | 91 ++++++ frontend/src/lib/pulls/use-branch-rules.ts | 25 ++ .../pulls/use-pull-request-conversation.ts | 37 +++ frontend/src/lib/rpc.ts | 38 ++- frontend/src/lib/use-codemirror-view.ts | 20 +- internal/pricing/pricing.go | 16 +- internal/pricing/pricing_test.go | 26 ++ internal/project/branchrules.go | 67 +++++ internal/project/branchrules_test.go | 73 +++++ internal/project/gherror.go | 14 + internal/project/pr.go | 15 - internal/project/pr_test.go | 38 --- internal/project/prreview.go | 198 +++++++++++++ internal/project/prreview_test.go | 230 ++++++++++++++ internal/project/prthreads.go | 280 ++++++++++++++++++ internal/project/prthreads_test.go | 174 +++++++++++ 48 files changed, 3766 insertions(+), 389 deletions(-) create mode 100644 frontend/src/components/diff/ReviewSlots.tsx create mode 100644 frontend/src/components/pulls/Byline.tsx create mode 100644 frontend/src/components/pulls/CommentBox.tsx create mode 100644 frontend/src/components/pulls/PullsConversation.tsx create mode 100644 frontend/src/components/pulls/ReviewThread.tsx create mode 100644 frontend/src/components/pulls/SubmitReviewDialog.tsx delete mode 100644 frontend/src/lib/codemirror-comment.ts create mode 100644 frontend/src/lib/codemirror-threads.test.ts create mode 100644 frontend/src/lib/codemirror-threads.ts create mode 100644 frontend/src/lib/pulls/conversation-timeline.test.ts create mode 100644 frontend/src/lib/pulls/conversation-timeline.ts create mode 100644 frontend/src/lib/pulls/pending-review-store.test.ts create mode 100644 frontend/src/lib/pulls/pending-review-store.ts create mode 100644 frontend/src/lib/pulls/review-slots.test.ts create mode 100644 frontend/src/lib/pulls/review-slots.ts create mode 100644 frontend/src/lib/pulls/use-branch-rules.ts create mode 100644 frontend/src/lib/pulls/use-pull-request-conversation.ts create mode 100644 internal/project/branchrules.go create mode 100644 internal/project/branchrules_test.go create mode 100644 internal/project/prreview.go create mode 100644 internal/project/prreview_test.go create mode 100644 internal/project/prthreads.go create mode 100644 internal/project/prthreads_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index caf534850..f9f590b7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Review a pull request without leaving lich.** Pulls only ever showed the + parts that travel one way — the checks, the commits, the diff — so a review + that asked for a change was invisible here and reading it meant the browser. + The diff now carries the conversation: a thread opens inline under the line it + is about, with its replies, and can be answered and resolved where it sits. + Right-click a selection and *Comment on the pull request* starts one of your + own; each comment waits with the others until **Submit review** sends them + together as a single review — approving, commenting, or requesting changes, + with a summary of your own above them. Comments waiting to be sent are marked + as pending and survive a refresh, a collapsed file and a restart, so a review + written over an afternoon is still there. A new **Conversation** tab holds the + whole exchange in the order it happened — verdicts, comments on the pull + request itself, and the threads whose lines the branch has since rewritten, + which no diff can show. Settled threads fold themselves away behind a count. - **Appearance can import custom color themes.** lich still ships the bundled Light and Dark themes, but Settings › Appearance now accepts theme JSON files saved under the app config directory. Imported themes can recolor both the @@ -18,14 +32,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the session a *reference* — a file, a line range — but never an instruction, so anything you actually wanted changed had to be typed out by hand while looking straight at it. Right-clicking a selection, in the Review dock or in a - pull request's Files changed, now also offers *Comment on lines*: a box opens - between the lines themselves, and the note is held against them instead of - being written out. Comments collect at the foot of the panel in the order you + pull request's Files changed, now also offers *Comment for the session*: a box + opens between the lines themselves, and the note is held against them instead + of being written out. Comments collect at the foot of the panel in the order you read the diff, each removable on its own, and one *Send* hands the whole batch over as a single prompt — pasted into the session unsent, so the last word before the agent starts is still yours. They outlive leaving the tab but not a restart: a line number written before an - edit points somewhere else after it. + edit points somewhere else after it. On a pull request's diff both destinations + are offered side by side, named for where the note goes. - **Every worktree gets a dev-server port of its own.** Two worktrees of one project both ran `pnpm dev` and both wanted the same port, so the second one lost — and the setup script that had just installed the dependencies had no @@ -57,13 +72,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **The Merge button says no before the click, not after.** Merging a pull - request GitHub would not take — a required review missing, a required check - still running, the base branch moved on, or GitHub simply still working out - whether the branch merges — offered the button anyway and answered with a - toast that named none of those reasons. The button is now disabled whenever - GitHub reports it cannot merge, and hovering it says which of them it is. A - failing check that no rule requires still merges: the checks readout shows it - red, and the call stays yours. + request GitHub would not take — a review missing, a reviewer asking for + changes, a conflict, the base branch moved on — offered the button anyway and + answered with a toast that named none of those reasons. The button is now + disabled for each of them, and hovering it says which. Everything else it + still offers: a failing check no rule requires merges fine, a base branch + governed by a ruleset merges fine, and GitHub still deciding is not a no. The + call stays yours, and where GitHub does refuse, it refuses out loud. +- **The Merge menu offers what your repository actually accepts.** A repository + can pin a branch to one merge method — squash only is the common one — and + nothing in a pull request says so, so the menu offered all three and GitHub + refused two of them after the click. lich now reads the base branch's rules + and offers only the methods that branch takes. A branch nothing governs, or + an answer lich cannot read, still offers all three: the menu never comes back + narrower than the truth. +- **Merging over red CI says so first.** A failing check that no rule requires + does not block a merge on GitHub, and lich offers the merge for the same + reason — the call is yours. It just used to offer it in silence, with the red + a line away in the status row. The Merge button now carries the number of + failing checks, and hovering it says GitHub will merge anyway. +- **Dropdown menus are as wide as what they say.** Every menu was pinned to the + width of the button that opened it, so the ones hanging off a small control — + New Session, the pull request filter, Merge — wrapped their items over two and + three lines. They size to their own text now, on one line each, with a little + more room around it. A select still matches its field, which is the one place + the old behaviour was right. - **Settings › Font lists your installed fonts on Windows.** The picker read the font list from fontconfig, which Windows does not have, so it offered nothing beyond the bundled default and whatever was already selected. It now reads the diff --git a/frontend/src/components/common/SkeletonLines.tsx b/frontend/src/components/common/SkeletonLines.tsx index f213a15f2..8e1f4116a 100644 --- a/frontend/src/components/common/SkeletonLines.tsx +++ b/frontend/src/components/common/SkeletonLines.tsx @@ -17,8 +17,12 @@ interface SkeletonLinesProps { export function SkeletonLines({ widths, height = "h-3", className }: SkeletonLinesProps) { return ( <> - {widths.map((width) => ( - + {/* Keyed by position, not by width: a ragged stack repeats widths by + design, and two bars keyed the same are two bars React folds into one. + The list is static, so the index is the identity. */} + {widths.map((width, line) => ( + // biome-ignore lint/suspicious/noArrayIndexKey: a fixed stack of bars, never reordered. + ))} ) diff --git a/frontend/src/components/diff/FileDiff.tsx b/frontend/src/components/diff/FileDiff.tsx index ca18e1efb..13122bbb0 100644 --- a/frontend/src/components/diff/FileDiff.tsx +++ b/frontend/src/components/diff/FileDiff.tsx @@ -1,8 +1,10 @@ -import { ChevronDown, ChevronRight, Paperclip, Undo2 } from "lucide-react" +import { ChevronDown, ChevronRight, MessageSquare, Paperclip, Undo2 } from "lucide-react" import { useEffect, useMemo, useRef, useState } from "react" +import { createPortal } from "react-dom" import { IconAction } from "@/components/common/IconAction" import { DiffStat } from "@/components/DiffStat" import { Checkbox } from "@/components/ui/checkbox" +import { threadSlots, type SlotElements, type ThreadSlot } from "@/lib/codemirror-threads" import { buildFileDoc, formatLineRef, @@ -11,10 +13,12 @@ import { type FileDoc, } from "@/lib/git/diff" import { languageAbbr, splitPath } from "@/lib/git/lang-badge" +import { COMPOSER_KEY, isAnchored, reviewSlots } from "@/lib/pulls/review-slots" import { cn } from "@/lib/utils" import type { DiffBulk } from "./diff-bulk" import { InjectMenu } from "./InjectMenu" import { useDiffEditor } from "./useDiffEditor" +import { type Composer, type DiffReview, type DiffSelection, ReviewSlot } from "./ReviewSlots" // Files whose rendered diff exceeds this many lines start collapsed, so one // giant lockfile doesn't swamp the panel (expanding is one click away). @@ -32,9 +36,10 @@ const MOUNT_MARGIN = "600px 0px" interface FileDiffProps { file: DiffFile onInject: (text: string) => void - /** Hold a comment on these lines for the panel's batch. Absent = no comment - * action, the shape every surface had before review comments existed. */ - onComment?: (path: string, lines: string, text: string) => void + /** Hold a comment on these lines for the panel's batch, which the session's + * next prompt carries. Absent = no session comment, the shape every surface + * had before review comments existed. */ + onSessionComment?: (path: string, lines: string, text: string) => void /** Ask the panel to confirm and revert this file's changes. Omitted for a * read-only diff (a PR's changes), where discarding makes no sense. */ onDiscard?: () => void @@ -45,6 +50,10 @@ interface FileDiffProps { /** Handle the tick. Absent = no Viewed control (the dock's working diff, * where every file is the current one). */ onViewed?: (next: boolean) => void + /** The pull request review over this file: threads to render inline, drafts + * waiting to be sent, and what a new comment does. Absent for the working + * diff, which has no pull request behind it. */ + review?: DiffReview } // The card must not clip overflow — a clipping ancestor would break the @@ -52,11 +61,12 @@ interface FileDiffProps { export function FileDiff({ file, onInject, - onComment, + onSessionComment, onDiscard, bulk, viewed, onViewed, + review, }: FileDiffProps) { const doc = useMemo(() => buildFileDoc(file), [file]) const [expanded, setExpanded] = useState(!file.binary && doc.lineMeta.length <= LARGE_FILE_LINES) @@ -72,6 +82,12 @@ export function FileDiff({ const Chevron = expanded ? ChevronDown : ChevronRight const badge = languageAbbr(file.newPath) const { dir, base } = splitPath(file.newPath) + // Only what opening the file would actually reveal: a settled thread is not + // outstanding, and one the diff cannot anchor lives in the Conversation tab. + const talking = review + ? review.threads.filter((thread) => !thread.isResolved && isAnchored(thread, doc.lineMeta)) + .length + review.drafts.length + : 0 return (
@@ -104,6 +120,15 @@ export function FileDiff({ {dir && {dir}} + {/* A collapsed file must still say it is being talked about — the + threads inside it are invisible until it is opened. Settled threads + are left out of the count: they are not what is outstanding. */} + {talking > 0 && ( + + + {talking} + + )} @@ -138,7 +163,13 @@ export function FileDiff({ (file.binary ? (

Binary file

) : ( - + ))}
) @@ -148,7 +179,8 @@ interface DiffBodyProps { doc: FileDoc path: string onInject: (text: string) => void - onComment?: (path: string, lines: string, text: string) => void + onSessionComment?: (path: string, lines: string, text: string) => void + review?: DiffReview } // A panel holds one CodeMirror view per expanded file, and a wide diff (a PR @@ -156,7 +188,7 @@ interface DiffBodyProps { // page. LazyDiffBody defers each one until its card comes near the viewport; // once built, the editor stays, so scrolling back keeps its selection and // highlighting. Collapsing the file still destroys it. -function LazyDiffBody({ doc, path, onInject, onComment }: DiffBodyProps) { +function LazyDiffBody({ doc, path, onInject, onSessionComment, review }: DiffBodyProps) { const placeholder = useRef(null) const [mounted, setMounted] = useState(false) @@ -178,49 +210,142 @@ function LazyDiffBody({ doc, path, onInject, onComment }: DiffBodyProps) { }, []) if (mounted) { - return + return ( + + ) } // The placeholder has to stand in for the file's height: zero-height cards // would all sit in the viewport at once and nothing would be deferred. return
} +const NO_SLOTS: SlotElements = new Map() + // DiffBody exists as its own component so collapsing the file unmounts it, // destroying the CodeMirror view instead of keeping it alive off-screen. // // A diff's document is not the file: the selection lands on doc lines, which -// newLineRange maps back to the line numbers the agent needs to be told. -function DiffBody({ doc, path, onInject, onComment }: DiffBodyProps) { - const { containerRef, getSelectedDocLines, openComment } = useDiffEditor( +// newLineRange maps back to the line numbers an inject writes, a session comment +// anchors to, and GitHub files a review comment against. +function DiffBody({ doc, path, onInject, onSessionComment, review }: DiffBodyProps) { + // One slot extension per view. It is memoised because it *is* part of the + // view's identity: a new one would rebuild the editor on every render. + const [elements, setElements] = useState(NO_SLOTS) + const slots = useMemo(() => threadSlots(setElements), []) + // The gaps exist wherever a comment can be written, which on the dock's + // working diff is the session's alone. + const gapped = Boolean(review || onSessionComment) + const { containerRef, getSelectedDocLines, view } = useDiffEditor( doc, path, - onComment && ((lines, text) => onComment(path, lines, text)), + gapped ? slots.extension : undefined, ) - // Both halves of the resolved selection: the file lines an inject or a - // comment is filed against, and the doc line the composer hangs under. - const [selection, setSelection] = useState<{ lines: string; docLine: number } | null>(null) + // Both halves of the resolved selection: the file lines a comment is filed + // against, and the doc line a composer hangs under. They differ — a selection + // ending on a deleted line has a doc line but no new-file one. + const [selection, setSelection] = useState(null) + const [composer, setComposer] = useState(null) + + // Where the gaps go, never what is in them: keyed off the composer's line + // rather than the composer, so typing into it does not re-dispatch the whole + // slot set on every keystroke. + const composerLine = composer?.docLine ?? 0 + const wanted = useMemo((): ThreadSlot[] => { + const built = review + ? reviewSlots({ + lineMeta: doc.lineMeta, + threads: review.threads, + drafts: review.drafts.map(({ comment }) => comment), + }) + : [] + // The composer is placed by doc line, not by file line: it belongs under + // the last line the selection covered, whichever side that line came from. + return composerLine > 0 ? [...built, { key: COMPOSER_KEY, docLine: composerLine }] : built + }, [review, doc.lineMeta, composerLine]) + + useEffect(() => { + if (view && gapped) { + slots.update(view, wanted) + } + }, [view, gapped, slots, wanted]) + + const openComposer = (kind: Composer["kind"]) => (): void => { + if (selection) { + setComposer({ ...selection, kind, body: "" }) + } + } + + // Where a written comment goes, which is the whole difference between the two + // menu items: the batch the session's next prompt carries, or the review + // GitHub is waiting for. + const fileComment = (): void => { + const body = composer?.body.trim() ?? "" + if (!composer || body === "") { + return + } + if (composer.kind === "session") { + onSessionComment?.(path, composer.lines, body) + } else { + review?.onAdd({ + path, + line: composer.range.end, + startLine: composer.range.start === composer.range.end ? 0 : composer.range.start, + side: "RIGHT", + body, + }) + } + setComposer(null) + } return ( - { - if (!open) { - return - } - const selected = getSelectedDocLines() - const range = selected ? newLineRange(doc.lineMeta, selected.from, selected.to) : null - setSelection( - range && selected ? { lines: formatLineRef(range), docLine: selected.to } : null, - ) - }} - onInject={onInject} - onComment={ - onComment && - (() => selection && openComment({ line: selection.docLine, lines: selection.lines })) - } - /> + <> + { + if (!open) { + return + } + const selected = getSelectedDocLines() + const range = selected ? newLineRange(doc.lineMeta, selected.from, selected.to) : null + setSelection( + selected && range ? { lines: formatLineRef(range), range, docLine: selected.to } : null, + ) + }} + onInject={onInject} + // Both items are offered whenever their destination exists, not only + // once a range is resolved: the selection is read as the menu opens, so + // gating them on it would hide them on the very open that produced it. + // With nothing selected the menu disables them, like Inject lines. + // + // Deliberately new-file lines only: a comment on a deleted line is a + // thread GitHub anchors on the other side, and that needs its own gesture. + onSessionComment={onSessionComment && openComposer("session")} + onReviewComment={review && openComposer("review")} + /> + {gapped && + [...elements].map(([key, element]) => + createPortal( + setComposer((held) => held && { ...held, body })} + onComposerSubmit={fileComment} + onComposerCancel={() => setComposer(null)} + />, + element, + key, + ), + )} + ) } diff --git a/frontend/src/components/diff/InjectMenu.tsx b/frontend/src/components/diff/InjectMenu.tsx index e38455b7c..a3744d08f 100644 --- a/frontend/src/components/diff/InjectMenu.tsx +++ b/frontend/src/components/diff/InjectMenu.tsx @@ -16,16 +16,24 @@ interface InjectMenuProps { /** Resolve the selection; fired as the menu opens, not on every change. */ onOpenChange: (open: boolean) => void onInject: (text: string) => void - /** Start a comment on the selected lines. Absent on a surface that is not a - * review — the dock's file preview — which offers the references only. */ - onComment?: (lines: string) => void + /** Start a comment held for the session's next prompt. Absent on a surface + * that is not a review — the dock's file preview. */ + onSessionComment?: () => void + /** Start a comment on the pull request itself. Absent wherever there is no + * pull request behind the diff — the dock's working diff. */ + onReviewComment?: () => void } // The right-click menu over a code view — a file's diff, or the read-only // preview in the dock — that writes a reference into the session's terminal. -// The whole file, or just the lines under the selection. On a review surface it -// also opens a comment on those lines, which is held for the batch rather than -// written out on its own. +// The whole file, or just the lines under the selection. +// +// One selection, and up to three things to do with it: inject it as a +// reference, comment on it for the session's next prompt, or comment on it for +// the pull request. That fork is the reason the two comment items are named for +// where they go rather than both reading "comment": on a pull request's diff +// both are offered, and picking the wrong one is a note that lands where nobody +// looks for it. // // The isolate wrapper keeps CodeMirror's high-z-index gutter from painting over // the sticky header of the card above it. @@ -35,7 +43,8 @@ export function InjectMenu({ lineRef, onOpenChange, onInject, - onComment, + onSessionComment, + onReviewComment, }: InjectMenuProps) { return ( @@ -48,12 +57,16 @@ export function InjectMenu({ > {lineRef === null ? "Inject lines" : `Inject lines ${lineRef}`} - {onComment && ( - lineRef && onComment(lineRef)} - > - {lineRef === null ? "Comment on lines…" : `Comment on lines ${lineRef}…`} + {onSessionComment && ( + + {lineRef === null ? "Comment for the session…" : `Comment for the session ${lineRef}…`} + + )} + {onReviewComment && ( + + {lineRef === null + ? "Comment on the pull request…" + : `Comment on the pull request ${lineRef}…`} )} diff --git a/frontend/src/components/diff/ReviewPanel.tsx b/frontend/src/components/diff/ReviewPanel.tsx index 1d419eaf9..7df973c04 100644 --- a/frontend/src/components/diff/ReviewPanel.tsx +++ b/frontend/src/components/diff/ReviewPanel.tsx @@ -74,7 +74,7 @@ export function ReviewPanel({ bulk }: { bulk: DiffBulk }) { files={files} failed={failed} onInject={inject} - onComment={(file, lines, text) => addReviewComment(path, file, lines, text)} + onSessionComment={(file, lines, text) => addReviewComment(path, file, lines, text)} onDiscard={setPendingDiscard} bulk={bulk} /> @@ -93,12 +93,12 @@ interface PanelBodyProps { files: DiffFile[] | null failed: boolean onInject: (text: string) => void - onComment: (path: string, lines: string, text: string) => void + onSessionComment: (path: string, lines: string, text: string) => void onDiscard: (file: DiffFile) => void bulk: DiffBulk } -function PanelBody({ files, failed, onInject, onComment, onDiscard, bulk }: PanelBodyProps) { +function PanelBody({ files, failed, onInject, onSessionComment, onDiscard, bulk }: PanelBodyProps) { if (failed) { return Not a git repository } @@ -115,7 +115,7 @@ function PanelBody({ files, failed, onInject, onComment, onDiscard, bulk }: Pane key={file.newPath} file={file} onInject={onInject} - onComment={onComment} + onSessionComment={onSessionComment} onDiscard={() => onDiscard(file)} bulk={bulk} /> diff --git a/frontend/src/components/diff/ReviewSlots.tsx b/frontend/src/components/diff/ReviewSlots.tsx new file mode 100644 index 000000000..9097a6f7b --- /dev/null +++ b/frontend/src/components/diff/ReviewSlots.tsx @@ -0,0 +1,115 @@ +import type { DraftReviewComment, ReviewThread as Thread } from "@/lib/api-types" +import type { NewLineRange } from "@/lib/git/diff" +import { COMPOSER_KEY, draftSlotKey, isThreadSlot, threadSlotKey } from "@/lib/pulls/review-slots" +import { CommentBox } from "@/components/pulls/CommentBox" +import { PendingComments, ReviewThread, type ThreadActions } from "@/components/pulls/ReviewThread" + +// The pull request review laid over one file's diff. Absent for the dock's +// working diff, which has no pull request behind it and therefore no threads, +// no drafts and nothing to submit. +export interface DiffReview { + /** Threads anchored in this file. */ + threads: Thread[] + /** Draft comments in this file, each with its index in the pending review — + * the index is what editing and discarding address. */ + drafts: { comment: DraftReviewComment; index: number }[] + actions: ThreadActions + onAdd: (comment: DraftReviewComment) => void + onEdit: (index: number, body: string) => void + onRemove: (index: number) => void +} + +/** A resolved selection: what to call the lines, which new-file lines they are, + * and the document line a composer opened on it hangs under. A selection that + * covers only deleted lines has no new-file range and so never becomes one of + * these — commenting on the other side needs its own gesture. */ +export interface DiffSelection { + lines: string + range: NewLineRange + docLine: number +} + +/** The comment being written, and where it goes when it is filed. Held by the + * diff body rather than by the box, so a refetch that rebuilds the editor moves + * the box without emptying it. */ +export interface Composer extends DiffSelection { + /** "session" is held for the next prompt; "review" waits for GitHub. */ + kind: "session" | "review" + body: string +} + +interface ReviewSlotProps { + slotKey: string + /** Absent on the dock's working diff, which has threads of no kind — only a + * composer aimed at the session. */ + review?: DiffReview + composer: Composer | null + onComposerChange: (body: string) => void + onComposerSubmit: () => void + onComposerCancel: () => void +} + +// What goes in one gap, resolved from the key the widget carries. A key with +// nothing behind it renders nothing: the slots and the data behind them are +// updated in separate passes, and a thread resolved in between must not throw +// on the way out. +// +// Nothing in here may carry a vertical margin. CodeMirror measures a block +// widget by its element's own box, so a margin inside is height the gutter +// never hears about, and the line numbers below drift by exactly that much. +// The spacing is the slot's padding (`.cm-thread-slot` in codemirror.ts). +export function ReviewSlot({ + slotKey, + review, + composer, + onComposerChange, + onComposerSubmit, + onComposerCancel, +}: ReviewSlotProps) { + if (slotKey === COMPOSER_KEY) { + if (!composer) { + return null + } + // The label says where the note is going, because the two destinations look + // identical once the box is open and only one of them reaches GitHub. + const session = composer.kind === "session" + return ( +
+ + {session ? "for the session" : "on the pull request"} · {composer.lines} + + +
+ ) + } + + if (!review) { + return null + } + + if (isThreadSlot(slotKey)) { + const thread = review.threads.find((held) => threadSlotKey(held) === slotKey) + return thread ? : null + } + + const drafts = review.drafts.filter(({ comment }) => draftSlotKey(comment) === slotKey) + return drafts.length > 0 ? ( +
+ +
+ ) : null +} diff --git a/frontend/src/components/diff/useDiffEditor.ts b/frontend/src/components/diff/useDiffEditor.ts index b83479298..b8de84016 100644 --- a/frontend/src/components/diff/useDiffEditor.ts +++ b/frontend/src/components/diff/useDiffEditor.ts @@ -1,7 +1,8 @@ -import { useMemo, useRef } from "react" +import { useMemo } from "react" import type { RefObject } from "react" +import type { Extension } from "@codemirror/state" +import type { EditorView } from "@codemirror/view" import { buildLineDecorations, diffGutter, type DocLineSelection } from "@/lib/codemirror" -import { commentComposer, setCommentDraft, type CommentDraft } from "@/lib/codemirror-comment" import { useCodeMirrorView } from "@/lib/use-codemirror-view" import type { FileDoc } from "@/lib/git/diff" @@ -9,43 +10,33 @@ export interface DiffEditor { containerRef: RefObject /** 1-based doc line span of the current selection, or null when empty. */ getSelectedDocLines: () => DocLineSelection | null - /** Open the inline comment composer under a draft's line. */ - openComment: (draft: CommentDraft) => void + /** The live view, for a caller that dispatches into it. */ + view: EditorView | null } // useDiffEditor is the diff card's read-only CodeMirror view: the shared editor -// plus the extensions that make it a diff — the +/- gutter, the per-line -// background, and the comment composer that opens between the lines. +// plus the two extensions that make it a diff — the +/- gutter and the per-line +// background. Its selection lands on *doc* lines, which are the diff's own +// interleaving of added and deleted, not the file's numbering; mapping them +// back is newLineRange's job at the call site. // -// Its selection lands on *doc* lines, which are the diff's own interleaving of -// added and deleted, not the file's numbering; mapping them back is -// newLineRange's job at the call site. -export function useDiffEditor( - doc: FileDoc, - filename: string, - onComment?: (lines: string, text: string) => void, -): DiffEditor { - // The extensions are built once per document, so the composer cannot capture - // this render's handler; it reads the current one off the ref instead. - const submit = useRef(onComment) - submit.current = onComment - +// `extra` is laid on top for a caller that needs more of the editor — every gap +// opened between two lines, whether it holds a review thread, a comment waiting +// for GitHub or one waiting for the session, comes through it. It has to be a +// stable reference: it rides the view's identity, so a new one on every render +// would rebuild the editor on every render. +export function useDiffEditor(doc: FileDoc, filename: string, extra?: Extension): DiffEditor { const source = useMemo( () => ({ text: doc.text, extensions: [ diffGutter(doc.lineMeta), buildLineDecorations(doc.lineMeta), - commentComposer((lines, text) => submit.current?.(lines, text)), + ...(extra ? [extra] : []), ], }), - [doc], + [doc, extra], ) - const { containerRef, getSelectedLines, dispatchEffects } = useCodeMirrorView(source, filename) - - return { - containerRef, - getSelectedDocLines: getSelectedLines, - openComment: (draft) => dispatchEffects([setCommentDraft.of(draft)]), - } + const { containerRef, getSelectedLines, view } = useCodeMirrorView(source, filename) + return { containerRef, getSelectedDocLines: getSelectedLines, view } } diff --git a/frontend/src/components/pulls/Byline.tsx b/frontend/src/components/pulls/Byline.tsx new file mode 100644 index 000000000..7a990ecbe --- /dev/null +++ b/frontend/src/components/pulls/Byline.tsx @@ -0,0 +1,24 @@ +import type { ReactNode } from "react" +import { updatedAgo } from "@/lib/pulls/pull-request-list" + +interface BylineProps { + author: string + /** gh's ISO timestamp, rendered as the age everything else on this screen + * reads in. */ + date: string + /** What sits between the name and the age — a review's verdict. */ + children?: ReactNode +} + +// Who said it and when: the line above every comment, thread message and review +// on the Pulls screen. GitHub leaves the author null for an account that has +// been deleted since, which arrives here as an empty login. +export function Byline({ author, date, children }: BylineProps) { + return ( +
+ {author || "someone"} + {children} + {updatedAgo(date)} +
+ ) +} diff --git a/frontend/src/components/pulls/CommentBox.tsx b/frontend/src/components/pulls/CommentBox.tsx new file mode 100644 index 000000000..c7c40398f --- /dev/null +++ b/frontend/src/components/pulls/CommentBox.tsx @@ -0,0 +1,89 @@ +import { Button } from "@/components/ui/button" +import { cn } from "@/lib/utils" + +/** The field every piece of review prose is typed into. Exported because the + * submit dialog writes the review's summary into a bare textarea — it carries + * the dialog's own footer instead of this box's buttons — and the two fields + * drifting apart would read as two different controls for the same thing. */ +export const commentFieldClass = + "w-full resize-y rounded-md border border-input bg-transparent px-2.5 py-1.5 font-sans text-sm shadow-xs outline-none transition-[color,box-shadow] placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-3 focus-visible:ring-ring/50 dark:bg-input/30" + +interface CommentBoxProps { + value: string + onChange: (value: string) => void + onSubmit: () => void + /** Absent leaves no way out — for a box that is always open (the review + * summary), where cancelling means clearing the text. */ + onCancel?: () => void + submitLabel: string + busy?: boolean + placeholder?: string + autoFocus?: boolean + rows?: number + className?: string + /** Send on a bare ⏎, with ⇧⏎ for a new line. For a one-line note taken while + * reading — a comment for the session — where reaching for a modifier is the + * slower half of writing it. Off elsewhere: a review comment is prose, and a + * stray ⏎ would file half of it. */ + submitOnEnter?: boolean +} + +// The one place review text is typed: a line comment, a reply, the summary above +// a review. It holds no draft of its own — the caller owns the text, because the +// editor under this box is rebuilt by CodeMirror whenever the diff refetches and +// a box that kept its own state would lose it there. +// +// Cmd/Ctrl+Enter sends and Escape backs out, so a comment written mid-review +// never needs the mouse. +export function CommentBox({ + value, + onChange, + onSubmit, + onCancel, + submitLabel, + busy = false, + placeholder, + autoFocus = false, + rows = 3, + className, + submitOnEnter = false, +}: CommentBoxProps) { + const empty = value.trim() === "" + return ( +
+