-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add review panel git, branch, and turn diff modes #344
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.
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
Changes from 1 commit
f76692d
75d6c5b
4a65d52
6e26826
4b73df9
6630c14
b3234eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,8 @@ import * as Diff from "diff"; | |
| import type { FileDiff, FileNode } from "../sdk/client"; | ||
| import { useSDK } from "../context/sdk"; | ||
| import { useEvents } from "../context/events"; | ||
| import { useLayout } from "../context/layout"; | ||
| import { useLayout, type ReviewMode } from "../context/layout"; | ||
| import { useSync } from "../context/sync"; | ||
|
|
||
| import { FileTree } from "./file-tree"; | ||
| import { FileViewer } from "./file-viewer"; | ||
|
|
@@ -41,17 +42,39 @@ interface ReviewPanelProps { | |
| sessionId: string; | ||
| } | ||
|
|
||
| const REVIEW_MODES: Array<{ value: ReviewMode; label: string }> = [ | ||
| { value: "session", label: "Session" }, | ||
| { value: "git", label: "Git" }, | ||
| { value: "branch", label: "Branch" }, | ||
| { value: "turn", label: "Turn" }, | ||
| ]; | ||
|
|
||
| function lastUserMessageID( | ||
| messages: Array<{ info: { id: string; role: string } }>, | ||
| ) { | ||
| for (let i = messages.length - 1; i >= 0; i--) { | ||
| if (messages[i].info.role === "user") return messages[i].info.id; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| export function ReviewPanel(props: ReviewPanelProps) { | ||
| const { client, directory } = useSDK(); | ||
| const events = useEvents(); | ||
| const layout = useLayout(); | ||
| const sync = useSync(); | ||
|
|
||
| const [diffs, setDiffs] = createSignal<FileDiff[]>([]); | ||
| const [selected, setSelected] = createSignal<string | null>(null); | ||
| const [loading, setLoading] = createSignal(false); | ||
| const [tab, setTab] = createSignal<"changes" | "all">("changes"); | ||
| const [isGitRepo, setIsGitRepo] = createSignal<boolean | null>(null); // null = unknown | ||
|
|
||
| const mode = () => layout.review.mode(); | ||
| const lastUserMessageId = createMemo(() => | ||
| lastUserMessageID(sync.messages(props.sessionId)), | ||
| ); | ||
|
|
||
| // Track the latest request to prevent race conditions | ||
| let version = 0; | ||
|
|
||
|
|
@@ -65,39 +88,82 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| } | ||
| } | ||
|
|
||
| function setFiles(files: FileDiff[]) { | ||
| setDiffs(files); | ||
| const paths = files.map((d) => d.file); | ||
| const sel = selected(); | ||
| if (!sel || !paths.includes(sel)) { | ||
| setSelected(files.length > 0 ? files[0].file : null); | ||
| } | ||
| } | ||
|
|
||
| async function loadDiffs() { | ||
| const current = ++version; | ||
| const currentMode = mode(); | ||
| setLoading(true); | ||
| try { | ||
| if (currentMode === "git" || currentMode === "branch") { | ||
| const res = await client.vcs.diff({ directory, mode: currentMode }); | ||
|
Comment on lines
119
to
+126
|
||
| if (current !== version) return; | ||
|
Comment on lines
+125
to
+127
|
||
| if (res.data) { | ||
| setFiles(res.data); | ||
| setIsGitRepo(true); | ||
| return; | ||
| } | ||
| setFiles([]); | ||
| await checkGitRepo(); | ||
| return; | ||
| } | ||
|
|
||
| if (currentMode === "turn") { | ||
| const messageID = lastUserMessageId(); | ||
| if (!messageID) { | ||
| if (current !== version) return; | ||
| setFiles([]); | ||
| setIsGitRepo(true); | ||
| return; | ||
| } | ||
| const res = await client.session.diff({ | ||
| sessionID: props.sessionId, | ||
| directory, | ||
| messageID, | ||
| }); | ||
| if (current !== version) return; | ||
| if (res.data) { | ||
| setFiles(res.data); | ||
| setIsGitRepo(true); | ||
| return; | ||
| } | ||
| setFiles([]); | ||
| await checkGitRepo(); | ||
| return; | ||
| } | ||
|
|
||
| const res = await client.session.diff({ sessionID: props.sessionId, directory }); | ||
| // Only update state if this is still the latest request | ||
| if (current !== version) return; | ||
| if (res.data) { | ||
| setDiffs(res.data); | ||
| setIsGitRepo(true); // If we got diffs, it's definitely a git repo | ||
| // Auto-select first file if none selected or selection no longer exists | ||
| const files = res.data.map((d) => d.file); | ||
| const sel = selected(); | ||
| if (!sel || !files.includes(sel)) { | ||
| setSelected(res.data.length > 0 ? res.data[0].file : null); | ||
| } | ||
| } else { | ||
| // No diffs returned - check if it's because no git repo | ||
| await checkGitRepo(); | ||
| setFiles(res.data); | ||
| setIsGitRepo(true); | ||
| return; | ||
| } | ||
| setFiles([]); | ||
| await checkGitRepo(); | ||
| } catch (e) { | ||
| if (current !== version) return; | ||
| console.error("[ReviewPanel] Failed to load diffs:", e); | ||
| // Check if it's a git repo issue | ||
| setFiles([]); | ||
| await checkGitRepo(); | ||
| } finally { | ||
| if (current === version) setLoading(false); | ||
| } | ||
| } | ||
|
|
||
| // Load diffs when session changes | ||
| // Load diffs when session, mode, or relevant turn changes | ||
| createEffect(() => { | ||
| const id = props.sessionId; | ||
| const currentMode = mode(); | ||
| if (currentMode === "turn") lastUserMessageId(); | ||
| if (id) { | ||
| // Reset selection when session changes | ||
| setSelected(null); | ||
|
|
@@ -119,16 +185,9 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| sessionID?: string; | ||
| diff?: FileDiff[]; | ||
| }; | ||
| if (eventProps.sessionID === id && eventProps.diff) { | ||
| setDiffs(eventProps.diff); | ||
| // Auto-select first file if none selected or selection no longer exists | ||
| const files = eventProps.diff.map((d) => d.file); | ||
| const sel = selected(); | ||
| if (!sel || !files.includes(sel)) { | ||
| setSelected( | ||
| eventProps.diff.length > 0 ? eventProps.diff[0].file : null, | ||
| ); | ||
| } | ||
| if (mode() === "session" && eventProps.sessionID === id && eventProps.diff) { | ||
| setFiles(eventProps.diff); | ||
| setIsGitRepo(true); | ||
| } | ||
| } | ||
| // Reload on session status idle to catch completed changes | ||
|
|
@@ -142,7 +201,7 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| } | ||
| } | ||
| if (event.type === "vcs.branch.updated") { | ||
| loadDiffs(); | ||
| if (mode() === "git" || mode() === "branch") loadDiffs(); | ||
| } | ||
| }); | ||
|
|
||
|
|
@@ -195,6 +254,13 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| }); | ||
|
|
||
| const count = createMemo(() => diffs().length); | ||
| const isGitMode = createMemo(() => mode() === "git" || mode() === "branch"); | ||
| const emptyText = createMemo(() => { | ||
| if (mode() === "git") return "No uncommitted changes"; | ||
| if (mode() === "branch") return "No branch changes"; | ||
| if (mode() === "turn") return "No changes in the last turn"; | ||
| return "No changes in this session"; | ||
| }); | ||
|
|
||
| // List of changed file paths for filtering | ||
| const diffFiles = createMemo(() => diffs().map((d) => d.file)); | ||
|
|
@@ -350,7 +416,7 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| </button> | ||
| </div> | ||
|
|
||
| {/* Tabs for Changed Files / All Files */} | ||
| {/* Review mode selector + Tabs for Changed Files / All Files */} | ||
| <Tabs | ||
| variant="pill" | ||
| value={tab()} | ||
|
|
@@ -370,6 +436,31 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| class="px-2 py-2" | ||
| style={{ "border-bottom": "1px solid var(--border-base)" }} | ||
| > | ||
| <div class="flex gap-1 mb-2"> | ||
| <For each={REVIEW_MODES}> | ||
| {(item) => ( | ||
| <button | ||
| type="button" | ||
| onClick={() => layout.review.setMode(item.value)} | ||
| class="flex-1 px-2 py-1 text-xs rounded transition-colors" | ||
| style={{ | ||
| background: | ||
| mode() === item.value | ||
| ? "var(--interactive-base)" | ||
| : "var(--surface-base)", | ||
| color: | ||
| mode() === item.value | ||
| ? "var(--text-on-interactive)" | ||
| : "var(--text-weak)", | ||
| border: "1px solid var(--border-base)", | ||
| }} | ||
| title={`Show ${item.label.toLowerCase()} changes`} | ||
| > | ||
| {item.label} | ||
| </button> | ||
| )} | ||
| </For> | ||
| </div> | ||
| <Tabs.List class="flex gap-1"> | ||
| <Tabs.Trigger | ||
| value="changes" | ||
|
|
@@ -418,7 +509,7 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| style={{ color: "var(--text-weak)" }} | ||
| > | ||
| <Show | ||
| when={isGitRepo() === true} | ||
| when={!isGitMode() || isGitRepo() !== false} | ||
| fallback={ | ||
| <div class="flex flex-col items-center gap-2"> | ||
| <GitBranch | ||
|
|
@@ -435,7 +526,7 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| </div> | ||
| } | ||
| > | ||
| <span>No changes in this session</span> | ||
| <span>{emptyText()}</span> | ||
| </Show> | ||
| </div> | ||
| } | ||
|
|
@@ -469,7 +560,7 @@ export function ReviewPanel(props: ReviewPanelProps) { | |
| > | ||
| {count() > 0 | ||
| ? "Select a file to view changes" | ||
| : "No changes in this session"} | ||
| : emptyText()} | ||
| </span> | ||
| </div> | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3014,6 +3014,36 @@ export class Vcs extends HeyApiClient { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...params, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Get VCS diff | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Get git or branch diff for the current project directory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public diff<ThrowOnError extends boolean = false>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parameters?: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| directory?: string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mode?: "git" | "branch" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| options?: Options<never, ThrowOnError>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const params = buildClientParams( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [parameters], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args: [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { in: "query", key: "directory" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { in: "query", key: "mode" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return (options?.client ?? this.client).get<SessionDiffResponses, unknown, ThrowOnError>({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| url: "/vcs/diff", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...options, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...params, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3018
to
+3047
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | |
| * Get VCS diff | |
| * | |
| * Get git or branch diff for the current project directory. | |
| */ | |
| public diff<ThrowOnError extends boolean = false>( | |
| parameters?: { | |
| directory?: string | |
| mode?: "git" | "branch" | |
| }, | |
| options?: Options<never, ThrowOnError>, | |
| ) { | |
| const params = buildClientParams( | |
| [parameters], | |
| [ | |
| { | |
| args: [ | |
| { in: "query", key: "directory" }, | |
| { in: "query", key: "mode" }, | |
| ], | |
| }, | |
| ], | |
| ) | |
| return (options?.client ?? this.client).get<SessionDiffResponses, unknown, ThrowOnError>({ | |
| url: "/vcs/diff", | |
| ...options, | |
| ...params, | |
| }) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkGitRepo()treats any failure ofclient.vcs.get()as “not a Git repository”. In git/branch mode this can incorrectly show the “Not a Git repository” empty state for transient/network/auth/server errors. Consider callingclient.vcs.getwiththrowOnError: false(or using a non-throwing client) and only settingisGitRepo(false)for the specific status/error that indicates “not a git repo”; otherwise keepisGitRepo(null)and surface a generic load error state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in b3234eb. I switched
checkGitRepo()to use a non-throwingclient.vcs.get({ directory }, { throwOnError: false })call, classify explicit not-a-git errors via message matching, and only setisGitRepo(false)for that case. For transient/auth/server/network failures, it now keepsisGitRepo(null)so the panel does not incorrectly render the "Not a Git repository" empty state.