Skip to content

feat(tui): add last turn source to diff viewer #48998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nexxeln wants to merge 1 commit into
base: v2
Choose a base branch
from
+154 −44
Open

feat(tui): add last turn source to diff viewer #48998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/tui/src/config/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ export const Plugin = Schema.Union([
}),
])

/** VCS review scopes plus the last session turn, which only exists while a session is open. */
export const DiffSource = Schema.Union([Vcs.Mode, Schema.Literal("turn")])
export type DiffSource = Schema.Schema.Type<typeof DiffSource>

export const Cursor = Schema.Struct({
style: Schema.optional(Schema.Literals(["block", "underline", "line", "default"])).annotate({
description: "Cursor shape. Use 'default' to preserve the terminal setting",
Expand Down Expand Up @@ -109,8 +113,9 @@ export const Info = Schema.Struct({
).annotate({ description: "System notification and sound settings" }),
diffs: Schema.optional(
Schema.Struct({
source: Schema.optional(Vcs.Mode).annotate({
description: "Initial diff source; defaults to 'branch' (branch and uncommitted changes)",
source: Schema.optional(DiffSource).annotate({
description:
"Initial diff source; defaults to 'branch' (branch and uncommitted changes). 'turn' shows the last session turn and falls back to 'branch' outside a session",
}),
wrap: Schema.optional(Schema.Literals(["word", "none"])).annotate({
description: "Line wrapping behavior in diff output",
Expand Down
68 changes: 49 additions & 19 deletions packages/tui/src/feature-plugins/system/diff-viewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { EmptyBorder } from "../../ui/border"
import { FilePath } from "../../ui/file-path"
import { getScrollAcceleration } from "../../util/scroll"
import { createDebouncedSignal } from "../../util/signal"
import { useConfig } from "../../config"
import { type DiffSource, useConfig } from "../../config"
import { locationKey } from "../../context/data"
import { useThemes } from "../../context/theme"
import { PatchDiff, type PatchDiffRef } from "../../component/patch-diff"
Expand All @@ -44,7 +44,7 @@ const FILE_TREE_MIN_WIDTH = 30
const FILE_TREE_MAX_WIDTH = 40
const FILE_HEADER_HEIGHT = 2
const VCS_DIFF_CONTEXT_LINES = 12
type DiffMode = Vcs.Mode
type DiffMode = DiffSource
type DiffView = "split" | "unified"
type SelectedHunk = { readonly fileIndex: number; readonly hunkIndex: number; readonly scrollTop: number }
type FileMenuState = { readonly fileIndex: number; readonly x: number; readonly y: number }
Expand Down Expand Up @@ -73,9 +73,13 @@ function storedView(value: unknown): DiffView | undefined {
function diffSourceLabel(mode: DiffMode) {
if (mode === "branch") return "All"
if (mode === "committed") return "Committed"
if (mode === "turn") return "Last turn"
return "Uncommitted"
}

/** Branch comparisons resolve a review base; the working copy and the session turn do not. */
const needsBase = (mode: DiffMode) => mode === "branch" || mode === "committed"

function DiffViewer(props: { context: Plugin.Context }) {
const dimensions = useTerminalDimensions()
const config = useConfig()
Expand All @@ -93,12 +97,15 @@ function DiffViewer(props: { context: Plugin.Context }) {
}
| undefined
}
const [mode, setMode] = createSignal(params()?.mode ?? memory.source ?? config.data.diffs?.source ?? "branch")
const sessionID = () => params()?.sessionID
const initialMode = params()?.mode ?? memory.source ?? config.data.diffs?.source ?? "branch"
// The session turn only exists inside a session; a remembered or configured turn source falls back elsewhere.
const [mode, setMode] = createSignal<DiffMode>(initialMode === "turn" && !sessionID() ? "branch" : initialMode)
const location = createMemo(
() => {
const sessionID = params()?.sessionID
return sessionID
? (props.context.data.session.get(sessionID)?.location ?? props.context.data.location.default())
const id = sessionID()
return id
? (props.context.data.session.get(id)?.location ?? props.context.data.location.default())
: props.context.data.location.default()
},
undefined,
Expand All @@ -121,19 +128,30 @@ function DiffViewer(props: { context: Plugin.Context }) {
bases.set(key, pending)
return pending
}
const diffInput = createMemo(() => ({
mode: mode(),
location: location(),
key: baseKey(),
selected: mode() === "working" ? undefined : selectedBase(),
}))
const diffInput = createMemo(() => {
const current = mode()
if (current === "turn") return { sessionID: sessionID() }
return {
mode: current,
location: location(),
key: baseKey(),
selected: needsBase(current) ? selectedBase() : undefined,
}
})
const [diff] = createResource(diffInput, async (input) => {
const base =
input.mode === "working"
? undefined
: input.selected
? { name: input.selected, ref: input.selected }
: (await loadBase(input.location, input.key)).data
if ("sessionID" in input) {
if (!input.sessionID) return { base: null, files: [] }
const files = await props.context.client.session.diff({
sessionID: input.sessionID,
context: VCS_DIFF_CONTEXT_LINES,
})
return { base: null, files: normalizeDiffs(files) }
}
const base = !needsBase(input.mode)
? undefined
: input.selected
? { name: input.selected, ref: input.selected }
: (await loadBase(input.location, input.key)).data
if (input !== diffInput() || (input.mode === "committed" && !base)) {
return { base: null, files: [] }
}
Expand All @@ -151,6 +169,7 @@ function DiffViewer(props: { context: Plugin.Context }) {
}
const result = () => (diff.error || diff.loading ? undefined : diff())
const sourceDetail = () => {
if (mode() === "turn") return diff.error ? "Diff unavailable" : "since your last prompt"
if (mode() === "working") return "vs HEAD"
if (diff.error) return "Base or diff unavailable"
if (!result()) return "Resolving diff…"
Expand All @@ -167,6 +186,7 @@ function DiffViewer(props: { context: Plugin.Context }) {
loading={diff.loading}
error={diff.error}
mode={mode()}
turnSource={!!sessionID()}
sourceDetail={sourceDetail()}
sourceBase={sourceBase()}
unavailable={mode() === "committed" && !!result() && !result()?.base}
Expand Down Expand Up @@ -261,6 +281,8 @@ export function DiffViewerContent(props: {
loading?: boolean
error?: unknown
mode: DiffMode
/** Offer the session turn source; only meaningful when the viewer opened from a session. */
turnSource?: boolean
sourceDetail?: string
sourceBase?: Pick<Vcs.Base, "name" | "ref"> | null
unavailable?: boolean
Expand Down Expand Up @@ -715,6 +737,14 @@ export function DiffViewerContent(props: {
value: "working" as const,
description: "Local changes only",
},
...(props.turnSource
? [
{
value: "turn" as const,
description: "Since your last prompt",
},
]
: []),
]
dialog.show(() => (
<DialogSelect<DiffMode | "base">
Expand Down Expand Up @@ -834,7 +864,7 @@ export function DiffViewerContent(props: {
<Match when={!props.loading && props.error}>
<box flexGrow={1} padding={2}>
<text fg={theme.text.feedback.error.default}>
{!props.sourceBase && mode() !== "working"
{!props.sourceBase && needsBase(mode())
? "Could not load diff. Choose a base branch from Diff source, or select Uncommitted."
: "Could not load diff. Reopen the diff viewer to try again."}
</text>
Expand Down
101 changes: 87 additions & 14 deletions packages/tui/test/cli/tui/diff-viewer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,68 @@ test("explicit route source overrides the configured default", async () => {
}
})

test("the turn source diffs the session without resolving a base", async () => {
const viewer = await renderDiffViewer(hunkDiff, { height: 30, kittyKeyboard: true })
try {
await chooseSource(viewer, 3)
await viewer.app.waitForFrame((frame) => frame.includes("Last turn · since your last prompt"))
expect(viewer.app.captureCharFrame()).toContain("const first")
expect(viewer.turnDiffRequests).toHaveLength(1)
expect(viewer.turnDiffRequests[0].searchParams.get("context")).toBe("12")
expect(viewer.turnDiffRequests[0].searchParams.has("from")).toBe(false)
expect(viewer.diffRequests).toHaveLength(1)
expect(viewer.baseRequests).toHaveLength(1)
await chooseSource(viewer, 2)
await viewer.app.waitForFrame((frame) => frame.includes("Uncommitted · vs HEAD") && frame.includes("const first"))
expect(viewer.vcsDiffInput()).toEqual({
location: { directory: "/repo/session" },
mode: "working",
context: "12",
})
} finally {
viewer.app.renderer.destroy()
}
})

test("an empty or failing turn diff uses the viewer's empty and error states", async () => {
const empty = await renderDiffViewer([], { source: "turn", turnDiffResponse: async () => json({ data: [] }) })
try {
expect(empty.baseRequests).toHaveLength(0)
expect(empty.diffRequests).toHaveLength(0)
expect(empty.turnDiffRequests).toHaveLength(1)
await empty.app.waitForFrame((frame) => frame.includes("No changes to show"))
} finally {
empty.app.renderer.destroy()
}
const failing = await renderDiffViewer([], { source: "turn", fail: true })
try {
await failing.app.waitForFrame((frame) => frame.includes("Could not load diff. Reopen the diff viewer"))
expect(failing.app.captureCharFrame()).toContain("Last turn · Diff unavailable")
expect(failing.app.captureCharFrame()).not.toContain("Choose a base branch")
} finally {
failing.app.renderer.destroy()
}
})

test("the turn source is unavailable outside a session and falls back to the branch scope", async () => {
const viewer = await renderDiffViewer(hunkDiff, { source: "turn", height: 30, initialRoute: { type: "home" } })
try {
expect(viewer.vcsDiffInput()).toEqual({
location: { directory: "/repo/default" },
mode: "branch",
base: "refs/heads/v2",
context: "12",
})
expect(viewer.turnDiffRequests).toHaveLength(0)
viewer.app.mockInput.pressKey("d")
await viewer.app.waitForFrame((frame) => frame.includes("Diff source"))
expect(viewer.app.captureCharFrame()).not.toContain("Last turn")
expect(viewer.app.captureCharFrame()).toMatch(/Base\s+v2/)
} finally {
viewer.app.renderer.destroy()
}
})

test.each([50, 80, 160])(
"keeps scope, base, and review count on one row with a selectable base at %i columns",
async (width) => {
Expand Down Expand Up @@ -240,9 +302,11 @@ test.each([50, 80, 160])(
expect(rows[first]).toMatch(/All\s+Branch \+ local changes/)
expect(rows[first + 1]).toMatch(/Committed\s+Branch commits only/)
expect(rows[first + 2]).toMatch(/Uncommitted\s+Local changes only/)
expect(rows[first + 3]).toMatch(/Base\s+release/)
expect(rows[first + 3]).toMatch(/Last turn\s+Since your last prompt/)
expect(rows[first + 4]).toMatch(/Base\s+release/)
expect(rows[first + 1].indexOf("Branch commits only")).toBe(rows[first].indexOf("Branch + local changes"))
expect(rows[first + 2].indexOf("Local changes only")).toBe(rows[first].indexOf("Branch + local changes"))
expect(rows[first + 3].indexOf("Since your last prompt")).toBe(rows[first].indexOf("Branch + local changes"))
viewer.app.mockInput.pressEscape()
await viewer.app.waitForFrame((frame) => !frame.includes("Diff source"))
viewer.commands.get("diff.mark_reviewed")!.run()
Expand Down Expand Up @@ -292,7 +356,7 @@ test.each([50, 80, 100, 160])(
)

test("opening the source chooser from initial Uncommitted does not resolve a branch base", async () => {
const viewer = await renderDiffViewer(hunkDiff, { source: "working" })
const viewer = await renderDiffViewer(hunkDiff, { source: "working", height: 30 })
try {
viewer.app.mockInput.pressKey("d")
await viewer.app.waitForFrame((frame) => frame.includes("Diff source"))
Expand All @@ -313,7 +377,7 @@ test.each(["branch", "committed", "working"] as const)(
viewer.commands.get("diff.mark_reviewed")!.run()
await viewer.app.flush()
expect(viewer.app.captureCharFrame()).toContain("1/1")
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("Base branch") && frame.includes("origin/release"))
expect(viewer.app.captureCharFrame()).toMatch(/●\s+v2/)
expect(viewer.branchesRequests[0].searchParams.get("location[directory]")).toBe("/repo/session")
Expand Down Expand Up @@ -347,7 +411,7 @@ test.each(["branch", "committed", "working"] as const)(
expect(viewer.app.captureCharFrame()).toContain("0/1")
expect(viewer.diffRequests).toHaveLength(source === "working" ? 2 : 3)
if (source !== "working") expect(viewer.vcsDiffInput()).toMatchObject({ base: "origin/release" })
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => /●\s+origin\/release/.test(frame))
expect(viewer.baseRequests).toHaveLength(1)
} finally {
Expand All @@ -371,7 +435,7 @@ test.each(["branch", "committed"] as const)("an ambiguous base never requests a
expect(viewer.app.captureCharFrame()).toContain("Choose a base branch")
expect(viewer.app.captureCharFrame()).not.toContain("No changes to show")
expect(viewer.diffRequests).toHaveLength(0)
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("Base branch") && frame.includes("origin/release"))
viewer.app.mockInput.pressKey("HOME")
viewer.app.mockInput.pressArrow("down")
Expand All @@ -388,7 +452,7 @@ test.each(["branch", "committed"] as const)("an ambiguous base never requests a
test("base and scope choices survive reopening but not a new TUI instance", async () => {
const viewer = await renderDiffViewer(hunkDiff, { source: "working", height: 30, kittyKeyboard: true })
try {
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("origin/release"))
viewer.app.mockInput.pressArrow("down")
viewer.app.mockInput.pressEnter()
Expand Down Expand Up @@ -421,7 +485,7 @@ test("base and scope choices survive reopening but not a new TUI instance", asyn
test("base choices are isolated by branch within the same location", async () => {
const viewer = await renderDiffViewer(hunkDiff, { height: 30, kittyKeyboard: true })
try {
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("origin/release"))
viewer.app.mockInput.pressArrow("down")
viewer.app.mockInput.pressEnter()
Expand Down Expand Up @@ -449,13 +513,13 @@ test("an invalid comparison reports an error and allows another base choice", as
: json({ location: session.location, data: hunkDiff }),
})
try {
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("origin/release"))
viewer.app.mockInput.pressArrow("down")
viewer.app.mockInput.pressEnter()
await viewer.app.waitForFrame((frame) => frame.includes("Base or diff unavailable"))
expect(viewer.app.captureCharFrame()).not.toContain("No changes to show")
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("origin/release"))
viewer.app.mockInput.pressKey("HOME")
viewer.app.mockInput.pressEnter()
Expand All @@ -474,7 +538,7 @@ test("base search failures are visible without changing the diff", async () => {
branchesResponse: async () => json({ message: "branches unavailable" }, { status: 503 }),
})
try {
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("Could not load branches"))
expect(viewer.app.captureCharFrame()).toContain("All · vs v2")
expect(viewer.mutationRequests).toHaveLength(0)
Expand All @@ -493,7 +557,7 @@ test("a late base lookup cannot overwrite an in-memory base choice", async () =>
baseResponse: () => pending.promise,
})
try {
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("origin/release"))
viewer.app.mockInput.pressArrow("down")
viewer.app.mockInput.pressEnter()
Expand Down Expand Up @@ -522,7 +586,7 @@ test("dismissing the base picker leaves the comparison unchanged", async () => {
kittyKeyboard: true,
})
try {
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("origin/release"))
viewer.app.mockInput.pressArrow("down")
viewer.app.mockInput.pressEscape()
Expand All @@ -540,7 +604,7 @@ test("dismissing the base picker leaves the comparison unchanged", async () => {
test("the base picker remembers its captured location without refreshing a moved session", async () => {
const viewer = await renderDiffViewer(hunkDiff, { height: 30, kittyKeyboard: true })
try {
await chooseSource(viewer, 3)
await chooseSource(viewer, 4)
await viewer.app.waitForFrame((frame) => frame.includes("origin/release"))
viewer.setSessionLocation({ directory: "/repo/moved" })
await viewer.app.flush()
Expand Down Expand Up @@ -1800,12 +1864,13 @@ async function renderDiffViewer(
onSessionTab?: () => void
keybinds?: TuiKeybind.KeybindOverrides
kittyKeyboard?: boolean
source?: "branch" | "committed" | "working"
source?: "branch" | "committed" | "working" | "turn"
base?: typeof baseFixture | null
open?: boolean
pending?: boolean
baseResponse?: () => Promise<Response>
diffResponse?: (url: URL) => Promise<Response>
turnDiffResponse?: (url: URL) => Promise<Response>
branchesResponse?: (url: URL) => Promise<Response>
state?: string
} = {},
Expand All @@ -1826,6 +1891,7 @@ async function renderDiffViewer(
const writes: Info[] = []
const baseRequests: URL[] = []
const diffRequests: URL[] = []
const turnDiffRequests: URL[] = []
const branchesRequests: URL[] = []
const mutationRequests: URL[] = []
const config = createTuiResolvedConfig(stored.info)
Expand Down Expand Up @@ -1858,6 +1924,12 @@ async function renderDiffViewer(
),
})
}
if (url.pathname === "/api/session/session-1/diff") {
turnDiffRequests.push(url)
if (options.turnDiffResponse) return options.turnDiffResponse(url)
if (options.fail) return json({ message: "boom" }, { status: 500 })
return json({ data: vcsDiff })
}
if (url.pathname !== "/api/vcs/diff") return
diffRequests.push(url)
vcsDiffInput = {
Expand Down Expand Up @@ -2003,6 +2075,7 @@ async function renderDiffViewer(
imageReadInput: () => imageReadInput,
baseRequests,
diffRequests,
turnDiffRequests,
branchesRequests,
mutationRequests,
setSessionLocation,
Expand Down
Loading
Loading