From a074fd7777ed67e17accccc2a569bee99c7a9dc2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 11:46:06 +0000 Subject: [PATCH 1/7] test: measure coverage against the logic the suite can reach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The coverage config's comment claimed the .tsx components stayed out of the denominator; they did not. `include` only decides which unloaded files are added to the report — a file a test imports is measured whether or not it matches, so every component pulled in transitively (providers/settings.tsx, ContextRing.tsx, the two CodeMirror hooks) was counted as uncovered, along with the React hooks no node-environment test can render and the OS/framework boundaries invariant #1 exempts. The number that reached review was 61%, against an 80% bar, for a suite whose actual subject sits at 95%. Exclude what the suite cannot reach so the summary means what invariant #1 says it means. Also drop components/ui/card.tsx: a stock shadcn primitive, never imported since it landed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RhBpWr7np1JiiiQhfaa4dV --- frontend/src/components/ui/card.tsx | 88 ----------------------------- frontend/vitest.config.ts | 36 ++++++++++-- 2 files changed, 31 insertions(+), 93 deletions(-) delete mode 100644 frontend/src/components/ui/card.tsx diff --git a/frontend/src/components/ui/card.tsx b/frontend/src/components/ui/card.tsx deleted file mode 100644 index 73234c87d..000000000 --- a/frontend/src/components/ui/card.tsx +++ /dev/null @@ -1,88 +0,0 @@ -import * as React from "react" - -import { cn } from "@/lib/utils" - -function Card({ - className, - size = "default", - ...props -}: React.ComponentProps<"div"> & { size?: "default" | "sm" }) { - return ( -
img:first-child]:pt-0 data-[size=sm]:[--card-spacing:--spacing(4)] *:[img:first-child]:rounded-t-xl *:[img:last-child]:rounded-b-xl", - className, - )} - {...props} - /> - ) -} - -function CardHeader({ className, ...props }: React.ComponentProps<"div">) { - return ( -
- ) -} - -function CardTitle({ className, ...props }: React.ComponentProps<"div">) { - return ( -
- ) -} - -function CardDescription({ className, ...props }: React.ComponentProps<"div">) { - return ( -
- ) -} - -function CardAction({ className, ...props }: React.ComponentProps<"div">) { - return ( -
- ) -} - -function CardContent({ className, ...props }: React.ComponentProps<"div">) { - return ( -
- ) -} - -function CardFooter({ className, ...props }: React.ComponentProps<"div">) { - return ( -
- ) -} - -export { Card, CardHeader, CardFooter, CardTitle, CardAction, CardDescription, CardContent } diff --git a/frontend/vitest.config.ts b/frontend/vitest.config.ts index daa419317..372065433 100644 --- a/frontend/vitest.config.ts +++ b/frontend/vitest.config.ts @@ -17,12 +17,38 @@ export default defineConfig({ provider: "v8", reporter: ["text-summary", "json-summary"], reportsDirectory: "coverage", - // The suite owns the pure logic layer (`.ts`: lib, hooks, stores); the - // `.tsx` components are the DOM/xterm boundary invariant #1 exempts, so - // they stay out of the denominator — including them would report a - // misleadingly low number against a suite that never targeted them. + // The denominator is the logic this suite can actually target: pure + // modules under `lib` — stores, parsers, reducers, gates. Everything the + // node environment cannot reach is excluded rather than counted as + // uncovered, because the number is read in review (invariant #1) and a + // denominator full of unreachable files reports a failing bar against a + // suite that is doing its job. + // + // `include` alone does not do this: a file loaded by a test is measured + // whether or not it matches, which is how the `.tsx` the comment here + // once claimed to exclude ended up in the report anyway. The exclusions + // below are what actually holds the line. include: ["src/**/*.ts"], - exclude: ["src/**/*.test.ts", "src/**/*.d.ts"], + exclude: [ + "src/**/*.test.ts", + "src/**/*.d.ts", + // Components: the DOM/xterm boundary invariant #1 exempts. Pulled in + // transitively by the tests that import their pure helpers. + "src/**/*.tsx", + // React hooks. Rendering one needs a renderer, and there is no jsdom + // here by design — the store behind each hook is what carries the + // tests. `useDiffEditor`/`useFileEditor` are hooks too, misfiled under + // components because they own a CodeMirror view. + "src/**/use-*.ts", + "src/components/**/use*.ts", + // OS/framework boundaries: fetch, EventSource, WebSocket, CodeMirror + // wiring, and the DEV-only paint reporter that production drops. + "src/lib/rpc.ts", + "src/lib/app-events.ts", + "src/lib/codemirror.ts", + "src/lib/terminal/term-transport.ts", + "src/lib/terminal/term-perf.ts", + ], }, }, }) From ae8ffab9f561d748bfa44626c02d624755d1eeaa Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 11:51:00 +0000 Subject: [PATCH 2/7] refactor(frontend): one lookup skeleton, one CodeMirror view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five hooks were each rebuilding the same sequence-guarded fetch — a ref counter so a late answer cannot land on a newer request, the loading/error pair, the focus listener — and the copies had drifted: the diff hook skipped focus, the checkouts hook swallowed its error, and only the list blanked its rows when the request behind them changed. useRemoteResource owns the skeleton and makes each of those a named option instead of an omission. Its key is the contract: it stands for the whole request, because `load` is read through a ref so callers can pass an inline closure, and a key missing an argument is a hook that answers with the previous request's data. useDiffEditor and useFileEditor were the same file twice, differing in their extension list and in what a selected doc line means afterwards. The view lifecycle moves to useCodeMirrorView; the two keep their names and their distinct selection contracts as thin wrappers. No behaviour change intended: same requests, same invalidation, same empty and error states. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RhBpWr7np1JiiiQhfaa4dV --- frontend/src/components/diff/useDiffEditor.ts | 61 +++------- frontend/src/components/dock/useFileEditor.ts | 52 ++------ frontend/src/lib/git/use-checkouts.ts | 40 ++---- .../src/lib/pulls/use-pull-request-detail.ts | Bin 3158 -> 2284 bytes .../src/lib/pulls/use-pull-request-diff.ts | 43 ++----- frontend/src/lib/pulls/use-pull-request.ts | 47 +++---- frontend/src/lib/pulls/use-pull-requests.ts | 72 +++-------- frontend/src/lib/use-codemirror-view.ts | 61 ++++++++++ frontend/src/lib/use-remote-resource.ts | 115 ++++++++++++++++++ 9 files changed, 259 insertions(+), 232 deletions(-) create mode 100644 frontend/src/lib/use-codemirror-view.ts create mode 100644 frontend/src/lib/use-remote-resource.ts diff --git a/frontend/src/components/diff/useDiffEditor.ts b/frontend/src/components/diff/useDiffEditor.ts index 6b7fe9ece..f9a967d98 100644 --- a/frontend/src/components/diff/useDiffEditor.ts +++ b/frontend/src/components/diff/useDiffEditor.ts @@ -1,15 +1,7 @@ -import { useEffect, useRef } from "react" +import { useMemo } from "react" import type { RefObject } from "react" -import { EditorState } from "@codemirror/state" -import { EditorView } from "@codemirror/view" -import { - buildLineDecorations, - diffGutter, - loadLanguage, - readOnlyCodeExtensions, - selectedDocLines, - type DocLineSelection, -} from "@/lib/codemirror" +import { buildLineDecorations, diffGutter, type DocLineSelection } from "@/lib/codemirror" +import { useCodeMirrorView } from "@/lib/use-codemirror-view" import type { FileDoc } from "@/lib/git/diff" export interface DiffEditor { @@ -18,38 +10,19 @@ export interface DiffEditor { getSelectedDocLines: () => DocLineSelection | null } -// useDiffEditor owns one read-only CodeMirror view: created when the container -// mounts, destroyed on unmount or when the doc is replaced by a refresh. +// useDiffEditor is the diff card's read-only CodeMirror view: the shared editor +// 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. export function useDiffEditor(doc: FileDoc, filename: string): DiffEditor { - const containerRef = useRef(null) - const viewRef = useRef(null) - - useEffect(() => { - const parent = containerRef.current - if (!parent) { - return - } - const view = new EditorView({ - state: EditorState.create({ - doc: doc.text, - extensions: [ - ...readOnlyCodeExtensions(), - diffGutter(doc.lineMeta), - buildLineDecorations(doc.lineMeta), - ], - }), - parent, - }) - viewRef.current = view - loadLanguage(view, filename, () => viewRef.current === view) - return () => { - viewRef.current = null - view.destroy() - } - }, [doc, filename]) - - const getSelectedDocLines = (): DocLineSelection | null => - viewRef.current ? selectedDocLines(viewRef.current) : null - - return { containerRef, getSelectedDocLines } + const source = useMemo( + () => ({ + text: doc.text, + extensions: [diffGutter(doc.lineMeta), buildLineDecorations(doc.lineMeta)], + }), + [doc], + ) + const { containerRef, getSelectedLines } = useCodeMirrorView(source, filename) + return { containerRef, getSelectedDocLines: getSelectedLines } } diff --git a/frontend/src/components/dock/useFileEditor.ts b/frontend/src/components/dock/useFileEditor.ts index b7fad282e..b25c0cc5d 100644 --- a/frontend/src/components/dock/useFileEditor.ts +++ b/frontend/src/components/dock/useFileEditor.ts @@ -1,13 +1,8 @@ -import { useEffect, useRef } from "react" +import { useMemo } from "react" import type { RefObject } from "react" -import { EditorState } from "@codemirror/state" -import { EditorView, lineNumbers } from "@codemirror/view" -import { - loadLanguage, - readOnlyCodeExtensions, - selectedDocLines, - type DocLineSelection, -} from "@/lib/codemirror" +import { lineNumbers } from "@codemirror/view" +import type { DocLineSelection } from "@/lib/codemirror" +import { useCodeMirrorView } from "@/lib/use-codemirror-view" export interface FileEditor { containerRef: RefObject @@ -15,37 +10,12 @@ export interface FileEditor { getSelectedLines: () => DocLineSelection | null } -// useFileEditor owns one read-only CodeMirror view over a whole file's text — -// the file-browser preview. Unlike the diff viewer there is no interleaving of -// added/deleted lines, so the plain line gutter is identity: doc line N is -// file line N, and a selection maps straight to file line numbers with no -// remap. Created on mount, destroyed on unmount or when the text is replaced. +// useFileEditor is the file-browser preview's read-only CodeMirror view: the +// shared editor plus a plain line gutter. Unlike the diff beside it there is no +// interleaving of added and deleted lines, so that gutter is identity — doc +// line N is file line N, and a selection maps straight to file line numbers +// with no remap. export function useFileEditor(text: string, filename: string): FileEditor { - const containerRef = useRef(null) - const viewRef = useRef(null) - - useEffect(() => { - const parent = containerRef.current - if (!parent) { - return - } - const view = new EditorView({ - state: EditorState.create({ - doc: text, - extensions: [...readOnlyCodeExtensions(), lineNumbers()], - }), - parent, - }) - viewRef.current = view - loadLanguage(view, filename, () => viewRef.current === view) - return () => { - viewRef.current = null - view.destroy() - } - }, [text, filename]) - - const getSelectedLines = (): DocLineSelection | null => - viewRef.current ? selectedDocLines(viewRef.current) : null - - return { containerRef, getSelectedLines } + const source = useMemo(() => ({ text, extensions: [lineNumbers()] }), [text]) + return useCodeMirrorView(source, filename) } diff --git a/frontend/src/lib/git/use-checkouts.ts b/frontend/src/lib/git/use-checkouts.ts index 2d1427f26..ee1c4d5a3 100644 --- a/frontend/src/lib/git/use-checkouts.ts +++ b/frontend/src/lib/git/use-checkouts.ts @@ -1,6 +1,8 @@ -import { useCallback, useEffect, useRef, useState } from "react" import { ProjectService } from "@/lib/rpc" import type { Worktree } from "@/lib/api-types" +import { useRemoteResource } from "@/lib/use-remote-resource" + +const NO_CHECKOUTS: Worktree[] = [] // useCheckouts lists every checkout of the project that holds a branch — its // own directory as much as its linked worktrees. It answers the question the @@ -13,37 +15,15 @@ import type { Worktree } from "@/lib/api-types" // — a worktree can be created or a branch switched from a terminal — and // through refresh() right after this screen creates one itself. An error yields // an empty list: the worst it costs is a checkout attempt that gh refuses with -// its own message. +// its own message, which is why the error itself is dropped here. export function useCheckouts(projectPath: string): { checkouts: Worktree[] refresh: () => void } { - const [checkouts, setCheckouts] = useState([]) - const seq = useRef(0) - - const refresh = useCallback(() => { - if (!projectPath) { - setCheckouts([]) - return - } - const mine = ++seq.current - ProjectService.ListCheckouts(projectPath) - .then((list) => { - if (mine === seq.current) setCheckouts(list ?? []) - }) - .catch(() => { - if (mine === seq.current) setCheckouts([]) - }) - }, [projectPath]) - - useEffect(() => { - refresh() - window.addEventListener("focus", refresh) - return () => { - seq.current++ - window.removeEventListener("focus", refresh) - } - }, [refresh]) - - return { checkouts, refresh } + const { data, refresh } = useRemoteResource( + projectPath, + () => ProjectService.ListCheckouts(projectPath).then((list) => list ?? NO_CHECKOUTS), + { empty: NO_CHECKOUTS, refetchOnFocus: true }, + ) + return { checkouts: data, refresh } } diff --git a/frontend/src/lib/pulls/use-pull-request-detail.ts b/frontend/src/lib/pulls/use-pull-request-detail.ts index 50e783c2f173b3a9447e07522921600ed0bfcc11..3e7bdf6fb18bab5f3d3403ce7a4e29e407e65f90 100644 GIT binary patch delta 470 zcmY*W%SyvQ6!ifXR1jT>3pt3CD$yWyA%%*96$DWbH?G=ga$`c1nKCo6lo0w2k}nZI z!@Zy2O8f#B-Xx&7nK_qx=H7G8yl;N4Z$!yhX^RZ0pXP=MqHWzo4D9fi&srCpt z0<9A4oVjr{bh&?^D*|6N-}4WPGuOpX z3+aUr(nmpBA^ sV>|8q1z4>n>xH-SQhBLi?>gnrDEUQy`D>Bo7I!NjOPxx2ak9Go1LsYiiU0rr delta 1384 zcmbtU&1w`u5JrO$_9A)`JXjf!%&_CGUd4@q<{$)=D0m1t^v={|n#^`@x_dSWaT%XL z2t@rCOd$&;yirj**WDvPj zdnqXG_&;)`v@kS6WhIofp)`yM4VV^a)T01_-TW-k+iK$kevKy>Gias}{Jn-=j7?T^ zNek-?!T3vn_N2vwRyQh3{3PkjLmDiIVc8llY8l5#1A`4XAvXY7K)f~4WENvCR|p=F;(ap z)M{VpL-qe4F1pyKSm^89fJcebH3>HeFGk~6uaJty@|fy)jShpWI$TnBsZ2iRG=&{* z0P692G(@xO8z{_slewilp`+p!%%N1dKJ1A+-<(h-x1@Eb(8N)pv)V=}6~H%O>)kd5 zEW@S|exoL$k~!TZAk;BP^|+|yUg>x$8y-uVYXKG8ss3ZSIuJ6U*2zJr9JJyQ4&qQa z#kWv9p?&5B+JGn=8mgS1e;!d@ms+LU4SRu+!I7RIk$Uk@R@~n#VW66Z)G3s7hwRRN Wul~B_ueRCQ?ep2%>VuWewDSkGv9(nI diff --git a/frontend/src/lib/pulls/use-pull-request-diff.ts b/frontend/src/lib/pulls/use-pull-request-diff.ts index 1a578f276..e2c1076b4 100644 --- a/frontend/src/lib/pulls/use-pull-request-diff.ts +++ b/frontend/src/lib/pulls/use-pull-request-diff.ts @@ -1,7 +1,8 @@ -import { useCallback, useEffect, useRef, useState } from "react" import { ProjectService } from "@/lib/rpc" import { parseDiff, type DiffFile } from "@/lib/git/diff" -import { errorText } from "@/lib/utils" +import { useRemoteResource } from "@/lib/use-remote-resource" + +const NO_FILES: DiffFile[] | null = null export interface PullRequestDiffState { files: DiffFile[] | null @@ -19,36 +20,10 @@ export function usePullRequestDiff( head: string, number: number, ): PullRequestDiffState { - const [files, setFiles] = useState(null) - const [error, setError] = useState(null) - const seq = useRef(0) - - const refresh = useCallback(() => { - if (!path) { - setFiles(null) - setError(null) - return - } - const mine = ++seq.current - ProjectService.PullRequestDiff(path, number) - .then((text) => { - if (mine !== seq.current) return - setFiles(parseDiff(text)) - setError(null) - }) - .catch((err: unknown) => { - if (mine !== seq.current) return - setFiles(null) - setError(errorText(err)) - }) - }, [path, number]) - - useEffect(() => { - refresh() - return () => { - seq.current++ - } - }, [refresh, head]) - - return { files, error } + const { data, error } = useRemoteResource( + path && `${path} ${number} ${head}`, + () => ProjectService.PullRequestDiff(path, number).then(parseDiff), + { empty: NO_FILES }, + ) + return { files: data, error } } diff --git a/frontend/src/lib/pulls/use-pull-request.ts b/frontend/src/lib/pulls/use-pull-request.ts index c96653794..3a254b6dc 100644 --- a/frontend/src/lib/pulls/use-pull-request.ts +++ b/frontend/src/lib/pulls/use-pull-request.ts @@ -1,9 +1,12 @@ -import { useEffect, useState } from "react" +import { useEffect } from "react" import { lookupPullRequest, onPullRequestInvalidated } from "./pull-request-lookup" import type { PullRequest } from "@/lib/api-types" +import { useRemoteResource } from "@/lib/use-remote-resource" export type { PullRequest } +const NO_PULL_REQUEST: PullRequest | null = null + // usePullRequest resolves the open GitHub PR for a path's current branch via the // gh CLI. Unlike git status it is not polled — each lookup is a network // round-trip — but it refetches whenever the checkout's HEAD moves, so a PR the @@ -15,34 +18,20 @@ export type { PullRequest } // on any error, or when the branch has no open PR (a merged or closed one is // filtered server-side), so the caller hides the badge. export function usePullRequest(path: string, branch: string, head: string): PullRequest | null { - const [pr, setPr] = useState(null) - - // A different checkout is a different PR: drop the old badge at once rather - // than showing it against the new branch. A new commit on the same branch - // keeps it, so the badge does not blink on every commit. - useEffect(() => { - setPr(null) - }, [path, branch]) + const { data, refresh } = useRemoteResource( + path && `${path} ${branch} ${head}`, + () => lookupPullRequest(path, branch, head), + { + empty: NO_PULL_REQUEST, + refetchOnFocus: true, + // A different checkout is a different PR: drop the old badge at once + // rather than showing it against the new branch. A new commit on the same + // branch keeps it, so the badge does not blink on every commit. + resetOn: `${path} ${branch}`, + }, + ) - useEffect(() => { - if (!path) { - return - } - let alive = true - const load = () => { - void lookupPullRequest(path, branch, head).then((result) => { - if (alive) setPr(result) - }) - } - load() - window.addEventListener("focus", load) - const unsubscribe = onPullRequestInvalidated(load) - return () => { - alive = false - window.removeEventListener("focus", load) - unsubscribe() - } - }, [path, branch, head]) + useEffect(() => onPullRequestInvalidated(refresh), [refresh]) - return pr + return data } diff --git a/frontend/src/lib/pulls/use-pull-requests.ts b/frontend/src/lib/pulls/use-pull-requests.ts index 2d7c0113d..4ab786feb 100644 --- a/frontend/src/lib/pulls/use-pull-requests.ts +++ b/frontend/src/lib/pulls/use-pull-requests.ts @@ -1,9 +1,11 @@ -import { useCallback, useEffect, useRef, useState } from "react" +import { useEffect } from "react" import { ProjectService } from "@/lib/rpc" import type { PullRequestSummary } from "@/lib/api-types" import { onPullRequestInvalidated } from "@/lib/pulls/pull-request-lookup" import type { PullsState } from "@/lib/pulls/pull-request-list" -import { errorText } from "@/lib/utils" +import { useRemoteResource } from "@/lib/use-remote-resource" + +const NO_PULL_REQUESTS: PullRequestSummary[] = [] export interface PullRequestListState { list: PullRequestSummary[] @@ -18,58 +20,20 @@ export interface PullRequestListState { // retires the badges (a merge, a PR opened from the terminal). An empty list // with no error is a repository whose pull requests are all closed. export function usePullRequests(path: string, state: PullsState): PullRequestListState { - const [list, setList] = useState([]) - // Starts true so the first paint reads as loading rather than as "no pull - // requests", the same contract as the detail hook. - const [loading, setLoading] = useState(true) - const [error, setError] = useState(null) - const seq = useRef(0) - const loaded = useRef(state) - - const refresh = useCallback(() => { - if (!path) { - setList([]) - setError(null) - setLoading(false) - return - } - const mine = ++seq.current - setLoading(true) - // Rows of the state being left have to go before the new ones arrive: the - // column labels itself from the query, so keeping them would caption open - // pull requests as merged for as long as gh takes to answer. - if (loaded.current !== state) { - loaded.current = state - setList([]) - } - ProjectService.ListPullRequests(path, state) - .then((result) => { - if (mine !== seq.current) return - setList(result ?? []) - setError(null) - }) - .catch((err: unknown) => { - if (mine !== seq.current) return - setList([]) - setError(errorText(err)) - }) - .finally(() => { - if (mine === seq.current) setLoading(false) - }) - }, [path, state]) + const { data, loading, error, refresh } = useRemoteResource( + path && `${path} ${state}`, + () => ProjectService.ListPullRequests(path, state).then((list) => list ?? NO_PULL_REQUESTS), + { + empty: NO_PULL_REQUESTS, + refetchOnFocus: true, + // Rows of the state being left have to go before the new ones arrive: the + // column labels itself from the query, so keeping them would caption open + // pull requests as merged for as long as gh takes to answer. + resetOn: state, + }, + ) - useEffect(() => { - refresh() - window.addEventListener("focus", refresh) - const unsubscribe = onPullRequestInvalidated(refresh) - return () => { - // Invalidate any in-flight call so a late response cannot land on the - // next path or after unmount. - seq.current++ - window.removeEventListener("focus", refresh) - unsubscribe() - } - }, [refresh]) + useEffect(() => onPullRequestInvalidated(refresh), [refresh]) - return { list, loading, error, refresh } + return { list: data, loading, error, refresh } } diff --git a/frontend/src/lib/use-codemirror-view.ts b/frontend/src/lib/use-codemirror-view.ts new file mode 100644 index 000000000..59aee1587 --- /dev/null +++ b/frontend/src/lib/use-codemirror-view.ts @@ -0,0 +1,61 @@ +import { useEffect, useRef } from "react" +import type { RefObject } from "react" +import type { Extension } from "@codemirror/state" +import { EditorState } from "@codemirror/state" +import { EditorView } from "@codemirror/view" +import { loadLanguage, readOnlyCodeExtensions, selectedDocLines } from "@/lib/codemirror" +import type { DocLineSelection } from "@/lib/codemirror" + +// What a view is built from. The two together are the view's identity: a new +// object rebuilds it, so callers hand over a memoised one and the editor +// survives every render that changed neither. +export interface CodeMirrorSource { + text: string + /** Laid on top of the shared read-only base — a diff's gutter and line + * decorations, a plain file's line numbers. */ + extensions: Extension[] +} + +export interface CodeMirrorView { + containerRef: RefObject + /** 1-based doc line span of the current selection, or null when empty. */ + getSelectedLines: () => DocLineSelection | null +} + +// useCodeMirrorView owns one read-only CodeMirror view for its container: +// created on mount, destroyed on unmount or when the source is replaced by a +// refresh. The diff cards and the file-browser preview are the same editor — +// they differ only in the extensions they layer on and in what a selection's +// doc lines mean afterwards, which is the wrapping hook's business, not this +// one's. +export function useCodeMirrorView(source: CodeMirrorSource, filename: string): CodeMirrorView { + const containerRef = useRef(null) + const viewRef = useRef(null) + + useEffect(() => { + const parent = containerRef.current + if (!parent) { + return + } + const view = new EditorView({ + state: EditorState.create({ + doc: source.text, + extensions: [...readOnlyCodeExtensions(), ...source.extensions], + }), + parent, + }) + viewRef.current = view + // Async: the language may still be loading when the view is replaced, so + // the callback checks it is still the live one before touching it. + loadLanguage(view, filename, () => viewRef.current === view) + return () => { + viewRef.current = null + view.destroy() + } + }, [source, filename]) + + const getSelectedLines = (): DocLineSelection | null => + viewRef.current ? selectedDocLines(viewRef.current) : null + + return { containerRef, getSelectedLines } +} diff --git a/frontend/src/lib/use-remote-resource.ts b/frontend/src/lib/use-remote-resource.ts new file mode 100644 index 000000000..071b7dea2 --- /dev/null +++ b/frontend/src/lib/use-remote-resource.ts @@ -0,0 +1,115 @@ +import { useCallback, useEffect, useRef, useState } from "react" +import { errorText } from "@/lib/utils" + +// The read side of a backend lookup: what came back, whether a call is in +// flight, why the last one failed, and the way to run it again. +export interface RemoteResource { + data: T + loading: boolean + error: string | null + refresh: () => void +} + +export interface RemoteResourceOptions { + /** What `data` holds before the first answer, and after a failed or skipped + * lookup. Pass a module-level constant for arrays and objects — a fresh one + * per render would notify subscribers on every failure. */ + empty: T + /** Re-read when the window regains focus. On for anything the user can + * change outside lich (a branch pushed, a review left); off where re-reading + * is expensive enough that an alt-tab must not pay for it. */ + refetchOnFocus?: boolean + /** Blank `data` back to `empty` the moment this value changes, ahead of the + * answer. For a readout that captions itself from the request — the pull + * request column labels its rows with the state it asked for — where holding + * the old rows would mislabel them for as long as the call takes. Off by + * default: a plain refetch keeps what it has, so a moved HEAD refreshes a + * screen in place instead of flashing it back to a skeleton. */ + resetOn?: string +} + +// useRemoteResource is the one lookup skeleton behind the screens' backend +// reads: a sequence-guarded call whose late answer can never land on a newer +// request or on an unmounted component, plus the loading/error pair every +// caller was otherwise rebuilding by hand. +// +// `key` is the contract. It stands for the whole request — every path, number +// and revision the call closes over — because `load` itself is read through a +// ref and is deliberately not a dependency: callers pass an inline closure +// rather than memoising one, and a key that leaves out an argument is a hook +// that answers with the previous request's data. +// +// An empty key is "nothing to look up", not a failure: the lookup is skipped, +// `data` holds `empty` and `loading` is false. That is what an absent path +// means everywhere in the app, and it keeps a screen with no repository from +// spending a round-trip. +export function useRemoteResource( + key: string, + load: () => Promise, + options: RemoteResourceOptions, +): RemoteResource { + const { empty, refetchOnFocus = false, resetOn } = options + const [data, setData] = useState(empty) + // Starts true so the first paint of a real request reads as loading rather + // than as an answered "there is nothing here". + const [loading, setLoading] = useState(key !== "") + const [error, setError] = useState(null) + // Sequence number of the newest request; a reply carrying an older one is + // dropped. The cleanup bumps it, which is also how unmount invalidates. + const seq = useRef(0) + const loadRef = useRef(load) + loadRef.current = load + const emptyRef = useRef(empty) + emptyRef.current = empty + + const refresh = useCallback(() => { + if (!key) { + seq.current++ + setData(emptyRef.current) + setError(null) + setLoading(false) + return + } + const mine = ++seq.current + setLoading(true) + loadRef + .current() + .then((result) => { + if (mine !== seq.current) return + setData(result) + setError(null) + }) + .catch((err: unknown) => { + if (mine !== seq.current) return + setData(emptyRef.current) + setError(errorText(err)) + }) + .finally(() => { + if (mine === seq.current) setLoading(false) + }) + }, [key]) + + // Blanking during render, not in an effect: the old data must never reach + // the screen once the request behind it has been abandoned, and an effect + // would paint it one frame first. + const lastReset = useRef(resetOn) + if (resetOn !== undefined && resetOn !== lastReset.current) { + lastReset.current = resetOn + setData(emptyRef.current) + } + + useEffect(() => { + refresh() + if (refetchOnFocus) { + window.addEventListener("focus", refresh) + } + return () => { + // Invalidate any in-flight call so a late answer cannot land on the next + // request or after unmount. + seq.current++ + window.removeEventListener("focus", refresh) + } + }, [refresh, refetchOnFocus]) + + return { data, loading, error, refresh } +} From 3e51c8be5b2ef8522df81504c1d14b63b1abde38 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 11:54:04 +0000 Subject: [PATCH 3/7] refactor(frontend): split the Pulls screen from the pull request it shows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pulls.tsx held six components in 732 lines: the screen, the pull request in full, its merge dialog, the skeleton, the empty state and a notice. The screen and the view have genuinely separate jobs — which pull request is in view and what an action there means for the checkout it lives in, against the header and the four tabs that read one — and the file gave no seam between them. Each component moves to a file named after it. The merge dialog takes its seed messages with it as mergeEditFor, which is where the "what GitHub would have written" decision belongs; the view keeps the in-flight state of the actions it owns and nothing else. Pulls.tsx: 732 -> 255 lines, and no file in the folder is now above 330. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RhBpWr7np1JiiiQhfaa4dV --- .../components/pulls/MergeMessageDialog.tsx | 105 ++++ .../src/components/pulls/PullRequestView.tsx | 319 +++++++++++ .../src/components/pulls/PullSkeleton.tsx | 40 ++ frontend/src/components/pulls/Pulls.tsx | 497 +----------------- .../src/components/pulls/PullsEmptyState.tsx | 50 ++ 5 files changed, 524 insertions(+), 487 deletions(-) create mode 100644 frontend/src/components/pulls/MergeMessageDialog.tsx create mode 100644 frontend/src/components/pulls/PullRequestView.tsx create mode 100644 frontend/src/components/pulls/PullSkeleton.tsx create mode 100644 frontend/src/components/pulls/PullsEmptyState.tsx diff --git a/frontend/src/components/pulls/MergeMessageDialog.tsx b/frontend/src/components/pulls/MergeMessageDialog.tsx new file mode 100644 index 000000000..653bf333d --- /dev/null +++ b/frontend/src/components/pulls/MergeMessageDialog.tsx @@ -0,0 +1,105 @@ +import { GitMerge } from "lucide-react" +import type { MergeMethod } from "@/lib/api-types" +import { Button } from "@/components/ui/button" +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from "@/components/ui/dialog" +import { Input } from "@/components/ui/input" +import { Label } from "@/components/ui/label" + +// MergeEdit carries a pending "edit commit message" merge: which method to run +// and the message the dialog is editing. The screen holds it as null while the +// dialog is closed. +export interface MergeEdit { + method: MergeMethod + title: string + subject: string + body: string +} + +// mergeEditFor seeds the dialog with what GitHub would have written by itself, +// which is what the user is here to adjust rather than to compose: a squash +// takes the pull request's title and body, a merge commit takes git's own +// "Merge pull request #N from branch" and no body. +export function mergeEditFor( + method: Extract, + detail: { number: number; title: string; body: string; headRefName: string }, +): MergeEdit { + if (method === "squash") { + return { + method, + title: "Squash and merge", + subject: `${detail.title} (#${detail.number})`, + body: detail.body, + } + } + return { + method, + title: "Create a merge commit", + subject: `Merge pull request #${detail.number} from ${detail.headRefName}`, + body: "", + } +} + +interface MergeMessageDialogProps { + edit: MergeEdit + merging: boolean + onChange: (next: MergeEdit) => void + onCancel: () => void + onConfirm: () => void +} + +export function MergeMessageDialog({ + edit, + merging, + onChange, + onCancel, + onConfirm, +}: MergeMessageDialogProps) { + return ( + !next && onCancel()}> + + + {edit.title} + Edit the commit message, then merge. + +
+
+ + onChange({ ...edit, subject: e.target.value })} + autoFocus + /> +
+
+ +