Repository navigation
feat: add custom themes - #131
Merged
omartelo merged 14 commits intoAug 3, 2026
Merged
Conversation
3 tasks done
omartelo
requested changes
Aug 3, 2026
omartelo
left a comment
omartelo
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed against the repo gate, reproduced locally on the branch: gofmt -l . clean, go vet ./internal/... clean, go test ./internal/themes 33 pass at 80.9% coverage, tsc --noEmit clean, vitest run 533/533, biome check src 12 warnings — all pre-existing, none in this PR's files (one of them is in the now-dead SegmentedControl.tsx).
The feature is well shaped: contract validation lives in the backend, a malformed theme file already on disk is skipped instead of blocking startup, and TestBundledThemesMatchFrontendJSON guards the Go↔JSON copy. I did not find a bug that breaks the happy path.
Requesting changes on the ten points below — dead code, a palette that now lives in three places, a file picker that bypasses the app's existing RPC seam, duplicated markup and string literals, unbounded validation, a silent overwrite on import, a stale preference nothing reconciles, and the two coverage gaps.
One more thing: the branch currently conflicts with main, so it needs a merge before it can land.
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.
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com>
Nine pull requests since v0.22.0: reviewing a pull request inside the diff, review comments handed to the session as one prompt, a dev-server port per worktree, the session cost readout, the new app icon, and the Windows and session-lifecycle fixes behind them. Minor rather than patch: four of the nine add something a user did not have.
* feat: merge past a rule that is holding a pull request back A ruleset can hold a pull request that has nothing wrong with it. Measured on a real one: approved, three checks passed, MERGEABLE, no conflicts, and mergeStateStatus BLOCKED — because the repository requires commit messages to match a Conventional Commits pattern, and the message the squash would write does not. Nothing on the pull request says that, and nothing lich showed came close: the toast said "it has conflicts or an unmet requirement", so the reading was conflicts a pull request did not have. There was also nothing to do about it from here. gh checks mergeStateStatus itself and refuses before it ever calls GitHub, so the "edit message" flow — the one thing that would fix a message-pattern rule — died at the same client-side check as the plain merge. Three changes, smallest first. gh writes one sentence for two very different failures and puts the difference in its tail. gherror.go matched only "not mergeable" and claimed conflicts either way, which is the wrong half of the time. Match the tails: "the merge commit cannot be cleanly created" is a conflict, "the base branch policy prohibits the merge" is a rule. The bare match stays last for a wording neither one catches. BranchRules already fetched the whole ruleset payload and threw away everything but allowed_merge_methods. The commit-scoped pattern rules are in that same answer, so keep them: message, author email and committer email, with the operator and the pattern each one tests. Where GitHub answers BLOCKED and one exists, the Merge button's hover and the failure name what the base branch requires. Branch and tag name patterns are left out on purpose — they gate a push, never a merge, and naming them would send the reader after the wrong rule. It is a note, never a verdict. GitHub publishes no "why is this BLOCKED", so the honest answer is where to look. Re-running the pattern here would be a guess dressed as an answer, in a regex dialect that is not the one GitHub evaluates. And the merge itself takes --admin, GitHub's own administrator override — which is also the only way gh calls GitHub at all once a rule holds the merge, so a real rule violation finally reaches the screen. The menu offers it only on BLOCKED and BEHIND, which is gh's own allowsAdminOverride: an override skips a rule, it does not resolve a conflict or finish a draft. Where it is offered, the ordinary items are gone rather than shown beside it — gh has already refused those from the client, so a menu carrying both would put the dead click next to the working one. Without the privilege the dead click stays, because it is what carries gh's refusal and the rule note back. Whether the account may bypass is read, not assumed: `gh repo view --json viewerPermission`, and only ADMIN counts. This is the one answer in the merge gate that fails closed. Everything else there would rather offer an option GitHub turns down than drop one it would have taken, because a refusal at least says what happened — but a bypass reads as a privilege you hold, so one that is not there turns GitHub's refusal into what looks like a broken button. A ruleset can name any role as a bypass actor, so an account that may bypass without administering the repository does not get the option; reading each ruleset's bypass list is the upgrade path. A failed permission read costs the bypass and nothing else — the rules are worth drawing on their own. The tests that changed did so because the contract did: mergeArgs took a parameter, and the gherror case asserted the single sentence that no longer exists. fakeGH grew per-call answers and a call history, which the two-call read needs. Not smoke-tested in task dev: seeing the bypass menu needs a pull request sitting at BLOCKED, and this repository has none. The parser was run against the real ruleset payload that prompted this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: never offer a bypass over a conflict a stale merge state hides canAdminOverride read mergeStateStatus alone, so BLOCKED reported over a conflict — the case mergeBlockedReason already reads both fields for — took the bypass past every guard: the Merge button went live, the menu read "Squash and merge, bypass rules", and the click failed on a conflict no override resolves. That is the dead click the fail-closed design exists to remove, and the shipped promise is "never over a conflict or a draft, which no override resolves". Read mergeable too, as the rest of the gate does. Also names what the menu's all-or-nothing rule costs: BLOCKED is what GitHub answers for any governed base branch, rules met or not, so an administrator's ordinary approved merge goes out with --admin and GitHub records it as an override. gh leaves no other way through, and nothing on screen or in the code said so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
GitHub sends a review comment with the whole hunk it sits in, and the Conversation tab printed it verbatim: on a file the branch adds, a remark about ten lines arrived under a hundred and twenty of them. The snippet is now parsed and cut to the span the thread covers — the range for a multi-line thread, the line plus a little context for a single one — with a fallback to the hunk's tail for a thread GitHub re-anchored, since the hunk always ends on the line the comment was filed on. The lines carry the diff viewer's own green and red, numbered from the hunk header, and the header's file reference reads the whole range. The diff colours lose their cm- prefix along the way: they are no longer the CodeMirror viewer's alone. Threads GitHub marks outdated null their startLine, so the query asks for originalStartLine too — without it an outdated range collapsed to one line and took the snippet with it.
Every pull request from a fork was refused a session, on the grounds that an agent's commits would have nowhere to push. GitHub asks the author that exact question when the pull request is opened, and "allow edits by maintainers" is on by default — so the blanket refusal turned a permission that was already granted into a dead button. The gate now reads maintainerCanModify alongside isCrossRepository, in both places that held the rule: CreateWorktreeFromPR, which refuses before the worktree exists, and the header button, which greys out with the reason on its tooltip. Only a fork that withholds the permission is still refused, and both messages now name which of the two it is. Verified by hand against a fork PR: gh pr checkout inside a detached worktree sets branch.<name>.remote to the fork, so a push goes back to the branch the pull request is on.
…-themes # Conflicts: # CHANGELOG.md # frontend/src/lib/rpc.ts
omartelo
approved these changes
Aug 3, 2026
…validation The template built its JSON in the page and pushed it through a synthetic anchor, so it landed wherever Chromium sends downloads — a window with no download shelf, and a toast that claimed success without knowing it. It is now themes/template.json, embedded beside the bundled pair and written by themes.SaveTemplate to the path the native save dialog returned, so the toast can name where the file went. Import already went through the picker; this puts both directions on the same seam. Color validation moves from a denylist to what each consumer actually parses. App values take hex, a CSS color name or a color function, which leaves out url(), image-set(), var() and attr() by construction rather than by blocklist. Terminal values must be hex: xterm parses those directly, and everything else goes through a round-trip that fails on translucency and is swallowed into a silent fallback, applying a theme halfway with no error shown. A theme id can no longer be a Windows device name. It names a file, and con.json still addresses the console. The id regex leaves the frontend. reconcileThemeSelections already checks the only thing that decides anything — membership — once the theme list lands, so the shape check ahead of it changed no outcome while keeping a third copy of a rule the backend owns. A Select renders its raw value until the Root is handed the value to label map, because items register themselves only once the popup has been opened. Both theme pickers pass it now, so a closed trigger reads System, not system. Also: the documented example and the template answer to the real validator in tests, findTheme is inlined into resolveTheme, and normalizeTheme only clones.
Collaborator
|
Approved, and I pushed one follow-up commit to this branch (b425813) rather than filing it separately — everything in it came out of reviewing your changes, so it belongs with them. All of it is polish on top of what you built; nothing here reverses a decision you made. The template is a backend asset now. It was generated in the page and pushed out through a synthetic A side effect worth having: the template answers to Color validation is an allowlist, split per consumer. Terminal values are stricter: hex only. xterm parses hex directly, and everything else goes through a canvas round-trip that throws on translucency — into a A theme id can't be a Windows device name. The id regex is gone from the frontend. This one is worth explaining, because your reasoning for adding Select triggers showed the raw value. Separate bug, and the one that prompted the rest: with Heads up: Smaller things: the documented example in Local gate on the merged result: Thanks for the back and forth on this one — the |
This was referenced Aug 3, 2026
Merged
omartelo
added a commit
that referenced
this pull request
Aug 4, 2026
* chore: cut v0.24.0 Six pull requests since v0.23.0: custom color themes, a Help section that hands over the log and the bug form, the last five closed projects in the tab strip, the administrator bypass for a merge a rule is holding, and the fork-session and review-snippet fixes. Minor rather than patch: four of the six add something a user did not have. Also moves the custom themes entry out of the v0.23.0 section, where it did not belong. #131 resolved a CHANGELOG conflict against upstream/main by writing its entry under the heading the merge brought back rather than under [Unreleased], so the file claimed a release that shipped on July 30 carried a feature that landed today. The published v0.23.0 notes are unaffected — the release workflow read that section at tag time, before the entry existed — but cutting from the file as it stood would have shipped this release without its largest feature in the notes. The template entry folds into the themes entry rather than standing beside it: a reader meeting theme import for the first time in these notes has no use for a template as a separate feature. Blank lines between bullets go, so the section matches every other one in the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rDoe8TE7cXPiUjtPaMiUE * chore: date v0.24.0 to the day it ships --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR turns lich's light/dark theme handling into a data-driven theme system.
The app still ships the bundled Light and Dark palettes, but it can now import
custom JSON themes and apply them to both the React UI tokens and the xterm
terminal palette.
What changed
themesRPC service backed by the lich config directory:<config-dir>/lich/themes<config-dir>/lich/themes-devvalidated, saved as JSON files, listed, and removable.
id,name,scheme,originapptoken colors for the existing Tailwind/CSS-variable surfaceterminalcolors for xterm, including optional ANSI colorssystem/light/darkenums:systemstill follows the OS scheme.darkis still toggled from the resolved theme scheme so existing darkvariants keep working
theme
TerminalViewso xterm consumes the resolved theme's terminal paletteinstead of the previous hard-coded Light/Dark color table.
first paint CSS defaults in place until React applies the selected theme.
frontend/DESIGN.mdto clarify that the zinc/no-accent palette is thedefault bundled design, not a hard product limit anymore.
Validation and failure behavior
.,_, and-, with reserved ids likelight,dark,system, andmatchrejected.background/foreground.produce partial themes.
instead of blocking app startup.
Light/System path rather than breaking launch.
Testing
gofmt -l .go vet ./...go test ./...cd frontend && pnpm testcd frontend && pnpm checkcd frontend && pnpm buildpnpm checkpassed with existing Biome/a11y warnings unrelated to this PR.